Messages in this thread | | | Subject | Re: [PATCH v1 2/3] clk: fractional-divider: Introduce NO_PRESCALER flag | From | Liu Ying <> | Date | Mon, 19 Jul 2021 11:16:07 +0800 |
| |
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. >
| |