Date:   Mon, 25 Feb 2019 09:11:43 +0000
From:   Parshuram Raju Thombare <pthombar@...ence.com>
To:     Florian Fainelli <f.fainelli@...il.com>,
        "nicolas.ferre@...rochip.com" <nicolas.ferre@...rochip.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "hkallweit1@...il.com" <hkallweit1@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Rafal Ciepiela <rafalc@...ence.com>,
        Piotr Sroka <piotrs@...ence.com>, Jan Kotas <jank@...ence.com>
Subject: RE: [PATCH 1/3] net: ethernet: add support for PCS and 2.5G speed

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

New ethernet controller have MAC which support 2.5G speed. 
Also there is addition of PCS and SGMII interface.

>>
>> Signed-off-by: Parshuram Thombare <pthombar@...ence.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.

Do you mean a tab each for rate and break lines ?
All switch statements are aligned at a tab.  I am not sure how does case and rate got on same line.

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

I ran checkpatch.pl and all indentation issues were cleared. But I think having function
is better option, I will make that change.

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

PHY_INTERFACE_MODE_SGMII is selected only on the basis of presence of PCS.
This additional check is to make sure PHY also support 1G/2.5G.

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

Ok, I will change it.

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

My understanding is all GEM (ID >= 0x2) support GIGABIT mode. 
Was there any other reason for this check ?

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

My intention was to continue (instead of failing) with whatever functionality is available.
Can we have some error message and continue with what is available ?

Regards,
Parshuram Thombare

Powered by blists - more mailing lists