Date:   Tue, 21 Mar 2023 11:29:30 +0000
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Siddharth Vadapalli <s-vadapalli@ti.com>
Cc:     davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, rogerq@kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        srk@ti.com
Subject: Re: [PATCH net-next 2/4] net: ethernet: ti: am65-cpsw: Add support
 for SGMII mode
Message-ID: <ZBmVGu2vf1ADmEuN@shell.armlinux.org.uk>
References: <20230321111958.2800005-1-s-vadapalli@ti.com>
 <20230321111958.2800005-3-s-vadapalli@ti.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230321111958.2800005-3-s-vadapalli@ti.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

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?

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?

>  #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.

Thanks.

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