Re: [RFC PATCH v1] irqchip: Add IRQCHIP_MODULE_BEGIN/END helper macros

[Saravana Kannan <saravanak@google.com>]

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.

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

-Saravana