[PATCH v7 0/3] PCI: brcmstb: Configure appropriate HW CLKREQ# mode

[PATCH v7 0/3] PCI: brcmstb: Configure appropriate HW CLKREQ# mode

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 3844 bytes --]

v7 -- Manivannan Sadhasivam suggested (a) making the property look like a
      network phy-mode and (b) keeping the code simple (not counting clkreq
      signal appearances, un-advertising capabilites, etc).  This is
      what I have done.  The property is now "brcm,clkreq-mode" and
      the values may be one of "safe", "default", and "no-l1ss".  The
      default setting is to employ the most capable power savings mode.

v6 -- No code has been changed.
   -- Changed commit subject and comment in "#PERST" commit (Bjorn, Cyril)
   -- Changed sign-off and author email address for all commits.
      This was due to a change in Broadcom's upstreaming policy.

v5 -- Remove DT property "brcm,completion-timeout-us" from	 
      "DT bindings" commit.  Although this error may be reported	 
      as a completion timeout, its cause was traced to an	 
      internal bus timeout which may occur even when there is	 
      no PCIe access being processed.  We set a timeout of four	 
      seconds only if we are operating in "L1SS CLKREQ#" mode.
   -- Correct CEM 2.0 reference provided by HW engineer,
      s/3.2.5.2.5/3.2.5.2.2/ (Bjorn)
   -- Add newline to dev_info() string (Stefan)
   -- Change variable rval to unsigned (Stefan)
   -- s/implementaion/implementation/ (Bjorn)
   -- s/superpowersave/powersupersave/ (Bjorn)
   -- Slightly modify message on "PERST#" commit.
   -- Rebase to torvalds master

v4 -- New commit that asserts PERST# for 2711/RPi SOCs at PCIe RC
      driver probe() time.  This is done in Raspian Linux and its
      absence may be the cause of a failing test case.
   -- New commit that removes stale comment.

v3 -- Rewrote commit msgs and comments refering panics if L1SS
      is enabled/disabled; the code snippet that unadvertises L1SS
      eliminates the panic scenario. (Bjorn)
   -- Add reference for "400ns of CLKREQ# assertion" blurb (Bjorn)
   -- Put binding names in DT commit Subject (Bjorn)
   -- Add a verb to a commit's subject line (Bjorn)
   -- s/accomodat(\w+)/accommodat$1/g (Bjorn)
   -- Rewrote commit msgs and comments refering panics if L1SS
      is enabled/disabled; the code snippet that unadvertises L1SS
      eliminates the panic scenario. (Bjorn)

v2 -- Changed binding property 'brcm,completion-timeout-msec' to
      'brcm,completion-timeout-us'.  (StefanW for standard suffix).
   -- Warn when clamping timeout value, and include clamped
      region in message. Also add min and max in YAML. (StefanW)
   -- Qualify description of "brcm,completion-timeout-us" so that
      it refers to PCIe transactions. (StefanW)
   -- Remvove mention of Linux specifics in binding description. (StefanW)
   -- s/clkreq#/CLKREQ#/g (Bjorn)
   -- Refactor completion-timeout-us code to compare max and min to
      value given by the property (as opposed to the computed value).

v1 -- The current driver assumes the downstream devices can
      provide CLKREQ# for ASPM.  These commits accomodate devices
      w/ or w/o clkreq# and also handle L1SS-capable devices.

   -- The Raspian Linux folks have already been using a PCIe RC
      property "brcm,enable-l1ss".  These commits use the same
      property, in a backward-compatible manner, and the implementaion
      adds more detail and also automatically identifies devices w/o
      a clkreq# signal, i.e. most devices plugged into an RPi CM4
      IO board.


Jim Quinlan (3):
  dt-bindings: PCI: brcmstb: Add property "brcm,clkreq-mode"
  PCI: brcmstb: Configure HW CLKREQ# mode appropriate for downstream
    device
  PCI: brcmstb: Set higher value for internal bus timeout

 .../bindings/pci/brcm,stb-pcie.yaml           | 21 +++++
 drivers/pci/controller/pcie-brcmstb.c         | 81 ++++++++++++++++---
 2 files changed, 92 insertions(+), 10 deletions(-)


base-commit: 305230142ae0637213bf6e04f6d9f10bbcb74af8
-- 
2.17.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]

[PATCH v7 1/3] dt-bindings: PCI: brcmstb: Add property "brcm,clkreq-mode"

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 2061 bytes --]

The Broadcom STB/CM PCIe HW -- a core that is also used by RPi SOCs --
requires the driver to deliberately place the RC HW one of three CLKREQ#
modes.  The "brcm,clkreq-mode" property allows the user to override the
default setting.  If this property is omitted, the default mode shall be
"default".

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

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index 7e15aae7d69e..992b35e915a5 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -64,6 +64,27 @@ properties:
 
   aspm-no-l0s: true
 
+  brcm,clkreq-mode:
+    description: A string that determines the operating
+      clkreq mode of the PCIe RC HW WRT controlling the refclk signal.
+      There are three different modes --
+      "safe", which drives the
+      refclk signal unconditionally and will work for all devices but does
+      not provide any power savings;
+      "no-l1ss" -- which provides Clock Power Management, L0s, and
+      L1, but cannot provide L1 substate (L1SS) power
+      savings. If the downstream device connected to the RC is
+      L1SS capable AND the OS enables L1SS, all PCIe traffic
+      may abruptly halt, potentially hanging the system;
+      "default" -- which provides L0s, L1, and L1SS, but not
+      compliant to provide Clock Power Management;
+      specifically, may not be able to meet the Tclron max
+      timing of 400ns as specified in "Dynamic Clock Control",
+      section 3.2.5.2.2 of the PCIe spec.  This situation is
+      atypical and should happen only with older devices.
+    $ref: /schemas/types.yaml#/definitions/string
+    enum: [ safe, no-l1ss, default ]
+
   brcm,scb-sizes:
     description: u64 giving the 64bit PCIe memory
       viewport size of a memory controller.  There may be up to
-- 
2.17.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]

Re: [PATCH v7 1/3] dt-bindings: PCI: brcmstb: Add property "brcm,clkreq-mode"

[Bjorn Helgaas]

On Thu, Nov 09, 2023 at 02:13:52PM -0500, Jim Quinlan wrote:
> The Broadcom STB/CM PCIe HW -- a core that is also used by RPi SOCs --
> requires the driver to deliberately place the RC HW one of three CLKREQ#
> modes.  The "brcm,clkreq-mode" property allows the user to override the
> default setting.  If this property is omitted, the default mode shall be
> "default".
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  .../bindings/pci/brcm,stb-pcie.yaml           | 21 +++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> index 7e15aae7d69e..992b35e915a5 100644
> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> @@ -64,6 +64,27 @@ properties:
>  
>    aspm-no-l0s: true
>  
> +  brcm,clkreq-mode:
> +    description: A string that determines the operating
> +      clkreq mode of the PCIe RC HW WRT controlling the refclk signal.

I assume "WRT" is shorthand for "with respect to", but it's slightly
confusing following all the other acronyms.

> +      There are three different modes --
> +      "safe", which drives the
> +      refclk signal unconditionally and will work for all devices but does
> +      not provide any power savings;
> +      "no-l1ss" -- which provides Clock Power Management, L0s, and
> +      L1, but cannot provide L1 substate (L1SS) power
> +      savings. If the downstream device connected to the RC is
> +      L1SS capable AND the OS enables L1SS, all PCIe traffic
> +      may abruptly halt, potentially hanging the system;
> +      "default" -- which provides L0s, L1, and L1SS, but not
> +      compliant to provide Clock Power Management;
> +      specifically, may not be able to meet the Tclron max
> +      timing of 400ns as specified in "Dynamic Clock Control",
> +      section 3.2.5.2.2 of the PCIe spec.  This situation is
> +      atypical and should happen only with older devices.

These are all weirdly wrapped.  Really no reason to use lines shorter
than 80.

Same spec citation question as in patch 2/3.

> +    $ref: /schemas/types.yaml#/definitions/string
> +    enum: [ safe, no-l1ss, default ]
> +
>    brcm,scb-sizes:
>      description: u64 giving the 64bit PCIe memory
>        viewport size of a memory controller.  There may be up to

Re: [PATCH v7 0/3] PCI: brcmstb: Configure appropriate HW CLKREQ# mode

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 4208 bytes --]

On 11/9/23 11:13, Jim Quinlan wrote:
> v7 -- Manivannan Sadhasivam suggested (a) making the property look like a
>        network phy-mode and (b) keeping the code simple (not counting clkreq
>        signal appearances, un-advertising capabilites, etc).  This is
>        what I have done.  The property is now "brcm,clkreq-mode" and
>        the values may be one of "safe", "default", and "no-l1ss".  The
>        default setting is to employ the most capable power savings mode.
> 
> v6 -- No code has been changed.
>     -- Changed commit subject and comment in "#PERST" commit (Bjorn, Cyril)
>     -- Changed sign-off and author email address for all commits.
>        This was due to a change in Broadcom's upstreaming policy.
> 
> v5 -- Remove DT property "brcm,completion-timeout-us" from	
>        "DT bindings" commit.  Although this error may be reported	
>        as a completion timeout, its cause was traced to an	
>        internal bus timeout which may occur even when there is	
>        no PCIe access being processed.  We set a timeout of four	
>        seconds only if we are operating in "L1SS CLKREQ#" mode.
>     -- Correct CEM 2.0 reference provided by HW engineer,
>        s/3.2.5.2.5/3.2.5.2.2/ (Bjorn)
>     -- Add newline to dev_info() string (Stefan)
>     -- Change variable rval to unsigned (Stefan)
>     -- s/implementaion/implementation/ (Bjorn)
>     -- s/superpowersave/powersupersave/ (Bjorn)
>     -- Slightly modify message on "PERST#" commit.
>     -- Rebase to torvalds master
> 
> v4 -- New commit that asserts PERST# for 2711/RPi SOCs at PCIe RC
>        driver probe() time.  This is done in Raspian Linux and its
>        absence may be the cause of a failing test case.
>     -- New commit that removes stale comment.
> 
> v3 -- Rewrote commit msgs and comments refering panics if L1SS
>        is enabled/disabled; the code snippet that unadvertises L1SS
>        eliminates the panic scenario. (Bjorn)
>     -- Add reference for "400ns of CLKREQ# assertion" blurb (Bjorn)
>     -- Put binding names in DT commit Subject (Bjorn)
>     -- Add a verb to a commit's subject line (Bjorn)
>     -- s/accomodat(\w+)/accommodat$1/g (Bjorn)
>     -- Rewrote commit msgs and comments refering panics if L1SS
>        is enabled/disabled; the code snippet that unadvertises L1SS
>        eliminates the panic scenario. (Bjorn)
> 
> v2 -- Changed binding property 'brcm,completion-timeout-msec' to
>        'brcm,completion-timeout-us'.  (StefanW for standard suffix).
>     -- Warn when clamping timeout value, and include clamped
>        region in message. Also add min and max in YAML. (StefanW)
>     -- Qualify description of "brcm,completion-timeout-us" so that
>        it refers to PCIe transactions. (StefanW)
>     -- Remvove mention of Linux specifics in binding description. (StefanW)
>     -- s/clkreq#/CLKREQ#/g (Bjorn)
>     -- Refactor completion-timeout-us code to compare max and min to
>        value given by the property (as opposed to the computed value).
> 
> v1 -- The current driver assumes the downstream devices can
>        provide CLKREQ# for ASPM.  These commits accomodate devices
>        w/ or w/o clkreq# and also handle L1SS-capable devices.
> 
>     -- The Raspian Linux folks have already been using a PCIe RC
>        property "brcm,enable-l1ss".  These commits use the same
>        property, in a backward-compatible manner, and the implementaion
>        adds more detail and also automatically identifies devices w/o
>        a clkreq# signal, i.e. most devices plugged into an RPi CM4
>        IO board.

For the entire series:

Tested-by: Florian Fainelli <florian.fainelli@broadcom.com>

on all of my available devices:

- SUNIX Co., Ltd. Multiport serial controller
- Broadcom BCM4331 Wi-Fi
- Broadcom BCM43224 Wi-Fi
- Broadcom BCM4322 Wi-Fi
- Qualcomm Atheros AR5008
- Broadcom BCM4366 Wi-Fi
- Marvell Technology Group Ltd. 88SE9125 PCIe SATA 6.0 Gb/s controller
- Intel 7260 Wi-Fi
- Intel Corporation 82574L Gigabit Network Connection
- Broadcom NetXtreme BCM5751 Gigabit Ethernet
- Pepperl+Fuchs RocketPort EXPRESS 8-port w/Octa Cable
- Micron/Crucial Technology P2 NVMe PCIe SSD
- ASM1184e PCIe Switch Port
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]