[U-Boot] [PATCH v5 00/26] clk: Add Allwinner CLK, RESET support

[Jagan Teki]

On Sun, Jan 6, 2019 at 6:49 PM André Przywara <andre.przywara at arm.com> wrote:
>
> On 31/12/2018 16:59, Jagan Teki wrote:
>
> Hi Jagan,
>
> many thanks for picking this up, I was about to come back to this
> myself. I am looking at the pinctrl part at the moment, so good you are
> working on the clocks!
>
> TL;DR: I am good with the first patches, but would like to drop the last
> five 5 patches from this series, and discuss the whole tree approach (or
> at least the patch) further.
>
> > Although the previous version[1] is properly handled the clock gates
> > with enable and disable management, but this series is trying to add
> > some more complex Allwinner CLK architecture by handling parent clock
> > and other CLK attributes.
> >
> > Allwinner Clock control unit comprises of parent clocks, gates, multiplexers,
> > dividers, multipliers, pre/post dividers and flags etc.
> >
> > So, the U-Boot implementation of ccu has divided into gates and tree.
> > gates are generic clock configuration of enable/disable bit management
> > and these can be handled via ccu_clock_gate, which is almost same as
> > previous version changes.
>
> So I like this part very much, all those gates and resets are nicely
> readable.
> Also the removal of the special sunxi USB drivers is very welcome.
> So up until patch including 21/26, except 15/26, I am all in.

Yes my plan is to do the same.

>
> > Tree clock has more Allwinner CLK attributes like clock type, fixed clocks,
> > misc clocks, mp, nk, nkm, nkmp, pre/post div, flags etc and these can be
> > managed via ccu_clock_tree.
>
> Mmmh, this looks quite complex for U-Boot's very limited needs to me.
> As far as I can see we basically have a static setup for most devices,
> the ones where we actually change the parent clocks are only clock
> consumers, namely DE/TCON and MMC. The former chooses a parent once,
> based on the calculated clock rate. Only for MMC we switch it from OSC24
> (for the initial SPI-like setup phase) to PLL_PERIPH0 at runtime.
> But however this does not really affect the tree, and also we never do
> re-parenting.
> Under this premises, can't we do this somewhat simpler? At the end of
> the day this is U-Boot, not a sophisticated kernel, and we got away with
> quite simple clock code so far.

Yes, Idea is to go with limited clock setups but the thing is as you
know Allwinner deals numerous number of clock attributes and I found
it difficult to handle them w/o traversing the re-parenting stuff. I
have tried simple version of set_rate in previous version[2] but it
wouldn't ended as we desired. These operations need re-parenting, so
we have to handle parents in clock attributes.

So finally I came up with tree structure.

My idea here is to support get and set rate with minimal re-parenting
like OSC24, PLL_PERIPH0, PLL_PERIPH1 not adding much code like Linux
does. Like GATE macro, TREE macro is also a readable structure with
all possible/needed clk attributes at one glance. The code that handle
these tree attributes may look confusing, but it is obvious because it
would need to handle re-parenting.

If SoC has a big CLK architecture like this and to support all or
needed clock operations, re-parenting may be one of painful code for
SoC driver in U-Boot, Since CLK framework doesn't handle these
generically(may be no need) as compared to Linux. and the existing SoC
like mediatek clock code does the same.

At, this point, I think this tree approach can be possible way to move
since it handle all clock attribute types and yes we can only support
clock re-parenting which are required for U-Boot.

> Meanwhile I am struggling to understand your approach, which looks
> impressive on the first glance, but leave me scratching my head when
> looking at details. I will comment on 15/26 for the parts I am wondering
> about. I was trying to add MMC support and hit some issues.

As I said above there is no generic way to handle re-parenting in
U-Boot, and we need to traverse based on the SoC Clock architecture.

15/26 handling get_rate, say for example UART request a get_rate.


                              |==> OSC_32K
UART-->APB2--->|==> OSC_24M
                              |==> PLL_PH0   ===> OSC_24M
                              |==> PLL_PH0

Each clock tree, has clock type so UART is marked as MISC since it
can't not usual clock tree unlike MMC. MMC can be MP clock type and
so..on

So, if driver requesting get_rate with UART clock, then we need to
traverse all the UART clock tree and get the rate based on the type
and identified parent clock. UART is MISC so it can directly trigger
parent clock abp2, apb2 need to find the parent using mux shift,
width, once apb2 found the parent it will go to next as so on.

>
> On the SPI part:
> I am bit puzzled about the last 5 patches dealing with SPI. I see that
> they depend on the others, but would like to deal with them separately.

True, but these are previous version changes ie reason I grouped it
here. anyway will handle accordingly in next versions.

> First, patch 25/26 (the sun6i SPI driver) is a bit surprising to find in
> here. I guess you want to have an easy user for another clock, but I
> believe this should be a separate series. There were other posts already
> for a sun6i driver, also I think we should have a unified driver instead
> of duplicating most of it again. See Oskari's latest post.

Yes, I saw this sun6i driver is lasting for many releases and my plan
it to unified the driver w/o any ifdef.

>
> Secondly: I don't think the SPI clocks are correct. You model them as
> simple gates only, kind of ignoring the divider settings. This is what
> the current sun4i driver does as well, but: it writes 1U << 31 into the
> clock register, which explicitly writes 0 to the other bits, setting 24
> MHz as the input rate. Your approach would read-modify-write the
> register, just setting bit 31 and leaving the other ones as they were
> before.
> So why isn't that using your new tree approach? Looks like a standard
> mux/divider clock to me.

Yes these yet to implement, ie reason I have just added clk tree only
on A64. Idea is to send next version for full support on all SoC
types.

[2] https://patchwork.ozlabs.org/patch/955993/