Re: S0ix failure due to "clk: x86: Do not gate clocks enabled by the firmware"

Re: S0ix failure due to "clk: x86: Do not gate clocks enabled by the firmware"

[Johannes Stezenbach]

(adding linux-acpi)

On Wed, Sep 06, 2017 at 04:02:10PM -0500, Pierre-Louis Bossart wrote:
> On 9/6/17 3:42 PM, Johannes Stezenbach wrote:
> > I'm poking around on Asus E200HA, based on Atom x5-Z8300 (Cherrytrail),
> > trying to get S0ix working, atm with gross hacks that are recorded in
> > https://bugzilla.kernel.org/show_bug.cgi?id=193891
> > 
> > 
> > I found commit d31fd43c0f9a4 "clk: x86: Do not gate clocks enabled by the firmware"
> > prevents S0i3 entry.  Unfortunately the commit message doesn't explain
> > the problem it fixes, nor does it mention the platform.
> > 
> > How can we fix d31fd43c0f9a4 to not break S0i3?
> 
> In the existing audio machine drivers, I only use the clock framework for
> Baytrail. on Cherrytrail, the pmc_platform_clk3 used for the audio MCLK is
> actually managed by the BIOS (look for CLK3 in the DSDT) so there is no
> activity in the drivers, the audio don't even get/prepare/enable the
> pmc_platform_clk3 so I would guess there is an issue with a non-audio driver
> or the BIOS. Put differently, it may be so that by allowing the drivers to
> gate an unused clock we allowed you to enter S0i3 even if the bios didn't
> enable this mode. I would remove all pmc-clock related stuff and see what
> happens.

Thanks for this info and Carlo's reply.  I've been busy reading
source code, DSDT and ACPI spec, and now I'm confused. ;-/

As you say, these clocks are managed by BIOS and
clk-pmc-atom.c shouldn't mess with them, thus
d31fd43c0f9a4 is correct.  Nothing else in the
system besides sound/soc/intel/boards/ uses
pmc_plt_clk_3 (or other pmc_plt_clk*).

Now the question is why ACPI doesn't handle the clocks.

E200HA DSDT https://bugzilla.kernel.org/attachment.cgi?id=253921
has PowerResource defintions for the clocks
(where _ON and _OFF just set the same registers that
clk-pmc-atom.c uses, e.g. CKC3 at 0x6c for CLK3).
However, these PowerResources are only listed as child nodes
of the devices that use them, but are not referenced by _PR[0-3],
with the exception of CLK3 used by the audio codec.

The power resources exist in /sys as LNXPOWER:xx subnodes
of the device (e.g. i2c 809622C1:02/LNXPOWER:05) and are
also listed during boot.

acpi_bus_init_power_state() sets device->power.flags.power_resources = 1
only if _PR[0-3] exist.  Then acpi_device_set_power() only
calls acpi_power_transition() if power_resource == 1,
and acpi_power_transition() would only call acpi_power_off_list()
for the resources according to _PR[0-3].

I think this is all OK, maybe the BIOS is not optimal in
making the clocks available for device runtime pm.
What doesn't seem to happen, though, is that the PowerResources
are turned _OFF when system transitions to S0ix.  The PowerResources
have systemlevel == 0, meaning they should be turned off when the
system transitions out of S0, if I'm understanding the ACPI (6.1)
spec correctly in section 7.2 Declaring a Power Resource Object.
Well, actually I'm not sure, the wording of the ACPI spec is:

  "Systemlevel is the deepest system sleep level OSPM must maintain to keep
  this power resource on (0 equates to S0, 1 equates to S1, and so on)."

FWIW, I used "echo 0x00800000 >/sys/module/acpi/parameters/debug_layer"
to confirm devices transition between D0 and D3hot, but clock
resources are never turned off.


Johannes

Re: S0ix failure due to "clk: x86: Do not gate clocks enabled by the firmware"

[Johannes Stezenbach]

Hi,

TL;DR: ACPI 6.1 spec interpretation question:

  7.2 Declaring a Power Resource Object
  ...
  Power resource objects can appear wherever is convenient in the namespace.
  ...
  Systemlevel is the deepest system sleep level OSPM must maintain to keep this
  power resource on (0 equates to S0, 1 equates to S1, and so on).

Does it imply the OS should turn Power Resources off when changing
the system level below the systemlevel of the PowerResource?

Otherwise, what purpose do PowerResource objects have that are not
mentioned in any device _PR[0-3W]?  Or is it BIOS bug?


Thanks,
Johannes

On Fri, Sep 08, 2017 at 03:49:20PM +0200, Johannes Stezenbach wrote:
> (adding linux-acpi)
> 
> On Wed, Sep 06, 2017 at 04:02:10PM -0500, Pierre-Louis Bossart wrote:
> > On 9/6/17 3:42 PM, Johannes Stezenbach wrote:
> > > I'm poking around on Asus E200HA, based on Atom x5-Z8300 (Cherrytrail),
> > > trying to get S0ix working, atm with gross hacks that are recorded in
> > > https://bugzilla.kernel.org/show_bug.cgi?id=193891
> > > 
> > > 
> > > I found commit d31fd43c0f9a4 "clk: x86: Do not gate clocks enabled by the firmware"
> > > prevents S0i3 entry.  Unfortunately the commit message doesn't explain
> > > the problem it fixes, nor does it mention the platform.
> > > 
> > > How can we fix d31fd43c0f9a4 to not break S0i3?
> > 
> > In the existing audio machine drivers, I only use the clock framework for
> > Baytrail. on Cherrytrail, the pmc_platform_clk3 used for the audio MCLK is
> > actually managed by the BIOS (look for CLK3 in the DSDT) so there is no
> > activity in the drivers, the audio don't even get/prepare/enable the
> > pmc_platform_clk3 so I would guess there is an issue with a non-audio driver
> > or the BIOS. Put differently, it may be so that by allowing the drivers to
> > gate an unused clock we allowed you to enter S0i3 even if the bios didn't
> > enable this mode. I would remove all pmc-clock related stuff and see what
> > happens.
> 
> Thanks for this info and Carlo's reply.  I've been busy reading
> source code, DSDT and ACPI spec, and now I'm confused. ;-/
> 
> As you say, these clocks are managed by BIOS and
> clk-pmc-atom.c shouldn't mess with them, thus
> d31fd43c0f9a4 is correct.  Nothing else in the
> system besides sound/soc/intel/boards/ uses
> pmc_plt_clk_3 (or other pmc_plt_clk*).
> 
> Now the question is why ACPI doesn't handle the clocks.
> 
> E200HA DSDT https://bugzilla.kernel.org/attachment.cgi?id=253921
> has PowerResource defintions for the clocks
> (where _ON and _OFF just set the same registers that
> clk-pmc-atom.c uses, e.g. CKC3 at 0x6c for CLK3).
> However, these PowerResources are only listed as child nodes
> of the devices that use them, but are not referenced by _PR[0-3],
> with the exception of CLK3 used by the audio codec.
> 
> The power resources exist in /sys as LNXPOWER:xx subnodes
> of the device (e.g. i2c 809622C1:02/LNXPOWER:05) and are
> also listed during boot.
> 
> acpi_bus_init_power_state() sets device->power.flags.power_resources = 1
> only if _PR[0-3] exist.  Then acpi_device_set_power() only
> calls acpi_power_transition() if power_resource == 1,
> and acpi_power_transition() would only call acpi_power_off_list()
> for the resources according to _PR[0-3].
> 
> I think this is all OK, maybe the BIOS is not optimal in
> making the clocks available for device runtime pm.
> What doesn't seem to happen, though, is that the PowerResources
> are turned _OFF when system transitions to S0ix.  The PowerResources
> have systemlevel == 0, meaning they should be turned off when the
> system transitions out of S0, if I'm understanding the ACPI (6.1)
> spec correctly in section 7.2 Declaring a Power Resource Object.
> Well, actually I'm not sure, the wording of the ACPI spec is:
> 
>   "Systemlevel is the deepest system sleep level OSPM must maintain to keep
>   this power resource on (0 equates to S0, 1 equates to S1, and so on)."
> 
> FWIW, I used "echo 0x00800000 >/sys/module/acpi/parameters/debug_layer"
> to confirm devices transition between D0 and D3hot, but clock
> resources are never turned off.
> 
> 
> Johannes
> 

Re: S0ix failure due to "clk: x86: Do not gate clocks enabled by the firmware"

[Rafael J. Wysocki]

On Thursday, September 21, 2017 11:40:13 AM CEST Johannes Stezenbach wrote:
> Hi,
> 
> TL;DR: ACPI 6.1 spec interpretation question:
> 
>   7.2 Declaring a Power Resource Object
>   ...
>   Power resource objects can appear wherever is convenient in the namespace.
>   ...
>   Systemlevel is the deepest system sleep level OSPM must maintain to keep this
>   power resource on (0 equates to S0, 1 equates to S1, and so on).
> 
> Does it imply the OS should turn Power Resources off when changing
> the system level below the systemlevel of the PowerResource?

You can interpret that this way I guess.

However, so far we have not had to deal with "dangling" power resources.

> Otherwise, what purpose do PowerResource objects have that are not
> mentioned in any device _PR[0-3W]?  Or is it BIOS bug?

There also is _RDI that may refer to power resources, but I'm not aware of
anyone using it in practice ATM.

Other than that, I don't see why it would ever be necessary to declare
a power resource that is not pointed to by any of the above.

At least when the "power level" is changing from S0 to anything like S3,
the firmware gets control anyway at one point and then it is free to
manipulate whatever power resources it needs to turn off at that point.

So I would be inclined to think of that as a BIOS issue.

Thanks,
Rafael

Re: S0ix failure due to "clk: x86: Do not gate clocks enabled by the firmware"

[Johannes Stezenbach]

On Thu, Sep 21, 2017 at 04:21:46PM +0200, Rafael J. Wysocki wrote:
> So I would be inclined to think of that as a BIOS issue.

OK, wrt $Subject it sparks the question how to fix the
issue exposed by commit d31fd43c0f9a4 "clk: x86: Do not
gate clocks enabled by the firmware".  Add quirks to
the drivers for the devices that have the dangling
PowerResources and manually call _OFF/_ON during suspend/resume?

I must admit I haven't looked deeper into the issue yet,
i.e. I don't know which clock causes S0ix failure.
If I understand correctly, before d31fd43c0f9a4 the
clk framework would just disable all unused clocks
registered by clk-pmc-atom.c, which are all of then except
CLK3 which is used by the audio codec. And the audio codec
would disable CLK3 during suspend.
After d31fd43c0f9a4 all clocks that BIOS had enabled during boot
are kept running.  Presumably we could also add the quirk to
clk-pmc-atom.c to bypass what d31fd43c0f9a4 added on Asus E200HA?


Thanks,
Johannes

Re: S0ix failure due to "clk: x86: Do not gate clocks enabled by the firmware"

[Rafael J. Wysocki]

On Thursday, September 21, 2017 6:23:07 PM CEST Johannes Stezenbach wrote:
> On Thu, Sep 21, 2017 at 04:21:46PM +0200, Rafael J. Wysocki wrote:
> > So I would be inclined to think of that as a BIOS issue.
> 
> OK, wrt $Subject it sparks the question how to fix the
> issue exposed by commit d31fd43c0f9a4 "clk: x86: Do not
> gate clocks enabled by the firmware".  Add quirks to
> the drivers for the devices that have the dangling
> PowerResources and manually call _OFF/_ON during suspend/resume?

Well, it may just be better to do that in the core at the suspend
time, so I'm wondering if the appended patch makes any difference?

---
 drivers/acpi/power.c |   36 ++++++++++++++++++++++++++++++++++++
 drivers/acpi/sleep.h |    1 +
 2 files changed, 37 insertions(+)

Index: linux-pm/drivers/acpi/power.c
===================================================================
--- linux-pm.orig/drivers/acpi/power.c
+++ linux-pm/drivers/acpi/power.c
@@ -64,6 +64,7 @@ struct acpi_power_resource {
 	unsigned int ref_count;
 	bool wakeup_enabled;
 	struct mutex resource_lock;
+	bool suspended;
 };
 
 struct acpi_power_resource_entry {
@@ -839,6 +840,37 @@ int acpi_add_power_resource(acpi_handle
 }
 
 #ifdef CONFIG_ACPI_SLEEP
+void acpi_suspend_power_resources(u32 acpi_state)
+{
+	struct acpi_power_resource *resource;
+
+	mutex_lock(&power_resource_list_lock);
+
+	list_for_each_entry(resource, &acpi_power_resource_list, list_node) {
+		int result, state;
+
+		mutex_lock(&resource->resource_lock);
+
+		result = acpi_power_get_state(resource->device.handle, &state);
+		if (result) {
+			mutex_unlock(&resource->resource_lock);
+			continue;
+		}
+
+		if (state == ACPI_POWER_RESOURCE_STATE_ON
+		    && resource->system_level < acpi_state) {
+			dev_info(&resource->device.dev, "Turning OFF\n");
+			__acpi_power_off(resource);
+			resource->ref_count++;
+			resource->suspended = true;
+		}
+
+		mutex_unlock(&resource->resource_lock);
+	}
+
+	mutex_unlock(&power_resource_list_lock);
+}
+
 void acpi_resume_power_resources(void)
 {
 	struct acpi_power_resource *resource;
@@ -860,6 +892,10 @@ void acpi_resume_power_resources(void)
 		    && resource->ref_count) {
 			dev_info(&resource->device.dev, "Turning ON\n");
 			__acpi_power_on(resource);
+			if (resource->suspended) {
+				resource->ref_count--;
+				resource->suspended = false;
+			}
 		}
 
 		mutex_unlock(&resource->resource_lock);
Index: linux-pm/drivers/acpi/sleep.h
===================================================================
--- linux-pm.orig/drivers/acpi/sleep.h
+++ linux-pm/drivers/acpi/sleep.h
@@ -5,6 +5,7 @@ extern void acpi_disable_wakeup_devices(
 extern struct list_head acpi_wakeup_device_list;
 extern struct mutex acpi_device_lock;
 
+extern void acpi_suspend_power_resources(u32 acpi_state);
 extern void acpi_resume_power_resources(void);
 extern void acpi_turn_off_unused_power_resources(void);
 

Re: S0ix failure due to "clk: x86: Do not gate clocks enabled by the firmware"

[Rafael J. Wysocki]

On Friday, September 22, 2017 12:20:55 AM CEST Rafael J. Wysocki wrote:
> On Thursday, September 21, 2017 6:23:07 PM CEST Johannes Stezenbach wrote:
> > On Thu, Sep 21, 2017 at 04:21:46PM +0200, Rafael J. Wysocki wrote:
> > > So I would be inclined to think of that as a BIOS issue.
> > 
> > OK, wrt $Subject it sparks the question how to fix the
> > issue exposed by commit d31fd43c0f9a4 "clk: x86: Do not
> > gate clocks enabled by the firmware".  Add quirks to
> > the drivers for the devices that have the dangling
> > PowerResources and manually call _OFF/_ON during suspend/resume?
> 
> Well, it may just be better to do that in the core at the suspend
> time, so I'm wondering if the appended patch makes any difference?

But if this is S0ix, then the system states is S0 anyway all the time, so
I'm not sure about the point here, honestly.

Thanks,
Rafael

Re: S0ix failure due to "clk: x86: Do not gate clocks enabled by the firmware"

[Rafael J. Wysocki]

On Thursday, September 21, 2017 6:23:07 PM CEST Johannes Stezenbach wrote:
> On Thu, Sep 21, 2017 at 04:21:46PM +0200, Rafael J. Wysocki wrote:
> > So I would be inclined to think of that as a BIOS issue.
> 
> OK, wrt $Subject it sparks the question how to fix the
> issue exposed by commit d31fd43c0f9a4 "clk: x86: Do not
> gate clocks enabled by the firmware".  Add quirks to
> the drivers for the devices that have the dangling
> PowerResources and manually call _OFF/_ON during suspend/resume?
> 
> I must admit I haven't looked deeper into the issue yet,
> i.e. I don't know which clock causes S0ix failure.
> If I understand correctly, before d31fd43c0f9a4 the
> clk framework would just disable all unused clocks
> registered by clk-pmc-atom.c, which are all of then except
> CLK3 which is used by the audio codec. And the audio codec
> would disable CLK3 during suspend.
> After d31fd43c0f9a4 all clocks that BIOS had enabled during boot
> are kept running.  Presumably we could also add the quirk to
> clk-pmc-atom.c to bypass what d31fd43c0f9a4 added on Asus E200HA?

Since this is S0ix, the "system level" is 0 all the time (as I said in a
previous message).

So there seem to be two possible ways to address this, unfortunately both
based on white- or blacklisting.

One of them would be to only do the d31fd43c0f9a4 approach for selected
systems where it needs to be done to avoid breakage.  The other one would
be to do it for all systems by default and blacklist the ones where that
leads to problems.

IMO it is better to disable clocks over suspend-resume, because that reduces
the system's power draw while suspended.  So that's what I would do and then
keep the clocks running only if that is known to be necessary.

Thanks,
Rafael

Re: S0ix failure due to "clk: x86: Do not gate clocks enabled by the firmware"

[Johannes Stezenbach]

On Fri, Sep 22, 2017 at 12:35:15AM +0200, Rafael J. Wysocki wrote:
> On Thursday, September 21, 2017 6:23:07 PM CEST Johannes Stezenbach wrote:
> > On Thu, Sep 21, 2017 at 04:21:46PM +0200, Rafael J. Wysocki wrote:
> > > So I would be inclined to think of that as a BIOS issue.
> > 
> > OK, wrt $Subject it sparks the question how to fix the
> > issue exposed by commit d31fd43c0f9a4 "clk: x86: Do not
> > gate clocks enabled by the firmware".  Add quirks to
> > the drivers for the devices that have the dangling
> > PowerResources and manually call _OFF/_ON during suspend/resume?
> > 
> > I must admit I haven't looked deeper into the issue yet,
> > i.e. I don't know which clock causes S0ix failure.
> > If I understand correctly, before d31fd43c0f9a4 the
> > clk framework would just disable all unused clocks
> > registered by clk-pmc-atom.c, which are all of then except
> > CLK3 which is used by the audio codec. And the audio codec
> > would disable CLK3 during suspend.
> > After d31fd43c0f9a4 all clocks that BIOS had enabled during boot
> > are kept running.  Presumably we could also add the quirk to
> > clk-pmc-atom.c to bypass what d31fd43c0f9a4 added on Asus E200HA?
> 
> Since this is S0ix, the "system level" is 0 all the time (as I said in a
> previous message).

Damn.  Reading ACPI spec (6.1 and 6.2), it talks about _LPI and _RDI
but E200HA doesn't have that in any ACPI table.
The table in section 7.1 Power Resource Objects and the Power Management Models
says "Choose a supported device state to enable a targeted system sleep or
Low- power Idle state ... [use] _PRx".

> So there seem to be two possible ways to address this, unfortunately both
> based on white- or blacklisting.
> 
> One of them would be to only do the d31fd43c0f9a4 approach for selected
> systems where it needs to be done to avoid breakage.  The other one would
> be to do it for all systems by default and blacklist the ones where that
> leads to problems.
> 
> IMO it is better to disable clocks over suspend-resume, because that reduces
> the system's power draw while suspended.  So that's what I would do and then
> keep the clocks running only if that is known to be necessary.

d31fd43c0f9a4 was added to fix a "hard hangup on boot" issue,
so I guess the default should be to keep clocks enabled.

BTW, in E200HA DSDT, the PowerResources in question are children of
the I2Cx devices, presumably it means these clocks are to be used by
devices connected to the i2c busses.  Maybe it's a left over from devices
that are not present in E200HA, and we should ignore them.

In conclusion the BIOS bug is that it enables these clocks at boot,
and the correct quirk is to make clk-pmc-atom.c disable them during init,
but leave the _PRx handling for the used clocks (CLK3) to ACPI.

Right?


Thanks,
Johannes

Re: S0ix failure due to "clk: x86: Do not gate clocks enabled by the firmware"

[Takashi Iwai]

On Fri, 22 Sep 2017 10:04:53 +0200,
Johannes Stezenbach wrote:
> 
> On Fri, Sep 22, 2017 at 12:35:15AM +0200, Rafael J. Wysocki wrote:
> > On Thursday, September 21, 2017 6:23:07 PM CEST Johannes Stezenbach wrote:
> > > On Thu, Sep 21, 2017 at 04:21:46PM +0200, Rafael J. Wysocki wrote:
> > > > So I would be inclined to think of that as a BIOS issue.
> > > 
> > > OK, wrt $Subject it sparks the question how to fix the
> > > issue exposed by commit d31fd43c0f9a4 "clk: x86: Do not
> > > gate clocks enabled by the firmware".  Add quirks to
> > > the drivers for the devices that have the dangling
> > > PowerResources and manually call _OFF/_ON during suspend/resume?
> > > 
> > > I must admit I haven't looked deeper into the issue yet,
> > > i.e. I don't know which clock causes S0ix failure.
> > > If I understand correctly, before d31fd43c0f9a4 the
> > > clk framework would just disable all unused clocks
> > > registered by clk-pmc-atom.c, which are all of then except
> > > CLK3 which is used by the audio codec. And the audio codec
> > > would disable CLK3 during suspend.
> > > After d31fd43c0f9a4 all clocks that BIOS had enabled during boot
> > > are kept running.  Presumably we could also add the quirk to
> > > clk-pmc-atom.c to bypass what d31fd43c0f9a4 added on Asus E200HA?
> > 
> > Since this is S0ix, the "system level" is 0 all the time (as I said in a
> > previous message).
> 
> Damn.  Reading ACPI spec (6.1 and 6.2), it talks about _LPI and _RDI
> but E200HA doesn't have that in any ACPI table.
> The table in section 7.1 Power Resource Objects and the Power Management Models
> says "Choose a supported device state to enable a targeted system sleep or
> Low- power Idle state ... [use] _PRx".
> 
> > So there seem to be two possible ways to address this, unfortunately both
> > based on white- or blacklisting.
> > 
> > One of them would be to only do the d31fd43c0f9a4 approach for selected
> > systems where it needs to be done to avoid breakage.  The other one would
> > be to do it for all systems by default and blacklist the ones where that
> > leads to problems.
> > 
> > IMO it is better to disable clocks over suspend-resume, because that reduces
> > the system's power draw while suspended.  So that's what I would do and then
> > keep the clocks running only if that is known to be necessary.
> 
> d31fd43c0f9a4 was added to fix a "hard hangup on boot" issue,
> so I guess the default should be to keep clocks enabled.
> 
> BTW, in E200HA DSDT, the PowerResources in question are children of
> the I2Cx devices, presumably it means these clocks are to be used by
> devices connected to the i2c busses.  Maybe it's a left over from devices
> that are not present in E200HA, and we should ignore them.
> 
> In conclusion the BIOS bug is that it enables these clocks at boot,
> and the correct quirk is to make clk-pmc-atom.c disable them during init,
> but leave the _PRx handling for the used clocks (CLK3) to ACPI.

Well, I can't say which is the buggy behavior, but at least, I agree
with Rafael that white/blacklisting is likely the way to go.

One thing I'm wondering is whether it's specific to device or specific
to platform.  The ASUS one that showed a regression Johannes and I
have is the Cherrytrail, while the device showed the hang-at-boot was
Baytrail, IIRC.


thanks,

Takashi

Re: S0ix failure due to "clk: x86: Do not gate clocks enabled by the firmware"

[Johannes Stezenbach]

On Fri, Sep 22, 2017 at 02:27:57PM +0200, Takashi Iwai wrote:
> On Fri, 22 Sep 2017 10:04:53 +0200,
> Johannes Stezenbach wrote:
> > 
> > In conclusion the BIOS bug is that it enables these clocks at boot,
> > and the correct quirk is to make clk-pmc-atom.c disable them during init,
> > but leave the _PRx handling for the used clocks (CLK3) to ACPI.
> 
> Well, I can't say which is the buggy behavior, but at least, I agree
> with Rafael that white/blacklisting is likely the way to go.
> 
> One thing I'm wondering is whether it's specific to device or specific
> to platform.  The ASUS one that showed a regression Johannes and I
> have is the Cherrytrail, while the device showed the hang-at-boot was
> Baytrail, IIRC.

If I understand correctly, the clk-pmc-atom.c clocks are not used
by the SoC itself but provided for devices added to system by OEM.
So I don't think its platform (BYT vs. CHT) specific.

Meanwhile I have doubts about d31fd43c0f9a41, though.

The original issue mentioned by Carlo:
https://www.spinics.net/lists/platform-driver-x86/msg12092.html

>>> We have a quirk downstream in place where we basically modified the
>>> r8169 driver to claim the 25MHz pmc_plt_clk_4 clock, and this seems to
>>> work fine, but we really want to find a more upstreamable and
>>> definitive solution.

It seems to me that this quirk to claim pmc_plt_clk_4 would
actually be a better solution than just keeping
clocks enabled.  And you'd need to handle the clock
during suspend for S0ix to work, if Baytrail pmc_plt_clk_4
has the same S0ix blocking behaviour as Cherrytrail CLK3.
IOW, the driver which uses the pmc_plt_clk_4 clock needs
to handle it if ACPI DSDT doesn't handle it via _PRx.


Thanks,
Johannes

Re: S0ix failure due to "clk: x86: Do not gate clocks enabled by the firmware"

[Rafael J. Wysocki]

On Friday, September 22, 2017 2:27:57 PM CEST Takashi Iwai wrote:
> On Fri, 22 Sep 2017 10:04:53 +0200,
> Johannes Stezenbach wrote:
> > 
> > On Fri, Sep 22, 2017 at 12:35:15AM +0200, Rafael J. Wysocki wrote:
> > > On Thursday, September 21, 2017 6:23:07 PM CEST Johannes Stezenbach wrote:
> > > > On Thu, Sep 21, 2017 at 04:21:46PM +0200, Rafael J. Wysocki wrote:
> > > > > So I would be inclined to think of that as a BIOS issue.
> > > > 
> > > > OK, wrt $Subject it sparks the question how to fix the
> > > > issue exposed by commit d31fd43c0f9a4 "clk: x86: Do not
> > > > gate clocks enabled by the firmware".  Add quirks to
> > > > the drivers for the devices that have the dangling
> > > > PowerResources and manually call _OFF/_ON during suspend/resume?
> > > > 
> > > > I must admit I haven't looked deeper into the issue yet,
> > > > i.e. I don't know which clock causes S0ix failure.
> > > > If I understand correctly, before d31fd43c0f9a4 the
> > > > clk framework would just disable all unused clocks
> > > > registered by clk-pmc-atom.c, which are all of then except
> > > > CLK3 which is used by the audio codec. And the audio codec
> > > > would disable CLK3 during suspend.
> > > > After d31fd43c0f9a4 all clocks that BIOS had enabled during boot
> > > > are kept running.  Presumably we could also add the quirk to
> > > > clk-pmc-atom.c to bypass what d31fd43c0f9a4 added on Asus E200HA?
> > > 
> > > Since this is S0ix, the "system level" is 0 all the time (as I said in a
> > > previous message).
> > 
> > Damn.  Reading ACPI spec (6.1 and 6.2), it talks about _LPI and _RDI
> > but E200HA doesn't have that in any ACPI table.
> > The table in section 7.1 Power Resource Objects and the Power Management Models
> > says "Choose a supported device state to enable a targeted system sleep or
> > Low- power Idle state ... [use] _PRx".
> > 
> > > So there seem to be two possible ways to address this, unfortunately both
> > > based on white- or blacklisting.
> > > 
> > > One of them would be to only do the d31fd43c0f9a4 approach for selected
> > > systems where it needs to be done to avoid breakage.  The other one would
> > > be to do it for all systems by default and blacklist the ones where that
> > > leads to problems.
> > > 
> > > IMO it is better to disable clocks over suspend-resume, because that reduces
> > > the system's power draw while suspended.  So that's what I would do and then
> > > keep the clocks running only if that is known to be necessary.
> > 
> > d31fd43c0f9a4 was added to fix a "hard hangup on boot" issue,
> > so I guess the default should be to keep clocks enabled.
> > 
> > BTW, in E200HA DSDT, the PowerResources in question are children of
> > the I2Cx devices, presumably it means these clocks are to be used by
> > devices connected to the i2c busses.  Maybe it's a left over from devices
> > that are not present in E200HA, and we should ignore them.
> > 
> > In conclusion the BIOS bug is that it enables these clocks at boot,
> > and the correct quirk is to make clk-pmc-atom.c disable them during init,
> > but leave the _PRx handling for the used clocks (CLK3) to ACPI.
> 
> Well, I can't say which is the buggy behavior, but at least, I agree
> with Rafael that white/blacklisting is likely the way to go.
> 
> One thing I'm wondering is whether it's specific to device or specific
> to platform.  The ASUS one that showed a regression Johannes and I
> have is the Cherrytrail, while the device showed the hang-at-boot was
> Baytrail, IIRC.

If I were to place bets, I'd bet on platform-specific and not machine model-
specific.

Thanks,
Rafael

Re: S0ix failure due to "clk: x86: Do not gate clocks enabled by the firmware"

[Rafael J. Wysocki]

On Friday, September 22, 2017 10:04:53 AM CEST Johannes Stezenbach wrote:
> On Fri, Sep 22, 2017 at 12:35:15AM +0200, Rafael J. Wysocki wrote:
> > On Thursday, September 21, 2017 6:23:07 PM CEST Johannes Stezenbach wrote:
> > > On Thu, Sep 21, 2017 at 04:21:46PM +0200, Rafael J. Wysocki wrote:
> > > > So I would be inclined to think of that as a BIOS issue.
> > > 
> > > OK, wrt $Subject it sparks the question how to fix the
> > > issue exposed by commit d31fd43c0f9a4 "clk: x86: Do not
> > > gate clocks enabled by the firmware".  Add quirks to
> > > the drivers for the devices that have the dangling
> > > PowerResources and manually call _OFF/_ON during suspend/resume?
> > > 
> > > I must admit I haven't looked deeper into the issue yet,
> > > i.e. I don't know which clock causes S0ix failure.
> > > If I understand correctly, before d31fd43c0f9a4 the
> > > clk framework would just disable all unused clocks
> > > registered by clk-pmc-atom.c, which are all of then except
> > > CLK3 which is used by the audio codec. And the audio codec
> > > would disable CLK3 during suspend.
> > > After d31fd43c0f9a4 all clocks that BIOS had enabled during boot
> > > are kept running.  Presumably we could also add the quirk to
> > > clk-pmc-atom.c to bypass what d31fd43c0f9a4 added on Asus E200HA?
> > 
> > Since this is S0ix, the "system level" is 0 all the time (as I said in a
> > previous message).
> 
> Damn.  Reading ACPI spec (6.1 and 6.2), it talks about _LPI and _RDI
> but E200HA doesn't have that in any ACPI table.
> The table in section 7.1 Power Resource Objects and the Power Management Models
> says "Choose a supported device state to enable a targeted system sleep or
> Low- power Idle state ... [use] _PRx".

Yeah.  So power resources should be pointed to by *something* like _PRx.

> > So there seem to be two possible ways to address this, unfortunately both
> > based on white- or blacklisting.
> > 
> > One of them would be to only do the d31fd43c0f9a4 approach for selected
> > systems where it needs to be done to avoid breakage.  The other one would
> > be to do it for all systems by default and blacklist the ones where that
> > leads to problems.
> > 
> > IMO it is better to disable clocks over suspend-resume, because that reduces
> > the system's power draw while suspended.  So that's what I would do and then
> > keep the clocks running only if that is known to be necessary.
> 
> d31fd43c0f9a4 was added to fix a "hard hangup on boot" issue,
> so I guess the default should be to keep clocks enabled.

Well, it still kind of make sense to try to disable them over suspend/resume
if that actually works. :-)

> BTW, in E200HA DSDT, the PowerResources in question are children of
> the I2Cx devices, presumably it means these clocks are to be used by
> devices connected to the i2c busses.  Maybe it's a left over from devices
> that are not present in E200HA, and we should ignore them.

Maybe.  Hard to say.

> In conclusion the BIOS bug is that it enables these clocks at boot,
> and the correct quirk is to make clk-pmc-atom.c disable them during init,
> but leave the _PRx handling for the used clocks (CLK3) to ACPI.
> 
> Right?

Well, depending.  The BIOS may need them for something, we just don't know.

But if the above works, that would be the cleanest way I suppose.

Thanks,
Rafael

Re: S0ix failure due to "clk: x86: Do not gate clocks enabled by the firmware"

[Johannes Stezenbach]

On Sat, Sep 23, 2017 at 12:09:32AM +0200, Rafael J. Wysocki wrote:
> On Friday, September 22, 2017 10:04:53 AM CEST Johannes Stezenbach wrote:
> > In conclusion the BIOS bug is that it enables these clocks at boot,
> > and the correct quirk is to make clk-pmc-atom.c disable them during init,
> > but leave the _PRx handling for the used clocks (CLK3) to ACPI.
> > 
> > Right?
> 
> Well, depending.  The BIOS may need them for something, we just don't know.
> 
> But if the above works, that would be the cleanest way I suppose.

I had a previous quirk patch attached to
https://bugzilla.kernel.org/show_bug.cgi?id=193891
which disables the SATA controller.  I decided to
do the clk quirk on top of it and sending both as RFC.

I test with v4.14-rc2 + sound/topic/dollar-cove-ti-4.13-v5
+ sound/topic/soc-cx2072x-4.13 + your v4 i2c: designware
patches + some more out of tree patches that allow me to enter S0ix.

Thanks,
Johannes

[RFC PATCH 1/2] platform/x86: add Atom PMC quirk to disable SATA

[Johannes Stezenbach]

SATA controller is enabled on Asus E200HA even though the
machine doesn't use it (it has eMMC storage), however
SATA being on blocks S0ix entry so we need to disable it.

Signed-off-by: Johannes Stezenbach <js@sig21.net>
---
 drivers/platform/x86/pmc_atom.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c
index 8d13c86cc418..b5dd38712268 100644
--- a/drivers/platform/x86/pmc_atom.c
+++ b/drivers/platform/x86/pmc_atom.c
@@ -17,6 +17,7 @@
 
 #include <linux/debugfs.h>
 #include <linux/device.h>
+#include <linux/dmi.h>
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/platform_data/x86/clk-pmc-atom.h>
@@ -57,6 +58,9 @@ struct pmc_dev {
 static struct pmc_dev pmc_device;
 static u32 acpi_base_addr;
 
+static u32 quirks;
+#define QUIRK_DISABLE_SATA BIT(0)
+
 static const struct pmc_clk byt_clks[] = {
 	{
 		.name = "xtal",
@@ -271,6 +275,15 @@ static void pmc_hw_reg_setup(struct pmc_dev *pmc)
 	 * - GPIO_SCORE shared IRQ
 	 */
 	pmc_reg_write(pmc, PMC_S0IX_WAKE_EN, (u32)PMC_WAKE_EN_SETTING);
+
+	if (quirks & QUIRK_DISABLE_SATA) {
+		u32 func_dis;
+
+		pr_info("pmc: disable SATA IP\n");
+		func_dis = pmc_reg_read(pmc, PMC_FUNC_DIS);
+		func_dis |= BIT_SATA;
+		pmc_reg_write(pmc, PMC_FUNC_DIS, func_dis);
+	}
 }
 
 #ifdef CONFIG_DEBUG_FS
@@ -500,6 +513,24 @@ static struct pmc_notifier_block pmc_freeze_nb = {
 };
 #endif
 
+static int cht_asus_e200ha_cb(const struct dmi_system_id *id)
+{
+	pr_info("pmc: Asus E200HA detected\n");
+	quirks |= QUIRK_DISABLE_SATA;
+	return 1;
+}
+
+static const struct dmi_system_id cht_table[] = {
+	{
+		.callback = cht_asus_e200ha_cb,
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "E200HA"),
+		},
+	},
+	{ }
+};
+
 static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	struct pmc_dev *pmc = &pmc_device;
@@ -526,6 +557,8 @@ static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	pmc->map = map;
 
+	dmi_check_system(cht_table);
+
 	/* PMC hardware registers setup */
 	pmc_hw_reg_setup(pmc);
 
-- 
2.14.1

Re: [RFC PATCH 1/2] platform/x86: add Atom PMC quirk to disable SATA

[Rafael J. Wysocki]

On Monday, September 25, 2017 9:21:09 PM CET Johannes Stezenbach wrote:
> SATA controller is enabled on Asus E200HA even though the
> machine doesn't use it (it has eMMC storage), however
> SATA being on blocks S0ix entry so we need to disable it.
> 
> Signed-off-by: Johannes Stezenbach <js@sig21.net>

Mika, Andy, Hans, any comments on this one?

> ---
>  drivers/platform/x86/pmc_atom.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c
> index 8d13c86cc418..b5dd38712268 100644
> --- a/drivers/platform/x86/pmc_atom.c
> +++ b/drivers/platform/x86/pmc_atom.c
> @@ -17,6 +17,7 @@
>  
>  #include <linux/debugfs.h>
>  #include <linux/device.h>
> +#include <linux/dmi.h>
>  #include <linux/init.h>
>  #include <linux/io.h>
>  #include <linux/platform_data/x86/clk-pmc-atom.h>
> @@ -57,6 +58,9 @@ struct pmc_dev {
>  static struct pmc_dev pmc_device;
>  static u32 acpi_base_addr;
>  
> +static u32 quirks;
> +#define QUIRK_DISABLE_SATA BIT(0)
> +
>  static const struct pmc_clk byt_clks[] = {
>  	{
>  		.name = "xtal",
> @@ -271,6 +275,15 @@ static void pmc_hw_reg_setup(struct pmc_dev *pmc)
>  	 * - GPIO_SCORE shared IRQ
>  	 */
>  	pmc_reg_write(pmc, PMC_S0IX_WAKE_EN, (u32)PMC_WAKE_EN_SETTING);
> +
> +	if (quirks & QUIRK_DISABLE_SATA) {
> +		u32 func_dis;
> +
> +		pr_info("pmc: disable SATA IP\n");
> +		func_dis = pmc_reg_read(pmc, PMC_FUNC_DIS);
> +		func_dis |= BIT_SATA;
> +		pmc_reg_write(pmc, PMC_FUNC_DIS, func_dis);
> +	}
>  }
>  
>  #ifdef CONFIG_DEBUG_FS
> @@ -500,6 +513,24 @@ static struct pmc_notifier_block pmc_freeze_nb = {
>  };
>  #endif
>  
> +static int cht_asus_e200ha_cb(const struct dmi_system_id *id)
> +{
> +	pr_info("pmc: Asus E200HA detected\n");
> +	quirks |= QUIRK_DISABLE_SATA;
> +	return 1;
> +}
> +
> +static const struct dmi_system_id cht_table[] = {
> +	{
> +		.callback = cht_asus_e200ha_cb,
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "E200HA"),
> +		},
> +	},
> +	{ }
> +};
> +
>  static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent)
>  {
>  	struct pmc_dev *pmc = &pmc_device;
> @@ -526,6 +557,8 @@ static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent)
>  
>  	pmc->map = map;
>  
> +	dmi_check_system(cht_table);
> +
>  	/* PMC hardware registers setup */
>  	pmc_hw_reg_setup(pmc);
>  
> 

Re: [RFC PATCH 1/2] platform/x86: add Atom PMC quirk to disable SATA

[Hans de Goede]

Hi,

On 13-12-17 01:00, Rafael J. Wysocki wrote:
> On Monday, September 25, 2017 9:21:09 PM CET Johannes Stezenbach wrote:
>> SATA controller is enabled on Asus E200HA even though the
>> machine doesn't use it (it has eMMC storage), however
>> SATA being on blocks S0ix entry so we need to disable it.
>>
>> Signed-off-by: Johannes Stezenbach <js@sig21.net>
> 
> Mika, Andy, Hans, any comments on this one?

Seems sensible to me, I'm afraid we may need the same quirk on
other devices, but I see no way around this.

Although, maybe we need to have a specialized (derived)
ahci driver for these Atom SoCs and in there if no
disk is detected do this through the clock framework?

That may be better then a long list of quirks.

Johannes, question how did you test this and figure out
which clocks to disable, a quick howto on this, I think
a patch adding a little howto / README as say
Documentation/power/intel-S0ix-debugging.txt
documenting this would be great. I'm certainly interested
in trying to reproduce this on some of my own Bay Trail and
Cherry Trail devices and add fixes for those if necessary.

Regards,

Hans




> 
>> ---
>>   drivers/platform/x86/pmc_atom.c | 33 +++++++++++++++++++++++++++++++++
>>   1 file changed, 33 insertions(+)
>>
>> diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c
>> index 8d13c86cc418..b5dd38712268 100644
>> --- a/drivers/platform/x86/pmc_atom.c
>> +++ b/drivers/platform/x86/pmc_atom.c
>> @@ -17,6 +17,7 @@
>>   
>>   #include <linux/debugfs.h>
>>   #include <linux/device.h>
>> +#include <linux/dmi.h>
>>   #include <linux/init.h>
>>   #include <linux/io.h>
>>   #include <linux/platform_data/x86/clk-pmc-atom.h>
>> @@ -57,6 +58,9 @@ struct pmc_dev {
>>   static struct pmc_dev pmc_device;
>>   static u32 acpi_base_addr;
>>   
>> +static u32 quirks;
>> +#define QUIRK_DISABLE_SATA BIT(0)
>> +
>>   static const struct pmc_clk byt_clks[] = {
>>   	{
>>   		.name = "xtal",
>> @@ -271,6 +275,15 @@ static void pmc_hw_reg_setup(struct pmc_dev *pmc)
>>   	 * - GPIO_SCORE shared IRQ
>>   	 */
>>   	pmc_reg_write(pmc, PMC_S0IX_WAKE_EN, (u32)PMC_WAKE_EN_SETTING);
>> +
>> +	if (quirks & QUIRK_DISABLE_SATA) {
>> +		u32 func_dis;
>> +
>> +		pr_info("pmc: disable SATA IP\n");
>> +		func_dis = pmc_reg_read(pmc, PMC_FUNC_DIS);
>> +		func_dis |= BIT_SATA;
>> +		pmc_reg_write(pmc, PMC_FUNC_DIS, func_dis);
>> +	}
>>   }
>>   
>>   #ifdef CONFIG_DEBUG_FS
>> @@ -500,6 +513,24 @@ static struct pmc_notifier_block pmc_freeze_nb = {
>>   };
>>   #endif
>>   
>> +static int cht_asus_e200ha_cb(const struct dmi_system_id *id)
>> +{
>> +	pr_info("pmc: Asus E200HA detected\n");
>> +	quirks |= QUIRK_DISABLE_SATA;
>> +	return 1;
>> +}
>> +
>> +static const struct dmi_system_id cht_table[] = {
>> +	{
>> +		.callback = cht_asus_e200ha_cb,
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "E200HA"),
>> +		},
>> +	},
>> +	{ }
>> +};
>> +
>>   static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent)
>>   {
>>   	struct pmc_dev *pmc = &pmc_device;
>> @@ -526,6 +557,8 @@ static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent)
>>   
>>   	pmc->map = map;
>>   
>> +	dmi_check_system(cht_table);
>> +
>>   	/* PMC hardware registers setup */
>>   	pmc_hw_reg_setup(pmc);
>>   
>>
> 
> 

Re: [RFC PATCH 1/2] platform/x86: add Atom PMC quirk to disable SATA

[Johannes Stezenbach]

On Wed, Dec 13, 2017 at 09:53:21AM +0100, Hans de Goede wrote:
> On 13-12-17 01:00, Rafael J. Wysocki wrote:
> > On Monday, September 25, 2017 9:21:09 PM CET Johannes Stezenbach wrote:
> > > SATA controller is enabled on Asus E200HA even though the
> > > machine doesn't use it (it has eMMC storage), however
> > > SATA being on blocks S0ix entry so we need to disable it.
> > > 
> > > Signed-off-by: Johannes Stezenbach <js@sig21.net>
> > 
> > Mika, Andy, Hans, any comments on this one?
> 
> Seems sensible to me, I'm afraid we may need the same quirk on
> other devices, but I see no way around this.
> 
> Although, maybe we need to have a specialized (derived)
> ahci driver for these Atom SoCs and in there if no
> disk is detected do this through the clock framework?
> 
> That may be better then a long list of quirks.
> 
> Johannes, question how did you test this and figure out
> which clocks to disable, a quick howto on this, I think
> a patch adding a little howto / README as say
> Documentation/power/intel-S0ix-debugging.txt
> documenting this would be great. I'm certainly interested
> in trying to reproduce this on some of my own Bay Trail and
> Cherry Trail devices and add fixes for those if necessary.

I put my E200HA aside due to lack of time, so it's
unlikely I'll send documentation patches anytime soon.

Basically everything is documented in bugzilla:
https://bugzilla.kernel.org/show_bug.cgi?id=193891

For the SATA issue I just poked wildly around in registers using
busybox devmem, after applying S0ix blocker debug patch
I tried to disable some devices which were printed:
https://bugzilla.kernel.org/show_bug.cgi?id=193891#c53
(Obviously bug 193891 is misnamed, most of what's
discussed there doesn't directly relate to PMIC. Sorry
for creating a mess, but my understanding of the platform
was very low when I created it.)

The thing is that the public CHT datasheet (atom-z8000-datasheet-vol-1.pdf + vol-2)
doesn't even mention SATA, and there is no PCI device for it.
OTOH, baytrail datasheet (atom-e3800-family-datasheet.pdf)
specifies SATA and BIT_SATA was already defined in pmc_atom.h.

Besides SATA, I also needed to disable dw DMA, using a
hack patch or devmem, but eventually it might be solved properly:
https://bugzilla.kernel.org/show_bug.cgi?id=196861


Thanks,
Johannes

Re: [RFC PATCH 1/2] platform/x86: add Atom PMC quirk to disable SATA

[Michael Turquette]

Hell Hans, all,

On Wed, Dec 13, 2017 at 12:53 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 13-12-17 01:00, Rafael J. Wysocki wrote:
>>
>> On Monday, September 25, 2017 9:21:09 PM CET Johannes Stezenbach wrote:
>>>
>>> SATA controller is enabled on Asus E200HA even though the
>>> machine doesn't use it (it has eMMC storage), however
>>> SATA being on blocks S0ix entry so we need to disable it.
>>>
>>> Signed-off-by: Johannes Stezenbach <js@sig21.net>
>>
>>
>> Mika, Andy, Hans, any comments on this one?
>
>
> Seems sensible to me, I'm afraid we may need the same quirk on
> other devices, but I see no way around this.

Quirks go directly into the driver? Is there a Device Tree analogue
for x86 that could help here?

>
> Although, maybe we need to have a specialized (derived)
> ahci driver for these Atom SoCs and in there if no
> disk is detected do this through the clock framework?

Yes please. x86 is already modeling some clocks properly through the
clock framework. During late init we clean up any clocks that were
enabled out of reset or by the firmware/bootloader but not claimed and
enabled by any Linux driver. That should ideally disable this
particular clock for the case when no SATA drive is present, and
require no quirk logic in the driver.

Regards,
Mike

>
> That may be better then a long list of quirks.
>
> Johannes, question how did you test this and figure out
> which clocks to disable, a quick howto on this, I think
> a patch adding a little howto / README as say
> Documentation/power/intel-S0ix-debugging.txt
> documenting this would be great. I'm certainly interested
> in trying to reproduce this on some of my own Bay Trail and
> Cherry Trail devices and add fixes for those if necessary.
>
> Regards,
>
> Hans
>
>
>
>
>
>>
>>> ---
>>>   drivers/platform/x86/pmc_atom.c | 33 +++++++++++++++++++++++++++++++++
>>>   1 file changed, 33 insertions(+)
>>>
>>> diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c
>>> index 8d13c86cc418..b5dd38712268 100644
>>> --- a/drivers/platform/x86/pmc_atom.c
>>> +++ b/drivers/platform/x86/pmc_atom.c
>>> @@ -17,6 +17,7 @@
>>>     #include <linux/debugfs.h>
>>>   #include <linux/device.h>
>>> +#include <linux/dmi.h>
>>>   #include <linux/init.h>
>>>   #include <linux/io.h>
>>>   #include <linux/platform_data/x86/clk-pmc-atom.h>
>>> @@ -57,6 +58,9 @@ struct pmc_dev {
>>>   static struct pmc_dev pmc_device;
>>>   static u32 acpi_base_addr;
>>>   +static u32 quirks;
>>> +#define QUIRK_DISABLE_SATA BIT(0)
>>> +
>>>   static const struct pmc_clk byt_clks[] = {
>>>         {
>>>                 .name = "xtal",
>>> @@ -271,6 +275,15 @@ static void pmc_hw_reg_setup(struct pmc_dev *pmc)
>>>          * - GPIO_SCORE shared IRQ
>>>          */
>>>         pmc_reg_write(pmc, PMC_S0IX_WAKE_EN, (u32)PMC_WAKE_EN_SETTING);
>>> +
>>> +       if (quirks & QUIRK_DISABLE_SATA) {
>>> +               u32 func_dis;
>>> +
>>> +               pr_info("pmc: disable SATA IP\n");
>>> +               func_dis = pmc_reg_read(pmc, PMC_FUNC_DIS);
>>> +               func_dis |= BIT_SATA;
>>> +               pmc_reg_write(pmc, PMC_FUNC_DIS, func_dis);
>>> +       }
>>>   }
>>>     #ifdef CONFIG_DEBUG_FS
>>> @@ -500,6 +513,24 @@ static struct pmc_notifier_block pmc_freeze_nb = {
>>>   };
>>>   #endif
>>>   +static int cht_asus_e200ha_cb(const struct dmi_system_id *id)
>>> +{
>>> +       pr_info("pmc: Asus E200HA detected\n");
>>> +       quirks |= QUIRK_DISABLE_SATA;
>>> +       return 1;
>>> +}
>>> +
>>> +static const struct dmi_system_id cht_table[] = {
>>> +       {
>>> +               .callback = cht_asus_e200ha_cb,
>>> +               .matches = {
>>> +                       DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
>>> +                       DMI_MATCH(DMI_PRODUCT_NAME, "E200HA"),
>>> +               },
>>> +       },
>>> +       { }
>>> +};
>>> +
>>>   static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent)
>>>   {
>>>         struct pmc_dev *pmc = &pmc_device;
>>> @@ -526,6 +557,8 @@ static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent)
>>>         pmc->map = map;
>>>   +     dmi_check_system(cht_table);
>>> +
>>>         /* PMC hardware registers setup */
>>>         pmc_hw_reg_setup(pmc);
>>>
>>
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/2] platform/x86: add Atom PMC quirk to disable SATA

[Hans de Goede]

Hi,

On 13-12-17 16:25, Michael Turquette wrote:
> Hell Hans, all,
> 
> On Wed, Dec 13, 2017 at 12:53 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 13-12-17 01:00, Rafael J. Wysocki wrote:
>>>
>>> On Monday, September 25, 2017 9:21:09 PM CET Johannes Stezenbach wrote:
>>>>
>>>> SATA controller is enabled on Asus E200HA even though the
>>>> machine doesn't use it (it has eMMC storage), however
>>>> SATA being on blocks S0ix entry so we need to disable it.
>>>>
>>>> Signed-off-by: Johannes Stezenbach <js@sig21.net>
>>>
>>>
>>> Mika, Andy, Hans, any comments on this one?
>>
>>
>> Seems sensible to me, I'm afraid we may need the same quirk on
>> other devices, but I see no way around this.
> 
> Quirks go directly into the driver? Is there a Device Tree analogue
> for x86 that could help here?

No.

>> Although, maybe we need to have a specialized (derived)
>> ahci driver for these Atom SoCs and in there if no
>> disk is detected do this through the clock framework?
> 
> Yes please. x86 is already modeling some clocks properly through the
> clock framework. During late init we clean up any clocks that were
> enabled out of reset or by the firmware/bootloader but not claimed and
> enabled by any Linux driver. That should ideally disable this
> particular clock for the case when no SATA drive is present, and
> require no quirk logic in the driver.

Ah so you're thinking a special ahci driver which knows about
the clock, yes I think that could work.

Or maybe do a match on the CPU model and if it is know to
not have SATA (or not routed to the outside), disable
the clock? That seems better because if I understood Johannes
correctly there is no SATA/AHCI PCI device (so nothing for
a driver to bind to) but the clock is still enabled, although
in that case the clock framework should do the right thing
if we revert commit d31fd43c0f9a "clk: x86: Do not gate clocks enabled by the firmware"

Regards,

Hans



> 
> Regards,
> Mike
> 
>>
>> That may be better then a long list of quirks.
>>
>> Johannes, question how did you test this and figure out
>> which clocks to disable, a quick howto on this, I think
>> a patch adding a little howto / README as say
>> Documentation/power/intel-S0ix-debugging.txt
>> documenting this would be great. I'm certainly interested
>> in trying to reproduce this on some of my own Bay Trail and
>> Cherry Trail devices and add fixes for those if necessary.
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>
>>
>>>
>>>> ---
>>>>    drivers/platform/x86/pmc_atom.c | 33 +++++++++++++++++++++++++++++++++
>>>>    1 file changed, 33 insertions(+)
>>>>
>>>> diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c
>>>> index 8d13c86cc418..b5dd38712268 100644
>>>> --- a/drivers/platform/x86/pmc_atom.c
>>>> +++ b/drivers/platform/x86/pmc_atom.c
>>>> @@ -17,6 +17,7 @@
>>>>      #include <linux/debugfs.h>
>>>>    #include <linux/device.h>
>>>> +#include <linux/dmi.h>
>>>>    #include <linux/init.h>
>>>>    #include <linux/io.h>
>>>>    #include <linux/platform_data/x86/clk-pmc-atom.h>
>>>> @@ -57,6 +58,9 @@ struct pmc_dev {
>>>>    static struct pmc_dev pmc_device;
>>>>    static u32 acpi_base_addr;
>>>>    +static u32 quirks;
>>>> +#define QUIRK_DISABLE_SATA BIT(0)
>>>> +
>>>>    static const struct pmc_clk byt_clks[] = {
>>>>          {
>>>>                  .name = "xtal",
>>>> @@ -271,6 +275,15 @@ static void pmc_hw_reg_setup(struct pmc_dev *pmc)
>>>>           * - GPIO_SCORE shared IRQ
>>>>           */
>>>>          pmc_reg_write(pmc, PMC_S0IX_WAKE_EN, (u32)PMC_WAKE_EN_SETTING);
>>>> +
>>>> +       if (quirks & QUIRK_DISABLE_SATA) {
>>>> +               u32 func_dis;
>>>> +
>>>> +               pr_info("pmc: disable SATA IP\n");
>>>> +               func_dis = pmc_reg_read(pmc, PMC_FUNC_DIS);
>>>> +               func_dis |= BIT_SATA;
>>>> +               pmc_reg_write(pmc, PMC_FUNC_DIS, func_dis);
>>>> +       }
>>>>    }
>>>>      #ifdef CONFIG_DEBUG_FS
>>>> @@ -500,6 +513,24 @@ static struct pmc_notifier_block pmc_freeze_nb = {
>>>>    };
>>>>    #endif
>>>>    +static int cht_asus_e200ha_cb(const struct dmi_system_id *id)
>>>> +{
>>>> +       pr_info("pmc: Asus E200HA detected\n");
>>>> +       quirks |= QUIRK_DISABLE_SATA;
>>>> +       return 1;
>>>> +}
>>>> +
>>>> +static const struct dmi_system_id cht_table[] = {
>>>> +       {
>>>> +               .callback = cht_asus_e200ha_cb,
>>>> +               .matches = {
>>>> +                       DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
>>>> +                       DMI_MATCH(DMI_PRODUCT_NAME, "E200HA"),
>>>> +               },
>>>> +       },
>>>> +       { }
>>>> +};
>>>> +
>>>>    static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent)
>>>>    {
>>>>          struct pmc_dev *pmc = &pmc_device;
>>>> @@ -526,6 +557,8 @@ static int pmc_setup_dev(struct pci_dev *pdev, const struct pci_device_id *ent)
>>>>          pmc->map = map;
>>>>    +     dmi_check_system(cht_table);
>>>> +
>>>>          /* PMC hardware registers setup */
>>>>          pmc_hw_reg_setup(pmc);
>>>>
>>>
>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/2] platform/x86: add Atom PMC quirk to disable SATA

[Johannes Stezenbach]

Hi,

On Wed, Dec 13, 2017 at 05:04:34PM +0100, Hans de Goede wrote:
> On 13-12-17 16:25, Michael Turquette wrote:
> > On Wed, Dec 13, 2017 at 12:53 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> > > Although, maybe we need to have a specialized (derived)
> > > ahci driver for these Atom SoCs and in there if no
> > > disk is detected do this through the clock framework?
> > 
> > Yes please. x86 is already modeling some clocks properly through the
> > clock framework. During late init we clean up any clocks that were
> > enabled out of reset or by the firmware/bootloader but not claimed and
> > enabled by any Linux driver. That should ideally disable this
> > particular clock for the case when no SATA drive is present, and
> > require no quirk logic in the driver.
> 
> Ah so you're thinking a special ahci driver which knows about
> the clock, yes I think that could work.
> 
> Or maybe do a match on the CPU model and if it is know to
> not have SATA (or not routed to the outside), disable
> the clock? That seems better because if I understood Johannes
> correctly there is no SATA/AHCI PCI device (so nothing for
> a driver to bind to) but the clock is still enabled, although
> in that case the clock framework should do the right thing
> if we revert commit d31fd43c0f9a "clk: x86: Do not gate clocks enabled by the firmware"

Please don't get confused with the other thread about clocks.
This issue sets the "disable IP" bit, found by doing stupid
experiments to enable S0ix on E200HA.

1. no idea if Cherry Trail even has SATA IP, maybe this is a
   meaningless bit but PMC firmware carried over from
   Bay Trail looks at it

2. BIOS should have set the bit, so it is a BIOS quirk

3. or maybe there is a much better solution that I don't know about

https://bugzilla.kernel.org/show_bug.cgi?id=193891
also has lspci output


Thanks,
Johannes

Re: [RFC PATCH 1/2] platform/x86: add Atom PMC quirk to disable SATA

[Hans de Goede]

Hi,

On 13-12-17 17:22, Johannes Stezenbach wrote:
> Hi,
> 
> On Wed, Dec 13, 2017 at 05:04:34PM +0100, Hans de Goede wrote:
>> On 13-12-17 16:25, Michael Turquette wrote:
>>> On Wed, Dec 13, 2017 at 12:53 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>> Although, maybe we need to have a specialized (derived)
>>>> ahci driver for these Atom SoCs and in there if no
>>>> disk is detected do this through the clock framework?
>>>
>>> Yes please. x86 is already modeling some clocks properly through the
>>> clock framework. During late init we clean up any clocks that were
>>> enabled out of reset or by the firmware/bootloader but not claimed and
>>> enabled by any Linux driver. That should ideally disable this
>>> particular clock for the case when no SATA drive is present, and
>>> require no quirk logic in the driver.
>>
>> Ah so you're thinking a special ahci driver which knows about
>> the clock, yes I think that could work.
>>
>> Or maybe do a match on the CPU model and if it is know to
>> not have SATA (or not routed to the outside), disable
>> the clock? That seems better because if I understood Johannes
>> correctly there is no SATA/AHCI PCI device (so nothing for
>> a driver to bind to) but the clock is still enabled, although
>> in that case the clock framework should do the right thing
>> if we revert commit d31fd43c0f9a "clk: x86: Do not gate clocks enabled by the firmware"
> 
> Please don't get confused with the other thread about clocks.
> This issue sets the "disable IP" bit, found by doing stupid
> experiments to enable S0ix on E200HA.

Ah my bad.

> 1. no idea if Cherry Trail even has SATA IP, maybe this is a
>     meaningless bit but PMC firmware carried over from
>     Bay Trail looks at it


There are no CHT SoCs with SATA AFAIK, but Braswell SoCs,
which I believe is the same die do have SATA.

I think the best fix here is to look at the model-string part
of the CPU-id and do a quirk based on that, setting the "disable IP"
bit for the SATA on all SoC models known to not have SATA
(Z8300, Z8350, Z8500, Z8550, Z8700, Z8750).

Rafael, Andy how does that sound as a solution?

> 2. BIOS should have set the bit, so it is a BIOS quirk
> 
> 3. or maybe there is a much better solution that I don't know about
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=193891
> also has lspci output

Regards,

Hans

Re: [RFC PATCH 1/2] platform/x86: add Atom PMC quirk to disable SATA

[Andy Shevchenko]

On Wed, 2017-12-13 at 17:37 +0100, Hans de Goede wrote:
> Hi,
> 
> On 13-12-17 17:22, Johannes Stezenbach wrote:
> > 


> > Please don't get confused with the other thread about clocks.
> > This issue sets the "disable IP" bit, found by doing stupid
> > experiments to enable S0ix on E200HA.
> 
> Ah my bad.

Oh, perhaps I also need to refresh my memory from that buglink.

> > 1. no idea if Cherry Trail even has SATA IP, maybe this is a
> >     meaningless bit but PMC firmware carried over from
> >     Bay Trail looks at it
> 
> 
> There are no CHT SoCs with SATA AFAIK, but Braswell SoCs,
> which I believe is the same die do have SATA.
> 
> I think the best fix here is to look at the model-string part
> of the CPU-id and do a quirk based on that, setting the "disable IP"
> bit for the SATA on all SoC models known to not have SATA
> (Z8300, Z8350, Z8500, Z8550, Z8700, Z8750).
> 
> Rafael, Andy how does that sound as a solution?

Yeah, that bit is a property of PMC microcontroller and thus belongs to
its driver in Linux kernel.

To make it strict we need a matching property. AFAIR CPU model ID is all
the same for all CHT and BSW SoCs, so, can't be used to distinguish
them. So, you are thinking about comparing CPU model name then?

It might work. However, SATA itself is a part of PCH, and thus can not
exactly be matched by CPU ID.

Btw, Pentium Celeron N-series according to spec has SATA host.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [RFC PATCH 1/2] platform/x86: add Atom PMC quirk to disable SATA

[Hans de Goede]

Hi,

On 13-12-17 20:33, Andy Shevchenko wrote:
> On Wed, 2017-12-13 at 17:37 +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 13-12-17 17:22, Johannes Stezenbach wrote:
>>>
> 
> 
>>> Please don't get confused with the other thread about clocks.
>>> This issue sets the "disable IP" bit, found by doing stupid
>>> experiments to enable S0ix on E200HA.
>>
>> Ah my bad.
> 
> Oh, perhaps I also need to refresh my memory from that buglink.
> 
>>> 1. no idea if Cherry Trail even has SATA IP, maybe this is a
>>>      meaningless bit but PMC firmware carried over from
>>>      Bay Trail looks at it
>>
>>
>> There are no CHT SoCs with SATA AFAIK, but Braswell SoCs,
>> which I believe is the same die do have SATA.
>>
>> I think the best fix here is to look at the model-string part
>> of the CPU-id and do a quirk based on that, setting the "disable IP"
>> bit for the SATA on all SoC models known to not have SATA
>> (Z8300, Z8350, Z8500, Z8550, Z8700, Z8750).
>>
>> Rafael, Andy how does that sound as a solution?
> 
> Yeah, that bit is a property of PMC microcontroller and thus belongs to
> its driver in Linux kernel.
> 
> To make it strict we need a matching property. AFAIR CPU model ID is all
> the same for all CHT and BSW SoCs, so, can't be used to distinguish
> them. So, you are thinking about comparing CPU model name then?

Yes CPU model name.

> It might work. However, SATA itself is a part of PCH, and thus can not
> exactly be matched by CPU ID.

AFAIK the PCH is integrated, at least with the Z83xx Z85xx and X87xx
models.

So using a CPU model name match seems a better solution to me
then DMI product-name matching, as the CPU model name match should
fix this for all devices with such a SoC.

> Btw, Pentium Celeron N-series according to spec has SATA host.

Right as I said the Braswell models do have SATA.

Regards,

Hans

[RFC PATCH 2/2] clk: x86: Disable unused clocks to fix S0ix

[Johannes Stezenbach]

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(-)

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;
 };
-- 
2.14.1

Re: [RFC PATCH 2/2] clk: x86: Disable unused clocks to fix S0ix

[Rafael J. Wysocki]

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;
>  };
> 

Re: [RFC PATCH 2/2] clk: x86: Disable unused clocks to fix S0ix

[Hans de Goede]

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;
>>   };
>>
> 
> 

Re: [RFC PATCH 2/2] clk: x86: Disable unused clocks to fix S0ix

[Carlo Caione]

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,

-- 
Carlo Caione  |  +39.340.80.30.096  |  Endless

Re: [RFC PATCH 2/2] clk: x86: Disable unused clocks to fix S0ix

[Johannes Stezenbach]

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

Re: [RFC PATCH 2/2] clk: x86: Disable unused clocks to fix S0ix

[Pierre-Louis Bossart]

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

Re: [RFC PATCH 2/2] clk: x86: Disable unused clocks to fix S0ix

[Andy Shevchenko]

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)

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy