| Message ID | 20191204174439.69934-10-giulio.benetti@benettiengineering.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | Add i.MXRT family support | expand |
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? 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 ? 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@denx.de
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
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@denx.de
On 12/10/19 1:07 AM, Lukasz Majewski wrote: > 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). 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
Hi Giulio, > On 12/10/19 1:07 AM, Lukasz Majewski wrote: > > 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). > > 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. Maybe after converting (or better _just_ trying) the clk to OF_PLATDATA (and removing libtdt support from SPL) we would save enough space to allowing compiling in all the PLLs ? > > > > > 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 Thanks. Feel free to ask if you have any questions. > > > > > - 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 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@denx.de
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, +};
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(+)