On Fri, 2020-07-24 at 00:39 +0200, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> On Thu, Jul 23, 2020 at 01:55:04PM +0200, Andre Edich wrote:
> > Generally, each PHY has their own configuration and it can be done
> > through an external PHY driver. The smsc95xx driver uses only the
> > hard-coded internal PHY configuration.
> >
> > This patch adds PAL (PHY Abstraction Layer) support to probe
> > external
> > PHY drivers for configuring external PHYs.
>
> Hi Andre
>
> We call it phylib, not PAL.
Hi Andrew,
thank you for the feedback. In the next version, I will correct these
wordings as well.
>
> > static int __must_check smsc95xx_wait_eeprom(struct usbnet *dev)
> > {
> > unsigned long start_time = jiffies;
> > @@ -559,15 +580,20 @@ static int smsc95xx_link_reset(struct usbnet
> > *dev)
> > u16 lcladv, rmtadv;
> > int ret;
> >
> > - /* clear interrupt status */
> > - ret = smsc95xx_mdio_read(dev->net, mii->phy_id, PHY_INT_SRC);
> > - if (ret < 0)
> > - return ret;
> > -
> > ret = smsc95xx_write_reg(dev, INT_STS, INT_STS_CLEAR_ALL_);
> > if (ret < 0)
> > return ret;
> >
> > + if (pdata->internal_phy) {
> > + /* clear interrupt status */
> > + ret = smsc95xx_mdio_read(dev->net, mii->phy_id,
> > PHY_INT_SRC);
> > + if (ret < 0)
> > + return ret;
> > +
> > + smsc95xx_mdio_write(dev->net, mii->phy_id,
> > PHY_INT_MASK,
> > + PHY_INT_MASK_DEFAULT_);
> > + }
>
> The PHY driver should do this, not the MAC driver.
>
> Which PHY driver is used for the internal PHY? In theory, you should
> not need to know if it is internal or external, it is just a PHY.
Yes sure, you are right. I see the drivers/net/phy/smsc.c that is
probed for the internal PHY of the DUT's Ethernet controller.
>
> That
> might mean you need to move some code from this driver into the PHY
> driver, if it is currently missing in the PHY driver.
Correct, the PHY driver does interrupt setup activities, so that they
can be removed from the smsc95xx.
>
> > +
> > mii_check_media(mii, 1, 1);
> > mii_ethtool_gset(&dev->mii, &ecmd);
> > lcladv = smsc95xx_mdio_read(dev->net, mii->phy_id,
> > MII_ADVERTISE);
> > @@ -851,10 +877,10 @@ static int smsc95xx_get_link_ksettings(struct
> > net_device *net,
> > int retval;
> >
> > retval = usbnet_get_link_ksettings(net, cmd);
> > -
> > - cmd->base.eth_tp_mdix = pdata->mdix_ctrl;
> > - cmd->base.eth_tp_mdix_ctrl = pdata->mdix_ctrl;
> > -
> > + if (pdata->internal_phy) {
> > + cmd->base.eth_tp_mdix = pdata->mdix_ctrl;
> > + cmd->base.eth_tp_mdix_ctrl = pdata->mdix_ctrl;
> > + }
>
> Again, they PHY driver should take care of this. You need to set
> phydev->mdix_ctrl before starting the PHY. The PHY driver should set
> phdev->mdix to the current status.
The SMSC Phy driver does not have any MDIX setup code, but I think I've
got the idea now.
>
> > +static void smsc95xx_handle_link_change(struct net_device *net)
> > +{
> > + phy_print_status(net->phydev);
>
> So the MAC does not care about the speed? The pause configuration?
> Duplex?
Now, I'm wondering how those "care about speed", "pause", and "duplex"
work in the current smsc95xx. I guess, we did not touch any of those
activities with our patches.
Thanks a lot.
Andre
>
> Andrew
