Re: [Patch v9] driver/clk/clk-si5338: Add common clock framework driver for si5338

[Stephen Boyd]

Quoting Radu Nicolae Pirea (2019-05-31 07:06:03)
> > +     remove_common_factor(&rate[1]);
> > +     dev_dbg(&drvdata->client->dev,
> > +             "PLL output frequency: %llu+%llu/%llu Hz\n",
> > +             rate[0], rate[1], rate[2]);
> > +
> > +     return rate[0];
> > +}
> > +
> > +static long si5338_pll_round_rate(struct clk_hw *hw, unsigned long
> > rate,
> > +                               unsigned long *parent_rate)
> > +{
> I think is a designs problem in clock subsystem.
> Description of the function round_rate is this.
> 
> @round_rate: Given a target rate as input, returns the closest rate
> actually supported by the clock. The parent rate is an input/output
> parameter.
> 
> If given rate is unsigned long and the return value is long, we have a
> problem. Return value can be an error value but can also be a 32 bit
> frequency value that can be huge enoguh to set MSB bit and if MSB bit
> is set, the return value is a negative value and will be considered
> error code.
> 
> In drivers/clk/clk.c, in function clk_core_determine_round_nolock, on
> line 1199 is this sequence of code that checks if the value returned by
> round_rate function is negative:
> 
>          } else if (core->ops->round_rate) {
>                  rate = core->ops->round_rate(core->hw, req->rate,
>                                               &req->best_parent_rate);
>                  if (rate < 0)
>                          return rate;
> 
> In drivers/clk/clk.c, in function clk_calc_new_rates, on line 1780, is
> the next sequence of code that checks if
> clk_core_determine_round_nolock returns a negative value:
> 
>                  ret = clk_core_determine_round_nolock(core, &req);
>                  if (ret < 0)
>                           return NULL;
> 
> and... if function clk_calc_new_rates returns NULL, in function
> clk_core_set_rate_nolock from drivers/clk/clk.c, on line 2023, is the
> next sequence of code that evaluates NULL, returned by
> clk_calc_new_rates as -EINVAL
> 
>          top = clk_calc_new_rates(core, req_rate);
>          if (!top)
>                  return -EINVAL;
> 
> So... si5338_pll_round_rate can return 32 bit values like 2500000000 Hz
> who have MSB bit set and are interpreted as error codes.
> 
> Have someone a suggestion to fix this issue?
> 

Can you use the determine_rate clk op? It won't work for rounding
outside of the framework though so things like clk_round_rate() still
fail.