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

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

On Thu, 2021-07-22 at 12:34 +0300, Andy Shevchenko wrote:
> 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!

But, the topic is to decouple the prescaler knowledge.
I reviewed it as a whole though I was not Cc'ed for the patch 1/3. It
looks like Rockchip driver doesn't have to be touched if the prescaler
knowledge is decoupled from this fractional divider clk driver.  If you
consolidate the prescaler knowledge in the Rockchip driver as patch 1/3
does, you touch it.

Regards,
Liu Ying

> 
> >  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.
>