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
