Re: Possible Spam [PATCH v2 2/2] ARM: irqchip: mxs: add Alpascale ASM9260 support

[Marc Zyngier <marc.zyngier@arm.com>]

On Fri, 18 Sep 2015 11:18:42 +0200
Oleksij Rempel <linux@rempel-privat.de> wrote:

> From: Oleksij Rempel <external.Oleksij.Rempel@de.bosch.com>
> 
> Freescale iMX23/iMX28 and Alphascale ASM9260 have similar

Is it Alphascale or Alpascale? You may need to fix the patch title.

> interrupt collectors. It makes easy to reuse irq-mxs code for ASM9260.
> Differences between this devices are fallowing:
> - different register offsets
> - different count of intterupt lines per register
> - ASM9260 don't provide reset bit
> - ASM9260 don't support FIQ.
> 
> Signed-off-by: Oleksij Rempel <external.Oleksij.Rempel@de.bosch.com>
> ---
>  drivers/irqchip/Kconfig                    |   5 ++
>  drivers/irqchip/Makefile                   |   2 +-
>  drivers/irqchip/alphascale_asm9260-icoll.h | 109 +++++++++++++++++++++++++++++
>  drivers/irqchip/irq-mxs.c                  | 106 +++++++++++++++++++++++++++-
>  4 files changed, 220 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/irqchip/alphascale_asm9260-icoll.h
> 

[...]

> diff --git a/drivers/irqchip/irq-mxs.c b/drivers/irqchip/irq-mxs.c
> index 14374de..1470087 100644
> --- a/drivers/irqchip/irq-mxs.c
> +++ b/drivers/irqchip/irq-mxs.c
> @@ -1,5 +1,7 @@
>  /*
>   * Copyright (C) 2009-2010 Freescale Semiconductor, Inc. All Rights Reserved.
> + * Copyright (C) 2014 Oleksij Rempel <linux@rempel-privat.de>
> + *	Add Alphascale ASM9260 support.
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License as published by
> @@ -28,6 +30,8 @@
>  #include <linux/stmp_device.h>
>  #include <asm/exception.h>
>  
> +#include "alphascale_asm9260-icoll.h"
> +
>  /*
>   * this device provide 4 offsets for each register:
>   * 0x0 - plain read write mode
> @@ -49,6 +53,11 @@
>  
>  #define ICOLL_NUM_IRQS		128
>  
> +enum icoll_type {
> +	ICOLL,
> +	ASM9260_ICOLL,
> +};
> +
>  struct icoll_priv {
>  	void __iomem *vector;
>  	void __iomem *levelack;
> @@ -58,10 +67,38 @@ struct icoll_priv {
>  	/* number of interrupts per register */
>  	int ;
>  	void __iomem *clear;
> +	enum icoll_type type;
>  };
>  
>  static struct icoll_priv icoll_priv;
>  static struct irq_domain *icoll_domain;
> +static DEFINE_RAW_SPINLOCK(icoll_lock);
> +
> +/* calculate bit offset depending on number of intterupt per register */
> +static u32 icoll_intr_bitshift(struct irq_data *d, u32 bit)
> +{
> +	/*
> +	 * We expect intr_per_reg to be 4 or 1, it means
> +	 * "n" will be 3 or 0.
> +	 */
> +	int n = icoll_priv.intr_per_reg - 1;
> +
> +	/*
> +	 * If n = 0, "bit" is never shifted.
> +	 * If n = 3, mask lower part of hwirq to convert it
> +	 * in 0, 1, 2 or 3 and then multiply it by 8 (or shift by 3)
> +	 */
> +	return bit << ((d->hwirq & n) << n);
> +}
> +
> +/* calculate mem offset depending on number of intterupt per register */
> +static void __iomem *icoll_intr_reg(struct irq_data *d)
> +{
> +	int n = icoll_priv.intr_per_reg >> 1;
> +
> +	/* offset = hwirq / intr_per_reg * 0x10 */
> +	return icoll_priv.intr + ((d->hwirq >> n) * 0x10);
> +}

Please correct me if I'm wrong, but it looks like these function are
only useful when used on ams9260. So why do we need intr_per_reg at
all? MXS doesn't need it (always 1), and ams9260 always need it (always
4). Save yourself some previous cycles and simplify the whole thing.

>  
>  static void icoll_ack_irq(struct irq_data *d)
>  {
> @@ -86,12 +123,38 @@ static void icoll_unmask_irq(struct irq_data *d)
>  			icoll_priv.intr + SET_REG + HW_ICOLL_INTERRUPTn(d->hwirq));
>  }
>  
> +static void asm9260_mask_irq(struct irq_data *d)
> +{
> +	raw_spin_lock(&icoll_lock);
> +	__raw_writel(icoll_intr_bitshift(d, BM_ICOLL_INTR_ENABLE),
> +			icoll_intr_reg(d) + CLR_REG);
> +	raw_spin_unlock(&icoll_lock);
> +}
> +
> +static void asm9260_unmask_irq(struct irq_data *d)
> +{
> +	raw_spin_lock(&icoll_lock);
> +	__raw_writel(ASM9260_BM_CLEAR_BIT(d->hwirq),
> +		     icoll_priv.clear +
> +		     ASM9260_HW_ICOLL_CLEARn(d->hwirq));
> +
> +	__raw_writel(icoll_intr_bitshift(d, BM_ICOLL_INTR_ENABLE),
> +			icoll_intr_reg(d) + SET_REG);
> +	raw_spin_unlock(&icoll_lock);
> +}

Can you please explain the rational for this lock? mask/unmask use
different registers, and it is not obvious to me what race you are
trying to avoid here.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.