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

[Adam Ford]

On Thu, Jun 3, 2021 at 3:34 AM Luca Ceresoli <luca at lucaceresoli.net> 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/

I am usually more careful about grammar.  I must have been tired.  :-)

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

make sense.

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

This was included a while ago in order to allow the device trees to build.

>
> > +
> > +/* 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 thought about it.  Because it was a port from Linux, I thought it
might be nice to keep it consistent, but I can remove the unused
references for cleaner code.

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

i can do that.
>
> --
> Luca