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

[Maxime Ripard <maxime.ripard@bootlin.com>]

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@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?

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190107/26a42f3f/attachment.sig>