diff mbox

PCI: dwc: dra7xx: Print link state to console for debug

Message ID 1508219031-25592-1-git-send-email-faiz_abbas@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Faiz Abbas Oct. 17, 2017, 5:43 a.m. UTC 
Enable support for printing the LTSSM link state for debugging PCI
when link is down.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 drivers/pci/dwc/pci-dra7xx.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Comments

Joe Perches Oct. 17, 2017, 6:35 a.m. UTC | #1 
On Tue, 2017-10-17 at 11:13 +0530, Faiz Abbas wrote:
> Enable support for printing the LTSSM link state for debugging PCI
> when link is down.
[]
> diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
> index 34427a6..7b150b0 100644
> --- a/drivers/pci/dwc/pci-dra7xx.c
> +++ b/drivers/pci/dwc/pci-dra7xx.c
> @@ -98,6 +98,18 @@ struct dra7xx_pcie_of_data {
>  
>  #define to_dra7xx_pcie(x)	dev_get_drvdata((x)->dev)
>  
> +static char state[][20] = {
> +	"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"

what's wrong with using the far more typical

static const char *link_state[] = {
	"DETECT_QUIET",
	...
};

> +};
> +
>  static inline u32 dra7xx_pcie_readl(struct dra7xx_pcie *pcie, u32 offset)
>  {
>  	return readl(pcie->base + offset);
> @@ -118,6 +130,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_err(pci->dev, "Link state:%s\n", state[ltssm_state]);

and why is this a dev_err and not dev_info?
and if it's really for debugging, why not dev_dbg?

> +	}
>  
>  	return !!(reg & LINK_UP);
>  }
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Laight Oct. 17, 2017, 2:45 p.m. UTC | #2 
From: Joe Perches
> Sent: 17 October 2017 07:35
> On Tue, 2017-10-17 at 11:13 +0530, Faiz Abbas wrote:
> > Enable support for printing the LTSSM link state for debugging PCI
> > when link is down.
> []
> > diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
> > index 34427a6..7b150b0 100644
> > --- a/drivers/pci/dwc/pci-dra7xx.c
> > +++ b/drivers/pci/dwc/pci-dra7xx.c
> > @@ -98,6 +98,18 @@ struct dra7xx_pcie_of_data {
> >
> >  #define to_dra7xx_pcie(x)	dev_get_drvdata((x)->dev)
> >
> > +static char state[][20] = {
> > +	"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"
> 
> what's wrong with using the far more typical
> 
> static const char *link_state[] = {
> 	"DETECT_QUIET",
> 	...

Or 4 aligned columns.
Better still used named initialisers:
	[xxx_DETECT_QUIET] = "DETECT_QUIET",
you really don't want an 'off-by-one' in that list.

	David

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Faiz Abbas Oct. 18, 2017, 6:16 a.m. UTC | #3 
On Tuesday 17 October 2017 12:05 PM, Joe Perches wrote:
> On Tue, 2017-10-17 at 11:13 +0530, Faiz Abbas wrote:
>> Enable support for printing the LTSSM link state for debugging PCI
>> when link is down.
> []
>> diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
>> index 34427a6..7b150b0 100644
>> --- a/drivers/pci/dwc/pci-dra7xx.c
>> +++ b/drivers/pci/dwc/pci-dra7xx.c
>> @@ -98,6 +98,18 @@ struct dra7xx_pcie_of_data {
>>  
>>  #define to_dra7xx_pcie(x)	dev_get_drvdata((x)->dev)
>>  
>> +static char state[][20] = {
>> +	"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"
> 
> what's wrong with using the far more typical
> 
> static const char *link_state[] = {
> 	"DETECT_QUIET",
> 	...
> };

Too many lines?

> 
>> +};
>> +
>>  static inline u32 dra7xx_pcie_readl(struct dra7xx_pcie *pcie, u32 offset)
>>  {
>>  	return readl(pcie->base + offset);
>> @@ -118,6 +130,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_err(pci->dev, "Link state:%s\n", state[ltssm_state]);
> 
> and why is this a dev_err and not dev_info?
> and if it's really for debugging, why not dev_dbg?
> 
>> +	}
>>  
>>  	return !!(reg & LINK_UP);
>>  }

Will change to dev_dbg.

Thanks,
Faiz
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Faiz Abbas Oct. 19, 2017, 1:23 p.m. UTC | #4 
Hi,

On Tuesday 17 October 2017 08:15 PM, David Laight wrote:
> From: Joe Perches
>> Sent: 17 October 2017 07:35
>> On Tue, 2017-10-17 at 11:13 +0530, Faiz Abbas wrote:
>>> Enable support for printing the LTSSM link state for debugging PCI
>>> when link is down.
>> []
>>> diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
>>> index 34427a6..7b150b0 100644
>>> --- a/drivers/pci/dwc/pci-dra7xx.c
>>> +++ b/drivers/pci/dwc/pci-dra7xx.c
>>> @@ -98,6 +98,18 @@ struct dra7xx_pcie_of_data {
>>>
>>>  #define to_dra7xx_pcie(x)	dev_get_drvdata((x)->dev)
>>>
>>> +static char state[][20] = {
>>> +	"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"
>>
>> what's wrong with using the far more typical
>>
>> static const char *link_state[] = {
>> 	"DETECT_QUIET",
>> 	...
> 
> Or 4 aligned columns.

4 aligned columns are difficult as the strings are of different sizes.

> Better still used named initialisers:
> 	[xxx_DETECT_QUIET] = "DETECT_QUIET",
> you really don't want an 'off-by-one' in that list.

Sounds good. Let me see how I can implement this.

Thanks,
Faiz


--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
index 34427a6..7b150b0 100644
--- a/drivers/pci/dwc/pci-dra7xx.c
+++ b/drivers/pci/dwc/pci-dra7xx.c
@@ -98,6 +98,18 @@  struct dra7xx_pcie_of_data {
 
 #define to_dra7xx_pcie(x)	dev_get_drvdata((x)->dev)
 
+static char state[][20] = {
+	"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 +130,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_err(pci->dev, "Link state:%s\n", state[ltssm_state]);
+	}
 
 	return !!(reg & LINK_UP);
 }