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;
Message-ID: <d6cfce0d7210762bf0c4130ea4d59470cbabfb56.camel@redhat.com>
Subject: Re: [PATCH 2/4] net: mediatek: sgmii: ensure the SGMII PHY is
 powered down on configuration
From:   Paolo Abeni <pabeni@redhat.com>
To:     Alexander 'lynxis' Couzens <lynxis@fe80.eu>
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>
Date:   Tue, 23 Aug 2022 18:38:00 +0200
In-Reply-To: <20220823161712.2cafa970@javelin>
References: <20220820224538.59489-1-lynxis@fe80.eu>
         <20220820224538.59489-3-lynxis@fe80.eu>
         <efd1c89e3bbd7364ef381292c9ceff430cbeda8d.camel@redhat.com>
         <20220823161712.2cafa970@javelin>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.4 (3.42.4-2.fc35) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 2022-08-23 at 16:17 +0200, Alexander 'lynxis' Couzens wrote:
> 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

I see. 

Since it looks like a new revision of this patchset will be needed - as
per Russell comments - I suggest to extend/replace the comments with
something more alike this longer description, it will make the future
mainteinance simpler.

Thanks,

Paolo