diff mbox series

[4/9] PCI: create platform devices for child OF nodes of the port node

Message ID 20240117160748.37682-5-brgl@bgdev.pl
State New
Headers show
Series PCI: introduce the concept of power sequencing of PCIe devices | expand

Commit Message

Bartosz Golaszewski Jan. 17, 2024, 4:07 p.m. UTC
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

In order to introduce PCI power-sequencing, we need to create platform
devices for child nodes of the port node. They will get matched against
the pwrseq drivers (if one exists) and then the actual PCI device will
reuse the node once it's detected on the bus.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/pci/bus.c    | 9 ++++++++-
 drivers/pci/remove.c | 3 ++-
 2 files changed, 10 insertions(+), 2 deletions(-)

Comments

Bjorn Andersson Jan. 30, 2024, 9:54 p.m. UTC | #1
On Thu, Jan 18, 2024 at 12:15:27PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Jan 18, 2024 at 11:58:50AM +0100, Bartosz Golaszewski wrote:
> > On Wed, Jan 17, 2024 at 5:45 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Wed, Jan 17, 2024 at 05:07:43PM +0100, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >
> > > > In order to introduce PCI power-sequencing, we need to create platform
> > > > devices for child nodes of the port node.
> > >
> > > Ick, why a platform device?  What is the parent of this device, a PCI
> > > device?  If so, then this can't be a platform device, as that's not what
> > > it is, it's something else so make it a device of that type,.
> > >
> > 
> > Greg,
> > 
> > This is literally what we agreed on at LPC. In fact: during one of the
> > hall track discussions I said that you typically NAK any attempts at
> > using the platform bus for "fake" devices but you responded that this
> > is what the USB on-board HUB does and while it's not pretty, this is
> > what we need to do.
> 
> Ah, you need to remind me of these things, this changelog was pretty
> sparse :)
> 

I believe I missed this part of the discussion, why does this need to be
a platform_device? What does the platform_bus bring that can't be
provided by some other bus?

(I'm not questioning the need for having a bus, creating devices, and
matching/binding them to a set of drivers)

Regards,
Bjorn

> > Now as for the implementation, the way I see it we have two solutions:
> > either we introduce a fake, top-level PCI slot platform device device
> > that will reference the PCI host controller by phandle or we will live
> > with a secondary, "virtual" platform device for power sequencing that
> > is tied to the actual PCI device. The former requires us to add DT
> > bindings, add a totally fake DT node representing the "slot" which
> > doesn't really exist (and Krzysztof already expressed his negative
> > opinion of that) and then have code that will be more complex than it
> > needs to be. The latter allows us to not change DT at all (other than
> > adding regulators, clocks and GPIOs to already existing WLAN nodes),
> > reuse the existing parent-child relationship between the port node and
> > the instantiated platform device as well as result in simpler code.
> > 
> > Given that DT needs to be stable while the underlying C code can
> > freely change if we find a better solution, I think that the second
> > option is a no-brainer here.
> 
> Ok, I remove my objections, sorry about that, my confusion.
> 
> greg k-h
Bartosz Golaszewski Jan. 31, 2024, 11:04 a.m. UTC | #2
On Tue, Jan 30, 2024 at 10:54 PM Bjorn Andersson <andersson@kernel.org> wrote:
>
> On Thu, Jan 18, 2024 at 12:15:27PM +0100, Greg Kroah-Hartman wrote:
> > On Thu, Jan 18, 2024 at 11:58:50AM +0100, Bartosz Golaszewski wrote:
> > > On Wed, Jan 17, 2024 at 5:45 PM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Wed, Jan 17, 2024 at 05:07:43PM +0100, Bartosz Golaszewski wrote:
> > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > >
> > > > > In order to introduce PCI power-sequencing, we need to create platform
> > > > > devices for child nodes of the port node.
> > > >
> > > > Ick, why a platform device?  What is the parent of this device, a PCI
> > > > device?  If so, then this can't be a platform device, as that's not what
> > > > it is, it's something else so make it a device of that type,.
> > > >
> > >
> > > Greg,
> > >
> > > This is literally what we agreed on at LPC. In fact: during one of the
> > > hall track discussions I said that you typically NAK any attempts at
> > > using the platform bus for "fake" devices but you responded that this
> > > is what the USB on-board HUB does and while it's not pretty, this is
> > > what we need to do.
> >
> > Ah, you need to remind me of these things, this changelog was pretty
> > sparse :)
> >
>
> I believe I missed this part of the discussion, why does this need to be
> a platform_device? What does the platform_bus bring that can't be
> provided by some other bus?
>

Does it need to be a platform_device? No, of course not. Does it make
sense for it to be one? Yes, for two reasons:

1. The ATH11K WLAN module is represented on the device tree like a
platform device, we know it's always there and it consumes regulators
from another platform device. The fact it uses PCIe doesn't change the
fact that it is logically a platform device.
2. The platform bus already provides us with the entire infrastructure
that we'd now need to duplicate (possibly adding bugs) in order to
introduce a "power sequencing bus".

Bart

> (I'm not questioning the need for having a bus, creating devices, and
> matching/binding them to a set of drivers)
>
> Regards,
> Bjorn
>

[snip]
Bjorn Andersson Feb. 2, 2024, 12:03 a.m. UTC | #3
On Wed, Jan 31, 2024 at 12:04:14PM +0100, Bartosz Golaszewski wrote:
> On Tue, Jan 30, 2024 at 10:54 PM Bjorn Andersson <andersson@kernel.org> wrote:
> >
> > On Thu, Jan 18, 2024 at 12:15:27PM +0100, Greg Kroah-Hartman wrote:
> > > On Thu, Jan 18, 2024 at 11:58:50AM +0100, Bartosz Golaszewski wrote:
> > > > On Wed, Jan 17, 2024 at 5:45 PM Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Wed, Jan 17, 2024 at 05:07:43PM +0100, Bartosz Golaszewski wrote:
> > > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > > >
> > > > > > In order to introduce PCI power-sequencing, we need to create platform
> > > > > > devices for child nodes of the port node.
> > > > >
> > > > > Ick, why a platform device?  What is the parent of this device, a PCI
> > > > > device?  If so, then this can't be a platform device, as that's not what
> > > > > it is, it's something else so make it a device of that type,.
> > > > >
> > > >
> > > > Greg,
> > > >
> > > > This is literally what we agreed on at LPC. In fact: during one of the
> > > > hall track discussions I said that you typically NAK any attempts at
> > > > using the platform bus for "fake" devices but you responded that this
> > > > is what the USB on-board HUB does and while it's not pretty, this is
> > > > what we need to do.
> > >
> > > Ah, you need to remind me of these things, this changelog was pretty
> > > sparse :)
> > >
> >
> > I believe I missed this part of the discussion, why does this need to be
> > a platform_device? What does the platform_bus bring that can't be
> > provided by some other bus?
> >
> 
> Does it need to be a platform_device? No, of course not. Does it make
> sense for it to be one? Yes, for two reasons:
> 
> 1. The ATH11K WLAN module is represented on the device tree like a
> platform device, we know it's always there and it consumes regulators
> from another platform device. The fact it uses PCIe doesn't change the
> fact that it is logically a platform device.

Are you referring to the ath11k SNOC (firmware running on co-processor
in the SoC) variant?

Afaict the PCIe-attached ath11k is not represented as a platform_device
in DeviceTree.

Said platform_device is also not a child under the PCIe bus, so this
would be a different platform_device...

> 2. The platform bus already provides us with the entire infrastructure
> that we'd now need to duplicate (possibly adding bugs) in order to
> introduce a "power sequencing bus".
> 

This is a perfectly reasonable desire. Look at our PMICs, they are full
of platform_devices. But through the years it's been said many times,
that this is not a valid or good reason for using platform_devices, and
as a result we have e.g. auxiliary bus.

Anyway, (please) don't claim that "we need to", when it actually is "we
want to use platform_device because that's more convenient"!

Regards,
Bjorn

> Bart
> 
> > (I'm not questioning the need for having a bus, creating devices, and
> > matching/binding them to a set of drivers)
> >
> > Regards,
> > Bjorn
> >
> 
> [snip]
Jeff Johnson Feb. 2, 2024, 4:50 a.m. UTC | #4
On 2/1/2024 4:03 PM, Bjorn Andersson wrote:
> On Wed, Jan 31, 2024 at 12:04:14PM +0100, Bartosz Golaszewski wrote:
>> On Tue, Jan 30, 2024 at 10:54 PM Bjorn Andersson <andersson@kernel.org> wrote:
>>>
>>> On Thu, Jan 18, 2024 at 12:15:27PM +0100, Greg Kroah-Hartman wrote:
>>>> On Thu, Jan 18, 2024 at 11:58:50AM +0100, Bartosz Golaszewski wrote:
>>>>> On Wed, Jan 17, 2024 at 5:45 PM Greg Kroah-Hartman
>>>>> <gregkh@linuxfoundation.org> wrote:
>>>>>>
>>>>>> On Wed, Jan 17, 2024 at 05:07:43PM +0100, Bartosz Golaszewski wrote:
>>>>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>>>
>>>>>>> In order to introduce PCI power-sequencing, we need to create platform
>>>>>>> devices for child nodes of the port node.
>>>>>>
>>>>>> Ick, why a platform device?  What is the parent of this device, a PCI
>>>>>> device?  If so, then this can't be a platform device, as that's not what
>>>>>> it is, it's something else so make it a device of that type,.
>>>>>>
>>>>>
>>>>> Greg,
>>>>>
>>>>> This is literally what we agreed on at LPC. In fact: during one of the
>>>>> hall track discussions I said that you typically NAK any attempts at
>>>>> using the platform bus for "fake" devices but you responded that this
>>>>> is what the USB on-board HUB does and while it's not pretty, this is
>>>>> what we need to do.
>>>>
>>>> Ah, you need to remind me of these things, this changelog was pretty
>>>> sparse :)
>>>>
>>>
>>> I believe I missed this part of the discussion, why does this need to be
>>> a platform_device? What does the platform_bus bring that can't be
>>> provided by some other bus?
>>>
>>
>> Does it need to be a platform_device? No, of course not. Does it make
>> sense for it to be one? Yes, for two reasons:
>>
>> 1. The ATH11K WLAN module is represented on the device tree like a
>> platform device, we know it's always there and it consumes regulators
>> from another platform device. The fact it uses PCIe doesn't change the
>> fact that it is logically a platform device.
> 
> Are you referring to the ath11k SNOC (firmware running on co-processor
> in the SoC) variant?
> 
> Afaict the PCIe-attached ath11k is not represented as a platform_device
> in DeviceTree.

Are you considering out-of-tree drivers? My understanding is that there
are different PCIe modules, ones that don't have GPIO-control for x86
and ones that do have GPIO-control for ARM. The out-of-tree cnss
platform driver used for Android has a large amount of DT control
<https://git.codelinaro.org/clo/la/platform/vendor/qcom-opensource/wlan/platform/-/blob/wlan-platform.lnx.1.0.r1-rel/cnss2/power.c?ref_type=heads>

(not sure off hand where the DT files themselves are)

/jeff
Bartosz Golaszewski Feb. 2, 2024, 10:02 a.m. UTC | #5
On Fri, Feb 2, 2024 at 1:03 AM Bjorn Andersson <andersson@kernel.org> wrote:
>

[snip]

> > >
> > > I believe I missed this part of the discussion, why does this need to be
> > > a platform_device? What does the platform_bus bring that can't be
> > > provided by some other bus?
> > >
> >
> > Does it need to be a platform_device? No, of course not. Does it make
> > sense for it to be one? Yes, for two reasons:
> >
> > 1. The ATH11K WLAN module is represented on the device tree like a
> > platform device, we know it's always there and it consumes regulators
> > from another platform device. The fact it uses PCIe doesn't change the
> > fact that it is logically a platform device.
>
> Are you referring to the ath11k SNOC (firmware running on co-processor
> in the SoC) variant?
>
> Afaict the PCIe-attached ath11k is not represented as a platform_device
> in DeviceTree.
>

My bad. In RB5 it isn't (yet - I want to add it in the power
sequencing series). It is in X13s though[1].

> Said platform_device is also not a child under the PCIe bus, so this
> would be a different platform_device...
>

It's the child of the PCIe port node but there's a reason for it to
have the `compatible` property. It's because it's an entity of whose
existence we are aware before the system boots.

> > 2. The platform bus already provides us with the entire infrastructure
> > that we'd now need to duplicate (possibly adding bugs) in order to
> > introduce a "power sequencing bus".
> >
>
> This is a perfectly reasonable desire. Look at our PMICs, they are full
> of platform_devices. But through the years it's been said many times,
> that this is not a valid or good reason for using platform_devices, and
> as a result we have e.g. auxiliary bus.
>

Ok, so I cannot find this information anywhere (nor any example). Do
you happen to know if the auxiliary bus offers any software node
integration so that the `compatible` property from DT can get
seamlessly mapped to auxiliary device IDs?

> Anyway, (please) don't claim that "we need to", when it actually is "we
> want to use platform_device because that's more convenient"!

Bart

[snip]

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts#n744
Bartosz Golaszewski Feb. 7, 2024, 4:32 p.m. UTC | #6
On Fri, Feb 2, 2024 at 11:02 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Fri, Feb 2, 2024 at 1:03 AM Bjorn Andersson <andersson@kernel.org> wrote:
> >
>
> [snip]
>
> > > >
> > > > I believe I missed this part of the discussion, why does this need to be
> > > > a platform_device? What does the platform_bus bring that can't be
> > > > provided by some other bus?
> > > >
> > >
> > > Does it need to be a platform_device? No, of course not. Does it make
> > > sense for it to be one? Yes, for two reasons:
> > >
> > > 1. The ATH11K WLAN module is represented on the device tree like a
> > > platform device, we know it's always there and it consumes regulators
> > > from another platform device. The fact it uses PCIe doesn't change the
> > > fact that it is logically a platform device.
> >
> > Are you referring to the ath11k SNOC (firmware running on co-processor
> > in the SoC) variant?
> >
> > Afaict the PCIe-attached ath11k is not represented as a platform_device
> > in DeviceTree.
> >
>
> My bad. In RB5 it isn't (yet - I want to add it in the power
> sequencing series). It is in X13s though[1].
>
> > Said platform_device is also not a child under the PCIe bus, so this
> > would be a different platform_device...
> >
>
> It's the child of the PCIe port node but there's a reason for it to
> have the `compatible` property. It's because it's an entity of whose
> existence we are aware before the system boots.
>
> > > 2. The platform bus already provides us with the entire infrastructure
> > > that we'd now need to duplicate (possibly adding bugs) in order to
> > > introduce a "power sequencing bus".
> > >
> >
> > This is a perfectly reasonable desire. Look at our PMICs, they are full
> > of platform_devices. But through the years it's been said many times,
> > that this is not a valid or good reason for using platform_devices, and
> > as a result we have e.g. auxiliary bus.
> >
>
> Ok, so I cannot find this information anywhere (nor any example). Do
> you happen to know if the auxiliary bus offers any software node
> integration so that the `compatible` property from DT can get
> seamlessly mapped to auxiliary device IDs?
>

So I was just trying to port this to using the auxiliary bus, only to
find myself literally reimplementing functions from
drivers/of/device.c. I have a feeling that this is simply wrong. If
we're instantiating devices well defined on the device-tree then IMO
we *should* make them platform devices. Anything else and we'll be
reimplementing drivers/of/ because we will need to parse the device
nodes, check the compatible, match it against drivers etc. Things that
are already implemented for the platform bus and of_* APIs.

Greg: Could you chime in and confirm that it's alright to use the
platform bus here? Or maybe there is some infrastructure to create
auxiliary devices from software nodes?

Bartosz

> > Anyway, (please) don't claim that "we need to", when it actually is "we
> > want to use platform_device because that's more convenient"!
>
> Bart
>
> [snip]
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts#n744
Greg KH Feb. 14, 2024, 3:34 p.m. UTC | #7
On Wed, Feb 07, 2024 at 05:32:38PM +0100, Bartosz Golaszewski wrote:
> On Fri, Feb 2, 2024 at 11:02 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > On Fri, Feb 2, 2024 at 1:03 AM Bjorn Andersson <andersson@kernel.org> wrote:
> > >
> >
> > [snip]
> >
> > > > >
> > > > > I believe I missed this part of the discussion, why does this need to be
> > > > > a platform_device? What does the platform_bus bring that can't be
> > > > > provided by some other bus?
> > > > >
> > > >
> > > > Does it need to be a platform_device? No, of course not. Does it make
> > > > sense for it to be one? Yes, for two reasons:
> > > >
> > > > 1. The ATH11K WLAN module is represented on the device tree like a
> > > > platform device, we know it's always there and it consumes regulators
> > > > from another platform device. The fact it uses PCIe doesn't change the
> > > > fact that it is logically a platform device.
> > >
> > > Are you referring to the ath11k SNOC (firmware running on co-processor
> > > in the SoC) variant?
> > >
> > > Afaict the PCIe-attached ath11k is not represented as a platform_device
> > > in DeviceTree.
> > >
> >
> > My bad. In RB5 it isn't (yet - I want to add it in the power
> > sequencing series). It is in X13s though[1].
> >
> > > Said platform_device is also not a child under the PCIe bus, so this
> > > would be a different platform_device...
> > >
> >
> > It's the child of the PCIe port node but there's a reason for it to
> > have the `compatible` property. It's because it's an entity of whose
> > existence we are aware before the system boots.
> >
> > > > 2. The platform bus already provides us with the entire infrastructure
> > > > that we'd now need to duplicate (possibly adding bugs) in order to
> > > > introduce a "power sequencing bus".
> > > >
> > >
> > > This is a perfectly reasonable desire. Look at our PMICs, they are full
> > > of platform_devices. But through the years it's been said many times,
> > > that this is not a valid or good reason for using platform_devices, and
> > > as a result we have e.g. auxiliary bus.
> > >
> >
> > Ok, so I cannot find this information anywhere (nor any example). Do
> > you happen to know if the auxiliary bus offers any software node
> > integration so that the `compatible` property from DT can get
> > seamlessly mapped to auxiliary device IDs?
> >
> 
> So I was just trying to port this to using the auxiliary bus, only to
> find myself literally reimplementing functions from
> drivers/of/device.c. I have a feeling that this is simply wrong. If
> we're instantiating devices well defined on the device-tree then IMO
> we *should* make them platform devices. Anything else and we'll be
> reimplementing drivers/of/ because we will need to parse the device
> nodes, check the compatible, match it against drivers etc. Things that
> are already implemented for the platform bus and of_* APIs.
> 
> Greg: Could you chime in and confirm that it's alright to use the
> platform bus here? Or maybe there is some infrastructure to create
> auxiliary devices from software nodes?

Note, I HATE the use of the platform bus here, but I don't have a better
suggestion.

I'd love for the auxbus to work, and if you can create that from
software nodes, all the better!  But I don't think that's possible just
yet, and you would end up implementing all the same stuff that the
platform bus has today for this functionality, so I doubt it would be
worth it.

thanks,

greg k-h
diff mbox series

Patch

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 9c2137dae429..8ab07f711834 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -12,6 +12,7 @@ 
 #include <linux/errno.h>
 #include <linux/ioport.h>
 #include <linux/of.h>
+#include <linux/of_platform.h>
 #include <linux/proc_fs.h>
 #include <linux/slab.h>
 
@@ -342,8 +343,14 @@  void pci_bus_add_device(struct pci_dev *dev)
 	 */
 	pcibios_bus_add_device(dev);
 	pci_fixup_device(pci_fixup_final, dev);
-	if (pci_is_bridge(dev))
+	if (pci_is_bridge(dev)) {
 		of_pci_make_dev_node(dev);
+		retval = of_platform_populate(dev->dev.of_node, NULL, NULL,
+					      &dev->dev);
+		if (retval)
+			pci_err(dev, "failed to populate child OF nodes (%d)\n",
+				retval);
+	}
 	pci_create_sysfs_dev_files(dev);
 	pci_proc_attach_device(dev);
 	pci_bridge_d3_update(dev);
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index d749ea8250d6..77be0630b7b3 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -1,6 +1,7 @@ 
 // SPDX-License-Identifier: GPL-2.0
 #include <linux/pci.h>
 #include <linux/module.h>
+#include <linux/of_platform.h>
 #include "pci.h"
 
 static void pci_free_resources(struct pci_dev *dev)
@@ -18,11 +19,11 @@  static void pci_stop_dev(struct pci_dev *dev)
 	pci_pme_active(dev, false);
 
 	if (pci_dev_is_added(dev)) {
-
 		device_release_driver(&dev->dev);
 		pci_proc_detach_device(dev);
 		pci_remove_sysfs_dev_files(dev);
 		of_pci_remove_node(dev);
+		of_platform_depopulate(&dev->dev);
 
 		pci_dev_assign_added(dev, false);
 	}