On Wed, May 3, 2023 at 6:18 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > > On Wed, May 03, 2023 at 05:38:15PM -0400, Jim Quinlan wrote: > > 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. > > Non-L1SS behavior is a subset of L1SS, so if you've tested with L1SS > enabled, I would think you'd be covered. > > But I'm not a hardware engineer, so maybe there's some subtlety there. > The "asking for trouble" comment from your engineer is definitely > concerning, but I have no idea what's behind that. > > And obviously even if we have "brcm,enable-l1ss", the user may decide > to disable L1SS administratively, so even if the Root Port and the > device both support L1SS, it may be never be enabled. > > > 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. > > They prefer editing the DT? > > I agree the "pci=B:D.F" stuff is a bit ugly. Do you have multiple > slots such that you would have to apply this parameter to some but not > others? I guess I was imagining a single-slot system where you > wouldn't need to identify the specific device because there *is* only > one. Hi Bjorn, We typically have a single device per controller. Occasionally, there is a mismatch in needs, and the customer adds a switch to their board until we can add another controller to the next rev of the SOC. Some of our customers have a habit of doing "rmmod, sleep, insmod" on the RC driver for various uncommon reasons, so "pci,linux-domain" was quite useful for them to simplify their shell scripts. As far as preferring DT: customers have to modify the DT already*, so they really don't want to be modifying two separate configurations (DT and boot params). Often, the DT blob is stored in a different partition or medium than the bootline params, and it is a hassle to configure both and keep them in "sync". Regards, Jim Quinlan Broadcom STB * We have a tool system that we and our customers use which takes a high-level configuration file and generates a custom DT blob and bootloader for a particular SOC/board(s). And we provide the default config, so our customers only have to change a few things. For example, adding "-l1ss" to the existing "pcie -n 0" line will do what you'd expect. And this is actually not a good example of the tool's power. > > Bjorn
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
