Re: [PATCH net-next v2 3/6] smsc95xx: add PAL support to use external PHY drivers

[Andrew Lunn <andrew@lunn.ch>]

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.

>  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. 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.

> +
>  	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. 

> +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?

	Andrew