Messages in this thread | | | Date | Wed, 03 Jun 2020 11:12:49 +0100 | From | Marc Zyngier <> | Subject | Re: [RFC PATCH v1] irqchip: Add IRQCHIP_MODULE_BEGIN/END helper macros |
| |
Hi Saravana,
On 2020-05-01 21:23, Saravana Kannan wrote: > On Fri, May 1, 2020 at 1:48 AM Marc Zyngier <maz@kernel.org> wrote: >> >> On 2020-04-29 20:04, Saravana Kannan wrote: >> > On Wed, Apr 29, 2020 at 2:28 AM Marc Zyngier <maz@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.
By having a config option that forces it on if that option is selected by modular irqchips? More importantly, what is the drawback of having fw_devlink on at all times? It definitely looks like the best thing since sliced bread (with cheese), so what is the catch?
> >> - 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
That would have my preference, provided that there is no drawbacks.
> 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?
That'd be a lot better. We also need some guards for things that cannot be a driver (primary interrupt controllers don't have a struct device).
Thanks,
M. -- Jazz is not dead. It just smells funny...
| |