lkml.org 
[lkml]   [2021]   [Jul]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [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

    \
     
     \ /
      Last update: 2021-07-22 19:36    [W:3.275 / U:0.404 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site