[U-Boot] [RFC] 83xx: uec: miiphybb: Added support for bitBang SMI and uec SMI enabled at the same time.

[U-Boot] [RFC] 83xx: uec: miiphybb: Added support for bitBang SMI and uec SMI enabled at the same time.

[Richard Retanubun]

Greetings,

Our board have one PHY's SMI attached via GPIO (bitBang SMI)
and the other network device (a switch) connected to the 8360e's real SMI controller.

This patch aims to make u-boot able to use both at the same time. I am checking if I am
on the right track and if I have missed something.

The board-specific header file #define looks like this (similar to the FIXED_PHY) method

[snip]
#define CONFIG_SYS_BITBANG_PHY_PORT(devnum) devnum,
#define CONFIG_SYS_BITBANG_PHY_PORTS \
	CONFIG_SYS_BITBANG_PHY_PORT(10)
[/snip]

My code's baseline is at commit 57fe30194d3c15c37d9ff06dbd2a4c1ffccda018 (post v2009.06)

Thank you for everyone's time.

- Richard

 From 644a9cd7b9d9331989a3cf59d920a072c9dafe05 Mon Sep 17 00:00:00 2001
From: Richard Retanubun <RichardRetanubun@RuggedCom.com>
Date: Wed, 17 Jun 2009 16:00:41 -0400
Subject: [PATCH] 83xx: UEC: Added support for bitBang MII driver access to PHYs

This patch enabled support for having PHYs on bitBang MII and uec MII
operating at the same time.
---
  drivers/net/phy/miiphybb.c |   28 ++++++++++++++++++++++------
  drivers/net/phy/miiphybb.h |   26 ++++++++++++++++++++++++++
  drivers/qe/uec.c           |    6 ++----
  drivers/qe/uec_phy.c       |   34 ++++++++++++++++++++++++++++++++++
  4 files changed, 84 insertions(+), 10 deletions(-)
  create mode 100644 drivers/net/phy/miiphybb.h

diff --git a/drivers/net/phy/miiphybb.c b/drivers/net/phy/miiphybb.c
index e3c163a..1a15707 100644
--- a/drivers/net/phy/miiphybb.c
+++ b/drivers/net/phy/miiphybb.c
@@ -29,6 +29,7 @@
  #include <common.h>
  #include <ioports.h>
  #include <ppc_asm.tmpl>
+#include "miiphybb.h"

  /*****************************************************************************
   *
@@ -38,8 +39,13 @@
  static void miiphy_pre (char read, unsigned char addr, unsigned char reg)
  {
  	int j;			/* counter */
-#if !(defined(CONFIG_EP8248) || defined(CONFIG_EP82XXM))
-	volatile ioport_t *iop = ioport_addr ((immap_t *) CONFIG_SYS_IMMR, MDIO_PORT);
+#if !(defined(CONFIG_EP8248) || defined(CONFIG_EP82XXM)) \
+	&& !defined(CONFIG_MPC83XX)
+	volatile gpio_n_t *iop = ioport_addr ((immap_t *) CONFIG_SYS_IMMR, MDIO_PORT);
+#elif defined(CONFIG_MPC83XX)
+	volatile immap_t *im = (volatile immap_t *)CONFIG_SYS_IMMR;
+	volatile qepio83xx_t *par_io = (volatile qepio83xx_t *)&im->qepio;
+        volatile gpio_n_t *iop = &(par_io->ioport[MDIO_PORT]);
  #endif

  	/*
@@ -123,8 +129,13 @@ int bb_miiphy_read (char *devname, unsigned char addr,
  {
  	short rdreg;		/* register working value */
  	int j;			/* counter */
-#if !(defined(CONFIG_EP8248) || defined(CONFIG_EP82XXM))
-	volatile ioport_t *iop = ioport_addr ((immap_t *) CONFIG_SYS_IMMR, MDIO_PORT);
+#if !(defined(CONFIG_EP8248) || defined(CONFIG_EP82XXM)) \
+	&& !defined(CONFIG_MPC83XX)
+	volatile gpio_n_t *iop = ioport_addr ((immap_t *) CONFIG_SYS_IMMR, MDIO_PORT);
+#elif defined(CONFIG_MPC83XX)
+	volatile immap_t *im = (volatile immap_t *)CONFIG_SYS_IMMR;
+	volatile qepio83xx_t *par_io = (volatile qepio83xx_t *)&im->qepio;
+        volatile gpio_n_t *iop = &(par_io->ioport[MDIO_PORT]);
  #endif

  	miiphy_pre (1, addr, reg);
@@ -190,8 +201,13 @@ int bb_miiphy_write (char *devname, unsigned char addr,
  		unsigned char reg, unsigned short value)
  {
  	int j;			/* counter */
-#if !(defined(CONFIG_EP8248) || defined(CONFIG_EP82XXM))
-	volatile ioport_t *iop = ioport_addr ((immap_t *) CONFIG_SYS_IMMR, MDIO_PORT);
+#if !(defined(CONFIG_EP8248) || defined(CONFIG_EP82XXM)) \
+	&& !defined(CONFIG_MPC83XX)
+	volatile gpio_n_t *iop = ioport_addr ((immap_t *) CONFIG_SYS_IMMR, MDIO_PORT);
+#elif defined(CONFIG_MPC83XX)
+	volatile immap_t *im = (volatile immap_t *)CONFIG_SYS_IMMR;
+	volatile qepio83xx_t *par_io = (volatile qepio83xx_t *)&im->qepio;
+        volatile gpio_n_t *iop = &(par_io->ioport[MDIO_PORT]);
  #endif

  	miiphy_pre (0, addr, reg);
diff --git a/drivers/net/phy/miiphybb.h b/drivers/net/phy/miiphybb.h
new file mode 100644
index 0000000..4fff40d
--- /dev/null
+++ b/drivers/net/phy/miiphybb.h
@@ -0,0 +1,26 @@
+/*
+ * Copyright (C) 2006-2009 RuggedCom, Inc.
+ *
+ * Richard Retanubun <richardretanubun@ruggedcom.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+int bb_miiphy_read (char *devname, unsigned char addr,
+		unsigned char reg, unsigned short *value);
+
+int bb_miiphy_write (char *devname, unsigned char addr,
+		unsigned char reg, unsigned short value);
diff --git a/drivers/qe/uec.c b/drivers/qe/uec.c
index 63fede9..a9bac99 100644
--- a/drivers/qe/uec.c
+++ b/drivers/qe/uec.c
@@ -579,8 +579,7 @@ static void phy_change(struct eth_device *dev)
  	adjust_link(dev);
  }

-#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) \
-	&& !defined(BITBANGMII)
+#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) /*&& !defined(BITBANGMII)*/

  /*
   * Find a device index from the devlist by name
@@ -1383,8 +1382,7 @@ int uec_initialize(bd_t *bis, uec_info_t *uec_info)
  		return err;
  	}

-#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) \
-	&& !defined(BITBANGMII)
+#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) /* && !defined(CONFIG_BITBANGMII) */
  	miiphy_register(dev->name, uec_miiphy_read, uec_miiphy_write);
  #endif

diff --git a/drivers/qe/uec_phy.c b/drivers/qe/uec_phy.c
index d613f3e..c7c7439 100644
--- a/drivers/qe/uec_phy.c
+++ b/drivers/qe/uec_phy.c
@@ -25,6 +25,7 @@
  #include "uec.h"
  #include "uec_phy.h"
  #include "miiphy.h"
+#include "../net/phy/miiphybb.h"

  #define ugphy_printk(format, arg...)  \
  	printf(format "\n", ## arg)
@@ -92,6 +93,15 @@ static const struct fixed_phy_port fixed_phy_port[] = {
  	CONFIG_SYS_FIXED_PHY_PORTS /* defined in board configuration file */
  };

+/* BitBang MII support for ethernet ports */
+#ifndef CONFIG_SYS_BITBANG_PHY_PORTS
+#define CONFIG_SYS_BITBANG_PHY_PORTS	/* default is an empty array */
+#endif
+
+static const unsigned short bitbang_phy_port[] = {
+	CONFIG_SYS_BITBANG_PHY_PORTS /* defined in board configuration file */
+};
+
  static void config_genmii_advert (struct uec_mii_info *mii_info);
  static void genmii_setup_forced (struct uec_mii_info *mii_info);
  static void genmii_restart_aneg (struct uec_mii_info *mii_info);
@@ -112,6 +122,18 @@ void uec_write_phy_reg (struct eth_device *dev, int mii_id, int regnum, int valu
  	enet_tbi_mii_reg_e mii_reg = (enet_tbi_mii_reg_e) regnum;
  	u32 tmp_reg;

+
+#if defined(CONFIG_BITBANGMII)
+	u8 i = 0;
+
+	for (i = 0; i < ARRAY_SIZE(bitbang_phy_port); i++) {
+		if (mii_id == bitbang_phy_port[i]) {
+			(void)bb_miiphy_write(NULL, mii_id, regnum, value);
+			return;
+		}
+	}
+#endif /* CONFIG_BITBANGMII */
+
  	ug_regs = ugeth->uec_mii_regs;

  	/* Stop the MII management read cycle */
@@ -139,6 +161,18 @@ int uec_read_phy_reg (struct eth_device *dev, int mii_id, int regnum)
  	u32 tmp_reg;
  	u16 value;

+
+#if defined(CONFIG_BITBANGMII)
+	u8 i = 0;
+
+	for (i = 0; i < ARRAY_SIZE(bitbang_phy_port); i++) {
+		if (mii_id == bitbang_phy_port[i]) {
+			(void)bb_miiphy_read(NULL, mii_id, regnum, &value);
+			return (value);
+		}
+	}
+#endif /* CONFIG_BITBANGMII */
+
  	ug_regs = ugeth->uec_mii_regs;

  	/* Setting up the MII Mangement Address Register */
--
1.6.2.4

[U-Boot] [RFC] 83xx: uec: miiphybb: Added support for bitBang SMI and uec SMI enabled at the same time.

[Wolfgang Denk]

Dear Kim,

In message <4A394E90.6080600@RuggedCom.com> Richard Retanubun wrote:
> Greetings,
> 
> Our board have one PHY's SMI attached via GPIO (bitBang SMI)
> and the other network device (a switch) connected to the 8360e's real SMI controller.
> 
> This patch aims to make u-boot able to use both at the same time. I am checking if I am
> on the right track and if I have missed something.
> 
> The board-specific header file #define looks like this (similar to the FIXED_PHY) method
> 
> [snip]
> #define CONFIG_SYS_BITBANG_PHY_PORT(devnum) devnum,
> #define CONFIG_SYS_BITBANG_PHY_PORTS \
> 	CONFIG_SYS_BITBANG_PHY_PORT(10)
> [/snip]
> 
> My code's baseline is at commit 57fe30194d3c15c37d9ff06dbd2a4c1ffccda018 (post v2009.06)
> 
> Thank you for everyone's time.
> 
> - Richard
> 
>  From 644a9cd7b9d9331989a3cf59d920a072c9dafe05 Mon Sep 17 00:00:00 2001
> From: Richard Retanubun <RichardRetanubun@RuggedCom.com>
> Date: Wed, 17 Jun 2009 16:00:41 -0400
> Subject: [PATCH] 83xx: UEC: Added support for bitBang MII driver access to PHYs
> 
> This patch enabled support for having PHYs on bitBang MII and uec MII
> operating at the same time.
> ---
>   drivers/net/phy/miiphybb.c |   28 ++++++++++++++++++++++------
>   drivers/net/phy/miiphybb.h |   26 ++++++++++++++++++++++++++
>   drivers/qe/uec.c           |    6 ++----
>   drivers/qe/uec_phy.c       |   34 ++++++++++++++++++++++++++++++++++
>   4 files changed, 84 insertions(+), 10 deletions(-)
>   create mode 100644 drivers/net/phy/miiphybb.h

Can you please check the status of this patch:
http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/61758

Thanks.

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
The years of peak mental activity are undoubtedly between the ages of
four and eighteen. At four we know all the questions, at eighteen all
the answers.

[U-Boot] [RFC] 83xx: uec: miiphybb: Added support for bitBang SMI and uec SMI enabled at the same time.

[Richard Retanubun]

Wolfgang Denk wrote:
<snip>
> Can you please check the status of this patch:
> http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/61758
> 
> Thanks.
> 
> Best regards,
> 
> Wolfgang Denk
> 

Hello,

Wofgang, I believe at the time Ben made the comment that my patch's usage of things like these:

-#if !(defined(CONFIG_EP8248) || defined(CONFIG_EP82XXM))
-	volatile ioport_t *iop = ioport_addr ((immap_t *) CONFIG_SYS_IMMR, MDIO_PORT);
+#if !(defined(CONFIG_EP8248) || defined(CONFIG_EP82XXM)) \
+	&& !defined(CONFIG_MPC83XX)
+	volatile gpio_n_t *iop = ioport_addr ((immap_t *) CONFIG_SYS_IMMR, MDIO_PORT);
+#elif defined(CONFIG_MPC83XX)
+	volatile immap_t *im = (volatile immap_t *)CONFIG_SYS_IMMR;
+	volatile qepio83xx_t *par_io = (volatile qepio83xx_t *)&im->qepio;
+        volatile gpio_n_t *iop = &(par_io->ioport[MDIO_PORT]);
   #endif

Is propagating bad practice, and I agreed. Since then, Luigi's deft implementation MDIO_DECLARE and MDC_DELCARE
have eliminated this problem. (or at least move it away from Ben's vigilant eyes into the board specific header file).

As a refresher, the declaration for which UEC uses the bitbang in the board header file is like this:

#define CONFIG_SYS_BITBANG_PHY_PORT(name) name,

/* Bit-bang SMI ports */
#define CONFIG_SYS_BITBANG_PHY_PORTS \
	CONFIG_SYS_BITBANG_PHY_PORT("FSL UEC4") \
	CONFIG_SYS_BITBANG_PHY_PORT("FSL UEC5")

similar to how CONFIG_SYS_FIXED_PHY_PORT is used.


And so, here is V2 of the patch, with less ugly board specific code and rebased to commit 2ff6922280025c1315c53fa2eb4ab33f0c9591de
(Tue Jan 12 23:47:03 2010 +0100)

- Richard

====================================================================================================
 From 46380bee3b8e63afff9d00729f38cb960e0d2bfb Mon Sep 17 00:00:00 2001
From: Richard Retanubun <RichardRetanubun@RuggedCom.com>
Date: Wed, 17 Jun 2009 16:00:41 -0400
Subject: [PATCH] 83xx: UEC: Added support for bitBang MII driver access to PHYs

This patch enabled support for having PHYs on bitBang MII and uec MII
operating at the same time. Modeled after the MPC8360ADS implementation
and added the ability to specify which ethernet interfaces have bitbang
SMI on the board header file.

Signed-off-by: Richard Retanubun <RichardRetanubun@RuggedCom.com>
---
  drivers/net/phy/miiphybb.h |   26 +++++++++++++++++++++++
  drivers/qe/uec.c           |    6 +---
  drivers/qe/uec_phy.c       |   48 ++++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 76 insertions(+), 4 deletions(-)
  create mode 100644 drivers/net/phy/miiphybb.h

diff --git a/drivers/net/phy/miiphybb.h b/drivers/net/phy/miiphybb.h
new file mode 100644
index 0000000..4fff40d
--- /dev/null
+++ b/drivers/net/phy/miiphybb.h
@@ -0,0 +1,26 @@
+/*
+ * Copyright (C) 2006-2009 RuggedCom, Inc.
+ *
+ * Richard Retanubun <richardretanubun@ruggedcom.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+int bb_miiphy_read (char *devname, unsigned char addr,
+		unsigned char reg, unsigned short *value);
+
+int bb_miiphy_write (char *devname, unsigned char addr,
+		unsigned char reg, unsigned short value);
diff --git a/drivers/qe/uec.c b/drivers/qe/uec.c
index db95ada..109edd9 100644
--- a/drivers/qe/uec.c
+++ b/drivers/qe/uec.c
@@ -579,8 +579,7 @@ static void phy_change(struct eth_device *dev)
  	adjust_link(dev);
  }

-#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) \
-	&& !defined(BITBANGMII)
+#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) /*&& !defined(BITBANGMII)*/

  /*
   * Find a device index from the devlist by name
@@ -1372,8 +1371,7 @@ int uec_initialize(bd_t *bis, uec_info_t *uec_info)
  		return err;
  	}

-#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) \
-	&& !defined(BITBANGMII)
+#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) /* && !defined(CONFIG_BITBANGMII) */
  	miiphy_register(dev->name, uec_miiphy_read, uec_miiphy_write);
  #endif

diff --git a/drivers/qe/uec_phy.c b/drivers/qe/uec_phy.c
index 9715183..6fb3b4d 100644
--- a/drivers/qe/uec_phy.c
+++ b/drivers/qe/uec_phy.c
@@ -25,6 +25,7 @@
  #include "uec.h"
  #include "uec_phy.h"
  #include "miiphy.h"
+#include "../net/phy/miiphybb.h"

  #define ugphy_printk(format, arg...)  \
  	printf(format "\n", ## arg)
@@ -93,6 +94,27 @@ static const struct fixed_phy_port fixed_phy_port[] = {
  	CONFIG_SYS_FIXED_PHY_PORTS /* defined in board configuration file */
  };

+/*--------------------------------------------------------------------+
+ * BitBang MII support for ethernet ports
+ *
+ * Based from MPC8560ADS implementation
+ *--------------------------------------------------------------------*/
+/*
+ * Example board header file to define bitbang ethernet ports:
+ *
+ * #define CONFIG_SYS_BITBANG_PHY_PORT(name) name,
+ * #define CONFIG_SYS_BITBANG_PHY_PORTS CONFIG_SYS_BITBANG_PHY_PORT("FSL UEC0")
+*/
+#ifndef CONFIG_SYS_BITBANG_PHY_PORTS
+#define CONFIG_SYS_BITBANG_PHY_PORTS	/* default is an empty array */
+#endif
+
+#if defined(CONFIG_BITBANGMII)
+static const char *bitbang_phy_port[] = {
+	CONFIG_SYS_BITBANG_PHY_PORTS /* defined in board configuration file */
+};
+#endif /* CONFIG_BITBANGMII */
+
  static void config_genmii_advert (struct uec_mii_info *mii_info);
  static void genmii_setup_forced (struct uec_mii_info *mii_info);
  static void genmii_restart_aneg (struct uec_mii_info *mii_info);
@@ -113,6 +135,19 @@ void uec_write_phy_reg (struct eth_device *dev, int mii_id, int regnum, int valu
  	enet_tbi_mii_reg_e mii_reg = (enet_tbi_mii_reg_e) regnum;
  	u32 tmp_reg;

+
+#if defined(CONFIG_BITBANGMII)
+	u8 i = 0;
+
+	for (i = 0; i < ARRAY_SIZE(bitbang_phy_port); i++) {
+		if (strncmp(dev->name, bitbang_phy_port[i],
+			strlen(dev->name)) == 0) {
+			(void)bb_miiphy_write(NULL, mii_id, regnum, value);
+			return;
+		}
+	}
+#endif /* CONFIG_BITBANGMII */
+
  	ug_regs = ugeth->uec_mii_regs;

  	/* Stop the MII management read cycle */
@@ -140,6 +175,19 @@ int uec_read_phy_reg (struct eth_device *dev, int mii_id, int regnum)
  	u32 tmp_reg;
  	u16 value;

+
+#if defined(CONFIG_BITBANGMII)
+	u8 i = 0;
+
+	for (i = 0; i < ARRAY_SIZE(bitbang_phy_port); i++) {
+		if (strncmp(dev->name, bitbang_phy_port[i],
+			strlen(dev->name)) == 0) {
+			(void)bb_miiphy_read(NULL, mii_id, regnum, &value);
+			return (value);
+		}
+	}
+#endif /* CONFIG_BITBANGMII */
+
  	ug_regs = ugeth->uec_mii_regs;

  	/* Setting up the MII Mangement Address Register */
--
1.6.5

[U-Boot] [RFC] 83xx: uec: miiphybb: Added support for bitBang SMI and uec SMI enabled at the same time.

[Ben Warren]

Hi Richard,

Richard Retanubun wrote:
> Wolfgang Denk wrote:
> <snip>
>> Can you please check the status of this patch:
>> http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/61758
>>
>> Thanks.
>>
>> Best regards,
>>
>> Wolfgang Denk
>>
>
> Hello,
>
> Wofgang, I believe at the time Ben made the comment that my patch's 
> usage of things like these:
>
> -#if !(defined(CONFIG_EP8248) || defined(CONFIG_EP82XXM))
> -    volatile ioport_t *iop = ioport_addr ((immap_t *) 
> CONFIG_SYS_IMMR, MDIO_PORT);
> +#if !(defined(CONFIG_EP8248) || defined(CONFIG_EP82XXM)) \
> +    && !defined(CONFIG_MPC83XX)
> +    volatile gpio_n_t *iop = ioport_addr ((immap_t *) 
> CONFIG_SYS_IMMR, MDIO_PORT);
> +#elif defined(CONFIG_MPC83XX)
> +    volatile immap_t *im = (volatile immap_t *)CONFIG_SYS_IMMR;
> +    volatile qepio83xx_t *par_io = (volatile qepio83xx_t *)&im->qepio;
> +        volatile gpio_n_t *iop = &(par_io->ioport[MDIO_PORT]);
>   #endif
>
> Is propagating bad practice, and I agreed. Since then, Luigi's deft 
> implementation MDIO_DECLARE and MDC_DELCARE
> have eliminated this problem. (or at least move it away from Ben's 
> vigilant eyes into the board specific header file).
>
Luigi did some good things here.  I'm glad you're sync'ing up.
> As a refresher, the declaration for which UEC uses the bitbang in the 
> board header file is like this:
>
> #define CONFIG_SYS_BITBANG_PHY_PORT(name) name,
>
> /* Bit-bang SMI ports */
> #define CONFIG_SYS_BITBANG_PHY_PORTS \
>     CONFIG_SYS_BITBANG_PHY_PORT("FSL UEC4") \
>     CONFIG_SYS_BITBANG_PHY_PORT("FSL UEC5")
>
> similar to how CONFIG_SYS_FIXED_PHY_PORT is used.
>
>
> And so, here is V2 of the patch, with less ugly board specific code 
> and rebased to commit 2ff6922280025c1315c53fa2eb4ab33f0c9591de
> (Tue Jan 12 23:47:03 2010 +0100)
>
> - Richard
>
> ==================================================================================================== 
>
> From 46380bee3b8e63afff9d00729f38cb960e0d2bfb Mon Sep 17 00:00:00 2001
> From: Richard Retanubun <RichardRetanubun@RuggedCom.com>
> Date: Wed, 17 Jun 2009 16:00:41 -0400
> Subject: [PATCH] 83xx: UEC: Added support for bitBang MII driver 
> access to PHYs
>
> This patch enabled support for having PHYs on bitBang MII and uec MII
> operating at the same time. Modeled after the MPC8360ADS implementation
> and added the ability to specify which ethernet interfaces have bitbang
> SMI on the board header file.
>
> Signed-off-by: Richard Retanubun <RichardRetanubun@RuggedCom.com>
> ---
>  drivers/net/phy/miiphybb.h |   26 +++++++++++++++++++++++
>  drivers/qe/uec.c           |    6 +---
>  drivers/qe/uec_phy.c       |   48 
> ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 76 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/net/phy/miiphybb.h
>
> diff --git a/drivers/net/phy/miiphybb.h b/drivers/net/phy/miiphybb.h
> new file mode 100644
> index 0000000..4fff40d
> --- /dev/null
> +++ b/drivers/net/phy/miiphybb.h
> @@ -0,0 +1,26 @@
> +/*
> + * Copyright (C) 2006-2009 RuggedCom, Inc.
> + *
> + * Richard Retanubun <richardretanubun@ruggedcom.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +int bb_miiphy_read (char *devname, unsigned char addr,
> +        unsigned char reg, unsigned short *value);
> +
> +int bb_miiphy_write (char *devname, unsigned char addr,
> +        unsigned char reg, unsigned short value);
There's already BB-related stuff in include/miiphy.h.  Why not put the 
prototypes there?
> diff --git a/drivers/qe/uec.c b/drivers/qe/uec.c
> index db95ada..109edd9 100644
> --- a/drivers/qe/uec.c
> +++ b/drivers/qe/uec.c
> @@ -579,8 +579,7 @@ static void phy_change(struct eth_device *dev)
>      adjust_link(dev);
>  }
>
> -#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) \
> -    && !defined(BITBANGMII)
> +#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) /*&& 
> !defined(BITBANGMII)*/
What's the point of this comment?
>
>  /*
>   * Find a device index from the devlist by name
> @@ -1372,8 +1371,7 @@ int uec_initialize(bd_t *bis, uec_info_t *uec_info)
>          return err;
>      }
>
> -#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) \
> -    && !defined(BITBANGMII)
> +#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) /* && 
> !defined(CONFIG_BITBANGMII) */
>      miiphy_register(dev->name, uec_miiphy_read, uec_miiphy_write);
Same comment-related comment as above.
>  #endif
>
> diff --git a/drivers/qe/uec_phy.c b/drivers/qe/uec_phy.c
> index 9715183..6fb3b4d 100644
> --- a/drivers/qe/uec_phy.c
> +++ b/drivers/qe/uec_phy.c
> @@ -25,6 +25,7 @@
>  #include "uec.h"
>  #include "uec_phy.h"
>  #include "miiphy.h"
> +#include "../net/phy/miiphybb.h"
Please don't use relative includes.  As I'm sure you've noticed, people 
around here are keen on massive file reorganization, and this sort of 
thing makes that more work.  If you put your prototypes in miiphy.h it 
won't be a problem
>
>  #define ugphy_printk(format, arg...)  \
>      printf(format "\n", ## arg)
> @@ -93,6 +94,27 @@ static const struct fixed_phy_port fixed_phy_port[] 
> = {
>      CONFIG_SYS_FIXED_PHY_PORTS /* defined in board configuration file */
>  };
>
> +/*--------------------------------------------------------------------+
> + * BitBang MII support for ethernet ports
> + *
> + * Based from MPC8560ADS implementation
> + *--------------------------------------------------------------------*/
> +/*
> + * Example board header file to define bitbang ethernet ports:
> + *
> + * #define CONFIG_SYS_BITBANG_PHY_PORT(name) name,
> + * #define CONFIG_SYS_BITBANG_PHY_PORTS 
> CONFIG_SYS_BITBANG_PHY_PORT("FSL UEC0")
Maybe you could come up with a shorter name?
> +*/
> +#ifndef CONFIG_SYS_BITBANG_PHY_PORTS
> +#define CONFIG_SYS_BITBANG_PHY_PORTS    /* default is an empty array */
> +#endif
> +
> +#if defined(CONFIG_BITBANGMII)
> +static const char *bitbang_phy_port[] = {
> +    CONFIG_SYS_BITBANG_PHY_PORTS /* defined in board configuration 
> file */
> +};
> +#endif /* CONFIG_BITBANGMII */
> +
>  static void config_genmii_advert (struct uec_mii_info *mii_info);
>  static void genmii_setup_forced (struct uec_mii_info *mii_info);
>  static void genmii_restart_aneg (struct uec_mii_info *mii_info);
> @@ -113,6 +135,19 @@ void uec_write_phy_reg (struct eth_device *dev, 
> int mii_id, int regnum, int valu
>      enet_tbi_mii_reg_e mii_reg = (enet_tbi_mii_reg_e) regnum;
>      u32 tmp_reg;
>
> +
> +#if defined(CONFIG_BITBANGMII)
> +    u8 i = 0;
Why is this a u8?  I don't believe you save any space by making it less 
than an int, and probably invite compiler warnings with the comparison 
to ARRAY_SIZE()
> +
> +    for (i = 0; i < ARRAY_SIZE(bitbang_phy_port); i++) {
> +        if (strncmp(dev->name, bitbang_phy_port[i],
> +            strlen(dev->name)) == 0) {
It's probably better to do sizeof(dev->name)
> +            (void)bb_miiphy_write(NULL, mii_id, regnum, value);
What's the point of this cast?
> +            return;
> +        }
> +    }
> +#endif /* CONFIG_BITBANGMII */
> +
>      ug_regs = ugeth->uec_mii_regs;
>
>      /* Stop the MII management read cycle */
> @@ -140,6 +175,19 @@ int uec_read_phy_reg (struct eth_device *dev, int 
> mii_id, int regnum)
>      u32 tmp_reg;
>      u16 value;
>
> +
> +#if defined(CONFIG_BITBANGMII)
> +    u8 i = 0;
> +
> +    for (i = 0; i < ARRAY_SIZE(bitbang_phy_port); i++) {
> +        if (strncmp(dev->name, bitbang_phy_port[i],
> +            strlen(dev->name)) == 0) {
> +            (void)bb_miiphy_read(NULL, mii_id, regnum, &value);
> +            return (value);
> +        }
> +    }
Same comments as above.
> +#endif /* CONFIG_BITBANGMII */
> +
>      ug_regs = ugeth->uec_mii_regs;
>
>      /* Setting up the MII Mangement Address Register */
> -- 
> 1.6.5
>

thanks a lot,
Ben

[U-Boot] [RFC] 83xx: uec: miiphybb: Added support for bitBang SMI and uec SMI enabled at the same time.

[Richard Retanubun]

Hi Ben,

Thanks for the feedback, my comments are inline. I'll hold off on a patch-V2
until you bless the #define CONFIG_SYS_* name and my use of (void) casting function call.

- Richard

<snip>
>> +int bb_miiphy_read (char *devname, unsigned char addr,
>> +        unsigned char reg, unsigned short *value);
>> +
>> +int bb_miiphy_write (char *devname, unsigned char addr,
>> +        unsigned char reg, unsigned short value);
> There's already BB-related stuff in include/miiphy.h.  Why not put the 
> prototypes there?

Indeed there is, I think Luigi added them and I missed it; sorry about that
my patch-V2 will have this file removed.

<snip>
>> @@ -579,8 +579,7 @@ static void phy_change(struct eth_device *dev)
>>      adjust_link(dev);
>>  }
>>
>> -#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) \
>> -    && !defined(BITBANGMII)
>> +#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) /*&& 
>> !defined(BITBANGMII)*/
> What's the point of this comment?

I think at the time I was unsure of what/how BITBANGMII was being used
(it is some legacy thing, no?). I was trying to (gently) signal its removal
by commenting it out from the #ifdef and forgot to clean it up.
will be removed in patch-V2.

<snip>
>> @@ -1372,8 +1371,7 @@ int uec_initialize(bd_t *bis, uec_info_t *uec_info)
>>          return err;
>>      }
>>
>> -#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) \
>> -    && !defined(BITBANGMII)
>> +#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) /* && 
>> !defined(CONFIG_BITBANGMII) */
>>      miiphy_register(dev->name, uec_miiphy_read, uec_miiphy_write);
> Same comment-related comment as above.

Same explanation, will remove in patch-V2

<snip>
>>  #endif
>>
>> diff --git a/drivers/qe/uec_phy.c b/drivers/qe/uec_phy.c
>> index 9715183..6fb3b4d 100644
>> --- a/drivers/qe/uec_phy.c
>> +++ b/drivers/qe/uec_phy.c
>> @@ -25,6 +25,7 @@
>>  #include "uec.h"
>>  #include "uec_phy.h"
>>  #include "miiphy.h"
>> +#include "../net/phy/miiphybb.h"
> Please don't use relative includes.  As I'm sure you've noticed, people 
> around here are keen on massive file reorganization, and this sort of 
> thing makes that more work.  If you put your prototypes in miiphy.h it 
> won't be a problem

Understood, will do in patch-V2

<snip>
>> + * #define CONFIG_SYS_BITBANG_PHY_PORT(name) name,
>> + * #define CONFIG_SYS_BITBANG_PHY_PORTS 
>> CONFIG_SYS_BITBANG_PHY_PORT("FSL UEC0")
> Maybe you could come up with a shorter name?

I thought this name lines up well with CONFIG_SYS_FIXED_PHY_PORTS;
but I can go with CONFIG_SYS_BB_PHY_PORT(S) (5 less chars) if you like.
I think _BITBANG_ is easier to read though.

Let me know and I'll make it so in patch-V2


>> +#if defined(CONFIG_BITBANGMII)
>> +    u8 i = 0;
> Why is this a u8?  I don't believe you save any space by making it less 
> than an int, and probably invite compiler warnings with the comparison 
> to ARRAY_SIZE()
Good point, I'll make it a u32 then.

>> +
>> +    for (i = 0; i < ARRAY_SIZE(bitbang_phy_port); i++) {
>> +        if (strncmp(dev->name, bitbang_phy_port[i],
>> +            strlen(dev->name)) == 0) {
> It's probably better to do sizeof(dev->name)
Understood, will do in patch-V2.


>> +            (void)bb_miiphy_write(NULL, mii_id, regnum, value);
> What's the point of this cast?

I was thought that it is a good practice to do when you call a function
that have a return code, but you don't care/check for it now.

With the casting, if you choose to do check return codes in the future,
you know which functions have return code.


>> +    for (i = 0; i < ARRAY_SIZE(bitbang_phy_port); i++) {
>> +        if (strncmp(dev->name, bitbang_phy_port[i],
>> +            strlen(dev->name)) == 0) {
>> +            (void)bb_miiphy_read(NULL, mii_id, regnum, &value);
>> +            return (value);
>> +        }
>> +    }
> Same comments as above.
Will do the same things as above (u32 and sizeof).

Thanks a lot for your time.

- Richard

[U-Boot] [RFC] 83xx: uec: miiphybb: Added support for bitBang SMI and uec SMI enabled at the same time.

[richardretanubun at ruggedcom.com]

[U-Boot] [RFC] 83xx: uec: miiphybb: Added support for bitBang SMI and uec SMI enabled at the same time.

[Ben Warren]

Hi Richard,

On 4/1/2010 11:17 AM, richardretanubun at ruggedcom.com wrote:
>  From 1f50506ad9a305c1c9dfc68aa70551166b44d3a0 Mon Sep 17 00:00:00 2001
> From: Richard Retanubun<RichardRetanubun@RuggedCom.com>
> Date: Wed, 17 Jun 2009 16:00:41 -0400
> Subject: [PATCH] 83xx: UEC: Added support for bitBang MII driver access to PHYs
>
> This patch enabled support for having PHYs on bitBang MII and uec MII
> operating at the same time. Modeled after the MPC8360ADS implementation.
>
> Added the ability to specify which ethernet interfaces have bitbang SMI
> on the board header file.
> ---
> Hi Ben,
>
> Just wanted to follow up, I posted replies to your feedback for V1 of this
> patch, but did not hear from you about it. Now that the merge window is open
> again, I thought I should try again. So here is V2 with the changes you
> suggested.
>
> Thanks
>    
I'm really sorry this took so long.

Applied to net repo.

thanks,
Ben