Re: [PATCH net-next v1 1/7] net: usb: lan78xx: Convert to PHYlink for improved PHY and MAC management

[Oleksij Rempel <o.rempel@pengutronix.de>]

Hi Rengarajan,

On Fri, Jan 17, 2025 at 10:50:48AM +0000, Rengarajan.S@microchip.com wrote:
> Hi Oleksji,
> 
> On Wed, 2025-01-08 at 13:13 +0100, Oleksij Rempel wrote:
> > +static int lan78xx_configure_usb(struct lan78xx_net *dev, int speed)
> > +{
> > +       int ret;
> > +
> > +       if (dev->udev->speed != USB_SPEED_SUPER)
> > +               return 0;
> > +
> > +       if (speed != SPEED_1000)
> > +               return lan78xx_update_reg(dev, USB_CFG1,
> > +                                         USB_CFG1_DEV_U2_INIT_EN_ |
> > +                                         USB_CFG1_DEV_U1_INIT_EN_,
> > +                                         USB_CFG1_DEV_U2_INIT_EN_ |
> > +                                         USB_CFG1_DEV_U1_INIT_EN_);
> > +
> > +       /* disable U2 */
> > +       ret = lan78xx_update_reg(dev, USB_CFG1,
> > +                                USB_CFG1_DEV_U2_INIT_EN_, 0);
> > +       if (ret < 0)
> > +               return ret;
> > +       /* enable U1 */
> > +       return lan78xx_update_reg(dev, USB_CFG1,
> > +                                 USB_CFG1_DEV_U1_INIT_EN_,
> > +                                 USB_CFG1_DEV_U1_INIT_EN_);
> 
> Since, in the all the above cases we update the USB_CFG1 based on the
> mask and value depending on various speeds, have a variable for mask
> and value and use them to represent enabled flags instead of passing
> the flags directly for better readability.
> 
> For Eg: have mask = USB_CFG1_DEV_U2_INIT_EN_ and val = 0 and pass them
> as arguments.

ok.

> +
> > +       if (duplex == DUPLEX_FULL)
> > +               mac_cr |= MAC_CR_FULL_DUPLEX_;
> > +
> > +       /* make sure TXEN and RXEN are disabled before reconfiguring
> > MAC */
> > +       ret = lan78xx_update_reg(dev, MAC_CR, MAC_CR_SPEED_MASK_ |
> > +                                MAC_CR_FULL_DUPLEX_ |
> > MAC_CR_EEE_EN_, mac_cr);
> 
> The auto-speed and duplex are disabled in lan78xx_mac_config is it
> necessary to disable this again here. You can disable it once to 
> avoid redundancy.

ok.

> > +static const struct phylink_mac_ops lan78xx_phylink_mac_ops = {
> > +       .mac_config = lan78xx_mac_config,
> > +       .mac_link_down = lan78xx_mac_link_down,
> > +       .mac_link_up = lan78xx_mac_link_up,
> > +};
> 
> If possible, have MAC and phy related APIs in seperate patch.

I'll think about it, but I ques it will be not so good.

> > @@ -2609,18 +2701,7 @@ static int lan78xx_phy_init(struct lan78xx_net
> > *dev)
> >                 return -EIO;
> >         }
> > 
> > -       /* MAC doesn't support 1000T Half */
> > -       phy_remove_link_mode(phydev,
> > ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
> 
> Since MAC doesn't support 1000Base-T half-duplex does phylink handle
> its removal from the advertised link modes. Previously, it was done
> using phy_remove_link_mode. If not, ensure that the phy doesn't end
> up advertising an unsupported mode.

Yes, it is done by following line:
  dev->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
    MAC_10 | MAC_100 | MAC_1000FD;

Thank you,

Best regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |