LinuxLists.cc - [PATCH v4 0/5] PCI: brcmstb: Configure appropriate HW CLKREQ# mode

[permalink] [raw]

Subject: [PATCH v4 2/5] PCI: brcmstb: Configure HW CLKREQ# mode appropriate for downstream device

The Broadcom STB/CM PCIe HW core, which is also used in RPi SOCs, must be
deliberately set by the RC probe() into one of three mutually exclusive
modes:

(a) No CLKREQ# expected or required, refclk is always available.
(b) CLKREQ# is expected to be driven by downstream device when needed.
(c) Bidirectional CLKREQ# for L1SS capable devices.

Previously, only (b) was supported by the driver, as almost all STB/CM
boards operate in this mode. But now there is interest in activating L1SS
power savings from STB/CM customers, and also interest in accommodating
mode (a) for designs such as the RPi CM4 with IO board.

The HW+driver is able to tell us when mode (a) or (b) is needed. All
devices should be functional using the RC-driver selected (a) or (b) mode.
For those with L1SS-capable devices that desire the power savings that come
with mode (c) we rely on the DT prop "brcm,enable-l1ss". It would be nice
to do this automatically but there is no easy way to determine this at the
time the PCI RC driver executes its probe(). Using this mode only makes
sense when the downstream device is L1SS-capable and the OS has been
configured to activate L1SS (e.g. policy==powersupersave).

The "brcm,enable-l1ss" property has already been in use by Raspian Linux,
but this implementation adds more details and discerns between (a) and (b)
automatically.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=217276
Tested-by: Florian Fainelli <[email protected]>
Signed-off-by: Jim Quinlan <[email protected]>
---
drivers/pci/controller/pcie-brcmstb.c | 69 +++++++++++++++++++++++----
1 file changed, 59 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index edf283e2b5dd..c4b076ea5180 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -48,10 +48,17 @@
#define PCIE_RC_CFG_PRIV1_LINK_CAPABILITY 0x04dc
#define PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK 0xc00

+#define PCIE_RC_CFG_PRIV1_ROOT_CAP 0x4f8
+#define PCIE_RC_CFG_PRIV1_ROOT_CAP_L1SS_MODE_MASK 0xf8
+
#define PCIE_RC_DL_MDIO_ADDR 0x1100
#define PCIE_RC_DL_MDIO_WR_DATA 0x1104
#define PCIE_RC_DL_MDIO_RD_DATA 0x1108

+#define PCIE_0_RC_PL_PHY_DBG_CLKREQ2_0 0x1e30
+#define CLKREQ2_0_CLKREQ_IN_CNT_MASK 0x3f000000
+#define CLKREQ2_0_CLKREQ_IN_MASK 0x40000000
+
#define PCIE_MISC_MISC_CTRL 0x4008
#define PCIE_MISC_MISC_CTRL_PCIE_RCB_64B_MODE_MASK 0x80
#define PCIE_MISC_MISC_CTRL_PCIE_RCB_MPS_MODE_MASK 0x400
@@ -121,9 +128,12 @@

#define PCIE_MISC_HARD_PCIE_HARD_DEBUG 0x4204
#define PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK 0x2
+#define PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABLE_MASK 0x200000
#define PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK 0x08000000
#define PCIE_BMIPS_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK 0x00800000
-
+#define PCIE_CLKREQ_MASK \
+ (PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK | \
+ PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABLE_MASK)

#define PCIE_INTR2_CPU_BASE 0x4300
#define PCIE_MSI_INTR2_BASE 0x4500
@@ -1024,13 +1034,58 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
return 0;
}

+static void brcm_config_clkreq(struct brcm_pcie *pcie)
+{
+ bool l1ss = of_property_read_bool(pcie->np, "brcm,enable-l1ss");
+ void __iomem *base = pcie->base;
+ u32 clkreq_set, tmp = readl(base + PCIE_0_RC_PL_PHY_DBG_CLKREQ2_0);
+ bool clkreq_in_seen;
+
+ /*
+ * We have "seen" CLKREQ# if it is asserted or has been in the past.
+ * Note that the CLKREQ_IN_MASK is 1 if CLKREQ# is asserted.
+ */
+ clkreq_in_seen = !!(tmp & CLKREQ2_0_CLKREQ_IN_MASK) ||
+ !!FIELD_GET(CLKREQ2_0_CLKREQ_IN_CNT_MASK, tmp);
+
+ /* Start with safest setting where we provide refclk regardless */
+ clkreq_set = readl(pcie->base + PCIE_MISC_HARD_PCIE_HARD_DEBUG) &
+ ~PCIE_CLKREQ_MASK;
+
+ if (l1ss && IS_ENABLED(CONFIG_PCIEASPM)) {
+ /*
+ * Note: For boards using a mini-card connector, this mode
+ * may not meet the TCRLon maximum time of 400ns, as
+ * specified in 3.2.5.2.5 of the PCI Express Mini CEM 2.0
+ * specification.
+ */
+ clkreq_set |= PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABLE_MASK;
+ dev_info(pcie->dev, "bi-dir CLKREQ# for L1SS power savings");
+ } else {
+ if (clkreq_in_seen && IS_ENABLED(CONFIG_PCIEASPM)) {
+ clkreq_set |= PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK;
+ dev_info(pcie->dev, "uni-dir CLKREQ# for L0s, L1 ASPM\n");
+ } else {
+ dev_info(pcie->dev, "CLKREQ# ignored; no ASPM\n");
+ /* Might as well unadvertise ASPM */
+ tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY) &
+ ~PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK;
+ writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
+ }
+ /* Setting the field to 2 unadvertises L1SS support */
+ tmp = readl(base + PCIE_RC_CFG_PRIV1_ROOT_CAP);
+ u32p_replace_bits(&tmp, 2, PCIE_RC_CFG_PRIV1_ROOT_CAP_L1SS_MODE_MASK);
+ writel(tmp, base + PCIE_RC_CFG_PRIV1_ROOT_CAP);
+ }
+ writel(clkreq_set, pcie->base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
+}
+
static int brcm_pcie_start_link(struct brcm_pcie *pcie)
{
struct device *dev = pcie->dev;
void __iomem *base = pcie->base;
u16 nlw, cls, lnksta;
bool ssc_good = false;
- u32 tmp;
int ret, i;

/* Unassert the fundamental reset */
@@ -1055,6 +1110,8 @@ static int brcm_pcie_start_link(struct brcm_pcie *pcie)
return -ENODEV;
}

+ brcm_config_clkreq(pcie);
+
if (pcie->gen)
brcm_pcie_set_gen(pcie, pcie->gen);

@@ -1073,14 +1130,6 @@ static int brcm_pcie_start_link(struct brcm_pcie *pcie)
pci_speed_string(pcie_link_speed[cls]), nlw,
ssc_good ? "(SSC)" : "(!SSC)");

- /*
- * Refclk from RC should be gated with CLKREQ# input when ASPM L0s,L1
- * is enabled => setting the CLKREQ_DEBUG_ENABLE field to 1.
- */
- tmp = readl(base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
- tmp |= PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK;
- writel(tmp, base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
-
return 0;
}

--
2.17.1

2023-04-28 22:36:19

[permalink] [raw]

Subject: [PATCH v4 1/5] dt-bindings: PCI: brcmstb: brcm,{enable-l1ss,completion-timeout-us} props

This commit introduces two new properties:

brcm,enable-l1ss (bool):

The Broadcom STB/CM PCIe HW -- a core that is also used by RPi SOCs --
requires the driver probe() to deliberately place the HW one of three
CLKREQ# modes:

(a) CLKREQ# driven by the RC unconditionally
(b) CLKREQ# driven by the EP for ASPM L0s, L1
(c) Bidirectional CLKREQ#, as used for L1 Substates (L1SS).

The HW+driver can tell the difference between downstream devices that
need (a) and (b), but does not know when to configure (c). All devices
should work fine when the driver chooses (a) or (b), but (c) may be
desired to realize the extra power savings that L1SS offers. So we
introduce the boolean "brcm,enable-l1ss" property to inform the driver
that (c) is desired. Setting this property only makes sense when the
downstream device is L1SS-capable and the OS is configured to activate
this mode (e.g. policy==superpowersave).

This property is already present in the Raspian version of Linux, but the
upstream driver implementaion that follows adds more details and discerns
between (a) and (b).

brcm,completion-timeout-us (u32):

Our HW will cause a CPU abort on any PCI transaction completion abort
error. It makes sense then to increase the timeout value for this type
of error in hopes that the response is merely delayed. Further,
L1SS-capable devices may have a long L1SS exit time and may require a
custom timeout value: we've been asked by our customers to make this
configurable for just this reason.

Signed-off-by: Jim Quinlan <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
.../devicetree/bindings/pci/brcm,stb-pcie.yaml | 16 ++++++++++++++++
1 file changed, 16 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index 7e15aae7d69e..239cc95545bd 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -64,6 +64,22 @@ properties:

aspm-no-l0s: true

+ brcm,enable-l1ss:
+ description: Indicates that PCIe L1SS power savings
+ are desired, the downstream device is L1SS-capable, and the
+ OS has been configured to enable this mode. For boards
+ using a mini-card connector, this mode may not meet the
+ TCRLon maximum time of 400ns, as specified in 3.2.5.2.5
+ of the PCI Express Mini CEM 2.0 specification.
+ type: boolean
+
+ brcm,completion-timeout-us:
+ description: Number of microseconds before PCI transaction
+ completion timeout abort is signalled.
+ minimum: 16
+ default: 1000000
+ maximum: 19884107
+
brcm,scb-sizes:
description: u64 giving the 64bit PCIe memory
viewport size of a memory controller. There may be up to
--
2.17.1

2023-04-28 22:36:33

[permalink] [raw]

Subject: [PATCH v4 3/5] PCI: brcmstb: Set PCIe transaction completion timeout

Since the STB PCIe HW will cause a CPU abort on a PCIe transaction
completion timeout abort, we might as well extend the default timeout
limit. Further, different devices and systems may requires a larger or
smaller amount commensurate with their L1SS exit time, so the property
"brcm,completion-timeout-us" may be used to set a custom timeout value.

Tested-by: Florian Fainelli <[email protected]>
Signed-off-by: Jim Quinlan <[email protected]>
---
drivers/pci/controller/pcie-brcmstb.c | 30 +++++++++++++++++++++++++++
1 file changed, 30 insertions(+)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index c4b076ea5180..c2cb683447ac 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1080,6 +1080,35 @@ static void brcm_config_clkreq(struct brcm_pcie *pcie)
writel(clkreq_set, pcie->base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
}

+static void brcm_config_completion_timeout(struct brcm_pcie *pcie)
+{
+ /* TIMEOUT register is two registers before RGR1_SW_INIT_1 */
+ const char *fmt = "brcm,completion-timeout-us clamped to region [%u..%u]\n";
+ const unsigned int REG_OFFSET = PCIE_RGR1_SW_INIT_1(pcie) - 8;
+ const u32 timeout_us_min = 16;
+ const u32 timeout_us_max = 19884107;
+ u32 timeout_us = 1000000; /* Our default, 1 second */
+ int rval, ret;
+
+ ret = of_property_read_u32(pcie->np, "brcm,completion-timeout-us",
+ &timeout_us);
+ if (ret && ret != -EINVAL)
+ dev_err(pcie->dev, "malformed/invalid 'brcm,completion-timeout-us'\n");
+
+ /* If needed, clamp the requested timeout value and issue a warning */
+ if (timeout_us < timeout_us_min) {
+ timeout_us = timeout_us_min;
+ dev_warn(pcie->dev, fmt, timeout_us_min, timeout_us_max);
+ } else if (timeout_us > timeout_us_max) {
+ timeout_us = timeout_us_max;
+ dev_warn(pcie->dev, fmt, timeout_us_min, timeout_us_max);
+ }
+
+ /* Each unit in timeout register is 1/216,000,000 seconds */
+ rval = 216 * timeout_us;
+ writel(rval, pcie->base + REG_OFFSET);
+}
+
static int brcm_pcie_start_link(struct brcm_pcie *pcie)
{
struct device *dev = pcie->dev;
@@ -1110,6 +1139,7 @@ static int brcm_pcie_start_link(struct brcm_pcie *pcie)
return -ENODEV;
}

+ brcm_config_completion_timeout(pcie);
brcm_config_clkreq(pcie);

if (pcie->gen)
--
2.17.1

2023-04-28 22:36:38

[permalink] [raw]

Subject: [PATCH v4 4/5] PCI: brcmstb: Don't assume 2711 bootloader leaves PERST# asserted

The current PCIe driver assumes PERST# is asserted when probe() is invoked.
The reasons are as follows:

(1) One Broadcom SOC (7278) causes a panic if the PERST# register is
written during this time window.

(2) If PERST# is deasserted at Linux probe() time, experience and QA
suspend/resume tests have shown that some endpoint devices fail if the
PERST# is pulsed (deasserted => asserted => deasserted) quickly in this
fashion, even though the timing is in accordance with their datasheets.

(3) Keeping things in reset tends to save power, if for some reason the
PCIe driver is not yet present.

Broadcom STB and CM SOCs bootloaders always have PERST# asserted at probe()
time. This is not necessarily the case for the 2711/RPi bootloader. In
addition, there is a failing test case [1] that may be caused by a
deasserted PERST#. Finally, Raspian version of Linux does assert PERST# at
probe() time.

So, for 2711/RPi SOCs, do what Raspian does and assert PERST#.

[1] https://lore.kernel.org/linux-pci/[email protected]/T/#m39ebab8bc2827b2304aeeff470a6c6a58f46f987

Signed-off-by: Jim Quinlan <[email protected]>
---
drivers/pci/controller/pcie-brcmstb.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index c2cb683447ac..c486f4b979cc 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -884,6 +884,11 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)

/* Reset the bridge */
pcie->bridge_sw_init_set(pcie, 1);
+
+ /* Ensure that PERST# is asserted; some bootloaders may deassert it. */
+ if (pcie->type == BCM2711)
+ pcie->perst_set(pcie, 1);
+
usleep_range(100, 200);

/* Take the bridge out of reset */
--
2.17.1

2023-04-28 22:37:34

[permalink] [raw]

Subject: [PATCH v4 5/5] PCI: brcmstb: Remove stale comment

A comment says that Multi-MSI is not supported by the driver.
A past commit [1] added this feature, so the comment is
incorrect and is removed.

[1] commit 198acab1772f22f2 ("PCI: brcmstb: Enable Multi-MSI")

Signed-off-by: Jim Quinlan <[email protected]>
---
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 c486f4b979cc..25f11f03fa09 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -449,7 +449,6 @@ static struct irq_chip brcm_msi_irq_chip = {
};

static struct msi_domain_info brcm_msi_domain_info = {
- /* Multi MSI is supported by the controller, but not by this driver */
.flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
MSI_FLAG_MULTI_PCI_MSI),
.chip = &brcm_msi_irq_chip,
--
2.17.1

2023-04-30 20:07:32

[permalink] [raw]

Subject: Re: [PATCH v4 3/5] PCI: brcmstb: Set PCIe transaction completion timeout

On Fri, Apr 28, 2023 at 06:34:57PM -0400, Jim Quinlan wrote:
> Since the STB PCIe HW will cause a CPU abort on a PCIe transaction
> completion timeout abort, we might as well extend the default timeout
> limit. Further, different devices and systems may requires a larger or
> smaller amount commensurate with their L1SS exit time, so the property
> "brcm,completion-timeout-us" may be used to set a custom timeout value.

s/requires/require/

AFAIK, other platforms do not tweak Configuration Timeout values based
on L1SS exit time. Why is brcm different?

Bjorn

2023-04-30 20:07:34

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: PCI: brcmstb: brcm,{enable-l1ss,completion-timeout-us} props

On Fri, Apr 28, 2023 at 06:34:55PM -0400, Jim Quinlan wrote:
> This commit introduces two new properties:

Doing two things makes this a candidate for splitting into two
patches, as you've already done for the driver support. They seem
incidentally related but not indivisible.

> brcm,enable-l1ss (bool):
>
> The Broadcom STB/CM PCIe HW -- a core that is also used by RPi SOCs --
> requires the driver probe() to deliberately place the HW one of three
> CLKREQ# modes:
>
> (a) CLKREQ# driven by the RC unconditionally
> (b) CLKREQ# driven by the EP for ASPM L0s, L1
> (c) Bidirectional CLKREQ#, as used for L1 Substates (L1SS).
>
> The HW+driver can tell the difference between downstream devices that
> need (a) and (b), but does not know when to configure (c). All devices
> should work fine when the driver chooses (a) or (b), but (c) may be
> desired to realize the extra power savings that L1SS offers. So we
> introduce the boolean "brcm,enable-l1ss" property to inform the driver
> that (c) is desired. Setting this property only makes sense when the
> downstream device is L1SS-capable and the OS is configured to activate
> this mode (e.g. policy==superpowersave).

Is this related to the existing generic "supports-clkreq" property? I
guess not, because supports-clkreq looks like a description of CLKREQ
signal routing, while brcm,enable-l1ss looks like a description of
what kind of downstream device is present?

What bad things would happen if the driver always configured (c)?

Other platforms don't require this, and having to edit the DT based on
what PCIe device is plugged in seems wrong. If brcmstb does need it,
that suggests a hardware defect. If we need this to work around a
defect, that's OK, but we should acknowledge the defect so we can stop
using this for future hardware that doesn't need it.

Maybe the name should be more specific to CLKREQ#, since this doesn't
actually *enable* L1SS; apparently it's just one of the pieces needed
to enable L1SS?

> This property is already present in the Raspian version of Linux, but the
> upstream driver implementaion that follows adds more details and discerns
> between (a) and (b).

s/implementaion/implementation/

> brcm,completion-timeout-us (u32):
>
> Our HW will cause a CPU abort on any PCI transaction completion abort
> error. It makes sense then to increase the timeout value for this type
> of error in hopes that the response is merely delayed. Further,
> L1SS-capable devices may have a long L1SS exit time and may require a
> custom timeout value: we've been asked by our customers to make this
> configurable for just this reason.

I asked before whether this should be made generic and not
brcm-specific, since completion timeouts are generic PCIe things. I
didn't see any discussion, but Rob reviewed this so I guess it's OK
as-is.

Is there something unique about brcm that requires this? I think it's
common for PCIe Completion Timeouts to cause CPU aborts.

Surely other drivers need to configure the completion timeout, but
pcie-rcar-host.c and pcie-rcar-ep.c are the only ones I could find.
Maybe the brcmstb power-up values are just too small? Does the
correct value need to be in DT, or could it just be built into the
driver?

This sounds like something dependent on the downstream device
connected, which again sounds hard for users to deal with. How would
they know what to use here?

> Signed-off-by: Jim Quinlan <[email protected]>
> Reviewed-by: Rob Herring <[email protected]>
> ---
> .../devicetree/bindings/pci/brcm,stb-pcie.yaml | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> index 7e15aae7d69e..239cc95545bd 100644
> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> @@ -64,6 +64,22 @@ properties:
>
> aspm-no-l0s: true
>
> + brcm,enable-l1ss:
> + description: Indicates that PCIe L1SS power savings
> + are desired, the downstream device is L1SS-capable, and the
> + OS has been configured to enable this mode. For boards
> + using a mini-card connector, this mode may not meet the
> + TCRLon maximum time of 400ns, as specified in 3.2.5.2.5
> + of the PCI Express Mini CEM 2.0 specification.
> + type: boolean
> +
> + brcm,completion-timeout-us:
> + description: Number of microseconds before PCI transaction
> + completion timeout abort is signalled.
> + minimum: 16
> + default: 1000000
> + maximum: 19884107
> +
> brcm,scb-sizes:
> description: u64 giving the 64bit PCIe memory
> viewport size of a memory controller. There may be up to
> --
> 2.17.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

2023-04-30 21:25:29

[permalink] [raw]

Subject: Re: [PATCH v4 3/5] PCI: brcmstb: Set PCIe transaction completion timeout

On Sun, Apr 30, 2023 at 3:13 PM Bjorn Helgaas <[email protected]> wrote:
>
> On Fri, Apr 28, 2023 at 06:34:57PM -0400, Jim Quinlan wrote:
> > Since the STB PCIe HW will cause a CPU abort on a PCIe transaction
> > completion timeout abort, we might as well extend the default timeout
> > limit. Further, different devices and systems may requires a larger or
> > smaller amount commensurate with their L1SS exit time, so the property
> > "brcm,completion-timeout-us" may be used to set a custom timeout value.
>
> s/requires/require/
>
> AFAIK, other platforms do not tweak Configuration Timeout values based
> on L1SS exit time. Why is brcm different?

Keep in mind that our Brcm PCIe HW signals a CPU abort on a PCIe
completion timeout. Other PCIe HW just returns 0xffffffff.

I've been maintaining this driver for over eight years or so and we've
done fine with the HW default completion timeout value.
Only recently has a major customer requested that this timeout value
be changed, and their reason was so they could
avoid a CPU abort when using L1SS.

Now we could set this value to a big number for all cases and not
require "brcm,completion-timeout-us". I cannot see any
downsides, other than another customer coming along asking us to
double the default or lessen it.

But I'm certainly willing to do that -- would that be acceptable?

Regards,
Jim

>
> Bjorn

Attachments:

2023-04-30 23:18:40

[permalink] [raw]

Subject: Re: [PATCH v4 3/5] PCI: brcmstb: Set PCIe transaction completion timeout

On Sun, Apr 30, 2023 at 05:24:26PM -0400, Jim Quinlan wrote:
> On Sun, Apr 30, 2023 at 3:13 PM Bjorn Helgaas <[email protected]> wrote:
> > On Fri, Apr 28, 2023 at 06:34:57PM -0400, Jim Quinlan wrote:
> > > Since the STB PCIe HW will cause a CPU abort on a PCIe transaction
> > > completion timeout abort, we might as well extend the default timeout
> > > limit. Further, different devices and systems may requires a larger or
> > > smaller amount commensurate with their L1SS exit time, so the property
> > > "brcm,completion-timeout-us" may be used to set a custom timeout value.
> >
> > s/requires/require/
> >
> > AFAIK, other platforms do not tweak Configuration Timeout values based
> > on L1SS exit time. Why is brcm different?
>
> Keep in mind that our Brcm PCIe HW signals a CPU abort on a PCIe
> completion timeout. Other PCIe HW just returns 0xffffffff.

Most does, but I'm pretty sure there are other controllers used on
arm64 that signal CPU aborts, e.g., imx6q_pcie_abort_handler() seems
similar.

> I've been maintaining this driver for over eight years or so and we've
> done fine with the HW default completion timeout value.
> Only recently has a major customer requested that this timeout value
> be changed, and their reason was so they could
> avoid a CPU abort when using L1SS.
>
> Now we could set this value to a big number for all cases and not
> require "brcm,completion-timeout-us". I cannot see any
> downsides, other than another customer coming along asking us to
> double the default or lessen it.
>
> But I'm certainly willing to do that -- would that be acceptable?

That would be fine with me.

Bjorn

2023-05-01 20:56:47

by Lukas Wunner

[permalink] [raw]

Subject: Re: [PATCH v4 3/5] PCI: brcmstb: Set PCIe transaction completion timeout

On Sun, Apr 30, 2023 at 05:24:26PM -0400, Jim Quinlan wrote:
> I've been maintaining this driver for over eight years or so and we've
> done fine with the HW default completion timeout value.
> Only recently has a major customer requested that this timeout value
> be changed, and their reason was so they could
> avoid a CPU abort when using L1SS.
>
> Now we could set this value to a big number for all cases and not
> require "brcm,completion-timeout-us". I cannot see any
> downsides, other than another customer coming along asking us to
> double the default or lessen it.

The Completion Timeout is configurable in the Device Control 2 Register
(PCIe r2.1 sec 7.8.16). Your controller does expose that register:

DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis- LTR+ OBFF Disabled, ARIFwd-
AtomicOpsCtl: ReqEn- EgressBlck-

Why does your controller allow configuration of the timeout in a
proprietary register in addition to DevCtl2?

If you make the Completion Timeout configurable, please do so in
a spec-compliant way, i.e. via DevCtl2, so that it works for
other products as well.

If the proprietary register has advantages over DevCtl2 (e.g.
finer granularity), perhaps you can divert accesses to the
Completion Timeout Value in DevCtl2 to your proprietary register.

Thanks,

Lukas

2023-05-02 23:18:02

by Cyril Brulebois

[permalink] [raw]

Subject: Re: [PATCH v4 0/5] PCI: brcmstb: Configure appropriate HW CLKREQ# mode

Hi,

Jim Quinlan <[email protected]> (2023-04-28):
> Note: (a) With this series, all downstream devices should work w/o DT changes.
> Only if the user desires L1SS savings and has an L1SS-capable
> device is a DT change required (brcm,enable-l1ss).

I'm still seeing some problems, but tweaking two things can lead to
massive improvements:
- setting brcm,enable-l1ss;
- upgrading the CM4's EEPROM.

Seeing how patch #4 was about the bootloader, I've prepared an updated
test image the following way:
- Kernel: 76f598ba7d8e2bfb4855b5298caedd5af0c374a8 + this series.
- Userland: Debian testing as of 2023-05-01.
- Serial console set as previously.
- Bootloader: latest release upstream, 1.20230405
(That is: bootcode.bin, *.dat, *.elf)

Then, seeing how setting brcm,enable-l1ss was helping some test cases,
I've extended testing to be quite systematic, using those components:
- CM4 IO Board (always the same).
- 1 CM4 among:
- CM4 Lite Rev 1.0 (extra test, storage much quicker to deploy)
- CM4 8/32 Rev 1.0 (as before)
- CM4 4/32 Rev 1.1 (as before)
- 1 PCIe card among:
- 006 = SupaHub PCIe->USB adapter, reference PCE6U1C-R02, VER 006
→ based on Renesas UPD720201/UPD720202
→ CONFIG_USB_XHCI_PCI_RENESAS=m
→ /lib/firmware/renesas_usb_fw.mem
- 006S = SupaHub PCIe->USB adapter, reference PCE6U1C-R02, VER 006S
→ based on Renesas UPD720201/UPD720202
→ CONFIG_USB_XHCI_PCI_RENESAS=m
→ /lib/firmware/renesas_usb_fw.mem
- VIA = Waveshare PCIe-to-multiple-USB adapter (no obvious reference)
→ based on VIA VL805/806
- 1 Kingston DataTraveler G4 32G (always the same), plugged on one of the
USB port of the PCIe card being tested.

I've tested a cold boot with each combination, first without touching the
DTB at all (pristine), then after enabling L1SS. The results are as
follows, legend is below.

+----------+----------+----------+
| 006 | 006S | VIA |
+------------------------+----------+----------+----------+
| 1. CM4 Lite Rev 1.0 | KP* | KP* | OK, 72 |
| pristine | | | |
+------------------------+----------+----------+----------+
| 2. CM4 Lite Rev 1.0 | boot + | OK, 72 | OK, 72 |
| + brcm,enable-l1ss | timeouts | | |
+------------------------+----------+----------+----------+
| 3. CM4 8/32 Rev 1.0 | KP | KP | KP |
| pristine | | | |
+------------------------+----------+----------+----------+
| 4. CM4 8/32 Rev 1.0 | OK, 69 | OK, 69 | OK, 69 |
| + brcm,enable-l1ss | | | |
+------------------------+----------+----------+----------+
| 5. CM4 4/32 Rev 1.1 | boot + | OK, 69 | OK, 69 |
| pristine | timeouts | | |
+------------------------+----------+----------+----------+
| 6. CM4 4/32 Rev 1.1 | OK, 82 | OK, 69 | OK, 69 |
| + brcm,enable-l1ss | | | |
+------------------------+----------+----------+----------+

Legend:
- OK, XXX = boots fine, memory stick visible, and reading from it using
`dd if=/dev/sda of=/dev/null bs=8M status=progress` for a few seconds
gives an XXX MB/s transfer rate.
- KP = kernel panic involving brcm_pcie_probe().
- KP* = probably the same kernel panic; unfortunately, the serial console
hardly works, but booting for 1 minute, shutting down for 10 seconds,
in a loop… ends up showing excerpts from a trace, one word or sometimes
several lines at a time. Since brcm_pcie_driver_init() or SError
appeared, getting the same trace looks probable to me. [See also the
very end of the mail.]
- boot + timeouts = the system boots, the memory stick is not visible
though, as XHCI timeouts show up, e.g.:

[ 34.144748] xhci_hcd 0000:01:00.0: Timeout while waiting for setup device command
[ 34.357273] usb 3-1.4: Device not responding to setup address.
[ 34.568429] usb 3-1.4: device not accepting address 6, error -71
[ 34.575730] usb 3-1-port4: unable to enumerate USB device

So it looks like *for these combinations* setting brcm,enable-l1ss is only
helping, even if one particular thing remains not fully fonctional (but
at least boots now): CM4 Lite Rev 1.0 + 006 card.

And since you mentioned the EEPROM topic off-list, I've investigated that
part as well. It turns out that what *seemed* (at least to my non-expert
eyes) sort of related to the hardware revisions… could have actually be
directly linked to the EEPROM version shipped with each Compute Module.

After deploying the relevant tooling, and based on the reported
timestamps, here are the relevant EEPROM filenames in the rpi-eeprom
repository (https://github.com/raspberrypi/rpi-eeprom):
- CM4 Lite Rev 1.0 [lines 1-2]
→ firmware/stable/pieeprom-2021-02-16.bin
- CM4 8/32 Rev 1.0 [lines 3-4]
→ firmware/stable/pieeprom-2021-02-16.bin
- CM4 4/32 Rev 1.1 [lines 5-6]
→ firmware/stable/pieeprom-2021-12-02.bin

Try to upgrade a first CM4 Lite to the latest version (2023-01-11) gave
solid results (which I'm not including in this report as I was only in
exploratory mode, with a slightly different Kingston DataTraveler anyway;
for reference its EEPROM dated back to 2020, and it seemed to have ever
been in some beta state…), so I decided to replicate all the tests above with
the very same 3 CM4, upgraded to 2023-01-11.

In passing: That might explain why it always felt like later revisions
were working “better” than the old ones: being designed + manufactured
later, they just ended up being shipped with a newer/better EEPROM?

Upgrade: via usbboot (https://github.com/raspberrypi/usbboot) and the
recovery procedure (which by default deploys the latest stable version).

Results with everyone at 2023-01-11.

+----------+----------+----------+
| 006 | 006S | VIA |
+------------------------+----------+----------+----------+
| 1. CM4 Lite Rev 1.0 | OK, 83 | OK, 72 | OK, 72 |
| pristine | | | |
+------------------------+----------+----------+----------+
| 2. CM4 Lite Rev 1.0 | OK, 82 | OK, 72 | OK, 72 |
| + brcm,enable-l1ss | | | |
+------------------------+----------+----------+----------+
| 3. CM4 8/32 Rev 1.0 | OK, 82 | OK, 69 | OK, 69 |
| pristine | | | |
+------------------------+----------+----------+----------+
| 4. CM4 8/32 Rev 1.0 | OK, 82 | OK, 69 | OK, 69 |
| + brcm,enable-l1ss | | | |
+------------------------+----------+----------+----------+
| 5. CM4 4/32 Rev 1.1 | OK, 82 | OK, 69 | OK, 69 |
| pristine | | | |
+------------------------+----------+----------+----------+
| 6. CM4 4/32 Rev 1.1 | OK, 82 | OK, 69 | OK, 69 |
| + brcm,enable-l1ss | | | |
+------------------------+----------+----------+----------+

Takeaways:
- Upgrading the EEPROM solved all problems;
- brcm,enable-l1ss (which used to help) is not needed, as mentioned in
your cover letter.

Now that I'm a little more familiar with the EEPROM tooling:
- It looks like I'm able to downgrade the EEPROM to an earlier version.
But I cannot guarantee I can recover exactly the previous state as
there are two different things at least: the EEPROM itself and what's
called “bootloader config” in vcgencmd). I've seen at least the LED
change behaviour (via POWER_OFF_ON_HALT).
- Upon downgrading, without brcm,enable-l1ss, the CM4 Lite is indeed
showing me a black screen/no logs in the serial console again with
either one of the 006/006S cards.
- It's possible to specify a boot config file when deploying the EEPROM,
and I've tried enabling BOOT_UART on the CM4 Lite. Now I'm getting the
kernel panic on the console!

Where should I go from here?
- Does it make sense to gather a trace for the kernel panic on say two
combinations, without brcm,enable-l1ss set:
+ CM4 Lite Rev 1.0 (old EEPROM) + 006 [first KP* in first table]
+ CM4 8/32 Rev 1.0 (old EEPROM) + 006 [first KP in first table]
then get a trace without your patches, and attach all four resulting
files?
- Or should one just consider that the very first thing that each and
every CM4 user is supposed to do is upgrade their EEPROM?

On a personal side, I'm very fine with being told to just upgrade the
EEPROM already (and that seems to cover any use case I could think of,
and test). But if getting and comparing traces before/after your patches
is helpful to you and the wider community, I'm happy to spend some more
time testing and gathering details.

Cheers,
--
Cyril Brulebois ([email protected]) <https://debamax.com/>
D-I release manager -- Release team member -- Freelance Consultant

Attachments:

(No filename) (9.15 kB)
signature.asc (849.00 B)
Download all attachments

2023-05-03 06:08:11

by Stefan Wahren

[permalink] [raw]

Subject: Re: [PATCH v4 3/5] PCI: brcmstb: Set PCIe transaction completion timeout

Hi Jim,

Am 29.04.23 um 00:34 schrieb Jim Quinlan:
> Since the STB PCIe HW will cause a CPU abort on a PCIe transaction
> completion timeout abort, we might as well extend the default timeout
> limit. Further, different devices and systems may requires a larger or
> smaller amount commensurate with their L1SS exit time, so the property
> "brcm,completion-timeout-us" may be used to set a custom timeout value.
>
> Tested-by: Florian Fainelli <[email protected]>
> Signed-off-by: Jim Quinlan <[email protected]>
> ---
> drivers/pci/controller/pcie-brcmstb.c | 30 +++++++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
>
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index c4b076ea5180..c2cb683447ac 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -1080,6 +1080,35 @@ static void brcm_config_clkreq(struct brcm_pcie *pcie)
> writel(clkreq_set, pcie->base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
> }
>
> +static void brcm_config_completion_timeout(struct brcm_pcie *pcie)
> +{
> + /* TIMEOUT register is two registers before RGR1_SW_INIT_1 */
> + const char *fmt = "brcm,completion-timeout-us clamped to region [%u..%u]\n";
> + const unsigned int REG_OFFSET = PCIE_RGR1_SW_INIT_1(pcie) - 8;
> + const u32 timeout_us_min = 16;
> + const u32 timeout_us_max = 19884107;
> + u32 timeout_us = 1000000; /* Our default, 1 second */
> + int rval, ret;
> +
> + ret = of_property_read_u32(pcie->np, "brcm,completion-timeout-us",
> + &timeout_us);
> + if (ret && ret != -EINVAL)
> + dev_err(pcie->dev, "malformed/invalid 'brcm,completion-timeout-us'\n");
> +
> + /* If needed, clamp the requested timeout value and issue a warning */
> + if (timeout_us < timeout_us_min) {
> + timeout_us = timeout_us_min;
> + dev_warn(pcie->dev, fmt, timeout_us_min, timeout_us_max);
> + } else if (timeout_us > timeout_us_max) {
> + timeout_us = timeout_us_max;
> + dev_warn(pcie->dev, fmt, timeout_us_min, timeout_us_max);
> + }
> +
> + /* Each unit in timeout register is 1/216,000,000 seconds */
> + rval = 216 * timeout_us;
> + writel(rval, pcie->base + REG_OFFSET);

i don't think "int" is the proper type for rval here

> +}
> +
> static int brcm_pcie_start_link(struct brcm_pcie *pcie)
> {
> struct device *dev = pcie->dev;
> @@ -1110,6 +1139,7 @@ static int brcm_pcie_start_link(struct brcm_pcie *pcie)
> return -ENODEV;
> }
>
> + brcm_config_completion_timeout(pcie);
> brcm_config_clkreq(pcie);
>
> if (pcie->gen)

2023-05-03 06:33:32

by Stefan Wahren

[permalink] [raw]

Subject: Re: [PATCH v4 2/5] PCI: brcmstb: Configure HW CLKREQ# mode appropriate for downstream device

Am 29.04.23 um 00:34 schrieb Jim Quinlan:
> The Broadcom STB/CM PCIe HW core, which is also used in RPi SOCs, must be
> deliberately set by the RC probe() into one of three mutually exclusive
> modes:
>
> (a) No CLKREQ# expected or required, refclk is always available.
> (b) CLKREQ# is expected to be driven by downstream device when needed.
> (c) Bidirectional CLKREQ# for L1SS capable devices.
>
> Previously, only (b) was supported by the driver, as almost all STB/CM
> boards operate in this mode. But now there is interest in activating L1SS
> power savings from STB/CM customers, and also interest in accommodating
> mode (a) for designs such as the RPi CM4 with IO board.
>
> The HW+driver is able to tell us when mode (a) or (b) is needed. All
> devices should be functional using the RC-driver selected (a) or (b) mode.
> For those with L1SS-capable devices that desire the power savings that come
> with mode (c) we rely on the DT prop "brcm,enable-l1ss". It would be nice
> to do this automatically but there is no easy way to determine this at the
> time the PCI RC driver executes its probe(). Using this mode only makes
> sense when the downstream device is L1SS-capable and the OS has been
> configured to activate L1SS (e.g. policy==powersupersave).
>
> The "brcm,enable-l1ss" property has already been in use by Raspian Linux,
> but this implementation adds more details and discerns between (a) and (b)
> automatically.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217276
> Tested-by: Florian Fainelli <[email protected]>
> Signed-off-by: Jim Quinlan <[email protected]>
> ---
> drivers/pci/controller/pcie-brcmstb.c | 69 +++++++++++++++++++++++----
> 1 file changed, 59 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index edf283e2b5dd..c4b076ea5180 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -48,10 +48,17 @@
> #define PCIE_RC_CFG_PRIV1_LINK_CAPABILITY 0x04dc
> #define PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK 0xc00
>
> +#define PCIE_RC_CFG_PRIV1_ROOT_CAP 0x4f8
> +#define PCIE_RC_CFG_PRIV1_ROOT_CAP_L1SS_MODE_MASK 0xf8
> +
> #define PCIE_RC_DL_MDIO_ADDR 0x1100
> #define PCIE_RC_DL_MDIO_WR_DATA 0x1104
> #define PCIE_RC_DL_MDIO_RD_DATA 0x1108
>
> +#define PCIE_0_RC_PL_PHY_DBG_CLKREQ2_0 0x1e30
> +#define CLKREQ2_0_CLKREQ_IN_CNT_MASK 0x3f000000
> +#define CLKREQ2_0_CLKREQ_IN_MASK 0x40000000
> +
> #define PCIE_MISC_MISC_CTRL 0x4008
> #define PCIE_MISC_MISC_CTRL_PCIE_RCB_64B_MODE_MASK 0x80
> #define PCIE_MISC_MISC_CTRL_PCIE_RCB_MPS_MODE_MASK 0x400
> @@ -121,9 +128,12 @@
>
> #define PCIE_MISC_HARD_PCIE_HARD_DEBUG 0x4204
> #define PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK 0x2
> +#define PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABLE_MASK 0x200000
> #define PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK 0x08000000
> #define PCIE_BMIPS_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK 0x00800000
> -
> +#define PCIE_CLKREQ_MASK \
> + (PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK | \
> + PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABLE_MASK)
>
> #define PCIE_INTR2_CPU_BASE 0x4300
> #define PCIE_MSI_INTR2_BASE 0x4500
> @@ -1024,13 +1034,58 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
> return 0;
> }
>
> +static void brcm_config_clkreq(struct brcm_pcie *pcie)
> +{
> + bool l1ss = of_property_read_bool(pcie->np, "brcm,enable-l1ss");
> + void __iomem *base = pcie->base;
> + u32 clkreq_set, tmp = readl(base + PCIE_0_RC_PL_PHY_DBG_CLKREQ2_0);
> + bool clkreq_in_seen;
> +
> + /*
> + * We have "seen" CLKREQ# if it is asserted or has been in the past.
> + * Note that the CLKREQ_IN_MASK is 1 if CLKREQ# is asserted.
> + */
> + clkreq_in_seen = !!(tmp & CLKREQ2_0_CLKREQ_IN_MASK) ||
> + !!FIELD_GET(CLKREQ2_0_CLKREQ_IN_CNT_MASK, tmp);
> +
> + /* Start with safest setting where we provide refclk regardless */
> + clkreq_set = readl(pcie->base + PCIE_MISC_HARD_PCIE_HARD_DEBUG) &
> + ~PCIE_CLKREQ_MASK;
> +
> + if (l1ss && IS_ENABLED(CONFIG_PCIEASPM)) {
> + /*
> + * Note: For boards using a mini-card connector, this mode
> + * may not meet the TCRLon maximum time of 400ns, as
> + * specified in 3.2.5.2.5 of the PCI Express Mini CEM 2.0
> + * specification.
> + */
> + clkreq_set |= PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABLE_MASK;
> + dev_info(pcie->dev, "bi-dir CLKREQ# for L1SS power savings");

Please append the missing newline to the log message

> + } else {
> + if (clkreq_in_seen && IS_ENABLED(CONFIG_PCIEASPM)) {
> + clkreq_set |= PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK;
> + dev_info(pcie->dev, "uni-dir CLKREQ# for L0s, L1 ASPM\n");
> + } else {
> + dev_info(pcie->dev, "CLKREQ# ignored; no ASPM\n");
> + /* Might as well unadvertise ASPM */
> + tmp = readl(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY) &
> + ~PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK;
> + writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> + }
> + /* Setting the field to 2 unadvertises L1SS support */
> + tmp = readl(base + PCIE_RC_CFG_PRIV1_ROOT_CAP);
> + u32p_replace_bits(&tmp, 2, PCIE_RC_CFG_PRIV1_ROOT_CAP_L1SS_MODE_MASK);
> + writel(tmp, base + PCIE_RC_CFG_PRIV1_ROOT_CAP);
> + }
> + writel(clkreq_set, pcie->base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
> +}
> +
> static int brcm_pcie_start_link(struct brcm_pcie *pcie)
> {
> struct device *dev = pcie->dev;
> void __iomem *base = pcie->base;
> u16 nlw, cls, lnksta;
> bool ssc_good = false;
> - u32 tmp;
> int ret, i;
>
> /* Unassert the fundamental reset */
> @@ -1055,6 +1110,8 @@ static int brcm_pcie_start_link(struct brcm_pcie *pcie)
> return -ENODEV;
> }
>
> + brcm_config_clkreq(pcie);
> +
> if (pcie->gen)
> brcm_pcie_set_gen(pcie, pcie->gen);
>
> @@ -1073,14 +1130,6 @@ static int brcm_pcie_start_link(struct brcm_pcie *pcie)
> pci_speed_string(pcie_link_speed[cls]), nlw,
> ssc_good ? "(SSC)" : "(!SSC)");
>
> - /*
> - * Refclk from RC should be gated with CLKREQ# input when ASPM L0s,L1
> - * is enabled => setting the CLKREQ_DEBUG_ENABLE field to 1.
> - */
> - tmp = readl(base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
> - tmp |= PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK;
> - writel(tmp, base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
> -
> return 0;
> }
>

2023-05-03 14:13:53

[permalink] [raw]

Subject: Re: [PATCH v4 3/5] PCI: brcmstb: Set PCIe transaction completion timeout

On Mon, May 1, 2023 at 4:55 PM Lukas Wunner <[email protected]> wrote:
>
> On Sun, Apr 30, 2023 at 05:24:26PM -0400, Jim Quinlan wrote:
> > I've been maintaining this driver for over eight years or so and we've
> > done fine with the HW default completion timeout value.
> > Only recently has a major customer requested that this timeout value
> > be changed, and their reason was so they could
> > avoid a CPU abort when using L1SS.
> >
> > Now we could set this value to a big number for all cases and not
> > require "brcm,completion-timeout-us". I cannot see any
> > downsides, other than another customer coming along asking us to
> > double the default or lessen it.
>
> The Completion Timeout is configurable in the Device Control 2 Register
> (PCIe r2.1 sec 7.8.16). Your controller does expose that register:
>
> DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis- LTR+ OBFF Disabled, ARIFwd-
> AtomicOpsCtl: ReqEn- EgressBlck-
>
> Why does your controller allow configuration of the timeout in a
> proprietary register in addition to DevCtl2?
>
> If you make the Completion Timeout configurable, please do so in
> a spec-compliant way, i.e. via DevCtl2, so that it works for
> other products as well.
>
> If the proprietary register has advantages over DevCtl2 (e.g.
> finer granularity), perhaps you can divert accesses to the
> Completion Timeout Value in DevCtl2 to your proprietary register.

Hello Lukas & Bjorn,

Yes, you are right. I was under the (mis)understanding that writing
this register is (a) necessary for long L1SS periods and (b) overrides
or updates the value in the CFG register you mentioned. It turns out
that only (a) is true.

After communicating with the HW engineer, it appears this register is
for an internal bus timeout. Further, this bus timeout can occur when
there is no PCIe access involved. However, the error appears as a
completion timeout, which I also am investigating.

At any rate, I am dropping the "brcm,completion-timeout-usec" property
completely and V5 will just set this internal timeout to a higher
default if the "brcm,enable-l1ss" is present.

Thanks & regards,
Jim

>
> Thanks,
>
> Lukas

Attachments:

2023-05-03 14:58:38

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: PCI: brcmstb: brcm,{enable-l1ss,completion-timeout-us} props

On Sun, Apr 30, 2023 at 3:10 PM Bjorn Helgaas <[email protected]> wrote:
>
> On Fri, Apr 28, 2023 at 06:34:55PM -0400, Jim Quinlan wrote:
> > This commit introduces two new properties:
>
> Doing two things makes this a candidate for splitting into two
> patches, as you've already done for the driver support. They seem
> incidentally related but not indivisible.
>
> > brcm,enable-l1ss (bool):
> >
> > The Broadcom STB/CM PCIe HW -- a core that is also used by RPi SOCs --
> > requires the driver probe() to deliberately place the HW one of three
> > CLKREQ# modes:
> >
> > (a) CLKREQ# driven by the RC unconditionally
> > (b) CLKREQ# driven by the EP for ASPM L0s, L1
> > (c) Bidirectional CLKREQ#, as used for L1 Substates (L1SS).
> >
> > The HW+driver can tell the difference between downstream devices that
> > need (a) and (b), but does not know when to configure (c). All devices
> > should work fine when the driver chooses (a) or (b), but (c) may be
> > desired to realize the extra power savings that L1SS offers. So we
> > introduce the boolean "brcm,enable-l1ss" property to inform the driver
> > that (c) is desired. Setting this property only makes sense when the
> > downstream device is L1SS-capable and the OS is configured to activate
> > this mode (e.g. policy==superpowersave).
>
> Is this related to the existing generic "supports-clkreq" property? I
> guess not, because supports-clkreq looks like a description of CLKREQ
> signal routing, while brcm,enable-l1ss looks like a description of
> what kind of downstream device is present?

It is related, I thought about using it, but not helpful for our needs. Both
cases (b) and (c) assume "supports-clkreq", and our HW needs to know
the difference between them. Further, we have a register that tells
us if the endpoint device has requested a CLKREQ#, so we already
have this information.

As an aside, I would think that the "supports-clkreq" property should be in
the port-driver or endpoint node.

>
> What bad things would happen if the driver always configured (c)?
Well, our driver has traditionally only supported (b) and our existing
boards have
been designed with this in mind. I would not want to switch modes w'o
the user/customer/engineer opting-in to do so.
Further, the PCIe HW engineer
told me defaulting to (c) was a bad idea and was "asking for trouble".
Note that the commit's comment has that warning about L1SS mode
not meeting this 400ns spec, and I suspect that many of our existing designs
have bumped into that.

But to answer your question, I haven't found a scenario that did not work
by setting mode (c). That doesn't mean they are not out there.

>
> Other platforms don't require this, and having to edit the DT based on
> what PCIe device is plugged in seems wrong. If brcmstb does need it,
> that suggests a hardware defect. If we need this to work around a
> defect, that's OK, but we should acknowledge the defect so we can stop
> using this for future hardware that doesn't need it.

All devices should work w/o the user having to change the DT. Only if they
desire L1SS must they add the "brcm,enable-l1ss" property.

Now there is this case where Cyril has found a regression, but recent
investigation
into this indicates that this particular failure was due to the RPi
CM4 using a "beta" eeprom
version -- after updating, it works fine.

>
> Maybe the name should be more specific to CLKREQ#, since this doesn't
> actually *enable* L1SS; apparently it's just one of the pieces needed
> to enable L1SS?

The other pieces are: (a) policy == POWERSUPERSAVE and (b) an L1SS-capable
device, which seem unrelated and are out of the scope of the driver.

The RPi Raspian folks have been using "brcm,enable-l1ss" for a while now and
I would prefer to keep that name for compatibility.

>
> > This property is already present in the Raspian version of Linux, but the
> > upstream driver implementaion that follows adds more details and discerns
> > between (a) and (b).
>
> s/implementaion/implementation/
>
> > brcm,completion-timeout-us (u32):
> >
> > Our HW will cause a CPU abort on any PCI transaction completion abort
> > error. It makes sense then to increase the timeout value for this type
> > of error in hopes that the response is merely delayed. Further,
> > L1SS-capable devices may have a long L1SS exit time and may require a
> > custom timeout value: we've been asked by our customers to make this
> > configurable for just this reason.
>
> I asked before whether this should be made generic and not
> brcm-specific, since completion timeouts are generic PCIe things. I
> didn't see any discussion, but Rob reviewed this so I guess it's OK
> as-is.
I am going to drop it, thanks for questioning its purpose and
I apologize for the noise.

Regards,
Jim Quinlan
Broadcom STB
>
> Is there something unique about brcm that requires this? I think it's
> common for PCIe Completion Timeouts to cause CPU aborts.
>
> Surely other drivers need to configure the completion timeout, but
> pcie-rcar-host.c and pcie-rcar-ep.c are the only ones I could find.
> Maybe the brcmstb power-up values are just too small? Does the
> correct value need to be in DT, or could it just be built into the
> driver?
>
> This sounds like something dependent on the downstream device
> connected, which again sounds hard for users to deal with. How would
> they know what to use here?
>
> > Signed-off-by: Jim Quinlan <[email protected]>
> > Reviewed-by: Rob Herring <[email protected]>
> > ---
> > .../devicetree/bindings/pci/brcm,stb-pcie.yaml | 16 ++++++++++++++++
> > 1 file changed, 16 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > index 7e15aae7d69e..239cc95545bd 100644
> > --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > @@ -64,6 +64,22 @@ properties:
> >
> > aspm-no-l0s: true
> >
> > + brcm,enable-l1ss:
> > + description: Indicates that PCIe L1SS power savings
> > + are desired, the downstream device is L1SS-capable, and the
> > + OS has been configured to enable this mode. For boards
> > + using a mini-card connector, this mode may not meet the
> > + TCRLon maximum time of 400ns, as specified in 3.2.5.2.5
> > + of the PCI Express Mini CEM 2.0 specification.
> > + type: boolean
> > +
> > + brcm,completion-timeout-us:
> > + description: Number of microseconds before PCI transaction
> > + completion timeout abort is signalled.
> > + minimum: 16
> > + default: 1000000
> > + maximum: 19884107
> > +
> > brcm,scb-sizes:
> > description: u64 giving the 64bit PCIe memory
> > viewport size of a memory controller. There may be up to
> > --
> > 2.17.1
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Attachments:

2023-05-03 18:19:46

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: PCI: brcmstb: brcm,{enable-l1ss,completion-timeout-us} props

On Wed, May 03, 2023 at 10:38:57AM -0400, Jim Quinlan wrote:
> On Sun, Apr 30, 2023 at 3:10 PM Bjorn Helgaas <[email protected]> wrote:
> > On Fri, Apr 28, 2023 at 06:34:55PM -0400, Jim Quinlan wrote:
> > > brcm,enable-l1ss (bool):
> > >
> > > The Broadcom STB/CM PCIe HW -- a core that is also used by RPi SOCs --
> > > requires the driver probe() to deliberately place the HW one of three
> > > CLKREQ# modes:
> > >
> > > (a) CLKREQ# driven by the RC unconditionally
> > > (b) CLKREQ# driven by the EP for ASPM L0s, L1
> > > (c) Bidirectional CLKREQ#, as used for L1 Substates (L1SS).
> > >
> > > The HW+driver can tell the difference between downstream devices that
> > > need (a) and (b), but does not know when to configure (c). All devices
> > > should work fine when the driver chooses (a) or (b), but (c) may be
> > > desired to realize the extra power savings that L1SS offers. So we
> > > introduce the boolean "brcm,enable-l1ss" property to inform the driver
> > > that (c) is desired. Setting this property only makes sense when the
> > > downstream device is L1SS-capable and the OS is configured to activate
> > > this mode (e.g. policy==superpowersave).
> ...

> > What bad things would happen if the driver always configured (c)?
>
> Well, our driver has traditionally only supported (b) and our
> existing boards have been designed with this in mind. I would not
> want to switch modes w'o the user/customer/engineer opting-in to do
> so. Further, the PCIe HW engineer told me defaulting to (c) was a
> bad idea and was "asking for trouble". Note that the commit's
> comment has that warning about L1SS mode not meeting this 400ns
> spec, and I suspect that many of our existing designs have bumped
> into that.
>
> But to answer your question, I haven't found a scenario that did not
> work by setting mode (c). That doesn't mean they are not out there.
>
> > Other platforms don't require this, and having to edit the DT
> > based on what PCIe device is plugged in seems wrong. If brcmstb
> > does need it, that suggests a hardware defect. If we need this to
> > work around a defect, that's OK, but we should acknowledge the
> > defect so we can stop using this for future hardware that doesn't
> > need it.
>
> All devices should work w/o the user having to change the DT. Only
> if they desire L1SS must they add the "brcm,enable-l1ss" property.

I thought the DT was supposed to describe properties of the
*hardware*, but this seems more like "use this untested clkreq
configuration," which maybe could be done via a module parameter?

Whatever the mechanism, it looks like patch 2/5 makes brcmstb
advertise the appropriate ASPM and L1SS stuff in the PCIe and L1SS
Capabilities so the OS will do the right thing without any core
changes.

> > Maybe the name should be more specific to CLKREQ#, since this
> > doesn't actually *enable* L1SS; apparently it's just one of the
> > pieces needed to enable L1SS?
>
> The other pieces are: (a) policy == POWERSUPERSAVE and (b) an
> L1SS-capable device, which seem unrelated and are out of the scope
> of the driver.

Right. Of course, if ASPM and L1SS support are advertised, the OS can
still choose whether to enable them, and that choice can change at
run-time.

> The RPi Raspian folks have been using "brcm,enable-l1ss" for a
> while now and I would prefer to keep that name for compatibility.

BTW, the DT comment in the patch refers to PCIe Mini CEM .0 sec
3.2.5.2.5. I think the correct section is 3.2.5.2.2 (at least in the
r2.1 spec).

There's also a footnote to the effect that T_CRLon is allowed to
exceed 400ns when LTR is supported and enabled. L1.2 requires LTR, so
if L1.2 is the case where brcmstb exceeds 400ns, that might not be a
problem.

Bjorn

2023-05-03 18:21:19

[permalink] [raw]

Subject: Re: [PATCH v4 0/5] PCI: brcmstb: Configure appropriate HW CLKREQ# mode

On Tue, May 2, 2023 at 7:16 PM Cyril Brulebois <[email protected]> wrote:
>
> Hi,
>
> Jim Quinlan <[email protected]> (2023-04-28):
> > Note: (a) With this series, all downstream devices should work w/o DT changes.
> > Only if the user desires L1SS savings and has an L1SS-capable
> > device is a DT change required (brcm,enable-l1ss).
>
> I'm still seeing some problems, but tweaking two things can lead to
> massive improvements:
> - setting brcm,enable-l1ss;
> - upgrading the CM4's EEPROM.
>
> Seeing how patch #4 was about the bootloader, I've prepared an updated
> test image the following way:
> - Kernel: 76f598ba7d8e2bfb4855b5298caedd5af0c374a8 + this series.
> - Userland: Debian testing as of 2023-05-01.
> - Serial console set as previously.
> - Bootloader: latest release upstream, 1.20230405
> (That is: bootcode.bin, *.dat, *.elf)
>
> Then, seeing how setting brcm,enable-l1ss was helping some test cases,
> I've extended testing to be quite systematic, using those components:
> - CM4 IO Board (always the same).
> - 1 CM4 among:
> - CM4 Lite Rev 1.0 (extra test, storage much quicker to deploy)
> - CM4 8/32 Rev 1.0 (as before)
> - CM4 4/32 Rev 1.1 (as before)
> - 1 PCIe card among:
> - 006 = SupaHub PCIe->USB adapter, reference PCE6U1C-R02, VER 006
> → based on Renesas UPD720201/UPD720202
> → CONFIG_USB_XHCI_PCI_RENESAS=m
> → /lib/firmware/renesas_usb_fw.mem
> - 006S = SupaHub PCIe->USB adapter, reference PCE6U1C-R02, VER 006S
> → based on Renesas UPD720201/UPD720202
> → CONFIG_USB_XHCI_PCI_RENESAS=m
> → /lib/firmware/renesas_usb_fw.mem
> - VIA = Waveshare PCIe-to-multiple-USB adapter (no obvious reference)
> → based on VIA VL805/806
> - 1 Kingston DataTraveler G4 32G (always the same), plugged on one of the
> USB port of the PCIe card being tested.
>
> I've tested a cold boot with each combination, first without touching the
> DTB at all (pristine), then after enabling L1SS. The results are as
> follows, legend is below.
>
> +----------+----------+----------+
> | 006 | 006S | VIA |
> +------------------------+----------+----------+----------+
> | 1. CM4 Lite Rev 1.0 | KP* | KP* | OK, 72 |
> | pristine | | | |
> +------------------------+----------+----------+----------+
> | 2. CM4 Lite Rev 1.0 | boot + | OK, 72 | OK, 72 |
> | + brcm,enable-l1ss | timeouts | | |
> +------------------------+----------+----------+----------+
> | 3. CM4 8/32 Rev 1.0 | KP | KP | KP |
> | pristine | | | |
> +------------------------+----------+----------+----------+
> | 4. CM4 8/32 Rev 1.0 | OK, 69 | OK, 69 | OK, 69 |
> | + brcm,enable-l1ss | | | |
> +------------------------+----------+----------+----------+
> | 5. CM4 4/32 Rev 1.1 | boot + | OK, 69 | OK, 69 |
> | pristine | timeouts | | |
> +------------------------+----------+----------+----------+
> | 6. CM4 4/32 Rev 1.1 | OK, 82 | OK, 69 | OK, 69 |
> | + brcm,enable-l1ss | | | |
> +------------------------+----------+----------+----------+

Hello Cyril,

I'm confused by your result table above which has a number of
failures. Further in your message you say:

Takeaways:
- Upgrading the EEPROM solved all problems;
- brcm,enable-l1ss (which used to help) is not needed [...]

May I conclude that if one uses a modern CM4 eeprom that these failures go away?
You mentioned in a personal email that at least one of your "CM4" was
running a Beta eeprom image.

I'm much less concerned about folks having problems with old or
pre-release versions of the CM4 eeprom because (a) most of these folks
are using Raspian Linux anyway and (b) they can just upgrade their
eeprom.
Further, the Rpi eeprom is closed-source and my questions on the Rpi
forum and Rpi Github have not yet led to any answers about why a
different eeprom image is changing the behavior of a clkreq signal.

Regards,
Jim Quinlan
Broadcom STB

>
> Legend:
> - OK, XXX = boots fine, memory stick visible, and reading from it using
> `dd if=/dev/sda of=/dev/null bs=8M status=progress` for a few seconds
> gives an XXX MB/s transfer rate.
> - KP = kernel panic involving brcm_pcie_probe().
> - KP* = probably the same kernel panic; unfortunately, the serial console
> hardly works, but booting for 1 minute, shutting down for 10 seconds,
> in a loop… ends up showing excerpts from a trace, one word or sometimes
> several lines at a time. Since brcm_pcie_driver_init() or SError
> appeared, getting the same trace looks probable to me. [See also the
> very end of the mail.]
> - boot + timeouts = the system boots, the memory stick is not visible
> though, as XHCI timeouts show up, e.g.:
>
> [ 34.144748] xhci_hcd 0000:01:00.0: Timeout while waiting for setup device command
> [ 34.357273] usb 3-1.4: Device not responding to setup address.
> [ 34.568429] usb 3-1.4: device not accepting address 6, error -71
> [ 34.575730] usb 3-1-port4: unable to enumerate USB device
>
> So it looks like *for these combinations* setting brcm,enable-l1ss is only
> helping, even if one particular thing remains not fully fonctional (but
> at least boots now): CM4 Lite Rev 1.0 + 006 card.
>
>
> And since you mentioned the EEPROM topic off-list, I've investigated that
> part as well. It turns out that what *seemed* (at least to my non-expert
> eyes) sort of related to the hardware revisions… could have actually be
> directly linked to the EEPROM version shipped with each Compute Module.
>
> After deploying the relevant tooling, and based on the reported
> timestamps, here are the relevant EEPROM filenames in the rpi-eeprom
> repository (https://github.com/raspberrypi/rpi-eeprom):
> - CM4 Lite Rev 1.0 [lines 1-2]
> → firmware/stable/pieeprom-2021-02-16.bin
> - CM4 8/32 Rev 1.0 [lines 3-4]
> → firmware/stable/pieeprom-2021-02-16.bin
> - CM4 4/32 Rev 1.1 [lines 5-6]
> → firmware/stable/pieeprom-2021-12-02.bin
>
> Try to upgrade a first CM4 Lite to the latest version (2023-01-11) gave
> solid results (which I'm not including in this report as I was only in
> exploratory mode, with a slightly different Kingston DataTraveler anyway;
> for reference its EEPROM dated back to 2020, and it seemed to have ever
> been in some beta state…), so I decided to replicate all the tests above with
> the very same 3 CM4, upgraded to 2023-01-11.
>
> In passing: That might explain why it always felt like later revisions
> were working “better” than the old ones: being designed + manufactured
> later, they just ended up being shipped with a newer/better EEPROM?
>
> Upgrade: via usbboot (https://github.com/raspberrypi/usbboot) and the
> recovery procedure (which by default deploys the latest stable version).
>
> Results with everyone at 2023-01-11.
>
> +----------+----------+----------+
> | 006 | 006S | VIA |
> +------------------------+----------+----------+----------+
> | 1. CM4 Lite Rev 1.0 | OK, 83 | OK, 72 | OK, 72 |
> | pristine | | | |
> +------------------------+----------+----------+----------+
> | 2. CM4 Lite Rev 1.0 | OK, 82 | OK, 72 | OK, 72 |
> | + brcm,enable-l1ss | | | |
> +------------------------+----------+----------+----------+
> | 3. CM4 8/32 Rev 1.0 | OK, 82 | OK, 69 | OK, 69 |
> | pristine | | | |
> +------------------------+----------+----------+----------+
> | 4. CM4 8/32 Rev 1.0 | OK, 82 | OK, 69 | OK, 69 |
> | + brcm,enable-l1ss | | | |
> +------------------------+----------+----------+----------+
> | 5. CM4 4/32 Rev 1.1 | OK, 82 | OK, 69 | OK, 69 |
> | pristine | | | |
> +------------------------+----------+----------+----------+
> | 6. CM4 4/32 Rev 1.1 | OK, 82 | OK, 69 | OK, 69 |
> | + brcm,enable-l1ss | | | |
> +------------------------+----------+----------+----------+
>
> Takeaways:
> - Upgrading the EEPROM solved all problems;
> - brcm,enable-l1ss (which used to help) is not needed, as mentioned in
> your cover letter.
>
> Now that I'm a little more familiar with the EEPROM tooling:
> - It looks like I'm able to downgrade the EEPROM to an earlier version.
> But I cannot guarantee I can recover exactly the previous state as
> there are two different things at least: the EEPROM itself and what's
> called “bootloader config” in vcgencmd). I've seen at least the LED
> change behaviour (via POWER_OFF_ON_HALT).
> - Upon downgrading, without brcm,enable-l1ss, the CM4 Lite is indeed
> showing me a black screen/no logs in the serial console again with
> either one of the 006/006S cards.
> - It's possible to specify a boot config file when deploying the EEPROM,
> and I've tried enabling BOOT_UART on the CM4 Lite. Now I'm getting the
> kernel panic on the console!
>
> Where should I go from here?
> - Does it make sense to gather a trace for the kernel panic on say two
> combinations, without brcm,enable-l1ss set:
> + CM4 Lite Rev 1.0 (old EEPROM) + 006 [first KP* in first table]
> + CM4 8/32 Rev 1.0 (old EEPROM) + 006 [first KP in first table]
> then get a trace without your patches, and attach all four resulting
> files?
> - Or should one just consider that the very first thing that each and
> every CM4 user is supposed to do is upgrade their EEPROM?
>
> On a personal side, I'm very fine with being told to just upgrade the
> EEPROM already (and that seems to cover any use case I could think of,
> and test). But if getting and comparing traces before/after your patches
> is helpful to you and the wider community, I'm happy to spend some more
> time testing and gathering details.
>
>
> Cheers,
> --
> Cyril Brulebois ([email protected]) <https://debamax.com/>
> D-I release manager -- Release team member -- Freelance Consultant

Attachments:

2023-05-03 19:32:27

by Cyril Brulebois

[permalink] [raw]

Subject: Re: [PATCH v4 0/5] PCI: brcmstb: Configure appropriate HW CLKREQ# mode

Hi Jim,

Jim Quinlan <[email protected]> (2023-05-03):
> > +----------+----------+----------+
> > | 006 | 006S | VIA |
> > +------------------------+----------+----------+----------+
> > | 1. CM4 Lite Rev 1.0 | KP* | KP* | OK, 72 |
> > | pristine | | | |
> > +------------------------+----------+----------+----------+
> > | 2. CM4 Lite Rev 1.0 | boot + | OK, 72 | OK, 72 |
> > | + brcm,enable-l1ss | timeouts | | |
> > +------------------------+----------+----------+----------+
> > | 3. CM4 8/32 Rev 1.0 | KP | KP | KP |
> > | pristine | | | |
> > +------------------------+----------+----------+----------+
> > | 4. CM4 8/32 Rev 1.0 | OK, 69 | OK, 69 | OK, 69 |
> > | + brcm,enable-l1ss | | | |
> > +------------------------+----------+----------+----------+
> > | 5. CM4 4/32 Rev 1.1 | boot + | OK, 69 | OK, 69 |
> > | pristine | timeouts | | |
> > +------------------------+----------+----------+----------+
> > | 6. CM4 4/32 Rev 1.1 | OK, 82 | OK, 69 | OK, 69 |
> > | + brcm,enable-l1ss | | | |
> > +------------------------+----------+----------+----------+
>
> Hello Cyril,
>
> I'm confused by your result table above which has a number of
> failures. Further in your message you say:
>
> Takeaways:
> - Upgrading the EEPROM solved all problems;
> - brcm,enable-l1ss (which used to help) is not needed [...]
>
> May I conclude that if one uses a modern CM4 eeprom that these
> failures go away?

Sorry that wasn't clear enough. The table with failures, quoted above,
was with 3 compute modules in their stock configuration:
- CM4 Lite Rev 1.0 (lines 1-2) had an 2021-02-16 EEPROM;
- CM4 8/32 Rev 1.0 (lines 3-4) had an 2021-02-16 EEPROM;
- CM4 4/32 Rev 1.1 (lines 5-6) had an 2021-12-02 EEPROM.

Upgrading them all to current 2023-01-11 led to the second table when I
tested again, where everything worked fine.

The 2 versions (2021-02-16 and 2021-12-02) are marked as stable in the
rpi-eeprom.git repository.

> You mentioned in a personal email that at least one of your "CM4" was
> running a Beta eeprom image.

That one was another CM4 Lite Rev 1.0, and had a 2020-10-02 EEPROM. That
one is marked as an old beta in the rpi-eeprom.git. (That CM4 Lite also
works very fine once the current 2023-01-11 is deployed on it.)

[Regarding EEPROM variety in the field: I've mentioned this topic on the
#debian-raspberrypi IRC channel, warning others about troubles that
might be linked to the EEPROM version. I've seen at least one CM4 user
report the 2020-10-02 beta EEPROM, and another one report a different
2022-04-26 stable EEPROM.]

> I'm much less concerned about folks having problems with old or
> pre-release versions of the CM4 eeprom because (a) most of these folks
> are using Raspian Linux anyway and (b) they can just upgrade their
> eeprom.

That looks totally fair to me. So I can stop here, wait for the next
iteration of your patch series if there's one (rechecking everything
still works fine), and only the latest EEPROM matters? Sounds good.

> Further, the Rpi eeprom is closed-source and my questions on the Rpi
> forum and Rpi Github have not yet led to any answers about why a
> different eeprom image is changing the behavior of a clkreq signal.

The following doesn't shed much light but seems consistent with results
getting better with newer EEPROM versions (a number of “PCIe” hits, some
about probing, some about resets):
https://github.com/raspberrypi/rpi-eeprom/blob/master/firmware/release-notes.md

[If I had known how much of a difference an upgraded EEPROM would make,
and how easy it is to upgrade, I would have probably bothered you much
less with all those weird results… Sorry about that.]

The whole series is:

Tested-By: Cyril Brulebois <[email protected]>

Cheers,
--
Cyril Brulebois ([email protected]) <https://debamax.com/>
D-I release manager -- Release team member -- Freelance Consultant

Attachments:

(No filename) (4.25 kB)
signature.asc (849.00 B)
Download all attachments

2023-05-03 21:58:55

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: PCI: brcmstb: brcm,{enable-l1ss,completion-timeout-us} props

On Wed, May 3, 2023 at 2:07 PM Bjorn Helgaas <[email protected]> wrote:
>
> On Wed, May 03, 2023 at 10:38:57AM -0400, Jim Quinlan wrote:
> > On Sun, Apr 30, 2023 at 3:10 PM Bjorn Helgaas <[email protected]> wrote:
> > > On Fri, Apr 28, 2023 at 06:34:55PM -0400, Jim Quinlan wrote:
> > > > brcm,enable-l1ss (bool):
> > > >
> > > > The Broadcom STB/CM PCIe HW -- a core that is also used by RPi SOCs --
> > > > requires the driver probe() to deliberately place the HW one of three
> > > > CLKREQ# modes:
> > > >
> > > > (a) CLKREQ# driven by the RC unconditionally
> > > > (b) CLKREQ# driven by the EP for ASPM L0s, L1
> > > > (c) Bidirectional CLKREQ#, as used for L1 Substates (L1SS).
> > > >
> > > > The HW+driver can tell the difference between downstream devices that
> > > > need (a) and (b), but does not know when to configure (c). All devices
> > > > should work fine when the driver chooses (a) or (b), but (c) may be
> > > > desired to realize the extra power savings that L1SS offers. So we
> > > > introduce the boolean "brcm,enable-l1ss" property to inform the driver
> > > > that (c) is desired. Setting this property only makes sense when the
> > > > downstream device is L1SS-capable and the OS is configured to activate
> > > > this mode (e.g. policy==superpowersave).
> > ...
>
> > > What bad things would happen if the driver always configured (c)?
> >
> > Well, our driver has traditionally only supported (b) and our
> > existing boards have been designed with this in mind. I would not
> > want to switch modes w'o the user/customer/engineer opting-in to do
> > so. Further, the PCIe HW engineer told me defaulting to (c) was a
> > bad idea and was "asking for trouble". Note that the commit's
> > comment has that warning about L1SS mode not meeting this 400ns
> > spec, and I suspect that many of our existing designs have bumped
> > into that.
> >
> > But to answer your question, I haven't found a scenario that did not
> > work by setting mode (c). That doesn't mean they are not out there.
> >
> > > Other platforms don't require this, and having to edit the DT
> > > based on what PCIe device is plugged in seems wrong. If brcmstb
> > > does need it, that suggests a hardware defect. If we need this to
> > > work around a defect, that's OK, but we should acknowledge the
> > > defect so we can stop using this for future hardware that doesn't
> > > need it.
> >
> > All devices should work w/o the user having to change the DT. Only
> > if they desire L1SS must they add the "brcm,enable-l1ss" property.
>
> I thought the DT was supposed to describe properties of the
> *hardware*, but this seems more like "use this untested clkreq
> configuration," which maybe could be done via a module parameter?
Electrically, it has been tested, but specifically for L1SS capable
devices. What is untested AFAICT are platforms using this mode on
non-L1SS capable
devices. I was not aware that Raspian OS was turning this on as a
default until the CM4 came out.

As far as "DT describing the HW only", one doesn't have to go far to
find exceptions to the rule.
One example off the top of my head is "linux,pci-domain" -- all this
does is assign
an "id" to a controller to make life easier. We've gone from not
using it, with three controllers no less,
to using it, but the HW was the same all along.

WRT bootline param
pci=[<domain>:]<bus>:<dev>.<func>[/<dev>.<func>]*pci:<vendor>:<device>[:<subvendor>:<subdevice>]:
this does not look compatible for vendor specific DT options like
"brcm,enable-l1ss". I observe that pci_dev_str_match_path() is a
static function and I don't see a single option in pci.c that is
vendor specific. FWIW, moving something like this to the bootline
would not be popular with our customers; for some reason they really
don't like changes to the bootline.

>
> Whatever the mechanism, it looks like patch 2/5 makes brcmstb
> advertise the appropriate ASPM and L1SS stuff in the PCIe and L1SS
> Capabilities so the OS will do the right thing without any core
> changes.
>
> > > Maybe the name should be more specific to CLKREQ#, since this
> > > doesn't actually *enable* L1SS; apparently it's just one of the
> > > pieces needed to enable L1SS?
> >
> > The other pieces are: (a) policy == POWERSUPERSAVE and (b) an
> > L1SS-capable device, which seem unrelated and are out of the scope
> > of the driver.
>
> Right. Of course, if ASPM and L1SS support are advertised, the OS can
> still choose whether to enable them, and that choice can change at
> run-time.
Agree.

Thanks & regards,
Jim Quinlan
Broadcom STB
>
> > The RPi Raspian folks have been using "brcm,enable-l1ss" for a
> > while now and I would prefer to keep that name for compatibility.
>
> BTW, the DT comment in the patch refers to PCIe Mini CEM .0 sec
> 3.2.5.2.5. I think the correct section is 3.2.5.2.2 (at least in the
> r2.1 spec).
>
> There's also a footnote to the effect that T_CRLon is allowed to
> exceed 400ns when LTR is supported and enabled. L1.2 requires LTR, so
> if L1.2 is the case where brcmstb exceeds 400ns, that might not be a
> problem.
>
> Bjorn

Attachments:

2023-05-03 22:26:16

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: PCI: brcmstb: brcm,{enable-l1ss,completion-timeout-us} props

On Wed, May 03, 2023 at 05:38:15PM -0400, Jim Quinlan wrote:
> On Wed, May 3, 2023 at 2:07 PM Bjorn Helgaas <[email protected]> wrote:
> > On Wed, May 03, 2023 at 10:38:57AM -0400, Jim Quinlan wrote:
> > > On Sun, Apr 30, 2023 at 3:10 PM Bjorn Helgaas <[email protected]> wrote:
> > > > On Fri, Apr 28, 2023 at 06:34:55PM -0400, Jim Quinlan wrote:
> > > > > brcm,enable-l1ss (bool):
> > > > >
> > > > > The Broadcom STB/CM PCIe HW -- a core that is also used by RPi SOCs --
> > > > > requires the driver probe() to deliberately place the HW one of three
> > > > > CLKREQ# modes:
> > > > >
> > > > > (a) CLKREQ# driven by the RC unconditionally
> > > > > (b) CLKREQ# driven by the EP for ASPM L0s, L1
> > > > > (c) Bidirectional CLKREQ#, as used for L1 Substates (L1SS).
> > > > >
> > > > > The HW+driver can tell the difference between downstream devices that
> > > > > need (a) and (b), but does not know when to configure (c). All devices
> > > > > should work fine when the driver chooses (a) or (b), but (c) may be
> > > > > desired to realize the extra power savings that L1SS offers. So we
> > > > > introduce the boolean "brcm,enable-l1ss" property to inform the driver
> > > > > that (c) is desired. Setting this property only makes sense when the
> > > > > downstream device is L1SS-capable and the OS is configured to activate
> > > > > this mode (e.g. policy==superpowersave).
> > > ...
> >
> > > > What bad things would happen if the driver always configured (c)?
> > >
> > > Well, our driver has traditionally only supported (b) and our
> > > existing boards have been designed with this in mind. I would not
> > > want to switch modes w'o the user/customer/engineer opting-in to do
> > > so. Further, the PCIe HW engineer told me defaulting to (c) was a
> > > bad idea and was "asking for trouble". Note that the commit's
> > > comment has that warning about L1SS mode not meeting this 400ns
> > > spec, and I suspect that many of our existing designs have bumped
> > > into that.
> > >
> > > But to answer your question, I haven't found a scenario that did not
> > > work by setting mode (c). That doesn't mean they are not out there.
> > >
> > > > Other platforms don't require this, and having to edit the DT
> > > > based on what PCIe device is plugged in seems wrong. If brcmstb
> > > > does need it, that suggests a hardware defect. If we need this to
> > > > work around a defect, that's OK, but we should acknowledge the
> > > > defect so we can stop using this for future hardware that doesn't
> > > > need it.
> > >
> > > All devices should work w/o the user having to change the DT. Only
> > > if they desire L1SS must they add the "brcm,enable-l1ss" property.
> >
> > I thought the DT was supposed to describe properties of the
> > *hardware*, but this seems more like "use this untested clkreq
> > configuration," which maybe could be done via a module parameter?
>
> Electrically, it has been tested, but specifically for L1SS capable
> devices. What is untested AFAICT are platforms using this mode on
> non-L1SS capable devices.

Non-L1SS behavior is a subset of L1SS, so if you've tested with L1SS
enabled, I would think you'd be covered.

But I'm not a hardware engineer, so maybe there's some subtlety there.
The "asking for trouble" comment from your engineer is definitely
concerning, but I have no idea what's behind that.

And obviously even if we have "brcm,enable-l1ss", the user may decide
to disable L1SS administratively, so even if the Root Port and the
device both support L1SS, it may be never be enabled.

> WRT bootline param
> pci=[<domain>:]<bus>:<dev>.<func>[/<dev>.<func>]*pci:<vendor>:<device>[:<subvendor>:<subdevice>]:
> this does not look compatible for vendor specific DT options like
> "brcm,enable-l1ss". I observe that pci_dev_str_match_path() is a
> static function and I don't see a single option in pci.c that is
> vendor specific. FWIW, moving something like this to the bootline
> would not be popular with our customers; for some reason they really
> don't like changes to the bootline.

They prefer editing the DT?

I agree the "pci=B:D.F" stuff is a bit ugly. Do you have multiple
slots such that you would have to apply this parameter to some but not
others? I guess I was imagining a single-slot system where you
wouldn't need to identify the specific device because there *is* only
one.

Bjorn

2023-05-05 12:53:58

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: PCI: brcmstb: brcm,{enable-l1ss,completion-timeout-us} props

On Wed, May 3, 2023 at 6:18 PM Bjorn Helgaas <[email protected]> wrote:
>
> On Wed, May 03, 2023 at 05:38:15PM -0400, Jim Quinlan wrote:
> > On Wed, May 3, 2023 at 2:07 PM Bjorn Helgaas <[email protected]> wrote:
> > > On Wed, May 03, 2023 at 10:38:57AM -0400, Jim Quinlan wrote:
> > > > On Sun, Apr 30, 2023 at 3:10 PM Bjorn Helgaas <[email protected]> wrote:
> > > > > On Fri, Apr 28, 2023 at 06:34:55PM -0400, Jim Quinlan wrote:
> > > > > > brcm,enable-l1ss (bool):
> > > > > >
> > > > > > The Broadcom STB/CM PCIe HW -- a core that is also used by RPi SOCs --
> > > > > > requires the driver probe() to deliberately place the HW one of three
> > > > > > CLKREQ# modes:
> > > > > >
> > > > > > (a) CLKREQ# driven by the RC unconditionally
> > > > > > (b) CLKREQ# driven by the EP for ASPM L0s, L1
> > > > > > (c) Bidirectional CLKREQ#, as used for L1 Substates (L1SS).
> > > > > >
> > > > > > The HW+driver can tell the difference between downstream devices that
> > > > > > need (a) and (b), but does not know when to configure (c). All devices
> > > > > > should work fine when the driver chooses (a) or (b), but (c) may be
> > > > > > desired to realize the extra power savings that L1SS offers. So we
> > > > > > introduce the boolean "brcm,enable-l1ss" property to inform the driver
> > > > > > that (c) is desired. Setting this property only makes sense when the
> > > > > > downstream device is L1SS-capable and the OS is configured to activate
> > > > > > this mode (e.g. policy==superpowersave).
> > > > ...
> > >
> > > > > What bad things would happen if the driver always configured (c)?
> > > >
> > > > Well, our driver has traditionally only supported (b) and our
> > > > existing boards have been designed with this in mind. I would not
> > > > want to switch modes w'o the user/customer/engineer opting-in to do
> > > > so. Further, the PCIe HW engineer told me defaulting to (c) was a
> > > > bad idea and was "asking for trouble". Note that the commit's
> > > > comment has that warning about L1SS mode not meeting this 400ns
> > > > spec, and I suspect that many of our existing designs have bumped
> > > > into that.
> > > >
> > > > But to answer your question, I haven't found a scenario that did not
> > > > work by setting mode (c). That doesn't mean they are not out there.
> > > >
> > > > > Other platforms don't require this, and having to edit the DT
> > > > > based on what PCIe device is plugged in seems wrong. If brcmstb
> > > > > does need it, that suggests a hardware defect. If we need this to
> > > > > work around a defect, that's OK, but we should acknowledge the
> > > > > defect so we can stop using this for future hardware that doesn't
> > > > > need it.
> > > >
> > > > All devices should work w/o the user having to change the DT. Only
> > > > if they desire L1SS must they add the "brcm,enable-l1ss" property.
> > >
> > > I thought the DT was supposed to describe properties of the
> > > *hardware*, but this seems more like "use this untested clkreq
> > > configuration," which maybe could be done via a module parameter?
> >
> > Electrically, it has been tested, but specifically for L1SS capable
> > devices. What is untested AFAICT are platforms using this mode on
> > non-L1SS capable devices.
>
> Non-L1SS behavior is a subset of L1SS, so if you've tested with L1SS
> enabled, I would think you'd be covered.
>
> But I'm not a hardware engineer, so maybe there's some subtlety there.
> The "asking for trouble" comment from your engineer is definitely
> concerning, but I have no idea what's behind that.
>
> And obviously even if we have "brcm,enable-l1ss", the user may decide
> to disable L1SS administratively, so even if the Root Port and the
> device both support L1SS, it may be never be enabled.
>
> > WRT bootline param
> > pci=[<domain>:]<bus>:<dev>.<func>[/<dev>.<func>]*pci:<vendor>:<device>[:<subvendor>:<subdevice>]:
> > this does not look compatible for vendor specific DT options like
> > "brcm,enable-l1ss". I observe that pci_dev_str_match_path() is a
> > static function and I don't see a single option in pci.c that is
> > vendor specific. FWIW, moving something like this to the bootline
> > would not be popular with our customers; for some reason they really
> > don't like changes to the bootline.
>
> They prefer editing the DT?
>
> I agree the "pci=B:D.F" stuff is a bit ugly. Do you have multiple
> slots such that you would have to apply this parameter to some but not
> others? I guess I was imagining a single-slot system where you
> wouldn't need to identify the specific device because there *is* only
> one.
Hi Bjorn,

We typically have a single device per controller. Occasionally, there
is a mismatch in needs, and the customer adds a switch to their board
until we can add another controller to the next rev of the SOC.

Some of our customers have a habit of doing "rmmod, sleep, insmod" on
the RC driver for various uncommon reasons, so "pci,linux-domain"
was quite useful for them to simplify their shell scripts.

As far as preferring DT: customers have to modify the DT already*, so
they really don't want to be modifying two separate configurations (DT
and boot params). Often, the DT blob is stored in a different
partition or medium than the bootline params, and it is a hassle to
configure both and keep them in "sync".

Regards,
Jim Quinlan
Broadcom STB

* We have a tool system that we and our customers use which takes a
high-level configuration file and generates a custom DT blob and
bootloader for a particular SOC/board(s). And we provide the default
config, so our customers only have to change a few things. For
example, adding "-l1ss" to the existing "pcie -n 0" line will do what
you'd expect. And this is actually not a good example of the tool's
power.

>
> Bjorn

Attachments:

2023-05-05 14:10:21

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: PCI: brcmstb: brcm,{enable-l1ss,completion-timeout-us} props

On Fri, May 05, 2023 at 08:39:52AM -0400, Jim Quinlan wrote:
> On Wed, May 3, 2023 at 6:18 PM Bjorn Helgaas <[email protected]> wrote:
> > On Wed, May 03, 2023 at 05:38:15PM -0400, Jim Quinlan wrote:
> > > On Wed, May 3, 2023 at 2:07 PM Bjorn Helgaas <[email protected]> wrote:
> > > > On Wed, May 03, 2023 at 10:38:57AM -0400, Jim Quinlan wrote:
> > > > > On Sun, Apr 30, 2023 at 3:10 PM Bjorn Helgaas <[email protected]> wrote:
> > > > > > On Fri, Apr 28, 2023 at 06:34:55PM -0400, Jim Quinlan wrote:
> > > > > > > brcm,enable-l1ss (bool):
> > > > > > >
> > > > > > > The Broadcom STB/CM PCIe HW -- a core that is also used by RPi SOCs --
> > > > > > > requires the driver probe() to deliberately place the HW one of three
> > > > > > > CLKREQ# modes:
> > > > > > >
> > > > > > > (a) CLKREQ# driven by the RC unconditionally
> > > > > > > (b) CLKREQ# driven by the EP for ASPM L0s, L1
> > > > > > > (c) Bidirectional CLKREQ#, as used for L1 Substates (L1SS).
> > > > > > >
> > > > > > > The HW+driver can tell the difference between downstream devices that
> > > > > > > need (a) and (b), but does not know when to configure (c). All devices
> > > > > > > should work fine when the driver chooses (a) or (b), but (c) may be
> > > > > > > desired to realize the extra power savings that L1SS offers. So we
> > > > > > > introduce the boolean "brcm,enable-l1ss" property to inform the driver
> > > > > > > that (c) is desired. Setting this property only makes sense when the
> > > > > > > downstream device is L1SS-capable and the OS is configured to activate
> > > > > > > this mode (e.g. policy==superpowersave).

Just noticed that this should be "policy==powersupersave"

> > > > > > What bad things would happen if the driver always configured (c)?
> > > > >
> > > > > Well, our driver has traditionally only supported (b) and our
> > > > > existing boards have been designed with this in mind. I would not
> > > > > want to switch modes w'o the user/customer/engineer opting-in to do
> > > > > so. Further, the PCIe HW engineer told me defaulting to (c) was a
> > > > > bad idea and was "asking for trouble". Note that the commit's
> > > > > comment has that warning about L1SS mode not meeting this 400ns
> > > > > spec, and I suspect that many of our existing designs have bumped
> > > > > into that.
> > > > >
> > > > > But to answer your question, I haven't found a scenario that did not
> > > > > work by setting mode (c). That doesn't mean they are not out there.
> > > > >
> > > > > > Other platforms don't require this, and having to edit the DT
> > > > > > based on what PCIe device is plugged in seems wrong. If brcmstb
> > > > > > does need it, that suggests a hardware defect. If we need this to
> > > > > > work around a defect, that's OK, but we should acknowledge the
> > > > > > defect so we can stop using this for future hardware that doesn't
> > > > > > need it.
> > > > >
> > > > > All devices should work w/o the user having to change the DT. Only
> > > > > if they desire L1SS must they add the "brcm,enable-l1ss" property.
> > > >
> > > > I thought the DT was supposed to describe properties of the
> > > > *hardware*, but this seems more like "use this untested clkreq
> > > > configuration," which maybe could be done via a module parameter?
> > >
> > > Electrically, it has been tested, but specifically for L1SS capable
> > > devices. What is untested AFAICT are platforms using this mode on
> > > non-L1SS capable devices.
> >
> > Non-L1SS behavior is a subset of L1SS, so if you've tested with L1SS
> > enabled, I would think you'd be covered.

I think this point is still worth considering. Maybe your hardware
folks have an opinion here?

> > But I'm not a hardware engineer, so maybe there's some subtlety there.
> > The "asking for trouble" comment from your engineer is definitely
> > concerning, but I have no idea what's behind that.
> >
> > And obviously even if we have "brcm,enable-l1ss", the user may decide
> > to disable L1SS administratively, so even if the Root Port and the
> > device both support L1SS, it may be never be enabled.
> >
> > > WRT bootline param
> > > pci=[<domain>:]<bus>:<dev>.<func>[/<dev>.<func>]*pci:<vendor>:<device>[:<subvendor>:<subdevice>]:
> > > this does not look compatible for vendor specific DT options like
> > > "brcm,enable-l1ss". I observe that pci_dev_str_match_path() is a
> > > static function and I don't see a single option in pci.c that is
> > > vendor specific. FWIW, moving something like this to the bootline
> > > would not be popular with our customers; for some reason they really
> > > don't like changes to the bootline.
> >
> > They prefer editing the DT?
> >
> > I agree the "pci=B:D.F" stuff is a bit ugly. Do you have multiple
> > slots such that you would have to apply this parameter to some but not
> > others? I guess I was imagining a single-slot system where you
> > wouldn't need to identify the specific device because there *is* only
> > one.
>
> We typically have a single device per controller. Occasionally,
> there is a mismatch in needs, and the customer adds a switch to
> their board until we can add another controller to the next rev of
> the SOC.

If you add a switch, it sounds like there's still only a single link
between the brcmstb controller and the switch. I'm assuming
"brcm,enable-l1ss" only affects CLKREQ# on that link and it has
nothing to do with links below the switch.

(c) must be the standard PCIe situation because no other systems
require the user to configure CLKREQ# based on the type of card
plugged in. And we don't know about any actual problems that happen
in (c) with any cards.

That makes me think the ideal end state would be to use (c) by
default so everything just works like every other platform with no
fuss. If there's some situation that requires (a) or (b), there could
be a property or parameter to select *that* because that would be the
unusual case.

But obviously the comment from the hardware engineer:

> > > > > Further, the PCIe HW engineer told me defaulting to (c) was
> > > > > a bad idea and was "asking for trouble".

would need to be understood before doing that.

Bjorn

2023-05-05 14:55:01

[permalink] [raw]

Subject: Re: [PATCH v4 1/5] dt-bindings: PCI: brcmstb: brcm,{enable-l1ss,completion-timeout-us} props

On Fri, May 5, 2023 at 9:34 AM Bjorn Helgaas <[email protected]> wrote:
>
> On Fri, May 05, 2023 at 08:39:52AM -0400, Jim Quinlan wrote:
> > On Wed, May 3, 2023 at 6:18 PM Bjorn Helgaas <[email protected]> wrote:
> > > On Wed, May 03, 2023 at 05:38:15PM -0400, Jim Quinlan wrote:
> > > > On Wed, May 3, 2023 at 2:07 PM Bjorn Helgaas <[email protected]> wrote:
> > > > > On Wed, May 03, 2023 at 10:38:57AM -0400, Jim Quinlan wrote:
> > > > > > On Sun, Apr 30, 2023 at 3:10 PM Bjorn Helgaas <[email protected]> wrote:
> > > > > > > On Fri, Apr 28, 2023 at 06:34:55PM -0400, Jim Quinlan wrote:
> > > > > > > > brcm,enable-l1ss (bool):
> > > > > > > >
> > > > > > > > The Broadcom STB/CM PCIe HW -- a core that is also used by RPi SOCs --
> > > > > > > > requires the driver probe() to deliberately place the HW one of three
> > > > > > > > CLKREQ# modes:
> > > > > > > >
> > > > > > > > (a) CLKREQ# driven by the RC unconditionally
> > > > > > > > (b) CLKREQ# driven by the EP for ASPM L0s, L1
> > > > > > > > (c) Bidirectional CLKREQ#, as used for L1 Substates (L1SS).
> > > > > > > >
> > > > > > > > The HW+driver can tell the difference between downstream devices that
> > > > > > > > need (a) and (b), but does not know when to configure (c). All devices
> > > > > > > > should work fine when the driver chooses (a) or (b), but (c) may be
> > > > > > > > desired to realize the extra power savings that L1SS offers. So we
> > > > > > > > introduce the boolean "brcm,enable-l1ss" property to inform the driver
> > > > > > > > that (c) is desired. Setting this property only makes sense when the
> > > > > > > > downstream device is L1SS-capable and the OS is configured to activate
> > > > > > > > this mode (e.g. policy==superpowersave).
>
> Just noticed that this should be "policy==powersupersave"
>
> > > > > > > What bad things would happen if the driver always configured (c)?
> > > > > >
> > > > > > Well, our driver has traditionally only supported (b) and our
> > > > > > existing boards have been designed with this in mind. I would not
> > > > > > want to switch modes w'o the user/customer/engineer opting-in to do
> > > > > > so. Further, the PCIe HW engineer told me defaulting to (c) was a
> > > > > > bad idea and was "asking for trouble". Note that the commit's
> > > > > > comment has that warning about L1SS mode not meeting this 400ns
> > > > > > spec, and I suspect that many of our existing designs have bumped
> > > > > > into that.
> > > > > >
> > > > > > But to answer your question, I haven't found a scenario that did not
> > > > > > work by setting mode (c). That doesn't mean they are not out there.
> > > > > >
> > > > > > > Other platforms don't require this, and having to edit the DT
> > > > > > > based on what PCIe device is plugged in seems wrong. If brcmstb
> > > > > > > does need it, that suggests a hardware defect. If we need this to
> > > > > > > work around a defect, that's OK, but we should acknowledge the
> > > > > > > defect so we can stop using this for future hardware that doesn't
> > > > > > > need it.
> > > > > >
> > > > > > All devices should work w/o the user having to change the DT. Only
> > > > > > if they desire L1SS must they add the "brcm,enable-l1ss" property.
> > > > >
> > > > > I thought the DT was supposed to describe properties of the
> > > > > *hardware*, but this seems more like "use this untested clkreq
> > > > > configuration," which maybe could be done via a module parameter?
> > > >
> > > > Electrically, it has been tested, but specifically for L1SS capable
> > > > devices. What is untested AFAICT are platforms using this mode on
> > > > non-L1SS capable devices.
> > >
> > > Non-L1SS behavior is a subset of L1SS, so if you've tested with L1SS
> > > enabled, I would think you'd be covered.
>
> I think this point is still worth considering. Maybe your hardware
> folks have an opinion here?
See below.
>
> > > But I'm not a hardware engineer, so maybe there's some subtlety there.
> > > The "asking for trouble" comment from your engineer is definitely
> > > concerning, but I have no idea what's behind that.
> > >
> > > And obviously even if we have "brcm,enable-l1ss", the user may decide
> > > to disable L1SS administratively, so even if the Root Port and the
> > > device both support L1SS, it may be never be enabled.
> > >
> > > > WRT bootline param
> > > > pci=[<domain>:]<bus>:<dev>.<func>[/<dev>.<func>]*pci:<vendor>:<device>[:<subvendor>:<subdevice>]:
> > > > this does not look compatible for vendor specific DT options like
> > > > "brcm,enable-l1ss". I observe that pci_dev_str_match_path() is a
> > > > static function and I don't see a single option in pci.c that is
> > > > vendor specific. FWIW, moving something like this to the bootline
> > > > would not be popular with our customers; for some reason they really
> > > > don't like changes to the bootline.
> > >
> > > They prefer editing the DT?
> > >
> > > I agree the "pci=B:D.F" stuff is a bit ugly. Do you have multiple
> > > slots such that you would have to apply this parameter to some but not
> > > others? I guess I was imagining a single-slot system where you
> > > wouldn't need to identify the specific device because there *is* only
> > > one.
> >
> > We typically have a single device per controller. Occasionally,
> > there is a mismatch in needs, and the customer adds a switch to
> > their board until we can add another controller to the next rev of
> > the SOC.
>
> If you add a switch, it sounds like there's still only a single link
> between the brcmstb controller and the switch. I'm assuming
> "brcm,enable-l1ss" only affects CLKREQ# on that link and it has
> nothing to do with links below the switch.
>
> (c) must be the standard PCIe situation because no other systems
> require the user to configure CLKREQ# based on the type of card
> plugged in. And we don't know about any actual problems that happen
> in (c) with any cards.
>
> That makes me think the ideal end state would be to use (c) by
> default so everything just works like every other platform with no
> fuss. If there's some situation that requires (a) or (b), there could
> be a property or parameter to select *that* because that would be the
> unusual case.
>
> But obviously the comment from the hardware engineer:
>
> > > > > > Further, the PCIe HW engineer told me defaulting to (c) was
> > > > > > a bad idea and was "asking for trouble".
>
> would need to be understood before doing that.

Keep in mind that our controller is already unusual in that it
requires this manual mode setting whereas
other controllers don't seem to have this issue. As far as discussing
this with the HW person, either I am not understanding the reason(s)
or he is not explaining them well. We've tried a couple of times,
FWIW. At any rate, one thing he has repeated with emphasis is that
only l1ss capable devices should be using our l1ss mode.
For me, this feedback trumps all other choices.

Finally, experience has made me quite wary of silently changing a
default for all of our STB/CM existing customers, regardless of the
fact that the CM4 Raspian folks have been using l1ss-mode (most likely
as a workaround to boot with cards lacking a functional CLKREQ# pin).

Regards.
Jim Quinlan
Broadcom STB

>
> Bjorn

Attachments:

2023-05-05 14:57:37