On Sun, Jul 29, 2012 at 11:08 PM, Daniel Mack [off-list ref] wrote:

Hi Haojian,

On 28.07.2012 17:42, Haojian Zhuang wrote:

quoted

On Sat, Jul 28, 2012 at 5:56 PM, Daniel Mack [off-list ref] wrote:

quoted

On 28.07.2012 09:17, Haojian Zhuang wrote:

quoted

On Fri, Jul 27, 2012 at 3:16 AM, Daniel Mack [off-list ref] wrote:

quoted

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));

quoted

quoted

quoted

+       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.

Regards
Haojian