| Message ID | 20100609193219.GA8629@oksana.dev.rtsoft.ru (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | 619baba195d92ec39379e24c151f4a640898d140 |
| Headers | show |
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.
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
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
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
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.
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);
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(-)