| Message ID | 20200516063901.18365-4-anup.patel@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | More improvements for multiple PLICs | expand |
On 2020-05-16 07:39, Anup Patel wrote: > To distinguish interrupts from multiple PLIC instances, we use a > per-PLIC irq_chip instance with a different name. > > Signed-off-by: Anup Patel <anup.patel@wdc.com> > --- > drivers/irqchip/irq-sifive-plic.c | 28 +++++++++++++++------------- > 1 file changed, 15 insertions(+), 13 deletions(-) > > diff --git a/drivers/irqchip/irq-sifive-plic.c > b/drivers/irqchip/irq-sifive-plic.c > index 2d3db927a551..e42fc082ad18 100644 > --- a/drivers/irqchip/irq-sifive-plic.c > +++ b/drivers/irqchip/irq-sifive-plic.c > @@ -60,6 +60,7 @@ > #define PLIC_ENABLE_THRESHOLD 0 > > struct plic_priv { > + struct irq_chip chip; > struct cpumask lmask; > struct irq_domain *irqdomain; > void __iomem *regs; > @@ -76,6 +77,7 @@ struct plic_handler { > void __iomem *enable_base; > struct plic_priv *priv; > }; > +static unsigned int plic_count; > static bool plic_cpuhp_setup_done; > static DEFINE_PER_CPU(struct plic_handler, plic_handlers); > > @@ -164,20 +166,12 @@ static void plic_irq_eoi(struct irq_data *d) > writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM); > } > > -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 > -}; > - > static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq, > irq_hw_number_t hwirq) > { > - irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data, > + struct plic_priv *priv = d->host_data; > + > + irq_domain_set_info(d, irq, hwirq, &priv->chip, d->host_data, > handle_fasteoi_irq, NULL, NULL); > irq_set_noprobe(irq); > return 0; > @@ -294,6 +288,14 @@ static int __init plic_init(struct device_node > *node, > if (!priv) > return -ENOMEM; > > + priv->chip.name = kasprintf(GFP_KERNEL, "PLIC%d", plic_count++); > + priv->chip.irq_mask = plic_irq_mask, > + priv->chip.irq_unmask = plic_irq_unmask, > + priv->chip.irq_eoi = plic_irq_eoi, > +#ifdef CONFIG_SMP > + priv->chip.irq_set_affinity = plic_set_affinity, > +#endif > + > priv->regs = of_iomap(node, 0); > if (WARN_ON(!priv->regs)) { > error = -EIO; > @@ -383,9 +385,9 @@ static int __init plic_init(struct device_node > *node, > } > > pr_info("interrupt-controller at 0x%llx " > - "(interrupts=%d, contexts=%d, handlers=%d)\n", > + "(interrupts=%d, contexts=%d, handlers=%d) (%s)\n", > (unsigned long long)iores.start, nr_irqs, > - nr_contexts, nr_handlers); > + nr_contexts, nr_handlers, priv->chip.name); > set_handle_irq(plic_handle_irq); > return 0; I really dislike this patch for multiple reasons: - Allocating a new struc irq_chip just for a string seems over the top, specially as all the *useful* stuff stays the same. - Even if I hate it, /proc is API. I'm sure something, somewhere is parsing this. Changing the string is likely to confuse it. - If you do this for debug purposes, then CONFIG_GENERIC_IRQ_DEBUGFS is the right way to look up the information. - If, for reasons that are beyond me, you actually *need* this, then implementing irq_print_chip in your irq_chip structure is the way to go. But frankly, I'd rather you drop this altogether. Thanks, M.
> -----Original Message----- > From: Marc Zyngier <maz@kernel.org> > Sent: 16 May 2020 17:59 > To: Anup Patel <Anup.Patel@wdc.com> > Cc: Palmer Dabbelt <palmer@dabbelt.com>; Paul Walmsley > <paul.walmsley@sifive.com>; Thomas Gleixner <tglx@linutronix.de>; Jason > Cooper <jason@lakedaemon.net>; Atish Patra <Atish.Patra@wdc.com>; Alistair > Francis <Alistair.Francis@wdc.com>; Anup Patel <anup@brainfault.org>; linux- > riscv@lists.infradead.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH 3/4] irqchip/sifive-plic: Separate irq_chip for muiltiple PLIC > instances > > On 2020-05-16 07:39, Anup Patel wrote: > > To distinguish interrupts from multiple PLIC instances, we use a > > per-PLIC irq_chip instance with a different name. > > > > Signed-off-by: Anup Patel <anup.patel@wdc.com> > > --- > > drivers/irqchip/irq-sifive-plic.c | 28 +++++++++++++++------------- > > 1 file changed, 15 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/irqchip/irq-sifive-plic.c > > b/drivers/irqchip/irq-sifive-plic.c > > index 2d3db927a551..e42fc082ad18 100644 > > --- a/drivers/irqchip/irq-sifive-plic.c > > +++ b/drivers/irqchip/irq-sifive-plic.c > > @@ -60,6 +60,7 @@ > > #define PLIC_ENABLE_THRESHOLD 0 > > > > struct plic_priv { > > + struct irq_chip chip; > > struct cpumask lmask; > > struct irq_domain *irqdomain; > > void __iomem *regs; > > @@ -76,6 +77,7 @@ struct plic_handler { > > void __iomem *enable_base; > > struct plic_priv *priv; > > }; > > +static unsigned int plic_count; > > static bool plic_cpuhp_setup_done; > > static DEFINE_PER_CPU(struct plic_handler, plic_handlers); > > > > @@ -164,20 +166,12 @@ static void plic_irq_eoi(struct irq_data *d) > > writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM); } > > > > -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 > > -}; > > - > > static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq, > > irq_hw_number_t hwirq) > > { > > - irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data, > > + struct plic_priv *priv = d->host_data; > > + > > + irq_domain_set_info(d, irq, hwirq, &priv->chip, d->host_data, > > handle_fasteoi_irq, NULL, NULL); > > irq_set_noprobe(irq); > > return 0; > > @@ -294,6 +288,14 @@ static int __init plic_init(struct device_node > > *node, > > if (!priv) > > return -ENOMEM; > > > > + priv->chip.name = kasprintf(GFP_KERNEL, "PLIC%d", plic_count++); > > + priv->chip.irq_mask = plic_irq_mask, > > + priv->chip.irq_unmask = plic_irq_unmask, > > + priv->chip.irq_eoi = plic_irq_eoi, > > +#ifdef CONFIG_SMP > > + priv->chip.irq_set_affinity = plic_set_affinity, #endif > > + > > priv->regs = of_iomap(node, 0); > > if (WARN_ON(!priv->regs)) { > > error = -EIO; > > @@ -383,9 +385,9 @@ static int __init plic_init(struct device_node > > *node, > > } > > > > pr_info("interrupt-controller at 0x%llx " > > - "(interrupts=%d, contexts=%d, handlers=%d)\n", > > + "(interrupts=%d, contexts=%d, handlers=%d) (%s)\n", > > (unsigned long long)iores.start, nr_irqs, > > - nr_contexts, nr_handlers); > > + nr_contexts, nr_handlers, priv->chip.name); > > set_handle_irq(plic_handle_irq); > > return 0; > > I really dislike this patch for multiple reasons: > > - Allocating a new struc irq_chip just for a string seems over the top, > specially as all the *useful* stuff stays the same. > > - Even if I hate it, /proc is API. I'm sure something, somewhere is > parsing this. Changing the string is likely to confuse it. AFAIK, we don't have scripts in RISC-V world that depend on /proc/interrupts content. May be in future such scripts will show up. For system with multiple PLICs, we are seeing same "SiFive PLIC" string for all PLIC interrupts in "cat /proc/interrupts". I am trying to assign different string based on PLIC instance. This is similar to what GICv2 driver is doing (e.g. GIC-0, GIC-1, ... in /proc/interrupts). Is there a better way to do this ? > > - If you do this for debug purposes, then CONFIG_GENERIC_IRQ_DEBUGFS > is the right way to look up the information. > > - If, for reasons that are beyond me, you actually *need* this, then > implementing irq_print_chip in your irq_chip structure is the way > to go. > > But frankly, I'd rather you drop this altogether. I just want to differentiate which interrupt belongs to which PLIC Instance in /proc/interrupts. I can take a different approach if you suggest. Regards, Anup
On 2020-05-16 14:01, Anup Patel wrote: >> -----Original Message----- >> From: Marc Zyngier <maz@kernel.org> >> Sent: 16 May 2020 17:59 >> To: Anup Patel <Anup.Patel@wdc.com> >> Cc: Palmer Dabbelt <palmer@dabbelt.com>; Paul Walmsley >> <paul.walmsley@sifive.com>; Thomas Gleixner <tglx@linutronix.de>; >> Jason >> Cooper <jason@lakedaemon.net>; Atish Patra <Atish.Patra@wdc.com>; >> Alistair >> Francis <Alistair.Francis@wdc.com>; Anup Patel <anup@brainfault.org>; >> linux- >> riscv@lists.infradead.org; linux-kernel@vger.kernel.org >> Subject: Re: [PATCH 3/4] irqchip/sifive-plic: Separate irq_chip for >> muiltiple PLIC >> instances >> >> On 2020-05-16 07:39, Anup Patel wrote: >> > To distinguish interrupts from multiple PLIC instances, we use a >> > per-PLIC irq_chip instance with a different name. >> > >> > Signed-off-by: Anup Patel <anup.patel@wdc.com> >> > --- >> > drivers/irqchip/irq-sifive-plic.c | 28 +++++++++++++++------------- >> > 1 file changed, 15 insertions(+), 13 deletions(-) >> > >> > diff --git a/drivers/irqchip/irq-sifive-plic.c >> > b/drivers/irqchip/irq-sifive-plic.c >> > index 2d3db927a551..e42fc082ad18 100644 >> > --- a/drivers/irqchip/irq-sifive-plic.c >> > +++ b/drivers/irqchip/irq-sifive-plic.c >> > @@ -60,6 +60,7 @@ >> > #define PLIC_ENABLE_THRESHOLD 0 >> > >> > struct plic_priv { >> > + struct irq_chip chip; >> > struct cpumask lmask; >> > struct irq_domain *irqdomain; >> > void __iomem *regs; >> > @@ -76,6 +77,7 @@ struct plic_handler { >> > void __iomem *enable_base; >> > struct plic_priv *priv; >> > }; >> > +static unsigned int plic_count; >> > static bool plic_cpuhp_setup_done; >> > static DEFINE_PER_CPU(struct plic_handler, plic_handlers); >> > >> > @@ -164,20 +166,12 @@ static void plic_irq_eoi(struct irq_data *d) >> > writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM); } >> > >> > -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 >> > -}; >> > - >> > static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq, >> > irq_hw_number_t hwirq) >> > { >> > - irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data, >> > + struct plic_priv *priv = d->host_data; >> > + >> > + irq_domain_set_info(d, irq, hwirq, &priv->chip, d->host_data, >> > handle_fasteoi_irq, NULL, NULL); >> > irq_set_noprobe(irq); >> > return 0; >> > @@ -294,6 +288,14 @@ static int __init plic_init(struct device_node >> > *node, >> > if (!priv) >> > return -ENOMEM; >> > >> > + priv->chip.name = kasprintf(GFP_KERNEL, "PLIC%d", plic_count++); >> > + priv->chip.irq_mask = plic_irq_mask, >> > + priv->chip.irq_unmask = plic_irq_unmask, >> > + priv->chip.irq_eoi = plic_irq_eoi, >> > +#ifdef CONFIG_SMP >> > + priv->chip.irq_set_affinity = plic_set_affinity, #endif >> > + >> > priv->regs = of_iomap(node, 0); >> > if (WARN_ON(!priv->regs)) { >> > error = -EIO; >> > @@ -383,9 +385,9 @@ static int __init plic_init(struct device_node >> > *node, >> > } >> > >> > pr_info("interrupt-controller at 0x%llx " >> > - "(interrupts=%d, contexts=%d, handlers=%d)\n", >> > + "(interrupts=%d, contexts=%d, handlers=%d) (%s)\n", >> > (unsigned long long)iores.start, nr_irqs, >> > - nr_contexts, nr_handlers); >> > + nr_contexts, nr_handlers, priv->chip.name); >> > set_handle_irq(plic_handle_irq); >> > return 0; >> >> I really dislike this patch for multiple reasons: >> >> - Allocating a new struc irq_chip just for a string seems over the >> top, >> specially as all the *useful* stuff stays the same. >> >> - Even if I hate it, /proc is API. I'm sure something, somewhere is >> parsing this. Changing the string is likely to confuse it. > > AFAIK, we don't have scripts in RISC-V world that depend on > /proc/interrupts content. May be in future such scripts will show up. How do you know that? Do you keep an exhaustive repository of all the possible parsers of /proc/cpuinfo (rhetorical question)? > For system with multiple PLICs, we are seeing same "SiFive PLIC" > string for all PLIC interrupts in "cat /proc/interrupts". I am trying > to > assign different string based on PLIC instance. This is similar to > what GICv2 driver is doing (e.g. GIC-0, GIC-1, ... in > /proc/interrupts). Which was a *very* bad idea the first place, and I wish I could get rid of it. I cannot, for the reason outlined above (it's ABI). Furthermore, in this case, the GICs are different (they are cascaded). In your case, they have the same position in the interrupt hierarchy. > Is there a better way to do this ? > >> >> - If you do this for debug purposes, then CONFIG_GENERIC_IRQ_DEBUGFS >> is the right way to look up the information. >> >> - If, for reasons that are beyond me, you actually *need* this, then >> implementing irq_print_chip in your irq_chip structure is the way >> to go. >> >> But frankly, I'd rather you drop this altogether. > > I just want to differentiate which interrupt belongs to which PLIC > Instance in /proc/interrupts. I can take a different approach if you > suggest. I *have* given you a way to implement that in a better way. But again, I'd rather you *don't* do it for the reason I have outlined above. Thanks, M.
> -----Original Message----- > From: Marc Zyngier <maz@kernel.org> > Sent: 16 May 2020 18:46 > To: Anup Patel <Anup.Patel@wdc.com> > Cc: Palmer Dabbelt <palmer@dabbelt.com>; Paul Walmsley > <paul.walmsley@sifive.com>; Thomas Gleixner <tglx@linutronix.de>; Jason > Cooper <jason@lakedaemon.net>; Atish Patra <Atish.Patra@wdc.com>; Alistair > Francis <Alistair.Francis@wdc.com>; Anup Patel <anup@brainfault.org>; linux- > riscv@lists.infradead.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH 3/4] irqchip/sifive-plic: Separate irq_chip for muiltiple PLIC > instances > > On 2020-05-16 14:01, Anup Patel wrote: > >> -----Original Message----- > >> From: Marc Zyngier <maz@kernel.org> > >> Sent: 16 May 2020 17:59 > >> To: Anup Patel <Anup.Patel@wdc.com> > >> Cc: Palmer Dabbelt <palmer@dabbelt.com>; Paul Walmsley > >> <paul.walmsley@sifive.com>; Thomas Gleixner <tglx@linutronix.de>; > >> Jason Cooper <jason@lakedaemon.net>; Atish Patra > >> <Atish.Patra@wdc.com>; Alistair Francis <Alistair.Francis@wdc.com>; > >> Anup Patel <anup@brainfault.org>; > >> linux- > >> riscv@lists.infradead.org; linux-kernel@vger.kernel.org > >> Subject: Re: [PATCH 3/4] irqchip/sifive-plic: Separate irq_chip for > >> muiltiple PLIC instances > >> > >> On 2020-05-16 07:39, Anup Patel wrote: > >> > To distinguish interrupts from multiple PLIC instances, we use a > >> > per-PLIC irq_chip instance with a different name. > >> > > >> > Signed-off-by: Anup Patel <anup.patel@wdc.com> > >> > --- > >> > drivers/irqchip/irq-sifive-plic.c | 28 > >> > +++++++++++++++------------- > >> > 1 file changed, 15 insertions(+), 13 deletions(-) > >> > > >> > diff --git a/drivers/irqchip/irq-sifive-plic.c > >> > b/drivers/irqchip/irq-sifive-plic.c > >> > index 2d3db927a551..e42fc082ad18 100644 > >> > --- a/drivers/irqchip/irq-sifive-plic.c > >> > +++ b/drivers/irqchip/irq-sifive-plic.c > >> > @@ -60,6 +60,7 @@ > >> > #define PLIC_ENABLE_THRESHOLD 0 > >> > > >> > struct plic_priv { > >> > + struct irq_chip chip; > >> > struct cpumask lmask; > >> > struct irq_domain *irqdomain; > >> > void __iomem *regs; > >> > @@ -76,6 +77,7 @@ struct plic_handler { > >> > void __iomem *enable_base; > >> > struct plic_priv *priv; > >> > }; > >> > +static unsigned int plic_count; > >> > static bool plic_cpuhp_setup_done; static DEFINE_PER_CPU(struct > >> > plic_handler, plic_handlers); > >> > > >> > @@ -164,20 +166,12 @@ static void plic_irq_eoi(struct irq_data *d) > >> > writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM); } > >> > > >> > -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 > >> > -}; > >> > - > >> > static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq, > >> > irq_hw_number_t hwirq) > >> > { > >> > - irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data, > >> > + struct plic_priv *priv = d->host_data; > >> > + > >> > + irq_domain_set_info(d, irq, hwirq, &priv->chip, d->host_data, > >> > handle_fasteoi_irq, NULL, NULL); > >> > irq_set_noprobe(irq); > >> > return 0; > >> > @@ -294,6 +288,14 @@ static int __init plic_init(struct device_node > >> > *node, > >> > if (!priv) > >> > return -ENOMEM; > >> > > >> > + priv->chip.name = kasprintf(GFP_KERNEL, "PLIC%d", plic_count++); > >> > + priv->chip.irq_mask = plic_irq_mask, > >> > + priv->chip.irq_unmask = plic_irq_unmask, > >> > + priv->chip.irq_eoi = plic_irq_eoi, #ifdef CONFIG_SMP > >> > + priv->chip.irq_set_affinity = plic_set_affinity, #endif > >> > + > >> > priv->regs = of_iomap(node, 0); > >> > if (WARN_ON(!priv->regs)) { > >> > error = -EIO; > >> > @@ -383,9 +385,9 @@ static int __init plic_init(struct device_node > >> > *node, > >> > } > >> > > >> > pr_info("interrupt-controller at 0x%llx " > >> > - "(interrupts=%d, contexts=%d, handlers=%d)\n", > >> > + "(interrupts=%d, contexts=%d, handlers=%d) (%s)\n", > >> > (unsigned long long)iores.start, nr_irqs, > >> > - nr_contexts, nr_handlers); > >> > + nr_contexts, nr_handlers, priv->chip.name); > >> > set_handle_irq(plic_handle_irq); > >> > return 0; > >> > >> I really dislike this patch for multiple reasons: > >> > >> - Allocating a new struc irq_chip just for a string seems over the > >> top, > >> specially as all the *useful* stuff stays the same. > >> > >> - Even if I hate it, /proc is API. I'm sure something, somewhere is > >> parsing this. Changing the string is likely to confuse it. > > > > AFAIK, we don't have scripts in RISC-V world that depend on > > /proc/interrupts content. May be in future such scripts will show up. > > How do you know that? Do you keep an exhaustive repository of all the possible > parsers of /proc/cpuinfo (rhetorical question)? > > > For system with multiple PLICs, we are seeing same "SiFive PLIC" > > string for all PLIC interrupts in "cat /proc/interrupts". I am trying > > to assign different string based on PLIC instance. This is similar to > > what GICv2 driver is doing (e.g. GIC-0, GIC-1, ... in > > /proc/interrupts). > > Which was a *very* bad idea the first place, and I wish I could get rid of it. I > cannot, for the reason outlined above (it's ABI). > Furthermore, in this case, the GICs are different (they are cascaded). > In your case, they have the same position in the interrupt hierarchy. > > > Is there a better way to do this ? > > > >> > >> - If you do this for debug purposes, then CONFIG_GENERIC_IRQ_DEBUGFS > >> is the right way to look up the information. > >> > >> - If, for reasons that are beyond me, you actually *need* this, then > >> implementing irq_print_chip in your irq_chip structure is the way > >> to go. > >> > >> But frankly, I'd rather you drop this altogether. > > > > I just want to differentiate which interrupt belongs to which PLIC > > Instance in /proc/interrupts. I can take a different approach if you > > suggest. > > I *have* given you a way to implement that in a better way. But again, I'd > rather you *don't* do it for the reason I have outlined above. I explored kernel/irq/proc.c and we can achieve what this patch does by implementing irq_print_chip() callback of "struct irq_chip" so we certainly don't need separate "struct irq_chip" for each PLIC instance. I will implement irq_print_chip() callback in v2 series. Regards, Anup
On 2020-05-16 17:38, Anup Patel wrote: >> -----Original Message----- >> From: Marc Zyngier <maz@kernel.org> [...] >> I *have* given you a way to implement that in a better way. But again, >> I'd >> rather you *don't* do it for the reason I have outlined above. > > I explored kernel/irq/proc.c and we can achieve what this patch does > by implementing irq_print_chip() callback of "struct irq_chip" so we > certainly don't need separate "struct irq_chip" for each PLIC instance. > > I will implement irq_print_chip() callback in v2 series. You still haven't explained *why* you need to have this change. As it stands, I'm not prepared to take it. Thanks, M.
> -----Original Message----- > From: Marc Zyngier <maz@kernel.org> > Sent: 18 May 2020 13:45 > To: Anup Patel <Anup.Patel@wdc.com> > Cc: Palmer Dabbelt <palmer@dabbelt.com>; Paul Walmsley > <paul.walmsley@sifive.com>; Thomas Gleixner <tglx@linutronix.de>; Jason > Cooper <jason@lakedaemon.net>; Atish Patra <Atish.Patra@wdc.com>; Alistair > Francis <Alistair.Francis@wdc.com>; Anup Patel <anup@brainfault.org>; linux- > riscv@lists.infradead.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH 3/4] irqchip/sifive-plic: Separate irq_chip for muiltiple PLIC > instances > > On 2020-05-16 17:38, Anup Patel wrote: > >> -----Original Message----- > >> From: Marc Zyngier <maz@kernel.org> > > [...] > > >> I *have* given you a way to implement that in a better way. But > >> again, I'd rather you *don't* do it for the reason I have outlined > >> above. > > > > I explored kernel/irq/proc.c and we can achieve what this patch does > > by implementing irq_print_chip() callback of "struct irq_chip" so we > > certainly don't need separate "struct irq_chip" for each PLIC instance. > > > > I will implement irq_print_chip() callback in v2 series. > > You still haven't explained *why* you need to have this change. > As it stands, I'm not prepared to take it. > This is only for differentiating interrupts of multiple PLIC instance In /proc/interrupts. I will drop this patch since (like you mentioned) contents of /proc/interrupts is considered an ABI and this patch breaks it. For now, we can infer the PLIC instance for interrupt X based on contents of /proc/irq/X/node (i.e. interrupt NUMA node id). Thanks, Anup
diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c index 2d3db927a551..e42fc082ad18 100644 --- a/drivers/irqchip/irq-sifive-plic.c +++ b/drivers/irqchip/irq-sifive-plic.c @@ -60,6 +60,7 @@ #define PLIC_ENABLE_THRESHOLD 0 struct plic_priv { + struct irq_chip chip; struct cpumask lmask; struct irq_domain *irqdomain; void __iomem *regs; @@ -76,6 +77,7 @@ struct plic_handler { void __iomem *enable_base; struct plic_priv *priv; }; +static unsigned int plic_count; static bool plic_cpuhp_setup_done; static DEFINE_PER_CPU(struct plic_handler, plic_handlers); @@ -164,20 +166,12 @@ static void plic_irq_eoi(struct irq_data *d) writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM); } -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 -}; - static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hwirq) { - irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data, + struct plic_priv *priv = d->host_data; + + irq_domain_set_info(d, irq, hwirq, &priv->chip, d->host_data, handle_fasteoi_irq, NULL, NULL); irq_set_noprobe(irq); return 0; @@ -294,6 +288,14 @@ static int __init plic_init(struct device_node *node, if (!priv) return -ENOMEM; + priv->chip.name = kasprintf(GFP_KERNEL, "PLIC%d", plic_count++); + priv->chip.irq_mask = plic_irq_mask, + priv->chip.irq_unmask = plic_irq_unmask, + priv->chip.irq_eoi = plic_irq_eoi, +#ifdef CONFIG_SMP + priv->chip.irq_set_affinity = plic_set_affinity, +#endif + priv->regs = of_iomap(node, 0); if (WARN_ON(!priv->regs)) { error = -EIO; @@ -383,9 +385,9 @@ static int __init plic_init(struct device_node *node, } pr_info("interrupt-controller at 0x%llx " - "(interrupts=%d, contexts=%d, handlers=%d)\n", + "(interrupts=%d, contexts=%d, handlers=%d) (%s)\n", (unsigned long long)iores.start, nr_irqs, - nr_contexts, nr_handlers); + nr_contexts, nr_handlers, priv->chip.name); set_handle_irq(plic_handle_irq); return 0;
To distinguish interrupts from multiple PLIC instances, we use a per-PLIC irq_chip instance with a different name. Signed-off-by: Anup Patel <anup.patel@wdc.com> --- drivers/irqchip/irq-sifive-plic.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-)