[U-Boot] [PATCH 0/3]: arm:Kirkwood network driver fixes

[U-Boot] [PATCH 0/3]: arm:Kirkwood network driver fixes

[Simon Kagstrom]

Hi!

Three patches to fix various network driver issues on kirkwood. Patch 3
is a repost of the patch sent earlier today which is rebased on top of
the other two.

// Simon

[U-Boot] [PATCH 1/3]: arm:kirkwood Define kirkwood phy address magic number

[Simon Kagstrom]

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
---
 drivers/net/kirkwood_egiga.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/net/kirkwood_egiga.c b/drivers/net/kirkwood_egiga.c
index f31fefc..065e335 100644
--- a/drivers/net/kirkwood_egiga.c
+++ b/drivers/net/kirkwood_egiga.c
@@ -38,6 +38,8 @@
 #include <asm/arch/kirkwood.h>
 #include "kirkwood_egiga.h"
 
+#define KIRKWOOD_PHY_ADR_REQUEST 0xee
+
 /*
  * smi_reg_read - miiphy_read callback function.
  *
@@ -52,7 +54,8 @@ static int smi_reg_read(char *devname, u8 phy_adr, u8 reg_ofs, u16 * data)
 	u32 timeout;
 
 	/* Phyadr read request */
-	if (phy_adr == 0xEE && reg_ofs == 0xEE) {
+	if (phy_adr == KIRKWOOD_PHY_ADR_REQUEST &&
+			reg_ofs == KIRKWOOD_PHY_ADR_REQUEST) {
 		/* */
 		*data = (u16) (KWGBEREG_RD(regs->phyadr) & PHYADR_MASK);
 		return 0;
@@ -127,7 +130,8 @@ static int smi_reg_write(char *devname, u8 phy_adr, u8 reg_ofs, u16 data)
 	u32 timeout;
 
 	/* Phyadr write request*/
-	if (phy_adr == 0xEE && reg_ofs == 0xEE) {
+	if (phy_adr == KIRKWOOD_PHY_ADR_REQUEST &&
+			reg_ofs == KIRKWOOD_PHY_ADR_REQUEST) {
 		KWGBEREG_WR(regs->phyadr, data);
 		return 0;
 	}
@@ -444,7 +448,8 @@ static int kwgbe_init(struct eth_device *dev)
 #if (defined (CONFIG_MII) || defined (CONFIG_CMD_MII)) \
 	 && defined (CONFIG_SYS_FAULT_ECHO_LINK_DOWN)
 	u16 phyadr;
-	miiphy_read(dev->name, 0xEE, 0xEE, &phyadr);
+	miiphy_read(dev->name, KIRKWOOD_PHY_ADR_REQUEST,
+			KIRKWOOD_PHY_ADR_REQUEST, &phyadr);
 	if (!miiphy_link(dev->name, phyadr)) {
 		printf("%s: No link on %s\n", __FUNCTION__, dev->name);
 		return -1;
@@ -670,7 +675,8 @@ int kirkwood_egiga_initialize(bd_t * bis)
 #if defined(CONFIG_MII) || defined(CONFIG_CMD_MII)
 		miiphy_register(dev->name, smi_reg_read, smi_reg_write);
 		/* Set phy address of the port */
-		miiphy_write(dev->name, 0xEE, 0xEE, PHY_BASE_ADR + devnum);
+		miiphy_write(dev->name, KIRKWOOD_PHY_ADR_REQUEST,
+				KIRKWOOD_PHY_ADR_REQUEST, PHY_BASE_ADR + devnum);
 #endif
 	}
 	return 0;
-- 
1.6.0.4

[U-Boot] [PATCH 2/3]: Wait for the link to come up on kirkwood network init

[Simon Kagstrom]

This patch makes the device wait for up to 5 seconds for the link to
come up, similar to what many of the other network drivers do. This
avoids confusing situations where, e.g., a tftp fails when initiated
early after U-boot has started (before the link has come up).

v2: Remove function name from printout

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
---
 drivers/net/kirkwood_egiga.c |   20 ++++++++++++++------
 1 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/net/kirkwood_egiga.c b/drivers/net/kirkwood_egiga.c
index 065e335..9f36633 100644
--- a/drivers/net/kirkwood_egiga.c
+++ b/drivers/net/kirkwood_egiga.c
@@ -400,6 +400,7 @@ static int kwgbe_init(struct eth_device *dev)
 {
 	struct kwgbe_device *dkwgbe = to_dkwgbe(dev);
 	struct kwgbe_registers *regs = dkwgbe->regs;
+	int i;
 
 	/* setup RX rings */
 	kwgbe_init_rx_desc_ring(dkwgbe);
@@ -447,13 +448,20 @@ static int kwgbe_init(struct eth_device *dev)
 
 #if (defined (CONFIG_MII) || defined (CONFIG_CMD_MII)) \
 	 && defined (CONFIG_SYS_FAULT_ECHO_LINK_DOWN)
-	u16 phyadr;
-	miiphy_read(dev->name, KIRKWOOD_PHY_ADR_REQUEST,
-			KIRKWOOD_PHY_ADR_REQUEST, &phyadr);
-	if (!miiphy_link(dev->name, phyadr)) {
-		printf("%s: No link on %s\n", __FUNCTION__, dev->name);
-		return -1;
+	/* Wait up to 5s for the link status */
+	for (i = 0; i < 5; i++) {
+		u16 phyadr;
+
+		miiphy_read(dev->name, KIRKWOOD_PHY_ADR_REQUEST,
+				KIRKWOOD_PHY_ADR_REQUEST, &phyadr);
+		/* Return if we get link up */
+		if (miiphy_link(dev->name, phyadr))
+			return 0;
+		udelay(1000000);
 	}
+
+	printf("No link on %s\n", dev->name);
+	return -1;
 #endif
 	return 0;
 }
-- 
1.6.0.4

[U-Boot] [PATCH 3/3] [repost]: 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 malloced temporary buffer if it is
non-aligned.

v2: Malloc send buffer
v3: No need to use jumbo frames, use 1518 bytes buffer instead
v4: Correct alignment passed to memalign (should be 8!),
    allocate buffer at initialization(), use PKTSIZE_ALIGN

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
---
 drivers/net/kirkwood_egiga.c |   21 +++++++++++++++++----
 drivers/net/kirkwood_egiga.h |    1 +
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/net/kirkwood_egiga.c b/drivers/net/kirkwood_egiga.c
index 9f36633..479035d 100644
--- a/drivers/net/kirkwood_egiga.c
+++ b/drivers/net/kirkwood_egiga.c
@@ -500,18 +500,26 @@ 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;
+	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;
+		if (datasize > PKTSIZE_ALIGN) {
+			printf("Non-aligned data too large (%d)\n",
+					datasize);
+			return -1;
+		}
+
+		memcpy(dkwgbe->p_aligned_txbuf, p, datasize);
+		p = dkwgbe->p_aligned_txbuf;
 	}
+
 	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 */
@@ -628,8 +636,13 @@ int kirkwood_egiga_initialize(bd_t * bis)
 							* PKTSIZE_ALIGN + 1)))
 			goto error3;
 
+		if (!(dkwgbe->p_aligned_txbuf = memalign(8, PKTSIZE_ALIGN)))
+			goto error4;
+
 		if (!(dkwgbe->p_txdesc = (struct kwgbe_txdesc *)
 		      memalign(PKTALIGN, sizeof(struct kwgbe_txdesc) + 1))) {
+			free(dkwgbe->p_aligned_txbuf);
+		      error4:
 			free(dkwgbe->p_rxbuf);
 		      error3:
 			free(dkwgbe->p_rxdesc);
diff --git a/drivers/net/kirkwood_egiga.h b/drivers/net/kirkwood_egiga.h
index 9c893d1..16d5214 100644
--- a/drivers/net/kirkwood_egiga.h
+++ b/drivers/net/kirkwood_egiga.h
@@ -499,6 +499,7 @@ struct kwgbe_device {
 	struct kwgbe_rxdesc *p_rxdesc;
 	struct kwgbe_rxdesc *p_rxdesc_curr;
 	u8 *p_rxbuf;
+	u8 *p_aligned_txbuf;
 };
 
 #endif /* __EGIGA_H__ */
-- 
1.6.0.4

[U-Boot] [PATCH 3/3] [repost]: arm: kirkwood: See to it that sent data is 8-byte aligned

[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: Thursday, August 20, 2009 1:44 PM
> To: U-Boot ML
> Subject: [U-Boot] [PATCH 3/3] [repost]: 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 malloced temporary buffer if it is
> non-aligned.
<snip...>
> diff --git a/drivers/net/kirkwood_egiga.h 
> b/drivers/net/kirkwood_egiga.h
> index 9c893d1..16d5214 100644
> --- a/drivers/net/kirkwood_egiga.h
> +++ b/drivers/net/kirkwood_egiga.h
> @@ -499,6 +499,7 @@ struct kwgbe_device {
>  	struct kwgbe_rxdesc *p_rxdesc;
>  	struct kwgbe_rxdesc *p_rxdesc_curr;
>  	u8 *p_rxbuf;
> +	u8 *p_aligned_txbuf;
>  };
>  
>  #endif /* __EGIGA_H__ */
> -- 
> 1.6.0.4
>
Ack,
Technically this patch is okay,
Unless we all agree this to be done in low level drivers :-)

Regards..
Prafulla . .

> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
> 

[U-Boot] [PATCH 1/3]: arm:kirkwood Define kirkwood phy address magic number

[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: Thursday, August 20, 2009 1:42 PM
> Cc: U-Boot ML
> Subject: [U-Boot] [PATCH 1/3]: arm:kirkwood Define kirkwood 
> phy address magic number
> 
> Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
> ---
>  drivers/net/kirkwood_egiga.c |   14 ++++++++++----
>  1 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/kirkwood_egiga.c 
> b/drivers/net/kirkwood_egiga.c
> index f31fefc..065e335 100644
> --- a/drivers/net/kirkwood_egiga.c
> +++ b/drivers/net/kirkwood_egiga.c
> @@ -38,6 +38,8 @@
>  #include <asm/arch/kirkwood.h>
>  #include "kirkwood_egiga.h"
>  
> +#define KIRKWOOD_PHY_ADR_REQUEST 0xee
define this in header file

Basically this is needed in drivers/net/phy/mv88e61xx.c for multi chip support
in this case we need to define this in include/miiphy.h.
which conflicts with other phy address definition, that's why not done earlier

It makes more sense to add APIs miiphy_read/write_phyadr to miiutils

Regards..
Prafulla . .

[U-Boot] [PATCH 1/3]: arm:kirkwood Define kirkwood phy address magic number

[Simon Kagstrom]

On Thu, 20 Aug 2009 02:40:48 -0700
Prafulla Wadaskar <prafulla@marvell.com> wrote:

> >  
> > +#define KIRKWOOD_PHY_ADR_REQUEST 0xee
> define this in header file
> 
> Basically this is needed in drivers/net/phy/mv88e61xx.c for multi chip support
> in this case we need to define this in include/miiphy.h.
> which conflicts with other phy address definition, that's why not done earlier
> 
> It makes more sense to add APIs miiphy_read/write_phyadr to miiutils

But is this really general functionality? miiphy.h is something I
suppose should be generic between phys and not contain device-specific
things like this.

Perhaps we should revive the patches that move out the phy
initialization from sheevaplug.c and place the define and some
implementation there?

// Simon

[U-Boot] [PATCH 1/3]: arm:kirkwood Define kirkwood phy address magic number

[Prafulla Wadaskar]

 

> -----Original Message-----
> From: Simon Kagstrom [mailto:simon.kagstrom at netinsight.net] 
> Sent: Thursday, August 20, 2009 5:17 PM
> To: Prafulla Wadaskar
> Cc: U-Boot ML; Ashish Karkare; Prabhanjan Sarnaik
> Subject: Re: [U-Boot] [PATCH 1/3]: arm:kirkwood Define 
> kirkwood phy address magic number
> 
> On Thu, 20 Aug 2009 02:40:48 -0700
> Prafulla Wadaskar <prafulla@marvell.com> wrote:
> 
> > >  
> > > +#define KIRKWOOD_PHY_ADR_REQUEST 0xee
> > define this in header file
> > 
> > Basically this is needed in drivers/net/phy/mv88e61xx.c for 
> multi chip support
> > in this case we need to define this in include/miiphy.h.
> > which conflicts with other phy address definition, that's 
> why not done earlier
> > 
> > It makes more sense to add APIs miiphy_read/write_phyadr to miiutils
> 
> But is this really general functionality? miiphy.h is something I
> suppose should be generic between phys and not contain device-specific
> things like this.
Agreed,
Current PHY support need to be re-architected to support switches in generic ways
This is old pending issue, may be we should re-trigger it again after scheduled release

> 
> Perhaps we should revive the patches that move out the phy
> initialization from sheevaplug.c and place the define and some
> implementation there?
Sure
the code can be abstracted from sheevaplug as well as rd6281a for MV88E1116 initialization.
In the beginning I had posted my first patch as separate driver for MV88E1116, but again this is gated due to above rework to be done

Regards..
Prafulla . .

> 
> // Simon
> 

[U-Boot] [PATCH 1/3]: arm:kirkwood Define kirkwood phy address magic number

[Simon Kagstrom]

On Thu, 20 Aug 2009 20:20:08 -0700
Prafulla Wadaskar <prafulla@marvell.com> wrote:

> > > > +#define KIRKWOOD_PHY_ADR_REQUEST 0xee
> > > define this in header file
> > > 
> > > Basically this is needed in drivers/net/phy/mv88e61xx.c for 
> > multi chip support
> > > in this case we need to define this in include/miiphy.h.
> > > which conflicts with other phy address definition, that's 
> > why not done earlier
> > > 
> > > It makes more sense to add APIs miiphy_read/write_phyadr to miiutils
> > 
> > But is this really general functionality? miiphy.h is something I
> > suppose should be generic between phys and not contain device-specific
> > things like this.
> Agreed,
> Current PHY support need to be re-architected to support switches in generic ways
> This is old pending issue, may be we should re-trigger it again after scheduled release

Right. But for now, perhaps it's OK to merge the patch as-is and then
correct the location of the define when the restructuring is done?

In the same way, I guess we should postspone moving out PHY
initialization from sheevaplug and keep it duplicated until it's clear
what to do there?

// Simon

[U-Boot] [PATCH 1/3]: arm:kirkwood Define kirkwood phy address magic number

[Ben Warren]

Simon Kagstrom wrote:
> Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
> ---
>  drivers/net/kirkwood_egiga.c |   14 ++++++++++----
>  1 files changed, 10 insertions(+), 4 deletions(-)
>   
Patches 1-3 applied to net repo.

thanks,
Ben