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: <3b9793e1b2d16c49eb4af40c2c48609a09e2bc5a.camel@redhat.com>
Subject: Re: [PATCH net-next 2/4] net: ethernet: ti: am65-cpsw: Add support
 for SGMII mode
From:   Paolo Abeni <pabeni@redhat.com>
To:     "Russell King (Oracle)" <linux@armlinux.org.uk>,
        Siddharth Vadapalli <s-vadapalli@ti.com>
Cc:     davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        rogerq@kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        srk@ti.com
Date:   Wed, 22 Mar 2023 15:49:41 +0100
In-Reply-To: <ZBnPdlFS2P3Iie5k@shell.armlinux.org.uk>
References: <20230321111958.2800005-1-s-vadapalli@ti.com>
         <20230321111958.2800005-3-s-vadapalli@ti.com>
         <ZBmVGu2vf1ADmEuN@shell.armlinux.org.uk>
         <9b9ba199-8379-0840-b99a-d729f8ad33e1@ti.com>
         <ZBnPdlFS2P3Iie5k@shell.armlinux.org.uk>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

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,
> >=20
> > 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.
> > > >=20
> > > > Depending on the SoC, allow selecting SGMII mode as a supported int=
erface,
> > > > based on the compatible used.
> > > >=20
> > > > 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(-)
> > > >=20
> > > > 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
> > > > =20
> > > >  #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)
> > >=20
> > > Isn't this misplaced? Shouldn't AM65_CPSW_SGMII_MR_ADV_ABILITY_REG co=
me
> > > after AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE, rather than splitting tha=
t
> > > from its register offset definition?
> >=20
> > Thank you for reviewing the patch. The registers are as follows:
> > CONTROL_REG offset 0x10
> > STATUS_REG offset  0x14
> > MR_ADV_REG offset  0x18
> >=20
> > 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=
.
> >=20
> > Please let me know if I should move it after the CONTROL_MR_AN_ENABLE
> > define instead.
>=20
> 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.
>=20
> > > If the advertisement register is at 0x18, and the lower 16 bits is th=
e
> > > advertisement, are the link partner advertisement found in the upper
> > > 16 bits?
> >=20
> > 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 CPS=
W
> > Hardware specification describes the process of configuring the CPSW MA=
C
> > 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.
>=20
> Good to hear that there is a link partner register.
>=20
> > > >  #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 =3D container_of(slave, struct am65_c=
psw_port, slave);
> > > >  	struct am65_cpsw_common *common =3D port->common;
> > > > =20
> > > > -	if (common->pdata.extra_modes & BIT(state->interface))
> > > > +	if (common->pdata.extra_modes & BIT(state->interface)) {
> > > > +		if (state->interface =3D=3D PHY_INTERFACE_MODE_SGMII)
> > > > +			writel(ADVERTISE_SGMII,
> > > > +			       port->sgmii_base + AM65_CPSW_SGMII_MR_ADV_ABILITY_REG);
> > > > +
> > >=20
> > > I think we can do better with this, by implementing proper PCS suppor=
t.
> > >=20
> > > 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.
> > >=20
> > > However, given the definition of AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE=
,
> > > I suspect you won't find a compatible implementation.
> >=20
> > 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.
>=20
> 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.
>=20
> 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