On Monday, September 10, 2018 2:18:28 PM CEST Mika Westerberg wrote:
> On Mon, Sep 10, 2018 at 02:14:14PM +0200, Hans de Goede wrote:
> > Hi,
> >
> > On 10-09-18 13:51, Mika Westerberg wrote:
> > > On Sat, Sep 08, 2018 at 08:08:13PM +0200, Hans de Goede wrote:
> > > > lpss_iosf_enter_d3_state() checks if all hw-blocks using the DMA
> > > > controllers are in d3 before powering down the DMA controllers.
> > > >
> > > > But on devices, where the I2C bus connected to the PMIC is shared by
> > > > the PUNIT, the controller for that bus will never reach d3 since it has
> > > > an effectively empty _PS3 method. Instead it appears to automatically
> > > > power-down during S0i3 and we never see it as being in d3.
> > > >
> > > > This causes the DMA controllers to never be powered-down on these devices,
> > > > causing them to never reach S0i3. This commit uses the ACPI _SEM method
> > > > to detect if an I2C bus is shared with the PUNIT and if it is, it removes
> > > > it from the mask of devices which lpss_iosf_enter_d3_state() checks for.
> > > >
> > > > This fixes these devices never reaching any S0ix states.
> > > >
> > > > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> > > > ---
> > > > drivers/acpi/acpi_lpss.c | 22 ++++++++++++++++++++--
> > > > 1 file changed, 20 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
> > > > index 969bf8d515c0..83875305b1d4 100644
> > > > --- a/drivers/acpi/acpi_lpss.c
> > > > +++ b/drivers/acpi/acpi_lpss.c
> > > > @@ -99,6 +99,9 @@ struct lpss_private_data {
> > > > u32 prv_reg_ctx[LPSS_PRV_REG_COUNT];
> > > > };
> > > > +/* Devices which need to be in D3 before lpss_iosf_enter_d3_state() proceeds */
> > > > +static u32 pmc_atom_d3_mask = 0xfe000ffe;
> > > > +
> > > > /* LPSS run time quirks */
> > > > static unsigned int lpss_quirks;
> > > > @@ -175,6 +178,21 @@ static void byt_pwm_setup(struct lpss_private_data *pdata)
> > > > static void byt_i2c_setup(struct lpss_private_data *pdata)
> > > > {
> > > > + const char *uid_str = acpi_device_uid(pdata->adev);
> > > > + acpi_handle handle = pdata->adev->handle;
> > > > + unsigned long long shared_host = 0;
> > > > + acpi_status status;
> > > > + long uid = 0;
> > >
> > > Hmm, I wonder if uid=0 is actually valid? IIRC they start from 0 and go
> > > forward from that.
> >
> > You are right some devices have 0 based UIDs but not the I2C controllers
> > on BYT/CHT those have 1 based UIDs.
> >
> > > > +
> > > > + /* Expected to always be true, but better safe then sorry */
> > > > + if (uid_str)
> > > > + uid = simple_strtol(uid_str, NULL, 10);
> > > > +
> > > > + /* Detect I2C bus shared with PUNIT and ignore its d3 status */
> > > > + status = acpi_evaluate_integer(handle, "_SEM", NULL, &shared_host);
> > > > + if (ACPI_SUCCESS(status) && shared_host && uid)
> > > > + pmc_atom_d3_mask &= ~(BIT_LPSS2_F1_I2C1 << (uid - 1));
> >
> > Hence also the - 1 here.
>
> Ah, missed that :) Thanks for pointing it out.
>
> Acked-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
>
Patch applied, thanks!
