Mark, Sorry for reply late. > -----Original Message----- > From: Mark Rutland [mailto:mark.rutland@xxxxxxx] > Sent: 2013年11月15日 20:50 > To: Neil Zhang > Cc: Haojian Zhuang; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > Thomas Gleixner > Subject: Re: [PATCH 2/2 v2] irqchip: mmp: add dt support for wakeup > > 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? No, we have to use GIC as an interrupt controller since they are SMP system. > > > > > > > > > > > - 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? Different SoCs will have different global mask registers, so it's not suitable to hard code in driver. > > > > > > > > > > > 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? > As the patch did, we need to configure it to cp when init and kernel will ignore to change them in runtime. > > > > > > > > > > - 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? As I said above, different SoCs will have different global mask registers, It's not suitable to hard code in driver. > > [...] > > > > > > + 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); > } > Thanks very much, I'll change to use this way. > > > > > > > > > > + 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. Best Regards, Neil Zhang ��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
