mbox series

[net-next,v2,0/3] Add support for J784S4 CPSW9G

Message ID 20230403110106.983994-1-s-vadapalli@ti.com
Headers
Series Add support for J784S4 CPSW9G |

Message

Siddharth Vadapalli April 3, 2023, 11:01 a.m. UTC 
  Hello,

This series adds a new compatible to am65-cpsw driver for the CPSW9G
instance of the CPSW Ethernet Switch on TI's J784S4 SoC which has 8
external ports and 1 internal host port.

The CPSW9G instance supports QSGMII and USXGMII modes for which driver
support is added.

Additionally, the interface mode specific configurations are moved to the
am65_cpsw_nuss_mac_config() callback. Also, a TODO comment is added for
verifying whether in-band mode is necessary for 10 Mbps RGMII mode.

Changes from v1:
1. Add a patch to move interface mode specific configuration from the
   mac_link_up() callback to the mac_config() callback of the am65-cpsw
   driver. Also, add a TODO comment for 10 Mbps RGMII in-band mode.
2. Add MAC_5000FD to the list of mac_capabilities member unconditionally,
   since the CPSW MAC supports it.
3. Add USXGMII mode specific configuration in the mac_config() callback
   along with the SGMII mode specific configuration, instead of the
   mac_link_up() callback which was incorrectly done in the v1 series.

v1:
https://lore.kernel.org/r/20230331065110.604516-1-s-vadapalli@ti.com/

Regards,
Siddharth.

Siddharth Vadapalli (3):
  net: ethernet: ti: am65-cpsw: Move mode specific config to
    mac_config()
  net: ethernet: ti: am65-cpsw: Enable QSGMII for J784S4 CPSW9G
  net: ethernet: ti: am65-cpsw: Enable USXGMII mode for J784S4 CPSW9G

 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 32 +++++++++++++++++++++---
 1 file changed, 28 insertions(+), 4 deletions(-)

Comments

Russell King (Oracle) April 3, 2023, 11:08 a.m. UTC | #1 
On Mon, Apr 03, 2023 at 04:31:04PM +0530, Siddharth Vadapalli wrote:
> Move the interface mode specific configuration to the mac_config()
> callback am65_cpsw_nuss_mac_config().
> 
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---
>  drivers/net/ethernet/ti/am65-cpsw-nuss.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> index d17757ecbf42..74e099828978 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> @@ -1504,12 +1504,17 @@ static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned in
>  							  phylink_config);
>  	struct am65_cpsw_port *port = container_of(slave, struct am65_cpsw_port, slave);
>  	struct am65_cpsw_common *common = port->common;
> +	u32 mac_control = 0;
>  
>  	if (common->pdata.extra_modes & BIT(state->interface)) {
> -		if (state->interface == PHY_INTERFACE_MODE_SGMII)
> +		if (state->interface == PHY_INTERFACE_MODE_SGMII) {
> +			mac_control |= CPSW_SL_CTL_EXT_EN;
>  			writel(ADVERTISE_SGMII,
>  			       port->sgmii_base + AM65_CPSW_SGMII_MR_ADV_ABILITY_REG);
> +		}
>  
> +		if (mac_control)
> +			cpsw_sl_ctl_set(port->slave.mac_sl, mac_control);
>  		writel(AM65_CPSW_SGMII_CONTROL_MR_AN_ENABLE,
>  		       port->sgmii_base + AM65_CPSW_SGMII_CONTROL_REG);
>  	}
> @@ -1553,8 +1558,7 @@ static void am65_cpsw_nuss_mac_link_up(struct phylink_config *config, struct phy
>  
>  	if (speed == SPEED_1000)
>  		mac_control |= CPSW_SL_CTL_GIG;
> -	if (interface == PHY_INTERFACE_MODE_SGMII)
> -		mac_control |= CPSW_SL_CTL_EXT_EN;
> +	/* TODO: Verify whether in-band is necessary for 10 Mbps RGMII */
>  	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;

I'm afraid I can see you haven't thought this patch through properly.

am65_cpsw_nuss_mac_link_down() will call
cpsw_sl_ctl_reset(port->slave.mac_sl); which has the effect of clearing
to zero the entire MAC control register. This will clear
CPSW_SL_CTL_EXT_EN that was set in am65_cpsw_nuss_mac_config() which is
not what you want to be doing.

Given that we have the 10Mbps issue with RGMII, I think what you want
to be doing is:

1. Set CPSW_SL_CTL_EXT_EN in am65_cpsw_nuss_mac_config() if in SGMII
   mode, otherwise clear this bit.

2. Clear the mac_control register in am65_cpsw_nuss_mac_link_down()
   if in RMGII mode, otherwise preserve the state of
   CPSW_SL_CTL_EXT_EN but clear all other bits.

3. Set CPSW_SL_CTL_EXT_EN in am65_cpsw_nuss_mac_link_up() if in
   RGMII mode and 10Mbps.