diff mbox series

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

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

Commit Message

Siddharth Vadapalli April 3, 2023, 11:01 a.m. UTC 
  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(-)

Comments

Siddharth Vadapalli April 3, 2023, 11:17 a.m. UTC | #1 
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.
Siddharth Vadapalli April 3, 2023, 1:01 p.m. UTC | #2 
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.
Russell King (Oracle) April 3, 2023, 1:13 p.m. UTC | #3 
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.
Siddharth Vadapalli April 3, 2023, 1:50 p.m. UTC | #4 
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.
Russell King (Oracle) April 3, 2023, 1:55 p.m. UTC | #5 
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.
Siddharth Vadapalli April 3, 2023, 2:08 p.m. UTC | #6 
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.
diff mbox series

Patch

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;