Hi Peng,

> > -----Original Message-----
> > From: Lukasz Majewski [mailto:lu...@denx.de]
> > Sent: 2019年4月23日 16:46
> > To: Peng Fan <peng....@nxp.com>
> > Cc: Jagan Teki <ja...@amarulasolutions.com>; Stefano Babic
> > <sba...@denx.de>; Fabio Estevam <fabio.este...@nxp.com>; Simon Glass
> > <s...@chromium.org>; Tom Rini <tr...@konsulko.com>; Marek Vasut
> > <marek.vasut+rene...@gmail.com>; Neil Armstrong
> > <narmstr...@baylibre.com>; Philipp Tomsich
> > <philipp.toms...@theobroma-systems.com>; Maxime Ripard
> > <maxime.rip...@bootlin.com>; Michael Trimarchi
> > <mich...@amarulasolutions.com>; Andre Przywara
> > <andre.przyw...@arm.com>; U-Boot-Denx <u-boot@lists.denx.de>;
> > linux-amar...@amarulasolutions.com; dl-uboot-imx <uboot-...@nxp.com>
> > Subject: Re: [U-Boot] [PATCH v2 00/10] clk: imx: Add i.MX6 CLK
> > support
> > 
> > On Tue, 23 Apr 2019 07:47:38 +0000
> > Peng Fan <peng....@nxp.com> wrote:
> >   
> > > Hi Lukasz,
> > >  
> > > > -----Original Message-----
> > > > From: Lukasz Majewski [mailto:lu...@denx.de]
> > > > Sent: 2019年4月20日 6:18
> > > > To: Peng Fan <peng....@nxp.com>
> > > > Cc: Jagan Teki <ja...@amarulasolutions.com>; Stefano Babic
> > > > <sba...@denx.de>; Fabio Estevam <fabio.este...@nxp.com>; Simon  
> > Glass  
> > > > <s...@chromium.org>; Tom Rini <tr...@konsulko.com>; Marek Vasut
> > > > <marek.vasut+rene...@gmail.com>; Neil Armstrong
> > > > <narmstr...@baylibre.com>; Philipp Tomsich
> > > > <philipp.toms...@theobroma-systems.com>; Maxime Ripard
> > > > <maxime.rip...@bootlin.com>; Michael Trimarchi
> > > > <mich...@amarulasolutions.com>; Andre Przywara
> > > > <andre.przyw...@arm.com>; U-Boot-Denx <u-boot@lists.denx.de>;
> > > > linux-amar...@amarulasolutions.com; dl-uboot-imx  
> > <uboot-...@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....@nxp.com> wrote:
> > > >  
> > > > > > On Fri, 19 Apr 2019 11:56:25 +0530 Jagan Teki
> > > > > > <ja...@amarulasolutions.com> wrote:
> > > > > >  
> > > > > > > On Fri, Apr 5, 2019 at 2:20 AM Lukasz Majewski
> > > > > > > <lu...@denx.de> wrote:  
> > > > > > > >
> > > > > > > > Hi Jagan,
> > > > > > > >  
> > > > > > > > > On Thu, Apr 4, 2019 at 3:31 PM Lukasz Majewski
> > > > > > > > > <lu...@denx.de> wrote:  
> > > > > > > > > >
> > > > > > > > > > On Thu, 4 Apr 2019 14:56:36 +0530 Jagan Teki
> > > > > > > > > > <ja...@amarulasolutions.com> wrote:
> > > > > > > > > >  
> > > > > > > > > > > On Thu, Apr 4, 2019 at 2:31 PM Lukasz Majewski
> > > > > > > > > > > <lu...@denx.de> wrote:  
> > > > > > > > > > > >
> > > > > > > > > > > > On Tue,  2 Apr 2019 16:58:33 +0530 Jagan Teki
> > > > > > > > > > > > <ja...@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?  
> > 
> > Please find the CCF devel work:
> > https://github.com/lmajewski/u-boot-dfu/commits/CCF-devel
> > 
> > The CCF starts from: c792297e1a47ead02ff2baa4f162de8782b29910  
> 
> Thanks.
> 
> > 
> > (below you will find imx6q DM/DTS conversion code).
> > 
> > What is added when compared to the original one:
> > 
> > - SPL support
> > 
> > - Some fixes for v2019.04+
> > 
> > What is on the TO DO list:
> > 
> > - OF_PLATDATA for SPL (as I did not used any optimisations yet).
> > 
> >   
> > >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.  
> > 
> > Some mux/divider is provided (clk-mux.c / clk-divider.c)  
> 
> After reading into details, there is no clk core logic.
> mux not support reparenting.
> Divider not support rate setting.
> no composite clk support.

I've stripped out the code, which I couldn't test/needed on imx6q.

This version only has the "must have" for imx6q (including SPL) with
some generic code for clock framework in u-boot.

> We could not directly reuse Linux vendor clk driver,
> need adapt to
> U-Boot, 

Yes, you would need to adapt the code (as it was done in barebox and
this patch set).

> should not be hard from your code.

I wanted to keep the structure of the code untouched from Linux.
However, for example, we can't afford in imx6q the
clk[IMX6QDL_<clock_name>] table - it is simply too big for SPL.

> 
> When multiple devices sources from one PLL, one device might would
> like to set pll value that break other devices, there is no logic to
> protect if support freq changing.

Ok, so in your use-case you need the frequency re-calc code? (Which
changes - updates - the clocks up in the tree when root PLL is
changed) ?

No, it was not added.

> 
> Do you have plan work on the upper items?

For the imx6q use case - in which I need some muxing and gating only - I
do not need to recalc/reparent the frequency.

The CCF porting patch only brought:

- the smallest subset of CCF functions for my use case

- directory (and source files) structure from Linux

- some clock uclass enhancement for u-boot

- the clock generic (register) code (clk/clk.c)


The plan was to allow others to port their subset of code when needed.
In that way we only get the code which is really used and tested.

> i.MX8MM use composite heavily in Linux, so I have started.

Ok. I see. Your patches/review is more than welcome.

> 
> Thanks,
> Peng.
> 
> >   
> > > To avoid conflict with
> > > you work, if you have a public tree, that could be good.  
> > 
> > No problem. Thanks for the interest.
> >   
> > >
> > > 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: lu...@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: lu...@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:
> > lu...@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: lu...@denx.de

Attachment: pgpxoHK9HZhEn.pgp
Description: OpenPGP digital signature

_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Reply via email to