RE: [PATCH v1 net-next] microchip_t1: Add driver for Microchip LAN87XX T1 PHYs

[Nisar.Sayed]

Hi Florian 

> > diff --git a/drivers/net/phy/microchip_t1.c
> > b/drivers/net/phy/microchip_t1.c new file mode 100644 index
> > 000..1f6f299
> > --- /dev/null
> > +++ b/drivers/net/phy/microchip_t1.c
> > @@ -0,0 +1,88 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> 
> This is not the standard comment for a .c file, it should be: // (C++ style)
> 

Thanks, will update it.

> > +/*
> > + * Copyright (C) 2018 Microchip Technology
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, see .
> 
> That needs to go away now that you used SPDX
> 

Ok fine will remove it.

> > + */
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +/* Interrupt Source Register */
> > +#define LAN87XX_INTERRUPT_SOURCE(0x18)
> > +
> > +/* Interrupt Mask Register */
> > +#define LAN87XX_INTERRUPT_MASK  (0x19)
> > +#define LAN87XX_MASK_LINK_UP(0x0004)
> > +#define LAN87XX_MASK_LINK_DOWN  (0x0002)
> > +
> > +#define DRIVER_AUTHOR  "Nisar Sayed "
> > +#define DRIVER_DESC"Microchip LAN87XX T1 PHY driver"
> > +
> > +static int lan87xx_phy_config_intr(struct phy_device *phydev) {
> > +   int rc, val = 0;
> > +
> > +   if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> > +   /* unmask all source and clear them before enable */
> > +   rc = phy_write(phydev, LAN87XX_INTERRUPT_MASK,
> 0x7FFF);
> > +   rc = phy_read(phydev, LAN87XX_INTERRUPT_SOURCE);
> > +   val = (LAN87XX_MASK_LINK_UP |
> LAN87XX_MASK_LINK_DOWN);
> 
> The parenthesis are not necessary here.

Thanks, will remove as suggested.

> 
> > +   }
> > +
> > +   rc = phy_write(phydev, LAN87XX_INTERRUPT_MASK, val);
> > +
> > +   return rc < 0 ? rc : 0;
> > +}
> > +
> > +static int lan87xx_phy_ack_interrupt(struct phy_device *phydev) {
> > +   int rc = phy_read(phydev, LAN87XX_INTERRUPT_SOURCE);
> > +
> > +   return rc < 0 ? rc : 0;
> > +}
> > +
> > +static struct phy_driver microchip_t1_phy_driver[] = {
> > +   {
> > +   .phy_id = 0x0007c150,
> > +   .phy_id_mask= 0xfff0,
> > +   .name   = "Microchip LAN87xx",
> 
> Would you want to name this "Microchip LAN87xx T1"?

Thanks, yes it's good to mention, will update and send v2

- Nisar

RE: [PATCH net-next] microchipT1phy: Add driver for Microchip LAN87XX T1 PHYs

[Nisar.Sayed]

Hi Andrew,

> > Fine, will change the filename.
> 
> > The reason for moving to separate file is that we have a series of
> > T1 standard PHYs, which support cable diagnostics, signal quality
> > indicator(SQI) and sleep and wakeup (TC10) support etc. we planned to
> > keep all T1 standard PHYs separate to support additional features
> > supported by these PHYs.
> 
> Is there anything shared with the other microchip PHYs? If there is potential
> for code sharing, you should do it.

Yes, there will be no code sharing between existing microchip PHYs and the 
newly getting added T1 phys.

> 
> > > > + */
> > > > +#ifndef _MICROCHIPT1PHY_H_
> > > > +#define _MICROCHIPT1PHY_H_
> > > > +
> > > > +/* Interrupt Source Register */
> > > > +#define LAN87XX_INTERRUPT_SOURCE(0x18)
> > > > +
> > > > +/* Interrupt Mask Register */
> > > > +#define LAN87XX_INTERRUPT_MASK  (0x19)
> > > > +#define LAN87XX_MASK_LINK_UP(0x0004)
> > > > +#define LAN87XX_MASK_LINK_DOWN  (0x0002)
> > >
> > > What's the point of that header file if all definitions are consumed
> > > by the same driver?
> > >
> >
> > We have planned a series of patches where we planned to use this further.
> 
> Are you adding multiple files which share the header? If not, just add the
> defines to the C code.
> 
> Andrew

We have a plan, I think as you suggested better to go with defines in C codes 
itself now.
Maybe we can create/move during future submissions.

- Nisar

RE: [PATCH net-next] microchipT1phy: Add driver for Microchip LAN87XX T1 PHYs

[Nisar.Sayed]

Hi Florian

> On 04/25/2018 11:49 AM, Nisar Sayed wrote:
> > Add driver for Microchip LAN87XX T1 PHYs
> >
> > This patch support driver for Microchp T1 PHYs.
> > There will be followup patches to this driver to support T1 PHY
> > features such as cable diagnostics, signal quality indicator(SQI),
> > sleep and wakeup (TC10) support.
> >
> > Signed-off-by: Nisar Sayed 
> > ---
> >  drivers/net/phy/Kconfig  |   5 ++
> >  drivers/net/phy/Makefile |   1 +
> >  drivers/net/phy/microchipT1phy.c | 116
> +++
> >  include/linux/microchipT1phy.h   |  28 ++
> >  4 files changed, 150 insertions(+)
> >  create mode 100644 drivers/net/phy/microchipT1phy.c  create mode
> > 100644 include/linux/microchipT1phy.h
> >
> > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index
> > bdfbabb..7b0b351 100644
> > --- a/drivers/net/phy/Kconfig
> > +++ b/drivers/net/phy/Kconfig
> > @@ -354,6 +354,11 @@ config MICROCHIP_PHY
> > help
> >   Supports the LAN88XX PHYs.
> >
> > +config MICROCHIP_T1_PHY
> > +   tristate "Microchip T1 PHYs"
> > +   ---help---
> > + Supports the LAN87XX PHYs.
> > +
> >  config MICROSEMI_PHY
> > tristate "Microsemi PHYs"
> > ---help---
> > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile index
> > 01acbcb..5706613 100644
> > --- a/drivers/net/phy/Makefile
> > +++ b/drivers/net/phy/Makefile
> > @@ -70,6 +70,7 @@ obj-$(CONFIG_MESON_GXL_PHY)   += meson-
> gxl.o
> >  obj-$(CONFIG_MICREL_KS8995MA)  += spi_ks8995.o
> >  obj-$(CONFIG_MICREL_PHY)   += micrel.o
> >  obj-$(CONFIG_MICROCHIP_PHY)+= microchip.o
> > +obj-$(CONFIG_MICROCHIP_T1_PHY) += microchipT1phy.o
> >  obj-$(CONFIG_MICROSEMI_PHY)+= mscc.o
> >  obj-$(CONFIG_NATIONAL_PHY) += national.o
> >  obj-$(CONFIG_QSEMI_PHY)+= qsemi.o
> > diff --git a/drivers/net/phy/microchipT1phy.c
> > b/drivers/net/phy/microchipT1phy.c
> > new file mode 100644
> > index 000..15fbb04
> > --- /dev/null
> > +++ b/drivers/net/phy/microchipT1phy.c
> 
> Can we avoid CamelCase file names? Any reasons why this is not part of
> microchip.c?
> 

Fine, will change the filename.
The reason for moving to separate file is that we have a series of T1 standard 
PHYs, which support cable diagnostics, signal quality indicator(SQI) and sleep 
and wakeup (TC10) support etc. we planned to keep all T1 standard PHYs separate 
to support additional features supported by these PHYs.

> > @@ -0,0 +1,116 @@
> > +/*
> > + * Copyright (C) 2018 Microchip Technology
> 
> Please use a SPDX license tag.
> 

Thanks, will update.

> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, see .
> > + */
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#define DRIVER_AUTHOR  "Nisar Sayed "
> > +#define DRIVER_DESC"Microchip LAN87XX T1 PHY driver"
> > +
> > +struct lan87xx_priv {
> > +   int chip_id;
> > +   int chip_rev;
> > +};
> > +
> > +static int lan87xx_phy_config_intr(struct phy_device *phydev) {
> > +   int rc = 0;
> > +
> > +   if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> > +   /* unmask all source and clear them before enable */
> > +   rc = phy_write(phydev, LAN87XX_INTERRUPT_MASK,
> 0x7FFF);
> > +   rc = phy_read(phydev, LAN87XX_INTERRUPT_SOURCE);
> > +   rc = phy_write(phydev, LAN87XX_INTERRUPT_MASK,
> > +  LAN87XX_MASK_LINK_UP |
> LAN87XX_MASK_LINK_DOWN);
> > +   } else {
> > +   rc = phy_write(phydev, LAN87XX_INTERRUPT_MASK, 0);
> > +   }
> 
> Since the last thing you do in both branches is write to
> LAN87XX_INTERRUPT_MASK, just move that write outside of the branches
> and make sure the value you will set is correct in both cases.
> 

Thanks, Sure will modify as suggested.

> > +
> > +   return rc < 0 ? rc : 0;
> > +}
> > +
> > +static int lan87xx_phy_ack_interrupt(struct phy_device *phydev) {
> > +   int rc = phy_read(phydev, LAN87XX_INTERRUPT_SOURCE);
> > +
> > +   return rc < 0 ? rc : 0;
> > +}
> > +
> > +static int lan87xx_probe(struct phy_device *phydev) {
> > +   struct device *dev = &phydev->mdio.dev;
> > +   struct lan87xx_priv *priv;
> > +
> > +   priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +   if (!priv)
> > +   return -ENOMEM;
> > +
> > +   /* these values

RE: [PATCH] lan78xx: Don't reset the interface on open

[Nisar.Sayed]

Hi Phil,

> Hi Nisar,
> 
> On 10/04/2018 15:16, nisar.sa...@microchip.com wrote:
> > Thanks Phil, for identifying the issues.
> >
> >> -  ret = lan78xx_reset(dev);
> >> -  if (ret < 0)
> >> -  goto done;
> >> -
> >>phy_start(net->phydev);
> >>
> >>netif_dbg(dev, ifup, dev->net, "phy initialised successfully");
> >> --
> >
> > You may need to start the interrupts before "phy_start" instead of
> suppressing call to "lan78xx_reset".
> >
> > + if (dev->domain_data.phyirq > 0)
> > + phy_start_interrupts(dev->net->phydev);
> 
> Shouldn't phy_connect_direct, called from lan78xx_phy_init, already have
> enabled interrupts for us?
> 
> This patch addresses two problems - time wasted by renegotiating the link
> after the reset and the missed interrupt - and I'd like both to be fixed. 
> Unless
> you can come up with a good reason for performing the reset from the open
> handler I think it should be removed.
> 
> Phil

Thanks, we have verified suspected test cases and these are passed, the changes 
are good to go.

- Nisar

RE: [PATCH] lan78xx: Don't reset the interface on open

[Nisar.Sayed]

Thanks Phil, for identifying the issues.

> - ret = lan78xx_reset(dev);
> - if (ret < 0)
> - goto done;
> -
>   phy_start(net->phydev);
> 
>   netif_dbg(dev, ifup, dev->net, "phy initialised successfully");
> --

You may need to start the interrupts before "phy_start" instead of suppressing 
call to "lan78xx_reset".

+ if (dev->domain_data.phyirq > 0)
+ phy_start_interrupts(dev->net->phydev);

- Nisar

RE: [PATCH] lan78xx: Connect phy early

[Nisar.Sayed]

Hi Alexander,

Thanks for the patch.

> @@ -2575,13 +2571,7 @@ static int lan78xx_stop(struct net_device *net)
>   if (timer_pending(&dev->stat_monitor))
>   del_timer_sync(&dev->stat_monitor);
> 
> - phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, 0xfff0);
> - phy_unregister_fixup_for_uid(PHY_LAN8835, 0xfff0);
> -
>   phy_stop(net->phydev);
> - phy_disconnect(net->phydev);
> -
> - net->phydev = NULL;
> 
>   clear_bit(EVENT_DEV_OPEN, &dev->flags);
>   netif_stop_queue(net);

Please do add valid "phydev"  check before phy_stop, since "phy_disconnect" 
should be called before "unregister_netdev"

+ if (net->phydev)
phy_stop(net->phydev);

> @@ -3481,6 +3471,11 @@ static void lan78xx_disconnect(struct
> usb_interface *intf)
>   net = dev->net;
>   unregister_netdev(net);
> 
> + phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, 0xfff0);
> + phy_unregister_fixup_for_uid(PHY_LAN8835, 0xfff0);
> +
> + phy_disconnect(net->phydev);
> +
>   cancel_delayed_work_sync(&dev->wq);
> 
>   usb_scuttle_anchored_urbs(&dev->deferred);

Please move "unregister_netdev" after "phy_disconnect", otherwise 
"phy_disconnect" will fail while we disconnect USB.

> @@ -3634,6 +3629,10 @@ static int lan78xx_probe(struct usb_interface
> *intf,
>   pm_runtime_set_autosuspend_delay(&udev->dev,
>DEFAULT_AUTOSUSPEND_DELAY);
> 
> + ret = lan78xx_phy_init(dev);
> + if (ret < 0)
> + return ret;
> +
>   return 0;
> 
>  out3:
> --
> 2.12.3

We should "goto out4" instead of "return" upon "lan78xx_phy_init" fail

+ out4:
+   unregister_netdev(netdev);

In addition to current changes, you might have to take care of the following.

In function "lan78xx_reset_resume"

-  lan78xx_phy_init(dev);
+ phy_start(dev->net->phydev);

Thanks,
Sd.Nisar

RE: [PATCH v4 net 2/3] lan78xx: Allow EEPROM write for less than MAX_EEPROM_SIZE

[Nisar.Sayed]

Thanks Sergei, I will update it and submit next version.

- Nisar

 > Hello!
> 
> On 09/19/2017 01:02 AM, Nisar Sayed wrote:
> 
> > Allow EEPROM write for less than MAX_EEPROM_SIZE
> >
> > Fixes: 55d7de9de6c3 ("Microchip's LAN7800 family USB 2/3 to
> > 10/100/1000 Ethernet device driver")
> > Signed-off-by: Nisar Sayed 
> > ---
> >   drivers/net/usb/lan78xx.c | 9 -
> >   1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> > index fcf85ae37435..3292f56ffe02 100644
> > --- a/drivers/net/usb/lan78xx.c
> > +++ b/drivers/net/usb/lan78xx.c
> > @@ -1290,11 +1290,10 @@ static int lan78xx_ethtool_set_eeprom(struct
> net_device *netdev,
> > if (ret)
> > return ret;
> >
> > -   /* Allow entire eeprom update only */
> > -   if ((ee->magic == LAN78XX_EEPROM_MAGIC) &&
> > -   (ee->offset == 0) &&
> > -   (ee->len == 512) &&
> > -   (data[0] == EEPROM_INDICATOR))
> > +   /* Invalid EEPROM_INDICATOR at offset zero will result in fail to
> 
> s/fail/a failure/.
> 
> > +* load data from EEPROM
> > +*/
> > +   if (ee->magic == LAN78XX_EEPROM_MAGIC)
> > ret = lan78xx_write_raw_eeprom(dev, ee->offset, ee->len,
> data);
> > else if ((ee->magic == LAN78XX_OTP_MAGIC) &&
> >  (ee->offset == 0) &&
> >
> 
> MBR, Sergei

RE: [PATCH v2 net 1/3] lan78xx: Fix for eeprom read/write when device auto suspend

[Nisar.Sayed]

> > From: Nisar Sayed 
> >
> > Fix for eeprom read/write when device auto suspend
> >
> > Fixes: 55d7de9de6c3 ("Microchip's LAN7800 family USB 2/3 to
> > 10/100/1000 Ethernet device driver")
> > Signed-off-by: Nisar Sayed 
> > ---
> >  drivers/net/usb/lan78xx.c | 22 ++
> >  1 file changed, 18 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> > index b99a7fb..baf91c7 100644
> > --- a/drivers/net/usb/lan78xx.c
> > +++ b/drivers/net/usb/lan78xx.c
> > @@ -1265,30 +1265,44 @@ static int lan78xx_ethtool_get_eeprom(struct
> net_device *netdev,
> >   struct ethtool_eeprom *ee, u8 *data)  {
> > struct lan78xx_net *dev = netdev_priv(netdev);
> > +   int ret = -EINVAL;
> > +
> > +   if (usb_autopm_get_interface(dev->intf) < 0)
> > +   return ret;
> 
> Hi Nisar
> 
> It is better to do
> 
>ret = usb_autopm_get_interface(dev->intf;
>if (ret)
> return ret;
> 
> i.e. use the error code usb_autopm_get_interface() gives you.
> 
> > ee->magic = LAN78XX_EEPROM_MAGIC;
> >
> > -   return lan78xx_read_raw_eeprom(dev, ee->offset, ee->len, data);
> > +   ret = lan78xx_read_raw_eeprom(dev, ee->offset, ee->len, data);
> > +
> > +   usb_autopm_put_interface(dev->intf);
> > +
> > +   return ret;
> >  }
> >
> >  static int lan78xx_ethtool_set_eeprom(struct net_device *netdev,
> >   struct ethtool_eeprom *ee, u8 *data)  {
> > struct lan78xx_net *dev = netdev_priv(netdev);
> > +   int ret = -EINVAL;
> > +
> > +   if (usb_autopm_get_interface(dev->intf) < 0)
> > +   return ret;
> 
> Same here.
> 
>  Andrew

Thanks Andrew, will update it.

- Nisar

[PATCH v2 net 2/3] lan78xx: Allow EEPROM write for less than MAX_EEPROM_SIZE

[Nisar.Sayed]

From: Nisar Sayed 

Allow EEPROM write for less than MAX_EEPROM_SIZE

Fixes: 55d7de9de6c3 ("Microchip's LAN7800 family USB 2/3 to 10/100/1000 
Ethernet device driver")
Signed-off-by: Nisar Sayed 
---
 drivers/net/usb/lan78xx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index baf91c7..02d64f75 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -1290,8 +1290,8 @@ static int lan78xx_ethtool_set_eeprom(struct net_device 
*netdev,
 
/* Allow entire eeprom update only */
if ((ee->magic == LAN78XX_EEPROM_MAGIC) &&
-   (ee->offset == 0) &&
-   (ee->len == 512) &&
+   (ee->offset >= 0 && ee->offset < MAX_EEPROM_SIZE) &&
+   (ee->len > 0 && (ee->offset + ee->len) <= MAX_EEPROM_SIZE) &&
(data[0] == EEPROM_INDICATOR))
ret = lan78xx_write_raw_eeprom(dev, ee->offset, ee->len, data);
else if ((ee->magic == LAN78XX_OTP_MAGIC) &&
-- 
1.9.1

[PATCH v2 net 3/3] lan78xx: Use default value loaded from EEPROM/OTP when resetting

[Nisar.Sayed]

From: Nisar Sayed 

Use default value loaded from EEPROM/OTP when resetting

The LAN78xx allows platform configurations to be loaded from EEPROM/OTP.
Ex: When external phy is connected, the MAC can be configured to
have speed, duplex, polarity configurations loadded from the EEPROM/OTP.

Fixes: 55d7de9de6c3 ("Microchip's LAN7800 family USB 2/3 to 10/100/1000 
Ethernet device driver")
Signed-off-by: Nisar Sayed 
---
 drivers/net/usb/lan78xx.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 02d64f75..ffe302a 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -2448,7 +2448,6 @@ static int lan78xx_reset(struct lan78xx_net *dev)
/* LAN7801 only has RGMII mode */
if (dev->chipid == ID_REV_CHIP_ID_7801_)
buf &= ~MAC_CR_GMII_EN_;
-   buf |= MAC_CR_AUTO_DUPLEX_ | MAC_CR_AUTO_SPEED_;
ret = lan78xx_write_reg(dev, MAC_CR, buf);
 
ret = lan78xx_read_reg(dev, MAC_TX, &buf);
-- 
1.9.1

[PATCH v2 net 1/3] lan78xx: Fix for eeprom read/write when device auto suspend

[Nisar.Sayed]

From: Nisar Sayed 

Fix for eeprom read/write when device auto suspend

Fixes: 55d7de9de6c3 ("Microchip's LAN7800 family USB 2/3 to 10/100/1000 
Ethernet device driver")
Signed-off-by: Nisar Sayed 
---
 drivers/net/usb/lan78xx.c | 22 ++
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index b99a7fb..baf91c7 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -1265,30 +1265,44 @@ static int lan78xx_ethtool_get_eeprom(struct net_device 
*netdev,
  struct ethtool_eeprom *ee, u8 *data)
 {
struct lan78xx_net *dev = netdev_priv(netdev);
+   int ret = -EINVAL;
+
+   if (usb_autopm_get_interface(dev->intf) < 0)
+   return ret;
 
ee->magic = LAN78XX_EEPROM_MAGIC;
 
-   return lan78xx_read_raw_eeprom(dev, ee->offset, ee->len, data);
+   ret = lan78xx_read_raw_eeprom(dev, ee->offset, ee->len, data);
+
+   usb_autopm_put_interface(dev->intf);
+
+   return ret;
 }
 
 static int lan78xx_ethtool_set_eeprom(struct net_device *netdev,
  struct ethtool_eeprom *ee, u8 *data)
 {
struct lan78xx_net *dev = netdev_priv(netdev);
+   int ret = -EINVAL;
+
+   if (usb_autopm_get_interface(dev->intf) < 0)
+   return ret;
 
/* Allow entire eeprom update only */
if ((ee->magic == LAN78XX_EEPROM_MAGIC) &&
(ee->offset == 0) &&
(ee->len == 512) &&
(data[0] == EEPROM_INDICATOR))
-   return lan78xx_write_raw_eeprom(dev, ee->offset, ee->len, data);
+   ret = lan78xx_write_raw_eeprom(dev, ee->offset, ee->len, data);
else if ((ee->magic == LAN78XX_OTP_MAGIC) &&
 (ee->offset == 0) &&
 (ee->len == 512) &&
 (data[0] == OTP_INDICATOR_1))
-   return lan78xx_write_raw_otp(dev, ee->offset, ee->len, data);
+   ret = lan78xx_write_raw_otp(dev, ee->offset, ee->len, data);
 
-   return -EINVAL;
+   usb_autopm_put_interface(dev->intf);
+
+   return ret;
 }
 
 static void lan78xx_get_strings(struct net_device *netdev, u32 stringset,
-- 
1.9.1

[PATCH v2 net 0/3] lan78xx: Fixes to lan78xx driver

[Nisar.Sayed]

From: Nisar Sayed 

This series of patches are for lan78xx driver.

These patches fixes potential issues associated with lan78xx driver

v2
- Added patch version information
- Added fixes tag
- Updated patch description
- Updated changes as per comments

v1
- Spitted patches as per comments
- Dropped "fixed_phy device support" and "Fix for system suspend" changes

Nisar Sayed (3):
  Fix for eeprom read/write when device auto suspend
  Allow EEPROM write for less than MAX_EEPROM_SIZE
  Use default value loaded from EEPROM/OTP when resetting

 drivers/net/usb/lan78xx.c | 27 ---
 1 file changed, 20 insertions(+), 7 deletions(-)

-- 
1.9.1

[PATCH v2 net] smsc95xx: Configure pause time to 0xffff when tx flow control enabled

[Nisar.Sayed]

From: Nisar Sayed 

Configure pause time to 0x when tx flow control enabled

Set pause time to 0x in the pause frame to indicate the
partner to stop sending the packets. When RX buffer frees up,
the device sends pause frame with pause time zero for partner to
resume transmission.

Fixes: 2f7ca802bdae ("Add SMSC LAN9500 USB2.0 10/100 ethernet adapter driver")
Signed-off-by: Nisar Sayed 
---
v0 -> v1:
 * Added patch description in detail.
v1 -> v2:
 * Added fixes tag
---
 drivers/net/usb/smsc95xx.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 340c134..309b88a 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -526,7 +526,7 @@ static void smsc95xx_set_multicast(struct net_device 
*netdev)
 static int smsc95xx_phy_update_flowcontrol(struct usbnet *dev, u8 duplex,
   u16 lcladv, u16 rmtadv)
 {
-   u32 flow, afc_cfg = 0;
+   u32 flow = 0, afc_cfg;
 
int ret = smsc95xx_read_reg(dev, AFC_CFG, &afc_cfg);
if (ret < 0)
@@ -537,20 +537,19 @@ static int smsc95xx_phy_update_flowcontrol(struct usbnet 
*dev, u8 duplex,
 
if (cap & FLOW_CTRL_RX)
flow = 0x0002;
-   else
-   flow = 0;
 
-   if (cap & FLOW_CTRL_TX)
+   if (cap & FLOW_CTRL_TX) {
afc_cfg |= 0xF;
-   else
+   flow |= 0x;
+   } else {
afc_cfg &= ~0xF;
+   }
 
netif_dbg(dev, link, dev->net, "rx pause %s, tx pause %s\n",
   cap & FLOW_CTRL_RX ? "enabled" : "disabled",
   cap & FLOW_CTRL_TX ? "enabled" : "disabled");
} else {
netif_dbg(dev, link, dev->net, "half duplex\n");
-   flow = 0;
afc_cfg |= 0xF;
}
 
-- 
1.9.1

RE: [PATCH v1 net] smsc95xx: Configure pause time to 0xffff when tx flow control enabled

[Nisar.Sayed]

> On Mon, Sep 11, 2017 at 12:32:10PM +, nisar.sa...@microchip.com
> wrote:
> > From: Nisar Sayed 
> >
> > Configure pause time to 0x when tx flow control enabled
> >
> > Set pause time to 0x in the pause frame to indicate the partner to
> > stop sending the packets. When RX buffer frees up, the device sends
> > pause frame with pause time zero for partner to resume transmission.
> 
> Hi Nisar
> 
> Thanks for the updated description. Since you are posting this for net, not
> net-next, could you add a fixes: tag?
> 
> Thanks
>   Andrew

Thanks Andrew, yes I will add and will submit next revision

- Nisar

[PATCH v1 net] smsc95xx: Configure pause time to 0xffff when tx flow control enabled

[Nisar.Sayed]

From: Nisar Sayed 

Configure pause time to 0x when tx flow control enabled

Set pause time to 0x in the pause frame to indicate the
partner to stop sending the packets. When RX buffer frees up,
the device sends pause frame with pause time zero for partner to
resume transmission.

Signed-off-by: Nisar Sayed 
---
v0 -> v1:
 * Added patch description in detail.
---
 drivers/net/usb/smsc95xx.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 340c134..309b88a 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -526,7 +526,7 @@ static void smsc95xx_set_multicast(struct net_device 
*netdev)
 static int smsc95xx_phy_update_flowcontrol(struct usbnet *dev, u8 duplex,
   u16 lcladv, u16 rmtadv)
 {
-   u32 flow, afc_cfg = 0;
+   u32 flow = 0, afc_cfg;
 
int ret = smsc95xx_read_reg(dev, AFC_CFG, &afc_cfg);
if (ret < 0)
@@ -537,20 +537,19 @@ static int smsc95xx_phy_update_flowcontrol(struct usbnet 
*dev, u8 duplex,
 
if (cap & FLOW_CTRL_RX)
flow = 0x0002;
-   else
-   flow = 0;
 
-   if (cap & FLOW_CTRL_TX)
+   if (cap & FLOW_CTRL_TX) {
afc_cfg |= 0xF;
-   else
+   flow |= 0x;
+   } else {
afc_cfg &= ~0xF;
+   }
 
netif_dbg(dev, link, dev->net, "rx pause %s, tx pause %s\n",
   cap & FLOW_CTRL_RX ? "enabled" : "disabled",
   cap & FLOW_CTRL_TX ? "enabled" : "disabled");
} else {
netif_dbg(dev, link, dev->net, "half duplex\n");
-   flow = 0;
afc_cfg |= 0xF;
}
 
-- 
1.9.1

RE: [PATCH 3/3] lan78xx: Use default value loaded from EEPROM/OTP when resetting

[Nisar.Sayed]

> On Thu, Sep 07, 2017 at 07:11:50AM +, nisar.sa...@microchip.com
> wrote:
> > From: Nisar Sayed 
> >
> > Use default value loaded from EEPROM/OTP when resetting
> 
> Hi Nisar
> 
> Subject: [PATCH 3/3]
> 
> Is this a fix for net, or further development for net-next?
> 
> Why do we want the default values?
> 
> Andrew

Thanks Andrew,

Yes it is for "net", sorry missed to include it, will update in next version of 
submit.

These bits are "reset protected" and should not be modified and must use the
Must be configured from EEPROM only. Will update the description.

- Nisar

RE: [PATCH net] smsc95xx: Configure pause time to 0xffff when tx flow control enabled

[Nisar.Sayed]

> On Thu, Sep 07, 2017 at 06:51:37AM +, nisar.sa...@microchip.com
> wrote:
> > From: Nisar Sayed 
> >
> > Configure pause time to 0x when tx flow control enabled
> 
> Hi Nisar
> 
> You should explain the 'Why' in the commit message. Why do we want a
> pause time of 0x?
> 
>   Andrew

Thanks Andrew,

I shall update the description and submit next version.

- Nisar

[PATCH 3/3] lan78xx: Use default value loaded from EEPROM/OTP when resetting

[Nisar.Sayed]

From: Nisar Sayed 

Use default value loaded from EEPROM/OTP when resetting

Signed-off-by: Nisar Sayed 
---
 drivers/net/usb/lan78xx.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 94ef943..8fd7c2f 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -2452,7 +2452,6 @@ static int lan78xx_reset(struct lan78xx_net *dev)
/* LAN7801 only has RGMII mode */
if (dev->chipid == ID_REV_CHIP_ID_7801_)
buf &= ~MAC_CR_GMII_EN_;
-   buf |= MAC_CR_AUTO_DUPLEX_ | MAC_CR_AUTO_SPEED_;
ret = lan78xx_write_reg(dev, MAC_CR, buf);
 
ret = lan78xx_read_reg(dev, MAC_TX, &buf);
-- 
1.9.1

[PATCH net 0/3] lan78xx: Fixes to lan78xx driver

[Nisar.Sayed]

From: Nisar Sayed 

This series of patches are for lan78xx driver.

These patches fixes potential issues associated with lan78xx driver

Nisar Sayed (3):
  Fix for eeprom read/write when device autosuspend
  Allow EEPROM write for less than MAX_EEPROM_SIZE
  Use default value loaded from EEPROM/OTP when resetting

 drivers/net/usb/lan78xx.c | 27 ++-
 1 file changed, 22 insertions(+), 5 deletions(-)

-- 
1.9.1

[PATCH net 2/3] lan78xx: Allow EEPROM write for less than MAX_EEPROM_SIZE

[Nisar.Sayed]

From: Nisar Sayed 

Allow EEPROM write for less than MAX_EEPROM_SIZE

Signed-off-by: Nisar Sayed 
---
 drivers/net/usb/lan78xx.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index baf91c7..94ef943 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -1299,6 +1299,10 @@ static int lan78xx_ethtool_set_eeprom(struct net_device 
*netdev,
 (ee->len == 512) &&
 (data[0] == OTP_INDICATOR_1))
ret = lan78xx_write_raw_otp(dev, ee->offset, ee->len, data);
+   else if ((ee->magic == LAN78XX_EEPROM_MAGIC) &&
+(ee->offset >= 0 && ee->offset < MAX_EEPROM_SIZE) &&
+(ee->len > 0 && (ee->offset + ee->len) <= MAX_EEPROM_SIZE))
+   ret = lan78xx_write_raw_eeprom(dev, ee->offset, ee->len, data);
 
usb_autopm_put_interface(dev->intf);
 
-- 
1.9.1

[PATCH net 1/3] lan78xx: Fix for eeprom read/write when device autosuspend

[Nisar.Sayed]

From: Nisar Sayed 

Fix for eeprom read/write when device autosuspend

Signed-off-by: Nisar Sayed 
---
 drivers/net/usb/lan78xx.c | 22 ++
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index b99a7fb..baf91c7 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -1265,30 +1265,44 @@ static int lan78xx_ethtool_get_eeprom(struct net_device 
*netdev,
  struct ethtool_eeprom *ee, u8 *data)
 {
struct lan78xx_net *dev = netdev_priv(netdev);
+   int ret = -EINVAL;
+
+   if (usb_autopm_get_interface(dev->intf) < 0)
+   return ret;
 
ee->magic = LAN78XX_EEPROM_MAGIC;
 
-   return lan78xx_read_raw_eeprom(dev, ee->offset, ee->len, data);
+   ret = lan78xx_read_raw_eeprom(dev, ee->offset, ee->len, data);
+
+   usb_autopm_put_interface(dev->intf);
+
+   return ret;
 }
 
 static int lan78xx_ethtool_set_eeprom(struct net_device *netdev,
  struct ethtool_eeprom *ee, u8 *data)
 {
struct lan78xx_net *dev = netdev_priv(netdev);
+   int ret = -EINVAL;
+
+   if (usb_autopm_get_interface(dev->intf) < 0)
+   return ret;
 
/* Allow entire eeprom update only */
if ((ee->magic == LAN78XX_EEPROM_MAGIC) &&
(ee->offset == 0) &&
(ee->len == 512) &&
(data[0] == EEPROM_INDICATOR))
-   return lan78xx_write_raw_eeprom(dev, ee->offset, ee->len, data);
+   ret = lan78xx_write_raw_eeprom(dev, ee->offset, ee->len, data);
else if ((ee->magic == LAN78XX_OTP_MAGIC) &&
 (ee->offset == 0) &&
 (ee->len == 512) &&
 (data[0] == OTP_INDICATOR_1))
-   return lan78xx_write_raw_otp(dev, ee->offset, ee->len, data);
+   ret = lan78xx_write_raw_otp(dev, ee->offset, ee->len, data);
 
-   return -EINVAL;
+   usb_autopm_put_interface(dev->intf);
+
+   return ret;
 }
 
 static void lan78xx_get_strings(struct net_device *netdev, u32 stringset,
-- 
1.9.1

[PATCH net] smsc95xx: Configure pause time to 0xffff when tx flow control enabled

[Nisar.Sayed]

From: Nisar Sayed 

Configure pause time to 0x when tx flow control enabled

Signed-off-by: Nisar Sayed 
---
 drivers/net/usb/smsc95xx.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 340c134..309b88a 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -526,7 +526,7 @@ static void smsc95xx_set_multicast(struct net_device 
*netdev)
 static int smsc95xx_phy_update_flowcontrol(struct usbnet *dev, u8 duplex,
   u16 lcladv, u16 rmtadv)
 {
-   u32 flow, afc_cfg = 0;
+   u32 flow = 0, afc_cfg;
 
int ret = smsc95xx_read_reg(dev, AFC_CFG, &afc_cfg);
if (ret < 0)
@@ -537,20 +537,19 @@ static int smsc95xx_phy_update_flowcontrol(struct usbnet 
*dev, u8 duplex,
 
if (cap & FLOW_CTRL_RX)
flow = 0x0002;
-   else
-   flow = 0;
 
-   if (cap & FLOW_CTRL_TX)
+   if (cap & FLOW_CTRL_TX) {
afc_cfg |= 0xF;
-   else
+   flow |= 0x;
+   } else {
afc_cfg &= ~0xF;
+   }
 
netif_dbg(dev, link, dev->net, "rx pause %s, tx pause %s\n",
   cap & FLOW_CTRL_RX ? "enabled" : "disabled",
   cap & FLOW_CTRL_TX ? "enabled" : "disabled");
} else {
netif_dbg(dev, link, dev->net, "half duplex\n");
-   flow = 0;
afc_cfg |= 0xF;
}
 
-- 
1.9.1

RE: [PATCH net 3/4] lan78xx: Fix for eeprom read/write when device autosuspend

[Nisar.Sayed]

Thanks, will make separate patch.

> Hi Nisar
> 
> > +   else if ((ee->magic == LAN78XX_EEPROM_MAGIC) &&
> > +(ee->offset >= 0 && ee->offset < MAX_EEPROM_SIZE) &&
> > +(ee->len > 0 && (ee->offset + ee->len) <=
> MAX_EEPROM_SIZE))
> > +   ret = lan78xx_write_raw_eeprom(dev, ee->offset, ee->len,
> data);
> 
> This change does not appear to have anything to do with auto suspend.
> Please make it a separate patch.
> 
>Andrew

RE: [PATCH net 2/4] lan78xx: Add fixed_phy device support for LAN7801 device

[Nisar.Sayed]

Thanks Andrew, will try to change as suggested.

> On Wed, Sep 06, 2017 at 10:51:44AM +, nisar.sa...@microchip.com
> wrote:
> > From: Nisar Sayed 
> >
> > Add fixed_phy device support for LAN7801 device
> >
> > When LAN7801 device connected to PHY Device which does not have
> > MDIO/MDC access, fixex_phy device will be added.
> 
> Please try to find a way to do this without all the #ifdefs. They can be
> acceptable in header files, but should be avoided in .c files.
> 
>Andrew

RE: [PATCH net 1/4] lan78xx: Fix for crash associated with System suspend

[Nisar.Sayed]

Thanks Andrew inputs.
 
> On Wed, Sep 06, 2017 at 10:51:31AM +, nisar.sa...@microchip.com
> wrote:
> > From: Nisar Sayed 
> >
> > Fix for crash associated with System suspend
> >
> > Since ndo_stop removes phydev which makes phydev NULL.
> > Whenever system suspend is initiated or after "ifconfig 
> > down", if set_wol or get_wol is triggered phydev is NULL leads system
> crash.
> > Hence phy_start/phy_stop for ndo_start/ndo_stop fixes the issues
> > instead of adding/removing phydevice
> 
> Looking at this patch, there apears to be lots of different things going on.
> Please can you split it up into multiple patches.

Sure will split it up.

> 
> > Signed-off-by: Nisar Sayed 
> > ---
> >  drivers/net/usb/lan78xx.c | 44
> > 
> >  1 file changed, 28 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> > index b99a7fb..955ab3b 100644
> > --- a/drivers/net/usb/lan78xx.c
> > +++ b/drivers/net/usb/lan78xx.c
> > @@ -2024,6 +2024,8 @@ static int lan78xx_phy_init(struct lan78xx_net
> *dev)
> >  lan8835_fixup);
> > if (ret < 0) {
> > netdev_err(dev->net, "fail to register fixup\n");
> > +   phy_unregister_fixup_for_uid(PHY_KSZ9031RNX,
> > +0xfff0);
> 
> goto error; would be better. phy_unregister_fixup_for_uid() does not care if
> you try to unregister something which has not been registered.
> 
> Also, this should be a separate patch.

Ok, will make it as separate patch

> 
> > return ret;
> > }
> > /* add more external PHY fixup here if needed */ @@ -
> 2031,8 +2033,7
> > @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
> > phydev->is_internal = false;
> > } else {
> > netdev_err(dev->net, "unknown ID found\n");
> > -   ret = -EIO;
> > -   goto error;
> > +   return -EIO;
> > }
> >
> > /* if phyirq is not set, use polling mode in phylib */ @@ -2051,7
> > +2052,10 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
> > if (ret) {
> > netdev_err(dev->net, "can't attach PHY to %s\n",
> >dev->mdiobus->id);
> > -   return -EIO;
> > +   ret = -EIO;
> > +   if (dev->chipid == ID_REV_CHIP_ID_7801_)
> > +   goto error;
> > +   return ret;
> 
> Why not add the if (dev->chipid == ID_REV_CHIP_ID_7801_) after the
> error: label?
> 
> 


Yes, will correct it

> 
> > }
> >
> > /* MAC doesn't support 1000T Half */ @@ -2067,8 +2071,6 @@ static
> > int lan78xx_phy_init(struct lan78xx_net *dev)
> >
> > dev->fc_autoneg = phydev->autoneg;
> >
> > -   phy_start(phydev);
> > -
> > netif_dbg(dev, ifup, dev->net, "phy initialised successfully");
> >
> > return 0;
> > @@ -2497,9 +2499,9 @@ static int lan78xx_open(struct net_device *net)
> > if (ret < 0)
> > goto done;
> >
> > -   ret = lan78xx_phy_init(dev);
> > -   if (ret < 0)
> > -   goto done;
> > +   if (dev->domain_data.phyirq > 0)
> > +   phy_start_interrupts(dev->net->phydev);
> 
> This is unusual. I don't see any other MAC driver starting interrupts.
> This needs explaining.
> 
>  Andrew

Since "lan78xx_open" calls  "lan78xx_reset" (Device reset) it is required to 
start/enable interrupt back
Initially when "phydev->state = PHY_READY" state  "phy_start" will not enable 
interrupts,
However after "lan78xx_stop" when "phy_stop" makes "phydev->state = PHY_HALTED"
Subsequent call to "phy_start" will enable interrupt.

Hence "phy_start_interrupts" used after "lan78xx_reset"

- Nisar

[PATCH net 3/4] lan78xx: Fix for eeprom read/write when device autosuspend

[Nisar.Sayed]

From: Nisar Sayed 

Fix for eeprom read/write when device autosuspend

Signed-off-by: Nisar Sayed 
---
 drivers/net/usb/lan78xx.c | 26 ++
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 6242cb7..e04ec23 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -1278,30 +1278,48 @@ static int lan78xx_ethtool_get_eeprom(struct net_device 
*netdev,
  struct ethtool_eeprom *ee, u8 *data)
 {
struct lan78xx_net *dev = netdev_priv(netdev);
+   int ret = -EINVAL;
+
+   if (usb_autopm_get_interface(dev->intf) < 0)
+   return ret;
 
ee->magic = LAN78XX_EEPROM_MAGIC;
 
-   return lan78xx_read_raw_eeprom(dev, ee->offset, ee->len, data);
+   ret = lan78xx_read_raw_eeprom(dev, ee->offset, ee->len, data);
+
+   usb_autopm_put_interface(dev->intf);
+
+   return ret;
 }
 
 static int lan78xx_ethtool_set_eeprom(struct net_device *netdev,
  struct ethtool_eeprom *ee, u8 *data)
 {
struct lan78xx_net *dev = netdev_priv(netdev);
+   int ret = -EINVAL;
+
+   if (usb_autopm_get_interface(dev->intf) < 0)
+   return ret;
 
/* Allow entire eeprom update only */
if ((ee->magic == LAN78XX_EEPROM_MAGIC) &&
(ee->offset == 0) &&
(ee->len == 512) &&
(data[0] == EEPROM_INDICATOR))
-   return lan78xx_write_raw_eeprom(dev, ee->offset, ee->len, data);
+   ret = lan78xx_write_raw_eeprom(dev, ee->offset, ee->len, data);
else if ((ee->magic == LAN78XX_OTP_MAGIC) &&
 (ee->offset == 0) &&
 (ee->len == 512) &&
 (data[0] == OTP_INDICATOR_1))
-   return lan78xx_write_raw_otp(dev, ee->offset, ee->len, data);
+   ret = lan78xx_write_raw_otp(dev, ee->offset, ee->len, data);
+   else if ((ee->magic == LAN78XX_EEPROM_MAGIC) &&
+(ee->offset >= 0 && ee->offset < MAX_EEPROM_SIZE) &&
+(ee->len > 0 && (ee->offset + ee->len) <= MAX_EEPROM_SIZE))
+   ret = lan78xx_write_raw_eeprom(dev, ee->offset, ee->len, data);
 
-   return -EINVAL;
+   usb_autopm_put_interface(dev->intf);
+
+   return ret;
 }
 
 static void lan78xx_get_strings(struct net_device *netdev, u32 stringset,
-- 
1.9.1

[PATCH net 1/4] lan78xx: Fix for crash associated with System suspend

[Nisar.Sayed]

From: Nisar Sayed 

Fix for crash associated with System suspend

Since ndo_stop removes phydev which makes phydev NULL.
Whenever system suspend is initiated or after "ifconfig  down",
if set_wol or get_wol is triggered phydev is NULL leads system crash.
Hence phy_start/phy_stop for ndo_start/ndo_stop fixes the issues
instead of adding/removing phydevice

Signed-off-by: Nisar Sayed 
---
 drivers/net/usb/lan78xx.c | 44 
 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index b99a7fb..955ab3b 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -2024,6 +2024,8 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
 lan8835_fixup);
if (ret < 0) {
netdev_err(dev->net, "fail to register fixup\n");
+   phy_unregister_fixup_for_uid(PHY_KSZ9031RNX,
+0xfff0);
return ret;
}
/* add more external PHY fixup here if needed */
@@ -2031,8 +2033,7 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
phydev->is_internal = false;
} else {
netdev_err(dev->net, "unknown ID found\n");
-   ret = -EIO;
-   goto error;
+   return -EIO;
}
 
/* if phyirq is not set, use polling mode in phylib */
@@ -2051,7 +2052,10 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
if (ret) {
netdev_err(dev->net, "can't attach PHY to %s\n",
   dev->mdiobus->id);
-   return -EIO;
+   ret = -EIO;
+   if (dev->chipid == ID_REV_CHIP_ID_7801_)
+   goto error;
+   return ret;
}
 
/* MAC doesn't support 1000T Half */
@@ -2067,8 +2071,6 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
 
dev->fc_autoneg = phydev->autoneg;
 
-   phy_start(phydev);
-
netif_dbg(dev, ifup, dev->net, "phy initialised successfully");
 
return 0;
@@ -2497,9 +2499,9 @@ static int lan78xx_open(struct net_device *net)
if (ret < 0)
goto done;
 
-   ret = lan78xx_phy_init(dev);
-   if (ret < 0)
-   goto done;
+   if (dev->domain_data.phyirq > 0)
+   phy_start_interrupts(dev->net->phydev);
+   phy_start(dev->net->phydev);
 
/* for Link Check */
if (dev->urb_intr) {
@@ -2560,13 +2562,11 @@ static int lan78xx_stop(struct net_device *net)
if (timer_pending(&dev->stat_monitor))
del_timer_sync(&dev->stat_monitor);
 
-   phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, 0xfff0);
-   phy_unregister_fixup_for_uid(PHY_LAN8835, 0xfff0);
-
-   phy_stop(net->phydev);
-   phy_disconnect(net->phydev);
-
-   net->phydev = NULL;
+   if (net->phydev) {
+   if (dev->domain_data.phyirq > 0)
+   phy_stop_interrupts(net->phydev);
+   phy_stop(net->phydev);
+   }
 
clear_bit(EVENT_DEV_OPEN, &dev->flags);
netif_stop_queue(net);
@@ -3464,6 +3464,12 @@ static void lan78xx_disconnect(struct usb_interface 
*intf)
udev = interface_to_usbdev(intf);
 
net = dev->net;
+   if (dev->chipid == ID_REV_CHIP_ID_7801_) {
+   phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, 0xfff0);
+   phy_unregister_fixup_for_uid(PHY_LAN8835, 0xfff0);
+   }
+   phy_disconnect(net->phydev);
+   net->phydev = NULL;
unregister_netdev(net);
 
cancel_delayed_work_sync(&dev->wq);
@@ -3613,6 +3619,10 @@ static int lan78xx_probe(struct usb_interface *intf,
goto out3;
}
 
+   ret = lan78xx_phy_init(dev);
+   if (ret < 0)
+   goto out3;
+
usb_set_intfdata(intf, dev);
 
ret = device_set_wakeup_enable(&udev->dev, true);
@@ -3972,7 +3982,9 @@ static int lan78xx_reset_resume(struct usb_interface 
*intf)
 
lan78xx_reset(dev);
 
-   lan78xx_phy_init(dev);
+   if (dev->domain_data.phyirq > 0)
+   phy_start_interrupts(dev->net->phydev);
+   phy_start(dev->net->phydev);
 
return lan78xx_resume(intf);
 }
-- 
1.9.1

[PATCH net 4/4] lan78xx: Use default value loaded from EEPROM/OTP when resetting the chip

[Nisar.Sayed]

From: Nisar Sayed 

Use default value loaded from EEPROM/OTP when resetting
 the chip

Signed-off-by: Nisar Sayed 
---
 drivers/net/usb/lan78xx.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index e04ec23..84491e7 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -2497,7 +2497,6 @@ static int lan78xx_reset(struct lan78xx_net *dev)
/* LAN7801 only has RGMII mode */
if (dev->chipid == ID_REV_CHIP_ID_7801_)
buf &= ~MAC_CR_GMII_EN_;
-   buf |= MAC_CR_AUTO_DUPLEX_ | MAC_CR_AUTO_SPEED_;
ret = lan78xx_write_reg(dev, MAC_CR, buf);
 
ret = lan78xx_read_reg(dev, MAC_TX, &buf);
-- 
1.9.1

[PATCH net 2/4] lan78xx: Add fixed_phy device support for LAN7801 device

[Nisar.Sayed]

From: Nisar Sayed 

Add fixed_phy device support for LAN7801 device

When LAN7801 device connected to PHY Device which does not have
MDIO/MDC access, fixex_phy device will be added.

Signed-off-by: Nisar Sayed 
---
 drivers/net/usb/Kconfig   | 10 
 drivers/net/usb/lan78xx.c | 59 ++-
 2 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
index f28bd74..34ef670 100644
--- a/drivers/net/usb/Kconfig
+++ b/drivers/net/usb/Kconfig
@@ -123,6 +123,16 @@ config USB_LAN78XX
  To compile this driver as a module, choose M here: the
  module will be called lan78xx.
 
+   if USB_LAN78XX
+   config LAN7801_NO_MDIO_DEVICE
+   bool "NO MDIO Device Support"
+   select FIXED_PHY
+   ---help---
+   If LAN7801 connected to a PHY device which does not
+   have MDIO/MDC access this option adds support to LAN7801
+   with fixed_phy support
+   endif
+
 config USB_USBNET
tristate "Multi-purpose USB Networking Framework"
select MII
diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 955ab3b..6242cb7 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -42,7 +43,7 @@
 #define DRIVER_AUTHOR  "WOOJUNG HUH "
 #define DRIVER_DESC"LAN78XX USB 3.0 Gigabit Ethernet Devices"
 #define DRIVER_NAME"lan78xx"
-#define DRIVER_VERSION "1.0.6"
+#define DRIVER_VERSION "1.0.7"
 
 #define TX_TIMEOUT_JIFFIES (5 * HZ)
 #define THROTTLE_JIFFIES   (HZ / 8)
@@ -335,6 +336,7 @@ struct statstage {
struct lan78xx_statstage64  curr_stat;
 };
 
+#ifndef CONFIG_LAN7801_NO_MDIO_DEVICE
 struct irq_domain_data {
struct irq_domain   *irqdomain;
unsigned intphyirq;
@@ -343,6 +345,7 @@ struct irq_domain_data {
u32 irqenable;
struct mutexirq_lock;   /* for irq bus access */
 };
+#endif
 
 struct lan78xx_net {
struct net_device   *net;
@@ -401,7 +404,9 @@ struct lan78xx_net {
int delta;
struct statstagestats;
 
+#ifndef CONFIG_LAN7801_NO_MDIO_DEVICE
struct irq_domain_data  domain_data;
+#endif
 };
 
 /* define external phy id */
@@ -1169,6 +1174,9 @@ static int lan78xx_link_reset(struct lan78xx_net *dev)
ret = lan78xx_write_reg(dev, MAC_CR, buf);
if (unlikely(ret < 0))
return -EIO;
+#ifdef CONFIG_LAN7801_NO_MDIO_DEVICE
+   phy_mac_interrupt(phydev, 0);
+#endif
 
del_timer(&dev->stat_monitor);
} else if (phydev->link && !dev->link_on) {
@@ -1209,6 +1217,9 @@ static int lan78xx_link_reset(struct lan78xx_net *dev)
 
ret = lan78xx_update_flowcontrol(dev, ecmd.base.duplex, ladv,
 radv);
+#ifdef CONFIG_LAN7801_NO_MDIO_DEVICE
+   phy_mac_interrupt(phydev, 1);
+#endif
 
if (!timer_pending(&dev->stat_monitor)) {
dev->delta = 1;
@@ -1249,8 +1260,10 @@ static void lan78xx_status(struct lan78xx_net *dev, 
struct urb *urb)
netif_dbg(dev, link, dev->net, "PHY INTR: 0x%08x\n", intdata);
lan78xx_defer_kevent(dev, EVENT_LINK_RESET);
 
+#ifndef CONFIG_LAN7801_NO_MDIO_DEVICE
if (dev->domain_data.phyirq > 0)
generic_handle_irq(dev->domain_data.phyirq);
+#endif
} else
netdev_warn(dev->net,
"unexpected interrupt: 0x%08x\n", intdata);
@@ -1825,6 +1838,7 @@ static void lan78xx_link_status_change(struct net_device 
*net)
}
 }
 
+#ifndef CONFIG_LAN7801_NO_MDIO_DEVICE
 static int irq_map(struct irq_domain *d, unsigned int irq,
   irq_hw_number_t hwirq)
 {
@@ -1945,6 +1959,7 @@ static void lan78xx_remove_irq_domain(struct lan78xx_net 
*dev)
dev->domain_data.phyirq = 0;
dev->domain_data.irqdomain = NULL;
 }
+#endif
 
 static int lan8835_fixup(struct phy_device *phydev)
 {
@@ -1987,12 +2002,37 @@ static int ksz9031rnx_fixup(struct phy_device *phydev)
return 1;
 }
 
+#ifdef CONFIG_LAN7801_NO_MDIO_DEVICE
+static struct fixed_phy_status fphy_status = {
+   .link = 1,
+   .speed = SPEED_1000,
+   .duplex = DUPLEX_FULL,
+};
+#endif
+
 static int lan78xx_phy_init(struct lan78xx_net *dev)
 {
int ret;
u32 mii_adv;
struct phy_device *phydev = dev->net->phydev;
 
+#ifdef CONFIG_LAN7801_NO_MDIO_DEVICE
+   if (dev->chipid != ID_REV_CHIP_ID_7801_) {
+   netdev_err(dev->net, "Invalid chip id : %x\n", dev->chipid);
+   return -EIO;
+   }
+   phydev = fixed_phy_register(PHY_POLL, &fphy_status,
+

[PATCH net 0/4] lan78xx: Fixes and Enhancements to lan78xx driver

[Nisar.Sayed]

From: Nisar Sayed 

This series of patches are for lan78xx driver.

These patches supports fixes and enhancements to lan78xx driver

Nisar Sayed (4):
  Fix for crash associated with System suspend
  Add fixed_phy device support for LAN7801 device
  Fix for eeprom read/write when device autosuspend
  Use default value loaded from EEPROM/OTP when resetting the chip

 drivers/net/usb/Kconfig   |  10 
 drivers/net/usb/lan78xx.c | 130 ++
 2 files changed, 118 insertions(+), 22 deletions(-)

-- 
1.9.1

[PATCH net 2/2] lan78xx: Fix to handle hard_header_len update

[Nisar.Sayed]

From: Nisar Sayed 

Fix to handle hard_header_len update

When ifconfig up/down sequence is initiated hard_header_len
get updated incrementally for each ifconfig up /down sequence,
this leads invalid hard_header_len, moving to lan78xx_bind
to have one time update of hard_header_len addresses the issue.

Signed-off-by: Nisar Sayed 
---
 drivers/net/usb/lan78xx.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 8ef3639..b99a7fb 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -2367,9 +2367,6 @@ static int lan78xx_reset(struct lan78xx_net *dev)
/* Init LTM */
lan78xx_init_ltm(dev);
 
-   dev->net->hard_header_len += TX_OVERHEAD;
-   dev->hard_mtu = dev->net->mtu + dev->net->hard_header_len;
-
if (dev->udev->speed == USB_SPEED_SUPER) {
buf = DEFAULT_BURST_CAP_SIZE / SS_USB_PKT_SIZE;
dev->rx_urb_size = DEFAULT_BURST_CAP_SIZE;
@@ -2855,6 +2852,9 @@ static int lan78xx_bind(struct lan78xx_net *dev, struct 
usb_interface *intf)
return ret;
}
 
+   dev->net->hard_header_len += TX_OVERHEAD;
+   dev->hard_mtu = dev->net->mtu + dev->net->hard_header_len;
+
/* Init all registers */
ret = lan78xx_reset(dev);
 
-- 
1.9.1

[PATCH net 1/2] lan78xx: USB fast connect/disconnect crash fix

[Nisar.Sayed]

From: Nisar Sayed 

USB fast connect/disconnect crash fix

When USB plugged/unplugged at fast rate,
lan78xx_mdio_init() in lan78xx_bind() failing case is not handled.
Whenever  lan78xx_mdio_init() failed, dev->mdiobus will be freed, however
since lan78xx_bind() not consider as error and try to proceed for
further initialization in lan78xx_probe() which leads system hung/crash.
Also when register_netdev() failed, netdev is freed without calling 
lan78xx_unbind().
Hence halting the failed cases right manner fixes the system crash/hung issue.

Signed-off-by: Nisar Sayed 
---
 drivers/net/usb/lan78xx.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 5833f7e..8ef3639 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -2858,13 +2858,13 @@ static int lan78xx_bind(struct lan78xx_net *dev, struct 
usb_interface *intf)
/* Init all registers */
ret = lan78xx_reset(dev);
 
-   lan78xx_mdio_init(dev);
+   ret = lan78xx_mdio_init(dev);
 
dev->net->flags |= IFF_MULTICAST;
 
pdata->wol = WAKE_MAGIC;
 
-   return 0;
+   return ret;
 }
 
 static void lan78xx_unbind(struct lan78xx_net *dev, struct usb_interface *intf)
@@ -3525,11 +3525,11 @@ static int lan78xx_probe(struct usb_interface *intf,
udev = interface_to_usbdev(intf);
udev = usb_get_dev(udev);
 
-   ret = -ENOMEM;
netdev = alloc_etherdev(sizeof(struct lan78xx_net));
if (!netdev) {
-   dev_err(&intf->dev, "Error: OOM\n");
-   goto out1;
+   dev_err(&intf->dev, "Error: OOM\n");
+   ret = -ENOMEM;
+   goto out1;
}
 
/* netdev_printk() needs this */
@@ -3610,7 +3610,7 @@ static int lan78xx_probe(struct usb_interface *intf,
ret = register_netdev(netdev);
if (ret != 0) {
netif_err(dev, probe, netdev, "couldn't register the device\n");
-   goto out2;
+   goto out3;
}
 
usb_set_intfdata(intf, dev);
-- 
1.9.1

[PATCH net 0/2] lan78xx: Fixes to lan78xx driver

[Nisar.Sayed]

From: Nisar Sayed 

This series of patches are for lan78xx driver.

These patches fixes potential issues associated with lan78xx driver

Nisar Sayed (2):
  USB fast connect/disconnect crash fix
  Fix to handle hard_header_len update

 drivers/net/usb/lan78xx.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

-- 
1.9.1

RE: [PATCH v2 net] smsc95xx: Support only IPv4 TCP/UDP csum offload

[Nisar.Sayed]

> > When TX checksum offload is used, if the computed checksum is 0 the
> > LAN95xx device do not alter the checksum to 0x.  In the case of ipv4
> > UDP checksum, it indicates to receiver that no checksum is calculated.
> > Under ipv6, UDP checksum yields a result of zero must be changed to
> > 0x. Hence disabling checksum offload for ipv6 packets.
> >
> > Signed-off-by: Nisar Sayed 
> >
> > Reported-by: popcorn mix 
> 
> Appied, thanks.

Thanks.

[PATCH v2 net] smsc95xx: Support only IPv4 TCP/UDP csum offload

[Nisar.Sayed]

From: Nisar Sayed 

When TX checksum offload is used, if the computed checksum is 0 the
LAN95xx device do not alter the checksum to 0x.  In the case of ipv4
UDP checksum, it indicates to receiver that no checksum is calculated.
Under ipv6, UDP checksum yields a result of zero must be changed to
0x. Hence disabling checksum offload for ipv6 packets.

Signed-off-by: Nisar Sayed 

Reported-by: popcorn mix 
---
Changes
V2
- Updated e-mail alias
- Updated commit message and comments

 drivers/net/usb/smsc95xx.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 765400b..2dfca96 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -681,7 +681,7 @@ static int smsc95xx_set_features(struct net_device *netdev,
if (ret < 0)
return ret;
 
-   if (features & NETIF_F_HW_CSUM)
+   if (features & NETIF_F_IP_CSUM)
read_buf |= Tx_COE_EN_;
else
read_buf &= ~Tx_COE_EN_;
@@ -1279,12 +1279,19 @@ static int smsc95xx_bind(struct usbnet *dev, struct 
usb_interface *intf)
 
spin_lock_init(&pdata->mac_cr_lock);
 
+   /* LAN95xx devices do not alter the computed checksum of 0 to 0x.
+* RFC 2460, ipv6 UDP calculated checksum yields a result of zero must
+* be changed to 0x. RFC 768, ipv4 UDP computed checksum is zero,
+* it is transmitted as all ones. The zero transmitted checksum means
+* transmitter generated no checksum. Hence, enable csum offload only
+* for ipv4 packets.
+*/
if (DEFAULT_TX_CSUM_ENABLE)
-   dev->net->features |= NETIF_F_HW_CSUM;
+   dev->net->features |= NETIF_F_IP_CSUM;
if (DEFAULT_RX_CSUM_ENABLE)
dev->net->features |= NETIF_F_RXCSUM;
 
-   dev->net->hw_features = NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
+   dev->net->hw_features = NETIF_F_IP_CSUM | NETIF_F_RXCSUM;
 
smsc95xx_init_mac_address(dev);
 
-- 
1.9.1