[PATCH v2 0/3] Improve pwm-ir-tx precision

[PATCH v2 0/3] Improve pwm-ir-tx precision

[Sean Young]

The pwm-ir-tx driver has to turn the pwm signal on and off, and suffers
from delays as this is done in process context. Make this work in atomic
context.

changes:

v2:
 - Simplified the pwm ir tx based on Ivaylo Dimitrov suggestions
 - Renamed pwm_can_sleep() to pwm_is_atomic
 - Fix sleeping issue in pwm-bcm2835 (clk_get_rate() can sleep)
 - use non_block_start() in pwm to check atomic-ness

Sean Young (3):
  pwm: make it possible to apply pwm changes in atomic context
  pwm: bcm2835: allow pwm driver to be used in atomic context
  media: pwm-ir-tx: trigger edges from hrtimer interrupt context

 drivers/media/rc/pwm-ir-tx.c  | 79 +++++++++++++++++++++++++++++++++--
 drivers/pwm/core.c            | 62 ++++++++++++++++++++++-----
 drivers/pwm/pwm-bcm2835.c     | 21 ++++++----
 drivers/pwm/pwm-renesas-tpu.c |  1 -
 include/linux/pwm.h           | 29 ++++++++++++-
 5 files changed, 166 insertions(+), 26 deletions(-)

-- 
2.42.0

[PATCH v2 1/3] pwm: make it possible to apply pwm changes in atomic context

[Sean Young]

Some drivers require sleeping, for example if the pwm device is connected
over i2c. The pwm-ir-tx requires precise timing, and sleeping causes havoc
with the generated IR signal when sleeping occurs.

This patch makes it possible to use pwm when the driver does not sleep,
by introducing the pwm_can_sleep() function.

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/pwm/core.c            | 62 ++++++++++++++++++++++++++++-------
 drivers/pwm/pwm-renesas-tpu.c |  1 -
 include/linux/pwm.h           | 29 ++++++++++++++--
 3 files changed, 78 insertions(+), 14 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index dc66e3405bf5..241510ba1823 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -489,24 +489,15 @@ static void pwm_apply_state_debug(struct pwm_device *pwm,
 }
 
 /**
- * pwm_apply_state() - atomically apply a new state to a PWM device
+ * pwm_apply_state_unchecked() - atomically apply a new state to a PWM device
  * @pwm: PWM device
  * @state: new state to apply
  */
-int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
+static int pwm_apply_state_unchecked(struct pwm_device *pwm, const struct pwm_state *state)
 {
 	struct pwm_chip *chip;
 	int err;
 
-	/*
-	 * Some lowlevel driver's implementations of .apply() make use of
-	 * mutexes, also with some drivers only returning when the new
-	 * configuration is active calling pwm_apply_state() from atomic context
-	 * is a bad idea. So make it explicit that calling this function might
-	 * sleep.
-	 */
-	might_sleep();
-
 	if (!pwm || !state || !state->period ||
 	    state->duty_cycle > state->period)
 		return -EINVAL;
@@ -535,8 +526,57 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
 
 	return 0;
 }
+
+/**
+ * pwm_apply_state() - atomically apply a new state to a PWM device
+ * Cannot be used in atomic context.
+ * @pwm: PWM device
+ * @state: new state to apply
+ */
+int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
+{
+	/*
+	 * Some lowlevel driver's implementations of .apply() make use of
+	 * mutexes, also with some drivers only returning when the new
+	 * configuration is active calling pwm_apply_state() from atomic context
+	 * is a bad idea. So make it explicit that calling this function might
+	 * sleep.
+	 */
+	might_sleep();
+
+	if (IS_ENABLED(CONFIG_PWM_DEBUG) && pwm->chip->ops->atomic) {
+		int err;
+
+		/*
+		 * Catch any sleeping drivers when atomic is set.
+		 */
+		non_block_start();
+		err = pwm_apply_state_unchecked(pwm, state);
+		non_block_end();
+
+		return err;
+	}
+
+	return pwm_apply_state_unchecked(pwm, state);
+}
 EXPORT_SYMBOL_GPL(pwm_apply_state);
 
+/**
+ * pwm_apply_state_atomic() - atomically apply a new state to a PWM device
+ * Can be used from atomic context.
+ * @pwm: PWM device
+ * @state: new state to apply
+ */
+int pwm_apply_state_atomic(struct pwm_device *pwm,
+			   const struct pwm_state *state)
+{
+	WARN_ONCE(!pwm->chip->ops->atomic,
+		  "sleeping pwm driver used in atomic context");
+
+	return pwm_apply_state_unchecked(pwm, state);
+}
+EXPORT_SYMBOL_GPL(pwm_apply_state_atomic);
+
 /**
  * pwm_capture() - capture and report a PWM signal
  * @pwm: PWM device
diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c
index d7311614c846..96797a33d8c6 100644
--- a/drivers/pwm/pwm-renesas-tpu.c
+++ b/drivers/pwm/pwm-renesas-tpu.c
@@ -11,7 +11,6 @@
 #include <linux/init.h>
 #include <linux/ioport.h>
 #include <linux/module.h>
-#include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index d2f9f690a9c1..93f166ab03c1 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -267,6 +267,7 @@ struct pwm_capture {
  * @get_state: get the current PWM state. This function is only
  *	       called once per PWM device when the PWM chip is
  *	       registered.
+ * @atomic: can the driver execute pwm_apply_state in atomic context
  * @owner: helps prevent removal of modules exporting active PWMs
  */
 struct pwm_ops {
@@ -278,6 +279,7 @@ struct pwm_ops {
 		     const struct pwm_state *state);
 	int (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm,
 			 struct pwm_state *state);
+	bool atomic;
 	struct module *owner;
 };
 
@@ -310,6 +312,7 @@ struct pwm_chip {
 #if IS_ENABLED(CONFIG_PWM)
 /* PWM user APIs */
 int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state);
+int pwm_apply_state_atomic(struct pwm_device *pwm, const struct pwm_state *state);
 int pwm_adjust_config(struct pwm_device *pwm);
 
 /**
@@ -380,6 +383,17 @@ static inline void pwm_disable(struct pwm_device *pwm)
 	pwm_apply_state(pwm, &state);
 }
 
+/**
+ * pwm_is_atomic() - is pwm_apply_state_atomic() supported?
+ * @pwm: PWM device
+ *
+ * Returns: true pwm_apply_state_atomic() can be called from atomic context.
+ */
+static inline bool pwm_is_atomic(struct pwm_device *pwm)
+{
+	return pwm->chip->ops->atomic;
+}
+
 /* PWM provider APIs */
 int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result,
 		unsigned long timeout);
@@ -408,16 +422,27 @@ struct pwm_device *devm_fwnode_pwm_get(struct device *dev,
 				       struct fwnode_handle *fwnode,
 				       const char *con_id);
 #else
+static inline bool pwm_is_atomic(struct pwm_device *pwm)
+{
+	return false;
+}
+
 static inline int pwm_apply_state(struct pwm_device *pwm,
 				  const struct pwm_state *state)
 {
 	might_sleep();
-	return -ENOTSUPP;
+	return -EOPNOTSUPP;
+}
+
+static inline int pwm_apply_state_atomic(struct pwm_device *pwm,
+					 const struct pwm_state *state)
+{
+	return -EOPNOTSUPP;
 }
 
 static inline int pwm_adjust_config(struct pwm_device *pwm)
 {
-	return -ENOTSUPP;
+	return -EOPNOTSUPP;
 }
 
 static inline int pwm_config(struct pwm_device *pwm, int duty_ns,
-- 
2.42.0

Re: [PATCH v2 1/3] pwm: make it possible to apply pwm changes in atomic context

[Thierry Reding]

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

On Fri, Oct 13, 2023 at 11:46:14AM +0100, Sean Young wrote:
[...]
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index d2f9f690a9c1..93f166ab03c1 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -267,6 +267,7 @@ struct pwm_capture {
>   * @get_state: get the current PWM state. This function is only
>   *	       called once per PWM device when the PWM chip is
>   *	       registered.
> + * @atomic: can the driver execute pwm_apply_state in atomic context
>   * @owner: helps prevent removal of modules exporting active PWMs
>   */
>  struct pwm_ops {
> @@ -278,6 +279,7 @@ struct pwm_ops {
>  		     const struct pwm_state *state);
>  	int (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm,
>  			 struct pwm_state *state);
> +	bool atomic;
>  	struct module *owner;
>  };

As I mentioned earlier, this really belongs in struct pwm_chip rather
than struct pwm_ops. I know that Uwe said this is unlikely to happen,
and that may be true, but at the same time it's not like I'm asking
much. Whether you put this in struct pwm_ops or struct pwm_chip is
about the same amount of code, and putting it into pwm_chip is much
more flexible, so it's really a no-brainer.

Thierry

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

Re: [PATCH v2 1/3] pwm: make it possible to apply pwm changes in atomic context

[Sean Young]

On Fri, Oct 13, 2023 at 01:51:40PM +0200, Thierry Reding wrote:
> On Fri, Oct 13, 2023 at 11:46:14AM +0100, Sean Young wrote:
> [...]
> > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > index d2f9f690a9c1..93f166ab03c1 100644
> > --- a/include/linux/pwm.h
> > +++ b/include/linux/pwm.h
> > @@ -267,6 +267,7 @@ struct pwm_capture {
> >   * @get_state: get the current PWM state. This function is only
> >   *	       called once per PWM device when the PWM chip is
> >   *	       registered.
> > + * @atomic: can the driver execute pwm_apply_state in atomic context
> >   * @owner: helps prevent removal of modules exporting active PWMs
> >   */
> >  struct pwm_ops {
> > @@ -278,6 +279,7 @@ struct pwm_ops {
> >  		     const struct pwm_state *state);
> >  	int (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm,
> >  			 struct pwm_state *state);
> > +	bool atomic;
> >  	struct module *owner;
> >  };
> 
> As I mentioned earlier, this really belongs in struct pwm_chip rather
> than struct pwm_ops. I know that Uwe said this is unlikely to happen,
> and that may be true, but at the same time it's not like I'm asking
> much. Whether you put this in struct pwm_ops or struct pwm_chip is
> about the same amount of code, and putting it into pwm_chip is much
> more flexible, so it's really a no-brainer.

Happy to change this of course. I changed it and then changed it back after
Uwe's comment, I'll fix this in the next version.

One tiny advantage is that pwm_ops is static const while pwm_chip is
allocated per-pwm, so will need instructions for setting the value. Having
said that, the difference is tiny, it's a single bool.


Sean

Re: [PATCH v2 1/3] pwm: make it possible to apply pwm changes in atomic context

[Thierry Reding]

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

On Fri, Oct 13, 2023 at 03:58:30PM +0100, Sean Young wrote:
> On Fri, Oct 13, 2023 at 01:51:40PM +0200, Thierry Reding wrote:
> > On Fri, Oct 13, 2023 at 11:46:14AM +0100, Sean Young wrote:
> > [...]
> > > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > > index d2f9f690a9c1..93f166ab03c1 100644
> > > --- a/include/linux/pwm.h
> > > +++ b/include/linux/pwm.h
> > > @@ -267,6 +267,7 @@ struct pwm_capture {
> > >   * @get_state: get the current PWM state. This function is only
> > >   *	       called once per PWM device when the PWM chip is
> > >   *	       registered.
> > > + * @atomic: can the driver execute pwm_apply_state in atomic context
> > >   * @owner: helps prevent removal of modules exporting active PWMs
> > >   */
> > >  struct pwm_ops {
> > > @@ -278,6 +279,7 @@ struct pwm_ops {
> > >  		     const struct pwm_state *state);
> > >  	int (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm,
> > >  			 struct pwm_state *state);
> > > +	bool atomic;
> > >  	struct module *owner;
> > >  };
> > 
> > As I mentioned earlier, this really belongs in struct pwm_chip rather
> > than struct pwm_ops. I know that Uwe said this is unlikely to happen,
> > and that may be true, but at the same time it's not like I'm asking
> > much. Whether you put this in struct pwm_ops or struct pwm_chip is
> > about the same amount of code, and putting it into pwm_chip is much
> > more flexible, so it's really a no-brainer.
> 
> Happy to change this of course. I changed it and then changed it back after
> Uwe's comment, I'll fix this in the next version.
> 
> One tiny advantage is that pwm_ops is static const while pwm_chip is
> allocated per-pwm, so will need instructions for setting the value. Having
> said that, the difference is tiny, it's a single bool.

Yeah, it's typically a single assignment, so from a code point of view
it should be pretty much the same. I suppose from an instruction level
point of view, yes, this might add a teeny-tiny bit of overhead.

On the other hand it lets us do interesting things like initialize
chip->atomic = !regmap_might_sleep() for those drivers that use regmap
and then not worry about it any longer.

Given that, I'm also wondering if we should try to keep the terminology
a bit more consistent. "Atomic" is somewhat overloaded because ->apply()
and ->get_state() are part of the "atomic" PWM API (in the sense that
applying changes are done as a single, atomic operation, rather than in
the sense of "non-sleeping" operation).

So pwm_apply_state_atomic() is then doubly atomic, which is a bit weird.
On the other hand it's a bit tedious to convert all existing users to
pwm_apply_state_might_sleep().

Perhaps as a compromise we can add pwm_apply_state_might_sleep() and
make pwm_apply_state() a (deprecated) alias for that, so that existing
drivers can be converted one by one.

Eventually we would then end up with both pwm_apply_state_might_sleep()
and pwm_apply_state_atomic(), which has the nice side-effect of these
being unambiguous.

That doesn't get rid of the ambiguity of that _atomic() suffix, but I
can probably live with that one. It's used for this same meaning in
other contexts and if we add a _might_sleep() variant it becomes clearer
how the two are different.

Anyway, the bottom line is that I'd prefer the "atomic" field to be
renamed "might_sleep". It'd also be nice to add the new _might_sleep()
variant since you're already changing all of this anyway. No need to
mass-convert all the drivers to the _might_sleep() variant yet, though,
since we can have a transitional alias for that.

Of course feel free to give it a shot if you feel like it.

Thierry

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

Re: [PATCH v2 1/3] pwm: make it possible to apply pwm changes in atomic context

[Uwe Kleine-König]

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

Hello,

On Fri, Oct 13, 2023 at 05:34:34PM +0200, Thierry Reding wrote:
> On Fri, Oct 13, 2023 at 03:58:30PM +0100, Sean Young wrote:
> > On Fri, Oct 13, 2023 at 01:51:40PM +0200, Thierry Reding wrote:
> > > On Fri, Oct 13, 2023 at 11:46:14AM +0100, Sean Young wrote:
> > > [...]
> > > > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > > > index d2f9f690a9c1..93f166ab03c1 100644
> > > > --- a/include/linux/pwm.h
> > > > +++ b/include/linux/pwm.h
> > > > @@ -267,6 +267,7 @@ struct pwm_capture {
> > > >   * @get_state: get the current PWM state. This function is only
> > > >   *	       called once per PWM device when the PWM chip is
> > > >   *	       registered.
> > > > + * @atomic: can the driver execute pwm_apply_state in atomic context
> > > >   * @owner: helps prevent removal of modules exporting active PWMs
> > > >   */
> > > >  struct pwm_ops {
> > > > @@ -278,6 +279,7 @@ struct pwm_ops {
> > > >  		     const struct pwm_state *state);
> > > >  	int (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm,
> > > >  			 struct pwm_state *state);
> > > > +	bool atomic;
> > > >  	struct module *owner;
> > > >  };
> > > 
> > > As I mentioned earlier, this really belongs in struct pwm_chip rather
> > > than struct pwm_ops. I know that Uwe said this is unlikely to happen,
> > > and that may be true, but at the same time it's not like I'm asking
> > > much. Whether you put this in struct pwm_ops or struct pwm_chip is
> > > about the same amount of code, and putting it into pwm_chip is much
> > > more flexible, so it's really a no-brainer.
> > 
> > Happy to change this of course. I changed it and then changed it back after
> > Uwe's comment, I'll fix this in the next version.
> > 
> > One tiny advantage is that pwm_ops is static const while pwm_chip is
> > allocated per-pwm, so will need instructions for setting the value. Having
> > said that, the difference is tiny, it's a single bool.
> 
> Yeah, it's typically a single assignment, so from a code point of view
> it should be pretty much the same. I suppose from an instruction level
> point of view, yes, this might add a teeny-tiny bit of overhead.
> 
> On the other hand it lets us do interesting things like initialize
> chip->atomic = !regmap_might_sleep() for those drivers that use regmap
> and then not worry about it any longer.
> 
> Given that, I'm also wondering if we should try to keep the terminology
> a bit more consistent. "Atomic" is somewhat overloaded because ->apply()
> and ->get_state() are part of the "atomic" PWM API (in the sense that
> applying changes are done as a single, atomic operation, rather than in
> the sense of "non-sleeping" operation).
> 
> So pwm_apply_state_atomic() is then doubly atomic, which is a bit weird.
> On the other hand it's a bit tedious to convert all existing users to
> pwm_apply_state_might_sleep().
> 
> Perhaps as a compromise we can add pwm_apply_state_might_sleep() and
> make pwm_apply_state() a (deprecated) alias for that, so that existing
> drivers can be converted one by one.

To throw in my green for our bike shed: I'd pick

	pwm_apply_state_cansleep()

to match what gpio does (with gpiod_set_value_cansleep()). (Though I
have to admit that semantically Thierry's might_sleep is nicer as it
matches might_sleep().)

If we don't want to have an explicit indicator for the atomic/fast
variant (again similar to the gpio framework), maybe we can drop
"_state" which I think is somehow redundant and go for:

	pwm_apply (fast)
	pwm_apply_cansleep (sleeping)
	pwm_apply_state (compat alias for pwm_apply_cansleep())

(maybe replace cansleep with might_sleep). Similar for pwm_get_state()
we could use the opportunity and make

	pwm_get()

actually call ->get_state() and introduce

	pwm_get_lastapplied()

with the semantic of todays pwm_get_state(). Do we need a
pwm_get_cansleep/might_sleep()?

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 --]

Re: [PATCH v2 1/3] pwm: make it possible to apply pwm changes in atomic context

[Sean Young]

Hello,

On Fri, Oct 13, 2023 at 08:04:49PM +0200, Uwe Kleine-König wrote:
> On Fri, Oct 13, 2023 at 05:34:34PM +0200, Thierry Reding wrote:
> > On Fri, Oct 13, 2023 at 03:58:30PM +0100, Sean Young wrote:
> > > On Fri, Oct 13, 2023 at 01:51:40PM +0200, Thierry Reding wrote:
> > > > On Fri, Oct 13, 2023 at 11:46:14AM +0100, Sean Young wrote:
> > > > [...]
> > > > > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > > > > index d2f9f690a9c1..93f166ab03c1 100644
> > > > > --- a/include/linux/pwm.h
> > > > > +++ b/include/linux/pwm.h
> > > > > @@ -267,6 +267,7 @@ struct pwm_capture {
> > > > >   * @get_state: get the current PWM state. This function is only
> > > > >   *	       called once per PWM device when the PWM chip is
> > > > >   *	       registered.
> > > > > + * @atomic: can the driver execute pwm_apply_state in atomic context
> > > > >   * @owner: helps prevent removal of modules exporting active PWMs
> > > > >   */
> > > > >  struct pwm_ops {
> > > > > @@ -278,6 +279,7 @@ struct pwm_ops {
> > > > >  		     const struct pwm_state *state);
> > > > >  	int (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > >  			 struct pwm_state *state);
> > > > > +	bool atomic;
> > > > >  	struct module *owner;
> > > > >  };
> > > > 
> > > > As I mentioned earlier, this really belongs in struct pwm_chip rather
> > > > than struct pwm_ops. I know that Uwe said this is unlikely to happen,
> > > > and that may be true, but at the same time it's not like I'm asking
> > > > much. Whether you put this in struct pwm_ops or struct pwm_chip is
> > > > about the same amount of code, and putting it into pwm_chip is much
> > > > more flexible, so it's really a no-brainer.
> > > 
> > > Happy to change this of course. I changed it and then changed it back after
> > > Uwe's comment, I'll fix this in the next version.
> > > 
> > > One tiny advantage is that pwm_ops is static const while pwm_chip is
> > > allocated per-pwm, so will need instructions for setting the value. Having
> > > said that, the difference is tiny, it's a single bool.
> > 
> > Yeah, it's typically a single assignment, so from a code point of view
> > it should be pretty much the same. I suppose from an instruction level
> > point of view, yes, this might add a teeny-tiny bit of overhead.
> > 
> > On the other hand it lets us do interesting things like initialize
> > chip->atomic = !regmap_might_sleep() for those drivers that use regmap
> > and then not worry about it any longer.

Sure.

> > Given that, I'm also wondering if we should try to keep the terminology
> > a bit more consistent. "Atomic" is somewhat overloaded because ->apply()
> > and ->get_state() are part of the "atomic" PWM API (in the sense that
> > applying changes are done as a single, atomic operation, rather than in
> > the sense of "non-sleeping" operation).

Good point.

> > So pwm_apply_state_atomic() is then doubly atomic, which is a bit weird.
> > On the other hand it's a bit tedious to convert all existing users to
> > pwm_apply_state_might_sleep().
> > 
> > Perhaps as a compromise we can add pwm_apply_state_might_sleep() and
> > make pwm_apply_state() a (deprecated) alias for that, so that existing
> > drivers can be converted one by one.
> 
> To throw in my green for our bike shed: I'd pick
> 
> 	pwm_apply_state_cansleep()
> 
> to match what gpio does (with gpiod_set_value_cansleep()). (Though I
> have to admit that semantically Thierry's might_sleep is nicer as it
> matches might_sleep().)

I have to agree here. "can" is shorter and clearer than "might", since
"can" expresses capability. Having said that the mixture of nomenclature is
not great, so there is very little between them.

> If we don't want to have an explicit indicator for the atomic/fast
> variant (again similar to the gpio framework), maybe we can drop
> "_state" which I think is somehow redundant and go for:
> 
> 	pwm_apply (fast)
> 	pwm_apply_cansleep (sleeping)
> 	pwm_apply_state (compat alias for pwm_apply_cansleep())
> 
> (maybe replace cansleep with might_sleep).

This is very nice. :) There are callsites in 15 files, might as well rename
it and do away with pwm_apply_state()

> Similar for pwm_get_state()
> we could use the opportunity and make
> 
> 	pwm_get()
> 
> actually call ->get_state() and introduce
> 
> 	pwm_get_lastapplied()
> 
> with the semantic of todays pwm_get_state(). Do we need a
> pwm_get_cansleep/might_sleep()?

Not all drivers implement .get_state(), how would we handle those?


Thanks,

Sean

[PATCH v2 2/3] pwm: bcm2835: allow pwm driver to be used in atomic context

[Sean Young]

clk_get_rate() may do a mutex lock. Since the clock rate cannot change on
an rpi, simply fetch it once.

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/pwm/pwm-bcm2835.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c
index bdfc2a5ec0d6..59ea154dd657 100644
--- a/drivers/pwm/pwm-bcm2835.c
+++ b/drivers/pwm/pwm-bcm2835.c
@@ -28,6 +28,7 @@ struct bcm2835_pwm {
 	struct device *dev;
 	void __iomem *base;
 	struct clk *clk;
+	unsigned long rate;
 };
 
 static inline struct bcm2835_pwm *to_bcm2835_pwm(struct pwm_chip *chip)
@@ -63,17 +64,11 @@ static int bcm2835_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 {
 
 	struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
-	unsigned long rate = clk_get_rate(pc->clk);
 	unsigned long long period_cycles;
 	u64 max_period;
 
 	u32 val;
 
-	if (!rate) {
-		dev_err(pc->dev, "failed to get clock rate\n");
-		return -EINVAL;
-	}
-
 	/*
 	 * period_cycles must be a 32 bit value, so period * rate / NSEC_PER_SEC
 	 * must be <= U32_MAX. As U32_MAX * NSEC_PER_SEC < U64_MAX the
@@ -88,13 +83,13 @@ static int bcm2835_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * <=> period < ((U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC/2) / rate
 	 * <=> period <= ceil((U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC/2) / rate) - 1
 	 */
-	max_period = DIV_ROUND_UP_ULL((u64)U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC / 2, rate) - 1;
+	max_period = DIV_ROUND_UP_ULL((u64)U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC / 2, pc->rate) - 1;
 
 	if (state->period > max_period)
 		return -EINVAL;
 
 	/* set period */
-	period_cycles = DIV_ROUND_CLOSEST_ULL(state->period * rate, NSEC_PER_SEC);
+	period_cycles = DIV_ROUND_CLOSEST_ULL(state->period * pc->rate, NSEC_PER_SEC);
 
 	/* don't accept a period that is too small */
 	if (period_cycles < PERIOD_MIN)
@@ -103,7 +98,7 @@ static int bcm2835_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	writel(period_cycles, pc->base + PERIOD(pwm->hwpwm));
 
 	/* set duty cycle */
-	val = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * rate, NSEC_PER_SEC);
+	val = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * pc->rate, NSEC_PER_SEC);
 	writel(val, pc->base + DUTY(pwm->hwpwm));
 
 	/* set polarity */
@@ -129,6 +124,7 @@ static const struct pwm_ops bcm2835_pwm_ops = {
 	.request = bcm2835_pwm_request,
 	.free = bcm2835_pwm_free,
 	.apply = bcm2835_pwm_apply,
+	.atomic = true,
 	.owner = THIS_MODULE,
 };
 
@@ -156,6 +152,13 @@ static int bcm2835_pwm_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	pc->rate = clk_get_rate(pc->clk);
+	if (!pc->rate) {
+		dev_err(pc->dev, "failed to get clock rate\n");
+		ret = -EINVAL;
+		goto add_fail;
+	}
+
 	pc->chip.dev = &pdev->dev;
 	pc->chip.ops = &bcm2835_pwm_ops;
 	pc->chip.npwm = 2;
-- 
2.42.0

Re: [PATCH v2 2/3] pwm: bcm2835: allow pwm driver to be used in atomic context

[Stefan Wahren]

Hi Sean,

Am 13.10.23 um 12:46 schrieb Sean Young:
> clk_get_rate() may do a mutex lock. Since the clock rate cannot change on
> an rpi, simply fetch it once.
does it mean you checked all possible SoCs (BCM2835, BCM2836, BCM2837,
BCM2711, BCM2712) for this change?

Is it impossible that the real clock can never be influenced by turbo
mode like SPI?

Best regards
>
> Signed-off-by: Sean Young <sean@mess.org>
> ---
>   drivers/pwm/pwm-bcm2835.c | 21 ++++++++++++---------
>   1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c
> index bdfc2a5ec0d6..59ea154dd657 100644
> --- a/drivers/pwm/pwm-bcm2835.c
> +++ b/drivers/pwm/pwm-bcm2835.c
> @@ -28,6 +28,7 @@ struct bcm2835_pwm {
>   	struct device *dev;
>   	void __iomem *base;
>   	struct clk *clk;
> +	unsigned long rate;
>   };
>
>   static inline struct bcm2835_pwm *to_bcm2835_pwm(struct pwm_chip *chip)
> @@ -63,17 +64,11 @@ static int bcm2835_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>   {
>
>   	struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
> -	unsigned long rate = clk_get_rate(pc->clk);
>   	unsigned long long period_cycles;
>   	u64 max_period;
>
>   	u32 val;
>
> -	if (!rate) {
> -		dev_err(pc->dev, "failed to get clock rate\n");
> -		return -EINVAL;
> -	}
> -
>   	/*
>   	 * period_cycles must be a 32 bit value, so period * rate / NSEC_PER_SEC
>   	 * must be <= U32_MAX. As U32_MAX * NSEC_PER_SEC < U64_MAX the
> @@ -88,13 +83,13 @@ static int bcm2835_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>   	 * <=> period < ((U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC/2) / rate
>   	 * <=> period <= ceil((U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC/2) / rate) - 1
>   	 */
> -	max_period = DIV_ROUND_UP_ULL((u64)U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC / 2, rate) - 1;
> +	max_period = DIV_ROUND_UP_ULL((u64)U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC / 2, pc->rate) - 1;
>
>   	if (state->period > max_period)
>   		return -EINVAL;
>
>   	/* set period */
> -	period_cycles = DIV_ROUND_CLOSEST_ULL(state->period * rate, NSEC_PER_SEC);
> +	period_cycles = DIV_ROUND_CLOSEST_ULL(state->period * pc->rate, NSEC_PER_SEC);
>
>   	/* don't accept a period that is too small */
>   	if (period_cycles < PERIOD_MIN)
> @@ -103,7 +98,7 @@ static int bcm2835_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>   	writel(period_cycles, pc->base + PERIOD(pwm->hwpwm));
>
>   	/* set duty cycle */
> -	val = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * rate, NSEC_PER_SEC);
> +	val = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * pc->rate, NSEC_PER_SEC);
>   	writel(val, pc->base + DUTY(pwm->hwpwm));
>
>   	/* set polarity */
> @@ -129,6 +124,7 @@ static const struct pwm_ops bcm2835_pwm_ops = {
>   	.request = bcm2835_pwm_request,
>   	.free = bcm2835_pwm_free,
>   	.apply = bcm2835_pwm_apply,
> +	.atomic = true,
>   	.owner = THIS_MODULE,
>   };
>
> @@ -156,6 +152,13 @@ static int bcm2835_pwm_probe(struct platform_device *pdev)
>   	if (ret)
>   		return ret;
>
> +	pc->rate = clk_get_rate(pc->clk);
> +	if (!pc->rate) {
> +		dev_err(pc->dev, "failed to get clock rate\n");
> +		ret = -EINVAL;
> +		goto add_fail;
> +	}
> +
>   	pc->chip.dev = &pdev->dev;
>   	pc->chip.ops = &bcm2835_pwm_ops;
>   	pc->chip.npwm = 2;

Re: [PATCH v2 2/3] pwm: bcm2835: allow pwm driver to be used in atomic context

[Alexander Stein]

Hi,

Am Freitag, 13. Oktober 2023, 13:04:48 CEST schrieb Stefan Wahren:
> Hi Sean,
> 
> Am 13.10.23 um 12:46 schrieb Sean Young:
> > clk_get_rate() may do a mutex lock. Since the clock rate cannot change on
> > an rpi, simply fetch it once.
> 
> does it mean you checked all possible SoCs (BCM2835, BCM2836, BCM2837,
> BCM2711, BCM2712) for this change?
> 
> Is it impossible that the real clock can never be influenced by turbo
> mode like SPI?

Assuming the clock can change, which I would, then a clock notifier seems 
appropriate. See [1] for an example.

Best regards,
Alexander

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
commit/?id=90ad2cbe88c22d0215225ab9594eeead0eb24fde

> Best regards
> 
> > Signed-off-by: Sean Young <sean@mess.org>
> > ---
> > 
> >   drivers/pwm/pwm-bcm2835.c | 21 ++++++++++++---------
> >   1 file changed, 12 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c
> > index bdfc2a5ec0d6..59ea154dd657 100644
> > --- a/drivers/pwm/pwm-bcm2835.c
> > +++ b/drivers/pwm/pwm-bcm2835.c
> > @@ -28,6 +28,7 @@ struct bcm2835_pwm {
> > 
> >   	struct device *dev;
> >   	void __iomem *base;
> >   	struct clk *clk;
> > 
> > +	unsigned long rate;
> > 
> >   };
> >   
> >   static inline struct bcm2835_pwm *to_bcm2835_pwm(struct pwm_chip *chip)
> > 
> > @@ -63,17 +64,11 @@ static int bcm2835_pwm_apply(struct pwm_chip *chip,
> > struct pwm_device *pwm,> 
> >   {
> >   
> >   	struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
> > 
> > -	unsigned long rate = clk_get_rate(pc->clk);
> > 
> >   	unsigned long long period_cycles;
> >   	u64 max_period;
> >   	
> >   	u32 val;
> > 
> > -	if (!rate) {
> > -		dev_err(pc->dev, "failed to get clock rate\n");
> > -		return -EINVAL;
> > -	}
> > -
> > 
> >   	/*
> >   	
> >   	 * period_cycles must be a 32 bit value, so period * rate /
> >   	 NSEC_PER_SEC
> >   	 * must be <= U32_MAX. As U32_MAX * NSEC_PER_SEC < U64_MAX the
> > 
> > @@ -88,13 +83,13 @@ static int bcm2835_pwm_apply(struct pwm_chip *chip,
> > struct pwm_device *pwm,> 
> >   	 * <=> period < ((U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC/2) / rate
> >   	 * <=> period <= ceil((U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC/2) / 
rate)
> >   	 - 1
> >   	 */
> > 
> > -	max_period = DIV_ROUND_UP_ULL((u64)U32_MAX * NSEC_PER_SEC + 
NSEC_PER_SEC
> > / 2, rate) - 1; +	max_period = DIV_ROUND_UP_ULL((u64)U32_MAX *
> > NSEC_PER_SEC + NSEC_PER_SEC / 2, pc->rate) - 1;> 
> >   	if (state->period > max_period)
> >   	
> >   		return -EINVAL;
> >   	
> >   	/* set period */
> > 
> > -	period_cycles = DIV_ROUND_CLOSEST_ULL(state->period * rate,
> > NSEC_PER_SEC); +	period_cycles = DIV_ROUND_CLOSEST_ULL(state->period *
> > pc->rate, NSEC_PER_SEC);> 
> >   	/* don't accept a period that is too small */
> >   	if (period_cycles < PERIOD_MIN)
> > 
> > @@ -103,7 +98,7 @@ static int bcm2835_pwm_apply(struct pwm_chip *chip,
> > struct pwm_device *pwm,> 
> >   	writel(period_cycles, pc->base + PERIOD(pwm->hwpwm));
> >   	
> >   	/* set duty cycle */
> > 
> > -	val = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * rate, NSEC_PER_SEC);
> > +	val = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * pc->rate, 
NSEC_PER_SEC);
> > 
> >   	writel(val, pc->base + DUTY(pwm->hwpwm));
> >   	
> >   	/* set polarity */
> > 
> > @@ -129,6 +124,7 @@ static const struct pwm_ops bcm2835_pwm_ops = {
> > 
> >   	.request = bcm2835_pwm_request,
> >   	.free = bcm2835_pwm_free,
> >   	.apply = bcm2835_pwm_apply,
> > 
> > +	.atomic = true,
> > 
> >   	.owner = THIS_MODULE,
> >   
> >   };
> > 
> > @@ -156,6 +152,13 @@ static int bcm2835_pwm_probe(struct platform_device
> > *pdev)> 
> >   	if (ret)
> >   	
> >   		return ret;
> > 
> > +	pc->rate = clk_get_rate(pc->clk);
> > +	if (!pc->rate) {
> > +		dev_err(pc->dev, "failed to get clock rate\n");
> > +		ret = -EINVAL;
> > +		goto add_fail;
> > +	}
> > +
> > 
> >   	pc->chip.dev = &pdev->dev;
> >   	pc->chip.ops = &bcm2835_pwm_ops;
> >   	pc->chip.npwm = 2;


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH v2 2/3] pwm: bcm2835: allow pwm driver to be used in atomic context

[Stefan Wahren]

Hi Alexander,

Am 13.10.23 um 13:13 schrieb Alexander Stein:
> Hi,
>
> Am Freitag, 13. Oktober 2023, 13:04:48 CEST schrieb Stefan Wahren:
>> Hi Sean,
>>
>> Am 13.10.23 um 12:46 schrieb Sean Young:
>>> clk_get_rate() may do a mutex lock. Since the clock rate cannot change on
>>> an rpi, simply fetch it once.
>> does it mean you checked all possible SoCs (BCM2835, BCM2836, BCM2837,
>> BCM2711, BCM2712) for this change?
>>
>> Is it impossible that the real clock can never be influenced by turbo
>> mode like SPI?
> Assuming the clock can change, which I would, then a clock notifier seems
> appropriate. See [1] for an example.
i remember a similiar approach for the CPU frequency for the RPi. At end
the we decided to let the firmware handle it and don't use clock
notifier, see [2] and the related links for more background. The fact
that the VideoCore has the real control makes it hard.

I don't want to say that's impossible.

[2] -
https://patchwork.kernel.org/project/linux-arm-kernel/cover/20190520104708.11980-1-nsaenzjulienne@suse.de/
>
> Best regards,
> Alexander
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
> commit/?id=90ad2cbe88c22d0215225ab9594eeead0eb24fde
>
>> Best regards
>>
>>> Signed-off-by: Sean Young <sean@mess.org>
>>> ---
>>>
>>>    drivers/pwm/pwm-bcm2835.c | 21 ++++++++++++---------
>>>    1 file changed, 12 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c
>>> index bdfc2a5ec0d6..59ea154dd657 100644
>>> --- a/drivers/pwm/pwm-bcm2835.c
>>> +++ b/drivers/pwm/pwm-bcm2835.c
>>> @@ -28,6 +28,7 @@ struct bcm2835_pwm {
>>>
>>>    	struct device *dev;
>>>    	void __iomem *base;
>>>    	struct clk *clk;
>>>
>>> +	unsigned long rate;
>>>
>>>    };
>>>
>>>    static inline struct bcm2835_pwm *to_bcm2835_pwm(struct pwm_chip *chip)
>>>
>>> @@ -63,17 +64,11 @@ static int bcm2835_pwm_apply(struct pwm_chip *chip,
>>> struct pwm_device *pwm,>
>>>    {
>>>
>>>    	struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
>>>
>>> -	unsigned long rate = clk_get_rate(pc->clk);
>>>
>>>    	unsigned long long period_cycles;
>>>    	u64 max_period;
>>>
>>>    	u32 val;
>>>
>>> -	if (!rate) {
>>> -		dev_err(pc->dev, "failed to get clock rate\n");
>>> -		return -EINVAL;
>>> -	}
>>> -
>>>
>>>    	/*
>>>
>>>    	 * period_cycles must be a 32 bit value, so period * rate /
>>>    	 NSEC_PER_SEC
>>>    	 * must be <= U32_MAX. As U32_MAX * NSEC_PER_SEC < U64_MAX the
>>>
>>> @@ -88,13 +83,13 @@ static int bcm2835_pwm_apply(struct pwm_chip *chip,
>>> struct pwm_device *pwm,>
>>>    	 * <=> period < ((U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC/2) / rate
>>>    	 * <=> period <= ceil((U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC/2) /
> rate)
>>>    	 - 1
>>>    	 */
>>>
>>> -	max_period = DIV_ROUND_UP_ULL((u64)U32_MAX * NSEC_PER_SEC +
> NSEC_PER_SEC
>>> / 2, rate) - 1; +	max_period = DIV_ROUND_UP_ULL((u64)U32_MAX *
>>> NSEC_PER_SEC + NSEC_PER_SEC / 2, pc->rate) - 1;>
>>>    	if (state->period > max_period)
>>>
>>>    		return -EINVAL;
>>>
>>>    	/* set period */
>>>
>>> -	period_cycles = DIV_ROUND_CLOSEST_ULL(state->period * rate,
>>> NSEC_PER_SEC); +	period_cycles = DIV_ROUND_CLOSEST_ULL(state->period *
>>> pc->rate, NSEC_PER_SEC);>
>>>    	/* don't accept a period that is too small */
>>>    	if (period_cycles < PERIOD_MIN)
>>>
>>> @@ -103,7 +98,7 @@ static int bcm2835_pwm_apply(struct pwm_chip *chip,
>>> struct pwm_device *pwm,>
>>>    	writel(period_cycles, pc->base + PERIOD(pwm->hwpwm));
>>>
>>>    	/* set duty cycle */
>>>
>>> -	val = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * rate, NSEC_PER_SEC);
>>> +	val = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * pc->rate,
> NSEC_PER_SEC);
>>>    	writel(val, pc->base + DUTY(pwm->hwpwm));
>>>
>>>    	/* set polarity */
>>>
>>> @@ -129,6 +124,7 @@ static const struct pwm_ops bcm2835_pwm_ops = {
>>>
>>>    	.request = bcm2835_pwm_request,
>>>    	.free = bcm2835_pwm_free,
>>>    	.apply = bcm2835_pwm_apply,
>>>
>>> +	.atomic = true,
>>>
>>>    	.owner = THIS_MODULE,
>>>
>>>    };
>>>
>>> @@ -156,6 +152,13 @@ static int bcm2835_pwm_probe(struct platform_device
>>> *pdev)>
>>>    	if (ret)
>>>
>>>    		return ret;
>>>
>>> +	pc->rate = clk_get_rate(pc->clk);
>>> +	if (!pc->rate) {
>>> +		dev_err(pc->dev, "failed to get clock rate\n");
>>> +		ret = -EINVAL;
>>> +		goto add_fail;
>>> +	}
>>> +
>>>
>>>    	pc->chip.dev = &pdev->dev;
>>>    	pc->chip.ops = &bcm2835_pwm_ops;
>>>    	pc->chip.npwm = 2;
>

Re: [PATCH v2 2/3] pwm: bcm2835: allow pwm driver to be used in atomic context

[Uwe Kleine-König]

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

Hello,

On Fri, Oct 13, 2023 at 01:13:50PM +0200, Alexander Stein wrote:
> Am Freitag, 13. Oktober 2023, 13:04:48 CEST schrieb Stefan Wahren:
> > Am 13.10.23 um 12:46 schrieb Sean Young:
> > > clk_get_rate() may do a mutex lock. Since the clock rate cannot change on
> > > an rpi, simply fetch it once.
> > 
> > does it mean you checked all possible SoCs (BCM2835, BCM2836, BCM2837,
> > BCM2711, BCM2712) for this change?
> > 
> > Is it impossible that the real clock can never be influenced by turbo
> > mode like SPI?
> 
> Assuming the clock can change, which I would, then a clock notifier seems 
> appropriate. See [1] for an example.

I'm not a fan. If the clock changes, the output also changes. With a
clock notifier you can soften the issue and reconfigure to something
similar as the original wave form, but a glitch happens for sure.

I already toyed with the thought to add clk_rate_exclusive_get() to all
PWM drivers, but didn't come around it yet.

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 --]

Re: [PATCH v2 2/3] pwm: bcm2835: allow pwm driver to be used in atomic context

[Ivaylo Dimitrov]

On 13.10.23 г. 20:51 ч., Uwe Kleine-König wrote:
> Hello,
> 
> On Fri, Oct 13, 2023 at 01:13:50PM +0200, Alexander Stein wrote:
>> Am Freitag, 13. Oktober 2023, 13:04:48 CEST schrieb Stefan Wahren:
>>> Am 13.10.23 um 12:46 schrieb Sean Young:
>>>> clk_get_rate() may do a mutex lock. Since the clock rate cannot change on
>>>> an rpi, simply fetch it once.
>>>
>>> does it mean you checked all possible SoCs (BCM2835, BCM2836, BCM2837,
>>> BCM2711, BCM2712) for this change?
>>>
>>> Is it impossible that the real clock can never be influenced by turbo
>>> mode like SPI?
>>
>> Assuming the clock can change, which I would, then a clock notifier seems
>> appropriate. See [1] for an example.
> 
> I'm not a fan. If the clock changes, the output also changes. With a
> clock notifier you can soften the issue and reconfigure to something
> similar as the original wave form, but a glitch happens for sure.
> 

Right, but without notifier, everything rate related after the change 
will be wrong

Ivo

Re: [PATCH v2 2/3] pwm: bcm2835: allow pwm driver to be used in atomic context

[Uwe Kleine-König]

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

Hello Ivaylo,

On Sat, Oct 14, 2023 at 09:51:12AM +0300, Ivaylo Dimitrov wrote:
> On 13.10.23 г. 20:51 ч., Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Fri, Oct 13, 2023 at 01:13:50PM +0200, Alexander Stein wrote:
> > > Am Freitag, 13. Oktober 2023, 13:04:48 CEST schrieb Stefan Wahren:
> > > > Am 13.10.23 um 12:46 schrieb Sean Young:
> > > > > clk_get_rate() may do a mutex lock. Since the clock rate cannot change on
> > > > > an rpi, simply fetch it once.
> > > > 
> > > > does it mean you checked all possible SoCs (BCM2835, BCM2836, BCM2837,
> > > > BCM2711, BCM2712) for this change?
> > > > 
> > > > Is it impossible that the real clock can never be influenced by turbo
> > > > mode like SPI?
> > > 
> > > Assuming the clock can change, which I would, then a clock notifier seems
> > > appropriate. See [1] for an example.
> > 
> > I'm not a fan. If the clock changes, the output also changes. With a
> > clock notifier you can soften the issue and reconfigure to something
> > similar as the original wave form, but a glitch happens for sure.
> > 
> 
> Right, but without notifier, everything rate related after the change will
> be wrong

So we agree clk_rate_exclusive_get() is the way to go?! It's simple, no
need for a notifier and no glitches.

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 --]

[PATCH v2 3/3] media: pwm-ir-tx: trigger edges from hrtimer interrupt context

[Sean Young]

This makes the driver much more precise.

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/media/rc/pwm-ir-tx.c | 79 ++++++++++++++++++++++++++++++++++--
 1 file changed, 76 insertions(+), 3 deletions(-)

diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c
index c5f37c03af9c..3e801fa8ee2c 100644
--- a/drivers/media/rc/pwm-ir-tx.c
+++ b/drivers/media/rc/pwm-ir-tx.c
@@ -10,6 +10,8 @@
 #include <linux/slab.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/hrtimer.h>
+#include <linux/completion.h>
 #include <media/rc-core.h>
 
 #define DRIVER_NAME	"pwm-ir-tx"
@@ -17,8 +19,14 @@
 
 struct pwm_ir {
 	struct pwm_device *pwm;
-	unsigned int carrier;
-	unsigned int duty_cycle;
+	struct hrtimer timer;
+	struct completion completion;
+	struct pwm_state *state;
+	uint carrier;
+	uint duty_cycle;
+	uint *txbuf;
+	uint txbuf_len;
+	uint txbuf_index;
 };
 
 static const struct of_device_id pwm_ir_of_match[] = {
@@ -82,6 +90,62 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
 	return count;
 }
 
+static int pwm_ir_tx_atomic(struct rc_dev *dev, unsigned int *txbuf,
+			    unsigned int count)
+{
+	struct pwm_ir *pwm_ir = dev->priv;
+	struct pwm_device *pwm = pwm_ir->pwm;
+	struct pwm_state state;
+
+	pwm_init_state(pwm, &state);
+
+	state.period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier);
+	pwm_set_relative_duty_cycle(&state, pwm_ir->duty_cycle, 100);
+
+	pwm_ir->txbuf = txbuf;
+	pwm_ir->txbuf_len = count;
+	pwm_ir->txbuf_index = 0;
+	pwm_ir->state = &state;
+
+	hrtimer_start(&pwm_ir->timer, 0, HRTIMER_MODE_REL);
+
+	wait_for_completion(&pwm_ir->completion);
+
+	return count;
+}
+
+static enum hrtimer_restart pwm_ir_timer(struct hrtimer *timer)
+{
+	struct pwm_ir *pwm_ir = container_of(timer, struct pwm_ir, timer);
+	ktime_t now;
+
+	/*
+	 * If we happen to hit an odd latency spike, loop through the
+	 * pulses until we catch up.
+	 */
+	do {
+		u64 ns;
+
+		pwm_ir->state->enabled = !(pwm_ir->txbuf_index % 2);
+		pwm_apply_state_atomic(pwm_ir->pwm, pwm_ir->state);
+
+		if (pwm_ir->txbuf_index >= pwm_ir->txbuf_len) {
+			complete(&pwm_ir->completion);
+
+			return HRTIMER_NORESTART;
+		}
+
+		ns = US_TO_NS(pwm_ir->txbuf[pwm_ir->txbuf_index]);
+		hrtimer_add_expires_ns(timer, ns);
+
+		pwm_ir->txbuf_index++;
+
+		now = timer->base->get_time();
+	} while (hrtimer_get_expires_tv64(timer) < now);
+
+	return HRTIMER_RESTART;
+}
+
 static int pwm_ir_probe(struct platform_device *pdev)
 {
 	struct pwm_ir *pwm_ir;
@@ -103,10 +167,19 @@ static int pwm_ir_probe(struct platform_device *pdev)
 	if (!rcdev)
 		return -ENOMEM;
 
+	if (pwm_is_atomic(pwm_ir->pwm)) {
+		init_completion(&pwm_ir->completion);
+		hrtimer_init(&pwm_ir->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+		pwm_ir->timer.function = pwm_ir_timer;
+		rcdev->tx_ir = pwm_ir_tx_atomic;
+	} else {
+		dev_info(&pdev->dev, "tx will not be accurate as pwm device does not support atomic mode");
+		rcdev->tx_ir = pwm_ir_tx;
+	}
+
 	rcdev->priv = pwm_ir;
 	rcdev->driver_name = DRIVER_NAME;
 	rcdev->device_name = DEVICE_NAME;
-	rcdev->tx_ir = pwm_ir_tx;
 	rcdev->s_tx_duty_cycle = pwm_ir_set_duty_cycle;
 	rcdev->s_tx_carrier = pwm_ir_set_carrier;
 
-- 
2.42.0

Re: [PATCH v2 3/3] media: pwm-ir-tx: trigger edges from hrtimer interrupt context

[Ivaylo Dimitrov]

On 13.10.23 г. 13:46 ч., Sean Young wrote:
> This makes the driver much more precise.
> 
> Signed-off-by: Sean Young <sean@mess.org>
> ---
>   drivers/media/rc/pwm-ir-tx.c | 79 ++++++++++++++++++++++++++++++++++--
>   1 file changed, 76 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c
> index c5f37c03af9c..3e801fa8ee2c 100644
> --- a/drivers/media/rc/pwm-ir-tx.c
> +++ b/drivers/media/rc/pwm-ir-tx.c
> @@ -10,6 +10,8 @@
>   #include <linux/slab.h>
>   #include <linux/of.h>
>   #include <linux/platform_device.h>
> +#include <linux/hrtimer.h>
> +#include <linux/completion.h>
>   #include <media/rc-core.h>
>   
>   #define DRIVER_NAME	"pwm-ir-tx"
> @@ -17,8 +19,14 @@
>   
>   struct pwm_ir {
>   	struct pwm_device *pwm;
> -	unsigned int carrier;
> -	unsigned int duty_cycle;
> +	struct hrtimer timer;
> +	struct completion completion;

what about 'struct completion tx_done'?

> +	struct pwm_state *state;
> +	uint carrier;
> +	uint duty_cycle;

With my c++ developer hat on, I think either 'u32' or 'unsigned int' is 
more proper type for carrier and duty_cycle. Both s_tx_duty_cycle and 
s_tx_carrier are declared with second parameter of type u32, maybe 
that's what have to be used all over the place if you are to change from 
'unsigned int'. But better leave as it is, pwm_set_relative_duty_cycle() 
takes 'unsigned int' anyway.

> +	uint *txbuf;
> +	uint txbuf_len;
> +	uint txbuf_index;

OTOH, it is (*tx_ir)(struct rc_dev *dev, unsigned *txbuf, unsigned n), 
so maybe you should use 'unsigned' or 'unsigned int' for those.

I know at the end all those will be compiled to same type, but still :)

>   };
>   
>   static const struct of_device_id pwm_ir_of_match[] = {
> @@ -82,6 +90,62 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
>   	return count;
>   }
>   
> +static int pwm_ir_tx_atomic(struct rc_dev *dev, unsigned int *txbuf,
> +			    unsigned int count)
> +{
> +	struct pwm_ir *pwm_ir = dev->priv;
> +	struct pwm_device *pwm = pwm_ir->pwm;
> +	struct pwm_state state;
> +
> +	pwm_init_state(pwm, &state);
> +
> +	state.period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier);
> +	pwm_set_relative_duty_cycle(&state, pwm_ir->duty_cycle, 100);
> +
> +	pwm_ir->txbuf = txbuf;
> +	pwm_ir->txbuf_len = count;
> +	pwm_ir->txbuf_index = 0;
> +	pwm_ir->state = &state;
> +
> +	hrtimer_start(&pwm_ir->timer, 0, HRTIMER_MODE_REL);
> +
> +	wait_for_completion(&pwm_ir->completion);
> +
> +	return count;
> +}
> +
> +static enum hrtimer_restart pwm_ir_timer(struct hrtimer *timer)
> +{
> +	struct pwm_ir *pwm_ir = container_of(timer, struct pwm_ir, timer);
> +	ktime_t now;
> +
> +	/*
> +	 * If we happen to hit an odd latency spike, loop through the
> +	 * pulses until we catch up.
> +	 */
> +	do {
> +		u64 ns;
> +
> +		pwm_ir->state->enabled = !(pwm_ir->txbuf_index % 2);
> +		pwm_apply_state_atomic(pwm_ir->pwm, pwm_ir->state);
> +
> +		if (pwm_ir->txbuf_index >= pwm_ir->txbuf_len) {
> +			complete(&pwm_ir->completion);
> +
> +			return HRTIMER_NORESTART;
> +		}
> +
> +		ns = US_TO_NS(pwm_ir->txbuf[pwm_ir->txbuf_index]);
> +		hrtimer_add_expires_ns(timer, ns);
> +
> +		pwm_ir->txbuf_index++;
> +
> +		now = timer->base->get_time();
> +	} while (hrtimer_get_expires_tv64(timer) < now);
> +
> +	return HRTIMER_RESTART;
> +}
> +
>   static int pwm_ir_probe(struct platform_device *pdev)
>   {
>   	struct pwm_ir *pwm_ir;
> @@ -103,10 +167,19 @@ static int pwm_ir_probe(struct platform_device *pdev)
>   	if (!rcdev)
>   		return -ENOMEM;
>   
> +	if (pwm_is_atomic(pwm_ir->pwm)) {
> +		init_completion(&pwm_ir->completion);
> +		hrtimer_init(&pwm_ir->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +		pwm_ir->timer.function = pwm_ir_timer;
> +		rcdev->tx_ir = pwm_ir_tx_atomic;
> +	} else {
> +		dev_info(&pdev->dev, "tx will not be accurate as pwm device does not support atomic mode");
> +		rcdev->tx_ir = pwm_ir_tx;
> +	}
> +
>   	rcdev->priv = pwm_ir;
>   	rcdev->driver_name = DRIVER_NAME;
>   	rcdev->device_name = DEVICE_NAME;
> -	rcdev->tx_ir = pwm_ir_tx;
>   	rcdev->s_tx_duty_cycle = pwm_ir_set_duty_cycle;
>   	rcdev->s_tx_carrier = pwm_ir_set_carrier;
>   
> 

Re: [PATCH v2 3/3] media: pwm-ir-tx: trigger edges from hrtimer interrupt context

[Sean Young]

On Sun, Oct 15, 2023 at 09:31:34AM +0300, Ivaylo Dimitrov wrote:
> On 13.10.23 г. 13:46 ч., Sean Young wrote:
> > This makes the driver much more precise.
> > 
> > Signed-off-by: Sean Young <sean@mess.org>
> > ---
> >   drivers/media/rc/pwm-ir-tx.c | 79 ++++++++++++++++++++++++++++++++++--
> >   1 file changed, 76 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c
> > index c5f37c03af9c..3e801fa8ee2c 100644
> > --- a/drivers/media/rc/pwm-ir-tx.c
> > +++ b/drivers/media/rc/pwm-ir-tx.c
> > @@ -10,6 +10,8 @@
> >   #include <linux/slab.h>
> >   #include <linux/of.h>
> >   #include <linux/platform_device.h>
> > +#include <linux/hrtimer.h>
> > +#include <linux/completion.h>
> >   #include <media/rc-core.h>
> >   #define DRIVER_NAME	"pwm-ir-tx"
> > @@ -17,8 +19,14 @@
> >   struct pwm_ir {
> >   	struct pwm_device *pwm;
> > -	unsigned int carrier;
> > -	unsigned int duty_cycle;
> > +	struct hrtimer timer;
> > +	struct completion completion;
> 
> what about 'struct completion tx_done'?

Agreed, that's much better.

> > +	struct pwm_state *state;
> > +	uint carrier;
> > +	uint duty_cycle;
> 
> With my c++ developer hat on, I think either 'u32' or 'unsigned int' is more
> proper type for carrier and duty_cycle. Both s_tx_duty_cycle and
> s_tx_carrier are declared with second parameter of type u32, maybe that's
> what have to be used all over the place if you are to change from 'unsigned
> int'. But better leave as it is, pwm_set_relative_duty_cycle() takes
> 'unsigned int' anyway.

I much prefer the rust way of u64/u32/u16/u8/usize and simply no int/short/long
types at all. int is useful when your compiler needs to work on weird
architectures with non-power-of-two register sizes like the pdp-9 (18 bits
anyone?), but on contemporary cpus there is really no need for int: int is
always a 32 bit value.

So I'm all for banishing int in every form, but for now the kernel uses
unsigned int and u32 interchangably, so it's hard to be consistent with this.

> > +	uint *txbuf;
> > +	uint txbuf_len;
> > +	uint txbuf_index;
> 
> OTOH, it is (*tx_ir)(struct rc_dev *dev, unsigned *txbuf, unsigned n), so
> maybe you should use 'unsigned' or 'unsigned int' for those.
> 
> I know at the end all those will be compiled to same type, but still :)

Maybe it's time for tx_ir to be defined with u32 types and do away with
this madness.

However, as it stands I agree with your points. I guess it's best to be
consistent with the apis this driver implements/uses.

Thanks,

Sean