RE: smsc95xx: Invalid max MTU
> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c index > 06b4d29..420a0e4 100644 > --- a/drivers/net/usb/smsc95xx.c > +++ b/drivers/net/usb/smsc95xx.c > @@ -1318,6 +1318,7 @@ static int smsc95xx_bind(struct usbnet *dev, struct > usb_interface *intf) > dev->net->ethtool_ops = _ethtool_ops; > dev->net->flags |= IFF_MULTICAST; > dev->net->hard_header_len += SMSC95XX_TX_OVERHEAD_CSUM; > + dev->net->max_mtu = ETH_DATA_LEN; > dev->hard_mtu = dev->net->mtu + dev->net->hard_header_len; > > pdata->dev = dev; Thanks for the patch. Because device max_mtu is checked before changing the MTU value, I think your patch looks good to me. Maybe you also want to add min_mtu too? Thanks, -Raghu
RE: [PATCH v1 net-next] lan78xx: Lan7801 Support for Fixed PHY
> >* Minor cleanup > > This patch doesn't apply cleanly to net-next, please respin. Apologies. Have sent updated version v5. Thanks, -Raghu
RE: [PATCH v2 net-next] lan78xx: Lan7801 Support for Fixed PHY
Hi Florian,
> > v0->v1:
> >* Remove driver version #define
>
> This should be a separate patch of its own, more comment below.
>
OK. Patch should be for net, correct?
> > -static int lan78xx_phy_init(struct lan78xx_net *dev)
> > +static int lan7801_phy_init(struct phy_device **phydev,
> > + struct lan78xx_net *dev)
>
> Passing a **phydev looks really unnecessary here, why don't just return a
> struct phy_device * as a return argument here? If you need to communicate
> a specific return value, you can use ERR_PTR()/PTR_ERR() for that purpose.
>
Done.
> > if (ret < 0) {
> > - netdev_err(dev->net, "fail to register fixup\n");
> > + netdev_err(dev->net, "fail to register fixup
> PHY_KSZ9031RNX\n");
>
> Unrelated message change, that should be a separate commit.
OK.
> > + phydev->is_internal = true;
>
> Uggh you are not really supposed to set that, the matching PHY driver should
> have PHY_IS_INTERNAL in the .flags member to indicate that to the core PHY
> library.
>
OK. Will handle this too in separate patch/commit for net.
Thanks,
Raghu
RE: [PATCH net-next] lan78xx: Lan7801 Support for Fixed PHY
Hi Florian, > It still is completely unnecessary, you can do something like the following: > > struct phy_device *phydev = netdev->phydev; > > phy_disconnect(phydev); > if (phy_is_pseudo_fixed_link(phydev)) > fixed_phy_unregister(phydev); > > while netdev->phydev becomes NULL after phy_disconnect(), you retained > a reference to the original PHY device before disconnecting, in order to > unregister it. Can you see if that works? > -- Done. Thanks. Thanks, Raghu
RE: [PATCH net-next] lan78xx: Lan7801 Support for Fixed PHY
Hi Andrew, > > > > > dev->fixedphy stores the fixed phydev, which will be passed to the > > fixed_phy_unregister routine , so I think phy_is_pseudo_fixed_link check > is not necessary. > > I'm saying you can get rid of dev->fixedphy, and just use > netdev->phydev, and phy_is_pseudo_fixed_link(netdev->phydev) > After phy_disconnect() , the netdev->phydev becomes null, but the phydev->mdio instances are still valid. So I'm saving the phydev ptr and passing to unregister the fixed phy. If I try to unregister first and disconnect, I see panic at sysfs remove link. I believe having dev->fixedphy should not cause any problem. Thanks, Raghu
RE: [PATCH net-next] lan78xx: Lan7801 Support for Fixed PHY
Hi Andrew, > > +#define DRIVER_VERSION "1.0.7" > > Hi Raghuram > > Driver version strings a pretty pointless. You might want to remove it. > OK, will remove it. > > + netdev_info(dev->net, "Registered FIXED PHY\n"); > > There are too many detdev_info() messages here. Maybe make them both > netdev_dbg(). > OK. Will modify to netdev_dbg() > > + dev->interface = PHY_INTERFACE_MODE_RGMII; > > + dev->fixedphy = phydev; > > You can use > > if (!phy_is_pseudo_fixed_link(phydev)) > > to determine is a PHY is a fixed phy. I think you can then do without > dev->fixedphy. > dev->fixedphy stores the fixed phydev, which will be passed to the fixed_phy_unregister routine , so I think phy_is_pseudo_fixed_link check is not necessary. > > + ret = lan78xx_write_reg(dev, HW_CFG, buf); > > + goto phyinit; > > Please don't use a goto like this. Maybe turn this into a switch statement? > OK. Will modify. > > static int lan78xx_phy_init(struct lan78xx_net *dev) > > goto error; > > Please take a look at what happens at error: It does not look correct. > Probably now is a good time to refactor the whole of lan78xx_phy_init() > OK. Will take care. Thanks, -Raghu
RE: [PATCH v1 net 0/3] lan78xx: Fixes and enhancements
> > > These series of patches have fix and enhancements for lan78xx driver. > > Two problems with this series: > > 1) Only bug fixes are appropriate at this time. Features and "enhancements" >belong in net-next which is not open right now. > > 2) Patch #3 doesn't even apply cleanly. Always base your patches on the >current tree. > Sure, will send #2,#3 patches in net-next when it opens. Also will make sure they will be based on current tree. Thanks, Raghu
RE: [PATCH net 3/3] lan78xx: Lan7801 Support for Fixed PHY
> Ah, cool. I was thinking you were going to say an SFP cage. > > What switch is it? Does it have a DSA driver? > We have 3 port switch KSZ9893 yet to release which is similar to the one KSZ9477/KSZ9897 which has DSA driver. Most of the time 3 port switch being used with LAN7801 to extend the ports. Thanks, -Raghu
RE: [PATCH net 1/3] lan78xx: PHY DSP registers initialization to address EEE link drop issues with long cables
> >
> > Hi Raghuram
> >
> > You might want to look at phy_read_paged(), phy_write_paged(), etc.
> >
> > There can be race conditions with paged access.
>
> Yep, so something like:
>
> static void lan88xx_TR_reg_set(struct phy_device *phydev, u16 regaddr,
> u32 data)
> {
> int save_page, val;
> u16 buf;
>
> save_page = phy_save_page(phydev);
> phy_write_paged(phydev, LAN88XX_EXT_PAGE_ACCESS_TR,
> LAN88XX_EXT_PAGE_TR_LOW_DATA, (data &
> 0x));
> phy_write_paged(phydev, LAN88XX_EXT_PAGE_ACCESS_TR,
> LAN88XX_EXT_PAGE_TR_HIGH_DATA,
> (data & 0x00FF) >> 16);
>
> /* Config control bits [15:13] of register */
> buf = (regaddr & ~(0x3 << 13));/* Clr [14:13] to write data in reg */
> buf |= 0x8000; /* Set [15] to Packet transmit */
>
> phy_write_paged(phydev, LAN88XX_EXT_PAGE_ACCESS_TR,
> LAN88XX_EXT_PAGE_TR_CR, buf);
> usleep_range(1000, 2000);/* Wait for Data to be written */
>
> val = phy_read_paged(phydev, LAN88XX_EXT_PAGE_ACCESS_TR,
>LAN88XX_EXT_PAGE_TR_CR);
> if (!(val & 0x8000))
> pr_warn("TR Register[0x%X] configuration failed\n",
> regaddr);
>
> phy_restore_page(phydev, save_page, 0); }
>
> Since PHY accesses and thus things like phy_save_page() can fail, the return
> type of this function should be changed to 'int' and some error checking
> should be added.
Thanks David/Andrew.
Will take care of it.
Thanks,
Raghu
RE: [PATCH net 2/3] lan78xx: Add support to dump lan78xx registers
> If there is no PHY attached, you probably should not include PHY_REG_SIZE > here. > Sure, will address it. Thanks, Raghu
RE: [PATCH net 3/3] lan78xx: Lan7801 Support for Fixed PHY
> > What do you expect is connected to the MAC if there is no PHY? > Hi Andrew, We connect the Ethernet switch to this MAC. The Ethernet switch port connected to MAC do not have the phy. In this case, need to load the MAC driver and link speed/duplex set. Thanks, Raghu
RE: [PATCH net 1/3] lan78xx: Set ASD in MAC_CR when EEE is enabled.
Hi Sergei, > Hello! > > Only stylistic comments. Thanks for the comments. Will address them and submit in v1 patch. Thanks, -Raghu
