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

[Jim Quinlan]

On Thu, Apr 6, 2023 at 3:09 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> On Thu, Apr 06, 2023 at 08:46:23AM -0400, Jim Quinlan wrote:
> > The Broadcom STB/CM PCIe HW core, which is also used in RPi SOCs, may be
> > set into three mutually exclusive modes:
> >
> >   (a) No clkreq# expected or required, refclk is always available.
> >   (b) Clkreq# is expected to be driven by downstream device when needed.
> >   (c) Bidirectional clkreq# for L1SS capable devices.
> >
> > Previously, only (b) was supported by the driver, as almost all STB boards
> > operate in this mode.  But now there is interest in activating L1SS power
> > savings from STB customers, and also interest in accomodating mode (a) for
> > designs such as the RPi CM4 with IO board.
> >
> > The HW can tell us when mode (a) mode is needed.  But there is no easy way
> > to tell if L1SS mode is needed.  Unfortunately, getting this wrong causes a
> > panic during boot time.  So we rely on the DT prop "brcm,enable-l1ss" to
> > tell us when mode (c) is desired.  This property has already been in
> > use by Raspian Linux, but this immplementation adds more details and
> > discerns between (a) and (b) automatically.
> >
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217276
> > Signed-off-by: Jim Quinlan <jim2101024@xxxxxxxxx>
>
> > +      * We have "seen" clkreq# if it is asserted or has been in the past.
> > +      * Note that the CLKREQ_IN_MASK is 1 if clkreq# is asserted.
>
> "CLKREQ#" to match PCIe spec and comments below.

Will do.
>
> > +     if (l1ss && IS_ENABLED(CONFIG_PCIEASPM)) {
> > +             /*
> > +              * Note: This (l1ss) mode may not meet requirement for
> > +              * Endpoints that require CLKREQ# assertion to clock active
> > +              * within 400ns.
> > +              */
> > +             clkreq_set |= PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABLE_MASK;
> > +             dev_info(pcie->dev, "bi-dir clkreq; l1ss-capable devs only\n");
> > +             dev_info(pcie->dev, "ASPM policy must be set to powersupersave\n");
>
> Seems problematic since L1SS can be enabled/disabled at run-time:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing/sysfs-bus-pci?id=v6.2#n420

Yes it is problematic.  AFAICT there are no notifier chain for
changing modes; you probably don't want me to add one and neither do I
:-).
Our HW should shift gears when there is a change the standard L1SS
control field, but it does not -- more on this below.

>
> The simplistic answer is to advertise L1SS support if and only if you
> can safely support it.
We can only safely support it if we know a priori that (a) the
downstream device is ASPM capable and (b) the policy is set to
"power_supersave" and will not be changed.

For (a), I thought about having the RC probe "peek" at the downstream
devices capabilities, but that would require it to go through the
capability linked-list.  I have a feeling that would not be approved
by you folks.

For (b), there is no way to know at RC probe-time that the policy is
going to be "power_supersave".  This calculation happens after the RC
probe exits, and besides, pcie_aspm_get_policy() is a static function.
And as you mentioned, the ASPM policy may be changed at runtime.
That is the reason for the "brcm,enable-l1ss" property.

>
> I don't know why this is an issue for this device but not others.  Is
> it because there's some problem in the way the board is designed?  Or
> (after skimming the bugzilla) maybe a problem with the plug-in cards?
FWIW,  it's  not clear that all of the devices for drivers in
drivers/pci/controller/  support L1SS -- our driver had no mention of
ti until now.

However, I asked the PCIe HW design engineer a question similar to
yours; i.e. from looking at all of the drivers' code  as well as
pcie/asmp.c,  there doesn't seem to be any issue wrt  seamlessly
switching between uni-dir and bi-dir CLKREQ# in response to ASPM
control settings.  He just answered something on the lines that for
this design, one has to make a deliberate choice of L1SS or !L1SS and
stick with it.

Regards,
Jim Quinlan
Broadcom STB


>
> Bjorn