[v1] PCI: qcom: Binding fix

[PATCH 3/3] PCI: qcom: Treat PHY and PERST# as optional for the new binding

Posted by Manivannan Sadhasivam 2 months, 1 week ago

Even for the new DT binding where the PHY and PERST# properties are
specified in the Root Port, both are optional. Hence, treat them as
optional in the driver too.

If both properties are not specified, then fall back to parsing the legacy
binding for backwards compatibility.

Fixes: a2fbecdbbb9d ("PCI: qcom: Add support for parsing the new Root Port binding")
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 805edbbfe7eba496bc99ca82051dee43d240f359..d380981cf3ad78f549de3dc06bd2f626f8f53920 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1720,13 +1720,20 @@ static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node
 
 	reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(node),
 				      "reset", GPIOD_OUT_HIGH, "PERST#");
-	if (IS_ERR(reset))
+	if (IS_ERR(reset) && PTR_ERR(reset) != -ENOENT)
 		return PTR_ERR(reset);
 
-	phy = devm_of_phy_get(dev, node, NULL);
+	phy = devm_of_phy_optional_get(dev, node, NULL);
 	if (IS_ERR(phy))
 		return PTR_ERR(phy);
 
+	/*
+	 * If both PHY and PERST# properties are not specified, then try the
+	 * legacy binding.
+	 */
+	if (PTR_ERR(reset) == -ENOENT && !phy)
+		return -ENOENT;
+
 	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
 	if (!port)
 		return -ENOMEM;

-- 
2.48.1

Re: [PATCH 3/3] PCI: qcom: Treat PHY and PERST# as optional for the new binding

Posted by Konrad Dybcio 2 months, 1 week ago

On 10/10/25 8:25 PM, Manivannan Sadhasivam wrote:
> Even for the new DT binding where the PHY and PERST# properties are
> specified in the Root Port, both are optional. Hence, treat them as
> optional in the driver too.

I suppose this makes sense if the PHY is transparent to the OS
or otherwise pre-programmed and PERST# is hardwired or otherwise
unnecessary.. both of which I suppose aren't totally impossible..

> 
> If both properties are not specified, then fall back to parsing the legacy
> binding for backwards compatibility.
> 
> Fixes: a2fbecdbbb9d ("PCI: qcom: Add support for parsing the new Root Port binding")
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 805edbbfe7eba496bc99ca82051dee43d240f359..d380981cf3ad78f549de3dc06bd2f626f8f53920 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1720,13 +1720,20 @@ static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node
>  
>  	reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(node),
>  				      "reset", GPIOD_OUT_HIGH, "PERST#");
> -	if (IS_ERR(reset))
> +	if (IS_ERR(reset) && PTR_ERR(reset) != -ENOENT)
>  		return PTR_ERR(reset);

Please introduce an _optional variant instead

Konrad

Re: [PATCH 3/3] PCI: qcom: Treat PHY and PERST# as optional for the new binding

Posted by Manivannan Sadhasivam 2 months, 1 week ago

+ GPIO folks for the below API query

On Fri, Oct 10, 2025 at 08:32:51PM +0200, Konrad Dybcio wrote:
> On 10/10/25 8:25 PM, Manivannan Sadhasivam wrote:
> > Even for the new DT binding where the PHY and PERST# properties are
> > specified in the Root Port, both are optional. Hence, treat them as
> > optional in the driver too.
> 
> I suppose this makes sense if the PHY is transparent to the OS
> or otherwise pre-programmed and PERST# is hardwired or otherwise
> unnecessary.. both of which I suppose aren't totally impossible..
> 

PERST# is by definition an optional signal, but I'm not sure about why PHY is
not used by the controller driver.

> > 
> > If both properties are not specified, then fall back to parsing the legacy
> > binding for backwards compatibility.
> > 
> > Fixes: a2fbecdbbb9d ("PCI: qcom: Add support for parsing the new Root Port binding")
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-qcom.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 805edbbfe7eba496bc99ca82051dee43d240f359..d380981cf3ad78f549de3dc06bd2f626f8f53920 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -1720,13 +1720,20 @@ static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node
> >  
> >  	reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(node),
> >  				      "reset", GPIOD_OUT_HIGH, "PERST#");
> > -	if (IS_ERR(reset))
> > +	if (IS_ERR(reset) && PTR_ERR(reset) != -ENOENT)
> >  		return PTR_ERR(reset);
> 
> Please introduce an _optional variant instead
> 

Linus, Bartosz, are you OK with devm_fwnode_gpiod_get_optional() API? Just
wanted to confirm before I go ahead as there are existing users checking for
-ENOENT explicitly. Not sure if they are doing it for a reason other than the
absence of the _optional variant or not.

- Mani

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

Re: [PATCH 3/3] PCI: qcom: Treat PHY and PERST# as optional for the new binding

Posted by Krishna Chaitanya Chundru 2 months ago


On 10/11/2025 9:39 AM, Manivannan Sadhasivam wrote:
> + GPIO folks for the below API query
> 
> On Fri, Oct 10, 2025 at 08:32:51PM +0200, Konrad Dybcio wrote:
>> On 10/10/25 8:25 PM, Manivannan Sadhasivam wrote:
>>> Even for the new DT binding where the PHY and PERST# properties are
>>> specified in the Root Port, both are optional. Hence, treat them as
>>> optional in the driver too.
>>
>> I suppose this makes sense if the PHY is transparent to the OS
>> or otherwise pre-programmed and PERST# is hardwired or otherwise
>> unnecessary.. both of which I suppose aren't totally impossible..
>>
> 
> PERST# is by definition an optional signal, but I'm not sure about why PHY is
> not used by the controller driver.
> 
one case where phy is optional is in pcie tunneling through usb4, there
we don't require any phy.

- Krishna Chaitanya.
>>>
>>> If both properties are not specified, then fall back to parsing the legacy
>>> binding for backwards compatibility.
>>>
>>> Fixes: a2fbecdbbb9d ("PCI: qcom: Add support for parsing the new Root Port binding")
>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
>>> ---
>>>   drivers/pci/controller/dwc/pcie-qcom.c | 11 +++++++++--
>>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>>> index 805edbbfe7eba496bc99ca82051dee43d240f359..d380981cf3ad78f549de3dc06bd2f626f8f53920 100644
>>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>>> @@ -1720,13 +1720,20 @@ static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node
>>>   
>>>   	reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(node),
>>>   				      "reset", GPIOD_OUT_HIGH, "PERST#");
>>> -	if (IS_ERR(reset))
>>> +	if (IS_ERR(reset) && PTR_ERR(reset) != -ENOENT)
>>>   		return PTR_ERR(reset);
>>
>> Please introduce an _optional variant instead
>>
> 
> Linus, Bartosz, are you OK with devm_fwnode_gpiod_get_optional() API? Just
> wanted to confirm before I go ahead as there are existing users checking for
> -ENOENT explicitly. Not sure if they are doing it for a reason other than the
> absence of the _optional variant or not.
> 
> - Mani
>

Re: [PATCH 3/3] PCI: qcom: Treat PHY and PERST# as optional for the new binding

Posted by Bartosz Golaszewski 2 months, 1 week ago

On Sat, Oct 11, 2025 at 6:09 AM Manivannan Sadhasivam <mani@kernel.org> wrote:
>
> + GPIO folks for the below API query
>
> On Fri, Oct 10, 2025 at 08:32:51PM +0200, Konrad Dybcio wrote:
> > On 10/10/25 8:25 PM, Manivannan Sadhasivam wrote:
> > > Even for the new DT binding where the PHY and PERST# properties are
> > > specified in the Root Port, both are optional. Hence, treat them as
> > > optional in the driver too.
> >
> > I suppose this makes sense if the PHY is transparent to the OS
> > or otherwise pre-programmed and PERST# is hardwired or otherwise
> > unnecessary.. both of which I suppose aren't totally impossible..
> >
>
> PERST# is by definition an optional signal, but I'm not sure about why PHY is
> not used by the controller driver.
>
> > >
> > > If both properties are not specified, then fall back to parsing the legacy
> > > binding for backwards compatibility.
> > >
> > > Fixes: a2fbecdbbb9d ("PCI: qcom: Add support for parsing the new Root Port binding")
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > > ---
> > >  drivers/pci/controller/dwc/pcie-qcom.c | 11 +++++++++--
> > >  1 file changed, 9 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > index 805edbbfe7eba496bc99ca82051dee43d240f359..d380981cf3ad78f549de3dc06bd2f626f8f53920 100644
> > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > @@ -1720,13 +1720,20 @@ static int qcom_pcie_parse_port(struct qcom_pcie *pcie, struct device_node *node
> > >
> > >     reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(node),
> > >                                   "reset", GPIOD_OUT_HIGH, "PERST#");
> > > -   if (IS_ERR(reset))
> > > +   if (IS_ERR(reset) && PTR_ERR(reset) != -ENOENT)
> > >             return PTR_ERR(reset);
> >
> > Please introduce an _optional variant instead
> >
>
> Linus, Bartosz, are you OK with devm_fwnode_gpiod_get_optional() API? Just
> wanted to confirm before I go ahead as there are existing users checking for
> -ENOENT explicitly. Not sure if they are doing it for a reason other than the
> absence of the _optional variant or not.
>

I'm fine as long as it follows the conventions established by other
GPIOLIB _optional interfaces.

Bart