On Fri, Nov 15, 2013 at 11:49:20AM +0000, Neil Zhang wrote: > > > -----Original Message----- > > From: Mark Rutland [mailto:mark.rutland@xxxxxxx] > > Sent: 2013年11月14日 20:28 > > To: Haojian Zhuang > > Cc: Neil Zhang; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > > Thomas Gleixner > > Subject: Re: [PATCH 2/2 v2] irqchip: mmp: add dt support for wakeup > > > > On Thu, Nov 14, 2013 at 10:28:53AM +0000, Haojian Zhuang wrote: > > > On Fri, Oct 11, 2013 at 4:23 PM, Neil Zhang <zhangwm@xxxxxxxxxxx> wrote: > > > > Some of the Marvell SoCs use GIC as its interrupt controller,and ICU > > > > only used as wakeup logic. When AP subsystem is powered off, GIC > > > > will lose its context, the PMU will need ICU to wakeup the AP subsystem. > > > > So add wakeup entry for such kind of usage. > > > > > > > > Signed-off-by: Neil Zhang <zhangwm@xxxxxxxxxxx> > > > > --- > > > > .../devicetree/bindings/arm/mrvl/intc.txt | 14 ++- > > > > drivers/irqchip/irq-mmp.c | 124 > > ++++++++++++++++++++ > > > > include/linux/irqchip/mmp.h | 13 ++ > > > > 3 files changed, 150 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/Documentation/devicetree/bindings/arm/mrvl/intc.txt > > > > b/Documentation/devicetree/bindings/arm/mrvl/intc.txt > > > > index 8b53273..4180928 100644 > > > > --- a/Documentation/devicetree/bindings/arm/mrvl/intc.txt > > > > +++ b/Documentation/devicetree/bindings/arm/mrvl/intc.txt > > > > @@ -2,7 +2,7 @@ > > > > > > > > Required properties: > > > > - compatible : Should be "mrvl,mmp-intc", "mrvl,mmp2-intc" or > > > > - "mrvl,mmp2-mux-intc" > > > > + "mrvl,mmp2-mux-intc", "mrvl,mmp-intc-wakeupgen" > > > > Why do we need a new compatible string? > > As the patch comments said, we don't use the ICU as an interrupt controller in some Marvell Socs, > Just use them to wakeup CPU when GIC is powered off. Hmm. Is it possible to use the ICU as an interrupt controller in those SoCs? > > > > > > > - reg : Address and length of the register set of the interrupt controller. > > > > If the interrupt controller is intc, address and length means the range > > > > of the whold interrupt controller. If the interrupt controller is > > > > mux-intc, @@ -15,6 +15,9 @@ Required properties: > > > > - interrupt-controller : Identifies the node as an interrupt controller. > > > > - #interrupt-cells : Specifies the number of cells needed to encode an > > > > interrupt source. > > > > +- mrvl,intc-gbl-mask : Specifies the address and value for global > > > > +mask in the > > > > + interrupt controller. > > > > > > As my understanding, we should avoid to write register settings in DTS file. > > > > In general, yes. We should describe the hardware and let Linux choose how to > > configure it as far as possible. > > > > What is this global mask? What is it used for? Why do there seem to be multiple > > global masks (judging by the example)? > > > > Global mask will prevent distributing interrupt from ICU to GIC. > Since we will use GIC as the interrupt controller, so we need to mask the ICU global mask. > ICU has connection to every core in the system, so we need to mask all global mask registers for each core. Why can the driver not figure out these masks for itself? > > > > > > > Loop devicetree guys. > > > > > > > +- mrvl,intc-for-cp : Specifies the irqs that will be routed to cp > > > > cp? > > > > _why_ do we need this, and what exactly does routing the irqs to the cp imply? > > Communication processor. > Kernel should avoid to handle the irq lines that has been routed to communication processor. Ok. Does this just tell the kernel the set of IRQs to ignore, or does this imply that the kernel must configure something in the hardware based on this? If so, what specifically? > > > > > > > - mrvl,intc-nr-irqs : Specifies the number of interrupts in the interrupt > > > > controller. > > > > - mrvl,clr-mfp-irq : Specifies the interrupt that needs to clear > > > > MFP edge @@ -39,6 +42,15 @@ Example: > > > > mrvl,intc-nr-irqs = <2>; > > > > }; > > > > > > > > + intc: wakeupgen@d4282000 { > > > > + compatible = "mrvl,mmp-intc-wakeupgen"; > > > > + reg = <0xd4282000 0x1000>; > > > > + mrvl,intc-nr-irqs = <64>; > > > > + mrvl,intc-gbl-mask = <0x114 0x3 > > > > + 0x144 0x3>; > > > > Are these multiple entries? The binding text implied there was only one entry. > > What is this? > > This specify the register offset and value to mask for each core. Why can the driver not have these offsets hard-coded? How variable are they? [...] > > > > + size /= sizeof(*wakeup_reg); > > > > + while (i < size) { > > > > + unsigned offset, val; > > > > + > > > > + offset = be32_to_cpup(wakeup_reg + i++); > > > > + val = be32_to_cpup(wakeup_reg + i++); > > > > Use of_property_read_u32_index. You don't need to deal with the raw dtb. > > It's offset / value pair, it there convenient way to get such kind of key / value pair? > Thanks. Unfortunately there's no of_property_read_u32_array_index, but it's perfectly possible to do something similar to what you have here, without relying on the raw binary DTB: for (;;) { u32 offset, val; if (of_property_read_u32_index(node, PROP_NAME, i++, &offset) != 0) break; if (of_property_read_u32_index(node, PROP_NAME, i++, &val) != 0) break; do_something(offset, val); } > > > > > > > + writel_relaxed(val, mmp_icu_base + offset); > > > > + } > > > > + > > > > + /* Get the irq lines and ignore irqs */ > > > > + cp_irq_reg = of_get_property(node, "mrvl,intc-for-cp", &size); > > > > + if (!cp_irq_reg) > > > > + return; > > > > + > > > > + irq_for_cp_nr = size / sizeof(*cp_irq_reg); > > > > + for (i = 0; i < irq_for_cp_nr; i++) { > > > > + irq_for_cp[i] = be32_to_cpup(cp_irq_reg + i); > > > > Use of_property_read_u32_index. > > Yes, it should be OK, thanks very much. Cheers, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
