* [PATCH] macb: detect IP version to determin if we are on at91 or avr32 @ 2011-03-11 17:13 Jean-Christophe PLAGNIOL-VILLARD 2011-03-14 10:15 ` Russell King - ARM Linux 2011-03-14 10:39 ` Jamie Iles 0 siblings, 2 replies; 4+ messages in thread From: Jean-Christophe PLAGNIOL-VILLARD @ 2011-03-11 17:13 UTC (permalink / raw) To: linux-arm-kernel Cc: netdev, linux-kernel, Jean-Christophe PLAGNIOL-VILLARD, Hans-Christian Egtvedt, Nicolas Ferre, Jamie Iles 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] macb: detect IP version to determin if we are on at91 or avr32 2011-03-11 17:13 [PATCH] macb: detect IP version to determin if we are on at91 or avr32 Jean-Christophe PLAGNIOL-VILLARD @ 2011-03-14 10:15 ` Russell King - ARM Linux 2011-03-14 17:24 ` Jean-Christophe PLAGNIOL-VILLARD 2011-03-14 10:39 ` Jamie Iles 1 sibling, 1 reply; 4+ messages in thread From: Russell King - ARM Linux @ 2011-03-14 10:15 UTC (permalink / raw) To: Jean-Christophe PLAGNIOL-VILLARD Cc: linux-arm-kernel, netdev, Nicolas Ferre, linux-kernel, Jamie Iles, Hans-Christian Egtvedt 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] macb: detect IP version to determin if we are on at91 or avr32 2011-03-14 10:15 ` Russell King - ARM Linux @ 2011-03-14 17:24 ` Jean-Christophe PLAGNIOL-VILLARD 0 siblings, 0 replies; 4+ messages in thread From: Jean-Christophe PLAGNIOL-VILLARD @ 2011-03-14 17:24 UTC (permalink / raw) To: Russell King - ARM Linux Cc: netdev, Nicolas Ferre, linux-kernel, Jamie Iles, Hans-Christian Egtvedt, linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] macb: detect IP version to determin if we are on at91 or avr32 2011-03-11 17:13 [PATCH] macb: detect IP version to determin if we are on at91 or avr32 Jean-Christophe PLAGNIOL-VILLARD 2011-03-14 10:15 ` Russell King - ARM Linux @ 2011-03-14 10:39 ` Jamie Iles 1 sibling, 0 replies; 4+ messages in thread From: Jamie Iles @ 2011-03-14 10:39 UTC (permalink / raw) To: Jean-Christophe PLAGNIOL-VILLARD Cc: linux-arm-kernel, netdev, linux-kernel, Hans-Christian Egtvedt, Nicolas Ferre, 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-03-14 17:24 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-03-11 17:13 [PATCH] macb: detect IP version to determin if we are on at91 or avr32 Jean-Christophe PLAGNIOL-VILLARD 2011-03-14 10:15 ` Russell King - ARM Linux 2011-03-14 17:24 ` Jean-Christophe PLAGNIOL-VILLARD 2011-03-14 10:39 ` Jamie Iles
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).