RE: [EXT] Re: [v1 1/3] clk: ls1028a: Add clock driver for Display output interface

[Wen He]

> -----Original Message-----
> From: Stephen Boyd <sboyd@xxxxxxxxxx>
> Sent: 2019å8æ14æ 2:25
> To: Michael Turquette <mturquette@xxxxxxxxxxxx>; Wen He
> <wen.he_1@xxxxxxx>; Leo Li <leoyang.li@xxxxxxx>;
> linux-clk@xxxxxxxxxxxxxxx; linux-devel@xxxxxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; liviu.dudau@xxxxxxx
> Cc: Wen He <wen.he_1@xxxxxxx>
> 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.he_1@xxxxxxx>
> > + *
> > + */
> > +
> > +#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