clk: x86: Stop marking clocks as CLK_IS_CRITICAL is causing non-working hardware.

clk: x86: Stop marking clocks as CLK_IS_CRITICAL is causing non-working hardware.

[David Müller]

Hello

I have a x86 (Bay Trail) based hardware, where pmc_plt_clk0 is used to
drive some external hardware (USB HSIC chip attached to the SoC's xHCI
root port).

Since commit 88659387b9d509ee4bb8f0b0fe9ce2ff00988b46, the pmc_plt_clk0
is unconditionally gated off, resulting in a non-working HSIC chip and
all the USB devices behind it.

What is the proper way to fix this regression:
- revert this commit?
- implement some kind of "quirk" in the clk-pmc-atom driver?
- ???


Thanks

Re: clk: x86: Stop marking clocks as CLK_IS_CRITICAL is causing non-working hardware.

[David Müller]

Stephen Boyd wrote:

> Quoting David Müller (2019-03-14 03:18:59)
>> Since commit 88659387b9d509ee4bb8f0b0fe9ce2ff00988b46, the pmc_plt_clk0
>
> Is this a stable tree commit? It's not in Linus' tree.

Yes, the upstream commit is 648e921888ad96ea3dc922739e96716ad3225d7f

Re: clk: x86: Stop marking clocks as CLK_IS_CRITICAL is causing non-working hardware.

[Hans de Goede]

Hi,

On 14-03-19 16:47, Stephen Boyd wrote:
> Quoting David Müller (2019-03-14 03:18:59)
>> Hello
>>
>> I have a x86 (Bay Trail) based hardware, where pmc_plt_clk0 is used to
>> drive some external hardware (USB HSIC chip attached to the SoC's xHCI
>> root port).
>>
>> Since commit 88659387b9d509ee4bb8f0b0fe9ce2ff00988b46, the pmc_plt_clk0
> 
> Is this a stable tree commit? It's not in Linus' tree.

Likely, I believe the mainline commit being talked about is:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=648e921888ad96ea3dc922739e96716ad3225d7f

>> is unconditionally gated off, resulting in a non-working HSIC chip and
>> all the USB devices behind it.
>>
>> What is the proper way to fix this regression:
>> - revert this commit?
>> - implement some kind of "quirk" in the clk-pmc-atom driver?

So the background here is that the ACPI tables would normally be
responsible for turning on/off the pmc clocks and they are on cherry-trail
but on bay-trail the OS is somehow supposed to figure this all out.

The commit which is causing this problem for you is in essence a revert
of commit d31fd43c0f9a ("clk: x86: Do not gate clocks enabled by the firmware"),
which was an (in hindsight) ill advised fix to fix a similar problem with
a NIC. Since that commit broke entering deep sleep states during suspend
on all Bay Trail devices, I first upstreamed another fix for the NIC:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b1e3454d39f992e5409cd19f97782185950df6e7
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c2f6f3ee7f22521fabc3295f51149bc3f4dd9202

And then I submitted the "clk: x86: Stop marking clocks as CLK_IS_CRITICAL"
which in hindsight I should have submitted as a revert of d31fd43c0f9a,
rather then as a new patch to make sure that this patch was unbreaking things.

TL;DR: we do not want to revert "clk: x86: Stop marking clocks as CLK_IS_CRITICAL"
as that will regress power-consumption during suspend on many devices.

So you will need to do something similar to the 2 commits I did for the r8169.c
problem, but then limited to your board/model since we do not want to be enabling
pmc_plt_clk0 on every Bay Trail machine.

Do you know if the kernel already has some special code-paths to deal with the HSIC
phy in the xhci code ? Then we could just get and enable the clock there.

Otherwise I guess we should probably conditionally register a platform_usb_phy_clk
clock alias (see commit b1e3454d39f992) based on a dmi_system_id table and have
the xhci driver (optionally) request this clock and enable it if present. There are
some interesting probe-ordering issues here though, for this to work the
clk-pmc-atom (builtin) platform driver must bind to the clk-pmc-atom platform-device
*before* the xhci driver tries to get the clock.

Regards,

Hans

Re: clk: x86: Stop marking clocks as CLK_IS_CRITICAL is causing non-working hardware.

[David Müller]

Hello

Hans de Goede wrote:

> So the background here is that the ACPI tables would normally be
> responsible for turning on/off the pmc clocks and they are on cherry-trail
> but on bay-trail the OS is somehow supposed to figure this all out.

Sorry, I'm not familiar with CherryTrail but the "clk-pmc-atom" clk
driver is used by both BayTrail and CherryTrail, isn't it?
I don't see any ACPI related clock handling in this driver. Is it
handled somewhere else?

> Do you know if the kernel already has some special code-paths to deal
> with the HSIC phy in the xhci code ?

The HSIC chip is more an USB hub than a phy. There is also no specific
driver loaded/needed for HSIC to work, at least not on x86.
I have also not seen any "external clock enabling code-paths" in the
xHCI code, but maybe I overlooked it.

> Otherwise I guess we should probably conditionally register a
> platform_usb_phy_clk clock alias (see commit b1e3454d39f992)
> based on a dmi_system_id table and have the xhci driver (optionally)
> request this clock and enable it if present.
> There are some interesting probe-ordering issues here though,
> for this to work the clk-pmc-atom (builtin) platform driver must
> bind to the clk-pmc-atom platform-device *before* the xhci
> driver tries to get the clock.

Wouldn't it be simpler and more straightforward to put a "dmi_system_id
table" based quirk into the clk-pmc-atom driver itself?
That would also avoid any potential probe-ordering issues.

Re: clk: x86: Stop marking clocks as CLK_IS_CRITICAL is causing non-working hardware.

[Hans de Goede]

Hi,

On 19-03-19 14:16, David Müller wrote:
> Hello
> 
> Hans de Goede wrote:
> 
>> So the background here is that the ACPI tables would normally be
>> responsible for turning on/off the pmc clocks and they are on cherry-trail
>> but on bay-trail the OS is somehow supposed to figure this all out.
> 
> Sorry, I'm not familiar with CherryTrail but the "clk-pmc-atom" clk
> driver is used by both BayTrail and CherryTrail, isn't it?
> I don't see any ACPI related clock handling in this driver. Is it
> handled somewhere else?
> 
>> Do you know if the kernel already has some special code-paths to deal
>> with the HSIC phy in the xhci code ?
> 
> The HSIC chip is more an USB hub than a phy. There is also no specific
> driver loaded/needed for HSIC to work, at least not on x86.
> I have also not seen any "external clock enabling code-paths" in the
> xHCI code, but maybe I overlooked it.
> 
>> Otherwise I guess we should probably conditionally register a
>> platform_usb_phy_clk clock alias (see commit b1e3454d39f992)
>> based on a dmi_system_id table and have the xhci driver (optionally)
>> request this clock and enable it if present.
>> There are some interesting probe-ordering issues here though,
>> for this to work the clk-pmc-atom (builtin) platform driver must
>> bind to the clk-pmc-atom platform-device *before* the xhci
>> driver tries to get the clock.
> 
> Wouldn't it be simpler and more straightforward to put a "dmi_system_id
> table" based quirk into the clk-pmc-atom driver itself?
> That would also avoid any potential probe-ordering issues.

That is possible, I see 2 downsides to that approach:

1) I do not think a quirk for an USB HUB belongs in a generic clk
driver, but if Stephen is ok with this I'm fine with this approach
since it certainly seems the simplest solution.

Stephen ?

2) If we just restore the CLK_IS_CRITICAL flag for pmc_clk0 on the
device in question, based on a dmi check, then that means the
device in question will not be able to reach S0i3 when suspended
and thus use more power while suspended, this is certainly better
then having non working USB ports, so if you are ok with this
downside (so in essence going back to the old situation as a
revert would do), then that is fine with me.

Regards,

Hans