Re: [Patch v2] driver/clk/clk-si5338: Add common clock framework driver for si5338
On 06/16/2015 08:18 AM, York Sun wrote: > Paul, > > Thanks for reviewing. > > On 06/16/2015 01:21 AM, Paul Bolle wrote: >> One question and a few nits follow. >> >> On Mon, 2015-06-15 at 10:07 -0700, York Sun wrote: >>> SI5338 is a programmable clock generator. It has 4 sets of inputs, >>> PLL, multisynth and dividers to make 4 outputs. This driver splits >>> them into multiple clocks to comply with common clock framework. >>> >>> See Documentation/devicetree/bindings/clock/silabs,si5338.txt for >>> details. >>> >>> Signed-off-by: York Sun >>> CC: Mike Turquette >> >> Apparently that's now mturque...@baylibre.com . > > Thanks. Will change. > >> >>> CC: Sebastian Hesselbarth >>> CC: Guenter Roeck >>> CC: Andrey Filippov >> >>> --- a/drivers/clk/Kconfig >>> +++ b/drivers/clk/Kconfig >> >>> config COMMON_CLK >>> - bool >>> + tristate "Common Clock" >>> select HAVE_CLK_PREPARE >>> select CLKDEV_LOOKUP >>> select SRCU >> >> Why? The commit explanation doesn't mention this. Did you use an unclean >> tree? If not, you just created over a dozen of new modules: > > Thanks for catching this. I was testing building the driver within and outside > of kernel tree for another kernel version. If this driver is built-in, I don't > need to make it tristate. Will revert in next version. > Now I remember why I did this. COMMON_CLK wasn't an option users can select, because it is a bool and only selected by some platforms. I think it should be a tristate so one can build a driver with it. When it is selected, some drivers are built, either into kernel or as modules, up to user's choice. They are needed by common clock framework. I should add explanation in commit message. Or separate it into an individual patch. Which one is preferred? York -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch v2] driver/clk/clk-si5338: Add common clock framework driver for si5338
Paul,
Thanks for reviewing.
On 06/16/2015 01:21 AM, Paul Bolle wrote:
> One question and a few nits follow.
>
> On Mon, 2015-06-15 at 10:07 -0700, York Sun wrote:
>> SI5338 is a programmable clock generator. It has 4 sets of inputs,
>> PLL, multisynth and dividers to make 4 outputs. This driver splits
>> them into multiple clocks to comply with common clock framework.
>>
>> See Documentation/devicetree/bindings/clock/silabs,si5338.txt for
>> details.
>>
>> Signed-off-by: York Sun
>> CC: Mike Turquette
>
> Apparently that's now mturque...@baylibre.com .
Thanks. Will change.
>
>> CC: Sebastian Hesselbarth
>> CC: Guenter Roeck
>> CC: Andrey Filippov
>
>> --- a/drivers/clk/Kconfig
>> +++ b/drivers/clk/Kconfig
>
>> config COMMON_CLK
>> -bool
>> +tristate "Common Clock"
>> select HAVE_CLK_PREPARE
>> select CLKDEV_LOOKUP
>> select SRCU
>
> Why? The commit explanation doesn't mention this. Did you use an unclean
> tree? If not, you just created over a dozen of new modules:
Thanks for catching this. I was testing building the driver within and outside
of kernel tree for another kernel version. If this driver is built-in, I don't
need to make it tristate. Will revert in next version.
> $ git grep -nw CONFIG_COMMON_CLK -- "*Makefile*"
> arch/arm/mach-mmp/Makefile:13:ifeq ($(CONFIG_COMMON_CLK), )
> arch/arm/mach-shmobile/Makefile:21:ifndef CONFIG_COMMON_CLK
> arch/powerpc/platforms/512x/Makefile:4:obj-$(CONFIG_COMMON_CLK) +=
> clock-commonclk.o
> drivers/clk/Makefile:4:obj-$(CONFIG_COMMON_CLK) += clk.o
> drivers/clk/Makefile:5:obj-$(CONFIG_COMMON_CLK) += clk-divider.o
> drivers/clk/Makefile:6:obj-$(CONFIG_COMMON_CLK) += clk-fixed-factor.o
> drivers/clk/Makefile:7:obj-$(CONFIG_COMMON_CLK) += clk-fixed-rate.o
> drivers/clk/Makefile:8:obj-$(CONFIG_COMMON_CLK) += clk-gate.o
> drivers/clk/Makefile:9:obj-$(CONFIG_COMMON_CLK) += clk-mux.o
> drivers/clk/Makefile:10:obj-$(CONFIG_COMMON_CLK)+= clk-composite.o
> drivers/clk/Makefile:11:obj-$(CONFIG_COMMON_CLK)+=
> clk-fractional-divider.o
> drivers/clk/Makefile:12:obj-$(CONFIG_COMMON_CLK)+= clk-gpio-gate.o
> drivers/clk/Makefile:14:obj-$(CONFIG_COMMON_CLK)+= clk-conf.o
> drivers/clk/Makefile:59:ifeq ($(CONFIG_COMMON_CLK), y)
> drivers/clk/samsung/Makefile:5:obj-$(CONFIG_COMMON_CLK) += clk.o clk-pll.o
> drivers/gpu/drm/msm/Makefile:53:msm-$(CONFIG_COMMON_CLK) +=
> mdp/mdp4/mdp4_lvds_pll.o
> drivers/sh/Makefile:5:ifneq ($(CONFIG_COMMON_CLK),y)
>
>> +config COMMON_CLK_SI5338
>> +tristate "Clock driver for SiLabs 5338"
>> +depends on I2C
>> +select REGMAP_I2C
>> +select RATIONAL
>> +---help---
>> + This driver supports Silicon Labs 5338 programmable clock generators,
>> + using common clock framework. It needs parent clock as input(s).
>> + Internal clocks are registered with unique names in case multiple
>> + devices exist. See devicetree/bindings/clock/silabs,si5338.txt
>> + under Documentation for details.
>
>> --- /dev/null
>> +++ b/drivers/clk/clk-si5338.c
>
>> +unsigned long si5338_divrefclk_recalc_rate(struct clk_hw *hw,
>> + unsigned long parent_rate)
>> +{
>> +[...]
>> +}
>
> Can't this be made static? It compiles cleanly with static too. Is there
> some subtle issue I'm missing?
>
Absolutely. I must have missed them for some functions.
>> +static const struct clk_ops si5338_divrefclk_ops = {
>> +.recalc_rate = si5338_divrefclk_recalc_rate,
>> +};
>
>> +unsigned long si5338_divfbclk_recalc_rate(struct clk_hw *hw,
>> + unsigned long parent_rate)
>> +{
>> +[...]
>> +}
>
> Ditto.
>
>> +static const struct clk_ops si5338_divfbclk_ops = {
>> +.recalc_rate = si5338_divfbclk_recalc_rate,
>> +};
>
>> --- /dev/null
>> +++ b/include/dt-bindings/clock/clk-si5338.h
>
>> +#ifndef _DT_BINDINGS_CLK_DSI5338_H
>> +#define _DT_BINDINGS_CLK_DSI5338_H
>
> (I spotted a D that looks odd here.)
Me, too. It takes fresh eyes to spot this non-sense error.
>
> And git am whines:
> new blank line at EOF.
Thanks.
York
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Patch v2] driver/clk/clk-si5338: Add common clock framework driver for si5338
One question and a few nits follow.
On Mon, 2015-06-15 at 10:07 -0700, York Sun wrote:
> SI5338 is a programmable clock generator. It has 4 sets of inputs,
> PLL, multisynth and dividers to make 4 outputs. This driver splits
> them into multiple clocks to comply with common clock framework.
>
> See Documentation/devicetree/bindings/clock/silabs,si5338.txt for
> details.
>
> Signed-off-by: York Sun
> CC: Mike Turquette
Apparently that's now mturque...@baylibre.com .
> CC: Sebastian Hesselbarth
> CC: Guenter Roeck
> CC: Andrey Filippov
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> config COMMON_CLK
> - bool
> + tristate "Common Clock"
> select HAVE_CLK_PREPARE
> select CLKDEV_LOOKUP
> select SRCU
Why? The commit explanation doesn't mention this. Did you use an unclean
tree? If not, you just created over a dozen of new modules:
$ git grep -nw CONFIG_COMMON_CLK -- "*Makefile*"
arch/arm/mach-mmp/Makefile:13:ifeq ($(CONFIG_COMMON_CLK), )
arch/arm/mach-shmobile/Makefile:21:ifndef CONFIG_COMMON_CLK
arch/powerpc/platforms/512x/Makefile:4:obj-$(CONFIG_COMMON_CLK) +=
clock-commonclk.o
drivers/clk/Makefile:4:obj-$(CONFIG_COMMON_CLK) += clk.o
drivers/clk/Makefile:5:obj-$(CONFIG_COMMON_CLK) += clk-divider.o
drivers/clk/Makefile:6:obj-$(CONFIG_COMMON_CLK) += clk-fixed-factor.o
drivers/clk/Makefile:7:obj-$(CONFIG_COMMON_CLK) += clk-fixed-rate.o
drivers/clk/Makefile:8:obj-$(CONFIG_COMMON_CLK) += clk-gate.o
drivers/clk/Makefile:9:obj-$(CONFIG_COMMON_CLK) += clk-mux.o
drivers/clk/Makefile:10:obj-$(CONFIG_COMMON_CLK)+= clk-composite.o
drivers/clk/Makefile:11:obj-$(CONFIG_COMMON_CLK)+=
clk-fractional-divider.o
drivers/clk/Makefile:12:obj-$(CONFIG_COMMON_CLK)+= clk-gpio-gate.o
drivers/clk/Makefile:14:obj-$(CONFIG_COMMON_CLK)+= clk-conf.o
drivers/clk/Makefile:59:ifeq ($(CONFIG_COMMON_CLK), y)
drivers/clk/samsung/Makefile:5:obj-$(CONFIG_COMMON_CLK) += clk.o clk-pll.o
drivers/gpu/drm/msm/Makefile:53:msm-$(CONFIG_COMMON_CLK) +=
mdp/mdp4/mdp4_lvds_pll.o
drivers/sh/Makefile:5:ifneq ($(CONFIG_COMMON_CLK),y)
> +config COMMON_CLK_SI5338
> + tristate "Clock driver for SiLabs 5338"
> + depends on I2C
> + select REGMAP_I2C
> + select RATIONAL
> + ---help---
> + This driver supports Silicon Labs 5338 programmable clock generators,
> + using common clock framework. It needs parent clock as input(s).
> + Internal clocks are registered with unique names in case multiple
> + devices exist. See devicetree/bindings/clock/silabs,si5338.txt
> + under Documentation for details.
> --- /dev/null
> +++ b/drivers/clk/clk-si5338.c
> +unsigned long si5338_divrefclk_recalc_rate(struct clk_hw *hw,
> +unsigned long parent_rate)
> +{
> + [...]
> +}
Can't this be made static? It compiles cleanly with static too. Is there
some subtle issue I'm missing?
> +static const struct clk_ops si5338_divrefclk_ops = {
> + .recalc_rate = si5338_divrefclk_recalc_rate,
> +};
> +unsigned long si5338_divfbclk_recalc_rate(struct clk_hw *hw,
> +unsigned long parent_rate)
> +{
> + [...]
> +}
Ditto.
> +static const struct clk_ops si5338_divfbclk_ops = {
> + .recalc_rate = si5338_divfbclk_recalc_rate,
> +};
> --- /dev/null
> +++ b/include/dt-bindings/clock/clk-si5338.h
> +#ifndef _DT_BINDINGS_CLK_DSI5338_H
> +#define _DT_BINDINGS_CLK_DSI5338_H
(I spotted a D that looks odd here.)
And git am whines:
new blank line at EOF.
Thanks,
Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[Patch v2] driver/clk/clk-si5338: Add common clock framework driver for si5338
SI5338 is a programmable clock generator. It has 4 sets of inputs, PLL, multisynth and dividers to make 4 outputs. This driver splits them into multiple clocks to comply with common clock framework. See Documentation/devicetree/bindings/clock/silabs,si5338.txt for details. Signed-off-by: York Sun CC: Mike Turquette CC: Sebastian Hesselbarth CC: Guenter Roeck CC: Andrey Filippov --- Change log: v2: Fix handling name prefix if the driver is unloaded and loaded again .../devicetree/bindings/clock/silabs,si5338.txt| 173 + drivers/clk/Kconfig| 14 +- drivers/clk/Makefile |1 + drivers/clk/clk-si5338.c | 3656 drivers/clk/clk-si5338.h | 305 ++ include/dt-bindings/clock/clk-si5338.h | 69 + include/linux/platform_data/si5338.h | 49 + 7 files changed, 4266 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/clock/silabs,si5338.txt create mode 100644 drivers/clk/clk-si5338.c create mode 100644 drivers/clk/clk-si5338.h create mode 100644 include/dt-bindings/clock/clk-si5338.h create mode 100644 include/linux/platform_data/si5338.h diff --git a/Documentation/devicetree/bindings/clock/silabs,si5338.txt b/Documentation/devicetree/bindings/clock/silabs,si5338.txt new file mode 100644 index 000..aeeb81d --- /dev/null +++ b/Documentation/devicetree/bindings/clock/silabs,si5338.txt @@ -0,0 +1,173 @@ +Binding for Silicon Labs Si5338 programmable i2c clock generator. + +Reference +[1] Si5338 Data Sheet +http://www.silabs.com/Support%20Documents/TechnicalDocs/Si5338.pdf + +The Si5338 is a programmable i2c clock generators with up to 4 output +clocks. It has 4 sets of possible input clocks + +IN1/IN2: differential +IN3: single-ended +IN4: single-ended +IN5/IN6: differential + +Additionally, IN1/IN2 can be used as XTAL with different setting. +The clock tree looks like below (without support of zero-delay) + + + IN1/IN2 IN3 IN4 IN5/IN6 + | | | | + --| | | | + | | | | | + | \ / \ / + | \ / \ / + | \ / \ / + XTAL REFCLKFBCLK + | | \ / | + | | \ /| + | | DIVREFCLK DIVFBCLK | + | | \ / | + | | \ / | + | | \ /| + | |PLL | + | | / | | \ | + | | / / \ \ | + | |/ / \ \ | + | | / | | \| + | | | | | || + | | MS0 MS1 MS2 MS3 | + | | | | | || + + OUT0 OUT1 OUT2 OUT3 + +The output clock can choose from any of the above clock as its source, with +exceptions: MS1 can only be used for OUT1, MS2 can only be used for OUT2, MS3 +can only be used for OUT3. + +==I2C device node== + +Required properties: +- compatible: shall be "silabs,si5338". +- reg: i2c device address, shall be 0x60, 0x61, 0x70, or 0x71 +- #clock-cells: shall be set to 1 for multiple outputs +- clocks: list of parent clocks in the order of , , , , + Note, xtal and in1/2 are mutually exclusive. Only one can be set. +- #address-cells: shall be set to 1. +- #size-cells: shall be set to 0. + +Optional properties if not set by platform driver: +- silab,ref-source: source of refclk, valid value is defined as + #define SI5338_REF_SRC_CLKIN12 0 + #define SI5338_REF_SRC_CLKIN3 1 + #define SI5338_REF_SRC_XTAL 4 +- silab,fb-source: source of fbclk, valid value is defined as + #define SI5338_FB_SRC_CLKIN42 + #define SI5338_FB_SRC_CLKIN56 3 + #define SI5338_FB_SRC_NOCLK 5 +- silabs,pll-source: source of pll, valid value is defined as + #define SI5338_PFD_IN_REF_REFCLK 0 + #define SI5338_PFD_IN_REF_FBCLK1 + #define SI5338_PFD_IN_REF_DIVREFCLK2 + #define SI5338_PFD_IN_REF_DIVFBCLK 3 + #define SI5338_PFD_IN_REF_XOCLK4 + #define SI5338_PFD_IN_REF_NOCLK5 +- silabs,pll-master: Pick one MS (0, 1, 2, or 3) to allow chaning PLL rate + This is arbitrary since MS0/1/2/3 share one PLL. PLL can be calculated + backward to satisfy MS. + +==Child nodes== + +Each of the clock outputs can be configured individually by +using a child node to the I2C device node. If a child node for a clock +output is not set, platform driver has to set up. + +Required child node properties: +- reg: number of clock output. + +Optional child node properties: +- silabs,drive-config: the configuration of output driver + The valid value list is long. Please refer to soruce code. +- silabs,clock-source: source clock of the output
