From: Shenwei Wang <Shenwei.Wang@freescale.com>
To: Thomas Gleixner <tglx@linutronix.de>
CC: "shawn.guo@linaro.org" <shawn.guo@linaro.org>,
        "jason@lakedaemon.net" <jason@lakedaemon.net>,
        "linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Huang Anson <Anson.Huang@freescale.com>
Subject: RE: [PATCH v8 1/2] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup
 sources
Thread-Topic: [PATCH v8 1/2] irqchip: imx-gpcv2: IMX GPCv2 driver for wakeup
 sources
Thread-Index: AQHQy9jL+RBhKH42jEGfmT8ssDAQNZ4ZjWsAgAHi6uCAAB7vAIAAC9Cw
Date: Mon, 24 Aug 2015 18:25:07 +0000
Message-ID: <BY1PR0301MB0839B8F390ABA092FB80362083620@BY1PR0301MB0839.namprd03.prod.outlook.com>
References: <1438378439-11569-1-git-send-email-shenwei.wang@freescale.com>
 <1438378439-11569-2-git-send-email-shenwei.wang@freescale.com>
 <alpine.DEB.2.11.1508231251040.3873@nanos>
 <CY1PR0301MB084328829F8128A8FBFA6A9283620@CY1PR0301MB0843.namprd03.prod.outlook.com>
 <alpine.DEB.2.11.1508241933150.3873@nanos>
In-Reply-To: <alpine.DEB.2.11.1508241933150.3873@nanos>
Accept-Language: zh-CN, en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Aug 2015 18:25:07.6210
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 710a03f5-10f6-4d38-9ff4-a80b81da590d
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLUPR03MB1361
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 2127
Lines: 57


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