[RFC,v1,3/9] clk: renesas: add R906G032 driver

Clock driver for the Renesas RZ/N1 SoC family. This is based
on the Linux kernel drivers/clk/renesas/r9a06g032-clocks.c.

Notable difference: this version avoids allocating a 'struct clk'
for each clock source, as this is problematic before relocation.
Instead, it uses the same approach as existing Renesas RCAR2/3
clock drivers, using a temporary structure filled on-the-fly.

Signed-off-by: Ralph Siemsen <ralph.siemsen@linaro.org>
---
- TODO: add support for div_table

 drivers/clk/renesas/Kconfig            |   6 +
 drivers/clk/renesas/Makefile           |   1 +
 drivers/clk/renesas/r9a06g032-clocks.c | 734 +++++++++++++++++++++++++
 3 files changed, 741 insertions(+)
 create mode 100644 drivers/clk/renesas/r9a06g032-clocks.c

On 8/9/22 8:59 AM, Ralph Siemsen wrote:
> Clock driver for the Renesas RZ/N1 SoC family. This is based
> on the Linux kernel drivers/clk/renesas/r9a06g032-clocks.c.
> 
> Notable difference: this version avoids allocating a 'struct clk'
> for each clock source, as this is problematic before relocation.
> Instead, it uses the same approach as existing Renesas RCAR2/3
> clock drivers, using a temporary structure filled on-the-fly.
> 
> Signed-off-by: Ralph Siemsen <ralph.siemsen@linaro.org>
> ---
> - TODO: add support for div_table
> 
>   drivers/clk/renesas/Kconfig            |   6 +
>   drivers/clk/renesas/Makefile           |   1 +
>   drivers/clk/renesas/r9a06g032-clocks.c | 734 +++++++++++++++++++++++++
>   3 files changed, 741 insertions(+)
>   create mode 100644 drivers/clk/renesas/r9a06g032-clocks.c
> 
> diff --git a/drivers/clk/renesas/Kconfig b/drivers/clk/renesas/Kconfig
> index c53ff3ce01..e2f72fc04f 100644
> --- a/drivers/clk/renesas/Kconfig
> +++ b/drivers/clk/renesas/Kconfig
> @@ -120,3 +120,9 @@ config CLK_R8A779A0
>   	depends on CLK_RCAR_GEN3
>   	help
>   	  Enable this to support the clocks on Renesas R8A779A0 SoC.
> +
> +config CLK_R9A06G032
> +	bool "Renesas R9A06G032 clock driver"
> +	depends on CLK_RENESAS
> +	help
> +	  Enable this to support the clocks on Renesas R9A06G032 SoC.

nit: on the

...

> diff --git a/drivers/clk/renesas/Makefile b/drivers/clk/renesas/Makefile
> index 2cd2c69f68..9981f1a0bc 100644
> --- a/drivers/clk/renesas/Makefile
> +++ b/drivers/clk/renesas/Makefile
> @@ -17,3 +17,4 @@ obj-$(CONFIG_CLK_R8A77980) += r8a77980-cpg-mssr.o
>   obj-$(CONFIG_CLK_R8A77990) += r8a77990-cpg-mssr.o
>   obj-$(CONFIG_CLK_R8A77995) += r8a77995-cpg-mssr.o
>   obj-$(CONFIG_CLK_R8A779A0) += r8a779a0-cpg-mssr.o
> +obj-$(CONFIG_CLK_R9A06G032) += r9a06g032-clocks.o
> diff --git a/drivers/clk/renesas/r9a06g032-clocks.c b/drivers/clk/renesas/r9a06g032-clocks.c
> new file mode 100644
> index 0000000000..9c8f51eb96
> --- /dev/null
> +++ b/drivers/clk/renesas/r9a06g032-clocks.c
> @@ -0,0 +1,734 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * R9A06G032 clock driver
> + *
> + * Copyright (C) 2018 Renesas Electronics Europe Limited
> + *
> + * Michel Pollet <michel.pollet@bp.renesas.com>, <buserror@gmail.com>
> + */
> +
> +#include <common.h>
> +#include <clk-uclass.h>
> +#include <dm.h>
> +#include <regmap.h>
> +#include <syscon.h>
> +#include <linux/bitops.h>
> +#include <linux/clk-provider.h>
> +#include <linux/delay.h>
> +#include <asm/io.h>
> +
> +#include <dt-bindings/clock/r9a06g032-sysctrl.h>
> +
> +struct r9a06g032_gate {

Can you add some documentation for each of the fields? Same for r9a06g032_clkdesc.

https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html

> +	u16 gate, reset, ready, midle,
> +		scon, mirack, mistat;

What are the scon/mirack/mistat fields for? You define them for a lot
of clocks, but I don't see them used in the driver.

> +};
> +
> +/* This is used to describe a clock for instantiation */
> +struct r9a06g032_clkdesc {
> +	const char *name;
> +	uint32_t managed: 1;
> +	uint32_t type: 3;

I wonder if we could define the enum here?

> +	uint32_t index: 8;
> +	uint32_t source : 8; /* source index + 1 (0 == none) */
> +	/* these are used to populate the bitsel struct */
> +	union {
> +		struct r9a06g032_gate gate;
> +		/* for dividers */
> +		struct {
> +			unsigned int div_min : 10, div_max : 10, reg: 10;
> +			u16 div_table[4];
> +		};
> +		/* For fixed-factor ones */
> +		struct {
> +			u16 div, mul;
> +		};
> +		/* for dual gate */
> +		struct {
> +			uint16_t group : 1;
> +			u16 sel, g1, r1, g2, r2;
> +		} dual;
> +	};
> +};
> +
> +#define I_GATE(_clk, _rst, _rdy, _midle, _scon, _mirack, _mistat) \
> +	{ .gate = _clk, .reset = _rst, \

If these fields have bitfield inside them, then those bitfields should
be assigned/constructed separately. That is, if .reset is actually a combined
offset/bit, then you need to expose those in the macro. Since you have a lot of these, you might want to do something like

#define BIT_OFFSET	GENMASK(15, 5)
#define BIT_SHIFT	GENMASK(4, 0)

#define PACK_BIT(offset, shift)		(FIELD_PREP(BIT_OFFSET, offset) | FIELD_PREP(BIT_SHIFT, shift))

> +		.ready = _rdy, .midle = _midle, \
> +		.scon = _scon, .mirack = _mirack, .mistat = _mistat }

Please put each assignment on a separate line

> +#define D_GATE(_idx, _n, _src, ...) \
> +	{ .type = K_GATE, .index = R9A06G032_##_idx, \
> +		.source = 1 + R9A06G032_##_src, .name = _n, \
> +		.gate = I_GATE(__VA_ARGS__) }
> +#define D_MODULE(_idx, _n, _src, ...) \
> +	{ .type = K_GATE, .index = R9A06G032_##_idx, \
> +		.source = 1 + R9A06G032_##_src, .name = _n, \
> +		.managed = 1, .gate = I_GATE(__VA_ARGS__) }
> +#define D_ROOT(_idx, _n, _mul, _div) \
> +	{ .type = K_FFC, .index = R9A06G032_##_idx, .name = _n, \
> +		.div = _div, .mul = _mul }
> +#define D_FFC(_idx, _n, _src, _div) \
> +	{ .type = K_FFC, .index = R9A06G032_##_idx, \
> +		.source = 1 + R9A06G032_##_src, .name = _n, \
> +		.div = _div, .mul = 1}
> +#define D_DIV(_idx, _n, _src, _reg, _min, _max, ...) \
> +	{ .type = K_DIV, .index = R9A06G032_##_idx, \
> +		.source = 1 + R9A06G032_##_src, .name = _n, \
> +		.reg = _reg, .div_min = _min, .div_max = _max, \
> +		.div_table = { __VA_ARGS__ } }
> +#define D_UGATE(_idx, _n, _src, _g, _g1, _r1, _g2, _r2) \
> +	{ .type = K_DUALGATE, .index = R9A06G032_##_idx, \
> +		.source = 1 + R9A06G032_##_src, .name = _n, \
> +		.dual = { .group = _g, \
> +			.g1 = _g1, .r1 = _r1, .g2 = _g2, .r2 = _r2 }, }
> +
> +enum { K_GATE = 0, K_FFC, K_DIV, K_BITSEL, K_DUALGATE };

Please put each member on a separate line.

> +
> +/* Internal clock IDs */
> +#define R9A06G032_CLKOUT		0
> +#define R9A06G032_CLKOUT_D10		2
> +#define R9A06G032_CLKOUT_D16		3
> +#define R9A06G032_CLKOUT_D160		4
> +#define R9A06G032_CLKOUT_D1OR2		5
> +#define R9A06G032_CLKOUT_D20		6
> +#define R9A06G032_CLKOUT_D40		7
> +#define R9A06G032_CLKOUT_D5		8
> +#define R9A06G032_CLKOUT_D8		9
> +#define R9A06G032_DIV_ADC		10
> +#define R9A06G032_DIV_I2C		11
> +#define R9A06G032_DIV_NAND		12
> +#define R9A06G032_DIV_P1_PG		13
> +#define R9A06G032_DIV_P2_PG		14
> +#define R9A06G032_DIV_P3_PG		15
> +#define R9A06G032_DIV_P4_PG		16
> +#define R9A06G032_DIV_P5_PG		17
> +#define R9A06G032_DIV_P6_PG		18
> +#define R9A06G032_DIV_QSPI0		19
> +#define R9A06G032_DIV_QSPI1		20
> +#define R9A06G032_DIV_REF_SYNC		21
> +#define R9A06G032_DIV_SDIO0		22
> +#define R9A06G032_DIV_SDIO1		23
> +#define R9A06G032_DIV_SWITCH		24
> +#define R9A06G032_DIV_UART		25
> +#define R9A06G032_DIV_MOTOR		64
> +#define R9A06G032_CLK_DDRPHY_PLLCLK_D4	78
> +#define R9A06G032_CLK_ECAT100_D4	79
> +#define R9A06G032_CLK_HSR100_D2		80
> +#define R9A06G032_CLK_REF_SYNC_D4	81
> +#define R9A06G032_CLK_REF_SYNC_D8	82
> +#define R9A06G032_CLK_SERCOS100_D2	83
> +#define R9A06G032_DIV_CA7		84
> +
> +#define R9A06G032_UART_GROUP_012	154
> +#define R9A06G032_UART_GROUP_34567	155

Can you put these in your dt-bindings header? I think that would make it
much clearer why there are gaps, and would avoid someone accidentally
duplicating a clock id (although I suppose your array initializer below
might complain?)

> +#define R9A06G032_CLOCK_COUNT		(R9A06G032_UART_GROUP_34567 + 1)
> +
> +static const struct r9a06g032_clkdesc r9a06g032_clocks[] = {
> +	D_ROOT(CLKOUT, "clkout", 25, 1),
> +	D_ROOT(CLK_PLL_USB, "clk_pll_usb", 12, 10),
> +	D_FFC(CLKOUT_D10, "clkout_d10", CLKOUT, 10),
> +	D_FFC(CLKOUT_D16, "clkout_d16", CLKOUT, 16),
> +	D_FFC(CLKOUT_D160, "clkout_d160", CLKOUT, 160),
> +	D_DIV(CLKOUT_D1OR2, "clkout_d1or2", CLKOUT, 0, 1, 2),
> +	D_FFC(CLKOUT_D20, "clkout_d20", CLKOUT, 20),
> +	D_FFC(CLKOUT_D40, "clkout_d40", CLKOUT, 40),
> +	D_FFC(CLKOUT_D5, "clkout_d5", CLKOUT, 5),
> +	D_FFC(CLKOUT_D8, "clkout_d8", CLKOUT, 8),
> +	D_DIV(DIV_ADC, "div_adc", CLKOUT, 77, 50, 250),
> +	D_DIV(DIV_I2C, "div_i2c", CLKOUT, 78, 12, 16),
> +	D_DIV(DIV_NAND, "div_nand", CLKOUT, 82, 12, 32),
> +	D_DIV(DIV_P1_PG, "div_p1_pg", CLKOUT, 68, 12, 200),
> +	D_DIV(DIV_P2_PG, "div_p2_pg", CLKOUT, 62, 12, 128),
> +	D_DIV(DIV_P3_PG, "div_p3_pg", CLKOUT, 64, 8, 128),
> +	D_DIV(DIV_P4_PG, "div_p4_pg", CLKOUT, 66, 8, 128),
> +	D_DIV(DIV_P5_PG, "div_p5_pg", CLKOUT, 71, 10, 40),
> +	D_DIV(DIV_P6_PG, "div_p6_pg", CLKOUT, 18, 12, 64),
> +	D_DIV(DIV_QSPI0, "div_qspi0", CLKOUT, 73, 3, 7),
> +	D_DIV(DIV_QSPI1, "div_qspi1", CLKOUT, 25, 3, 7),
> +	D_DIV(DIV_REF_SYNC, "div_ref_sync", CLKOUT, 56, 2, 16, 2, 4, 8, 16),
> +	D_DIV(DIV_SDIO0, "div_sdio0", CLKOUT, 74, 20, 128),
> +	D_DIV(DIV_SDIO1, "div_sdio1", CLKOUT, 75, 20, 128),
> +	D_DIV(DIV_SWITCH, "div_switch", CLKOUT, 37, 5, 40),
> +	D_DIV(DIV_UART, "div_uart", CLKOUT, 79, 12, 128),
> +	D_GATE(CLK_25_PG4, "clk_25_pg4", CLKOUT_D40, 0x749, 0x74a, 0x74b, 0, 0xae3, 0, 0),
> +	D_GATE(CLK_25_PG5, "clk_25_pg5", CLKOUT_D40, 0x74c, 0x74d, 0x74e, 0, 0xae4, 0, 0),
> +	D_GATE(CLK_25_PG6, "clk_25_pg6", CLKOUT_D40, 0x74f, 0x750, 0x751, 0, 0xae5, 0, 0),
> +	D_GATE(CLK_25_PG7, "clk_25_pg7", CLKOUT_D40, 0x752, 0x753, 0x754, 0, 0xae6, 0, 0),
> +	D_GATE(CLK_25_PG8, "clk_25_pg8", CLKOUT_D40, 0x755, 0x756, 0x757, 0, 0xae7, 0, 0),
> +	D_GATE(CLK_ADC, "clk_adc", DIV_ADC, 0x1ea, 0x1eb, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_ECAT100, "clk_ecat100", CLKOUT_D10, 0x405, 0, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_HSR100, "clk_hsr100", CLKOUT_D10, 0x483, 0, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_I2C0, "clk_i2c0", DIV_I2C, 0x1e6, 0x1e7, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_I2C1, "clk_i2c1", DIV_I2C, 0x1e8, 0x1e9, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_MII_REF, "clk_mii_ref", CLKOUT_D40, 0x342, 0, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_NAND, "clk_nand", DIV_NAND, 0x284, 0x285, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_NOUSBP2_PG6, "clk_nousbp2_pg6", DIV_P2_PG, 0x774, 0x775, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_P1_PG2, "clk_p1_pg2", DIV_P1_PG, 0x862, 0x863, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_P1_PG3, "clk_p1_pg3", DIV_P1_PG, 0x864, 0x865, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_P1_PG4, "clk_p1_pg4", DIV_P1_PG, 0x866, 0x867, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_P4_PG3, "clk_p4_pg3", DIV_P4_PG, 0x824, 0x825, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_P4_PG4, "clk_p4_pg4", DIV_P4_PG, 0x826, 0x827, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_P6_PG1, "clk_p6_pg1", DIV_P6_PG, 0x8a0, 0x8a1, 0x8a2, 0, 0xb60, 0, 0),
> +	D_GATE(CLK_P6_PG2, "clk_p6_pg2", DIV_P6_PG, 0x8a3, 0x8a4, 0x8a5, 0, 0xb61, 0, 0),
> +	D_GATE(CLK_P6_PG3, "clk_p6_pg3", DIV_P6_PG, 0x8a6, 0x8a7, 0x8a8, 0, 0xb62, 0, 0),
> +	D_GATE(CLK_P6_PG4, "clk_p6_pg4", DIV_P6_PG, 0x8a9, 0x8aa, 0x8ab, 0, 0xb63, 0, 0),
> +	D_MODULE(CLK_PCI_USB, "clk_pci_usb", CLKOUT_D40, 0xe6, 0, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_QSPI0, "clk_qspi0", DIV_QSPI0, 0x2a4, 0x2a5, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_QSPI1, "clk_qspi1", DIV_QSPI1, 0x484, 0x485, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_RGMII_REF, "clk_rgmii_ref", CLKOUT_D8, 0x340, 0, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_RMII_REF, "clk_rmii_ref", CLKOUT_D20, 0x341, 0, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_SDIO0, "clk_sdio0", DIV_SDIO0, 0x64, 0, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_SDIO1, "clk_sdio1", DIV_SDIO1, 0x644, 0, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_SERCOS100, "clk_sercos100", CLKOUT_D10, 0x425, 0, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_SLCD, "clk_slcd", DIV_P1_PG, 0x860, 0x861, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_SPI0, "clk_spi0", DIV_P3_PG, 0x7e0, 0x7e1, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_SPI1, "clk_spi1", DIV_P3_PG, 0x7e2, 0x7e3, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_SPI2, "clk_spi2", DIV_P3_PG, 0x7e4, 0x7e5, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_SPI3, "clk_spi3", DIV_P3_PG, 0x7e6, 0x7e7, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_SPI4, "clk_spi4", DIV_P4_PG, 0x820, 0x821, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_SPI5, "clk_spi5", DIV_P4_PG, 0x822, 0x823, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_SWITCH, "clk_switch", DIV_SWITCH, 0x982, 0x983, 0, 0, 0, 0, 0),
> +	D_DIV(DIV_MOTOR, "div_motor", CLKOUT_D5, 84, 2, 8),
> +	D_MODULE(HCLK_ECAT125, "hclk_ecat125", CLKOUT_D8, 0x400, 0x401, 0, 0x402, 0, 0x440, 0x441),
> +	D_MODULE(HCLK_PINCONFIG, "hclk_pinconfig", CLKOUT_D40, 0x740, 0x741, 0x742, 0, 0xae0, 0, 0),
> +	D_MODULE(HCLK_SERCOS, "hclk_sercos", CLKOUT_D10, 0x420, 0x422, 0, 0x421, 0, 0x460, 0x461),
> +	D_MODULE(HCLK_SGPIO2, "hclk_sgpio2", DIV_P5_PG, 0x8c3, 0x8c4, 0x8c5, 0, 0xb41, 0, 0),
> +	D_MODULE(HCLK_SGPIO3, "hclk_sgpio3", DIV_P5_PG, 0x8c6, 0x8c7, 0x8c8, 0, 0xb42, 0, 0),
> +	D_MODULE(HCLK_SGPIO4, "hclk_sgpio4", DIV_P5_PG, 0x8c9, 0x8ca, 0x8cb, 0, 0xb43, 0, 0),
> +	D_MODULE(HCLK_TIMER0, "hclk_timer0", CLKOUT_D40, 0x743, 0x744, 0x745, 0, 0xae1, 0, 0),
> +	D_MODULE(HCLK_TIMER1, "hclk_timer1", CLKOUT_D40, 0x746, 0x747, 0x748, 0, 0xae2, 0, 0),
> +	D_MODULE(HCLK_USBF, "hclk_usbf", CLKOUT_D8, 0xe3, 0, 0, 0xe4, 0, 0x102, 0x103),
> +	D_MODULE(HCLK_USBH, "hclk_usbh", CLKOUT_D8, 0xe0, 0xe1, 0, 0xe2, 0, 0x100, 0x101),
> +	D_MODULE(HCLK_USBPM, "hclk_usbpm", CLKOUT_D8, 0xe5, 0, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_48_PG_F, "clk_48_pg_f", CLK_48, 0x78c, 0x78d, 0, 0x78e, 0, 0xb04, 0xb05),
> +	D_GATE(CLK_48_PG4, "clk_48_pg4", CLK_48, 0x789, 0x78a, 0x78b, 0, 0xb03, 0, 0),
> +	D_FFC(CLK_DDRPHY_PLLCLK_D4, "clk_ddrphy_pllclk_d4", CLK_DDRPHY_PLLCLK, 4),
> +	D_FFC(CLK_ECAT100_D4, "clk_ecat100_d4", CLK_ECAT100, 4),
> +	D_FFC(CLK_HSR100_D2, "clk_hsr100_d2", CLK_HSR100, 2),
> +	D_FFC(CLK_REF_SYNC_D4, "clk_ref_sync_d4", CLK_REF_SYNC, 4),
> +	D_FFC(CLK_REF_SYNC_D8, "clk_ref_sync_d8", CLK_REF_SYNC, 8),
> +	D_FFC(CLK_SERCOS100_D2, "clk_sercos100_d2", CLK_SERCOS100, 2),
> +	D_DIV(DIV_CA7, "div_ca7", CLK_REF_SYNC, 57, 1, 4, 1, 2, 4),
> +	D_MODULE(HCLK_CAN0, "hclk_can0", CLK_48, 0x783, 0x784, 0x785, 0, 0xb01, 0, 0),
> +	D_MODULE(HCLK_CAN1, "hclk_can1", CLK_48, 0x786, 0x787, 0x788, 0, 0xb02, 0, 0),
> +	D_MODULE(HCLK_DELTASIGMA, "hclk_deltasigma", DIV_MOTOR, 0x1ef, 0x1f0, 0x1f1, 0, 0, 0, 0),
> +	D_MODULE(HCLK_PWMPTO, "hclk_pwmpto", DIV_MOTOR, 0x1ec, 0x1ed, 0x1ee, 0, 0, 0, 0),
> +	D_MODULE(HCLK_RSV, "hclk_rsv", CLK_48, 0x780, 0x781, 0x782, 0, 0xb00, 0, 0),
> +	D_MODULE(HCLK_SGPIO0, "hclk_sgpio0", DIV_MOTOR, 0x1e0, 0x1e1, 0x1e2, 0, 0, 0, 0),
> +	D_MODULE(HCLK_SGPIO1, "hclk_sgpio1", DIV_MOTOR, 0x1e3, 0x1e4, 0x1e5, 0, 0, 0, 0),
> +	D_DIV(RTOS_MDC, "rtos_mdc", CLK_REF_SYNC, 100, 80, 640, 80, 160, 320, 640),
> +	D_GATE(CLK_CM3, "clk_cm3", CLK_REF_SYNC_D4, 0xba0, 0xba1, 0, 0xba2, 0, 0xbc0, 0xbc1),
> +	D_GATE(CLK_DDRC, "clk_ddrc", CLK_DDRPHY_PLLCLK_D4, 0x323, 0x324, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_ECAT25, "clk_ecat25", CLK_ECAT100_D4, 0x403, 0x404, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_HSR50, "clk_hsr50", CLK_HSR100_D2, 0x484, 0x485, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_HW_RTOS, "clk_hw_rtos", CLK_REF_SYNC_D4, 0xc60, 0xc61, 0, 0, 0, 0, 0),
> +	D_GATE(CLK_SERCOS50, "clk_sercos50", CLK_SERCOS100_D2, 0x424, 0x423, 0, 0, 0, 0, 0),
> +	D_MODULE(HCLK_ADC, "hclk_adc", CLK_REF_SYNC_D8, 0x1af, 0x1b0, 0x1b1, 0, 0, 0, 0),
> +	D_MODULE(HCLK_CM3, "hclk_cm3", CLK_REF_SYNC_D4, 0xc20, 0xc21, 0xc22, 0, 0, 0, 0),
> +	D_MODULE(HCLK_CRYPTO_EIP150, "hclk_crypto_eip150", CLK_REF_SYNC_D4, 0x123, 0x124, 0x125, 0, 0x142, 0, 0),
> +	D_MODULE(HCLK_CRYPTO_EIP93, "hclk_crypto_eip93", CLK_REF_SYNC_D4, 0x120, 0x121, 0, 0x122, 0, 0x140, 0x141),
> +	D_MODULE(HCLK_DDRC, "hclk_ddrc", CLK_REF_SYNC_D4, 0x320, 0x322, 0, 0x321, 0, 0x3a0, 0x3a1),
> +	D_MODULE(HCLK_DMA0, "hclk_dma0", CLK_REF_SYNC_D4, 0x260, 0x261, 0x262, 0x263, 0x2c0, 0x2c1, 0x2c2),
> +	D_MODULE(HCLK_DMA1, "hclk_dma1", CLK_REF_SYNC_D4, 0x264, 0x265, 0x266, 0x267, 0x2c3, 0x2c4, 0x2c5),
> +	D_MODULE(HCLK_GMAC0, "hclk_gmac0", CLK_REF_SYNC_D4, 0x360, 0x361, 0x362, 0x363, 0x3c0, 0x3c1, 0x3c2),
> +	D_MODULE(HCLK_GMAC1, "hclk_gmac1", CLK_REF_SYNC_D4, 0x380, 0x381, 0x382, 0x383, 0x3e0, 0x3e1, 0x3e2),
> +	D_MODULE(HCLK_GPIO0, "hclk_gpio0", CLK_REF_SYNC_D4, 0x212, 0x213, 0x214, 0, 0, 0, 0),
> +	D_MODULE(HCLK_GPIO1, "hclk_gpio1", CLK_REF_SYNC_D4, 0x215, 0x216, 0x217, 0, 0, 0, 0),
> +	D_MODULE(HCLK_GPIO2, "hclk_gpio2", CLK_REF_SYNC_D4, 0x229, 0x22a, 0x22b, 0, 0, 0, 0),
> +	D_MODULE(HCLK_HSR, "hclk_hsr", CLK_HSR100_D2, 0x480, 0x482, 0, 0x481, 0, 0x4c0, 0x4c1),
> +	D_MODULE(HCLK_I2C0, "hclk_i2c0", CLK_REF_SYNC_D8, 0x1a9, 0x1aa, 0x1ab, 0, 0, 0, 0),
> +	D_MODULE(HCLK_I2C1, "hclk_i2c1", CLK_REF_SYNC_D8, 0x1ac, 0x1ad, 0x1ae, 0, 0, 0, 0),
> +	D_MODULE(HCLK_LCD, "hclk_lcd", CLK_REF_SYNC_D4, 0x7a0, 0x7a1, 0x7a2, 0, 0xb20, 0, 0),
> +	D_MODULE(HCLK_MSEBI_M, "hclk_msebi_m", CLK_REF_SYNC_D4, 0x164, 0x165, 0x166, 0, 0x183, 0, 0),
> +	D_MODULE(HCLK_MSEBI_S, "hclk_msebi_s", CLK_REF_SYNC_D4, 0x160, 0x161, 0x162, 0x163, 0x180, 0x181, 0x182),
> +	D_MODULE(HCLK_NAND, "hclk_nand", CLK_REF_SYNC_D4, 0x280, 0x281, 0x282, 0x283, 0x2e0, 0x2e1, 0x2e2),
> +	D_MODULE(HCLK_PG_I, "hclk_pg_i", CLK_REF_SYNC_D4, 0x7ac, 0x7ad, 0, 0x7ae, 0, 0xb24, 0xb25),
> +	D_MODULE(HCLK_PG19, "hclk_pg19", CLK_REF_SYNC_D4, 0x22c, 0x22d, 0x22e, 0, 0, 0, 0),
> +	D_MODULE(HCLK_PG20, "hclk_pg20", CLK_REF_SYNC_D4, 0x22f, 0x230, 0x231, 0, 0, 0, 0),
> +	D_MODULE(HCLK_PG3, "hclk_pg3", CLK_REF_SYNC_D4, 0x7a6, 0x7a7, 0x7a8, 0, 0xb22, 0, 0),
> +	D_MODULE(HCLK_PG4, "hclk_pg4", CLK_REF_SYNC_D4, 0x7a9, 0x7aa, 0x7ab, 0, 0xb23, 0, 0),
> +	D_MODULE(HCLK_QSPI0, "hclk_qspi0", CLK_REF_SYNC_D4, 0x2a0, 0x2a1, 0x2a2, 0x2a3, 0x300, 0x301, 0x302),
> +	D_MODULE(HCLK_QSPI1, "hclk_qspi1", CLK_REF_SYNC_D4, 0x480, 0x481, 0x482, 0x483, 0x4c0, 0x4c1, 0x4c2),
> +	D_MODULE(HCLK_ROM, "hclk_rom", CLK_REF_SYNC_D4, 0xaa0, 0xaa1, 0xaa2, 0, 0xb80, 0, 0),
> +	D_MODULE(HCLK_RTC, "hclk_rtc", CLK_REF_SYNC_D8, 0xa00, 0, 0, 0, 0, 0, 0),
> +	D_MODULE(HCLK_SDIO0, "hclk_sdio0", CLK_REF_SYNC_D4, 0x60, 0x61, 0x62, 0x63, 0x80, 0x81, 0x82),
> +	D_MODULE(HCLK_SDIO1, "hclk_sdio1", CLK_REF_SYNC_D4, 0x640, 0x641, 0x642, 0x643, 0x660, 0x661, 0x662),
> +	D_MODULE(HCLK_SEMAP, "hclk_semap", CLK_REF_SYNC_D4, 0x7a3, 0x7a4, 0x7a5, 0, 0xb21, 0, 0),
> +	D_MODULE(HCLK_SPI0, "hclk_spi0", CLK_REF_SYNC_D4, 0x200, 0x201, 0x202, 0, 0, 0, 0),
> +	D_MODULE(HCLK_SPI1, "hclk_spi1", CLK_REF_SYNC_D4, 0x203, 0x204, 0x205, 0, 0, 0, 0),
> +	D_MODULE(HCLK_SPI2, "hclk_spi2", CLK_REF_SYNC_D4, 0x206, 0x207, 0x208, 0, 0, 0, 0),
> +	D_MODULE(HCLK_SPI3, "hclk_spi3", CLK_REF_SYNC_D4, 0x209, 0x20a, 0x20b, 0, 0, 0, 0),
> +	D_MODULE(HCLK_SPI4, "hclk_spi4", CLK_REF_SYNC_D4, 0x20c, 0x20d, 0x20e, 0, 0, 0, 0),
> +	D_MODULE(HCLK_SPI5, "hclk_spi5", CLK_REF_SYNC_D4, 0x20f, 0x210, 0x211, 0, 0, 0, 0),
> +	D_MODULE(HCLK_SWITCH, "hclk_switch", CLK_REF_SYNC_D4, 0x980, 0, 0x981, 0, 0, 0, 0),
> +	D_MODULE(HCLK_SWITCH_RG, "hclk_switch_rg", CLK_REF_SYNC_D4, 0xc40, 0xc41, 0xc42, 0, 0, 0, 0),
> +	D_MODULE(HCLK_UART0, "hclk_uart0", CLK_REF_SYNC_D8, 0x1a0, 0x1a1, 0x1a2, 0, 0, 0, 0),
> +	D_MODULE(HCLK_UART1, "hclk_uart1", CLK_REF_SYNC_D8, 0x1a3, 0x1a4, 0x1a5, 0, 0, 0, 0),
> +	D_MODULE(HCLK_UART2, "hclk_uart2", CLK_REF_SYNC_D8, 0x1a6, 0x1a7, 0x1a8, 0, 0, 0, 0),
> +	D_MODULE(HCLK_UART3, "hclk_uart3", CLK_REF_SYNC_D4, 0x218, 0x219, 0x21a, 0, 0, 0, 0),
> +	D_MODULE(HCLK_UART4, "hclk_uart4", CLK_REF_SYNC_D4, 0x21b, 0x21c, 0x21d, 0, 0, 0, 0),
> +	D_MODULE(HCLK_UART5, "hclk_uart5", CLK_REF_SYNC_D4, 0x220, 0x221, 0x222, 0, 0, 0, 0),
> +	D_MODULE(HCLK_UART6, "hclk_uart6", CLK_REF_SYNC_D4, 0x223, 0x224, 0x225, 0, 0, 0, 0),
> +	D_MODULE(HCLK_UART7, "hclk_uart7", CLK_REF_SYNC_D4, 0x226, 0x227, 0x228, 0, 0, 0, 0),
> +	/*
> +	 * These are not hardware clocks, but are needed to handle the special
> +	 * case where we have a 'selector bit' that doesn't just change the
> +	 * parent for a clock, but also the gate it's supposed to use.
> +	 */
> +	{
> +		.index = R9A06G032_UART_GROUP_012,
> +		.name = "uart_group_012",
> +		.type = K_BITSEL,
> +		.source = 1 + R9A06G032_DIV_UART,
> +		/* R9A06G032_SYSCTRL_REG_PWRCTRL_PG0_0 */
> +		.dual.sel = ((0x34 / 4) << 5) | 30,
> +		.dual.group = 0,
> +	},
> +	{
> +		.index = R9A06G032_UART_GROUP_34567,
> +		.name = "uart_group_34567",
> +		.type = K_BITSEL,
> +		.source = 1 + R9A06G032_DIV_P2_PG,
> +		/* R9A06G032_SYSCTRL_REG_PWRCTRL_PG1_PR2 */
> +		.dual.sel = ((0xec / 4) << 5) | 24,
> +		.dual.group = 1,
> +	},
> +	D_UGATE(CLK_UART0, "clk_uart0", UART_GROUP_012, 0, 0x1b2, 0x1b3, 0x1b4, 0x1b5),
> +	D_UGATE(CLK_UART1, "clk_uart1", UART_GROUP_012, 0, 0x1b6, 0x1b7, 0x1b8, 0x1b9),
> +	D_UGATE(CLK_UART2, "clk_uart2", UART_GROUP_012, 0, 0x1ba, 0x1bb, 0x1bc, 0x1bd),
> +	D_UGATE(CLK_UART3, "clk_uart3", UART_GROUP_34567, 1, 0x760, 0x761, 0x762, 0x763),
> +	D_UGATE(CLK_UART4, "clk_uart4", UART_GROUP_34567, 1, 0x764, 0x765, 0x766, 0x767),
> +	D_UGATE(CLK_UART5, "clk_uart5", UART_GROUP_34567, 1, 0x768, 0x769, 0x76a, 0x76b),
> +	D_UGATE(CLK_UART6, "clk_uart6", UART_GROUP_34567, 1, 0x76c, 0x76d, 0x76e, 0x76f),
> +	D_UGATE(CLK_UART7, "clk_uart7", UART_GROUP_34567, 1, 0x770, 0x771, 0x772, 0x773),
> +};
> +
> +struct r9a06g032_priv {
> +	struct regmap		*regmap;
> +	struct clk		mclk;
> +};
> +
> +static const struct r9a06g032_clkdesc *r9a06g032_clk_get(struct clk *clk)
> +{
> +	const unsigned long clkid = clk->id & 0xffff;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(r9a06g032_clocks); i++) {
> +		if (r9a06g032_clocks[i].index == clkid)
> +			return &r9a06g032_clocks[i];
> +	}
> +
> +	return NULL;
> +}
> +
> +static int r9a06g032_clk_get_parent(struct clk *clk, struct clk *parent)
> +{
> +	const struct r9a06g032_clkdesc *desc = r9a06g032_clk_get(clk);
> +
> +	if (!desc)
> +		return -ENODEV;

ENOENT please

see https://u-boot.readthedocs.io/en/latest/develop/driver-model/design.html#error-codes

> +
> +	if (desc->source == 0)

if (!desc->source)

although I would reverse the clauses to make the condition clearer

> +		parent->id = ~0;	/* Top-level clock */

Can you use a define for this (instead of referring to ~0 everywhere)

> +	else
> +		parent->id = desc->source - 1;
> +
> +	parent->dev = clk->dev;

I think you need to clk_request here.

> +	return 0;
> +}
> +
> +static ulong r9a06g032_clk_get_parent_rate(struct clk *clk)
> +{
> +	struct clk parent;
> +
> +	if (r9a06g032_clk_get_parent(clk, &parent)) {
> +		debug("Failed to get parent clock for id=%lu\b", clk->id);

dev_dbg please

> +		return 0;

Return -ENOENT here please. You can check for this with IS_ERR.

> +	}
> +
> +	if (parent.id == ~0) {
> +		struct r9a06g032_priv *clocks = dev_get_priv(clk->dev);
> +		ulong rate = clk_get_rate(&clocks->mclk);

You need a newline here

> +		return rate;
> +	}
> +
> +	return clk_get_rate(&parent);
> +}
> +
> +/* register/bit pairs are encoded as an uint16_t */
> +static void
> +clk_rdesc_set(struct r9a06g032_priv *clocks,
> +	      u16 one, unsigned int on)
> +{
> +	uint offset = 4 * (one >> 5);
> +	uint mask = 1U << (one & 0x1f);
> +	uint val = ((!!on) << (one & 0x1f));

Please either use bitfields for this, or use FIELD_GET() and friends.

> +	regmap_update_bits(clocks->regmap, offset, mask, val);
> +}
> +
> +static int
> +clk_rdesc_get(struct r9a06g032_priv *clocks,
> +	      uint16_t one)

I think this all fits on one line?

> +{
> +	uint offset = 4 * (one >> 5);
> +	u32 val = 0;
> +
> +	regmap_read(clocks->regmap, offset, &val);
> +
> +	return !!(val & (1U << (one & 0x1f)));
> +}
> +
> +/*
> + * Cheating a little bit here: leverage the existing code to control the
> + * per-clock reset. It should really be handled by a reset controller instead.
> + */
> +void clk_rzn1_reset_state(struct clk *clk, int on)
> +{
> +	struct r9a06g032_priv *clocks = dev_get_priv(clk->dev);
> +	const struct r9a06g032_clkdesc *desc = r9a06g032_clk_get(clk);
> +	assert(desc);
> +	assert(desc->type == K_GATE);
> +	const struct r9a06g032_gate *g = &desc->gate;
> +	assert(g->reset);

Please order declarations all at the beginning. In this case, you will need to do something like

	struct r9a06g032_priv *clocks = dev_get_priv(clk->dev);
	const struct r9a06g032_clkdesc *desc = r9a06g032_clk_get(clk);
	const struct r9a06g032_gate *g;

	assert(desc);
	assert(desc->type == K_GATE);
	g = &desc->gate
	assert(g->reset);

> +	clk_rdesc_set(clocks, g->reset, on);
> +}
> +
> +/*
> + * This implements the R9A06G032 clock gate 'driver'. We cannot use the system's
> + * clock gate framework as the gates on the R9A06G032 have a special enabling
> + * sequence, therefore we use this little proxy.
> + */
> +static int r9a06g032_clk_gate_set(struct clk *clk, int on)
> +{
> +	struct r9a06g032_priv *clocks = dev_get_priv(clk->dev);
> +	const struct r9a06g032_clkdesc *desc = r9a06g032_clk_get(clk);
> +	assert(desc);
> +	assert(desc->type == K_GATE);
> +	const struct r9a06g032_gate *g = &desc->gate;

ditto

> +	clk_rdesc_set(clocks, g->gate, on);
> +	/* De-assert reset */
> +	if (g->reset)
> +		clk_rdesc_set(clocks, g->reset, 1);
> +
> +	/* Hardware manual recommends 5us delay after enabling clock & reset */
> +	udelay(5);
> +
> +	/* If the peripheral is memory mapped (i.e. an AXI slave), there is an
> +	 * associated SLVRDY bit in the System Controller that needs to be set
> +	 * so that the FlexWAY bus fabric passes on the read/write requests.
> +	 */
> +	if (g->ready || g->midle) {
> +		if (g->ready)
> +			clk_rdesc_set(clocks, g->ready, on);
> +		/* Clear 'Master Idle Request' bit */
> +		if (g->midle)
> +			clk_rdesc_set(clocks, g->midle, !on);
> +	}
> +	/* Note: We don't wait for FlexWAY Socket Connection signal */
> +
> +	return 0;
> +}
> +
> +static int r9a06g032_clk_gate_enable(struct clk *clk)
> +{
> +	return r9a06g032_clk_gate_set(clk, 1);
> +}
> +
> +static int r9a06g032_clk_gate_disable(struct clk *clk)
> +{
> +	return r9a06g032_clk_gate_set(clk, 0);
> +}
> +
> +/*
> + * Fixed factor clock
> + */
> +static ulong r9a06g032_ffc_get_rate(struct clk *clk)
> +{
> +	const struct r9a06g032_clkdesc *desc = r9a06g032_clk_get(clk);
> +	unsigned long parent_rate = r9a06g032_clk_get_parent_rate(clk);
> +	unsigned long long rate;
> +
> +	if (parent_rate == 0) {
> +		debug("%s: parent_rate is zero\n", __func__);
> +		return 0;
> +	}
> +
> +	rate = (unsigned long long)parent_rate * desc->mul;
> +	rate = DIV_ROUND_UP(rate, desc->div);
> +	return (ulong)rate;
> +}
> +
> +/*
> + * This implements R9A06G032 clock divider 'driver'. This differs from the
> + * standard clk_divider because the set_rate method must also set b[31] to
> + * trigger the hardware rate change. In theory it should also wait for this
> + * bit to clear.
> + */
> +static ulong r9a06g032_div_get_rate(struct clk *clk)
> +{
> +	struct r9a06g032_priv *clocks = dev_get_priv(clk->dev);
> +	const struct r9a06g032_clkdesc *desc = r9a06g032_clk_get(clk);
> +	unsigned long parent_rate = r9a06g032_clk_get_parent_rate(clk);
> +	u32 div = 0;
> +
> +	if (parent_rate == 0) {
> +		debug("%s: parent_rate is zero\n", __func__);

Didn't you already log this?

> +		return 0;
> +	}
> +
> +	regmap_read(clocks->regmap, 4 * desc->reg, &div);
> +
> +	if (div < desc->div_min)
> +		div = desc->div_min;
> +	else if (div > desc->div_max)
> +		div = desc->div_max;
> +	return DIV_ROUND_UP(parent_rate, div);

DIV_ROUND_CLOSEST?

> +}
> +
> +static ulong r9a06g032_div_set_rate(struct clk *clk, ulong rate)
> +{
> +	struct r9a06g032_priv *clocks = dev_get_priv(clk->dev);
> +	const struct r9a06g032_clkdesc *desc = r9a06g032_clk_get(clk);
> +	unsigned long parent_rate = r9a06g032_clk_get_parent_rate(clk);
> +
> +	if (parent_rate == 0) {
> +		debug("%s: parent_rate is zero\n", __func__);
> +		return 0;
> +	}
> +
> +	/* + 1 to cope with rates that have the remainder dropped */
> +	u32 div = DIV_ROUND_UP(parent_rate, rate + 1);
> +
> +	/* Clamp to allowable range */
> +	if (div < desc->div_min)
> +		div = desc->div_min;
> +	else if (div > desc->div_max)
> +		div = desc->div_max;
> +
> +	/* TODO: use the .div_table if provided */
> +	if (desc->div_table[0])
> +		pr_err("ERROR: %s: div_table not implemented\n", __func__);

dev_err

But can't you just leave out the div_table member?

> +
> +	pr_devel("%s clkid %lu rate %ld parent %ld div %d\n", __func__, clk->id,
> +		 rate, parent_rate, div);

dev_dbg

> +
> +	/*
> +	 * Need to write the bit 31 with the divider value to
> +	 * latch it. Technically we should wait until it has been
> +	 * cleared too.
> +	 * TODO: Find whether this callback is sleepable, in case
> +	 * the hardware /does/ require some sort of spinloop here.
> +	 */
> +	regmap_write(clocks->regmap, 4 * desc->reg, div | BIT(31));
> +
> +	return 0;
> +}
> +
> +/*
> + * Dual gate. This handles toggling the approprate clock/reset bits,
> + * which depends on the mux setting above.
> + */
> +static int r9a06g032_clk_dualgate_setenable(struct r9a06g032_priv *clocks,
> +					    const struct r9a06g032_clkdesc *desc,
> +					    int enable)
> +{
> +	u8 sel_bit = clk_rdesc_get(clocks, desc->dual.sel);
> +	u16 gate[2] = { desc->dual.g1, desc->dual.g2 };
> +	u16 reset[2] = { desc->dual.r1, desc->dual.r2 };
> +
> +	/* we always turn off the 'other' gate, regardless */
> +	clk_rdesc_set(clocks, gate[!sel_bit], 0);
> +	if (reset[!sel_bit])
> +		clk_rdesc_set(clocks, reset[!sel_bit], 1);
> +
> +	/* set the gate as requested */
> +	clk_rdesc_set(clocks, gate[sel_bit], enable);
> +	if (reset[sel_bit])
> +		clk_rdesc_set(clocks, reset[sel_bit], 1);
> +
> +	return 0;
> +}
> +
> +static int r9a06g032_clk_dualgate_enable(struct clk *clk)
> +{
> +	struct r9a06g032_priv *clocks = dev_get_priv(clk->dev);
> +	const struct r9a06g032_clkdesc *desc = r9a06g032_clk_get(clk);
> +
> +	return r9a06g032_clk_dualgate_setenable(clocks, desc, 1);
> +}
> +
> +static int r9a06g032_clk_dualgate_disable(struct clk *clk)
> +{
> +	struct r9a06g032_priv *clocks = dev_get_priv(clk->dev);
> +	const struct r9a06g032_clkdesc *desc = r9a06g032_clk_get(clk);
> +
> +	return r9a06g032_clk_dualgate_setenable(clocks, desc, 0);
> +}
> +
> +static int r9a06g032_clk_dualgate_is_enabled(struct clk *clk)
> +{
> +	struct r9a06g032_priv *clocks = dev_get_priv(clk->dev);
> +	const struct r9a06g032_clkdesc *desc = r9a06g032_clk_get(clk);
> +	u8 sel_bit = clk_rdesc_get(clocks, desc->dual.sel);
> +	u16 gate[2] = { desc->dual.g1, desc->dual.g2 };
> +
> +	return clk_rdesc_get(clocks, gate[sel_bit]);
> +}
> +
> +/*
> + * Main clock driver
> + */
> +static int r9a06g032_clk_enable(struct clk *clk)
> +{
> +	const struct r9a06g032_clkdesc *desc = r9a06g032_clk_get(clk);
> +
> +	switch (desc->type) {
> +	case K_GATE:
> +		return r9a06g032_clk_gate_enable(clk);
> +	case K_DUALGATE:
> +		return r9a06g032_clk_dualgate_enable(clk);
> +	default:
> +		printf("ERROR: %s:%d unhandled type=%d\n", __func__, __LINE__, desc->type);

Assert or dev_dbg is better here. This is "impossible" so we try and
avoid increasing image size in these cases.

> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int r9a06g032_clk_disable(struct clk *clk)
> +{
> +	const struct r9a06g032_clkdesc *desc = r9a06g032_clk_get(clk);
> +
> +	switch (desc->type) {
> +	case K_GATE:
> +		return r9a06g032_clk_gate_disable(clk);
> +	case K_DUALGATE:
> +		return r9a06g032_clk_dualgate_disable(clk);
> +	default:
> +		printf("ERROR: %s:%d unhandled type=%d\n", __func__, __LINE__, desc->type);
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static ulong r9a06g032_clk_get_rate(struct clk *clk)
> +{
> +	const struct r9a06g032_clkdesc *desc = r9a06g032_clk_get(clk);
> +	ulong ret = 0;
> +
> +	assert(desc);
> +
> +	switch (desc->type) {
> +	case K_FFC:
> +		ret = r9a06g032_ffc_get_rate(clk);
> +		break;
> +	case K_GATE:
> +		ret = r9a06g032_clk_get_parent_rate(clk);
> +		break;
> +	case K_DIV:
> +		ret = r9a06g032_div_get_rate(clk);
> +		break;
> +	case K_BITSEL:
> +		/*
> +		 * Look at the mux to determine parent.
> +		 * 0 means it is coming from UART DIV (group 012 or 34567)
> +		 * 1 means it is coming from USB_PLL
> +		 */
> +		if (r9a06g032_clk_dualgate_is_enabled(clk)) {
> +			struct clk clk = { .id = R9A06G032_CLK_PLL_USB };
> +			ret = r9a06g032_clk_get_parent_rate(&clk);
> +		}
> +		ret = r9a06g032_clk_get_parent_rate(clk);
> +		break;
> +	case K_DUALGATE:
> +		ret = r9a06g032_clk_get_parent_rate(clk);
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static ulong r9a06g032_clk_set_rate(struct clk *clk, ulong rate)
> +{
> +	const struct r9a06g032_clkdesc *desc = r9a06g032_clk_get(clk);
> +	ulong ret = 0;
> +
> +	assert(desc);
> +
> +	switch (desc->type) {
> +	case K_DIV:
> +		ret = r9a06g032_div_set_rate(clk, rate);
> +		break;
> +	default:
> +		printf("ERROR: %s:%d not implemented yet\n", __func__, __LINE__);
> +	};
> +
> +	return ret;
> +}
> +
> +static int r9a06g032_clk_of_xlate(struct clk *clk, struct ofnode_phandle_args *args)
> +{
> +	if (args->args_count != 1) {
> +		debug("Invalid args_count: %d\n", args->args_count);
> +		return -EINVAL;
> +	}
> +
> +	clk->id = args->args[0];
> +
> +	return 0;
> +}
> +
> +static const struct clk_ops r9a06g032_clk_ops = {
> +	.enable		= r9a06g032_clk_enable,
> +	.disable	= r9a06g032_clk_disable,
> +	.get_rate	= r9a06g032_clk_get_rate,
> +	.set_rate	= r9a06g032_clk_set_rate,
> +	.of_xlate	= r9a06g032_clk_of_xlate,
> +};
> +
> +static int r9a06g032_clk_probe(struct udevice *dev)
> +{
> +	struct r9a06g032_priv *priv = dev_get_priv(dev);
> +	int err;
> +
> +	priv->regmap = syscon_regmap_lookup_by_phandle(dev, "regmap");
> +	if (!priv->regmap) {

IS_ERR(priv->regmap)

> +		pr_err("unable to find regmap\n");

dev_dbg

> +		return -ENODEV;

return ERR_PTR(priv->regmap)

> +	}
> +
> +	/* Enable S/W reset */
> +	regmap_write(priv->regmap, 0x120, 0x41);
> +
> +	err = clk_get_by_name(dev, "mclk", &priv->mclk);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
> +static int r9a06g032_clk_remove(struct udevice *dev)
> +{
> +	return 0;
> +}

Not necessary.

> +
> +static const struct udevice_id r9a06g032_clk_ids[] = {
> +	{ .compatible = "renesas,r9a06g032-sysctrl" },
> +	{ }
> +};
> +
> +U_BOOT_DRIVER(clk_r9a06g032) = {
> +	.name		= "clk_r9a06g032",
> +	.id		= UCLASS_CLK,
> +	.of_match	= r9a06g032_clk_ids,
> +	.priv_auto	= sizeof(struct r9a06g032_priv),
> +	.ops		= &r9a06g032_clk_ops,
> +	.probe		= &r9a06g032_clk_probe,
> +	.remove		= &r9a06g032_clk_remove,
> +	.flags		= DM_FLAG_PRE_RELOC,
> +};
> 

The overall structure looks good; most of these things you
should be able to iron out fairly easily.

--Sean

On Sat, Aug 13, 2022 at 01:30:19AM -0400, Sean Anderson wrote:
>+
>>+	u16 gate, reset, ready, midle,
>>+		scon, mirack, mistat;
>
>What are the scon/mirack/mistat fields for? You define them for a lot
>of clocks, but I don't see them used in the driver.

These came from the Linux driver of the same name. I can only speculate 
that in turn, the Linux driver definitions were auto-generated from 
vendor provided XML or similar documentation.

I figured that it would be best to match the Linux kernel clock driver. 
That way fixes can easily be shared. In fact while doing this work, I 
found an error in the clock table [1] and I also made some 
simplifications [2].

Regarding the unused fields (scon, mirack, mistat): I am not really sure 
what their purpose is. Maybe there is some value in having them. I'll 
try to find out more information about them. If we do decide to drop 
them, I would like to keep it synchronised with the Linux driver.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2dee50ab9e72a3cae75b65e5934c8dd3e9bf01bc

[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f46efcc4746f5c1a539df9db625c04321f75e494

>>+};
>>+
>>+/* This is used to describe a clock for instantiation */
>>+struct r9a06g032_clkdesc {
>>+	const char *name;
>>+	uint32_t managed: 1;
>>+	uint32_t type: 3;
>
>I wonder if we could define the enum here?

This is still part of the code which I've intentionally kept identical 
to the Linux driver. I do agree that moving the enum seems reasonable, 
I'll put together a patch for this (and test it) and see if I can get it 
accepted on the kernel side.

>>+	uint32_t index: 8;
>>+	uint32_t source : 8; /* source index + 1 (0 == none) */
>>+	/* these are used to populate the bitsel struct */
>>+	union {
>>+		struct r9a06g032_gate gate;
>>+		/* for dividers */
>>+		struct {
>>+			unsigned int div_min : 10, div_max : 10, reg: 10;
>>+			u16 div_table[4];
>>+		};
>>+		/* For fixed-factor ones */
>>+		struct {
>>+			u16 div, mul;
>>+		};
>>+		/* for dual gate */
>>+		struct {
>>+			uint16_t group : 1;
>>+			u16 sel, g1, r1, g2, r2;
>>+		} dual;
>>+	};
>>+};
>>+
>>+#define I_GATE(_clk, _rst, _rdy, _midle, _scon, _mirack, _mistat) \
>>+	{ .gate = _clk, .reset = _rst, \
>
>If these fields have bitfield inside them, then those bitfields should
>be assigned/constructed separately. That is, if .reset is actually a combined
>offset/bit, then you need to expose those in the macro. Since you have a lot of these, you might want to do something like
>
>#define BIT_OFFSET	GENMASK(15, 5)
>#define BIT_SHIFT	GENMASK(4, 0)
>
>#define PACK_BIT(offset, shift)		(FIELD_PREP(BIT_OFFSET, offset) | FIELD_PREP(BIT_SHIFT, shift))

I think it happened before I started working on RZ/N1, but there seemed 
to be quite a few iterations on how to represent the clock tree. At one 
point there were macros to assign/construct the bitfield values. And 
then a different way, and eventually the direct hex values you now see 
in the clock tables.

At the risk of re-opening old wounds (luckily not mine) I decided to 
just leave this part exactly as-is in the Linux driver.

>>+
>>+/* Internal clock IDs */
>>+#define R9A06G032_CLKOUT		0
>>+#define R9A06G032_CLKOUT_D10		2
>>+#define R9A06G032_CLKOUT_D16		3
>>+#define R9A06G032_CLKOUT_D160		4
>>[...]
>>+#define R9A06G032_UART_GROUP_012	154
>>+#define R9A06G032_UART_GROUP_34567	155
>
>Can you put these in your dt-bindings header? I think that would make it
>much clearer why there are gaps, and would avoid someone accidentally
>duplicating a clock id (although I suppose your array initializer below
>might complain?)

In fact quite a few of them are in the dt-bindings already, see 
include/dt-bindings/clock/r9a06g032-sysctrl.h

I'm not really sure why some of these are defined in the .C file while 
others are in the dt-bindings header. Like much of the other bits, this 
was something I just carried over as-is from the Linux driver.

>>+		parent->id = ~0;	/* Top-level clock */
>
>Can you use a define for this (instead of referring to ~0 everywhere)

Yes, that sounds reasonable. My list of fixes (for both Linux driver and 
the u-boot one) is going to get rather long ;-)

>>+	else
>>+		parent->id = desc->source - 1;
>>+
>>+	parent->dev = clk->dev;
>
>I think you need to clk_request here.

Normally clk_request is called by a driver wishing to use a particular 
clock. That is not the case here. This is in a helper function used to 
compute the current rate of a given clock. It only looks at the local 
table (struct r9a06g032_clkdsc).

>>+/* register/bit pairs are encoded as an uint16_t */
>>+static void
>>+clk_rdesc_set(struct r9a06g032_priv *clocks,
>>+	      u16 one, unsigned int on)
>>+{
>>+	uint offset = 4 * (one >> 5);
>>+	uint mask = 1U << (one & 0x1f);
>>+	uint val = ((!!on) << (one & 0x1f));
>
>Please either use bitfields for this, or use FIELD_GET() and friends.

Yes, this would be clearer - however as mentioned above, there was 
already quite a bit of teeth-gnashing about this encoding. I will 
prepare a patch for the Linux side and see what kind of reply I get.

I would very much prefer to keep both in sync as much as possible.

>>+/*
>>+ * Fixed factor clock
>>+ */
>>+static ulong r9a06g032_ffc_get_rate(struct clk *clk)
>>+{
>>+	const struct r9a06g032_clkdesc *desc = r9a06g032_clk_get(clk);
>>+	unsigned long parent_rate = r9a06g032_clk_get_parent_rate(clk);
>>+	unsigned long long rate;
>>+
>>+	if (parent_rate == 0) {
>>+		debug("%s: parent_rate is zero\n", __func__);
>>+		return 0;
>>+	}
>>+
>>+	rate = (unsigned long long)parent_rate * desc->mul;
>>+	rate = DIV_ROUND_UP(rate, desc->div);
>>+	return (ulong)rate;
>>+}
>>+
>>+/*
>>+ * This implements R9A06G032 clock divider 'driver'. This differs from the
>>+ * standard clk_divider because the set_rate method must also set b[31] to
>>+ * trigger the hardware rate change. In theory it should also wait for this
>>+ * bit to clear.
>>+ */
>>+static ulong r9a06g032_div_get_rate(struct clk *clk)
>>+{
>>+	struct r9a06g032_priv *clocks = dev_get_priv(clk->dev);
>>+	const struct r9a06g032_clkdesc *desc = r9a06g032_clk_get(clk);
>>+	unsigned long parent_rate = r9a06g032_clk_get_parent_rate(clk);
>>+	u32 div = 0;
>>+
>>+	if (parent_rate == 0) {
>>+		debug("%s: parent_rate is zero\n", __func__);
>
>Didn't you already log this?

It is for a different clock type. However I will see if I do something 
to avoid the duplication.

>
>>+		return 0;
>>+	}
>>+
>>+	regmap_read(clocks->regmap, 4 * desc->reg, &div);
>>+
>>+	if (div < desc->div_min)
>>+		div = desc->div_min;
>>+	else if (div > desc->div_max)
>>+		div = desc->div_max;
>>+	return DIV_ROUND_UP(parent_rate, div);
>
>DIV_ROUND_CLOSEST?

I'm hesitant to change the logic on this, as it could subtly alter the 
values.

>>+	/* TODO: use the .div_table if provided */
>>+	if (desc->div_table[0])
>>+		pr_err("ERROR: %s: div_table not implemented\n", __func__);
>
>dev_err
>
>But can't you just leave out the div_table member?

Only a few of the clocks have a div_table, but for those, the table 
specifies the allowable divider values. So right now, the code cannot 
correctly set such clocks, as it may try programming illegal values 
(most likely with the result that you don't get the expected rate).

The Linux driver does implement the div_table logic, I simply did not 
carry it over yet to u-boot. Mostly because I have not yet had need for 
one of the few clocks which does use the table. I do plan to add it.

>>+/*
>>+ * Main clock driver
>>+ */
>>+static int r9a06g032_clk_enable(struct clk *clk)
>>+{
>>+	const struct r9a06g032_clkdesc *desc = r9a06g032_clk_get(clk);
>>+
>>+	switch (desc->type) {
>>+	case K_GATE:
>>+		return r9a06g032_clk_gate_enable(clk);
>>+	case K_DUALGATE:
>>+		return r9a06g032_clk_dualgate_enable(clk);
>>+	default:
>>+		printf("ERROR: %s:%d unhandled type=%d\n", __func__, __LINE__, desc->type);
>
>Assert or dev_dbg is better here. This is "impossible" so we try and
>avoid increasing image size in these cases.

Fair enough. Though I have to say I have been bitten by this kind of 
thing a few times. After having spent time debugging-via-printf, I would 
then discover an assert or dev_dbg that points out the exact problem. If 
only I had enabled DEBUG for that file! And yet if I enable DEBUG 
globally, there is so much noise, that I don't notice the one message 
that might have been helpful.

>The overall structure looks good; most of these things you
>should be able to iron out fairly easily.

I have skipped over a few of the smaller points regarding return values, 
etc, but I will address them in next version of the patch. Thanks again 
for your time reviewing this.

Regards,
-Ralph

On 8/14/22 10:48 PM, Ralph Siemsen wrote:
> On Sat, Aug 13, 2022 at 01:30:19AM -0400, Sean Anderson wrote:
>> +
>>> +    u16 gate, reset, ready, midle,
>>> +        scon, mirack, mistat;
>>
>> What are the scon/mirack/mistat fields for? You define them for a lot
>> of clocks, but I don't see them used in the driver.
> 
> These came from the Linux driver of the same name. I can only speculate that in turn, the Linux driver definitions were auto-generated from vendor provided XML or similar documentation.
> 
> I figured that it would be best to match the Linux kernel clock driver. That way fixes can easily be shared. In fact while doing this work, I found an error in the clock table [1] and I also made some simplifications [2].
> 
> Regarding the unused fields (scon, mirack, mistat): I am not really sure what their purpose is. Maybe there is some value in having them. I'll try to find out more information about them. If we do decide to drop them, I would like to keep it synchronised with the Linux driver.

OK, well if you don't use them then perhaps you can just leave them in
the macro but remove them from the struct. That way you can add support
for them later if you need to, but they don't take up space in the mean
time. A comment summarizing your explanation above would be helpful.

> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2dee50ab9e72a3cae75b65e5934c8dd3e9bf01bc
> 
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f46efcc4746f5c1a539df9db625c04321f75e494
> 
>>> +};
>>> +
>>> +/* This is used to describe a clock for instantiation */
>>> +struct r9a06g032_clkdesc {
>>> +    const char *name;
>>> +    uint32_t managed: 1;
>>> +    uint32_t type: 3;
>>
>> I wonder if we could define the enum here?
> 
> This is still part of the code which I've intentionally kept identical to the Linux driver. I do agree that moving the enum seems reasonable, I'll put together a patch for this (and test it) and see if I can get it accepted on the kernel side.
> 
>>> +    uint32_t index: 8;
>>> +    uint32_t source : 8; /* source index + 1 (0 == none) */
>>> +    /* these are used to populate the bitsel struct */
>>> +    union {
>>> +        struct r9a06g032_gate gate;
>>> +        /* for dividers */
>>> +        struct {
>>> +            unsigned int div_min : 10, div_max : 10, reg: 10;
>>> +            u16 div_table[4];
>>> +        };
>>> +        /* For fixed-factor ones */
>>> +        struct {
>>> +            u16 div, mul;
>>> +        };
>>> +        /* for dual gate */
>>> +        struct {
>>> +            uint16_t group : 1;
>>> +            u16 sel, g1, r1, g2, r2;
>>> +        } dual;
>>> +    };
>>> +};
>>> +
>>> +#define I_GATE(_clk, _rst, _rdy, _midle, _scon, _mirack, _mistat) \
>>> +    { .gate = _clk, .reset = _rst, \
>>
>> If these fields have bitfield inside them, then those bitfields should
>> be assigned/constructed separately. That is, if .reset is actually a combined
>> offset/bit, then you need to expose those in the macro. Since you have a lot of these, you might want to do something like
>>
>> #define BIT_OFFSET    GENMASK(15, 5)
>> #define BIT_SHIFT    GENMASK(4, 0)
>>
>> #define PACK_BIT(offset, shift)        (FIELD_PREP(BIT_OFFSET, offset) | FIELD_PREP(BIT_SHIFT, shift))
> 
> I think it happened before I started working on RZ/N1, but there seemed to be quite a few iterations on how to represent the clock tree. At one point there were macros to assign/construct the bitfield values. And then a different way, and eventually the direct hex values you now see in the clock tables.
> 
> At the risk of re-opening old wounds (luckily not mine) I decided to just leave this part exactly as-is in the Linux driver.

Can you link to that discussion? The earliest discussion of that
series I could find was [1], and there's no mention of the encoding.
This encoding scheme seems to be used only for this SoC, and not for
any of the other renesas drivers. I suspect that this just wasn't
reviewed in detail the first time around...

[1] https://lore.kernel.org/all/1527154169-32380-6-git-send-email-michel.pollet@bp.renesas.com/

>>> +
>>> +/* Internal clock IDs */
>>> +#define R9A06G032_CLKOUT        0
>>> +#define R9A06G032_CLKOUT_D10        2
>>> +#define R9A06G032_CLKOUT_D16        3
>>> +#define R9A06G032_CLKOUT_D160        4
>>> [...]
>>> +#define R9A06G032_UART_GROUP_012    154
>>> +#define R9A06G032_UART_GROUP_34567    155
>>
>> Can you put these in your dt-bindings header? I think that would make it
>> much clearer why there are gaps, and would avoid someone accidentally
>> duplicating a clock id (although I suppose your array initializer below
>> might complain?)
> 
> In fact quite a few of them are in the dt-bindings already, see include/dt-bindings/clock/r9a06g032-sysctrl.h
> 
> I'm not really sure why some of these are defined in the .C file while others are in the dt-bindings header. Like much of the other bits, this was something I just carried over as-is from the Linux driver.

I think these are "internal" clocks (that is, clocks which don't really
exist like intermediate dividers) whereas the others are public-facing
clocks. It's up to you, but maybe have a comment noting where the other
ids come from.

>>> +        parent->id = ~0;    /* Top-level clock */
>>
>> Can you use a define for this (instead of referring to ~0 everywhere)
> 
> Yes, that sounds reasonable. My list of fixes (for both Linux driver and the u-boot one) is going to get rather long ;-)
> 
>>> +    else
>>> +        parent->id = desc->source - 1;
>>> +
>>> +    parent->dev = clk->dev;
>>
>> I think you need to clk_request here.
> 
> Normally clk_request is called by a driver wishing to use a particular clock. That is not the case here. This is in a helper function used to compute the current rate of a given clock. It only looks at the local table (struct r9a06g032_clkdsc).

You call clk_get_rate on it. Any time you "create" a new clock, you
must call clk_request.

>>> +/* register/bit pairs are encoded as an uint16_t */
>>> +static void
>>> +clk_rdesc_set(struct r9a06g032_priv *clocks,
>>> +          u16 one, unsigned int on)
>>> +{
>>> +    uint offset = 4 * (one >> 5);
>>> +    uint mask = 1U << (one & 0x1f);
>>> +    uint val = ((!!on) << (one & 0x1f));
>>
>> Please either use bitfields for this, or use FIELD_GET() and friends.
> 
> Yes, this would be clearer - however as mentioned above, there was already quite a bit of teeth-gnashing about this encoding. I will prepare a patch for the Linux side and see what kind of reply I get.
> 
> I would very much prefer to keep both in sync as much as possible.
> 
>>> +/*
>>> + * Fixed factor clock
>>> + */
>>> +static ulong r9a06g032_ffc_get_rate(struct clk *clk)
>>> +{
>>> +    const struct r9a06g032_clkdesc *desc = r9a06g032_clk_get(clk);
>>> +    unsigned long parent_rate = r9a06g032_clk_get_parent_rate(clk);
>>> +    unsigned long long rate;
>>> +
>>> +    if (parent_rate == 0) {
>>> +        debug("%s: parent_rate is zero\n", __func__);
>>> +        return 0;
>>> +    }
>>> +
>>> +    rate = (unsigned long long)parent_rate * desc->mul;
>>> +    rate = DIV_ROUND_UP(rate, desc->div);
>>> +    return (ulong)rate;
>>> +}
>>> +
>>> +/*
>>> + * This implements R9A06G032 clock divider 'driver'. This differs from the
>>> + * standard clk_divider because the set_rate method must also set b[31] to
>>> + * trigger the hardware rate change. In theory it should also wait for this
>>> + * bit to clear.
>>> + */
>>> +static ulong r9a06g032_div_get_rate(struct clk *clk)
>>> +{
>>> +    struct r9a06g032_priv *clocks = dev_get_priv(clk->dev);
>>> +    const struct r9a06g032_clkdesc *desc = r9a06g032_clk_get(clk);
>>> +    unsigned long parent_rate = r9a06g032_clk_get_parent_rate(clk);
>>> +    u32 div = 0;
>>> +
>>> +    if (parent_rate == 0) {
>>> +        debug("%s: parent_rate is zero\n", __func__);
>>
>> Didn't you already log this?
> 
> It is for a different clock type. However I will see if I do something to avoid the duplication.

I mean in r9a06g032_clk_get_parent_rate. You can also just do

if (!parent_rate)
	...

>>
>>> +        return 0;
>>> +    }
>>> +
>>> +    regmap_read(clocks->regmap, 4 * desc->reg, &div);
>>> +
>>> +    if (div < desc->div_min)
>>> +        div = desc->div_min;
>>> +    else if (div > desc->div_max)
>>> +        div = desc->div_max;
>>> +    return DIV_ROUND_UP(parent_rate, div);
>>
>> DIV_ROUND_CLOSEST?
> 
> I'm hesitant to change the logic on this, as it could subtly alter the values.

Well if you have 2MHz divided by 3, the resulting rate is closer to
666667 kHz than 666666 Hz.

>>> +    /* TODO: use the .div_table if provided */
>>> +    if (desc->div_table[0])
>>> +        pr_err("ERROR: %s: div_table not implemented\n", __func__);
>>
>> dev_err
>>
>> But can't you just leave out the div_table member?
> 
> Only a few of the clocks have a div_table, but for those, the table specifies the allowable divider values. So right now, the code cannot correctly set such clocks, as it may try programming illegal values (most likely with the result that you don't get the expected rate).
> 
> The Linux driver does implement the div_table logic, I simply did not carry it over yet to u-boot. Mostly because I have not yet had need for one of the few clocks which does use the table. I do plan to add it.

OK

>>> +/*
>>> + * Main clock driver
>>> + */
>>> +static int r9a06g032_clk_enable(struct clk *clk)
>>> +{
>>> +    const struct r9a06g032_clkdesc *desc = r9a06g032_clk_get(clk);
>>> +
>>> +    switch (desc->type) {
>>> +    case K_GATE:
>>> +        return r9a06g032_clk_gate_enable(clk);
>>> +    case K_DUALGATE:
>>> +        return r9a06g032_clk_dualgate_enable(clk);
>>> +    default:
>>> +        printf("ERROR: %s:%d unhandled type=%d\n", __func__, __LINE__, desc->type);
>>
>> Assert or dev_dbg is better here. This is "impossible" so we try and
>> avoid increasing image size in these cases.
> 
> Fair enough. Though I have to say I have been bitten by this kind of thing a few times. After having spent time debugging-via-printf, I would then discover an assert or dev_dbg that points out the exact problem. If only I had enabled DEBUG for that file! And yet if I enable DEBUG globally, there is so much noise, that I don't notice the one message that might have been helpful.

I generally stick a `#define DEBUG` at the top of a file any time I get
an error with no message. You still return 0, so I suggest returning
-EINVAL (or something else uncommon) so you have somewhere to start.

>> The overall structure looks good; most of these things you
>> should be able to iron out fairly easily.
> 
> I have skipped over a few of the smaller points regarding return values, etc, but I will address them in next version of the patch. Thanks again for your time reviewing this.
> 
> Regards,
> -Ralph

On Tue, Aug 23, 2022 at 12:14:31AM -0400, Sean Anderson wrote:
>>Regarding the unused fields (scon, mirack, mistat): I am not really 
>>sure what their purpose is. Maybe there is some value in having them. 
>>I'll try to find out more information about them. If we do decide to 
>>drop them, I would like to keep it synchronised with the Linux driver.
>
>OK, well if you don't use them then perhaps you can just leave them in
>the macro but remove them from the struct. That way you can add support
>for them later if you need to, but they don't take up space in the mean
>time. A comment summarizing your explanation above would be helpful.

I did figure out (mostly) what they are for, so I can see some value in 
keeping at least some of them. But as you said, they are currently 
unused, so dropping them from the structure make sense.

I have prepared patches for this firstly on the kernel side, and then I 
will make the same change in this u-boot driver. Stay tuned :-)

>>I think it happened before I started working on RZ/N1, but there 
>>seemed to be quite a few iterations on how to represent the clock 
>>tree. At one point there were macros to assign/construct the bitfield 
>>values. And then a different way, and eventually the direct hex values 
>>you now see in the clock tables.
>>
>>At the risk of re-opening old wounds (luckily not mine) I decided to 
>>just leave this part exactly as-is in the Linux driver.
>
>Can you link to that discussion? The earliest discussion of that
>series I could find was [1], and there's no mention of the encoding.
>This encoding scheme seems to be used only for this SoC, and not for
>any of the other renesas drivers. I suspect that this just wasn't
>reviewed in detail the first time around...
>
>[1] https://lore.kernel.org/all/1527154169-32380-6-git-send-email-michel.pollet@bp.renesas.com/

That link [1] is the current driver, which uses the packed encoding 
(with a register offset and bit number stored as a packed uint16_t).

This is based (loosely) on an earlier version of the driver, which you 
can find in the Schneider kernel repo [2] on the 4.19 and older branch.
This version stores clock information is in the device tree [3] and uses 
_BIT() macro in the clock tables [4]

[2] https://github.com/renesas-rz/rzn1_linux/tree/rzn1-stable-v4.19/drivers/clk/rzn1

[3] https://github.com/renesas-rz/rzn1_linux/blob/rzn1-stable-v4.19/arch/arm/boot/dts/rzn1-clocks.dtsi

[4] https://github.com/renesas-rz/rzn1_linux/blob/rzn1-stable-v4.19/drivers/clk/rzn1/rzn1-clkctrl-tables.h

Evidently this was not deemed suitable, and thus morphed into the 
version that did get merged [1]. At least that is my guess, I don't know 
for sure what transpired.

I have modified the clock table so that the register offset and bitnum 
are explicit values, rather than packed together. I will run this up the 
kernel side and see if they agree. Am still trying to test it though...


>>In fact quite a few of them are in the dt-bindings already, see 
>>include/dt-bindings/clock/r9a06g032-sysctrl.h
>>
>>I'm not really sure why some of these are defined in the .C file while 
>>others are in the dt-bindings header. Like much of the other bits, 
>>this was something I just carried over as-is from the Linux driver.
>
>I think these are "internal" clocks (that is, clocks which don't really
>exist like intermediate dividers) whereas the others are public-facing
>clocks. It's up to you, but maybe have a comment noting where the other
>ids come from.

I guess that is plausible explanation.. I will add a comment...

>>>>+    else
>>>>+        parent->id = desc->source - 1;
>>>>+
>>>>+    parent->dev = clk->dev;
>>>
>>>I think you need to clk_request here.
>>
>>Normally clk_request is called by a driver wishing to use a particular 
>>clock. That is not the case here. This is in a helper function used to 
>>compute the current rate of a given clock. It only looks at the local 
>>table (struct r9a06g032_clkdsc).
>
>You call clk_get_rate on it. Any time you "create" a new clock, you
>must call clk_request.

So the situation here is similar to that on the mediatek patches from 
Weijie Gao [5], where you made a similar comment. These are not real 
clocks, they have no ops->request, and the only field used is clk->id.
This is done primarily to avoid a bunch of malloc of struct clk, 
particularly in the early u-boot (before relocation).
  
[5] https://lore.kernel.org/all/31b0e1313267c8d342e0e3d1c9f15eaa8e666114.camel@mediatek.com/

>>It is for a different clock type. However I will see if I do something 
>>to avoid the duplication.
>
>I mean in r9a06g032_clk_get_parent_rate. You can also just do
>
>if (!parent_rate)
>	...

I've fixed this (and several similar instances elsewhere).

>>>DIV_ROUND_CLOSEST?
>>
>>I'm hesitant to change the logic on this, as it could subtly alter the values.
>
>Well if you have 2MHz divided by 3, the resulting rate is closer to
>666667 kHz than 666666 Hz.

While I can't argue with the math, the linux driver upon which this is 
based uses DIV_ROUND_UP everywhere. Maybe that is something to also 
re-consider, but for the time being, I don't want to make the change 
even more complicated...

>>Fair enough. Though I have to say I have been bitten by this kind of 
>>thing a few times. After having spent time debugging-via-printf, I 
>>would then discover an assert or dev_dbg that points out the exact 
>>problem. If only I had enabled DEBUG for that file! And yet if I 
>>enable DEBUG globally, there is so much noise, that I don't notice the 
>>one message that might have been helpful.
>
>I generally stick a `#define DEBUG` at the top of a file any time I get
>an error with no message. You still return 0, so I suggest returning
>-EINVAL (or something else uncommon) so you have somewhere to start.

Yep, that works fine when developing a driver... but if you are 
debugging a board, it is often not clear which file(s) should get 
sprinkled with DEBUG... and turning it on globally is too verbose to be 
useful in most cases.

Anyhow, thanks for your review, and v3 will be posted eventually, once I
get the kernel side sorted.

-Ralph

Hi Sean,

I finally got around to posting v3 of this patch series. I wanted to
touch on a few issues you had mentioned in previous review.

On Fri, Aug 26, 2022 at 11:47 AM Ralph Siemsen <ralph.siemsen@linaro.org> wrote:
>
> On Tue, Aug 23, 2022 at 12:14:31AM -0400, Sean Anderson wrote:
> >>Regarding the unused fields (scon, mirack, mistat): I am not really
> >>sure what their purpose is. Maybe there is some value in having them.
> >>I'll try to find out more information about them. If we do decide to
> >>drop them, I would like to keep it synchronised with the Linux driver.
> >
> >OK, well if you don't use them then perhaps you can just leave them in
> >the macro but remove them from the struct. That way you can add support
> >for them later if you need to, but they don't take up space in the mean
> >time. A comment summarizing your explanation above would be helpful.

So I ended up doing as you suggested: keeping the values in the
tables, in case they are needed in the future, but omitting them from
the structure to save space.

> >>>DIV_ROUND_CLOSEST?
> >>
> >>I'm hesitant to change the logic on this, as it could subtly alter the values.
> >
> >Well if you have 2MHz divided by 3, the resulting rate is closer to
> >666667 kHz than 666666 Hz.

This could result in the chosen clock rate being higher than what was
requested. And in turn that could result in violating the spec of a
hardware device.

So I have kept the original logic here.

Regards
Ralph