Re: [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic dt parse in opensbi

[Guo Ren <guoren@kernel.org>]

On Sun, Jan 30, 2022 at 2:32 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Sat, 29 Jan 2022 16:27:26 +0000,
> guoren@kernel.org wrote:
> >
> > From: Guo Ren <guoren@linux.alibaba.com>
> >
> > The thead,c900-plic has been used in opensbi to distinguish
> > PLIC [1]. Although PLICs have the same behaviors in Linux,
> > they are different hardware with some custom initializing in
> > firmware(opensbi).
> >
> > [1]: https://github.com/riscv-software-src/opensbi/commit/78c2b19218bd62653b9fb31623a42ced45f38ea6
> >
> > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > Cc: Anup Patel <anup@brainfault.org>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Palmer Dabbelt <palmer@dabbelt.com>
> > Cc: Samuel Holland <samuel@sholland.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > ---
> >  drivers/irqchip/irq-sifive-plic.c | 25 +++++++++++++++++++++++--
> >  1 file changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > index 259065d271ef..245655928076 100644
> > --- a/drivers/irqchip/irq-sifive-plic.c
> > +++ b/drivers/irqchip/irq-sifive-plic.c
> > @@ -172,7 +172,7 @@ static void plic_irq_eoi(struct irq_data *d)
> >       }
> >  }
> >
> > -static struct irq_chip plic_chip = {
> > +static struct irq_chip sifive_plic_chip = {
> >       .name           = "SiFive PLIC",
> >       .irq_mask       = plic_irq_mask,
> >       .irq_unmask     = plic_irq_unmask,
> > @@ -182,12 +182,24 @@ static struct irq_chip plic_chip = {
> >  #endif
> >  };
> >
> > +static struct irq_chip thead_plic_chip = {
> > +     .name           = "T-Head PLIC",
> > +     .irq_mask       = plic_irq_mask,
> > +     .irq_unmask     = plic_irq_unmask,
> > +     .irq_eoi        = plic_irq_eoi,
> > +#ifdef CONFIG_SMP
> > +     .irq_set_affinity = plic_set_affinity,
> > +#endif
> > +};
>
> For pure entertainment, let's compare the two structures:
>
> static struct irq_chip plic_chip = {
>         .name           = "SiFive PLIC",
>         .irq_mask       = plic_irq_mask,
>         .irq_unmask     = plic_irq_unmask,
>         .irq_eoi        = plic_irq_eoi,
> #ifdef CONFIG_SMP
>         .irq_set_affinity = plic_set_affinity,
> #endif
> };
>
> Oh wait: a string. Must be really important. Not.
No, pls see below comment.

>
> > +
> > +static struct irq_chip *def_plic_chip = &sifive_plic_chip;
> > +
> >  static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
> >                             irq_hw_number_t hwirq)
> >  {
> >       struct plic_priv *priv = d->host_data;
> >
> > -     irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data,
> > +     irq_domain_set_info(d, irq, hwirq, def_plic_chip, d->host_data,
> >                           handle_fasteoi_irq, NULL, NULL);
> >       irq_set_noprobe(irq);
> >       irq_set_affinity(irq, &priv->lmask);
> > @@ -396,5 +408,14 @@ static int __init plic_init(struct device_node *node,
> >       return error;
> >  }
> >
> > +static int __init thead_c900_plic_init(struct device_node *node,
> > +             struct device_node *parent)
> > +{
> > +     def_plic_chip = &thead_plic_chip;
> > +
> > +     return plic_init(node, parent);
> > +}
> > +
> >  IRQCHIP_DECLARE(sifive_plic, "sifive,plic-1.0.0", plic_init);
> >  IRQCHIP_DECLARE(riscv_plic0, "riscv,plic0", plic_init); /* for legacy systems */
> > +IRQCHIP_DECLARE(thead_c900_plic, "thead,c900-plic", thead_c900_plic_init);
>
> Sorry, but I can't see any point to this patch.
You didn't see the link I've put in the patch. In that opensbi patch:

                intc: interrupt-controller@10000000 {
                        #interrupt-cells = <1>;
-                       compatible = "riscv,plic0";
+                       compatible = "allwinner,sun20i-d1-plic",
+                                    "thead,c900-plic";

+#define THEAD_PLIC_CTRL_REG 0x1ffffc
+
+static void thead_plic_plat_init(struct plic_data *pd)
+{
+       writel_relaxed(BIT(0), (void *)pd->addr + THEAD_PLIC_CTRL_REG);
+}
+
 static const struct fdt_match irqchip_plic_match[] = {
        { .compatible = "riscv,plic0" },
        { .compatible = "sifive,plic-1.0.0" },
+       { .compatible = "thead,c900-plic",
+         .data = thead_plic_plat_init },
        { },
 };

We've changed the compatible name for thead,c900-plic, and there is no
riscv,plic0 / sifive,plic-1.0.0 in dts. Without the patch, the newest
opensbi + newest Linux would be broken in the Allwinner D1 dev board.

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



-- 
Best Regards
 Guo Ren

ML: https://lore.kernel.org/linux-csky/