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

[Adam Ford]

On Thu, Jun 3, 2021 at 9:52 AM Sean Anderson <sean.anderson at seco.com> wrote:
>
>
>
> 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.\

OK.  Disregard my V2, and I'll submit a V3 today or tomorrow with the
register definitions left intact.

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