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

Mon Apr 22 14:03:19 UTC 2019

On Fri, Apr 19, 2019 at 08:52:28AM +0000, Peng Fan 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.

I just want to re-iterate my support again here that we should be
looking at adapting and stripping down frameworks from the kernel.  They
are:
- Familiar to a large subset of our developers as most folks also work
  on the Linux kernel.
- Already (generally) well designed to take into account the various
  designs of vastly different SoCs that we also want to support.
- Likely to already be fed by device tree and we can just leverage
  what's already in the dts* files.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190422/b0d88689/attachment.sig>