When p3 is eual to 0 then result for fOUT is 0. In that case we should
return 0 not parent_rate;
This issue was causing deadlock in si5351 chip when user set_rate for
ms0->clk0 and then set_rate for ms1->clk1 (both ms sourced from plla).
After that clk1 was 0 and it wasn't possible to enable it again (try also
manually with i2cset commands). Only power cycle helps.
Signed-off-by: Marek Belisko <[email protected]>
---
drivers/clk/clk-si5351.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
index fc215ce..a8fc0f4 100644
--- a/drivers/clk/clk-si5351.c
+++ b/drivers/clk/clk-si5351.c
@@ -607,7 +607,7 @@ static unsigned long si5351_msynth_recalc_rate(struct clk_hw *hw,
si5351_read_parameters(hwdata->drvdata, reg, &hwdata->params);
if (hwdata->params.p3 == 0)
- return parent_rate;
+ return 0;
/*
* multisync0-5: fOUT = (128 * P3 * fIN) / (P1*P3 + P2 + 512*P3)
--
1.7.9.5
When rate is 0 powerdown clock output.
Signed-off-by: Marek Belisko <[email protected]>
---
drivers/clk/clk-si5351.c | 32 +++++++++++++++++++-------------
1 file changed, 19 insertions(+), 13 deletions(-)
diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
index a8fc0f4..e254e76 100644
--- a/drivers/clk/clk-si5351.c
+++ b/drivers/clk/clk-si5351.c
@@ -1042,20 +1042,22 @@ static int si5351_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
struct si5351_hw_data *hwdata =
container_of(hw, struct si5351_hw_data, hw);
unsigned long new_rate, new_err, err;
- unsigned char rdiv;
+ unsigned char rdiv, pdn = 0;
/* round to closed rdiv */
rdiv = SI5351_OUTPUT_CLK_DIV_1;
- new_rate = parent_rate;
- err = abs(new_rate - rate);
- do {
- new_rate >>= 1;
- new_err = abs(new_rate - rate);
- if (new_err > err || rdiv == SI5351_OUTPUT_CLK_DIV_128)
- break;
- rdiv++;
- err = new_err;
- } while (1);
+ if (rate) {
+ new_rate = parent_rate;
+ err = abs(new_rate - rate);
+ do {
+ new_rate >>= 1;
+ new_err = abs(new_rate - rate);
+ if (new_err > err || rdiv == SI5351_OUTPUT_CLK_DIV_128)
+ break;
+ rdiv++;
+ err = new_err;
+ } while (1);
+ }
/* write output divider */
switch (hwdata->num) {
@@ -1075,9 +1077,13 @@ static int si5351_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
rdiv << SI5351_OUTPUT_CLK_DIV_SHIFT);
}
- /* powerup clkout */
+ /* when rate is 0 powerdown output */
+ if (rate == 0)
+ pdn = SI5351_CLK_POWERDOWN;
+
+ /* powerup/down clkout */
si5351_set_bits(hwdata->drvdata, SI5351_CLK0_CTRL + hwdata->num,
- SI5351_CLK_POWERDOWN, 0);
+ SI5351_CLK_POWERDOWN, pdn);
dev_dbg(&hwdata->drvdata->client->dev,
"%s - %s: rdiv = %u, parent_rate = %lu, rate = %lu\n",
--
1.7.9.5
On 05/07/2013 10:14 AM, Marek Belisko wrote:
> When p3 is eual to 0 then result for fOUT is 0. In that case we should
> return 0 not parent_rate;
>
> This issue was causing deadlock in si5351 chip when user set_rate for
> ms0->clk0 and then set_rate for ms1->clk1 (both ms sourced from plla).
> After that clk1 was 0 and it wasn't possible to enable it again (try also
> manually with i2cset commands). Only power cycle helps.
Marek,
does the fix below also affect the behavior above, i.e. not causing
si5351 to hang?
Anyway, your fix looks very sane to me and you get my
Acked-by: Sebastian Hesselbarth <[email protected]>
> Signed-off-by: Marek Belisko<[email protected]>
> ---
> drivers/clk/clk-si5351.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
> index fc215ce..a8fc0f4 100644
> --- a/drivers/clk/clk-si5351.c
> +++ b/drivers/clk/clk-si5351.c
> @@ -607,7 +607,7 @@ static unsigned long si5351_msynth_recalc_rate(struct clk_hw *hw,
> si5351_read_parameters(hwdata->drvdata, reg,&hwdata->params);
>
> if (hwdata->params.p3 == 0)
> - return parent_rate;
> + return 0;
>
> /*
> * multisync0-5: fOUT = (128 * P3 * fIN) / (P1*P3 + P2 + 512*P3)
On 05/07/2013 10:14 AM, Marek Belisko wrote:
> When rate is 0 powerdown clock output.
>
> Signed-off-by: Marek Belisko<[email protected]>
> ---
> drivers/clk/clk-si5351.c | 32 +++++++++++++++++++-------------
> 1 file changed, 19 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
> index a8fc0f4..e254e76 100644
> --- a/drivers/clk/clk-si5351.c
> +++ b/drivers/clk/clk-si5351.c
> @@ -1042,20 +1042,22 @@ static int si5351_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
> struct si5351_hw_data *hwdata =
> container_of(hw, struct si5351_hw_data, hw);
> unsigned long new_rate, new_err, err;
> - unsigned char rdiv;
> + unsigned char rdiv, pdn = 0;
>
> /* round to closed rdiv */
> rdiv = SI5351_OUTPUT_CLK_DIV_1;
> - new_rate = parent_rate;
> - err = abs(new_rate - rate);
> - do {
> - new_rate>>= 1;
> - new_err = abs(new_rate - rate);
> - if (new_err> err || rdiv == SI5351_OUTPUT_CLK_DIV_128)
> - break;
> - rdiv++;
> - err = new_err;
> - } while (1);
> + if (rate) {
> + new_rate = parent_rate;
Marek,
initalizing pdn to SI5351_CLK_POWERDOWN and skip (with goto) to
powerup/down call on zero rate will save us a level of indention here.
> + err = abs(new_rate - rate);
> + do {
> + new_rate>>= 1;
> + new_err = abs(new_rate - rate);
> + if (new_err> err || rdiv == SI5351_OUTPUT_CLK_DIV_128)
> + break;
> + rdiv++;
> + err = new_err;
> + } while (1);
of course, pdn should be set to 0 here.
> + }
>
> /* write output divider */
> switch (hwdata->num) {
> @@ -1075,9 +1077,13 @@ static int si5351_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
> rdiv<< SI5351_OUTPUT_CLK_DIV_SHIFT);
> }
>
> - /* powerup clkout */
> + /* when rate is 0 powerdown output */
> + if (rate == 0)
> + pdn = SI5351_CLK_POWERDOWN;
> +
> + /* powerup/down clkout */
> si5351_set_bits(hwdata->drvdata, SI5351_CLK0_CTRL + hwdata->num,
> - SI5351_CLK_POWERDOWN, 0);
> + SI5351_CLK_POWERDOWN, pdn);
>
> dev_dbg(&hwdata->drvdata->client->dev,
> "%s - %s: rdiv = %u, parent_rate = %lu, rate = %lu\n",
On 05/07/2013 12:03 PM, Sebastian Hesselbarth wrote:
> On 05/07/2013 10:14 AM, Marek Belisko wrote:
>> When p3 is eual to 0 then result for fOUT is 0. In that case we should
>> return 0 not parent_rate;
>>
>> This issue was causing deadlock in si5351 chip when user set_rate for
>> ms0->clk0 and then set_rate for ms1->clk1 (both ms sourced from plla).
>> After that clk1 was 0 and it wasn't possible to enable it again (try also
>> manually with i2cset commands). Only power cycle helps.
>
> Marek,
>
> does the fix below also affect the behavior above, i.e. not causing
> si5351 to hang?
Yes this fix problems with si5351 hang. Didn't investigate why but I
assume some wrong register access or something.
>
> Anyway, your fix looks very sane to me and you get my
>
> Acked-by: Sebastian Hesselbarth <[email protected]>
>
>> Signed-off-by: Marek Belisko<[email protected]>
>> ---
>> drivers/clk/clk-si5351.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
>> index fc215ce..a8fc0f4 100644
>> --- a/drivers/clk/clk-si5351.c
>> +++ b/drivers/clk/clk-si5351.c
>> @@ -607,7 +607,7 @@ static unsigned long
>> si5351_msynth_recalc_rate(struct clk_hw *hw,
>> si5351_read_parameters(hwdata->drvdata, reg,&hwdata->params);
>>
>> if (hwdata->params.p3 == 0)
>> - return parent_rate;
>> + return 0;
>>
>> /*
>> * multisync0-5: fOUT = (128 * P3 * fIN) / (P1*P3 + P2 + 512*P3)
>
>
~marek
--
Marek Belisko
Software Developer
StreamUnlimited Engineering GmbH
Gutheil Schodergasse 8-12
A-1100 Vienna, Austria
Office: +421 267200087
e-mail: [email protected]
http://www.streamunlimited.com
Meet us at:
High End - Munich, 09 - 12 May, Hall 4/F05
IFA - Berlin, 6-11 September
CEDIA - Denver, 25-28 September
On 05/07/2013 12:07 PM, Sebastian Hesselbarth wrote:
> On 05/07/2013 10:14 AM, Marek Belisko wrote:
>> When rate is 0 powerdown clock output.
>>
>> Signed-off-by: Marek Belisko<[email protected]>
>> ---
>> drivers/clk/clk-si5351.c | 32 +++++++++++++++++++-------------
>> 1 file changed, 19 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
>> index a8fc0f4..e254e76 100644
>> --- a/drivers/clk/clk-si5351.c
>> +++ b/drivers/clk/clk-si5351.c
>> @@ -1042,20 +1042,22 @@ static int si5351_clkout_set_rate(struct
>> clk_hw *hw, unsigned long rate,
>> struct si5351_hw_data *hwdata =
>> container_of(hw, struct si5351_hw_data, hw);
>> unsigned long new_rate, new_err, err;
>> - unsigned char rdiv;
>> + unsigned char rdiv, pdn = 0;
>>
>> /* round to closed rdiv */
>> rdiv = SI5351_OUTPUT_CLK_DIV_1;
>> - new_rate = parent_rate;
>> - err = abs(new_rate - rate);
>> - do {
>> - new_rate>>= 1;
>> - new_err = abs(new_rate - rate);
>> - if (new_err> err || rdiv == SI5351_OUTPUT_CLK_DIV_128)
>> - break;
>> - rdiv++;
>> - err = new_err;
>> - } while (1);
>> + if (rate) {
>> + new_rate = parent_rate;
>
> Marek,
>
> initalizing pdn to SI5351_CLK_POWERDOWN and skip (with goto) to
> powerup/down call on zero rate will save us a level of indention here.
OK will fix and resubmit in v2. Thanks.
>
>> + err = abs(new_rate - rate);
>> + do {
>> + new_rate>>= 1;
>> + new_err = abs(new_rate - rate);
>> + if (new_err> err || rdiv == SI5351_OUTPUT_CLK_DIV_128)
>> + break;
>> + rdiv++;
>> + err = new_err;
>> + } while (1);
>
> of course, pdn should be set to 0 here.
>
>> + }
>>
>> /* write output divider */
>> switch (hwdata->num) {
>> @@ -1075,9 +1077,13 @@ static int si5351_clkout_set_rate(struct clk_hw
>> *hw, unsigned long rate,
>> rdiv<< SI5351_OUTPUT_CLK_DIV_SHIFT);
>> }
>>
>> - /* powerup clkout */
>> + /* when rate is 0 powerdown output */
>> + if (rate == 0)
>> + pdn = SI5351_CLK_POWERDOWN;
>> +
>> + /* powerup/down clkout */
>> si5351_set_bits(hwdata->drvdata, SI5351_CLK0_CTRL + hwdata->num,
>> - SI5351_CLK_POWERDOWN, 0);
>> + SI5351_CLK_POWERDOWN, pdn);
>>
>> dev_dbg(&hwdata->drvdata->client->dev,
>> "%s - %s: rdiv = %u, parent_rate = %lu, rate = %lu\n",
>
>
marek
--
Marek Belisko
Software Developer
StreamUnlimited Engineering GmbH
Gutheil Schodergasse 8-12
A-1100 Vienna, Austria
Office: +421 267200087
e-mail: [email protected]
http://www.streamunlimited.com
Meet us at:
High End - Munich, 09 - 12 May, Hall 4/F05
IFA - Berlin, 6-11 September
CEDIA - Denver, 25-28 September