[PATCH 00/58] ARM: at91: rework Atmel TCB drivers

[PATCH 00/58] ARM: at91: rework Atmel TCB drivers

[Alexandre Belloni]

Hi,

This series reworks the Atmel Timer counter Block drivers. Those blocks
each have 3 counters with 2 channels each and can be used for
multiple functions:
 - timers
 - PWMs
 - Quadrature decoders
 - Stepper motor counters

Up until now, each TCB was fully used by each driver, possibly wasting
counters/channels.

There is a second issue motivating that rework. Until now, the PIT is
still used to boot then later in the boot sequence, the clocksource is
switched to the TCB. This ends up not working well with preempt-rt
because on some SoCs, the PIT interrupt is shared with the DBGU uart.
When using preempt-rt the interrupt flags for the PIT and the DBGU end
up being incompatible.

The rework breaks the DT ABI. Backward compatibility can be kept by
keeping tcb_clksrc and atmel_tclib but as AVR32 is now gone from the
kernel, I don't think it makes much sense to keep them.

Also, there is no other choice than breaking the mainly unused
pwm-atmel-tcb binding. Only the kizbox is actually using it.

I think the bindings are now ok and I hope we can take the DT changes
for 4.13.

Cc: Antoine Aubert <a.aubert-ZNYIgs0QAGpBDgjK7y7TUQ@public.gmane.org>
Cc: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: Douglas Gilbert <dgilbert-qazKcTl6WRFWk0Htik3J/w@public.gmane.org>
Cc: Fabio Porcedda <fabio.porcedda-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Cc: Gregory Hermant <gregory.hermant-WVCdZa36k+6/3pe1ocb+swC/G2K4zDHf@public.gmane.org>
Cc: Joachim Eastwood <manabian-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: linux-pwm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
Cc: Martin Reimann <martin.reimann-inJ12LbcDfezQB+pC5nmwQ@public.gmane.org>
Cc: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
Cc: Raashid Muhammed <raashidmuhammed-1MQmoylvZ5VBDgjK7y7TUQ@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Rodolfo Giometti <giometti-k2GhghHVRtY@public.gmane.org>
Cc: Sergio Tanzilli <tanzilli-ryiEoTCBuMXrBCbmRaR9Nw@public.gmane.org>
Cc: Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
Cc: Tim Schendekehl <tim.schendekehl-inJ12LbcDfezQB+pC5nmwQ@public.gmane.org>

Alexandre Belloni (58):
  ARM: at91: Document new TCB bindings
  ARM: dts: at91: at91rm9200: TC blocks are also simple-mfd and syscon
    devices
  ARM: dts: at91: at91rm9200ek: use TCB0 as clocksource
  ARM: dts: at91: mpa1600: use TCB0 as clocksource
  ARM: dts: at91: at91sam9260: TC blocks are also simple-mfd and syscon
    devices
  ARM: dts: at91: at91sam9260ek: use TCB0 as clocksource
  ARM: dts: at91: sam9_l9260: use TCB0 as clocksource
  ARM: dts: at91: ethernut5: use TCB0 as clocksource
  ARM: dts: at91: foxg20: use TCB0 as clocksource
  ARM: dts: at91: animeo_ip: use TCB0 as clocksource
  ARM: dts: at91: kizbox: use TCB0 as clocksource
  ARM: dts: at91: at91sam9g20ek: use TCB0 as clocksource
  ARM: dts: at91: ge863-pro3: use TCB0 as clocksource
  ARM: dts: at91: at91sam9261: TC blocks are also simple-mfd and syscon
    devices
  ARM: dts: at91: at91sam9261ek: use TCB0 as clocksource
  ARM: dts: at91: at91sam9263: TC blocks are also simple-mfd and syscon
    devices
  ARM: dts: at91: at91sam9263ek: use TCB0 as clocksource
  ARM: dts: at91: calao: use TCB0 as clocksource
  ARM: dts: at91: at91sam9g45: TC blocks are also simple-mfd and syscon
    devices
  ARM: dts: at91: at91sam9m10g45ek: use TCB0 as clocksource
  ARM: dts: at91: pm9g45: use TCB0 as clocksource
  ARM: dts: at91: at91sam9rl: TC blocks are also simple-mfd and syscon
    devices
  ARM: dts: at91: at91sam9rlek: use TCB0 as clocksource
  ARM: dts: at91: at91sam9n12: TC blocks are also simple-mfd and syscon
    devices
  ARM: dts: at91: at91sam9n12ek: use TCB0 as clocksource
  ARM: dts: at91: at91sam9x5: TC blocks are also simple-mfd and syscon
    devices
  ARM: dts: at91: at91sam9x5cm: use TCB0 as clocksource
  ARM: dts: at91: acme/g25: use TCB0 as clocksource
  ARM: dts: at91: cosino: use TCB0 as clocksource
  ARM: dts: at91: kizboxmini: use TCB0 as clocksource
  ARM: dts: at91: sama5d3: TC blocks are also simple-mfd and syscon
    devices
  ARM: dts: at91: sama5d3xek: use TCB0 as clocksource
  ARM: dts: at91: sama5d3 Xplained: use TCB0 as clocksource
  ARM: dts: at91: kizbox2: use TCB0 as clocksource
  ARM: dts: at91: sama5d3xek_cmp: use TCB0 as clocksource
  ARM: dts: at91: linea/tse850-3: use TCB0 as clocksource
  ARM: dts: at91: sama5d4: TC blocks are also simple-mfd and syscon
    devices
  ARM: dts: at91: sama5d4: Add TCB2
  ARM: dts: at91: sama5d4ek: use TCB2 as clocksource
  ARM: dts: at91: sama5d4 Xplained: use TCB2 as clocksource
  ARM: dts: at91: ma5d4: use TCB2 as clocksource
  ARM: dts: at91: vinco: use TCB2 as clocksource
  ARM: dts: at91: sama5d2: TC blocks are also simple-mfd and syscon
    devices
  ARM: dts: at91: sama5d2 Xplained: use TCB0 as clocksource
  ARM: at91: add TCB registers definitions
  clocksource/drivers: Add a new driver for the Atmel ARM TC blocks
  clocksource/drivers: timer-atmel-tcbclksrc: add clockevent device
  clocksource/drivers: timer-atmel-tcbclksrc: add clockevent device on
    separate channel
  clocksource/drivers: atmel-pit: allow unselecting ATMEL_PIT
  ARM: at91/defconfig: sama5: unselect ATMEL_PIT
  ARM: at91/defconfig: at91_dt unselect ATMEL_PIT
  PWM: atmel-tcb: switch to new binding
  ARM: dts: at91: kizbox: switch to new pwm-atmel-tcb binding
  clocksource/drivers: remove tcb_clksrc
  misc: remove atmel_tclib.c
  ARM: configs: at91: remove ATMEL_TCLIB
  ARM: multi_v7_defconfig: Remove ATMEL_TCLIB Kconfig symbol
  ARM: multi_v5_defconfig: Remove ATMEL_TCLIB Kconfig symbol

 .../devicetree/bindings/arm/atmel-at91.txt         |  32 --
 .../devicetree/bindings/mfd/atmel-tcb.txt          |  58 +++
 .../devicetree/bindings/pwm/atmel-tcb-pwm.txt      |  12 +-
 arch/arm/boot/dts/animeo_ip.dts                    |  12 +
 arch/arm/boot/dts/at91-ariag25.dts                 |  12 +
 arch/arm/boot/dts/at91-ariettag25.dts              |  12 +
 arch/arm/boot/dts/at91-cosino.dtsi                 |  12 +
 arch/arm/boot/dts/at91-foxg20.dts                  |  12 +
 arch/arm/boot/dts/at91-kizbox.dts                  |  54 ++-
 arch/arm/boot/dts/at91-kizbox2.dts                 |  12 +
 arch/arm/boot/dts/at91-kizboxmini.dts              |  12 +
 arch/arm/boot/dts/at91-linea.dtsi                  |  12 +
 arch/arm/boot/dts/at91-qil_a9260.dts               |  12 +
 arch/arm/boot/dts/at91-sam9_l9260.dts              |  12 +
 arch/arm/boot/dts/at91-sama5d2_xplained.dts        |  12 +
 arch/arm/boot/dts/at91-sama5d3_xplained.dts        |  12 +
 arch/arm/boot/dts/at91-sama5d4_ma5d4.dtsi          |  12 +
 arch/arm/boot/dts/at91-sama5d4_xplained.dts        |  12 +
 arch/arm/boot/dts/at91-sama5d4ek.dts               |  12 +
 arch/arm/boot/dts/at91-vinco.dts                   |  12 +
 arch/arm/boot/dts/at91rm9200.dtsi                  |   8 +-
 arch/arm/boot/dts/at91rm9200ek.dts                 |  12 +
 arch/arm/boot/dts/at91sam9260.dtsi                 |   8 +-
 arch/arm/boot/dts/at91sam9260ek.dts                |  12 +
 arch/arm/boot/dts/at91sam9261.dtsi                 |   4 +-
 arch/arm/boot/dts/at91sam9261ek.dts                |  12 +
 arch/arm/boot/dts/at91sam9263.dtsi                 |   4 +-
 arch/arm/boot/dts/at91sam9263ek.dts                |  12 +
 arch/arm/boot/dts/at91sam9g20ek_common.dtsi        |  12 +
 arch/arm/boot/dts/at91sam9g45.dtsi                 |   8 +-
 arch/arm/boot/dts/at91sam9m10g45ek.dts             |  12 +
 arch/arm/boot/dts/at91sam9n12.dtsi                 |   8 +-
 arch/arm/boot/dts/at91sam9n12ek.dts                |  12 +
 arch/arm/boot/dts/at91sam9rl.dtsi                  |   4 +-
 arch/arm/boot/dts/at91sam9rlek.dts                 |  12 +
 arch/arm/boot/dts/at91sam9x5.dtsi                  |   8 +-
 arch/arm/boot/dts/at91sam9x5cm.dtsi                |  12 +
 arch/arm/boot/dts/ethernut5.dts                    |  12 +
 arch/arm/boot/dts/ge863-pro3.dtsi                  |  12 +
 arch/arm/boot/dts/mpa1600.dts                      |  12 +
 arch/arm/boot/dts/pm9g45.dts                       |  12 +
 arch/arm/boot/dts/sama5d2.dtsi                     |   8 +-
 arch/arm/boot/dts/sama5d3.dtsi                     |   4 +-
 arch/arm/boot/dts/sama5d3_tcb1.dtsi                |   4 +-
 arch/arm/boot/dts/sama5d3xcm.dtsi                  |  12 +
 arch/arm/boot/dts/sama5d3xcm_cmp.dtsi              |  12 +
 arch/arm/boot/dts/sama5d4.dtsi                     |  18 +-
 arch/arm/boot/dts/tny_a9260_common.dtsi            |  12 +
 arch/arm/boot/dts/tny_a9263.dts                    |  12 +
 arch/arm/boot/dts/usb_a9260_common.dtsi            |  12 +
 arch/arm/boot/dts/usb_a9263.dts                    |  12 +
 arch/arm/configs/at91_dt_defconfig                 |   2 +-
 arch/arm/configs/multi_v5_defconfig                |   1 -
 arch/arm/configs/multi_v7_defconfig                |   1 -
 arch/arm/configs/sama5_defconfig                   |   2 +-
 drivers/clocksource/Kconfig                        |  22 +-
 drivers/clocksource/Makefile                       |   2 +-
 drivers/clocksource/tcb_clksrc.c                   | 381 ----------------
 drivers/clocksource/timer-atmel-tcbclksrc.c        | 502 +++++++++++++++++++++
 drivers/misc/Kconfig                               |  33 --
 drivers/misc/Makefile                              |   1 -
 drivers/misc/atmel_tclib.c                         | 198 --------
 drivers/pwm/Kconfig                                |   3 +-
 drivers/pwm/pwm-atmel-tcb.c                        | 221 ++++-----
 include/linux/atmel_tc.h                           |   9 -
 include/soc/at91/atmel_tcb.h                       | 229 ++++++++++
 66 files changed, 1468 insertions(+), 801 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/atmel-tcb.txt
 delete mode 100644 drivers/clocksource/tcb_clksrc.c
 create mode 100644 drivers/clocksource/timer-atmel-tcbclksrc.c
 delete mode 100644 drivers/misc/atmel_tclib.c
 create mode 100644 include/soc/at91/atmel_tcb.h

-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 01/58] ARM: at91: Document new TCB bindings

[Alexandre Belloni]

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 <daniel.lezcano@linaro.org>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: linux-pwm@vger.kernel.org
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 .../devicetree/bindings/arm/atmel-at91.txt         | 32 ------------
 .../devicetree/bindings/mfd/atmel-tcb.txt          | 58 ++++++++++++++++++++++
 .../devicetree/bindings/pwm/atmel-tcb-pwm.txt      | 12 +++--
 3 files changed, 65 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 799af90dd75b..44798554e855 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@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@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..693c7361e1ba
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/atmel-tcb.txt
@@ -0,0 +1,58 @@
+* 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 timer
+   - compatible: Should be "atmel,tcb-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 PWM chip: see ../pwm/atmel-tcb-pwm.txt
+
+Examples:
+
+One interrupt per TC block:
+	tcb0: timer@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@0 {
+			compatible = "atmel,tcb-timer";
+			reg = <0>, <1>;
+		};
+
+		timer@2 {
+			compatible = "atmel,tcb-timer";
+			reg = <2>;
+		};
+	};
+
+One interrupt per TC channel in a TC block:
+	tcb1: timer@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@f800c000 {
+	pwm@0 {
+		compatible = "atmel,tcb-pwm";
+		reg = <0>;
+		#pwm-cells = <3>;
+	};
 };
-- 
2.11.0

Re: [PATCH 00/58] ARM: at91: rework Atmel TCB drivers

[Peter Rosin]

On 2017-05-30 23:50, Alexandre Belloni wrote:
> Hi,
> 
> This series reworks the Atmel Timer counter Block drivers. Those blocks
> each have 3 counters with 2 channels each and can be used for
> multiple functions:
>  - timers
>  - PWMs
>  - Quadrature decoders
>  - Stepper motor counters

I'd like to test this, but I have a hard time doing that since all I got
was this cover letter and a single DT-only patch.

I understand that you were reluctant to spam too many people with
dozens of patches, but perhaps the series can be fetched from
somewhere?

Cheers,
peda

Re: [PATCH 00/58] ARM: at91: rework Atmel TCB drivers

[Alexandre Belloni]

Hi,

On 31/05/2017 at 08:34:00 +0200, Peter Rosin wrote:
> On 2017-05-30 23:50, Alexandre Belloni wrote:
> > Hi,
> > 
> > This series reworks the Atmel Timer counter Block drivers. Those blocks
> > each have 3 counters with 2 channels each and can be used for
> > multiple functions:
> >  - timers
> >  - PWMs
> >  - Quadrature decoders
> >  - Stepper motor counters
> 
> I'd like to test this, but I have a hard time doing that since all I got
> was this cover letter and a single DT-only patch.
> 
> I understand that you were reluctant to spam too many people with
> dozens of patches, but perhaps the series can be fetched from
> somewhere?
> 

Sure, I've pushed the branch here:
https://github.com/alexandrebelloni/linux/tree/at91-tcb-v3

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

Re: [PATCH 01/58] ARM: at91: Document new TCB bindings

[Rob Herring]

On Tue, May 30, 2017 at 11:50:42PM +0200, Alexandre Belloni wrote:
> 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 <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: linux-pwm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Signed-off-by: Alexandre Belloni <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  .../devicetree/bindings/arm/atmel-at91.txt         | 32 ------------
>  .../devicetree/bindings/mfd/atmel-tcb.txt          | 58 ++++++++++++++++++++++
>  .../devicetree/bindings/pwm/atmel-tcb-pwm.txt      | 12 +++--
>  3 files changed, 65 insertions(+), 37 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/mfd/atmel-tcb.txt

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 46/58] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks

[Boris Brezillon]

Le Thu, 8 Jun 2017 01:17:15 +0200,
Alexandre Belloni <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> a écrit :

> On 07/06/2017 at 23:08:48 +0200, Daniel Lezcano wrote:
> > > I was going to agree but this is not flexible enough because the
> > > quadrature decoder always uses the first two channels. So on some
> > > products, we may have:
> > >  - TCB0:
> > >    o channels 0,1: qdec
> > >    o channel 2: clocksource
> > > 
> > >  - TCB1:
> > >    o channels 0,1: qdec
> > >    o channel 2: clockevent
> > > 
> > > This avoids wasting TCB channels.  
> > 
> > Ok. In this case you can check if the interrupt is specified for the node, if
> > yes, then it is a clockevent.
> >   
> 
> But currently it is always specified in the SoC's dtsi. I don't find
> that too practical to push that to the board's dts. Also, lying by
> omission (the IRQ is always wired) in the DT is not different from
> having a property selecting which timer is the clocksource and which is
> the clockevent.
> 

I agree with Alexandre here. Really, there's not much we can do to
detect which timer should be used as a clockevent and which one should
be used as a clocksource except explicitly specifying it in the DT.
Having an interrupt defined in one case (clockevent) and undefined in
the other case (clocksource), is just as hack-ish as the detection logic
Alexandre developed to avoid explicitly specifying the function
assigned to a specific timer.

Can we please find a solution that makes everyone happy (DT,
clocksoure/clockevent and at91 maintainers)?

How about adding a linux,timer-function property to specify which
function this timer is providing?

Something like that for example:

	tcb0: timer@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@0 {
			compatible = "atmel,tcb-timer";
			reg = <0>, <1>;
			linux,timer-function = "clocksource";
		};

		timer@2 {
			compatible = "atmel,tcb-timer";
			reg = <2>;
			linux,timer-function = "clockevent";
		};
	};

Alternatively, we could have a property or a node in chosen describing which
timer should be used:

	chosen {
		clockevent {
			timer = <&timer2>;
		};

		clocksource {
			timer = <&timer0>;
		};

		/*
		 * or
		 *
		 * clockevent = <&timer2>;
		 * clocksource = <&timer0>;
		 *
		 * but I think the clocksource/clockevent node approach
		 * is more future proof in case we need to add extra
		 * information like the expected resolution/precision or
		 * anything that could be tweakable.
		 */
	};

	tcb0: timer@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";

		timer0: timer@0 {
			compatible = "atmel,tcb-timer";
			reg = <0>, <1>;
		};

		timer2: timer@2 {
			compatible = "atmel,tcb-timer";
			reg = <2>;
		};
	};
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 46/58] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks

[Daniel Lezcano]

+Mark Rutland, +Rob Herring


Alexandre, Boris, have a look at https://www.spinics.net/lists/arm-kernel/msg572652.html

That will tell you the story.

On Thu, Jun 08, 2017 at 07:42:36AM +0200, Boris Brezillon wrote:
> Le Thu, 8 Jun 2017 01:17:15 +0200,
> Alexandre Belloni <alexandre.belloni@free-electrons.com> a écrit :
> 
> > On 07/06/2017 at 23:08:48 +0200, Daniel Lezcano wrote:
> > > > I was going to agree but this is not flexible enough because the
> > > > quadrature decoder always uses the first two channels. So on some
> > > > products, we may have:
> > > >  - TCB0:
> > > >    o channels 0,1: qdec
> > > >    o channel 2: clocksource
> > > > 
> > > >  - TCB1:
> > > >    o channels 0,1: qdec
> > > >    o channel 2: clockevent
> > > > 
> > > > This avoids wasting TCB channels.  
> > > 
> > > Ok. In this case you can check if the interrupt is specified for the node, if
> > > yes, then it is a clockevent.
> > >   
> > 
> > But currently it is always specified in the SoC's dtsi. I don't find
> > that too practical to push that to the board's dts. Also, lying by
> > omission (the IRQ is always wired) in the DT is not different from
> > having a property selecting which timer is the clocksource and which is
> > the clockevent.
> > 
> 
> I agree with Alexandre here. Really, there's not much we can do to
> detect which timer should be used as a clockevent and which one should
> be used as a clocksource except explicitly specifying it in the DT.
> Having an interrupt defined in one case (clockevent) and undefined in
> the other case (clocksource), is just as hack-ish as the detection logic
> Alexandre developed to avoid explicitly specifying the function
> assigned to a specific timer.
> 
> Can we please find a solution that makes everyone happy (DT,
> clocksoure/clockevent and at91 maintainers)?
> 
> How about adding a linux,timer-function property to specify which
> function this timer is providing?
> 
> Something like that for example:
> 
> 	tcb0: timer@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@0 {
> 			compatible = "atmel,tcb-timer";
> 			reg = <0>, <1>;
> 			linux,timer-function = "clocksource";
> 		};
> 
> 		timer@2 {
> 			compatible = "atmel,tcb-timer";
> 			reg = <2>;
> 			linux,timer-function = "clockevent";
> 		};
> 	};
> 
> Alternatively, we could have a property or a node in chosen describing which
> timer should be used:
> 
> 	chosen {
> 		clockevent {
> 			timer = <&timer2>;
> 		};
> 
> 		clocksource {
> 			timer = <&timer0>;
> 		};
> 
> 		/*
> 		 * or
> 		 *
> 		 * clockevent = <&timer2>;
> 		 * clocksource = <&timer0>;
> 		 *
> 		 * but I think the clocksource/clockevent node approach
> 		 * is more future proof in case we need to add extra
> 		 * information like the expected resolution/precision or
> 		 * anything that could be tweakable.
> 		 */
> 	};
> 
> 	tcb0: timer@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";
> 
> 		timer0: timer@0 {
> 			compatible = "atmel,tcb-timer";
> 			reg = <0>, <1>;
> 		};
> 
> 		timer2: timer@2 {
> 			compatible = "atmel,tcb-timer";
> 			reg = <2>;
> 		};
> 	};

-- 

 <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

Re: [PATCH 46/58] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks

[Alexandre Belloni]

On 08/06/2017 at 09:44:46 +0200, Daniel Lezcano wrote:
> 
> +Mark Rutland, +Rob Herring
> 
> 
> Alexandre, Boris, have a look at https://www.spinics.net/lists/arm-kernel/msg572652.html
> 
> That will tell you the story.
> 

Ok, so is the solution putting the driver back in mach-at91 were we can
do whatever we want like mach-omap2 is doing?

> On Thu, Jun 08, 2017 at 07:42:36AM +0200, Boris Brezillon wrote:
> > Le Thu, 8 Jun 2017 01:17:15 +0200,
> > Alexandre Belloni <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> a écrit :
> > 
> > > On 07/06/2017 at 23:08:48 +0200, Daniel Lezcano wrote:
> > > > > I was going to agree but this is not flexible enough because the
> > > > > quadrature decoder always uses the first two channels. So on some
> > > > > products, we may have:
> > > > >  - TCB0:
> > > > >    o channels 0,1: qdec
> > > > >    o channel 2: clocksource
> > > > > 
> > > > >  - TCB1:
> > > > >    o channels 0,1: qdec
> > > > >    o channel 2: clockevent
> > > > > 
> > > > > This avoids wasting TCB channels.  
> > > > 
> > > > Ok. In this case you can check if the interrupt is specified for the node, if
> > > > yes, then it is a clockevent.
> > > >   
> > > 
> > > But currently it is always specified in the SoC's dtsi. I don't find
> > > that too practical to push that to the board's dts. Also, lying by
> > > omission (the IRQ is always wired) in the DT is not different from
> > > having a property selecting which timer is the clocksource and which is
> > > the clockevent.
> > > 
> > 
> > I agree with Alexandre here. Really, there's not much we can do to
> > detect which timer should be used as a clockevent and which one should
> > be used as a clocksource except explicitly specifying it in the DT.
> > Having an interrupt defined in one case (clockevent) and undefined in
> > the other case (clocksource), is just as hack-ish as the detection logic
> > Alexandre developed to avoid explicitly specifying the function
> > assigned to a specific timer.
> > 
> > Can we please find a solution that makes everyone happy (DT,
> > clocksoure/clockevent and at91 maintainers)?
> > 
> > How about adding a linux,timer-function property to specify which
> > function this timer is providing?
> > 
> > Something like that for example:
> > 
> > 	tcb0: timer@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@0 {
> > 			compatible = "atmel,tcb-timer";
> > 			reg = <0>, <1>;
> > 			linux,timer-function = "clocksource";
> > 		};
> > 
> > 		timer@2 {
> > 			compatible = "atmel,tcb-timer";
> > 			reg = <2>;
> > 			linux,timer-function = "clockevent";
> > 		};
> > 	};
> > 
> > Alternatively, we could have a property or a node in chosen describing which
> > timer should be used:
> > 
> > 	chosen {
> > 		clockevent {
> > 			timer = <&timer2>;
> > 		};
> > 
> > 		clocksource {
> > 			timer = <&timer0>;
> > 		};
> > 
> > 		/*
> > 		 * or
> > 		 *
> > 		 * clockevent = <&timer2>;
> > 		 * clocksource = <&timer0>;
> > 		 *
> > 		 * but I think the clocksource/clockevent node approach
> > 		 * is more future proof in case we need to add extra
> > 		 * information like the expected resolution/precision or
> > 		 * anything that could be tweakable.
> > 		 */
> > 	};
> > 
> > 	tcb0: timer@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";
> > 
> > 		timer0: timer@0 {
> > 			compatible = "atmel,tcb-timer";
> > 			reg = <0>, <1>;
> > 		};
> > 
> > 		timer2: timer@2 {
> > 			compatible = "atmel,tcb-timer";
> > 			reg = <2>;
> > 		};
> > 	};
> 
> -- 
> 
>  <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

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 46/58] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks

[Boris Brezillon]

On Thu, 8 Jun 2017 09:44:46 +0200
Daniel Lezcano <daniel.lezcano@linaro.org> wrote:

> +Mark Rutland, +Rob Herring

Mark doesn't seem to be CCed.

> 
> 
> Alexandre, Boris, have a look at https://www.spinics.net/lists/arm-kernel/msg572652.html
> 
> That will tell you the story.

Then we're in a deadlock situation here. I'm tired of hearing this kind
of argument "DT is only supposed to describe HW, not configuration, bla
bla". The truth is, we already have plenty of bindings that do not
strictly describe HW.

A simple example: ECC configuration on NAND devices. This is clearly a
configuration thing, the NAND controller is usually able to support
several kind of strength+ECC-block-size config, but we are able to
overload this with the nand-ecc-xxx properties. Another example, still
MTD related: MTD partitions, this is purely a software configuration,
still we allow users to pass this information in the DT. You want
another one? What about the linux,code and linux,input-type properties
described here [1]?

So please, let's not use these "this is not decribing HW" or "this is
linux specific" arguments every time someone tries to encode something
that can be considered a configuration detail.

Let's be pragmatic. How you want to use your timer counter blocks (I'm
talking about atmel TCBs) is clearly board specific. Whether you want
to use the PIT for your clocksource or use one or 2 channels of a TCB
at a specific resolution is again board specific. We need a solution to
assign timer channels to a linux function, and I'm not convinced
passing this information through the command line makes much more sense
than specifying it in the DT (and it's definitely less intuitive, since
you have to reference something defined in the DT from the cmdline).

Now, in his review, Mark says:

"
To me it sounds like what we need is Linux infrastructure that allows
one to register a device as having both clockevent/clocksource
functionality.

That way, we can choose to do something sane at boot time, and if the
user really wants specific devices used in specific ways, they can
request that.
"

Does that mean that, after adding this "HW timer" infrastructure, we
would have a standard way to assign a function to a specific timer
block from the DT? How is this different from what I suggest below
(note the linux, prefix on my linux,timer-function property, which
clearly shows that this is Linux specific)?

> 
> On Thu, Jun 08, 2017 at 07:42:36AM +0200, Boris Brezillon wrote:
> > Le Thu, 8 Jun 2017 01:17:15 +0200,
> > Alexandre Belloni <alexandre.belloni@free-electrons.com> a écrit :
> >   
> > > On 07/06/2017 at 23:08:48 +0200, Daniel Lezcano wrote:  
> > > > > I was going to agree but this is not flexible enough because the
> > > > > quadrature decoder always uses the first two channels. So on some
> > > > > products, we may have:
> > > > >  - TCB0:
> > > > >    o channels 0,1: qdec
> > > > >    o channel 2: clocksource
> > > > > 
> > > > >  - TCB1:
> > > > >    o channels 0,1: qdec
> > > > >    o channel 2: clockevent
> > > > > 
> > > > > This avoids wasting TCB channels.    
> > > > 
> > > > Ok. In this case you can check if the interrupt is specified for the node, if
> > > > yes, then it is a clockevent.
> > > >     
> > > 
> > > But currently it is always specified in the SoC's dtsi. I don't find
> > > that too practical to push that to the board's dts. Also, lying by
> > > omission (the IRQ is always wired) in the DT is not different from
> > > having a property selecting which timer is the clocksource and which is
> > > the clockevent.
> > >   
> > 
> > I agree with Alexandre here. Really, there's not much we can do to
> > detect which timer should be used as a clockevent and which one should
> > be used as a clocksource except explicitly specifying it in the DT.
> > Having an interrupt defined in one case (clockevent) and undefined in
> > the other case (clocksource), is just as hack-ish as the detection logic
> > Alexandre developed to avoid explicitly specifying the function
> > assigned to a specific timer.
> > 
> > Can we please find a solution that makes everyone happy (DT,
> > clocksoure/clockevent and at91 maintainers)?
> > 
> > How about adding a linux,timer-function property to specify which
> > function this timer is providing?
> > 
> > Something like that for example:
> > 
> > 	tcb0: timer@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@0 {
> > 			compatible = "atmel,tcb-timer";
> > 			reg = <0>, <1>;
> > 			linux,timer-function = "clocksource";
> > 		};
> > 
> > 		timer@2 {
> > 			compatible = "atmel,tcb-timer";
> > 			reg = <2>;
> > 			linux,timer-function = "clockevent";
> > 		};
> > 	};
> > 
> > Alternatively, we could have a property or a node in chosen describing which
> > timer should be used:
> > 
> > 	chosen {
> > 		clockevent {
> > 			timer = <&timer2>;
> > 		};
> > 
> > 		clocksource {
> > 			timer = <&timer0>;
> > 		};
> > 
> > 		/*
> > 		 * or
> > 		 *
> > 		 * clockevent = <&timer2>;
> > 		 * clocksource = <&timer0>;
> > 		 *
> > 		 * but I think the clocksource/clockevent node approach
> > 		 * is more future proof in case we need to add extra
> > 		 * information like the expected resolution/precision or
> > 		 * anything that could be tweakable.
> > 		 */
> > 	};
> > 
> > 	tcb0: timer@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";
> > 
> > 		timer0: timer@0 {
> > 			compatible = "atmel,tcb-timer";
> > 			reg = <0>, <1>;
> > 		};
> > 
> > 		timer2: timer@2 {
> > 			compatible = "atmel,tcb-timer";
> > 			reg = <2>;
> > 		};
> > 	};  
> 

[1]http://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/input/gpio-keys.txt

Re: [PATCH 46/58] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks

[Daniel Lezcano]

On Thu, Jun 08, 2017 at 09:59:01AM +0200, Alexandre Belloni wrote:
> On 08/06/2017 at 09:44:46 +0200, Daniel Lezcano wrote:
> > 
> > +Mark Rutland, +Rob Herring
> > 
> > 
> > Alexandre, Boris, have a look at https://www.spinics.net/lists/arm-kernel/msg572652.html
> > 
> > That will tell you the story.
> > 
> 
> Ok, so is the solution putting the driver back in mach-at91 were we can
> do whatever we want like mach-omap2 is doing?

No. And putting a driver in mach-<whatever> does not give the permission to do
whatever you want. I won't tell you how OSS works, but moving code around or
using another tree to circumvent a code review is just the best way to upset
maintainers in general and hurt your karma.

That said, I think you misunderstood my comment (or I was not clear). In the
discussion given in the link above, I am in favor, somehow, to distinguish
clockevent and clocksource to solve exactly what you are facing.

Rob Herring told me it could be acceptable to have a property to tell if it is
a clockevent or a clocksource.

Mark Rutland disagreed on this.

I was alone in the discussion, no consensus have been found.

Now, you have a particular use case and I would like to resurrect the
discussion in order to find a solution which can apply to all DT drivers.

> > On Thu, Jun 08, 2017 at 07:42:36AM +0200, Boris Brezillon wrote:
> > > Le Thu, 8 Jun 2017 01:17:15 +0200,
> > > Alexandre Belloni <alexandre.belloni@free-electrons.com> a écrit :
> > > 
> > > > On 07/06/2017 at 23:08:48 +0200, Daniel Lezcano wrote:
> > > > > > I was going to agree but this is not flexible enough because the
> > > > > > quadrature decoder always uses the first two channels. So on some
> > > > > > products, we may have:
> > > > > >  - TCB0:
> > > > > >    o channels 0,1: qdec
> > > > > >    o channel 2: clocksource
> > > > > > 
> > > > > >  - TCB1:
> > > > > >    o channels 0,1: qdec
> > > > > >    o channel 2: clockevent
> > > > > > 
> > > > > > This avoids wasting TCB channels.  
> > > > > 
> > > > > Ok. In this case you can check if the interrupt is specified for the node, if
> > > > > yes, then it is a clockevent.
> > > > >   
> > > > 
> > > > But currently it is always specified in the SoC's dtsi. I don't find
> > > > that too practical to push that to the board's dts. Also, lying by
> > > > omission (the IRQ is always wired) in the DT is not different from
> > > > having a property selecting which timer is the clocksource and which is
> > > > the clockevent.
> > > > 
> > > 
> > > I agree with Alexandre here. Really, there's not much we can do to
> > > detect which timer should be used as a clockevent and which one should
> > > be used as a clocksource except explicitly specifying it in the DT.
> > > Having an interrupt defined in one case (clockevent) and undefined in
> > > the other case (clocksource), is just as hack-ish as the detection logic
> > > Alexandre developed to avoid explicitly specifying the function
> > > assigned to a specific timer.
> > > 
> > > Can we please find a solution that makes everyone happy (DT,
> > > clocksoure/clockevent and at91 maintainers)?
> > > 
> > > How about adding a linux,timer-function property to specify which
> > > function this timer is providing?
> > > 
> > > Something like that for example:
> > > 
> > > 	tcb0: timer@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@0 {
> > > 			compatible = "atmel,tcb-timer";
> > > 			reg = <0>, <1>;
> > > 			linux,timer-function = "clocksource";
> > > 		};
> > > 
> > > 		timer@2 {
> > > 			compatible = "atmel,tcb-timer";
> > > 			reg = <2>;
> > > 			linux,timer-function = "clockevent";
> > > 		};
> > > 	};
> > > 
> > > Alternatively, we could have a property or a node in chosen describing which
> > > timer should be used:
> > > 
> > > 	chosen {
> > > 		clockevent {
> > > 			timer = <&timer2>;
> > > 		};
> > > 
> > > 		clocksource {
> > > 			timer = <&timer0>;
> > > 		};
> > > 
> > > 		/*
> > > 		 * or
> > > 		 *
> > > 		 * clockevent = <&timer2>;
> > > 		 * clocksource = <&timer0>;
> > > 		 *
> > > 		 * but I think the clocksource/clockevent node approach
> > > 		 * is more future proof in case we need to add extra
> > > 		 * information like the expected resolution/precision or
> > > 		 * anything that could be tweakable.
> > > 		 */
> > > 	};
> > > 
> > > 	tcb0: timer@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";
> > > 
> > > 		timer0: timer@0 {
> > > 			compatible = "atmel,tcb-timer";
> > > 			reg = <0>, <1>;
> > > 		};
> > > 
> > > 		timer2: timer@2 {
> > > 			compatible = "atmel,tcb-timer";
> > > 			reg = <2>;
> > > 		};
> > > 	};
> > 
> > -- 
> > 
> >  <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
> 
> -- 
> Alexandre Belloni, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

-- 

 <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

Re: [PATCH 46/58] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks

[Boris Brezillon]

On Thu, 8 Jun 2017 10:24:17 +0200
Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> On Thu, Jun 08, 2017 at 09:59:01AM +0200, Alexandre Belloni wrote:
> > On 08/06/2017 at 09:44:46 +0200, Daniel Lezcano wrote:  
> > > 
> > > +Mark Rutland, +Rob Herring
> > > 
> > > 
> > > Alexandre, Boris, have a look at https://www.spinics.net/lists/arm-kernel/msg572652.html
> > > 
> > > That will tell you the story.
> > >   
> > 
> > Ok, so is the solution putting the driver back in mach-at91 were we can
> > do whatever we want like mach-omap2 is doing?  
> 
> No. And putting a driver in mach-<whatever> does not give the permission to do
> whatever you want. I won't tell you how OSS works, but moving code around or
> using another tree to circumvent a code review is just the best way to upset
> maintainers in general and hurt your karma.
> 
> That said, I think you misunderstood my comment (or I was not clear). In the
> discussion given in the link above, I am in favor, somehow, to distinguish
> clockevent and clocksource to solve exactly what you are facing.
> 
> Rob Herring told me it could be acceptable to have a property to tell if it is
> a clockevent or a clocksource.
> 
> Mark Rutland disagreed on this.
> 
> I was alone in the discussion, no consensus have been found.

Indeed, I misunderstood your point.

> 
> Now, you have a particular use case and I would like to resurrect the
> discussion in order to find a solution which can apply to all DT drivers.

Ok, glad to see we're on the same page.

Mark, can we re-open the discussion?

Thanks,

Boris
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 46/58] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks

[Daniel Lezcano]

On Thu, Jun 08, 2017 at 10:13:34AM +0200, Boris Brezillon wrote:
> On Thu, 8 Jun 2017 09:44:46 +0200
> Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> 
> > +Mark Rutland, +Rob Herring
> 
> Mark doesn't seem to be CCed.

Ah, yes. Thanks for fixing this.

> > 
> > 
> > Alexandre, Boris, have a look at https://www.spinics.net/lists/arm-kernel/msg572652.html
> > 
> > That will tell you the story.
> 
> Then we're in a deadlock situation here. I'm tired of hearing this kind
> of argument "DT is only supposed to describe HW, not configuration, bla
> bla". The truth is, we already have plenty of bindings that do not
> strictly describe HW.
> 
> A simple example: ECC configuration on NAND devices. This is clearly a
> configuration thing, the NAND controller is usually able to support
> several kind of strength+ECC-block-size config, but we are able to
> overload this with the nand-ecc-xxx properties. Another example, still
> MTD related: MTD partitions, this is purely a software configuration,
> still we allow users to pass this information in the DT. You want
> another one? What about the linux,code and linux,input-type properties
> described here [1]?
> 
> So please, let's not use these "this is not decribing HW" or "this is
> linux specific" arguments every time someone tries to encode something
> that can be considered a configuration detail.
> 
> Let's be pragmatic. How you want to use your timer counter blocks (I'm
> talking about atmel TCBs) is clearly board specific. Whether you want
> to use the PIT for your clocksource or use one or 2 channels of a TCB
> at a specific resolution is again board specific. We need a solution to
> assign timer channels to a linux function, and I'm not convinced
> passing this information through the command line makes much more sense
> than specifying it in the DT (and it's definitely less intuitive, since
> you have to reference something defined in the DT from the cmdline).
> 
> Now, in his review, Mark says:
> 
> "
> To me it sounds like what we need is Linux infrastructure that allows
> one to register a device as having both clockevent/clocksource
> functionality.
> 
> That way, we can choose to do something sane at boot time, and if the
> user really wants specific devices used in specific ways, they can
> request that.
> "
> 
> Does that mean that, after adding this "HW timer" infrastructure, we
> would have a standard way to assign a function to a specific timer
> block from the DT? How is this different from what I suggest below
> (note the linux, prefix on my linux,timer-function property, which
> clearly shows that this is Linux specific)?

I like the 'chosen' approach with the nodes you are proposing below. Thanks for
the constructive suggestion. The binding description matches perfectly what we
are trying to achieve.

Rob? what do you think?

> > On Thu, Jun 08, 2017 at 07:42:36AM +0200, Boris Brezillon wrote:
> > > Le Thu, 8 Jun 2017 01:17:15 +0200,
> > > Alexandre Belloni <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> a écrit :
> > >   
> > > > On 07/06/2017 at 23:08:48 +0200, Daniel Lezcano wrote:  
> > > > > > I was going to agree but this is not flexible enough because the
> > > > > > quadrature decoder always uses the first two channels. So on some
> > > > > > products, we may have:
> > > > > >  - TCB0:
> > > > > >    o channels 0,1: qdec
> > > > > >    o channel 2: clocksource
> > > > > > 
> > > > > >  - TCB1:
> > > > > >    o channels 0,1: qdec
> > > > > >    o channel 2: clockevent
> > > > > > 
> > > > > > This avoids wasting TCB channels.    
> > > > > 
> > > > > Ok. In this case you can check if the interrupt is specified for the node, if
> > > > > yes, then it is a clockevent.
> > > > >     
> > > > 
> > > > But currently it is always specified in the SoC's dtsi. I don't find
> > > > that too practical to push that to the board's dts. Also, lying by
> > > > omission (the IRQ is always wired) in the DT is not different from
> > > > having a property selecting which timer is the clocksource and which is
> > > > the clockevent.
> > > >   
> > > 
> > > I agree with Alexandre here. Really, there's not much we can do to
> > > detect which timer should be used as a clockevent and which one should
> > > be used as a clocksource except explicitly specifying it in the DT.
> > > Having an interrupt defined in one case (clockevent) and undefined in
> > > the other case (clocksource), is just as hack-ish as the detection logic
> > > Alexandre developed to avoid explicitly specifying the function
> > > assigned to a specific timer.
> > > 
> > > Can we please find a solution that makes everyone happy (DT,
> > > clocksoure/clockevent and at91 maintainers)?
> > > 
> > > How about adding a linux,timer-function property to specify which
> > > function this timer is providing?
> > > 
> > > Something like that for example:
> > > 
> > > 	tcb0: timer@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@0 {
> > > 			compatible = "atmel,tcb-timer";
> > > 			reg = <0>, <1>;
> > > 			linux,timer-function = "clocksource";
> > > 		};
> > > 
> > > 		timer@2 {
> > > 			compatible = "atmel,tcb-timer";
> > > 			reg = <2>;
> > > 			linux,timer-function = "clockevent";
> > > 		};
> > > 	};
> > > 
> > > Alternatively, we could have a property or a node in chosen describing which
> > > timer should be used:
> > > 
> > > 	chosen {
> > > 		clockevent {
> > > 			timer = <&timer2>;
> > > 		};
> > > 
> > > 		clocksource {
> > > 			timer = <&timer0>;
> > > 		};
> > > 
> > > 		/*
> > > 		 * or
> > > 		 *
> > > 		 * clockevent = <&timer2>;
> > > 		 * clocksource = <&timer0>;
> > > 		 *
> > > 		 * but I think the clocksource/clockevent node approach
> > > 		 * is more future proof in case we need to add extra
> > > 		 * information like the expected resolution/precision or
> > > 		 * anything that could be tweakable.
> > > 		 */
> > > 	};
> > > 
> > > 	tcb0: timer@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";
> > > 
> > > 		timer0: timer@0 {
> > > 			compatible = "atmel,tcb-timer";
> > > 			reg = <0>, <1>;
> > > 		};
> > > 
> > > 		timer2: timer@2 {
> > > 			compatible = "atmel,tcb-timer";
> > > 			reg = <2>;
> > > 		};
> > > 	};  
> > 
> 
> [1]http://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/input/gpio-keys.txt

-- 

 <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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 46/58] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks

[Alexandre Belloni]

On 08/06/2017 at 10:24:17 +0200, Daniel Lezcano wrote:
> On Thu, Jun 08, 2017 at 09:59:01AM +0200, Alexandre Belloni wrote:
> > On 08/06/2017 at 09:44:46 +0200, Daniel Lezcano wrote:
> > > 
> > > +Mark Rutland, +Rob Herring
> > > 
> > > 
> > > Alexandre, Boris, have a look at https://www.spinics.net/lists/arm-kernel/msg572652.html
> > > 
> > > That will tell you the story.
> > > 
> > 
> > Ok, so is the solution putting the driver back in mach-at91 were we can
> > do whatever we want like mach-omap2 is doing?
> 
> No. And putting a driver in mach-<whatever> does not give the permission to do
> whatever you want. I won't tell you how OSS works, but moving code around or
> using another tree to circumvent a code review is just the best way to upset
> maintainers in general and hurt your karma.
> 

I know that but some SoCs will not be able to even boot until we have a
solution. And it has been one year since we raised the issue without any
solution coming from the DT maintainers.


> That said, I think you misunderstood my comment (or I was not clear). In the
> discussion given in the link above, I am in favor, somehow, to distinguish
> clockevent and clocksource to solve exactly what you are facing.
> 
> Rob Herring told me it could be acceptable to have a property to tell if it is
> a clockevent or a clocksource.
> 

Ok, then let's do it!


> Mark Rutland disagreed on this.
> 
> I was alone in the discussion, no consensus have been found.
> 
> Now, you have a particular use case and I would like to resurrect the
> discussion in order to find a solution which can apply to all DT drivers.
> 

Again, it has been one year and it seems nobody actually cares. There is
a whole family of SoCs that can't boot because of that. Should I resort
to the evil vendor tree argument again?




> > > On Thu, Jun 08, 2017 at 07:42:36AM +0200, Boris Brezillon wrote:
> > > > Le Thu, 8 Jun 2017 01:17:15 +0200,
> > > > Alexandre Belloni <alexandre.belloni@free-electrons.com> a écrit :
> > > > 
> > > > > On 07/06/2017 at 23:08:48 +0200, Daniel Lezcano wrote:
> > > > > > > I was going to agree but this is not flexible enough because the
> > > > > > > quadrature decoder always uses the first two channels. So on some
> > > > > > > products, we may have:
> > > > > > >  - TCB0:
> > > > > > >    o channels 0,1: qdec
> > > > > > >    o channel 2: clocksource
> > > > > > > 
> > > > > > >  - TCB1:
> > > > > > >    o channels 0,1: qdec
> > > > > > >    o channel 2: clockevent
> > > > > > > 
> > > > > > > This avoids wasting TCB channels.  
> > > > > > 
> > > > > > Ok. In this case you can check if the interrupt is specified for the node, if
> > > > > > yes, then it is a clockevent.
> > > > > >   
> > > > > 
> > > > > But currently it is always specified in the SoC's dtsi. I don't find
> > > > > that too practical to push that to the board's dts. Also, lying by
> > > > > omission (the IRQ is always wired) in the DT is not different from
> > > > > having a property selecting which timer is the clocksource and which is
> > > > > the clockevent.
> > > > > 
> > > > 
> > > > I agree with Alexandre here. Really, there's not much we can do to
> > > > detect which timer should be used as a clockevent and which one should
> > > > be used as a clocksource except explicitly specifying it in the DT.
> > > > Having an interrupt defined in one case (clockevent) and undefined in
> > > > the other case (clocksource), is just as hack-ish as the detection logic
> > > > Alexandre developed to avoid explicitly specifying the function
> > > > assigned to a specific timer.
> > > > 
> > > > Can we please find a solution that makes everyone happy (DT,
> > > > clocksoure/clockevent and at91 maintainers)?
> > > > 
> > > > How about adding a linux,timer-function property to specify which
> > > > function this timer is providing?
> > > > 
> > > > Something like that for example:
> > > > 
> > > > 	tcb0: timer@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@0 {
> > > > 			compatible = "atmel,tcb-timer";
> > > > 			reg = <0>, <1>;
> > > > 			linux,timer-function = "clocksource";
> > > > 		};
> > > > 
> > > > 		timer@2 {
> > > > 			compatible = "atmel,tcb-timer";
> > > > 			reg = <2>;
> > > > 			linux,timer-function = "clockevent";
> > > > 		};
> > > > 	};
> > > > 
> > > > Alternatively, we could have a property or a node in chosen describing which
> > > > timer should be used:
> > > > 
> > > > 	chosen {
> > > > 		clockevent {
> > > > 			timer = <&timer2>;
> > > > 		};
> > > > 
> > > > 		clocksource {
> > > > 			timer = <&timer0>;
> > > > 		};
> > > > 
> > > > 		/*
> > > > 		 * or
> > > > 		 *
> > > > 		 * clockevent = <&timer2>;
> > > > 		 * clocksource = <&timer0>;
> > > > 		 *
> > > > 		 * but I think the clocksource/clockevent node approach
> > > > 		 * is more future proof in case we need to add extra
> > > > 		 * information like the expected resolution/precision or
> > > > 		 * anything that could be tweakable.
> > > > 		 */
> > > > 	};
> > > > 
> > > > 	tcb0: timer@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";
> > > > 
> > > > 		timer0: timer@0 {
> > > > 			compatible = "atmel,tcb-timer";
> > > > 			reg = <0>, <1>;
> > > > 		};
> > > > 
> > > > 		timer2: timer@2 {
> > > > 			compatible = "atmel,tcb-timer";
> > > > 			reg = <2>;
> > > > 		};
> > > > 	};
> > > 
> > > -- 
> > > 
> > >  <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
> > 
> > -- 
> > Alexandre Belloni, Free Electrons
> > Embedded Linux and Kernel engineering
> > http://free-electrons.com
> 
> -- 
> 
>  <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

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

Re: [PATCH 46/58] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks

[Boris Brezillon]

On Thu, 8 Jun 2017 10:40:26 +0200
Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> > > 
> > > Alexandre, Boris, have a look at https://www.spinics.net/lists/arm-kernel/msg572652.html
> > > 
> > > That will tell you the story.  
> > 
> > Then we're in a deadlock situation here. I'm tired of hearing this kind
> > of argument "DT is only supposed to describe HW, not configuration, bla
> > bla". The truth is, we already have plenty of bindings that do not
> > strictly describe HW.
> > 
> > A simple example: ECC configuration on NAND devices. This is clearly a
> > configuration thing, the NAND controller is usually able to support
> > several kind of strength+ECC-block-size config, but we are able to
> > overload this with the nand-ecc-xxx properties. Another example, still
> > MTD related: MTD partitions, this is purely a software configuration,
> > still we allow users to pass this information in the DT. You want
> > another one? What about the linux,code and linux,input-type properties
> > described here [1]?
> > 
> > So please, let's not use these "this is not decribing HW" or "this is
> > linux specific" arguments every time someone tries to encode something
> > that can be considered a configuration detail.
> > 
> > Let's be pragmatic. How you want to use your timer counter blocks (I'm
> > talking about atmel TCBs) is clearly board specific. Whether you want
> > to use the PIT for your clocksource or use one or 2 channels of a TCB
> > at a specific resolution is again board specific. We need a solution to
> > assign timer channels to a linux function, and I'm not convinced
> > passing this information through the command line makes much more sense
> > than specifying it in the DT (and it's definitely less intuitive, since
> > you have to reference something defined in the DT from the cmdline).
> > 
> > Now, in his review, Mark says:
> > 
> > "
> > To me it sounds like what we need is Linux infrastructure that allows
> > one to register a device as having both clockevent/clocksource
> > functionality.
> > 
> > That way, we can choose to do something sane at boot time, and if the
> > user really wants specific devices used in specific ways, they can
> > request that.
> > "
> > 
> > Does that mean that, after adding this "HW timer" infrastructure, we
> > would have a standard way to assign a function to a specific timer
> > block from the DT? How is this different from what I suggest below
> > (note the linux, prefix on my linux,timer-function property, which
> > clearly shows that this is Linux specific)?  
> 
> I like the 'chosen' approach with the nodes you are proposing below. Thanks for
> the constructive suggestion. The binding description matches perfectly what we
> are trying to achieve.

Actually, this is Alexandre who initially suggested the chosen
approach (I thought it was important to mention that ;-)).
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 46/58] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks

[Nicolas Ferre]

Le 08/06/2017 à 10:40, Daniel Lezcano a écrit :
> On Thu, Jun 08, 2017 at 10:13:34AM +0200, Boris Brezillon wrote:
>> On Thu, 8 Jun 2017 09:44:46 +0200
>> Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>
>>> +Mark Rutland, +Rob Herring
>>
>> Mark doesn't seem to be CCed.
> 
> Ah, yes. Thanks for fixing this.
> 
>>>
>>>
>>> Alexandre, Boris, have a look at https://www.spinics.net/lists/arm-kernel/msg572652.html
>>>
>>> That will tell you the story.
>>
>> Then we're in a deadlock situation here. I'm tired of hearing this kind
>> of argument "DT is only supposed to describe HW, not configuration, bla
>> bla". The truth is, we already have plenty of bindings that do not
>> strictly describe HW.
>>
>> A simple example: ECC configuration on NAND devices. This is clearly a
>> configuration thing, the NAND controller is usually able to support
>> several kind of strength+ECC-block-size config, but we are able to
>> overload this with the nand-ecc-xxx properties. Another example, still
>> MTD related: MTD partitions, this is purely a software configuration,
>> still we allow users to pass this information in the DT. You want
>> another one? What about the linux,code and linux,input-type properties
>> described here [1]?
>>
>> So please, let's not use these "this is not decribing HW" or "this is
>> linux specific" arguments every time someone tries to encode something
>> that can be considered a configuration detail.
>>
>> Let's be pragmatic. How you want to use your timer counter blocks (I'm
>> talking about atmel TCBs) is clearly board specific. Whether you want
>> to use the PIT for your clocksource or use one or 2 channels of a TCB
>> at a specific resolution is again board specific. We need a solution to
>> assign timer channels to a linux function, and I'm not convinced
>> passing this information through the command line makes much more sense
>> than specifying it in the DT (and it's definitely less intuitive, since
>> you have to reference something defined in the DT from the cmdline).
>>
>> Now, in his review, Mark says:
>>
>> "
>> To me it sounds like what we need is Linux infrastructure that allows
>> one to register a device as having both clockevent/clocksource
>> functionality.
>>
>> That way, we can choose to do something sane at boot time, and if the
>> user really wants specific devices used in specific ways, they can
>> request that.
>> "
>>
>> Does that mean that, after adding this "HW timer" infrastructure, we
>> would have a standard way to assign a function to a specific timer
>> block from the DT? How is this different from what I suggest below
>> (note the linux, prefix on my linux,timer-function property, which
>> clearly shows that this is Linux specific)?
> 
> I like the 'chosen' approach with the nodes you are proposing below. Thanks for
> the constructive suggestion. The binding description matches perfectly what we
> are trying to achieve.
> 
> Rob? what do you think?

I'm following this work from a distance but as we've just celebrated the
1st anniversary for this patch series (11 June 2016), I propose that we
now make up our mind quickly. Everybody seem to be on the same page and
willing to make this rework move forward.

In Microchip/Atmel we would like to actually use this TCB rework both
internally and in our mainline work to avoid having to rely on our own
out-of-tree implementation.

The newly-added samv7 cortex-M can't boot without this series and a use
of our current cortex-A SoCs with TrustZone in Secure World (SWd) is not
possible with current mainline code only. On these two examples, the
current timer on which we rely, the PIT, is not present.

So you probably understand that more than one year without real progress
begins to be a little bit frustrating for the AT91 users...

Regards,


>>> On Thu, Jun 08, 2017 at 07:42:36AM +0200, Boris Brezillon wrote:
>>>> Le Thu, 8 Jun 2017 01:17:15 +0200,
>>>> Alexandre Belloni <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> a écrit :
>>>>   
>>>>> On 07/06/2017 at 23:08:48 +0200, Daniel Lezcano wrote:  
>>>>>>> I was going to agree but this is not flexible enough because the
>>>>>>> quadrature decoder always uses the first two channels. So on some
>>>>>>> products, we may have:
>>>>>>>  - TCB0:
>>>>>>>    o channels 0,1: qdec
>>>>>>>    o channel 2: clocksource
>>>>>>>
>>>>>>>  - TCB1:
>>>>>>>    o channels 0,1: qdec
>>>>>>>    o channel 2: clockevent
>>>>>>>
>>>>>>> This avoids wasting TCB channels.    
>>>>>>
>>>>>> Ok. In this case you can check if the interrupt is specified for the node, if
>>>>>> yes, then it is a clockevent.
>>>>>>     
>>>>>
>>>>> But currently it is always specified in the SoC's dtsi. I don't find
>>>>> that too practical to push that to the board's dts. Also, lying by
>>>>> omission (the IRQ is always wired) in the DT is not different from
>>>>> having a property selecting which timer is the clocksource and which is
>>>>> the clockevent.
>>>>>   
>>>>
>>>> I agree with Alexandre here. Really, there's not much we can do to
>>>> detect which timer should be used as a clockevent and which one should
>>>> be used as a clocksource except explicitly specifying it in the DT.
>>>> Having an interrupt defined in one case (clockevent) and undefined in
>>>> the other case (clocksource), is just as hack-ish as the detection logic
>>>> Alexandre developed to avoid explicitly specifying the function
>>>> assigned to a specific timer.
>>>>
>>>> Can we please find a solution that makes everyone happy (DT,
>>>> clocksoure/clockevent and at91 maintainers)?
>>>>
>>>> How about adding a linux,timer-function property to specify which
>>>> function this timer is providing?
>>>>
>>>> Something like that for example:
>>>>
>>>> 	tcb0: timer@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@0 {
>>>> 			compatible = "atmel,tcb-timer";
>>>> 			reg = <0>, <1>;
>>>> 			linux,timer-function = "clocksource";
>>>> 		};
>>>>
>>>> 		timer@2 {
>>>> 			compatible = "atmel,tcb-timer";
>>>> 			reg = <2>;
>>>> 			linux,timer-function = "clockevent";
>>>> 		};
>>>> 	};
>>>>
>>>> Alternatively, we could have a property or a node in chosen describing which
>>>> timer should be used:
>>>>
>>>> 	chosen {
>>>> 		clockevent {
>>>> 			timer = <&timer2>;
>>>> 		};
>>>>
>>>> 		clocksource {
>>>> 			timer = <&timer0>;
>>>> 		};
>>>>
>>>> 		/*
>>>> 		 * or
>>>> 		 *
>>>> 		 * clockevent = <&timer2>;
>>>> 		 * clocksource = <&timer0>;
>>>> 		 *
>>>> 		 * but I think the clocksource/clockevent node approach
>>>> 		 * is more future proof in case we need to add extra
>>>> 		 * information like the expected resolution/precision or
>>>> 		 * anything that could be tweakable.
>>>> 		 */
>>>> 	};
>>>>
>>>> 	tcb0: timer@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";
>>>>
>>>> 		timer0: timer@0 {
>>>> 			compatible = "atmel,tcb-timer";
>>>> 			reg = <0>, <1>;
>>>> 		};
>>>>
>>>> 		timer2: timer@2 {
>>>> 			compatible = "atmel,tcb-timer";
>>>> 			reg = <2>;
>>>> 		};
>>>> 	};  
>>>
>>
>> [1]http://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/input/gpio-keys.txt
> 


-- 
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 46/58] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks

[Daniel Lezcano]

On 12/06/2017 14:54, Nicolas Ferre wrote:

[ ... ]

>> I like the 'chosen' approach with the nodes you are proposing below. Thanks for
>> the constructive suggestion. The binding description matches perfectly what we
>> are trying to achieve.
>>
>> Rob? what do you think?
> 
> I'm following this work from a distance but as we've just celebrated the
> 1st anniversary for this patch series (11 June 2016), I propose that we
> now make up our mind quickly. Everybody seem to be on the same page and
> willing to make this rework move forward.
> 
> In Microchip/Atmel we would like to actually use this TCB rework both
> internally and in our mainline work to avoid having to rely on our own
> out-of-tree implementation.
> 
> The newly-added samv7 cortex-M can't boot without this series and a use
> of our current cortex-A SoCs with TrustZone in Secure World (SWd) is not
> possible with current mainline code only. On these two examples, the
> current timer on which we rely, the PIT, is not present.
> 
> So you probably understand that more than one year without real progress
> begins to be a little bit frustrating for the AT91 users...

Nicolas,

who are you exactly blaming?

Are you surprised a 58 patches series, with a gazillion of Cc'ed people
posted awhile ago, is ignored?




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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 46/58] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks

[Nicolas Ferre]

Le 12/06/2017 à 15:25, Daniel Lezcano a écrit :
> On 12/06/2017 14:54, Nicolas Ferre wrote:
> 
> [ ... ]
> 
>>> I like the 'chosen' approach with the nodes you are proposing below. Thanks for
>>> the constructive suggestion. The binding description matches perfectly what we
>>> are trying to achieve.
>>>
>>> Rob? what do you think?
>>
>> I'm following this work from a distance but as we've just celebrated the
>> 1st anniversary for this patch series (11 June 2016), I propose that we
>> now make up our mind quickly. Everybody seem to be on the same page and
>> willing to make this rework move forward.
>>
>> In Microchip/Atmel we would like to actually use this TCB rework both
>> internally and in our mainline work to avoid having to rely on our own
>> out-of-tree implementation.
>>
>> The newly-added samv7 cortex-M can't boot without this series and a use
>> of our current cortex-A SoCs with TrustZone in Secure World (SWd) is not
>> possible with current mainline code only. On these two examples, the
>> current timer on which we rely, the PIT, is not present.
>>
>> So you probably understand that more than one year without real progress
>> begins to be a little bit frustrating for the AT91 users...
> 
> Nicolas,
> 
> who are you exactly blaming?
> 
> Are you surprised a 58 patches series, with a gazillion of Cc'ed people
> posted awhile ago, is ignored?

Daniel,

Well, I'm talking about the only patch about DT bindings here, not the
other (large number) of soc/board dts changes.
Note that Alexandre tried to extract this discussion from the other
patches without coming to a conclusion either ("[PATCH v3] ARM: at91:
Document new TCB bindings", for instance).

I had the feeling that this email thread was about to be fading out on a
DT binding discussion, as it had done a couple of times during the last
months... Both you and me produced "ping" messages to make this
discussion progress with no real success so far... This is why I was a
little worried.

Regards,
-- 
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/58] ARM: at91: rework Atmel TCB drivers

[Thierry Reding]

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

On Tue, May 30, 2017 at 11:50:41PM +0200, Alexandre Belloni wrote:
> Hi,
> 
> This series reworks the Atmel Timer counter Block drivers. Those blocks
> each have 3 counters with 2 channels each and can be used for
> multiple functions:
>  - timers
>  - PWMs
>  - Quadrature decoders
>  - Stepper motor counters
> 
> Up until now, each TCB was fully used by each driver, possibly wasting
> counters/channels.
> 
> There is a second issue motivating that rework. Until now, the PIT is
> still used to boot then later in the boot sequence, the clocksource is
> switched to the TCB. This ends up not working well with preempt-rt
> because on some SoCs, the PIT interrupt is shared with the DBGU uart.
> When using preempt-rt the interrupt flags for the PIT and the DBGU end
> up being incompatible.
> 
> The rework breaks the DT ABI. Backward compatibility can be kept by
> keeping tcb_clksrc and atmel_tclib but as AVR32 is now gone from the
> kernel, I don't think it makes much sense to keep them.
> 
> Also, there is no other choice than breaking the mainly unused
> pwm-atmel-tcb binding. Only the kizbox is actually using it.
> 
> I think the bindings are now ok and I hope we can take the DT changes
> for 4.13.

Has anyone volunteered to pick this up? What are the dependencies here
and how did you plan to get this merged?

Thierry

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