[PATCH] clk: clk_versaclock: Add support for versaclock driver

[Luca Ceresoli]

On 24/05/21 19:53, Adam Ford wrote:
> The driver is based on the Versaclock driver from the Linux code, but
> do differences in the clock API between them, some pieces had to change.

s/do/due to/ ?
s/had to change/had to be changed/

> This driver creates a mux, pfd, pll, and a series of fod ouputs.
>  Rate               Usecnt      Name
> ------------------------------------------
>  25000000             0        `-- x304-clock
>  25000000             0            `-- clock-controller at 6a.mux
>  25000000             0                |-- clock-controller at 6a.pfd
>  2800000000           0                |   `-- clock-controller at 6a.pll
>  33333333             0                |       |-- clock-controller at 6a.fod0
>  33333333             0                |       |   `-- clock-controller at 6a.out1
>  33333333             0                |       |-- clock-controller at 6a.fod1
>  33333333             0                |       |   `-- clock-controller at 6a.out2
>  50000000             0                |       |-- clock-controller at 6a.fod2
>  50000000             0                |       |   `-- clock-controller at 6a.out3
>  125000000            0                |       `-- clock-controller at 6a.fod3
>  125000000            0                |           `-- clock-controller at 6a.out4
>  25000000             0                `-- clock-controller at 6a.out0_sel_i2cb
> 
> A translation function is added so the references to <&versaclock X> get routed
> to the corresponding clock-controller at 6a.outX.
> 
> Signed-off-by: Adam Ford <aford173 at gmail.com>

I've been reviewing this patch and it looks mostly OK to me except for a
few notes below, mostly minor ones. However my knowledge of the U-Boot
driver model is minimal, thus I couldn't go in depth in the most
interesting and critical part of Adam's work, i.e. the adaptations for
the U-Boot codebase. I'm afraid I cannot do more.

> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 645709b855..2a9ebec860 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -51,3 +51,4 @@ obj-$(CONFIG_SANDBOX_CLK_CCF) += clk_sandbox_ccf.o
>  obj-$(CONFIG_STM32H7) += clk_stm32h7.o
>  obj-$(CONFIG_CLK_VERSAL) += clk_versal.o
>  obj-$(CONFIG_CLK_CDCE9XX) += clk-cdce9xx.o
> +obj-$(CONFIG_CLK_VERSACLOCK) +=clk_versaclock.o

Nit: space after '+='.

> diff --git a/drivers/clk/clk_versaclock.c b/drivers/clk/clk_versaclock.c
> new file mode 100644
> index 0000000000..30e49fad31
> --- /dev/null
> +++ b/drivers/clk/clk_versaclock.c
> @@ -0,0 +1,1025 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Driver for IDT Versaclock 5/6
> + *
> + * Derived from code Copyright (C) 2017 Marek Vasut <marek.vasut at gmail.com>
> + */
> +
> +#include <common.h>
> +#include <clk.h>
> +#include <clk-uclass.h>
> +#include <dm.h>
> +#include <errno.h>
> +#include <i2c.h>
> +#include <dm/device_compat.h>
> +#include <log.h>
> +#include <linux/clk-provider.h>
> +#include <linux/kernel.h>
> +#include <linux/math64.h>
> +
> +#include <dt-bindings/clk/versaclock.h>

Missing file?

> +
> +/* VersaClock5 registers */
> +#define VC5_OTP_CONTROL				0x00
> +
> +/* Factory-reserved register block */
> +#define VC5_RSVD_DEVICE_ID			0x01
> +#define VC5_RSVD_ADC_GAIN_7_0			0x02
> +#define VC5_RSVD_ADC_GAIN_15_8			0x03
> +#define VC5_RSVD_ADC_OFFSET_7_0			0x04
> +#define VC5_RSVD_ADC_OFFSET_15_8		0x05
> +#define VC5_RSVD_TEMPY				0x06
> +#define VC5_RSVD_OFFSET_TBIN			0x07
> +#define VC5_RSVD_GAIN				0x08
> +#define VC5_RSVD_TEST_NP			0x09
> +#define VC5_RSVD_UNUSED				0x0a
> +#define VC5_RSVD_BANDGAP_TRIM_UP		0x0b
> +#define VC5_RSVD_BANDGAP_TRIM_DN		0x0c
> +#define VC5_RSVD_CLK_R_12_CLK_AMP_4		0x0d
> +#define VC5_RSVD_CLK_R_34_CLK_AMP_4		0x0e
> +#define VC5_RSVD_CLK_AMP_123			0x0f

I wonder whether it really makes sense to define so many registers that
are not used in the driver. But it's done the same way in the Linux
driver, and it doesn't hurt much, so I'll be fine with or without them.

[...]

> +static const struct udevice_id versaclock_ids[] = {
> +	{ .compatible = "idt,5p49v5923", .data = (ulong)&idt_5p49v5923_info },
> +	{ .compatible = "idt,5p49v5925", .data = (ulong)&idt_5p49v5925_info },
> +	{ .compatible = "idt,5p49v5933", .data = (ulong)&idt_5p49v5933_info },
> +	{ .compatible = "idt,5p49v5935", .data = (ulong)&idt_5p49v5935_info },
> +	{ .compatible = "idt,5p49v6901", .data = (ulong)&idt_5p49v6901_info },
> +	{ .compatible = "idt,5p49v6965", .data = (ulong)&idt_5p49v6965_info },
> +	{},
> +};

Why not putting this array below, right before the U_BOOT_DRIVER() call
where it is used, similarly to the Linux driver?

-- 
Luca