diff mbox

issue with kernel patch 2a3cdead8b408351fa1e3079b220fa331480ffbc

Message ID 985237851.71212.1461605748986.JavaMail.zimbra@xes-inc.com
State Not Applicable
Headers show

Commit Message

Aaron Sierra April 25, 2016, 5:35 p.m. UTC 
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:


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

Comments

Jochen Henneberg April 26, 2016, 8:02 a.m. UTC | #1 
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
Fujinaka, Todd April 26, 2016, 2:47 p.m. UTC | #2 
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@intel.com
(503) 712-4565


-----Original Message-----
From: Jochen Henneberg [mailto:jh@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@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
Aaron Sierra April 26, 2016, 3:52 p.m. UTC | #3 
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
Jochen Henneberg April 27, 2016, 11:55 a.m. UTC | #4 
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@intel.com
> (503) 712-4565
> 
> 
> -----Original Message-----
> From: Jochen Henneberg [mailto:jh@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@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
> 
> 
>
Jochen Henneberg April 27, 2016, 12:08 p.m. UTC | #5 
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
diff mbox

Patch

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: