[PATCH net v2] net: ti: icssg-prueth: Fix emac link speed handling

[PATCH net v2] net: ti: icssg-prueth: Fix emac link speed handling

[MD Danish Anwar]

When link settings are changed emac->speed is populated by
emac_adjust_link(). The link speed and other settings are then written into
the DRAM. However if both ports are brought down after this and brought up
again or if the operating mode is changed and a firmware reload is needed,
the DRAM is cleared by icssg_config(). As a result the link settings are
lost.

Fix this by calling emac_adjust_link() after icssg_config(). This re
populates the settings in the DRAM after a new firmware load.

Fixes: 9facce84f406 ("net: ti: icssg-prueth: Fix firmware load sequence.")
Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
---
v1 - v2: Added phydev lock before calling emac_adjust_link() as suggested
by Andrew Lunn <andrew@lunn.ch>
v1 https://lore.kernel.org/all/20250731120812.1606839-1-danishanwar@ti.com/

 drivers/net/ethernet/ti/icssg/icssg_prueth.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
index 2b973d6e2341..58aec94b7771 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
@@ -50,6 +50,8 @@
 /* CTRLMMR_ICSSG_RGMII_CTRL register bits */
 #define ICSSG_CTRL_RGMII_ID_MODE                BIT(24)
 
+static void emac_adjust_link(struct net_device *ndev);
+
 static int emac_get_tx_ts(struct prueth_emac *emac,
 			  struct emac_tx_ts_response *rsp)
 {
@@ -229,6 +231,12 @@ static int prueth_emac_common_start(struct prueth *prueth)
 		ret = icssg_config(prueth, emac, slice);
 		if (ret)
 			goto disable_class;
+
+		if (emac->ndev->phydev) {
+			mutex_lock(&emac->ndev->phydev->lock);
+			emac_adjust_link(emac->ndev);
+			mutex_unlock(&emac->ndev->phydev->lock);
+		}
 	}
 
 	ret = prueth_emac_start(prueth);

base-commit: 01051012887329ea78eaca19b1d2eac4c9f601b5
-- 
2.34.1

Re: [PATCH net v2] net: ti: icssg-prueth: Fix emac link speed handling

[Andrew Lunn]

On Fri, Aug 01, 2025 at 05:49:48PM +0530, MD Danish Anwar wrote:
> When link settings are changed emac->speed is populated by
> emac_adjust_link(). The link speed and other settings are then written into
> the DRAM. However if both ports are brought down after this and brought up
> again or if the operating mode is changed and a firmware reload is needed,
> the DRAM is cleared by icssg_config(). As a result the link settings are
> lost.
> 
> Fix this by calling emac_adjust_link() after icssg_config(). This re
> populates the settings in the DRAM after a new firmware load.
> 
> Fixes: 9facce84f406 ("net: ti: icssg-prueth: Fix firmware load sequence.")
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> ---
> v1 - v2: Added phydev lock before calling emac_adjust_link() as suggested
> by Andrew Lunn <andrew@lunn.ch>
> v1 https://lore.kernel.org/all/20250731120812.1606839-1-danishanwar@ti.com/
> 
>  drivers/net/ethernet/ti/icssg/icssg_prueth.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> index 2b973d6e2341..58aec94b7771 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> @@ -50,6 +50,8 @@
>  /* CTRLMMR_ICSSG_RGMII_CTRL register bits */
>  #define ICSSG_CTRL_RGMII_ID_MODE                BIT(24)
>  
> +static void emac_adjust_link(struct net_device *ndev);
> +
>  static int emac_get_tx_ts(struct prueth_emac *emac,
>  			  struct emac_tx_ts_response *rsp)
>  {
> @@ -229,6 +231,12 @@ static int prueth_emac_common_start(struct prueth *prueth)
>  		ret = icssg_config(prueth, emac, slice);
>  		if (ret)
>  			goto disable_class;
> +
> +		if (emac->ndev->phydev) {
> +			mutex_lock(&emac->ndev->phydev->lock);
> +			emac_adjust_link(emac->ndev);
> +			mutex_unlock(&emac->ndev->phydev->lock);
> +		}

What about the else case? The link settings are lost, and the MAC does
not work?

	Andrew

Re: [PATCH net v2] net: ti: icssg-prueth: Fix emac link speed handling

[MD Danish Anwar]

Hi Andrew,

On 04/08/25 10:19 pm, Andrew Lunn wrote:
> On Fri, Aug 01, 2025 at 05:49:48PM +0530, MD Danish Anwar wrote:
>> When link settings are changed emac->speed is populated by
>> emac_adjust_link(). The link speed and other settings are then written into
>> the DRAM. However if both ports are brought down after this and brought up
>> again or if the operating mode is changed and a firmware reload is needed,
>> the DRAM is cleared by icssg_config(). As a result the link settings are
>> lost.
>>
>> Fix this by calling emac_adjust_link() after icssg_config(). This re
>> populates the settings in the DRAM after a new firmware load.
>>
>> Fixes: 9facce84f406 ("net: ti: icssg-prueth: Fix firmware load sequence.")
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>> ---
>> v1 - v2: Added phydev lock before calling emac_adjust_link() as suggested
>> by Andrew Lunn <andrew@lunn.ch>
>> v1 https://lore.kernel.org/all/20250731120812.1606839-1-danishanwar@ti.com/
>>
>>  drivers/net/ethernet/ti/icssg/icssg_prueth.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> index 2b973d6e2341..58aec94b7771 100644
>> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
>> @@ -50,6 +50,8 @@
>>  /* CTRLMMR_ICSSG_RGMII_CTRL register bits */
>>  #define ICSSG_CTRL_RGMII_ID_MODE                BIT(24)
>>  
>> +static void emac_adjust_link(struct net_device *ndev);
>> +
>>  static int emac_get_tx_ts(struct prueth_emac *emac,
>>  			  struct emac_tx_ts_response *rsp)
>>  {
>> @@ -229,6 +231,12 @@ static int prueth_emac_common_start(struct prueth *prueth)
>>  		ret = icssg_config(prueth, emac, slice);
>>  		if (ret)
>>  			goto disable_class;
>> +
>> +		if (emac->ndev->phydev) {
>> +			mutex_lock(&emac->ndev->phydev->lock);
>> +			emac_adjust_link(emac->ndev);
>> +			mutex_unlock(&emac->ndev->phydev->lock);
>> +		}
> 
> What about the else case? The link settings are lost, and the MAC does
> not work?
> 

Actually this if else is not needed at all. If phydev == NULL, the
driver would have already returned -ENODEV in emac_phy_connect() and the
device's probe would have failed.

If we have reached this point that means phydev is not null and there is
no need for this check.

I will drop this if check and send a v3.

> 	Andrew

-- 
Thanks and Regards,
Danish