| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 17 Oct 2018 12:51:35 -0700
From: Stephen Boyd <sboyd@...nel.org>
To: Abel Vesa <abel.vesa@....com>,
Andrey Smirnov <andrew.smirnov@...il.com>,
Anson Huang <anson.huang@....com>,
Dong Aisheng <aisheng.dong@....com>,
Fabio Estevam <fabio.estevam@....com>,
Lucas Stach <l.stach@...gutronix.de>,
Rob Herring <robh@...nel.org>,
Sascha Hauer <kernel@...gutronix.de>
Cc: linux-imx@....com, Abel Vesa <abelvesa@...ux.com>,
Abel Vesa <abel.vesa@....com>, Shawn Guo <shawnguo@...nel.org>,
Sascha Hauer <s.hauer@...gutronix.de>,
Michael Turquette <mturquette@...libre.com>,
open list <linux-kernel@...r.kernel.org>,
"moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE"
<linux-arm-kernel@...ts.infradead.org>,
"open list:COMMON CLK FRAMEWORK" <linux-clk@...r.kernel.org>
Subject: Re: [PATCH v9 4/5] clk: imx: add imx composite clock
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;
> +}
Powered by blists - more mailing lists