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

[Rob Herring]

On Wed, Jan 25, 2023 at 9:00 AM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx> 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