Re: [PATCH v5 04/21] dt-bindings: Add doc for the Ingenic TCU drivers

Re: [PATCH v5 04/21] dt-bindings: Add doc for the Ingenic TCU drivers

[Daniel Lezcano]

On 03/10/2018 12:32, Paul Cercueil wrote:
> 
> Le 1 oct. 2018 10:48, Daniel Lezcano <daniel.lezcano@linaro.org> a
> écrit :
>> 
>> On 31/07/2018 00:01, Paul Cercueil wrote:
>> 
>> [ ... ]
>> 
>>>>> +- ingenic,timer-channel: Specifies the TCU channel that
>>>>> should be used as +  system timer. If not provided, the TCU
>>>>> channel 0 is used for the system timer. + +-
>>>>> ingenic,clocksource-channel: Specifies the TCU channel that 
>>>>> should be used +  as clocksource and sched_clock. It must be
>>>>> a different channel than the one +  used as system timer. If
>>>>> not provided, neither a clocksource nor a +  sched_clock is
>>>>> instantiated.
>>>> 
>>>> clocksource and sched_clock are Linux specific and don't belong
>>>> in DT. You should define properties of the hardware or use
>>>> existing properties like interrupts or clocks to figure out
>>>> which channel to use. For example, if some channels don't have
>>>> an interrupt, then use them for clocksource and not a
>>>> clockevent. Or you could have timers that run in low-power
>>>> modes or not. If all the channels are identical, then it 
>>>> shouldn't matter which ones the OS picks.
>> 
>> It can't work in this case because the pmw and the timer driver are
>> not communicating and the first one can stole a channel to the last
>> one.
> 
> In that particular case the timer driver will always request its
> channels first; with no timer set the system hangs before
> subsys_initcall, and the PWM driver is a subnode of the timer node,
> so is probed only after the timer probed.
> 
>>> We already talked about that. All the TCU channels can be used
>>> for PWM. The problem is I cannot know from the driver's scope
>>> which channels will be free and which channels will be requested
>>> for PWM. You suggested that I parse the devicetree for clients,
>>> and I did that in the V3/V4 patchset. But it only works for
>>> clients requesting through devicetree, not from platform code or
>>> even sysfs.
>>> 
>>> One thing I can try is to dynamically change the channels the
>>> system timer and clocksource are using when the current ones are
>>> requested for PWM. But that sounds hardcore...
>> 
>> Yes, it is :/
>> 
>> Sorry for letting you wasting time and effort to write an overkill
>> code not suitable for upstream.
>> 
>> A very gross thought, wouldn't be possible to "register" a channel
>> from the timer driver code in a shared data area (but well
>> self-encapsulated) and the pwm code will check such channel isn't
>> in use ?
> 
> Probably, but it's the contrary I need to do. The timer driver code
> can use any channel, and probes first. The PWM driver code must use
> specific channels, and probes last. So either the timer driver knows
> what channels it can't use, thanks to a device property, or it adapts
> itself when a channel in use is requested for PWM, which is what I
> tried in v7.

When you say "must use specific channels", where is coming this
information ?

> I think we could find a way to use a devicetree property that doesn't
> trigger Rob. That would still be the easiest and cleanest solution.
> 
> Maybe "ingenic,reserved-channels-mask", which would contain a mask of
> channels that can only be used by the timer driver. And what the
> timer driver does with these channels, would be specific to the
> implementation and would not appear in the bindings. I hope Rob can
> work with that.
> 
> -Paul
> 


-- 
 <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 v5 04/21] dt-bindings: Add doc for the Ingenic TCU drivers

[Lee Jones]

On Wed, 03 Oct 2018, Paul Cercueil wrote:
> Le 3 oct. 2018 3:02 PM, Daniel Lezcano <daniel.lezcano@linaro.org> a écrit :
> > On 03/10/2018 14:51, Paul Cercueil wrote: 
> > > 
> > > Le 3 oct. 2018 2:47 PM, Daniel Lezcano <daniel.lezcano@linaro.org> a 
> > > écrit : 
> > >> 
> > >> On 03/10/2018 12:32, Paul Cercueil wrote: 
> > >>> 
> > >>> Le 1 oct. 2018 10:48, Daniel Lezcano <daniel.lezcano@linaro.org> 
> > >>> a écrit : 
> > >>>> 
> > >>>> On 31/07/2018 00:01, Paul Cercueil wrote: 
> > >>>> 
> > >>>> [ ... ] 

Paul,

I think there is something wrong with your mailer since your replies
are being scattered across my inbox un-threaded, making conversations
very difficult to follow.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v5 04/21] dt-bindings: Add doc for the Ingenic TCU drivers

[Alexandre Belloni]

On 03/10/2018 12:32:51+0200, Paul Cercueil wrote:
> 
> Le 1 oct. 2018 10:48, Daniel Lezcano <daniel.lezcano@linaro.org> a écrit :
> >
> > On 31/07/2018 00:01, Paul Cercueil wrote: 
> >
> > [ ... ] 
> >
> > >>>  +- ingenic,timer-channel: Specifies the TCU channel that should be 
> > >>> used as 
> > >>>  +  system timer. If not provided, the TCU channel 0 is used for the 
> > >>> system timer. 
> > >>>  + 
> > >>>  +- ingenic,clocksource-channel: Specifies the TCU channel that 
> > >>> should be used 
> > >>>  +  as clocksource and sched_clock. It must be a different channel 
> > >>> than the one 
> > >>>  +  used as system timer. If not provided, neither a clocksource nor a 
> > >>>  +  sched_clock is instantiated. 
> > >> 
> > >> clocksource and sched_clock are Linux specific and don't belong in DT. 
> > >> You should define properties of the hardware or use existing properties 
> > >> like interrupts or clocks to figure out which channel to use. For 
> > >> example, if some channels don't have an interrupt, then use them for 
> > >> clocksource and not a clockevent. Or you could have timers that run in 
> > >> low-power modes or not. If all the channels are identical, then it 
> > >> shouldn't matter which ones the OS picks. 
> >
> > It can't work in this case because the pmw and the timer driver are not 
> > communicating and the first one can stole a channel to the last one. 
> 
> In that particular case the timer driver will always request its channels first; with no timer set the system hangs before subsys_initcall, and the PWM driver is a subnode of the timer node, so is probed only after the timer probed.
> 
> > > We already talked about that. All the TCU channels can be used for PWM. 
> > > The problem is I cannot know from the driver's scope which channels will 
> > > be free and which channels will be requested for PWM. You suggested that I 
> > > parse the devicetree for clients, and I did that in the V3/V4 patchset. But 
> > > it only works for clients requesting through devicetree, not from platform 
> > > code or even sysfs. 
> > > 
> > > One thing I can try is to dynamically change the channels the system timer 
> > > and clocksource are using when the current ones are requested for PWM. But 
> > > that sounds hardcore... 
> >
> > Yes, it is :/ 
> >
> > Sorry for letting you wasting time and effort to write an overkill code 
> > not suitable for upstream. 
> >
> > A very gross thought, wouldn't be possible to "register" a channel from 
> > the timer driver code in a shared data area (but well self-encapsulated) 
> > and the pwm code will check such channel isn't in use ? 
> 
> Probably, but it's the contrary I need to do. The timer driver code can use any channel, and probes first. The PWM driver code must use specific channels, and probes last. So either the timer driver knows what channels it can't use, thanks to a device property, or it adapts itself when a channel in use is requested for PWM, which is what I tried in v7.
> 
> I think we could find a way to use a devicetree property that doesn't trigger Rob. That would still be the easiest and cleanest solution. 
> 
> Maybe "ingenic,reserved-channels-mask", which would contain a mask of channels that can only be used by the timer driver. And what the timer driver does with these channels, would be specific to the implementation and would not appear in the bindings. I hope Rob can work with that.
> 

Rob did ack the following binding:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/mfd/atmel-tcb.txt

another subdevice is a PWM (not documented here).


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v5 04/21] dt-bindings: Add doc for the Ingenic TCU drivers

[Daniel Lezcano]

On 03/10/2018 14:51, Paul Cercueil wrote:
> 
> Le 3 oct. 2018 2:47 PM, Daniel Lezcano <daniel.lezcano@linaro.org> a
> écrit :
>> 
>> On 03/10/2018 12:32, Paul Cercueil wrote:
>>> 
>>> Le 1 oct. 2018 10:48, Daniel Lezcano <daniel.lezcano@linaro.org>
>>> a écrit :
>>>> 
>>>> On 31/07/2018 00:01, Paul Cercueil wrote:
>>>> 
>>>> [ ... ]
>>>> 
>>>>>>> +- ingenic,timer-channel: Specifies the TCU channel that
>>>>>>>  should be used as +  system timer. If not provided, the
>>>>>>> TCU channel 0 is used for the system timer. + +- 
>>>>>>> ingenic,clocksource-channel: Specifies the TCU channel
>>>>>>> that should be used +  as clocksource and sched_clock. It
>>>>>>> must be a different channel than the one +  used as
>>>>>>> system timer. If not provided, neither a clocksource nor
>>>>>>> a +  sched_clock is instantiated.
>>>>>> 
>>>>>> clocksource and sched_clock are Linux specific and don't
>>>>>> belong in DT. You should define properties of the hardware
>>>>>> or use existing properties like interrupts or clocks to
>>>>>> figure out which channel to use. For example, if some
>>>>>> channels don't have an interrupt, then use them for
>>>>>> clocksource and not a clockevent. Or you could have timers
>>>>>> that run in low-power modes or not. If all the channels are
>>>>>> identical, then it shouldn't matter which ones the OS
>>>>>> picks.
>>>> 
>>>> It can't work in this case because the pmw and the timer driver
>>>> are not communicating and the first one can stole a channel to
>>>> the last one.
>>> 
>>> In that particular case the timer driver will always request its
>>>  channels first; with no timer set the system hangs before 
>>> subsys_initcall, and the PWM driver is a subnode of the timer
>>> node, so is probed only after the timer probed.
>>> 
>>>>> We already talked about that. All the TCU channels can be
>>>>> used for PWM. The problem is I cannot know from the driver's
>>>>> scope which channels will be free and which channels will be
>>>>> requested for PWM. You suggested that I parse the devicetree
>>>>> for clients, and I did that in the V3/V4 patchset. But it
>>>>> only works for clients requesting through devicetree, not
>>>>> from platform code or even sysfs.
>>>>> 
>>>>> One thing I can try is to dynamically change the channels the
>>>>>  system timer and clocksource are using when the current ones
>>>>> are requested for PWM. But that sounds hardcore...
>>>> 
>>>> Yes, it is :/
>>>> 
>>>> Sorry for letting you wasting time and effort to write an
>>>> overkill code not suitable for upstream.
>>>> 
>>>> A very gross thought, wouldn't be possible to "register" a
>>>> channel from the timer driver code in a shared data area (but
>>>> well self-encapsulated) and the pwm code will check such
>>>> channel isn't in use ?
>>> 
>>> Probably, but it's the contrary I need to do. The timer driver
>>> code can use any channel, and probes first. The PWM driver code
>>> must use specific channels, and probes last. So either the timer
>>> driver knows what channels it can't use, thanks to a device
>>> property, or it adapts itself when a channel in use is requested
>>> for PWM, which is what I tried in v7.
>> 
>> When you say "must use specific channels", where is coming this 
>> information ?
> 
> If the backlight for the LCD is connected to the pin that corresponds
> to PWM1, then you must use the TCU channel 1. It's that simple.

Is it a runtime detection or is it hardcoded somewhere ?

(just trying to understand the whole picture)

>>> I think we could find a way to use a devicetree property that
>>> doesn't trigger Rob. That would still be the easiest and cleanest
>>> solution.
>>> 
>>> Maybe "ingenic,reserved-channels-mask", which would contain a
>>> mask of channels that can only be used by the timer driver. And
>>> what the timer driver does with these channels, would be specific
>>> to the implementation and would not appear in the bindings. I
>>> hope Rob can work with that.
>>> 
>>> -Paul


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

[PATCH v5 00/21] Ingenic JZ47xx TCU patchset v5

[Paul Cercueil]

Hi,

This is the V5 of my patchset that adds support to the Timer/Counter
Unit (TCU) present on Ingenic JZ47xx SoCs.

Way too much has changed since V4, we went from 8 patches in V4 up to
21 patches in V5.
I did not know if I had to submit it as V5 or start a new series.
In doubt, I sent a V5.

Very succint description of the changes:

- The interrupt, clocks, clocksource drivers have been merged into one;
  ingenic-timer now inits the TCU clocks, handles the interrupts,
  provides a system timer and optionally also a clocksource and
  sched_clock. The TCU channel #0 will be used as system timer, unless
  another channel is specified from devicetree.

- The simple-mfd and syscon are gone. The ingenic-timer driver will
  probe the drivers registered as children in the devicetree.

- The watchdog driver was updated to use the WDT clock and regmap
  provided by ingenic-timer. This breaks the devicetree ABI, but as
  explained in the corresponding commit message, right now all Ingenic
  boards compile the devicetree into the kernel anyway - so it's better
  to update it now before it's too late.

- The PWM driver was updated to use the TCU timer clocks as well as the
  regmap provided by ingenic-timer. Unused devicetree compatible strings
  were removed (the driver was never probed from devicetree), and
  support for the JZ4725B has been added.

- A new ingenic-ost driver was added, which provides a clocksource and
  a sched_clock that are more accurate than the ones provided by
  ingenic-timer (32 or 64-bit vs. 16-bit). It is not available on every
  SoC.

- Mostly tested on the JZ4725B and JZ4770. It would be great if somebody
  could test on the JZ4780 and JZ4740.

Thanks,
-Paul

[PATCH v5 04/21] dt-bindings: Add doc for the Ingenic TCU drivers

[Paul Cercueil]

Add documentation about how to properly use the Ingenic TCU
(Timer/Counter Unit) drivers from devicetree.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 .../devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt |  24 +---
 .../devicetree/bindings/timer/ingenic,tcu.txt      | 147 +++++++++++++++++++++
 .../bindings/watchdog/ingenic,jz4740-wdt.txt       |  17 +--
 3 files changed, 151 insertions(+), 37 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/timer/ingenic,tcu.txt

 v4: New patch in this series. Corresponds to V2 patches 3-4-5 with
     added content.

 v5: - Edited PWM/watchdog DT bindings documentation to point to the new
       document.
     - Moved main document to
       Documentation/devicetree/bindings/timer/ingenic,tcu.txt
     - Updated documentation to reflect the new devicetree bindings.

diff --git a/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt b/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
index 7d9d3f90641b..a722cdde3aa7 100644
--- a/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
@@ -1,25 +1,5 @@
 Ingenic JZ47xx PWM Controller
 =============================
 
-Required properties:
-- compatible: One of:
-  * "ingenic,jz4740-pwm"
-  * "ingenic,jz4770-pwm"
-  * "ingenic,jz4780-pwm"
-- #pwm-cells: Should be 3. See pwm.txt in this directory for a description
-  of the cells format.
-- clocks : phandle to the external clock.
-- clock-names : Should be "ext".
-
-
-Example:
-
-	pwm: pwm@10002000 {
-		compatible = "ingenic,jz4740-pwm";
-		reg = <0x10002000 0x1000>;
-
-		#pwm-cells = <3>;
-
-		clocks = <&ext>;
-		clock-names = "ext";
-	};
+This documentation has moved; for a description of the devicetree bindings of
+this driver, please refer to ../timer/ingenic,tcu.txt.
diff --git a/Documentation/devicetree/bindings/timer/ingenic,tcu.txt b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
new file mode 100644
index 000000000000..65d125b460aa
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
@@ -0,0 +1,147 @@
+Ingenic JZ47xx SoCs Timer/Counter Unit devicetree bindings
+==========================================================
+
+For a description of the TCU hardware and drivers, have a look at
+Documentation/mips/ingenic-tcu.txt.
+
+Required properties:
+
+- compatible: Must be one of:
+  * ingenic,jz4740-tcu
+  * ingenic,jz4725b-tcu
+  * ingenic,jz4770-tcu
+- reg: Should be the offset/length value corresponding to the TCU registers
+- #address-cells: Should be <1>;
+- #size-cells: Should be <1>;
+- ranges: Should be one range for the full TCU registers area
+- clocks: List of phandle & clock specifiers for clocks external to the TCU.
+  The "pclk", "rtc", "ext" and "tcu" clocks should be provided.
+- clock-names: List of name strings for the external clocks.
+- #clock-cells: Should be <1>;
+  Clock consumers specify this argument to identify a clock. The valid values
+  may be found in <dt-bindings/clock/ingenic,tcu.h>.
+- interrupt-controller : Identifies the node as an interrupt controller
+- #interrupt-cells : Specifies the number of cells needed to encode an
+  interrupt source. The value should be 1.
+- interrupt-parent : phandle of the interrupt controller.
+- interrupts : Specifies the interrupt the controller is connected to.
+
+Optional properties:
+
+- ingenic,timer-channel: Specifies the TCU channel that should be used as
+  system timer. If not provided, the TCU channel 0 is used for the system timer.
+
+- ingenic,clocksource-channel: Specifies the TCU channel that should be used
+  as clocksource and sched_clock. It must be a different channel than the one
+  used as system timer. If not provided, neither a clocksource nor a
+  sched_clock is instantiated.
+
+
+Children nodes
+==========================================================
+
+
+PWM node:
+---------
+
+Required properties:
+
+- compatible: Must be one of:
+  * ingenic,jz4740-pwm
+  * ingenic,jz4725b-pwm
+- #pwm-cells: Should be 3. See ../pwm/pwm.txt for a description of the cell
+  format.
+- clocks: List of phandle & clock specifiers for the TCU clocks.
+- clock-names: List of name strings for the TCU clocks.
+
+
+Watchdog node:
+--------------
+
+Required properties:
+
+- compatible: Must be one of:
+  * ingenic,jz4740-watchdog
+  * ingenic,jz4780-watchdog
+- clocks: phandle to the RTC clock
+- clock-names: should be "rtc"
+
+
+OST node:
+---------
+
+Required properties:
+
+- compatible: Must be one of:
+  * ingenic,jz4725b-ost
+  * ingenic,jz4770-ost
+- clocks: phandle to the OST clock
+- clock-names: should be "ost"
+- interrupts : Specifies the interrupt the OST is connected to.
+
+
+Example
+==========================================================
+
+#include <dt-bindings/clock/jz4770-cgu.h>
+#include <dt-bindings/clock/ingenic,tcu.h>
+
+/ {
+	tcu: timer@10002000 {
+		compatible = "ingenic,jz4770-tcu";
+		reg = <0x10002000 0x1000>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0x0 0x10002000 0x1000>;
+
+		#clock-cells = <1>;
+
+		clocks = <&cgu JZ4770_CLK_RTC
+			  &cgu JZ4770_CLK_EXT
+			  &cgu JZ4770_CLK_PCLK
+			  &cgu JZ4770_CLK_EXT>;
+		clock-names = "rtc", "ext", "pclk", "tcu";
+
+		interrupt-controller;
+		#interrupt-cells = <1>;
+
+		interrupt-parent = <&intc>;
+		interrupts = <27 26 25>;
+
+		watchdog: watchdog@0 {
+			compatible = "ingenic,jz4740-watchdog";
+			reg = <0x0 0x10>;
+
+			clocks = <&tcu TCU_CLK_WDT>;
+			clock-names = "wdt";
+		};
+
+		pwm: pwm@10 {
+			compatible = "ingenic,jz4740-pwm";
+			reg = <0x10 0xff0>;
+
+			#pwm-cells = <3>;
+
+			clocks = <&tcu TCU_CLK_TIMER0
+				  &tcu TCU_CLK_TIMER1
+				  &tcu TCU_CLK_TIMER2
+				  &tcu TCU_CLK_TIMER3
+				  &tcu TCU_CLK_TIMER4
+				  &tcu TCU_CLK_TIMER5
+				  &tcu TCU_CLK_TIMER6
+				  &tcu TCU_CLK_TIMER7>;
+			clock-names = "timer0", "timer1", "timer2", "timer3",
+				      "timer4", "timer5", "timer6", "timer7";
+		};
+
+		ost: timer@e0 {
+			compatible = "ingenic,jz4770-ost";
+			reg = <0xe0 0x20>;
+
+			clocks = <&tcu TCU_CLK_OST>;
+			clock-names = "ost";
+
+			interrupts = <15>;
+		};
+	};
+};
diff --git a/Documentation/devicetree/bindings/watchdog/ingenic,jz4740-wdt.txt b/Documentation/devicetree/bindings/watchdog/ingenic,jz4740-wdt.txt
index ce1cb72d5345..b12ddc0c2b00 100644
--- a/Documentation/devicetree/bindings/watchdog/ingenic,jz4740-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/ingenic,jz4740-wdt.txt
@@ -1,17 +1,4 @@
 Ingenic Watchdog Timer (WDT) Controller for JZ4740 & JZ4780
 
-Required properties:
-compatible: "ingenic,jz4740-watchdog" or "ingenic,jz4780-watchdog"
-reg: Register address and length for watchdog registers
-clocks: phandle to the RTC clock
-clock-names: should be "rtc"
-
-Example:
-
-watchdog: jz4740-watchdog@10002000 {
-	compatible = "ingenic,jz4740-watchdog";
-	reg = <0x10002000 0x10>;
-
-	clocks = <&cgu JZ4740_CLK_RTC>;
-	clock-names = "rtc";
-};
+This documentation has moved; for a description of the devicetree bindings of
+this driver, please refer to ../timer/ingenic,tcu.txt.
-- 
2.11.0

Re: [PATCH v5 04/21] dt-bindings: Add doc for the Ingenic TCU drivers

[Rob Herring]

On Wed, Jul 25, 2018 at 01:19:41AM +0200, Paul Cercueil wrote:
> Add documentation about how to properly use the Ingenic TCU
> (Timer/Counter Unit) drivers from devicetree.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  .../devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt |  24 +---
>  .../devicetree/bindings/timer/ingenic,tcu.txt      | 147 +++++++++++++++++++++
>  .../bindings/watchdog/ingenic,jz4740-wdt.txt       |  17 +--
>  3 files changed, 151 insertions(+), 37 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/timer/ingenic,tcu.txt
> 
>  v4: New patch in this series. Corresponds to V2 patches 3-4-5 with
>      added content.
> 
>  v5: - Edited PWM/watchdog DT bindings documentation to point to the new
>        document.
>      - Moved main document to
>        Documentation/devicetree/bindings/timer/ingenic,tcu.txt
>      - Updated documentation to reflect the new devicetree bindings.
> 
> diff --git a/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt b/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
> index 7d9d3f90641b..a722cdde3aa7 100644
> --- a/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
> +++ b/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
> @@ -1,25 +1,5 @@
>  Ingenic JZ47xx PWM Controller
>  =============================
>  
> -Required properties:
> -- compatible: One of:
> -  * "ingenic,jz4740-pwm"
> -  * "ingenic,jz4770-pwm"
> -  * "ingenic,jz4780-pwm"
> -- #pwm-cells: Should be 3. See pwm.txt in this directory for a description
> -  of the cells format.
> -- clocks : phandle to the external clock.
> -- clock-names : Should be "ext".
> -
> -
> -Example:
> -
> -	pwm: pwm@10002000 {
> -		compatible = "ingenic,jz4740-pwm";
> -		reg = <0x10002000 0x1000>;
> -
> -		#pwm-cells = <3>;
> -
> -		clocks = <&ext>;
> -		clock-names = "ext";
> -	};
> +This documentation has moved; for a description of the devicetree bindings of
> +this driver, please refer to ../timer/ingenic,tcu.txt.

This should be evident from the git log. Just remove it.

> diff --git a/Documentation/devicetree/bindings/timer/ingenic,tcu.txt b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
> new file mode 100644
> index 000000000000..65d125b460aa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
> @@ -0,0 +1,147 @@
> +Ingenic JZ47xx SoCs Timer/Counter Unit devicetree bindings
> +==========================================================
> +
> +For a description of the TCU hardware and drivers, have a look at
> +Documentation/mips/ingenic-tcu.txt.
> +
> +Required properties:
> +
> +- compatible: Must be one of:
> +  * ingenic,jz4740-tcu
> +  * ingenic,jz4725b-tcu
> +  * ingenic,jz4770-tcu
> +- reg: Should be the offset/length value corresponding to the TCU registers

> +- #address-cells: Should be <1>;
> +- #size-cells: Should be <1>;
> +- ranges: Should be one range for the full TCU registers area

These can all be implied.

> +- clocks: List of phandle & clock specifiers for clocks external to the TCU.
> +  The "pclk", "rtc", "ext" and "tcu" clocks should be provided.
> +- clock-names: List of name strings for the external clocks.
> +- #clock-cells: Should be <1>;
> +  Clock consumers specify this argument to identify a clock. The valid values
> +  may be found in <dt-bindings/clock/ingenic,tcu.h>.
> +- interrupt-controller : Identifies the node as an interrupt controller
> +- #interrupt-cells : Specifies the number of cells needed to encode an
> +  interrupt source. The value should be 1.
> +- interrupt-parent : phandle of the interrupt controller.
> +- interrupts : Specifies the interrupt the controller is connected to.

How many and what are they. The example shows there are 3.
> +
> +Optional properties:
> +
> +- ingenic,timer-channel: Specifies the TCU channel that should be used as
> +  system timer. If not provided, the TCU channel 0 is used for the system timer.
> +
> +- ingenic,clocksource-channel: Specifies the TCU channel that should be used
> +  as clocksource and sched_clock. It must be a different channel than the one
> +  used as system timer. If not provided, neither a clocksource nor a
> +  sched_clock is instantiated.

clocksource and sched_clock are Linux specific and don't belong in DT. 
You should define properties of the hardware or use existing properties 
like interrupts or clocks to figure out which channel to use. For 
example, if some channels don't have an interrupt, then use them for 
clocksource and not a clockevent. Or you could have timers that run in 
low-power modes or not. If all the channels are identical, then it 
shouldn't matter which ones the OS picks.

> +
> +
> +Children nodes
> +==========================================================
> +
> +
> +PWM node:
> +---------
> +
> +Required properties:
> +
> +- compatible: Must be one of:
> +  * ingenic,jz4740-pwm
> +  * ingenic,jz4725b-pwm
> +- #pwm-cells: Should be 3. See ../pwm/pwm.txt for a description of the cell
> +  format.
> +- clocks: List of phandle & clock specifiers for the TCU clocks.
> +- clock-names: List of name strings for the TCU clocks.
> +
> +
> +Watchdog node:
> +--------------
> +
> +Required properties:
> +
> +- compatible: Must be one of:
> +  * ingenic,jz4740-watchdog
> +  * ingenic,jz4780-watchdog
> +- clocks: phandle to the RTC clock
> +- clock-names: should be "rtc"
> +
> +
> +OST node:
> +---------
> +
> +Required properties:
> +
> +- compatible: Must be one of:
> +  * ingenic,jz4725b-ost
> +  * ingenic,jz4770-ost
> +- clocks: phandle to the OST clock
> +- clock-names: should be "ost"
> +- interrupts : Specifies the interrupt the OST is connected to.
> +
> +
> +Example
> +==========================================================
> +
> +#include <dt-bindings/clock/jz4770-cgu.h>
> +#include <dt-bindings/clock/ingenic,tcu.h>
> +
> +/ {
> +	tcu: timer@10002000 {
> +		compatible = "ingenic,jz4770-tcu";
> +		reg = <0x10002000 0x1000>;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges = <0x0 0x10002000 0x1000>;
> +
> +		#clock-cells = <1>;
> +
> +		clocks = <&cgu JZ4770_CLK_RTC
> +			  &cgu JZ4770_CLK_EXT
> +			  &cgu JZ4770_CLK_PCLK
> +			  &cgu JZ4770_CLK_EXT>;
> +		clock-names = "rtc", "ext", "pclk", "tcu";
> +
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +
> +		interrupt-parent = <&intc>;
> +		interrupts = <27 26 25>;
> +
> +		watchdog: watchdog@0 {
> +			compatible = "ingenic,jz4740-watchdog";
> +			reg = <0x0 0x10>;
> +
> +			clocks = <&tcu TCU_CLK_WDT>;
> +			clock-names = "wdt";
> +		};
> +
> +		pwm: pwm@10 {
> +			compatible = "ingenic,jz4740-pwm";
> +			reg = <0x10 0xff0>;
> +
> +			#pwm-cells = <3>;
> +
> +			clocks = <&tcu TCU_CLK_TIMER0
> +				  &tcu TCU_CLK_TIMER1
> +				  &tcu TCU_CLK_TIMER2
> +				  &tcu TCU_CLK_TIMER3
> +				  &tcu TCU_CLK_TIMER4
> +				  &tcu TCU_CLK_TIMER5
> +				  &tcu TCU_CLK_TIMER6
> +				  &tcu TCU_CLK_TIMER7>;
> +			clock-names = "timer0", "timer1", "timer2", "timer3",
> +				      "timer4", "timer5", "timer6", "timer7";
> +		};
> +
> +		ost: timer@e0 {
> +			compatible = "ingenic,jz4770-ost";
> +			reg = <0xe0 0x20>;

This is creating an overlapping region with PWM which should be avoided. 
Are timers and PWM the same h/w? Then there should be one node (or maybe 
you can do 1 node per channel if each channel is independent (has its 
own register range, clocks, interrupts, etc.

Or the PWM node needs to exclude this region (by having 2 reg regions).

> +
> +			clocks = <&tcu TCU_CLK_OST>;
> +			clock-names = "ost";
> +
> +			interrupts = <15>;
> +		};
> +	};
> +};

Re: [PATCH v5 04/21] dt-bindings: Add doc for the Ingenic TCU drivers

[Paul Cercueil]

Hi Rob,

Le mer. 25 juil. 2018 à 17:21, Rob Herring <robh@kernel.org> a écrit :
> On Wed, Jul 25, 2018 at 01:19:41AM +0200, Paul Cercueil wrote:
>>  Add documentation about how to properly use the Ingenic TCU
>>  (Timer/Counter Unit) drivers from devicetree.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>>   .../devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt |  24 +---
>>   .../devicetree/bindings/timer/ingenic,tcu.txt      | 147 
>> +++++++++++++++++++++
>>   .../bindings/watchdog/ingenic,jz4740-wdt.txt       |  17 +--
>>   3 files changed, 151 insertions(+), 37 deletions(-)
>>   create mode 100644 
>> Documentation/devicetree/bindings/timer/ingenic,tcu.txt
>> 
>>   v4: New patch in this series. Corresponds to V2 patches 3-4-5 with
>>       added content.
>> 
>>   v5: - Edited PWM/watchdog DT bindings documentation to point to 
>> the new
>>         document.
>>       - Moved main document to
>>         Documentation/devicetree/bindings/timer/ingenic,tcu.txt
>>       - Updated documentation to reflect the new devicetree bindings.
>> 
>>  diff --git 
>> a/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt 
>> b/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
>>  index 7d9d3f90641b..a722cdde3aa7 100644
>>  --- a/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
>>  +++ b/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
>>  @@ -1,25 +1,5 @@
>>   Ingenic JZ47xx PWM Controller
>>   =============================
>> 
>>  -Required properties:
>>  -- compatible: One of:
>>  -  * "ingenic,jz4740-pwm"
>>  -  * "ingenic,jz4770-pwm"
>>  -  * "ingenic,jz4780-pwm"
>>  -- #pwm-cells: Should be 3. See pwm.txt in this directory for a 
>> description
>>  -  of the cells format.
>>  -- clocks : phandle to the external clock.
>>  -- clock-names : Should be "ext".
>>  -
>>  -
>>  -Example:
>>  -
>>  -	pwm: pwm@10002000 {
>>  -		compatible = "ingenic,jz4740-pwm";
>>  -		reg = <0x10002000 0x1000>;
>>  -
>>  -		#pwm-cells = <3>;
>>  -
>>  -		clocks = <&ext>;
>>  -		clock-names = "ext";
>>  -	};
>>  +This documentation has moved; for a description of the devicetree 
>> bindings of
>>  +this driver, please refer to ../timer/ingenic,tcu.txt.
> 
> This should be evident from the git log. Just remove it.

Ok.

>>  diff --git 
>> a/Documentation/devicetree/bindings/timer/ingenic,tcu.txt 
>> b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
>>  new file mode 100644
>>  index 000000000000..65d125b460aa
>>  --- /dev/null
>>  +++ b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
>>  @@ -0,0 +1,147 @@
>>  +Ingenic JZ47xx SoCs Timer/Counter Unit devicetree bindings
>>  +==========================================================
>>  +
>>  +For a description of the TCU hardware and drivers, have a look at
>>  +Documentation/mips/ingenic-tcu.txt.
>>  +
>>  +Required properties:
>>  +
>>  +- compatible: Must be one of:
>>  +  * ingenic,jz4740-tcu
>>  +  * ingenic,jz4725b-tcu
>>  +  * ingenic,jz4770-tcu
>>  +- reg: Should be the offset/length value corresponding to the TCU 
>> registers
> 
>>  +- #address-cells: Should be <1>;
>>  +- #size-cells: Should be <1>;
>>  +- ranges: Should be one range for the full TCU registers area
> 
> These can all be implied.

Ok.

>>  +- clocks: List of phandle & clock specifiers for clocks external 
>> to the TCU.
>>  +  The "pclk", "rtc", "ext" and "tcu" clocks should be provided.
>>  +- clock-names: List of name strings for the external clocks.
>>  +- #clock-cells: Should be <1>;
>>  +  Clock consumers specify this argument to identify a clock. The 
>> valid values
>>  +  may be found in <dt-bindings/clock/ingenic,tcu.h>.
>>  +- interrupt-controller : Identifies the node as an interrupt 
>> controller
>>  +- #interrupt-cells : Specifies the number of cells needed to 
>> encode an
>>  +  interrupt source. The value should be 1.
>>  +- interrupt-parent : phandle of the interrupt controller.
>>  +- interrupts : Specifies the interrupt the controller is connected 
>> to.
> 
> How many and what are they. The example shows there are 3.

They are 2 or 3, and they are all the same (three parent IRQs for one 
irqchip).
It's explained in the hardware doc, should I add it here as well?

>>  +
>>  +Optional properties:
>>  +
>>  +- ingenic,timer-channel: Specifies the TCU channel that should be 
>> used as
>>  +  system timer. If not provided, the TCU channel 0 is used for the 
>> system timer.
>>  +
>>  +- ingenic,clocksource-channel: Specifies the TCU channel that 
>> should be used
>>  +  as clocksource and sched_clock. It must be a different channel 
>> than the one
>>  +  used as system timer. If not provided, neither a clocksource nor 
>> a
>>  +  sched_clock is instantiated.
> 
> clocksource and sched_clock are Linux specific and don't belong in DT.
> You should define properties of the hardware or use existing 
> properties
> like interrupts or clocks to figure out which channel to use. For
> example, if some channels don't have an interrupt, then use them for
> clocksource and not a clockevent. Or you could have timers that run in
> low-power modes or not. If all the channels are identical, then it
> shouldn't matter which ones the OS picks.

We already talked about that. All the TCU channels can be used for PWM.
The problem is I cannot know from the driver's scope which channels will
be free and which channels will be requested for PWM. You suggested 
that I
parse the devicetree for clients, and I did that in the V3/V4 patchset. 
But
it only works for clients requesting through devicetree, not from 
platform
code or even sysfs.

One thing I can try is to dynamically change the channels the system 
timer
and clocksource are using when the current ones are requested for PWM. 
But
that sounds hardcore...

>>  +
>>  +
>>  +Children nodes
>>  +==========================================================
>>  +
>>  +
>>  +PWM node:
>>  +---------
>>  +
>>  +Required properties:
>>  +
>>  +- compatible: Must be one of:
>>  +  * ingenic,jz4740-pwm
>>  +  * ingenic,jz4725b-pwm
>>  +- #pwm-cells: Should be 3. See ../pwm/pwm.txt for a description of 
>> the cell
>>  +  format.
>>  +- clocks: List of phandle & clock specifiers for the TCU clocks.
>>  +- clock-names: List of name strings for the TCU clocks.
>>  +
>>  +
>>  +Watchdog node:
>>  +--------------
>>  +
>>  +Required properties:
>>  +
>>  +- compatible: Must be one of:
>>  +  * ingenic,jz4740-watchdog
>>  +  * ingenic,jz4780-watchdog
>>  +- clocks: phandle to the RTC clock
>>  +- clock-names: should be "rtc"
>>  +
>>  +
>>  +OST node:
>>  +---------
>>  +
>>  +Required properties:
>>  +
>>  +- compatible: Must be one of:
>>  +  * ingenic,jz4725b-ost
>>  +  * ingenic,jz4770-ost
>>  +- clocks: phandle to the OST clock
>>  +- clock-names: should be "ost"
>>  +- interrupts : Specifies the interrupt the OST is connected to.
>>  +
>>  +
>>  +Example
>>  +==========================================================
>>  +
>>  +#include <dt-bindings/clock/jz4770-cgu.h>
>>  +#include <dt-bindings/clock/ingenic,tcu.h>
>>  +
>>  +/ {
>>  +	tcu: timer@10002000 {
>>  +		compatible = "ingenic,jz4770-tcu";
>>  +		reg = <0x10002000 0x1000>;
>>  +		#address-cells = <1>;
>>  +		#size-cells = <1>;
>>  +		ranges = <0x0 0x10002000 0x1000>;
>>  +
>>  +		#clock-cells = <1>;
>>  +
>>  +		clocks = <&cgu JZ4770_CLK_RTC
>>  +			  &cgu JZ4770_CLK_EXT
>>  +			  &cgu JZ4770_CLK_PCLK
>>  +			  &cgu JZ4770_CLK_EXT>;
>>  +		clock-names = "rtc", "ext", "pclk", "tcu";
>>  +
>>  +		interrupt-controller;
>>  +		#interrupt-cells = <1>;
>>  +
>>  +		interrupt-parent = <&intc>;
>>  +		interrupts = <27 26 25>;
>>  +
>>  +		watchdog: watchdog@0 {
>>  +			compatible = "ingenic,jz4740-watchdog";
>>  +			reg = <0x0 0x10>;
>>  +
>>  +			clocks = <&tcu TCU_CLK_WDT>;
>>  +			clock-names = "wdt";
>>  +		};
>>  +
>>  +		pwm: pwm@10 {
>>  +			compatible = "ingenic,jz4740-pwm";
>>  +			reg = <0x10 0xff0>;
>>  +
>>  +			#pwm-cells = <3>;
>>  +
>>  +			clocks = <&tcu TCU_CLK_TIMER0
>>  +				  &tcu TCU_CLK_TIMER1
>>  +				  &tcu TCU_CLK_TIMER2
>>  +				  &tcu TCU_CLK_TIMER3
>>  +				  &tcu TCU_CLK_TIMER4
>>  +				  &tcu TCU_CLK_TIMER5
>>  +				  &tcu TCU_CLK_TIMER6
>>  +				  &tcu TCU_CLK_TIMER7>;
>>  +			clock-names = "timer0", "timer1", "timer2", "timer3",
>>  +				      "timer4", "timer5", "timer6", "timer7";
>>  +		};
>>  +
>>  +		ost: timer@e0 {
>>  +			compatible = "ingenic,jz4770-ost";
>>  +			reg = <0xe0 0x20>;
> 
> This is creating an overlapping region with PWM which should be 
> avoided.
> Are timers and PWM the same h/w? Then there should be one node (or 
> maybe
> you can do 1 node per channel if each channel is independent (has its
> own register range, clocks, interrupts, etc.
> 
> Or the PWM node needs to exclude this region (by having 2 reg 
> regions).

I will use two regions then.

>> 
>  +};
>> 
>  +	};
>>  +
>>  +			clocks = <&tcu TCU_CLK_OST>;
>>  +			clock-names = "ost";
>>  +
>>  +			interrupts = <15>;
>>  +		};

Thanks,
-Paul Cercueil

Re: [PATCH v5 04/21] dt-bindings: Add doc for the Ingenic TCU drivers

[Daniel Lezcano]

On 31/07/2018 00:01, Paul Cercueil wrote:

[ ... ]

>>>  +- ingenic,timer-channel: Specifies the TCU channel that should be
>>> used as
>>>  +  system timer. If not provided, the TCU channel 0 is used for the
>>> system timer.
>>>  +
>>>  +- ingenic,clocksource-channel: Specifies the TCU channel that
>>> should be used
>>>  +  as clocksource and sched_clock. It must be a different channel
>>> than the one
>>>  +  used as system timer. If not provided, neither a clocksource nor a
>>>  +  sched_clock is instantiated.
>>
>> clocksource and sched_clock are Linux specific and don't belong in DT.
>> You should define properties of the hardware or use existing properties
>> like interrupts or clocks to figure out which channel to use. For
>> example, if some channels don't have an interrupt, then use them for
>> clocksource and not a clockevent. Or you could have timers that run in
>> low-power modes or not. If all the channels are identical, then it
>> shouldn't matter which ones the OS picks.

It can't work in this case because the pmw and the timer driver are not
communicating and the first one can stole a channel to the last one.


> We already talked about that. All the TCU channels can be used for PWM.
> The problem is I cannot know from the driver's scope which channels will
> be free and which channels will be requested for PWM. You suggested that I
> parse the devicetree for clients, and I did that in the V3/V4 patchset. But
> it only works for clients requesting through devicetree, not from platform
> code or even sysfs.
> 
> One thing I can try is to dynamically change the channels the system timer
> and clocksource are using when the current ones are requested for PWM. But
> that sounds hardcore...

Yes, it is :/

Sorry for letting you wasting time and effort to write an overkill code
not suitable for upstream.

A very gross thought, wouldn't be possible to "register" a channel from
the timer driver code in a shared data area (but well self-encapsulated)
and the pwm code will check such channel isn't in use ?

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