From: Marc Zyngier <maz@kernel.org> 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 Date: Sat, 16 May 2020 14:16:22 +0100 [thread overview] Message-ID: <6fbfca8f084f6cd0dc6818c4bbf58843@kernel.org> (raw) In-Reply-To: <DM6PR04MB6201FD64C78C51A94C4776558DBA0@DM6PR04MB6201.namprd04.prod.outlook.com> 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. -- Jazz is not dead. It just smells funny...
WARNING: multiple messages have this Message-ID (diff)
From: Marc Zyngier <maz@kernel.org> To: Anup Patel <Anup.Patel@wdc.com> Cc: Jason Cooper <jason@lakedaemon.net>, Anup Patel <anup@brainfault.org>, linux-kernel@vger.kernel.org, Atish Patra <Atish.Patra@wdc.com>, Palmer Dabbelt <palmer@dabbelt.com>, Paul Walmsley <paul.walmsley@sifive.com>, Alistair Francis <Alistair.Francis@wdc.com>, Thomas Gleixner <tglx@linutronix.de>, linux-riscv@lists.infradead.org Subject: Re: [PATCH 3/4] irqchip/sifive-plic: Separate irq_chip for muiltiple PLIC instances Date: Sat, 16 May 2020 14:16:22 +0100 [thread overview] Message-ID: <6fbfca8f084f6cd0dc6818c4bbf58843@kernel.org> (raw) In-Reply-To: <DM6PR04MB6201FD64C78C51A94C4776558DBA0@DM6PR04MB6201.namprd04.prod.outlook.com> 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. -- Jazz is not dead. It just smells funny...
next prev parent reply other threads:[~2020-05-16 13:16 UTC|newest] Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-05-16 6:38 [PATCH 0/4] More improvements for multiple PLICs Anup Patel 2020-05-16 6:38 ` Anup Patel 2020-05-16 6:38 ` [PATCH 1/4] irqchip/sifive-plic: Setup cpuhp once after current handler is present Anup Patel 2020-05-16 6:38 ` Anup Patel 2020-05-16 12:11 ` Marc Zyngier 2020-05-16 12:11 ` Marc Zyngier 2020-05-16 12:52 ` Anup Patel 2020-05-16 12:52 ` Anup Patel 2020-05-16 13:30 ` Marc Zyngier 2020-05-16 13:30 ` Marc Zyngier 2020-05-16 16:28 ` Anup Patel 2020-05-16 16:28 ` Anup Patel 2020-05-17 8:02 ` Anup Patel 2020-05-17 8:02 ` Anup Patel 2020-05-16 6:38 ` [PATCH 2/4] irqchip/sifive-plic: Improve boot prints for multiple PLIC instances Anup Patel 2020-05-16 6:38 ` Anup Patel 2020-05-16 12:20 ` Marc Zyngier 2020-05-16 12:20 ` Marc Zyngier 2020-05-16 12:53 ` Anup Patel 2020-05-16 12:53 ` Anup Patel 2020-05-16 6:39 ` [PATCH 3/4] irqchip/sifive-plic: Separate irq_chip for muiltiple " Anup Patel 2020-05-16 6:39 ` Anup Patel 2020-05-16 12:29 ` Marc Zyngier 2020-05-16 12:29 ` Marc Zyngier 2020-05-16 13:01 ` Anup Patel 2020-05-16 13:01 ` Anup Patel 2020-05-16 13:16 ` Marc Zyngier [this message] 2020-05-16 13:16 ` Marc Zyngier 2020-05-16 16:38 ` Anup Patel 2020-05-16 16:38 ` Anup Patel 2020-05-18 8:14 ` Marc Zyngier 2020-05-18 8:14 ` Marc Zyngier 2020-05-18 9:00 ` Anup Patel 2020-05-18 9:00 ` Anup Patel 2020-05-16 6:39 ` [PATCH 4/4] irqchip/sifive-plic: Set default irq affinity in plic_irqdomain_map() Anup Patel 2020-05-16 6:39 ` Anup Patel 2020-05-16 12:30 ` Marc Zyngier 2020-05-16 12:30 ` Marc Zyngier 2020-05-16 12:53 ` Anup Patel 2020-05-16 12:53 ` Anup Patel
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=6fbfca8f084f6cd0dc6818c4bbf58843@kernel.org \ --to=maz@kernel.org \ --cc=Alistair.Francis@wdc.com \ --cc=Anup.Patel@wdc.com \ --cc=Atish.Patra@wdc.com \ --cc=anup@brainfault.org \ --cc=jason@lakedaemon.net \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-riscv@lists.infradead.org \ --cc=palmer@dabbelt.com \ --cc=paul.walmsley@sifive.com \ --cc=tglx@linutronix.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.