Re: [PATCH] fec: Restart PPS after link state change

["Csókás Bence" <csokas.bence@prolan.hu>]

On 2022. 08. 09. 19:28, Andrew Lunn wrote:
> On Tue, Aug 09, 2022 at 02:41:19PM +0200, Csókás Bence wrote:
>> On link state change, the controller gets reset,
>> causing PPS to drop out. So we restart it if needed.
>>
>> Signed-off-by: Csókás Bence <csokas.bence@prolan.hu>
>> ---
>>   drivers/net/ethernet/freescale/fec_main.c | 27 ++++++++++++++++++++++-
>>   1 file changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
>> index ca5d49361fdf..c264b1dd5286 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -954,6 +954,7 @@ fec_restart(struct net_device *ndev)
>>   	u32 temp_mac[2];
>>   	u32 rcntl = OPT_FRAME_SIZE | 0x04;
>>   	u32 ecntl = 0x2; /* ETHEREN */
>> +	struct ptp_clock_request ptp_rq = { .type = PTP_CLK_REQ_PPS };
> 
> Is it safe to hard code this? What if the user configured
> PTP_CLK_REQ_EXTTS or PTP_CLK_REQ_PEROUT?

The fec driver doesn't support anything other than PTP_CLK_REQ_PPS. And 
if it will at some point, this will need to be amended anyways.

> 
>>   	/* Whack a reset.  We should wait for this.
>>   	 * For i.MX6SX SOC, enet use AXI bus, we use disable MAC
>> @@ -1119,6 +1120,13 @@ fec_restart(struct net_device *ndev)
>>   	if (fep->bufdesc_ex)
>>   		fec_ptp_start_cyclecounter(ndev);
>>   
>> +	/* Restart PPS if needed */
>> +	if (fep->pps_enable) {
>> +		/* Clear flag so fec_ptp_enable_pps() doesn't return immediately */
>> +		fep->pps_enable = 0;
> 
> If reset causes PPS to stop, maybe it would be better to do this
> unconditionally?

But if it wasn't enabled before the reset in the first place, we 
wouldn't want to unexpectedly start it.

> 
> 	fep->pps_enable = 0;
> 	fep->ptp_caps.enable(&fep->ptp_caps, &ptp_rq, 1);
> 
>> +	if (fep->bufdesc_ex)
>> +		ecntl |= (1 << 4);
> 
> Please replace (1 << 4) with a #define to make it clear what this is doing.

I took it from the original source, line 1138 as of commit #504148f. It 
is the EN1588 bit by the way. I shall replace it with a #define in both 
places then. Though the code is riddled with other magic numbers without 
explanation, and I probably won't be bothered to fix them all.

> 
>> +
>>   	/* We have to keep ENET enabled to have MII interrupt stay working */
>>   	if (fep->quirks & FEC_QUIRK_ENET_MAC &&
>>   		!(fep->wol_flag & FEC_WOL_FLAG_SLEEP_ON)) {
>> -		writel(2, fep->hwp + FEC_ECNTRL);
>> +		ecntl |= 0x2;
>>   		writel(rmii_mode, fep->hwp + FEC_R_CNTRL);
>>   	}
>> +
>> +	writel(ecntl, fep->hwp + FEC_ECNTRL);
>> +
>> +	if (fep->bufdesc_ex)
>> +		fec_ptp_start_cyclecounter(ndev);
>> +
>> +	/* Restart PPS if needed */
>> +	if (fep->pps_enable) {
>> +		/* Clear flag so fec_ptp_enable_pps() doesn't return immediately */
>> +		fep->pps_enable = 0;
>> +		fep->ptp_caps.enable(&fep->ptp_caps, &ptp_rq, 1);
>> +	}
> 
> So you re-start PPS in stop()? Should it keep outputting when the
> interface is down?

Yes. We use PPS to synchronize devices on a common backplane. We use PTP 
to sync this PPS to a master clock. But if PTP sync drops out, we 
wouldn't want the backplane-level synchronization to fail. The PPS needs 
to stay on as long as userspace *explicitly* disables it, regardless of 
what happens to the link.

> 
> Also, if it is always outputting, don't you need to stop it in
> fec_drv_remove(). You probably don't want to still going after the
> driver is unloaded.

Good point, that is one exception we could make to the above statement 
(though even in this case, userspace *really* should disable PPS before 
unloading the module).

> 
>         Andrew

Thanks for the insights,
Bence