Re: [PATCH v1 2/3] clk: fractional-divider: Introduce NO_PRESCALER flag

[Liu Ying <victor.liu@nxp.com>]

On Fri, 2021-07-16 at 16:19 +0300, Andy Shevchenko wrote:
> On Fri, Jul 16, 2021 at 10:43:57AM +0800, Liu Ying wrote:
> > On Thu, 2021-07-15 at 15:07 +0300, Andy Shevchenko wrote:
> > > The newly introduced flag, when set, makes the flow to skip
> > > the assumption that the caller will use an additional 2^scale
> > > prescaler to get the desired clock rate.
> > 
> > Now, I start to be aware of the reason why the "left shifting" is
> > needed but still not 100% sure that details are all right. IIUC, you
> > are considering a potential HW prescaler here, while I thought the HW
> > model is just a fractional divider(M/N) and the driver is fully
> > agnostic to the potential HW prescaler.
> 
> It's not AFAICS. Otherwise we will get saturated values which is much worse
> then shifted left frequency. Anyway, this driver appeared first for the hardware
> that has it for all users, so currently the assumption stays.
> 
> ...
> 
> > >  	scale = fls_long(*parent_rate / rate - 1);
> > > -	if (scale > fd->nwidth)
> > > +	if (scale > fd->nwidth && !(fd->flags & CLK_FRAC_DIVIDER_NO_PRESCALER))
> > >  		rate <<= scale - fd->nwidth;
> > 
> > First of all, check the CLK_FRAC_DIVIDER_NO_PRESCALER flag for the
> > entire above snippet of code?
> 
> OK.
> 
> > Second and more important, it seems that it would be good to decouple
> > the prescaler knowledge from this fractional divider clk driver so as
> > to make it simple(Output rate = (m / n) * parent_rate).  This way, the
> > CLK_FRAC_DIVIDER_NO_PRESCALER flag is not even needed at the first
> > place, which means rational_best_approximation() just _directly_
> > offer best_{numerator,denominator} for all cases.
> 
> Feel free to submit a patch, just give a good test to avoid breakage of almost
> all users of this driver.

Maybe someone may do that.  I just shared my thought that it sounds
like a good idea to decouple the prescaler knowledge from this
fractional divider clk driver.

> 
> > Further more, is it
> > possilbe for rational_best_approximation() to make sure there is no
> > risk of overflow for best_{numerator,denominator}, since
> > max_{numerator,denominator} are already handed over to
> > rational_best_approximation()?
> 
> How? It can not be satisfied for all possible inputs.

Just have rational_best_approximation() make sure
best_{numerator,denominator} are in the range of
[1, max_{numerator,denominator}] for all given_{numerator,denominator}.
At the same time, best_numerator/best_denominator should be as close
to given_numerator/given_denominator as possible. For this particular
fractional divider clk use case, clk_round_rate() can be called
multiple times until users find rounded rate is ok.

> 
> > Overflowed/unreasonable
> > best_{numerator,denominator} don't sound like the "best" offered value.
> 
> I don't follow here. If you got saturated values it means that your input is
> not convergent. In practice it means that we will supply quite a bad value to
> the caller.

Just like I mentioned above, if given_{numerator,denominator} are not
convergent, best_numerator/best_denominator should be as close
to given_numerator/given_denominator as possible and at the same time
best_{numerator,denominator} are in the range of
[1, max_{numerator,denominator}].  This way, caller may have chance to
propose convergent inputs.

Regards,
Liu Ying

> 
> > If that's impossible, then audit best_{numerator,denominator} after
> > calling rational_best_approximation()?
> 
> And? I do not understand what you will do if you get the values of m and n
> as m = 1, n = 2^nlim - 1.
> 
> > Make sense?
> 
> Not really. I probably miss your point, sorry.
> 
> So, I will submit v2 with addressed first comment and LKP noticed compiler
> error.
>