On Thu, Jun 3, 2021 at 3:34 AM Luca Ceresoli <[email protected]> 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 `-- [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 '+='. 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 <[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? 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
