Re: [PATCH v2 54/65] clk: da8xx: clk48: Switch to determine_rate

David Lechner
2022-11-04T16:50:25Z

On 11/4/22 8:18 AM, Maxime Ripard wrote:
> The TI DA8xx USB0 clk48 clocks implements a mux with a set_parent
> hook, but doesn't provide a determine_rate implementation.
>
> This is a bit odd, since set_parent() is there to, as its name implies,
> change the parent of a clock. However, the most likely candidate to
> trigger that parent change is a call to clk_set_rate(), with
> determine_rate() figuring out which parent is the best suited for a
> given rate.
>
> The other trigger would be a call to clk_set_parent(), but it's far less
> used, and it doesn't look like there's any obvious user for that clock.
>
> So, the set_parent hook is effectively unused, possibly because of an
> oversight. However, it could also be an explicit decision by the
> original author to avoid any reparenting but through an explicit call to
> clk_set_parent().
>
> The driver does implement round_rate() though, which means that we can
> change the rate of the clock, but we will never get to change the
> parent.
>
> However, It's hard to tell whether it's been done on purpose or not.
>
> Since we'll start mandating a determine_rate() implementation, let's
> convert the round_rate() implementation to a determine_rate(), which
> will also make the current behavior explicit. And if it was an
> oversight, the clock behaviour can be adjusted later on.

I think this one should be the same as the clk:davinci changes and
not allow re-parenting. Since this is a USB 48MHz PHY clock, a rate
change will never be requested.

>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
> drivers/clk/davinci/da8xx-cfgchip.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/clk/davinci/da8xx-cfgchip.c b/drivers/clk/davinci/da8xx-cfgchip.c
> index 4c1cc59bba53..f60c97091818 100644
> --- a/drivers/clk/davinci/da8xx-cfgchip.c
> +++ b/drivers/clk/davinci/da8xx-cfgchip.c
> @@ -462,10 +462,12 @@ static unsigned long da8xx_usb0_clk48_recalc_rate(struct clk_hw *hw,
> return 48000000;
> }
>
> -static long da8xx_usb0_clk48_round_rate(struct clk_hw *hw, unsigned long rate,
> - unsigned long *parent_rate)
> +static int da8xx_usb0_clk48_determine_rate(struct clk_hw *hw,
> + struct clk_rate_request *req)
> {
> - return 48000000;
> + req->rate = 48000000;
> +
> + return 0;
> }
>
> static int da8xx_usb0_clk48_set_parent(struct clk_hw *hw, u8 index)
> @@ -494,7 +496,7 @@ static const struct clk_ops da8xx_usb0_clk48_ops = {
> .disable = da8xx_usb0_clk48_disable,
> .is_enabled = da8xx_usb0_clk48_is_enabled,
> .recalc_rate = da8xx_usb0_clk48_recalc_rate,
> - .round_rate = da8xx_usb0_clk48_round_rate,
> + .determine_rate = da8xx_usb0_clk48_determine_rate,
> .set_parent = da8xx_usb0_clk48_set_parent,
> .get_parent = da8xx_usb0_clk48_get_parent,
> };
>