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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Mon 17 May 2021 at 22:37, Martin Blumenstingl <[email protected]> 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(-)
On Mon 17 May 2021 at 22:37, Martin Blumenstingl <[email protected]> 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 <[email protected]>
> ---
> 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);
On Mon 17 May 2021 at 22:37, Martin Blumenstingl <[email protected]> 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 <[email protected]>
Reviewed-by: Jerome Brunet <[email protected]>
> ---
> 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);
On Mon 17 May 2021 at 22:37, Martin Blumenstingl <[email protected]> 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 <[email protected]>
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,
Hi Jerome,
On Tue, May 18, 2021 at 9:37 AM Jerome Brunet <[email protected]> wrote:
>
>
> On Mon 17 May 2021 at 22:37, Martin Blumenstingl <[email protected]> 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
Hi Jerome,
On Tue, May 18, 2021 at 9:44 AM Jerome Brunet <[email protected]> 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
On Tue 18 May 2021 at 22:33, Martin Blumenstingl <[email protected]> wrote:
> Hi Jerome,
>
> On Tue, May 18, 2021 at 9:44 AM Jerome Brunet <[email protected]> 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
On Tue 18 May 2021 at 22:17, Martin Blumenstingl <[email protected]> wrote:
> Hi Jerome,
>
> On Tue, May 18, 2021 at 9:50 AM Jerome Brunet <[email protected]> wrote:
>>
>>
>> On Mon 17 May 2021 at 22:37, Martin Blumenstingl <[email protected]> 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 <[email protected]>
>>
>> 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
Hi Jerome,
On Tue, May 18, 2021 at 9:50 AM Jerome Brunet <[email protected]> wrote:
>
>
> On Mon 17 May 2021 at 22:37, Martin Blumenstingl <[email protected]> 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 <[email protected]>
>
> 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