Message ID | 1299863585-17263-1-git-send-email-plagnioj@jcrosoft.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 */
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(-)