linux-pwm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Baolin Wang <baolin.wang@linaro.org>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Thierry Reding <thierry.reding@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Orson Zhai <orsonzhai@gmail.com>,
	Chunyan Zhang <zhang.lyra@gmail.com>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	linux-pwm@vger.kernel.org, DTML <devicetree@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 2/2] pwm: sprd: Add Spreadtrum PWM support
Date: Thu, 15 Aug 2019 11:34:27 +0800	[thread overview]
Message-ID: <CAMz4kuLiS=cGTA=uEi9ABOVAOb1M0Pcd2a_xU5VsdLo1DGd0Hg@mail.gmail.com> (raw)
In-Reply-To: <20190814150304.x44lalde3cwp67ge@pengutronix.de>

Hi Uwe,

On Wed, 14 Aug 2019 at 23:03, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Wed, Aug 14, 2019 at 08:46:11PM +0800, Baolin Wang wrote:

> > +
> > +     /*
> > +      * The hardware provides a counter that is feed by the source clock.
> > +      * The period length is (PRESCALE + 1) * MOD counter steps.
> > +      * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
> > +      * Thus the period_ns and duty_ns calculation formula should be:
> > +      * period_ns = NSEC_PER_SEC * (prescale + 1) * mod / clk_rate
> > +      * duty_ns = NSEC_PER_SEC * (prescale + 1) * duty / clk_rate
> > +      */
> > +     val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE);
> > +     prescale = val & SPRD_PWM_PRESCALE_MSK;
> > +     tmp = (prescale + 1) * NSEC_PER_SEC * SPRD_PWM_MOD_MAX;
> > +     state->period = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> > +
> > +     val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY);
> > +     duty = val & SPRD_PWM_DUTY_MSK;
> > +     tmp = (prescale + 1) * NSEC_PER_SEC * duty;
> > +     state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> > +
> > +     /* Disable PWM clocks if the PWM channel is not in enable state. */
> > +     if (!state->enabled)
> > +             clk_bulk_disable_unprepare(SPRD_PWM_CHN_CLKS_NUM, chn->clks);
> > +}
> > +
> > +static int sprd_pwm_config(struct sprd_pwm_chip *spc, struct pwm_device *pwm,
> > +                        int duty_ns, int period_ns)
> > +{
> > +     struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> > +     u32 prescale, duty;
> > +     u64 tmp;
> > +
> > +     /*
> > +      * The hardware provides a counter that is feed by the source clock.
> > +      * The period length is (PRESCALE + 1) * MOD counter steps.
> > +      * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
> > +      *
> > +      * To keep the maths simple we're always using MOD = SPRD_PWM_MOD_MAX.
>
> Did you spend some thoughts about how wrong your period can get because
> of that "lazyness"?
>
> Let's assume a clk rate of 100/3 MHz. Then the available period lengths
> are:
>
>         PRESCALE =  0  ->  period =   7.65 µs
>         PRESCALE =  1  ->  period =  15.30 µs
>         ...
>         PRESCALE = 17  ->  period = 137.70 µs
>         PRESCALE = 18  ->  period = 145.35 µs
>
> So the error can be up to (nearly) 7.65 µs (or in general

Yes, but for our use case (pwm backlight), the precision can meet our
requirement. Moreover, we usually do not change the period, just
adjust the duty to change the back light.

> 255 / clk_rate) because if 145.34 µs is requested you configure
> PRESCALE = 17 and so yield a period of 137.70 µs. If however you'd pick

I did not get you here, if period is 145.34, we still get the
corresponding PRESCALE = 18 by below formula:

tmp = (u64)chn->clk_rate * period_ns;
do_div(tmp, NSEC_PER_SEC);
prescale = DIV_ROUND_CLOSEST_ULL(tmp, SPRD_PWM_MOD_MAX) - 1;

> PRESCALE = 18 and MOD = 254 you get a period of 144.78 µs and so the
> error is only 0.56 µs which is a factor of 13 better.
>
> Hmm.
>
> > +      * The value for PRESCALE is selected such that the resulting period
> > +      * gets the maximal length not bigger than the requested one with the
> > +      * given settings (MOD = SPRD_PWM_MOD_MAX and input clock).
> > +      */
> > +     duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;
>
> I wonder if you loose some precision here as you use period_ns but might
> actually implement a shorter period.
>
> Quick example, again consider clk_rate = 100 / 3 MHz,
> period_ns = 145340, duty_ns = 72670. Then you end up with
>
>         PRESCALE = 17
>         MOD = 255
>         DUTY = 127

Incorrect, we will get PRESCALE = 18,  MOD = 255, DUTY = 127.

> That corresponds to period_ns = 137700, duty_ns = 68580. With DUTY = 134
> you get 72360 ns which is still smaller than the requested 72670 ns.

Incorrect, with DUTY = 134 (PRESCALE = 18  ->  period = 145.35 µs),
duty_ns = 76380ns

> (But then again it is not obvious which of the two is the "better"
> approximation because Thierry doesn't seem to see the necessity to
> discuss or define a policy here.)

Like I said, this is the simple calculation formula which can meet our
requirement (we limit our DUTY value can only be 0 - 254).
duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;

>
> (And to pick up the thoughts about not using SPRD_PWM_MOD_MAX
> unconditionally, you could also use
>
>         PRESCALE = 18
>         MOD = 254
>         DUTY = 127
>
> yielding period_ns = 144780 and duty_ns = 72390. Summary:
>
>         Request:                 72670 / 145340
>         your result:             68580 / 137700
>         also possible:           72390 / 144780
>
> Judge yourself.)
>
> > +     tmp = (u64)chn->clk_rate * period_ns;
> > +     do_div(tmp, NSEC_PER_SEC);
> > +     prescale = DIV_ROUND_CLOSEST_ULL(tmp, SPRD_PWM_MOD_MAX) - 1;
>
> Now that you use DIV_ROUND_CLOSEST_ULL the comment is wrong because you
> might provide a period bigger than the requested one. Also you divide

Again, that's the precision can meet our requirement.

> twice instead of once before. (I don't know what architecture your SoC
> uses, but compared to a multiplication a division is usually expensive.)
> Also the math is more complicated now as you have a round-down div and a
> round-closest div.
>
> My preference for how to fix that is to restore the behaviour of v2 that
> matches the comment and adapt .get_state() instead.

Using DIV_ROUND_CLOSEST_ULL can get a same prescale which matches with
.get_state().

>
> For .get_state() this should then be:
>
>         val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE);
>         prescale = FIELD_GET(SPRD_PWM_PRESCALE_MSK, val);
>
>         tmp = (prescale + 1) * NSEC_PER_SEC * SPRD_PWM_MOD_MAX;
>         state->period = DIV_ROUND_UP_ULL(tmp, chn->clk_rate);
>
> > +     if (prescale > SPRD_PWM_PRESCALE_MSK)
> > +             prescale = SPRD_PWM_PRESCALE_MSK;
> > +
> > +     /*
> > +      * Note: Writing DUTY triggers the hardware to actually apply the
> > +      * values written to MOD and DUTY to the output, so must keep writing
> > +      * DUTY last.
> > +      *
> > +      * The hardware can ensures that current running period is completed
> > +      * before changing a new configuration to avoid mixed settings.
>
> I'm not a native English speaker, but I would write:
>
>         * The hardware ensures that currently running period is
>         * completed before changing to the new configuration to avoid
>         * mixed settings.

Sure

>
> Does this mechanism also apply to PRESCALE? If yes, please include it in

Yes.

> your description. If not there is still a possibility for a wrong
> period.
>
> > +      */
> > +     sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PRESCALE, prescale);
> > +     sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_MOD, SPRD_PWM_MOD_MAX);
> > +     sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_DUTY, duty);
> > +
> > +     return 0;
> > +}
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |



-- 
Baolin Wang
Best Regards

  reply	other threads:[~2019-08-15  3:34 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-14 12:46 [PATCH v3 1/2] dt-bindings: pwm: sprd: Add Spreadtrum PWM documentation Baolin Wang
2019-08-14 12:46 ` [PATCH v3 2/2] pwm: sprd: Add Spreadtrum PWM support Baolin Wang
2019-08-14 15:03   ` Uwe Kleine-König
2019-08-15  3:34     ` Baolin Wang [this message]
2019-08-15  6:15       ` Uwe Kleine-König
2019-08-15  8:16         ` Baolin Wang
2019-08-15  8:54           ` Uwe Kleine-König
2019-08-15  9:34             ` Baolin Wang
2019-08-15 10:11               ` Uwe Kleine-König
2019-08-15 11:05                 ` Baolin Wang
2019-08-15 12:25                   ` Uwe Kleine-König
2019-08-16  2:44                     ` Baolin Wang
2019-08-16  6:45                       ` Uwe Kleine-König
2019-08-27 16:20 ` [PATCH v3 1/2] dt-bindings: pwm: sprd: Add Spreadtrum PWM documentation Rob Herring

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAMz4kuLiS=cGTA=uEi9ABOVAOb1M0Pcd2a_xU5VsdLo1DGd0Hg@mail.gmail.com' \
    --to=baolin.wang@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pwm@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=orsonzhai@gmail.com \
    --cc=robh+dt@kernel.org \
    --cc=thierry.reding@gmail.com \
    --cc=u.kleine-koenig@pengutronix.de \
    --cc=vincent.guittot@linaro.org \
    --cc=zhang.lyra@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).