Re: [PATCH net 2/2] net: ti: icssg-prueth: Fix clearing of IEP_CMP_CFG registers during iep_init

[Roger Quadros]

Hi,

On 06/11/2024 09:40, Meghana Malladi wrote:
> When ICSSG interfaces are brought down and brought up again, the
> pru cores are shut down and booted again, flushing out all the memories
> and start again in a clean state. Hence it is expected that the
> IEP_CMP_CFG register needs to be flushed during iep_init() to ensure
> that the existing residual configuration doesn't cause any unusual
> behavior. If the register is not cleared, existing IEP_CMP_CFG set for
> CMP1 will result in SYNC0_OUT signal based on the SYNC_OUT register values.
> 
> After bringing the interface up, calling PPS enable doesn't work as
> the driver believes PPS is already enabled, (iep->pps_enabled is not
> cleared during interface bring down) and driver  will just return true
> even though there is no signal. Fix this by setting the iep->pps_enable
> and iep->perout_enable flags to false during the link down.
> 
> Fixes: c1e0230eeaab ("net: ti: icss-iep: Add IEP driver")
> Signed-off-by: Meghana Malladi <m-malladi@xxxxxx>
> ---
>  drivers/net/ethernet/ti/icssg/icss_iep.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/net/ethernet/ti/icssg/icss_iep.c b/drivers/net/ethernet/ti/icssg/icss_iep.c
> index 5d6d1cf78e93..03abc25ced12 100644
> --- a/drivers/net/ethernet/ti/icssg/icss_iep.c
> +++ b/drivers/net/ethernet/ti/icssg/icss_iep.c
> @@ -195,6 +195,12 @@ static void icss_iep_enable_shadow_mode(struct icss_iep *iep)
>  
>  	icss_iep_disable(iep);
>  
> +	/* clear compare config */
> +	for (cmp = IEP_MIN_CMP; cmp < IEP_MAX_CMP; cmp++) {
> +		regmap_update_bits(iep->map, ICSS_IEP_CMP_CFG_REG,
> +				   IEP_CMP_CFG_CMP_EN(cmp), 0);
> +	}
> +

A bit later we are clearing compare status. Can clearing CMP be done in same for loop?

>  	/* disable shadow mode */
>  	regmap_update_bits(iep->map, ICSS_IEP_CMP_CFG_REG,
>  			   IEP_CMP_CFG_SHADOW_EN, 0);
> @@ -778,6 +784,10 @@ int icss_iep_exit(struct icss_iep *iep)
>  		ptp_clock_unregister(iep->ptp_clock);
>  		iep->ptp_clock = NULL;
>  	}
> +
> +	iep->pps_enabled = false;
> +	iep->perout_enabled = false;
> +

But how do you keep things in sync with user space?
User might have enabled PPS or PEROUT and then put SLICE0 interface down.
Then if SLICE0 is brought up should PPS/PEROUT keep working like before?
We did call ptp_clock_unregister() so it should unregister the PPS as well.
What I'm not sure is if it calls the ptp->enable() hook to disable the PPS/PEROUT.

If yes then that should take care of the flags as well.

If not then you need to call the relevant hooks explicitly but just after
ptp_clock_unregister().
e.g.
	if (iep->pps_enabled)
		icss_iep_pps_enable(iep, false);
	else if (iep->perout_enabled)
		icss_iep_perout_enable(iep, NULL, false);

But this means that user has to again setup PPS/PEROUT.	

>  	icss_iep_disable(iep);
>  
>  	return 0;

-- 
cheers,
-roger