[PATCH v4 0/4] PCI: designware: LTSSM #define cleanup

[PATCH v4 0/4] PCI: designware: LTSSM #define cleanup

[Bjorn Helgaas]

This is a revision of Fabio's series:
http://lkml.kernel.org/r/1444664808-16445-1-git-send-email-festevam@gmail.com

Lucas, you reviewed the v3 patches, but I fiddled enough with this that I
didn't want to blindly carry your review forward.  I don't *think* I
changed anything substantive, but I might have missed something.

Changes since Fabio's v3:
  - Split removal of spear unused #defines to separate patch
  - Combine LTSSM_STATE_MASK definition and use changes in one patch
  - Combine LTSSM_STATE_RCVRY_LOCK definition and use in one patch
  - Use common LTSSM_STATE_L0 definition

---

Bjorn Helgaas (1):
      PCI: designware: Use common LTSSM_STATE_L0 definition

Fabio Estevam (3):
      PCI: designware: Use common LTSSM_STATE_MASK definition
      PCI: designware: Use common LTSSM_STATE_RCVRY_LOCK definition
      PCI: spear: Remove unused #defines


 drivers/pci/host/pci-imx6.c        |    3 +-
 drivers/pci/host/pci-keystone-dw.c |    2 -
 drivers/pci/host/pci-layerscape.c  |    4 +-
 drivers/pci/host/pcie-designware.h |    4 ++
 drivers/pci/host/pcie-spear13xx.c  |   70 ------------------------------------
 5 files changed, 6 insertions(+), 77 deletions(-)

[PATCH v4 1/4] PCI: designware: Use common LTSSM_STATE_MASK definition

[Bjorn Helgaas]

From: Fabio Estevam <fabio.estevam@freescale.com>

Add a common #define for LTSSM_STATE_MASK and use it in all the
DesignWare-based drivers.

Note that this changes LTSSM_STATE_MASK from 6 bits (0x3f) to 5 bits (0x1f)
for i.MX6 and Layerscape.  We believe this is safe for all DesignWare-based
PCIe cores.

[bhelgaas: changelog, move removal of unused SPEAr13xx #defines to separate
patch, add only LTSSM_STATE_MASK and change all users for reviewability]
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/host/pci-imx6.c        |    3 +--
 drivers/pci/host/pci-keystone-dw.c |    1 -
 drivers/pci/host/pci-layerscape.c  |    1 -
 drivers/pci/host/pcie-designware.h |    2 ++
 4 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index 6f43086..7b0d120 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -55,7 +55,6 @@ struct imx6_pcie {
 #define PCIE_PL_PFLR_LINK_STATE_MASK		(0x3f << 16)
 #define PCIE_PL_PFLR_FORCE_LINK			(1 << 15)
 #define PCIE_PHY_DEBUG_R0 (PL_OFFSET + 0x28)
-#define PCIE_PHY_DEBUG_R0_LTSSM_MASK		(0x3f << 0)
 #define PCIE_PHY_DEBUG_R1 (PL_OFFSET + 0x2c)
 #define PCIE_PHY_DEBUG_R1_XMLH_LINK_IN_TRAINING	(1 << 29)
 #define PCIE_PHY_DEBUG_R1_XMLH_LINK_UP		(1 << 4)
@@ -508,7 +507,7 @@ static int imx6_pcie_link_up(struct pcie_port *pp)
 	if (rx_valid & PCIE_PHY_RX_ASIC_OUT_VALID)
 		return 0;
 
-	if ((debug_r0 & PCIE_PHY_DEBUG_R0_LTSSM_MASK) != 0x0d)
+	if ((debug_r0 & LTSSM_STATE_MASK) != 0x0d)
 		return 0;
 
 	dev_err(pp->dev, "transition to gen2 is stuck, reset PHY!\n");
diff --git a/drivers/pci/host/pci-keystone-dw.c b/drivers/pci/host/pci-keystone-dw.c
index e71da99..95a8b13 100644
--- a/drivers/pci/host/pci-keystone-dw.c
+++ b/drivers/pci/host/pci-keystone-dw.c
@@ -25,7 +25,6 @@
 
 /* Application register defines */
 #define LTSSM_EN_VAL		        1
-#define LTSSM_STATE_MASK		0x1f
 #define LTSSM_STATE_L0			0x11
 #define DBI_CS2_EN_VAL			0x20
 #define OB_XLAT_EN_VAL		        2
diff --git a/drivers/pci/host/pci-layerscape.c b/drivers/pci/host/pci-layerscape.c
index b2328ea1..f02752e 100644
--- a/drivers/pci/host/pci-layerscape.c
+++ b/drivers/pci/host/pci-layerscape.c
@@ -29,7 +29,6 @@
 /* PEX1/2 Misc Ports Status Register */
 #define SCFG_PEXMSCPORTSR(pex_idx)	(0x94 + (pex_idx) * 4)
 #define LTSSM_STATE_SHIFT	20
-#define LTSSM_STATE_MASK	0x3f
 #define LTSSM_PCIE_L0		0x11 /* L0 state */
 
 /* Symbol Timer Register and Filter Mask Register 1 */
diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
index d0bbd27..a1a76ff 100644
--- a/drivers/pci/host/pcie-designware.h
+++ b/drivers/pci/host/pcie-designware.h
@@ -22,6 +22,8 @@
 #define MAX_MSI_IRQS			32
 #define MAX_MSI_CTRLS			(MAX_MSI_IRQS / 32)
 
+#define LTSSM_STATE_MASK		0x1f
+
 struct pcie_port {
 	struct device		*dev;
 	u8			root_bus_nr;

Re: [PATCH v4 1/4] PCI: designware: Use common LTSSM_STATE_MASK definition

[Murali Karicheri]

On 10/21/2015 02:42 PM, Bjorn Helgaas wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
>
> Add a common #define for LTSSM_STATE_MASK and use it in all the
> DesignWare-based drivers.
>
> Note that this changes LTSSM_STATE_MASK from 6 bits (0x3f) to 5 bits (0x1f)
> for i.MX6 and Layerscape.  We believe this is safe for all DesignWare-based
> PCIe cores.
>
> [bhelgaas: changelog, move removal of unused SPEAr13xx #defines to separate
> patch, add only LTSSM_STATE_MASK and change all users for reviewability]
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>   drivers/pci/host/pci-imx6.c        |    3 +--
>   drivers/pci/host/pci-keystone-dw.c |    1 -
>   drivers/pci/host/pci-layerscape.c  |    1 -
>   drivers/pci/host/pcie-designware.h |    2 ++
>   4 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index 6f43086..7b0d120 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -55,7 +55,6 @@ struct imx6_pcie {
>   #define PCIE_PL_PFLR_LINK_STATE_MASK		(0x3f << 16)
>   #define PCIE_PL_PFLR_FORCE_LINK			(1 << 15)
>   #define PCIE_PHY_DEBUG_R0 (PL_OFFSET + 0x28)
> -#define PCIE_PHY_DEBUG_R0_LTSSM_MASK		(0x3f << 0)

>   #define PCIE_PHY_DEBUG_R1 (PL_OFFSET + 0x2c)
>   #define PCIE_PHY_DEBUG_R1_XMLH_LINK_IN_TRAINING	(1 << 29)
>   #define PCIE_PHY_DEBUG_R1_XMLH_LINK_UP		(1 << 4)
> @@ -508,7 +507,7 @@ static int imx6_pcie_link_up(struct pcie_port *pp)
>   	if (rx_valid & PCIE_PHY_RX_ASIC_OUT_VALID)
>   		return 0;
>
> -	if ((debug_r0 & PCIE_PHY_DEBUG_R0_LTSSM_MASK) != 0x0d)
> +	if ((debug_r0 & LTSSM_STATE_MASK) != 0x0d)
>   		return 0;
>
>   	dev_err(pp->dev, "transition to gen2 is stuck, reset PHY!\n");
> diff --git a/drivers/pci/host/pci-keystone-dw.c b/drivers/pci/host/pci-keystone-dw.c
> index e71da99..95a8b13 100644
> --- a/drivers/pci/host/pci-keystone-dw.c
> +++ b/drivers/pci/host/pci-keystone-dw.c
> @@ -25,7 +25,6 @@
>
>   /* Application register defines */
>   #define LTSSM_EN_VAL		        1
> -#define LTSSM_STATE_MASK		0x1f
>   #define LTSSM_STATE_L0			0x11
>   #define DBI_CS2_EN_VAL			0x20
>   #define OB_XLAT_EN_VAL		        2
> diff --git a/drivers/pci/host/pci-layerscape.c b/drivers/pci/host/pci-layerscape.c
> index b2328ea1..f02752e 100644
> --- a/drivers/pci/host/pci-layerscape.c
> +++ b/drivers/pci/host/pci-layerscape.c
> @@ -29,7 +29,6 @@
>   /* PEX1/2 Misc Ports Status Register */
>   #define SCFG_PEXMSCPORTSR(pex_idx)	(0x94 + (pex_idx) * 4)
>   #define LTSSM_STATE_SHIFT	20
> -#define LTSSM_STATE_MASK	0x3f
>   #define LTSSM_PCIE_L0		0x11 /* L0 state */
>
>   /* Symbol Timer Register and Filter Mask Register 1 */
> diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
> index d0bbd27..a1a76ff 100644
> --- a/drivers/pci/host/pcie-designware.h
> +++ b/drivers/pci/host/pcie-designware.h
> @@ -22,6 +22,8 @@
>   #define MAX_MSI_IRQS			32
>   #define MAX_MSI_CTRLS			(MAX_MSI_IRQS / 32)
>
> +#define LTSSM_STATE_MASK		0x1f
> +
>   struct pcie_port {
>   	struct device		*dev;
>   	u8			root_bus_nr;
>
>
>

For pci-keystone-dw.c

Acked-by: Murali Karicheri <m-karicheri2@ti.com>
-- 
Murali Karicheri
Linux Kernel, Keystone

Re: [PATCH v4 1/4] PCI: designware: Use common LTSSM_STATE_MASK definition

[Bjorn Helgaas]

[+cc Minghuan]

On Wed, Oct 21, 2015 at 01:42:50PM -0500, Bjorn Helgaas wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> Add a common #define for LTSSM_STATE_MASK and use it in all the
> DesignWare-based drivers.
> 
> Note that this changes LTSSM_STATE_MASK from 6 bits (0x3f) to 5 bits (0x1f)
> for i.MX6 and Layerscape.  We believe this is safe for all DesignWare-based
> PCIe cores.

OK, DesignWare experts, what's the story on this LTSSM_STATE_MASK?

Minghuan says Layerscape requires a mask of 0x3f and it actually uses
states 0x20, 0x21, 0x22, and 0x23:

MH> Our LTSSM mask is 0x3f because it includes the following states:
MH> 0x20 S_RCVRY_EQ0
MH> 0x21 S_RCVRY_EQ1
MH> 0x22 S_RCVRY_EQ2
MH> 0x23 S_RCVRY_EQ3

MH> And I checked DesignWare Cores PCI Express Controller Databook
MH> v4.21a and found the following descriptor:

MH> [5:0]: smlh_ltssm_state: LTSSM current state. Encoding is defined in workspace/src/include/cxpl_defs.vh

MH> So could we use the mask 0x3f for all SoCs?

I couldn't find the "DesignWare Cores PCI Express Controller Databook
v4.21a", but I do see the Keystone mask (sec 3.9.11 of
http://www.ti.com/lit/ug/sprugs6d/sprugs6d.pdf) is definitely only 5
bits, but that's clearly a TI register, not a generic DesignWare
register.

So it looks like a mistake to make a common LTSSM_STATE_MASK
definition.  It looks like this is something with some variation and
we shouldn't make assumptions about it being common.

Now I'm concerned that the LTSSM state definitions I put in
drivers/pci/host/pcie-designware.h might not actually apply to
everything derived from DW.  For example, the Layerscape S_RCVRY
states appear to be Layerscape-specific.

I don't want to put misleading stuff in
drivers/pci/host/pcie-designware.h if it's not really generic.  Better
to have it in the individual drivers, with a prefix that indicates
that it applies to that driver, e.g., KS_LTSSM_MASK, LS_LTSSM_MASK,
etc.

Bjorn

> [bhelgaas: changelog, move removal of unused SPEAr13xx #defines to separate
> patch, add only LTSSM_STATE_MASK and change all users for reviewability]
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/host/pci-imx6.c        |    3 +--
>  drivers/pci/host/pci-keystone-dw.c |    1 -
>  drivers/pci/host/pci-layerscape.c  |    1 -
>  drivers/pci/host/pcie-designware.h |    2 ++
>  4 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index 6f43086..7b0d120 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -55,7 +55,6 @@ struct imx6_pcie {
>  #define PCIE_PL_PFLR_LINK_STATE_MASK		(0x3f << 16)
>  #define PCIE_PL_PFLR_FORCE_LINK			(1 << 15)
>  #define PCIE_PHY_DEBUG_R0 (PL_OFFSET + 0x28)
> -#define PCIE_PHY_DEBUG_R0_LTSSM_MASK		(0x3f << 0)
>  #define PCIE_PHY_DEBUG_R1 (PL_OFFSET + 0x2c)
>  #define PCIE_PHY_DEBUG_R1_XMLH_LINK_IN_TRAINING	(1 << 29)
>  #define PCIE_PHY_DEBUG_R1_XMLH_LINK_UP		(1 << 4)
> @@ -508,7 +507,7 @@ static int imx6_pcie_link_up(struct pcie_port *pp)
>  	if (rx_valid & PCIE_PHY_RX_ASIC_OUT_VALID)
>  		return 0;
>  
> -	if ((debug_r0 & PCIE_PHY_DEBUG_R0_LTSSM_MASK) != 0x0d)
> +	if ((debug_r0 & LTSSM_STATE_MASK) != 0x0d)
>  		return 0;
>  
>  	dev_err(pp->dev, "transition to gen2 is stuck, reset PHY!\n");
> diff --git a/drivers/pci/host/pci-keystone-dw.c b/drivers/pci/host/pci-keystone-dw.c
> index e71da99..95a8b13 100644
> --- a/drivers/pci/host/pci-keystone-dw.c
> +++ b/drivers/pci/host/pci-keystone-dw.c
> @@ -25,7 +25,6 @@
>  
>  /* Application register defines */
>  #define LTSSM_EN_VAL		        1
> -#define LTSSM_STATE_MASK		0x1f
>  #define LTSSM_STATE_L0			0x11
>  #define DBI_CS2_EN_VAL			0x20
>  #define OB_XLAT_EN_VAL		        2
> diff --git a/drivers/pci/host/pci-layerscape.c b/drivers/pci/host/pci-layerscape.c
> index b2328ea1..f02752e 100644
> --- a/drivers/pci/host/pci-layerscape.c
> +++ b/drivers/pci/host/pci-layerscape.c
> @@ -29,7 +29,6 @@
>  /* PEX1/2 Misc Ports Status Register */
>  #define SCFG_PEXMSCPORTSR(pex_idx)	(0x94 + (pex_idx) * 4)
>  #define LTSSM_STATE_SHIFT	20
> -#define LTSSM_STATE_MASK	0x3f
>  #define LTSSM_PCIE_L0		0x11 /* L0 state */
>  
>  /* Symbol Timer Register and Filter Mask Register 1 */
> diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
> index d0bbd27..a1a76ff 100644
> --- a/drivers/pci/host/pcie-designware.h
> +++ b/drivers/pci/host/pcie-designware.h
> @@ -22,6 +22,8 @@
>  #define MAX_MSI_IRQS			32
>  #define MAX_MSI_CTRLS			(MAX_MSI_IRQS / 32)
>  
> +#define LTSSM_STATE_MASK		0x1f
> +
>  struct pcie_port {
>  	struct device		*dev;
>  	u8			root_bus_nr;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/4] PCI: designware: Use common LTSSM_STATE_MASK definition

[Fabio Estevam]

Hi Bjorn,

On Tue, Oct 27, 2015 at 1:15 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> [+cc Minghuan]
>
> On Wed, Oct 21, 2015 at 01:42:50PM -0500, Bjorn Helgaas wrote:
>> From: Fabio Estevam <fabio.estevam@freescale.com>
>>
>> Add a common #define for LTSSM_STATE_MASK and use it in all the
>> DesignWare-based drivers.
>>
>> Note that this changes LTSSM_STATE_MASK from 6 bits (0x3f) to 5 bits (0x1f)
>> for i.MX6 and Layerscape.  We believe this is safe for all DesignWare-based
>> PCIe cores.
>
> OK, DesignWare experts, what's the story on this LTSSM_STATE_MASK?
>
> Minghuan says Layerscape requires a mask of 0x3f and it actually uses
> states 0x20, 0x21, 0x22, and 0x23:
>
> MH> Our LTSSM mask is 0x3f because it includes the following states:
> MH> 0x20 S_RCVRY_EQ0
> MH> 0x21 S_RCVRY_EQ1
> MH> 0x22 S_RCVRY_EQ2
> MH> 0x23 S_RCVRY_EQ3
>
> MH> And I checked DesignWare Cores PCI Express Controller Databook
> MH> v4.21a and found the following descriptor:
>
> MH> [5:0]: smlh_ltssm_state: LTSSM current state. Encoding is defined in workspace/src/include/cxpl_defs.vh
>
> MH> So could we use the mask 0x3f for all SoCs?
>
> I couldn't find the "DesignWare Cores PCI Express Controller Databook
> v4.21a", but I do see the Keystone mask (sec 3.9.11 of
> http://www.ti.com/lit/ug/sprugs6d/sprugs6d.pdf) is definitely only 5
> bits, but that's clearly a TI register, not a generic DesignWare
> register.
>
> So it looks like a mistake to make a common LTSSM_STATE_MASK
> definition.  It looks like this is something with some variation and
> we shouldn't make assumptions about it being common.

Yes, what if we keep the original behaviour: define a common
LTSSM_STATE_MASK as 0x3f and then add KS_LTSSM_STATE_MASK as 0x1f for
Keystone only?

>
> Now I'm concerned that the LTSSM state definitions I put in
> drivers/pci/host/pcie-designware.h might not actually apply to
> everything derived from DW.  For example, the Layerscape S_RCVRY
> states appear to be Layerscape-specific.
>
> I don't want to put misleading stuff in
> drivers/pci/host/pcie-designware.h if it's not really generic.  Better
> to have it in the individual drivers, with a prefix that indicates
> that it applies to that driver, e.g., KS_LTSSM_MASK, LS_LTSSM_MASK,
> etc.

The LTSSM states we put in drivers/pci/host/pcie-designware.h are from
0-1f, so they are common.

Would you like to drop this series from your tree and then I can
resend it with the proposed changes above if everyone agrees?

Thanks,

Fabio Estevam

Re: [PATCH v4 1/4] PCI: designware: Use common LTSSM_STATE_MASK definition

[Bjorn Helgaas]

On Tue, Oct 27, 2015 at 02:05:17PM -0200, Fabio Estevam wrote:
> Hi Bjorn,
> 
> On Tue, Oct 27, 2015 at 1:15 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > [+cc Minghuan]
> >
> > On Wed, Oct 21, 2015 at 01:42:50PM -0500, Bjorn Helgaas wrote:
> >> From: Fabio Estevam <fabio.estevam@freescale.com>
> >>
> >> Add a common #define for LTSSM_STATE_MASK and use it in all the
> >> DesignWare-based drivers.
> >>
> >> Note that this changes LTSSM_STATE_MASK from 6 bits (0x3f) to 5 bits (0x1f)
> >> for i.MX6 and Layerscape.  We believe this is safe for all DesignWare-based
> >> PCIe cores.
> >
> > OK, DesignWare experts, what's the story on this LTSSM_STATE_MASK?
> >
> > Minghuan says Layerscape requires a mask of 0x3f and it actually uses
> > states 0x20, 0x21, 0x22, and 0x23:
> >
> > MH> Our LTSSM mask is 0x3f because it includes the following states:
> > MH> 0x20 S_RCVRY_EQ0
> > MH> 0x21 S_RCVRY_EQ1
> > MH> 0x22 S_RCVRY_EQ2
> > MH> 0x23 S_RCVRY_EQ3
> >
> > MH> And I checked DesignWare Cores PCI Express Controller Databook
> > MH> v4.21a and found the following descriptor:
> >
> > MH> [5:0]: smlh_ltssm_state: LTSSM current state. Encoding is defined in workspace/src/include/cxpl_defs.vh
> >
> > MH> So could we use the mask 0x3f for all SoCs?
> >
> > I couldn't find the "DesignWare Cores PCI Express Controller Databook
> > v4.21a", but I do see the Keystone mask (sec 3.9.11 of
> > http://www.ti.com/lit/ug/sprugs6d/sprugs6d.pdf) is definitely only 5
> > bits, but that's clearly a TI register, not a generic DesignWare
> > register.
> >
> > So it looks like a mistake to make a common LTSSM_STATE_MASK
> > definition.  It looks like this is something with some variation and
> > we shouldn't make assumptions about it being common.
> 
> Yes, what if we keep the original behaviour: define a common
> LTSSM_STATE_MASK as 0x3f and then add KS_LTSSM_STATE_MASK as 0x1f for
> Keystone only?
> 
> >
> > Now I'm concerned that the LTSSM state definitions I put in
> > drivers/pci/host/pcie-designware.h might not actually apply to
> > everything derived from DW.  For example, the Layerscape S_RCVRY
> > states appear to be Layerscape-specific.
> >
> > I don't want to put misleading stuff in
> > drivers/pci/host/pcie-designware.h if it's not really generic.  Better
> > to have it in the individual drivers, with a prefix that indicates
> > that it applies to that driver, e.g., KS_LTSSM_MASK, LS_LTSSM_MASK,
> > etc.
> 
> The LTSSM states we put in drivers/pci/host/pcie-designware.h are from
> 0-1f, so they are common.

Well, maybe.  Are those states documented in the DesignWare spec?  I
don't want to put things in pcie-designware.h that are only common by
accident or even by convention.  We should only put things there if
they are documented things that users of that IP can rely on.

LTSSM_MASK is documented in the TI Keystone spec, so its definition
probably belongs in pci-keystone-dw.c.  The same TI spec also contains
LTSSM state definitions, so I suspect they're in the same boat --
things that might accidentally be the same across devices, but they
don't *have* to be.

So I'm going to drop the following patches from my tree for now:

  1ad5fdbc8410 PCI: designware: Add LTSSM state definitions
  b09464f77dd2 PCI: designware: Use common LTSSM_STATE_L0 definition
  fa15c15fd95d PCI: designware: Use common LTSSM_STATE_RCVRY_LOCK definition
  4788fe6ebf45 PCI: designware: Use common LTSSM_STATE_MASK definition

We can add pieces back if they make sense.  If we add things to shared
files like pcie-designware.h, I'd like a reference to the DW spec that
justifies the sharing.

Bjorn

Re: [PATCH v4 1/4] PCI: designware: Use common LTSSM_STATE_MASK definition

[Fabio Estevam]

On Tue, Oct 27, 2015 at 2:20 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:

> Well, maybe.  Are those states documented in the DesignWare spec?  I

Unfortunately I cannot find them in DesignWare spec that I have access to.

> don't want to put things in pcie-designware.h that are only common by
> accident or even by convention.  We should only put things there if
> they are documented things that users of that IP can rely on.
>
> LTSSM_MASK is documented in the TI Keystone spec, so its definition
> probably belongs in pci-keystone-dw.c.  The same TI spec also contains
> LTSSM state definitions, so I suspect they're in the same boat --
> things that might accidentally be the same across devices, but they
> don't *have* to be.
>
> So I'm going to drop the following patches from my tree for now:
>
>   1ad5fdbc8410 PCI: designware: Add LTSSM state definitions
>   b09464f77dd2 PCI: designware: Use common LTSSM_STATE_L0 definition
>   fa15c15fd95d PCI: designware: Use common LTSSM_STATE_RCVRY_LOCK definition
>   4788fe6ebf45 PCI: designware: Use common LTSSM_STATE_MASK definition
>
> We can add pieces back if they make sense.  If we add things to shared
> files like pcie-designware.h, I'd like a reference to the DW spec that
> justifies the sharing.

Fair enough, thanks.

[PATCH v4 2/4] PCI: designware: Use common LTSSM_STATE_RCVRY_LOCK definition

[Bjorn Helgaas]

From: Fabio Estevam <fabio.estevam@freescale.com>

Add a #define for LTSSM_STATE_RCVRY_LOCK and use it to improve code
readability.

[bhelgaas: include both definition and use for reviewability]
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/host/pci-imx6.c        |    2 +-
 drivers/pci/host/pcie-designware.h |    1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index 7b0d120..4c788d27 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -507,7 +507,7 @@ static int imx6_pcie_link_up(struct pcie_port *pp)
 	if (rx_valid & PCIE_PHY_RX_ASIC_OUT_VALID)
 		return 0;
 
-	if ((debug_r0 & LTSSM_STATE_MASK) != 0x0d)
+	if ((debug_r0 & LTSSM_STATE_MASK) != LTSSM_STATE_RCVRY_LOCK)
 		return 0;
 
 	dev_err(pp->dev, "transition to gen2 is stuck, reset PHY!\n");
diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
index a1a76ff..f3a7583 100644
--- a/drivers/pci/host/pcie-designware.h
+++ b/drivers/pci/host/pcie-designware.h
@@ -22,6 +22,7 @@
 #define MAX_MSI_IRQS			32
 #define MAX_MSI_CTRLS			(MAX_MSI_IRQS / 32)
 
+#define LTSSM_STATE_RCVRY_LOCK		0x0d
 #define LTSSM_STATE_MASK		0x1f
 
 struct pcie_port {

[PATCH v4 3/4] PCI: designware: Use common LTSSM_STATE_L0 definition

[Bjorn Helgaas]

Add a common #define for LTSSM_STATE_L0 and use it in all the
DesignWare-based drivers.

Based-on-patch-from: Fabio Estevam <fabio.estevam@freescale.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/host/pci-keystone-dw.c |    1 -
 drivers/pci/host/pci-layerscape.c  |    3 +--
 drivers/pci/host/pcie-designware.h |    1 +
 3 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/host/pci-keystone-dw.c b/drivers/pci/host/pci-keystone-dw.c
index 95a8b13..c00ba57 100644
--- a/drivers/pci/host/pci-keystone-dw.c
+++ b/drivers/pci/host/pci-keystone-dw.c
@@ -25,7 +25,6 @@
 
 /* Application register defines */
 #define LTSSM_EN_VAL		        1
-#define LTSSM_STATE_L0			0x11
 #define DBI_CS2_EN_VAL			0x20
 #define OB_XLAT_EN_VAL		        2
 
diff --git a/drivers/pci/host/pci-layerscape.c b/drivers/pci/host/pci-layerscape.c
index f02752e..930b193 100644
--- a/drivers/pci/host/pci-layerscape.c
+++ b/drivers/pci/host/pci-layerscape.c
@@ -29,7 +29,6 @@
 /* PEX1/2 Misc Ports Status Register */
 #define SCFG_PEXMSCPORTSR(pex_idx)	(0x94 + (pex_idx) * 4)
 #define LTSSM_STATE_SHIFT	20
-#define LTSSM_PCIE_L0		0x11 /* L0 state */
 
 /* Symbol Timer Register and Filter Mask Register 1 */
 #define PCIE_STRFMR1 0x71c
@@ -55,7 +54,7 @@ static int ls_pcie_link_up(struct pcie_port *pp)
 	regmap_read(pcie->scfg, SCFG_PEXMSCPORTSR(pcie->index), &state);
 	state = (state >> LTSSM_STATE_SHIFT) & LTSSM_STATE_MASK;
 
-	if (state < LTSSM_PCIE_L0)
+	if (state < LTSSM_STATE_L0)
 		return 0;
 
 	return 1;
diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
index f3a7583..33f74a8 100644
--- a/drivers/pci/host/pcie-designware.h
+++ b/drivers/pci/host/pcie-designware.h
@@ -23,6 +23,7 @@
 #define MAX_MSI_CTRLS			(MAX_MSI_IRQS / 32)
 
 #define LTSSM_STATE_RCVRY_LOCK		0x0d
+#define LTSSM_STATE_L0			0x11
 #define LTSSM_STATE_MASK		0x1f
 
 struct pcie_port {

Re: [PATCH v4 3/4] PCI: designware: Use common LTSSM_STATE_L0 definition

[Murali Karicheri]

On 10/21/2015 02:43 PM, Bjorn Helgaas wrote:
> Add a common #define for LTSSM_STATE_L0 and use it in all the
> DesignWare-based drivers.
>
> Based-on-patch-from: Fabio Estevam <fabio.estevam@freescale.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>   drivers/pci/host/pci-keystone-dw.c |    1 -
>   drivers/pci/host/pci-layerscape.c  |    3 +--
>   drivers/pci/host/pcie-designware.h |    1 +
>   3 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/host/pci-keystone-dw.c b/drivers/pci/host/pci-keystone-dw.c
> index 95a8b13..c00ba57 100644
> --- a/drivers/pci/host/pci-keystone-dw.c
> +++ b/drivers/pci/host/pci-keystone-dw.c
> @@ -25,7 +25,6 @@
>
>   /* Application register defines */
>   #define LTSSM_EN_VAL		        1
> -#define LTSSM_STATE_L0			0x11
>   #define DBI_CS2_EN_VAL			0x20
>   #define OB_XLAT_EN_VAL		        2
>
> diff --git a/drivers/pci/host/pci-layerscape.c b/drivers/pci/host/pci-layerscape.c
> index f02752e..930b193 100644
> --- a/drivers/pci/host/pci-layerscape.c
> +++ b/drivers/pci/host/pci-layerscape.c
> @@ -29,7 +29,6 @@
>   /* PEX1/2 Misc Ports Status Register */
>   #define SCFG_PEXMSCPORTSR(pex_idx)	(0x94 + (pex_idx) * 4)
>   #define LTSSM_STATE_SHIFT	20
> -#define LTSSM_PCIE_L0		0x11 /* L0 state */
>
>   /* Symbol Timer Register and Filter Mask Register 1 */
>   #define PCIE_STRFMR1 0x71c
> @@ -55,7 +54,7 @@ static int ls_pcie_link_up(struct pcie_port *pp)
>   	regmap_read(pcie->scfg, SCFG_PEXMSCPORTSR(pcie->index), &state);
>   	state = (state >> LTSSM_STATE_SHIFT) & LTSSM_STATE_MASK;
>
> -	if (state < LTSSM_PCIE_L0)
> +	if (state < LTSSM_STATE_L0)
>   		return 0;
>
>   	return 1;
> diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
> index f3a7583..33f74a8 100644
> --- a/drivers/pci/host/pcie-designware.h
> +++ b/drivers/pci/host/pcie-designware.h
> @@ -23,6 +23,7 @@
>   #define MAX_MSI_CTRLS			(MAX_MSI_IRQS / 32)
>
>   #define LTSSM_STATE_RCVRY_LOCK		0x0d
> +#define LTSSM_STATE_L0			0x11
>   #define LTSSM_STATE_MASK		0x1f
>
>   struct pcie_port {
>
>
>

for pci-keystone-dw.c
Acked-by: Murali Karicheri <m-karicheri2@ti.com>

-- 
Murali Karicheri
Linux Kernel, Keystone

[PATCH v4 4/4] PCI: spear: Remove unused #defines

[Bjorn Helgaas]

From: Fabio Estevam <fabio.estevam@freescale.com>

Remove unused #defines.

[bhelgaas: split into separate patch]
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/host/pcie-spear13xx.c |   70 -------------------------------------
 1 file changed, 70 deletions(-)

diff --git a/drivers/pci/host/pcie-spear13xx.c b/drivers/pci/host/pcie-spear13xx.c
index 98d2683..10e33ad 100644
--- a/drivers/pci/host/pcie-spear13xx.c
+++ b/drivers/pci/host/pcie-spear13xx.c
@@ -58,87 +58,17 @@ struct pcie_app_reg {
 };
 
 /* CR0 ID */
-#define RX_LANE_FLIP_EN_ID			0
-#define TX_LANE_FLIP_EN_ID			1
-#define SYS_AUX_PWR_DET_ID			2
 #define APP_LTSSM_ENABLE_ID			3
-#define SYS_ATTEN_BUTTON_PRESSED_ID		4
-#define SYS_MRL_SENSOR_STATE_ID			5
-#define SYS_PWR_FAULT_DET_ID			6
-#define SYS_MRL_SENSOR_CHGED_ID			7
-#define SYS_PRE_DET_CHGED_ID			8
-#define SYS_CMD_CPLED_INT_ID			9
-#define APP_INIT_RST_0_ID			11
-#define APP_REQ_ENTR_L1_ID			12
-#define APP_READY_ENTR_L23_ID			13
-#define APP_REQ_EXIT_L1_ID			14
-#define DEVICE_TYPE_EP				(0 << 25)
-#define DEVICE_TYPE_LEP				(1 << 25)
 #define DEVICE_TYPE_RC				(4 << 25)
-#define SYS_INT_ID				29
 #define MISCTRL_EN_ID				30
 #define REG_TRANSLATION_ENABLE			31
 
-/* CR1 ID */
-#define APPS_PM_XMT_TURNOFF_ID			2
-#define APPS_PM_XMT_PME_ID			5
-
 /* CR3 ID */
-#define XMLH_LTSSM_STATE_DETECT_QUIET		0x00
-#define XMLH_LTSSM_STATE_DETECT_ACT		0x01
-#define XMLH_LTSSM_STATE_POLL_ACTIVE		0x02
-#define XMLH_LTSSM_STATE_POLL_COMPLIANCE	0x03
-#define XMLH_LTSSM_STATE_POLL_CONFIG		0x04
-#define XMLH_LTSSM_STATE_PRE_DETECT_QUIET	0x05
-#define XMLH_LTSSM_STATE_DETECT_WAIT		0x06
-#define XMLH_LTSSM_STATE_CFG_LINKWD_START	0x07
-#define XMLH_LTSSM_STATE_CFG_LINKWD_ACEPT	0x08
-#define XMLH_LTSSM_STATE_CFG_LANENUM_WAIT	0x09
-#define XMLH_LTSSM_STATE_CFG_LANENUM_ACEPT	0x0A
-#define XMLH_LTSSM_STATE_CFG_COMPLETE		0x0B
-#define XMLH_LTSSM_STATE_CFG_IDLE		0x0C
-#define XMLH_LTSSM_STATE_RCVRY_LOCK		0x0D
-#define XMLH_LTSSM_STATE_RCVRY_SPEED		0x0E
-#define XMLH_LTSSM_STATE_RCVRY_RCVRCFG		0x0F
-#define XMLH_LTSSM_STATE_RCVRY_IDLE		0x10
-#define XMLH_LTSSM_STATE_L0			0x11
-#define XMLH_LTSSM_STATE_L0S			0x12
-#define XMLH_LTSSM_STATE_L123_SEND_EIDLE	0x13
-#define XMLH_LTSSM_STATE_L1_IDLE		0x14
-#define XMLH_LTSSM_STATE_L2_IDLE		0x15
-#define XMLH_LTSSM_STATE_L2_WAKE		0x16
-#define XMLH_LTSSM_STATE_DISABLED_ENTRY		0x17
-#define XMLH_LTSSM_STATE_DISABLED_IDLE		0x18
-#define XMLH_LTSSM_STATE_DISABLED		0x19
-#define XMLH_LTSSM_STATE_LPBK_ENTRY		0x1A
-#define XMLH_LTSSM_STATE_LPBK_ACTIVE		0x1B
-#define XMLH_LTSSM_STATE_LPBK_EXIT		0x1C
-#define XMLH_LTSSM_STATE_LPBK_EXIT_TIMEOUT	0x1D
-#define XMLH_LTSSM_STATE_HOT_RESET_ENTRY	0x1E
-#define XMLH_LTSSM_STATE_HOT_RESET		0x1F
-#define XMLH_LTSSM_STATE_MASK			0x3F
 #define XMLH_LINK_UP				(1 << 6)
 
-/* CR4 ID */
-#define CFG_MSI_EN_ID				18
-
 /* CR6 */
-#define INTA_CTRL_INT				(1 << 7)
-#define INTB_CTRL_INT				(1 << 8)
-#define INTC_CTRL_INT				(1 << 9)
-#define INTD_CTRL_INT				(1 << 10)
 #define MSI_CTRL_INT				(1 << 26)
 
-/* CR19 ID */
-#define VEN_MSI_REQ_ID				11
-#define VEN_MSI_FUN_NUM_ID			8
-#define VEN_MSI_TC_ID				5
-#define VEN_MSI_VECTOR_ID			0
-#define VEN_MSI_REQ_EN		((u32)0x1 << VEN_MSI_REQ_ID)
-#define VEN_MSI_FUN_NUM_MASK	((u32)0x7 << VEN_MSI_FUN_NUM_ID)
-#define VEN_MSI_TC_MASK		((u32)0x7 << VEN_MSI_TC_ID)
-#define VEN_MSI_VECTOR_MASK	((u32)0x1F << VEN_MSI_VECTOR_ID)
-
 #define EXP_CAP_ID_OFFSET			0x70
 
 #define to_spear13xx_pcie(x)	container_of(x, struct spear13xx_pcie, pp)

Re: [PATCH v4 0/4] PCI: designware: LTSSM #define cleanup

[Fabio Estevam]

Hi Bjorn,

On Wed, Oct 21, 2015 at 4:42 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> This is a revision of Fabio's series:
> http://lkml.kernel.org/r/1444664808-16445-1-git-send-email-festevam@gmail.com
>
> Lucas, you reviewed the v3 patches, but I fiddled enough with this that I
> didn't want to blindly carry your review forward.  I don't *think* I
> changed anything substantive, but I might have missed something.
>
> Changes since Fabio's v3:
>   - Split removal of spear unused #defines to separate patch
>   - Combine LTSSM_STATE_MASK definition and use changes in one patch
>   - Combine LTSSM_STATE_RCVRY_LOCK definition and use in one patch
>   - Use common LTSSM_STATE_L0 definition

Thanks for reworking the series. It looks better now, thanks.

Re: [PATCH v4 0/4] PCI: designware: LTSSM #define cleanup

[Lucas Stach]

Hi Bjorn,

Am Mittwoch, den 21.10.2015, 13:42 -0500 schrieb Bjorn Helgaas:
> This is a revision of Fabio's series:
> http://lkml.kernel.org/r/1444664808-16445-1-git-send-email-festevam@gmail.com
> 
> Lucas, you reviewed the v3 patches, but I fiddled enough with this that I
> didn't want to blindly carry your review forward.  I don't *think* I
> changed anything substantive, but I might have missed something.
> 
I'm not sure if I like the removal of all the LTSSM state defines, as
not all reference manuals include them and so I liked to have the header
as a reference. But if you prefer to not carry unused defines in the
kernel I won't object strongly to the removal.

Otherwise the series looks fine, which you may take as a Reviewed-by.

Regards,
Lucas

> Changes since Fabio's v3:
>   - Split removal of spear unused #defines to separate patch
>   - Combine LTSSM_STATE_MASK definition and use changes in one patch
>   - Combine LTSSM_STATE_RCVRY_LOCK definition and use in one patch
>   - Use common LTSSM_STATE_L0 definition
> 
> ---
> 
> Bjorn Helgaas (1):
>       PCI: designware: Use common LTSSM_STATE_L0 definition
> 
> Fabio Estevam (3):
>       PCI: designware: Use common LTSSM_STATE_MASK definition
>       PCI: designware: Use common LTSSM_STATE_RCVRY_LOCK definition
>       PCI: spear: Remove unused #defines
> 
> 
>  drivers/pci/host/pci-imx6.c        |    3 +-
>  drivers/pci/host/pci-keystone-dw.c |    2 -
>  drivers/pci/host/pci-layerscape.c  |    4 +-
>  drivers/pci/host/pcie-designware.h |    4 ++
>  drivers/pci/host/pcie-spear13xx.c  |   70 ------------------------------------
>  5 files changed, 6 insertions(+), 77 deletions(-)

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

Re: [PATCH v4 0/4] PCI: designware: LTSSM #define cleanup

[Bjorn Helgaas]

On Thu, Oct 22, 2015 at 11:04:26AM +0200, Lucas Stach wrote:
> Hi Bjorn,
> 
> Am Mittwoch, den 21.10.2015, 13:42 -0500 schrieb Bjorn Helgaas:
> > This is a revision of Fabio's series:
> > http://lkml.kernel.org/r/1444664808-16445-1-git-send-email-festevam@gmail.com
> > 
> > Lucas, you reviewed the v3 patches, but I fiddled enough with this that I
> > didn't want to blindly carry your review forward.  I don't *think* I
> > changed anything substantive, but I might have missed something.
> > 
> I'm not sure if I like the removal of all the LTSSM state defines, as
> not all reference manuals include them and so I liked to have the header
> as a reference. But if you prefer to not carry unused defines in the
> kernel I won't object strongly to the removal.
> 
> Otherwise the series looks fine, which you may take as a Reviewed-by.

Thanks, Lucas.

OK, I put the rest of the LTSSM #defines back in
drivers/pci/host/pcie-designware.h as a separate patch (below):


commit 25026d3fcff7dd0e139b04cb52436b9c7e0e545e
Author: Fabio Estevam <fabio.estevam@freescale.com>
Date:   Thu Oct 22 10:24:52 2015 -0500

    PCI: designware: Add LTSSM state definitions
    
    Add the rest of the LTSSM state definitions.  These aren't currently used,
    so they're here as documentation.
    
    [bhelgaas: split into separate patch]
    Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
index efe5fca..8cb5725 100644
--- a/drivers/pci/host/pcie-designware.h
+++ b/drivers/pci/host/pcie-designware.h
@@ -22,8 +22,39 @@
 #define MAX_MSI_IRQS			32
 #define MAX_MSI_CTRLS			(MAX_MSI_IRQS / 32)
 
+#define LTSSM_STATE_DETECT_QUIET	0x00
+#define LTSSM_STATE_DETECT_ACT		0x01
+#define LTSSM_STATE_POLL_ACTIVE		0x02
+#define LTSSM_STATE_POLL_COMPLIANCE	0x03
+#define LTSSM_STATE_POLL_CONFIG		0x04
+#define LTSSM_STATE_PRE_DETECT_QUIET	0x05
+#define LTSSM_STATE_DETECT_WAIT		0x06
+#define LTSSM_STATE_CFG_LINKWD_START	0x07
+#define LTSSM_STATE_CFG_LINKWD_ACCEPT	0x08
+#define LTSSM_STATE_CFG_LANENUM_WAIT	0x09
+#define LTSSM_STATE_CFG_LANENUM_ACCEPT	0x0a
+#define LTSSM_STATE_CFG_COMPLETE	0x0b
+#define LTSSM_STATE_CFG_IDLE		0x0c
 #define LTSSM_STATE_RCVRY_LOCK		0x0d
+#define LTSSM_STATE_RCVRY_SPEED		0x0e
+#define LTSSM_STATE_RCVRY_RCVRCFG	0x0f
+#define LTSSM_STATE_RCVRY_IDLE		0x10
 #define LTSSM_STATE_L0			0x11
+#define LTSSM_STATE_L0S			0x12
+#define LTSSM_STATE_L123_SEND_EIDLE	0x13
+#define LTSSM_STATE_L1_IDLE		0x14
+#define LTSSM_STATE_L2_IDLE		0x15
+#define LTSSM_STATE_L2_WAKE		0x16
+#define LTSSM_STATE_DISABLED_ENTRY	0x17
+#define LTSSM_STATE_DISABLED_IDLE	0x18
+#define LTSSM_STATE_DISABLED		0x19
+#define LTSSM_STATE_LPBK_ENTRY		0x1a
+#define LTSSM_STATE_LPBK_ACTIVE		0x1b
+#define LTSSM_STATE_LPBK_EXIT		0x1c
+#define LTSSM_STATE_LPBK_EXIT_TIMEOUT	0x1d
+#define LTSSM_STATE_HOT_RESET_ENTRY	0x1e
+#define LTSSM_STATE_HOT_RESET		0x1f
+
 #define LTSSM_STATE_MASK		0x1f
 
 struct pcie_port {

Re: [PATCH v4 0/4] PCI: designware: LTSSM #define cleanup

[Pratyush Anand]

On Thu, Oct 22, 2015 at 9:04 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Thu, Oct 22, 2015 at 11:04:26AM +0200, Lucas Stach wrote:
>> I'm not sure if I like the removal of all the LTSSM state defines, as
>> not all reference manuals include them and so I liked to have the header
>> as a reference. But if you prefer to not carry unused defines in the
>> kernel I won't object strongly to the removal.
>>
>> Otherwise the series looks fine, which you may take as a Reviewed-by.
>
> Thanks, Lucas.
>
> OK, I put the rest of the LTSSM #defines back in
> drivers/pci/host/pcie-designware.h as a separate patch (below):
>

Thanks for putting it back. Its useful to debug issues related to link
training, which one finds many a time with new silicon. And as Lucas
pointed, its not mentioned in most of the user manual. I remember, I
had to grep it into RTL code :(.

For the series
Acked-by: Pratyush Anand <pratyush.anand@gmail.com>