[RFC v1 0/7] Meson8b: make the CPU clock mutable

[RFC v1 0/7] Meson8b: make the CPU clock mutable

[Martin Blumenstingl]

This allows changing the CPU clock on the 32-bit Amlogic Meson SoCs
(Meson8, Meson8b and Meson8m2).
CPU frequency scaling will be enabled with a separate series by adding
the CPU clock and the OPP tables to meson8.dtsi and meson8b.dtsi.

While changing the CPU frequency (sys_pll or any of it's post-dividers)
we need to run the CPU clock off the XTAL clock. Otherwise the system
will lock up because we need to disable the sys_pll to change it's
rate.

This also makes the clk-pll's .enable hook a no-op if the clock is
already enabled. Otherwise we're getting lockups when calling the
first clk_{prepare_}enable on the sys_pll or any of it's children (as
the CCF propagates the enable event up to the sys_pll). This is because
the .enable hook unconditionally disables and enables the clock.
However, we can't disable that clock (not even temporarily) if the CPU
is running off sys_pll.

Additionally this adds support for more M/N combinations in sys_pll to
achieve all of the OPPs on Meson8b and all OPPs <= 1608 MHz on Meson8
and Meson8m2.

Compared to Amlogic's 3.10 kernel there's one notable difference: we
are actually allowing changes to the sys_pll. Amlogic's kernel sets
sys_pll to a fixed rate during boot and then uses a timer generate a
"virtual clock rate" by toggling between various dividers (for example:
sys_pll is set to 1536MHz. to achieve 1008MHz they are toggling every
2500us between 1536MHZ and 768MHz so the average over <period, for
example one second> is 1008MHz).
I could reproduce any situation where changing sys_pll failed (for
example due to high temperature). To prove that I ran "stress --cpu 4"
for multiple hours and then cycled through all available CPU
frequencies (while keeping "stress" running in the background). This
worked fine on my Meson8b Odroid-C1 and EC-100 boards as well as my
Meson8m2 board.


Martin Blumenstingl (7):
  clk: meson: meson8b: run from the XTAL when changing the CPU frequency
  clk: meson: meson8b: do not use cpu_div3 for cpu_scale_out_sel
  clk: meson: clk-pll: check if the clock is already enabled
  clk: meson: clk-pll: add the is_enabled function in the clk_ops
  clk: meson: meson8b: mark the CPU clock as CLK_IS_CRITICAL
  clk: meson: meson8b: add support for more M/N values in sys_pll
  clk: meson: meson8b: allow changing the CPU clock tree

 drivers/clk/meson/clk-pll.c | 23 +++++++++
 drivers/clk/meson/meson8b.c | 94 +++++++++++++++++++++++++++++++++----
 2 files changed, 108 insertions(+), 9 deletions(-)

-- 
2.19.1

[RFC v1 1/7] clk: meson: meson8b: run from the XTAL when changing the CPU frequency

[Martin Blumenstingl]

Changing the CPU clock requires changing various clocks including the
SYS PLL. The existing meson clk-pll and clk-regmap drivers can change
all of the relevant clocks already.
However, changing for exampe the SYS PLL is problematic because as long
as the CPU is running off a clock derived from SYS PLL changing the
latter results in a full system lockup.
Fix this system lockup by switching the CPU clock to run off the XTAL
while we are changing the any of the clocks in the CPU clock tree.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/clk/meson/meson8b.c | 63 +++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/drivers/clk/meson/meson8b.c b/drivers/clk/meson/meson8b.c
index 9bd5920da0ff..40e77fe4ba7c 100644
--- a/drivers/clk/meson/meson8b.c
+++ b/drivers/clk/meson/meson8b.c
@@ -1103,6 +1103,53 @@ static const struct reset_control_ops meson8b_clk_reset_ops = {
 	.deassert = meson8b_clk_reset_deassert,
 };
 
+struct meson8b_nb_data {
+	struct notifier_block nb;
+	struct clk_hw_onecell_data *onecell_data;
+};
+
+static int meson8b_cpu_clk_notifier_cb(struct notifier_block *nb,
+				       unsigned long event, void *data)
+{
+	struct meson8b_nb_data *nb_data =
+		container_of(nb, struct meson8b_nb_data, nb);
+	struct clk_hw **hws = nb_data->onecell_data->hws;
+	struct clk_hw *cpu_clk_hw, *parent_clk_hw;
+	struct clk *cpu_clk, *parent_clk;
+	int ret;
+
+	switch (event) {
+	case PRE_RATE_CHANGE:
+		parent_clk_hw = hws[CLKID_XTAL];
+		break;
+
+	case POST_RATE_CHANGE:
+		parent_clk_hw = hws[CLKID_CPU_SCALE_OUT_SEL];
+		break;
+
+	default:
+		return NOTIFY_DONE;
+	}
+
+	cpu_clk_hw = hws[CLKID_CPUCLK];
+	cpu_clk = __clk_lookup(clk_hw_get_name(cpu_clk_hw));
+
+	parent_clk = __clk_lookup(clk_hw_get_name(parent_clk_hw));
+
+	ret = clk_set_parent(cpu_clk, parent_clk);
+	if (ret)
+		return notifier_from_errno(ret);
+
+	udelay(100);
+
+	return NOTIFY_OK;
+}
+
+static struct meson8b_nb_data meson8b_cpu_nb_data = {
+	.nb.notifier_call = meson8b_cpu_clk_notifier_cb,
+	.onecell_data = &meson8b_hw_onecell_data,
+};
+
 static const struct regmap_config clkc_regmap_config = {
 	.reg_bits       = 32,
 	.val_bits       = 32,
@@ -1112,6 +1159,8 @@ static const struct regmap_config clkc_regmap_config = {
 static void __init meson8b_clkc_init(struct device_node *np)
 {
 	struct meson8b_clk_reset *rstc;
+	const char *notifier_clk_name;
+	struct clk *notifier_clk;
 	void __iomem *clk_base;
 	struct regmap *map;
 	int i, ret;
@@ -1166,6 +1215,20 @@ static void __init meson8b_clkc_init(struct device_node *np)
 			return;
 	}
 
+	/*
+	 * FIXME we shouldn't program the muxes in notifier handlers. The
+	 * tricky programming sequence will be handled by the forthcoming
+	 * coordinated clock rates mechanism once that feature is released.
+	 */
+	notifier_clk_name = clk_hw_get_name(&meson8b_cpu_scale_out_sel.hw);
+	notifier_clk = __clk_lookup(notifier_clk_name);
+	ret = clk_notifier_register(notifier_clk, &meson8b_cpu_nb_data.nb);
+	if (ret) {
+		pr_err("%s: failed to register the CPU clock notifier\n",
+		       __func__);
+		return;
+	}
+
 	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get,
 				     &meson8b_hw_onecell_data);
 	if (ret)
-- 
2.19.1

Re: [RFC v1 1/7] clk: meson: meson8b: run from the XTAL when changing the CPU frequency

[Jerome Brunet]

On Wed, 2018-11-14 at 23:57 +0100, Martin Blumenstingl wrote:
> Changing the CPU clock requires changing various clocks including the
> SYS PLL. The existing meson clk-pll and clk-regmap drivers can change
> all of the relevant clocks already.
> However, changing for exampe the SYS PLL is problematic because as long
> as the CPU is running off a clock derived from SYS PLL changing the
> latter results in a full system lockup.
> Fix this system lockup by switching the CPU clock to run off the XTAL
> while we are changing the any of the clocks in the CPU clock tree.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Original submission wasn't that far off after all ;)

Thanks for digging and fixing all the bugs around it ! It must have been quite
a challenge !

If you don't mind, I would prefer if your series put all the clock fixes
first, and this particular change just before the last one allowing RW ops.

Reviewed-by: Jerome Brunet <jbrunet@baylibre.com>

> ---
>  drivers/clk/meson/meson8b.c | 63 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
> 
> diff --git a/drivers/clk/meson/meson8b.c b/drivers/clk/meson/meson8b.c
> index 9bd5920da0ff..40e77fe4ba7c 100644
> --- a/drivers/clk/meson/meson8b.c
> +++ b/drivers/clk/meson/meson8b.c
> @@ -1103,6 +1103,53 @@ static const struct reset_control_ops
> meson8b_clk_reset_ops = {
>  	.deassert = meson8b_clk_reset_deassert,
>  };
>  
> +struct meson8b_nb_data {
> +	struct notifier_block nb;
> +	struct clk_hw_onecell_data *onecell_data;
> +};
> +
> +static int meson8b_cpu_clk_notifier_cb(struct notifier_block *nb,
> +				       unsigned long event, void *data)
> +{
> +	struct meson8b_nb_data *nb_data =
> +		container_of(nb, struct meson8b_nb_data, nb);
> +	struct clk_hw **hws = nb_data->onecell_data->hws;
> +	struct clk_hw *cpu_clk_hw, *parent_clk_hw;
> +	struct clk *cpu_clk, *parent_clk;
> +	int ret;
> +
> +	switch (event) {
> +	case PRE_RATE_CHANGE:
> +		parent_clk_hw = hws[CLKID_XTAL];
> +		break;
> +
> +	case POST_RATE_CHANGE:
> +		parent_clk_hw = hws[CLKID_CPU_SCALE_OUT_SEL];
> +		break;
> +
> +	default:
> +		return NOTIFY_DONE;
> +	}
> +
> +	cpu_clk_hw = hws[CLKID_CPUCLK];
> +	cpu_clk = __clk_lookup(clk_hw_get_name(cpu_clk_hw));
> +
> +	parent_clk = __clk_lookup(clk_hw_get_name(parent_clk_hw));
> +
> +	ret = clk_set_parent(cpu_clk, parent_clk);
> +	if (ret)
> +		return notifier_from_errno(ret);
> +
> +	udelay(100);
> +
> +	return NOTIFY_OK;
> +}
> +
> +static struct meson8b_nb_data meson8b_cpu_nb_data = {
> +	.nb.notifier_call = meson8b_cpu_clk_notifier_cb,
> +	.onecell_data = &meson8b_hw_onecell_data,
> +};
> +
>  static const struct regmap_config clkc_regmap_config = {
>  	.reg_bits       = 32,
>  	.val_bits       = 32,
> @@ -1112,6 +1159,8 @@ static const struct regmap_config clkc_regmap_config =
> {
>  static void __init meson8b_clkc_init(struct device_node *np)
>  {
>  	struct meson8b_clk_reset *rstc;
> +	const char *notifier_clk_name;
> +	struct clk *notifier_clk;
>  	void __iomem *clk_base;
>  	struct regmap *map;
>  	int i, ret;
> @@ -1166,6 +1215,20 @@ static void __init meson8b_clkc_init(struct
> device_node *np)
>  			return;
>  	}
>  
> +	/*
> +	 * FIXME we shouldn't program the muxes in notifier handlers. The
> +	 * tricky programming sequence will be handled by the forthcoming
> +	 * coordinated clock rates mechanism once that feature is released.
> +	 */
> +	notifier_clk_name = clk_hw_get_name(&meson8b_cpu_scale_out_sel.hw);
> +	notifier_clk = __clk_lookup(notifier_clk_name);
> +	ret = clk_notifier_register(notifier_clk, &meson8b_cpu_nb_data.nb);
> +	if (ret) {
> +		pr_err("%s: failed to register the CPU clock notifier\n",
> +		       __func__);
> +		return;
> +	}
> +
>  	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get,
>  				     &meson8b_hw_onecell_data);
>  	if (ret)

[RFC v1 2/7] clk: meson: meson8b: do not use cpu_div3 for cpu_scale_out_sel

[Martin Blumenstingl]

The cpu_div3 clock (cpu_in divided by 3) generates a signal with a duty
cycle of 33%. The CPU clock however requires a clock signal with a duty
cycle of 50% to run stable.
cpu_div3 was observed to be problematic when cycling through all
available CPU frequencies (with additional patches on top of this one)
while running "stress --cpu 4" in the background. This caused sporadic
hangs where the whole system would fully lock up.

Amlogic's 3.10 kernel code also does not use the cpu_div3 clock either
when changing the CPU clock.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/clk/meson/meson8b.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/meson/meson8b.c b/drivers/clk/meson/meson8b.c
index 40e77fe4ba7c..8a3c346e110d 100644
--- a/drivers/clk/meson/meson8b.c
+++ b/drivers/clk/meson/meson8b.c
@@ -608,20 +608,27 @@ static struct clk_regmap meson8b_cpu_scale_div = {
 	},
 };
 
+static u32 mux_table_cpu_scale_out_sel[] = { 0, 1, 3 };
 static struct clk_regmap meson8b_cpu_scale_out_sel = {
 	.data = &(struct clk_regmap_mux_data){
 		.offset = HHI_SYS_CPU_CLK_CNTL0,
 		.mask = 0x3,
 		.shift = 2,
+		.table = mux_table_cpu_scale_out_sel,
 	},
 	.hw.init = &(struct clk_init_data){
 		.name = "cpu_scale_out_sel",
 		.ops = &clk_regmap_mux_ro_ops,
+		/*
+		 * NOTE: We are skipping the parent with value 0x2 (which is
+		 * "cpu_div3") because it results in a duty cycle of 33% which
+		 * makes the system unstable and can result in a lockup of the
+		 * whole system.
+		 */
 		.parent_names = (const char *[]) { "cpu_in_sel",
 						   "cpu_div2",
-						   "cpu_div3",
 						   "cpu_scale_div" },
-		.num_parents = 4,
+		.num_parents = 3,
 		.flags = CLK_SET_RATE_PARENT,
 	},
 };
-- 
2.19.1

Re: [RFC v1 2/7] clk: meson: meson8b: do not use cpu_div3 for cpu_scale_out_sel

[Jerome Brunet]

On Wed, 2018-11-14 at 23:57 +0100, Martin Blumenstingl wrote:
> The cpu_div3 clock (cpu_in divided by 3) generates a signal with a duty
> cycle of 33%. The CPU clock however requires a clock signal with a duty
> cycle of 50% to run stable.
> cpu_div3 was observed to be problematic when cycling through all
> available CPU frequencies (with additional patches on top of this one)
> while running "stress --cpu 4" in the background. This caused sporadic
> hangs where the whole system would fully lock up.
> 
> Amlogic's 3.10 kernel code also does not use the cpu_div3 clock either
> when changing the CPU clock.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/clk/meson/meson8b.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/meson/meson8b.c b/drivers/clk/meson/meson8b.c
> index 40e77fe4ba7c..8a3c346e110d 100644
> --- a/drivers/clk/meson/meson8b.c
> +++ b/drivers/clk/meson/meson8b.c
> @@ -608,20 +608,27 @@ static struct clk_regmap meson8b_cpu_scale_div = {
>  	},
>  };
>  
> +static u32 mux_table_cpu_scale_out_sel[] = { 0, 1, 3 };
>  static struct clk_regmap meson8b_cpu_scale_out_sel = {
>  	.data = &(struct clk_regmap_mux_data){
>  		.offset = HHI_SYS_CPU_CLK_CNTL0,
>  		.mask = 0x3,
>  		.shift = 2,
> +		.table = mux_table_cpu_scale_out_sel,
>  	},
>  	.hw.init = &(struct clk_init_data){
>  		.name = "cpu_scale_out_sel",
>  		.ops = &clk_regmap_mux_ro_ops,
> +		/*
> +		 * NOTE: We are skipping the parent with value 0x2 (which is
> +		 * "cpu_div3") because it results in a duty cycle of 33% which
> +		 * makes the system unstable and can result in a lockup of the
> +		 * whole system.
> +		 */
>  		.parent_names = (const char *[]) { "cpu_in_sel",
>  						   "cpu_div2",
> -						   "cpu_div3",
>  						   "cpu_scale_div" },
> -		.num_parents = 4,
> +		.num_parents = 3,
>  		.flags = CLK_SET_RATE_PARENT,
>  	},
>  };

Reviewed-by: Jerome Brunet <jbrunet@baylibre.com>

[RFC v1 3/7] clk: meson: clk-pll: check if the clock is already enabled

[Martin Blumenstingl]

Since commit 6f888e7bc7bd58 ("clk: meson: clk-pll: add enable bit") our
PLLs also support the "enable" bit. Currently meson_clk_pll_enable
unconditionally resets the PLL, enables it, takes it out of reset and
waits until it is locked.

This works fine for our current clock trees. However, there will be a
problem once we allow modifications to sys_pll on Meson8, Meson8b and
Meson8m2 (which will be required for CPU frequency scaling):
the CPU clock is derived from the sys_pll clock. Once clk_enable is
called on the CPU clock this will be propagated by the common clock
framework up until the sys_pll clock. If we reset the PLL
unconditionally in meson_clk_pll_enable the CPU will be stopped (on
Meson8, Meson8b and Meson8m2).
To prevent this we simply check if the PLL is already enabled and do
reset the PLL if it's already enabled and locked.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/clk/meson/clk-pll.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
index f5b5b3fabe3c..b46cca953f4f 100644
--- a/drivers/clk/meson/clk-pll.c
+++ b/drivers/clk/meson/clk-pll.c
@@ -200,11 +200,32 @@ static void meson_clk_pll_init(struct clk_hw *hw)
 	}
 }
 
+static int meson_clk_pll_is_enabled(struct clk_hw *hw)
+{
+	struct clk_regmap *clk = to_clk_regmap(hw);
+	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
+
+	if (meson_parm_read(clk->map, &pll->rst))
+		return 0;
+
+	if (!meson_parm_read(clk->map, &pll->en))
+		return 0;
+
+	if (!meson_parm_read(clk->map, &pll->l))
+		return 0;
+
+	return 1;
+}
+
 static int meson_clk_pll_enable(struct clk_hw *hw)
 {
 	struct clk_regmap *clk = to_clk_regmap(hw);
 	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
 
+	/* do nothing if the PLL is already enabled */
+	if (meson_clk_pll_is_enabled(hw))
+		return 0;
+
 	/* Make sure the pll is in reset */
 	meson_parm_write(clk->map, &pll->rst, 1);
 
-- 
2.19.1

Re: [RFC v1 3/7] clk: meson: clk-pll: check if the clock is already enabled

[Jerome Brunet]

On Wed, 2018-11-14 at 23:57 +0100, Martin Blumenstingl wrote:
> Since commit 6f888e7bc7bd58 ("clk: meson: clk-pll: add enable bit") our
> PLLs also support the "enable" bit. Currently meson_clk_pll_enable
> unconditionally resets the PLL, enables it, takes it out of reset and
> waits until it is locked.
> 
> This works fine for our current clock trees. However, there will be a
> problem once we allow modifications to sys_pll on Meson8, Meson8b and
> Meson8m2 (which will be required for CPU frequency scaling):
> the CPU clock is derived from the sys_pll clock. Once clk_enable is
> called on the CPU clock this will be propagated by the common clock
> framework up until the sys_pll clock. If we reset the PLL
> unconditionally in meson_clk_pll_enable the CPU will be stopped (on
> Meson8, Meson8b and Meson8m2).
> To prevent this we simply check if the PLL is already enabled and do
> reset the PLL if it's already enabled and locked.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/clk/meson/clk-pll.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
> index f5b5b3fabe3c..b46cca953f4f 100644
> --- a/drivers/clk/meson/clk-pll.c
> +++ b/drivers/clk/meson/clk-pll.c
> @@ -200,11 +200,32 @@ static void meson_clk_pll_init(struct clk_hw *hw)
>  	}
>  }
>  
> +static int meson_clk_pll_is_enabled(struct clk_hw *hw)
> +{
> +	struct clk_regmap *clk = to_clk_regmap(hw);
> +	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
> +
> +	if (meson_parm_read(clk->map, &pll->rst))
> +		return 0;
> +
> +	if (!meson_parm_read(clk->map, &pll->en))
> +		return 0;
> +
> +	if (!meson_parm_read(clk->map, &pll->l))
> +		return 0;

Could you use an OR instead of these 3 seperate checks ?

> +
> +	return 1;
> +}
> +
>  static int meson_clk_pll_enable(struct clk_hw *hw)
>  {
>  	struct clk_regmap *clk = to_clk_regmap(hw);
>  	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
>  
> +	/* do nothing if the PLL is already enabled */
> +	if (meson_clk_pll_is_enabled(hw))
> +		return 0;
> +
>  	/* Make sure the pll is in reset */
>  	meson_parm_write(clk->map, &pll->rst, 1);
> 
>  

With the small comment above taken care of, it makes perfect sense
and it will be valuable to other PLLs, Thx Martin !

Reviewed-by: Jerome Brunet <jbrunet@baylibre.com>

[RFC v1 4/7] clk: meson: clk-pll: add the is_enabled function in the clk_ops

[Martin Blumenstingl]

Now that we have a utility function to check whether the PLL is enabled
we can also pass that to our clk_ops to let the common clock framework
know about the status of the hardware clock.
For now this is of limited use since the only common clock framework's
internal "disabled unused clocks" mechanism checks for this. Everything
else still uses the ref-counting (internal to the common clock
framework) when clk_enable is called.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/clk/meson/clk-pll.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
index b46cca953f4f..65eeae0989d9 100644
--- a/drivers/clk/meson/clk-pll.c
+++ b/drivers/clk/meson/clk-pll.c
@@ -309,10 +309,12 @@ const struct clk_ops meson_clk_pll_ops = {
 	.recalc_rate	= meson_clk_pll_recalc_rate,
 	.round_rate	= meson_clk_pll_round_rate,
 	.set_rate	= meson_clk_pll_set_rate,
+	.is_enabled	= meson_clk_pll_is_enabled,
 	.enable		= meson_clk_pll_enable,
 	.disable	= meson_clk_pll_disable
 };
 
 const struct clk_ops meson_clk_pll_ro_ops = {
 	.recalc_rate	= meson_clk_pll_recalc_rate,
+	.is_enabled	= meson_clk_pll_is_enabled,
 };
-- 
2.19.1

Re: [RFC v1 4/7] clk: meson: clk-pll: add the is_enabled function in the clk_ops

[Jerome Brunet]

On Wed, 2018-11-14 at 23:57 +0100, Martin Blumenstingl wrote:
> Now that we have a utility function to check whether the PLL is enabled
> we can also pass that to our clk_ops to let the common clock framework
> know about the status of the hardware clock.
> For now this is of limited use since the only common clock framework's

Still, it is nice to have ;)

> internal "disabled unused clocks" mechanism checks for this. Everything
> else still uses the ref-counting (internal to the common clock
> framework) when clk_enable is called.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/clk/meson/clk-pll.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
> index b46cca953f4f..65eeae0989d9 100644
> --- a/drivers/clk/meson/clk-pll.c
> +++ b/drivers/clk/meson/clk-pll.c
> @@ -309,10 +309,12 @@ const struct clk_ops meson_clk_pll_ops = {
>  	.recalc_rate	= meson_clk_pll_recalc_rate,
>  	.round_rate	= meson_clk_pll_round_rate,
>  	.set_rate	= meson_clk_pll_set_rate,
> +	.is_enabled	= meson_clk_pll_is_enabled,
>  	.enable		= meson_clk_pll_enable,
>  	.disable	= meson_clk_pll_disable
>  };
>  
>  const struct clk_ops meson_clk_pll_ro_ops = {
>  	.recalc_rate	= meson_clk_pll_recalc_rate,
> +	.is_enabled	= meson_clk_pll_is_enabled,
>  };

Looks good to me
Feel free to squash this with patch 3.

Jerome

[RFC v1 5/7] clk: meson: meson8b: mark the CPU clock as CLK_IS_CRITICAL

[Martin Blumenstingl]

We don't want the common clock framework to disable the "cpu_clk" if
it's not used by any device. The cpufreq-dt driver does not enable the
CPU clocks. However, even if it would we would still want the CPU clock
to be enabled at all times because the CPU clock is also required even
if we disable CPU frequency scaling on a specific board.

The reason why we want the CPU clock to be enabled is a clock further up
in the tree:
Since commit 6f888e7bc7bd58 ("clk: meson: clk-pll: add enable bit") the
sys_pll can be disabled. However, since the CPU clock is derived from
sys_pll we don't want sys_pll to get disabled. The common clock
framework takes care of that for us by enabling all parent clocks of our
CPU clock when we mark the CPU clock with CLK_IS_CRITICAL.

Until now this is not a problem yet because all clocks in the CPU
clock's tree (including sys_pll) are read-only. However, once we allow
modifications to the clocks in that tree we will need this.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/clk/meson/meson8b.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/meson/meson8b.c b/drivers/clk/meson/meson8b.c
index 8a3c346e110d..d566dd5bc567 100644
--- a/drivers/clk/meson/meson8b.c
+++ b/drivers/clk/meson/meson8b.c
@@ -646,7 +646,8 @@ static struct clk_regmap meson8b_cpu_clk = {
 						  "cpu_scale_out_sel" },
 		.num_parents = 2,
 		.flags = (CLK_SET_RATE_PARENT |
-			  CLK_SET_RATE_NO_REPARENT),
+			  CLK_SET_RATE_NO_REPARENT |
+			  CLK_IS_CRITICAL),
 	},
 };
 
-- 
2.19.1

Re: [RFC v1 5/7] clk: meson: meson8b: mark the CPU clock as CLK_IS_CRITICAL

[Jerome Brunet]

On Wed, 2018-11-14 at 23:57 +0100, Martin Blumenstingl wrote:
> We don't want the common clock framework to disable the "cpu_clk" if
> it's not used by any device. The cpufreq-dt driver does not enable the
> CPU clocks. However, even if it would we would still want the CPU clock
> to be enabled at all times because the CPU clock is also required even
> if we disable CPU frequency scaling on a specific board.
> 
> The reason why we want the CPU clock to be enabled is a clock further up
> in the tree:
> Since commit 6f888e7bc7bd58 ("clk: meson: clk-pll: add enable bit") the
> sys_pll can be disabled. However, since the CPU clock is derived from
> sys_pll we don't want sys_pll to get disabled. The common clock
> framework takes care of that for us by enabling all parent clocks of our
> CPU clock when we mark the CPU clock with CLK_IS_CRITICAL.
> 
> Until now this is not a problem yet because all clocks in the CPU
> clock's tree (including sys_pll) are read-only. However, once we allow
> modifications to the clocks in that tree we will need this.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/clk/meson/meson8b.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/meson/meson8b.c b/drivers/clk/meson/meson8b.c
> index 8a3c346e110d..d566dd5bc567 100644
> --- a/drivers/clk/meson/meson8b.c
> +++ b/drivers/clk/meson/meson8b.c
> @@ -646,7 +646,8 @@ static struct clk_regmap meson8b_cpu_clk = {
>  						  "cpu_scale_out_sel" },
>  		.num_parents = 2,
>  		.flags = (CLK_SET_RATE_PARENT |
> -			  CLK_SET_RATE_NO_REPARENT),
> +			  CLK_SET_RATE_NO_REPARENT |
> +			  CLK_IS_CRITICAL),
>  	},
>  };
>  

Acked-by: Jerome Brunet <jbrunet@baylibre.com>

[RFC v1 6/7] clk: meson: meson8b: add support for more M/N values in sys_pll

[Martin Blumenstingl]

The sys_pll on the EC-100 board is configured to 1584MHz at boot
(either by u-boot, firmware or chip defaults). This is achieved by using
M = 66, N = 1 (24MHz * 66 / 1).
At boot the CPU clock is running off sys_pll divided by 2 which results
in 792MHz. Thus M = 66 is considered to be a "safe" value for Meson8b.

To achieve 1608MHz (one of the CPU OPPs on Meson8 and Meson8m2) we need
M = 67, N = 1. I ran "stress --cpu 4" while infinitely cycling through
all available frequencies on my Meson8m2 board and could not spot any
issues with this setting (after ~12 hours of running this).

On Meson8, Meson8b and Meson8m2 we also want to be able to use 408MHz
and 816MHz CPU frequencies. These can be achieved by dividing sys_pll by
4 (for 408MHz) or 2 (for 816MHz). That means that sys_pll has to run at
1632MHz which can be generated using M = 68, N = 1.
Similarily we also want to be able to use 1008MHz as CPU frequency. This
means that sys_pll has to run either at 1008MHz or 2016MHz. The former
would result in an M value of 42, which is lower than the smallest value
used by the 3.10 GPL kernel sources from Amlogic (50 is the lower limit
there). Thus we need to run sys_pll at 2016MHz which can ge generated
using M = 84, N = 1.
I tested M = 68 and M = 84 on my Meson8b Odroid-C1 and my Meson8m2 board
by running "stress --cpu 4" while infinitely cycling thorugh all
available frequencies. I could not spot any issues after ~12 hours of
running this.

Amlogic's 3.10 GPL kernel sources have more M/N combinations. I did not
add them yet because M = 74 (to achieve close to 1800MHz on Meson8) and
M = 82 (to achieve close to 1992MHz on Meson8 as well) caused my
Meson8m2 board to hang randomly. It's not clear why this is (for example
because the board's voltage regulator design is bad, some missing bits
for these values in our clk-pll driver, etc.). Thus the following M
values from the Amlogic 3.10 GPL kernel sources are skipped as of now:
69, 70, 71, 72, 73, 74, 76, 78, 80, 82, 84, 86, 88, 90, 92, 94, 96, 98

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/clk/meson/meson8b.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/clk/meson/meson8b.c b/drivers/clk/meson/meson8b.c
index d566dd5bc567..c06a1a7faa4c 100644
--- a/drivers/clk/meson/meson8b.c
+++ b/drivers/clk/meson/meson8b.c
@@ -43,6 +43,11 @@ static const struct pll_params_table sys_pll_params_table[] = {
 	PLL_PARAMS(62, 1),
 	PLL_PARAMS(63, 1),
 	PLL_PARAMS(64, 1),
+	PLL_PARAMS(65, 1),
+	PLL_PARAMS(66, 1),
+	PLL_PARAMS(67, 1),
+	PLL_PARAMS(68, 1),
+	PLL_PARAMS(84, 1),
 	{ /* sentinel */ },
 };
 
-- 
2.19.1

Re: [RFC v1 6/7] clk: meson: meson8b: add support for more M/N values in sys_pll

[Jerome Brunet]

On Wed, 2018-11-14 at 23:57 +0100, Martin Blumenstingl wrote:
> The sys_pll on the EC-100 board is configured to 1584MHz at boot
> (either by u-boot, firmware or chip defaults). This is achieved by using
> M = 66, N = 1 (24MHz * 66 / 1).
> At boot the CPU clock is running off sys_pll divided by 2 which results
> in 792MHz. Thus M = 66 is considered to be a "safe" value for Meson8b.
> 
> To achieve 1608MHz (one of the CPU OPPs on Meson8 and Meson8m2) we need
> M = 67, N = 1. I ran "stress --cpu 4" while infinitely cycling through
> all available frequencies on my Meson8m2 board and could not spot any
> issues with this setting (after ~12 hours of running this).
> 
> On Meson8, Meson8b and Meson8m2 we also want to be able to use 408MHz
> and 816MHz CPU frequencies. These can be achieved by dividing sys_pll by
> 4 (for 408MHz) or 2 (for 816MHz). That means that sys_pll has to run at
> 1632MHz which can be generated using M = 68, N = 1.
> Similarily we also want to be able to use 1008MHz as CPU frequency. This
> means that sys_pll has to run either at 1008MHz or 2016MHz. The former
> would result in an M value of 42, which is lower than the smallest value
> used by the 3.10 GPL kernel sources from Amlogic (50 is the lower limit
> there). Thus we need to run sys_pll at 2016MHz which can ge generated
> using M = 84, N = 1.
> I tested M = 68 and M = 84 on my Meson8b Odroid-C1 and my Meson8m2 board
> by running "stress --cpu 4" while infinitely cycling thorugh all
> available frequencies. I could not spot any issues after ~12 hours of
> running this.
> 
> Amlogic's 3.10 GPL kernel sources have more M/N combinations. I did not
> add them yet because M = 74 (to achieve close to 1800MHz on Meson8) and
> M = 82 (to achieve close to 1992MHz on Meson8 as well) caused my
> Meson8m2 board to hang randomly. It's not clear why this is (for example
> because the board's voltage regulator design is bad, some missing bits
> for these values in our clk-pll driver, etc.). Thus the following M
> values from the Amlogic 3.10 GPL kernel sources are skipped as of now:
> 69, 70, 71, 72, 73, 74, 76, 78, 80, 82, 84, 86, 88, 90, 92, 94, 96, 98
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/clk/meson/meson8b.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/clk/meson/meson8b.c b/drivers/clk/meson/meson8b.c
> index d566dd5bc567..c06a1a7faa4c 100644
> --- a/drivers/clk/meson/meson8b.c
> +++ b/drivers/clk/meson/meson8b.c
> @@ -43,6 +43,11 @@ static const struct pll_params_table
> sys_pll_params_table[] = {
>  	PLL_PARAMS(62, 1),
>  	PLL_PARAMS(63, 1),
>  	PLL_PARAMS(64, 1),
> +	PLL_PARAMS(65, 1),
> +	PLL_PARAMS(66, 1),
> +	PLL_PARAMS(67, 1),
> +	PLL_PARAMS(68, 1),
> +	PLL_PARAMS(84, 1),
>  	{ /* sentinel */ },
>  };
>  

Acked-by: Jerome Brunet <jbrunet@baylibre.com>

[RFC v1 7/7] clk: meson: meson8b: allow changing the CPU clock tree

[Martin Blumenstingl]

Currently all clocks in the CPU clock tree are marked as read-only
(using the corresponding _ro_ clk_ops). This was correct since changing
the clock tree could cause the system to lock up.
Switch all clocks to their corresponding clk_ops variant which is not
read-only to allow changing the CPU clock tree since the bug which
locked up the system is now fixed (by switching the CPU clock temporary
to run off XTAL while changing the CPU clock tree).

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/clk/meson/meson8b.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/meson/meson8b.c b/drivers/clk/meson/meson8b.c
index c06a1a7faa4c..b3bdc7e05441 100644
--- a/drivers/clk/meson/meson8b.c
+++ b/drivers/clk/meson/meson8b.c
@@ -203,7 +203,7 @@ static struct clk_regmap meson8b_sys_pll_dco = {
 	},
 	.hw.init = &(struct clk_init_data){
 		.name = "sys_pll_dco",
-		.ops = &meson_clk_pll_ro_ops,
+		.ops = &meson_clk_pll_ops,
 		.parent_names = (const char *[]){ "xtal" },
 		.num_parents = 1,
 	},
@@ -218,7 +218,7 @@ static struct clk_regmap meson8b_sys_pll = {
 	},
 	.hw.init = &(struct clk_init_data){
 		.name = "sys_pll",
-		.ops = &clk_regmap_divider_ro_ops,
+		.ops = &clk_regmap_divider_ops,
 		.parent_names = (const char *[]){ "sys_pll_dco" },
 		.num_parents = 1,
 		.flags = CLK_SET_RATE_PARENT,
@@ -552,7 +552,7 @@ static struct clk_regmap meson8b_cpu_in_sel = {
 	},
 	.hw.init = &(struct clk_init_data){
 		.name = "cpu_in_sel",
-		.ops = &clk_regmap_mux_ro_ops,
+		.ops = &clk_regmap_mux_ops,
 		.parent_names = (const char *[]){ "xtal", "sys_pll" },
 		.num_parents = 2,
 		.flags = (CLK_SET_RATE_PARENT |
@@ -606,7 +606,7 @@ static struct clk_regmap meson8b_cpu_scale_div = {
 	},
 	.hw.init = &(struct clk_init_data){
 		.name = "cpu_scale_div",
-		.ops = &clk_regmap_divider_ro_ops,
+		.ops = &clk_regmap_divider_ops,
 		.parent_names = (const char *[]){ "cpu_in_sel" },
 		.num_parents = 1,
 		.flags = CLK_SET_RATE_PARENT,
@@ -623,7 +623,7 @@ static struct clk_regmap meson8b_cpu_scale_out_sel = {
 	},
 	.hw.init = &(struct clk_init_data){
 		.name = "cpu_scale_out_sel",
-		.ops = &clk_regmap_mux_ro_ops,
+		.ops = &clk_regmap_mux_ops,
 		/*
 		 * NOTE: We are skipping the parent with value 0x2 (which is
 		 * "cpu_div3") because it results in a duty cycle of 33% which
@@ -646,7 +646,7 @@ static struct clk_regmap meson8b_cpu_clk = {
 	},
 	.hw.init = &(struct clk_init_data){
 		.name = "cpu_clk",
-		.ops = &clk_regmap_mux_ro_ops,
+		.ops = &clk_regmap_mux_ops,
 		.parent_names = (const char *[]){ "xtal",
 						  "cpu_scale_out_sel" },
 		.num_parents = 2,
-- 
2.19.1

Re: [RFC v1 7/7] clk: meson: meson8b: allow changing the CPU clock tree

[Jerome Brunet]

On Wed, 2018-11-14 at 23:57 +0100, Martin Blumenstingl wrote:
> Currently all clocks in the CPU clock tree are marked as read-only
> (using the corresponding _ro_ clk_ops). This was correct since changing
> the clock tree could cause the system to lock up.
> Switch all clocks to their corresponding clk_ops variant which is not
> read-only to allow changing the CPU clock tree since the bug which
> locked up the system is now fixed (by switching the CPU clock temporary
> to run off XTAL while changing the CPU clock tree).
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/clk/meson/meson8b.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/clk/meson/meson8b.c b/drivers/clk/meson/meson8b.c
> index c06a1a7faa4c..b3bdc7e05441 100644
> --- a/drivers/clk/meson/meson8b.c
> +++ b/drivers/clk/meson/meson8b.c
> @@ -203,7 +203,7 @@ static struct clk_regmap meson8b_sys_pll_dco = {
>  	},
>  	.hw.init = &(struct clk_init_data){
>  		.name = "sys_pll_dco",
> -		.ops = &meson_clk_pll_ro_ops,
> +		.ops = &meson_clk_pll_ops,
>  		.parent_names = (const char *[]){ "xtal" },
>  		.num_parents = 1,
>  	},
> @@ -218,7 +218,7 @@ static struct clk_regmap meson8b_sys_pll = {
>  	},
>  	.hw.init = &(struct clk_init_data){
>  		.name = "sys_pll",
> -		.ops = &clk_regmap_divider_ro_ops,
> +		.ops = &clk_regmap_divider_ops,
>  		.parent_names = (const char *[]){ "sys_pll_dco" },
>  		.num_parents = 1,
>  		.flags = CLK_SET_RATE_PARENT,
> @@ -552,7 +552,7 @@ static struct clk_regmap meson8b_cpu_in_sel = {
>  	},
>  	.hw.init = &(struct clk_init_data){
>  		.name = "cpu_in_sel",
> -		.ops = &clk_regmap_mux_ro_ops,
> +		.ops = &clk_regmap_mux_ops,
>  		.parent_names = (const char *[]){ "xtal", "sys_pll" },
>  		.num_parents = 2,
>  		.flags = (CLK_SET_RATE_PARENT |
> @@ -606,7 +606,7 @@ static struct clk_regmap meson8b_cpu_scale_div = {
>  	},
>  	.hw.init = &(struct clk_init_data){
>  		.name = "cpu_scale_div",
> -		.ops = &clk_regmap_divider_ro_ops,
> +		.ops = &clk_regmap_divider_ops,
>  		.parent_names = (const char *[]){ "cpu_in_sel" },
>  		.num_parents = 1,
>  		.flags = CLK_SET_RATE_PARENT,
> @@ -623,7 +623,7 @@ static struct clk_regmap meson8b_cpu_scale_out_sel = {
>  	},
>  	.hw.init = &(struct clk_init_data){
>  		.name = "cpu_scale_out_sel",
> -		.ops = &clk_regmap_mux_ro_ops,
> +		.ops = &clk_regmap_mux_ops,
>  		/*
>  		 * NOTE: We are skipping the parent with value 0x2 (which is
>  		 * "cpu_div3") because it results in a duty cycle of 33% which
> @@ -646,7 +646,7 @@ static struct clk_regmap meson8b_cpu_clk = {
>  	},
>  	.hw.init = &(struct clk_init_data){
>  		.name = "cpu_clk",
> -		.ops = &clk_regmap_mux_ro_ops,
> +		.ops = &clk_regmap_mux_ops,
>  		.parent_names = (const char *[]){ "xtal",
>  						  "cpu_scale_out_sel" },
>  		.num_parents = 2,

Reviewed-by: Jerome Brunet <jbrunet@baylibre.com>

Re: [RFC v1 0/7] Meson8b: make the CPU clock mutable

[Neil Armstrong]

Hi Martin,

On 14/11/2018 23:57, Martin Blumenstingl wrote:
> This allows changing the CPU clock on the 32-bit Amlogic Meson SoCs
> (Meson8, Meson8b and Meson8m2).
> CPU frequency scaling will be enabled with a separate series by adding
> the CPU clock and the OPP tables to meson8.dtsi and meson8b.dtsi.
> 
> While changing the CPU frequency (sys_pll or any of it's post-dividers)
> we need to run the CPU clock off the XTAL clock. Otherwise the system
> will lock up because we need to disable the sys_pll to change it's
> rate.
> 
> This also makes the clk-pll's .enable hook a no-op if the clock is
> already enabled. Otherwise we're getting lockups when calling the
> first clk_{prepare_}enable on the sys_pll or any of it's children (as
> the CCF propagates the enable event up to the sys_pll). This is because
> the .enable hook unconditionally disables and enables the clock.
> However, we can't disable that clock (not even temporarily) if the CPU
> is running off sys_pll.
> 
> Additionally this adds support for more M/N combinations in sys_pll to
> achieve all of the OPPs on Meson8b and all OPPs <= 1608 MHz on Meson8
> and Meson8m2.
> 
> Compared to Amlogic's 3.10 kernel there's one notable difference: we
> are actually allowing changes to the sys_pll. Amlogic's kernel sets
> sys_pll to a fixed rate during boot and then uses a timer generate a
> "virtual clock rate" by toggling between various dividers (for example:
> sys_pll is set to 1536MHz. to achieve 1008MHz they are toggling every
> 2500us between 1536MHZ and 768MHz so the average over <period, for
> example one second> is 1008MHz).
> I could reproduce any situation where changing sys_pll failed (for
> example due to high temperature). To prove that I ran "stress --cpu 4"
> for multiple hours and then cycled through all available CPU
> frequencies (while keeping "stress" running in the background). This
> worked fine on my Meson8b Odroid-C1 and EC-100 boards as well as my
> Meson8m2 board.
> 
> 
> Martin Blumenstingl (7):
>   clk: meson: meson8b: run from the XTAL when changing the CPU frequency
>   clk: meson: meson8b: do not use cpu_div3 for cpu_scale_out_sel
>   clk: meson: clk-pll: check if the clock is already enabled
>   clk: meson: clk-pll: add the is_enabled function in the clk_ops
>   clk: meson: meson8b: mark the CPU clock as CLK_IS_CRITICAL
>   clk: meson: meson8b: add support for more M/N values in sys_pll
>   clk: meson: meson8b: allow changing the CPU clock tree
> 
>  drivers/clk/meson/clk-pll.c | 23 +++++++++
>  drivers/clk/meson/meson8b.c | 94 +++++++++++++++++++++++++++++++++----
>  2 files changed, 108 insertions(+), 9 deletions(-)
> 

I suppose it depends on the fixes you sent earlier ?

Anyway, resend it without RFC and by squashing patch 4 in 3 and
address the small comment issue and I'll take it for 4.21 !

Thanks,
Neil