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

[Marc Zyngier]

Hi Saravana,

On 2020-05-01 21:23, Saravana Kannan wrote:
> On Fri, May 1, 2020 at 1:48 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
> > On 2020-04-29 20:04, Saravana Kannan wrote:
> > > On Wed, Apr 29, 2020 at 2:28 AM Marc Zyngier <maz@xxxxxxxxxx> 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...