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

[Marc Zyngier]

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.

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.

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.