diff mbox

[v2,4/7] ARM: pxa: add devicetree code for irq handling

Message ID 50158890.8080504@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Mack July 29, 2012, 7:01 p.m. UTC
On 29.07.2012 17:54, Haojian Zhuang wrote:
> On Sun, Jul 29, 2012 at 11:08 PM, Daniel Mack <zonque@gmail.com> wrote:
>> Hi Haojian,
>>
>> On 28.07.2012 17:42, Haojian Zhuang wrote:
>>> On Sat, Jul 28, 2012 at 5:56 PM, Daniel Mack <zonque@gmail.com> wrote:
>>>> On 28.07.2012 09:17, Haojian Zhuang wrote:
>>>>> On Fri, Jul 27, 2012 at 3:16 AM, Daniel Mack <zonque@gmail.com> wrote:
>>>>>> Properly register on-chip interrupt using the irqdomain logic. The
>>>>>> number of interrupts is taken from the devicetree node.
>>>>>>
>>>>>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>>>>>> ---
>>>>>>  arch/arm/mach-pxa/irq.c    | 73 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>  arch/arm/mach-pxa/pxa3xx.c | 17 +++++++++--
>>>>>>  2 files changed, 88 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> +#ifdef CONFIG_OF
>>>>>> +static struct irq_domain *pxa_irq_domain;
>>>>>> +
>>>>>> +static int pxa_irq_map(struct irq_domain *h, unsigned int virq,
>>>>>> +                      irq_hw_number_t hw)
>>>>>> +{
>>>>>> +       int irq, i = hw % 32;
>>>>>> +       void __iomem *base = irq_base(hw / 32);
>>>>>> +
>>>>>> +       /* initialize interrupt priority */
>>>>>> +       if (cpu_has_ipr())
>>>>>> +               __raw_writel(i | IPR_VALID, IRQ_BASE + IPR(i));
>>>>> Since we have DT support at here. Could we use property for interrupt priority?
>>>>
>>>> Not sure what you mean here. Can you elaborate? I couldn't find any
>>>> reference to IRQ priorities in other platforms either.
>>>>
>>>> Maybe we can also add that in a separate patch, which would also help in
>>>> tracking possible regressions du to such a change?
>>>>
>>> cpu_has_ipr() returns true if CPU isn't PXA25x.
>>> My point is that we can avoid to use cpu_is_xxx() while DT is used. We only need
>>> to append a property "marvell,intc-priority" is DTS. So the code could
>>> be changed
>>> in below.
>>> if (of_find_property(np, "marvell,intc-priority", NULL))
>>>            __raw_writel(i | IPR_VALID, IRQ_BASE + IPR(i));
>>>
>>>>>> +       irq = PXA_IRQ(virq);
>>>>> #ifdef CONFIG_PXA_HAVE_ISA_IRQS
>>>>> #define PXA_ISA_IRQ(x)  (x)
>>>>> #define PXA_ISA_IRQ_NUM (16)
>>>>> #else
>>>>> #define PXA_ISA_IRQ_NUM (0)
>>>>> #endif
>>>>>
>>>>> Could we avoid to use PXA_IRQ() at here? We can make use of
>>>>> NR_IRQS_LEGACY that is 16. Since you already use irq_alloc_descs()
>>>>> to allocate irqs that virtual irq number starts from 16. So you needn't
>>>>> use PXA_IRQ() any more.
>>>>
>>>> Ok, I changed this. Note that there's still need to subtract
>>>> NR_IRQS_LEGACY from the virq that is passed in to the .map function,
>>>> because early_irq_init() in kernel/irq/irqdesc.c will pre-allocate the
>>>> IRQs the platform claims to have natively, which defaults to 16 on PXA,
>>>> unless the machine descriptor sets nr_irqs, which it doesn't in case of DT.
>>>>
>>> You needn't subtract NR_IRQS_LEGACY. PXA25x hwirq starts from
>>> 16 & PXA27x/PXA3xx hwirq starts from 0. While DT is used, irq_alloc_descs()
>>> allocates virq from NR_IRQS_LEGACY. For PXA25x, there's exactly match.
>>> For PXA27x/PXA3xx, there's a little different. But it doesn't matter. We needn't
>>> force virq starting from 0 on PXA27x/PXA3xx. The first virq starts from 16 is
>>> also OK.
>>
>> Ok, now I got you. By simply ignoring the virq passed in and only taking
>> into account the hw irq, this is of course possible.
>>
>> Please see the attached patch. Does that look better to you? I removed
>> the cpu_has_ipr() inline function and made it a variable that is used
>> and initalized from both the DT and the legacy code.
>>
>>
>> Daniel
>>
> 
> Yes, both of these two are fixed perfectly. Now let's focus on this in below.
> I just find it.
> 
> +	if (cpu_has_ipr)
> +		__raw_writel(bit | IPR_VALID, IRQ_BASE + IPR(bit));
> 
> #define IRQ_BASE                io_p2v(0x40d00000)
> 
> IRQ_BASE is defined in arch/arm/mach-pxa/irq.c. It's OK for non-DT mode.
> If we want to support DT, I hope that all registers mapping should be covered by
> of_iomap(). We should discard this kind of static register mapping. You can
> find some reference in current code base.

Ok, you're right. I thought it's ok to keep it that way as the entire
code here is really limited to pxa SoCs (which all have the same
physical address offset), but we should indeed take as much information
as possible from the tree.

Please check the appended version. What I also changed now is that
pxa_init_irq() only really handles non-DT initialization, so we have to
care for some details in pxa_dt_irq_init() separately. Also, the map()
function still had a bug, which I also fixed.


Thanks for your feedback,
Daniel

Comments

Haojian Zhuang July 30, 2012, 1:20 a.m. UTC | #1
On Mon, Jul 30, 2012 at 3:01 AM, Daniel Mack <zonque@gmail.com> wrote:
>
> Please check the appended version. What I also changed now is that
> pxa_init_irq() only really handles non-DT initialization, so we have to
> care for some details in pxa_dt_irq_init() separately. Also, the map()
> function still had a bug, which I also fixed.
>
>
Acked-by: Haojian Zhuang <haojian.zhuang@gmail.com>

Since "marvell,intc-priority" is appended. Maybe you need to update the DTSI
file. How about send out the updated v4 patch series? I'll merge it in
this week.

Regards
Haojian
Haojian Zhuang July 30, 2012, 1:24 a.m. UTC | #2
On Mon, Jul 30, 2012 at 9:20 AM, Haojian Zhuang
<haojian.zhuang@gmail.com> wrote:
> On Mon, Jul 30, 2012 at 3:01 AM, Daniel Mack <zonque@gmail.com> wrote:
>>
>> Please check the appended version. What I also changed now is that
>> pxa_init_irq() only really handles non-DT initialization, so we have to
>> care for some details in pxa_dt_irq_init() separately. Also, the map()
>> function still had a bug, which I also fixed.
>>
>>
> Acked-by: Haojian Zhuang <haojian.zhuang@gmail.com>
>
> Since "marvell,intc-priority" is appended. Maybe you need to update the DTSI
> file. How about send out the updated v4 patch series? I'll merge it in
> this week.
>
> Regards
> Haojian

I requested for you sending patch since 2 patches in v3 are lost in my
gmail account.

Regards
Haojian
Daniel Mack July 30, 2012, 7:11 a.m. UTC | #3
On 30.07.2012 03:24, Haojian Zhuang wrote:
> On Mon, Jul 30, 2012 at 9:20 AM, Haojian Zhuang
> <haojian.zhuang@gmail.com> wrote:
>> On Mon, Jul 30, 2012 at 3:01 AM, Daniel Mack <zonque@gmail.com> wrote:
>>>
>>> Please check the appended version. What I also changed now is that
>>> pxa_init_irq() only really handles non-DT initialization, so we have to
>>> care for some details in pxa_dt_irq_init() separately. Also, the map()
>>> function still had a bug, which I also fixed.
>>>
>>>
>> Acked-by: Haojian Zhuang <haojian.zhuang@gmail.com>
>>
>> Since "marvell,intc-priority" is appended. Maybe you need to update the DTSI
>> file. How about send out the updated v4 patch series? I'll merge it in
>> this week.
>>
>> Regards
>> Haojian
> 
> I requested for you sending patch since 2 patches in v3 are lost in my
> gmail account.

To avoid mailing out the whole series to everybody again, even though
most of them haven't changed, I prepared a branch that includes all 9
patches on top of Linus' current tree:

  git://github.com/zonque/linux.git pxa-dt

Is it ok for you to pick the patches from there? Otherwise, I can of
course send them out by email again as well.


Thanks,
Daniel
diff mbox

Patch

From 5ad13e2826d6b7d1c51a4c1545514dd38c227839 Mon Sep 17 00:00:00 2001
From: Daniel Mack <zonque@gmail.com>
Date: Sun, 22 Jul 2012 19:50:22 +0200
Subject: [PATCH v4] ARM: pxa: add devicetree code for irq handling

Properly register on-chip interrupt using the irqdomain logic. The
number of interrupts is taken from the devicetree node. That includes
the following changes:

- cpu_has_ipr() was converted from an inline function to a static bool
variable, so it can be set using the "marvell,intc-priority" property
inside the device node of the tree.

- IRQ_BASE was converted from a macro to a runtime variable so that it
can be initialized dynamically from the DT init code.

- irq_base() now uses pxa_irq_base and just adds an offset.

Hence, there are now no compile-time fixed values used in case of DT
initialization.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 arch/arm/mach-pxa/irq.c    | 131 +++++++++++++++++++++++++++++++++++++--------
 arch/arm/mach-pxa/pxa3xx.c |  17 +++++-
 2 files changed, 125 insertions(+), 23 deletions(-)

diff --git a/arch/arm/mach-pxa/irq.c b/arch/arm/mach-pxa/irq.c
index 5dae15e..4213a4e 100644
--- a/arch/arm/mach-pxa/irq.c
+++ b/arch/arm/mach-pxa/irq.c
@@ -17,6 +17,8 @@ 
 #include <linux/syscore_ops.h>
 #include <linux/io.h>
 #include <linux/irq.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
 
 #include <asm/exception.h>
 
@@ -25,8 +27,6 @@ 
 
 #include "generic.h"
 
-#define IRQ_BASE		io_p2v(0x40d00000)
-
 #define ICIP			(0x000)
 #define ICMR			(0x004)
 #define ICLR			(0x008)
@@ -48,22 +48,19 @@ 
  * This is for peripheral IRQs internal to the PXA chip.
  */
 
+static void __iomem *pxa_irq_base;
 static int pxa_internal_irq_nr;
-
-static inline int cpu_has_ipr(void)
-{
-	return !cpu_is_pxa25x();
-}
+static bool cpu_has_ipr;
 
 static inline void __iomem *irq_base(int i)
 {
-	static unsigned long phys_base[] = {
-		0x40d00000,
-		0x40d0009c,
-		0x40d00130,
+	static unsigned long phys_base_offset[] = {
+		0x0,
+		0x9c,
+		0x130,
 	};
 
-	return io_p2v(phys_base[i]);
+	return pxa_irq_base + phys_base_offset[i];
 }
 
 void pxa_mask_irq(struct irq_data *d)
@@ -96,8 +93,8 @@  asmlinkage void __exception_irq_entry icip_handle_irq(struct pt_regs *regs)
 	uint32_t icip, icmr, mask;
 
 	do {
-		icip = __raw_readl(IRQ_BASE + ICIP);
-		icmr = __raw_readl(IRQ_BASE + ICMR);
+		icip = __raw_readl(pxa_irq_base + ICIP);
+		icmr = __raw_readl(pxa_irq_base + ICMR);
 		mask = icip & icmr;
 
 		if (mask == 0)
@@ -128,6 +125,8 @@  void __init pxa_init_irq(int irq_nr, int (*fn)(struct irq_data *, unsigned int))
 	BUG_ON(irq_nr > MAX_INTERNAL_IRQS);
 
 	pxa_internal_irq_nr = irq_nr;
+	cpu_has_ipr = !cpu_is_pxa25x();
+	pxa_irq_base = io_p2v(0x40d00000);
 
 	for (n = 0; n < irq_nr; n += 32) {
 		void __iomem *base = irq_base(n >> 5);
@@ -136,8 +135,8 @@  void __init pxa_init_irq(int irq_nr, int (*fn)(struct irq_data *, unsigned int))
 		__raw_writel(0, base + ICLR);	/* all IRQs are IRQ, not FIQ */
 		for (i = n; (i < (n + 32)) && (i < irq_nr); i++) {
 			/* initialize interrupt priority */
-			if (cpu_has_ipr())
-				__raw_writel(i | IPR_VALID, IRQ_BASE + IPR(i));
+			if (cpu_has_ipr)
+				__raw_writel(i | IPR_VALID, pxa_irq_base + IPR(i));
 
 			irq = PXA_IRQ(i);
 			irq_set_chip_and_handler(irq, &pxa_internal_irq_chip,
@@ -168,9 +167,9 @@  static int pxa_irq_suspend(void)
 		__raw_writel(0, base + ICMR);
 	}
 
-	if (cpu_has_ipr()) {
+	if (cpu_has_ipr) {
 		for (i = 0; i < pxa_internal_irq_nr; i++)
-			saved_ipr[i] = __raw_readl(IRQ_BASE + IPR(i));
+			saved_ipr[i] = __raw_readl(pxa_irq_base + IPR(i));
 	}
 
 	return 0;
@@ -187,11 +186,11 @@  static void pxa_irq_resume(void)
 		__raw_writel(0, base + ICLR);
 	}
 
-	if (cpu_has_ipr())
+	if (cpu_has_ipr)
 		for (i = 0; i < pxa_internal_irq_nr; i++)
-			__raw_writel(saved_ipr[i], IRQ_BASE + IPR(i));
+			__raw_writel(saved_ipr[i], pxa_irq_base + IPR(i));
 
-	__raw_writel(1, IRQ_BASE + ICCR);
+	__raw_writel(1, pxa_irq_base + ICCR);
 }
 #else
 #define pxa_irq_suspend		NULL
@@ -202,3 +201,93 @@  struct syscore_ops pxa_irq_syscore_ops = {
 	.suspend	= pxa_irq_suspend,
 	.resume		= pxa_irq_resume,
 };
+
+#ifdef CONFIG_OF
+static struct irq_domain *pxa_irq_domain;
+
+static int pxa_irq_map(struct irq_domain *h, unsigned int virq,
+		       irq_hw_number_t hw)
+{
+	void __iomem *base = irq_base(hw / 32);
+
+	/* initialize interrupt priority */
+	if (cpu_has_ipr)
+		__raw_writel(hw | IPR_VALID, pxa_irq_base + IPR(hw));
+
+	irq_set_chip_and_handler(hw, &pxa_internal_irq_chip,
+				 handle_level_irq);
+	irq_set_chip_data(hw, base);
+	set_irq_flags(hw, IRQF_VALID);
+
+	return 0;
+}
+
+static struct irq_domain_ops pxa_irq_ops = {
+	.map    = pxa_irq_map,
+	.xlate  = irq_domain_xlate_onecell,
+};
+
+static const struct of_device_id intc_ids[] __initconst = {
+	{ .compatible = "marvell,pxa-intc", },
+	{}
+};
+
+void __init pxa_dt_irq_init(int (*fn)(struct irq_data *, unsigned int))
+{
+	struct device_node *node;
+	const struct of_device_id *of_id;
+	struct pxa_intc_conf *conf;
+	struct resource res;
+	int n, ret;
+
+	node = of_find_matching_node(NULL, intc_ids);
+	if (!node) {
+		pr_err("Failed to find interrupt controller in arch-pxa\n");
+		return;
+	}
+	of_id = of_match_node(intc_ids, node);
+	conf = of_id->data;
+
+	ret = of_property_read_u32(node, "mrvl,intc-nr-irqs",
+				   &pxa_internal_irq_nr);
+	if (ret) {
+		pr_err("Not found mrvl,intc-nr-irqs property\n");
+		return;
+	}
+
+	ret = of_address_to_resource(node, 0, &res);
+	if (ret < 0) {
+		pr_err("No registers defined for node\n");
+		return;
+	}
+	pxa_irq_base = io_p2v(res.start);
+
+	if (of_find_property(node, "marvell,intc-priority", NULL))
+		cpu_has_ipr = 1;
+
+	ret = irq_alloc_descs(-1, 0, pxa_internal_irq_nr, 0);
+	if (ret < 0) {
+		pr_err("Failed to allocate IRQ numbers\n");
+		return;
+	}
+
+	pxa_irq_domain = irq_domain_add_legacy(node, pxa_internal_irq_nr, 0, 0,
+					       &pxa_irq_ops, NULL);
+	if (!pxa_irq_domain)
+		panic("Unable to add PXA IRQ domain\n");
+
+	irq_set_default_host(pxa_irq_domain);
+
+	for (n = 0; n < pxa_internal_irq_nr; n += 32) {
+		void __iomem *base = irq_base(n >> 5);
+
+		__raw_writel(0, base + ICMR);	/* disable all IRQs */
+		__raw_writel(0, base + ICLR);	/* all IRQs are IRQ, not FIQ */
+	}
+
+	/* only unmasked interrupts kick us out of idle */
+	__raw_writel(1, irq_base(0) + ICCR);
+
+	pxa_internal_irq_chip.irq_set_wake = fn;
+}
+#endif /* CONFIG_OF */
diff --git a/arch/arm/mach-pxa/pxa3xx.c b/arch/arm/mach-pxa/pxa3xx.c
index dffb7e8..1827d3c 100644
--- a/arch/arm/mach-pxa/pxa3xx.c
+++ b/arch/arm/mach-pxa/pxa3xx.c
@@ -40,6 +40,8 @@ 
 #define PECR_IE(n)	((1 << ((n) * 2)) << 28)
 #define PECR_IS(n)	((1 << ((n) * 2)) << 29)
 
+extern void __init pxa_dt_irq_init(int (*fn)(struct irq_data *, unsigned int));
+
 static DEFINE_PXA3_CKEN(pxa3xx_ffuart, FFUART, 14857000, 1);
 static DEFINE_PXA3_CKEN(pxa3xx_btuart, BTUART, 14857000, 1);
 static DEFINE_PXA3_CKEN(pxa3xx_stuart, STUART, 14857000, 1);
@@ -382,7 +384,7 @@  static void __init pxa_init_ext_wakeup_irq(int (*fn)(struct irq_data *,
 	pxa_ext_wakeup_chip.irq_set_wake = fn;
 }
 
-void __init pxa3xx_init_irq(void)
+static void __init __pxa3xx_init_irq(void)
 {
 	/* enable CP6 access */
 	u32 value;
@@ -390,10 +392,21 @@  void __init pxa3xx_init_irq(void)
 	value |= (1 << 6);
 	__asm__ __volatile__("mcr p15, 0, %0, c15, c1, 0\n": :"r"(value));
 
-	pxa_init_irq(56, pxa3xx_set_wake);
 	pxa_init_ext_wakeup_irq(pxa3xx_set_wake);
 }
 
+void __init pxa3xx_init_irq(void)
+{
+	__pxa3xx_init_irq();
+	pxa_init_irq(56, pxa3xx_set_wake);
+}
+
+void __init pxa3xx_dt_init_irq(void)
+{
+	__pxa3xx_init_irq();
+	pxa_dt_irq_init(pxa3xx_set_wake);
+}
+
 static struct map_desc pxa3xx_io_desc[] __initdata = {
 	{	/* Mem Ctl */
 		.virtual	= (unsigned long)SMEMC_VIRT,
-- 
1.7.11.2