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, 24 Jun 2019 10:35:33 +0100
From:   Russell King - ARM Linux admin <linux@armlinux.org.uk>
To:     Parshuram Raju Thombare <pthombar@cadence.com>
Cc:     "andrew@lunn.ch" <andrew@lunn.ch>,
        "nicolas.ferre@microchip.com" <nicolas.ferre@microchip.com>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "f.fainelli@gmail.com" <f.fainelli@gmail.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "hkallweit1@gmail.com" <hkallweit1@gmail.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Rafal Ciepiela <rafalc@cadence.com>,
        Anil Joy Varughese <aniljoy@cadence.com>,
        Piotr Sroka <piotrs@cadence.com>
Subject: Re: [PATCH v4 2/5] net: macb: add support for sgmii MAC-PHY interface
Message-ID: <20190624093533.4vhvjmqqrucq2ixf@shell.armlinux.org.uk>
References: <1561281419-6030-1-git-send-email-pthombar@cadence.com>
 <1561281781-13479-1-git-send-email-pthombar@cadence.com>
 <20190623101224.nzwodgfo6vvv65cx@shell.armlinux.org.uk>
 <CO2PR07MB246931C79F736F39D0523D3BC1E00@CO2PR07MB2469.namprd07.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CO2PR07MB246931C79F736F39D0523D3BC1E00@CO2PR07MB2469.namprd07.prod.outlook.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Jun 24, 2019 at 06:35:44AM +0000, Parshuram Raju Thombare wrote:
> 
> >> +	if (change_interface) {
> >> +		if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) {
> >> +			gem_writel(bp, NCFGR, ~GEM_BIT(SGMIIEN) &
> >> +				   ~GEM_BIT(PCSSEL) &
> >> +				   gem_readl(bp, NCFGR));
> >> +			gem_writel(bp, NCR, ~GEM_BIT(TWO_PT_FIVE_GIG) &
> >> +				   gem_readl(bp, NCR));
> >> +			gem_writel(bp, PCS_CTRL, gem_readl(bp, PCS_CTRL) |
> >> +				   GEM_BIT(PCS_CTRL_RST));
> >> +		}
> >I still don't think this makes much sense, splitting the interface
> >configuration between here and below.
> Do you mean splitting mac_config in two *_configure functions ?
> This was done as per Andrew's suggestion to make code mode readable
> and easy to manage by splitting MAC configuration for different interfaces.

No, I mean here you disable SGMII if we're switching away from SGMII
mode.... (note, this means there is more to come for this sentence)

> 
> >> +		bp->phy_interface = state->interface;
> >> +	}
> >> +
> >>  	if (!phylink_autoneg_inband(mode) &&
> >>  	    (bp->speed != state->speed ||
> >> -	     bp->duplex != state->duplex)) {
> >> +	     bp->duplex != state->duplex ||
> >> +	     change_interface)) {
> >>  		u32 reg;
> >>
> >>  		reg = macb_readl(bp, NCFGR);
> >>  		reg &= ~(MACB_BIT(SPD) | MACB_BIT(FD));
> >>  		if (macb_is_gem(bp))
> >>  			reg &= ~GEM_BIT(GBE);
> >> +		macb_or_gem_writel(bp, NCFGR, reg);
> >> +
> >> +		if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII)
> >> +			gem_writel(bp, NCFGR, GEM_BIT(SGMIIEN) |
> >> +				   GEM_BIT(PCSSEL) |
> >> +				   gem_readl(bp, NCFGR));
> >This will only be executed when we are not using inband mode, which
> >basically means it's not possible to switch to SGMII in-band mode.
> SGMII is used in default PHY mode. And above code is to program MAC to 
> select PCS and SGMII interface.

... and here you enable it for SGMII mode, but only for non-inband
modes.

For inband modes, you do not have any code that enables SGMII mode.
Since the only inband mode you support is SGMII, this is not very
good behaviour.

Why not:

	if (change_interface) {
		u32 ncfgr;

		bp->phy_interface = state->interface;

		// We don't support 2.5G modes
		gem_writel(bp, NCR, ~GEM_BIT(TWO_PT_FIVE_GIG) &
			   gem_readl(bp, NCR));

		ncfgr = gem_readl(bp, NCFGR);
		if (state->interface == PHY_INTERFACE_MODE_SGMII) {
			// Enable SGMII mode and PCS
			gem_writel(bp, NCFGR, ncfgr | GEM_BIT(SGMIIEN) |
				   GEM_BIT(PCSSEL));
		} else {
			// Disable SGMII mode and PCS
			gem_writel(bp, NCFGR, ncfgr & ~(GEM_BIT(SGMIIEN) |
				   GEM_BIT(PCSSEL)));

			// Reset PCS
			gem_writel(bp, PCS_CTRL, gem_readl(bp, PCS_CTRL) |
				   GEM_BIT(PCS_CTRL_RST));
		}
	}

	if (!phylink_autoneg_inband(mode) &&
	    (bp->speed != state->speed || bp->duplex != state->duplex)) {

?

> 
> >> +
> >> +		if (!interface_supported) {
> >> +			netdev_err(dev, "Phy mode %s not supported",
> >> +				   phy_modes(phy_mode));
> >> +			goto err_out_free_netdev;
> >> +		}
> >> +
> >>  		bp->phy_interface = phy_mode;
> >> +	} else {
> >> +		bp->phy_interface = phy_mode;
> >> +	}
> >If bp->phy_interface is PHY_INTERFACE_MODE_SGMII here, and mac_config()
> >is called with state->interface = PHY_INTERFACE_MODE_SGMII, then
> >mac_config() won't configure the MAC for the interface type - is that
> >intentional?
> 
> In mac_config configure MAC for non in-band mode, there is also check for speed, duplex
> changes. bp->speed and bp->duplex are initialized to SPEED_UNKNOWN and DUPLEX_UNKNOWN
> values so it is expected that for non in band mode state contains valid speed and duplex mode
> which are different from *_UNKNOWN values.

Sorry, this reply doesn't answer my question.  I'm not asking about
bp->speed and bp->duplex.  I'm asking:

1) why you are initialising bp->phy_interface here
2) you to consider the impact that has on the mac_config() implementation
   you are proposing

because I think it's buggy.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up