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