| Message ID | 20230120-simple-mfd-pci-v1-1-c46b3d6601ef@axis.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| 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 8:32 AM Vincent Whitchurch <vincent.whitchurch@axis.com> 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. There's similar work underway for Xilinx/AMD PCIe FPGAs[1]. It's the same thing really. Non-discoverable things downstream of a PCI device. There's also a desire for that to work on non-DT (ACPI) based hosts. While UML supports DT, that's currently only for the unittest AFAIK. So it's more like a non-DT host. How does the DT get populated for UML for this to work? Can you provide details on the actual h/w you want to use. What problem are you trying to solve? Really, what I want to see here is everyone interested in this feature to work together on it. Not just creating a one-off solution for their 1 use case that's a subset of a bigger solution. > The driver has no id_table by default; user space needs to provide one > using the new_id mechanism in sysfs. But your DT will have the id in it already. Wouldn't you rather everything work without userspace intervention? I can't imagine the list here would be too long. > > 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); Really, this could be anything in the child DT. Not just what Linux classifies as an MFD. So maybe drivers/mfd is not the right place. Rob [1] https://lore.kernel.org/all/1674183732-5157-1-git-send-email-lizhi.hou@amd.com/
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.) thanks, greg k-h
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 1/23/23 08:36, Rob Herring wrote: > 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. (I tried to reply with my comment this morning, but it did not post to kernel mail alias. I am re-sending it. Please ignore if you already received my reply) I have proposed the address format for hardware apertures on PCI BAR. The address consists of BAR index and offset. It uses the following encoding: 0xIooooooo 0xoooooooo Where: I = BAR index oooooo oooooooo = BAR offset The endpoint is compatible with 'simple-bus' and contains 'ranges' property for translating aperture address to CPU address. Ref: https://lore.kernel.org/lkml/20220305052304.726050-3-lizhi.hou@xilinx.com/ The 64-bit address can use the default translator defined of/address.c I implemented an example of top of my latest patch https://lore.kernel.org/lkml/1674183732-5157-1-git-send-email-lizhi.hou@amd.com/ to verify the address translation. The test device tree nodes are defined to present two hardware apertures on our alveo PCI device: pr-isolation: PCI BAR 0, offset 0x41000, len 0x1000 xdma: PCI BAR 2, offset 0x0, len 0x40000 / { fragment@0 { target-path=""; __overlay__ { pci-ep-bus@0 { compatible = "simple-bus"; #address-cells = <2>; #size-cells = <2>; ranges = <0x0 0x0 0x0 0x0 0x0 0x2000000>; pr_isolate_ulp@41000 { compatible = "xlnx,alveo-pr-isolation"; reg = <0x0 0x41000 0x0 0x1000>; }; }; pci-ep-bus@2 { compatible = "simple-bus"; #address-cells = <2>; #size-cells = <2>; ranges = <0x0 0x0 0x20000000 0x0 0x0 0x40000>; alveo-xdma@0 { compatible = "xlnx,alveo-xdma"; reg = <0x0 0x0 0x0 0x40000>; }; }; }; }; }; Overall, the test is as below 1) added code to generate 'ranges' for PCI endpoint dt node 00000000 00000000 C30B0000 00000080 10000000 00000000 02000000 (BAR 0) 20000000 00000000 C30B0000 00000080 14000000 00000000 00040000 (BAR 2) ^ bar index ^^^^^^^^^^^^^BAR IOMEM address code link: https://github.com/houlz0507/linux-xoclv2/compare/29031e597fd6272f825dd04ba41a38defb77a99a...pci-dt-v6?diff=unified#diff-bf1b86155c18e04c439b74f5a02bad99c91a8c04f3c21243afce996c2174be56 2) overlay the test device tree fragment under PCI endpoint. 3) Alveo pci device driver probe function calls of_platform_default_populate(). I can see the BAR index+offset is translated to IO address correctly. # ls /sys/bus/platform/devices/ 0.flash 3f000000.pcie 3f000000.pcie:pci@2,0:pci@0,0:pci@0,0:dev@0,0:pci-ep-bus@0 3f000000.pcie:pci@2,0:pci@0,0:pci@0,0:dev@0,0:pci-ep-bus@2 8010041000.pr_isolate_ulp 8014000000.alveo-xdma # lspci -vd 10ee:5020 | grep Memory Memory at 8010000000 (64-bit, prefetchable) [disabled] [size=32M] Memory at 8014000000 (64-bit, prefetchable) [disabled] [size=256K] This test needs our Alveo PCI device. It might be a reference to create unittest. Thanks, Lizhi > > Rob
On Mon, Jan 23, 2023 at 05:13:06PM +0100, Rob Herring wrote: > On Mon, Jan 23, 2023 at 8:32 AM Vincent Whitchurch > <vincent.whitchurch@axis.com> 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. > > There's similar work underway for Xilinx/AMD PCIe FPGAs[1]. It's the > same thing really. Non-discoverable things downstream of a PCI device. > There's also a desire for that to work on non-DT (ACPI) based hosts. > While UML supports DT, that's currently only for the unittest AFAIK. It's possible to pass a devicetree blob to UML via a command line argument[0]. The roadtest[1][2] framework uses this to test device drivers. [0] https://lore.kernel.org/lkml/20211208151123.29313-3-vincent.whitchurch@axis.com/ [1] https://lore.kernel.org/lkml/20220311162445.346685-1-vincent.whitchurch@axis.com/ [2] https://lwn.net/Articles/887974/ > So it's more like a non-DT host. How does the DT get populated for UML > for this to work? The dts is generated by the test framework based on the test cases being run (see the files being patched in [3]) and is compiled and passed to UML via the command line argument. > Can you provide details on the actual h/w you want to use. What > problem are you trying to solve? There is no real hardware. I'm using this to add support for platform devices to roadtest. As the commit message said, UML supports PCI but I want to test platform devices so I just need something to allow me to put arbitrary platform devices under the PCI device and have them get probed. The PCI "hardware" (in backend.c in [3]) is just enough implementation of the BARs to keep Linux happy and forward the register accesses to the platform hardware implementation which is in Python as part of the test cases (eg. test_platform.py in [3]). See my WIP patch for platform device support to roadtest which includes a test for the goldfish UART: [3] https://github.com/vwax/linux/commit/636f4150b086dc581fdfb464869eb98b8a22a254 (The roadtest code is placed in a kernel tree but the only patches to the kernel proper are this one, https://lore.kernel.org/lkml/20230120-uml-pci-of-v1-1-134fb66643d8@axis.com/, and a couple of ongoing fixes at the top of the tree. Roadtest is designed to work on unpatched mainline kernels.) > Really, what I want to see here is everyone interested in this feature > to work together on it. Not just creating a one-off solution for their > 1 use case that's a subset of a bigger solution. > > > The driver has no id_table by default; user space needs to provide one > > using the new_id mechanism in sysfs. > > But your DT will have the id in it already. Wouldn't you rather > everything work without userspace intervention? I can't imagine the > list here would be too long. I would be nice for things to work without userspace intervention (see the change to init.sh in [3]), but I don't have real hardware or real PCI IDs, and I don't think we would want to hardcode made-up numbers in the ID table? > > 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); > > Really, this could be anything in the child DT. Not just what Linux > classifies as an MFD. So maybe drivers/mfd is not the right place. What would be the right place? drivers/bus? Or perhaps something UML-specific similar to arch/x86/kernel/devicetree.c?
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 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. Given this "hardware", it's not clear what a "real driver" would do differently. The auxiliary bus cannot be used since it naturally does not support platform devices. A hard coded list of sub-devices cannot be used since arbitrary platform devices with arbitrary devicetree properties need to be supported. 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.
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. > 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. > 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? thanks, greg k-h
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 02:06:26PM +0100, Vincent Whitchurch wrote: > 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. Those are not platform devices below a PCI device from what I can tell, and if it is, it's wrong and should be fixed up. Don't perpetuate bad design decisions of the past please. > 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/ That too is wrong. That is NOT what the platform devices and drivers are for at all, make them some other type of device please, which is why the aux bus was created. > 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. I don't see how PCI is relevant here, sorry. > > > 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. Ok, let's make a new bus for them then. As obviously they are not platform devices if they live on the PCI bus. > > > 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? The same way we reuse lots of drivers for devices on different busses today. > > > 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. So you are saying that just because a bus type exports MMIO regions, it should be allowed to be a platform device? Sorry, but make that a new bus type as obviously it is something else. Maybe these are all just real PCI devices (or sub devices), so treat them like that please. thanks, greg k-h
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? > > 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]? 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. > > 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. 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. Rob [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=eca3930163ba8884060ce9d9ff5ef0d9b7c7b00f
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
On Wed, Jan 25, 2023 at 9:00 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > 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? Well, yes, I'm sure there's lots of issues with hot-unplug and DT. That's pretty much anything using DT, not just platform devices. Those will only get fixed when folks try to do that, but so far we've mostly prevented doing that. For example, we don't support a generic mechanism to add and remove DT overlays because most drivers aren't ready for their DT node to disappear. Is there some restriction that says platform_bus can't do hotplug? I thought everything is hotpluggable (in theory). > > > > 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? Only because platform_bus is the bus type in the kernel that supports MMIO devices and that the DT code uses to instantiate them. The DT code doesn't care if those are at the root level or behind some other bus type. > Perhaps we just add support for that to the aux > bus? Yes, we could add IOMEM resources, DT ID table and matching, etc., but we'd just end up back at of_platform_bus with a new name. Every driver doing both would have 2 driver structs and register calls. What do we gain from that? Rob
On Wed, Jan 25, 2023 at 09:34:01AM -0600, Rob Herring wrote: > On Wed, Jan 25, 2023 at 9:00 AM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > 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? > > Well, yes, I'm sure there's lots of issues with hot-unplug and DT. > That's pretty much anything using DT, not just platform devices. Those > will only get fixed when folks try to do that, but so far we've mostly > prevented doing that. For example, we don't support a generic > mechanism to add and remove DT overlays because most drivers aren't > ready for their DT node to disappear. > > Is there some restriction that says platform_bus can't do hotplug? I > thought everything is hotpluggable (in theory). > > > > > > 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? > > Only because platform_bus is the bus type in the kernel that supports > MMIO devices and that the DT code uses to instantiate them. The DT > code doesn't care if those are at the root level or behind some other > bus type. > > > Perhaps we just add support for that to the aux > > bus? > > Yes, we could add IOMEM resources, DT ID table and matching, etc., but > we'd just end up back at of_platform_bus with a new name. Every driver > doing both would have 2 driver structs and register calls. What do we > gain from that? As you know, nothing :) Ok, I'll stop arguing now, maybe this is a valid use of a platform device, but it feels really wrong that such a thing could live below a PCI device that can be removed from the system at any point in time. 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,