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

[Andre Przywara]

On Mon, 7 Jan 2019 14:01:01 +0100
Maxime Ripard <maxime.ripard at bootlin.com> wrote:

Hi,

> On Mon, Jan 07, 2019 at 01:03:33AM +0000, André Przywara 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  
> 
> We have PLL with 2 multipliers, but also others with other factors
> sets, so that will end up being confusing. If the MP, NK and so on
> stuff is confusing, maybe we should just add a comment on top of that
> structure to explain what those factors are and what it actually
> means?

Fair enough, or we name it CCU_CLK_TYPE_PLL_NK, because this is what
this type deals with. Point is that by chance I happened to know about
those naming of factors in the manual, but other might be lost by just
seeing "mp" or "nk", without any explanation - and the comment doesn't
help here at all.

The other part is that the "TYPE_MP" is twice as confusing, as it can
perfectly describe the MMC clocks, which use "N" and "M" in the A64
manual, for instance. That's why my suggestion for calling a spade a
spade and saying it's a divider clock with a multiplexer. Happy to have
the Linux naming in the comments.

Thanks,
Andre.