Remove ir-rx51 driver

The generic pwm-ir-tx driver works for the Nokia n900, so nokia,n900-ir can be removed.

Signed-off-by Sean Young <[email protected]>

---
 .../bindings/leds/irled/pwm-ir-tx.yaml        |  5 ++++-
 .../devicetree/bindings/media/nokia,n900-ir   | 20 -------------------
 2 files changed, 4 insertions(+), 21 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/media/nokia,n900-ir

Documentation/devicetree/bindings/leds/irled/pwm-ir-tx.yaml +4 -1

index f2a6fa140f38..7526e3149f72 100644
@@ -15,7 +15,10 @@ description:
 
 properties:
   compatible:
-    const: pwm-ir-tx
+    oneOf:
+      - const: pwm-ir-tx
+      - const: nokia,n900-ir
+        deprecated: true
 
   pwms:
     maxItems: 1

Documentation/devicetree/bindings/media/nokia,n900-ir deleted +0 -20

deleted file mode 100644
index 13a18ce37dd1..000000000000
@@ -1,20 +0,0 @@
-Device-Tree bindings for LIRC TX driver for Nokia N900(RX51)
-
-Required properties:
-	- compatible: should be "nokia,n900-ir".
-	- pwms: specifies PWM used for IR signal transmission.
-
-Example node:
-
-	pwm9: dmtimer-pwm@9 {
-		compatible = "ti,omap-dmtimer-pwm";
-		ti,timers = <&timer9>;
-		ti,clock-source = <0x00>; /* timer_sys_ck */
-		#pwm-cells = <3>;
-	};
-
-	ir: n900-ir {
-		compatible = "nokia,n900-ir";
-
-		pwms = <&pwm9 0 26316 0>; /* 38000 Hz */
-	};

Signature · 1 line

2.42.0

Quoted 4 lines · Sean Young

The generic pwm-ir-tx driver works for the Nokia n900, so nokia,n900-ir can be removed.

Signed-off-by Sean Young <[email protected]>

Acked-by Conor Dooley <[email protected]>

Quoted 2 lines

---
 .../bindings/leds/irled/pwm-ir-tx.yaml        |  5 ++++-

Quoted 49 lines

 .../devicetree/bindings/media/nokia,n900-ir   | 20 -------------------
 2 files changed, 4 insertions(+), 21 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/media/nokia,n900-ir

Documentation/devicetree/bindings/leds/irled/pwm-ir-tx.yaml +4 -1

index f2a6fa140f38..7526e3149f72 100644
@@ -15,7 +15,10 @@ description:
 
 properties:
   compatible:
-    const: pwm-ir-tx
+    oneOf:
+      - const: pwm-ir-tx
+      - const: nokia,n900-ir
+        deprecated: true
 
   pwms:
     maxItems: 1

Documentation/devicetree/bindings/media/nokia,n900-ir deleted +0 -20

deleted file mode 100644
index 13a18ce37dd1..000000000000
@@ -1,20 +0,0 @@
-Device-Tree bindings for LIRC TX driver for Nokia N900(RX51)
-
-Required properties:
-	- compatible: should be "nokia,n900-ir".
-	- pwms: specifies PWM used for IR signal transmission.
-
-Example node:
-
-	pwm9: dmtimer-pwm@9 {
-		compatible = "ti,omap-dmtimer-pwm";
-		ti,timers = <&timer9>;
-		ti,clock-source = <0x00>; /* timer_sys_ck */
-		#pwm-cells = <3>;
-	};
-
-	ir: n900-ir {
-		compatible = "nokia,n900-ir";
-
-		pwms = <&pwm9 0 26316 0>; /* 38000 Hz */
-	};

Signature · 1 line

2.42.0

The ir-rx51 is a pwm-based TX driver specific to the N900. This can be handled entirely by the generic pwm-ir-tx driver, and in fact the pwm-ir-tx driver has been compatible with ir-rx51 from the start.

Note that the suspend code in the ir-rx51 driver is unnecessary, since during transmit, the process is not in interruptable sleep. The process is not put to sleep until the transmit completes.

Cc Timo Kokkonen <[email protected]>

Cc Pali Rohár <[email protected]>

Cc Ivaylo Dimitrov <[email protected]>

Tested-by Sicelo A. Mhlongo <[email protected]>

Signed-off-by Sean Young <[email protected]>

The ir-rx51 is a pwm-based TX driver specific to the N900. This can be handled entirely by the generic pwm-ir-tx driver, and in fact the pwm-ir-tx driver has been compatible with ir-rx51 from the start.

Quoted 4 lines · Sean Young

The ir-rx51 is a pwm-based TX driver specific to the N900. This can be handled entirely by the generic pwm-ir-tx driver, and in fact the pwm-ir-tx driver has been compatible with ir-rx51 from the start.

Unfortunately, pwm-ir-tx does not work on n900. My investigation shows that for some reason usleep_range() sleeps for at least 300-400 us more than what interval it is requested to sleep. I played with cyclictest from rt-tests package and it gives similar results - increasing the priority helps, but I was not able to make it sleep for less that 300 us in average. I tried cpu_latency_qos_add_request() in pwm-ir-tx, but it made no difference.

I get similar results on motorola droid4 (OMAP4), albeit there average sleep is in 200-300 us range, which makes me believe that either OMAPs have issues with hrtimers or the config we use has some issue which leads to scheduler latency. Or, something else...

In either case help is appreciated to dig further trying to find the reason for such a big delay.

Quoted 13 lines · Ivaylo Dimitrov

Quoted 4 lines · Sean Young

The ir-rx51 is a pwm-based TX driver specific to the N900. This can be handled entirely by the generic pwm-ir-tx driver, and in fact the pwm-ir-tx driver has been compatible with ir-rx51 from the start.

Unfortunately, pwm-ir-tx does not work on n900. My investigation shows that for some reason usleep_range() sleeps for at least 300-400 us more than what interval it is requested to sleep. I played with cyclictest from rt-tests package and it gives similar results - increasing the priority helps, but I was not able to make it sleep for less that 300 us in average. I tried cpu_latency_qos_add_request() in pwm-ir-tx, but it made no difference.

Quoted 4 lines

I get similar results on motorola droid4 (OMAP4), albeit there average sleep is in 200-300 us range, which makes me believe that either OMAPs have issues with hrtimers or the config we use has some issue which leads to scheduler latency. Or, something else...

The pwm-ir-tx driver does suffer from this problem, but I was under the impression that the ir-rx51 has the same problem.

Quoted 2 lines

In either case help is appreciated to dig further trying to find the reason for such a big delay.

pwm-ir-tx uses usleep_range() and ir-rx51 uses hrtimers. I thought that usleep_range() uses hrtimers; however if you're not seeing the same delay on ir-rx51 then maybe it's time to switch pwm-ir-tx to hrtimers.

I don't have a n900 to test on, unfortunately.

Quoted 25 lines · Sean Young

Quoted 13 lines · Ivaylo Dimitrov

Quoted 4 lines · Sean Young

The ir-rx51 is a pwm-based TX driver specific to the N900. This can be handled entirely by the generic pwm-ir-tx driver, and in fact the pwm-ir-tx driver has been compatible with ir-rx51 from the start.

Unfortunately, pwm-ir-tx does not work on n900. My investigation shows that for some reason usleep_range() sleeps for at least 300-400 us more than what interval it is requested to sleep. I played with cyclictest from rt-tests package and it gives similar results - increasing the priority helps, but I was not able to make it sleep for less that 300 us in average. I tried cpu_latency_qos_add_request() in pwm-ir-tx, but it made no difference.

Quoted 4 lines

I get similar results on motorola droid4 (OMAP4), albeit there average sleep is in 200-300 us range, which makes me believe that either OMAPs have issues with hrtimers or the config we use has some issue which leads to scheduler latency. Or, something else...

The pwm-ir-tx driver does suffer from this problem, but I was under the impression that the ir-rx51 has the same problem.

Quoted 2 lines

In either case help is appreciated to dig further trying to find the reason for such a big delay.

Quoted 3 lines

pwm-ir-tx uses usleep_range() and ir-rx51 uses hrtimers. I thought that usleep_range() uses hrtimers; however if you're not seeing the same delay on ir-rx51 then maybe it's time to switch pwm-ir-tx to hrtimers.

Maybe using fsleep() fixes this issue? See commit c6af13d33475 ("timer: add fsleep for flexible sleeping"), and Documentation/timers/timers-howto.rst.

The long wake-up time for an idle state could explain the values. I think Ivaylo already tested with most cpuidle states disabled via sysfs though.

Quoted 1 line

I don't have a n900 to test on, unfortunately.

If you want one for development, the maemo folks cc:ed here likely have some available devices.

Regards,

Tony

Quoted 13 lines · Ivaylo Dimitrov

Quoted 4 lines · Sean Young

The ir-rx51 is a pwm-based TX driver specific to the N900. This can be handled entirely by the generic pwm-ir-tx driver, and in fact the pwm-ir-tx driver has been compatible with ir-rx51 from the start.

Unfortunately, pwm-ir-tx does not work on n900. My investigation shows that for some reason usleep_range() sleeps for at least 300-400 us more than what interval it is requested to sleep. I played with cyclictest from rt-tests package and it gives similar results - increasing the priority helps, but I was not able to make it sleep for less that 300 us in average. I tried cpu_latency_qos_add_request() in pwm-ir-tx, but it made no difference.

Quoted 4 lines

I get similar results on motorola droid4 (OMAP4), albeit there average sleep is in 200-300 us range, which makes me believe that either OMAPs have issues with hrtimers or the config we use has some issue which leads to scheduler latency. Or, something else...

The pwm-ir-tx driver does suffer from this problem, but I was under the impression that the ir-rx51 has the same problem.

Quoted 2 lines

In either case help is appreciated to dig further trying to find the reason for such a big delay.

pwm-ir-tx uses usleep_range() and ir-rx51 uses hrtimers. I thought that usleep_range() uses hrtimers; however if you're not seeing the same delay on ir-rx51 then maybe it's time to switch pwm-ir-tx to hrtimers.

I don't have a n900 to test on, unfortunately.

Thanks
Sean

Quoted 19 lines · Sean Young

Quoted 17 lines · Ivaylo Dimitrov

Quoted 4 lines · Sean Young

The ir-rx51 is a pwm-based TX driver specific to the N900. This can be handled entirely by the generic pwm-ir-tx driver, and in fact the pwm-ir-tx driver has been compatible with ir-rx51 from the start.

Unfortunately, pwm-ir-tx does not work on n900. My investigation shows that for some reason usleep_range() sleeps for at least 300-400 us more than what interval it is requested to sleep. I played with cyclictest from rt-tests package and it gives similar results - increasing the priority helps, but I was not able to make it sleep for less that 300 us in average. I tried cpu_latency_qos_add_request() in pwm-ir-tx, but it made no difference.

I get similar results on motorola droid4 (OMAP4), albeit there average sleep is in 200-300 us range, which makes me believe that either OMAPs have issues with hrtimers or the config we use has some issue which leads to scheduler latency. Or, something else...

Quoted 3 lines

The pwm-ir-tx driver does suffer from this problem, but I was under the impression that the ir-rx51 has the same problem.

Could you elaborate on the "pwm-ir-tx driver does suffer from this problem"? Where do you see that?

ir-rx51 does not suffer from the same problem (albeit it has its own one, see bellow)

Quoted 7 lines

Quoted 2 lines

In either case help is appreciated to dig further trying to find the reason for such a big delay.

pwm-ir-tx uses usleep_range() and ir-rx51 uses hrtimers. I thought that usleep_range() uses hrtimers; however if you're not seeing the same delay on ir-rx51 then maybe it's time to switch pwm-ir-tx to hrtimers.

usleep_range() is backed by hrtimers already, however the difference comes from how hrtimer is used in ir-rx51: it uses timer callback function that gets called in softirq context, while usleep_range() puts the task in TASK_UNINTERRUPTIBLE state and then calls schedule_hrtimeout_range(). For some reason it takes at least 200-400 us (on average) even on OMAP4 to switch back to TASK_RUNNING state.

The issue with ir-rx51 and the way it uses hrtimers is that it calls pwm_apply_state() from hrtimer function, which is not ok, per the comment here
https://elixir.bootlin.com/linux/v6.6-rc3/source/drivers/pwm/core.c#L502

I can make pwm-ir-tx switch to hrtimers, that's not an issue, but I am afraid that there is some general scheduler or timers (or something else) issue that manifests itself with usleep_range() misbehaving.

Quoted 2 lines

I don't have a n900 to test on, unfortunately.

I have and once I have an idea what's going on will port pwm-ir-tx to hrtimers, if needed. Don't want to do it now as I am afraid the completion I will have to use will have the same latency problems as usleep_range()

Quoted 20 lines · Ivaylo Dimitrov

Quoted 19 lines · Sean Young

Quoted 17 lines · Ivaylo Dimitrov

Quoted 4 lines · Sean Young

The ir-rx51 is a pwm-based TX driver specific to the N900. This can be handled entirely by the generic pwm-ir-tx driver, and in fact the pwm-ir-tx driver has been compatible with ir-rx51 from the start.

Unfortunately, pwm-ir-tx does not work on n900. My investigation shows that for some reason usleep_range() sleeps for at least 300-400 us more than what interval it is requested to sleep. I played with cyclictest from rt-tests package and it gives similar results - increasing the priority helps, but I was not able to make it sleep for less that 300 us in average. I tried cpu_latency_qos_add_request() in pwm-ir-tx, but it made no difference.

I get similar results on motorola droid4 (OMAP4), albeit there average sleep is in 200-300 us range, which makes me believe that either OMAPs have issues with hrtimers or the config we use has some issue which leads to scheduler latency. Or, something else...

Quoted 6 lines

Quoted 3 lines

The pwm-ir-tx driver does suffer from this problem, but I was under the impression that the ir-rx51 has the same problem.

Could you elaborate on the "pwm-ir-tx driver does suffer from this problem"? Where do you see that?

So on a raspberry pi (model 3b), if I use the pwm-ir-tx driver, I get random delays of up to 100us. It's a bit random and certainly depends on the load.

I'm measuring using a logic analyzer.

There have been reports by others on different machines with random delays and/or transmit failures (as in the receiver occassionally fails to decode the IR). I usually suggest they use the gpio-ir-tx driver, which does work as far as I know (the signal looks perfect with a logic analyzer).

So far I've taken the view that the driver works ok for most situations, since IR is usually fine with upto 100us missing here or there.

The gpio-ir-tx driver works much better because it does the entire send under spinlock - obviously that has its own problems, because an IR transmit can be 10s or even 100s of milliseconds.

I've never known of a solution to the pwm-ir-tx driver. If using hrtimers directly improves the situation even a bit, then that would be great.

Quoted 23 lines

ir-rx51 does not suffer from the same problem (albeit it has its own one, see bellow)

Quoted 7 lines

Quoted 2 lines

In either case help is appreciated to dig further trying to find the reason for such a big delay.

pwm-ir-tx uses usleep_range() and ir-rx51 uses hrtimers. I thought that usleep_range() uses hrtimers; however if you're not seeing the same delay on ir-rx51 then maybe it's time to switch pwm-ir-tx to hrtimers.

usleep_range() is backed by hrtimers already, however the difference comes from how hrtimer is used in ir-rx51: it uses timer callback function that gets called in softirq context, while usleep_range() puts the task in TASK_UNINTERRUPTIBLE state and then calls schedule_hrtimeout_range(). For some reason it takes at least 200-400 us (on average) even on OMAP4 to switch back to TASK_RUNNING state.

The issue with ir-rx51 and the way it uses hrtimers is that it calls pwm_apply_state() from hrtimer function, which is not ok, per the comment here
https://elixir.bootlin.com/linux/v6.6-rc3/source/drivers/pwm/core.c#L502

Quoted 3 lines

I can make pwm-ir-tx switch to hrtimers, that's not an issue, but I am afraid that there is some general scheduler or timers (or something else) issue that manifests itself with usleep_range() misbehaving.

If we can switch pwm-ir-tx to hrtimers, that would be great.

The ir-rx51 removal patches have already been queued to media_staging; we may have to remove them from there if we can't solve this problem.

Quoted 6 lines

Quoted 2 lines

I don't have a n900 to test on, unfortunately.

I have and once I have an idea what's going on will port pwm-ir-tx to hrtimers, if needed. Don't want to do it now as I am afraid the completion I will have to use will have the same latency problems as usleep_range()

That would be fantastic. Please do keep us up to date with how you are getting on. Like I said, it would be nice to this resolved before the next merge window.

Thanks,
Sean

Quoted 2 lines · Sean Young

I've never known of a solution to the pwm-ir-tx driver. If using hrtimers directly improves the situation even a bit, then that would be great.

The issue with hrtimers is that we cannot use them directly, as pwm_apply_state() may sleep, but hrtimer function is called in atomic context.

Quoted 7 lines · Ivaylo Dimitrov

Quoted 2 lines · Sean Young

I've never known of a solution to the pwm-ir-tx driver. If using hrtimers directly improves the situation even a bit, then that would be great.

The issue with hrtimers is that we cannot use them directly, as pwm_apply_state() may sleep, but hrtimer function is called in atomic context.

I've also been looking at this problem and came to same conclusion: the fact that pwm_apply_state() sleeps is a huge problem.

1) The vast majority of pwm drivers don't sleep, or could even be converted to spinlocks (e.g pwm-sifive.c could use spinlocks, as far as I can see).

2) Sure, some pwm devices are on i2c busses, so the driver needs to sleep. Those devices aren't great for what we're trying to do here, since the sleeping may cause delays and affect the generated signal.

What would be ideal here is to have pwm-ir-tx work in atomic context if a non-sleeping pwm device is used, and another (non-optimal) code path for sleeping pwm drivers. We could even just refuse to run on sleeping pwm drivers.

Uwe what do you think of this idea? The pwm api could have a bool pwm_may_sleep(struct pwm *pwm) function,

and pwm_apply_state() does
not contain might_sleep() - only the driver-specific apply calls might_sleep().

It would be nice if this could all be done at compile time through e.g. a device tree attribute.