RE: [PATCH v8 1/2] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources
On Mon, 24 Aug 2015, Shenwei Wang wrote: > > That's what you want achieve. Still you save the full content of the > > registers and > > restore the full content. That saves/restores the enabled and disabled > > interrupts. > > So enabled_irqs is a misnomer as you save the full state. > > How about change its name to "saved_irq_mask"? Way better. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v8 1/2] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources
> -Original Message-
> From: Thomas Gleixner [mailto:t...@linutronix.de]
> > > > +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->enabled_irqs[i] = readl_relaxed(reg);
> > >
> > > You read the full state of the register and restore the full state.
> > > So why enabled_irqs?
> >
> > There are two user scenarios:
> > In CPU Idle state, the system need to be woke up by any enabled irqs,
> > not just the ones that marked as wakeup sources.
> > In Suspend State, they system will only be woke up by the one that
> > marked as a wakeup source. Enabled_irqs are used to save the values
> > before suspend, and restore them after resume.
>
> That's what you want achieve. Still you save the full content of the
> registers and
> restore the full content. That saves/restores the enabled and disabled
> interrupts.
> So enabled_irqs is a misnomer as you save the full state.
How about change its name to "saved_irq_mask"?
> > > set_wake() or leave it when they want to have resume functionality?
> > >
> > Each time system goes into the suspend state, it will call set_wake
> > (ON) again to configure the wakeup sources. Clearing wakeup_sources
> > here can make sure the system work as expected no matter that a driver
> > calls set_wake (OFF) during resume stage.
>
> We rather make sure that the drivers call set_wake(OFF) as they are supposed
> to,
> because if they do not then the set_wake(ON) logic in the core code will see
> the
> counter != 0 and not invoke the irq callback.
Sounds reasonable. Then I will remove this line in new patch.
Thanks,
Shenwei
> Thanks,
>
> tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v8 1/2] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources
On Mon, 24 Aug 2015, Shenwei Wang wrote:
> > > +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->enabled_irqs[i] = readl_relaxed(reg);
> >
> > You read the full state of the register and restore the full state. So why
> > enabled_irqs?
>
> There are two user scenarios:
> In CPU Idle state, the system need to be woke up by any enabled
> irqs, not just the ones that marked as wakeup sources.
> In Suspend State, they system will only be woke up by the one that
> marked as a wakeup source. Enabled_irqs are used to save the values
> before suspend, and restore them after resume.
That's what you want achieve. Still you save the full content of the
registers and restore the full content. That saves/restores the
enabled and disabled interrupts. So enabled_irqs is a misnomer as you
save the full state.
> > > + 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->enabled_irqs[i], reg);
> > > + cd->wakeup_sources[i] = ~0;
> >
> > Why are you clearing that info on resume? Drivers will clear that via
> > set_wake() or leave it when they want to have resume functionality?
> >
> Each time system goes into the suspend state, it will call set_wake
> (ON) again to configure the wakeup sources. Clearing wakeup_sources
> here can make sure the system work as expected no matter that a
> driver calls set_wake (OFF) during resume stage.
We rather make sure that the drivers call set_wake(OFF) as they are
supposed to, because if they do not then the set_wake(ON) logic in the
core code will see the counter != 0 and not invoke the irq callback.
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v8 1/2] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources
> -Original Message-
> From: Thomas Gleixner [mailto:t...@linutronix.de]
> Sent: 2015年8月23日 5:58
> To: Wang Shenwei-B38339
> Cc: shawn@linaro.org; ja...@lakedaemon.net;
> linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org; Huang
> Yongcai-B20788
> Subject: Re: [PATCH v8 1/2] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup
> sources
>
> On Fri, 31 Jul 2015, Shenwei Wang wrote:
> > +struct gpcv2_irqchip_data {
> > + struct raw_spinlock rlock;
> > + void __iomem *gpc_base;
> > + u32 wakeup_sources[IMR_NUM];
> > + u32 enabled_irqs[IMR_NUM];
> > + u32 cpu2wakeup;
>
> Can you please format that in a readable way?
>
> struct raw_spinlockrlock;
> void __iomem *gpc_base;
>
I did try to be careful about the format, but did not notice this one. Will
change it in the new version.:)
> > +};
> > +
> > +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->enabled_irqs[i] = readl_relaxed(reg);
>
> You read the full state of the register and restore the full state. So why
> enabled_irqs?
There are two user scenarios:
In CPU Idle state, the system need to be woke up by any enabled irqs, not just
the ones that marked as wakeup sources.
In Suspend State, they system will only be woke up by the one that marked as a
wakeup source.
Enabled_irqs are used to save the values before suspend, and restore them after
resume.
> > + 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->enabled_irqs[i], reg);
> > + cd->wakeup_sources[i] = ~0;
>
> Why are you clearing that info on resume? Drivers will clear that via
> set_wake() or leave it when they want to have resume functionality?
>
Each time system goes into the suspend state, it will call set_wake (ON) again
to configure
the wakeup sources. Clearing wakeup_sources here can make sure the system work
as
expected no matter that a driver calls set_wake (OFF) during resume stage.
> > +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);
> > + BUG_ON(!cd);
>
> You return an error code for all other failures. Why BUG here?
Good point. To be consistent, I will change it to return an error code.
Thanks,
Shenwei
>
> Otherwise this looks very clean now. Can you please resend ASAP with these
> minor points addressed?
>
> Thanks,
>
> tglx
>
RE: [PATCH v8 1/2] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources
-Original Message-
From: Thomas Gleixner [mailto:t...@linutronix.de]
Sent: 2015年8月23日 5:58
To: Wang Shenwei-B38339
Cc: shawn@linaro.org; ja...@lakedaemon.net;
linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org; Huang
Yongcai-B20788
Subject: Re: [PATCH v8 1/2] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup
sources
On Fri, 31 Jul 2015, Shenwei Wang wrote:
+struct gpcv2_irqchip_data {
+ struct raw_spinlock rlock;
+ void __iomem *gpc_base;
+ u32 wakeup_sources[IMR_NUM];
+ u32 enabled_irqs[IMR_NUM];
+ u32 cpu2wakeup;
Can you please format that in a readable way?
struct raw_spinlockrlock;
void __iomem *gpc_base;
I did try to be careful about the format, but did not notice this one. Will
change it in the new version.:)
+};
+
+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-enabled_irqs[i] = readl_relaxed(reg);
You read the full state of the register and restore the full state. So why
enabled_irqs?
There are two user scenarios:
In CPU Idle state, the system need to be woke up by any enabled irqs, not just
the ones that marked as wakeup sources.
In Suspend State, they system will only be woke up by the one that marked as a
wakeup source.
Enabled_irqs are used to save the values before suspend, and restore them after
resume.
+ 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-enabled_irqs[i], reg);
+ cd-wakeup_sources[i] = ~0;
Why are you clearing that info on resume? Drivers will clear that via
set_wake() or leave it when they want to have resume functionality?
Each time system goes into the suspend state, it will call set_wake (ON) again
to configure
the wakeup sources. Clearing wakeup_sources here can make sure the system work
as
expected no matter that a driver calls set_wake (OFF) during resume stage.
+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);
+ BUG_ON(!cd);
You return an error code for all other failures. Why BUG here?
Good point. To be consistent, I will change it to return an error code.
Thanks,
Shenwei
Otherwise this looks very clean now. Can you please resend ASAP with these
minor points addressed?
Thanks,
tglx
RE: [PATCH v8 1/2] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources
On Mon, 24 Aug 2015, Shenwei Wang wrote:
+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-enabled_irqs[i] = readl_relaxed(reg);
You read the full state of the register and restore the full state. So why
enabled_irqs?
There are two user scenarios:
In CPU Idle state, the system need to be woke up by any enabled
irqs, not just the ones that marked as wakeup sources.
In Suspend State, they system will only be woke up by the one that
marked as a wakeup source. Enabled_irqs are used to save the values
before suspend, and restore them after resume.
That's what you want achieve. Still you save the full content of the
registers and restore the full content. That saves/restores the
enabled and disabled interrupts. So enabled_irqs is a misnomer as you
save the full state.
+ 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-enabled_irqs[i], reg);
+ cd-wakeup_sources[i] = ~0;
Why are you clearing that info on resume? Drivers will clear that via
set_wake() or leave it when they want to have resume functionality?
Each time system goes into the suspend state, it will call set_wake
(ON) again to configure the wakeup sources. Clearing wakeup_sources
here can make sure the system work as expected no matter that a
driver calls set_wake (OFF) during resume stage.
We rather make sure that the drivers call set_wake(OFF) as they are
supposed to, because if they do not then the set_wake(ON) logic in the
core code will see the counter != 0 and not invoke the irq callback.
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v8 1/2] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources
On Mon, 24 Aug 2015, Shenwei Wang wrote: That's what you want achieve. Still you save the full content of the registers and restore the full content. That saves/restores the enabled and disabled interrupts. So enabled_irqs is a misnomer as you save the full state. How about change its name to saved_irq_mask? Way better. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v8 1/2] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources
-Original Message-
From: Thomas Gleixner [mailto:t...@linutronix.de]
+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-enabled_irqs[i] = readl_relaxed(reg);
You read the full state of the register and restore the full state.
So why enabled_irqs?
There are two user scenarios:
In CPU Idle state, the system need to be woke up by any enabled irqs,
not just the ones that marked as wakeup sources.
In Suspend State, they system will only be woke up by the one that
marked as a wakeup source. Enabled_irqs are used to save the values
before suspend, and restore them after resume.
That's what you want achieve. Still you save the full content of the
registers and
restore the full content. That saves/restores the enabled and disabled
interrupts.
So enabled_irqs is a misnomer as you save the full state.
How about change its name to saved_irq_mask?
set_wake() or leave it when they want to have resume functionality?
Each time system goes into the suspend state, it will call set_wake
(ON) again to configure the wakeup sources. Clearing wakeup_sources
here can make sure the system work as expected no matter that a driver
calls set_wake (OFF) during resume stage.
We rather make sure that the drivers call set_wake(OFF) as they are supposed
to,
because if they do not then the set_wake(ON) logic in the core code will see
the
counter != 0 and not invoke the irq callback.
Sounds reasonable. Then I will remove this line in new patch.
Thanks,
Shenwei
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 1/2] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources
On Fri, 31 Jul 2015, Shenwei Wang wrote:
> +struct gpcv2_irqchip_data {
> + struct raw_spinlock rlock;
> + void __iomem *gpc_base;
> + u32 wakeup_sources[IMR_NUM];
> + u32 enabled_irqs[IMR_NUM];
> + u32 cpu2wakeup;
Can you please format that in a readable way?
struct raw_spinlockrlock;
void __iomem *gpc_base;
> +};
> +
> +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->enabled_irqs[i] = readl_relaxed(reg);
You read the full state of the register and restore the full state. So
why enabled_irqs?
> + 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->enabled_irqs[i], reg);
> + cd->wakeup_sources[i] = ~0;
Why are you clearing that info on resume? Drivers will clear that via
set_wake() or leave it when they want to have resume functionality?
> +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);
> + BUG_ON(!cd);
You return an error code for all other failures. Why BUG here?
Otherwise this looks very clean now. Can you please resend ASAP with
these minor points addressed?
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v8 1/2] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources
On Fri, 31 Jul 2015, Shenwei Wang wrote:
+struct gpcv2_irqchip_data {
+ struct raw_spinlock rlock;
+ void __iomem *gpc_base;
+ u32 wakeup_sources[IMR_NUM];
+ u32 enabled_irqs[IMR_NUM];
+ u32 cpu2wakeup;
Can you please format that in a readable way?
struct raw_spinlockrlock;
void __iomem *gpc_base;
+};
+
+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-enabled_irqs[i] = readl_relaxed(reg);
You read the full state of the register and restore the full state. So
why enabled_irqs?
+ 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-enabled_irqs[i], reg);
+ cd-wakeup_sources[i] = ~0;
Why are you clearing that info on resume? Drivers will clear that via
set_wake() or leave it when they want to have resume functionality?
+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);
+ BUG_ON(!cd);
You return an error code for all other failures. Why BUG here?
Otherwise this looks very clean now. Can you please resend ASAP with
these minor points addressed?
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v8 1/2] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources
Ping.
Shenwei
> -Original Message-
> From: linux-kernel-ow...@vger.kernel.org
> [mailto:linux-kernel-ow...@vger.kernel.org] On Behalf Of Shenwei Wang
> Sent: 2015年7月31日 16:34
> To: shawn@linaro.org; t...@linutronix.de; ja...@lakedaemon.net
> Cc: linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org; Huang
> Yongcai-B20788
> Subject: [PATCH v8 1/2] 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.
> This patch adds a new irqchip driver to manage the interrupt wakeup sources on
> IMX7D.
> 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
> Signed-off-by: Anson Huang
> ---
> drivers/irqchip/Kconfig | 7 ++
> drivers/irqchip/Makefile| 1 +
> drivers/irqchip/irq-imx-gpcv2.c | 273
>
> 3 files changed, 281 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 000..c2728b0
> --- /dev/null
> +++ b/drivers/irqchip/irq-imx-gpcv2.c
> @@ -0,0 +1,273 @@
> +/*
> + * 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
> +#include
> +#include
> +#include
> +#include
> +
> +#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 enabled_irqs[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->enabled_irqs[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->enabled_irqs[i], reg);
> + cd->wakeup_sources[i] = ~0;
> + }
> +}
> +
> +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(>rlock, flags);
> + reg = cd->gpc_base + cd->cpu2wakeup + idx * 4;
> + mask = 1 << d->hwirq % 32;
> + val =
RE: [PATCH v8 1/2] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup sources
Ping.
Shenwei
-Original Message-
From: linux-kernel-ow...@vger.kernel.org
[mailto:linux-kernel-ow...@vger.kernel.org] On Behalf Of Shenwei Wang
Sent: 2015年7月31日 16:34
To: shawn@linaro.org; t...@linutronix.de; ja...@lakedaemon.net
Cc: linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org; Huang
Yongcai-B20788
Subject: [PATCH v8 1/2] 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.
This patch adds a new irqchip driver to manage the interrupt wakeup sources on
IMX7D.
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 shenwei.w...@freescale.com
Signed-off-by: Anson Huang b20...@freescale.com
---
drivers/irqchip/Kconfig | 7 ++
drivers/irqchip/Makefile| 1 +
drivers/irqchip/irq-imx-gpcv2.c | 273
3 files changed, 281 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 000..c2728b0
--- /dev/null
+++ b/drivers/irqchip/irq-imx-gpcv2.c
@@ -0,0 +1,273 @@
+/*
+ * 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 enabled_irqs[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-enabled_irqs[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-enabled_irqs[i], reg);
+ cd-wakeup_sources[i] = ~0;
+ }
+}
+
+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];
+
+
