From: Brian Masney <bmasney@redhat.com>

This driver implements both the determine_rate() and round_rate() clk
ops, and the round_rate() clk ops is deprecated. When both are defined,
clk_core_determine_round_nolock() from the clk core will only use the
determine_rate() clk ops, so let's remove the round_rate() clk ops since
it's unused.

Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/clk-scmi.c | 30 ------------------------------
 1 file changed, 30 deletions(-)

diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
index d2408403283fc72f0cf902e65f4c08bcbc7b4b0b..6c6ddb92e7cf6a0cfac2c7e19c0f15f777bb8c51 100644
--- a/drivers/clk/clk-scmi.c
+++ b/drivers/clk/clk-scmi.c
@@ -54,35 +54,6 @@ static unsigned long scmi_clk_recalc_rate(struct clk_hw *hw,
 	return rate;
 }
 
-static long scmi_clk_round_rate(struct clk_hw *hw, unsigned long rate,
-				unsigned long *parent_rate)
-{
-	u64 fmin, fmax, ftmp;
-	struct scmi_clk *clk = to_scmi_clk(hw);
-
-	/*
-	 * We can't figure out what rate it will be, so just return the
-	 * rate back to the caller. scmi_clk_recalc_rate() will be called
-	 * after the rate is set and we'll know what rate the clock is
-	 * running at then.
-	 */
-	if (clk->info->rate_discrete)
-		return rate;
-
-	fmin = clk->info->range.min_rate;
-	fmax = clk->info->range.max_rate;
-	if (rate <= fmin)
-		return fmin;
-	else if (rate >= fmax)
-		return fmax;
-
-	ftmp = rate - fmin;
-	ftmp += clk->info->range.step_size - 1; /* to round up */
-	do_div(ftmp, clk->info->range.step_size);
-
-	return ftmp * clk->info->range.step_size + fmin;
-}
-
 static int scmi_clk_set_rate(struct clk_hw *hw, unsigned long rate,
 			     unsigned long parent_rate)
 {
@@ -300,7 +271,6 @@ scmi_clk_ops_alloc(struct device *dev, unsigned long feats_key)
 
 	/* Rate ops */
 	ops->recalc_rate = scmi_clk_recalc_rate;
-	ops->round_rate = scmi_clk_round_rate;
 	ops->determine_rate = scmi_clk_determine_rate;
 	if (feats_key & BIT(SCMI_CLK_RATE_CTRL_SUPPORTED))
 		ops->set_rate = scmi_clk_set_rate;

-- 
2.50.1

Hi Brian, Sudeep, Cristian

On Mon, Aug 11, 2025 at 11:19:44AM -0400, Brian Masney via B4 Relay wrote:
>From: Brian Masney <bmasney@redhat.com>
>
>This driver implements both the determine_rate() and round_rate() clk
>ops, and the round_rate() clk ops is deprecated. When both are defined,
>clk_core_determine_round_nolock() from the clk core will only use the
>determine_rate() clk ops, so let's remove the round_rate() clk ops since
>it's unused.
>
>Signed-off-by: Brian Masney <bmasney@redhat.com>
>---
> drivers/clk/clk-scmi.c | 30 ------------------------------
> 1 file changed, 30 deletions(-)
>
>diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
>index d2408403283fc72f0cf902e65f4c08bcbc7b4b0b..6c6ddb92e7cf6a0cfac2c7e19c0f15f777bb8c51 100644
>--- a/drivers/clk/clk-scmi.c
>+++ b/drivers/clk/clk-scmi.c
>@@ -54,35 +54,6 @@ static unsigned long scmi_clk_recalc_rate(struct clk_hw *hw,
> 	return rate;
> }
> 
>-static long scmi_clk_round_rate(struct clk_hw *hw, unsigned long rate,
>-				unsigned long *parent_rate)
>-{

I see the point of round_rate is not used if determine_rate is there.
But reading the code of round_rate, It might be better to rename
scmi_clk_round_rate to scmi_clk_determine_rate.

Anyway, need Sudeep and Cristian to comment.

Thanks,
Peng

On Wed, Aug 27, 2025 at 03:09:33PM +0800, Peng Fan wrote:
> Hi Brian, Sudeep, Cristian
> 
> On Mon, Aug 11, 2025 at 11:19:44AM -0400, Brian Masney via B4 Relay wrote:
> >From: Brian Masney <bmasney@redhat.com>
> >
> >This driver implements both the determine_rate() and round_rate() clk
> >ops, and the round_rate() clk ops is deprecated. When both are defined,
> >clk_core_determine_round_nolock() from the clk core will only use the
> >determine_rate() clk ops, so let's remove the round_rate() clk ops since
> >it's unused.
> >
> >Signed-off-by: Brian Masney <bmasney@redhat.com>
> >---
> > drivers/clk/clk-scmi.c | 30 ------------------------------
> > 1 file changed, 30 deletions(-)
> >
> >diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> >index d2408403283fc72f0cf902e65f4c08bcbc7b4b0b..6c6ddb92e7cf6a0cfac2c7e19c0f15f777bb8c51 100644
> >--- a/drivers/clk/clk-scmi.c
> >+++ b/drivers/clk/clk-scmi.c
> >@@ -54,35 +54,6 @@ static unsigned long scmi_clk_recalc_rate(struct clk_hw *hw,
> > 	return rate;
> > }
> > 
> >-static long scmi_clk_round_rate(struct clk_hw *hw, unsigned long rate,
> >-				unsigned long *parent_rate)
> >-{
> 
> I see the point of round_rate is not used if determine_rate is there.
> But reading the code of round_rate, It might be better to rename
> scmi_clk_round_rate to scmi_clk_determine_rate.
> 
> Anyway, need Sudeep and Cristian to comment.

In this case, yes the round_rate implementation is filled out, whereas
the determine_rate lets the firmware handle it, and
scmi_clk_recalc_rate() will later populate the rate the clock is running
at.

I can convert round_rate over to determine_rate in this case, however it
would be a change to what's there now, and risks a regression. Here's
the relevant code from drivers/clk/clk.c where the determine_rate and
round_rate ops are called:

    static int clk_core_determine_round_nolock(struct clk_core *core,
                                               struct clk_rate_request *req)
    {
    	...
            if (clk_core_rate_is_protected(core)) {
                    req->rate = core->rate;
            } else if (core->ops->determine_rate) {
                    return core->ops->determine_rate(core->hw, req);
            } else if (core->ops->round_rate) {
                    rate = core->ops->round_rate(core->hw, req->rate,
                                                 &req->best_parent_rate);
    	...

If Sudeep / Cristian want the round rate converted to determine rate in
this driver, then I can do that in a v2.

Brian

On Wed, Aug 27, 2025 at 09:13:17AM -0400, Brian Masney wrote:
> On Wed, Aug 27, 2025 at 03:09:33PM +0800, Peng Fan wrote:
> > Hi Brian, Sudeep, Cristian
> > 
> > On Mon, Aug 11, 2025 at 11:19:44AM -0400, Brian Masney via B4 Relay wrote:
> > >From: Brian Masney <bmasney@redhat.com>
> > >
> > >This driver implements both the determine_rate() and round_rate() clk
> > >ops, and the round_rate() clk ops is deprecated. When both are defined,
> > >clk_core_determine_round_nolock() from the clk core will only use the
> > >determine_rate() clk ops, so let's remove the round_rate() clk ops since
> > >it's unused.
> > >
> > >Signed-off-by: Brian Masney <bmasney@redhat.com>
> > >---
> > > drivers/clk/clk-scmi.c | 30 ------------------------------
> > > 1 file changed, 30 deletions(-)
> > >
> > >diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> > >index d2408403283fc72f0cf902e65f4c08bcbc7b4b0b..6c6ddb92e7cf6a0cfac2c7e19c0f15f777bb8c51 100644
> > >--- a/drivers/clk/clk-scmi.c
> > >+++ b/drivers/clk/clk-scmi.c
> > >@@ -54,35 +54,6 @@ static unsigned long scmi_clk_recalc_rate(struct clk_hw *hw,
> > > 	return rate;
> > > }
> > > 
> > >-static long scmi_clk_round_rate(struct clk_hw *hw, unsigned long rate,
> > >-				unsigned long *parent_rate)
> > >-{
> > 
> > I see the point of round_rate is not used if determine_rate is there.
> > But reading the code of round_rate, It might be better to rename
> > scmi_clk_round_rate to scmi_clk_determine_rate.
> > 
> > Anyway, need Sudeep and Cristian to comment.
> 
> In this case, yes the round_rate implementation is filled out, whereas
> the determine_rate lets the firmware handle it, and
> scmi_clk_recalc_rate() will later populate the rate the clock is running
> at.
> 
> I can convert round_rate over to determine_rate in this case, however it
> would be a change to what's there now, and risks a regression. Here's
> the relevant code from drivers/clk/clk.c where the determine_rate and
> round_rate ops are called:

I am inclined towards this. Determine rate was added recently when the
clock parent support was added IIUC, so I don't think it should regress
anything.

> 
>     static int clk_core_determine_round_nolock(struct clk_core *core,
>                                                struct clk_rate_request *req)
>     {
>     	...
>             if (clk_core_rate_is_protected(core)) {
>                     req->rate = core->rate;
>             } else if (core->ops->determine_rate) {
>                     return core->ops->determine_rate(core->hw, req);
>             } else if (core->ops->round_rate) {
>                     rate = core->ops->round_rate(core->hw, req->rate,
>                                                  &req->best_parent_rate);
>     	...
> 
> If Sudeep / Cristian want the round rate converted to determine rate in
> this driver, then I can do that in a v2.
> 

Yes please. Also please post it independent if it doesn't have to be in
the series. To many in cc and lots of patches to respin all.

-- 
Regards,
Sudeep

On Mon, 2025-08-11 at 11:19 -0400, Brian Masney via B4 Relay wrote:
> From: Brian Masney <bmasney@redhat.com>
> 
> This driver implements both the determine_rate() and round_rate() clk
> ops, and the round_rate() clk ops is deprecated. When both are defined,
> clk_core_determine_round_nolock() from the clk core will only use the
> determine_rate() clk ops, so let's remove the round_rate() clk ops since
> it's unused.
> 
> Signed-off-by: Brian Masney <bmasney@redhat.com>

Reviewed-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>

> ---
>  drivers/clk/clk-scmi.c | 30 ------------------------------
>  1 file changed, 30 deletions(-)
> 
> diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> index d2408403283fc72f0cf902e65f4c08bcbc7b4b0b..6c6ddb92e7cf6a0cfac2c7e19c0f15f777bb8c51 100644
> --- a/drivers/clk/clk-scmi.c
> +++ b/drivers/clk/clk-scmi.c
> @@ -54,35 +54,6 @@ static unsigned long scmi_clk_recalc_rate(struct clk_hw *hw,
>  	return rate;
>  }
>  
> -static long scmi_clk_round_rate(struct clk_hw *hw, unsigned long rate,
> -				unsigned long *parent_rate)
> -{
> -	u64 fmin, fmax, ftmp;
> -	struct scmi_clk *clk = to_scmi_clk(hw);
> -
> -	/*
> -	 * We can't figure out what rate it will be, so just return the
> -	 * rate back to the caller. scmi_clk_recalc_rate() will be called
> -	 * after the rate is set and we'll know what rate the clock is
> -	 * running at then.
> -	 */
> -	if (clk->info->rate_discrete)
> -		return rate;
> -
> -	fmin = clk->info->range.min_rate;
> -	fmax = clk->info->range.max_rate;
> -	if (rate <= fmin)
> -		return fmin;
> -	else if (rate >= fmax)
> -		return fmax;
> -
> -	ftmp = rate - fmin;
> -	ftmp += clk->info->range.step_size - 1; /* to round up */
> -	do_div(ftmp, clk->info->range.step_size);
> -
> -	return ftmp * clk->info->range.step_size + fmin;
> -}
> -
>  static int scmi_clk_set_rate(struct clk_hw *hw, unsigned long rate,
>  			     unsigned long parent_rate)
>  {
> @@ -300,7 +271,6 @@ scmi_clk_ops_alloc(struct device *dev, unsigned long feats_key)
>  
>  	/* Rate ops */
>  	ops->recalc_rate = scmi_clk_recalc_rate;
> -	ops->round_rate = scmi_clk_round_rate;
>  	ops->determine_rate = scmi_clk_determine_rate;
>  	if (feats_key & BIT(SCMI_CLK_RATE_CTRL_SUPPORTED))
>  		ops->set_rate = scmi_clk_set_rate;

-- 
Alexander Sverdlin.