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 11:22:24 +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: <20190624102224.6gudxxdaz43wrlcc@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>
 <20190624093533.4vhvjmqqrucq2ixf@shell.armlinux.org.uk>
 <CO2PR07MB24699250A3773DE76B6D2E9EC1E00@CO2PR07MB2469.namprd07.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CO2PR07MB24699250A3773DE76B6D2E9EC1E00@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 10:14:41AM +0000, Parshuram Raju Thombare wrote:
> >> >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)
> Sorry, I misunderstood your original question. I think disabling old interface
> and enabling new one can be done in single place. I will do this change.
> 
> >> >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.
> >
> >Why not:
> >	if (change_interface)  {
> >		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)) {
> >?
> Ok
> 
> >> >> +
> >> >> +		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.
> bp->phy_interface is to store phy mode value from device tree. This is used later 
> to know what phy interface user has selected for PHY-MAC. Same is used
> to configure MAC correctly and based on your suggestion code is
> added to handle PHY dynamically changing phy interface, in which 
> case bp->phy_interface is also updated. Though it may not be what user want, 
> if phy interface is totally decided by PHY and is anyway going to be different from what user
> has selected in DT, initializing it here doesn't make sense.
> But in case of PHY not changing phy_interface dynamically bp->phy_interface need to be
> initialized with value from DT.

When phylink_start() is called, you will receive a mac_config() call to
configure the MAC for the initial operating settings, which will include
the current PHY interface mode.  This will initially be whatever
interface mode was passed in to phylink_create().

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