diff mbox series

[v5,05/11] PCI: cadence: Extract link setup sequence from cdns_pcie_host_setup()

Message ID 20240102-j7200-pcie-s2r-v5-5-4b8c46711ded@bootlin.com (mailing list archive)
State New, archived
Headers show
Series Add suspend to ram support for PCIe on J7200 | expand

Commit Message

Thomas Richard April 16, 2024, 1:29 p.m. UTC 
The function cdns_pcie_host_setup() mixes probe structure and link setup.

The link setup must be done during the resume sequence. So extract it from
cdns_pcie_host_setup() and create a dedicated function.

Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com>
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/pci/controller/cadence/pcie-cadence-host.c | 39 ++++++++++++++--------
 drivers/pci/controller/cadence/pcie-cadence.h      |  6 ++++
 2 files changed, 32 insertions(+), 13 deletions(-)

Comments

Dan Carpenter April 16, 2024, 2:16 p.m. UTC | #1 
On Tue, Apr 16, 2024 at 03:29:54PM +0200, Thomas Richard wrote:
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> index 5b14f7ee3c79..93d9922730af 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> @@ -497,6 +497,30 @@ static int cdns_pcie_host_init(struct device *dev,
>  	return cdns_pcie_host_init_address_translation(rc);
>  }
>  
> +int cdns_pcie_host_link_setup(struct cdns_pcie_rc *rc)
> +{
> +	struct cdns_pcie *pcie = &rc->pcie;
> +	struct device *dev = rc->pcie.dev;
> +	int ret;
> +
> +	if (rc->quirk_detect_quiet_flag)
> +		cdns_pcie_detect_quiet_min_delay_set(&rc->pcie);
> +
> +	cdns_pcie_host_enable_ptm_response(pcie);
> +
> +	ret = cdns_pcie_start_link(pcie);
> +	if (ret) {
> +		dev_err(dev, "Failed to start link\n");
> +		return ret;
> +	}
> +
> +	ret = cdns_pcie_host_start_link(rc);
> +	if (ret)
> +		dev_dbg(dev, "PCIe link never came up\n");

If we're going to ignore this error the message should be a dev_err()
at least.


> +
> +	return 0;
> +}
> +

regards,
dan carpenter
Thomas Richard April 16, 2024, 4:01 p.m. UTC | #2 
On 4/16/24 16:16, Dan Carpenter wrote:
> On Tue, Apr 16, 2024 at 03:29:54PM +0200, Thomas Richard wrote:
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> index 5b14f7ee3c79..93d9922730af 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> @@ -497,6 +497,30 @@ static int cdns_pcie_host_init(struct device *dev,
>>  	return cdns_pcie_host_init_address_translation(rc);
>>  }
>>  
>> +int cdns_pcie_host_link_setup(struct cdns_pcie_rc *rc)
>> +{
>> +	struct cdns_pcie *pcie = &rc->pcie;
>> +	struct device *dev = rc->pcie.dev;
>> +	int ret;
>> +
>> +	if (rc->quirk_detect_quiet_flag)
>> +		cdns_pcie_detect_quiet_min_delay_set(&rc->pcie);
>> +
>> +	cdns_pcie_host_enable_ptm_response(pcie);
>> +
>> +	ret = cdns_pcie_start_link(pcie);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to start link\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = cdns_pcie_host_start_link(rc);
>> +	if (ret)
>> +		dev_dbg(dev, "PCIe link never came up\n");
> 
> If we're going to ignore this error the message should be a dev_err()
> at least.

Hello Dan,

In fact we already ignore this error [1]
I only moved the hardware configuration part of cdns_pcie_host_setup()
into a new function cdns_pcie_host_link_setup().

But I can use this patch to switch to dev_err() if needed.

[1]
https://elixir.bootlin.com/linux/v6.9-rc4/source/drivers/pci/controller/cadence/pcie-cadence-host.c#L549

Regards,

Thomas
Thomas Richard May 14, 2024, 1:15 p.m. UTC | #3 
On 4/16/24 16:16, Dan Carpenter wrote:
> On Tue, Apr 16, 2024 at 03:29:54PM +0200, Thomas Richard wrote:
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> index 5b14f7ee3c79..93d9922730af 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> @@ -497,6 +497,30 @@ static int cdns_pcie_host_init(struct device *dev,
>>  	return cdns_pcie_host_init_address_translation(rc);
>>  }
>>  
>> +int cdns_pcie_host_link_setup(struct cdns_pcie_rc *rc)
>> +{
>> +	struct cdns_pcie *pcie = &rc->pcie;
>> +	struct device *dev = rc->pcie.dev;
>> +	int ret;
>> +
>> +	if (rc->quirk_detect_quiet_flag)
>> +		cdns_pcie_detect_quiet_min_delay_set(&rc->pcie);
>> +
>> +	cdns_pcie_host_enable_ptm_response(pcie);
>> +
>> +	ret = cdns_pcie_start_link(pcie);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to start link\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = cdns_pcie_host_start_link(rc);
>> +	if (ret)
>> +		dev_dbg(dev, "PCIe link never came up\n");
> 
> If we're going to ignore this error the message should be a dev_err()
> at least.

Hello Dan,

In fact it could not be really an error.
If you physically don't have a device on the PCIe bus,
cdns_pcie_host_start_link() will not return 0.

So if we use dev_err(), we will always have the error if there is no
device on the PCIe bus.

Regards,

Thomas
Dan Carpenter May 15, 2024, 7:05 a.m. UTC | #4 
On Tue, May 14, 2024 at 03:15:34PM +0200, Thomas Richard wrote:
> On 4/16/24 16:16, Dan Carpenter wrote:
> > On Tue, Apr 16, 2024 at 03:29:54PM +0200, Thomas Richard wrote:
> >> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> >> index 5b14f7ee3c79..93d9922730af 100644
> >> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> >> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> >> @@ -497,6 +497,30 @@ static int cdns_pcie_host_init(struct device *dev,
> >>  	return cdns_pcie_host_init_address_translation(rc);
> >>  }
> >>  
> >> +int cdns_pcie_host_link_setup(struct cdns_pcie_rc *rc)
> >> +{
> >> +	struct cdns_pcie *pcie = &rc->pcie;
> >> +	struct device *dev = rc->pcie.dev;
> >> +	int ret;
> >> +
> >> +	if (rc->quirk_detect_quiet_flag)
> >> +		cdns_pcie_detect_quiet_min_delay_set(&rc->pcie);
> >> +
> >> +	cdns_pcie_host_enable_ptm_response(pcie);
> >> +
> >> +	ret = cdns_pcie_start_link(pcie);
> >> +	if (ret) {
> >> +		dev_err(dev, "Failed to start link\n");
> >> +		return ret;
> >> +	}
> >> +
> >> +	ret = cdns_pcie_host_start_link(rc);
> >> +	if (ret)
> >> +		dev_dbg(dev, "PCIe link never came up\n");
> > 
> > If we're going to ignore this error the message should be a dev_err()
> > at least.
> 
> Hello Dan,
> 
> In fact it could not be really an error.
> If you physically don't have a device on the PCIe bus,
> cdns_pcie_host_start_link() will not return 0.
> 
> So if we use dev_err(), we will always have the error if there is no
> device on the PCIe bus.

Ah okay.  Thanks for looking at this.  It feels like maybe
cdns_pcie_host_start_link() should just check for that at the start and
return 0 instead of doing waiting 1 second and returning -ETIMEOUT.
But I don't know this code well enough to say if that's even possible.

regards,
dan carpenter
diff mbox series

Patch

diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
index 5b14f7ee3c79..93d9922730af 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-host.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
@@ -497,6 +497,30 @@  static int cdns_pcie_host_init(struct device *dev,
 	return cdns_pcie_host_init_address_translation(rc);
 }
 
+int cdns_pcie_host_link_setup(struct cdns_pcie_rc *rc)
+{
+	struct cdns_pcie *pcie = &rc->pcie;
+	struct device *dev = rc->pcie.dev;
+	int ret;
+
+	if (rc->quirk_detect_quiet_flag)
+		cdns_pcie_detect_quiet_min_delay_set(&rc->pcie);
+
+	cdns_pcie_host_enable_ptm_response(pcie);
+
+	ret = cdns_pcie_start_link(pcie);
+	if (ret) {
+		dev_err(dev, "Failed to start link\n");
+		return ret;
+	}
+
+	ret = cdns_pcie_host_start_link(rc);
+	if (ret)
+		dev_dbg(dev, "PCIe link never came up\n");
+
+	return 0;
+}
+
 int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 {
 	struct device *dev = rc->pcie.dev;
@@ -533,20 +557,9 @@  int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 		return PTR_ERR(rc->cfg_base);
 	rc->cfg_res = res;
 
-	if (rc->quirk_detect_quiet_flag)
-		cdns_pcie_detect_quiet_min_delay_set(&rc->pcie);
-
-	cdns_pcie_host_enable_ptm_response(pcie);
-
-	ret = cdns_pcie_start_link(pcie);
-	if (ret) {
-		dev_err(dev, "Failed to start link\n");
-		return ret;
-	}
-
-	ret = cdns_pcie_host_start_link(rc);
+	ret = cdns_pcie_host_link_setup(rc);
 	if (ret)
-		dev_dbg(dev, "PCIe link never came up\n");
+		return ret;
 
 	for (bar = RP_BAR0; bar <= RP_NO_BAR; bar++)
 		rc->avail_ib_bar[bar] = true;
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index 7a66a2f815dc..1d37d5f9f811 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -521,10 +521,16 @@  static inline bool cdns_pcie_link_up(struct cdns_pcie *pcie)
 }
 
 #ifdef CONFIG_PCIE_CADENCE_HOST
+int cdns_pcie_host_link_setup(struct cdns_pcie_rc *rc);
 int cdns_pcie_host_setup(struct cdns_pcie_rc *rc);
 void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
 			       int where);
 #else
+static inline int cdns_pcie_host_link_setup(struct cdns_pcie_rc *rc)
+{
+	return 0;
+}
+
 static inline int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 {
 	return 0;