linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/5] PCI: brcmstb: don't clk_put() a managed clock
@ 2020-04-30 18:55 Jim Quinlan
  2020-04-30 18:55 ` [PATCH 2/5] PCI: brcmstb: fix window register offset from 4 to 8 Jim Quinlan
                   ` (4 more replies)
  0 siblings, 5 replies; 15+ messages in thread
From: Jim Quinlan @ 2020-04-30 18:55 UTC (permalink / raw)
  To: james.quinlan
  Cc: Jim Quinlan, Nicolas Saenz Julienne, Lorenzo Pieralisi,
	Rob Herring, Bjorn Helgaas, Florian Fainelli,
	maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS, open list

From: Jim Quinlan <jquinlan@broadcom.com>

clk_put() was being invoked on a clock obtained by
devm_clk_get_optional().

Signed-off-by: Jim Quinlan <jquinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 6d79d14527a6..454917ee9241 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -899,7 +899,6 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
 	brcm_msi_remove(pcie);
 	brcm_pcie_turn_off(pcie);
 	clk_disable_unprepare(pcie->clk);
-	clk_put(pcie->clk);
 }
 
 static int brcm_pcie_remove(struct platform_device *pdev)
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/5] PCI: brcmstb: fix window register offset from 4 to 8
  2020-04-30 18:55 [PATCH 1/5] PCI: brcmstb: don't clk_put() a managed clock Jim Quinlan
@ 2020-04-30 18:55 ` Jim Quinlan
  2020-04-30 19:07   ` Florian Fainelli
  2020-04-30 20:43   ` Bjorn Helgaas
  2020-04-30 18:55 ` [PATCH 3/5] PCI: brcmstb: enable CRS Jim Quinlan
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 15+ messages in thread
From: Jim Quinlan @ 2020-04-30 18:55 UTC (permalink / raw)
  To: james.quinlan
  Cc: Jim Quinlan, Nicolas Saenz Julienne, Lorenzo Pieralisi,
	Rob Herring, Bjorn Helgaas, Florian Fainelli,
	maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS, open list

From: Jim Quinlan <jquinlan@broadcom.com>

The oubound memory window registers were being referenced
with an incorrect offset.  This probably wasn't noticed
previously as there was likely only one such outbound window.

Signed-off-by: Jim Quinlan <jquinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 454917ee9241..5b0dec5971b8 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -54,11 +54,11 @@
 
 #define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LO		0x400c
 #define PCIE_MEM_WIN0_LO(win)	\
-		PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LO + ((win) * 4)
+		PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LO + ((win) * 8)
 
 #define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_HI		0x4010
 #define PCIE_MEM_WIN0_HI(win)	\
-		PCIE_MISC_CPU_2_PCIE_MEM_WIN0_HI + ((win) * 4)
+		PCIE_MISC_CPU_2_PCIE_MEM_WIN0_HI + ((win) * 8)
 
 #define PCIE_MISC_RC_BAR1_CONFIG_LO			0x402c
 #define  PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK		0x1f
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 3/5] PCI: brcmstb: enable CRS
  2020-04-30 18:55 [PATCH 1/5] PCI: brcmstb: don't clk_put() a managed clock Jim Quinlan
  2020-04-30 18:55 ` [PATCH 2/5] PCI: brcmstb: fix window register offset from 4 to 8 Jim Quinlan
@ 2020-04-30 18:55 ` Jim Quinlan
  2020-04-30 19:19   ` Florian Fainelli
  2020-04-30 20:32   ` Bjorn Helgaas
  2020-04-30 18:55 ` [PATCH 4/5] dt-bindings: PCI: brcmstb: New prop 'brcm,aspm-en-l0s' Jim Quinlan
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 15+ messages in thread
From: Jim Quinlan @ 2020-04-30 18:55 UTC (permalink / raw)
  To: james.quinlan
  Cc: Jim Quinlan, Nicolas Saenz Julienne, Lorenzo Pieralisi,
	Rob Herring, Bjorn Helgaas, Florian Fainelli,
	maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS, open list

From: Jim Quinlan <jquinlan@broadcom.com>

Configuration Retry Request Status is off by default on this
PCIe controller.  Turn it on.

Signed-off-by: Jim Quinlan <jquinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 5b0dec5971b8..2bc913c0262c 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -34,6 +34,9 @@
 #define BRCM_PCIE_CAP_REGS				0x00ac
 
 /* Broadcom STB PCIe Register Offsets */
+#define PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL			0x00c8
+#define  PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL_RC_CRS_EN_MASK	0x10
+
 #define PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1				0x0188
 #define  PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1_ENDIAN_MODE_BAR2_MASK	0xc
 #define  PCIE_RC_CFG_VENDOR_SPCIFIC_REG1_LITTLE_ENDIAN			0x0
@@ -827,6 +830,12 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 		 pci_speed_string(pcie_link_speed[cls]), nlw,
 		 ssc_good ? "(SSC)" : "(!SSC)");
 
+	/* Enable configuration request retry (CRS) */
+	tmp = readl(base + PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL);
+	u32p_replace_bits(&tmp, 1,
+			  PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL_RC_CRS_EN_MASK);
+	writel(tmp, base + PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL);
+
 	/* PCIe->SCB endian mode for BAR */
 	tmp = readl(base + PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1);
 	u32p_replace_bits(&tmp, PCIE_RC_CFG_VENDOR_SPCIFIC_REG1_LITTLE_ENDIAN,
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 4/5] dt-bindings: PCI: brcmstb: New prop 'brcm,aspm-en-l0s'
  2020-04-30 18:55 [PATCH 1/5] PCI: brcmstb: don't clk_put() a managed clock Jim Quinlan
  2020-04-30 18:55 ` [PATCH 2/5] PCI: brcmstb: fix window register offset from 4 to 8 Jim Quinlan
  2020-04-30 18:55 ` [PATCH 3/5] PCI: brcmstb: enable CRS Jim Quinlan
@ 2020-04-30 18:55 ` Jim Quinlan
  2020-04-30 19:20   ` Florian Fainelli
  2020-04-30 18:55 ` [PATCH 5/5] PCI: brcmstb: disable L0s component of ASPM by default Jim Quinlan
  2020-04-30 19:05 ` [PATCH 1/5] PCI: brcmstb: don't clk_put() a managed clock Florian Fainelli
  4 siblings, 1 reply; 15+ messages in thread
From: Jim Quinlan @ 2020-04-30 18:55 UTC (permalink / raw)
  To: james.quinlan
  Cc: Jim Quinlan, Florian Fainelli,
	maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE,
	Nicolas Saenz Julienne, Bjorn Helgaas, Rob Herring,
	moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	open list:PCI SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list

From: Jim Quinlan <jquinlan@broadcom.com>

For various reasons, the L0s component of ASPM is intentionally
disabled.  Specifying the 'brcm,aspm-en-l0s' property enables it.

Signed-off-by: Jim Quinlan <jquinlan@broadcom.com>
---
 Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index 77d3e81a437b..b3e43597b89c 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -56,6 +56,10 @@ properties:
     description: Indicates usage of spread-spectrum clocking.
     type: boolean
 
+  brcm,aspm-en-l0s:
+    description: Enables ASPM L0s savings; off by default.
+    type: boolean
+
 required:
   - reg
   - dma-ranges
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 5/5] PCI: brcmstb: disable L0s component of ASPM by default
  2020-04-30 18:55 [PATCH 1/5] PCI: brcmstb: don't clk_put() a managed clock Jim Quinlan
                   ` (2 preceding siblings ...)
  2020-04-30 18:55 ` [PATCH 4/5] dt-bindings: PCI: brcmstb: New prop 'brcm,aspm-en-l0s' Jim Quinlan
@ 2020-04-30 18:55 ` Jim Quinlan
  2020-04-30 19:21   ` Florian Fainelli
  2020-04-30 20:40   ` Bjorn Helgaas
  2020-04-30 19:05 ` [PATCH 1/5] PCI: brcmstb: don't clk_put() a managed clock Florian Fainelli
  4 siblings, 2 replies; 15+ messages in thread
From: Jim Quinlan @ 2020-04-30 18:55 UTC (permalink / raw)
  To: james.quinlan
  Cc: Jim Quinlan, Nicolas Saenz Julienne, Lorenzo Pieralisi,
	Rob Herring, Bjorn Helgaas, Florian Fainelli,
	maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS, open list

From: Jim Quinlan <jquinlan@broadcom.com>

Some informal internal experiments has shown that the BrcmSTB ASPM L0s
savings may introduce an undesirable noise signal on some customers'
boards.  In addition, L0s was found lacking in realized power savings,
especially relative to the L1 ASPM component.  This is BrcmSTB's
experience and may not hold for others.  At any rate, we disable L0s
savings by default unless the DT node has the 'brcm,aspm-en-l0s'
property.

Signed-off-by: Jim Quinlan <jquinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 2bc913c0262c..bc1d514b19e4 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -44,6 +44,9 @@
 #define PCIE_RC_CFG_PRIV1_ID_VAL3			0x043c
 #define  PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK	0xffffff
 
+#define PCIE_RC_CFG_PRIV1_LINK_CAPABILITY			0x04dc
+#define  PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK	0xc00
+
 #define PCIE_RC_DL_MDIO_ADDR				0x1100
 #define PCIE_RC_DL_MDIO_WR_DATA				0x1104
 #define PCIE_RC_DL_MDIO_RD_DATA				0x1108
@@ -696,7 +699,7 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 	int num_out_wins = 0;
 	u16 nlw, cls, lnksta;
 	int i, ret;
-	u32 tmp;
+	u32 tmp, aspm_support;
 
 	/* Reset the bridge */
 	brcm_pcie_bridge_sw_init_set(pcie, 1);
@@ -806,6 +809,15 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 		num_out_wins++;
 	}
 
+	/* Only support ASPM L1 unless L0s is explicitly desired */
+	aspm_support = PCIE_LINK_STATE_L1;
+	if (of_property_read_bool(pcie->np, "brcm,aspm-en-l0s"))
+		aspm_support |= PCIE_LINK_STATE_L0S;
+	tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
+	u32p_replace_bits(&tmp, aspm_support,
+		PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK);
+	writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
+
 	/*
 	 * For config space accesses on the RC, show the right class for
 	 * a PCIe-PCIe bridge (the default setting is to be EP mode).
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/5] PCI: brcmstb: don't clk_put() a managed clock
  2020-04-30 18:55 [PATCH 1/5] PCI: brcmstb: don't clk_put() a managed clock Jim Quinlan
                   ` (3 preceding siblings ...)
  2020-04-30 18:55 ` [PATCH 5/5] PCI: brcmstb: disable L0s component of ASPM by default Jim Quinlan
@ 2020-04-30 19:05 ` Florian Fainelli
  4 siblings, 0 replies; 15+ messages in thread
From: Florian Fainelli @ 2020-04-30 19:05 UTC (permalink / raw)
  To: Jim Quinlan
  Cc: Nicolas Saenz Julienne, Lorenzo Pieralisi, Rob Herring,
	Bjorn Helgaas, maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS, open list

On 4/30/20 11:55 AM, Jim Quinlan wrote:
> From: Jim Quinlan <jquinlan@broadcom.com>
> 
> clk_put() was being invoked on a clock obtained by
> devm_clk_get_optional().
> 
> Signed-off-by: Jim Quinlan <jquinlan@broadcom.com>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>

Fixes: c0452137034b ("PCI: brcmstb: Add Broadcom STB PCIe host
controller driver")
-- 
Florian

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/5] PCI: brcmstb: fix window register offset from 4 to 8
  2020-04-30 18:55 ` [PATCH 2/5] PCI: brcmstb: fix window register offset from 4 to 8 Jim Quinlan
@ 2020-04-30 19:07   ` Florian Fainelli
  2020-04-30 20:43   ` Bjorn Helgaas
  1 sibling, 0 replies; 15+ messages in thread
From: Florian Fainelli @ 2020-04-30 19:07 UTC (permalink / raw)
  To: Jim Quinlan
  Cc: Nicolas Saenz Julienne, Lorenzo Pieralisi, Rob Herring,
	Bjorn Helgaas, Florian Fainelli,
	maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS, open list

On 4/30/20 11:55 AM, Jim Quinlan wrote:
> From: Jim Quinlan <jquinlan@broadcom.com>
> 
> The oubound memory window registers were being referenced
> with an incorrect offset.  This probably wasn't noticed
> previously as there was likely only one such outbound window.
> 
> Signed-off-by: Jim Quinlan <jquinlan@broadcom.com>

Fixes: c0452137034b ("PCI: brcmstb: Add Broadcom STB PCIe host
controller driver")

Acked-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/5] PCI: brcmstb: enable CRS
  2020-04-30 18:55 ` [PATCH 3/5] PCI: brcmstb: enable CRS Jim Quinlan
@ 2020-04-30 19:19   ` Florian Fainelli
  2020-04-30 20:32   ` Bjorn Helgaas
  1 sibling, 0 replies; 15+ messages in thread
From: Florian Fainelli @ 2020-04-30 19:19 UTC (permalink / raw)
  To: Jim Quinlan
  Cc: Nicolas Saenz Julienne, Lorenzo Pieralisi, Rob Herring,
	Bjorn Helgaas, maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS, open list

On 4/30/20 11:55 AM, Jim Quinlan wrote:
> From: Jim Quinlan <jquinlan@broadcom.com>
> 
> Configuration Retry Request Status is off by default on this
> PCIe controller.  Turn it on.
> 
> Signed-off-by: Jim Quinlan <jquinlan@broadcom.com>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/5] dt-bindings: PCI: brcmstb: New prop 'brcm,aspm-en-l0s'
  2020-04-30 18:55 ` [PATCH 4/5] dt-bindings: PCI: brcmstb: New prop 'brcm,aspm-en-l0s' Jim Quinlan
@ 2020-04-30 19:20   ` Florian Fainelli
  0 siblings, 0 replies; 15+ messages in thread
From: Florian Fainelli @ 2020-04-30 19:20 UTC (permalink / raw)
  To: Jim Quinlan
  Cc: maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE,
	Nicolas Saenz Julienne, Bjorn Helgaas, Rob Herring,
	moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	open list:PCI SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list

On 4/30/20 11:55 AM, Jim Quinlan wrote:
> From: Jim Quinlan <jquinlan@broadcom.com>
> 
> For various reasons, the L0s component of ASPM is intentionally
> disabled.  Specifying the 'brcm,aspm-en-l0s' property enables it.
> 
> Signed-off-by: Jim Quinlan <jquinlan@broadcom.com>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 5/5] PCI: brcmstb: disable L0s component of ASPM by default
  2020-04-30 18:55 ` [PATCH 5/5] PCI: brcmstb: disable L0s component of ASPM by default Jim Quinlan
@ 2020-04-30 19:21   ` Florian Fainelli
  2020-04-30 20:40   ` Bjorn Helgaas
  1 sibling, 0 replies; 15+ messages in thread
From: Florian Fainelli @ 2020-04-30 19:21 UTC (permalink / raw)
  To: Jim Quinlan
  Cc: Nicolas Saenz Julienne, Lorenzo Pieralisi, Rob Herring,
	Bjorn Helgaas, maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS, open list

On 4/30/20 11:55 AM, Jim Quinlan wrote:
> From: Jim Quinlan <jquinlan@broadcom.com>
> 
> Some informal internal experiments has shown that the BrcmSTB ASPM L0s
> savings may introduce an undesirable noise signal on some customers'
> boards.  In addition, L0s was found lacking in realized power savings,
> especially relative to the L1 ASPM component.  This is BrcmSTB's
> experience and may not hold for others.  At any rate, we disable L0s
> savings by default unless the DT node has the 'brcm,aspm-en-l0s'
> property.
> 
> Signed-off-by: Jim Quinlan <jquinlan@broadcom.com>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/5] PCI: brcmstb: enable CRS
  2020-04-30 18:55 ` [PATCH 3/5] PCI: brcmstb: enable CRS Jim Quinlan
  2020-04-30 19:19   ` Florian Fainelli
@ 2020-04-30 20:32   ` Bjorn Helgaas
  2020-04-30 21:00     ` Jim Quinlan
  1 sibling, 1 reply; 15+ messages in thread
From: Bjorn Helgaas @ 2020-04-30 20:32 UTC (permalink / raw)
  To: Jim Quinlan
  Cc: Nicolas Saenz Julienne, Lorenzo Pieralisi, Rob Herring,
	Bjorn Helgaas, Florian Fainelli,
	maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS, open list

On Thu, Apr 30, 2020 at 02:55:20PM -0400, Jim Quinlan wrote:
> From: Jim Quinlan <jquinlan@broadcom.com>
> 
> Configuration Retry Request Status is off by default on this
> PCIe controller.  Turn it on.

Are you talking about CRS itself, i.e., the ability of a Root Port to
deal with Completions with Configuration Retry Request Status?  That
really shouldn't be switchable in the hardware since it's a required
feature for all PCIe devices.

Or are you talking about CRS Software Visibility, which is controlled
by a bit in the PCIe Root Control register?  That *should* be managed
by the PCI core in pci_enable_crs().  Does that generic method of
controlling it not work for this device?

It looks like maybe the latter, since the generic:

  #define  PCI_EXP_RTCTL_CRSSVE   0x0010

matches your new PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL_RC_CRS_EN_MASK.

If pci_enable_crs() doesn't work on this device, it sounds like a
hardware defect that we need to work around, but I'm not sure that
just enabling it unconditionally here is the right thing.

> Signed-off-by: Jim Quinlan <jquinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 5b0dec5971b8..2bc913c0262c 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -34,6 +34,9 @@
>  #define BRCM_PCIE_CAP_REGS				0x00ac
>  
>  /* Broadcom STB PCIe Register Offsets */
> +#define PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL			0x00c8
> +#define  PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL_RC_CRS_EN_MASK	0x10
> +
>  #define PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1				0x0188
>  #define  PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1_ENDIAN_MODE_BAR2_MASK	0xc
>  #define  PCIE_RC_CFG_VENDOR_SPCIFIC_REG1_LITTLE_ENDIAN			0x0
> @@ -827,6 +830,12 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
>  		 pci_speed_string(pcie_link_speed[cls]), nlw,
>  		 ssc_good ? "(SSC)" : "(!SSC)");
>  
> +	/* Enable configuration request retry (CRS) */
> +	tmp = readl(base + PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL);
> +	u32p_replace_bits(&tmp, 1,
> +			  PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL_RC_CRS_EN_MASK);
> +	writel(tmp, base + PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL);
> +
>  	/* PCIe->SCB endian mode for BAR */
>  	tmp = readl(base + PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1);
>  	u32p_replace_bits(&tmp, PCIE_RC_CFG_VENDOR_SPCIFIC_REG1_LITTLE_ENDIAN,
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 5/5] PCI: brcmstb: disable L0s component of ASPM by default
  2020-04-30 18:55 ` [PATCH 5/5] PCI: brcmstb: disable L0s component of ASPM by default Jim Quinlan
  2020-04-30 19:21   ` Florian Fainelli
@ 2020-04-30 20:40   ` Bjorn Helgaas
  2020-04-30 21:17     ` Jim Quinlan
  1 sibling, 1 reply; 15+ messages in thread
From: Bjorn Helgaas @ 2020-04-30 20:40 UTC (permalink / raw)
  To: Jim Quinlan
  Cc: Nicolas Saenz Julienne, Lorenzo Pieralisi, Rob Herring,
	Bjorn Helgaas, Florian Fainelli,
	maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS, open list

On Thu, Apr 30, 2020 at 02:55:22PM -0400, Jim Quinlan wrote:
> From: Jim Quinlan <jquinlan@broadcom.com>
> 
> Some informal internal experiments has shown that the BrcmSTB ASPM L0s
> savings may introduce an undesirable noise signal on some customers'
> boards.  In addition, L0s was found lacking in realized power savings,
> especially relative to the L1 ASPM component.  This is BrcmSTB's
> experience and may not hold for others.  At any rate, we disable L0s
> savings by default unless the DT node has the 'brcm,aspm-en-l0s'
> property.

I assume this works by writing the PCIe Link Capabilities register,
which is read-only via the config space path used by the generic ASPM
code, so that code thinks the device doesn't support L0s at all.

Documentation/devicetree/bindings/pci/rockchip-pcie-host.txt includes
an "aspm-no-l0s" property.  It'd be nice if this could use the same
property.

> Signed-off-by: Jim Quinlan <jquinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 2bc913c0262c..bc1d514b19e4 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -44,6 +44,9 @@
>  #define PCIE_RC_CFG_PRIV1_ID_VAL3			0x043c
>  #define  PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK	0xffffff
>  
> +#define PCIE_RC_CFG_PRIV1_LINK_CAPABILITY			0x04dc
> +#define  PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK	0xc00
> +
>  #define PCIE_RC_DL_MDIO_ADDR				0x1100
>  #define PCIE_RC_DL_MDIO_WR_DATA				0x1104
>  #define PCIE_RC_DL_MDIO_RD_DATA				0x1108
> @@ -696,7 +699,7 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
>  	int num_out_wins = 0;
>  	u16 nlw, cls, lnksta;
>  	int i, ret;
> -	u32 tmp;
> +	u32 tmp, aspm_support;
>  
>  	/* Reset the bridge */
>  	brcm_pcie_bridge_sw_init_set(pcie, 1);
> @@ -806,6 +809,15 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
>  		num_out_wins++;
>  	}
>  
> +	/* Only support ASPM L1 unless L0s is explicitly desired */
> +	aspm_support = PCIE_LINK_STATE_L1;
> +	if (of_property_read_bool(pcie->np, "brcm,aspm-en-l0s"))
> +		aspm_support |= PCIE_LINK_STATE_L0S;
> +	tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> +	u32p_replace_bits(&tmp, aspm_support,
> +		PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK);
> +	writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> +
>  	/*
>  	 * For config space accesses on the RC, show the right class for
>  	 * a PCIe-PCIe bridge (the default setting is to be EP mode).
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/5] PCI: brcmstb: fix window register offset from 4 to 8
  2020-04-30 18:55 ` [PATCH 2/5] PCI: brcmstb: fix window register offset from 4 to 8 Jim Quinlan
  2020-04-30 19:07   ` Florian Fainelli
@ 2020-04-30 20:43   ` Bjorn Helgaas
  1 sibling, 0 replies; 15+ messages in thread
From: Bjorn Helgaas @ 2020-04-30 20:43 UTC (permalink / raw)
  To: Jim Quinlan
  Cc: Nicolas Saenz Julienne, Lorenzo Pieralisi, Rob Herring,
	Bjorn Helgaas, Florian Fainelli,
	maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS, open list

On Thu, Apr 30, 2020 at 02:55:19PM -0400, Jim Quinlan wrote:
> From: Jim Quinlan <jquinlan@broadcom.com>
> 
> The oubound memory window registers were being referenced
> with an incorrect offset.  This probably wasn't noticed
> previously as there was likely only one such outbound window.

If you repost these for any other reason:

Capitalize the first word of all the subject lines to match history.
s/oubound/outbound/

> Signed-off-by: Jim Quinlan <jquinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 454917ee9241..5b0dec5971b8 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -54,11 +54,11 @@
>  
>  #define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LO		0x400c
>  #define PCIE_MEM_WIN0_LO(win)	\
> -		PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LO + ((win) * 4)
> +		PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LO + ((win) * 8)
>  
>  #define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_HI		0x4010
>  #define PCIE_MEM_WIN0_HI(win)	\
> -		PCIE_MISC_CPU_2_PCIE_MEM_WIN0_HI + ((win) * 4)
> +		PCIE_MISC_CPU_2_PCIE_MEM_WIN0_HI + ((win) * 8)
>  
>  #define PCIE_MISC_RC_BAR1_CONFIG_LO			0x402c
>  #define  PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK		0x1f
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/5] PCI: brcmstb: enable CRS
  2020-04-30 20:32   ` Bjorn Helgaas
@ 2020-04-30 21:00     ` Jim Quinlan
  0 siblings, 0 replies; 15+ messages in thread
From: Jim Quinlan @ 2020-04-30 21:00 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Nicolas Saenz Julienne, Lorenzo Pieralisi, Rob Herring,
	Bjorn Helgaas, Florian Fainelli,
	maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS, open list

On Thu, Apr 30, 2020 at 4:32 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Thu, Apr 30, 2020 at 02:55:20PM -0400, Jim Quinlan wrote:
> > From: Jim Quinlan <jquinlan@broadcom.com>
> >
> > Configuration Retry Request Status is off by default on this
> > PCIe controller.  Turn it on.
>
> Are you talking about CRS itself, i.e., the ability of a Root Port to
> deal with Completions with Configuration Retry Request Status?  That
> really shouldn't be switchable in the hardware since it's a required
> feature for all PCIe devices.
>
> Or are you talking about CRS Software Visibility, which is controlled
> by a bit in the PCIe Root Control register?  That *should* be managed
> by the PCI core in pci_enable_crs().  Does that generic method of
> controlling it not work for this device?
>
My mistake; the commit will be dropped.

Thanks,
Jim
> It looks like maybe the latter, since the generic:
>
>   #define  PCI_EXP_RTCTL_CRSSVE   0x0010
>
> matches your new PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL_RC_CRS_EN_MASK.
>
> If pci_enable_crs() doesn't work on this device, it sounds like a
> hardware defect that we need to work around, but I'm not sure that
> just enabling it unconditionally here is the right thing.
>
> > Signed-off-by: Jim Quinlan <jquinlan@broadcom.com>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index 5b0dec5971b8..2bc913c0262c 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -34,6 +34,9 @@
> >  #define BRCM_PCIE_CAP_REGS                           0x00ac
> >
> >  /* Broadcom STB PCIe Register Offsets */
> > +#define PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL                    0x00c8
> > +#define  PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL_RC_CRS_EN_MASK    0x10
> > +
> >  #define PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1                              0x0188
> >  #define  PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1_ENDIAN_MODE_BAR2_MASK       0xc
> >  #define  PCIE_RC_CFG_VENDOR_SPCIFIC_REG1_LITTLE_ENDIAN                       0x0
> > @@ -827,6 +830,12 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
> >                pci_speed_string(pcie_link_speed[cls]), nlw,
> >                ssc_good ? "(SSC)" : "(!SSC)");
> >
> > +     /* Enable configuration request retry (CRS) */
> > +     tmp = readl(base + PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL);
> > +     u32p_replace_bits(&tmp, 1,
> > +                       PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL_RC_CRS_EN_MASK);
> > +     writel(tmp, base + PCIE_RC_CFG_PCIE_ROOT_CAP_CONTROL);
> > +
> >       /* PCIe->SCB endian mode for BAR */
> >       tmp = readl(base + PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1);
> >       u32p_replace_bits(&tmp, PCIE_RC_CFG_VENDOR_SPCIFIC_REG1_LITTLE_ENDIAN,
> > --
> > 2.17.1
> >

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 5/5] PCI: brcmstb: disable L0s component of ASPM by default
  2020-04-30 20:40   ` Bjorn Helgaas
@ 2020-04-30 21:17     ` Jim Quinlan
  0 siblings, 0 replies; 15+ messages in thread
From: Jim Quinlan @ 2020-04-30 21:17 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Nicolas Saenz Julienne, Lorenzo Pieralisi, Rob Herring,
	Bjorn Helgaas, Florian Fainelli,
	maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS, open list

On Thu, Apr 30, 2020 at 4:40 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Thu, Apr 30, 2020 at 02:55:22PM -0400, Jim Quinlan wrote:
> > From: Jim Quinlan <jquinlan@broadcom.com>
> >
> > Some informal internal experiments has shown that the BrcmSTB ASPM L0s
> > savings may introduce an undesirable noise signal on some customers'
> > boards.  In addition, L0s was found lacking in realized power savings,
> > especially relative to the L1 ASPM component.  This is BrcmSTB's
> > experience and may not hold for others.  At any rate, we disable L0s
> > savings by default unless the DT node has the 'brcm,aspm-en-l0s'
> > property.
>
> I assume this works by writing the PCIe Link Capabilities register,
> which is read-only via the config space path used by the generic ASPM
> code, so that code thinks the device doesn't support L0s at all.
Correct.
>
> Documentation/devicetree/bindings/pci/rockchip-pcie-host.txt includes
> an "aspm-no-l0s" property.  It'd be nice if this could use the same
> property.
I'd like to use the existing "aspm-no-l0s" but we'd really like to
have it disabled by default.  I'll probably switch but let me dwell on
it a little.
Thanks, Jim
>
> > Signed-off-by: Jim Quinlan <jquinlan@broadcom.com>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index 2bc913c0262c..bc1d514b19e4 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -44,6 +44,9 @@
> >  #define PCIE_RC_CFG_PRIV1_ID_VAL3                    0x043c
> >  #define  PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK   0xffffff
> >
> > +#define PCIE_RC_CFG_PRIV1_LINK_CAPABILITY                    0x04dc
> > +#define  PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK 0xc00
> > +
> >  #define PCIE_RC_DL_MDIO_ADDR                         0x1100
> >  #define PCIE_RC_DL_MDIO_WR_DATA                              0x1104
> >  #define PCIE_RC_DL_MDIO_RD_DATA                              0x1108
> > @@ -696,7 +699,7 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
> >       int num_out_wins = 0;
> >       u16 nlw, cls, lnksta;
> >       int i, ret;
> > -     u32 tmp;
> > +     u32 tmp, aspm_support;
> >
> >       /* Reset the bridge */
> >       brcm_pcie_bridge_sw_init_set(pcie, 1);
> > @@ -806,6 +809,15 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
> >               num_out_wins++;
> >       }
> >
> > +     /* Only support ASPM L1 unless L0s is explicitly desired */
> > +     aspm_support = PCIE_LINK_STATE_L1;
> > +     if (of_property_read_bool(pcie->np, "brcm,aspm-en-l0s"))
> > +             aspm_support |= PCIE_LINK_STATE_L0S;
> > +     tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> > +     u32p_replace_bits(&tmp, aspm_support,
> > +             PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK);
> > +     writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> > +
> >       /*
> >        * For config space accesses on the RC, show the right class for
> >        * a PCIe-PCIe bridge (the default setting is to be EP mode).
> > --
> > 2.17.1
> >

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2020-04-30 21:17 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-30 18:55 [PATCH 1/5] PCI: brcmstb: don't clk_put() a managed clock Jim Quinlan
2020-04-30 18:55 ` [PATCH 2/5] PCI: brcmstb: fix window register offset from 4 to 8 Jim Quinlan
2020-04-30 19:07   ` Florian Fainelli
2020-04-30 20:43   ` Bjorn Helgaas
2020-04-30 18:55 ` [PATCH 3/5] PCI: brcmstb: enable CRS Jim Quinlan
2020-04-30 19:19   ` Florian Fainelli
2020-04-30 20:32   ` Bjorn Helgaas
2020-04-30 21:00     ` Jim Quinlan
2020-04-30 18:55 ` [PATCH 4/5] dt-bindings: PCI: brcmstb: New prop 'brcm,aspm-en-l0s' Jim Quinlan
2020-04-30 19:20   ` Florian Fainelli
2020-04-30 18:55 ` [PATCH 5/5] PCI: brcmstb: disable L0s component of ASPM by default Jim Quinlan
2020-04-30 19:21   ` Florian Fainelli
2020-04-30 20:40   ` Bjorn Helgaas
2020-04-30 21:17     ` Jim Quinlan
2020-04-30 19:05 ` [PATCH 1/5] PCI: brcmstb: don't clk_put() a managed clock Florian Fainelli

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).