Re: [PATCH v2 1/2] dt-bindings: PCI: qcom: Enforce check for PHY, PERST# properties

[Manivannan Sadhasivam]

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@xxxxxxxxxxxxxxxx>
> > Reported-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxxxx>
> > Closes: https://lore.kernel.org/linux-pci/8f2e0631-6c59-4298-b36e-060708970ced@xxxxxxxxxxxxxxxx
> > Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxxxxxxxx>
> 
> 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/

-- 
மணிவண்ணன் சதாசிவம்