[PATCH RFC v1 0/3] clk: meson: rounding for fast clocks on 32-bit SoCs

[PATCH RFC v1 0/3] clk: meson: rounding for fast clocks on 32-bit SoCs

[Martin Blumenstingl]

On the 32-bit Amlogic Meson8/8b/8m2 SoCs we run into a problem with the
fast HDMI PLL and it's OD (post-dividers). This clock tree can run at
up to approx. 3GHz.
This however causes a problem, because these rates require BIT(31) to
be usable. Unfortunately this is not the case with clk_ops.round_rate
on 32-bit systems. BIT(31) is reserved for the sign (+ or -).

clk_ops.determine_rate does not suffer from this limitation. It uses
an int to signal any errors and can then take all availble 32 bits for
the clock rate.

I am sending this as RFC to start a discussion whether:
- this is a good way to solve it?
- what are the alternatives?
- getting some feedback on areas that need to be improved


As always: any feedback is welcome!


Thank you!
Martin


Martin Blumenstingl (3):
  clk: divider: Add re-usable determine_rate implementations
  clk: meson: regmap: switch to determine_rate for the dividers
  clk: meson: pll: switch to determine_rate for the PLL ops

 drivers/clk/clk-divider.c      | 39 +++++++++++++++++++++++++++++++++-
 drivers/clk/meson/clk-pll.c    | 26 +++++++++++++----------
 drivers/clk/meson/clk-regmap.c | 19 ++++++++---------
 include/linux/clk-provider.h   |  6 ++++++
 4 files changed, 68 insertions(+), 22 deletions(-)

-- 
2.31.1


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

[PATCH RFC v1 1/3] clk: divider: Add re-usable determine_rate implementations

[Martin Blumenstingl]

These are useful when running on 32-bit systems to increase the upper
supported frequency limit. clk_ops.round_rate returns a signed long
which limits the maximum rate on 32-bit systems to 2^31 (or approx.
2.14GHz). clk_ops.determine_rate internally uses an unsigned long so
the maximum rate on 32-bit systems is 2^32 or approx. 4.29GHz.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/clk/clk-divider.c    | 39 +++++++++++++++++++++++++++++++++++-
 include/linux/clk-provider.h |  6 ++++++
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index 344997203f0e..24c94d2a3643 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -377,7 +377,6 @@ long divider_ro_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
 }
 EXPORT_SYMBOL_GPL(divider_ro_round_rate_parent);
 
-
 static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
 				unsigned long *prate)
 {
@@ -399,6 +398,44 @@ static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
 				  divider->width, divider->flags);
 }
 
+int divider_determine_rate(struct clk_hw *hw, struct clk_rate_request *req,
+			   const struct clk_div_table *table, u8 width,
+			   unsigned long flags)
+{
+	int div;
+
+	div = clk_divider_bestdiv(hw, req->best_parent_hw, req->rate,
+				  &req->best_parent_rate, table, width, flags);
+
+	req->rate = DIV_ROUND_UP_ULL((u64)req->best_parent_rate, div);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(divider_determine_rate);
+
+int divider_ro_determine_rate(struct clk_hw *hw, struct clk_rate_request *req,
+			      const struct clk_div_table *table, u8 width,
+			      unsigned long flags, unsigned int val)
+{
+	int div;
+
+	div = _get_div(table, val, flags, width);
+
+	/* Even a read-only clock can propagate a rate change */
+	if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
+		if (!req->best_parent_hw)
+			return -EINVAL;
+
+		req->best_parent_rate = clk_hw_round_rate(req->best_parent_hw,
+							  req->rate * div);
+	}
+
+	req->rate = DIV_ROUND_UP_ULL((u64)req->best_parent_rate, div);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(divider_ro_determine_rate);
+
 int divider_get_val(unsigned long rate, unsigned long parent_rate,
 		    const struct clk_div_table *table, u8 width,
 		    unsigned long flags)
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 162a2e5546a3..d83b829305c0 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -629,6 +629,12 @@ long divider_ro_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
 				  unsigned long rate, unsigned long *prate,
 				  const struct clk_div_table *table, u8 width,
 				  unsigned long flags, unsigned int val);
+int divider_determine_rate(struct clk_hw *hw, struct clk_rate_request *req,
+			   const struct clk_div_table *table, u8 width,
+			   unsigned long flags);
+int divider_ro_determine_rate(struct clk_hw *hw, struct clk_rate_request *req,
+			      const struct clk_div_table *table, u8 width,
+			      unsigned long flags, unsigned int val);
 int divider_get_val(unsigned long rate, unsigned long parent_rate,
 		const struct clk_div_table *table, u8 width,
 		unsigned long flags);
-- 
2.31.1


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

[PATCH RFC v1 2/3] clk: meson: regmap: switch to determine_rate for the dividers

[Martin Blumenstingl]

This increases the maxmium supported frequency on 32-bit systems from
2^31 (signed long as used by clk_ops.round_rate, maximum value:
approx. 2.14GHz) to 2^32 (unsigned long as used by
clk_ops.determine_rate, maximum value: approx. 4.29GHz).
On Meson8/8b/8m2 the HDMI PLL and it's OD (post-dividers) are
capable of running at up to 2.97GHz. So switch the divider
implementation in clk-regmap to clk_ops.determine_rate to support these
higher frequencies on 32-bit systems.

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

diff --git a/drivers/clk/meson/clk-regmap.c b/drivers/clk/meson/clk-regmap.c
index dcd1757cc5df..8ad8977cf1c2 100644
--- a/drivers/clk/meson/clk-regmap.c
+++ b/drivers/clk/meson/clk-regmap.c
@@ -75,8 +75,8 @@ static unsigned long clk_regmap_div_recalc_rate(struct clk_hw *hw,
 				   div->width);
 }
 
-static long clk_regmap_div_round_rate(struct clk_hw *hw, unsigned long rate,
-				      unsigned long *prate)
+static int clk_regmap_div_determine_rate(struct clk_hw *hw,
+					 struct clk_rate_request *req)
 {
 	struct clk_regmap *clk = to_clk_regmap(hw);
 	struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
@@ -87,18 +87,17 @@ static long clk_regmap_div_round_rate(struct clk_hw *hw, unsigned long rate,
 	if (div->flags & CLK_DIVIDER_READ_ONLY) {
 		ret = regmap_read(clk->map, div->offset, &val);
 		if (ret)
-			/* Gives a hint that something is wrong */
-			return 0;
+			return ret;
 
 		val >>= div->shift;
 		val &= clk_div_mask(div->width);
 
-		return divider_ro_round_rate(hw, rate, prate, div->table,
-					     div->width, div->flags, val);
+		return divider_ro_determine_rate(hw, req, div->table,
+						 div->width, div->flags, val);
 	}
 
-	return divider_round_rate(hw, rate, prate, div->table, div->width,
-				  div->flags);
+	return divider_determine_rate(hw, req, div->table, div->width,
+				      div->flags);
 }
 
 static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
@@ -123,14 +122,14 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
 
 const struct clk_ops clk_regmap_divider_ops = {
 	.recalc_rate = clk_regmap_div_recalc_rate,
-	.round_rate = clk_regmap_div_round_rate,
+	.determine_rate = clk_regmap_div_determine_rate,
 	.set_rate = clk_regmap_div_set_rate,
 };
 EXPORT_SYMBOL_GPL(clk_regmap_divider_ops);
 
 const struct clk_ops clk_regmap_divider_ro_ops = {
 	.recalc_rate = clk_regmap_div_recalc_rate,
-	.round_rate = clk_regmap_div_round_rate,
+	.determine_rate = clk_regmap_div_determine_rate,
 };
 EXPORT_SYMBOL_GPL(clk_regmap_divider_ro_ops);
 
-- 
2.31.1


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

[PATCH RFC v1 3/3] clk: meson: pll: switch to determine_rate for the PLL ops

[Martin Blumenstingl]

This increases the maxmium supported frequency on 32-bit systems from
2^31 (signed long as used by clk_ops.round_rate, maximum value:
approx. 2.14GHz) to 2^32 (unsigned long as used by
clk_ops.determine_rate, maximum value: approx. 4.29GHz).
On Meson8/8b/8m2 the HDMI PLL and it's OD (post-dividers) are
capable of running at up to 2.97GHz. So switch the divider
implementation in clk-regmap to clk_ops.determine_rate to support these
higher frequencies on 32-bit systems.

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

diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
index 49f27fe53213..9e55617bc3b4 100644
--- a/drivers/clk/meson/clk-pll.c
+++ b/drivers/clk/meson/clk-pll.c
@@ -242,8 +242,8 @@ static int meson_clk_get_pll_settings(unsigned long rate,
 	return best ? 0 : -EINVAL;
 }
 
-static long meson_clk_pll_round_rate(struct clk_hw *hw, unsigned long rate,
-				     unsigned long *parent_rate)
+static int meson_clk_pll_determine_rate(struct clk_hw *hw,
+					struct clk_rate_request *req)
 {
 	struct clk_regmap *clk = to_clk_regmap(hw);
 	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
@@ -251,22 +251,26 @@ static long meson_clk_pll_round_rate(struct clk_hw *hw, unsigned long rate,
 	unsigned long round;
 	int ret;
 
-	ret = meson_clk_get_pll_settings(rate, *parent_rate, &m, &n, pll);
+	ret = meson_clk_get_pll_settings(req->rate, req->best_parent_rate,
+					 &m, &n, pll);
 	if (ret)
-		return meson_clk_pll_recalc_rate(hw, *parent_rate);
+		return ret;
 
-	round = __pll_params_to_rate(*parent_rate, m, n, 0, pll);
+	round = __pll_params_to_rate(req->best_parent_rate, m, n, 0, pll);
 
-	if (!MESON_PARM_APPLICABLE(&pll->frac) || rate == round)
-		return round;
+	if (!MESON_PARM_APPLICABLE(&pll->frac) || req->rate == round) {
+		req->rate = round;
+		return 0;
+	}
 
 	/*
 	 * The rate provided by the setting is not an exact match, let's
 	 * try to improve the result using the fractional parameter
 	 */
-	frac = __pll_params_with_frac(rate, *parent_rate, m, n, pll);
+	frac = __pll_params_with_frac(req->rate, req->best_parent_rate, m, n, pll);
+	req->rate = __pll_params_to_rate(req->best_parent_rate, m, n, frac, pll);
 
-	return __pll_params_to_rate(*parent_rate, m, n, frac, pll);
+	return 0;
 }
 
 static int meson_clk_pll_wait_lock(struct clk_hw *hw)
@@ -419,7 +423,7 @@ static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
  */
 const struct clk_ops meson_clk_pcie_pll_ops = {
 	.recalc_rate	= meson_clk_pll_recalc_rate,
-	.round_rate	= meson_clk_pll_round_rate,
+	.determine_rate	= meson_clk_pll_determine_rate,
 	.is_enabled	= meson_clk_pll_is_enabled,
 	.enable		= meson_clk_pcie_pll_enable,
 	.disable	= meson_clk_pll_disable
@@ -429,7 +433,7 @@ EXPORT_SYMBOL_GPL(meson_clk_pcie_pll_ops);
 const struct clk_ops meson_clk_pll_ops = {
 	.init		= meson_clk_pll_init,
 	.recalc_rate	= meson_clk_pll_recalc_rate,
-	.round_rate	= meson_clk_pll_round_rate,
+	.determine_rate	= meson_clk_pll_determine_rate,
 	.set_rate	= meson_clk_pll_set_rate,
 	.is_enabled	= meson_clk_pll_is_enabled,
 	.enable		= meson_clk_pll_enable,
-- 
2.31.1


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

Re: [PATCH RFC v1 0/3] clk: meson: rounding for fast clocks on 32-bit SoCs

[Jerome Brunet]

On Mon 17 May 2021 at 22:37, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> On the 32-bit Amlogic Meson8/8b/8m2 SoCs we run into a problem with the
> fast HDMI PLL and it's OD (post-dividers). This clock tree can run at
> up to approx. 3GHz.
> This however causes a problem, because these rates require BIT(31) to
> be usable. Unfortunately this is not the case with clk_ops.round_rate
> on 32-bit systems. BIT(31) is reserved for the sign (+ or -).
>
> clk_ops.determine_rate does not suffer from this limitation. It uses
> an int to signal any errors and can then take all availble 32 bits for
> the clock rate.
>
> I am sending this as RFC to start a discussion whether:
> - this is a good way to solve it?

.determine_rate() was meant to replace .round_rate() so I guess it is
good to do it :)

> - what are the alternatives?

I don't see any ATM. Even with determine_rate(), 4.29GHz limitation
seems a bit low nowadays. In AML SoC, most PLLs should be able to reach
6GHz ... hopefully we won't need that on the 32bits variant ;)

> - getting some feedback on areas that need to be improved
>
>
> As always: any feedback is welcome!
>

Overall, looks good to me.

>
> Thank you!
> Martin
>
>
> Martin Blumenstingl (3):
>   clk: divider: Add re-usable determine_rate implementations
>   clk: meson: regmap: switch to determine_rate for the dividers
>   clk: meson: pll: switch to determine_rate for the PLL ops
>
>  drivers/clk/clk-divider.c      | 39 +++++++++++++++++++++++++++++++++-
>  drivers/clk/meson/clk-pll.c    | 26 +++++++++++++----------
>  drivers/clk/meson/clk-regmap.c | 19 ++++++++---------
>  include/linux/clk-provider.h   |  6 ++++++
>  4 files changed, 68 insertions(+), 22 deletions(-)


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

Re: [PATCH RFC v1 1/3] clk: divider: Add re-usable determine_rate implementations

[Jerome Brunet]

On Mon 17 May 2021 at 22:37, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> These are useful when running on 32-bit systems to increase the upper
> supported frequency limit. clk_ops.round_rate returns a signed long
> which limits the maximum rate on 32-bit systems to 2^31 (or approx.
> 2.14GHz). clk_ops.determine_rate internally uses an unsigned long so
> the maximum rate on 32-bit systems is 2^32 or approx. 4.29GHz.
>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/clk/clk-divider.c    | 39 +++++++++++++++++++++++++++++++++++-
>  include/linux/clk-provider.h |  6 ++++++
>  2 files changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> index 344997203f0e..24c94d2a3643 100644
> --- a/drivers/clk/clk-divider.c
> +++ b/drivers/clk/clk-divider.c
> @@ -377,7 +377,6 @@ long divider_ro_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
>  }
>  EXPORT_SYMBOL_GPL(divider_ro_round_rate_parent);
>  
> -
>  static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
>  				unsigned long *prate)
>  {
> @@ -399,6 +398,44 @@ static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
>  				  divider->width, divider->flags);
>  }
>  
> +int divider_determine_rate(struct clk_hw *hw, struct clk_rate_request *req,
> +			   const struct clk_div_table *table, u8 width,
> +			   unsigned long flags)
> +{
> +	int div;
> +
> +	div = clk_divider_bestdiv(hw, req->best_parent_hw, req->rate,
> +				  &req->best_parent_rate, table, width, flags);
> +
> +	req->rate = DIV_ROUND_UP_ULL((u64)req->best_parent_rate, div);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(divider_determine_rate);
> +
> +int divider_ro_determine_rate(struct clk_hw *hw, struct clk_rate_request *req,
> +			      const struct clk_div_table *table, u8 width,
> +			      unsigned long flags, unsigned int val)
> +{
> +	int div;
> +
> +	div = _get_div(table, val, flags, width);
> +
> +	/* Even a read-only clock can propagate a rate change */
> +	if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
> +		if (!req->best_parent_hw)
> +			return -EINVAL;
> +
> +		req->best_parent_rate = clk_hw_round_rate(req->best_parent_hw,
> +							  req->rate * div);
> +	}
> +
> +	req->rate = DIV_ROUND_UP_ULL((u64)req->best_parent_rate, div);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(divider_ro_determine_rate);

For a final version, could you factorize the code with the .round_rate()
variant ? It would remove a bit of duplication. 

Maybe determine_rate() can also replace round_rate() in the generic
divider ops ?

> +
>  int divider_get_val(unsigned long rate, unsigned long parent_rate,
>  		    const struct clk_div_table *table, u8 width,
>  		    unsigned long flags)
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 162a2e5546a3..d83b829305c0 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -629,6 +629,12 @@ long divider_ro_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
>  				  unsigned long rate, unsigned long *prate,
>  				  const struct clk_div_table *table, u8 width,
>  				  unsigned long flags, unsigned int val);
> +int divider_determine_rate(struct clk_hw *hw, struct clk_rate_request *req,
> +			   const struct clk_div_table *table, u8 width,
> +			   unsigned long flags);
> +int divider_ro_determine_rate(struct clk_hw *hw, struct clk_rate_request *req,
> +			      const struct clk_div_table *table, u8 width,
> +			      unsigned long flags, unsigned int val);
>  int divider_get_val(unsigned long rate, unsigned long parent_rate,
>  		const struct clk_div_table *table, u8 width,
>  		unsigned long flags);


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

Re: [PATCH RFC v1 2/3] clk: meson: regmap: switch to determine_rate for the dividers

[Jerome Brunet]

On Mon 17 May 2021 at 22:37, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> This increases the maxmium supported frequency on 32-bit systems from
> 2^31 (signed long as used by clk_ops.round_rate, maximum value:
> approx. 2.14GHz) to 2^32 (unsigned long as used by
> clk_ops.determine_rate, maximum value: approx. 4.29GHz).
> On Meson8/8b/8m2 the HDMI PLL and it's OD (post-dividers) are
> capable of running at up to 2.97GHz. So switch the divider
> implementation in clk-regmap to clk_ops.determine_rate to support these
> higher frequencies on 32-bit systems.
>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

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

> ---
>  drivers/clk/meson/clk-regmap.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/clk/meson/clk-regmap.c b/drivers/clk/meson/clk-regmap.c
> index dcd1757cc5df..8ad8977cf1c2 100644
> --- a/drivers/clk/meson/clk-regmap.c
> +++ b/drivers/clk/meson/clk-regmap.c
> @@ -75,8 +75,8 @@ static unsigned long clk_regmap_div_recalc_rate(struct clk_hw *hw,
>  				   div->width);
>  }
>  
> -static long clk_regmap_div_round_rate(struct clk_hw *hw, unsigned long rate,
> -				      unsigned long *prate)
> +static int clk_regmap_div_determine_rate(struct clk_hw *hw,
> +					 struct clk_rate_request *req)
>  {
>  	struct clk_regmap *clk = to_clk_regmap(hw);
>  	struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
> @@ -87,18 +87,17 @@ static long clk_regmap_div_round_rate(struct clk_hw *hw, unsigned long rate,
>  	if (div->flags & CLK_DIVIDER_READ_ONLY) {
>  		ret = regmap_read(clk->map, div->offset, &val);
>  		if (ret)
> -			/* Gives a hint that something is wrong */
> -			return 0;
> +			return ret;
>  
>  		val >>= div->shift;
>  		val &= clk_div_mask(div->width);
>  
> -		return divider_ro_round_rate(hw, rate, prate, div->table,
> -					     div->width, div->flags, val);
> +		return divider_ro_determine_rate(hw, req, div->table,
> +						 div->width, div->flags, val);
>  	}
>  
> -	return divider_round_rate(hw, rate, prate, div->table, div->width,
> -				  div->flags);
> +	return divider_determine_rate(hw, req, div->table, div->width,
> +				      div->flags);
>  }
>  
>  static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
> @@ -123,14 +122,14 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate,
>  
>  const struct clk_ops clk_regmap_divider_ops = {
>  	.recalc_rate = clk_regmap_div_recalc_rate,
> -	.round_rate = clk_regmap_div_round_rate,
> +	.determine_rate = clk_regmap_div_determine_rate,
>  	.set_rate = clk_regmap_div_set_rate,
>  };
>  EXPORT_SYMBOL_GPL(clk_regmap_divider_ops);
>  
>  const struct clk_ops clk_regmap_divider_ro_ops = {
>  	.recalc_rate = clk_regmap_div_recalc_rate,
> -	.round_rate = clk_regmap_div_round_rate,
> +	.determine_rate = clk_regmap_div_determine_rate,
>  };
>  EXPORT_SYMBOL_GPL(clk_regmap_divider_ro_ops);

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

Re: [PATCH RFC v1 3/3] clk: meson: pll: switch to determine_rate for the PLL ops

[Jerome Brunet]

On Mon 17 May 2021 at 22:37, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> This increases the maxmium supported frequency on 32-bit systems from
> 2^31 (signed long as used by clk_ops.round_rate, maximum value:
> approx. 2.14GHz) to 2^32 (unsigned long as used by
> clk_ops.determine_rate, maximum value: approx. 4.29GHz).
> On Meson8/8b/8m2 the HDMI PLL and it's OD (post-dividers) are
> capable of running at up to 2.97GHz. So switch the divider
> implementation in clk-regmap to clk_ops.determine_rate to support these
> higher frequencies on 32-bit systems.
>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Looks good. I see no reason to keep this one as RFC.
I can take it directly if this is OK with you ?

> ---
>  drivers/clk/meson/clk-pll.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
> index 49f27fe53213..9e55617bc3b4 100644
> --- a/drivers/clk/meson/clk-pll.c
> +++ b/drivers/clk/meson/clk-pll.c
> @@ -242,8 +242,8 @@ static int meson_clk_get_pll_settings(unsigned long rate,
>  	return best ? 0 : -EINVAL;
>  }
>  
> -static long meson_clk_pll_round_rate(struct clk_hw *hw, unsigned long rate,
> -				     unsigned long *parent_rate)
> +static int meson_clk_pll_determine_rate(struct clk_hw *hw,
> +					struct clk_rate_request *req)
>  {
>  	struct clk_regmap *clk = to_clk_regmap(hw);
>  	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
> @@ -251,22 +251,26 @@ static long meson_clk_pll_round_rate(struct clk_hw *hw, unsigned long rate,
>  	unsigned long round;
>  	int ret;
>  
> -	ret = meson_clk_get_pll_settings(rate, *parent_rate, &m, &n, pll);
> +	ret = meson_clk_get_pll_settings(req->rate, req->best_parent_rate,
> +					 &m, &n, pll);
>  	if (ret)
> -		return meson_clk_pll_recalc_rate(hw, *parent_rate);
> +		return ret;
>  
> -	round = __pll_params_to_rate(*parent_rate, m, n, 0, pll);
> +	round = __pll_params_to_rate(req->best_parent_rate, m, n, 0, pll);
>  
> -	if (!MESON_PARM_APPLICABLE(&pll->frac) || rate == round)
> -		return round;
> +	if (!MESON_PARM_APPLICABLE(&pll->frac) || req->rate == round) {
> +		req->rate = round;
> +		return 0;
> +	}
>  
>  	/*
>  	 * The rate provided by the setting is not an exact match, let's
>  	 * try to improve the result using the fractional parameter
>  	 */
> -	frac = __pll_params_with_frac(rate, *parent_rate, m, n, pll);
> +	frac = __pll_params_with_frac(req->rate, req->best_parent_rate, m, n, pll);
> +	req->rate = __pll_params_to_rate(req->best_parent_rate, m, n, frac, pll);
>  
> -	return __pll_params_to_rate(*parent_rate, m, n, frac, pll);
> +	return 0;
>  }
>  
>  static int meson_clk_pll_wait_lock(struct clk_hw *hw)
> @@ -419,7 +423,7 @@ static int meson_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
>   */
>  const struct clk_ops meson_clk_pcie_pll_ops = {
>  	.recalc_rate	= meson_clk_pll_recalc_rate,
> -	.round_rate	= meson_clk_pll_round_rate,
> +	.determine_rate	= meson_clk_pll_determine_rate,
>  	.is_enabled	= meson_clk_pll_is_enabled,
>  	.enable		= meson_clk_pcie_pll_enable,
>  	.disable	= meson_clk_pll_disable
> @@ -429,7 +433,7 @@ EXPORT_SYMBOL_GPL(meson_clk_pcie_pll_ops);
>  const struct clk_ops meson_clk_pll_ops = {
>  	.init		= meson_clk_pll_init,
>  	.recalc_rate	= meson_clk_pll_recalc_rate,
> -	.round_rate	= meson_clk_pll_round_rate,
> +	.determine_rate	= meson_clk_pll_determine_rate,
>  	.set_rate	= meson_clk_pll_set_rate,
>  	.is_enabled	= meson_clk_pll_is_enabled,
>  	.enable		= meson_clk_pll_enable,


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

Re: [PATCH RFC v1 3/3] clk: meson: pll: switch to determine_rate for the PLL ops

[Martin Blumenstingl]

Hi Jerome,

On Tue, May 18, 2021 at 9:50 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
>
>
> On Mon 17 May 2021 at 22:37, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:
>
> > This increases the maxmium supported frequency on 32-bit systems from
> > 2^31 (signed long as used by clk_ops.round_rate, maximum value:
> > approx. 2.14GHz) to 2^32 (unsigned long as used by
> > clk_ops.determine_rate, maximum value: approx. 4.29GHz).
> > On Meson8/8b/8m2 the HDMI PLL and it's OD (post-dividers) are
> > capable of running at up to 2.97GHz. So switch the divider
> > implementation in clk-regmap to clk_ops.determine_rate to support these
> > higher frequencies on 32-bit systems.
> >
> > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>
> Looks good. I see no reason to keep this one as RFC.
Great, thanks for checking!

> I can take it directly if this is OK with you ?
That would be amazing.
Obviously no objections from my side :-)


Best regards,
Martin

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

Re: [PATCH RFC v1 0/3] clk: meson: rounding for fast clocks on 32-bit SoCs

[Martin Blumenstingl]

Hi Jerome,

On Tue, May 18, 2021 at 9:37 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
>
>
> On Mon 17 May 2021 at 22:37, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:
>
> > On the 32-bit Amlogic Meson8/8b/8m2 SoCs we run into a problem with the
> > fast HDMI PLL and it's OD (post-dividers). This clock tree can run at
> > up to approx. 3GHz.
> > This however causes a problem, because these rates require BIT(31) to
> > be usable. Unfortunately this is not the case with clk_ops.round_rate
> > on 32-bit systems. BIT(31) is reserved for the sign (+ or -).
> >
> > clk_ops.determine_rate does not suffer from this limitation. It uses
> > an int to signal any errors and can then take all availble 32 bits for
> > the clock rate.
> >
> > I am sending this as RFC to start a discussion whether:
> > - this is a good way to solve it?
>
> .determine_rate() was meant to replace .round_rate() so I guess it is
> good to do it :)
ah, now things make more sense.
thanks for the background info

> > - what are the alternatives?
>
> I don't see any ATM. Even with determine_rate(), 4.29GHz limitation
> seems a bit low nowadays. In AML SoC, most PLLs should be able to reach
> 6GHz ... hopefully we won't need that on the 32bits variant ;)
according to the public datasheet the maximum PLL frequency is at around 3GHz
so I also hope that we're safe with this


Martin

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

Re: [PATCH RFC v1 1/3] clk: divider: Add re-usable determine_rate implementations

[Martin Blumenstingl]

Hi Jerome,

On Tue, May 18, 2021 at 9:44 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
[...]
> > +int divider_ro_determine_rate(struct clk_hw *hw, struct clk_rate_request *req,
> > +                           const struct clk_div_table *table, u8 width,
> > +                           unsigned long flags, unsigned int val)
> > +{
> > +     int div;
> > +
> > +     div = _get_div(table, val, flags, width);
> > +
> > +     /* Even a read-only clock can propagate a rate change */
> > +     if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
> > +             if (!req->best_parent_hw)
> > +                     return -EINVAL;
> > +
> > +             req->best_parent_rate = clk_hw_round_rate(req->best_parent_hw,
> > +                                                       req->rate * div);
> > +     }
> > +
> > +     req->rate = DIV_ROUND_UP_ULL((u64)req->best_parent_rate, div);
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(divider_ro_determine_rate);
>
> For a final version, could you factorize the code with the .round_rate()
> variant ? It would remove a bit of duplication.
my first idea was to basically let the new _determine_rate code just
forward all relevant parameters to _round_rate
however, I discarded that as it turned out to be less understandable
for me as parameters need to be mapped in both ways

while writing this mail I noticed that the opposite direction
(meaning: _round_rate forwards to _determine_rate) will probably work.
I'll give it a try in the next days
if you had anything else in mind then please let me know

> Maybe determine_rate() can also replace round_rate() in the generic
> divider ops ?
sure, I'll add that as a separate patch in this series
note to myself: testing can be done with the MMC drivers as we're
using the generic clk_divider_ops there


Best regards,
Martin

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

Re: [PATCH RFC v1 1/3] clk: divider: Add re-usable determine_rate implementations

[Jerome Brunet]

On Tue 18 May 2021 at 22:33, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> Hi Jerome,
>
> On Tue, May 18, 2021 at 9:44 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
> [...]
>> > +int divider_ro_determine_rate(struct clk_hw *hw, struct clk_rate_request *req,
>> > +                           const struct clk_div_table *table, u8 width,
>> > +                           unsigned long flags, unsigned int val)
>> > +{
>> > +     int div;
>> > +
>> > +     div = _get_div(table, val, flags, width);
>> > +
>> > +     /* Even a read-only clock can propagate a rate change */
>> > +     if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
>> > +             if (!req->best_parent_hw)
>> > +                     return -EINVAL;
>> > +
>> > +             req->best_parent_rate = clk_hw_round_rate(req->best_parent_hw,
>> > +                                                       req->rate * div);
>> > +     }
>> > +
>> > +     req->rate = DIV_ROUND_UP_ULL((u64)req->best_parent_rate, div);
>> > +
>> > +     return 0;
>> > +}
>> > +EXPORT_SYMBOL_GPL(divider_ro_determine_rate);
>>
>> For a final version, could you factorize the code with the .round_rate()
>> variant ? It would remove a bit of duplication.
> my first idea was to basically let the new _determine_rate code just
> forward all relevant parameters to _round_rate
> however, I discarded that as it turned out to be less understandable
> for me as parameters need to be mapped in both ways
>
> while writing this mail I noticed that the opposite direction
> (meaning: _round_rate forwards to _determine_rate) will probably work.
> I'll give it a try in the next days
> if you had anything else in mind then please let me know

Yep, the idea would be to use the determine_rate() part as the common
implementation. AFAICT, all you need is to build req_rate structure in
the round_rate() part.

>
>> Maybe determine_rate() can also replace round_rate() in the generic
>> divider ops ?
> sure, I'll add that as a separate patch in this series
> note to myself: testing can be done with the MMC drivers as we're
> using the generic clk_divider_ops there
>
>
> Best regards,
> Martin


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

Re: [PATCH RFC v1 3/3] clk: meson: pll: switch to determine_rate for the PLL ops

[Jerome Brunet]

On Tue 18 May 2021 at 22:17, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> Hi Jerome,
>
> On Tue, May 18, 2021 at 9:50 AM Jerome Brunet <jbrunet@baylibre.com> wrote:
>>
>>
>> On Mon 17 May 2021 at 22:37, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:
>>
>> > This increases the maxmium supported frequency on 32-bit systems from
>> > 2^31 (signed long as used by clk_ops.round_rate, maximum value:
>> > approx. 2.14GHz) to 2^32 (unsigned long as used by
>> > clk_ops.determine_rate, maximum value: approx. 4.29GHz).
>> > On Meson8/8b/8m2 the HDMI PLL and it's OD (post-dividers) are
>> > capable of running at up to 2.97GHz. So switch the divider
>> > implementation in clk-regmap to clk_ops.determine_rate to support these
>> > higher frequencies on 32-bit systems.
>> >
>> > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>>
>> Looks good. I see no reason to keep this one as RFC.
> Great, thanks for checking!
>
>> I can take it directly if this is OK with you ?
> That would be amazing.
> Obviously no objections from my side :-)
>
>
> Best regards,
> Martin

Applied then. Thx

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