Re: [PATCH net v3 1/1] net: ravb: Wait for operating mode to be applied

[Sergey Shtylyov]

On 1/2/24 2:01 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> 
> 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 CCC.OPC (and CCC.GAC, CCC.CSEL, if any) 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 CCC.OPC.
> 
> In case of platforms with GAC, if GAC needs to be enabled, the CCC.GAC and

   Better to spell it out, I think: in case of platforms that support gPTP
while in the config[uration] mode..

> CCC.CSEL needs to be configured along with CCC.OPC. For this,
> ravb_set_opmode() allows passing GAC and CSEL as part of opmode and the
> function updates accordingly CCC register.
> 
> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>

Reviewed-by: Sergey Shtylyov <s.shtylyov@xxxxxx>

[...]

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c> index 664eda4b5a11..9835d18a7adf 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -66,16 +66,23 @@ 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_ops = 1U << (opmode & CCC_OPC);
> +	u32 ccc_mask = CCC_OPC;
>  	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);
> -	if (error)
> -		netdev_err(ndev, "failed to switch device to config mode\n");
> +	if (opmode & CCC_GAC)

   Worth a comment?

> +		ccc_mask |= CCC_GAC | CCC_CSEL;

   Adding CCC.GAC to the mask not strictly necessary but OK...

[...]

MBR, Sergey