Re: [PATCH v8 03/26] dt-bindings: Add doc for the Ingenic TCU drivers

[Paul Cercueil <paul@xxxxxxxxxxxxxxx>]

Hi Rob,

Le lun. 17 déc. 2018 à 22:05, Rob Herring <robh@xxxxxxxxxx> a écrit :
> On Wed, Dec 12, 2018 at 11:08:58PM +0100, 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@xxxxxxxxxxxxxxx>
> >  ---
> >
> >  Notes:
> >       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.
> >
> >       v6: - Removed PWM/watchdog documentation files as asked by 
> > upstream
> >           - Removed doc about properties that should be implicit
> >           - Removed doc about ingenic,timer-channel /
> >             ingenic,clocksource-channel as they are gone
> >           - Fix WDT clock name in the binding doc
> >           - Fix lengths of register areas in watchdog/pwm nodes
> >
> >       v7: No change
> >
> >       v8: - Fix address of the PWM node
> >           - Added doc about system timer and clocksource children 
> > nodes
>
> I thought we'd sorted this out...

Yeah, well I just can't please everybody. V6/V7 didn't have the
system timer or clocksource in devicetree, which was good for
you, but then the driver nearly doubled in size and complexity,
and Thierry rightfully refused it. Now I'm at the point where
I'm trying alternative ways of encoding the information in
devicetree, as suggested by various people, just so that you
accept it. Because I don't see any other option.

> >   .../devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt |  25 ---
> >   .../devicetree/bindings/timer/ingenic,tcu.txt      | 176 
> > +++++++++++++++++++++
> >   .../bindings/watchdog/ingenic,jz4740-wdt.txt       |  17 --
> >   3 files changed, 176 insertions(+), 42 deletions(-)
> >   delete mode 100644 
> > Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
> >   create mode 100644 
> > Documentation/devicetree/bindings/timer/ingenic,tcu.txt
> >   delete mode 100644 
> > Documentation/devicetree/bindings/watchdog/ingenic,jz4740-wdt.txt
> >
> >  diff --git 
> > a/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt 
> > b/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
> >  deleted file mode 100644
> >  index 7d9d3f90641b..000000000000
> >  --- a/Documentation/devicetree/bindings/pwm/ingenic,jz47xx-pwm.txt
> >  +++ /dev/null
> >  @@ -1,25 +0,0 @@
> >  -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";
> >  -	};
> >  diff --git 
> > a/Documentation/devicetree/bindings/timer/ingenic,tcu.txt 
> > b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
> >  new file mode 100644
> >  index 000000000000..8a4ce7edf50f
> >  --- /dev/null
> >  +++ b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
> >  @@ -0,0 +1,176 @@
> >  +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
> >  +- 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.
> >  +
> >  +
> >  +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 WDT clock
> >  +- clock-names: should be "wdt"
> >  +
> >  +
> >  +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.
> >  +
> >  +
> >  +System timer node:
> >  +------------------
> >  +
> >  +Required properties:
> >  +
> >  +- compatible: Must be "ingenic,jz4740-tcu-timer"
>
> Is this an actual sub-block? Or just a way to assign a timer to a
> clockevent?

Just a way to assign a timer to a clockevent.

> >  +- clocks: phandle to the clock of the TCU channel used
> >  +- clock-names: Should be "timer"
> >  +- interrupts : Specifies the interrupt line of the TCU channel 
> > used.
> >  +
> >  +
> >  +System clocksource node:
> >  +------------------------
> >  +
> >  +Required properties:
> >  +
> >  +- compatible: Must be "ingenic,jz4740-tcu-clocksource"
>
> The h/w has a block called 'clocksource'?
>
> If there's a difference in the timer channels, then that difference
> should be described in DT, not just 'use timer X for clocksource'.

Same as above, this is just a way to assign a channel to the 
clocksource.

> >
>  +- clock-names: Should be "timer"
>  +- clocks: phandle to the clock of the TCU channel use

Regards,
- Paul Cercueil