On Fri, May 1, 2020 at 1:23 PM Saravana Kannan <sarava...@google.com> wrote: > > On Fri, May 1, 2020 at 1:48 AM Marc Zyngier <m...@kernel.org> wrote: > > > > On 2020-04-29 20:04, Saravana Kannan wrote: > > > On Wed, Apr 29, 2020 at 2:28 AM Marc Zyngier <m...@kernel.org> wrote: > > > > [...] > > > > >> One thing though: this seems to be exclusively DT driven. Have you > > >> looked into how that would look like for other firmware types such as > > >> ACPI? > > > > > > I'm not very familiar with ACPI at all. I've just started to learn > > > about how it works in the past few months poking at code when I have > > > some time. So I haven't tried to get this to work with ACPI nor do I > > > think I'll be able to do that anytime in the near future. I hope that > > > doesn't block this from being used for DT based platforms. > > > > As long as you don't try to modularise a driver that does both DT and > > ACPI, you'll be safe. I'm also actively trying to discourage people > > from inventing custom irqchips on ACPI platforms (the spec almost > > forbids them, but not quite). > > > > >> Another thing is the handling of dependencies. Statically built > > >> irqchips are initialized in the right order based on the topology > > >> described in DT, and are initialized early enough that client devices > > >> will find their irqchip This doesn't work here, obviously. > > > > > > Yeah, I read that code thoroughly :) > > > > > >> How do you > > >> propose we handle these dependencies, both between irqchip drivers and > > >> client drivers? > > > > > > For client drivers, we don't need to do anything. The IRQ apis seem to > > > already handle -EPROBE_DEFER correctly in this case. > > > > > > For irqchip drivers, the easy answer can be: Load the IRQ modules > > > early if you make them modules. > > > > Uhuh. I'm afraid that's not a practical solution. We need to offer the > > same behaviour for both and not rely on the user to understand the > > topology of the SoC. > > > > > But in my case, I've been testing this with fw_devlink=on. The TL;DR > > > of "fw_devlink=on" in this context is that the IRQ devices will get > > > device links created based on "interrupt-parent" property. So, with > > > the magic of device links, these IRQ devices will probe in the right > > > topological order without any wasted deferred probe attempts. For > > > cases without fw_devlink=on, I think I can improve > > > platform_irqchip_probe() in my patch to check if the parent device has > > > probed and defer if it hasn't. > > > > Seems like an interesting option. Two things then: > > > > - Can we enforce the use of fw_devlink for modularized irqchips? > > fw_devlink doesn't have any config and it's a command line option. So > not sure how you can enforce that. > > > - For those irqchips that can be modularized, it is apparent that they > > should have been written as platform devices the first place. Maybe > > we should just do that (long term, though). > > I agree. If they can be platform devices, they should be. But when > those platform device drivers are built in, you'll either need: > 1) fw_devlink=on to enforce the topological init order > Or > 2) have a generic irqchip probe helper function that ensures that. > My patch with some additional checks added to platform_irqchip_probe() > can provide (2). > > In the short term, my patch series also makes it easier to convert > existing non-platform drivers into platform drivers. > > So if I fix up platform_irqchip_probe() to also do -EPROBE_DEFER to > enforce topology, will that make this patch acceptable? >
Friendly reminder. -Saravana