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 `-- [email protected] > 25000000 0 |-- [email protected] > 2800000000 0 | `-- [email protected] > 33333333 0 | |-- [email protected] > 33333333 0 | | `-- > [email protected] > 33333333 0 | |-- [email protected] > 33333333 0 | | `-- > [email protected] > 50000000 0 | |-- [email protected] > 50000000 0 | | `-- > [email protected] > 125000000 0 | `-- [email protected] > 125000000 0 | `-- > [email protected] > 25000000 0 `-- [email protected]_sel_i2cb > > A translation function is added so the references to <&versaclock X> get > routed > to the corresponding [email protected]. > > Signed-off-by: Adam Ford <[email protected]> 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 <[email protected]> > + */ > + > +#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
