| Message ID | 20240102-j7200-pcie-s2r-v5-5-4b8c46711ded@bootlin.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add suspend to ram support for PCIe on J7200 | expand |
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
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 --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;