[PATCH 00/10] net/fec: add dual fec support for i.MX28

[PATCH 00/10] net/fec: add dual fec support for i.MX28

[Shawn Guo]

This patch series is to add dual fec support for mx28, which is
a mxs-based soc. Some code changes related to the following commits
are also made in this patch set for some reasons.

  e6b043d512fa8d9a3801bf5d72bfa3b8fc3b3cc8
  netdev/fec.c: add phylib supporting to enable carrier detection (v2)

  e3fe8558c7fc182972c3d947d88744482111f304
  net/fec: fix pm to survive to suspend/resume

Also I do not understand one line change made in commit below.

  commit 6fcc040f02d281c7e9563127358a77ce2bbfe284
  net: allow FEC driver to use fixed PHY support

-	snprintf(fep->mii_bus->id, MII_BUS_ID_SIZE, "%x", pdev->id);
+	snprintf(fep->mii_bus->id, MII_BUS_ID_SIZE, "%x", pdev->id + 1);

Can someone help me understand if it's really needed?  The patch
set can work with this change, and I'm just curious.

It's been tested on mx28 evk and mx51 babbage. For mx28, it has
to work against the following patch set, which has not got
merged yet.

 [PATCH v8 00/15] ARM: mxs: Add initial support for MX23 and MX28

Thanks for review.

Regards,
Shawn

Shawn Guo (10):
 arch/arm/mach-mxs/Makefile                      |    2 +-
 arch/arm/mach-mxs/clock-mx28.c                  |    7 +-
 arch/arm/mach-mxs/devices/platform-fec.c        |    2 +-
 arch/arm/mach-mxs/include/mach/common.h         |    1 +
 arch/arm/mach-mxs/include/mach/devices-common.h |    2 +-
 arch/arm/mach-mxs/mach-mx28evk.c                |   59 ++++++-
 arch/arm/mach-mxs/ocotp.c                       |   52 ++++++
 arch/arm/mach-mxs/pm.c                          |   44 +++++
 drivers/net/Kconfig                             |    7 +-
 drivers/net/fec.c                               |  220 +++++++++++++++++------
 drivers/net/fec.h                               |    5 +-
 include/linux/fec.h                             |    2 +
 12 files changed, 335 insertions(+), 68 deletions(-)

 [PATCH 01/10] net/fec: fix MMFR_OP type in fec_enet_mdio_write
 [PATCH 02/10] net/fec: remove the use of "index" which is legacy
 [PATCH 03/10] net/fec: add mac field into platform data and consolidate fec_get_mac
 [PATCH 04/10] net/fec: improve pm for better suspend/resume
 [PATCH 05/10] net/fec: add dual fec support for mx28
 [PATCH 06/10] ARM: mx28: update clocks for dual fec support
 [PATCH 07/10] ARM: mx28: add the second fec device registration
 [PATCH 08/10] ARM: mxs: add ocotp read function
 [PATCH 09/10] ARM: mx28: read fec mac address from ocotp
 [PATCH 10/10] ARM: mxs: add initial pm support

[PATCH 01/10] net/fec: fix MMFR_OP type in fec_enet_mdio_write

[Shawn Guo]

FEC_MMFR_OP_WRITE should be used than FEC_MMFR_OP_READ in
a mdio write operation.

It's probably a typo introduced by commit:

e6b043d512fa8d9a3801bf5d72bfa3b8fc3b3cc8
netdev/fec.c: add phylib supporting to enable carrier detection (v2)

Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
---
 drivers/net/fec.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/fec.c b/drivers/net/fec.c
index cce32d4..52e9ca8 100644
--- a/drivers/net/fec.c
+++ b/drivers/net/fec.c
@@ -651,8 +651,8 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
 	fep->mii_timeout = 0;
 	init_completion(&fep->mdio_done);
 
-	/* start a read op */
-	writel(FEC_MMFR_ST | FEC_MMFR_OP_READ |
+	/* start a write op */
+	writel(FEC_MMFR_ST | FEC_MMFR_OP_WRITE |
 		FEC_MMFR_PA(mii_id) | FEC_MMFR_RA(regnum) |
 		FEC_MMFR_TA | FEC_MMFR_DATA(value),
 		fep->hwp + FEC_MII_DATA);
-- 
1.7.1

[PATCH 02/10] net/fec: remove the use of "index" which is legacy

[Shawn Guo]

The "index" becomes legacy since fep->pdev->id starts working
to identify the instance.

Moreover, the call of fec_enet_init(ndev, 0) always passes 0
to fep->index. This makes the following code in fec_get_mac buggy.

	/* Adjust MAC if using default MAC address */
	if (iap == fec_mac_default)
		dev->dev_addr[ETH_ALEN-1] = fec_mac_default[ETH_ALEN-1] + fep->index;

It may be the time to remove "index" and use fep->pdev->id instead.

Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
---
 drivers/net/fec.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/net/fec.c b/drivers/net/fec.c
index 52e9ca8..47f6b3b 100644
--- a/drivers/net/fec.c
+++ b/drivers/net/fec.c
@@ -186,7 +186,6 @@ struct fec_enet_private {
 	int     mii_timeout;
 	uint    phy_speed;
 	phy_interface_t	phy_interface;
-	int	index;
 	int	link;
 	int	full_duplex;
 	struct	completion mdio_done;
@@ -566,7 +565,7 @@ static void __inline__ fec_get_mac(struct net_device *dev)
 
 	/* Adjust MAC if using default MAC address */
 	if (iap == fec_mac_default)
-		 dev->dev_addr[ETH_ALEN-1] = fec_mac_default[ETH_ALEN-1] + fep->index;
+		 dev->dev_addr[ETH_ALEN-1] = fec_mac_default[ETH_ALEN-1] + fep->pdev->id;
 }
 #endif
 
@@ -1067,9 +1066,8 @@ static const struct net_device_ops fec_netdev_ops = {
  /*
   * XXX:  We need to clean up on failure exits here.
   *
-  * index is only used in legacy code
   */
-static int fec_enet_init(struct net_device *dev, int index)
+static int fec_enet_init(struct net_device *dev)
 {
 	struct fec_enet_private *fep = netdev_priv(dev);
 	struct bufdesc *cbd_base;
@@ -1086,7 +1084,6 @@ static int fec_enet_init(struct net_device *dev, int index)
 
 	spin_lock_init(&fep->hw_lock);
 
-	fep->index = index;
 	fep->hwp = (void __iomem *)dev->base_addr;
 	fep->netdev = dev;
 
@@ -1316,7 +1313,7 @@ fec_probe(struct platform_device *pdev)
 	}
 	clk_enable(fep->clk);
 
-	ret = fec_enet_init(ndev, 0);
+	ret = fec_enet_init(ndev);
 	if (ret)
 		goto failed_init;
 
-- 
1.7.1

[PATCH 03/10] net/fec: add mac field into platform data and consolidate fec_get_mac

[Shawn Guo]

Add mac field into fec_platform_data and consolidate function
fec_get_mac to get mac address in following order.

 1) kernel command line fec_mac=xx:xx:xx...
 2) from flash in case of CONFIG_M5272 or fec_platform_data mac
    field for others, which typically have mac stored in fuse
 3) fec mac address registers set by bootloader

Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
---
 drivers/net/fec.c   |   90 ++++++++++++++++++++++++++++----------------------
 include/linux/fec.h |    2 +
 2 files changed, 52 insertions(+), 40 deletions(-)

diff --git a/drivers/net/fec.c b/drivers/net/fec.c
index 47f6b3b..cd59814 100644
--- a/drivers/net/fec.c
+++ b/drivers/net/fec.c
@@ -59,15 +59,9 @@
 #define FEC_ALIGNMENT	0x3
 #endif
 
-/*
- * Define the fixed address of the FEC hardware.
- */
-#if defined(CONFIG_M5272)
-
-static unsigned char	fec_mac_default[] = {
-	0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-};
+static unsigned char fec_mac_default[ETH_ALEN];
 
+#if defined(CONFIG_M5272)
 /*
  * Some hardware gets it MAC address out of local flash memory.
  * if this is non-zero then assume it is the address to get MAC from.
@@ -537,27 +531,40 @@ rx_processing_done:
 }
 
 /* ------------------------------------------------------------------------- */
-#ifdef CONFIG_M5272
 static void __inline__ fec_get_mac(struct net_device *dev)
 {
 	struct fec_enet_private *fep = netdev_priv(dev);
+	struct fec_platform_data *pdata = fep->pdev->dev.platform_data;
 	unsigned char *iap, tmpaddr[ETH_ALEN];
 
-	if (FEC_FLASHMAC) {
-		/*
-		 * Get MAC address from FLASH.
-		 * If it is all 1's or 0's, use the default.
-		 */
-		iap = (unsigned char *)FEC_FLASHMAC;
-		if ((iap[0] == 0) && (iap[1] == 0) && (iap[2] == 0) &&
-		    (iap[3] == 0) && (iap[4] == 0) && (iap[5] == 0))
-			iap = fec_mac_default;
-		if ((iap[0] == 0xff) && (iap[1] == 0xff) && (iap[2] == 0xff) &&
-		    (iap[3] == 0xff) && (iap[4] == 0xff) && (iap[5] == 0xff))
-			iap = fec_mac_default;
-	} else {
-		*((unsigned long *) &tmpaddr[0]) = readl(fep->hwp + FEC_ADDR_LOW);
-		*((unsigned short *) &tmpaddr[4]) = (readl(fep->hwp + FEC_ADDR_HIGH) >> 16);
+	/*
+	 * try to get mac address in following order:
+	 *
+	 * 1) kernel command line fec_mac=xx:xx:xx...
+	 */
+	iap = fec_mac_default;
+
+	/*
+	 * 2) from flash or fuse (via platform data)
+	 */
+	if (!is_valid_ether_addr(iap)) {
+#ifdef CONFIG_M5272
+		if (FEC_FLASHMAC)
+			iap = (unsigned char *)FEC_FLASHMAC;
+#else
+		if (pdata)
+			memcpy(iap, pdata->mac, ETH_ALEN);
+#endif
+	}
+
+	/*
+	 * 3) FEC mac registers set by bootloader
+	 */
+	if (!is_valid_ether_addr(iap)) {
+		*((unsigned long *) &tmpaddr[0]) =
+			be32_to_cpu(readl(fep->hwp + FEC_ADDR_LOW));
+		*((unsigned short *) &tmpaddr[4]) =
+			be16_to_cpu(readl(fep->hwp + FEC_ADDR_HIGH) >> 16);
 		iap = &tmpaddr[0];
 	}
 
@@ -567,7 +574,6 @@ static void __inline__ fec_get_mac(struct net_device *dev)
 	if (iap == fec_mac_default)
 		 dev->dev_addr[ETH_ALEN-1] = fec_mac_default[ETH_ALEN-1] + fep->pdev->id;
 }
-#endif
 
 /* ------------------------------------------------------------------------- */
 
@@ -1063,6 +1069,24 @@ static const struct net_device_ops fec_netdev_ops = {
 	.ndo_do_ioctl           = fec_enet_ioctl,
 };
 
+static int __init fec_mac_addr_setup(char *mac_addr)
+{
+	int i;
+	unsigned int tmp;
+
+	for (i = 0; i < ETH_ALEN; i++) {
+		if (sscanf(mac_addr + 3*i, "%2x", &tmp) != 1) {
+			printk(KERN_WARNING "Malformed fec mac address\n");
+			return 0;
+		}
+		fec_mac_default[i] = tmp;
+	}
+
+	return 1;
+}
+
+__setup("fec_mac=", fec_mac_addr_setup);
+
  /*
   * XXX:  We need to clean up on failure exits here.
   *
@@ -1087,22 +1111,8 @@ static int fec_enet_init(struct net_device *dev)
 	fep->hwp = (void __iomem *)dev->base_addr;
 	fep->netdev = dev;
 
-	/* Set the Ethernet address */
-#ifdef CONFIG_M5272
+	/* Get the Ethernet address */
 	fec_get_mac(dev);
-#else
-	{
-		unsigned long l;
-		l = readl(fep->hwp + FEC_ADDR_LOW);
-		dev->dev_addr[0] = (unsigned char)((l & 0xFF000000) >> 24);
-		dev->dev_addr[1] = (unsigned char)((l & 0x00FF0000) >> 16);
-		dev->dev_addr[2] = (unsigned char)((l & 0x0000FF00) >> 8);
-		dev->dev_addr[3] = (unsigned char)((l & 0x000000FF) >> 0);
-		l = readl(fep->hwp + FEC_ADDR_HIGH);
-		dev->dev_addr[4] = (unsigned char)((l & 0xFF000000) >> 24);
-		dev->dev_addr[5] = (unsigned char)((l & 0x00FF0000) >> 16);
-	}
-#endif
 
 	/* Set receive and transmit descriptor base. */
 	fep->rx_bd_base = cbd_base;
diff --git a/include/linux/fec.h b/include/linux/fec.h
index 5d3523d..bf0c69f 100644
--- a/include/linux/fec.h
+++ b/include/linux/fec.h
@@ -1,6 +1,7 @@
 /* include/linux/fec.h
  *
  * Copyright (c) 2009 Orex Computed Radiography
+ * Copyright (C) 2010 Freescale Semiconductor, Inc.
  *   Baruch Siach <baruch@tkos.co.il>
  *
  * Header file for the FEC platform data
@@ -16,6 +17,7 @@
 
 struct fec_platform_data {
 	phy_interface_t phy;
+	unsigned char mac[ETH_ALEN];
 };
 
 #endif
-- 
1.7.1

[PATCH 03/10] net/fec: add mac field into platform data and consolidate fec_get_mac

[Greg Ungerer]

Hi Shawn,

On 29/12/10 00:55, Shawn Guo wrote:
> Add mac field into fec_platform_data and consolidate function
> fec_get_mac to get mac address in following order.
>
>   1) kernel command line fec_mac=xx:xx:xx...
>   2) from flash in case of CONFIG_M5272 or fec_platform_data mac
>      field for others, which typically have mac stored in fuse

Looking at this we can move that 5272 specific code into its
arch code with these changes. I can do that later if you don't
want to do it in this series.

Regards
Greg


>   3) fec mac address registers set by bootloader
>
> Signed-off-by: Shawn Guo<shawn.guo@freescale.com>
> ---
>   drivers/net/fec.c   |   90 ++++++++++++++++++++++++++++----------------------
>   include/linux/fec.h |    2 +
>   2 files changed, 52 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/net/fec.c b/drivers/net/fec.c
> index 47f6b3b..cd59814 100644
> --- a/drivers/net/fec.c
> +++ b/drivers/net/fec.c
> @@ -59,15 +59,9 @@
>   #define FEC_ALIGNMENT	0x3
>   #endif
>
> -/*
> - * Define the fixed address of the FEC hardware.
> - */
> -#if defined(CONFIG_M5272)
> -
> -static unsigned char	fec_mac_default[] = {
> -	0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> -};
> +static unsigned char fec_mac_default[ETH_ALEN];
>
> +#if defined(CONFIG_M5272)
>   /*
>    * Some hardware gets it MAC address out of local flash memory.
>    * if this is non-zero then assume it is the address to get MAC from.
> @@ -537,27 +531,40 @@ rx_processing_done:
>   }
>
>   /* ------------------------------------------------------------------------- */
> -#ifdef CONFIG_M5272
>   static void __inline__ fec_get_mac(struct net_device *dev)
>   {
>   	struct fec_enet_private *fep = netdev_priv(dev);
> +	struct fec_platform_data *pdata = fep->pdev->dev.platform_data;
>   	unsigned char *iap, tmpaddr[ETH_ALEN];
>
> -	if (FEC_FLASHMAC) {
> -		/*
> -		 * Get MAC address from FLASH.
> -		 * If it is all 1's or 0's, use the default.
> -		 */
> -		iap = (unsigned char *)FEC_FLASHMAC;
> -		if ((iap[0] == 0)&&  (iap[1] == 0)&&  (iap[2] == 0)&&
> -		    (iap[3] == 0)&&  (iap[4] == 0)&&  (iap[5] == 0))
> -			iap = fec_mac_default;
> -		if ((iap[0] == 0xff)&&  (iap[1] == 0xff)&&  (iap[2] == 0xff)&&
> -		    (iap[3] == 0xff)&&  (iap[4] == 0xff)&&  (iap[5] == 0xff))
> -			iap = fec_mac_default;
> -	} else {
> -		*((unsigned long *)&tmpaddr[0]) = readl(fep->hwp + FEC_ADDR_LOW);
> -		*((unsigned short *)&tmpaddr[4]) = (readl(fep->hwp + FEC_ADDR_HIGH)>>  16);
> +	/*
> +	 * try to get mac address in following order:
> +	 *
> +	 * 1) kernel command line fec_mac=xx:xx:xx...
> +	 */
> +	iap = fec_mac_default;
> +
> +	/*
> +	 * 2) from flash or fuse (via platform data)
> +	 */
> +	if (!is_valid_ether_addr(iap)) {
> +#ifdef CONFIG_M5272
> +		if (FEC_FLASHMAC)
> +			iap = (unsigned char *)FEC_FLASHMAC;
> +#else
> +		if (pdata)
> +			memcpy(iap, pdata->mac, ETH_ALEN);
> +#endif
> +	}
> +
> +	/*
> +	 * 3) FEC mac registers set by bootloader
> +	 */
> +	if (!is_valid_ether_addr(iap)) {
> +		*((unsigned long *)&tmpaddr[0]) =
> +			be32_to_cpu(readl(fep->hwp + FEC_ADDR_LOW));
> +		*((unsigned short *)&tmpaddr[4]) =
> +			be16_to_cpu(readl(fep->hwp + FEC_ADDR_HIGH)>>  16);
>   		iap =&tmpaddr[0];
>   	}
>
> @@ -567,7 +574,6 @@ static void __inline__ fec_get_mac(struct net_device *dev)
>   	if (iap == fec_mac_default)
>   		 dev->dev_addr[ETH_ALEN-1] = fec_mac_default[ETH_ALEN-1] + fep->pdev->id;
>   }
> -#endif
>
>   /* ------------------------------------------------------------------------- */
>
> @@ -1063,6 +1069,24 @@ static const struct net_device_ops fec_netdev_ops = {
>   	.ndo_do_ioctl           = fec_enet_ioctl,
>   };
>
> +static int __init fec_mac_addr_setup(char *mac_addr)
> +{
> +	int i;
> +	unsigned int tmp;
> +
> +	for (i = 0; i<  ETH_ALEN; i++) {
> +		if (sscanf(mac_addr + 3*i, "%2x",&tmp) != 1) {
> +			printk(KERN_WARNING "Malformed fec mac address\n");
> +			return 0;
> +		}
> +		fec_mac_default[i] = tmp;
> +	}
> +
> +	return 1;
> +}
> +
> +__setup("fec_mac=", fec_mac_addr_setup);
> +
>    /*
>     * XXX:  We need to clean up on failure exits here.
>     *
> @@ -1087,22 +1111,8 @@ static int fec_enet_init(struct net_device *dev)
>   	fep->hwp = (void __iomem *)dev->base_addr;
>   	fep->netdev = dev;
>
> -	/* Set the Ethernet address */
> -#ifdef CONFIG_M5272
> +	/* Get the Ethernet address */
>   	fec_get_mac(dev);
> -#else
> -	{
> -		unsigned long l;
> -		l = readl(fep->hwp + FEC_ADDR_LOW);
> -		dev->dev_addr[0] = (unsigned char)((l&  0xFF000000)>>  24);
> -		dev->dev_addr[1] = (unsigned char)((l&  0x00FF0000)>>  16);
> -		dev->dev_addr[2] = (unsigned char)((l&  0x0000FF00)>>  8);
> -		dev->dev_addr[3] = (unsigned char)((l&  0x000000FF)>>  0);
> -		l = readl(fep->hwp + FEC_ADDR_HIGH);
> -		dev->dev_addr[4] = (unsigned char)((l&  0xFF000000)>>  24);
> -		dev->dev_addr[5] = (unsigned char)((l&  0x00FF0000)>>  16);
> -	}
> -#endif
>
>   	/* Set receive and transmit descriptor base. */
>   	fep->rx_bd_base = cbd_base;
> diff --git a/include/linux/fec.h b/include/linux/fec.h
> index 5d3523d..bf0c69f 100644
> --- a/include/linux/fec.h
> +++ b/include/linux/fec.h
> @@ -1,6 +1,7 @@
>   /* include/linux/fec.h
>    *
>    * Copyright (c) 2009 Orex Computed Radiography
> + * Copyright (C) 2010 Freescale Semiconductor, Inc.
>    *   Baruch Siach<baruch@tkos.co.il>
>    *
>    * Header file for the FEC platform data
> @@ -16,6 +17,7 @@
>
>   struct fec_platform_data {
>   	phy_interface_t phy;
> +	unsigned char mac[ETH_ALEN];
>   };
>
>   #endif


-- 
------------------------------------------------------------------------
Greg Ungerer  --  Principal Engineer        EMAIL:     gerg at snapgear.com
SnapGear Group, McAfee                      PHONE:       +61 7 3435 2888
8 Gardner Close                             FAX:         +61 7 3217 5323
Milton, QLD, 4064, Australia                WEB: http://www.SnapGear.com

[PATCH 03/10] net/fec: add mac field into platform data and consolidate fec_get_mac

[Shawn Guo]

Hi Greg,

On Wed, Dec 29, 2010 at 03:29:01PM +1000, Greg Ungerer wrote:
> Hi Shawn,
> 
> On 29/12/10 00:55, Shawn Guo wrote:
> >Add mac field into fec_platform_data and consolidate function
> >fec_get_mac to get mac address in following order.
> >
> >  1) kernel command line fec_mac=xx:xx:xx...
> >  2) from flash in case of CONFIG_M5272 or fec_platform_data mac
> >     field for others, which typically have mac stored in fuse
> 
> Looking at this we can move that 5272 specific code into its
> arch code with these changes. I can do that later if you don't
> want to do it in this series.
> 
It would be great if you can do that, since I do not have the
the platform to test anyway.

-- 
Regards,
Shawn

[PATCH 03/10] net/fec: add mac field into platform data and consolidate fec_get_mac

[Baruch Siach]

Hi Shawn,

Please add netdev at vger.kernel.org to the Cc of all your fec driver patches.

On Tue, Dec 28, 2010 at 10:55:48PM +0800, Shawn Guo wrote:
> Add mac field into fec_platform_data and consolidate function
> fec_get_mac to get mac address in following order.
> 
>  1) kernel command line fec_mac=xx:xx:xx...

In the case of dual MAC that you later add support for, the address which one 
is being set? Is there a way to set both in kernel command line?

Also, instead of inventing another kernel command line parameter, it is better 
IMO to use the module parameters mechanism. The greth NIC driver already uses 
this method for setting MAC address. Look for 'macaddr' in 
drivers/net/greth.c, and the explanation at the beginning of 
Documentation/kernel-parameters.txt.

>  2) from flash in case of CONFIG_M5272 or fec_platform_data mac
>     field for others, which typically have mac stored in fuse
>  3) fec mac address registers set by bootloader
> 
> Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> ---
>  drivers/net/fec.c   |   90 ++++++++++++++++++++++++++++----------------------
>  include/linux/fec.h |    2 +
>  2 files changed, 52 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/net/fec.c b/drivers/net/fec.c
> index 47f6b3b..cd59814 100644
> --- a/drivers/net/fec.c
> +++ b/drivers/net/fec.c
> @@ -59,15 +59,9 @@
>  #define FEC_ALIGNMENT	0x3
>  #endif
>  
> -/*
> - * Define the fixed address of the FEC hardware.
> - */
> -#if defined(CONFIG_M5272)
> -
> -static unsigned char	fec_mac_default[] = {
> -	0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> -};
> +static unsigned char fec_mac_default[ETH_ALEN];
>  
> +#if defined(CONFIG_M5272)
>  /*
>   * Some hardware gets it MAC address out of local flash memory.
>   * if this is non-zero then assume it is the address to get MAC from.
> @@ -537,27 +531,40 @@ rx_processing_done:
>  }
>  
>  /* ------------------------------------------------------------------------- */
> -#ifdef CONFIG_M5272
>  static void __inline__ fec_get_mac(struct net_device *dev)
>  {
>  	struct fec_enet_private *fep = netdev_priv(dev);
> +	struct fec_platform_data *pdata = fep->pdev->dev.platform_data;
>  	unsigned char *iap, tmpaddr[ETH_ALEN];
>  
> -	if (FEC_FLASHMAC) {
> -		/*
> -		 * Get MAC address from FLASH.
> -		 * If it is all 1's or 0's, use the default.
> -		 */
> -		iap = (unsigned char *)FEC_FLASHMAC;
> -		if ((iap[0] == 0) && (iap[1] == 0) && (iap[2] == 0) &&
> -		    (iap[3] == 0) && (iap[4] == 0) && (iap[5] == 0))
> -			iap = fec_mac_default;
> -		if ((iap[0] == 0xff) && (iap[1] == 0xff) && (iap[2] == 0xff) &&
> -		    (iap[3] == 0xff) && (iap[4] == 0xff) && (iap[5] == 0xff))
> -			iap = fec_mac_default;
> -	} else {
> -		*((unsigned long *) &tmpaddr[0]) = readl(fep->hwp + FEC_ADDR_LOW);
> -		*((unsigned short *) &tmpaddr[4]) = (readl(fep->hwp + FEC_ADDR_HIGH) >> 16);
> +	/*
> +	 * try to get mac address in following order:
> +	 *
> +	 * 1) kernel command line fec_mac=xx:xx:xx...
> +	 */
> +	iap = fec_mac_default;
> +
> +	/*
> +	 * 2) from flash or fuse (via platform data)
> +	 */

Again, how do you handle the dual MAC case?

> +	if (!is_valid_ether_addr(iap)) {
> +#ifdef CONFIG_M5272
> +		if (FEC_FLASHMAC)
> +			iap = (unsigned char *)FEC_FLASHMAC;
> +#else
> +		if (pdata)
> +			memcpy(iap, pdata->mac, ETH_ALEN);
> +#endif
> +	}
> +
> +	/*
> +	 * 3) FEC mac registers set by bootloader
> +	 */
> +	if (!is_valid_ether_addr(iap)) {
> +		*((unsigned long *) &tmpaddr[0]) =
> +			be32_to_cpu(readl(fep->hwp + FEC_ADDR_LOW));
> +		*((unsigned short *) &tmpaddr[4]) =
> +			be16_to_cpu(readl(fep->hwp + FEC_ADDR_HIGH) >> 16);
>  		iap = &tmpaddr[0];
>  	}
>  
> @@ -567,7 +574,6 @@ static void __inline__ fec_get_mac(struct net_device *dev)
>  	if (iap == fec_mac_default)
>  		 dev->dev_addr[ETH_ALEN-1] = fec_mac_default[ETH_ALEN-1] + fep->pdev->id;
>  }
> -#endif
>  
>  /* ------------------------------------------------------------------------- */
>  
> @@ -1063,6 +1069,24 @@ static const struct net_device_ops fec_netdev_ops = {
>  	.ndo_do_ioctl           = fec_enet_ioctl,
>  };
>  
> +static int __init fec_mac_addr_setup(char *mac_addr)
> +{
> +	int i;
> +	unsigned int tmp;
> +
> +	for (i = 0; i < ETH_ALEN; i++) {
> +		if (sscanf(mac_addr + 3*i, "%2x", &tmp) != 1) {
> +			printk(KERN_WARNING "Malformed fec mac address\n");
> +			return 0;
> +		}
> +		fec_mac_default[i] = tmp;
> +	}
> +
> +	return 1;
> +}
> +
> +__setup("fec_mac=", fec_mac_addr_setup);
> +
>   /*
>    * XXX:  We need to clean up on failure exits here.
>    *
> @@ -1087,22 +1111,8 @@ static int fec_enet_init(struct net_device *dev)
>  	fep->hwp = (void __iomem *)dev->base_addr;
>  	fep->netdev = dev;
>  
> -	/* Set the Ethernet address */
> -#ifdef CONFIG_M5272
> +	/* Get the Ethernet address */
>  	fec_get_mac(dev);
> -#else
> -	{
> -		unsigned long l;
> -		l = readl(fep->hwp + FEC_ADDR_LOW);
> -		dev->dev_addr[0] = (unsigned char)((l & 0xFF000000) >> 24);
> -		dev->dev_addr[1] = (unsigned char)((l & 0x00FF0000) >> 16);
> -		dev->dev_addr[2] = (unsigned char)((l & 0x0000FF00) >> 8);
> -		dev->dev_addr[3] = (unsigned char)((l & 0x000000FF) >> 0);
> -		l = readl(fep->hwp + FEC_ADDR_HIGH);
> -		dev->dev_addr[4] = (unsigned char)((l & 0xFF000000) >> 24);
> -		dev->dev_addr[5] = (unsigned char)((l & 0x00FF0000) >> 16);
> -	}
> -#endif
>  
>  	/* Set receive and transmit descriptor base. */
>  	fep->rx_bd_base = cbd_base;
> diff --git a/include/linux/fec.h b/include/linux/fec.h
> index 5d3523d..bf0c69f 100644
> --- a/include/linux/fec.h
> +++ b/include/linux/fec.h
> @@ -1,6 +1,7 @@
>  /* include/linux/fec.h
>   *
>   * Copyright (c) 2009 Orex Computed Radiography
> + * Copyright (C) 2010 Freescale Semiconductor, Inc.
>   *   Baruch Siach <baruch@tkos.co.il>

This breaks the original copyright notice. Please move the Freescale copyright 
notice below the original one.

>   *
>   * Header file for the FEC platform data
> @@ -16,6 +17,7 @@
>  
>  struct fec_platform_data {
>  	phy_interface_t phy;
> +	unsigned char mac[ETH_ALEN];
>  };
>  
>  #endif

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH 03/10] net/fec: add mac field into platform data and consolidate fec_get_mac

[Shawn Guo]

Hi Baruch,

On Wed, Dec 29, 2010 at 08:53:30AM +0200, Baruch Siach wrote:
> Hi Shawn,
> 
> Please add netdev at vger.kernel.org to the Cc of all your fec driver patches.
> 
> On Tue, Dec 28, 2010 at 10:55:48PM +0800, Shawn Guo wrote:
> > Add mac field into fec_platform_data and consolidate function
> > fec_get_mac to get mac address in following order.
> > 
> >  1) kernel command line fec_mac=xx:xx:xx...
> 
> In the case of dual MAC that you later add support for, the address which one 
> is being set? Is there a way to set both in kernel command line?
> 
The fec0 gets fec_mac and fec1 gets fec_mac + 1. The following code
in function fec_get_mac takes care of it.

	/* Adjust MAC if using default MAC address */
	if (iap == fec_mac_default)
		dev->dev_addr[ETH_ALEN-1] = fec_mac_default[ETH_ALEN-1] + fep->pdev->id;


> Also, instead of inventing another kernel command line parameter, it is better 
> IMO to use the module parameters mechanism. The greth NIC driver already uses 
> this method for setting MAC address. Look for 'macaddr' in 
> drivers/net/greth.c, and the explanation at the beginning of 
> Documentation/kernel-parameters.txt.
> 
Sounds good. Thanks.

[...]

> > +	/*
> > +	 * try to get mac address in following order:
> > +	 *
> > +	 * 1) kernel command line fec_mac=xx:xx:xx...
> > +	 */
> > +	iap = fec_mac_default;
> > +
> > +	/*
> > +	 * 2) from flash or fuse (via platform data)
> > +	 */
> 
> Again, how do you handle the dual MAC case?
> 
See above, please.

[...]

> > @@ -1,6 +1,7 @@
> >  /* include/linux/fec.h
> >   *
> >   * Copyright (c) 2009 Orex Computed Radiography
> > + * Copyright (C) 2010 Freescale Semiconductor, Inc.
> >   *   Baruch Siach <baruch@tkos.co.il>
> 
> This breaks the original copyright notice. Please move the Freescale copyright 
> notice below the original one.
> 
Sorry. Will fix it.

-- 
Regards,
Shawn

[PATCH 03/10] net/fec: add mac field into platform data and consolidate fec_get_mac

[Uwe Kleine-König]

Hello Shawn,

On Wed, Dec 29, 2010 at 06:05:21PM +0800, Shawn Guo wrote:
> On Wed, Dec 29, 2010 at 08:53:30AM +0200, Baruch Siach wrote:
> > Hi Shawn,
> > In the case of dual MAC that you later add support for, the address which one 
> > is being set? Is there a way to set both in kernel command line?
> > 
> The fec0 gets fec_mac and fec1 gets fec_mac + 1. The following code
> in function fec_get_mac takes care of it.
> 
> 	/* Adjust MAC if using default MAC address */
default MAC address means the address passed via cmdline?  Sounds
confusing to me.

> 	if (iap == fec_mac_default)
> 		dev->dev_addr[ETH_ALEN-1] = fec_mac_default[ETH_ALEN-1] + fep->pdev->id;
Can this overflow?  (I didn't check the code, so my concern might be
completely stupid here.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 03/10] net/fec: add mac field into platform data and consolidate fec_get_mac

[Shawn Guo]

Hi Uwe,

On Wed, Dec 29, 2010 at 11:31:38AM +0100, Uwe Kleine-K?nig wrote:
> Hello Shawn,
> 
> On Wed, Dec 29, 2010 at 06:05:21PM +0800, Shawn Guo wrote:
> > On Wed, Dec 29, 2010 at 08:53:30AM +0200, Baruch Siach wrote:
> > > Hi Shawn,
> > > In the case of dual MAC that you later add support for, the address which one 
> > > is being set? Is there a way to set both in kernel command line?
> > > 
> > The fec0 gets fec_mac and fec1 gets fec_mac + 1. The following code
> > in function fec_get_mac takes care of it.
> > 
> > 	/* Adjust MAC if using default MAC address */
> default MAC address means the address passed via cmdline?  Sounds
> confusing to me.
Will change the comment and fec_mac_default name to address
the concern.

> 
> > 	if (iap == fec_mac_default)
> > 		dev->dev_addr[ETH_ALEN-1] = fec_mac_default[ETH_ALEN-1] + fep->pdev->id;
> Can this overflow?  (I didn't check the code, so my concern might be
> completely stupid here.)
No. dev->dev_addr points to netdev_hw_addr->addr, which is a 32 bytes array.

-- 
Regards,
Shawn

[PATCH 03/10] net/fec: add mac field into platform data and consolidate fec_get_mac

[Uwe Kleine-König]

Hello Shawn,

On Wed, Dec 29, 2010 at 07:58:09PM +0800, Shawn Guo wrote:
> On Wed, Dec 29, 2010 at 11:31:38AM +0100, Uwe Kleine-K?nig wrote:
> > On Wed, Dec 29, 2010 at 06:05:21PM +0800, Shawn Guo wrote:
> > > On Wed, Dec 29, 2010 at 08:53:30AM +0200, Baruch Siach wrote:
> > > 	if (iap == fec_mac_default)
> > > 		dev->dev_addr[ETH_ALEN-1] = fec_mac_default[ETH_ALEN-1] + fep->pdev->id;
> > Can this overflow?  (I didn't check the code, so my concern might be
> > completely stupid here.)
> No. dev->dev_addr points to netdev_hw_addr->addr, which is a 32 bytes array.
I didn't mean an out-of-bound access, but what is if
fec_mac_default[ETH_ALEN-1] is 0xff and you add 1?  Does that result in
0x100 or 0?  What if id is <0?  For big ids you might even handle a
carry to indixes <ETH_ALEN-2.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 03/10] net/fec: add mac field into platform data and consolidate fec_get_mac

[Shawn Guo]

Hi Uwe,

On Wed, Dec 29, 2010 at 01:42:21PM +0100, Uwe Kleine-K?nig wrote:
> Hello Shawn,
> 
> On Wed, Dec 29, 2010 at 07:58:09PM +0800, Shawn Guo wrote:
> > On Wed, Dec 29, 2010 at 11:31:38AM +0100, Uwe Kleine-K?nig wrote:
> > > On Wed, Dec 29, 2010 at 06:05:21PM +0800, Shawn Guo wrote:
> > > > On Wed, Dec 29, 2010 at 08:53:30AM +0200, Baruch Siach wrote:
> > > > 	if (iap == fec_mac_default)
> > > > 		dev->dev_addr[ETH_ALEN-1] = fec_mac_default[ETH_ALEN-1] + fep->pdev->id;
> > > Can this overflow?  (I didn't check the code, so my concern might be
> > > completely stupid here.)
> > No. dev->dev_addr points to netdev_hw_addr->addr, which is a 32 bytes array.
> I didn't mean an out-of-bound access, but what is if
> fec_mac_default[ETH_ALEN-1] is 0xff and you add 1?  Does that result in
> 0x100 or 0?  What if id is <0?  For big ids you might even handle a
> carry to indixes <ETH_ALEN-2.
> 
First of all, all my patch did is changing fep->index to,
fep->pdev->id, which should not bring any problem you are concerned.

Secondly, I do not understand how the overflow on 
fec_mac_default[ETH_ALEN-1] can result in a carry on the next array
element. Here is what I'm seeing with fec_mac=00:04:9f:01:30:ff.

eth0      Link encap:Ethernet  HWaddr 00:04:9F:01:30:FF
eth1      Link encap:Ethernet  HWaddr 00:04:9F:01:30:00

-- 
Regards,
Shawn

[PATCH 03/10] net/fec: add mac field into platform data and consolidate fec_get_mac

[Uwe Kleine-König]

Hello Shawn,

On Thu, Dec 30, 2010 at 10:12:44AM +0800, Shawn Guo wrote:
> On Wed, Dec 29, 2010 at 01:42:21PM +0100, Uwe Kleine-K?nig wrote:
> > On Wed, Dec 29, 2010 at 07:58:09PM +0800, Shawn Guo wrote:
> > > On Wed, Dec 29, 2010 at 11:31:38AM +0100, Uwe Kleine-K?nig wrote:
> > > > On Wed, Dec 29, 2010 at 06:05:21PM +0800, Shawn Guo wrote:
> > > > > On Wed, Dec 29, 2010 at 08:53:30AM +0200, Baruch Siach wrote:
> > > > > 	if (iap == fec_mac_default)
> > > > > 		dev->dev_addr[ETH_ALEN-1] = fec_mac_default[ETH_ALEN-1] + fep->pdev->id;
> > > > Can this overflow?  (I didn't check the code, so my concern might be
> > > > completely stupid here.)
> > > No. dev->dev_addr points to netdev_hw_addr->addr, which is a 32 bytes array.
> > I didn't mean an out-of-bound access, but what is if
> > fec_mac_default[ETH_ALEN-1] is 0xff and you add 1?  Does that result in
> > 0x100 or 0?  What if id is <0?  For big ids you might even handle a
> > carry to indixes <ETH_ALEN-2.
> > 
> First of all, all my patch did is changing fep->index to,
> fep->pdev->id, which should not bring any problem you are concerned.
> 
> Secondly, I do not understand how the overflow on 
> fec_mac_default[ETH_ALEN-1] can result in a carry on the next array
> element. Here is what I'm seeing with fec_mac=00:04:9f:01:30:ff.
There is no automatic carry to the next array element.  I just wondered
how overflow should be handled ...
 
> eth0      Link encap:Ethernet  HWaddr 00:04:9F:01:30:FF
> eth1      Link encap:Ethernet  HWaddr 00:04:9F:01:30:00
If this is intended, it's totally OK for me.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 03/10] net/fec: add mac field into platform data and consolidate fec_get_mac

[Shawn Guo]

Hi Baruch,

On Wed, Dec 29, 2010 at 06:05:20PM +0800, Shawn Guo wrote:
> Hi Baruch,
> 
> On Wed, Dec 29, 2010 at 08:53:30AM +0200, Baruch Siach wrote:
> > Hi Shawn,
> > 
> > Please add netdev at vger.kernel.org to the Cc of all your fec driver patches.
> > 
> > On Tue, Dec 28, 2010 at 10:55:48PM +0800, Shawn Guo wrote:
> > > Add mac field into fec_platform_data and consolidate function
> > > fec_get_mac to get mac address in following order.
> > > 
> > >  1) kernel command line fec_mac=xx:xx:xx...
> > 
> > In the case of dual MAC that you later add support for, the address which one 
> > is being set? Is there a way to set both in kernel command line?
> > 
> The fec0 gets fec_mac and fec1 gets fec_mac + 1. The following code
> in function fec_get_mac takes care of it.
> 
> 	/* Adjust MAC if using default MAC address */
> 	if (iap == fec_mac_default)
> 		dev->dev_addr[ETH_ALEN-1] = fec_mac_default[ETH_ALEN-1] + fep->pdev->id;
> 
> 
> > Also, instead of inventing another kernel command line parameter, it is better 
> > IMO to use the module parameters mechanism. The greth NIC driver already uses 
> > this method for setting MAC address. Look for 'macaddr' in 
> > drivers/net/greth.c, and the explanation at the beginning of 
> > Documentation/kernel-parameters.txt.
> > 
> Sounds good. Thanks.
> 
I just thought this over again. Module parameter might not be
the best here. fec is usually used to mount nfs during boot,
which requires a built-in driver other than module.

-- 
Regards,
Shawn

[PATCH 03/10] net/fec: add mac field into platform data and consolidate fec_get_mac

[Baruch Siach]

Hi Shawn,

On Thu, Dec 30, 2010 at 12:29:00PM +0800, Shawn Guo wrote:
> On Wed, Dec 29, 2010 at 06:05:20PM +0800, Shawn Guo wrote:
> > On Wed, Dec 29, 2010 at 08:53:30AM +0200, Baruch Siach wrote:
> > > Please add netdev at vger.kernel.org to the Cc of all your fec driver 
> > > patches.
> > > 
> > > On Tue, Dec 28, 2010 at 10:55:48PM +0800, Shawn Guo wrote:
> > > > Add mac field into fec_platform_data and consolidate function
> > > > fec_get_mac to get mac address in following order.
> > > > 
> > > >  1) kernel command line fec_mac=xx:xx:xx...
> > > 
> > > In the case of dual MAC that you later add support for, the address which one 
> > > is being set? Is there a way to set both in kernel command line?
> > > 
> > The fec0 gets fec_mac and fec1 gets fec_mac + 1. The following code
> > in function fec_get_mac takes care of it.
> > 
> > 	/* Adjust MAC if using default MAC address */
> > 	if (iap == fec_mac_default)
> > 		dev->dev_addr[ETH_ALEN-1] = fec_mac_default[ETH_ALEN-1] + fep->pdev->id;
> > > Also, instead of inventing another kernel command line parameter, it is 
> > > better IMO to use the module parameters mechanism. The greth NIC driver 
> > > already uses this method for setting MAC address. Look for 'macaddr' in 
> > > drivers/net/greth.c, and the explanation at the beginning of 
> > > Documentation/kernel-parameters.txt.
> > > 
> > Sounds good. Thanks.
> > 
> I just thought this over again. Module parameter might not be
> the best here. fec is usually used to mount nfs during boot,
> which requires a built-in driver other than module.

See the explanation at the beginning of Documentation/kernel-parameters.txt.  
You can give module parameters at the kernel command line.

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH 03/10] net/fec: add mac field into platform data and consolidate fec_get_mac

[Shawn Guo]

On Thu, Dec 30, 2010 at 07:29:25AM +0200, Baruch Siach wrote:
> Hi Shawn,
> 
> On Thu, Dec 30, 2010 at 12:29:00PM +0800, Shawn Guo wrote:
> > On Wed, Dec 29, 2010 at 06:05:20PM +0800, Shawn Guo wrote:
> > > On Wed, Dec 29, 2010 at 08:53:30AM +0200, Baruch Siach wrote:
> > > > Please add netdev at vger.kernel.org to the Cc of all your fec driver 
> > > > patches.
> > > > 
> > > > On Tue, Dec 28, 2010 at 10:55:48PM +0800, Shawn Guo wrote:
> > > > > Add mac field into fec_platform_data and consolidate function
> > > > > fec_get_mac to get mac address in following order.
> > > > > 
> > > > >  1) kernel command line fec_mac=xx:xx:xx...
> > > > 
> > > > In the case of dual MAC that you later add support for, the address which one 
> > > > is being set? Is there a way to set both in kernel command line?
> > > > 
> > > The fec0 gets fec_mac and fec1 gets fec_mac + 1. The following code
> > > in function fec_get_mac takes care of it.
> > > 
> > > 	/* Adjust MAC if using default MAC address */
> > > 	if (iap == fec_mac_default)
> > > 		dev->dev_addr[ETH_ALEN-1] = fec_mac_default[ETH_ALEN-1] + fep->pdev->id;
> > > > Also, instead of inventing another kernel command line parameter, it is 
> > > > better IMO to use the module parameters mechanism. The greth NIC driver 
> > > > already uses this method for setting MAC address. Look for 'macaddr' in 
> > > > drivers/net/greth.c, and the explanation at the beginning of 
> > > > Documentation/kernel-parameters.txt.
> > > > 
> > > Sounds good. Thanks.
> > > 
> > I just thought this over again. Module parameter might not be
> > the best here. fec is usually used to mount nfs during boot,
> > which requires a built-in driver other than module.
> 
> See the explanation at the beginning of Documentation/kernel-parameters.txt.  
> You can give module parameters at the kernel command line.
> 
Ah, my dumb.

Just tested, and it works.  Thanks, Baruch.

-- 
Regards,
Shawn

[PATCH 03/10] net/fec: add mac field into platform data and consolidate fec_get_mac

[Shawn Guo]

Sorry, Baruch.  I missed two comments in the last reply.

On Wed, Dec 29, 2010 at 08:53:30AM +0200, Baruch Siach wrote:
> Hi Shawn,
> 
> Please add netdev at vger.kernel.org to the Cc of all your fec driver patches.
> 
I was aware of this after I sent out the patch set last night.
Can I just resend all the patches in v2 and get netdev at vger.kernel.org
included?

> On Tue, Dec 28, 2010 at 10:55:48PM +0800, Shawn Guo wrote:
[...]
> > +	/*
> > +	 * try to get mac address in following order:
> > +	 *
> > +	 * 1) kernel command line fec_mac=xx:xx:xx...
> > +	 */
> > +	iap = fec_mac_default;
> > +
> > +	/*
> > +	 * 2) from flash or fuse (via platform data)
> > +	 */
> 
> Again, how do you handle the dual MAC case?
> 
For the platform data case, the following patch reads both mac
addresses.

[PATCH 09/10] ARM: mx28: read fec mac address from ocotp

+static int __init mx28evk_fec_get_mac(void)
+{
+       int i, ret;
+       u32 val;
+
+       /*
+        * OCOTP only stores the last 4 octets for each mac address,
+        * so hard-coding the first two octets as Freescale OUI (00:04:9f)
+        * is needed.
+        */
+       for (i = 0; i < 2; i++) {
+               ret = mxs_read_ocotp(0x20 + i * 0x10, 1, &val);
+               if (ret)
+                       goto error;
+
+               mx28_fec_pdata[i].mac[0] = 0x00;
+               mx28_fec_pdata[i].mac[1] = 0x04;
+               mx28_fec_pdata[i].mac[2] = (val >> 24) & 0xff;
+               mx28_fec_pdata[i].mac[3] = (val >> 16) & 0xff;
+               mx28_fec_pdata[i].mac[4] = (val >> 8) & 0xff;
+               mx28_fec_pdata[i].mac[5] = (val >> 0) & 0xff;
+       }
+
+       return 0;
+
+error:
+       pr_err("%s: timeout when reading fec mac from OCOTP\n", __func__);
+       return -ETIMEDOUT;
+}
+

-- 
Regards,
Shawn

[PATCH 03/10] net/fec: add mac field into platform data and consolidate fec_get_mac

[Uwe Kleine-König]

Hello,

On Wed, Dec 29, 2010 at 06:30:15PM +0800, Shawn Guo wrote:
> Sorry, Baruch.  I missed two comments in the last reply.
> 
> On Wed, Dec 29, 2010 at 08:53:30AM +0200, Baruch Siach wrote:
> > Hi Shawn,
> > 
> > Please add netdev at vger.kernel.org to the Cc of all your fec driver patches.
> > 
> I was aware of this after I sent out the patch set last night.
> Can I just resend all the patches in v2 and get netdev at vger.kernel.org
> included?
[I just added netdev for this reply.  I think adding it for v2 is OK
then.  For those interested, the whole series can be found at

	http://thread.gmane.org/gmane.linux.ports.arm.kernel/100902

.]
 
> > On Tue, Dec 28, 2010 at 10:55:48PM +0800, Shawn Guo wrote:
> [...]
> > > +	/*
> > > +	 * try to get mac address in following order:
> > > +	 *
> > > +	 * 1) kernel command line fec_mac=xx:xx:xx...
> > > +	 */
> > > +	iap = fec_mac_default;
> > > +
> > > +	/*
> > > +	 * 2) from flash or fuse (via platform data)
> > > +	 */
> > 
> > Again, how do you handle the dual MAC case?
> > 
> For the platform data case, the following patch reads both mac
> addresses.
> 
> [PATCH 09/10] ARM: mx28: read fec mac address from ocotp
> 
> +static int __init mx28evk_fec_get_mac(void)
> +{
> +       int i, ret;
> +       u32 val;
> +
> +       /*
> +        * OCOTP only stores the last 4 octets for each mac address,
> +        * so hard-coding the first two octets as Freescale OUI (00:04:9f)
> +        * is needed.
> +        */
> +       for (i = 0; i < 2; i++) {
> +               ret = mxs_read_ocotp(0x20 + i * 0x10, 1, &val);
> +               if (ret)
> +                       goto error;
> +
> +               mx28_fec_pdata[i].mac[0] = 0x00;
> +               mx28_fec_pdata[i].mac[1] = 0x04;
> +               mx28_fec_pdata[i].mac[2] = (val >> 24) & 0xff;
> +               mx28_fec_pdata[i].mac[3] = (val >> 16) & 0xff;
> +               mx28_fec_pdata[i].mac[4] = (val >> 8) & 0xff;
> +               mx28_fec_pdata[i].mac[5] = (val >> 0) & 0xff;
uuh.  Is ((val >> 24) & 0xff) supposed to be 0x9f?  If not this might
have unwanted effects (practical, don't know about legal ones).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 03/10] net/fec: add mac field into platform data and consolidate fec_get_mac

[Shawn Guo]

Hi Uwe,

On Wed, Dec 29, 2010 at 11:37:58AM +0100, Uwe Kleine-K?nig wrote:
> Hello,
> 
> On Wed, Dec 29, 2010 at 06:30:15PM +0800, Shawn Guo wrote:
> > Sorry, Baruch.  I missed two comments in the last reply.
> > 
> > On Wed, Dec 29, 2010 at 08:53:30AM +0200, Baruch Siach wrote:
> > > Hi Shawn,
> > > 
> > > Please add netdev at vger.kernel.org to the Cc of all your fec driver patches.
> > > 
> > I was aware of this after I sent out the patch set last night.
> > Can I just resend all the patches in v2 and get netdev at vger.kernel.org
> > included?
> [I just added netdev for this reply.  I think adding it for v2 is OK
> then.  For those interested, the whole series can be found at
> 
> 	http://thread.gmane.org/gmane.linux.ports.arm.kernel/100902
> 
> .]
>  
Thanks.

> > > On Tue, Dec 28, 2010 at 10:55:48PM +0800, Shawn Guo wrote:
> > [...]
> > > > +	/*
> > > > +	 * try to get mac address in following order:
> > > > +	 *
> > > > +	 * 1) kernel command line fec_mac=xx:xx:xx...
> > > > +	 */
> > > > +	iap = fec_mac_default;
> > > > +
> > > > +	/*
> > > > +	 * 2) from flash or fuse (via platform data)
> > > > +	 */
> > > 
> > > Again, how do you handle the dual MAC case?
> > > 
> > For the platform data case, the following patch reads both mac
> > addresses.
> > 
> > [PATCH 09/10] ARM: mx28: read fec mac address from ocotp
> > 
> > +static int __init mx28evk_fec_get_mac(void)
> > +{
> > +       int i, ret;
> > +       u32 val;
> > +
> > +       /*
> > +        * OCOTP only stores the last 4 octets for each mac address,
> > +        * so hard-coding the first two octets as Freescale OUI (00:04:9f)
> > +        * is needed.
> > +        */
> > +       for (i = 0; i < 2; i++) {
> > +               ret = mxs_read_ocotp(0x20 + i * 0x10, 1, &val);
> > +               if (ret)
> > +                       goto error;
> > +
> > +               mx28_fec_pdata[i].mac[0] = 0x00;
> > +               mx28_fec_pdata[i].mac[1] = 0x04;
> > +               mx28_fec_pdata[i].mac[2] = (val >> 24) & 0xff;
> > +               mx28_fec_pdata[i].mac[3] = (val >> 16) & 0xff;
> > +               mx28_fec_pdata[i].mac[4] = (val >> 8) & 0xff;
> > +               mx28_fec_pdata[i].mac[5] = (val >> 0) & 0xff;
> uuh.  Is ((val >> 24) & 0xff) supposed to be 0x9f?  If not this might
> have unwanted effects (practical, don't know about legal ones).
Yes, it is 0x9f.

-- 
Regards,
Shawn

[PATCH 03/10] net/fec: add mac field into platform data and consolidate fec_get_mac

[Uwe Kleine-König]

Hi Shawn,

On Wed, Dec 29, 2010 at 07:08:29PM +0800, Shawn Guo wrote:
> On Wed, Dec 29, 2010 at 11:37:58AM +0100, Uwe Kleine-K?nig wrote:
> > On Wed, Dec 29, 2010 at 06:30:15PM +0800, Shawn Guo wrote:
> > > > On Tue, Dec 28, 2010 at 10:55:48PM +0800, Shawn Guo wrote:
> > > [...]
> > > > > +	/*
> > > > > +	 * try to get mac address in following order:
> > > > > +	 *
> > > > > +	 * 1) kernel command line fec_mac=xx:xx:xx...
> > > > > +	 */
> > > > > +	iap = fec_mac_default;
> > > > > +
> > > > > +	/*
> > > > > +	 * 2) from flash or fuse (via platform data)
> > > > > +	 */
> > > > 
> > > > Again, how do you handle the dual MAC case?
> > > > 
> > > For the platform data case, the following patch reads both mac
> > > addresses.
> > > 
> > > [PATCH 09/10] ARM: mx28: read fec mac address from ocotp
> > > 
> > > +static int __init mx28evk_fec_get_mac(void)
> > > +{
> > > +       int i, ret;
> > > +       u32 val;
> > > +
> > > +       /*
> > > +        * OCOTP only stores the last 4 octets for each mac address,
> > > +        * so hard-coding the first two octets as Freescale OUI (00:04:9f)
> > > +        * is needed.
> > > +        */
> > > +       for (i = 0; i < 2; i++) {
> > > +               ret = mxs_read_ocotp(0x20 + i * 0x10, 1, &val);
> > > +               if (ret)
> > > +                       goto error;
> > > +
> > > +               mx28_fec_pdata[i].mac[0] = 0x00;
> > > +               mx28_fec_pdata[i].mac[1] = 0x04;
> > > +               mx28_fec_pdata[i].mac[2] = (val >> 24) & 0xff;
> > > +               mx28_fec_pdata[i].mac[3] = (val >> 16) & 0xff;
> > > +               mx28_fec_pdata[i].mac[4] = (val >> 8) & 0xff;
> > > +               mx28_fec_pdata[i].mac[5] = (val >> 0) & 0xff;
> > uuh.  Is ((val >> 24) & 0xff) supposed to be 0x9f?  If not this might
> > have unwanted effects (practical, don't know about legal ones).
> Yes, it is 0x9f.
Then maybe hardcode that, or at least warn if it's not?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 03/10] net/fec: add mac field into platform data and consolidate fec_get_mac

[Shawn Guo]

Hi Uwe,

On Wed, Dec 29, 2010 at 12:10:19PM +0100, Uwe Kleine-K?nig wrote:
> Hi Shawn,
> 
> On Wed, Dec 29, 2010 at 07:08:29PM +0800, Shawn Guo wrote:
> > On Wed, Dec 29, 2010 at 11:37:58AM +0100, Uwe Kleine-K?nig wrote:
> > > On Wed, Dec 29, 2010 at 06:30:15PM +0800, Shawn Guo wrote:
> > > > > On Tue, Dec 28, 2010 at 10:55:48PM +0800, Shawn Guo wrote:
> > > > [...]
> > > > > > +	/*
> > > > > > +	 * try to get mac address in following order:
> > > > > > +	 *
> > > > > > +	 * 1) kernel command line fec_mac=xx:xx:xx...
> > > > > > +	 */
> > > > > > +	iap = fec_mac_default;
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * 2) from flash or fuse (via platform data)
> > > > > > +	 */
> > > > > 
> > > > > Again, how do you handle the dual MAC case?
> > > > > 
> > > > For the platform data case, the following patch reads both mac
> > > > addresses.
> > > > 
> > > > [PATCH 09/10] ARM: mx28: read fec mac address from ocotp
> > > > 
> > > > +static int __init mx28evk_fec_get_mac(void)
> > > > +{
> > > > +       int i, ret;
> > > > +       u32 val;
> > > > +
> > > > +       /*
> > > > +        * OCOTP only stores the last 4 octets for each mac address,
> > > > +        * so hard-coding the first two octets as Freescale OUI (00:04:9f)
> > > > +        * is needed.
> > > > +        */
> > > > +       for (i = 0; i < 2; i++) {
> > > > +               ret = mxs_read_ocotp(0x20 + i * 0x10, 1, &val);
> > > > +               if (ret)
> > > > +                       goto error;
> > > > +
> > > > +               mx28_fec_pdata[i].mac[0] = 0x00;
> > > > +               mx28_fec_pdata[i].mac[1] = 0x04;
> > > > +               mx28_fec_pdata[i].mac[2] = (val >> 24) & 0xff;
> > > > +               mx28_fec_pdata[i].mac[3] = (val >> 16) & 0xff;
> > > > +               mx28_fec_pdata[i].mac[4] = (val >> 8) & 0xff;
> > > > +               mx28_fec_pdata[i].mac[5] = (val >> 0) & 0xff;
> > > uuh.  Is ((val >> 24) & 0xff) supposed to be 0x9f?  If not this might
> > > have unwanted effects (practical, don't know about legal ones).
> > Yes, it is 0x9f.
> Then maybe hardcode that, or at least warn if it's not?
> 
Will hard-code it.

-- 
Regards,
Shawn

[PATCH 04/10] net/fec: improve pm for better suspend/resume

[Shawn Guo]

The following commit made a fix to use fec_enet_open/fec_enet_close
over fec_enet_init/fec_stop for suspend/resume, because fec_enet_init
does not allow to have a working network interface at resume.

  e3fe8558c7fc182972c3d947d88744482111f304
  net/fec: fix pm to survive to suspend/resume

This fix works for i.mx/mxc fec controller, but fails on mx28 fec
which gets a different interrupt logic design. On i.mx fec, interrupt
can be triggered even bit ETHER_EN of ECR register is not set. But
on mx28 fec, ETHER_EN must be set to get interrupt work. Meanwhile,
MII interrupt is mandatory to resume the driver, because MDIO
read/write changed to interrupt mode by commit below.

  97b72e4320a9aaa4a7f1592ee7d2da7e2c9bd349
  fec: use interrupt for MDIO completion indication

fec_restart/fec_stop comes out as the solution working for both
cases.

Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
---
 drivers/net/fec.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/fec.c b/drivers/net/fec.c
index cd59814..f147508 100644
--- a/drivers/net/fec.c
+++ b/drivers/net/fec.c
@@ -1387,8 +1387,10 @@ fec_suspend(struct device *dev)
 
 	if (ndev) {
 		fep = netdev_priv(ndev);
-		if (netif_running(ndev))
-			fec_enet_close(ndev);
+		if (netif_running(ndev)) {
+			fec_stop(ndev);
+			netif_device_detach(ndev);
+		}
 		clk_disable(fep->clk);
 	}
 	return 0;
@@ -1403,8 +1405,10 @@ fec_resume(struct device *dev)
 	if (ndev) {
 		fep = netdev_priv(ndev);
 		clk_enable(fep->clk);
-		if (netif_running(ndev))
-			fec_enet_open(ndev);
+		if (netif_running(ndev)) {
+			fec_restart(ndev, fep->full_duplex);
+			netif_device_attach(ndev);
+		}
 	}
 	return 0;
 }
-- 
1.7.1

[PATCH 05/10] net/fec: add dual fec support for mx28

[Shawn Guo]

This patch is to add mx28 dual fec support. Here are some key notes
for mx28 fec controller.

 - mx28 fec design made an assumption that it runs on a
   big-endian system, which is incorrect. As the result, the
   driver has to swap every frame going to and coming from
   the controller.
 - external phys can only be configured by fec0, which means
   fec1 can not work independently and both phys need to be
   configured by mii_bus attached on fec0.
 - mx28 fec reset will get mac address registers reset too.
 - MII/RMII mode and 10M/100M speed are configured differently
   from i.mx/mxs fec controller.
 - ETHER_EN bit must be set to get interrupt work.

Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
---
 drivers/net/Kconfig |    7 ++-
 drivers/net/fec.c   |  105 +++++++++++++++++++++++++++++++++++++++++++++++++--
 drivers/net/fec.h   |    5 +-
 3 files changed, 108 insertions(+), 9 deletions(-)

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 4f1755b..f34629b 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -1944,18 +1944,19 @@ config 68360_ENET
 config FEC
 	bool "FEC ethernet controller (of ColdFire and some i.MX CPUs)"
 	depends on M523x || M527x || M5272 || M528x || M520x || M532x || \
-		MACH_MX27 || ARCH_MX35 || ARCH_MX25 || ARCH_MX5
+		MACH_MX27 || ARCH_MX35 || ARCH_MX25 || ARCH_MX5 || SOC_IMX28
 	select PHYLIB
 	help
 	  Say Y here if you want to use the built-in 10/100 Fast ethernet
 	  controller on some Motorola ColdFire and Freescale i.MX processors.
 
 config FEC2
-	bool "Second FEC ethernet controller (on some ColdFire CPUs)"
+	bool "Second FEC ethernet controller"
 	depends on FEC
 	help
 	  Say Y here if you want to use the second built-in 10/100 Fast
-	  ethernet controller on some Motorola ColdFire processors.
+	  ethernet controller on some Motorola ColdFire and Freescale
+	  i.MX processors.
 
 config FEC_MPC52xx
 	tristate "MPC52xx FEC driver"
diff --git a/drivers/net/fec.c b/drivers/net/fec.c
index f147508..301013e 100644
--- a/drivers/net/fec.c
+++ b/drivers/net/fec.c
@@ -17,6 +17,8 @@
  *
  * Bug fixes and cleanup by Philippe De Muyter (phdm at macqel.be)
  * Copyright (c) 2004-2006 Macq Electronique SA.
+ *
+ * Copyright (C) 2010 Freescale Semiconductor, Inc.
  */
 
 #include <linux/module.h>
@@ -45,21 +47,31 @@
 
 #include <asm/cacheflush.h>
 
-#ifndef CONFIG_ARCH_MXC
+#if !defined(CONFIG_ARCH_MXC) && !defined(CONFIG_SOC_IMX28)
 #include <asm/coldfire.h>
 #include <asm/mcfsim.h>
 #endif
 
 #include "fec.h"
 
-#ifdef CONFIG_ARCH_MXC
-#include <mach/hardware.h>
+#ifdef CONFIG_SOC_IMX28
+/*
+ * mx28 does not have MIIGSK registers
+ */
+#undef FEC_MIIGSK_ENR
+#include <mach/mxs.h>
+#else
+#define cpu_is_mx28()	(0)
+#endif
+
+#if defined(CONFIG_ARCH_MXC) || defined(CONFIG_SOC_IMX28)
 #define FEC_ALIGNMENT	0xf
 #else
 #define FEC_ALIGNMENT	0x3
 #endif
 
 static unsigned char fec_mac_default[ETH_ALEN];
+static struct mii_bus *fec_mii_bus;
 
 #if defined(CONFIG_M5272)
 /*
@@ -127,7 +139,8 @@ static unsigned char fec_mac_default[ETH_ALEN];
  * account when setting it.
  */
 #if defined(CONFIG_M523x) || defined(CONFIG_M527x) || defined(CONFIG_M528x) || \
-    defined(CONFIG_M520x) || defined(CONFIG_M532x) || defined(CONFIG_ARCH_MXC)
+    defined(CONFIG_M520x) || defined(CONFIG_M532x) || \
+    defined(CONFIG_ARCH_MXC) || defined(CONFIG_SOC_IMX28)
 #define	OPT_FRAME_SIZE	(PKT_MAXBUF_SIZE << 16)
 #else
 #define	OPT_FRAME_SIZE	0
@@ -206,6 +219,17 @@ static void fec_stop(struct net_device *dev);
 /* Transmitter timeout */
 #define TX_TIMEOUT (2 * HZ)
 
+static void *swap_buffer(void *bufaddr, int len)
+{
+	int i;
+	unsigned int *buf = bufaddr;
+
+	for (i = 0; i < (len + 3) / 4; i++, buf++)
+		*buf = __swab32(*buf);
+
+	return bufaddr;
+}
+
 static netdev_tx_t
 fec_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
@@ -254,6 +278,14 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		bufaddr = fep->tx_bounce[index];
 	}
 
+	/*
+	 * mx28 fec design made an incorrect assumption that it's running
+	 * on a big endian system. As the result, the driver has to swap
+	 * every frame going to and coming from the controller.
+	 */
+	if (cpu_is_mx28())
+		swap_buffer(bufaddr, skb->len);
+
 	/* Save skb pointer */
 	fep->tx_skbuff[fep->skb_cur] = skb;
 
@@ -485,6 +517,9 @@ fec_enet_rx(struct net_device *dev)
 	        dma_unmap_single(NULL, bdp->cbd_bufaddr, bdp->cbd_datlen,
         			DMA_FROM_DEVICE);
 
+		if (cpu_is_mx28())
+			swap_buffer(data, pkt_len);
+
 		/* This does 16 byte alignment, exactly what we need.
 		 * The packet length includes FCS, but we don't want to
 		 * include that when passing upstream as it messes up
@@ -686,6 +721,7 @@ static int fec_enet_mii_probe(struct net_device *dev)
 	char mdio_bus_id[MII_BUS_ID_SIZE];
 	char phy_name[MII_BUS_ID_SIZE + 3];
 	int phy_id;
+	int dev_id = fep->pdev->id;
 
 	fep->phy_dev = NULL;
 
@@ -697,6 +733,8 @@ static int fec_enet_mii_probe(struct net_device *dev)
 			continue;
 		if (fep->mii_bus->phy_map[phy_id]->phy_id == 0)
 			continue;
+		if (cpu_is_mx28() && dev_id--)
+			continue;
 		strncpy(mdio_bus_id, fep->mii_bus->id, MII_BUS_ID_SIZE);
 		break;
 	}
@@ -738,6 +776,28 @@ static int fec_enet_mii_init(struct platform_device *pdev)
 	struct fec_enet_private *fep = netdev_priv(dev);
 	int err = -ENXIO, i;
 
+	/*
+	 * The dual fec interfaces are not equivalent on mx28. Here are the
+	 * differences:
+	 *
+	 *  - fec0 supports MII & RMII modes while fec1 only supports RMII
+	 *  - fec0 acts as the 1588 time master while fec1 is slave
+	 *  - external phys can only be configured by fec0
+	 *
+	 * That is to say fec1 can not work independently. It only works
+	 * when fec0 is working. The reason behind this design is that the
+	 * second interface is added primarily for Switch mode.
+	 *
+	 * Because of the last point above, both phys are attached on fec0
+	 * mdio interface in board design, and need to be configured by
+	 * fec0 mii_bus.
+	 */
+	if (cpu_is_mx28() && pdev->id) {
+		/* fec1 uses fec0 mii_bus */
+		fep->mii_bus = fec_mii_bus;
+		return 0;
+	}
+
 	fep->mii_timeout = 0;
 
 	/*
@@ -774,6 +834,10 @@ static int fec_enet_mii_init(struct platform_device *pdev)
 	if (mdiobus_register(fep->mii_bus))
 		goto err_out_free_mdio_irq;
 
+	/* save fec0 mii_bus */
+	if (cpu_is_mx28())
+		fec_mii_bus = fep->mii_bus;
+
 	return 0;
 
 err_out_free_mdio_irq:
@@ -1164,11 +1228,22 @@ fec_restart(struct net_device *dev, int duplex)
 {
 	struct fec_enet_private *fep = netdev_priv(dev);
 	int i;
+	u32 val, temp_mac[2];
 
 	/* Whack a reset.  We should wait for this. */
 	writel(1, fep->hwp + FEC_ECNTRL);
 	udelay(10);
 
+	/*
+	 * The fec reset on mx28 will reset mac address too,
+	 * so need to reconfigure it.
+	 */
+	if (cpu_is_mx28()) {
+		memcpy(&temp_mac, dev->dev_addr, ETH_ALEN);
+		writel(cpu_to_be32(temp_mac[0]), fep->hwp + FEC_ADDR_LOW);
+		writel(cpu_to_be32(temp_mac[1]), fep->hwp + FEC_ADDR_HIGH);
+	}
+
 	/* Clear any outstanding interrupt. */
 	writel(0xffc00000, fep->hwp + FEC_IEVENT);
 
@@ -1215,6 +1290,28 @@ fec_restart(struct net_device *dev, int duplex)
 	/* Set MII speed */
 	writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
 
+	/*
+	 * The phy interface and speed need to get configured
+	 * differently on mx28.
+	 */
+	if (cpu_is_mx28()) {
+		val = readl(fep->hwp + FEC_R_CNTRL);
+
+		/* MII or RMII */
+		if (fep->phy_interface == PHY_INTERFACE_MODE_RMII)
+			val |= (1 << 8);
+		else
+			val &= ~(1 << 8);
+
+		/* 10M or 100M */
+		if (fep->phy_dev && fep->phy_dev->speed == SPEED_100)
+			val &= ~(1 << 9);
+		else
+			val |= (1 << 9);
+
+		writel(val, fep->hwp + FEC_R_CNTRL);
+	}
+
 #ifdef FEC_MIIGSK_ENR
 	if (fep->phy_interface == PHY_INTERFACE_MODE_RMII) {
 		/* disable the gasket and wait */
diff --git a/drivers/net/fec.h b/drivers/net/fec.h
index 2c48b25..ace318d 100644
--- a/drivers/net/fec.h
+++ b/drivers/net/fec.h
@@ -14,7 +14,8 @@
 /****************************************************************************/
 
 #if defined(CONFIG_M523x) || defined(CONFIG_M527x) || defined(CONFIG_M528x) || \
-    defined(CONFIG_M520x) || defined(CONFIG_M532x) || defined(CONFIG_ARCH_MXC)
+    defined(CONFIG_M520x) || defined(CONFIG_M532x) || \
+    defined(CONFIG_ARCH_MXC) || defined(CONFIG_SOC_IMX28)
 /*
  *	Just figures, Motorola would have to change the offsets for
  *	registers in the same peripheral device on different models
@@ -78,7 +79,7 @@
 /*
  *	Define the buffer descriptor structure.
  */
-#ifdef CONFIG_ARCH_MXC
+#if defined(CONFIG_ARCH_MXC) || defined(CONFIG_SOC_IMX28)
 struct bufdesc {
 	unsigned short cbd_datlen;	/* Data length */
 	unsigned short cbd_sc;	/* Control and status info */
-- 
1.7.1

[PATCH 06/10] ARM: mx28: update clocks for dual fec support

[Shawn Guo]

* Change fec clock registration to use con_id only for looking up,
  so that dual fec driver can find the same clock for both fec0
  and fec1.

* Explicitly call clk_enable for some clocks that have been on to
  reflect the clock status and get usecount updated. Otherwise,
  clocks could be turned off by pair call of clk_enable/clk_disable
  in drivers, because of the incorrect initial usecount.

Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
---
 arch/arm/mach-mxs/clock-mx28.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-mxs/clock-mx28.c b/arch/arm/mach-mxs/clock-mx28.c
index dd6d158..6b77aff 100644
--- a/arch/arm/mach-mxs/clock-mx28.c
+++ b/arch/arm/mach-mxs/clock-mx28.c
@@ -603,7 +603,7 @@ _DEFINE_CLOCK(fec_clk, ENET, DISABLE, &hbus_clk);
 
 static struct clk_lookup lookups[] = {
 	_REGISTER_CLOCK("uart", NULL, uart_clk)
-	_REGISTER_CLOCK("fec.0", NULL, fec_clk)
+	_REGISTER_CLOCK(NULL, "fec_clk", fec_clk)
 	_REGISTER_CLOCK("rtc", NULL, rtc_clk)
 	_REGISTER_CLOCK("pll2", NULL, pll2_clk)
 	_REGISTER_CLOCK(NULL, "hclk", hbus_clk)
@@ -726,6 +726,11 @@ int __init mx28_clocks_init(void)
 {
 	clk_misc_init();
 
+	clk_enable(&cpu_clk);
+	clk_enable(&hbus_clk);
+	clk_enable(&emi_clk);
+	clk_enable(&uart_clk);
+
 	clkdev_add_table(lookups, ARRAY_SIZE(lookups));
 
 	mxs_timer_init(&clk32k_clk, MX28_INT_TIMER0);
-- 
1.7.1

[PATCH 06/10] ARM: mx28: update clocks for dual fec support

[Baruch Siach]

Hi Shawn,

On Tue, Dec 28, 2010 at 10:55:51PM +0800, Shawn Guo wrote:
> * Change fec clock registration to use con_id only for looking up,
>   so that dual fec driver can find the same clock for both fec0
>   and fec1.
> 
> * Explicitly call clk_enable for some clocks that have been on to
>   reflect the clock status and get usecount updated. Otherwise,
>   clocks could be turned off by pair call of clk_enable/clk_disable
>   in drivers, because of the incorrect initial usecount.
> 
> Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> ---
>  arch/arm/mach-mxs/clock-mx28.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-mxs/clock-mx28.c b/arch/arm/mach-mxs/clock-mx28.c
> index dd6d158..6b77aff 100644
> --- a/arch/arm/mach-mxs/clock-mx28.c
> +++ b/arch/arm/mach-mxs/clock-mx28.c
> @@ -603,7 +603,7 @@ _DEFINE_CLOCK(fec_clk, ENET, DISABLE, &hbus_clk);
>  
>  static struct clk_lookup lookups[] = {
>  	_REGISTER_CLOCK("uart", NULL, uart_clk)
> -	_REGISTER_CLOCK("fec.0", NULL, fec_clk)
> +	_REGISTER_CLOCK(NULL, "fec_clk", fec_clk)

Looks like a wrong use of the clk API. You should define define two clocks 
"fec.0" and "fec.1".

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH 06/10] ARM: mx28: update clocks for dual fec support

[Uwe Kleine-König]

On Wed, Dec 29, 2010 at 08:57:34AM +0200, Baruch Siach wrote:
> Hi Shawn,
> 
> On Tue, Dec 28, 2010 at 10:55:51PM +0800, Shawn Guo wrote:
> > * Change fec clock registration to use con_id only for looking up,
> >   so that dual fec driver can find the same clock for both fec0
> >   and fec1.
> > 
> > * Explicitly call clk_enable for some clocks that have been on to
> >   reflect the clock status and get usecount updated. Otherwise,
> >   clocks could be turned off by pair call of clk_enable/clk_disable
> >   in drivers, because of the incorrect initial usecount.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> > ---
> >  arch/arm/mach-mxs/clock-mx28.c |    7 ++++++-
> >  1 files changed, 6 insertions(+), 1 deletions(-)
> > 
> > diff --git a/arch/arm/mach-mxs/clock-mx28.c b/arch/arm/mach-mxs/clock-mx28.c
> > index dd6d158..6b77aff 100644
> > --- a/arch/arm/mach-mxs/clock-mx28.c
> > +++ b/arch/arm/mach-mxs/clock-mx28.c
> > @@ -603,7 +603,7 @@ _DEFINE_CLOCK(fec_clk, ENET, DISABLE, &hbus_clk);
> >  
> >  static struct clk_lookup lookups[] = {
> >  	_REGISTER_CLOCK("uart", NULL, uart_clk)
> > -	_REGISTER_CLOCK("fec.0", NULL, fec_clk)
> > +	_REGISTER_CLOCK(NULL, "fec_clk", fec_clk)
> 
> Looks like a wrong use of the clk API. You should define define two clocks 
> "fec.0" and "fec.1".
The commit log suggests that both should enable the same (hardware)
clock.  Still I agree with Baruch that in this case clk_get() should
just return the same clock for both fec.0 and fec.1.
I think it's a bit more work than just adding
	_REGISTER_CLOCK("fec.1", NULL, fec_clk)

, maybe just define fec1_clk as a dummy clk with parent=fec_clk?

(And IMHO don't inlude "_clk" in clknames, that's implicit.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 06/10] ARM: mx28: update clocks for dual fec support

[Shawn Guo]

Hi Uwe,

On Wed, Dec 29, 2010 at 09:10:44AM +0100, Uwe Kleine-K?nig wrote:
> On Wed, Dec 29, 2010 at 08:57:34AM +0200, Baruch Siach wrote:
> > Hi Shawn,
> > 
> > On Tue, Dec 28, 2010 at 10:55:51PM +0800, Shawn Guo wrote:
> > > * Change fec clock registration to use con_id only for looking up,
> > >   so that dual fec driver can find the same clock for both fec0
> > >   and fec1.
> > > 
> > > * Explicitly call clk_enable for some clocks that have been on to
> > >   reflect the clock status and get usecount updated. Otherwise,
> > >   clocks could be turned off by pair call of clk_enable/clk_disable
> > >   in drivers, because of the incorrect initial usecount.
> > > 
> > > Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> > > ---
> > >  arch/arm/mach-mxs/clock-mx28.c |    7 ++++++-
> > >  1 files changed, 6 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/arch/arm/mach-mxs/clock-mx28.c b/arch/arm/mach-mxs/clock-mx28.c
> > > index dd6d158..6b77aff 100644
> > > --- a/arch/arm/mach-mxs/clock-mx28.c
> > > +++ b/arch/arm/mach-mxs/clock-mx28.c
> > > @@ -603,7 +603,7 @@ _DEFINE_CLOCK(fec_clk, ENET, DISABLE, &hbus_clk);
> > >  
> > >  static struct clk_lookup lookups[] = {
> > >  	_REGISTER_CLOCK("uart", NULL, uart_clk)
> > > -	_REGISTER_CLOCK("fec.0", NULL, fec_clk)
> > > +	_REGISTER_CLOCK(NULL, "fec_clk", fec_clk)
> > 
> > Looks like a wrong use of the clk API. You should define define two clocks 
> > "fec.0" and "fec.1".
> The commit log suggests that both should enable the same (hardware)
> clock.  Still I agree with Baruch that in this case clk_get() should
> just return the same clock for both fec.0 and fec.1.
> I think it's a bit more work than just adding
> 	_REGISTER_CLOCK("fec.1", NULL, fec_clk)
> 
I originally had what you suggest above. But when I found fec driver
only calls clk_get like below, I change the code to current one to 
save one line code.

	fep->clk = clk_get(&pdev->dev, "fec_clk");

But it looks people use dev_id to address clock as the preference.
Will change it back.

> , maybe just define fec1_clk as a dummy clk with parent=fec_clk?
> 
> (And IMHO don't inlude "_clk" in clknames, that's implicit.)
> 

-- 
Regards,
Shawn

[PATCH 06/10] ARM: mx28: update clocks for dual fec support

[Uwe Kleine-König]

Hello,

On Wed, Dec 29, 2010 at 06:14:42PM +0800, Shawn Guo wrote:
> On Wed, Dec 29, 2010 at 09:10:44AM +0100, Uwe Kleine-K?nig wrote:
> > On Wed, Dec 29, 2010 at 08:57:34AM +0200, Baruch Siach wrote:
> > > Hi Shawn,
> > > 
> > > On Tue, Dec 28, 2010 at 10:55:51PM +0800, Shawn Guo wrote:
> > > > * Change fec clock registration to use con_id only for looking up,
> > > >   so that dual fec driver can find the same clock for both fec0
> > > >   and fec1.
> > > > 
> > > > * Explicitly call clk_enable for some clocks that have been on to
> > > >   reflect the clock status and get usecount updated. Otherwise,
> > > >   clocks could be turned off by pair call of clk_enable/clk_disable
> > > >   in drivers, because of the incorrect initial usecount.
> > > > 
> > > > Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> > > > ---
> > > >  arch/arm/mach-mxs/clock-mx28.c |    7 ++++++-
> > > >  1 files changed, 6 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/arch/arm/mach-mxs/clock-mx28.c b/arch/arm/mach-mxs/clock-mx28.c
> > > > index dd6d158..6b77aff 100644
> > > > --- a/arch/arm/mach-mxs/clock-mx28.c
> > > > +++ b/arch/arm/mach-mxs/clock-mx28.c
> > > > @@ -603,7 +603,7 @@ _DEFINE_CLOCK(fec_clk, ENET, DISABLE, &hbus_clk);
> > > >  
> > > >  static struct clk_lookup lookups[] = {
> > > >  	_REGISTER_CLOCK("uart", NULL, uart_clk)
> > > > -	_REGISTER_CLOCK("fec.0", NULL, fec_clk)
> > > > +	_REGISTER_CLOCK(NULL, "fec_clk", fec_clk)
> > > 
> > > Looks like a wrong use of the clk API. You should define define two clocks 
> > > "fec.0" and "fec.1".
> > The commit log suggests that both should enable the same (hardware)
> > clock.  Still I agree with Baruch that in this case clk_get() should
> > just return the same clock for both fec.0 and fec.1.
> > I think it's a bit more work than just adding
> > 	_REGISTER_CLOCK("fec.1", NULL, fec_clk)
> > 
> I originally had what you suggest above. But when I found fec driver
> only calls clk_get like below, I change the code to current one to 
> save one line code.
> 
> 	fep->clk = clk_get(&pdev->dev, "fec_clk");
if there is only a single (software) clock for the fec, the driver
should pass NULL as 2nd argument to clk_get.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 07/10] ARM: mx28: add the second fec device registration

[Shawn Guo]

Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
---
 arch/arm/mach-mxs/mach-mx28evk.c |   28 +++++++++++++++++++++++++---
 1 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-mxs/mach-mx28evk.c b/arch/arm/mach-mxs/mach-mx28evk.c
index d162e95..def6519 100644
--- a/arch/arm/mach-mxs/mach-mx28evk.c
+++ b/arch/arm/mach-mxs/mach-mx28evk.c
@@ -57,6 +57,19 @@ static const iomux_cfg_t mx28evk_pads[] __initconst = {
 		(MXS_PAD_8MA | MXS_PAD_3V3 | MXS_PAD_PULLUP),
 	MX28_PAD_ENET_CLK__CLKCTRL_ENET |
 		(MXS_PAD_8MA | MXS_PAD_3V3 | MXS_PAD_PULLUP),
+	/* fec1 */
+	MX28_PAD_ENET0_CRS__ENET1_RX_EN |
+		(MXS_PAD_8MA | MXS_PAD_3V3 | MXS_PAD_PULLUP),
+	MX28_PAD_ENET0_RXD2__ENET1_RXD0 |
+		(MXS_PAD_8MA | MXS_PAD_3V3 | MXS_PAD_PULLUP),
+	MX28_PAD_ENET0_RXD3__ENET1_RXD1 |
+		(MXS_PAD_8MA | MXS_PAD_3V3 | MXS_PAD_PULLUP),
+	MX28_PAD_ENET0_COL__ENET1_TX_EN |
+		(MXS_PAD_8MA | MXS_PAD_3V3 | MXS_PAD_PULLUP),
+	MX28_PAD_ENET0_TXD2__ENET1_TXD0 |
+		(MXS_PAD_8MA | MXS_PAD_3V3 | MXS_PAD_PULLUP),
+	MX28_PAD_ENET0_TXD3__ENET1_TXD1 |
+		(MXS_PAD_8MA | MXS_PAD_3V3 | MXS_PAD_PULLUP),
 	/* phy power line */
 	MX28_PAD_SSP1_DATA3__GPIO_2_15 |
 		(MXS_PAD_4MA | MXS_PAD_3V3 | MXS_PAD_NOPULL),
@@ -106,8 +119,14 @@ static void __init mx28evk_fec_reset(void)
 	gpio_set_value(MX28EVK_FEC_PHY_RESET, 1);
 }
 
-static const struct fec_platform_data mx28_fec_pdata __initconst = {
-	.phy = PHY_INTERFACE_MODE_RMII,
+static struct fec_platform_data mx28_fec_pdata[] = {
+	{
+		/* fec0 */
+		.phy = PHY_INTERFACE_MODE_RMII,
+	}, {
+		/* fec1 */
+		.phy = PHY_INTERFACE_MODE_RMII,
+	},
 };
 
 static void __init mx28evk_init(void)
@@ -117,7 +136,10 @@ static void __init mx28evk_init(void)
 	mx28_add_duart();
 
 	mx28evk_fec_reset();
-	mx28_add_fec(0, &mx28_fec_pdata);
+	mx28_add_fec(0, &mx28_fec_pdata[0]);
+#ifdef CONFIG_FEC2
+	mx28_add_fec(1, &mx28_fec_pdata[1]);
+#endif
 }
 
 static void __init mx28evk_timer_init(void)
-- 
1.7.1

[PATCH 07/10] ARM: mx28: add the second fec device registration

[Uwe Kleine-König]

Hello Shawn,

> -static const struct fec_platform_data mx28_fec_pdata __initconst = {
> -	.phy = PHY_INTERFACE_MODE_RMII,
> +static struct fec_platform_data mx28_fec_pdata[] = {
> +	{
> +		/* fec0 */
> +		.phy = PHY_INTERFACE_MODE_RMII,
> +	}, {
> +		/* fec1 */
> +		.phy = PHY_INTERFACE_MODE_RMII,
> +	},
>  };
I think there is no need to duplicate the platform_data as mx28_add_fec
makes a copy anyhow.  Did you remove __initconst and const on purpose?

>  static void __init mx28evk_init(void)
> @@ -117,7 +136,10 @@ static void __init mx28evk_init(void)
>  	mx28_add_duart();
>  
>  	mx28evk_fec_reset();
> -	mx28_add_fec(0, &mx28_fec_pdata);
> +	mx28_add_fec(0, &mx28_fec_pdata[0]);
> +#ifdef CONFIG_FEC2
> +	mx28_add_fec(1, &mx28_fec_pdata[1]);
> +#endif
>  }
>  
>  static void __init mx28evk_timer_init(void)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 07/10] ARM: mx28: add the second fec device registration

[Shawn Guo]

Hi Uwe,

On Wed, Dec 29, 2010 at 11:50:01AM +0100, Uwe Kleine-K?nig wrote:
> Hello Shawn,
> 
> > -static const struct fec_platform_data mx28_fec_pdata __initconst = {
> > -	.phy = PHY_INTERFACE_MODE_RMII,
> > +static struct fec_platform_data mx28_fec_pdata[] = {
> > +	{
> > +		/* fec0 */
> > +		.phy = PHY_INTERFACE_MODE_RMII,
> > +	}, {
> > +		/* fec1 */
> > +		.phy = PHY_INTERFACE_MODE_RMII,
> > +	},
> >  };
> I think there is no need to duplicate the platform_data as mx28_add_fec
> makes a copy anyhow.  Did you remove __initconst and const on purpose?
> 
The mac field is different for fec0 and fec1 and needs to be
assigned in mx28evk_fec_get_mac.

-- 
Regards,
Shawn

[PATCH 08/10] ARM: mxs: add ocotp read function

[Shawn Guo]

Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
---
 arch/arm/mach-mxs/Makefile              |    2 +-
 arch/arm/mach-mxs/include/mach/common.h |    1 +
 arch/arm/mach-mxs/ocotp.c               |   52 +++++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/mach-mxs/ocotp.c

diff --git a/arch/arm/mach-mxs/Makefile b/arch/arm/mach-mxs/Makefile
index 39d3f9c..f23ebbd 100644
--- a/arch/arm/mach-mxs/Makefile
+++ b/arch/arm/mach-mxs/Makefile
@@ -1,5 +1,5 @@
 # Common support
-obj-y := clock.o devices.o gpio.o icoll.o iomux.o system.o timer.o
+obj-y := clock.o devices.o gpio.o icoll.o iomux.o ocotp.o system.o timer.o
 
 obj-$(CONFIG_SOC_IMX23) += clock-mx23.o mm-mx23.o
 obj-$(CONFIG_SOC_IMX28) += clock-mx28.o mm-mx28.o
diff --git a/arch/arm/mach-mxs/include/mach/common.h b/arch/arm/mach-mxs/include/mach/common.h
index 59133eb..cf02552 100644
--- a/arch/arm/mach-mxs/include/mach/common.h
+++ b/arch/arm/mach-mxs/include/mach/common.h
@@ -13,6 +13,7 @@
 
 struct clk;
 
+extern int mxs_read_ocotp(int offset, int count, u32 *values);
 extern int mxs_reset_block(void __iomem *);
 extern void mxs_timer_init(struct clk *, int);
 
diff --git a/arch/arm/mach-mxs/ocotp.c b/arch/arm/mach-mxs/ocotp.c
new file mode 100644
index 0000000..24457d7
--- /dev/null
+++ b/arch/arm/mach-mxs/ocotp.c
@@ -0,0 +1,52 @@
+/*
+ * Copyright 2010 Freescale Semiconductor, Inc. All Rights Reserved.
+ *
+ * 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.
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+
+#include <mach/mxs.h>
+
+#define BM_OCOTP_CTRL_RD_BANK_OPEN	(1 << 12)
+#define BM_OCOTP_CTRL_BUSY		(1 << 8)
+
+int mxs_read_ocotp(int offset, int count, u32 *values)
+{
+	void __iomem *ocotp_base = MXS_IO_ADDRESS(MXS_OCOTP_BASE_ADDR);
+	int i, timeout = 0x400;
+
+	/* open OCOTP banks for read */
+	__mxs_setl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);
+
+	/* approximately wait 32 hclk cycles */
+	udelay(1);
+
+	/* poll BUSY bit becoming cleared */
+	while ((__raw_readl(ocotp_base) & BM_OCOTP_CTRL_BUSY) && --timeout)
+		/* nothing */;
+
+	if (unlikely(!timeout))
+		goto error;
+
+	for (i = 0; i < count; i++, offset += 4)
+		*values++ = __raw_readl(ocotp_base + offset);
+
+	/* close banks for power saving */
+	__mxs_clrl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);
+
+	return 0;
+
+error:
+	pr_err("%s: timeout in reading OCOTP\n", __func__);
+	return -ETIMEDOUT;
+}
-- 
1.7.1

[PATCH 08/10] ARM: mxs: add ocotp read function

[Uwe Kleine-König]

Hello Shawn,

On Tue, Dec 28, 2010 at 10:55:53PM +0800, Shawn Guo wrote:
> Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> ---
>  arch/arm/mach-mxs/Makefile              |    2 +-
>  arch/arm/mach-mxs/include/mach/common.h |    1 +
>  arch/arm/mach-mxs/ocotp.c               |   52 +++++++++++++++++++++++++++++++
>  3 files changed, 54 insertions(+), 1 deletions(-)
>  create mode 100644 arch/arm/mach-mxs/ocotp.c
> 
> diff --git a/arch/arm/mach-mxs/Makefile b/arch/arm/mach-mxs/Makefile
> index 39d3f9c..f23ebbd 100644
> --- a/arch/arm/mach-mxs/Makefile
> +++ b/arch/arm/mach-mxs/Makefile
> @@ -1,5 +1,5 @@
>  # Common support
> -obj-y := clock.o devices.o gpio.o icoll.o iomux.o system.o timer.o
> +obj-y := clock.o devices.o gpio.o icoll.o iomux.o ocotp.o system.o timer.o
>  
>  obj-$(CONFIG_SOC_IMX23) += clock-mx23.o mm-mx23.o
>  obj-$(CONFIG_SOC_IMX28) += clock-mx28.o mm-mx28.o
> diff --git a/arch/arm/mach-mxs/include/mach/common.h b/arch/arm/mach-mxs/include/mach/common.h
> index 59133eb..cf02552 100644
> --- a/arch/arm/mach-mxs/include/mach/common.h
> +++ b/arch/arm/mach-mxs/include/mach/common.h
> @@ -13,6 +13,7 @@
>  
>  struct clk;
>  
> +extern int mxs_read_ocotp(int offset, int count, u32 *values);
>  extern int mxs_reset_block(void __iomem *);
>  extern void mxs_timer_init(struct clk *, int);
>  
> diff --git a/arch/arm/mach-mxs/ocotp.c b/arch/arm/mach-mxs/ocotp.c
> new file mode 100644
> index 0000000..24457d7
> --- /dev/null
> +++ b/arch/arm/mach-mxs/ocotp.c
> @@ -0,0 +1,52 @@
> +/*
> + * Copyright 2010 Freescale Semiconductor, Inc. All Rights Reserved.
> + *
> + * 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.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +
> +#include <mach/mxs.h>
> +
> +#define BM_OCOTP_CTRL_RD_BANK_OPEN	(1 << 12)
> +#define BM_OCOTP_CTRL_BUSY		(1 << 8)
> +
> +int mxs_read_ocotp(int offset, int count, u32 *values)
Maybe use size_t as type for count?  unsigned for offset?

> +{
> +	void __iomem *ocotp_base = MXS_IO_ADDRESS(MXS_OCOTP_BASE_ADDR);
> +	int i, timeout = 0x400;
> +
> +	/* open OCOTP banks for read */
> +	__mxs_setl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);
> +
> +	/* approximately wait 32 hclk cycles */
> +	udelay(1);
> +
> +	/* poll BUSY bit becoming cleared */
> +	while ((__raw_readl(ocotp_base) & BM_OCOTP_CTRL_BUSY) && --timeout)
> +		/* nothing */;
> +
> +	if (unlikely(!timeout))
> +		goto error;
> +
> +	for (i = 0; i < count; i++, offset += 4)
> +		*values++ = __raw_readl(ocotp_base + offset);
> +
> +	/* close banks for power saving */
> +	__mxs_clrl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);
> +
> +	return 0;
> +
> +error:
> +	pr_err("%s: timeout in reading OCOTP\n", __func__);
> +	return -ETIMEDOUT;
> +}
does this need locking?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 08/10] ARM: mxs: add ocotp read function

[Shawn Guo]

On Wed, Dec 29, 2010 at 11:47:45AM +0100, Uwe Kleine-K?nig wrote:
> Hello Shawn,
> 
> On Tue, Dec 28, 2010 at 10:55:53PM +0800, Shawn Guo wrote:
> > Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> > ---
> >  arch/arm/mach-mxs/Makefile              |    2 +-
> >  arch/arm/mach-mxs/include/mach/common.h |    1 +
> >  arch/arm/mach-mxs/ocotp.c               |   52 +++++++++++++++++++++++++++++++
> >  3 files changed, 54 insertions(+), 1 deletions(-)
> >  create mode 100644 arch/arm/mach-mxs/ocotp.c
> > 
> > diff --git a/arch/arm/mach-mxs/Makefile b/arch/arm/mach-mxs/Makefile
> > index 39d3f9c..f23ebbd 100644
> > --- a/arch/arm/mach-mxs/Makefile
> > +++ b/arch/arm/mach-mxs/Makefile
> > @@ -1,5 +1,5 @@
> >  # Common support
> > -obj-y := clock.o devices.o gpio.o icoll.o iomux.o system.o timer.o
> > +obj-y := clock.o devices.o gpio.o icoll.o iomux.o ocotp.o system.o timer.o
> >  
> >  obj-$(CONFIG_SOC_IMX23) += clock-mx23.o mm-mx23.o
> >  obj-$(CONFIG_SOC_IMX28) += clock-mx28.o mm-mx28.o
> > diff --git a/arch/arm/mach-mxs/include/mach/common.h b/arch/arm/mach-mxs/include/mach/common.h
> > index 59133eb..cf02552 100644
> > --- a/arch/arm/mach-mxs/include/mach/common.h
> > +++ b/arch/arm/mach-mxs/include/mach/common.h
> > @@ -13,6 +13,7 @@
> >  
> >  struct clk;
> >  
> > +extern int mxs_read_ocotp(int offset, int count, u32 *values);
> >  extern int mxs_reset_block(void __iomem *);
> >  extern void mxs_timer_init(struct clk *, int);
> >  
> > diff --git a/arch/arm/mach-mxs/ocotp.c b/arch/arm/mach-mxs/ocotp.c
> > new file mode 100644
> > index 0000000..24457d7
> > --- /dev/null
> > +++ b/arch/arm/mach-mxs/ocotp.c
> > @@ -0,0 +1,52 @@
> > +/*
> > + * Copyright 2010 Freescale Semiconductor, Inc. All Rights Reserved.
> > + *
> > + * 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.
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +
> > +#include <mach/mxs.h>
> > +
> > +#define BM_OCOTP_CTRL_RD_BANK_OPEN	(1 << 12)
> > +#define BM_OCOTP_CTRL_BUSY		(1 << 8)
> > +
> > +int mxs_read_ocotp(int offset, int count, u32 *values)
> Maybe use size_t as type for count?  unsigned for offset?
> 
OK.

> > +{
> > +	void __iomem *ocotp_base = MXS_IO_ADDRESS(MXS_OCOTP_BASE_ADDR);
> > +	int i, timeout = 0x400;
> > +
> > +	/* open OCOTP banks for read */
> > +	__mxs_setl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);
> > +
> > +	/* approximately wait 32 hclk cycles */
> > +	udelay(1);
> > +
> > +	/* poll BUSY bit becoming cleared */
> > +	while ((__raw_readl(ocotp_base) & BM_OCOTP_CTRL_BUSY) && --timeout)
> > +		/* nothing */;
> > +
> > +	if (unlikely(!timeout))
> > +		goto error;
> > +
> > +	for (i = 0; i < count; i++, offset += 4)
> > +		*values++ = __raw_readl(ocotp_base + offset);
> > +
> > +	/* close banks for power saving */
> > +	__mxs_clrl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);
> > +
> > +	return 0;
> > +
> > +error:
> > +	pr_err("%s: timeout in reading OCOTP\n", __func__);
> > +	return -ETIMEDOUT;
> > +}
> does this need locking?
> 
You are suggesting mutex or spinlock?

-- 
Regards,
Shawn

[PATCH 08/10] ARM: mxs: add ocotp read function

[Uwe Kleine-König]

Hello Shawn,

On Wed, Dec 29, 2010 at 08:08:23PM +0800, Shawn Guo wrote:
> On Wed, Dec 29, 2010 at 11:47:45AM +0100, Uwe Kleine-K?nig wrote:
> > On Tue, Dec 28, 2010 at 10:55:53PM +0800, Shawn Guo wrote:
> > > +{
> > > +	void __iomem *ocotp_base = MXS_IO_ADDRESS(MXS_OCOTP_BASE_ADDR);
> > > +	int i, timeout = 0x400;
> > > +
> > > +	/* open OCOTP banks for read */
> > > +	__mxs_setl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);
> > > +
> > > +	/* approximately wait 32 hclk cycles */
> > > +	udelay(1);
> > > +
> > > +	/* poll BUSY bit becoming cleared */
> > > +	while ((__raw_readl(ocotp_base) & BM_OCOTP_CTRL_BUSY) && --timeout)
> > > +		/* nothing */;
> > > +
> > > +	if (unlikely(!timeout))
> > > +		goto error;
> > > +
> > > +	for (i = 0; i < count; i++, offset += 4)
> > > +		*values++ = __raw_readl(ocotp_base + offset);
> > > +
> > > +	/* close banks for power saving */
> > > +	__mxs_clrl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);
> > > +
> > > +	return 0;
> > > +
> > > +error:
> > > +	pr_err("%s: timeout in reading OCOTP\n", __func__);
> > > +	return -ETIMEDOUT;
> > > +}
> > does this need locking?
> > 
> You are suggesting mutex or spinlock?
This isn't time critical, so I'd advice to use a mutex.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 08/10] ARM: mxs: add ocotp read function

[Uwe Kleine-König]

Hello,

On Tue, Dec 28, 2010 at 10:55:53PM +0800, Shawn Guo wrote:
> diff --git a/arch/arm/mach-mxs/ocotp.c b/arch/arm/mach-mxs/ocotp.c
> new file mode 100644
> index 0000000..24457d7
> --- /dev/null
> +++ b/arch/arm/mach-mxs/ocotp.c
> @@ -0,0 +1,52 @@
> +/*
> + * Copyright 2010 Freescale Semiconductor, Inc. All Rights Reserved.
> + *
> + * 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.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +
> +#include <mach/mxs.h>
> +
> +#define BM_OCOTP_CTRL_RD_BANK_OPEN	(1 << 12)
> +#define BM_OCOTP_CTRL_BUSY		(1 << 8)
> +
> +int mxs_read_ocotp(int offset, int count, u32 *values)
> +{
> +	void __iomem *ocotp_base = MXS_IO_ADDRESS(MXS_OCOTP_BASE_ADDR);
> +	int i, timeout = 0x400;
> +
> +	/* open OCOTP banks for read */
> +	__mxs_setl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);
The reference manual specifies:
	1. Program the HCLK to a frequency up to the maximum allowable HCLK
	   frequency. [...]
	2. Check that HW_OCOTP_CTRL_BUSY and HW_OCOTP_CTRL_ERROR are clear.
	3. Set HW_OCOTP_CTRL_RD_BANK_OPEN. [...]

1. isn't done (which is probably OK, or should it aquire a clk?)
For 2. there is no check for HW_OCOTP_CTRL_ERROR which is not OK i
guess?!

(Independant of that, shouldn't the list use BM_ prefixes as you did in
the code?)

> +
> +	/* approximately wait 32 hclk cycles */
> +	udelay(1);
> +
> +	/* poll BUSY bit becoming cleared */
> +	while ((__raw_readl(ocotp_base) & BM_OCOTP_CTRL_BUSY) && --timeout)
> +		/* nothing */;
> +
> +	if (unlikely(!timeout))
> +		goto error;
And I think you should recheck for BM_OCOTP_CTRL_RD_BANK_OPEN here (step
4 from the reference manual).

> +
> +	for (i = 0; i < count; i++, offset += 4)
> +		*values++ = __raw_readl(ocotp_base + offset);
> +
> +	/* close banks for power saving */
> +	__mxs_clrl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);
> +
> +	return 0;
> +
> +error:
> +	pr_err("%s: timeout in reading OCOTP\n", __func__);
> +	return -ETIMEDOUT;
> +}
Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 08/10] ARM: mxs: add ocotp read function

[Shawn Guo]

Hi Uwe,

On Wed, Dec 29, 2010 at 12:22:08PM +0100, Uwe Kleine-K?nig wrote:
> Hello,
> 
> On Tue, Dec 28, 2010 at 10:55:53PM +0800, Shawn Guo wrote:
> > diff --git a/arch/arm/mach-mxs/ocotp.c b/arch/arm/mach-mxs/ocotp.c
> > new file mode 100644
> > index 0000000..24457d7
> > --- /dev/null
> > +++ b/arch/arm/mach-mxs/ocotp.c
> > @@ -0,0 +1,52 @@
> > +/*
> > + * Copyright 2010 Freescale Semiconductor, Inc. All Rights Reserved.
> > + *
> > + * 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.
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +
> > +#include <mach/mxs.h>
> > +
> > +#define BM_OCOTP_CTRL_RD_BANK_OPEN	(1 << 12)
> > +#define BM_OCOTP_CTRL_BUSY		(1 << 8)
> > +
> > +int mxs_read_ocotp(int offset, int count, u32 *values)
> > +{
> > +	void __iomem *ocotp_base = MXS_IO_ADDRESS(MXS_OCOTP_BASE_ADDR);
> > +	int i, timeout = 0x400;
> > +
> > +	/* open OCOTP banks for read */
> > +	__mxs_setl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);
> The reference manual specifies:
> 	1. Program the HCLK to a frequency up to the maximum allowable HCLK
> 	   frequency. [...]
> 	2. Check that HW_OCOTP_CTRL_BUSY and HW_OCOTP_CTRL_ERROR are clear.
> 	3. Set HW_OCOTP_CTRL_RD_BANK_OPEN. [...]
> 
> 1. isn't done (which is probably OK, or should it aquire a clk?)
> For 2. there is no check for HW_OCOTP_CTRL_ERROR which is not OK i
> guess?!
> 
Will add the check. Thanks.

> (Independant of that, shouldn't the list use BM_ prefixes as you did in
> the code?)
> 
Document description uses register name followed by bit-field name
to mean the bit.

> > +
> > +	/* approximately wait 32 hclk cycles */
> > +	udelay(1);
> > +
> > +	/* poll BUSY bit becoming cleared */
> > +	while ((__raw_readl(ocotp_base) & BM_OCOTP_CTRL_BUSY) && --timeout)
> > +		/* nothing */;
> > +
> > +	if (unlikely(!timeout))
> > +		goto error;
> And I think you should recheck for BM_OCOTP_CTRL_RD_BANK_OPEN here (step
> 4 from the reference manual).
> 
The RM says:

Poll for HW_OCOTP_CTRL_BUSY clear. When HW_OCOTP_CTRL_BUSY is clear
and HW_OCOTP_CTRL_RD_BANK_OPEN is set, read the data from the
appropriate memory-mapped address.

My understanding is we only need to poll busy bit clear. And the
second sentence just tells that read operation should be working
when bit busy clear and rd_bank_open set, which has been done
in step 3. I also consulted the designer and was told that only
busy clear polling is needed.

-- 
Regards,
Shawn

[PATCH 08/10] ARM: mxs: add ocotp read function

[Uwe Kleine-König]

Hello Shawn,

On Thu, Dec 30, 2010 at 01:50:12PM +0800, Shawn Guo wrote:
> On Wed, Dec 29, 2010 at 12:22:08PM +0100, Uwe Kleine-K?nig wrote:
> > On Tue, Dec 28, 2010 at 10:55:53PM +0800, Shawn Guo wrote:
> > > diff --git a/arch/arm/mach-mxs/ocotp.c b/arch/arm/mach-mxs/ocotp.c
> > > new file mode 100644
> > > index 0000000..24457d7
> > > --- /dev/null
> > > +++ b/arch/arm/mach-mxs/ocotp.c
> > > @@ -0,0 +1,52 @@
> > > +/*
> > > + * Copyright 2010 Freescale Semiconductor, Inc. All Rights Reserved.
> > > + *
> > > + * 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.
> > > + */
> > > +
> > > +#include <linux/delay.h>
> > > +#include <linux/err.h>
> > > +
> > > +#include <mach/mxs.h>
> > > +
> > > +#define BM_OCOTP_CTRL_RD_BANK_OPEN	(1 << 12)
> > > +#define BM_OCOTP_CTRL_BUSY		(1 << 8)
> > > +
> > > +int mxs_read_ocotp(int offset, int count, u32 *values)
> > > +{
> > > +	void __iomem *ocotp_base = MXS_IO_ADDRESS(MXS_OCOTP_BASE_ADDR);
> > > +	int i, timeout = 0x400;
> > > +
> > > +	/* open OCOTP banks for read */
> > > +	__mxs_setl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);
> > The reference manual specifies:
> > 	1. Program the HCLK to a frequency up to the maximum allowable HCLK
> > 	   frequency. [...]
> > 	2. Check that HW_OCOTP_CTRL_BUSY and HW_OCOTP_CTRL_ERROR are clear.
> > 	3. Set HW_OCOTP_CTRL_RD_BANK_OPEN. [...]
> > 
> > 1. isn't done (which is probably OK, or should it aquire a clk?)
> > For 2. there is no check for HW_OCOTP_CTRL_ERROR which is not OK i
> > guess?!
> > 
> Will add the check. Thanks.
> 
> > (Independant of that, shouldn't the list use BM_ prefixes as you did in
> > the code?)
> > 
> Document description uses register name followed by bit-field name
> to mean the bit.
After rereading chapter 39.4 (Register Macro Usage/Naming Convention)
I wonder if the BM_ values are shifted or not.  If they are not (as I
think it is meant) your usage is inconsitent with the manual (but IMHO
sane).  sigh.

> > > +	/* approximately wait 32 hclk cycles */
> > > +	udelay(1);
> > > +
> > > +	/* poll BUSY bit becoming cleared */
> > > +	while ((__raw_readl(ocotp_base) & BM_OCOTP_CTRL_BUSY) && --timeout)
> > > +		/* nothing */;
> > > +
> > > +	if (unlikely(!timeout))
> > > +		goto error;
> > And I think you should recheck for BM_OCOTP_CTRL_RD_BANK_OPEN here (step
> > 4 from the reference manual).
> > 
> The RM says:
> 
> Poll for HW_OCOTP_CTRL_BUSY clear. When HW_OCOTP_CTRL_BUSY is clear
> and HW_OCOTP_CTRL_RD_BANK_OPEN is set, read the data from the
> appropriate memory-mapped address.
> 
> My understanding is we only need to poll busy bit clear. And the
> second sentence just tells that read operation should be working
> when bit busy clear and rd_bank_open set, which has been done
> in step 3. I also consulted the designer and was told that only
> busy clear polling is needed.
I expected to need this (shortend the names to improve readability):

	while (__raw_readl(ocotp_base) & BUSY) && --timeout);

	if (unlikely((__raw_readl(ocotp_base) & (BUSY | RDBOPEN)) != RDBOPEN))
		goto error_unlock;

*shrug*

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 08/10] ARM: mxs: add ocotp read function

[Shawn Guo]

On Thu, Dec 30, 2010 at 10:15:41AM +0100, Uwe Kleine-K?nig wrote:
> Hello Shawn,
> 
> On Thu, Dec 30, 2010 at 01:50:12PM +0800, Shawn Guo wrote:
> > On Wed, Dec 29, 2010 at 12:22:08PM +0100, Uwe Kleine-K?nig wrote:
> > > On Tue, Dec 28, 2010 at 10:55:53PM +0800, Shawn Guo wrote:
> > > > diff --git a/arch/arm/mach-mxs/ocotp.c b/arch/arm/mach-mxs/ocotp.c
> > > > new file mode 100644
> > > > index 0000000..24457d7
> > > > --- /dev/null
> > > > +++ b/arch/arm/mach-mxs/ocotp.c
> > > > @@ -0,0 +1,52 @@
> > > > +/*
> > > > + * Copyright 2010 Freescale Semiconductor, Inc. All Rights Reserved.
> > > > + *
> > > > + * 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.
> > > > + */
> > > > +
> > > > +#include <linux/delay.h>
> > > > +#include <linux/err.h>
> > > > +
> > > > +#include <mach/mxs.h>
> > > > +
> > > > +#define BM_OCOTP_CTRL_RD_BANK_OPEN	(1 << 12)
> > > > +#define BM_OCOTP_CTRL_BUSY		(1 << 8)
> > > > +
> > > > +int mxs_read_ocotp(int offset, int count, u32 *values)
> > > > +{
> > > > +	void __iomem *ocotp_base = MXS_IO_ADDRESS(MXS_OCOTP_BASE_ADDR);
> > > > +	int i, timeout = 0x400;
> > > > +
> > > > +	/* open OCOTP banks for read */
> > > > +	__mxs_setl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);
> > > The reference manual specifies:
> > > 	1. Program the HCLK to a frequency up to the maximum allowable HCLK
> > > 	   frequency. [...]
> > > 	2. Check that HW_OCOTP_CTRL_BUSY and HW_OCOTP_CTRL_ERROR are clear.
> > > 	3. Set HW_OCOTP_CTRL_RD_BANK_OPEN. [...]
> > > 
> > > 1. isn't done (which is probably OK, or should it aquire a clk?)
> > > For 2. there is no check for HW_OCOTP_CTRL_ERROR which is not OK i
> > > guess?!
> > > 
> > Will add the check. Thanks.
> > 
> > > (Independant of that, shouldn't the list use BM_ prefixes as you did in
> > > the code?)
> > > 
> > Document description uses register name followed by bit-field name
> > to mean the bit.
> After rereading chapter 39.4 (Register Macro Usage/Naming Convention)
> I wonder if the BM_ values are shifted or not.  If they are not (as I
> think it is meant) your usage is inconsitent with the manual (but IMHO
> sane).  sigh.
> 
They are shifted. You can check regs-clkctrl-mx28.h as an example.

> > > > +	/* approximately wait 32 hclk cycles */
> > > > +	udelay(1);
> > > > +
> > > > +	/* poll BUSY bit becoming cleared */
> > > > +	while ((__raw_readl(ocotp_base) & BM_OCOTP_CTRL_BUSY) && --timeout)
> > > > +		/* nothing */;
> > > > +
> > > > +	if (unlikely(!timeout))
> > > > +		goto error;
> > > And I think you should recheck for BM_OCOTP_CTRL_RD_BANK_OPEN here (step
> > > 4 from the reference manual).
> > > 
> > The RM says:
> > 
> > Poll for HW_OCOTP_CTRL_BUSY clear. When HW_OCOTP_CTRL_BUSY is clear
> > and HW_OCOTP_CTRL_RD_BANK_OPEN is set, read the data from the
> > appropriate memory-mapped address.
> > 
> > My understanding is we only need to poll busy bit clear. And the
> > second sentence just tells that read operation should be working
> > when bit busy clear and rd_bank_open set, which has been done
> > in step 3. I also consulted the designer and was told that only
> > busy clear polling is needed.
> I expected to need this (shortend the names to improve readability):
> 
IMHO, BM_OCOTP_CTRL_BUSY is more readable than BUSY. Reader can
understand which bit it is more easily than BUSY.

> 	while (__raw_readl(ocotp_base) & BUSY) && --timeout);
> 
> 	if (unlikely((__raw_readl(ocotp_base) & (BUSY | RDBOPEN)) != RDBOPEN))
> 		goto error_unlock;
> 
I still did not get it. When we set bit RDBOPEN in step 3, it gets set.
That is it. Why do we still need to bother the unnecessary checking?

> *shrug*
> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-K?nig            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> 

-- 
Regards,
Shawn

[PATCH 08/10] ARM: mxs: add ocotp read function

[Uwe Kleine-König]

Hello Shawn,

On Fri, Dec 31, 2010 at 09:43:55AM +0800, Shawn Guo wrote:
> On Thu, Dec 30, 2010 at 10:15:41AM +0100, Uwe Kleine-K?nig wrote:
> > Hello Shawn,
> > 
> > On Thu, Dec 30, 2010 at 01:50:12PM +0800, Shawn Guo wrote:
> > > On Wed, Dec 29, 2010 at 12:22:08PM +0100, Uwe Kleine-K?nig wrote:
> > > > On Tue, Dec 28, 2010 at 10:55:53PM +0800, Shawn Guo wrote:
> > > > > diff --git a/arch/arm/mach-mxs/ocotp.c b/arch/arm/mach-mxs/ocotp.c
> > > > > new file mode 100644
> > > > > index 0000000..24457d7
> > > > > --- /dev/null
> > > > > +++ b/arch/arm/mach-mxs/ocotp.c
> > > > > @@ -0,0 +1,52 @@
> > > > > +/*
> > > > > + * Copyright 2010 Freescale Semiconductor, Inc. All Rights Reserved.
> > > > > + *
> > > > > + * 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.
> > > > > + */
> > > > > +
> > > > > +#include <linux/delay.h>
> > > > > +#include <linux/err.h>
> > > > > +
> > > > > +#include <mach/mxs.h>
> > > > > +
> > > > > +#define BM_OCOTP_CTRL_RD_BANK_OPEN	(1 << 12)
> > > > > +#define BM_OCOTP_CTRL_BUSY		(1 << 8)
> > > > > +
> > > > > +int mxs_read_ocotp(int offset, int count, u32 *values)
> > > > > +{
> > > > > +	void __iomem *ocotp_base = MXS_IO_ADDRESS(MXS_OCOTP_BASE_ADDR);
> > > > > +	int i, timeout = 0x400;
> > > > > +
> > > > > +	/* open OCOTP banks for read */
> > > > > +	__mxs_setl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);
> > > > The reference manual specifies:
> > > > 	1. Program the HCLK to a frequency up to the maximum allowable HCLK
> > > > 	   frequency. [...]
> > > > 	2. Check that HW_OCOTP_CTRL_BUSY and HW_OCOTP_CTRL_ERROR are clear.
> > > > 	3. Set HW_OCOTP_CTRL_RD_BANK_OPEN. [...]
> > > > 
> > > > 1. isn't done (which is probably OK, or should it aquire a clk?)
> > > > For 2. there is no check for HW_OCOTP_CTRL_ERROR which is not OK i
> > > > guess?!
> > > > 
> > > Will add the check. Thanks.
> > > 
> > > > (Independant of that, shouldn't the list use BM_ prefixes as you did in
> > > > the code?)
> > > > 
> > > Document description uses register name followed by bit-field name
> > > to mean the bit.
> > After rereading chapter 39.4 (Register Macro Usage/Naming Convention)
> > I wonder if the BM_ values are shifted or not.  If they are not (as I
> > think it is meant) your usage is inconsitent with the manual (but IMHO
> > sane).  sigh.
> > 
> They are shifted. You can check regs-clkctrl-mx28.h as an example.
> 
> > > > > +	/* approximately wait 32 hclk cycles */
> > > > > +	udelay(1);
> > > > > +
> > > > > +	/* poll BUSY bit becoming cleared */
> > > > > +	while ((__raw_readl(ocotp_base) & BM_OCOTP_CTRL_BUSY) && --timeout)
> > > > > +		/* nothing */;
> > > > > +
> > > > > +	if (unlikely(!timeout))
> > > > > +		goto error;
> > > > And I think you should recheck for BM_OCOTP_CTRL_RD_BANK_OPEN here (step
> > > > 4 from the reference manual).
> > > > 
> > > The RM says:
> > > 
> > > Poll for HW_OCOTP_CTRL_BUSY clear. When HW_OCOTP_CTRL_BUSY is clear
> > > and HW_OCOTP_CTRL_RD_BANK_OPEN is set, read the data from the
> > > appropriate memory-mapped address.
> > > 
> > > My understanding is we only need to poll busy bit clear. And the
> > > second sentence just tells that read operation should be working
> > > when bit busy clear and rd_bank_open set, which has been done
> > > in step 3. I also consulted the designer and was told that only
> > > busy clear polling is needed.
> > I expected to need this (shortend the names to improve readability):
> > 
> IMHO, BM_OCOTP_CTRL_BUSY is more readable than BUSY. Reader can
> understand which bit it is more easily than BUSY.
I only shortend it for the mail.  For a patch of course use the full
name.
 
> > 	while (__raw_readl(ocotp_base) & BUSY) && --timeout);
> > 
> > 	if (unlikely((__raw_readl(ocotp_base) & (BUSY | RDBOPEN)) != RDBOPEN))
> > 		goto error_unlock;
> > 
> I still did not get it. When we set bit RDBOPEN in step 3, it gets set.
> That is it. Why do we still need to bother the unnecessary checking?
That's how I interpret the hardware reference.  And note, registers are
not memory.  Just because you write a value to it it doesn't mean it is
set when you reread it.

Best regards and happy new year (don't know if you have that? :-)
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 08/10] ARM: mxs: add ocotp read function

[Shawn Guo]

On Fri, Dec 31, 2010 at 05:11:41PM +0100, Uwe Kleine-K?nig wrote:
> Hello Shawn,
> 
> On Fri, Dec 31, 2010 at 09:43:55AM +0800, Shawn Guo wrote:
> > On Thu, Dec 30, 2010 at 10:15:41AM +0100, Uwe Kleine-K?nig wrote:
> > > Hello Shawn,
> > > 
> > > On Thu, Dec 30, 2010 at 01:50:12PM +0800, Shawn Guo wrote:
> > > > On Wed, Dec 29, 2010 at 12:22:08PM +0100, Uwe Kleine-K?nig wrote:
> > > > > On Tue, Dec 28, 2010 at 10:55:53PM +0800, Shawn Guo wrote:
> > > > > > diff --git a/arch/arm/mach-mxs/ocotp.c b/arch/arm/mach-mxs/ocotp.c
> > > > > > new file mode 100644
> > > > > > index 0000000..24457d7
> > > > > > --- /dev/null
> > > > > > +++ b/arch/arm/mach-mxs/ocotp.c
> > > > > > @@ -0,0 +1,52 @@
> > > > > > +/*
> > > > > > + * Copyright 2010 Freescale Semiconductor, Inc. All Rights Reserved.
> > > > > > + *
> > > > > > + * 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.
> > > > > > + */
> > > > > > +
> > > > > > +#include <linux/delay.h>
> > > > > > +#include <linux/err.h>
> > > > > > +
> > > > > > +#include <mach/mxs.h>
> > > > > > +
> > > > > > +#define BM_OCOTP_CTRL_RD_BANK_OPEN	(1 << 12)
> > > > > > +#define BM_OCOTP_CTRL_BUSY		(1 << 8)
> > > > > > +
> > > > > > +int mxs_read_ocotp(int offset, int count, u32 *values)
> > > > > > +{
> > > > > > +	void __iomem *ocotp_base = MXS_IO_ADDRESS(MXS_OCOTP_BASE_ADDR);
> > > > > > +	int i, timeout = 0x400;
> > > > > > +
> > > > > > +	/* open OCOTP banks for read */
> > > > > > +	__mxs_setl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);
> > > > > The reference manual specifies:
> > > > > 	1. Program the HCLK to a frequency up to the maximum allowable HCLK
> > > > > 	   frequency. [...]
> > > > > 	2. Check that HW_OCOTP_CTRL_BUSY and HW_OCOTP_CTRL_ERROR are clear.
> > > > > 	3. Set HW_OCOTP_CTRL_RD_BANK_OPEN. [...]
> > > > > 
> > > > > 1. isn't done (which is probably OK, or should it aquire a clk?)
> > > > > For 2. there is no check for HW_OCOTP_CTRL_ERROR which is not OK i
> > > > > guess?!
> > > > > 
> > > > Will add the check. Thanks.
> > > > 
> > > > > (Independant of that, shouldn't the list use BM_ prefixes as you did in
> > > > > the code?)
> > > > > 
> > > > Document description uses register name followed by bit-field name
> > > > to mean the bit.
> > > After rereading chapter 39.4 (Register Macro Usage/Naming Convention)
> > > I wonder if the BM_ values are shifted or not.  If they are not (as I
> > > think it is meant) your usage is inconsitent with the manual (but IMHO
> > > sane).  sigh.
> > > 
> > They are shifted. You can check regs-clkctrl-mx28.h as an example.
> > 
> > > > > > +	/* approximately wait 32 hclk cycles */
> > > > > > +	udelay(1);
> > > > > > +
> > > > > > +	/* poll BUSY bit becoming cleared */
> > > > > > +	while ((__raw_readl(ocotp_base) & BM_OCOTP_CTRL_BUSY) && --timeout)
> > > > > > +		/* nothing */;
> > > > > > +
> > > > > > +	if (unlikely(!timeout))
> > > > > > +		goto error;
> > > > > And I think you should recheck for BM_OCOTP_CTRL_RD_BANK_OPEN here (step
> > > > > 4 from the reference manual).
> > > > > 
> > > > The RM says:
> > > > 
> > > > Poll for HW_OCOTP_CTRL_BUSY clear. When HW_OCOTP_CTRL_BUSY is clear
> > > > and HW_OCOTP_CTRL_RD_BANK_OPEN is set, read the data from the
> > > > appropriate memory-mapped address.
> > > > 
> > > > My understanding is we only need to poll busy bit clear. And the
> > > > second sentence just tells that read operation should be working
> > > > when bit busy clear and rd_bank_open set, which has been done
> > > > in step 3. I also consulted the designer and was told that only
> > > > busy clear polling is needed.
> > > I expected to need this (shortend the names to improve readability):
> > > 
> > IMHO, BM_OCOTP_CTRL_BUSY is more readable than BUSY. Reader can
> > understand which bit it is more easily than BUSY.
> I only shortend it for the mail.  For a patch of course use the full
> name.
>  
> > > 	while (__raw_readl(ocotp_base) & BUSY) && --timeout);
> > > 
> > > 	if (unlikely((__raw_readl(ocotp_base) & (BUSY | RDBOPEN)) != RDBOPEN))
> > > 		goto error_unlock;
> > > 
> > I still did not get it. When we set bit RDBOPEN in step 3, it gets set.
> > That is it. Why do we still need to bother the unnecessary checking?
> That's how I interpret the hardware reference.  And note, registers are
> not memory.  Just because you write a value to it it doesn't mean it is
> set when you reread it.
> 
The note is right. That why I asked designer whether the bit will
definitely get set or not if software sets the bit implicitly.
The answer I got is yes.

> Best regards and happy new year (don't know if you have that? :-)
I have that holiday, but I still need to check your reply ;)

> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-K?nig            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> 

-- 
Regards,
Shawn

[PATCH 08/10] ARM: mxs: add ocotp read function

[Shawn Guo]

On Wed, Dec 29, 2010 at 12:22:08PM +0100, Uwe Kleine-K?nig wrote:
> Hello,
> 
> On Tue, Dec 28, 2010 at 10:55:53PM +0800, Shawn Guo wrote:
> > diff --git a/arch/arm/mach-mxs/ocotp.c b/arch/arm/mach-mxs/ocotp.c
> > new file mode 100644
> > index 0000000..24457d7
> > --- /dev/null
> > +++ b/arch/arm/mach-mxs/ocotp.c
> > @@ -0,0 +1,52 @@
> > +/*
> > + * Copyright 2010 Freescale Semiconductor, Inc. All Rights Reserved.
> > + *
> > + * 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.
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +
> > +#include <mach/mxs.h>
> > +
> > +#define BM_OCOTP_CTRL_RD_BANK_OPEN	(1 << 12)
> > +#define BM_OCOTP_CTRL_BUSY		(1 << 8)
> > +
> > +int mxs_read_ocotp(int offset, int count, u32 *values)
> > +{
> > +	void __iomem *ocotp_base = MXS_IO_ADDRESS(MXS_OCOTP_BASE_ADDR);
> > +	int i, timeout = 0x400;
> > +
> > +	/* open OCOTP banks for read */
> > +	__mxs_setl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);
> The reference manual specifies:
> 	1. Program the HCLK to a frequency up to the maximum allowable HCLK
> 	   frequency. [...]
> 	2. Check that HW_OCOTP_CTRL_BUSY and HW_OCOTP_CTRL_ERROR are clear.
> 	3. Set HW_OCOTP_CTRL_RD_BANK_OPEN. [...]
> 
> 1. isn't done (which is probably OK, or should it aquire a clk?)
ocotp needs clk_h, which must be on when system is running.

> For 2. there is no check for HW_OCOTP_CTRL_ERROR which is not OK i
> guess?!
> 
How does the new code look to you?

static DEFINE_MUTEX(ocotp_mutex);

int mxs_read_ocotp(unsigned offset, size_t count, u32 *values)
{
	void __iomem *ocotp_base = MXS_IO_ADDRESS(MXS_OCOTP_BASE_ADDR);
	int i, timeout = 0x400;

	mutex_lock(&ocotp_mutex);

	/* clear ERROR bit anyway */
	__mxs_clrl(BM_OCOTP_CTRL_ERROR, ocotp_base);

	/* check both BUSY and ERROR cleared */
	while ((__raw_readl(ocotp_base) &
		(BM_OCOTP_CTRL_BUSY | BM_OCOTP_CTRL_ERROR)) && --timeout)
		/* nothing */;

	if (unlikely(!timeout))
		goto error;

	/* open OCOTP banks for read */
	__mxs_setl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);

	/* approximately wait 32 hclk cycles */
	udelay(1);

	/* poll BUSY bit becoming cleared */
	timeout = 0x400;
	while ((__raw_readl(ocotp_base) & BM_OCOTP_CTRL_BUSY) && --timeout)
		/* nothing */;

	if (unlikely(!timeout))
		goto error;

	for (i = 0; i < count; i++, offset += 4)
		*values++ = __raw_readl(ocotp_base + offset);

	/* close banks for power saving */
	__mxs_clrl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);

	mutex_unlock(&ocotp_mutex);

	return 0;

error:
	pr_err("%s: timeout in reading OCOTP\n", __func__);
	return -ETIMEDOUT;
}

-- 
Regards,
Shawn

[PATCH 08/10] ARM: mxs: add ocotp read function

[Uwe Kleine-König]

Hello Shawn,

On Thu, Dec 30, 2010 at 04:41:40PM +0800, Shawn Guo wrote:
> On Wed, Dec 29, 2010 at 12:22:08PM +0100, Uwe Kleine-K?nig wrote:
> > On Tue, Dec 28, 2010 at 10:55:53PM +0800, Shawn Guo wrote:
> > > diff --git a/arch/arm/mach-mxs/ocotp.c b/arch/arm/mach-mxs/ocotp.c
> > > new file mode 100644
> > > index 0000000..24457d7
> > > --- /dev/null
> > > +++ b/arch/arm/mach-mxs/ocotp.c
> > > @@ -0,0 +1,52 @@
> > > +/*
> > > + * Copyright 2010 Freescale Semiconductor, Inc. All Rights Reserved.
> > > + *
> > > + * 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.
> > > + */
> > > +
> > > +#include <linux/delay.h>
> > > +#include <linux/err.h>
> > > +
> > > +#include <mach/mxs.h>
> > > +
> > > +#define BM_OCOTP_CTRL_RD_BANK_OPEN	(1 << 12)
> > > +#define BM_OCOTP_CTRL_BUSY		(1 << 8)
> > > +
> > > +int mxs_read_ocotp(int offset, int count, u32 *values)
> > > +{
> > > +	void __iomem *ocotp_base = MXS_IO_ADDRESS(MXS_OCOTP_BASE_ADDR);
> > > +	int i, timeout = 0x400;
> > > +
> > > +	/* open OCOTP banks for read */
> > > +	__mxs_setl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);
> > The reference manual specifies:
> > 	1. Program the HCLK to a frequency up to the maximum allowable HCLK
> > 	   frequency. [...]
> > 	2. Check that HW_OCOTP_CTRL_BUSY and HW_OCOTP_CTRL_ERROR are clear.
> > 	3. Set HW_OCOTP_CTRL_RD_BANK_OPEN. [...]
> > 
> > 1. isn't done (which is probably OK, or should it aquire a clk?)
> ocotp needs clk_h, which must be on when system is running.
maybe add a comment?  "clk_enable(clk_h) can be skipped as it must be on
anyhow because ..." or similar?  It's always good to document implicit
assumptions.

> > For 2. there is no check for HW_OCOTP_CTRL_ERROR which is not OK i
> > guess?!
> > 
> How does the new code look to you?
> 
> static DEFINE_MUTEX(ocotp_mutex);
> 
> int mxs_read_ocotp(unsigned offset, size_t count, u32 *values)
> {
> 	void __iomem *ocotp_base = MXS_IO_ADDRESS(MXS_OCOTP_BASE_ADDR);
> 	int i, timeout = 0x400;
i should be size_t, too

> 
> 	mutex_lock(&ocotp_mutex);
> 
> 	/* clear ERROR bit anyway */
anyway?  Better?: "try to clear ERROR bit"

> 	__mxs_clrl(BM_OCOTP_CTRL_ERROR, ocotp_base);
> 
> 	/* check both BUSY and ERROR cleared */
> 	while ((__raw_readl(ocotp_base) &
> 		(BM_OCOTP_CTRL_BUSY | BM_OCOTP_CTRL_ERROR)) && --timeout)
> 		/* nothing */;
> 
> 	if (unlikely(!timeout))
> 		goto error;
This goto and some more leave the critical section without unlocking the
mutex.

> 
> 	/* open OCOTP banks for read */
> 	__mxs_setl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);
> 
> 	/* approximately wait 32 hclk cycles */
> 	udelay(1);
> 
> 	/* poll BUSY bit becoming cleared */
> 	timeout = 0x400;
> 	while ((__raw_readl(ocotp_base) & BM_OCOTP_CTRL_BUSY) && --timeout)
> 		/* nothing */;
> 
> 	if (unlikely(!timeout))
> 		goto error;
> 
> 	for (i = 0; i < count; i++, offset += 4)
> 		*values++ = __raw_readl(ocotp_base + offset);
> 
> 	/* close banks for power saving */
> 	__mxs_clrl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);
> 
> 	mutex_unlock(&ocotp_mutex);
> 
> 	return 0;
> 
> error:
> 	pr_err("%s: timeout in reading OCOTP\n", __func__);
> 	return -ETIMEDOUT;
> }
I think it's OK then.  One optimisation that comes to my mind is to make
BM_OCOTP_CTRL_RD_BANK_OPEN a clk (that can depend on clk_h).  This would
allow for multiple readers, but I think it's not worth the effort.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 08/10] ARM: mxs: add ocotp read function

[Shawn Guo]

Hi Uwe,

On Thu, Dec 30, 2010 at 10:02:24AM +0100, Uwe Kleine-K?nig wrote:
> Hello Shawn,
> 
> On Thu, Dec 30, 2010 at 04:41:40PM +0800, Shawn Guo wrote:
> > On Wed, Dec 29, 2010 at 12:22:08PM +0100, Uwe Kleine-K?nig wrote:
> > > On Tue, Dec 28, 2010 at 10:55:53PM +0800, Shawn Guo wrote:
> > > > diff --git a/arch/arm/mach-mxs/ocotp.c b/arch/arm/mach-mxs/ocotp.c
> > > > new file mode 100644
> > > > index 0000000..24457d7
> > > > --- /dev/null
> > > > +++ b/arch/arm/mach-mxs/ocotp.c
> > > > @@ -0,0 +1,52 @@
> > > > +/*
> > > > + * Copyright 2010 Freescale Semiconductor, Inc. All Rights Reserved.
> > > > + *
> > > > + * 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.
> > > > + */
> > > > +
> > > > +#include <linux/delay.h>
> > > > +#include <linux/err.h>
> > > > +
> > > > +#include <mach/mxs.h>
> > > > +
> > > > +#define BM_OCOTP_CTRL_RD_BANK_OPEN	(1 << 12)
> > > > +#define BM_OCOTP_CTRL_BUSY		(1 << 8)
> > > > +
> > > > +int mxs_read_ocotp(int offset, int count, u32 *values)
> > > > +{
> > > > +	void __iomem *ocotp_base = MXS_IO_ADDRESS(MXS_OCOTP_BASE_ADDR);
> > > > +	int i, timeout = 0x400;
> > > > +
> > > > +	/* open OCOTP banks for read */
> > > > +	__mxs_setl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);
> > > The reference manual specifies:
> > > 	1. Program the HCLK to a frequency up to the maximum allowable HCLK
> > > 	   frequency. [...]
> > > 	2. Check that HW_OCOTP_CTRL_BUSY and HW_OCOTP_CTRL_ERROR are clear.
> > > 	3. Set HW_OCOTP_CTRL_RD_BANK_OPEN. [...]
> > > 
> > > 1. isn't done (which is probably OK, or should it aquire a clk?)
> > ocotp needs clk_h, which must be on when system is running.
> maybe add a comment?  "clk_enable(clk_h) can be skipped as it must be on
> anyhow because ..." or similar?  It's always good to document implicit
> assumptions.
> 
OK.

> > > For 2. there is no check for HW_OCOTP_CTRL_ERROR which is not OK i
> > > guess?!
> > > 
> > How does the new code look to you?
> > 
> > static DEFINE_MUTEX(ocotp_mutex);
> > 
> > int mxs_read_ocotp(unsigned offset, size_t count, u32 *values)
> > {
> > 	void __iomem *ocotp_base = MXS_IO_ADDRESS(MXS_OCOTP_BASE_ADDR);
> > 	int i, timeout = 0x400;
> i should be size_t, too
> 
OK.

> > 
> > 	mutex_lock(&ocotp_mutex);
> > 
> > 	/* clear ERROR bit anyway */
> anyway?  Better?: "try to clear ERROR bit"
> 
> > 	__mxs_clrl(BM_OCOTP_CTRL_ERROR, ocotp_base);
> > 
> > 	/* check both BUSY and ERROR cleared */
> > 	while ((__raw_readl(ocotp_base) &
> > 		(BM_OCOTP_CTRL_BUSY | BM_OCOTP_CTRL_ERROR)) && --timeout)
> > 		/* nothing */;
> > 
> > 	if (unlikely(!timeout))
> > 		goto error;
> This goto and some more leave the critical section without unlocking the
> mutex.
> 
My bad. Thanks.

> > 
> > 	/* open OCOTP banks for read */
> > 	__mxs_setl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);
> > 
> > 	/* approximately wait 32 hclk cycles */
> > 	udelay(1);
> > 
> > 	/* poll BUSY bit becoming cleared */
> > 	timeout = 0x400;
> > 	while ((__raw_readl(ocotp_base) & BM_OCOTP_CTRL_BUSY) && --timeout)
> > 		/* nothing */;
> > 
> > 	if (unlikely(!timeout))
> > 		goto error;
> > 
> > 	for (i = 0; i < count; i++, offset += 4)
> > 		*values++ = __raw_readl(ocotp_base + offset);
> > 
> > 	/* close banks for power saving */
> > 	__mxs_clrl(BM_OCOTP_CTRL_RD_BANK_OPEN, ocotp_base);
> > 
> > 	mutex_unlock(&ocotp_mutex);
> > 
> > 	return 0;
> > 
> > error:
> > 	pr_err("%s: timeout in reading OCOTP\n", __func__);
> > 	return -ETIMEDOUT;
> > }
> I think it's OK then.  One optimisation that comes to my mind is to make
> BM_OCOTP_CTRL_RD_BANK_OPEN a clk (that can depend on clk_h).  This would
> allow for multiple readers, but I think it's not worth the effort.
> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-K?nig            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> 

-- 
Regards,
Shawn

[PATCH 09/10] ARM: mx28: read fec mac address from ocotp

[Shawn Guo]

Read fec mac address from ocotp and save it into fec_platform_data
mac field for fec driver to use.

Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
---
 arch/arm/mach-mxs/devices/platform-fec.c        |    2 +-
 arch/arm/mach-mxs/include/mach/devices-common.h |    2 +-
 arch/arm/mach-mxs/mach-mx28evk.c                |   31 +++++++++++++++++++++++
 3 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-mxs/devices/platform-fec.c b/arch/arm/mach-mxs/devices/platform-fec.c
index c08168c..dc978ee 100644
--- a/arch/arm/mach-mxs/devices/platform-fec.c
+++ b/arch/arm/mach-mxs/devices/platform-fec.c
@@ -31,7 +31,7 @@ const struct mxs_fec_data mx28_fec_data[] __initconst = {
 
 struct platform_device *__init mxs_add_fec(
 		const struct mxs_fec_data *data,
-		const struct fec_platform_data *pdata)
+		struct fec_platform_data *pdata)
 {
 	struct resource res[] = {
 		{
diff --git a/arch/arm/mach-mxs/include/mach/devices-common.h b/arch/arm/mach-mxs/include/mach/devices-common.h
index fca0551..d206e0a 100644
--- a/arch/arm/mach-mxs/include/mach/devices-common.h
+++ b/arch/arm/mach-mxs/include/mach/devices-common.h
@@ -38,4 +38,4 @@ struct mxs_fec_data {
 };
 struct platform_device *__init mxs_add_fec(
 		const struct mxs_fec_data *data,
-		const struct fec_platform_data *pdata);
+		struct fec_platform_data *pdata);
diff --git a/arch/arm/mach-mxs/mach-mx28evk.c b/arch/arm/mach-mxs/mach-mx28evk.c
index def6519..d133733 100644
--- a/arch/arm/mach-mxs/mach-mx28evk.c
+++ b/arch/arm/mach-mxs/mach-mx28evk.c
@@ -129,6 +129,36 @@ static struct fec_platform_data mx28_fec_pdata[] = {
 	},
 };
 
+static int __init mx28evk_fec_get_mac(void)
+{
+	int i, ret;
+	u32 val;
+
+	/*
+	 * OCOTP only stores the last 4 octets for each mac address,
+	 * so hard-coding the first two octets as Freescale OUI (00:04:9f)
+	 * is needed.
+	 */
+	for (i = 0; i < 2; i++) {
+		ret = mxs_read_ocotp(0x20 + i * 0x10, 1, &val);
+		if (ret)
+			goto error;
+
+		mx28_fec_pdata[i].mac[0] = 0x00;
+		mx28_fec_pdata[i].mac[1] = 0x04;
+		mx28_fec_pdata[i].mac[2] = (val >> 24) & 0xff;
+		mx28_fec_pdata[i].mac[3] = (val >> 16) & 0xff;
+		mx28_fec_pdata[i].mac[4] = (val >> 8) & 0xff;
+		mx28_fec_pdata[i].mac[5] = (val >> 0) & 0xff;
+	}
+
+	return 0;
+
+error:
+	pr_err("%s: timeout when reading fec mac from OCOTP\n", __func__);
+	return -ETIMEDOUT;
+}
+
 static void __init mx28evk_init(void)
 {
 	mxs_iomux_setup_multiple_pads(mx28evk_pads, ARRAY_SIZE(mx28evk_pads));
@@ -136,6 +166,7 @@ static void __init mx28evk_init(void)
 	mx28_add_duart();
 
 	mx28evk_fec_reset();
+	mx28evk_fec_get_mac();
 	mx28_add_fec(0, &mx28_fec_pdata[0]);
 #ifdef CONFIG_FEC2
 	mx28_add_fec(1, &mx28_fec_pdata[1]);
-- 
1.7.1

[PATCH 09/10] ARM: mx28: read fec mac address from ocotp

[Uwe Kleine-König]

Hello Shawn,

On Tue, Dec 28, 2010 at 10:55:54PM +0800, Shawn Guo wrote:
> Read fec mac address from ocotp and save it into fec_platform_data
> mac field for fec driver to use.
> 
> Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> ---
>  arch/arm/mach-mxs/devices/platform-fec.c        |    2 +-
>  arch/arm/mach-mxs/include/mach/devices-common.h |    2 +-
>  arch/arm/mach-mxs/mach-mx28evk.c                |   31 +++++++++++++++++++++++
>  3 files changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-mxs/devices/platform-fec.c b/arch/arm/mach-mxs/devices/platform-fec.c
> index c08168c..dc978ee 100644
> --- a/arch/arm/mach-mxs/devices/platform-fec.c
> +++ b/arch/arm/mach-mxs/devices/platform-fec.c
> @@ -31,7 +31,7 @@ const struct mxs_fec_data mx28_fec_data[] __initconst = {
>  
>  struct platform_device *__init mxs_add_fec(
>  		const struct mxs_fec_data *data,
> -		const struct fec_platform_data *pdata)
> +		struct fec_platform_data *pdata)
>  {
>  	struct resource res[] = {
>  		{
> diff --git a/arch/arm/mach-mxs/include/mach/devices-common.h b/arch/arm/mach-mxs/include/mach/devices-common.h
> index fca0551..d206e0a 100644
> --- a/arch/arm/mach-mxs/include/mach/devices-common.h
> +++ b/arch/arm/mach-mxs/include/mach/devices-common.h
> @@ -38,4 +38,4 @@ struct mxs_fec_data {
>  };
>  struct platform_device *__init mxs_add_fec(
>  		const struct mxs_fec_data *data,
> -		const struct fec_platform_data *pdata);
> +		struct fec_platform_data *pdata);
Why that?

> diff --git a/arch/arm/mach-mxs/mach-mx28evk.c b/arch/arm/mach-mxs/mach-mx28evk.c
> index def6519..d133733 100644
> --- a/arch/arm/mach-mxs/mach-mx28evk.c
> +++ b/arch/arm/mach-mxs/mach-mx28evk.c
> @@ -129,6 +129,36 @@ static struct fec_platform_data mx28_fec_pdata[] = {
>  	},
>  };
>  
> +static int __init mx28evk_fec_get_mac(void)
> +{
> +	int i, ret;
> +	u32 val;
> +
> +	/*
> +	 * OCOTP only stores the last 4 octets for each mac address,
> +	 * so hard-coding the first two octets as Freescale OUI (00:04:9f)
> +	 * is needed.
> +	 */
> +	for (i = 0; i < 2; i++) {
> +		ret = mxs_read_ocotp(0x20 + i * 0x10, 1, &val);
> +		if (ret)
> +			goto error;
> +
> +		mx28_fec_pdata[i].mac[0] = 0x00;
> +		mx28_fec_pdata[i].mac[1] = 0x04;
> +		mx28_fec_pdata[i].mac[2] = (val >> 24) & 0xff;
> +		mx28_fec_pdata[i].mac[3] = (val >> 16) & 0xff;
> +		mx28_fec_pdata[i].mac[4] = (val >> 8) & 0xff;
> +		mx28_fec_pdata[i].mac[5] = (val >> 0) & 0xff;
> +	}
> +
> +	return 0;
> +
> +error:
> +	pr_err("%s: timeout when reading fec mac from OCOTP\n", __func__);
> +	return -ETIMEDOUT;
return ret please.

> +}
I think this should be defined in a more generic place.

> +
>  static void __init mx28evk_init(void)
>  {
>  	mxs_iomux_setup_multiple_pads(mx28evk_pads, ARRAY_SIZE(mx28evk_pads));
> @@ -136,6 +166,7 @@ static void __init mx28evk_init(void)
>  	mx28_add_duart();
>  
>  	mx28evk_fec_reset();
> +	mx28evk_fec_get_mac();
You ignore the return value here.  Intended?

>  	mx28_add_fec(0, &mx28_fec_pdata[0]);
>  #ifdef CONFIG_FEC2
>  	mx28_add_fec(1, &mx28_fec_pdata[1]);
> -- 
> 1.7.1
> 
> 
> 

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 09/10] ARM: mx28: read fec mac address from ocotp

[Shawn Guo]

Hi Uwe,

On Wed, Dec 29, 2010 at 11:53:28AM +0100, Uwe Kleine-K?nig wrote:
> Hello Shawn,
> 
> On Tue, Dec 28, 2010 at 10:55:54PM +0800, Shawn Guo wrote:
> > Read fec mac address from ocotp and save it into fec_platform_data
> > mac field for fec driver to use.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> > ---
> >  arch/arm/mach-mxs/devices/platform-fec.c        |    2 +-
> >  arch/arm/mach-mxs/include/mach/devices-common.h |    2 +-
> >  arch/arm/mach-mxs/mach-mx28evk.c                |   31 +++++++++++++++++++++++
> >  3 files changed, 33 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm/mach-mxs/devices/platform-fec.c b/arch/arm/mach-mxs/devices/platform-fec.c
> > index c08168c..dc978ee 100644
> > --- a/arch/arm/mach-mxs/devices/platform-fec.c
> > +++ b/arch/arm/mach-mxs/devices/platform-fec.c
> > @@ -31,7 +31,7 @@ const struct mxs_fec_data mx28_fec_data[] __initconst = {
> >  
> >  struct platform_device *__init mxs_add_fec(
> >  		const struct mxs_fec_data *data,
> > -		const struct fec_platform_data *pdata)
> > +		struct fec_platform_data *pdata)
> >  {
> >  	struct resource res[] = {
> >  		{
> > diff --git a/arch/arm/mach-mxs/include/mach/devices-common.h b/arch/arm/mach-mxs/include/mach/devices-common.h
> > index fca0551..d206e0a 100644
> > --- a/arch/arm/mach-mxs/include/mach/devices-common.h
> > +++ b/arch/arm/mach-mxs/include/mach/devices-common.h
> > @@ -38,4 +38,4 @@ struct mxs_fec_data {
> >  };
> >  struct platform_device *__init mxs_add_fec(
> >  		const struct mxs_fec_data *data,
> > -		const struct fec_platform_data *pdata);
> > +		struct fec_platform_data *pdata);
> Why that?
> 
pdata->mac needs to get assigned by ocotp read function.

> > diff --git a/arch/arm/mach-mxs/mach-mx28evk.c b/arch/arm/mach-mxs/mach-mx28evk.c
> > index def6519..d133733 100644
> > --- a/arch/arm/mach-mxs/mach-mx28evk.c
> > +++ b/arch/arm/mach-mxs/mach-mx28evk.c
> > @@ -129,6 +129,36 @@ static struct fec_platform_data mx28_fec_pdata[] = {
> >  	},
> >  };
> >  
> > +static int __init mx28evk_fec_get_mac(void)
> > +{
> > +	int i, ret;
> > +	u32 val;
> > +
> > +	/*
> > +	 * OCOTP only stores the last 4 octets for each mac address,
> > +	 * so hard-coding the first two octets as Freescale OUI (00:04:9f)
> > +	 * is needed.
> > +	 */
> > +	for (i = 0; i < 2; i++) {
> > +		ret = mxs_read_ocotp(0x20 + i * 0x10, 1, &val);
> > +		if (ret)
> > +			goto error;
> > +
> > +		mx28_fec_pdata[i].mac[0] = 0x00;
> > +		mx28_fec_pdata[i].mac[1] = 0x04;
> > +		mx28_fec_pdata[i].mac[2] = (val >> 24) & 0xff;
> > +		mx28_fec_pdata[i].mac[3] = (val >> 16) & 0xff;
> > +		mx28_fec_pdata[i].mac[4] = (val >> 8) & 0xff;
> > +		mx28_fec_pdata[i].mac[5] = (val >> 0) & 0xff;
> > +	}
> > +
> > +	return 0;
> > +
> > +error:
> > +	pr_err("%s: timeout when reading fec mac from OCOTP\n", __func__);
> > +	return -ETIMEDOUT;
> return ret please.
> 
OK.

> > +}
> I think this should be defined in a more generic place.
> 
Necessary?

> > +
> >  static void __init mx28evk_init(void)
> >  {
> >  	mxs_iomux_setup_multiple_pads(mx28evk_pads, ARRAY_SIZE(mx28evk_pads));
> > @@ -136,6 +166,7 @@ static void __init mx28evk_init(void)
> >  	mx28_add_duart();
> >  
> >  	mx28evk_fec_reset();
> > +	mx28evk_fec_get_mac();
> You ignore the return value here.  Intended?
> 
Not intended.  Will check and give a warning.

> >  	mx28_add_fec(0, &mx28_fec_pdata[0]);
> >  #ifdef CONFIG_FEC2
> >  	mx28_add_fec(1, &mx28_fec_pdata[1]);
> > -- 
> > 1.7.1
> > 
> > 
> > 
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-K?nig            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> 

-- 
Regards,
Shawn

[PATCH 09/10] ARM: mx28: read fec mac address from ocotp

[Uwe Kleine-König]

Hello Shawn,

On Wed, Dec 29, 2010 at 08:13:55PM +0800, Shawn Guo wrote:
> On Wed, Dec 29, 2010 at 11:53:28AM +0100, Uwe Kleine-K?nig wrote:
> > On Tue, Dec 28, 2010 at 10:55:54PM +0800, Shawn Guo wrote:
> > > Read fec mac address from ocotp and save it into fec_platform_data
> > > mac field for fec driver to use.
> > > 
> > > Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> > > ---
> > >  arch/arm/mach-mxs/devices/platform-fec.c        |    2 +-
> > >  arch/arm/mach-mxs/include/mach/devices-common.h |    2 +-
> > >  arch/arm/mach-mxs/mach-mx28evk.c                |   31 +++++++++++++++++++++++
> > >  3 files changed, 33 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/arm/mach-mxs/devices/platform-fec.c b/arch/arm/mach-mxs/devices/platform-fec.c
> > > index c08168c..dc978ee 100644
> > > --- a/arch/arm/mach-mxs/devices/platform-fec.c
> > > +++ b/arch/arm/mach-mxs/devices/platform-fec.c
> > > @@ -31,7 +31,7 @@ const struct mxs_fec_data mx28_fec_data[] __initconst = {
> > >  
> > >  struct platform_device *__init mxs_add_fec(
> > >  		const struct mxs_fec_data *data,
> > > -		const struct fec_platform_data *pdata)
> > > +		struct fec_platform_data *pdata)
> > >  {
> > >  	struct resource res[] = {
> > >  		{
> > > diff --git a/arch/arm/mach-mxs/include/mach/devices-common.h b/arch/arm/mach-mxs/include/mach/devices-common.h
> > > index fca0551..d206e0a 100644
> > > --- a/arch/arm/mach-mxs/include/mach/devices-common.h
> > > +++ b/arch/arm/mach-mxs/include/mach/devices-common.h
> > > @@ -38,4 +38,4 @@ struct mxs_fec_data {
> > >  };
> > >  struct platform_device *__init mxs_add_fec(
> > >  		const struct mxs_fec_data *data,
> > > -		const struct fec_platform_data *pdata);
> > > +		struct fec_platform_data *pdata);
> > Why that?
> > 
> pdata->mac needs to get assigned by ocotp read function.
... which is done before mxs_add_fec is called.

> > > diff --git a/arch/arm/mach-mxs/mach-mx28evk.c b/arch/arm/mach-mxs/mach-mx28evk.c
> > > index def6519..d133733 100644
> > > --- a/arch/arm/mach-mxs/mach-mx28evk.c
> > > +++ b/arch/arm/mach-mxs/mach-mx28evk.c
> > > @@ -129,6 +129,36 @@ static struct fec_platform_data mx28_fec_pdata[] = {
> > >  	},
> > >  };
> > >  
> > > +static int __init mx28evk_fec_get_mac(void)
> > > +{
> > > +	int i, ret;
> > > +	u32 val;
> > > +
> > > +	/*
> > > +	 * OCOTP only stores the last 4 octets for each mac address,
> > > +	 * so hard-coding the first two octets as Freescale OUI (00:04:9f)
> > > +	 * is needed.
> > > +	 */
> > > +	for (i = 0; i < 2; i++) {
> > > +		ret = mxs_read_ocotp(0x20 + i * 0x10, 1, &val);
> > > +		if (ret)
> > > +			goto error;
> > > +
> > > +		mx28_fec_pdata[i].mac[0] = 0x00;
> > > +		mx28_fec_pdata[i].mac[1] = 0x04;
> > > +		mx28_fec_pdata[i].mac[2] = (val >> 24) & 0xff;
> > > +		mx28_fec_pdata[i].mac[3] = (val >> 16) & 0xff;
> > > +		mx28_fec_pdata[i].mac[4] = (val >> 8) & 0xff;
> > > +		mx28_fec_pdata[i].mac[5] = (val >> 0) & 0xff;
> > > +	}
> > > +
> > > +	return 0;
> > > +
> > > +error:
> > > +	pr_err("%s: timeout when reading fec mac from OCOTP\n", __func__);
> > > +	return -ETIMEDOUT;
> > return ret please.
> > 
> OK.
> 
> > > +}
> > I think this should be defined in a more generic place.
> > 
> Necessary?
I just rechecked with the reference manual and it's not documented that
0x20 and 0x30 are used to hold the mac address.  Then it's probably OK
to keep this function machine-local.

> > > +
> > >  static void __init mx28evk_init(void)
> > >  {
> > >  	mxs_iomux_setup_multiple_pads(mx28evk_pads, ARRAY_SIZE(mx28evk_pads));
> > > @@ -136,6 +166,7 @@ static void __init mx28evk_init(void)
> > >  	mx28_add_duart();
> > >  
> > >  	mx28evk_fec_reset();
> > > +	mx28evk_fec_get_mac();
> > You ignore the return value here.  Intended?
> > 
> Not intended.  Will check and give a warning.
> 
> > >  	mx28_add_fec(0, &mx28_fec_pdata[0]);
... and maybe skip the call to add_fec?  Up to you.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 09/10] ARM: mx28: read fec mac address from ocotp

[Shawn Guo]

Hi Uwe,

On Wed, Dec 29, 2010 at 01:45:06PM +0100, Uwe Kleine-K?nig wrote:
> Hello Shawn,
> 
> On Wed, Dec 29, 2010 at 08:13:55PM +0800, Shawn Guo wrote:
> > On Wed, Dec 29, 2010 at 11:53:28AM +0100, Uwe Kleine-K?nig wrote:
> > > On Tue, Dec 28, 2010 at 10:55:54PM +0800, Shawn Guo wrote:
> > > > Read fec mac address from ocotp and save it into fec_platform_data
> > > > mac field for fec driver to use.
> > > > 
> > > > Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> > > > ---
> > > >  arch/arm/mach-mxs/devices/platform-fec.c        |    2 +-
> > > >  arch/arm/mach-mxs/include/mach/devices-common.h |    2 +-
> > > >  arch/arm/mach-mxs/mach-mx28evk.c                |   31 +++++++++++++++++++++++
> > > >  3 files changed, 33 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/arch/arm/mach-mxs/devices/platform-fec.c b/arch/arm/mach-mxs/devices/platform-fec.c
> > > > index c08168c..dc978ee 100644
> > > > --- a/arch/arm/mach-mxs/devices/platform-fec.c
> > > > +++ b/arch/arm/mach-mxs/devices/platform-fec.c
> > > > @@ -31,7 +31,7 @@ const struct mxs_fec_data mx28_fec_data[] __initconst = {
> > > >  
> > > >  struct platform_device *__init mxs_add_fec(
> > > >  		const struct mxs_fec_data *data,
> > > > -		const struct fec_platform_data *pdata)
> > > > +		struct fec_platform_data *pdata)
> > > >  {
> > > >  	struct resource res[] = {
> > > >  		{
> > > > diff --git a/arch/arm/mach-mxs/include/mach/devices-common.h b/arch/arm/mach-mxs/include/mach/devices-common.h
> > > > index fca0551..d206e0a 100644
> > > > --- a/arch/arm/mach-mxs/include/mach/devices-common.h
> > > > +++ b/arch/arm/mach-mxs/include/mach/devices-common.h
> > > > @@ -38,4 +38,4 @@ struct mxs_fec_data {
> > > >  };
> > > >  struct platform_device *__init mxs_add_fec(
> > > >  		const struct mxs_fec_data *data,
> > > > -		const struct fec_platform_data *pdata);
> > > > +		struct fec_platform_data *pdata);
> > > Why that?
> > > 
> > pdata->mac needs to get assigned by ocotp read function.
> ... which is done before mxs_add_fec is called.
> 
> > > > diff --git a/arch/arm/mach-mxs/mach-mx28evk.c b/arch/arm/mach-mxs/mach-mx28evk.c
> > > > index def6519..d133733 100644
> > > > --- a/arch/arm/mach-mxs/mach-mx28evk.c
> > > > +++ b/arch/arm/mach-mxs/mach-mx28evk.c
> > > > @@ -129,6 +129,36 @@ static struct fec_platform_data mx28_fec_pdata[] = {
> > > >  	},
> > > >  };
> > > >  
> > > > +static int __init mx28evk_fec_get_mac(void)
> > > > +{
> > > > +	int i, ret;
> > > > +	u32 val;
> > > > +
> > > > +	/*
> > > > +	 * OCOTP only stores the last 4 octets for each mac address,
> > > > +	 * so hard-coding the first two octets as Freescale OUI (00:04:9f)
> > > > +	 * is needed.
> > > > +	 */
> > > > +	for (i = 0; i < 2; i++) {
> > > > +		ret = mxs_read_ocotp(0x20 + i * 0x10, 1, &val);
> > > > +		if (ret)
> > > > +			goto error;
> > > > +
> > > > +		mx28_fec_pdata[i].mac[0] = 0x00;
> > > > +		mx28_fec_pdata[i].mac[1] = 0x04;
> > > > +		mx28_fec_pdata[i].mac[2] = (val >> 24) & 0xff;
> > > > +		mx28_fec_pdata[i].mac[3] = (val >> 16) & 0xff;
> > > > +		mx28_fec_pdata[i].mac[4] = (val >> 8) & 0xff;
> > > > +		mx28_fec_pdata[i].mac[5] = (val >> 0) & 0xff;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +
> > > > +error:
> > > > +	pr_err("%s: timeout when reading fec mac from OCOTP\n", __func__);
> > > > +	return -ETIMEDOUT;
> > > return ret please.
> > > 
> > OK.
> > 
> > > > +}
> > > I think this should be defined in a more generic place.
> > > 
> > Necessary?
> I just rechecked with the reference manual and it's not documented that
> 0x20 and 0x30 are used to hold the mac address.  Then it's probably OK
> to keep this function machine-local.
> 
> > > > +
> > > >  static void __init mx28evk_init(void)
> > > >  {
> > > >  	mxs_iomux_setup_multiple_pads(mx28evk_pads, ARRAY_SIZE(mx28evk_pads));
> > > > @@ -136,6 +166,7 @@ static void __init mx28evk_init(void)
> > > >  	mx28_add_duart();
> > > >  
> > > >  	mx28evk_fec_reset();
> > > > +	mx28evk_fec_get_mac();
> > > You ignore the return value here.  Intended?
> > > 
> > Not intended.  Will check and give a warning.
> > 
> > > >  	mx28_add_fec(0, &mx28_fec_pdata[0]);
> ... and maybe skip the call to add_fec?  Up to you.
> 
I would not. fec driver still has the last chance to get mac
address from fec mac registers which may set up by bootloader. 

-- 
Regards,
Shawn

[PATCH 09/10] ARM: mx28: read fec mac address from ocotp

[Shawn Guo]

Hi Uwe,

On Wed, Dec 29, 2010 at 01:45:06PM +0100, Uwe Kleine-K?nig wrote:
> Hello Shawn,
> 
> On Wed, Dec 29, 2010 at 08:13:55PM +0800, Shawn Guo wrote:
> > On Wed, Dec 29, 2010 at 11:53:28AM +0100, Uwe Kleine-K?nig wrote:
> > > On Tue, Dec 28, 2010 at 10:55:54PM +0800, Shawn Guo wrote:
> > > > Read fec mac address from ocotp and save it into fec_platform_data
> > > > mac field for fec driver to use.
> > > > 
> > > > Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> > > > ---
> > > >  arch/arm/mach-mxs/devices/platform-fec.c        |    2 +-
> > > >  arch/arm/mach-mxs/include/mach/devices-common.h |    2 +-
> > > >  arch/arm/mach-mxs/mach-mx28evk.c                |   31 +++++++++++++++++++++++
> > > >  3 files changed, 33 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/arch/arm/mach-mxs/devices/platform-fec.c b/arch/arm/mach-mxs/devices/platform-fec.c
> > > > index c08168c..dc978ee 100644
> > > > --- a/arch/arm/mach-mxs/devices/platform-fec.c
> > > > +++ b/arch/arm/mach-mxs/devices/platform-fec.c
> > > > @@ -31,7 +31,7 @@ const struct mxs_fec_data mx28_fec_data[] __initconst = {
> > > >  
> > > >  struct platform_device *__init mxs_add_fec(
> > > >  		const struct mxs_fec_data *data,
> > > > -		const struct fec_platform_data *pdata)
> > > > +		struct fec_platform_data *pdata)
> > > >  {
> > > >  	struct resource res[] = {
> > > >  		{
> > > > diff --git a/arch/arm/mach-mxs/include/mach/devices-common.h b/arch/arm/mach-mxs/include/mach/devices-common.h
> > > > index fca0551..d206e0a 100644
> > > > --- a/arch/arm/mach-mxs/include/mach/devices-common.h
> > > > +++ b/arch/arm/mach-mxs/include/mach/devices-common.h
> > > > @@ -38,4 +38,4 @@ struct mxs_fec_data {
> > > >  };
> > > >  struct platform_device *__init mxs_add_fec(
> > > >  		const struct mxs_fec_data *data,
> > > > -		const struct fec_platform_data *pdata);
> > > > +		struct fec_platform_data *pdata);
> > > Why that?
> > > 
> > pdata->mac needs to get assigned by ocotp read function.
> ... which is done before mxs_add_fec is called.
> 
My bad. Will add it back.

-- 
Regards,
Shawn

[PATCH 10/10] ARM: mxs: add initial pm support

[Shawn Guo]

This is a very initial pm support and basically does nothing.
With this pm support entry, drivers can start testing their own
pm functions.

Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
---
 arch/arm/mach-mxs/Makefile |    2 +-
 arch/arm/mach-mxs/pm.c     |   44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/mach-mxs/pm.c

diff --git a/arch/arm/mach-mxs/Makefile b/arch/arm/mach-mxs/Makefile
index f23ebbd..020547d 100644
--- a/arch/arm/mach-mxs/Makefile
+++ b/arch/arm/mach-mxs/Makefile
@@ -1,5 +1,5 @@
 # Common support
-obj-y := clock.o devices.o gpio.o icoll.o iomux.o ocotp.o system.o timer.o
+obj-y := clock.o devices.o gpio.o icoll.o iomux.o ocotp.o pm.o system.o timer.o
 
 obj-$(CONFIG_SOC_IMX23) += clock-mx23.o mm-mx23.o
 obj-$(CONFIG_SOC_IMX28) += clock-mx28.o mm-mx28.o
diff --git a/arch/arm/mach-mxs/pm.c b/arch/arm/mach-mxs/pm.c
new file mode 100644
index 0000000..e4f4e31
--- /dev/null
+++ b/arch/arm/mach-mxs/pm.c
@@ -0,0 +1,44 @@
+/*
+ * Copyright (C) 2010 Freescale Semiconductor, Inc.
+ *
+ * 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.
+ */
+
+#include <linux/kernel.h>
+#include <linux/suspend.h>
+#include <linux/io.h>
+#include <mach/system.h>
+
+static int mxs_suspend_enter(suspend_state_t state)
+{
+	switch (state) {
+	case PM_SUSPEND_MEM:
+		arch_idle();
+		break;
+
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static struct platform_suspend_ops mxs_suspend_ops = {
+	.enter = mxs_suspend_enter,
+	.valid = suspend_valid_only_mem,
+};
+
+static int __init mxs_pm_init(void)
+{
+	suspend_set_ops(&mxs_suspend_ops);
+	return 0;
+}
+
+device_initcall(mxs_pm_init);
-- 
1.7.1

[PATCH 10/10] ARM: mxs: add initial pm support

[Uwe Kleine-König]

Hello Shawn,

On Tue, Dec 28, 2010 at 10:55:55PM +0800, Shawn Guo wrote:
> This is a very initial pm support and basically does nothing.
> With this pm support entry, drivers can start testing their own
> pm functions.
> 
> Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> ---
>  arch/arm/mach-mxs/Makefile |    2 +-
>  arch/arm/mach-mxs/pm.c     |   44 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+), 1 deletions(-)
>  create mode 100644 arch/arm/mach-mxs/pm.c
> 
> diff --git a/arch/arm/mach-mxs/Makefile b/arch/arm/mach-mxs/Makefile
> index f23ebbd..020547d 100644
> --- a/arch/arm/mach-mxs/Makefile
> +++ b/arch/arm/mach-mxs/Makefile
> @@ -1,5 +1,5 @@
>  # Common support
> -obj-y := clock.o devices.o gpio.o icoll.o iomux.o ocotp.o system.o timer.o
> +obj-y := clock.o devices.o gpio.o icoll.o iomux.o ocotp.o pm.o system.o timer.o
I don't know what is usual here, but I guess doing

	obj-$(CONFIG_PM) += pm.o

is better.  (Or CONFIG_SUSPEND?)

>  obj-$(CONFIG_SOC_IMX23) += clock-mx23.o mm-mx23.o
>  obj-$(CONFIG_SOC_IMX28) += clock-mx28.o mm-mx28.o
> diff --git a/arch/arm/mach-mxs/pm.c b/arch/arm/mach-mxs/pm.c
> new file mode 100644
> index 0000000..e4f4e31
> --- /dev/null
> +++ b/arch/arm/mach-mxs/pm.c
> @@ -0,0 +1,44 @@
> +/*
> + * Copyright (C) 2010 Freescale Semiconductor, Inc.
> + *
> + * 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.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/suspend.h>
> +#include <linux/io.h>
> +#include <mach/system.h>
> +
> +static int mxs_suspend_enter(suspend_state_t state)
> +{
> +	switch (state) {
> +	case PM_SUSPEND_MEM:
> +		arch_idle();
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static struct platform_suspend_ops mxs_suspend_ops = {
> +	.enter = mxs_suspend_enter,
> +	.valid = suspend_valid_only_mem,
> +};
> +
> +static int __init mxs_pm_init(void)
> +{
> +	suspend_set_ops(&mxs_suspend_ops);
> +	return 0;
> +}
> +
can you please remove this empty line?

> +device_initcall(mxs_pm_init);
> -- 
> 1.7.1
> 
> 
> 

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 10/10] ARM: mxs: add initial pm support

[Shawn Guo]

Hi Uwe,

On Wed, Dec 29, 2010 at 11:57:09AM +0100, Uwe Kleine-K?nig wrote:
> Hello Shawn,
> 
> On Tue, Dec 28, 2010 at 10:55:55PM +0800, Shawn Guo wrote:
> > This is a very initial pm support and basically does nothing.
> > With this pm support entry, drivers can start testing their own
> > pm functions.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> > ---
> >  arch/arm/mach-mxs/Makefile |    2 +-
> >  arch/arm/mach-mxs/pm.c     |   44 ++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 45 insertions(+), 1 deletions(-)
> >  create mode 100644 arch/arm/mach-mxs/pm.c
> > 
> > diff --git a/arch/arm/mach-mxs/Makefile b/arch/arm/mach-mxs/Makefile
> > index f23ebbd..020547d 100644
> > --- a/arch/arm/mach-mxs/Makefile
> > +++ b/arch/arm/mach-mxs/Makefile
> > @@ -1,5 +1,5 @@
> >  # Common support
> > -obj-y := clock.o devices.o gpio.o icoll.o iomux.o ocotp.o system.o timer.o
> > +obj-y := clock.o devices.o gpio.o icoll.o iomux.o ocotp.o pm.o system.o timer.o
> I don't know what is usual here, but I guess doing
> 
> 	obj-$(CONFIG_PM) += pm.o
> 
> is better.  (Or CONFIG_SUSPEND?)
> 
OK.

> >  obj-$(CONFIG_SOC_IMX23) += clock-mx23.o mm-mx23.o
> >  obj-$(CONFIG_SOC_IMX28) += clock-mx28.o mm-mx28.o
> > diff --git a/arch/arm/mach-mxs/pm.c b/arch/arm/mach-mxs/pm.c
> > new file mode 100644
> > index 0000000..e4f4e31
> > --- /dev/null
> > +++ b/arch/arm/mach-mxs/pm.c
> > @@ -0,0 +1,44 @@
> > +/*
> > + * Copyright (C) 2010 Freescale Semiconductor, Inc.
> > + *
> > + * 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.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/suspend.h>
> > +#include <linux/io.h>
> > +#include <mach/system.h>
> > +
> > +static int mxs_suspend_enter(suspend_state_t state)
> > +{
> > +	switch (state) {
> > +	case PM_SUSPEND_MEM:
> > +		arch_idle();
> > +		break;
> > +
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static struct platform_suspend_ops mxs_suspend_ops = {
> > +	.enter = mxs_suspend_enter,
> > +	.valid = suspend_valid_only_mem,
> > +};
> > +
> > +static int __init mxs_pm_init(void)
> > +{
> > +	suspend_set_ops(&mxs_suspend_ops);
> > +	return 0;
> > +}
> > +
> can you please remove this empty line?
> 
OK.

> > +device_initcall(mxs_pm_init);
> > -- 
> > 1.7.1
> > 
> > 
> > 
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-K?nig            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> 

-- 
Regards,
Shawn

[PATCH 10/10] ARM: mxs: add initial pm support

[Shawn Guo]

Hi Uwe,

On Wed, Dec 29, 2010 at 11:57:09AM +0100, Uwe Kleine-K?nig wrote:
> Hello Shawn,
> 
> On Tue, Dec 28, 2010 at 10:55:55PM +0800, Shawn Guo wrote:
> > This is a very initial pm support and basically does nothing.
> > With this pm support entry, drivers can start testing their own
> > pm functions.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> > ---
> >  arch/arm/mach-mxs/Makefile |    2 +-
> >  arch/arm/mach-mxs/pm.c     |   44 ++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 45 insertions(+), 1 deletions(-)
> >  create mode 100644 arch/arm/mach-mxs/pm.c
> > 
> > diff --git a/arch/arm/mach-mxs/Makefile b/arch/arm/mach-mxs/Makefile
> > index f23ebbd..020547d 100644
> > --- a/arch/arm/mach-mxs/Makefile
> > +++ b/arch/arm/mach-mxs/Makefile
> > @@ -1,5 +1,5 @@
> >  # Common support
> > -obj-y := clock.o devices.o gpio.o icoll.o iomux.o ocotp.o system.o timer.o
> > +obj-y := clock.o devices.o gpio.o icoll.o iomux.o ocotp.o pm.o system.o timer.o
> I don't know what is usual here, but I guess doing
> 
> 	obj-$(CONFIG_PM) += pm.o
> 
Will do.  Thanks.

> is better.  (Or CONFIG_SUSPEND?)
> 
> >  obj-$(CONFIG_SOC_IMX23) += clock-mx23.o mm-mx23.o
> >  obj-$(CONFIG_SOC_IMX28) += clock-mx28.o mm-mx28.o
> > diff --git a/arch/arm/mach-mxs/pm.c b/arch/arm/mach-mxs/pm.c
> > new file mode 100644
> > index 0000000..e4f4e31
> > --- /dev/null
> > +++ b/arch/arm/mach-mxs/pm.c
> > @@ -0,0 +1,44 @@
> > +/*
> > + * Copyright (C) 2010 Freescale Semiconductor, Inc.
> > + *
> > + * 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.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/suspend.h>
> > +#include <linux/io.h>
> > +#include <mach/system.h>
> > +
> > +static int mxs_suspend_enter(suspend_state_t state)
> > +{
> > +	switch (state) {
> > +	case PM_SUSPEND_MEM:
> > +		arch_idle();
> > +		break;
> > +
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static struct platform_suspend_ops mxs_suspend_ops = {
> > +	.enter = mxs_suspend_enter,
> > +	.valid = suspend_valid_only_mem,
> > +};
> > +
> > +static int __init mxs_pm_init(void)
> > +{
> > +	suspend_set_ops(&mxs_suspend_ops);
> > +	return 0;
> > +}
> > +
> can you please remove this empty line?
> 
OK.

-- 
Regards,
Shawn

[PATCH 00/10] net/fec: add dual fec support for i.MX28

[Greg Ungerer]

Hi Shawn,

On 29/12/10 00:55, Shawn Guo wrote:
> This patch series is to add dual fec support for mx28, which is
> a mxs-based soc. Some code changes related to the following commits
> are also made in this patch set for some reasons.
>
>    e6b043d512fa8d9a3801bf5d72bfa3b8fc3b3cc8
>    netdev/fec.c: add phylib supporting to enable carrier detection (v2)
>
>    e3fe8558c7fc182972c3d947d88744482111f304
>    net/fec: fix pm to survive to suspend/resume
>
> Also I do not understand one line change made in commit below.
>
>    commit 6fcc040f02d281c7e9563127358a77ce2bbfe284
>    net: allow FEC driver to use fixed PHY support
>
> -	snprintf(fep->mii_bus->id, MII_BUS_ID_SIZE, "%x", pdev->id);
> +	snprintf(fep->mii_bus->id, MII_BUS_ID_SIZE, "%x", pdev->id + 1);
>
> Can someone help me understand if it's really needed?  The patch
> set can work with this change, and I'm just curious.

It is needed, at least in my case. I can't recall exactly why
right at the moment though. It was either that 0 didn't work with
the fixed phy setup, or it there ended up being a a duplicate
id. I'll need to check on that board/setup again.

Regards
Greg


> It's been tested on mx28 evk and mx51 babbage. For mx28, it has
> to work against the following patch set, which has not got
> merged yet.
>
>   [PATCH v8 00/15] ARM: mxs: Add initial support for MX23 and MX28
>
> Thanks for review.
>
> Regards,
> Shawn
>
> Shawn Guo (10):
>   arch/arm/mach-mxs/Makefile                      |    2 +-
>   arch/arm/mach-mxs/clock-mx28.c                  |    7 +-
>   arch/arm/mach-mxs/devices/platform-fec.c        |    2 +-
>   arch/arm/mach-mxs/include/mach/common.h         |    1 +
>   arch/arm/mach-mxs/include/mach/devices-common.h |    2 +-
>   arch/arm/mach-mxs/mach-mx28evk.c                |   59 ++++++-
>   arch/arm/mach-mxs/ocotp.c                       |   52 ++++++
>   arch/arm/mach-mxs/pm.c                          |   44 +++++
>   drivers/net/Kconfig                             |    7 +-
>   drivers/net/fec.c                               |  220 +++++++++++++++++------
>   drivers/net/fec.h                               |    5 +-
>   include/linux/fec.h                             |    2 +
>   12 files changed, 335 insertions(+), 68 deletions(-)
>
>   [PATCH 01/10] net/fec: fix MMFR_OP type in fec_enet_mdio_write
>   [PATCH 02/10] net/fec: remove the use of "index" which is legacy
>   [PATCH 03/10] net/fec: add mac field into platform data and consolidate fec_get_mac
>   [PATCH 04/10] net/fec: improve pm for better suspend/resume
>   [PATCH 05/10] net/fec: add dual fec support for mx28
>   [PATCH 06/10] ARM: mx28: update clocks for dual fec support
>   [PATCH 07/10] ARM: mx28: add the second fec device registration
>   [PATCH 08/10] ARM: mxs: add ocotp read function
>   [PATCH 09/10] ARM: mx28: read fec mac address from ocotp
>   [PATCH 10/10] ARM: mxs: add initial pm support
>
>


-- 
------------------------------------------------------------------------
Greg Ungerer  --  Principal Engineer        EMAIL:     gerg at snapgear.com
SnapGear Group, McAfee                      PHONE:       +61 7 3435 2888
8 Gardner Close                             FAX:         +61 7 3217 5323
Milton, QLD, 4064, Australia                WEB: http://www.SnapGear.com