Re: [PATCH 2/6] clk: Clock driver support for Broadcom Cygnus SoC

[Mark Rutland <mark.rutland@arm.com>]

On Tue, Sep 16, 2014 at 08:58:13PM +0100, Jonathan Richardson wrote:
> The iProc clock driver controls PLL's common across iProc chips. The

Nit: s/PLL's/PLLs/ (we aren't greengrocers [1]).

> cygnus driver controls cygnus specific features and variations.
> 
> Reviewed-by: Ray Jui <rjui@broadcom.com>
> Tested-by: Jonathan Richardson <jonathar@broadcom.com>
> Reviewed-by: JD (Jiandong) Zheng <jdzheng@broadcom.com>
> Signed-off-by: Jonathan Richardson <jonathar@broadcom.com>
> ---
>  drivers/clk/Makefile         |    1 +
>  drivers/clk/bcm/Makefile     |    2 +
>  drivers/clk/bcm/clk-cygnus.c | 1179 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/bcm/clk-iproc.c  |  446 ++++++++++++++++
>  4 files changed, 1628 insertions(+)
>  create mode 100644 drivers/clk/bcm/clk-cygnus.c
>  create mode 100644 drivers/clk/bcm/clk-iproc.c
 
[...]

> +/*
> + * Enable clocks controlled through the top clock gating control.
> + *
> + * @param state true = enable clock, false = disable clock
> + */
> +static void cygnus_clkgate_enable(void __iomem *clkgate_reg,
> +       enum cygnus_top_clk_gating_ctrl_offsets offset, bool state)
> +{
> +       u32 val = readl(clkgate_reg);
> +
> +       /* Enable or disable the clock. */

This function is misnamed if it does both, and 'state' is not a very
descriptive name.

> +       if (state)
> +               val |= 1 << offset;
> +       else
> +               val &= ~(1 << offset);
> +
> +       writel(val, clkgate_reg);
> +}
> +
> +/*
> + * Powers on/off the MIPI GENPLL using CRMU_PLL_AON_CTRL register.
> + *
> + * @param state true to power on PLL, false to power off
> + */
> +static void cygnus_mipi_genpll_poweron(void __iomem *pll_ctrl_reg,
> +       bool state)
> +{
> +       u32 val;
> +       u32 pll_ldo_on = ((1 << ASIU_MIPI_GENPLL_PWRON_SHIFT) |
> +               (1 << ASIU_MIPI_GENPLL_PWRON_PLL_SHIFT) |
> +               (1 << ASIU_MIPI_GENPLL_PWRON_BG_SHIFT)  |
> +               (1 << ASIU_MIPI_GENPLL_PWRON_LDO_SHIFT));
> +
> +       val = readl(pll_ctrl_reg);
> +
> +       /*
> +        * Set PLL on/off. Set input isolation mode to 1 when disabled, 0 when
> +        * enabled.
> +        */

As with cygnus_clkgate_enable, this function is misnamed and 'state' is
a confusing parameter name.

> +       if (state) {
> +               val |= pll_ldo_on;
> +               val &= ~(1 << ASIU_MIPI_GENPLL_ISO_IN_SHIFT);
> +       } else {
> +               val &= ~pll_ldo_on;
> +               val |= 1 << ASIU_MIPI_GENPLL_ISO_IN_SHIFT;
> +       }
> +
> +       writel(val, pll_ctrl_reg);
> +}
> +
> +/*
> + * Powers on/off the audio PLL using CRMU_PLL_AON_CTRL register.
> + *
> + * @param state true to power on PLL, false to power off
> + */
> +static void cygnus_audio_genpll_poweron(void __iomem *pll_ctrl_reg,
> +       bool state)
> +{
> +       u32 val;
> +       u32 pll_ldo_on = ((1 << ASIU_AUDIO_GENPLL_PWRON_PLL_SHIFT) |
> +               (1 << ASIU_AUDIO_GENPLL_PWRON_BG_SHIFT) |
> +               (1 << ASIU_AUDIO_GENPLL_PWRON_LDO_SHIFT));
> +
> +       val = readl(pll_ctrl_reg);
> +
> +       /*
> +        * Set PLL on/off. Set input isolation mode to 1 when disabled, 0 when
> +        * enabled.
> +        */

Misnamed function, confusing parameter name.

> +       if (state) {
> +               val |= pll_ldo_on;
> +               val &= ~(1 << ASIU_AUDIO_GENPLL_ISO_IN);
> +       } else {
> +               val &= ~pll_ldo_on;
> +               val |= 1 << ASIU_AUDIO_GENPLL_ISO_IN;
> +       }
> +
> +       writel(val, pll_ctrl_reg);
> +}

[...]

> +static __init struct clk *cygnus_clock_init(struct device_node *node,
> +       const struct clk_ops *ops)
> +{
> +       u32 channel = 0;
> +       struct clk *clk;
> +       struct cygnus_clk *cygnus_clk;
> +       const char *clk_name = node->name;
> +       const char *parent_name;
> +       struct clk_init_data init;
> +       int rc;
> +
> +       pr_debug("Clock name %s\n", node->name);
> +
> +       of_property_read_u32(node, "channel", &channel);
> +       cygnus_clk = kzalloc(sizeof(*cygnus_clk), GFP_KERNEL);
> +       if (WARN_ON(!cygnus_clk))
> +               return NULL;
> +
> +       cygnus_clk->state = CLK_DISABLED;
> +
> +       /* Read base address from device tree and map to virtual address. */
> +       cygnus_clk->regs_base = of_iomap(node, CYGNUS_CLK_BASE_REG);
> +       if (WARN_ON(!cygnus_clk->regs_base))
> +               goto err_alloc;
> +
> +       /* Read optional base addresses for PLL control and clock gating. */
> +       cygnus_clk->clock_gate_ctrl_reg = of_iomap(node,
> +               CYGNUS_CLK_GATE_CTRL_REG);
> +       cygnus_clk->pll_ctrl_reg = of_iomap(node, CYGNUS_PLL_CTRL_REG);
> +
> +       of_property_read_u32(node, "channel", &channel);

Why do we read this twice?

> +       cygnus_clk->chan = channel;
> +       of_property_read_string(node, "clock-output-names", &clk_name);

What happens if this is missing from the dt?

> +
> +       /*
> +        * Internal divider is optional and used for PLL derived clocks with
> +        * hardcoded dividers.
> +        */
> +       cygnus_clk->internal_div = CLK_RATE_NO_DIV;
> +       of_property_read_u32(node, "div", &cygnus_clk->internal_div);
> +
> +       init.name = clk_name;
> +       init.ops = ops;
> +       init.flags = CLK_GET_RATE_NOCACHE;
> +       parent_name = of_clk_get_parent_name(node, 0);
> +       init.parent_names = &parent_name;
> +       init.num_parents = 1;
> +
> +       cygnus_clk->hw.init = &init;
> +
> +       clk = clk_register(NULL, &cygnus_clk->hw);
> +       if (WARN_ON(IS_ERR(clk)))
> +               goto err_unmap;
> +
> +       rc = of_clk_add_provider(node, of_clk_src_simple_get, clk);
> +       if (WARN_ON(IS_ERR_VALUE(rc)))
> +               goto err_unregister;
> +
> +       rc = clk_register_clkdev(clk, clk_name, NULL);
> +       if (WARN_ON(IS_ERR_VALUE(rc)))
> +               goto err_provider;
> +
> +       return clk;
> +
> +err_provider:
> +       of_clk_del_provider(node);
> +
> +err_unregister:
> +       clk_unregister(clk);
> +
> +err_unmap:
> +       iounmap(cygnus_clk->regs_base);
> +       iounmap(cygnus_clk->clock_gate_ctrl_reg);
> +       iounmap(cygnus_clk->pll_ctrl_reg);
> +
> +err_alloc:
> +       kfree(cygnus_clk);
> +
> +       return NULL;
> +}

Thanks,
Mark.

[1] http://en.wikipedia.org/wiki/Apostrophe#Superfluous_apostrophes_.28.22greengrocers.27_apostrophes.22.29