[U-Boot] [PATCH 0/4]: arm: Kirkwood: Various egiga fixes
Hi again!
This is a refreshed version of the http://lists.denx.de/pipermail/u-boot/2009-July/055370.html patch series. I've added one more patch to fix an issue with the MAC address under Linux. The patches are now:
1. (new) Set MAC address during egiga registration (so that Linux gets it even if the network interface has not been initialized under U-boot).
2. Correct the optimization bug in kgwbe_send. This patch now uses IO accessor functions instead of volatile (and also corrects recv).
3. Check the error summary bit during send (as before but based on the above patches)
4. Allow sending data which is not 8-byte aligned (as before but based on the above patches)
// Simon
This patch sets the MAC address during registration in addition to during device init. Since U-boot might not access the ethernet device, Linux might end up with the MAC address unset.
Signed-off-by: Simon Kagstrom simon.kagstrom@netinsight.net --- drivers/net/kirkwood_egiga.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/net/kirkwood_egiga.c b/drivers/net/kirkwood_egiga.c index 3c5db19..d760e1d 100644 --- a/drivers/net/kirkwood_egiga.c +++ b/drivers/net/kirkwood_egiga.c @@ -653,6 +653,8 @@ int kirkwood_egiga_initialize(bd_t * bis) dev->send = (void *)kwgbe_send; dev->recv = (void *)kwgbe_recv;
+ port_uc_addr_set(dkwgbe->regs, dev->enetaddr); + eth_register(dev);
#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII)
-----Original Message----- From: Simon Kagstrom [mailto:simon.kagstrom@netinsight.net] Sent: Wednesday, July 08, 2009 4:32 PM Cc: U-Boot ML; Prafulla Wadaskar Subject: [PATCH 1/4]: arm: Kirkwood: Set MAC address during registration for kirkwood egiga
This patch sets the MAC address during registration in addition to during device init. Since U-boot might not access the ethernet device, Linux might end up with the MAC address unset.
Signed-off-by: Simon Kagstrom simon.kagstrom@netinsight.net
drivers/net/kirkwood_egiga.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/net/kirkwood_egiga.c b/drivers/net/kirkwood_egiga.c index 3c5db19..d760e1d 100644 --- a/drivers/net/kirkwood_egiga.c +++ b/drivers/net/kirkwood_egiga.c @@ -653,6 +653,8 @@ int kirkwood_egiga_initialize(bd_t * bis) dev->send = (void *)kwgbe_send; dev->recv = (void *)kwgbe_recv;
port_uc_addr_set(dkwgbe->regs, dev->enetaddr);- eth_register(dev);
#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII)
Ack. Good catch, I had removed it during code size reduction :-(
Regards.. Prafulla . .
1.6.0.4
Simon Kagstrom wrote:
This patch sets the MAC address during registration in addition to during device init. Since U-boot might not access the ethernet device, Linux might end up with the MAC address unset.
I think this violates U-boot policy of only touching hardware if it's used, but Wolfgang can say for sure. In general, initialize() functions should just set up data structures and register the device. There's a long history of MAC-address issues with ARM chips and Linux that I've happily stayed clear of so can't claim to be an expert here.
regards, Ben
On Thu, 09 Jul 2009 22:44:10 -0700 Ben Warren biggerbadderben@gmail.com wrote:
Simon Kagstrom wrote:
This patch sets the MAC address during registration in addition to during device init. Since U-boot might not access the ethernet device, Linux might end up with the MAC address unset.
I think this violates U-boot policy of only touching hardware if it's used, but Wolfgang can say for sure. In general, initialize() functions should just set up data structures and register the device. There's a long history of MAC-address issues with ARM chips and Linux that I've happily stayed clear of so can't claim to be an expert here.
OK, I've tried looking around at how other boards do it, and at least the most similar (mv6436x_eth_initialize() for other Marvell boards) do it the same way as well as some others (ax88180.c).
Should Linux otherwise read the passed U-boot environment and set it up by itself?
// Simon
-----Original Message----- From: u-boot-bounces@lists.denx.de [mailto:u-boot-bounces@lists.denx.de] On Behalf Of Simon Kagstrom Sent: Friday, July 10, 2009 12:55 PM To: Ben Warren Cc: U-Boot ML Subject: Re: [U-Boot] [PATCH 1/4]: arm: Kirkwood: Set MAC address during registration for kirkwood egiga
On Thu, 09 Jul 2009 22:44:10 -0700 Ben Warren biggerbadderben@gmail.com wrote:
Simon Kagstrom wrote:
This patch sets the MAC address during registration in
addition to
during device init. Since U-boot might not access the ethernet device, Linux might end up with the MAC address unset.
I think this violates U-boot policy of only touching
hardware if it's
used, but Wolfgang can say for sure. In general, initialize() functions should just set up data structures and register
the device.
There's a long history of MAC-address issues with ARM chips
and Linux
that I've happily stayed clear of so can't claim to be an
expert here.
OK, I've tried looking around at how other boards do it, and at least the most similar (mv6436x_eth_initialize() for other
This is a wrong reference, even this was my initial reference, but there are no updates to this code, we were planning to clean it but we do not have hardware to test it. Does any one have it?
Marvell boards) do it the same way as well as some others (ax88180.c).
If posible can be improved.
Should Linux otherwise read the passed U-boot environment and set it up by itself?
I think this is better approach, Regards.. Prafulla . .
// Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Dear Simon Kagstrom,
In message 20090710092452.492dfafe@marrow.netinsight.se you wrote:
OK, I've tried looking around at how other boards do it, and at least the most similar (mv6436x_eth_initialize() for other Marvell boards) do it the same way as well as some others (ax88180.c).
Then those slipped through the review process. Patches to clean these up are welcome.
Should Linux otherwise read the passed U-boot environment and set it up by itself?
Unfortunately ARM Linux does not provide yet a standard way to pass the MAC address from the boot loader. In the long term this problem will be solved by using the device tree for ARM; in the meantime, some drivers use a "ethaddr=" command line parameter (but that will probably bring up discussions on the Linux front again).
Best regards,
Wolfgang Denk
On Sat, 11 Jul 2009 11:27:57 +0200 Wolfgang Denk wd@denx.de wrote:
Should Linux otherwise read the passed U-boot environment and set it up by itself?
Unfortunately ARM Linux does not provide yet a standard way to pass the MAC address from the boot loader. In the long term this problem will be solved by using the device tree for ARM; in the meantime, some drivers use a "ethaddr=" command line parameter (but that will probably bring up discussions on the Linux front again).
OK, so until ARM has moved to the device tree approach (and from LKML I see there is some vocal opposition to this), we'd still need to resort to some driver-specific hack [with Linux support] for this?
Or is it OK to do some board-specific "hack" to initialize it like
board/atmel/at91rm9200ek/misc.c
does it?
Coming from a PowerPC-setting, I must say I miss the device trees though :-)
// Simon
Dear Simon Kagstrom,
In message 20090713101307.30d3d13c@marrow.netinsight.se you wrote:
OK, so until ARM has moved to the device tree approach (and from LKML I see there is some vocal opposition to this), we'd still need to resort to some driver-specific hack [with Linux support] for this?
Driver-specific hack or fix yes - but in Linux, not in U-Boot.
Or is it OK to do some board-specific "hack" to initialize it like
board/atmel/at91rm9200ek/misc.c
does it?
I this driver does something like this, then it should ne fixed.
Coming from a PowerPC-setting, I must say I miss the device trees though :-)
Me, too. Let's keep the fingers crossed that gcl makes fast progress.
Best regards,
Wolfgang Denk
Dear Ben Warren,
In message 4A56D52A.8010401@gmail.com you wrote:
Simon Kagstrom wrote:
This patch sets the MAC address during registration in addition to during device init. Since U-boot might not access the ethernet device, Linux might end up with the MAC address unset.
I think this violates U-boot policy of only touching hardware if it's used, but Wolfgang can say for sure. In general, initialize() functions should just set up data structures and register the device. There's a long history of MAC-address issues with ARM chips and Linux that I've happily stayed clear of so can't claim to be an expert here.
You are correct. Devices shall only be initialized when actually used in U-Boot (upon first use).
Best regards,
Wolfgang Denk
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 uses readl to force a read from device memory. Other volatile accesses have also been replaced with readl/writel where appropriate (as per suggestions on the U-boot mailing list).
Signed-off-by: Simon Kagstrom simon.kagstrom@netinsight.net --- drivers/net/kirkwood_egiga.c | 31 +++++++++++++++++++------------ 1 files changed, 19 insertions(+), 12 deletions(-)
diff --git a/drivers/net/kirkwood_egiga.c b/drivers/net/kirkwood_egiga.c index d760e1d..9cab7fd 100644 --- a/drivers/net/kirkwood_egiga.c +++ b/drivers/net/kirkwood_egiga.c @@ -49,7 +49,7 @@ static int smi_reg_read(char *devname, u8 phy_adr, u8 reg_ofs, u16 * data) struct kwgbe_device *dkwgbe = to_dkwgbe(dev); struct kwgbe_registers *regs = dkwgbe->regs; u32 smi_reg; - volatile u32 timeout; + u32 timeout;
/* Phyadr read request */ if (phy_adr == 0xEE && reg_ofs == 0xEE) { @@ -124,7 +124,7 @@ static int smi_reg_write(char *devname, u8 phy_adr, u8 reg_ofs, u16 data) struct kwgbe_device *dkwgbe = to_dkwgbe(dev); struct kwgbe_registers *regs = dkwgbe->regs; u32 smi_reg; - volatile u32 timeout; + u32 timeout;
/* Phyadr write request*/ if (phy_adr == 0xEE && reg_ofs == 0xEE) { @@ -370,7 +370,7 @@ static void port_uc_addr_set(struct kwgbe_registers *regs, u8 * p_addr) */ static void kwgbe_init_rx_desc_ring(struct kwgbe_device *dkwgbe) { - volatile struct kwgbe_rxdesc *p_rx_desc; + struct kwgbe_rxdesc *p_rx_desc; int i;
/* initialize the Rx descriptors ring */ @@ -487,6 +487,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; + u32 cmd_sts;
if ((u32) dataptr & 0x07) { printf("Err..(%s) xmit dataptr not 64bit aligned\n", @@ -507,21 +508,24 @@ static int kwgbe_send(struct eth_device *dev, volatile void *dataptr, /* * wait for packet xmit completion */ - while (p_txdesc->cmd_sts & KWGBE_BUFFER_OWNED_BY_DMA) { + cmd_sts = readl(&p_txdesc->cmd_sts); + while (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 (cmd_sts & (KWGBE_UR_ERROR | KWGBE_RL_ERROR)) { printf("Err..(%s) in xmit packet\n", __FUNCTION__); return -1; } + cmd_sts = readl(&p_txdesc->cmd_sts); }; return 0; }
static int kwgbe_recv(struct eth_device *dev) { - volatile struct kwgbe_device *dkwgbe = to_dkwgbe(dev); - volatile struct kwgbe_rxdesc *p_rxdesc_curr = dkwgbe->p_rxdesc_curr; - volatile u32 timeout = 0; + struct kwgbe_device *dkwgbe = to_dkwgbe(dev); + struct kwgbe_rxdesc *p_rxdesc_curr = dkwgbe->p_rxdesc_curr; + u32 cmd_sts; + u32 timeout = 0;
/* wait untill rx packet available or timeout */ do { @@ -531,7 +535,7 @@ static int kwgbe_recv(struct eth_device *dev) debug("%s time out...\n", __FUNCTION__); return -1; } - } while (p_rxdesc_curr->cmd_sts & KWGBE_BUFFER_OWNED_BY_DMA); + } while (readl(&p_rxdesc_curr->cmd_sts) & KWGBE_BUFFER_OWNED_BY_DMA);
if (p_rxdesc_curr->byte_cnt != 0) { debug("%s: Received %d byte Packet @ 0x%x (cmd_sts= %08x)\n", @@ -545,14 +549,16 @@ static int kwgbe_recv(struct eth_device *dev) * OR the error summary bit is on, * the packets needs to be dropeed. */ - if ((p_rxdesc_curr->cmd_sts & + cmd_sts = readl(&p_rxdesc_curr->cmd_sts); + + if ((cmd_sts & (KWGBE_RX_FIRST_DESC | KWGBE_RX_LAST_DESC)) != (KWGBE_RX_FIRST_DESC | KWGBE_RX_LAST_DESC)) {
printf("Err..(%s) Dropping packet spread on" " multiple descriptors\n", __FUNCTION__);
- } else if (p_rxdesc_curr->cmd_sts & KWGBE_ERROR_SUMMARY) { + } else if (cmd_sts & KWGBE_ERROR_SUMMARY) {
printf("Err..(%s) Dropping packet with errors\n", __FUNCTION__); @@ -574,7 +580,8 @@ static int kwgbe_recv(struct eth_device *dev) p_rxdesc_curr->buf_size = PKTSIZE_ALIGN; p_rxdesc_curr->byte_cnt = 0;
- dkwgbe->p_rxdesc_curr = p_rxdesc_curr->nxtdesc_p; + writel((unsigned)p_rxdesc_curr->nxtdesc_p, &dkwgbe->p_rxdesc_curr); + return 0; }
Simon Kagstrom wrote:
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 uses readl to force a read from device memory. Other volatile accesses have also been replaced with readl/writel where appropriate (as per suggestions on the U-boot mailing list).
Signed-off-by: Simon Kagstrom simon.kagstrom@netinsight.net
drivers/net/kirkwood_egiga.c | 31 +++++++++++++++++++------------ 1 files changed, 19 insertions(+), 12 deletions(-)
Applied to net repo.
thanks,
Simon Kagstrom wrote:
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 uses readl to force a read from device memory. Other volatile accesses have also been replaced with readl/writel where appropriate (as per suggestions on the U-boot mailing list).
Signed-off-by: Simon Kagstrom simon.kagstrom@netinsight.net
Applied to net repo.
thanks, Ben
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 9cab7fd..537343f 100644 --- a/drivers/net/kirkwood_egiga.c +++ b/drivers/net/kirkwood_egiga.c @@ -511,7 +511,9 @@ static int kwgbe_send(struct eth_device *dev, volatile void *dataptr, cmd_sts = readl(&p_txdesc->cmd_sts); while (cmd_sts & KWGBE_BUFFER_OWNED_BY_DMA) { /* return fail if error is detected */ - if (cmd_sts & (KWGBE_UR_ERROR | KWGBE_RL_ERROR)) { + if ((cmd_sts & (KWGBE_ERROR_SUMMARY | KWGBE_TX_LAST_FRAME)) == + (KWGBE_ERROR_SUMMARY | KWGBE_TX_LAST_FRAME) && + 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..9c893d1 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
Simon Kagstrom wrote:
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
Applied to net repo.
thanks, Ben
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 537343f..24269c1 100644 --- a/drivers/net/kirkwood_egiga.c +++ b/drivers/net/kirkwood_egiga.c @@ -481,7 +481,7 @@ 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) { struct kwgbe_device *dkwgbe = to_dkwgbe(dev); @@ -489,11 +489,6 @@ static int kwgbe_send(struct eth_device *dev, volatile void *dataptr, struct kwgbe_txdesc *p_txdesc = dkwgbe->p_txdesc; u32 cmd_sts;
- 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; @@ -522,6 +517,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 too large (%d)\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) { struct kwgbe_device *dkwgbe = to_dkwgbe(dev);
-----Original Message----- From: Simon Kagstrom [mailto:simon.kagstrom@netinsight.net] Sent: Wednesday, July 08, 2009 4:36 PM To: U-Boot ML; Prafulla Wadaskar Subject: [PATCH 4/4]: arm: Kirkwood: 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.
Kirkwood egiga driver may not be the only client for this kind of requirements. Any Ethernet controller using DMA will require aligned data pointer. I would suggest either upper layers should pass the aligned buffer pointers Or this patch should go in net/ethic, so that other drivers can benefit
Regards.. Prafulla . .
On Wed, 8 Jul 2009 05:17:08 -0700 Prafulla Wadaskar prafulla@marvell.com wrote:
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.
Kirkwood egiga driver may not be the only client for this kind of requirements. Any Ethernet controller using DMA will require aligned data pointer. I would suggest either upper layers should pass the aligned buffer pointers Or this patch should go in net/ethic, so that other drivers can benefit
Well, I've looked around a bit and other drivers also handle it this way - see greth.c for example. I agree that it would be nice to handle it at an upper layer, but that would require a good code review to find the potential places where this problem could occur. I feel I'm a bit too U-boot green to do that as of now :-)
I'll submit a new patch with this malloc'ed though.
// Simon
On 14:44 Wed 08 Jul , Simon Kagstrom wrote:
On Wed, 8 Jul 2009 05:17:08 -0700 Prafulla Wadaskar prafulla@marvell.com wrote:
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.
Kirkwood egiga driver may not be the only client for this kind of requirements. Any Ethernet controller using DMA will require aligned data pointer. I would suggest either upper layers should pass the aligned buffer pointers Or this patch should go in net/ethic, so that other drivers can benefit
Well, I've looked around a bit and other drivers also handle it this way - see greth.c for example. I agree that it would be nice to handle it at an upper layer, but that would require a good code review to find the potential places where this problem could occur. I feel I'm a bit too U-boot green to do that as of now :-)
I'll submit a new patch with this malloc'ed though.
I agree with Prafulla Ben what do you think?
Best Regards, J.
On Wednesday 08 July 2009 13:05:42 Simon Kagstrom wrote:
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 537343f..24269c1 100644 --- a/drivers/net/kirkwood_egiga.c +++ b/drivers/net/kirkwood_egiga.c @@ -481,7 +481,7 @@ 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) { struct kwgbe_device *dkwgbe = to_dkwgbe(dev); @@ -489,11 +489,6 @@ static int kwgbe_send(struct eth_device *dev, volatile void *dataptr, struct kwgbe_txdesc *p_txdesc = dkwgbe->p_txdesc; u32 cmd_sts;
- 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;
@@ -522,6 +517,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];
I would prefer to malloc such big area's.
And I second Prafulla's comment, that this should be handled by the upper network layer.
Thanks.
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de =====================================================================
I would prefer to malloc such big area's.
And I second Prafulla's comment, that this should be handled by the upper network layer.
Here is a second version which uses malloc instead. It's still handled in the driver though.
// Simon -- From d8555440b033ac32eecd8973945936fb8c9f0421 Mon Sep 17 00:00:00 2001 From: Simon Kagstrom simon.kagstrom@netinsight.net Date: Thu, 2 Jul 2009 14:33:04 +0200 Subject: [PATCH] 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 malloced 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, 22 insertions(+), 4 deletions(-)
diff --git a/drivers/net/kirkwood_egiga.c b/drivers/net/kirkwood_egiga.c index 537343f..a9acc9d 100644 --- a/drivers/net/kirkwood_egiga.c +++ b/drivers/net/kirkwood_egiga.c @@ -481,24 +481,42 @@ static int kwgbe_halt(struct eth_device *dev) return 0; }
+#define KWGBE_SEND_BUF_SIZE 9000 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; + void *p = (void *)dataptr; u32 cmd_sts;
+ /* Copy buffer if it's misaligned */ if ((u32) dataptr & 0x07) { - printf("Err..(%s) xmit dataptr not 64bit aligned\n", - __FUNCTION__); - return -1; + static void *aligned_buf; + + if (!aligned_buf) + aligned_buf = memalign(sizeof(u32), + KWGBE_SEND_BUF_SIZE); + if (!aligned_buf) { + printf("Err...(%s): Cannot allocate aligned buffer\n", + __FUNCTION__); + return -1; + } + if (datasize > KWGBE_SEND_BUF_SIZE) { + printf("Err..(%s) Non-aligned data too large (%d)\n", + __FUNCTION__, datasize); + return -1; + } + memcpy(aligned_buf, p, datasize); + p = aligned_buf; } + 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; p_txdesc->cmd_sts |= KWGBE_TX_EN_INTERRUPT; - p_txdesc->buf_ptr = (u8 *) dataptr; + p_txdesc->buf_ptr = (u8 *) p; p_txdesc->byte_cnt = datasize;
/* Apply send command using zeroth RXUQ */
Simon,
Simon Kagstrom wrote:
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 537343f..24269c1 100644 --- a/drivers/net/kirkwood_egiga.c +++ b/drivers/net/kirkwood_egiga.c @@ -481,7 +481,7 @@ 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) { struct kwgbe_device *dkwgbe = to_dkwgbe(dev); @@ -489,11 +489,6 @@ static int kwgbe_send(struct eth_device *dev, volatile void *dataptr, struct kwgbe_txdesc *p_txdesc = dkwgbe->p_txdesc; u32 cmd_sts;
- 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;
@@ -522,6 +517,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];
Why do you need to send a jumbo frame? U-boot is hard-coded in some ways to only handle frames with an MTU of 1518 bytes on Rx.
regards, Ben
participants (6)
-
Ben Warren -
Jean-Christophe PLAGNIOL-VILLARD -
Prafulla Wadaskar -
Simon Kagstrom -
Stefan Roese -
Wolfgang Denk