Hi Simon, On Wed, Jul 19, 2017 at 11:05 AM, Simon Glass <s...@chromium.org> wrote: > On 14 July 2017 at 05:55, Mario Six <mario....@gdsys.cc> wrote: >> Add a driver for the ICS8N3QV01 Quad-Frequency Programmable VCXO. >> >> Signed-off-by: Mario Six <mario....@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....@gdsys.cc >> + * >> + * based on the gdsys osd driver, which is >> + * >> + * (C) Copyright 2010 >> + * Dirk Eibach, Guntermann & Drunck GmbH, eib...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot