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;
Date:   Fri, 31 Mar 2023 08:57:17 +0100
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/2] net: ethernet: ti: am65-cpsw: Enable
 USXGMII mode for J784S4 CPSW9G
Message-ID: <ZCaSXQFZ/e/JIDEj@shell.armlinux.org.uk>
References: <20230331065110.604516-1-s-vadapalli@ti.com>
 <20230331065110.604516-3-s-vadapalli@ti.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230331065110.604516-3-s-vadapalli@ti.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Fri, Mar 31, 2023 at 12:21:10PM +0530, Siddharth Vadapalli wrote:
> TI's J784S4 SoC supports USXGMII mode. Add USXGMII mode to the
> extra_modes member of the J784S4 SoC data. Additionally, configure the
> MAC Control register for supporting USXGMII mode. Also, for USXGMII
> mode, include MAC_5000FD in the "mac_capabilities" member of struct
> "phylink_config".

I don't think TI "get" phylink at all...

> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> index 4b4d06199b45..ab33e6fe5b1a 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> @@ -1555,6 +1555,8 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy
>  		mac_control |= CPSW_SL_CTL_GIG;
>  	if (interface == PHY_INTERFACE_MODE_SGMII)
>  		mac_control |= CPSW_SL_CTL_EXT_EN;
> +	if (interface == PHY_INTERFACE_MODE_USXGMII)
> +		mac_control |= CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN;

The configuration of the interface mode should *not* happen in
mac_link_up(), but should happen in e.g. mac_config().

>  	if (speed == SPEED_10 && phy_interface_mode_is_rgmii(interface))
>  		/* Can be used with in band mode only */
>  		mac_control |= CPSW_SL_CTL_EXT_EN;
> @@ -2175,6 +2177,7 @@ am65_cpsw_nuss_init_port_ndev(struct am65_cpsw_common *common, u32 port_idx)
>  
>  	case PHY_INTERFACE_MODE_QSGMII:
>  	case PHY_INTERFACE_MODE_SGMII:
> +	case PHY_INTERFACE_MODE_USXGMII:
>  		if (common->pdata.extra_modes & BIT(port->slave.phy_if)) {
>  			__set_bit(port->slave.phy_if,
>  				  port->slave.phylink_config.supported_interfaces);
> @@ -2182,6 +2185,9 @@ am65_cpsw_nuss_init_port_ndev(struct am65_cpsw_common *common, u32 port_idx)
>  			dev_err(dev, "selected phy-mode is not supported\n");
>  			return -EOPNOTSUPP;
>  		}
> +		/* For USXGMII mode, enable MAC_5000FD */
> +		if (port->slave.phy_if == PHY_INTERFACE_MODE_USXGMII)
> +			port->slave.phylink_config.mac_capabilities |= MAC_5000FD;

MAC capabilities should not be conditional in the interface mode.
Phylink already knows the capabilities of each interface mode, and
will mask the mac_capabilities accordingly. Phylink wants to know
what speeds the MAC itself is capable of unbound by the interface
mode.

The interface modes that you already support (RGMII, RMII, QSGMII
and SGMII) do not support anything faster than 1G, so only
mac_capabilities up to and including 1G speeds will be permitted
for those interface modes internally by phylink.

So, making this conditional on USXGMII is just repeating logic that
is already present internally in phylink.

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