Re: Possible Spam [PATCH v2 2/2] ARM: irqchip: mxs: add Alpascale ASM9260 support
Am 18.09.2015 um 12:42 schrieb Marc Zyngier:
> On Fri, 18 Sep 2015 11:18:42 +0200
> Oleksij Rempel wrote:
>
>> From: Oleksij Rempel
>>
>> 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
>> ---
>> 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
>> + * 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
>> #include
>>
>> +#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
signature.asc
Description: OpenPGP digital signature
Re: Possible Spam [PATCH v2 2/2] ARM: irqchip: mxs: add Alpascale ASM9260 support
On Fri, 18 Sep 2015 11:18:42 +0200
Oleksij Rempel wrote:
> From: Oleksij Rempel
>
> 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
> ---
> 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
> + * 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
> #include
>
> +#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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
