Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On Thu, Feb 19, 2015 at 01:32:33PM -0800, Mike Turquette wrote: > Quoting Stephen Boyd (2015-02-06 11:30:18) > > On 02/06/15 05:39, Russell King - ARM Linux wrote: > > > On Thu, Feb 05, 2015 at 05:35:28PM -0800, Stephen Boyd wrote: > > > > > >> From what I can tell this code is > > >> now broken because we made all clk getting functions (there's quite a > > >> few...) return unique pointers every time they're called. It seems that > > >> the driver wants to know if extclk and clk are the same so it can do > > >> something differently in kirkwood_set_rate(). Do we need some sort of > > >> clk_equal(struct clk *a, struct clk *b) function for drivers like this? > > > Well, the clocks in question are the SoC internal clock (which is more or > > > less fixed, but has a programmable divider) and an externally supplied > > > clock, and the IP has a multiplexer on its input which allows us to select > > > between those two sources. > > > > > > If it were possible to bind both to the same clock, it wouldn't be a > > > useful configuration - nothing would be gained from doing so in terms of > > > available rates. > > > > > > What the comparison is there for is to catch the case with legacy lookups > > > where a clkdev lookup entry with a NULL connection ID results in matching > > > any connection ID passed to clk_get(). If the patch changes this, then > > > we will have a regression - and this is something which needs fixing > > > _before_ we do this "return unique clocks". > > > > > > > Ok. It seems that we might need a clk_equal() or similar API after all. > > My understanding is that this driver is calling clk_get() twice with > > NULL for the con_id and then "extclk" in attempts to get the SoC > > internal clock and the externally supplied clock. If we're using legacy > > lookups then both clk_get() calls may map to the same clk_lookup entry > > and before Tomeu's patch that returns unique clocks the driver could > > detect this case and know that there isn't an external clock. Looking at > > arch/arm/mach-dove/common.c it seems that there is only one lookup per > > device and it has a wildcard NULL for con_id so both clk_get() calls > > here are going to find the same lookup and get a unique struct clk pointer. > > > > Why don't we make the legacy lookup more specific and actually indicate > > "internal" for the con_id? Then the external clock would fail to be > > found, but we can detect that case and figure out that it's not due to > > probe defer, but instead due to the fact that there really isn't any > > mapping. It looks like the code is already prepared for this anyway. > > > > 8< > > > > From: Stephen Boyd > > Subject: [PATCH] ARM: dove: Remove wildcard from mvebu-audio device clk > > lookup > > > > This i2s driver is using the wildcard nature of clkdev lookups to > > figure out if there's an external clock or not. It does this by > > calling clk_get() twice with NULL for the con_id first and then > > "external" for the con_id the second time around and then > > compares the two pointers. With DT the wildcard feature of > > clk_get() is gone and so the driver has to handle an error from > > the second clk_get() call as meaning "no external clock > > specified". Let's use that logic even with clk lookups to > > simplify the code and remove the struct clk pointer comparisons > > which may not work in the future when clk_get() returns unique > > pointers. > > > > Signed-off-by: Stephen Boyd > > Russell et al, > > I'm happy to take this patch through the clock tree (where the problem > shows up) with an ack. It's not up to me - I don't maintain this driver. I'm just an interested party. Note that much more than just this has now broken. The iMX6 code has broken as well, and it's not going to take such a simple fix there to fix it either. Please either revert the patches creating this breakage (and have another attempt at the next merge window) or supply fixes for these places. -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On Thu, Feb 19, 2015 at 01:32:33PM -0800, Mike Turquette wrote: Quoting Stephen Boyd (2015-02-06 11:30:18) On 02/06/15 05:39, Russell King - ARM Linux wrote: On Thu, Feb 05, 2015 at 05:35:28PM -0800, Stephen Boyd wrote: From what I can tell this code is now broken because we made all clk getting functions (there's quite a few...) return unique pointers every time they're called. It seems that the driver wants to know if extclk and clk are the same so it can do something differently in kirkwood_set_rate(). Do we need some sort of clk_equal(struct clk *a, struct clk *b) function for drivers like this? Well, the clocks in question are the SoC internal clock (which is more or less fixed, but has a programmable divider) and an externally supplied clock, and the IP has a multiplexer on its input which allows us to select between those two sources. If it were possible to bind both to the same clock, it wouldn't be a useful configuration - nothing would be gained from doing so in terms of available rates. What the comparison is there for is to catch the case with legacy lookups where a clkdev lookup entry with a NULL connection ID results in matching any connection ID passed to clk_get(). If the patch changes this, then we will have a regression - and this is something which needs fixing _before_ we do this return unique clocks. Ok. It seems that we might need a clk_equal() or similar API after all. My understanding is that this driver is calling clk_get() twice with NULL for the con_id and then extclk in attempts to get the SoC internal clock and the externally supplied clock. If we're using legacy lookups then both clk_get() calls may map to the same clk_lookup entry and before Tomeu's patch that returns unique clocks the driver could detect this case and know that there isn't an external clock. Looking at arch/arm/mach-dove/common.c it seems that there is only one lookup per device and it has a wildcard NULL for con_id so both clk_get() calls here are going to find the same lookup and get a unique struct clk pointer. Why don't we make the legacy lookup more specific and actually indicate internal for the con_id? Then the external clock would fail to be found, but we can detect that case and figure out that it's not due to probe defer, but instead due to the fact that there really isn't any mapping. It looks like the code is already prepared for this anyway. 8 From: Stephen Boyd sb...@codeaurora.org Subject: [PATCH] ARM: dove: Remove wildcard from mvebu-audio device clk lookup This i2s driver is using the wildcard nature of clkdev lookups to figure out if there's an external clock or not. It does this by calling clk_get() twice with NULL for the con_id first and then external for the con_id the second time around and then compares the two pointers. With DT the wildcard feature of clk_get() is gone and so the driver has to handle an error from the second clk_get() call as meaning no external clock specified. Let's use that logic even with clk lookups to simplify the code and remove the struct clk pointer comparisons which may not work in the future when clk_get() returns unique pointers. Signed-off-by: Stephen Boyd sb...@codeaurora.org Russell et al, I'm happy to take this patch through the clock tree (where the problem shows up) with an ack. It's not up to me - I don't maintain this driver. I'm just an interested party. Note that much more than just this has now broken. The iMX6 code has broken as well, and it's not going to take such a simple fix there to fix it either. Please either revert the patches creating this breakage (and have another attempt at the next merge window) or supply fixes for these places. -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
Quoting Stephen Boyd (2015-02-06 11:30:18)
> On 02/06/15 05:39, Russell King - ARM Linux wrote:
> > On Thu, Feb 05, 2015 at 05:35:28PM -0800, Stephen Boyd wrote:
> >
> >> From what I can tell this code is
> >> now broken because we made all clk getting functions (there's quite a
> >> few...) return unique pointers every time they're called. It seems that
> >> the driver wants to know if extclk and clk are the same so it can do
> >> something differently in kirkwood_set_rate(). Do we need some sort of
> >> clk_equal(struct clk *a, struct clk *b) function for drivers like this?
> > Well, the clocks in question are the SoC internal clock (which is more or
> > less fixed, but has a programmable divider) and an externally supplied
> > clock, and the IP has a multiplexer on its input which allows us to select
> > between those two sources.
> >
> > If it were possible to bind both to the same clock, it wouldn't be a
> > useful configuration - nothing would be gained from doing so in terms of
> > available rates.
> >
> > What the comparison is there for is to catch the case with legacy lookups
> > where a clkdev lookup entry with a NULL connection ID results in matching
> > any connection ID passed to clk_get(). If the patch changes this, then
> > we will have a regression - and this is something which needs fixing
> > _before_ we do this "return unique clocks".
> >
>
> Ok. It seems that we might need a clk_equal() or similar API after all.
> My understanding is that this driver is calling clk_get() twice with
> NULL for the con_id and then "extclk" in attempts to get the SoC
> internal clock and the externally supplied clock. If we're using legacy
> lookups then both clk_get() calls may map to the same clk_lookup entry
> and before Tomeu's patch that returns unique clocks the driver could
> detect this case and know that there isn't an external clock. Looking at
> arch/arm/mach-dove/common.c it seems that there is only one lookup per
> device and it has a wildcard NULL for con_id so both clk_get() calls
> here are going to find the same lookup and get a unique struct clk pointer.
>
> Why don't we make the legacy lookup more specific and actually indicate
> "internal" for the con_id? Then the external clock would fail to be
> found, but we can detect that case and figure out that it's not due to
> probe defer, but instead due to the fact that there really isn't any
> mapping. It looks like the code is already prepared for this anyway.
>
> 8<
>
> From: Stephen Boyd
> Subject: [PATCH] ARM: dove: Remove wildcard from mvebu-audio device clk lookup
>
> This i2s driver is using the wildcard nature of clkdev lookups to
> figure out if there's an external clock or not. It does this by
> calling clk_get() twice with NULL for the con_id first and then
> "external" for the con_id the second time around and then
> compares the two pointers. With DT the wildcard feature of
> clk_get() is gone and so the driver has to handle an error from
> the second clk_get() call as meaning "no external clock
> specified". Let's use that logic even with clk lookups to
> simplify the code and remove the struct clk pointer comparisons
> which may not work in the future when clk_get() returns unique
> pointers.
>
> Signed-off-by: Stephen Boyd
Russell et al,
I'm happy to take this patch through the clock tree (where the problem
shows up) with an ack.
Regards,
Mike
> ---
> arch/arm/mach-dove/common.c | 4 ++--
> sound/soc/kirkwood/kirkwood-i2s.c | 13 -
> 2 files changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c
> index 0d1a89298ece..f290fc944cc1 100644
> --- a/arch/arm/mach-dove/common.c
> +++ b/arch/arm/mach-dove/common.c
> @@ -124,8 +124,8 @@ static void __init dove_clk_init(void)
> orion_clkdev_add(NULL, "sdhci-dove.1", sdio1);
> orion_clkdev_add(NULL, "orion_nand", nand);
> orion_clkdev_add(NULL, "cafe1000-ccic.0", camera);
> - orion_clkdev_add(NULL, "mvebu-audio.0", i2s0);
> - orion_clkdev_add(NULL, "mvebu-audio.1", i2s1);
> + orion_clkdev_add("internal", "mvebu-audio.0", i2s0);
> + orion_clkdev_add("internal", "mvebu-audio.1", i2s1);
> orion_clkdev_add(NULL, "mv_crypto", crypto);
> orion_clkdev_add(NULL, "dove-ac97", ac97);
> orion_clkdev_add(NULL, "dove-pdma", pdma);
> diff --git a/sound/soc/kirkwood/kirkwood-i2s.c
> b/sound/soc/kirkwood/kirkwood-i2s.c
> index def7d8260c4e..0bfeb712a997 100644
> --- a/sound/soc/kirkwood/kirkwood-i2s.c
> +++ b/sound/soc/kirkwood/kirkwood-i2s.c
> @@ -564,7 +564,7 @@ static int kirkwood_i2s_dev_probe(struct platform_device
> *pdev)
> return -EINVAL;
> }
>
> - priv->clk = devm_clk_get(>dev, np ? "internal" : NULL);
> + priv->clk = devm_clk_get(>dev, "internal");
> if (IS_ERR(priv->clk)) {
> dev_err(>dev, "no clock\n");
> return PTR_ERR(priv->clk);
> @@
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
Quoting Stephen Boyd (2015-02-06 11:30:18)
On 02/06/15 05:39, Russell King - ARM Linux wrote:
On Thu, Feb 05, 2015 at 05:35:28PM -0800, Stephen Boyd wrote:
From what I can tell this code is
now broken because we made all clk getting functions (there's quite a
few...) return unique pointers every time they're called. It seems that
the driver wants to know if extclk and clk are the same so it can do
something differently in kirkwood_set_rate(). Do we need some sort of
clk_equal(struct clk *a, struct clk *b) function for drivers like this?
Well, the clocks in question are the SoC internal clock (which is more or
less fixed, but has a programmable divider) and an externally supplied
clock, and the IP has a multiplexer on its input which allows us to select
between those two sources.
If it were possible to bind both to the same clock, it wouldn't be a
useful configuration - nothing would be gained from doing so in terms of
available rates.
What the comparison is there for is to catch the case with legacy lookups
where a clkdev lookup entry with a NULL connection ID results in matching
any connection ID passed to clk_get(). If the patch changes this, then
we will have a regression - and this is something which needs fixing
_before_ we do this return unique clocks.
Ok. It seems that we might need a clk_equal() or similar API after all.
My understanding is that this driver is calling clk_get() twice with
NULL for the con_id and then extclk in attempts to get the SoC
internal clock and the externally supplied clock. If we're using legacy
lookups then both clk_get() calls may map to the same clk_lookup entry
and before Tomeu's patch that returns unique clocks the driver could
detect this case and know that there isn't an external clock. Looking at
arch/arm/mach-dove/common.c it seems that there is only one lookup per
device and it has a wildcard NULL for con_id so both clk_get() calls
here are going to find the same lookup and get a unique struct clk pointer.
Why don't we make the legacy lookup more specific and actually indicate
internal for the con_id? Then the external clock would fail to be
found, but we can detect that case and figure out that it's not due to
probe defer, but instead due to the fact that there really isn't any
mapping. It looks like the code is already prepared for this anyway.
8
From: Stephen Boyd sb...@codeaurora.org
Subject: [PATCH] ARM: dove: Remove wildcard from mvebu-audio device clk lookup
This i2s driver is using the wildcard nature of clkdev lookups to
figure out if there's an external clock or not. It does this by
calling clk_get() twice with NULL for the con_id first and then
external for the con_id the second time around and then
compares the two pointers. With DT the wildcard feature of
clk_get() is gone and so the driver has to handle an error from
the second clk_get() call as meaning no external clock
specified. Let's use that logic even with clk lookups to
simplify the code and remove the struct clk pointer comparisons
which may not work in the future when clk_get() returns unique
pointers.
Signed-off-by: Stephen Boyd sb...@codeaurora.org
Russell et al,
I'm happy to take this patch through the clock tree (where the problem
shows up) with an ack.
Regards,
Mike
---
arch/arm/mach-dove/common.c | 4 ++--
sound/soc/kirkwood/kirkwood-i2s.c | 13 -
2 files changed, 6 insertions(+), 11 deletions(-)
diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c
index 0d1a89298ece..f290fc944cc1 100644
--- a/arch/arm/mach-dove/common.c
+++ b/arch/arm/mach-dove/common.c
@@ -124,8 +124,8 @@ static void __init dove_clk_init(void)
orion_clkdev_add(NULL, sdhci-dove.1, sdio1);
orion_clkdev_add(NULL, orion_nand, nand);
orion_clkdev_add(NULL, cafe1000-ccic.0, camera);
- orion_clkdev_add(NULL, mvebu-audio.0, i2s0);
- orion_clkdev_add(NULL, mvebu-audio.1, i2s1);
+ orion_clkdev_add(internal, mvebu-audio.0, i2s0);
+ orion_clkdev_add(internal, mvebu-audio.1, i2s1);
orion_clkdev_add(NULL, mv_crypto, crypto);
orion_clkdev_add(NULL, dove-ac97, ac97);
orion_clkdev_add(NULL, dove-pdma, pdma);
diff --git a/sound/soc/kirkwood/kirkwood-i2s.c
b/sound/soc/kirkwood/kirkwood-i2s.c
index def7d8260c4e..0bfeb712a997 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -564,7 +564,7 @@ static int kirkwood_i2s_dev_probe(struct platform_device
*pdev)
return -EINVAL;
}
- priv-clk = devm_clk_get(pdev-dev, np ? internal : NULL);
+ priv-clk = devm_clk_get(pdev-dev, internal);
if (IS_ERR(priv-clk)) {
dev_err(pdev-dev, no clock\n);
return PTR_ERR(priv-clk);
@@ -579,14 +579,9 @@ static int kirkwood_i2s_dev_probe(struct platform_device
*pdev)
if
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/06/15 11:37, Russell King - ARM Linux wrote: > On Fri, Feb 06, 2015 at 11:30:18AM -0800, Stephen Boyd wrote: >> Why don't we make the legacy lookup more specific and actually indicate >> "internal" for the con_id? Then the external clock would fail to be >> found, but we can detect that case and figure out that it's not due to >> probe defer, but instead due to the fact that there really isn't any >> mapping. It looks like the code is already prepared for this anyway. > We _could_, and that would solve this specific issue, but I'd suggest > coccinelle is used to locate any other similar instances of this. > > As I'm allergic to coccinelle (or coccinelle is allergic to me since > I never seem to be able to get it to do what I want...) > Great. I'd like to avoid adding clk_equal() until we actually need it, and I hope we don't ever need it. We've already got coccinelle in the works to find similar issues and it seems you and I have the same allergies because I can't get it to work for me right now. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On Fri, Feb 06, 2015 at 11:30:18AM -0800, Stephen Boyd wrote: > Why don't we make the legacy lookup more specific and actually indicate > "internal" for the con_id? Then the external clock would fail to be > found, but we can detect that case and figure out that it's not due to > probe defer, but instead due to the fact that there really isn't any > mapping. It looks like the code is already prepared for this anyway. We _could_, and that would solve this specific issue, but I'd suggest coccinelle is used to locate any other similar instances of this. As I'm allergic to coccinelle (or coccinelle is allergic to me since I never seem to be able to get it to do what I want...) -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/06/15 05:39, Russell King - ARM Linux wrote:
> On Thu, Feb 05, 2015 at 05:35:28PM -0800, Stephen Boyd wrote:
>
>> From what I can tell this code is
>> now broken because we made all clk getting functions (there's quite a
>> few...) return unique pointers every time they're called. It seems that
>> the driver wants to know if extclk and clk are the same so it can do
>> something differently in kirkwood_set_rate(). Do we need some sort of
>> clk_equal(struct clk *a, struct clk *b) function for drivers like this?
> Well, the clocks in question are the SoC internal clock (which is more or
> less fixed, but has a programmable divider) and an externally supplied
> clock, and the IP has a multiplexer on its input which allows us to select
> between those two sources.
>
> If it were possible to bind both to the same clock, it wouldn't be a
> useful configuration - nothing would be gained from doing so in terms of
> available rates.
>
> What the comparison is there for is to catch the case with legacy lookups
> where a clkdev lookup entry with a NULL connection ID results in matching
> any connection ID passed to clk_get(). If the patch changes this, then
> we will have a regression - and this is something which needs fixing
> _before_ we do this "return unique clocks".
>
Ok. It seems that we might need a clk_equal() or similar API after all.
My understanding is that this driver is calling clk_get() twice with
NULL for the con_id and then "extclk" in attempts to get the SoC
internal clock and the externally supplied clock. If we're using legacy
lookups then both clk_get() calls may map to the same clk_lookup entry
and before Tomeu's patch that returns unique clocks the driver could
detect this case and know that there isn't an external clock. Looking at
arch/arm/mach-dove/common.c it seems that there is only one lookup per
device and it has a wildcard NULL for con_id so both clk_get() calls
here are going to find the same lookup and get a unique struct clk pointer.
Why don't we make the legacy lookup more specific and actually indicate
"internal" for the con_id? Then the external clock would fail to be
found, but we can detect that case and figure out that it's not due to
probe defer, but instead due to the fact that there really isn't any
mapping. It looks like the code is already prepared for this anyway.
8<
From: Stephen Boyd
Subject: [PATCH] ARM: dove: Remove wildcard from mvebu-audio device clk lookup
This i2s driver is using the wildcard nature of clkdev lookups to
figure out if there's an external clock or not. It does this by
calling clk_get() twice with NULL for the con_id first and then
"external" for the con_id the second time around and then
compares the two pointers. With DT the wildcard feature of
clk_get() is gone and so the driver has to handle an error from
the second clk_get() call as meaning "no external clock
specified". Let's use that logic even with clk lookups to
simplify the code and remove the struct clk pointer comparisons
which may not work in the future when clk_get() returns unique
pointers.
Signed-off-by: Stephen Boyd
---
arch/arm/mach-dove/common.c | 4 ++--
sound/soc/kirkwood/kirkwood-i2s.c | 13 -
2 files changed, 6 insertions(+), 11 deletions(-)
diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c
index 0d1a89298ece..f290fc944cc1 100644
--- a/arch/arm/mach-dove/common.c
+++ b/arch/arm/mach-dove/common.c
@@ -124,8 +124,8 @@ static void __init dove_clk_init(void)
orion_clkdev_add(NULL, "sdhci-dove.1", sdio1);
orion_clkdev_add(NULL, "orion_nand", nand);
orion_clkdev_add(NULL, "cafe1000-ccic.0", camera);
- orion_clkdev_add(NULL, "mvebu-audio.0", i2s0);
- orion_clkdev_add(NULL, "mvebu-audio.1", i2s1);
+ orion_clkdev_add("internal", "mvebu-audio.0", i2s0);
+ orion_clkdev_add("internal", "mvebu-audio.1", i2s1);
orion_clkdev_add(NULL, "mv_crypto", crypto);
orion_clkdev_add(NULL, "dove-ac97", ac97);
orion_clkdev_add(NULL, "dove-pdma", pdma);
diff --git a/sound/soc/kirkwood/kirkwood-i2s.c
b/sound/soc/kirkwood/kirkwood-i2s.c
index def7d8260c4e..0bfeb712a997 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -564,7 +564,7 @@ static int kirkwood_i2s_dev_probe(struct platform_device
*pdev)
return -EINVAL;
}
- priv->clk = devm_clk_get(>dev, np ? "internal" : NULL);
+ priv->clk = devm_clk_get(>dev, "internal");
if (IS_ERR(priv->clk)) {
dev_err(>dev, "no clock\n");
return PTR_ERR(priv->clk);
@@ -579,14 +579,9 @@ static int kirkwood_i2s_dev_probe(struct platform_device
*pdev)
if (PTR_ERR(priv->extclk) == -EPROBE_DEFER)
return -EPROBE_DEFER;
} else {
- if (priv->extclk == priv->clk) {
- devm_clk_put(>dev, priv->extclk);
- priv->extclk =
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On Thu, Feb 05, 2015 at 05:35:28PM -0800, Stephen Boyd wrote:
> On 02/05/15 16:42, Russell King - ARM Linux wrote:
> > On Thu, Feb 05, 2015 at 02:14:01PM -0800, Stephen Boyd wrote:
> >> Actually we can bury the __clk_create_clk() inside
> >> __of_clk_get_from_provider(). We should also move __clk_get() into there
> >> because right now we have a hole where whoever calls
> >> of_clk_get_from_provider() never calls __clk_get() on the clk, leading
> >> to possible badness. v2 coming soon.
> > There's some other issues here too...
> >
> > sound/soc/kirkwood/kirkwood-i2s.c:
> >
> > priv->clk = devm_clk_get(>dev, np ? "internal" : NULL);
> > ...
> > priv->extclk = devm_clk_get(>dev, "extclk");
> > if (IS_ERR(priv->extclk)) {
> > ...
> > } else {
> > if (priv->extclk == priv->clk) {
> > devm_clk_put(>dev, priv->extclk);
> > priv->extclk = ERR_PTR(-EINVAL);
> > } else {
> > dev_info(>dev, "found external clock\n");
> > clk_prepare_enable(priv->extclk);
> > soc_dai = kirkwood_i2s_dai_extclk;
> > }
> >
> > It should be fine provided your "trick" is only done for DT clocks,
> > but not for legacy - with legacy, a NULL in the clkdev tables will
> > match both these requests, hence the need to compare the clk_get()
> > return value to tell whether we get the same clock.
> >
>
> Are we still talking about of_clk_get_from_provider()? Or are we talking
> about comparing struct clk pointers?
Comparing struct clk pointers, and the implications of the patch changing
the clk_get() et.al. to be unique struct clk pointers.
> From what I can tell this code is
> now broken because we made all clk getting functions (there's quite a
> few...) return unique pointers every time they're called. It seems that
> the driver wants to know if extclk and clk are the same so it can do
> something differently in kirkwood_set_rate(). Do we need some sort of
> clk_equal(struct clk *a, struct clk *b) function for drivers like this?
Well, the clocks in question are the SoC internal clock (which is more or
less fixed, but has a programmable divider) and an externally supplied
clock, and the IP has a multiplexer on its input which allows us to select
between those two sources.
If it were possible to bind both to the same clock, it wouldn't be a
useful configuration - nothing would be gained from doing so in terms of
available rates.
What the comparison is there for is to catch the case with legacy lookups
where a clkdev lookup entry with a NULL connection ID results in matching
any connection ID passed to clk_get(). If the patch changes this, then
we will have a regression - and this is something which needs fixing
_before_ we do this "return unique clocks".
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On Fri, Feb 06, 2015 at 11:30:18AM -0800, Stephen Boyd wrote: Why don't we make the legacy lookup more specific and actually indicate internal for the con_id? Then the external clock would fail to be found, but we can detect that case and figure out that it's not due to probe defer, but instead due to the fact that there really isn't any mapping. It looks like the code is already prepared for this anyway. We _could_, and that would solve this specific issue, but I'd suggest coccinelle is used to locate any other similar instances of this. As I'm allergic to coccinelle (or coccinelle is allergic to me since I never seem to be able to get it to do what I want...) -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/06/15 05:39, Russell King - ARM Linux wrote:
On Thu, Feb 05, 2015 at 05:35:28PM -0800, Stephen Boyd wrote:
From what I can tell this code is
now broken because we made all clk getting functions (there's quite a
few...) return unique pointers every time they're called. It seems that
the driver wants to know if extclk and clk are the same so it can do
something differently in kirkwood_set_rate(). Do we need some sort of
clk_equal(struct clk *a, struct clk *b) function for drivers like this?
Well, the clocks in question are the SoC internal clock (which is more or
less fixed, but has a programmable divider) and an externally supplied
clock, and the IP has a multiplexer on its input which allows us to select
between those two sources.
If it were possible to bind both to the same clock, it wouldn't be a
useful configuration - nothing would be gained from doing so in terms of
available rates.
What the comparison is there for is to catch the case with legacy lookups
where a clkdev lookup entry with a NULL connection ID results in matching
any connection ID passed to clk_get(). If the patch changes this, then
we will have a regression - and this is something which needs fixing
_before_ we do this return unique clocks.
Ok. It seems that we might need a clk_equal() or similar API after all.
My understanding is that this driver is calling clk_get() twice with
NULL for the con_id and then extclk in attempts to get the SoC
internal clock and the externally supplied clock. If we're using legacy
lookups then both clk_get() calls may map to the same clk_lookup entry
and before Tomeu's patch that returns unique clocks the driver could
detect this case and know that there isn't an external clock. Looking at
arch/arm/mach-dove/common.c it seems that there is only one lookup per
device and it has a wildcard NULL for con_id so both clk_get() calls
here are going to find the same lookup and get a unique struct clk pointer.
Why don't we make the legacy lookup more specific and actually indicate
internal for the con_id? Then the external clock would fail to be
found, but we can detect that case and figure out that it's not due to
probe defer, but instead due to the fact that there really isn't any
mapping. It looks like the code is already prepared for this anyway.
8
From: Stephen Boyd sb...@codeaurora.org
Subject: [PATCH] ARM: dove: Remove wildcard from mvebu-audio device clk lookup
This i2s driver is using the wildcard nature of clkdev lookups to
figure out if there's an external clock or not. It does this by
calling clk_get() twice with NULL for the con_id first and then
external for the con_id the second time around and then
compares the two pointers. With DT the wildcard feature of
clk_get() is gone and so the driver has to handle an error from
the second clk_get() call as meaning no external clock
specified. Let's use that logic even with clk lookups to
simplify the code and remove the struct clk pointer comparisons
which may not work in the future when clk_get() returns unique
pointers.
Signed-off-by: Stephen Boyd sb...@codeaurora.org
---
arch/arm/mach-dove/common.c | 4 ++--
sound/soc/kirkwood/kirkwood-i2s.c | 13 -
2 files changed, 6 insertions(+), 11 deletions(-)
diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c
index 0d1a89298ece..f290fc944cc1 100644
--- a/arch/arm/mach-dove/common.c
+++ b/arch/arm/mach-dove/common.c
@@ -124,8 +124,8 @@ static void __init dove_clk_init(void)
orion_clkdev_add(NULL, sdhci-dove.1, sdio1);
orion_clkdev_add(NULL, orion_nand, nand);
orion_clkdev_add(NULL, cafe1000-ccic.0, camera);
- orion_clkdev_add(NULL, mvebu-audio.0, i2s0);
- orion_clkdev_add(NULL, mvebu-audio.1, i2s1);
+ orion_clkdev_add(internal, mvebu-audio.0, i2s0);
+ orion_clkdev_add(internal, mvebu-audio.1, i2s1);
orion_clkdev_add(NULL, mv_crypto, crypto);
orion_clkdev_add(NULL, dove-ac97, ac97);
orion_clkdev_add(NULL, dove-pdma, pdma);
diff --git a/sound/soc/kirkwood/kirkwood-i2s.c
b/sound/soc/kirkwood/kirkwood-i2s.c
index def7d8260c4e..0bfeb712a997 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -564,7 +564,7 @@ static int kirkwood_i2s_dev_probe(struct platform_device
*pdev)
return -EINVAL;
}
- priv-clk = devm_clk_get(pdev-dev, np ? internal : NULL);
+ priv-clk = devm_clk_get(pdev-dev, internal);
if (IS_ERR(priv-clk)) {
dev_err(pdev-dev, no clock\n);
return PTR_ERR(priv-clk);
@@ -579,14 +579,9 @@ static int kirkwood_i2s_dev_probe(struct platform_device
*pdev)
if (PTR_ERR(priv-extclk) == -EPROBE_DEFER)
return -EPROBE_DEFER;
} else {
- if (priv-extclk == priv-clk) {
- devm_clk_put(pdev-dev, priv-extclk);
- priv-extclk = ERR_PTR(-EINVAL);
-
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/06/15 11:37, Russell King - ARM Linux wrote: On Fri, Feb 06, 2015 at 11:30:18AM -0800, Stephen Boyd wrote: Why don't we make the legacy lookup more specific and actually indicate internal for the con_id? Then the external clock would fail to be found, but we can detect that case and figure out that it's not due to probe defer, but instead due to the fact that there really isn't any mapping. It looks like the code is already prepared for this anyway. We _could_, and that would solve this specific issue, but I'd suggest coccinelle is used to locate any other similar instances of this. As I'm allergic to coccinelle (or coccinelle is allergic to me since I never seem to be able to get it to do what I want...) Great. I'd like to avoid adding clk_equal() until we actually need it, and I hope we don't ever need it. We've already got coccinelle in the works to find similar issues and it seems you and I have the same allergies because I can't get it to work for me right now. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On Thu, Feb 05, 2015 at 05:35:28PM -0800, Stephen Boyd wrote:
On 02/05/15 16:42, Russell King - ARM Linux wrote:
On Thu, Feb 05, 2015 at 02:14:01PM -0800, Stephen Boyd wrote:
Actually we can bury the __clk_create_clk() inside
__of_clk_get_from_provider(). We should also move __clk_get() into there
because right now we have a hole where whoever calls
of_clk_get_from_provider() never calls __clk_get() on the clk, leading
to possible badness. v2 coming soon.
There's some other issues here too...
sound/soc/kirkwood/kirkwood-i2s.c:
priv-clk = devm_clk_get(pdev-dev, np ? internal : NULL);
...
priv-extclk = devm_clk_get(pdev-dev, extclk);
if (IS_ERR(priv-extclk)) {
...
} else {
if (priv-extclk == priv-clk) {
devm_clk_put(pdev-dev, priv-extclk);
priv-extclk = ERR_PTR(-EINVAL);
} else {
dev_info(pdev-dev, found external clock\n);
clk_prepare_enable(priv-extclk);
soc_dai = kirkwood_i2s_dai_extclk;
}
It should be fine provided your trick is only done for DT clocks,
but not for legacy - with legacy, a NULL in the clkdev tables will
match both these requests, hence the need to compare the clk_get()
return value to tell whether we get the same clock.
Are we still talking about of_clk_get_from_provider()? Or are we talking
about comparing struct clk pointers?
Comparing struct clk pointers, and the implications of the patch changing
the clk_get() et.al. to be unique struct clk pointers.
From what I can tell this code is
now broken because we made all clk getting functions (there's quite a
few...) return unique pointers every time they're called. It seems that
the driver wants to know if extclk and clk are the same so it can do
something differently in kirkwood_set_rate(). Do we need some sort of
clk_equal(struct clk *a, struct clk *b) function for drivers like this?
Well, the clocks in question are the SoC internal clock (which is more or
less fixed, but has a programmable divider) and an externally supplied
clock, and the IP has a multiplexer on its input which allows us to select
between those two sources.
If it were possible to bind both to the same clock, it wouldn't be a
useful configuration - nothing would be gained from doing so in terms of
available rates.
What the comparison is there for is to catch the case with legacy lookups
where a clkdev lookup entry with a NULL connection ID results in matching
any connection ID passed to clk_get(). If the patch changes this, then
we will have a regression - and this is something which needs fixing
_before_ we do this return unique clocks.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/05/15 16:42, Russell King - ARM Linux wrote:
> On Thu, Feb 05, 2015 at 02:14:01PM -0800, Stephen Boyd wrote:
>> Actually we can bury the __clk_create_clk() inside
>> __of_clk_get_from_provider(). We should also move __clk_get() into there
>> because right now we have a hole where whoever calls
>> of_clk_get_from_provider() never calls __clk_get() on the clk, leading
>> to possible badness. v2 coming soon.
> There's some other issues here too...
>
> sound/soc/kirkwood/kirkwood-i2s.c:
>
> priv->clk = devm_clk_get(>dev, np ? "internal" : NULL);
> ...
> priv->extclk = devm_clk_get(>dev, "extclk");
> if (IS_ERR(priv->extclk)) {
> ...
> } else {
> if (priv->extclk == priv->clk) {
> devm_clk_put(>dev, priv->extclk);
> priv->extclk = ERR_PTR(-EINVAL);
> } else {
> dev_info(>dev, "found external clock\n");
> clk_prepare_enable(priv->extclk);
> soc_dai = kirkwood_i2s_dai_extclk;
> }
>
> It should be fine provided your "trick" is only done for DT clocks,
> but not for legacy - with legacy, a NULL in the clkdev tables will
> match both these requests, hence the need to compare the clk_get()
> return value to tell whether we get the same clock.
>
Are we still talking about of_clk_get_from_provider()? Or are we talking
about comparing struct clk pointers? From what I can tell this code is
now broken because we made all clk getting functions (there's quite a
few...) return unique pointers every time they're called. It seems that
the driver wants to know if extclk and clk are the same so it can do
something differently in kirkwood_set_rate(). Do we need some sort of
clk_equal(struct clk *a, struct clk *b) function for drivers like this?
Also, even on DT this could fail if the DT author made internal and
extclk map to the same clock provider and cell.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On Thu, Feb 05, 2015 at 02:14:01PM -0800, Stephen Boyd wrote:
> Actually we can bury the __clk_create_clk() inside
> __of_clk_get_from_provider(). We should also move __clk_get() into there
> because right now we have a hole where whoever calls
> of_clk_get_from_provider() never calls __clk_get() on the clk, leading
> to possible badness. v2 coming soon.
There's some other issues here too...
sound/soc/kirkwood/kirkwood-i2s.c:
priv->clk = devm_clk_get(>dev, np ? "internal" : NULL);
...
priv->extclk = devm_clk_get(>dev, "extclk");
if (IS_ERR(priv->extclk)) {
...
} else {
if (priv->extclk == priv->clk) {
devm_clk_put(>dev, priv->extclk);
priv->extclk = ERR_PTR(-EINVAL);
} else {
dev_info(>dev, "found external clock\n");
clk_prepare_enable(priv->extclk);
soc_dai = kirkwood_i2s_dai_extclk;
}
It should be fine provided your "trick" is only done for DT clocks,
but not for legacy - with legacy, a NULL in the clkdev tables will
match both these requests, hence the need to compare the clk_get()
return value to tell whether we get the same clock.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/05/15 12:07, Stephen Boyd wrote:
> On 02/05/15 11:44, Sylwester Nawrocki wrote:
>> Hi Tomeu,
>>
>> On 23/01/15 12:03, Tomeu Vizoso wrote:
>>> int __clk_get(struct clk *clk)
>>> {
>>> - if (clk) {
>>> - if (!try_module_get(clk->owner))
>>> + struct clk_core *core = !clk ? NULL : clk->core;
>>> +
>>> + if (core) {
>>> + if (!try_module_get(core->owner))
>>> return 0;
>>>
>>> - kref_get(>ref);
>>> + kref_get(>ref);
>>> }
>>> return 1;
>>> }
>>>
>>> -void __clk_put(struct clk *clk)
>>> +static void clk_core_put(struct clk_core *core)
>>> {
>>> struct module *owner;
>>>
>>> - if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
>>> - return;
>>> + owner = core->owner;
>>>
>>> clk_prepare_lock();
>>> - owner = clk->owner;
>>> - kref_put(>ref, __clk_release);
>>> + kref_put(>ref, __clk_release);
>>> clk_prepare_unlock();
>>>
>>> module_put(owner);
>>> }
>>>
>>> +void __clk_put(struct clk *clk)
>>> +{
>>> + if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
>>> + return;
>>> +
>>> + clk_core_put(clk->core);
>>> + kfree(clk);
>> Why do we have kfree() here? clk_get() doesn't allocate the data structure
>> being freed here. What happens if we do clk_get(), clk_put(), clk_get()
>> on same clock?
>>
>> I suspect __clk_free_clk() should be called in __clk_release() callback
>> instead, but then there is an issue of safely getting reference to
>> struct clk from struct clk_core pointer.
>>
>> I tested linux-next on Odroid U3 and booting fails with oopses as below.
>> There is no problems when the above kfree() is commented out.
>>
>>
> Ah now I get it. You meant to say that of_clk_get_by_clkspec() doesn't
> return an allocated clk pointer. Let's fix that.
>
> 8<
>
> From: Stephen Boyd
> Subject: [PATCH] clkdev: Always allocate a struct clk in OF functions
>
> of_clk_get_by_clkspec() returns a struct clk pointer but it
> doesn't create a new handle for the consumers. Instead it just
> returns whatever the OF clk provider hands out. Let's create a
> per-user handle here so that clk_put() can properly unlink it and
> free it when the consumer is done.
>
> Fixes: 035a61c314eb "clk: Make clk API return per-user struct clk instances"
> Reported-by: Sylwester Nawrocki
> Signed-off-by: Stephen Boyd
> ---
> drivers/clk/clkdev.c | 44 +++-
> 1 file changed, 23 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> index 29a1ab7af4b8..00d747d09b2a 100644
> --- a/drivers/clk/clkdev.c
> +++ b/drivers/clk/clkdev.c
> @@ -29,15 +29,8 @@ static DEFINE_MUTEX(clocks_mutex);
>
> #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
>
> -/**
> - * of_clk_get_by_clkspec() - Lookup a clock form a clock provider
> - * @clkspec: pointer to a clock specifier data structure
> - *
> - * This function looks up a struct clk from the registered list of clock
> - * providers, an input is a clock specifier data structure as returned
> - * from the of_parse_phandle_with_args() function call.
> - */
> -struct clk *of_clk_get_by_clkspec(struct of_phandle_args *clkspec)
> +static struct clk *__of_clk_get_by_clkspec(struct of_phandle_args *clkspec,
> + const char *dev_id, const char *con_id)
> {
> struct clk *clk;
>
> @@ -47,6 +40,8 @@ struct clk *of_clk_get_by_clkspec(struct of_phandle_args
> *clkspec)
> of_clk_lock();
> clk = __of_clk_get_from_provider(clkspec);
>
> + if (!IS_ERR(clk))
> + clk = __clk_create_clk(__clk_get_hw(clk), dev_id, con_id);
> if (!IS_ERR(clk) && !__clk_get(clk))
> clk = ERR_PTR(-ENOENT);
>
Actually we can bury the __clk_create_clk() inside
__of_clk_get_from_provider(). We should also move __clk_get() into there
because right now we have a hole where whoever calls
of_clk_get_from_provider() never calls __clk_get() on the clk, leading
to possible badness. v2 coming soon.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/05/15 11:44, Sylwester Nawrocki wrote:
> Hi Tomeu,
>
> On 23/01/15 12:03, Tomeu Vizoso wrote:
>> int __clk_get(struct clk *clk)
>> {
>> -if (clk) {
>> -if (!try_module_get(clk->owner))
>> +struct clk_core *core = !clk ? NULL : clk->core;
>> +
>> +if (core) {
>> +if (!try_module_get(core->owner))
>> return 0;
>>
>> -kref_get(>ref);
>> +kref_get(>ref);
>> }
>> return 1;
>> }
>>
>> -void __clk_put(struct clk *clk)
>> +static void clk_core_put(struct clk_core *core)
>> {
>> struct module *owner;
>>
>> -if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
>> -return;
>> +owner = core->owner;
>>
>> clk_prepare_lock();
>> -owner = clk->owner;
>> -kref_put(>ref, __clk_release);
>> +kref_put(>ref, __clk_release);
>> clk_prepare_unlock();
>>
>> module_put(owner);
>> }
>>
>> +void __clk_put(struct clk *clk)
>> +{
>> +if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
>> +return;
>> +
>> +clk_core_put(clk->core);
>> +kfree(clk);
> Why do we have kfree() here? clk_get() doesn't allocate the data structure
> being freed here. What happens if we do clk_get(), clk_put(), clk_get()
> on same clock?
>
> I suspect __clk_free_clk() should be called in __clk_release() callback
> instead, but then there is an issue of safely getting reference to
> struct clk from struct clk_core pointer.
>
> I tested linux-next on Odroid U3 and booting fails with oopses as below.
> There is no problems when the above kfree() is commented out.
>
>
Ah now I get it. You meant to say that of_clk_get_by_clkspec() doesn't
return an allocated clk pointer. Let's fix that.
8<
From: Stephen Boyd
Subject: [PATCH] clkdev: Always allocate a struct clk in OF functions
of_clk_get_by_clkspec() returns a struct clk pointer but it
doesn't create a new handle for the consumers. Instead it just
returns whatever the OF clk provider hands out. Let's create a
per-user handle here so that clk_put() can properly unlink it and
free it when the consumer is done.
Fixes: 035a61c314eb "clk: Make clk API return per-user struct clk instances"
Reported-by: Sylwester Nawrocki
Signed-off-by: Stephen Boyd
---
drivers/clk/clkdev.c | 44 +++-
1 file changed, 23 insertions(+), 21 deletions(-)
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 29a1ab7af4b8..00d747d09b2a 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -29,15 +29,8 @@ static DEFINE_MUTEX(clocks_mutex);
#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
-/**
- * of_clk_get_by_clkspec() - Lookup a clock form a clock provider
- * @clkspec: pointer to a clock specifier data structure
- *
- * This function looks up a struct clk from the registered list of clock
- * providers, an input is a clock specifier data structure as returned
- * from the of_parse_phandle_with_args() function call.
- */
-struct clk *of_clk_get_by_clkspec(struct of_phandle_args *clkspec)
+static struct clk *__of_clk_get_by_clkspec(struct of_phandle_args *clkspec,
+const char *dev_id, const char *con_id)
{
struct clk *clk;
@@ -47,6 +40,8 @@ struct clk *of_clk_get_by_clkspec(struct of_phandle_args
*clkspec)
of_clk_lock();
clk = __of_clk_get_from_provider(clkspec);
+ if (!IS_ERR(clk))
+ clk = __clk_create_clk(__clk_get_hw(clk), dev_id, con_id);
if (!IS_ERR(clk) && !__clk_get(clk))
clk = ERR_PTR(-ENOENT);
@@ -54,7 +49,21 @@ struct clk *of_clk_get_by_clkspec(struct of_phandle_args
*clkspec)
return clk;
}
-static struct clk *__of_clk_get(struct device_node *np, int index)
+/**
+ * of_clk_get_by_clkspec() - Lookup a clock form a clock provider
+ * @clkspec: pointer to a clock specifier data structure
+ *
+ * This function looks up a struct clk from the registered list of clock
+ * providers, an input is a clock specifier data structure as returned
+ * from the of_parse_phandle_with_args() function call.
+ */
+struct clk *of_clk_get_by_clkspec(struct of_phandle_args *clkspec)
+{
+ return __of_clk_get_by_clkspec(clkspec, NULL, __func__);
+}
+
+static struct clk *__of_clk_get(struct device_node *np, int index,
+ const char *dev_id, const char *con_id)
{
struct of_phandle_args clkspec;
struct clk *clk;
@@ -68,7 +77,7 @@ static struct clk *__of_clk_get(struct device_node *np, int
index)
if (rc)
return ERR_PTR(rc);
- clk = of_clk_get_by_clkspec();
+ clk = __of_clk_get_by_clkspec(, dev_id, con_id);
of_node_put(clkspec.np);
return clk;
@@ -76,12 +85,7 @@ static struct clk *__of_clk_get(struct device_node *np, int
index)
struct clk *of_clk_get(struct device_node *np, int index)
{
- struct clk *clk = __of_clk_get(np, index);
-
- if (!IS_ERR(clk))
-
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 05/02/15 20:44, Sylwester Nawrocki wrote:
>> +void __clk_put(struct clk *clk)
>> > +{
>> > + if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
>> > + return;
>> > +
>> > + clk_core_put(clk->core);
>> > + kfree(clk);
>
> Why do we have kfree() here? clk_get() doesn't allocate the data structure
> being freed here. What happens if we do clk_get(), clk_put(), clk_get()
> on same clock?
>
> I suspect __clk_free_clk() should be called in __clk_release() callback
> instead, but then there is an issue of safely getting reference to
> struct clk from struct clk_core pointer.
Please ignore this comment, I missed __clk_create_clk() calls in clkdev.c
Anyway, in current -next I'm seeing random pointer dereferences while
booting Odroid U3, I'll get back to debugging this tomorrow morning.
--
Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
Hi Tomeu,
On 23/01/15 12:03, Tomeu Vizoso wrote:
> int __clk_get(struct clk *clk)
> {
> - if (clk) {
> - if (!try_module_get(clk->owner))
> + struct clk_core *core = !clk ? NULL : clk->core;
> +
> + if (core) {
> + if (!try_module_get(core->owner))
> return 0;
>
> - kref_get(>ref);
> + kref_get(>ref);
> }
> return 1;
> }
>
> -void __clk_put(struct clk *clk)
> +static void clk_core_put(struct clk_core *core)
> {
> struct module *owner;
>
> - if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
> - return;
> + owner = core->owner;
>
> clk_prepare_lock();
> - owner = clk->owner;
> - kref_put(>ref, __clk_release);
> + kref_put(>ref, __clk_release);
> clk_prepare_unlock();
>
> module_put(owner);
> }
>
> +void __clk_put(struct clk *clk)
> +{
> + if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
> + return;
> +
> + clk_core_put(clk->core);
> + kfree(clk);
Why do we have kfree() here? clk_get() doesn't allocate the data structure
being freed here. What happens if we do clk_get(), clk_put(), clk_get()
on same clock?
I suspect __clk_free_clk() should be called in __clk_release() callback
instead, but then there is an issue of safely getting reference to
struct clk from struct clk_core pointer.
I tested linux-next on Odroid U3 and booting fails with oopses as below.
There is no problems when the above kfree() is commented out.
> +}
[1.345850] Unable to handle kernel paging request at virtual address
00200200
[1.349319] pgd = c0004000
[1.352072] [00200200] *pgd=
[1.355574] Internal error: Oops: 805 [#1] PREEMPT SMP ARM
[1.361035] Modules linked in:
[1.364078] CPU: 0 PID: 1 Comm: swapper/0 Not tainted
3.19.0-rc1-00104-ga251361a-dirty #992
[1.372405] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[1.378483] task: ee00b000 ti: ee088000 task.ti: ee088000
[1.383879] PC is at __clk_put+0x24/0xd0
[1.387773] LR is at clk_prepare_lock+0xc/0xec
[1.392198] pc : []lr : []psr: 2153
[1.392198] sp : ee089de8 ip : fp :
[1.403653] r10: ee02f480 r9 : 0001 r8 :
[1.408862] r7 : ee031cc0 r6 : ee089e08 r5 : r4 : ee02f480
[1.415373] r3 : 00100100 r2 : 00200200 r1 : 091e r0 : 0001
[1.421884] Flags: nzCv IRQs on FIQs off Mode SVC_32 ISA ARM Segment
kernel
[1.429261] Control: 10c5387d Table: 4000404a DAC: 0015
[1.434989] Process swapper/0 (pid: 1, stack limit = 0xee088238)
[1.440978] Stack: (0xee089de8 to 0xee08a000)
[1.445321] 9de0: ee7c8f14 c03f0ec8 ee089e08
c0718dc8 0001
[1.453480] 9e00: c04ee0f0 ee7e0844 0001 0181 c04edb58
ee2bd320
[1.461639] 9e20: c011dc5c ee16a1e0 c0718dc8
ee16a1e0 ee2bd1e0
[1.469798] 9e40: c0641740 ee16a1e0 ee2bd320 c0718dc8 ee1d3e10
ee1d3e10
[1.477957] 9e60: c0769a88 c0718dc8 c02c3124
c02c310c ee1d3e10
[1.486117] 9e80: c07b4eec c0769a88 c02c1d0c ee1d3e10 c0769a88
ee1d3e44
[1.494276] 9ea0: c07091dc c02c1eb8 c0769a88 c02c1e2c c02c0544
ee005478 ee1676c0
[1.502435] 9ec0: c0769a88 ee3a4e80 c0760ce8 c02c150c c0669b90 c0769a88
c0746cd8 c0769a88
[1.510594] 9ee0: c0746cd8 ee2bc4c0 c0778c00 c02c24e0 c0746cd8
c0746cd8 c07091f0
[1.518753] 9f00: c0008944 c04f405c 0025 ee00b000 6153
c074ab00
[1.526913] 9f20: c074ab90 6153 ef7fca5d c050860c
00b6 c0036b88
[1.535071] 9f40: c065ecc4 c06bc728 0006 0006 c074ab30 ef7fca40
c0739bdc 0006
[1.543231] 9f60: c0718dbc c0778c00 00b6 c0718dc8 c06ed598 c06edd64
0006 0006
[1.551390] 9f80: c06ed598 c003b438 c04e64f4
[1.559549] 9fa0: c04e64fc c000e838
[1.567708] 9fc0:
[1.575867] 9fe0: 0013
c0c0c0c0 c0c0c0c0
[1.584045] [] (__clk_put) from []
(of_clk_set_defaults+0xe0/0x2c0)
[1.592024] [] (of_clk_set_defaults) from []
(platform_drv_probe+0x18/0xa4)
[1.600698] [] (platform_drv_probe) from []
(driver_probe_device+0x10c/0x22c)
[1.609549] [] (driver_probe_device) from []
(__driver_attach+0x8c/0x90)
[1.617968] [] (__driver_attach) from []
(bus_for_each_dev+0x54/0x88)
[1.626128] [] (bus_for_each_dev) from []
(bus_add_driver+0xd4/0x1d0)
[1.634286] [] (bus_add_driver) from []
(driver_register+0x78/0xf4)
[1.642275] [] (driver_register) from []
(fimc_md_init+0x14/0x30)
[1.650089] [] (fimc_md_init) from []
(do_one_initcall+0x80/0x1d0)
[1.657989] [] (do_one_initcall) from []
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/05/15 12:07, Stephen Boyd wrote:
On 02/05/15 11:44, Sylwester Nawrocki wrote:
Hi Tomeu,
On 23/01/15 12:03, Tomeu Vizoso wrote:
int __clk_get(struct clk *clk)
{
- if (clk) {
- if (!try_module_get(clk-owner))
+ struct clk_core *core = !clk ? NULL : clk-core;
+
+ if (core) {
+ if (!try_module_get(core-owner))
return 0;
- kref_get(clk-ref);
+ kref_get(core-ref);
}
return 1;
}
-void __clk_put(struct clk *clk)
+static void clk_core_put(struct clk_core *core)
{
struct module *owner;
- if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
- return;
+ owner = core-owner;
clk_prepare_lock();
- owner = clk-owner;
- kref_put(clk-ref, __clk_release);
+ kref_put(core-ref, __clk_release);
clk_prepare_unlock();
module_put(owner);
}
+void __clk_put(struct clk *clk)
+{
+ if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
+ return;
+
+ clk_core_put(clk-core);
+ kfree(clk);
Why do we have kfree() here? clk_get() doesn't allocate the data structure
being freed here. What happens if we do clk_get(), clk_put(), clk_get()
on same clock?
I suspect __clk_free_clk() should be called in __clk_release() callback
instead, but then there is an issue of safely getting reference to
struct clk from struct clk_core pointer.
I tested linux-next on Odroid U3 and booting fails with oopses as below.
There is no problems when the above kfree() is commented out.
Ah now I get it. You meant to say that of_clk_get_by_clkspec() doesn't
return an allocated clk pointer. Let's fix that.
8
From: Stephen Boyd sb...@codeaurora.org
Subject: [PATCH] clkdev: Always allocate a struct clk in OF functions
of_clk_get_by_clkspec() returns a struct clk pointer but it
doesn't create a new handle for the consumers. Instead it just
returns whatever the OF clk provider hands out. Let's create a
per-user handle here so that clk_put() can properly unlink it and
free it when the consumer is done.
Fixes: 035a61c314eb clk: Make clk API return per-user struct clk instances
Reported-by: Sylwester Nawrocki s.nawro...@samsung.com
Signed-off-by: Stephen Boyd sb...@codeaurora.org
---
drivers/clk/clkdev.c | 44 +++-
1 file changed, 23 insertions(+), 21 deletions(-)
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 29a1ab7af4b8..00d747d09b2a 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -29,15 +29,8 @@ static DEFINE_MUTEX(clocks_mutex);
#if defined(CONFIG_OF) defined(CONFIG_COMMON_CLK)
-/**
- * of_clk_get_by_clkspec() - Lookup a clock form a clock provider
- * @clkspec: pointer to a clock specifier data structure
- *
- * This function looks up a struct clk from the registered list of clock
- * providers, an input is a clock specifier data structure as returned
- * from the of_parse_phandle_with_args() function call.
- */
-struct clk *of_clk_get_by_clkspec(struct of_phandle_args *clkspec)
+static struct clk *__of_clk_get_by_clkspec(struct of_phandle_args *clkspec,
+ const char *dev_id, const char *con_id)
{
struct clk *clk;
@@ -47,6 +40,8 @@ struct clk *of_clk_get_by_clkspec(struct of_phandle_args
*clkspec)
of_clk_lock();
clk = __of_clk_get_from_provider(clkspec);
+ if (!IS_ERR(clk))
+ clk = __clk_create_clk(__clk_get_hw(clk), dev_id, con_id);
if (!IS_ERR(clk) !__clk_get(clk))
clk = ERR_PTR(-ENOENT);
Actually we can bury the __clk_create_clk() inside
__of_clk_get_from_provider(). We should also move __clk_get() into there
because right now we have a hole where whoever calls
of_clk_get_from_provider() never calls __clk_get() on the clk, leading
to possible badness. v2 coming soon.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On Thu, Feb 05, 2015 at 02:14:01PM -0800, Stephen Boyd wrote:
Actually we can bury the __clk_create_clk() inside
__of_clk_get_from_provider(). We should also move __clk_get() into there
because right now we have a hole where whoever calls
of_clk_get_from_provider() never calls __clk_get() on the clk, leading
to possible badness. v2 coming soon.
There's some other issues here too...
sound/soc/kirkwood/kirkwood-i2s.c:
priv-clk = devm_clk_get(pdev-dev, np ? internal : NULL);
...
priv-extclk = devm_clk_get(pdev-dev, extclk);
if (IS_ERR(priv-extclk)) {
...
} else {
if (priv-extclk == priv-clk) {
devm_clk_put(pdev-dev, priv-extclk);
priv-extclk = ERR_PTR(-EINVAL);
} else {
dev_info(pdev-dev, found external clock\n);
clk_prepare_enable(priv-extclk);
soc_dai = kirkwood_i2s_dai_extclk;
}
It should be fine provided your trick is only done for DT clocks,
but not for legacy - with legacy, a NULL in the clkdev tables will
match both these requests, hence the need to compare the clk_get()
return value to tell whether we get the same clock.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/05/15 16:42, Russell King - ARM Linux wrote:
On Thu, Feb 05, 2015 at 02:14:01PM -0800, Stephen Boyd wrote:
Actually we can bury the __clk_create_clk() inside
__of_clk_get_from_provider(). We should also move __clk_get() into there
because right now we have a hole where whoever calls
of_clk_get_from_provider() never calls __clk_get() on the clk, leading
to possible badness. v2 coming soon.
There's some other issues here too...
sound/soc/kirkwood/kirkwood-i2s.c:
priv-clk = devm_clk_get(pdev-dev, np ? internal : NULL);
...
priv-extclk = devm_clk_get(pdev-dev, extclk);
if (IS_ERR(priv-extclk)) {
...
} else {
if (priv-extclk == priv-clk) {
devm_clk_put(pdev-dev, priv-extclk);
priv-extclk = ERR_PTR(-EINVAL);
} else {
dev_info(pdev-dev, found external clock\n);
clk_prepare_enable(priv-extclk);
soc_dai = kirkwood_i2s_dai_extclk;
}
It should be fine provided your trick is only done for DT clocks,
but not for legacy - with legacy, a NULL in the clkdev tables will
match both these requests, hence the need to compare the clk_get()
return value to tell whether we get the same clock.
Are we still talking about of_clk_get_from_provider()? Or are we talking
about comparing struct clk pointers? From what I can tell this code is
now broken because we made all clk getting functions (there's quite a
few...) return unique pointers every time they're called. It seems that
the driver wants to know if extclk and clk are the same so it can do
something differently in kirkwood_set_rate(). Do we need some sort of
clk_equal(struct clk *a, struct clk *b) function for drivers like this?
Also, even on DT this could fail if the DT author made internal and
extclk map to the same clock provider and cell.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
Hi Tomeu,
On 23/01/15 12:03, Tomeu Vizoso wrote:
int __clk_get(struct clk *clk)
{
- if (clk) {
- if (!try_module_get(clk-owner))
+ struct clk_core *core = !clk ? NULL : clk-core;
+
+ if (core) {
+ if (!try_module_get(core-owner))
return 0;
- kref_get(clk-ref);
+ kref_get(core-ref);
}
return 1;
}
-void __clk_put(struct clk *clk)
+static void clk_core_put(struct clk_core *core)
{
struct module *owner;
- if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
- return;
+ owner = core-owner;
clk_prepare_lock();
- owner = clk-owner;
- kref_put(clk-ref, __clk_release);
+ kref_put(core-ref, __clk_release);
clk_prepare_unlock();
module_put(owner);
}
+void __clk_put(struct clk *clk)
+{
+ if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
+ return;
+
+ clk_core_put(clk-core);
+ kfree(clk);
Why do we have kfree() here? clk_get() doesn't allocate the data structure
being freed here. What happens if we do clk_get(), clk_put(), clk_get()
on same clock?
I suspect __clk_free_clk() should be called in __clk_release() callback
instead, but then there is an issue of safely getting reference to
struct clk from struct clk_core pointer.
I tested linux-next on Odroid U3 and booting fails with oopses as below.
There is no problems when the above kfree() is commented out.
+}
[1.345850] Unable to handle kernel paging request at virtual address
00200200
[1.349319] pgd = c0004000
[1.352072] [00200200] *pgd=
[1.355574] Internal error: Oops: 805 [#1] PREEMPT SMP ARM
[1.361035] Modules linked in:
[1.364078] CPU: 0 PID: 1 Comm: swapper/0 Not tainted
3.19.0-rc1-00104-ga251361a-dirty #992
[1.372405] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[1.378483] task: ee00b000 ti: ee088000 task.ti: ee088000
[1.383879] PC is at __clk_put+0x24/0xd0
[1.387773] LR is at clk_prepare_lock+0xc/0xec
[1.392198] pc : [c03eef38]lr : [c03ec1f4]psr: 2153
[1.392198] sp : ee089de8 ip : fp :
[1.403653] r10: ee02f480 r9 : 0001 r8 :
[1.408862] r7 : ee031cc0 r6 : ee089e08 r5 : r4 : ee02f480
[1.415373] r3 : 00100100 r2 : 00200200 r1 : 091e r0 : 0001
[1.421884] Flags: nzCv IRQs on FIQs off Mode SVC_32 ISA ARM Segment
kernel
[1.429261] Control: 10c5387d Table: 4000404a DAC: 0015
[1.434989] Process swapper/0 (pid: 1, stack limit = 0xee088238)
[1.440978] Stack: (0xee089de8 to 0xee08a000)
[1.445321] 9de0: ee7c8f14 c03f0ec8 ee089e08
c0718dc8 0001
[1.453480] 9e00: c04ee0f0 ee7e0844 0001 0181 c04edb58
ee2bd320
[1.461639] 9e20: c011dc5c ee16a1e0 c0718dc8
ee16a1e0 ee2bd1e0
[1.469798] 9e40: c0641740 ee16a1e0 ee2bd320 c0718dc8 ee1d3e10
ee1d3e10
[1.477957] 9e60: c0769a88 c0718dc8 c02c3124
c02c310c ee1d3e10
[1.486117] 9e80: c07b4eec c0769a88 c02c1d0c ee1d3e10 c0769a88
ee1d3e44
[1.494276] 9ea0: c07091dc c02c1eb8 c0769a88 c02c1e2c c02c0544
ee005478 ee1676c0
[1.502435] 9ec0: c0769a88 ee3a4e80 c0760ce8 c02c150c c0669b90 c0769a88
c0746cd8 c0769a88
[1.510594] 9ee0: c0746cd8 ee2bc4c0 c0778c00 c02c24e0 c0746cd8
c0746cd8 c07091f0
[1.518753] 9f00: c0008944 c04f405c 0025 ee00b000 6153
c074ab00
[1.526913] 9f20: c074ab90 6153 ef7fca5d c050860c
00b6 c0036b88
[1.535071] 9f40: c065ecc4 c06bc728 0006 0006 c074ab30 ef7fca40
c0739bdc 0006
[1.543231] 9f60: c0718dbc c0778c00 00b6 c0718dc8 c06ed598 c06edd64
0006 0006
[1.551390] 9f80: c06ed598 c003b438 c04e64f4
[1.559549] 9fa0: c04e64fc c000e838
[1.567708] 9fc0:
[1.575867] 9fe0: 0013
c0c0c0c0 c0c0c0c0
[1.584045] [c03eef38] (__clk_put) from [c03f0ec8]
(of_clk_set_defaults+0xe0/0x2c0)
[1.592024] [c03f0ec8] (of_clk_set_defaults) from [c02c3124]
(platform_drv_probe+0x18/0xa4)
[1.600698] [c02c3124] (platform_drv_probe) from [c02c1d0c]
(driver_probe_device+0x10c/0x22c)
[1.609549] [c02c1d0c] (driver_probe_device) from [c02c1eb8]
(__driver_attach+0x8c/0x90)
[1.617968] [c02c1eb8] (__driver_attach) from [c02c0544]
(bus_for_each_dev+0x54/0x88)
[1.626128] [c02c0544] (bus_for_each_dev) from [c02c150c]
(bus_add_driver+0xd4/0x1d0)
[1.634286] [c02c150c] (bus_add_driver) from [c02c24e0]
(driver_register+0x78/0xf4)
[1.642275] [c02c24e0] (driver_register) from [c07091f0]
(fimc_md_init+0x14/0x30)
[1.650089]
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 05/02/15 20:44, Sylwester Nawrocki wrote:
+void __clk_put(struct clk *clk)
+{
+ if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
+ return;
+
+ clk_core_put(clk-core);
+ kfree(clk);
Why do we have kfree() here? clk_get() doesn't allocate the data structure
being freed here. What happens if we do clk_get(), clk_put(), clk_get()
on same clock?
I suspect __clk_free_clk() should be called in __clk_release() callback
instead, but then there is an issue of safely getting reference to
struct clk from struct clk_core pointer.
Please ignore this comment, I missed __clk_create_clk() calls in clkdev.c
Anyway, in current -next I'm seeing random pointer dereferences while
booting Odroid U3, I'll get back to debugging this tomorrow morning.
--
Regards,
Sylwester
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/05/15 11:44, Sylwester Nawrocki wrote:
Hi Tomeu,
On 23/01/15 12:03, Tomeu Vizoso wrote:
int __clk_get(struct clk *clk)
{
-if (clk) {
-if (!try_module_get(clk-owner))
+struct clk_core *core = !clk ? NULL : clk-core;
+
+if (core) {
+if (!try_module_get(core-owner))
return 0;
-kref_get(clk-ref);
+kref_get(core-ref);
}
return 1;
}
-void __clk_put(struct clk *clk)
+static void clk_core_put(struct clk_core *core)
{
struct module *owner;
-if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
-return;
+owner = core-owner;
clk_prepare_lock();
-owner = clk-owner;
-kref_put(clk-ref, __clk_release);
+kref_put(core-ref, __clk_release);
clk_prepare_unlock();
module_put(owner);
}
+void __clk_put(struct clk *clk)
+{
+if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
+return;
+
+clk_core_put(clk-core);
+kfree(clk);
Why do we have kfree() here? clk_get() doesn't allocate the data structure
being freed here. What happens if we do clk_get(), clk_put(), clk_get()
on same clock?
I suspect __clk_free_clk() should be called in __clk_release() callback
instead, but then there is an issue of safely getting reference to
struct clk from struct clk_core pointer.
I tested linux-next on Odroid U3 and booting fails with oopses as below.
There is no problems when the above kfree() is commented out.
Ah now I get it. You meant to say that of_clk_get_by_clkspec() doesn't
return an allocated clk pointer. Let's fix that.
8
From: Stephen Boyd sb...@codeaurora.org
Subject: [PATCH] clkdev: Always allocate a struct clk in OF functions
of_clk_get_by_clkspec() returns a struct clk pointer but it
doesn't create a new handle for the consumers. Instead it just
returns whatever the OF clk provider hands out. Let's create a
per-user handle here so that clk_put() can properly unlink it and
free it when the consumer is done.
Fixes: 035a61c314eb clk: Make clk API return per-user struct clk instances
Reported-by: Sylwester Nawrocki s.nawro...@samsung.com
Signed-off-by: Stephen Boyd sb...@codeaurora.org
---
drivers/clk/clkdev.c | 44 +++-
1 file changed, 23 insertions(+), 21 deletions(-)
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 29a1ab7af4b8..00d747d09b2a 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -29,15 +29,8 @@ static DEFINE_MUTEX(clocks_mutex);
#if defined(CONFIG_OF) defined(CONFIG_COMMON_CLK)
-/**
- * of_clk_get_by_clkspec() - Lookup a clock form a clock provider
- * @clkspec: pointer to a clock specifier data structure
- *
- * This function looks up a struct clk from the registered list of clock
- * providers, an input is a clock specifier data structure as returned
- * from the of_parse_phandle_with_args() function call.
- */
-struct clk *of_clk_get_by_clkspec(struct of_phandle_args *clkspec)
+static struct clk *__of_clk_get_by_clkspec(struct of_phandle_args *clkspec,
+const char *dev_id, const char *con_id)
{
struct clk *clk;
@@ -47,6 +40,8 @@ struct clk *of_clk_get_by_clkspec(struct of_phandle_args
*clkspec)
of_clk_lock();
clk = __of_clk_get_from_provider(clkspec);
+ if (!IS_ERR(clk))
+ clk = __clk_create_clk(__clk_get_hw(clk), dev_id, con_id);
if (!IS_ERR(clk) !__clk_get(clk))
clk = ERR_PTR(-ENOENT);
@@ -54,7 +49,21 @@ struct clk *of_clk_get_by_clkspec(struct of_phandle_args
*clkspec)
return clk;
}
-static struct clk *__of_clk_get(struct device_node *np, int index)
+/**
+ * of_clk_get_by_clkspec() - Lookup a clock form a clock provider
+ * @clkspec: pointer to a clock specifier data structure
+ *
+ * This function looks up a struct clk from the registered list of clock
+ * providers, an input is a clock specifier data structure as returned
+ * from the of_parse_phandle_with_args() function call.
+ */
+struct clk *of_clk_get_by_clkspec(struct of_phandle_args *clkspec)
+{
+ return __of_clk_get_by_clkspec(clkspec, NULL, __func__);
+}
+
+static struct clk *__of_clk_get(struct device_node *np, int index,
+ const char *dev_id, const char *con_id)
{
struct of_phandle_args clkspec;
struct clk *clk;
@@ -68,7 +77,7 @@ static struct clk *__of_clk_get(struct device_node *np, int
index)
if (rc)
return ERR_PTR(rc);
- clk = of_clk_get_by_clkspec(clkspec);
+ clk = __of_clk_get_by_clkspec(clkspec, dev_id, con_id);
of_node_put(clkspec.np);
return clk;
@@ -76,12 +85,7 @@ static struct clk *__of_clk_get(struct device_node *np, int
index)
struct clk *of_clk_get(struct device_node *np, int index)
{
- struct clk *clk = __of_clk_get(np, index);
-
- if (!IS_ERR(clk))
- clk =
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
* Tero Kristo [150203 00:49]: > On 02/03/2015 09:03 AM, Tomeu Vizoso wrote: > > > >I think you got it right, just wanted to mention that we can and > >probably should make the clk_get_parent_* calls in the consumer API to > >return per-user clk instances but that we need to make sure first that > >callers call clk_put afterwards. > > > >This should also allow us to remove the reference to struct clk from > >clk_hw, which is at best awkward. > > For the DPLL code it should just be fine to be able to get the current > parent index (not parent clock handle), and read a parent clock rate based > on an arbitrary index (not just the current one.) I don't think there is any > other need for having the clk_ref / clk_bypass clock handles around. I'd like to avoid the situation where the children have know that certain parent index and parent rate means bypass mode for both parent and children. Maybe we can hide that knowledge into some Linux generic PLL code so the children can get the PLL output as a mux clock. For a PLL, there can be three mux clock outputs: refclk*multi/div, bypass clock, or no output. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/03/2015 09:03 AM, Tomeu Vizoso wrote:
On 02/02/2015 11:41 PM, Mike Turquette wrote:
Quoting Tero Kristo (2015-02-02 11:32:01)
On 02/01/2015 11:24 PM, Mike Turquette wrote:
Quoting Tomeu Vizoso (2015-01-23 03:03:30)
Moves clock state to struct clk_core, but takes care to change as little API as
possible.
struct clk_hw still has a pointer to a struct clk, which is the
implementation's per-user clk instance, for backwards compatibility.
The struct clk that clk_get_parent() returns isn't owned by the caller, but by
the clock implementation, so the former shouldn't call clk_put() on it.
Because some boards in mach-omap2 still register clocks statically, their clock
registration had to be updated to take into account that the clock information
is stored in struct clk_core now.
Tero, Paul & Tony,
Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and
struct dpll_data, namely this snippet from
arch/arm/mach-omap2/dpll3xxx.c:
parent = __clk_get_parent(hw->clk);
if (__clk_get_rate(hw->clk) == __clk_get_rate(dd->clk_bypass)) {
WARN(parent != dd->clk_bypass,
"here0, parent name is %s, bypass name is
%s\n",
__clk_get_name(parent),
__clk_get_name(dd->clk_bypass));
r = _omap3_noncore_dpll_bypass(clk);
} else {
WARN(parent != dd->clk_ref,
"here1, parent name is %s, ref name is %s\n",
__clk_get_name(parent),
__clk_get_name(dd->clk_ref));
r = _omap3_noncore_dpll_lock(clk);
}
struct dpll_data has members clk_ref and clk_bypass which are struct clk
pointers. This was always a bit of a violation of the clk.h contract
since drivers are not supposed to deref struct clk pointers. Now that we
generate unique pointers for each call to clk_get (clk_ref & clk_bypass
are populated by of_clk_get in ti_clk_register_dpll) then the pointer
comparisons above will never be equal (even if they resolve down to the
same struct clk_core). I added the verbose traces to the WARNs above to
illustrate the point: the names are always the same but the pointers
differ.
AFAICT this doesn't break anything, but booting on OMAP3+ results in
noisy WARNs.
I think the correct fix is to replace clk_bypass and clk_ref pointers
with a simple integer parent_index. In fact we already have this index.
See how the pointers are populated in ti_clk_register_dpll:
The problem is we still need to be able to get runtime parent clock
rates (the parent rate may change also), so simple index value is not
sufficient. We need a handle of some sort to the bypass/ref clocks. The
DPLL code generally requires knowledge of the bypass + reference clock
rates to work properly, as it calculates the M/N values based on these.
We can maybe introduce something like of_clk_get_parent_rate, as we have
analogous stuff for getting parent names and indexes. Without
introducing a new helper you could probably just do:
clk_ref = clk_get_parent_by_index(dpll_clk, 0);
ref_rate = clk_get_rate(clk_ref);
clk_bypass = clk_get_parent_by_index(dpll_clk, 1);
bypass_rate = clk_get_rate(clk_bypass);
Currently the semantics around this call are weird. It seems like it
would create a new struct clk pointer but it does not. So don't call
clk_put on clk_ref and clk_bypass yet. That might change in the future
as we iron out this brave new world that we all live in. Probably best
to leave a FIXME in there.
Stephen & Tomeu, let me know if I got any of that wrong.
I think you got it right, just wanted to mention that we can and
probably should make the clk_get_parent_* calls in the consumer API to
return per-user clk instances but that we need to make sure first that
callers call clk_put afterwards.
This should also allow us to remove the reference to struct clk from
clk_hw, which is at best awkward.
Regards,
For the DPLL code it should just be fine to be able to get the current
parent index (not parent clock handle), and read a parent clock rate
based on an arbitrary index (not just the current one.) I don't think
there is any other need for having the clk_ref / clk_bypass clock
handles around.
-Tero
Tomeu
Shall I change the DPLL code to check against clk_hw pointers or what is
the preferred approach here? The patch at the end does this and fixes
the dpll related warnings.
Yes, for now that is fine, but feels a bit hacky to me. I don't know
honestly, let me sleep on it. Anyways for 3.20 that is perfectly fine
but we might want to switch to something like the scheme above.
Btw, the rate constraints patch broke boot for me completely, but sounds
like you reverted it already.
Fixed with Stephen's patch from last week. Thanks for dealing with all
the breakage so promptly. It has helped a lot!
Regards,
Mike
-Tero
Author: Tero Kristo
Date:
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
* Tero Kristo t-kri...@ti.com [150203 00:49]: On 02/03/2015 09:03 AM, Tomeu Vizoso wrote: I think you got it right, just wanted to mention that we can and probably should make the clk_get_parent_* calls in the consumer API to return per-user clk instances but that we need to make sure first that callers call clk_put afterwards. This should also allow us to remove the reference to struct clk from clk_hw, which is at best awkward. For the DPLL code it should just be fine to be able to get the current parent index (not parent clock handle), and read a parent clock rate based on an arbitrary index (not just the current one.) I don't think there is any other need for having the clk_ref / clk_bypass clock handles around. I'd like to avoid the situation where the children have know that certain parent index and parent rate means bypass mode for both parent and children. Maybe we can hide that knowledge into some Linux generic PLL code so the children can get the PLL output as a mux clock. For a PLL, there can be three mux clock outputs: refclk*multi/div, bypass clock, or no output. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/03/2015 09:03 AM, Tomeu Vizoso wrote:
On 02/02/2015 11:41 PM, Mike Turquette wrote:
Quoting Tero Kristo (2015-02-02 11:32:01)
On 02/01/2015 11:24 PM, Mike Turquette wrote:
Quoting Tomeu Vizoso (2015-01-23 03:03:30)
Moves clock state to struct clk_core, but takes care to change as little API as
possible.
struct clk_hw still has a pointer to a struct clk, which is the
implementation's per-user clk instance, for backwards compatibility.
The struct clk that clk_get_parent() returns isn't owned by the caller, but by
the clock implementation, so the former shouldn't call clk_put() on it.
Because some boards in mach-omap2 still register clocks statically, their clock
registration had to be updated to take into account that the clock information
is stored in struct clk_core now.
Tero, Paul Tony,
Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and
struct dpll_data, namely this snippet from
arch/arm/mach-omap2/dpll3xxx.c:
parent = __clk_get_parent(hw-clk);
if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) {
WARN(parent != dd-clk_bypass,
here0, parent name is %s, bypass name is
%s\n,
__clk_get_name(parent),
__clk_get_name(dd-clk_bypass));
r = _omap3_noncore_dpll_bypass(clk);
} else {
WARN(parent != dd-clk_ref,
here1, parent name is %s, ref name is %s\n,
__clk_get_name(parent),
__clk_get_name(dd-clk_ref));
r = _omap3_noncore_dpll_lock(clk);
}
struct dpll_data has members clk_ref and clk_bypass which are struct clk
pointers. This was always a bit of a violation of the clk.h contract
since drivers are not supposed to deref struct clk pointers. Now that we
generate unique pointers for each call to clk_get (clk_ref clk_bypass
are populated by of_clk_get in ti_clk_register_dpll) then the pointer
comparisons above will never be equal (even if they resolve down to the
same struct clk_core). I added the verbose traces to the WARNs above to
illustrate the point: the names are always the same but the pointers
differ.
AFAICT this doesn't break anything, but booting on OMAP3+ results in
noisy WARNs.
I think the correct fix is to replace clk_bypass and clk_ref pointers
with a simple integer parent_index. In fact we already have this index.
See how the pointers are populated in ti_clk_register_dpll:
The problem is we still need to be able to get runtime parent clock
rates (the parent rate may change also), so simple index value is not
sufficient. We need a handle of some sort to the bypass/ref clocks. The
DPLL code generally requires knowledge of the bypass + reference clock
rates to work properly, as it calculates the M/N values based on these.
We can maybe introduce something like of_clk_get_parent_rate, as we have
analogous stuff for getting parent names and indexes. Without
introducing a new helper you could probably just do:
clk_ref = clk_get_parent_by_index(dpll_clk, 0);
ref_rate = clk_get_rate(clk_ref);
clk_bypass = clk_get_parent_by_index(dpll_clk, 1);
bypass_rate = clk_get_rate(clk_bypass);
Currently the semantics around this call are weird. It seems like it
would create a new struct clk pointer but it does not. So don't call
clk_put on clk_ref and clk_bypass yet. That might change in the future
as we iron out this brave new world that we all live in. Probably best
to leave a FIXME in there.
Stephen Tomeu, let me know if I got any of that wrong.
I think you got it right, just wanted to mention that we can and
probably should make the clk_get_parent_* calls in the consumer API to
return per-user clk instances but that we need to make sure first that
callers call clk_put afterwards.
This should also allow us to remove the reference to struct clk from
clk_hw, which is at best awkward.
Regards,
For the DPLL code it should just be fine to be able to get the current
parent index (not parent clock handle), and read a parent clock rate
based on an arbitrary index (not just the current one.) I don't think
there is any other need for having the clk_ref / clk_bypass clock
handles around.
-Tero
Tomeu
Shall I change the DPLL code to check against clk_hw pointers or what is
the preferred approach here? The patch at the end does this and fixes
the dpll related warnings.
Yes, for now that is fine, but feels a bit hacky to me. I don't know
honestly, let me sleep on it. Anyways for 3.20 that is perfectly fine
but we might want to switch to something like the scheme above.
Btw, the rate constraints patch broke boot for me completely, but sounds
like you reverted it already.
Fixed with Stephen's patch from last week. Thanks for dealing with all
the breakage so promptly. It has helped a lot!
Regards,
Mike
-Tero
Author: Tero Kristo t-kri...@ti.com
Date:
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/02/2015 11:41 PM, Mike Turquette wrote:
> Quoting Tero Kristo (2015-02-02 11:32:01)
>> On 02/01/2015 11:24 PM, Mike Turquette wrote:
>>> Quoting Tomeu Vizoso (2015-01-23 03:03:30)
Moves clock state to struct clk_core, but takes care to change as little
API as
possible.
struct clk_hw still has a pointer to a struct clk, which is the
implementation's per-user clk instance, for backwards compatibility.
The struct clk that clk_get_parent() returns isn't owned by the caller,
but by
the clock implementation, so the former shouldn't call clk_put() on it.
Because some boards in mach-omap2 still register clocks statically, their
clock
registration had to be updated to take into account that the clock
information
is stored in struct clk_core now.
>>>
>>> Tero, Paul & Tony,
>>>
>>> Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and
>>> struct dpll_data, namely this snippet from
>>> arch/arm/mach-omap2/dpll3xxx.c:
>>>
>>> parent = __clk_get_parent(hw->clk);
>>>
>>> if (__clk_get_rate(hw->clk) == __clk_get_rate(dd->clk_bypass)) {
>>> WARN(parent != dd->clk_bypass,
>>> "here0, parent name is %s, bypass name is
>>> %s\n",
>>> __clk_get_name(parent),
>>> __clk_get_name(dd->clk_bypass));
>>> r = _omap3_noncore_dpll_bypass(clk);
>>> } else {
>>> WARN(parent != dd->clk_ref,
>>> "here1, parent name is %s, ref name is
>>> %s\n",
>>> __clk_get_name(parent),
>>> __clk_get_name(dd->clk_ref));
>>> r = _omap3_noncore_dpll_lock(clk);
>>> }
>>>
>>> struct dpll_data has members clk_ref and clk_bypass which are struct clk
>>> pointers. This was always a bit of a violation of the clk.h contract
>>> since drivers are not supposed to deref struct clk pointers. Now that we
>>> generate unique pointers for each call to clk_get (clk_ref & clk_bypass
>>> are populated by of_clk_get in ti_clk_register_dpll) then the pointer
>>> comparisons above will never be equal (even if they resolve down to the
>>> same struct clk_core). I added the verbose traces to the WARNs above to
>>> illustrate the point: the names are always the same but the pointers
>>> differ.
>>>
>>> AFAICT this doesn't break anything, but booting on OMAP3+ results in
>>> noisy WARNs.
>>>
>>> I think the correct fix is to replace clk_bypass and clk_ref pointers
>>> with a simple integer parent_index. In fact we already have this index.
>>> See how the pointers are populated in ti_clk_register_dpll:
>>
>> The problem is we still need to be able to get runtime parent clock
>> rates (the parent rate may change also), so simple index value is not
>> sufficient. We need a handle of some sort to the bypass/ref clocks. The
>> DPLL code generally requires knowledge of the bypass + reference clock
>> rates to work properly, as it calculates the M/N values based on these.
>
> We can maybe introduce something like of_clk_get_parent_rate, as we have
> analogous stuff for getting parent names and indexes. Without
> introducing a new helper you could probably just do:
>
> clk_ref = clk_get_parent_by_index(dpll_clk, 0);
> ref_rate = clk_get_rate(clk_ref);
>
> clk_bypass = clk_get_parent_by_index(dpll_clk, 1);
> bypass_rate = clk_get_rate(clk_bypass);
>
> Currently the semantics around this call are weird. It seems like it
> would create a new struct clk pointer but it does not. So don't call
> clk_put on clk_ref and clk_bypass yet. That might change in the future
> as we iron out this brave new world that we all live in. Probably best
> to leave a FIXME in there.
>
> Stephen & Tomeu, let me know if I got any of that wrong.
I think you got it right, just wanted to mention that we can and
probably should make the clk_get_parent_* calls in the consumer API to
return per-user clk instances but that we need to make sure first that
callers call clk_put afterwards.
This should also allow us to remove the reference to struct clk from
clk_hw, which is at best awkward.
Regards,
Tomeu
>>
>> Shall I change the DPLL code to check against clk_hw pointers or what is
>> the preferred approach here? The patch at the end does this and fixes
>> the dpll related warnings.
>
> Yes, for now that is fine, but feels a bit hacky to me. I don't know
> honestly, let me sleep on it. Anyways for 3.20 that is perfectly fine
> but we might want to switch to something like the scheme above.
>
>>
>> Btw, the rate constraints patch broke boot for me completely, but sounds
>> like you reverted it already.
>
> Fixed with Stephen's patch from last week. Thanks for dealing with all
> the breakage so promptly. It has helped a lot!
>
> Regards,
> Mike
>
>>
>> -Tero
>>
>>
>>
>> Author: Tero Kristo
>> Date: Mon Feb 2 17:19:17
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
* Mike Turquette [150202 14:51]: > Quoting Tony Lindgren (2015-02-02 12:44:02) > > > > Thanks Tero, looks like your fix fixes all the issues I'm seeing with > > commit 59cf3fcf9baf. That is noisy dmesg, dpll_abe_ck not locking > > on 4430sdp, and off-idle not working for omap3. > > > > I could not get the patch to apply, below is what I applied manually. > > > > Mike, If possible, maybe fold this into 59cf3fcf9baf? It applies with > > some fuzz on that too. And inn that case, please feel also to add the > > following for Tomeu's patch: > > > > Tested-by: Tony Lindgren > > Done and done. Things look good in my testing. I've pushed another > branch out to the mirrors and hopefully the autobuild/autoboot testing > will give us the green light. Thanks I just checked that your updated branch works for me now. > This implementation can be revisited probably after 3.19 comes out if > Tero doesn't like using clk_hw directly, or if we provide a better > interface. Sounds like what Tero is saying also relates to knowing if the parent clock is in bypass mode or not in addition to the parent rate. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/02/15 14:41, Mike Turquette wrote: > Quoting Tero Kristo (2015-02-02 11:32:01) >> On 02/01/2015 11:24 PM, Mike Turquette wrote: >>> >>> AFAICT this doesn't break anything, but booting on OMAP3+ results in >>> noisy WARNs. >>> >>> I think the correct fix is to replace clk_bypass and clk_ref pointers >>> with a simple integer parent_index. In fact we already have this index. >>> See how the pointers are populated in ti_clk_register_dpll: >> The problem is we still need to be able to get runtime parent clock >> rates (the parent rate may change also), so simple index value is not >> sufficient. We need a handle of some sort to the bypass/ref clocks. The >> DPLL code generally requires knowledge of the bypass + reference clock >> rates to work properly, as it calculates the M/N values based on these. > We can maybe introduce something like of_clk_get_parent_rate, as we have > analogous stuff for getting parent names and indexes. Without > introducing a new helper you could probably just do: > > clk_ref = clk_get_parent_by_index(dpll_clk, 0); > ref_rate = clk_get_rate(clk_ref); > > clk_bypass = clk_get_parent_by_index(dpll_clk, 1); > bypass_rate = clk_get_rate(clk_bypass); > > Currently the semantics around this call are weird. It seems like it > would create a new struct clk pointer but it does not. So don't call > clk_put on clk_ref and clk_bypass yet. That might change in the future > as we iron out this brave new world that we all live in. Probably best > to leave a FIXME in there. > > Stephen & Tomeu, let me know if I got any of that wrong. The plan is to make clk_get_parent_by_index() return a clk_hw pointer instead of a clk pointer (probably with a new clk_get_parent_hw_by_index() API). Then drivers that are clk providers can deal in struct clk_hw and clk consumers can deal in struct clk, nicely splitting the API between consumers and providers on the structures they use to interact with the framework. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
Quoting Stephen Boyd (2015-02-02 14:35:59) > On 02/02/15 13:31, Julia Lawall wrote: > > > > On Mon, 2 Feb 2015, Stephen Boyd wrote: > > > >> Julia, > >> > >> Is there a way we can write a coccinelle script to check for this? The > >> goal being to find all drivers that are comparing struct clk pointers or > >> attempting to dereference them. There are probably other frameworks that > >> could use the same type of check (regulator, gpiod, reset, pwm, etc.). > >> Probably anything that has a get/put API. > > Comparing or dereferencing pointers of a particular type should be > > straightforward to check for. Is there an example of how to use the > > parent_index value to fix the problem? > > > > I'm not sure how to fix this case with parent_index values generically. > I imagine it would be highly specific to the surrounding code to the > point where we couldn't fix it automatically. For example, if it's a clk > consumer (not provider like in this case) using an index wouldn't be the > right fix. We would need some sort of clk API that we don't currently > have and I would wonder why clock consumers even care to compare such > pointers in the first place. Ack. Is there precedent for a "Don't do that" kind of response? Regards, Mike > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
Quoting Tony Lindgren (2015-02-02 12:44:02)
> * Tero Kristo [150202 11:35]:
> > On 02/01/2015 11:24 PM, Mike Turquette wrote:
> > >Quoting Tomeu Vizoso (2015-01-23 03:03:30)
> > >
> > >AFAICT this doesn't break anything, but booting on OMAP3+ results in
> > >noisy WARNs.
> > >
> > >I think the correct fix is to replace clk_bypass and clk_ref pointers
> > >with a simple integer parent_index. In fact we already have this index.
> > >See how the pointers are populated in ti_clk_register_dpll:
> >
> > The problem is we still need to be able to get runtime parent clock rates
> > (the parent rate may change also), so simple index value is not sufficient.
> > We need a handle of some sort to the bypass/ref clocks. The DPLL code
> > generally requires knowledge of the bypass + reference clock rates to work
> > properly, as it calculates the M/N values based on these.
> >
> > Shall I change the DPLL code to check against clk_hw pointers or what is the
> > preferred approach here? The patch at the end does this and fixes the dpll
> > related warnings.
> >
> > Btw, the rate constraints patch broke boot for me completely, but sounds
> > like you reverted it already.
>
> Thanks Tero, looks like your fix fixes all the issues I'm seeing with
> commit 59cf3fcf9baf. That is noisy dmesg, dpll_abe_ck not locking
> on 4430sdp, and off-idle not working for omap3.
>
> I could not get the patch to apply, below is what I applied manually.
>
> Mike, If possible, maybe fold this into 59cf3fcf9baf? It applies with
> some fuzz on that too. And inn that case, please feel also to add the
> following for Tomeu's patch:
>
> Tested-by: Tony Lindgren
Done and done. Things look good in my testing. I've pushed another
branch out to the mirrors and hopefully the autobuild/autoboot testing
will give us the green light.
This implementation can be revisited probably after 3.19 comes out if
Tero doesn't like using clk_hw directly, or if we provide a better
interface.
Thanks,
Mike
>
> 8<
> From: Tero Kristo
> Date: Mon, 2 Feb 2015 12:17:00 -0800
> Subject: [PATCH] ARM: OMAP3+: clock: dpll: fix logic for comparing parent
> clocks
>
> DPLL code uses reference and bypass clock pointers for determining runtime
> properties for these clocks, like parent clock rates.
> As clock API now returns per-user clock structs, using a global handle
> in the clock driver code does not work properly anymore. Fix this by
> using the clk_hw instead, and comparing this against the parents.
>
> Fixes: 59cf3fcf9baf ("clk: Make clk API return per-user struct clk instances")
> Signed-off-by: Tero Kristo
> [t...@atomide.com: updated to apply]
> Signed-off-by: Tony Lindgren
>
> --- a/arch/arm/mach-omap2/dpll3xxx.c
> +++ b/arch/arm/mach-omap2/dpll3xxx.c
> @@ -410,7 +410,7 @@ int omap3_noncore_dpll_enable(struct clk_hw *hw)
> struct clk_hw_omap *clk = to_clk_hw_omap(hw);
> int r;
> struct dpll_data *dd;
> - struct clk *parent;
> + struct clk_hw *parent;
>
> dd = clk->dpll_data;
> if (!dd)
> @@ -427,13 +427,13 @@ int omap3_noncore_dpll_enable(struct clk_hw *hw)
> }
> }
>
> - parent = __clk_get_parent(hw->clk);
> + parent = __clk_get_hw(__clk_get_parent(hw->clk));
>
> if (__clk_get_rate(hw->clk) == __clk_get_rate(dd->clk_bypass)) {
> - WARN_ON(parent != dd->clk_bypass);
> + WARN_ON(parent != __clk_get_hw(dd->clk_bypass));
> r = _omap3_noncore_dpll_bypass(clk);
> } else {
> - WARN_ON(parent != dd->clk_ref);
> + WARN_ON(parent != __clk_get_hw(dd->clk_ref));
> r = _omap3_noncore_dpll_lock(clk);
> }
>
> @@ -549,7 +549,8 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw,
> unsigned long rate,
> if (!dd)
> return -EINVAL;
>
> - if (__clk_get_parent(hw->clk) != dd->clk_ref)
> + if (__clk_get_hw(__clk_get_parent(hw->clk)) !=
> + __clk_get_hw(dd->clk_ref))
> return -EINVAL;
>
> if (dd->last_rounded_rate == 0)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
Quoting Tero Kristo (2015-02-02 11:32:01)
> On 02/01/2015 11:24 PM, Mike Turquette wrote:
> > Quoting Tomeu Vizoso (2015-01-23 03:03:30)
> >> Moves clock state to struct clk_core, but takes care to change as little
> >> API as
> >> possible.
> >>
> >> struct clk_hw still has a pointer to a struct clk, which is the
> >> implementation's per-user clk instance, for backwards compatibility.
> >>
> >> The struct clk that clk_get_parent() returns isn't owned by the caller,
> >> but by
> >> the clock implementation, so the former shouldn't call clk_put() on it.
> >>
> >> Because some boards in mach-omap2 still register clocks statically, their
> >> clock
> >> registration had to be updated to take into account that the clock
> >> information
> >> is stored in struct clk_core now.
> >
> > Tero, Paul & Tony,
> >
> > Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and
> > struct dpll_data, namely this snippet from
> > arch/arm/mach-omap2/dpll3xxx.c:
> >
> > parent = __clk_get_parent(hw->clk);
> >
> > if (__clk_get_rate(hw->clk) == __clk_get_rate(dd->clk_bypass)) {
> > WARN(parent != dd->clk_bypass,
> > "here0, parent name is %s, bypass name is
> > %s\n",
> > __clk_get_name(parent),
> > __clk_get_name(dd->clk_bypass));
> > r = _omap3_noncore_dpll_bypass(clk);
> > } else {
> > WARN(parent != dd->clk_ref,
> > "here1, parent name is %s, ref name is
> > %s\n",
> > __clk_get_name(parent),
> > __clk_get_name(dd->clk_ref));
> > r = _omap3_noncore_dpll_lock(clk);
> > }
> >
> > struct dpll_data has members clk_ref and clk_bypass which are struct clk
> > pointers. This was always a bit of a violation of the clk.h contract
> > since drivers are not supposed to deref struct clk pointers. Now that we
> > generate unique pointers for each call to clk_get (clk_ref & clk_bypass
> > are populated by of_clk_get in ti_clk_register_dpll) then the pointer
> > comparisons above will never be equal (even if they resolve down to the
> > same struct clk_core). I added the verbose traces to the WARNs above to
> > illustrate the point: the names are always the same but the pointers
> > differ.
> >
> > AFAICT this doesn't break anything, but booting on OMAP3+ results in
> > noisy WARNs.
> >
> > I think the correct fix is to replace clk_bypass and clk_ref pointers
> > with a simple integer parent_index. In fact we already have this index.
> > See how the pointers are populated in ti_clk_register_dpll:
>
> The problem is we still need to be able to get runtime parent clock
> rates (the parent rate may change also), so simple index value is not
> sufficient. We need a handle of some sort to the bypass/ref clocks. The
> DPLL code generally requires knowledge of the bypass + reference clock
> rates to work properly, as it calculates the M/N values based on these.
We can maybe introduce something like of_clk_get_parent_rate, as we have
analogous stuff for getting parent names and indexes. Without
introducing a new helper you could probably just do:
clk_ref = clk_get_parent_by_index(dpll_clk, 0);
ref_rate = clk_get_rate(clk_ref);
clk_bypass = clk_get_parent_by_index(dpll_clk, 1);
bypass_rate = clk_get_rate(clk_bypass);
Currently the semantics around this call are weird. It seems like it
would create a new struct clk pointer but it does not. So don't call
clk_put on clk_ref and clk_bypass yet. That might change in the future
as we iron out this brave new world that we all live in. Probably best
to leave a FIXME in there.
Stephen & Tomeu, let me know if I got any of that wrong.
>
> Shall I change the DPLL code to check against clk_hw pointers or what is
> the preferred approach here? The patch at the end does this and fixes
> the dpll related warnings.
Yes, for now that is fine, but feels a bit hacky to me. I don't know
honestly, let me sleep on it. Anyways for 3.20 that is perfectly fine
but we might want to switch to something like the scheme above.
>
> Btw, the rate constraints patch broke boot for me completely, but sounds
> like you reverted it already.
Fixed with Stephen's patch from last week. Thanks for dealing with all
the breakage so promptly. It has helped a lot!
Regards,
Mike
>
> -Tero
>
>
>
> Author: Tero Kristo
> Date: Mon Feb 2 17:19:17 2015 +0200
>
> ARM: OMAP3+: clock: dpll: fix logic for comparing parent clocks
>
> DPLL code uses reference and bypass clock pointers for determining
> runtime
> properties for these clocks, like parent clock rates.
>
> As clock API now returns per-user clock structs, using a global handle
> in the clock driver code does not work properly anymore. Fix this by
> using the clk_hw instead, and comparing this against the parents.
>
>
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/02/15 13:31, Julia Lawall wrote: > > On Mon, 2 Feb 2015, Stephen Boyd wrote: > >> Julia, >> >> Is there a way we can write a coccinelle script to check for this? The >> goal being to find all drivers that are comparing struct clk pointers or >> attempting to dereference them. There are probably other frameworks that >> could use the same type of check (regulator, gpiod, reset, pwm, etc.). >> Probably anything that has a get/put API. > Comparing or dereferencing pointers of a particular type should be > straightforward to check for. Is there an example of how to use the > parent_index value to fix the problem? > I'm not sure how to fix this case with parent_index values generically. I imagine it would be highly specific to the surrounding code to the point where we couldn't fix it automatically. For example, if it's a clk consumer (not provider like in this case) using an index wouldn't be the right fix. We would need some sort of clk API that we don't currently have and I would wonder why clock consumers even care to compare such pointers in the first place. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On Mon, 2 Feb 2015, Stephen Boyd wrote:
> On 02/01/15 13:24, Mike Turquette wrote:
> > Quoting Tomeu Vizoso (2015-01-23 03:03:30)
> >> Moves clock state to struct clk_core, but takes care to change as little
> >> API as
> >> possible.
> >>
> >> struct clk_hw still has a pointer to a struct clk, which is the
> >> implementation's per-user clk instance, for backwards compatibility.
> >>
> >> The struct clk that clk_get_parent() returns isn't owned by the caller,
> >> but by
> >> the clock implementation, so the former shouldn't call clk_put() on it.
> >>
> >> Because some boards in mach-omap2 still register clocks statically, their
> >> clock
> >> registration had to be updated to take into account that the clock
> >> information
> >> is stored in struct clk_core now.
> > Tero, Paul & Tony,
> >
> > Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and
> > struct dpll_data, namely this snippet from
> > arch/arm/mach-omap2/dpll3xxx.c:
> >
> > parent = __clk_get_parent(hw->clk);
> >
> > if (__clk_get_rate(hw->clk) == __clk_get_rate(dd->clk_bypass)) {
> > WARN(parent != dd->clk_bypass,
> > "here0, parent name is %s, bypass name is
> > %s\n",
> > __clk_get_name(parent),
> > __clk_get_name(dd->clk_bypass));
> > r = _omap3_noncore_dpll_bypass(clk);
> > } else {
> > WARN(parent != dd->clk_ref,
> > "here1, parent name is %s, ref name is
> > %s\n",
> > __clk_get_name(parent),
> > __clk_get_name(dd->clk_ref));
> > r = _omap3_noncore_dpll_lock(clk);
> > }
> >
> > struct dpll_data has members clk_ref and clk_bypass which are struct clk
> > pointers. This was always a bit of a violation of the clk.h contract
> > since drivers are not supposed to deref struct clk pointers.
>
> Julia,
>
> Is there a way we can write a coccinelle script to check for this? The
> goal being to find all drivers that are comparing struct clk pointers or
> attempting to dereference them. There are probably other frameworks that
> could use the same type of check (regulator, gpiod, reset, pwm, etc.).
> Probably anything that has a get/put API.
Comparing or dereferencing pointers of a particular type should be
straightforward to check for. Is there an example of how to use the
parent_index value to fix the problem?
julia
>
> -Stephen
>
> > Now that we
> > generate unique pointers for each call to clk_get (clk_ref & clk_bypass
> > are populated by of_clk_get in ti_clk_register_dpll) then the pointer
> > comparisons above will never be equal (even if they resolve down to the
> > same struct clk_core). I added the verbose traces to the WARNs above to
> > illustrate the point: the names are always the same but the pointers
> > differ.
> >
> > AFAICT this doesn't break anything, but booting on OMAP3+ results in
> > noisy WARNs.
> >
> > I think the correct fix is to replace clk_bypass and clk_ref pointers
> > with a simple integer parent_index. In fact we already have this index.
> > See how the pointers are populated in ti_clk_register_dpll:
> >
> >
> > dd->clk_ref = of_clk_get(node, 0);
> > dd->clk_bypass = of_clk_get(node, 1);
> >
> > Tony, the same problem affects the FAPLL code which copy/pastes some of
> > the DPLL code.
> >
> > Thoughts?
> >
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
* Tero Kristo [150202 11:35]:
> On 02/01/2015 11:24 PM, Mike Turquette wrote:
> >Quoting Tomeu Vizoso (2015-01-23 03:03:30)
> >
> >AFAICT this doesn't break anything, but booting on OMAP3+ results in
> >noisy WARNs.
> >
> >I think the correct fix is to replace clk_bypass and clk_ref pointers
> >with a simple integer parent_index. In fact we already have this index.
> >See how the pointers are populated in ti_clk_register_dpll:
>
> The problem is we still need to be able to get runtime parent clock rates
> (the parent rate may change also), so simple index value is not sufficient.
> We need a handle of some sort to the bypass/ref clocks. The DPLL code
> generally requires knowledge of the bypass + reference clock rates to work
> properly, as it calculates the M/N values based on these.
>
> Shall I change the DPLL code to check against clk_hw pointers or what is the
> preferred approach here? The patch at the end does this and fixes the dpll
> related warnings.
>
> Btw, the rate constraints patch broke boot for me completely, but sounds
> like you reverted it already.
Thanks Tero, looks like your fix fixes all the issues I'm seeing with
commit 59cf3fcf9baf. That is noisy dmesg, dpll_abe_ck not locking
on 4430sdp, and off-idle not working for omap3.
I could not get the patch to apply, below is what I applied manually.
Mike, If possible, maybe fold this into 59cf3fcf9baf? It applies with
some fuzz on that too. And inn that case, please feel also to add the
following for Tomeu's patch:
Tested-by: Tony Lindgren
8<
From: Tero Kristo
Date: Mon, 2 Feb 2015 12:17:00 -0800
Subject: [PATCH] ARM: OMAP3+: clock: dpll: fix logic for comparing parent clocks
DPLL code uses reference and bypass clock pointers for determining runtime
properties for these clocks, like parent clock rates.
As clock API now returns per-user clock structs, using a global handle
in the clock driver code does not work properly anymore. Fix this by
using the clk_hw instead, and comparing this against the parents.
Fixes: 59cf3fcf9baf ("clk: Make clk API return per-user struct clk instances")
Signed-off-by: Tero Kristo
[t...@atomide.com: updated to apply]
Signed-off-by: Tony Lindgren
--- a/arch/arm/mach-omap2/dpll3xxx.c
+++ b/arch/arm/mach-omap2/dpll3xxx.c
@@ -410,7 +410,7 @@ int omap3_noncore_dpll_enable(struct clk_hw *hw)
struct clk_hw_omap *clk = to_clk_hw_omap(hw);
int r;
struct dpll_data *dd;
- struct clk *parent;
+ struct clk_hw *parent;
dd = clk->dpll_data;
if (!dd)
@@ -427,13 +427,13 @@ int omap3_noncore_dpll_enable(struct clk_hw *hw)
}
}
- parent = __clk_get_parent(hw->clk);
+ parent = __clk_get_hw(__clk_get_parent(hw->clk));
if (__clk_get_rate(hw->clk) == __clk_get_rate(dd->clk_bypass)) {
- WARN_ON(parent != dd->clk_bypass);
+ WARN_ON(parent != __clk_get_hw(dd->clk_bypass));
r = _omap3_noncore_dpll_bypass(clk);
} else {
- WARN_ON(parent != dd->clk_ref);
+ WARN_ON(parent != __clk_get_hw(dd->clk_ref));
r = _omap3_noncore_dpll_lock(clk);
}
@@ -549,7 +549,8 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw, unsigned
long rate,
if (!dd)
return -EINVAL;
- if (__clk_get_parent(hw->clk) != dd->clk_ref)
+ if (__clk_get_hw(__clk_get_parent(hw->clk)) !=
+ __clk_get_hw(dd->clk_ref))
return -EINVAL;
if (dd->last_rounded_rate == 0)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/01/15 13:24, Mike Turquette wrote:
> Quoting Tomeu Vizoso (2015-01-23 03:03:30)
>> Moves clock state to struct clk_core, but takes care to change as little API
>> as
>> possible.
>>
>> struct clk_hw still has a pointer to a struct clk, which is the
>> implementation's per-user clk instance, for backwards compatibility.
>>
>> The struct clk that clk_get_parent() returns isn't owned by the caller, but
>> by
>> the clock implementation, so the former shouldn't call clk_put() on it.
>>
>> Because some boards in mach-omap2 still register clocks statically, their
>> clock
>> registration had to be updated to take into account that the clock
>> information
>> is stored in struct clk_core now.
> Tero, Paul & Tony,
>
> Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and
> struct dpll_data, namely this snippet from
> arch/arm/mach-omap2/dpll3xxx.c:
>
> parent = __clk_get_parent(hw->clk);
>
> if (__clk_get_rate(hw->clk) == __clk_get_rate(dd->clk_bypass)) {
> WARN(parent != dd->clk_bypass,
> "here0, parent name is %s, bypass name is
> %s\n",
> __clk_get_name(parent),
> __clk_get_name(dd->clk_bypass));
> r = _omap3_noncore_dpll_bypass(clk);
> } else {
> WARN(parent != dd->clk_ref,
> "here1, parent name is %s, ref name is %s\n",
> __clk_get_name(parent),
> __clk_get_name(dd->clk_ref));
> r = _omap3_noncore_dpll_lock(clk);
> }
>
> struct dpll_data has members clk_ref and clk_bypass which are struct clk
> pointers. This was always a bit of a violation of the clk.h contract
> since drivers are not supposed to deref struct clk pointers.
Julia,
Is there a way we can write a coccinelle script to check for this? The
goal being to find all drivers that are comparing struct clk pointers or
attempting to dereference them. There are probably other frameworks that
could use the same type of check (regulator, gpiod, reset, pwm, etc.).
Probably anything that has a get/put API.
-Stephen
> Now that we
> generate unique pointers for each call to clk_get (clk_ref & clk_bypass
> are populated by of_clk_get in ti_clk_register_dpll) then the pointer
> comparisons above will never be equal (even if they resolve down to the
> same struct clk_core). I added the verbose traces to the WARNs above to
> illustrate the point: the names are always the same but the pointers
> differ.
>
> AFAICT this doesn't break anything, but booting on OMAP3+ results in
> noisy WARNs.
>
> I think the correct fix is to replace clk_bypass and clk_ref pointers
> with a simple integer parent_index. In fact we already have this index.
> See how the pointers are populated in ti_clk_register_dpll:
>
>
> dd->clk_ref = of_clk_get(node, 0);
> dd->clk_bypass = of_clk_get(node, 1);
>
> Tony, the same problem affects the FAPLL code which copy/pastes some of
> the DPLL code.
>
> Thoughts?
>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/01/2015 11:24 PM, Mike Turquette wrote:
Quoting Tomeu Vizoso (2015-01-23 03:03:30)
Moves clock state to struct clk_core, but takes care to change as little API as
possible.
struct clk_hw still has a pointer to a struct clk, which is the
implementation's per-user clk instance, for backwards compatibility.
The struct clk that clk_get_parent() returns isn't owned by the caller, but by
the clock implementation, so the former shouldn't call clk_put() on it.
Because some boards in mach-omap2 still register clocks statically, their clock
registration had to be updated to take into account that the clock information
is stored in struct clk_core now.
Tero, Paul & Tony,
Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and
struct dpll_data, namely this snippet from
arch/arm/mach-omap2/dpll3xxx.c:
parent = __clk_get_parent(hw->clk);
if (__clk_get_rate(hw->clk) == __clk_get_rate(dd->clk_bypass)) {
WARN(parent != dd->clk_bypass,
"here0, parent name is %s, bypass name is
%s\n",
__clk_get_name(parent),
__clk_get_name(dd->clk_bypass));
r = _omap3_noncore_dpll_bypass(clk);
} else {
WARN(parent != dd->clk_ref,
"here1, parent name is %s, ref name is %s\n",
__clk_get_name(parent),
__clk_get_name(dd->clk_ref));
r = _omap3_noncore_dpll_lock(clk);
}
struct dpll_data has members clk_ref and clk_bypass which are struct clk
pointers. This was always a bit of a violation of the clk.h contract
since drivers are not supposed to deref struct clk pointers. Now that we
generate unique pointers for each call to clk_get (clk_ref & clk_bypass
are populated by of_clk_get in ti_clk_register_dpll) then the pointer
comparisons above will never be equal (even if they resolve down to the
same struct clk_core). I added the verbose traces to the WARNs above to
illustrate the point: the names are always the same but the pointers
differ.
AFAICT this doesn't break anything, but booting on OMAP3+ results in
noisy WARNs.
I think the correct fix is to replace clk_bypass and clk_ref pointers
with a simple integer parent_index. In fact we already have this index.
See how the pointers are populated in ti_clk_register_dpll:
The problem is we still need to be able to get runtime parent clock
rates (the parent rate may change also), so simple index value is not
sufficient. We need a handle of some sort to the bypass/ref clocks. The
DPLL code generally requires knowledge of the bypass + reference clock
rates to work properly, as it calculates the M/N values based on these.
Shall I change the DPLL code to check against clk_hw pointers or what is
the preferred approach here? The patch at the end does this and fixes
the dpll related warnings.
Btw, the rate constraints patch broke boot for me completely, but sounds
like you reverted it already.
-Tero
Author: Tero Kristo
Date: Mon Feb 2 17:19:17 2015 +0200
ARM: OMAP3+: clock: dpll: fix logic for comparing parent clocks
DPLL code uses reference and bypass clock pointers for determining
runtime
properties for these clocks, like parent clock rates.
As clock API now returns per-user clock structs, using a global handle
in the clock driver code does not work properly anymore. Fix this by
using the clk_hw instead, and comparing this against the parents.
Signed-off-by: Tero Kristo
Fixes: 59cf3fcf9baf ("clk: Make clk API return per-user struct clk
instances")
diff --git a/arch/arm/mach-omap2/dpll3xxx.c b/arch/arm/mach-omap2/dpll3xxx.c
index c2da2a0..49752d7 100644
--- a/arch/arm/mach-omap2/dpll3xxx.c
+++ b/arch/arm/mach-omap2/dpll3xxx.c
@@ -410,7 +410,7 @@ int omap3_noncore_dpll_enable(struct clk_hw *hw)
struct clk_hw_omap *clk = to_clk_hw_omap(hw);
int r;
struct dpll_data *dd;
- struct clk *parent;
+ struct clk_hw *parent;
dd = clk->dpll_data;
if (!dd)
@@ -427,13 +427,13 @@ int omap3_noncore_dpll_enable(struct clk_hw *hw)
}
}
- parent = __clk_get_parent(hw->clk);
+ parent = __clk_get_hw(__clk_get_parent(hw->clk));
if (__clk_get_rate(hw->clk) == __clk_get_rate(dd->clk_bypass)) {
- WARN_ON(parent != dd->clk_bypass);
+ WARN_ON(parent != __clk_get_hw(dd->clk_bypass));
r = _omap3_noncore_dpll_bypass(clk);
} else {
- WARN_ON(parent != dd->clk_ref);
+ WARN_ON(parent != __clk_get_hw(dd->clk_ref));
r = _omap3_noncore_dpll_lock(clk);
}
@@ -549,7 +549,8 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw,
unsigned long rate,
if (!dd)
return -EINVAL;
- if (__clk_get_parent(hw->clk) != dd->clk_ref)
+ if (__clk_get_hw(__clk_get_parent(hw->clk)) !=
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
* Mike Turquette [150201 13:27]:
> Quoting Tomeu Vizoso (2015-01-23 03:03:30)
> > Moves clock state to struct clk_core, but takes care to change as little
> > API as
> > possible.
> >
> > struct clk_hw still has a pointer to a struct clk, which is the
> > implementation's per-user clk instance, for backwards compatibility.
> >
> > The struct clk that clk_get_parent() returns isn't owned by the caller, but
> > by
> > the clock implementation, so the former shouldn't call clk_put() on it.
> >
> > Because some boards in mach-omap2 still register clocks statically, their
> > clock
> > registration had to be updated to take into account that the clock
> > information
> > is stored in struct clk_core now.
>
> Tero, Paul & Tony,
>
> Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and
> struct dpll_data, namely this snippet from
> arch/arm/mach-omap2/dpll3xxx.c:
>
> parent = __clk_get_parent(hw->clk);
>
> if (__clk_get_rate(hw->clk) == __clk_get_rate(dd->clk_bypass)) {
> WARN(parent != dd->clk_bypass,
> "here0, parent name is %s, bypass name is
> %s\n",
> __clk_get_name(parent),
> __clk_get_name(dd->clk_bypass));
> r = _omap3_noncore_dpll_bypass(clk);
> } else {
> WARN(parent != dd->clk_ref,
> "here1, parent name is %s, ref name is %s\n",
> __clk_get_name(parent),
> __clk_get_name(dd->clk_ref));
> r = _omap3_noncore_dpll_lock(clk);
> }
>
> struct dpll_data has members clk_ref and clk_bypass which are struct clk
> pointers. This was always a bit of a violation of the clk.h contract
> since drivers are not supposed to deref struct clk pointers. Now that we
> generate unique pointers for each call to clk_get (clk_ref & clk_bypass
> are populated by of_clk_get in ti_clk_register_dpll) then the pointer
> comparisons above will never be equal (even if they resolve down to the
> same struct clk_core). I added the verbose traces to the WARNs above to
> illustrate the point: the names are always the same but the pointers
> differ.
>
> AFAICT this doesn't break anything, but booting on OMAP3+ results in
> noisy WARNs.
Also on at least omap4 like I posted. So sounds like the check for
WARN is wrong but harmless. Paul & Tero, what do you want to do
about that?
> I think the correct fix is to replace clk_bypass and clk_ref pointers
> with a simple integer parent_index. In fact we already have this index.
> See how the pointers are populated in ti_clk_register_dpll:
>
>
> dd->clk_ref = of_clk_get(node, 0);
> dd->clk_bypass = of_clk_get(node, 1);
>
> Tony, the same problem affects the FAPLL code which copy/pastes some of
> the DPLL code.
>
> Thoughts?
Not seeing these warnings with dm186x as fapll.c does not use
dpll3xxx.c. This is because of the way the PLL's child synthesizers
need to also access the PLL registers for power and bypass mode.
Not related to the $subject bug, but to me it seems that we could
possibly have Linux generic PLL code if we add support for
parent_in_bypass_mode in addition to the parent_rate. This is because
the PLL can in theory generate the same rate both in bypass mode and
regular mode so parent_rate is not enough to tell it to the child
synthesizers. Not sure how the PLL registers enabling and disabling
it's children should be handled, maybe regmap would work there.
Regards,
Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
* Tero Kristo t-kri...@ti.com [150202 11:35]:
On 02/01/2015 11:24 PM, Mike Turquette wrote:
Quoting Tomeu Vizoso (2015-01-23 03:03:30)
AFAICT this doesn't break anything, but booting on OMAP3+ results in
noisy WARNs.
I think the correct fix is to replace clk_bypass and clk_ref pointers
with a simple integer parent_index. In fact we already have this index.
See how the pointers are populated in ti_clk_register_dpll:
The problem is we still need to be able to get runtime parent clock rates
(the parent rate may change also), so simple index value is not sufficient.
We need a handle of some sort to the bypass/ref clocks. The DPLL code
generally requires knowledge of the bypass + reference clock rates to work
properly, as it calculates the M/N values based on these.
Shall I change the DPLL code to check against clk_hw pointers or what is the
preferred approach here? The patch at the end does this and fixes the dpll
related warnings.
Btw, the rate constraints patch broke boot for me completely, but sounds
like you reverted it already.
Thanks Tero, looks like your fix fixes all the issues I'm seeing with
commit 59cf3fcf9baf. That is noisy dmesg, dpll_abe_ck not locking
on 4430sdp, and off-idle not working for omap3.
I could not get the patch to apply, below is what I applied manually.
Mike, If possible, maybe fold this into 59cf3fcf9baf? It applies with
some fuzz on that too. And inn that case, please feel also to add the
following for Tomeu's patch:
Tested-by: Tony Lindgren t...@atomide.com
8
From: Tero Kristo t-kri...@ti.com
Date: Mon, 2 Feb 2015 12:17:00 -0800
Subject: [PATCH] ARM: OMAP3+: clock: dpll: fix logic for comparing parent clocks
DPLL code uses reference and bypass clock pointers for determining runtime
properties for these clocks, like parent clock rates.
As clock API now returns per-user clock structs, using a global handle
in the clock driver code does not work properly anymore. Fix this by
using the clk_hw instead, and comparing this against the parents.
Fixes: 59cf3fcf9baf (clk: Make clk API return per-user struct clk instances)
Signed-off-by: Tero Kristo t-kri...@ti.com
[t...@atomide.com: updated to apply]
Signed-off-by: Tony Lindgren t...@atomide.com
--- a/arch/arm/mach-omap2/dpll3xxx.c
+++ b/arch/arm/mach-omap2/dpll3xxx.c
@@ -410,7 +410,7 @@ int omap3_noncore_dpll_enable(struct clk_hw *hw)
struct clk_hw_omap *clk = to_clk_hw_omap(hw);
int r;
struct dpll_data *dd;
- struct clk *parent;
+ struct clk_hw *parent;
dd = clk-dpll_data;
if (!dd)
@@ -427,13 +427,13 @@ int omap3_noncore_dpll_enable(struct clk_hw *hw)
}
}
- parent = __clk_get_parent(hw-clk);
+ parent = __clk_get_hw(__clk_get_parent(hw-clk));
if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) {
- WARN_ON(parent != dd-clk_bypass);
+ WARN_ON(parent != __clk_get_hw(dd-clk_bypass));
r = _omap3_noncore_dpll_bypass(clk);
} else {
- WARN_ON(parent != dd-clk_ref);
+ WARN_ON(parent != __clk_get_hw(dd-clk_ref));
r = _omap3_noncore_dpll_lock(clk);
}
@@ -549,7 +549,8 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw, unsigned
long rate,
if (!dd)
return -EINVAL;
- if (__clk_get_parent(hw-clk) != dd-clk_ref)
+ if (__clk_get_hw(__clk_get_parent(hw-clk)) !=
+ __clk_get_hw(dd-clk_ref))
return -EINVAL;
if (dd-last_rounded_rate == 0)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/01/15 13:24, Mike Turquette wrote:
Quoting Tomeu Vizoso (2015-01-23 03:03:30)
Moves clock state to struct clk_core, but takes care to change as little API
as
possible.
struct clk_hw still has a pointer to a struct clk, which is the
implementation's per-user clk instance, for backwards compatibility.
The struct clk that clk_get_parent() returns isn't owned by the caller, but
by
the clock implementation, so the former shouldn't call clk_put() on it.
Because some boards in mach-omap2 still register clocks statically, their
clock
registration had to be updated to take into account that the clock
information
is stored in struct clk_core now.
Tero, Paul Tony,
Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and
struct dpll_data, namely this snippet from
arch/arm/mach-omap2/dpll3xxx.c:
parent = __clk_get_parent(hw-clk);
if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) {
WARN(parent != dd-clk_bypass,
here0, parent name is %s, bypass name is
%s\n,
__clk_get_name(parent),
__clk_get_name(dd-clk_bypass));
r = _omap3_noncore_dpll_bypass(clk);
} else {
WARN(parent != dd-clk_ref,
here1, parent name is %s, ref name is %s\n,
__clk_get_name(parent),
__clk_get_name(dd-clk_ref));
r = _omap3_noncore_dpll_lock(clk);
}
struct dpll_data has members clk_ref and clk_bypass which are struct clk
pointers. This was always a bit of a violation of the clk.h contract
since drivers are not supposed to deref struct clk pointers.
Julia,
Is there a way we can write a coccinelle script to check for this? The
goal being to find all drivers that are comparing struct clk pointers or
attempting to dereference them. There are probably other frameworks that
could use the same type of check (regulator, gpiod, reset, pwm, etc.).
Probably anything that has a get/put API.
-Stephen
Now that we
generate unique pointers for each call to clk_get (clk_ref clk_bypass
are populated by of_clk_get in ti_clk_register_dpll) then the pointer
comparisons above will never be equal (even if they resolve down to the
same struct clk_core). I added the verbose traces to the WARNs above to
illustrate the point: the names are always the same but the pointers
differ.
AFAICT this doesn't break anything, but booting on OMAP3+ results in
noisy WARNs.
I think the correct fix is to replace clk_bypass and clk_ref pointers
with a simple integer parent_index. In fact we already have this index.
See how the pointers are populated in ti_clk_register_dpll:
dd-clk_ref = of_clk_get(node, 0);
dd-clk_bypass = of_clk_get(node, 1);
Tony, the same problem affects the FAPLL code which copy/pastes some of
the DPLL code.
Thoughts?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On Mon, 2 Feb 2015, Stephen Boyd wrote:
On 02/01/15 13:24, Mike Turquette wrote:
Quoting Tomeu Vizoso (2015-01-23 03:03:30)
Moves clock state to struct clk_core, but takes care to change as little
API as
possible.
struct clk_hw still has a pointer to a struct clk, which is the
implementation's per-user clk instance, for backwards compatibility.
The struct clk that clk_get_parent() returns isn't owned by the caller,
but by
the clock implementation, so the former shouldn't call clk_put() on it.
Because some boards in mach-omap2 still register clocks statically, their
clock
registration had to be updated to take into account that the clock
information
is stored in struct clk_core now.
Tero, Paul Tony,
Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and
struct dpll_data, namely this snippet from
arch/arm/mach-omap2/dpll3xxx.c:
parent = __clk_get_parent(hw-clk);
if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) {
WARN(parent != dd-clk_bypass,
here0, parent name is %s, bypass name is
%s\n,
__clk_get_name(parent),
__clk_get_name(dd-clk_bypass));
r = _omap3_noncore_dpll_bypass(clk);
} else {
WARN(parent != dd-clk_ref,
here1, parent name is %s, ref name is
%s\n,
__clk_get_name(parent),
__clk_get_name(dd-clk_ref));
r = _omap3_noncore_dpll_lock(clk);
}
struct dpll_data has members clk_ref and clk_bypass which are struct clk
pointers. This was always a bit of a violation of the clk.h contract
since drivers are not supposed to deref struct clk pointers.
Julia,
Is there a way we can write a coccinelle script to check for this? The
goal being to find all drivers that are comparing struct clk pointers or
attempting to dereference them. There are probably other frameworks that
could use the same type of check (regulator, gpiod, reset, pwm, etc.).
Probably anything that has a get/put API.
Comparing or dereferencing pointers of a particular type should be
straightforward to check for. Is there an example of how to use the
parent_index value to fix the problem?
julia
-Stephen
Now that we
generate unique pointers for each call to clk_get (clk_ref clk_bypass
are populated by of_clk_get in ti_clk_register_dpll) then the pointer
comparisons above will never be equal (even if they resolve down to the
same struct clk_core). I added the verbose traces to the WARNs above to
illustrate the point: the names are always the same but the pointers
differ.
AFAICT this doesn't break anything, but booting on OMAP3+ results in
noisy WARNs.
I think the correct fix is to replace clk_bypass and clk_ref pointers
with a simple integer parent_index. In fact we already have this index.
See how the pointers are populated in ti_clk_register_dpll:
dd-clk_ref = of_clk_get(node, 0);
dd-clk_bypass = of_clk_get(node, 1);
Tony, the same problem affects the FAPLL code which copy/pastes some of
the DPLL code.
Thoughts?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
* Mike Turquette mturque...@linaro.org [150202 14:51]: Quoting Tony Lindgren (2015-02-02 12:44:02) Thanks Tero, looks like your fix fixes all the issues I'm seeing with commit 59cf3fcf9baf. That is noisy dmesg, dpll_abe_ck not locking on 4430sdp, and off-idle not working for omap3. I could not get the patch to apply, below is what I applied manually. Mike, If possible, maybe fold this into 59cf3fcf9baf? It applies with some fuzz on that too. And inn that case, please feel also to add the following for Tomeu's patch: Tested-by: Tony Lindgren t...@atomide.com Done and done. Things look good in my testing. I've pushed another branch out to the mirrors and hopefully the autobuild/autoboot testing will give us the green light. Thanks I just checked that your updated branch works for me now. This implementation can be revisited probably after 3.19 comes out if Tero doesn't like using clk_hw directly, or if we provide a better interface. Sounds like what Tero is saying also relates to knowing if the parent clock is in bypass mode or not in addition to the parent rate. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/02/15 14:41, Mike Turquette wrote: Quoting Tero Kristo (2015-02-02 11:32:01) On 02/01/2015 11:24 PM, Mike Turquette wrote: AFAICT this doesn't break anything, but booting on OMAP3+ results in noisy WARNs. I think the correct fix is to replace clk_bypass and clk_ref pointers with a simple integer parent_index. In fact we already have this index. See how the pointers are populated in ti_clk_register_dpll: The problem is we still need to be able to get runtime parent clock rates (the parent rate may change also), so simple index value is not sufficient. We need a handle of some sort to the bypass/ref clocks. The DPLL code generally requires knowledge of the bypass + reference clock rates to work properly, as it calculates the M/N values based on these. We can maybe introduce something like of_clk_get_parent_rate, as we have analogous stuff for getting parent names and indexes. Without introducing a new helper you could probably just do: clk_ref = clk_get_parent_by_index(dpll_clk, 0); ref_rate = clk_get_rate(clk_ref); clk_bypass = clk_get_parent_by_index(dpll_clk, 1); bypass_rate = clk_get_rate(clk_bypass); Currently the semantics around this call are weird. It seems like it would create a new struct clk pointer but it does not. So don't call clk_put on clk_ref and clk_bypass yet. That might change in the future as we iron out this brave new world that we all live in. Probably best to leave a FIXME in there. Stephen Tomeu, let me know if I got any of that wrong. The plan is to make clk_get_parent_by_index() return a clk_hw pointer instead of a clk pointer (probably with a new clk_get_parent_hw_by_index() API). Then drivers that are clk providers can deal in struct clk_hw and clk consumers can deal in struct clk, nicely splitting the API between consumers and providers on the structures they use to interact with the framework. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/02/15 13:31, Julia Lawall wrote: On Mon, 2 Feb 2015, Stephen Boyd wrote: Julia, Is there a way we can write a coccinelle script to check for this? The goal being to find all drivers that are comparing struct clk pointers or attempting to dereference them. There are probably other frameworks that could use the same type of check (regulator, gpiod, reset, pwm, etc.). Probably anything that has a get/put API. Comparing or dereferencing pointers of a particular type should be straightforward to check for. Is there an example of how to use the parent_index value to fix the problem? I'm not sure how to fix this case with parent_index values generically. I imagine it would be highly specific to the surrounding code to the point where we couldn't fix it automatically. For example, if it's a clk consumer (not provider like in this case) using an index wouldn't be the right fix. We would need some sort of clk API that we don't currently have and I would wonder why clock consumers even care to compare such pointers in the first place. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
Quoting Tony Lindgren (2015-02-02 12:44:02)
* Tero Kristo t-kri...@ti.com [150202 11:35]:
On 02/01/2015 11:24 PM, Mike Turquette wrote:
Quoting Tomeu Vizoso (2015-01-23 03:03:30)
AFAICT this doesn't break anything, but booting on OMAP3+ results in
noisy WARNs.
I think the correct fix is to replace clk_bypass and clk_ref pointers
with a simple integer parent_index. In fact we already have this index.
See how the pointers are populated in ti_clk_register_dpll:
The problem is we still need to be able to get runtime parent clock rates
(the parent rate may change also), so simple index value is not sufficient.
We need a handle of some sort to the bypass/ref clocks. The DPLL code
generally requires knowledge of the bypass + reference clock rates to work
properly, as it calculates the M/N values based on these.
Shall I change the DPLL code to check against clk_hw pointers or what is the
preferred approach here? The patch at the end does this and fixes the dpll
related warnings.
Btw, the rate constraints patch broke boot for me completely, but sounds
like you reverted it already.
Thanks Tero, looks like your fix fixes all the issues I'm seeing with
commit 59cf3fcf9baf. That is noisy dmesg, dpll_abe_ck not locking
on 4430sdp, and off-idle not working for omap3.
I could not get the patch to apply, below is what I applied manually.
Mike, If possible, maybe fold this into 59cf3fcf9baf? It applies with
some fuzz on that too. And inn that case, please feel also to add the
following for Tomeu's patch:
Tested-by: Tony Lindgren t...@atomide.com
Done and done. Things look good in my testing. I've pushed another
branch out to the mirrors and hopefully the autobuild/autoboot testing
will give us the green light.
This implementation can be revisited probably after 3.19 comes out if
Tero doesn't like using clk_hw directly, or if we provide a better
interface.
Thanks,
Mike
8
From: Tero Kristo t-kri...@ti.com
Date: Mon, 2 Feb 2015 12:17:00 -0800
Subject: [PATCH] ARM: OMAP3+: clock: dpll: fix logic for comparing parent
clocks
DPLL code uses reference and bypass clock pointers for determining runtime
properties for these clocks, like parent clock rates.
As clock API now returns per-user clock structs, using a global handle
in the clock driver code does not work properly anymore. Fix this by
using the clk_hw instead, and comparing this against the parents.
Fixes: 59cf3fcf9baf (clk: Make clk API return per-user struct clk instances)
Signed-off-by: Tero Kristo t-kri...@ti.com
[t...@atomide.com: updated to apply]
Signed-off-by: Tony Lindgren t...@atomide.com
--- a/arch/arm/mach-omap2/dpll3xxx.c
+++ b/arch/arm/mach-omap2/dpll3xxx.c
@@ -410,7 +410,7 @@ int omap3_noncore_dpll_enable(struct clk_hw *hw)
struct clk_hw_omap *clk = to_clk_hw_omap(hw);
int r;
struct dpll_data *dd;
- struct clk *parent;
+ struct clk_hw *parent;
dd = clk-dpll_data;
if (!dd)
@@ -427,13 +427,13 @@ int omap3_noncore_dpll_enable(struct clk_hw *hw)
}
}
- parent = __clk_get_parent(hw-clk);
+ parent = __clk_get_hw(__clk_get_parent(hw-clk));
if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) {
- WARN_ON(parent != dd-clk_bypass);
+ WARN_ON(parent != __clk_get_hw(dd-clk_bypass));
r = _omap3_noncore_dpll_bypass(clk);
} else {
- WARN_ON(parent != dd-clk_ref);
+ WARN_ON(parent != __clk_get_hw(dd-clk_ref));
r = _omap3_noncore_dpll_lock(clk);
}
@@ -549,7 +549,8 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw,
unsigned long rate,
if (!dd)
return -EINVAL;
- if (__clk_get_parent(hw-clk) != dd-clk_ref)
+ if (__clk_get_hw(__clk_get_parent(hw-clk)) !=
+ __clk_get_hw(dd-clk_ref))
return -EINVAL;
if (dd-last_rounded_rate == 0)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
Quoting Tero Kristo (2015-02-02 11:32:01)
On 02/01/2015 11:24 PM, Mike Turquette wrote:
Quoting Tomeu Vizoso (2015-01-23 03:03:30)
Moves clock state to struct clk_core, but takes care to change as little
API as
possible.
struct clk_hw still has a pointer to a struct clk, which is the
implementation's per-user clk instance, for backwards compatibility.
The struct clk that clk_get_parent() returns isn't owned by the caller,
but by
the clock implementation, so the former shouldn't call clk_put() on it.
Because some boards in mach-omap2 still register clocks statically, their
clock
registration had to be updated to take into account that the clock
information
is stored in struct clk_core now.
Tero, Paul Tony,
Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and
struct dpll_data, namely this snippet from
arch/arm/mach-omap2/dpll3xxx.c:
parent = __clk_get_parent(hw-clk);
if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) {
WARN(parent != dd-clk_bypass,
here0, parent name is %s, bypass name is
%s\n,
__clk_get_name(parent),
__clk_get_name(dd-clk_bypass));
r = _omap3_noncore_dpll_bypass(clk);
} else {
WARN(parent != dd-clk_ref,
here1, parent name is %s, ref name is
%s\n,
__clk_get_name(parent),
__clk_get_name(dd-clk_ref));
r = _omap3_noncore_dpll_lock(clk);
}
struct dpll_data has members clk_ref and clk_bypass which are struct clk
pointers. This was always a bit of a violation of the clk.h contract
since drivers are not supposed to deref struct clk pointers. Now that we
generate unique pointers for each call to clk_get (clk_ref clk_bypass
are populated by of_clk_get in ti_clk_register_dpll) then the pointer
comparisons above will never be equal (even if they resolve down to the
same struct clk_core). I added the verbose traces to the WARNs above to
illustrate the point: the names are always the same but the pointers
differ.
AFAICT this doesn't break anything, but booting on OMAP3+ results in
noisy WARNs.
I think the correct fix is to replace clk_bypass and clk_ref pointers
with a simple integer parent_index. In fact we already have this index.
See how the pointers are populated in ti_clk_register_dpll:
The problem is we still need to be able to get runtime parent clock
rates (the parent rate may change also), so simple index value is not
sufficient. We need a handle of some sort to the bypass/ref clocks. The
DPLL code generally requires knowledge of the bypass + reference clock
rates to work properly, as it calculates the M/N values based on these.
We can maybe introduce something like of_clk_get_parent_rate, as we have
analogous stuff for getting parent names and indexes. Without
introducing a new helper you could probably just do:
clk_ref = clk_get_parent_by_index(dpll_clk, 0);
ref_rate = clk_get_rate(clk_ref);
clk_bypass = clk_get_parent_by_index(dpll_clk, 1);
bypass_rate = clk_get_rate(clk_bypass);
Currently the semantics around this call are weird. It seems like it
would create a new struct clk pointer but it does not. So don't call
clk_put on clk_ref and clk_bypass yet. That might change in the future
as we iron out this brave new world that we all live in. Probably best
to leave a FIXME in there.
Stephen Tomeu, let me know if I got any of that wrong.
Shall I change the DPLL code to check against clk_hw pointers or what is
the preferred approach here? The patch at the end does this and fixes
the dpll related warnings.
Yes, for now that is fine, but feels a bit hacky to me. I don't know
honestly, let me sleep on it. Anyways for 3.20 that is perfectly fine
but we might want to switch to something like the scheme above.
Btw, the rate constraints patch broke boot for me completely, but sounds
like you reverted it already.
Fixed with Stephen's patch from last week. Thanks for dealing with all
the breakage so promptly. It has helped a lot!
Regards,
Mike
-Tero
Author: Tero Kristo t-kri...@ti.com
Date: Mon Feb 2 17:19:17 2015 +0200
ARM: OMAP3+: clock: dpll: fix logic for comparing parent clocks
DPLL code uses reference and bypass clock pointers for determining
runtime
properties for these clocks, like parent clock rates.
As clock API now returns per-user clock structs, using a global handle
in the clock driver code does not work properly anymore. Fix this by
using the clk_hw instead, and comparing this against the parents.
Signed-off-by: Tero Kristo t-kri...@ti.com
Fixes: 59cf3fcf9baf (clk: Make clk API return per-user struct clk
instances)
diff --git
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
Quoting Stephen Boyd (2015-02-02 14:35:59) On 02/02/15 13:31, Julia Lawall wrote: On Mon, 2 Feb 2015, Stephen Boyd wrote: Julia, Is there a way we can write a coccinelle script to check for this? The goal being to find all drivers that are comparing struct clk pointers or attempting to dereference them. There are probably other frameworks that could use the same type of check (regulator, gpiod, reset, pwm, etc.). Probably anything that has a get/put API. Comparing or dereferencing pointers of a particular type should be straightforward to check for. Is there an example of how to use the parent_index value to fix the problem? I'm not sure how to fix this case with parent_index values generically. I imagine it would be highly specific to the surrounding code to the point where we couldn't fix it automatically. For example, if it's a clk consumer (not provider like in this case) using an index wouldn't be the right fix. We would need some sort of clk API that we don't currently have and I would wonder why clock consumers even care to compare such pointers in the first place. Ack. Is there precedent for a Don't do that kind of response? Regards, Mike -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/02/2015 11:41 PM, Mike Turquette wrote:
Quoting Tero Kristo (2015-02-02 11:32:01)
On 02/01/2015 11:24 PM, Mike Turquette wrote:
Quoting Tomeu Vizoso (2015-01-23 03:03:30)
Moves clock state to struct clk_core, but takes care to change as little
API as
possible.
struct clk_hw still has a pointer to a struct clk, which is the
implementation's per-user clk instance, for backwards compatibility.
The struct clk that clk_get_parent() returns isn't owned by the caller,
but by
the clock implementation, so the former shouldn't call clk_put() on it.
Because some boards in mach-omap2 still register clocks statically, their
clock
registration had to be updated to take into account that the clock
information
is stored in struct clk_core now.
Tero, Paul Tony,
Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and
struct dpll_data, namely this snippet from
arch/arm/mach-omap2/dpll3xxx.c:
parent = __clk_get_parent(hw-clk);
if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) {
WARN(parent != dd-clk_bypass,
here0, parent name is %s, bypass name is
%s\n,
__clk_get_name(parent),
__clk_get_name(dd-clk_bypass));
r = _omap3_noncore_dpll_bypass(clk);
} else {
WARN(parent != dd-clk_ref,
here1, parent name is %s, ref name is
%s\n,
__clk_get_name(parent),
__clk_get_name(dd-clk_ref));
r = _omap3_noncore_dpll_lock(clk);
}
struct dpll_data has members clk_ref and clk_bypass which are struct clk
pointers. This was always a bit of a violation of the clk.h contract
since drivers are not supposed to deref struct clk pointers. Now that we
generate unique pointers for each call to clk_get (clk_ref clk_bypass
are populated by of_clk_get in ti_clk_register_dpll) then the pointer
comparisons above will never be equal (even if they resolve down to the
same struct clk_core). I added the verbose traces to the WARNs above to
illustrate the point: the names are always the same but the pointers
differ.
AFAICT this doesn't break anything, but booting on OMAP3+ results in
noisy WARNs.
I think the correct fix is to replace clk_bypass and clk_ref pointers
with a simple integer parent_index. In fact we already have this index.
See how the pointers are populated in ti_clk_register_dpll:
The problem is we still need to be able to get runtime parent clock
rates (the parent rate may change also), so simple index value is not
sufficient. We need a handle of some sort to the bypass/ref clocks. The
DPLL code generally requires knowledge of the bypass + reference clock
rates to work properly, as it calculates the M/N values based on these.
We can maybe introduce something like of_clk_get_parent_rate, as we have
analogous stuff for getting parent names and indexes. Without
introducing a new helper you could probably just do:
clk_ref = clk_get_parent_by_index(dpll_clk, 0);
ref_rate = clk_get_rate(clk_ref);
clk_bypass = clk_get_parent_by_index(dpll_clk, 1);
bypass_rate = clk_get_rate(clk_bypass);
Currently the semantics around this call are weird. It seems like it
would create a new struct clk pointer but it does not. So don't call
clk_put on clk_ref and clk_bypass yet. That might change in the future
as we iron out this brave new world that we all live in. Probably best
to leave a FIXME in there.
Stephen Tomeu, let me know if I got any of that wrong.
I think you got it right, just wanted to mention that we can and
probably should make the clk_get_parent_* calls in the consumer API to
return per-user clk instances but that we need to make sure first that
callers call clk_put afterwards.
This should also allow us to remove the reference to struct clk from
clk_hw, which is at best awkward.
Regards,
Tomeu
Shall I change the DPLL code to check against clk_hw pointers or what is
the preferred approach here? The patch at the end does this and fixes
the dpll related warnings.
Yes, for now that is fine, but feels a bit hacky to me. I don't know
honestly, let me sleep on it. Anyways for 3.20 that is perfectly fine
but we might want to switch to something like the scheme above.
Btw, the rate constraints patch broke boot for me completely, but sounds
like you reverted it already.
Fixed with Stephen's patch from last week. Thanks for dealing with all
the breakage so promptly. It has helped a lot!
Regards,
Mike
-Tero
Author: Tero Kristo t-kri...@ti.com
Date: Mon Feb 2 17:19:17 2015 +0200
ARM: OMAP3+: clock: dpll: fix logic for comparing parent clocks
DPLL code uses reference and bypass clock pointers for determining
runtime
properties for these clocks, like parent clock rates.
As clock API now returns
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
* Mike Turquette mturque...@linaro.org [150201 13:27]:
Quoting Tomeu Vizoso (2015-01-23 03:03:30)
Moves clock state to struct clk_core, but takes care to change as little
API as
possible.
struct clk_hw still has a pointer to a struct clk, which is the
implementation's per-user clk instance, for backwards compatibility.
The struct clk that clk_get_parent() returns isn't owned by the caller, but
by
the clock implementation, so the former shouldn't call clk_put() on it.
Because some boards in mach-omap2 still register clocks statically, their
clock
registration had to be updated to take into account that the clock
information
is stored in struct clk_core now.
Tero, Paul Tony,
Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and
struct dpll_data, namely this snippet from
arch/arm/mach-omap2/dpll3xxx.c:
parent = __clk_get_parent(hw-clk);
if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) {
WARN(parent != dd-clk_bypass,
here0, parent name is %s, bypass name is
%s\n,
__clk_get_name(parent),
__clk_get_name(dd-clk_bypass));
r = _omap3_noncore_dpll_bypass(clk);
} else {
WARN(parent != dd-clk_ref,
here1, parent name is %s, ref name is %s\n,
__clk_get_name(parent),
__clk_get_name(dd-clk_ref));
r = _omap3_noncore_dpll_lock(clk);
}
struct dpll_data has members clk_ref and clk_bypass which are struct clk
pointers. This was always a bit of a violation of the clk.h contract
since drivers are not supposed to deref struct clk pointers. Now that we
generate unique pointers for each call to clk_get (clk_ref clk_bypass
are populated by of_clk_get in ti_clk_register_dpll) then the pointer
comparisons above will never be equal (even if they resolve down to the
same struct clk_core). I added the verbose traces to the WARNs above to
illustrate the point: the names are always the same but the pointers
differ.
AFAICT this doesn't break anything, but booting on OMAP3+ results in
noisy WARNs.
Also on at least omap4 like I posted. So sounds like the check for
WARN is wrong but harmless. Paul Tero, what do you want to do
about that?
I think the correct fix is to replace clk_bypass and clk_ref pointers
with a simple integer parent_index. In fact we already have this index.
See how the pointers are populated in ti_clk_register_dpll:
dd-clk_ref = of_clk_get(node, 0);
dd-clk_bypass = of_clk_get(node, 1);
Tony, the same problem affects the FAPLL code which copy/pastes some of
the DPLL code.
Thoughts?
Not seeing these warnings with dm186x as fapll.c does not use
dpll3xxx.c. This is because of the way the PLL's child synthesizers
need to also access the PLL registers for power and bypass mode.
Not related to the $subject bug, but to me it seems that we could
possibly have Linux generic PLL code if we add support for
parent_in_bypass_mode in addition to the parent_rate. This is because
the PLL can in theory generate the same rate both in bypass mode and
regular mode so parent_rate is not enough to tell it to the child
synthesizers. Not sure how the PLL registers enabling and disabling
it's children should be handled, maybe regmap would work there.
Regards,
Tony
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
On 02/01/2015 11:24 PM, Mike Turquette wrote:
Quoting Tomeu Vizoso (2015-01-23 03:03:30)
Moves clock state to struct clk_core, but takes care to change as little API as
possible.
struct clk_hw still has a pointer to a struct clk, which is the
implementation's per-user clk instance, for backwards compatibility.
The struct clk that clk_get_parent() returns isn't owned by the caller, but by
the clock implementation, so the former shouldn't call clk_put() on it.
Because some boards in mach-omap2 still register clocks statically, their clock
registration had to be updated to take into account that the clock information
is stored in struct clk_core now.
Tero, Paul Tony,
Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and
struct dpll_data, namely this snippet from
arch/arm/mach-omap2/dpll3xxx.c:
parent = __clk_get_parent(hw-clk);
if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) {
WARN(parent != dd-clk_bypass,
here0, parent name is %s, bypass name is
%s\n,
__clk_get_name(parent),
__clk_get_name(dd-clk_bypass));
r = _omap3_noncore_dpll_bypass(clk);
} else {
WARN(parent != dd-clk_ref,
here1, parent name is %s, ref name is %s\n,
__clk_get_name(parent),
__clk_get_name(dd-clk_ref));
r = _omap3_noncore_dpll_lock(clk);
}
struct dpll_data has members clk_ref and clk_bypass which are struct clk
pointers. This was always a bit of a violation of the clk.h contract
since drivers are not supposed to deref struct clk pointers. Now that we
generate unique pointers for each call to clk_get (clk_ref clk_bypass
are populated by of_clk_get in ti_clk_register_dpll) then the pointer
comparisons above will never be equal (even if they resolve down to the
same struct clk_core). I added the verbose traces to the WARNs above to
illustrate the point: the names are always the same but the pointers
differ.
AFAICT this doesn't break anything, but booting on OMAP3+ results in
noisy WARNs.
I think the correct fix is to replace clk_bypass and clk_ref pointers
with a simple integer parent_index. In fact we already have this index.
See how the pointers are populated in ti_clk_register_dpll:
The problem is we still need to be able to get runtime parent clock
rates (the parent rate may change also), so simple index value is not
sufficient. We need a handle of some sort to the bypass/ref clocks. The
DPLL code generally requires knowledge of the bypass + reference clock
rates to work properly, as it calculates the M/N values based on these.
Shall I change the DPLL code to check against clk_hw pointers or what is
the preferred approach here? The patch at the end does this and fixes
the dpll related warnings.
Btw, the rate constraints patch broke boot for me completely, but sounds
like you reverted it already.
-Tero
Author: Tero Kristo t-kri...@ti.com
Date: Mon Feb 2 17:19:17 2015 +0200
ARM: OMAP3+: clock: dpll: fix logic for comparing parent clocks
DPLL code uses reference and bypass clock pointers for determining
runtime
properties for these clocks, like parent clock rates.
As clock API now returns per-user clock structs, using a global handle
in the clock driver code does not work properly anymore. Fix this by
using the clk_hw instead, and comparing this against the parents.
Signed-off-by: Tero Kristo t-kri...@ti.com
Fixes: 59cf3fcf9baf (clk: Make clk API return per-user struct clk
instances)
diff --git a/arch/arm/mach-omap2/dpll3xxx.c b/arch/arm/mach-omap2/dpll3xxx.c
index c2da2a0..49752d7 100644
--- a/arch/arm/mach-omap2/dpll3xxx.c
+++ b/arch/arm/mach-omap2/dpll3xxx.c
@@ -410,7 +410,7 @@ int omap3_noncore_dpll_enable(struct clk_hw *hw)
struct clk_hw_omap *clk = to_clk_hw_omap(hw);
int r;
struct dpll_data *dd;
- struct clk *parent;
+ struct clk_hw *parent;
dd = clk-dpll_data;
if (!dd)
@@ -427,13 +427,13 @@ int omap3_noncore_dpll_enable(struct clk_hw *hw)
}
}
- parent = __clk_get_parent(hw-clk);
+ parent = __clk_get_hw(__clk_get_parent(hw-clk));
if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) {
- WARN_ON(parent != dd-clk_bypass);
+ WARN_ON(parent != __clk_get_hw(dd-clk_bypass));
r = _omap3_noncore_dpll_bypass(clk);
} else {
- WARN_ON(parent != dd-clk_ref);
+ WARN_ON(parent != __clk_get_hw(dd-clk_ref));
r = _omap3_noncore_dpll_lock(clk);
}
@@ -549,7 +549,8 @@ int omap3_noncore_dpll_set_rate(struct clk_hw *hw,
unsigned long rate,
if (!dd)
return -EINVAL;
- if (__clk_get_parent(hw-clk) != dd-clk_ref)
+ if (__clk_get_hw(__clk_get_parent(hw-clk))
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
Quoting Tomeu Vizoso (2015-01-23 03:03:30)
> Moves clock state to struct clk_core, but takes care to change as little API
> as
> possible.
>
> struct clk_hw still has a pointer to a struct clk, which is the
> implementation's per-user clk instance, for backwards compatibility.
>
> The struct clk that clk_get_parent() returns isn't owned by the caller, but by
> the clock implementation, so the former shouldn't call clk_put() on it.
>
> Because some boards in mach-omap2 still register clocks statically, their
> clock
> registration had to be updated to take into account that the clock information
> is stored in struct clk_core now.
Tero, Paul & Tony,
Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and
struct dpll_data, namely this snippet from
arch/arm/mach-omap2/dpll3xxx.c:
parent = __clk_get_parent(hw->clk);
if (__clk_get_rate(hw->clk) == __clk_get_rate(dd->clk_bypass)) {
WARN(parent != dd->clk_bypass,
"here0, parent name is %s, bypass name is %s\n",
__clk_get_name(parent),
__clk_get_name(dd->clk_bypass));
r = _omap3_noncore_dpll_bypass(clk);
} else {
WARN(parent != dd->clk_ref,
"here1, parent name is %s, ref name is %s\n",
__clk_get_name(parent),
__clk_get_name(dd->clk_ref));
r = _omap3_noncore_dpll_lock(clk);
}
struct dpll_data has members clk_ref and clk_bypass which are struct clk
pointers. This was always a bit of a violation of the clk.h contract
since drivers are not supposed to deref struct clk pointers. Now that we
generate unique pointers for each call to clk_get (clk_ref & clk_bypass
are populated by of_clk_get in ti_clk_register_dpll) then the pointer
comparisons above will never be equal (even if they resolve down to the
same struct clk_core). I added the verbose traces to the WARNs above to
illustrate the point: the names are always the same but the pointers
differ.
AFAICT this doesn't break anything, but booting on OMAP3+ results in
noisy WARNs.
I think the correct fix is to replace clk_bypass and clk_ref pointers
with a simple integer parent_index. In fact we already have this index.
See how the pointers are populated in ti_clk_register_dpll:
dd->clk_ref = of_clk_get(node, 0);
dd->clk_bypass = of_clk_get(node, 1);
Tony, the same problem affects the FAPLL code which copy/pastes some of
the DPLL code.
Thoughts?
Regards,
Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances
Quoting Tomeu Vizoso (2015-01-23 03:03:30)
Moves clock state to struct clk_core, but takes care to change as little API
as
possible.
struct clk_hw still has a pointer to a struct clk, which is the
implementation's per-user clk instance, for backwards compatibility.
The struct clk that clk_get_parent() returns isn't owned by the caller, but by
the clock implementation, so the former shouldn't call clk_put() on it.
Because some boards in mach-omap2 still register clocks statically, their
clock
registration had to be updated to take into account that the clock information
is stored in struct clk_core now.
Tero, Paul Tony,
Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and
struct dpll_data, namely this snippet from
arch/arm/mach-omap2/dpll3xxx.c:
parent = __clk_get_parent(hw-clk);
if (__clk_get_rate(hw-clk) == __clk_get_rate(dd-clk_bypass)) {
WARN(parent != dd-clk_bypass,
here0, parent name is %s, bypass name is %s\n,
__clk_get_name(parent),
__clk_get_name(dd-clk_bypass));
r = _omap3_noncore_dpll_bypass(clk);
} else {
WARN(parent != dd-clk_ref,
here1, parent name is %s, ref name is %s\n,
__clk_get_name(parent),
__clk_get_name(dd-clk_ref));
r = _omap3_noncore_dpll_lock(clk);
}
struct dpll_data has members clk_ref and clk_bypass which are struct clk
pointers. This was always a bit of a violation of the clk.h contract
since drivers are not supposed to deref struct clk pointers. Now that we
generate unique pointers for each call to clk_get (clk_ref clk_bypass
are populated by of_clk_get in ti_clk_register_dpll) then the pointer
comparisons above will never be equal (even if they resolve down to the
same struct clk_core). I added the verbose traces to the WARNs above to
illustrate the point: the names are always the same but the pointers
differ.
AFAICT this doesn't break anything, but booting on OMAP3+ results in
noisy WARNs.
I think the correct fix is to replace clk_bypass and clk_ref pointers
with a simple integer parent_index. In fact we already have this index.
See how the pointers are populated in ti_clk_register_dpll:
dd-clk_ref = of_clk_get(node, 0);
dd-clk_bypass = of_clk_get(node, 1);
Tony, the same problem affects the FAPLL code which copy/pastes some of
the DPLL code.
Thoughts?
Regards,
Mike
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
