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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

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