[PATCH 1/4] irqchip: ingenic: Drop redundant irq_suspend / irq_resume functions

[PATCH 1/4] irqchip: ingenic: Drop redundant irq_suspend / irq_resume functions

[Paul Cercueil]

The same behaviour can be obtained by using the IRQCHIP_MASK_ON_SUSPEND
flag on the IRQ chip.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/irqchip/irq-ingenic.c   | 24 +-----------------------
 include/linux/irqchip/ingenic.h | 14 --------------
 2 files changed, 1 insertion(+), 37 deletions(-)
 delete mode 100644 include/linux/irqchip/ingenic.h

diff --git a/drivers/irqchip/irq-ingenic.c b/drivers/irqchip/irq-ingenic.c
index f126255b3260..06fa810e89bb 100644
--- a/drivers/irqchip/irq-ingenic.c
+++ b/drivers/irqchip/irq-ingenic.c
@@ -10,7 +10,6 @@
 #include <linux/interrupt.h>
 #include <linux/ioport.h>
 #include <linux/irqchip.h>
-#include <linux/irqchip/ingenic.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/timex.h>
@@ -50,26 +49,6 @@ static irqreturn_t intc_cascade(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static void intc_irq_set_mask(struct irq_chip_generic *gc, uint32_t mask)
-{
-	struct irq_chip_regs *regs = &gc->chip_types->regs;
-
-	writel(mask, gc->reg_base + regs->enable);
-	writel(~mask, gc->reg_base + regs->disable);
-}
-
-void ingenic_intc_irq_suspend(struct irq_data *data)
-{
-	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(data);
-	intc_irq_set_mask(gc, gc->wake_active);
-}
-
-void ingenic_intc_irq_resume(struct irq_data *data)
-{
-	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(data);
-	intc_irq_set_mask(gc, gc->mask_cache);
-}
-
 static struct irqaction intc_cascade_action = {
 	.handler = intc_cascade,
 	.name = "SoC intc cascade interrupt",
@@ -127,8 +106,7 @@ static int __init ingenic_intc_of_init(struct device_node *node,
 		ct->chip.irq_mask = irq_gc_mask_disable_reg;
 		ct->chip.irq_mask_ack = irq_gc_mask_disable_reg;
 		ct->chip.irq_set_wake = irq_gc_set_wake;
-		ct->chip.irq_suspend = ingenic_intc_irq_suspend;
-		ct->chip.irq_resume = ingenic_intc_irq_resume;
+		ct->chip.flags = IRQCHIP_MASK_ON_SUSPEND;
 
 		irq_setup_generic_chip(gc, IRQ_MSK(32), 0, 0,
 				       IRQ_NOPROBE | IRQ_LEVEL);
diff --git a/include/linux/irqchip/ingenic.h b/include/linux/irqchip/ingenic.h
deleted file mode 100644
index 146558853ad4..000000000000
--- a/include/linux/irqchip/ingenic.h
+++ /dev/null
@@ -1,14 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-or-later */
-/*
- *  Copyright (C) 2010, Lars-Peter Clausen <lars@metafoo.de>
- */
-
-#ifndef __LINUX_IRQCHIP_INGENIC_H__
-#define __LINUX_IRQCHIP_INGENIC_H__
-
-#include <linux/irq.h>
-
-extern void ingenic_intc_irq_suspend(struct irq_data *data);
-extern void ingenic_intc_irq_resume(struct irq_data *data);
-
-#endif
-- 
2.21.0.593.g511ec345e18

[PATCH 2/4] irqchip: ingenic: Error out if IRQ domain creation failed

[Paul Cercueil]

If we cannot create the IRQ domain, the driver should fail to probe
instead of succeeding with just a warning message.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/irqchip/irq-ingenic.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/irqchip/irq-ingenic.c b/drivers/irqchip/irq-ingenic.c
index 06fa810e89bb..d97a3a500249 100644
--- a/drivers/irqchip/irq-ingenic.c
+++ b/drivers/irqchip/irq-ingenic.c
@@ -87,6 +87,14 @@ static int __init ingenic_intc_of_init(struct device_node *node,
 		goto out_unmap_irq;
 	}
 
+	domain = irq_domain_add_legacy(node, num_chips * 32,
+				       JZ4740_IRQ_BASE, 0,
+				       &irq_domain_simple_ops, NULL);
+	if (!domain) {
+		err = -ENOMEM;
+		goto out_unmap_base;
+	}
+
 	for (i = 0; i < num_chips; i++) {
 		/* Mask all irqs */
 		writel(0xffffffff, intc->base + (i * CHIP_SIZE) +
@@ -112,14 +120,11 @@ static int __init ingenic_intc_of_init(struct device_node *node,
 				       IRQ_NOPROBE | IRQ_LEVEL);
 	}
 
-	domain = irq_domain_add_legacy(node, num_chips * 32, JZ4740_IRQ_BASE, 0,
-				       &irq_domain_simple_ops, NULL);
-	if (!domain)
-		pr_warn("unable to register IRQ domain\n");
-
 	setup_irq(parent_irq, &intc_cascade_action);
 	return 0;
 
+out_unmap_base:
+	iounmap(intc->base);
 out_unmap_irq:
 	irq_dispose_mapping(parent_irq);
 out_free:
-- 
2.21.0.593.g511ec345e18

[PATCH 3/4] irqchip: ingenic: Get virq number from IRQ domain

[Paul Cercueil]

Get the virq number from the IRQ domain instead of calculating it from
the hardcoded irq base.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/irqchip/irq-ingenic.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-ingenic.c b/drivers/irqchip/irq-ingenic.c
index d97a3a500249..82a079fa3a3d 100644
--- a/drivers/irqchip/irq-ingenic.c
+++ b/drivers/irqchip/irq-ingenic.c
@@ -21,6 +21,7 @@
 
 struct ingenic_intc_data {
 	void __iomem *base;
+	struct irq_domain *domain;
 	unsigned num_chips;
 };
 
@@ -34,6 +35,7 @@ struct ingenic_intc_data {
 static irqreturn_t intc_cascade(int irq, void *data)
 {
 	struct ingenic_intc_data *intc = irq_get_handler_data(irq);
+	struct irq_domain *domain = intc->domain;
 	uint32_t irq_reg;
 	unsigned i;
 
@@ -43,7 +45,8 @@ static irqreturn_t intc_cascade(int irq, void *data)
 		if (!irq_reg)
 			continue;
 
-		generic_handle_irq(__fls(irq_reg) + (i * 32) + JZ4740_IRQ_BASE);
+		irq = irq_find_mapping(domain, __fls(irq_reg) + (i * 32));
+		generic_handle_irq(irq);
 	}
 
 	return IRQ_HANDLED;
@@ -95,6 +98,8 @@ static int __init ingenic_intc_of_init(struct device_node *node,
 		goto out_unmap_base;
 	}
 
+	intc->domain = domain;
+
 	for (i = 0; i < num_chips; i++) {
 		/* Mask all irqs */
 		writel(0xffffffff, intc->base + (i * CHIP_SIZE) +
-- 
2.21.0.593.g511ec345e18

[PATCH 4/4] irqchip: ingenic: Alloc generic chips from IRQ domain

[Paul Cercueil]

By creating the generic chips from the IRQ domain, we don't rely on the
JZ4740_IRQ_BASE macro. It also makes the code a bit cleaner.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/irqchip/irq-ingenic.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/irqchip/irq-ingenic.c b/drivers/irqchip/irq-ingenic.c
index 82a079fa3a3d..06ab3ad22ad2 100644
--- a/drivers/irqchip/irq-ingenic.c
+++ b/drivers/irqchip/irq-ingenic.c
@@ -36,12 +36,14 @@ static irqreturn_t intc_cascade(int irq, void *data)
 {
 	struct ingenic_intc_data *intc = irq_get_handler_data(irq);
 	struct irq_domain *domain = intc->domain;
+	struct irq_chip_generic *gc;
 	uint32_t irq_reg;
 	unsigned i;
 
 	for (i = 0; i < intc->num_chips; i++) {
-		irq_reg = readl(intc->base + (i * CHIP_SIZE) +
-				JZ_REG_INTC_PENDING);
+		gc = irq_get_domain_generic_chip(domain, i * 32);
+
+		irq_reg = irq_reg_readl(gc, JZ_REG_INTC_PENDING);
 		if (!irq_reg)
 			continue;
 
@@ -92,7 +94,7 @@ static int __init ingenic_intc_of_init(struct device_node *node,
 
 	domain = irq_domain_add_legacy(node, num_chips * 32,
 				       JZ4740_IRQ_BASE, 0,
-				       &irq_domain_simple_ops, NULL);
+				       &irq_generic_chip_ops, NULL);
 	if (!domain) {
 		err = -ENOMEM;
 		goto out_unmap_base;
@@ -100,17 +102,17 @@ static int __init ingenic_intc_of_init(struct device_node *node,
 
 	intc->domain = domain;
 
-	for (i = 0; i < num_chips; i++) {
-		/* Mask all irqs */
-		writel(0xffffffff, intc->base + (i * CHIP_SIZE) +
-		       JZ_REG_INTC_SET_MASK);
+	err = irq_alloc_domain_generic_chips(domain, 32, 1, "INTC",
+					     handle_level_irq, 0,
+					     IRQ_NOPROBE | IRQ_LEVEL, 0);
+	if (err)
+		goto out_domain_remove;
 
-		gc = irq_alloc_generic_chip("INTC", 1,
-					    JZ4740_IRQ_BASE + (i * 32),
-					    intc->base + (i * CHIP_SIZE),
-					    handle_level_irq);
+	for (i = 0; i < num_chips; i++) {
+		gc = irq_get_domain_generic_chip(domain, i * 32);
 
 		gc->wake_enabled = IRQ_MSK(32);
+		gc->reg_base = intc->base + (i * CHIP_SIZE);
 
 		ct = gc->chip_types;
 		ct->regs.enable = JZ_REG_INTC_CLEAR_MASK;
@@ -121,13 +123,15 @@ static int __init ingenic_intc_of_init(struct device_node *node,
 		ct->chip.irq_set_wake = irq_gc_set_wake;
 		ct->chip.flags = IRQCHIP_MASK_ON_SUSPEND;
 
-		irq_setup_generic_chip(gc, IRQ_MSK(32), 0, 0,
-				       IRQ_NOPROBE | IRQ_LEVEL);
+		/* Mask all irqs */
+		irq_reg_writel(gc, IRQ_MSK(32), JZ_REG_INTC_SET_MASK);
 	}
 
 	setup_irq(parent_irq, &intc_cascade_action);
 	return 0;
 
+out_domain_remove:
+	irq_domain_remove(domain);
 out_unmap_base:
 	iounmap(intc->base);
 out_unmap_irq:
-- 
2.21.0.593.g511ec345e18

Re: [PATCH 3/4] irqchip: ingenic: Get virq number from IRQ domain

[Marc Zyngier]

[+ Zhou Yanjie]

Paul,

On 27/07/2019 20:17, Paul Cercueil wrote:
> Get the virq number from the IRQ domain instead of calculating it from
> the hardcoded irq base.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/irqchip/irq-ingenic.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-ingenic.c b/drivers/irqchip/irq-ingenic.c
> index d97a3a500249..82a079fa3a3d 100644
> --- a/drivers/irqchip/irq-ingenic.c
> +++ b/drivers/irqchip/irq-ingenic.c
> @@ -21,6 +21,7 @@
>  
>  struct ingenic_intc_data {
>  	void __iomem *base;
> +	struct irq_domain *domain;
>  	unsigned num_chips;
>  };
>  
> @@ -34,6 +35,7 @@ struct ingenic_intc_data {
>  static irqreturn_t intc_cascade(int irq, void *data)
>  {
>  	struct ingenic_intc_data *intc = irq_get_handler_data(irq);
> +	struct irq_domain *domain = intc->domain;
>  	uint32_t irq_reg;
>  	unsigned i;
>  
> @@ -43,7 +45,8 @@ static irqreturn_t intc_cascade(int irq, void *data)
>  		if (!irq_reg)
>  			continue;
>  
> -		generic_handle_irq(__fls(irq_reg) + (i * 32) + JZ4740_IRQ_BASE);
> +		irq = irq_find_mapping(domain, __fls(irq_reg) + (i * 32));
> +		generic_handle_irq(irq);
>  	}
>  
>  	return IRQ_HANDLED;
> @@ -95,6 +98,8 @@ static int __init ingenic_intc_of_init(struct device_node *node,
>  		goto out_unmap_base;
>  	}
>  
> +	intc->domain = domain;
> +
>  	for (i = 0; i < num_chips; i++) {
>  		/* Mask all irqs */
>  		writel(0xffffffff, intc->base + (i * CHIP_SIZE) +
> 

This is likely to conflict with this[1] series, which turns the
intc_cascade function into a chained handler (which it should have been
from the start). Can you please work with Zhou to post a unified series?

Having two people working independently on the same file is likely to
end badly otherwise.

Thanks,

	M.

[1]
https://lore.kernel.org/lkml/1564335273-22931-1-git-send-email-zhouyanjie@zoho.com/
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 3/4] irqchip: ingenic: Get virq number from IRQ domain

[Paul Cercueil]

Hi Marc,


Le lun. 29 juil. 2019 à 6:38, Marc Zyngier <marc.zyngier@arm.com> a 
écrit :
> [+ Zhou Yanjie]
> 
> Paul,
> 
> On 27/07/2019 20:17, Paul Cercueil wrote:
>>  Get the virq number from the IRQ domain instead of calculating it 
>> from
>>  the hardcoded irq base.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>>   drivers/irqchip/irq-ingenic.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>> 
>>  diff --git a/drivers/irqchip/irq-ingenic.c 
>> b/drivers/irqchip/irq-ingenic.c
>>  index d97a3a500249..82a079fa3a3d 100644
>>  --- a/drivers/irqchip/irq-ingenic.c
>>  +++ b/drivers/irqchip/irq-ingenic.c
>>  @@ -21,6 +21,7 @@
>> 
>>   struct ingenic_intc_data {
>>   	void __iomem *base;
>>  +	struct irq_domain *domain;
>>   	unsigned num_chips;
>>   };
>> 
>>  @@ -34,6 +35,7 @@ struct ingenic_intc_data {
>>   static irqreturn_t intc_cascade(int irq, void *data)
>>   {
>>   	struct ingenic_intc_data *intc = irq_get_handler_data(irq);
>>  +	struct irq_domain *domain = intc->domain;
>>   	uint32_t irq_reg;
>>   	unsigned i;
>> 
>>  @@ -43,7 +45,8 @@ static irqreturn_t intc_cascade(int irq, void 
>> *data)
>>   		if (!irq_reg)
>>   			continue;
>> 
>>  -		generic_handle_irq(__fls(irq_reg) + (i * 32) + JZ4740_IRQ_BASE);
>>  +		irq = irq_find_mapping(domain, __fls(irq_reg) + (i * 32));
>>  +		generic_handle_irq(irq);
>>   	}
>> 
>>   	return IRQ_HANDLED;
>>  @@ -95,6 +98,8 @@ static int __init ingenic_intc_of_init(struct 
>> device_node *node,
>>   		goto out_unmap_base;
>>   	}
>> 
>>  +	intc->domain = domain;
>>  +
>>   	for (i = 0; i < num_chips; i++) {
>>   		/* Mask all irqs */
>>   		writel(0xffffffff, intc->base + (i * CHIP_SIZE) +
>> 
> 
> This is likely to conflict with this[1] series, which turns the
> intc_cascade function into a chained handler (which it should have 
> been
> from the start). Can you please work with Zhou to post a unified 
> series?
> 
> Having two people working independently on the same file is likely to
> end badly otherwise.

I'm registered as maintainer for Ingenic SoCs (including ingenic-irq.c)
and Zhou didn't Cc me on his patchset... And if he did I'd have a few
comments on his patches that would have to be addressed in a V5.

If you think my patchset is fine, then maybe merge it then Zhou can just
rebase on top?

Cheers,
-Paul

> Thanks,
> 
> 	M.
> 
> [1]
> https://lore.kernel.org/lkml/1564335273-22931-1-git-send-email-zhouyanjie@zoho.com/
> --
> Jazz is not dead. It just smells funny...

Re: [PATCH 3/4] irqchip: ingenic: Get virq number from IRQ domain

[Zhou Yanjie]

Hi Paul,

These patches was originally sent on January 26th. It was still the old 
maintainer
information. When sending v4, I may have some problems with cc setting 
so that
no cc is given to you. I am really sorry. The main purpose on this patch 
is to change
the cascade irq to chained irq, I think chained irq is more generic way. 
Look forward
to your and Marc's comments.

On 2019年07月30日 00:57, Paul Cercueil wrote:
> Hi Marc,
>
>
> Le lun. 29 juil. 2019 à 6:38, Marc Zyngier <marc.zyngier@arm.com> a 
> écrit :
>> [+ Zhou Yanjie]
>>
>> Paul,
>>
>> On 27/07/2019 20:17, Paul Cercueil wrote:
>>>  Get the virq number from the IRQ domain instead of calculating it from
>>>  the hardcoded irq base.
>>>
>>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>>  ---
>>>   drivers/irqchip/irq-ingenic.c | 7 ++++++-
>>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>>  diff --git a/drivers/irqchip/irq-ingenic.c 
>>> b/drivers/irqchip/irq-ingenic.c
>>>  index d97a3a500249..82a079fa3a3d 100644
>>>  --- a/drivers/irqchip/irq-ingenic.c
>>>  +++ b/drivers/irqchip/irq-ingenic.c
>>>  @@ -21,6 +21,7 @@
>>>
>>>   struct ingenic_intc_data {
>>>       void __iomem *base;
>>>  +    struct irq_domain *domain;
>>>       unsigned num_chips;
>>>   };
>>>
>>>  @@ -34,6 +35,7 @@ struct ingenic_intc_data {
>>>   static irqreturn_t intc_cascade(int irq, void *data)
>>>   {
>>>       struct ingenic_intc_data *intc = irq_get_handler_data(irq);
>>>  +    struct irq_domain *domain = intc->domain;
>>>       uint32_t irq_reg;
>>>       unsigned i;
>>>
>>>  @@ -43,7 +45,8 @@ static irqreturn_t intc_cascade(int irq, void *data)
>>>           if (!irq_reg)
>>>               continue;
>>>
>>>  -        generic_handle_irq(__fls(irq_reg) + (i * 32) + 
>>> JZ4740_IRQ_BASE);
>>>  +        irq = irq_find_mapping(domain, __fls(irq_reg) + (i * 32));
>>>  +        generic_handle_irq(irq);
>>>       }
>>>
>>>       return IRQ_HANDLED;
>>>  @@ -95,6 +98,8 @@ static int __init ingenic_intc_of_init(struct 
>>> device_node *node,
>>>           goto out_unmap_base;
>>>       }
>>>
>>>  +    intc->domain = domain;
>>>  +
>>>       for (i = 0; i < num_chips; i++) {
>>>           /* Mask all irqs */
>>>           writel(0xffffffff, intc->base + (i * CHIP_SIZE) +
>>>
>>
>> This is likely to conflict with this[1] series, which turns the
>> intc_cascade function into a chained handler (which it should have been
>> from the start). Can you please work with Zhou to post a unified series?
>>
>> Having two people working independently on the same file is likely to
>> end badly otherwise.
>
> I'm registered as maintainer for Ingenic SoCs (including ingenic-irq.c)
> and Zhou didn't Cc me on his patchset... And if he did I'd have a few
> comments on his patches that would have to be addressed in a V5.
>
> If you think my patchset is fine, then maybe merge it then Zhou can just
> rebase on top?
>
> Cheers,
> -Paul
>
>> Thanks,
>>
>>     M.
>>
>> [1]
>> https://lore.kernel.org/lkml/1564335273-22931-1-git-send-email-zhouyanjie@zoho.com/ 
>>
>> -- 
>> Jazz is not dead. It just smells funny...
>
>