arm:kirkwood See to it that sent data is 8-byte aligned

PATCH New Simon Kagstrom 3 participants 6 messages Aug 18, 2009 – Aug 20, 2009 Network drivers

lore

Share & tools

lore

Simon Kagstrom

author

2009-08-18 09:32 UTC #0 lore raw

PATCH 1/1 arm:kirkwood See to it that sent data is 8-byte aligned

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.

v2: Malloc send buffer (comment from Stefan Roese)
v3: No need to use jumbo frames, use 1518 bytes buffer instead
(comment from Ben Warren)

Signed-off-by Simon Kagstrom <[email protected]>

---
 drivers/net/kirkwood_egiga.c |   26 ++++++++++++++++++++++----
 1 files changed, 22 insertions(+), 4 deletions(-)

drivers/net/kirkwood_egiga.c +22 -4

index f31fefc..6627412 100644
@@ -481,24 +481,42 @@ static int kwgbe_halt(struct eth_device *dev)
 	return 0;
 }
 
+#define KWGBE_SEND_BUF_SIZE 1518
 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 */

Signature · 1 line

1.6.0.4

Prafulla Wadaskar

2009-08-18 10:12 UTC #1 lore raw

PATCH 1/1 arm:kirkwood See to it that sent data is 8-byte aligned

Quoted 10 lines

-----Original Message-----
From: u-boot-bounces at lists.denx.de [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Simon Kagstrom
Sent: Tuesday, August 18, 2009 3:02 PM
To: U-Boot ML
Subject: [U-Boot] [PATCH] arm:kirkwood See to it that sent data is 8-byte aligned

Quoted 8 lines

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.
v2: Malloc send buffer (comment from Stefan Roese)

Malloc will always be an overhead.
I strongly recommend- we should pass aligned buffers from upper layers to avoid such rework in all low level drivers, (few are already aligned).

Quoted 1 line

v3: No need to use jumbo frames, use 1518 bytes buffer instead

Better to use PKTSIZE_ALIGN here, avoid magic numbers

Quoted 13 lines

(comment from Ben Warren)
Signed-off-by Simon Kagstrom <[email protected]>
---
 drivers/net/kirkwood_egiga.c |   26 ++++++++++++++++++++++----
 1 files changed, 22 insertions(+), 4 deletions(-)
drivers/net/kirkwood_egiga.c +0 -0
index f31fefc..6627412 100644

drivers/net/kirkwood_egiga.c static int kwgbe_halt(struct eth_device *dev) · line 484

Quoted 4 lines

drivers/net/kirkwood_egiga.c +1 -0

@@ -481,24 +481,42 @@ static int kwgbe_halt(struct eth_device *dev)
 	return 0;
 }
 
+#define KWGBE_SEND_BUF_SIZE 1518

Better to use PKTSIZE_ALIGN here, avoid magic numbers Regards..
Prafulla . .

Simon Kagstrom

author

2009-08-18 10:48 UTC #2 lore raw

PATCH 1/1 arm:kirkwood See to it that sent data is 8-byte aligned

Thanks for the review Prafulla!

Quoted 2 lines · On Tue, 18 Aug 2009 03:12:07 -0700 Prafulla Wadaskar <[email protected]>…

Quoted 1 line
v2: Malloc send buffer (comment from Stefan Roese)
Malloc will always be an overhead.

It's only allocated once (the first time a non-aligned buffer is passed), so the overhead is minimal.

Quoted 3 lines

I strongly recommend- we should pass aligned buffers from upper layers to avoid such rework in all low level drivers, (few are already aligned).

We could put the same fix in eth_send instead. Then the issue is really just how we know what alignment requirement to go for. I guess one could add a field to the eth_device structure to store this and then fixup all drivers to supply this.

If the rest of you thinks this is a good idea, I can cook up a patch. Opinions?

Quoted 2 lines

Quoted 1 line
v3: No need to use jumbo frames, use 1518 bytes buffer instead
Better to use PKTSIZE_ALIGN here, avoid magic numbers

OK, I'll fix that.

Thanks,
// Simon

Jean-Christophe PLAGNIOL-VILLARD

2009-08-19 22:36 UTC #3 lore raw

PATCH 1/1 arm:kirkwood See to it that sent data is 8-byte aligned

Quoted 15 lines · Simon Kagstrom

Thanks for the review Prafulla!
Quoted 2 lines · On Tue, 18 Aug 2009 03:12:07 -0700 Prafulla Wadaskar <[email protected]>…
Quoted 1 line
v2: Malloc send buffer (comment from Stefan Roese)
Malloc will always be an overhead.
It's only allocated once (the first time a non-aligned buffer is passed), so the overhead is minimal.
Quoted 3 lines
I strongly recommend- we should pass aligned buffers from upper layers to avoid such rework in all low level drivers, (few are already aligned).

Quoted 7 lines

We could put the same fix in eth_send instead. Then the issue is really just how we know what alignment requirement to go for. I guess one could add a field to the eth_device structure to store this and then fixup all drivers to supply this.
If the rest of you thinks this is a good idea, I can cook up a patch. Opinions?

some drivers would also have the possibility to have multiple receive buffer specially when you use dma
so IMHO it make more sense to let each drivers handle it itself

Best Regards,
J.

Prafulla Wadaskar

2009-08-20 08:37 UTC #4 lore raw

PATCH 1/1 arm:kirkwood See to it that sent data is 8-byte aligned

Quoted 33 lines

-----Original Message-----
From: Jean-Christophe PLAGNIOL-VILLARD [mailto:plagnioj at jcrosoft.com]
Sent: Thursday, August 20, 2009 4:07 AM
To: Simon Kagstrom
Cc: Prafulla Wadaskar; U-Boot ML; Prabhanjan Sarnaik; Ashish Karkare
Subject: Re: [U-Boot] [PATCH] arm:kirkwood See to it that sent data is 8-byte aligned
Quoted 12 lines · Simon Kagstrom
Thanks for the review Prafulla!
Quoted 2 lines · On Tue, 18 Aug 2009 03:12:07 -0700 Prafulla Wadaskar <[email protected]>…
Quoted 1 line
v2: Malloc send buffer (comment from Stefan Roese)
Malloc will always be an overhead.
It's only allocated once (the first time a non-aligned buffer is passed), so the overhead is minimal.
Quoted 1 line
I strongly recommend- we should pass aligned buffers from
upper layers
Quoted 4 lines
Quoted 2 lines
to avoid such rework in all low level drivers, (few are already aligned).
We could put the same fix in eth_send instead. Then the
issue is really
Quoted 4 lines
just how we know what alignment requirement to go for. I guess one could add a field to the eth_device structure to store this and then fixup all drivers to supply this.

Quoted 7 lines

Quoted 1 line
If the rest of you thinks this is a good idea, I can cook
up a patch.
Quoted 1 line
Opinions?
some drivers would also have the possibility to have multiple receive buffer
specially when you use dma
so IMHO it make more sense to let each drivers handle it itself

In that case it makes more sense to implement this even for rx path too in upper layer. We should always push common functionalities to upper layers
And this is very generic need for any driver that uses dma for transfers

Regards..
Prafulla . .

Quoted 4 lines

Best Regards,
J.

Simon Kagstrom

author

2009-08-19 07:54 UTC #5 lore raw

PATCH 1/1 arm:kirkwood See to it that sent data is 8-byte aligned

In reply to Simon Kagstrom

drivers/net/kirkwood_egiga.c static int kwgbe_halt(struct eth_device *dev) · line 500

Quoted 10 lines · On Tue, 18 Aug 2009 11:32:06 +0200 Simon Kagstrom…

drivers/net/kirkwood_egiga.c +6 -3

+	/* 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);

The memalign requests the wrong alignment here. I'll send a new patch unless the consensus is to prefer the other variant ("[PATCH] net: See to it that sent data is aligned to the ethernet controllers wishes").

// Simon