Since a lot of clocks on imx8 are formed by a mux, gate, predivider and
divider, the idea here is to combine all of those into one composite clock,
but we need to deal with both predivider and divider at the same time and
therefore we add the imx_clk_composite_divider_ops and register
the composite clock with those.
Signed-off-by: Abel Vesa <[email protected]>
Suggested-by: Sascha Hauer <[email protected]>
---
drivers/clk/imx/Makefile | 1 +
drivers/clk/imx/clk-composite.c | 181 ++++++++++++++++++++++++++++++++++++++++
drivers/clk/imx/clk.h | 14 ++++
3 files changed, 196 insertions(+)
create mode 100644 drivers/clk/imx/clk-composite.c
diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile
index b87513c..4fabb0a 100644
--- a/drivers/clk/imx/Makefile
+++ b/drivers/clk/imx/Makefile
@@ -3,6 +3,7 @@
obj-y += \
clk.o \
clk-busy.o \
+ clk-composite.o \
clk-cpu.o \
clk-fixup-div.o \
clk-fixup-mux.o \
diff --git a/drivers/clk/imx/clk-composite.c b/drivers/clk/imx/clk-composite.c
new file mode 100644
index 0000000..4b03107
--- /dev/null
+++ b/drivers/clk/imx/clk-composite.c
@@ -0,0 +1,181 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2018 NXP
+ */
+
+#include <linux/errno.h>
+#include <linux/slab.h>
+#include <linux/clk-provider.h>
+#include <linux/clk.h>
+
+#include "clk.h"
+
+#define PCG_PREDIV_SHIFT 16
+#define PCG_PREDIV_WIDTH 3
+#define PCG_PREDIV_MAX 8
+
+#define PCG_DIV_SHIFT 0
+#define PCG_DIV_WIDTH 6
+#define PCG_DIV_MAX 64
+
+#define PCG_PCS_SHIFT 24
+#define PCG_PCS_MASK 0x7
+
+#define PCG_CGC_SHIFT 28
+
+static unsigned long imx_clk_composite_divider_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct clk_divider *divider = to_clk_divider(hw);
+ unsigned long prediv_rate;
+ unsigned int prediv_value;
+ unsigned int div_value;
+
+ prediv_value = clk_readl(divider->reg) >> divider->shift;
+ prediv_value &= clk_div_mask(divider->width);
+
+ prediv_rate = divider_recalc_rate(hw, parent_rate, prediv_value,
+ NULL, divider->flags,
+ divider->width);
+
+ div_value = clk_readl(divider->reg) >> PCG_DIV_SHIFT;
+ div_value &= clk_div_mask(PCG_DIV_WIDTH);
+
+ return divider_recalc_rate(hw, prediv_rate, div_value, NULL,
+ divider->flags, PCG_DIV_WIDTH);
+}
+
+static int imx_clk_composite_compute_dividers(unsigned long rate,
+ unsigned long parent_rate,
+ int *prediv, int *postdiv)
+{
+ int div1, div2;
+ int error = INT_MAX;
+ int ret = -EINVAL;
+
+ /* default values */
+ *prediv = 1;
+ *postdiv = 1;
+
+ for (div1 = 1; div1 <= PCG_PREDIV_MAX; div1++) {
+ for (div2 = 1; div2 <= PCG_DIV_MAX; div2++) {
+ int new_error = ((parent_rate / div1) / div2) - rate;
+
+ if (abs(new_error) < abs(error)) {
+ *prediv = div1;
+ *postdiv = div2;
+ error = new_error;
+ ret = 0;
+ }
+ }
+ }
+ return ret;
+}
+
+static long imx_clk_composite_divider_round_rate(struct clk_hw *hw,
+ unsigned long rate,
+ unsigned long *prate)
+{
+ int prediv_value;
+ int div_value;
+
+ imx_clk_composite_compute_dividers(rate, *prate,
+ &prediv_value, &div_value);
+
+ rate = DIV_ROUND_UP_ULL((u64)*prate, prediv_value);
+ rate = DIV_ROUND_UP_ULL((u64)rate, div_value);
+
+ return rate;
+}
+
+static int imx_clk_composite_divider_set_rate(struct clk_hw *hw,
+ unsigned long rate,
+ unsigned long parent_rate)
+{
+ struct clk_divider *divider = to_clk_divider(hw);
+ unsigned long flags = 0;
+ int prediv_value;
+ int div_value;
+ int ret = 0;
+ u32 val;
+
+ ret = imx_clk_composite_compute_dividers(rate, parent_rate,
+ &prediv_value, &div_value);
+ if (ret)
+ return -EINVAL;
+
+ spin_lock_irqsave(divider->lock, flags);
+
+ val = clk_readl(divider->reg);
+ val &= ~((clk_div_mask(divider->width) << divider->shift) |
+ (clk_div_mask(PCG_DIV_WIDTH) << PCG_DIV_SHIFT));
+
+ val |= (u32)(prediv_value - 1) << divider->shift;
+ val |= (u32)(div_value - 1) << PCG_DIV_SHIFT;
+ clk_writel(val, divider->reg);
+
+ spin_unlock_irqrestore(divider->lock, flags);
+
+ return ret;
+}
+
+static const struct clk_ops imx_clk_composite_divider_ops = {
+ .recalc_rate = imx_clk_composite_divider_recalc_rate,
+ .round_rate = imx_clk_composite_divider_round_rate,
+ .set_rate = imx_clk_composite_divider_set_rate,
+};
+
+struct clk *imx_clk_composite_flags(const char *name,
+ const char **parent_names,
+ int num_parents, void __iomem *reg,
+ unsigned long flags)
+{
+ struct clk_hw *mux_hw = NULL, *div_hw = NULL, *gate_hw = NULL;
+ struct clk_divider *div = NULL;
+ struct clk_gate *gate = NULL;
+ struct clk_mux *mux = NULL;
+ struct clk *clk = ERR_PTR(-ENOMEM);
+
+ mux = kzalloc(sizeof(*mux), GFP_KERNEL);
+ if (!mux)
+ goto fail;
+
+ mux_hw = &mux->hw;
+ mux->reg = reg;
+ mux->shift = PCG_PCS_SHIFT;
+ mux->mask = PCG_PCS_MASK;
+
+ div = kzalloc(sizeof(*div), GFP_KERNEL);
+ if (!div)
+ goto fail;
+
+ div_hw = &div->hw;
+ div->reg = reg;
+ div->shift = PCG_PREDIV_SHIFT;
+ div->width = PCG_PREDIV_WIDTH;
+ div->lock = &imx_ccm_lock;
+ div->flags = CLK_DIVIDER_ROUND_CLOSEST;
+
+ gate = kzalloc(sizeof(*gate), GFP_KERNEL);
+ if (!gate)
+ goto fail;
+
+ gate_hw = &gate->hw;
+ gate->reg = reg;
+ gate->bit_idx = PCG_CGC_SHIFT;
+
+ clk = clk_register_composite(NULL, name, parent_names, num_parents,
+ mux_hw, &clk_mux_ops, div_hw,
+ &imx_clk_composite_divider_ops, gate_hw,
+ &clk_gate_ops, flags);
+ if (IS_ERR(clk))
+ goto fail;
+
+ return clk;
+
+fail:
+ kfree(gate);
+ kfree(div);
+ kfree(mux);
+ return clk;
+}
diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
index 12b3fd6..9d7c9c8 100644
--- a/drivers/clk/imx/clk.h
+++ b/drivers/clk/imx/clk.h
@@ -232,4 +232,18 @@ struct clk *imx_clk_cpu(const char *name, const char *parent_name,
struct clk *div, struct clk *mux, struct clk *pll,
struct clk *step);
+struct clk *imx_clk_composite_flags(const char *name, const char **parent_names,
+ int num_parents, void __iomem *reg, unsigned long flags);
+
+#define __imx_clk_composite(name, parent_names, reg, flags) \
+ imx_clk_composite_flags(name, parent_names, \
+ ARRAY_SIZE(parent_names), reg, \
+ flags | CLK_SET_RATE_NO_REPARENT | CLK_OPS_PARENT_ENABLE)
+
+#define imx_clk_composite(name, parent_names, reg) \
+ __imx_clk_composite(name, parent_names, reg, 0)
+
+#define imx_clk_composite_critical(name, parent_names, reg) \
+ __imx_clk_composite(name, parent_names, reg, CLK_IS_CRITICAL)
+
#endif
--
2.7.4
Hi Abel,
On Mon, Sep 24, 2018 at 7:39 AM, Abel Vesa <[email protected]> wrote:
> +static long imx_clk_composite_divider_round_rate(struct clk_hw *hw,
> + unsigned long rate,
> + unsigned long *prate)
> +{
> + int prediv_value;
> + int div_value;
> +
> + imx_clk_composite_compute_dividers(rate, *prate,
> + &prediv_value, &div_value);
> +
> + rate = DIV_ROUND_UP_ULL((u64)*prate, prediv_value);
You assing a value to 'rate' here.
> + rate = DIV_ROUND_UP_ULL((u64)rate, div_value);
And then overwrite it immediately after.
Is this really the intended behavior?
On Tue, Sep 25, 2018 at 01:42:12PM -0300, Fabio Estevam wrote:
> Hi Abel,
>
> On Mon, Sep 24, 2018 at 7:39 AM, Abel Vesa <[email protected]> wrote:
>
> > +static long imx_clk_composite_divider_round_rate(struct clk_hw *hw,
> > + unsigned long rate,
> > + unsigned long *prate)
> > +{
> > + int prediv_value;
> > + int div_value;
> > +
> > + imx_clk_composite_compute_dividers(rate, *prate,
> > + &prediv_value, &div_value);
> > +
> > + rate = DIV_ROUND_UP_ULL((u64)*prate, prediv_value);
>
> You assing a value to 'rate' here.
>
> > + rate = DIV_ROUND_UP_ULL((u64)rate, div_value);
>
> And then overwrite it immediately after.
It's:
rate = *prate / prediv_value;
rate = rate / div_value;
To me this looks correct. However, For an unsigned long type we have
DIV_ROUND_UP() with which we do not need any casting. For 64bit code
unsigned long is 64bit anyway which makes the cast a no-op and for 32bit
code there's also no point in exanding the initial 32bit value to 64bit.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Hi Sascha,
On Wed, Sep 26, 2018 at 3:47 AM, Sascha Hauer <[email protected]> wrote:
> It's:
> rate = *prate / prediv_value;
> rate = rate / div_value;
Yes, this looks correct. Thanks for the feedback.
> To me this looks correct. However, For an unsigned long type we have
> DIV_ROUND_UP() with which we do not need any casting. For 64bit code
> unsigned long is 64bit anyway which makes the cast a no-op and for 32bit
> code there's also no point in exanding the initial 32bit value to 64bit.
That's a good point too.
Thanks
Quoting Abel Vesa (2018-09-24 03:39:56)
> diff --git a/drivers/clk/imx/clk-composite.c b/drivers/clk/imx/clk-composite.c
> new file mode 100644
> index 0000000..4b03107
> --- /dev/null
> +++ b/drivers/clk/imx/clk-composite.c
> @@ -0,0 +1,181 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2018 NXP
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/slab.h>
> +#include <linux/clk-provider.h>
> +#include <linux/clk.h>
Is this include used?
> +
> +#include "clk.h"
> +
> +#define PCG_PREDIV_SHIFT 16
> +#define PCG_PREDIV_WIDTH 3
> +#define PCG_PREDIV_MAX 8
> +
> +#define PCG_DIV_SHIFT 0
> +#define PCG_DIV_WIDTH 6
> +#define PCG_DIV_MAX 64
> +
> +#define PCG_PCS_SHIFT 24
> +#define PCG_PCS_MASK 0x7
> +
> +#define PCG_CGC_SHIFT 28
> +
> +static unsigned long imx_clk_composite_divider_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct clk_divider *divider = to_clk_divider(hw);
> + unsigned long prediv_rate;
> + unsigned int prediv_value;
> + unsigned int div_value;
> +
> + prediv_value = clk_readl(divider->reg) >> divider->shift;
> + prediv_value &= clk_div_mask(divider->width);
> +
> + prediv_rate = divider_recalc_rate(hw, parent_rate, prediv_value,
> + NULL, divider->flags,
> + divider->width);
> +
> + div_value = clk_readl(divider->reg) >> PCG_DIV_SHIFT;
> + div_value &= clk_div_mask(PCG_DIV_WIDTH);
> +
> + return divider_recalc_rate(hw, prediv_rate, div_value, NULL,
> + divider->flags, PCG_DIV_WIDTH);
> +}
> +
> +static int imx_clk_composite_compute_dividers(unsigned long rate,
> + unsigned long parent_rate,
> + int *prediv, int *postdiv)
> +{
> + int div1, div2;
> + int error = INT_MAX;
> + int ret = -EINVAL;
> +
> + /* default values */
> + *prediv = 1;
> + *postdiv = 1;
> +
> + for (div1 = 1; div1 <= PCG_PREDIV_MAX; div1++) {
> + for (div2 = 1; div2 <= PCG_DIV_MAX; div2++) {
> + int new_error = ((parent_rate / div1) / div2) - rate;
> +
> + if (abs(new_error) < abs(error)) {
> + *prediv = div1;
> + *postdiv = div2;
> + error = new_error;
> + ret = 0;
> + }
> + }
> + }
> + return ret;
> +}
> +
> +static long imx_clk_composite_divider_round_rate(struct clk_hw *hw,
> + unsigned long rate,
> + unsigned long *prate)
> +{
> + int prediv_value;
> + int div_value;
> +
> + imx_clk_composite_compute_dividers(rate, *prate,
> + &prediv_value, &div_value);
> +
> + rate = DIV_ROUND_UP_ULL((u64)*prate, prediv_value);
> + rate = DIV_ROUND_UP_ULL((u64)rate, div_value);
> +
> + return rate;
Looks the same as another patch, maybe it is?
Anyway, same nitpick about returning the DIV_ROUND_UP_ULL() result.
> +}
> +
> +static int imx_clk_composite_divider_set_rate(struct clk_hw *hw,
> + unsigned long rate,
> + unsigned long parent_rate)
> +{
> + struct clk_divider *divider = to_clk_divider(hw);
> + unsigned long flags = 0;
> + int prediv_value;
> + int div_value;
> + int ret = 0;
> + u32 val;
> +
> + ret = imx_clk_composite_compute_dividers(rate, parent_rate,
> + &prediv_value, &div_value);
> + if (ret)
> + return -EINVAL;
> +
> + spin_lock_irqsave(divider->lock, flags);
> +
> + val = clk_readl(divider->reg);
> + val &= ~((clk_div_mask(divider->width) << divider->shift) |
> + (clk_div_mask(PCG_DIV_WIDTH) << PCG_DIV_SHIFT));
> +
> + val |= (u32)(prediv_value - 1) << divider->shift;
> + val |= (u32)(div_value - 1) << PCG_DIV_SHIFT;
> + clk_writel(val, divider->reg);
Please don't use clk_writel().
> +
> + spin_unlock_irqrestore(divider->lock, flags);
> +
> + return ret;
> +}
> +
> +static const struct clk_ops imx_clk_composite_divider_ops = {
> + .recalc_rate = imx_clk_composite_divider_recalc_rate,
> + .round_rate = imx_clk_composite_divider_round_rate,
> + .set_rate = imx_clk_composite_divider_set_rate,
> +};
> +
> +struct clk *imx_clk_composite_flags(const char *name,
> + const char **parent_names,
> + int num_parents, void __iomem *reg,
> + unsigned long flags)
> +{
> + struct clk_hw *mux_hw = NULL, *div_hw = NULL, *gate_hw = NULL;
> + struct clk_divider *div = NULL;
> + struct clk_gate *gate = NULL;
> + struct clk_mux *mux = NULL;
> + struct clk *clk = ERR_PTR(-ENOMEM);
> +
> + mux = kzalloc(sizeof(*mux), GFP_KERNEL);
> + if (!mux)
> + goto fail;
> +
> + mux_hw = &mux->hw;
> + mux->reg = reg;
> + mux->shift = PCG_PCS_SHIFT;
> + mux->mask = PCG_PCS_MASK;
> +
> + div = kzalloc(sizeof(*div), GFP_KERNEL);
> + if (!div)
> + goto fail;
> +
> + div_hw = &div->hw;
> + div->reg = reg;
> + div->shift = PCG_PREDIV_SHIFT;
> + div->width = PCG_PREDIV_WIDTH;
> + div->lock = &imx_ccm_lock;
> + div->flags = CLK_DIVIDER_ROUND_CLOSEST;
> +
> + gate = kzalloc(sizeof(*gate), GFP_KERNEL);
> + if (!gate)
> + goto fail;
> +
> + gate_hw = &gate->hw;
> + gate->reg = reg;
> + gate->bit_idx = PCG_CGC_SHIFT;
> +
> + clk = clk_register_composite(NULL, name, parent_names, num_parents,
> + mux_hw, &clk_mux_ops, div_hw,
> + &imx_clk_composite_divider_ops, gate_hw,
> + &clk_gate_ops, flags);
Didn't I already review this? I'd prefer we move this to using clk_hw
based APIs and then return the clk pointer if needed.
> + if (IS_ERR(clk))
> + goto fail;
> +
> + return clk;
> +
> +fail:
> + kfree(gate);
> + kfree(div);
> + kfree(mux);
> + return clk;
> +}
On Wed, Oct 17, 2018 at 12:51:35PM -0700, Stephen Boyd wrote:
> Quoting Abel Vesa (2018-09-24 03:39:56)
> > + clk = clk_register_composite(NULL, name, parent_names, num_parents,
> > + mux_hw, &clk_mux_ops, div_hw,
> > + &imx_clk_composite_divider_ops, gate_hw,
> > + &clk_gate_ops, flags);
>
> Didn't I already review this? I'd prefer we move this to using clk_hw
> based APIs and then return the clk pointer if needed.
>
Yes, you reviewed the v11, so you can ignore this 9th version.
I'll implement all the comments from you in the 12th version before sending.
I'll also switch all clk based register functions call sites
to clk_hw based ones.
Thanks
> > + if (IS_ERR(clk))
> > + goto fail;
> > +
> > + return clk;
> > +
> > +fail:
> > + kfree(gate);
> > + kfree(div);
> > + kfree(mux);
> > + return clk;
> > +}
--