Re: [PATCH 3/4] net: mediatek: sgmii: mtk_pcs_setup_mode_an: don't rely on register defaults

[Russell King (Oracle)]

On Fri, Sep 02, 2022 at 05:47:10PM +0200, Alexander 'lynxis' Couzens wrote:
> On Tue, 23 Aug 2022 16:28:47 +0100
> "Russell King (Oracle)" <linux@xxxxxxxxxxxxxxx> wrote:
> 
> > On Sun, Aug 21, 2022 at 12:45:37AM +0200, Alexander Couzens wrote:
> > > Ensure autonegotiation is enabled.
> > > 
> > > Signed-off-by: Alexander Couzens <lynxis@xxxxxxx>
> > > ---
> > >  drivers/net/ethernet/mediatek/mtk_sgmii.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c
> > > b/drivers/net/ethernet/mediatek/mtk_sgmii.c index
> > > 782812434367..aa69baf1a42f 100644 ---
> > > a/drivers/net/ethernet/mediatek/mtk_sgmii.c +++
> > > b/drivers/net/ethernet/mediatek/mtk_sgmii.c @@ -32,12 +32,13 @@
> > > static int mtk_pcs_setup_mode_an(struct mtk_pcs *mpcs)
> > > regmap_write(mpcs->regmap, SGMSYS_PCS_LINK_TIMER,
> > > SGMII_LINK_TIMER_DEFAULT); 
> > > +	/* disable remote fault & enable auto neg */
> > >  	regmap_read(mpcs->regmap, SGMSYS_SGMII_MODE, &val);
> > > -	val |= SGMII_REMOTE_FAULT_DIS;
> > > +	val |= SGMII_REMOTE_FAULT_DIS | SGMII_SPEED_DUPLEX_AN;  
> > 
> > Does SGMII_SPEED_DUPLEX_AN need to be cleared in
> > mtk_pcs_setup_mode_force(), so mtk_pcs_link_up() can force the
> > duplex setting for base-X protocols?
> > 
> 
> Yes SGMII_SPEED_DUPLEX_AN needs to be cleared to have FORCE_DUPLEX
> working. But mtk_pcs_setup_mode_force() is clearing it implicit by
> 
> val &= SGMII_DUPLEX_FULL & ~SGMII_IF_MODE_MASK

It would make the code more reasonable to spell out what is included
in SGMII_IF_MODE_MASK. It's:

#define SGMII_SPEED_DUPLEX_AN           BIT(1)
#define SGMII_SPEED_MASK                GENMASK(3, 2)
#define SGMII_DUPLEX_FULL               BIT(4)
#define SGMII_IF_MODE_BIT5              BIT(5)

I'm guessing no one knows what SGMII_IF_MODE_BIT0 and
SGMII_IF_MODE_BIT5 actually do - and as neither seem to be used in
the code, the definitions are redundant.

> because it's included in the SGMII_IF_MODE_MASK.
> I also don't understand why it's forcing it in the
> mtk_pcs_link_up().

Please note that I've forgotten the contents of these patches, so
these comments may not be entirely accurate...

A lot of the code in the driver is quite weird, so when I converted it
to phylink_pcs, I tried to keep the decision making the same as in the
original code. It would help readability if the decision making was
cleaned up - so similar tests in mtk_pcs_link_up() and
mtk_pcs_config().

By that, I mean - if the test in mtk_pcs_link_up() is for 802.3z modes,
then shouldn't the test in mtk_pcs_config() also be using the same?
>From what I understand from mtk_eth_soc.c as it originally stood, the
PCS driver is only used for SGMII, 1000base-X and 2500base-X.

The SGMII duplex setting is changed in mtk_pcs_link_up() only for 802.3z
modes - in other words, 1000base-X and 2500base-X.

When mtk_pcs_config() is called for 1000base-X and 2500base-X, it calls
mtk_pcs_setup_mode_force(). This clears SGMII_AN_ENABLE, disabling
autonegotiation, and forcing 1000Mbps. Presumably, this PCS as the code
was originally written is only capable of SGMII negotiation.

It looks to me like the original code does not support autonegotiation
on 802.3z interface modes.

Thanks.

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