[PATCH 0/2] pwm: axi-pwmgen: always enable FORCE_ALIGN

[PATCH 0/2] pwm: axi-pwmgen: always enable FORCE_ALIGN

[David Lechner]

When using the axi-pwmgen as a trigger for ADCs, we've found that the
default behavior of the PWMGEN IP block is not ideal. The default
behavior is to wait for the period of all PWM outputs to run out before
applying any new settings. But there isn't a way to block until this
happens (and even if there was, it could take a long time). So the
pwm apply function returns before the new settings are actually applied.

This makes certain use cases impossible. For example, to use the PWM
like a GPIO to create a single pulse on and off to trigger a single ADC
conversion.

The AXI PWMGEN has a FORCE_ALIGN configuration option that changes the
behavior so that any new output settings (period, duty cycle, etc.) are
applied immediately. This can cause glitches in the output, but makes
the PWM actually useable for most applications.

Also, there was a naming conflict with register names, so there is a
preliminary cleanup patch to sort that out.

---
David Lechner (2):
      pwm: axi-pwmgen: rename 0x10 register
      pwm: axi-pwmgen: enable FORCE_ALIGN by default

 drivers/pwm/pwm-axi-pwmgen.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)
---
base-commit: ff25451372ee1aa4c4f4401dc96516782a00dd4d
change-id: 20241009-pwm-axi-pwmgen-enable-force_align-cfb403da4612

Best regards,
-- 
David Lechner <dlechner@baylibre.com>

[PATCH 1/2] pwm: axi-pwmgen: rename 0x10 register

[David Lechner]

Rename the 0x10 register from REG_CONFIG to REG_RSTN. Also rename the
associated bit macros accordingly.

While touching this, move the bit macros close to the register address
macro for better organization.

According to [1], the name of the 0x10 register is REG_RSTN, and there
is a different register named REG_CONFIG (0x18). So we should not be
using REG_CONFIG for the 0x10 register to avoid confusion.

[1]: http://analogdevicesinc.github.io/hdl/library/axi_pwm_gen/index.html

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/pwm/pwm-axi-pwmgen.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pwm/pwm-axi-pwmgen.c b/drivers/pwm/pwm-axi-pwmgen.c
index 6e56ceb23d18..e1ddeaa4998b 100644
--- a/drivers/pwm/pwm-axi-pwmgen.c
+++ b/drivers/pwm/pwm-axi-pwmgen.c
@@ -9,7 +9,7 @@
  *
  * Limitations:
  * - The writes to registers for period and duty are shadowed until
- *   LOAD_CONFIG is written to AXI_PWMGEN_REG_CONFIG, at which point
+ *   LOAD_CONFIG is written to AXI_PWMGEN_REG_RSTN, at which point
  *   they take effect.
  * - Writing LOAD_CONFIG also has the effect of re-synchronizing all
  *   enabled channels, which could cause glitching on other channels. It
@@ -33,14 +33,14 @@
 #define AXI_PWMGEN_REG_ID		0x04
 #define AXI_PWMGEN_REG_SCRATCHPAD	0x08
 #define AXI_PWMGEN_REG_CORE_MAGIC	0x0C
-#define AXI_PWMGEN_REG_CONFIG		0x10
+#define AXI_PWMGEN_REG_RSTN		0x10
+#define   AXI_PWMGEN_REG_RSTN_LOAD_CONFIG	BIT(1)
+#define   AXI_PWMGEN_REG_RSTN_RESET		BIT(0)
 #define AXI_PWMGEN_REG_NPWM		0x14
 #define AXI_PWMGEN_CHX_PERIOD(ch)	(0x40 + (4 * (ch)))
 #define AXI_PWMGEN_CHX_DUTY(ch)		(0x80 + (4 * (ch)))
 #define AXI_PWMGEN_CHX_OFFSET(ch)	(0xC0 + (4 * (ch)))
 #define AXI_PWMGEN_REG_CORE_MAGIC_VAL	0x601A3471 /* Identification number to test during setup */
-#define AXI_PWMGEN_LOAD_CONFIG		BIT(1)
-#define AXI_PWMGEN_REG_CONFIG_RESET	BIT(0)
 
 struct axi_pwmgen_ddata {
 	struct regmap *regmap;
@@ -152,7 +152,7 @@ static int axi_pwmgen_write_waveform(struct pwm_chip *chip,
 	if (ret)
 		return ret;
 
-	return regmap_write(regmap, AXI_PWMGEN_REG_CONFIG, AXI_PWMGEN_LOAD_CONFIG);
+	return regmap_write(regmap, AXI_PWMGEN_REG_RSTN, AXI_PWMGEN_REG_RSTN_LOAD_CONFIG);
 }
 
 static int axi_pwmgen_read_waveform(struct pwm_chip *chip,
@@ -223,7 +223,7 @@ static int axi_pwmgen_setup(struct regmap *regmap, struct device *dev)
 	}
 
 	/* Enable the core */
-	ret = regmap_clear_bits(regmap, AXI_PWMGEN_REG_CONFIG, AXI_PWMGEN_REG_CONFIG_RESET);
+	ret = regmap_clear_bits(regmap, AXI_PWMGEN_REG_RSTN, AXI_PWMGEN_REG_RSTN_RESET);
 	if (ret)
 		return ret;
 

-- 
2.43.0

[PATCH 2/2] pwm: axi-pwmgen: enable FORCE_ALIGN by default

[David Lechner]

Enable the FORCE_ALIGN flag by default in the AXI PWMGEN driver. This
flag makes the behavior of the PWM output consistent with the
description at the top of the driver file.

    * Limitations:
    * - The writes to registers for period and duty are shadowed until
    *   LOAD_CONFIG is written to AXI_PWMGEN_REG_RSTN, at which point
    *   they take effect.
    * - 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.

Without this flag, the PWM output does not change until the period of
all PWM output channels has run out, which makes the PWM impossible to
use in some cases because it takes too long to change the output.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/pwm/pwm-axi-pwmgen.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/pwm/pwm-axi-pwmgen.c b/drivers/pwm/pwm-axi-pwmgen.c
index e1ddeaa4998b..4259a0db9ff4 100644
--- a/drivers/pwm/pwm-axi-pwmgen.c
+++ b/drivers/pwm/pwm-axi-pwmgen.c
@@ -37,6 +37,8 @@
 #define   AXI_PWMGEN_REG_RSTN_LOAD_CONFIG	BIT(1)
 #define   AXI_PWMGEN_REG_RSTN_RESET		BIT(0)
 #define AXI_PWMGEN_REG_NPWM		0x14
+#define AXI_PWMGEN_REG_CONFIG		0x18
+#define   AXI_PWMGEN_REG_CONFIG_FORCE_ALIGN	BIT(1)
 #define AXI_PWMGEN_CHX_PERIOD(ch)	(0x40 + (4 * (ch)))
 #define AXI_PWMGEN_CHX_DUTY(ch)		(0x80 + (4 * (ch)))
 #define AXI_PWMGEN_CHX_OFFSET(ch)	(0xC0 + (4 * (ch)))
@@ -227,6 +229,16 @@ static int axi_pwmgen_setup(struct regmap *regmap, struct device *dev)
 	if (ret)
 		return ret;
 
+	/*
+	 * Enable force align so that changes to PWM period and duty cycle take
+	 * effect immediately. Otherwise, the effect of the change is delayed
+	 * until the period of all channels run out, which can be long after the
+	 * apply function returns.
+	 */
+	ret = regmap_set_bits(regmap, AXI_PWMGEN_REG_CONFIG, AXI_PWMGEN_REG_CONFIG_FORCE_ALIGN);
+	if (ret)
+		return ret;
+
 	ret = regmap_read(regmap, AXI_PWMGEN_REG_NPWM, &val);
 	if (ret)
 		return ret;

-- 
2.43.0

Re: [PATCH 0/2] pwm: axi-pwmgen: always enable FORCE_ALIGN

[Nuno Sá]

On Wed, 2024-10-09 at 16:11 -0500, David Lechner wrote:
> When using the axi-pwmgen as a trigger for ADCs, we've found that the
> default behavior of the PWMGEN IP block is not ideal. The default
> behavior is to wait for the period of all PWM outputs to run out before
> applying any new settings. But there isn't a way to block until this
> happens (and even if there was, it could take a long time). So the
> pwm apply function returns before the new settings are actually applied.
> 
> This makes certain use cases impossible. For example, to use the PWM
> like a GPIO to create a single pulse on and off to trigger a single ADC
> conversion.
> 
> The AXI PWMGEN has a FORCE_ALIGN configuration option that changes the
> behavior so that any new output settings (period, duty cycle, etc.) are
> applied immediately. This can cause glitches in the output, but makes
> the PWM actually useable for most applications.
> 
> Also, there was a naming conflict with register names, so there is a
> preliminary cleanup patch to sort that out.
> 
> ---

Reviewed-by: Nuno Sa <nuno.sa@analog.com>

> David Lechner (2):
>       pwm: axi-pwmgen: rename 0x10 register
>       pwm: axi-pwmgen: enable FORCE_ALIGN by default
> 
>  drivers/pwm/pwm-axi-pwmgen.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> ---
> base-commit: ff25451372ee1aa4c4f4401dc96516782a00dd4d
> change-id: 20241009-pwm-axi-pwmgen-enable-force_align-cfb403da4612
> 
> Best regards,

Re: [PATCH 0/2] pwm: axi-pwmgen: always enable FORCE_ALIGN

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1531 bytes --]

On Thu, Oct 10, 2024 at 10:33:20AM +0200, Nuno Sá wrote:
> On Wed, 2024-10-09 at 16:11 -0500, David Lechner wrote:
> > When using the axi-pwmgen as a trigger for ADCs, we've found that the
> > default behavior of the PWMGEN IP block is not ideal. The default
> > behavior is to wait for the period of all PWM outputs to run out before
> > applying any new settings. But there isn't a way to block until this
> > happens (and even if there was, it could take a long time). So the
> > pwm apply function returns before the new settings are actually applied.
> > 
> > This makes certain use cases impossible. For example, to use the PWM
> > like a GPIO to create a single pulse on and off to trigger a single ADC
> > conversion.
> > 
> > The AXI PWMGEN has a FORCE_ALIGN configuration option that changes the
> > behavior so that any new output settings (period, duty cycle, etc.) are
> > applied immediately. This can cause glitches in the output, but makes
> > the PWM actually useable for most applications.
> > 
> > Also, there was a naming conflict with register names, so there is a
> > preliminary cleanup patch to sort that out.
> > 
> > ---
> 
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>

b4 diagnoses for that:
NOTE: some trailers ignored due to from/email mismatches:
    ! Trailer: Reviewed-by: Nuno Sa <nuno.sa@analog.com>
     Msg From: Nuno Sá <noname.nuno@gmail.com>

I fixed that manually now, but would be nice if you'd care for matching
addresses in the future.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/2] pwm: axi-pwmgen: always enable FORCE_ALIGN

[Nuno Sá]

On Fri, 2024-10-11 at 11:51 +0200, Uwe Kleine-König wrote:
> On Thu, Oct 10, 2024 at 10:33:20AM +0200, Nuno Sá wrote:
> > On Wed, 2024-10-09 at 16:11 -0500, David Lechner wrote:
> > > When using the axi-pwmgen as a trigger for ADCs, we've found that the
> > > default behavior of the PWMGEN IP block is not ideal. The default
> > > behavior is to wait for the period of all PWM outputs to run out before
> > > applying any new settings. But there isn't a way to block until this
> > > happens (and even if there was, it could take a long time). So the
> > > pwm apply function returns before the new settings are actually applied.
> > > 
> > > This makes certain use cases impossible. For example, to use the PWM
> > > like a GPIO to create a single pulse on and off to trigger a single ADC
> > > conversion.
> > > 
> > > The AXI PWMGEN has a FORCE_ALIGN configuration option that changes the
> > > behavior so that any new output settings (period, duty cycle, etc.) are
> > > applied immediately. This can cause glitches in the output, but makes
> > > the PWM actually useable for most applications.
> > > 
> > > Also, there was a naming conflict with register names, so there is a
> > > preliminary cleanup patch to sort that out.
> > > 
> > > ---
> > 
> > Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> 
> b4 diagnoses for that:
> NOTE: some trailers ignored due to from/email mismatches:
>     ! Trailer: Reviewed-by: Nuno Sa <nuno.sa@analog.com>
>      Msg From: Nuno Sá <noname.nuno@gmail.com>
> 
> I fixed that manually now, but would be nice if you'd care for matching
> addresses in the future.
> 
> 

Arghh,

Completely forgot... As i said in the first time, I'm doing this all the time
and never got a complain about it. I'll try to remember for pwm to reply from my
work email client (which I want to avoid for mailing lists as you might guess
the client I'm speaking about).

- Nuno Sá

Re: [PATCH 0/2] pwm: axi-pwmgen: always enable FORCE_ALIGN

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1131 bytes --]

Hello David,

On Wed, Oct 09, 2024 at 04:11:48PM -0500, David Lechner wrote:
> When using the axi-pwmgen as a trigger for ADCs, we've found that the
> default behavior of the PWMGEN IP block is not ideal. The default
> behavior is to wait for the period of all PWM outputs to run out before
> applying any new settings. But there isn't a way to block until this
> happens (and even if there was, it could take a long time). So the
> pwm apply function returns before the new settings are actually applied.
> 
> This makes certain use cases impossible. For example, to use the PWM
> like a GPIO to create a single pulse on and off to trigger a single ADC
> conversion.
> 
> The AXI PWMGEN has a FORCE_ALIGN configuration option that changes the
> behavior so that any new output settings (period, duty cycle, etc.) are
> applied immediately. This can cause glitches in the output, but makes
> the PWM actually useable for most applications.
> 
> Also, there was a naming conflict with register names, so there is a
> preliminary cleanup patch to sort that out.

Applied with Nuno's review-tag.

Thanks
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 484 bytes --]