From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Trevor Gamblin <tgamblin@baylibre.com>
Cc: linux-pwm@vger.kernel.org, linux-kernel@vger.kernel.org,
michael.hennerich@analog.com, nuno.sa@analog.com,
devicetree@vger.kernel.org, robh+dt@kernel.org,
krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
dlechner@baylibre.com, Drew Fustini <dfustini@baylibre.com>,
Sergiu Cuciurean <sergiu.cuciurean@analog.com>
Subject: Re: [PATCH 2/2 v4] pwm: Add driver for AXI PWM generator
Date: Thu, 11 Apr 2024 17:37:26 +0200 [thread overview]
Message-ID: <7i44urixaxohcae44gleqm6tvgterwjz7kerbozijxzrau7czd@bl6rkkmpes3u> (raw)
In-Reply-To: <f2579349-2cb0-434a-bae1-493218a62d53@baylibre.com>
[-- Attachment #1: Type: text/plain, Size: 3148 bytes --]
Hello,
On Thu, Apr 11, 2024 at 10:07:54AM -0400, Trevor Gamblin wrote:
> On 2024-04-11 12:59, Uwe Kleine-König wrote:
> > > + * - Writing LOAD_CONFIG also has the effect of re-synchronizing all
> > > + * enabled channels, which could cause glitching on other channels. It
> > > + * is therefore expected that channels are assigned harmonic periods
> > > + * and all have a single user coordinating this.
> > What does "re-synchronize" mean here? Are all counters reset to zero?
> > "harmonic" means that all channels should use the same period length?
> Yes, it means that all counters are reset to zero. Harmonic in this case
> means that channels can have different periods, but they should be integer
> multiples of each other. Should I rewrite the comment to be more explicit?
I hesitate to say "yes, please be more specific" because I think it's
mood. If all pwm lines restart with their counter = 0 as soon as one
line is reconfigured (without completing the current period) being a
multiple of each other doesn't help at all. So I think the right thing
to write there is:
- Reconfiguring a channel doesn't complete the currently running period
and resets the counters of all other channels and so very likely
introduces glitches on these unrelated outputs.
(Even if the period was completed, and only assuming configuration
updates that don't modify the period, all channels that don't have a
period that is a divider of the just configured line (might) glitch. So
if you have one PWM with period = 200 and another with period = 400,
everything is fine if you update the latter, however updating the former
might make the latter glitch. So essentially you need to have a single
period for all channels. That's why I asked if "harmonic" means that all
channels should use the same period.)
> > Reading https://wiki.analog.com/resources/fpga/docs/axi_pwm_gen I would
> > have expected:
> >
> > /* ch in { 0, ... 15 } */
> > #define AXI_PWMGEN_REG_PULSE_X_PERIOD(ch) (0x40 + 4 * (ch))
> > #define AXI_PWMGEN_REG_PULSE_X_WIDTH(ch) (0x80 + 4 * (ch))
> > #define AXI_PWMGEN_REG_PULSE_X_OFFSET (0xc0 + 4 * (ch))
>
> The regmap you find there now reflects v2 of the pwmgen IP; v1 used a step
> of 12 instead of 4. The v2 series sent a little bit later on adds this extra
> support: https://lore.kernel.org/linux-pwm/20240314204722.1291993-1-tgamblin@baylibre.com/
>
> I've added support for both versions since v1 of the IP could still be in
> use on some devices. Would it be better to have the two patch series
> squashed together into a v5 of the axi-pwmgen driver?
Not necessarily squashed, but I suggest to send them in a single series.
(Note, this doesn't mean "Don't squash". I didn't look at the other
series yet, so make a sensible choice yourself (or wait until I come
around reviewing that other series and hope that I remember the context
to comment about this question. :-)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
prev parent reply other threads:[~2024-04-11 15:37 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-01 17:33 [PATCH 0/2 v4] pwm: add axi-pwm-gen driver Trevor Gamblin
2024-03-01 17:33 ` [PATCH 1/2 v4] dt-bindings: pwm: Add AXI PWM generator Trevor Gamblin
2024-04-11 10:32 ` Uwe Kleine-König
2024-04-11 10:38 ` Hennerich, Michael
2024-04-11 10:42 ` Nuno Sá
2024-03-01 17:33 ` [PATCH 2/2 v4] pwm: Add driver for " Trevor Gamblin
2024-04-11 10:59 ` Uwe Kleine-König
2024-04-11 14:07 ` Trevor Gamblin
2024-04-11 14:29 ` Trevor Gamblin
2024-04-11 15:37 ` Uwe Kleine-König [this message]
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=7i44urixaxohcae44gleqm6tvgterwjz7kerbozijxzrau7czd@bl6rkkmpes3u \
--to=u.kleine-koenig@pengutronix.de \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=dfustini@baylibre.com \
--cc=dlechner@baylibre.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pwm@vger.kernel.org \
--cc=michael.hennerich@analog.com \
--cc=nuno.sa@analog.com \
--cc=robh+dt@kernel.org \
--cc=sergiu.cuciurean@analog.com \
--cc=tgamblin@baylibre.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 read-only IMAP folder(s) and NNTP newsgroup(s).