Messages in this thread | | | From | Andy Shevchenko <> | Date | Thu, 22 Jul 2021 12:34:10 +0300 | Subject | Re: [PATCH v1 2/3] clk: fractional-divider: Introduce NO_PRESCALER flag |
| |
On Thu, Jul 22, 2021 at 12:11 PM Liu Ying <victor.liu@nxp.com> wrote: > On Thu, 2021-07-22 at 10:24 +0300, Andy Shevchenko wrote: > > On Thu, Jul 22, 2021 at 9:04 AM Liu Ying <victor.liu@nxp.com> wrote: > > > On Mon, 2021-07-19 at 15:09 +0300, Andy Shevchenko wrote: > > > > On Mon, Jul 19, 2021 at 11:16:07AM +0800, Liu Ying wrote: > > > > > 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:
...
> > > > core (or even TTY) has a specific function to approximate the baud rate and it > > > > tries it 2 or 3 times. In case of *saturated* values it won't progress anyhow > > > > because from best rational approximation algorithm the very first attempt would > > > > be done against the best possible clock rate. > > > > > > > > Can you provide some code skeleton to see? > > > > > > Perhaps, two approaches can be taken in driver which uses the > > > fractional divider clock: > > > 1) Tune prescaler to generate higher rate or lower rate accordingly > > > when clk_round_rate() for the fractional divider clock returns lower or > > > higher rates then desired rate. This might take several rounds until > > > desired rate is satisfied w/wo a tolerated bias. > > > 2) Put working clock rates and/or parent clock rates in a table as sort > > > of prior knowledge, which means less code for rate negotiation. > > > > Often 2) is a bad idea which I'm against from day 1. I prefer to > > calculate what can be calculated. > > The 1) looks better but requires several (unnecessary IIRC) rounds. > > Why not supply the additional parameter(s) to tell that we have a > > prescaller with certain limitations? > > To me, it's kinda too much information to this common frational divider > clk driver. Making the common driver simple and easy to maintain is > important.
But it has to have it due to the nature of the hardware design. If you leave it w/o that you have immediately come into the situation where the clock rate will be far too wrong because of *saturated* values. Have you done the arithmetics on the paper by the way?
...
> > I might disagree on the grounds of the HW hierarchy and the best that > > we may achieve in _one_ pass. For example, for a 16-bit additional > > prescaler it will require up to 16 steps to get the best possible > > Would that be an unacceptable performance penalty?
Yes.
> > values for the m/n. Instead we may supply to this driver the > > information about subordinate prescaler and get the best m/n. The > > caller will need to just divide the resulting rate by the asked rate > > to get a prescaler value. > > IMHO, a simpler fractional divider clk driver without the prescaler > knowledge wins the tradeoff.
I'm far from being convinced.
...
> > > > TL;DR: please send a code to discuss.
^^^^ I am tired of telling you this, btw.
> > > It seems that you have some experience on those intel drivers, this > > > clock driver and rational algorithm driver and you probably have intel > > > HWs to test. May I encourage you to look into this and decouple the > > > prescaler knowledge out :-) > > > > > > > Thanks for review and you review of v2 is warmly welcomed! > > > > > > I'd like to see patches to decouple the prescaler knowledge out. > > > > Then produce them! Currently the code works for all its users and does > > not need any changes (documentation is indeed a gap). > > IIUC, only the two Intel drivers mentioned before are affected. > Rockchip has it's own ->approximation() callback
...which is using the same algo, look at the patch 1 of the series. It seems you missed to actually review. Just review the series as a whole, please!
> and i.MX7ulp hasn't > the prescaler(IIUC), thus kinda not affected. So, perhaps you may help > look into this and decouple the prescaler knowledge out, as it seems > that you have experience on the relevant drivers and HW to test.
> Anyway, to me, it is _not_ a must to have if you really think it's hard > to do or unnesessary :-)
...
> > > V2, like v1, tries to consolidate the knowledge in this fractional > > > divider clk driver. So, not the right direction I think. > > > > Then why are you commenting here and not there? :-) > > Maybe v2 was sent too quickly as the decoupling comment on v1 hasn't > been sufficiently discussed :-)
Maybe.
> I'll comment v2 briefly.
Thanks!
...
> > I think I would drop patch 2 from the set (patch 1 is Acked and patch > > 3 is definitely needed to describe current state of affairs) on the > > grounds of the comments. > > Please consider i.MX7ulp, as it hasn't the prescaler IIUC. i.MX7ulp > needs NO_PRESCALER flag, if we keep the prescaler knowledge in this > driver ofc.
Then we need a flag and v2 can go as is.
-- With Best Regards, Andy Shevchenko
| |