[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