[PATCH v2 0/3] PCI: brcmstb: CLKREQ# accomodations of downstream device

[PATCH v2 0/3] PCI: brcmstb: CLKREQ# accomodations of downstream device

[Jim Quinlan]

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):
  PCI: brcmstb: CLKREQ# accomodations of downstream device
  PCI: brcmstb: Set PCIe transaction completion timeout
  blah blah

 drivers/pci/controller/pcie-brcmstb.c | 99 ++++++++++++++++++++++++---
 init/do_mounts.c                      | 16 ++++-
 2 files changed, 102 insertions(+), 13 deletions(-)


base-commit: 76f598ba7d8e2bfb4855b5298caedd5af0c374a8
prerequisite-patch-id: f38de8681d8746126d60b3430eaf218d2dd169cd
prerequisite-patch-id: 23e13189f200358976abf5bf3600973a20cf386c
prerequisite-patch-id: edfbe6ea39ed6a4937e2cec3bb8ee0e60091546d
prerequisite-patch-id: c87dd155e8506a2a277726c47d85bf3fa83727d5
prerequisite-patch-id: 579841e1dc179517506a7a7c42e0e651b3bc3649
prerequisite-patch-id: b5b079998ea451821edffd7c52cd3d89d06046a1
prerequisite-patch-id: b51b3918e554e279b2ace1f68ed6b4176f8ccc24
prerequisite-patch-id: 333e5188fb27d0ed010f5359e83e539172a67690
prerequisite-patch-id: bb107ee7b4811a9719508ea667cad2466933dec0
prerequisite-patch-id: 1258db336e778eb57d5cbea88834cd25aa1346ba
-- 
2.17.1

[PATCH v2 1/3] dt-bindings: PCI: brcmstb: Add two optional props

[Jim Quinlan]

Regarding "brcm,enable-l1ss":

  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).  Further, the
  HW may cause a CPU abort on boot if guesses wrong regarding the need for
  (c).  So we introduce the boolean "brcm,enable-l1ss" property to indicate
  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 will follow adds more details and
  discerns between (a) and (b).

Regarding "brcm,completion-timeout-us"

  Our HW will cause a CPU abort if the L1SS exit time is longer than the
  PCIe transaction completion abort timeout.  We've been asked to make this
  configurable, so we are introducing "brcm,completion-timeout-us".

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
---
 .../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..f7fc2f6561bb 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.  Note that when
+      in this mode, this particular HW may not meet the requirement
+      that requires CLKREQ# assertion to clock active to be
+      within 400ns.
+    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

Re: [PATCH v2 1/3] dt-bindings: PCI: brcmstb: Add two optional props

[Krzysztof Kozlowski]

On 11/04/2023 18:59, Jim Quinlan wrote:
> Regarding "brcm,enable-l1ss":
> 
>   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).  Further, the
>   HW may cause a CPU abort on boot if guesses wrong regarding the need for
>   (c).  So we introduce the boolean "brcm,enable-l1ss" property to indicate
>   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 will follow adds more details and

typo, implementation

>   discerns between (a) and (b).
> 
> Regarding "brcm,completion-timeout-us"
> 
>   Our HW will cause a CPU abort if the L1SS exit time is longer than the
>   PCIe transaction completion abort timeout.  We've been asked to make this
>   configurable, so we are introducing "brcm,completion-timeout-us".
> 
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>

What happened here? Where is the changelog?

Best regards,
Krzysztof

Re: [PATCH v2 1/3] dt-bindings: PCI: brcmstb: Add two optional props

[Florian Fainelli]

On 4/12/2023 1:09 AM, Krzysztof Kozlowski wrote:
> On 11/04/2023 18:59, Jim Quinlan wrote:
>> Regarding "brcm,enable-l1ss":
>>
>>    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).  Further, the
>>    HW may cause a CPU abort on boot if guesses wrong regarding the need for
>>    (c).  So we introduce the boolean "brcm,enable-l1ss" property to indicate
>>    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 will follow adds more details and
> 
> typo, implementation
> 
>>    discerns between (a) and (b).
>>
>> Regarding "brcm,completion-timeout-us"
>>
>>    Our HW will cause a CPU abort if the L1SS exit time is longer than the
>>    PCIe transaction completion abort timeout.  We've been asked to make this
>>    configurable, so we are introducing "brcm,completion-timeout-us".
>>
>> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> 
> What happened here? Where is the changelog?

It is in the cover letter:

https://lore.kernel.org/all/20230411165919.23955-1-jim2101024@gmail.com/

but it does not look like the cover letter was copied to you or Rob.
-- 
Florian

Re: [PATCH v2 1/3] dt-bindings: PCI: brcmstb: Add two optional props

[Krzysztof Kozlowski]

On 12/04/2023 13:49, Florian Fainelli wrote:
> 
> 
> On 4/12/2023 1:09 AM, Krzysztof Kozlowski wrote:
>> On 11/04/2023 18:59, Jim Quinlan wrote:
>>> Regarding "brcm,enable-l1ss":
>>>
>>>    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).  Further, the
>>>    HW may cause a CPU abort on boot if guesses wrong regarding the need for
>>>    (c).  So we introduce the boolean "brcm,enable-l1ss" property to indicate
>>>    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 will follow adds more details and
>>
>> typo, implementation
>>
>>>    discerns between (a) and (b).
>>>
>>> Regarding "brcm,completion-timeout-us"
>>>
>>>    Our HW will cause a CPU abort if the L1SS exit time is longer than the
>>>    PCIe transaction completion abort timeout.  We've been asked to make this
>>>    configurable, so we are introducing "brcm,completion-timeout-us".
>>>
>>> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
>>
>> What happened here? Where is the changelog?
> 
> It is in the cover letter:
> 
> https://lore.kernel.org/all/20230411165919.23955-1-jim2101024@gmail.com/
> 
> but it does not look like the cover letter was copied to you or Rob.

As you said, I did not get it.

Best regards,
Krzysztof

Re: [PATCH v2 1/3] dt-bindings: PCI: brcmstb: Add two optional props

[Jim Quinlan]

On Wed, Apr 12, 2023 at 7:56 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 12/04/2023 13:49, Florian Fainelli wrote:
> >
> >
> > On 4/12/2023 1:09 AM, Krzysztof Kozlowski wrote:
> >> On 11/04/2023 18:59, Jim Quinlan wrote:
> >>> Regarding "brcm,enable-l1ss":
> >>>
> >>>    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).  Further, the
> >>>    HW may cause a CPU abort on boot if guesses wrong regarding the need for
> >>>    (c).  So we introduce the boolean "brcm,enable-l1ss" property to indicate
> >>>    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 will follow adds more details and
> >>
> >> typo, implementation
> >>
> >>>    discerns between (a) and (b).
> >>>
> >>> Regarding "brcm,completion-timeout-us"
> >>>
> >>>    Our HW will cause a CPU abort if the L1SS exit time is longer than the
> >>>    PCIe transaction completion abort timeout.  We've been asked to make this
> >>>    configurable, so we are introducing "brcm,completion-timeout-us".
> >>>
> >>> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> >>
> >> What happened here? Where is the changelog?
> >
> > It is in the cover letter:
> >
> > https://lore.kernel.org/all/20230411165919.23955-1-jim2101024@gmail.com/
> >
> > but it does not look like the cover letter was copied to you or Rob.
>
> As you said, I did not get it.

Yes, sorry about that; I use a wrapper over the "cocci_cc" script and
I need to modify one or both scripts to send the cover to the
superset of recipients in the constituent commits.

Regards,
Jim Quinan
Broadcom STB
>
> Best regards,
> Krzysztof
>

Re: [PATCH v2 1/3] dt-bindings: PCI: brcmstb: Add two optional props

[Rob Herring]

On Wed, Apr 12, 2023 at 10:14:46AM -0400, Jim Quinlan wrote:
> On Wed, Apr 12, 2023 at 7:56 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
> >
> > On 12/04/2023 13:49, Florian Fainelli wrote:
> > >
> > >
> > > On 4/12/2023 1:09 AM, Krzysztof Kozlowski wrote:
> > >> On 11/04/2023 18:59, Jim Quinlan wrote:
> > >>> Regarding "brcm,enable-l1ss":
> > >>>
> > >>>    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).  Further, the
> > >>>    HW may cause a CPU abort on boot if guesses wrong regarding the need for
> > >>>    (c).  So we introduce the boolean "brcm,enable-l1ss" property to indicate
> > >>>    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 will follow adds more details and
> > >>
> > >> typo, implementation
> > >>
> > >>>    discerns between (a) and (b).
> > >>>
> > >>> Regarding "brcm,completion-timeout-us"
> > >>>
> > >>>    Our HW will cause a CPU abort if the L1SS exit time is longer than the
> > >>>    PCIe transaction completion abort timeout.  We've been asked to make this
> > >>>    configurable, so we are introducing "brcm,completion-timeout-us".
> > >>>
> > >>> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> > >>
> > >> What happened here? Where is the changelog?
> > >
> > > It is in the cover letter:
> > >
> > > https://lore.kernel.org/all/20230411165919.23955-1-jim2101024@gmail.com/
> > >
> > > but it does not look like the cover letter was copied to you or Rob.
> >
> > As you said, I did not get it.
> 
> Yes, sorry about that; I use a wrapper over the "cocci_cc" script and
> I need to modify one or both scripts to send the cover to the
> superset of recipients in the constituent commits.

Try out 'b4'. It's much easier.

In any case, I don't read cover letters. Changes to a patch belong with 
the patch.

Rob

Re: [PATCH v2 1/3] dt-bindings: PCI: brcmstb: Add two optional props

[Florian Fainelli]

On 4/12/23 08:37, Rob Herring wrote:
> On Wed, Apr 12, 2023 at 10:14:46AM -0400, Jim Quinlan wrote:
>> On Wed, Apr 12, 2023 at 7:56 AM Krzysztof Kozlowski
>> <krzysztof.kozlowski@linaro.org> wrote:
>>>
>>> On 12/04/2023 13:49, Florian Fainelli wrote:
>>>>
>>>>
>>>> On 4/12/2023 1:09 AM, Krzysztof Kozlowski wrote:
>>>>> On 11/04/2023 18:59, Jim Quinlan wrote:
>>>>>> Regarding "brcm,enable-l1ss":
>>>>>>
>>>>>>     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).  Further, the
>>>>>>     HW may cause a CPU abort on boot if guesses wrong regarding the need for
>>>>>>     (c).  So we introduce the boolean "brcm,enable-l1ss" property to indicate
>>>>>>     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 will follow adds more details and
>>>>>
>>>>> typo, implementation
>>>>>
>>>>>>     discerns between (a) and (b).
>>>>>>
>>>>>> Regarding "brcm,completion-timeout-us"
>>>>>>
>>>>>>     Our HW will cause a CPU abort if the L1SS exit time is longer than the
>>>>>>     PCIe transaction completion abort timeout.  We've been asked to make this
>>>>>>     configurable, so we are introducing "brcm,completion-timeout-us".
>>>>>>
>>>>>> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
>>>>>
>>>>> What happened here? Where is the changelog?
>>>>
>>>> It is in the cover letter:
>>>>
>>>> https://lore.kernel.org/all/20230411165919.23955-1-jim2101024@gmail.com/
>>>>
>>>> but it does not look like the cover letter was copied to you or Rob.
>>>
>>> As you said, I did not get it.
>>
>> Yes, sorry about that; I use a wrapper over the "cocci_cc" script and
>> I need to modify one or both scripts to send the cover to the
>> superset of recipients in the constituent commits.
> 
> Try out 'b4'. It's much easier.
> 
> In any case, I don't read cover letters. Changes to a patch belong with
> the patch.

This is not what most other maintainers do, and there does not appear to 
be a general consensus amongst maintainers that the changes belong in 
the individual patches, or in the cover letter. Some trees like the 
networking tree do merge commits of patch sets where the cover letter is 
used as part of the merge commit message. Other maintainers don't, and 
some want the change log after the '---' and some do not.
-- 
Florian

Re: [PATCH v2 0/3] PCI: brcmstb: CLKREQ# accomodations of downstream device

[Florian Fainelli]

On 4/11/23 09:59, Jim Quinlan wrote:
> 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):
>    PCI: brcmstb: CLKREQ# accomodations of downstream device
>    PCI: brcmstb: Set PCIe transaction completion timeout
>    blah blah

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

On a 7216 system test with:

01:00.0 Network controller: Intel Corporation Wireless 7260 (rev 73)

and on the CM4 I/O board with:

01:00.0 Network controller: Intel Corporation Wireless 7260 (rev 73)

01:00.0 Ethernet controller: Intel Corporation 82574L Gigabit Network 
Connection

01:00.0 Network controller: Broadcom Inc. and subsidiaries BCM43224 
802.11a/b/g/n (rev 01)

01:00.0 SATA controller: Marvell Technology Group Ltd. 88SE9125 PCIe 
SATA 6.0 Gb/s controller (rev 11) (prog-if 01 [AHCI 1.0])

01:00.0 Ethernet controller: Broadcom Inc. and subsidiaries NetXtreme 
BCM5751 Gigabit Ethernet PCI Express (rev 21)

01:00.0 PCI bridge: PLX Technology, Inc. PEX8112 x1 Lane PCI 
Express-to-PCI Bridge (rev aa)
02:00.0 Multiport serial controller: Pepperl+Fuchs RocketPort EXPRESS 
8-port w/Octa Cable

01:00.0 Ethernet controller: Qualcomm Atheros AR5008 Wireless Network 
Adapter (rev 01)

01:00.0 Network controller: Broadcom Inc. and subsidiaries BCM4311 
802.11a/b/g (rev 01)

01:00.0 Network controller: Broadcom Inc. and subsidiaries BCM4322 
802.11a/b/g/n Wireless LAN Controller (rev 01)

01:00.0 Network controller: Broadcom Inc. and subsidiaries BCM43602 
802.11ac Wireless LAN SoC (rev 01)

and finally with a 4 port switch:

-[0000:00]---00.0-[01-07]----00.0-[02-07]--+-01.0-[03]----00.0  Intel 
Corporation 82574L Gigabit Network Connection
 
+-03.0-[04-05]----00.0-[05]----00.0  Pepperl+Fuchs RocketPort EXPRESS 
8-port w/Octa Cable
                                            +-05.0-[06]----00.0 
Broadcom Inc. and subsidiaries NetXtreme BCM5751 Gigabit Ethernet PCI 
Express
                                            \-07.0-[07]----00.0  Intel 
Corporation 82574L Gigabit Network Connection

And than I ran out of devices that I could plug, the others were x4, x8 
or x16.

Most (all?) would previously fail, so definitively an improvement!

Thanks!
-- 
-- 
Florian

Re: [PATCH v2 1/3] dt-bindings: PCI: brcmstb: Add two optional props

[Bjorn Helgaas]

It'd be nice to mention the property names (maybe omit the "brcm,"
prefix if that helps) in the commit log so "git log --oneline" is more
useful:

  959e000f0463 ("dt-bindings: PCI: brcmstb: Add two optional props")
  ea372f45cfff ("dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators")
  504253e44a9d ("dt-bindings: PCI: Correct brcmstb interrupts, interrupt-map.")
  145790e55d82 ("dt-bindings: PCI: Add compatible string for Brcmstb 74[23]5 MIPs SOCs")
  5e8a7d26d935 ("dt-bindings: PCI: brcmstb: compatible is required")
  f435ce7ebf8c ("dt-bindings: PCI: brcmstb: add BCM4908 binding")

On Tue, Apr 11, 2023 at 12:59:16PM -0400, Jim Quinlan wrote:
> Regarding "brcm,enable-l1ss":
> 
>   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).  Further, the
>   HW may cause a CPU abort on boot if guesses wrong regarding the need for
>   (c).  So we introduce the boolean "brcm,enable-l1ss" property to indicate
>   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 will follow adds more details and
>   discerns between (a) and (b).
> 
> Regarding "brcm,completion-timeout-us"
> 
>   Our HW will cause a CPU abort if the L1SS exit time is longer than the
>   PCIe transaction completion abort timeout.  We've been asked to make this
>   configurable, so we are introducing "brcm,completion-timeout-us".

Completion Timeout is a generic PCIe concept.  Do we want a generic
(non-brcm) name that would be documented elsewhere?  Rob?

> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> ---
>  .../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..f7fc2f6561bb 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.  Note that when
> +      in this mode, this particular HW may not meet the requirement
> +      that requires CLKREQ# assertion to clock active to be
> +      within 400ns.

Maybe a pointer to the source of the 400ns requirement?

"requirement that requires" is a little redundant, maybe "... may not
meet the requirement that Refclk be valid within 400ns of CLKREQ#
assertion"?

(I don't actually know whether this refers to Refclk or if that would
be a true statement; this is just a possible sentence structure.)

Re: [PATCH v2 1/3] dt-bindings: PCI: brcmstb: Add two optional props

[Rob Herring]

On Wed, Apr 12, 2023 at 09:12:07AM -0700, Florian Fainelli wrote:
> On 4/12/23 08:37, Rob Herring wrote:
> > On Wed, Apr 12, 2023 at 10:14:46AM -0400, Jim Quinlan wrote:
> > > On Wed, Apr 12, 2023 at 7:56 AM Krzysztof Kozlowski
> > > <krzysztof.kozlowski@linaro.org> wrote:
> > > > 
> > > > On 12/04/2023 13:49, Florian Fainelli wrote:
> > > > > 
> > > > > 
> > > > > On 4/12/2023 1:09 AM, Krzysztof Kozlowski wrote:
> > > > > > On 11/04/2023 18:59, Jim Quinlan wrote:
> > > > > > > Regarding "brcm,enable-l1ss":
> > > > > > > 
> > > > > > >     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).  Further, the
> > > > > > >     HW may cause a CPU abort on boot if guesses wrong regarding the need for
> > > > > > >     (c).  So we introduce the boolean "brcm,enable-l1ss" property to indicate
> > > > > > >     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 will follow adds more details and
> > > > > > 
> > > > > > typo, implementation
> > > > > > 
> > > > > > >     discerns between (a) and (b).
> > > > > > > 
> > > > > > > Regarding "brcm,completion-timeout-us"
> > > > > > > 
> > > > > > >     Our HW will cause a CPU abort if the L1SS exit time is longer than the
> > > > > > >     PCIe transaction completion abort timeout.  We've been asked to make this
> > > > > > >     configurable, so we are introducing "brcm,completion-timeout-us".
> > > > > > > 
> > > > > > > Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> > > > > > 
> > > > > > What happened here? Where is the changelog?
> > > > > 
> > > > > It is in the cover letter:
> > > > > 
> > > > > https://lore.kernel.org/all/20230411165919.23955-1-jim2101024@gmail.com/
> > > > > 
> > > > > but it does not look like the cover letter was copied to you or Rob.
> > > > 
> > > > As you said, I did not get it.
> > > 
> > > Yes, sorry about that; I use a wrapper over the "cocci_cc" script and
> > > I need to modify one or both scripts to send the cover to the
> > > superset of recipients in the constituent commits.
> > 
> > Try out 'b4'. It's much easier.
> > 
> > In any case, I don't read cover letters. Changes to a patch belong with
> > the patch.
> 
> This is not what most other maintainers do, and there does not appear to be
> a general consensus amongst maintainers that the changes belong in the
> individual patches, or in the cover letter. 

Well, I stole that phrase from someone else (gregkh). 

> Some trees like the networking
> tree do merge commits of patch sets where the cover letter is used as part
> of the merge commit message. Other maintainers don't, and some want the
> change log after the '---' and some do not.

I'm not aware of anyone except for DRM wanting the changelog in the 
final commits, but that's really a different issue.

I'm pretty sure no one will complain about a changelog in the patches. I 
guess you just have to duplicate it if you think it should be in both. 
b4 could be taught to do that I suppose. IMO, the cover letter should 
have a higher level changelog than the individual patches.

Rob

Re: [PATCH v2 1/3] dt-bindings: PCI: brcmstb: Add two optional props

[Konstantin Ryabitsev]

April 18, 2023 2:35 PM, "Rob Herring" <robh@kernel.org> wrote:
>> Some trees like the networking
>> tree do merge commits of patch sets where the cover letter is used as part
>> of the merge commit message. Other maintainers don't, and some want the
>> change log after the '---' and some do not.
> 
> I'm not aware of anyone except for DRM wanting the changelog in the
> final commits, but that's really a different issue.

I don't think anyone wants changelogs in actual final commits, they usually go under "---" in patch submissions.

> I'm pretty sure no one will complain about a changelog in the patches. I
> guess you just have to duplicate it if you think it should be in both.
> b4 could be taught to do that I suppose. IMO, the cover letter should
> have a higher level changelog than the individual patches.

b4 doesn't really need to manage per-patch changelogs -- they should just go under "---" in the commit. When you send the series either via "b4 send" or via git-send-email, the changelogs will be properly included in the message, but they won't make it into the tree after the maintainer runs "git am", because git will drop anything under the first "---" in the commit message.

The cover letter changelog is supposed to be higher level than individual patch changelogs, so I don't think it makes sense for b4 to collect them from individual patches.

-K