Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 17 Jun 2019 17:01:45 +0200
From:   Andrew Lunn <andrew@lunn.ch>
To:     Parshuram Thombare <pthombar@cadence.com>
Cc:     nicolas.ferre@microchip.com, davem@davemloft.net,
        f.fainelli@gmail.com, netdev@vger.kernel.org, hkallweit1@gmail.com,
        linux-kernel@vger.kernel.org, rafalc@cadence.com,
        aniljoy@cadence.com, piotrs@cadence.com
Subject: Re: [PATCH 2/6] net: macb: add support for sgmii MAC-PHY interface
Message-ID: <20190617150145.GF25211@lunn.ch>
References: <1560642367-26425-1-git-send-email-pthombar@cadence.com>
 <1560642409-27074-1-git-send-email-pthombar@cadence.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1560642409-27074-1-git-send-email-pthombar@cadence.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

> @@ -159,6 +160,9 @@
>  #define GEM_PEFTN		0x01f4 /* PTP Peer Event Frame Tx Ns */
>  #define GEM_PEFRSL		0x01f8 /* PTP Peer Event Frame Rx Sec Low */
>  #define GEM_PEFRN		0x01fc /* PTP Peer Event Frame Rx Ns */
> +#define GEM_PCS_CTRL		0x0200 /* PCS Control */
> +#define GEM_PCS_STATUS          0x0204 /* PCS Status */
> +#define GEM_PCS_AN_LP_BASE      0x0214 /* PCS AN LP BASE*/

It looks like there are some space vs tab issues here and else where.

>  static int gem_phylink_mac_link_state(struct phylink_config *pl_config,
>  				      struct phylink_link_state *state)
>  {
> +	u32 status;
>  	struct net_device *netdev = to_net_dev(pl_config->dev);
>  	struct macb *bp = netdev_priv(netdev);

Reverse christmas tree please, here and everywhere you add new
variables.

>  
> -	state->speed = bp->speed;
> -	state->duplex = bp->duplex;
> -	state->link = bp->link;
> +	if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) {
> +		status = gem_readl(bp, PCS_STATUS);
> +		state->an_complete = GEM_BFEXT(PCS_STATUS_AN_DONE, status);
> +		status = gem_readl(bp, PCS_AN_LP_BASE);
> +		switch (GEM_BFEXT(PCS_AN_LP_BASE_SPEED, status)) {
> +		case 0:
> +			state->speed = 10;
> +			break;
> +		case 1:
> +			state->speed = 100;
> +			break;
> +		case 2:
> +			state->speed = 1000;
> +			break;
> +		default:
> +			break;

It would be nice to use SPEED_10, SPEED_100, etc.

> @@ -494,17 +551,23 @@ static void gem_mac_config(struct phylink_config *pl_config, unsigned int mode,
>  		reg &= ~(MACB_BIT(SPD) | MACB_BIT(FD));
>  		if (macb_is_gem(bp))
>  			reg &= ~GEM_BIT(GBE);
> -
>  		if (state->duplex)
>  			reg |= MACB_BIT(FD);
> -		if (state->speed == SPEED_100)
> -			reg |= MACB_BIT(SPD);
> -		if (state->speed == SPEED_1000 &&
> -		    bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)
> -			reg |= GEM_BIT(GBE);
> -
>  		macb_or_gem_writel(bp, NCFGR, reg);
>  
> +		if (state->speed == SPEED_2500) {
> +			gem_writel(bp, NCFGR, GEM_BIT(GBE) |
> +				   gem_readl(bp, NCFGR));
> +			gem_writel(bp, NCR, GEM_BIT(TWO_PT_FIVE_GIG) |
> +				   gem_readl(bp, NCR));
> +		} else if (state->speed == SPEED_1000) {
> +			gem_writel(bp, NCFGR, GEM_BIT(GBE) |
> +				   gem_readl(bp, NCFGR));
> +		} else if (state->speed == SPEED_100) {
> +			macb_writel(bp, NCFGR, MACB_BIT(SPD) |
> +				    macb_readl(bp, NCFGR));
> +		}

Maybe a switch statement?

> @@ -4232,11 +4327,37 @@ static int macb_probe(struct platform_device *pdev)
>  	}
>  
>  	err = of_get_phy_mode(np);

The following code would be more readable if you replaced err with
phy_mode, or interface.

> -	if (err < 0)
> +	if (err < 0) {
>  		/* not found in DT, MII by default */
>  		bp->phy_interface = PHY_INTERFACE_MODE_MII;
> -	else
> +	} else if (bp->caps & MACB_CAPS_MACB_IS_GEM_GXL) {
> +		u32 interface_supported = 1;
> +
> +		if (err == PHY_INTERFACE_MODE_SGMII ||
> +		    err == PHY_INTERFACE_MODE_1000BASEX ||
> +		    err == PHY_INTERFACE_MODE_2500BASEX) {
> +			if (!(bp->caps & MACB_CAPS_PCS))
> +				interface_supported = 0;
> +		} else if (err == PHY_INTERFACE_MODE_GMII ||
> +			   err == PHY_INTERFACE_MODE_RGMII) {
> +			if (!macb_is_gem(bp))
> +				interface_supported = 0;
> +		} else if (err != PHY_INTERFACE_MODE_RMII &&
> +			   err != PHY_INTERFACE_MODE_MII) {
> +			/* Add new mode before this */
> +			interface_supported = 0;
> +		}
> +
> +		if (!interface_supported) {
> +			netdev_err(dev, "Phy mode %s not supported",
> +				   phy_modes(err));
> +			goto err_out_free_netdev;
> +		}
> +
>  		bp->phy_interface = err;
> +	} else {
> +		bp->phy_interface = err;
> +	}

  Andrew