From: Sean Anderson <sean.anderson@seco.com>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: linux-pwm@vger.kernel.org, devicetree@vger.kernel.org,
Thierry Reding <thierry.reding@gmail.com>,
Alvaro Gamez <alvaro.gamez@hazent.com>,
michal.simek@xilinx.com, Lee Jones <lee.jones@linaro.org>,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 3/3] pwm: Add support for Xilinx AXI Timer
Date: Tue, 17 Aug 2021 18:56:28 -0400 [thread overview]
Message-ID: <082d39cd-1ce5-344f-3229-3beb7cea0a15@seco.com> (raw)
In-Reply-To: <20210817180407.ru4prwu344dxpynu@pengutronix.de>
On 8/17/21 2:04 PM, Uwe Kleine-König wrote:
> On Mon, Aug 16, 2021 at 07:51:17PM -0400, Sean Anderson wrote:
>>
>>
>> On 8/14/21 4:47 PM, Uwe Kleine-König wrote:
>> > Hello Sean,
>> >
>> > sorry for having you let waiting so long. Now here some more feedback:
>> >
>> > On Mon, Jul 19, 2021 at 06:13:22PM -0400, Sean Anderson wrote:
>> > > +static int xilinx_pwm_apply(struct pwm_chip *chip, struct pwm_device *unused,
>> > > + const struct pwm_state *state)
>> > > +{
>> > > + bool enabled;
>> > > + struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
>> > > + u32 tlr0, tlr1, tcsr0, tcsr1;
>> > > + u64 period_cycles, duty_cycles;
>> > > + unsigned long rate;
>> > > +
>> > > + if (state->polarity != PWM_POLARITY_NORMAL)
>> > > + return -EINVAL;
>> > > +
>> > > + /*
>> > > + * To be representable by TLR, cycles must be between 2 and
>> > > + * priv->max + 2. To enforce this we can reduce the duty
>> > > + * cycle, but we may not increase it.
>> > > + */
>> > > + rate = clk_get_rate(priv->clk);
>> > > + period_cycles = mul_u64_u32_div(state->period, rate, NSEC_PER_SEC);
>> >
>> > cool, I didn't know mul_u64_u32_div.
>>
>> I didn't either. Alas, many useful functions like these have no
>> documentation...
>>
>> >
>> > Hmm, we still have a problem here if
>> >
>> > state->period * rate > 1000000000 * U64_MAX.
>>
>> Note that this can only occur with rate > 1GHz (and period = U64_MAX).
>> The highest fmax in the datasheet is 300 MHz (on a very expensive FPGA).
>>
>> Maybe it is more prudent to do
>>
>> period = min(state->period, ULONG_MAX * NSEC_PER_SEC)
>
> Together with a check for rate being <= 300 MHz to be safe that's fine.
That's what the ULONG_MAX is for; whatever we get back from
clk_get_rate, it has to fit in a ulong.
>
>> I think a period of 136 years is adequate :) This comparison also has
>> the advantage of being against const values.
>
> *nod*
>
>> > > +static void xilinx_pwm_get_state(struct pwm_chip *chip,
>> > > + struct pwm_device *unused,
>> > > + struct pwm_state *state)
>> > > +{
>> > > + struct xilinx_timer_priv *priv = xilinx_pwm_chip_to_priv(chip);
>> > > + u32 tlr0, tlr1, tcsr0, tcsr1;
>> > > +
>> > > + regmap_read(priv->map, TLR0, &tlr0);
>> > > + regmap_read(priv->map, TLR1, &tlr1);
>> > > + regmap_read(priv->map, TCSR0, &tcsr0);
>> > > + regmap_read(priv->map, TCSR1, &tcsr1);
>> > > + state->period = xilinx_timer_get_period(priv, tlr0, tcsr0);
>> >
>> > xilinx_timer_get_period rounds down, this is however wrong for
>> > .get_state().
>>
>> Why is this wrong? I thought get_state should return values which would
>> not be rounded if passed to apply_state.
>
> Consider a PWM that yields a period of π * $regval ns when a certain
> register is programmed with the value $regval.
>
> Consider the HW is programmed with regval = 317. The exact period is
> 995.8848711879644. If now .get_state() rounds down and returns 995 ns and
> you feed that value back into .apply the new regval (assuming round down
> in .apply(), too) this yields regval = 316. If however .get_state()
> rounds up and returns 996, putting this value back into .apply() you get
> the desired 317.
Will fix for v6, but please document this somewhere :)
--Sean
next prev parent reply other threads:[~2021-08-17 22:56 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-19 22:13 [PATCH v5 1/3] dt-bindings: pwm: Add Xilinx AXI Timer Sean Anderson
2021-07-19 22:13 ` [PATCH v5 2/3] clocksource: Rewrite Xilinx AXI timer driver Sean Anderson
2021-07-20 21:41 ` kernel test robot
2021-07-20 21:44 ` Sean Anderson
2021-07-21 2:35 ` kernel test robot
2021-07-19 22:13 ` [PATCH v5 3/3] pwm: Add support for Xilinx AXI Timer Sean Anderson
2021-07-21 13:26 ` kernel test robot
2021-07-22 19:18 ` Sean Anderson
2021-08-09 14:48 ` Sean Anderson
2021-08-14 20:47 ` Uwe Kleine-König
2021-08-16 23:51 ` Sean Anderson
2021-08-17 18:04 ` Uwe Kleine-König
2021-08-17 22:56 ` Sean Anderson [this message]
2021-07-20 13:11 ` [PATCH v5 1/3] dt-bindings: pwm: Add " Rob Herring
2021-07-20 21:45 ` Sean Anderson
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=082d39cd-1ce5-344f-3229-3beb7cea0a15@seco.com \
--to=sean.anderson@seco.com \
--cc=alvaro.gamez@hazent.com \
--cc=devicetree@vger.kernel.org \
--cc=lee.jones@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pwm@vger.kernel.org \
--cc=michal.simek@xilinx.com \
--cc=thierry.reding@gmail.com \
--cc=u.kleine-koenig@pengutronix.de \
/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).