LinuxLists.cc - [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources

[permalink] [raw]

Subject: Re: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources

On Mon, 24 Aug 2015, Shenwei Wang wrote:
>
> Signed-off-by: Shenwei Wang <[email protected]>
> Signed-off-by: Anson Huang <[email protected]>

Ok, last question. How is Anson related to this? He did not convey
your patch and in the first posted versions his SOB was never there.

Thanks,

tglx

2015-08-24 19:35:08

[permalink] [raw]

Subject: RE: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources

> -----Original Message-----
> From: Thomas Gleixner [mailto:[email protected]]
> Sent: 2015??8??24?? 14:15
> To: Wang Shenwei-B38339
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; Huang
> Yongcai-B20788
> Subject: Re: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup
> sources
>
> On Mon, 24 Aug 2015, Shenwei Wang wrote:
> >
> > Signed-off-by: Shenwei Wang <[email protected]>
> > Signed-off-by: Anson Huang <[email protected]>
>
> Ok, last question. How is Anson related to this? He did not convey your patch and
> in the first posted versions his SOB was never there.

This patch was based on his internal version of GPCv2 driver. I reused some of his
implementation and ported it to latest linux kernel. Adding his name here to give
the credits to him.

Thanks,
Shenwei

> Thanks,
>
> tglx
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?

2015-08-24 19:30:54

[permalink] [raw]

Subject: RE: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources

On Mon, 24 Aug 2015, Shenwei Wang wrote:
> > Ok, last question. How is Anson related to this? He did not convey your patch and
> > in the first posted versions his SOB was never there.
>
> This patch was based on his internal version of GPCv2 driver. I reused some of his
> implementation and ported it to latest linux kernel. Adding his name here to give
> the credits to him.

So the proper way to express this is:

Based-on-a-patch-by: Anson
Signed-off-by: You

The way you did it suggests that you wrote it and Anson conveyed it,
which is not the case.

I'll change it that way then.

Thanks,

tglx

2015-08-24 19:32:58

by tip-bot for Vasyl Gomonovych

[permalink] [raw]

Subject: RE: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources

Thank you, Thomas!

Shenwei

> -----Original Message-----
> From: Thomas Gleixner [mailto:[email protected]]
> Sent: 2015??8??24?? 14:30
> To: Wang Shenwei-B38339
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; Huang
> Yongcai-B20788
> Subject: RE: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup
> sources
>
> On Mon, 24 Aug 2015, Shenwei Wang wrote:
> > > Ok, last question. How is Anson related to this? He did not convey
> > > your patch and in the first posted versions his SOB was never there.
> >
> > This patch was based on his internal version of GPCv2 driver. I reused
> > some of his implementation and ported it to latest linux kernel.
> > Adding his name here to give the credits to him.
>
> So the proper way to express this is:
>
> Based-on-a-patch-by: Anson
> Signed-off-by: You
>
> The way you did it suggests that you wrote it and Anson conveyed it, which is not
> the case.
>
> I'll change it that way then.
>
> Thanks,
>
> tglx

????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?

2015-08-24 19:52:42

[permalink] [raw]

Subject: [tip:irq/core] irqchip/imx-gpcv2: IMX GPCv2 driver for wakeup sources

Commit-ID: e324c4dc4a5991d5b1171f434884a4026345e4b4
Gitweb: http://git.kernel.org/tip/e324c4dc4a5991d5b1171f434884a4026345e4b4
Author: Shenwei Wang <[email protected]>
AuthorDate: Mon, 24 Aug 2015 14:04:15 -0500
Committer: Thomas Gleixner <[email protected]>
CommitDate: Mon, 24 Aug 2015 21:49:34 +0200

irqchip/imx-gpcv2: IMX GPCv2 driver for wakeup sources

IMX7D contains a new version of GPC IP block (GPCv2). It has two major
functions: power management and wakeup source management.

When the system is in WFI (wait for interrupt) mode, the GPC block
will be the first block on the platform to be activated and signaled.

In normal wait mode during cpu idle, the system can be woken up by any
enabled interrupts. In standby or suspend mode, the system can only be
wokem up by the pre-defined wakeup sources.

Based-on-patch-by: Anson Huang <[email protected]>
Signed-off-by: Shenwei Wang <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Thomas Gleixner <[email protected]>
---
drivers/irqchip/Kconfig | 6 +
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-imx-gpcv2.c | 278 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 285 insertions(+)

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 5b85371..27b52c8 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -181,3 +181,9 @@ config RENESAS_H8300H_INTC
config RENESAS_H8S_INTC
bool
select IRQ_DOMAIN
+
+config IMX_GPCV2
+ bool
+ select IRQ_DOMAIN
+ help
+ Enables the wakeup IRQs for IMX platforms with GPCv2 block
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index e5e0069..bb3048f 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -54,3 +54,4 @@ obj-$(CONFIG_RENESAS_H8300H_INTC) += irq-renesas-h8300h.o
obj-$(CONFIG_RENESAS_H8S_INTC) += irq-renesas-h8s.o
obj-$(CONFIG_ARCH_SA1100) += irq-sa11x0.o
obj-$(CONFIG_INGENIC_IRQ) += irq-ingenic.o
+obj-$(CONFIG_IMX_GPCV2) += irq-imx-gpcv2.o
diff --git a/drivers/irqchip/irq-imx-gpcv2.c b/drivers/irqchip/irq-imx-gpcv2.c
new file mode 100644
index 0000000..e48d330
--- /dev/null
+++ b/drivers/irqchip/irq-imx-gpcv2.c
@@ -0,0 +1,278 @@
+/*
+ * Copyright (C) 2015 Freescale Semiconductor, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/slab.h>
+#include <linux/irqchip.h>
+#include <linux/syscore_ops.h>
+
+#define IMR_NUM 4
+#define GPC_MAX_IRQS (IMR_NUM * 32)
+
+#define GPC_IMR1_CORE0 0x30
+#define GPC_IMR1_CORE1 0x40
+
+struct gpcv2_irqchip_data {
+ struct raw_spinlock rlock;
+ void __iomem *gpc_base;
+ u32 wakeup_sources[IMR_NUM];
+ u32 saved_irq_mask[IMR_NUM];
+ u32 cpu2wakeup;
+};
+
+static struct gpcv2_irqchip_data *imx_gpcv2_instance;
+
+/*
+ * Interface for the low level wakeup code.
+ */
+u32 imx_gpcv2_get_wakeup_source(u32 **sources)
+{
+ if (!imx_gpcv2_instance)
+ return 0;
+
+ if (sources)
+ *sources = imx_gpcv2_instance->wakeup_sources;
+
+ return IMR_NUM;
+}
+
+static int gpcv2_wakeup_source_save(void)
+{
+ struct gpcv2_irqchip_data *cd;
+ void __iomem *reg;
+ int i;
+
+ cd = imx_gpcv2_instance;
+ if (!cd)
+ return 0;
+
+ for (i = 0; i < IMR_NUM; i++) {
+ reg = cd->gpc_base + cd->cpu2wakeup + i * 4;
+ cd->saved_irq_mask[i] = readl_relaxed(reg);
+ writel_relaxed(cd->wakeup_sources[i], reg);
+ }
+
+ return 0;
+}
+
+static void gpcv2_wakeup_source_restore(void)
+{
+ struct gpcv2_irqchip_data *cd;
+ void __iomem *reg;
+ int i;
+
+ cd = imx_gpcv2_instance;
+ if (!cd)
+ return;
+
+ for (i = 0; i < IMR_NUM; i++) {
+ reg = cd->gpc_base + cd->cpu2wakeup + i * 4;
+ writel_relaxed(cd->saved_irq_mask[i], reg);
+ }
+}
+
+static struct syscore_ops imx_gpcv2_syscore_ops = {
+ .suspend = gpcv2_wakeup_source_save,
+ .resume = gpcv2_wakeup_source_restore,
+};
+
+static int imx_gpcv2_irq_set_wake(struct irq_data *d, unsigned int on)
+{
+ struct gpcv2_irqchip_data *cd = d->chip_data;
+ unsigned int idx = d->hwirq / 32;
+ unsigned long flags;
+ void __iomem *reg;
+ u32 mask, val;
+
+ raw_spin_lock_irqsave(&cd->rlock, flags);
+ reg = cd->gpc_base + cd->cpu2wakeup + idx * 4;
+ mask = 1 << d->hwirq % 32;
+ val = cd->wakeup_sources[idx];
+
+ cd->wakeup_sources[idx] = on ? (val & ~mask) : (val | mask);
+ raw_spin_unlock_irqrestore(&cd->rlock, flags);
+
+ /*
+ * Do *not* call into the parent, as the GIC doesn't have any
+ * wake-up facility...
+ */
+
+ return 0;
+}
+
+static void imx_gpcv2_irq_unmask(struct irq_data *d)
+{
+ struct gpcv2_irqchip_data *cd = d->chip_data;
+ void __iomem *reg;
+ u32 val;
+
+ raw_spin_lock(&cd->rlock);
+ reg = cd->gpc_base + cd->cpu2wakeup + d->hwirq / 32 * 4;
+ val = readl_relaxed(reg);
+ val &= ~(1 << d->hwirq % 32);
+ writel_relaxed(val, reg);
+ raw_spin_unlock(&cd->rlock);
+
+ irq_chip_unmask_parent(d);
+}
+
+static void imx_gpcv2_irq_mask(struct irq_data *d)
+{
+ struct gpcv2_irqchip_data *cd = d->chip_data;
+ void __iomem *reg;
+ u32 val;
+
+ raw_spin_lock(&cd->rlock);
+ reg = cd->gpc_base + cd->cpu2wakeup + d->hwirq / 32 * 4;
+ val = readl_relaxed(reg);
+ val |= 1 << (d->hwirq % 32);
+ writel_relaxed(val, reg);
+ raw_spin_unlock(&cd->rlock);
+
+ irq_chip_mask_parent(d);
+}
+
+static struct irq_chip gpcv2_irqchip_data_chip = {
+ .name = "GPCv2",
+ .irq_eoi = irq_chip_eoi_parent,
+ .irq_mask = imx_gpcv2_irq_mask,
+ .irq_unmask = imx_gpcv2_irq_unmask,
+ .irq_set_wake = imx_gpcv2_irq_set_wake,
+ .irq_retrigger = irq_chip_retrigger_hierarchy,
+#ifdef CONFIG_SMP
+ .irq_set_affinity = irq_chip_set_affinity_parent,
+#endif
+};
+
+static int imx_gpcv2_domain_xlate(struct irq_domain *domain,
+ struct device_node *controller,
+ const u32 *intspec,
+ unsigned int intsize,
+ unsigned long *out_hwirq,
+ unsigned int *out_type)
+{
+ /* Shouldn't happen, really... */
+ if (domain->of_node != controller)
+ return -EINVAL;
+
+ /* Not GIC compliant */
+ if (intsize != 3)
+ return -EINVAL;
+
+ /* No PPI should point to this domain */
+ if (intspec[0] != 0)
+ return -EINVAL;
+
+ *out_hwirq = intspec[1];
+ *out_type = intspec[2];
+ return 0;
+}
+
+static int imx_gpcv2_domain_alloc(struct irq_domain *domain,
+ unsigned int irq, unsigned int nr_irqs,
+ void *data)
+{
+ struct of_phandle_args *args = data;
+ struct of_phandle_args parent_args;
+ irq_hw_number_t hwirq;
+ int i;
+
+ /* Not GIC compliant */
+ if (args->args_count != 3)
+ return -EINVAL;
+
+ /* No PPI should point to this domain */
+ if (args->args[0] != 0)
+ return -EINVAL;
+
+ /* Can't deal with this */
+ hwirq = args->args[1];
+ if (hwirq >= GPC_MAX_IRQS)
+ return -EINVAL;
+
+ for (i = 0; i < nr_irqs; i++) {
+ irq_domain_set_hwirq_and_chip(domain, irq + i, hwirq + i,
+ &gpcv2_irqchip_data_chip, domain->host_data);
+ }
+
+ parent_args = *args;
+ parent_args.np = domain->parent->of_node;
+ return irq_domain_alloc_irqs_parent(domain, irq, nr_irqs, &parent_args);
+}
+
+static struct irq_domain_ops gpcv2_irqchip_data_domain_ops = {
+ .xlate = imx_gpcv2_domain_xlate,
+ .alloc = imx_gpcv2_domain_alloc,
+ .free = irq_domain_free_irqs_common,
+};
+
+static int __init imx_gpcv2_irqchip_init(struct device_node *node,
+ struct device_node *parent)
+{
+ struct irq_domain *parent_domain, *domain;
+ struct gpcv2_irqchip_data *cd;
+ int i;
+
+ if (!parent) {
+ pr_err("%s: no parent, giving up\n", node->full_name);
+ return -ENODEV;
+ }
+
+ parent_domain = irq_find_host(parent);
+ if (!parent_domain) {
+ pr_err("%s: unable to get parent domain\n", node->full_name);
+ return -ENXIO;
+ }
+
+ cd = kzalloc(sizeof(struct gpcv2_irqchip_data), GFP_KERNEL);
+ if (!cd) {
+ pr_err("kzalloc failed!\n");
+ return -ENOMEM;
+ }
+
+ cd->gpc_base = of_iomap(node, 0);
+ if (!cd->gpc_base) {
+ pr_err("fsl-gpcv2: unable to map gpc registers\n");
+ kfree(cd);
+ return -ENOMEM;
+ }
+
+ domain = irq_domain_add_hierarchy(parent_domain, 0, GPC_MAX_IRQS,
+ node, &gpcv2_irqchip_data_domain_ops, cd);
+ if (!domain) {
+ iounmap(cd->gpc_base);
+ kfree(cd);
+ return -ENOMEM;
+ }
+ irq_set_default_host(domain);
+
+ /* Initially mask all interrupts */
+ for (i = 0; i < IMR_NUM; i++) {
+ writel_relaxed(~0, cd->gpc_base + GPC_IMR1_CORE0 + i * 4);
+ writel_relaxed(~0, cd->gpc_base + GPC_IMR1_CORE1 + i * 4);
+ cd->wakeup_sources[i] = ~0;
+ }
+
+ /* Let CORE0 as the default CPU to wake up by GPC */
+ cd->cpu2wakeup = GPC_IMR1_CORE0;
+
+ /*
+ * Due to hardware design failure, need to make sure GPR
+ * interrupt(#32) is unmasked during RUN mode to avoid entering
+ * DSM by mistake.
+ */
+ writel_relaxed(~0x1, cd->gpc_base + cd->cpu2wakeup);
+
+ imx_gpcv2_instance = cd;
+ register_syscore_ops(&imx_gpcv2_syscore_ops);
+
+ return 0;
+}
+
+IRQCHIP_DECLARE(imx_gpcv2, "fsl,imx7d-gpc", imx_gpcv2_irqchip_init);

2015-08-25 09:24:41

[permalink] [raw]

Subject: Re: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources

On 24/08/15 20:04, Shenwei Wang wrote:
> IMX7D contains a new version of GPC IP block (GPCv2). It has two
> major functions: power management and wakeup source management.
> This patch adds a new irqchip driver to manage the interrupt wakeup
> sources on IMX7D.

Interesting, you mention that this IP block has mainly power management
and it itself requires save/restore. Is it not in always on domain ?

> When the system is in WFI (wait for interrupt) mode, this GPC block
> will be the first block on the platform to be activated and signaled.
> Under normal wait mode during cpu idle, the system can be woke up
> by any enabled interrupts. Under standby or suspend mode, the system
> can only be woke up by the pre-defined wakeup sources.
>
> Signed-off-by: Shenwei Wang <[email protected]>
> Signed-off-by: Anson Huang <[email protected]>
> ---
> Change log:
> Renamed the enabled_irqs to saved_irq_mask in struct gpcv2_irqchip_data
> Removed "BUG_ON()" in imx_gpcv2_irqchip_init to unify the error handling codes.
>
> drivers/irqchip/Kconfig | 7 +
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-imx-gpcv2.c | 275 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 283 insertions(+)
> create mode 100644 drivers/irqchip/irq-imx-gpcv2.c
>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 120d815..3fc0fac 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -177,3 +177,10 @@ config RENESAS_H8300H_INTC
> config RENESAS_H8S_INTC
> bool
> select IRQ_DOMAIN
> +
> +config IMX_GPCV2
> + bool
> + select IRQ_DOMAIN
> + help
> + Enables the wakeup IRQs for IMX platforms with GPCv2 block
> +
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index b8d4e96..8eb5f60 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -52,3 +52,4 @@ obj-$(CONFIG_RENESAS_H8300H_INTC) += irq-renesas-h8300h.o
> obj-$(CONFIG_RENESAS_H8S_INTC) += irq-renesas-h8s.o
> obj-$(CONFIG_ARCH_SA1100) += irq-sa11x0.o
> obj-$(CONFIG_INGENIC_IRQ) += irq-ingenic.o
> +obj-$(CONFIG_IMX_GPCV2) += irq-imx-gpcv2.o
> diff --git a/drivers/irqchip/irq-imx-gpcv2.c b/drivers/irqchip/irq-imx-gpcv2.c
> new file mode 100644
> index 0000000..4a97afa
> --- /dev/null
> +++ b/drivers/irqchip/irq-imx-gpcv2.c
> @@ -0,0 +1,275 @@
> +/*
> + * Copyright (C) 2015 Freescale Semiconductor, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/slab.h>
> +#include <linux/irqchip.h>
> +#include <linux/syscore_ops.h>
> +
> +#define IMR_NUM 4
> +#define GPC_MAX_IRQS (IMR_NUM * 32)
> +
> +#define GPC_IMR1_CORE0 0x30
> +#define GPC_IMR1_CORE1 0x40
> +
> +struct gpcv2_irqchip_data {
> + struct raw_spinlock rlock;
> + void __iomem *gpc_base;
> + u32 wakeup_sources[IMR_NUM];
> + u32 saved_irq_mask[IMR_NUM];
> + u32 cpu2wakeup;
> +};
> +
> +static struct gpcv2_irqchip_data *imx_gpcv2_instance;
> +
> +u32 imx_gpcv2_get_wakeup_source(u32 **sources)
> +{
> + if (!imx_gpcv2_instance)
> + return 0;
> +
> + if (sources)
> + *sources = imx_gpcv2_instance->wakeup_sources;
> +
> + return IMR_NUM;
> +}
> +
> +static int gpcv2_wakeup_source_save(void)
> +{
> + struct gpcv2_irqchip_data *cd;
> + void __iomem *reg;
> + int i;
> +
> + cd = imx_gpcv2_instance;
> + if (!cd)
> + return 0;
> +
> + for (i = 0; i < IMR_NUM; i++) {
> + reg = cd->gpc_base + cd->cpu2wakeup + i * 4;
> + cd->saved_irq_mask[i] = readl_relaxed(reg);
> + writel_relaxed(cd->wakeup_sources[i], reg);
> + }
> +
> + return 0;
> +}
> +
> +static void gpcv2_wakeup_source_restore(void)
> +{
> + struct gpcv2_irqchip_data *cd;
> + void __iomem *reg;
> + int i;
> +
> + cd = imx_gpcv2_instance;
> + if (!cd)
> + return;
> +
> + for (i = 0; i < IMR_NUM; i++) {
> + reg = cd->gpc_base + cd->cpu2wakeup + i * 4;
> + writel_relaxed(cd->saved_irq_mask[i], reg);

Instead of saving all the non-wakeup sources, can't you use raw
save/restore of these registers and mask all the non-wakeup sources
by setting MASK_ON_SUSPEND ?

Also your interrupt controller seems like has no special way to
configure wakeups, you are just leaving them enabled. i.e. I see
cpu2wakeup used for both {un,}masking and wakeup enable. So you can just
use IRQCHIP_SKIP_SET_WAKE. Correct me if my understanding is wrong.

Regards,
Sudeep

2015-08-25 13:38:33

[permalink] [raw]

Subject: RE: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources

> -----Original Message-----
> From: Sudeep Holla [mailto:[email protected]]
> Sent: 2015??8??25?? 4:25
> To: Wang Shenwei-B38339
> Cc: [email protected]; [email protected]; [email protected]; Sudeep
> Holla; Huang Yongcai-B20788; [email protected];
> [email protected]
> Subject: Re: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup
> sources
>
>
>
> On 24/08/15 20:04, Shenwei Wang wrote:
> > IMX7D contains a new version of GPC IP block (GPCv2). It has two major
> > functions: power management and wakeup source management.
> > This patch adds a new irqchip driver to manage the interrupt wakeup
> > sources on IMX7D.
>
> Interesting, you mention that this IP block has mainly power management and it
> itself requires save/restore. Is it not in always on domain ?

Yes, it is in always on domain.

> > When the system is in WFI (wait for interrupt) mode, this GPC block
> > will be the first block on the platform to be activated and signaled.
> > Under normal wait mode during cpu idle, the system can be woke up by
> > +static void gpcv2_wakeup_source_restore(void) {
> > + struct gpcv2_irqchip_data *cd;
> > + void __iomem *reg;
> > + int i;
> > +
> > + cd = imx_gpcv2_instance;
> > + if (!cd)
> > + return;
> > +
> > + for (i = 0; i < IMR_NUM; i++) {
> > + reg = cd->gpc_base + cd->cpu2wakeup + i * 4;
> > + writel_relaxed(cd->saved_irq_mask[i], reg);
>
> Instead of saving all the non-wakeup sources, can't you use raw save/restore of
> these registers and mask all the non-wakeup sources by setting
> MASK_ON_SUSPEND ?

I can't catch what you mean. Can you show me an example?

> Also your interrupt controller seems like has no special way to configure wakeups,
> you are just leaving them enabled. i.e. I see cpu2wakeup used for both
> {un,}masking and wakeup enable. So you can just use IRQCHIP_SKIP_SET_WAKE.
> Correct me if my understanding is wrong.

In this driver, the Core0 is configured to be the default core to be woke up, not both. You can
configure it to Core1 by changing the cpu2wakeup value. I don't see any reason to
use IRQCHIP_SKIP_SET_WAKE flag.

Thanks,
Shenwei

> Regards,
> Sudeep
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?

2015-08-25 13:54:26

[permalink] [raw]

Subject: Re: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources

On 25/08/15 14:38, Shenwei Wang wrote:
>
>
>> -----Original Message-----
>> From: Sudeep Holla [mailto:[email protected]]
>> Sent: 2015年8月25日 4:25
>> To: Wang Shenwei-B38339
>> Cc: [email protected]; [email protected]; [email protected]; Sudeep
>> Holla; Huang Yongcai-B20788; [email protected];
>> [email protected]
>> Subject: Re: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup
>> sources
>>
>>
>>
>> On 24/08/15 20:04, Shenwei Wang wrote:
>>> IMX7D contains a new version of GPC IP block (GPCv2). It has two major
>>> functions: power management and wakeup source management.
>>> This patch adds a new irqchip driver to manage the interrupt wakeup
>>> sources on IMX7D.
>>
>> Interesting, you mention that this IP block has mainly power management and it
>> itself requires save/restore. Is it not in always on domain ?
>
> Yes, it is in always on domain.
>

Hmm, then why do you need to save and restore the mask ?

>>> When the system is in WFI (wait for interrupt) mode, this GPC block
>>> will be the first block on the platform to be activated and signaled.
>>> Under normal wait mode during cpu idle, the system can be woke up by
>>> +static void gpcv2_wakeup_source_restore(void) {
>>> + struct gpcv2_irqchip_data *cd;
>>> + void __iomem *reg;
>>> + int i;
>>> +
>>> + cd = imx_gpcv2_instance;
>>> + if (!cd)
>>> + return;
>>> +
>>> + for (i = 0; i < IMR_NUM; i++) {
>>> + reg = cd->gpc_base + cd->cpu2wakeup + i * 4;
>>> + writel_relaxed(cd->saved_irq_mask[i], reg);
>>
>> Instead of saving all the non-wakeup sources, can't you use raw save/restore of
>> these registers and mask all the non-wakeup sources by setting
>> MASK_ON_SUSPEND ?
>
> I can't catch what you mean. Can you show me an example?
>

What I meant is instead of you tracking all the enabled irqs(both wakeup
and non-wakeup) in saved_irq_mask, just set MASK_ON_SUSPEND flag
where all the non-wakeup interrupts are masked on suspend and unmasked
on resume by the irq core. You can then just save and restore the wakeup
irq mask, but as I asked above do you have to do that as you are saying
it's in always on domain ?

>> Also your interrupt controller seems like has no special way to configure wakeups,
>> you are just leaving them enabled. i.e. I see cpu2wakeup used for both
>> {un,}masking and wakeup enable. So you can just use IRQCHIP_SKIP_SET_WAKE.
>> Correct me if my understanding is wrong.
>
> In this driver, the Core0 is configured to be the default core to be woke up, not both. You can
> configure it to Core1 by changing the cpu2wakeup value. I don't see any reason to
> use IRQCHIP_SKIP_SET_WAKE flag.
>

I don't see this driver doing anything extra apart from keeping the
wakeup irqs enabled. i.e. You use the same cpu*wake register to
mask/unmask the interrupt as well as set the wakeup source. Since
the wakeup interrupt will be enabled by the driver, you just need to
mark it as wake-up source and nothing extra in the controller right ?
If so, you need to set IRQCHIP_SKIP_SET_WAKE as you are just leaving
that irq enabled and not doing any extra configuration to enable it as
wakeup source. Please correct if that wrong, but from the code that's
what I could infer.

Regards,
Sudeep

2015-08-25 14:30:36

[permalink] [raw]

Subject: RE: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources

> -----Original Message-----
> From: Sudeep Holla [mailto:[email protected]]
> >>> IMX7D contains a new version of GPC IP block (GPCv2). It has two
> >>> major
> >>> functions: power management and wakeup source management.
> >>> This patch adds a new irqchip driver to manage the interrupt wakeup
> >>> sources on IMX7D.
> >>
> >> Interesting, you mention that this IP block has mainly power
> >> management and it itself requires save/restore. Is it not in always on
> domain ?
> >
> > Yes, it is in always on domain.
> >
>
> Hmm, then why do you need to save and restore the mask ?

The save/restore is used to handle the two different low power states: one is
WFI in cpu idle, and the other case is suspend. This has nothing to do with this
IP block itself.

> >>> When the system is in WFI (wait for interrupt) mode, this GPC block
> >>> will be the first block on the platform to be activated and signaled.
> >>> Under normal wait mode during cpu idle, the system can be woke up by
> >>> +static void gpcv2_wakeup_source_restore(void) {
> >>> + struct gpcv2_irqchip_data *cd;
> >>> + void __iomem *reg;
> >>> + int i;
> >>> +
> >>> + cd = imx_gpcv2_instance;
> >>> + if (!cd)
> >>> + return;
> >>> +
> >>> + for (i = 0; i < IMR_NUM; i++) {
> >>> + reg = cd->gpc_base + cd->cpu2wakeup + i * 4;
> >>> + writel_relaxed(cd->saved_irq_mask[i], reg);
> >>
> >> Instead of saving all the non-wakeup sources, can't you use raw
> >> save/restore of these registers and mask all the non-wakeup sources
> >> by setting MASK_ON_SUSPEND ?
> >
> > I can't catch what you mean. Can you show me an example?
> >
>
> What I meant is instead of you tracking all the enabled irqs(both wakeup and
> non-wakeup) in saved_irq_mask, just set MASK_ON_SUSPEND flag where all the
> non-wakeup interrupts are masked on suspend and unmasked on resume by the
> irq core. You can then just save and restore the wakeup irq mask, but as I asked
> above do you have to do that as you are saying it's in always on domain ?
>
> >> Also your interrupt controller seems like has no special way to
> >> configure wakeups, you are just leaving them enabled. i.e. I see
> >> cpu2wakeup used for both {un,}masking and wakeup enable. So you can just
> use IRQCHIP_SKIP_SET_WAKE.
> >> Correct me if my understanding is wrong.
> >
> > In this driver, the Core0 is configured to be the default core to be
> > woke up, not both. You can configure it to Core1 by changing the
> > cpu2wakeup value. I don't see any reason to use IRQCHIP_SKIP_SET_WAKE flag.
> >
>
> I don't see this driver doing anything extra apart from keeping the wakeup irqs
> enabled. i.e. You use the same cpu*wake register to mask/unmask the interrupt
> as well as set the wakeup source. Since the wakeup interrupt will be enabled by
> the driver, you just need to mark it as wake-up source and nothing extra in the
> controller right ?
> If so, you need to set IRQCHIP_SKIP_SET_WAKE as you are just leaving that irq
> enabled and not doing any extra configuration to enable it as wakeup source.
> Please correct if that wrong, but from the code that's what I could infer.

There is no special for this driver. We just use the IRQCHIP driver framework to
manage the wakeup sources. Why did you propose to set IRQCHIP_SKIP_SET_WAKE
flag here? If you don't need the wakeup feature, you should just not enable this
driver in the configuration.

Thanks,
Shenwei

> Regards,
> Sudeep
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?

2015-08-25 14:46:01

[permalink] [raw]

Subject: Re: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources

On 25/08/15 15:14, Shenwei Wang wrote:
>
>
>> -----Original Message-----
>> From: Sudeep Holla [mailto:[email protected]]

[...]

>> I don't see this driver doing anything extra apart from keeping the wakeup irqs
>> enabled. i.e. You use the same cpu*wake register to mask/unmask the interrupt
>> as well as set the wakeup source. Since the wakeup interrupt will be enabled by
>> the driver, you just need to mark it as wake-up source and nothing extra in the
>> controller right ?
>> If so, you need to set IRQCHIP_SKIP_SET_WAKE as you are just leaving that irq
>> enabled and not doing any extra configuration to enable it as wakeup source.
>> Please correct if that wrong, but from the code that's what I could infer.
>
> There is no special for this driver. We just use the IRQCHIP driver framework to
> manage the wakeup sources. Why did you propose to set IRQCHIP_SKIP_SET_WAKE
> flag here? If you don't need the wakeup feature, you should just not enable this
> driver in the configuration.
>

No, if the driver doesn't nothing extra to configure the wake up source
other than keeping it enabled, then it fits the case of SKIP_SET_WAKE.
The driver using this wake would have requested and enabled the irq.
When it calls enable_irq_wake, you have nothing extra to set(atleast
from the looks of the driver), so setting SKIP_SET_WAKE will skip the
call and updated the wake flags in irq core.

I don't see the real need of 2 separate sets of irq mask being saved in
either case.

Regards,
Sudeep

2015-08-25 14:54:50

[permalink] [raw]

Subject: RE: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources

> -----Original Message-----
> From: Sudeep Holla [mailto:[email protected]]
> Sent: 2015年8月25日 9:46
> To: Wang Shenwei-B38339
> Cc: Sudeep Holla; [email protected]; [email protected];
> [email protected]; Huang Yongcai-B20788; [email protected];
> [email protected]
> Subject: Re: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup
> sources
>
>
>
> On 25/08/15 15:14, Shenwei Wang wrote:
> >
> >
> >> -----Original Message-----
> >> From: Sudeep Holla [mailto:[email protected]]
>
> [...]
>
> >> I don't see this driver doing anything extra apart from keeping the
> >> wakeup irqs enabled. i.e. You use the same cpu*wake register to
> >> mask/unmask the interrupt as well as set the wakeup source. Since the
> >> wakeup interrupt will be enabled by the driver, you just need to mark
> >> it as wake-up source and nothing extra in the controller right ?
> >> If so, you need to set IRQCHIP_SKIP_SET_WAKE as you are just leaving
> >> that irq enabled and not doing any extra configuration to enable it as wakeup
> source.
> >> Please correct if that wrong, but from the code that's what I could infer.
> >
> > There is no special for this driver. We just use the IRQCHIP driver
> > framework to manage the wakeup sources. Why did you propose to set
> > IRQCHIP_SKIP_SET_WAKE flag here? If you don't need the wakeup feature,
> > you should just not enable this driver in the configuration.
> >
>
> No, if the driver doesn't nothing extra to configure the wake up source other than
> keeping it enabled, then it fits the case of SKIP_SET_WAKE.
> The driver using this wake would have requested and enabled the irq.
> When it calls enable_irq_wake, you have nothing extra to set(atleast from the
> looks of the driver), so setting SKIP_SET_WAKE will skip the call and updated the
> wake flags in irq core.
>
> I don't see the real need of 2 separate sets of irq mask being saved in either case.

You don't really understand what happens after a driver calls enable_irq_wake. In suspend state, even the interrupt
controller itself is powered off. How can you get the system up again by just using a SKIP_SET_WAKE.

Regards,
Shenwei

> Regards,
> Sudeep
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?

2015-08-25 16:24:32

[permalink] [raw]

Subject: Re: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources

On 25/08/15 15:54, Shenwei Wang wrote:
>
>
>> -----Original Message-----
>> From: Sudeep Holla [mailto:[email protected]]
>> Sent: 2015年8月25日 9:46
>> To: Wang Shenwei-B38339
>> Cc: Sudeep Holla; [email protected]; [email protected];
>> [email protected]; Huang Yongcai-B20788; [email protected];
>> [email protected]
>> Subject: Re: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup
>> sources
>>
>>
>>
>> On 25/08/15 15:14, Shenwei Wang wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Sudeep Holla [mailto:[email protected]]
>>
>> [...]
>>
>>>> I don't see this driver doing anything extra apart from keeping
>>>> the wakeup irqs enabled. i.e. You use the same cpu*wake
>>>> register to mask/unmask the interrupt as well as set the wakeup
>>>> source. Since the wakeup interrupt will be enabled by the
>>>> driver, you just need to mark it as wake-up source and nothing
>>>> extra in the controller right ? If so, you need to set
>>>> IRQCHIP_SKIP_SET_WAKE as you are just leaving that irq enabled
>>>> and not doing any extra configuration to enable it as wakeup source.
>>>> Please correct if that wrong, but from the code that's what I
>>>> couldinfer.
>>>
>>> There is no special for this driver. We just use the IRQCHIP
>>> driver framework to manage the wakeup sources. Why did you
>>> propose to set IRQCHIP_SKIP_SET_WAKE flag here? If you don't need
>>> the wakeup feature, you should just not enable this driver in the
>>> configuration.
>>>
>>
>> No, if the driver doesn't nothing extra to configure the wake up
>> source other than keeping it enabled, then it fits the case of
>> SKIP_SET_WAKE. The driver using this wake would have requested
>> and enabled the irq. When it calls enable_irq_wake, you have
>> nothing extra to set(atleast from the looks of the driver), so
>> setting SKIP_SET_WAKE will skip the call and updated the wake
>> flags in irq core.
>>
>> I don't see the real need of 2 separate sets of irq mask being
>> saved in either case.
>
> You don't really understand what happens after a driver calls
> enable_irq_wake. In suspend state, even the interrupt
> controller itself is powered off. How can you get the system up
> again by just using a SKIP_SET_WAKE.
>

Sorry for that, let me try to understand aloud. So you have
irq_{un,}mask function that are called when interrupts are enabled and
disabled. So suppose you have 3 irqs that are enabled and only one of
then is set as wakeup source.

Now you call enable_irq_wake, you save that in wakeup_sources, fine.
Later when you enter suspend, you save all the 3 active irqs in
saved_irq_mask and over-write cpu2wakeup with wakeup_sources, right?

All fine, what I am saying is let irq-core know that you want to mask
the 2 non-wakeup irqs you have using MASK_ON_SUSPEND. So when
suspend_device_irqs is called in suspend path, that's done for you
automatically and the cpu2wakeup will have just 1 wakeup enabled which
is what you are doing in suspend callback, right ?

Now that it's already done for you, you need not do anything extra and
hence just set SKIP_SET_WAKE to do nothing.

Hope this clarifies, sorry if I am still missing to understand something
here, but I don't see anything. Let me know.

Regards,
Sudeep

2015-08-25 19:24:33

[permalink] [raw]

Subject: RE: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources

> -----Original Message-----
> From: Sudeep Holla [mailto:[email protected]]
> Sent: 2015年8月25日 11:24
> To: Wang Shenwei-B38339
> Cc: Sudeep Holla; [email protected]; [email protected];
> [email protected]; Huang Yongcai-B20788; [email protected];
> [email protected]
> Subject: Re: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup
> >
> > You don't really understand what happens after a driver calls
> > enable_irq_wake. In suspend state, even the interrupt controller
> > itself is powered off. How can you get the system up again by just
> > using a SKIP_SET_WAKE.
> >
>
> Sorry for that, let me try to understand aloud. So you have irq_{un,}mask function
> that are called when interrupts are enabled and disabled. So suppose you have 3
> irqs that are enabled and only one of then is set as wakeup source.
>
> Now you call enable_irq_wake, you save that in wakeup_sources, fine.
> Later when you enter suspend, you save all the 3 active irqs in saved_irq_mask
> and over-write cpu2wakeup with wakeup_sources, right?
>
> All fine, what I am saying is let irq-core know that you want to mask the 2
> non-wakeup irqs you have using MASK_ON_SUSPEND. So when
> suspend_device_irqs is called in suspend path, that's done for you automatically
> and the cpu2wakeup will have just 1 wakeup enabled which is what you are doing
> in suspend callback, right ?

/*
* Hardware which has no wakeup source configuration facility
* requires that the non wakeup interrupts are masked at the
* chip level. The chip implementation indicates that with
* IRQCHIP_MASK_ON_SUSPEND.
*/
if (irq_desc_get_chip(desc)->flags & IRQCHIP_MASK_ON_SUSPEND)
mask_irq(desc);

IRQCHIP_MASK_ON_SUSPEND flag is for the hardware that has no wakeup source capability.
This GPCv2 block is designed to manage the wakeup source, so the flag does not make any sense.

> Now that it's already done for you, you need not do anything extra and hence just
> set SKIP_SET_WAKE to do nothing.

static int set_irq_wake_real(unsigned int irq, unsigned int on)
{
struct irq_desc *desc = irq_to_desc(irq);
int ret = -ENXIO;

if (irq_desc_get_chip(desc)->flags & IRQCHIP_SKIP_SET_WAKE)
return 0;

if (desc->irq_data.chip->irq_set_wake)
ret = desc->irq_data.chip->irq_set_wake(&desc->irq_data, on);

return ret;
}
>From the codes above, if a irqchip can not handle the wakeup sources, it can set SKIP flag.
This driver is intended to manage the wakeup sources, what's the reason to skip here?

Regards,
Shenwei

> Hope this clarifies, sorry if I am still missing to understand something here, but I
> don't see anything. Let me know.
>
> Regards,
> Sudeep
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?

2015-08-25 19:26:54

[permalink] [raw]

Subject: Re: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources

On Tue, 25 Aug 2015, Sudeep Holla wrote:
> On 25/08/15 15:54, Shenwei Wang wrote:
> > You don't really understand what happens after a driver calls
> > enable_irq_wake. In suspend state, even the interrupt
> > controller itself is powered off. How can you get the system up
> > again by just using a SKIP_SET_WAKE.
>
> Sorry for that, let me try to understand aloud. So you have
> irq_{un,}mask function that are called when interrupts are enabled and
> disabled. So suppose you have 3 irqs that are enabled and only one of
> then is set as wakeup source.
>
> Now you call enable_irq_wake, you save that in wakeup_sources, fine.
> Later when you enter suspend, you save all the 3 active irqs in
> saved_irq_mask and over-write cpu2wakeup with wakeup_sources, right?
>
> All fine, what I am saying is let irq-core know that you want to mask
> the 2 non-wakeup irqs you have using MASK_ON_SUSPEND. So when
> suspend_device_irqs is called in suspend path, that's done for you
> automatically and the cpu2wakeup will have just 1 wakeup enabled which
> is what you are doing in suspend callback, right ?

I missed that when I reviewed the patch. You are right, it can be
simplified.

> Now that it's already done for you, you need not do anything extra and
> hence just set SKIP_SET_WAKE to do nothing.

He still needs the set_wake function to capture the wake enabled
interrupts as they are handed over to the low level asm code via
imx_gpcv2_get_wakeup_source(). Though they could be read back from the
hw registers as well.

Thanks,

tglx

2015-08-25 19:30:23

[permalink] [raw]

Subject: RE: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources

On Tue, 25 Aug 2015, Shenwei Wang wrote:
> > From: Sudeep Holla [mailto:[email protected]]
> > All fine, what I am saying is let irq-core know that you want to mask the 2
> > non-wakeup irqs you have using MASK_ON_SUSPEND. So when
> > suspend_device_irqs is called in suspend path, that's done for you automatically
> > and the cpu2wakeup will have just 1 wakeup enabled which is what you are doing
> > in suspend callback, right ?
>
> /*
> * Hardware which has no wakeup source configuration facility
> * requires that the non wakeup interrupts are masked at the
> * chip level. The chip implementation indicates that with
> * IRQCHIP_MASK_ON_SUSPEND.
> */
> if (irq_desc_get_chip(desc)->flags & IRQCHIP_MASK_ON_SUSPEND)
> mask_irq(desc);
>
> IRQCHIP_MASK_ON_SUSPEND flag is for the hardware that has no wakeup
> source capability. This GPCv2 block is designed to manage the
> wakeup source, so the flag does not make any sense.

You have no seperate wakeup source mechanism. All you do is to mask all
non wakeup sources and keep the wakeup sources unmask.

That's what happens in gpcv2_wakeup_source_save()

writel_relaxed(cd->wakeup_sources[i], reg);

So it's the same as letting the core mask all non wakeup sources and
leave the wakeup sources unmask.

> > Now that it's already done for you, you need not do anything extra and hence just
> > set SKIP_SET_WAKE to do nothing.
>
> static int set_irq_wake_real(unsigned int irq, unsigned int on)
> {
> struct irq_desc *desc = irq_to_desc(irq);
> int ret = -ENXIO;
>
> if (irq_desc_get_chip(desc)->flags & IRQCHIP_SKIP_SET_WAKE)
> return 0;
>
> if (desc->irq_data.chip->irq_set_wake)
> ret = desc->irq_data.chip->irq_set_wake(&desc->irq_data, on);
>
> return ret;
> }
> From the codes above, if a irqchip can not handle the wakeup
> sources, it can set SKIP flag. This driver is intended to manage
> the wakeup sources, what's the reason to skip here?

To reduce code. Everything the core can do for you is something you
don't have to implement.

Thanks,

tglx

2015-08-25 19:58:54

[permalink] [raw]

Subject: RE: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources

> -----Original Message-----
> From: Thomas Gleixner [mailto:[email protected]]
> Sent: 2015??8??25?? 14:30
> To: Wang Shenwei-B38339
> Cc: Sudeep Holla; [email protected]; [email protected]; Huang
> Yongcai-B20788; [email protected];
> [email protected]
> Subject: RE: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup
> sources
>
> On Tue, 25 Aug 2015, Shenwei Wang wrote:
> > > From: Sudeep Holla [mailto:[email protected]] All fine, what I am
> > > saying is let irq-core know that you want to mask the 2 non-wakeup
> > > irqs you have using MASK_ON_SUSPEND. So when suspend_device_irqs is
> > > called in suspend path, that's done for you automatically and the
> > > cpu2wakeup will have just 1 wakeup enabled which is what you are
> > > doing in suspend callback, right ?
> >
> > /*
> > * Hardware which has no wakeup source configuration facility
> > * requires that the non wakeup interrupts are masked at the
> > * chip level. The chip implementation indicates that with
> > * IRQCHIP_MASK_ON_SUSPEND.
> > */
> > if (irq_desc_get_chip(desc)->flags & IRQCHIP_MASK_ON_SUSPEND)
> > mask_irq(desc);
> >
> > IRQCHIP_MASK_ON_SUSPEND flag is for the hardware that has no wakeup
> > source capability. This GPCv2 block is designed to manage the wakeup
> > source, so the flag does not make any sense.
>
> You have no seperate wakeup source mechanism. All you do is to mask all non
> wakeup sources and keep the wakeup sources unmask.
>
> That's what happens in gpcv2_wakeup_source_save()
>
> writel_relaxed(cd->wakeup_sources[i], reg);
>
> So it's the same as letting the core mask all non wakeup sources and leave the
> wakeup sources unmask.

Does it mean an unexpected interrupt may activate the system, and the core will let
the system go into suspend again if the core determines it not a wakeup source? The current design
is to ignore all the unexpected interrupts in the hardware level. Only the presetting
wakeup sources can activate the platform. Here power consumption is more
important.

> > > Now that it's already done for you, you need not do anything extra
> > > and hence just set SKIP_SET_WAKE to do nothing.
> >
> > static int set_irq_wake_real(unsigned int irq, unsigned int on) {
> > struct irq_desc *desc = irq_to_desc(irq);
> > int ret = -ENXIO;
> >
> > if (irq_desc_get_chip(desc)->flags & IRQCHIP_SKIP_SET_WAKE)
> > return 0;
> >
> > if (desc->irq_data.chip->irq_set_wake)
> > ret = desc->irq_data.chip->irq_set_wake(&desc->irq_data, on);
> >
> > return ret;
> > }
> > From the codes above, if a irqchip can not handle the wakeup sources,
> > it can set SKIP flag. This driver is intended to manage the wakeup
> > sources, what's the reason to skip here?
>
> To reduce code. Everything the core can do for you is something you don't have to
> implement.

Same as above.

Thanks,
Shenwei

> Thanks,
>
> tglx
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?

2015-08-25 20:16:32

[permalink] [raw]

Subject: RE: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources

On Tue, 25 Aug 2015, Shenwei Wang wrote:
> > From: Thomas Gleixner [mailto:[email protected]]
> > > IRQCHIP_MASK_ON_SUSPEND flag is for the hardware that has no wakeup
> > > source capability. This GPCv2 block is designed to manage the wakeup
> > > source, so the flag does not make any sense.
> >
> > You have no seperate wakeup source mechanism. All you do is to mask all non
> > wakeup sources and keep the wakeup sources unmask.
> >
> > That's what happens in gpcv2_wakeup_source_save()
> >
> > writel_relaxed(cd->wakeup_sources[i], reg);
> >
> > So it's the same as letting the core mask all non wakeup sources and leave the
> > wakeup sources unmask.
>
> Does it mean an unexpected interrupt may activate the system, and
> the core will let the system go into suspend again if the core
> determines it not a wakeup source? The current design is to ignore
> all the unexpected interrupts in the hardware level. Only the
> presetting wakeup sources can activate the platform. Here power
> consumption is more important.

Did you actually read, what I wrote?

The core does in case of MASK_ON_SUSPEND

for_each_irq() {
if (!irq->wakeupsource)
mask(irq)
}

That's identical to what you are doing. You just do it differently by
saving the active wakeup sources in your own data structure and then
write that info to the mask register, which leaves only the wakeup
sources unmasked.

Thanks,

tglx

2015-08-25 20:43:41

[permalink] [raw]

Subject: RE: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources

> -----Original Message-----
> From: Thomas Gleixner [mailto:[email protected]]
> Sent: 2015??8??25?? 15:16
> To: Wang Shenwei-B38339
> Cc: Sudeep Holla; [email protected]; [email protected]; Huang
> Yongcai-B20788; [email protected];
> [email protected]
> Subject: RE: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup
> sources
>
> On Tue, 25 Aug 2015, Shenwei Wang wrote:
> > > From: Thomas Gleixner [mailto:[email protected]]
> > > > IRQCHIP_MASK_ON_SUSPEND flag is for the hardware that has no
> > > > wakeup source capability. This GPCv2 block is designed to manage
> > > > the wakeup source, so the flag does not make any sense.
> > >
> > > You have no seperate wakeup source mechanism. All you do is to mask
> > > all non wakeup sources and keep the wakeup sources unmask.
> > >
> > > That's what happens in gpcv2_wakeup_source_save()
> > >
> > > writel_relaxed(cd->wakeup_sources[i], reg);
> > >
> > > So it's the same as letting the core mask all non wakeup sources and
> > > leave the wakeup sources unmask.
> >
> > Does it mean an unexpected interrupt may activate the system, and the
> > core will let the system go into suspend again if the core determines
> > it not a wakeup source? The current design is to ignore all the
> > unexpected interrupts in the hardware level. Only the presetting
> > wakeup sources can activate the platform. Here power consumption is
> > more important.
>
> Did you actually read, what I wrote?
>
> The core does in case of MASK_ON_SUSPEND
>
> for_each_irq() {
> if (!irq->wakeupsource)
> mask(irq)
> }
>
> That's identical to what you are doing. You just do it differently by saving the
> active wakeup sources in your own data structure and then write that info to the
> mask register, which leaves only the wakeup sources unmasked.

Sorry. I just took a study on the two flags: MASK_ON_SUSPEND and IRQCHIP_SKIP_SET_WAKE.
MASK_ON_SUSPEND flag does simply the implementation.
IRQCHIP_SKIP_SET_WAKE flag can't be used here because the wakeup sources are required for power management.
I will send out a subsequent patch to simply the implementation by using this idea.

Thank you Sudeep for the insightful review!

Thanks,
Shenwei

> Thanks,
>
> tglx
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?

2015-08-25 20:50:07

[permalink] [raw]

Subject: RE: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources

On Tue, 25 Aug 2015, Shenwei Wang wrote:
>
> IRQCHIP_SKIP_SET_WAKE flag can't be used here because the wakeup
> sources are required for power management.

You could use it. Instead of copying the saved values, you can read
the values back from the register.

Thanks,

tglx

2015-08-25 20:54:03

[permalink] [raw]

Subject: RE: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources

> -----Original Message-----
> From: Thomas Gleixner [mailto:[email protected]]
> Sent: 2015??8??25?? 15:50
> To: Wang Shenwei-B38339
> Cc: Sudeep Holla; [email protected]; [email protected]; Huang
> Yongcai-B20788; [email protected];
> [email protected]
> Subject: RE: [PATCH v9 1/1] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup
> sources
>
> On Tue, 25 Aug 2015, Shenwei Wang wrote:
> >
> > IRQCHIP_SKIP_SET_WAKE flag can't be used here because the wakeup
> > sources are required for power management.
>
> You could use it. Instead of copying the saved values, you can read the values
> back from the register.

It is an option too.

Thanks,
Shenwei

> Thanks,
>
> tglx
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?

2015-08-26 08:52:22