Re: [PATCH 4/7] pwm: jz4740: Improve algorithm of clock calculation

[Paul Cercueil]

[Re-send my message in plain text, as it was bounced by the
lists - sorry about that]


Le lun. 12 août 2019 à 23:48, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= 
<u.kleine-koenig@xxxxxxxxxxxxxx> a écrit :
> Hello Paul,
>
> On Mon, Aug 12, 2019 at 10:43:10PM +0200, Paul Cercueil wrote:
> >  Le lun. 12 août 2019 à 8:15, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
> >  <u.kleine-koenig@xxxxxxxxxxxxxx> a écrit :
> >  > On Fri, Aug 09, 2019 at 07:14:45PM +0200, Paul Cercueil wrote:
> >  > >  Le ven. 9 août 2019 à 19:05, Uwe 
> > =?iso-8859-1?q?Kleine-K=F6nig?=
> >  > >  <u.kleine-koenig@xxxxxxxxxxxxxx> a écrit :
> >  > >  > On Fri, Aug 09, 2019 at 02:30:28PM +0200, Paul Cercueil 
> > wrote:
> >  > >  > > [...]
> >  > >  > >  +	/* Reset the clock to the maximum rate, and we'll 
> > reduce it if needed */
> >  > >  > >  +	ret = clk_set_max_rate(clk, parent_rate);
> >  > >  >
> >  > >  > What is the purpose of this call? IIUC this limits the 
> > allowed range of
> >  > >  > rates for clk. I assume the idea is to prevent other 
> > consumers to change
> >  > >  > the rate in a way that makes it unsuitable for this pwm. But 
> > this only
> >  > >  > makes sense if you had a notifier for clk changes, doesn't 
> > it? I'm
> >  > >  > confused.
> >  > >
> >  > >  Nothing like that. The second call to clk_set_max_rate() might 
> > have set
> >  > >  a maximum clock rate that's lower than the parent's rate, and 
> > we want to
> >  > >  undo that.
> >  >
> >  > I still don't get the purpose of this call. Why do you limit the 
> > clock
> >  > rate at all?
> >
> >  As it says below, we "limit the clock to a maximum rate that still 
> > gives
> >  us a period value which fits in 16 bits". So that the computed 
> > hardware
> >  values won't overflow.
>
> But why not just using clk_set_rate? You want to have the clock 
> running
> at a certain rate, not any rate below that certain rate, don't you?

I'll let yourself answer yourself:
https://patchwork.ozlabs.org/patch/1018969/

It's enough to run it below a certain rate, yes. The actual rate doesn't
actually matter that much.


> >  E.g. if at a rate of 12 MHz your computed hardware value for the 
> > period
> >  is 0xf000, then at a rate of 24 MHz it won't fit in 16 bits. So the 
> > clock
> >  rate must be reduced to the highest possible that will still give 
> > you a
> >  < 16-bit value.
> >
> >  We always want the highest possible clock rate that works, for the 
> > sake of
> >  precision.
>
> This is dubious; but ok to keep the driver simple. (Consider a PWM 
> that
> can run at i MHz for i in [1, .. 30]. If a period of 120 ns and a duty
> cycle of 40 ns is requested you can get an exact match with 25 MHz, 
> but
> not with 30 MHz.)

The clock rate is actually (parent_rate >> (2 * x) )
for x = 0, 1, 2, ...

So if your parent_rate is 30 MHz the next valid one is 7.5 MHz, and the
next one is 1.875 MHz. It'd be very unlikely that you get a better 
match at a
lower clock.


> >  > >  Basically, we start from the maximum clock rate we can get for 
> > that PWM
> >  > >  - which is the rate of the parent clk - and from that compute 
> > the maximum
> >  > >  clock rate that we can support that still gives us < 16-bits 
> > hardware
> >  > >  values for the period and duty.
> >  > >
> >  > >  We then pass that computed maximum clock rate to 
> > clk_set_max_rate(), which
> >  > >  may or may not update the current PWM clock's rate to match 
> > the new limits.
> >  > >  Finally we read back the PWM clock's rate and compute the 
> > period and duty
> >  > >  from that.
> >  >
> >  > If you change the clk rate, is this externally visible on the PWM
> >  > output? Does this affect other PWM instances?
> >
> >  The clock rate doesn't change the PWM output because the hardware 
> > values for
> >  the period and duty are adapted accordingly to reflect the change.
>
> It doesn't change it in the end. But in the (short) time frame between
> the call to change the clock and the update of the PWM registers there
> is a glitch, right?

The PWM is disabled, so the line is in inactive state, and will be in 
that state
until the PWM is enabled again. No glitch to fear.


> You didn't answer to the question about other PWM instances. Does that
> mean others are not affected?

Sorry. Yes, they are not affected - all PWM channels are independent.


> Best regards
> Uwe
>
> PS: It would be great if you could fix your mailer to not damage the
> quoted mail. Also it doesn't seem to understand how my name is encoded
> in the From line. I fixed up the quotes in my reply.

I guess I'll submit a bug report to Geary then.


> --
> Pengutronix e.K.                           | Uwe Kleine-König        
>     |
> Industrial Linux Solutions                 | 
> http://www.pengutronix.de/  |