| Message ID | 20220324013734.18234-4-Sergey.Semin@baikalelectronics.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: dwc: Add dma-ranges/YAML-schema/Baikal-T1 support | expand |
On Thu, 2022-03-24 at 04:37 +0300, Serge Semin wrote: > Printing just "link up" isn't that much informative especially when it > comes to working with the PCI Express bus. Even if the link is up, due to > multiple reasons the bus performance can degrade to slower speeds or to > narrower width than both Root Port and its partner is capable of. In that > case it would be handy to know the link specifications as early as > possible. So let's add a more verbose message to the busy-wait link-state > method, which will contain the link speed generation and the PCIe bus > width in case if the link up state is discovered. Otherwise an error will > be printed to the system log. > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > --- > drivers/pci/controller/dwc/pcie-designware.c | 22 +++++++++++++++----- > 1 file changed, 17 insertions(+), 5 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c [] > @@ -528,14 +528,26 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci) > > /* Check if the link is up or not */ > for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) { > - if (dw_pcie_link_up(pci)) { > - dev_info(pci->dev, "Link up\n"); > - return 0; > - } > + if (dw_pcie_link_up(pci)) > + break; > + > usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX); > } > > - dev_info(pci->dev, "Phy link never came up\n"); > + if (retries < LINK_WAIT_MAX_RETRIES) { > + u32 offset, val; > + > + offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP); > + val = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA); > + > + dev_info(pci->dev, "PCIe Gen.%u x%u link up\n", > + FIELD_GET(PCI_EXP_LNKSTA_CLS, val), > + FIELD_GET(PCI_EXP_LNKSTA_NLW, val)); > + > + return 0; > + } > + > + dev_err(pci->dev, "Phy link never came up\n"); > > return -ETIMEDOUT; > } IMO: it's generally bette to test the error condition and unindent the typical return. if (retries >= LINK_WAIT_MAX_RETRIES) { dev_err(pci->dev, "Phy link never came up\n"); return -ETIMEDOUT; } offset = ... val = ... dev_info(...) return 0; }
On Thu, Mar 24, 2022 at 04:37:21AM +0300, Serge Semin wrote: > Printing just "link up" isn't that much informative especially when it > comes to working with the PCI Express bus. Even if the link is up, due to > multiple reasons the bus performance can degrade to slower speeds or to > narrower width than both Root Port and its partner is capable of. In that > case it would be handy to know the link specifications as early as > possible. So let's add a more verbose message to the busy-wait link-state > method, which will contain the link speed generation and the PCIe bus > width in case if the link up state is discovered. Otherwise an error will > be printed to the system log. > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > --- > drivers/pci/controller/dwc/pcie-designware.c | 22 +++++++++++++++----- > 1 file changed, 17 insertions(+), 5 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > index 6e81264fdfb4..f1693e25afcb 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.c > +++ b/drivers/pci/controller/dwc/pcie-designware.c > @@ -528,14 +528,26 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci) > > /* Check if the link is up or not */ > for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) { > - if (dw_pcie_link_up(pci)) { > - dev_info(pci->dev, "Link up\n"); > - return 0; > - } > + if (dw_pcie_link_up(pci)) > + break; > + > usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX); > } > > - dev_info(pci->dev, "Phy link never came up\n"); > + if (retries < LINK_WAIT_MAX_RETRIES) { > + u32 offset, val; > + > + offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP); > + val = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA); > + > + dev_info(pci->dev, "PCIe Gen.%u x%u link up\n", > + FIELD_GET(PCI_EXP_LNKSTA_CLS, val), > + FIELD_GET(PCI_EXP_LNKSTA_NLW, val)); Given these are standard registers can we do this in the core code? The main issue I think is that the config space accessors don't work until you create the bus struct. That still should be early enough. I think it is possible some implementations don't report the link state in these registers. Maybe we don't really need to care. Rob
On Mon, Mar 28, 2022 at 04:31:53PM -0700, Joe Perches wrote: > On Thu, 2022-03-24 at 04:37 +0300, Serge Semin wrote: > > Printing just "link up" isn't that much informative especially when it > > comes to working with the PCI Express bus. Even if the link is up, due to > > multiple reasons the bus performance can degrade to slower speeds or to > > narrower width than both Root Port and its partner is capable of. In that > > case it would be handy to know the link specifications as early as > > possible. So let's add a more verbose message to the busy-wait link-state > > method, which will contain the link speed generation and the PCIe bus > > width in case if the link up state is discovered. Otherwise an error will > > be printed to the system log. > > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > --- > > drivers/pci/controller/dwc/pcie-designware.c | 22 +++++++++++++++----- > > 1 file changed, 17 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > [] > > @@ -528,14 +528,26 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci) > > > > /* Check if the link is up or not */ > > for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) { > > - if (dw_pcie_link_up(pci)) { > > - dev_info(pci->dev, "Link up\n"); > > - return 0; > > - } > > + if (dw_pcie_link_up(pci)) > > + break; > > + > > usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX); > > } > > > > - dev_info(pci->dev, "Phy link never came up\n"); > > + if (retries < LINK_WAIT_MAX_RETRIES) { > > + u32 offset, val; > > + > > + offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP); > > + val = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA); > > + > > + dev_info(pci->dev, "PCIe Gen.%u x%u link up\n", > > + FIELD_GET(PCI_EXP_LNKSTA_CLS, val), > > + FIELD_GET(PCI_EXP_LNKSTA_NLW, val)); > > + > > + return 0; > > + } > > + > > + dev_err(pci->dev, "Phy link never came up\n"); > > > > return -ETIMEDOUT; > > } > > IMO: it's generally bette to test the error condition and unindent > the typical return. Absolutely right. Thanks for noticing that. No idea why I haven't done the way you said 'cause it seems neater, more maintainable than what I suggested here. -Sergey > > if (retries >= LINK_WAIT_MAX_RETRIES) { > dev_err(pci->dev, "Phy link never came up\n"); > return -ETIMEDOUT; > } > > offset = ... > val = ... > dev_info(...) > > return 0; > } >
On Tue, Mar 29, 2022 at 09:47:02AM -0500, Rob Herring wrote: > On Thu, Mar 24, 2022 at 04:37:21AM +0300, Serge Semin wrote: > > Printing just "link up" isn't that much informative especially when it > > comes to working with the PCI Express bus. Even if the link is up, due to > > multiple reasons the bus performance can degrade to slower speeds or to > > narrower width than both Root Port and its partner is capable of. In that > > case it would be handy to know the link specifications as early as > > possible. So let's add a more verbose message to the busy-wait link-state > > method, which will contain the link speed generation and the PCIe bus > > width in case if the link up state is discovered. Otherwise an error will > > be printed to the system log. > > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > --- > > drivers/pci/controller/dwc/pcie-designware.c | 22 +++++++++++++++----- > > 1 file changed, 17 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > index 6e81264fdfb4..f1693e25afcb 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > @@ -528,14 +528,26 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci) > > > > /* Check if the link is up or not */ > > for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) { > > - if (dw_pcie_link_up(pci)) { > > - dev_info(pci->dev, "Link up\n"); > > - return 0; > > - } > > + if (dw_pcie_link_up(pci)) > > + break; > > + > > usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX); > > } > > > > - dev_info(pci->dev, "Phy link never came up\n"); > > + if (retries < LINK_WAIT_MAX_RETRIES) { > > + u32 offset, val; > > + > > + offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP); > > + val = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA); > > + > > + dev_info(pci->dev, "PCIe Gen.%u x%u link up\n", > > + FIELD_GET(PCI_EXP_LNKSTA_CLS, val), > > + FIELD_GET(PCI_EXP_LNKSTA_NLW, val)); > > Given these are standard registers can we do this in the core code? The > main issue I think is that the config space accessors don't work until > you create the bus struct. That still should be early enough. AFAICS there are generic methods in the core code to get and print the link status. See the __pcie_print_link_status() method implementation. But as you said they rely on having the bus struct instance created and properly initialized. It's created in the framework of the PCI Host bridge registration procedure: pci_host_probe() +-> pci_scan_root_bus_bridge() +-> pci_register_host_bridge() +-> pci_alloc_bus(NULL); +-> ... As for me it would be more logical to have the PCIe link established (at least activated) and it' status logged before any of the denoted actions are made since further initialization rely on the PCIe bus transfers. Moreover the PCIe host probe procedure doesn't really perform any link up/down related activity, so there is no logical place to implement the link state checking except someplace at the traceback top position, but again the bus struct instance isn't available at that stage. Of course we could implement an alternative __pcie_print_HOST_link_status() method, which wouldn't need the bus struct passed. But that would have required some more modifications (and may cause some functionality duplication) than fixing a few lines of code and wasn't a subject of this patchset. As such I decided to stick with having the local link status logging procedure especially seeing it's done in the framework of the link-wait method, which is called right after the DW PCIe LTSSM is activated (at least for the DW PCIe Host controller). > > I think it is possible some implementations don't report the link state > in these registers. Maybe we don't really need to care. I don't see a way to disable the PCIe capability in the DW PCIe controllers. So if some implementations lack of these registers reporting the link state, then either those implementations must have been broken or they violate the PCIe Base Specification [1]. IMO that must be considered as abnormal situation and needs to be specifically handled. [1] PCI Express® Base Specification Revision 5.0, p. 742. -Sergey > > Rob
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c index 6e81264fdfb4..f1693e25afcb 100644 --- a/drivers/pci/controller/dwc/pcie-designware.c +++ b/drivers/pci/controller/dwc/pcie-designware.c @@ -528,14 +528,26 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci) /* Check if the link is up or not */ for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) { - if (dw_pcie_link_up(pci)) { - dev_info(pci->dev, "Link up\n"); - return 0; - } + if (dw_pcie_link_up(pci)) + break; + usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX); } - dev_info(pci->dev, "Phy link never came up\n"); + if (retries < LINK_WAIT_MAX_RETRIES) { + u32 offset, val; + + offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP); + val = dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKSTA); + + dev_info(pci->dev, "PCIe Gen.%u x%u link up\n", + FIELD_GET(PCI_EXP_LNKSTA_CLS, val), + FIELD_GET(PCI_EXP_LNKSTA_NLW, val)); + + return 0; + } + + dev_err(pci->dev, "Phy link never came up\n"); return -ETIMEDOUT; }
Printing just "link up" isn't that much informative especially when it comes to working with the PCI Express bus. Even if the link is up, due to multiple reasons the bus performance can degrade to slower speeds or to narrower width than both Root Port and its partner is capable of. In that case it would be handy to know the link specifications as early as possible. So let's add a more verbose message to the busy-wait link-state method, which will contain the link speed generation and the PCIe bus width in case if the link up state is discovered. Otherwise an error will be printed to the system log. Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> --- drivers/pci/controller/dwc/pcie-designware.c | 22 +++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)