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 `-- [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.
I would leave them in so the next person who works on this driver doesn't have
to add them.\
--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?
>
