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

Oleksij Rempel
2025-01-22T08:16:42Z

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 |