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

[Paolo Abeni]

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 at 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?

Thanks,

Paolo