[Intel-wired-lan] issue with kernel patch 2a3cdead8b408351fa1e3079b220fa331480ffbc

[Intel-wired-lan] issue with kernel patch 2a3cdead8b408351fa1e3079b220fa331480ffbc

[Fujinaka, Todd]

Jochen,
This change did not come from us. You should take it to the public mailing lists for discussion. This appears to have originated with Aaron Sierra.

Todd Fujinaka
Software Application Engineer
Networking Division (ND)
Intel Corporation
todd.fujinaka at intel.com
(503) 712-4565


-----Original Message-----
From: Wyborny, Carolyn 
Sent: Monday, April 25, 2016 7:50 AM
To: Jochen Henneberg <jh@henneberg-systemdesign.com>
Cc: Fujinaka, Todd <todd.fujinaka@intel.com>
Subject: RE: issue with kernel patch 2a3cdead8b408351fa1e3079b220fa331480ffbc

Adding Todd for i210 support.  Those phy functions were designed for that specific PHY.  If the PHY you have on your design identifies as the PHY id for the gs40g PHY, but does not work the way the functions expect, the failure makes sense. 

Thanks,

Carolyn

> -----Original Message-----
> From: Jochen Henneberg [mailto:jh at henneberg-systemdesign.com]
> Sent: Monday, April 25, 2016 2:46 AM
> To: Wyborny, Carolyn <carolyn.wyborny@intel.com>
> Subject: issue with kernel patch
> 2a3cdead8b408351fa1e3079b220fa331480ffbc
> 
> Hi Carolyn,
> 
> I have an embedded device here which comes with a BYT SoC and a 
> separate
> I210 ethernet controller attached to the PCIe bus.
> Before the mentioned patch everything worked fine, now reading phy id 
> fails for the external chip, the one on the SoC reports the correct 
> phy id. If I circumvent the phy id check in 
> igb_init_phy_params_82575() everything works fine again.
> From what I can see the phy id is not part of the NVM flash image, so 
> I wonder why this fails now that the special gs40g register read 
> functions have been removed.
> Could you probably give me a hint? Or does the driver after this patch 
> probably not cover all situations anymore?
> The HW vendor will contact Intel with the same issue in parallel, I 
> just wanted to know if you are aware of any corner cases where the 
> generic register reads may fail, especially for the phy id (as I 
> pointed out before, the other phy register like connection status and 
> such seem to behave as expected).
> 
> Thanks in advance,
> Jochen Henneberg
> --
> Henneberg - Systemdesign
> Jochen Henneberg
> Loehnfeld 26
> 21423 Winsen (Luhe)
> --
> Fon: +49 4174 668 773
> Mobile: +49 172 160 14 69
> Fax: +49 321 210 761 64
> www: www.henneberg-systemdesign.com
> 
> 

[Intel-wired-lan] issue with kernel patch 2a3cdead8b408351fa1e3079b220fa331480ffbc

[Aaron Sierra]

Jochen,
I have similar hardware to you. What PHY ID are you finding?

-Aaron

-- 
--- Extreme Engineering Solutions, Inc (X-ES) ---
Aaron Sierra
Senior Embedded Engineer
X-ES http://www.xes-inc.com
3225 Deming Way, Ste 120
Middleton, WI  53562
Phone: (608) 833-1155 x115
Email: asierra at xes-inc.com

----- Original Message -----
> From: "Todd Fujinaka" <todd.fujinaka@intel.com>
> To: "Jochen Henneberg" <jh@henneberg-systemdesign.com>, intel-wired-lan at lists.osuosl.org, asierra at xes-inc.com
> Cc: "Carolyn Wyborny" <carolyn.wyborny@intel.com>
> Sent: Monday, April 25, 2016 10:07:30 AM
> Subject: RE: issue with kernel patch 2a3cdead8b408351fa1e3079b220fa331480ffbc
> 
> Jochen,
> This change did not come from us. You should take it to the public mailing
> lists for discussion. This appears to have originated with Aaron Sierra.
> 
> Todd Fujinaka
> Software Application Engineer
> Networking Division (ND)
> Intel Corporation
> todd.fujinaka at intel.com
> (503) 712-4565
> 
> 
> -----Original Message-----
> From: Wyborny, Carolyn
> Sent: Monday, April 25, 2016 7:50 AM
> To: Jochen Henneberg <jh@henneberg-systemdesign.com>
> Cc: Fujinaka, Todd <todd.fujinaka@intel.com>
> Subject: RE: issue with kernel patch 2a3cdead8b408351fa1e3079b220fa331480ffbc
> 
> Adding Todd for i210 support.  Those phy functions were designed for that
> specific PHY.  If the PHY you have on your design identifies as the PHY id
> for the gs40g PHY, but does not work the way the functions expect, the
> failure makes sense.
> 
> Thanks,
> 
> Carolyn
> 
> > -----Original Message-----
> > From: Jochen Henneberg [mailto:jh at henneberg-systemdesign.com]
> > Sent: Monday, April 25, 2016 2:46 AM
> > To: Wyborny, Carolyn <carolyn.wyborny@intel.com>
> > Subject: issue with kernel patch
> > 2a3cdead8b408351fa1e3079b220fa331480ffbc
> > 
> > Hi Carolyn,
> > 
> > I have an embedded device here which comes with a BYT SoC and a
> > separate
> > I210 ethernet controller attached to the PCIe bus.
> > Before the mentioned patch everything worked fine, now reading phy id
> > fails for the external chip, the one on the SoC reports the correct
> > phy id. If I circumvent the phy id check in
> > igb_init_phy_params_82575() everything works fine again.
> > From what I can see the phy id is not part of the NVM flash image, so
> > I wonder why this fails now that the special gs40g register read
> > functions have been removed.
> > Could you probably give me a hint? Or does the driver after this patch
> > probably not cover all situations anymore?
> > The HW vendor will contact Intel with the same issue in parallel, I
> > just wanted to know if you are aware of any corner cases where the
> > generic register reads may fail, especially for the phy id (as I
> > pointed out before, the other phy register like connection status and
> > such seem to behave as expected).
> > 
> > Thanks in advance,
> > Jochen Henneberg
> > --
> > Henneberg - Systemdesign
> > Jochen Henneberg
> > Loehnfeld 26
> > 21423 Winsen (Luhe)
> > --
> > Fon: +49 4174 668 773
> > Mobile: +49 172 160 14 69
> > Fax: +49 321 210 761 64
> > www: www.henneberg-systemdesign.com
> > 
> > 
> 
> 

[Intel-wired-lan] issue with kernel patch 2a3cdead8b408351fa1e3079b220fa331480ffbc

[Jochen Henneberg]

Hi Aaron,

On Mo, 2016-04-25 at 10:27 -0500, Aaron Sierra wrote:
> Jochen,
> I have similar hardware to you. What PHY ID are you finding?

The chip is WTI210IT from Intel, I am currently not in the office but I
will provide the PHY ID tomorrow. However, the chip datasheet
(http://www.intel.de/content/dam/www/public/us/en/documents/datasheets/i210-ethernet-controller-datasheet.pdf)
8.27.3.3/8.27.3.4 says that the PHY IDs are fixed (RO), but the values
read from the chip are different from what is defined in the datasheet
and I wonder if this is related to the modified register read/write
after the patch.
However, the patch works fine with the BYT embedded network controller.

Jochen

> 
> -Aaron
> 

-- 
Henneberg - Systemdesign
Jochen Henneberg
Loehnfeld 26
21423 Winsen (Luhe)
--
Fon: +49 4174 668 773
Mobile: +49 172 160 14 69
Fax: +49 321 210 761 64
www: www.henneberg-systemdesign.com

[Intel-wired-lan] issue with kernel patch 2a3cdead8b408351fa1e3079b220fa331480ffbc

[Aaron Sierra]

Jochen,

> I wonder if this is related to the modified register read/write
> after the patch.

Possibly. The GS40G functions explicitly set the PHY page with every access. I
suspect that your external I210 PHY was left with some other page selected (by
your BIOS?). Try adding the following debug to your driver to see which page is
selected:

diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.c b/drivers/net/ethernet
index a23aa67..5cd9e91 100644
--- a/drivers/net/ethernet/intel/igb/e1000_82575.c
+++ b/drivers/net/ethernet/intel/igb/e1000_82575.c
@@ -226,6 +226,7 @@ static s32 igb_init_phy_params_82575(struct e1000_hw *hw)
                phy->ops.read_reg = igb_read_phy_reg_sgmii_82575;
                phy->ops.write_reg = igb_write_phy_reg_sgmii_82575;
        } else {
+               u16 page;
                switch (hw->mac.type) {
                case e1000_82580:
                case e1000_i350:
@@ -234,6 +235,8 @@ static s32 igb_init_phy_params_82575(struct e1000_hw *hw)
                case e1000_i211:
                        phy->ops.read_reg = igb_read_phy_reg_82580;
                        phy->ops.write_reg = igb_write_phy_reg_82580;
+                       phy->ops.read_reg(hw, I347AT4_PAGE_SELECT, &page);
+                       pr_info("%s: default page: %x\n", __func__, page);
                        break;
                default:
                        phy->ops.read_reg = igb_read_phy_reg_igp;
@@ -251,6 +254,7 @@ static s32 igb_init_phy_params_82575(struct e1000_hw *hw)
                return ret_val;
 
        /* Verify phy id and set remaining function pointers */
+       pr_info("%s: PHY ID: %x\n", __func__, phy->id);
        switch (phy->id) {
        case M88E1543_E_PHY_ID:
        case M88E1512_E_PHY_ID:

This is the output that I get from an external I210 device attached to a
Bay Trail SoC:

igb 0000:02:00.0: added PHC on eth0
igb 0000:02:00.0: Intel(R) Gigabit Ethernet Network Connection
igb 0000:02:00.0: eth0: (PCIe:2.5Gb/s:Width x1) 00:17:3c:02:88:56
igb 0000:02:00.0: eth0: PBA No: FFFFFF-0FF
igb 0000:02:00.0: Using MSI-X interrupts. 2 rx queue(s), 2 tx queue(s)
igb: igb_init_phy_params_82575: default page: 0
igb: igb_init_phy_params_82575: PHY ID: 1410c00

-Aaron

[Intel-wired-lan] issue with kernel patch 2a3cdead8b408351fa1e3079b220fa331480ffbc

[Jochen Henneberg]

On Mo, 2016-04-25 at 12:35 -0500, Aaron Sierra wrote:
> This is the output that I get from an external I210 device attached to
> a
> Bay Trail SoC:
> 
> igb 0000:02:00.0: added PHC on eth0
> igb 0000:02:00.0: Intel(R) Gigabit Ethernet Network Connection
> igb 0000:02:00.0: eth0: (PCIe:2.5Gb/s:Width x1) 00:17:3c:02:88:56
> igb 0000:02:00.0: eth0: PBA No: FFFFFF-0FF
> igb 0000:02:00.0: Using MSI-X interrupts. 2 rx queue(s), 2 tx queue(s)
> igb: igb_init_phy_params_82575: default page: 0
> igb: igb_init_phy_params_82575: PHY ID: 1410c00

And here is mine with the patch applied:

igb: Intel(R) Gigabit Ethernet Network Driver - version 5.3.0-k
igb: Copyright (c) 2007-2014 Intel Corporation.
igb: igb_init_phy_params_82575: default page: fc
igb: igb_init_phy_params_82575: PHY ID: a0044e90
igb: probe of 0000:01:00.0 failed with error -2
igb: igb_init_phy_params_82575: default page: 0
igb: igb_init_phy_params_82575: PHY ID: 1410c00
pps pps0: new PPS source ptp0
igb 0000:04:00.0: added PHC on eth0
igb 0000:04:00.0: Intel(R) Gigabit Ethernet Network Connection
igb 0000:04:00.0: eth0: (PCIe:2.5Gb/s:Width x1) 00:e0:4b:47:38:6f
igb 0000:04:00.0: eth0: PBA No: FFFFFF-0FF
igb 0000:04:00.0: Using MSI-X interrupts. 2 rx queue(s), 2 tx queue(s)

At PCI 0000:04:00.0 is the BYT NIC, this comes up with the correct page
selection and phy id. Above it (0000:01:00.0) is the external NIC which
comes up with 'anything' (but 'anything' is the same after each power
cycle)!?
Some explanations to the HW setup, the BYT resides on a Kontron nanoETX
board, the external controller is on the custom base board, it is not at
all integrated into the BIOS setup (the BIOS should only take care of
the internal NIC).
The datasheet states that the page selection register should come up
with 0 after power on, but either somebody writes the register up-front
(but who) or this comes from a broken setup in the NVM?

Jochen
-- 
Henneberg - Systemdesign
Jochen Henneberg
Loehnfeld 26
21423 Winsen (Luhe)
--
Fon: +49 4174 668 773
Mobile: +49 172 160 14 69
Fax: +49 321 210 761 64
www: www.henneberg-systemdesign.com

[Intel-wired-lan] issue with kernel patch 2a3cdead8b408351fa1e3079b220fa331480ffbc

[Fujinaka, Todd]

Have you reported this to Kontron? We may have to get them involved with this.

Todd Fujinaka
Software Application Engineer
Networking Division (ND)
Intel Corporation
todd.fujinaka at intel.com
(503) 712-4565


-----Original Message-----
From: Jochen Henneberg [mailto:jh at henneberg-systemdesign.com] 
Sent: Tuesday, April 26, 2016 1:02 AM
To: Aaron Sierra <asierra@xes-inc.com>
Cc: Fujinaka, Todd <todd.fujinaka@intel.com>; intel-wired-lan at lists.osuosl.org; Wyborny, Carolyn <carolyn.wyborny@intel.com>
Subject: Re: issue with kernel patch 2a3cdead8b408351fa1e3079b220fa331480ffbc

On Mo, 2016-04-25 at 12:35 -0500, Aaron Sierra wrote:
> This is the output that I get from an external I210 device attached to 
> a Bay Trail SoC:
> 
> igb 0000:02:00.0: added PHC on eth0
> igb 0000:02:00.0: Intel(R) Gigabit Ethernet Network Connection igb 
> 0000:02:00.0: eth0: (PCIe:2.5Gb/s:Width x1) 00:17:3c:02:88:56 igb 
> 0000:02:00.0: eth0: PBA No: FFFFFF-0FF igb 0000:02:00.0: Using MSI-X 
> interrupts. 2 rx queue(s), 2 tx queue(s)
> igb: igb_init_phy_params_82575: default page: 0
> igb: igb_init_phy_params_82575: PHY ID: 1410c00

And here is mine with the patch applied:

igb: Intel(R) Gigabit Ethernet Network Driver - version 5.3.0-k
igb: Copyright (c) 2007-2014 Intel Corporation.
igb: igb_init_phy_params_82575: default page: fc
igb: igb_init_phy_params_82575: PHY ID: a0044e90
igb: probe of 0000:01:00.0 failed with error -2
igb: igb_init_phy_params_82575: default page: 0
igb: igb_init_phy_params_82575: PHY ID: 1410c00 pps pps0: new PPS source ptp0 igb 0000:04:00.0: added PHC on eth0 igb 0000:04:00.0: Intel(R) Gigabit Ethernet Network Connection igb 0000:04:00.0: eth0: (PCIe:2.5Gb/s:Width x1) 00:e0:4b:47:38:6f igb 0000:04:00.0: eth0: PBA No: FFFFFF-0FF igb 0000:04:00.0: Using MSI-X interrupts. 2 rx queue(s), 2 tx queue(s)

At PCI 0000:04:00.0 is the BYT NIC, this comes up with the correct page selection and phy id. Above it (0000:01:00.0) is the external NIC which comes up with 'anything' (but 'anything' is the same after each power cycle)!?
Some explanations to the HW setup, the BYT resides on a Kontron nanoETX board, the external controller is on the custom base board, it is not at all integrated into the BIOS setup (the BIOS should only take care of the internal NIC).
The datasheet states that the page selection register should come up with 0 after power on, but either somebody writes the register up-front (but who) or this comes from a broken setup in the NVM?

Jochen
--
Henneberg - Systemdesign
Jochen Henneberg
Loehnfeld 26
21423 Winsen (Luhe)
--
Fon: +49 4174 668 773
Mobile: +49 172 160 14 69
Fax: +49 321 210 761 64
www: www.henneberg-systemdesign.com

[Intel-wired-lan] issue with kernel patch 2a3cdead8b408351fa1e3079b220fa331480ffbc

[Jochen Henneberg]

Hi Todd,

I have send all background information to Kontron (FAE), they will take
a look if the BIOS somehow communicates with the external NIC.
The FAE told me that he does not want to join the mailing list, so I
will exchange the information from/to Kontron off-band and push them to
the list if I get updates.

Regards,
Jochen

On Di, 2016-04-26 at 14:47 +0000, Fujinaka, Todd wrote:
> Have you reported this to Kontron? We may have to get them involved with this.
> 
> Todd Fujinaka
> Software Application Engineer
> Networking Division (ND)
> Intel Corporation
> todd.fujinaka at intel.com
> (503) 712-4565
> 
> 
> -----Original Message-----
> From: Jochen Henneberg [mailto:jh at henneberg-systemdesign.com] 
> Sent: Tuesday, April 26, 2016 1:02 AM
> To: Aaron Sierra <asierra@xes-inc.com>
> Cc: Fujinaka, Todd <todd.fujinaka@intel.com>; intel-wired-lan at lists.osuosl.org; Wyborny, Carolyn <carolyn.wyborny@intel.com>
> Subject: Re: issue with kernel patch 2a3cdead8b408351fa1e3079b220fa331480ffbc
> 
> On Mo, 2016-04-25 at 12:35 -0500, Aaron Sierra wrote:
> > This is the output that I get from an external I210 device attached to 
> > a Bay Trail SoC:
> > 
> > igb 0000:02:00.0: added PHC on eth0
> > igb 0000:02:00.0: Intel(R) Gigabit Ethernet Network Connection igb 
> > 0000:02:00.0: eth0: (PCIe:2.5Gb/s:Width x1) 00:17:3c:02:88:56 igb 
> > 0000:02:00.0: eth0: PBA No: FFFFFF-0FF igb 0000:02:00.0: Using MSI-X 
> > interrupts. 2 rx queue(s), 2 tx queue(s)
> > igb: igb_init_phy_params_82575: default page: 0
> > igb: igb_init_phy_params_82575: PHY ID: 1410c00
> 
> And here is mine with the patch applied:
> 
> igb: Intel(R) Gigabit Ethernet Network Driver - version 5.3.0-k
> igb: Copyright (c) 2007-2014 Intel Corporation.
> igb: igb_init_phy_params_82575: default page: fc
> igb: igb_init_phy_params_82575: PHY ID: a0044e90
> igb: probe of 0000:01:00.0 failed with error -2
> igb: igb_init_phy_params_82575: default page: 0
> igb: igb_init_phy_params_82575: PHY ID: 1410c00 pps pps0: new PPS source ptp0 igb 0000:04:00.0: added PHC on eth0 igb 0000:04:00.0: Intel(R) Gigabit Ethernet Network Connection igb 0000:04:00.0: eth0: (PCIe:2.5Gb/s:Width x1) 00:e0:4b:47:38:6f igb 0000:04:00.0: eth0: PBA No: FFFFFF-0FF igb 0000:04:00.0: Using MSI-X interrupts. 2 rx queue(s), 2 tx queue(s)
> 
> At PCI 0000:04:00.0 is the BYT NIC, this comes up with the correct page selection and phy id. Above it (0000:01:00.0) is the external NIC which comes up with 'anything' (but 'anything' is the same after each power cycle)!?
> Some explanations to the HW setup, the BYT resides on a Kontron nanoETX board, the external controller is on the custom base board, it is not at all integrated into the BIOS setup (the BIOS should only take care of the internal NIC).
> The datasheet states that the page selection register should come up with 0 after power on, but either somebody writes the register up-front (but who) or this comes from a broken setup in the NVM?
> 
> Jochen
> --
> Henneberg - Systemdesign
> Jochen Henneberg
> Loehnfeld 26
> 21423 Winsen (Luhe)
> --
> Fon: +49 4174 668 773
> Mobile: +49 172 160 14 69
> Fax: +49 321 210 761 64
> www: www.henneberg-systemdesign.com
> 
> 
> 

-- 
Henneberg - Systemdesign
Jochen Henneberg
Loehnfeld 26
21423 Winsen (Luhe)
--
Fon: +49 4174 668 773
Mobile: +49 172 160 14 69
Fax: +49 321 210 761 64
www: www.henneberg-systemdesign.com

[Intel-wired-lan] issue with kernel patch 2a3cdead8b408351fa1e3079b220fa331480ffbc

[Aaron Sierra]

Jochen,

----- Original Message -----
> From: "Jochen Henneberg" <jh@henneberg-systemdesign.com>
> Sent: Tuesday, April 26, 2016 3:02:12 AM
> 
> On Mo, 2016-04-25 at 12:35 -0500, Aaron Sierra wrote:
> > This is the output that I get from an external I210 device attached to
> > a Bay Trail SoC:
> > 
> > igb 0000:02:00.0: added PHC on eth0
> > igb 0000:02:00.0: Intel(R) Gigabit Ethernet Network Connection
> > igb 0000:02:00.0: eth0: (PCIe:2.5Gb/s:Width x1) 00:17:3c:02:88:56
> > igb 0000:02:00.0: eth0: PBA No: FFFFFF-0FF
> > igb 0000:02:00.0: Using MSI-X interrupts. 2 rx queue(s), 2 tx queue(s)
> > igb: igb_init_phy_params_82575: default page: 0
> > igb: igb_init_phy_params_82575: PHY ID: 1410c00
> 
> And here is mine with the patch applied:
> 
> igb: Intel(R) Gigabit Ethernet Network Driver - version 5.3.0-k
> igb: Copyright (c) 2007-2014 Intel Corporation.
> igb: igb_init_phy_params_82575: default page: fc
> igb: igb_init_phy_params_82575: PHY ID: a0044e90
> igb: probe of 0000:01:00.0 failed with error -2

[snip]

> The datasheet states that the page selection register should come up
> with 0 after power on, but either somebody writes the register up-front
> (but who) or this comes from a broken setup in the NVM?

Hmm, that's the same page, E1000_PHY_PLL_FREQ_PAGE, touched by
igb_pll_workaround_i210() in the commit that you referenced, but there's
an explicit write to reset the page to zero at the end. Do you get into
that function with the external I210?

-Aaron
 
> Jochen

[Intel-wired-lan] issue with kernel patch 2a3cdead8b408351fa1e3079b220fa331480ffbc

[Jochen Henneberg]

Hi Aaron,

On Di, 2016-04-26 at 10:52 -0500, Aaron Sierra wrote:
> > igb: Intel(R) Gigabit Ethernet Network Driver - version 5.3.0-k
> > igb: Copyright (c) 2007-2014 Intel Corporation.
> > igb: igb_init_phy_params_82575: default page: fc
> > igb: igb_init_phy_params_82575: PHY ID: a0044e90
> > igb: probe of 0000:01:00.0 failed with error -2
> 
> [snip]
> 
> > The datasheet states that the page selection register should come up
> > with 0 after power on, but either somebody writes the register
> up-front
> > (but who) or this comes from a broken setup in the NVM?
> 
> Hmm, that's the same page, E1000_PHY_PLL_FREQ_PAGE, touched by
> igb_pll_workaround_i210() in the commit that you referenced, but
> there's
> an explicit write to reset the page to zero at the end. Do you get
> into
> that function with the external I210?

That function is not called before the issue happens. I have checked
this, reading the phy id is the first time that the phy read/write
functions are used.
So either the datasheet is wrong with respect to the power-up values of
the page select register (which I do not believe), or somebody else is
writing the chip before the kernel comes up, e. g. the BIOS.
Kontron will check the BIOS, however, even they do not know the whole
code nor will they spent unlimited time in helping me with that issue.

However, if this can happen (and it obviously does) and there is no easy
way to change the BIOS (which is often the case) what would be the
disadvantage of simply setting the page select when entering the
igb_get_phy_id() function? This works fine from what I can see and
causes almost no overhead.

Even more, my feeling is that instead of changing the page select back
and forth in some functions (such that everybody needs to now that page
select 0 is the implicit default throughout the driver), wouldn't it
make sense for each function to ensure the correct page selection?
If this causes too much overhead, page select could be encapsulated in
its own functions, keep the current register value and check if a change
is necessary or not.

Anyway, for me it would be great if the probe could at least ensure that
the assumed initial page is selected by calling page select once it is
entered or in the phy id read function.

Please let me know what you think.

Regards,
Jochen

-- 
Henneberg - Systemdesign
Jochen Henneberg
Loehnfeld 26
21423 Winsen (Luhe)
--
Fon: +49 4174 668 773
Mobile: +49 172 160 14 69
Fax: +49 321 210 761 64
www: www.henneberg-systemdesign.com