| Message ID | 65a34dd943b0260bfe45ec76dcf414a67e5d8343.1565785291.git.baolin.wang@linaro.org |
|---|---|
| State | Accepted |
| Commit | bdaadd5948179f07ca8b508a62a8f8b00ece51ed |
| Headers | show |
| Series | [v3,1/2] dt-bindings: pwm: sprd: Add Spreadtrum PWM documentation | expand |
On Thu, 15 Aug 2019 at 16:54, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Hello Baolin, > > On Thu, Aug 15, 2019 at 04:16:32PM +0800, Baolin Wang wrote: > > On Thu, 15 Aug 2019 at 14:15, Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > > > On Thu, Aug 15, 2019 at 11:34:27AM +0800, Baolin Wang wrote: > > > > 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. > > > > > > Is this a license requirement for you SoC to only drive a backlight with > > > the PWM? The idea of having a PWM driver on your platform is that it can > > > also be used to control a step motor or a laser. > > > > Not a license requirement. Until now we have not got any higher > > precision requirements, and we've run this driver for many years in > > our downstream kernel. > > I understood that you're not ambitious to do something better than "it > worked for years". How do you know that? If there are some cases expect a higher precision, then we can analyze how precision asked by the user, then we have a goal to improve it, even improve the hardware. But now, I said there are no these use cases, why I should add more mathematics to increase load and complication. > > > > > 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 > > > > > > Yes, as above. When using rounding-closest your error is not in [0, 7.65 > > > µs] but in [-3.825 µs, 3.825 µs]. Doesn't make it better. > > > > Actually our use case really dose not care about this error. > > I assume that Thierry will apply your patch anyhow. But be prepared that > you get a patch from me then to improve precision. It would be a waste > of resources not to do that after doing all the necessary math already. Glad to see your improvement without introducing complicated and more mathematics. > > > > > > (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; > > > > > > simple is often good but sometimes different from correct. And even with > > > > I do not think this is incorrect. > > Well, "correct" is probably not the right term. The problem is that my > efforts to improve the PWM framework are not going forward. And so the > suggestions I made here are not normative (yet) and undocumented. OK. > > > > rounding closest instead of rounding down you're giving away precision > > > here and the size of the error interval is the same, it is just centered > > > around 0 instead of only positive. If I hadn't spend so much time with > > > pwm reviews this week I'd try to come up with an example. > > > > I am very appreciated for your comments. > > > > > > > (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. > > > > > > If you go back to rounding down, use the matching rounding up in > > > .get_state and adapt your comment describing you're sticking to MOD=255 > > > to say explicitly that you're loosing precision I can live with that. I > > > even did the math for .get_state in my previous mail for you. > > > > > > The idea of the requirement to round down is that I want to introduce > > > this as policy for the PWM framework to get some uniform behaviour from > > > > Have you made a consensus about this? Documented it? > > I tried. Check the pwm patchwork, there are uncommented patches that > first try to document the current status quo. When these are "in" I > intend to discuss the improvements I have in mind. But don't expect this > to be quickly done as even the (AFAICT) noncontroversial documentation > patches[1] fail to get applied. OK. > > > > all lowlevel drivers. If you do this now I won't bother you later when > > > the requirement is implemented in your driver. And the comment helps > > > someone who evaluates your SoC to judge if there is still work to do if > > > they have higher requirements for the PWM. > > > > So what you asked is something like below, right? > > diff --git a/drivers/pwm/pwm-sprd.c b/drivers/pwm/pwm-sprd.c > > index 96f8aa0..1d3db94 100644 > > --- a/drivers/pwm/pwm-sprd.c > > +++ b/drivers/pwm/pwm-sprd.c > > @@ -103,12 +103,12 @@ static void sprd_pwm_get_state(struct pwm_chip > > *chip, struct pwm_device *pwm, > > 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); > > + state->period = DIV_ROUND_UP_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); > > + state->duty_cycle = DIV_ROUND_UP_ULL(tmp, chn->clk_rate); > > > > /* Disable PWM clocks if the PWM channel is not in enable state. */ > > if (!state->enabled) > > @@ -135,8 +135,8 @@ static int sprd_pwm_config(struct sprd_pwm_chip > > *spc, struct pwm_device *pwm, > > duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns; > > > > tmp = (u64)chn->clk_rate * period_ns; > > - do_div(tmp, NSEC_PER_SEC); > > - prescale = DIV_ROUND_CLOSEST_ULL(tmp, SPRD_PWM_MOD_MAX) - 1; > > + div = 1000000000ULL * SPRD_PWM_MOD_MAX; > > + prescale = div64_u64(tmp, div) - 1; > > if (prescale > SPRD_PWM_PRESCALE_MSK) > > prescale = SPRD_PWM_PRESCALE_MSK; > > This goes in the right direction for sure. > > Without taking paper and pencil I wouldn't be surprised if the > calculation of duty_cycle in .get_state didn't match the calculation of > DUTY in .apply yet though. > > > But our MOD is constant, it did not help to improve the precision. > > Instead, like you said, when period_ns = 145340, we will set PRESCALE > > = 17, so in .get_state(), user will get period_ns = 137700 (error > > is145340 - 137700). > > > > But if we use DIV_ROUND_CLOSEST, in .get_state(), user will get > > period_ns = 145350 (error is 145350 - 145340). > > In this case DIV_ROUND_CLOSEST seems to get nearer to the requested > value than when rounding down. But this example was constructed to show > your original algorithm to be bad, and just because you modify your > algorithm to perform better on that constructed example doesn't imply > the new one is better. Moreover you implement a bigger period than > requested which is something I intend to forbid in the future. > > And note that with PWMs there is no "objective" metric that can tell you > which of two implementable outputs better match a given request. It > depends on the use case, so the best we can do is to tell our users our > metric and with that in mind they can create a request that then fits > their needs. Yes, that should be asked by the use case, some cases do not care a little bigger period than requested. As you said, what you asked did not get a consensus yet, so I'd like to wait for Thierry's suggestion. > > > > > 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(). > > > > > > I don't get you here. Do you say that with DIV_ROUND_CLOSEST_ULL you get > > > the same result but DIV_ROUND_CLOSEST_ULL matches .get_state while > > > rounding down doesn't? I cannot follow. > > > > Yes, that's what I mean. > > But that is logically broken. If both approaches yield the same > results it cannot be true that exactly one of them matches the inverse > of .get_state. What I mean is use DIV_ROUND_CLOSEST_ULL we can get a nearer value to the requested like above example. -- Baolin Wang Best Regards
On Wed, 14 Aug 2019 20:46:10 +0800, Baolin Wang wrote: > Add Spreadtrum PWM controller documentation. > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > --- > Changes from v2: > - Fix some typos. > - Move assigned-clocks to be optional. > > Changes from v1: > - Use assigned-clock-parents and assigned-clocks to set PWM clock parent. > --- > Documentation/devicetree/bindings/pwm/pwm-sprd.txt | 40 ++++++++++++++++++++ > 1 file changed, 40 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sprd.txt > Reviewed-by: Rob Herring <robh@kernel.org>
diff --git a/Documentation/devicetree/bindings/pwm/pwm-sprd.txt b/Documentation/devicetree/bindings/pwm/pwm-sprd.txt new file mode 100644 index 0000000..16fa5a0 --- /dev/null +++ b/Documentation/devicetree/bindings/pwm/pwm-sprd.txt @@ -0,0 +1,40 @@ +Spreadtrum PWM controller + +Spreadtrum SoCs PWM controller provides 4 PWM channels. + +Required properties: +- compatible : Should be "sprd,ums512-pwm". +- reg: Physical base address and length of the controller's registers. +- clocks: The phandle and specifier referencing the controller's clocks. +- clock-names: Should contain following entries: + "pwmn": used to derive the functional clock for PWM channel n (n range: 0 ~ 3). + "enablen": for PWM channel n enable clock (n range: 0 ~ 3). +- #pwm-cells: Should be 2. See pwm.txt in this directory for a description of + the cells format. + +Optional properties: +- assigned-clocks: Reference to the PWM clock entries. +- assigned-clock-parents: The phandle of the parent clock of PWM clock. + +Example: + pwms: pwm@32260000 { + compatible = "sprd,ums512-pwm"; + reg = <0 0x32260000 0 0x10000>; + clock-names = "pwm0", "enable0", + "pwm1", "enable1", + "pwm2", "enable2", + "pwm3", "enable3"; + clocks = <&aon_clk CLK_PWM0>, <&aonapb_gate CLK_PWM0_EB>, + <&aon_clk CLK_PWM1>, <&aonapb_gate CLK_PWM1_EB>, + <&aon_clk CLK_PWM2>, <&aonapb_gate CLK_PWM2_EB>, + <&aon_clk CLK_PWM3>, <&aonapb_gate CLK_PWM3_EB>; + assigned-clocks = <&aon_clk CLK_PWM0>, + <&aon_clk CLK_PWM1>, + <&aon_clk CLK_PWM2>, + <&aon_clk CLK_PWM3>; + assigned-clock-parents = <&ext_26m>, + <&ext_26m>, + <&ext_26m>, + <&ext_26m>; + #pwm-cells = <2>; + };
Add Spreadtrum PWM controller documentation. Signed-off-by: Baolin Wang <baolin.wang@linaro.org> --- Changes from v2: - Fix some typos. - Move assigned-clocks to be optional. Changes from v1: - Use assigned-clock-parents and assigned-clocks to set PWM clock parent. --- Documentation/devicetree/bindings/pwm/pwm-sprd.txt | 40 ++++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sprd.txt -- 1.7.9.5