[PATCH RFC 0/2] clk: meson: g12a: handle clock hw changes while in suspend

[PATCH RFC 0/2] clk: meson: g12a: handle clock hw changes while in suspend

[Neil Armstrong]

This serie aime to support when the suspend/resume firmware alters the
clock tree, leading to an incorrect representation of the clock tree
after a resume from suspend-to-mem.

For the Amlogic G12A/G12B/SM1 case, the SCPI firmware handling suspend
alters the CPU clock tree in various ways.

Since we know which part of the tree is possibly altered, we introduce here
the clk_invalidate_rate() function that will rebuild the tree from the
hardware registers in case parents and dividers have changed.

Finally we call clk_invalidate_rate() from a new resume callback to refresh
the CPU clock tree after a resume.

With the clock tree refreshed, CCF can now handle the new clock tree
configuration and avoid crashing the system on further DVFS set_rates.

Neil Armstrong (2):
  clk: introduce clk_invalidate_rate()
  clk: meson: g12a: add suspend-resume hooks

 drivers/clk/clk.c        | 70 +++++++++++++++++++++++++++++++++++++++
 drivers/clk/meson/g12a.c | 71 +++++++++++++++++++++++++++++++++++-----
 include/linux/clk.h      | 13 ++++++++
 3 files changed, 146 insertions(+), 8 deletions(-)

-- 
2.22.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH RFC 1/2] clk: introduce clk_invalidate_rate()

[Neil Armstrong]

This introduces the clk_invalidate_rate() call used to recalculate the
rate and parent tree of a particular clock if it's known that the
underlying registers set has been altered by the firmware, like from
a suspend/resume handler running in trusted cpu mode.

The call refreshes the actual parent and when changed, instructs CCF
the parent has changed. Finally the call will recalculate the rate of
each part of the tree to make sure the CCF cached tree is in sync with
the hardware.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/clk/clk.c   | 70 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/clk.h | 13 +++++++++
 2 files changed, 83 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index ca99e9db6575..8acf38ce3cc4 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2557,6 +2557,76 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
 }
 EXPORT_SYMBOL_GPL(clk_set_parent);
 
+/**
+ * __clk_invalidate_tree
+ * @core: first clk in the subtree
+ *
+ * Walks the subtree of clks starting with clk and recalculates the parents,
+ * then accuracies and rates as it goes.
+ */
+static int __clk_invalidate_tree(struct clk_core *core)
+{
+	struct clk_core *parent, *old_parent;
+	int ret, i, num_parents;
+
+	num_parents = core->num_parents;
+
+	for (i = 0; i < num_parents; i++) {
+		parent = clk_core_get_parent_by_index(core, i);
+		if (!parent)
+			continue;
+
+		ret = __clk_invalidate_tree(parent);
+		if (ret)
+			return ret;
+	}
+
+	parent = __clk_init_parent(core);
+
+	if (parent != core->parent) {
+		old_parent = __clk_set_parent_before(core, parent);
+		__clk_set_parent_after(core, parent, old_parent);
+	}
+
+	__clk_recalc_accuracies(core);
+	__clk_recalc_rates(core, 0);
+
+	return 0;
+}
+
+static int clk_core_invalidate_rate(struct clk_core *core)
+{
+	int ret;
+
+	clk_prepare_lock();
+
+	ret = __clk_invalidate_tree(core);
+
+	clk_prepare_unlock();
+
+	return ret;
+}
+
+/**
+ * clk_invalidate_rate - invalidate and recalc rate of the clock and it's tree
+ * @clk: the clk whose rate is too be invalidated
+ *
+ * If it's known the actual hardware state of a clock tree has changed,
+ * this call will invalidate the cached rate of the clk and it's possible
+ * parents tree to permit recalculation of the actual rate.
+ *
+ * Returns 0 on success, -EERROR otherwise.
+ * If clk is NULL then returns 0.
+ */
+int clk_invalidate_rate(struct clk *clk)
+{
+	if (!clk)
+		return 0;
+
+	return clk_core_invalidate_rate(clk->core);
+}
+EXPORT_SYMBOL_GPL(clk_invalidate_rate);
+
 static int clk_core_set_phase_nolock(struct clk_core *core, int degrees)
 {
 	int ret = -EINVAL;
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 853a8f181394..46db47ffb7b2 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -629,6 +629,19 @@ long clk_round_rate(struct clk *clk, unsigned long rate);
  */
 int clk_set_rate(struct clk *clk, unsigned long rate);
 
+/**
+ * clk_invalidate_rate - invalidate and recalc rate of the clock and it's tree
+ * @clk: the clk whose rate is too be invalidated
+ *
+ * If it's known the actual hardware state of a clock tree has changed,
+ * this call will invalidate the cached rate of the clk and it's possible
+ * parents tree to permit recalculation of the actual rate.
+ *
+ * Returns 0 on success, -EERROR otherwise.
+ * If clk is NULL then returns 0.
+ */
+int clk_invalidate_rate(struct clk *clk);
+
 /**
  * clk_set_rate_exclusive- set the clock rate and claim exclusivity over
  *                         clock source
-- 
2.22.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH RFC 1/2] clk: introduce clk_invalidate_rate()

[Stephen Boyd]

Quoting Neil Armstrong (2019-09-19 03:25:17)
> This introduces the clk_invalidate_rate() call used to recalculate the
> rate and parent tree of a particular clock if it's known that the
> underlying registers set has been altered by the firmware, like from
> a suspend/resume handler running in trusted cpu mode.
> 
> The call refreshes the actual parent and when changed, instructs CCF
> the parent has changed. Finally the call will recalculate the rate of
> each part of the tree to make sure the CCF cached tree is in sync with
> the hardware.
> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---

The knee-jerk reaction to these patches is that it shouldn't be a
consumer API (i.e. taking a struct clk) but a provider API (i.e. taking
a struct clk_hw). I haven't looked in any more detail but just know that
it's a non-starter to be a consumer based API because we don't want
random consumers out there to be telling the CCF or provider drivers
that some clk has lost state and needs to be "refreshed".


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH RFC 1/2] clk: introduce clk_invalidate_rate()

[Neil Armstrong]

On 27/09/2019 02:14, Stephen Boyd wrote:
> Quoting Neil Armstrong (2019-09-19 03:25:17)
>> This introduces the clk_invalidate_rate() call used to recalculate the
>> rate and parent tree of a particular clock if it's known that the
>> underlying registers set has been altered by the firmware, like from
>> a suspend/resume handler running in trusted cpu mode.
>>
>> The call refreshes the actual parent and when changed, instructs CCF
>> the parent has changed. Finally the call will recalculate the rate of
>> each part of the tree to make sure the CCF cached tree is in sync with
>> the hardware.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
> 
> The knee-jerk reaction to these patches is that it shouldn't be a
> consumer API (i.e. taking a struct clk) but a provider API (i.e. taking
> a struct clk_hw). I haven't looked in any more detail but just know that
> it's a non-starter to be a consumer based API because we don't want
> random consumers out there to be telling the CCF or provider drivers
> that some clk has lost state and needs to be "refreshed".
> 

Totally agree, I hesitated and obviously did the wrong choice, but
this is a nit, the main algorithm is not tied to the API level.

Should I resend it with clk_hw ? the difference will be small and
the main subject is the resync algorithm.

Neil

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH RFC 1/2] clk: introduce clk_invalidate_rate()

[Jerome Brunet]

On Fri 27 Sep 2019 at 08:40, Neil Armstrong <narmstrong@baylibre.com> wrote:

> On 27/09/2019 02:14, Stephen Boyd wrote:
>> Quoting Neil Armstrong (2019-09-19 03:25:17)
>>> This introduces the clk_invalidate_rate() call used to recalculate the
>>> rate and parent tree of a particular clock if it's known that the
>>> underlying registers set has been altered by the firmware, like from
>>> a suspend/resume handler running in trusted cpu mode.
>>>
>>> The call refreshes the actual parent and when changed, instructs CCF
>>> the parent has changed. Finally the call will recalculate the rate of
>>> each part of the tree to make sure the CCF cached tree is in sync with
>>> the hardware.
>>>
>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>> ---
>> 
>> The knee-jerk reaction to these patches is that it shouldn't be a
>> consumer API (i.e. taking a struct clk) but a provider API (i.e. taking
>> a struct clk_hw). I haven't looked in any more detail but just know that
>> it's a non-starter to be a consumer based API because we don't want
>> random consumers out there to be telling the CCF or provider drivers
>> that some clk has lost state and needs to be "refreshed".
>> 
>
> Totally agree, I hesitated and obviously did the wrong choice, but
> this is a nit, the main algorithm is not tied to the API level.
>
> Should I resend it with clk_hw ? the difference will be small and
> the main subject is the resync algorithm.

Independent of the point above (partly a least), I wonder what will
happen in some particular use cases

* If clock is changed while in suspend. This clock can be a parent of
  the clock invalidated but currently is not. What happen, if later,
  it becomes the parent ?

  Since it is not parent on resume it won't be invalidated. CCF might
  still take a decision based on an invalid cached value.

* If a mux is changed while in suspend, the parent is not correct
  anymore. The proposed patch recurse through the parents, it might
  not invalidate what we need/expect ... things are getting a bit
  unpredictable

IOW, this change take a leaf clock and tries to tell CCF that any parent
of this clock should not be trusted, but it might get it wrong in some
cases.

I think we should do it in the opposite way:
 * Mark the "rogue" clock with a flag (CLK_REFRESH ?)
 * Let CCF update the children of these clocks based on the new status

Back to Stephen point, I don't know which API it should be, but I
think the platform (fw driver or power stuff - not only clock provider)
should be able somehow to trigger the mechanism to let CCF know
something sketchy may have happened.

For the parameter, maybe there should not be any (no struct clk or
clk_hw) ? Maybe it would better if we let CCF refresh all the "rogue"
clocks ?

>
> Neil


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH RFC 2/2] clk: meson: g12a: add suspend-resume hooks

[Neil Armstrong]

Add suspend and resume hooks used to refresh the CPU clock tree
when resuming from suspend, in the case where the PSCI firmware
alters the clock tree.

In the Amlogic G12A suspend/resume case, the PSCI firmware will
alter the Fixed PLL dyn tree when entering with the CPU clock from
this same tree, but using a different path to achieve the same rate.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/clk/meson/g12a.c | 71 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 63 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c
index b3af61cc6fb9..9f6f634225b7 100644
--- a/drivers/clk/meson/g12a.c
+++ b/drivers/clk/meson/g12a.c
@@ -4992,6 +4992,19 @@ static int meson_g12b_dvfs_setup(struct platform_device *pdev)
 	return 0;
 }
 
+static int meson_g12b_resume(struct device *dev)
+{
+	u32 ret;
+
+	ret = clk_invalidate_rate(
+			__clk_lookup(clk_hw_get_name(&g12b_cpu_clk.hw)));
+	if (ret)
+		return ret;
+
+	return clk_invalidate_rate(
+			__clk_lookup(clk_hw_get_name(&g12b_cpub_clk.hw)));
+}
+
 static int meson_g12a_dvfs_setup(struct platform_device *pdev)
 {
 	struct clk_hw **hws = g12a_hw_onecell_data.hws;
@@ -5024,34 +5037,68 @@ static int meson_g12a_dvfs_setup(struct platform_device *pdev)
 	return 0;
 }
 
+static int meson_g12a_resume(struct device *dev)
+{
+	return clk_invalidate_rate(
+			__clk_lookup(clk_hw_get_name(&g12a_cpu_clk.hw)));
+}
+
 struct meson_g12a_data {
 	const struct meson_eeclkc_data eeclkc_data;
 	int (*dvfs_setup)(struct platform_device *pdev);
+	int (*resume)(struct device *dev);
 };
 
+static const struct
+meson_g12a_data *meson_g12a_get_data(struct device *dev)
+{
+	const struct meson_eeclkc_data *eeclkc_data =
+			of_device_get_match_data(dev);
+
+	if (!eeclkc_data)
+		return ERR_PTR(-EINVAL);
+
+	return container_of(eeclkc_data, struct meson_g12a_data,
+			    eeclkc_data);
+}
+
 static int meson_g12a_probe(struct platform_device *pdev)
 {
-	const struct meson_eeclkc_data *eeclkc_data;
-	const struct meson_g12a_data *g12a_data;
 	int ret;
+	const struct meson_g12a_data *g12a_data =
+			meson_g12a_get_data(&pdev->dev);
 
-	eeclkc_data = of_device_get_match_data(&pdev->dev);
-	if (!eeclkc_data)
-		return -EINVAL;
+	if (IS_ERR(g12a_data))
+		return PTR_ERR(g12a_data);
 
 	ret = meson_eeclkc_probe(pdev);
 	if (ret)
 		return ret;
 
-	g12a_data = container_of(eeclkc_data, struct meson_g12a_data,
-				 eeclkc_data);
-
 	if (g12a_data->dvfs_setup)
 		return g12a_data->dvfs_setup(pdev);
 
 	return 0;
 }
 
+static int __maybe_unused g12a_clkc_suspend(struct device *dev)
+{
+	return 0;
+}
+
+static int __maybe_unused g12a_clkc_resume(struct device *dev)
+{
+	const struct meson_g12a_data *g12a_data = meson_g12a_get_data(dev);
+
+	if (IS_ERR(g12a_data))
+		return PTR_ERR(g12a_data);
+
+	if (g12a_data->resume)
+		return g12a_data->resume(dev);
+
+	return 0;
+}
+
 static const struct meson_g12a_data g12a_clkc_data = {
 	.eeclkc_data = {
 		.regmap_clks = g12a_clk_regmaps,
@@ -5061,6 +5108,7 @@ static const struct meson_g12a_data g12a_clkc_data = {
 		.init_count = ARRAY_SIZE(g12a_init_regs),
 	},
 	.dvfs_setup = meson_g12a_dvfs_setup,
+	.resume = meson_g12a_resume,
 };
 
 static const struct meson_g12a_data g12b_clkc_data = {
@@ -5070,6 +5118,7 @@ static const struct meson_g12a_data g12b_clkc_data = {
 		.hw_onecell_data = &g12b_hw_onecell_data,
 	},
 	.dvfs_setup = meson_g12b_dvfs_setup,
+	.resume = meson_g12b_resume,
 };
 
 static const struct meson_g12a_data sm1_clkc_data = {
@@ -5079,6 +5128,11 @@ static const struct meson_g12a_data sm1_clkc_data = {
 		.hw_onecell_data = &sm1_hw_onecell_data,
 	},
 	.dvfs_setup = meson_g12a_dvfs_setup,
+	.resume = meson_g12a_resume,
+};
+
+static const struct dev_pm_ops g12a_clkc_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(g12a_clkc_suspend, g12a_clkc_resume)
 };
 
 static const struct of_device_id clkc_match_table[] = {
@@ -5102,6 +5156,7 @@ static struct platform_driver g12a_driver = {
 	.driver		= {
 		.name	= "g12a-clkc",
 		.of_match_table = clkc_match_table,
+		.pm	= &g12a_clkc_dev_pm_ops,
 	},
 };
 
-- 
2.22.0


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH RFC 0/2] clk: meson: g12a: handle clock hw changes while in suspend

[Kevin Hilman]

Neil Armstrong <narmstrong@baylibre.com> writes:

> This serie aime to support when the suspend/resume firmware alters the
> clock tree, leading to an incorrect representation of the clock tree
> after a resume from suspend-to-mem.
>
> For the Amlogic G12A/G12B/SM1 case, the SCPI firmware handling suspend
> alters the CPU clock tree in various ways.
>
> Since we know which part of the tree is possibly altered, we introduce here
> the clk_invalidate_rate() function that will rebuild the tree from the
> hardware registers in case parents and dividers have changed.
>
> Finally we call clk_invalidate_rate() from a new resume callback to refresh
> the CPU clock tree after a resume.
>
> With the clock tree refreshed, CCF can now handle the new clock tree
> configuration and avoid crashing the system on further DVFS set_rates.

For clarification, does this series work without the other proposed
fixes[1]?  or is this dependent on that?

Kevin

[1] https://lore.kernel.org/linux-amlogic/20190919093627.21245-1-narmstrong@baylibre.com/

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH RFC 0/2] clk: meson: g12a: handle clock hw changes while in suspend

[Neil Armstrong]

On 26/09/2019 22:46, Kevin Hilman wrote:
> Neil Armstrong <narmstrong@baylibre.com> writes:
> 
>> This serie aime to support when the suspend/resume firmware alters the
>> clock tree, leading to an incorrect representation of the clock tree
>> after a resume from suspend-to-mem.
>>
>> For the Amlogic G12A/G12B/SM1 case, the SCPI firmware handling suspend
>> alters the CPU clock tree in various ways.
>>
>> Since we know which part of the tree is possibly altered, we introduce here
>> the clk_invalidate_rate() function that will rebuild the tree from the
>> hardware registers in case parents and dividers have changed.
>>
>> Finally we call clk_invalidate_rate() from a new resume callback to refresh
>> the CPU clock tree after a resume.
>>
>> With the clock tree refreshed, CCF can now handle the new clock tree
>> configuration and avoid crashing the system on further DVFS set_rates.
> 
> For clarification, does this series work without the other proposed
> fixes[1]?  or is this dependent on that?
> 
> Kevin
> 
> [1] https://lore.kernel.org/linux-amlogic/20190919093627.21245-1-narmstrong@baylibre.com/
> 

These are independent, but you'll need both to have suspend/resume fully working.

Neil

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic