Re: [PATCH 1/3] net: ethernet: add support for PCS and 2.5G speed

[Florian Fainelli <f.fainelli@gmail.com>]

Le 2/22/19 à 12:12 PM, Parshuram Thombare a écrit :
> This patch add support for PCS (for SGMII interface) and 2.5Gbps MAC
> in Cadence ethernet controller driver.

At a high level you don't seem to be making use of PHYLINK so which
2.5Gbps interfaces do you actually support?

> 
> Signed-off-by: Parshuram Thombare <pthombar@cadence.com>
> ---

[snip]

> @@ -361,26 +361,50 @@ static int macb_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
>   * macb_set_tx_clk() - Set a clock to a new frequency
>   * @clk		Pointer to the clock to change
>   * @rate	New frequency in Hz
> + * @interafce	Phy interface

Typo: @interface and this is an unrelated change.

>   * @dev		Pointer to the struct net_device
>   */
> -static void macb_set_tx_clk(struct clk *clk, int speed, struct net_device *dev)
> +static void macb_set_tx_clk(struct clk *clk, int speed,
> +			    phy_interface_t interface, struct net_device *dev)
>  {
>  	long ferr, rate, rate_rounded;
>  
>  	if (!clk)
>  		return;
>  
> -	switch (speed) {
> -	case SPEED_10:
> +	if (interface == PHY_INTERFACE_MODE_GMII ||
> +	    interface == PHY_INTERFACE_MODE_MII) {
> +		switch (speed) {
> +		case SPEED_10:>  		rate = 2500000;

You need to add one tab to align rate and break.

>  		break;
> -	case SPEED_100:
> +		case SPEED_100:
>  		rate = 25000000;
>  		break;
> -	case SPEED_1000:
> +		case SPEED_1000:
>  		rate = 125000000;
>  		break;
> -	default:
> +		default:
> +		return;
> +		}
> +	} else if (interface == PHY_INTERFACE_MODE_SGMII) {
> +		switch (speed) {
> +		case SPEED_10:
> +		rate = 1250000;
> +		break;
> +		case SPEED_100:
> +		rate = 12500000;
> +		break;
> +		case SPEED_1000:
> +		rate = 125000000;
> +		break;
> +		case SPEED_2500:
> +		rate = 312500000;
> +		break;
> +		default:
> +		return;

The indentation is broken here and you can greatly simplify this with a
simple function that returns speed * 1250 and does an initial check for
unsupported speeds.

> +		}
> +	} else {
>  		return;
>  	}
>  
> @@ -410,30 +434,49 @@ static void macb_handle_link_change(struct net_device *dev)
>  
>  	spin_lock_irqsave(&bp->lock, flags);
>  
> -	if (phydev->link) {
> -		if ((bp->speed != phydev->speed) ||
> -		    (bp->duplex != phydev->duplex)) {
> -			u32 reg;
> -
> -			reg = macb_readl(bp, NCFGR);
> -			reg &= ~(MACB_BIT(SPD) | MACB_BIT(FD));
> -			if (macb_is_gem(bp))
> -				reg &= ~GEM_BIT(GBE);
> +	if (phydev->link && (bp->speed != phydev->speed ||
> +			     bp->duplex != phydev->duplex)) {
> +		u32 reg;
>  
> -			if (phydev->duplex)
> -				reg |= MACB_BIT(FD);
> +		reg = macb_readl(bp, NCFGR);
> +		reg &= ~(MACB_BIT(SPD) | MACB_BIT(FD));
> +		if (macb_is_gem(bp))
> +			reg &= ~GEM_BIT(GBE);
> +		if (phydev->duplex)
> +			reg |= MACB_BIT(FD);
> +		macb_or_gem_writel(bp, NCFGR, reg);
> +
> +		if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII &&
> +		    (phydev->speed == SPEED_1000 ||
> +		     phydev->speed == SPEED_2500)) {
> +			if (bp->caps & MACB_CAPS_TWO_PT_FIVE_GIG_SPEED) {
> +				reg = gem_readl(bp, NCR) &
> +					~GEM_BIT(TWO_PT_FIVE_GIG);
> +				gem_writel(bp, NCR, reg);
> +			}

If you are making correct use of the capabilities then there is no point
in re-checking them here. If you allowed the MAC to advertise 2.5Gbps
then it is de-facto SGMII capable.

> +			gem_writel(bp, NCFGR, GEM_BIT(GBE) |
> +					gem_readl(bp, NCFGR));
> +			if (bp->caps & MACB_CAPS_TWO_PT_FIVE_GIG_SPEED &&
> +			    phydev->speed == SPEED_2500)
> +				gem_writel(bp, NCR, gem_readl(bp, NCR) |
> +						GEM_BIT(TWO_PT_FIVE_GIG));
> +		} else if (phydev->speed == SPEED_1000) {
> +			gem_writel(bp, NCFGR, GEM_BIT(GBE) |
> +					gem_readl(bp, NCFGR));
> +		} else {
> +			if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) {
> +				reg = gem_readl(bp, NCFGR);
> +				reg &= ~(GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL));
> +				gem_writel(bp, NCFGR, reg);
> +			}
>  			if (phydev->speed == SPEED_100)
> -				reg |= MACB_BIT(SPD);
> -			if (phydev->speed == SPEED_1000 &&
> -			    bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)
> -				reg |= GEM_BIT(GBE);
> -
> -			macb_or_gem_writel(bp, NCFGR, reg);
> -
> -			bp->speed = phydev->speed;
> -			bp->duplex = phydev->duplex;
> -			status_change = 1;
> +				macb_writel(bp, NCFGR, MACB_BIT(SPD) |
> +					macb_readl(bp, NCFGR));
>  		}

There is a lot of repetition while setting the GBE bit which always set
based on speed == 1000 irrespective of the mode, so take that part out
of the if () else if () else () clauses.

> +
> +		bp->speed = phydev->speed;
> +		bp->duplex = phydev->duplex;
> +		status_change = 1;
>  	}
>  
>  	if (phydev->link != bp->link) {
> @@ -453,7 +496,8 @@ static void macb_handle_link_change(struct net_device *dev)
>  			/* Update the TX clock rate if and only if the link is
>  			 * up and there has been a link change.
>  			 */
> -			macb_set_tx_clk(bp->tx_clk, phydev->speed, dev);
> +			macb_set_tx_clk(bp->tx_clk, phydev->speed,
> +					bp->phy_interface, dev);
>  
>  			netif_carrier_on(dev);
>  			netdev_info(dev, "link up (%d/%s)\n",
> @@ -543,10 +587,16 @@ static int macb_mii_probe(struct net_device *dev)
>  	}
>  
>  	/* mask with MAC supported features */
> -	if (macb_is_gem(bp) && bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)
> -		phy_set_max_speed(phydev, SPEED_1000);
> -	else
> -		phy_set_max_speed(phydev, SPEED_100);
> +	if (macb_is_gem(bp)) {

You have changed the previous logic that also checked for
MACB_CAPS_GIGABIT_MODE_AVAILABLE, why?

> +		linkmode_copy(phydev->supported, PHY_GBIT_FEATURES);
> +		if (bp->caps & MACB_CAPS_TWO_PT_FIVE_GIG_SPEED)
> +			linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
> +					 phydev->supported);
> +	} else {
> +		linkmode_copy(phydev->supported, PHY_BASIC_FEATURES);
> +	}
> +
> +	linkmode_copy(phydev->advertising, phydev->supported);
>  
>  	if (bp->caps & MACB_CAPS_NO_GIGABIT_HALF)
>  		phy_remove_link_mode(phydev,
> @@ -2217,8 +2267,6 @@ static void macb_init_hw(struct macb *bp)
>  	macb_set_hwaddr(bp);
>  
>  	config = macb_mdc_clk_div(bp);
> -	if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII)
> -		config |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL);
>  	config |= MACB_BF(RBOF, NET_IP_ALIGN);	/* Make eth data aligned */
>  	config |= MACB_BIT(PAE);		/* PAuse Enable */
>  	config |= MACB_BIT(DRFCS);		/* Discard Rx FCS */
> @@ -3255,6 +3303,23 @@ static void macb_configure_caps(struct macb *bp,
>  		dcfg = gem_readl(bp, DCFG1);
>  		if (GEM_BFEXT(IRQCOR, dcfg) == 0)
>  			bp->caps |= MACB_CAPS_ISR_CLEAR_ON_WRITE;
> +		if (GEM_BFEXT(NO_PCS, dcfg) == 0)
> +			bp->caps |= MACB_CAPS_PCS;
> +		switch (MACB_BFEXT(IDNUM, macb_readl(bp, MID))) {
> +		case MACB_GEM7016_IDNUM:
> +		case MACB_GEM7017_IDNUM:
> +		case MACB_GEM7017A_IDNUM:
> +		case MACB_GEM7020_IDNUM:
> +		case MACB_GEM7021_IDNUM:
> +		case MACB_GEM7021A_IDNUM:
> +		case MACB_GEM7022_IDNUM:
> +		if (bp->caps & MACB_CAPS_PCS)
> +			bp->caps |= MACB_CAPS_TWO_PT_FIVE_GIG_SPEED;
> +		break;
> +
> +		default:
> +		break;
> +		}
>  		dcfg = gem_readl(bp, DCFG2);
>  		if ((dcfg & (GEM_BIT(RX_PKT_BUFF) | GEM_BIT(TX_PKT_BUFF))) == 0)
>  			bp->caps |= MACB_CAPS_FIFO_MODE;
> @@ -4110,7 +4175,28 @@ static int macb_probe(struct platform_device *pdev)
>  		else
>  			bp->phy_interface = PHY_INTERFACE_MODE_MII;
>  	} else {
> +		switch (err) {
> +		case PHY_INTERFACE_MODE_SGMII:
> +		if (bp->caps & MACB_CAPS_PCS) {
> +			bp->phy_interface = PHY_INTERFACE_MODE_SGMII;
> +			break;
> +		}

If SGMII was selected on a version of the IP that does not support it,
then falling back to GMII or MII does not sound correct, this is a hard
error that must be handled as such.
-- 
Florian