[patch 0/2] FEC_MPC52XX_PHY: Updates

[patch 0/2] FEC_MPC52XX_PHY: Updates

[Wolfram Sang]

Hello,

While debugging FEC-problems, I encountered some issues in the FEC_PHY
driver.

- Patch 1 removes some obsolete code remaining from a (according to old
  mails) broken attempt to handle irqs.
- Patch 2 refactors read/write routines to use common code, so updates
  regarding bus transfers just have to be made once.

Tested on a Phytec phyCORE-MPC5200B-IO board.

All the best,

   Wolfram

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

[patch 1/2] FEC_MPC52XX_PHY: Remove obsolete code

[Wolfram Sang]

Remove last remains of the former (and broken) irq support code. The
driver is polling only.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
 drivers/net/fec_mpc52xx_phy.c |   20 --------------------
 1 file changed, 20 deletions(-)

Index: .kernel/drivers/net/fec_mpc52xx_phy.c
===================================================================
--- .kernel.orig/drivers/net/fec_mpc52xx_phy.c
+++ .kernel/drivers/net/fec_mpc52xx_phy.c
@@ -76,7 +76,6 @@
 {
 	struct device *dev = &of->dev;
 	struct device_node *np = of->node;
-	struct device_node *child = NULL;
 	struct mii_bus *bus;
 	struct mpc52xx_fec_mdio_priv *priv;
 	struct resource res = {};
@@ -105,15 +104,6 @@
 	for (i=0; i<PHY_MAX_ADDR; i++)
 		bus->irq[i] = PHY_POLL;
 
-	while ((child = of_get_next_child(np, child)) != NULL) {
-		int irq = irq_of_parse_and_map(child, 0);
-		if (irq != NO_IRQ) {
-			const u32 *id = of_get_property(child, "reg", NULL);
-			if (id)
-				bus->irq[*id] = irq;
-		}
-	}
-
 	/* setup registers */
 	err = of_address_to_resource(np, 0, &res);
 	if (err)
@@ -133,9 +123,6 @@
 	/* set MII speed */
 	out_be32(&priv->regs->mii_speed, ((mpc52xx_find_ipb_freq(of->node) >> 20) / 5) << 1);
 
-	/* enable MII interrupt */
-	out_be32(&priv->regs->imask, in_be32(&priv->regs->imask) | FEC_IMASK_MII);
-
 	err = mdiobus_register(bus);
 	if (err)
 		goto out_unmap;
@@ -145,9 +132,6 @@
  out_unmap:
 	iounmap(priv->regs);
  out_free:
-	for (i=0; i<PHY_MAX_ADDR; i++)
-		if (bus->irq[i] != PHY_POLL)
-			irq_dispose_mapping(bus->irq[i]);
 	kfree(bus->irq);
 	kfree(priv);
 	kfree(bus);
@@ -160,15 +144,11 @@
 	struct device *dev = &of->dev;
 	struct mii_bus *bus = dev_get_drvdata(dev);
 	struct mpc52xx_fec_mdio_priv *priv = bus->priv;
-	int i;
 
 	mdiobus_unregister(bus);
 	dev_set_drvdata(dev, NULL);
 
 	iounmap(priv->regs);
-	for (i=0; i<PHY_MAX_ADDR; i++)
-		if (bus->irq[i])
-			irq_dispose_mapping(bus->irq[i]);
 	kfree(priv);
 	kfree(bus->irq);
 	kfree(bus);

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

[patch 2/2] FEC_MPC52XX_PHY: Refactor read/write routines

[Wolfram Sang]

Read & write functions now call a generic transfer function, so identical
code in both routines could be eliminated. The result is easier to maintain
and smaller in source and binary code. Also, fix some checkpatch warnings.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
 drivers/net/fec_mpc52xx_phy.c |   55 ++++++++++++++++--------------------------
 1 file changed, 22 insertions(+), 33 deletions(-)

Index: .kernel/drivers/net/fec_mpc52xx_phy.c
===================================================================
--- .kernel.orig/drivers/net/fec_mpc52xx_phy.c
+++ .kernel/drivers/net/fec_mpc52xx_phy.c
@@ -2,6 +2,7 @@
  * Driver for the MPC5200 Fast Ethernet Controller - MDIO bus driver
  *
  * Copyright (C) 2007  Domen Puncer, Telargo, Inc.
+ * Copyright (C) 2008  Wolfram Sang, Pengutronix
  *
  * This file is licensed under the terms of the GNU General Public License
  * version 2. This program is licensed "as is" without any warranty of any
@@ -21,58 +22,45 @@
 	struct mpc52xx_fec __iomem *regs;
 };
 
-static int mpc52xx_fec_mdio_read(struct mii_bus *bus, int phy_id, int reg)
+static int mpc52xx_fec_mdio_transfer(struct mii_bus *bus, int phy_id,
+		int reg, u32 value)
 {
 	struct mpc52xx_fec_mdio_priv *priv = bus->priv;
 	struct mpc52xx_fec __iomem *fec;
 	int tries = 100;
-	u32 request = FEC_MII_READ_FRAME;
+
+	value |= (phy_id << FEC_MII_DATA_PA_SHIFT) & FEC_MII_DATA_PA_MSK;
+	value |= (reg << FEC_MII_DATA_RA_SHIFT) & FEC_MII_DATA_RA_MSK;
 
 	fec = priv->regs;
 	out_be32(&fec->ievent, FEC_IEVENT_MII);
-
-	request |= (phy_id << FEC_MII_DATA_PA_SHIFT) & FEC_MII_DATA_PA_MSK;
-	request |= (reg << FEC_MII_DATA_RA_SHIFT) & FEC_MII_DATA_RA_MSK;
-
-	out_be32(&priv->regs->mii_data, request);
+	out_be32(&priv->regs->mii_data, value);
 
 	/* wait for it to finish, this takes about 23 us on lite5200b */
 	while (!(in_be32(&fec->ievent) & FEC_IEVENT_MII) && --tries)
 		udelay(5);
 
-	if (tries == 0)
+	if (!tries)
 		return -ETIMEDOUT;
 
-	return in_be32(&priv->regs->mii_data) & FEC_MII_DATA_DATAMSK;
+	return value & FEC_MII_DATA_OP_RD ?
+		in_be32(&priv->regs->mii_data) & FEC_MII_DATA_DATAMSK : 0;
 }
 
-static int mpc52xx_fec_mdio_write(struct mii_bus *bus, int phy_id, int reg, u16 data)
+static int mpc52xx_fec_mdio_read(struct mii_bus *bus, int phy_id, int reg)
 {
-	struct mpc52xx_fec_mdio_priv *priv = bus->priv;
-	struct mpc52xx_fec __iomem *fec;
-	u32 value = data;
-	int tries = 100;
-
-	fec = priv->regs;
-	out_be32(&fec->ievent, FEC_IEVENT_MII);
-
-	value |= FEC_MII_WRITE_FRAME;
-	value |= (phy_id << FEC_MII_DATA_PA_SHIFT) & FEC_MII_DATA_PA_MSK;
-	value |= (reg << FEC_MII_DATA_RA_SHIFT) & FEC_MII_DATA_RA_MSK;
-
-	out_be32(&priv->regs->mii_data, value);
-
-	/* wait for request to finish */
-	while (!(in_be32(&fec->ievent) & FEC_IEVENT_MII) && --tries)
-		udelay(5);
-
-	if (tries == 0)
-		return -ETIMEDOUT;
+	return mpc52xx_fec_mdio_transfer(bus, phy_id, reg, FEC_MII_READ_FRAME);
+}
 
-	return 0;
+static int mpc52xx_fec_mdio_write(struct mii_bus *bus, int phy_id, int reg,
+		u16 data)
+{
+	return mpc52xx_fec_mdio_transfer(bus, phy_id, reg,
+		data | FEC_MII_WRITE_FRAME);
 }
 
-static int mpc52xx_fec_mdio_probe(struct of_device *of, const struct of_device_id *match)
+static int mpc52xx_fec_mdio_probe(struct of_device *of,
+		const struct of_device_id *match)
 {
 	struct device *dev = &of->dev;
 	struct device_node *np = of->node;
@@ -121,7 +109,8 @@
 	dev_set_drvdata(dev, bus);
 
 	/* set MII speed */
-	out_be32(&priv->regs->mii_speed, ((mpc52xx_find_ipb_freq(of->node) >> 20) / 5) << 1);
+	out_be32(&priv->regs->mii_speed,
+		((mpc52xx_find_ipb_freq(of->node) >> 20) / 5) << 1);
 
 	err = mdiobus_register(bus);
 	if (err)

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

Re: [patch 1/2] FEC_MPC52XX_PHY: Remove obsolete code

[Grant Likely]

On Sat, Aug 16, 2008 at 04:32:00AM +0200, Wolfram Sang wrote:
> Remove last remains of the former (and broken) irq support code. The
> driver is polling only.
> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>

I'm not convinced on this one.  At the very least I need a more detailed
description of what is wrong and why the code needs to be removed.  As
far as I can tell, this code affects two different IRQ paths.  First is
the MDIO irq which tells the processor that an MDIO transaction has
completed.  Second is an IRQ from the PHY itself which can come in from
an entirely different path.

As it stands, I'm going to hold off picking this one up.

g.

> ---
>  drivers/net/fec_mpc52xx_phy.c |   20 --------------------
>  1 file changed, 20 deletions(-)
> 
> Index: .kernel/drivers/net/fec_mpc52xx_phy.c
> ===================================================================
> --- .kernel.orig/drivers/net/fec_mpc52xx_phy.c
> +++ .kernel/drivers/net/fec_mpc52xx_phy.c
> @@ -76,7 +76,6 @@
>  {
>  	struct device *dev = &of->dev;
>  	struct device_node *np = of->node;
> -	struct device_node *child = NULL;
>  	struct mii_bus *bus;
>  	struct mpc52xx_fec_mdio_priv *priv;
>  	struct resource res = {};
> @@ -105,15 +104,6 @@
>  	for (i=0; i<PHY_MAX_ADDR; i++)
>  		bus->irq[i] = PHY_POLL;
>  
> -	while ((child = of_get_next_child(np, child)) != NULL) {
> -		int irq = irq_of_parse_and_map(child, 0);
> -		if (irq != NO_IRQ) {
> -			const u32 *id = of_get_property(child, "reg", NULL);
> -			if (id)
> -				bus->irq[*id] = irq;
> -		}
> -	}
> -
>  	/* setup registers */
>  	err = of_address_to_resource(np, 0, &res);
>  	if (err)
> @@ -133,9 +123,6 @@
>  	/* set MII speed */
>  	out_be32(&priv->regs->mii_speed, ((mpc52xx_find_ipb_freq(of->node) >> 20) / 5) << 1);
>  
> -	/* enable MII interrupt */
> -	out_be32(&priv->regs->imask, in_be32(&priv->regs->imask) | FEC_IMASK_MII);
> -
>  	err = mdiobus_register(bus);
>  	if (err)
>  		goto out_unmap;
> @@ -145,9 +132,6 @@
>   out_unmap:
>  	iounmap(priv->regs);
>   out_free:
> -	for (i=0; i<PHY_MAX_ADDR; i++)
> -		if (bus->irq[i] != PHY_POLL)
> -			irq_dispose_mapping(bus->irq[i]);
>  	kfree(bus->irq);
>  	kfree(priv);
>  	kfree(bus);
> @@ -160,15 +144,11 @@
>  	struct device *dev = &of->dev;
>  	struct mii_bus *bus = dev_get_drvdata(dev);
>  	struct mpc52xx_fec_mdio_priv *priv = bus->priv;
> -	int i;
>  
>  	mdiobus_unregister(bus);
>  	dev_set_drvdata(dev, NULL);
>  
>  	iounmap(priv->regs);
> -	for (i=0; i<PHY_MAX_ADDR; i++)
> -		if (bus->irq[i])
> -			irq_dispose_mapping(bus->irq[i]);
>  	kfree(priv);
>  	kfree(bus->irq);
>  	kfree(bus);
> 
> -- 
>   Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
>  Pengutronix - Linux Solutions for Science and Industry
> _______________________________________________
> Linuxppc-embedded mailing list
> Linuxppc-embedded@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-embedded

Re: [patch 2/2] FEC_MPC52XX_PHY: Refactor read/write routines

[Grant Likely]

On Sat, Aug 16, 2008 at 04:32:01AM +0200, Wolfram Sang wrote:
> Read & write functions now call a generic transfer function, so identical
> code in both routines could be eliminated. The result is easier to maintain
> and smaller in source and binary code. Also, fix some checkpatch warnings.

This looks good.  I'll pick it up.

g.

> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> ---
>  drivers/net/fec_mpc52xx_phy.c |   55 ++++++++++++++++--------------------------
>  1 file changed, 22 insertions(+), 33 deletions(-)
> 
> Index: .kernel/drivers/net/fec_mpc52xx_phy.c
> ===================================================================
> --- .kernel.orig/drivers/net/fec_mpc52xx_phy.c
> +++ .kernel/drivers/net/fec_mpc52xx_phy.c
> @@ -2,6 +2,7 @@
>   * Driver for the MPC5200 Fast Ethernet Controller - MDIO bus driver
>   *
>   * Copyright (C) 2007  Domen Puncer, Telargo, Inc.
> + * Copyright (C) 2008  Wolfram Sang, Pengutronix
>   *
>   * This file is licensed under the terms of the GNU General Public License
>   * version 2. This program is licensed "as is" without any warranty of any
> @@ -21,58 +22,45 @@
>  	struct mpc52xx_fec __iomem *regs;
>  };
>  
> -static int mpc52xx_fec_mdio_read(struct mii_bus *bus, int phy_id, int reg)
> +static int mpc52xx_fec_mdio_transfer(struct mii_bus *bus, int phy_id,
> +		int reg, u32 value)
>  {
>  	struct mpc52xx_fec_mdio_priv *priv = bus->priv;
>  	struct mpc52xx_fec __iomem *fec;
>  	int tries = 100;
> -	u32 request = FEC_MII_READ_FRAME;
> +
> +	value |= (phy_id << FEC_MII_DATA_PA_SHIFT) & FEC_MII_DATA_PA_MSK;
> +	value |= (reg << FEC_MII_DATA_RA_SHIFT) & FEC_MII_DATA_RA_MSK;
>  
>  	fec = priv->regs;
>  	out_be32(&fec->ievent, FEC_IEVENT_MII);
> -
> -	request |= (phy_id << FEC_MII_DATA_PA_SHIFT) & FEC_MII_DATA_PA_MSK;
> -	request |= (reg << FEC_MII_DATA_RA_SHIFT) & FEC_MII_DATA_RA_MSK;
> -
> -	out_be32(&priv->regs->mii_data, request);
> +	out_be32(&priv->regs->mii_data, value);
>  
>  	/* wait for it to finish, this takes about 23 us on lite5200b */
>  	while (!(in_be32(&fec->ievent) & FEC_IEVENT_MII) && --tries)
>  		udelay(5);
>  
> -	if (tries == 0)
> +	if (!tries)
>  		return -ETIMEDOUT;
>  
> -	return in_be32(&priv->regs->mii_data) & FEC_MII_DATA_DATAMSK;
> +	return value & FEC_MII_DATA_OP_RD ?
> +		in_be32(&priv->regs->mii_data) & FEC_MII_DATA_DATAMSK : 0;
>  }
>  
> -static int mpc52xx_fec_mdio_write(struct mii_bus *bus, int phy_id, int reg, u16 data)
> +static int mpc52xx_fec_mdio_read(struct mii_bus *bus, int phy_id, int reg)
>  {
> -	struct mpc52xx_fec_mdio_priv *priv = bus->priv;
> -	struct mpc52xx_fec __iomem *fec;
> -	u32 value = data;
> -	int tries = 100;
> -
> -	fec = priv->regs;
> -	out_be32(&fec->ievent, FEC_IEVENT_MII);
> -
> -	value |= FEC_MII_WRITE_FRAME;
> -	value |= (phy_id << FEC_MII_DATA_PA_SHIFT) & FEC_MII_DATA_PA_MSK;
> -	value |= (reg << FEC_MII_DATA_RA_SHIFT) & FEC_MII_DATA_RA_MSK;
> -
> -	out_be32(&priv->regs->mii_data, value);
> -
> -	/* wait for request to finish */
> -	while (!(in_be32(&fec->ievent) & FEC_IEVENT_MII) && --tries)
> -		udelay(5);
> -
> -	if (tries == 0)
> -		return -ETIMEDOUT;
> +	return mpc52xx_fec_mdio_transfer(bus, phy_id, reg, FEC_MII_READ_FRAME);
> +}
>  
> -	return 0;
> +static int mpc52xx_fec_mdio_write(struct mii_bus *bus, int phy_id, int reg,
> +		u16 data)
> +{
> +	return mpc52xx_fec_mdio_transfer(bus, phy_id, reg,
> +		data | FEC_MII_WRITE_FRAME);
>  }
>  
> -static int mpc52xx_fec_mdio_probe(struct of_device *of, const struct of_device_id *match)
> +static int mpc52xx_fec_mdio_probe(struct of_device *of,
> +		const struct of_device_id *match)
>  {
>  	struct device *dev = &of->dev;
>  	struct device_node *np = of->node;
> @@ -121,7 +109,8 @@
>  	dev_set_drvdata(dev, bus);
>  
>  	/* set MII speed */
> -	out_be32(&priv->regs->mii_speed, ((mpc52xx_find_ipb_freq(of->node) >> 20) / 5) << 1);
> +	out_be32(&priv->regs->mii_speed,
> +		((mpc52xx_find_ipb_freq(of->node) >> 20) / 5) << 1);
>  
>  	err = mdiobus_register(bus);
>  	if (err)
> 
> -- 
>   Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
>  Pengutronix - Linux Solutions for Science and Industry
> _______________________________________________
> Linuxppc-embedded mailing list
> Linuxppc-embedded@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-embedded

Re: [patch 1/2] FEC_MPC52XX_PHY: Remove obsolete code

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1756 bytes --]

On Wed, Oct 15, 2008 at 08:28:55AM -0600, Grant Likely wrote:

> description of what is wrong and why the code needs to be removed.  As
> far as I can tell, this code affects two different IRQ paths.  First is

Ups, you are right! I am sorry that I missed that somehow :( Here is
hopefully a better version, which just skips the really unnecessary
interrupt and fixes a glitch in mdio_remove:

===
[MPC52XX] fec_mpc52xx_phy: Minor cleanups

As this driver polls for a complete MDIO transaction, there is no need to enable
interrupts for it. Furthermore, make both checks for freeing MDIO-bus irqs
consistent.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
 drivers/net/fec_mpc52xx_phy.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/drivers/net/fec_mpc52xx_phy.c b/drivers/net/fec_mpc52xx_phy.c
index 08e18bc..3ad3750 100644
--- a/drivers/net/fec_mpc52xx_phy.c
+++ b/drivers/net/fec_mpc52xx_phy.c
@@ -133,9 +133,6 @@ static int mpc52xx_fec_mdio_probe(struct of_device *of, const struct of_device_i
 	/* set MII speed */
 	out_be32(&priv->regs->mii_speed, ((mpc52xx_find_ipb_freq(of->node) >> 20) / 5) << 1);
 
-	/* enable MII interrupt */
-	out_be32(&priv->regs->imask, in_be32(&priv->regs->imask) | FEC_IMASK_MII);
-
 	err = mdiobus_register(bus);
 	if (err)
 		goto out_unmap;
@@ -167,7 +164,7 @@ static int mpc52xx_fec_mdio_remove(struct of_device *of)
 
 	iounmap(priv->regs);
 	for (i=0; i<PHY_MAX_ADDR; i++)
-		if (bus->irq[i])
+		if (bus->irq[i] != PHY_POLL)
 			irq_dispose_mapping(bus->irq[i]);
 	kfree(priv);
 	kfree(bus->irq);
-- 
1.5.6.5

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]