| Message ID | 20230406124625.41325-2-jim2101024@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: brcmstb: Clkreq# accomodations of downstream device | expand |
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
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
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
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
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).
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.
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
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
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
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(+)