[PATCH 0/4] clk: si5351: Some fixes

[PATCH 0/4] clk: si5351: Some fixes

[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.

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 <mturquette@linaro.org>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Jean-Francois Moine <moinejf@free.fr>
Cc: Michael Welling <mwelling@ieee.org>
Cc: Russell King <rmk+linux@arm.linux.org.uk>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: devicetree@vger.kernel.org
Cc: linux-clk@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
-- 
2.1.0

[PATCH 1/4] clk: si5351: Mention clock-names in the binding documentation

[Sebastian Hesselbarth]

Since the introduction of clk-si5351 the way we should deal with DT provided
clocks has changed from indexed to named clock phandles. Amend the binding
documentation to reflect named clock phandles by clock-names property.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: Mike Turquette <mturquette@linaro.org>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Jean-Francois Moine <moinejf@free.fr>
Cc: Michael Welling <mwelling@ieee.org>
Cc: Russell King <rmk+linux@arm.linux.org.uk>
Cc: devicetree@vger.kernel.org
Cc: linux-clk@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 Documentation/devicetree/bindings/clock/silabs,si5351.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/clock/silabs,si5351.txt b/Documentation/devicetree/bindings/clock/silabs,si5351.txt
index c40711e8e8f7..28b28309f535 100644
--- a/Documentation/devicetree/bindings/clock/silabs,si5351.txt
+++ b/Documentation/devicetree/bindings/clock/silabs,si5351.txt
@@ -17,7 +17,8 @@ Required properties:
 - #clock-cells: from common clock binding; shall be set to 1.
 - clocks: from common clock binding; list of parent clock
   handles, shall be xtal reference clock or xtal and clkin for
-  si5351c only.
+  si5351c only. Corresponding clock input names are "xtal" and
+  "clkin" respectively.
 - #address-cells: shall be set to 1.
 - #size-cells: shall be set to 0.
 
@@ -71,6 +72,7 @@ i2c-master-node {
 
 		/* connect xtal input to 25MHz reference */
 		clocks = <&ref25>;
+		clock-names = "xtal";
 
 		/* connect xtal input as source of pll0 and pll1 */
 		silabs,pll-source = <0 0>, <1 0>;
-- 
2.1.0

[PATCH 2/4] ARM: dove: Add clock-names to CuBox Si5351 clk generator

[Sebastian Hesselbarth]

Si5351 clock generator on CuBox uses XTAL as clock reference, name the
clock phandle accordingly.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: Mike Turquette <mturquette@linaro.org>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Jean-Francois Moine <moinejf@free.fr>
Cc: Michael Welling <mwelling@ieee.org>
Cc: Russell King <rmk+linux@arm.linux.org.uk>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: devicetree@vger.kernel.org
Cc: linux-clk@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 arch/arm/boot/dts/dove-cubox.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/dove-cubox.dts b/arch/arm/boot/dts/dove-cubox.dts
index aae7efc09b0b..e6fa251e17b9 100644
--- a/arch/arm/boot/dts/dove-cubox.dts
+++ b/arch/arm/boot/dts/dove-cubox.dts
@@ -87,6 +87,7 @@
 
 		/* connect xtal input to 25MHz reference */
 		clocks = <&ref25>;
+		clock-names = "xtal";
 
 		/* connect xtal input as source of pll0 and pll1 */
 		silabs,pll-source = <0 0>, <1 0>;
-- 
2.1.0

[PATCH 3/4] clk: si5351: Do not pass struct clk in platform_data

[Sebastian Hesselbarth]

When registering clk-si5351 by platform_data, we should not pass struct clk
for the reference clocks. Drop struct clk from platform_data and rework the
driver to use devm_clk_get of named clock references.

While at it, check for at least one valid input clock and properly prepare/
enable valid reference clocks.

Reported-by: Michael Welling <mwelling@ieee.org>
Reported-by: Jean-Francois Moine <moinejf@free.fr>
Reported-by: Russell King <rmk+linux@arm.linux.org.uk>
Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: Mike Turquette <mturquette@linaro.org>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Jean-Francois Moine <moinejf@free.fr>
Cc: Michael Welling <mwelling@ieee.org>
Cc: Russell King <rmk+linux@arm.linux.org.uk>
Cc: linux-clk@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/clk/clk-si5351.c             | 58 +++++++++++++++++++++++++-----------
 include/linux/platform_data/si5351.h |  4 ---
 2 files changed, 40 insertions(+), 22 deletions(-)

diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
index 44ea107cfc67..beeb57bbb04c 100644
--- a/drivers/clk/clk-si5351.c
+++ b/drivers/clk/clk-si5351.c
@@ -1128,13 +1128,6 @@ static int si5351_dt_parse(struct i2c_client *client,
 	if (!pdata)
 		return -ENOMEM;
 
-	pdata->clk_xtal = of_clk_get(np, 0);
-	if (!IS_ERR(pdata->clk_xtal))
-		clk_put(pdata->clk_xtal);
-	pdata->clk_clkin = of_clk_get(np, 1);
-	if (!IS_ERR(pdata->clk_clkin))
-		clk_put(pdata->clk_clkin);
-
 	/*
 	 * property silabs,pll-source : <num src>, [<..>]
 	 * allow to selectively set pll source
@@ -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");
+
+	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)) {
+		dev_err(&client->dev, "missing at least one parent clock\n");
+		return -EINVAL;
+	}
 
 	drvdata->regmap = devm_regmap_init_i2c(client, &si5351_regmap_config);
 	if (IS_ERR(drvdata->regmap)) {
@@ -1393,6 +1395,11 @@ static int si5351_i2c_probe(struct i2c_client *client,
 		}
 	}
 
+	if (!IS_ERR(drvdata->pxtal))
+		clk_prepare_enable(drvdata->pxtal);
+	if (!IS_ERR(drvdata->pclkin))
+		clk_prepare_enable(drvdata->pclkin);
+
 	/* register xtal input clock gate */
 	memset(&init, 0, sizeof(init));
 	init.name = si5351_input_names[0];
@@ -1407,7 +1414,8 @@ static int si5351_i2c_probe(struct i2c_client *client,
 	clk = devm_clk_register(&client->dev, &drvdata->xtal);
 	if (IS_ERR(clk)) {
 		dev_err(&client->dev, "unable to register %s\n", init.name);
-		return PTR_ERR(clk);
+		ret = PTR_ERR(clk);
+		goto err_clk;
 	}
 
 	/* register clkin input clock gate */
@@ -1425,7 +1433,8 @@ static int si5351_i2c_probe(struct i2c_client *client,
 		if (IS_ERR(clk)) {
 			dev_err(&client->dev, "unable to register %s\n",
 				init.name);
-			return PTR_ERR(clk);
+			ret = PTR_ERR(clk);
+			goto err_clk;
 		}
 	}
 
@@ -1447,7 +1456,8 @@ static int si5351_i2c_probe(struct i2c_client *client,
 	clk = devm_clk_register(&client->dev, &drvdata->pll[0].hw);
 	if (IS_ERR(clk)) {
 		dev_err(&client->dev, "unable to register %s\n", init.name);
-		return -EINVAL;
+		ret = PTR_ERR(clk);
+		goto err_clk;
 	}
 
 	/* register PLLB or VXCO (Si5351B) */
@@ -1471,7 +1481,8 @@ static int si5351_i2c_probe(struct i2c_client *client,
 	clk = devm_clk_register(&client->dev, &drvdata->pll[1].hw);
 	if (IS_ERR(clk)) {
 		dev_err(&client->dev, "unable to register %s\n", init.name);
-		return -EINVAL;
+		ret = PTR_ERR(clk);
+		goto err_clk;
 	}
 
 	/* register clk multisync and clk out divider */
@@ -1492,8 +1503,10 @@ static int si5351_i2c_probe(struct i2c_client *client,
 		num_clocks * sizeof(*drvdata->onecell.clks), GFP_KERNEL);
 
 	if (WARN_ON(!drvdata->msynth || !drvdata->clkout ||
-		    !drvdata->onecell.clks))
-		return -ENOMEM;
+		    !drvdata->onecell.clks)) {
+		ret = -ENOMEM;
+		goto err_clk;
+	}
 
 	for (n = 0; n < num_clocks; n++) {
 		drvdata->msynth[n].num = n;
@@ -1511,7 +1524,8 @@ static int si5351_i2c_probe(struct i2c_client *client,
 		if (IS_ERR(clk)) {
 			dev_err(&client->dev, "unable to register %s\n",
 				init.name);
-			return -EINVAL;
+			ret = PTR_ERR(clk);
+			goto err_clk;
 		}
 	}
 
@@ -1538,7 +1552,8 @@ static int si5351_i2c_probe(struct i2c_client *client,
 		if (IS_ERR(clk)) {
 			dev_err(&client->dev, "unable to register %s\n",
 				init.name);
-			return -EINVAL;
+			ret = PTR_ERR(clk);
+			goto err_clk;
 		}
 		drvdata->onecell.clks[n] = clk;
 
@@ -1557,10 +1572,17 @@ static int si5351_i2c_probe(struct i2c_client *client,
 				  &drvdata->onecell);
 	if (ret) {
 		dev_err(&client->dev, "unable to add clk provider\n");
-		return ret;
+		goto err_clk;
 	}
 
 	return 0;
+
+err_clk:
+	if (!IS_ERR(drvdata->pxtal))
+		clk_disable_unprepare(drvdata->pxtal);
+	if (!IS_ERR(drvdata->pclkin))
+		clk_disable_unprepare(drvdata->pclkin);
+	return ret;
 }
 
 static const struct i2c_device_id si5351_i2c_ids[] = {
diff --git a/include/linux/platform_data/si5351.h b/include/linux/platform_data/si5351.h
index a947ab8b441a..533d9807e543 100644
--- a/include/linux/platform_data/si5351.h
+++ b/include/linux/platform_data/si5351.h
@@ -5,8 +5,6 @@
 #ifndef __LINUX_PLATFORM_DATA_SI5351_H__
 #define __LINUX_PLATFORM_DATA_SI5351_H__
 
-struct clk;
-
 /**
  * enum si5351_pll_src - Si5351 pll clock source
  * @SI5351_PLL_SRC_DEFAULT: default, do not change eeprom config
@@ -107,8 +105,6 @@ struct si5351_clkout_config {
  * @clkout: array of clkout configuration
  */
 struct si5351_platform_data {
-	struct clk *clk_xtal;
-	struct clk *clk_clkin;
 	enum si5351_pll_src pll_src[2];
 	struct si5351_clkout_config clkout[8];
 };
-- 
2.1.0

[PATCH 4/4] clk: si5351: Reset PLL after rate change

[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 <sebastian.hesselbarth@gmail.com>
---
Cc: Mike Turquette <mturquette@linaro.org>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Jean-Francois Moine <moinejf@free.fr>
Cc: Michael Welling <mwelling@ieee.org>
Cc: Russell King <rmk+linux@arm.linux.org.uk>
Cc: devicetree@vger.kernel.org
Cc: linux-clk@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/clk/clk-si5351.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
index beeb57bbb04c..9b97c134e3c1 100644
--- a/drivers/clk/clk-si5351.c
+++ b/drivers/clk/clk-si5351.c
@@ -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)
 {
@@ -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);
+
 	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),
-- 
2.1.0

Re: [PATCH 3/4] clk: si5351: Do not pass struct clk in platform_data

[Fabio Estevam]

On Thu, Apr 30, 2015 at 2:45 PM, Sebastian Hesselbarth
<sebastian.hesselbarth@gmail.com> wrote:

>          * property silabs,pll-source : <num src>, [<..>]
>          * allow to selectively set pll source
> @@ -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");
> +
> +       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?

Re: [PATCH 3/4] clk: si5351: Do not pass struct clk in platform_data

[Michael Welling]

On Thu, Apr 30, 2015 at 03:20:38PM -0300, Fabio Estevam wrote:
> On Thu, Apr 30, 2015 at 2:45 PM, Sebastian Hesselbarth
> <sebastian.hesselbarth@gmail.com> wrote:
> 
> >          * property silabs,pll-source : <num src>, [<..>]
> >          * allow to selectively set pll source
> > @@ -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");
> > +
> > +       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.

Re: [PATCH 3/4] clk: si5351: Do not pass struct clk in platform_data

[Sebastian Hesselbarth]

On 30.04.2015 20:30, Michael Welling wrote:
> On Thu, Apr 30, 2015 at 03:20:38PM -0300, Fabio Estevam wrote:
>> On Thu, Apr 30, 2015 at 2:45 PM, Sebastian Hesselbarth
>> <sebastian.hesselbarth@gmail.com> wrote:
>>> @@ -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");
>>> +
>>> +       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.

But thanks for looking at it and I appreciate the review.

Sebastian

Re: [PATCH 0/4] clk: si5351: Some fixes

[Michael Welling]

On Thu, Apr 30, 2015 at 07:45:50PM +0200, Sebastian Hesselbarth wrote:
> 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.

I am in the process of testing this patch series.

Hopefully this fixes some issues.

> 
> 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 <mturquette@linaro.org>
> Cc: Stephen Boyd <sboyd@codeaurora.org>
> Cc: Jean-Francois Moine <moinejf@free.fr>
> Cc: Michael Welling <mwelling@ieee.org>
> Cc: Russell King <rmk+linux@arm.linux.org.uk>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Gregory Clement <gregory.clement@free-electrons.com>
> Cc: devicetree@vger.kernel.org
> Cc: linux-clk@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> -- 
> 2.1.0
> 

Re: [PATCH 4/4] clk: si5351: Reset PLL after rate change

[Michael Welling]

On Thu, Apr 30, 2015 at 07:45:54PM +0200, Sebastian Hesselbarth wrote:
> 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 <sebastian.hesselbarth@gmail.com>
> ---
> Cc: Mike Turquette <mturquette@linaro.org>
> Cc: Stephen Boyd <sboyd@codeaurora.org>
> Cc: Jean-Francois Moine <moinejf@free.fr>
> Cc: Michael Welling <mwelling@ieee.org>
> Cc: Russell King <rmk+linux@arm.linux.org.uk>
> Cc: devicetree@vger.kernel.org
> Cc: linux-clk@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/clk/clk-si5351.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
> index beeb57bbb04c..9b97c134e3c1 100644
> --- a/drivers/clk/clk-si5351.c
> +++ b/drivers/clk/clk-si5351.c
> @@ -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)
>  {
> @@ -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?

>  	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),
> -- 
> 2.1.0
> 

Re: [PATCH 4/4] clk: si5351: Reset PLL after rate change

[Sebastian Hesselbarth]

On 30.04.2015 20:49, Michael Welling wrote:
> On Thu, Apr 30, 2015 at 07:45:54PM +0200, Sebastian Hesselbarth wrote:
>> 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 <sebastian.hesselbarth@gmail.com>
>> ---
[...]
>> @@ -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?

Good point, I'll pass that on to the return value of .set_rate()
callback for v2.

Sebastian

Re: [PATCH 3/4] clk: si5351: Do not pass struct clk in platform_data

[Fabio Estevam]

Hi Sebastian,

On Thu, Apr 30, 2015 at 3:44 PM, Sebastian Hesselbarth
<sebastian.hesselbarth@gmail.com> wrote:

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

+       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" ?

Regards,

Fabio Estevam

Re: [PATCH 0/4] clk: si5351: Some fixes

[Michael Welling]

On Thu, Apr 30, 2015 at 07:45:50PM +0200, Sebastian Hesselbarth wrote:
> 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.
>

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@som3517-som200:~# head -n 15 /sys/kernel/debug/clk/clk_summary 
   clock                         enable_cnt  prepare_cnt        rate   accuracy   phase
----------------------------------------------------------------------------------------
 ref27                                    1            1    27000000          0 0  
    xtal                                  0            0    27000000          0 0  
       pllb                               0            0   599999994          0 0  
          ms2                             0            0    12288000          0 0  
             clk2                         0            0    12288000          0 0  
          ms0                             0            0    12499999          0 0  
             clk0                         0            0    12499999          0 0  
       plla                               0            0   599999994          0 0  
          ms1                             0            0     5357142          0 0  
             clk1                         0            0     5357142          0 0  
 tclkin_ck                                0            0    12000000          0 0  
 virt_26000000_ck                         1            1    26000000          0 0  
    sys_clkin_ck                         11           23    26000000          0 0 

Device tree entry:
        si5351: clock-generator {
                #address-cells = <1>;
                #size-cells = <0>;
                #clock-cells = <1>;
                compatible = "silabs,si5351a-msop";
                reg = <0x60>;
                status = "okay";

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

                /* connect xtal input as source of pll0 and pll1 */
                silabs,pll-source = <0 0>, <1 0>;

                clkout0: clkout0 {
                        reg = <0>;
                        silabs,drive-strength = <8>;
                        silabs,multisynth-source = <1>;
                        silabs,clock-source = <0>;
                        silabs,pll-master;
                        clock-frequency = <18432000>;
                 };

                clkout1: clkout1 {
                        reg = <1>;
                        silabs,drive-strength = <8>;
                        silabs,multisynth-source = <0>;
                        silabs,clock-source = <0>;
                        silabs,pll-master;
                        clock-frequency = <8000000>;
                };

                clkout2: clkout2 {
                        reg = <2>;
                        silabs,drive-strength = <8>;
                        silabs,multisynth-source = <1>;
                        silabs,clock-source = <0>;
                        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@som3517-som200:~# head -n 15 /sys/kernel/debug/clk/clk_summary
   clock                         enable_cnt  prepare_cnt        rate   accuracy   phase
----------------------------------------------------------------------------------------
 ref27                                    1            1    27000000          0 0  
    xtal                                  0            0    27000000          0 0  
       pllb                               0            0   884736000          0 0  
          ms2                             0            0    12288000          0 0  
             clk2                         0            0    12288000          0 0  
          ms0                             0            0    18432000          0 0  
             clk0                         0            0    18432000          0 0  
       plla                               0            0   895999995          0 0  
          ms1                             0            0     7999999          0 0  
             clk1                         0            0     7999999          0 0  
 tclkin_ck                                0            0    12000000          0 0  
 virt_26000000_ck                         1            1    26000000          0 0  
    sys_clkin_ck                         11           23    26000000          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.

> 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 <mturquette@linaro.org>
> Cc: Stephen Boyd <sboyd@codeaurora.org>
> Cc: Jean-Francois Moine <moinejf@free.fr>
> Cc: Michael Welling <mwelling@ieee.org>
> Cc: Russell King <rmk+linux@arm.linux.org.uk>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Gregory Clement <gregory.clement@free-electrons.com>
> Cc: devicetree@vger.kernel.org
> Cc: linux-clk@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> -- 
> 2.1.0
> 

Re: [PATCH 0/4] clk: si5351: Some fixes

[Sebastian Hesselbarth]

On 30.04.2015 21:33, Michael Welling wrote:
> On Thu, Apr 30, 2015 at 07:45:50PM +0200, Sebastian Hesselbarth wrote:
>> 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/

[...]
>
> 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.

> 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.

> Measured frequencies:
> clk0 12.5Mhz
> clk1 5.357Mhz
> clk2 0 Hz
>
> Reported frequencies:
> root@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.

> 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.

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.

> 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().

> [    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@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.

> 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?

> 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.

> 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

Re: [PATCH 3/4] clk: si5351: Do not pass struct clk in platform_data

[Sebastian Hesselbarth]

On 30.04.2015 21:16, Fabio Estevam wrote:
> On Thu, Apr 30, 2015 at 3:44 PM, Sebastian Hesselbarth
> <sebastian.hesselbarth@gmail.com> wrote:
>> Michael is right, the check is for bailing out if none of the parent
>> clocks is available.
>
> +       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" ?

Yeah, probably. I'll reword the error message and also check the
variant as only 5351C can have a "clkin" parent clock.

Sebastian

Re: [PATCH 0/4] clk: si5351: Some fixes

[Michael Welling]

On Thu, Apr 30, 2015 at 10:44:07PM +0200, Sebastian Hesselbarth wrote:
> On 30.04.2015 21:33, Michael Welling wrote:
> >On Thu, Apr 30, 2015 at 07:45:50PM +0200, Sebastian Hesselbarth wrote:
> >>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/
> 
> [...]
> >
> >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.
> 
> >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.
> 
> >Measured frequencies:
> >clk0 12.5Mhz
> >clk1 5.357Mhz
> >clk2 0 Hz
> >
> >Reported frequencies:
> >root@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.

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

Otherwise is there a simple registration that will do this?

> 
> >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.
> 
> 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.
> 
> >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().
> 
> >[    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@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.
> 
> >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.
 
> >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.
> 

I have been battling this chip for a while now as it is on a few
different products I have dealt with.

> >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
> 

Re: [PATCH 0/4] clk: si5351: Some fixes

[Sebastian Hesselbarth]

On 30.04.2015 23:20, Michael Welling wrote:
> On Thu, Apr 30, 2015 at 10:44:07PM +0200, Sebastian Hesselbarth wrote:
[...]
>> 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.
>
> 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?

> 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.

[...]
>>> 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.

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

Re: [PATCH 0/4] clk: si5351: Some fixes

[Michael Welling]

On Fri, May 01, 2015 at 12:21:20AM +0200, Sebastian Hesselbarth wrote:
> On 30.04.2015 23:20, Michael Welling wrote:
> >On Thu, Apr 30, 2015 at 10:44:07PM +0200, Sebastian Hesselbarth wrote:
> [...]
> >>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.
> >
> >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.

> 
> >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.
> 
> [...]
> >>>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.
> 
> 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.

> Sebastian
> 

Re: [PATCH 0/4] clk: si5351: Some fixes

[Sebastian Hesselbarth]

On 01.05.2015 00:36, Michael Welling wrote:
> On Fri, May 01, 2015 at 12:21:20AM +0200, Sebastian Hesselbarth wrote:
>> On 30.04.2015 23:20, Michael Welling wrote:
>>> On Thu, Apr 30, 2015 at 10:44:07PM +0200, Sebastian Hesselbarth wrote:
>> [...]
>>>> 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.
>>>
>>> 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.

>>
>>> 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.
>>
>> [...]
>>>>> 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.
>>
>> 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.

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.

> 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

Re: [PATCH 0/4] clk: si5351: Some fixes

[Jean-Francois Moine]

On Thu, 30 Apr 2015 19:45:50 +0200
Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote:

> 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.

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

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH 0/4] clk: si5351: Some fixes

[Sebastian Hesselbarth]

On 01.05.2015 11:14, Jean-Francois Moine wrote:
> On Thu, 30 Apr 2015 19:45:50 +0200
> Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote:
>> 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.

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?

Sebastian

Re: [PATCH 0/4] clk: si5351: Some fixes

[Jean-Francois Moine]

On Fri, 01 May 2015 11:30:18 +0200
Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote:

> > 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?

v3.19 works, v4.0 does not.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH 0/4] clk: si5351: Some fixes

[Michael Welling]

On Fri, May 01, 2015 at 10:17:35AM +0200, Sebastian Hesselbarth wrote:
> On 01.05.2015 00:36, Michael Welling wrote:
> >On Fri, May 01, 2015 at 12:21:20AM +0200, Sebastian Hesselbarth wrote:
> >>On 30.04.2015 23:20, Michael Welling wrote:
> >>>On Thu, Apr 30, 2015 at 10:44:07PM +0200, Sebastian Hesselbarth wrote:
> >>[...]
> >>>>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.
> >>>
> >>>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.
> 
> >>
> >>>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.
> >>
> >>[...]
> >>>>>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.
> >>
> >>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.
> 
> 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?

The clock is stuck at around 2 volts until the first clk_disable.
When the clock is disabled it drops to GND. Future clk_enables make the
clock come at the reported frequency.

As for the clock being initialized incorrectly, I selectively added
mdelay(100) at each dev_dbg with debugging disabled until the 
initialization came up correct.

I found that adding a delay at the end of si5351_msynth_round_rate
appears to be magically fixing the incorrect frequency settings.
Not sure if this sheds any light on the issue but I figured I would 
share this information.

> 
> 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.
> 
> >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
>