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

[Andrew Lunn <andrew@lunn.ch>]

> 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.

O.K.

> > 
> > >   	/* 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.

We should decide what fep->pps_enable actually means. It should be
enabled, or it is actually enabled? Then it becomes clear if the reset
function should clear it to reflect the hardware, or if the
fec_ptp_enable_pps() should not be looking at it, and needs to read
the status from the hardware.

> > 	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.

Yes, i understand. It just makes it easier to review if you fixup
parts of the code you are changing.

> > 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.

We need the PTP Maintainers view on that. I don't know if that is
normal or not.

> > 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).

Never trust userspace. Ever.

      Andrew