Re: [PATCH 2/2 v2] irqchip: mmp: add dt support for wakeup

Re: [PATCH 2/2 v2] irqchip: mmp: add dt support for wakeup

[Haojian Zhuang]

On Fri, Oct 11, 2013 at 4:23 PM, Neil Zhang <zhangwm-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> 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-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> ---
>  .../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"
>  - 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.

Loop devicetree guys.

> +- mrvl,intc-for-cp : Specifies the irqs that will be routed to cp
>  - 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>;
> +               mrvl,intc-for-cp = <0 31 32>;
> +     };
> +
>  * Marvell Orion Interrupt controller
>
>  Required properties
> diff --git a/drivers/irqchip/irq-mmp.c b/drivers/irqchip/irq-mmp.c
> index 470c5de..445a00c 100644
> --- a/drivers/irqchip/irq-mmp.c
> +++ b/drivers/irqchip/irq-mmp.c
> @@ -16,6 +16,8 @@
>  #include <linux/init.h>
>  #include <linux/irq.h>
>  #include <linux/irqdomain.h>
> +#include <linux/irqchip/mmp.h>
> +#include <linux/irqchip/arm-gic.h>
>  #include <linux/io.h>
>  #include <linux/ioport.h>
>  #include <linux/of_address.h>
> @@ -58,6 +60,8 @@ struct mmp_intc_conf {
>  static void __iomem *mmp_icu_base;
>  static struct icu_chip_data icu_data[MAX_ICU_NR];
>  static int max_icu_nr;
> +static u32 irq_for_cp[64];
> +static u32 irq_for_cp_nr;      /* How many irqs will be routed to cp */
>
>  extern void mmp2_clear_pmic_int(void);
>
> @@ -123,6 +127,50 @@ static void icu_unmask_irq(struct irq_data *d)
>         }
>  }
>
> +static int irq_ignore_wakeup(struct icu_chip_data *data, int hwirq)
> +{
> +       int i;
> +
> +       if (hwirq < 0 || hwirq >= data->nr_irqs)
> +               return 1;
> +
> +       for (i = 0; i < irq_for_cp_nr; i++)
> +               if (irq_for_cp[i] == hwirq)
> +                       return 1;
> +
> +       return 0;
> +}
> +
> +static void icu_mask_irq_wakeup(struct irq_data *d)
> +{
> +       struct icu_chip_data *data = &icu_data[0];
> +       int hwirq = d->hwirq - data->virq_base;
> +       u32 r;
> +
> +       if (irq_ignore_wakeup(data, hwirq))
> +               return;
> +
> +       r = readl_relaxed(mmp_icu_base + (hwirq << 2));
> +       r &= ~data->conf_mask;
> +       r |= data->conf_disable;
> +       writel_relaxed(r, mmp_icu_base + (hwirq << 2));
> +}
> +
> +static void icu_unmask_irq_wakeup(struct irq_data *d)
> +{
> +       struct icu_chip_data *data = &icu_data[0];
> +       int hwirq = d->irq - data->virq_base;
> +       u32 r;
> +
> +       if (irq_ignore_wakeup(data, hwirq))
> +               return;
> +
> +       r = readl_relaxed(mmp_icu_base + (hwirq << 2));
> +       r &= ~data->conf_mask;
> +       r |= data->conf_enable;
> +       writel_relaxed(r, mmp_icu_base + (hwirq << 2));
> +}
> +
>  struct irq_chip icu_irq_chip = {
>         .name           = "icu_irq",
>         .irq_mask       = icu_mask_irq,
> @@ -491,5 +539,81 @@ err:
>         irq_domain_remove(icu_data[i].domain);
>         return -EINVAL;
>  }
> +
> +void __init mmp_of_wakeup_init(void)
> +{
> +       struct device_node *node;
> +       const __be32 *wakeup_reg;
> +       const __be32 *cp_irq_reg;
> +       int ret, nr_irqs;
> +       int size, i = 0;
> +       int irq;
> +
> +       node = of_find_compatible_node(NULL, NULL, "mrvl,mmp-intc-wakeupgen");
> +       if (!node) {
> +               pr_err("Failed to find interrupt controller in arch-mmp\n");
> +               return;
> +       }
> +
> +       mmp_icu_base = of_iomap(node, 0);
> +       if (!mmp_icu_base) {
> +               pr_err("Failed to get interrupt controller register\n");
> +               return;
> +       }
> +
> +       ret = of_property_read_u32(node, "mrvl,intc-nr-irqs", &nr_irqs);
> +       if (ret) {
> +               pr_err("Not found mrvl,intc-nr-irqs property\n");
> +               return;
> +       }
> +
> +       /*
> +        * Config all the interrupt source be able to interrupt the cpu 0,
> +        * in IRQ mode, with priority 0 as masked by default.
> +        */
> +       for (irq = 0; irq < nr_irqs; irq++)
> +               __raw_writel(ICU_IRQ_CPU0_MASKED, mmp_icu_base + (irq << 2));
> +
> +       /* ICU is only used as wake up logic
> +         * disable the global irq/fiq in icu for all cores.
> +         */
> +       wakeup_reg = of_get_property(node, "mrvl,intc-gbl-mask", &size);
> +       if (!wakeup_reg) {
> +               pr_err("Not found mrvl,intc-gbl-mask property\n");
> +               return;
> +       }
> +
> +       size /= sizeof(*wakeup_reg);
> +       while (i < size) {
> +               unsigned offset, val;
> +
> +               offset = be32_to_cpup(wakeup_reg + i++);
> +               val = be32_to_cpup(wakeup_reg + i++);
> +               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);
> +               __raw_writel(ICU_CONF_SEAGULL, mmp_icu_base + (irq_for_cp[i] << 2));
> +       }
> +
> +       icu_data[0].conf_enable = mmp_conf.conf_enable;
> +       icu_data[0].conf_disable = mmp_conf.conf_disable;
> +       icu_data[0].conf_mask = mmp_conf.conf_mask;
> +       icu_data[0].nr_irqs = nr_irqs;
> +       icu_data[0].virq_base = 32;
> +
> +       gic_arch_extn.irq_mask = icu_mask_irq_wakeup;
> +       gic_arch_extn.irq_unmask = icu_unmask_irq_wakeup;
> +
> +       return;
> +}
> +
>  IRQCHIP_DECLARE(mmp2_mux_intc, "mrvl,mmp2-mux-intc", mmp2_mux_of_init);
>  #endif
> diff --git a/include/linux/irqchip/mmp.h b/include/linux/irqchip/mmp.h
> index c78a892..93b05ad 100644
> --- a/include/linux/irqchip/mmp.h
> +++ b/include/linux/irqchip/mmp.h
> @@ -1,6 +1,19 @@
>  #ifndef        __IRQCHIP_MMP_H
>  #define        __IRQCHIP_MMP_H
>
> +#define ICU_CONF_CPU3      (1 << 9)
> +#define ICU_CONF_CPU2      (1 << 8)
> +#define ICU_CONF_CPU1      (1 << 7)
> +#define ICU_CONF_CPU0      (1 << 6)
> +#define ICU_CONF_AP(n)     (1 << (6 + (n & 0x3)))
> +#define ICU_CONF_AP_MASK   (0xF << 6)
> +#define ICU_CONF_SEAGULL   (1 << 5)
> +#define ICU_CONF_IRQ_FIQ   (1 << 4)
> +#define ICU_CONF_PRIO(n)   (n & 0xF)
> +
> +#define ICU_IRQ_CPU0_MASKED    (ICU_CONF_IRQ_FIQ | ICU_CONF_CPU0)
> +
>  extern struct irq_chip icu_irq_chip;
>
> +extern void __init mmp_of_wakeup_init(void);
>  #endif /* __IRQCHIP_MMP_H */
> --
> 1.7.9.5
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2 v2] irqchip: mmp: add dt support for wakeup

[Mark Rutland]

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-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> 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-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> > ---
> >  .../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?

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

> 
> 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?

> >  - 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?

[...]

> > +void __init mmp_of_wakeup_init(void)
> > +{
> > +       struct device_node *node;
> > +       const __be32 *wakeup_reg;
> > +       const __be32 *cp_irq_reg;
> > +       int ret, nr_irqs;
> > +       int size, i = 0;
> > +       int irq;
> > +
> > +       node = of_find_compatible_node(NULL, NULL, "mrvl,mmp-intc-wakeupgen");
> > +       if (!node) {
> > +               pr_err("Failed to find interrupt controller in arch-mmp\n");
> > +               return;
> > +       }

Why can this not be done in the standard probe path, using the node
you'll have been handed by the core code?

> > +
> > +       mmp_icu_base = of_iomap(node, 0);
> > +       if (!mmp_icu_base) {
> > +               pr_err("Failed to get interrupt controller register\n");
> > +               return;
> > +       }
> > +
> > +       ret = of_property_read_u32(node, "mrvl,intc-nr-irqs", &nr_irqs);
> > +       if (ret) {
> > +               pr_err("Not found mrvl,intc-nr-irqs property\n");
> > +               return;
> > +       }
> > +
> > +       /*
> > +        * Config all the interrupt source be able to interrupt the cpu 0,
> > +        * in IRQ mode, with priority 0 as masked by default.
> > +        */
> > +       for (irq = 0; irq < nr_irqs; irq++)
> > +               __raw_writel(ICU_IRQ_CPU0_MASKED, mmp_icu_base + (irq << 2));
> > +
> > +       /* ICU is only used as wake up logic
> > +         * disable the global irq/fiq in icu for all cores.
> > +         */
> > +       wakeup_reg = of_get_property(node, "mrvl,intc-gbl-mask", &size);
> > +       if (!wakeup_reg) {
> > +               pr_err("Not found mrvl,intc-gbl-mask property\n");
> > +               return;
> > +       }
> > +
> > +       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.

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

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/2 v2] irqchip: mmp: add dt support for wakeup

[Neil Zhang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 7289 bytes --]


> -----Original Message-----
> From: Mark Rutland [mailto:mark.rutland@arm.com]
> Sent: 2013Äê11ÔÂ14ÈÕ 20:28
> To: Haojian Zhuang
> Cc: Neil Zhang; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> 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@marvell.com> 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@marvell.com>
> > > ---
> > >  .../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.

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

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

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

> 
> [...]
> 
> > > +void __init mmp_of_wakeup_init(void) {
> > > +       struct device_node *node;
> > > +       const __be32 *wakeup_reg;
> > > +       const __be32 *cp_irq_reg;
> > > +       int ret, nr_irqs;
> > > +       int size, i = 0;
> > > +       int irq;
> > > +
> > > +       node = of_find_compatible_node(NULL, NULL,
> "mrvl,mmp-intc-wakeupgen");
> > > +       if (!node) {
> > > +               pr_err("Failed to find interrupt controller in
> arch-mmp\n");
> > > +               return;
> > > +       }
> 
> Why can this not be done in the standard probe path, using the node you'll have
> been handed by the core code?

We don't use it as interrupt controller, so can't use IRQCHIP_DECLARE.

> 
> > > +
> > > +       mmp_icu_base = of_iomap(node, 0);
> > > +       if (!mmp_icu_base) {
> > > +               pr_err("Failed to get interrupt controller register\n");
> > > +               return;
> > > +       }
> > > +
> > > +       ret = of_property_read_u32(node, "mrvl,intc-nr-irqs", &nr_irqs);
> > > +       if (ret) {
> > > +               pr_err("Not found mrvl,intc-nr-irqs property\n");
> > > +               return;
> > > +       }
> > > +
> > > +       /*
> > > +        * Config all the interrupt source be able to interrupt the cpu 0,
> > > +        * in IRQ mode, with priority 0 as masked by default.
> > > +        */
> > > +       for (irq = 0; irq < nr_irqs; irq++)
> > > +               __raw_writel(ICU_IRQ_CPU0_MASKED, mmp_icu_base +
> > > + (irq << 2));
> > > +
> > > +       /* ICU is only used as wake up logic
> > > +         * disable the global irq/fiq in icu for all cores.
> > > +         */
> > > +       wakeup_reg = of_get_property(node, "mrvl,intc-gbl-mask", &size);
> > > +       if (!wakeup_reg) {
> > > +               pr_err("Not found mrvl,intc-gbl-mask property\n");
> > > +               return;
> > > +       }
> > > +
> > > +       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.

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

> 
> Thanks,
> Mark.

Best Regards,
Neil Zhang
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·zøœzÚÞz)í…æèw*\x1fjg¬±¨\x1e¶‰šŽŠÝ¢j.ïÛ°\½½MŽúgjÌæa×\x02››–' ™©Þ¢¸\f¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾\a«‘êçzZ+ƒùšŽŠÝ¢j"ú!¶i

Re: [PATCH 2/2 v2] irqchip: mmp: add dt support for wakeup

[Mark Rutland]

On Fri, Nov 15, 2013 at 11:49:20AM +0000, Neil Zhang wrote:
> 
> > -----Original Message-----
> > From: Mark Rutland [mailto:mark.rutland-5wv7dgnIgG8@public.gmane.org]
> > Sent: 2013年11月14日 20:28
> > To: Haojian Zhuang
> > Cc: Neil Zhang; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-kernel-u79uwXL29TZNg+MwTxZMZA@public.gmane.orgl.org;
> > 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-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> 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-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> > > > ---
> > > >  .../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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/2 v2] irqchip: mmp: add dt support for wakeup

[Neil Zhang]

Mark,

Sorry for reply late.

> -----Original Message-----
> From: Mark Rutland [mailto:mark.rutland@arm.com]
> Sent: 2013年11月15日 20:50
> To: Neil Zhang
> Cc: Haojian Zhuang; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> 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@arm.com]
> > > Sent: 2013年11月14日 20:28
> > > To: Haojian Zhuang
> > > Cc: Neil Zhang; devicetree@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; 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@marvell.com>
> 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@marvell.com>
> > > > > ---
> > > > >  .../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