Date:   Wed, 24 Nov 2021 16:04:33 +0000
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Horatiu Vultur <horatiu.vultur@microchip.com>
Cc:     davem@davemloft.net, kuba@kernel.org, robh+dt@kernel.org,
        UNGLinuxDriver@microchip.com, p.zabel@pengutronix.de,
        netdev@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v3 3/6] net: lan966x: add port module support
Message-ID: <YZ5ikamCVeyGFw3x@shell.armlinux.org.uk>
References: <20211124083915.2223065-1-horatiu.vultur@microchip.com>
 <20211124083915.2223065-4-horatiu.vultur@microchip.com>
 <YZ4SB/wX6UT3zrEV@shell.armlinux.org.uk>
 <20211124145800.my4niep3sifqpg55@soft-dev3-1.localhost>
 <YZ5UXdiNNf011skU@shell.armlinux.org.uk>
 <20211124154323.44liimrwzthsh547@soft-dev3-1.localhost>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211124154323.44liimrwzthsh547@soft-dev3-1.localhost>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Wed, Nov 24, 2021 at 04:43:23PM +0100, Horatiu Vultur wrote:
> > > Actually, port->config.phy_mode will not get zeroed. Because right after
> > > the memset it follows: 'config = port->config'.
> > 
> > Ah, missed that, thanks. However, why should portmode and phy_mode be
> > different?
> 
> Because the serdes knows only few modes(QSGMII, SGMII, GMII) and this
> information will come from DT. So I would like to have one variable that
> will configure the serdes ('phy_mode') and one will configure the MAC
> ('portmode').

I don't follow why you need this to be different.

Isn't the point of interfaces such as phy_set_mode_ext() such that we
can achieve independence of the details of what is behind that
interface - so, as it takes a PHY interface mode, if we're operating
in 1000BASE-X, we pass that to phy_set_mode_ext(). It is then the
responsibility of the Serdes PHY driver to decide that means "sgmii"
mode for the Serdes?

For example, the Marvell CP110 comphy driver does this:

        if (submode == PHY_INTERFACE_MODE_1000BASEX)
                submode = PHY_INTERFACE_MODE_SGMII;

because the serdes phy settings for PHY_INTERFACE_MODE_1000BASEX are
no different from PHY_INTERFACE_MODE_SGMII - and that detail is hidden
from the network driver.

The next question this brings up is... you're setting all the different
interface modes in phylink_config.supported_interfaces, which basically
means you're giving permission for phylink to switch between any of
those modes. So, what if the serdes is in QSGMII mode but phylink
requests SGMII mode. Doesn't your driver architecture mean that if
you're in QSGMII mode you can't use SGMII or GMII mode?

Is there some kind of restriction that you need to split this, or is
this purely down to the way this driver has been written?

I don't see any other driver in the kernel making this kind of split.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!