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

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

[Siddharth Vadapalli]

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(-)

-- 
2.25.1

[PATCH net-next v2 1/3] net: ethernet: ti: am65-cpsw: Move mode specific config to mac_config()

[Siddharth Vadapalli]

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;
-- 
2.25.1

Re: [PATCH net-next v2 1/3] net: ethernet: ti: am65-cpsw: Move mode specific config to mac_config()

[Russell King (Oracle)]

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.

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

Re: [PATCH net-next v2 1/3] net: ethernet: ti: am65-cpsw: Move mode specific config to mac_config()

[Siddharth Vadapalli]

Hello Russell,

Thank you for reviewing the patch.

On 03/04/23 16:38, Russell King (Oracle) wrote:
> 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.

Right! I missed noticing this. It appeared to me that simply moving the code
from mac_link_up() to mac_config() would suffice. However, as rightly pointed
out by you, it doesn't account for the case where the interface state is toggled.

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

I will implement these changes and post the v3 series. Thank you for the
feedback. Please let me know if I may add a "Suggested-by" tag.

Regards,
Siddharth.

Re: [PATCH net-next v2 1/3] net: ethernet: ti: am65-cpsw: Move mode specific config to mac_config()

[Siddharth Vadapalli]

On 03-04-2023 16:38, Russell King (Oracle) wrote:
> 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.

I plan to implement it as follows:
1. Add a member "u32 mode_config" to "struct am65_cpsw_slave_data" in
"am65-cpsw-nuss.h".
2. In am65_cpsw_nuss_mac_config(), store the value of mac_control in
"port->slave.mode_config".
3. In am65_cpsw_nuss_mac_link_down(), after the reset via
cpsw_sl_ctl_reset(), execute:
cpsw_sl_ctl_set(port->slave.mac_sl, port->slave.mode_config) in order to
restore the configuration performed in am65_cpsw_nuss_mac_config().

Please let me know in case of any suggestions to implement it in a
better manner.

Regards,
Siddharth.

Re: [PATCH net-next v2 1/3] net: ethernet: ti: am65-cpsw: Move mode specific config to mac_config()

[Russell King (Oracle)]

On Mon, Apr 03, 2023 at 06:31:52PM +0530, Siddharth Vadapalli wrote:
> 
> 
> On 03-04-2023 16:38, Russell King (Oracle) wrote:
> > 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.
> 
> I plan to implement it as follows:
> 1. Add a member "u32 mode_config" to "struct am65_cpsw_slave_data" in
> "am65-cpsw-nuss.h".
> 2. In am65_cpsw_nuss_mac_config(), store the value of mac_control in
> "port->slave.mode_config".
> 3. In am65_cpsw_nuss_mac_link_down(), after the reset via
> cpsw_sl_ctl_reset(), execute:
> cpsw_sl_ctl_set(port->slave.mac_sl, port->slave.mode_config) in order to
> restore the configuration performed in am65_cpsw_nuss_mac_config().
> 
> Please let me know in case of any suggestions to implement it in a
> better manner.

Do you think this complexity is really worth it?

Let's look at what's available:

cpsw_sl_ctl_set() - sets bits in the mac control register
cpsw_sl_ctl_clr() - clears bits in the mac control register
cpsw_sl_ctl_reset() - sets the mac control register to zero

So, in mac_config(), we can do:

	if (interface == SGMII)
		cpsw_sl_ctl_set(CPSW_SL_CTL_EXT_EN);
	else
		cpsw_sl_ctl_clr(CPSW_SL_CTL_EXT_EN);


in mac_link_down():

	u32 ctl;

	ctl = CPSW_SL_CTL_GMII_EN | CPSW_SL_CTL_GIG |
	      CPSW_SL_CTL_IFCTL_A | CPSW_SL_CTL_FULLDUPLEX |
	      CPSW_SL_CTL_RX_FLOW_EN | CPSW_SL_CTL_TX_FLOW_EN;
	if (phy_interface_mode_is_rgmii(interface))
		ctl |= CPSW_SL_CTL_EXT_EN;

	cpsw_sl_ctl_clr(ctl);

This ensures that we don't touch any bits in mac_link_down() which we
aren't modifying in the corresponding mac_link_up() implementation.

Q: do we really need to clear the mac control register on link down?
If we don't, then we can do better, but we need an additional helper
which allows read-modify-write of the mac control register using a
mask value and a value of bits to set. Then we can have mac_link_up()
setting and clearing the bits as necessary - but I would still keep
the clearing of CPSW_SL_CTL_EXT_EN for RGMII modes in mac_link_down()
for now.

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

Re: [PATCH net-next v2 1/3] net: ethernet: ti: am65-cpsw: Move mode specific config to mac_config()

[Siddharth Vadapalli]

On 03-04-2023 18:43, Russell King (Oracle) wrote:
> On Mon, Apr 03, 2023 at 06:31:52PM +0530, Siddharth Vadapalli wrote:
>>
>>
>> On 03-04-2023 16:38, Russell King (Oracle) wrote:
>>> 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.
>>
>> I plan to implement it as follows:
>> 1. Add a member "u32 mode_config" to "struct am65_cpsw_slave_data" in
>> "am65-cpsw-nuss.h".
>> 2. In am65_cpsw_nuss_mac_config(), store the value of mac_control in
>> "port->slave.mode_config".
>> 3. In am65_cpsw_nuss_mac_link_down(), after the reset via
>> cpsw_sl_ctl_reset(), execute:
>> cpsw_sl_ctl_set(port->slave.mac_sl, port->slave.mode_config) in order to
>> restore the configuration performed in am65_cpsw_nuss_mac_config().
>>
>> Please let me know in case of any suggestions to implement it in a
>> better manner.
> 
> Do you think this complexity is really worth it?
> 
> Let's look at what's available:
> 
> cpsw_sl_ctl_set() - sets bits in the mac control register
> cpsw_sl_ctl_clr() - clears bits in the mac control register
> cpsw_sl_ctl_reset() - sets the mac control register to zero
> 
> So, in mac_config(), we can do:
> 
> 	if (interface == SGMII)
> 		cpsw_sl_ctl_set(CPSW_SL_CTL_EXT_EN);
> 	else
> 		cpsw_sl_ctl_clr(CPSW_SL_CTL_EXT_EN);

While this will work for patch 1/3, once I add support for USXGMII mode
as in patch 3/3, I believe that I have to invert it, beginning by
invoking a cpsw_sl_ctl_clr(CPSW_SL_CTL_EXT_EN) at the start in
mac_config() followed by switching through the modes. If the mode is
SGMII, then I invoke cpsw_sl_ctl_set(CPSW_SL_CTL_EXT_EN), along with the
write to the MR_ADV_ABILITY_REG register to advertise SGMII. If the mode
is USXGMII, then I invoke:
cpsw_sl_ctl_set(CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN)

> 
> 
> in mac_link_down():
> 
> 	u32 ctl;
> 
> 	ctl = CPSW_SL_CTL_GMII_EN | CPSW_SL_CTL_GIG |
> 	      CPSW_SL_CTL_IFCTL_A | CPSW_SL_CTL_FULLDUPLEX |
> 	      CPSW_SL_CTL_RX_FLOW_EN | CPSW_SL_CTL_TX_FLOW_EN;
> 	if (phy_interface_mode_is_rgmii(interface))
> 		ctl |= CPSW_SL_CTL_EXT_EN;
> 
> 	cpsw_sl_ctl_clr(ctl);
> 
> This ensures that we don't touch any bits in mac_link_down() which we
> aren't modifying in the corresponding mac_link_up() implementation.

This is very helpful. Thank you for the suggestion :)

> 
> Q: do we really need to clear the mac control register on link down?
> If we don't, then we can do better, but we need an additional helper
> which allows read-modify-write of the mac control register using a
> mask value and a value of bits to set. Then we can have mac_link_up()
> setting and clearing the bits as necessary - but I would still keep
> the clearing of CPSW_SL_CTL_EXT_EN for RGMII modes in mac_link_down()
> for now.

Clearing the entire register is not necessary. As long as those bits
that were set during mac_link_up() are cleared, it should be sufficient.

For now, I will implement the changes assuming that the mac control
register will be cleared on link down, based on the above suggestions
provided by you.

Regards,
Siddharth.

Re: [PATCH net-next v2 1/3] net: ethernet: ti: am65-cpsw: Move mode specific config to mac_config()

[Russell King (Oracle)]

On Mon, Apr 03, 2023 at 07:20:21PM +0530, Siddharth Vadapalli wrote:
> 
> 
> On 03-04-2023 18:43, Russell King (Oracle) wrote:
> > On Mon, Apr 03, 2023 at 06:31:52PM +0530, Siddharth Vadapalli wrote:
> >>
> >>
> >> On 03-04-2023 16:38, Russell King (Oracle) wrote:
> >>> 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.
> >>
> >> I plan to implement it as follows:
> >> 1. Add a member "u32 mode_config" to "struct am65_cpsw_slave_data" in
> >> "am65-cpsw-nuss.h".
> >> 2. In am65_cpsw_nuss_mac_config(), store the value of mac_control in
> >> "port->slave.mode_config".
> >> 3. In am65_cpsw_nuss_mac_link_down(), after the reset via
> >> cpsw_sl_ctl_reset(), execute:
> >> cpsw_sl_ctl_set(port->slave.mac_sl, port->slave.mode_config) in order to
> >> restore the configuration performed in am65_cpsw_nuss_mac_config().
> >>
> >> Please let me know in case of any suggestions to implement it in a
> >> better manner.
> > 
> > Do you think this complexity is really worth it?
> > 
> > Let's look at what's available:
> > 
> > cpsw_sl_ctl_set() - sets bits in the mac control register
> > cpsw_sl_ctl_clr() - clears bits in the mac control register
> > cpsw_sl_ctl_reset() - sets the mac control register to zero
> > 
> > So, in mac_config(), we can do:
> > 
> > 	if (interface == SGMII)
> > 		cpsw_sl_ctl_set(CPSW_SL_CTL_EXT_EN);
> > 	else
> > 		cpsw_sl_ctl_clr(CPSW_SL_CTL_EXT_EN);
> 
> While this will work for patch 1/3, once I add support for USXGMII mode
> as in patch 3/3, I believe that I have to invert it, beginning by
> invoking a cpsw_sl_ctl_clr(CPSW_SL_CTL_EXT_EN) at the start in
> mac_config() followed by switching through the modes. If the mode is
> SGMII, then I invoke cpsw_sl_ctl_set(CPSW_SL_CTL_EXT_EN), along with the
> write to the MR_ADV_ABILITY_REG register to advertise SGMII. If the mode
> is USXGMII, then I invoke:
> cpsw_sl_ctl_set(CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN)

For patch 1, I did leave out the write for MR_ADV_ABILITY_REG, I had
assumed you'd get the idea on that and merge the if() condition you
already had with my suggestion above (which isn't literal code!)

In patch 3, you simply need to add:

	if (interface == USXGMII)
		cpsw_sl_ctl_set(CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN);
	else
		cpsw_sl_ctl_clr(CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN);

Thanks.

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

Re: [PATCH net-next v2 1/3] net: ethernet: ti: am65-cpsw: Move mode specific config to mac_config()

[Siddharth Vadapalli]

On 03-04-2023 19:25, Russell King (Oracle) wrote:
> On Mon, Apr 03, 2023 at 07:20:21PM +0530, Siddharth Vadapalli wrote:
>>
>>
>> On 03-04-2023 18:43, Russell King (Oracle) wrote:
>>> On Mon, Apr 03, 2023 at 06:31:52PM +0530, Siddharth Vadapalli wrote:
>>>>
>>>>
>>>> On 03-04-2023 16:38, Russell King (Oracle) wrote:
>>>>> 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.
>>>>
>>>> I plan to implement it as follows:
>>>> 1. Add a member "u32 mode_config" to "struct am65_cpsw_slave_data" in
>>>> "am65-cpsw-nuss.h".
>>>> 2. In am65_cpsw_nuss_mac_config(), store the value of mac_control in
>>>> "port->slave.mode_config".
>>>> 3. In am65_cpsw_nuss_mac_link_down(), after the reset via
>>>> cpsw_sl_ctl_reset(), execute:
>>>> cpsw_sl_ctl_set(port->slave.mac_sl, port->slave.mode_config) in order to
>>>> restore the configuration performed in am65_cpsw_nuss_mac_config().
>>>>
>>>> Please let me know in case of any suggestions to implement it in a
>>>> better manner.
>>>
>>> Do you think this complexity is really worth it?
>>>
>>> Let's look at what's available:
>>>
>>> cpsw_sl_ctl_set() - sets bits in the mac control register
>>> cpsw_sl_ctl_clr() - clears bits in the mac control register
>>> cpsw_sl_ctl_reset() - sets the mac control register to zero
>>>
>>> So, in mac_config(), we can do:
>>>
>>> 	if (interface == SGMII)
>>> 		cpsw_sl_ctl_set(CPSW_SL_CTL_EXT_EN);
>>> 	else
>>> 		cpsw_sl_ctl_clr(CPSW_SL_CTL_EXT_EN);
>>
>> While this will work for patch 1/3, once I add support for USXGMII mode
>> as in patch 3/3, I believe that I have to invert it, beginning by
>> invoking a cpsw_sl_ctl_clr(CPSW_SL_CTL_EXT_EN) at the start in
>> mac_config() followed by switching through the modes. If the mode is
>> SGMII, then I invoke cpsw_sl_ctl_set(CPSW_SL_CTL_EXT_EN), along with the
>> write to the MR_ADV_ABILITY_REG register to advertise SGMII. If the mode
>> is USXGMII, then I invoke:
>> cpsw_sl_ctl_set(CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN)
> 
> For patch 1, I did leave out the write for MR_ADV_ABILITY_REG, I had
> assumed you'd get the idea on that and merge the if() condition you
> already had with my suggestion above (which isn't literal code!)
> 
> In patch 3, you simply need to add:
> 
> 	if (interface == USXGMII)
> 		cpsw_sl_ctl_set(CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN);
> 	else
> 		cpsw_sl_ctl_clr(CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN);

Thank you for answering all my questions and sharing your valuable
feedback. I will implement all the suggestions in the v3 series.

Regards,
Siddharth.

[PATCH net-next v2 2/3] net: ethernet: ti: am65-cpsw: Enable QSGMII for J784S4 CPSW9G

[Siddharth Vadapalli]

TI's J784S4 SoC supports QSGMII mode with the CPSW9G instance of the
CPSW Ethernet Switch. Add a new compatible for J784S4 SoC and enable
QSGMII support for it by adding QSGMII mode to the extra_modes member of
the "j784s4_cpswxg_pdata" SoC data.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 74e099828978..6c118a9abb2f 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -2800,12 +2800,20 @@ static const struct am65_cpsw_pdata j721e_cpswxg_pdata = {
 	.extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII) | BIT(PHY_INTERFACE_MODE_SGMII),
 };
 
+static const struct am65_cpsw_pdata j784s4_cpswxg_pdata = {
+	.quirks = 0,
+	.ale_dev_id = "am64-cpswxg",
+	.fdqring_mode = K3_RINGACC_RING_MODE_MESSAGE,
+	.extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII),
+};
+
 static const struct of_device_id am65_cpsw_nuss_of_mtable[] = {
 	{ .compatible = "ti,am654-cpsw-nuss", .data = &am65x_sr1_0},
 	{ .compatible = "ti,j721e-cpsw-nuss", .data = &j721e_pdata},
 	{ .compatible = "ti,am642-cpsw-nuss", .data = &am64x_cpswxg_pdata},
 	{ .compatible = "ti,j7200-cpswxg-nuss", .data = &j7200_cpswxg_pdata},
 	{ .compatible = "ti,j721e-cpswxg-nuss", .data = &j721e_cpswxg_pdata},
+	{ .compatible = "ti,j784s4-cpswxg-nuss", .data = &j784s4_cpswxg_pdata},
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, am65_cpsw_nuss_of_mtable);
-- 
2.25.1

[PATCH net-next v2 3/3] net: ethernet: ti: am65-cpsw: Enable USXGMII mode for J784S4 CPSW9G

[Siddharth Vadapalli]

TI's J784S4 SoC supports USXGMII mode. Add USXGMII mode to the
extra_modes member of the J784S4 SoC data.

Additionally, convert the IF statement in am65_cpsw_nuss_mac_config() to
SWITCH statement to scale for new modes. Configure MAC control register
for supporting USXGMII mode and add MAC_5000FD in the "mac_capabilities"
member of struct "phylink_config".

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 6c118a9abb2f..f4d4f987563c 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -1507,10 +1507,20 @@ static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned in
 	u32 mac_control = 0;
 
 	if (common->pdata.extra_modes & BIT(state->interface)) {
-		if (state->interface == PHY_INTERFACE_MODE_SGMII) {
+		switch (state->interface) {
+		case PHY_INTERFACE_MODE_SGMII:
 			mac_control |= CPSW_SL_CTL_EXT_EN;
 			writel(ADVERTISE_SGMII,
 			       port->sgmii_base + AM65_CPSW_SGMII_MR_ADV_ABILITY_REG);
+			break;
+
+		case PHY_INTERFACE_MODE_USXGMII:
+			mac_control |= CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN;
+			break;
+
+		default:
+			/* No special configuration is required for other modes */
+			break;
 		}
 
 		if (mac_control)
@@ -2161,7 +2171,8 @@ am65_cpsw_nuss_init_port_ndev(struct am65_cpsw_common *common, u32 port_idx)
 	/* Configuring Phylink */
 	port->slave.phylink_config.dev = &port->ndev->dev;
 	port->slave.phylink_config.type = PHYLINK_NETDEV;
-	port->slave.phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_10 | MAC_100 | MAC_1000FD;
+	port->slave.phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_10 | MAC_100 |
+						      MAC_1000FD | MAC_5000FD;
 	port->slave.phylink_config.mac_managed_pm = true; /* MAC does PM */
 
 	switch (port->slave.phy_if) {
@@ -2179,6 +2190,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);
@@ -2804,7 +2816,7 @@ static const struct am65_cpsw_pdata j784s4_cpswxg_pdata = {
 	.quirks = 0,
 	.ale_dev_id = "am64-cpswxg",
 	.fdqring_mode = K3_RINGACC_RING_MODE_MESSAGE,
-	.extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII),
+	.extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII) | BIT(PHY_INTERFACE_MODE_USXGMII),
 };
 
 static const struct of_device_id am65_cpsw_nuss_of_mtable[] = {
-- 
2.25.1

Re: [PATCH net-next v2 3/3] net: ethernet: ti: am65-cpsw: Enable USXGMII mode for J784S4 CPSW9G

[Russell King (Oracle)]

On Mon, Apr 03, 2023 at 04:31:06PM +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, convert the IF statement in am65_cpsw_nuss_mac_config() to
> SWITCH statement to scale for new modes. Configure MAC control register
> for supporting USXGMII mode and add MAC_5000FD in the "mac_capabilities"
> member of struct "phylink_config".
> 
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---
>  drivers/net/ethernet/ti/am65-cpsw-nuss.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> index 6c118a9abb2f..f4d4f987563c 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> @@ -1507,10 +1507,20 @@ static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned in
>  	u32 mac_control = 0;
>  
>  	if (common->pdata.extra_modes & BIT(state->interface)) {
> -		if (state->interface == PHY_INTERFACE_MODE_SGMII) {
> +		switch (state->interface) {
> +		case PHY_INTERFACE_MODE_SGMII:
>  			mac_control |= CPSW_SL_CTL_EXT_EN;
>  			writel(ADVERTISE_SGMII,
>  			       port->sgmii_base + AM65_CPSW_SGMII_MR_ADV_ABILITY_REG);
> +			break;
> +
> +		case PHY_INTERFACE_MODE_USXGMII:
> +			mac_control |= CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN;

Following on to my comments on patch 1, with the addition of these
control bits, you now will want am65_cpsw_nuss_mac_link_down() to
avoid clearing these bits as well.

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

Re: [PATCH net-next v2 3/3] net: ethernet: ti: am65-cpsw: Enable USXGMII mode for J784S4 CPSW9G

[Siddharth Vadapalli]

On 03/04/23 16:40, Russell King (Oracle) wrote:
> On Mon, Apr 03, 2023 at 04:31:06PM +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, convert the IF statement in am65_cpsw_nuss_mac_config() to
>> SWITCH statement to scale for new modes. Configure MAC control register
>> for supporting USXGMII mode and add MAC_5000FD in the "mac_capabilities"
>> member of struct "phylink_config".
>>
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>> ---
>>  drivers/net/ethernet/ti/am65-cpsw-nuss.c | 18 +++++++++++++++---
>>  1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> index 6c118a9abb2f..f4d4f987563c 100644
>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> @@ -1507,10 +1507,20 @@ static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned in
>>  	u32 mac_control = 0;
>>  
>>  	if (common->pdata.extra_modes & BIT(state->interface)) {
>> -		if (state->interface == PHY_INTERFACE_MODE_SGMII) {
>> +		switch (state->interface) {
>> +		case PHY_INTERFACE_MODE_SGMII:
>>  			mac_control |= CPSW_SL_CTL_EXT_EN;
>>  			writel(ADVERTISE_SGMII,
>>  			       port->sgmii_base + AM65_CPSW_SGMII_MR_ADV_ABILITY_REG);
>> +			break;
>> +
>> +		case PHY_INTERFACE_MODE_USXGMII:
>> +			mac_control |= CPSW_SL_CTL_XGIG | CPSW_SL_CTL_XGMII_EN;
> 
> Following on to my comments on patch 1, with the addition of these
> control bits, you now will want am65_cpsw_nuss_mac_link_down() to
> avoid clearing these bits as well.

Yes, I will ensure this in the v3 series.

Regards,
Siddharth.