[PATCH 0/3] irqchip: sun4i: various cleanups

[PATCH 0/3] irqchip: sun4i: various cleanups

[Hans de Goede]

Hi All,

So here is a second patchset for irq-sun4i.c I'm not 100% sure about these
changes. They all seem the right thing to do, but they definitely need a
good review first.

I've run various tests with this set and everything works as advertised.

Regards,

Hans

[PATCH 1/3] irqchip: sun4i: Don't mask + unmask for the non oneshot case

[Hans de Goede]

Since sun4i and sun5i are single core SOCs there is no need to mask non
oneshot IRQs, to achieve this we use handle_fasteoi_irq with a dummy eoi.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/irqchip/irq-sun4i.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-sun4i.c b/drivers/irqchip/irq-sun4i.c
index a0ed1ea..0a71990 100644
--- a/drivers/irqchip/irq-sun4i.c
+++ b/drivers/irqchip/irq-sun4i.c
@@ -74,8 +74,17 @@ static void sun4i_irq_unmask(struct irq_data *irqd)
 	       sun4i_irq_base + SUN4I_IRQ_ENABLE_REG(reg));
 }
 
+/*
+ * Since sun4i and sun5i are single core SOCs there is no need to mask non
+ * oneshot IRQs, to achieve this we use handle_fasteoi_irq with a dummy eoi.
+ */
+static void sun4i_irq_dummy_eoi(struct irq_data *irqd)
+{
+}
+
 static struct irq_chip sun4i_irq_chip = {
 	.name		= "sun4i_irq",
+	.irq_eoi	= sun4i_irq_dummy_eoi,
 	.irq_mask	= sun4i_irq_mask,
 	.irq_unmask	= sun4i_irq_unmask,
 };
@@ -97,7 +106,7 @@ static int sun4i_irq_map(struct irq_domain *d, unsigned int virq,
 					 handle_fasteoi_irq);
 	else
 		irq_set_chip_and_handler(virq, &sun4i_irq_chip,
-					 handle_level_irq);
+					 handle_fasteoi_irq);
 
 	set_irq_flags(virq, IRQF_VALID | IRQF_PROBE);
 
-- 
1.9.0

Re: [PATCH 1/3] irqchip: sun4i: Don't mask + unmask for the non oneshot case

[Thomas Gleixner]

On Thu, 13 Mar 2014, Hans de Goede wrote:

> Since sun4i and sun5i are single core SOCs there is no need to mask non
> oneshot IRQs, to achieve this we use handle_fasteoi_irq with a dummy eoi.

This is slightly wrong :)

Even on a SMP system there is no need to mask the interrupt when the
controller works like that sunxi one. If the controller is not broken
beyond repair then it does not deliver the same interrupt to a
different cpu. Such a thing would always deliver it to all cores and
those would race to grab the spinlock and mask it. I've seen such
horror, but don't ask how that performs.

The reason why you can spare the mask/unmask dance is that the
controller does not require any action and clears the interrupt when
the level goes back to inactive. That happens when the device handler
acks it at the device level.

Now there might be the case when the device reactivates the interrupt
before the RETI. But that does not matter as we run the primary
interrupt handlers with interrupts disabled.

Thanks,

	tglx
 
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/irqchip/irq-sun4i.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-sun4i.c b/drivers/irqchip/irq-sun4i.c
> index a0ed1ea..0a71990 100644
> --- a/drivers/irqchip/irq-sun4i.c
> +++ b/drivers/irqchip/irq-sun4i.c
> @@ -74,8 +74,17 @@ static void sun4i_irq_unmask(struct irq_data *irqd)
>  	       sun4i_irq_base + SUN4I_IRQ_ENABLE_REG(reg));
>  }
>  
> +/*
> + * Since sun4i and sun5i are single core SOCs there is no need to mask non
> + * oneshot IRQs, to achieve this we use handle_fasteoi_irq with a dummy eoi.
> + */
> +static void sun4i_irq_dummy_eoi(struct irq_data *irqd)
> +{
> +}
> +
>  static struct irq_chip sun4i_irq_chip = {
>  	.name		= "sun4i_irq",
> +	.irq_eoi	= sun4i_irq_dummy_eoi,
>  	.irq_mask	= sun4i_irq_mask,
>  	.irq_unmask	= sun4i_irq_unmask,
>  };
> @@ -97,7 +106,7 @@ static int sun4i_irq_map(struct irq_domain *d, unsigned int virq,
>  					 handle_fasteoi_irq);
>  	else
>  		irq_set_chip_and_handler(virq, &sun4i_irq_chip,
> -					 handle_level_irq);
> +					 handle_fasteoi_irq);
>  
>  	set_irq_flags(virq, IRQF_VALID | IRQF_PROBE);
>  
> -- 
> 1.9.0
> 
> 

Re: [PATCH 1/3] irqchip: sun4i: Don't mask + unmask for the non oneshot case

[Hans de Goede]

Hi,

On 03/14/2014 12:35 PM, Thomas Gleixner wrote:
> On Thu, 13 Mar 2014, Hans de Goede wrote:
> 
>> Since sun4i and sun5i are single core SOCs there is no need to mask non
>> oneshot IRQs, to achieve this we use handle_fasteoi_irq with a dummy eoi.
> 
> This is slightly wrong :)
> 
> Even on a SMP system there is no need to mask the interrupt when the
> controller works like that sunxi one. If the controller is not broken
> beyond repair then it does not deliver the same interrupt to a
> different cpu. Such a thing would always deliver it to all cores and
> those would race to grab the spinlock and mask it. I've seen such
> horror, but don't ask how that performs.
> 
> The reason why you can spare the mask/unmask dance is that the
> controller does not require any action and clears the interrupt when
> the level goes back to inactive. That happens when the device handler
> acks it at the device level.
> 
> Now there might be the case when the device reactivates the interrupt
> before the RETI. But that does not matter as we run the primary
> interrupt handlers with interrupts disabled.

Ok, I'm going to wait a bit to see if Maxime has anything to say
to this second series, and then I'll do a v2 with the commit msg
fixed.

Regards,

Hans


> 
> Thanks,
> 
> 	tglx
>  
>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/irqchip/irq-sun4i.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-sun4i.c b/drivers/irqchip/irq-sun4i.c
>> index a0ed1ea..0a71990 100644
>> --- a/drivers/irqchip/irq-sun4i.c
>> +++ b/drivers/irqchip/irq-sun4i.c
>> @@ -74,8 +74,17 @@ static void sun4i_irq_unmask(struct irq_data *irqd)
>>  	       sun4i_irq_base + SUN4I_IRQ_ENABLE_REG(reg));
>>  }
>>  
>> +/*
>> + * Since sun4i and sun5i are single core SOCs there is no need to mask non
>> + * oneshot IRQs, to achieve this we use handle_fasteoi_irq with a dummy eoi.
>> + */
>> +static void sun4i_irq_dummy_eoi(struct irq_data *irqd)
>> +{
>> +}
>> +
>>  static struct irq_chip sun4i_irq_chip = {
>>  	.name		= "sun4i_irq",
>> +	.irq_eoi	= sun4i_irq_dummy_eoi,
>>  	.irq_mask	= sun4i_irq_mask,
>>  	.irq_unmask	= sun4i_irq_unmask,
>>  };
>> @@ -97,7 +106,7 @@ static int sun4i_irq_map(struct irq_domain *d, unsigned int virq,
>>  					 handle_fasteoi_irq);
>>  	else
>>  		irq_set_chip_and_handler(virq, &sun4i_irq_chip,
>> -					 handle_level_irq);
>> +					 handle_fasteoi_irq);
>>  
>>  	set_irq_flags(virq, IRQF_VALID | IRQF_PROBE);
>>  
>> -- 
>> 1.9.0
>>
>>

[PATCH 2/3] irqchip: sun4i: Simplify irq mapping

[Hans de Goede]

Now that we're using handle_fasteio_irq for all interrupts, we can get rid
of having 2 irq_chip structs by making sun4i_irq_ack a nop for all irqs
except irq 0.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/irqchip/irq-sun4i.c | 27 ++++-----------------------
 1 file changed, 4 insertions(+), 23 deletions(-)

diff --git a/drivers/irqchip/irq-sun4i.c b/drivers/irqchip/irq-sun4i.c
index 0a71990..6a8c88d 100644
--- a/drivers/irqchip/irq-sun4i.c
+++ b/drivers/irqchip/irq-sun4i.c
@@ -45,6 +45,9 @@ static void sun4i_irq_ack(struct irq_data *irqd)
 	int reg = irq / 32;
 	u32 val;
 
+	if (irq != 0)
+		return; /* Only IRQ 0 / the ENMI needs to be acked */
+
 	val = readl(sun4i_irq_base + SUN4I_IRQ_PENDING_REG(reg));
 	writel(val | (1 << irq_off),
 	       sun4i_irq_base + SUN4I_IRQ_PENDING_REG(reg));
@@ -74,24 +77,8 @@ static void sun4i_irq_unmask(struct irq_data *irqd)
 	       sun4i_irq_base + SUN4I_IRQ_ENABLE_REG(reg));
 }
 
-/*
- * Since sun4i and sun5i are single core SOCs there is no need to mask non
- * oneshot IRQs, to achieve this we use handle_fasteoi_irq with a dummy eoi.
- */
-static void sun4i_irq_dummy_eoi(struct irq_data *irqd)
-{
-}
-
 static struct irq_chip sun4i_irq_chip = {
 	.name		= "sun4i_irq",
-	.irq_eoi	= sun4i_irq_dummy_eoi,
-	.irq_mask	= sun4i_irq_mask,
-	.irq_unmask	= sun4i_irq_unmask,
-};
-
-/* IRQ 0 / the ENMI needs a late eoi call */
-static struct irq_chip sun4i_irq_chip_enmi = {
-	.name		= "sun4i_irq",
 	.irq_eoi	= sun4i_irq_ack,
 	.irq_mask	= sun4i_irq_mask,
 	.irq_unmask	= sun4i_irq_unmask,
@@ -101,13 +88,7 @@ static struct irq_chip sun4i_irq_chip_enmi = {
 static int sun4i_irq_map(struct irq_domain *d, unsigned int virq,
 			 irq_hw_number_t hw)
 {
-	if (hw == 0)
-		irq_set_chip_and_handler(virq, &sun4i_irq_chip_enmi,
-					 handle_fasteoi_irq);
-	else
-		irq_set_chip_and_handler(virq, &sun4i_irq_chip,
-					 handle_fasteoi_irq);
-
+	irq_set_chip_and_handler(virq, &sun4i_irq_chip, handle_fasteoi_irq);
 	set_irq_flags(virq, IRQF_VALID | IRQF_PROBE);
 
 	return 0;
-- 
1.9.0

Re: [PATCH 2/3] irqchip: sun4i: Simplify irq mapping

[Maxime Ripard]

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

On Thu, Mar 13, 2014 at 08:50:01PM +0100, Hans de Goede wrote:
> Now that we're using handle_fasteio_irq for all interrupts, we can get rid
> of having 2 irq_chip structs by making sun4i_irq_ack a nop for all irqs
> except irq 0.
> 
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/irqchip/irq-sun4i.c | 27 ++++-----------------------
>  1 file changed, 4 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-sun4i.c b/drivers/irqchip/irq-sun4i.c
> index 0a71990..6a8c88d 100644
> --- a/drivers/irqchip/irq-sun4i.c
> +++ b/drivers/irqchip/irq-sun4i.c
> @@ -45,6 +45,9 @@ static void sun4i_irq_ack(struct irq_data *irqd)
>  	int reg = irq / 32;
>  	u32 val;
>  
> +	if (irq != 0)
> +		return; /* Only IRQ 0 / the ENMI needs to be acked */
> +
>  	val = readl(sun4i_irq_base + SUN4I_IRQ_PENDING_REG(reg));
>  	writel(val | (1 << irq_off),
>  	       sun4i_irq_base + SUN4I_IRQ_PENDING_REG(reg));
> @@ -74,24 +77,8 @@ static void sun4i_irq_unmask(struct irq_data *irqd)
>  	       sun4i_irq_base + SUN4I_IRQ_ENABLE_REG(reg));
>  }
>  
> -/*
> - * Since sun4i and sun5i are single core SOCs there is no need to mask non
> - * oneshot IRQs, to achieve this we use handle_fasteoi_irq with a dummy eoi.
> - */
> -static void sun4i_irq_dummy_eoi(struct irq_data *irqd)
> -{
> -}
> -
>  static struct irq_chip sun4i_irq_chip = {
>  	.name		= "sun4i_irq",
> -	.irq_eoi	= sun4i_irq_dummy_eoi,


Is it just me or did you just remove the code you added in patch 1?

Patches 1 and 2 can be easily squashed.

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 3/3] irqchip: sun4i: simplify sun4i_irq_ack

[Hans de Goede]

Now that we only ack irq 0 the code can be simplified a lot.

Also switch from read / modify / write to a simple write clear:
1) This is what the android code does (it has a hack for acking irq 0
  in its unmask code doing this)
2) read / modify / write simply does not make sense for an irq status
  register like this, if the other bits are writeable (and the data sheet says
  they are not) they should be write 1 to clear, since otherwise a read /
  modify / write can race with a device raising an interrupt and then clear
  the pending bit unintentionally

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/irqchip/irq-sun4i.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/irqchip/irq-sun4i.c b/drivers/irqchip/irq-sun4i.c
index 6a8c88d..75615b5 100644
--- a/drivers/irqchip/irq-sun4i.c
+++ b/drivers/irqchip/irq-sun4i.c
@@ -41,16 +41,11 @@ static asmlinkage void __exception_irq_entry sun4i_handle_irq(struct pt_regs *re
 static void sun4i_irq_ack(struct irq_data *irqd)
 {
 	unsigned int irq = irqd_to_hwirq(irqd);
-	unsigned int irq_off = irq % 32;
-	int reg = irq / 32;
-	u32 val;
 
 	if (irq != 0)
 		return; /* Only IRQ 0 / the ENMI needs to be acked */
 
-	val = readl(sun4i_irq_base + SUN4I_IRQ_PENDING_REG(reg));
-	writel(val | (1 << irq_off),
-	       sun4i_irq_base + SUN4I_IRQ_PENDING_REG(reg));
+	writel(BIT(0), sun4i_irq_base + SUN4I_IRQ_PENDING_REG(0));
 }
 
 static void sun4i_irq_mask(struct irq_data *irqd)
-- 
1.9.0