Re: [PATCH v2] irqchip: add TS7800v1 fpga based controller driver
From: firas ashkar
Date: Fri Oct 21 2022 - 12:19:13 EST
On Fri, 2022-10-21 at 10:22 +0100, Marc Zyngier wrote:
> On Thu, 20 Oct 2022 15:13:51 +0100,
> Firas Ashkar <firas.ashkar@xxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > 1. add TS-7800v1 fpga based irq controller driver, and
> > 2. add related memory and irq resources
> >
> > By default only mapped FPGA interrupts will be chained/multiplexed,
> > these
> > chained interrupts will then be available to other device drivers
> > to
> > request via the corresponding Linux IRQs.
> >
> > $ cat /proc/cpuinfo
> > processor : 0
> > model name : Feroceon rev 0 (v5l)
> > BogoMIPS : 333.33
> > Features : swp half thumb fastmult edsp
> > CPU implementer : 0x41
> > CPU architecture: 5TEJ
> > CPU variant : 0x0
> > CPU part : 0x926
> > CPU revision : 0
> >
> > Hardware : Technologic Systems TS-78xx SBC
> > Revision : 0000
> > Serial : 0000000000000000
> > $
> >
> > $ cat /proc/interrupts
> > CPU0
> > 1: 902 orion_irq Level orion_tick
> > 4: 795 orion_irq Level ttyS0
> > 13: 0 orion_irq Level ehci_hcd:usb2
> > 18: 0 orion_irq Level ehci_hcd:usb1
> > 22: 69 orion_irq Level eth0
> > 23: 171 orion_irq Level orion-mdio
> > 29: 0 orion_irq Level mv_crypto
> > 31: 2 orion_irq Level mv_xor.0
> > 65: 1 ts7800-irqc 0 Edge ts-dmac-cpupci
> > Err: 0
> > $
> >
> > $ uname -a
> > Linux ts-7800 6.1.0-rc1 #2 PREEMPT Mon Oct 17 11:19:12 EDT 2022
> > armv5tel
> > GNU/Linux
> > $
> >
> > Signed-off-by: Firas Ashkar <firas.ashkar@xxxxxxxxxxxxxxxxxxxx>
> > ---
> >
> > Notes:
> > Changes in V2
> > * limit the commit message
>
> Well, there is still a lot of work to do. Most of this commit message
> could be reduced to a single paragraph:
>
> "Add support for FooBar IRQ controller usually found on Zorglub
> platform."
>
> The rest is plain obvious.
nack, commit message is fine as is!
>
> > * format comments in source code
> > * use raw spin locks to protect mask/unmask ops
> > * use 'handle_edge_irq' and 'irq_ack' logic
> > * remove 'irq_domain_xlate_onecell'
> > * remove unnecessary status flags
> > * use 'builtin_platform_driver' helper routine
> >
> > :100644 100644 2f4fe3ca5c1a ed8378893208 M arch/arm/mach-
> > orion5x/ts78xx-fpga.h
> > :100644 100644 af810e7ccd79 d319a68b7160 M arch/arm/mach-
> > orion5x/ts78xx-setup.c
> > :100644 100644 7ef9f5e696d3 d184fb435c5d
> > M drivers/irqchip/Kconfig
> > :100644 100644 87b49a10962c b022eece2042
> > M drivers/irqchip/Makefile
> > :000000 100644 000000000000 e975607fa4d5
> > A drivers/irqchip/irq-ts7800.c
> > arch/arm/mach-orion5x/ts78xx-fpga.h | 1 +
> > arch/arm/mach-orion5x/ts78xx-setup.c | 55 +++++
> > drivers/irqchip/Kconfig | 12 +
> > drivers/irqchip/Makefile | 1 +
> > drivers/irqchip/irq-ts7800.c | 347
> > +++++++++++++++++++++++++++
> > 5 files changed, 416 insertions(+)
> >
> > diff --git a/arch/arm/mach-orion5x/ts78xx-fpga.h b/arch/arm/mach-
> > orion5x/ts78xx-fpga.h
> > index 2f4fe3ca5c1a..ed8378893208 100644
> > --- a/arch/arm/mach-orion5x/ts78xx-fpga.h
> > +++ b/arch/arm/mach-orion5x/ts78xx-fpga.h
> > @@ -32,6 +32,7 @@ struct fpga_devices {
> > struct fpga_device ts_rtc;
> > struct fpga_device ts_nand;
> > struct fpga_device ts_rng;
> > + struct fpga_device ts_irqc;
> > };
> >
> > struct ts78xx_fpga_data {
> > diff --git a/arch/arm/mach-orion5x/ts78xx-setup.c b/arch/arm/mach-
> > orion5x/ts78xx-setup.c
> > index af810e7ccd79..d319a68b7160 100644
> > --- a/arch/arm/mach-orion5x/ts78xx-setup.c
> > +++ b/arch/arm/mach-orion5x/ts78xx-setup.c
> > @@ -322,6 +322,49 @@ static void ts78xx_ts_rng_unload(void)
> > platform_device_del(&ts78xx_ts_rng_device);
> > }
> >
> > +/*****************************************************************
> > ************
> > + * fpga irq controller
> > +
> > *******************************************************************
> > *********/
>
> [...]
>
> Sorry, but I have to ask: what part of "we're not taking any
> additional non-DT changes to these obsolete setups" did I fail to
> accurately communicate?
>
> Until this board is entirely converted to DT, I'm not taking any
> irqchip changes other than the most obvious bug fixes.
as long as this board is present in the kernel in its current legacy
form, this is a valid patch!
>
> [...]
>
> > +static void ts7800_irq_mask(struct irq_data *d)
> > +{
> > + struct ts7800_irq_data *data =
> > irq_data_get_irq_chip_data(d);
> > + u32 fpga_mask_reg = readl(data->base + IRQ_MASK_REG);
> > + u32 mask_bits = 1 << d->hwirq;
> > + unsigned long flags;
> > +
> > + raw_spin_lock_irqsave(&data->lock, flags);
> > + writel(fpga_mask_reg & ~mask_bits, data->base +
> > IRQ_MASK_REG);
> > + writel(fpga_mask_reg & ~mask_bits, data->bridge +
> > IRQ_MASK_REG);
> > + raw_spin_unlock_irqrestore(&data->lock, flags);
>
> OMFG. What do you expect this protects? Same question applies to all
> the instances where you take this pointless lock.
don't jump now
the locks added as per your previous comment, quoting:
"I know your HW is UP, but seeing these RMW sequences without a lock
makes me jump."
On this single CPU based arch TS78xx, locks are waste of cpu cycles,
also note that IRQs/preemption are/is already off in this context
maybe you meant adding locks as to promote general correct coding ?
or maybe it was like this previous nonsense comment, quoting :
"We don't remove interrupt controllers. What happens if someone still
had a mapping?"
>
> [...]
>
> > +static void ts7800_ic_chained_handle_irq(struct irq_desc *desc)
> > +{
> > + struct ts7800_irq_data *data =
> > irq_desc_get_handler_data(desc);
> > + struct irq_chip *chip = irq_desc_get_chip(desc);
> > + u32 mask_bits = readl(data->base + IRQ_MASK_REG);
> > + u32 status = readl(data->bridge);
> > +
> > + chained_irq_enter(chip, desc);
> > +
> > + if (unlikely(status == 0)) {
> > + handle_bad_irq(desc);
> > + goto out;
> > + }
> > +
> > + do {
> > + unsigned int bit = __ffs(status);
> > + int irq = irq_find_mapping(data->domain, bit);
> > +
> > + if (irq && (mask_bits & BIT(bit)))
> > + generic_handle_irq(irq);
>
> Again, this is not appropriate. I've pointed you to the right API in
> my previous review of this patch.
'generic_handle_domain_irq' causing some issues
>
> [...]
>
> > +static struct platform_driver ts7800_ic_driver = {
> > + .probe = ts7800_ic_probe,
> > + .remove = ts7800_ic_remove,
> > + .id_table = ts7800v1_ic_ids,
> > + .driver = {
> > + .name = DRIVER_NAME,
> > + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> > + },
> > +};
> > +builtin_platform_driver(ts7800_ic_driver);
>
> Again, this isn't appropriate either, and I pointed it out last
> time. We have specific helpers for irqchip, and using them isn't
> optional. But of course, you'll need to move to DT for that.
>
> Anyway, this is the last time I review this patch until you convert
> the corresponding platform to DT.
no problems, though have to note this is not constructive!
>
> M.
>