[PATCH] clk: update clk_clean_rate_cache to use private clk struct
In clk_clean_rate_cache, clk->rate should update the private clock struct, in particular when CCF is activated, to save the cached rate value.
When clk_get_parent_rate is called, the cached information is read from pclk->rate, with pclk = clk_get_parent(clk).
As the cached is read from private clk data, the update should be done also on it.
Fixes: 6b7fd3128f7 ("clk: fix set_rate to clean up cached rates for the hierarchy") Signed-off-by: Patrick Delaunay patrick.delaunay@foss.st.com ---
drivers/clk/clk-uclass.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index d245b672fa..62393b73e1 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -576,6 +576,21 @@ ulong clk_round_rate(struct clk *clk, ulong rate) return ops->round_rate(clk, rate); }
+static void clk_get_priv(struct clk *clk, struct clk **clkp) +{ + *clkp = NULL; + + /* get private clock struct associated to the provided clock */ + if (CONFIG_IS_ENABLED(CLK_CCF)) { + /* Take id 0 as a non-valid clk, such as dummy */ + if (clk->id) + clk_get_by_id(clk->id, clkp); + } else { + *clkp = dev_get_clk_ptr(clk->dev); + } +} + +/* clean cache, called with private clock struct */ static void clk_clean_rate_cache(struct clk *clk) { struct udevice *child_dev; @@ -595,6 +610,7 @@ static void clk_clean_rate_cache(struct clk *clk) ulong clk_set_rate(struct clk *clk, ulong rate) { const struct clk_ops *ops; + struct clk *clkp;
debug("%s(clk=%p, rate=%lu)\n", __func__, clk, rate); if (!clk_valid(clk)) @@ -604,8 +620,10 @@ ulong clk_set_rate(struct clk *clk, ulong rate) if (!ops->set_rate) return -ENOSYS;
+ /* get private clock struct used for cache */ + clk_get_priv(clk, &clkp); /* Clean up cached rates for us and all child clocks */ - clk_clean_rate_cache(clk); + clk_clean_rate_cache(clkp);
return ops->set_rate(clk, rate); }
On 1/31/22 10:18 AM, Patrick Delaunay wrote:
In clk_clean_rate_cache, clk->rate should update the private clock struct, in particular when CCF is activated, to save the cached rate value.
When clk_get_parent_rate is called, the cached information is read from pclk->rate, with pclk = clk_get_parent(clk).
As the cached is read from private clk data, the update should be done also on it.
Fixes: 6b7fd3128f7 ("clk: fix set_rate to clean up cached rates for the hierarchy") Signed-off-by: Patrick Delaunay patrick.delaunay@foss.st.com
drivers/clk/clk-uclass.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index d245b672fa..62393b73e1 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -576,6 +576,21 @@ ulong clk_round_rate(struct clk *clk, ulong rate) return ops->round_rate(clk, rate); }
+static void clk_get_priv(struct clk *clk, struct clk **clkp) +{
- *clkp = NULL;
- /* get private clock struct associated to the provided clock */
- if (CONFIG_IS_ENABLED(CLK_CCF)) {
/* Take id 0 as a non-valid clk, such as dummy */if (clk->id)clk_get_by_id(clk->id, clkp);- } else {
*clkp = dev_get_clk_ptr(clk->dev);- }
Does this even do anything without CCF? AFAIK those clocks are the only ones which mirror the clock structure in udevices and keep their "real" struct clock as their private data. IMO this whole system is broken because most drivers do not/should not do this.
--Sean
+}
+/* clean cache, called with private clock struct */ static void clk_clean_rate_cache(struct clk *clk) { struct udevice *child_dev; @@ -595,6 +610,7 @@ static void clk_clean_rate_cache(struct clk *clk) ulong clk_set_rate(struct clk *clk, ulong rate) { const struct clk_ops *ops;
struct clk *clkp;
debug("%s(clk=%p, rate=%lu)\n", __func__, clk, rate); if (!clk_valid(clk))
@@ -604,8 +620,10 @@ ulong clk_set_rate(struct clk *clk, ulong rate) if (!ops->set_rate) return -ENOSYS;
- /* get private clock struct used for cache */
- clk_get_priv(clk, &clkp); /* Clean up cached rates for us and all child clocks */
- clk_clean_rate_cache(clk);
clk_clean_rate_cache(clkp);
return ops->set_rate(clk, rate); }
Hi Sean
On 2/1/22 16:14, Sean Anderson wrote:
On 1/31/22 10:18 AM, Patrick Delaunay wrote:
In clk_clean_rate_cache, clk->rate should update the private clock struct, in particular when CCF is activated, to save the cached rate value.
When clk_get_parent_rate is called, the cached information is read from pclk->rate, with pclk = clk_get_parent(clk).
As the cached is read from private clk data, the update should be done also on it.
Fixes: 6b7fd3128f7 ("clk: fix set_rate to clean up cached rates for the hierarchy") Signed-off-by: Patrick Delaunay patrick.delaunay@foss.st.com
drivers/clk/clk-uclass.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index d245b672fa..62393b73e1 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -576,6 +576,21 @@ ulong clk_round_rate(struct clk *clk, ulong rate) return ops->round_rate(clk, rate); } +static void clk_get_priv(struct clk *clk, struct clk **clkp) +{ + *clkp = NULL;
+ /* get private clock struct associated to the provided clock */ + if (CONFIG_IS_ENABLED(CLK_CCF)) { + /* Take id 0 as a non-valid clk, such as dummy */ + if (clk->id) + clk_get_by_id(clk->id, clkp); + } else { + *clkp = dev_get_clk_ptr(clk->dev); + }
Does this even do anything without CCF? AFAIK those clocks are the only ones which mirror the clock structure in udevices and keep their "real" struct clock as their private data. IMO this whole system is broken because most drivers do not/should not do this.
I take some time to follow the clk struct management in clk U-Class with
(the clk struct is in private data of U-Class) or without CCF
(the clock provider don't manage private data = clk struct is allocated by
clk API caller).
In particular, I check the clock identifier and how to be sure with CCF that the
identifier is unique for all clock provider (cf clk_get_by_id) and the FIXME
comment in clk_register() about dev_set_uclass_priv.
But yes , in this in this function I think, I need to change the else case to
+ } else {
+ *clkp = clk; + }
to avoid issue with other drivers without CCF.
--Sean
+}
+/* clean cache, called with private clock struct */ static void clk_clean_rate_cache(struct clk *clk) { struct udevice *child_dev; @@ -595,6 +610,7 @@ static void clk_clean_rate_cache(struct clk *clk) ulong clk_set_rate(struct clk *clk, ulong rate) { const struct clk_ops *ops; + struct clk *clkp; debug("%s(clk=%p, rate=%lu)\n", __func__, clk, rate); if (!clk_valid(clk)) @@ -604,8 +620,10 @@ ulong clk_set_rate(struct clk *clk, ulong rate) if (!ops->set_rate) return -ENOSYS; + /* get private clock struct used for cache */ + clk_get_priv(clk, &clkp); /* Clean up cached rates for us and all child clocks */ - clk_clean_rate_cache(clk); + clk_clean_rate_cache(clkp); return ops->set_rate(clk, rate); }
Regards
Patrick
participants (3)
-
Patrick DELAUNAY -
Patrick Delaunay
-
Sean Anderson