[PATCH net v2 0/1] net: ravb: fixes for the ravb driver

[PATCH net v2 0/1] net: ravb: fixes for the ravb driver

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Hi,

Series adds one fix for the ravb driver to wait for the operating
mode to be applied by hardware before proceeding.

Thank you,
Claudiu Beznea

Changes in v2:
- dropped patch 2/2 from v1 ("net: ravb: Check that GTI loading request is
  done")
- kept a single "Fixes" entry in commit description
- updated commit description for patch 1/1
- introduce ravb_set_opmode() that does all the necessities for
  setting the operating mode (set DMA.CCC and wait for CSR.OPS) and call it
  where needed; this should comply with all the HW manuals requirements as
  different manual variants specify different modes need to be checked in
  CSR.OPS when setting DMA.CCC.
 
Claudiu Beznea (1):
  net: ravb: Wait for operation mode to be applied

 drivers/net/ethernet/renesas/ravb_main.c | 52 ++++++++++++++----------
 1 file changed, 31 insertions(+), 21 deletions(-)

-- 
2.39.2

[PATCH net v2 1/1] net: ravb: Wait for operation mode to be applied

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

CSR.OPS bits specify the current operating mode and (according to
documentation) they are updated by HW when the operating mode change
request is processed. To comply with this check CSR.OPS before proceeding.

Commit introduces ravb_set_opmode() that does all the necessities for
setting the operating mode (set DMA.CCC and wait for CSR.OPS) and call it
where needed. This should comply with all the HW manuals requirements as
different manual variants specify that different modes need to be checked
in CSR.OPS when setting DMA.CCC.

Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 52 ++++++++++++++----------
 1 file changed, 31 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 664eda4b5a11..ae99d035a3b6 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -66,14 +66,15 @@ int ravb_wait(struct net_device *ndev, enum ravb_reg reg, u32 mask, u32 value)
 	return -ETIMEDOUT;
 }
 
-static int ravb_config(struct net_device *ndev)
+static int ravb_set_opmode(struct net_device *ndev, u32 opmode)
 {
+	u32 csr_opmode = 1UL << opmode;
 	int error;
 
-	/* Set config mode */
-	ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG);
-	/* Check if the operating mode is changed to the config mode */
-	error = ravb_wait(ndev, CSR, CSR_OPS, CSR_OPS_CONFIG);
+	/* Set operating mode */
+	ravb_modify(ndev, CCC, CCC_OPC, opmode);
+	/* Check if the operating mode is changed to the requested one */
+	error = ravb_wait(ndev, CSR, CSR_OPS, csr_opmode);
 	if (error)
 		netdev_err(ndev, "failed to switch device to config mode\n");
 
@@ -673,7 +674,7 @@ static int ravb_dmac_init(struct net_device *ndev)
 	int error;
 
 	/* Set CONFIG mode */
-	error = ravb_config(ndev);
+	error = ravb_set_opmode(ndev, CCC_OPC_CONFIG);
 	if (error)
 		return error;
 
@@ -682,9 +683,7 @@ static int ravb_dmac_init(struct net_device *ndev)
 		return error;
 
 	/* Setting the control will start the AVB-DMAC process. */
-	ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_OPERATION);
-
-	return 0;
+	return ravb_set_opmode(ndev, CCC_OPC_OPERATION);
 }
 
 static void ravb_get_tx_tstamp(struct net_device *ndev)
@@ -1046,7 +1045,7 @@ static int ravb_stop_dma(struct net_device *ndev)
 		return error;
 
 	/* Stop AVB-DMAC process */
-	return ravb_config(ndev);
+	return ravb_set_opmode(ndev, CCC_OPC_CONFIG);
 }
 
 /* E-MAC interrupt handler */
@@ -2560,21 +2559,23 @@ static int ravb_set_gti(struct net_device *ndev)
 	return 0;
 }
 
-static void ravb_set_config_mode(struct net_device *ndev)
+static int ravb_set_config_mode(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 	const struct ravb_hw_info *info = priv->info;
+	int error;
 
 	if (info->gptp) {
-		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG);
+		error = ravb_set_opmode(ndev, CCC_OPC_CONFIG);
 		/* Set CSEL value */
 		ravb_modify(ndev, CCC, CCC_CSEL, CCC_CSEL_HPB);
 	} else if (info->ccc_gac) {
-		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG |
-			    CCC_GAC | CCC_CSEL_HPB);
+		error = ravb_set_opmode(ndev, CCC_OPC_CONFIG | CCC_GAC | CCC_CSEL_HPB);
 	} else {
-		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG);
+		error = ravb_set_opmode(ndev, CCC_OPC_CONFIG);
 	}
+
+	return error;
 }
 
 /* Set tx and rx clock internal delay modes */
@@ -2794,7 +2795,9 @@ static int ravb_probe(struct platform_device *pdev)
 	ndev->ethtool_ops = &ravb_ethtool_ops;
 
 	/* Set AVB config mode */
-	ravb_set_config_mode(ndev);
+	error = ravb_set_config_mode(ndev);
+	if (error)
+		goto out_disable_gptp_clk;
 
 	if (info->gptp || info->ccc_gac) {
 		/* Set GTI value */
@@ -2902,6 +2905,7 @@ static void ravb_remove(struct platform_device *pdev)
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct ravb_private *priv = netdev_priv(ndev);
 	const struct ravb_hw_info *info = priv->info;
+	int error;
 
 	unregister_netdev(ndev);
 	if (info->nc_queues)
@@ -2917,8 +2921,9 @@ static void ravb_remove(struct platform_device *pdev)
 	dma_free_coherent(ndev->dev.parent, priv->desc_bat_size, priv->desc_bat,
 			  priv->desc_bat_dma);
 
-	/* Set reset mode */
-	ravb_write(ndev, CCC_OPC_RESET, CCC);
+	error = ravb_set_opmode(ndev, CCC_OPC_RESET);
+	if (error)
+		netdev_err(ndev, "Failed to reset ndev\n");
 
 	clk_disable_unprepare(priv->gptp_clk);
 	clk_disable_unprepare(priv->refclk);
@@ -3000,8 +3005,11 @@ static int __maybe_unused ravb_resume(struct device *dev)
 	int ret = 0;
 
 	/* If WoL is enabled set reset mode to rearm the WoL logic */
-	if (priv->wol_enabled)
-		ravb_write(ndev, CCC_OPC_RESET, CCC);
+	if (priv->wol_enabled) {
+		ret = ravb_set_opmode(ndev, CCC_OPC_RESET);
+		if (ret)
+			return ret;
+	}
 
 	/* All register have been reset to default values.
 	 * Restore all registers which where setup at probe time and
@@ -3009,7 +3017,9 @@ static int __maybe_unused ravb_resume(struct device *dev)
 	 */
 
 	/* Set AVB config mode */
-	ravb_set_config_mode(ndev);
+	ret = ravb_set_config_mode(ndev);
+	if (ret)
+		return ret;
 
 	if (info->gptp || info->ccc_gac) {
 		/* Set GTI value */
-- 
2.39.2

Re: [PATCH net v2 1/1] net: ravb: Wait for operation mode to be applied

[Sergey Shtylyov]

On 12/22/23 2:35 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> CSR.OPS bits specify the current operating mode and (according to
> documentation) they are updated by HW when the operating mode change
> request is processed. To comply with this check CSR.OPS before proceeding.
> 
> Commit introduces ravb_set_opmode() that does all the necessities for
> setting the operating mode (set DMA.CCC and wait for CSR.OPS) and call it
> where needed. This should comply with all the HW manuals requirements as
> different manual variants specify that different modes need to be checked
> in CSR.OPS when setting DMA.CCC.
> 
> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 52 ++++++++++++++----------
>  1 file changed, 31 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 664eda4b5a11..ae99d035a3b6 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -66,14 +66,15 @@ int ravb_wait(struct net_device *ndev, enum ravb_reg reg, u32 mask, u32 value)
>  	return -ETIMEDOUT;
>  }
>  
> -static int ravb_config(struct net_device *ndev)
> +static int ravb_set_opmode(struct net_device *ndev, u32 opmode)

   Since you pass the complete CCC register value below, you should
rather call the function ravb_set_ccc() and call the parameter opmode
ccc.

>  {
> +	u32 csr_opmode = 1UL << opmode;

   Please use the correct expression, 1U << (ccc & CCC_OPC) instead.
And I'd suggest calling the variable csr_ops or just ops.

>  	int error;
>  
> -	/* Set config mode */
> -	ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG);
> -	/* Check if the operating mode is changed to the config mode */
> -	error = ravb_wait(ndev, CSR, CSR_OPS, CSR_OPS_CONFIG);
> +	/* Set operating mode */
> +	ravb_modify(ndev, CCC, CCC_OPC, opmode);
> +	/* Check if the operating mode is changed to the requested one */
> +	error = ravb_wait(ndev, CSR, CSR_OPS, csr_opmode);
>  	if (error)
>  		netdev_err(ndev, "failed to switch device to config mode\n");

   s/config/requested/? Or just print out that mode...

[...]
> @@ -2560,21 +2559,23 @@ static int ravb_set_gti(struct net_device *ndev)
>  	return 0;
>  }
>  
> -static void ravb_set_config_mode(struct net_device *ndev)
> +static int ravb_set_config_mode(struct net_device *ndev)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
>  	const struct ravb_hw_info *info = priv->info;
> +	int error;
>  
>  	if (info->gptp) {
> -		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG);
> +		error = ravb_set_opmode(ndev, CCC_OPC_CONFIG);

   Don't we need to return on error here?

>  		/* Set CSEL value */
>  		ravb_modify(ndev, CCC, CCC_CSEL, CCC_CSEL_HPB);
>  	} else if (info->ccc_gac) {
> -		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG |
> -			    CCC_GAC | CCC_CSEL_HPB);
> +		error = ravb_set_opmode(ndev, CCC_OPC_CONFIG | CCC_GAC | CCC_CSEL_HPB);

   See, you pass more than just CCC.OPC value here -- need to mask it out
above...

[...]
> @@ -2917,8 +2921,9 @@ static void ravb_remove(struct platform_device *pdev)
>  	dma_free_coherent(ndev->dev.parent, priv->desc_bat_size, priv->desc_bat,
>  			  priv->desc_bat_dma);
>  
> -	/* Set reset mode */
> -	ravb_write(ndev, CCC_OPC_RESET, CCC);
> +	error = ravb_set_opmode(ndev, CCC_OPC_RESET);
> +	if (error)
> +		netdev_err(ndev, "Failed to reset ndev\n");

   ravb_set_opmode() will have complained already at this point...

[...]

MBR, Sergey

Re: [PATCH net v2 1/1] net: ravb: Wait for operation mode to be applied

[Sergey Shtylyov]

On 12/22/23 2:35 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> CSR.OPS bits specify the current operating mode and (according to
> documentation) they are updated by HW when the operating mode change
> request is processed. To comply with this check CSR.OPS before proceeding.
> 
> Commit introduces ravb_set_opmode() that does all the necessities for
> setting the operating mode (set DMA.CCC and wait for CSR.OPS) and call it

   What's DMA.CCC? Maybe CCC.OPC?

> where needed. This should comply with all the HW manuals requirements as
> different manual variants specify that different modes need to be checked
> in CSR.OPS when setting DMA.CCC.
> 
> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
[...]

MBR, Sergey

Re: [PATCH net v2 1/1] net: ravb: Wait for operation mode to be applied

[claudiu beznea]

On 23.12.2023 21:39, Sergey Shtylyov wrote:
> On 12/22/23 2:35 PM, Claudiu wrote:
> 
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> CSR.OPS bits specify the current operating mode and (according to
>> documentation) they are updated by HW when the operating mode change
>> request is processed. To comply with this check CSR.OPS before proceeding.
>>
>> Commit introduces ravb_set_opmode() that does all the necessities for
>> setting the operating mode (set DMA.CCC and wait for CSR.OPS) and call it
>> where needed. This should comply with all the HW manuals requirements as
>> different manual variants specify that different modes need to be checked
>> in CSR.OPS when setting DMA.CCC.
>>
>> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>  drivers/net/ethernet/renesas/ravb_main.c | 52 ++++++++++++++----------
>>  1 file changed, 31 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index 664eda4b5a11..ae99d035a3b6 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -66,14 +66,15 @@ int ravb_wait(struct net_device *ndev, enum ravb_reg reg, u32 mask, u32 value)
>>  	return -ETIMEDOUT;
>>  }
>>  
>> -static int ravb_config(struct net_device *ndev)
>> +static int ravb_set_opmode(struct net_device *ndev, u32 opmode)
> 
>    Since you pass the complete CCC register value below, you should
> rather call the function ravb_set_ccc() and call the parameter opmode
> ccc.

This will be confusing. E.g., if renaming it ravb_set_ccc() one would
expect to set any fields of CCC though this function but this is not true
as ravb_modify() in this function masks only CCC_OPC. The call of:

error = ravb_set_opmode(ndev, CCC_OPC_CONFIG | CCC_GAC | CCC_CSEL_HPB);

bellow is just to comply with datasheet requirements, previous code and at
the same time re-use this function.

> 
>>  {
>> +	u32 csr_opmode = 1UL << opmode;
> 
>    Please use the correct expression, 1U << (ccc & CCC_OPC) instead.

Ok, good point.


> And I'd suggest calling the variable csr_ops or just ops.

ok

> 
>>  	int error;
>>  
>> -	/* Set config mode */
>> -	ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG);
>> -	/* Check if the operating mode is changed to the config mode */
>> -	error = ravb_wait(ndev, CSR, CSR_OPS, CSR_OPS_CONFIG);
>> +	/* Set operating mode */
>> +	ravb_modify(ndev, CCC, CCC_OPC, opmode);
>> +	/* Check if the operating mode is changed to the requested one */
>> +	error = ravb_wait(ndev, CSR, CSR_OPS, csr_opmode);
>>  	if (error)
>>  		netdev_err(ndev, "failed to switch device to config mode\n");
> 
>    s/config/requested/? Or just print out that mode...
> 
> [...]
>> @@ -2560,21 +2559,23 @@ static int ravb_set_gti(struct net_device *ndev)
>>  	return 0;
>>  }
>>  
>> -static void ravb_set_config_mode(struct net_device *ndev)
>> +static int ravb_set_config_mode(struct net_device *ndev)
>>  {
>>  	struct ravb_private *priv = netdev_priv(ndev);
>>  	const struct ravb_hw_info *info = priv->info;
>> +	int error;
>>  
>>  	if (info->gptp) {
>> -		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG);
>> +		error = ravb_set_opmode(ndev, CCC_OPC_CONFIG);
> 
>    Don't we need to return on error here?

I kept it like this to have a single exit point from function. But probably
setting CSEL when OPC setup failed may lead to failures. I'll adjust it,
thanks.

> 
>>  		/* Set CSEL value */
>>  		ravb_modify(ndev, CCC, CCC_CSEL, CCC_CSEL_HPB);
>>  	} else if (info->ccc_gac) {
>> -		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG |
>> -			    CCC_GAC | CCC_CSEL_HPB);
>> +		error = ravb_set_opmode(ndev, CCC_OPC_CONFIG | CCC_GAC | CCC_CSEL_HPB);
> 
>    See, you pass more than just CCC.OPC value here -- need to mask it out
> above...

Agree.

> 
> [...]
>> @@ -2917,8 +2921,9 @@ static void ravb_remove(struct platform_device *pdev)
>>  	dma_free_coherent(ndev->dev.parent, priv->desc_bat_size, priv->desc_bat,
>>  			  priv->desc_bat_dma);
>>  
>> -	/* Set reset mode */
>> -	ravb_write(ndev, CCC_OPC_RESET, CCC);
>> +	error = ravb_set_opmode(ndev, CCC_OPC_RESET);
>> +	if (error)
>> +		netdev_err(ndev, "Failed to reset ndev\n");
> 
>    ravb_set_opmode() will have complained already at this point...
> 
> [...]
> 
> MBR, Sergey

Re: [PATCH net v2 1/1] net: ravb: Wait for operation mode to be applied

[claudiu beznea]

On 27.12.2023 12:10, claudiu beznea wrote:
>>> -static int ravb_config(struct net_device *ndev)
>>> +static int ravb_set_opmode(struct net_device *ndev, u32 opmode)
>>    Since you pass the complete CCC register value below, you should
>> rather call the function ravb_set_ccc() and call the parameter opmode
>> ccc.
> This will be confusing. E.g., if renaming it ravb_set_ccc() one would
> expect to set any fields of CCC though this function but this is not true
> as ravb_modify() in this function masks only CCC_OPC. The call of:

What about ravb_set_opc() or ravb_set_ccc_opc() ?

Re: [PATCH net v2 1/1] net: ravb: Wait for operation mode to be applied

[Sergey Shtylyov]

On 12/27/23 1:10 PM, claudiu beznea wrote:

[...]

>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>
>>> CSR.OPS bits specify the current operating mode and (according to
>>> documentation) they are updated by HW when the operating mode change
>>> request is processed. To comply with this check CSR.OPS before proceeding.
>>>
>>> Commit introduces ravb_set_opmode() that does all the necessities for
>>> setting the operating mode (set DMA.CCC and wait for CSR.OPS) and call it
>>> where needed. This should comply with all the HW manuals requirements as
>>> different manual variants specify that different modes need to be checked
>>> in CSR.OPS when setting DMA.CCC.
>>>
>>> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>> ---
>>>  drivers/net/ethernet/renesas/ravb_main.c | 52 ++++++++++++++----------
>>>  1 file changed, 31 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>> index 664eda4b5a11..ae99d035a3b6 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> @@ -66,14 +66,15 @@ int ravb_wait(struct net_device *ndev, enum ravb_reg reg, u32 mask, u32 value)
>>>  	return -ETIMEDOUT;
>>>  }
>>>  
>>> -static int ravb_config(struct net_device *ndev)
>>> +static int ravb_set_opmode(struct net_device *ndev, u32 opmode)
>>
>>    Since you pass the complete CCC register value below, you should
>> rather call the function ravb_set_ccc() and call the parameter opmode
>> ccc.
> 
> This will be confusing. E.g., if renaming it ravb_set_ccc() one would
> expect to set any fields of CCC though this function but this is not true
> as ravb_modify() in this function masks only CCC_OPC. The call of:
> 
> error = ravb_set_opmode(ndev, CCC_OPC_CONFIG | CCC_GAC | CCC_CSEL_HPB);
> 
> bellow is just to comply with datasheet requirements, previous code and at
> the same time re-use this function.

   How about the following then (ugly... but does the job):

	/* Set operating mode */
	if (opmode & ~CCC_OPC)
		ravb_write(ndev, opmode, CCC);
	else
		ravb_modify(ndev, CCC, CCC_OPC, opmode);

   Either that or just don't use ravb_set_opmode() when writing the whole
32-bit value below...

[...]

>>> @@ -2560,21 +2559,23 @@ static int ravb_set_gti(struct net_device *ndev)
[...]
>>
>>>  		/* Set CSEL value */
>>>  		ravb_modify(ndev, CCC, CCC_CSEL, CCC_CSEL_HPB);
>>>  	} else if (info->ccc_gac) {
>>> -		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG |
>>> -			    CCC_GAC | CCC_CSEL_HPB);
>>> +		error = ravb_set_opmode(ndev, CCC_OPC_CONFIG | CCC_GAC | CCC_CSEL_HPB);

   ... like this?

		ravb_write(ndev, CCC_OPC_CONFIG | CCC_GAC | CCC_CSEL_HPB, CCC);
		error = ravb_wait(ndev, CSR, CSR_OPS, CSR_OPS_CONFIG);

[...]

MBR, Sergey

Re: [PATCH net v2 1/1] net: ravb: Wait for operation mode to be applied

[claudiu beznea]

On 28.12.2023 21:07, Sergey Shtylyov wrote:
> On 12/27/23 1:10 PM, claudiu beznea wrote:
> 
> [...]
> 
>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>
>>>> CSR.OPS bits specify the current operating mode and (according to
>>>> documentation) they are updated by HW when the operating mode change
>>>> request is processed. To comply with this check CSR.OPS before proceeding.
>>>>
>>>> Commit introduces ravb_set_opmode() that does all the necessities for
>>>> setting the operating mode (set DMA.CCC and wait for CSR.OPS) and call it
>>>> where needed. This should comply with all the HW manuals requirements as
>>>> different manual variants specify that different modes need to be checked
>>>> in CSR.OPS when setting DMA.CCC.
>>>>
>>>> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>> ---
>>>>  drivers/net/ethernet/renesas/ravb_main.c | 52 ++++++++++++++----------
>>>>  1 file changed, 31 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>>> index 664eda4b5a11..ae99d035a3b6 100644
>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>>> @@ -66,14 +66,15 @@ int ravb_wait(struct net_device *ndev, enum ravb_reg reg, u32 mask, u32 value)
>>>>  	return -ETIMEDOUT;
>>>>  }
>>>>  
>>>> -static int ravb_config(struct net_device *ndev)
>>>> +static int ravb_set_opmode(struct net_device *ndev, u32 opmode)
>>>
>>>    Since you pass the complete CCC register value below, you should
>>> rather call the function ravb_set_ccc() and call the parameter opmode
>>> ccc.
>>
>> This will be confusing. E.g., if renaming it ravb_set_ccc() one would
>> expect to set any fields of CCC though this function but this is not true
>> as ravb_modify() in this function masks only CCC_OPC. The call of:
>>
>> error = ravb_set_opmode(ndev, CCC_OPC_CONFIG | CCC_GAC | CCC_CSEL_HPB);
>>
>> bellow is just to comply with datasheet requirements, previous code and at
>> the same time re-use this function.
> 
>    How about the following then (ugly... but does the job):
> 
> 	/* Set operating mode */
> 	if (opmode & ~CCC_OPC)
> 		ravb_write(ndev, opmode, CCC);
> 	else
> 		ravb_modify(ndev, CCC, CCC_OPC, opmode);
> 
>    Either that or just don't use ravb_set_opmode() when writing the whole
> 32-bit value below...

This looks uglier to me...

We have this discussion because of ccc_gac. For ccc_gac platforms we need
to set OPC, GAC, CSEL at the same time. This is how we can change the
operating mode to configuration mode in case we also need to configure GAC
(due to restrictions imposed by hardware).

What I want to say is that setting GAC and CSEL along with CCC is part of
changing the operating mode to configuration mode for platforms supporting
GAC because of hardware limitations.

> 
> [...]
> 
>>>> @@ -2560,21 +2559,23 @@ static int ravb_set_gti(struct net_device *ndev)
> [...]
>>>
>>>>  		/* Set CSEL value */
>>>>  		ravb_modify(ndev, CCC, CCC_CSEL, CCC_CSEL_HPB);
>>>>  	} else if (info->ccc_gac) {
>>>> -		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG |
>>>> -			    CCC_GAC | CCC_CSEL_HPB);
>>>> +		error = ravb_set_opmode(ndev, CCC_OPC_CONFIG | CCC_GAC | CCC_CSEL_HPB);
> 
>    ... like this?
> 
> 		ravb_write(ndev, CCC_OPC_CONFIG | CCC_GAC | CCC_CSEL_HPB, CCC);
> 		error = ravb_wait(ndev, CSR, CSR_OPS, CSR_OPS_CONFIG);
> 
> [...]
> 
> MBR, Sergey

Re: [PATCH net v2 1/1] net: ravb: Wait for operation mode to be applied

[Sergey Shtylyov]

On 12/29/23 6:07 PM, claudiu beznea wrote:

[...]

>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>
>>>>> CSR.OPS bits specify the current operating mode and (according to
>>>>> documentation) they are updated by HW when the operating mode change
>>>>> request is processed. To comply with this check CSR.OPS before proceeding.
>>>>>
>>>>> Commit introduces ravb_set_opmode() that does all the necessities for
>>>>> setting the operating mode (set DMA.CCC and wait for CSR.OPS) and call it
>>>>> where needed. This should comply with all the HW manuals requirements as
>>>>> different manual variants specify that different modes need to be checked
>>>>> in CSR.OPS when setting DMA.CCC.
>>>>>
>>>>> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>> ---
>>>>>  drivers/net/ethernet/renesas/ravb_main.c | 52 ++++++++++++++----------
>>>>>  1 file changed, 31 insertions(+), 21 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>>>> index 664eda4b5a11..ae99d035a3b6 100644
>>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>>>> @@ -66,14 +66,15 @@ int ravb_wait(struct net_device *ndev, enum ravb_reg reg, u32 mask, u32 value)
>>>>>  	return -ETIMEDOUT;
>>>>>  }
>>>>>  
>>>>> -static int ravb_config(struct net_device *ndev)
>>>>> +static int ravb_set_opmode(struct net_device *ndev, u32 opmode)
>>>>
>>>>    Since you pass the complete CCC register value below, you should
>>>> rather call the function ravb_set_ccc() and call the parameter opmode
>>>> ccc.
>>>
>>> This will be confusing. E.g., if renaming it ravb_set_ccc() one would
>>> expect to set any fields of CCC though this function but this is not true
>>> as ravb_modify() in this function masks only CCC_OPC. The call of:
>>>
>>> error = ravb_set_opmode(ndev, CCC_OPC_CONFIG | CCC_GAC | CCC_CSEL_HPB);
>>>
>>> bellow is just to comply with datasheet requirements, previous code and at
>>> the same time re-use this function.
>>
>>    How about the following then (ugly... but does the job):
>>
>> 	/* Set operating mode */
>> 	if (opmode & ~CCC_OPC)
>> 		ravb_write(ndev, opmode, CCC);
>> 	else
>> 		ravb_modify(ndev, CCC, CCC_OPC, opmode);
>>
>>    Either that or just don't use ravb_set_opmode() when writing the whole
>> 32-bit value below...
> 
> This looks uglier to me...
> 
> We have this discussion because of ccc_gac. For ccc_gac platforms we need
> to set OPC, GAC, CSEL at the same time. This is how we can change the
> operating mode to configuration mode in case we also need to configure GAC
> (due to restrictions imposed by hardware).
> 
> What I want to say is that setting GAC and CSEL along with CCC is part of
> changing the operating mode to configuration mode for platforms supporting
> GAC because of hardware limitations.

   After thinking about it once more, your description seems correct.
But then we need something like this:

static int ravb_set_opmode(struct net_device *ndev, u32 opmode)
{
	u32 ccc_mask = CCC_OPC;
	u32 csr_ops = 1U << (opmode & CCC_OPC);
 	int error;

 	if (opmode & CCC_GAC)
 		ccc_mask |= CCC_CSEL;

	/* Set operating mode */
	ravb_modify(ndev, CCC, ccc_mask, opmode);
	/* Check if the operating mode is changed to the requested one */
	error = ravb_wait(ndev, CSR, CSR_OPS, csr_ops);
 	if (error)
 		netdev_err(ndev, "failed to switch device to  requested mode\n");

	return error;
}

[...]

MBR, Sergey