[U-Boot] [PATCH 0/3]: arm: Kirkwood: Various egiga send fixes
Hi!
These three patches fix a few issues I've had with sending data on the network interface on the OpenRD base board. In summary, they do:
- Fix compiler optimization bug in kwgbe_send - Check the error summary bit during send - Allow sending data which is not 8-byte aligned
// Simon
Fix compiler optimization bug for kwgbe_send
kwgbe_send/recv both have loops waiting for the hardware to set a bit. GCC 4.3.3 cleverly optimizes the send case to ... a while(1); loop. This patch makes the structure volatile to force re-loading of the transmit descriptor.
Signed-off-by: Simon Kagstrom simon.kagstrom@netinsight.net --- drivers/net/kirkwood_egiga.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/kirkwood_egiga.c b/drivers/net/kirkwood_egiga.c index 3c5db19..db55d9d 100644 --- a/drivers/net/kirkwood_egiga.c +++ b/drivers/net/kirkwood_egiga.c @@ -484,9 +484,9 @@ static int kwgbe_halt(struct eth_device *dev) static int kwgbe_send(struct eth_device *dev, volatile void *dataptr, int datasize) { - struct kwgbe_device *dkwgbe = to_dkwgbe(dev); - struct kwgbe_registers *regs = dkwgbe->regs; - struct kwgbe_txdesc *p_txdesc = dkwgbe->p_txdesc; + volatile struct kwgbe_device *dkwgbe = to_dkwgbe(dev); + volatile struct kwgbe_registers *regs = dkwgbe->regs; + volatile struct kwgbe_txdesc *p_txdesc = dkwgbe->p_txdesc;
if ((u32) dataptr & 0x07) { printf("Err..(%s) xmit dataptr not 64bit aligned\n",
-----Original Message----- From: u-boot-bounces@lists.denx.de [mailto:u-boot-bounces@lists.denx.de] On Behalf Of Simon Kagstrom Sent: Thursday, July 02, 2009 6:32 PM To: Simon Kagstrom Cc: U-Boot ML Subject: [U-Boot] [PATCH 1/3]: arm: Kirkwood: Fix compiler optimization bug for kwgbe_send
Fix compiler optimization bug for kwgbe_send
kwgbe_send/recv both have loops waiting for the hardware to set a bit. GCC 4.3.3 cleverly optimizes the send case to ... a while(1); loop. This patch makes the structure volatile to force re-loading of the transmit descriptor.
Signed-off-by: Simon Kagstrom simon.kagstrom@netinsight.net
drivers/net/kirkwood_egiga.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/kirkwood_egiga.c b/drivers/net/kirkwood_egiga.c index 3c5db19..db55d9d 100644 --- a/drivers/net/kirkwood_egiga.c +++ b/drivers/net/kirkwood_egiga.c @@ -484,9 +484,9 @@ static int kwgbe_halt(struct eth_device *dev) static int kwgbe_send(struct eth_device *dev, volatile void *dataptr, int datasize) {
- struct kwgbe_device *dkwgbe = to_dkwgbe(dev);
- struct kwgbe_registers *regs = dkwgbe->regs;
- struct kwgbe_txdesc *p_txdesc = dkwgbe->p_txdesc;
- volatile struct kwgbe_device *dkwgbe = to_dkwgbe(dev);
- volatile struct kwgbe_registers *regs = dkwgbe->regs;
- volatile struct kwgbe_txdesc *p_txdesc = dkwgbe->p_txdesc;
Only p_txdesc needed volatile, not others...
Regards .... Prafulla . .
Thanks for the comments Prafulla!
On Thu, 2 Jul 2009 21:36:19 -0700 Prafulla Wadaskar prafulla@marvell.com wrote:
- volatile struct kwgbe_device *dkwgbe = to_dkwgbe(dev);
- volatile struct kwgbe_registers *regs = dkwgbe->regs;
- volatile struct kwgbe_txdesc *p_txdesc = dkwgbe->p_txdesc;
Only p_txdesc needed volatile, not others...
Right - I just took the approach from kwgbe_recv. Here is an updated patch which only makes the transmit descriptor volatile.
// Simon -- Fix compiler optimization bug for kwgbe_send
kwgbe_send/recv both have loops waiting for the hardware to set a bit. GCC 4.3.3 cleverly optimizes the send case to ... a while(1); loop. This patch makes the structure volatile to force re-loading of the transmit descriptor.
Signed-off-by: Simon Kagstrom simon.kagstrom@netinsight.net --- drivers/net/kirkwood_egiga.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/kirkwood_egiga.c b/drivers/net/kirkwood_egiga.c index 3c5db19..b8a771f 100644 --- a/drivers/net/kirkwood_egiga.c +++ b/drivers/net/kirkwood_egiga.c @@ -486,7 +486,7 @@ static int kwgbe_send(struct eth_device *dev, volatile void *dataptr, { struct kwgbe_device *dkwgbe = to_dkwgbe(dev); struct kwgbe_registers *regs = dkwgbe->regs; - struct kwgbe_txdesc *p_txdesc = dkwgbe->p_txdesc; + volatile struct kwgbe_txdesc *p_txdesc = dkwgbe->p_txdesc;
if ((u32) dataptr & 0x07) { printf("Err..(%s) xmit dataptr not 64bit aligned\n",
Check the error summary bit for error detection
The Marvell documentation for the 88f6281 states that the error coding is only valid if the error summary and last frame bits in the transmit descriptor status field are set. This patch adds checks for these for transmit (I would get transmit errors on bootp with the current check, which I believe are spurious).
Signed-off-by: Simon Kagstrom simon.kagstrom@netinsight.net --- drivers/net/kirkwood_egiga.c | 4 +++- drivers/net/kirkwood_egiga.h | 1 + 2 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/drivers/net/kirkwood_egiga.c b/drivers/net/kirkwood_egiga.c index db55d9d..ecd3a28 100644 --- a/drivers/net/kirkwood_egiga.c +++ b/drivers/net/kirkwood_egiga.c @@ -509,7 +509,9 @@ static int kwgbe_send(struct eth_device *dev, volatile void *dataptr, */ while (p_txdesc->cmd_sts & KWGBE_BUFFER_OWNED_BY_DMA) { /* return fail if error is detected */ - if (p_txdesc->cmd_sts & (KWGBE_UR_ERROR | KWGBE_RL_ERROR)) { + if ((p_txdesc->cmd_sts & (KWGBE_ERROR_SUMMARY | KWGBE_TX_LAST_FRAME)) == + (KWGBE_ERROR_SUMMARY | KWGBE_TX_LAST_FRAME) && + p_txdesc->cmd_sts & (KWGBE_UR_ERROR | KWGBE_RL_ERROR)) { printf("Err..(%s) in xmit packet\n", __FUNCTION__); return -1; } diff --git a/drivers/net/kirkwood_egiga.h b/drivers/net/kirkwood_egiga.h index 8b67c9c..be85280 100644 --- a/drivers/net/kirkwood_egiga.h +++ b/drivers/net/kirkwood_egiga.h @@ -256,6 +256,7 @@ #define KWGBE_UR_ERROR (1 << 1) #define KWGBE_RL_ERROR (1 << 2) #define KWGBE_LLC_SNAP_FORMAT (1 << 9) +#define KWGBE_TX_LAST_FRAME (1 << 20)
/* Rx descriptors status */ #define KWGBE_CRC_ERROR 0
Dear Ben,
In message 20090702150303.53c40e14@marrow.netinsight.se Simon Kagstrom wrote:
Check the error summary bit for error detection
The Marvell documentation for the 88f6281 states that the error coding is only valid if the error summary and last frame bits in the transmit descriptor status field are set. This patch adds checks for these for transmit (I would get transmit errors on bootp with the current check, which I believe are spurious).
Signed-off-by: Simon Kagstrom simon.kagstrom@netinsight.net
drivers/net/kirkwood_egiga.c | 4 +++- drivers/net/kirkwood_egiga.h | 1 + 2 files changed, 4 insertions(+), 1 deletions(-)
Ping?
Best regards,
Wolfgang Denk
-----Original Message----- From: u-boot-bounces@lists.denx.de [mailto:u-boot-bounces@lists.denx.de] On Behalf Of Wolfgang Denk Sent: Thursday, August 06, 2009 2:37 AM To: Ben Warren Cc: U-Boot ML Subject: Re: [U-Boot] [PATCH 2/3]: arm: Kirkwood: Check the error summary bit for error detection
Dear Ben,
In message 20090702150303.53c40e14@marrow.netinsight.se Simon Kagstrom wrote:
Check the error summary bit for error detection
The Marvell documentation for the 88f6281 states that the
error coding
is only valid if the error summary and last frame bits in
the transmit
descriptor status field are set. This patch adds checks for
these for
transmit (I would get transmit errors on bootp with the
current check,
which I believe are spurious).
Signed-off-by: Simon Kagstrom simon.kagstrom@netinsight.net
drivers/net/kirkwood_egiga.c | 4 +++- drivers/net/kirkwood_egiga.h | 1 + 2 files changed, 4 insertions(+), 1 deletions(-)
Ping?
This patch is already applied Regards.. Prafulla . .
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de It's hard to make a program foolproof because fools are so ingenious. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Dear Prafulla Wadaskar,
In message 73173D32E9439E4ABB5151606C3E19E202E1215DD7@SC-VEXCH1.marvell.com you wrote:
Ping?
This patch is already applied
Ah. thanks for pointing out.
Best regards,
Wolfgang Denk
See to it that sent data is 8-byte aligned
U-boot might use non-8-byte-aligned addresses for sending data, which the kwgbe_send doesn't accept (bootp does this for me). This patch copies the data to be sent to a temporary buffer if it is non-aligned.
Signed-off-by: Simon Kagstrom simon.kagstrom@netinsight.net --- drivers/net/kirkwood_egiga.c | 26 ++++++++++++++++++++------ 1 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/drivers/net/kirkwood_egiga.c b/drivers/net/kirkwood_egiga.c index ecd3a28..1fe549c 100644 --- a/drivers/net/kirkwood_egiga.c +++ b/drivers/net/kirkwood_egiga.c @@ -481,18 +481,13 @@ static int kwgbe_halt(struct eth_device *dev) return 0; }
-static int kwgbe_send(struct eth_device *dev, volatile void *dataptr, +static int kwgbe_send_aligned(struct eth_device *dev, volatile void *dataptr, int datasize) { volatile struct kwgbe_device *dkwgbe = to_dkwgbe(dev); volatile struct kwgbe_registers *regs = dkwgbe->regs; volatile struct kwgbe_txdesc *p_txdesc = dkwgbe->p_txdesc;
- if ((u32) dataptr & 0x07) { - printf("Err..(%s) xmit dataptr not 64bit aligned\n", - __FUNCTION__); - return -1; - } p_txdesc->cmd_sts = KWGBE_ZERO_PADDING | KWGBE_GEN_CRC; p_txdesc->cmd_sts |= KWGBE_TX_FIRST_DESC | KWGBE_TX_LAST_DESC; p_txdesc->cmd_sts |= KWGBE_BUFFER_OWNED_BY_DMA; @@ -519,6 +514,25 @@ static int kwgbe_send(struct eth_device *dev, volatile void *dataptr, return 0; }
+static int kwgbe_send(struct eth_device *dev, volatile void *dataptr, + int datasize) +{ + static u8 __attribute__((aligned(8))) aligned_buf[9000]; + void *p = (void*)dataptr; + + if ((u32) dataptr & 0x07) { + if (datasize > sizeof(aligned_buf)) { + printf("Err..(%s) Non-aligned data is too large: %d bytes\n", + __FUNCTION__, datasize); + return -1; + } + memcpy(aligned_buf, p, datasize); + p = aligned_buf; + } + + return kwgbe_send_aligned(dev, p, datasize); +} + static int kwgbe_recv(struct eth_device *dev) { volatile struct kwgbe_device *dkwgbe = to_dkwgbe(dev);
Dear Simon Kagstrom,
In message 20090702150401.06ded440@marrow.netinsight.se Simon Kagstrom wrote:
See to it that sent data is 8-byte aligned
U-boot might use non-8-byte-aligned addresses for sending data, which the kwgbe_send doesn't accept (bootp does this for me). This patch copies the data to be sent to a temporary buffer if it is non-aligned.
Signed-off-by: Simon Kagstrom simon.kagstrom@netinsight.net
drivers/net/kirkwood_egiga.c | 26 ++++++++++++++++++++------ 1 files changed, 20 insertions(+), 6 deletions(-)
Ping?
Best regards,
Wolfgang Denk
-----Original Message----- From: u-boot-bounces@lists.denx.de [mailto:u-boot-bounces@lists.denx.de] On Behalf Of Wolfgang Denk Sent: Thursday, August 06, 2009 2:38 AM To: Ben Warren Cc: U-Boot ML Subject: Re: [U-Boot] [PATCH 3/3]: arm: Kirkwood: See to it that sent data is 8-byte aligned
Dear Simon Kagstrom,
In message 20090702150401.06ded440@marrow.netinsight.se Simon Kagstrom wrote:
See to it that sent data is 8-byte aligned
U-boot might use non-8-byte-aligned addresses for sending
data, which
the kwgbe_send doesn't accept (bootp does this for me). This patch copies the data to be sent to a temporary buffer if it is
non-aligned.
Ack, This is useful patch for kirkwood egiga driver, I have tested it -possible pls get this in.
On the other hand- I had a suggestion to do this in upper layer (i.e. in net/eth.c) so that every client driver can benefit from this. But we can do this latter too.
Regards.. Prafulla . .
Signed-off-by: Simon Kagstrom simon.kagstrom@netinsight.net
drivers/net/kirkwood_egiga.c | 26 ++++++++++++++++++++------ 1 files changed, 20 insertions(+), 6 deletions(-)
Ping?
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de Q: Why do mountain climbers rope themselves together? A: To prevent the sensible ones from going home. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
On Wed, 05 Aug 2009 23:08:16 +0200 Wolfgang Denk wd@denx.de wrote:
In message 20090702150401.06ded440@marrow.netinsight.se Simon Kagstrom wrote:
See to it that sent data is 8-byte aligned
U-boot might use non-8-byte-aligned addresses for sending data, which the kwgbe_send doesn't accept (bootp does this for me). This patch copies the data to be sent to a temporary buffer if it is non-aligned.
Signed-off-by: Simon Kagstrom simon.kagstrom@netinsight.net
drivers/net/kirkwood_egiga.c | 26 ++++++++++++++++++++------ 1 files changed, 20 insertions(+), 6 deletions(-)
Ping?
Ack!
I've been on vacation but I'm back now. I'll start reworking the non-accepted patches from this series according to the comments I got. The series consisted of
(not accepted) [PATCH 1/4]: arm: Kirkwood: Set MAC address during registration for kirkwood egiga (accepted, applied) [PATCH 2/4]: arm: Kirkwood: Fix compiler optimization bug for kwgbe_send (accepted, applied) [PATCH 3/4]: arm: Kirkwood: Check the error summary bit for error detection (not accepted) [PATCH 4/4]: arm: Kirkwood: See to it that sent data is 8-byte aligned
// Simon
participants (3)
-
Prafulla Wadaskar -
Simon Kagstrom -
Wolfgang Denk