[U-Boot] [PATCH 0/4]: arm: Kirkwood: Various egiga fixes

[U-Boot] [PATCH 0/4]: arm: Kirkwood: Various egiga fixes

[Simon Kagstrom]

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

[U-Boot] [PATCH 1/4]: arm: Kirkwood: Set MAC address during registration for kirkwood egiga

[Simon Kagstrom]

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)
-- 
1.6.0.4

[U-Boot] [PATCH 1/4]: arm: Kirkwood: Set MAC address during registration for kirkwood egiga

[Prafulla Wadaskar]

 

> -----Original Message-----
> From: Simon Kagstrom [mailto:simon.kagstrom at 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
> 
> 

[U-Boot] [PATCH 1/4]: arm: Kirkwood: Set MAC address during registration for kirkwood egiga

[Ben Warren]

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

[U-Boot] [PATCH 1/4]: arm: Kirkwood: Set MAC address during registration for kirkwood egiga

[Simon Kagstrom]

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

[U-Boot] [PATCH 1/4]: arm: Kirkwood: Set MAC address during registration for kirkwood egiga

[Prafulla Wadaskar]

 

> -----Original Message-----
> From: u-boot-bounces at lists.denx.de 
> [mailto:u-boot-bounces at 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 at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
> 

[U-Boot] [PATCH 1/4]: arm: Kirkwood: Set MAC address during registration for kirkwood egiga

[Wolfgang Denk]

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

-- 
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 at denx.de
Few people do business well who do nothing else.
                                       -- Philip Earl of Chesterfield

[U-Boot] [PATCH 1/4]: arm: Kirkwood: Set MAC address during registration for kirkwood egiga

[Simon Kagstrom]

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

[U-Boot] [PATCH 1/4]: arm: Kirkwood: Set MAC address during registration for kirkwood egiga

[Wolfgang Denk]

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

-- 
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 at denx.de
"I didn't know it was impossible when I did it."

[U-Boot] [PATCH 1/4]: arm: Kirkwood: Set MAC address during registration for kirkwood egiga

[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

-- 
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 at denx.de
There is an order of things in this universe.
	-- Apollo, "Who Mourns for Adonais?" stardate 3468.1

[U-Boot] [PATCH 2/4]: arm: Kirkwood: Fix compiler optimization bug for kwgbe_send

[Simon Kagstrom]

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;
 }
 
-- 
1.6.0.4

[U-Boot] [PATCH 2/4]: arm: Kirkwood: Fix compiler optimization bug for kwgbe_send

[Ben Warren]

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,

[U-Boot] [PATCH 2/4]: arm: Kirkwood: Fix compiler optimization bug for kwgbe_send

[Ben Warren]

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

[U-Boot] [PATCH 3/4]: arm: Kirkwood: Check the error summary bit for error detection

[Simon Kagstrom]

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
-- 
1.6.0.4

[U-Boot] [PATCH 3/4]: arm: Kirkwood: Check the error summary bit for error detection

[Ben Warren]

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] [PATCH 4/4]: arm: Kirkwood: See to it that sent data is 8-byte aligned

[Simon Kagstrom]

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);
-- 
1.6.0.4

[U-Boot] [PATCH 4/4]: arm: Kirkwood: See to it that sent data is 8-byte aligned

[Prafulla Wadaskar]

 

> -----Original Message-----
> From: Simon Kagstrom [mailto:simon.kagstrom at 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 . .

[U-Boot] [PATCH 4/4]: arm: Kirkwood: See to it that sent data is 8-byte aligned

[Simon Kagstrom]

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

[U-Boot] [PATCH 4/4]: arm: Kirkwood: See to it that sent data is 8-byte aligned

[Jean-Christophe PLAGNIOL-VILLARD]

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.

[U-Boot] [PATCH 4/4]: arm: Kirkwood: See to it that sent data is 8-byte aligned

[Stefan Roese]

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 at denx.de
=====================================================================

[U-Boot] [PATCH 4/4]: arm: Kirkwood: See to it that sent data is 8-byte aligned

[Simon Kagstrom]

> 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
--

[U-Boot] [PATCH 4/4]: arm: Kirkwood: See to it that sent data is 8-byte aligned

[Ben Warren]

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