[PATCH 09/20] clk: imx: pllv3: add support for PLLV3_AV type
Giulio Benetti
giulio.benetti at benettiengineering.com
Wed Dec 11 13:47:33 CET 2019
On 12/10/19 1:07 AM, Lukasz Majewski wrote:
> On Mon, 9 Dec 2019 18:13:04 +0100
> Giulio Benetti <giulio.benetti at 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 at benettiengineering.com> wrote:
>>>
>>>> Add support for PLLV3 AV type.
>>>>
>>>> Signed-off-by: Giulio Benetti
>>>> <giulio.benetti at 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).
That is true, it provides 32K of sram by default, but with DCD on
imximage you can set OCRAM to be 512K on imxrt1050 and 256K on
imxrt1020(the shortest Mcu of the family with EBI). So you have 256K for
SPL at least(including text, rodata and stack).
So i.MXRT has not so much size costraint on SPL.
Anyway, let me know how to proceed, if keeping a big pllv3.c or making
it possible to select which kind of pll to compile.
>
> 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.
Yes, I've seen it, very cool.
> 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.
I'll check it out
>
> - If OF_PLATDATA is used - then it is possible to NOT compile libfdt and
> friends, which brings the substantial footprint reduction.
Yes
>>
>> 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 :-).
That's perfect then! :-)
Thank you
Best regards
--
Giulio Benetti
Benetti Engineering sas
>>
>> 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
>
More information about the U-Boot
mailing list