[PATCH v2] ARM: at91: Document new TCB bindings

The current binding for the TCB is not flexible enough for some use cases
and prevents proper utilization of all the channels.

Cc: Daniel Lezcano <redacted>
Cc: Thierry Reding <redacted>
Cc: linux-pwm at vger.kernel.org
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree at vger.kernel.org
Signed-off-by: Alexandre Belloni <redacted>
---

Changes in v2:
 - added slow_clk in the examples
 - removed linuxisms

Rob, does that fit what you wanted? I prefer getting your ack on that one before
resending a 48 patches series.


 .../devicetree/bindings/arm/atmel-at91.txt         | 32 -----------
 .../devicetree/bindings/mfd/atmel-tcb.txt          | 62 ++++++++++++++++++++++
 .../devicetree/bindings/pwm/atmel-tcb-pwm.txt      | 12 +++--
 3 files changed, 69 insertions(+), 37 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/atmel-tcb.txt

diff --git a/Documentation/devicetree/bindings/arm/atmel-at91.txt b/Documentation/devicetree/bindings/arm/atmel-at91.txt
index e1f5ad855f14..3dc758403d03 100644
--- a/Documentation/devicetree/bindings/arm/atmel-at91.txt
+++ b/Documentation/devicetree/bindings/arm/atmel-at91.txt

@@ -60,38 +60,6 @@ System Timer (ST) required properties:
 Its subnodes can be:
 - watchdog: compatible should be "atmel,at91rm9200-wdt"
 
-TC/TCLIB Timer required properties:
-- compatible: Should be "atmel,<chip>-tcb".
-  <chip> can be "at91rm9200" or "at91sam9x5"
-- reg: Should contain registers location and length
-- interrupts: Should contain all interrupts for the TC block
-  Note that you can specify several interrupt cells if the TC
-  block has one interrupt per channel.
-- clock-names: tuple listing input clock names.
-	Required elements: "t0_clk", "slow_clk"
-	Optional elements: "t1_clk", "t2_clk"
-- clocks: phandles to input clocks.
-
-Examples:
-
-One interrupt per TC block:
-	tcb0: timer at fff7c000 {
-		compatible = "atmel,at91rm9200-tcb";
-		reg = <0xfff7c000 0x100>;
-		interrupts = <18 4>;
-		clocks = <&tcb0_clk>;
-		clock-names = "t0_clk";
-	};
-
-One interrupt per TC channel in a TC block:
-	tcb1: timer at fffdc000 {
-		compatible = "atmel,at91rm9200-tcb";
-		reg = <0xfffdc000 0x100>;
-		interrupts = <26 4 27 4 28 4>;
-		clocks = <&tcb1_clk>;
-		clock-names = "t0_clk";
-	};
-
 RSTC Reset Controller required properties:
 - compatible: Should be "atmel,<chip>-rstc".
   <chip> can be "at91sam9260" or "at91sam9g45" or "sama5d3"

diff --git a/Documentation/devicetree/bindings/mfd/atmel-tcb.txt b/Documentation/devicetree/bindings/mfd/atmel-tcb.txt
new file mode 100644
index 000000000000..71c8d83c01a7
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/atmel-tcb.txt

@@ -0,0 +1,62 @@
+* Device tree bindings for Atmel Timer Counter Blocks
+- compatible: Should be "atmel,<chip>-tcb", "simple-mfd", "syscon".
+  <chip> can be "at91rm9200" or "at91sam9x5"
+- reg: Should contain registers location and length
+- #address-cells: has to be 1
+- #size-cells: has to be 0
+- interrupts: Should contain all interrupts for the TC block
+  Note that you can specify several interrupt cells if the TC
+  block has one interrupt per channel.
+- clock-names: tuple listing input clock names.
+	Required elements: "t0_clk", "slow_clk"
+	Optional elements: "t1_clk", "t2_clk"
+- clocks: phandles to input clocks.
+
+The TCB can expose multiple subdevices:
+ * a clocksource and clockevent device
+   - compatible: Should be "atmel,tcb-free-running-timer"
+   - reg: Should contain the TCB channels to be used. If the
+     counter width is 16 bits (at91rm9200-tcb), two consecutive
+     channels are needed. Else, only one channel will be used.
+
+ * a clockevent device
+   - compatible: Should be "atmel,tcb-programmable-timer"
+   - reg: Should contain the TCB channel to be used
+
+ * a PWM chip: see ../pwm/atmel-tcb-pwm.txt
+
+Examples:
+
+One interrupt per TC block:
+	tcb0: timer at fff7c000 {
+		compatible = "atmel,at91rm9200-tcb", "simple-mfd", "syscon";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <0xfff7c000 0x100>;
+		interrupts = <18 4>;
+		clocks = <&tcb0_clk>, <&clk32k>;
+		clock-names = "t0_clk", "slow_clk";
+
+		timer at 0 {
+			compatible = "atmel,tcb-free-running-timer";
+			reg = <0>, <1>;
+		};
+
+		timer at 2 {
+			compatible = "atmel,tcb-programmable-timer";
+			reg = <2>;
+		};
+	};
+
+One interrupt per TC channel in a TC block:
+	tcb1: timer at fffdc000 {
+		compatible = "atmel,at91rm9200-tcb", "simple-mfd", "syscon";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <0xfffdc000 0x100>;
+		interrupts = <26 4>, <27 4>, <28 4>;
+		clocks = <&tcb1_clk>, <&clk32k>;
+		clock-names = "t0_clk", "slow_clk";
+	};
+
+

diff --git a/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt b/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt
index 8031148bcf85..ab8fbd5ba184 100644
--- a/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/atmel-tcb-pwm.txt

@@ -2,15 +2,17 @@ Atmel TCB PWM controller
 
 Required properties:
 - compatible: should be "atmel,tcb-pwm"
+- reg: tcb channel to use. Each channel can export 2 PWMs
 - #pwm-cells: should be 3. See pwm.txt in this directory for a description of
   the cells format. The only third cell flag supported by this binding is
   PWM_POLARITY_INVERTED.
-- tc-block: The Timer Counter block to use as a PWM chip.
 
 Example:
 
-pwm {
-	compatible = "atmel,tcb-pwm";
-	#pwm-cells = <3>;
-	tc-block = <1>;
+tcb0: timer at f800c000 {
+	pwm at 0 {
+		compatible = "atmel,tcb-pwm";
+		reg = <0>;
+		#pwm-cells = <3>;
+	};
 };

-- 
2.8.1

On Mon, Jun 27, 2016 at 05:05:01PM +0200, Alexandre Belloni wrote:

This still uses Linux terminology.

+   - compatible: Should be "atmel,tcb-free-running-timer"
+   - reg: Should contain the TCB channels to be used. If the
+     counter width is 16 bits (at91rm9200-tcb), two consecutive
+     channels are needed. Else, only one channel will be used.
+
+ * a clockevent device
+   - compatible: Should be "atmel,tcb-programmable-timer"

This still looks like assigning usage in DT. As I'm willing to accept 
that for PWM, either timer channels should be whatever channels are not 
assigned to PWM (i.e. not in DT) or they should just be "timer" and let 
the kernel decide their usage.

Rob

On Thu, 30 Jun 2016 20:27:43 -0500
Rob Herring [off-list ref] wrote:

On Mon, Jun 27, 2016 at 05:05:01PM +0200, Alexandre Belloni wrote:

quoted

The current binding for the TCB is not flexible enough for some use cases
and prevents proper utilization of all the channels.

Cc: Daniel Lezcano <redacted>
Cc: Thierry Reding <redacted>
Cc: linux-pwm at vger.kernel.org
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree at vger.kernel.org
Signed-off-by: Alexandre Belloni <redacted>
---

Changes in v2:
 - added slow_clk in the examples
 - removed linuxisms

Rob, does that fit what you wanted? I prefer getting your ack on that one before
resending a 48 patches series.


 .../devicetree/bindings/arm/atmel-at91.txt         | 32 -----------
 .../devicetree/bindings/mfd/atmel-tcb.txt          | 62 ++++++++++++++++++++++
 .../devicetree/bindings/pwm/atmel-tcb-pwm.txt      | 12 +++--
 3 files changed, 69 insertions(+), 37 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/atmel-tcb.txt

diff --git a/Documentation/devicetree/bindings/arm/atmel-at91.txt b/Documentation/devicetree/bindings/arm/atmel-at91.txt
index e1f5ad855f14..3dc758403d03 100644
--- a/Documentation/devicetree/bindings/arm/atmel-at91.txt
+++ b/Documentation/devicetree/bindings/arm/atmel-at91.txt

@@ -60,38 +60,6 @@ System Timer (ST) required properties:
 Its subnodes can be:
 - watchdog: compatible should be "atmel,at91rm9200-wdt"
 
-TC/TCLIB Timer required properties:
-- compatible: Should be "atmel,<chip>-tcb".
-  <chip> can be "at91rm9200" or "at91sam9x5"
-- reg: Should contain registers location and length
-- interrupts: Should contain all interrupts for the TC block
-  Note that you can specify several interrupt cells if the TC
-  block has one interrupt per channel.
-- clock-names: tuple listing input clock names.
-	Required elements: "t0_clk", "slow_clk"
-	Optional elements: "t1_clk", "t2_clk"
-- clocks: phandles to input clocks.
-
-Examples:
-
-One interrupt per TC block:
-	tcb0: timer at fff7c000 {
-		compatible = "atmel,at91rm9200-tcb";
-		reg = <0xfff7c000 0x100>;
-		interrupts = <18 4>;
-		clocks = <&tcb0_clk>;
-		clock-names = "t0_clk";
-	};
-
-One interrupt per TC channel in a TC block:
-	tcb1: timer at fffdc000 {
-		compatible = "atmel,at91rm9200-tcb";
-		reg = <0xfffdc000 0x100>;
-		interrupts = <26 4 27 4 28 4>;
-		clocks = <&tcb1_clk>;
-		clock-names = "t0_clk";
-	};
-
 RSTC Reset Controller required properties:
 - compatible: Should be "atmel,<chip>-rstc".
   <chip> can be "at91sam9260" or "at91sam9g45" or "sama5d3"

diff --git a/Documentation/devicetree/bindings/mfd/atmel-tcb.txt b/Documentation/devicetree/bindings/mfd/atmel-tcb.txt
new file mode 100644
index 000000000000..71c8d83c01a7
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/atmel-tcb.txt

@@ -0,0 +1,62 @@
+* Device tree bindings for Atmel Timer Counter Blocks
+- compatible: Should be "atmel,<chip>-tcb", "simple-mfd", "syscon".
+  <chip> can be "at91rm9200" or "at91sam9x5"
+- reg: Should contain registers location and length
+- #address-cells: has to be 1
+- #size-cells: has to be 0
+- interrupts: Should contain all interrupts for the TC block
+  Note that you can specify several interrupt cells if the TC
+  block has one interrupt per channel.
+- clock-names: tuple listing input clock names.
+	Required elements: "t0_clk", "slow_clk"
+	Optional elements: "t1_clk", "t2_clk"
+- clocks: phandles to input clocks.
+
+The TCB can expose multiple subdevices:
+ * a clocksource and clockevent device  

This still uses Linux terminology.

quoted

+   - compatible: Should be "atmel,tcb-free-running-timer"
+   - reg: Should contain the TCB channels to be used. If the
+     counter width is 16 bits (at91rm9200-tcb), two consecutive
+     channels are needed. Else, only one channel will be used.
+
+ * a clockevent device
+   - compatible: Should be "atmel,tcb-programmable-timer"  

This still looks like assigning usage in DT. As I'm willing to accept 
that for PWM, either timer channels should be whatever channels are not 
assigned to PWM (i.e. not in DT) or they should just be "timer" and let 
the kernel decide their usage.

I just reviewed Alexandre's new binding, and it makes the whole thing
a lot more obscure: on older SoCs, we have to chain 2 channels to
create an acceptable wraparound time (16 bits at 5MHz is generating too
much interrupts to be acceptable).

If we don't assign the mode from the DT, how should we know which
channels should be chained to create the free-running timer? Note that
not all channels can be chained together: they have to be part of the
same timer counter block and have to be consecutive (0+1, 1+2 or 3+0).

Honestly, I don't see the difference between assigning the channel to a
PWM mode and assigning it to a free-running or oneshot timer mode. Why
is it more acceptable?

On Mon, Jul 4, 2016 at 8:10 AM, Boris Brezillon
[off-list ref] wrote:

On Thu, 30 Jun 2016 20:27:43 -0500
Rob Herring [off-list ref] wrote:

quoted

On Mon, Jun 27, 2016 at 05:05:01PM +0200, Alexandre Belloni wrote:

quoted

The current binding for the TCB is not flexible enough for some use cases
and prevents proper utilization of all the channels.

Cc: Daniel Lezcano <redacted>
Cc: Thierry Reding <redacted>
Cc: linux-pwm at vger.kernel.org
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree at vger.kernel.org
Signed-off-by: Alexandre Belloni <redacted>
---

Changes in v2:
 - added slow_clk in the examples
 - removed linuxisms

Rob, does that fit what you wanted? I prefer getting your ack on that one before
resending a 48 patches series.


 .../devicetree/bindings/arm/atmel-at91.txt         | 32 -----------
 .../devicetree/bindings/mfd/atmel-tcb.txt          | 62 ++++++++++++++++++++++
 .../devicetree/bindings/pwm/atmel-tcb-pwm.txt      | 12 +++--
 3 files changed, 69 insertions(+), 37 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/atmel-tcb.txt

diff --git a/Documentation/devicetree/bindings/arm/atmel-at91.txt b/Documentation/devicetree/bindings/arm/atmel-at91.txt
index e1f5ad855f14..3dc758403d03 100644
--- a/Documentation/devicetree/bindings/arm/atmel-at91.txt
+++ b/Documentation/devicetree/bindings/arm/atmel-at91.txt

@@ -60,38 +60,6 @@ System Timer (ST) required properties:
 Its subnodes can be:
 - watchdog: compatible should be "atmel,at91rm9200-wdt"

-TC/TCLIB Timer required properties:
-- compatible: Should be "atmel,<chip>-tcb".
-  <chip> can be "at91rm9200" or "at91sam9x5"
-- reg: Should contain registers location and length
-- interrupts: Should contain all interrupts for the TC block
-  Note that you can specify several interrupt cells if the TC
-  block has one interrupt per channel.
-- clock-names: tuple listing input clock names.
-   Required elements: "t0_clk", "slow_clk"
-   Optional elements: "t1_clk", "t2_clk"
-- clocks: phandles to input clocks.
-
-Examples:
-
-One interrupt per TC block:
-   tcb0: timer at fff7c000 {
-           compatible = "atmel,at91rm9200-tcb";
-           reg = <0xfff7c000 0x100>;
-           interrupts = <18 4>;
-           clocks = <&tcb0_clk>;
-           clock-names = "t0_clk";
-   };
-
-One interrupt per TC channel in a TC block:
-   tcb1: timer at fffdc000 {
-           compatible = "atmel,at91rm9200-tcb";
-           reg = <0xfffdc000 0x100>;
-           interrupts = <26 4 27 4 28 4>;
-           clocks = <&tcb1_clk>;
-           clock-names = "t0_clk";
-   };
-
 RSTC Reset Controller required properties:
 - compatible: Should be "atmel,<chip>-rstc".
   <chip> can be "at91sam9260" or "at91sam9g45" or "sama5d3"

diff --git a/Documentation/devicetree/bindings/mfd/atmel-tcb.txt b/Documentation/devicetree/bindings/mfd/atmel-tcb.txt
new file mode 100644
index 000000000000..71c8d83c01a7
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/atmel-tcb.txt

@@ -0,0 +1,62 @@
+* Device tree bindings for Atmel Timer Counter Blocks
+- compatible: Should be "atmel,<chip>-tcb", "simple-mfd", "syscon".
+  <chip> can be "at91rm9200" or "at91sam9x5"
+- reg: Should contain registers location and length
+- #address-cells: has to be 1
+- #size-cells: has to be 0
+- interrupts: Should contain all interrupts for the TC block
+  Note that you can specify several interrupt cells if the TC
+  block has one interrupt per channel.
+- clock-names: tuple listing input clock names.
+   Required elements: "t0_clk", "slow_clk"
+   Optional elements: "t1_clk", "t2_clk"
+- clocks: phandles to input clocks.
+
+The TCB can expose multiple subdevices:
+ * a clocksource and clockevent device

This still uses Linux terminology.

quoted

+   - compatible: Should be "atmel,tcb-free-running-timer"
+   - reg: Should contain the TCB channels to be used. If the
+     counter width is 16 bits (at91rm9200-tcb), two consecutive
+     channels are needed. Else, only one channel will be used.
+
+ * a clockevent device
+   - compatible: Should be "atmel,tcb-programmable-timer"

This still looks like assigning usage in DT. As I'm willing to accept
that for PWM, either timer channels should be whatever channels are not
assigned to PWM (i.e. not in DT) or they should just be "timer" and let
the kernel decide their usage.

I just reviewed Alexandre's new binding, and it makes the whole thing
a lot more obscure: on older SoCs, we have to chain 2 channels to
create an acceptable wraparound time (16 bits at 5MHz is generating too
much interrupts to be acceptable).

If we don't assign the mode from the DT, how should we know which
channels should be chained to create the free-running timer? Note that
not all channels can be chained together: they have to be part of the
same timer counter block and have to be consecutive (0+1, 1+2 or 3+0).

The driver can have this knowledge if it is just picking 2 consecutive
timers. It should already know it has 16-bit timers based on the
compatible string. If it gets more complicated then the features or
limitations of the channels should be listed so the driver can make a
choice. OMAP is a good example of lots of timers with differing
features.

Honestly, I don't see the difference between assigning the channel to a
PWM mode and assigning it to a free-running or oneshot timer mode. Why
is it more acceptable?

The difference is that pwm's have clients (e.g. a backlight) in DT and
timers do not. We could just make the pwm clients reference the parent
node, but then it is hard to figure out which channels are in use for
pwm. That could also be solved with a simple "pwm-channels" property
listing the channels in use.

Rob

On 05/07/2016 at 10:40:22 -0500, Rob Herring wrote :

quoted

quoted

quoted

+   - compatible: Should be "atmel,tcb-free-running-timer"
+   - reg: Should contain the TCB channels to be used. If the
+     counter width is 16 bits (at91rm9200-tcb), two consecutive
+     channels are needed. Else, only one channel will be used.
+
+ * a clockevent device
+   - compatible: Should be "atmel,tcb-programmable-timer"

This still looks like assigning usage in DT. As I'm willing to accept
that for PWM, either timer channels should be whatever channels are not
assigned to PWM (i.e. not in DT) or they should just be "timer" and let
the kernel decide their usage.

I just reviewed Alexandre's new binding, and it makes the whole thing
a lot more obscure: on older SoCs, we have to chain 2 channels to
create an acceptable wraparound time (16 bits at 5MHz is generating too
much interrupts to be acceptable).

If we don't assign the mode from the DT, how should we know which
channels should be chained to create the free-running timer? Note that
not all channels can be chained together: they have to be part of the
same timer counter block and have to be consecutive (0+1, 1+2 or 3+0).

The driver can have this knowledge if it is just picking 2 consecutive
timers. It should already know it has 16-bit timers based on the
compatible string. If it gets more complicated then the features or
limitations of the channels should be listed so the driver can make a
choice. OMAP is a good example of lots of timers with differing
features.

quoted

Honestly, I don't see the difference between assigning the channel to a
PWM mode and assigning it to a free-running or oneshot timer mode. Why
is it more acceptable?

The difference is that pwm's have clients (e.g. a backlight) in DT and
timers do not. We could just make the pwm clients reference the parent
node, but then it is hard to figure out which channels are in use for
pwm. That could also be solved with a simple "pwm-channels" property
listing the channels in use.

Well, at some point, we will probably have the ADC referring to channels
used as timers as they can be used to trigger it periodically.

We'll also get frequency measurement that will go in iio, quadrature
decoders, gray counters and event counters that will probably used by
different subsystems and will certainly have to be referred to in DT.

It will definitively feel a bit weird to have all the channels have
their own nodes but not the timers...

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Hi Rob,

Sorry to revive this old discussion, but there's still one aspect I'm
not sure about.

On Tue, 5 Jul 2016 10:40:22 -0500
Rob Herring [off-list ref] wrote:

quoted

quoted

quoted

+   - compatible: Should be "atmel,tcb-free-running-timer"
+   - reg: Should contain the TCB channels to be used. If the
+     counter width is 16 bits (at91rm9200-tcb), two consecutive
+     channels are needed. Else, only one channel will be used.
+
+ * a clockevent device
+   - compatible: Should be "atmel,tcb-programmable-timer"  

This still looks like assigning usage in DT. As I'm willing to accept
that for PWM, either timer channels should be whatever channels are not
assigned to PWM (i.e. not in DT) or they should just be "timer" and let
the kernel decide their usage.  

I just reviewed Alexandre's new binding, and it makes the whole thing
a lot more obscure: on older SoCs, we have to chain 2 channels to
create an acceptable wraparound time (16 bits at 5MHz is generating too
much interrupts to be acceptable).

If we don't assign the mode from the DT, how should we know which
channels should be chained to create the free-running timer? Note that
not all channels can be chained together: they have to be part of the
same timer counter block and have to be consecutive (0+1, 1+2 or 3+0).  

The driver can have this knowledge if it is just picking 2 consecutive
timers. It should already know it has 16-bit timers based on the
compatible string. If it gets more complicated then the features or
limitations of the channels should be listed so the driver can make a
choice. OMAP is a good example of lots of timers with differing
features.

Yes it's possible to do that, but what about DT overlays then? Say you
have some TCB channels you'd like to reserve because they are connected
to pins that are exposed on your board. Those pins are not connected to
any device yet, but extension boards can be added, and in this case you
might want to expose new PWM devices by dynamically loading DT overlays.

If your clksource/clkevent driver parsed the initial DT and picked X
free channels randomly, it may conflicts with the one requested by the
DT overlay.

What's your solution for this case?

Thanks,

Boris

Le 25/01/2017 ? 16:11, Boris Brezillon a ?crit :

Hi Rob,

Sorry to revive this old discussion, but there's still one aspect I'm
not sure about.

On Tue, 5 Jul 2016 10:40:22 -0500
Rob Herring [off-list ref] wrote:

quoted

quoted

quoted

quoted

+   - compatible: Should be "atmel,tcb-free-running-timer"
+   - reg: Should contain the TCB channels to be used. If the
+     counter width is 16 bits (at91rm9200-tcb), two consecutive
+     channels are needed. Else, only one channel will be used.
+
+ * a clockevent device
+   - compatible: Should be "atmel,tcb-programmable-timer"  

This still looks like assigning usage in DT. As I'm willing to accept
that for PWM, either timer channels should be whatever channels are not
assigned to PWM (i.e. not in DT) or they should just be "timer" and let
the kernel decide their usage.  

I just reviewed Alexandre's new binding, and it makes the whole thing
a lot more obscure: on older SoCs, we have to chain 2 channels to
create an acceptable wraparound time (16 bits at 5MHz is generating too
much interrupts to be acceptable).

If we don't assign the mode from the DT, how should we know which
channels should be chained to create the free-running timer? Note that
not all channels can be chained together: they have to be part of the
same timer counter block and have to be consecutive (0+1, 1+2 or 3+0).  

The driver can have this knowledge if it is just picking 2 consecutive
timers. It should already know it has 16-bit timers based on the
compatible string. If it gets more complicated then the features or
limitations of the channels should be listed so the driver can make a
choice. OMAP is a good example of lots of timers with differing
features.

Yes it's possible to do that, but what about DT overlays then? Say you
have some TCB channels you'd like to reserve because they are connected
to pins that are exposed on your board. Those pins are not connected to
any device yet, but extension boards can be added, and in this case you
might want to expose new PWM devices by dynamically loading DT overlays.

If your clksource/clkevent driver parsed the initial DT and picked X
free channels randomly, it may conflicts with the one requested by the
DT overlay.

What's your solution for this case?

It seems that we don't have any progress on this topic for more than 6
months which is a pity as we now experience an issue that would have
been addressed completely by the TC rework [1].

aka "ping"... ;-)

Best regards,

[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2017-March/492080.html
-- 
Nicolas Ferre

On 13/03/2017 16:18, Nicolas Ferre wrote:

Le 25/01/2017 ? 16:11, Boris Brezillon a ?crit :

quoted

Hi Rob,

Sorry to revive this old discussion, but there's still one aspect I'm
not sure about.

On Tue, 5 Jul 2016 10:40:22 -0500
Rob Herring [off-list ref] wrote:

quoted

quoted

quoted

quoted

+   - compatible: Should be "atmel,tcb-free-running-timer"
+   - reg: Should contain the TCB channels to be used. If the
+     counter width is 16 bits (at91rm9200-tcb), two consecutive
+     channels are needed. Else, only one channel will be used.
+
+ * a clockevent device
+   - compatible: Should be "atmel,tcb-programmable-timer"  

This still looks like assigning usage in DT. As I'm willing to accept
that for PWM, either timer channels should be whatever channels are not
assigned to PWM (i.e. not in DT) or they should just be "timer" and let
the kernel decide their usage.  

I just reviewed Alexandre's new binding, and it makes the whole thing
a lot more obscure: on older SoCs, we have to chain 2 channels to
create an acceptable wraparound time (16 bits at 5MHz is generating too
much interrupts to be acceptable).

If we don't assign the mode from the DT, how should we know which
channels should be chained to create the free-running timer? Note that
not all channels can be chained together: they have to be part of the
same timer counter block and have to be consecutive (0+1, 1+2 or 3+0).  

The driver can have this knowledge if it is just picking 2 consecutive
timers. It should already know it has 16-bit timers based on the
compatible string. If it gets more complicated then the features or
limitations of the channels should be listed so the driver can make a
choice. OMAP is a good example of lots of timers with differing
features.

Yes it's possible to do that, but what about DT overlays then? Say you
have some TCB channels you'd like to reserve because they are connected
to pins that are exposed on your board. Those pins are not connected to
any device yet, but extension boards can be added, and in this case you
might want to expose new PWM devices by dynamically loading DT overlays.

If your clksource/clkevent driver parsed the initial DT and picked X
free channels randomly, it may conflicts with the one requested by the
DT overlay.

What's your solution for this case?

It seems that we don't have any progress on this topic for more than 6
months which is a pity as we now experience an issue that would have
been addressed completely by the TC rework [1].

aka "ping"... ;-)

Hi Nicolas, Boris,

is there any news from your side ?

Thanks.

  -- Daniel


-- 
 <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

On 07/04/2017 at 14:15:36 +0200, Daniel Lezcano wrote:

quoted

quoted

What's your solution for this case?

It seems that we don't have any progress on this topic for more than 6
months which is a pity as we now experience an issue that would have
been addressed completely by the TC rework [1].

aka "ping"... ;-)

Hi Nicolas, Boris,

is there any news from your side ?

I'm planning to resubmit news bindings and the full rework when I'll
have time to work on that. But I must admit I don't quite see the issue
with the proposed bindings.

Also, we are starting to see more and more flexible timer IPs that can
be used as RTC, timer, pwm, counters, quad decoders. I'm starting to
think we may need a new subsystem.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com