Re: [U-Boot] [PATCH V3 3/4] clk: imx: add i.MX8M composite clk support
> Subject: Re: [PATCH V3 3/4] clk: imx: add i.MX8M composite clk support
>
> On Fri, Aug 9, 2019 at 5:02 AM Peng Fan wrote:
>
> > + * The clk are not binded to a dev, because it is part of composite
> > + clk
>
> s/binded/bound
Fix in V4.
>
> > + * use composite clk to get dev
> > + */
> > +static ulong imx8m_clk_composite_divider_set_rate(struct clk *clk,
> > + unsigned long
> rate)
> > +{
> > + struct clk_divider *divider = (struct clk_divider
> *)to_clk_divider(clk);
> > + struct clk_composite *composite = (struct clk_composite
> *)clk->data;
> > + ulong parent_rate = clk_get_parent_rate(>clk);
> > + int prediv_value;
> > + int div_value;
> > + int ret;
> > + u32 val;
> > +
> > + ret = imx8m_clk_composite_compute_dividers(rate, parent_rate,
> > +
> _value, _value);
> > + if (ret)
> > + return -EINVAL;
>
> What about returning ret directly instead?
As wrote in commit log. The code was imported from Linux,
I would not modify this.
>
>
> > +struct clk *imx8m_clk_composite_flags(const char *name,
> > + const char * const
> *parent_names,
> > + int num_parents, void
> __iomem *reg,
> > + unsigned long flags) {
> > + struct clk *clk = ERR_PTR(-ENOMEM);
> > + struct clk_divider *div = NULL;
> > + struct clk_gate *gate = NULL;
> > + struct clk_mux *mux = NULL;
>
> Why all these NULL assignments?
Explained above.
>
> > +
> > + mux = kzalloc(sizeof(*mux), GFP_KERNEL);
> > + if (!mux)
> > + goto fail;
>
> It would be better like this:
Ditto
>
>mux = kzalloc(sizeof(*mux), GFP_KERNEL);
>if (!mux)
>return ERR_PTR(-ENOMEM);
>
> > +
> > + mux->reg = reg;
> > + mux->shift = PCG_PCS_SHIFT;
> > + mux->mask = PCG_PCS_MASK;
> > + mux->num_parents = num_parents;
> > + mux->flags = flags;
> > + mux->parent_names = parent_names;
> > +
> > + div = kzalloc(sizeof(*div), GFP_KERNEL);
> > + if (!div)
> > + goto fail;
> > +
> > + div->reg = reg;
> > + div->shift = PCG_PREDIV_SHIFT;
> > + div->width = PCG_PREDIV_WIDTH;
> > + div->flags = CLK_DIVIDER_ROUND_CLOSEST | flags;
> > +
> > + gate = kzalloc(sizeof(*gate), GFP_KERNEL);
> > + if (!gate)
> > + goto fail;
> > +
> > + gate->reg = reg;
> > + gate->bit_idx = PCG_CGC_SHIFT;
> > + gate->flags = flags;
> > +
> > + clk = clk_register_composite(NULL, name,
> > +parent_names, num_parents,
> > +>clk, _mux_ops,
> >clk,
> > +
> _clk_composite_divider_ops,
> > +>clk, _gate_ops,
> flags);
> > + if (IS_ERR(clk))
> > + goto fail;
> > +
> > + return clk;
> > +
> > +fail:
> > + kfree(gate);
> > + kfree(div);
> > + kfree(mux);
> > + return ERR_CAST(clk);
>
> ERR_CAST(clk) is only valid when for the IS_ERR(clk) path.
>
> Please rework the error handling.
Ditto.
Thanks,
Peng.
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH V3 3/4] clk: imx: add i.MX8M composite clk support
On Fri, Aug 9, 2019 at 5:02 AM Peng Fan wrote:
> + * The clk are not binded to a dev, because it is part of composite clk
s/binded/bound
> + * use composite clk to get dev
> + */
> +static ulong imx8m_clk_composite_divider_set_rate(struct clk *clk,
> + unsigned long rate)
> +{
> + struct clk_divider *divider = (struct clk_divider
> *)to_clk_divider(clk);
> + struct clk_composite *composite = (struct clk_composite *)clk->data;
> + ulong parent_rate = clk_get_parent_rate(>clk);
> + int prediv_value;
> + int div_value;
> + int ret;
> + u32 val;
> +
> + ret = imx8m_clk_composite_compute_dividers(rate, parent_rate,
> + _value, _value);
> + if (ret)
> + return -EINVAL;
What about returning ret directly instead?
> +struct clk *imx8m_clk_composite_flags(const char *name,
> + const char * const *parent_names,
> + int num_parents, void __iomem *reg,
> + unsigned long flags)
> +{
> + struct clk *clk = ERR_PTR(-ENOMEM);
> + struct clk_divider *div = NULL;
> + struct clk_gate *gate = NULL;
> + struct clk_mux *mux = NULL;
Why all these NULL assignments?
> +
> + mux = kzalloc(sizeof(*mux), GFP_KERNEL);
> + if (!mux)
> + goto fail;
It would be better like this:
mux = kzalloc(sizeof(*mux), GFP_KERNEL);
if (!mux)
return ERR_PTR(-ENOMEM);
> +
> + mux->reg = reg;
> + mux->shift = PCG_PCS_SHIFT;
> + mux->mask = PCG_PCS_MASK;
> + mux->num_parents = num_parents;
> + mux->flags = flags;
> + mux->parent_names = parent_names;
> +
> + div = kzalloc(sizeof(*div), GFP_KERNEL);
> + if (!div)
> + goto fail;
> +
> + div->reg = reg;
> + div->shift = PCG_PREDIV_SHIFT;
> + div->width = PCG_PREDIV_WIDTH;
> + div->flags = CLK_DIVIDER_ROUND_CLOSEST | flags;
> +
> + gate = kzalloc(sizeof(*gate), GFP_KERNEL);
> + if (!gate)
> + goto fail;
> +
> + gate->reg = reg;
> + gate->bit_idx = PCG_CGC_SHIFT;
> + gate->flags = flags;
> +
> + clk = clk_register_composite(NULL, name,
> +parent_names, num_parents,
> +>clk, _mux_ops, >clk,
> +_clk_composite_divider_ops,
> +>clk, _gate_ops, flags);
> + if (IS_ERR(clk))
> + goto fail;
> +
> + return clk;
> +
> +fail:
> + kfree(gate);
> + kfree(div);
> + kfree(mux);
> + return ERR_CAST(clk);
ERR_CAST(clk) is only valid when for the IS_ERR(clk) path.
Please rework the error handling.
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH V3 3/4] clk: imx: add i.MX8M composite clk support
> Import i.MX8M composite clk from Linux Kernel 5.3.0-rc2
>
> Signed-off-by: Peng Fan
> ---
> drivers/clk/imx/clk-composite-8m.c | 170
> +
> 1 file changed, 170 insertions(+)
> create mode 100644 drivers/clk/imx/clk-composite-8m.c
>
> diff --git a/drivers/clk/imx/clk-composite-8m.c
> b/drivers/clk/imx/clk-composite-8m.c
> new file mode 100644
> index 00..57ebbc3bb0
> --- /dev/null
> +++ b/drivers/clk/imx/clk-composite-8m.c
> @@ -0,0 +1,170 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2019 NXP
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include "clk.h"
> +
> +#define UBOOT_DM_CLK_IMX_COMPOSITE "imx_clk_composite"
> +
> +#define PCG_PREDIV_SHIFT 16
> +#define PCG_PREDIV_WIDTH 3
> +#define PCG_PREDIV_MAX 8
> +
> +#define PCG_DIV_SHIFT0
> +#define PCG_DIV_WIDTH6
> +#define PCG_DIV_MAX 64
> +
> +#define PCG_PCS_SHIFT24
> +#define PCG_PCS_MASK 0x7
> +
> +#define PCG_CGC_SHIFT28
> +
> +static unsigned long imx8m_clk_composite_divider_recalc_rate(struct clk *clk)
> +{
> + struct clk_divider *divider = (struct clk_divider *)to_clk_divider(clk);
> + struct clk_composite *composite = (struct clk_composite *)clk->data;
> + ulong parent_rate = clk_get_parent_rate(>clk);
> + unsigned long prediv_rate;
> + unsigned int prediv_value;
> + unsigned int div_value;
> +
> + debug("%s: name %s prate: %lu reg: %p\n", __func__,
> + (>clk)->dev->name, parent_rate, divider->reg);
> + prediv_value = readl(divider->reg) >> divider->shift;
> + prediv_value &= clk_div_mask(divider->width);
> +
> + prediv_rate = divider_recalc_rate(clk, parent_rate, prediv_value,
> + NULL, divider->flags,
> + divider->width);
> +
> + div_value = readl(divider->reg) >> PCG_DIV_SHIFT;
> + div_value &= clk_div_mask(PCG_DIV_WIDTH);
> +
> + return divider_recalc_rate(clk, prediv_rate, div_value, NULL,
> +divider->flags, PCG_DIV_WIDTH);
> +}
> +
> +static int imx8m_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;
> +
> + *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;
> +}
> +
> +/*
> + * The clk are not binded to a dev, because it is part of composite clk
> + * use composite clk to get dev
> + */
> +static ulong imx8m_clk_composite_divider_set_rate(struct clk *clk,
> + unsigned long rate)
> +{
> + struct clk_divider *divider = (struct clk_divider *)to_clk_divider(clk);
> + struct clk_composite *composite = (struct clk_composite *)clk->data;
> + ulong parent_rate = clk_get_parent_rate(>clk);
> + int prediv_value;
> + int div_value;
> + int ret;
> + u32 val;
> +
> + ret = imx8m_clk_composite_compute_dividers(rate, parent_rate,
> +_value, _value);
> + if (ret)
> + return -EINVAL;
> +
> + val = 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;
> + writel(val, divider->reg);
> +
> + return clk_get_rate(>clk);
> +}
> +
> +static const struct clk_ops imx8m_clk_composite_divider_ops = {
> + .get_rate = imx8m_clk_composite_divider_recalc_rate,
> + .set_rate = imx8m_clk_composite_divider_set_rate,
> +};
> +
> +struct clk *imx8m_clk_composite_flags(const char *name,
> + const char * const *parent_names,
> + int num_parents, void __iomem *reg,
> + unsigned long flags)
> +{
> + struct clk *clk = ERR_PTR(-ENOMEM);
> + struct clk_divider *div = NULL;
> + struct clk_gate *gate = NULL;
> + struct clk_mux *mux = NULL;
> +
> + mux = kzalloc(sizeof(*mux), GFP_KERNEL);
> + if (!mux)
> + goto fail;
> +
> + mux->reg =
[U-Boot] [PATCH V3 3/4] clk: imx: add i.MX8M composite clk support
Import i.MX8M composite clk from Linux Kernel 5.3.0-rc2
Signed-off-by: Peng Fan
---
drivers/clk/imx/clk-composite-8m.c | 170 +
1 file changed, 170 insertions(+)
create mode 100644 drivers/clk/imx/clk-composite-8m.c
diff --git a/drivers/clk/imx/clk-composite-8m.c
b/drivers/clk/imx/clk-composite-8m.c
new file mode 100644
index 00..57ebbc3bb0
--- /dev/null
+++ b/drivers/clk/imx/clk-composite-8m.c
@@ -0,0 +1,170 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019 NXP
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include "clk.h"
+
+#define UBOOT_DM_CLK_IMX_COMPOSITE "imx_clk_composite"
+
+#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_MAX64
+
+#define PCG_PCS_SHIFT 24
+#define PCG_PCS_MASK 0x7
+
+#define PCG_CGC_SHIFT 28
+
+static unsigned long imx8m_clk_composite_divider_recalc_rate(struct clk *clk)
+{
+ struct clk_divider *divider = (struct clk_divider *)to_clk_divider(clk);
+ struct clk_composite *composite = (struct clk_composite *)clk->data;
+ ulong parent_rate = clk_get_parent_rate(>clk);
+ unsigned long prediv_rate;
+ unsigned int prediv_value;
+ unsigned int div_value;
+
+ debug("%s: name %s prate: %lu reg: %p\n", __func__,
+ (>clk)->dev->name, parent_rate, divider->reg);
+ prediv_value = readl(divider->reg) >> divider->shift;
+ prediv_value &= clk_div_mask(divider->width);
+
+ prediv_rate = divider_recalc_rate(clk, parent_rate, prediv_value,
+ NULL, divider->flags,
+ divider->width);
+
+ div_value = readl(divider->reg) >> PCG_DIV_SHIFT;
+ div_value &= clk_div_mask(PCG_DIV_WIDTH);
+
+ return divider_recalc_rate(clk, prediv_rate, div_value, NULL,
+ divider->flags, PCG_DIV_WIDTH);
+}
+
+static int imx8m_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;
+
+ *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;
+}
+
+/*
+ * The clk are not binded to a dev, because it is part of composite clk
+ * use composite clk to get dev
+ */
+static ulong imx8m_clk_composite_divider_set_rate(struct clk *clk,
+ unsigned long rate)
+{
+ struct clk_divider *divider = (struct clk_divider *)to_clk_divider(clk);
+ struct clk_composite *composite = (struct clk_composite *)clk->data;
+ ulong parent_rate = clk_get_parent_rate(>clk);
+ int prediv_value;
+ int div_value;
+ int ret;
+ u32 val;
+
+ ret = imx8m_clk_composite_compute_dividers(rate, parent_rate,
+ _value, _value);
+ if (ret)
+ return -EINVAL;
+
+ val = 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;
+ writel(val, divider->reg);
+
+ return clk_get_rate(>clk);
+}
+
+static const struct clk_ops imx8m_clk_composite_divider_ops = {
+ .get_rate = imx8m_clk_composite_divider_recalc_rate,
+ .set_rate = imx8m_clk_composite_divider_set_rate,
+};
+
+struct clk *imx8m_clk_composite_flags(const char *name,
+ const char * const *parent_names,
+ int num_parents, void __iomem *reg,
+ unsigned long flags)
+{
+ struct clk *clk = ERR_PTR(-ENOMEM);
+ struct clk_divider *div = NULL;
+ struct clk_gate *gate = NULL;
+ struct clk_mux *mux = NULL;
+
+ mux = kzalloc(sizeof(*mux), GFP_KERNEL);
+ if (!mux)
+ goto fail;
+
+ mux->reg = reg;
+ mux->shift = PCG_PCS_SHIFT;
+ mux->mask = PCG_PCS_MASK;
+ mux->num_parents = num_parents;
+ mux->flags = flags;
+ mux->parent_names =
