| Message ID | 1467409925-17761-1-git-send-email-alexandre.belloni@free-electrons.com |
|---|---|
| State | Rejected |
| Headers | show |
On Fri, 1 Jul 2016 23:52:05 +0200 Alexandre Belloni <alexandre.belloni@free-electrons.com> 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@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 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@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>; > + }; And how can you differentiate the clocksource from the clkevent? > + }; > + > +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>; > + }; > }; -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/07/2016 at 12:24:52 +0200, Boris Brezillon wrote : > On Fri, 1 Jul 2016 23:52:05 +0200 > Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > > +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>; > > + }; > > And how can you differentiate the clocksource from the clkevent? > It doesn't really matter actually, I'll do the selection in the driver, as suggested by Rob.
On Mon, 4 Jul 2016 12:36:31 +0200 Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > On 04/07/2016 at 12:24:52 +0200, Boris Brezillon wrote : > > On Fri, 1 Jul 2016 23:52:05 +0200 > > Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > > > +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>; > > > + }; > > > > And how can you differentiate the clocksource from the clkevent? > > > > It doesn't really matter actually, I'll do the selection in the driver, > as suggested by Rob. > Yes, I've read Rob's review, but then what's the point of defining 2 timer nodes, just do the detection based on the number of channels you've reserved for the timer and define a single node. -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/07/2016 at 14:03:58 +0200, Boris Brezillon wrote : > On Mon, 4 Jul 2016 12:36:31 +0200 > Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > > > On 04/07/2016 at 12:24:52 +0200, Boris Brezillon wrote : > > > On Fri, 1 Jul 2016 23:52:05 +0200 > > > Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > > > > +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>; > > > > + }; > > > > > > And how can you differentiate the clocksource from the clkevent? > > > > > > > It doesn't really matter actually, I'll do the selection in the driver, > > as suggested by Rob. > > > > Yes, I've read Rob's review, but then what's the point of defining 2 > timer nodes, just do the detection based on the number of channels > you've reserved for the timer and define a single node. I agree this is a really hypothetical use case but one may want to have the clocksource on one TCB and the clockevent on another. This would allow to have for example a quadrature decoder and the clocksource on one TCB and another quadrature decoder and the clockevent device on another TCB.
On Mon, 4 Jul 2016 14:11:27 +0200 Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > On 04/07/2016 at 14:03:58 +0200, Boris Brezillon wrote : > > On Mon, 4 Jul 2016 12:36:31 +0200 > > Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > > > > > On 04/07/2016 at 12:24:52 +0200, Boris Brezillon wrote : > > > > On Fri, 1 Jul 2016 23:52:05 +0200 > > > > Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > > > > > +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>; > > > > > + }; > > > > > > > > And how can you differentiate the clocksource from the clkevent? > > > > > > > > > > It doesn't really matter actually, I'll do the selection in the driver, > > > as suggested by Rob. > > > > > > > Yes, I've read Rob's review, but then what's the point of defining 2 > > timer nodes, just do the detection based on the number of channels > > you've reserved for the timer and define a single node. > > I agree this is a really hypothetical use case but one may want to have > the clocksource on one TCB and the clockevent on another. This would > allow to have for example a quadrature decoder and the clocksource on > one TCB and another quadrature decoder and the clockevent device on > another TCB. > > Hm, that sounds like a valid but unlikely use case, but it makes the DT bindinds even more obscure. When should one define a single timer node, when to define two, how many channels per timer depending on the type of timer you want to instantiate, etc. That should probably be documented. ITOH, by choosing the single timer-node approach, you hide everything in the driver, and let the implementation choose the correct channels for the clkevent and clksource devices. -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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>; + }; };
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