Re: [PATCH 2/2 v4] pwm: Add driver for AXI PWM generator

[Uwe Kleine-König]

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@xxxxxxxxxxxx/
> 
> 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: signature.asc
Description: PGP signature