Currently, the binding supports specifying the required PHY, PERST#
properties in two ways:
1. Controller node (deprecated)
- phys
- perst-gpios
2. Root Port node
- phys
- reset-gpios
But there is no check to make sure that the both variants are not mixed.
For instance, if the Controller node specifies 'phys', 'reset-gpios',
or if the Root Port node specifies 'phys', 'perst-gpios', then the driver
will fail as reported. Hence, enforce the check in the binding to catch
these issues.
It is also possible that DTs could have 'phys' property in Controller node
and 'reset-gpios' properties in the Root Port node. It will also be a
problem, but it is not possible to catch these cross-node issues in the
binding.
Reported-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
Reported-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Closes: https://lore.kernel.org/linux-pci/8f2e0631-6c59-4298-b36e-060708970ced@oss.qualcomm.com
Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
.../devicetree/bindings/pci/qcom,pcie-common.yaml | 16 ++++++++++++++++
.../devicetree/bindings/pci/qcom,pcie-sc8180x.yaml | 3 +++
2 files changed, 19 insertions(+)
diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml
index ab2509ec1c4b40ac91a93033d1bab1b12c39362f..d56c0dc2ae4d3944294ca50cab708915c9f60ea8 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml
@@ -111,6 +111,14 @@ patternProperties:
phys:
maxItems: 1
+ oneOf:
+ - required:
+ - phys
+ - reset-gpios
+ - properties:
+ phys: false
+ reset-gpios: false
+
unevaluatedProperties: false
required:
@@ -129,6 +137,14 @@ anyOf:
- required:
- msi-map
+oneOf:
+ - required:
+ - phys
+ - perst-gpios
+ - properties:
+ phys: false
+ perst-gpios: false
+
allOf:
- $ref: /schemas/pci/pci-host-bridge.yaml#
diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-sc8180x.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-sc8180x.yaml
index 34a4d7b2c8459aeb615736f54c1971014adb205f..17abc7f7b7e9d71777380ddbfe90288e6187a827 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie-sc8180x.yaml
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie-sc8180x.yaml
@@ -77,6 +77,7 @@ unevaluatedProperties: false
examples:
- |
#include <dt-bindings/clock/qcom,gcc-sc8180x.h>
+ #include <dt-bindings/gpio/gpio.h>
#include <dt-bindings/interconnect/qcom,sc8180x.h>
#include <dt-bindings/interrupt-controller/arm-gic.h>
@@ -164,5 +165,7 @@ examples:
resets = <&gcc GCC_PCIE_0_BCR>;
reset-names = "pci";
+
+ perst-gpios = <&tlmm 175 GPIO_ACTIVE_LOW>;
};
};
--
2.48.1On Thu, Nov 06, 2025 at 04:57:16PM +0530, Manivannan Sadhasivam wrote: > Currently, the binding supports specifying the required PHY, PERST# > properties in two ways: > > 1. Controller node (deprecated) > - phys > - perst-gpios > > 2. Root Port node > - phys > - reset-gpios > > But there is no check to make sure that the both variants are not mixed. > For instance, if the Controller node specifies 'phys', 'reset-gpios', Schema already does not allow it, unless I missed which schema defines reset-gpios in controller node. > or if the Root Port node specifies 'phys', 'perst-gpios', then the driver > will fail as reported. Hence, enforce the check in the binding to catch > these issues. I do not see such check. > > It is also possible that DTs could have 'phys' property in Controller node > and 'reset-gpios' properties in the Root Port node. It will also be a > problem, but it is not possible to catch these cross-node issues in the > binding. ... so this commit changes nothing? The commit actually does change, but something completely different than you write here, so entire commit msg is describing entirely different cast. What you achieve here is to require perst-gpios, if controller node defined phys. Unfortunately your commit msg does not explain why perst-gpios are now required... > > Reported-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com> > Reported-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> > Closes: https://lore.kernel.org/linux-pci/8f2e0631-6c59-4298-b36e-060708970ced@oss.qualcomm.com > Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> That's too many tags. Either someone reported you bug or someone suggested you to do something, not both (and proposing solution is not suggesting a commit since you already knew you need to make the commit because of bug...) > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com> > --- > .../devicetree/bindings/pci/qcom,pcie-common.yaml | 16 ++++++++++++++++ > .../devicetree/bindings/pci/qcom,pcie-sc8180x.yaml | 3 +++ > 2 files changed, 19 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml > index ab2509ec1c4b40ac91a93033d1bab1b12c39362f..d56c0dc2ae4d3944294ca50cab708915c9f60ea8 100644 > --- a/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-common.yaml > @@ -111,6 +111,14 @@ patternProperties: > phys: > maxItems: 1 > > + oneOf: > + - required: > + - phys > + - reset-gpios > + - properties: > + phys: false > + reset-gpios: false > + > unevaluatedProperties: false > > required: > @@ -129,6 +137,14 @@ anyOf: > - required: > - msi-map > > +oneOf: > + - required: > + - phys > + - perst-gpios > + - properties: > + phys: false > + perst-gpios: false > + > allOf: > - $ref: /schemas/pci/pci-host-bridge.yaml# > > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-sc8180x.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-sc8180x.yaml > index 34a4d7b2c8459aeb615736f54c1971014adb205f..17abc7f7b7e9d71777380ddbfe90288e6187a827 100644 > --- a/Documentation/devicetree/bindings/pci/qcom,pcie-sc8180x.yaml > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-sc8180x.yaml > @@ -77,6 +77,7 @@ unevaluatedProperties: false > examples: > - | > #include <dt-bindings/clock/qcom,gcc-sc8180x.h> > + #include <dt-bindings/gpio/gpio.h> > #include <dt-bindings/interconnect/qcom,sc8180x.h> > #include <dt-bindings/interrupt-controller/arm-gic.h> > > @@ -164,5 +165,7 @@ examples: > > resets = <&gcc GCC_PCIE_0_BCR>; > reset-names = "pci"; > + > + perst-gpios = <&tlmm 175 GPIO_ACTIVE_LOW>; > }; > }; > > -- > 2.48.1 >
On Sat, Nov 08, 2025 at 12:59:50PM +0100, Krzysztof Kozlowski wrote: > On Thu, Nov 06, 2025 at 04:57:16PM +0530, Manivannan Sadhasivam wrote: > > Currently, the binding supports specifying the required PHY, PERST# > > properties in two ways: > > > > 1. Controller node (deprecated) > > - phys > > - perst-gpios > > > > 2. Root Port node > > - phys > > - reset-gpios > > > > But there is no check to make sure that the both variants are not mixed. > > For instance, if the Controller node specifies 'phys', 'reset-gpios', > > Schema already does not allow it, unless I missed which schema defines > reset-gpios in controller node. > 'reset-gpios' is currently a valid property for both controller and Root Port nodes. Where does the schema restricts it? > > or if the Root Port node specifies 'phys', 'perst-gpios', then the driver > > will fail as reported. Hence, enforce the check in the binding to catch > > these issues. > > I do not see such check. > Don't you think the below required properties not enforce this check for Root Port and Controller node? This atleast makes sure that if 'phys' is present, 'reset-gpios' would be required for Root Port and 'perst-gpios' is required for Controller node. > > > > It is also possible that DTs could have 'phys' property in Controller node > > and 'reset-gpios' properties in the Root Port node. It will also be a > > problem, but it is not possible to catch these cross-node issues in the > > binding. > > ... so this commit changes nothing? > > The commit actually does change, but something completely different than > you write here, so entire commit msg is describing entirely different > cast. What you achieve here is to require perst-gpios, if controller > node defined phys. Unfortunately your commit msg does not explain why > perst-gpios are now required... > The Qcom PCIe controller node never supported 'reset-gpios' for PERST#. It used the 'perst-gpios' property instead. And I do not wanted to replace it with 'reset-gpios' property since we had decided to move PERST# to Root Port node, where 'reset-gpios' is already the norm. > > > > Reported-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com> > > Reported-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> > > Closes: https://lore.kernel.org/linux-pci/8f2e0631-6c59-4298-b36e-060708970ced@oss.qualcomm.com > > Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> > > That's too many tags. Either someone reported you bug or someone > suggested you to do something, not both (and proposing solution is not > suggesting a commit since you already knew you need to make the commit > because of bug...) > I disagree. Both Konrad and Krishna reported the issue in mixing up the properties and driver ended up failing the probe. Then Dmitry suggested a schema snippet [1] to catch these kind of mixups during DT validation. I did see it as a valid suggestion that deserved the tag. - Mani [1] https://lore.kernel.org/linux-pci/qref5ooh6pl2sznf7iifrbric7hsap63ffbytkizdyrzt6mtqz@q5r27ho2sbq3/ -- மணிவண்ணன் சதாசிவம்
© 2016 - 2025 Red Hat, Inc.