[U-Boot] [PATCH v5 15/26] clk: sunxi: Add ccu clock tree support

[Jagan Teki]

On Mon, Jan 7, 2019 at 6:35 AM André Przywara <andre.przywara at arm.com> wrote:
>
> On 31/12/2018 16:59, Jagan Teki wrote:
> > 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
> > which can be handle via ccu_clock_gate.
>
> So if I understand this correctly, you implement the gate functionality
> separately from the complex clock code, even if they are the same clock
> from the DT point of view? So if one wants to enable the MMC0 clock,
> which is a mux/divider clock, one needs to specify an extra entry in the
> gate array to describe the enable bit in this special clock register?
> Sounds a bit surprising, but is probably a neat trick to keep things
> simple. There should be a comment in the code to this regard then.
>
> > Tree clocks are parent clock type, fixed clocks, mp, nk, nkm, nkmp,
> > pre/post div, flags etc. which were managed via ccu_clock_tree.
>
> For a start, can we use more descriptive names than those very specific
> MP/NK names? DIV_MUX and PLL sound more descriptive to me. I understand
> that Linux uses those terms, but it would be great if uninitiated people
> have a chance to understand this as well.
>
> > This patch add support for MP, NK, MISC, FIXED clock types as part of
> > ccu clock tree with get_rate functionality this eventually used by
> > uart driver. and rest of the infrastructure will try to add while CLK
> > is being used on respective peripherals.
> >
> > Note that few of the tree type clock would require to enable gates on
> > their specific clock, in that case we need to add the gate details via
> > ccu_clock_gate, example: MP with gate so the gate offset, bit value
> > should add as part of ccu_clock_gate.
> >
> > Signed-off-by: Jagan Teki <jagan at amarulasolutions.com>
> > ---
> >  arch/arm/include/asm/arch-sunxi/ccu.h | 192 +++++++++++++++++++++++++-
> >  drivers/clk/sunxi/clk_a64.c           |  40 ++++++
> >  drivers/clk/sunxi/clk_sunxi.c         | 182 ++++++++++++++++++++++++
> >  3 files changed, 413 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/include/asm/arch-sunxi/ccu.h b/arch/arm/include/asm/arch-sunxi/ccu.h
> > index 3fdc26978d..61b8c36b3b 100644
> > --- a/arch/arm/include/asm/arch-sunxi/ccu.h
> > +++ b/arch/arm/include/asm/arch-sunxi/ccu.h
> > @@ -7,15 +7,204 @@
> >  #ifndef _ASM_ARCH_CCU_H
> >  #define _ASM_ARCH_CCU_H
> >
> > +#define OSC_32K_ULL          32000ULL
>
> 32768
>
> And why ULL? The whole Allwinner clock system works with 32-bit values,
> so just U would be totally sufficient. This avoid blowing this up to 64
> bit unnecessarily, which sounds painful for those poor ARMv7 parts.
>
> > +#define OSC_24M_ULL          24000000ULL
> > +
> > +/**
> > + * enum ccu_clk_type - ccu clock types
> > + *
> > + * @CCU_CLK_TYPE_MISC:                       misc clock type
>
> What is MISC, exactly? Seems like an artefact clock to me, some
> placeholder you need because gate clocks are handled separately in the
> gates struct. Should this be called something with SIMPLE instead, or GATE?
>
> > + * @CCU_CLK_TYPE_FIXED:                      fixed clock type
> > + * @CCU_CLK_TYPE_MP:                 mp clock type
> > + * @CCU_CLK_TYPE_NK:                 nk clock type
>
> What is the point of those comments, as you are basically repeating the
> enum name? What about:
>  * @CCU_CLK_TYPE_PLL:           PLL clock with two multiplier fields
>  * @CCU_CLK_TYPE_MUX_DIV:       multiple parents, two divider fields
>
> This gives more speaking names, plus some explanation.

My idea is to give generic name for a given clock type for example MP
clock has different varients like MP clock, MP with DIV, MP with
Postdiv, MP can be MMC etc. same like NK clock type as NK with
postdiv, NK can be PLL.

With this we can expose the base name to outside and keep fill the
require variants during macro initialization. This can avoid to many
names on the same clock based on the different variants.  Yes we can
add proper full detailed comments on the given type.