Re: [PATCH v5 7/9] irqchip: Add RISC-V advanced PLIC driver

[Anup Patel]

On Fri, Jul 14, 2023 at 2:31 PM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> Anup,
>
> On Fri, 14 Jul 2023 00:56:22 +0100,
> Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
> >
> > On Mon, Jul 10, 2023 at 2:44 AM Anup Patel <apatel@xxxxxxxxxxxxxxxx> wrote:
> > >
> > > The RISC-V advanced interrupt architecture (AIA) specification defines
> > > a new interrupt controller for managing wired interrupts on a RISC-V
> > > platform. This new interrupt controller is referred to as advanced
> > > platform-level interrupt controller (APLIC) which can forward wired
> > > interrupts to CPUs (or HARTs) as local interrupts OR as message
> > > signaled interrupts.
> > > (For more details refer https://github.com/riscv/riscv-aia)
> > >
> > > This patch adds an irqchip driver for RISC-V APLIC found on RISC-V
> > > platforms.
> > >
> > > Signed-off-by: Anup Patel <apatel@xxxxxxxxxxxxxxxx>
>
> [...]
>
> > > +static int __init aplic_dt_init(struct device_node *node,
> > > +                               struct device_node *parent)
> > > +{
> > > +       /*
> > > +        * The APLIC platform driver needs to be probed early
> > > +        * so for device tree:
> > > +        *
> > > +        * 1) Set the FWNODE_FLAG_BEST_EFFORT flag in fwnode which
> > > +        *    provides a hint to the device driver core to probe the
> > > +        *    platform driver early.
> > > +        * 2) Clear the OF_POPULATED flag in device_node because
> > > +        *    of_irq_init() sets it which prevents creation of
> > > +        *    platform device.
> > > +        */
> > > +       node->fwnode.flags |= FWNODE_FLAG_BEST_EFFORT;
> >
> > Please stop spamming us with broken patches. Already told you this is
> > not an option.
> >
> > Nack.
>
> What puzzles me here is that *no other arch* requires this sort of
> hack. What is so special about the APLIC that it requires it? I see
> nothing in this patch that even hints at it, despite the "discussion"
> in the last round.
>
> The rules are simple:
>
> - either the APLIC is so fundamental to the system that it has to be
>   initialised super early, much like the GIC on arm64, at which point
>   it cannot be a platform device, and the story is pretty simple.
>
> - or it isn't that fundamental, and it can be probed as a platform
>   device using the dependency infrastructure that is already used by
>   multiple other interrupt controller drivers, without any need to
>   mess with internal flags. Again, this should be simple enough.

APLIC manages all wired interrupts whereas IMSIC manages all
MSIs. Both APLIC and IMSIC are fundamental devices which need
to be probed super early.

Now APLIC has two modes of operations:
1) Direct mode where there is no IMSIC in the system and APLIC
    directly injects interrupt to CPUs
2) MSI mode where IMSIC is present in the system and APLIC
    converts wired interrupts into MSIs

The APLIC driver added by this patch is a common driver for
both above modes.

For #2, APLIC needs to be a platform device to create a device
MSI domain using platform_msi_create_device_domain() which
is why the APLIC driver is a platform driver.

>
> If these rules don't apply to your stuff, please explain what is so
> different. And I mean actually explain the issue. Which isn't telling
> us "it doesn't work without it". Because as things stand, there is no
> way I will even consider taking this ugly mix of probing methods.

Yes, I don't want this ugly FWNODE_FLAG_BEST_EFFORT hack
in this driver.

I tried several things but setting the FWNODE_FLAG_BEST_EFFORT
flag is the only thing which works right now.

>
> In any case, reposting the same stuff ad nauseam is only going to
> result in this series being ignored, which I don't think is what you
> want.
>
> Thanks,
>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.

Regards,
Anup