From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

dw_pcie_wait_for_link() API waits for the link to be up and returns failure
if the link is not up within the 1 second interval. But if there was no
device connected to the bus, then the link up failure would be expected.
In that case, the callers might want to skip the failure in a hope that the
link will be up later when a device gets connected.

One of the callers, dw_pcie_host_init() is currently skipping the failure
irrespective of the link state, in an assumption that the link may come up
later. But this assumption is wrong, since LTSSM states other than
Detect.Quiet and Detect.Active during link training phase are considered to
be fatal and the link needs to be retrained.

So to avoid callers making wrong assumptions, skip returning failure from
dw_pcie_wait_for_link() only if the link is in Detect.Quiet or
Detect.Active states after timeout and also check the return value of the
API in dw_pcie_host_init().

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-designware-host.c |  8 +++++---
 drivers/pci/controller/dwc/pcie-designware.c      | 12 +++++++++++-
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 43d091128ef7..ef6d9ae6eddb 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -670,9 +670,11 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
 	 * If there is no Link Up IRQ, we should not bypass the delay
 	 * because that would require users to manually rescan for devices.
 	 */
-	if (!pp->use_linkup_irq)
-		/* Ignore errors, the link may come up later */
-		dw_pcie_wait_for_link(pci);
+	if (!pp->use_linkup_irq) {
+		ret = dw_pcie_wait_for_link(pci);
+		if (ret)
+			goto err_stop_link;
+	}
 
 	ret = pci_host_probe(bridge);
 	if (ret)
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 75fc8b767fcc..b58baf26ce58 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -641,7 +641,7 @@ void dw_pcie_disable_atu(struct dw_pcie *pci, u32 dir, int index)
 
 int dw_pcie_wait_for_link(struct dw_pcie *pci)
 {
-	u32 offset, val;
+	u32 offset, val, ltssm;
 	int retries;
 
 	/* Check if the link is up or not */
@@ -653,6 +653,16 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
 	}
 
 	if (retries >= PCIE_LINK_WAIT_MAX_RETRIES) {
+		/*
+		 * If the link is in Detect.Quiet or Detect.Active state, it
+		 * indicates that no device is detected. So return success to
+		 * allow the device to show up later.
+		 */
+		ltssm = dw_pcie_get_ltssm(pci);
+		if (ltssm == DW_PCIE_LTSSM_DETECT_QUIET ||
+		    ltssm == DW_PCIE_LTSSM_DETECT_ACT)
+			return 0;
+
 		dev_info(pci->dev, "Phy link never came up\n");
 		return -ETIMEDOUT;
 	}

-- 
2.48.1

在 2025/12/18 星期四 20:04, Manivannan Sadhasivam via B4 Relay 写道:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> 
> dw_pcie_wait_for_link() API waits for the link to be up and returns failure
> if the link is not up within the 1 second interval. But if there was no
> device connected to the bus, then the link up failure would be expected.
> In that case, the callers might want to skip the failure in a hope that the
> link will be up later when a device gets connected.
> 
> One of the callers, dw_pcie_host_init() is currently skipping the failure
> irrespective of the link state, in an assumption that the link may come up
> later. But this assumption is wrong, since LTSSM states other than
> Detect.Quiet and Detect.Active during link training phase are considered to
> be fatal and the link needs to be retrained.
> 
> So to avoid callers making wrong assumptions, skip returning failure from
> dw_pcie_wait_for_link() only if the link is in Detect.Quiet or
> Detect.Active states after timeout and also check the return value of the
> API in dw_pcie_host_init().
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
>   drivers/pci/controller/dwc/pcie-designware-host.c |  8 +++++---
>   drivers/pci/controller/dwc/pcie-designware.c      | 12 +++++++++++-
>   2 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 43d091128ef7..ef6d9ae6eddb 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -670,9 +670,11 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>   	 * If there is no Link Up IRQ, we should not bypass the delay
>   	 * because that would require users to manually rescan for devices.
>   	 */
> -	if (!pp->use_linkup_irq)
> -		/* Ignore errors, the link may come up later */
> -		dw_pcie_wait_for_link(pci);
> +	if (!pp->use_linkup_irq) {
> +		ret = dw_pcie_wait_for_link(pci);
> +		if (ret)
> +			goto err_stop_link;
> +	}
>   
>   	ret = pci_host_probe(bridge);
>   	if (ret)
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 75fc8b767fcc..b58baf26ce58 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -641,7 +641,7 @@ void dw_pcie_disable_atu(struct dw_pcie *pci, u32 dir, int index)
>   
>   int dw_pcie_wait_for_link(struct dw_pcie *pci)
>   {
> -	u32 offset, val;
> +	u32 offset, val, ltssm;
>   	int retries;
>   
>   	/* Check if the link is up or not */
> @@ -653,6 +653,16 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
>   	}
>   
>   	if (retries >= PCIE_LINK_WAIT_MAX_RETRIES) {
> +		/*
> +		 * If the link is in Detect.Quiet or Detect.Active state, it
> +		 * indicates that no device is detected. So return success to
> +		 * allow the device to show up later.
> +		 */
> +		ltssm = dw_pcie_get_ltssm(pci);
> +		if (ltssm == DW_PCIE_LTSSM_DETECT_QUIET ||
> +		    ltssm == DW_PCIE_LTSSM_DETECT_ACT)
> +			return 0;

By looking more closely, this changes the behaviour of pcie-tegra194.c
which relies on it in tegra_pcie_dw_start_link() to do some retries.

pcie-intel-gw.c/pci-imx6.c also continue to do some setups in this case,
not sure if it's safe.

> +
>   		dev_info(pci->dev, "Phy link never came up\n");
>   		return -ETIMEDOUT;
>   	}
> 

On Thu, Dec 18, 2025 at 08:45:15PM +0800, Shawn Lin wrote:
> 在 2025/12/18 星期四 20:04, Manivannan Sadhasivam via B4 Relay 写道:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > 
> > dw_pcie_wait_for_link() API waits for the link to be up and returns failure
> > if the link is not up within the 1 second interval. But if there was no
> > device connected to the bus, then the link up failure would be expected.
> > In that case, the callers might want to skip the failure in a hope that the
> > link will be up later when a device gets connected.
> > 
> > One of the callers, dw_pcie_host_init() is currently skipping the failure
> > irrespective of the link state, in an assumption that the link may come up
> > later. But this assumption is wrong, since LTSSM states other than
> > Detect.Quiet and Detect.Active during link training phase are considered to
> > be fatal and the link needs to be retrained.
> > 
> > So to avoid callers making wrong assumptions, skip returning failure from
> > dw_pcie_wait_for_link() only if the link is in Detect.Quiet or
> > Detect.Active states after timeout and also check the return value of the
> > API in dw_pcie_host_init().
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > ---
> >   drivers/pci/controller/dwc/pcie-designware-host.c |  8 +++++---
> >   drivers/pci/controller/dwc/pcie-designware.c      | 12 +++++++++++-
> >   2 files changed, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index 43d091128ef7..ef6d9ae6eddb 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -670,9 +670,11 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> >   	 * If there is no Link Up IRQ, we should not bypass the delay
> >   	 * because that would require users to manually rescan for devices.
> >   	 */
> > -	if (!pp->use_linkup_irq)
> > -		/* Ignore errors, the link may come up later */
> > -		dw_pcie_wait_for_link(pci);
> > +	if (!pp->use_linkup_irq) {
> > +		ret = dw_pcie_wait_for_link(pci);
> > +		if (ret)
> > +			goto err_stop_link;
> > +	}
> >   	ret = pci_host_probe(bridge);
> >   	if (ret)
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index 75fc8b767fcc..b58baf26ce58 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -641,7 +641,7 @@ void dw_pcie_disable_atu(struct dw_pcie *pci, u32 dir, int index)
> >   int dw_pcie_wait_for_link(struct dw_pcie *pci)
> >   {
> > -	u32 offset, val;
> > +	u32 offset, val, ltssm;
> >   	int retries;
> >   	/* Check if the link is up or not */
> > @@ -653,6 +653,16 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
> >   	}
> >   	if (retries >= PCIE_LINK_WAIT_MAX_RETRIES) {
> > +		/*
> > +		 * If the link is in Detect.Quiet or Detect.Active state, it
> > +		 * indicates that no device is detected. So return success to
> > +		 * allow the device to show up later.
> > +		 */
> > +		ltssm = dw_pcie_get_ltssm(pci);
> > +		if (ltssm == DW_PCIE_LTSSM_DETECT_QUIET ||
> > +		    ltssm == DW_PCIE_LTSSM_DETECT_ACT)
> > +			return 0;
> 
> By looking more closely, this changes the behaviour of pcie-tegra194.c
> which relies on it in tegra_pcie_dw_start_link() to do some retries.
> 
> pcie-intel-gw.c/pci-imx6.c also continue to do some setups in this case,
> not sure if it's safe.
> 

Hmm, I agree. I did check the callers during v1, but fail to notice pci-imx6
which writes to LNKCAP and PCIE_LINK_WIDTH_SPEED_CONTROL registers after
success.

To be on the safe side, dw_pcie_wait_for_link() should return -ENODEV if the
device is not found and the callers can check for this errno and skip the
failure.

- Mani

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

On 12/18/2025 5:34 PM, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
>
> dw_pcie_wait_for_link() API waits for the link to be up and returns failure
> if the link is not up within the 1 second interval. But if there was no
> device connected to the bus, then the link up failure would be expected.
> In that case, the callers might want to skip the failure in a hope that the
> link will be up later when a device gets connected.
>
> One of the callers, dw_pcie_host_init() is currently skipping the failure
> irrespective of the link state, in an assumption that the link may come up
> later. But this assumption is wrong, since LTSSM states other than
> Detect.Quiet and Detect.Active during link training phase are considered to
> be fatal and the link needs to be retrained.
>
> So to avoid callers making wrong assumptions, skip returning failure from
> dw_pcie_wait_for_link() only if the link is in Detect.Quiet or
> Detect.Active states after timeout and also check the return value of the
> API in dw_pcie_host_init().
>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
>   drivers/pci/controller/dwc/pcie-designware-host.c |  8 +++++---
>   drivers/pci/controller/dwc/pcie-designware.c      | 12 +++++++++++-
>   2 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 43d091128ef7..ef6d9ae6eddb 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -670,9 +670,11 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>   	 * If there is no Link Up IRQ, we should not bypass the delay
>   	 * because that would require users to manually rescan for devices.
>   	 */
> -	if (!pp->use_linkup_irq)
> -		/* Ignore errors, the link may come up later */
> -		dw_pcie_wait_for_link(pci);
> +	if (!pp->use_linkup_irq) {
> +		ret = dw_pcie_wait_for_link(pci);
> +		if (ret)
> +			goto err_stop_link;
> +	}
>   
>   	ret = pci_host_probe(bridge);
>   	if (ret)
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 75fc8b767fcc..b58baf26ce58 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -641,7 +641,7 @@ void dw_pcie_disable_atu(struct dw_pcie *pci, u32 dir, int index)
>   
>   int dw_pcie_wait_for_link(struct dw_pcie *pci)
>   {
> -	u32 offset, val;
> +	u32 offset, val, ltssm;
>   	int retries;
>   
>   	/* Check if the link is up or not */
> @@ -653,6 +653,16 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
>   	}
>   
>   	if (retries >= PCIE_LINK_WAIT_MAX_RETRIES) {
> +		/*
> +		 * If the link is in Detect.Quiet or Detect.Active state, it
> +		 * indicates that no device is detected. So return success to
> +		 * allow the device to show up later.
> +		 */
> +		ltssm = dw_pcie_get_ltssm(pci);
> +		if (ltssm == DW_PCIE_LTSSM_DETECT_QUIET ||
> +		    ltssm == DW_PCIE_LTSSM_DETECT_ACT)
> +			return 0;
> +
>   		dev_info(pci->dev, "Phy link never came up\n");
Can you move this print above, as this print is useful for the user to 
know that, link is not up yet.

- Krishna Chaitanya.
>   		return -ETIMEDOUT;
>   	}
>

On Thu, Dec 18, 2025 at 05:57:30PM +0530, Krishna Chaitanya Chundru wrote:
> 
> 
> On 12/18/2025 5:34 PM, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > 
> > dw_pcie_wait_for_link() API waits for the link to be up and returns failure
> > if the link is not up within the 1 second interval. But if there was no
> > device connected to the bus, then the link up failure would be expected.
> > In that case, the callers might want to skip the failure in a hope that the
> > link will be up later when a device gets connected.
> > 
> > One of the callers, dw_pcie_host_init() is currently skipping the failure
> > irrespective of the link state, in an assumption that the link may come up
> > later. But this assumption is wrong, since LTSSM states other than
> > Detect.Quiet and Detect.Active during link training phase are considered to
> > be fatal and the link needs to be retrained.
> > 
> > So to avoid callers making wrong assumptions, skip returning failure from
> > dw_pcie_wait_for_link() only if the link is in Detect.Quiet or
> > Detect.Active states after timeout and also check the return value of the
> > API in dw_pcie_host_init().
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > ---
> >   drivers/pci/controller/dwc/pcie-designware-host.c |  8 +++++---
> >   drivers/pci/controller/dwc/pcie-designware.c      | 12 +++++++++++-
> >   2 files changed, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index 43d091128ef7..ef6d9ae6eddb 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -670,9 +670,11 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> >   	 * If there is no Link Up IRQ, we should not bypass the delay
> >   	 * because that would require users to manually rescan for devices.
> >   	 */
> > -	if (!pp->use_linkup_irq)
> > -		/* Ignore errors, the link may come up later */
> > -		dw_pcie_wait_for_link(pci);
> > +	if (!pp->use_linkup_irq) {
> > +		ret = dw_pcie_wait_for_link(pci);
> > +		if (ret)
> > +			goto err_stop_link;
> > +	}
> >   	ret = pci_host_probe(bridge);
> >   	if (ret)
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index 75fc8b767fcc..b58baf26ce58 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -641,7 +641,7 @@ void dw_pcie_disable_atu(struct dw_pcie *pci, u32 dir, int index)
> >   int dw_pcie_wait_for_link(struct dw_pcie *pci)
> >   {
> > -	u32 offset, val;
> > +	u32 offset, val, ltssm;
> >   	int retries;
> >   	/* Check if the link is up or not */
> > @@ -653,6 +653,16 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
> >   	}
> >   	if (retries >= PCIE_LINK_WAIT_MAX_RETRIES) {
> > +		/*
> > +		 * If the link is in Detect.Quiet or Detect.Active state, it
> > +		 * indicates that no device is detected. So return success to
> > +		 * allow the device to show up later.
> > +		 */
> > +		ltssm = dw_pcie_get_ltssm(pci);
> > +		if (ltssm == DW_PCIE_LTSSM_DETECT_QUIET ||
> > +		    ltssm == DW_PCIE_LTSSM_DETECT_ACT)
> > +			return 0;
> > +
> >   		dev_info(pci->dev, "Phy link never came up\n");
> Can you move this print above, as this print is useful for the user to know
> that, link is not up yet.
> 

If the device is not connected to the bus, what information does this log
provide to the user?

- Mani

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

On 12/18/2025 6:16 PM, Manivannan Sadhasivam wrote:
> On Thu, Dec 18, 2025 at 05:57:30PM +0530, Krishna Chaitanya Chundru wrote:
>>
>> On 12/18/2025 5:34 PM, Manivannan Sadhasivam via B4 Relay wrote:
>>> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
>>>
>>> dw_pcie_wait_for_link() API waits for the link to be up and returns failure
>>> if the link is not up within the 1 second interval. But if there was no
>>> device connected to the bus, then the link up failure would be expected.
>>> In that case, the callers might want to skip the failure in a hope that the
>>> link will be up later when a device gets connected.
>>>
>>> One of the callers, dw_pcie_host_init() is currently skipping the failure
>>> irrespective of the link state, in an assumption that the link may come up
>>> later. But this assumption is wrong, since LTSSM states other than
>>> Detect.Quiet and Detect.Active during link training phase are considered to
>>> be fatal and the link needs to be retrained.
>>>
>>> So to avoid callers making wrong assumptions, skip returning failure from
>>> dw_pcie_wait_for_link() only if the link is in Detect.Quiet or
>>> Detect.Active states after timeout and also check the return value of the
>>> API in dw_pcie_host_init().
>>>
>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
>>> ---
>>>    drivers/pci/controller/dwc/pcie-designware-host.c |  8 +++++---
>>>    drivers/pci/controller/dwc/pcie-designware.c      | 12 +++++++++++-
>>>    2 files changed, 16 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>>> index 43d091128ef7..ef6d9ae6eddb 100644
>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>>> @@ -670,9 +670,11 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>>>    	 * If there is no Link Up IRQ, we should not bypass the delay
>>>    	 * because that would require users to manually rescan for devices.
>>>    	 */
>>> -	if (!pp->use_linkup_irq)
>>> -		/* Ignore errors, the link may come up later */
>>> -		dw_pcie_wait_for_link(pci);
>>> +	if (!pp->use_linkup_irq) {
>>> +		ret = dw_pcie_wait_for_link(pci);
>>> +		if (ret)
>>> +			goto err_stop_link;
>>> +	}
>>>    	ret = pci_host_probe(bridge);
>>>    	if (ret)
>>> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
>>> index 75fc8b767fcc..b58baf26ce58 100644
>>> --- a/drivers/pci/controller/dwc/pcie-designware.c
>>> +++ b/drivers/pci/controller/dwc/pcie-designware.c
>>> @@ -641,7 +641,7 @@ void dw_pcie_disable_atu(struct dw_pcie *pci, u32 dir, int index)
>>>    int dw_pcie_wait_for_link(struct dw_pcie *pci)
>>>    {
>>> -	u32 offset, val;
>>> +	u32 offset, val, ltssm;
>>>    	int retries;
>>>    	/* Check if the link is up or not */
>>> @@ -653,6 +653,16 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
>>>    	}
>>>    	if (retries >= PCIE_LINK_WAIT_MAX_RETRIES) {
>>> +		/*
>>> +		 * If the link is in Detect.Quiet or Detect.Active state, it
>>> +		 * indicates that no device is detected. So return success to
>>> +		 * allow the device to show up later.
>>> +		 */
>>> +		ltssm = dw_pcie_get_ltssm(pci);
>>> +		if (ltssm == DW_PCIE_LTSSM_DETECT_QUIET ||
>>> +		    ltssm == DW_PCIE_LTSSM_DETECT_ACT)
>>> +			return 0;
>>> +
>>>    		dev_info(pci->dev, "Phy link never came up\n");
>> Can you move this print above, as this print is useful for the user to know
>> that, link is not up yet.
>>
> If the device is not connected to the bus, what information does this log
> provide to the user?
Not every user is aware that device is not connected, at-least this log 
will give info
that there is no device connected.

- Krishna Chaitanya.
> - Mani
>

On Thu, Dec 18, 2025 at 06:26:12PM +0530, Krishna Chaitanya Chundru wrote:
> 
> 
> On 12/18/2025 6:16 PM, Manivannan Sadhasivam wrote:
> > On Thu, Dec 18, 2025 at 05:57:30PM +0530, Krishna Chaitanya Chundru wrote:
> > > 
> > > On 12/18/2025 5:34 PM, Manivannan Sadhasivam via B4 Relay wrote:
> > > > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > > > 
> > > > dw_pcie_wait_for_link() API waits for the link to be up and returns failure
> > > > if the link is not up within the 1 second interval. But if there was no
> > > > device connected to the bus, then the link up failure would be expected.
> > > > In that case, the callers might want to skip the failure in a hope that the
> > > > link will be up later when a device gets connected.
> > > > 
> > > > One of the callers, dw_pcie_host_init() is currently skipping the failure
> > > > irrespective of the link state, in an assumption that the link may come up
> > > > later. But this assumption is wrong, since LTSSM states other than
> > > > Detect.Quiet and Detect.Active during link training phase are considered to
> > > > be fatal and the link needs to be retrained.
> > > > 
> > > > So to avoid callers making wrong assumptions, skip returning failure from
> > > > dw_pcie_wait_for_link() only if the link is in Detect.Quiet or
> > > > Detect.Active states after timeout and also check the return value of the
> > > > API in dw_pcie_host_init().
> > > > 
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > > > ---
> > > >    drivers/pci/controller/dwc/pcie-designware-host.c |  8 +++++---
> > > >    drivers/pci/controller/dwc/pcie-designware.c      | 12 +++++++++++-
> > > >    2 files changed, 16 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > index 43d091128ef7..ef6d9ae6eddb 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > @@ -670,9 +670,11 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> > > >    	 * If there is no Link Up IRQ, we should not bypass the delay
> > > >    	 * because that would require users to manually rescan for devices.
> > > >    	 */
> > > > -	if (!pp->use_linkup_irq)
> > > > -		/* Ignore errors, the link may come up later */
> > > > -		dw_pcie_wait_for_link(pci);
> > > > +	if (!pp->use_linkup_irq) {
> > > > +		ret = dw_pcie_wait_for_link(pci);
> > > > +		if (ret)
> > > > +			goto err_stop_link;
> > > > +	}
> > > >    	ret = pci_host_probe(bridge);
> > > >    	if (ret)
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > > > index 75fc8b767fcc..b58baf26ce58 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > > @@ -641,7 +641,7 @@ void dw_pcie_disable_atu(struct dw_pcie *pci, u32 dir, int index)
> > > >    int dw_pcie_wait_for_link(struct dw_pcie *pci)
> > > >    {
> > > > -	u32 offset, val;
> > > > +	u32 offset, val, ltssm;
> > > >    	int retries;
> > > >    	/* Check if the link is up or not */
> > > > @@ -653,6 +653,16 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
> > > >    	}
> > > >    	if (retries >= PCIE_LINK_WAIT_MAX_RETRIES) {
> > > > +		/*
> > > > +		 * If the link is in Detect.Quiet or Detect.Active state, it
> > > > +		 * indicates that no device is detected. So return success to
> > > > +		 * allow the device to show up later.
> > > > +		 */
> > > > +		ltssm = dw_pcie_get_ltssm(pci);
> > > > +		if (ltssm == DW_PCIE_LTSSM_DETECT_QUIET ||
> > > > +		    ltssm == DW_PCIE_LTSSM_DETECT_ACT)
> > > > +			return 0;
> > > > +
> > > >    		dev_info(pci->dev, "Phy link never came up\n");
> > > Can you move this print above, as this print is useful for the user to know
> > > that, link is not up yet.
> > > 
> > If the device is not connected to the bus, what information does this log
> > provide to the user?
> Not every user is aware that device is not connected, at-least this log will
> give info
> that there is no device connected.
> 

Users won't grep the dmesg log to check whether the device is connected to the
bus or not. They will use lspci.

- Mani

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

On 12/18/2025 6:30 PM, Manivannan Sadhasivam wrote:
> On Thu, Dec 18, 2025 at 06:26:12PM +0530, Krishna Chaitanya Chundru wrote:
>>
>> On 12/18/2025 6:16 PM, Manivannan Sadhasivam wrote:
>>> On Thu, Dec 18, 2025 at 05:57:30PM +0530, Krishna Chaitanya Chundru wrote:
>>>> On 12/18/2025 5:34 PM, Manivannan Sadhasivam via B4 Relay wrote:
>>>>> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
>>>>>
>>>>> dw_pcie_wait_for_link() API waits for the link to be up and returns failure
>>>>> if the link is not up within the 1 second interval. But if there was no
>>>>> device connected to the bus, then the link up failure would be expected.
>>>>> In that case, the callers might want to skip the failure in a hope that the
>>>>> link will be up later when a device gets connected.
>>>>>
>>>>> One of the callers, dw_pcie_host_init() is currently skipping the failure
>>>>> irrespective of the link state, in an assumption that the link may come up
>>>>> later. But this assumption is wrong, since LTSSM states other than
>>>>> Detect.Quiet and Detect.Active during link training phase are considered to
>>>>> be fatal and the link needs to be retrained.
>>>>>
>>>>> So to avoid callers making wrong assumptions, skip returning failure from
>>>>> dw_pcie_wait_for_link() only if the link is in Detect.Quiet or
>>>>> Detect.Active states after timeout and also check the return value of the
>>>>> API in dw_pcie_host_init().
>>>>>
>>>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
>>>>> ---
>>>>>     drivers/pci/controller/dwc/pcie-designware-host.c |  8 +++++---
>>>>>     drivers/pci/controller/dwc/pcie-designware.c      | 12 +++++++++++-
>>>>>     2 files changed, 16 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>> index 43d091128ef7..ef6d9ae6eddb 100644
>>>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>> @@ -670,9 +670,11 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>>>>>     	 * If there is no Link Up IRQ, we should not bypass the delay
>>>>>     	 * because that would require users to manually rescan for devices.
>>>>>     	 */
>>>>> -	if (!pp->use_linkup_irq)
>>>>> -		/* Ignore errors, the link may come up later */
>>>>> -		dw_pcie_wait_for_link(pci);
>>>>> +	if (!pp->use_linkup_irq) {
>>>>> +		ret = dw_pcie_wait_for_link(pci);
>>>>> +		if (ret)
>>>>> +			goto err_stop_link;
>>>>> +	}
>>>>>     	ret = pci_host_probe(bridge);
>>>>>     	if (ret)
>>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
>>>>> index 75fc8b767fcc..b58baf26ce58 100644
>>>>> --- a/drivers/pci/controller/dwc/pcie-designware.c
>>>>> +++ b/drivers/pci/controller/dwc/pcie-designware.c
>>>>> @@ -641,7 +641,7 @@ void dw_pcie_disable_atu(struct dw_pcie *pci, u32 dir, int index)
>>>>>     int dw_pcie_wait_for_link(struct dw_pcie *pci)
>>>>>     {
>>>>> -	u32 offset, val;
>>>>> +	u32 offset, val, ltssm;
>>>>>     	int retries;
>>>>>     	/* Check if the link is up or not */
>>>>> @@ -653,6 +653,16 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
>>>>>     	}
>>>>>     	if (retries >= PCIE_LINK_WAIT_MAX_RETRIES) {
>>>>> +		/*
>>>>> +		 * If the link is in Detect.Quiet or Detect.Active state, it
>>>>> +		 * indicates that no device is detected. So return success to
>>>>> +		 * allow the device to show up later.
>>>>> +		 */
>>>>> +		ltssm = dw_pcie_get_ltssm(pci);
>>>>> +		if (ltssm == DW_PCIE_LTSSM_DETECT_QUIET ||
>>>>> +		    ltssm == DW_PCIE_LTSSM_DETECT_ACT)
>>>>> +			return 0;
>>>>> +
>>>>>     		dev_info(pci->dev, "Phy link never came up\n");
>>>> Can you move this print above, as this print is useful for the user to know
>>>> that, link is not up yet.
>>>>
>>> If the device is not connected to the bus, what information does this log
>>> provide to the user?
>> Not every user is aware that device is not connected, at-least this log will
>> give info
>> that there is no device connected.
>>
> Users won't grep the dmesg log to check whether the device is connected to the
> bus or not. They will use lspci.
ack.

- Krishna Chaitanya.
> - Mani
>