clk: Add Allwinner CLK, RESET support

Add common reset driver for all Allwinner SoC's.

Since CLK and RESET share common DT compatible, it is CLK driver job is to bind the reset driver. So add CLK bind call on respective SoC driver by passing ccu map descriptor so-that reset deassert, deassert operations held based on ccu reset table defined from CLK driver.

Select DM_RESET via CLK_SUNXI, this make hidden section of RESET since CLK and RESET share common DT compatible and code.

Signed-off-by Jagan Teki <[email protected]>

Acked-by Maxime Ripard <[email protected]>

---
 arch/arm/include/asm/arch-sunxi/ccu.h |  29 ++++++
 drivers/clk/sunxi/Kconfig             |   1 +
 drivers/clk/sunxi/clk_a64.c           |  20 +++++
 drivers/reset/Kconfig                 |   8 ++
 drivers/reset/Makefile                |   1 +
 drivers/reset/reset-sunxi.c           | 125 ++++++++++++++++++++++++++
 6 files changed, 184 insertions(+)
 create mode 100644 drivers/reset/reset-sunxi.c

arch/arm/include/asm/arch-sunxi/ccu.h +29 -0

index db69c8f0d5..3fdc26978d 100644
@@ -34,13 +34,33 @@ struct ccu_clk_gate {
 	.flags = CCU_CLK_F_INIT_DONE,		\
 }
 
+/**
+ * struct ccu_reset - ccu reset
+ * @off:	reset offset
+ * @bit:	reset bit
+ * @flags:	reset flags
+ */
+struct ccu_reset {
+	u16 off;
+	u32 bit;
+	enum ccu_clk_flags flags;
+};
+
+#define RESET(_off, _bit) {			\
+	.off = _off,				\
+	.bit = _bit,				\
+	.flags = CCU_CLK_F_INIT_DONE,		\
+}
+
 /**
  * struct ccu_desc - clock control unit descriptor
  *
  * @gates:	clock gates
+ * @resets:	reset unit
  */
 struct ccu_desc {
 	const struct ccu_clk_gate *gates;
+	const struct ccu_reset *resets;
 };
 
 /**

@@ -62,4 +82,13 @@ int sunxi_clk_probe(struct udevice *dev);
 
 extern struct clk_ops sunxi_clk_ops;
 
+/**
+ * sunxi_reset_bind() - reset binding
+ *
+ * @dev:       reset device
+ * @count:     reset count
+ * @return 0 success, or error value
+ */
+int sunxi_reset_bind(struct udevice *dev, ulong count);
+
 #endif /* _ASM_ARCH_CCU_H */

drivers/clk/sunxi/Kconfig +1 -0

index bf5ecb3801..041d711e58 100644
@@ -1,6 +1,7 @@
 config CLK_SUNXI
 	bool "Clock support for Allwinner SoCs"
 	depends on CLK && ARCH_SUNXI
+	select DM_RESET
 	default y
 	help
 	  This enables support for common clock driver API on Allwinner

drivers/clk/sunxi/clk_a64.c +11 -0

index 803a2f711d..28bda1f497 100644
@@ -10,6 +10,7 @@
 #include <errno.h>
 #include <asm/arch/ccu.h>
 #include <dt-bindings/clock/sun50i-a64-ccu.h>
+#include <dt-bindings/reset/sun50i-a64-ccu.h>
 
 static const struct ccu_clk_gate a64_gates[] = {
 	[CLK_BUS_OTG]		= GATE(0x060, BIT(23)),

@@ -26,10 +27,28 @@ static const struct ccu_clk_gate a64_gates[] = {
 	[CLK_USB_OHCI1]		= GATE(0x0cc, BIT(17)),
 };
 
+static const struct ccu_reset a64_resets[] = {
+	[RST_USB_PHY0]          = RESET(0x0cc, BIT(0)),
+	[RST_USB_PHY1]          = RESET(0x0cc, BIT(1)),
+	[RST_USB_HSIC]          = RESET(0x0cc, BIT(2)),
+
+	[RST_BUS_OTG]           = RESET(0x2c0, BIT(23)),
+	[RST_BUS_EHCI0]         = RESET(0x2c0, BIT(24)),
+	[RST_BUS_EHCI1]         = RESET(0x2c0, BIT(25)),
+	[RST_BUS_OHCI0]         = RESET(0x2c0, BIT(28)),
+	[RST_BUS_OHCI1]         = RESET(0x2c0, BIT(29)),

drivers/clk/sunxi/clk_a64.c +3 -0

+}
+
 static const struct udevice_id a64_ccu_ids[] = {
 	{ .compatible = "allwinner,sun50i-a64-ccu",
 	  .data = (ulong)&a64_ccu_desc },

@@ -43,4 +62,5 @@ U_BOOT_DRIVER(clk_sun50i_a64) = {
 	.priv_auto_alloc_size	= sizeof(struct ccu_priv),
 	.ops		= &sunxi_clk_ops,
 	.probe		= sunxi_clk_probe,
+	.bind		= a64_clk_bind,
 };

drivers/reset/Kconfig +8 -0

index 9c5208b7da..b6b40b6ce9 100644
@@ -106,4 +106,12 @@ config RESET_SOCFPGA
 	help
 	  Support for reset controller on SoCFPGA platform.
 
+config RESET_SUNXI
+	bool "RESET support for Allwinner SoCs"
+	depends on DM_RESET && ARCH_SUNXI
+	default y
+	help
+	  This enables support for common reset driver for
+	  Allwinner SoCs.
+
 endmenu

drivers/reset/Makefile +1 -0

index f4520878b7..377c038163 100644
@@ -17,3 +17,4 @@ obj-$(CONFIG_AST2500_RESET) += ast2500-reset.o
 obj-$(CONFIG_RESET_ROCKCHIP) += reset-rockchip.o
 obj-$(CONFIG_RESET_MESON) += reset-meson.o
 obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o
+obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o

drivers/reset/reset-sunxi.c new file +24 -0

new file mode 100644
index 0000000000..af63cac64e
@@ -0,0 +1,125 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (C) 2018 Amarula Solutions.
+ * Author: Jagan Teki <[email protected]>
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <errno.h>
+#include <reset-uclass.h>
+#include <asm/io.h>
+#include <dm/lists.h>
+#include <linux/log2.h>
+#include <asm/arch/ccu.h>
+
+struct sunxi_reset_priv {
+	void *base;
+	ulong count;
+	const struct ccu_desc *desc;
+};
+
+static const struct ccu_reset *priv_to_reset(struct sunxi_reset_priv *priv,
+					     unsigned long id)
+{

drivers/reset/reset-sunxi.c new file +91 -0

+	if (reset_ctl->id >= priv->count)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int sunxi_reset_free(struct reset_ctl *reset_ctl)
+{
+	debug("%s: (RST#%ld)\n", __func__, reset_ctl->id);
+
+	return 0;
+}
+
+static int sunxi_set_reset(struct reset_ctl *reset_ctl, bool on)
+{
+	struct sunxi_reset_priv *priv = dev_get_priv(reset_ctl->dev);
+	const struct ccu_reset *reset = priv_to_reset(priv, reset_ctl->id);
+	u32 reg;
+
+	if (!(reset->flags & CCU_CLK_F_INIT_DONE)) {
+		printf("%s: (RST#%ld) unhandled\n", __func__, reset_ctl->id);
+		return 0;
+	}
+
+	debug("%s: (RST#%ld) off#0x%x, BIT(%d)\n", __func__,
+	      reset_ctl->id, reset->off, ilog2(reset->bit));
+
+	reg = readl(priv->base + reset->off);
+	if (on)
+		reg |= reset->bit;
+	else
+		reg &= ~reset->bit;
+
+	writel(reg, priv->base + reset->off);
+
+	return 0;
+}
+
+static int sunxi_reset_assert(struct reset_ctl *reset_ctl)
+{
+	return sunxi_set_reset(reset_ctl, false);
+}
+
+static int sunxi_reset_deassert(struct reset_ctl *reset_ctl)
+{
+	return sunxi_set_reset(reset_ctl, true);
+}
+
+struct reset_ops sunxi_reset_ops = {
+	.request = sunxi_reset_request,
+	.free = sunxi_reset_free,
+	.rst_assert = sunxi_reset_assert,
+	.rst_deassert = sunxi_reset_deassert,
+};
+
+static int sunxi_reset_probe(struct udevice *dev)
+{
+	struct sunxi_reset_priv *priv = dev_get_priv(dev);
+
+	priv->base = dev_read_addr_ptr(dev);
+
+	return 0;
+}
+
+int sunxi_reset_bind(struct udevice *dev, ulong count)
+{
+	struct udevice *rst_dev;
+	struct sunxi_reset_priv *priv;
+	int ret;
+
+	ret = device_bind_driver_to_node(dev, "sunxi_reset", "reset",
+					 dev_ofnode(dev), &rst_dev);
+	if (ret) {
+		debug("failed to bind sunxi_reset driver (ret=%d)\n", ret);
+		return ret;
+	}
+	priv = malloc(sizeof(struct sunxi_reset_priv));
+	priv->count = count;
+	priv->desc = (const struct ccu_desc *)dev_get_driver_data(dev);
+	rst_dev->priv = priv;
+
+	return 0;
+}
+
+U_BOOT_DRIVER(sunxi_reset) = {
+	.name		= "sunxi_reset",
+	.id		= UCLASS_RESET,
+	.ops		= &sunxi_reset_ops,
+	.probe		= sunxi_reset_probe,
+	.priv_auto_alloc_size = sizeof(struct sunxi_reset_priv),
+};

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.

Tree clocks are parent clock type, fixed clocks, mp, nk, nkm, nkmp, pre/post div, flags etc. which were managed via ccu_clock_tree.

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 <[email protected]>

---
 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(-)

arch/arm/include/asm/arch-sunxi/ccu.h +0 -0

index 3fdc26978d..61b8c36b3b 100644

arch/arm/include/asm/arch-sunxi/ccu.h +79 -0

+ * @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

arch/arm/include/asm/arch-sunxi/ccu.h +6 -0

+	enum ccu_clk_type type;
+	u16 off;
+
+	struct ccu_div m;
+	struct ccu_div p;
+	struct ccu_mux mux;

arch/arm/include/asm/arch-sunxi/ccu.h +17 -0

+	     _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,					\

arch/arm/include/asm/arch-sunxi/ccu.h +40 -0

+
+#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;
 };

drivers/clk/sunxi/clk_a64.c +0 -0

index 162ec769d6..1d0cd98183 100644

drivers/clk/sunxi/clk_a64.c +35 -0

+	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,
 };

drivers/clk/sunxi/clk_sunxi.c +0 -0

index 345d706c2a..2aebd257d1 100644

drivers/clk/sunxi/clk_sunxi.c +6 -0

+{
+	return &priv->desc->tree[id];
+}
+
+static int sunxi_get_parent_idx(const struct ccu_clk_tree *tree, void *base)
+{

drivers/clk/sunxi/clk_sunxi.c +152 -0

+static ulong sunxi_nk_get_parent_rate(struct clk *clk, unsigned long id)
+{
+	struct ccu_priv *priv = dev_get_priv(clk->dev);
+	const struct ccu_clk_tree *tree = priv_to_tree(priv, id);
+	ulong rate = 0;
+
+	switch (tree->type) {
+	case CCU_CLK_TYPE_FIXED:
+		rate = sunxi_fixed_get_rate(clk, id);
+		break;
+	default:
+		printf("%s: Unknown (TYPE#%d)\n", __func__, tree->type);
+		break;
+	}
+
+	return rate;
+}
+
+static ulong sunxi_nk_get_rate(struct clk *clk, unsigned long id)
+{
+	struct ccu_priv *priv = dev_get_priv(clk->dev);
+	const struct ccu_clk_tree *tree = priv_to_tree(priv, id);
+	ulong rate, parent_rate;
+	unsigned long n, k;
+	u32 reg;
+
+	parent_rate = sunxi_nk_get_parent_rate(clk, tree->parent[0]);
+
+	reg = readl(priv->base + tree->off);
+
+	n = reg >> tree->n.shift;
+	n &= (1 << tree->n.width) - 1;
+	n += tree->n.offset;
+	if (!n)
+		n++;
+
+	k = reg >> tree->k.shift;
+	k &= (1 << tree->k.width) - 1;
+	k += tree->k.offset;
+	if (!k)
+		k++;
+
+	rate = parent_rate * n * k;
+	if (tree->flags & CCU_CLK_F_POSTDIV)
+		rate /= tree->postdiv;
+
+	return rate;
+}
+
+static ulong sunxi_mp_get_parent_rate(struct clk *clk, unsigned long id)
+{
+	struct ccu_priv *priv = dev_get_priv(clk->dev);
+	const struct ccu_clk_tree *tree = priv_to_tree(priv, id);
+	ulong rate = 0;
+
+	if (!(tree->flags & CCU_CLK_F_INIT_DONE)) {
+		printf("%s: (CLK#%ld) unhandled\n", __func__, clk->id);
+		return 0;
+	}
+
+	switch (tree->type) {
+	case CCU_CLK_TYPE_FIXED:
+		rate = sunxi_fixed_get_rate(clk, id);
+		break;
+	case CCU_CLK_TYPE_NK:
+		rate = sunxi_nk_get_rate(clk, id);
+		break;
+	default:
+		printf("%s: (TYPE#%d) unhandled\n", __func__, tree->type);
+		break;
+	}
+
+	return rate;
+}
+
+static ulong sunxi_mp_get_rate(struct clk *clk, unsigned long id)
+{
+	struct ccu_priv *priv = dev_get_priv(clk->dev);
+	const struct ccu_clk_tree *tree = priv_to_tree(priv, id);
+	unsigned int m, p;
+	ulong parent_rate;
+	u32 reg, idx;
+
+	idx = sunxi_get_parent_idx(tree, priv->base);
+	if (idx < 0) {
+		printf("%s: Wrong parent index %d\n", __func__, idx);
+		return 0;
+	}
+
+	parent_rate = sunxi_mp_get_parent_rate(clk, tree->parent[idx]);
+
+	reg = readl(priv->base + tree->off);
+
+	m = reg >> tree->m.shift;
+	m &= (1 << tree->m.width) - 1;
+	m += tree->m.offset;
+	if (!m)
+		m++;
+
+	p = reg >> tree->p.shift;
+	p &= (1 << tree->p.width) - 1;
+
+	return (parent_rate >> p) / m;
+}
+
+static ulong sunxi_misc_get_rate(struct clk *clk, unsigned long id)
+{
+	struct ccu_priv *priv = dev_get_priv(clk->dev);
+	const struct ccu_clk_tree *tree = priv_to_tree(priv, id);
+	ulong rate = 0;
+
+	if (!(tree->flags & CCU_CLK_F_INIT_DONE)) {
+		printf("%s: (CLK#%ld) unhandled\n", __func__, clk->id);
+		return 0;
+	}
+
+	switch (tree->type) {
+	case CCU_CLK_TYPE_MP:
+		rate = sunxi_mp_get_rate(clk, id);
+		break;
+	default:
+		printf("%s: (TYPE#%d) unhandled\n", __func__, tree->type);
+		break;
+	}
+
+	return rate;
+}
+
+static ulong sunxi_clk_get_rate(struct clk *clk)
+{
+	struct ccu_priv *priv = dev_get_priv(clk->dev);
+	const struct ccu_clk_tree *tree = priv_to_tree(priv, clk->id);
+	ulong rate = 0;
+
+	if (!(tree->flags & CCU_CLK_F_INIT_DONE)) {
+		printf("%s: (CLK#%ld) unhandled\n", __func__, clk->id);
+		return 0;
+	}
+
+	switch (tree->type) {
+	case CCU_CLK_TYPE_MISC:
+		rate = sunxi_misc_get_rate(clk, tree->parent[0]);
+		break;
+	default:
+		printf("%s: (TYPE#%d) unhandled\n", __func__, tree->type);
+		break;
+	}
+
+	return rate;
+}
+
 static int sunxi_set_gate(struct clk *clk, bool on)
 {
 	struct ccu_priv *priv = dev_get_priv(clk->dev);

@@ -56,6 +237,7 @@ static int sunxi_clk_disable(struct clk *clk)
 struct clk_ops sunxi_clk_ops = {
 	.enable = sunxi_clk_enable,
 	.disable = sunxi_clk_disable,
+	.get_rate = sunxi_clk_get_rate,
 };
 
 int sunxi_clk_probe(struct udevice *dev)

Quoted 6 lines · Jagan Teki

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.

Quoted 2 lines

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.

Quoted 22 lines

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 <[email protected]>

---
 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(-)

arch/arm/include/asm/arch-sunxi/ccu.h +0 -0

index 3fdc26978d..61b8c36b3b 100644

arch/arm/include/asm/arch-sunxi/ccu.h line 10

Quoted 4 lines

arch/arm/include/asm/arch-sunxi/ccu.h +1 -0

@@ -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.

arch/arm/include/asm/arch-sunxi/ccu.h line 16

Quoted 6 lines

arch/arm/include/asm/arch-sunxi/ccu.h +6 -0

+#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?

arch/arm/include/asm/arch-sunxi/ccu.h line 19

Quoted 3 lines

arch/arm/include/asm/arch-sunxi/ccu.h +3 -0

+ * @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

Quoted 71 lines · Jagan Teki

Quoted 7 lines

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.

Quoted 3 lines

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.

Quoted 19 lines

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 <[email protected]>

---
 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(-)

(quoted diff) +0 -0

Quoted 8 lines

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.

arch/arm/include/asm/arch-sunxi/ccu.h line 16

Quoted 6 lines

arch/arm/include/asm/arch-sunxi/ccu.h +6 -0

+#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?

Quoted 8 lines

Quoted 3 lines

+ * @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?

Quoted 13 lines

Quoted 4 lines

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?

Quoted 8 lines

Quoted 3 lines

+ * @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

Quoted 5 lines

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.

Quoted 6 lines · Jagan Teki

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.

Quoted 3 lines

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.

Quoted 22 lines

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 <[email protected]>

---
 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(-)

arch/arm/include/asm/arch-sunxi/ccu.h +0 -0

index 3fdc26978d..61b8c36b3b 100644

arch/arm/include/asm/arch-sunxi/ccu.h line 10

Quoted 4 lines

arch/arm/include/asm/arch-sunxi/ccu.h +1 -0

@@ -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.

arch/arm/include/asm/arch-sunxi/ccu.h line 16

Quoted 6 lines

arch/arm/include/asm/arch-sunxi/ccu.h +6 -0

+#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?

arch/arm/include/asm/arch-sunxi/ccu.h line 19

Quoted 4 lines

arch/arm/include/asm/arch-sunxi/ccu.h +3 -0

+ * @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.

Quoted 9 lines

arch/arm/include/asm/arch-sunxi/ccu.h +8 -0

+ */
+enum ccu_clk_type {
+     CCU_CLK_TYPE_MISC               = 0,
+     CCU_CLK_TYPE_FIXED              = 1,
+     CCU_CLK_TYPE_MP                 = 2,
+     CCU_CLK_TYPE_NK                 = 3,
+};
+
 /**

arch/arm/include/asm/arch-sunxi/ccu.h line 31

Quoted 4 lines

arch/arm/include/asm/arch-sunxi/ccu.h +1 -1

  * 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.

Quoted 85 lines

arch/arm/include/asm/arch-sunxi/ccu.h +79 -0

+ * @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

arch/arm/include/asm/arch-sunxi/ccu.h line 125

Quoted 10 lines

arch/arm/include/asm/arch-sunxi/ccu.h +10 -0

+ * @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?

Quoted 6 lines

arch/arm/include/asm/arch-sunxi/ccu.h +6 -0

+     enum ccu_clk_type type;
+     u16 off;
+
+     struct ccu_div m;
+     struct ccu_div p;
+     struct ccu_mux mux;

arch/arm/include/asm/arch-sunxi/ccu.h line 141

Quoted 10 lines

arch/arm/include/asm/arch-sunxi/ccu.h +10 -0

+     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?

Quoted 17 lines

arch/arm/include/asm/arch-sunxi/ccu.h +17 -0

+          _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,                                        \

arch/arm/include/asm/arch-sunxi/ccu.h line 168

Quoted 10 lines

arch/arm/include/asm/arch-sunxi/ccu.h +10 -0

+}
+
+#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?

Quoted 55 lines

arch/arm/include/asm/arch-sunxi/ccu.h +40 -0

+
+#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;
 };

drivers/clk/sunxi/clk_a64.c +0 -0

index 162ec769d6..1d0cd98183 100644

drivers/clk/sunxi/clk_a64.c line 19

Quoted 8 lines

drivers/clk/sunxi/clk_a64.c +5 -0

@@ -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.

Quoted 50 lines

drivers/clk/sunxi/clk_a64.c +35 -0

+     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,
 };

drivers/clk/sunxi/clk_sunxi.c +0 -0

index 345d706c2a..2aebd257d1 100644

drivers/clk/sunxi/clk_sunxi.c static const struct ccu_clk_gate *priv_to_gate(struct ccu_priv *priv, · line 22

Quoted 5 lines

drivers/clk/sunxi/clk_sunxi.c +2 -0

@@ -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.

Quoted 6 lines

drivers/clk/sunxi/clk_sunxi.c +6 -0

+{
+     return &priv->desc->tree[id];
+}
+
+static int sunxi_get_parent_idx(const struct ccu_clk_tree *tree, void *base)
+{

drivers/clk/sunxi/clk_sunxi.c static const struct ccu_clk_gate *priv_to_gate(struct ccu_priv *priv, · line 38

Quoted 10 lines

drivers/clk/sunxi/clk_sunxi.c +10 -0

+     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.

drivers/clk/sunxi/clk_sunxi.c static const struct ccu_clk_gate *priv_to_gate(struct ccu_priv *priv, · line 50

Quoted 12 lines

drivers/clk/sunxi/clk_sunxi.c +12 -0

+{
+     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)

...
}

Quoted 10 lines · André Przywara

Quoted 7 lines · Jagan Teki

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.

Quoted 8 lines

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.

Quoted 44 lines

Quoted 4 lines

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.

Quoted 19 lines

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 <[email protected]>

---
 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(-)

(quoted diff) +0 -0

Quoted 8 lines

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.

Quoted 11 lines

arch/arm/include/asm/arch-sunxi/ccu.h line 16

Quoted 6 lines

arch/arm/include/asm/arch-sunxi/ccu.h +6 -0

+#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.

Quoted 21 lines

Quoted 3 lines

+ * @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.

Quoted 10 lines

+ */

arch/arm/include/asm/arch-sunxi/ccu.h +7 -0

+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

Quoted 8 lines

Quoted 4 lines

*
- * @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.

and eventually the same verified in code whether the init done or not.

Quoted 340 lines

Quoted 86 lines

+ * @CCU_CLK_F_POSTDIV: clock post divider
*/
enum ccu_clk_flags {
CCU_CLK_F_INIT_DONE = BIT(0),

arch/arm/include/asm/arch-sunxi/ccu.h +19 -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, \

arch/arm/include/asm/arch-sunxi/ccu.h +59 -0

+                           _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

arch/arm/include/asm/arch-sunxi/ccu.h line 125

Quoted 10 lines

arch/arm/include/asm/arch-sunxi/ccu.h +10 -0

+ * @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?

Quoted 6 lines

arch/arm/include/asm/arch-sunxi/ccu.h +6 -0

+     enum ccu_clk_type type;
+     u16 off;
+
+     struct ccu_div m;
+     struct ccu_div p;
+     struct ccu_mux mux;

arch/arm/include/asm/arch-sunxi/ccu.h line 141

Quoted 10 lines

arch/arm/include/asm/arch-sunxi/ccu.h +10 -0

+     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?

Quoted 21 lines

arch/arm/include/asm/arch-sunxi/ccu.h +20 -0

+          _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) \

arch/arm/include/asm/arch-sunxi/ccu.h line 168

Quoted 7 lines

arch/arm/include/asm/arch-sunxi/ccu.h +7 -0

+     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?

Quoted 55 lines

arch/arm/include/asm/arch-sunxi/ccu.h +40 -0

+
+#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;
 };

(quoted diff) +0 -0

b/drivers/clk/sunxi/clk_a64.c index 162ec769d6..1d0cd98183 100644

drivers/clk/sunxi/clk_a64.c line 19

Quoted 8 lines

drivers/clk/sunxi/clk_a64.c +5 -0

@@ -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.

Quoted 50 lines

drivers/clk/sunxi/clk_a64.c +35 -0

+     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,
 };

(quoted diff) +0 -0

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

Review static const struct ccu_clk_gate · line 23

Quoted 6 lines

(quoted diff) +2 -0

@@ -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.

Quoted 7 lines

drivers/clk/sunxi/clk_sunxi.c +6 -0

+{
+     return &priv->desc->tree[id];
+}
+
+static int sunxi_get_parent_idx(const struct ccu_clk_tree *tree,
 void *base) +{
+     u32 reg, idx;

drivers/clk/sunxi/clk_sunxi.c static const struct ccu_clk_gate *priv_to_gate(struct ccu_priv *priv,

Quoted 10 lines

drivers/clk/sunxi/clk_sunxi.c +9 -0

+
+     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.

drivers/clk/sunxi/clk_sunxi.c static const struct ccu_clk_gate *priv_to_gate(struct ccu_priv *priv, · line 50

Quoted 13 lines

drivers/clk/sunxi/clk_sunxi.c +6 -0

+{
+     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);

drivers/clk/sunxi/clk_sunxi.c +6 -0

+             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)

Quoted 2 lines

...
}

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.

On Tue, 8 Jan 2019 16:27:14 +0530
Jagan Teki <[email protected]> wrote:

Hi,

Quoted 86 lines

Quoted 10 lines · André Przywara

Quoted 7 lines · Jagan Teki

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.

Quoted 8 lines

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.

Quoted 44 lines

Quoted 4 lines

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.

Quoted 19 lines

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 <[email protected]>

---
 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(-)

(quoted diff) +0 -0

Quoted 8 lines

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.

Quoted 11 lines

arch/arm/include/asm/arch-sunxi/ccu.h line 16

Quoted 6 lines

arch/arm/include/asm/arch-sunxi/ccu.h +6 -0

+#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?

Quoted 4 lines

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.

Quoted 31 lines

Quoted 30 lines

Quoted 4 lines

+ * @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.

Quoted 13 lines

+ */

arch/arm/include/asm/arch-sunxi/ccu.h +8 -1

+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

Quoted 1 line

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.

Quoted 3 lines

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.

Quoted 346 lines

and eventually the same verified in code whether the init done or not.

Quoted 340 lines

Quoted 86 lines

+ * @CCU_CLK_F_POSTDIV: clock post divider
*/
enum ccu_clk_flags {
CCU_CLK_F_INIT_DONE = BIT(0),

arch/arm/include/asm/arch-sunxi/ccu.h +19 -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, \

arch/arm/include/asm/arch-sunxi/ccu.h +59 -0

+                           _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

arch/arm/include/asm/arch-sunxi/ccu.h line 125

Quoted 10 lines

arch/arm/include/asm/arch-sunxi/ccu.h +10 -0

+ * @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?

Quoted 6 lines

arch/arm/include/asm/arch-sunxi/ccu.h +6 -0

+     enum ccu_clk_type type;
+     u16 off;
+
+     struct ccu_div m;
+     struct ccu_div p;
+     struct ccu_mux mux;

arch/arm/include/asm/arch-sunxi/ccu.h line 141

Quoted 10 lines

arch/arm/include/asm/arch-sunxi/ccu.h +10 -0

+     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?

Quoted 21 lines

arch/arm/include/asm/arch-sunxi/ccu.h +20 -0

+          _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) \

arch/arm/include/asm/arch-sunxi/ccu.h line 168

Quoted 7 lines

arch/arm/include/asm/arch-sunxi/ccu.h +7 -0

+     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?

Quoted 55 lines

arch/arm/include/asm/arch-sunxi/ccu.h +40 -0

+
+#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;
 };

(quoted diff) +0 -0

b/drivers/clk/sunxi/clk_a64.c index 162ec769d6..1d0cd98183 100644

drivers/clk/sunxi/clk_a64.c line 19

Quoted 8 lines

drivers/clk/sunxi/clk_a64.c +5 -0

@@ -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.

Quoted 50 lines

drivers/clk/sunxi/clk_a64.c +35 -0

+     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,
 };

(quoted diff) +0 -0

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

Review static const struct ccu_clk_gate · line 23

Quoted 6 lines

(quoted diff) +2 -0

@@ -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.

Quoted 7 lines

drivers/clk/sunxi/clk_sunxi.c +6 -0

+{
+     return &priv->desc->tree[id];
+}
+
+static int sunxi_get_parent_idx(const struct ccu_clk_tree *tree,
 void *base) +{
+     u32 reg, idx;

drivers/clk/sunxi/clk_sunxi.c static const struct ccu_clk_gate *priv_to_gate(struct ccu_priv *priv,

Quoted 10 lines

drivers/clk/sunxi/clk_sunxi.c +9 -0

+
+     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.

drivers/clk/sunxi/clk_sunxi.c static const struct ccu_clk_gate *priv_to_gate(struct ccu_priv *priv, · line 50

Quoted 13 lines

drivers/clk/sunxi/clk_sunxi.c +6 -0

+{
+     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);

drivers/clk/sunxi/clk_sunxi.c +6 -0

+             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)

Quoted 2 lines

...
}

Quoted 4 lines

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.

Quoted 102 lines · Andre Przywara

On Tue, 8 Jan 2019 16:27:14 +0530
Jagan Teki <[email protected]> wrote:

Hi,

Quoted 86 lines

Quoted 10 lines · André Przywara

Quoted 7 lines · Jagan Teki

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.

Quoted 8 lines

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.

Quoted 44 lines

Quoted 4 lines

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.

Quoted 19 lines

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 <[email protected]>

---
 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(-)

(quoted diff) +0 -0

Quoted 8 lines

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.

Quoted 11 lines

arch/arm/include/asm/arch-sunxi/ccu.h line 16

Quoted 6 lines

arch/arm/include/asm/arch-sunxi/ccu.h +6 -0

+#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?

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 35 lines

Quoted 34 lines

Quoted 22 lines

Quoted 4 lines

+ * @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.

Quoted 10 lines

+ */

arch/arm/include/asm/arch-sunxi/ccu.h +7 -0

+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

Quoted 8 lines

Quoted 4 lines

*
- * @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.

Quoted 3 lines

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.

Quoted 363 lines

Quoted 345 lines

and eventually the same verified in code whether the init done or not.

Quoted 340 lines

Quoted 86 lines

+ * @CCU_CLK_F_POSTDIV: clock post divider
*/
enum ccu_clk_flags {
CCU_CLK_F_INIT_DONE = BIT(0),

arch/arm/include/asm/arch-sunxi/ccu.h +19 -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, \

arch/arm/include/asm/arch-sunxi/ccu.h +59 -0

+                           _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

arch/arm/include/asm/arch-sunxi/ccu.h line 125

Quoted 10 lines

arch/arm/include/asm/arch-sunxi/ccu.h +10 -0

+ * @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?

Quoted 6 lines

arch/arm/include/asm/arch-sunxi/ccu.h +6 -0

+     enum ccu_clk_type type;
+     u16 off;
+
+     struct ccu_div m;
+     struct ccu_div p;
+     struct ccu_mux mux;

arch/arm/include/asm/arch-sunxi/ccu.h line 141

Quoted 10 lines

arch/arm/include/asm/arch-sunxi/ccu.h +10 -0

+     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?

Quoted 21 lines

arch/arm/include/asm/arch-sunxi/ccu.h +20 -0

+          _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) \

arch/arm/include/asm/arch-sunxi/ccu.h line 168

Quoted 7 lines

arch/arm/include/asm/arch-sunxi/ccu.h +7 -0

+     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?

Quoted 55 lines

arch/arm/include/asm/arch-sunxi/ccu.h +40 -0

+
+#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;
 };

(quoted diff) +0 -0

b/drivers/clk/sunxi/clk_a64.c index 162ec769d6..1d0cd98183 100644

drivers/clk/sunxi/clk_a64.c line 19

Quoted 8 lines

drivers/clk/sunxi/clk_a64.c +5 -0

@@ -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.

Quoted 50 lines

drivers/clk/sunxi/clk_a64.c +35 -0

+     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,
 };

(quoted diff) +0 -0

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

Review static const struct ccu_clk_gate · line 23

Quoted 6 lines

(quoted diff) +2 -0

@@ -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.

Quoted 7 lines

drivers/clk/sunxi/clk_sunxi.c +6 -0

+{
+     return &priv->desc->tree[id];
+}
+
+static int sunxi_get_parent_idx(const struct ccu_clk_tree *tree,
 void *base) +{
+     u32 reg, idx;

drivers/clk/sunxi/clk_sunxi.c static const struct ccu_clk_gate *priv_to_gate(struct ccu_priv *priv,

Quoted 10 lines

drivers/clk/sunxi/clk_sunxi.c +9 -0

+
+     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.

drivers/clk/sunxi/clk_sunxi.c static const struct ccu_clk_gate *priv_to_gate(struct ccu_priv *priv, · line 50

Quoted 13 lines

drivers/clk/sunxi/clk_sunxi.c +6 -0

+{
+     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);

drivers/clk/sunxi/clk_sunxi.c +6 -0

+             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)

Quoted 2 lines

...
}

Quoted 4 lines

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