83xx: uec: miiphybb: Added support for bitBang SMI and uec SMI enabled at the same time.

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 <[email protected]>
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

Wolfgang Denk wrote:
<snip>

Quoted 9 lines

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:

(quoted diff) +0 -2

-#if !(defined(CONFIG_EP8248) || defined(CONFIG_EP82XXM))
-    volatile ioport_t *iop = ioport_addr ((immap_t *) 

CONFIG_SYS_IMMR, MDIO_PORT);

(quoted diff) +3 -0

+#if !(defined(CONFIG_EP8248) || defined(CONFIG_EP82XXM)) \
+    && !defined(CONFIG_MPC83XX)
+    volatile gpio_n_t *iop = ioport_addr ((immap_t *) 

CONFIG_SYS_IMMR, MDIO_PORT);

(quoted diff) +4 -0

+#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 <[email protected]>
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 <[email protected]>

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

drivers/net/phy/miiphybb.h new file +16 -0

new file mode 100644
index 0000000..4fff40d
@@ -0,0 +1,26 @@
+/*
+ * Copyright (C) 2006-2009 RuggedCom, Inc.
+ *
+ * Richard Retanubun <[email protected]>
+ *
+ * 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

drivers/qe/uec.c +1 -2

index db95ada..109edd9 100644
@@ -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

drivers/qe/uec.c +1 -2

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

drivers/qe/uec_phy.c +1 -0

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

drivers/qe/uec_phy.c +8 -0

+*/
+#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 */

drivers/qe/uec_phy.c +3 -0

+};
+#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;

+ (void)bb_miiphy_write(NULL, mii_id, regnum, value);

drivers/qe/uec_phy.c +6 -0

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

drivers/qe/uec_phy.c +2 -0

+#endif /* CONFIG_BITBANGMII */
+
     ug_regs = ugeth->uec_mii_regs;
 

     /* Setting up the MII Mangement Address Register */

Signature · 1 line

1.6.5

drivers/net/phy/miiphybb.h line 26

Quoted 5 lines

drivers/net/phy/miiphybb.h new file +5 -0

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

Quoted 8 lines

drivers/qe/uec.c +1 -2

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

Quoted 9 lines

drivers/qe/uec.c +1 -2

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

drivers/qe/uec_phy.c line 28

Quoted 11 lines

#endif

drivers/qe/uec_phy.c +1 -0

index 9715183..6fb3b4d 100644
@@ -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

drivers/qe/uec_phy.c line 31

Quoted 3 lines

drivers/qe/uec_phy.c +2 -0

+ * #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?

drivers/qe/uec_phy.c

Quoted 2 lines

drivers/qe/uec_phy.c +2 -0

+#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()

drivers/qe/uec_phy.c void uec_write_phy_reg (struct eth_device *dev, int mii_id, int regnum, int valu · line 144

Quoted 4 lines

drivers/qe/uec_phy.c +4 -0

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

Quoted 1 line

+ (void)bb_miiphy_write(NULL, mii_id, regnum, value);

What's the point of this cast?

drivers/qe/uec_phy.c int uec_read_phy_reg (struct eth_device *dev, int mii_id, int regnum) · line 188

Quoted 7 lines

drivers/qe/uec_phy.c +7 -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.

From 1f50506ad9a305c1c9dfc68aa70551166b44d3a0 Mon Sep 17 00:00:00 2001
From: Richard Retanubun<[email protected]>
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