On Thu, Jan 10, 2019 at 6:21 AM André Przywara <andre.przyw...@arm.com> wrote: > > On 08/01/2019 19:12, Jagan Teki wrote: > > 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. > > Great, thanks! > > >>> 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? > > No need, just rework this into this patch and keep your authorship. You > did the bulk of the work anyway. If someone complains about the code, > send them to me ;-) > > > 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. > > Sure, lets nail this in the upcoming merge window. > > Btw. I still get the warning for DM_USB, shouldn't this be fixed already > by this series?
We already have DM_USB enabled, since the check will also look for BLK it's still showing the warning. Once we move DM_MMC it will gone. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot