On Thu, Jun 3, 2021 at 3:34 AM Luca Ceresoli luca@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@6a.mux 25000000 0 |-- clock-controller@6a.pfd 2800000000 0 | `-- clock-controller@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-controller@6a.outX.
Signed-off-by: Adam Ford aford173@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@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