| Message ID | 20210401212148.47033-3-jim2101024@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: brcmstb: add slot0 device regulators and panic handler | expand |
On Thu, Apr 01, 2021 at 05:21:42PM -0400, Jim Quinlan wrote: > Similar to the regulator bindings found in "rockchip-pcie-host.txt", this > allows optional regulators to be attached and controlled by the PCIe RC > driver. That being said, this driver searches in the DT subnode (the EP > node, eg pci@0,0) for the regulator property. > The use of a regulator property in the pcie EP subnode such as > "vpcie12v-supply" depends on a pending pullreq to the pci-bus.yaml > file at > > https://github.com/devicetree-org/dt-schema/pull/54 > > Signed-off-by: Jim Quinlan <jim2101024@gmail.com> > --- > Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml > index f90557f6deb8..f2caa5b3b281 100644 > --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml > +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml > @@ -64,6 +64,9 @@ properties: > > aspm-no-l0s: true > > + vpcie12v-supply: true > + vpcie3v3-supply: true > + No great problem with having these in the controller node (assming it accurately describes the hardware) but I do think we ought to also be able to describe these per slot.
On Tue, Apr 6, 2021 at 12:47 PM Mark Brown <broonie@kernel.org> wrote: > > On Thu, Apr 01, 2021 at 05:21:42PM -0400, Jim Quinlan wrote: > > Similar to the regulator bindings found in "rockchip-pcie-host.txt", this > > allows optional regulators to be attached and controlled by the PCIe RC > > driver. That being said, this driver searches in the DT subnode (the EP > > node, eg pci@0,0) for the regulator property. > > > The use of a regulator property in the pcie EP subnode such as > > "vpcie12v-supply" depends on a pending pullreq to the pci-bus.yaml > > file at > > > > https://github.com/devicetree-org/dt-schema/pull/54 > > > > Signed-off-by: Jim Quinlan <jim2101024@gmail.com> > > --- > > Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml > > index f90557f6deb8..f2caa5b3b281 100644 > > --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml > > +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml > > @@ -64,6 +64,9 @@ properties: > > > > aspm-no-l0s: true > > > > + vpcie12v-supply: true > > + vpcie3v3-supply: true > > + > > No great problem with having these in the controller node (assming it > accurately describes the hardware) but I do think we ought to also be > able to describe these per slot. Hi Mark, Can you explain what you think that would look like in the DT? Thanks, Jim Quinlan Broadcom STB
On Tue, Apr 06, 2021 at 01:26:51PM -0400, Jim Quinlan wrote: > On Tue, Apr 6, 2021 at 12:47 PM Mark Brown <broonie@kernel.org> wrote: > > No great problem with having these in the controller node (assming it > > accurately describes the hardware) but I do think we ought to also be > > able to describe these per slot. > Can you explain what you think that would look like in the DT? I *think* that's just some properties on the nodes for the endpoints, note that the driver could just ignore them for now. Not sure where or if we document any extensions but child nodes are in section 4 of the v2.1 PCI bus binding.
On Tue, Apr 6, 2021 at 1:32 PM Mark Brown <broonie@kernel.org> wrote: > > On Tue, Apr 06, 2021 at 01:26:51PM -0400, Jim Quinlan wrote: > > On Tue, Apr 6, 2021 at 12:47 PM Mark Brown <broonie@kernel.org> wrote: > > > > No great problem with having these in the controller node (assming it > > > accurately describes the hardware) but I do think we ought to also be > > > able to describe these per slot. > > > Can you explain what you think that would look like in the DT? > > I *think* that's just some properties on the nodes for the endpoints, > note that the driver could just ignore them for now. Not sure where or > if we document any extensions but child nodes are in section 4 of the > v2.1 PCI bus binding. Hi Mark, I'm a little confused -- here is how I remember the chronology of the "DT bindings" commit reviews, please correct me if I'm wrong: o JimQ submitted a pullreq for using voltage regulators in the same style as the existing "rockport" PCIe driver. o After some deliberation, RobH preferred that the voltage regulators should go into the PCIe subnode device's DT node. o JimQ put the voltage regulators in the subnode device's DT node. o MarkB didn't like the fact that the code did a global search for the regulator since it could not provide the owning struct device* handle. o RobH relented, and said that if it is just two specific and standard voltage regulators, perhaps they can go in the parent DT node after all. o JimQ put the regulators back in the PCIe node. o MarkB now wants the regulators to go back into the child node again? Folks, please advise. Regards, Jim Quinlan Broadcom STB
On Tue, Apr 06, 2021 at 02:25:49PM -0400, Jim Quinlan wrote: > I'm a little confused -- here is how I remember the chronology of the > "DT bindings" commit reviews, please correct me if I'm wrong: > o JimQ submitted a pullreq for using voltage regulators in the same > style as the existing "rockport" PCIe driver. > o After some deliberation, RobH preferred that the voltage regulators > should go into the PCIe subnode device's DT node. > o JimQ put the voltage regulators in the subnode device's DT node. > o MarkB didn't like the fact that the code did a global search for the > regulator since it could not provide the owning struct device* handle. > o RobH relented, and said that if it is just two specific and standard > voltage regulators, perhaps they can go in the parent DT node after > all. > o JimQ put the regulators back in the PCIe node. > o MarkB now wants the regulators to go back into the child node again? ...having pointed out a couple of times now that there's no physical requirement that the supplies be shared between slots never mind with the controller. Also note that as I've said depending on what the actual requirements of the controller node are you might want to have the regulators in both places, and further note that the driver does not have to actively use everything in the binding document (although if it's not using something that turns out to be a requirement it's likely to run into hardware where that causes bugs at some point). Frankly I'm not clear why you're trying to handle powering on PCI slots in a specific driver, surely PCI devices are PCI devices regardless of the controller?
On 4/7/2021 4:27 AM, Mark Brown wrote: > On Tue, Apr 06, 2021 at 02:25:49PM -0400, Jim Quinlan wrote: > >> I'm a little confused -- here is how I remember the chronology of the >> "DT bindings" commit reviews, please correct me if I'm wrong: > >> o JimQ submitted a pullreq for using voltage regulators in the same >> style as the existing "rockport" PCIe driver. >> o After some deliberation, RobH preferred that the voltage regulators >> should go into the PCIe subnode device's DT node. >> o JimQ put the voltage regulators in the subnode device's DT node. >> o MarkB didn't like the fact that the code did a global search for the >> regulator since it could not provide the owning struct device* handle. >> o RobH relented, and said that if it is just two specific and standard >> voltage regulators, perhaps they can go in the parent DT node after >> all. >> o JimQ put the regulators back in the PCIe node. >> o MarkB now wants the regulators to go back into the child node again? > > ...having pointed out a couple of times now that there's no physical > requirement that the supplies be shared between slots never mind with > the controller. Also note that as I've said depending on what the > actual requirements of the controller node are you might want to have > the regulators in both places, and further note that the driver does not > have to actively use everything in the binding document (although if > it's not using something that turns out to be a requirement it's likely > to run into hardware where that causes bugs at some point). > > Frankly I'm not clear why you're trying to handle powering on PCI slots > in a specific driver, surely PCI devices are PCI devices regardless of > the controller? There is no currently a way to deal with that situation since you have a chicken and egg problem to solve: there is no pci_device created until you enumerate the PCI bus, and you cannot enumerate the bus and create those pci_devices unless you power on the slots/PCIe end-points attached to the root complex. There are precedents like the rockchip PCIe RC driver, and yes, we should not be cargo culting this, which is why we are trying to understand what is it that should be done here and there has been conflicting advice, or rather our interpretation has led to perceiving it as a conflicting. If the regulator had a variation where it supported passing a device_node reference to look up regulator properties, we could solve this generically for all devices, that does not exist, and you stated you will not accept changes like those, fair enough. When you suggested to look at soundwire for an example of "software devices", we did that but it was not clear where the sdw_slave would be created prior to sdw_slave_add(), but this does not matter too much. Let us say we try to solve this generically using the same idea that we would pre-populate pci_device prior to calling pci_scan_child_bus(). We could do something along these lines: - pci_scan_child_bus() would attempt to walk the list of device_node children from the PCIe root complex's device_node and call pci_alloc_dev() for each of these devices that it finds, along with calling device_initialize() and ensuring that pci_dev::device::of_node is set correctly by calling pci_set_of_node()/set_dev_node(). Finally we call list_add_tail() with the pci_bus_sem semaphore held to add that pci_device to bus->devices such that we can later find them - from there on we try to get the regulators of those pci_device objects we just allocated and we try to enable their regulators, either based on a specific list of supplies or just trying to enable all supplied declared. - now pci_scan_child_bus() will attempt to scan the bus for real by reading the pci_device objects ID but we already have such objects, so we need to update pci_scan_device() to search bus::devices and if nothing is found we allocate one Is that roughly what you have in mind as to what should be done?
On Wed, Apr 07, 2021 at 11:35:57AM -0700, Florian Fainelli wrote: > On 4/7/2021 4:27 AM, Mark Brown wrote: > > Frankly I'm not clear why you're trying to handle powering on PCI slots > > in a specific driver, surely PCI devices are PCI devices regardless of > > the controller? > There is no currently a way to deal with that situation since you have a > chicken and egg problem to solve: there is no pci_device created until > you enumerate the PCI bus, and you cannot enumerate the bus and create > those pci_devices unless you power on the slots/PCIe end-points attached > to the root complex. There are precedents like the rockchip PCIe RC > driver, and yes, we should not be cargo culting this, which is why we > are trying to understand what is it that should be done here and there > has been conflicting advice, or rather our interpretation has led to > perceiving it as a conflicting. As you note below I've pointed you at Slimbus which has a similar problem and solves it at a bus level although it thought of this from day one which makes life easier; I do think it'd be good to get this stuff in the driver core since it's an issue that affects every enumerable bus in the end but nobody's summoned up the enthusiasm for that (including me). > If the regulator had a variation where it supported passing a > device_node reference to look up regulator properties, we could solve > this generically for all devices, that does not exist, and you stated > you will not accept changes like those, fair enough. I'm certainly not enthusiastic about the idea and the likely abuse isn't inspiring, and of course regulators aren't the only resource that might be needed to get something up and running and would need to be extended in the end. That said I've not seen any concrete proposals either. > When you suggested to look at soundwire for an example of "software > devices", we did that but it was not clear where the sdw_slave would be > created prior to sdw_slave_add(), but this does not matter too much. Looks like sdw_acpi_find_slaves() and sdw_of_find_slaves(). > Let us say we try to solve this generically using the same idea that we > would pre-populate pci_device prior to calling pci_scan_child_bus(). We > could do something along these lines: ... > - from there on we try to get the regulators of those pci_device objects > we just allocated and we try to enable their regulators, either based on > a specific list of supplies or just trying to enable all supplied declared. I'd suggest specfying the supplies that PCI provides to slots in the spec with standard names and just using that list, at least as a start. That'd probably cover most cases and allow the binding to be written at the generic PCI level rather than having individual devices need to name their supplies for the binding documentation and validation stuff which seems easier. Devices with extra stuff can always extend the binding. > - now pci_scan_child_bus() will attempt to scan the bus for real by > reading the pci_device objects ID but we already have such objects, so > we need to update pci_scan_device() to search bus::devices and if > nothing is found we allocate one > Is that roughly what you have in mind as to what should be done? Yes, pretty much. Ideally there'd be some way for drivers to get a callback prior to enumeration to handle any custom stuff for embedded cases but unless someone actually has a use case for that you could just punt.
On Tue, Apr 06, 2021 at 02:25:49PM -0400, Jim Quinlan wrote: > On Tue, Apr 6, 2021 at 1:32 PM Mark Brown <broonie@kernel.org> wrote: > > > > On Tue, Apr 06, 2021 at 01:26:51PM -0400, Jim Quinlan wrote: > > > On Tue, Apr 6, 2021 at 12:47 PM Mark Brown <broonie@kernel.org> wrote: > > > > > > No great problem with having these in the controller node (assming it > > > > accurately describes the hardware) but I do think we ought to also be > > > > able to describe these per slot. PCIe is effectively point to point, so there's only 1 slot unless there's a PCIe switch in the middle. If that's the case, then it's all more complicated. > > > Can you explain what you think that would look like in the DT? > > > > I *think* that's just some properties on the nodes for the endpoints, > > note that the driver could just ignore them for now. Not sure where or > > if we document any extensions but child nodes are in section 4 of the > > v2.1 PCI bus binding. > > Hi Mark, > > I'm a little confused -- here is how I remember the chronology of the > "DT bindings" commit reviews, please correct me if I'm wrong: > > o JimQ submitted a pullreq for using voltage regulators in the same > style as the existing "rockport" PCIe driver. > o After some deliberation, RobH preferred that the voltage regulators > should go into the PCIe subnode device's DT node. IIRC, that's because you said there isn't a standard slot. > o JimQ put the voltage regulators in the subnode device's DT node. > o MarkB didn't like the fact that the code did a global search for the > regulator since it could not provide the owning struct device* handle. > o RobH relented, and said that if it is just two specific and standard > voltage regulators, perhaps they can go in the parent DT node after > all. > o JimQ put the regulators back in the PCIe node. > o MarkB now wants the regulators to go back into the child node again? > > Folks, please advise. > > Regards, > Jim Quinlan > Broadcom STB
On Thu, Apr 08, 2021 at 11:20:16AM -0500, Rob Herring wrote: > On Tue, Apr 06, 2021 at 02:25:49PM -0400, Jim Quinlan wrote: > > On Tue, Apr 6, 2021 at 1:32 PM Mark Brown <broonie@kernel.org> wrote: > > > > On Tue, Apr 6, 2021 at 12:47 PM Mark Brown <broonie@kernel.org> wrote: > > > > > No great problem with having these in the controller node (assming it > > > > > accurately describes the hardware) but I do think we ought to also be > > > > > able to describe these per slot. > PCIe is effectively point to point, so there's only 1 slot unless > there's a PCIe switch in the middle. If that's the case, then it's all > more complicated. So even for PCIe that case exists, and it's not entirely clear to me that this is all definitively PCIe specific. > > o After some deliberation, RobH preferred that the voltage regulators > > should go into the PCIe subnode device's DT node. > IIRC, that's because you said there isn't a standard slot. I recall some message in the thread that said both cases exist even with this specific system.
On Thu, Apr 8, 2021 at 12:20 PM Rob Herring <robh@kernel.org> wrote: > > On Tue, Apr 06, 2021 at 02:25:49PM -0400, Jim Quinlan wrote: > > On Tue, Apr 6, 2021 at 1:32 PM Mark Brown <broonie@kernel.org> wrote: > > > > > > On Tue, Apr 06, 2021 at 01:26:51PM -0400, Jim Quinlan wrote: > > > > On Tue, Apr 6, 2021 at 12:47 PM Mark Brown <broonie@kernel.org> wrote: > > > > > > > > No great problem with having these in the controller node (assming it > > > > > accurately describes the hardware) but I do think we ought to also be > > > > > able to describe these per slot. > > PCIe is effectively point to point, so there's only 1 slot unless > there's a PCIe switch in the middle. If that's the case, then it's all > more complicated. > > > > > Can you explain what you think that would look like in the DT? > > > > > > I *think* that's just some properties on the nodes for the endpoints, > > > note that the driver could just ignore them for now. Not sure where or > > > if we document any extensions but child nodes are in section 4 of the > > > v2.1 PCI bus binding. > > > > Hi Mark, > > > > I'm a little confused -- here is how I remember the chronology of the > > "DT bindings" commit reviews, please correct me if I'm wrong: > > > > o JimQ submitted a pullreq for using voltage regulators in the same > > style as the existing "rockport" PCIe driver. > > o After some deliberation, RobH preferred that the voltage regulators > > should go into the PCIe subnode device's DT node. > > IIRC, that's because you said there isn't a standard slot. Admittedly, I'm not exactly sure what you mean by a "standard slot". Our PCIIe HW does not support hotplug or have a presence bit or anything like that. Our root complex has one port; it can only directly connect to a single switch or endpoint. This connection shows up as slot0. The voltage regulator(s) involved depend on a GPIO that turns the power on/off for the connected device/chip. The gpio pin can vary from board to board but this is easily handled in our DT. Some boards have regulators that are always on and not associated with a GPIO pin -- these have no representation in our DT. Regards, Jim > > > o JimQ put the voltage regulators in the subnode device's DT node. > > o MarkB didn't like the fact that the code did a global search for the > > regulator since it could not provide the owning struct device* handle. > > o RobH relented, and said that if it is just two specific and standard > > voltage regulators, perhaps they can go in the parent DT node after > > all. > > o JimQ put the regulators back in the PCIe node. > > o MarkB now wants the regulators to go back into the child node again? > > > > Folks, please advise. > > > > Regards, > > Jim Quinlan > > Broadcom STB
On Tue, Apr 06, 2021 at 05:47:08PM +0100, Mark Brown wrote: > On Thu, Apr 01, 2021 at 05:21:42PM -0400, Jim Quinlan wrote: > > Similar to the regulator bindings found in "rockchip-pcie-host.txt", this > > allows optional regulators to be attached and controlled by the PCIe RC > > driver. That being said, this driver searches in the DT subnode (the EP > > node, eg pci@0,0) for the regulator property. > > > The use of a regulator property in the pcie EP subnode such as > > "vpcie12v-supply" depends on a pending pullreq to the pci-bus.yaml > > file at > > > > https://github.com/devicetree-org/dt-schema/pull/54 > > > > Signed-off-by: Jim Quinlan <jim2101024@gmail.com> > > --- > > Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml > > index f90557f6deb8..f2caa5b3b281 100644 > > --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml > > +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml > > @@ -64,6 +64,9 @@ properties: > > > > aspm-no-l0s: true > > > > + vpcie12v-supply: true > > + vpcie3v3-supply: true > > + > > No great problem with having these in the controller node (assming it > accurately describes the hardware) but I do think we ought to also be > able to describe these per slot. My hesistancy here is child nodes are already defined to be devices, not slots. That's generally the same thing though. However, 'reg' is a bit problematic as it includes the bus number which is dynamically assigned. (This is a mismatch between true OpenFirmware and FDT as OF was designed to populate the DT with what was discovered and that includes some runtime config such as bus number assignments.) Maybe we just say for FDT, the bus number is always 0 and ignored. In any case, there needs to be some thought on this as well as the more complicated case of devices needing device specific setup to be enumerated. Rob
On Thu, Apr 08, 2021 at 12:58:05PM -0400, Jim Quinlan wrote: > On Thu, Apr 8, 2021 at 12:20 PM Rob Herring <robh@kernel.org> wrote: > > > > On Tue, Apr 06, 2021 at 02:25:49PM -0400, Jim Quinlan wrote: > > > On Tue, Apr 6, 2021 at 1:32 PM Mark Brown <broonie@kernel.org> wrote: > > > > > > > > On Tue, Apr 06, 2021 at 01:26:51PM -0400, Jim Quinlan wrote: > > > > > On Tue, Apr 6, 2021 at 12:47 PM Mark Brown <broonie@kernel.org> wrote: > > > > > > > > > > No great problem with having these in the controller node (assming it > > > > > > accurately describes the hardware) but I do think we ought to also be > > > > > > able to describe these per slot. > > > > PCIe is effectively point to point, so there's only 1 slot unless > > there's a PCIe switch in the middle. If that's the case, then it's all > > more complicated. > > > > > > > Can you explain what you think that would look like in the DT? > > > > > > > > I *think* that's just some properties on the nodes for the endpoints, > > > > note that the driver could just ignore them for now. Not sure where or > > > > if we document any extensions but child nodes are in section 4 of the > > > > v2.1 PCI bus binding. > > > > > > Hi Mark, > > > > > > I'm a little confused -- here is how I remember the chronology of the > > > "DT bindings" commit reviews, please correct me if I'm wrong: > > > > > > o JimQ submitted a pullreq for using voltage regulators in the same > > > style as the existing "rockport" PCIe driver. > > > o After some deliberation, RobH preferred that the voltage regulators > > > should go into the PCIe subnode device's DT node. > > > > IIRC, that's because you said there isn't a standard slot. > Admittedly, I'm not exactly sure what you mean by a "standard slot". > Our PCIIe HW does not support hotplug or have a presence bit or > anything like that. Our root complex has one port; it can only > directly connect to a single switch or endpoint. This connection shows > up as slot0. The voltage regulator(s) involved depend on a GPIO that > turns the power on/off for the connected device/chip. The gpio pin > can vary from board to board but this is easily handled in our DT. > Some boards have regulators that are always on and not associated with > a GPIO pin -- these have no representation in our DT. By standard slot, I mean you have standard voltage rails 12V and 3.3V (or 1.5 and 3.3 for mini PCIe) and PERST# signal, no other extra things to make a device discoverable, and the timing for those rails and PERST# follow what the spec defines. There's also CLKREQ, WAKE, and hotplug detect signals, but I think those are all optional and could be tied off. I think most PCI h/w is not hotplug capable. Rob
diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml index f90557f6deb8..f2caa5b3b281 100644 --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml @@ -64,6 +64,9 @@ properties: aspm-no-l0s: true + vpcie12v-supply: true + vpcie3v3-supply: true + brcm,scb-sizes: description: u64 giving the 64bit PCIe memory viewport size of a memory controller. There may be up to @@ -156,5 +159,6 @@ examples: <0x42000000 0x1 0x80000000 0x3 0x00000000 0x0 0x80000000>; brcm,enable-ssc; brcm,scb-sizes = <0x0000000080000000 0x0000000080000000>; + vpcie12v-supply = <&vreg12>; }; };
Similar to the regulator bindings found in "rockchip-pcie-host.txt", this allows optional regulators to be attached and controlled by the PCIe RC driver. That being said, this driver searches in the DT subnode (the EP node, eg pci@0,0) for the regulator property. The use of a regulator property in the pcie EP subnode such as "vpcie12v-supply" depends on a pending pullreq to the pci-bus.yaml file at https://github.com/devicetree-org/dt-schema/pull/54 Signed-off-by: Jim Quinlan <jim2101024@gmail.com> --- Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml | 4 ++++ 1 file changed, 4 insertions(+)