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

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.

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?

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

Best regards