this will make macb soc generic and will allow to use it on other arch
Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <[email protected]>
Cc: Hans-Christian Egtvedt <[email protected]>
Cc: Nicolas Ferre <[email protected]>
Cc: Jamie Iles <[email protected]>
---
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
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.
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 <[email protected]>
> Cc: Hans-Christian Egtvedt <[email protected]>
> Cc: Nicolas Ferre <[email protected]>
> Cc: Jamie Iles <[email protected]>
> ---
[...]
> 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
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.