[PATCH] net: fec: ptp: fix convergence issue to support LinuxPTP stack

Discussion:

Fugang Duan

2014-10-14 05:39:47 UTC

IEEE 1588 module has one hw issue in capturing the ATVR register. According
to the user manual it is:
ENET0->ATCR |= ENET_ATCR_CAPTURE_MASK;
while(ENET0->ATCR & ENET_ATCR_CAPTURE_MASK);
ts_counter_ns = ENET0->ATVR;

Incorrect behavior for ENET_ATCR[Capture and Restart Bits]. These bits will always
read a value zero. According to SPEC, when these bits are set to 1'b1, these should
hold value 1'b1 until the counter value is capture in the register clock domain.

Unfortunately there is a bug with the way the bit "ENET_ATCR_CAPTURE" clears.
So need something like:
ENET0->ATCR |= ENET_ATCR_CAPTURE_MASK;
wait();
ts_counter_ns = ENET0->ATVR;

The wait-time to be at least 6 clock cycle of the slower clock between the register
clock and the 1588 clock. The 1588 ts_clk is 25Mhz, register clock is 66Mhz, so the
wait-time must be greater than 240ns (40ns * 6). The workaround is that adding 1us
delay before read ATVR.

Signed-off-by: Fugang Duan <***@freescale.com>
---
drivers/net/ethernet/freescale/fec.h | 47 +++++++++++++++++++++++++++++
drivers/net/ethernet/freescale/fec_main.c | 43 +-------------------------
drivers/net/ethernet/freescale/fec_ptp.c | 5 +++
3 files changed, 53 insertions(+), 42 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 1d5e182..b4dccec 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -367,6 +367,53 @@ struct bufdesc_ex {
#define FEC_VLAN_TAG_LEN 0x04
#define FEC_ETHTYPE_LEN 0x02

+/* Controller is ENET-MAC */
+#define FEC_QUIRK_ENET_MAC (1 << 0)
+/* Controller needs driver to swap frame */
+#define FEC_QUIRK_SWAP_FRAME (1 << 1)
+/* Controller uses gasket */
+#define FEC_QUIRK_USE_GASKET (1 << 2)
+/* Controller has GBIT support */
+#define FEC_QUIRK_HAS_GBIT (1 << 3)
+/* Controller has extend desc buffer */
+#define FEC_QUIRK_HAS_BUFDESC_EX (1 << 4)
+/* Controller has hardware checksum support */
+#define FEC_QUIRK_HAS_CSUM (1 << 5)
+/* Controller has hardware vlan support */
+#define FEC_QUIRK_HAS_VLAN (1 << 6)
+/* ENET IP errata ERR006358
+ *
+ * If the ready bit in the transmit buffer descriptor (TxBD[R]) is previously
+ * detected as not set during a prior frame transmission, then the
+ * ENET_TDAR[TDAR] bit is cleared at a later time, even if additional TxBDs
+ * were added to the ring and the ENET_TDAR[TDAR] bit is set. This results in
+ * frames not being transmitted until there is a 0-to-1 transition on
+ * ENET_TDAR[TDAR].
+ */
+#define FEC_QUIRK_ERR006358 (1 << 7)
+/* ENET IP hw AVB
+ *
+ * i.MX6SX ENET IP add Audio Video Bridging (AVB) feature support.
+ * - Two class indicators on receive with configurable priority
+ * - Two class indicators and line speed timer on transmit allowing
+ * implementation class credit based shapers externally
+ * - Additional DMA registers provisioned to allow managing up to 3
+ * independent rings
+ */
+#define FEC_QUIRK_HAS_AVB (1 << 8)
+/* There is a TDAR race condition for mutliQ when the software sets TDAR
+ * and the UDMA clears TDAR simultaneously or in a small window (2-4 cycles).
+ * This will cause the udma_tx and udma_tx_arbiter state machines to hang.
+ * The issue exist at i.MX6SX enet IP.
+ */
+#define FEC_QUIRK_ERR007885 (1 << 9)
+/* ENET Block Guide/ Chapter for the iMX6SLX (PELE) address one issue:
+ * Incorrect behavior for ENET_ATCR[Capture and Restart Bits]. These bits will
+ * always read a value zero. When these bits are set to 1'b1, these should hold
+ * value 1'b1 until the counter value is capture in the register clock domain.
+ */
+#define FEC_QUIRK_BUG_CAPTURE (1 << 10)
+
struct fec_enet_priv_tx_q {
int index;
unsigned char *tx_bounce[TX_RING_SIZE];
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 7a8209e..26d0525 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -78,47 +78,6 @@ static void fec_enet_itr_coal_init(struct net_device *ndev);
#define FEC_ENET_RAFL_V 0x8
#define FEC_ENET_OPD_V 0xFFF0

-/* Controller is ENET-MAC */
-#define FEC_QUIRK_ENET_MAC (1 << 0)
-/* Controller needs driver to swap frame */
-#define FEC_QUIRK_SWAP_FRAME (1 << 1)
-/* Controller uses gasket */
-#define FEC_QUIRK_USE_GASKET (1 << 2)
-/* Controller has GBIT support */
-#define FEC_QUIRK_HAS_GBIT (1 << 3)
-/* Controller has extend desc buffer */
-#define FEC_QUIRK_HAS_BUFDESC_EX (1 << 4)
-/* Controller has hardware checksum support */
-#define FEC_QUIRK_HAS_CSUM (1 << 5)
-/* Controller has hardware vlan support */
-#define FEC_QUIRK_HAS_VLAN (1 << 6)
-/* ENET IP errata ERR006358
- *
- * If the ready bit in the transmit buffer descriptor (TxBD[R]) is previously
- * detected as not set during a prior frame transmission, then the
- * ENET_TDAR[TDAR] bit is cleared at a later time, even if additional TxBDs
- * were added to the ring and the ENET_TDAR[TDAR] bit is set. This results in
- * frames not being transmitted until there is a 0-to-1 transition on
- * ENET_TDAR[TDAR].
- */
-#define FEC_QUIRK_ERR006358 (1 << 7)
-/* ENET IP hw AVB
- *
- * i.MX6SX ENET IP add Audio Video Bridging (AVB) feature support.
- * - Two class indicators on receive with configurable priority
- * - Two class indicators and line speed timer on transmit allowing
- * implementation class credit based shapers externally
- * - Additional DMA registers provisioned to allow managing up to 3
- * independent rings
- */
-#define FEC_QUIRK_HAS_AVB (1 << 8)
-/* There is a TDAR race condition for mutliQ when the software sets TDAR
- * and the UDMA clears TDAR simultaneously or in a small window (2-4 cycles).
- * This will cause the udma_tx and udma_tx_arbiter state machines to hang.
- * The issue exist at i.MX6SX enet IP.
- */
-#define FEC_QUIRK_ERR007885 (1 << 9)
-
static struct platform_device_id fec_devtype[] = {
{
/* keep it for coldfire */
@@ -146,7 +105,7 @@ static struct platform_device_id fec_devtype[] = {
.driver_data = FEC_QUIRK_ENET_MAC | FEC_QUIRK_HAS_GBIT |
FEC_QUIRK_HAS_BUFDESC_EX | FEC_QUIRK_HAS_CSUM |
FEC_QUIRK_HAS_VLAN | FEC_QUIRK_HAS_AVB |
- FEC_QUIRK_ERR007885,
+ FEC_QUIRK_ERR007885 | FEC_QUIRK_BUG_CAPTURE,
}, {
/* sentinel */
}
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index cca3617..380bb10 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -82,12 +82,17 @@ static cycle_t fec_ptp_read(const struct cyclecounter *cc)
{
struct fec_enet_private *fep =
container_of(cc, struct fec_enet_private, cc);
+ const struct platform_device_id *id_entry =
+ platform_get_device_id(fep->pdev);
u32 tempval;

tempval = readl(fep->hwp + FEC_ATIME_CTRL);
tempval |= FEC_T_CTRL_CAPTURE;
writel(tempval, fep->hwp + FEC_ATIME_CTRL);

+ if (id_entry->driver_data & FEC_QUIRK_BUG_CAPTURE)
+ udelay(1);
+
return readl(fep->hwp + FEC_ATIME);
}

--
1.7.8

Richard Cochran

2014-10-14 10:36:10 UTC

Permalink

Post by Fugang Duan
IEEE 1588 module has one hw issue in capturing the ATVR register. According
ENET0->ATCR |= ENET_ATCR_CAPTURE_MASK;
while(ENET0->ATCR & ENET_ATCR_CAPTURE_MASK);
ts_counter_ns = ENET0->ATVR;

...

Post by Fugang Duan
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index cca3617..380bb10 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -82,12 +82,17 @@ static cycle_t fec_ptp_read(const struct cyclecounter *cc)
{
struct fec_enet_private *fep =
container_of(cc, struct fec_enet_private, cc);
+ const struct platform_device_id *id_entry =
+ platform_get_device_id(fep->pdev);
u32 tempval;
tempval = readl(fep->hwp + FEC_ATIME_CTRL);
tempval |= FEC_T_CTRL_CAPTURE;
writel(tempval, fep->hwp + FEC_ATIME_CTRL);
+ if (id_entry->driver_data & FEC_QUIRK_BUG_CAPTURE)
+ udelay(1);
+

What? You never had

while(ENET0->ATCR & ENET_ATCR_CAPTURE_MASK);

in the first place. Maybe you should try that.

Did this code ever work? I guess not.

Post by Fugang Duan
return readl(fep->hwp + FEC_ATIME);
}

Thanks,
Richard

David Miller

2014-10-14 16:52:39 UTC

Permalink

From: Richard Cochran <***@gmail.com>
Date: Tue, 14 Oct 2014 12:36:10 +0200

Post by Richard Cochran

...

What? You never had
while(ENET0->ATCR & ENET_ATCR_CAPTURE_MASK);
in the first place. Maybe you should try that.
Did this code ever work? I guess not.

Also see Luwei Zhou's series:

http://patchwork.ozlabs.org/patch/398467/
http://patchwork.ozlabs.org/patch/398465/
http://patchwork.ozlabs.org/patch/398466/

Let's get down to the bottom of all of this before I apply anything. Does this
fec PTP code work at all?

F***@freescale.com

2014-10-14 18:27:28 UTC

Permalink

-----Original Message-----
Sent: Tuesday, October 14, 2014 11:53 AM
Frank-B20596
Subject: Re: [PATCH] net: fec: ptp: fix convergence issue to support LinuxPTP
stack
Date: Tue, 14 Oct 2014 12:36:10 +0200

Post by Richard Cochran

Post by Fugang Duan
IEEE 1588 module has one hw issue in capturing the ATVR register.
ENET0->ATCR |= ENET_ATCR_CAPTURE_MASK;
while(ENET0->ATCR & ENET_ATCR_CAPTURE_MASK);
ts_counter_ns = ENET0->ATVR;

...

Post by Fugang Duan
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
b/drivers/net/ethernet/freescale/fec_ptp.c
index cca3617..380bb10 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -82,12 +82,17 @@ static cycle_t fec_ptp_read(const struct
cyclecounter *cc) {
struct fec_enet_private *fep =
container_of(cc, struct fec_enet_private, cc);
+ const struct platform_device_id *id_entry =
+ platform_get_device_id(fep->pdev);
u32 tempval;
tempval = readl(fep->hwp + FEC_ATIME_CTRL);
tempval |= FEC_T_CTRL_CAPTURE;
writel(tempval, fep->hwp + FEC_ATIME_CTRL);
+ if (id_entry->driver_data & FEC_QUIRK_BUG_CAPTURE)
+ udelay(1);
+

What? You never had
while(ENET0->ATCR & ENET_ATCR_CAPTURE_MASK);
in the first place. Maybe you should try that.
Did this code ever work? I guess not.

http://patchwork.ozlabs.org/patch/398467/
http://patchwork.ozlabs.org/patch/398465/
http://patchwork.ozlabs.org/patch/398466/
Let's get down to the bottom of all of this before I apply anything. Does this
fec PTP code work at all?

Yes, Luwei's 3 patch tested at MX6Q, MX6DL platform.

Only i.MX6SX has the problem, which fixed by fugang's patch.

i.MX6SX's ptp have the same issue even though without luwei's patch.

So luwei's patch is independent with fugang's patch.

Luwei's patch use hardware ptp adjustment method.
Fugang's patch fix the problem existed in MX6SX platform.

So I suggest apply luwei's 3 patches firstly.

Best regards
Frank Li

Richard Cochran

2014-10-14 18:39:33 UTC

Permalink

Frank,

Post by F***@freescale.com
Fugang's patch fix the problem existed in MX6SX platform.

You say that Fugang's patch fixes a quirk in the SX device.

But he said that the user's manual tells us to wait for some status
bits to clear. (See the third line, with the 'while' key word).

Post by F***@freescale.com

That applies to all IMX devices, doesn't it?
In any case, Fugang's change log is not clear.

Confused,
Richard

F***@freescale.com

2014-10-14 18:43:51 UTC

Permalink

-----Original Message-----
Sent: Tuesday, October 14, 2014 1:40 PM
To: Li Frank-B20596
Subject: Re: [PATCH] net: fec: ptp: fix convergence issue to support LinuxPTP
stack
Frank,

Post by F***@freescale.com
Fugang's patch fix the problem existed in MX6SX platform.

You say that Fugang's patch fixes a quirk in the SX device.
But he said that the user's manual tells us to wait for some status bits to
clear. (See the third line, with the 'while' key word).

Post by F***@freescale.com

That applies to all IMX devices, doesn't it?
In any case, Fugang's change log is not clear.

Confused,
Richard

Richard Cochran

2014-10-14 18:49:52 UTC

Permalink

Post by F***@freescale.com
Only MX6 SX. Only MX6 SX added FEC_QUIRK_BUG_CAPTURE.

But what about this comment:

+/* ENET Block Guide/ Chapter for the iMX6SLX (PELE) address one issue:
+ * Incorrect behavior for ENET_ATCR[Capture and Restart Bits]. These bits will
+ * always read a value zero. When these bits are set to 1'b1, these should hold
+ * value 1'b1 until the counter value is capture in the register clock domain.
+ */

It sounds like the bits are "sticky" until the counter value has been
latched. Therefore you need to check the bits before reading the
counter, but the code does not do this for the case of !SX.

Thanks,
Richard

f***@freescale.com

2014-10-15 01:28:31 UTC

Permalink

To: Li Frank-B20596
Subject: Re: [PATCH] net: fec: ptp: fix convergence issue to support
LinuxPTP stack

Post by F***@freescale.com
Only MX6 SX. Only MX6 SX added FEC_QUIRK_BUG_CAPTURE.

+ * Incorrect behavior for ENET_ATCR[Capture and Restart Bits]. These
+bits will
+ * always read a value zero. When these bits are set to 1'b1, these
+should hold
+ * value 1'b1 until the counter value is capture in the register clock domain.
+ */
It sounds like the bits are "sticky" until the counter value has been
latched. Therefore you need to check the bits before reading the counter,
but the code does not do this for the case of !SX.
Thanks,
Richard

Hi, Richard,

Fristly, the patch just fix ptp issue for imx6sx.
Secondly, pls see the patch commit log:

IEEE 1588 module has one hw issue in capturing the ATVR register. According
to the user manual it is:
ENET0->ATCR |= ENET_ATCR_CAPTURE_MASK;
while(ENET0->ATCR & ENET_ATCR_CAPTURE_MASK);
ts_counter_ns = ENET0->ATVR;
Incorrect behavior for ENET_ATCR[Capture and Restart Bits]. These bits will always
read a value zero. According to SPEC, when these bits are set to 1'b1, these should
hold value 1'b1 until the counter value is capture in the register clock domain.

Unfortunately there is a bug with the way the bit "ENET_ATCR_CAPTURE" clears.
So need something like:
ENET0->ATCR |= ENET_ATCR_CAPTURE_MASK;
wait();
ts_counter_ns = ENET0->ATVR;

The wait-time to be at least 6 clock cycle of the slower clock between the register
clock and the 1588 clock. The 1588 ts_clk is 25Mhz, register clock is 66Mhz, so the
wait-time must be greater than 240ns (40ns * 6). The workaround is that adding 1us
delay before read ATVR.

There need add delay instead of while().

Thanks,
Andy

f***@freescale.com

2014-10-15 01:37:14 UTC

Permalink

To: Li Frank-B20596
Subject: Re: [PATCH] net: fec: ptp: fix convergence issue to support
LinuxPTP stack

Post by F***@freescale.com
Only MX6 SX. Only MX6 SX added FEC_QUIRK_BUG_CAPTURE.

Hi, Richard,

These "__should__" hold 1'b1 until the counter value is capture in the register clock domain.
But "Incorrect behavior for ENET_ATCR[Capture and Restart Bits]. These bits will always read a value zero"

Thanks,
Andy

f***@freescale.com

2014-10-15 01:30:41 UTC

Permalink

To: Li Frank-B20596
Subject: Re: [PATCH] net: fec: ptp: fix convergence issue to support
LinuxPTP stack
Frank,

Post by F***@freescale.com
Fugang's patch fix the problem existed in MX6SX platform.

You say that Fugang's patch fixes a quirk in the SX device.
But he said that the user's manual tells us to wait for some status bits
to clear. (See the third line, with the 'while' key word).

Post by F***@freescale.com

That applies to all IMX devices, doesn't it?
In any case, Fugang's change log is not clear.
Confused,
Richard

Pls read the commit log in entire ?

Regrads,
Andy

Richard Cochran

2014-10-15 07:25:40 UTC

Permalink

Post by f***@freescale.com
Pls read the commit log in entire ?

I did read it.

It is incomprehensible.

Can you please fix it?

Thanks,
Richard

f***@freescale.com

2014-10-15 07:42:13 UTC

Permalink

To: Duan Fugang-B38611
Subject: Re: [PATCH] net: fec: ptp: fix convergence issue to support
LinuxPTP stack

Post by f***@freescale.com
Pls read the commit log in entire ?

I did read it.
It is incomprehensible.
Can you please fix it?
Thanks,
Richard

Ok, I will enhance the commit/comment log and then send out V2.

Thanks,
Andy