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 <ja...@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
signature.asc
Description: PGP signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
