[PATCH 0/4] clk-pmc-atom + r8169: Add ether_clk handling to fix suspend issues

[PATCH 0/4] clk-pmc-atom + r8169: Add ether_clk handling to fix suspend issues

[Hans de Goede]

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.

Carlos can you test the 4th patch (when you have time) and let us know if
it works?

Regards,

Hans

[PATCH 1/4] clk: x86: add "ether_clk" alias for Bay Trail / Cherry Trail

[Hans de Goede]

Commit d31fd43c0f9a ("clk: x86: Do not gate clocks enabled by the
firmware") causes all unclaimed PMC clocks on Cherry Trail devices to be on
all the time, resulting on the device not being able to reach S0i2 or S0i3
when suspended.

The reason for this commit is that on some Bay Trail / Cherry Trail devices
the ethernet controller uses pmc_plt_clk_4. This commit adds an "ether_clk"
alias, so that the relevant ethernet drivers can try to (optionally) use
this, without needing X86 specific code / hacks, thus fixing ethernet on
these devices without breaking S0i3 support.

This commit uses clkdev_hw_create() to create the alias, mirroring the code
for the already existing "mclk" alias for pmc_plt_clk_3.

Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=193891#c102
Cc: Johannes Stezenbach <js@sig21.net>
Cc: Carlo Caione <carlo@endlessm.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/clk/x86/clk-pmc-atom.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/clk/x86/clk-pmc-atom.c b/drivers/clk/x86/clk-pmc-atom.c
index 08ef69945ffb..75151901ff7d 100644
--- a/drivers/clk/x86/clk-pmc-atom.c
+++ b/drivers/clk/x86/clk-pmc-atom.c
@@ -55,6 +55,7 @@ struct clk_plt_data {
 	u8 nparents;
 	struct clk_plt *clks[PMC_CLK_NUM];
 	struct clk_lookup *mclk_lookup;
+	struct clk_lookup *ether_clk_lookup;
 };
 
 /* Return an index in parent table */
@@ -351,11 +352,20 @@ static int plt_clk_probe(struct platform_device *pdev)
 		goto err_unreg_clk_plt;
 	}
 
+	data->ether_clk_lookup = clkdev_hw_create(&data->clks[4]->hw,
+						  "ether_clk", NULL);
+	if (!data->ether_clk_lookup) {
+		err = -ENOMEM;
+		goto err_drop_mclk;
+	}
+
 	plt_clk_free_parent_names_loop(parent_names, data->nparents);
 
 	platform_set_drvdata(pdev, data);
 	return 0;
 
+err_drop_mclk:
+	clkdev_drop(data->mclk_lookup);
 err_unreg_clk_plt:
 	plt_clk_unregister_loop(data, i);
 	plt_clk_unregister_parents(data);
@@ -369,6 +379,7 @@ static int plt_clk_remove(struct platform_device *pdev)
 
 	data = platform_get_drvdata(pdev);
 
+	clkdev_drop(data->ether_clk_lookup);
 	clkdev_drop(data->mclk_lookup);
 	plt_clk_unregister_loop(data, PMC_CLK_NUM);
 	plt_clk_unregister_parents(data);
-- 
2.18.0

Re: [PATCH 1/4] clk: x86: add "ether_clk" alias for Bay Trail / Cherry Trail

[Stephen Boyd]

Quoting Hans de Goede (2018-08-27 07:31:57)
> Commit d31fd43c0f9a ("clk: x86: Do not gate clocks enabled by the
> firmware") causes all unclaimed PMC clocks on Cherry Trail devices to be =
on
> all the time, resulting on the device not being able to reach S0i2 or S0i3
> when suspended.
> =

> The reason for this commit is that on some Bay Trail / Cherry Trail devic=
es
> the ethernet controller uses pmc_plt_clk_4. This commit adds an "ether_cl=
k"
> alias, so that the relevant ethernet drivers can try to (optionally) use
> this, without needing X86 specific code / hacks, thus fixing ethernet on
> these devices without breaking S0i3 support.
> =

> This commit uses clkdev_hw_create() to create the alias, mirroring the co=
de
> for the already existing "mclk" alias for pmc_plt_clk_3.
> =

> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=3D193891#c102
> Cc: Johannes Stezenbach <js@sig21.net>
> Cc: Carlo Caione <carlo@endlessm.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---

Acked-by: Stephen Boyd <sboyd@kernel.org>

[PATCH 2/4] r8169: Get and enable optional ether_clk clock

[Hans de Goede]

On some boards a platform clock is used as clock for the r8169 chip,
this commit adds support for getting and enabling this clock (assuming
it has an "ether_clk" alias set on it).

This is related to commit d31fd43c0f9a ("clk: x86: Do not gate clocks
enabled by the firmware") which is a previous attempt to fix this for some
x86 boards, but this causes all Cherry Trail SoC using boards to not reach
there lowest power states when suspending.

This commit (together with an atom-pmc-clk driver commit adding the alias)
fixes things properly by making the r8169 get the clock and enable it when
it needs it.

Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=193891#c102
Cc: Johannes Stezenbach <js@sig21.net>
Cc: Carlo Caione <carlo@endlessm.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/net/ethernet/realtek/r8169.c | 33 ++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index eaedc11ed686..779b02979493 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -13,6 +13,7 @@
 #include <linux/pci.h>
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
+#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/ethtool.h>
 #include <linux/mii.h>
@@ -765,6 +766,7 @@ struct rtl8169_private {
 
 	u16 event_slow;
 	const struct rtl_coalesce_info *coalesce_info;
+	struct clk *clk;
 
 	struct mdio_ops {
 		void (*write)(struct rtl8169_private *, int, int);
@@ -7614,6 +7616,11 @@ static void rtl_hw_initialize(struct rtl8169_private *tp)
 	}
 }
 
+static void rtl_disable_clk(void *data)
+{
+	clk_disable_unprepare(data);
+}
+
 static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	const struct rtl_cfg_info *cfg = rtl_cfg_infos + ent->driver_data;
@@ -7647,6 +7654,32 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	mii->reg_num_mask = 0x1f;
 	mii->supports_gmii = cfg->has_gmii;
 
+	/* Get the *optional* external "ether_clk" used on some boards */
+	tp->clk = devm_clk_get(&pdev->dev, "ether_clk");
+	if (IS_ERR(tp->clk)) {
+		rc = PTR_ERR(tp->clk);
+		if (rc == -ENOENT) {
+			/* clk-core allows NULL (for suspend / resume) */
+			tp->clk = NULL;
+		} else if (rc == -EPROBE_DEFER) {
+			return rc;
+		} else {
+			dev_err(&pdev->dev, "failed to get clk: %d\n", rc);
+			return rc;
+		}
+	} else {
+		rc = clk_prepare_enable(tp->clk);
+		if (rc) {
+			dev_err(&pdev->dev, "failed to enable clk: %d\n", rc);
+			return rc;
+		}
+
+		rc = devm_add_action_or_reset(&pdev->dev, rtl_disable_clk,
+					      tp->clk);
+		if (rc)
+			return rc;
+	}
+
 	/* disable ASPM completely as that cause random device stop working
 	 * problems as well as full system hangs for some PCIe devices users */
 	pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1 |
-- 
2.18.0

Re: [PATCH 2/4] r8169: Get and enable optional ether_clk clock

[Stephen Boyd]

Quoting Hans de Goede (2018-08-27 07:31:58)
> On some boards a platform clock is used as clock for the r8169 chip,
> this commit adds support for getting and enabling this clock (assuming
> it has an "ether_clk" alias set on it).
> =

> This is related to commit d31fd43c0f9a ("clk: x86: Do not gate clocks
> enabled by the firmware") which is a previous attempt to fix this for some
> x86 boards, but this causes all Cherry Trail SoC using boards to not reach
> there lowest power states when suspending.
> =

> This commit (together with an atom-pmc-clk driver commit adding the alias)
> fixes things properly by making the r8169 get the clock and enable it when
> it needs it.
> =

> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=3D193891#c102
> Cc: Johannes Stezenbach <js@sig21.net>
> Cc: Carlo Caione <carlo@endlessm.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Stephen Boyd <sboyd@kernel.org>

> @@ -7614,6 +7616,11 @@ static void rtl_hw_initialize(struct rtl8169_priva=
te *tp)
>         }
>  }
>  =

> +static void rtl_disable_clk(void *data)
> +{
> +       clk_disable_unprepare(data);
> +}
> +
>  static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id=
 *ent)
>  {
>         const struct rtl_cfg_info *cfg =3D rtl_cfg_infos + ent->driver_da=
ta;
> @@ -7647,6 +7654,32 @@ static int rtl_init_one(struct pci_dev *pdev, cons=
t struct pci_device_id *ent)
>         mii->reg_num_mask =3D 0x1f;
>         mii->supports_gmii =3D cfg->has_gmii;
>  =

> +       /* Get the *optional* external "ether_clk" used on some boards */
> +       tp->clk =3D devm_clk_get(&pdev->dev, "ether_clk");

An "optional" clk API is in flight, but it's easier to wait for that to
merge and then this to be updated, so just take that as an FYI. It would
be interesting to see how to support optional clks with clkdev lookups
though, which would be needed in this case.

How would you know that a clk device driver hasn't probed yet and isn't
the driver that's actually providing the clk to this device on x86
systems? With DT systems we can figure that out by looking at the DT and
seeing if the device driver requesting the clk has the clocks property.
On x86 systems it's all clkdev which doesn't really lend itself to
solving this problem.

> +       if (IS_ERR(tp->clk)) {
> +               rc =3D PTR_ERR(tp->clk);

Re: [PATCH 2/4] r8169: Get and enable optional ether_clk clock

[Hans de Goede]

Hi,

On 27-08-18 20:47, Stephen Boyd wrote:
> Quoting Hans de Goede (2018-08-27 07:31:58)
>> On some boards a platform clock is used as clock for the r8169 chip,
>> this commit adds support for getting and enabling this clock (assuming
>> it has an "ether_clk" alias set on it).
>>
>> This is related to commit d31fd43c0f9a ("clk: x86: Do not gate clocks
>> enabled by the firmware") which is a previous attempt to fix this for some
>> x86 boards, but this causes all Cherry Trail SoC using boards to not reach
>> there lowest power states when suspending.
>>
>> This commit (together with an atom-pmc-clk driver commit adding the alias)
>> fixes things properly by making the r8169 get the clock and enable it when
>> it needs it.
>>
>> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=193891#c102
>> Cc: Johannes Stezenbach <js@sig21.net>
>> Cc: Carlo Caione <carlo@endlessm.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Acked-by: Stephen Boyd <sboyd@kernel.org>

Thanks.

>> @@ -7614,6 +7616,11 @@ static void rtl_hw_initialize(struct rtl8169_private *tp)
>>          }
>>   }
>>   
>> +static void rtl_disable_clk(void *data)
>> +{
>> +       clk_disable_unprepare(data);
>> +}
>> +
>>   static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>>   {
>>          const struct rtl_cfg_info *cfg = rtl_cfg_infos + ent->driver_data;
>> @@ -7647,6 +7654,32 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>>          mii->reg_num_mask = 0x1f;
>>          mii->supports_gmii = cfg->has_gmii;
>>   
>> +       /* Get the *optional* external "ether_clk" used on some boards */
>> +       tp->clk = devm_clk_get(&pdev->dev, "ether_clk");
> 
> An "optional" clk API is in flight, but it's easier to wait for that to
> merge and then this to be updated, so just take that as an FYI.

That is good to know.

> It would
> be interesting to see how to support optional clks with clkdev lookup > though, which would be needed in this case.

Ack.

> How would you know that a clk device driver hasn't probed yet and isn't
> the driver that's actually providing the clk to this device on x86
> systems? With DT systems we can figure that out by looking at the DT and
> seeing if the device driver requesting the clk has the clocks property.
> On x86 systems it's all clkdev which doesn't really lend itself to
> solving this problem.

Right on x86 the assumption is that the clk driver will be builtin and
will probe before the consumer. In this case that is true as the
pmc-atom-clk driver can only be builtin and its platform device is
instantiated from the acpi_lpss code and acpi init happens before
the PCI bus is scanned.

We have the same problem with ACPI OpRegions and drivers who have
_PS0 / _PS3 methods using those OpRegions and the "solution" so far
is the same, make sure all drivers providing OpRegion handlers are
builtin.

Basically we (x86) miss the nice dependency info and complete hw
description devicetree gives, so we rely on initialization order
for some of this. I added the -EPROBE_DEFER handling for completeness
sake / for other platforms, as you point out it will never trigger
on x86.

Regards,

Hans




> 
>> +       if (IS_ERR(tp->clk)) {
>> +               rc = PTR_ERR(tp->clk);

Re: [PATCH 2/4] r8169: Get and enable optional ether_clk clock

[Stephen Boyd]

Quoting Hans de Goede (2018-08-27 11:53:19)
> On 27-08-18 20:47, Stephen Boyd wrote:
> > How would you know that a clk device driver hasn't probed yet and isn't
> > the driver that's actually providing the clk to this device on x86
> > systems? With DT systems we can figure that out by looking at the DT and
> > seeing if the device driver requesting the clk has the clocks property.
> > On x86 systems it's all clkdev which doesn't really lend itself to
> > solving this problem.
> =

> Right on x86 the assumption is that the clk driver will be builtin and
> will probe before the consumer. In this case that is true as the
> pmc-atom-clk driver can only be builtin and its platform device is
> instantiated from the acpi_lpss code and acpi init happens before
> the PCI bus is scanned.

If we can go with this assumption then we can make the optional clk API
work even on clkdev based systems. Maybe if x86 had some way of
indicating that all builtin clks are registered? That might work but
it's not very clean. Or if we could check to see if we're running on an
ACPI based system in clkdev we could use that to assume that clk_get()
will only be called after all providers have registered their lookups.

> =

> We have the same problem with ACPI OpRegions and drivers who have
> _PS0 / _PS3 methods using those OpRegions and the "solution" so far
> is the same, make sure all drivers providing OpRegion handlers are
> builtin.
> =

> Basically we (x86) miss the nice dependency info and complete hw
> description devicetree gives, so we rely on initialization order
> for some of this. I added the -EPROBE_DEFER handling for completeness
> sake / for other platforms, as you point out it will never trigger
> on x86.
> =


Thanks for the info!

Re: [PATCH 2/4] r8169: Get and enable optional ether_clk clock

[Hans de Goede]

Hi,

On 27-08-18 21:14, Stephen Boyd wrote:
> Quoting Hans de Goede (2018-08-27 11:53:19)
>> On 27-08-18 20:47, Stephen Boyd wrote:
>>> How would you know that a clk device driver hasn't probed yet and isn't
>>> the driver that's actually providing the clk to this device on x86
>>> systems? With DT systems we can figure that out by looking at the DT and
>>> seeing if the device driver requesting the clk has the clocks property.
>>> On x86 systems it's all clkdev which doesn't really lend itself to
>>> solving this problem.
>>
>> Right on x86 the assumption is that the clk driver will be builtin and
>> will probe before the consumer. In this case that is true as the
>> pmc-atom-clk driver can only be builtin and its platform device is
>> instantiated from the acpi_lpss code and acpi init happens before
>> the PCI bus is scanned.
> 
> If we can go with this assumption then we can make the optional clk API
> work even on clkdev based systems. Maybe if x86 had some way of
> indicating that all builtin clks are registered?

Unfortunately there is no such thing I'm afraid.

> That might work but
> it's not very clean. Or if we could check to see if we're running on an
> ACPI based system in clkdev we could use that to assume that clk_get()
> will only be called after all providers have registered their lookups.

Yes some check for x86 + ACPI (ARM also uses ACPI, but there we
should no do this AFAICT) is probably best. That or not use the
new optional clk API on x86, but that means that any cross platform
driver cannot use it, which would be a pain.

BTW does your Acked-by indicate you are ok with merging this series
through the netdev tree as I suggested in the cover-letter? If so
can I also add your Acked-by to the 3th patch ?

Regards,

Hans

Re: [PATCH 2/4] r8169: Get and enable optional ether_clk clock

[Stephen Boyd]

Quoting Hans de Goede (2018-08-29 10:09:57)
> Hi,
> =

> On 27-08-18 21:14, Stephen Boyd wrote:
> > Quoting Hans de Goede (2018-08-27 11:53:19)
> >> On 27-08-18 20:47, Stephen Boyd wrote:
> >>> How would you know that a clk device driver hasn't probed yet and isn=
't
> >>> the driver that's actually providing the clk to this device on x86
> >>> systems? With DT systems we can figure that out by looking at the DT =
and
> >>> seeing if the device driver requesting the clk has the clocks propert=
y.
> >>> On x86 systems it's all clkdev which doesn't really lend itself to
> >>> solving this problem.
> >>
> >> Right on x86 the assumption is that the clk driver will be builtin and
> >> will probe before the consumer. In this case that is true as the
> >> pmc-atom-clk driver can only be builtin and its platform device is
> >> instantiated from the acpi_lpss code and acpi init happens before
> >> the PCI bus is scanned.
> > =

> > If we can go with this assumption then we can make the optional clk API
> > work even on clkdev based systems. Maybe if x86 had some way of
> > indicating that all builtin clks are registered?
> =

> Unfortunately there is no such thing I'm afraid.

Ugh!

> =

> > That might work but
> > it's not very clean. Or if we could check to see if we're running on an
> > ACPI based system in clkdev we could use that to assume that clk_get()
> > will only be called after all providers have registered their lookups.
> =

> Yes some check for x86 + ACPI (ARM also uses ACPI, but there we
> should no do this AFAICT) is probably best. That or not use the
> new optional clk API on x86, but that means that any cross platform
> driver cannot use it, which would be a pain.

Right. The optional clk API will be not so great until we can get ACPI
to move way from clkdev.

> =

> BTW does your Acked-by indicate you are ok with merging this series
> through the netdev tree as I suggested in the cover-letter? If so
> can I also add your Acked-by to the 3th patch ?
> =


Yep, I thought I did that but now I've really done it.

[PATCH 3/4] clk: x86: Stop marking clocks as CLK_IS_CRITICAL

[Hans de Goede]

Commit d31fd43c0f9a ("clk: x86: Do not gate clocks enabled by the
firmware"), which added the code to mark clocks as CLK_IS_CRITICAL, causes
all unclaimed PMC clocks on Cherry Trail devices to be on all the time,
resulting on the device not being able to reach S0i3 when suspended.

The reason for this commit is that on some Bay Trail / Cherry Trail devices
the r8169 ethernet controller uses pmc_plt_clk_4. Now that the clk-pmc-atom
driver exports an "ether_clk" alias for pmc_plt_clk_4 and the r8169 driver
has been modified to get and enable this clock (if present) the marking of
the clocks as CLK_IS_CRITICAL is no longer necessary.

This commit removes the CLK_IS_CRITICAL marking, fixing Cherry Trail
devices not being able to reach S0i3 greatly decreasing their battery
drain when suspended.

Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=193891#c102
Cc: Johannes Stezenbach <js@sig21.net>
Cc: Carlo Caione <carlo@endlessm.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/clk/x86/clk-pmc-atom.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/clk/x86/clk-pmc-atom.c b/drivers/clk/x86/clk-pmc-atom.c
index 75151901ff7d..d977193842df 100644
--- a/drivers/clk/x86/clk-pmc-atom.c
+++ b/drivers/clk/x86/clk-pmc-atom.c
@@ -187,13 +187,6 @@ static struct clk_plt *plt_clk_register(struct platform_device *pdev, int id,
 	pclk->reg = base + PMC_CLK_CTL_OFFSET + id * PMC_CLK_CTL_SIZE;
 	spin_lock_init(&pclk->lock);
 
-	/*
-	 * 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))
-		init.flags |= CLK_IS_CRITICAL;
-
 	ret = devm_clk_hw_register(&pdev->dev, &pclk->hw);
 	if (ret) {
 		pclk = ERR_PTR(ret);
-- 
2.18.0

Re: [PATCH 3/4] clk: x86: Stop marking clocks as CLK_IS_CRITICAL

[Stephen Boyd]

Quoting Hans de Goede (2018-08-27 07:31:59)
> Commit d31fd43c0f9a ("clk: x86: Do not gate clocks enabled by the
> firmware"), which added the code to mark clocks as CLK_IS_CRITICAL, causes
> all unclaimed PMC clocks on Cherry Trail devices to be on all the time,
> resulting on the device not being able to reach S0i3 when suspended.
> =

> The reason for this commit is that on some Bay Trail / Cherry Trail devic=
es
> the r8169 ethernet controller uses pmc_plt_clk_4. Now that the clk-pmc-at=
om
> driver exports an "ether_clk" alias for pmc_plt_clk_4 and the r8169 driver
> has been modified to get and enable this clock (if present) the marking of
> the clocks as CLK_IS_CRITICAL is no longer necessary.
> =

> This commit removes the CLK_IS_CRITICAL marking, fixing Cherry Trail
> devices not being able to reach S0i3 greatly decreasing their battery
> drain when suspended.
> =

> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=3D193891#c102
> Cc: Johannes Stezenbach <js@sig21.net>
> Cc: Carlo Caione <carlo@endlessm.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---

Acked-by: Stephen Boyd <sboyd@kernel.org>

[PATCH 4/4] RFC: r8169: Disable clk during suspend / resume

[Hans de Goede]

Disable the clk during suspend to save power. Note that tp->clk may be
NULL, the clk core functions handle this without problems.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/net/ethernet/realtek/r8169.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 779b02979493..aebc90158bd9 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -7312,8 +7312,10 @@ static int rtl8169_suspend(struct device *device)
 {
 	struct pci_dev *pdev = to_pci_dev(device);
 	struct net_device *dev = pci_get_drvdata(pdev);
+	struct rtl8169_private *tp = netdev_priv(dev);
 
 	rtl8169_net_suspend(dev);
+	clk_disable_unprepare(tp->clk);
 
 	return 0;
 }
@@ -7340,6 +7342,7 @@ static int rtl8169_resume(struct device *device)
 	struct net_device *dev = pci_get_drvdata(pdev);
 	struct rtl8169_private *tp = netdev_priv(dev);
 
+	clk_prepare_enable(tp->clk);
 	rtl8169_init_phy(dev, tp);
 
 	if (netif_running(dev))
-- 
2.18.0

Re: [PATCH 0/4] clk-pmc-atom + r8169: Add ether_clk handling to fix suspend issues

[Andy Shevchenko]

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@linux.intel.com>

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

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

> Carlos can you test the 4th patch (when you have time) and let us know if
> it works?
> 
> Regards,
> 
> Hans
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/4] clk-pmc-atom + r8169: Add ether_clk handling to fix suspend issues

[Hans de Goede]

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@linux.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@sig21.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

Re: [PATCH 0/4] clk-pmc-atom + r8169: Add ether_clk handling to fix suspend issues

[Andy Shevchenko]

On Wed, Aug 29, 2018 at 07:06:09PM +0200, Hans de Goede wrote:

> > What a nice stuff, thanks!
> > 
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.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@sig21.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.

Forgot to mention, instead of Irina, which is not longer working for Intel for
more than a year, put Pierre's address there.


-- 
With Best Regards,
Andy Shevchenko