[U-Boot] [PATCH v2 00/10] clk: imx: Add i.MX6 CLK support

Peng Fan peng.fan at nxp.com
Tue Apr 23 07:47:38 UTC 2019


Hi Lukasz,

> -----Original Message-----
> From: Lukasz Majewski [mailto:lukma at denx.de]
> Sent: 2019年4月20日 6:18
> To: Peng Fan <peng.fan at nxp.com>
> Cc: Jagan Teki <jagan at amarulasolutions.com>; Stefano Babic
> <sbabic at denx.de>; Fabio Estevam <fabio.estevam at nxp.com>; Simon Glass
> <sjg at chromium.org>; Tom Rini <trini at konsulko.com>; Marek Vasut
> <marek.vasut+renesas at gmail.com>; Neil Armstrong
> <narmstrong at baylibre.com>; Philipp Tomsich
> <philipp.tomsich at theobroma-systems.com>; Maxime Ripard
> <maxime.ripard at bootlin.com>; Michael Trimarchi
> <michael at amarulasolutions.com>; Andre Przywara
> <andre.przywara at arm.com>; U-Boot-Denx <u-boot at lists.denx.de>;
> linux-amarula at amarulasolutions.com; dl-uboot-imx <uboot-imx at nxp.com>
> Subject: Re: [U-Boot] [PATCH v2 00/10] clk: imx: Add i.MX6 CLK support
> 
> On Fri, 19 Apr 2019 08:52:28 +0000
> Peng Fan <peng.fan at nxp.com> wrote:
> 
> > > On Fri, 19 Apr 2019 11:56:25 +0530
> > > Jagan Teki <jagan at amarulasolutions.com> wrote:
> > >
> > > > On Fri, Apr 5, 2019 at 2:20 AM Lukasz Majewski <lukma at denx.de>
> > > > wrote:
> > > > >
> > > > > Hi Jagan,
> > > > >
> > > > > > On Thu, Apr 4, 2019 at 3:31 PM Lukasz Majewski <lukma at denx.de>
> > > > > > wrote:
> > > > > > >
> > > > > > > On Thu, 4 Apr 2019 14:56:36 +0530 Jagan Teki
> > > > > > > <jagan at amarulasolutions.com> wrote:
> > > > > > >
> > > > > > > > On Thu, Apr 4, 2019 at 2:31 PM Lukasz Majewski
> > > > > > > > <lukma at denx.de> wrote:
> > > > > > > > >
> > > > > > > > > On Tue,  2 Apr 2019 16:58:33 +0530 Jagan Teki
> > > > > > > > > <jagan at amarulasolutions.com> wrote:
> > > > > > > > >
> > > > > > > > > > This is revised version of previous i.MX6 clock
> > > > > > > > > > management [1].
> > > > > > > > > >
> > > > > > > > > > The main difference between previous version is
> > > > > > > > > > - Group the i.MX6 ccm clocks into gates and tree
> > > > > > > > > > instead of handling the clocks in simple way using
> > > > > > > > > > case statement.
> > > > > > > > > > - use gate clocks for enable/disable management.
> > > > > > > > > > - use tree clocks for get/set rate or parent traverse
> > > > > > > > > > management.
> > > > > > > > > > - parent clock handling via clock type.
> > > > > > > > > > - traverse the parent clock using recursive
> > > > > > > > > > functionlaity.
> > > > > > > > > >
> > > > > > > > > > The main motive behind this tree framework is to make
> > > > > > > > > > the clock tree management simple and useful for U-Boot
> > > > > > > > > > requirements instead of garbing Linux clock management
> > > > > > > > > > code.
> > > > > > > > > >
> > > > > > > > > > We are trying to manage the Allwinner clocks with
> > > > > > > > > > similar kind, so having this would really help i.MX6
> > > > > > > > > > as well.
> > > > > > > > > >
> > > > > > > > > > Added simple names for clock macros, but will update
> > > > > > > > > > it in future version.
> > > > > > > > > >
> > > > > > > > > > I have skipped ENET clocks from previous series, will
> > > > > > > > > > add it in future patches.
> > > > > > > > > >
> > > > > > > > > > Changes for v2:
> > > > > > > > > > - changed framework patches.
> > > > > > > > > > - add support for imx6qdl and imx6ul boards
> > > > > > > > > > - add clock gates, tree.
> > > > > > > > > >
> > > > > > > > > > [1] https://patchwork.ozlabs.org/cover/950964/
> > > > > > > > > >
> > > > > > > > > > Any inputs?
> > > > > > > > >
> > > > > > > > > Hmm.... It looks like we are doing some development in
> > > > > > > > > parallel.
> > > > > > > > >
> > > > > > > > > Please look into following commit [1]:
> > > > > > > > > https://patchwork.ozlabs.org/patch/1034051/
> > > > > > > > >
> > > > > > > > > It ports from Linux 5.0 the CCF framework for iMX6Q,
> > > > > > > > > which IMHO in the long term is a better approach.
> > > > > > > > > The code is kept simple and resembles the code from
> > > > > > > > > Barebox.
> > > > > > > > >
> > > > > > > > > Please correct me if I'm wrong, but the code from your
> > > > > > > > > work is not modeling muxes, gates and other components
> > > > > > > > > from Linux CCF.
> > > > > > > >
> > > > > > > > The U-Boot implementation of CLK would require as minimal
> > > > > > > > and simple as possible due to requirement of U-Boot
> > > > > > > > itself. Hope you agree this point?
> > > > > > >
> > > > > > > Now i.MX6 is using clock.c CLK implementation. If we decide
> > > > > > > to replace it - we shall do it in a way, which would allow
> > > > > > > us to follow Linux kernel. (the barebox implementation is a
> > > > > > > stripped CCF from Linux, the same is in patch [1]).
> > > > > > >
> > > > > > > > if yes having CCF stack code to handle all clock with
> > > > > > > > respective separate drivers management is may not require
> > > > > > > > as of now, IMHO.
> > > > > > >
> > > > > > > I do have a gut feeling, that we will end up with the need
> > > > > > > to have the CCF framework ported anyway. As for example
> > > > > > > imx7/8 can re-use muxes, gates code.
> > > > > >
> > > > > > As per my experience the main the over-ahead to handle clocks
> > > > > > in U-Boot if we go with separate clock drivers is for Video
> > > > > > and Ethernet peripherals. these are key IP's which use more
> > > > > > clocks from U-Boot point-of-view, others can be handle pretty
> > > > > > straight-forward unless if they don't have too much tree
> > > > > > chain.
> > > > > >
> > > > > > On this series, the tree management is already supported ENET
> > > > > > in i.MX6, and Allwinner platforms.
> > > > > >
> > > > > > As of now, I'm thinking I can handle reset of the clocks with
> > > > > > similar way.
> > > > >
> > > > > But this code also supports ENET and ESDHCI clocks on i.MX6Q (as
> > > > > supporting those was the motivator for this work).
> > > > >
> > > > > One important thing to be aware of - the problem with SPL's
> > > > > footprint. The implementation with clock.c is small and simple,
> > > > > but doesn't scale well.
> > > > >
> > > > > >
> > > > > > >
> > > > > > > However, those are only my "feelings" after a glimpse look
> > > > > > > - I will look into your code more thoroughly and provide
> > > > > > > feedback.
> > > > > >
> > > > > > Please have a look, if possible check even the code size by
> > > > > > adding USDHC clocks.
> > > > >
> > > > > Yes, code size (especially in SPL) is an _important_ factor
> > > > > here.
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > This series is using recursive calls for handling
> > > > > > > > parenting stuff to handle get or set rates, which is fine
> > > > > > > > for handling clock tree management as far as U-Boot
> > > > > > > > point-of-view. We have faced similar situation as I
> > > > > > > > explained in commit message about Allwinner clocks [2] and
> > > > > > > > we ended up going this way.
> > > > > > >
> > > > > > > I'm not Allwinner expert - but if I may ask - how far away
> > > > > > > is this implementation from mainline Linux kernel?
> > > > > > >
> > > > > > > How difficult is it to port the new code (or update it)?
> > > > > >
> > > > > > Allwinner clocks also has similar gates, muxs, and with other
> > > > > > platform stuff which has too much scope in Linux to use CCM.
> > > > >
> > > > > For example the barebox managed to get subset of Linux CCF
> > > > > ported, without loosing the CCF similarity.
> > > > >
> > > > >
> > > > > Important factors/requirements for the i.MX clock code:
> > > > >
> > > > > 1. Easy maintenance in long-term
> > > > >
> > > > > 2. Reusing the code in SPL (with a very important factor of
> > > > > _code_size_).
> > > > >
> > > > > 3. Reuse the code for other i.MX SoCs (imx7, imx8)
> > > > >
> > > > > 4. Effort needed to use DM with this code
> > > >
> > > > I understand your points, I was managed this series based on these
> > > > requirements as well.
> > >
> > > Ok.
> > >
> > > Could you share the delta of footprint size (u-boot.img/SPL) with
> > > and without your patch (on imx6q) ?
> > >
> > > In my case the CCF caused increase of u-boot.img proper (as it was
> > > not yet adapted to SPL):
> > >
> > > 415KiB -> 421KiB = 6KiB increase of size (< 2%).
> > >
> > > (This can be further reduced by using OF_PLATDATA).
> > >
> > > This CCF code hasn't been ported to SPL (yet)
> > >
> > > > We even consider the foot-print, atleast for recursive calls of
> > > > handling parenting scale well.
> > >
> > > With CCF porting v3 I'm going to provide some caching, so the
> > > descending would be done at most once.
> > >
> > > > May be we can
> > > > consider to design based on this as per U-Boot.
> > > >
> > >
> > > Please look into point 1. Having code ported from Linux is IMHO
> > > better in the long term.
> >
> > Agree.
> >
> > >
> > > > Let me come-back with another series or do you have any inputs or
> > > > questions, please post it.
> > >
> > > I will post CCF port for imx6q v3 in a few days.
> >
> > Looking forward your new patchset.
> > Working on enabling i.MX8MM CCF support.
> 
> Output of 'dm tree' on imx6q:
> 
>  clk          1  [   ]   fixed_rate_clock      |-- ckil
>  clk          2  [   ]   fixed_rate_clock      |-- ckih1
>  clk          3  [ + ]   fixed_rate_clock      `-- osc
>  clk          4  [ + ]   imx_clk_pllv3             |-- pll2_bus
>  clk          7  [   ]   imx_clk_pfd               |   |--
> pll2_pfd0_352m
>  clk          8  [ + ]   imx_clk_pfd               |   `--
> pll2_pfd2_396m
>  clk          9  [ + ]   imx_clk_mux               |       |--
> usdhc1_sel
>  clk         13  [ + ]   imx_clk_divider           |       |   `--
> usdhc1_podf
>  clk         22  [   ]   imx_clk_gate2             |       |
> `-- usdhc1
>  clk         10  [ + ]   imx_clk_mux               |       |--
> usdhc2_sel
>  clk         14  [ + ]   imx_clk_divider           |       |   `--
> usdhc2_podf
>  clk         23  [   ]   imx_clk_gate2             |       |
> `-- usdhc2
>  clk         11  [ + ]   imx_clk_mux               |       |--
> usdhc3_sel
>  clk         15  [ + ]   imx_clk_divider           |       |   `--
> usdhc3_podf
>  clk         24  [   ]   imx_clk_gate2             |       |
> `-- usdhc3
>  clk         12  [ + ]   imx_clk_mux               |       `--
> usdhc4_sel
>  clk         16  [ + ]   imx_clk_divider           |           `--
> usdhc4_podf
>  clk         25  [   ]   imx_clk_gate2             |
> `-- usdhc4
>  clk          5  [ + ]   imx_clk_pllv3             `-- pll3_usb_otg
>  clk          6  [ + ]   imx_clk_fixed_factor          `-- pll3_60m
>  clk         17  [ + ]   imx_clk_divider                   `--
> ecspi_root
>  clk         18  [   ]   imx_clk_gate2                         |--
> ecspi1
>  clk         19  [   ]   imx_clk_gate2                         |--
> ecspi2
>  clk         20  [   ]   imx_clk_gate2                         |--
> ecspi3
>  clk         21  [   ]   imx_clk_gate2                         `--
> ecspi4
> 

Do you have a public tree/branch for CCF? I am adding imx8mm clk and would like
to base on your tree. I think need to extend clk_ops to support mux/divider,
but not just get rate. To avoid conflict with you work, if you have a public tree,
that could be good.

Thanks,
Peng.


> 
> 
> >
> > Regards,
> > Peng.
> >
> > >
> > > >
> > > > Jagan.
> > >
> > >
> > >
> > >
> > > 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
> 
> 
> 
> 
> 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