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

[Marc Zyngier]

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.

>     * 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.

[...]

> +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.

[...]

> +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.

[...]

> +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.

	M.

-- 
Without deviation from the norm, progress is not possible.