> -----Original Message----- > From: Stephen Boyd <sb...@kernel.org> > Sent: 2019年8月14日 2:25 > To: Michael Turquette <mturque...@baylibre.com>; Wen He > <wen.h...@nxp.com>; Leo Li <leoyang...@nxp.com>; > linux-...@vger.kernel.org; linux-de...@linux.nxdi.nxp.com; > linux-kernel@vger.kernel.org; liviu.du...@arm.com > Cc: Wen He <wen.h...@nxp.com> > Subject: [EXT] Re: [v1 1/3] clk: ls1028a: Add clock driver for Display output > interface > > > 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?
Actually it also depends Display modules, but we allow building display drivers as modules, so is here whether need add Display modules depend and also allow clock driver building to a module? Would it be better to reduce the number of the modules insert, I think the clock driver should be long available for the system. looks like great if named Kconfig variable to '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. > Understand, > > +#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. Understand.. > > > +#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? Yes, you are 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 */ > Understand, > > + * 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. OK, > > > + > > + /* 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. Yes, you are right, I guess I was thinking too much.. > > > +} > > + > > +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. I think that is good suggestion, should be use platform devices APIs to instead. Best Regards, Wen