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? 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. > 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
