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

Mon Jan 7 01:21:30 UTC 2019

On 06/01/2019 19:22, Jagan Teki wrote:
> 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.

Just for clarification: with re-parenting I meant selecting a different
parent clock *at runtime*, potentially adjusting other clocks which
depend on that. We would not need that in U-Boot, fortunately.

> So finally I came up with tree structure.

Yeah, so looking at patch 15/26 in more detail this is probably the way
to go. I was just hit by that wall of code. Implementing get_rate in a
recursive fashion reduces the file size drastically, see the reply to
that patch.
I was just concerned that making this all data structure based (as
opposed to handle them all in a giant switch/case) would require to
abstract this too much, up to a point where it gets more complicated.
Especially when it comes to special clocks like the MMC beast with its
new clock mode.

> 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.

Yeah, so we need to traverse a clock tree to get and set the parent
rate. I think with the recursive rework this is now quite neat,
especially as we can keep the per-SoC code small.
We just have to see how the special clocks blend in there.

>> 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.

Yeah, I understand this, but was hoping we could handle this in a
switch/case, for clarity. Those macro designs look nice, but are hard to
understand for outsiders.
But I now think that your data driven approach is better to support
multiple SoCs. I can write up some documentation for this.

>> 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.

So I did this, basically by introducing a register map for both
versions. That works, but is not very nice. Oskari's patch has the
advantage of being quite small, and we will probably never need to
support both SPI devices in one build.

>> 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.

Do we need to merge this for all SoCs in the beginning? I think it would
be safer to test this on one SoC (A64) first, to avoid breaking everyone
in case of problems. Especially if we can't test this on every board.

Cheers,
Andre.

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