| Message ID | 20230120-simple-mfd-pci-v1-1-c46b3d6601ef@axis.com |
|---|---|
| State | New |
| Headers | show |
| Series | mfd: Add Simple PCI MFD driver | expand |
On Mon, 23 Jan 2023, Vincent Whitchurch wrote: > Add a PCI driver which registers all child nodes specified in the > devicetree. It will allow platform devices to be used on virtual > systems which already support PCI and devicetree, such as UML with > virt-pci. > > The driver has no id_table by default; user space needs to provide one > using the new_id mechanism in sysfs. This feels wrong for several reasons. Firstly, I think Greg (Cc:ed) will have something to say about this. Secondly, this driver does literally nothing. Why can't you use of of the other, pre-existing "also register my children" compatibles? See: drivers/bus/simple-pm-bus.c drivers/of/platform.c > Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com> > --- > drivers/mfd/Kconfig | 11 +++++++++++ > drivers/mfd/Makefile | 1 + > drivers/mfd/simple-mfd-pci.c | 21 +++++++++++++++++++++ > 3 files changed, 33 insertions(+) > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index 30db49f31866..1e325334e9ae 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -1277,6 +1277,17 @@ config MFD_SIMPLE_MFD_I2C > sub-devices represented by child nodes in Device Tree will be > subsequently registered. > > +config MFD_SIMPLE_MFD_PCI > + tristate "Simple Multi-Functional Device support (PCI)" > + depends on PCI > + depends on OF || COMPILE_TEST > + help > + This enables support for a PCI driver for which any sub-devices > + represented by child nodes in the devicetree will be registered. > + > + The driver does not bind to any devices by default; that should > + be done via sysfs using new_id. > + > config MFD_SL28CPLD > tristate "Kontron sl28cpld Board Management Controller" > depends on I2C > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index 457471478a93..7ae329039a13 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -268,6 +268,7 @@ obj-$(CONFIG_MFD_QCOM_PM8008) += qcom-pm8008.o > > obj-$(CONFIG_SGI_MFD_IOC3) += ioc3.o > obj-$(CONFIG_MFD_SIMPLE_MFD_I2C) += simple-mfd-i2c.o > +obj-$(CONFIG_MFD_SIMPLE_MFD_PCI) += simple-mfd-pci.o > obj-$(CONFIG_MFD_SMPRO) += smpro-core.o > obj-$(CONFIG_MFD_INTEL_M10_BMC) += intel-m10-bmc.o > > diff --git a/drivers/mfd/simple-mfd-pci.c b/drivers/mfd/simple-mfd-pci.c > new file mode 100644 > index 000000000000..c5b2540e924a > --- /dev/null > +++ b/drivers/mfd/simple-mfd-pci.c > @@ -0,0 +1,21 @@ > +// SPDX-License-Identifier: GPL-2.0-only > + > +#include <linux/module.h> > +#include <linux/of_platform.h> > +#include <linux/pci.h> > + > +static int simple_mfd_pci_probe(struct pci_dev *pdev, > + const struct pci_device_id *id) > +{ > + return devm_of_platform_populate(&pdev->dev); > +} > + > +static struct pci_driver simple_mfd_pci_driver = { > + /* No id_table, use new_id in sysfs */ > + .name = "simple-mfd-pci", > + .probe = simple_mfd_pci_probe, > +}; > + > +module_pci_driver(simple_mfd_pci_driver); > + > +MODULE_LICENSE("GPL"); > > --- > base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2 > change-id: 20230120-simple-mfd-pci-54f0d9b90dfc > > Best regards, > -- > Vincent Whitchurch <vincent.whitchurch@axis.com>
On Mon, Jan 23, 2023 at 04:32:55PM +0100, Lee Jones wrote: > On Mon, 23 Jan 2023, Vincent Whitchurch wrote: > > Add a PCI driver which registers all child nodes specified in the > > devicetree. It will allow platform devices to be used on virtual > > systems which already support PCI and devicetree, such as UML with > > virt-pci. > > > > The driver has no id_table by default; user space needs to provide one > > using the new_id mechanism in sysfs. > > This feels wrong for several reasons. > > Firstly, I think Greg (Cc:ed) will have something to say about this. > > Secondly, this driver does literally nothing. Well, it does do what the commit message says. If there's another way of accomplishing that, I'm all ears. > Why can't you use of of the other, pre-existing "also register my > children" compatibles? > > See: drivers/bus/simple-pm-bus.c > drivers/of/platform.c simple-pm-bus registers a platform driver, and drivers/of/platform.c works on the platform bus. The driver added by this patch is a PCI driver. So I don't understand how the files you mention could be used here? In case it helps, the relevant nodes in my UML devicetree look something like this: virtio@2 { compatible = "virtio,uml"; virtio-device-id = <1234>; ranges; pci { #address-cells = <3>; #size-cells = <2>; ranges = <0x0000000 0 0 0 0xf0000000 0 0x20000>; compatible = "virtio,device4d2", "pci"; device_type = "pci"; bus-range = <0 0>; platform_parent: device@0,0 { compatible = "pci494f,dc8"; reg = <0x00000 0 0 0x0 0x10000>; ranges; uart@10000 { compatible = "google,goldfish-tty"; reg = <0x00000 0 0x10000 0 0x10000>; }; }; }; };
On Mon, Jan 23, 2023 at 10:02 AM Vincent Whitchurch <vincent.whitchurch@axis.com> wrote: > > On Mon, Jan 23, 2023 at 04:32:55PM +0100, Lee Jones wrote: > > On Mon, 23 Jan 2023, Vincent Whitchurch wrote: > > > Add a PCI driver which registers all child nodes specified in the > > > devicetree. It will allow platform devices to be used on virtual > > > systems which already support PCI and devicetree, such as UML with > > > virt-pci. > > > > > > The driver has no id_table by default; user space needs to provide one > > > using the new_id mechanism in sysfs. > > > > This feels wrong for several reasons. > > > > Firstly, I think Greg (Cc:ed) will have something to say about this. > > > > Secondly, this driver does literally nothing. > > Well, it does do what the commit message says. If there's another way > of accomplishing that, I'm all ears. > > > Why can't you use of of the other, pre-existing "also register my > > children" compatibles? > > > > See: drivers/bus/simple-pm-bus.c > > drivers/of/platform.c > > simple-pm-bus registers a platform driver, and drivers/of/platform.c > works on the platform bus. The driver added by this patch is a PCI > driver. So I don't understand how the files you mention could be used > here? > > In case it helps, the relevant nodes in my UML devicetree look something > like this: > > virtio@2 { dtc should complain about this... > compatible = "virtio,uml"; Binding? > virtio-device-id = <1234>; > ranges; > > pci { > #address-cells = <3>; > #size-cells = <2>; > ranges = <0x0000000 0 0 0 0xf0000000 0 0x20000>; > compatible = "virtio,device4d2", "pci"; "pci" is not a valid compatible string. > device_type = "pci"; > bus-range = <0 0>; > > platform_parent: device@0,0 { > compatible = "pci494f,dc8"; > reg = <0x00000 0 0 0x0 0x10000>; > ranges; > > uart@10000 { > compatible = "google,goldfish-tty"; > reg = <0x00000 0 0x10000 0 0x10000>; This is not a PCI device, so it shouldn't be using PCI addressing. 'ranges' needs an entry (for each BAR) to translate to just a normal MMIO bus with 1 or 2 address/size cells. Maybe we want a 'simple-bus' node for each BAR. The FPGA series needs the same things, but that aspect hasn't really been addressed as the first issue is populating the PCI devices dynamically. The DT address translation code should support all this (MMIO->PCI->MMIO), but I don't think there's any existing examples. An example (that I can test) would be great. If the unittest had that example, I'd be thrilled. Rob
On Mon, Jan 23, 2023 at 05:36:06PM +0100, Rob Herring wrote: > On Mon, Jan 23, 2023 at 10:02 AM Vincent Whitchurch > <vincent.whitchurch@axis.com> wrote: > dtc should complain about this... It probably does, the test framework currently doesn't report these to the test runner/writer; maybe it should. > > compatible = "virtio,uml"; > > Binding? There was some discussion earlier about whether a binding was needed here (you were on CC): https://lore.kernel.org/lkml/20211222103417.GB25135@axis.com/ > > > virtio-device-id = <1234>; > > ranges; > > > > pci { > > #address-cells = <3>; > > #size-cells = <2>; > > ranges = <0x0000000 0 0 0 0xf0000000 0 0x20000>; > > compatible = "virtio,device4d2", "pci"; > > "pci" is not a valid compatible string. I think it's there since I based this tree off from arch/x86/platform/ce4100/falconfalls.dts. I see that there is some code in arch/x86/kernel/devicetree.c to handle this compatible and register all platform devices under that. Do we need something like that for UML instead of this patch? > > > device_type = "pci"; > > bus-range = <0 0>; > > > > platform_parent: device@0,0 { > > compatible = "pci494f,dc8"; > > reg = <0x00000 0 0 0x0 0x10000>; > > ranges; > > > > uart@10000 { > > compatible = "google,goldfish-tty"; > > reg = <0x00000 0 0x10000 0 0x10000>; > > This is not a PCI device, so it shouldn't be using PCI addressing. > 'ranges' needs an entry (for each BAR) to translate to just a normal > MMIO bus with 1 or 2 address/size cells. Maybe we want a 'simple-bus' > node for each BAR. The FPGA series needs the same things, but that > aspect hasn't really been addressed as the first issue is populating > the PCI devices dynamically. Yes, this ranges stuff can be fixed in the Python code which generates these trees. In my cases the devicetree blob contains all the devices under the PCI devices, see my other email. > The DT address translation code should support all this > (MMIO->PCI->MMIO), but I don't think there's any existing examples. An > example (that I can test) would be great. If the unittest had that > example, I'd be thrilled. Anyone can run what I'm running since it uses UML and there is no real hardware, but the setup is a bit more complicated than an in-kernel unit test since there is a virtio backend in userspace which implements the "hardware". If you want to try it: git remote add vwax https://github.com/vwax/linux.git git fetch vwax git checkout vmax/roadtest/platform-wip make -C tools/testing/roadtest/ -j24 OPTS="-v -k platform" You should see a "PASSED roadtest/tests/base/test_platform.py::test_foo" if it works. See Documentation/dev-tools/roadtest.rst for more info. As mentioned in the other email, the only patches to the kernel proper in that tree are already posted ones and WIP fixes.
On Wed, Jan 25, 2023 at 01:29:32PM +0100, Greg Kroah-Hartman wrote: > On Wed, Jan 25, 2023 at 11:15:38AM +0100, Vincent Whitchurch wrote: > > I hope it's clear from my other replies in this thread that the entire > > purpose of this driver is to allow arbitrary platform devices to be used > > via a PCI device in virtual environments like User Mode Linux in order > > to test existing platform drivers using mocked hardware. > > That still feels wrong, why is PCI involved here at all? > > Don't abuse platform devices like this please, mock up a platform device > framework instead if you want to test them that way, don't think that > adding a platform device "below" a PCI device is somehow allowed at all. As you know, PCI allows exposing an MMIO region to the host, so the host can use ioremap() and readl()/writel() on it. This allows reusing platform drivers even though the device is on the other side of a PCI bus. There is hardware already supported by the kernel since a long time ago which is handled by putting platform devices below PCI devices. See add_bus_probe() in arch/x86/kernel/devicetree.c. And this hardware also wants to do the same thing: https://lore.kernel.org/lkml/1674183732-5157-1-git-send-email-lizhi.hou@amd.com/ Also, UML already supports out-of-process PCI, and there is ongoing work in QEMU to add support for out-of-process PCI emulation. So using PCI will allow this to work on different kinds of virtual environments without having to invent a new method specifically for platform devices. > > Given this "hardware", it's not clear what a "real driver" would do > > differently. > > Again, you can not have a platform device below a PCI device, that's not > what a platform device is for at all. See above. > > The auxiliary bus cannot be used since it naturally does > > not support platform devices. > > The aux bus can support any type of bus (it's there to be used as you > want, it's just that people are currently using it for PCI devices right > now). I assume we're talking about drivers/base/auxiliary.c? The kernel doc says: * A key requirement for utilizing the auxiliary bus is that there is no * dependency on a physical bus, device, register accesses or regmap support. * These individual devices split from the core cannot live on the platform bus * as they are not physical devices that are controlled by DT/ACPI. But this case the sub-devices do need standard register access with readl()/writel() and _are_ controlled by devicetree. > > A hard coded list of sub-devices cannot be used since arbitrary > > platform devices with arbitrary devicetree properties need to be > > supported. > > Then make a new bus type and again, do not abuse platform devices. How can existing platform drivers be re-used if you invent a new bus type and don't create platform devices? > > I could move this driver to drivers/bus/ and pitch it as a > > "PCI<->platform bridge for testing in virtual environments", if that > > makes more sense. > > Again, nope, a platform device is NOT ever a child of a PCI device. > That's just not how PCI works at all. > > Would you do the attempt to do this for USB? (hint, no.) So why is PCI > somehow special here? PCI is special because it allows exposing an MMIO region to the host and allowing the host to access it like any other I/O memory. USB doesn't allow that.
On Wed, Jan 25, 2023 at 08:54:21AM -0600, Rob Herring wrote: > On Wed, Jan 25, 2023 at 6:29 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Wed, Jan 25, 2023 at 11:15:38AM +0100, Vincent Whitchurch wrote: > > > On Mon, Jan 23, 2023 at 05:31:21PM +0100, Greg Kroah-Hartman wrote: > > > > On Mon, Jan 23, 2023 at 03:32:55PM +0000, Lee Jones wrote: > > > > > On Mon, 23 Jan 2023, Vincent Whitchurch wrote: > > > > > > > > > > > Add a PCI driver which registers all child nodes specified in the > > > > > > devicetree. It will allow platform devices to be used on virtual > > > > > > systems which already support PCI and devicetree, such as UML with > > > > > > virt-pci. > > > > > > > > > > > > The driver has no id_table by default; user space needs to provide one > > > > > > using the new_id mechanism in sysfs. > > > > > > > > > > This feels wrong for several reasons. > > > > > > > > > > Firstly, I think Greg (Cc:ed) will have something to say about this. > > > > > > > > Yes, this isn't ok. Please write a real driver for the hardware under > > > > control here, and that would NOT be a MFD driver (hint, if you want to > > > > split up a PCI device into different drivers, use the aux bus code, that > > > > is what it is there for.) > > > > > > I hope it's clear from my other replies in this thread that the entire > > > purpose of this driver is to allow arbitrary platform devices to be used > > > via a PCI device in virtual environments like User Mode Linux in order > > > to test existing platform drivers using mocked hardware. > > > > That still feels wrong, why is PCI involved here at all? > > > > Don't abuse platform devices like this please, mock up a platform device > > framework instead if you want to test them that way, don't think that > > adding a platform device "below" a PCI device is somehow allowed at all. > > My question as well. However, that's only for Vincent's usecase. The > other ones I'm aware of are definitely non-discoverable MMIO devices > behind a PCI device. > > It is perfectly valid in DT to have the same device either directly on > an MMIO bus or behind some other MMIO capable bus. So what bus type > should they all be? If the mmio space is behind a PCI device, then why isn't that a special bus type for that "pci-mmio" or something, right? Otherwise what happens when you yank out that PCI device? Does that work today for these platform devices? > > > Given this "hardware", it's not clear what a "real driver" would do > > > differently. > > > > Again, you can not have a platform device below a PCI device, that's not > > what a platform device is for at all. > > > > > The auxiliary bus cannot be used since it naturally does > > > not support platform devices. > > > > The aux bus can support any type of bus (it's there to be used as you > > want, it's just that people are currently using it for PCI devices right > > now). > > > > > A hard coded list of sub-devices cannot be used since arbitrary > > > platform devices with arbitrary devicetree properties need to be > > > supported. > > > > Then make a new bus type and again, do not abuse platform devices. > > How about of_platform_bus[1]? Fair enough :) > At this point, it would be easier to create a new bus type for > whatever it is you think *should* be a platform device and move those > to the new bus leaving platform_bus as the DT/ACPI devices bus. platfom bus should be for DT/ACPI devices like that, but that's not what a "hang a DT off a PCI device" should be, right? Why is mmio space somehow special here? Perhaps we just add support for that to the aux bus? > > > I could move this driver to drivers/bus/ and pitch it as a > > > "PCI<->platform bridge for testing in virtual environments", if that > > > makes more sense. > > > > Again, nope, a platform device is NOT ever a child of a PCI device. > > That's just not how PCI works at all. > > > > Would you do the attempt to do this for USB? (hint, no.) So why is PCI > > somehow special here? > > Actually, yes. It is limited since USB cannot tunnel MMIO accesses > (though I suppose USB4 PCIe tunneling can?), but we do have some > platform drivers which don't do MMIO. USB4 is really just pci, ignore the "USB" term :) > Suppose I have an FTDI chip with GPIOs on it and I want to do GPIO > LEDs, keys, bitbanged I2C, etc. Those would use the leds-gpio, > gpio-keys, i2c-gpio platform drivers. Then those drivers need to be tweaked to support the new bus type, but that can't be a platform device hanging off of a USB device as that makes no sense... thanks, greg k-h
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index 30db49f31866..1e325334e9ae 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -1277,6 +1277,17 @@ config MFD_SIMPLE_MFD_I2C sub-devices represented by child nodes in Device Tree will be subsequently registered. +config MFD_SIMPLE_MFD_PCI + tristate "Simple Multi-Functional Device support (PCI)" + depends on PCI + depends on OF || COMPILE_TEST + help + This enables support for a PCI driver for which any sub-devices + represented by child nodes in the devicetree will be registered. + + The driver does not bind to any devices by default; that should + be done via sysfs using new_id. + config MFD_SL28CPLD tristate "Kontron sl28cpld Board Management Controller" depends on I2C diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index 457471478a93..7ae329039a13 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -268,6 +268,7 @@ obj-$(CONFIG_MFD_QCOM_PM8008) += qcom-pm8008.o obj-$(CONFIG_SGI_MFD_IOC3) += ioc3.o obj-$(CONFIG_MFD_SIMPLE_MFD_I2C) += simple-mfd-i2c.o +obj-$(CONFIG_MFD_SIMPLE_MFD_PCI) += simple-mfd-pci.o obj-$(CONFIG_MFD_SMPRO) += smpro-core.o obj-$(CONFIG_MFD_INTEL_M10_BMC) += intel-m10-bmc.o diff --git a/drivers/mfd/simple-mfd-pci.c b/drivers/mfd/simple-mfd-pci.c new file mode 100644 index 000000000000..c5b2540e924a --- /dev/null +++ b/drivers/mfd/simple-mfd-pci.c @@ -0,0 +1,21 @@ +// SPDX-License-Identifier: GPL-2.0-only + +#include <linux/module.h> +#include <linux/of_platform.h> +#include <linux/pci.h> + +static int simple_mfd_pci_probe(struct pci_dev *pdev, + const struct pci_device_id *id) +{ + return devm_of_platform_populate(&pdev->dev); +} + +static struct pci_driver simple_mfd_pci_driver = { + /* No id_table, use new_id in sysfs */ + .name = "simple-mfd-pci", + .probe = simple_mfd_pci_probe, +}; + +module_pci_driver(simple_mfd_pci_driver); + +MODULE_LICENSE("GPL");
Add a PCI driver which registers all child nodes specified in the devicetree. It will allow platform devices to be used on virtual systems which already support PCI and devicetree, such as UML with virt-pci. The driver has no id_table by default; user space needs to provide one using the new_id mechanism in sysfs. Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com> --- drivers/mfd/Kconfig | 11 +++++++++++ drivers/mfd/Makefile | 1 + drivers/mfd/simple-mfd-pci.c | 21 +++++++++++++++++++++ 3 files changed, 33 insertions(+) --- base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2 change-id: 20230120-simple-mfd-pci-54f0d9b90dfc Best regards,