[U-Boot] [PATCH v1 2/3] net: macb: Add the clock support

[Joe Hershberger]

On Wed, Oct 19, 2016 at 9:55 PM, Wenyou Yang <wenyou.yang at atmel.com> wrote:
> Due to introducing the at91 clock driver, add the clock support.
>
> Signed-off-by: Wenyou Yang <wenyou.yang at atmel.com>
> ---
>
>  drivers/net/macb.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 67 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/macb.c b/drivers/net/macb.c
> index 01527f7..0628ff5 100644
> --- a/drivers/net/macb.c
> +++ b/drivers/net/macb.c
> @@ -4,6 +4,7 @@
>   * SPDX-License-Identifier:    GPL-2.0+
>   */
>  #include <common.h>
> +#include <clk.h>
>  #include <dm.h>
>
>  /*
> @@ -112,6 +113,7 @@ struct macb_device {
>         struct mii_dev          *bus;
>
>  #ifdef CONFIG_DM_ETH
> +       unsigned long           pclk_rate;
>         phy_interface_t         phy_interface;
>  #endif
>  };
> @@ -751,10 +753,18 @@ static int _macb_write_hwaddr(struct macb_device *macb, unsigned char *enetaddr)
>         return 0;
>  }
>
> +#ifndef CONFIG_DM_ETH

Please use positive logic throughout. If you have an #else, always use #ifdef.

Please fix throughout.

>  static u32 macb_mdc_clk_div(int id, struct macb_device *macb)

What's the benefit of splitting the signature of this function instead
of just passing in the result of get_macb_pclk_rate() in the !DM case?

> +#else
> +static u32 macb_mdc_clk_div(unsigned long pck_rate)
> +#endif
>  {
>         u32 config;
> +#ifndef CONFIG_DM_ETH
>         unsigned long macb_hz = get_macb_pclk_rate(id);
> +#else
> +       unsigned long macb_hz = pck_rate;
> +#endif
>
>         if (macb_hz < 20000000)
>                 config = MACB_BF(CLK, MACB_CLK_DIV8);
> @@ -768,10 +778,18 @@ static u32 macb_mdc_clk_div(int id, struct macb_device *macb)
>         return config;
>  }
>
> +#ifndef CONFIG_DM_ETH
>  static u32 gem_mdc_clk_div(int id, struct macb_device *macb)
> +#else
> +static u32 gem_mdc_clk_div(unsigned long pck_rate)
> +#endif
>  {
>         u32 config;
> +#ifndef CONFIG_DM_ETH
>         unsigned long macb_hz = get_macb_pclk_rate(id);
> +#else
> +       unsigned long macb_hz = pck_rate;
> +#endif
>
>         if (macb_hz < 20000000)
>                 config = GEM_BF(CLK, GEM_CLK_DIV8);
> @@ -807,9 +825,19 @@ static u32 macb_dbw(struct macb_device *macb)
>         }
>  }
>
> +#ifndef CONFIG_DM_ETH
>  static void _macb_eth_initialize(struct macb_device *macb)
> +#else
> +static void _macb_eth_initialize(struct udevice *dev)

It appears you only use the macb pointer in this function, so why pass
in the dev only to have to look up the macb priv ptr? Just always pass
the macb ptr.

> +#endif
>  {
> +#ifdef CONFIG_DM_ETH
> +       struct macb_device *macb = dev_get_priv(dev);
> +#endif
> +
> +#ifndef CONFIG_DM_ETH
>         int id = 0;     /* This is not used by functions we call */

You could just hard-code the 0 (with comment) once in the call to
get_macb_pclk_rate() and avoid this ifdef.

> +#endif
>         u32 ncfgr;
>
>         /* TODO: we need check the rx/tx_ring_dma is dcache line aligned */
> @@ -827,10 +855,18 @@ static void _macb_eth_initialize(struct macb_device *macb)
>          * to the PHY
>          */
>         if (macb_is_gem(macb)) {
> +#ifndef CONFIG_DM_ETH
>                 ncfgr = gem_mdc_clk_div(id, macb);
> +#else
> +               ncfgr = gem_mdc_clk_div(macb->pclk_rate);
> +#endif
>                 ncfgr |= macb_dbw(macb);
>         } else {
> +#ifndef CONFIG_DM_ETH
>                 ncfgr = macb_mdc_clk_div(id, macb);
> +#else
> +               ncfgr = macb_mdc_clk_div(macb->pclk_rate);
> +#endif
>         }
>
>         macb_writel(macb, NCFGR, ncfgr);
> @@ -991,6 +1027,30 @@ static const struct eth_ops macb_eth_ops = {
>         .write_hwaddr   = macb_write_hwaddr,
>  };
>
> +static int macb_enable_clk(struct udevice *dev)
> +{
> +       struct macb_device *macb = dev_get_priv(dev);
> +       struct clk clk;
> +       ulong clk_rate;
> +       int ret;
> +
> +       ret = clk_get_by_index(dev, 0, &clk);
> +       if (ret)
> +               return -EINVAL;
> +
> +       ret = clk_enable(&clk);
> +       if (ret)
> +               return ret;
> +
> +       clk_rate = clk_get_rate(&clk);
> +       if (!clk_rate)
> +               return -EINVAL;
> +
> +       macb->pclk_rate = clk_rate;
> +
> +       return 0;
> +}
> +
>  static int macb_eth_probe(struct udevice *dev)
>  {
>         struct eth_pdata *pdata = dev_get_platdata(dev);
> @@ -998,6 +1058,7 @@ static int macb_eth_probe(struct udevice *dev)
>
>  #ifdef CONFIG_DM_ETH
>         const char *phy_mode;
> +       int ret;
>
>         phy_mode = fdt_getprop(gd->fdt_blob, dev->of_offset, "phy-mode", NULL);
>         if (phy_mode)
> @@ -1010,7 +1071,12 @@ static int macb_eth_probe(struct udevice *dev)
>
>         macb->regs = (void *)pdata->iobase;
>
> -       _macb_eth_initialize(macb);
> +       ret = macb_enable_clk(dev);
> +       if (ret)
> +               return ret;
> +
> +       _macb_eth_initialize(dev);
> +
>  #if defined(CONFIG_CMD_MII) || defined(CONFIG_PHYLIB)
>         int retval;
>         struct mii_dev *mdiodev = mdio_alloc();
> --
> 2.7.4
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot