* [PATCH v2] dwc: dra7xx: Print link state to console for debug
@ 2017-10-19 12:43 ` Faiz Abbas
0 siblings, 0 replies; 19+ messages in thread
From: Faiz Abbas @ 2017-10-19 12:43 UTC (permalink / raw)
To: kishon; +Cc: bhelgaas, linux-omap, linux-pci, linux-kernel, faiz_abbas
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);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v2] dwc: dra7xx: Print link state to console for debug
2017-10-19 12:43 ` Faiz Abbas
@ 2017-10-19 13:08 ` Faiz Abbas
-1 siblings, 0 replies; 19+ messages in thread
From: Faiz Abbas @ 2017-10-19 13:08 UTC (permalink / raw)
To: kishon; +Cc: bhelgaas, linux-omap, linux-pci, linux-kernel
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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2] dwc: dra7xx: Print link state to console for debug
@ 2017-10-19 13:08 ` Faiz Abbas
0 siblings, 0 replies; 19+ messages in thread
From: Faiz Abbas @ 2017-10-19 13:08 UTC (permalink / raw)
To: kishon; +Cc: bhelgaas, linux-omap, linux-pci, linux-kernel
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
^ permalink raw reply [flat|nested] 19+ messages in thread* RE: [PATCH v2] dwc: dra7xx: Print link state to console for debug
2017-10-19 13:08 ` Faiz Abbas
@ 2017-10-19 13:26 ` David Laight
-1 siblings, 0 replies; 19+ messages in thread
From: David Laight @ 2017-10-19 13:26 UTC (permalink / raw)
To: 'Faiz Abbas', kishon
Cc: bhelgaas, linux-omap, linux-pci, linux-kernel
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
^ permalink raw reply [flat|nested] 19+ messages in thread* RE: [PATCH v2] dwc: dra7xx: Print link state to console for debug
@ 2017-10-19 13:26 ` David Laight
0 siblings, 0 replies; 19+ messages in thread
From: David Laight @ 2017-10-19 13:26 UTC (permalink / raw)
To: 'Faiz Abbas', kishon
Cc: bhelgaas, linux-omap, linux-pci, linux-kernel
RnJvbTogRmFpeiBBYmJhcw0KPiBTZW50OiAxOSBPY3RvYmVyIDIwMTcgMTQ6MDkNCj4gT24gVGh1
cnNkYXkgMTkgT2N0b2JlciAyMDE3IDA2OjEzIFBNLCBGYWl6IEFiYmFzIHdyb3RlOg0KPiA+IEVu
YWJsZSBzdXBwb3J0IGZvciBwcmludGluZyB0aGUgTFRTU00gbGluayBzdGF0ZSBmb3IgZGVidWdn
aW5nIFBDSQ0KPiA+IHdoZW4gbGluayBpcyBkb3duLg0KPiA+DQo+ID4gU2lnbmVkLW9mZi1ieTog
RmFpeiBBYmJhcyA8ZmFpel9hYmJhc0B0aS5jb20+DQo+ID4gLS0tDQo+ID4gdjI6DQo+ID4gIDEu
IENoYW5nZWQgZGV2X2VycigpIHRvIGRldl9kYmcoKQ0KPiA+ICAyLiBDaGFuZ2VkIHN0YXRpYyBj
aGFyIGFycmF5IHRvIHN0YXRpYyBjb25zdCBjaGFyICogY29uc3QNCj4gPiAgMy4gZm9ybWF0IGNo
YW5nZXMNCj4gPg0KPiA+ICBkcml2ZXJzL3BjaS9kd2MvcGNpLWRyYTd4eC5jIHwgNDggKysrKysr
KysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysNCj4gPiAgMSBmaWxlIGNoYW5n
ZWQsIDQ4IGluc2VydGlvbnMoKykNCj4gPg0KPiA+IGRpZmYgLS1naXQgYS9kcml2ZXJzL3BjaS9k
d2MvcGNpLWRyYTd4eC5jIGIvZHJpdmVycy9wY2kvZHdjL3BjaS1kcmE3eHguYw0KPiA+IGluZGV4
IDM0NDI3YTYuLjBlNzBlNzcgMTAwNjQ0DQo+ID4gLS0tIGEvZHJpdmVycy9wY2kvZHdjL3BjaS1k
cmE3eHguYw0KPiA+ICsrKyBiL2RyaXZlcnMvcGNpL2R3Yy9wY2ktZHJhN3h4LmMNCj4gPiBAQCAt
OTgsNiArOTgsNDUgQEAgc3RydWN0IGRyYTd4eF9wY2llX29mX2RhdGEgew0KPiA+DQo+ID4gICNk
ZWZpbmUgdG9fZHJhN3h4X3BjaWUoeCkJZGV2X2dldF9kcnZkYXRhKCh4KS0+ZGV2KQ0KPiA+DQo+
ID4gK3N0YXRpYyBjb25zdCBjaGFyICogY29uc3Qgc3RhdGVbXSA9IHsNCj4gPiArCSJERVRFQ1Rf
UVVJRVQiLA0KLi4uDQo+ID4gKwkiUkNWUllfRVEzIg0KPiA+ICt9Ow0KPiA+ICsNCj4gPiAgc3Rh
dGljIGlubGluZSB1MzIgZHJhN3h4X3BjaWVfcmVhZGwoc3RydWN0IGRyYTd4eF9wY2llICpwY2ll
LCB1MzIgb2Zmc2V0KQ0KPiA+ICB7DQo+ID4gIAlyZXR1cm4gcmVhZGwocGNpZS0+YmFzZSArIG9m
ZnNldCk7DQo+ID4gQEAgLTExOCw2ICsxNTcsMTUgQEAgc3RhdGljIGludCBkcmE3eHhfcGNpZV9s
aW5rX3VwKHN0cnVjdCBkd19wY2llICpwY2kpDQo+ID4gIHsNCj4gPiAgCXN0cnVjdCBkcmE3eHhf
cGNpZSAqZHJhN3h4ID0gdG9fZHJhN3h4X3BjaWUocGNpKTsNCj4gPiAgCXUzMiByZWcgPSBkcmE3
eHhfcGNpZV9yZWFkbChkcmE3eHgsIFBDSUVDVFJMX0RSQTdYWF9DT05GX1BIWV9DUyk7DQo+ID4g
Kwl1MzIgY21kX3JlZzsNCj4gPiArCXUzMiBsdHNzbV9zdGF0ZTsNCj4gPiArDQo+ID4gKwlpZiAo
IShyZWcgJiBMSU5LX1VQKSkgew0KPiA+ICsJCWNtZF9yZWcgPSBkcmE3eHhfcGNpZV9yZWFkbChk
cmE3eHgsDQo+ID4gKwkJCQkJICAgIFBDSUVDVFJMX0RSQTdYWF9DT05GX0RFVklDRV9DTUQpOw0K
PiA+ICsJCWx0c3NtX3N0YXRlID0gKGNtZF9yZWcgJiBHRU5NQVNLKDcsIDIpKSA+PiAyOw0KPiA+
ICsJCWRldl9kYmcocGNpLT5kZXYsICJMaW5rIHN0YXRlOiVzXG4iLCBzdGF0ZVtsdHNzbV9zdGF0
ZV0pOw0KDQpIbW1tLi4uIEdFTk1BU0sgbGVhdmVzIGJ5IGh1bnRpbmcgaGVhZGVyIGZpbGVzLi4u
DQpXaHkgbm90IChjbWRfcmVnID4+IDIpICYgNjMgYW5kIGV4cGxpY2l0bHkgZGVmaW5lIHN0YXRl
WzY0XQ0KdG8gZ3VhcmFudGVlIHRoYXQgeW91IG5ldmVyIHByaW50IGFueXRoaW5nIHdvcnNlIHRo
YW4gYSBOVUxMDQpwb2ludGVyLg0KDQo+ID4gKwl9DQo+ID4NCj4gPiAgCXJldHVybiAhIShyZWcg
JiBMSU5LX1VQKTsNCj4gPiAgfQ0KPiA+DQo+IA0KPiBJIG1pc3NlZCBEYXZpZCdzIGNvbW1lbnQg
aW4gdjEuIFdpbGwgc3VibWl0IGEgbmV3IHZlcnNpb24uIFBsZWFzZSBpZ25vcmUuDQoNCkkndmUg
YSAnbmVhdCcgdHJpY2sgZm9yIGdlbmVyYXRpbmcgc3RyaW5ncyB0aGF0IG1hdGNoIGNvbnN0YW50
cy4NCllvdSBjYW4gZ2V0IHRoZSBjb21waWxlciB0byBkbyBhbGwgdGhlIHdvcmsgZm9yIHlvdToN
CihBc3N1bWluZyBJJ3ZlIHR5cGVkIGl0IGNvcnJlY3RseSkNCg0KI2RlZmluZSBMVFNTTV9ERUZT
KHgpIFwNCiAgeChERVRFQ1RfUVVJRVQpIFwNCiAgeChERVRFQ1RfQUNUKSBcDQooY29udGludWUg
Zm9yIGFsbCB0aGUgbmFtZXMpDQoNCkRlZmluZSBhbiBlbnVtIHdpdGggdGhlIG5hbWVkIGNvbnN0
YW50czoNCiNkZWZpbmUgWChuYW1lKSBMVFNTTV9TVEFURV8jI25hbWUsDQplbnVtIChMVFNTTV9E
RUZTKFgpIExUU1NNX1NUQVRFX1NJWkU9NjQpOw0KI3VuZGVmIFgNCg0KQXJyYXkgb2Ygc3RyaW5n
czoNCiNkZWZpbmUgWChuYW1lKSBbTFRTU01fU1RBVEVfIyNuYW1lXSA9ICNuYW1lDQpzdGF0aWMg
Y29uc3QgY2hhciAqIGNvbnN0IHN0YXRlX25hbWVzW0xUU1NNX1NUQVRFX1NJWkVdID0geyBMVFNT
TV9ERUZTKFgpIH07DQojdW5kZWYgWA0KDQoJRGF2aWQNCg0K
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] dwc: dra7xx: Print link state to console for debug
2017-10-19 13:26 ` David Laight
(?)
@ 2017-10-26 7:59 ` Faiz Abbas
2017-10-30 8:48 ` Faiz Abbas
-1 siblings, 1 reply; 19+ messages in thread
From: Faiz Abbas @ 2017-10-26 7:59 UTC (permalink / raw)
To: David Laight, kishon; +Cc: bhelgaas, linux-omap, linux-pci, linux-kernel
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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2] dwc: dra7xx: Print link state to console for debug
2017-10-26 7:59 ` Faiz Abbas
@ 2017-10-30 8:48 ` Faiz Abbas
2017-11-06 2:56 ` Faiz Abbas
0 siblings, 1 reply; 19+ messages in thread
From: Faiz Abbas @ 2017-10-30 8:48 UTC (permalink / raw)
To: David Laight, kishon; +Cc: bhelgaas, linux-omap, linux-pci, linux-kernel
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.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2] dwc: dra7xx: Print link state to console for debug
2017-10-30 8:48 ` Faiz Abbas
@ 2017-11-06 2:56 ` Faiz Abbas
0 siblings, 0 replies; 19+ messages in thread
From: Faiz Abbas @ 2017-11-06 2:56 UTC (permalink / raw)
To: David Laight, kishon; +Cc: bhelgaas, linux-omap, linux-pci, linux-kernel
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] dwc: dra7xx: Print link state to console for debug
2017-10-19 12:43 ` Faiz Abbas
(?)
(?)
@ 2017-10-20 23:09 ` Bjorn Helgaas
2017-10-23 10:29 ` Faiz Abbas
-1 siblings, 1 reply; 19+ messages in thread
From: Bjorn Helgaas @ 2017-10-20 23:09 UTC (permalink / raw)
To: Faiz Abbas; +Cc: kishon, bhelgaas, linux-omap, linux-pci, linux-kernel
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
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2] dwc: dra7xx: Print link state to console for debug
2017-10-20 23:09 ` Bjorn Helgaas
@ 2017-10-23 10:29 ` Faiz Abbas
0 siblings, 0 replies; 19+ messages in thread
From: Faiz Abbas @ 2017-10-23 10:29 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: kishon, bhelgaas, linux-omap, linux-pci, linux-kernel
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] dwc: dra7xx: Print link state to console for debug
@ 2017-10-23 10:29 ` Faiz Abbas
0 siblings, 0 replies; 19+ messages in thread
From: Faiz Abbas @ 2017-10-23 10:29 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: kishon, bhelgaas, linux-omap, linux-pci, linux-kernel
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] dwc: dra7xx: Print link state to console for debug
2017-10-23 10:29 ` Faiz Abbas
(?)
@ 2017-10-23 14:04 ` Bjorn Helgaas
2017-10-24 6:18 ` Kishon Vijay Abraham I
-1 siblings, 1 reply; 19+ messages in thread
From: Bjorn Helgaas @ 2017-10-23 14:04 UTC (permalink / raw)
To: Faiz Abbas; +Cc: kishon, bhelgaas, linux-omap, linux-pci, linux-kernel
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;
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2] dwc: dra7xx: Print link state to console for debug
2017-10-23 14:04 ` Bjorn Helgaas
@ 2017-10-24 6:18 ` Kishon Vijay Abraham I
0 siblings, 0 replies; 19+ messages in thread
From: Kishon Vijay Abraham I @ 2017-10-24 6:18 UTC (permalink / raw)
To: Bjorn Helgaas, Faiz Abbas; +Cc: bhelgaas, linux-omap, linux-pci, linux-kernel
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] dwc: dra7xx: Print link state to console for debug
@ 2017-10-24 6:18 ` Kishon Vijay Abraham I
0 siblings, 0 replies; 19+ messages in thread
From: Kishon Vijay Abraham I @ 2017-10-24 6:18 UTC (permalink / raw)
To: Bjorn Helgaas, Faiz Abbas; +Cc: bhelgaas, linux-omap, linux-pci, linux-kernel
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] dwc: dra7xx: Print link state to console for debug
2017-10-19 12:43 ` Faiz Abbas
` (2 preceding siblings ...)
(?)
@ 2017-10-24 19:59 ` Bjorn Helgaas
2017-10-25 8:21 ` Faiz Abbas
-1 siblings, 1 reply; 19+ messages in thread
From: Bjorn Helgaas @ 2017-10-24 19:59 UTC (permalink / raw)
To: Faiz Abbas; +Cc: kishon, bhelgaas, linux-omap, linux-pci, linux-kernel
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)
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2] dwc: dra7xx: Print link state to console for debug
2017-10-24 19:59 ` Bjorn Helgaas
@ 2017-10-25 8:21 ` Faiz Abbas
0 siblings, 0 replies; 19+ messages in thread
From: Faiz Abbas @ 2017-10-25 8:21 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: kishon, bhelgaas, linux-omap, linux-pci, linux-kernel
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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2] dwc: dra7xx: Print link state to console for debug
@ 2017-10-25 8:21 ` Faiz Abbas
0 siblings, 0 replies; 19+ messages in thread
From: Faiz Abbas @ 2017-10-25 8:21 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: kishon, bhelgaas, linux-omap, linux-pci, linux-kernel
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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2] dwc: dra7xx: Print link state to console for debug
2017-10-25 8:21 ` Faiz Abbas
(?)
@ 2017-10-25 13:23 ` Bjorn Helgaas
-1 siblings, 0 replies; 19+ messages in thread
From: Bjorn Helgaas @ 2017-10-25 13:23 UTC (permalink / raw)
To: Faiz Abbas; +Cc: kishon, bhelgaas, linux-omap, linux-pci, linux-kernel
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.
^ permalink raw reply [flat|nested] 19+ messages in thread