| Message ID | 1379544219-23579-1-git-send-email-soren.brinkmann@xilinx.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 2013-09-18 at 15:43 -0700, Soren Brinkmann wrote: > Add a driver for SILabs 570, 571, 598, 599 programmable oscillators. > The devices generate low-jitter clock signals and are reprogrammable via > an I2C interface. [] > v2: [] > - use 10000 as MIN and MAX value in usleep_range [] > diff --git a/drivers/clk/clk-si570.c b/drivers/clk/clk-si570.c [] > +static int si570_set_frequency(struct clk_si570 *data, unsigned long frequency) > +{ [] > + /* Applying a new frequency can take up to 10ms */ > + usleep_range(10000, 10000); Generally it's nicer to have an actual range for usleep_range. Is there a bit you could periodically poll to see if the new frequency has been set or is stable so that a 10ms delay isn't always used?
On Wed, Sep 18, 2013 at 04:02:41PM -0700, Joe Perches wrote: > On Wed, 2013-09-18 at 15:43 -0700, Soren Brinkmann wrote: > > Add a driver for SILabs 570, 571, 598, 599 programmable oscillators. > > The devices generate low-jitter clock signals and are reprogrammable via > > an I2C interface. > [] > > v2: > [] > > - use 10000 as MIN and MAX value in usleep_range > [] > > diff --git a/drivers/clk/clk-si570.c b/drivers/clk/clk-si570.c > [] > > +static int si570_set_frequency(struct clk_si570 *data, unsigned long frequency) > > +{ > [] > > + /* Applying a new frequency can take up to 10ms */ > > + usleep_range(10000, 10000); > > Generally it's nicer to have an actual range for usleep_range. Well, as I said in the discussion with Guenther. I'm flexible and nobody objected when I said to make both equal. A real range doesn't make sense here though, but I don't know what's common practice for cases like this. > > Is there a bit you could periodically poll to see > if the new frequency has been set or is stable so > that a 10ms delay isn't always used? Unfortunately not. The data sheet describes one bit which is cleared when the new frequency configuration is applied, but it seems to be cleared before the output is actually stable/enabled. At least our testing failed when some driver changed the frequency and continued operation w/o this additional delay. Sören
On Wed, 2013-09-18 at 16:09 -0700, Sören Brinkmann wrote: > On Wed, Sep 18, 2013 at 04:02:41PM -0700, Joe Perches wrote: > > On Wed, 2013-09-18 at 15:43 -0700, Soren Brinkmann wrote: > > > Add a driver for SILabs 570, 571, 598, 599 programmable oscillators. > > > The devices generate low-jitter clock signals and are reprogrammable via > > > an I2C interface. > > [] > > > v2: > > [] > > > - use 10000 as MIN and MAX value in usleep_range > > [] > > > diff --git a/drivers/clk/clk-si570.c b/drivers/clk/clk-si570.c > > [] > > > +static int si570_set_frequency(struct clk_si570 *data, unsigned long frequency) > > > +{ > > [] > > > + /* Applying a new frequency can take up to 10ms */ > > > + usleep_range(10000, 10000); > > > > Generally it's nicer to have an actual range for usleep_range. > Well, as I said in the discussion with Guenther. I'm flexible and nobody > objected when I said to make both equal. A real range doesn't make sense > here though, but I don't know what's common practice for cases like > this. udelay is normal, but I guess you don't need atomic context. > > Is there a bit you could periodically poll to see > > if the new frequency has been set or is stable so > > that a 10ms delay isn't always used? > Unfortunately not. Thanks. I suppose I should read the datasheets before asking. http://www.silabs.com/Support%20Documents/TechnicalDocs/si570.pdf (page 12) Anyway, perhaps si570_set_frequency_small needs a delay too.' The 570 datasheet says it needs a 100uS delay. > On Wed, 2013-09-18 at 15:43 -0700, Soren Brinkmann wrote: > Add a driver for SILabs 570, 571, 598, 599 programmable oscillators. > The devices generate low-jitter clock signals and are reprogrammable via > an I2C interface. > [] > +static int si570_set_frequency_small(struct clk_si570 *data, > + unsigned long frequency) > +{ > + /* > + * This is a re-implementation of DIV_ROUND_CLOSEST > + * using the div64_u64 function lieu of letting the compiler > + * insert EABI calls > + */ > + data->rfreq = div64_u64((data->rfreq * frequency) + > + div_u64(data->frequency, 2), data->frequency); > + regmap_write(data->regmap, SI570_REG_CONTROL, SI570_CNTRL_FREEZE_M); > + si570_update_rfreq(data); > + regmap_write(data->regmap, SI570_REG_CONTROL, 0); > + > + return 0; > +}
On Wed, Sep 18, 2013 at 04:18:56PM -0700, Joe Perches wrote: > On Wed, 2013-09-18 at 16:09 -0700, Sören Brinkmann wrote: > > On Wed, Sep 18, 2013 at 04:02:41PM -0700, Joe Perches wrote: > > > On Wed, 2013-09-18 at 15:43 -0700, Soren Brinkmann wrote: > > > > Add a driver for SILabs 570, 571, 598, 599 programmable oscillators. > > > > The devices generate low-jitter clock signals and are reprogrammable via > > > > an I2C interface. > > > [] > > > > v2: > > > [] > > > > - use 10000 as MIN and MAX value in usleep_range > > > [] > > > > diff --git a/drivers/clk/clk-si570.c b/drivers/clk/clk-si570.c > > > [] > > > > +static int si570_set_frequency(struct clk_si570 *data, unsigned long frequency) > > > > +{ > > > [] > > > > + /* Applying a new frequency can take up to 10ms */ > > > > + usleep_range(10000, 10000); > > > > > > Generally it's nicer to have an actual range for usleep_range. > > Well, as I said in the discussion with Guenther. I'm flexible and nobody > > objected when I said to make both equal. A real range doesn't make sense > > here though, but I don't know what's common practice for cases like > > this. > > udelay is normal, but I guess you don't need atomic context. After checkpatch correcting me a few times I went with what Documentation/timers/timers-howto.txt suggests. But yes, then we have this situation, that I want to sleep 10ms, but not longer using a *_range function. I guess it is very application specific whether a longer delay here is acceptable or not. > > > > Is there a bit you could periodically poll to see > > > if the new frequency has been set or is stable so > > > that a 10ms delay isn't always used? > > > Unfortunately not. > > Thanks. I suppose I should read the datasheets before asking. > > http://www.silabs.com/Support%20Documents/TechnicalDocs/si570.pdf > (page 12) > > Anyway, perhaps si570_set_frequency_small needs a delay too.' > > The 570 datasheet says it needs a 100uS delay. You're right. I'll add a delay there as well. The 'rang' question applies here as well. Sören
On Wed, Sep 18, 2013 at 04:32:59PM -0700, Sören Brinkmann wrote: > On Wed, Sep 18, 2013 at 04:18:56PM -0700, Joe Perches wrote: > > On Wed, 2013-09-18 at 16:09 -0700, Sören Brinkmann wrote: > > > On Wed, Sep 18, 2013 at 04:02:41PM -0700, Joe Perches wrote: > > > > On Wed, 2013-09-18 at 15:43 -0700, Soren Brinkmann wrote: > > > > > Add a driver for SILabs 570, 571, 598, 599 programmable oscillators. > > > > > The devices generate low-jitter clock signals and are reprogrammable via > > > > > an I2C interface. > > > > [] > > > > > v2: > > > > [] > > > > > - use 10000 as MIN and MAX value in usleep_range > > > > [] > > > > > diff --git a/drivers/clk/clk-si570.c b/drivers/clk/clk-si570.c > > > > [] > > > > > +static int si570_set_frequency(struct clk_si570 *data, unsigned long frequency) > > > > > +{ > > > > [] > > > > > + /* Applying a new frequency can take up to 10ms */ > > > > > + usleep_range(10000, 10000); > > > > > > > > Generally it's nicer to have an actual range for usleep_range. > > > Well, as I said in the discussion with Guenther. I'm flexible and nobody > > > objected when I said to make both equal. A real range doesn't make sense > > > here though, but I don't know what's common practice for cases like > > > this. > > > > udelay is normal, but I guess you don't need atomic context. > After checkpatch correcting me a few times I went with what > Documentation/timers/timers-howto.txt suggests. But yes, then we have > this situation, that I want to sleep 10ms, but not longer using a > *_range function. I guess it is very application specific whether a > longer delay here is acceptable or not. > You really want to sleep and not call udelay for 10ms. The idea behind usleep_range is that you give the kernel some slack. In this case, you could for example make it 10-12 ms. That doesn't make much difference for the driver, but it might save a timer interrupt in the kernel because it might be able to coalesce more than one event. After all, it doesn't have to be _exactly_ 10 ms, which is what you are claiming with the fixed number. Prior to usleep_range, you would have happily called msleep(10) without realizing that it might sleep up to 20 ms on you. Keep that in mind ... > You're right. I'll add a delay there as well. The 'rang' question > applies here as well. > Same thing, really. You could make it 100-200uS. That doesn't make much difference for this driver, but it might make a difference for overall performance, especially if everyone is playing nicely. Guenter
On Wed, Sep 18, 2013 at 03:43:38PM -0700, Soren Brinkmann wrote: > Add a driver for SILabs 570, 571, 598, 599 programmable oscillators. > The devices generate low-jitter clock signals and are reprogrammable via > an I2C interface. > > Cc: Guenter Roeck <linux@roeck-us.net> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > --- > v2: > - document clock-output-names in bindings documentation > - don't use wildcards in compatibility string > - change Kconfig entry to "... 570 and compatible devices" > - change some indentation flaws > - use 10000 as MIN and MAX value in usleep_range > - fail probe() if 'factory-fout' is not provided in DT > - default factory fout #defines removed > - use i2c driver_data instead of OF data > - remove some related structs and data > - rename DT property 'initial-fout' => 'clock-frequency' (like si5351 > driver) > - add some more details regarding the 'factory-fout' DT property > > .../devicetree/bindings/clock/silabs,si570.txt | 38 ++ > drivers/clk/Kconfig | 10 + > drivers/clk/Makefile | 1 + > drivers/clk/clk-si570.c | 517 +++++++++++++++++++++ > 4 files changed, 566 insertions(+) > create mode 100644 Documentation/devicetree/bindings/clock/silabs,si570.txt > create mode 100644 drivers/clk/clk-si570.c > > diff --git a/Documentation/devicetree/bindings/clock/silabs,si570.txt b/Documentation/devicetree/bindings/clock/silabs,si570.txt > new file mode 100644 > index 0000000..7ab5c8b > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/silabs,si570.txt > @@ -0,0 +1,38 @@ > +Binding for Silicon Labs 570, 571, 598 and 599 programmable > +I2C clock generators. > + > +Reference > +This binding uses the common clock binding[1]. Details about the devices can be > +found in the data sheets[2][3]. > + > +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt > +[2] Si570/571 Data Sheet > + http://www.silabs.com/Support%20Documents/TechnicalDocs/si570.pdf > +[3] Si598/599 Data Sheet > + http://www.silabs.com/Support%20Documents/TechnicalDocs/si598-99.pdf > + > +Required properties: > + - compatible: Shall be one of "silabs,si570", "silabs,si571", > + "silabs,si598", "silabs,si599" > + - reg: I2C device address. > + - #clock-cells: From common clock bindings: Shall be 0. > + - factory-fout: Factory set default frequency. This frequency is part specific. > + The correct frequency for the part used has to be provided in > + order to generate the correct output frequencies. For more > + details, please refer to the data sheet. > + > +Optional properties: > + - clock-output-names: From common clock bindings. Recommended to be "si570". > + - clock-frequency: Output frequency to generate. This defines the output > + frequency set during boot. It can be reprogrammed during > + runtime using the common clock framework. > + - temperature-stability-7ppm: Indicate a device with a temperature stability > + of 7ppm > + > +Example: > + si570: clock-generator@5d { > + #clock-cells = <0>; > + compatible = "silabs,si570"; > + reg = <0x5d>; > + factory-fout = <156250000>; > + }; > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig > index 279407a..349b88a 100644 > --- a/drivers/clk/Kconfig > +++ b/drivers/clk/Kconfig > @@ -64,6 +64,16 @@ config COMMON_CLK_SI5351 > This driver supports Silicon Labs 5351A/B/C programmable clock > generators. > > +config COMMON_CLK_SI570 > + tristate "Clock driver for SiLabs 570 and compatible devices" > + depends on I2C > + depends on OF > + select REGMAP_I2C > + help > + ---help--- > + This driver supports Silicon Labs 570/571/598/599 programmable > + clock generators. > + > config COMMON_CLK_S2MPS11 > tristate "Clock driver for S2MPS11 MFD" > depends on MFD_SEC_CORE > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile > index 7b11106..c0e94b3 100644 > --- a/drivers/clk/Makefile > +++ b/drivers/clk/Makefile > @@ -40,6 +40,7 @@ obj-$(CONFIG_COMMON_CLK_AXI_CLKGEN) += clk-axi-clkgen.o > obj-$(CONFIG_COMMON_CLK_WM831X) += clk-wm831x.o > obj-$(CONFIG_COMMON_CLK_MAX77686) += clk-max77686.o > obj-$(CONFIG_COMMON_CLK_SI5351) += clk-si5351.o > +obj-$(CONFIG_COMMON_CLK_SI570) += clk-si570.o > obj-$(CONFIG_COMMON_CLK_S2MPS11) += clk-s2mps11.o > obj-$(CONFIG_CLK_TWL6040) += clk-twl6040.o > obj-$(CONFIG_CLK_PPC_CORENET) += clk-ppc-corenet.o > diff --git a/drivers/clk/clk-si570.c b/drivers/clk/clk-si570.c > new file mode 100644 > index 0000000..c20dfce > --- /dev/null > +++ b/drivers/clk/clk-si570.c > @@ -0,0 +1,517 @@ > +/* > + * Driver for Silicon Labs Si570/Si571 Programmable XO/VCXO > + * > + * Copyright (C) 2010, 2011 Ericsson AB. > + * Copyright (C) 2011 Guenter Roeck. > + * Copyright (C) 2011 - 2013 Xilinx Inc. > + * > + * Author: Guenter Roeck <guenter.roeck@ericsson.com> > + * Sören Brinkmann <soren.brinkmann@xilinx.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/clk-provider.h> > +#include <linux/delay.h> > +#include <linux/module.h> > +#include <linux/i2c.h> > +#include <linux/regmap.h> > +#include <linux/slab.h> > + > +/* Si570 registers */ > +#define SI570_REG_HS_N1 7 > +#define SI570_REG_N1_RFREQ0 8 > +#define SI570_REG_RFREQ1 9 > +#define SI570_REG_RFREQ2 10 > +#define SI570_REG_RFREQ3 11 > +#define SI570_REG_RFREQ4 12 > +#define SI570_REG_CONTROL 135 > +#define SI570_REG_FREEZE_DCO 137 > +#define SI570_DIV_OFFSET_7PPM 6 > + > +#define HS_DIV_SHIFT 5 > +#define HS_DIV_MASK 0xe0 > +#define HS_DIV_OFFSET 4 > +#define N1_6_2_MASK 0x1f > +#define N1_1_0_MASK 0xc0 > +#define RFREQ_37_32_MASK 0x3f > + > +#define SI570_MIN_FREQ 10000000L > +#define SI570_MAX_FREQ 1417500000L > +#define SI598_MAX_FREQ 525000000L > + > +#define FDCO_MIN 4850000000LL > +#define FDCO_MAX 5670000000LL > + > +#define SI570_CNTRL_RECALL (1 << 0) > +#define SI570_CNTRL_FREEZE_M (1 << 5) > +#define SI570_CNTRL_NEWFREQ (1 << 6) > + > +#define SI570_FREEZE_DCO (1 << 4) > + > +/** > + * struct clk_si570: > + * @hw: Clock hw struct > + * @regmap: Device's regmap > + * @div_offset: Rgister offset for dividers > + * @max_freq: Maximum frequency for this device > + * @fxtal: Factory xtal frequency > + * @n1: Clock divider N1 > + * @hs_div: Clock divider HSDIV > + * @rfreq: Clock multiplier RFREQ > + * @frequency: Current output frequency > + * @i2c_client: I2C client pointer > + */ > +struct clk_si570 { > + struct clk_hw hw; > + struct regmap *regmap; > + unsigned int div_offset; > + u64 max_freq; > + u64 fxtal; > + unsigned int n1; > + unsigned int hs_div; > + u64 rfreq; > + u64 frequency; > + struct i2c_client *i2c_client; > +}; > +#define to_clk_si570(_hw) container_of(_hw, struct clk_si570, hw) > + > +/** > + * si570_get_divs() - Read clock dividers from HW > + * @data: Pointer to struct clk_si570 > + * @rfreq: Fractional multiplier (output) > + * @n1: Divider N1 (output) > + * @hs_div: Divider HSDIV (output) > + * Returns 0 on success, negative errno otherwise. > + * > + * Retrieve clock dividers and multipliers from the HW. > + */ > +static int si570_get_divs(struct clk_si570 *data, u64 *rfreq, > + unsigned int *n1, unsigned int *hs_div) > +{ > + int err; > + u8 reg[6]; > + u64 tmp; > + > + err = regmap_bulk_read(data->regmap, SI570_REG_HS_N1 + data->div_offset, > + reg, ARRAY_SIZE(reg)); > + if (err) > + return err; > + > + *hs_div = ((reg[0] & HS_DIV_MASK) >> HS_DIV_SHIFT) + HS_DIV_OFFSET; > + *n1 = ((reg[0] & N1_6_2_MASK) << 2) + ((reg[1] & N1_1_0_MASK) >> 6) + 1; > + /* Handle invalid cases */ > + if (*n1 > 1) > + *n1 &= ~1; > + > + tmp = reg[1] & RFREQ_37_32_MASK; > + tmp = (tmp << 8) + reg[2]; > + tmp = (tmp << 8) + reg[3]; > + tmp = (tmp << 8) + reg[4]; > + tmp = (tmp << 8) + reg[5]; > + *rfreq = tmp; > + > + return 0; > +} > + > +/** > + * si570_get_defaults() - Get default values > + * @data: Driver data structure > + * @fout: Factory frequency output > + * Returns 0 on success, negative errno otherwise. > + */ > +static int si570_get_defaults(struct clk_si570 *data, u64 fout) > +{ > + int err; > + u64 fdco; > + > + regmap_write(data->regmap, SI570_REG_CONTROL, SI570_CNTRL_RECALL); > + > + err = si570_get_divs(data, &data->rfreq, &data->n1, &data->hs_div); > + if (err) > + return err; > + > + /* > + * Accept optional precision loss to avoid arithmetic overflows. > + * Acceptable per Silicon Labs Application Note AN334. > + */ > + fdco = fout * data->n1 * data->hs_div; > + if (fdco >= (1LL << 36)) > + data->fxtal = div64_u64(fdco << 24, data->rfreq >> 4); > + else > + data->fxtal = div64_u64(fdco << 28, data->rfreq); > + > + data->frequency = fout; > + > + return 0; > +} > + > +/** > + * si570_update_rfreq() - Update clock multiplier > + * @data: Driver data structure > + * Passes on regmap_bulk_write() return value. > + */ > +static int si570_update_rfreq(struct clk_si570 *data) > +{ > + u8 reg[5]; > + > + reg[0] = ((data->n1 - 1) << 6) | > + ((data->rfreq >> 32) & RFREQ_37_32_MASK); > + reg[1] = (data->rfreq >> 24) & 0xff; > + reg[2] = (data->rfreq >> 16) & 0xff; > + reg[3] = (data->rfreq >> 8) & 0xff; > + reg[4] = data->rfreq & 0xff; > + > + return regmap_bulk_write(data->regmap, SI570_REG_N1_RFREQ0 + > + data->div_offset, reg, ARRAY_SIZE(reg)); > +} > + > +/** > + * si570_calc_divs() - Caluclate clock dividers > + * @frequency: Target frequency > + * @data: Driver data structure > + * @out_rfreq: RFREG fractional multiplier (output) > + * @out_n1: Clock divider N1 (output) > + * @out_hs_div: Clock divider HSDIV (output) > + * Returns 0 on success, negative errno otherwise. > + * > + * Calculate the clock dividers (@out_hs_div, @out_n1) and clock multiplier > + * (@out_rfreq) for a given target @frequency. > + */ > +static int si570_calc_divs(unsigned long frequency, struct clk_si570 *data, > + u64 *out_rfreq, unsigned int *out_n1, unsigned int *out_hs_div) > +{ > + int i; > + unsigned int n1, hs_div; > + u64 fdco, best_fdco = ULLONG_MAX; > + static const uint8_t si570_hs_div_values[] = { 11, 9, 7, 6, 5, 4 }; > + > + for (i = 0; i < ARRAY_SIZE(si570_hs_div_values); i++) { > + hs_div = si570_hs_div_values[i]; > + /* Calculate lowest possible value for n1 */ > + n1 = div_u64(div_u64(FDCO_MIN, hs_div), frequency); > + if (!n1 || (n1 & 1)) > + n1++; > + while (n1 <= 128) { > + fdco = (u64)frequency * (u64)hs_div * (u64)n1; > + if (fdco > FDCO_MAX) > + break; > + if (fdco >= FDCO_MIN && fdco < best_fdco) { > + *out_n1 = n1; > + *out_hs_div = hs_div; > + *out_rfreq = div64_u64(fdco << 28, data->fxtal); > + best_fdco = fdco; > + } > + n1 += (n1 == 1 ? 1 : 2); > + } > + } > + > + if (best_fdco == ULLONG_MAX) > + return -EINVAL; > + > + return 0; > +} > + > +static unsigned long si570_recalc_rate(struct clk_hw *hw, > + unsigned long parent_rate) > +{ > + int err; > + u64 rfreq, rate; > + unsigned int n1, hs_div; > + struct clk_si570 *data = to_clk_si570(hw); > + > + err = si570_get_divs(data, &rfreq, &n1, &hs_div); > + if (err) { > + dev_err(&data->i2c_client->dev, "unable to recalc rate\n"); [ Not really helpful that you can not return an error here ] > + return data->frequency; > + } > + > + rfreq = div_u64(rfreq, hs_div * n1); > + rate = (data->fxtal * rfreq) >> 28; > + > + return rate; > +} > + > +static long si570_round_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long *parent_rate) > +{ > + int err; > + u64 rfreq; > + unsigned int n1, hs_div; > + struct clk_si570 *data = to_clk_si570(hw); > + > + if (!rate) > + return 0; > + > + if (div64_u64(abs(rate - data->frequency) * 10000LL, > + data->frequency) < 35) { > + rfreq = div64_u64((data->rfreq * rate) + > + div64_u64(data->frequency, 2), data->frequency); > + n1 = data->n1; > + hs_div = data->hs_div; > + > + } else { > + err = si570_calc_divs(rate, data, &rfreq, &n1, &hs_div); > + if (err) { > + dev_err(&data->i2c_client->dev, > + "unable to round rate\n"); > + return 0; > + } > + } > + > + return rate; > +} > + > +/** > + * si570_set_frequency() - Adjust output frequency > + * @data: Driver data structure > + * @frequency: Target frequency > + * Returns 0 on success. > + * > + * Update output frequency for big frequency changes (> 3,500 ppm). > + */ > +static int si570_set_frequency(struct clk_si570 *data, unsigned long frequency) > +{ > + int err; > + > + err = si570_calc_divs(frequency, data, &data->rfreq, &data->n1, > + &data->hs_div); > + if (err) > + return err; > + > + /* > + * The DCO reg should be accessed with a read-modify-write operation > + * per AN334 > + */ > + regmap_write(data->regmap, SI570_REG_FREEZE_DCO, SI570_FREEZE_DCO); > + regmap_write(data->regmap, SI570_REG_HS_N1 + data->div_offset, > + ((data->hs_div - HS_DIV_OFFSET) << HS_DIV_SHIFT) | > + (((data->n1 - 1) >> 2) & N1_6_2_MASK)); > + si570_update_rfreq(data); > + regmap_write(data->regmap, SI570_REG_FREEZE_DCO, 0); > + regmap_write(data->regmap, SI570_REG_CONTROL, SI570_CNTRL_NEWFREQ); > + > + /* Applying a new frequency can take up to 10ms */ > + usleep_range(10000, 10000); > + > + return 0; > +} > + > +/** > + * si570_set_frequency_small() - Adjust output frequency > + * @data: Driver data structure > + * @frequency: Target frequency > + * Returns 0 on success. > + * > + * Update output frequency for small frequency changes (< 3,500 ppm). > + */ > +static int si570_set_frequency_small(struct clk_si570 *data, > + unsigned long frequency) > +{ > + /* > + * This is a re-implementation of DIV_ROUND_CLOSEST > + * using the div64_u64 function lieu of letting the compiler > + * insert EABI calls > + */ > + data->rfreq = div64_u64((data->rfreq * frequency) + > + div_u64(data->frequency, 2), data->frequency); > + regmap_write(data->regmap, SI570_REG_CONTROL, SI570_CNTRL_FREEZE_M); > + si570_update_rfreq(data); > + regmap_write(data->regmap, SI570_REG_CONTROL, 0); > + > + return 0; > +} > + > +static int si570_set_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long parent_rate) > +{ > + struct clk_si570 *data = to_clk_si570(hw); > + struct i2c_client *client = data->i2c_client; > + int err; > + > + if (rate < SI570_MIN_FREQ || rate > data->max_freq) { > + dev_warn(&client->dev, > + "requested frequency %lu Hz is out of range\n", rate); Shouldn't this be dev_err ? > + return -EINVAL; > + } > + > + if (div64_u64(abs(rate - data->frequency) * 10000LL, > + data->frequency) < 35) > + err = si570_set_frequency_small(data, rate); > + else > + err = si570_set_frequency(data, rate); > + > + if (err) > + return err; > + > + data->frequency = rate; > + > + return 0; > +} > + > +static const struct clk_ops si570_clk_ops = { > + .recalc_rate = si570_recalc_rate, > + .round_rate = si570_round_rate, > + .set_rate = si570_set_rate, > +}; > + > +static bool si570_regmap_is_volatile(struct device *dev, unsigned int reg) > +{ > + switch (reg) { > + case 135: > + return true; > + default: > + return false; > + } > +} > + > +static bool si570_regmap_is_writeable(struct device *dev, unsigned int reg) > +{ > + switch (reg) { > + case 7 ... 18: > + case 135: > + case 137: > + return true; > + default: > + return false; > + } > +} > + > +static struct regmap_config si570_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .cache_type = REGCACHE_RBTREE, > + .max_register = 137, > + .writeable_reg = si570_regmap_is_writeable, > + .volatile_reg = si570_regmap_is_volatile, > +}; > + > +static int si570_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct clk_si570 *data; > + struct clk_init_data init; > + struct clk *clk; > + u32 initial_fout, factory_fout; > + int err; > + > + data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + init.ops = &si570_clk_ops; > + init.flags = CLK_IS_ROOT; > + init.num_parents = 0; > + data->hw.init = &init; > + data->max_freq = id->driver_data; > + data->i2c_client = client; > + > + if (of_property_read_bool(client->dev.of_node, > + "temperature-stability-7ppm")) > + data->div_offset = SI570_DIV_OFFSET_7PPM; > + Just noticed that you dropped platform data support. Doesn't matter much for me right now, but in my previous company we used the chip on an x86 system which does not support devicetree. Would be nice to keep it and derive platform data from devicetree data if provided, like other drivers do it. The 7ppm option is only relevant for si570/si751 and not supported on si598/si599. You should mention that in the bindings document and check for it. Even better would be if it can be auto-detected; in that case you would not need the property at all. Unfortunately I don't have access to a chip, so I have no idea if that is possible and can not check it myself either. > + if (of_property_read_string(client->dev.of_node, "clock-output-names", > + &init.name)) > + init.name = client->dev.of_node->name; > + > + err = of_property_read_u32(client->dev.of_node, "factory-fout", > + &factory_fout); > + if (err) { > + dev_err(&client->dev, "'factory-fout' property missing\n"); > + return err; > + } > + > + data->regmap = devm_regmap_init_i2c(client, &si570_regmap_config); > + if (IS_ERR(data->regmap)) { > + dev_err(&client->dev, "failed to allocate register map\n"); > + return PTR_ERR(data->regmap); > + } > + > + i2c_set_clientdata(client, data); > + err = si570_get_defaults(data, factory_fout); > + if (err) > + return err; > + > + clk = devm_clk_register(&client->dev, &data->hw); > + if (IS_ERR(clk)) { > + dev_err(&client->dev, "clock registration failed\n"); > + return PTR_ERR(clk); > + } > + err = of_clk_add_provider(client->dev.of_node, of_clk_src_simple_get, > + clk); > + if (err) { > + dev_err(&client->dev, "unable to add clk provider\n"); > + return err; > + } > + > + /* > + * Read the requested initial fout from either platform data or the > + * device tree > + */ But you don't do anything with platform data. > + if (!of_property_read_u32(client->dev.of_node, "clock-frequency", > + &initial_fout)) { > + err = clk_set_rate(clk, initial_fout); > + if (err) > + dev_warn(&client->dev, > + "unable to set initial output frequency %u: %d\n", > + initial_fout, err); No bailout ? Also it seems that this generates two error messages, once in the code which experiences the error and once here. Maybe it would be better to just bail out and return the error. After all, something is seriously wrong and the system won't operate as specified. > + } > + > + /* Display a message indicating that we've successfully registered */ > + dev_info(&client->dev, "registered, current frequency %llu Hz\n", > + data->frequency); > + > + return 0; > +} > + > +static int si570_remove(struct i2c_client *client) > +{ > + struct clk_si570 *data = i2c_get_clientdata(client); > + > + of_clk_del_provider(client->dev.of_node); > + clk_unregister(data->hw.clk); > + > + return 0; > +} > + > +static const struct i2c_device_id si570_id[] = { > + { "si570", SI570_MAX_FREQ }, > + { "si571", SI570_MAX_FREQ }, > + { "si598", SI598_MAX_FREQ }, > + { "si599", SI598_MAX_FREQ }, > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, si570_id); > + > +static const struct of_device_id clk_si570_of_match[] = { > + { .compatible = "silabs,si570" }, > + { .compatible = "silabs,si571" }, > + { .compatible = "silabs,si598" }, > + { .compatible = "silabs,si599" }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, clk_si570_of_match); > + > +static struct i2c_driver si570_driver = { > + .driver = { > + .name = "si570", > + .of_match_table = of_match_ptr(clk_si570_of_match), > + }, > + .probe = si570_probe, > + .remove = si570_remove, > + .id_table = si570_id, > +}; > +module_i2c_driver(si570_driver); > + > +MODULE_AUTHOR("Guenter Roeck <guenter.roeck@ericsson.com>"); > +MODULE_AUTHOR("Soeren Brinkmann <soren.brinkmann@xilinx.com"); > +MODULE_DESCRIPTION("Si570 driver"); > +MODULE_LICENSE("GPL"); > -- > 1.8.4 > >
On Wed, Sep 18, 2013 at 05:23:08PM -0700, Guenter Roeck wrote: > On Wed, Sep 18, 2013 at 04:32:59PM -0700, Sören Brinkmann wrote: > > On Wed, Sep 18, 2013 at 04:18:56PM -0700, Joe Perches wrote: > > > On Wed, 2013-09-18 at 16:09 -0700, Sören Brinkmann wrote: > > > > On Wed, Sep 18, 2013 at 04:02:41PM -0700, Joe Perches wrote: > > > > > On Wed, 2013-09-18 at 15:43 -0700, Soren Brinkmann wrote: > > > > > > Add a driver for SILabs 570, 571, 598, 599 programmable oscillators. > > > > > > The devices generate low-jitter clock signals and are reprogrammable via > > > > > > an I2C interface. > > > > > [] > > > > > > v2: > > > > > [] > > > > > > - use 10000 as MIN and MAX value in usleep_range > > > > > [] > > > > > > diff --git a/drivers/clk/clk-si570.c b/drivers/clk/clk-si570.c > > > > > [] > > > > > > +static int si570_set_frequency(struct clk_si570 *data, unsigned long frequency) > > > > > > +{ > > > > > [] > > > > > > + /* Applying a new frequency can take up to 10ms */ > > > > > > + usleep_range(10000, 10000); > > > > > > > > > > Generally it's nicer to have an actual range for usleep_range. > > > > Well, as I said in the discussion with Guenther. I'm flexible and nobody > > > > objected when I said to make both equal. A real range doesn't make sense > > > > here though, but I don't know what's common practice for cases like > > > > this. > > > > > > udelay is normal, but I guess you don't need atomic context. > > After checkpatch correcting me a few times I went with what > > Documentation/timers/timers-howto.txt suggests. But yes, then we have > > this situation, that I want to sleep 10ms, but not longer using a > > *_range function. I guess it is very application specific whether a > > longer delay here is acceptable or not. > > > You really want to sleep and not call udelay for 10ms. The idea behind usleep_range > is that you give the kernel some slack. In this case, you could for example make it > 10-12 ms. That doesn't make much difference for the driver, but it might save a > timer interrupt in the kernel because it might be able to coalesce more than one > event. After all, it doesn't have to be _exactly_ 10 ms, which is what you are > claiming with the fixed number. Prior to usleep_range, you would have happily > called msleep(10) without realizing that it might sleep up to 20 ms on you. > Keep that in mind ... > > > You're right. I'll add a delay there as well. The 'rang' question > > applies here as well. > > > Same thing, really. You could make it 100-200uS. That doesn't make much > difference for this driver, but it might make a difference for overall > performance, especially if everyone is playing nicely. > Okay, so I'll use a real range. 10 - 12 ms for big frequency changes and 100 - 200 us for small ones, as Guenther suggests. Does that sound okay? Thanks, Sören
On Thu, Sep 19, 2013 at 06:17:12AM -0700, Guenter Roeck wrote: > On Wed, Sep 18, 2013 at 03:43:38PM -0700, Soren Brinkmann wrote: > > Add a driver for SILabs 570, 571, 598, 599 programmable oscillators. > > The devices generate low-jitter clock signals and are reprogrammable via > > an I2C interface. > > > > Cc: Guenter Roeck <linux@roeck-us.net> > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > > --- > > v2: > > - document clock-output-names in bindings documentation > > - don't use wildcards in compatibility string > > - change Kconfig entry to "... 570 and compatible devices" > > - change some indentation flaws > > - use 10000 as MIN and MAX value in usleep_range > > - fail probe() if 'factory-fout' is not provided in DT > > - default factory fout #defines removed > > - use i2c driver_data instead of OF data > > - remove some related structs and data > > - rename DT property 'initial-fout' => 'clock-frequency' (like si5351 > > driver) > > - add some more details regarding the 'factory-fout' DT property > > > > .../devicetree/bindings/clock/silabs,si570.txt | 38 ++ > > drivers/clk/Kconfig | 10 + > > drivers/clk/Makefile | 1 + > > drivers/clk/clk-si570.c | 517 +++++++++++++++++++++ > > 4 files changed, 566 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/clock/silabs,si570.txt > > create mode 100644 drivers/clk/clk-si570.c > > [...] > > diff --git a/drivers/clk/clk-si570.c b/drivers/clk/clk-si570.c > > new file mode 100644 > > index 0000000..c20dfce > > --- /dev/null > > +++ b/drivers/clk/clk-si570.c [...] > > +static unsigned long si570_recalc_rate(struct clk_hw *hw, > > + unsigned long parent_rate) > > +{ > > + int err; > > + u64 rfreq, rate; > > + unsigned int n1, hs_div; > > + struct clk_si570 *data = to_clk_si570(hw); > > + > > + err = si570_get_divs(data, &rfreq, &n1, &hs_div); > > + if (err) { > > + dev_err(&data->i2c_client->dev, "unable to recalc rate\n"); > > [ Not really helpful that you can not return an error here ] IIUC, the CCF does not really support error reporting for this function. I think a return value of recalc_rate is always interpreted as frequency. Hence, I decided to take the last point of certainty and return the last known frequency. > > > + return data->frequency; > > + } > > + > > + rfreq = div_u64(rfreq, hs_div * n1); > > + rate = (data->fxtal * rfreq) >> 28; > > + > > + return rate; > > +} [...] > > +static int si570_set_rate(struct clk_hw *hw, unsigned long rate, > > + unsigned long parent_rate) > > +{ > > + struct clk_si570 *data = to_clk_si570(hw); > > + struct i2c_client *client = data->i2c_client; > > + int err; > > + > > + if (rate < SI570_MIN_FREQ || rate > data->max_freq) { > > + dev_warn(&client->dev, > > + "requested frequency %lu Hz is out of range\n", rate); > > Shouldn't this be dev_err ? You're probably right. I'll change this. > > > + return -EINVAL; > > + } [...] > > +static int si570_probe(struct i2c_client *client, > > + const struct i2c_device_id *id) > > +{ > > + struct clk_si570 *data; > > + struct clk_init_data init; > > + struct clk *clk; > > + u32 initial_fout, factory_fout; > > + int err; > > + > > + data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL); > > + if (!data) > > + return -ENOMEM; > > + > > + init.ops = &si570_clk_ops; > > + init.flags = CLK_IS_ROOT; > > + init.num_parents = 0; > > + data->hw.init = &init; > > + data->max_freq = id->driver_data; > > + data->i2c_client = client; > > + > > + if (of_property_read_bool(client->dev.of_node, > > + "temperature-stability-7ppm")) > > + data->div_offset = SI570_DIV_OFFSET_7PPM; > > + > Just noticed that you dropped platform data support. Doesn't matter much for me > right now, but in my previous company we used the chip on an x86 system which > does not support devicetree. Would be nice to keep it and derive platform data > from devicetree data if provided, like other drivers do it. I'll look into this. The issue I have with that is, I can hardly test it since we only use this on Zynq which uses DT. So, I'd rather prefer to not include it unless somebody volunteers to test it. > The 7ppm option is only relevant for si570/si751 and not supported on > si598/si599. You should mention that in the bindings document and check for it. Right, I'll add a note in the doc. And ignore it for devices this does not apply. > Even better would be if it can be auto-detected; in that case you would not need > the property at all. Unfortunately I don't have access to a chip, so I have > no idea if that is possible and can not check it myself either. I didn't see any way to detect this during runtime. And we don't have such a device either. > > > + if (of_property_read_string(client->dev.of_node, "clock-output-names", > > + &init.name)) > > + init.name = client->dev.of_node->name; > > + > > + err = of_property_read_u32(client->dev.of_node, "factory-fout", > > + &factory_fout); > > + if (err) { > > + dev_err(&client->dev, "'factory-fout' property missing\n"); > > + return err; > > + } > > + > > + data->regmap = devm_regmap_init_i2c(client, &si570_regmap_config); > > + if (IS_ERR(data->regmap)) { > > + dev_err(&client->dev, "failed to allocate register map\n"); > > + return PTR_ERR(data->regmap); > > + } > > + > > + i2c_set_clientdata(client, data); > > + err = si570_get_defaults(data, factory_fout); > > + if (err) > > + return err; > > + > > + clk = devm_clk_register(&client->dev, &data->hw); > > + if (IS_ERR(clk)) { > > + dev_err(&client->dev, "clock registration failed\n"); > > + return PTR_ERR(clk); > > + } > > + err = of_clk_add_provider(client->dev.of_node, of_clk_src_simple_get, > > + clk); > > + if (err) { > > + dev_err(&client->dev, "unable to add clk provider\n"); > > + return err; > > + } > > + > > + /* > > + * Read the requested initial fout from either platform data or the > > + * device tree > > + */ > > But you don't do anything with platform data. copy&paste error. I'll remove it. > > > + if (!of_property_read_u32(client->dev.of_node, "clock-frequency", > > + &initial_fout)) { > > + err = clk_set_rate(clk, initial_fout); > > + if (err) > > + dev_warn(&client->dev, > > + "unable to set initial output frequency %u: %d\n", > > + initial_fout, err); > > No bailout ? > > Also it seems that this generates two error messages, once in the code which > experiences the error and once here. I remove the message here. > > Maybe it would be better to just bail out and return the error. > After all, something is seriously wrong and the system won't operate > as specified. I do more think of a spurious error (typo in DT prop giving an f out of range,...) and a driver actually controlling this clock generator later. In that case later calls to clk_set_rate() might succeed and everything's fine or the driver can handle the error. In case of using this device as a fixed clock, bailing out here might make more sense though. I'd prefer leaving it like this. Thanks, Sören
On Thu, Sep 19, 2013 at 09:01:01AM -0700, Sören Brinkmann wrote: > On Thu, Sep 19, 2013 at 06:17:12AM -0700, Guenter Roeck wrote: > > On Wed, Sep 18, 2013 at 03:43:38PM -0700, Soren Brinkmann wrote: > > > Add a driver for SILabs 570, 571, 598, 599 programmable oscillators. > > > The devices generate low-jitter clock signals and are reprogrammable via > > > an I2C interface. > > > [...] > > > +static unsigned long si570_recalc_rate(struct clk_hw *hw, > > > + unsigned long parent_rate) > > > +{ > > > + int err; > > > + u64 rfreq, rate; > > > + unsigned int n1, hs_div; > > > + struct clk_si570 *data = to_clk_si570(hw); > > > + > > > + err = si570_get_divs(data, &rfreq, &n1, &hs_div); > > > + if (err) { > > > + dev_err(&data->i2c_client->dev, "unable to recalc rate\n"); > > > > [ Not really helpful that you can not return an error here ] > IIUC, the CCF does not really support error reporting for this function. I > think a return value of recalc_rate is always interpreted as frequency. > Hence, I decided to take the last point of certainty and return the last > known frequency. > Yes, I wasn't complaining about you, but about the clock infrastructure. [...] > > > +static int si570_probe(struct i2c_client *client, > > > + const struct i2c_device_id *id) > > > +{ > > > + struct clk_si570 *data; > > > + struct clk_init_data init; > > > + struct clk *clk; > > > + u32 initial_fout, factory_fout; > > > + int err; > > > + > > > + data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL); > > > + if (!data) > > > + return -ENOMEM; > > > + > > > + init.ops = &si570_clk_ops; > > > + init.flags = CLK_IS_ROOT; > > > + init.num_parents = 0; > > > + data->hw.init = &init; > > > + data->max_freq = id->driver_data; > > > + data->i2c_client = client; > > > + > > > + if (of_property_read_bool(client->dev.of_node, > > > + "temperature-stability-7ppm")) > > > + data->div_offset = SI570_DIV_OFFSET_7PPM; > > > + > > Just noticed that you dropped platform data support. Doesn't matter much for me > > right now, but in my previous company we used the chip on an x86 system which > > does not support devicetree. Would be nice to keep it and derive platform data > > from devicetree data if provided, like other drivers do it. > I'll look into this. The issue I have with that is, I can hardly test it > since we only use this on Zynq which uses DT. So, I'd rather prefer to > not include it unless somebody volunteers to test it. > Fair enough. I can not test it myself anymore, and my previous employer now has a strict non-contributions-to-linux policy, so I guess they won't test it either or at least not publish any test results. Leave it out. > > The 7ppm option is only relevant for si570/si751 and not supported on > > si598/si599. You should mention that in the bindings document and check for it. > Right, I'll add a note in the doc. And ignore it for devices this does > not apply. > I would bail out, but that is your call. > > Even better would be if it can be auto-detected; in that case you would not need > > the property at all. Unfortunately I don't have access to a chip, so I have > > no idea if that is possible and can not check it myself either. > I didn't see any way to detect this during runtime. And we don't have > such a device either. > Check what the registers return when reading. But yes, you would need access to the various devices to be sure that the detection works. Wonder if SiLabs provides samples ... this one would be really nice to have, because it would enable us to drop the extra property. > > > > > + if (of_property_read_string(client->dev.of_node, "clock-output-names", > > > + &init.name)) > > > + init.name = client->dev.of_node->name; > > > + > > > + err = of_property_read_u32(client->dev.of_node, "factory-fout", > > > + &factory_fout); > > > + if (err) { > > > + dev_err(&client->dev, "'factory-fout' property missing\n"); > > > + return err; > > > + } > > > + > > > + data->regmap = devm_regmap_init_i2c(client, &si570_regmap_config); > > > + if (IS_ERR(data->regmap)) { > > > + dev_err(&client->dev, "failed to allocate register map\n"); > > > + return PTR_ERR(data->regmap); > > > + } > > > + > > > + i2c_set_clientdata(client, data); > > > + err = si570_get_defaults(data, factory_fout); > > > + if (err) > > > + return err; > > > + > > > + clk = devm_clk_register(&client->dev, &data->hw); > > > + if (IS_ERR(clk)) { > > > + dev_err(&client->dev, "clock registration failed\n"); > > > + return PTR_ERR(clk); > > > + } > > > + err = of_clk_add_provider(client->dev.of_node, of_clk_src_simple_get, > > > + clk); > > > + if (err) { > > > + dev_err(&client->dev, "unable to add clk provider\n"); > > > + return err; > > > + } > > > + > > > + /* > > > + * Read the requested initial fout from either platform data or the > > > + * device tree > > > + */ > > > > But you don't do anything with platform data. > copy&paste error. I'll remove it. > > > > > > + if (!of_property_read_u32(client->dev.of_node, "clock-frequency", > > > + &initial_fout)) { > > > + err = clk_set_rate(clk, initial_fout); > > > + if (err) > > > + dev_warn(&client->dev, > > > + "unable to set initial output frequency %u: %d\n", > > > + initial_fout, err); > > > > No bailout ? > > > > Also it seems that this generates two error messages, once in the code which > > experiences the error and once here. > I remove the message here. > > > > > Maybe it would be better to just bail out and return the error. > > After all, something is seriously wrong and the system won't operate > > as specified. > I do more think of a spurious error (typo in DT prop giving an f out of > range,...) and a driver actually controlling this clock generator later. > In that case later calls to clk_set_rate() might succeed and everything's > fine or the driver can handle the error. In case of using this device as > a fixed clock, bailing out here might make more sense though. I'd prefer > leaving it like this. > A typo in dt data isn't really a spurious error, and it would be easy to fix. I would suggest to bail out; this ensures that the devicetree data is correct. Guenter
Quoting Soren Brinkmann (2013-09-18 15:43:38) > diff --git a/Documentation/devicetree/bindings/clock/silabs,si570.txt b/Documentation/devicetree/bindings/clock/silabs,si570.txt > new file mode 100644 > index 0000000..7ab5c8b > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/silabs,si570.txt > @@ -0,0 +1,38 @@ > +Binding for Silicon Labs 570, 571, 598 and 599 programmable > +I2C clock generators. > + > +Reference > +This binding uses the common clock binding[1]. Details about the devices can be > +found in the data sheets[2][3]. > + > +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt > +[2] Si570/571 Data Sheet > + http://www.silabs.com/Support%20Documents/TechnicalDocs/si570.pdf > +[3] Si598/599 Data Sheet > + http://www.silabs.com/Support%20Documents/TechnicalDocs/si598-99.pdf > + > +Required properties: > + - compatible: Shall be one of "silabs,si570", "silabs,si571", > + "silabs,si598", "silabs,si599" > + - reg: I2C device address. > + - #clock-cells: From common clock bindings: Shall be 0. > + - factory-fout: Factory set default frequency. This frequency is part specific. > + The correct frequency for the part used has to be provided in > + order to generate the correct output frequencies. For more > + details, please refer to the data sheet. > + > +Optional properties: > + - clock-output-names: From common clock bindings. Recommended to be "si570". > + - clock-frequency: Output frequency to generate. This defines the output > + frequency set during boot. It can be reprogrammed during > + runtime using the common clock framework. > + - temperature-stability-7ppm: Indicate a device with a temperature stability > + of 7ppm Some DT binding bike-shedding: Should this be "temperature-stability-ppm = <7>;" ? Do you think that this value might change in the future? > diff --git a/drivers/clk/clk-si570.c b/drivers/clk/clk-si570.c > new file mode 100644 > index 0000000..c20dfce > --- /dev/null > +++ b/drivers/clk/clk-si570.c > @@ -0,0 +1,517 @@ <snip> > +static bool si570_regmap_is_volatile(struct device *dev, unsigned int reg) > +{ > + switch (reg) { > + case 135: > + return true; > + default: > + return false; > + } > +} > + > +static bool si570_regmap_is_writeable(struct device *dev, unsigned int reg) > +{ > + switch (reg) { > + case 7 ... 18: > + case 135: > + case 137: > + return true; > + default: > + return false; > + } > +} Should magic numbers above be symbolic constants? Regards, Mike
On Thu, Sep 19, 2013 at 11:05:12AM -0700, Mike Turquette wrote: > Quoting Soren Brinkmann (2013-09-18 15:43:38) > > diff --git a/Documentation/devicetree/bindings/clock/silabs,si570.txt b/Documentation/devicetree/bindings/clock/silabs,si570.txt > > new file mode 100644 > > index 0000000..7ab5c8b > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/clock/silabs,si570.txt > > @@ -0,0 +1,38 @@ > > +Binding for Silicon Labs 570, 571, 598 and 599 programmable > > +I2C clock generators. > > + > > +Reference > > +This binding uses the common clock binding[1]. Details about the devices can be > > +found in the data sheets[2][3]. > > + > > +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt > > +[2] Si570/571 Data Sheet > > + http://www.silabs.com/Support%20Documents/TechnicalDocs/si570.pdf > > +[3] Si598/599 Data Sheet > > + http://www.silabs.com/Support%20Documents/TechnicalDocs/si598-99.pdf > > + > > +Required properties: > > + - compatible: Shall be one of "silabs,si570", "silabs,si571", > > + "silabs,si598", "silabs,si599" > > + - reg: I2C device address. > > + - #clock-cells: From common clock bindings: Shall be 0. > > + - factory-fout: Factory set default frequency. This frequency is part specific. > > + The correct frequency for the part used has to be provided in > > + order to generate the correct output frequencies. For more > > + details, please refer to the data sheet. > > + > > +Optional properties: > > + - clock-output-names: From common clock bindings. Recommended to be "si570". > > + - clock-frequency: Output frequency to generate. This defines the output > > + frequency set during boot. It can be reprogrammed during > > + runtime using the common clock framework. > > + - temperature-stability-7ppm: Indicate a device with a temperature stability > > + of 7ppm > > Some DT binding bike-shedding: > > Should this be "temperature-stability-ppm = <7>;" ? Do you think that > this value might change in the future? Well, according to the data sheet all devices behave the same and only the si570/571 with a temperature stability of 7ppm (there are actually some more choices for this) need some special attention. So, the only case we really care about and has to be treated differently is the 7ppm case. I don't mind how we detect it, but with this boolean property it results in the least lines of code, I think. > > > diff --git a/drivers/clk/clk-si570.c b/drivers/clk/clk-si570.c > > new file mode 100644 > > index 0000000..c20dfce > > --- /dev/null > > +++ b/drivers/clk/clk-si570.c > > @@ -0,0 +1,517 @@ > <snip> > > +static bool si570_regmap_is_volatile(struct device *dev, unsigned int reg) > > +{ > > + switch (reg) { > > + case 135: > > + return true; > > + default: > > + return false; > > + } > > +} > > + > > +static bool si570_regmap_is_writeable(struct device *dev, unsigned int reg) > > +{ > > + switch (reg) { > > + case 7 ... 18: > > + case 135: > > + case 137: > > + return true; > > + default: > > + return false; > > + } > > +} > > Should magic numbers above be symbolic constants? I'll change it. Corresponding #defines already exist anyway. Sören
On Thu, Sep 19, 2013 at 09:44:38AM -0700, Guenter Roeck wrote: > On Thu, Sep 19, 2013 at 09:01:01AM -0700, Sören Brinkmann wrote: > > On Thu, Sep 19, 2013 at 06:17:12AM -0700, Guenter Roeck wrote: > > > On Wed, Sep 18, 2013 at 03:43:38PM -0700, Soren Brinkmann wrote: [...] > > > > + if (of_property_read_bool(client->dev.of_node, > > > > + "temperature-stability-7ppm")) > > > > + data->div_offset = SI570_DIV_OFFSET_7PPM; > > > > + > > > Just noticed that you dropped platform data support. Doesn't matter much for me > > > right now, but in my previous company we used the chip on an x86 system which > > > does not support devicetree. Would be nice to keep it and derive platform data > > > from devicetree data if provided, like other drivers do it. > > I'll look into this. The issue I have with that is, I can hardly test it > > since we only use this on Zynq which uses DT. So, I'd rather prefer to > > not include it unless somebody volunteers to test it. > > > Fair enough. I can not test it myself anymore, and my previous employer > now has a strict non-contributions-to-linux policy, so I guess they won't > test it either or at least not publish any test results. Leave it out. > > > > The 7ppm option is only relevant for si570/si751 and not supported on > > > si598/si599. You should mention that in the bindings document and check for it. > > Right, I'll add a note in the doc. And ignore it for devices this does > > not apply. > > > I would bail out, but that is your call. Correct me if I'm wrong, but in general drivers do not test for unsupported properties. So, in case we have one of the supported 59x device, we should not test whether a (unsupported) property is present, just to fail in that case, IMHO. [...] > > > > + if (!of_property_read_u32(client->dev.of_node, "clock-frequency", > > > > + &initial_fout)) { > > > > + err = clk_set_rate(clk, initial_fout); > > > > + if (err) > > > > + dev_warn(&client->dev, > > > > + "unable to set initial output frequency %u: %d\n", > > > > + initial_fout, err); > > > > > > No bailout ? > > > > > > Also it seems that this generates two error messages, once in the code which > > > experiences the error and once here. > > I remove the message here. > > > > > > > > Maybe it would be better to just bail out and return the error. > > > After all, something is seriously wrong and the system won't operate > > > as specified. > > I do more think of a spurious error (typo in DT prop giving an f out of > > range,...) and a driver actually controlling this clock generator later. > > In that case later calls to clk_set_rate() might succeed and everything's > > fine or the driver can handle the error. In case of using this device as > > a fixed clock, bailing out here might make more sense though. I'd prefer > > leaving it like this. > > > A typo in dt data isn't really a spurious error, and it would be easy to fix. > I would suggest to bail out; this ensures that the devicetree data is correct. Okay, bailing out it is. Sören
On Thu, Sep 19, 2013 at 01:59:38PM -0700, Sören Brinkmann wrote: > On Thu, Sep 19, 2013 at 09:44:38AM -0700, Guenter Roeck wrote: > > On Thu, Sep 19, 2013 at 09:01:01AM -0700, Sören Brinkmann wrote: > > > On Thu, Sep 19, 2013 at 06:17:12AM -0700, Guenter Roeck wrote: > > > > On Wed, Sep 18, 2013 at 03:43:38PM -0700, Soren Brinkmann wrote: > [...] > > > > > + if (of_property_read_bool(client->dev.of_node, > > > > > + "temperature-stability-7ppm")) > > > > > + data->div_offset = SI570_DIV_OFFSET_7PPM; > > > > > + > > > > Just noticed that you dropped platform data support. Doesn't matter much for me > > > > right now, but in my previous company we used the chip on an x86 system which > > > > does not support devicetree. Would be nice to keep it and derive platform data > > > > from devicetree data if provided, like other drivers do it. > > > I'll look into this. The issue I have with that is, I can hardly test it > > > since we only use this on Zynq which uses DT. So, I'd rather prefer to > > > not include it unless somebody volunteers to test it. > > > > > Fair enough. I can not test it myself anymore, and my previous employer > > now has a strict non-contributions-to-linux policy, so I guess they won't > > test it either or at least not publish any test results. Leave it out. > > > > > > The 7ppm option is only relevant for si570/si751 and not supported on > > > > si598/si599. You should mention that in the bindings document and check for it. > > > Right, I'll add a note in the doc. And ignore it for devices this does > > > not apply. > > > > > I would bail out, but that is your call. > Correct me if I'm wrong, but in general drivers do not test for > unsupported properties. So, in case we have one of the supported 59x > device, we should not test whether a (unsupported) property is present, > just to fail in that case, IMHO. > Ok with me if that is the accepted way of handling this condition. Guenter
On Thu, Sep 19, 2013 at 11:05:12AM -0700, Mike Turquette wrote: > Quoting Soren Brinkmann (2013-09-18 15:43:38) > > diff --git a/Documentation/devicetree/bindings/clock/silabs,si570.txt b/Documentation/devicetree/bindings/clock/silabs,si570.txt > > new file mode 100644 > > index 0000000..7ab5c8b > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/clock/silabs,si570.txt > > @@ -0,0 +1,38 @@ > > +Binding for Silicon Labs 570, 571, 598 and 599 programmable > > +I2C clock generators. > > + > > +Reference > > +This binding uses the common clock binding[1]. Details about the devices can be > > +found in the data sheets[2][3]. > > + > > +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt > > +[2] Si570/571 Data Sheet > > + http://www.silabs.com/Support%20Documents/TechnicalDocs/si570.pdf > > +[3] Si598/599 Data Sheet > > + http://www.silabs.com/Support%20Documents/TechnicalDocs/si598-99.pdf > > + > > +Required properties: > > + - compatible: Shall be one of "silabs,si570", "silabs,si571", > > + "silabs,si598", "silabs,si599" > > + - reg: I2C device address. > > + - #clock-cells: From common clock bindings: Shall be 0. > > + - factory-fout: Factory set default frequency. This frequency is part specific. > > + The correct frequency for the part used has to be provided in > > + order to generate the correct output frequencies. For more > > + details, please refer to the data sheet. > > + > > +Optional properties: > > + - clock-output-names: From common clock bindings. Recommended to be "si570". > > + - clock-frequency: Output frequency to generate. This defines the output > > + frequency set during boot. It can be reprogrammed during > > + runtime using the common clock framework. > > + - temperature-stability-7ppm: Indicate a device with a temperature stability > > + of 7ppm > > Some DT binding bike-shedding: > > Should this be "temperature-stability-ppm = <7>;" ? Do you think that > this value might change in the future? > Valid values are 7, 20, and 50 as far as I know. Problem is that the value is not used directly, but only to hint that a specific set of registers shall be used. Given that, it may in fact be better to use an explicit number. Even though the two register sets are specified by <7> in one case and <20,50> in the other today, there may at some point be yet another value which might use the 7 ppm register set or the 20/50 ppm register set ... or yet another register set. An explicit number would cover all future accuracy ranges, not just the existing ones. Guenter
On Thu, Sep 19, 2013 at 02:15:35PM -0700, Guenter Roeck wrote: > On Thu, Sep 19, 2013 at 11:05:12AM -0700, Mike Turquette wrote: > > Quoting Soren Brinkmann (2013-09-18 15:43:38) > > > diff --git a/Documentation/devicetree/bindings/clock/silabs,si570.txt b/Documentation/devicetree/bindings/clock/silabs,si570.txt > > > new file mode 100644 > > > index 0000000..7ab5c8b > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/clock/silabs,si570.txt > > > @@ -0,0 +1,38 @@ > > > +Binding for Silicon Labs 570, 571, 598 and 599 programmable > > > +I2C clock generators. > > > + > > > +Reference > > > +This binding uses the common clock binding[1]. Details about the devices can be > > > +found in the data sheets[2][3]. > > > + > > > +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt > > > +[2] Si570/571 Data Sheet > > > + http://www.silabs.com/Support%20Documents/TechnicalDocs/si570.pdf > > > +[3] Si598/599 Data Sheet > > > + http://www.silabs.com/Support%20Documents/TechnicalDocs/si598-99.pdf > > > + > > > +Required properties: > > > + - compatible: Shall be one of "silabs,si570", "silabs,si571", > > > + "silabs,si598", "silabs,si599" > > > + - reg: I2C device address. > > > + - #clock-cells: From common clock bindings: Shall be 0. > > > + - factory-fout: Factory set default frequency. This frequency is part specific. > > > + The correct frequency for the part used has to be provided in > > > + order to generate the correct output frequencies. For more > > > + details, please refer to the data sheet. > > > + > > > +Optional properties: > > > + - clock-output-names: From common clock bindings. Recommended to be "si570". > > > + - clock-frequency: Output frequency to generate. This defines the output > > > + frequency set during boot. It can be reprogrammed during > > > + runtime using the common clock framework. > > > + - temperature-stability-7ppm: Indicate a device with a temperature stability > > > + of 7ppm > > > > Some DT binding bike-shedding: > > > > Should this be "temperature-stability-ppm = <7>;" ? Do you think that > > this value might change in the future? > > > Valid values are 7, 20, and 50 as far as I know. Problem is that the value is > not used directly, but only to hint that a specific set of registers shall be > used. > > Given that, it may in fact be better to use an explicit number. Even though the > two register sets are specified by <7> in one case and <20,50> in the other > today, there may at some point be yet another value which might use the 7 ppm > register set or the 20/50 ppm register set ... or yet another register set. > An explicit number would cover all future accuracy ranges, not just the > existing ones. Okay, I'm convinced. Changing that, I'd also make it a mandatory property. Sören
diff --git a/Documentation/devicetree/bindings/clock/silabs,si570.txt b/Documentation/devicetree/bindings/clock/silabs,si570.txt new file mode 100644 index 0000000..7ab5c8b --- /dev/null +++ b/Documentation/devicetree/bindings/clock/silabs,si570.txt @@ -0,0 +1,38 @@ +Binding for Silicon Labs 570, 571, 598 and 599 programmable +I2C clock generators. + +Reference +This binding uses the common clock binding[1]. Details about the devices can be +found in the data sheets[2][3]. + +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt +[2] Si570/571 Data Sheet + http://www.silabs.com/Support%20Documents/TechnicalDocs/si570.pdf +[3] Si598/599 Data Sheet + http://www.silabs.com/Support%20Documents/TechnicalDocs/si598-99.pdf + +Required properties: + - compatible: Shall be one of "silabs,si570", "silabs,si571", + "silabs,si598", "silabs,si599" + - reg: I2C device address. + - #clock-cells: From common clock bindings: Shall be 0. + - factory-fout: Factory set default frequency. This frequency is part specific. + The correct frequency for the part used has to be provided in + order to generate the correct output frequencies. For more + details, please refer to the data sheet. + +Optional properties: + - clock-output-names: From common clock bindings. Recommended to be "si570". + - clock-frequency: Output frequency to generate. This defines the output + frequency set during boot. It can be reprogrammed during + runtime using the common clock framework. + - temperature-stability-7ppm: Indicate a device with a temperature stability + of 7ppm + +Example: + si570: clock-generator@5d { + #clock-cells = <0>; + compatible = "silabs,si570"; + reg = <0x5d>; + factory-fout = <156250000>; + }; diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index 279407a..349b88a 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -64,6 +64,16 @@ config COMMON_CLK_SI5351 This driver supports Silicon Labs 5351A/B/C programmable clock generators. +config COMMON_CLK_SI570 + tristate "Clock driver for SiLabs 570 and compatible devices" + depends on I2C + depends on OF + select REGMAP_I2C + help + ---help--- + This driver supports Silicon Labs 570/571/598/599 programmable + clock generators. + config COMMON_CLK_S2MPS11 tristate "Clock driver for S2MPS11 MFD" depends on MFD_SEC_CORE diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index 7b11106..c0e94b3 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -40,6 +40,7 @@ obj-$(CONFIG_COMMON_CLK_AXI_CLKGEN) += clk-axi-clkgen.o obj-$(CONFIG_COMMON_CLK_WM831X) += clk-wm831x.o obj-$(CONFIG_COMMON_CLK_MAX77686) += clk-max77686.o obj-$(CONFIG_COMMON_CLK_SI5351) += clk-si5351.o +obj-$(CONFIG_COMMON_CLK_SI570) += clk-si570.o obj-$(CONFIG_COMMON_CLK_S2MPS11) += clk-s2mps11.o obj-$(CONFIG_CLK_TWL6040) += clk-twl6040.o obj-$(CONFIG_CLK_PPC_CORENET) += clk-ppc-corenet.o diff --git a/drivers/clk/clk-si570.c b/drivers/clk/clk-si570.c new file mode 100644 index 0000000..c20dfce --- /dev/null +++ b/drivers/clk/clk-si570.c @@ -0,0 +1,517 @@ +/* + * Driver for Silicon Labs Si570/Si571 Programmable XO/VCXO + * + * Copyright (C) 2010, 2011 Ericsson AB. + * Copyright (C) 2011 Guenter Roeck. + * Copyright (C) 2011 - 2013 Xilinx Inc. + * + * Author: Guenter Roeck <guenter.roeck@ericsson.com> + * Sören Brinkmann <soren.brinkmann@xilinx.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/clk-provider.h> +#include <linux/delay.h> +#include <linux/module.h> +#include <linux/i2c.h> +#include <linux/regmap.h> +#include <linux/slab.h> + +/* Si570 registers */ +#define SI570_REG_HS_N1 7 +#define SI570_REG_N1_RFREQ0 8 +#define SI570_REG_RFREQ1 9 +#define SI570_REG_RFREQ2 10 +#define SI570_REG_RFREQ3 11 +#define SI570_REG_RFREQ4 12 +#define SI570_REG_CONTROL 135 +#define SI570_REG_FREEZE_DCO 137 +#define SI570_DIV_OFFSET_7PPM 6 + +#define HS_DIV_SHIFT 5 +#define HS_DIV_MASK 0xe0 +#define HS_DIV_OFFSET 4 +#define N1_6_2_MASK 0x1f +#define N1_1_0_MASK 0xc0 +#define RFREQ_37_32_MASK 0x3f + +#define SI570_MIN_FREQ 10000000L +#define SI570_MAX_FREQ 1417500000L +#define SI598_MAX_FREQ 525000000L + +#define FDCO_MIN 4850000000LL +#define FDCO_MAX 5670000000LL + +#define SI570_CNTRL_RECALL (1 << 0) +#define SI570_CNTRL_FREEZE_M (1 << 5) +#define SI570_CNTRL_NEWFREQ (1 << 6) + +#define SI570_FREEZE_DCO (1 << 4) + +/** + * struct clk_si570: + * @hw: Clock hw struct + * @regmap: Device's regmap + * @div_offset: Rgister offset for dividers + * @max_freq: Maximum frequency for this device + * @fxtal: Factory xtal frequency + * @n1: Clock divider N1 + * @hs_div: Clock divider HSDIV + * @rfreq: Clock multiplier RFREQ + * @frequency: Current output frequency + * @i2c_client: I2C client pointer + */ +struct clk_si570 { + struct clk_hw hw; + struct regmap *regmap; + unsigned int div_offset; + u64 max_freq; + u64 fxtal; + unsigned int n1; + unsigned int hs_div; + u64 rfreq; + u64 frequency; + struct i2c_client *i2c_client; +}; +#define to_clk_si570(_hw) container_of(_hw, struct clk_si570, hw) + +/** + * si570_get_divs() - Read clock dividers from HW + * @data: Pointer to struct clk_si570 + * @rfreq: Fractional multiplier (output) + * @n1: Divider N1 (output) + * @hs_div: Divider HSDIV (output) + * Returns 0 on success, negative errno otherwise. + * + * Retrieve clock dividers and multipliers from the HW. + */ +static int si570_get_divs(struct clk_si570 *data, u64 *rfreq, + unsigned int *n1, unsigned int *hs_div) +{ + int err; + u8 reg[6]; + u64 tmp; + + err = regmap_bulk_read(data->regmap, SI570_REG_HS_N1 + data->div_offset, + reg, ARRAY_SIZE(reg)); + if (err) + return err; + + *hs_div = ((reg[0] & HS_DIV_MASK) >> HS_DIV_SHIFT) + HS_DIV_OFFSET; + *n1 = ((reg[0] & N1_6_2_MASK) << 2) + ((reg[1] & N1_1_0_MASK) >> 6) + 1; + /* Handle invalid cases */ + if (*n1 > 1) + *n1 &= ~1; + + tmp = reg[1] & RFREQ_37_32_MASK; + tmp = (tmp << 8) + reg[2]; + tmp = (tmp << 8) + reg[3]; + tmp = (tmp << 8) + reg[4]; + tmp = (tmp << 8) + reg[5]; + *rfreq = tmp; + + return 0; +} + +/** + * si570_get_defaults() - Get default values + * @data: Driver data structure + * @fout: Factory frequency output + * Returns 0 on success, negative errno otherwise. + */ +static int si570_get_defaults(struct clk_si570 *data, u64 fout) +{ + int err; + u64 fdco; + + regmap_write(data->regmap, SI570_REG_CONTROL, SI570_CNTRL_RECALL); + + err = si570_get_divs(data, &data->rfreq, &data->n1, &data->hs_div); + if (err) + return err; + + /* + * Accept optional precision loss to avoid arithmetic overflows. + * Acceptable per Silicon Labs Application Note AN334. + */ + fdco = fout * data->n1 * data->hs_div; + if (fdco >= (1LL << 36)) + data->fxtal = div64_u64(fdco << 24, data->rfreq >> 4); + else + data->fxtal = div64_u64(fdco << 28, data->rfreq); + + data->frequency = fout; + + return 0; +} + +/** + * si570_update_rfreq() - Update clock multiplier + * @data: Driver data structure + * Passes on regmap_bulk_write() return value. + */ +static int si570_update_rfreq(struct clk_si570 *data) +{ + u8 reg[5]; + + reg[0] = ((data->n1 - 1) << 6) | + ((data->rfreq >> 32) & RFREQ_37_32_MASK); + reg[1] = (data->rfreq >> 24) & 0xff; + reg[2] = (data->rfreq >> 16) & 0xff; + reg[3] = (data->rfreq >> 8) & 0xff; + reg[4] = data->rfreq & 0xff; + + return regmap_bulk_write(data->regmap, SI570_REG_N1_RFREQ0 + + data->div_offset, reg, ARRAY_SIZE(reg)); +} + +/** + * si570_calc_divs() - Caluclate clock dividers + * @frequency: Target frequency + * @data: Driver data structure + * @out_rfreq: RFREG fractional multiplier (output) + * @out_n1: Clock divider N1 (output) + * @out_hs_div: Clock divider HSDIV (output) + * Returns 0 on success, negative errno otherwise. + * + * Calculate the clock dividers (@out_hs_div, @out_n1) and clock multiplier + * (@out_rfreq) for a given target @frequency. + */ +static int si570_calc_divs(unsigned long frequency, struct clk_si570 *data, + u64 *out_rfreq, unsigned int *out_n1, unsigned int *out_hs_div) +{ + int i; + unsigned int n1, hs_div; + u64 fdco, best_fdco = ULLONG_MAX; + static const uint8_t si570_hs_div_values[] = { 11, 9, 7, 6, 5, 4 }; + + for (i = 0; i < ARRAY_SIZE(si570_hs_div_values); i++) { + hs_div = si570_hs_div_values[i]; + /* Calculate lowest possible value for n1 */ + n1 = div_u64(div_u64(FDCO_MIN, hs_div), frequency); + if (!n1 || (n1 & 1)) + n1++; + while (n1 <= 128) { + fdco = (u64)frequency * (u64)hs_div * (u64)n1; + if (fdco > FDCO_MAX) + break; + if (fdco >= FDCO_MIN && fdco < best_fdco) { + *out_n1 = n1; + *out_hs_div = hs_div; + *out_rfreq = div64_u64(fdco << 28, data->fxtal); + best_fdco = fdco; + } + n1 += (n1 == 1 ? 1 : 2); + } + } + + if (best_fdco == ULLONG_MAX) + return -EINVAL; + + return 0; +} + +static unsigned long si570_recalc_rate(struct clk_hw *hw, + unsigned long parent_rate) +{ + int err; + u64 rfreq, rate; + unsigned int n1, hs_div; + struct clk_si570 *data = to_clk_si570(hw); + + err = si570_get_divs(data, &rfreq, &n1, &hs_div); + if (err) { + dev_err(&data->i2c_client->dev, "unable to recalc rate\n"); + return data->frequency; + } + + rfreq = div_u64(rfreq, hs_div * n1); + rate = (data->fxtal * rfreq) >> 28; + + return rate; +} + +static long si570_round_rate(struct clk_hw *hw, unsigned long rate, + unsigned long *parent_rate) +{ + int err; + u64 rfreq; + unsigned int n1, hs_div; + struct clk_si570 *data = to_clk_si570(hw); + + if (!rate) + return 0; + + if (div64_u64(abs(rate - data->frequency) * 10000LL, + data->frequency) < 35) { + rfreq = div64_u64((data->rfreq * rate) + + div64_u64(data->frequency, 2), data->frequency); + n1 = data->n1; + hs_div = data->hs_div; + + } else { + err = si570_calc_divs(rate, data, &rfreq, &n1, &hs_div); + if (err) { + dev_err(&data->i2c_client->dev, + "unable to round rate\n"); + return 0; + } + } + + return rate; +} + +/** + * si570_set_frequency() - Adjust output frequency + * @data: Driver data structure + * @frequency: Target frequency + * Returns 0 on success. + * + * Update output frequency for big frequency changes (> 3,500 ppm). + */ +static int si570_set_frequency(struct clk_si570 *data, unsigned long frequency) +{ + int err; + + err = si570_calc_divs(frequency, data, &data->rfreq, &data->n1, + &data->hs_div); + if (err) + return err; + + /* + * The DCO reg should be accessed with a read-modify-write operation + * per AN334 + */ + regmap_write(data->regmap, SI570_REG_FREEZE_DCO, SI570_FREEZE_DCO); + regmap_write(data->regmap, SI570_REG_HS_N1 + data->div_offset, + ((data->hs_div - HS_DIV_OFFSET) << HS_DIV_SHIFT) | + (((data->n1 - 1) >> 2) & N1_6_2_MASK)); + si570_update_rfreq(data); + regmap_write(data->regmap, SI570_REG_FREEZE_DCO, 0); + regmap_write(data->regmap, SI570_REG_CONTROL, SI570_CNTRL_NEWFREQ); + + /* Applying a new frequency can take up to 10ms */ + usleep_range(10000, 10000); + + return 0; +} + +/** + * si570_set_frequency_small() - Adjust output frequency + * @data: Driver data structure + * @frequency: Target frequency + * Returns 0 on success. + * + * Update output frequency for small frequency changes (< 3,500 ppm). + */ +static int si570_set_frequency_small(struct clk_si570 *data, + unsigned long frequency) +{ + /* + * This is a re-implementation of DIV_ROUND_CLOSEST + * using the div64_u64 function lieu of letting the compiler + * insert EABI calls + */ + data->rfreq = div64_u64((data->rfreq * frequency) + + div_u64(data->frequency, 2), data->frequency); + regmap_write(data->regmap, SI570_REG_CONTROL, SI570_CNTRL_FREEZE_M); + si570_update_rfreq(data); + regmap_write(data->regmap, SI570_REG_CONTROL, 0); + + return 0; +} + +static int si570_set_rate(struct clk_hw *hw, unsigned long rate, + unsigned long parent_rate) +{ + struct clk_si570 *data = to_clk_si570(hw); + struct i2c_client *client = data->i2c_client; + int err; + + if (rate < SI570_MIN_FREQ || rate > data->max_freq) { + dev_warn(&client->dev, + "requested frequency %lu Hz is out of range\n", rate); + return -EINVAL; + } + + if (div64_u64(abs(rate - data->frequency) * 10000LL, + data->frequency) < 35) + err = si570_set_frequency_small(data, rate); + else + err = si570_set_frequency(data, rate); + + if (err) + return err; + + data->frequency = rate; + + return 0; +} + +static const struct clk_ops si570_clk_ops = { + .recalc_rate = si570_recalc_rate, + .round_rate = si570_round_rate, + .set_rate = si570_set_rate, +}; + +static bool si570_regmap_is_volatile(struct device *dev, unsigned int reg) +{ + switch (reg) { + case 135: + return true; + default: + return false; + } +} + +static bool si570_regmap_is_writeable(struct device *dev, unsigned int reg) +{ + switch (reg) { + case 7 ... 18: + case 135: + case 137: + return true; + default: + return false; + } +} + +static struct regmap_config si570_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + .cache_type = REGCACHE_RBTREE, + .max_register = 137, + .writeable_reg = si570_regmap_is_writeable, + .volatile_reg = si570_regmap_is_volatile, +}; + +static int si570_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + struct clk_si570 *data; + struct clk_init_data init; + struct clk *clk; + u32 initial_fout, factory_fout; + int err; + + data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL); + if (!data) + return -ENOMEM; + + init.ops = &si570_clk_ops; + init.flags = CLK_IS_ROOT; + init.num_parents = 0; + data->hw.init = &init; + data->max_freq = id->driver_data; + data->i2c_client = client; + + if (of_property_read_bool(client->dev.of_node, + "temperature-stability-7ppm")) + data->div_offset = SI570_DIV_OFFSET_7PPM; + + if (of_property_read_string(client->dev.of_node, "clock-output-names", + &init.name)) + init.name = client->dev.of_node->name; + + err = of_property_read_u32(client->dev.of_node, "factory-fout", + &factory_fout); + if (err) { + dev_err(&client->dev, "'factory-fout' property missing\n"); + return err; + } + + data->regmap = devm_regmap_init_i2c(client, &si570_regmap_config); + if (IS_ERR(data->regmap)) { + dev_err(&client->dev, "failed to allocate register map\n"); + return PTR_ERR(data->regmap); + } + + i2c_set_clientdata(client, data); + err = si570_get_defaults(data, factory_fout); + if (err) + return err; + + clk = devm_clk_register(&client->dev, &data->hw); + if (IS_ERR(clk)) { + dev_err(&client->dev, "clock registration failed\n"); + return PTR_ERR(clk); + } + err = of_clk_add_provider(client->dev.of_node, of_clk_src_simple_get, + clk); + if (err) { + dev_err(&client->dev, "unable to add clk provider\n"); + return err; + } + + /* + * Read the requested initial fout from either platform data or the + * device tree + */ + if (!of_property_read_u32(client->dev.of_node, "clock-frequency", + &initial_fout)) { + err = clk_set_rate(clk, initial_fout); + if (err) + dev_warn(&client->dev, + "unable to set initial output frequency %u: %d\n", + initial_fout, err); + } + + /* Display a message indicating that we've successfully registered */ + dev_info(&client->dev, "registered, current frequency %llu Hz\n", + data->frequency); + + return 0; +} + +static int si570_remove(struct i2c_client *client) +{ + struct clk_si570 *data = i2c_get_clientdata(client); + + of_clk_del_provider(client->dev.of_node); + clk_unregister(data->hw.clk); + + return 0; +} + +static const struct i2c_device_id si570_id[] = { + { "si570", SI570_MAX_FREQ }, + { "si571", SI570_MAX_FREQ }, + { "si598", SI598_MAX_FREQ }, + { "si599", SI598_MAX_FREQ }, + { } +}; +MODULE_DEVICE_TABLE(i2c, si570_id); + +static const struct of_device_id clk_si570_of_match[] = { + { .compatible = "silabs,si570" }, + { .compatible = "silabs,si571" }, + { .compatible = "silabs,si598" }, + { .compatible = "silabs,si599" }, + { }, +}; +MODULE_DEVICE_TABLE(of, clk_si570_of_match); + +static struct i2c_driver si570_driver = { + .driver = { + .name = "si570", + .of_match_table = of_match_ptr(clk_si570_of_match), + }, + .probe = si570_probe, + .remove = si570_remove, + .id_table = si570_id, +}; +module_i2c_driver(si570_driver); + +MODULE_AUTHOR("Guenter Roeck <guenter.roeck@ericsson.com>"); +MODULE_AUTHOR("Soeren Brinkmann <soren.brinkmann@xilinx.com"); +MODULE_DESCRIPTION("Si570 driver"); +MODULE_LICENSE("GPL");
Add a driver for SILabs 570, 571, 598, 599 programmable oscillators. The devices generate low-jitter clock signals and are reprogrammable via an I2C interface. Cc: Guenter Roeck <linux@roeck-us.net> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> --- v2: - document clock-output-names in bindings documentation - don't use wildcards in compatibility string - change Kconfig entry to "... 570 and compatible devices" - change some indentation flaws - use 10000 as MIN and MAX value in usleep_range - fail probe() if 'factory-fout' is not provided in DT - default factory fout #defines removed - use i2c driver_data instead of OF data - remove some related structs and data - rename DT property 'initial-fout' => 'clock-frequency' (like si5351 driver) - add some more details regarding the 'factory-fout' DT property .../devicetree/bindings/clock/silabs,si570.txt | 38 ++ drivers/clk/Kconfig | 10 + drivers/clk/Makefile | 1 + drivers/clk/clk-si570.c | 517 +++++++++++++++++++++ 4 files changed, 566 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/silabs,si570.txt create mode 100644 drivers/clk/clk-si570.c