> -----Original Message-----
> From: Marc Zyngier <m...@kernel.org>
> Sent: 16 May 2020 17:59
> To: Anup Patel <anup.pa...@wdc.com>
> Cc: Palmer Dabbelt <pal...@dabbelt.com>; Paul Walmsley
> <paul.walms...@sifive.com>; Thomas Gleixner <t...@linutronix.de>; Jason
> Cooper <ja...@lakedaemon.net>; Atish Patra <atish.pa...@wdc.com>; Alistair
> Francis <alistair.fran...@wdc.com>; Anup Patel <a...@brainfault.org>; linux-
> ri...@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.pa...@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
