From: Guo Ren <[email protected]>
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 <[email protected]>
Cc: Anup Patel <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
Cc: Atish Patra <[email protected]>
---
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;
+
+ 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
On Tue, Oct 12, 2021 at 9:04 PM <[email protected]> wrote:
>
> From: Guo Ren <[email protected]>
>
> 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 <[email protected]>
> Cc: Anup Patel <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> Cc: Palmer Dabbelt <[email protected]>
> Cc: Atish Patra <[email protected]>
>
> ---
>
> 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
Hi,
Am Dienstag, 12. Oktober 2021, 18:40:26 CEST schrieb Anup Patel:
> On Tue, Oct 12, 2021 at 9:04 PM <[email protected]> wrote:
> >
> > From: Guo Ren <[email protected]>
> >
> > 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 <[email protected]>
> > Cc: Anup Patel <[email protected]>
> > Cc: Thomas Gleixner <[email protected]>
> > Cc: Marc Zyngier <[email protected]>
> > Cc: Palmer Dabbelt <[email protected]>
> > Cc: Atish Patra <[email protected]>
> >
> > ---
> >
> > 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/[email protected]/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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
>
On Wed, Oct 13, 2021 at 7:06 AM Heiko Stübner <[email protected]> wrote:
>
> Hi,
>
> Am Dienstag, 12. Oktober 2021, 18:40:26 CEST schrieb Anup Patel:
> > On Tue, Oct 12, 2021 at 9:04 PM <[email protected]> wrote:
> > >
> > > From: Guo Ren <[email protected]>
> > >
> > > 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 <[email protected]>
> > > Cc: Anup Patel <[email protected]>
> > > Cc: Thomas Gleixner <[email protected]>
> > > Cc: Marc Zyngier <[email protected]>
> > > Cc: Palmer Dabbelt <[email protected]>
> > > Cc: Atish Patra <[email protected]>
> > >
> > > ---
> > >
> > > 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/[email protected]/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
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
> >
>
>
>
>
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
On Wed, Oct 13, 2021 at 12:40 AM Anup Patel <[email protected]> wrote:
>
> On Tue, Oct 12, 2021 at 9:04 PM <[email protected]> wrote:
> >
> > From: Guo Ren <[email protected]>
> >
> > 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 <[email protected]>
> > Cc: Anup Patel <[email protected]>
> > Cc: Thomas Gleixner <[email protected]>
> > Cc: Marc Zyngier <[email protected]>
> > Cc: Palmer Dabbelt <[email protected]>
> > Cc: Atish Patra <[email protected]>
> >
> > ---
> >
> > 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/