Mike, Stephen,
this is v2 of dealing with recently reported clk-si5351 issues. Compared
to v1 [1], I decided to first split-off the pure fixes and postpone the
improvements to a later patch set. Patches 1 and 3 should go though clk
fixes while patch 2 should be taken by mvebu maintainers.
This fixes are intended for v4.1-rc1 and deal with issues where DT claimed
parent clocks are not properly detected starting with v4.0. Looking at the
code of clk-si5351, I just realized that the way the driver deals with
parent clocks is utter nonsense. Russell King also mentioned that passing
struct clk though platform_data shouldn't be done at all.
Therefore, this 3 patches rework parent clock handling of clk-si5351 to
make use of (a) named parent clocks, (b) devm_clk_get() for both DT- and
platform_data passed parent clocks, and (c) properly clk_prepare_enable()
valid parent clocks.
However, I do have a stripped down backport of the fix for stable v4.0
that just fixes the real issue, i.e. DT based probing and missing
clk_prepare_enable(). If you agree with this fixes for v4.1-rc1 we should
consider the backport for v4.0 stable.
Overall changes compared to v1:
- Postpone improvement patch ("clk: si5351: Reset PLL after rate change")
for later patch set.
- Reword parent clock check error message. (Suggested by Fabio Estevam)
Sebastian
[1] https://lkml.org/lkml/2015/4/30/688
Sebastian Hesselbarth (3):
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
.../devicetree/bindings/clock/silabs,si5351.txt | 4 +-
arch/arm/boot/dts/dove-cubox.dts | 1 +
drivers/clk/clk-si5351.c | 63 +++++++++++++++-------
include/linux/platform_data/si5351.h | 4 --
4 files changed, 49 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]
--
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: Jason Cooper <[email protected]>
Cc: Andrew Lunn <[email protected]>
Cc: Gregory Clement <[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]
---
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.
Signed-off-by: Sebastian Hesselbarth <[email protected]>
Reported-by: Michael Welling <[email protected]>
Reported-by: Jean-Francois Moine <[email protected]>
Reported-by: Russell King <[email protected]>
Tested-by: Michael Welling <[email protected]>
Tested-by: Jean-Francois Moine <[email protected]>
---
Changelog:
v1->v2:
- Reword parent clock check error message and add a comment about
the required parent clocks per variant. (Suggested by Fabio Estevam)
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]
---
drivers/clk/clk-si5351.c | 63 +++++++++++++++++++++++++-----------
include/linux/platform_data/si5351.h | 4 ---
2 files changed, 45 insertions(+), 22 deletions(-)
diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c
index 44ea107cfc67..30335d3b99af 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,22 @@ 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;
+
+ /*
+ * Check for valid parent clock: VARIANT_A and VARIANT_B need XTAL,
+ * VARIANT_C can have CLKIN instead.
+ */
+ if (IS_ERR(drvdata->pxtal) &&
+ (drvdata->variant != SI5351_VARIANT_C || IS_ERR(drvdata->pclkin))) {
+ dev_err(&client->dev, "missing parent clock\n");
+ return -EINVAL;
+ }
drvdata->regmap = devm_regmap_init_i2c(client, &si5351_regmap_config);
if (IS_ERR(drvdata->regmap)) {
@@ -1393,6 +1400,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 +1419,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 +1438,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 +1461,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 +1486,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 +1508,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 +1529,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 +1557,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 +1577,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
Hi Sebastian,
On 04/05/2015 23:04, Sebastian Hesselbarth wrote:
> Si5351 clock generator on CuBox uses XTAL as clock reference, name the
> clock phandle accordingly.
as soon as the binding will be accepted (in patch 1), I will apply it
on mvebu/fixes.
Thanks,
Gregory
>
> 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]
> ---
> 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>;
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
Quoting Sebastian Hesselbarth (2015-05-04 14:04:13)
> Mike, Stephen,
>
> this is v2 of dealing with recently reported clk-si5351 issues. Compared
> to v1 [1], I decided to first split-off the pure fixes and postpone the
> improvements to a later patch set. Patches 1 and 3 should go though clk
> fixes while patch 2 should be taken by mvebu maintainers.
>
> This fixes are intended for v4.1-rc1 and deal with issues where DT claimed
> parent clocks are not properly detected starting with v4.0. Looking at the
> code of clk-si5351, I just realized that the way the driver deals with
> parent clocks is utter nonsense. Russell King also mentioned that passing
> struct clk though platform_data shouldn't be done at all.
>
> Therefore, this 3 patches rework parent clock handling of clk-si5351 to
> make use of (a) named parent clocks, (b) devm_clk_get() for both DT- and
> platform_data passed parent clocks, and (c) properly clk_prepare_enable()
> valid parent clocks.
>
> However, I do have a stripped down backport of the fix for stable v4.0
> that just fixes the real issue, i.e. DT based probing and missing
> clk_prepare_enable(). If you agree with this fixes for v4.1-rc1 we should
> consider the backport for v4.0 stable.
I've pushed patches #1 and #3 to clk-fixes, now merged into clk-next.
I'll give it a couple of cycles in linux-next and then submit it for
4.1-rc4.
Regards,
Mike
>
> Overall changes compared to v1:
> - Postpone improvement patch ("clk: si5351: Reset PLL after rate change")
> for later patch set.
> - Reword parent clock check error message. (Suggested by Fabio Estevam)
>
> Sebastian
>
> [1] https://lkml.org/lkml/2015/4/30/688
>
> Sebastian Hesselbarth (3):
> 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
>
> .../devicetree/bindings/clock/silabs,si5351.txt | 4 +-
> arch/arm/boot/dts/dove-cubox.dts | 1 +
> drivers/clk/clk-si5351.c | 63 +++++++++++++++-------
> include/linux/platform_data/si5351.h | 4 --
> 4 files changed, 49 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]
> --
> 2.1.0
>
Quoting Gregory CLEMENT (2015-05-06 06:14:22)
> Hi Sebastian,
>
> On 04/05/2015 23:04, Sebastian Hesselbarth wrote:
> > Si5351 clock generator on CuBox uses XTAL as clock reference, name the
> > clock phandle accordingly.
>
> as soon as the binding will be accepted (in patch 1), I will apply it
> on mvebu/fixes.
It is on the clk git mirror now. Should be picked up in -next soon.
Regards,
Mike
>
> Thanks,
>
> Gregory
>
>
> >
> > 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]
> > ---
> > 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>;
> >
>
>
> --
> Gregory Clement, Free Electrons
> Kernel, drivers, real-time and embedded Linux
> development, consulting, training and support.
> http://free-electrons.com
Hi Mike, Sebastian,
On 08/05/2015 21:00, Michael Turquette wrote:
> Quoting Gregory CLEMENT (2015-05-06 06:14:22)
>> Hi Sebastian,
>>
>> On 04/05/2015 23:04, Sebastian Hesselbarth wrote:
>>> Si5351 clock generator on CuBox uses XTAL as clock reference, name the
>>> clock phandle accordingly.
>>
>> as soon as the binding will be accepted (in patch 1), I will apply it
>> on mvebu/fixes.
>
applied on mvebu/fixes
Thanks,
Gregory
> It is on the clk git mirror now. Should be picked up in -next soon.
>
> Regards,
> Mike
>
>>
>> Thanks,
>>
>> Gregory
>>
>>
>>>
>>> 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]
>>> ---
>>> 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>;
>>>
>>
>>
>> --
>> Gregory Clement, Free Electrons
>> Kernel, drivers, real-time and embedded Linux
>> development, consulting, training and support.
>> http://free-electrons.com
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com