Date:   Wed, 29 Aug 2018 19:06:09 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Irina Tirdea <irina.tirdea@...el.com>, netdev@...r.kernel.org,
        Johannes Stezenbach <js@...21.net>,
        Carlo Caione <carlo@...lessm.com>, linux-clk@...r.kernel.org
Subject: Re: [PATCH 0/4] clk-pmc-atom + r8169: Add ether_clk handling to fix
 suspend issues

Hi,

On 29-08-18 18:31, Andy Shevchenko wrote:
> On Mon, Aug 27, 2018 at 04:31:56PM +0200, Hans de Goede wrote:
>> Hi All,
>>
>> This series has as goal to revert commit d31fd43c0f9a ("clk: x86: Do not gate
>> clocks enabled by the firmware"), because that commit causes almost all
>> Cherry Trail devices to not use the S0i3 powerstate when suspending, leading
>> to them quickly draining their battery when suspended.
>>
>> This commit was added to fix issues with the r8169 NIC on some Bay Trail
>> devices, where the pmc_plt_clk_4 was used as clk for the r8169 chip.
>>
>> This series fixes this properly, so that we can undo the trouble some
>> commit.
>>
>> The first patch adds an "ether_clk" alias to the clk-pmc-atom driver so
>> that the r8169 can pass "ether_clk" as generic id to clk_get instead of
>> having to add x86 specific code to the r8169 driver.
>>
>> The second patch makes the r8169 driver do a clk_get for "ether_clk"
>> (ignoring -ENOENT errors so this is optional) and if that succeeds then
>> it enables the clock.
>>
>> The third patch effectively revert the troublesome commit.
>>
>> This series has been tested on a HP Stream x360 - 11-p000nd, which uses
>> a Bay Trail SoC with a r8169 ethernet chip and I can confirm that the
>> pmc_plt_clk_4 gets properly enabled, so this series should not cause any
>> regressions wrt devices needing pmc_plt_clk_4 enabled (which is not the
>> case on the Stream x360).
>>
>> To avoid regressions we need to have patches 1 and 2 merged before 3,
>> so it is probably best if this entire series gets merged through a single
>> tree. Given that clk-pmc-atom.c has not seen any changes for over a
>> year I suggest that all 3 patches are merged through the netdev tree,
>> with an ack from the clk maintainers. Assuming that is ok with the clk
>> maintainers of course.
>>
>> Note there is a fourth patch in this series, this patch is necessary to
>> reach S0i3 state on Bay Trail devices wich have a r8169 ethernet chip,
>> since I do not have access to hardware where the r8169 actually needs
>> the pmc_plt_clk_4 I have not been able to test this, hence it is marked
>> as RFC for now.
>>
> 
> What a nice stuff, thanks!
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>

Thank you (and also thank you for the other reviews)

> Btw, you probably better to refer to
> https://bugzilla.kernel.org/show_bug.cgi?id=196861 which is dedicated for S0ix
> issue.

Ok, I've added a link to that. I've also added:

Reported-by: Johannes Stezenbach <js@...21.net>

To honor Johannes for figuring out that leaving the clocks enabled
was a problem in the first place.

This will all be included in v2 of the series when I send it out.

> Another thing, clk_prepare_enable() can fail, I dunno what you can do at
> ->resume() stage with it failed.

Right, I know that it can fail, but in practice it never fails unless
things are seriously foo-barred already and there is not much we can
do when it fails, so I decided to just ignore the return code.

Regards,

Hans

Powered by blists - more mailing lists