[PATCH 09/20] clk: imx: pllv3: add support for PLLV3_AV type

Lukasz Majewski
2019-12-10T00:07:03Z

On Mon, 9 Dec 2019 18:13:04 +0100
Giulio Benetti <giulio.benetti@benettiengineering.com> wrote:

> Hi Lukasz,
>
> On 12/8/19 4:05 PM, Lukasz Majewski wrote:
> > On Wed, 4 Dec 2019 18:44:28 +0100
> > Giulio Benetti <giulio.benetti@benettiengineering.com> wrote:
> >
> >> Add support for PLLV3 AV type.
> >>
> >> Signed-off-by: Giulio Benetti
> >> <giulio.benetti@benettiengineering.com> ---
> >> drivers/clk/imx/clk-pllv3.c | 76
> >> +++++++++++++++++++++++++++++++++++++ 1 file changed, 76
> >> insertions(+)
> >>
> >> diff --git a/drivers/clk/imx/clk-pllv3.c
> >> b/drivers/clk/imx/clk-pllv3.c index d5087a104e..fc16416d5f 100644
> >> --- a/drivers/clk/imx/clk-pllv3.c
> >> +++ b/drivers/clk/imx/clk-pllv3.c
> >> @@ -6,6 +6,7 @@
> >>
> >> #include <common.h>
> >> #include <asm/io.h>
> >> +#include <div64.h>
> >> #include <malloc.h>
> >> #include <clk-uclass.h>
> >> #include <dm/device.h>
> >> @@ -16,6 +17,10 @@
> >> #define UBOOT_DM_CLK_IMX_PLLV3_GENERIC
> >> "imx_clk_pllv3_generic" #define UBOOT_DM_CLK_IMX_PLLV3_SYS
> >> "imx_clk_pllv3_sys" #define UBOOT_DM_CLK_IMX_PLLV3_USB
> >> "imx_clk_pllv3_usb" +#define UBOOT_DM_CLK_IMX_PLLV3_AV
> >> "imx_clk_pllv3_av" +
> >> +#define PLL_NUM_OFFSET 0x10
> >> +#define PLL_DENOM_OFFSET 0x20
> >>
> >> #define BM_PLL_POWER (0x1 << 12)
> >> #define BM_PLL_LOCK (0x1 << 31)
> >> @@ -143,6 +148,65 @@ static const struct clk_ops clk_pllv3_sys_ops
> >> = { .set_rate = clk_pllv3_sys_set_rate,
> >> };
> >>
> >> +static ulong clk_pllv3_av_get_rate(struct clk *clk)
> >> +{
> >> + struct clk_pllv3 *pll = to_clk_pllv3(clk);
> >> + unsigned long parent_rate = clk_get_parent_rate(clk);
> >> + u32 mfn = readl(pll->base + PLL_NUM_OFFSET);
> >> + u32 mfd = readl(pll->base + PLL_DENOM_OFFSET);
> >> + u32 div = readl(pll->base) & pll->div_mask;
> >> + u64 temp64 = (u64)parent_rate;
> >> +
> >> + temp64 *= mfn;
> >> + do_div(temp64, mfd);
> >> +
> >> + return parent_rate * div + (unsigned long)temp64;
> >> +}
> >> +
> >> +static ulong clk_pllv3_av_set_rate(struct clk *clk, ulong rate)
> >> +{
> >> + struct clk_pllv3 *pll = to_clk_pllv3(clk);
> >> + unsigned long parent_rate = clk_get_parent_rate(clk);
> >> + unsigned long min_rate = parent_rate * 27;
> >> + unsigned long max_rate = parent_rate * 54;
> >> + u32 val, div;
> >> + u32 mfn, mfd = 1000000;
> >> + u32 max_mfd = 0x3FFFFFFF;
> >> + u64 temp64;
> >> +
> >> + if (rate < min_rate || rate > max_rate)
> >> + return -EINVAL;
> >> +
> >> + if (parent_rate <= max_mfd)
> >> + mfd = parent_rate;
> >> +
> >> + div = rate / parent_rate;
> >> + temp64 = (u64)(rate - div * parent_rate);
> >> + temp64 *= mfd;
> >> + do_div(temp64, parent_rate);
> >> + mfn = temp64;
> >> +
> >> + val = readl(pll->base);
> >> + val &= ~pll->div_mask;
> >> + val |= div;
> >> + writel(val, pll->base);
> >> + writel(mfn, pll->base + PLL_NUM_OFFSET);
> >> + writel(mfd, pll->base + PLL_DENOM_OFFSET);
> >> +
> >> + /* Wait for PLL to lock */
> >> + while (!(readl(pll->base) & BM_PLL_LOCK))
> >> + ;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static const struct clk_ops clk_pllv3_av_ops = {
> >> + .enable = clk_pllv3_generic_enable,
> >> + .disable = clk_pllv3_generic_disable,
> >> + .get_rate = clk_pllv3_av_get_rate,
> >> + .set_rate = clk_pllv3_av_set_rate,
> >> +};
> >> +
> >> struct clk *imx_clk_pllv3(enum imx_pllv3_type type, const char
> >> *name, const char *parent_name, void __iomem
> >> *base, u32 div_mask)
> >> @@ -174,6 +238,11 @@ struct clk *imx_clk_pllv3(enum imx_pllv3_type
> >> type, const char *name, pll->div_shift = 1;
> >> pll->powerup_set = true;
> >> break;
> >> + case IMX_PLLV3_AV:
> >> + drv_name = UBOOT_DM_CLK_IMX_PLLV3_AV;
> >> + pll->div_shift = 0;
> >> + pll->powerup_set = false;
> >> + break;
> >> default:
> >> kfree(pll);
> >> return ERR_PTR(-ENOTSUPP);
> >> @@ -212,3 +281,10 @@ U_BOOT_DRIVER(clk_pllv3_usb) = {
> >> .ops = &clk_pllv3_generic_ops,
> >> .flags = DM_FLAG_PRE_RELOC,
> >> };
> >> +
> >> +U_BOOT_DRIVER(clk_pllv3_av) = {
> >> + .name = UBOOT_DM_CLK_IMX_PLLV3_AV,
> >> + .id = UCLASS_CLK,
> >> + .ops = &clk_pllv3_av_ops,
> >> + .flags = DM_FLAG_PRE_RELOC,
> >> +};
> >
> > I don't mind about adding this new functionality, but I'm a bit
> > concerned about increase if the size of SPL binary (as it sets the
> > DM_FLAG_PRE_RELOC).
> >
> > Do you have any data about increase of the final binary size?
>
> Yes, following what you've pointed me below(using board kp_imx6q_tpc
> with buildman) these are the results(from patch 1 to 7):
> 01: clk: imx: pllv3: register PLLV3 GENERIC and USB as 2 different
> clocks 07: clk: imx: pllv3: add support for PLLV3_AV type
> arm: (for 1/1 boards) all +963.0 data +136.0 rodata +99.0
> spl/u-boot-spl:all +831.0 spl/u-boot-spl:data +136.0
> spl/u-boot-spl:rodata +99.0 spl/u-boot-spl:text +596.0 text +728.0
>
> So for SPL, as I understand, latest text increment of +728 should be
> taken because it's the sum of text(596)+data(136) then padded, right?
> So SPL increased not few.

This board's SPL size is ~35KiB. The increase is 2%.

Seems to be not much, but ...

>
> > The buildman script has options to check the difference of the final
> > binary (i.e. SPL) size (as provided by Tom Rini):
> >
> > ./tools/buildman/$ export SOURCE_DATE_EPOCH=`date +%s`
> > $ ./tools/buildman/buildman -o /tmp/test --step 0 -b
> > origin/master.. --force-build -CveE $ ./tools/buildman/buildman -o
> > /tmp/test --step 0 -b origin/master.. -Ssdel
> >
> > to get size changes between point A and point Z in a branch, and
> > omit --step 0 when I need to see which patch in between them caused
> > the size change.
> >
> >
> > If the SPL growth is too big - maybe we shall introduce some Kconfig
> > options to add a separate support for those PLLv3 options ?
>
> If you want I can slice drivers down with different Kconfig entries
> and corresponding clk-pllv3-generic.c, clk-pllv3-usb.c etc.
> Or maybe using #ifdef inside the same file?

I do guess that iMXRT is some kind of resource constrained SoC?
Wikipedia [1] says that it is for low-latency with real-time (memory
protection - MPU - instead of MMU).

If this is the case, then there are some (better?) options for SPL:

- One can use OF_PLATDATA framework to reduce the SPL size - in short -
it removes from SPL libfdt, which parses device tree (DT) and
replaces DT with compiled in C structures. For SPL it brings good
results in reducing binary size.

One good example of OF_PLATDATA conversion is the xea (i.MX28 [*])
board support (now it waits for upstreaming) [2]. It can be easily
applied on top of current -master and you will see how this works in
practice.

- If OF_PLATDATA is used - then it is possible to NOT compile libfdt and
friends, which brings the substantial footprint reduction.

>
> Anyway the only board using this except imxrt1050-evk is
> kp_imx6q_tpc, so this should be easy to test.

You would not believe, but by chance I do have one kp_imx6q_tpc board
available for testing :-).

>
> Best regards

Links:

[1] - https://en.wikipedia.org/wiki/ARM_Cortex-R

[2] - https://patchwork.ozlabs.org/patch/1205772/

Note:

[*] - i.MX28 is a pretty old ARM9 SoC, with not so much resources (now)
available. To use the device tree drivers, the OF_PLATDATA had to be
used. Moreover, some old drivers (like gpio, pinctrl, eth, etc.) had to
be converted to device tree support with OF_PLATDATA (please refer to
mxs_* prefix in drivers).
Now all those drivers are upstreamed, so it is just enough to apply the
patch which adds xea board.

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20191210/5c136e84/attachment-0001.sig>