| Message ID | 20180908180813.1225-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Rafael Wysocki |
| Headers | show |
| Series | ACPI / LPSS: Exclude I2C busses shared with PUNIT from pmc_atom_d3_mask | expand |
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@redhat.com> > --- > 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. > + > + /* 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));
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@redhat.com> >> --- >> 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. Regards, Hans
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@redhat.com> > > > --- > > > 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@linux.intel.com>
Hi, On 08-09-18 20:08, 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@redhat.com> As mentioned there was some discussion in: https://bugzilla.kernel.org/show_bug.cgi?id=200989 If this patch is necessary, or if we could somehow fix that bug differently, which would mean that we don't need this patch. That discussion is resolved now and we need to keep the code to override the powerstate of the LPSS DMA devices around as removing it stops some devices from booting :| We also need to keep Zhang Rui's fix to make sure that lpss_iosf_exit_d3_state() executes at least once and forces the DMA controllers to be on / in D0. That also means that we need this patch to be able to enter S0ix states on Bay Trail and Cherry Trail devices with the shared I2C bus. So this patch is ready for merging now, assuming it passes review. Regards, Hans > --- > 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; > + > + /* 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)); > + > lpss_deassert_reset(pdata); > > if (readl(pdata->mmio_base + pdata->dev_desc->prv_offset)) > @@ -894,7 +912,7 @@ static void lpss_iosf_enter_d3_state(void) > * Here we read the values related to LPSS power island, i.e. LPSS > * devices, excluding both LPSS DMA controllers, along with SCC domain. > */ > - u32 func_dis, d3_sts_0, pmc_status, pmc_mask = 0xfe000ffe; > + u32 func_dis, d3_sts_0, pmc_status; > int ret; > > ret = pmc_atom_read(PMC_FUNC_DIS, &func_dis); > @@ -912,7 +930,7 @@ static void lpss_iosf_enter_d3_state(void) > * Shutdown both LPSS DMA controllers if and only if all other devices > * are already in D3hot. > */ > - pmc_status = (~(d3_sts_0 | func_dis)) & pmc_mask; > + pmc_status = (~(d3_sts_0 | func_dis)) & pmc_atom_d3_mask; > if (pmc_status) > goto exit; > >
On Tue, Sep 11, 2018 at 8:46 AM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 08-09-18 20:08, 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@redhat.com> > > As mentioned there was some discussion in: > > https://bugzilla.kernel.org/show_bug.cgi?id=200989 > > If this patch is necessary, or if we could somehow fix that bug > differently, which would mean that we don't need this patch. > > That discussion is resolved now and we need to keep the code to > override the powerstate of the LPSS DMA devices around as removing > it stops some devices from booting :| > > We also need to keep Zhang Rui's fix to make sure that > lpss_iosf_exit_d3_state() executes at least once and forces the > DMA controllers to be on / in D0. > > That also means that we need this patch to be able to enter S0ix > states on Bay Trail and Cherry Trail devices with the shared > I2C bus. > > So this patch is ready for merging now, assuming it passes review. OK One process remark here, though. Sending cover letters for individual patches is not quite useful as it generally makes processing them harder. If there is additional information about the patch you don't need/want to get into the git log, you can still add it to the patch itself, between the tags and the code changes. Thanks, Rafael
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@redhat.com> > > > > --- > > > > 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@linux.intel.com> > Patch applied, thanks!
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; + + /* 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)); + lpss_deassert_reset(pdata); if (readl(pdata->mmio_base + pdata->dev_desc->prv_offset)) @@ -894,7 +912,7 @@ static void lpss_iosf_enter_d3_state(void) * Here we read the values related to LPSS power island, i.e. LPSS * devices, excluding both LPSS DMA controllers, along with SCC domain. */ - u32 func_dis, d3_sts_0, pmc_status, pmc_mask = 0xfe000ffe; + u32 func_dis, d3_sts_0, pmc_status; int ret; ret = pmc_atom_read(PMC_FUNC_DIS, &func_dis); @@ -912,7 +930,7 @@ static void lpss_iosf_enter_d3_state(void) * Shutdown both LPSS DMA controllers if and only if all other devices * are already in D3hot. */ - pmc_status = (~(d3_sts_0 | func_dis)) & pmc_mask; + pmc_status = (~(d3_sts_0 | func_dis)) & pmc_atom_d3_mask; if (pmc_status) goto exit;
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@redhat.com> --- drivers/acpi/acpi_lpss.c | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-)