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

[Marc Zyngier]

On Fri, 14 Jul 2023 10:35:34 +0100,
Anup Patel <apatel@xxxxxxxxxxxxxxxx> wrote:
> 
> 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.

Which it doesn't need to be. You are pointlessly making life difficult
for yourself, and everyone else. The MSI bridge behaviour has *zero*
reason to be the same driver as the main "I need it super early"
driver. They may be called the same, but they *are* different things
in the system.

They can share code, but they are fundamentally a different thing in
the system. And I guess this silly approach has other ramifications:
the IMSIC is also some early driver when it really doesn't need to be.
Who needs MSIs that early in the life of the system? I don't buy this
for even a second.

Frankly, this whole thing needs to be taken apart and rebuilt from the
ground up.

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

You can't have your cake and eat it. If needed super early, and it
cannot be a platform driver. End of the story.

And to my earlier point: IMSIC and APLIC-as-MSI-bridge have no purpose
being early drivers. They must be platform drivers, and only that.

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

And yet you are hammering it even when told this is wrong.

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

How about you take a step back and realise that the way you've
architected your drivers makes little sense? I don't think you have
tried *that*.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.