clk: si5351: Some fixes

* property silabs,pll-source : <num src>, [<..>]
* allow to selectively set pll source

drivers/clk/clk-si5351.c +0 -0

@@ -1328,8 +1321,17 @@ static int si5351_i2c_probe(struct i2c_client *client,
        i2c_set_clientdata(client, drvdata);
        drvdata->client = client;
        drvdata->variant = variant;

Quoted 10 lines · Sebastian Hesselbarth

* property silabs,pll-source : <num src>, [<..>]
* allow to selectively set pll source

drivers/clk/clk-si5351.c +2 -2

@@ -1328,8 +1321,17 @@ static int si5351_i2c_probe(struct i2c_client *client,
        i2c_set_clientdata(client, drvdata);
        drvdata->client = client;
        drvdata->variant = variant;
-       drvdata->pxtal = pdata->clk_xtal;
-       drvdata->pclkin = pdata->clk_clkin;
+       drvdata->pxtal = devm_clk_get(&client->dev, "xtal");
+       drvdata->pclkin = devm_clk_get(&client->dev, "clkin");

drivers/clk/clk-si5351.c static int si5351_i2c_probe(struct i2c_client *client, · line 1331

Quoted 6 lines

drivers/clk/clk-si5351.c +6 -0

+
+       if (PTR_ERR(drvdata->pxtal) == -EPROBE_DEFER ||
+           PTR_ERR(drvdata->pclkin) == -EPROBE_DEFER)
+               return -EPROBE_DEFER;
+
+       if (IS_ERR(drvdata->pxtal) && IS_ERR(drvdata->pclkin)) {

Don't you want || instead?

Quoted 18 lines · Fabio Estevam

Quoted 4 lines · Sebastian Hesselbarth

drivers/clk/clk-si5351.c +0 -0

@@ -1328,8 +1321,17 @@ static int si5351_i2c_probe(struct i2c_client *client,
         i2c_set_clientdata(client, drvdata);
         drvdata->client = client;
         drvdata->variant = variant;

drivers/clk/clk-si5351.c static int si5351_i2c_probe(struct i2c_client *client, · line 1331

Quoted 10 lines

drivers/clk/clk-si5351.c +8 -2

-       drvdata->pxtal = pdata->clk_xtal;
-       drvdata->pclkin = pdata->clk_clkin;
+       drvdata->pxtal = devm_clk_get(&client->dev, "xtal");
+       drvdata->pclkin = devm_clk_get(&client->dev, "clkin");
+
+       if (PTR_ERR(drvdata->pxtal) == -EPROBE_DEFER ||
+           PTR_ERR(drvdata->pclkin) == -EPROBE_DEFER)
+               return -EPROBE_DEFER;
+
+       if (IS_ERR(drvdata->pxtal) && IS_ERR(drvdata->pclkin)) {

Don't you want || instead?

I doubt it. He is checking if both are not available.

The driver could work with only one of them.

If you use || then you assume to need both.

Fabio,

Michael is right, the check is for bailing out if none of the parent clocks is available.

Quoted 2 lines · Sebastian Hesselbarth

Michael is right, the check is for bailing out if none of the parent clocks is available.

drivers/clk/clk-si5351.c +4 -0

+       if (IS_ERR(drvdata->pxtal) && IS_ERR(drvdata->pclkin)) {
+               dev_err(&client->dev, "missing at least one parent clock\n");
+               return -EINVAL;
+       }

Then shouldn't the error message be: "missing both parent clocks\n" ?

When changing PLL rate significantly, PLLs have to be reset. Add a function to perform and check for successful PLL reset.

Signed-off-by Sebastian Hesselbarth <[email protected]>

---
Cc: Mike Turquette <[email protected]>
Cc: Stephen Boyd <[email protected]>
Cc: Jean-Francois Moine <[email protected]>
Cc: Michael Welling <[email protected]>
Cc: Russell King <[email protected]>
Cc: devicetree at vger.kernel.org
Cc: linux-clk at vger.kernel.org
Cc: linux-arm-kernel at lists.infradead.org
Cc: linux-kernel at vger.kernel.org

---
 drivers/clk/clk-si5351.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

drivers/clk/clk-si5351.c +26 -0

index beeb57bbb04c..9b97c134e3c1 100644
@@ -366,6 +366,32 @@ static const struct clk_ops si5351_vxco_ops = {
  *         = (MSNx_P1*MSNx_P3 + MSNx_P2 + 512*MSNx_P3)/(128*MSNx_P3)
  *
  */
+static int si5351_pll_reset(struct si5351_hw_data *hwdata)
+{
+	unsigned long timeout;
+	u8 mask = (hwdata->num == 0) ?
+		SI5351_STATUS_LOL_A : SI5351_STATUS_LOL_B;
+
+	si5351_reg_write(hwdata->drvdata, SI5351_PLL_RESET,
+			 (hwdata->num == 0) ? SI5351_PLL_RESET_A :
+			 SI5351_PLL_RESET_B);
+	timeout = jiffies + msecs_to_jiffies(100);
+	do {
+		if ((si5351_reg_read(hwdata->drvdata, SI5351_DEVICE_STATUS) &
+		     mask) == 0)
+			break;
+		if (time_after(jiffies, timeout)) {
+			dev_err(&hwdata->drvdata->client->dev,
+				"timeout waiting for pll %d reset\n",
+				hwdata->num);
+			return -EBUSY;
+		};
+		udelay(250);
+	} while (true);
+
+	return 0;
+}
+
 static int _si5351_pll_reparent(struct si5351_driver_data *drvdata,
 				int num, enum si5351_pll_src parent)
 {

drivers/clk/clk-si5351.c +0 -0

 	dev_dbg(&hwdata->drvdata->client->dev,
 		"%s - %s: p1 = %lu, p2 = %lu, p3 = %lu, parent_rate = %lu, rate = %lu\n",
 		__func__, __clk_get_name(hwdata->hw.clk),

Signature · 1 line

2.1.0

Quoted 5 lines · Sebastian Hesselbarth

When changing PLL rate significantly, PLLs have to be reset. Add a function to perform and check for successful PLL reset.

Signed-off-by Sebastian Hesselbarth <[email protected]>

---

drivers/clk/clk-si5351.c static int si5351_pll_set_rate(struct clk_hw *hw, unsigned long rate, · line 550

Quoted 7 lines

drivers/clk/clk-si5351.c +3 -0

@@ -519,6 +545,9 @@ static int si5351_pll_set_rate(struct clk_hw *hw, unsigned long rate,
  		SI5351_CLK_INTEGER_MODE,
  		(hwdata->params.p2 == 0) ? SI5351_CLK_INTEGER_MODE : 0);
 
+	/* reset pll after rate change */
+	si5351_pll_reset(hwdata);
+

What is the point of having a return code if it is not being used?

For Si5351 clock driver, Michael Welling and Jean-Francois Moine reported issues with recent v4.x kernels due to broken/missing/wrong parent clock claming. This patch set now deals with the issues reported.

Patch 1 amends the binding documentation mention clock-names property for the "xtal" and "clkin" parent clock inputs of Si5351 variants.

Patch 2 adds the clock-names property for the SolidRun CuBox using Si5351 with a fixed oscillator connected to "xtal" input.

Patch 3 reworks the way we claim parent clocks by using devm_clk_get() for both DT and platform_data based registration. Also, properly check for errors returned by devm_clk_get() and prepare/enable the parent clocks.

Patch 4 introduces a function to reset PLLs on rate change. This should improve generated clock output stability. I currently have no scope at hand to actually test that properly, so there may be more issues remaining.

@Michael, Jean-Francois: Please test and report if there are still issues remaining.

Sebastian

Sebastian Hesselbarth (4):
clk: si5351: Mention clock-names in the binding documentation
ARM: dove: Add clock-names to CuBox Si5351 clk generator
clk: si5351: Do not pass struct clk in platform_data
clk: si5351: Reset PLL after rate change

 .../devicetree/bindings/clock/silabs,si5351.txt    |  4 +-
 arch/arm/boot/dts/dove-cubox.dts                   |  1 +
 drivers/clk/clk-si5351.c                           | 87 +++++++++++++++++-----
 include/linux/platform_data/si5351.h               |  4 -
 4 files changed, 73 insertions(+), 23 deletions(-)

---
Cc: Mike Turquette <[email protected]>
Cc: Stephen Boyd <[email protected]>
Cc: Jean-Francois Moine <[email protected]>
Cc: Michael Welling <[email protected]>
Cc: Russell King <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: Andrew Lunn <[email protected]>
Cc: Gregory Clement <[email protected]>
Cc: devicetree at vger.kernel.org
Cc: linux-clk at vger.kernel.org
Cc: linux-arm-kernel at lists.infradead.org
Cc: linux-kernel at vger.kernel.org

Signature · 1 line

2.1.0

For Si5351 clock driver, Michael Welling and Jean-Francois Moine reported issues with recent v4.x kernels due to broken/missing/wrong parent clock claming. This patch set now deals with the issues reported.

Patch 1 amends the binding documentation mention clock-names property for the "xtal" and "clkin" parent clock inputs of Si5351 variants.

Patch 2 adds the clock-names property for the SolidRun CuBox using Si5351 with a fixed oscillator connected to "xtal" input.

Patch 3 reworks the way we claim parent clocks by using devm_clk_get() for both DT and platform_data based registration. Also, properly check for errors returned by devm_clk_get() and prepare/enable the parent clocks.

Patch 4 introduces a function to reset PLLs on rate change. This should improve generated clock output stability. I currently have no scope at hand to actually test that properly, so there may be more issues remaining.

@Michael, Jean-Francois: Please test and report if there are still issues remaining.

Sebastian

Sebastian Hesselbarth (4):
clk: si5351: Mention clock-names in the binding documentation
ARM: dove: Add clock-names to CuBox Si5351 clk generator
clk: si5351: Do not pass struct clk in platform_data
clk: si5351: Reset PLL after rate change

 .../devicetree/bindings/clock/silabs,si5351.txt    |  4 +-
 arch/arm/boot/dts/dove-cubox.dts                   |  1 +
 drivers/clk/clk-si5351.c                           | 87 +++++++++++++++++-----
 include/linux/platform_data/si5351.h               |  4 -
 4 files changed, 73 insertions(+), 23 deletions(-)

---
Cc: Mike Turquette <[email protected]>
Cc: Stephen Boyd <[email protected]>
Cc: Jean-Francois Moine <[email protected]>
Cc: Michael Welling <[email protected]>
Cc: Russell King <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: Andrew Lunn <[email protected]>
Cc: Gregory Clement <[email protected]>
Cc: devicetree at vger.kernel.org
Cc: linux-clk at vger.kernel.org
Cc: linux-arm-kernel at lists.infradead.org
Cc: linux-kernel at vger.kernel.org

Signature · 1 line

2.1.0

Quoted 3 lines · Sebastian Hesselbarth

For Si5351 clock driver, Michael Welling and Jean-Francois Moine reported issues with recent v4.x kernels due to broken/missing/wrong parent clock claming. This patch set now deals with the issues reported.

Okay, the results are in and they are mixed. Firstly the clocks register unlike before. This is a positive step that was certianly expected.

Second the reported and measured clock frequencies do not match the device tree entries.

Measured frequencies:
clk0 12.5Mhz
clk1 5.357Mhz
clk2 0 Hz

Reported frequencies:
root at som3517-som200:~# head -n 15 /sys/kernel/debug/clk/clk_summary
clock enable_cnt prepare_cnt rate accuracy phase
----------------------------------------------------------------------------------------

              clk2                         0            0    12288000          0 0
              clk0                         0            0    12499999          0 0
              clk1                         0            0     5357142          0 0

Device tree entry:

         si5351: clock-generator {
                 /* connect xtal input to 27MHz reference */
                 clocks = <&ref27>;
                 clock-names = "xtal";

                 clkout0: clkout0 {
                         reg = <0>;
                         clock-frequency = <18432000>;
                 };

                 clkout1: clkout1 {
                         reg = <1>;
                         clock-frequency = <8000000>;
                 };

                 clkout2: clkout2 {
                         reg = <2>;
                         clock-frequency = <12288000>;
                 };
         };

Lastly if #define DEBUG is added the behavior is different.

Debugging output:

[    2.970753] si5351 0-0060: si5351_clkout_round_rate - clk0: rdiv = 1, parent_rate = 18432000, rate = 18432000
[    2.981207] si5351 0-0060: si5351_msynth_round_rate - ms0: a = 48, b = 0, c = 1, divby4 = 0, parent_rate = 884736000, rate = 18432000
[    2.993777] si5351 0-0060: si5351_pll_round_rate - pllb: a = 32, b = 96, c = 125, parent_rate = 27000000, rate = 884736000
[    3.005362] si5351 0-0060: si5351_msynth_recalc_rate - ms0: p1 = 5632, p2 = 0, p3 = 1, m = 6144, parent_rate = 884736000, rate = 18432000
[    3.026281] si5351 0-0060: si5351_pll_set_rate - pllb: p1 = 3682, p2 = 38, p3 = 125, parent_rate = 27000000, rate = 884736000
[    3.038151] si5351 0-0060: si5351_pll_recalc_rate - pllb: p1 = 3682, p2 = 38, p3 = 125, parent_rate = 27000000, rate = 884736000
[    3.053933] si5351 0-0060: si5351_msynth_set_rate - ms2: p1 = 0, p2 = 0, p3 = 0, divby4 = 1, parent_rate = 884736000, rate = 884736000

[    3.068067] si5351 0-0060: si5351_msynth_set_rate - ms0: p1 = 5632, p2 = 0, p3 = 1, divby4 = 0, parent_rate = 884736000, rate = 18432000
[    3.080913] si5351 0-0060: si5351_msynth_recalc_rate - ms0: p1 = 5632, p2 = 0, p3 = 1, m = 6144, parent_rate = 884736000, rate = 18432000
[    3.093843] si5351 0-0060: si5351_clkout_set_rate - clk0: rdiv = 1, parent_rate = 18432000, rate = 18432000

[    3.104184] si5351 0-0060: si5351_clkout_round_rate - clk1: rdiv = 1, parent_rate = 8000000, rate = 8000000
[    3.114408] si5351 0-0060: si5351_msynth_round_rate - ms1: a = 112, b = 0, c = 1, divby4 = 0, parent_rate = 896000000, rate = 8000000
[    3.126973] si5351 0-0060: si5351_pll_round_rate - plla: a = 33, b = 37037, c = 200000, parent_rate = 27000000, rate = 895999995
[    3.139085] si5351 0-0060: si5351_msynth_recalc_rate - ms1: p1 = 13824, p2 = 0, p3 = 1, m = 14336, parent_rate = 895999995, rate = 7999999
[    3.155510] si5351 0-0060: si5351_pll_set_rate - plla: p1 = 3735, p2 = 140736, p3 = 200000, parent_rate = 27000000, rate = 895999995
[    3.167993] si5351 0-0060: si5351_pll_recalc_rate - plla: p1 = 3735, p2 = 140736, p3 = 200000, parent_rate = 27000000, rate = 895999995
[    3.182186] si5351 0-0060: si5351_msynth_set_rate - ms1: p1 = 13824, p2 = 0, p3 = 1, divby4 = 0, parent_rate = 895999995, rate = 8000000
[    3.195028] si5351 0-0060: si5351_msynth_recalc_rate - ms1: p1 = 13824, p2 = 0, p3 = 1, m = 14336, parent_rate = 895999995, rate = 7999999
[    3.208046] si5351 0-0060: si5351_clkout_set_rate - clk1: rdiv = 1, parent_rate = 7999999, rate = 8000000

[    3.218150] si5351 0-0060: si5351_clkout_round_rate - clk2: rdiv = 1, parent_rate = 12288000, rate = 12288000
[    3.228544] si5351 0-0060: si5351_msynth_round_rate - ms2: a = 72, b = 0, c = 1, divby4 = 0, parent_rate = 884736000, rate = 12288000
[    3.242565] si5351 0-0060: si5351_msynth_set_rate - ms2: p1 = 8704, p2 = 0, p3 = 1, divby4 = 0, parent_rate = 884736000, rate = 12288000
[    3.255416] si5351 0-0060: si5351_msynth_recalc_rate - ms2: p1 = 8704, p2 = 0, p3 = 1, m = 9216, parent_rate = 884736000, rate = 12288000
[    3.268345] si5351 0-0060: si5351_clkout_set_rate - clk2: rdiv = 1, parent_rate = 12288000, rate = 12288000

Measured frequencies:
clk0 18.432Mhz
clk1 8Mhz
clk2 0Hz

Reported frequencies:
root at som3517-som200:~# head -n 15 /sys/kernel/debug/clk/clk_summary
clock enable_cnt prepare_cnt rate accuracy phase
----------------------------------------------------------------------------------------

              clk2                         0            0    12288000          0 0
              clk0                         0            0    18432000          0 0
              clk1                         0            0     7999999          0 0

It should be noted that if I program the device's register map in the bootloader the device keeps the correct frequency outputs.

So the patch series appears to fix the registration issue but there is still more work to be done.

Still not sure how to explain the difference when DEBUG is defined. I will dig into the datasheet and see what I can find.

Quoted 4 lines · Michael Welling

Quoted 3 lines · Sebastian Hesselbarth

For Si5351 clock driver, Michael Welling and Jean-Francois Moine reported issues with recent v4.x kernels due to broken/missing/wrong parent clock claming. This patch set now deals with the issues reported.

I should have been more precise above,
e.g. s/deals with/deals with some/

[...]

Quoted 3 lines

Okay, the results are in and they are mixed. Firstly the clocks register unlike before. This is a positive step that was certianly expected.

Yes, claiming parent clocks is the main fix.

The pll reset patch should improve stability but datasheet isn't very clear about when to reset pll nor how long it may take at max. Even the Loss-of-Lock check isn't documented but seems to make sense.

Quoted 2 lines

Second the reported and measured clock frequencies do not match the device tree entries.

But generated frequencies do always match reported frequencies.

I striped down your reports the the very essential lines.

Quoted 5 lines

Measured frequencies:
clk0 12.5Mhz
clk1 5.357Mhz
clk2 0 Hz

Quoted 7 lines

Reported frequencies:
root at som3517-som200:~# head -n 15 /sys/kernel/debug/clk/clk_summary
clock enable_cnt prepare_cnt rate accuracy phase
----------------------------------------------------------------------------------------

             clk2                         0            0    12288000          0 0
             clk0                         0            0    12499999          0 0
             clk1                         0            0     5357142          0 0

What I noticed about your clk2 that you always measure as 0 Hz is that none of your clocks is prepared/enabled.

Currently, the si5351 driver only ensures the output is enabled when si5351_clkout_prepare() is called.

As long as you do not have a clk consumer that properly prepare/enables the clock output, it may remain disabled.

We should probably have additional DT properties and corresponding pdata to force clkoutN always on.

Quoted 23 lines

Device tree entry:

        si5351: clock-generator {
                /* connect xtal input to 27MHz reference */
                clocks = <&ref27>;
                clock-names = "xtal";

                clkout0: clkout0 {
                        reg = <0>;
                        clock-frequency = <18432000>;
                };

                clkout1: clkout1 {
                        reg = <1>;
                        clock-frequency = <8000000>;
                };

                clkout2: clkout2 {
                        reg = <2>;
                        clock-frequency = <12288000>;
                };
        };

Quoted 1 line

Lastly if #define DEBUG is added the behavior is different.

Ok, I didn't dig into this. I think I'll rebuild your DT setup above and see if I can reproduce it. It will be different with respect to XTAL frequency, which is 25MHz on CuBox.

Quoted 8 lines

Debugging output:

[    2.970753] si5351 0-0060: si5351_clkout_round_rate - clk0: rdiv = 1, parent_rate = 18432000, rate = 18432000
[    2.981207] si5351 0-0060: si5351_msynth_round_rate - ms0: a = 48, b = 0, c = 1, divby4 = 0, parent_rate = 884736000, rate = 18432000
[    2.993777] si5351 0-0060: si5351_pll_round_rate - pllb: a = 32, b = 96, c = 125, parent_rate = 27000000, rate = 884736000
[    3.005362] si5351 0-0060: si5351_msynth_recalc_rate - ms0: p1 = 5632, p2 = 0, p3 = 1, m = 6144, parent_rate = 884736000, rate = 18432000
[    3.026281] si5351 0-0060: si5351_pll_set_rate - pllb: p1 = 3682, p2 = 38, p3 = 125, parent_rate = 27000000, rate = 884736000
[    3.038151] si5351 0-0060: si5351_pll_recalc_rate - pllb: p1 = 3682, p2 = 38, p3 = 125, parent_rate = 27000000, rate = 884736000
[    3.053933] si5351 0-0060: si5351_msynth_set_rate - ms2: p1 = 0, p2 = 0, p3 = 0, divby4 = 1, parent_rate = 884736000, rate = 884736000

Above ms2 .set_rate() doesn't look good. It is called because ms2 is child of pllb but the .params have not been setup. Usually this is done in si5351_msynth_recalc_rate() but it has not been called yet.

I will probably initialize .params with current register contents on probe().

Quoted 25 lines

[    3.068067] si5351 0-0060: si5351_msynth_set_rate - ms0: p1 = 5632, p2 = 0, p3 = 1, divby4 = 0, parent_rate = 884736000, rate = 18432000
[    3.080913] si5351 0-0060: si5351_msynth_recalc_rate - ms0: p1 = 5632, p2 = 0, p3 = 1, m = 6144, parent_rate = 884736000, rate = 18432000
[    3.093843] si5351 0-0060: si5351_clkout_set_rate - clk0: rdiv = 1, parent_rate = 18432000, rate = 18432000

[    3.104184] si5351 0-0060: si5351_clkout_round_rate - clk1: rdiv = 1, parent_rate = 8000000, rate = 8000000
[    3.114408] si5351 0-0060: si5351_msynth_round_rate - ms1: a = 112, b = 0, c = 1, divby4 = 0, parent_rate = 896000000, rate = 8000000
[    3.126973] si5351 0-0060: si5351_pll_round_rate - plla: a = 33, b = 37037, c = 200000, parent_rate = 27000000, rate = 895999995
[    3.139085] si5351 0-0060: si5351_msynth_recalc_rate - ms1: p1 = 13824, p2 = 0, p3 = 1, m = 14336, parent_rate = 895999995, rate = 7999999
[    3.155510] si5351 0-0060: si5351_pll_set_rate - plla: p1 = 3735, p2 = 140736, p3 = 200000, parent_rate = 27000000, rate = 895999995
[    3.167993] si5351 0-0060: si5351_pll_recalc_rate - plla: p1 = 3735, p2 = 140736, p3 = 200000, parent_rate = 27000000, rate = 895999995
[    3.182186] si5351 0-0060: si5351_msynth_set_rate - ms1: p1 = 13824, p2 = 0, p3 = 1, divby4 = 0, parent_rate = 895999995, rate = 8000000
[    3.195028] si5351 0-0060: si5351_msynth_recalc_rate - ms1: p1 = 13824, p2 = 0, p3 = 1, m = 14336, parent_rate = 895999995, rate = 7999999
[    3.208046] si5351 0-0060: si5351_clkout_set_rate - clk1: rdiv = 1, parent_rate = 7999999, rate = 8000000

[    3.218150] si5351 0-0060: si5351_clkout_round_rate - clk2: rdiv = 1, parent_rate = 12288000, rate = 12288000
[    3.228544] si5351 0-0060: si5351_msynth_round_rate - ms2: a = 72, b = 0, c = 1, divby4 = 0, parent_rate = 884736000, rate = 12288000
[    3.242565] si5351 0-0060: si5351_msynth_set_rate - ms2: p1 = 8704, p2 = 0, p3 = 1, divby4 = 0, parent_rate = 884736000, rate = 12288000
[    3.255416] si5351 0-0060: si5351_msynth_recalc_rate - ms2: p1 = 8704, p2 = 0, p3 = 1, m = 9216, parent_rate = 884736000, rate = 12288000
[    3.268345] si5351 0-0060: si5351_clkout_set_rate - clk2: rdiv = 1, parent_rate = 12288000, rate = 12288000

Measured frequencies:
clk0 18.432Mhz
clk1 8Mhz
clk2 0Hz

Quoted 7 lines

Reported frequencies:
root at som3517-som200:~# head -n 15 /sys/kernel/debug/clk/clk_summary
clock enable_cnt prepare_cnt rate accuracy phase
----------------------------------------------------------------------------------------

             clk2                         0            0    12288000          0 0
             clk0                         0            0    18432000          0 0
             clk1                         0            0     7999999          0 0

Here again, reported frequencies and measured frequencies look quite good. Why the requested frequencies differ when enabling DEBUG, I'll have to have a closer look.

Quoted 2 lines

It should be noted that if I program the device's register map in the bootloader the device keeps the correct frequency outputs.

"keeps"? You mean "generates", don't you?

Quoted 2 lines

So the patch series appears to fix the registration issue but there is still more work to be done.

Yep. And your testing on a different setup definitely helps.

Quoted 2 lines

Still not sure how to explain the difference when DEBUG is defined. I will dig into the datasheet and see what I can find.

Me neither.

Sebastian

On Thu, Apr 30, 2015 at 10:44:07PM +0200, Sebastian Hesselbarth wrote:

Quoted 6 lines

What I noticed about your clk2 that you always measure as 0 Hz is that none of your clocks is prepared/enabled.

Currently, the si5351 driver only ensures the output is enabled when si5351_clkout_prepare() is called.

Quoted 5 lines

As long as you do not have a clk consumer that properly prepare/enables the clock output, it may remain disabled.

We should probably have additional DT properties and corresponding pdata to force clkoutN always on.

Does the silabs,disable-state of 3 (SI5351_DISABLE_NEVER) take care of this?

Otherwise is there a simple registration that will do this?

Quoted 5 lines

Quoted 2 lines

It should be noted that if I program the device's register map in the bootloader the device keeps the correct frequency outputs.

"keeps"? You mean "generates", don't you?

Yes the clocks are generated and do not get effected by the driver.

Quoted 1 line · Michael Welling

On Thu, Apr 30, 2015 at 10:44:07PM +0200, Sebastian Hesselbarth wrote:

[...]

Quoted 9 lines

Quoted 9 lines

What I noticed about your clk2 that you always measure as 0 Hz is that none of your clocks is prepared/enabled.

Currently, the si5351 driver only ensures the output is enabled when si5351_clkout_prepare() is called.

As long as you do not have a clk consumer that properly prepare/enables the clock output, it may remain disabled.

Quoted 5 lines

Quoted 2 lines

We should probably have additional DT properties and corresponding pdata to force clkoutN always on.

Does the silabs,disable-state of 3 (SI5351_DISABLE_NEVER) take care of this?

That would be the HW version of never disabling the clock output. I never really tried the property, does it work as expected?

Quoted 2 lines

Otherwise is there a simple registration that will do this?

The SW version of such a property would involve CLK_IGNORE_UNUSED and enabling all requested clock outputs on probe().

If above HW property already works, I think it should be enough.

[...]

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

It should be noted that if I program the device's register map in the bootloader the device keeps the correct frequency outputs.

Quoted 4 lines

Quoted 2 lines

"keeps"? You mean "generates", don't you?

Yes the clocks are generated and do not get effected by the driver.

IIRC, clk API does check if requested rate and current rate match already. If they do, it does not request the same rate again.

Sebastian

Quoted 15 lines · Sebastian Hesselbarth

Quoted 1 line · Michael Welling

On Thu, Apr 30, 2015 at 10:44:07PM +0200, Sebastian Hesselbarth wrote:

[...]

Quoted 12 lines

Quoted 11 lines

What I noticed about your clk2 that you always measure as 0 Hz is that none of your clocks is prepared/enabled.

Currently, the si5351 driver only ensures the output is enabled when si5351_clkout_prepare() is called.

As long as you do not have a clk consumer that properly prepare/enables the clock output, it may remain disabled.

We should probably have additional DT properties and corresponding pdata to force clkoutN always on.

Quoted 5 lines

Quoted 2 lines

Does the silabs,disable-state of 3 (SI5351_DISABLE_NEVER) take care of this?

That would be the HW version of never disabling the clock output. I never really tried the property, does it work as expected?

This did not appear to effect the behavior.

Quoted 12 lines

Quoted 2 lines

Otherwise is there a simple registration that will do this?

The SW version of such a property would involve CLK_IGNORE_UNUSED and enabling all requested clock outputs on probe().

If above HW property already works, I think it should be enough.

[...]

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

It should be noted that if I program the device's register map in the bootloader the device keeps the correct frequency outputs.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

"keeps"? You mean "generates", don't you?

Yes the clocks are generated and do not get effected by the driver.

IIRC, clk API does check if requested rate and current rate match already. If they do, it does not request the same rate again.

So I found that the audio codec that I am driving with clk2 could register the clock and allowed the clock to be enabled and disabled by playing audio.

This is when I noticed some strange behavior. The first time I attempt to play audio the clock does not turn on blocking the audio from playing. After I interrupt and the clock is disabled for the first time, the successive clock enables work as expected.

Something tells me that a fault off some kind is occurring on initial configuration.

Quoted 16 lines · Michael Welling

Quoted 15 lines · Sebastian Hesselbarth

Quoted 1 line · Michael Welling

On Thu, Apr 30, 2015 at 10:44:07PM +0200, Sebastian Hesselbarth wrote:

[...]

Quoted 12 lines

Quoted 11 lines

What I noticed about your clk2 that you always measure as 0 Hz is that none of your clocks is prepared/enabled.

Currently, the si5351 driver only ensures the output is enabled when si5351_clkout_prepare() is called.

As long as you do not have a clk consumer that properly prepare/enables the clock output, it may remain disabled.

We should probably have additional DT properties and corresponding pdata to force clkoutN always on.

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

Does the silabs,disable-state of 3 (SI5351_DISABLE_NEVER) take care of this?

That would be the HW version of never disabling the clock output. I never really tried the property, does it work as expected?

This did not appear to effect the behavior.

I think it is also a good idea to expose register values to debugfs, so we can easily check what is really written into internal registers.

Quoted 24 lines

Quoted 12 lines

Quoted 2 lines

Otherwise is there a simple registration that will do this?

The SW version of such a property would involve CLK_IGNORE_UNUSED and enabling all requested clock outputs on probe().

If above HW property already works, I think it should be enough.

[...]

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

It should be noted that if I program the device's register map in the bootloader the device keeps the correct frequency outputs.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

"keeps"? You mean "generates", don't you?

Yes the clocks are generated and do not get effected by the driver.

IIRC, clk API does check if requested rate and current rate match already. If they do, it does not request the same rate again.

So I found that the audio codec that I am driving with clk2 could register the clock and allowed the clock to be enabled and disabled by playing audio.

Quoted 4 lines

This is when I noticed some strange behavior. The first time I attempt to play audio the clock does not turn on blocking the audio from playing. After I interrupt and the clock is disabled for the first time, the successive clock enables work as expected.

Does "does not turn on" mean you cannot measure any clock on the output or is it just a guess because audio does not play?

It could be that we just need to add some delay when we enable a clock output. Datasheet just says 10us max from OEB pin pulled low to valid clock output - not exactly what we are looking for but it could be a good start.

Quoted 2 lines

Something tells me that a fault off some kind is occurring on initial configuration.

What I noticed when adding the pll reset and checking DEVICE_STATUS is that SYS_INIT is still set. According to the datasheet, the meaning of the bit is that si5351 is still copying NVM content to its internal registers and therefore, we shouldn't try to access them.

What really irritates me about it is that it is seconds after power-up and copying the contents shouldn't really take _that_ long. However, the datasheet does not mention anything about how long it may take.

Sebastian

For Si5351 clock driver, Michael Welling and Jean-Francois Moine reported issues with recent v4.x kernels due to broken/missing/wrong parent clock claming. This patch set now deals with the issues reported.

Patch 1 amends the binding documentation mention clock-names property for the "xtal" and "clkin" parent clock inputs of Si5351 variants.

Patch 2 adds the clock-names property for the SolidRun CuBox using Si5351 with a fixed oscillator connected to "xtal" input.

Patch 3 reworks the way we claim parent clocks by using devm_clk_get() for both DT and platform_data based registration. Also, properly check for errors returned by devm_clk_get() and prepare/enable the parent clocks.

Patch 4 introduces a function to reset PLLs on rate change. This should improve generated clock output stability. I currently have no scope at hand to actually test that properly, so there may be more issues remaining.

@Michael, Jean-Francois: Please test and report if there are still issues remaining.

Quoted 13 lines · On Thu, 30 Apr 2015 19:45:50 +0200 Sebastian Hesselbarth…

For Si5351 clock driver, Michael Welling and Jean-Francois Moine reported issues with recent v4.x kernels due to broken/missing/wrong parent clock claming. This patch set now deals with the issues reported.

Patch 1 amends the binding documentation mention clock-names property for the "xtal" and "clkin" parent clock inputs of Si5351 variants.

Patch 2 adds the clock-names property for the SolidRun CuBox using Si5351 with a fixed oscillator connected to "xtal" input.

Patch 3 reworks the way we claim parent clocks by using devm_clk_get() for both DT and platform_data based registration. Also, properly check for errors returned by devm_clk_get() and prepare/enable the parent clocks.

I applied the patches 2 and 3, and the audio and video in the Cubox work fine again. Thanks.

Quoted 2 lines

I applied the patches 2 and 3, and the audio and video in the Cubox work fine again. Thanks.

Ok, good. You mentioned that on v3.19-rc1 it still "works" i.e. despite the broken/missing clk_get/clk_prepare_enable?

Can you check the stable (v3.19, v4.0) versions and see how far we should backport the fix?

Quoted 2 lines

I applied the patches 2 and 3, and the audio and video in the Cubox work fine again. Thanks.

Ok, good. You mentioned that on v3.19-rc1 it still "works" i.e. despite the broken/missing clk_get/clk_prepare_enable?

Can you check the stable (v3.19, v4.0) versions and see how far we should backport the fix?