Re: [PATCH v4 1/5] dt-bindings: PCI: brcmstb: brcm,{enable-l1ss,completion-timeout-us} props

[Jim Quinlan]

On Wed, May 3, 2023 at 2:07 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> On Wed, May 03, 2023 at 10:38:57AM -0400, Jim Quinlan wrote:
> > On Sun, Apr 30, 2023 at 3:10 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> > > On Fri, Apr 28, 2023 at 06:34:55PM -0400, Jim Quinlan wrote:
> > > > brcm,enable-l1ss (bool):
> > > >
> > > >   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).  All devices
> > > >   should work fine when the driver chooses (a) or (b), but (c) may be
> > > >   desired to realize the extra power savings that L1SS offers.  So we
> > > >   introduce the boolean "brcm,enable-l1ss" property to inform the driver
> > > >   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).
> > ...
>
> > > What bad things would happen if the driver always configured (c)?
> >
> > Well, our driver has traditionally only supported (b) and our
> > existing boards have been designed with this in mind.  I would not
> > want to switch modes w'o the user/customer/engineer opting-in to do
> > so.  Further, the PCIe HW engineer told me defaulting to (c) was a
> > bad idea and was "asking for trouble".  Note that the commit's
> > comment has that warning about L1SS mode not meeting this 400ns
> > spec, and I suspect that many of our existing designs have bumped
> > into that.
> >
> > But to answer your question, I haven't found a scenario that did not
> > work by setting mode (c).  That doesn't mean they are not out there.
> >
> > > Other platforms don't require this, and having to edit the DT
> > > based on what PCIe device is plugged in seems wrong.  If brcmstb
> > > does need it, that suggests a hardware defect.  If we need this to
> > > work around a defect, that's OK, but we should acknowledge the
> > > defect so we can stop using this for future hardware that doesn't
> > > need it.
> >
> > All devices should work w/o the user having to change the DT.  Only
> > if they desire L1SS must they add the "brcm,enable-l1ss" property.
>
> I thought the DT was supposed to describe properties of the
> *hardware*, but this seems more like "use this untested clkreq
> configuration," which maybe could be done via a module parameter?
Electrically, it has been tested, but  specifically for L1SS capable
devices.  What is untested AFAICT are platforms using this mode on
non-L1SS capable
devices.  I was not aware that Raspian OS was turning this on as a
default until the CM4 came out.

As far as "DT describing the HW only", one doesn't have to go far to
find exceptions to the rule.
One example off the top of my head is "linux,pci-domain" -- all this
does is assign
an "id" to a controller to make life easier.  We've gone from not
using it, with three controllers no less,
to using it, but the HW was the same all along.

WRT bootline param
pci=[<domain>:]<bus>:<dev>.<func>[/<dev>.<func>]*pci:<vendor>:<device>[:<subvendor>:<subdevice>]:
this does not look compatible for vendor specific DT options like
"brcm,enable-l1ss".  I observe that pci_dev_str_match_path() is a
static function and I don't see a single option in pci.c  that is
vendor specific.  FWIW, moving something like this to the bootline
would not be popular with our customers; for some reason they really
don't like changes to the bootline.


>
> Whatever the mechanism, it looks like patch 2/5 makes brcmstb
> advertise the appropriate ASPM and L1SS stuff in the PCIe and L1SS
> Capabilities so the OS will do the right thing without any core
> changes.
>
> > > Maybe the name should be more specific to CLKREQ#, since this
> > > doesn't actually *enable* L1SS; apparently it's just one of the
> > > pieces needed to enable L1SS?
> >
> > The other pieces are:  (a) policy == POWERSUPERSAVE and (b) an
> > L1SS-capable device, which seem unrelated and are out of the scope
> > of the driver.
>
> Right.  Of course, if ASPM and L1SS support are advertised, the OS can
> still choose whether to enable them, and that choice can change at
> run-time.
Agree.

Thanks & regards,
Jim Quinlan
Broadcom STB
>
> > The RPi Raspian folks have been using "brcm,enable-l1ss"  for a
> > while now and I would prefer to keep that name for compatibility.
>
> BTW, the DT comment in the patch refers to PCIe Mini CEM .0 sec
> 3.2.5.2.5.  I think the correct section is 3.2.5.2.2 (at least in the
> r2.1 spec).
>
> There's also a footnote to the effect that T_CRLon is allowed to
> exceed 400ns when LTR is supported and enabled.  L1.2 requires LTR, so
> if L1.2 is the case where brcmstb exceeds 400ns, that might not be a
> problem.
>
> Bjorn

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