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

[claudiu beznea]

On 14.12.2023 14:39, Niklas Söderlund wrote:
> On 2023-12-14 14:25:57 +0200, claudiu beznea wrote:
>> Hi, Niklas,
>>
>> On 14.12.2023 14:11, Niklas Söderlund wrote:
>>> Hi Claudiu,
>>>
>>> Thanks for your patch.
>>>
>>> On 2023-12-14 13:31:36 +0200, Claudiu wrote:
>>>> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>>>>
>>>> CSR.OPS bits specify the current operating mode and (according to
>>>> documentation) they are updated when the operating mode change request
>>>> is processed. Thus, check CSR.OPS before proceeding.
>>>>
>>>> Fixes: 568b3ce7a8ef ("ravb: factor out register bit twiddling code")
>>>> Fixes: 0184165b2f42 ("ravb: add sleep PM suspend/resume support")
>>>> Fixes: 7e09a052dc4e ("ravb: Exclude gPTP feature support for RZ/G2L")
>>>> Fixes: 3e3d647715d4 ("ravb: add wake-on-lan support via magic packet")
>>>> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
>>>
>>> I think the list of fixes tags can be reduced. The last item in the list 
>>> is the patch which adds the RAVB driver so what's the point of listing 
>>> the rest?
>>
>> In commit c156633f1353 ("Renesas Ethernet AVB driver proper") different
>> features that were touched by the rest of commits in the fixes list might
>> not be present. So, it might be possible that this patch to not be
>> back-portable to c156633f1353 ("Renesas Ethernet AVB driver proper") but to
>> other commits in the list.
> 
> All the other commits depends on commit c156633f1353 ("Renesas Ethernet 
> AVB driver proper"). It would be hard to add wake-on-lan to a driver 
> that do not exists :-)
> 
> I do not feel strongly about this so keep it as if if you wish, I just 
> think it looks odd.

I usually add here all the blamed commits. I can change it, I don't have
nothing against, this is how I did it till now.

> 
>>
>>>
>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>>>> ---
>>>>  drivers/net/ethernet/renesas/ravb_main.c | 47 ++++++++++++++++++++----
>>>>  1 file changed, 39 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>>> index 9178f6d60e74..ce95eb5af354 100644
>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>>> @@ -683,8 +683,11 @@ static int ravb_dmac_init(struct net_device *ndev)
>>>>  
>>>>  	/* Setting the control will start the AVB-DMAC process. */
>>>>  	ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_OPERATION);
>>>> +	error = ravb_wait(ndev, CSR, CSR_OPS, CSR_OPS_OPERATION);
>>>> +	if (error)
>>>> +		netdev_err(ndev, "failed to switch device to operation mode\n");
>>>
>>> As you add ravb_set_reset_mode() to compliment the existing 
>>> ravb_set_config_mode(), would it not be coherent to also add a 
>>> ravb_set_operation_mode() instead of open coding it here?
>>
>> CSR_OPS_OPERATION is set only in this place. Reset is done in more than one
>> place. Due to this I've added a function for it.
> 
> OK. Then maybe add a generic change mode operation like rswitch do in 
> rswitch_gwca_change_mode() ? That way you ensure any future mode changes 
> will always confirm the change is successful ? I'm a but worried that 

ravb_set_config_mode() does things depending on gPTP, CCC_GAC.
ravb_set_reset_mode() just set CCC_OPC and goes out. Doing it like this
will move logic out of ravb_set_config_mode() (AFAICT) which will lead to
even complicated code (AFAICT).

On top of that there is also ravb_config()... Maybe this one could be
merged with ravb_set_reset_mode()...

> future changes might forget to add the ravb_wait() to confirm a mode 
> change is successful and a good helper could avoid that.

Review should help with this. There are other bits in other registers that
needs polling.

> 
>>
>>>
>>>>  
>>>> -	return 0;
>>>> +	return error;
>>>>  }
>>>>  
>>>>  static void ravb_get_tx_tstamp(struct net_device *ndev)
>>>> @@ -1744,6 +1747,18 @@ static inline int ravb_hook_irq(unsigned int irq, irq_handler_t handler,
>>>>  	return error;
>>>>  }
>>>>  
>>>> +static int ravb_set_reset_mode(struct net_device *ndev)
>>>
>>> nit: Maybe move this to be close to ravb_set_config_mode() to co-locate 
>>> all mode changing logic?
>>
>> I've did this but not in this patch. It could be found on the final version
>> of the driver proposed by
>> https://lore.kernel.org/all/20231214114600.2451162-1-claudiu.beznea.uj@xxxxxxxxxxxxxx/
> 
> Why not add it in the final intended location straight away then moving 
> it around in a later patch?

I guess it was easier when formatted the patches (ones in this series and
the ones at [1]).

> This just makes the later patch harder to 
> review as it moves more code around.
> 
>>
>> Thank you for your review,
>> Claudiu Beznea
>>
>>>
>>>> +{
>>>> +	int error;
>>>> +
>>>> +	ravb_write(ndev, CCC_OPC_RESET, CCC);
>>>> +	error = ravb_wait(ndev, CSR, CSR_OPS, CSR_OPS_RESET);
>>>> +	if (error)
>>>> +		netdev_err(ndev, "failed to switch device to reset mode\n");
>>>> +
>>>> +	return error;
>>>> +}
>>>> +
>>>>  /* Network device open function for Ethernet AVB */
>>>>  static int ravb_open(struct net_device *ndev)
>>>>  {
>>>> @@ -2551,10 +2566,11 @@ 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);
>>>> @@ -2566,6 +2582,12 @@ static void ravb_set_config_mode(struct net_device *ndev)
>>>>  	} else {
>>>>  		ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG);
>>>>  	}
>>>> +
>>>> +	error = ravb_wait(ndev, CSR, CSR_OPS, CSR_OPS_CONFIG);
>>>> +	if (error)
>>>> +		netdev_err(ndev, "failed to switch device to config mode\n");
>>>> +
>>>> +	return error;
>>>>  }
>>>>  
>>>>  /* Set tx and rx clock internal delay modes */
>>>> @@ -2785,7 +2807,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_refclk;
>>>>  
>>>>  	if (info->gptp || info->ccc_gac) {
>>>>  		/* Set GTI value */
>>>> @@ -2893,6 +2917,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)
>>>> @@ -2908,8 +2933,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_reset_mode(ndev);
>>>> +	if (error)
>>>> +		netdev_err(ndev, "Failed to reset ndev\n");
>>>>  
>>>>  	clk_disable_unprepare(priv->gptp_clk);
>>>>  	clk_disable_unprepare(priv->refclk);
>>>> @@ -2991,8 +3017,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_reset_mode(ndev);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +	}
>>>>  
>>>>  	/* All register have been reset to default values.
>>>>  	 * Restore all registers which where setup at probe time and
>>>> @@ -3000,7 +3029,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
>>>>
>>>
>