Re: [PATCH v4 08/10] irqchip: Add RISC-V advanced PLIC driver

[Conor Dooley]

On Thu, Jun 15, 2023 at 01:45:55PM -0700, Saravana Kannan wrote:
> On Thu, Jun 15, 2023 at 12:31 PM Conor Dooley <conor@xxxxxxxxxx> wrote:
> > On Thu, Jun 15, 2023 at 12:17:08PM -0700, Saravana Kannan wrote:
> > > On Tue, Jun 13, 2023 at 8:35 AM Anup Patel <apatel@xxxxxxxxxxxxxxxx> wrote:
> >
> > btw, please try to delete the 100s of lines of unrelated context when
> > replying
> 
> I always feel like some people like me to do this and others don't.
> Also, at times, people might want to reference the other lines of code
> when replying to my point. That's why I generally leave them in.

Yah, perhaps I cull too aggressively but there's a middle ground ;)

> > > > +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;
> > >
> > > NACK. You are blindly plastering flags without trying to understand
> > > the real issue and fixing this correctly.
> > >
> > > > +       of_node_clear_flag(node, OF_POPULATED);
> 
> Also, this part is not needed if the macros I mentioned below are used.
> 
> > > > +       return 0;
> > > > +}
> > > > +IRQCHIP_DECLARE(riscv_aplic, "riscv,aplic", aplic_dt_init);
> > >
> > > This macro pretty much skips the entire driver core framework to probe
> > > and calls init and you are supposed to initialize the device when the
> > > init function is called.
> > >
> > > If you want your device/driver to follow the proper platform driver
> > > path (which is recommended), then you need to use the
> > > IRQCHIP_PLATFORM_DRIVER_BEGIN() and related macros. Grep for plenty of examples.
> > >
> > > I offered to help you debug this issue and I asked for a dts file that
> > > corresponds to a board you are testing this on and seeing an issue.
> >
> > There isn't a dts file for this because there's no publicly available
> > hardware that actually has an APLIC. Maybe Ventana have pre-production
> > silicon that has it, but otherwise it's a QEMU job.
> 
> 1. QEMU example is fine too if it can be reproduced. I just asked for
> a dts file because I need the full global view of the dependencies. At
> a minimum, I'd at least expect to see some example DT and explanation
> of what dependency is causing the IRQ device to not be initialized on
> time, etc. Instead I just see random uses of flags with no description
> of the actual issue.

It's Anup's responsibility to provide you with that information, I have
not reproduced this issue, so I won't mislead you with QEMU invocations
that may not be what's required to reproduce.

> 2. If it's not a dts available upstream, why should these drivers be
> accepted? I thought the norm was to only accept drivers that can
> actually be used.

I think it's not unusual (and desirable?) to start the upstreaming
process for stuff before hardware is publicly available, so that once it
is, support is already upstream, or close to. I do know that people have
tested this series in FPGA based hardware emulation platforms etc.
Posting patches for it also helps avoid duplication of effort between
the various vendors in RISC-V land, who would otherwise have to write
their own drivers. Also, the documented RISC-V policy for accepting
support for ISA stuff says:
	We'll only accept patches for new modules or extensions if the
	specifications for those modules or extensions are listed as being
	unlikely to be incompatibly changed in the future.  For
	specifications from the RISC-V foundation this means "Frozen"
	(Documentation/riscv/patch-acceptance.rst)
AIA (the spec behind the APLIC/IMSIC) is frozen, and qualifies from a
RISC-V point of view. What Marc is willing to accept, in terms of
pre-production hardware support, is up to him obviously!

Cheers,
Conor.

> > > But you haven't answered my question [1] and are pointing to some
> > > random commit and blaming it. That commit has no impact on any
> > > existing devices/drivers.
> > >
> > > Hi Marc,
> > >
> > > Please consider this patch Nacked as long as FWNODE_FLAG_BEST_EFFORT
> > > is used or until Anup actually works with us to debug the real issue.
> > >
> > > -Saravana
> > > [1] - https://lore.kernel.org/lkml/CAAhSdy2p6K70fc2yZLPdVGqEq61Y8F7WVT2J8st5mQrzBi4WHg@xxxxxxxxxxxxxx/

Attachment: signature.asc
Description: PGP signature