[PATCH v1 0/3] PCI: brcmstb: Clkreq# accomodations of downstream device

[PATCH v1 0/3] PCI: brcmstb: Clkreq# accomodations of downstream device

[Jim Quinlan]

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 two optional props
  PCI: brcmstb: Clkreq# accomodations of downstream device
  PCI: brcmstb: Allow setting the completion timeout

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


base-commit: 99ddf2254febae9eab7fb0bcc02c5322243f5c49
-- 
2.17.1

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

[Jim Quinlan]

Regarding "brcm,enable-l1ss":

  The Broadcom STB/CM PCIe HW -- which is also used by RPi SOCs -- requires
  the driver probe to configure one of three clkreq# modes:

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

  The HW can tell the difference between (a) and (b), but does not know
  when to configure (c).  Further, the HW will 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.  This
  property is already present in the Raspian version of Linux, but the
  driver implementaion that will follow adds more details and discerns
  between (a) and (b).

Regarding "brcm,completion-timeout-msecs"

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

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

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index 7e15aae7d69e..ef4ccc05b258 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -64,6 +64,18 @@ properties:
 
   aspm-no-l0s: true
 
+  brcm,enable-l1ss:
+    description: Indicates that the downstream device is L1SS
+      capable and L1SS is desired, e.g. by setting
+      CONFIG_PCIEASPM_POWER_SUPERSAVE=y.  Note that CLKREQ#
+      assertion to clock active must be within 400ns.
+    type: boolean
+
+  brcm,completion-timeout-msecs:
+    description: Number of msecs before completion timeout
+      abort occurs.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
   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 v1 1/3] dt-bindings: PCI: brcmstb: Add two optional props

[Stefan Wahren]

Hi Jim,

Am 06.04.23 um 14:46 schrieb Jim Quinlan:
> Regarding "brcm,enable-l1ss":
> 
>    The Broadcom STB/CM PCIe HW -- which is also used by RPi SOCs -- requires
>    the driver probe to configure one of three clkreq# modes:
> 
>    (a) clkreq# driven by the RC
>    (b) clkreq# driven by the EP for ASPM L0s, L1
>    (c) bidirectional clkreq#, as used for L1 Substates (L1SS).
> 
>    The HW can tell the difference between (a) and (b), but does not know
>    when to configure (c).  Further, the HW will 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.  This
>    property is already present in the Raspian version of Linux, but the
>    driver implementaion that will follow adds more details and discerns
>    between (a) and (b).
> 
> Regarding "brcm,completion-timeout-msecs"
> 
>    Our HW will cause a CPU abort if the L1SS exit time is longer than the
>    completion abort timeout.  We've been asked to make this configurable, so
>    we are introducing "brcm,completion-abort-msecs".
> 
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> ---
>   .../devicetree/bindings/pci/brcm,stb-pcie.yaml       | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> index 7e15aae7d69e..ef4ccc05b258 100644
> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> @@ -64,6 +64,18 @@ properties:
>   
>     aspm-no-l0s: true
>   
> +  brcm,enable-l1ss:
> +    description: Indicates that the downstream device is L1SS
> +      capable and L1SS is desired, e.g. by setting
> +      CONFIG_PCIEASPM_POWER_SUPERSAVE=y.  Note that CLKREQ#

not sure about this, but maybe we should avoid references to Linux 
kernel config parameter in a DT binding. Since the driver already gaves 
warning in case the DT parameter is present, but kernel config doesn't 
fit, this should be enough.

> +      assertion to clock active must be within 400ns.
> +    type: boolean
> +
> +  brcm,completion-timeout-msecs:
> +    description: Number of msecs before completion timeout
> +      abort occurs.
> +    $ref: /schemas/types.yaml#/definitions/uint32

According to the driver at least 0 is not allowed, maybe we should 
define minimum and maximum here and let dtbs_check take care of invalid 
values?

Best regards

> +
>     brcm,scb-sizes:
>       description: u64 giving the 64bit PCIe memory
>         viewport size of a memory controller.  There may be up to

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

[Jim Quinlan]

On Thu, Apr 6, 2023 at 11:39 AM Stefan Wahren <stefan.wahren@i2se.com> wrote:
>
> Hi Jim,
>
> Am 06.04.23 um 14:46 schrieb Jim Quinlan:
> > Regarding "brcm,enable-l1ss":
> >
> >    The Broadcom STB/CM PCIe HW -- which is also used by RPi SOCs -- requires
> >    the driver probe to configure one of three clkreq# modes:
> >
> >    (a) clkreq# driven by the RC
> >    (b) clkreq# driven by the EP for ASPM L0s, L1
> >    (c) bidirectional clkreq#, as used for L1 Substates (L1SS).
> >
> >    The HW can tell the difference between (a) and (b), but does not know
> >    when to configure (c).  Further, the HW will 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.  This
> >    property is already present in the Raspian version of Linux, but the
> >    driver implementaion that will follow adds more details and discerns
> >    between (a) and (b).
> >
> > Regarding "brcm,completion-timeout-msecs"
> >
> >    Our HW will cause a CPU abort if the L1SS exit time is longer than the
> >    completion abort timeout.  We've been asked to make this configurable, so
> >    we are introducing "brcm,completion-abort-msecs".
> >
> > Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> > ---
> >   .../devicetree/bindings/pci/brcm,stb-pcie.yaml       | 12 ++++++++++++
> >   1 file changed, 12 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > index 7e15aae7d69e..ef4ccc05b258 100644
> > --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > @@ -64,6 +64,18 @@ properties:
> >
> >     aspm-no-l0s: true
> >
> > +  brcm,enable-l1ss:
> > +    description: Indicates that the downstream device is L1SS
> > +      capable and L1SS is desired, e.g. by setting
> > +      CONFIG_PCIEASPM_POWER_SUPERSAVE=y.  Note that CLKREQ#
>
> not sure about this, but maybe we should avoid references to Linux
> kernel config parameter in a DT binding. Since the driver already gaves
> warning in case the DT parameter is present, but kernel config doesn't
> fit, this should be enough.

Hello Stefan,
I will remove this reference.
>
> > +      assertion to clock active must be within 400ns.
> > +    type: boolean
> > +
> > +  brcm,completion-timeout-msecs:
> > +    description: Number of msecs before completion timeout
> > +      abort occurs.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
>
> According to the driver at least 0 is not allowed, maybe we should
> define minimum and maximum here and let dtbs_check take care of invalid
> values?
I'm not sure I follow what you mean about a zero value;  the property
may have any value but the driver will clamp it
to a minimum of ~30msec.  Regardless, I can add a  "minimum: 30" line
to the YAML.

Thanks,
Jim Quinlan
Broadcom STB
>
> Best regards
>
> > +
> >     brcm,scb-sizes:
> >       description: u64 giving the 64bit PCIe memory
> >         viewport size of a memory controller.  There may be up to

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

[Krzysztof Kozlowski]

On 06/04/2023 18:58, Jim Quinlan wrote:
> On Thu, Apr 6, 2023 at 11:39 AM Stefan Wahren <stefan.wahren@i2se.com> wrote:
>>
>> Hi Jim,
>>
>> Am 06.04.23 um 14:46 schrieb Jim Quinlan:
>>> Regarding "brcm,enable-l1ss":
>>>
>>>    The Broadcom STB/CM PCIe HW -- which is also used by RPi SOCs -- requires
>>>    the driver probe to configure one of three clkreq# modes:
>>>
>>>    (a) clkreq# driven by the RC
>>>    (b) clkreq# driven by the EP for ASPM L0s, L1
>>>    (c) bidirectional clkreq#, as used for L1 Substates (L1SS).
>>>
>>>    The HW can tell the difference between (a) and (b), but does not know
>>>    when to configure (c).  Further, the HW will 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.  This
>>>    property is already present in the Raspian version of Linux, but the
>>>    driver implementaion that will follow adds more details and discerns
>>>    between (a) and (b).
>>>
>>> Regarding "brcm,completion-timeout-msecs"
>>>
>>>    Our HW will cause a CPU abort if the L1SS exit time is longer than the
>>>    completion abort timeout.  We've been asked to make this configurable, so
>>>    we are introducing "brcm,completion-abort-msecs".
>>>
>>> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
>>> ---
>>>   .../devicetree/bindings/pci/brcm,stb-pcie.yaml       | 12 ++++++++++++
>>>   1 file changed, 12 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>> index 7e15aae7d69e..ef4ccc05b258 100644
>>> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>> @@ -64,6 +64,18 @@ properties:
>>>
>>>     aspm-no-l0s: true
>>>
>>> +  brcm,enable-l1ss:
>>> +    description: Indicates that the downstream device is L1SS
>>> +      capable and L1SS is desired, e.g. by setting
>>> +      CONFIG_PCIEASPM_POWER_SUPERSAVE=y.  Note that CLKREQ#
>>
>> not sure about this, but maybe we should avoid references to Linux
>> kernel config parameter in a DT binding. Since the driver already gaves
>> warning in case the DT parameter is present, but kernel config doesn't
>> fit, this should be enough.
> 
> Hello Stefan,
> I will remove this reference.
>>
>>> +      assertion to clock active must be within 400ns.
>>> +    type: boolean
>>> +
>>> +  brcm,completion-timeout-msecs:
>>> +    description: Number of msecs before completion timeout
>>> +      abort occurs.
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>
>> According to the driver at least 0 is not allowed, maybe we should
>> define minimum and maximum here and let dtbs_check take care of invalid
>> values?
> I'm not sure I follow what you mean about a zero value;  the property
> may have any value but the driver will clamp it
> to a minimum of ~30msec.  Regardless, I can add a  "minimum: 30" line
> to the YAML.

If "completion" means Linux completion, then it is not suitable for DT
and entire property should be removed. If it is something else, then
explain here and commit msg. So far both refer to some completion...

Best regards,
Krzysztof

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

[Florian Fainelli]

On 4/6/23 11:35, Krzysztof Kozlowski wrote:
> On 06/04/2023 18:58, Jim Quinlan wrote:
>> On Thu, Apr 6, 2023 at 11:39 AM Stefan Wahren <stefan.wahren@i2se.com> wrote:
>>>
>>> Hi Jim,
>>>
>>> Am 06.04.23 um 14:46 schrieb Jim Quinlan:
>>>> Regarding "brcm,enable-l1ss":
>>>>
>>>>     The Broadcom STB/CM PCIe HW -- which is also used by RPi SOCs -- requires
>>>>     the driver probe to configure one of three clkreq# modes:
>>>>
>>>>     (a) clkreq# driven by the RC
>>>>     (b) clkreq# driven by the EP for ASPM L0s, L1
>>>>     (c) bidirectional clkreq#, as used for L1 Substates (L1SS).
>>>>
>>>>     The HW can tell the difference between (a) and (b), but does not know
>>>>     when to configure (c).  Further, the HW will 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.  This
>>>>     property is already present in the Raspian version of Linux, but the
>>>>     driver implementaion that will follow adds more details and discerns
>>>>     between (a) and (b).
>>>>
>>>> Regarding "brcm,completion-timeout-msecs"
>>>>
>>>>     Our HW will cause a CPU abort if the L1SS exit time is longer than the
>>>>     completion abort timeout.  We've been asked to make this configurable, so
>>>>     we are introducing "brcm,completion-abort-msecs".
>>>>
>>>> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
>>>> ---
>>>>    .../devicetree/bindings/pci/brcm,stb-pcie.yaml       | 12 ++++++++++++
>>>>    1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>>> index 7e15aae7d69e..ef4ccc05b258 100644
>>>> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>>> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>>> @@ -64,6 +64,18 @@ properties:
>>>>
>>>>      aspm-no-l0s: true
>>>>
>>>> +  brcm,enable-l1ss:
>>>> +    description: Indicates that the downstream device is L1SS
>>>> +      capable and L1SS is desired, e.g. by setting
>>>> +      CONFIG_PCIEASPM_POWER_SUPERSAVE=y.  Note that CLKREQ#
>>>
>>> not sure about this, but maybe we should avoid references to Linux
>>> kernel config parameter in a DT binding. Since the driver already gaves
>>> warning in case the DT parameter is present, but kernel config doesn't
>>> fit, this should be enough.
>>
>> Hello Stefan,
>> I will remove this reference.
>>>
>>>> +      assertion to clock active must be within 400ns.
>>>> +    type: boolean
>>>> +
>>>> +  brcm,completion-timeout-msecs:
>>>> +    description: Number of msecs before completion timeout
>>>> +      abort occurs.
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>
>>> According to the driver at least 0 is not allowed, maybe we should
>>> define minimum and maximum here and let dtbs_check take care of invalid
>>> values?
>> I'm not sure I follow what you mean about a zero value;  the property
>> may have any value but the driver will clamp it
>> to a minimum of ~30msec.  Regardless, I can add a  "minimum: 30" line
>> to the YAML.
> 
> If "completion" means Linux completion, then it is not suitable for DT
> and entire property should be removed. If it is something else, then
> explain here and commit msg. So far both refer to some completion...

This is a PCIe root complex binding so completion needs to be understood 
in the context of PCIe, that is the time needed for the root complex to 
complete/finish/proceed with a PCIe transaction layer packet (TLP).
-- 
Florian

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

[Krzysztof Kozlowski]

On 06/04/2023 20:47, Florian Fainelli wrote:
>>>>> +
>>>>> +  brcm,completion-timeout-msecs:
>>>>> +    description: Number of msecs before completion timeout
>>>>> +      abort occurs.
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>
>>>> According to the driver at least 0 is not allowed, maybe we should
>>>> define minimum and maximum here and let dtbs_check take care of invalid
>>>> values?
>>> I'm not sure I follow what you mean about a zero value;  the property
>>> may have any value but the driver will clamp it
>>> to a minimum of ~30msec.  Regardless, I can add a  "minimum: 30" line
>>> to the YAML.
>>
>> If "completion" means Linux completion, then it is not suitable for DT
>> and entire property should be removed. If it is something else, then
>> explain here and commit msg. So far both refer to some completion...
> 
> This is a PCIe root complex binding so completion needs to be understood 
> in the context of PCIe, that is the time needed for the root complex to 
> complete/finish/proceed with a PCIe transaction layer packet (TLP).

OK, so I assume keyword "completion" is well known term in PCI (although
for some reason no other bindings mention it).

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 06/04/2023 14:46, Jim Quinlan wrote:
> Regarding "brcm,enable-l1ss":
> 
>   The Broadcom STB/CM PCIe HW -- which is also used by RPi SOCs -- requires
>   the driver probe to configure one of three clkreq# modes:
> 
>   (a) clkreq# driven by the RC
>   (b) clkreq# driven by the EP for ASPM L0s, L1
>   (c) bidirectional clkreq#, as used for L1 Substates (L1SS).
> 
>   The HW can tell the difference between (a) and (b), but does not know
>   when to configure (c).  Further, the HW will 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.  This
>   property is already present in the Raspian version of Linux, but the
>   driver implementaion that will follow adds more details and discerns
>   between (a) and (b).
> 
> Regarding "brcm,completion-timeout-msecs"
> 
>   Our HW will cause a CPU abort if the L1SS exit time is longer than the
>   completion abort timeout.  We've been asked to make this configurable, so
>   we are introducing "brcm,completion-abort-msecs".
> 
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> ---
>  .../devicetree/bindings/pci/brcm,stb-pcie.yaml       | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> index 7e15aae7d69e..ef4ccc05b258 100644
> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> @@ -64,6 +64,18 @@ properties:
>  
>    aspm-no-l0s: true
>  
> +  brcm,enable-l1ss:
> +    description: Indicates that the downstream device is L1SS
> +      capable and L1SS is desired, e.g. by setting
> +      CONFIG_PCIEASPM_POWER_SUPERSAVE=y.  Note that CLKREQ#

How does CONFIG_PCIEASPM_POWER_SUPERSAVE apply to *BSD?

> +      assertion to clock active must be within 400ns.
> +    type: boolean
> +
> +  brcm,completion-timeout-msecs:

Use standard unit suffixes.

> +    description: Number of msecs before completion timeout
> +      abort occurs.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
>    brcm,scb-sizes:
>      description: u64 giving the 64bit PCIe memory
>        viewport size of a memory controller.  There may be up to

Best regards,
Krzysztof

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

[Florian Fainelli]

On 4/6/23 11:34, Krzysztof Kozlowski wrote:
> On 06/04/2023 14:46, Jim Quinlan wrote:
>> Regarding "brcm,enable-l1ss":
>>
>>    The Broadcom STB/CM PCIe HW -- which is also used by RPi SOCs -- requires
>>    the driver probe to configure one of three clkreq# modes:
>>
>>    (a) clkreq# driven by the RC
>>    (b) clkreq# driven by the EP for ASPM L0s, L1
>>    (c) bidirectional clkreq#, as used for L1 Substates (L1SS).
>>
>>    The HW can tell the difference between (a) and (b), but does not know
>>    when to configure (c).  Further, the HW will 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.  This
>>    property is already present in the Raspian version of Linux, but the
>>    driver implementaion that will follow adds more details and discerns
>>    between (a) and (b).
>>
>> Regarding "brcm,completion-timeout-msecs"
>>
>>    Our HW will cause a CPU abort if the L1SS exit time is longer than the
>>    completion abort timeout.  We've been asked to make this configurable, so
>>    we are introducing "brcm,completion-abort-msecs".
>>
>> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
>> ---
>>   .../devicetree/bindings/pci/brcm,stb-pcie.yaml       | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>> index 7e15aae7d69e..ef4ccc05b258 100644
>> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>> @@ -64,6 +64,18 @@ properties:
>>   
>>     aspm-no-l0s: true
>>   
>> +  brcm,enable-l1ss:
>> +    description: Indicates that the downstream device is L1SS
>> +      capable and L1SS is desired, e.g. by setting
>> +      CONFIG_PCIEASPM_POWER_SUPERSAVE=y.  Note that CLKREQ#
> 
> How does CONFIG_PCIEASPM_POWER_SUPERSAVE apply to *BSD?

In other words, there should be no OS/Linux specific comments in a 
Device Tree binding, which would be a friendlier and nicer way of 
providing the same feedback.
-- 
Florian

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

[Krzysztof Kozlowski]

On 06/04/2023 20:53, Florian Fainelli wrote:
> On 4/6/23 11:34, Krzysztof Kozlowski wrote:
>> On 06/04/2023 14:46, Jim Quinlan wrote:
>>> Regarding "brcm,enable-l1ss":
>>>
>>>    The Broadcom STB/CM PCIe HW -- which is also used by RPi SOCs -- requires
>>>    the driver probe to configure one of three clkreq# modes:
>>>
>>>    (a) clkreq# driven by the RC
>>>    (b) clkreq# driven by the EP for ASPM L0s, L1
>>>    (c) bidirectional clkreq#, as used for L1 Substates (L1SS).
>>>
>>>    The HW can tell the difference between (a) and (b), but does not know
>>>    when to configure (c).  Further, the HW will 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.  This
>>>    property is already present in the Raspian version of Linux, but the
>>>    driver implementaion that will follow adds more details and discerns
>>>    between (a) and (b).
>>>
>>> Regarding "brcm,completion-timeout-msecs"
>>>
>>>    Our HW will cause a CPU abort if the L1SS exit time is longer than the
>>>    completion abort timeout.  We've been asked to make this configurable, so
>>>    we are introducing "brcm,completion-abort-msecs".
>>>
>>> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
>>> ---
>>>   .../devicetree/bindings/pci/brcm,stb-pcie.yaml       | 12 ++++++++++++
>>>   1 file changed, 12 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>> index 7e15aae7d69e..ef4ccc05b258 100644
>>> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>> @@ -64,6 +64,18 @@ properties:
>>>   
>>>     aspm-no-l0s: true
>>>   
>>> +  brcm,enable-l1ss:
>>> +    description: Indicates that the downstream device is L1SS
>>> +      capable and L1SS is desired, e.g. by setting
>>> +      CONFIG_PCIEASPM_POWER_SUPERSAVE=y.  Note that CLKREQ#
>>
>> How does CONFIG_PCIEASPM_POWER_SUPERSAVE apply to *BSD?
> 
> In other words, there should be no OS/Linux specific comments in a 
> Device Tree binding, which would be a friendlier and nicer way of 
> providing the same feedback.

I want to give also the answer also why there should be no OS/Linux
specific comments, so the reader can stop a bit and think about it :)

Best regards,
Krzysztof

Re: [PATCH v1 0/3] PCI: brcmstb: Clkreq# accomodations of downstream device

[Cyril Brulebois]

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

Hi Jim,

Jim Quinlan <jim2101024@gmail.com> (2023-04-06):
> 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 two optional props
>   PCI: brcmstb: Clkreq# accomodations of downstream device
>   PCI: brcmstb: Allow setting the completion timeout
> 
>  .../bindings/pci/brcm,stb-pcie.yaml           | 12 +++
>  drivers/pci/controller/pcie-brcmstb.c         | 93 +++++++++++++++++--
>  2 files changed, 95 insertions(+), 10 deletions(-)
> 
> 
> base-commit: 99ddf2254febae9eab7fb0bcc02c5322243f5c49

I've just verified with the exact same hardware as in Bugzilla#217276
that latest master (v6.3-rc5-137-gf2afccfefe7b) still gets a kernel
panic at boot, which goes away once those 3 patches are applied. Do you
need any extra information, log excerpt, or something like that?


Cheers,
-- 
Cyril Brulebois (kibi@debian.org)            <https://debamax.com/>
D-I release manager -- Release team member -- Freelance Consultant

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 0/3] PCI: brcmstb: Clkreq# accomodations of downstream device

[Hank Barta]

Apologies to anyone who is seeing this for the second time. I forgot
to format as plain text on the first transmission.

Cyril Brulebois kindly provided me with Debian packages for the two
kernels he built and I tested them as well. I can confirm that the one
without the three patches still had the panic on power up and when a
PCIe device is present. Otherwise it booted without difficulty. The
kernel with three patches booted successfully on all tries both cold
and warm with a PCIe/NVME installed. I was also able to mount
partitions on the NVME driver and copy data off.

My hardware

CM4 with WiFi/BT, 8GG RAM and no eMMC.
(Official) I/O board
PCIe/NVME adapter card with Intel 670p SSD installed
32GB Samsung EVO Select SD card (boot device.)

Thanks to all who contributed to this fix.


On Thu, Apr 6, 2023 at 4:31 PM Cyril Brulebois <kibi@debian.org> wrote:
>
> Hi Jim,
>
> Jim Quinlan <jim2101024@gmail.com> (2023-04-06):
> > 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 two optional props
> >   PCI: brcmstb: Clkreq# accomodations of downstream device
> >   PCI: brcmstb: Allow setting the completion timeout
> >
> >  .../bindings/pci/brcm,stb-pcie.yaml           | 12 +++
> >  drivers/pci/controller/pcie-brcmstb.c         | 93 +++++++++++++++++--
> >  2 files changed, 95 insertions(+), 10 deletions(-)
> >
> >
> > base-commit: 99ddf2254febae9eab7fb0bcc02c5322243f5c49
>
> I've just verified with the exact same hardware as in Bugzilla#217276
> that latest master (v6.3-rc5-137-gf2afccfefe7b) still gets a kernel
> panic at boot, which goes away once those 3 patches are applied. Do you
> need any extra information, log excerpt, or something like that?
>
>
> Cheers,
> --
> Cyril Brulebois (kibi@debian.org)            <https://debamax.com/>
> D-I release manager -- Release team member -- Freelance Consultant



-- 
Beautiful Sunny Winfield