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

[Roger Quadros <rogerq@kernel.org>]

On 12/11/2024 11:04, Meghana Malladi wrote:
> 
> On 11/11/24 19:23, Roger Quadros wrote:
>> 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@ti.com>
>>> ---
>>>   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?
>>
> 
> Yes it can be done in the same loop, I will update that.
> 
>>>       /* 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?
> 
> No, why? Because either both SLICE0 and SLICE1 run when atleast one interface is up and both SLICE0 and SLICE1 are stopped when both the interfaces are brought down. So when SLICE0 is brought down, SLICE1 is also brought down. Next time you bring an interface up, it is a fresh boot for both SLICE1 and SLICE0. In this case, just like how we register for ptp clock (this is handled by the driver in icss_iep_init(),
> pps also needs to be enabled (this has to be done by the user).

I just checked that PPS/PEROUT sysfs don't implement the show hook. So there
is nothing to be in sync with user space.

> 
>> 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.
>>
> 
> No, ptp_clock_unregister() doesn't unregister PPS.
> 
>> 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);
>>
> 
> This doesn't work because if pps_enabled is already true, it goes to icss_iep_pps_enable(), but inside it checks if pps_enabled is true, if so it returns 0, without acutally enabling pps. Which is why we need to set pps_enable and perout_enable to false.

Note that we are passing false in the last argument. i.e. we want to disable PPS/PEROUT.
I don't see why it won't work.

> 
>> But this means that user has to again setup PPS/PEROUT.   
>>
> 
> So yes, this is the expected behavior for user to setup PPS/PEROUT after bringing up an interface. To clarify when user needs to again setup PPS:
> 
> 1. eth1 and eth2 are up, and one interface is brought down -> PPS/PEROUT will be working the same
> 2. No interface is up, and one interface is brought up -> PPS/PEROUT needs to be enabled

OK.

> 
>>>       icss_iep_disable(iep);
>>>         return 0;
>>

-- 
cheers,
-roger