Re: [PATCH net-next 2/4] net: ethernet: ti: am65-cpsw: Add support for SGMII mode

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Wed, Mar 22, 2023 at 03:49:41PM +0100, Paolo Abeni wrote:
> Hi Russell,
> 
> On Tue, 2023-03-21 at 15:38 +0000, Russell King (Oracle) wrote:
> > On Tue, Mar 21, 2023 at 07:04:50PM +0530, Siddharth Vadapalli wrote:
> > > Hello Russell,
> > > 
> > > On 21-03-2023 16:59, Russell King (Oracle) wrote:
> > > > On Tue, Mar 21, 2023 at 04:49:56PM +0530, Siddharth Vadapalli wrote:
> > > > > Add support for configuring the CPSW Ethernet Switch in SGMII mode.
> > > > > 
> > > > > Depending on the SoC, allow selecting SGMII mode as a supported interface,
> > > > > based on the compatible used.
> > > > > 
> > > > > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> > > > > ---
> > > > >  drivers/net/ethernet/ti/am65-cpsw-nuss.c | 11 ++++++++++-
> > > > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > > > > index cba8db14e160..d2ca1f2035f4 100644
> > > > > --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > > > > +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> > > > > @@ -76,6 +76,7 @@
> > > > >  #define AM65_CPSW_PORTN_REG_TS_CTL_LTYPE2       0x31C
> > > > >  
> > > > >  #define AM65_CPSW_SGMII_CONTROL_REG		0x010
> > > > > +#define AM65_CPSW_SGMII_MR_ADV_ABILITY_REG	0x018
> > > > >  #define AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE	BIT(0)
> > > > 
> > > > Isn't this misplaced? Shouldn't AM65_CPSW_SGMII_MR_ADV_ABILITY_REG come
> > > > after AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE, rather than splitting that
> > > > from its register offset definition?
> > > 
> > > Thank you for reviewing the patch. The registers are as follows:
> > > CONTROL_REG offset 0x10
> > > STATUS_REG offset  0x14
> > > MR_ADV_REG offset  0x18
> > > 
> > > Since the STATUS_REG is not used in the driver, its offset is omitted.
> > > The next register is the MR_ADV_REG, which I placed after the
> > > CONTROL_REG. I grouped the register offsets together, to represent the
> > > order in which the registers are placed. Due to this, the
> > > MR_ADV_ABILITY_REG offset is placed after the CONTROL_REG offset define.
> > > 
> > > Please let me know if I should move it after the CONTROL_MR_AN_ENABLE
> > > define instead.
> > 
> > Well, it's up to you - whether you wish to group the register offsets
> > separately from the bit definitions for those registers, or whether
> > you wish to describe the register offset and its associated bit
> > definitions in one group before moving on to the next register.
> > 
> > > > If the advertisement register is at 0x18, and the lower 16 bits is the
> > > > advertisement, are the link partner advertisement found in the upper
> > > > 16 bits?
> > > 
> > > The MR_LP_ADV_ABILITY_REG is at offset 0x020, which is the the register
> > > corresponding to the Link Partner advertised value. Also, the
> > > AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE Bit is in the CONTROL_REG. The CPSW
> > > Hardware specification describes the process of configuring the CPSW MAC
> > > for SGMII mode as follows:
> > > 1. Write 0x1 (ADVERTISE_SGMII) to the MR_ADV_ABILITY_REG register.
> > > 2. Enable auto-negotiation in the CONTROL_REG by setting the
> > > AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE bit.
> > 
> > Good to hear that there is a link partner register.
> > 
> > > > >  #define AM65_CPSW_CTL_VLAN_AWARE		BIT(1)
> > > > > @@ -1496,9 +1497,14 @@ static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned in
> > > > >  	struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave);
> > > > >  	struct am65_cpsw_common *common = port->common;
> > > > >  
> > > > > -	if (common->pdata.extra_modes & BIT(state->interface))
> > > > > +	if (common->pdata.extra_modes & BIT(state->interface)) {
> > > > > +		if (state->interface == PHY_INTERFACE_MODE_SGMII)
> > > > > +			writel(ADVERTISE_SGMII,
> > > > > +			       port->sgmii_base + AM65_CPSW_SGMII_MR_ADV_ABILITY_REG);
> > > > > +
> > > > 
> > > > I think we can do better with this, by implementing proper PCS support.
> > > > 
> > > > It seems manufacturers tend to use bought-in IP for this, so have a
> > > > look at drivers/net/pcs/ to see whether any of those (or the one in
> > > > the Mediatek patch set on netdev that has recently been applied) will
> > > > idrive your hardware.
> > > > 
> > > > However, given the definition of AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE,
> > > > I suspect you won't find a compatible implementation.
> > > 
> > > I have tested with an SGMII Ethernet PHY in the standard SGMII MAC2PHY
> > > configuration. I am not sure if PCS support will be required or not. I
> > > hope that the information shared above by me regarding the CPSW
> > > Hardware's specification for configuring it in SGMII mode will help
> > > determine what the right approach might be. Please let me know whether
> > > the current implementation is acceptable or PCS support is necessary.
> > 
> > Nevertheless, this SGMII block is a PCS, and if you're going to want to
> > support inband mode (e.g. to read the SGMII word from the PHY), or if
> > someone ever wants to use 1000base-X, you're going to need to implement
> > this properly as a PCS.
> > 
> > That said, it can be converted later, so isn't a blocking sisue.
> 
> Just to be on the same page, I read all the above as you do accept/do
> not oppose to this series in the current form, am I correct?

I don't oppose this series.

Thanks.

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