On Tue, 9 Feb 2016 22:51:34 +0100
Alban <albeu@xxxxxxx> wrote:
> On Tue, 9 Feb 2016 11:13:47 +0300
> Antony Pavlov <antonynpavlov@xxxxxxxxx> wrote:
>
> > This driver can be easely upgraded for other Atheros
> > SoCs (e.g. AR724X/AR913X) support.
> >
> > Signed-off-by: Antony Pavlov <antonynpavlov@xxxxxxxxx>
> > Cc: Alban Bedel <albeu@xxxxxxx>
> > Cc: Michael Turquette <mturquette@xxxxxxxxxxxx>
> > Cc: Stephen Boyd <sboyd@xxxxxxxxxxxxxx>
> > Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> > Cc: Paul Burton <paul.burton@xxxxxxxxxx>
> > Cc: linux-clk@xxxxxxxxxxxxxxx
> > Cc: linux-mips@xxxxxxxxxxxxxx
> > Cc: devicetree@xxxxxxxxxxxxxxx
> > ---
> > drivers/clk/Makefile | 1 +
> > drivers/clk/clk-ath79.c | 354 ++++++++++++++++++++++++++++++++++
> > include/dt-bindings/clock/ath79-clk.h | 22 +++
> > 3 files changed, 377 insertions(+)
> >
> > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> > index b038e36..d7ad50e 100644
> > --- a/drivers/clk/Makefile
> > +++ b/drivers/clk/Makefile
> > @@ -18,6 +18,7 @@ endif
> > # hardware specific clock types
> > # please keep this section sorted lexicographically by file/directory path name
> > obj-$(CONFIG_MACH_ASM9260) += clk-asm9260.o
> > +obj-$(CONFIG_ATH79) += clk-ath79.o
> > obj-$(CONFIG_COMMON_CLK_AXI_CLKGEN) += clk-axi-clkgen.o
> > obj-$(CONFIG_ARCH_AXXIA) += clk-axm5516.o
> > obj-$(CONFIG_COMMON_CLK_CDCE706) += clk-cdce706.o
> > diff --git a/drivers/clk/clk-ath79.c b/drivers/clk/clk-ath79.c
> > new file mode 100644
> > index 0000000..e899d31
> > --- /dev/null
> > +++ b/drivers/clk/clk-ath79.c
> > @@ -0,0 +1,354 @@
> > +/*
> > + * Clock driver for Atheros AR933X SoCs
> > + *
> > + * Copyright (C) 2016 Antony Pavlov <antonynpavlov@xxxxxxxxx>
> > + *
> > + * This driver is based on Ingenic CGU linux driver by Paul Burton
> > + * and AR9331 barebox driver by Antony Pavlov.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation; either version 2 of
> > + * the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/clkdev.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +
> > +#include <dt-bindings/clock/ath79-clk.h>
> > +
> > +#include "asm/mach-ath79/ar71xx_regs.h"
>
> This header shouldn't be used in new code, just defines the few
> registers needed here. Not using this header allow the driver
> to be built in compile test which increase test coverage.
ok.
> > +struct ath79_pll_info {
> > + u32 div_shift;
> > + u32 div_mask;
> > +};
> > +
> > +struct ath79_cblk;
> > +
> > +/**
> > + * struct ath79_clk_info - information about a clock
> > + * @name: name of the clock
> > + * @type: a bitmask formed from ATH79_CLK_* values
> > + * @parents: an index of parent of this clock
> > + * within the clock_info array, or -1
> > + * which correspond to no valid parent
> > + * @pll: information valid if type includes ATH79_CLK_PLL
> > + */
> > +struct ath79_clk_info {
> > + const char *name;
> > +
> > + enum {
> > + ATH79_CLK_NONE = 0,
> > + ATH79_CLK_EXT = 1,
> > + ATH79_CLK_PLL = 2,
> > + ATH79_CLK_ALIAS = 3,
> > + } type;
> > +
> > + struct ath79_cblk *cblk;
> > + int parent;
> > +
> > + struct ath79_pll_info pll;
> > +};
> > +
> > +struct ath79_cblk {
> > + struct device_node *np;
> > + void __iomem *base;
> > +
> > + const struct ath79_clk_info *clock_info;
> > + struct clk_onecell_data clocks;
> > +};
> > +
> > +/**
> > + * struct ath79_clk - private data for a clock
> > + * @hw: see Documentation/clk.txt
> > + * @cblk: a pointer to the cblk data
> > + * @idx: the index of this clock cblk->clock_info
> > + * @pll: information valid if type includes ATH79_CLK_PLL
> > + */
> > +struct ath79_clk {
> > + struct clk_hw hw;
> > + struct ath79_cblk *cblk;
> > + unsigned idx;
> > +};
> > +
> > +#define to_ath79_clk(_hw) container_of(_hw, struct ath79_clk, hw)
> > +
> > +static const struct ath79_clk_info ar9331_clocks[] = {
> > +
> > + /* External clock */
> > + [ATH79_CLK_REF] = { "ref", ATH79_CLK_EXT },
> > +
> > + [ATH79_CLK_CPU] = {
> > + "cpu", ATH79_CLK_PLL,
> > + .parent = ATH79_CLK_REF,
> > + .pll = {
> > + .div_shift = AR933X_PLL_CLOCK_CTRL_CPU_DIV_SHIFT,
> > + .div_mask = AR933X_PLL_CLOCK_CTRL_CPU_DIV_MASK,
> > + },
> > + },
> > +
> > + [ATH79_CLK_DDR] = {
> > + "ddr", ATH79_CLK_PLL,
> > + .parent = ATH79_CLK_REF,
> > + .pll = {
> > + .div_shift = AR933X_PLL_CLOCK_CTRL_DDR_DIV_SHIFT,
> > + .div_mask = AR933X_PLL_CLOCK_CTRL_DDR_DIV_MASK,
> > + },
> > + },
> > +
> > + [ATH79_CLK_AHB] = {
> > + "ahb", ATH79_CLK_PLL,
> > + .parent = ATH79_CLK_REF,
> > + .pll = {
> > + .div_shift = AR933X_PLL_CLOCK_CTRL_AHB_DIV_SHIFT,
> > + .div_mask = AR933X_PLL_CLOCK_CTRL_AHB_DIV_MASK,
> > + },
> > + },
> > +
> > + [ATH79_CLK_WDT] = {
> > + "wdt", ATH79_CLK_ALIAS,
> > + .parent = ATH79_CLK_AHB,
> > + },
> > +
> > + [ATH79_CLK_UART] = {
> > + "uart", ATH79_CLK_ALIAS,
> > + .parent = ATH79_CLK_REF,
> > + },
> > +};
> > +
> > +struct ath79_cblk *
> > +ath79_cblk_new(const struct ath79_clk_info *clock_info,
> > + unsigned num_clocks, struct device_node *np)
> > +{
> > + struct ath79_cblk *cblk;
> > +
> > + cblk = kzalloc(sizeof(*cblk), GFP_KERNEL);
> > + if (!cblk)
> > + goto err_out;
> > +
> > + cblk->base = of_iomap(np, 0);
> > + if (!cblk->base) {
> > + pr_err("%s: failed to map clock block registers\n", __func__);
> > + goto err_out_free;
> > + }
> > +
> > + cblk->np = np;
> > + cblk->clock_info = clock_info;
> > + cblk->clocks.clk_num = num_clocks;
> > +
> > + return cblk;
> > +
> > +err_out_free:
> > + kfree(cblk);
> > +
> > +err_out:
> > + return NULL;
> > +}
> > +
> > +static unsigned long
> > +ath79_pll_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
> > +{
> > + struct ath79_clk *ath79_clk = to_ath79_clk(hw);
> > + struct ath79_cblk *cblk = ath79_clk->cblk;
> > + const struct ath79_clk_info *clk_info = &cblk->clock_info[ath79_clk->idx];
> > + const struct ath79_pll_info *pll_info;
> > + unsigned long rate;
> > + unsigned long freq;
> > + u32 clock_ctrl;
> > + u32 cpu_config;
> > + u32 t;
> > +
> > + BUG_ON(clk_info->type != ATH79_CLK_PLL);
>
> It's probably debatable if such a BUG_ON() is really needed.
In simple RFC v5 driver version this check is redundant.
I suppose it's reasonable for more advanced version of the driver.
> > + clock_ctrl = __raw_readl(cblk->base + AR933X_PLL_CLOCK_CTRL_REG);
> > +
> > + if (clock_ctrl & AR933X_PLL_CLOCK_CTRL_BYPASS) {
> > + return parent_rate;
> > + }
>
> Those brace should goes away.
Ok.
> > + cpu_config = __raw_readl(cblk->base + AR933X_PLL_CPU_CONFIG_REG);
> > +
> > + t = (cpu_config >> AR933X_PLL_CPU_CONFIG_REFDIV_SHIFT) &
> > + AR933X_PLL_CPU_CONFIG_REFDIV_MASK;
> > + freq = parent_rate / t;
> > +
> > + t = (cpu_config >> AR933X_PLL_CPU_CONFIG_NINT_SHIFT) &
> > + AR933X_PLL_CPU_CONFIG_NINT_MASK;
> > + freq *= t;
> > +
> > + t = (cpu_config >> AR933X_PLL_CPU_CONFIG_OUTDIV_SHIFT) &
> > + AR933X_PLL_CPU_CONFIG_OUTDIV_MASK;
> > + if (t == 0)
> > + t = 1;
> > +
> > + freq >>= t;
> > +
> > + pll_info = &clk_info->pll;
> > +
> > + t = ((clock_ctrl >> pll_info->div_shift) & pll_info->div_mask) + 1;
> > + rate = freq / t;
>
> If we just compute a fixed rate we could as well use
> clk_register_fixed_factor() and drop 80% of the code of this driver.
80% is an overstatement.
> > + return rate;
> > +}
> > +
> > +static const struct clk_ops ath79_pll_clk_ops = {
> > + .recalc_rate = ath79_pll_recalc_rate,
> > +};
> > +
> > +static int ath79_register_clock(struct ath79_cblk *cblk, unsigned idx)
> > +{
> > + const struct ath79_clk_info *clk_info = &cblk->clock_info[idx];
> > + const struct ath79_clk_info *parent_clk_info;
> > + struct clk_init_data clk_init;
> > + struct ath79_clk *ath79_clk = NULL;
> > + struct clk *clk;
> > + int err = -EINVAL;
> > +
> > + if (clk_info->type == ATH79_CLK_EXT) {
> > + clk = of_clk_get_by_name(cblk->np, clk_info->name);
> > + if (IS_ERR(clk)) {
> > + pr_err("%s: no external clock '%s' provided\n",
> > + __func__, clk_info->name);
> > + err = -ENODEV;
> > + goto out;
> > + }
> > +
> > + err = clk_register_clkdev(clk, clk_info->name, NULL);
> > + if (err) {
> > + clk_put(clk);
> > + goto out;
> > + }
>
> clk_register_clkdev() and naming providers is not needed on OF
> platforms. This should only be used on legacy platforms.
I can't drop these clk_register_clkdev() just now without patching legacy code.
If I just drop clk_register_clkdev() then I get
Kernel panic - not syncing: unable to get cpu clock, err=-2
on start.
> > + cblk->clocks.clks[idx] = clk;
> > +
> > + return 0;
> > + }
> > +
> > + parent_clk_info = &cblk->clock_info[clk_info->parent];
> > +
> > + if (clk_info->type == ATH79_CLK_ALIAS) {
> > + clk = clk_register_fixed_factor(NULL, clk_info->name,
> > + parent_clk_info->name, 0, 1, 1);
> > + if (IS_ERR(clk)) {
> > + pr_err("%s: failed to register clock '%s'\n", __func__,
> > + clk_info->name);
> > + err = PTR_ERR(clk);
> > + goto out;
> > + }
> > +
> > + cblk->clocks.clks[idx] = clk;
> > +
> > + return 0;
> > + }
>
> I really don't get why you keep insisting on having those useless alias
> clocks. Alias are only needed on legacy platforms to form connections
> between clock providers and consumers. On OF platforms these
> connections are nicely represented in the DT, so it is just not
> needed at all.
I have droppped these aliases in RFC v6 series.
--
Best regards,
Antony Pavlov
