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

[Lukasz Majewski <lukma@denx.de>]

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 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/20191212/a1e47304/attachment.sig>