LinuxLists.cc - [PATCH] mfd: Add Simple PCI MFD driver

[permalink] [raw]

Subject: Re: [PATCH] mfd: Add Simple PCI MFD driver

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>;
};
};
};
};

2023-01-23 16:13:23

[permalink] [raw]

Subject: Re: [PATCH] mfd: Add Simple PCI MFD driver

On Mon, Jan 23, 2023 at 8:32 AM Vincent Whitchurch
<[email protected]> 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 <[email protected]>
> ---
> 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/[email protected]/

2023-01-23 16:31:29

[permalink] [raw]

Subject: Re: [PATCH] mfd: Add Simple PCI MFD driver

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

2023-01-23 16:36:29

[permalink] [raw]

Subject: Re: [PATCH] mfd: Add Simple PCI MFD driver

On Mon, Jan 23, 2023 at 10:02 AM Vincent Whitchurch
<[email protected]> 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

2023-01-24 02:31:10

by Lizhi Hou

[permalink] [raw]

Subject: Re: [PATCH] mfd: Add Simple PCI MFD driver

On 1/23/23 08:36, Rob Herring wrote:
> On Mon, Jan 23, 2023 at 10:02 AM Vincent Whitchurch
> <[email protected]> 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/[email protected]/
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/[email protected]/
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

2023-01-24 12:55:49

[permalink] [raw]

Subject: Re: [PATCH] mfd: Add Simple PCI MFD driver

On Mon, Jan 23, 2023 at 05:13:06PM +0100, Rob Herring wrote:
> On Mon, Jan 23, 2023 at 8:32 AM Vincent Whitchurch
> <[email protected]> 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/[email protected]/
[1] https://lore.kernel.org/lkml/[email protected]/
[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/[email protected]/,
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?

2023-01-24 13:15:27

[permalink] [raw]

Subject: Re: [PATCH] mfd: Add Simple PCI MFD driver

On Mon, Jan 23, 2023 at 05:36:06PM +0100, Rob Herring wrote:
> On Mon, Jan 23, 2023 at 10:02 AM Vincent Whitchurch
> <[email protected]> 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/[email protected]/

>
> > 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.

2023-01-25 10:15:46

[permalink] [raw]

Subject: Re: [PATCH] mfd: Add Simple PCI MFD driver

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.

2023-01-25 12:29:42

[permalink] [raw]

Subject: Re: [PATCH] mfd: Add Simple PCI MFD driver

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

2023-01-25 13:06:36

[permalink] [raw]

Subject: Re: [PATCH] mfd: Add Simple PCI MFD driver

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/[email protected]/

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.

2023-01-25 13:34:11

[permalink] [raw]

Subject: Re: [PATCH] mfd: Add Simple PCI MFD driver

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/[email protected]/

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

2023-01-25 14:54:42

[permalink] [raw]

Subject: Re: [PATCH] mfd: Add Simple PCI MFD driver

On Wed, Jan 25, 2023 at 6:29 AM Greg Kroah-Hartman
<[email protected]> 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

2023-01-25 15:01:02

[permalink] [raw]

Subject: Re: [PATCH] mfd: Add Simple PCI MFD driver

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
> <[email protected]> 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

2023-01-25 15:34:29

[permalink] [raw]

Subject: Re: [PATCH] mfd: Add Simple PCI MFD driver

On Wed, Jan 25, 2023 at 9:00 AM Greg Kroah-Hartman
<[email protected]> 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
> > <[email protected]> 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

2023-01-31 15:11:07