# Re: [PATCH v3 2/2] pwm: sprd: Add Spreadtrum PWM support

**From: **Baolin Wang

**Date: ** Wed Aug 14 2019 - 23:34:54 EST

Hi Uwe,

On Wed, 14 Aug 2019 at 23:03, Uwe Kleine-KÃnig

<u.kleine-koenig@xxxxxxxxxxxxxx> 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