On Mon, 7 Jan 2019 14:01:01 +0100 Maxime Ripard <maxime.rip...@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 <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? 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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
