[PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic dt parse in opensbi
Guo Ren
guoren at kernel.org
Sun Jan 30 05:09:21 PST 2022
On Sun, Jan 30, 2022 at 6:28 PM Marc Zyngier <maz at kernel.org> wrote:
>
> On Sun, 30 Jan 2022 04:39:34 +0000,
> Guo Ren <guoren at kernel.org> wrote:
> >
> > On Sun, Jan 30, 2022 at 10:50 AM Samuel Holland <samuel at sholland.org> wrote:
> > >
> > > On 1/29/22 8:08 PM, Guo Ren wrote:
> > > > On Sun, Jan 30, 2022 at 2:32 AM Marc Zyngier <maz at kernel.org> wrote:
> > > >>
> > > >> On Sat, 29 Jan 2022 16:27:26 +0000,
> > > >> guoren at kernel.org wrote:
> > > >>>
> > > >>> From: Guo Ren <guoren at 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 at linux.alibaba.com>
> > > >>> Cc: Anup Patel <anup at brainfault.org>
> > > >>> Cc: Marc Zyngier <maz at kernel.org>
> > > >>> Cc: Palmer Dabbelt <palmer at dabbelt.com>
> > > >>> Cc: Samuel Holland <samuel at sholland.org>
> > > >>> Cc: Thomas Gleixner <tglx at 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:
>
> No. If you can't explain why you need this in the commit message, why
> should I reverse engineer that from some obscure piece of firmware?
I've explaned that in the commit message:
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
If you think the above doesn't satisfy you, please point it out. Then
I can improve the commit-msg.
>
> > > > intc: interrupt-controller at 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.
>
> So the firmware changes things in incompatible ways. Why does it
> matter to Linux?
The opensbi (firmware) reuses dts with the Linux, so PLIC driver
separates into two parts: firmware & linux. For firmware part, we need
another name to distinguish.
> Why isn't the fix directly applied to the firmware instead?
The opensbi patch commit-msg has explained:
The T-HEAD PLIC implementation requires setting a delegation bit to
allow access from S-mode. Now that the T-HEAD PLIC has its own
compatible string, set this bit automatically from the PLIC driver,
instead of reaching into the PLIC's MMIO space from another driver.
If we can't modify the Linux driver, then we must abandon that patch in opensbi.
> Why isn't the riscv,plic0 fallback appropriate?
We can't use riscv,plic0 as the tag of thead,plic, because it's
reserved for past.
>
> > > Yes, some patch is still necessary, because the hardware is indeed incompatible
> > > with riscv,plic0. However, this driver does not care about the difference. So
> > > all you need to do is hook up the existing code to the new compatible:
> > >
> > > +IRQCHIP_DECLARE(thead_c900_plic, "thead,c900-plic", plic_init);
> > I think we should give clear info in /proc/interrupts. I hope we could
> > keep thead_plic_init.
>
> Why? There is no material difference at the driver level, and
> /proc/interrupts won't be the target of a branding exercise (which
> this series seems to be all about).
It's not a branding exercise, the series just correct the broken code
& incorrect /proc/interrupts display.
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
More information about the linux-riscv
mailing list