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: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
--
2.1.0
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 <[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: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
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
Si5351 clock generator on CuBox uses XTAL as clock reference, name the
clock phandle accordingly.
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: Jason Cooper <[email protected]>
Cc: Andrew Lunn <[email protected]>
Cc: Gregory Clement <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
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
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 <[email protected]>
Reported-by: Jean-Francois Moine <[email protected]>
Reported-by: Russell King <[email protected]>
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: [email protected]
Cc: [email protected]
Cc: [email protected]
---
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
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: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
---
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
On Thu, Apr 30, 2015 at 2:45 PM, Sebastian Hesselbarth
<[email protected]> 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?
On Thu, Apr 30, 2015 at 03:20:38PM -0300, Fabio Estevam wrote:
> On Thu, Apr 30, 2015 at 2:45 PM, Sebastian Hesselbarth
> <[email protected]> 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.
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
>> <[email protected]> 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
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 <[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: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> --
> 2.1.0
>
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 <[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: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
> 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
>
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 <[email protected]>
>> ---
[...]
>> @@ -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
Hi Sebastian,
On Thu, Apr 30, 2015 at 3:44 PM, Sebastian Hesselbarth
<[email protected]> 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
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 <[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: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> --
> 2.1.0
>
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
On 30.04.2015 21:16, Fabio Estevam wrote:
> On Thu, Apr 30, 2015 at 3:44 PM, Sebastian Hesselbarth
> <[email protected]> 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
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
>
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
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
>
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
On Thu, 30 Apr 2015 19:45:50 +0200
Sebastian Hesselbarth <[email protected]> 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/
On 01.05.2015 11:14, Jean-Francois Moine wrote:
> On Thu, 30 Apr 2015 19:45:50 +0200
> Sebastian Hesselbarth <[email protected]> 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
On Fri, 01 May 2015 11:30:18 +0200
Sebastian Hesselbarth <[email protected]> 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/
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
>