Quoting Wen He (2019-08-12 03:01:03)
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 801fa1cd0321..0e6c7027d637 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -223,6 +223,15 @@ config CLK_QORIQ
> This adds the clock driver support for Freescale QorIQ platforms
> using common clock framework.
>
> +config CLK_PLLDIG
> + bool "Clock driver for LS1028A Display output"
> + depends on ARCH_LAYERSCAPE && OF
Does it actually depend on either of these to build? Probabl not, so
maybe just default ARCH_LAYERSCAPE && OF? Also, can your Kconfig
variable be named something more specific like CLK_LS1028A_PLLDIG?
> + help
> + This driver support the Display output interfaces(LCD, DPHY) pixel
> clocks
> + of the QorIQ Layerscape LS1028A, as implemented TSMC CLN28HPM PLL.
> Not all
> + features of the PLL are currently supported by the driver. By
> default,
> + configured bypass mode with this PLL.
> +
> config COMMON_CLK_XGENE
> bool "Clock driver for APM XGene SoC"
> default ARCH_XGENE
> diff --git a/drivers/clk/clk-plldig.c b/drivers/clk/clk-plldig.c
> new file mode 100644
> index 000000000000..15c9bb623a70
> --- /dev/null
> +++ b/drivers/clk/clk-plldig.c
> @@ -0,0 +1,277 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright 2019 NXP
> +
> +/*
> + * Clock driver for LS1028A Display output interfaces(LCD, DPHY).
> + *
> + * Author: Wen He <wen.h...@nxp.com>
> + *
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/clkdev.h>
PLease remove this unused include.
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
Only makes sense to include this if it's a platform device driver.
> +#include <linux/slab.h>
> +
[...]
> +
> +static inline int plldig_wait_lock(struct clk_plldig *plldig)
> +{
> + u32 csr;
> + /*
> + * Indicates whether PLL has acquired lock, if operating in bypass
> + * mode, the LOCK bit will still assert when the PLL acquires lock
> + * or negate when it loses lock.
> + */
> + return readl_poll_timeout(plldig->regs + PLLDIG_REG_PLLSR, csr,
> + csr & PLLDIG_LOCK_STATUS, 0, LOCK_TIMEOUT_US);
> +}
> +
> +static int plldig_enable(struct clk_hw *hw)
> +{
> + struct clk_plldig *data = to_clk_plldig(hw);
> + u32 val;
> +
> + val = readl(data->regs + PLLDIG_REG_PLLFM);
> + /*
> + * Use Bypass mode with PLL off by default,the frequency overshoot
Please add a space after comma,
> + * detector output was disable. SSCG Bypass mode should be enable.
> + */
> + val |= PLLDIG_SSCGBYP_ENABLE;
> + writel(val, data->regs + PLLDIG_REG_PLLFM);
> +
[...]
> +
> +static int plldig_is_enabled(struct clk_hw *hw)
> +{
> + struct clk_plldig *data = to_clk_plldig(hw);
> +
> + return (readl(data->regs + PLLDIG_REG_PLLFM) & PLLDIG_SSCGBYP_ENABLE);
Please remove extraneous parenthesis.
> +}
> +
> +/*
> + * Clock configuration relationship between the PHI1 frequency(fpll_phi) and
> + * the output frequency of the PLL is determined by the PLLDV, according to
> + * the following equation:
> + * pxclk = fpll_phi / RFDPHI1 = (pll_ref x PLLDV[MFD]) / PLLDV[RFDPHI1].
> + */
> +static bool plldig_is_valid_range(unsigned long rate, unsigned long
> parent_rate,
> + unsigned int *mult, unsigned int *rfdphi1,
> + unsigned long *round_rate_base)
> +{
> + u32 div, div_temp, mfd = PLLDIG_DEFAULE_MULT;
> + unsigned long round_rate;
> +
> + round_rate = parent_rate * mfd;
> +
> + /* Range of the diliver for driving the PHI1 output clock */
divider? Not diliver, right?
> + for (div = 1; div <= 63; div++) {
> + /* Checking match with default mult number at first */
> + if (round_rate / div == rate) {
> + *rfdphi1 = div;
> + *round_rate_base = round_rate;
> + *mult = mfd;
> + return true;
> + }
> + }
> +
> + for (div = 1; div <= 63; div++) {
> + mfd = (div * rate) / parent_rate;
> + /* Range of the muliplicationthe factor applied to the
/*
* Please make multi line comments look like this
*/
> + * output reference frequency
> + */
> + if ((mfd >= 10) && (mfd <= 150)) {
> + div_temp = (parent_rate * mfd) / rate;
> + if ((div_temp * rate) == (mfd * parent_rate)) {
> + *rfdphi1 = div_temp;
> + *mult = mfd;
> + *round_rate_base = mfd * parent_rate;
> + return true;
> + }
> + }
> + }
> +
> + return false;
> +}
> +
> +static unsigned long plldig_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct clk_plldig *plldig = to_clk_plldig(hw);
> + u32 mult, div, val;
> +
> + val = readl(plldig->regs + PLLDIG_REG_PLLDV);
> + pr_info("%s: current configuration: 0x%x\n", clk_hw_get_name(hw),
> val);
Remove debug prints please.
> +
> + /* Check if PLL is bypassed */
> + if (val & PLLDIG_SSCGBYP_ENABLE)
> + return parent_rate;
> +
> + /* Checkout multiplication factor divider value */
> + mult = val;
> + mult = PLLDIG_GET_MULT(mult);
> +
> + /* Checkout divider value of the output frequency */
> + div = val;
> + div = PLLDIG_GET_RFDPHI1(div);
> +
> + return (parent_rate * mult) / div;
> +}
> +
> +static long plldig_round_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long *parent)
> +{
> + unsigned long parent_rate = *parent;
> + unsigned long round_rate;
> + u32 mult = 0, rfdphi1 = 0;
> + bool found = false;
> +
> + found = plldig_is_valid_range(rate, parent_rate, &mult,
> + &rfdphi1, &round_rate);
> + if (!found) {
> + pr_warn("%s: unable to round rate %lu, parent rate :%lu\n",
> + clk_hw_get_name(hw), rate, parent_rate);
> + return 0;
> + }
> +
> + return round_rate / rfdphi1;
> +}
> +
> +static int plldig_set_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent_rate)
> +{
> + struct clk_plldig *data = to_clk_plldig(hw);
> + bool valid = false;
> + unsigned long round_rate = 0;
> + u32 rfdphi1 = 0, val, mult = 0;
> +
> + valid = plldig_is_valid_range(rate, parent_rate, &mult,
> + &rfdphi1, &round_rate);
> + if (!valid) {
> + pr_warn("%s: unable to support rate %lu, parent_rate: %lu\n",
> + clk_hw_get_name(hw), rate, parent_rate);
> + return -EINVAL;
> + }
> +
> + val = readl(data->regs + PLLDIG_REG_PLLDV);
> + val = mult;
> + rfdphi1 = PLLDIG_SET_RFDPHI1(rfdphi1);
> + val |= rfdphi1;
> +
> + writel(val, data->regs + PLLDIG_REG_PLLDV);
> +
> + return plldig_wait_lock(data);
> +}
> +
[...]
> +
> +struct clk_hw *_plldig_clk_init(const char *name, const char *parent_name,
> + void __iomem *regs)
> +{
> + struct clk_plldig *plldig;
> + struct clk_hw *hw;
> + struct clk_init_data init;
> + int ret;
> +
> + plldig = kzalloc(sizeof(*plldig), GFP_KERNEL);
> + if (!plldig)
> + return ERR_PTR(-ENOMEM);
> +
> + plldig->regs = regs;
> +
> + init.name = name;
> + init.ops = &plldig_clk_ops;
> + init.parent_names = &parent_name;
> + init.num_parents = 1;
> + init.flags = CLK_SET_RATE_GATE;
> +
> + plldig->hw.init = &init;
> +
> + hw = &plldig->hw;
> + ret = clk_hw_register(NULL, hw);
> + if (ret) {
> + kfree(plldig);
> + hw = ERR_PTR(ret);
> + }
> +
> + return hw;
> +}
> +
> +static void __init plldig_clk_init(struct device_node *node)
> +{
> + struct clk_hw_onecell_data *clk_data;
> + struct clk_hw **clks;
> + void __iomem *base;
> +
> + clk_data = kzalloc(struct_size(clk_data, hws, 1),
> + GFP_KERNEL);
> + if (!clk_data)
> + return;
> +
> + clk_data->num = 1;
> + clks = clk_data->hws;
> +
> + base = of_iomap(node, 0);
> + WARN_ON(!base);
> +
> + clks[0] = _plldig_clk_init("pixel-clk",
> + of_clk_get_parent_name(node, 0), base);
Can you use the new way of specifying clk parents instead of calling
of_clk_get-parent_name() here? It would be simpler to just indicate
which index it is (I guess 0) or what the name is going to be in
"clock-names" in this DT node.
> +
> + of_clk_add_hw_provider(node, of_clk_hw_onecell_get, clk_data);
Why is this a #clock-cells = <1> device? It provides one clk, so
presumably it can be #clock-cells = <0> and then this can use
of_clk_hw_simple_get() instead.
> +}
> +
> +CLK_OF_DECLARE(plldig_clockgen, "fsl,ls1028a-plldig", plldig_clk_init);
IS there a reason why this can't be a platform driver? It would be nice
to use platform device APIs.
