[PATCH v3 0/3] PCI: dwc: Skip waiting for link up if vendor drivers can detect Link up event

[PATCH v3 0/3] PCI: dwc: Skip waiting for link up if vendor drivers can detect Link up event

[Krishna chaitanya chundru]

If the vendor drivers can detect the Link up event using mechanisms
such as Link up IRQ, then waiting for Link up during probe is not
needed. if the drivers can be notified when the link comes up,
vendor driver can enumerate downstream devices instead of waiting
here, which optimizes the boot time.

So skip waiting for link to be up if the driver supports 'linkup_irq'.

Currently, only Qcom RC driver supports the 'linkup_irq' as it can detect
the Link Up event using its own 'global IRQ' interrupt. So set
'linkup_irq' flag for QCOM drivers.

As part of the PCIe link up event, the ICC and OPP values are updated.

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
Changes in v3:
- seperate dwc changes and qcom changes as suggested (mani)
- update commit & comments as suggested (mani & bjorn)
Changes in v2:
- Updated the bypass_link_up_wait name to linkup_irq  & added comment as
  suggested (mani).
- seperated the icc and opp update patch (mani).
- Link to v1: https://lore.kernel.org/r/20240917-remove_wait-v1-1-456d2551bc50@quicinc.com

---
Krishna chaitanya chundru (3):
      PCI: dwc: Skip waiting for link up if vendor drivers can detect Link up event
      PCI: qcom: Set linkup_irq if global IRQ handler is present
      PCI: qcom: Update ICC and OPP values during link up event

 drivers/pci/controller/dwc/pcie-designware-host.c | 10 ++++++++--
 drivers/pci/controller/dwc/pcie-designware.h      |  1 +
 drivers/pci/controller/dwc/pcie-qcom.c            |  7 ++++++-
 3 files changed, 15 insertions(+), 3 deletions(-)
---
base-commit: 070f00ffd0bac88ed5e874c9ae0378f75e2203f3
change-id: 20241101-remove_wait-80ad43547a44

Best regards,
-- 
Krishna chaitanya chundru <quic_krichai@quicinc.com>

[PATCH v3 1/3] PCI: dwc: Skip waiting for link up if vendor drivers can detect Link up event

[Krishna chaitanya chundru]

If the vendor drivers can detect the Link up event using mechanisms
such as Link up IRQ and can the driver can enumerate downstream devices
instead of waiting here, then waiting for Link up during probe is not
needed here, which optimizes the boot time.

So skip waiting for link to be up if the driver supports 'linkup_irq'.

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 10 ++++++++--
 drivers/pci/controller/dwc/pcie-designware.h      |  1 +
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 3e41865c7290..26418873ce14 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -530,8 +530,14 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
 			goto err_remove_edma;
 	}
 
-	/* Ignore errors, the link may come up later */
-	dw_pcie_wait_for_link(pci);
+	/*
+	 * Note: The link up delay is skipped only when a link up IRQ is present.
+	 * This flag should not be used to bypass the link up delay for arbitrary
+	 * reasons.
+	 */
+	if (!pp->linkup_irq)
+		/* Ignore errors, the link may come up later */
+		dw_pcie_wait_for_link(pci);
 
 	bridge->sysdata = pp;
 
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 347ab74ac35a..539c6d106bb0 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -379,6 +379,7 @@ struct dw_pcie_rp {
 	bool			use_atu_msg;
 	int			msg_atu_index;
 	struct resource		*msg_res;
+	bool			linkup_irq;
 };
 
 struct dw_pcie_ep_ops {

-- 
2.34.1

Re: [PATCH v3 1/3] PCI: dwc: Skip waiting for link up if vendor drivers can detect Link up event

[Bjorn Andersson]

On Fri, Nov 01, 2024 at 05:04:12PM GMT, Krishna chaitanya chundru wrote:
> If the vendor drivers can detect the Link up event using mechanisms
> such as Link up IRQ and can the driver can enumerate downstream devices
> instead of waiting here, then waiting for Link up during probe is not
> needed here, which optimizes the boot time.
> 
> So skip waiting for link to be up if the driver supports 'linkup_irq'.
> 
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 10 ++++++++--
>  drivers/pci/controller/dwc/pcie-designware.h      |  1 +
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 3e41865c7290..26418873ce14 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -530,8 +530,14 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>  			goto err_remove_edma;
>  	}
>  
> -	/* Ignore errors, the link may come up later */
> -	dw_pcie_wait_for_link(pci);
> +	/*
> +	 * Note: The link up delay is skipped only when a link up IRQ is present.
> +	 * This flag should not be used to bypass the link up delay for arbitrary
> +	 * reasons.

Perhaps by improving the naming of the variable, you don't need 3 lines
of comment describing the conditional.

> +	 */
> +	if (!pp->linkup_irq)
> +		/* Ignore errors, the link may come up later */

Does this mean that we will be able to start handling these errors?

> +		dw_pcie_wait_for_link(pci);
>  
>  	bridge->sysdata = pp;
>  
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 347ab74ac35a..539c6d106bb0 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -379,6 +379,7 @@ struct dw_pcie_rp {
>  	bool			use_atu_msg;
>  	int			msg_atu_index;
>  	struct resource		*msg_res;
> +	bool			linkup_irq;

Please name this for what it is, rather than some property from which
some other decision should be derived. (And then you need a comment to
describe how people should interpret and use it)

Also, "linkup_irq" sound like an int carrying the interrupt number, not
a boolean.


Please call it "use_async_linkup", "use_linkup_irq" or something.

Regards,
Bjorn

>  };
>  
>  struct dw_pcie_ep_ops {
> 
> -- 
> 2.34.1
> 
> 

Re: [PATCH v3 1/3] PCI: dwc: Skip waiting for link up if vendor drivers can detect Link up event

[Krishna Chaitanya Chundru]

On 11/1/2024 8:56 PM, Bjorn Andersson wrote:
> On Fri, Nov 01, 2024 at 05:04:12PM GMT, Krishna chaitanya chundru wrote:
>> If the vendor drivers can detect the Link up event using mechanisms
>> such as Link up IRQ and can the driver can enumerate downstream devices
>> instead of waiting here, then waiting for Link up during probe is not
>> needed here, which optimizes the boot time.
>>
>> So skip waiting for link to be up if the driver supports 'linkup_irq'.
>>
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> ---
>>   drivers/pci/controller/dwc/pcie-designware-host.c | 10 ++++++++--
>>   drivers/pci/controller/dwc/pcie-designware.h      |  1 +
>>   2 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>> index 3e41865c7290..26418873ce14 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>> @@ -530,8 +530,14 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>>   			goto err_remove_edma;
>>   	}
>>   
>> -	/* Ignore errors, the link may come up later */
>> -	dw_pcie_wait_for_link(pci);
>> +	/*
>> +	 * Note: The link up delay is skipped only when a link up IRQ is present.
>> +	 * This flag should not be used to bypass the link up delay for arbitrary
>> +	 * reasons.
> 
> Perhaps by improving the naming of the variable, you don't need 3 lines
> of comment describing the conditional.
> 
These comments are added so that no one will misuse this flag in the 
future which was happened previously.
>> +	 */
>> +	if (!pp->linkup_irq)
>> +		/* Ignore errors, the link may come up later */
> 
> Does this mean that we will be able to start handling these errors?
we haven't changed anything here it was present from long ago, the
reason why driver is not considering the return value is for some
platforms the link may come up later and the driver doesn't want to
fail here.
> 
>> +		dw_pcie_wait_for_link(pci);
>>   
>>   	bridge->sysdata = pp;
>>   
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
>> index 347ab74ac35a..539c6d106bb0 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>> @@ -379,6 +379,7 @@ struct dw_pcie_rp {
>>   	bool			use_atu_msg;
>>   	int			msg_atu_index;
>>   	struct resource		*msg_res;
>> +	bool			linkup_irq;
> 
> Please name this for what it is, rather than some property from which
> some other decision should be derived. (And then you need a comment to
> describe how people should interpret and use it)
> 
> Also, "linkup_irq" sound like an int carrying the interrupt number, not
> a boolean.
> 
> 
> Please call it "use_async_linkup", "use_linkup_irq" or something.
> 
ack will change it to "use_linkup_irq"

- Krishna Chaitanya.
> Regards,
> Bjorn
> 
>>   };
>>   
>>   struct dw_pcie_ep_ops {
>>
>> -- 
>> 2.34.1
>>
>>

Re: [PATCH v3 1/3] PCI: dwc: Skip waiting for link up if vendor drivers can detect Link up event

[Manivannan Sadhasivam]

On Fri, Nov 01, 2024 at 10:26:38AM -0500, Bjorn Andersson wrote:
> On Fri, Nov 01, 2024 at 05:04:12PM GMT, Krishna chaitanya chundru wrote:
> > If the vendor drivers can detect the Link up event using mechanisms
> > such as Link up IRQ and can the driver can enumerate downstream devices
> > instead of waiting here, then waiting for Link up during probe is not
> > needed here, which optimizes the boot time.
> > 
> > So skip waiting for link to be up if the driver supports 'linkup_irq'.
> > 
> > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware-host.c | 10 ++++++++--
> >  drivers/pci/controller/dwc/pcie-designware.h      |  1 +
> >  2 files changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index 3e41865c7290..26418873ce14 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -530,8 +530,14 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> >  			goto err_remove_edma;
> >  	}
> >  
> > -	/* Ignore errors, the link may come up later */
> > -	dw_pcie_wait_for_link(pci);
> > +	/*
> > +	 * Note: The link up delay is skipped only when a link up IRQ is present.
> > +	 * This flag should not be used to bypass the link up delay for arbitrary
> > +	 * reasons.
> 
> Perhaps by improving the naming of the variable, you don't need 3 lines
> of comment describing the conditional.
> 
> > +	 */
> > +	if (!pp->linkup_irq)
> > +		/* Ignore errors, the link may come up later */
> 
> Does this mean that we will be able to start handling these errors?
> 
> > +		dw_pcie_wait_for_link(pci);
> >  
> >  	bridge->sysdata = pp;
> >  
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > index 347ab74ac35a..539c6d106bb0 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -379,6 +379,7 @@ struct dw_pcie_rp {
> >  	bool			use_atu_msg;
> >  	int			msg_atu_index;
> >  	struct resource		*msg_res;
> > +	bool			linkup_irq;
> 
> Please name this for what it is, rather than some property from which
> some other decision should be derived. (And then you need a comment to
> describe how people should interpret and use it)
> 
> Also, "linkup_irq" sound like an int carrying the interrupt number, not
> a boolean.
> 
> 
> Please call it "use_async_linkup", "use_linkup_irq" or something.
> 

"use_linkup_irq" sounds good to me. But I do like to keep the note above as
there were incidents that people tried to avoid this delay as a "workaround" to
unrelated problems.

- Mani

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

[PATCH v3 2/3] PCI: qcom: Set linkup_irq if global IRQ handler is present

[Krishna chaitanya chundru]

In cases where a global IRQ handler is present to manage link up
interrupts, it may not be necessary to wait for the link to be up
during PCI initialization which optimizes the bootup time.

So, set linkup_irq flag if global IRQ is present and In order to set the
linkup_irq flag before calling dw_pcie_host_init() API, which waits for
link to be up, move platform_get_irq_byname_optional() API
above dw_pcie_host_init().

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index ef44a82be058..474b7525442d 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1692,6 +1692,10 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, pcie);
 
+	irq = platform_get_irq_byname_optional(pdev, "global");
+	if (irq > 0)
+		pp->linkup_irq = true;
+
 	ret = dw_pcie_host_init(pp);
 	if (ret) {
 		dev_err(dev, "cannot initialize host\n");
@@ -1705,7 +1709,6 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 		goto err_host_deinit;
 	}
 
-	irq = platform_get_irq_byname_optional(pdev, "global");
 	if (irq > 0) {
 		ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
 						qcom_pcie_global_irq_thread,

-- 
2.34.1

Re: [PATCH v3 2/3] PCI: qcom: Set linkup_irq if global IRQ handler is present

[Bjorn Andersson]

On Fri, Nov 01, 2024 at 05:04:13PM GMT, Krishna chaitanya chundru wrote:
> In cases where a global IRQ handler is present to manage link up
> interrupts, it may not be necessary to wait for the link to be up
> during PCI initialization which optimizes the bootup time.
> 
> So, set linkup_irq flag if global IRQ is present and In order to set the
> linkup_irq flag before calling dw_pcie_host_init() API, which waits for
> link to be up, move platform_get_irq_byname_optional() API
> above dw_pcie_host_init().
> 
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index ef44a82be058..474b7525442d 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1692,6 +1692,10 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, pcie);
>  
> +	irq = platform_get_irq_byname_optional(pdev, "global");
> +	if (irq > 0)
> +		pp->linkup_irq = true;

This seems to only ever being used in dw_pcie_host_init(), would it make
sense to use a argument to the function to pass the parameter instead of
stashing it in the persistent data structure?

Regards,
Bjorn

> +
>  	ret = dw_pcie_host_init(pp);
>  	if (ret) {
>  		dev_err(dev, "cannot initialize host\n");
> @@ -1705,7 +1709,6 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  		goto err_host_deinit;
>  	}
>  
> -	irq = platform_get_irq_byname_optional(pdev, "global");
>  	if (irq > 0) {
>  		ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
>  						qcom_pcie_global_irq_thread,
> 
> -- 
> 2.34.1
> 
> 

Re: [PATCH v3 2/3] PCI: qcom: Set linkup_irq if global IRQ handler is present

[Krishna Chaitanya Chundru]

On 11/1/2024 9:00 PM, Bjorn Andersson wrote:
> On Fri, Nov 01, 2024 at 05:04:13PM GMT, Krishna chaitanya chundru wrote:
>> In cases where a global IRQ handler is present to manage link up
>> interrupts, it may not be necessary to wait for the link to be up
>> during PCI initialization which optimizes the bootup time.
>>
>> So, set linkup_irq flag if global IRQ is present and In order to set the
>> linkup_irq flag before calling dw_pcie_host_init() API, which waits for
>> link to be up, move platform_get_irq_byname_optional() API
>> above dw_pcie_host_init().
>>
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> ---
>>   drivers/pci/controller/dwc/pcie-qcom.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index ef44a82be058..474b7525442d 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -1692,6 +1692,10 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>>   
>>   	platform_set_drvdata(pdev, pcie);
>>   
>> +	irq = platform_get_irq_byname_optional(pdev, "global");
>> +	if (irq > 0)
>> +		pp->linkup_irq = true;
> 
> This seems to only ever being used in dw_pcie_host_init(), would it make
> sense to use a argument to the function to pass the parameter instead of
> stashing it in the persistent data structure?
> 
dw_pcie_host_init() API is being used by multiple vendors under
drivers/pci/controller/dwc/* it may not be ideal to change the argument
here.

- Krishna Chaitanya.
> Regards,
> Bjorn
> 
>> +
>>   	ret = dw_pcie_host_init(pp);
>>   	if (ret) {
>>   		dev_err(dev, "cannot initialize host\n");
>> @@ -1705,7 +1709,6 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>>   		goto err_host_deinit;
>>   	}
>>   
>> -	irq = platform_get_irq_byname_optional(pdev, "global");
>>   	if (irq > 0) {
>>   		ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
>>   						qcom_pcie_global_irq_thread,
>>
>> -- 
>> 2.34.1
>>
>>

[PATCH v3 3/3] PCI: qcom: Update ICC and OPP values during link up event

[Krishna chaitanya chundru]

As part of the PCIe link up event, update ICC and OPP values
as at this point only driver can know the link speed and
width of the PCIe link.

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.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 474b7525442d..5826c0e7ca0b 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -1558,6 +1558,8 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
 		pci_lock_rescan_remove();
 		pci_rescan_bus(pp->bridge->bus);
 		pci_unlock_rescan_remove();
+
+		qcom_pcie_icc_opp_update(pcie);
 	} else {
 		dev_WARN_ONCE(dev, 1, "Received unknown event. INT_STATUS: 0x%08x\n",
 			      status);

-- 
2.34.1

Re: [PATCH v3 3/3] PCI: qcom: Update ICC and OPP values during link up event

[Bjorn Andersson]

On Fri, Nov 01, 2024 at 05:04:14PM GMT, Krishna chaitanya chundru wrote:
> As part of the PCIe link up event, update ICC and OPP values
> as at this point only driver can know the link speed and
> width of the PCIe link.
> 

It would be nice if you were to write your commit messages in the style
documented at https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
I.e. start with a clear problem description, then move into describing
the solution.

Your commit message is stating that this is the only place the driver
can know the link speed, but wouldn't that imply that there's some
actual problem with the code currently?


I'm guessing (because that's what your commit message is forcing me to
do) that in the case that we don't detect anything connected at probe
time and then we get a "hotplug" interrupt, we will have completely
incorrect bus votes?

If so, it would seem that this patch should have a:
Fixes: 4581403f6792 ("PCI: qcom: Enumerate endpoints based on Link up event in 'global_irq' interrupt")

And of course, a proper description of that problem.

Regards,
Bjorn

> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.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 474b7525442d..5826c0e7ca0b 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1558,6 +1558,8 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
>  		pci_lock_rescan_remove();
>  		pci_rescan_bus(pp->bridge->bus);
>  		pci_unlock_rescan_remove();
> +
> +		qcom_pcie_icc_opp_update(pcie);
>  	} else {
>  		dev_WARN_ONCE(dev, 1, "Received unknown event. INT_STATUS: 0x%08x\n",
>  			      status);
> 
> -- 
> 2.34.1
> 
>