Re: [PATCH] clk: add si5351 i2c common clock driver
Quoting Sebastian Hesselbarth (2013-02-09 04:59:32) > This patch adds a common clock driver for Silicon Labs Si5351a/b/c > i2c programmable clock generators. Currently, the driver supports > DT kernels only and VXCO feature of si5351b is not implemented. DT > bindings selectively allow to overwrite stored Si5351 configuration > which is very helpful for clock generators with empty eeprom > configuration. Corresponding device tree binding documentation is > also added. > > Signed-off-by: Sebastian Hesselbarth > --- > Notes: > - During development I used a debugfs clock consumer that I can also > post if there is interest in it. Please do. I have a set of patches that implement a fake clock subtree for testing the core framework. I've been thinking of pushing this to the list once it is more presentable and your work might fit into that nicely. > - With current (3.8-rc6) common clock framework there is two (minor) > issues: > * although clocks are registered with devm_clk_register they are not > removed from the clock tree on unloading. That makes reloading of > clk-si5351 as module impossible. This is a known issue. clk_unregister is a NOP and defining it has always been deferred until the day that someone needed it. Care to take a crack at it? > * potentially there could be more than one different external si5351 > generators but clocks are registered with names that do not refer > to e.g. the device name. Maybe common clock framework should > prepend the device name for each registered clock, i.e. 0-0060.clk0. > That would also avoid name collisions with same clock names from > different drivers (clk0 is likely to be used by others ;)) More unfinished work, just like clk_unregister above. I'm sure you are aware that clk_register takes struct device *dev as input, but does nothing with it. It wouldn't take much to concatenate the device name and clock name if dev is present. However a complication here is that the registration code takes a parent string name to match parents up for discrete subtrees; how could statically defined data know about the device name ahead of time? The above design decision took place before the big DT push we have today and was short-sighted. It would be better to change the framework to rely less on string name lookups and DT is one way out of that. 3.8-rc7 is already out and I don't plan to take anything that hasn't already been submitted for 3.9 now. Can you resubmit this after 3.9-rc1 comes out? Thanks, Mike ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH] clk: add si5351 i2c common clock driver
On 02/11/2013 06:46 AM, Mike Turquette wrote: Quoting Sebastian Hesselbarth (2013-02-09 04:59:32) This patch adds a common clock driver for Silicon Labs Si5351a/b/c i2c programmable clock generators. Currently, the driver supports DT kernels only and VXCO feature of si5351b is not implemented. DT bindings selectively allow to overwrite stored Si5351 configuration which is very helpful for clock generators with empty eeprom configuration. Corresponding device tree binding documentation is also added. Signed-off-by: Sebastian Hesselbarth --- Notes: - During development I used a debugfs clock consumer that I can also post if there is interest in it. Please do. I have a set of patches that implement a fake clock subtree for testing the core framework. I've been thinking of pushing this to the list once it is more presentable and your work might fit into that nicely. Mike, then I will clean the debugfs driver and post it together with this patch for 3.9-rc1 as an individual patch. - With current (3.8-rc6) common clock framework there is two (minor) issues: * although clocks are registered with devm_clk_register they are not removed from the clock tree on unloading. That makes reloading of clk-si5351 as module impossible. This is a known issue. clk_unregister is a NOP and defining it has always been deferred until the day that someone needed it. Care to take a crack at it? Ok. I can have a look at it and propose a patch but that will take a while as other stuff came in between. But IMHO, preparing/enabling clocks by clock consumers should increase reference count so referenced modules cannot be unloaded.. but that I have never had a look at, yet ;) * potentially there could be more than one different external si5351 generators but clocks are registered with names that do not refer to e.g. the device name. Maybe common clock framework should prepend the device name for each registered clock, i.e. 0-0060.clk0. That would also avoid name collisions with same clock names from different drivers (clk0 is likely to be used by others ;)) More unfinished work, just like clk_unregister above. I'm sure you are aware that clk_register takes struct device *dev as input, but does nothing with it. It wouldn't take much to concatenate the device name and clock name if dev is present. However a complication here is that the registration code takes a parent string name to match parents up for discrete subtrees; how could statically defined data know about the device name ahead of time? I see. Wrt the above comment about spare time, would prepending DT clocks be sufficient? Or/And use a fallback mechanism that first tries a full match, full match with own device name, and relaxed match for clock name as it is now? The above design decision took place before the big DT push we have today and was short-sighted. It would be better to change the framework to rely less on string name lookups and DT is one way out of that. 3.8-rc7 is already out and I don't plan to take anything that hasn't already been submitted for 3.9 now. Can you resubmit this after 3.9-rc1 comes out? Sure, but I'll be not available next 2 weeks or so. If 3.8 falls within that time, I will re-post it later. It is ok for me, if it has to go in after 3.9 also. Sebastian ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH] clk: add si5351 i2c common clock driver
Hi Sebastian,
On 09.02.2013 13:59, Sebastian Hesselbarth wrote:
> This patch adds a common clock driver for Silicon Labs Si5351a/b/c
> i2c programmable clock generators. Currently, the driver supports
> DT kernels only and VXCO feature of si5351b is not implemented. DT
> bindings selectively allow to overwrite stored Si5351 configuration
> which is very helpful for clock generators with empty eeprom
> configuration. Corresponding device tree binding documentation is
> also added.
Thank you very much for your work! I had to wait for OMAP platforms to
move over to the common clock framework, but with that in place now, I
could finally give it a test.
Some comments below.
> - The driver has been frequency tested for some common video/audio
> clocks and manages it to tune in every frequency successfully. A
> comparison with silabs windows tool shows a different heuristic
> for vco frequencies. The tests have been comfirmed by visual
> check on an 500MHz oscilloscope but no jitter measurements have
> been carried out. I will provide comparison by email on request.
I would be interested in more tests, yes. My first checks set up clkout1
to 32.768 KHz, and on my oscilloscope, I measured some 1.04 KHz only.
Will do some debugging later.
> +/*
> + * Si5351 xtal clock input
> + */
> +static int si5351_xtal_prepare(struct clk_hw *hw)
> +{
> + struct si5351_driver_data *drvdata =
> + container_of(hw, struct si5351_driver_data, xtal);
> + si5351_set_bits(drvdata, SI5351_FANOUT_ENABLE,
> + SI5351_XTAL_ENABLE, SI5351_XTAL_ENABLE);
> + return 0;
> +}
> +
> +static void si5351_xtal_unprepare(struct clk_hw *hw)
> +{
> + struct si5351_driver_data *drvdata =
> + container_of(hw, struct si5351_driver_data, xtal);
> + si5351_set_bits(drvdata, SI5351_FANOUT_ENABLE,
> + SI5351_XTAL_ENABLE, 0);
> +}
> +
> +static int si5351_xtal_is_enabled(struct clk_hw *hw)
> +{
> + struct si5351_driver_data *drvdata =
> + container_of(hw, struct si5351_driver_data, xtal);
> + unsigned char reg;
> + reg = si5351_reg_read(drvdata, SI5351_FANOUT_ENABLE);
> + return (reg & SI5351_XTAL_ENABLE) ? 1 : 0;
> +}
On an AM33xx platform, I got tons of BUGs like this one:
[5.028525] BUG: scheduling while atomic: swapper/1/0x0002
[5.034600] INFO: lockdep is turned off.
[5.038682] Modules linked in:
[5.041866] irq event stamp: 136090
[5.045496] hardirqs last enabled at (136089): []
_raw_spin_unlock_irqrestore+0x60/0x68
[5.054843] hardirqs last disabled at (136090): []
_raw_spin_lock_irqsave+0x1c/0x60
[5.063733] softirqs last enabled at (135026): []
__do_softirq+0x150/0x1b4
[5.071907] softirqs last disabled at (135019): []
irq_exit+0x98/0xa0
[5.079541] [] (unwind_backtrace+0x0/0xf8) from
[] (__schedule_bug+0x58/0x78)
[5.088793] [] (__schedule_bug+0x58/0x78) from []
(__schedule+0x3c8/0x45c)
[5.097773] [] (__schedule+0x3c8/0x45c) from []
(schedule_timeout+0x120/0x1d4)
[5.107114] [] (schedule_timeout+0x120/0x1d4) from
[] (msleep+0x14/0x20)
[5.115912] [] (msleep+0x14/0x20) from []
(omap_i2c_wait_for_bb+0x68/0xb0)
[5.124890] [] (omap_i2c_wait_for_bb+0x68/0xb0) from
[] (omap_i2c_xfer+0x3c/0xfc)
[5.134503] [] (omap_i2c_xfer+0x3c/0xfc) from []
(__i2c_transfer+0x44/0x80)
[5.143575] [] (__i2c_transfer+0x44/0x80) from []
(i2c_transfer+0x5c/0xbc)
[5.152555] [] (i2c_transfer+0x5c/0xbc) from []
(regmap_i2c_read+0x4c/0x6c)
[5.161625] [] (regmap_i2c_read+0x4c/0x6c) from
[] (_regmap_raw_read+0x98/0xdc)
[5.171056] [] (_regmap_raw_read+0x98/0xdc) from
[] (_regmap_read+0x64/0x9c)
[5.180215] [] (_regmap_read+0x64/0x9c) from []
(regmap_read+0x44/0x5c)
[5.188923] [] (regmap_read+0x44/0x5c) from []
(si5351_clkout_is_enabled+0x74/0xb8)
[5.198720] [] (si5351_clkout_is_enabled+0x74/0xb8) from
[] (clk_disable_unused_subtree+0x68/0xac)
[5.209873] [] (clk_disable_unused_subtree+0x68/0xac) from
[] (clk_disable_unused_subtree+0x20/0xac)
This is because clk_disable_unused_subtree() calls ->is_enabled() with a
spinlock held, but si5351_xtal_is_enabled() will be sleeping, either by
acquiring a mutex in the regmap core, or in the OMAP's i2c transfer
routine. So bottom line is to make this callback atomic, and I did that
locally for now by caching the 'prepare' state of xtal, clkin and the
individual clocks. I can share a patch if you like.
> +static unsigned long si5351_msynth_recalc_rate(struct clk_hw *hw,
> +unsigned long parent_rate)
> +{
> + struct si5351_hw_data *hwdata =
> + container_of(hw, struct si5351_hw_data, hw);
> + unsigned char reg = SI5351_CLK0_PARAMETERS +
> + (SI5351_PARAMETERS_LENGTH * hwdata->num);
> + unsigned long long rate;
> + unsigned long m;
> +
> + if (!hwdata->params.valid)
> + si5351_read_parameters(hwdata->drvdata, reg, &hwdata->params);
> +
Re: [PATCH] clk: add si5351 i2c common clock driver
Hi Sebastian,
I did some more tests today and it took me a while to dig for the root
cause why things were not working for me in the first place - see below.
On 09.02.2013 13:59, Sebastian Hesselbarth wrote:
> +==Example==
> +
> +/* 25MHz reference crystal */
> +ref25: ref25M {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <2500>;
> +};
> +
> +i2c-master-node {
> +
> + /* Si5351a msop10 i2c clock generator */
> + si5351a: clock-generator@60 {
> + compatible = "silabs,si5351a-msop";
> + reg = <0x60>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + #clock-cells = <1>;
> +
> + /* connect xtal input to 25MHz reference */
> + clocks = <&ref25>;
As referred to in another thread, registering the ref25M clock that way
didn't suffice for me on my platform - but that's a different story.
> +static void si5351_read_parameters(struct si5351_driver_data *drvdata,
> + unsigned char reg, struct si5351_parameters *params)
> +{
> + unsigned char buf[SI5351_PARAMETERS_LENGTH];
On a general note, I think you can use u8 instead of unsigned char all
over the place here, which will save you some indentation trouble.
> +static inline int _si5351_clkout_reparent(struct si5351_driver_data *drvdata,
> + unsigned char num, unsigned char parent)
> +{
> + struct clk *pclk;
> +
> + if (num > 8 ||
> + (drvdata->variant == SI5351_VARIANT_A3 && num > 3))
> + return -EINVAL;
> +
> + switch (parent) {
> + case 0:
> + pclk = drvdata->msynth[num].hw.clk;
> + break;
> + case 1:
> + pclk = drvdata->msynth[0].hw.clk;
> + if (num >= 4)
> + pclk = drvdata->msynth[4].hw.clk;
> + break;
> + case 2:
> + pclk = drvdata->xtal.clk;
> + break;
> + case 3:
> + if (drvdata->variant != SI5351_VARIANT_C)
> + return -EINVAL;
> + pclk = drvdata->clkin.clk;
> + break;
> + default:
> + return -EINVAL;
> + }
> + return clk_set_parent(drvdata->clkout[num].hw.clk, pclk);
> +}
[...]
> +static int si5351_clkout_set_parent(struct clk_hw *hw, u8 index)
> +{
> + struct si5351_hw_data *hwdata =
> + container_of(hw, struct si5351_hw_data, hw);
> + unsigned val;
> +
> + val = 0;
> + hw->clk->flags &= ~CLK_SET_RATE_PARENT;
> + switch (index) {
> + case 0:
> + hw->clk->flags |= CLK_SET_RATE_PARENT;
> + val = SI5351_CLK_INPUT_MULTISYNTH_N;
> + break;
I fugured that _si5351_clkout_reparent() wouldn't actually call
->set_parent() on the clock, which leads to the fact that
CLK_SET_RATE_PARENT is not set in the flags. That way, only the clkout
end leaf is actually supplied with a new rate, which leads to incorrect
effective clocks, depending on the current multisynth/pll configuration.
The reason for this is in clk_set_parent() itself, which bails if the
parent is already set to the passed value:
if (clk->parent == parent)
goto out;
I fixed that for now by explicitly setting the clock's parent to NULL
before calling clk_set_parent() in _si5351_clkout_reparent(), so the
calbacks are triggered. But there might be a nicer way, for example to
factor out the CLK_SET_RATE_PARENT handling to some function called from
_si5351_clkout_reparent() or so.
Anyway, with this hack in place along with the other details I mentioned
in my first mail, the driver seems to work for me now, which is great. I
will do more extensive tests later that week when I have access to
better scopes ...
Many thanks again,
Daniel
> + case 1:
> + /* clk0/clk4 can only connect to its own multisync */
> + if (hwdata->num == 0 || hwdata->num == 4)
> + val = SI5351_CLK_INPUT_MULTISYNTH_N;
> + else
> + val = SI5351_CLK_INPUT_MULTISYNTH_0_4;
> + break;
> + case 2:
> + val = SI5351_CLK_INPUT_XTAL;
> + break;
> + case 3:
> + val = SI5351_CLK_INPUT_CLKIN;
> + break;
> + }
> + si5351_set_bits(hwdata->drvdata, SI5351_CLK0_CTRL + hwdata->num,
> + SI5351_CLK_INPUT_MASK, val);
> +
> + return 0;
> +}
> +
> +static unsigned long si5351_clkout_recalc_rate(struct clk_hw *hw,
> +unsigned long parent_rate)
> +{
> + struct si5351_hw_data *hwdata =
> + container_of(hw, struct si5351_hw_data, hw);
> + unsigned char reg = SI5351_CLK0_PARAMETERS +
> + (SI5351_PARAMETERS_LENGTH * hwdata->num);
> + unsigned char rdiv;
> +
> + rdiv = (si5351_reg_read(hwdata->drvdata, reg + 2) &
> + SI5351_OUTPUT_CLK_DIV_MASK) >> SI5351_OUTPUT_CLK_DIV_SHIFT;
> +
> +
Re: [PATCH] clk: add si5351 i2c common clock driver
Daniel,
first of all sorry for the late answer but thanks for testing the driver.
On 2/19/13, Daniel Mack wrote:
> Hi Sebastian,
>
> I did some more tests today and it took me a while to dig for the root
> cause why things were not working for me in the first place - see below.
>
>
> On 09.02.2013 13:59, Sebastian Hesselbarth wrote:
>
>> +==Example==
>> +
>> +/* 25MHz reference crystal */
>> +ref25: ref25M {
>> +compatible = "fixed-clock";
>> +#clock-cells = <0>;
>> +clock-frequency = <2500>;
>> +};
>> +
>> +i2c-master-node {
>> +
>> +/* Si5351a msop10 i2c clock generator */
>> +si5351a: clock-generator@60 {
>> +compatible = "silabs,si5351a-msop";
>> +reg = <0x60>;
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +#clock-cells = <1>;
>> +
>> +/* connect xtal input to 25MHz reference */
>> +clocks = <&ref25>;
>
> As referred to in another thread, registering the ref25M clock that way
> didn't suffice for me on my platform - but that's a different story.
I guess "fixed-clock" isn't registered by OMAP's clock init code? I had
to do this on dove, too. Actually, I will come back to clock initialization for
dove later and was hoping that there will be some global way of registering
core common clock drivers (or at least fixed-clock) until then.
>> +static void si5351_read_parameters(struct si5351_driver_data *drvdata,
>> +unsigned char reg, struct si5351_parameters *params)
>> +{
>> +unsigned char buf[SI5351_PARAMETERS_LENGTH];
>
> On a general note, I think you can use u8 instead of unsigned char all
> over the place here, which will save you some indentation trouble.
Ok, I guess I was deriving "unsigned char" usage from other clock drivers
and never went to u8. But I ll reconsider using u8 when all issues are
worked out.
>> +static inline int _si5351_clkout_reparent(struct si5351_driver_data
>> *drvdata,
>> + unsigned char num, unsigned char parent)
>> +{
>> +struct clk *pclk;
>> +
>> +if (num > 8 ||
>> +(drvdata->variant == SI5351_VARIANT_A3 && num > 3))
>> +return -EINVAL;
>> +
>> +switch (parent) {
>> +case 0:
>> +pclk = drvdata->msynth[num].hw.clk;
>> +break;
>> +case 1:
>> +pclk = drvdata->msynth[0].hw.clk;
>> +if (num >= 4)
>> +pclk = drvdata->msynth[4].hw.clk;
>> +break;
>> +case 2:
>> +pclk = drvdata->xtal.clk;
>> +break;
>> +case 3:
>> +if (drvdata->variant != SI5351_VARIANT_C)
>> +return -EINVAL;
>> +pclk = drvdata->clkin.clk;
>> +break;
>> +default:
>> +return -EINVAL;
>> +}
>> +return clk_set_parent(drvdata->clkout[num].hw.clk, pclk);
>> +}
>
> [...]
>
>> +static int si5351_clkout_set_parent(struct clk_hw *hw, u8 index)
>> +{
>> +struct si5351_hw_data *hwdata =
>> +container_of(hw, struct si5351_hw_data, hw);
>> +unsigned val;
>> +
>> +val = 0;
>> +hw->clk->flags &= ~CLK_SET_RATE_PARENT;
>> +switch (index) {
>> +case 0:
>> +hw->clk->flags |= CLK_SET_RATE_PARENT;
>> +val = SI5351_CLK_INPUT_MULTISYNTH_N;
>> +break;
>
> I fugured that _si5351_clkout_reparent() wouldn't actually call
> ->set_parent() on the clock, which leads to the fact that
> CLK_SET_RATE_PARENT is not set in the flags. That way, only the clkout
> end leaf is actually supplied with a new rate, which leads to incorrect
> effective clocks, depending on the current multisynth/pll configuration.
Yeah, true. Unfortunately, _clkout_reparent() is more like a dirty hack to
allow to reparent the clock output. At registration internal configuration of
si5351 is not known and when I parse the DT for clock configuration I might
have been already assigned to the same parent clk that you later explicitly
configure.
What I basically want for si5351 (or any other eeprom based programmable
clock gen) is that stored configuration is not touched. But it can be changed
after eeprom contents have been copied into device's sram - and this _is_
mandatory for the si5351 that I use on CuBox where there is no useful config
stored at all.
Anyway, there still seem to be some more issues with doing it right on current
common clk framwork - thanks for pointing it out.
> The reason for this is in clk_set_parent() itself, which bails if the
> parent is already set to the passed value:
>
> if (clk->parent == parent)
> goto out;
>
> I fixed that for now by explicitly setting the clock's parent to NULL
> before calling clk_set_parent() in _si5351_clkout_reparent(), so the
> calbacks are triggered. But there might be a nicer way, for example to
> factor out the CLK_SET_RATE_PARENT handling to some function called from
> _si5351_clkout_reparent() or so.
>
> Anyway, with this hack in place along w
Re: [PATCH] clk: add si5351 i2c common clock driver
Hi Sebastian,
On 27.02.2013 11:01, Sebastian Hesselbarth wrote:
> first of all sorry for the late answer but thanks for testing the driver.
>
> On 2/19/13, Daniel Mack wrote:
>> Hi Sebastian,
>>
>> I did some more tests today and it took me a while to dig for the root
>> cause why things were not working for me in the first place - see below.
>>
>>
>> On 09.02.2013 13:59, Sebastian Hesselbarth wrote:
>>
>>> +==Example==
>>> +
>>> +/* 25MHz reference crystal */
>>> +ref25: ref25M {
>>> + compatible = "fixed-clock";
>>> + #clock-cells = <0>;
>>> + clock-frequency = <2500>;
>>> +};
>>> +
>>> +i2c-master-node {
>>> +
>>> + /* Si5351a msop10 i2c clock generator */
>>> + si5351a: clock-generator@60 {
>>> + compatible = "silabs,si5351a-msop";
>>> + reg = <0x60>;
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> + #clock-cells = <1>;
>>> +
>>> + /* connect xtal input to 25MHz reference */
>>> + clocks = <&ref25>;
>>
>> As referred to in another thread, registering the ref25M clock that way
>> didn't suffice for me on my platform - but that's a different story.
>
> I guess "fixed-clock" isn't registered by OMAP's clock init code? I had
> to do this on dove, too. Actually, I will come back to clock initialization
> for
> dove later and was hoping that there will be some global way of registering
> core common clock drivers (or at least fixed-clock) until then.
>
>>> +static void si5351_read_parameters(struct si5351_driver_data *drvdata,
>>> + unsigned char reg, struct si5351_parameters *params)
>>> +{
>>> + unsigned char buf[SI5351_PARAMETERS_LENGTH];
>>
>> On a general note, I think you can use u8 instead of unsigned char all
>> over the place here, which will save you some indentation trouble.
>
> Ok, I guess I was deriving "unsigned char" usage from other clock drivers
> and never went to u8. But I ll reconsider using u8 when all issues are
> worked out.
>
>>> +static inline int _si5351_clkout_reparent(struct si5351_driver_data
>>> *drvdata,
>>> + unsigned char num, unsigned char parent)
>>> +{
>>> + struct clk *pclk;
>>> +
>>> + if (num > 8 ||
>>> + (drvdata->variant == SI5351_VARIANT_A3 && num > 3))
>>> + return -EINVAL;
>>> +
>>> + switch (parent) {
>>> + case 0:
>>> + pclk = drvdata->msynth[num].hw.clk;
>>> + break;
>>> + case 1:
>>> + pclk = drvdata->msynth[0].hw.clk;
>>> + if (num >= 4)
>>> + pclk = drvdata->msynth[4].hw.clk;
>>> + break;
>>> + case 2:
>>> + pclk = drvdata->xtal.clk;
>>> + break;
>>> + case 3:
>>> + if (drvdata->variant != SI5351_VARIANT_C)
>>> + return -EINVAL;
>>> + pclk = drvdata->clkin.clk;
>>> + break;
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> + return clk_set_parent(drvdata->clkout[num].hw.clk, pclk);
>>> +}
>>
>> [...]
>>
>>> +static int si5351_clkout_set_parent(struct clk_hw *hw, u8 index)
>>> +{
>>> + struct si5351_hw_data *hwdata =
>>> + container_of(hw, struct si5351_hw_data, hw);
>>> + unsigned val;
>>> +
>>> + val = 0;
>>> + hw->clk->flags &= ~CLK_SET_RATE_PARENT;
>>> + switch (index) {
>>> + case 0:
>>> + hw->clk->flags |= CLK_SET_RATE_PARENT;
>>> + val = SI5351_CLK_INPUT_MULTISYNTH_N;
>>> + break;
>>
>> I fugured that _si5351_clkout_reparent() wouldn't actually call
>> ->set_parent() on the clock, which leads to the fact that
>> CLK_SET_RATE_PARENT is not set in the flags. That way, only the clkout
>> end leaf is actually supplied with a new rate, which leads to incorrect
>> effective clocks, depending on the current multisynth/pll configuration.
>
> Yeah, true. Unfortunately, _clkout_reparent() is more like a dirty hack to
> allow to reparent the clock output. At registration internal configuration of
> si5351 is not known and when I parse the DT for clock configuration I might
> have been already assigned to the same parent clk that you later explicitly
> configure.
Hmm, but then we need to either set the configuration on the chip to
match the internal state of the driver, or tell the clock framework that
the current state is not currently known (don't know if that's possible).
> What I basically want for si5351 (or any other eeprom based programmable
> clock gen) is that stored configuration is not touched. But it can be changed
> after eeprom contents have been copied into device's sram - and this _is_
> mandatory for the si5351 that I use on CuBox where there is no useful config
> stored at all.
Yes, same here. We set up the si5351 from the bootloader with some
binary blob via i2c to bring some components to live before the kernel
boots.
> Anyway, there still seem to be some more issues with doing it right on current
> common clk framwork - thanks for pointing it out.
I'd be happy to test more versio
