Re: [PATCH v2] irqchip: add TS7800v1 fpga based controller driver

From: firas ashkar
Date: Fri Oct 21 2022 - 14:15:02 EST


On Fri, 2022-10-21 at 18:18 +0100, Marc Zyngier wrote:
> On Fri, 21 Oct 2022 17:19:02 +0100,
> firas ashkar <firas.ashkar@xxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > 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!
>
> Allow me to be the judge of that.
>
> > >
> > > >     * 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!
>
> No. There is a cut of point. Code that we would taken 10 years ago
> isn't necessarily valid anymore. We want to improve the kernel as a
> whole, and not keep it in the past.
>
> >
> > >
> > > [...]
> > >
> > > > +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 ?
>
> Let me spell it out for you: RMW means Read-Modify-Write. Putting a
> lock around a *write only* serves zero purpose. It is just overhead,
> and it is incorrect.
>
> >
> > or maybe it was like this previous nonsense comment, quoting :
> > "We don't remove interrupt controllers. What happens if someone
> > still
> > had a mapping?"
>
> And I stand by it.
>
> >
> >
> > >
> > > [...]
> > >
> > > > +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
>
> Which issue?
# insmod /tmp/virt-dma.ko
# insmod /tmp/ts-dmac.ko
generic_handle_domain_irq failed, error -22
irq 2, desc: a285c39f, depth: 0, count: 0, unhandled: 0
->handle_irq(): e7046ee9, ts7800_ic_chained_handle_irq+0x0/0x1ec
[irq_ts7800]
->irq_data.chip(): 70d81143, 0xc14db44c
->action(): f799c8dd
->action->handler(): dcf07981, bad_chained_irq+0x0/0x64
IRQ_LEVEL set
IRQ_NOPROBE set
IRQ_NOREQUEST set
IRQ_NOTHREAD set
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
irq 2, desc: a285c39f, depth: 0, count: 0, unhandled: 0
->handle_irq(): e7046ee9, ts7800_ic_chained_handle_irq+0x0/0x1ec
[irq_ts7800]
->irq_data.chip(): 70d81143, 0xc14db44c
->action(): f799c8dd
->action->handler(): dcf07981, bad_chained_irq+0x0/0x64
IRQ_LEVEL set
IRQ_NOPROBE set
IRQ_NOREQUEST set
IRQ_NOTHREAD set
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
irq 2, desc: a285c39f, depth: 0, count: 0, unhandled: 0
->handle_irq(): e7046ee9, ts7800_ic_chained_handle_irq+0x0/0x1ec
[irq_ts7800]
->irq_data.chip(): 70d81143, 0xc14db44c
->action(): f799c8dd
->action->handler(): dcf07981, bad_chained_irq+0x0/0x64
IRQ_LEVEL set
IRQ_NOPROBE set
IRQ_NOREQUEST set
IRQ_NOTHREAD set
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
irq 2, desc: a285c39f, depth: 0, count: 0, unhandled: 0
->handle_irq(): e7046ee9, ts7800_ic_chained_handle_irq+0x0/0x1ec
[irq_ts7800]
->irq_data.chip(): 70d81143, 0xc14db44c
->action(): f799c8dd
->action->handler(): dcf07981, bad_chained_irq+0x0/0x64
IRQ_LEVEL set
IRQ_NOPROBE set
IRQ_NOREQUEST set
IRQ_NOTHREAD set
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
irq 2, desc: a285c39f, depth: 0, count: 0, unhandled: 0
->handle_irq(): e7046ee9, ts7800_ic_chained_handle_irq+0x0/0x1ec
[irq_ts7800]
->irq_data.chip(): 70d81143, 0xc14db44c
->action(): f799c8dd
->action->handler(): dcf07981, bad_chained_irq+0x0/0x64
IRQ_LEVEL set
IRQ_NOPROBE set
IRQ_NOREQUEST set
IRQ_NOTHREAD set
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
unexpected IRQ trap at vector 02
...
>
> > >
> > > [...]
> > >
> > > > +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!
>
> I've pointed out a bunch of issues, and provided advise on how you
> can
> fix them. That's constructive. A non constructive approach would be
> to
> just ignore your patch. If that's what you prefer, please say so.
>
> Thanks,
>
>         M.
>