RE: [EXT] Re: [v4 2/2] clk: ls1028a: Add clock driver for Display output interface
> -Original Message-
> From: Stephen Boyd
> Sent: 2019年9月19日 1:01
> To: devicet...@vger.kernel.org; linux-...@vger.kernel.org;
> linux-de...@linux.nxdi.nxp.com; linux-kernel@vger.kernel.org; Mark Rutland
> ; Michael Turquette ;
> Rob Herring ; Wen He
> Cc: Leo Li ; liviu.du...@arm.com
> Subject: RE: [EXT] Re: [v4 2/2] clk: ls1028a: Add clock driver for Display
> output
> interface
>
>
> Quoting Wen He (2019-09-18 02:20:26)
> > > -Original Message-
> > > From: Stephen Boyd Quoting Wen He (2019-08-29
> > > 03:59:19)
> > > > diff --git a/drivers/clk/clk-plldig.c b/drivers/clk/clk-plldig.c
> > > > new file mode 100644 index ..d3239bcf59de
> > > > --- /dev/null
> > > > +++ b/drivers/clk/clk-plldig.c
> > > > @@ -0,0 +1,298 @@
> [...]
> >
> > >
> > > > +
> > > > +/* Maximum of the divider */
> > > > +#define MAX_RFDPHI1 63
> > > > +
> > > > +/* Best value of multiplication factor divider */
> > > > +#define PLLDIG_DEFAULE_MULT 44
> > > > +
> > > > +/*
> > > > + * 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:
> > > > + * fpll_phi = (pll_ref * mfd) / div_rfdphi1 */ struct
> > > > +plldig_phi1_param {
> > > > + unsigned long rate;
> > > > + unsigned int rfdphi1;
> > > > + unsigned int mfd;
> > > > +};
> > > > +
> > > > +enum plldig_phi1_freq_range {
> > > > + PHI1_MIN= 2700U,
> > > > + PHI1_MAX= 6U
> > > > +};
> > >
> > > Please just inline these values in the one place they're used.
> > >
> > > > +
> > > > +struct clk_plldig {
> > > > + struct clk_hw hw;
> > > > + void __iomem *regs;
> > > > + struct device *dev;
> > >
> > > Please remove this, it is unused.
> >
> > It is used for probe.
>
> Use a local variable and don't store it away forever in the struct.
>
Understand, will remove it.
> > >
> > > > +
> > > > + val = readl(data->regs + PLLDIG_REG_PLLDV);
> > > > + val = phi1_param.mfd;
> > > > + rfdphi1 = phi1_param.rfdphi1;
> > > > + val |= rfdphi1;
> > > > +
> > > > + writel(val, data->regs + PLLDIG_REG_PLLDV);
> > > > +
> > > > + /* delay 200us make sure that old lock state is cleared */
> > > > + udelay(200);
> > > > +
> > > > + /* Wait until PLL is locked or timeout (maximum 1000 usecs) */
> > > > + ret = readl_poll_timeout_atomic(data->regs +
> > > > + PLLDIG_REG_PLLSR,
> > > cond,
> > > > + cond &
> PLLDIG_LOCK_MASK,
> > > 0,
> > > > + USEC_PER_MSEC);
> > > > +
> > > > + return ret;
> > >
> > > Just return readl_poll_timeout_atomic(...) here.
> >
> > Maybe use below code will to best describes.
> >
> > If (ret)
> > return -ETIMEOUT;
> >
> > return 0;
>
> No, just return readl_poll_timeout_atomic().
Understand, I will send next version patch for this.
Thanks && Best Regards,
Wen
RE: [EXT] Re: [v4 2/2] clk: ls1028a: Add clock driver for Display output interface
Quoting Wen He (2019-09-18 02:20:26)
> > -Original Message-
> > From: Stephen Boyd
> > Quoting Wen He (2019-08-29 03:59:19)
> > > diff --git a/drivers/clk/clk-plldig.c b/drivers/clk/clk-plldig.c new
> > > file mode 100644 index ..d3239bcf59de
> > > --- /dev/null
> > > +++ b/drivers/clk/clk-plldig.c
> > > @@ -0,0 +1,298 @@
[...]
>
> >
> > > +
> > > +/* Maximum of the divider */
> > > +#define MAX_RFDPHI1 63
> > > +
> > > +/* Best value of multiplication factor divider */
> > > +#define PLLDIG_DEFAULE_MULT 44
> > > +
> > > +/*
> > > + * 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:
> > > + * fpll_phi = (pll_ref * mfd) / div_rfdphi1 */ struct
> > > +plldig_phi1_param {
> > > + unsigned long rate;
> > > + unsigned int rfdphi1;
> > > + unsigned int mfd;
> > > +};
> > > +
> > > +enum plldig_phi1_freq_range {
> > > + PHI1_MIN= 2700U,
> > > + PHI1_MAX= 6U
> > > +};
> >
> > Please just inline these values in the one place they're used.
> >
> > > +
> > > +struct clk_plldig {
> > > + struct clk_hw hw;
> > > + void __iomem *regs;
> > > + struct device *dev;
> >
> > Please remove this, it is unused.
>
> It is used for probe.
Use a local variable and don't store it away forever in the struct.
> >
> > > +
> > > + val = readl(data->regs + PLLDIG_REG_PLLDV);
> > > + val = phi1_param.mfd;
> > > + rfdphi1 = phi1_param.rfdphi1;
> > > + val |= rfdphi1;
> > > +
> > > + writel(val, data->regs + PLLDIG_REG_PLLDV);
> > > +
> > > + /* delay 200us make sure that old lock state is cleared */
> > > + udelay(200);
> > > +
> > > + /* Wait until PLL is locked or timeout (maximum 1000 usecs) */
> > > + ret = readl_poll_timeout_atomic(data->regs + PLLDIG_REG_PLLSR,
> > cond,
> > > + cond & PLLDIG_LOCK_MASK,
> > 0,
> > > + USEC_PER_MSEC);
> > > +
> > > + return ret;
> >
> > Just return readl_poll_timeout_atomic(...) here.
>
> Maybe use below code will to best describes.
>
> If (ret)
> return -ETIMEOUT;
>
> return 0;
No, just return readl_poll_timeout_atomic().
RE: [EXT] Re: [v4 2/2] clk: ls1028a: Add clock driver for Display output interface
> -Original Message-
> From: Stephen Boyd
> Sent: 2019年9月17日 4:27
> To: Mark Rutland ; Michael Turquette
> ; Rob Herring ; Wen He
> ; devicet...@vger.kernel.org; linux-...@vger.kernel.org;
> linux-de...@linux.nxdi.nxp.com; linux-kernel@vger.kernel.org
> Cc: Leo Li ; liviu.du...@arm.com; Wen He
>
> Subject: [EXT] Re: [v4 2/2] clk: ls1028a: Add clock driver for Display output
> interface
>
>
> Quoting Wen He (2019-08-29 03:59:19)
> > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index
> > 801fa1cd0321..ab05f342af04 100644
> > --- a/drivers/clk/Kconfig
> > +++ b/drivers/clk/Kconfig
> > @@ -223,6 +223,16 @@ config CLK_QORIQ
> > This adds the clock driver support for Freescale QorIQ platforms
> > using common clock framework.
> >
> > +config CLK_LS1028A_PLLDIG
> > +bool "Clock driver for LS1028A Display output"
>
> Is this a tristate? The driver is made to be a module but it isn't allowed to
> be
> compiled as such.
>
> > +depends on ARCH_LAYERSCAPE || COMPILE_TEST
> > +default ARCH_LAYERSCAPE
> > +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 ..d3239bcf59de
> > --- /dev/null
> > +++ b/drivers/clk/clk-plldig.c
> > @@ -0,0 +1,298 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2019 NXP
> > + *
> > + * Clock driver for LS1028A Display output interfaces(LCD, DPHY).
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +/* PLLDIG register offsets and bit masks */
> > +#define PLLDIG_REG_PLLSR0x24
> > +#define PLLDIG_REG_PLLDV0x28
> > +#define PLLDIG_REG_PLLFM0x2c
> > +#define PLLDIG_REG_PLLFD0x30
> > +#define PLLDIG_REG_PLLCAL1 0x38
> > +#define PLLDIG_REG_PLLCAL2 0x3c
> > +#define PLLDIG_LOCK_MASKBIT(2)
> > +#define PLLDIG_SSCGBYP_ENABLE BIT(30)
> > +#define PLLDIG_FDEN BIT(30)
> > +#define PLLDIG_DTHRCTL (0x3 << 16)
> > +
> > +/* macro to get/set values into register */
> > +#define PLLDIG_GET_MULT(x) (((x) & ~(0xff00)) << 0)
> > +#define PLLDIG_GET_RFDPHI1(x) ((u32)(x) >> 25)
> > +#define PLLDIG_SET_RFDPHI1(x) ((u32)(x) << 25)
>
> Maybe you can use the FIELD_GET() APIs and genmask from bitfield.h?
Hmm, that a good idea.
>
> > +
> > +/* Maximum of the divider */
> > +#define MAX_RFDPHI1 63
> > +
> > +/* Best value of multiplication factor divider */
> > +#define PLLDIG_DEFAULE_MULT 44
> > +
> > +/*
> > + * 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:
> > + * fpll_phi = (pll_ref * mfd) / div_rfdphi1 */ struct
> > +plldig_phi1_param {
> > + unsigned long rate;
> > + unsigned int rfdphi1;
> > + unsigned int mfd;
> > +};
> > +
> > +enum plldig_phi1_freq_range {
> > + PHI1_MIN= 2700U,
> > + PHI1_MAX= 6U
> > +};
>
> Please just inline these values in the one place they're used.
>
> > +
> > +struct clk_plldig {
> > + struct clk_hw hw;
> > + void __iomem *regs;
> > + struct device *dev;
>
> Please remove this, it is unused.
It is used for probe.
>
> > +};
> > +
> > +#define to_clk_plldig(_hw) container_of(_hw, struct clk_plldig, hw)
> > +#define LOCK_TIMEOUT_USUSEC_PER_MSEC
>
> Is this used?
Yes, it is used.
>
> > +
> > +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
> > +* detector output was disable. SSCG Bypass mode should be
> enable.
> > +*/
> > + val |= PLLDIG_SSCGBYP_ENABLE;
> > + writel(val, data->regs + PLLDIG_REG_PLLFM);
> > +
> > + val = readl(data->regs + PLLDIG_REG_PLLFD);
> > + /* Disable dither and Sigma delta modulation in bypass mode */
> > + val |= (PLLDIG_FDEN | PLLDIG_DTHRCTL);
> > + writel(val, data->regs + PLLDIG_REG_PLLFD);
> > +
> > + return 0;
> > +}
> > +
> > +static void plldig_disable(struct clk_hw *hw) {
> > + struct clk_plldig *data = to_clk_plldig(hw);
