From: Stephen Boyd <sboyd@kernel.org>
To: Abel Vesa <abel.vesa@nxp.com>,
Dong Aisheng <aisheng.dong@nxp.com>,
Lucas Stach <l.stach@pengutronix.de>,
Sascha Hauer <kernel@pengutronix.de>
Cc: Michael Turquette <mturquette@baylibre.com>,
Rob Herring <robh@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Shawn Guo <shawnguo@kernel.org>,
Fabio Estevam <fabio.estevam@nxp.com>,
linux-imx@nxp.com, linux-clk@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
Abel Vesa <abelvesa@linux.com>, Abel Vesa <abel.vesa@nxp.com>
Subject: Re: [PATCH v11 4/5] clk: imx: add imx composite clock
Date: Wed, 17 Oct 2018 11:02:15 -0700 [thread overview]
Message-ID: <153979933548.5275.10155260505209370069@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <1539157520-25081-5-git-send-email-abel.vesa@nxp.com>
Quoting Abel Vesa (2018-10-10 00:45:19)
> diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
> new file mode 100644
> index 0000000..e214812
> --- /dev/null
> +++ b/drivers/clk/imx/clk-composite-8m.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? Same comment applies to all the building block
patches in this series.
> +
> +#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 imx8m_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 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;
> +
> + /* default values */
Nitpick alert: Useless comment?
> + *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 imx8m_clk_composite_divider_round_rate(struct clk_hw *hw,
> + unsigned long rate,
> + unsigned long *prate)
> +{
> + int prediv_value;
> + int div_value;
> +
> + imx8m_clk_composite_compute_dividers(rate, *prate,
> + &prediv_value, &div_value);
> +
> + rate = DIV_ROUND_UP(*prate, prediv_value);
> + rate = DIV_ROUND_UP(rate, div_value);
> +
> + return rate;
So just
return DIV_ROUND_UP(rate, div_value);
> +}
> +
> +static int imx8m_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 = imx8m_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_readl and clk_writel() unless you absolutely must
do so.
> +
> + spin_unlock_irqrestore(divider->lock, flags);
> +
> + return ret;
> +}
> +
> +static const struct clk_ops imx8m_clk_composite_divider_ops = {
> + .recalc_rate = imx8m_clk_composite_divider_recalc_rate,
> + .round_rate = imx8m_clk_composite_divider_round_rate,
> + .set_rate = imx8m_clk_composite_divider_set_rate,
> +};
> +
> +struct clk *imx8m_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,
> + &imx8m_clk_composite_divider_ops,
> + gate_hw, &clk_gate_ops, flags);
Can new imx driver code start using the clk_hw based registration
functions? If it's too hard to fix all call-sites, it's fine to pull the
struct clk pointer out of the hw structure and return that for now, but
eventually I'd like to get rid of clk based registration APIs. It's an
ongoing saga.
> + if (IS_ERR(clk))
> + goto fail;
> +
> + return clk;
> +
> +fail:
> + kfree(gate);
> + kfree(div);
> + kfree(mux);
> + return clk;
> +}
next prev parent reply other threads:[~2018-10-17 18:02 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-10 7:45 [PATCH v11 0/5] Add i.MX8MQ clock driver Abel Vesa
2018-10-10 7:45 ` [PATCH v11 1/5] dt-bindings: add binding for i.MX8MQ CCM Abel Vesa
2018-10-17 18:02 ` Stephen Boyd
2018-10-10 7:45 ` [PATCH v11 2/5] clk: imx: add fractional PLL output clock Abel Vesa
2018-10-10 7:45 ` [PATCH v11 3/5] clk: imx: add SCCG PLL type Abel Vesa
2018-10-10 7:45 ` [PATCH v11 4/5] clk: imx: add imx composite clock Abel Vesa
2018-10-17 18:02 ` Stephen Boyd [this message]
2018-10-10 7:45 ` [PATCH v11 5/5] clk: imx: add clock driver for i.MX8MQ CCM Abel Vesa
2018-10-17 17:55 ` Stephen Boyd
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=153979933548.5275.10155260505209370069@swboyd.mtv.corp.google.com \
--to=sboyd@kernel.org \
--cc=abel.vesa@nxp.com \
--cc=abelvesa@linux.com \
--cc=aisheng.dong@nxp.com \
--cc=devicetree@vger.kernel.org \
--cc=fabio.estevam@nxp.com \
--cc=kernel@pengutronix.de \
--cc=l.stach@pengutronix.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-imx@nxp.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mturquette@baylibre.com \
--cc=robh@kernel.org \
--cc=shawnguo@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).