From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maxime Ripard Date: Mon, 7 Jan 2019 14:01:01 +0100 Subject: [U-Boot] [PATCH v5 15/26] clk: sunxi: Add ccu clock tree support In-Reply-To: <799c63e9-e863-674c-efbe-c9c6d2ada20d@arm.com> References: <20181231165927.13803-1-jagan@amarulasolutions.com> <20181231165927.13803-16-jagan@amarulasolutions.com> <799c63e9-e863-674c-efbe-c9c6d2ada20d@arm.com> Message-ID: <20190107130100.6esckcmtveqpxbwj@flea> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: u-boot@lists.denx.de On Mon, Jan 07, 2019 at 01:03:33AM +0000, Andr=C3=A9 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. > >=20 > > 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. >=20 > 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. >=20 > > Tree clocks are parent clock type, fixed clocks, mp, nk, nkm, nkmp, > > pre/post div, flags etc. which were managed via ccu_clock_tree. >=20 > 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. >=20 > > 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. > >=20 > > 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. > >=20 > > Signed-off-by: Jagan Teki > > --- > > 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(-) > >=20 > > diff --git a/arch/arm/include/asm/arch-sunxi/ccu.h b/arch/arm/include/a= sm/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 > > =20 > > +#define OSC_32K_ULL 32000ULL >=20 > 32768 >=20 > 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. >=20 > > +#define OSC_24M_ULL 24000000ULL > > + > > +/** > > + * enum ccu_clk_type - ccu clock types > > + * > > + * @CCU_CLK_TYPE_MISC: misc clock type >=20 > 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 GAT= E? >=20 > > + * @CCU_CLK_TYPE_FIXED: fixed clock type > > + * @CCU_CLK_TYPE_MP: mp clock type > > + * @CCU_CLK_TYPE_NK: nk clock type >=20 > 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 --=20 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: