[PATCH v2 0/1] net: fec: ptp: avoid register access when ipg clock is disabled

[PATCH v2 0/1] net: fec: ptp: avoid register access when ipg clock is disabled

[Fugang Duan]

V2:
* As Richard Cochran's suggestion, use schedule_delayed_work instead of period timer.
* Stop delayed work before ipg clock disable like suspend, ethx close.

Fugang Duan (1):
  net: fec: ptp: avoid register access when ipg clock is disabled

 drivers/net/ethernet/freescale/fec.h      |    2 +-
 drivers/net/ethernet/freescale/fec_main.c |   12 +++++++++++-
 drivers/net/ethernet/freescale/fec_ptp.c  |   15 +++++++--------
 3 files changed, 19 insertions(+), 10 deletions(-)

-- 
1.7.8

[PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled

[Fugang Duan]

The current kernel hang on i.MX6SX with rootfs mount from MMC.
The root cause is ptp rise up period timer to access enet register
even if ipg clock is disabled.

FEC ptp driver start one period timer to read 1588 counter register in the
ptp init function that is called after FEC driver is probed.

To save power, after FEC probe finish, FEC driver disable all clocks including
ipg clock that is needed for register access.

i.MX5x, i.MX6q/dl/sl FEC register access don't cause system hang when ipg clock
is disabled, just return zero value. But for i.MX6sx SOC, it cause system hang.

To avoid the issue, we need to check ptp stack running status. If ptp4l run up,
and then schdule one delayed work to read ptp counter register. If ptp4l stack
is down, cancel the period delayed work. For ethx interface open/close, fec
suspend/resume situations, schdule/cancel the delayed work.

Signed-off-by: Fugang Duan <B38611@freescale.com>
---
 drivers/net/ethernet/freescale/fec.h      |    2 +-
 drivers/net/ethernet/freescale/fec_main.c |   12 +++++++++++-
 drivers/net/ethernet/freescale/fec_ptp.c  |   15 +++++++--------
 3 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index bd53caf..03fad35 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -334,7 +334,7 @@ struct fec_enet_private {
 	u32 cycle_speed;
 	int hwts_rx_en;
 	int hwts_tx_en;
-	struct timer_list time_keep;
+	struct delayed_work time_keep;
 	struct regulator *reg_phy;
 };
 
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 66fe1f6..8befa2c 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -2198,6 +2198,8 @@ fec_enet_open(struct net_device *ndev)
 	napi_enable(&fep->napi);
 	phy_start(fep->phy_dev);
 	netif_start_queue(ndev);
+	if (fep->hwts_tx_en)
+		schedule_delayed_work(&fep->time_keep, HZ);
 	return 0;
 }
 
@@ -2206,6 +2208,9 @@ fec_enet_close(struct net_device *ndev)
 {
 	struct fec_enet_private *fep = netdev_priv(ndev);
 
+	if (fep->hwts_tx_en)
+		cancel_delayed_work_sync(&fep->time_keep);
+
 	phy_stop(fep->phy_dev);
 
 	if (netif_device_present(ndev)) {
@@ -2682,10 +2687,11 @@ fec_drv_remove(struct platform_device *pdev)
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct fec_enet_private *fep = netdev_priv(ndev);
 
+	if (fep->hwts_tx_en)
+		cancel_delayed_work_sync(&fep->time_keep);
 	cancel_work_sync(&fep->tx_timeout_work);
 	unregister_netdev(ndev);
 	fec_enet_mii_remove(fep);
-	del_timer_sync(&fep->time_keep);
 	if (fep->reg_phy)
 		regulator_disable(fep->reg_phy);
 	if (fep->ptp_clock)
@@ -2703,6 +2709,8 @@ static int __maybe_unused fec_suspend(struct device *dev)
 
 	rtnl_lock();
 	if (netif_running(ndev)) {
+		if (fep->hwts_tx_en)
+			cancel_delayed_work_sync(&fep->time_keep);
 		phy_stop(fep->phy_dev);
 		napi_disable(&fep->napi);
 		netif_tx_lock_bh(ndev);
@@ -2746,6 +2754,8 @@ static int __maybe_unused fec_resume(struct device *dev)
 		netif_tx_unlock_bh(ndev);
 		napi_enable(&fep->napi);
 		phy_start(fep->phy_dev);
+		if (fep->hwts_tx_en)
+			schedule_delayed_work(&fep->time_keep, HZ);
 	}
 	rtnl_unlock();
 
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index 82386b2..281f4f8 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -289,9 +289,11 @@ int fec_ptp_set(struct net_device *ndev, struct ifreq *ifr)
 	switch (config.tx_type) {
 	case HWTSTAMP_TX_OFF:
 		fep->hwts_tx_en = 0;
+		cancel_delayed_work_sync(&fep->time_keep);
 		break;
 	case HWTSTAMP_TX_ON:
 		fep->hwts_tx_en = 1;
+		schedule_delayed_work(&fep->time_keep, HZ);
 		break;
 	default:
 		return -ERANGE;
@@ -338,9 +340,10 @@ int fec_ptp_get(struct net_device *ndev, struct ifreq *ifr)
  * fec_time_keep - call timecounter_read every second to avoid timer overrun
  *                 because ENET just support 32bit counter, will timeout in 4s
  */
-static void fec_time_keep(unsigned long _data)
+static void fec_time_keep(struct work_struct *work)
 {
-	struct fec_enet_private *fep = (struct fec_enet_private *)_data;
+	struct delayed_work *dwork = to_delayed_work(work);
+	struct fec_enet_private *fep = container_of(dwork, struct fec_enet_private, time_keep);
 	u64 ns;
 	unsigned long flags;
 
@@ -348,7 +351,7 @@ static void fec_time_keep(unsigned long _data)
 	ns = timecounter_read(&fep->tc);
 	spin_unlock_irqrestore(&fep->tmreg_lock, flags);
 
-	mod_timer(&fep->time_keep, jiffies + HZ);
+	schedule_delayed_work(&fep->time_keep, HZ);
 }
 
 /**
@@ -386,11 +389,7 @@ void fec_ptp_init(struct platform_device *pdev)
 
 	fec_ptp_start_cyclecounter(ndev);
 
-	init_timer(&fep->time_keep);
-	fep->time_keep.data = (unsigned long)fep;
-	fep->time_keep.function = fec_time_keep;
-	fep->time_keep.expires = jiffies + HZ;
-	add_timer(&fep->time_keep);
+	INIT_DELAYED_WORK(&fep->time_keep, fec_time_keep);
 
 	fep->ptp_clock = ptp_clock_register(&fep->ptp_caps, &pdev->dev);
 	if (IS_ERR(fep->ptp_clock)) {
-- 
1.7.8

Re: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled

[Richard Cochran]

On Wed, Aug 13, 2014 at 12:55:05PM +0800, Fugang Duan wrote:

> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 66fe1f6..8befa2c 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -2198,6 +2198,8 @@ fec_enet_open(struct net_device *ndev)
>  	napi_enable(&fep->napi);
>  	phy_start(fep->phy_dev);
>  	netif_start_queue(ndev);
> +	if (fep->hwts_tx_en)
> +		schedule_delayed_work(&fep->time_keep, HZ);
>  	return 0;
>  }
>  
> @@ -2206,6 +2208,9 @@ fec_enet_close(struct net_device *ndev)
>  {
>  	struct fec_enet_private *fep = netdev_priv(ndev);
>  
> +	if (fep->hwts_tx_en)
> +		cancel_delayed_work_sync(&fep->time_keep);

You make the clock logic depend on whether time stamping is enabled or
not. Why do you do that?

The clock should keep ticking, even if time stamping is disabled.

Thanks,
Richard

RE: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled

[fugang.duan]

From: Richard Cochran <richardcochran@gmail.com> Data: Thursday, August 14, 2014 5:20 AM
>To: Duan Fugang-B38611
>Cc: shawn.guo@linaro.org; davem@davemloft.net; netdev@vger.kernel.org
>Subject: Re: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg
>clock is disabled
>
>On Wed, Aug 13, 2014 at 12:55:05PM +0800, Fugang Duan wrote:
>
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>> b/drivers/net/ethernet/freescale/fec_main.c
>> index 66fe1f6..8befa2c 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -2198,6 +2198,8 @@ fec_enet_open(struct net_device *ndev)
>>  	napi_enable(&fep->napi);
>>  	phy_start(fep->phy_dev);
>>  	netif_start_queue(ndev);
>> +	if (fep->hwts_tx_en)
>> +		schedule_delayed_work(&fep->time_keep, HZ);
>>  	return 0;
>>  }
>>
>> @@ -2206,6 +2208,9 @@ fec_enet_close(struct net_device *ndev)  {
>>  	struct fec_enet_private *fep = netdev_priv(ndev);
>>
>> +	if (fep->hwts_tx_en)
>> +		cancel_delayed_work_sync(&fep->time_keep);
>
>You make the clock logic depend on whether time stamping is enabled or not.
>Why do you do that?
>
>The clock should keep ticking, even if time stamping is disabled.
>
If the ethx close, ptp stack cannot run normally, stack stop send sync message with master,
So we also cancel the delayed work.

In addition, ethx interface close also gate ipg clock, if there have register access hw will hang.
So it is necessary to cancel the delayed work.

>Thanks,
>Richard

Re: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled

[Richard Cochran]

On Thu, Aug 14, 2014 at 01:28:28AM +0000, fugang.duan@freescale.com wrote:
> If the ethx close, ptp stack cannot run normally, stack stop send sync message with master,

But this has no effect on whether transmit time stamps are enabled.

The timer work must run whenever the (physical) clock is
running. Otherwise you will have discontinuities in your PTP clock.
The kernel must always behave correctly, regardless of what user space
does.

When the user calls clock_gettime(), the result must always be
consistent. This has nothing to do with time stamping at all.

Thanks,
Richard

RE: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled

[fugang.duan]

From: Richard Cochran <richardcochran@gmail.com> Sent: Thursday, August 14, 2014 4:11 PM
>To: Duan Fugang-B38611
>Cc: shawn.guo@linaro.org; davem@davemloft.net; netdev@vger.kernel.org
>Subject: Re: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg
>clock is disabled
>
>On Thu, Aug 14, 2014 at 01:28:28AM +0000, fugang.duan@freescale.com wrote:
>> If the ethx close, ptp stack cannot run normally, stack stop send sync
>> message with master,
>
>But this has no effect on whether transmit time stamps are enabled.
>
>The timer work must run whenever the (physical) clock is running.
>Otherwise you will have discontinuities in your PTP clock.
>The kernel must always behave correctly, regardless of what user space
>does.
>
>When the user calls clock_gettime(), the result must always be consistent.
>This has nothing to do with time stamping at all.
>
I have some points against your views:
1. HW ptp is one feature of FEC IP, when the ethx interface is closed, ptp slave cannot sync with master,
   So the ptp timer is not accurate, of couse there have discontinuities in ptp clock.
2. When the ethx net interface is closed, FEC IP clocks are disabled, access ptp counter register causes
   Hw hang.
3. PTP is not separate HW IP, which is one hw feature of FEC IP, when the net interface is closed, user
   Call clock_gettime() is not reasonable.

Thanks,
Andy

Re: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled

[Richard Cochran]

On Thu, Aug 14, 2014 at 08:24:39AM +0000, fugang.duan@freescale.com wrote:
> 1. HW ptp is one feature of FEC IP, when the ethx interface is closed, ptp slave cannot sync with master,
>    So the ptp timer is not accurate, of couse there have discontinuities in ptp clock.

No, it just becomes a free running clock.

This is no different than the Linux system clock. It keeps running,
even if ntpd is not.

> 2. When the ethx net interface is closed, FEC IP clocks are disabled, access ptp counter register causes
>    Hw hang.

Well, then, you need to return an error, don't you?

> 3. PTP is not separate HW IP, which is one hw feature of FEC IP, when the net interface is closed, user
>    Call clock_gettime() is not reasonable.

Yes, it is. See #1.

Thanks,
Richard

RE: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled

[fugang.duan]

From: Richard Cochran <richardcochran@gmail.com> Sent: Thursday, August 14, 2014 4:43 PM
>To: Duan Fugang-B38611
>Cc: shawn.guo@linaro.org; davem@davemloft.net; netdev@vger.kernel.org
>Subject: Re: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg
>clock is disabled
>
>On Thu, Aug 14, 2014 at 08:24:39AM +0000, fugang.duan@freescale.com wrote:
>> 1. HW ptp is one feature of FEC IP, when the ethx interface is closed,
>ptp slave cannot sync with master,
>>    So the ptp timer is not accurate, of couse there have discontinuities
>in ptp clock.
>
>No, it just becomes a free running clock.
>
>This is no different than the Linux system clock. It keeps running, even
>if ntpd is not.
>
>> 2. When the ethx net interface is closed, FEC IP clocks are disabled,
>access ptp counter register causes
>>    Hw hang.
>
>Well, then, you need to return an error, don't you?

I know your means. Even if cancel the time_keep work, ptp timer still is running and continutious.
The time_keep work just to avoid timer overrun because FEC just support 32bit counter.

fec_time_keep()
{
...
ns = timecounter_read(&fep->tc);
...
}

static cycle_t fec_ptp_read(const struct cyclecounter *cc)
{
...
        tempval = readl(fep->hwp + FEC_ATIME_CTRL);
        tempval |= FEC_T_CTRL_CAPTURE;
        writel(tempval, fep->hwp + FEC_ATIME_CTRL);

        return readl(fep->hwp + FEC_ATIME);
...
}

And, you said "return an error", I don't know your mean.
In addition, when clock is disabled, FEC ptp counter don't run any more.

>
>> 3. PTP is not separate HW IP, which is one hw feature of FEC IP, when
>the net interface is closed, user
>>    Call clock_gettime() is not reasonable.
>
>Yes, it is. See #1.
>
>Thanks,
>Richard

Re: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled

[Richard Cochran]

On Thu, Aug 14, 2014 at 09:02:07AM +0000, fugang.duan@freescale.com wrote:
> 
> I know your means. Even if cancel the time_keep work, ptp timer still is running and continutious.
> The time_keep work just to avoid timer overrun because FEC just support 32bit counter.

Right, so the time_keep must continue when the clock is running. It
does not depend on whether time stamping is enabled.

> And, you said "return an error", I don't know your mean.
> In addition, when clock is disabled, FEC ptp counter don't run any more.

You can return an error from your 'gettime' method.

Thanks,
Richard

RE: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled

[fugang.duan]

From: Richard Cochran <richardcochran@gmail.com> Sent: Thursday, August 14, 2014 5:13 PM
>To: Duan Fugang-B38611
>Cc: shawn.guo@linaro.org; davem@davemloft.net; netdev@vger.kernel.org
>Subject: Re: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg
>clock is disabled
>
>On Thu, Aug 14, 2014 at 09:02:07AM +0000, fugang.duan@freescale.com wrote:
>>
>> I know your means. Even if cancel the time_keep work, ptp timer still is
>running and continutious.
>> The time_keep work just to avoid timer overrun because FEC just support
>32bit counter.
>
>Right, so the time_keep must continue when the clock is running. It does
>not depend on whether time stamping is enabled.

So, your mean we need to start the time_keep early like the orignal method, on the contrary,
The patch like below is not reasonable:

@@ -289,9 +289,11 @@ int fec_ptp_set(struct net_device *ndev, struct ifreq *ifr)
        switch (config.tx_type) {
        case HWTSTAMP_TX_OFF:
                fep->hwts_tx_en = 0;
+               cancel_delayed_work_sync(&fep->time_keep);
                break;
        case HWTSTAMP_TX_ON:
                fep->hwts_tx_en = 1;
+               schedule_delayed_work(&fep->time_keep, HZ);
                break;
        default:

>
>> And, you said "return an error", I don't know your mean.
>> In addition, when clock is disabled, FEC ptp counter don't run any more.
>
>You can return an error from your 'gettime' method.
>
In the driver:
Gettime()->fec_ptp_gettime()->timecounter_read()->fec_ptp_read()
Period delayed work: fec_time_keep()->timecounter_read()->fec_ptp_read()

When ethx disable, all clocks are disabled. For gettime(), your mean that we check clock is on or not,
If clock is disabled, return an error in .fec_ptp_gettime() ?
How did it in the delayed work ?

Thanks for your review, I am confused by your suggestion.

Thanks,
Andy

Re: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled

[Richard Cochran]

On Thu, Aug 14, 2014 at 09:26:45AM +0000, fugang.duan@freescale.com wrote:
> When ethx disable, all clocks are disabled. For gettime(), your mean that we check clock is on or not,
> If clock is disabled, return an error in .fec_ptp_gettime() ?

Yes.

> How did it in the delayed work ?

The work must run whenever the physical clock is runnning.

This is COMPLETELY INDEPENDENT from time stamping.

If your patch has anything with "fep->hwts_tx_en", then it is wrong.

> Thanks for your review, I am confused by your suggestion.

Just start and stop the work queue when you start and stop the
physical clock.

Is it clear now?

Thanks,
Richard

RE: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg clock is disabled

[fugang.duan]

From: Richard Cochran <richardcochran@gmail.com> Data: Thursday, August 14, 2014 10:34 PM
>To: Duan Fugang-B38611
>Cc: shawn.guo@linaro.org; davem@davemloft.net; netdev@vger.kernel.org
>Subject: Re: [PATCH v2 1/1] net: fec: ptp: avoid register access when ipg
>clock is disabled
>
>On Thu, Aug 14, 2014 at 09:26:45AM +0000, fugang.duan@freescale.com wrote:
>> When ethx disable, all clocks are disabled. For gettime(), your mean
>> that we check clock is on or not, If clock is disabled, return an error
>in .fec_ptp_gettime() ?
>
>Yes.
>
I will change it. Thanks! I will submit the next version.

Thanks for your review.