| Message ID | 1432054880-20922-3-git-send-email-tharvey@gateworks.com |
|---|---|
| State | Accepted |
| Delegated to: | Joe Hershberger |
| Headers | show |
On Wed, May 20, 2015 at 1:01 AM, Tim Harvey <tharvey@gateworks.com> wrote: > This reverts commit 17da7120249bfdef877f46be5bbcb3cc01212eb9. > > The i210/i211 do have the SW_FW_SYNC (0x5b5c) register and this is what should > be used when acquiring the semaphore. > > I believe the issue that this patch was trying to resolve is now resolved > by properly releasing the semaphore once no longer needed. > > Cc: Marcel Ziswiler <marcel@ziswiler.com> > Cc: Marek Vasut <marex@denx.de> > Cc: Aneesh Bansal <aneesh.bansal@freescale.com> > Cc: Naveen Burmi <NaveenBurmi@freescale.com> > Cc: Po Liu <po.liu@freescale.com> > Cc: Bin Meng <bmeng.cn@gmail.com> > Cc: Alison Wang <alison.wang@freescale.com> > Cc: Reinhard Arlt <reinhard.arlt@esd-electronics.com> > Cc: Shengzhou Liu <Shengzhou.Liu@freescale.com> > Cc: York Sun <yorksun@freescale.com> > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > --- > drivers/net/e1000.c | 6 ++---- > drivers/net/e1000.h | 1 - > 2 files changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/e1000.c b/drivers/net/e1000.c > index a64bc7b..f960024 100644 > --- a/drivers/net/e1000.c > +++ b/drivers/net/e1000.c > @@ -1120,10 +1120,7 @@ e1000_swfw_sync_acquire(struct e1000_hw *hw, uint16_t mask) > if (e1000_get_hw_eeprom_semaphore(hw)) > return -E1000_ERR_SWFW_SYNC; > > - if (hw->mac_type == e1000_igb) > - swfw_sync = E1000_READ_REG(hw, I210_SW_FW_SYNC); > - else > - swfw_sync = E1000_READ_REG(hw, SW_FW_SYNC); > + swfw_sync = E1000_READ_REG(hw, SW_FW_SYNC); > if (!(swfw_sync & (fwmask | swmask))) > break; > > @@ -4458,6 +4455,7 @@ e1000_phy_hw_reset(struct e1000_hw *hw) > > if (hw->mac_type >= e1000_82571) > mdelay(10); > + > } else { > /* Read the Extended Device Control Register, assert the PHY_RESET_DIR > * bit to put the PHY into reset. Then, take it out of reset. > diff --git a/drivers/net/e1000.h b/drivers/net/e1000.h > index f3b77b1..232c95d 100644 > --- a/drivers/net/e1000.h > +++ b/drivers/net/e1000.h > @@ -2496,7 +2496,6 @@ struct e1000_hw { > #define ICH_GFPREG_BASE_MASK 0x1FFF > #define ICH_FLASH_LINEAR_ADDR_MASK 0x00FFFFFF > > -#define E1000_I210_SW_FW_SYNC 0x5B50 /* Software-Firmware Synchronization - RW */ > #define E1000_SW_FW_SYNC 0x05B5C /* Software-Firmware Synchronization - RW */ > > /* SPI EEPROM Status Register */ > -- Tested on QEMU v2.3.0 Tested-by: Bin Meng <bmeng.cn@gmail.com>
On Tue, 2015-05-19 at 10:01 -0700, Tim Harvey wrote: > This reverts commit 17da7120249bfdef877f46be5bbcb3cc01212eb9. > > The i210/i211 do have the SW_FW_SYNC (0x5b5c) register and this is what should > be used when acquiring the semaphore. > > I believe the issue that this patch was trying to resolve is now resolved > by properly releasing the semaphore once no longer needed. > > Cc: Marcel Ziswiler <marcel@ziswiler.com> > Cc: Marek Vasut <marex@denx.de> > Cc: Aneesh Bansal <aneesh.bansal@freescale.com> > Cc: Naveen Burmi <NaveenBurmi@freescale.com> > Cc: Po Liu <po.liu@freescale.com> > Cc: Bin Meng <bmeng.cn@gmail.com> > Cc: Alison Wang <alison.wang@freescale.com> > Cc: Reinhard Arlt <reinhard.arlt@esd-electronics.com> > Cc: Shengzhou Liu <Shengzhou.Liu@freescale.com> > Cc: York Sun <yorksun@freescale.com> > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > --- > drivers/net/e1000.c | 6 ++---- > drivers/net/e1000.h | 1 - > 2 files changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/e1000.c b/drivers/net/e1000.c > index a64bc7b..f960024 100644 > --- a/drivers/net/e1000.c > +++ b/drivers/net/e1000.c > @@ -1120,10 +1120,7 @@ e1000_swfw_sync_acquire(struct e1000_hw *hw, uint16_t mask) > if (e1000_get_hw_eeprom_semaphore(hw)) > return -E1000_ERR_SWFW_SYNC; > > - if (hw->mac_type == e1000_igb) > - swfw_sync = E1000_READ_REG(hw, I210_SW_FW_SYNC); > - else > - swfw_sync = E1000_READ_REG(hw, SW_FW_SYNC); > + swfw_sync = E1000_READ_REG(hw, SW_FW_SYNC); > if (!(swfw_sync & (fwmask | swmask))) > break; > > @@ -4458,6 +4455,7 @@ e1000_phy_hw_reset(struct e1000_hw *hw) > > if (hw->mac_type >= e1000_82571) > mdelay(10); > + > } else { > /* Read the Extended Device Control Register, assert the PHY_RESET_DIR > * bit to put the PHY into reset. Then, take it out of reset. > diff --git a/drivers/net/e1000.h b/drivers/net/e1000.h > index f3b77b1..232c95d 100644 > --- a/drivers/net/e1000.h > +++ b/drivers/net/e1000.h > @@ -2496,7 +2496,6 @@ struct e1000_hw { > #define ICH_GFPREG_BASE_MASK 0x1FFF > #define ICH_FLASH_LINEAR_ADDR_MASK 0x00FFFFFF > > -#define E1000_I210_SW_FW_SYNC 0x5B50 /* Software-Firmware Synchronization - RW */ > #define E1000_SW_FW_SYNC 0x05B5C /* Software-Firmware Synchronization - RW */ > > /* SPI EEPROM Status Register */ Tested on Apalis T30 1GB V1.1A with properly fused i211 Tested on Apalis T30 2GB V1.1A with iNVM fused i210 Tested on Apalis T30 1GB V1.0A with tools only aka non fused i211 Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> --- BTW: Still fails on Apalis T30 2GB V1.0E with tools only aka non fused i210 as follows: e1000: e1000#0: ERROR: Hardware Initialization Failed In our downstream production U-Boot we temporarily hacked this as follows for now: http://git.toradex.com/cgit/u-boot-toradex.git/commit/?h=2015.04-toradex&id=2d8ea651b6da79047b6fa729863d25b5eb9e15d7
On Wed, May 20, 2015 at 4:27 AM, Marcel Ziswiler <marcel@ziswiler.com> wrote: > On Tue, 2015-05-19 at 10:01 -0700, Tim Harvey wrote: >> This reverts commit 17da7120249bfdef877f46be5bbcb3cc01212eb9. >> >> The i210/i211 do have the SW_FW_SYNC (0x5b5c) register and this is what should >> be used when acquiring the semaphore. >> >> I believe the issue that this patch was trying to resolve is now resolved >> by properly releasing the semaphore once no longer needed. >> <snip> > > Tested on Apalis T30 1GB V1.1A with properly fused i211 > Tested on Apalis T30 2GB V1.1A with iNVM fused i210 > Tested on Apalis T30 1GB V1.0A with tools only aka non fused i211 > Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> > --- > BTW: Still fails on Apalis T30 2GB V1.0E with tools only aka non fused > i210 as follows: > e1000: e1000#0: ERROR: Hardware Initialization Failed > In our downstream production U-Boot we temporarily hacked this as > follows for now: > http://git.toradex.com/cgit/u-boot-toradex.git/commit/?h=2015.04-toradex&id=2d8ea651b6da79047b6fa729863d25b5eb9e15d7 > Marcel, I don't understand your results above. What I'm most interested in is if this patch series (adding the proper semaphore release and removing your patch that uses the wrong register for i210) resolves the need for you having added this particular patch for whatever board you needed it for. Is the configuration that was failing for you requiring 17da7120249bfdef877f46be5bbcb3cc01212eb9 resolved with this series applied? When you say it 'still fails on Apalis T30 2GB V1.0E' does that mean you have that particular failure both before and after this patch series? That would indicate to me there is something more needed specifically for that configuration. I'm also not really clear what you mean by 'properly fused i211' vs 'iNVM fused i210' vis 'tools only aka non fused i211'. I believe you are referring to if they are programmed parts or not but I'm not clear if all of your configurations are using internal phy or an external phy. Your downstream patch indicates that in your non-working configuration the EEMNGCTL.CFG_DONE_P0 bit is not getting set indicating (from the datasheet) that the configuration cycle (configuration of SerDes, PHY, PCIe and PLLs) is not done for port 0 which very well may be the expected behavior on a non-programmed part. The configuration I required this series for was for an i210 with internal phy in copper mode. Without this series it would error out with 'ERROR: Hardware Initialization Failed' because e1000_swfw_sync_acquire() would timeout and return -E1000_ERR_SWFW_SYNC. Regards, Tim
On Wed, 2015-05-20 at 06:15 -0700, Tim Harvey wrote: <snip> > > Tested on Apalis T30 1GB V1.1A with properly fused i211 > > Tested on Apalis T30 2GB V1.1A with iNVM fused i210 > > Tested on Apalis T30 1GB V1.0A with tools only aka non fused i211 > > Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> > > --- > > BTW: Still fails on Apalis T30 2GB V1.0E with tools only aka non fused > > i210 as follows: > > e1000: e1000#0: ERROR: Hardware Initialization Failed > > In our downstream production U-Boot we temporarily hacked this as > > follows for now: > > http://git.toradex.com/cgit/u-boot-toradex.git/commit/?h=2015.04-toradex&id=2d8ea651b6da79047b6fa729863d25b5eb9e15d7 > > I don't understand your results above. What I'm most interested in is > if this patch series (adding the proper semaphore release and removing > your patch that uses the wrong register for i210) resolves the need > for you having added this particular patch for whatever board you > needed it for. Is the configuration that was failing for you requiring > 17da7120249bfdef877f46be5bbcb3cc01212eb9 resolved with this series > applied? Yes, exactly. > When you say it 'still fails on Apalis T30 2GB V1.0E' does that mean > you have that particular failure both before and after this patch > series? That would indicate to me there is something more needed > specifically for that configuration. Yes, exactly. As once mentioned before Intel actually claims tools only mode anyway not being operational at all on the other hand the Linux driver worked just fine for us with each and every such combination. Unfortunately so far I did not get to tracking this any further. > I'm also not really clear what you mean by 'properly fused i211' vs > 'iNVM fused i210' vis 'tools only aka non fused i211'. I believe you > are referring to if they are programmed parts or not Yes, exactly. By 'properly fused i211' I mean its iNVM being programmed as it albeit features no other possibility. The i210 on the other hand can be used with an external EEPROM or the iNVM so by 'iNVM fused i210' I mean that I only tested programming the iNVM. I did NOT do any tests with an external EEPROM and therefore also NOT with any activated management stuff or the like requiring external firmware therein. > but I'm not clear > if all of your configurations are using internal phy or an external > phy. Yes, sorry. I forgot to mention this: Our hardware only ever uses the internal copper PHY. > Your downstream patch indicates that in your non-working configuration > the EEMNGCTL.CFG_DONE_P0 bit is not getting set indicating (from the > datasheet) that the configuration cycle (configuration of SerDes, PHY, > PCIe and PLLs) is not done for port 0 which very well may be the > expected behavior on a non-programmed part. Yes, maybe the Linux driver just ignores this condition resp. I actually patched it to ignore failing NVM validation: http://git.toradex.com/cgit/linux-toradex.git/commit/?h=tegra&id=c4c3c7449bdb15c53bfebb0a29c73b24ea810d23 Plus the tools only PCI IDs are anyway missing in Linux: http://git.toradex.com/cgit/linux-toradex.git/commit/?h=tegra&id=2c7123458270c9b3ec9b5ed668f9d55a7f8dbad9 > The configuration I required this series for was for an i210 with > internal phy in copper mode. Without this series it would error out > with 'ERROR: Hardware Initialization Failed' because > e1000_swfw_sync_acquire() would timeout and return > -E1000_ERR_SWFW_SYNC. OK.
On Wed, May 20, 2015 at 7:14 AM, Marcel Ziswiler <marcel@ziswiler.com> wrote: > On Wed, 2015-05-20 at 06:15 -0700, Tim Harvey wrote: > <snip> >> > Tested on Apalis T30 1GB V1.1A with properly fused i211 >> > Tested on Apalis T30 2GB V1.1A with iNVM fused i210 >> > Tested on Apalis T30 1GB V1.0A with tools only aka non fused i211 >> > Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> >> > --- >> > BTW: Still fails on Apalis T30 2GB V1.0E with tools only aka non fused >> > i210 as follows: >> > e1000: e1000#0: ERROR: Hardware Initialization Failed >> > In our downstream production U-Boot we temporarily hacked this as >> > follows for now: >> > http://git.toradex.com/cgit/u-boot-toradex.git/commit/?h=2015.04-toradex&id=2d8ea651b6da79047b6fa729863d25b5eb9e15d7 >> >> I don't understand your results above. What I'm most interested in is >> if this patch series (adding the proper semaphore release and removing >> your patch that uses the wrong register for i210) resolves the need >> for you having added this particular patch for whatever board you >> needed it for. Is the configuration that was failing for you requiring >> 17da7120249bfdef877f46be5bbcb3cc01212eb9 resolved with this series >> applied? > > Yes, exactly. ok - thats great news > >> When you say it 'still fails on Apalis T30 2GB V1.0E' does that mean >> you have that particular failure both before and after this patch >> series? That would indicate to me there is something more needed >> specifically for that configuration. > > Yes, exactly. As once mentioned before Intel actually claims tools only > mode anyway not being operational at all on the other hand the Linux > driver worked just fine for us with each and every such combination. > Unfortunately so far I did not get to tracking this any further. It does make sense to me that an 'unprogrammed' device would work just fine as long as the programmed device-id's were supported by the driver (which they are) and the default mode matches your configuration. All 'programmed' means on an i210/i211 is that you've added some register writes to 'override' power-on defaults. As long as the power-on defaults work for your config then your ok. The default power-on config for i210/i211 is internal phy copper which is what you have. Tim
On Wed, May 20, 2015 at 8:00 AM, Tim Harvey <tharvey@gateworks.com> wrote: > On Wed, May 20, 2015 at 7:14 AM, Marcel Ziswiler <marcel@ziswiler.com> wrote: >> On Wed, 2015-05-20 at 06:15 -0700, Tim Harvey wrote: >> <snip> >>> > Tested on Apalis T30 1GB V1.1A with properly fused i211 >>> > Tested on Apalis T30 2GB V1.1A with iNVM fused i210 >>> > Tested on Apalis T30 1GB V1.0A with tools only aka non fused i211 >>> > Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> >>> > --- >>> > BTW: Still fails on Apalis T30 2GB V1.0E with tools only aka non fused >>> > i210 as follows: >>> > e1000: e1000#0: ERROR: Hardware Initialization Failed >>> > In our downstream production U-Boot we temporarily hacked this as >>> > follows for now: >>> > http://git.toradex.com/cgit/u-boot-toradex.git/commit/?h=2015.04-toradex&id=2d8ea651b6da79047b6fa729863d25b5eb9e15d7 >>> >>> I don't understand your results above. What I'm most interested in is >>> if this patch series (adding the proper semaphore release and removing >>> your patch that uses the wrong register for i210) resolves the need >>> for you having added this particular patch for whatever board you >>> needed it for. Is the configuration that was failing for you requiring >>> 17da7120249bfdef877f46be5bbcb3cc01212eb9 resolved with this series >>> applied? >> >> Yes, exactly. > > ok - thats great news > >> >>> When you say it 'still fails on Apalis T30 2GB V1.0E' does that mean >>> you have that particular failure both before and after this patch >>> series? That would indicate to me there is something more needed >>> specifically for that configuration. >> >> Yes, exactly. As once mentioned before Intel actually claims tools only >> mode anyway not being operational at all on the other hand the Linux >> driver worked just fine for us with each and every such combination. >> Unfortunately so far I did not get to tracking this any further. > > It does make sense to me that an 'unprogrammed' device would work just > fine as long as the programmed device-id's were supported by the > driver (which they are) and the default mode matches your > configuration. All 'programmed' means on an i210/i211 is that you've > added some register writes to 'override' power-on defaults. As long as > the power-on defaults work for your config then your ok. The default > power-on config for i210/i211 is internal phy copper which is what you > have. > > Tim Marcel, Could you give an 'acked-by' if you agree with this series? I would like to see it merged: https://patchwork.ozlabs.org/patch/473997/ https://patchwork.ozlabs.org/patch/473998/ https://patchwork.ozlabs.org/patch/473996/ Regards, Tim
On Fri, 2015-07-10 at 08:47 -0700, Tim Harvey wrote: > > Marcel, > > Could you give an 'acked-by' if you agree with this series? I would > like to see it merged: > > https://patchwork.ozlabs.org/patch/473997/ > https://patchwork.ozlabs.org/patch/473998/ > https://patchwork.ozlabs.org/patch/473996/ > > Regards, > > Tim Hi Tim Sure. Sorry, I thought my previously given tested-by would be stronger than just an acked-by. Here you go for the whole series: Acked-by Marcel Ziswiler <marcel.ziswiler@toradex.com> Cheers Marcel
On Sat, Jul 11, 2015 at 3:11 PM, Marcel Ziswiler <marcel@ziswiler.com> wrote: > On Fri, 2015-07-10 at 08:47 -0700, Tim Harvey wrote: >> >> Marcel, >> >> Could you give an 'acked-by' if you agree with this series? I would >> like to see it merged: >> >> https://patchwork.ozlabs.org/patch/473997/ >> https://patchwork.ozlabs.org/patch/473998/ >> https://patchwork.ozlabs.org/patch/473996/ >> >> Regards, >> >> Tim > > Hi Tim > > Sure. Sorry, I thought my previously given tested-by would be stronger > than just an acked-by. Here you go for the whole series: > > Acked-by Marcel Ziswiler <marcel.ziswiler@toradex.com> > > Cheers > > Marcel > Joe, Can you take a look at this series? You are the current delegate in patchwork. Thanks, Tim
On Mon, Jul 13, 2015 at 8:45 AM, Tim Harvey <tharvey@gateworks.com> wrote: > On Sat, Jul 11, 2015 at 3:11 PM, Marcel Ziswiler <marcel@ziswiler.com> wrote: >> On Fri, 2015-07-10 at 08:47 -0700, Tim Harvey wrote: >>> >>> Marcel, >>> >>> Could you give an 'acked-by' if you agree with this series? I would >>> like to see it merged: >>> >>> https://patchwork.ozlabs.org/patch/473997/ >>> https://patchwork.ozlabs.org/patch/473998/ >>> https://patchwork.ozlabs.org/patch/473996/ >>> >>> Regards, >>> >>> Tim >> >> Hi Tim >> >> Sure. Sorry, I thought my previously given tested-by would be stronger >> than just an acked-by. Here you go for the whole series: >> >> Acked-by Marcel Ziswiler <marcel.ziswiler@toradex.com> >> >> Cheers >> >> Marcel >> > > Joe, > > Can you take a look at this series? You are the current delegate in patchwork. > > Thanks, > > Tim Tom, Joe seems to be MIA - can you take a look at these please? https://patchwork.ozlabs.org/patch/473997/ https://patchwork.ozlabs.org/patch/473998/ https://patchwork.ozlabs.org/patch/473996/ Thanks, Tim
Hi Tim, On Tue, May 19, 2015 at 12:01 PM, Tim Harvey <tharvey@gateworks.com> wrote: > This reverts commit 17da7120249bfdef877f46be5bbcb3cc01212eb9. > > The i210/i211 do have the SW_FW_SYNC (0x5b5c) register and this is what should > be used when acquiring the semaphore. > > I believe the issue that this patch was trying to resolve is now resolved > by properly releasing the semaphore once no longer needed. > > Cc: Marcel Ziswiler <marcel@ziswiler.com> > Cc: Marek Vasut <marex@denx.de> > Cc: Aneesh Bansal <aneesh.bansal@freescale.com> > Cc: Naveen Burmi <NaveenBurmi@freescale.com> > Cc: Po Liu <po.liu@freescale.com> > Cc: Bin Meng <bmeng.cn@gmail.com> > Cc: Alison Wang <alison.wang@freescale.com> > Cc: Reinhard Arlt <reinhard.arlt@esd-electronics.com> > Cc: Shengzhou Liu <Shengzhou.Liu@freescale.com> > Cc: York Sun <yorksun@freescale.com> > Signed-off-by: Tim Harvey <tharvey@gateworks.com> Applied to u-boot-net, thanks! -Joe
diff --git a/drivers/net/e1000.c b/drivers/net/e1000.c index a64bc7b..f960024 100644 --- a/drivers/net/e1000.c +++ b/drivers/net/e1000.c @@ -1120,10 +1120,7 @@ e1000_swfw_sync_acquire(struct e1000_hw *hw, uint16_t mask) if (e1000_get_hw_eeprom_semaphore(hw)) return -E1000_ERR_SWFW_SYNC; - if (hw->mac_type == e1000_igb) - swfw_sync = E1000_READ_REG(hw, I210_SW_FW_SYNC); - else - swfw_sync = E1000_READ_REG(hw, SW_FW_SYNC); + swfw_sync = E1000_READ_REG(hw, SW_FW_SYNC); if (!(swfw_sync & (fwmask | swmask))) break; @@ -4458,6 +4455,7 @@ e1000_phy_hw_reset(struct e1000_hw *hw) if (hw->mac_type >= e1000_82571) mdelay(10); + } else { /* Read the Extended Device Control Register, assert the PHY_RESET_DIR * bit to put the PHY into reset. Then, take it out of reset. diff --git a/drivers/net/e1000.h b/drivers/net/e1000.h index f3b77b1..232c95d 100644 --- a/drivers/net/e1000.h +++ b/drivers/net/e1000.h @@ -2496,7 +2496,6 @@ struct e1000_hw { #define ICH_GFPREG_BASE_MASK 0x1FFF #define ICH_FLASH_LINEAR_ADDR_MASK 0x00FFFFFF -#define E1000_I210_SW_FW_SYNC 0x5B50 /* Software-Firmware Synchronization - RW */ #define E1000_SW_FW_SYNC 0x05B5C /* Software-Firmware Synchronization - RW */ /* SPI EEPROM Status Register */
This reverts commit 17da7120249bfdef877f46be5bbcb3cc01212eb9. The i210/i211 do have the SW_FW_SYNC (0x5b5c) register and this is what should be used when acquiring the semaphore. I believe the issue that this patch was trying to resolve is now resolved by properly releasing the semaphore once no longer needed. Cc: Marcel Ziswiler <marcel@ziswiler.com> Cc: Marek Vasut <marex@denx.de> Cc: Aneesh Bansal <aneesh.bansal@freescale.com> Cc: Naveen Burmi <NaveenBurmi@freescale.com> Cc: Po Liu <po.liu@freescale.com> Cc: Bin Meng <bmeng.cn@gmail.com> Cc: Alison Wang <alison.wang@freescale.com> Cc: Reinhard Arlt <reinhard.arlt@esd-electronics.com> Cc: Shengzhou Liu <Shengzhou.Liu@freescale.com> Cc: York Sun <yorksun@freescale.com> Signed-off-by: Tim Harvey <tharvey@gateworks.com> --- drivers/net/e1000.c | 6 ++---- drivers/net/e1000.h | 1 - 2 files changed, 2 insertions(+), 5 deletions(-)