Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1211579 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v8 1/2] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup  sources Thomas Gleixner <tglx@linutronix.de> - 2015-08-23 13:00 +0200
    RE: [PATCH v8 1/2] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup  sources Shenwei Wang <Shenwei.Wang@freescale.com> - 2015-08-24 18:10 +0200
      RE: [PATCH v8 1/2] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup  sources Thomas Gleixner <tglx@linutronix.de> - 2015-08-24 19:40 +0200
        RE: [PATCH v8 1/2] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup  sources Shenwei Wang <Shenwei.Wang@freescale.com> - 2015-08-24 20:30 +0200
          RE: [PATCH v8 1/2] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup  sources Thomas Gleixner <tglx@linutronix.de> - 2015-08-24 20:40 +0200

#1211579 — 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_spinlock    rlock;
      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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1212328

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogVGhvbWFzIEdsZWl4bmVy
IFttYWlsdG86dGdseEBsaW51dHJvbml4LmRlXQ0KPiBTZW50OiAyMDE1xOo41MIyM8jVIDU6NTgN
Cj4gVG86IFdhbmcgU2hlbndlaS1CMzgzMzkNCj4gQ2M6IHNoYXduLmd1b0BsaW5hcm8ub3JnOyBq
YXNvbkBsYWtlZGFlbW9uLm5ldDsNCj4gbGludXgtYXJtLWtlcm5lbEBsaXN0cy5pbmZyYWRlYWQu
b3JnOyBsaW51eC1rZXJuZWxAdmdlci5rZXJuZWwub3JnOyBIdWFuZw0KPiBZb25nY2FpLUIyMDc4
OA0KPiBTdWJqZWN0OiBSZTogW1BBVENIIHY4IDEvMl0gaXJxY2hpcDogaW14LWdwY3YyOiBJTVgg
R1BDdjIgZHJpdmVyIGZvciB3YWtldXANCj4gc291cmNlcw0KPiANCj4gT24gRnJpLCAzMSBKdWwg
MjAxNSwgU2hlbndlaSBXYW5nIHdyb3RlOg0KPiA+ICtzdHJ1Y3QgZ3BjdjJfaXJxY2hpcF9kYXRh
IHsNCj4gPiArCXN0cnVjdCByYXdfc3BpbmxvY2sgcmxvY2s7DQo+ID4gKwl2b2lkIF9faW9tZW0g
KmdwY19iYXNlOw0KPiA+ICsJdTMyIHdha2V1cF9zb3VyY2VzW0lNUl9OVU1dOw0KPiA+ICsJdTMy
IGVuYWJsZWRfaXJxc1tJTVJfTlVNXTsNCj4gPiArCXUzMiBjcHUyd2FrZXVwOw0KPiANCj4gQ2Fu
IHlvdSBwbGVhc2UgZm9ybWF0IHRoYXQgaW4gYSByZWFkYWJsZSB3YXk/DQo+IA0KPiAgICAgICBz
dHJ1Y3QgcmF3X3NwaW5sb2NrICAgIHJsb2NrOw0KPiAgICAgICB2b2lkIF9faW9tZW0JICAgICAq
Z3BjX2Jhc2U7DQo+ICAgICAgIC4uLi4NCg0KSSBkaWQgdHJ5IHRvIGJlIGNhcmVmdWwgYWJvdXQg
dGhlIGZvcm1hdCwgYnV0IGRpZCBub3Qgbm90aWNlIHRoaXMgb25lLiBXaWxsIGNoYW5nZSBpdCBp
biB0aGUgbmV3IHZlcnNpb24uOikNCiANCj4gPiArfTsNCj4gPiArDQo+ID4gK3N0YXRpYyBzdHJ1
Y3QgZ3BjdjJfaXJxY2hpcF9kYXRhICppbXhfZ3BjdjJfaW5zdGFuY2U7DQo+ID4gKw0KPiA+ICt1
MzIgaW14X2dwY3YyX2dldF93YWtldXBfc291cmNlKHUzMiAqKnNvdXJjZXMpIHsNCj4gPiArCWlm
ICghaW14X2dwY3YyX2luc3RhbmNlKQ0KPiA+ICsJCXJldHVybiAwOw0KPiA+ICsNCj4gPiArCWlm
IChzb3VyY2VzKQ0KPiA+ICsJCSpzb3VyY2VzID0gaW14X2dwY3YyX2luc3RhbmNlLT53YWtldXBf
c291cmNlczsNCj4gPiArDQo+ID4gKwlyZXR1cm4gSU1SX05VTTsNCj4gPiArfQ0KPiA+ICsNCj4g
PiArc3RhdGljIGludCBncGN2Ml93YWtldXBfc291cmNlX3NhdmUodm9pZCkgew0KPiA+ICsJc3Ry
dWN0IGdwY3YyX2lycWNoaXBfZGF0YSAqY2Q7DQo+ID4gKwl2b2lkIF9faW9tZW0gKnJlZzsNCj4g
PiArCWludCBpOw0KPiA+ICsNCj4gPiArCWNkID0gaW14X2dwY3YyX2luc3RhbmNlOw0KPiA+ICsJ
aWYgKCFjZCkNCj4gPiArCQlyZXR1cm4gMDsNCj4gPiArDQo+ID4gKwlmb3IgKGkgPSAwOyBpIDwg
SU1SX05VTTsgaSsrKSB7DQo+ID4gKwkJcmVnID0gY2QtPmdwY19iYXNlICsgY2QtPmNwdTJ3YWtl
dXAgKyBpICogNDsNCj4gPiArCQljZC0+ZW5hYmxlZF9pcnFzW2ldID0gcmVhZGxfcmVsYXhlZChy
ZWcpOw0KPiANCj4gWW91IHJlYWQgdGhlIGZ1bGwgc3RhdGUgb2YgdGhlIHJlZ2lzdGVyIGFuZCBy
ZXN0b3JlIHRoZSBmdWxsIHN0YXRlLiBTbyB3aHkNCj4gZW5hYmxlZF9pcnFzPw0KDQpUaGVyZSBh
cmUgdHdvIHVzZXIgc2NlbmFyaW9zOiANCkluIENQVSBJZGxlIHN0YXRlLCB0aGUgc3lzdGVtIG5l
ZWQgdG8gYmUgd29rZSB1cCBieSBhbnkgZW5hYmxlZCBpcnFzLCBub3QganVzdCB0aGUgb25lcyB0
aGF0IG1hcmtlZCBhcyB3YWtldXAgc291cmNlcy4NCkluIFN1c3BlbmQgU3RhdGUsIHRoZXkgc3lz
dGVtIHdpbGwgb25seSBiZSB3b2tlIHVwIGJ5IHRoZSBvbmUgdGhhdCBtYXJrZWQgYXMgYSB3YWtl
dXAgc291cmNlLiANCkVuYWJsZWRfaXJxcyBhcmUgdXNlZCB0byBzYXZlIHRoZSB2YWx1ZXMgYmVm
b3JlIHN1c3BlbmQsIGFuZCByZXN0b3JlIHRoZW0gYWZ0ZXIgcmVzdW1lLg0KDQo+ID4gKwkJd3Jp
dGVsX3JlbGF4ZWQoY2QtPndha2V1cF9zb3VyY2VzW2ldLCByZWcpOw0KPiA+ICsJfQ0KPiA+ICsN
Cj4gPiArCXJldHVybiAwOw0KPiA+ICt9DQo+ID4gKw0KPiA+ICtzdGF0aWMgdm9pZCBncGN2Ml93
YWtldXBfc291cmNlX3Jlc3RvcmUodm9pZCkgew0KPiA+ICsJc3RydWN0IGdwY3YyX2lycWNoaXBf
ZGF0YSAqY2Q7DQo+ID4gKwl2b2lkIF9faW9tZW0gKnJlZzsNCj4gPiArCWludCBpOw0KPiA+ICsN
Cj4gPiArCWNkID0gaW14X2dwY3YyX2luc3RhbmNlOw0KPiA+ICsJaWYgKCFjZCkNCj4gPiArCQly
ZXR1cm47DQo+ID4gKw0KPiA+ICsJZm9yIChpID0gMDsgaSA8IElNUl9OVU07IGkrKykgew0KPiA+
ICsJCXJlZyA9IGNkLT5ncGNfYmFzZSArIGNkLT5jcHUyd2FrZXVwICsgaSAqIDQ7DQo+ID4gKwkJ
d3JpdGVsX3JlbGF4ZWQoY2QtPmVuYWJsZWRfaXJxc1tpXSwgcmVnKTsNCj4gPiArCQljZC0+d2Fr
ZXVwX3NvdXJjZXNbaV0gPSB+MDsNCj4gDQo+IFdoeSBhcmUgeW91IGNsZWFyaW5nIHRoYXQgaW5m
byBvbiByZXN1bWU/IERyaXZlcnMgd2lsbCBjbGVhciB0aGF0IHZpYQ0KPiBzZXRfd2FrZSgpIG9y
IGxlYXZlIGl0IHdoZW4gdGhleSB3YW50IHRvIGhhdmUgcmVzdW1lIGZ1bmN0aW9uYWxpdHk/DQo+
IA0KRWFjaCB0aW1lIHN5c3RlbSBnb2VzIGludG8gdGhlIHN1c3BlbmQgc3RhdGUsIGl0IHdpbGwg
Y2FsbCBzZXRfd2FrZSAoT04pIGFnYWluIHRvIGNvbmZpZ3VyZQ0KdGhlIHdha2V1cCBzb3VyY2Vz
LiBDbGVhcmluZyB3YWtldXBfc291cmNlcyBoZXJlIGNhbiBtYWtlIHN1cmUgdGhlIHN5c3RlbSB3
b3JrIGFzDQpleHBlY3RlZCBubyBtYXR0ZXIgdGhhdCBhIGRyaXZlciBjYWxscyBzZXRfd2FrZSAo
T0ZGKSBkdXJpbmcgcmVzdW1lIHN0YWdlLg0KDQo+ID4gK3N0YXRpYyBpbnQgX19pbml0IGlteF9n
cGN2Ml9pcnFjaGlwX2luaXQoc3RydWN0IGRldmljZV9ub2RlICpub2RlLA0KPiA+ICsJCQkgICAg
ICAgc3RydWN0IGRldmljZV9ub2RlICpwYXJlbnQpIHsNCj4gPiArCXN0cnVjdCBpcnFfZG9tYWlu
ICpwYXJlbnRfZG9tYWluLCAqZG9tYWluOw0KPiA+ICsJc3RydWN0IGdwY3YyX2lycWNoaXBfZGF0
YSAqY2Q7DQo+ID4gKwlpbnQgaTsNCj4gPiArDQo+ID4gKwlpZiAoIXBhcmVudCkgew0KPiA+ICsJ
CXByX2VycigiJXM6IG5vIHBhcmVudCwgZ2l2aW5nIHVwXG4iLCBub2RlLT5mdWxsX25hbWUpOw0K
PiA+ICsJCXJldHVybiAtRU5PREVWOw0KPiA+ICsJfQ0KPiA+ICsNCj4gPiArCXBhcmVudF9kb21h
aW4gPSBpcnFfZmluZF9ob3N0KHBhcmVudCk7DQo+ID4gKwlpZiAoIXBhcmVudF9kb21haW4pIHsN
Cj4gPiArCQlwcl9lcnIoIiVzOiB1bmFibGUgdG8gZ2V0IHBhcmVudCBkb21haW5cbiIsIG5vZGUt
PmZ1bGxfbmFtZSk7DQo+ID4gKwkJcmV0dXJuIC1FTlhJTzsNCj4gPiArCX0NCj4gPiArDQo+ID4g
KwljZCA9IGt6YWxsb2Moc2l6ZW9mKHN0cnVjdCBncGN2Ml9pcnFjaGlwX2RhdGEpLCBHRlBfS0VS
TkVMKTsNCj4gPiArCUJVR19PTighY2QpOw0KPiANCj4gWW91IHJldHVybiBhbiBlcnJvciBjb2Rl
IGZvciBhbGwgb3RoZXIgZmFpbHVyZXMuIFdoeSBCVUcgaGVyZT8NCg0KR29vZCBwb2ludC4gVG8g
YmUgY29uc2lzdGVudCwgSSB3aWxsIGNoYW5nZSBpdCB0byByZXR1cm4gYW4gZXJyb3IgY29kZS4N
Cg0KVGhhbmtzLA0KU2hlbndlaQ0KPiANCj4gT3RoZXJ3aXNlIHRoaXMgbG9va3MgdmVyeSBjbGVh
biBub3cuIENhbiB5b3UgcGxlYXNlIHJlc2VuZCBBU0FQIHdpdGggdGhlc2UNCj4gbWlub3IgcG9p
bnRzIGFkZHJlc3NlZD8NCj4gDQo+IFRoYW5rcywNCj4gDQo+IAl0Z2x4DQo+IA0KDQo=
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1212376

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1212397

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1212404

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web