Re: [RFC v5 01/15] WIP: clk: add Atheros AR933X SoCs clock driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux