[U-Boot] [PATCH 35/51] drivers: Add ICS8N3QV01 driver
Mario Six
mario.six at gdsys.cc
Tue Jul 25 08:08:21 UTC 2017
Hi Simon,
On Wed, Jul 19, 2017 at 11:05 AM, Simon Glass <sjg at chromium.org> wrote:
> On 14 July 2017 at 05:55, Mario Six <mario.six at gdsys.cc> wrote:
>> Add a driver for the ICS8N3QV01 Quad-Frequency Programmable VCXO.
>>
>> Signed-off-by: Mario Six <mario.six at gdsys.cc>
>> ---
>>
>> drivers/clk/Kconfig | 6 ++
>> drivers/clk/Makefile | 1 +
>> drivers/clk/ics8n3qv01.c | 184 +++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 191 insertions(+)
>> create mode 100644 drivers/clk/ics8n3qv01.c
>>
>> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
>> index 44da716b26..f6f3810b64 100644
>> --- a/drivers/clk/Kconfig
>> +++ b/drivers/clk/Kconfig
>> @@ -56,4 +56,10 @@ source "drivers/clk/uniphier/Kconfig"
>> source "drivers/clk/exynos/Kconfig"
>> source "drivers/clk/at91/Kconfig"
>>
>> +config ICS8N3QV01
>> + bool "Enable ICS8N3QV01 VCXO driver"
>> + depends on CLK
>> + help
>> + Support for the ICS8N3QV01 VCXO.
>
> What is this? Can you describe it a bit more here?
>
I'll add some details in v2.
>> +
>> endmenu
>> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
>> index 2746a8016a..d7cc486d23 100644
>> --- a/drivers/clk/Makefile
>> +++ b/drivers/clk/Makefile
>> @@ -21,3 +21,4 @@ obj-$(CONFIG_CLK_BCM6345) += clk_bcm6345.o
>> obj-$(CONFIG_CLK_BOSTON) += clk_boston.o
>> obj-$(CONFIG_ARCH_ASPEED) += aspeed/
>> obj-$(CONFIG_STM32F7) += clk_stm32f7.o
>> +obj-$(CONFIG_ICS8N3QV01) += ics8n3qv01.o
>
> Would be good if we could have these in alpha order. If you have time
> can you do a patch to tidy that up before or after?
>
Sure, I'll order them.
>> diff --git a/drivers/clk/ics8n3qv01.c b/drivers/clk/ics8n3qv01.c
>> new file mode 100644
>> index 0000000000..f5f4b74982
>> --- /dev/null
>> +++ b/drivers/clk/ics8n3qv01.c
>> @@ -0,0 +1,184 @@
>> +/*
>> + * (C) Copyright 2017
>> + * Mario Six, Guntermann & Drunck GmbH, mario.six at gdsys.cc
>> + *
>> + * based on the gdsys osd driver, which is
>> + *
>> + * (C) Copyright 2010
>> + * Dirk Eibach, Guntermann & Drunck GmbH, eibach at gdsys.de
>> + *
>> + * SPDX-License-Identifier: GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <dm.h>
>> +#include <clk-uclass.h>
>> +#include <i2c.h>
>> +
>> +const long long ICS8N3QV01_FREF = 114285000;
>> +const long long ICS8N3QV01_FREF_LL = 114285000LL;
>> +const long long ICS8N3QV01_F_DEFAULT_0 = 156250000LL;
>> +const long long ICS8N3QV01_F_DEFAULT_1 = 125000000LL;
>> +const long long ICS8N3QV01_F_DEFAULT_2 = 100000000LL;
>> +const long long ICS8N3QV01_F_DEFAULT_3 = 25175000LL;
>> +
>> +struct ics8n3qv01_priv {
>> + ulong rate;
>> +};
>> +
>> +static uint ics8n3qv01_get_fout_calc(struct udevice *dev, uint index)
>> +{
>> + u64 n, mint, mfrac;
>> + u8 reg_a, reg_b, reg_c, reg_d, reg_f;
>> + u64 fout_calc;
>> +
>> + if (index > 3)
>
> What is 3? Should this be an enum/#define?
>
The device is a VCXO (Voltage-Controlled Crystal Oscillator) that supplies four
frequencies (indexed 0 to 3), so accessing frequency indexes higher than 3
makes no sense. Maybe a MAX_FREQ_INDEX define would help to clarify the intent
here.
>> + return 0;
>> +
>> + reg_a = dm_i2c_reg_read(dev, 0 + index);
>
> Error checking?
>
I'll refactor the method to return an error code and use a output parameter for
the frequency (and add error checking, of course).
>> + reg_b = dm_i2c_reg_read(dev, 4 + index);
>> + reg_c = dm_i2c_reg_read(dev, 8 + index);
>> + reg_d = dm_i2c_reg_read(dev, 12 + index);
>> + reg_f = dm_i2c_reg_read(dev, 20 + index);
>> +
>> + mint = ((reg_a >> 1) & 0x1f) | (reg_f & 0x20);
>> + mfrac = ((reg_a & 0x01) << 17) | (reg_b << 9) | (reg_c << 1)
>> + | (reg_d >> 7);
>> + n = reg_d & 0x7f;
>> +
>> + fout_calc = (mint * ICS8N3QV01_FREF_LL
>> + + mfrac * ICS8N3QV01_FREF_LL / 262144LL
>> + + ICS8N3QV01_FREF_LL / 524288LL
>> + + n / 2)
>> + / n
>> + * 1000000
>> + / (1000000 - 100);
>> +
>> + return fout_calc;
>> +}
>> +
>> +static void ics8n3qv01_calc_parameters(uint fout, uint *_mint, uint *_mfrac,
>> + uint *_n)
>> +{
>> + uint n, foutiic, fvcoiic, mint;
>> + u64 mfrac;
>> +
>> + n = (2215000000U + fout / 2) / fout;
>> + if ((n & 1) && (n > 5))
>> + n -= 1;
>> +
>> + foutiic = fout - (fout / 10000);
>> + fvcoiic = foutiic * n;
>> +
>> + mint = fvcoiic / 114285000;
>> + if ((mint < 17) || (mint > 63))
>> + printf("ics8n3qv01_calc_parameters: cannot determine mint\n");
>
> return error?
>
dito.
>> +
>> + mfrac = ((u64)fvcoiic % 114285000LL) * 262144LL
>> + / 114285000LL;
>> +
>> + *_mint = mint;
>> + *_mfrac = mfrac;
>> + *_n = n;
>> +}
>> +
>> +static ulong ics8n3qv01_set_rate(struct clk *clk, ulong fout)
>> +{
>> + struct ics8n3qv01_priv *priv = dev_get_priv(clk->dev);
>> + uint n, mint, mfrac, fout_calc;
>> + u64 fout_prog;
>> + long long off_ppm;
>> + u8 reg0, reg4, reg8, reg12, reg18, reg20;
>> +
>> + priv->rate = fout;
>> +
>> + fout_calc = ics8n3qv01_get_fout_calc(clk->dev, 1);
>> + off_ppm = (fout_calc - ICS8N3QV01_F_DEFAULT_1) * 1000000
>> + / ICS8N3QV01_F_DEFAULT_1;
>> + printf("%s: PLL is off by %lld ppm\n", clk->dev->name, off_ppm);
>
> debug()?
>
That's actually the "status message" we want to see from the clock during
startup; if we know that the value is reasonable during problem diagnosis, we
can be pretty sure that at least the pixel clock works reliably.
>> + fout_prog = (u64)fout * (u64)fout_calc
>> + / ICS8N3QV01_F_DEFAULT_1;
>> + ics8n3qv01_calc_parameters(fout_prog, &mint, &mfrac, &n);
>> +
>> + reg0 = dm_i2c_reg_read(clk->dev, 0) & 0xc0;
>> + reg0 |= (mint & 0x1f) << 1;
>> + reg0 |= (mfrac >> 17) & 0x01;
>> + dm_i2c_reg_write(clk->dev, 0, reg0);
>
> Check error?
>
See above, will refactor in v2.
>> +
>> + reg4 = mfrac >> 9;
>> + dm_i2c_reg_write(clk->dev, 4, reg4);
>> +
>> + reg8 = mfrac >> 1;
>> + dm_i2c_reg_write(clk->dev, 8, reg8);
>> +
>> + reg12 = mfrac << 7;
>> + reg12 |= n & 0x7f;
>> + dm_i2c_reg_write(clk->dev, 12, reg12);
>> +
>> + reg18 = dm_i2c_reg_read(clk->dev, 18) & 0x03;
>> + reg18 |= 0x20;
>> + dm_i2c_reg_write(clk->dev, 18, reg18);
>> +
>> + reg20 = dm_i2c_reg_read(clk->dev, 20) & 0x1f;
>> + reg20 |= mint & (1 << 5);
>> + dm_i2c_reg_write(clk->dev, 20, reg20);
>> +
>> + return 0;
>> +}
>> +
>> +static int ics8n3qv01_request(struct clk *clock)
>> +{
>> + return 0;
>> +}
>> +
>> +static ulong ics8n3qv01_get_rate(struct clk *clk)
>> +{
>> + struct ics8n3qv01_priv *priv = dev_get_priv(clk->dev);
>> +
>> + return priv->rate;
>> +}
>> +
>> +static int ics8n3qv01_enable(struct clk *clk)
>> +{
>> + return 0;
>> +}
>> +
>> +static int ics8n3qv01_disable(struct clk *clk)
>> +{
>> + return 0;
>> +}
>> +
>> +static const struct clk_ops ics8n3qv01_ops = {
>> + .request = ics8n3qv01_request,
>> + .get_rate = ics8n3qv01_get_rate,
>> + .set_rate = ics8n3qv01_set_rate,
>> + .enable = ics8n3qv01_enable,
>> + .disable = ics8n3qv01_disable,
>> +};
>> +
>> +static const struct udevice_id ics8n3qv01_ids[] = {
>> + { .compatible = "idt,ics8n3qv01" },
>> + { /* sentinel */ }
>> +};
>> +
>> +int ics8n3qv01_probe(struct udevice *dev)
>> +{
>> + struct dm_i2c_chip *chip = dev_get_parent_platdata(dev);
>> + struct udevice *dummy;
>> +
>> + if (dm_i2c_probe(dev->parent, chip->chip_addr, chip->flags, &dummy)) {
>
> You should not need to probe here - DM should do this for you.
>
Ah, OK, the check was in the old ics8n3qv01 driver, so I just adapted it
verbatim to DM. I'll remove it in v2.
>> + printf("ics8n3qv01: I2C probe did not succeed.\n");
>> + return -1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +U_BOOT_DRIVER(ics8n3qv01) = {
>> + .name = "ics8n3qv01",
>> + .id = UCLASS_CLK,
>> + .ops = &ics8n3qv01_ops,
>> + .of_match = ics8n3qv01_ids,
>> + .probe = ics8n3qv01_probe,
>> + .priv_auto_alloc_size = sizeof(struct ics8n3qv01_priv),
>> +};
>> --
>> 2.11.0
>>
>
> Regards,
> Simon
Best regards,
Mario
More information about the U-Boot
mailing list