The qcom_pcie_parse_ports() function currently iterates over all available
child nodes of the PCIe controller's device tree node. This can lead to
attempts to parse unrelated nodes like OPP nodes, resulting in unnecessary
errors or misconfiguration.

Restrict the parsing logic to only consider child nodes named "pcie" or
"pci", which are the expected node names for PCIe ports.

Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 294babe1816e4d0c2b2343fe22d89af72afcd6cd..5dbdb69fbdd1b9b78a3ebba3cd50d78168f2d595 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1740,6 +1740,8 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
 	int ret = -ENOENT;
 
 	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
+		if (!(of_node_name_eq(of_port, "pcie") || of_node_name_eq(of_port, "pci")))
+			continue;
 		ret = qcom_pcie_parse_port(pcie, of_port);
 		if (ret)
 			goto err_port_del;

-- 
2.34.1

On 26/08/2025 07:18, Krishna Chaitanya Chundru wrote:
> The qcom_pcie_parse_ports() function currently iterates over all available
> child nodes of the PCIe controller's device tree node. This can lead to
> attempts to parse unrelated nodes like OPP nodes, resulting in unnecessary
> errors or misconfiguration.
> 
> Restrict the parsing logic to only consider child nodes named "pcie" or
> "pci", which are the expected node names for PCIe ports.
> 
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 294babe1816e4d0c2b2343fe22d89af72afcd6cd..5dbdb69fbdd1b9b78a3ebba3cd50d78168f2d595 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1740,6 +1740,8 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
>  	int ret = -ENOENT;
>  
>  	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
> +		if (!(of_node_name_eq(of_port, "pcie") || of_node_name_eq(of_port, "pci")))


Huh? Where is this ABI documented?

Best regards,
Krzysztof

On 26/08/2025 10:27, Krzysztof Kozlowski wrote:
> On 26/08/2025 07:18, Krishna Chaitanya Chundru wrote:
>> The qcom_pcie_parse_ports() function currently iterates over all available
>> child nodes of the PCIe controller's device tree node. This can lead to
>> attempts to parse unrelated nodes like OPP nodes, resulting in unnecessary
>> errors or misconfiguration.
>>
>> Restrict the parsing logic to only consider child nodes named "pcie" or
>> "pci", which are the expected node names for PCIe ports.
>>
>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
>> ---
>>  drivers/pci/controller/dwc/pcie-qcom.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index 294babe1816e4d0c2b2343fe22d89af72afcd6cd..5dbdb69fbdd1b9b78a3ebba3cd50d78168f2d595 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -1740,6 +1740,8 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
>>  	int ret = -ENOENT;
>>  
>>  	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
>> +		if (!(of_node_name_eq(of_port, "pcie") || of_node_name_eq(of_port, "pci")))
> 
> 
> Huh? Where is this ABI documented?

I see it actually might be documented, but you did not mention it at
all. I doubt you even checked.

Please reference exactly where is the ABI, so reviewing will be easier.

I still think though that it is wrong - we don't want device node names
to be the ABI if we already have compatibles and the children here
should have them, right?
Best regards,
Krzysztof

On 8/26/2025 2:02 PM, Krzysztof Kozlowski wrote:
> On 26/08/2025 10:27, Krzysztof Kozlowski wrote:
>> On 26/08/2025 07:18, Krishna Chaitanya Chundru wrote:
>>> The qcom_pcie_parse_ports() function currently iterates over all available
>>> child nodes of the PCIe controller's device tree node. This can lead to
>>> attempts to parse unrelated nodes like OPP nodes, resulting in unnecessary
>>> errors or misconfiguration.
>>>
>>> Restrict the parsing logic to only consider child nodes named "pcie" or
>>> "pci", which are the expected node names for PCIe ports.
>>>
>>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
>>> ---
>>>   drivers/pci/controller/dwc/pcie-qcom.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>>> index 294babe1816e4d0c2b2343fe22d89af72afcd6cd..5dbdb69fbdd1b9b78a3ebba3cd50d78168f2d595 100644
>>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>>> @@ -1740,6 +1740,8 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
>>>   	int ret = -ENOENT;
>>>   
>>>   	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
>>> +		if (!(of_node_name_eq(of_port, "pcie") || of_node_name_eq(of_port, "pci")))
>>
>>
>> Huh? Where is this ABI documented?
> 
> I see it actually might be documented, but you did not mention it at
> all. I doubt you even checked.
> 
> Please reference exactly where is the ABI, so reviewing will be easier.
> 
> I still think though that it is wrong - we don't want device node names
> to be the ABI if we already have compatibles and the children here
> should have them, right?
I intended to check for device_type to be pci, my mistake I went with
the node name, I will update the patch with this logic

if (!of_node_is_type(np, "pci"))
	continue

Thanks for reviewing it.

- Krishna Chaitanya.

> Best regards,
> Krzysztof

On Tue, Aug 26, 2025 at 02:33:44PM GMT, Krishna Chaitanya Chundru wrote:
> 
> 
> On 8/26/2025 2:02 PM, Krzysztof Kozlowski wrote:
> > On 26/08/2025 10:27, Krzysztof Kozlowski wrote:
> > > On 26/08/2025 07:18, Krishna Chaitanya Chundru wrote:
> > > > The qcom_pcie_parse_ports() function currently iterates over all available
> > > > child nodes of the PCIe controller's device tree node. This can lead to
> > > > attempts to parse unrelated nodes like OPP nodes, resulting in unnecessary
> > > > errors or misconfiguration.
> > > > 
> > > > Restrict the parsing logic to only consider child nodes named "pcie" or
> > > > "pci", which are the expected node names for PCIe ports.
> > > > 
> > > > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> > > > ---
> > > >   drivers/pci/controller/dwc/pcie-qcom.c | 2 ++
> > > >   1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > index 294babe1816e4d0c2b2343fe22d89af72afcd6cd..5dbdb69fbdd1b9b78a3ebba3cd50d78168f2d595 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > @@ -1740,6 +1740,8 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
> > > >   	int ret = -ENOENT;
> > > >   	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
> > > > +		if (!(of_node_name_eq(of_port, "pcie") || of_node_name_eq(of_port, "pci")))
> > > 
> > > 
> > > Huh? Where is this ABI documented?
> > 
> > I see it actually might be documented, but you did not mention it at
> > all. I doubt you even checked.
> > 
> > Please reference exactly where is the ABI, so reviewing will be easier.
> > 
> > I still think though that it is wrong - we don't want device node names
> > to be the ABI if we already have compatibles and the children here
> > should have them, right?
> I intended to check for device_type to be pci, my mistake I went with
> the node name, I will update the patch with this logic
> 
> if (!of_node_is_type(np, "pci"))
> 	continue
> 

Yes, we can rely on it as it is a required property for PCI bridge nodes.

- Mani

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

On Tue, Aug 26, 2025 at 10:48:19AM GMT, Krishna Chaitanya Chundru wrote:
> The qcom_pcie_parse_ports() function currently iterates over all available
> child nodes of the PCIe controller's device tree node. This can lead to
> attempts to parse unrelated nodes like OPP nodes, resulting in unnecessary
> errors or misconfiguration.
> 

What errors? Errors you are seeing on your setup or you envision?

> Restrict the parsing logic to only consider child nodes named "pcie" or
> "pci", which are the expected node names for PCIe ports.
> 
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>

Since this is a fix, 'Fixes' tag is needed.

> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 294babe1816e4d0c2b2343fe22d89af72afcd6cd..5dbdb69fbdd1b9b78a3ebba3cd50d78168f2d595 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1740,6 +1740,8 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
>  	int ret = -ENOENT;
>  
>  	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
> +		if (!(of_node_name_eq(of_port, "pcie") || of_node_name_eq(of_port, "pci")))

May I know which platform has 'pci' as the node name for the bridge node? AFAIK,
all platforms defining bridge nodes have 'pcie' as the node name.

- Mani

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

On 26/08/2025 08:17, Manivannan Sadhasivam wrote:
> On Tue, Aug 26, 2025 at 10:48:19AM GMT, Krishna Chaitanya Chundru wrote:
>> The qcom_pcie_parse_ports() function currently iterates over all available
>> child nodes of the PCIe controller's device tree node. This can lead to
>> attempts to parse unrelated nodes like OPP nodes, resulting in unnecessary
>> errors or misconfiguration.
>>
> 
> What errors? Errors you are seeing on your setup or you envision?
> 
>> Restrict the parsing logic to only consider child nodes named "pcie" or
>> "pci", which are the expected node names for PCIe ports.
>>
>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> 
> Since this is a fix, 'Fixes' tag is needed.
> 
>> ---
>>  drivers/pci/controller/dwc/pcie-qcom.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index 294babe1816e4d0c2b2343fe22d89af72afcd6cd..5dbdb69fbdd1b9b78a3ebba3cd50d78168f2d595 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -1740,6 +1740,8 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
>>  	int ret = -ENOENT;
>>  
>>  	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
>> +		if (!(of_node_name_eq(of_port, "pcie") || of_node_name_eq(of_port, "pci")))
> 
> May I know which platform has 'pci' as the node name for the bridge node? AFAIK,
> all platforms defining bridge nodes have 'pcie' as the node name.

It does not matter. If I name my node name as "pc" it stops working?

No, Qualcomm cannot introduce such hidden ABI.
Best regards,
Krzysztof

On Tue, Aug 26, 2025 at 10:28:51AM GMT, Krzysztof Kozlowski wrote:
> On 26/08/2025 08:17, Manivannan Sadhasivam wrote:
> > On Tue, Aug 26, 2025 at 10:48:19AM GMT, Krishna Chaitanya Chundru wrote:
> >> The qcom_pcie_parse_ports() function currently iterates over all available
> >> child nodes of the PCIe controller's device tree node. This can lead to
> >> attempts to parse unrelated nodes like OPP nodes, resulting in unnecessary
> >> errors or misconfiguration.
> >>
> > 
> > What errors? Errors you are seeing on your setup or you envision?
> > 
> >> Restrict the parsing logic to only consider child nodes named "pcie" or
> >> "pci", which are the expected node names for PCIe ports.
> >>
> >> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> > 
> > Since this is a fix, 'Fixes' tag is needed.
> > 
> >> ---
> >>  drivers/pci/controller/dwc/pcie-qcom.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> >> index 294babe1816e4d0c2b2343fe22d89af72afcd6cd..5dbdb69fbdd1b9b78a3ebba3cd50d78168f2d595 100644
> >> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> >> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> >> @@ -1740,6 +1740,8 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
> >>  	int ret = -ENOENT;
> >>  
> >>  	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
> >> +		if (!(of_node_name_eq(of_port, "pcie") || of_node_name_eq(of_port, "pci")))
> > 
> > May I know which platform has 'pci' as the node name for the bridge node? AFAIK,
> > all platforms defining bridge nodes have 'pcie' as the node name.
> 
> It does not matter. If I name my node name as "pc" it stops working?
> 
> No, Qualcomm cannot introduce such hidden ABI.

There is no hidden ABI that Qcom is introducing. We are just trying to reuse the
standard node names documented in the devicetree spec. So you are saying that
we should not rely on it even though it is documented? Maybe because, the dt
tooling is not yet screaming if people put non-standard names in DT?

- Mani

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

On 26/08/2025 11:26, Manivannan Sadhasivam wrote:
> On Tue, Aug 26, 2025 at 10:28:51AM GMT, Krzysztof Kozlowski wrote:
>> On 26/08/2025 08:17, Manivannan Sadhasivam wrote:
>>> On Tue, Aug 26, 2025 at 10:48:19AM GMT, Krishna Chaitanya Chundru wrote:
>>>> The qcom_pcie_parse_ports() function currently iterates over all available
>>>> child nodes of the PCIe controller's device tree node. This can lead to
>>>> attempts to parse unrelated nodes like OPP nodes, resulting in unnecessary
>>>> errors or misconfiguration.
>>>>
>>>
>>> What errors? Errors you are seeing on your setup or you envision?
>>>
>>>> Restrict the parsing logic to only consider child nodes named "pcie" or
>>>> "pci", which are the expected node names for PCIe ports.
>>>>
>>>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
>>>
>>> Since this is a fix, 'Fixes' tag is needed.
>>>
>>>> ---
>>>>  drivers/pci/controller/dwc/pcie-qcom.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>>>> index 294babe1816e4d0c2b2343fe22d89af72afcd6cd..5dbdb69fbdd1b9b78a3ebba3cd50d78168f2d595 100644
>>>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>>>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>>>> @@ -1740,6 +1740,8 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
>>>>  	int ret = -ENOENT;
>>>>  
>>>>  	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
>>>> +		if (!(of_node_name_eq(of_port, "pcie") || of_node_name_eq(of_port, "pci")))
>>>
>>> May I know which platform has 'pci' as the node name for the bridge node? AFAIK,
>>> all platforms defining bridge nodes have 'pcie' as the node name.
>>
>> It does not matter. If I name my node name as "pc" it stops working?
>>
>> No, Qualcomm cannot introduce such hidden ABI.
> 
> There is no hidden ABI that Qcom is introducing. We are just trying to reuse the
> standard node names documented in the devicetree spec. So you are saying that
> we should not rely on it even though it is documented? Maybe because, the dt
> tooling is not yet screaming if people put non-standard names in DT?
> 

If it is documented, you can use it, but I doubted first the author even
checked that. Otherwise commit message would say that.

As I mentioned in other response, I still find it discouraged pattern if
you have (and you do have!) compatibles.


Best regards,
Krzysztof

On Tue, Aug 26, 2025 at 11:29:37AM GMT, Krzysztof Kozlowski wrote:
> On 26/08/2025 11:26, Manivannan Sadhasivam wrote:
> > On Tue, Aug 26, 2025 at 10:28:51AM GMT, Krzysztof Kozlowski wrote:
> >> On 26/08/2025 08:17, Manivannan Sadhasivam wrote:
> >>> On Tue, Aug 26, 2025 at 10:48:19AM GMT, Krishna Chaitanya Chundru wrote:
> >>>> The qcom_pcie_parse_ports() function currently iterates over all available
> >>>> child nodes of the PCIe controller's device tree node. This can lead to
> >>>> attempts to parse unrelated nodes like OPP nodes, resulting in unnecessary
> >>>> errors or misconfiguration.
> >>>>
> >>>
> >>> What errors? Errors you are seeing on your setup or you envision?
> >>>
> >>>> Restrict the parsing logic to only consider child nodes named "pcie" or
> >>>> "pci", which are the expected node names for PCIe ports.
> >>>>
> >>>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> >>>
> >>> Since this is a fix, 'Fixes' tag is needed.
> >>>
> >>>> ---
> >>>>  drivers/pci/controller/dwc/pcie-qcom.c | 2 ++
> >>>>  1 file changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> >>>> index 294babe1816e4d0c2b2343fe22d89af72afcd6cd..5dbdb69fbdd1b9b78a3ebba3cd50d78168f2d595 100644
> >>>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> >>>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> >>>> @@ -1740,6 +1740,8 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
> >>>>  	int ret = -ENOENT;
> >>>>  
> >>>>  	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
> >>>> +		if (!(of_node_name_eq(of_port, "pcie") || of_node_name_eq(of_port, "pci")))
> >>>
> >>> May I know which platform has 'pci' as the node name for the bridge node? AFAIK,
> >>> all platforms defining bridge nodes have 'pcie' as the node name.
> >>
> >> It does not matter. If I name my node name as "pc" it stops working?
> >>
> >> No, Qualcomm cannot introduce such hidden ABI.
> > 
> > There is no hidden ABI that Qcom is introducing. We are just trying to reuse the
> > standard node names documented in the devicetree spec. So you are saying that
> > we should not rely on it even though it is documented? Maybe because, the dt
> > tooling is not yet screaming if people put non-standard names in DT?
> > 
> 
> If it is documented, you can use it, but I doubted first the author even
> checked that. Otherwise commit message would say that.
> 
> As I mentioned in other response, I still find it discouraged pattern if
> you have (and you do have!) compatibles.
> 

Compatibles for the PCI bridges are not mandatory, so we cannot use it. But
'device_type' is and Krishna is going to use that instead.

- Mani

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

On 8/26/2025 11:47 AM, Manivannan Sadhasivam wrote:
> On Tue, Aug 26, 2025 at 10:48:19AM GMT, Krishna Chaitanya Chundru wrote:
>> The qcom_pcie_parse_ports() function currently iterates over all available
>> child nodes of the PCIe controller's device tree node. This can lead to
>> attempts to parse unrelated nodes like OPP nodes, resulting in unnecessary
>> errors or misconfiguration.
>>
> 
> What errors? Errors you are seeing on your setup or you envision?
we see driver is searching for reset in OPP node as it is not able to
find it is falling to legacy way. where there is no phy nodes defined in
the controller node probe is failling. I will add this in commit text.
> 
>> Restrict the parsing logic to only consider child nodes named "pcie" or
>> "pci", which are the expected node names for PCIe ports.
>>
>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> 
> Since this is a fix, 'Fixes' tag is needed.
> 
ack.
>> ---
>>   drivers/pci/controller/dwc/pcie-qcom.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index 294babe1816e4d0c2b2343fe22d89af72afcd6cd..5dbdb69fbdd1b9b78a3ebba3cd50d78168f2d595 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -1740,6 +1740,8 @@ static int qcom_pcie_parse_ports(struct qcom_pcie *pcie)
>>   	int ret = -ENOENT;
>>   
>>   	for_each_available_child_of_node_scoped(dev->of_node, of_port) {
>> +		if (!(of_node_name_eq(of_port, "pcie") || of_node_name_eq(of_port, "pci")))
> 
> May I know which platform has 'pci' as the node name for the bridge node? AFAIK,
> all platforms defining bridge nodes have 'pcie' as the node name.
>
I see most of the qcom platforms are using pci only. for reference i see
it  in sm8650[1] & sm8550.

[1] 
https://elixir.bootlin.com/linux/v6.16.3/source/arch/arm64/boot/dts/qcom/sm8650.dtsi#L3699

- Krishna Chaitanya.
> - Mani
>