Patchwork gianfar: Revive the driver for eTSEC devices (disable timestamping)

login
register
mail settings
Submitter Anton Vorontsov
Date June 9, 2010, 7:32 p.m.
Message ID <20100609193219.GA8629@oksana.dev.rtsoft.ru>
Download mbox | patch
Permalink /patch/55123/
State Accepted, archived
Commit 619baba195d92ec39379e24c151f4a640898d140
Headers show

Comments

Anton Vorontsov - June 9, 2010, 7:32 p.m.
Since commit cc772ab7cdcaa24d1fae332d92a1602788644f7a ("gianfar: Add
hardware RX timestamping support"), the driver no longer works on
at least MPC8313ERDB and MPC8568EMDS boards (and possibly much more
boards as well).

That's how MPC8313 Reference Manual describes RCTRL_TS_ENABLE bit:

  Timestamp incoming packets as padding bytes. PAL field is set
  to 8 if the PAL field is programmed to less than 8. Must be set
  to zero if TMR_CTRL[TE]=0.

I see that the commit above sets this bit, but it doesn't handle
TMR_CTRL. Manfred probably had this bit set by the firmware for
his boards. But obviously this isn't true for all boards in the
wild.

Also, I recall that Freescale BSPs were explicitly disabling the
timestamping because of a performance drop.

For now, the best way to deal with this is just disable the
timestamping, and later we can discuss proper device tree bindings
and implement enabling this feature via some property.

Signed-off-by: Anton Vorontsov <avorontsov@mvista.com>
---
 drivers/net/gianfar.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
David Miller - June 9, 2010, 11:27 p.m.
From: Anton Vorontsov <avorontsov@mvista.com>
Date: Wed, 9 Jun 2010 23:32:19 +0400

> Since commit cc772ab7cdcaa24d1fae332d92a1602788644f7a ("gianfar: Add
> hardware RX timestamping support"), the driver no longer works on
> at least MPC8313ERDB and MPC8568EMDS boards (and possibly much more
> boards as well).
> 
> That's how MPC8313 Reference Manual describes RCTRL_TS_ENABLE bit:
> 
>   Timestamp incoming packets as padding bytes. PAL field is set
>   to 8 if the PAL field is programmed to less than 8. Must be set
>   to zero if TMR_CTRL[TE]=0.
> 
> I see that the commit above sets this bit, but it doesn't handle
> TMR_CTRL. Manfred probably had this bit set by the firmware for
> his boards. But obviously this isn't true for all boards in the
> wild.
> 
> Also, I recall that Freescale BSPs were explicitly disabling the
> timestamping because of a performance drop.
> 
> For now, the best way to deal with this is just disable the
> timestamping, and later we can discuss proper device tree bindings
> and implement enabling this feature via some property.
> 
> Signed-off-by: Anton Vorontsov <avorontsov@mvista.com>

Agreed, applied, thanks Anton.
Richard Cochran - June 10, 2010, 6:29 a.m.
On Wed, Jun 09, 2010 at 11:32:19PM +0400, Anton Vorontsov wrote:
> Since commit cc772ab7cdcaa24d1fae332d92a1602788644f7a ("gianfar: Add
> hardware RX timestamping support"), the driver no longer works on
> at least MPC8313ERDB and MPC8568EMDS boards (and possibly much more
> boards as well).

What do you mean by, "no longer works?" The driver works fine for us,
even without TMR_CTRL[TE] set. We tested the driver on two MPC8313ERDB
REV C boards, one P2020DS, and one P2020RDB.

> That's how MPC8313 Reference Manual describes RCTRL_TS_ENABLE bit:
> 
>   Timestamp incoming packets as padding bytes. PAL field is set
>   to 8 if the PAL field is programmed to less than 8. Must be set
>   to zero if TMR_CTRL[TE]=0.
> 
> I see that the commit above sets this bit, but it doesn't handle
> TMR_CTRL. Manfred probably had this bit set by the firmware for
> his boards. But obviously this isn't true for all boards in the
> wild.

No, we did not set TMR_CTRL[TE].

For the Rx timestamps, we simply enabled them unconditionally. The
effect of not setting TMR_CTRL[TE] was that the timestamps were
invalid, but that should not matter if user space has not configured
the PTP clock. We left the TMR_CTRL[TE] bit for the PTP clock driver
(recently submitted and discussed on netdev). Actually, I copy the PTP
clock driver to the target via 'scp' during development, and I never
had any trouble.

> Also, I recall that Freescale BSPs were explicitly disabling the
> timestamping because of a performance drop.

The BSPs that we have, for the MPC8313ERDB and the P2020RBD both
include a (hacky) PTP timestmaping driver. Can you be more specific
about where and when Freescale is disabling timestamping?

> For now, the best way to deal with this is just disable the
> timestamping, and later we can discuss proper device tree bindings
> and implement enabling this feature via some property.

Okay, but now we want to identify what exactly works and what not. As
mentioned, we tested this driver on four different boards and did not
see any problems.

Thanks,
Richard
Anton Vorontsov - June 10, 2010, 9:31 a.m.
On Thu, Jun 10, 2010 at 08:29:08AM +0200, Richard Cochran wrote:
> On Wed, Jun 09, 2010 at 11:32:19PM +0400, Anton Vorontsov wrote:
> > Since commit cc772ab7cdcaa24d1fae332d92a1602788644f7a ("gianfar: Add
> > hardware RX timestamping support"), the driver no longer works on
> > at least MPC8313ERDB and MPC8568EMDS boards (and possibly much more
> > boards as well).
> 
> What do you mean by, "no longer works?" The driver works fine for us,

For me it doesn't even able to receive DHCP answer, boot logs
attached. tcpdump on the host side:

12:56:15.266562 IP 0.0.0.0.68 > 255.255.255.255.67: BOOTP/DHCP, Request from 00:e0:0c:00:7e:21, length 548
12:56:15.266697 IP 10.0.0.2.67 > 10.0.0.32.68: BOOTP/DHCP, Reply, length 324
12:56:22.106691 IP 0.0.0.0.68 > 255.255.255.255.67: BOOTP/DHCP, Request from 00:e0:0c:00:7e:21, length 548
12:56:22.106843 IP 10.0.0.2.67 > 10.0.0.32.68: BOOTP/DHCP, Reply, length 324
12:56:33.698908 IP 0.0.0.0.68 > 255.255.255.255.67: BOOTP/DHCP, Request from 00:e0:0c:00:7e:21, length 548
12:56:33.699083 IP 10.0.0.2.67 > 10.0.0.32.68: BOOTP/DHCP, Reply, length 324
12:56:53.607275 IP 0.0.0.0.68 > 255.255.255.255.67: BOOTP/DHCP, Request from 00:e0:0c:00:7e:21, length 548
12:56:53.607430 IP 10.0.0.2.67 > 10.0.0.32.68: BOOTP/DHCP, Reply, length 324

> > That's how MPC8313 Reference Manual describes RCTRL_TS_ENABLE bit:
> > 
> >   Timestamp incoming packets as padding bytes. PAL field is set
> >   to 8 if the PAL field is programmed to less than 8. Must be set
> >   to zero if TMR_CTRL[TE]=0.
> > 
> > I see that the commit above sets this bit, but it doesn't handle
> > TMR_CTRL. Manfred probably had this bit set by the firmware for
> > his boards. But obviously this isn't true for all boards in the
> > wild.
> 
> No, we did not set TMR_CTRL[TE].

So, you deliberately violate the spec and expect it to work
everywhere? :-)

> For the Rx timestamps, we simply enabled them unconditionally. The
> effect of not setting TMR_CTRL[TE] was that the timestamps were
> invalid, but that should not matter if user space has not configured
> the PTP clock.

It seems to matter here, but I haven't tested if enabling the
TE bit actually solves the problem. Disabling timestamping
makes the driver work for sure though.

> > Also, I recall that Freescale BSPs were explicitly disabling the
> > timestamping because of a performance drop.
> 
> The BSPs that we have, for the MPC8313ERDB and the P2020RBD both
> include a (hacky) PTP timestmaping driver. Can you be more specific
> about where and when Freescale is disabling timestamping?

Well, bitshrine site no longer allows[1] to list patches at
http://www.bitshrine.org/gpp/ , but there was a patch for sure.

I'll try to find it in a bunch of .iso files that I have on disk
tho.

> > For now, the best way to deal with this is just disable the
> > timestamping, and later we can discuss proper device tree bindings
> > and implement enabling this feature via some property.
> 
> Okay, but now we want to identify what exactly works and what not. As
> mentioned, we tested this driver on four different boards and did not
> see any problems.

Sure thing, I would happily test any patches. Or if you need
to dump some register from my board, feel free to send a patch
that adds a bunch of printks and I'll send the output to you.

Thanks,

[1] http://lists.nongnu.org/archive/html/ltib/2010-04/msg00192.html
Richard Cochran - June 10, 2010, 10:22 a.m.
On Thu, Jun 10, 2010 at 01:31:59PM +0400, Anton Vorontsov wrote:
> > No, we did not set TMR_CTRL[TE].
> 
> So, you deliberately violate the spec and expect it to work
> everywhere? :-)

Sometimes one must read the manual with a grain of salt ;)

> > > Also, I recall that Freescale BSPs were explicitly disabling the
> > > timestamping because of a performance drop.
> > 
> > The BSPs that we have, for the MPC8313ERDB and the P2020RBD both
> > include a (hacky) PTP timestmaping driver. Can you be more specific
> > about where and when Freescale is disabling timestamping?
> 
> Well, bitshrine site no longer allows[1] to list patches at
> http://www.bitshrine.org/gpp/ , but there was a patch for sure.
> 
> I'll try to find it in a bunch of .iso files that I have on disk
> tho.

Thanks, that will help to track the problem down.

> U-Boot 1.1.6 (Oct  9 2007 - 20:42:39) MPC83XX
...
> CPU: MPC8313E, Rev: 10 at 333.333 MHz

We have a newer CPU revision:

   U-Boot 1.3.0 (Dec 23 2008 - 13:38:14) MPC83XX
   CPU:   e300c3, MPC8313E, Rev: 21 at 333.333 MHz, CSB:  166 MHz

Perhaps a bug was fixed between revision 10 and 21...

You mentioned having trouble on the MPC8568EMDS. Was it the same
problem?

Thanks,
Richard
Anton Vorontsov - June 10, 2010, 10:41 a.m.
On Thu, Jun 10, 2010 at 12:22:26PM +0200, Richard Cochran wrote:
[...]
> > U-Boot 1.1.6 (Oct  9 2007 - 20:42:39) MPC83XX
> ...
> > CPU: MPC8313E, Rev: 10 at 333.333 MHz
> 
> We have a newer CPU revision:
> 
>    U-Boot 1.3.0 (Dec 23 2008 - 13:38:14) MPC83XX
>    CPU:   e300c3, MPC8313E, Rev: 21 at 333.333 MHz, CSB:  166 MHz
> 
> Perhaps a bug was fixed between revision 10 and 21...
> 
> You mentioned having trouble on the MPC8568EMDS. Was it the same
> problem?

Yep, the symptomps were exactly the same, it didn't want to get
the DHCP answer, while I saw the DHCP requests on the host side.

But I don't know if disabling the timestamping fixes 8568, maybe
there is some additional problem with it, no idea as I don't have
8568 board in my hands any longer. I can try any proposed fixes
as soon as I get it back though.

Patch

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index 1830f31..46c69cd 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -747,8 +747,7 @@  static int gfar_of_init(struct of_device *ofdev, struct net_device **pdev)
 			FSL_GIANFAR_DEV_HAS_CSUM |
 			FSL_GIANFAR_DEV_HAS_VLAN |
 			FSL_GIANFAR_DEV_HAS_MAGIC_PACKET |
-			FSL_GIANFAR_DEV_HAS_EXTENDED_HASH |
-			FSL_GIANFAR_DEV_HAS_TIMER;
+			FSL_GIANFAR_DEV_HAS_EXTENDED_HASH;
 
 	ctype = of_get_property(np, "phy-connection-type", NULL);