On Thu, Jun 3, 2021 at 9:52 AM Sean Anderson <sean.ander...@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-control...@6a.mux > >> 25000000 0 |-- clock-control...@6a.pfd > >> 2800000000 0 | `-- clock-control...@6a.pll > >> 33333333 0 | |-- > clock-controller@6a.fod0 > >> 33333333 0 | | `-- > clock-controller@6a.out1 > >> 33333333 0 | |-- > clock-controller@6a.fod1 > >> 33333333 0 | | `-- > clock-controller@6a.out2 > >> 50000000 0 | |-- > clock-controller@6a.fod2 > >> 50000000 0 | | `-- > clock-controller@6a.out3 > >> 125000000 0 | `-- > clock-controller@6a.fod3 > >> 125000000 0 | `-- > clock-controller@6a.out4 > >> 25000000 0 `-- > clock-controller@6a.out0_sel_i2cb > >> > >> A translation function is added so the references to <&versaclock X> get > routed > >> to the corresponding clock-control...@6a.outx. > >> > >> Signed-off-by: Adam Ford <aford...@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.va...@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? > >