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 16:17:12 +0200
From:   Alexander 'lynxis' Couzens <lynxis@fe80.eu>
To:     Paolo Abeni <pabeni@redhat.com>
Cc:     Felix Fietkau <nbd@nbd.name>, John Crispin <john@phrozen.org>,
        Sean Wang <sean.wang@mediatek.com>,
        Mark Lee <Mark-MC.Lee@mediatek.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        Russell King <linux@armlinux.org.uk>, netdev@vger.kernel.org,
        linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org,
        Daniel Golle <daniel@makrotopia.org>
Subject: Re: [PATCH 2/4] net: mediatek: sgmii: ensure the SGMII PHY is
 powered down on configuration
Message-ID: <20220823161712.2cafa970@javelin>
In-Reply-To: <efd1c89e3bbd7364ef381292c9ceff430cbeda8d.camel@redhat.com>
References: <20220820224538.59489-1-lynxis@fe80.eu>
        <20220820224538.59489-3-lynxis@fe80.eu>
        <efd1c89e3bbd7364ef381292c9ceff430cbeda8d.camel@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 23 Aug 2022 15:18:31 +0200
Paolo Abeni <pabeni@redhat.com> wrote:

> On Sun, 2022-08-21 at 00:45 +0200, Alexander Couzens wrote:
> > The code expect the PHY to be in power down which is only true
> > after reset. Allow changes of the SGMII parameters more than once.
> > 
> > Signed-off-by: Alexander Couzens <lynxis@fe80.eu>
> > ---
> >  drivers/net/ethernet/mediatek/mtk_sgmii.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c
> > b/drivers/net/ethernet/mediatek/mtk_sgmii.c index
> > a01bb20ea957..782812434367 100644 ---
> > a/drivers/net/ethernet/mediatek/mtk_sgmii.c +++
> > b/drivers/net/ethernet/mediatek/mtk_sgmii.c @@ -7,6 +7,7 @@
> >   *
> >   */
> >  
> > +#include <linux/delay.h>
> >  #include <linux/mfd/syscon.h>
> >  #include <linux/of.h>
> >  #include <linux/phylink.h>
> > @@ -24,6 +25,9 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs
> > *mpcs) {
> >  	unsigned int val;
> >  
> > +	/* PHYA power down */
> > +	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL,
> > SGMII_PHYA_PWD);  
> 
> in mtk_pcs_setup_mode_an() and in mtk_pcs_setup_mode_force() the code
> carefully flips only the SGMII_PHYA_PWD bit. Is it safe to overwrite
> the full register contents?

I've read out the register without my patch and it's 0x0. The old driver
worked as long the engine came out of reset.
When writing the single bit SGMII_PHYA_PWD (0x10), the register might
end up containing 0x19 and as long 0x9 is in the register the link
doesn't work.

I've tested the driver with a mt7622 and Daniel Golle tested it with a
mt7986.


> 
> > +
> >  	/* Setup the link timer and QPHY power up inside SGMIISYS
> > */ regmap_write(mpcs->regmap, SGMSYS_PCS_LINK_TIMER,
> >  		     SGMII_LINK_TIMER_DEFAULT);
> > @@ -36,6 +40,10 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs
> > *mpcs) val |= SGMII_AN_RESTART;
> >  	regmap_write(mpcs->regmap, SGMSYS_PCS_CONTROL_1, val);
> >  
> > +	/* Release PHYA power down state
> > +	 * unknown how much the QPHY needs but it is racy without
> > a sleep
> > +	 */
> > +	usleep_range(50, 100);  
> 
> Ouch, this looks fragile, without any related H/W specification. 

The datasheet [1] doesn't say anything about it. I'ven't found a
mediatek SDK which adds a usleep(). It seems they always expect the
SGMII came out of reset and don't change after initial configured.
But without it, it's racy.

[1] MT7622 Reference Manual, v1.0, 2018-12-19, 1972 pages

> 
> >  	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, 0);
> >  
> >  	return 0;
> > @@ -50,6 +58,9 @@ static int mtk_pcs_setup_mode_force(struct
> > mtk_pcs *mpcs, {
> >  	unsigned int val;
> >  
> > +	/* PHYA power down */
> > +	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL,
> > SGMII_PHYA_PWD); +
> >  	regmap_read(mpcs->regmap, mpcs->ana_rgc3, &val);
> >  	val &= ~RG_PHY_SPEED_MASK;
> >  	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> > @@ -67,7 +78,10 @@ static int mtk_pcs_setup_mode_force(struct
> > mtk_pcs *mpcs, val |= SGMII_SPEED_1000;
> >  	regmap_write(mpcs->regmap, SGMSYS_SGMII_MODE, val);
> >  
> > -	/* Release PHYA power down state */
> > +	/* Release PHYA power down state
> > +	 * unknown how much the QPHY needs but it is racy without
> > a sleep
> > +	 */
> > +	usleep_range(50, 100);  
> 
> Same here.

Best,
lynxis