| Message ID | 20230316104707.236034-4-keguang.zhang@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Devicetree support for Loongson-1 clock | expand |
Hi Keguang,
I love your patch! Yet something to improve:
[auto build test ERROR on 6f173737e1b5670c200329677e821cce1d3d755e]
url: https://github.com/intel-lab-lkp/linux/commits/Keguang-Zhang/dt-bindings-clock-Add-Loongson-1-clock/20230316-185026
base: 6f173737e1b5670c200329677e821cce1d3d755e
patch link: https://lore.kernel.org/r/20230316104707.236034-4-keguang.zhang%40gmail.com
patch subject: [PATCH v3 3/4] clk: loongson1: Re-implement the clock driver
config: mips-loongson1c_defconfig (https://download.01.org/0day-ci/archive/20230318/202303181358.BXLJVMkh-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install mips cross compiling tool for clang build
# apt-get install binutils-mipsel-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/de00eab744ddc82edb1853048dd5d50aa8220115
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Keguang-Zhang/dt-bindings-clock-Add-Loongson-1-clock/20230316-185026
git checkout de00eab744ddc82edb1853048dd5d50aa8220115
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=mips olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash drivers/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303181358.BXLJVMkh-lkp@intel.com/
All errors (new ones prefixed by >>):
>> drivers/clk/clk-loongson1.c:300:15: error: expected parameter declarator
MODULE_AUTHOR("Keguang Zhang <keguang.zhang@gmail.com>");
^
>> drivers/clk/clk-loongson1.c:300:15: error: expected ')'
drivers/clk/clk-loongson1.c:300:14: note: to match this '('
MODULE_AUTHOR("Keguang Zhang <keguang.zhang@gmail.com>");
^
>> drivers/clk/clk-loongson1.c:300:1: error: type specifier missing, defaults to 'int'; ISO C99 and later do not support implicit int [-Wimplicit-int]
MODULE_AUTHOR("Keguang Zhang <keguang.zhang@gmail.com>");
^
int
>> drivers/clk/clk-loongson1.c:300:14: error: a function declaration without a prototype is deprecated in all versions of C [-Werror,-Wstrict-prototypes]
MODULE_AUTHOR("Keguang Zhang <keguang.zhang@gmail.com>");
^
void
drivers/clk/clk-loongson1.c:301:20: error: expected parameter declarator
MODULE_DESCRIPTION("Loongson1 clock driver");
^
drivers/clk/clk-loongson1.c:301:20: error: expected ')'
drivers/clk/clk-loongson1.c:301:19: note: to match this '('
MODULE_DESCRIPTION("Loongson1 clock driver");
^
drivers/clk/clk-loongson1.c:301:1: error: type specifier missing, defaults to 'int'; ISO C99 and later do not support implicit int [-Wimplicit-int]
MODULE_DESCRIPTION("Loongson1 clock driver");
^
int
drivers/clk/clk-loongson1.c:301:19: error: a function declaration without a prototype is deprecated in all versions of C [-Werror,-Wstrict-prototypes]
MODULE_DESCRIPTION("Loongson1 clock driver");
^
void
8 errors generated.
vim +300 drivers/clk/clk-loongson1.c
299
> 300 MODULE_AUTHOR("Keguang Zhang <keguang.zhang@gmail.com>");
Quoting Keguang Zhang (2023-03-16 03:47:06) > diff --git a/drivers/clk/clk-loongson1.c b/drivers/clk/clk-loongson1.c > new file mode 100644 > index 000000000000..4fda55c67d8d > --- /dev/null > +++ b/drivers/clk/clk-loongson1.c > @@ -0,0 +1,301 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Clock driver for Loongson-1 SoC > + * > + * Copyright (C) 2012-2023 Keguang Zhang <keguang.zhang@gmail.com> > + */ > + > +#include <linux/clk-provider.h> > +#include <linux/of_address.h> > +#include <linux/slab.h> Need some more includes here, for container_of() and GENMASK(), readl(), etc. > + > +#include <dt-bindings/clock/loongson,ls1x-clk.h> > + > +/* Loongson 1 Clock Register Definitions */ > +#define CLK_PLL_FREQ 0x0 > +#define CLK_PLL_DIV 0x4 > + > +static DEFINE_SPINLOCK(ls1x_clk_div_lock); > + Needs include. > +struct ls1x_clk_pll_data { > + u32 fixed; > + u8 shift; > + u8 int_shift; > + u8 int_width; > + u8 frac_shift; > + u8 frac_width; > +}; > + > +struct ls1x_clk_div_data { > + u8 shift; > + u8 width; > + unsigned long flags; > + const struct clk_div_table *table; > + u8 bypass_shift; > + u8 bypass_inv; > + spinlock_t *lock; /* protect access to DIV registers */ > +}; > + > +struct ls1x_clk { > + void __iomem *reg; > + unsigned int offset; > + struct clk_hw hw; > + void *data; > +}; > + > +#define to_ls1x_clk(_hw) container_of(_hw, struct ls1x_clk, hw) > + > +static inline unsigned long ls1x_pll_rate_part(unsigned int val, return a u32? > + unsigned int shift, > + unsigned int width) > +{ > + return (val & GENMASK(shift + width, shift)) >> shift; > +} > + > +static unsigned long ls1x_pll_recalc_rate(struct clk_hw *hw, > + unsigned long parent_rate) > +{ > + struct ls1x_clk *ls1x_clk = to_ls1x_clk(hw); > + const struct ls1x_clk_pll_data *d = ls1x_clk->data; > + u32 val, rate; > + > + val = readl(ls1x_clk->reg); > + rate = d->fixed; > + rate += ls1x_pll_rate_part(val, d->int_shift, d->int_width); > + if (d->frac_width) > + rate += ls1x_pll_rate_part(val, d->frac_shift, d->frac_width); > + rate *= parent_rate; > + rate >>= d->shift; > + > + return rate; > +} > + > +static const struct clk_ops ls1x_pll_clk_ops = { > + .recalc_rate = ls1x_pll_recalc_rate, > +}; > + > +static unsigned long ls1x_divider_recalc_rate(struct clk_hw *hw, > + unsigned long parent_rate) > +{ > + struct ls1x_clk *ls1x_clk = to_ls1x_clk(hw); > + const struct ls1x_clk_div_data *d = ls1x_clk->data; > + unsigned int val; > + > + val = readl(ls1x_clk->reg) >> d->shift; > + val &= clk_div_mask(d->width); > + > + return divider_recalc_rate(hw, parent_rate, val, d->table, > + d->flags, d->width); > +} > + > +static long ls1x_divider_round_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long *prate) > +{ > + struct ls1x_clk *ls1x_clk = to_ls1x_clk(hw); > + const struct ls1x_clk_div_data *d = ls1x_clk->data; > + > + return divider_round_rate(hw, rate, prate, d->table, > + d->width, d->flags); > +} > + > +static int ls1x_divider_set_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long parent_rate) > +{ > + struct ls1x_clk *ls1x_clk = to_ls1x_clk(hw); > + const struct ls1x_clk_div_data *d = ls1x_clk->data; > + int val, div_val; > + unsigned long flags = 0; > + > + div_val = divider_get_val(rate, parent_rate, d->table, > + d->width, d->flags); > + if (div_val < 0) > + return div_val; > + > + if (d->lock) > + spin_lock_irqsave(d->lock, flags); > + else > + __acquire(d->lock); > + > + /* Bypass the clock */ > + val = readl(ls1x_clk->reg); > + if (d->bypass_inv) > + val &= ~BIT(d->bypass_shift); > + else > + val |= BIT(d->bypass_shift); > + writel(val, ls1x_clk->reg); > + > + val = readl(ls1x_clk->reg); > + val &= ~(clk_div_mask(d->width) << d->shift); > + val |= (u32)div_val << d->shift; > + writel(val, ls1x_clk->reg); > + > + /* Restore the clock */ > + val = readl(ls1x_clk->reg); > + if (d->bypass_inv) > + val |= BIT(d->bypass_shift); > + else > + val &= ~BIT(d->bypass_shift); > + writel(val, ls1x_clk->reg); > + > + if (d->lock) > + spin_unlock_irqrestore(d->lock, flags); > + else > + __release(d->lock); Is there a case where there isn't a lock? It would be easier to read if this always had a lock and it wasn't optional. > + > + return 0; > +} > + > +static const struct clk_ops ls1x_clk_divider_ops = { > + .recalc_rate = ls1x_divider_recalc_rate, > + .round_rate = ls1x_divider_round_rate, > + .set_rate = ls1x_divider_set_rate, > +}; > + > +#define LS1X_CLK_PLL(_name, _offset, _fixed, _shift, \ > + f_shift, f_width, i_shift, i_width) \ > +struct ls1x_clk _name = { \ > + .offset = (_offset), \ > + .data = &(struct ls1x_clk_pll_data) { \ > + .fixed = (_fixed), \ > + .shift = (_shift), \ > + .int_shift = (i_shift), \ > + .int_width = (i_width), \ > + .frac_shift = (f_shift), \ > + .frac_width = (f_width), \ > + }, \ > + .hw.init = &(struct clk_init_data) { \ > + .name = #_name, \ > + .ops = &ls1x_pll_clk_ops, \ > + .parent_data = &(const struct clk_parent_data) { \ > + .fw_name = "xtal", \ > + .name = "xtal", \ > + .index = -1, \ > + }, \ > + .num_parents = 1, \ > + }, \ > +} > + > +#define LS1X_CLK_DIV(_name, _pname, _offset, _shift, _width, \ > + _table, _bypass_shift, _bypass_inv, _flags) \ > +struct ls1x_clk _name = { \ > + .offset = (_offset), \ > + .data = &(struct ls1x_clk_div_data){ \ > + .shift = (_shift), \ > + .width = (_width), \ > + .table = (_table), \ > + .flags = (_flags), \ > + .bypass_shift = (_bypass_shift), \ > + .bypass_inv = (_bypass_inv), \ > + .lock = &ls1x_clk_div_lock, \ > + }, \ > + .hw.init = &(struct clk_init_data) { \ Can be const. > + .name = #_name, \ > + .ops = &ls1x_clk_divider_ops, \ > + .parent_hws = (const struct clk_hw *[]) { _pname }, \ > + .num_parents = 1, \ > + .flags = CLK_GET_RATE_NOCACHE, \ > + }, \ > +} > + > +static LS1X_CLK_PLL(ls1b_clk_pll, CLK_PLL_FREQ, 12, 1, 0, 5, 0, 0); > +static LS1X_CLK_DIV(ls1b_clk_cpu, &ls1b_clk_pll.hw, CLK_PLL_DIV, > + 20, 4, NULL, 8, 0, > + CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ROUND_CLOSEST); > +static LS1X_CLK_DIV(ls1b_clk_dc, &ls1b_clk_pll.hw, CLK_PLL_DIV, > + 26, 4, NULL, 12, 0, CLK_DIVIDER_ONE_BASED); > +static LS1X_CLK_DIV(ls1b_clk_ahb, &ls1b_clk_pll.hw, CLK_PLL_DIV, > + 14, 4, NULL, 10, 0, CLK_DIVIDER_ONE_BASED); > +static CLK_FIXED_FACTOR(ls1b_clk_apb, "ls1b_clk_apb", "ls1b_clk_ahb", 2, 1, > + CLK_SET_RATE_PARENT); > + > +static struct clk_hw_onecell_data ls1b_clk_hw_data = { > + .hws = { > + [LS1X_CLKID_PLL] = &ls1b_clk_pll.hw, > + [LS1X_CLKID_CPU] = &ls1b_clk_cpu.hw, > + [LS1X_CLKID_DC] = &ls1b_clk_dc.hw, > + [LS1X_CLKID_AHB] = &ls1b_clk_ahb.hw, > + [LS1X_CLKID_APB] = &ls1b_clk_apb.hw, > + [CLK_NR_CLKS] = NULL, Do you need a CLK_NR_CLKS sentinel entry? > + }, > + .num = CLK_NR_CLKS, > +}; > + > +static const struct clk_div_table ls1c_ahb_div_table[] = { > + [0] = { .val = 0, .div = 2 }, > + [1] = { .val = 1, .div = 4 }, > + [2] = { .val = 2, .div = 3 }, > + [3] = { .val = 3, .div = 3 }, > + [4] = { /* sentinel */ } > +}; > + > +static LS1X_CLK_PLL(ls1c_clk_pll, CLK_PLL_FREQ, 0, 2, 8, 8, 16, 8); > +static LS1X_CLK_DIV(ls1c_clk_cpu, &ls1c_clk_pll.hw, CLK_PLL_DIV, > + 8, 7, NULL, 0, 1, > + CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ROUND_CLOSEST); > +static LS1X_CLK_DIV(ls1c_clk_dc, &ls1c_clk_pll.hw, CLK_PLL_DIV, > + 24, 7, NULL, 4, 1, CLK_DIVIDER_ONE_BASED); > +static LS1X_CLK_DIV(ls1c_clk_ahb, &ls1c_clk_cpu.hw, CLK_PLL_FREQ, > + 0, 2, ls1c_ahb_div_table, 0, 0, CLK_DIVIDER_ALLOW_ZERO); > +static CLK_FIXED_FACTOR(ls1c_clk_apb, "ls1c_clk_apb", "ls1c_clk_ahb", 1, 1, > + CLK_SET_RATE_PARENT); > + > +static struct clk_hw_onecell_data ls1c_clk_hw_data = { > + .hws = { > + [LS1X_CLKID_PLL] = &ls1c_clk_pll.hw, > + [LS1X_CLKID_CPU] = &ls1c_clk_cpu.hw, > + [LS1X_CLKID_DC] = &ls1c_clk_dc.hw, > + [LS1X_CLKID_AHB] = &ls1c_clk_ahb.hw, > + [LS1X_CLKID_APB] = &ls1c_clk_apb.hw, > + [CLK_NR_CLKS] = NULL, > + }, > + .num = CLK_NR_CLKS, > +}; > + > +static void __init ls1x_clk_init(struct device_node *np, > + struct clk_hw_onecell_data *hw_data) > +{ > + struct ls1x_clk *ls1x_clk; > + void __iomem *reg; > + int i, ret; > + > + reg = of_iomap(np, 0); > + if (!reg) { > + pr_err("Unable to map base for %pOF\n", np); Needs include. > + return; > + } > + > + for (i = 0; i < CLK_NR_CLKS; i++) { > + /* array might be sparse */ > + if (!hw_data->hws[i]) > + continue; > + > + if (i != LS1X_CLKID_APB) { > + ls1x_clk = to_ls1x_clk(hw_data->hws[i]); > + ls1x_clk->reg = reg + ls1x_clk->offset; > + } > + > + ret = of_clk_hw_register(np, hw_data->hws[i]); > + if (ret) > + return; unmap memory on failure? and unregister clks? > + } > + > + ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, hw_data); > + if (ret) > + pr_err("Failed to register %pOF\n", np); unmap memory on failure? And unregister clks? > +} > + > +static void __init ls1b_clk_init(struct device_node *np) > +{ > + return ls1x_clk_init(np, &ls1b_clk_hw_data); > +} > + > +static void __init ls1c_clk_init(struct device_node *np) > +{ > + return ls1x_clk_init(np, &ls1c_clk_hw_data); > +} > + > +CLK_OF_DECLARE(ls1b_clk, "loongson,ls1b-clk", ls1b_clk_init); > +CLK_OF_DECLARE(ls1c_clk, "loongson,ls1c-clk", ls1c_clk_init); Any reason these can't be platform device drivers? > + > +MODULE_AUTHOR("Keguang Zhang <keguang.zhang@gmail.com>"); > +MODULE_DESCRIPTION("Loongson1 clock driver"); It's not a module. So these are useless macros. Drop them?
On Tue, Mar 21, 2023 at 4:06 AM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Keguang Zhang (2023-03-16 03:47:06) > > diff --git a/drivers/clk/clk-loongson1.c b/drivers/clk/clk-loongson1.c > > new file mode 100644 > > index 000000000000..4fda55c67d8d > > --- /dev/null > > +++ b/drivers/clk/clk-loongson1.c > > @@ -0,0 +1,301 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * Clock driver for Loongson-1 SoC > > + * > > + * Copyright (C) 2012-2023 Keguang Zhang <keguang.zhang@gmail.com> > > + */ > > + > > +#include <linux/clk-provider.h> > > +#include <linux/of_address.h> > > +#include <linux/slab.h> > > Need some more includes here, for container_of() and GENMASK(), readl(), > etc. > Will do. > > + > > +#include <dt-bindings/clock/loongson,ls1x-clk.h> > > + > > +/* Loongson 1 Clock Register Definitions */ > > +#define CLK_PLL_FREQ 0x0 > > +#define CLK_PLL_DIV 0x4 > > + > > +static DEFINE_SPINLOCK(ls1x_clk_div_lock); > > + > > Needs include. > Will do. > > +struct ls1x_clk_pll_data { > > + u32 fixed; > > + u8 shift; > > + u8 int_shift; > > + u8 int_width; > > + u8 frac_shift; > > + u8 frac_width; > > +}; > > + > > +struct ls1x_clk_div_data { > > + u8 shift; > > + u8 width; > > + unsigned long flags; > > + const struct clk_div_table *table; > > + u8 bypass_shift; > > + u8 bypass_inv; > > + spinlock_t *lock; /* protect access to DIV registers */ > > +}; > > + > > +struct ls1x_clk { > > + void __iomem *reg; > > + unsigned int offset; > > + struct clk_hw hw; > > + void *data; > > +}; > > + > > +#define to_ls1x_clk(_hw) container_of(_hw, struct ls1x_clk, hw) > > + > > +static inline unsigned long ls1x_pll_rate_part(unsigned int val, > > return a u32? > Yes. > > + unsigned int shift, > > + unsigned int width) > > +{ > > + return (val & GENMASK(shift + width, shift)) >> shift; > > +} > > + > > +static unsigned long ls1x_pll_recalc_rate(struct clk_hw *hw, > > + unsigned long parent_rate) > > +{ > > + struct ls1x_clk *ls1x_clk = to_ls1x_clk(hw); > > + const struct ls1x_clk_pll_data *d = ls1x_clk->data; > > + u32 val, rate; > > + > > + val = readl(ls1x_clk->reg); > > + rate = d->fixed; > > + rate += ls1x_pll_rate_part(val, d->int_shift, d->int_width); > > + if (d->frac_width) > > + rate += ls1x_pll_rate_part(val, d->frac_shift, d->frac_width); > > + rate *= parent_rate; > > + rate >>= d->shift; > > + > > + return rate; > > +} > > + > > +static const struct clk_ops ls1x_pll_clk_ops = { > > + .recalc_rate = ls1x_pll_recalc_rate, > > +}; > > + > > +static unsigned long ls1x_divider_recalc_rate(struct clk_hw *hw, > > + unsigned long parent_rate) > > +{ > > + struct ls1x_clk *ls1x_clk = to_ls1x_clk(hw); > > + const struct ls1x_clk_div_data *d = ls1x_clk->data; > > + unsigned int val; > > + > > + val = readl(ls1x_clk->reg) >> d->shift; > > + val &= clk_div_mask(d->width); > > + > > + return divider_recalc_rate(hw, parent_rate, val, d->table, > > + d->flags, d->width); > > +} > > + > > +static long ls1x_divider_round_rate(struct clk_hw *hw, unsigned long rate, > > + unsigned long *prate) > > +{ > > + struct ls1x_clk *ls1x_clk = to_ls1x_clk(hw); > > + const struct ls1x_clk_div_data *d = ls1x_clk->data; > > + > > + return divider_round_rate(hw, rate, prate, d->table, > > + d->width, d->flags); > > +} > > + > > +static int ls1x_divider_set_rate(struct clk_hw *hw, unsigned long rate, > > + unsigned long parent_rate) > > +{ > > + struct ls1x_clk *ls1x_clk = to_ls1x_clk(hw); > > + const struct ls1x_clk_div_data *d = ls1x_clk->data; > > + int val, div_val; > > + unsigned long flags = 0; > > + > > + div_val = divider_get_val(rate, parent_rate, d->table, > > + d->width, d->flags); > > + if (div_val < 0) > > + return div_val; > > + > > + if (d->lock) > > + spin_lock_irqsave(d->lock, flags); > > + else > > + __acquire(d->lock); > > + > > + /* Bypass the clock */ > > + val = readl(ls1x_clk->reg); > > + if (d->bypass_inv) > > + val &= ~BIT(d->bypass_shift); > > + else > > + val |= BIT(d->bypass_shift); > > + writel(val, ls1x_clk->reg); > > + > > + val = readl(ls1x_clk->reg); > > + val &= ~(clk_div_mask(d->width) << d->shift); > > + val |= (u32)div_val << d->shift; > > + writel(val, ls1x_clk->reg); > > + > > + /* Restore the clock */ > > + val = readl(ls1x_clk->reg); > > + if (d->bypass_inv) > > + val |= BIT(d->bypass_shift); > > + else > > + val &= ~BIT(d->bypass_shift); > > + writel(val, ls1x_clk->reg); > > + > > + if (d->lock) > > + spin_unlock_irqrestore(d->lock, flags); > > + else > > + __release(d->lock); > > Is there a case where there isn't a lock? It would be easier to read if > this always had a lock and it wasn't optional. > Indeed. The lock always exists in this driver. Will remove the unnecessary __acquire()/__release(). > > + > > + return 0; > > +} > > + > > +static const struct clk_ops ls1x_clk_divider_ops = { > > + .recalc_rate = ls1x_divider_recalc_rate, > > + .round_rate = ls1x_divider_round_rate, > > + .set_rate = ls1x_divider_set_rate, > > +}; > > + > > +#define LS1X_CLK_PLL(_name, _offset, _fixed, _shift, \ > > + f_shift, f_width, i_shift, i_width) \ > > +struct ls1x_clk _name = { \ > > + .offset = (_offset), \ > > + .data = &(struct ls1x_clk_pll_data) { \ > > + .fixed = (_fixed), \ > > + .shift = (_shift), \ > > + .int_shift = (i_shift), \ > > + .int_width = (i_width), \ > > + .frac_shift = (f_shift), \ > > + .frac_width = (f_width), \ > > + }, \ > > + .hw.init = &(struct clk_init_data) { \ > > + .name = #_name, \ > > + .ops = &ls1x_pll_clk_ops, \ > > + .parent_data = &(const struct clk_parent_data) { \ > > + .fw_name = "xtal", \ > > + .name = "xtal", \ > > + .index = -1, \ > > + }, \ > > + .num_parents = 1, \ > > + }, \ > > +} > > + > > +#define LS1X_CLK_DIV(_name, _pname, _offset, _shift, _width, \ > > + _table, _bypass_shift, _bypass_inv, _flags) \ > > +struct ls1x_clk _name = { \ > > + .offset = (_offset), \ > > + .data = &(struct ls1x_clk_div_data){ \ > > + .shift = (_shift), \ > > + .width = (_width), \ > > + .table = (_table), \ > > + .flags = (_flags), \ > > + .bypass_shift = (_bypass_shift), \ > > + .bypass_inv = (_bypass_inv), \ > > + .lock = &ls1x_clk_div_lock, \ > > + }, \ > > + .hw.init = &(struct clk_init_data) { \ > > Can be const. > Will do. > > + .name = #_name, \ > > + .ops = &ls1x_clk_divider_ops, \ > > + .parent_hws = (const struct clk_hw *[]) { _pname }, \ > > + .num_parents = 1, \ > > + .flags = CLK_GET_RATE_NOCACHE, \ > > + }, \ > > +} > > + > > +static LS1X_CLK_PLL(ls1b_clk_pll, CLK_PLL_FREQ, 12, 1, 0, 5, 0, 0); > > +static LS1X_CLK_DIV(ls1b_clk_cpu, &ls1b_clk_pll.hw, CLK_PLL_DIV, > > + 20, 4, NULL, 8, 0, > > + CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ROUND_CLOSEST); > > +static LS1X_CLK_DIV(ls1b_clk_dc, &ls1b_clk_pll.hw, CLK_PLL_DIV, > > + 26, 4, NULL, 12, 0, CLK_DIVIDER_ONE_BASED); > > +static LS1X_CLK_DIV(ls1b_clk_ahb, &ls1b_clk_pll.hw, CLK_PLL_DIV, > > + 14, 4, NULL, 10, 0, CLK_DIVIDER_ONE_BASED); > > +static CLK_FIXED_FACTOR(ls1b_clk_apb, "ls1b_clk_apb", "ls1b_clk_ahb", 2, 1, > > + CLK_SET_RATE_PARENT); > > + > > +static struct clk_hw_onecell_data ls1b_clk_hw_data = { > > + .hws = { > > + [LS1X_CLKID_PLL] = &ls1b_clk_pll.hw, > > + [LS1X_CLKID_CPU] = &ls1b_clk_cpu.hw, > > + [LS1X_CLKID_DC] = &ls1b_clk_dc.hw, > > + [LS1X_CLKID_AHB] = &ls1b_clk_ahb.hw, > > + [LS1X_CLKID_APB] = &ls1b_clk_apb.hw, > > + [CLK_NR_CLKS] = NULL, > > Do you need a CLK_NR_CLKS sentinel entry? > Not necessary. Will remove. > > + }, > > + .num = CLK_NR_CLKS, > > +}; > > + > > +static const struct clk_div_table ls1c_ahb_div_table[] = { > > + [0] = { .val = 0, .div = 2 }, > > + [1] = { .val = 1, .div = 4 }, > > + [2] = { .val = 2, .div = 3 }, > > + [3] = { .val = 3, .div = 3 }, > > + [4] = { /* sentinel */ } > > +}; > > + > > +static LS1X_CLK_PLL(ls1c_clk_pll, CLK_PLL_FREQ, 0, 2, 8, 8, 16, 8); > > +static LS1X_CLK_DIV(ls1c_clk_cpu, &ls1c_clk_pll.hw, CLK_PLL_DIV, > > + 8, 7, NULL, 0, 1, > > + CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ROUND_CLOSEST); > > +static LS1X_CLK_DIV(ls1c_clk_dc, &ls1c_clk_pll.hw, CLK_PLL_DIV, > > + 24, 7, NULL, 4, 1, CLK_DIVIDER_ONE_BASED); > > +static LS1X_CLK_DIV(ls1c_clk_ahb, &ls1c_clk_cpu.hw, CLK_PLL_FREQ, > > + 0, 2, ls1c_ahb_div_table, 0, 0, CLK_DIVIDER_ALLOW_ZERO); > > +static CLK_FIXED_FACTOR(ls1c_clk_apb, "ls1c_clk_apb", "ls1c_clk_ahb", 1, 1, > > + CLK_SET_RATE_PARENT); > > + > > +static struct clk_hw_onecell_data ls1c_clk_hw_data = { > > + .hws = { > > + [LS1X_CLKID_PLL] = &ls1c_clk_pll.hw, > > + [LS1X_CLKID_CPU] = &ls1c_clk_cpu.hw, > > + [LS1X_CLKID_DC] = &ls1c_clk_dc.hw, > > + [LS1X_CLKID_AHB] = &ls1c_clk_ahb.hw, > > + [LS1X_CLKID_APB] = &ls1c_clk_apb.hw, > > + [CLK_NR_CLKS] = NULL, > > + }, > > + .num = CLK_NR_CLKS, > > +}; > > + > > +static void __init ls1x_clk_init(struct device_node *np, > > + struct clk_hw_onecell_data *hw_data) > > +{ > > + struct ls1x_clk *ls1x_clk; > > + void __iomem *reg; > > + int i, ret; > > + > > + reg = of_iomap(np, 0); > > + if (!reg) { > > + pr_err("Unable to map base for %pOF\n", np); > > Needs include. > Will do. > > + return; > > + } > > + > > + for (i = 0; i < CLK_NR_CLKS; i++) { > > + /* array might be sparse */ > > + if (!hw_data->hws[i]) > > + continue; > > + > > + if (i != LS1X_CLKID_APB) { > > + ls1x_clk = to_ls1x_clk(hw_data->hws[i]); > > + ls1x_clk->reg = reg + ls1x_clk->offset; > > + } > > + > > + ret = of_clk_hw_register(np, hw_data->hws[i]); > > + if (ret) > > + return; > > unmap memory on failure? and unregister clks? > Will add the error handling. > > + } > > + > > + ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, hw_data); > > + if (ret) > > + pr_err("Failed to register %pOF\n", np); > > unmap memory on failure? And unregister clks? > Will add the error handling. > > +} > > + > > +static void __init ls1b_clk_init(struct device_node *np) > > +{ > > + return ls1x_clk_init(np, &ls1b_clk_hw_data); > > +} > > + > > +static void __init ls1c_clk_init(struct device_node *np) > > +{ > > + return ls1x_clk_init(np, &ls1c_clk_hw_data); > > +} > > + > > +CLK_OF_DECLARE(ls1b_clk, "loongson,ls1b-clk", ls1b_clk_init); > > +CLK_OF_DECLARE(ls1c_clk, "loongson,ls1c-clk", ls1c_clk_init); > > Any reason these can't be platform device drivers? > The Loongson1 system uses performance counter as the default clocksource. Then, mips_hpt_frequency should be figured out when doing plat_time_init(), just before calling mips_clockevent_init() and init_mips_clocksource(). And mips_hpt_frequency depends on CPU clock. So these clocks should be initialized as early as possible. It's too late for being a platform device driver. > > + > > +MODULE_AUTHOR("Keguang Zhang <keguang.zhang@gmail.com>"); > > +MODULE_DESCRIPTION("Loongson1 clock driver"); > > It's not a module. So these are useless macros. Drop them? Yes. I realized these macros will never be called. Will drop. -- Best regards, Keguang Zhang
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index b7b2c6d64636..417bc27ab6e8 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -43,6 +43,7 @@ obj-$(CONFIG_COMMON_CLK_K210) += clk-k210.o obj-$(CONFIG_LMK04832) += clk-lmk04832.o obj-$(CONFIG_COMMON_CLK_LAN966X) += clk-lan966x.o obj-$(CONFIG_COMMON_CLK_LOCHNAGAR) += clk-lochnagar.o +obj-$(CONFIG_MACH_LOONGSON32) += clk-loongson1.o obj-$(CONFIG_COMMON_CLK_MAX77686) += clk-max77686.o obj-$(CONFIG_COMMON_CLK_MAX9485) += clk-max9485.o obj-$(CONFIG_ARCH_MILBEAUT_M10V) += clk-milbeaut.o diff --git a/drivers/clk/clk-loongson1.c b/drivers/clk/clk-loongson1.c new file mode 100644 index 000000000000..4fda55c67d8d --- /dev/null +++ b/drivers/clk/clk-loongson1.c @@ -0,0 +1,301 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Clock driver for Loongson-1 SoC + * + * Copyright (C) 2012-2023 Keguang Zhang <keguang.zhang@gmail.com> + */ + +#include <linux/clk-provider.h> +#include <linux/of_address.h> +#include <linux/slab.h> + +#include <dt-bindings/clock/loongson,ls1x-clk.h> + +/* Loongson 1 Clock Register Definitions */ +#define CLK_PLL_FREQ 0x0 +#define CLK_PLL_DIV 0x4 + +static DEFINE_SPINLOCK(ls1x_clk_div_lock); + +struct ls1x_clk_pll_data { + u32 fixed; + u8 shift; + u8 int_shift; + u8 int_width; + u8 frac_shift; + u8 frac_width; +}; + +struct ls1x_clk_div_data { + u8 shift; + u8 width; + unsigned long flags; + const struct clk_div_table *table; + u8 bypass_shift; + u8 bypass_inv; + spinlock_t *lock; /* protect access to DIV registers */ +}; + +struct ls1x_clk { + void __iomem *reg; + unsigned int offset; + struct clk_hw hw; + void *data; +}; + +#define to_ls1x_clk(_hw) container_of(_hw, struct ls1x_clk, hw) + +static inline unsigned long ls1x_pll_rate_part(unsigned int val, + unsigned int shift, + unsigned int width) +{ + return (val & GENMASK(shift + width, shift)) >> shift; +} + +static unsigned long ls1x_pll_recalc_rate(struct clk_hw *hw, + unsigned long parent_rate) +{ + struct ls1x_clk *ls1x_clk = to_ls1x_clk(hw); + const struct ls1x_clk_pll_data *d = ls1x_clk->data; + u32 val, rate; + + val = readl(ls1x_clk->reg); + rate = d->fixed; + rate += ls1x_pll_rate_part(val, d->int_shift, d->int_width); + if (d->frac_width) + rate += ls1x_pll_rate_part(val, d->frac_shift, d->frac_width); + rate *= parent_rate; + rate >>= d->shift; + + return rate; +} + +static const struct clk_ops ls1x_pll_clk_ops = { + .recalc_rate = ls1x_pll_recalc_rate, +}; + +static unsigned long ls1x_divider_recalc_rate(struct clk_hw *hw, + unsigned long parent_rate) +{ + struct ls1x_clk *ls1x_clk = to_ls1x_clk(hw); + const struct ls1x_clk_div_data *d = ls1x_clk->data; + unsigned int val; + + val = readl(ls1x_clk->reg) >> d->shift; + val &= clk_div_mask(d->width); + + return divider_recalc_rate(hw, parent_rate, val, d->table, + d->flags, d->width); +} + +static long ls1x_divider_round_rate(struct clk_hw *hw, unsigned long rate, + unsigned long *prate) +{ + struct ls1x_clk *ls1x_clk = to_ls1x_clk(hw); + const struct ls1x_clk_div_data *d = ls1x_clk->data; + + return divider_round_rate(hw, rate, prate, d->table, + d->width, d->flags); +} + +static int ls1x_divider_set_rate(struct clk_hw *hw, unsigned long rate, + unsigned long parent_rate) +{ + struct ls1x_clk *ls1x_clk = to_ls1x_clk(hw); + const struct ls1x_clk_div_data *d = ls1x_clk->data; + int val, div_val; + unsigned long flags = 0; + + div_val = divider_get_val(rate, parent_rate, d->table, + d->width, d->flags); + if (div_val < 0) + return div_val; + + if (d->lock) + spin_lock_irqsave(d->lock, flags); + else + __acquire(d->lock); + + /* Bypass the clock */ + val = readl(ls1x_clk->reg); + if (d->bypass_inv) + val &= ~BIT(d->bypass_shift); + else + val |= BIT(d->bypass_shift); + writel(val, ls1x_clk->reg); + + val = readl(ls1x_clk->reg); + val &= ~(clk_div_mask(d->width) << d->shift); + val |= (u32)div_val << d->shift; + writel(val, ls1x_clk->reg); + + /* Restore the clock */ + val = readl(ls1x_clk->reg); + if (d->bypass_inv) + val |= BIT(d->bypass_shift); + else + val &= ~BIT(d->bypass_shift); + writel(val, ls1x_clk->reg); + + if (d->lock) + spin_unlock_irqrestore(d->lock, flags); + else + __release(d->lock); + + return 0; +} + +static const struct clk_ops ls1x_clk_divider_ops = { + .recalc_rate = ls1x_divider_recalc_rate, + .round_rate = ls1x_divider_round_rate, + .set_rate = ls1x_divider_set_rate, +}; + +#define LS1X_CLK_PLL(_name, _offset, _fixed, _shift, \ + f_shift, f_width, i_shift, i_width) \ +struct ls1x_clk _name = { \ + .offset = (_offset), \ + .data = &(struct ls1x_clk_pll_data) { \ + .fixed = (_fixed), \ + .shift = (_shift), \ + .int_shift = (i_shift), \ + .int_width = (i_width), \ + .frac_shift = (f_shift), \ + .frac_width = (f_width), \ + }, \ + .hw.init = &(struct clk_init_data) { \ + .name = #_name, \ + .ops = &ls1x_pll_clk_ops, \ + .parent_data = &(const struct clk_parent_data) { \ + .fw_name = "xtal", \ + .name = "xtal", \ + .index = -1, \ + }, \ + .num_parents = 1, \ + }, \ +} + +#define LS1X_CLK_DIV(_name, _pname, _offset, _shift, _width, \ + _table, _bypass_shift, _bypass_inv, _flags) \ +struct ls1x_clk _name = { \ + .offset = (_offset), \ + .data = &(struct ls1x_clk_div_data){ \ + .shift = (_shift), \ + .width = (_width), \ + .table = (_table), \ + .flags = (_flags), \ + .bypass_shift = (_bypass_shift), \ + .bypass_inv = (_bypass_inv), \ + .lock = &ls1x_clk_div_lock, \ + }, \ + .hw.init = &(struct clk_init_data) { \ + .name = #_name, \ + .ops = &ls1x_clk_divider_ops, \ + .parent_hws = (const struct clk_hw *[]) { _pname }, \ + .num_parents = 1, \ + .flags = CLK_GET_RATE_NOCACHE, \ + }, \ +} + +static LS1X_CLK_PLL(ls1b_clk_pll, CLK_PLL_FREQ, 12, 1, 0, 5, 0, 0); +static LS1X_CLK_DIV(ls1b_clk_cpu, &ls1b_clk_pll.hw, CLK_PLL_DIV, + 20, 4, NULL, 8, 0, + CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ROUND_CLOSEST); +static LS1X_CLK_DIV(ls1b_clk_dc, &ls1b_clk_pll.hw, CLK_PLL_DIV, + 26, 4, NULL, 12, 0, CLK_DIVIDER_ONE_BASED); +static LS1X_CLK_DIV(ls1b_clk_ahb, &ls1b_clk_pll.hw, CLK_PLL_DIV, + 14, 4, NULL, 10, 0, CLK_DIVIDER_ONE_BASED); +static CLK_FIXED_FACTOR(ls1b_clk_apb, "ls1b_clk_apb", "ls1b_clk_ahb", 2, 1, + CLK_SET_RATE_PARENT); + +static struct clk_hw_onecell_data ls1b_clk_hw_data = { + .hws = { + [LS1X_CLKID_PLL] = &ls1b_clk_pll.hw, + [LS1X_CLKID_CPU] = &ls1b_clk_cpu.hw, + [LS1X_CLKID_DC] = &ls1b_clk_dc.hw, + [LS1X_CLKID_AHB] = &ls1b_clk_ahb.hw, + [LS1X_CLKID_APB] = &ls1b_clk_apb.hw, + [CLK_NR_CLKS] = NULL, + }, + .num = CLK_NR_CLKS, +}; + +static const struct clk_div_table ls1c_ahb_div_table[] = { + [0] = { .val = 0, .div = 2 }, + [1] = { .val = 1, .div = 4 }, + [2] = { .val = 2, .div = 3 }, + [3] = { .val = 3, .div = 3 }, + [4] = { /* sentinel */ } +}; + +static LS1X_CLK_PLL(ls1c_clk_pll, CLK_PLL_FREQ, 0, 2, 8, 8, 16, 8); +static LS1X_CLK_DIV(ls1c_clk_cpu, &ls1c_clk_pll.hw, CLK_PLL_DIV, + 8, 7, NULL, 0, 1, + CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ROUND_CLOSEST); +static LS1X_CLK_DIV(ls1c_clk_dc, &ls1c_clk_pll.hw, CLK_PLL_DIV, + 24, 7, NULL, 4, 1, CLK_DIVIDER_ONE_BASED); +static LS1X_CLK_DIV(ls1c_clk_ahb, &ls1c_clk_cpu.hw, CLK_PLL_FREQ, + 0, 2, ls1c_ahb_div_table, 0, 0, CLK_DIVIDER_ALLOW_ZERO); +static CLK_FIXED_FACTOR(ls1c_clk_apb, "ls1c_clk_apb", "ls1c_clk_ahb", 1, 1, + CLK_SET_RATE_PARENT); + +static struct clk_hw_onecell_data ls1c_clk_hw_data = { + .hws = { + [LS1X_CLKID_PLL] = &ls1c_clk_pll.hw, + [LS1X_CLKID_CPU] = &ls1c_clk_cpu.hw, + [LS1X_CLKID_DC] = &ls1c_clk_dc.hw, + [LS1X_CLKID_AHB] = &ls1c_clk_ahb.hw, + [LS1X_CLKID_APB] = &ls1c_clk_apb.hw, + [CLK_NR_CLKS] = NULL, + }, + .num = CLK_NR_CLKS, +}; + +static void __init ls1x_clk_init(struct device_node *np, + struct clk_hw_onecell_data *hw_data) +{ + struct ls1x_clk *ls1x_clk; + void __iomem *reg; + int i, ret; + + reg = of_iomap(np, 0); + if (!reg) { + pr_err("Unable to map base for %pOF\n", np); + return; + } + + for (i = 0; i < CLK_NR_CLKS; i++) { + /* array might be sparse */ + if (!hw_data->hws[i]) + continue; + + if (i != LS1X_CLKID_APB) { + ls1x_clk = to_ls1x_clk(hw_data->hws[i]); + ls1x_clk->reg = reg + ls1x_clk->offset; + } + + ret = of_clk_hw_register(np, hw_data->hws[i]); + if (ret) + return; + } + + ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, hw_data); + if (ret) + pr_err("Failed to register %pOF\n", np); +} + +static void __init ls1b_clk_init(struct device_node *np) +{ + return ls1x_clk_init(np, &ls1b_clk_hw_data); +} + +static void __init ls1c_clk_init(struct device_node *np) +{ + return ls1x_clk_init(np, &ls1c_clk_hw_data); +} + +CLK_OF_DECLARE(ls1b_clk, "loongson,ls1b-clk", ls1b_clk_init); +CLK_OF_DECLARE(ls1c_clk, "loongson,ls1c-clk", ls1c_clk_init); + +MODULE_AUTHOR("Keguang Zhang <keguang.zhang@gmail.com>"); +MODULE_DESCRIPTION("Loongson1 clock driver");
Re-implement the clock driver for Loongson-1 to add devicetree support and fit into the clock framework. Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com> --- V2 -> V3: Add MODULE_AUTHOR and MODULE_DESCRIPTION info V1 -> V2: Implement one clock controller instead of single clocks (suggested by Krzysztof Kozlowski) --- drivers/clk/Makefile | 1 + drivers/clk/clk-loongson1.c | 301 ++++++++++++++++++++++++++++++++++++ 2 files changed, 302 insertions(+) create mode 100644 drivers/clk/clk-loongson1.c