[PATCH 0/5] dynamic detection of gianfar TPIPA

[PATCH 0/5] dynamic detection of gianfar TPIPA

[Paul Gortmaker]

This patch series consists of some minor cleanups that eventually
allow us to implement a dynamic assignment of the gianfar TBIPA.
This was the implementation that Andy and Scott indicated was
the most desireable solution at the bottom of this discussion:

http://patchwork.ozlabs.org/linuxppc-embedded/patch?id=12902

The lead up patches are as follows:

 -dont allocate a phydev for the "ghost" phy that seems to appear
  on ID 0x1f of all the MDIO bus scans.

 -add in the missing assignment to priv->mii_bus when we assign a
  phy to the device.

 -cleanup of unnecessary externs that the above change allows us.

 -make the mii_bus->priv point to struct gianfar_mdio_data

At this point, we can then actually use the phy_map of the mii_bus
to tell us which MDIO ID we can use for the TBIPA, instead of just
hard coding 0x1f.

Paul.

[PATCH 1/5] phylib: don't create a phydev for ID-less PHYs.

[Paul Gortmaker]

I've tested on 8360, 8540 and 8641D and in all cases, the PHY
ID returned for bus addr 0x1f is all zeros, and not all 0xf.
This means we've been allocating a phydev for this "ghost".

In addition to marking 0x0 as an invalid PHY ID, I've also
changed the existing somewhat useless printk to actually
list the bus IDs where it found a PHY so we get a basic
bus summary.

Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/net/phy/mdio_bus.c   |    4 +++-
 drivers/net/phy/phy_device.c |    5 +++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 963630c..e33a119 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -59,6 +59,7 @@ int mdiobus_register(struct mii_bus *bus)
 	if (bus->reset)
 		bus->reset(bus);
 
+	pr_info("%s: PHY(s) at:", bus->name);
 	for (i = 0; i < PHY_MAX_ADDR; i++) {
 		struct phy_device *phydev;
 
@@ -97,12 +98,13 @@ int mdiobus_register(struct mii_bus *bus)
 				phy_device_free(phydev);
 				phydev = NULL;
 			}
+			printk(" 0x%x", i);
 		}
 
 		bus->phy_map[i] = phydev;
 	}
 
-	pr_info("%s: probed\n", bus->name);
+	printk("\n");
 
 	return err;
 }
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index f4c4fd8..740dd2e 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -116,8 +116,9 @@ struct phy_device * get_phy_device(struct mii_bus *bus, int addr)
 
 	phy_id |= (phy_reg & 0xffff);
 
-	/* If the phy_id is all Fs, there is no device there */
-	if (0xffffffff == phy_id)
+	/* If the phy_id is all Fs, there is no device there. Similarly
+ 	   it seems common to get an ID of zero at addr 0x1f */
+	if (0xffffffff == phy_id || 0 == phy_id)
 		return NULL;
 
 	dev = phy_device_create(bus, addr, phy_id);
-- 
1.5.4.3

[PATCH 2/5] gianfar: assign mii_bus value in dev->priv

[Paul Gortmaker]

The gianfar priv struct has an entry for the mii_bus, but it
isn't being populated.  Assign a value for it at the same time
as we assign the phydev, so that it can be used in calls like
gianfar_mdio_read/write.

Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/net/gianfar.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index 718cf77..0ad74c1 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -471,6 +471,7 @@ static int init_phy(struct net_device *dev)
 	phydev->supported &= (GFAR_SUPPORTED | gigabit_support);
 	phydev->advertising = phydev->supported;
 
+	priv->mii_bus = phydev->bus;
 	priv->phydev = phydev;
 
 	return 0;
-- 
1.5.4.3

[PATCH 3/5] gianfar: limit scope of gfar_local_mdio functions

[Paul Gortmaker]

The gfar_local_mdio_read/write functions were brought in via
extern since they go right at the regs vs. going via the normal
gfar_mdio_read/write functions.  With the priv->mii_bus properly
set, we can get rid of the externs, the casts etc. and use the
normal gfar_mdio_read/write. We just need to move the call to the
gfar_configure_serdes down slightly to after where priv->mii_bus
is set to a sane value.

Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/net/gianfar.c     |   17 +++++++----------
 drivers/net/gianfar_mii.c |    4 ++--
 2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index 0ad74c1..9173784 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -130,8 +130,6 @@ static void free_skb_resources(struct gfar_private *priv);
 static void gfar_set_multi(struct net_device *dev);
 static void gfar_set_hash_for_addr(struct net_device *dev, u8 *addr);
 static void gfar_configure_serdes(struct net_device *dev);
-extern int gfar_local_mdio_write(struct gfar_mii __iomem *regs, int mii_id, int regnum, u16 value);
-extern int gfar_local_mdio_read(struct gfar_mii __iomem *regs, int mii_id, int regnum);
 #ifdef CONFIG_GFAR_NAPI
 static int gfar_poll(struct napi_struct *napi, int budget);
 #endif
@@ -459,9 +457,6 @@ static int init_phy(struct net_device *dev)
 
 	phydev = phy_connect(dev, phy_id, &adjust_link, 0, interface);
 
-	if (interface == PHY_INTERFACE_MODE_SGMII)
-		gfar_configure_serdes(dev);
-
 	if (IS_ERR(phydev)) {
 		printk(KERN_ERR "%s: Could not attach to PHY\n", dev->name);
 		return PTR_ERR(phydev);
@@ -474,27 +469,29 @@ static int init_phy(struct net_device *dev)
 	priv->mii_bus = phydev->bus;
 	priv->phydev = phydev;
 
+	if (interface == PHY_INTERFACE_MODE_SGMII)
+		gfar_configure_serdes(dev);
+
 	return 0;
 }
 
 static void gfar_configure_serdes(struct net_device *dev)
 {
 	struct gfar_private *priv = netdev_priv(dev);
-	struct gfar_mii __iomem *regs =
-			(void __iomem *)&priv->regs->gfar_mii_regs;
+	struct mii_bus *bus = priv->mii_bus;
 
 	/* Initialise TBI i/f to communicate with serdes (lynx phy) */
 
 	/* Single clk mode, mii mode off(for aerdes communication) */
-	gfar_local_mdio_write(regs, TBIPA_VALUE, MII_TBICON, TBICON_CLK_SELECT);
+	gfar_mdio_write(bus, TBIPA_VALUE, MII_TBICON, TBICON_CLK_SELECT);
 
 	/* Supported pause and full-duplex, no half-duplex */
-	gfar_local_mdio_write(regs, TBIPA_VALUE, MII_ADVERTISE,
+	gfar_mdio_write(bus, TBIPA_VALUE, MII_ADVERTISE,
 			ADVERTISE_1000XFULL | ADVERTISE_1000XPAUSE |
 			ADVERTISE_1000XPSE_ASYM);
 
 	/* ANEG enable, restart ANEG, full duplex mode, speed[1] set */
-	gfar_local_mdio_write(regs, TBIPA_VALUE, MII_BMCR, BMCR_ANENABLE |
+	gfar_mdio_write(bus, TBIPA_VALUE, MII_BMCR, BMCR_ANENABLE |
 			BMCR_ANRESTART | BMCR_FULLDPLX | BMCR_SPEED1000);
 }
 
diff --git a/drivers/net/gianfar_mii.c b/drivers/net/gianfar_mii.c
index 2432762..d5efa9c 100644
--- a/drivers/net/gianfar_mii.c
+++ b/drivers/net/gianfar_mii.c
@@ -51,7 +51,7 @@
  * the local mdio pins, which may not be the same as system mdio bus, used for
  * controlling the external PHYs, for example.
  */
-int gfar_local_mdio_write(struct gfar_mii __iomem *regs, int mii_id,
+static int gfar_local_mdio_write(struct gfar_mii __iomem *regs, int mii_id,
 			  int regnum, u16 value)
 {
 	/* Set the PHY address and the register address we want to write */
@@ -77,7 +77,7 @@ int gfar_local_mdio_write(struct gfar_mii __iomem *regs, int mii_id,
  * and are always tied to the local mdio pins, which may not be the
  * same as system mdio bus, used for controlling the external PHYs, for eg.
  */
-int gfar_local_mdio_read(struct gfar_mii __iomem *regs, int mii_id, int regnum)
+static int gfar_local_mdio_read(struct gfar_mii __iomem *regs, int mii_id, int regnum)
 
 {
 	u16 value;
-- 
1.5.4.3

[PATCH 4/5] gianfar: dont hog the mii_bus->priv with just the regs.

[Paul Gortmaker]

The gfar was previously using the mii_bus->priv to directly
store the gfar_mii regs, which leaves no room/flexibility to
store anything else there.  I believe it makes more sense to
have mii_bus->priv point at a struct gianfar_mdio_data and
then have the regs stored as a field within that.  It makes
the code easier to read too.

This isn't in a hot path, so there should be no performance
penalty associated with this.

Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/net/gianfar_mii.c   |   17 ++++++++++-------
 include/linux/fsl_devices.h |    1 +
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/net/gianfar_mii.c b/drivers/net/gianfar_mii.c
index d5efa9c..806df3f 100644
--- a/drivers/net/gianfar_mii.c
+++ b/drivers/net/gianfar_mii.c
@@ -104,10 +104,10 @@ static int gfar_local_mdio_read(struct gfar_mii __iomem *regs, int mii_id, int r
  * All PHY configuration is done through the TSEC1 MIIM regs */
 int gfar_mdio_write(struct mii_bus *bus, int mii_id, int regnum, u16 value)
 {
-	struct gfar_mii __iomem *regs = (void __iomem *)bus->priv;
+	struct gianfar_mdio_data *mdata = (struct gianfar_mdio_data *)bus->priv;
 
 	/* Write to the local MII regs */
-	return(gfar_local_mdio_write(regs, mii_id, regnum, value));
+	return(gfar_local_mdio_write(mdata->regs, mii_id, regnum, value));
 }
 
 /* Read the bus for PHY at addr mii_id, register regnum, and
@@ -115,16 +115,17 @@ int gfar_mdio_write(struct mii_bus *bus, int mii_id, int regnum, u16 value)
  * configuration has to be done through the TSEC1 MIIM regs */
 int gfar_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 {
-	struct gfar_mii __iomem *regs = (void __iomem *)bus->priv;
+	struct gianfar_mdio_data *mdata = (struct gianfar_mdio_data *)bus->priv;
 
 	/* Read the local MII regs */
-	return(gfar_local_mdio_read(regs, mii_id, regnum));
+	return(gfar_local_mdio_read(mdata->regs, mii_id, regnum));
 }
 
 /* Reset the MIIM registers, and wait for the bus to free */
 int gfar_mdio_reset(struct mii_bus *bus)
 {
-	struct gfar_mii __iomem *regs = (void __iomem *)bus->priv;
+	struct gianfar_mdio_data *mdata = (struct gianfar_mdio_data *)bus->priv;
+	struct gfar_mii __iomem *regs = mdata->regs;
 	unsigned int timeout = PHY_INIT_TIMEOUT;
 
 	mutex_lock(&bus->mdio_lock);
@@ -192,7 +193,8 @@ int gfar_mdio_probe(struct device *dev)
 		goto reg_map_fail;
 	}
 
-	new_bus->priv = (void __force *)regs;
+	new_bus->priv = pdata;
+	pdata->regs = (void __force *)regs;
 
 	new_bus->irq = pdata->irq;
 
@@ -221,12 +223,13 @@ reg_map_fail:
 int gfar_mdio_remove(struct device *dev)
 {
 	struct mii_bus *bus = dev_get_drvdata(dev);
+	struct gianfar_mdio_data *mdata = (struct gianfar_mdio_data *)bus->priv;
 
 	mdiobus_unregister(bus);
 
 	dev_set_drvdata(dev, NULL);
 
-	iounmap((void __iomem *)bus->priv);
+	iounmap(mdata->regs);
 	bus->priv = NULL;
 	kfree(bus);
 
diff --git a/include/linux/fsl_devices.h b/include/linux/fsl_devices.h
index 1831b19..4b304b6 100644
--- a/include/linux/fsl_devices.h
+++ b/include/linux/fsl_devices.h
@@ -58,6 +58,7 @@ struct gianfar_platform_data {
 
 struct gianfar_mdio_data {
 	/* board specific information */
+	struct	gfar_mii __iomem *regs;
 	int	irq[32];
 };
 
-- 
1.5.4.3

[PATCH 5/5] gianfar: don't hard code the TBIPA MDIO address

[Paul Gortmaker]

Currently the MDIO address for the gianfar Ten Bit
Interface is hard coded to be 0x1f, which can conflit
with some boards if they happen to put a PHY there.

Previous discussions indicated that the proper approach
here would be to dynamically allocate it, based on
picking the highest MDIO address that is not in use
by a PHY.

Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/net/gianfar.c       |   20 ++++++++++++++++----
 drivers/net/gianfar.h       |    1 -
 drivers/net/gianfar_mii.c   |   11 ++++++++++-
 include/linux/fsl_devices.h |    1 +
 4 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index 9173784..684c97b 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -123,6 +123,7 @@ static irqreturn_t gfar_transmit(int irq, void *dev_id);
 static irqreturn_t gfar_interrupt(int irq, void *dev_id);
 static void adjust_link(struct net_device *dev);
 static void init_registers(struct net_device *dev);
+static void gfar_set_tbipa(struct net_device *dev);
 static int init_phy(struct net_device *dev);
 static int gfar_probe(struct platform_device *pdev);
 static int gfar_remove(struct platform_device *pdev);
@@ -469,6 +470,8 @@ static int init_phy(struct net_device *dev)
 	priv->mii_bus = phydev->bus;
 	priv->phydev = phydev;
 
+	gfar_set_tbipa(dev);
+
 	if (interface == PHY_INTERFACE_MODE_SGMII)
 		gfar_configure_serdes(dev);
 
@@ -479,19 +482,20 @@ static void gfar_configure_serdes(struct net_device *dev)
 {
 	struct gfar_private *priv = netdev_priv(dev);
 	struct mii_bus *bus = priv->mii_bus;
+	struct gianfar_mdio_data *mdata = (struct gianfar_mdio_data *)bus->priv;
 
 	/* Initialise TBI i/f to communicate with serdes (lynx phy) */
 
 	/* Single clk mode, mii mode off(for aerdes communication) */
-	gfar_mdio_write(bus, TBIPA_VALUE, MII_TBICON, TBICON_CLK_SELECT);
+	gfar_mdio_write(bus, mdata->tbi_pa, MII_TBICON, TBICON_CLK_SELECT);
 
 	/* Supported pause and full-duplex, no half-duplex */
-	gfar_mdio_write(bus, TBIPA_VALUE, MII_ADVERTISE,
+	gfar_mdio_write(bus, mdata->tbi_pa, MII_ADVERTISE,
 			ADVERTISE_1000XFULL | ADVERTISE_1000XPAUSE |
 			ADVERTISE_1000XPSE_ASYM);
 
 	/* ANEG enable, restart ANEG, full duplex mode, speed[1] set */
-	gfar_mdio_write(bus, TBIPA_VALUE, MII_BMCR, BMCR_ANENABLE |
+	gfar_mdio_write(bus, mdata->tbi_pa, MII_BMCR, BMCR_ANENABLE |
 			BMCR_ANRESTART | BMCR_FULLDPLX | BMCR_SPEED1000);
 }
 
@@ -539,8 +543,16 @@ static void init_registers(struct net_device *dev)
 	/* Initialize the Minimum Frame Length Register */
 	gfar_write(&priv->regs->minflr, MINFLR_INIT_SETTINGS);
 
+}
+
+static void gfar_set_tbipa(struct net_device *dev)
+{
+	struct gfar_private *priv = netdev_priv(dev);
+	struct mii_bus *bus = priv->mii_bus;
+	struct gianfar_mdio_data *mdata = (struct gianfar_mdio_data *)bus->priv;
+
 	/* Assign the TBI an address which won't conflict with the PHYs */
-	gfar_write(&priv->regs->tbipa, TBIPA_VALUE);
+	gfar_write(&priv->regs->tbipa, mdata->tbi_pa);
 }
 
 
diff --git a/drivers/net/gianfar.h b/drivers/net/gianfar.h
index 46cd773..771aa5e 100644
--- a/drivers/net/gianfar.h
+++ b/drivers/net/gianfar.h
@@ -130,7 +130,6 @@ extern const char gfar_driver_version[];
 #define DEFAULT_RXCOUNT	16
 #define DEFAULT_RXTIME	4
 
-#define TBIPA_VALUE		0x1f
 #define MIIMCFG_INIT_VALUE	0x00000007
 #define MIIMCFG_RESET           0x80000000
 #define MIIMIND_BUSY            0x00000001
diff --git a/drivers/net/gianfar_mii.c b/drivers/net/gianfar_mii.c
index 806df3f..41ff8c4 100644
--- a/drivers/net/gianfar_mii.c
+++ b/drivers/net/gianfar_mii.c
@@ -160,7 +160,7 @@ int gfar_mdio_probe(struct device *dev)
 	struct gfar_mii __iomem *regs;
 	struct mii_bus *new_bus;
 	struct resource *r;
-	int err = 0;
+	int i, err = 0;
 
 	if (NULL == dev)
 		return -EINVAL;
@@ -209,6 +209,15 @@ int gfar_mdio_probe(struct device *dev)
 		goto bus_register_fail;
 	}
 
+	/* mdiobus_register has populated the phy_map, so we now know
+ 	   which doghouses have wild dogs living in them.  Search
+	   backwards for the 1st vacant one for the Ten Bit Interface */
+	for (i = PHY_MAX_ADDR - 1; i >= 0; i--)
+		if (new_bus->phy_map[i] == NULL) break;
+
+	printk(KERN_INFO "Gianfar MDIO: using ID 0x%x for TBIPA\n", i);
+	pdata->tbi_pa = i;
+
 	return 0;
 
 bus_register_fail:
diff --git a/include/linux/fsl_devices.h b/include/linux/fsl_devices.h
index 4b304b6..285bd80 100644
--- a/include/linux/fsl_devices.h
+++ b/include/linux/fsl_devices.h
@@ -60,6 +60,7 @@ struct gianfar_mdio_data {
 	/* board specific information */
 	struct	gfar_mii __iomem *regs;
 	int	irq[32];
+	int	tbi_pa;
 };
 
 /* Flags related to gianfar device features */
-- 
1.5.4.3

Re: [PATCH 1/5] phylib: don't create a phydev for ID-less PHYs.

[Joakim Tjernlund]

On Thu, 2008-04-10 at 13:51 -0400, Paul Gortmaker wrote:
> I've tested on 8360, 8540 and 8641D and in all cases, the PHY
> ID returned for bus addr 0x1f is all zeros, and not all 0xf.
> This means we've been allocating a phydev for this "ghost".
> 
> In addition to marking 0x0 as an invalid PHY ID, I've also
> changed the existing somewhat useless printk to actually
> list the bus IDs where it found a PHY so we get a basic
> bus summary.
 
PHY ID 0x0 isn't an invalid id, I got a Broadcom PHY that has
PHY ID=0. Maybe I am misunderstanding something?

 Jocke

Re: [PATCH 1/5] phylib: don't create a phydev for ID-less PHYs.

[Stefan Roese]

On Friday 11 April 2008, Joakim Tjernlund wrote:
> On Thu, 2008-04-10 at 13:51 -0400, Paul Gortmaker wrote:
> > In addition to marking 0x0 as an invalid PHY ID, I've also
> > changed the existing somewhat useless printk to actually
> > list the bus IDs where it found a PHY so we get a basic
> > bus summary.
>
> PHY ID 0x0 isn't an invalid id, I got a Broadcom PHY that has
> PHY ID=0. Maybe I am misunderstanding something?

IIRC, address 0 is the PHY broadcast address, but can be used without problem 
with some PHY's. I remember some designs were we had the PHY at address 0.

Best regards,
Stefan

Re: [PATCH 1/5] phylib: don't create a phydev for ID-less PHYs.

[Joakim Tjernlund]

On Fri, 2008-04-11 at 10:06 +0200, Stefan Roese wrote:
> On Friday 11 April 2008, Joakim Tjernlund wrote:
> > On Thu, 2008-04-10 at 13:51 -0400, Paul Gortmaker wrote:
> > > In addition to marking 0x0 as an invalid PHY ID, I've also
> > > changed the existing somewhat useless printk to actually
> > > list the bus IDs where it found a PHY so we get a basic
> > > bus summary.
> >
> > PHY ID 0x0 isn't an invalid id, I got a Broadcom PHY that has
> > PHY ID=0. Maybe I am misunderstanding something?
> 
> IIRC, address 0 is the PHY broadcast address, but can be used without problem 
> with some PHY's. I remember some designs were we had the PHY at address 0.

Ouh, if that is so, I guess we need a CONFIG option or a new mdio bus
property to select if PHY ID 0 is valid id or not.

 Jocke

Re: [PATCH 1/5] phylib: don't create a phydev for ID-less PHYs.

[Grant Likely]

On Fri, Apr 11, 2008 at 4:47 AM, Joakim Tjernlund
<joakim.tjernlund@transmode.se> wrote:
>
>  On Fri, 2008-04-11 at 10:06 +0200, Stefan Roese wrote:
>  > On Friday 11 April 2008, Joakim Tjernlund wrote:
>  > > PHY ID 0x0 isn't an invalid id, I got a Broadcom PHY that has
>  > > PHY ID=0. Maybe I am misunderstanding something?
>  >
>  > IIRC, address 0 is the PHY broadcast address, but can be used without problem
>  > with some PHY's. I remember some designs were we had the PHY at address 0.
>
>  Ouh, if that is so, I guess we need a CONFIG option or a new mdio bus
>  property to select if PHY ID 0 is valid id or not.

Or, don't probe for phys by default.  Rather, use the data in the
device tree to determine what PHY ids are valid.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 0/5] dynamic detection of gianfar TPIPA

[David Miller]

Please use netdev@vger.kernel.org for patch and development
discussion, linux-net is for user discussions only.

Re: [PATCH 0/5] dynamic detection of gianfar TPIPA

[Andy Fleming]

On Apr 10, 2008, at 12:51, Paul Gortmaker wrote:

>
> This patch series consists of some minor cleanups that eventually
> allow us to implement a dynamic assignment of the gianfar TBIPA.
> This was the implementation that Andy and Scott indicated was
> the most desireable solution at the bottom of this discussion:
>
> http://patchwork.ozlabs.org/linuxppc-embedded/patch?id=12902
>
> The lead up patches are as follows:
>
> -dont allocate a phydev for the "ghost" phy that seems to appear
>  on ID 0x1f of all the MDIO bus scans.
>
> -add in the missing assignment to priv->mii_bus when we assign a
>  phy to the device.
>
> -cleanup of unnecessary externs that the above change allows us.
>
> -make the mii_bus->priv point to struct gianfar_mdio_data
>
> At this point, we can then actually use the phy_map of the mii_bus
> to tell us which MDIO ID we can use for the TBIPA, instead of just
> hard coding 0x1f.


I may be missing something, but I don't think this quite right.

If you have a PHY at 0x1f, this patchset will cause no PHY device to  
be allocated for that address, and you'll actually end up assigning  
TBIPA to be 0x1f again, since there's no PHY there.  Right?  Were you  
able to use this code with a PHY at 0x1f?

I like the idea of passing around priv->mii_bus instead of regs, but I  
think it won't work without becoming unnecessarily unwieldy.  The  
problem is that the TBI PHY is not necessarily accessed through the  
same bus as the PHY.  Each controller has its own TBI PHY, and that  
PHY can only be accessed from *that* controller's MDIO bus.  So if you  
want to configure TSEC2's TBI PHY, you use TSEC2's MDIO regs.  That's  
what gfar_local_mdio_* allowed; they write the *local* controller's  
MDIO regs.  It looks like this code sets up priv->mii_bus to point at  
the bus which holds the PHY, but only TSEC0's bus (on most SoCs) is  
connected to actual PHYs.  So you will only ever be able to configure  
the TBI PHY on TSEC0, which will not allow any of the other TSECs to  
use an SGMII PHY.  Were you able to use other TSECs to connect to an  
SGMII PHY?

We could still pass around an mii_bus reference, but this would  
require creating an mii_bus instance for every single TSEC, which is a  
little heavyweight when we just want to configure the TBI PHY once on  
startup.

After some thinking, I went ahead and implemented a patch which isn't  
ideal, but should solve the problems your patches set out to solve.   
I've sent it in a separate message.  If you have some systems with  
SGMII and/or a PHY at 0x1f, please test this patch on them.  I don't  
currently have either.

Andy

Re: [PATCH 0/5] dynamic detection of gianfar TPIPA

[Paul Gortmaker]

In message: Re: [PATCH 0/5] dynamic detection of gianfar TPIPA
on 10/04/2008 Andy Fleming wrote:

>
> I may be missing something, but I don't think this quite right.
>
> If you have a PHY at 0x1f, this patchset will cause no PHY device to be 
> allocated for that address, and you'll actually end up assigning TBIPA to 
> be 0x1f again, since there's no PHY there.  Right?  Were you able to use 
> this code with a PHY at 0x1f?

I tested on several "normal" boards and on a board with the PHY @ 0x1f,
and it did what I expected it to do.  It was when I was testing on the
normal boards (8540MDS, 8360MDS, HPCN) that I observed we were showing
a PHY ID of 0x0 at 0x1f during the routine PHY scan, because the
autodetect code was skipping 0x1f even on those boards.  I backed out
all my patches and the situation was the same, hence why I decided to
skip IDs of either 0xffff or 0x0.


> I like the idea of passing around priv->mii_bus instead of regs, but I  
> think it won't work without becoming unnecessarily unwieldy.  The  
> problem is that the TBI PHY is not necessarily accessed through the same 
> bus as the PHY.  Each controller has its own TBI PHY, and that PHY can 
> only be accessed from *that* controller's MDIO bus.  So if you want to 
> configure TSEC2's TBI PHY, you use TSEC2's MDIO regs.  That's what 
> gfar_local_mdio_* allowed; they write the *local* controller's MDIO regs. 
>  It looks like this code sets up priv->mii_bus to point at the bus which 
> holds the PHY, but only TSEC0's bus (on most SoCs) is connected to actual 
> PHYs.  So you will only ever be able to configure the TBI PHY on TSEC0, 
> which will not allow any of the other TSECs to use an SGMII PHY.  Were 
> you able to use other TSECs to connect to an SGMII PHY?

Okay -- that explanation helps me understand the role of the *_local_*
variants -- it wasn't obvious to me that they were being used to jump
the device --> bus association and go right at MDIO bus of tsec0.  I
think this can still be handled sanely though -- we'd have to simply
say that if you wanted the bus of the TBI of the controller, you would
go at dev->priv->mii_bus, and if you wanted the bus of the PHY of the
controller, you'd go at dev->priv->phydev->bus.  I'd have to think a bit
to see if that would afford the same or similar cleanups, but the
distinction at least seems clearer to me now.

> We could still pass around an mii_bus reference, but this would require 
> creating an mii_bus instance for every single TSEC, which is a little 
> heavyweight when we just want to configure the TBI PHY once on startup.

Yep.  Is there any boards out there with more than 4 tsec?  I'd have
to go look at the size of mii_bus to see what the per bus cost is.

>
> After some thinking, I went ahead and implemented a patch which isn't  
> ideal, but should solve the problems your patches set out to solve.   
> I've sent it in a separate message.  If you have some systems with SGMII 
> and/or a PHY at 0x1f, please test this patch on them.  I don't currently 
> have either.

I'll go have a look.  I've only got the SBC8641D with the PHY @ 0x1f to
be the oddball guniea pig.

Paul.

>
> Andy
>