From: Jim Quinlan <james.quinlan@broadcom.com> To: Bjorn Helgaas <helgaas@kernel.org> Cc: "Jim Quinlan" <jim2101024@gmail.com>, linux-pci@vger.kernel.org, "Nicolas Saenz Julienne" <nsaenz@kernel.org>, "Bjorn Helgaas" <bhelgaas@google.com>, "Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>, "Cyril Brulebois" <kibi@debian.org>, "Phil Elwell" <phil@raspberrypi.com>, bcm-kernel-feedback-list@broadcom.com, "Florian Fainelli" <f.fainelli@gmail.com>, "Lorenzo Pieralisi" <lpieralisi@kernel.org>, "Krzysztof Wilczyński" <kw@linux.com>, "Rob Herring" <robh@kernel.org>, "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>, "moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE" <linux-arm-kernel@lists.infradead.org>, "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" <linux-rpi-kernel@lists.infradead.org>, "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" <devicetree@vger.kernel.org>, "open list" <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v4 1/5] dt-bindings: PCI: brcmstb: brcm,{enable-l1ss,completion-timeout-us} props Date: Wed, 3 May 2023 17:38:15 -0400 [thread overview] Message-ID: <CA+-6iNwBCn822NBv1wjLcbd0=bHEfx9V3R_UcAcuMz1_etUjMg@mail.gmail.com> (raw) In-Reply-To: <20230503180728.GA778510@bhelgaas> [-- Attachment #1: Type: text/plain, Size: 5251 bytes --] On Wed, May 3, 2023 at 2:07 PM Bjorn Helgaas <helgaas@kernel.org> 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@kernel.org> 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 #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 4210 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: Jim Quinlan <james.quinlan@broadcom.com> To: Bjorn Helgaas <helgaas@kernel.org> Cc: "Jim Quinlan" <jim2101024@gmail.com>, linux-pci@vger.kernel.org, "Nicolas Saenz Julienne" <nsaenz@kernel.org>, "Bjorn Helgaas" <bhelgaas@google.com>, "Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>, "Cyril Brulebois" <kibi@debian.org>, "Phil Elwell" <phil@raspberrypi.com>, bcm-kernel-feedback-list@broadcom.com, "Florian Fainelli" <f.fainelli@gmail.com>, "Lorenzo Pieralisi" <lpieralisi@kernel.org>, "Krzysztof Wilczyński" <kw@linux.com>, "Rob Herring" <robh@kernel.org>, "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>, "moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE" <linux-arm-kernel@lists.infradead.org>, "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" <linux-rpi-kernel@lists.infradead.org>, "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" <devicetree@vger.kernel.org>, "open list" <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v4 1/5] dt-bindings: PCI: brcmstb: brcm,{enable-l1ss,completion-timeout-us} props Date: Wed, 3 May 2023 17:38:15 -0400 [thread overview] Message-ID: <CA+-6iNwBCn822NBv1wjLcbd0=bHEfx9V3R_UcAcuMz1_etUjMg@mail.gmail.com> (raw) In-Reply-To: <20230503180728.GA778510@bhelgaas> [-- Attachment #1.1: Type: text/plain, Size: 5251 bytes --] On Wed, May 3, 2023 at 2:07 PM Bjorn Helgaas <helgaas@kernel.org> 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@kernel.org> 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 #1.2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 4210 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2023-05-03 21:38 UTC|newest] Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-04-28 22:34 [PATCH v4 0/5] PCI: brcmstb: Configure appropriate HW CLKREQ# mode Jim Quinlan 2023-04-28 22:34 ` Jim Quinlan 2023-04-28 22:34 ` [PATCH v4 1/5] dt-bindings: PCI: brcmstb: brcm,{enable-l1ss,completion-timeout-us} props Jim Quinlan 2023-04-28 22:34 ` Jim Quinlan 2023-04-30 19:10 ` Bjorn Helgaas 2023-04-30 19:10 ` Bjorn Helgaas 2023-05-03 14:38 ` Jim Quinlan 2023-05-03 14:38 ` Jim Quinlan 2023-05-03 18:07 ` Bjorn Helgaas 2023-05-03 18:07 ` Bjorn Helgaas 2023-05-03 21:38 ` Jim Quinlan [this message] 2023-05-03 21:38 ` Jim Quinlan 2023-05-03 22:18 ` Bjorn Helgaas 2023-05-03 22:18 ` Bjorn Helgaas 2023-05-05 12:39 ` Jim Quinlan 2023-05-05 12:39 ` Jim Quinlan 2023-05-05 13:34 ` Bjorn Helgaas 2023-05-05 13:34 ` Bjorn Helgaas 2023-05-05 14:40 ` Jim Quinlan 2023-05-05 14:40 ` Jim Quinlan 2023-05-05 14:54 ` Bjorn Helgaas 2023-05-05 14:54 ` Bjorn Helgaas 2023-04-28 22:34 ` [PATCH v4 2/5] PCI: brcmstb: Configure HW CLKREQ# mode appropriate for downstream device Jim Quinlan 2023-04-28 22:34 ` Jim Quinlan 2023-05-03 6:09 ` Stefan Wahren 2023-05-03 6:09 ` Stefan Wahren 2023-04-28 22:34 ` [PATCH v4 3/5] PCI: brcmstb: Set PCIe transaction completion timeout Jim Quinlan 2023-04-28 22:34 ` Jim Quinlan 2023-04-30 19:13 ` Bjorn Helgaas 2023-04-30 19:13 ` Bjorn Helgaas 2023-04-30 21:24 ` Jim Quinlan 2023-04-30 21:24 ` Jim Quinlan 2023-04-30 22:38 ` Bjorn Helgaas 2023-04-30 22:38 ` Bjorn Helgaas 2023-05-01 20:55 ` Lukas Wunner 2023-05-03 14:06 ` Jim Quinlan 2023-05-03 14:06 ` Jim Quinlan 2023-05-03 6:06 ` Stefan Wahren 2023-05-03 6:06 ` Stefan Wahren 2023-04-28 22:34 ` [PATCH v4 4/5] PCI: brcmstb: Don't assume 2711 bootloader leaves PERST# asserted Jim Quinlan 2023-04-28 22:34 ` Jim Quinlan 2023-04-28 22:34 ` [PATCH v4 5/5] PCI: brcmstb: Remove stale comment Jim Quinlan 2023-04-28 22:34 ` Jim Quinlan 2023-05-02 23:15 ` [PATCH v4 0/5] PCI: brcmstb: Configure appropriate HW CLKREQ# mode Cyril Brulebois 2023-05-02 23:15 ` Cyril Brulebois 2023-05-03 18:10 ` Jim Quinlan 2023-05-03 18:10 ` Jim Quinlan 2023-05-03 19:10 ` Cyril Brulebois 2023-05-03 19:10 ` Cyril Brulebois
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CA+-6iNwBCn822NBv1wjLcbd0=bHEfx9V3R_UcAcuMz1_etUjMg@mail.gmail.com' \ --to=james.quinlan@broadcom.com \ --cc=bcm-kernel-feedback-list@broadcom.com \ --cc=bhelgaas@google.com \ --cc=devicetree@vger.kernel.org \ --cc=f.fainelli@gmail.com \ --cc=helgaas@kernel.org \ --cc=jim2101024@gmail.com \ --cc=kibi@debian.org \ --cc=krzysztof.kozlowski+dt@linaro.org \ --cc=kw@linux.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pci@vger.kernel.org \ --cc=linux-rpi-kernel@lists.infradead.org \ --cc=lorenzo.pieralisi@arm.com \ --cc=lpieralisi@kernel.org \ --cc=nsaenz@kernel.org \ --cc=phil@raspberrypi.com \ --cc=robh@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.