Re: [PATCH] mfd: Add Simple PCI MFD driver - Greg Kroah-Hartman

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Rob Herring <robh@kernel.org>
Cc: Vincent Whitchurch <vincent.whitchurch@axis.com>,
	Lee Jones <lee@kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	kernel <kernel@axis.com>
Subject: Re: [PATCH] mfd: Add Simple PCI MFD driver
Date: Wed, 25 Jan 2023 16:00:52 +0100	[thread overview]
Message-ID: <Y9FEJAah8y0aY1L2@kroah.com> (raw)
In-Reply-To: <CAL_JsqLGworC4beavkWSk9Uf=qFUR1RtsKBezH2xvop83C15NQ@mail.gmail.com>

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