From: Anup Patel <Anup.Patel@wdc.com>
To: Marc Zyngier <maz@kernel.org>
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-riscv@lists.infradead.org>,
"linux-kernel@vger.kernel.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 16:38:16 +0000 [thread overview]
Message-ID: <MN2PR04MB62078C2854FE33CE81186FA88DBA0@MN2PR04MB6207.namprd04.prod.outlook.com> (raw)
In-Reply-To: <6fbfca8f084f6cd0dc6818c4bbf58843@kernel.org>
> -----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
next prev parent reply other threads:[~2020-05-16 16:38 UTC|newest]
Thread overview: 20+ 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 ` [PATCH 1/4] irqchip/sifive-plic: Setup cpuhp once after current handler is present Anup Patel
2020-05-16 12:11 ` Marc Zyngier
2020-05-16 12:52 ` Anup Patel
2020-05-16 13:30 ` Marc Zyngier
2020-05-16 16:28 ` 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 12:20 ` Marc Zyngier
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 12:29 ` Marc Zyngier
2020-05-16 13:01 ` Anup Patel
2020-05-16 13:16 ` Marc Zyngier
2020-05-16 16:38 ` Anup Patel [this message]
2020-05-18 8:14 ` Marc Zyngier
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 12:30 ` Marc Zyngier
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=MN2PR04MB62078C2854FE33CE81186FA88DBA0@MN2PR04MB6207.namprd04.prod.outlook.com \
--to=anup.patel@wdc.com \
--cc=Alistair.Francis@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=maz@kernel.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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).