Messages in this thread | | | Subject | Re: [PATCH v2] irqchip: add TS7800v1 fpga based controller driver | From | firas ashkar <> | Date | Fri, 21 Oct 2022 12:19:02 -0400 |
| |
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@savoirfairelinux.com> 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@savoirfairelinux.com> > > --- > > > > 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. >
| |