[PATCH] irq-dw-apb-ictl: convert to platform device

[PATCH] irq-dw-apb-ictl: convert to platform device

[Alexey Brodkin]

This allows utilization of probe deferral if master interrupt controller
was not yet probed.

In my case there's an DW APB GPIO configured as interrupt controller
which is a master for DW APB INTC.

Interrupt controller sub-node of DW APB GPIO gets parsed only on DW APB
GPIO real probe - before that slave interrupt controllers cannot resolve
its master connection which lead to failures on probes of devices that
use DW APB INTC as its interrupt controller.

Now when this driver is converted to a normal platform device it may use
probe deferral and so all dependences between devices and their
interrupt controllers are resolved automatically on boot.

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Vineet Gupta <vgupta@synopsys.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/irqchip/irq-dw-apb-ictl.c | 112 +++++++++++++++++++++++---------------
 1 file changed, 67 insertions(+), 45 deletions(-)

diff --git a/drivers/irqchip/irq-dw-apb-ictl.c b/drivers/irqchip/irq-dw-apb-ictl.c
index 53bb732..0b05c58 100644
--- a/drivers/irqchip/irq-dw-apb-ictl.c
+++ b/drivers/irqchip/irq-dw-apb-ictl.c
@@ -11,13 +11,15 @@
  * warranty of any kind, whether express or implied.
  */
 
-#include <linux/io.h>
-#include <linux/irq.h>
 #include <linux/irqchip/chained_irq.h>
-#include <linux/of_address.h>
 #include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/module.h>
 
-#include "irqchip.h"
+struct dw_apb_intl_priv {
+	struct device		*dev;
+	struct irq_domain	*irq_domain;
+};
 
 #define APB_INT_ENABLE_L	0x00
 #define APB_INT_ENABLE_H	0x04
@@ -65,41 +67,38 @@ static void dw_apb_ictl_resume(struct irq_data *d)
 #define dw_apb_ictl_resume	NULL
 #endif /* CONFIG_PM */
 
-static int __init dw_apb_ictl_init(struct device_node *np,
-				   struct device_node *parent)
+static int dw_apb_ictl_probe(struct platform_device *pdev)
 {
 	unsigned int clr = IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN;
-	struct resource r;
-	struct irq_domain *domain;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
 	struct irq_chip_generic *gc;
+	struct dw_apb_intl_priv *p;
+	struct resource *res;
 	void __iomem *iobase;
 	int ret, nrirqs, irq;
 	u32 reg;
 
-	/* Map the parent interrupt for the chained handler */
-	irq = irq_of_parse_and_map(np, 0);
-	if (irq <= 0) {
-		pr_err("%s: unable to parse irq\n", np->full_name);
-		return -EINVAL;
-	}
+	if (!np)
+		return -ENODEV;
 
-	ret = of_address_to_resource(np, 0, &r);
-	if (ret) {
-		pr_err("%s: unable to get resource\n", np->full_name);
-		return ret;
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		if (irq != -EPROBE_DEFER)
+			dev_err(dev, "no irq resource %d\n", irq);
+		return irq;
 	}
 
-	if (!request_mem_region(r.start, resource_size(&r), np->full_name)) {
-		pr_err("%s: unable to request mem region\n", np->full_name);
+	p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL);
+	if (!p)
 		return -ENOMEM;
-	}
 
-	iobase = ioremap(r.start, resource_size(&r));
-	if (!iobase) {
-		pr_err("%s: unable to map resource\n", np->full_name);
-		ret = -ENOMEM;
-		goto err_release;
-	}
+	p->dev = &pdev->dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	iobase = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(iobase))
+		return PTR_ERR(iobase);
 
 	/*
 	 * DW IP can be configured to allow 2-64 irqs. We can determine
@@ -120,25 +119,27 @@ static int __init dw_apb_ictl_init(struct device_node *np,
 	else
 		nrirqs = fls(readl_relaxed(iobase + APB_INT_ENABLE_L));
 
-	domain = irq_domain_add_linear(np, nrirqs,
-				       &irq_generic_chip_ops, NULL);
-	if (!domain) {
+	p->irq_domain = irq_domain_add_linear(np, nrirqs,
+					      &irq_generic_chip_ops, NULL);
+	if (!p->irq_domain) {
 		pr_err("%s: unable to add irq domain\n", np->full_name);
-		ret = -ENOMEM;
-		goto err_unmap;
+		return -ENOMEM;
 	}
 
-	ret = irq_alloc_domain_generic_chips(domain, 32, (nrirqs > 32) ? 2 : 1,
-					     np->name, handle_level_irq, clr, 0,
+	ret = irq_alloc_domain_generic_chips(p->irq_domain, 32,
+					     (nrirqs > 32) ? 2 : 1, np->name,
+					     handle_level_irq, clr, 0,
 					     IRQ_GC_MASK_CACHE_PER_TYPE |
 					     IRQ_GC_INIT_MASK_CACHE);
 	if (ret) {
-		pr_err("%s: unable to alloc irq domain gc\n", np->full_name);
-		goto err_unmap;
+		dev_info(p->dev, "irq_alloc_domain_generic_chips failed\n");
+		irq_domain_remove(p->irq_domain);
+		p->irq_domain = NULL;
+		return -ENOMEM;
 	}
 
-	gc = irq_get_domain_generic_chip(domain, 0);
-	gc->private = domain;
+	gc = irq_get_domain_generic_chip(p->irq_domain, 0);
+	gc->private = p->irq_domain;
 	gc->reg_base = iobase;
 
 	gc->chip_types[0].regs.mask = APB_INT_MASK_L;
@@ -159,12 +160,33 @@ static int __init dw_apb_ictl_init(struct device_node *np,
 	irq_set_chained_handler(irq, dw_apb_ictl_handler);
 
 	return 0;
+}
 
-err_unmap:
-	iounmap(iobase);
-err_release:
-	release_mem_region(r.start, resource_size(&r));
-	return ret;
+static int dw_apb_ictl_remove(struct platform_device *pdev)
+{
+	struct dw_apb_intl_priv *p = platform_get_drvdata(pdev);
+
+	irq_domain_remove(p->irq_domain);
+	return 0;
 }
-IRQCHIP_DECLARE(dw_apb_ictl,
-		"snps,dw-apb-ictl", dw_apb_ictl_init);
+
+static const struct of_device_id dw_apb_ictl_dt_ids[] = {
+	{ .compatible = "snps,dw-apb-ictl", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, dw_apb_ictl_dt_ids);
+
+static struct platform_driver dw_apb_ictl_device_driver = {
+	.probe		= dw_apb_ictl_probe,
+	.remove		= dw_apb_ictl_remove,
+	.driver		= {
+		.name	= "synopsys_dw_apb_irq",
+		.of_match_table	= of_match_ptr(dw_apb_ictl_dt_ids),
+	}
+};
+
+module_platform_driver(dw_apb_ictl_device_driver);
+
+MODULE_AUTHOR("Sebastian Hesselbarth");
+MODULE_DESCRIPTION("Synopsys DW APB ICTL irqchip driver");
+MODULE_LICENSE("GPL v2");
-- 
2.1.0

Re: [PATCH] irq-dw-apb-ictl: convert to platform device

[Jisheng Zhang]

Hi Alexsy,


+ Sebastian,

On Tue, 3 Mar 2015 06:56:59 -0800
Alexey Brodkin <Alexey.Brodkin@synopsys.com> wrote:

> This allows utilization of probe deferral if master interrupt controller
> was not yet probed.
> 
> In my case there's an DW APB GPIO configured as interrupt controller
> which is a master for DW APB INTC.
> 
> Interrupt controller sub-node of DW APB GPIO gets parsed only on DW APB
> GPIO real probe - before that slave interrupt controllers cannot resolve
> its master connection which lead to failures on probes of devices that
> use DW APB INTC as its interrupt controller.
> 
> Now when this driver is converted to a normal platform device it may use
> probe deferral and so all dependences between devices and their
> interrupt controllers are resolved automatically on boot.
> 
> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Vineet Gupta <vgupta@synopsys.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Jisheng Zhang <jszhang@marvell.com>
> ---
>  drivers/irqchip/irq-dw-apb-ictl.c | 112
> +++++++++++++++++++++++--------------- 1 file changed, 67 insertions(+), 45
> deletions(-)
> 
> diff --git a/drivers/irqchip/irq-dw-apb-ictl.c
> b/drivers/irqchip/irq-dw-apb-ictl.c index 53bb732..0b05c58 100644
> --- a/drivers/irqchip/irq-dw-apb-ictl.c
> +++ b/drivers/irqchip/irq-dw-apb-ictl.c
> @@ -11,13 +11,15 @@
>   * warranty of any kind, whether express or implied.
>   */
>  
> -#include <linux/io.h>

this headfile need to be kept for readl_relaxed/writel_relaxed

> -#include <linux/irq.h>
>  #include <linux/irqchip/chained_irq.h>
> -#include <linux/of_address.h>
>  #include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/module.h>

sort alphabetically

>  
> -#include "irqchip.h"
> +struct dw_apb_intl_priv {
> +	struct device		*dev;
> +	struct irq_domain	*irq_domain;
> +};
>  
>  #define APB_INT_ENABLE_L	0x00
>  #define APB_INT_ENABLE_H	0x04
> @@ -65,41 +67,38 @@ static void dw_apb_ictl_resume(struct irq_data *d)
>  #define dw_apb_ictl_resume	NULL
>  #endif /* CONFIG_PM */
>  
> -static int __init dw_apb_ictl_init(struct device_node *np,
> -				   struct device_node *parent)
> +static int dw_apb_ictl_probe(struct platform_device *pdev)
>  {
>  	unsigned int clr = IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN;
> -	struct resource r;
> -	struct irq_domain *domain;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
>  	struct irq_chip_generic *gc;
> +	struct dw_apb_intl_priv *p;
> +	struct resource *res;
>  	void __iomem *iobase;
>  	int ret, nrirqs, irq;
>  	u32 reg;
>  
> -	/* Map the parent interrupt for the chained handler */
> -	irq = irq_of_parse_and_map(np, 0);
> -	if (irq <= 0) {
> -		pr_err("%s: unable to parse irq\n", np->full_name);
> -		return -EINVAL;
> -	}
> +	if (!np)
> +		return -ENODEV;
>  
> -	ret = of_address_to_resource(np, 0, &r);
> -	if (ret) {
> -		pr_err("%s: unable to get resource\n", np->full_name);
> -		return ret;
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		if (irq != -EPROBE_DEFER)
> +			dev_err(dev, "no irq resource %d\n", irq);
> +		return irq;
>  	}
>  
> -	if (!request_mem_region(r.start, resource_size(&r),
> np->full_name)) {
> -		pr_err("%s: unable to request mem region\n",
> np->full_name);
> +	p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL);
> +	if (!p)
>  		return -ENOMEM;
> -	}
>  
> -	iobase = ioremap(r.start, resource_size(&r));
> -	if (!iobase) {
> -		pr_err("%s: unable to map resource\n", np->full_name);
> -		ret = -ENOMEM;
> -		goto err_release;
> -	}
> +	p->dev = &pdev->dev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	iobase = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(iobase))
> +		return PTR_ERR(iobase);
>  
>  	/*
>  	 * DW IP can be configured to allow 2-64 irqs. We can determine
> @@ -120,25 +119,27 @@ static int __init dw_apb_ictl_init(struct device_node
> *np, else
>  		nrirqs = fls(readl_relaxed(iobase + APB_INT_ENABLE_L));
>  
> -	domain = irq_domain_add_linear(np, nrirqs,
> -				       &irq_generic_chip_ops, NULL);
> -	if (!domain) {
> +	p->irq_domain = irq_domain_add_linear(np, nrirqs,
> +					      &irq_generic_chip_ops, NULL);
> +	if (!p->irq_domain) {
>  		pr_err("%s: unable to add irq domain\n", np->full_name);
> -		ret = -ENOMEM;
> -		goto err_unmap;
> +		return -ENOMEM;
>  	}
>  
> -	ret = irq_alloc_domain_generic_chips(domain, 32, (nrirqs > 32) ?
> 2 : 1,
> -					     np->name, handle_level_irq,
> clr, 0,
> +	ret = irq_alloc_domain_generic_chips(p->irq_domain, 32,
> +					     (nrirqs > 32) ? 2 : 1,
> np->name,
> +					     handle_level_irq, clr, 0,
>  					     IRQ_GC_MASK_CACHE_PER_TYPE |
>  					     IRQ_GC_INIT_MASK_CACHE);
>  	if (ret) {
> -		pr_err("%s: unable to alloc irq domain gc\n",
> np->full_name);
> -		goto err_unmap;
> +		dev_info(p->dev, "irq_alloc_domain_generic_chips
> failed\n");
> +		irq_domain_remove(p->irq_domain);
> +		p->irq_domain = NULL;
> +		return -ENOMEM;
>  	}
>  
> -	gc = irq_get_domain_generic_chip(domain, 0);
> -	gc->private = domain;
> +	gc = irq_get_domain_generic_chip(p->irq_domain, 0);
> +	gc->private = p->irq_domain;
>  	gc->reg_base = iobase;
>  
>  	gc->chip_types[0].regs.mask = APB_INT_MASK_L;
> @@ -159,12 +160,33 @@ static int __init dw_apb_ictl_init(struct device_node
> *np, irq_set_chained_handler(irq, dw_apb_ictl_handler);
>  
>  	return 0;
> +}
>  
> -err_unmap:
> -	iounmap(iobase);
> -err_release:
> -	release_mem_region(r.start, resource_size(&r));
> -	return ret;
> +static int dw_apb_ictl_remove(struct platform_device *pdev)
> +{
> +	struct dw_apb_intl_priv *p = platform_get_drvdata(pdev);
> +
> +	irq_domain_remove(p->irq_domain);
> +	return 0;
>  }
> -IRQCHIP_DECLARE(dw_apb_ictl,
> -		"snps,dw-apb-ictl", dw_apb_ictl_init);

So, the ictl won't be initialized in init_IRQ(), it will cause problems.

> +
> +static const struct of_device_id dw_apb_ictl_dt_ids[] = {
> +	{ .compatible = "snps,dw-apb-ictl", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, dw_apb_ictl_dt_ids);
> +
> +static struct platform_driver dw_apb_ictl_device_driver = {
> +	.probe		= dw_apb_ictl_probe,
> +	.remove		= dw_apb_ictl_remove,
> +	.driver		= {
> +		.name	= "synopsys_dw_apb_irq",
> +		.of_match_table	= of_match_ptr(dw_apb_ictl_dt_ids),
> +	}
> +};
> +
> +module_platform_driver(dw_apb_ictl_device_driver);

This is too late. If the ictl is needed during early boot, for example
on Marvell BG2Q DMP board, we still need to calibrate the delay loop, the
kernel will hang at "Calibrating delay loop..."

Thanks,
Jisheng

Re: [PATCH] irq-dw-apb-ictl: convert to platform device

[Sebastian Hesselbarth]

On 04.03.2015 04:11, Jisheng Zhang wrote:
> On Tue, 3 Mar 2015 06:56:59 -0800
> Alexey Brodkin <Alexey.Brodkin@synopsys.com> wrote:
>> This allows utilization of probe deferral if master interrupt controller
>> was not yet probed.
>>
>> In my case there's an DW APB GPIO configured as interrupt controller
>> which is a master for DW APB INTC.
>>
>> Interrupt controller sub-node of DW APB GPIO gets parsed only on DW APB
>> GPIO real probe - before that slave interrupt controllers cannot resolve
>> its master connection which lead to failures on probes of devices that
>> use DW APB INTC as its interrupt controller.

Alexey,

thanks for the input, but the point is not to break existing users which
your patch clearly does.

Just to get this straight for me, you are using DW_APB_INTC as a _slave_
interrupt controller of DW_APB_GPIO _master_ interrupt controller? Is
this a fictional setup to test your IP or is it a real world example?

IMHO, if the setup is real, i.e. an APB_INTC hooked up to a GPIO
upstream-wise, you should think of a new driver for an externally
connected but APB bus accessible (???) interrupt controller. If it
is rather some extension of the GPIO controller you should amend
dw_apb_gpio.c instead.

Sorry, but I cannot really wrap my head around that specific setup...

Sebastian

>> Now when this driver is converted to a normal platform device it may use
>> probe deferral and so all dependences between devices and their
>> interrupt controllers are resolved automatically on boot.
>>
>> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
>> Cc: Vineet Gupta <vgupta@synopsys.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Jason Cooper <jason@lakedaemon.net>
>> Cc: Jisheng Zhang <jszhang@marvell.com>
>> ---
>>   drivers/irqchip/irq-dw-apb-ictl.c | 112
>> +++++++++++++++++++++++--------------- 1 file changed, 67 insertions(+), 45
>> deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-dw-apb-ictl.c
>> b/drivers/irqchip/irq-dw-apb-ictl.c index 53bb732..0b05c58 100644
>> --- a/drivers/irqchip/irq-dw-apb-ictl.c
>> +++ b/drivers/irqchip/irq-dw-apb-ictl.c
>> @@ -11,13 +11,15 @@
>>    * warranty of any kind, whether express or implied.
>>    */
>>
>> -#include <linux/io.h>
>
> this headfile need to be kept for readl_relaxed/writel_relaxed
>
>> -#include <linux/irq.h>
>>   #include <linux/irqchip/chained_irq.h>
>> -#include <linux/of_address.h>
>>   #include <linux/of_irq.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/module.h>
>
> sort alphabetically
>
>>
>> -#include "irqchip.h"
>> +struct dw_apb_intl_priv {
>> +	struct device		*dev;
>> +	struct irq_domain	*irq_domain;
>> +};
>>
>>   #define APB_INT_ENABLE_L	0x00
>>   #define APB_INT_ENABLE_H	0x04
>> @@ -65,41 +67,38 @@ static void dw_apb_ictl_resume(struct irq_data *d)
>>   #define dw_apb_ictl_resume	NULL
>>   #endif /* CONFIG_PM */
>>
>> -static int __init dw_apb_ictl_init(struct device_node *np,
>> -				   struct device_node *parent)
>> +static int dw_apb_ictl_probe(struct platform_device *pdev)
>>   {
>>   	unsigned int clr = IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN;
>> -	struct resource r;
>> -	struct irq_domain *domain;
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *np = dev->of_node;
>>   	struct irq_chip_generic *gc;
>> +	struct dw_apb_intl_priv *p;
>> +	struct resource *res;
>>   	void __iomem *iobase;
>>   	int ret, nrirqs, irq;
>>   	u32 reg;
>>
>> -	/* Map the parent interrupt for the chained handler */
>> -	irq = irq_of_parse_and_map(np, 0);
>> -	if (irq <= 0) {
>> -		pr_err("%s: unable to parse irq\n", np->full_name);
>> -		return -EINVAL;
>> -	}
>> +	if (!np)
>> +		return -ENODEV;
>>
>> -	ret = of_address_to_resource(np, 0, &r);
>> -	if (ret) {
>> -		pr_err("%s: unable to get resource\n", np->full_name);
>> -		return ret;
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (irq < 0) {
>> +		if (irq != -EPROBE_DEFER)
>> +			dev_err(dev, "no irq resource %d\n", irq);
>> +		return irq;
>>   	}
>>
>> -	if (!request_mem_region(r.start, resource_size(&r),
>> np->full_name)) {
>> -		pr_err("%s: unable to request mem region\n",
>> np->full_name);
>> +	p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL);
>> +	if (!p)
>>   		return -ENOMEM;
>> -	}
>>
>> -	iobase = ioremap(r.start, resource_size(&r));
>> -	if (!iobase) {
>> -		pr_err("%s: unable to map resource\n", np->full_name);
>> -		ret = -ENOMEM;
>> -		goto err_release;
>> -	}
>> +	p->dev = &pdev->dev;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	iobase = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(iobase))
>> +		return PTR_ERR(iobase);
>>
>>   	/*
>>   	 * DW IP can be configured to allow 2-64 irqs. We can determine
>> @@ -120,25 +119,27 @@ static int __init dw_apb_ictl_init(struct device_node
>> *np, else
>>   		nrirqs = fls(readl_relaxed(iobase + APB_INT_ENABLE_L));
>>
>> -	domain = irq_domain_add_linear(np, nrirqs,
>> -				       &irq_generic_chip_ops, NULL);
>> -	if (!domain) {
>> +	p->irq_domain = irq_domain_add_linear(np, nrirqs,
>> +					      &irq_generic_chip_ops, NULL);
>> +	if (!p->irq_domain) {
>>   		pr_err("%s: unable to add irq domain\n", np->full_name);
>> -		ret = -ENOMEM;
>> -		goto err_unmap;
>> +		return -ENOMEM;
>>   	}
>>
>> -	ret = irq_alloc_domain_generic_chips(domain, 32, (nrirqs > 32) ?
>> 2 : 1,
>> -					     np->name, handle_level_irq,
>> clr, 0,
>> +	ret = irq_alloc_domain_generic_chips(p->irq_domain, 32,
>> +					     (nrirqs > 32) ? 2 : 1,
>> np->name,
>> +					     handle_level_irq, clr, 0,
>>   					     IRQ_GC_MASK_CACHE_PER_TYPE |
>>   					     IRQ_GC_INIT_MASK_CACHE);
>>   	if (ret) {
>> -		pr_err("%s: unable to alloc irq domain gc\n",
>> np->full_name);
>> -		goto err_unmap;
>> +		dev_info(p->dev, "irq_alloc_domain_generic_chips
>> failed\n");
>> +		irq_domain_remove(p->irq_domain);
>> +		p->irq_domain = NULL;
>> +		return -ENOMEM;
>>   	}
>>
>> -	gc = irq_get_domain_generic_chip(domain, 0);
>> -	gc->private = domain;
>> +	gc = irq_get_domain_generic_chip(p->irq_domain, 0);
>> +	gc->private = p->irq_domain;
>>   	gc->reg_base = iobase;
>>
>>   	gc->chip_types[0].regs.mask = APB_INT_MASK_L;
>> @@ -159,12 +160,33 @@ static int __init dw_apb_ictl_init(struct device_node
>> *np, irq_set_chained_handler(irq, dw_apb_ictl_handler);
>>
>>   	return 0;
>> +}
>>
>> -err_unmap:
>> -	iounmap(iobase);
>> -err_release:
>> -	release_mem_region(r.start, resource_size(&r));
>> -	return ret;
>> +static int dw_apb_ictl_remove(struct platform_device *pdev)
>> +{
>> +	struct dw_apb_intl_priv *p = platform_get_drvdata(pdev);
>> +
>> +	irq_domain_remove(p->irq_domain);
>> +	return 0;
>>   }
>> -IRQCHIP_DECLARE(dw_apb_ictl,
>> -		"snps,dw-apb-ictl", dw_apb_ictl_init);
>
> So, the ictl won't be initialized in init_IRQ(), it will cause problems.
>
>> +
>> +static const struct of_device_id dw_apb_ictl_dt_ids[] = {
>> +	{ .compatible = "snps,dw-apb-ictl", },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, dw_apb_ictl_dt_ids);
>> +
>> +static struct platform_driver dw_apb_ictl_device_driver = {
>> +	.probe		= dw_apb_ictl_probe,
>> +	.remove		= dw_apb_ictl_remove,
>> +	.driver		= {
>> +		.name	= "synopsys_dw_apb_irq",
>> +		.of_match_table	= of_match_ptr(dw_apb_ictl_dt_ids),
>> +	}
>> +};
>> +
>> +module_platform_driver(dw_apb_ictl_device_driver);
>
> This is too late. If the ictl is needed during early boot, for example
> on Marvell BG2Q DMP board, we still need to calibrate the delay loop, the
> kernel will hang at "Calibrating delay loop..."
>
> Thanks,
> Jisheng
>

Re: [PATCH] irq-dw-apb-ictl: convert to platform device

[Vineet Gupta]

+CC LinusW, Alexandre, Jamie

On Wednesday 04 March 2015 02:30 PM, Sebastian Hesselbarth wrote:
> On 04.03.2015 04:11, Jisheng Zhang wrote:
>> On Tue, 3 Mar 2015 06:56:59 -0800
>> Alexey Brodkin <Alexey.Brodkin@synopsys.com> wrote:
>>> This allows utilization of probe deferral if master interrupt controller
>>> was not yet probed.
>>>
>>> In my case there's an DW APB GPIO configured as interrupt controller
>>> which is a master for DW APB INTC.
>>>
>>> Interrupt controller sub-node of DW APB GPIO gets parsed only on DW APB
>>> GPIO real probe - before that slave interrupt controllers cannot resolve
>>> its master connection which lead to failures on probes of devices that
>>> use DW APB INTC as its interrupt controller.
> Alexey,
>
> thanks for the input, but the point is not to break existing users which
> your patch clearly does.
>
> Just to get this straight for me, you are using DW_APB_INTC as a _slave_
> interrupt controller of DW_APB_GPIO _master_ interrupt controller? Is
> this a fictional setup to test your IP or is it a real world example?
>
> IMHO, if the setup is real, i.e. an APB_INTC hooked up to a GPIO
> upstream-wise, you should think of a new driver for an externally
> connected but APB bus accessible (???) interrupt controller. If it
> is rather some extension of the GPIO controller you should amend
> dw_apb_gpio.c instead.
>
> Sorry, but I cannot really wrap my head around that specific setup...
>
> Sebastian

Hi Sebastian,

I think Alexey is away for rest of the week so let me fill in some of the details.
The setup is indeed real and part of ARC SDP (Software Development Platform) and
these patches are precursor to getting that merged upstream.

SDP has a split main board / cpu card design, where both cards have an intc of own
to mux the irqs from respective levels. MB has a DW APB intc which sends a single
uplink to DW GPIO intc on cpu card, which in turn hooks up to ARC core. In the
final design, the board guys decided that this GPIO intc will act as a pass thru
for the downstream IRQ, but we still need to clear the interrupt on it etc - hence
it needs to properly show up in the cascaded intc hierarchy.

Interestingly, what we have locally in our working setup (for about an year) is an
out-of-tree dw gpio intc driver, which Mischa (no longer at Synopsys) had put
together, and it seems be more or less a copy of ur dw apb intc (although the reg
offsets specified by APB_INT_* are aligned to GPIO IP) but rest of code all seems
to be exactly same.

Does that mean if we tweak irq-dw-apb-ictlc.c enough, it could double as driver
for intc component of dw-apb-gpio too ?

Thx for looking into this.

-Vineet

>
>>> Now when this driver is converted to a normal platform device it may use
>>> probe deferral and so all dependences between devices and their
>>> interrupt controllers are resolved automatically on boot.
>>>
>>> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
>>> Cc: Vineet Gupta <vgupta@synopsys.com>
>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>> Cc: Jason Cooper <jason@lakedaemon.net>
>>> Cc: Jisheng Zhang <jszhang@marvell.com>
>>> ---
>>>   drivers/irqchip/irq-dw-apb-ictl.c | 112
>>> +++++++++++++++++++++++--------------- 1 file changed, 67 insertions(+), 45
>>> deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-dw-apb-ictl.c
>>> b/drivers/irqchip/irq-dw-apb-ictl.c index 53bb732..0b05c58 100644
>>> --- a/drivers/irqchip/irq-dw-apb-ictl.c
>>> +++ b/drivers/irqchip/irq-dw-apb-ictl.c
>>> @@ -11,13 +11,15 @@
>>>    * warranty of any kind, whether express or implied.
>>>    */
>>>
>>> -#include <linux/io.h>
>> this headfile need to be kept for readl_relaxed/writel_relaxed
>>
>>> -#include <linux/irq.h>
>>>   #include <linux/irqchip/chained_irq.h>
>>> -#include <linux/of_address.h>
>>>   #include <linux/of_irq.h>
>>> +#include <linux/of_platform.h>
>>> +#include <linux/module.h>
>> sort alphabetically
>>
>>> -#include "irqchip.h"
>>> +struct dw_apb_intl_priv {
>>> +	struct device		*dev;
>>> +	struct irq_domain	*irq_domain;
>>> +};
>>>
>>>   #define APB_INT_ENABLE_L	0x00
>>>   #define APB_INT_ENABLE_H	0x04
>>> @@ -65,41 +67,38 @@ static void dw_apb_ictl_resume(struct irq_data *d)
>>>   #define dw_apb_ictl_resume	NULL
>>>   #endif /* CONFIG_PM */
>>>
>>> -static int __init dw_apb_ictl_init(struct device_node *np,
>>> -				   struct device_node *parent)
>>> +static int dw_apb_ictl_probe(struct platform_device *pdev)
>>>   {
>>>   	unsigned int clr = IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN;
>>> -	struct resource r;
>>> -	struct irq_domain *domain;
>>> +	struct device *dev = &pdev->dev;
>>> +	struct device_node *np = dev->of_node;
>>>   	struct irq_chip_generic *gc;
>>> +	struct dw_apb_intl_priv *p;
>>> +	struct resource *res;
>>>   	void __iomem *iobase;
>>>   	int ret, nrirqs, irq;
>>>   	u32 reg;
>>>
>>> -	/* Map the parent interrupt for the chained handler */
>>> -	irq = irq_of_parse_and_map(np, 0);
>>> -	if (irq <= 0) {
>>> -		pr_err("%s: unable to parse irq\n", np->full_name);
>>> -		return -EINVAL;
>>> -	}
>>> +	if (!np)
>>> +		return -ENODEV;
>>>
>>> -	ret = of_address_to_resource(np, 0, &r);
>>> -	if (ret) {
>>> -		pr_err("%s: unable to get resource\n", np->full_name);
>>> -		return ret;
>>> +	irq = platform_get_irq(pdev, 0);
>>> +	if (irq < 0) {
>>> +		if (irq != -EPROBE_DEFER)
>>> +			dev_err(dev, "no irq resource %d\n", irq);
>>> +		return irq;
>>>   	}
>>>
>>> -	if (!request_mem_region(r.start, resource_size(&r),
>>> np->full_name)) {
>>> -		pr_err("%s: unable to request mem region\n",
>>> np->full_name);
>>> +	p = devm_kzalloc(dev, sizeof(*p), GFP_KERNEL);
>>> +	if (!p)
>>>   		return -ENOMEM;
>>> -	}
>>>
>>> -	iobase = ioremap(r.start, resource_size(&r));
>>> -	if (!iobase) {
>>> -		pr_err("%s: unable to map resource\n", np->full_name);
>>> -		ret = -ENOMEM;
>>> -		goto err_release;
>>> -	}
>>> +	p->dev = &pdev->dev;
>>> +
>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +	iobase = devm_ioremap_resource(&pdev->dev, res);
>>> +	if (IS_ERR(iobase))
>>> +		return PTR_ERR(iobase);
>>>
>>>   	/*
>>>   	 * DW IP can be configured to allow 2-64 irqs. We can determine
>>> @@ -120,25 +119,27 @@ static int __init dw_apb_ictl_init(struct device_node
>>> *np, else
>>>   		nrirqs = fls(readl_relaxed(iobase + APB_INT_ENABLE_L));
>>>
>>> -	domain = irq_domain_add_linear(np, nrirqs,
>>> -				       &irq_generic_chip_ops, NULL);
>>> -	if (!domain) {
>>> +	p->irq_domain = irq_domain_add_linear(np, nrirqs,
>>> +					      &irq_generic_chip_ops, NULL);
>>> +	if (!p->irq_domain) {
>>>   		pr_err("%s: unable to add irq domain\n", np->full_name);
>>> -		ret = -ENOMEM;
>>> -		goto err_unmap;
>>> +		return -ENOMEM;
>>>   	}
>>>
>>> -	ret = irq_alloc_domain_generic_chips(domain, 32, (nrirqs > 32) ?
>>> 2 : 1,
>>> -					     np->name, handle_level_irq,
>>> clr, 0,
>>> +	ret = irq_alloc_domain_generic_chips(p->irq_domain, 32,
>>> +					     (nrirqs > 32) ? 2 : 1,
>>> np->name,
>>> +					     handle_level_irq, clr, 0,
>>>   					     IRQ_GC_MASK_CACHE_PER_TYPE |
>>>   					     IRQ_GC_INIT_MASK_CACHE);
>>>   	if (ret) {
>>> -		pr_err("%s: unable to alloc irq domain gc\n",
>>> np->full_name);
>>> -		goto err_unmap;
>>> +		dev_info(p->dev, "irq_alloc_domain_generic_chips
>>> failed\n");
>>> +		irq_domain_remove(p->irq_domain);
>>> +		p->irq_domain = NULL;
>>> +		return -ENOMEM;
>>>   	}
>>>
>>> -	gc = irq_get_domain_generic_chip(domain, 0);
>>> -	gc->private = domain;
>>> +	gc = irq_get_domain_generic_chip(p->irq_domain, 0);
>>> +	gc->private = p->irq_domain;
>>>   	gc->reg_base = iobase;
>>>
>>>   	gc->chip_types[0].regs.mask = APB_INT_MASK_L;
>>> @@ -159,12 +160,33 @@ static int __init dw_apb_ictl_init(struct device_node
>>> *np, irq_set_chained_handler(irq, dw_apb_ictl_handler);
>>>
>>>   	return 0;
>>> +}
>>>
>>> -err_unmap:
>>> -	iounmap(iobase);
>>> -err_release:
>>> -	release_mem_region(r.start, resource_size(&r));
>>> -	return ret;
>>> +static int dw_apb_ictl_remove(struct platform_device *pdev)
>>> +{
>>> +	struct dw_apb_intl_priv *p = platform_get_drvdata(pdev);
>>> +
>>> +	irq_domain_remove(p->irq_domain);
>>> +	return 0;
>>>   }
>>> -IRQCHIP_DECLARE(dw_apb_ictl,
>>> -		"snps,dw-apb-ictl", dw_apb_ictl_init);
>> So, the ictl won't be initialized in init_IRQ(), it will cause problems.
>>
>>> +
>>> +static const struct of_device_id dw_apb_ictl_dt_ids[] = {
>>> +	{ .compatible = "snps,dw-apb-ictl", },
>>> +	{},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, dw_apb_ictl_dt_ids);
>>> +
>>> +static struct platform_driver dw_apb_ictl_device_driver = {
>>> +	.probe		= dw_apb_ictl_probe,
>>> +	.remove		= dw_apb_ictl_remove,
>>> +	.driver		= {
>>> +		.name	= "synopsys_dw_apb_irq",
>>> +		.of_match_table	= of_match_ptr(dw_apb_ictl_dt_ids),
>>> +	}
>>> +};
>>> +
>>> +module_platform_driver(dw_apb_ictl_device_driver);
>> This is too late. If the ictl is needed during early boot, for example
>> on Marvell BG2Q DMP board, we still need to calibrate the delay loop, the
>> kernel will hang at "Calibrating delay loop..."
>>
>> Thanks,
>> Jisheng
>>
>

Re: [PATCH] irq-dw-apb-ictl: convert to platform device

[Sebastian Hesselbarth]

On 03/04/2015 11:46 AM, Vineet Gupta wrote:
> On Wednesday 04 March 2015 02:30 PM, Sebastian Hesselbarth wrote:
>> On 04.03.2015 04:11, Jisheng Zhang wrote:
>>> On Tue, 3 Mar 2015 06:56:59 -0800
>>> Alexey Brodkin <Alexey.Brodkin@synopsys.com> wrote:
>>>> This allows utilization of probe deferral if master interrupt controller
>>>> was not yet probed.
>>>>
>>>> In my case there's an DW APB GPIO configured as interrupt controller
>>>> which is a master for DW APB INTC.
>>>>
[...]
>> Just to get this straight for me, you are using DW_APB_INTC as a _slave_
>> interrupt controller of DW_APB_GPIO _master_ interrupt controller? Is
>> this a fictional setup to test your IP or is it a real world example?
[...]
>
> I think Alexey is away for rest of the week so let me fill in some of the details.
> The setup is indeed real and part of ARC SDP (Software Development Platform) and
> these patches are precursor to getting that merged upstream.
>
> SDP has a split main board / cpu card design, where both cards have an intc of own
> to mux the irqs from respective levels. MB has a DW APB intc which sends a single
> uplink to DW GPIO intc on cpu card, which in turn hooks up to ARC core. In the
> final design, the board guys decided that this GPIO intc will act as a pass thru
> for the downstream IRQ, but we still need to clear the interrupt on it etc - hence
> it needs to properly show up in the cascaded intc hierarchy.

Vineet, thanks for the clarification. So, above describes the following
setup:

APB_INTC->(GPIO line)->APB_GPIO->APB_INTC->(Main INTC)

I see that current early probing of APB_INTC does not fit well into the
picture.

> Does that mean if we tweak irq-dw-apb-ictlc.c enough, it could double as driver
> for intc component of dw-apb-gpio too ?

Hmm, I am inclined to say that if it is the setup above and not some
tweaked hardware you have, we should try to rework the existing drivers
to allow the setup.

We could share code of dw-apb-intc and try to distinguish early probed
vs late probed devices, but I haven't thought in detail how to achieve
this in a clean way.

Another option would be...

[...]
>>>> +module_platform_driver(dw_apb_ictl_device_driver);
>>> This is too late. If the ictl is needed during early boot, for example
>>> on Marvell BG2Q DMP board, we still need to calibrate the delay loop, the
>>> kernel will hang at "Calibrating delay loop..."

... to make Berlin use another timer that is not connected to
dw-apb-intc slave controller. IIRC, there is a TWD timer in all
(current) Berlin SoCs and it is directly connected to the master
intc. We'll have to rework (silence) current TWD implementation to
be usable on BG2CD UP and I haven't thought about the second clock
source, yet.

Also, I admit it will most likely just postpone the general issue to
the next SoC.

I'll have to have a deeper look into kernel sources, it has been a while
I looked at this in detail. In the meantime, I hope there is somebody
else with a better idea ;)

Sebastian

Re: [PATCH] irq-dw-apb-ictl: convert to platform device

[Vineet Gupta]

On Wednesday 04 March 2015 05:30 PM, Sebastian Hesselbarth wrote:
> On 03/04/2015 11:46 AM, Vineet Gupta wrote:
>> > On Wednesday 04 March 2015 02:30 PM, Sebastian Hesselbarth wrote:
>>> >> On 04.03.2015 04:11, Jisheng Zhang wrote:
>>>> >>> On Tue, 3 Mar 2015 06:56:59 -0800
>>>> >>> Alexey Brodkin <Alexey.Brodkin@synopsys.com> wrote:
>>>>> >>>> This allows utilization of probe deferral if master interrupt controller
>>>>> >>>> was not yet probed.
>>>>> >>>>
>>>>> >>>> In my case there's an DW APB GPIO configured as interrupt controller
>>>>> >>>> which is a master for DW APB INTC.
>>>>> >>>>
> [...]
>>> >> Just to get this straight for me, you are using DW_APB_INTC as a _slave_
>>> >> interrupt controller of DW_APB_GPIO _master_ interrupt controller? Is
>>> >> this a fictional setup to test your IP or is it a real world example?
> [...]
>> >
>> > I think Alexey is away for rest of the week so let me fill in some of the details.
>> > The setup is indeed real and part of ARC SDP (Software Development Platform) and
>> > these patches are precursor to getting that merged upstream.
>> >
>> > SDP has a split main board / cpu card design, where both cards have an intc of own
>> > to mux the irqs from respective levels. MB has a DW APB intc which sends a single
>> > uplink to DW GPIO intc on cpu card, which in turn hooks up to ARC core. In the
>> > final design, the board guys decided that this GPIO intc will act as a pass thru
>> > for the downstream IRQ, but we still need to clear the interrupt on it etc - hence
>> > it needs to properly show up in the cascaded intc hierarchy.
> Vineet, thanks for the clarification. So, above describes the following
> setup:
>
> APB_INTC->(GPIO line)->APB_GPIO->APB_INTC->(Main INTC)

Not quite, we only have 1 DW APB intc (see ASCII art below)

Ethernet or xyz device IRQ -> APB INTC -> GPIO line -> APB GPIO -> ARC Core intc

>
> I see that current early probing of APB_INTC does not fit well into the
> picture.

>> > Does that mean if we tweak irq-dw-apb-ictlc.c enough, it could double as driver
>> > for intc component of dw-apb-gpio too ?
> Hmm, I am inclined to say that if it is the setup above and not some
> tweaked hardware you have, we should try to rework the existing drivers
> to allow the setup.

Above is part 1 of setup - although to give you complete picture (perhaps
orthogonal to this exact issue), we have another GPIO intc at cpu level which is
used to wire up the cpu local peripherals (e.g. debug uart). Sounds a bit crazy,
but it is what it is.

        ---------------------
        |  snps,arc700-intc |
        ---------------------
          | #7          | #15
-------------------  ------------------
| DW GPIO INTC    |  | DW GPIO INTC   |
-------------------  ------------------
         |                       |        
         |                    [ Debug UART on cpu card ]
         |
--------------------    
|  DW APB INTC (MB)|
--------------------
  |      |     |  |
[eth] [uart] [... other perip on Main Board]


Note the APB INTC to GPIO intc is more like a single wire pass thru.


> We could share code of dw-apb-intc and try to distinguish early probed
> vs late probed devices, but I haven't thought in detail how to achieve
> this in a clean way.
>
> Another option would be...
>
> [...]
>>>>> >>>> +module_platform_driver(dw_apb_ictl_device_driver);
>>>> >>> This is too late. If the ictl is needed during early boot, for example
>>>> >>> on Marvell BG2Q DMP board, we still need to calibrate the delay loop, the
>>>> >>> kernel will hang at "Calibrating delay loop..."
> ... to make Berlin use another timer that is not connected to
> dw-apb-intc slave controller. IIRC, there is a TWD timer in all
> (current) Berlin SoCs and it is directly connected to the master
> intc. We'll have to rework (silence) current TWD implementation to
> be usable on BG2CD UP and I haven't thought about the second clock
> source, yet.
>
> Also, I admit it will most likely just postpone the general issue to
> the next SoC.

Yeah I don't like this approach - we fix it now for good !

>
> I'll have to have a deeper look into kernel sources, it has been a while
> I looked at this in detail. In the meantime, I hope there is somebody
> else with a better idea ;)

Lets see :-)

Thx,
-Vineet

Re: [PATCH] irq-dw-apb-ictl: convert to platform device

[Linus Walleij]

On Wed, Mar 4, 2015 at 11:46 AM, Vineet Gupta
<Vineet.Gupta1@synopsys.com> wrote:
> On Wednesday 04 March 2015 02:30 PM, Sebastian Hesselbarth wrote:
>> On 04.03.2015 04:11, Jisheng Zhang wrote:
>>> On Tue, 3 Mar 2015 06:56:59 -0800
>>> Alexey Brodkin <Alexey.Brodkin@synopsys.com> wrote:
>>>> This allows utilization of probe deferral if master interrupt controller
>>>> was not yet probed.
>>>>
>>>> In my case there's an DW APB GPIO configured as interrupt controller
>>>> which is a master for DW APB INTC.
>>>>
>>>> Interrupt controller sub-node of DW APB GPIO gets parsed only on DW APB
>>>> GPIO real probe - before that slave interrupt controllers cannot resolve
>>>> its master connection which lead to failures on probes of devices that
>>>> use DW APB INTC as its interrupt controller.
>> Alexey,
>>
>> thanks for the input, but the point is not to break existing users which
>> your patch clearly does.
>>
>> Just to get this straight for me, you are using DW_APB_INTC as a _slave_
>> interrupt controller of DW_APB_GPIO _master_ interrupt controller? Is
>> this a fictional setup to test your IP or is it a real world example?
>>
>> IMHO, if the setup is real, i.e. an APB_INTC hooked up to a GPIO
>> upstream-wise, you should think of a new driver for an externally
>> connected but APB bus accessible (???) interrupt controller. If it
>> is rather some extension of the GPIO controller you should amend
>> dw_apb_gpio.c instead.
>>
>> Sorry, but I cannot really wrap my head around that specific setup...
>>
>> Sebastian
>
> Hi Sebastian,
>
> I think Alexey is away for rest of the week so let me fill in some of the details.
> The setup is indeed real and part of ARC SDP (Software Development Platform) and
> these patches are precursor to getting that merged upstream.
>
> SDP has a split main board / cpu card design, where both cards have an intc of own
> to mux the irqs from respective levels. MB has a DW APB intc which sends a single
> uplink to DW GPIO intc on cpu card, which in turn hooks up to ARC core. In the
> final design, the board guys decided that this GPIO intc will act as a pass thru
> for the downstream IRQ, but we still need to clear the interrupt on it etc - hence
> it needs to properly show up in the cascaded intc hierarchy.

We have hierarchical IRQ domains in the Linux kernel since a while
back and I think that is what you want here.

I am looking at some way to have any GPIOLIB_IRQCHIP turn up
as a proper hierarchical irqchip but that means getting at the domain
from an IRQ number, not just knowing it from context so have not
gotten around to fixing this yet...

We have several cascaded irqchip drivers doing this already however,
look in drivers/irqchip/irq-vic.c function vic_init_cascaded() for example.
A cascaded GPIO-based irqchip is no different for what I can tell.
Just call irq_set_chained_handler() and be happy.

Well there is also irq_set_parent(), which the gpiolib irqchip calls
and which is helpful for the situation where the irq needs to be
resent (see kernel/irq/resend.c).

I set this also for the cascaded irqs on GPIOLIB_IRQCHIP but
don't know for sure if it's needed.

By the way please if you have time convert the DWAPB driver
to GPIOLIB_IRQCHIP so we can solve problems like this in the
GPIO core and need not deal with it in a myriad of drivers...

Yours,
Linus Walleij