On Tue, Jan 8, 2019 at 5:09 PM Andre Przywara <andre.przyw...@arm.com> wrote: > > On Tue, 8 Jan 2019 16:27:14 +0530 > Jagan Teki <ja...@amarulasolutions.com> wrote: > > Hi, > > > On Mon, Jan 7, 2019 at 6:35 AM André Przywara > > <andre.przyw...@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. > > > > Exactly. Idea is to keep the macro's as simple as possible. > > > > Adding gates clocks separately make easy and reasonable way to > > enable/disable clock operations. We even operate with single macro > > with all clock attributes along with gate(either another member or > > common structure like Linux does), but that seems not simple as per as > > my experince since there are many IP's like USB's just need > > enable/disable. > > > > > > > > > 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? > > > > Unlike MP type in MMC, UART doesn't need any clock attributes like > > dividers, mux, etc, just have attached parent. I don't think UART > > clock is simple one It has parent, that indeed have another parents > > and so...on, ie reason I named as MISC. > > Not really, as far as I can see the UART clock is a just a gate clock as > many others, with one parent (APB2). > The fact that APB2 in turn can have multiple parents doesn't affect the > UART clock itself, as you model this via the clock tree. > > In fact we could have similar clocks in the tree structure for the > other gate clocks (USB, for instance), it's just that the UART is the > only user so far which actually queries the clock rate. > > So MISC is way too generic, I would still prefer CCU_CLK_TYPE_GATE.
TYPE_GATE is more sense. fine for me. > > > > > + * @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. > > > > > > > + */ > > > > +enum ccu_clk_type { > > > > + CCU_CLK_TYPE_MISC = 0, > > > > + CCU_CLK_TYPE_FIXED = 1, > > > > + CCU_CLK_TYPE_MP = 2, > > > > + CCU_CLK_TYPE_NK = 3, > > > > +}; > > > > + > > > > /** > > > > * enum ccu_clk_flags - ccu clock flags > > > > * > > > > - * @CCU_CLK_F_INIT_DONE: clock gate init done check > > > > + * @CCU_CLK_F_INIT_DONE: clock tree/gate init done > > > > check > > > > > > Is this flag to tell implemented clocks apart from unimplemented > > > ones, which would be reset to all zeroes by the compiler? Then it > > > should be called something with VALID in it. > > > > Since the flags attached with real numeric initialized by macro itself > > rather than some other source like complier or any, so marked > > INIT_DONE seems to meaningful. > > When I read INIT_DONE I understand some code has initialised this > clock at some point, which isn't true. I don't fight the flag itself, > just the name. OK, I have updated INIT_DONE with IS_VALID and sent new version changes based on this. > > > and eventually the same verified in code whether the init done or not. > > > > > > > > > + * @CCU_CLK_F_POSTDIV: clock post divider > > > > */ > > > > enum ccu_clk_flags { > > > > CCU_CLK_F_INIT_DONE = BIT(0), > > > > + CCU_CLK_F_POSTDIV = BIT(1), > > > > }; > > > > > > > > +/** > > > > + * struct ccu_mult - ccu clock multiplier > > > > + * > > > > + * @shift: multiplier shift value > > > > + * @width: multiplier width value > > > > + * @offset: multiplier offset > > > > + * @min: minimum multiplier > > > > + * @max: maximum multiplier > > > > + */ > > > > +struct ccu_mult { > > > > + u8 shift; > > > > + u8 width; > > > > + u8 offset; > > > > + u8 min; > > > > + u8 max; > > > > +}; > > > > + > > > > +#define _CCU_MULT_OFF_MIN_MAX(_shift, _width, > > > > _offset, \ > > > > + _min, _max) { \ > > > > + .shift = _shift, \ > > > > + .width = _width, \ > > > > + .offset = _offset, \ > > > > + .min = _min, \ > > > > + .max = _max, \ > > > > +} > > > > + > > > > +#define _CCU_MULT_MIN(_shift, _width, _min) \ > > > > + _CCU_MULT_OFF_MIN_MAX(_shift, _width, 1, _min, 0) > > > > + > > > > +#define _CCU_MULT(_shift, _width) \ > > > > + _CCU_MULT_OFF_MIN_MAX(_shift, _width, 1, 1, 0) > > > > + > > > > +/** > > > > + * struct ccu_mux - ccu clock multiplexer > > > > + * > > > > + * @shift: multiplexer shift value > > > > + * @width: multiplexer width value > > > > + */ > > > > +struct ccu_mux { > > > > + u8 shift; > > > > + u8 width; > > > > +}; > > > > + > > > > +#define _CCU_MUX(_shift, _width) { \ > > > > + .shift = _shift, \ > > > > + .width = _width, \ > > > > +} > > > > + > > > > +/** > > > > + * struct ccu_div - ccu clock divider > > > > + * > > > > + * @shift: divider shift value > > > > + * @width: divider width value > > > > + * @offset: divider offset > > > > + * @max: maximum divider value > > > > + */ > > > > +struct ccu_div { > > > > + u8 shift; > > > > + u8 width; > > > > + u32 offset; > > > > + u32 max; > > > > +}; > > > > + > > > > +#define _CCU_DIV(_shift, _width) { \ > > > > + .shift = _shift, \ > > > > + .width = _width, \ > > > > + .offset = 1, \ > > > > + .max = 0, \ > > > > +} > > > > + > > > > +/** > > > > + * struct ccu_clk_tree - ccu clock tree > > > > + * > > > > + * @parent: parent clock tree > > > > + * @type: clock type > > > > + * @off: clock tree offset > > > > + * @m: divider m > > > > + * @p: divider p > > > > + * @mux: multiplexer mux > > > > + * @post: post divider value > > > > + * @n: multiplier n > > > > + * @k: multiplier k > > > > + * @fixed_rate: fixed rate > > > > + * @flags: clock tree flags > > > > + */ > > > > +struct ccu_clk_tree { > > > > + const unsigned long *parent; > > > > > > Shouldn't that be called "parents" instead? > > > > > > > + enum ccu_clk_type type; > > > > + u16 off; > > > > + > > > > + struct ccu_div m; > > > > + struct ccu_div p; > > > > + struct ccu_mux mux; > > > > + unsigned int postdiv; > > > > + > > > > + struct ccu_mult n; > > > > + struct ccu_mult k; > > > > + > > > > + ulong fixed_rate; > > > > + enum ccu_clk_flags flags; > > > > +}; > > > > + > > > > +#define TREE(_parent, _type, _off, \ > > > > > > Just a nit, but TREE is somewhat confusing here, as this just > > > constructs a single entry in an array. The tree property is > > > realised through the parent array, if I get this correctly. > > > So should we name this ENTRY or CLK_ENTRY instead? > > > > > > > + _m, _p, \ > > > > + _mux, \ > > > > + _postdiv, \ > > > > + _n, _k, \ > > > > + _fixed_rate, \ > > > > + _flags) { \ > > > > + .parent = _parent, \ > > > > + .type = _type, \ > > > > + .off = _off, \ > > > > + .m = _m, \ > > > > + .p = _p, \ > > > > + .mux = _mux, \ > > > > + .postdiv = _postdiv, \ > > > > + .n = _n, \ > > > > + .k = _k, \ > > > > + .fixed_rate = _fixed_rate, \ > > > > + .flags = _flags, \ > > > > +} > > > > + > > > > +#define > > > > MISC(_parent) \ > > > > + TREE(_parent, CCU_CLK_TYPE_MISC, 0, \ > > > > + {0}, {0}, \ > > > > + {0}, \ > > > > + 0, \ > > > > + {0}, {0}, \ > > > > + 0, \ > > > > + CCU_CLK_F_INIT_DONE) > > > > > > If MISC is really something like GATE or SIMPLE, would it be > > > possible to construct the single element array here, so that the > > > macro just takes the one parent clock ID here, instead of referring > > > to an extra array? > > > > + > > > > +#define FIXED(_fixed_rate) \ > > > > + TREE(NULL, CCU_CLK_TYPE_FIXED, 0, \ > > > > + {0}, {0}, \ > > > > + {0}, \ > > > > + 0, \ > > > > + {0}, {0}, \ > > > > + _fixed_rate, \ > > > > + CCU_CLK_F_INIT_DONE) > > > > + > > > > +#define NK(_parent, _off, \ > > > > + _nshift, _nwidth, \ > > > > + _kshift, _kwidth, _kmin, \ > > > > + _postdiv, \ > > > > + _flags) \ > > > > + TREE(_parent, CCU_CLK_TYPE_NK, _off, \ > > > > + {0}, {0}, \ > > > > + {0}, \ > > > > + _postdiv, \ > > > > + _CCU_MULT(_nshift, _nwidth), \ > > > > + _CCU_MULT_MIN(_kshift, _kwidth, _kmin), \ > > > > + 0, \ > > > > + CCU_CLK_F_INIT_DONE | _flags) > > > > + > > > > +#define MP(_parent, _off, \ > > > > + _mshift, _mwidth, \ > > > > + _pshift, _pwidth, \ > > > > + _muxshift, _muxwidth, \ > > > > + _postdiv, \ > > > > + _flags) \ > > > > + TREE(_parent, CCU_CLK_TYPE_MP, _off, \ > > > > + _CCU_DIV(_mshift, _mwidth), \ > > > > + _CCU_DIV(_pshift, _pwidth), \ > > > > + _CCU_MUX(_muxshift, _muxwidth), \ > > > > + _postdiv, \ > > > > + {0}, {0}, \ > > > > + 0, \ > > > > + CCU_CLK_F_INIT_DONE | _flags) > > > > + > > > > /** > > > > * struct ccu_clk_gate - ccu clock gate > > > > * @off: gate offset > > > > @@ -59,6 +248,7 @@ struct ccu_reset { > > > > * @resets: reset unit > > > > */ > > > > struct ccu_desc { > > > > + const struct ccu_clk_tree *tree; > > > > const struct ccu_clk_gate *gates; > > > > const struct ccu_reset *resets; > > > > }; > > > > diff --git a/drivers/clk/sunxi/clk_a64.c > > > > b/drivers/clk/sunxi/clk_a64.c index 162ec769d6..1d0cd98183 100644 > > > > --- a/drivers/clk/sunxi/clk_a64.c > > > > +++ b/drivers/clk/sunxi/clk_a64.c > > > > @@ -12,6 +12,45 @@ > > > > #include <dt-bindings/clock/sun50i-a64-ccu.h> > > > > #include <dt-bindings/reset/sun50i-a64-ccu.h> > > > > > > > > +#define CLK_APB2 26 > > > > +#define CLK_OSC_32K (CLK_GPU + 1) > > > > +#define CLK_OSC_24M (CLK_OSC_32K + 1) > > > > + > > > > +static const unsigned long periph0_parents[] = { > > > > > > Why is this long? That's the DT clock index, which is 32 bit wide, > > > right? Just unsigned int or uint32_t should be sufficient. long is > > > quite a treacherous type to use, especially as we share code > > > between 32 and 64-bit architectures. > > > > > > > + CLK_OSC_24M, > > > > +}; > > > > + > > > > +static const unsigned long apb2_parents[] = { > > > > + CLK_OSC_32K, > > > > + CLK_OSC_24M, > > > > + CLK_PLL_PERIPH0, > > > > + CLK_PLL_PERIPH0, > > > > +}; > > > > + > > > > +static const unsigned long uart_parents[] = { > > > > + CLK_APB2, > > > > +}; > > > > + > > > > +static const struct ccu_clk_tree a64_tree[] = { > > > > + [CLK_OSC_32K] = FIXED(OSC_32K_ULL), > > > > + [CLK_OSC_24M] = FIXED(OSC_24M_ULL), > > > > + > > > > + [CLK_PLL_PERIPH0] = NK(periph0_parents, 0x028, > > > > + 8, 5, /* N */ > > > > + 4, 2, 2, /* K */ > > > > + 2, /* post-div */ > > > > + CCU_CLK_F_POSTDIV), > > > > + > > > > + [CLK_APB2] = MP(apb2_parents, 0x058, > > > > + 0, 5, /* M */ > > > > + 16, 2, /* P */ > > > > + 24, 2, /* mux */ > > > > + 0, > > > > + 0), > > > > + > > > > + [CLK_BUS_UART0] = MISC(uart_parents), > > > > +}; > > > > + > > > > static const struct ccu_clk_gate a64_gates[] = { > > > > [CLK_BUS_OTG] = GATE(0x060, BIT(23)), > > > > [CLK_BUS_EHCI0] = GATE(0x060, BIT(24)), > > > > @@ -52,6 +91,7 @@ static const struct ccu_reset a64_resets[] = { > > > > }; > > > > > > > > static const struct ccu_desc a64_ccu_desc = { > > > > + .tree = a64_tree, > > > > .gates = a64_gates, > > > > .resets = a64_resets, > > > > }; > > > > diff --git a/drivers/clk/sunxi/clk_sunxi.c > > > > b/drivers/clk/sunxi/clk_sunxi.c index 345d706c2a..2aebd257d1 > > > > 100644 --- a/drivers/clk/sunxi/clk_sunxi.c > > > > +++ b/drivers/clk/sunxi/clk_sunxi.c > > > > @@ -18,6 +18,187 @@ static const struct ccu_clk_gate > > > > *priv_to_gate(struct ccu_priv *priv, return > > > > &priv->desc->gates[id]; } > > > > > > > > +static const struct ccu_clk_tree *priv_to_tree(struct ccu_priv > > > > *priv, > > > > + unsigned long id) > > > > > > Again, why long here? Especially as it's u32 in the function below. > > > > > > > +{ > > > > + return &priv->desc->tree[id]; > > > > +} > > > > + > > > > +static int sunxi_get_parent_idx(const struct ccu_clk_tree *tree, > > > > void *base) +{ > > > > + u32 reg, idx; > > > > + > > > > + reg = readl(base + tree->off); > > > > + idx = reg >> tree->mux.shift; > > > > + idx &= (1 << tree->mux.width) - 1; > > > > + > > > > + return idx; > > > > +} > > > > + > > > > +static ulong sunxi_fixed_get_rate(struct clk *clk, unsigned long > > > > id) > > > > > > Same "long" question for both the return type and the id. > > > And for everything below. > > > > > > > +{ > > > > + struct ccu_priv *priv = dev_get_priv(clk->dev); > > > > + const struct ccu_clk_tree *tree = priv_to_tree(priv, id); > > > > + > > > > + if (!(tree->flags & CCU_CLK_F_INIT_DONE)) { > > > > + printf("%s: (CLK#%ld) unhandled\n", __func__, > > > > clk->id); > > > > + return 0; > > > > + } > > > > + > > > > + return tree->fixed_rate; > > > > +} > > > > + > > > > > > So why are there all those separate functions? Isn't that all the > > > same algorithm: adjust the parent rate based on the clock type? > > > I reworked this with recursive calls and it's MUCH less code: > > > > > > (fill the gaps, using your types for your convenience ;-) > > > > > > static ulong sunxi_apply_pll(struct ccu_priv *priv, ulong id, ulong > > > parent_rate) { NK algorithm of sunxi_nk_get_rate } > > > static ulong sunxi_apply_div(struct ccu_priv *priv, ulong id, ulong > > > parent_rate) { MP algorithm of sunxi_mp_get_rate } > > > static ulong sunxi_calc_clk_rate(struct ccu_priv *priv, ulong clkid) > > > { > > > .... > > > switch (tree->type) { > > > case CCU_CLK_TYPE_MISC: > > > return sunxi_calc_clk_rate(priv, tree->parent[0]); > > > case CCU_CLK_TYPE_FIXED: > > > return tree->fixed_rate; > > > case CCU_CLK_TYPE_NK: > > > rate = sunxi_calc_clk_rate(priv, > > > sunxi_get_parent_id(tree, priv->base)); > > > return sunxi_apply_pll(priv, clkid, rate); > > > (similar for _MP) > > > ... > > > } > > > > Initially I would tried the recursive and yes code can reduce but > > using recursive can leed more disadvantage in-terms of code tracing > > during long run. Due to all these factors I used simple function > > calls. > > But I find those extra functions much more confusing, due to the > similar names and their very similar functionality. Also it seems that > you just implemented what we need so far, so you will probably need to > extend those functions, making them even more similar and duplicating > more code. Basically you try to roll out the tree structure. > > Since the clocks are organised in a tree-like structure, I believe this > recursive definition is a much better fit: A clock takes one of > possibly multiple input clocks and adjusts this rate. Full stop. The > rest is then just connecting them to other clocks. > The code looks much simpler and is much smaller this way: > https://gist.github.com/apritzel/db93dd06b4defb46504bccbfe4fc2c20#file-sunxi_clk-c-L86-L112 > > Typically the recursion depth is just two or three levels, so I don't > buy the argument of code tracing. OK. agreed thanks. I shall take this recursive and try to mark separate patch on behalf of you if possible, will that be fine? He is my next version TODO. - Fixing clock types with meaning full values - Adopt recursive function calls - Try to add full MMC blot. Let me know if you have any comments or question so-that we can collaborate smooth to get CLK in as soon as possible. Jagan. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot