| Message ID | 20170925192352.ihzakshd7yofowdd@sig21.net (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On Monday, September 25, 2017 9:23:52 PM CET Johannes Stezenbach wrote: > d31fd43c0f9a "clk: x86: Do not gate clocks enabled by the firmware" > exposed an issue on Asus E200HA where BIOS enables unused > Atom PMC clocks which prevent the system from entering S0ix. > Add a quirk to disable these clocks on E200HA. > > Signed-off-by: Johannes Stezenbach <js@sig21.net> Mika, Andy, Hans, any comments here? > --- > drivers/clk/x86/clk-pmc-atom.c | 10 +++++++--- > drivers/platform/x86/pmc_atom.c | 7 ++++++- > include/linux/platform_data/x86/clk-pmc-atom.h | 2 ++ > 3 files changed, 15 insertions(+), 4 deletions(-) > > diff --git a/drivers/clk/x86/clk-pmc-atom.c b/drivers/clk/x86/clk-pmc-atom.c > index 08ef69945ffb..213e71b905eb 100644 > --- a/drivers/clk/x86/clk-pmc-atom.c > +++ b/drivers/clk/x86/clk-pmc-atom.c > @@ -166,7 +166,8 @@ static const struct clk_ops plt_clk_ops = { > static struct clk_plt *plt_clk_register(struct platform_device *pdev, int id, > void __iomem *base, > const char **parent_names, > - int num_parents) > + int num_parents, > + bool disable_unused) > { > struct clk_plt *pclk; > struct clk_init_data init; > @@ -190,7 +191,7 @@ static struct clk_plt *plt_clk_register(struct platform_device *pdev, int id, > * If the clock was already enabled by the firmware mark it as critical > * to avoid it being gated by the clock framework if no driver owns it. > */ > - if (plt_clk_is_enabled(&pclk->hw)) > + if (!disable_unused && plt_clk_is_enabled(&pclk->hw)) > init.flags |= CLK_IS_CRITICAL; > > ret = devm_clk_hw_register(&pdev->dev, &pclk->hw); > @@ -324,6 +325,7 @@ static int plt_clk_probe(struct platform_device *pdev) > struct clk_plt_data *data; > unsigned int i; > int err; > + bool disable_unused; > > pmc_data = dev_get_platdata(&pdev->dev); > if (!pmc_data || !pmc_data->clks) > @@ -337,9 +339,11 @@ static int plt_clk_probe(struct platform_device *pdev) > if (IS_ERR(parent_names)) > return PTR_ERR(parent_names); > > + disable_unused = !!(pmc_data->flags & PMC_CLK_DATA_DISABLE_UNUSED); > for (i = 0; i < PMC_CLK_NUM; i++) { > data->clks[i] = plt_clk_register(pdev, i, pmc_data->base, > - parent_names, data->nparents); > + parent_names, data->nparents, > + disable_unused); > if (IS_ERR(data->clks[i])) { > err = PTR_ERR(data->clks[i]); > goto err_unreg_clk_plt; > diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c > index b5dd38712268..d81ada1a6f27 100644 > --- a/drivers/platform/x86/pmc_atom.c > +++ b/drivers/platform/x86/pmc_atom.c > @@ -59,7 +59,8 @@ static struct pmc_dev pmc_device; > static u32 acpi_base_addr; > > static u32 quirks; > -#define QUIRK_DISABLE_SATA BIT(0) > +#define QUIRK_DISABLE_SATA BIT(0) > +#define QUIRK_DISABLE_UNUSED_CLOCKS BIT(1) > > static const struct pmc_clk byt_clks[] = { > { > @@ -447,6 +448,8 @@ static int pmc_setup_clks(struct pci_dev *pdev, void __iomem *pmc_regmap, > > clk_data->base = pmc_regmap; /* offset is added by client */ > clk_data->clks = pmc_data->clks; > + if (quirks & QUIRK_DISABLE_UNUSED_CLOCKS) > + clk_data->flags = PMC_CLK_DATA_DISABLE_UNUSED; > > clkdev = platform_device_register_data(&pdev->dev, "clk-pmc-atom", > PLATFORM_DEVID_NONE, > @@ -517,6 +520,8 @@ static int cht_asus_e200ha_cb(const struct dmi_system_id *id) > { > pr_info("pmc: Asus E200HA detected\n"); > quirks |= QUIRK_DISABLE_SATA; > + /* BIOS enables some clocks at boot which are not needed but block S0ix */ > + quirks |= QUIRK_DISABLE_UNUSED_CLOCKS; > return 1; > } > > diff --git a/include/linux/platform_data/x86/clk-pmc-atom.h b/include/linux/platform_data/x86/clk-pmc-atom.h > index 3ab892208343..e481878ef238 100644 > --- a/include/linux/platform_data/x86/clk-pmc-atom.h > +++ b/include/linux/platform_data/x86/clk-pmc-atom.h > @@ -37,6 +37,8 @@ struct pmc_clk { > * @clks: pointer to set of registered clocks, typically 0..5 > */ > struct pmc_clk_data { > + int flags; > +#define PMC_CLK_DATA_DISABLE_UNUSED 1 > void __iomem *base; > const struct pmc_clk *clks; > }; >
Hi, On 13-12-17 01:01, Rafael J. Wysocki wrote: > On Monday, September 25, 2017 9:23:52 PM CET Johannes Stezenbach wrote: >> d31fd43c0f9a "clk: x86: Do not gate clocks enabled by the firmware" >> exposed an issue on Asus E200HA where BIOS enables unused >> Atom PMC clocks which prevent the system from entering S0ix. >> Add a quirk to disable these clocks on E200HA. >> >> Signed-off-by: Johannes Stezenbach <js@sig21.net> > > Mika, Andy, Hans, any comments here? This seems like it is papering over an issue in the d31fd43c0f9a "clk: x86: Do not gate clocks enabled by the firmware" patch to me. That patch seems like a somewhat hackish fix to me, it would be better to figure out which device needs the clock in question and fix the device's driver... And or maybe have a mask of clocks for which to do this check and not do it for all clocks at least? That way we can maybe fix both Johannes and Carlo's issue in one go without needing device specific quirks ? Carlo, do you remember which clock you needed this for ? Johannes, same question for you, which clock is being kept alive because of this ? Regards, Hans > >> --- >> drivers/clk/x86/clk-pmc-atom.c | 10 +++++++--- >> drivers/platform/x86/pmc_atom.c | 7 ++++++- >> include/linux/platform_data/x86/clk-pmc-atom.h | 2 ++ >> 3 files changed, 15 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/clk/x86/clk-pmc-atom.c b/drivers/clk/x86/clk-pmc-atom.c >> index 08ef69945ffb..213e71b905eb 100644 >> --- a/drivers/clk/x86/clk-pmc-atom.c >> +++ b/drivers/clk/x86/clk-pmc-atom.c >> @@ -166,7 +166,8 @@ static const struct clk_ops plt_clk_ops = { >> static struct clk_plt *plt_clk_register(struct platform_device *pdev, int id, >> void __iomem *base, >> const char **parent_names, >> - int num_parents) >> + int num_parents, >> + bool disable_unused) >> { >> struct clk_plt *pclk; >> struct clk_init_data init; >> @@ -190,7 +191,7 @@ static struct clk_plt *plt_clk_register(struct platform_device *pdev, int id, >> * If the clock was already enabled by the firmware mark it as critical >> * to avoid it being gated by the clock framework if no driver owns it. >> */ >> - if (plt_clk_is_enabled(&pclk->hw)) >> + if (!disable_unused && plt_clk_is_enabled(&pclk->hw)) >> init.flags |= CLK_IS_CRITICAL; >> >> ret = devm_clk_hw_register(&pdev->dev, &pclk->hw); >> @@ -324,6 +325,7 @@ static int plt_clk_probe(struct platform_device *pdev) >> struct clk_plt_data *data; >> unsigned int i; >> int err; >> + bool disable_unused; >> >> pmc_data = dev_get_platdata(&pdev->dev); >> if (!pmc_data || !pmc_data->clks) >> @@ -337,9 +339,11 @@ static int plt_clk_probe(struct platform_device *pdev) >> if (IS_ERR(parent_names)) >> return PTR_ERR(parent_names); >> >> + disable_unused = !!(pmc_data->flags & PMC_CLK_DATA_DISABLE_UNUSED); >> for (i = 0; i < PMC_CLK_NUM; i++) { >> data->clks[i] = plt_clk_register(pdev, i, pmc_data->base, >> - parent_names, data->nparents); >> + parent_names, data->nparents, >> + disable_unused); >> if (IS_ERR(data->clks[i])) { >> err = PTR_ERR(data->clks[i]); >> goto err_unreg_clk_plt; >> diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c >> index b5dd38712268..d81ada1a6f27 100644 >> --- a/drivers/platform/x86/pmc_atom.c >> +++ b/drivers/platform/x86/pmc_atom.c >> @@ -59,7 +59,8 @@ static struct pmc_dev pmc_device; >> static u32 acpi_base_addr; >> >> static u32 quirks; >> -#define QUIRK_DISABLE_SATA BIT(0) >> +#define QUIRK_DISABLE_SATA BIT(0) >> +#define QUIRK_DISABLE_UNUSED_CLOCKS BIT(1) >> >> static const struct pmc_clk byt_clks[] = { >> { >> @@ -447,6 +448,8 @@ static int pmc_setup_clks(struct pci_dev *pdev, void __iomem *pmc_regmap, >> >> clk_data->base = pmc_regmap; /* offset is added by client */ >> clk_data->clks = pmc_data->clks; >> + if (quirks & QUIRK_DISABLE_UNUSED_CLOCKS) >> + clk_data->flags = PMC_CLK_DATA_DISABLE_UNUSED; >> >> clkdev = platform_device_register_data(&pdev->dev, "clk-pmc-atom", >> PLATFORM_DEVID_NONE, >> @@ -517,6 +520,8 @@ static int cht_asus_e200ha_cb(const struct dmi_system_id *id) >> { >> pr_info("pmc: Asus E200HA detected\n"); >> quirks |= QUIRK_DISABLE_SATA; >> + /* BIOS enables some clocks at boot which are not needed but block S0ix */ >> + quirks |= QUIRK_DISABLE_UNUSED_CLOCKS; >> return 1; >> } >> >> diff --git a/include/linux/platform_data/x86/clk-pmc-atom.h b/include/linux/platform_data/x86/clk-pmc-atom.h >> index 3ab892208343..e481878ef238 100644 >> --- a/include/linux/platform_data/x86/clk-pmc-atom.h >> +++ b/include/linux/platform_data/x86/clk-pmc-atom.h >> @@ -37,6 +37,8 @@ struct pmc_clk { >> * @clks: pointer to set of registered clocks, typically 0..5 >> */ >> struct pmc_clk_data { >> + int flags; >> +#define PMC_CLK_DATA_DISABLE_UNUSED 1 >> void __iomem *base; >> const struct pmc_clk *clks; >> }; >> > >
On Wed, Dec 13, 2017 at 8:56 AM, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, /cut > Carlo, do you remember which clock you needed this for ? In one of our machine the problem was with the r8169 driver not claiming the pmc_plt_clk_4 clock. We also tried something like this https://www.spinics.net/lists/platform-driver-x86/msg12094.html but it's not like this is a much better solution IMO. Also AFAICT the same is done in: drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c sound/soc/intel/boards/bytcht_es8316.c sound/soc/intel/boards/bytcr_rt5640.c sound/soc/intel/boards/bytcr_rt5651.c sound/soc/intel/boards/cht_bsw_max98090_ti.c sound/soc/intel/boards/cht_bsw_rt5645.c sound/soc/intel/boards/cht_bsw_rt5672.c Cheers,
On Wed, Dec 13, 2017 at 09:56:45AM +0100, Hans de Goede wrote: > On 13-12-17 01:01, Rafael J. Wysocki wrote: > > On Monday, September 25, 2017 9:23:52 PM CET Johannes Stezenbach wrote: > > > d31fd43c0f9a "clk: x86: Do not gate clocks enabled by the firmware" > > > exposed an issue on Asus E200HA where BIOS enables unused > > > Atom PMC clocks which prevent the system from entering S0ix. > > > Add a quirk to disable these clocks on E200HA. > > > > > > Signed-off-by: Johannes Stezenbach <js@sig21.net> > > > > Mika, Andy, Hans, any comments here? > > This seems like it is papering over an issue in the > d31fd43c0f9a "clk: x86: Do not gate clocks enabled by the firmware" > patch to me. That patch seems like a somewhat hackish fix to > me, it would be better to figure out which device needs the clock > in question and fix the device's driver... > > And or maybe have a mask of clocks for which to do this check > and not do it for all clocks at least? That way we can maybe > fix both Johannes and Carlo's issue in one go without needing > device specific quirks ? > > Carlo, do you remember which clock you needed this for ? > > Johannes, same question for you, which clock is being kept > alive because of this ? IIRC all of these platform clocks need to be disabled for S0ix, but d31fd43c0f9a keeps them all enabled (if BIOS had enabled them), and only the one claimed by the audio codec driver is disabled by it before trying to enter S0ix. Yes, my vote would be to revert d31fd43c0f9a and fix the issue properly, but my patch was the result of previous discussion. Since it's BIOS' fault to have these clocks enabled I think a platform quirk isn't too bad. Thanks, Johannes
On 12/13/2017 05:22 AM, Johannes Stezenbach wrote: > On Wed, Dec 13, 2017 at 09:56:45AM +0100, Hans de Goede wrote: >> On 13-12-17 01:01, Rafael J. Wysocki wrote: >>> On Monday, September 25, 2017 9:23:52 PM CET Johannes Stezenbach wrote: >>>> d31fd43c0f9a "clk: x86: Do not gate clocks enabled by the firmware" >>>> exposed an issue on Asus E200HA where BIOS enables unused >>>> Atom PMC clocks which prevent the system from entering S0ix. >>>> Add a quirk to disable these clocks on E200HA. >>>> >>>> Signed-off-by: Johannes Stezenbach <js@sig21.net> >>> Mika, Andy, Hans, any comments here? >> This seems like it is papering over an issue in the >> d31fd43c0f9a "clk: x86: Do not gate clocks enabled by the firmware" >> patch to me. That patch seems like a somewhat hackish fix to >> me, it would be better to figure out which device needs the clock >> in question and fix the device's driver... >> >> And or maybe have a mask of clocks for which to do this check >> and not do it for all clocks at least? That way we can maybe >> fix both Johannes and Carlo's issue in one go without needing >> device specific quirks ? >> >> Carlo, do you remember which clock you needed this for ? >> >> Johannes, same question for you, which clock is being kept >> alive because of this ? > IIRC all of these platform clocks need to be disabled for S0ix, I don't know if this is 100% correct. We've enabled S0i1 with audio playback running in the past, and i think the pmc_clk_3 was used. > but d31fd43c0f9a keeps them all enabled (if BIOS had enabled them), > and only the one claimed by the audio codec driver > is disabled by it before trying to enter S0ix. > > Yes, my vote would be to revert d31fd43c0f9a and fix the issue > properly, but my patch was the result of previous discussion. > Since it's BIOS' fault to have these clocks enabled I think > a platform quirk isn't too bad. > > > Thanks, > Johannes
On Wed, 2017-12-13 at 09:56 +0100, Hans de Goede wrote: > Hi, > > On 13-12-17 01:01, Rafael J. Wysocki wrote: > > On Monday, September 25, 2017 9:23:52 PM CET Johannes Stezenbach > > wrote: > > > d31fd43c0f9a "clk: x86: Do not gate clocks enabled by the > > > firmware" > > > exposed an issue on Asus E200HA where BIOS enables unused > > > Atom PMC clocks which prevent the system from entering S0ix. > > > Add a quirk to disable these clocks on E200HA. > > > > > > Signed-off-by: Johannes Stezenbach <js@sig21.net> > > > > Mika, Andy, Hans, any comments here? I remember discussing those in bugzilla, though I agree with Hans, it looks hackish still. I'm not sure on the other hand we can solve this properly in a meantime. I also have some style related comments, but they are minor. I can go through better review after we settle down the way we would like to fix the issue. > This seems like it is papering over an issue in the > d31fd43c0f9a "clk: x86: Do not gate clocks enabled by the firmware" > patch to me. That patch seems like a somewhat hackish fix to > me, it would be better to figure out which device needs the clock > in question and fix the device's driver... My understanding of S0ix prerequisites is all devices in question *must* have drivers loaded and drivers *must* have implemented runtime PM. (Since I don't know if it's guaranteed by firmware that devices are left in D3 if they are not used. Besides that from the SATA case looks like some BIOS [hardware?] issue with power gating)
diff --git a/drivers/clk/x86/clk-pmc-atom.c b/drivers/clk/x86/clk-pmc-atom.c index 08ef69945ffb..213e71b905eb 100644 --- a/drivers/clk/x86/clk-pmc-atom.c +++ b/drivers/clk/x86/clk-pmc-atom.c @@ -166,7 +166,8 @@ static const struct clk_ops plt_clk_ops = { static struct clk_plt *plt_clk_register(struct platform_device *pdev, int id, void __iomem *base, const char **parent_names, - int num_parents) + int num_parents, + bool disable_unused) { struct clk_plt *pclk; struct clk_init_data init; @@ -190,7 +191,7 @@ static struct clk_plt *plt_clk_register(struct platform_device *pdev, int id, * If the clock was already enabled by the firmware mark it as critical * to avoid it being gated by the clock framework if no driver owns it. */ - if (plt_clk_is_enabled(&pclk->hw)) + if (!disable_unused && plt_clk_is_enabled(&pclk->hw)) init.flags |= CLK_IS_CRITICAL; ret = devm_clk_hw_register(&pdev->dev, &pclk->hw); @@ -324,6 +325,7 @@ static int plt_clk_probe(struct platform_device *pdev) struct clk_plt_data *data; unsigned int i; int err; + bool disable_unused; pmc_data = dev_get_platdata(&pdev->dev); if (!pmc_data || !pmc_data->clks) @@ -337,9 +339,11 @@ static int plt_clk_probe(struct platform_device *pdev) if (IS_ERR(parent_names)) return PTR_ERR(parent_names); + disable_unused = !!(pmc_data->flags & PMC_CLK_DATA_DISABLE_UNUSED); for (i = 0; i < PMC_CLK_NUM; i++) { data->clks[i] = plt_clk_register(pdev, i, pmc_data->base, - parent_names, data->nparents); + parent_names, data->nparents, + disable_unused); if (IS_ERR(data->clks[i])) { err = PTR_ERR(data->clks[i]); goto err_unreg_clk_plt; diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c index b5dd38712268..d81ada1a6f27 100644 --- a/drivers/platform/x86/pmc_atom.c +++ b/drivers/platform/x86/pmc_atom.c @@ -59,7 +59,8 @@ static struct pmc_dev pmc_device; static u32 acpi_base_addr; static u32 quirks; -#define QUIRK_DISABLE_SATA BIT(0) +#define QUIRK_DISABLE_SATA BIT(0) +#define QUIRK_DISABLE_UNUSED_CLOCKS BIT(1) static const struct pmc_clk byt_clks[] = { { @@ -447,6 +448,8 @@ static int pmc_setup_clks(struct pci_dev *pdev, void __iomem *pmc_regmap, clk_data->base = pmc_regmap; /* offset is added by client */ clk_data->clks = pmc_data->clks; + if (quirks & QUIRK_DISABLE_UNUSED_CLOCKS) + clk_data->flags = PMC_CLK_DATA_DISABLE_UNUSED; clkdev = platform_device_register_data(&pdev->dev, "clk-pmc-atom", PLATFORM_DEVID_NONE, @@ -517,6 +520,8 @@ static int cht_asus_e200ha_cb(const struct dmi_system_id *id) { pr_info("pmc: Asus E200HA detected\n"); quirks |= QUIRK_DISABLE_SATA; + /* BIOS enables some clocks at boot which are not needed but block S0ix */ + quirks |= QUIRK_DISABLE_UNUSED_CLOCKS; return 1; } diff --git a/include/linux/platform_data/x86/clk-pmc-atom.h b/include/linux/platform_data/x86/clk-pmc-atom.h index 3ab892208343..e481878ef238 100644 --- a/include/linux/platform_data/x86/clk-pmc-atom.h +++ b/include/linux/platform_data/x86/clk-pmc-atom.h @@ -37,6 +37,8 @@ struct pmc_clk { * @clks: pointer to set of registered clocks, typically 0..5 */ struct pmc_clk_data { + int flags; +#define PMC_CLK_DATA_DISABLE_UNUSED 1 void __iomem *base; const struct pmc_clk *clks; };
d31fd43c0f9a "clk: x86: Do not gate clocks enabled by the firmware" exposed an issue on Asus E200HA where BIOS enables unused Atom PMC clocks which prevent the system from entering S0ix. Add a quirk to disable these clocks on E200HA. Signed-off-by: Johannes Stezenbach <js@sig21.net> --- drivers/clk/x86/clk-pmc-atom.c | 10 +++++++--- drivers/platform/x86/pmc_atom.c | 7 ++++++- include/linux/platform_data/x86/clk-pmc-atom.h | 2 ++ 3 files changed, 15 insertions(+), 4 deletions(-)