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
