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

[Oleksij Rempel <linux@rempel-privat.de>]

[-- Attachment #1: Type: text/plain, Size: 4844 bytes --]

Am 18.09.2015 um 12:42 schrieb Marc Zyngier:
> 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.

ok.

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

ok.

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

Uff... in one of earliest reviews i was asked to add lock..
I also was asked to add asm9260 to some existing driver. Not sure if it
is still making sense.

-- 
Regards,
Oleksij


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]