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

[Sean Anderson]

On 6/3/21 4:34 AM, Luca Ceresoli wrote:
 > 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.

I would leave them in so the next person who works on this driver doesn't have to add them.\

--Sean

 >
 > [...]
 >
 >> +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?
 >