[PATCH] macb: detect IP version to determin if we are on at91 or avr32

[PATCH] macb: detect IP version to determin if we are on at91 or avr32

[Jean-Christophe PLAGNIOL-VILLARD]

this will make macb soc generic and will allow to use it on other arch

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: Hans-Christian Egtvedt <hans-christian.egtvedt@atmel.com>
Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
Cc: Jamie Iles <jamie@jamieiles.com>
---
 drivers/net/macb.c |   96 +++++++++++++++++++++++++---------------------------
 drivers/net/macb.h |    9 +++++
 2 files changed, 55 insertions(+), 50 deletions(-)

diff --git a/drivers/net/macb.c b/drivers/net/macb.c
index f251866..58cebf2 100644
--- a/drivers/net/macb.c
+++ b/drivers/net/macb.c
@@ -22,7 +22,6 @@
 #include <linux/phy.h>
 
 #include <mach/board.h>
-#include <mach/cpu.h>
 
 #include "macb.h"
 
@@ -1140,28 +1139,30 @@ static int __init macb_probe(struct platform_device *pdev)
 
 	spin_lock_init(&bp->lock);
 
-#if defined(CONFIG_ARCH_AT91)
-	bp->pclk = clk_get(&pdev->dev, "macb_clk");
-	if (IS_ERR(bp->pclk)) {
-		dev_err(&pdev->dev, "failed to get macb_clk\n");
-		goto err_out_free_dev;
-	}
-	clk_enable(bp->pclk);
-#else
-	bp->pclk = clk_get(&pdev->dev, "pclk");
-	if (IS_ERR(bp->pclk)) {
-		dev_err(&pdev->dev, "failed to get pclk\n");
-		goto err_out_free_dev;
-	}
-	bp->hclk = clk_get(&pdev->dev, "hclk");
-	if (IS_ERR(bp->hclk)) {
-		dev_err(&pdev->dev, "failed to get hclk\n");
-		goto err_out_put_pclk;
-	}
+	bp->version = macb_readl(bp, VERSION);
 
-	clk_enable(bp->pclk);
-	clk_enable(bp->hclk);
-#endif
+	if (macb_is_at91(bp)) {
+		bp->pclk = clk_get(&pdev->dev, "macb_clk");
+		if (IS_ERR(bp->pclk)) {
+			dev_err(&pdev->dev, "failed to get macb_clk\n");
+			goto err_out_free_dev;
+		}
+		clk_enable(bp->pclk);
+	} else {
+		bp->pclk = clk_get(&pdev->dev, "pclk");
+		if (IS_ERR(bp->pclk)) {
+			dev_err(&pdev->dev, "failed to get pclk\n");
+			goto err_out_free_dev;
+		}
+		bp->hclk = clk_get(&pdev->dev, "hclk");
+		if (IS_ERR(bp->hclk)) {
+			dev_err(&pdev->dev, "failed to get hclk\n");
+			goto err_out_put_pclk;
+		}
+
+		clk_enable(bp->pclk);
+		clk_enable(bp->hclk);
+	}
 
 	bp->regs = ioremap(regs->start, regs->end - regs->start + 1);
 	if (!bp->regs) {
@@ -1191,18 +1192,17 @@ static int __init macb_probe(struct platform_device *pdev)
 	macb_get_hwaddr(bp);
 	pdata = pdev->dev.platform_data;
 
-	if (pdata && pdata->is_rmii)
-#if defined(CONFIG_ARCH_AT91)
-		macb_writel(bp, USRIO, (MACB_BIT(RMII) | MACB_BIT(CLKEN)) );
-#else
-		macb_writel(bp, USRIO, 0);
-#endif
-	else
-#if defined(CONFIG_ARCH_AT91)
-		macb_writel(bp, USRIO, MACB_BIT(CLKEN));
-#else
-		macb_writel(bp, USRIO, MACB_BIT(MII));
-#endif
+	if (pdata && pdata->is_rmii) {
+		if (macb_is_at91(bp))
+			macb_writel(bp, USRIO, (MACB_BIT(RMII) | MACB_BIT(CLKEN)));
+		else
+			macb_writel(bp, USRIO, 0);
+	} else {
+		if (macb_is_at91(bp))
+			macb_writel(bp, USRIO, MACB_BIT(CLKEN));
+		else
+			macb_writel(bp, USRIO, MACB_BIT(MII));
+	}
 
 	bp->tx_pending = DEF_TX_RING_PENDING;
 
@@ -1245,14 +1245,12 @@ err_out_unregister_netdev:
 err_out_iounmap:
 	iounmap(bp->regs);
 err_out_disable_clocks:
-#ifndef CONFIG_ARCH_AT91
-	clk_disable(bp->hclk);
-	clk_put(bp->hclk);
-#endif
+	if (!macb_is_at91(bp)) {
+		clk_disable(bp->hclk);
+		clk_put(bp->hclk);
+	}
 	clk_disable(bp->pclk);
-#ifndef CONFIG_ARCH_AT91
 err_out_put_pclk:
-#endif
 	clk_put(bp->pclk);
 err_out_free_dev:
 	free_netdev(dev);
@@ -1278,10 +1276,10 @@ static int __exit macb_remove(struct platform_device *pdev)
 		unregister_netdev(dev);
 		free_irq(dev->irq, dev);
 		iounmap(bp->regs);
-#ifndef CONFIG_ARCH_AT91
-		clk_disable(bp->hclk);
-		clk_put(bp->hclk);
-#endif
+		if (!macb_is_at91(bp)) {
+			clk_disable(bp->hclk);
+			clk_put(bp->hclk);
+		}
 		clk_disable(bp->pclk);
 		clk_put(bp->pclk);
 		free_netdev(dev);
@@ -1299,9 +1297,8 @@ static int macb_suspend(struct platform_device *pdev, pm_message_t state)
 
 	netif_device_detach(netdev);
 
-#ifndef CONFIG_ARCH_AT91
-	clk_disable(bp->hclk);
-#endif
+	if (!macb_is_at91(bp))
+		clk_disable(bp->hclk);
 	clk_disable(bp->pclk);
 
 	return 0;
@@ -1313,9 +1310,8 @@ static int macb_resume(struct platform_device *pdev)
 	struct macb *bp = netdev_priv(netdev);
 
 	clk_enable(bp->pclk);
-#ifndef CONFIG_ARCH_AT91
-	clk_enable(bp->hclk);
-#endif
+	if (!macb_is_at91(bp))
+		clk_enable(bp->hclk);
 
 	netif_device_attach(netdev);
 
diff --git a/drivers/net/macb.h b/drivers/net/macb.h
index d3212f6..56a4fcb 100644
--- a/drivers/net/macb.h
+++ b/drivers/net/macb.h
@@ -59,6 +59,7 @@
 #define MACB_TPQ				0x00bc
 #define MACB_USRIO				0x00c0
 #define MACB_WOL				0x00c4
+#define MACB_VERSION				0x00fc
 
 /* Bitfields in NCR */
 #define MACB_LB_OFFSET				0
@@ -389,6 +390,14 @@ struct macb {
 	unsigned int 		link;
 	unsigned int 		speed;
 	unsigned int 		duplex;
+
+	uint32_t		version;
 };
 
+#define MACB_VERSION_MASK	0xffff0000
+#define macb_is_at91(bp)	\
+	(((bp)->version & MACB_VERSION_MASK) == 0x06010000)
+#define macb_is_avr32(bp)	\
+	(((bp)->version & MACB_VERSION_MASK) == 0x00010000)
+
 #endif /* _MACB_H */
-- 
1.7.4.1

Re: [PATCH] macb: detect IP version to determin if we are on at91 or avr32

[Russell King - ARM Linux]

On Fri, Mar 11, 2011 at 06:13:05PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> +	if (macb_is_at91(bp)) {
> +		bp->pclk = clk_get(&pdev->dev, "macb_clk");
> +		if (IS_ERR(bp->pclk)) {
> +			dev_err(&pdev->dev, "failed to get macb_clk\n");
> +			goto err_out_free_dev;
> +		}
> +		clk_enable(bp->pclk);
> +	} else {
> +		bp->pclk = clk_get(&pdev->dev, "pclk");
> +		if (IS_ERR(bp->pclk)) {
> +			dev_err(&pdev->dev, "failed to get pclk\n");
> +			goto err_out_free_dev;
> +		}
> +		bp->hclk = clk_get(&pdev->dev, "hclk");
> +		if (IS_ERR(bp->hclk)) {
> +			dev_err(&pdev->dev, "failed to get hclk\n");
> +			goto err_out_put_pclk;
> +		}
> +
> +		clk_enable(bp->pclk);
> +		clk_enable(bp->hclk);
> +	}

This is the same kind of sillyness that started getting OMAP into problems
with the clk API.  Just do this instead:

	bp->pclk = clk_get(&pdev->dev, "pclk");
	if (IS_ERR(bp->pclk)) {
		dev_err(&pdev->dev, "failed to get pclk\n");
		goto err_out_free_dev;
	}
	bp->hclk = clk_get(&pdev->dev, "hclk");
	if (IS_ERR(bp->hclk)) {
		dev_err(&pdev->dev, "failed to get hclk\n");
		goto err_out_put_pclk;
	}

	clk_enable(bp->pclk);
	clk_enable(bp->hclk);

And then require _all_ platforms using this driver to provide a pclk and
a hclk for this device, whether they exist in the SoC or not.  Where they
don't, provide dummy clocks for it.

This probably means you end up with _less_ bloat overall because you're
not having to build the above code.  You've less lines of source code to
maintain.  You have a simplified dirver with consistent requirements
across all platforms.  You don't need to read the version register, and
you don't need macb_is_at91() and macb_is_avr32().

With clkdev it's _cheap_ to provide these dummy clocks once you have one
dummy clock already in place.

Re: [PATCH] macb: detect IP version to determin if we are on at91 or avr32

[Jamie Iles]

Hi,

I agree with Russell's comments about providing a fake hclk to remove 
the conditional clock stuff.  I was planning on posting the patch below 
in my next spin of the gem support patches which also renames macb_clk 
to pclk to be consistent with avr32 and enables us to remove all of the 
conditional clock stuff.

On Fri, Mar 11, 2011 at 06:13:05PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> this will make macb soc generic and will allow to use it on other arch
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> Cc: Hans-Christian Egtvedt <hans-christian.egtvedt@atmel.com>
> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
> Cc: Jamie Iles <jamie@jamieiles.com>
> ---
[...]
> diff --git a/drivers/net/macb.h b/drivers/net/macb.h
> index d3212f6..56a4fcb 100644
> --- a/drivers/net/macb.h
> +++ b/drivers/net/macb.h
> @@ -59,6 +59,7 @@
>  #define MACB_TPQ				0x00bc
>  #define MACB_USRIO				0x00c0
>  #define MACB_WOL				0x00c4
> +#define MACB_VERSION				0x00fc
>  
>  /* Bitfields in NCR */
>  #define MACB_LB_OFFSET				0
> @@ -389,6 +390,14 @@ struct macb {
>  	unsigned int 		link;
>  	unsigned int 		speed;
>  	unsigned int 		duplex;
> +
> +	uint32_t		version;

Minor nitpick, this should be u32 to be consistent with the rest of the 
fields.

>  };
>  
> +#define MACB_VERSION_MASK	0xffff0000
> +#define macb_is_at91(bp)	\
> +	(((bp)->version & MACB_VERSION_MASK) == 0x06010000)
> +#define macb_is_avr32(bp)	\
> +	(((bp)->version & MACB_VERSION_MASK) == 0x00010000)
> +
>  #endif /* _MACB_H */

I can't convince myself that this is safe and correct.  From the Cadence 
GEM spec:

31:16 Module identification number - for the GEM, this
      value is fixed at 0x0002.
15:0 Module revision - fixed byte value specific to the
     revision of the design which is incremented after
     each release of the IP.

I don't have access to the MACB spec, but it seems that 31:16 are 
defined by Cadence so there could well be other chips with a module ID 
of 0x0001 that aren't avr32.

We can certainly use this to tell us if we are GEM/MACB but if we need 
to know if we are AT91/AVR32 for the RMII/MII USRIO stuff, then couldn't 
we just keep the #ifdef CONFIG_ARCH_AT91 and factor that out into a 
macb_set_usrio() type method?

Jamie

8<---------

diff --git a/arch/arm/mach-at91/at572d940hf.c b/arch/arm/mach-at91/at572d940hf.c
index a6b9c68..9b3a37e 100644
--- a/arch/arm/mach-at91/at572d940hf.c
+++ b/arch/arm/mach-at91/at572d940hf.c
@@ -71,10 +71,15 @@ static struct clk pioC_clk = {
 	.type		= CLK_TYPE_PERIPHERAL,
 };
 static struct clk macb_clk = {
-	.name		= "macb_clk",
+	.name		= "pclk",
 	.pmc_mask	= 1 << AT572D940HF_ID_EMAC,
 	.type		= CLK_TYPE_PERIPHERAL,
 };
+static struct clk macb_hclk = {
+	.name		= "hclk",
+	.pmc_mask	= 0,
+	.type		= CLK_TYPE_PERIPHERAL,
+};
 static struct clk usart0_clk = {
 	.name		= "usart0_clk",
 	.pmc_mask	= 1 << AT572D940HF_ID_US0,
@@ -182,6 +187,7 @@ static struct clk *periph_clocks[] __initdata = {
 	&pioB_clk,
 	&pioC_clk,
 	&macb_clk,
+	&macb_hclk,
 	&usart0_clk,
 	&usart1_clk,
 	&usart2_clk,
diff --git a/arch/arm/mach-at91/at91cap9.c b/arch/arm/mach-at91/at91cap9.c
index 7337617..0d38ce7 100644
--- a/arch/arm/mach-at91/at91cap9.c
+++ b/arch/arm/mach-at91/at91cap9.c
@@ -150,10 +150,15 @@ static struct clk pwm_clk = {
 	.type		= CLK_TYPE_PERIPHERAL,
 };
 static struct clk macb_clk = {
-	.name		= "macb_clk",
+	.name		= "pclk",
 	.pmc_mask	= 1 << AT91CAP9_ID_EMAC,
 	.type		= CLK_TYPE_PERIPHERAL,
 };
+static struct clk macb_hclk = {
+	.name		= "hclk",
+	.pmc_mask	= 0,
+	.type		= CLK_TYPE_PERIPHERAL,
+};
 static struct clk aestdes_clk = {
 	.name		= "aestdes_clk",
 	.pmc_mask	= 1 << AT91CAP9_ID_AESTDES,
@@ -212,6 +217,7 @@ static struct clk *periph_clocks[] __initdata = {
 	&tcb_clk,
 	&pwm_clk,
 	&macb_clk,
+	&macb_hclk,
 	&aestdes_clk,
 	&adc_clk,
 	&isi_clk,
diff --git a/arch/arm/mach-at91/at91sam9260.c b/arch/arm/mach-at91/at91sam9260.c
index 195208b..f00774c 100644
--- a/arch/arm/mach-at91/at91sam9260.c
+++ b/arch/arm/mach-at91/at91sam9260.c
@@ -162,10 +162,15 @@ static struct clk ohci_clk = {
 	.type		= CLK_TYPE_PERIPHERAL,
 };
 static struct clk macb_clk = {
-	.name		= "macb_clk",
+	.name		= "pclk",
 	.pmc_mask	= 1 << AT91SAM9260_ID_EMAC,
 	.type		= CLK_TYPE_PERIPHERAL,
 };
+static struct clk macb_hclk = {
+	.name		= "hclk",
+	.pmc_mask	= 0,
+	.type		= CLK_TYPE_PERIPHERAL,
+};
 static struct clk isi_clk = {
 	.name		= "isi_clk",
 	.pmc_mask	= 1 << AT91SAM9260_ID_ISI,
@@ -221,6 +226,7 @@ static struct clk *periph_clocks[] __initdata = {
 	&tc2_clk,
 	&ohci_clk,
 	&macb_clk,
+	&macb_hclk,
 	&isi_clk,
 	&usart3_clk,
 	&usart4_clk,
diff --git a/arch/arm/mach-at91/at91sam9263.c b/arch/arm/mach-at91/at91sam9263.c
index 249f900..25cbae1 100644
--- a/arch/arm/mach-at91/at91sam9263.c
+++ b/arch/arm/mach-at91/at91sam9263.c
@@ -136,10 +136,15 @@ static struct clk pwm_clk = {
 	.type		= CLK_TYPE_PERIPHERAL,
 };
 static struct clk macb_clk = {
-	.name		= "macb_clk",
+	.name		= "pclk",
 	.pmc_mask	= 1 << AT91SAM9263_ID_EMAC,
 	.type		= CLK_TYPE_PERIPHERAL,
 };
+static struct clk macb_hclk = {
+	.name		= "hclk",
+	.pmc_mask	= 0,
+	.type		= CLK_TYPE_PERIPHERAL,
+};
 static struct clk dma_clk = {
 	.name		= "dma_clk",
 	.pmc_mask	= 1 << AT91SAM9263_ID_DMA,
@@ -190,6 +195,7 @@ static struct clk *periph_clocks[] __initdata = {
 	&tcb_clk,
 	&pwm_clk,
 	&macb_clk,
+	&macb_hclk,
 	&twodge_clk,
 	&udc_clk,
 	&isi_clk,
diff --git a/arch/arm/mach-at91/at91sam9g45.c b/arch/arm/mach-at91/at91sam9g45.c
index c67b47f..a4d4a2d 100644
--- a/arch/arm/mach-at91/at91sam9g45.c
+++ b/arch/arm/mach-at91/at91sam9g45.c
@@ -157,10 +157,15 @@ static struct clk ac97_clk = {
 	.type		= CLK_TYPE_PERIPHERAL,
 };
 static struct clk macb_clk = {
-	.name		= "macb_clk",
+	.name		= "pclk",
 	.pmc_mask	= 1 << AT91SAM9G45_ID_EMAC,
 	.type		= CLK_TYPE_PERIPHERAL,
 };
+static struct clk macb_hclk = {
+	.name		= "hclk",
+	.pmc_mask	= 0,
+	.type		= CLK_TYPE_PERIPHERAL,
+};
 static struct clk isi_clk = {
 	.name		= "isi_clk",
 	.pmc_mask	= 1 << AT91SAM9G45_ID_ISI,
@@ -224,6 +229,7 @@ static struct clk *periph_clocks[] __initdata = {
 	&lcdc_clk,
 	&ac97_clk,
 	&macb_clk,
+	&macb_hclk,
 	&isi_clk,
 	&udphs_clk,
 	&mmc1_clk,
-- 
1.7.4

Re: [PATCH] macb: detect IP version to determin if we are on at91 or avr32

[Jean-Christophe PLAGNIOL-VILLARD]

On 10:15 Mon 14 Mar     , Russell King - ARM Linux wrote:
> On Fri, Mar 11, 2011 at 06:13:05PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > +	if (macb_is_at91(bp)) {
> > +		bp->pclk = clk_get(&pdev->dev, "macb_clk");
> > +		if (IS_ERR(bp->pclk)) {
> > +			dev_err(&pdev->dev, "failed to get macb_clk\n");
> > +			goto err_out_free_dev;
> > +		}
> > +		clk_enable(bp->pclk);
> > +	} else {
> > +		bp->pclk = clk_get(&pdev->dev, "pclk");
> > +		if (IS_ERR(bp->pclk)) {
> > +			dev_err(&pdev->dev, "failed to get pclk\n");
> > +			goto err_out_free_dev;
> > +		}
> > +		bp->hclk = clk_get(&pdev->dev, "hclk");
> > +		if (IS_ERR(bp->hclk)) {
> > +			dev_err(&pdev->dev, "failed to get hclk\n");
> > +			goto err_out_put_pclk;
> > +		}
> > +
> > +		clk_enable(bp->pclk);
> > +		clk_enable(bp->hclk);
> > +	}
> 
> This is the same kind of sillyness that started getting OMAP into problems
> with the clk API.  Just do this instead:
> 
> 	bp->pclk = clk_get(&pdev->dev, "pclk");
> 	if (IS_ERR(bp->pclk)) {
> 		dev_err(&pdev->dev, "failed to get pclk\n");
> 		goto err_out_free_dev;
> 	}
> 	bp->hclk = clk_get(&pdev->dev, "hclk");
> 	if (IS_ERR(bp->hclk)) {
> 		dev_err(&pdev->dev, "failed to get hclk\n");
> 		goto err_out_put_pclk;
> 	}
> 
> 	clk_enable(bp->pclk);
> 	clk_enable(bp->hclk);
> 
> And then require _all_ platforms using this driver to provide a pclk and
> a hclk for this device, whether they exist in the SoC or not.  Where they
> don't, provide dummy clocks for it.
> 
> This probably means you end up with _less_ bloat overall because you're
> not having to build the above code.  You've less lines of source code to
> maintain.  You have a simplified dirver with consistent requirements
> across all platforms.  You don't need to read the version register, and
> you don't need macb_is_at91() and macb_is_avr32().
no we do need it for some of the register IP implementation related to the MII
at least
> 
> With clkdev it's _cheap_ to provide these dummy clocks once you have one
> dummy clock already in place.
I known and already agree about the clock, Jamie Patches will take care about it

this patch remove the ifdef ARCH and now detect the IP revision to determin
the IP specific implementation

Best Regards,
J.