Re: [PATCH v6 0/5] PCI: brcmstb: Configure appropriate HW CLKREQ# mode

[Jim Quinlan]

On Mon, Aug 21, 2023 at 4:35 AM Lorenzo Pieralisi <lpieralisi@xxxxxxxxxx> wrote:
>
> On Fri, Jun 23, 2023 at 10:40:53AM -0400, Jim Quinlan wrote:
> > v6 -- No code has been changed.
> >    -- Changed commit subject and comment in "#PERST" commit (Bjorn, Cyril)
> >    -- Changed sign-off and author email address for all commits.
> >       This was due to a change in Broadcom's upstreaming policy.
> >
> > v5 -- Remove DT property "brcm,completion-timeout-us" from
> >       "DT bindings" commit.  Although this error may be reported
> >       as a completion timeout, its cause was traced to an
> >       internal bus timeout which may occur even when there is
> >       no PCIe access being processed.  We set a timeout of four
> >       seconds only if we are operating in "L1SS CLKREQ#" mode.
> >    -- Correct CEM 2.0 reference provided by HW engineer,
> >       s/3.2.5.2.5/3.2.5.2.2/ (Bjorn)
> >    -- Add newline to dev_info() string (Stefan)
> >    -- Change variable rval to unsigned (Stefan)
> >    -- s/implementaion/implementation/ (Bjorn)
> >    -- s/superpowersave/powersupersave/ (Bjorn)
> >    -- Slightly modify message on "PERST#" commit.
> >    -- Rebase to torvalds master
> >
> > v4 -- New commit that asserts PERST# for 2711/RPi SOCs at PCIe RC
> >       driver probe() time.  This is done in Raspian Linux and its
> >       absence may be the cause of a failing test case.
> >    -- New commit that removes stale comment.
> >
> > v3 -- Rewrote commit msgs and comments refering panics if L1SS
> >       is enabled/disabled; the code snippet that unadvertises L1SS
> >       eliminates the panic scenario. (Bjorn)
> >    -- Add reference for "400ns of CLKREQ# assertion" blurb (Bjorn)
> >    -- Put binding names in DT commit Subject (Bjorn)
> >    -- Add a verb to a commit's subject line (Bjorn)
> >    -- s/accomodat(\w+)/accommodat$1/g (Bjorn)
> >    -- Rewrote commit msgs and comments refering panics if L1SS
> >       is enabled/disabled; the code snippet that unadvertises L1SS
> >       eliminates the panic scenario. (Bjorn)
> >
> > v2 -- Changed binding property 'brcm,completion-timeout-msec' to
> >       'brcm,completion-timeout-us'.  (StefanW for standard suffix).
> >    -- Warn when clamping timeout value, and include clamped
> >       region in message. Also add min and max in YAML. (StefanW)
> >    -- Qualify description of "brcm,completion-timeout-us" so that
> >       it refers to PCIe transactions. (StefanW)
> >    -- Remvove mention of Linux specifics in binding description. (StefanW)
> >    -- s/clkreq#/CLKREQ#/g (Bjorn)
> >    -- Refactor completion-timeout-us code to compare max and min to
> >       value given by the property (as opposed to the computed value).
> >
> > v1 -- The current driver assumes the downstream devices can
> >       provide CLKREQ# for ASPM.  These commits accomodate devices
> >       w/ or w/o clkreq# and also handle L1SS-capable devices.
> >
> >    -- The Raspian Linux folks have already been using a PCIe RC
> >       property "brcm,enable-l1ss".  These commits use the same
> >       property, in a backward-compatible manner, and the implementaion
> >       adds more detail and also automatically identifies devices w/o
> >       a clkreq# signal, i.e. most devices plugged into an RPi CM4
> >       IO board.
> >
> >
> > Jim Quinlan (5):
> >   dt-bindings: PCI: brcmstb: Add brcm,enable-l1ss property
> >   PCI: brcmstb: Configure HW CLKREQ# mode appropriate for downstream
> >     device
>
> I am not merging the first two patches since the discussion thread
> is still open and I'd like to understand better what can/should be
> done, sorry.

Hello Lorenzo,

This patch-set has been stable for months, V5 was out early May and
the V6 changes
did not involve code.  I'm a little surprised that you are voicing
concern at this stage.

The previous discussions covered all aspects of these commits AFAICT.
Please  review
them and the commit messages and let me know what issues you do not understand
or any topics that were not considered.

Are you concerned about the Broadcom STB/CM community  or the RPi community?
For the former, I have direct communication w/ our customers and none of them
are even close to using upstream (they may backport my commits).  For
the latter, I have
tested these commits on the official RPi4 and CM4 IO platforms, and
Cyril has also put in
an admiral amount of testing.

Note that I have on my desk a CM4 IO board w/ a conventional PCIe
device, and it does not boot
upstream master Linux until these patches are applied.

Further, Raspian OS has already introduced the "brcm,enable-l1ss"
property but did not upstream it, and
my commits are backwards compatible with this.

>
> >   PCI: brcmstb: Set higher value for internal bus timeout
> >   PCI: brcmstb: Assert PERST# on BCM2711
> >   PCI: brcmstb: Remove stale comment
>
> Is it OK to apply these three on their own ? Overall it would be
> great to avoid mixing patches with different end goals in a single
> series.

Well, they are related for one customer who wants to use L1SS power
savings AND require
a long  period for the internal timeout.  But, yes, these commits are
fine  to apply
independently.

Regards,
Jim Quinlan
Broadcom STB

>
> Thanks,
> Lorenzo
>
> >  .../bindings/pci/brcm,stb-pcie.yaml           |  9 ++
> >  drivers/pci/controller/pcie-brcmstb.c         | 91 ++++++++++++++++---
> >  2 files changed, 89 insertions(+), 11 deletions(-)
> >
> >
> > base-commit: 8a28a0b6f1a1dcbf5a834600a9acfbe2ba51e5eb
> > --
> > 2.17.1
> >
>
>

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature