[PATCH] fec: Restart PPS after link state change

[PATCH] fec: Restart PPS after link state change

[Csókás Bence]

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 };
 
 	/* 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;
+		fep->ptp_caps.enable(&fep->ptp_caps, &ptp_rq, 1);
+	}
+
 	/* Enable interrupts we wish to service */
 	if (fep->link)
 		writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
@@ -1154,6 +1162,8 @@ fec_stop(struct net_device *ndev)
 	struct fec_enet_private *fep = netdev_priv(ndev);
 	u32 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & (1 << 8);
 	u32 val;
+	struct ptp_clock_request ptp_rq = { .type = PTP_CLK_REQ_PPS };
+	u32 ecntl = 0;
 
 	/* We cannot expect a graceful transmit stop without link !!! */
 	if (fep->link) {
@@ -1184,12 +1194,27 @@ fec_stop(struct net_device *ndev)
 	}
 	writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
 
+	if (fep->bufdesc_ex)
+		ecntl |= (1 << 4);
+
 	/* 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);
+	}
 }
 
 
-- 
2.25.1

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

[Andrew Lunn]

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?

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

	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.

> +
>  	/* 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?

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.

       Andrew

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

[Csókás Bence]

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

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

[Andrew Lunn]

> 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

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

[Richard Cochran]

On Thu, Aug 11, 2022 at 02:05:39AM +0200, Andrew Lunn wrote:
> > 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.

IMO the least surprising behavior is that once enabled, a feature
stays on until explicitly disabled.

Thanks,
Richard

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

[Andrew Lunn]

On Wed, Aug 10, 2022 at 06:37:19PM -0700, Richard Cochran wrote:
> On Thu, Aug 11, 2022 at 02:05:39AM +0200, Andrew Lunn wrote:
> > > 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.
> 
> IMO the least surprising behavior is that once enabled, a feature
> stays on until explicitly disabled.

O.K, thanks for the response.

Your answer is a bit surprising to me. To me, an interface which is
administratively down is completely inactive. The action to down it
should disable everything.

So your answer also implies PPS can be used before the interface is
set administratively up?

	Andrew

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

[Csókás Bence]

On 2022. 08. 11. 4:05, Andrew Lunn wrote:
> On Wed, Aug 10, 2022 at 06:37:19PM -0700, Richard Cochran wrote:
>> On Thu, Aug 11, 2022 at 02:05:39AM +0200, Andrew Lunn wrote:
>>>> 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.
>>
>> IMO the least surprising behavior is that once enabled, a feature
>> stays on until explicitly disabled.
> 
> O.K, thanks for the response.
> 
> Your answer is a bit surprising to me. To me, an interface which is
> administratively down is completely inactive. The action to down it
> should disable everything.

I think you are confusing two states here. This patch addresses the bug 
thatcauses the PPS to drop when the Ethernet link goes away. The 
interface remainsUP the whole time.

> 
> So your answer also implies PPS can be used before the interface is
> set administratively up?

The PPS can already be used before the first link-up, but once it has 
acquired a link once, PPS no longer works without a link.

> 
> 	Andrew

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

[Richard Cochran]

On Thu, Aug 11, 2022 at 04:05:03AM +0200, Andrew Lunn wrote:
> So your answer also implies PPS can be used before the interface is
> set administratively up?

Why not?

After all, the clock is (or should be) independent from the MAC.
It works all by itself.

Thanks,
Richard

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

[Csókás Bence]

On 2022. 08. 11. 2:05, Andrew Lunn wrote:
>>>
>>>>    	/* 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` is the state of the PPS the driver *believes* to be 
the case. After a reset, this belief may or may not be true anymore: if 
the driver believed formerly that the PPS is down, then after a reset, 
its belief will still be correct, thus nothing needs to be done about 
the situation. If, however, the driver thought that PPS was up, after 
controller reset, it no longer holds, so we need to update our 
world-view (`fep->pps_enable = 0;`), and then correct for the fact that 
PPS just unexpectedly stopped.

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

Amen. Said issue is already fixed in the next version of the patch.

> 
>        Andrew

Bence

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

[Andrew Lunn]

> `fep->pps_enable` is the state of the PPS the driver *believes* to be the
> case. After a reset, this belief may or may not be true anymore: if the
> driver believed formerly that the PPS is down, then after a reset, its
> belief will still be correct, thus nothing needs to be done about the
> situation. If, however, the driver thought that PPS was up, after controller
> reset, it no longer holds, so we need to update our world-view
> (`fep->pps_enable = 0;`), and then correct for the fact that PPS just
> unexpectedly stopped.

Your way of doing it just seems very unclean. I would make
fec_ptp_enable_pps() read the actual status from the
hardware. fep->pps_enable then has the clear meaning of user space
requested it should be enabled.

	  Andrew

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

[Csókás Bence]

On 2022. 08. 11. 15:30, Andrew Lunn wrote:
>> `fep->pps_enable` is the state of the PPS the driver *believes* to be the
>> case. After a reset, this belief may or may not be true anymore: if the
>> driver believed formerly that the PPS is down, then after a reset, its
>> belief will still be correct, thus nothing needs to be done about the
>> situation. If, however, the driver thought that PPS was up, after controller
>> reset, it no longer holds, so we need to update our world-view
>> (`fep->pps_enable = 0;`), and then correct for the fact that PPS just
>> unexpectedly stopped.
> 
> Your way of doing it just seems very unclean. I would make
> fec_ptp_enable_pps() read the actual status from the
> hardware. fep->pps_enable then has the clear meaning of user space
> requested it should be enabled.

1. It is not "my way", it is how it was in the original code. I am 
merely following those who came before me.
2. There is already a variable which holds userspace's wish: parameter 
`uint enable` in `fec_ptp_enable_pps()`. `fep->pps_enable` is whether 
the driver already fulfilled this wish.

> 
> 	  Andrew

Honestly, I would rather see the entire `fec` driver re-written from 
scratch, it is really bad code and full of bugs. Plus, Fugang Duan's 
mail server keeps bouncing back all my emails (I can only hope he sees 
these mails through the mailing list). However, that exceeds my 
capabilities unfortunately (I know not nearly enough of the various 
fec-based controllers and their internals, I only have the i.MX6 TX6UL 
to test). So the best I can do is provide fixes to the bugs we 
experienced, while changing as little of the original driver's code as 
possible, so as to (hopefully) not introduce even more bugs.

Bence

[PATCH] fec: Restart PPS after link state change

[Csókás Bence]

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 | 32 +++++++++++++++++++++--
 drivers/net/ethernet/freescale/fec_ptp.c  |  3 +++
 2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 366c52b62d4b..546a152df4f4 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -257,6 +257,9 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
 #define FEC_MMFR_TA		(2 << 16)
 #define FEC_MMFR_DATA(v)	(v & 0xffff)
 /* FEC ECR bits definition */
+#define FEC_ECR_RESET   	(1 << 0)
+#define FEC_ECR_ETHEREN 	(1 << 1)
+#define FEC_ECR_EN1588  	(1 << 4)
 #define FEC_ECR_MAGICEN		(1 << 2)
 #define FEC_ECR_SLEEP		(1 << 3)
 
@@ -955,6 +958,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 };
 
 	/* Whack a reset.  We should wait for this.
 	 * For i.MX6SX SOC, enet use AXI bus, we use disable MAC
@@ -1106,7 +1110,7 @@ fec_restart(struct net_device *ndev)
 	}
 
 	if (fep->bufdesc_ex)
-		ecntl |= (1 << 4);
+		ecntl |= FEC_ECR_EN1588;
 
 #ifndef CONFIG_M5272
 	/* Enable the MIB statistic event counters */
@@ -1120,6 +1124,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;
+		fep->ptp_caps.enable(&fep->ptp_caps, &ptp_rq, 1);
+	}
+
 	/* Enable interrupts we wish to service */
 	if (fep->link)
 		writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
@@ -1155,6 +1166,8 @@ fec_stop(struct net_device *ndev)
 	struct fec_enet_private *fep = netdev_priv(ndev);
 	u32 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & (1 << 8);
 	u32 val;
+	struct ptp_clock_request ptp_rq = { .type = PTP_CLK_REQ_PPS };
+	u32 ecntl = 0;
 
 	/* We cannot expect a graceful transmit stop without link !!! */
 	if (fep->link) {
@@ -1185,12 +1198,27 @@ fec_stop(struct net_device *ndev)
 	}
 	writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
 
+	if (fep->bufdesc_ex)
+		ecntl |= FEC_ECR_EN1588;
+
 	/* 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 |= FEC_ECR_ETHEREN;
 		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);
+	}
 }
 
 
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index a5077eff305b..869d149efc53 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -628,6 +628,9 @@ void fec_ptp_stop(struct platform_device *pdev)
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct fec_enet_private *fep = netdev_priv(ndev);
 
+	if (fep->pps_enable)
+		fec_ptp_enable_pps(fep, 0);
+
 	cancel_delayed_work_sync(&fep->time_keep);
 	if (fep->ptp_clock)
 		ptp_clock_unregister(fep->ptp_clock);
-- 
2.25.1