| Message ID | 1508417009-30869-1-git-send-email-faiz_abbas@ti.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v2] dwc: dra7xx: Print link state to console for debug | expand |
On Thursday 19 October 2017 06:13 PM, Faiz Abbas wrote: > Enable support for printing the LTSSM link state for debugging PCI > when link is down. > > Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> > --- > v2: > 1. Changed dev_err() to dev_dbg() > 2. Changed static char array to static const char * const > 3. format changes > > drivers/pci/dwc/pci-dra7xx.c | 48 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 48 insertions(+) > > diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c > index 34427a6..0e70e77 100644 > --- a/drivers/pci/dwc/pci-dra7xx.c > +++ b/drivers/pci/dwc/pci-dra7xx.c > @@ -98,6 +98,45 @@ struct dra7xx_pcie_of_data { > > #define to_dra7xx_pcie(x) dev_get_drvdata((x)->dev) > > +static const char * const state[] = { > + "DETECT_QUIET", > + "DETECT_ACT", > + "POLL_ACTIVE", > + "POLL_COMPLIANCE", > + "POLL_CONFIG", > + "PRE_DETECT_QUIET", > + "DETECT_WAIT", > + "CFG_LINKWD_START", > + "CFG_LINKWD_ACEPT", > + "CFG_LANENUM_WAIT", > + "CFG_LANENUM_ACEPT", > + "CFG_COMPLETE", > + "CFG_IDLE", > + "RCVRY_LOCK", > + "RCVRY_SPEED", > + "RCVRY_RCVRCFG", > + "RCVRY_IDLE", > + "L0", > + "L0S", > + "L123_SEND_EIDLE", > + "L1_IDLE", > + "L2_IDLE", > + "L2_WAKE", > + "DISABLED_ENTRY", > + "DISABLED_IDLE", > + "DISABLED", > + "LPBK_ENTRY", > + "LPBK_ACTIVE", > + "LPBK_EXIT", > + "LPBK_EXIT_TIMEOUT", > + "HOT_RESET_ENTRY", > + "HOT_RESET", > + "RCVRY_EQ0", > + "RCVRY_EQ1", > + "RCVRY_EQ2", > + "RCVRY_EQ3" > +}; > + > static inline u32 dra7xx_pcie_readl(struct dra7xx_pcie *pcie, u32 offset) > { > return readl(pcie->base + offset); > @@ -118,6 +157,15 @@ static int dra7xx_pcie_link_up(struct dw_pcie *pci) > { > struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pci); > u32 reg = dra7xx_pcie_readl(dra7xx, PCIECTRL_DRA7XX_CONF_PHY_CS); > + u32 cmd_reg; > + u32 ltssm_state; > + > + if (!(reg & LINK_UP)) { > + cmd_reg = dra7xx_pcie_readl(dra7xx, > + PCIECTRL_DRA7XX_CONF_DEVICE_CMD); > + ltssm_state = (cmd_reg & GENMASK(7, 2)) >> 2; > + dev_dbg(pci->dev, "Link state:%s\n", state[ltssm_state]); > + } > > return !!(reg & LINK_UP); > } > I missed David's comment in v1. Will submit a new version. Please ignore. Thanks, Faiz
From: Faiz Abbas > Sent: 19 October 2017 14:09 > On Thursday 19 October 2017 06:13 PM, Faiz Abbas wrote: > > Enable support for printing the LTSSM link state for debugging PCI > > when link is down. > > > > Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> > > --- > > v2: > > 1. Changed dev_err() to dev_dbg() > > 2. Changed static char array to static const char * const > > 3. format changes > > > > drivers/pci/dwc/pci-dra7xx.c | 48 ++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 48 insertions(+) > > > > diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c > > index 34427a6..0e70e77 100644 > > --- a/drivers/pci/dwc/pci-dra7xx.c > > +++ b/drivers/pci/dwc/pci-dra7xx.c > > @@ -98,6 +98,45 @@ struct dra7xx_pcie_of_data { > > > > #define to_dra7xx_pcie(x) dev_get_drvdata((x)->dev) > > > > +static const char * const state[] = { > > + "DETECT_QUIET", ... > > + "RCVRY_EQ3" > > +}; > > + > > static inline u32 dra7xx_pcie_readl(struct dra7xx_pcie *pcie, u32 offset) > > { > > return readl(pcie->base + offset); > > @@ -118,6 +157,15 @@ static int dra7xx_pcie_link_up(struct dw_pcie *pci) > > { > > struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pci); > > u32 reg = dra7xx_pcie_readl(dra7xx, PCIECTRL_DRA7XX_CONF_PHY_CS); > > + u32 cmd_reg; > > + u32 ltssm_state; > > + > > + if (!(reg & LINK_UP)) { > > + cmd_reg = dra7xx_pcie_readl(dra7xx, > > + PCIECTRL_DRA7XX_CONF_DEVICE_CMD); > > + ltssm_state = (cmd_reg & GENMASK(7, 2)) >> 2; > > + dev_dbg(pci->dev, "Link state:%s\n", state[ltssm_state]); Hmmm... GENMASK leaves by hunting header files... Why not (cmd_reg >> 2) & 63 and explicitly define state[64] to guarantee that you never print anything worse than a NULL pointer. > > + } > > > > return !!(reg & LINK_UP); > > } > > > > I missed David's comment in v1. Will submit a new version. Please ignore. I've a 'neat' trick for generating strings that match constants. You can get the compiler to do all the work for you: (Assuming I've typed it correctly) #define LTSSM_DEFS(x) \ x(DETECT_QUIET) \ x(DETECT_ACT) \ (continue for all the names) Define an enum with the named constants: #define X(name) LTSSM_STATE_##name, enum (LTSSM_DEFS(X) LTSSM_STATE_SIZE=64); #undef X Array of strings: #define X(name) [LTSSM_STATE_##name] = #name static const char * const state_names[LTSSM_STATE_SIZE] = { LTSSM_DEFS(X) }; #undef X David
On Thu, Oct 19, 2017 at 06:13:29PM +0530, Faiz Abbas wrote: > Enable support for printing the LTSSM link state for debugging PCI > when link is down. > > Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> > --- > v2: > 1. Changed dev_err() to dev_dbg() > 2. Changed static char array to static const char * const > 3. format changes I'm not really sure how much debug help we want to carry around in the mainline kernel. End users aren't going to use this; it seems like more of a lab tool, and in situations like that you usually end up carrying around some out-of-tree patches for a while anyway. But I can probably be convinced either way. > drivers/pci/dwc/pci-dra7xx.c | 48 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 48 insertions(+) > > diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c > index 34427a6..0e70e77 100644 > --- a/drivers/pci/dwc/pci-dra7xx.c > +++ b/drivers/pci/dwc/pci-dra7xx.c > @@ -98,6 +98,45 @@ struct dra7xx_pcie_of_data { > > #define to_dra7xx_pcie(x) dev_get_drvdata((x)->dev) > > +static const char * const state[] = { > + "DETECT_QUIET", > + "DETECT_ACT", > + "POLL_ACTIVE", > + "POLL_COMPLIANCE", > + "POLL_CONFIG", > + "PRE_DETECT_QUIET", > + "DETECT_WAIT", > + "CFG_LINKWD_START", > + "CFG_LINKWD_ACEPT", > + "CFG_LANENUM_WAIT", > + "CFG_LANENUM_ACEPT", > + "CFG_COMPLETE", > + "CFG_IDLE", > + "RCVRY_LOCK", > + "RCVRY_SPEED", > + "RCVRY_RCVRCFG", > + "RCVRY_IDLE", > + "L0", > + "L0S", > + "L123_SEND_EIDLE", > + "L1_IDLE", > + "L2_IDLE", > + "L2_WAKE", > + "DISABLED_ENTRY", > + "DISABLED_IDLE", > + "DISABLED", > + "LPBK_ENTRY", > + "LPBK_ACTIVE", > + "LPBK_EXIT", > + "LPBK_EXIT_TIMEOUT", > + "HOT_RESET_ENTRY", > + "HOT_RESET", > + "RCVRY_EQ0", > + "RCVRY_EQ1", > + "RCVRY_EQ2", > + "RCVRY_EQ3" > +}; > + > static inline u32 dra7xx_pcie_readl(struct dra7xx_pcie *pcie, u32 offset) > { > return readl(pcie->base + offset); > @@ -118,6 +157,15 @@ static int dra7xx_pcie_link_up(struct dw_pcie *pci) > { > struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pci); > u32 reg = dra7xx_pcie_readl(dra7xx, PCIECTRL_DRA7XX_CONF_PHY_CS); > + u32 cmd_reg; > + u32 ltssm_state; > + > + if (!(reg & LINK_UP)) { > + cmd_reg = dra7xx_pcie_readl(dra7xx, > + PCIECTRL_DRA7XX_CONF_DEVICE_CMD); > + ltssm_state = (cmd_reg & GENMASK(7, 2)) >> 2; > + dev_dbg(pci->dev, "Link state:%s\n", state[ltssm_state]); > + } > > return !!(reg & LINK_UP); > } > -- > 2.7.4 >
Hi Bjorn, On Saturday 21 October 2017 04:39 AM, Bjorn Helgaas wrote: > On Thu, Oct 19, 2017 at 06:13:29PM +0530, Faiz Abbas wrote: >> Enable support for printing the LTSSM link state for debugging PCI >> when link is down. >> >> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> >> --- >> v2: >> 1. Changed dev_err() to dev_dbg() >> 2. Changed static char array to static const char * const >> 3. format changes > > I'm not really sure how much debug help we want to carry around in the > mainline kernel. End users aren't going to use this; it seems like > more of a lab tool, and in situations like that you usually end up > carrying around some out-of-tree patches for a while anyway. But I > can probably be convinced either way. > It'll be easier to support customers if they can tell us what the state of the link is by just changing the log level. We won't have to send a debug patch to the customer to find that out. Thanks, Faiz
On Mon, Oct 23, 2017 at 03:59:49PM +0530, Faiz Abbas wrote: > On Saturday 21 October 2017 04:39 AM, Bjorn Helgaas wrote: > > On Thu, Oct 19, 2017 at 06:13:29PM +0530, Faiz Abbas wrote: > >> Enable support for printing the LTSSM link state for debugging PCI > >> when link is down. > >> > >> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> > >> --- > >> v2: > >> 1. Changed dev_err() to dev_dbg() > >> 2. Changed static char array to static const char * const > >> 3. format changes > > > > I'm not really sure how much debug help we want to carry around in the > > mainline kernel. End users aren't going to use this; it seems like > > more of a lab tool, and in situations like that you usually end up > > carrying around some out-of-tree patches for a while anyway. But I > > can probably be convinced either way. > > It'll be easier to support customers if they can tell us what the state > of the link is by just changing the log level. We won't have to send a > debug patch to the customer to find that out. That still sounds like a lab debug situation to me. Regular customers do not debug things at the level of the link state. I'm not aware of any other drivers that do this, so including this hints that this driver/hardware is not very mature. Printing text certainly *looks* nice, but it adds a lot of code and I'm not sure how much actual value they add. Just printing a hex value might be more reliable in terms of communicating it accurately back to you. E.g., it might be easier to lose the distinction between DISABLED_ENTRY and DISABLED_IDLE than between 0x745f and 0x7463, especially in a phone situation. Anyway, if Kishon acks this, I'll apply it. One nit: please do the "link up" test once, e.g., link_up = !!(reg & LINK_UP); if (!link_up) { cmd_reg = dra7xx_pcie_readl(...); dev_dbg(...); } return link_up;
Hi Bjorn, On Monday 23 October 2017 07:34 PM, Bjorn Helgaas wrote: > On Mon, Oct 23, 2017 at 03:59:49PM +0530, Faiz Abbas wrote: >> On Saturday 21 October 2017 04:39 AM, Bjorn Helgaas wrote: >>> On Thu, Oct 19, 2017 at 06:13:29PM +0530, Faiz Abbas wrote: >>>> Enable support for printing the LTSSM link state for debugging PCI >>>> when link is down. >>>> >>>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> >>>> --- >>>> v2: >>>> 1. Changed dev_err() to dev_dbg() >>>> 2. Changed static char array to static const char * const >>>> 3. format changes >>> >>> I'm not really sure how much debug help we want to carry around in the >>> mainline kernel. End users aren't going to use this; it seems like >>> more of a lab tool, and in situations like that you usually end up >>> carrying around some out-of-tree patches for a while anyway. But I >>> can probably be convinced either way. >> >> It'll be easier to support customers if they can tell us what the state >> of the link is by just changing the log level. We won't have to send a >> debug patch to the customer to find that out. > > That still sounds like a lab debug situation to me. Regular customers > do not debug things at the level of the link state. I'm not aware of > any other drivers that do this, so including this hints that this > driver/hardware is not very mature. > > Printing text certainly *looks* nice, but it adds a lot of code and > I'm not sure how much actual value they add. Just printing a hex > value might be more reliable in terms of communicating it accurately > back to you. E.g., it might be easier to lose the distinction between > DISABLED_ENTRY and DISABLED_IDLE than between 0x745f and 0x7463, > especially in a phone situation. > > Anyway, if Kishon acks this, I'll apply it. One nit: please do the > "link up" test once, e.g., IMHO both print text and the debug print itself helps to save developer effort. FWIW: Acked-by: Kishon Vijay Abraham I <kishon@ti.com> Thanks Kishon
On Thu, Oct 19, 2017 at 06:13:29PM +0530, Faiz Abbas wrote: > Enable support for printing the LTSSM link state for debugging PCI > when link is down. > > Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> Applied with Kishon's ack to pci/host-dra7xx for v4.15, thanks! I tweaked the "link up" testing as follows (what I suggested before): @@ -118,8 +157,18 @@ static int dra7xx_pcie_link_up(struct dw_pcie *pci) { struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pci); u32 reg = dra7xx_pcie_readl(dra7xx, PCIECTRL_DRA7XX_CONF_PHY_CS); + int link_up = !!(reg & LINK_UP); + u32 cmd_reg; + u32 ltssm_state; + + if (!link_up) { + cmd_reg = dra7xx_pcie_readl(dra7xx, + PCIECTRL_DRA7XX_CONF_DEVICE_CMD); + ltssm_state = (cmd_reg & GENMASK(7, 2)) >> 2; + dev_dbg(pci->dev, "Link state: %s\n", state[ltssm_state]); + } - return !!(reg & LINK_UP); + return link_up; } static void dra7xx_pcie_stop_link(struct dw_pcie *pci)
Bjorn, On Wednesday 25 October 2017 01:29 AM, Bjorn Helgaas wrote: > On Thu, Oct 19, 2017 at 06:13:29PM +0530, Faiz Abbas wrote: >> Enable support for printing the LTSSM link state for debugging PCI >> when link is down. >> >> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> > > Applied with Kishon's ack to pci/host-dra7xx for v4.15, thanks! > > I tweaked the "link up" testing as follows (what I suggested before): > > > @@ -118,8 +157,18 @@ static int dra7xx_pcie_link_up(struct dw_pcie *pci) > { > struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pci); > u32 reg = dra7xx_pcie_readl(dra7xx, PCIECTRL_DRA7XX_CONF_PHY_CS); > + int link_up = !!(reg & LINK_UP); > + u32 cmd_reg; > + u32 ltssm_state; > + > + if (!link_up) { > + cmd_reg = dra7xx_pcie_readl(dra7xx, > + PCIECTRL_DRA7XX_CONF_DEVICE_CMD); > + ltssm_state = (cmd_reg & GENMASK(7, 2)) >> 2; > + dev_dbg(pci->dev, "Link state: %s\n", state[ltssm_state]); > + } > > - return !!(reg & LINK_UP); > + return link_up; > } > > static void dra7xx_pcie_stop_link(struct dw_pcie *pci) > I wanted to send another version with David's suggestions included. Please don't merge. Thanks, Faiz
On Wed, Oct 25, 2017 at 01:51:53PM +0530, Faiz Abbas wrote: > Bjorn, > > On Wednesday 25 October 2017 01:29 AM, Bjorn Helgaas wrote: > > On Thu, Oct 19, 2017 at 06:13:29PM +0530, Faiz Abbas wrote: > >> Enable support for printing the LTSSM link state for debugging PCI > >> when link is down. > >> > >> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> > > > > Applied with Kishon's ack to pci/host-dra7xx for v4.15, thanks! > > > > I tweaked the "link up" testing as follows (what I suggested before): > > > > > > @@ -118,8 +157,18 @@ static int dra7xx_pcie_link_up(struct dw_pcie *pci) > > { > > struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pci); > > u32 reg = dra7xx_pcie_readl(dra7xx, PCIECTRL_DRA7XX_CONF_PHY_CS); > > + int link_up = !!(reg & LINK_UP); > > + u32 cmd_reg; > > + u32 ltssm_state; > > + > > + if (!link_up) { > > + cmd_reg = dra7xx_pcie_readl(dra7xx, > > + PCIECTRL_DRA7XX_CONF_DEVICE_CMD); > > + ltssm_state = (cmd_reg & GENMASK(7, 2)) >> 2; > > + dev_dbg(pci->dev, "Link state: %s\n", state[ltssm_state]); > > + } > > > > - return !!(reg & LINK_UP); > > + return link_up; > > } > > > > static void dra7xx_pcie_stop_link(struct dw_pcie *pci) > > > > I wanted to send another version with David's suggestions included. > Please don't merge. Dropped.
David, On Thursday 19 October 2017 06:56 PM, David Laight wrote: > From: Faiz Abbas >> Sent: 19 October 2017 14:09 >> On Thursday 19 October 2017 06:13 PM, Faiz Abbas wrote: >>> Enable support for printing the LTSSM link state for debugging PCI >>> when link is down. >>> >>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> >>> --- >>> v2: >>> 1. Changed dev_err() to dev_dbg() >>> 2. Changed static char array to static const char * const >>> 3. format changes >>> >>> drivers/pci/dwc/pci-dra7xx.c | 48 ++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 48 insertions(+) >>> >>> diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c >>> index 34427a6..0e70e77 100644 >>> --- a/drivers/pci/dwc/pci-dra7xx.c >>> +++ b/drivers/pci/dwc/pci-dra7xx.c >>> @@ -98,6 +98,45 @@ struct dra7xx_pcie_of_data { >>> >>> #define to_dra7xx_pcie(x) dev_get_drvdata((x)->dev) >>> >>> +static const char * const state[] = { >>> + "DETECT_QUIET", > ... >>> + "RCVRY_EQ3" >>> +}; >>> + >>> static inline u32 dra7xx_pcie_readl(struct dra7xx_pcie *pcie, u32 offset) >>> { >>> return readl(pcie->base + offset); >>> @@ -118,6 +157,15 @@ static int dra7xx_pcie_link_up(struct dw_pcie *pci) >>> { >>> struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pci); >>> u32 reg = dra7xx_pcie_readl(dra7xx, PCIECTRL_DRA7XX_CONF_PHY_CS); >>> + u32 cmd_reg; >>> + u32 ltssm_state; >>> + >>> + if (!(reg & LINK_UP)) { >>> + cmd_reg = dra7xx_pcie_readl(dra7xx, >>> + PCIECTRL_DRA7XX_CONF_DEVICE_CMD); >>> + ltssm_state = (cmd_reg & GENMASK(7, 2)) >> 2; >>> + dev_dbg(pci->dev, "Link state:%s\n", state[ltssm_state]); > > Hmmm... GENMASK leaves by hunting header files...> Why not (cmd_reg >> 2) & 63 and explicitly define state[64] > to guarantee that you never print anything worse than a NULL > pointer. I'm not sure what you mean. Are you worried we might print something outside the array bounds? How is this easier to decipher than GENMASK? > >>> + } >>> >>> return !!(reg & LINK_UP); >>> } >>> >> >> I missed David's comment in v1. Will submit a new version. Please ignore. > > I've a 'neat' trick for generating strings that match constants. > You can get the compiler to do all the work for you: > (Assuming I've typed it correctly) > > #define LTSSM_DEFS(x) \ > x(DETECT_QUIET) \ > x(DETECT_ACT) \ > (continue for all the names) > > Define an enum with the named constants: > #define X(name) LTSSM_STATE_##name, > enum (LTSSM_DEFS(X) LTSSM_STATE_SIZE=64); > #undef X > > Array of strings: > #define X(name) [LTSSM_STATE_##name] = #name > static const char * const state_names[LTSSM_STATE_SIZE] = { LTSSM_DEFS(X) }; > #undef X > > David > So I implemented your idea and it looks like this: http://pastebin.ubuntu.com/25821834/ I don't know how much we gained by adding the trick. I still had to be careful not to be off by 1 when writing the list. Plus we are never saying anything like printk("%s", state[LTSSM_STATE_DETECT_QUIET]. Its a register read which is used to index the list array. Thanks, Faiz
On Thursday 26 October 2017 01:29 PM, Faiz Abbas wrote: > David, > > On Thursday 19 October 2017 06:56 PM, David Laight wrote: >> From: Faiz Abbas >>> Sent: 19 October 2017 14:09 >>> On Thursday 19 October 2017 06:13 PM, Faiz Abbas wrote: >>>> Enable support for printing the LTSSM link state for debugging PCI >>>> when link is down. >>>> >>>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> >>>> --- >>>> v2: >>>> 1. Changed dev_err() to dev_dbg() >>>> 2. Changed static char array to static const char * const >>>> 3. format changes >>>> >>>> drivers/pci/dwc/pci-dra7xx.c | 48 ++++++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 48 insertions(+) >>>> >>>> diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c >>>> index 34427a6..0e70e77 100644 >>>> --- a/drivers/pci/dwc/pci-dra7xx.c >>>> +++ b/drivers/pci/dwc/pci-dra7xx.c >>>> @@ -98,6 +98,45 @@ struct dra7xx_pcie_of_data { >>>> >>>> #define to_dra7xx_pcie(x) dev_get_drvdata((x)->dev) >>>> >>>> +static const char * const state[] = { >>>> + "DETECT_QUIET", >> ... >>>> + "RCVRY_EQ3" >>>> +}; >>>> + >>>> static inline u32 dra7xx_pcie_readl(struct dra7xx_pcie *pcie, u32 offset) >>>> { >>>> return readl(pcie->base + offset); >>>> @@ -118,6 +157,15 @@ static int dra7xx_pcie_link_up(struct dw_pcie *pci) >>>> { >>>> struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pci); >>>> u32 reg = dra7xx_pcie_readl(dra7xx, PCIECTRL_DRA7XX_CONF_PHY_CS); >>>> + u32 cmd_reg; >>>> + u32 ltssm_state; >>>> + >>>> + if (!(reg & LINK_UP)) { >>>> + cmd_reg = dra7xx_pcie_readl(dra7xx, >>>> + PCIECTRL_DRA7XX_CONF_DEVICE_CMD); >>>> + ltssm_state = (cmd_reg & GENMASK(7, 2)) >> 2; >>>> + dev_dbg(pci->dev, "Link state:%s\n", state[ltssm_state]); >> >> Hmmm... GENMASK leaves by hunting header files...> Why not (cmd_reg >> 2) & 63 and explicitly define state[64] >> to guarantee that you never print anything worse than a NULL >> pointer. > > I'm not sure what you mean. Are you worried we might print something > outside the array bounds? How is this easier to decipher than GENMASK? > >> >>>> + } >>>> >>>> return !!(reg & LINK_UP); >>>> } >>>> >>> >>> I missed David's comment in v1. Will submit a new version. Please ignore. >> >> I've a 'neat' trick for generating strings that match constants. >> You can get the compiler to do all the work for you: >> (Assuming I've typed it correctly) >> >> #define LTSSM_DEFS(x) \ >> x(DETECT_QUIET) \ >> x(DETECT_ACT) \ >> (continue for all the names) >> >> Define an enum with the named constants: >> #define X(name) LTSSM_STATE_##name, >> enum (LTSSM_DEFS(X) LTSSM_STATE_SIZE=64); >> #undef X >> >> Array of strings: >> #define X(name) [LTSSM_STATE_##name] = #name >> static const char * const state_names[LTSSM_STATE_SIZE] = { LTSSM_DEFS(X) }; >> #undef X >> >> David >> > > So I implemented your idea and it looks like this: > http://pastebin.ubuntu.com/25821834/ > > I don't know how much we gained by adding the trick. I still had to be > careful not to be off by 1 when writing the list. Plus we are never > saying anything like printk("%s", state[LTSSM_STATE_DETECT_QUIET]. Its a > register read which is used to index the list array. > > Thanks, > Faiz > Gentle Ping.
On Monday 30 October 2017 02:18 PM, Faiz Abbas wrote: > > > On Thursday 26 October 2017 01:29 PM, Faiz Abbas wrote: >> David, >> >> On Thursday 19 October 2017 06:56 PM, David Laight wrote: >>> From: Faiz Abbas >>>> Sent: 19 October 2017 14:09 >>>> On Thursday 19 October 2017 06:13 PM, Faiz Abbas wrote: >>>>> Enable support for printing the LTSSM link state for debugging PCI >>>>> when link is down. >>>>> >>>>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> >>>>> --- >>>>> v2: >>>>> 1. Changed dev_err() to dev_dbg() >>>>> 2. Changed static char array to static const char * const >>>>> 3. format changes >>>>> >>>>> drivers/pci/dwc/pci-dra7xx.c | 48 ++++++++++++++++++++++++++++++++++++++++++++ >>>>> 1 file changed, 48 insertions(+) >>>>> >>>>> diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c >>>>> index 34427a6..0e70e77 100644 >>>>> --- a/drivers/pci/dwc/pci-dra7xx.c >>>>> +++ b/drivers/pci/dwc/pci-dra7xx.c >>>>> @@ -98,6 +98,45 @@ struct dra7xx_pcie_of_data { >>>>> >>>>> #define to_dra7xx_pcie(x) dev_get_drvdata((x)->dev) >>>>> >>>>> +static const char * const state[] = { >>>>> + "DETECT_QUIET", >>> ... >>>>> + "RCVRY_EQ3" >>>>> +}; >>>>> + >>>>> static inline u32 dra7xx_pcie_readl(struct dra7xx_pcie *pcie, u32 offset) >>>>> { >>>>> return readl(pcie->base + offset); >>>>> @@ -118,6 +157,15 @@ static int dra7xx_pcie_link_up(struct dw_pcie *pci) >>>>> { >>>>> struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pci); >>>>> u32 reg = dra7xx_pcie_readl(dra7xx, PCIECTRL_DRA7XX_CONF_PHY_CS); >>>>> + u32 cmd_reg; >>>>> + u32 ltssm_state; >>>>> + >>>>> + if (!(reg & LINK_UP)) { >>>>> + cmd_reg = dra7xx_pcie_readl(dra7xx, >>>>> + PCIECTRL_DRA7XX_CONF_DEVICE_CMD); >>>>> + ltssm_state = (cmd_reg & GENMASK(7, 2)) >> 2; >>>>> + dev_dbg(pci->dev, "Link state:%s\n", state[ltssm_state]); >>> >>> Hmmm... GENMASK leaves by hunting header files...> Why not (cmd_reg >> 2) & 63 and explicitly define state[64] >>> to guarantee that you never print anything worse than a NULL >>> pointer. >> >> I'm not sure what you mean. Are you worried we might print something >> outside the array bounds? How is this easier to decipher than GENMASK? >> >>> >>>>> + } >>>>> >>>>> return !!(reg & LINK_UP); >>>>> } >>>>> >>>> >>>> I missed David's comment in v1. Will submit a new version. Please ignore. >>> >>> I've a 'neat' trick for generating strings that match constants. >>> You can get the compiler to do all the work for you: >>> (Assuming I've typed it correctly) >>> >>> #define LTSSM_DEFS(x) \ >>> x(DETECT_QUIET) \ >>> x(DETECT_ACT) \ >>> (continue for all the names) >>> >>> Define an enum with the named constants: >>> #define X(name) LTSSM_STATE_##name, >>> enum (LTSSM_DEFS(X) LTSSM_STATE_SIZE=64); >>> #undef X >>> >>> Array of strings: >>> #define X(name) [LTSSM_STATE_##name] = #name >>> static const char * const state_names[LTSSM_STATE_SIZE] = { LTSSM_DEFS(X) }; >>> #undef X >>> >>> David >>> >> >> So I implemented your idea and it looks like this: >> http://pastebin.ubuntu.com/25821834/ >> >> I don't know how much we gained by adding the trick. I still had to be >> careful not to be off by 1 when writing the list. Plus we are never >> saying anything like printk("%s", state[LTSSM_STATE_DETECT_QUIET]. Its a >> register read which is used to index the list array. >> >> Thanks, >> Faiz >> > > Gentle Ping. > Ping Again.
diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c index 34427a6..0e70e77 100644 --- a/drivers/pci/dwc/pci-dra7xx.c +++ b/drivers/pci/dwc/pci-dra7xx.c @@ -98,6 +98,45 @@ struct dra7xx_pcie_of_data { #define to_dra7xx_pcie(x) dev_get_drvdata((x)->dev) +static const char * const state[] = { + "DETECT_QUIET", + "DETECT_ACT", + "POLL_ACTIVE", + "POLL_COMPLIANCE", + "POLL_CONFIG", + "PRE_DETECT_QUIET", + "DETECT_WAIT", + "CFG_LINKWD_START", + "CFG_LINKWD_ACEPT", + "CFG_LANENUM_WAIT", + "CFG_LANENUM_ACEPT", + "CFG_COMPLETE", + "CFG_IDLE", + "RCVRY_LOCK", + "RCVRY_SPEED", + "RCVRY_RCVRCFG", + "RCVRY_IDLE", + "L0", + "L0S", + "L123_SEND_EIDLE", + "L1_IDLE", + "L2_IDLE", + "L2_WAKE", + "DISABLED_ENTRY", + "DISABLED_IDLE", + "DISABLED", + "LPBK_ENTRY", + "LPBK_ACTIVE", + "LPBK_EXIT", + "LPBK_EXIT_TIMEOUT", + "HOT_RESET_ENTRY", + "HOT_RESET", + "RCVRY_EQ0", + "RCVRY_EQ1", + "RCVRY_EQ2", + "RCVRY_EQ3" +}; + static inline u32 dra7xx_pcie_readl(struct dra7xx_pcie *pcie, u32 offset) { return readl(pcie->base + offset); @@ -118,6 +157,15 @@ static int dra7xx_pcie_link_up(struct dw_pcie *pci) { struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pci); u32 reg = dra7xx_pcie_readl(dra7xx, PCIECTRL_DRA7XX_CONF_PHY_CS); + u32 cmd_reg; + u32 ltssm_state; + + if (!(reg & LINK_UP)) { + cmd_reg = dra7xx_pcie_readl(dra7xx, + PCIECTRL_DRA7XX_CONF_DEVICE_CMD); + ltssm_state = (cmd_reg & GENMASK(7, 2)) >> 2; + dev_dbg(pci->dev, "Link state:%s\n", state[ltssm_state]); + } return !!(reg & LINK_UP); }
Enable support for printing the LTSSM link state for debugging PCI when link is down. Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> --- v2: 1. Changed dev_err() to dev_dbg() 2. Changed static char array to static const char * const 3. format changes drivers/pci/dwc/pci-dra7xx.c | 48 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)