Re: [PATCHv2 2/4] ACPI / LPSS: custom power domain for LPSS

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

Hi Rafael,

On Tue, May 20, 2014 at 11:33:09PM +0200, Rafael J. Wysocki wrote:
> > +#ifdef CONFIG_PM
> 
> It would be good to add a kerneldoc explaining what's being saved here and why.

OK.

> > +static void acpi_lpss_save_ctx(struct device *dev)
> > +{
> > +	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
> > +	int i;
> > +
> > +	for (i = 0; i < LPSS_PRV_REG_COUNT; i++) {
> > +		pdata->prv_reg_ctx[i] = __lpss_reg_read(pdata, i * sizeof(u32));
> > +		dev_dbg(dev, "saving 0x%08x from LPSS reg at offset 0x%02x\n",
> > +			pdata->prv_reg_ctx[i], (unsigned int)(i * sizeof(u32)));
> > +	}
> > +}
> > +
> > +static void acpi_lpss_restore_ctx(struct device *dev)
> > +{
> > +	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
> > +	int i;
> > +
> > +	/* PCI spec expects 10ms delay when resuming from D3 to D0 */
> > +	msleep(10);
> 
> Two questions here.
> 
> First, is the 10 ms sleep really necessary?  I'd expect the AML to take care of
> such delays (this is not a PCI device formally).

Unfortunately that is not the case. There is nothing in the AML for
this. Mika, correct me if I'm wrong.

> And because this is not a PCI device formally, why is the comment talking about
> the PCI spec?  Why is PCI relevant in any way here?

Under the hood the devices are still PCI devices, even if they
formally aren't. Maybe I should point that out in the comment..

We put the sleep there because without it there was no guarantee if
the device was properly resumed by the time the drivers resume hooks
were called. The symptom in case of a failure was simply that the
registers could not be written, which leads into timeouts at least in
case of the I2C and UART and making them unusable until the next
suspend followed by resume.


Thanks,

-- 
heikki