* Re: [PATCH V2 2/2] irqchip/sifive-plic: Add thead,c9xx-plic support
@ 2021-10-12 16:40 ` Anup Patel
0 siblings, 0 replies; 18+ messages in thread
From: Anup Patel @ 2021-10-12 16:40 UTC (permalink / raw)
To: Guo Ren
Cc: Atish Patra, Marc Zyngier, Thomas Gleixner, Palmer Dabbelt,
linux-kernel@vger.kernel.org List, linux-riscv, Guo Ren
On Tue, Oct 12, 2021 at 9:04 PM <guoren@kernel.org> wrote:
>
> From: Guo Ren <guoren@linux.alibaba.com>
>
> thead,c9xx-plic would mask IRQ with readl(claim), so it needn't
> mask/unmask which needed in RISC-V PLIC.
>
> When in IRQS_ONESHOT & IRQCHIP_EOI_THREADED path, unnecessary mask
> operation would cause a blocking irq bug in thead,c9xx-plic. Because
> when IRQ is disabled in c9xx, writel(hwirq, claim) would be invalid.
>
> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> Cc: Anup Patel <anup@brainfault.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Cc: Atish Patra <atish.patra@wdc.com>
>
> ---
>
> Changes since V2:
> - Add a separate compatible string "thead,c9xx-plic"
> - set irq_mask/unmask of "plic_chip" to NULL and point
> irq_enable/disable of "plic_chip" to plic_irq_mask/unmask
> - Add a detailed comment block in plic_init() about the
> differences in Claim/Completion process of RISC-V PLIC and C9xx
> PLIC.
> ---
> drivers/irqchip/irq-sifive-plic.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> index cf74cfa82045..3756b1c147c3 100644
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -79,6 +79,7 @@ struct plic_handler {
> };
> static int plic_parent_irq __ro_after_init;
> static bool plic_cpuhp_setup_done __ro_after_init;
> +static bool disable_mask_unmask __ro_after_init;
> static DEFINE_PER_CPU(struct plic_handler, plic_handlers);
>
> static inline void plic_toggle(struct plic_handler *handler,
> @@ -181,6 +182,13 @@ static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
> {
> struct plic_priv *priv = d->host_data;
>
> + if (disable_mask_unmask) {
> + plic_chip.irq_mask = NULL;
> + plic_chip.irq_unmask = NULL;
> + plic_chip.irq_enable = plic_irq_unmask;
> + plic_chip.irq_disable = plic_irq_mask;
> + }
> +
> irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data,
> handle_fasteoi_irq, NULL, NULL);
> irq_set_noprobe(irq);
> @@ -390,5 +398,14 @@ static int __init plic_init(struct device_node *node,
> return error;
> }
>
> +static int __init thead_c9xx_plic_init(struct device_node *node,
> + struct device_node *parent)
> +{
> + disable_mask_unmask = true;
The plic_irqdomain_map() is called for each irq so "plic_chip"
will be updated multiple times.
You can drop the disable_mask_unmask variable and instead
directly update "plic_chip" here.
> +
> + 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_c9xx_plic, "thead,c9xx-plic", thead_c9xx_plic_init);
> --
> 2.25.1
>
Regards,
Anup
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH V2 2/2] irqchip/sifive-plic: Add thead,c9xx-plic support
2021-10-12 16:40 ` Anup Patel
@ 2021-10-12 23:06 ` Heiko Stübner
-1 siblings, 0 replies; 18+ messages in thread
From: Heiko Stübner @ 2021-10-12 23:06 UTC (permalink / raw)
To: Guo Ren, linux-riscv
Cc: Atish Patra, Marc Zyngier, Thomas Gleixner, Palmer Dabbelt,
linux-kernel@vger.kernel.org List, linux-riscv, Guo Ren,
Anup Patel
Hi,
Am Dienstag, 12. Oktober 2021, 18:40:26 CEST schrieb Anup Patel:
> On Tue, Oct 12, 2021 at 9:04 PM <guoren@kernel.org> wrote:
> >
> > From: Guo Ren <guoren@linux.alibaba.com>
> >
> > thead,c9xx-plic would mask IRQ with readl(claim), so it needn't
> > mask/unmask which needed in RISC-V PLIC.
> >
> > When in IRQS_ONESHOT & IRQCHIP_EOI_THREADED path, unnecessary mask
> > operation would cause a blocking irq bug in thead,c9xx-plic. Because
> > when IRQ is disabled in c9xx, writel(hwirq, claim) would be invalid.
> >
> > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > Cc: Anup Patel <anup@brainfault.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Palmer Dabbelt <palmer@dabbelt.com>
> > Cc: Atish Patra <atish.patra@wdc.com>
> >
> > ---
> >
> > Changes since V2:
> > - Add a separate compatible string "thead,c9xx-plic"
> > - set irq_mask/unmask of "plic_chip" to NULL and point
> > irq_enable/disable of "plic_chip" to plic_irq_mask/unmask
> > - Add a detailed comment block in plic_init() about the
> > differences in Claim/Completion process of RISC-V PLIC and C9xx
> > PLIC.
> > ---
> > drivers/irqchip/irq-sifive-plic.c | 17 +++++++++++++++++
> > 1 file changed, 17 insertions(+)
> >
> > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > index cf74cfa82045..3756b1c147c3 100644
> > --- a/drivers/irqchip/irq-sifive-plic.c
> > +++ b/drivers/irqchip/irq-sifive-plic.c
> > @@ -79,6 +79,7 @@ struct plic_handler {
> > };
> > static int plic_parent_irq __ro_after_init;
> > static bool plic_cpuhp_setup_done __ro_after_init;
> > +static bool disable_mask_unmask __ro_after_init;
> > static DEFINE_PER_CPU(struct plic_handler, plic_handlers);
> >
> > static inline void plic_toggle(struct plic_handler *handler,
> > @@ -181,6 +182,13 @@ static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
> > {
> > struct plic_priv *priv = d->host_data;
> >
> > + if (disable_mask_unmask) {
> > + plic_chip.irq_mask = NULL;
> > + plic_chip.irq_unmask = NULL;
> > + plic_chip.irq_enable = plic_irq_unmask;
> > + plic_chip.irq_disable = plic_irq_mask;
> > + }
> > +
> > irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data,
> > handle_fasteoi_irq, NULL, NULL);
> > irq_set_noprobe(irq);
> > @@ -390,5 +398,14 @@ static int __init plic_init(struct device_node *node,
> > return error;
> > }
> >
> > +static int __init thead_c9xx_plic_init(struct device_node *node,
> > + struct device_node *parent)
> > +{
> > + disable_mask_unmask = true;
>
> The plic_irqdomain_map() is called for each irq so "plic_chip"
> will be updated multiple times.
>
> You can drop the disable_mask_unmask variable and instead
> directly update "plic_chip" here.
Actually I'd think something more dynamic might be appropriate?
I.e. don't modify the generic plic_chip structure, but define a second
one for this type of chip and reference that one in plic_irqdomain_map
depending on the block found?
According to [0] a system can have multiple PLICs and nothing
guarantees that they'll always be the same variant on future socs
[hardware engineers are very creative]
So adding more stuff that modifies static content used by all PLICs
doesn't really improve driver quality here ;-)
Heiko
[0] https://lore.kernel.org/linux-riscv/1839bf9ef91de2358a7e8ecade361f7a@www.loen.fr/T/
>
> > +
> > + 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_c9xx_plic, "thead,c9xx-plic", thead_c9xx_plic_init);
> > --
> > 2.25.1
> >
>
> Regards,
> Anup
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH V2 2/2] irqchip/sifive-plic: Add thead,c9xx-plic support
@ 2021-10-12 23:06 ` Heiko Stübner
0 siblings, 0 replies; 18+ messages in thread
From: Heiko Stübner @ 2021-10-12 23:06 UTC (permalink / raw)
To: Guo Ren, linux-riscv
Cc: Atish Patra, Marc Zyngier, Thomas Gleixner, Palmer Dabbelt,
linux-kernel@vger.kernel.org List, linux-riscv, Guo Ren,
Anup Patel
Hi,
Am Dienstag, 12. Oktober 2021, 18:40:26 CEST schrieb Anup Patel:
> On Tue, Oct 12, 2021 at 9:04 PM <guoren@kernel.org> wrote:
> >
> > From: Guo Ren <guoren@linux.alibaba.com>
> >
> > thead,c9xx-plic would mask IRQ with readl(claim), so it needn't
> > mask/unmask which needed in RISC-V PLIC.
> >
> > When in IRQS_ONESHOT & IRQCHIP_EOI_THREADED path, unnecessary mask
> > operation would cause a blocking irq bug in thead,c9xx-plic. Because
> > when IRQ is disabled in c9xx, writel(hwirq, claim) would be invalid.
> >
> > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > Cc: Anup Patel <anup@brainfault.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Palmer Dabbelt <palmer@dabbelt.com>
> > Cc: Atish Patra <atish.patra@wdc.com>
> >
> > ---
> >
> > Changes since V2:
> > - Add a separate compatible string "thead,c9xx-plic"
> > - set irq_mask/unmask of "plic_chip" to NULL and point
> > irq_enable/disable of "plic_chip" to plic_irq_mask/unmask
> > - Add a detailed comment block in plic_init() about the
> > differences in Claim/Completion process of RISC-V PLIC and C9xx
> > PLIC.
> > ---
> > drivers/irqchip/irq-sifive-plic.c | 17 +++++++++++++++++
> > 1 file changed, 17 insertions(+)
> >
> > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > index cf74cfa82045..3756b1c147c3 100644
> > --- a/drivers/irqchip/irq-sifive-plic.c
> > +++ b/drivers/irqchip/irq-sifive-plic.c
> > @@ -79,6 +79,7 @@ struct plic_handler {
> > };
> > static int plic_parent_irq __ro_after_init;
> > static bool plic_cpuhp_setup_done __ro_after_init;
> > +static bool disable_mask_unmask __ro_after_init;
> > static DEFINE_PER_CPU(struct plic_handler, plic_handlers);
> >
> > static inline void plic_toggle(struct plic_handler *handler,
> > @@ -181,6 +182,13 @@ static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
> > {
> > struct plic_priv *priv = d->host_data;
> >
> > + if (disable_mask_unmask) {
> > + plic_chip.irq_mask = NULL;
> > + plic_chip.irq_unmask = NULL;
> > + plic_chip.irq_enable = plic_irq_unmask;
> > + plic_chip.irq_disable = plic_irq_mask;
> > + }
> > +
> > irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data,
> > handle_fasteoi_irq, NULL, NULL);
> > irq_set_noprobe(irq);
> > @@ -390,5 +398,14 @@ static int __init plic_init(struct device_node *node,
> > return error;
> > }
> >
> > +static int __init thead_c9xx_plic_init(struct device_node *node,
> > + struct device_node *parent)
> > +{
> > + disable_mask_unmask = true;
>
> The plic_irqdomain_map() is called for each irq so "plic_chip"
> will be updated multiple times.
>
> You can drop the disable_mask_unmask variable and instead
> directly update "plic_chip" here.
Actually I'd think something more dynamic might be appropriate?
I.e. don't modify the generic plic_chip structure, but define a second
one for this type of chip and reference that one in plic_irqdomain_map
depending on the block found?
According to [0] a system can have multiple PLICs and nothing
guarantees that they'll always be the same variant on future socs
[hardware engineers are very creative]
So adding more stuff that modifies static content used by all PLICs
doesn't really improve driver quality here ;-)
Heiko
[0] https://lore.kernel.org/linux-riscv/1839bf9ef91de2358a7e8ecade361f7a@www.loen.fr/T/
>
> > +
> > + 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_c9xx_plic, "thead,c9xx-plic", thead_c9xx_plic_init);
> > --
> > 2.25.1
> >
>
> Regards,
> Anup
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
>
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH V2 2/2] irqchip/sifive-plic: Add thead,c9xx-plic support
2021-10-12 23:06 ` Heiko Stübner
@ 2021-10-13 0:47 ` Guo Ren
-1 siblings, 0 replies; 18+ messages in thread
From: Guo Ren @ 2021-10-13 0:47 UTC (permalink / raw)
To: Heiko Stübner
Cc: linux-riscv, Atish Patra, Marc Zyngier, Thomas Gleixner,
Palmer Dabbelt, linux-kernel@vger.kernel.org List, Guo Ren,
Anup Patel
On Wed, Oct 13, 2021 at 7:06 AM Heiko Stübner <heiko@sntech.de> wrote:
>
> Hi,
>
> Am Dienstag, 12. Oktober 2021, 18:40:26 CEST schrieb Anup Patel:
> > On Tue, Oct 12, 2021 at 9:04 PM <guoren@kernel.org> wrote:
> > >
> > > From: Guo Ren <guoren@linux.alibaba.com>
> > >
> > > thead,c9xx-plic would mask IRQ with readl(claim), so it needn't
> > > mask/unmask which needed in RISC-V PLIC.
> > >
> > > When in IRQS_ONESHOT & IRQCHIP_EOI_THREADED path, unnecessary mask
> > > operation would cause a blocking irq bug in thead,c9xx-plic. Because
> > > when IRQ is disabled in c9xx, writel(hwirq, claim) would be invalid.
> > >
> > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > > Cc: Anup Patel <anup@brainfault.org>
> > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > Cc: Marc Zyngier <maz@kernel.org>
> > > Cc: Palmer Dabbelt <palmer@dabbelt.com>
> > > Cc: Atish Patra <atish.patra@wdc.com>
> > >
> > > ---
> > >
> > > Changes since V2:
> > > - Add a separate compatible string "thead,c9xx-plic"
> > > - set irq_mask/unmask of "plic_chip" to NULL and point
> > > irq_enable/disable of "plic_chip" to plic_irq_mask/unmask
> > > - Add a detailed comment block in plic_init() about the
> > > differences in Claim/Completion process of RISC-V PLIC and C9xx
> > > PLIC.
> > > ---
> > > drivers/irqchip/irq-sifive-plic.c | 17 +++++++++++++++++
> > > 1 file changed, 17 insertions(+)
> > >
> > > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > > index cf74cfa82045..3756b1c147c3 100644
> > > --- a/drivers/irqchip/irq-sifive-plic.c
> > > +++ b/drivers/irqchip/irq-sifive-plic.c
> > > @@ -79,6 +79,7 @@ struct plic_handler {
> > > };
> > > static int plic_parent_irq __ro_after_init;
> > > static bool plic_cpuhp_setup_done __ro_after_init;
> > > +static bool disable_mask_unmask __ro_after_init;
> > > static DEFINE_PER_CPU(struct plic_handler, plic_handlers);
> > >
> > > static inline void plic_toggle(struct plic_handler *handler,
> > > @@ -181,6 +182,13 @@ static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
> > > {
> > > struct plic_priv *priv = d->host_data;
> > >
> > > + if (disable_mask_unmask) {
> > > + plic_chip.irq_mask = NULL;
> > > + plic_chip.irq_unmask = NULL;
> > > + plic_chip.irq_enable = plic_irq_unmask;
> > > + plic_chip.irq_disable = plic_irq_mask;
> > > + }
> > > +
> > > irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data,
> > > handle_fasteoi_irq, NULL, NULL);
> > > irq_set_noprobe(irq);
> > > @@ -390,5 +398,14 @@ static int __init plic_init(struct device_node *node,
> > > return error;
> > > }
> > >
> > > +static int __init thead_c9xx_plic_init(struct device_node *node,
> > > + struct device_node *parent)
> > > +{
> > > + disable_mask_unmask = true;
> >
> > The plic_irqdomain_map() is called for each irq so "plic_chip"
> > will be updated multiple times.
> >
> > You can drop the disable_mask_unmask variable and instead
> > directly update "plic_chip" here.
>
> Actually I'd think something more dynamic might be appropriate?
>
> I.e. don't modify the generic plic_chip structure, but define a second
> one for this type of chip and reference that one in plic_irqdomain_map
> depending on the block found?
>
> According to [0] a system can have multiple PLICs and nothing
> guarantees that they'll always be the same variant on future socs
> [hardware engineers are very creative]
>
> So adding more stuff that modifies static content used by all PLICs
> doesn't really improve driver quality here ;-)
Agree, I like your style because it also could make cat
/proc/interrupts name properly.
static struct irq_chip sifive_plic_chip = {
.name = "SiFive PLIC",
static struct irq_chip thead_plic_chip = {
.name = "T-HEAD PLIC",
>
>
> Heiko
>
>
> [0] https://lore.kernel.org/linux-riscv/1839bf9ef91de2358a7e8ecade361f7a@www.loen.fr/T/
>
>
> >
> > > +
> > > + 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_c9xx_plic, "thead,c9xx-plic", thead_c9xx_plic_init);
> > > --
> > > 2.25.1
> > >
> >
> > Regards,
> > Anup
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
> >
>
>
>
>
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH V2 2/2] irqchip/sifive-plic: Add thead,c9xx-plic support
@ 2021-10-13 0:47 ` Guo Ren
0 siblings, 0 replies; 18+ messages in thread
From: Guo Ren @ 2021-10-13 0:47 UTC (permalink / raw)
To: Heiko Stübner
Cc: linux-riscv, Atish Patra, Marc Zyngier, Thomas Gleixner,
Palmer Dabbelt, linux-kernel@vger.kernel.org List, Guo Ren,
Anup Patel
On Wed, Oct 13, 2021 at 7:06 AM Heiko Stübner <heiko@sntech.de> wrote:
>
> Hi,
>
> Am Dienstag, 12. Oktober 2021, 18:40:26 CEST schrieb Anup Patel:
> > On Tue, Oct 12, 2021 at 9:04 PM <guoren@kernel.org> wrote:
> > >
> > > From: Guo Ren <guoren@linux.alibaba.com>
> > >
> > > thead,c9xx-plic would mask IRQ with readl(claim), so it needn't
> > > mask/unmask which needed in RISC-V PLIC.
> > >
> > > When in IRQS_ONESHOT & IRQCHIP_EOI_THREADED path, unnecessary mask
> > > operation would cause a blocking irq bug in thead,c9xx-plic. Because
> > > when IRQ is disabled in c9xx, writel(hwirq, claim) would be invalid.
> > >
> > > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > > Cc: Anup Patel <anup@brainfault.org>
> > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > Cc: Marc Zyngier <maz@kernel.org>
> > > Cc: Palmer Dabbelt <palmer@dabbelt.com>
> > > Cc: Atish Patra <atish.patra@wdc.com>
> > >
> > > ---
> > >
> > > Changes since V2:
> > > - Add a separate compatible string "thead,c9xx-plic"
> > > - set irq_mask/unmask of "plic_chip" to NULL and point
> > > irq_enable/disable of "plic_chip" to plic_irq_mask/unmask
> > > - Add a detailed comment block in plic_init() about the
> > > differences in Claim/Completion process of RISC-V PLIC and C9xx
> > > PLIC.
> > > ---
> > > drivers/irqchip/irq-sifive-plic.c | 17 +++++++++++++++++
> > > 1 file changed, 17 insertions(+)
> > >
> > > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > > index cf74cfa82045..3756b1c147c3 100644
> > > --- a/drivers/irqchip/irq-sifive-plic.c
> > > +++ b/drivers/irqchip/irq-sifive-plic.c
> > > @@ -79,6 +79,7 @@ struct plic_handler {
> > > };
> > > static int plic_parent_irq __ro_after_init;
> > > static bool plic_cpuhp_setup_done __ro_after_init;
> > > +static bool disable_mask_unmask __ro_after_init;
> > > static DEFINE_PER_CPU(struct plic_handler, plic_handlers);
> > >
> > > static inline void plic_toggle(struct plic_handler *handler,
> > > @@ -181,6 +182,13 @@ static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
> > > {
> > > struct plic_priv *priv = d->host_data;
> > >
> > > + if (disable_mask_unmask) {
> > > + plic_chip.irq_mask = NULL;
> > > + plic_chip.irq_unmask = NULL;
> > > + plic_chip.irq_enable = plic_irq_unmask;
> > > + plic_chip.irq_disable = plic_irq_mask;
> > > + }
> > > +
> > > irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data,
> > > handle_fasteoi_irq, NULL, NULL);
> > > irq_set_noprobe(irq);
> > > @@ -390,5 +398,14 @@ static int __init plic_init(struct device_node *node,
> > > return error;
> > > }
> > >
> > > +static int __init thead_c9xx_plic_init(struct device_node *node,
> > > + struct device_node *parent)
> > > +{
> > > + disable_mask_unmask = true;
> >
> > The plic_irqdomain_map() is called for each irq so "plic_chip"
> > will be updated multiple times.
> >
> > You can drop the disable_mask_unmask variable and instead
> > directly update "plic_chip" here.
>
> Actually I'd think something more dynamic might be appropriate?
>
> I.e. don't modify the generic plic_chip structure, but define a second
> one for this type of chip and reference that one in plic_irqdomain_map
> depending on the block found?
>
> According to [0] a system can have multiple PLICs and nothing
> guarantees that they'll always be the same variant on future socs
> [hardware engineers are very creative]
>
> So adding more stuff that modifies static content used by all PLICs
> doesn't really improve driver quality here ;-)
Agree, I like your style because it also could make cat
/proc/interrupts name properly.
static struct irq_chip sifive_plic_chip = {
.name = "SiFive PLIC",
static struct irq_chip thead_plic_chip = {
.name = "T-HEAD PLIC",
>
>
> Heiko
>
>
> [0] https://lore.kernel.org/linux-riscv/1839bf9ef91de2358a7e8ecade361f7a@www.loen.fr/T/
>
>
> >
> > > +
> > > + 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_c9xx_plic, "thead,c9xx-plic", thead_c9xx_plic_init);
> > > --
> > > 2.25.1
> > >
> >
> > Regards,
> > Anup
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
> >
>
>
>
>
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH V2 2/2] irqchip/sifive-plic: Add thead,c9xx-plic support
2021-10-12 16:40 ` Anup Patel
@ 2021-10-13 0:48 ` Guo Ren
-1 siblings, 0 replies; 18+ messages in thread
From: Guo Ren @ 2021-10-13 0:48 UTC (permalink / raw)
To: Anup Patel
Cc: Atish Patra, Marc Zyngier, Thomas Gleixner, Palmer Dabbelt,
linux-kernel@vger.kernel.org List, linux-riscv, Guo Ren
On Wed, Oct 13, 2021 at 12:40 AM Anup Patel <anup@brainfault.org> wrote:
>
> On Tue, Oct 12, 2021 at 9:04 PM <guoren@kernel.org> wrote:
> >
> > From: Guo Ren <guoren@linux.alibaba.com>
> >
> > thead,c9xx-plic would mask IRQ with readl(claim), so it needn't
> > mask/unmask which needed in RISC-V PLIC.
> >
> > When in IRQS_ONESHOT & IRQCHIP_EOI_THREADED path, unnecessary mask
> > operation would cause a blocking irq bug in thead,c9xx-plic. Because
> > when IRQ is disabled in c9xx, writel(hwirq, claim) would be invalid.
> >
> > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > Cc: Anup Patel <anup@brainfault.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Palmer Dabbelt <palmer@dabbelt.com>
> > Cc: Atish Patra <atish.patra@wdc.com>
> >
> > ---
> >
> > Changes since V2:
> > - Add a separate compatible string "thead,c9xx-plic"
> > - set irq_mask/unmask of "plic_chip" to NULL and point
> > irq_enable/disable of "plic_chip" to plic_irq_mask/unmask
> > - Add a detailed comment block in plic_init() about the
> > differences in Claim/Completion process of RISC-V PLIC and C9xx
> > PLIC.
> > ---
> > drivers/irqchip/irq-sifive-plic.c | 17 +++++++++++++++++
> > 1 file changed, 17 insertions(+)
> >
> > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > index cf74cfa82045..3756b1c147c3 100644
> > --- a/drivers/irqchip/irq-sifive-plic.c
> > +++ b/drivers/irqchip/irq-sifive-plic.c
> > @@ -79,6 +79,7 @@ struct plic_handler {
> > };
> > static int plic_parent_irq __ro_after_init;
> > static bool plic_cpuhp_setup_done __ro_after_init;
> > +static bool disable_mask_unmask __ro_after_init;
> > static DEFINE_PER_CPU(struct plic_handler, plic_handlers);
> >
> > static inline void plic_toggle(struct plic_handler *handler,
> > @@ -181,6 +182,13 @@ static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
> > {
> > struct plic_priv *priv = d->host_data;
> >
> > + if (disable_mask_unmask) {
> > + plic_chip.irq_mask = NULL;
> > + plic_chip.irq_unmask = NULL;
> > + plic_chip.irq_enable = plic_irq_unmask;
> > + plic_chip.irq_disable = plic_irq_mask;
> > + }
> > +
> > irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data,
> > handle_fasteoi_irq, NULL, NULL);
> > irq_set_noprobe(irq);
> > @@ -390,5 +398,14 @@ static int __init plic_init(struct device_node *node,
> > return error;
> > }
> >
> > +static int __init thead_c9xx_plic_init(struct device_node *node,
> > + struct device_node *parent)
> > +{
> > + disable_mask_unmask = true;
>
> The plic_irqdomain_map() is called for each irq so "plic_chip"
> will be updated multiple times.
>
> You can drop the disable_mask_unmask variable and instead
> directly update "plic_chip" here.
Good advice, better than mine.
>
> > +
> > + 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_c9xx_plic, "thead,c9xx-plic", thead_c9xx_plic_init);
> > --
> > 2.25.1
> >
>
> Regards,
> Anup
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH V2 2/2] irqchip/sifive-plic: Add thead,c9xx-plic support
@ 2021-10-13 0:48 ` Guo Ren
0 siblings, 0 replies; 18+ messages in thread
From: Guo Ren @ 2021-10-13 0:48 UTC (permalink / raw)
To: Anup Patel
Cc: Atish Patra, Marc Zyngier, Thomas Gleixner, Palmer Dabbelt,
linux-kernel@vger.kernel.org List, linux-riscv, Guo Ren
On Wed, Oct 13, 2021 at 12:40 AM Anup Patel <anup@brainfault.org> wrote:
>
> On Tue, Oct 12, 2021 at 9:04 PM <guoren@kernel.org> wrote:
> >
> > From: Guo Ren <guoren@linux.alibaba.com>
> >
> > thead,c9xx-plic would mask IRQ with readl(claim), so it needn't
> > mask/unmask which needed in RISC-V PLIC.
> >
> > When in IRQS_ONESHOT & IRQCHIP_EOI_THREADED path, unnecessary mask
> > operation would cause a blocking irq bug in thead,c9xx-plic. Because
> > when IRQ is disabled in c9xx, writel(hwirq, claim) would be invalid.
> >
> > Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> > Cc: Anup Patel <anup@brainfault.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Palmer Dabbelt <palmer@dabbelt.com>
> > Cc: Atish Patra <atish.patra@wdc.com>
> >
> > ---
> >
> > Changes since V2:
> > - Add a separate compatible string "thead,c9xx-plic"
> > - set irq_mask/unmask of "plic_chip" to NULL and point
> > irq_enable/disable of "plic_chip" to plic_irq_mask/unmask
> > - Add a detailed comment block in plic_init() about the
> > differences in Claim/Completion process of RISC-V PLIC and C9xx
> > PLIC.
> > ---
> > drivers/irqchip/irq-sifive-plic.c | 17 +++++++++++++++++
> > 1 file changed, 17 insertions(+)
> >
> > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > index cf74cfa82045..3756b1c147c3 100644
> > --- a/drivers/irqchip/irq-sifive-plic.c
> > +++ b/drivers/irqchip/irq-sifive-plic.c
> > @@ -79,6 +79,7 @@ struct plic_handler {
> > };
> > static int plic_parent_irq __ro_after_init;
> > static bool plic_cpuhp_setup_done __ro_after_init;
> > +static bool disable_mask_unmask __ro_after_init;
> > static DEFINE_PER_CPU(struct plic_handler, plic_handlers);
> >
> > static inline void plic_toggle(struct plic_handler *handler,
> > @@ -181,6 +182,13 @@ static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
> > {
> > struct plic_priv *priv = d->host_data;
> >
> > + if (disable_mask_unmask) {
> > + plic_chip.irq_mask = NULL;
> > + plic_chip.irq_unmask = NULL;
> > + plic_chip.irq_enable = plic_irq_unmask;
> > + plic_chip.irq_disable = plic_irq_mask;
> > + }
> > +
> > irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data,
> > handle_fasteoi_irq, NULL, NULL);
> > irq_set_noprobe(irq);
> > @@ -390,5 +398,14 @@ static int __init plic_init(struct device_node *node,
> > return error;
> > }
> >
> > +static int __init thead_c9xx_plic_init(struct device_node *node,
> > + struct device_node *parent)
> > +{
> > + disable_mask_unmask = true;
>
> The plic_irqdomain_map() is called for each irq so "plic_chip"
> will be updated multiple times.
>
> You can drop the disable_mask_unmask variable and instead
> directly update "plic_chip" here.
Good advice, better than mine.
>
> > +
> > + 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_c9xx_plic, "thead,c9xx-plic", thead_c9xx_plic_init);
> > --
> > 2.25.1
> >
>
> Regards,
> Anup
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 18+ messages in thread