r8169 broken when enabling the Atom PMC platform clocks

r8169 broken when enabling the Atom PMC platform clocks

[Carlo Caione]

Hi,
We are working on an Asus Z550M shipping a baytrail processor. From
the 4.11 kernel we noticed that the system is not bootable anymore
since it hangs during boot when probing the r8169 driver, not even a
trace is available.

We bisected this problem down to commit 282a4e4 ("platform/x86: Enable
Atom PMC platform clocks").

We suspected that the problem is that one of the PMC clocks is being
used by the Ethernet board as XTAL clock and since it is not
explicitly claimed by the driver, it is gated at boot by the clock
framework, causing the system to hang.

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.

The best solution would probably be avoiding to gate the clocks at all
when booting if these are being already used by the firmware, but IIUC
this information is not always available in the enable clock register.

Any idea how to approach this issue? I guess in the future we will see
more platforms needing a quirk like this because of fancy clock
routings.

Thanks,

-- 
Carlo Caione

Re: r8169 broken when enabling the Atom PMC platform clocks

[Andy Shevchenko]

On Mon, 2017-07-10 at 16:15 +0200, Carlo Caione wrote:
> Hi,
> We are working on an Asus Z550M shipping a baytrail processor. From
> the 4.11 kernel we noticed that the system is not bootable anymore
> since it hangs during boot when probing the r8169 driver, not even a
> trace is available.
> 
> We bisected this problem down to commit 282a4e4 ("platform/x86: Enable
> Atom PMC platform clocks").
> 
> We suspected that the problem is that one of the PMC clocks is being
> used by the Ethernet board as XTAL clock and since it is not
> explicitly claimed by the driver, it is gated at boot by the clock
> framework, causing the system to hang.
> 
> 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.

Can you copy in-place the hack patch you have?

> The best solution would probably be avoiding to gate the clocks at all
> when booting if these are being already used by the firmware, but IIUC
> this information is not always available in the enable clock register.

Yes, sounds sane.

> Any idea how to approach this issue? I guess in the future we will see
> more platforms needing a quirk like this because of fancy clock
> routings.

As simplest way we might do DMI matching, though I prefer to avoid as
much as possible loading kernel by quirks.

Pierre?

Darren, it seems not the first report regarding this series.
Like a quick fix we perhaps need to revert enabling patch and propagate
to stable. Opinions?

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

Re: r8169 broken when enabling the Atom PMC platform clocks

[Carlo Caione]

On 10/07/2017 16:20, Andy Shevchenko wrote:
> On Mon, 2017-07-10 at 16:15 +0200, Carlo Caione wrote:
>> Hi,
>> We are working on an Asus Z550M shipping a baytrail processor. From
>> the 4.11 kernel we noticed that the system is not bootable anymore
>> since it hangs during boot when probing the r8169 driver, not even a
>> trace is available.
>>
>> We bisected this problem down to commit 282a4e4 ("platform/x86: Enable
>> Atom PMC platform clocks").
>>
>> We suspected that the problem is that one of the PMC clocks is being
>> used by the Ethernet board as XTAL clock and since it is not
>> explicitly claimed by the driver, it is gated at boot by the clock
>> framework, causing the system to hang.
>>
>> 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.
> 
> Can you copy in-place the hack patch you have?

diff --git a/drivers/net/ethernet/realtek/r8169.c 
b/drivers/net/ethernet/realtek/r8169.c
index de013b3ba2ab..27ddda7b13b1 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -20,6 +20,8 @@
  #include <linux/crc32.h>
  #include <linux/in.h>
  #include <linux/ip.h>
+#include <linux/clk.h>
+#include <asm/cpu_device_id.h>
  #include <linux/tcp.h>
  #include <linux/interrupt.h>
  #include <linux/dma-mapping.h>
@@ -796,6 +798,7 @@ struct rtl8169_private {
  	struct ring_info tx_skb[NUM_TX_DESC];	/* Tx data buffers */
  	struct timer_list timer;
  	u16 cp_cmd;
+	struct clk *clk;

  	u16 event_slow;

@@ -8210,6 +8213,17 @@ static struct dmi_system_id rtl_dmi_table[] 
__initdata = {
  	},
  	{}
  };
+static bool is_valleyview(void)
+{
+	static const struct x86_cpu_id cpu_ids[] = {
+		{ X86_VENDOR_INTEL, 6, 55 }, /* Valleyview, Bay Trail */
+		{}
+	};
+
+	if (!x86_match_cpu(cpu_ids))
+		return false;
+	return true;
+}

  static int rtl_init_one(struct pci_dev *pdev, const struct 
pci_device_id *ent)
  {
@@ -8220,7 +8234,7 @@ static int rtl_init_one(struct pci_dev *pdev, 
const struct pci_device_id *ent)
  	struct net_device *dev;
  	void __iomem *ioaddr;
  	int chipset, i;
-	int rc;
+	int rc, ret;
  	static const struct {
  		unsigned long mask;
  		const char *type;
@@ -8254,6 +8268,39 @@ static int rtl_init_one(struct pci_dev *pdev, 
const struct pci_device_id *ent)
  	tp->pci_dev = pdev;
  	tp->msg_enable = netif_msg_init(debug.msg_enable, R8169_MSG_DEFAULT);

+	if (is_valleyview()) {
+		tp->clk = devm_clk_get(&pdev->dev, "pmc_plt_clk_4");
+		if (IS_ERR(tp->clk)) {
+			netif_err(tp, probe, dev, "Failed to get clock from pmc_plt_clk_4: 
%ld\n",
+				  PTR_ERR(tp->clk));
+			return PTR_ERR(tp->clk);
+		}
+
+		/*
+		 * The firmware might enable the clock at
+		 * boot (this information may or may not
+		 * be reflected in the enable clock register).
+		 * To change the rate we must disable the clock
+		 * first to cover these cases. Due to common
+		 * clock framework restrictions that do not allow
+		 * to disable a clock that has not been enabled,
+		 * we need to enable the clock first.
+		 */
+		ret = clk_prepare_enable(tp->clk);
+		if (!ret)
+			clk_disable_unprepare(tp->clk);
+
+		ret = clk_set_rate(tp->clk, 25000000);
+		if (ret)
+			netif_err(tp, probe, dev, "Unable to set clock\n");
+
+		ret = clk_prepare_enable(tp->clk);
+		if (ret < 0) {
+			netif_err(tp, probe, dev, "Couldn't configure clock\n");
+			return ret;
+		}
+	}
+
  	mii = &tp->mii;
  	mii->dev = dev;
  	mii->mdio_read = rtl_mdio_read;


-- 
Carlo Caione  |  +39.340.80.30.096  |  Endless

Re: r8169 broken when enabling the Atom PMC platform clocks

[Darren Hart]

On Mon, Jul 10, 2017 at 05:20:43PM +0300, Andy Shevchenko wrote:
> On Mon, 2017-07-10 at 16:15 +0200, Carlo Caione wrote:
> > Hi,
> > We are working on an Asus Z550M shipping a baytrail processor. From
> > the 4.11 kernel we noticed that the system is not bootable anymore
> > since it hangs during boot when probing the r8169 driver, not even a
> > trace is available.
> > 
> > We bisected this problem down to commit 282a4e4 ("platform/x86: Enable
> > Atom PMC platform clocks").
> > 
> > We suspected that the problem is that one of the PMC clocks is being
> > used by the Ethernet board as XTAL clock and since it is not
> > explicitly claimed by the driver, it is gated at boot by the clock
> > framework, causing the system to hang.
> > 
> > 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.
> 
> Can you copy in-place the hack patch you have?
> 
> > The best solution would probably be avoiding to gate the clocks at all
> > when booting if these are being already used by the firmware, but IIUC
> > this information is not always available in the enable clock register.
> 
> Yes, sounds sane.

Looking back, I've seen two instances that appear related to this patch
series. This one, and the one reported by Enric (on Cc) regarding the
Acer C200 audio.

Enric, did your issue get resolved?

In those two cases, are we able to determine programmatically if the
clock is already in use by the firmware? (via the enabled register I
presume?)

> 
> > Any idea how to approach this issue? I guess in the future we will see
> > more platforms needing a quirk like this because of fancy clock
> > routings.
> 
> As simplest way we might do DMI matching, though I prefer to avoid as
> much as possible loading kernel by quirks.
> 

This is the ultimate result of introducing these kinds of drivers well
after the release of the silicon. OEMs will do what they have to do to
get it to work, inevitably in a very platform specific way, which will
conflict with a general solution introduced later. It's unfortunate, but
the reality.

I would like to see us attempt to detect this via available means
(enable register?) and failing that, introduce the DMI quirks... it
sounds like the clocks are platform data after all...

> Pierre?
> 
> Darren, it seems not the first report regarding this series.
> Like a quick fix we perhaps need to revert enabling patch and propagate
> to stable. Opinions?

My concern with a revert is we'll just break platforms depending on the
new feature. It was first released with 4.12... so perhaps that is a
minimal risk.

What would be the long term solution? Presumably we want to enable it
again at some point?

I'd like to hear back from Pierre and Enric. If we can do this with
generic detection and a DMI based quirk, I *think* that would be
preferable.

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

-- 
Darren Hart
VMware Open Source Technology Center

Re: r8169 broken when enabling the Atom PMC platform clocks

[Carlo Caione]

On Mon, Jul 10, 2017 at 6:06 PM, Darren Hart <dvhart@infradead.org> wrote:
> On Mon, Jul 10, 2017 at 05:20:43PM +0300, Andy Shevchenko wrote:
>> On Mon, 2017-07-10 at 16:15 +0200, Carlo Caione wrote:
>> > Hi,
>> > We are working on an Asus Z550M shipping a baytrail processor. From
>> > the 4.11 kernel we noticed that the system is not bootable anymore
>> > since it hangs during boot when probing the r8169 driver, not even a
>> > trace is available.
>> >
>> > We bisected this problem down to commit 282a4e4 ("platform/x86: Enable
>> > Atom PMC platform clocks").
>> >
>> > We suspected that the problem is that one of the PMC clocks is being
>> > used by the Ethernet board as XTAL clock and since it is not
>> > explicitly claimed by the driver, it is gated at boot by the clock
>> > framework, causing the system to hang.
>> >
>> > 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.
>>
>> Can you copy in-place the hack patch you have?
>>
>> > The best solution would probably be avoiding to gate the clocks at all
>> > when booting if these are being already used by the firmware, but IIUC
>> > this information is not always available in the enable clock register.
>>
>> Yes, sounds sane.
>
> Looking back, I've seen two instances that appear related to this patch
> series. This one, and the one reported by Enric (on Cc) regarding the
> Acer C200 audio.
>
> Enric, did your issue get resolved?
>
> In those two cases, are we able to determine programmatically if the
> clock is already in use by the firmware? (via the enabled register I
> presume?)

Just tested this and the answer is no.
__clk_is_enabled() on the pmc_plt_clk_4 clock before
clk_prepare_enable() returns 0.

-- 
Carlo Caione  |  +39.340.80.30.096  |  Endless

Re: r8169 broken when enabling the Atom PMC platform clocks

[Carlo Caione]

On Mon, Jul 10, 2017 at 7:50 PM, Carlo Caione <carlo@endlessm.com> wrote:
> On Mon, Jul 10, 2017 at 6:06 PM, Darren Hart <dvhart@infradead.org> wrote:
>> On Mon, Jul 10, 2017 at 05:20:43PM +0300, Andy Shevchenko wrote:
>>> On Mon, 2017-07-10 at 16:15 +0200, Carlo Caione wrote:
>>> > Hi,
>>> > We are working on an Asus Z550M shipping a baytrail processor. From
>>> > the 4.11 kernel we noticed that the system is not bootable anymore
>>> > since it hangs during boot when probing the r8169 driver, not even a
>>> > trace is available.
>>> >
>>> > We bisected this problem down to commit 282a4e4 ("platform/x86: Enable
>>> > Atom PMC platform clocks").
>>> >
>>> > We suspected that the problem is that one of the PMC clocks is being
>>> > used by the Ethernet board as XTAL clock and since it is not
>>> > explicitly claimed by the driver, it is gated at boot by the clock
>>> > framework, causing the system to hang.
>>> >
>>> > 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.
>>>
>>> Can you copy in-place the hack patch you have?
>>>
>>> > The best solution would probably be avoiding to gate the clocks at all
>>> > when booting if these are being already used by the firmware, but IIUC
>>> > this information is not always available in the enable clock register.
>>>
>>> Yes, sounds sane.
>>
>> Looking back, I've seen two instances that appear related to this patch
>> series. This one, and the one reported by Enric (on Cc) regarding the
>> Acer C200 audio.
>>
>> Enric, did your issue get resolved?
>>
>> In those two cases, are we able to determine programmatically if the
>> clock is already in use by the firmware? (via the enabled register I
>> presume?)
>
> Just tested this and the answer is no.
> __clk_is_enabled() on the pmc_plt_clk_4 clock before
> clk_prepare_enable() returns 0.

Uhm, this is not true actually. I tested this in the r8169 driver at
probe time, so a bit too late if the clock framework already disabled
those clocks.
I checked again the enable registers in plt_clk_register() and I can
actually see that the pmc_plt_clk_4 is already enabled at boot by the
firmware. So yeah, at least in my case we are able to determine
programmatically if the clock is used already by the firmware.

-- 
Carlo Caione  |  +39.340.80.30.096  |  Endless

Re: r8169 broken when enabling the Atom PMC platform clocks

[Andy Shevchenko]

On Mon, 2017-07-10 at 20:23 +0200, Carlo Caione wrote:
> On Mon, Jul 10, 2017 at 7:50 PM, Carlo Caione <carlo@endlessm.com>
> wrote:
> > On Mon, Jul 10, 2017 at 6:06 PM, Darren Hart <dvhart@infradead.org>
> > wrote:
> > > On Mon, Jul 10, 2017 at 05:20:43PM +0300, Andy Shevchenko wrote:
> > > > On Mon, 2017-07-10 at 16:15 +0200, Carlo Caione wrote:

>>>>> The best solution would probably be avoiding to gate the clocks at
>>>>> all
> > > > > when booting if these are being already used by the firmware,
> > > > > but IIUC
> > > > > this information is not always available in the enable clock
> > > > > register.

> > > In those two cases, are we able to determine programmatically if
> > > the
> > > clock is already in use by the firmware? (via the enabled register
> > > I
> > > presume?)

> I checked again the enable registers in plt_clk_register() and I can
> actually see that the pmc_plt_clk_4 is already enabled at boot by the
> firmware. So yeah, at least in my case we are able to determine
> programmatically if the clock is used already by the firmware.

So, then it should be trivial to update the PMC clock driver to follow
firmware settings.

Please, Cc Enric when send the patch / new mail regarding topic.

Thanks, for investigating this!

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