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@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? 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
