Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 23 Aug 2022 11:37:12 -0700
From:   Jakub Kicinski <kuba@kernel.org>
To:     Andrew Lunn <andrew@lunn.ch>
Cc:     Frank <Frank.Sae@motor-comm.com>, Peter Geis <pgwipeout@gmail.com>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        "David S . Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Paolo Abeni <pabeni@redhat.com>, yinghong.zhang@motor-comm.com,
        fei.zhang@motor-comm.com, hua.sun@motor-comm.com,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4.4] net: phy: Add driver for Motorcomm yt8521 gigabit
 ethernet phy
Message-ID: <20220823113712.4c530516@kernel.org>
In-Reply-To: <YwTguA0azox3j5vi@lunn.ch>
References: <20220817112554.383-1-Frank.Sae@motor-comm.com>
        <20220822202147.4be904de@kernel.org>
        <YwTguA0azox3j5vi@lunn.ch>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 23 Aug 2022 16:14:16 +0200 Andrew Lunn wrote:
> On Mon, Aug 22, 2022 at 08:21:47PM -0700, Jakub Kicinski wrote:
> > On Wed, 17 Aug 2022 19:25:54 +0800 Frank wrote:  
> > > +static int yt8521_fiber_config_aneg(struct phy_device *phydev)
> > > +{
> > > +	int err, changed;
> > > +	u16 adv;
> > > +
> > > +	if (phydev->autoneg != AUTONEG_ENABLE)
> > > +		return yt8521_fiber_setup_forced(phydev);
> > > +
> > > +	err =  ytphy_modify_ext_with_lock(phydev, YTPHY_MISC_CONFIG_REG,
> > > +					  YTPHY_MCR_FIBER_SPEED_MASK,
> > > +					  YTPHY_MCR_FIBER_1000BX);
> > > +	if (err < 0)
> > > +		return err;
> > > +
> > > +	/* enable Fiber auto sensing */
> > > +	err =  ytphy_modify_ext_with_lock(phydev, YT8521_LINK_TIMER_CFG2_REG,
> > > +					  0, YT8521_LTCR_EN_AUTOSEN);
> > > +	if (err < 0)
> > > +		return err;
> > > +
> > > +	/* Setup fiber advertisement */
> > > +	adv = ADVERTISE_1000XFULL | ADVERTISE_1000XPAUSE |
> > > +	      ADVERTISE_1000XPSE_ASYM;  
> > 
> > Is it okay to ignore phydev->advertising and always set the same mask?  
> 
> The user could of changed the pause settings, which are going to be
> ignored here. Also, you should not assume the MAC can actually do
> asymmetric pause, not all can. phydev->advertising will be set to only
> include what the MAC can actually do.
> 
> The whole concept of having two line sides connected to one MAC and
> seeing which gets link first is unsupported in Linux. In theory, you
> want to be able to configure each line side differently. Maybe you
> want autoneg on copper, but fixed on fibre, asymmetric pause with
> fibre, but symmetric pause on copper, etc. Since there is only one
> instance of phydev here, you don't have anywhere to store two sets of
> configuration, nor any sort of kAPI to deal with two phydev structures
> etc. So the user experience is not so great.
> 
> With the Marvell Switches which also have this capability, i actually
> ignore it, use the phy-mode it decide which should be used, either
> copper or fibre, and leave the other powered off so it can never get
> link. There is at least one Marvell PHY which does however support
> first up wins, so this behaviour is not new. I just don't recommend
> it.
> 
> And it gets even more interesting when the SFP is actually copper. But
> since the integration with phylink is missing in this driver, that is
> not supported here.

Interesting. Just to confirm - regardless of the two-sided design..
-edness.. IIUC my question has merit and we need v5?