[PATCH v4 0/8] Ingenic JZ47xx Timer/Counter Unit drivers

[PATCH v4 0/8] Ingenic JZ47xx Timer/Counter Unit drivers

[Paul Cercueil]

Hi,

This is the 4th version of my TCU patchset.

The major change is a greatly improved documentation, both in-code
and as separate text files, to describe how the hardware works and
how the devicetree bindings should be used.

There are also cosmetic changes in the irqchip driver, and the
clocksource driver will now use as timers all TCU channels not
requested by the TCU PWM driver.

Cheers,
-Paul

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

[PATCH v4 1/8] mfd: syscon: Add ingenic-tcu.h header

[Paul Cercueil]

This header contains macros for the registers that are present in the
regmap shared by all the drivers related to the TCU (Timer Counter Unit)
of the Ingenic JZ47xx SoCs.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Acked-by: Lee Jones <lee.jones@linaro.org>
---
 include/linux/mfd/syscon/ingenic-tcu.h | 54 ++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 include/linux/mfd/syscon/ingenic-tcu.h

 v2: Use SPDX identifier for the license
 v3: - Use macros instead of enum
     - Add 'TCU_' at the beginning of each macro
	 - Remove useless include <linux/regmap.h>
 v4: No change

diff --git a/include/linux/mfd/syscon/ingenic-tcu.h b/include/linux/mfd/syscon/ingenic-tcu.h
new file mode 100644
index 000000000000..96dd59f7c3b2
--- /dev/null
+++ b/include/linux/mfd/syscon/ingenic-tcu.h
@@ -0,0 +1,54 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Header file for the Ingenic JZ47xx TCU driver
+ */
+#ifndef __LINUX_CLK_INGENIC_TCU_H_
+#define __LINUX_CLK_INGENIC_TCU_H_
+
+#include <linux/bitops.h>
+
+#define TCU_REG_WDT_TDR		0x00
+#define TCU_REG_WDT_TCER	0x04
+#define TCU_REG_WDT_TCNT	0x08
+#define TCU_REG_WDT_TCSR	0x0c
+#define TCU_REG_TER		0x10
+#define TCU_REG_TESR		0x14
+#define TCU_REG_TECR		0x18
+#define TCU_REG_TSR		0x1c
+#define TCU_REG_TFR		0x20
+#define TCU_REG_TFSR		0x24
+#define TCU_REG_TFCR		0x28
+#define TCU_REG_TSSR		0x2c
+#define TCU_REG_TMR		0x30
+#define TCU_REG_TMSR		0x34
+#define TCU_REG_TMCR		0x38
+#define TCU_REG_TSCR		0x3c
+#define TCU_REG_TDFR0		0x40
+#define TCU_REG_TDHR0		0x44
+#define TCU_REG_TCNT0		0x48
+#define TCU_REG_TCSR0		0x4c
+#define TCU_REG_OST_DR		0xe0
+#define TCU_REG_OST_CNTL	0xe4
+#define TCU_REG_OST_CNTH	0xe8
+#define TCU_REG_OST_TCSR	0xec
+#define TCU_REG_TSTR		0xf0
+#define TCU_REG_TSTSR		0xf4
+#define TCU_REG_TSTCR		0xf8
+#define TCU_REG_OST_CNTHBUF	0xfc
+
+#define TCU_TCSR_RESERVED_BITS		0x3f
+#define TCU_TCSR_PARENT_CLOCK_MASK	0x07
+#define TCU_TCSR_PRESCALE_LSB		3
+#define TCU_TCSR_PRESCALE_MASK		0x38
+
+#define TCU_TCSR_PWM_SD		BIT(9)	/* 0: Shutdown abruptly 1: gracefully */
+#define TCU_TCSR_PWM_INITL_HIGH	BIT(8)	/* Sets the initial output level */
+#define TCU_TCSR_PWM_EN		BIT(7)	/* PWM pin output enable */
+
+#define TCU_CHANNEL_STRIDE	0x10
+#define TCU_REG_TDFRc(c)	(TCU_REG_TDFR0 + ((c) * TCU_CHANNEL_STRIDE))
+#define TCU_REG_TDHRc(c)	(TCU_REG_TDHR0 + ((c) * TCU_CHANNEL_STRIDE))
+#define TCU_REG_TCNTc(c)	(TCU_REG_TCNT0 + ((c) * TCU_CHANNEL_STRIDE))
+#define TCU_REG_TCSRc(c)	(TCU_REG_TCSR0 + ((c) * TCU_CHANNEL_STRIDE))
+
+#endif /* __LINUX_CLK_INGENIC_TCU_H_ */
-- 
2.11.0

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

[PATCH v4 2/8] dt-bindings: ingenic: Add DT bindings for TCU clocks

[Paul Cercueil]

This header provides clock numbers for the ingenic,tcu
DT binding.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 include/dt-bindings/clock/ingenic,tcu.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 include/dt-bindings/clock/ingenic,tcu.h

 v2: Use SPDX identifier for the license
 v3: No change
 v4: No change

diff --git a/include/dt-bindings/clock/ingenic,tcu.h b/include/dt-bindings/clock/ingenic,tcu.h
new file mode 100644
index 000000000000..179815d7b3bb
--- /dev/null
+++ b/include/dt-bindings/clock/ingenic,tcu.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * This header provides clock numbers for the ingenic,tcu DT binding.
+ */
+
+#ifndef __DT_BINDINGS_CLOCK_INGENIC_TCU_H__
+#define __DT_BINDINGS_CLOCK_INGENIC_TCU_H__
+
+#define JZ4740_CLK_TIMER0	0
+#define JZ4740_CLK_TIMER1	1
+#define JZ4740_CLK_TIMER2	2
+#define JZ4740_CLK_TIMER3	3
+#define JZ4740_CLK_TIMER4	4
+#define JZ4740_CLK_TIMER5	5
+#define JZ4740_CLK_TIMER6	6
+#define JZ4740_CLK_TIMER7	7
+#define JZ4740_CLK_WDT		8
+#define JZ4740_CLK_LAST		JZ4740_CLK_WDT
+
+#define JZ4770_CLK_OST		9
+#define JZ4770_CLK_LAST		JZ4770_CLK_OST
+
+#endif /* __DT_BINDINGS_CLOCK_INGENIC_TCU_H__ */
-- 
2.11.0

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

Re: [PATCH v4 2/8] dt-bindings: ingenic: Add DT bindings for TCU clocks

[Stephen Boyd]

Quoting Paul Cercueil (2018-03-17 16:28:55)
> This header provides clock numbers for the ingenic,tcu
> DT binding.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---

Acked-by: Stephen Boyd <sboyd@kernel.org>

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

Re: [PATCH v4 2/8] dt-bindings: ingenic: Add DT bindings for TCU clocks

[Mathieu Malaterre]

Hi Paul,

Two things:

On Sun, Mar 18, 2018 at 12:28 AM, Paul Cercueil <paul@crapouillou.net> wrote:
> This header provides clock numbers for the ingenic,tcu
> DT binding.

I have tested the whole series on my Creator CI20 with success, using:

+ tcu@10002000 {
+ compatible = "ingenic,jz4780-tcu";
+ reg = <0x10002000 0x140>;
+
+ interrupt-parent = <&intc>;
+ interrupts = <27 26 25>;
+ };


So:

Tested-by: Mathieu Malaterre <malat@debian.org>

> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
>  include/dt-bindings/clock/ingenic,tcu.h | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>  create mode 100644 include/dt-bindings/clock/ingenic,tcu.h
>
>  v2: Use SPDX identifier for the license
>  v3: No change
>  v4: No change
>
> diff --git a/include/dt-bindings/clock/ingenic,tcu.h b/include/dt-bindings/clock/ingenic,tcu.h
> new file mode 100644
> index 000000000000..179815d7b3bb
> --- /dev/null
> +++ b/include/dt-bindings/clock/ingenic,tcu.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * This header provides clock numbers for the ingenic,tcu DT binding.
> + */
> +
> +#ifndef __DT_BINDINGS_CLOCK_INGENIC_TCU_H__
> +#define __DT_BINDINGS_CLOCK_INGENIC_TCU_H__
> +
> +#define JZ4740_CLK_TIMER0      0
> +#define JZ4740_CLK_TIMER1      1
> +#define JZ4740_CLK_TIMER2      2
> +#define JZ4740_CLK_TIMER3      3
> +#define JZ4740_CLK_TIMER4      4
> +#define JZ4740_CLK_TIMER5      5
> +#define JZ4740_CLK_TIMER6      6
> +#define JZ4740_CLK_TIMER7      7
> +#define JZ4740_CLK_WDT         8
> +#define JZ4740_CLK_LAST                JZ4740_CLK_WDT
> +
> +#define JZ4770_CLK_OST         9
> +#define JZ4770_CLK_LAST                JZ4770_CLK_OST
> +

When working on JZ4780 support, I always struggle to read those kind
of #define. Since this is a new patch would you mind changing
s/JZ4740/JZ47XX/ in your header ?

Thanks for your work on JZ !

> +#endif /* __DT_BINDINGS_CLOCK_INGENIC_TCU_H__ */
> --
> 2.11.0
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/8] dt-bindings: ingenic: Add DT bindings for TCU clocks,

[Paul Cercueil]

Le 2018-03-20 08:15, Mathieu Malaterre a écrit :
> Hi Paul,
> 
> Two things:
> 
> On Sun, Mar 18, 2018 at 12:28 AM, Paul Cercueil <paul@crapouillou.net> 
> wrote:
>> This header provides clock numbers for the ingenic,tcu
>> DT binding.
> 
> I have tested the whole series on my Creator CI20 with success, using:
> 
> + tcu@10002000 {
> + compatible = "ingenic,jz4780-tcu";
> + reg = <0x10002000 0x140>;
> +
> + interrupt-parent = <&intc>;
> + interrupts = <27 26 25>;
> + };
> 
> 
> So:
> 
> Tested-by: Mathieu Malaterre <malat@debian.org>

Great! Is that for the whole patchset or just this one patch?

>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>> Reviewed-by: Rob Herring <robh@kernel.org>
>> ---
>>  include/dt-bindings/clock/ingenic,tcu.h | 23 +++++++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>>  create mode 100644 include/dt-bindings/clock/ingenic,tcu.h
>> 
>>  v2: Use SPDX identifier for the license
>>  v3: No change
>>  v4: No change
>> 
>> diff --git a/include/dt-bindings/clock/ingenic,tcu.h 
>> b/include/dt-bindings/clock/ingenic,tcu.h
>> new file mode 100644
>> index 000000000000..179815d7b3bb
>> --- /dev/null
>> +++ b/include/dt-bindings/clock/ingenic,tcu.h
>> @@ -0,0 +1,23 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * This header provides clock numbers for the ingenic,tcu DT binding.
>> + */
>> +
>> +#ifndef __DT_BINDINGS_CLOCK_INGENIC_TCU_H__
>> +#define __DT_BINDINGS_CLOCK_INGENIC_TCU_H__
>> +
>> +#define JZ4740_CLK_TIMER0      0
>> +#define JZ4740_CLK_TIMER1      1
>> +#define JZ4740_CLK_TIMER2      2
>> +#define JZ4740_CLK_TIMER3      3
>> +#define JZ4740_CLK_TIMER4      4
>> +#define JZ4740_CLK_TIMER5      5
>> +#define JZ4740_CLK_TIMER6      6
>> +#define JZ4740_CLK_TIMER7      7
>> +#define JZ4740_CLK_WDT         8
>> +#define JZ4740_CLK_LAST                JZ4740_CLK_WDT
>> +
>> +#define JZ4770_CLK_OST         9
>> +#define JZ4770_CLK_LAST                JZ4770_CLK_OST
>> +
> 
> When working on JZ4780 support, I always struggle to read those kind
> of #define. Since this is a new patch would you mind changing
> s/JZ4740/JZ47XX/ in your header ?

Well the idea was that these defines are for JZ4740 and newer.
But that means all Ingenic SoCs, so I guess I can change it.

> Thanks for your work on JZ !

Sure, thank you for testing!

>> +#endif /* __DT_BINDINGS_CLOCK_INGENIC_TCU_H__ */
>> --
>> 2.11.0
>> 
>> 

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

Re: [PATCH v4 2/8] dt-bindings: ingenic: Add DT bindings for TCU clocks

[Mathieu Malaterre]

On Wed, Mar 28, 2018 at 5:04 PM, Paul Cercueil <paul@crapouillou.net> wrote:
> Le 2018-03-20 08:15, Mathieu Malaterre a écrit :
>>
>> Hi Paul,
>>
>> Two things:
>>
>> On Sun, Mar 18, 2018 at 12:28 AM, Paul Cercueil <paul@crapouillou.net>
>> wrote:
>>>
>>> This header provides clock numbers for the ingenic,tcu
>>> DT binding.
>>
>>
>> I have tested the whole series on my Creator CI20 with success, using:
>>
>> + tcu@10002000 {
>> + compatible = "ingenic,jz4780-tcu";
>> + reg = <0x10002000 0x140>;
>> +
>> + interrupt-parent = <&intc>;
>> + interrupts = <27 26 25>;
>> + };
>>
>>
>> So:
>>
>> Tested-by: Mathieu Malaterre <malat@debian.org>
>
>
> Great! Is that for the whole patchset or just this one patch?

The sentence just above was "whole series"  :) so yes that was for the
whole series. Technically I only tested it on JZ4780, I hope this is
acceptable for the tag.



>
>>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>> Reviewed-by: Rob Herring <robh@kernel.org>
>>> ---
>>>  include/dt-bindings/clock/ingenic,tcu.h | 23 +++++++++++++++++++++++
>>>  1 file changed, 23 insertions(+)
>>>  create mode 100644 include/dt-bindings/clock/ingenic,tcu.h
>>>
>>>  v2: Use SPDX identifier for the license
>>>  v3: No change
>>>  v4: No change
>>>
>>> diff --git a/include/dt-bindings/clock/ingenic,tcu.h
>>> b/include/dt-bindings/clock/ingenic,tcu.h
>>> new file mode 100644
>>> index 000000000000..179815d7b3bb
>>> --- /dev/null
>>> +++ b/include/dt-bindings/clock/ingenic,tcu.h
>>> @@ -0,0 +1,23 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + * This header provides clock numbers for the ingenic,tcu DT binding.
>>> + */
>>> +
>>> +#ifndef __DT_BINDINGS_CLOCK_INGENIC_TCU_H__
>>> +#define __DT_BINDINGS_CLOCK_INGENIC_TCU_H__
>>> +
>>> +#define JZ4740_CLK_TIMER0      0
>>> +#define JZ4740_CLK_TIMER1      1
>>> +#define JZ4740_CLK_TIMER2      2
>>> +#define JZ4740_CLK_TIMER3      3
>>> +#define JZ4740_CLK_TIMER4      4
>>> +#define JZ4740_CLK_TIMER5      5
>>> +#define JZ4740_CLK_TIMER6      6
>>> +#define JZ4740_CLK_TIMER7      7
>>> +#define JZ4740_CLK_WDT         8
>>> +#define JZ4740_CLK_LAST                JZ4740_CLK_WDT
>>> +
>>> +#define JZ4770_CLK_OST         9
>>> +#define JZ4770_CLK_LAST                JZ4770_CLK_OST
>>> +
>>
>>
>> When working on JZ4780 support, I always struggle to read those kind
>> of #define. Since this is a new patch would you mind changing
>> s/JZ4740/JZ47XX/ in your header ?
>
>
> Well the idea was that these defines are for JZ4740 and newer.
> But that means all Ingenic SoCs, so I guess I can change it.
>
>> Thanks for your work on JZ !
>
>
> Sure, thank you for testing!
>
>
>>> +#endif /* __DT_BINDINGS_CLOCK_INGENIC_TCU_H__ */
>>> --
>>> 2.11.0
>>>
>>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 3/8] doc: Add doc for the Ingenic TCU hardware

[Paul Cercueil]

Add a documentation file about the Timer/Counter Unit (TCU)
present in the Ingenic JZ47xx SoCs.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 Documentation/mips/00-INDEX        |  3 +++
 Documentation/mips/ingenic-tcu.txt | 50 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+)
 create mode 100644 Documentation/mips/ingenic-tcu.txt

 v4: New patch in this series

diff --git a/Documentation/mips/00-INDEX b/Documentation/mips/00-INDEX
index 8ae9cffc2262..8ab8c3f83771 100644
--- a/Documentation/mips/00-INDEX
+++ b/Documentation/mips/00-INDEX
@@ -2,3 +2,6 @@
 	- this file.
 AU1xxx_IDE.README
 	- README for MIPS AU1XXX IDE driver.
+ingenic-tcu.txt
+	- Information file about the Timer/Counter Unit present
+	  in Ingenic JZ47xx SoCs.
diff --git a/Documentation/mips/ingenic-tcu.txt b/Documentation/mips/ingenic-tcu.txt
new file mode 100644
index 000000000000..2508e5793da8
--- /dev/null
+++ b/Documentation/mips/ingenic-tcu.txt
@@ -0,0 +1,50 @@
+Ingenic JZ47xx SoCs Timer/Counter Unit hardware
+-----------------------------------------------
+
+The Timer/Counter Unit (TCU) in Ingenic JZ47xx SoCs is a multi-function
+hardware block. It features eight channels, that can be used as counters,
+timers, or PWM.
+
+- JZ4770 introduced a separate channel, called Operating System Timer (OST).
+  It is a 64-bit programmable timer.
+
+- Each one of the eight channels has its own clock, which can be reparented
+  to three different clocks (pclk, ext, rtc), gated, and reclocked, through
+  their TCSR register.
+  * The watchdog and OST hardware blocks also feature a TCSR register with
+	the same format in their register space.
+  * The TCU registers used to gate/ungate can also gate/ungate the watchdog
+	and OST clocks.
+
+- On SoCs >= JZ4770, there are two different modes:
+  * Channels 0, 3-7 operate in TCU1 mode: they cannot work in sleep mode,
+	but are easier to operate.
+  * Channels 1-2 operate in TCU2 mode: they can work in sleep mode, but
+	the operation is a bit more complicated than with TCU1 channels.
+
+- Each channel can generate an interrupt. Some channels share an interrupt
+  line, some don't, and this changes between SoC versions:
+  * on JZ4740, timer 0 and timer 1 have their own interrupt line; others share
+	one interrupt line.
+  * on JZ4770 and JZ4780, timer 5 has its own interrupt; timers 0-4 and 6-7 all
+	use one interrupt line; the OST uses the last interrupt.
+
+Implementation
+--------------
+
+The functionalities of the TCU hardware are spread across multiple drivers:
+- interrupts:   drivers/irqchip/irq-ingenic-tcu.c
+- clocks:       drivers/clk/ingenic/tcu.c
+- timer:        drivers/clocksource/timer-ingenic.c
+- PWM:          drivers/pwm/pwm-jz4740.c
+- watchdog:     drivers/watchdog/jz4740_wdt.c
+- OST:			(none yet)
+
+Because various functionalities of the TCU that belong to different drivers
+and frameworks can be controlled from the same registers, all of these drivers
+access their registers through the same regmap.
+
+In practice, the devicetree nodes for all of these drivers must be children
+of a "syscon" node that defines the register area.
+For more information regarding the devicetree bindings of the TCU drivers,
+have a look at Documentation/devicetree/bindings/mfd/ingenic,tcu.txt.
-- 
2.11.0

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

Re: [PATCH v4 3/8] doc: Add doc for the Ingenic TCU hardware

[Randy Dunlap]

On 03/17/2018 04:28 PM, Paul Cercueil wrote:
> Add a documentation file about the Timer/Counter Unit (TCU)
> present in the Ingenic JZ47xx SoCs.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  Documentation/mips/00-INDEX        |  3 +++
>  Documentation/mips/ingenic-tcu.txt | 50 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 53 insertions(+)
>  create mode 100644 Documentation/mips/ingenic-tcu.txt
> 
>  v4: New patch in this series

> diff --git a/Documentation/mips/ingenic-tcu.txt b/Documentation/mips/ingenic-tcu.txt
> new file mode 100644
> index 000000000000..2508e5793da8
> --- /dev/null
> +++ b/Documentation/mips/ingenic-tcu.txt
> @@ -0,0 +1,50 @@
> +Ingenic JZ47xx SoCs Timer/Counter Unit hardware
> +-----------------------------------------------
> +
> +The Timer/Counter Unit (TCU) in Ingenic JZ47xx SoCs is a multi-function
> +hardware block. It features eight channels, that can be used as counters,

                    drop comma ............. ^

> +timers, or PWM.
> +
> +- JZ4770 introduced a separate channel, called Operating System Timer (OST).
> +  It is a 64-bit programmable timer.
> +
> +- Each one of the eight channels has its own clock, which can be reparented
> +  to three different clocks (pclk, ext, rtc), gated, and reclocked, through
> +  their TCSR register.
> +  * The watchdog and OST hardware blocks also feature a TCSR register with
> +	the same format in their register space.
> +  * The TCU registers used to gate/ungate can also gate/ungate the watchdog
> +	and OST clocks.
> +
> +- On SoCs >= JZ4770, there are two different modes:
> +  * Channels 0, 3-7 operate in TCU1 mode: they cannot work in sleep mode,
> +	but are easier to operate.
> +  * Channels 1-2 operate in TCU2 mode: they can work in sleep mode, but
> +	the operation is a bit more complicated than with TCU1 channels.
> +
> +- Each channel can generate an interrupt. Some channels share an interrupt	
> +  line, some don't, and this changes between SoC versions:
> +  * on JZ4740, timer 0 and timer 1 have their own interrupt line; others share
> +	one interrupt line.
> +  * on JZ4770 and JZ4780, timer 5 has its own interrupt; timers 0-4 and 6-7 all
> +	use one interrupt line; the OST uses the last interrupt.

"The OST uses the last interrupt." is not clear to someone who doesn't know
about this hardware. (I can read it several ways.)

Does it mean that the 4770 and 4780 have 3 interrupt lines used like so?

- timer 5 uses one interrupt line
- timers 0-4 and 6-7 use a second interrupt line
- the OST uses a third interrupt line


thanks,
-- 
~Randy
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/8] doc: Add doc for the Ingenic TCU hardware,

[Paul Cercueil]

Le 2018-03-18 00:52, Randy Dunlap a écrit :
> On 03/17/2018 04:28 PM, Paul Cercueil wrote:
>> Add a documentation file about the Timer/Counter Unit (TCU)
>> present in the Ingenic JZ47xx SoCs.
>> 
>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>> ---
>>  Documentation/mips/00-INDEX        |  3 +++
>>  Documentation/mips/ingenic-tcu.txt | 50 
>> ++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 53 insertions(+)
>>  create mode 100644 Documentation/mips/ingenic-tcu.txt
>> 
>>  v4: New patch in this series
> 
>> diff --git a/Documentation/mips/ingenic-tcu.txt 
>> b/Documentation/mips/ingenic-tcu.txt
>> new file mode 100644
>> index 000000000000..2508e5793da8
>> --- /dev/null
>> +++ b/Documentation/mips/ingenic-tcu.txt
>> @@ -0,0 +1,50 @@
>> +Ingenic JZ47xx SoCs Timer/Counter Unit hardware
>> +-----------------------------------------------
>> +
>> +The Timer/Counter Unit (TCU) in Ingenic JZ47xx SoCs is a 
>> multi-function
>> +hardware block. It features eight channels, that can be used as 
>> counters,
> 
>                     drop comma ............. ^

Ok.

>> +timers, or PWM.
>> +
>> +- JZ4770 introduced a separate channel, called Operating System Timer 
>> (OST).
>> +  It is a 64-bit programmable timer.
>> +
>> +- Each one of the eight channels has its own clock, which can be 
>> reparented
>> +  to three different clocks (pclk, ext, rtc), gated, and reclocked, 
>> through
>> +  their TCSR register.
>> +  * The watchdog and OST hardware blocks also feature a TCSR register 
>> with
>> +	the same format in their register space.
>> +  * The TCU registers used to gate/ungate can also gate/ungate the 
>> watchdog
>> +	and OST clocks.
>> +
>> +- On SoCs >= JZ4770, there are two different modes:
>> +  * Channels 0, 3-7 operate in TCU1 mode: they cannot work in sleep 
>> mode,
>> +	but are easier to operate.
>> +  * Channels 1-2 operate in TCU2 mode: they can work in sleep mode, 
>> but
>> +	the operation is a bit more complicated than with TCU1 channels.
>> +
>> +- Each channel can generate an interrupt. Some channels share an 
>> interrupt
>> +  line, some don't, and this changes between SoC versions:
>> +  * on JZ4740, timer 0 and timer 1 have their own interrupt line; 
>> others share
>> +	one interrupt line.
>> +  * on JZ4770 and JZ4780, timer 5 has its own interrupt; timers 0-4 
>> and 6-7 all
>> +	use one interrupt line; the OST uses the last interrupt.
> 
> "The OST uses the last interrupt." is not clear to someone who doesn't 
> know
> about this hardware. (I can read it several ways.)
> 
> Does it mean that the 4770 and 4780 have 3 interrupt lines used like 
> so?
> 
> - timer 5 uses one interrupt line
> - timers 0-4 and 6-7 use a second interrupt line
> - the OST uses a third interrupt line

Correct. I'll make it more obvious.

Thanks,
-Paul

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

[PATCH v4 4/8] 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>
---
 .../bindings/clock/ingenic,tcu-clocks.txt          | 42 ++++++++++++++++
 .../bindings/interrupt-controller/ingenic,tcu.txt  | 39 +++++++++++++++
 .../devicetree/bindings/mfd/ingenic,tcu.txt        | 56 ++++++++++++++++++++++
 .../devicetree/bindings/timer/ingenic,tcu.txt      | 41 ++++++++++++++++
 4 files changed, 178 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/ingenic,tcu-clocks.txt
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt
 create mode 100644 Documentation/devicetree/bindings/mfd/ingenic,tcu.txt
 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.

diff --git a/Documentation/devicetree/bindings/clock/ingenic,tcu-clocks.txt b/Documentation/devicetree/bindings/clock/ingenic,tcu-clocks.txt
new file mode 100644
index 000000000000..471d27078599
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/ingenic,tcu-clocks.txt
@@ -0,0 +1,42 @@
+Ingenic SoC TCU binding
+
+The TCU is the Timer/Counter Unit present in all Ingenic SoCs. It features 8
+channels, each one having its own clock, that can be started and stopped,
+reparented, and reclocked.
+
+Required properties:
+- compatible : One of:
+  * ingenic,jz4740-tcu-clocks,
+  * ingenic,jz4770-tcu-clocks,
+  * ingenic,jz4780-tcu-clocks.
+- clocks : List of phandle & clock specifiers for clocks external to the TCU.
+  The "pclk", "rtc" and "ext" 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>.
+
+Example:
+
+/ {
+	tcu: mfd@10002000 {
+		compatible = "ingenic,tcu", "simple-mfd", "syscon";
+		reg = <0x10002000 0x1000>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0x0 0x10002000 0x1000>;
+
+		tcu_clk: clocks@10 {
+			compatible = "ingenic,jz4740-tcu-clocks";
+			reg = <0x10 0xff0>;
+
+			clocks = <&ext>, <&rtc>, <&pclk>;
+			clock-names = "ext", "rtc", "pclk";
+
+			#clock-cells = <1>;
+		};
+	};
+};
+
+For information about the top-level "ingenic,tcu" compatible node and other
+children nodes, see Documentation/devicetree/bindings/mfd/ingenic,tcu.txt.
diff --git a/Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt b/Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt
new file mode 100644
index 000000000000..7f3af2da77cd
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt
@@ -0,0 +1,39 @@
+Ingenic SoCs Timer/Counter Unit Interrupt Controller
+
+Required properties:
+
+- compatible : should be "ingenic,<socname>-tcu-intc". Valid strings are:
+  * ingenic,jz4740-tcu-intc
+  * ingenic,jz4770-tcu-intc
+  * ingenic,jz4780-tcu-intc
+- interrupt-controller : Identifies the node as an interrupt controller
+- #interrupt-cells : Specifies the number of cells needed to encode an
+  interrupt source. The value shall be 1.
+- interrupt-parent : phandle of the interrupt controller.
+- interrupts : Specifies the interrupt the controller is connected to.
+
+Example:
+
+/ {
+	tcu: mfd@10002000 {
+		compatible = "ingenic,tcu", "simple-mfd", "syscon";
+		reg = <0x10002000 0x1000>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0x0 0x10002000 0x1000>;
+
+		tcu_irq: interrupt-controller@20 {
+			compatible = "ingenic,jz4740-tcu-intc";
+			reg = <0x20 0x20>;
+
+			interrupt-controller;
+			#interrupt-cells = <1>;
+
+			interrupt-parent = <&intc>;
+			interrupts = <15>;
+		};
+	};
+};
+
+For information about the top-level "ingenic,tcu" compatible node and other
+children nodes, see Documentation/devicetree/bindings/mfd/ingenic,tcu.txt.
diff --git a/Documentation/devicetree/bindings/mfd/ingenic,tcu.txt b/Documentation/devicetree/bindings/mfd/ingenic,tcu.txt
new file mode 100644
index 000000000000..5742c3f21550
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/ingenic,tcu.txt
@@ -0,0 +1,56 @@
+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.
+
+The TCU is implemented as a parent node, whose role is to create the
+regmap, and child nodes for the various drivers listed in the aforementioned
+document.
+
+Required properties:
+
+- compatible: must be "ingenic,tcu", "simple-mfd", "syscon";
+- 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
+
+Accepted children nodes:
+- Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt
+- Documentation/devicetree/bindings/clock/ingenic,tcu-clocks.txt
+- Documentation/devicetree/bindings/timer/ingenic,tcu.txt
+
+
+Example:
+
+/ {
+	tcu: mfd@10002000 {
+		compatible = "ingenic,tcu", "simple-mfd", "syscon";
+		reg = <0x10002000 0x1000>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0x0 0x10002000 0x1000>;
+
+		tcu_irq: interrupt-controller@20 {
+			compatible = "ingenic,jz4740-tcu-intc";
+			reg = <0x20 0x20>;
+			...
+		};
+
+		tcu_clk: clocks@10 {
+			compatible = "ingenic,jz4740-tcu-clocks";
+			reg = <0x10 0xff0>;
+			...
+		};
+
+		tcu_timer: timer@10 {
+			compatible = "ingenic,jz4740-tcu";
+			reg = <0x10 0xff0>;
+			...
+		};
+	};
+};
+
+For more information about the children node, refer to the documents listed
+above in the "Accepted children nodes" section.
diff --git a/Documentation/devicetree/bindings/timer/ingenic,tcu.txt b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
new file mode 100644
index 000000000000..f910b7e96783
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
@@ -0,0 +1,41 @@
+Ingenic JZ47xx SoCs Timer/Counter Unit driver
+---------------------------------------------
+
+Required properties:
+
+- compatible : should be "ingenic,<socname>-tcu". Valid strings are:
+  * ingenic,jz4740-tcu
+  * ingenic,jz4770-tcu
+  * ingenic,jz4780-tcu
+- interrupt-parent : phandle of the TCU interrupt controller.
+- interrupts : Specifies the interrupts the controller is connected to.
+- clocks : List of phandle & clock specifiers for the TCU clocks.
+- clock-names : List of name strings for the TCU clocks.
+
+Example:
+
+/ {
+	tcu: mfd@10002000 {
+		compatible = "ingenic,tcu", "simple-mfd", "syscon";
+		reg = <0x10002000 0x1000>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0x0 0x10002000 0x1000>;
+
+		tcu_timer: timer@10 {
+			compatible = "ingenic,jz4740-tcu";
+			reg = <0x10 0xff0>;
+
+			clocks = <&tcu_clk 0>, <&tcu_clk 1>, <&tcu_clk 2>, <&tcu_clk 3>,
+					 <&tcu_clk 4>, <&tcu_clk 5>, <&tcu_clk 6>, <&tcu_clk 7>;
+			clock-names = "timer0", "timer1", "timer2", "timer3",
+						  "timer4", "timer5", "timer6", "timer7";
+
+			interrupt-parent = <&tcu_irq>;
+			interrupts = <0 1 2 3 4 5 6 7>;
+		};
+	};
+};
+
+For information about the top-level "ingenic,tcu" compatible node and other
+children nodes, see Documentation/devicetree/bindings/mfd/ingenic,tcu.txt.
-- 
2.11.0

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

Re: [PATCH v4 4/8] dt-bindings: Add doc for the Ingenic TCU drivers

[Marc Zyngier]

On 17/03/18 23:28, 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>
> ---
>  .../bindings/clock/ingenic,tcu-clocks.txt          | 42 ++++++++++++++++
>  .../bindings/interrupt-controller/ingenic,tcu.txt  | 39 +++++++++++++++
>  .../devicetree/bindings/mfd/ingenic,tcu.txt        | 56 ++++++++++++++++++++++
>  .../devicetree/bindings/timer/ingenic,tcu.txt      | 41 ++++++++++++++++
>  4 files changed, 178 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/ingenic,tcu-clocks.txt
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt
>  create mode 100644 Documentation/devicetree/bindings/mfd/ingenic,tcu.txt
>  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.
> 
> diff --git a/Documentation/devicetree/bindings/clock/ingenic,tcu-clocks.txt b/Documentation/devicetree/bindings/clock/ingenic,tcu-clocks.txt
> new file mode 100644
> index 000000000000..471d27078599
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/ingenic,tcu-clocks.txt
> @@ -0,0 +1,42 @@
> +Ingenic SoC TCU binding
> +
> +The TCU is the Timer/Counter Unit present in all Ingenic SoCs. It features 8
> +channels, each one having its own clock, that can be started and stopped,
> +reparented, and reclocked.
> +
> +Required properties:
> +- compatible : One of:
> +  * ingenic,jz4740-tcu-clocks,
> +  * ingenic,jz4770-tcu-clocks,
> +  * ingenic,jz4780-tcu-clocks.
> +- clocks : List of phandle & clock specifiers for clocks external to the TCU.
> +  The "pclk", "rtc" and "ext" 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>.
> +
> +Example:
> +
> +/ {
> +	tcu: mfd@10002000 {
> +		compatible = "ingenic,tcu", "simple-mfd", "syscon";
> +		reg = <0x10002000 0x1000>;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges = <0x0 0x10002000 0x1000>;
> +
> +		tcu_clk: clocks@10 {
> +			compatible = "ingenic,jz4740-tcu-clocks";
> +			reg = <0x10 0xff0>;
> +
> +			clocks = <&ext>, <&rtc>, <&pclk>;
> +			clock-names = "ext", "rtc", "pclk";
> +
> +			#clock-cells = <1>;
> +		};
> +	};
> +};
> +
> +For information about the top-level "ingenic,tcu" compatible node and other
> +children nodes, see Documentation/devicetree/bindings/mfd/ingenic,tcu.txt.
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt b/Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt
> new file mode 100644
> index 000000000000..7f3af2da77cd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt
> @@ -0,0 +1,39 @@
> +Ingenic SoCs Timer/Counter Unit Interrupt Controller
> +
> +Required properties:
> +
> +- compatible : should be "ingenic,<socname>-tcu-intc". Valid strings are:
> +  * ingenic,jz4740-tcu-intc
> +  * ingenic,jz4770-tcu-intc
> +  * ingenic,jz4780-tcu-intc
> +- interrupt-controller : Identifies the node as an interrupt controller
> +- #interrupt-cells : Specifies the number of cells needed to encode an
> +  interrupt source. The value shall be 1.
> +- interrupt-parent : phandle of the interrupt controller.
> +- interrupts : Specifies the interrupt the controller is connected to.
> +
> +Example:
> +
> +/ {
> +	tcu: mfd@10002000 {
> +		compatible = "ingenic,tcu", "simple-mfd", "syscon";
> +		reg = <0x10002000 0x1000>;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges = <0x0 0x10002000 0x1000>;
> +
> +		tcu_irq: interrupt-controller@20 {
> +			compatible = "ingenic,jz4740-tcu-intc";
> +			reg = <0x20 0x20>;
> +
> +			interrupt-controller;
> +			#interrupt-cells = <1>;
> +
> +			interrupt-parent = <&intc>;
> +			interrupts = <15>;
> +		};
> +	};
> +};
> +
> +For information about the top-level "ingenic,tcu" compatible node and other
> +children nodes, see Documentation/devicetree/bindings/mfd/ingenic,tcu.txt.
> diff --git a/Documentation/devicetree/bindings/mfd/ingenic,tcu.txt b/Documentation/devicetree/bindings/mfd/ingenic,tcu.txt
> new file mode 100644
> index 000000000000..5742c3f21550
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/ingenic,tcu.txt
> @@ -0,0 +1,56 @@
> +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.
> +
> +The TCU is implemented as a parent node, whose role is to create the
> +regmap, and child nodes for the various drivers listed in the aforementioned
> +document.

You're describing the Linux driver here. Please stick to a description
of the HW.

> +
> +Required properties:
> +
> +- compatible: must be "ingenic,tcu", "simple-mfd", "syscon";

Without any provision for an SoC version? Seems bold...

> +- 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
> +
> +Accepted children nodes:
> +- Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt
> +- Documentation/devicetree/bindings/clock/ingenic,tcu-clocks.txt
> +- Documentation/devicetree/bindings/timer/ingenic,tcu.txt

It is slightly confusing that you have 3 files named ingenic,tcu.txt.
How about ingenic,tcu-intc.txt and ingenic,tcu-timer.txt (amending the
binding for the timer)?

> +
> +
> +Example:
> +
> +/ {
> +	tcu: mfd@10002000 {
> +		compatible = "ingenic,tcu", "simple-mfd", "syscon";
> +		reg = <0x10002000 0x1000>;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges = <0x0 0x10002000 0x1000>;
> +
> +		tcu_irq: interrupt-controller@20 {
> +			compatible = "ingenic,jz4740-tcu-intc";
> +			reg = <0x20 0x20>;
> +			...
> +		};
> +
> +		tcu_clk: clocks@10 {
> +			compatible = "ingenic,jz4740-tcu-clocks";
> +			reg = <0x10 0xff0>;
> +			...
> +		};
> +
> +		tcu_timer: timer@10 {
> +			compatible = "ingenic,jz4740-tcu";
> +			reg = <0x10 0xff0>;
> +			...
> +		};
> +	};
> +};
> +
> +For more information about the children node, refer to the documents listed
> +above in the "Accepted children nodes" section.
> diff --git a/Documentation/devicetree/bindings/timer/ingenic,tcu.txt b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
> new file mode 100644
> index 000000000000..f910b7e96783
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
> @@ -0,0 +1,41 @@
> +Ingenic JZ47xx SoCs Timer/Counter Unit driver
> +---------------------------------------------
> +
> +Required properties:
> +
> +- compatible : should be "ingenic,<socname>-tcu". Valid strings are:
> +  * ingenic,jz4740-tcu
> +  * ingenic,jz4770-tcu
> +  * ingenic,jz4780-tcu
> +- interrupt-parent : phandle of the TCU interrupt controller.
> +- interrupts : Specifies the interrupts the controller is connected to.
> +- clocks : List of phandle & clock specifiers for the TCU clocks.
> +- clock-names : List of name strings for the TCU clocks.
> +
> +Example:
> +
> +/ {
> +	tcu: mfd@10002000 {
> +		compatible = "ingenic,tcu", "simple-mfd", "syscon";
> +		reg = <0x10002000 0x1000>;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges = <0x0 0x10002000 0x1000>;
> +
> +		tcu_timer: timer@10 {
> +			compatible = "ingenic,jz4740-tcu";
> +			reg = <0x10 0xff0>;
> +
> +			clocks = <&tcu_clk 0>, <&tcu_clk 1>, <&tcu_clk 2>, <&tcu_clk 3>,
> +					 <&tcu_clk 4>, <&tcu_clk 5>, <&tcu_clk 6>, <&tcu_clk 7>;
> +			clock-names = "timer0", "timer1", "timer2", "timer3",
> +						  "timer4", "timer5", "timer6", "timer7";
> +
> +			interrupt-parent = <&tcu_irq>;
> +			interrupts = <0 1 2 3 4 5 6 7>;
> +		};
> +	};
> +};
> +
> +For information about the top-level "ingenic,tcu" compatible node and other
> +children nodes, see Documentation/devicetree/bindings/mfd/ingenic,tcu.txt.
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 4/8] dt-bindings: Add doc for the Ingenic TCU drivers,

[Paul Cercueil]

Le 2018-03-20 09:52, Marc Zyngier a écrit :
> On 17/03/18 23:28, 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>
>> ---
>>  .../bindings/clock/ingenic,tcu-clocks.txt          | 42 
>> ++++++++++++++++
>>  .../bindings/interrupt-controller/ingenic,tcu.txt  | 39 
>> +++++++++++++++
>>  .../devicetree/bindings/mfd/ingenic,tcu.txt        | 56 
>> ++++++++++++++++++++++
>>  .../devicetree/bindings/timer/ingenic,tcu.txt      | 41 
>> ++++++++++++++++
>>  4 files changed, 178 insertions(+)
>>  create mode 100644 
>> Documentation/devicetree/bindings/clock/ingenic,tcu-clocks.txt
>>  create mode 100644 
>> Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt
>>  create mode 100644 
>> Documentation/devicetree/bindings/mfd/ingenic,tcu.txt
>>  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.
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/clock/ingenic,tcu-clocks.txt 
>> b/Documentation/devicetree/bindings/clock/ingenic,tcu-clocks.txt
>> new file mode 100644
>> index 000000000000..471d27078599
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/ingenic,tcu-clocks.txt
>> @@ -0,0 +1,42 @@
>> +Ingenic SoC TCU binding
>> +
>> +The TCU is the Timer/Counter Unit present in all Ingenic SoCs. It 
>> features 8
>> +channels, each one having its own clock, that can be started and 
>> stopped,
>> +reparented, and reclocked.
>> +
>> +Required properties:
>> +- compatible : One of:
>> +  * ingenic,jz4740-tcu-clocks,
>> +  * ingenic,jz4770-tcu-clocks,
>> +  * ingenic,jz4780-tcu-clocks.
>> +- clocks : List of phandle & clock specifiers for clocks external to 
>> the TCU.
>> +  The "pclk", "rtc" and "ext" 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>.
>> +
>> +Example:
>> +
>> +/ {
>> +	tcu: mfd@10002000 {
>> +		compatible = "ingenic,tcu", "simple-mfd", "syscon";
>> +		reg = <0x10002000 0x1000>;
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		ranges = <0x0 0x10002000 0x1000>;
>> +
>> +		tcu_clk: clocks@10 {
>> +			compatible = "ingenic,jz4740-tcu-clocks";
>> +			reg = <0x10 0xff0>;
>> +
>> +			clocks = <&ext>, <&rtc>, <&pclk>;
>> +			clock-names = "ext", "rtc", "pclk";
>> +
>> +			#clock-cells = <1>;
>> +		};
>> +	};
>> +};
>> +
>> +For information about the top-level "ingenic,tcu" compatible node and 
>> other
>> +children nodes, see 
>> Documentation/devicetree/bindings/mfd/ingenic,tcu.txt.
>> diff --git 
>> a/Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt 
>> b/Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt
>> new file mode 100644
>> index 000000000000..7f3af2da77cd
>> --- /dev/null
>> +++ 
>> b/Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt
>> @@ -0,0 +1,39 @@
>> +Ingenic SoCs Timer/Counter Unit Interrupt Controller
>> +
>> +Required properties:
>> +
>> +- compatible : should be "ingenic,<socname>-tcu-intc". Valid strings 
>> are:
>> +  * ingenic,jz4740-tcu-intc
>> +  * ingenic,jz4770-tcu-intc
>> +  * ingenic,jz4780-tcu-intc
>> +- interrupt-controller : Identifies the node as an interrupt 
>> controller
>> +- #interrupt-cells : Specifies the number of cells needed to encode 
>> an
>> +  interrupt source. The value shall be 1.
>> +- interrupt-parent : phandle of the interrupt controller.
>> +- interrupts : Specifies the interrupt the controller is connected 
>> to.
>> +
>> +Example:
>> +
>> +/ {
>> +	tcu: mfd@10002000 {
>> +		compatible = "ingenic,tcu", "simple-mfd", "syscon";
>> +		reg = <0x10002000 0x1000>;
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		ranges = <0x0 0x10002000 0x1000>;
>> +
>> +		tcu_irq: interrupt-controller@20 {
>> +			compatible = "ingenic,jz4740-tcu-intc";
>> +			reg = <0x20 0x20>;
>> +
>> +			interrupt-controller;
>> +			#interrupt-cells = <1>;
>> +
>> +			interrupt-parent = <&intc>;
>> +			interrupts = <15>;
>> +		};
>> +	};
>> +};
>> +
>> +For information about the top-level "ingenic,tcu" compatible node and 
>> other
>> +children nodes, see 
>> Documentation/devicetree/bindings/mfd/ingenic,tcu.txt.
>> diff --git a/Documentation/devicetree/bindings/mfd/ingenic,tcu.txt 
>> b/Documentation/devicetree/bindings/mfd/ingenic,tcu.txt
>> new file mode 100644
>> index 000000000000..5742c3f21550
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/ingenic,tcu.txt
>> @@ -0,0 +1,56 @@
>> +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.
>> +
>> +The TCU is implemented as a parent node, whose role is to create the
>> +regmap, and child nodes for the various drivers listed in the 
>> aforementioned
>> +document.
> 
> You're describing the Linux driver here. Please stick to a description
> of the HW.

There's already a whole file that describes the hardware. I just wanted 
to mention
that there should be child nodes, but I can rephrase that.

>> +
>> +Required properties:
>> +
>> +- compatible: must be "ingenic,tcu", "simple-mfd", "syscon";
> 
> Without any provision for an SoC version? Seems bold...

Well, we don't use the "ingenic,tcu" compatible string, the parent node 
is just
here to create the regmap (in this version of the patchset, it will 
probably change
later).

>> +- 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
>> +
>> +Accepted children nodes:
>> +- 
>> Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt
>> +- Documentation/devicetree/bindings/clock/ingenic,tcu-clocks.txt
>> +- Documentation/devicetree/bindings/timer/ingenic,tcu.txt
> 
> It is slightly confusing that you have 3 files named ingenic,tcu.txt.
> How about ingenic,tcu-intc.txt and ingenic,tcu-timer.txt (amending the
> binding for the timer)?

Right. I'll change that.

>> +
>> +
>> +Example:
>> +
>> +/ {
>> +	tcu: mfd@10002000 {
>> +		compatible = "ingenic,tcu", "simple-mfd", "syscon";
>> +		reg = <0x10002000 0x1000>;
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		ranges = <0x0 0x10002000 0x1000>;
>> +
>> +		tcu_irq: interrupt-controller@20 {
>> +			compatible = "ingenic,jz4740-tcu-intc";
>> +			reg = <0x20 0x20>;
>> +			...
>> +		};
>> +
>> +		tcu_clk: clocks@10 {
>> +			compatible = "ingenic,jz4740-tcu-clocks";
>> +			reg = <0x10 0xff0>;
>> +			...
>> +		};
>> +
>> +		tcu_timer: timer@10 {
>> +			compatible = "ingenic,jz4740-tcu";
>> +			reg = <0x10 0xff0>;
>> +			...
>> +		};
>> +	};
>> +};
>> +
>> +For more information about the children node, refer to the documents 
>> listed
>> +above in the "Accepted children nodes" section.
>> diff --git a/Documentation/devicetree/bindings/timer/ingenic,tcu.txt 
>> b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
>> new file mode 100644
>> index 000000000000..f910b7e96783
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
>> @@ -0,0 +1,41 @@
>> +Ingenic JZ47xx SoCs Timer/Counter Unit driver
>> +---------------------------------------------
>> +
>> +Required properties:
>> +
>> +- compatible : should be "ingenic,<socname>-tcu". Valid strings are:
>> +  * ingenic,jz4740-tcu
>> +  * ingenic,jz4770-tcu
>> +  * ingenic,jz4780-tcu
>> +- interrupt-parent : phandle of the TCU interrupt controller.
>> +- interrupts : Specifies the interrupts the controller is connected 
>> to.
>> +- clocks : List of phandle & clock specifiers for the TCU clocks.
>> +- clock-names : List of name strings for the TCU clocks.
>> +
>> +Example:
>> +
>> +/ {
>> +	tcu: mfd@10002000 {
>> +		compatible = "ingenic,tcu", "simple-mfd", "syscon";
>> +		reg = <0x10002000 0x1000>;
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		ranges = <0x0 0x10002000 0x1000>;
>> +
>> +		tcu_timer: timer@10 {
>> +			compatible = "ingenic,jz4740-tcu";
>> +			reg = <0x10 0xff0>;
>> +
>> +			clocks = <&tcu_clk 0>, <&tcu_clk 1>, <&tcu_clk 2>, <&tcu_clk 3>,
>> +					 <&tcu_clk 4>, <&tcu_clk 5>, <&tcu_clk 6>, <&tcu_clk 7>;
>> +			clock-names = "timer0", "timer1", "timer2", "timer3",
>> +						  "timer4", "timer5", "timer6", "timer7";
>> +
>> +			interrupt-parent = <&tcu_irq>;
>> +			interrupts = <0 1 2 3 4 5 6 7>;
>> +		};
>> +	};
>> +};
>> +
>> +For information about the top-level "ingenic,tcu" compatible node and 
>> other
>> +children nodes, see 
>> Documentation/devicetree/bindings/mfd/ingenic,tcu.txt.
>> 
> 
> Thanks,
> 
> 	M.

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

Re: [PATCH v4 4/8] dt-bindings: Add doc for the Ingenic TCU drivers

[Rob Herring]

On Sun, Mar 18, 2018 at 12:28:57AM +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@crapouillou.net>
> ---
>  .../bindings/clock/ingenic,tcu-clocks.txt          | 42 ++++++++++++++++
>  .../bindings/interrupt-controller/ingenic,tcu.txt  | 39 +++++++++++++++
>  .../devicetree/bindings/mfd/ingenic,tcu.txt        | 56 ++++++++++++++++++++++
>  .../devicetree/bindings/timer/ingenic,tcu.txt      | 41 ++++++++++++++++
>  4 files changed, 178 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/ingenic,tcu-clocks.txt
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt
>  create mode 100644 Documentation/devicetree/bindings/mfd/ingenic,tcu.txt
>  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.
> 
> diff --git a/Documentation/devicetree/bindings/clock/ingenic,tcu-clocks.txt b/Documentation/devicetree/bindings/clock/ingenic,tcu-clocks.txt
> new file mode 100644
> index 000000000000..471d27078599
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/ingenic,tcu-clocks.txt
> @@ -0,0 +1,42 @@
> +Ingenic SoC TCU binding
> +
> +The TCU is the Timer/Counter Unit present in all Ingenic SoCs. It features 8
> +channels, each one having its own clock, that can be started and stopped,
> +reparented, and reclocked.
> +
> +Required properties:
> +- compatible : One of:
> +  * ingenic,jz4740-tcu-clocks,
> +  * ingenic,jz4770-tcu-clocks,
> +  * ingenic,jz4780-tcu-clocks.
> +- clocks : List of phandle & clock specifiers for clocks external to the TCU.
> +  The "pclk", "rtc" and "ext" 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>.
> +
> +Example:

Let's just put one complete example in instead of all these duplicated 
and incomplete examples.

> +
> +/ {
> +	tcu: mfd@10002000 {
> +		compatible = "ingenic,tcu", "simple-mfd", "syscon";
> +		reg = <0x10002000 0x1000>;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges = <0x0 0x10002000 0x1000>;
> +
> +		tcu_clk: clocks@10 {
> +			compatible = "ingenic,jz4740-tcu-clocks";
> +			reg = <0x10 0xff0>;
> +
> +			clocks = <&ext>, <&rtc>, <&pclk>;
> +			clock-names = "ext", "rtc", "pclk";
> +
> +			#clock-cells = <1>;
> +		};
> +	};
> +};
> +
> +For information about the top-level "ingenic,tcu" compatible node and other
> +children nodes, see Documentation/devicetree/bindings/mfd/ingenic,tcu.txt.
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt b/Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt
> new file mode 100644
> index 000000000000..7f3af2da77cd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt
> @@ -0,0 +1,39 @@
> +Ingenic SoCs Timer/Counter Unit Interrupt Controller
> +
> +Required properties:
> +
> +- compatible : should be "ingenic,<socname>-tcu-intc". Valid strings are:
> +  * ingenic,jz4740-tcu-intc
> +  * ingenic,jz4770-tcu-intc
> +  * ingenic,jz4780-tcu-intc
> +- interrupt-controller : Identifies the node as an interrupt controller
> +- #interrupt-cells : Specifies the number of cells needed to encode an
> +  interrupt source. The value shall be 1.
> +- interrupt-parent : phandle of the interrupt controller.
> +- interrupts : Specifies the interrupt the controller is connected to.
> +
> +Example:
> +
> +/ {
> +	tcu: mfd@10002000 {
> +		compatible = "ingenic,tcu", "simple-mfd", "syscon";
> +		reg = <0x10002000 0x1000>;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges = <0x0 0x10002000 0x1000>;
> +
> +		tcu_irq: interrupt-controller@20 {
> +			compatible = "ingenic,jz4740-tcu-intc";
> +			reg = <0x20 0x20>;
> +
> +			interrupt-controller;
> +			#interrupt-cells = <1>;
> +
> +			interrupt-parent = <&intc>;
> +			interrupts = <15>;

The interrupt controller doesn't require any clocks?

> +		};
> +	};
> +};
> +
> +For information about the top-level "ingenic,tcu" compatible node and other
> +children nodes, see Documentation/devicetree/bindings/mfd/ingenic,tcu.txt.
> diff --git a/Documentation/devicetree/bindings/mfd/ingenic,tcu.txt b/Documentation/devicetree/bindings/mfd/ingenic,tcu.txt
> new file mode 100644
> index 000000000000..5742c3f21550
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/ingenic,tcu.txt
> @@ -0,0 +1,56 @@
> +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.
> +
> +The TCU is implemented as a parent node, whose role is to create the
> +regmap, and child nodes for the various drivers listed in the aforementioned
> +document.
> +
> +Required properties:
> +
> +- compatible: must be "ingenic,tcu", "simple-mfd", "syscon";
> +- 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
> +
> +Accepted children nodes:
> +- Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt
> +- Documentation/devicetree/bindings/clock/ingenic,tcu-clocks.txt
> +- Documentation/devicetree/bindings/timer/ingenic,tcu.txt
> +
> +
> +Example:
> +
> +/ {
> +	tcu: mfd@10002000 {
> +		compatible = "ingenic,tcu", "simple-mfd", "syscon";
> +		reg = <0x10002000 0x1000>;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges = <0x0 0x10002000 0x1000>;
> +
> +		tcu_irq: interrupt-controller@20 {
> +			compatible = "ingenic,jz4740-tcu-intc";
> +			reg = <0x20 0x20>;

I think you should drop this node and make the parent node the interrupt 
controller. That is the normal pattern where the parent node handles 
all the common functions. Otherwise, there is no need to have the parent 
node. You should then also drop simple-mfd as then you can control 
initialization order by initializing interrupt controller before 
its clients.

> +			...
> +		};
> +
> +		tcu_clk: clocks@10 {
> +			compatible = "ingenic,jz4740-tcu-clocks";
> +			reg = <0x10 0xff0>;
> +			...
> +		};
> +
> +		tcu_timer: timer@10 {
> +			compatible = "ingenic,jz4740-tcu";
> +			reg = <0x10 0xff0>;

Is this copy-n-paste or you really have 2 nodes at the same address? The 
latter is not valid.

> +			...
> +		};
> +	};
> +};
> +
> +For more information about the children node, refer to the documents listed
> +above in the "Accepted children nodes" section.
> diff --git a/Documentation/devicetree/bindings/timer/ingenic,tcu.txt b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
> new file mode 100644
> index 000000000000..f910b7e96783
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
> @@ -0,0 +1,41 @@
> +Ingenic JZ47xx SoCs Timer/Counter Unit driver
> +---------------------------------------------
> +
> +Required properties:
> +
> +- compatible : should be "ingenic,<socname>-tcu". Valid strings are:
> +  * ingenic,jz4740-tcu
> +  * ingenic,jz4770-tcu
> +  * ingenic,jz4780-tcu
> +- interrupt-parent : phandle of the TCU interrupt controller.
> +- interrupts : Specifies the interrupts the controller is connected to.
> +- clocks : List of phandle & clock specifiers for the TCU clocks.
> +- clock-names : List of name strings for the TCU clocks.
> +
> +Example:
> +
> +/ {
> +	tcu: mfd@10002000 {
> +		compatible = "ingenic,tcu", "simple-mfd", "syscon";
> +		reg = <0x10002000 0x1000>;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges = <0x0 0x10002000 0x1000>;
> +
> +		tcu_timer: timer@10 {
> +			compatible = "ingenic,jz4740-tcu";
> +			reg = <0x10 0xff0>;
> +
> +			clocks = <&tcu_clk 0>, <&tcu_clk 1>, <&tcu_clk 2>, <&tcu_clk 3>,
> +					 <&tcu_clk 4>, <&tcu_clk 5>, <&tcu_clk 6>, <&tcu_clk 7>;
> +			clock-names = "timer0", "timer1", "timer2", "timer3",
> +						  "timer4", "timer5", "timer6", "timer7";
> +
> +			interrupt-parent = <&tcu_irq>;
> +			interrupts = <0 1 2 3 4 5 6 7>;

Thinking about this some more... You simply have 8 timers (and no other 
functions?) with some internal clock and irq controls for each timer. I 
don't think it really makes sense to create separate clock and irq 
drivers in that case. That would be like creating clock drivers for 
every clock divider in timers, pwms, uarts, etc. Unless the clocks get 
exposed to other parts of the system, then there is no point.

> +		};
> +	};
> +};
> +
> +For information about the top-level "ingenic,tcu" compatible node and other
> +children nodes, see Documentation/devicetree/bindings/mfd/ingenic,tcu.txt.
> -- 
> 2.11.0
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 4/8] dt-bindings: Add doc for the Ingenic TCU drivers,

[Paul Cercueil]

Le 2018-03-27 16:46, Rob Herring a écrit :
> On Sun, Mar 18, 2018 at 12:28:57AM +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@crapouillou.net>
>> ---
>>  .../bindings/clock/ingenic,tcu-clocks.txt          | 42 
>> ++++++++++++++++
>>  .../bindings/interrupt-controller/ingenic,tcu.txt  | 39 
>> +++++++++++++++
>>  .../devicetree/bindings/mfd/ingenic,tcu.txt        | 56 
>> ++++++++++++++++++++++
>>  .../devicetree/bindings/timer/ingenic,tcu.txt      | 41 
>> ++++++++++++++++
>>  4 files changed, 178 insertions(+)
>>  create mode 100644 
>> Documentation/devicetree/bindings/clock/ingenic,tcu-clocks.txt
>>  create mode 100644 
>> Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt
>>  create mode 100644 
>> Documentation/devicetree/bindings/mfd/ingenic,tcu.txt
>>  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.
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/clock/ingenic,tcu-clocks.txt 
>> b/Documentation/devicetree/bindings/clock/ingenic,tcu-clocks.txt
>> new file mode 100644
>> index 000000000000..471d27078599
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/ingenic,tcu-clocks.txt
>> @@ -0,0 +1,42 @@
>> +Ingenic SoC TCU binding
>> +
>> +The TCU is the Timer/Counter Unit present in all Ingenic SoCs. It 
>> features 8
>> +channels, each one having its own clock, that can be started and 
>> stopped,
>> +reparented, and reclocked.
>> +
>> +Required properties:
>> +- compatible : One of:
>> +  * ingenic,jz4740-tcu-clocks,
>> +  * ingenic,jz4770-tcu-clocks,
>> +  * ingenic,jz4780-tcu-clocks.
>> +- clocks : List of phandle & clock specifiers for clocks external to 
>> the TCU.
>> +  The "pclk", "rtc" and "ext" 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>.
>> +
>> +Example:
> 
> Let's just put one complete example in instead of all these duplicated
> and incomplete examples.

Alright.

>> +
>> +/ {
>> +	tcu: mfd@10002000 {
>> +		compatible = "ingenic,tcu", "simple-mfd", "syscon";
>> +		reg = <0x10002000 0x1000>;
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		ranges = <0x0 0x10002000 0x1000>;
>> +
>> +		tcu_clk: clocks@10 {
>> +			compatible = "ingenic,jz4740-tcu-clocks";
>> +			reg = <0x10 0xff0>;
>> +
>> +			clocks = <&ext>, <&rtc>, <&pclk>;
>> +			clock-names = "ext", "rtc", "pclk";
>> +
>> +			#clock-cells = <1>;
>> +		};
>> +	};
>> +};
>> +
>> +For information about the top-level "ingenic,tcu" compatible node and 
>> other
>> +children nodes, see 
>> Documentation/devicetree/bindings/mfd/ingenic,tcu.txt.
>> diff --git 
>> a/Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt 
>> b/Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt
>> new file mode 100644
>> index 000000000000..7f3af2da77cd
>> --- /dev/null
>> +++ 
>> b/Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt
>> @@ -0,0 +1,39 @@
>> +Ingenic SoCs Timer/Counter Unit Interrupt Controller
>> +
>> +Required properties:
>> +
>> +- compatible : should be "ingenic,<socname>-tcu-intc". Valid strings 
>> are:
>> +  * ingenic,jz4740-tcu-intc
>> +  * ingenic,jz4770-tcu-intc
>> +  * ingenic,jz4780-tcu-intc
>> +- interrupt-controller : Identifies the node as an interrupt 
>> controller
>> +- #interrupt-cells : Specifies the number of cells needed to encode 
>> an
>> +  interrupt source. The value shall be 1.
>> +- interrupt-parent : phandle of the interrupt controller.
>> +- interrupts : Specifies the interrupt the controller is connected 
>> to.
>> +
>> +Example:
>> +
>> +/ {
>> +	tcu: mfd@10002000 {
>> +		compatible = "ingenic,tcu", "simple-mfd", "syscon";
>> +		reg = <0x10002000 0x1000>;
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		ranges = <0x0 0x10002000 0x1000>;
>> +
>> +		tcu_irq: interrupt-controller@20 {
>> +			compatible = "ingenic,jz4740-tcu-intc";
>> +			reg = <0x20 0x20>;
>> +
>> +			interrupt-controller;
>> +			#interrupt-cells = <1>;
>> +
>> +			interrupt-parent = <&intc>;
>> +			interrupts = <15>;
> 
> The interrupt controller doesn't require any clocks?

It doesn't. The TCU registers not related to TCU channels are always 
active.

>> +		};
>> +	};
>> +};
>> +
>> +For information about the top-level "ingenic,tcu" compatible node and 
>> other
>> +children nodes, see 
>> Documentation/devicetree/bindings/mfd/ingenic,tcu.txt.
>> diff --git a/Documentation/devicetree/bindings/mfd/ingenic,tcu.txt 
>> b/Documentation/devicetree/bindings/mfd/ingenic,tcu.txt
>> new file mode 100644
>> index 000000000000..5742c3f21550
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/ingenic,tcu.txt
>> @@ -0,0 +1,56 @@
>> +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.
>> +
>> +The TCU is implemented as a parent node, whose role is to create the
>> +regmap, and child nodes for the various drivers listed in the 
>> aforementioned
>> +document.
>> +
>> +Required properties:
>> +
>> +- compatible: must be "ingenic,tcu", "simple-mfd", "syscon";
>> +- 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
>> +
>> +Accepted children nodes:
>> +- 
>> Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt
>> +- Documentation/devicetree/bindings/clock/ingenic,tcu-clocks.txt
>> +- Documentation/devicetree/bindings/timer/ingenic,tcu.txt
>> +
>> +
>> +Example:
>> +
>> +/ {
>> +	tcu: mfd@10002000 {
>> +		compatible = "ingenic,tcu", "simple-mfd", "syscon";
>> +		reg = <0x10002000 0x1000>;
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		ranges = <0x0 0x10002000 0x1000>;
>> +
>> +		tcu_irq: interrupt-controller@20 {
>> +			compatible = "ingenic,jz4740-tcu-intc";
>> +			reg = <0x20 0x20>;
> 
> I think you should drop this node and make the parent node the 
> interrupt
> controller. That is the normal pattern where the parent node handles
> all the common functions. Otherwise, there is no need to have the 
> parent
> node. You should then also drop simple-mfd as then you can control
> initialization order by initializing interrupt controller before
> its clients.

Alright, good idea.

>> +			...
>> +		};
>> +
>> +		tcu_clk: clocks@10 {
>> +			compatible = "ingenic,jz4740-tcu-clocks";
>> +			reg = <0x10 0xff0>;
>> +			...
>> +		};
>> +
>> +		tcu_timer: timer@10 {
>> +			compatible = "ingenic,jz4740-tcu";
>> +			reg = <0x10 0xff0>;
> 
> Is this copy-n-paste or you really have 2 nodes at the same address? 
> The
> latter is not valid.

I do have 2 nodes at the same address...

>> +			...
>> +		};
>> +	};
>> +};
>> +
>> +For more information about the children node, refer to the documents 
>> listed
>> +above in the "Accepted children nodes" section.
>> diff --git a/Documentation/devicetree/bindings/timer/ingenic,tcu.txt 
>> b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
>> new file mode 100644
>> index 000000000000..f910b7e96783
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
>> @@ -0,0 +1,41 @@
>> +Ingenic JZ47xx SoCs Timer/Counter Unit driver
>> +---------------------------------------------
>> +
>> +Required properties:
>> +
>> +- compatible : should be "ingenic,<socname>-tcu". Valid strings are:
>> +  * ingenic,jz4740-tcu
>> +  * ingenic,jz4770-tcu
>> +  * ingenic,jz4780-tcu
>> +- interrupt-parent : phandle of the TCU interrupt controller.
>> +- interrupts : Specifies the interrupts the controller is connected 
>> to.
>> +- clocks : List of phandle & clock specifiers for the TCU clocks.
>> +- clock-names : List of name strings for the TCU clocks.
>> +
>> +Example:
>> +
>> +/ {
>> +	tcu: mfd@10002000 {
>> +		compatible = "ingenic,tcu", "simple-mfd", "syscon";
>> +		reg = <0x10002000 0x1000>;
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		ranges = <0x0 0x10002000 0x1000>;
>> +
>> +		tcu_timer: timer@10 {
>> +			compatible = "ingenic,jz4740-tcu";
>> +			reg = <0x10 0xff0>;
>> +
>> +			clocks = <&tcu_clk 0>, <&tcu_clk 1>, <&tcu_clk 2>, <&tcu_clk 3>,
>> +					 <&tcu_clk 4>, <&tcu_clk 5>, <&tcu_clk 6>, <&tcu_clk 7>;
>> +			clock-names = "timer0", "timer1", "timer2", "timer3",
>> +						  "timer4", "timer5", "timer6", "timer7";
>> +
>> +			interrupt-parent = <&tcu_irq>;
>> +			interrupts = <0 1 2 3 4 5 6 7>;
> 
> Thinking about this some more... You simply have 8 timers (and no other
> functions?) with some internal clock and irq controls for each timer. I
> don't think it really makes sense to create separate clock and irq
> drivers in that case. That would be like creating clock drivers for
> every clock divider in timers, pwms, uarts, etc. Unless the clocks get
> exposed to other parts of the system, then there is no point.

I have 8 timers with some internal clock and IRQ controls, that can be 
used
as PWM. But the TCU also controls the IRQ of the OS Timer (which is 
separate),
as well as masking of the clocks for the OS timer and the watchdog.

I thought having clean drivers for different frameworks working on the 
same
regmap was cleaner than having one big-ass driver handling everything.

>> +		};
>> +	};
>> +};
>> +
>> +For information about the top-level "ingenic,tcu" compatible node and 
>> other
>> +children nodes, see 
>> Documentation/devicetree/bindings/mfd/ingenic,tcu.txt.
>> --
>> 2.11.0
>> 

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

Re: [PATCH v4 4/8] dt-bindings: Add doc for the Ingenic TCU drivers

[Rob Herring]

On Wed, Mar 28, 2018 at 10:33 AM, Paul Cercueil <paul@crapouillou.net> wrote:
> Le 2018-03-27 16:46, Rob Herring a écrit :
>>
>> On Sun, Mar 18, 2018 at 12:28:57AM +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@crapouillou.net>
>>> ---
>>>  .../bindings/clock/ingenic,tcu-clocks.txt          | 42 ++++++++++++++++
>>>  .../bindings/interrupt-controller/ingenic,tcu.txt  | 39 +++++++++++++++
>>>  .../devicetree/bindings/mfd/ingenic,tcu.txt        | 56
>>> ++++++++++++++++++++++
>>>  .../devicetree/bindings/timer/ingenic,tcu.txt      | 41 ++++++++++++++++
>>>  4 files changed, 178 insertions(+)
>>>  create mode 100644
>>> Documentation/devicetree/bindings/clock/ingenic,tcu-clocks.txt
>>>  create mode 100644
>>> Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt
>>>  create mode 100644 Documentation/devicetree/bindings/mfd/ingenic,tcu.txt
>>>  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.
>>> +/ {
>>> +       tcu: mfd@10002000 {
>>> +               compatible = "ingenic,tcu", "simple-mfd", "syscon";
>>> +               reg = <0x10002000 0x1000>;
>>> +               #address-cells = <1>;
>>> +               #size-cells = <1>;
>>> +               ranges = <0x0 0x10002000 0x1000>;
>>> +
>>> +               tcu_timer: timer@10 {
>>> +                       compatible = "ingenic,jz4740-tcu";
>>> +                       reg = <0x10 0xff0>;
>>> +
>>> +                       clocks = <&tcu_clk 0>, <&tcu_clk 1>, <&tcu_clk
>>> 2>, <&tcu_clk 3>,
>>> +                                        <&tcu_clk 4>, <&tcu_clk 5>,
>>> <&tcu_clk 6>, <&tcu_clk 7>;
>>> +                       clock-names = "timer0", "timer1", "timer2",
>>> "timer3",
>>> +                                                 "timer4", "timer5",
>>> "timer6", "timer7";
>>> +
>>> +                       interrupt-parent = <&tcu_irq>;
>>> +                       interrupts = <0 1 2 3 4 5 6 7>;
>>
>>
>> Thinking about this some more... You simply have 8 timers (and no other
>> functions?) with some internal clock and irq controls for each timer. I
>> don't think it really makes sense to create separate clock and irq
>> drivers in that case. That would be like creating clock drivers for
>> every clock divider in timers, pwms, uarts, etc. Unless the clocks get
>> exposed to other parts of the system, then there is no point.
>
>
> I have 8 timers with some internal clock and IRQ controls, that can be used
> as PWM.

Please include how you plan to do the PWM support too. I need a
complete picture of the h/w, not piecemeal, evolving bindings.

> But the TCU also controls the IRQ of the OS Timer (which is
> separate),
> as well as masking of the clocks for the OS timer and the watchdog.

The OS timer and watchdog are different blocks outside the TCU? This
doesn't seem to be the case based on the register definitions.

> I thought having clean drivers for different frameworks working on the same
> regmap was cleaner than having one big-ass driver handling everything.

DT is not the only way to instantiate drivers and how one OS splits
drivers should not define your DT binding. An MFD driver can create
child devices and a single DT node can be a provider of multiple
things. It is appropriate for an MFD to have child nodes primarily
when the sub devices need their own resources defined as properties in
DT or when the sub device is an IP block reused in multiple devices.
Just to have a node per driver/provider is not what should drive the
decision.

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

Re: [PATCH v4 4/8] dt-bindings: Add doc for the Ingenic TCU drivers

[Paul Cercueil]

Hi Rob,

Le mer. 28 mars 2018 à 18:28, Rob Herring <robh@kernel.org> a écrit :
> On Wed, Mar 28, 2018 at 10:33 AM, Paul Cercueil 
> <paul@crapouillou.net> wrote:
>>  Le 2018-03-27 16:46, Rob Herring a écrit :
>>> 
>>>  On Sun, Mar 18, 2018 at 12:28:57AM +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@crapouillou.net>
>>>>  ---
>>>>   .../bindings/clock/ingenic,tcu-clocks.txt          | 42 
>>>> ++++++++++++++++
>>>>   .../bindings/interrupt-controller/ingenic,tcu.txt  | 39 
>>>> +++++++++++++++
>>>>   .../devicetree/bindings/mfd/ingenic,tcu.txt        | 56
>>>>  ++++++++++++++++++++++
>>>>   .../devicetree/bindings/timer/ingenic,tcu.txt      | 41 
>>>> ++++++++++++++++
>>>>   4 files changed, 178 insertions(+)
>>>>   create mode 100644
>>>>  Documentation/devicetree/bindings/clock/ingenic,tcu-clocks.txt
>>>>   create mode 100644
>>>>  
>>>> Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt
>>>>   create mode 100644 
>>>> Documentation/devicetree/bindings/mfd/ingenic,tcu.txt
>>>>   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.
>>>>  +/ {
>>>>  +       tcu: mfd@10002000 {
>>>>  +               compatible = "ingenic,tcu", "simple-mfd", 
>>>> "syscon";
>>>>  +               reg = <0x10002000 0x1000>;
>>>>  +               #address-cells = <1>;
>>>>  +               #size-cells = <1>;
>>>>  +               ranges = <0x0 0x10002000 0x1000>;
>>>>  +
>>>>  +               tcu_timer: timer@10 {
>>>>  +                       compatible = "ingenic,jz4740-tcu";
>>>>  +                       reg = <0x10 0xff0>;
>>>>  +
>>>>  +                       clocks = <&tcu_clk 0>, <&tcu_clk 1>, 
>>>> <&tcu_clk
>>>>  2>, <&tcu_clk 3>,
>>>>  +                                        <&tcu_clk 4>, <&tcu_clk 
>>>> 5>,
>>>>  <&tcu_clk 6>, <&tcu_clk 7>;
>>>>  +                       clock-names = "timer0", "timer1", 
>>>> "timer2",
>>>>  "timer3",
>>>>  +                                                 "timer4", 
>>>> "timer5",
>>>>  "timer6", "timer7";
>>>>  +
>>>>  +                       interrupt-parent = <&tcu_irq>;
>>>>  +                       interrupts = <0 1 2 3 4 5 6 7>;
>>> 
>>> 
>>>  Thinking about this some more... You simply have 8 timers (and no 
>>> other
>>>  functions?) with some internal clock and irq controls for each 
>>> timer. I
>>>  don't think it really makes sense to create separate clock and irq
>>>  drivers in that case. That would be like creating clock drivers for
>>>  every clock divider in timers, pwms, uarts, etc. Unless the clocks 
>>> get
>>>  exposed to other parts of the system, then there is no point.
>> 
>> 
>>  I have 8 timers with some internal clock and IRQ controls, that can 
>> be used
>>  as PWM.
> 
> Please include how you plan to do the PWM support too. I need a
> complete picture of the h/w, not piecemeal, evolving bindings.

Alright.

>>  But the TCU also controls the IRQ of the OS Timer (which is
>>  separate),
>>  as well as masking of the clocks for the OS timer and the watchdog.
> 
> The OS timer and watchdog are different blocks outside the TCU? This
> doesn't seem to be the case based on the register definitions.

Their register areas are mostly separate, although contiguous. On the 
other
hand, the watchdog and OST can be started/stopped from a bit within a 
TCU
register, so they're probably part of the same h/w block.

>>  I thought having clean drivers for different frameworks working on 
>> the same
>>  regmap was cleaner than having one big-ass driver handling 
>> everything.
> 
> DT is not the only way to instantiate drivers and how one OS splits
> drivers should not define your DT binding. An MFD driver can create
> child devices and a single DT node can be a provider of multiple
> things. It is appropriate for an MFD to have child nodes primarily
> when the sub devices need their own resources defined as properties in
> DT or when the sub device is an IP block reused in multiple devices.
> Just to have a node per driver/provider is not what should drive the
> decision.

The idea is not to have necesarily one node per driver. I just wanted 
to keep
it simple.

Regards,
-Paul

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

[PATCH v4 5/8] irqchip: Add the ingenic-tcu-intc driver

[Paul Cercueil]

This simple driver handles the IRQ chip of the TCU
(Timer Counter Unit) of the JZ47xx Ingenic SoCs.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/irqchip/Kconfig           |   6 ++
 drivers/irqchip/Makefile          |   1 +
 drivers/irqchip/irq-ingenic-tcu.c | 161 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 168 insertions(+)
 create mode 100644 drivers/irqchip/irq-ingenic-tcu.c

 v2: - Use SPDX identifier for the license
     - Make KConfig option select CONFIG_IRQ_DOMAIN since we depend on it
 v3: - Move documentation to its own patch
     - Add comment explaining why we only use IRQCHIP_DECLARE
 v4: - Rename variables to avoid splitting long lines
     - Add comment about the multiple IRQ parents

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index d913aec85109..2b56587d04ed 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -267,6 +267,12 @@ config INGENIC_IRQ
 	depends on MACH_INGENIC
 	default y
 
+config INGENIC_TCU_IRQ
+	bool
+	depends on MACH_INGENIC || COMPILE_TEST
+	select IRQ_DOMAIN
+	default y
+
 config RENESAS_H8300H_INTC
         bool
 	select IRQ_DOMAIN
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index d27e3e3619e0..48b0bdf2b1a2 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -70,6 +70,7 @@ obj-$(CONFIG_RENESAS_H8300H_INTC)	+= irq-renesas-h8300h.o
 obj-$(CONFIG_RENESAS_H8S_INTC)		+= irq-renesas-h8s.o
 obj-$(CONFIG_ARCH_SA1100)		+= irq-sa11x0.o
 obj-$(CONFIG_INGENIC_IRQ)		+= irq-ingenic.o
+obj-$(CONFIG_INGENIC_TCU_IRQ)		+= irq-ingenic-tcu.o
 obj-$(CONFIG_IMX_GPCV2)			+= irq-imx-gpcv2.o
 obj-$(CONFIG_PIC32_EVIC)		+= irq-pic32-evic.o
 obj-$(CONFIG_MVEBU_GICP)		+= irq-mvebu-gicp.o
diff --git a/drivers/irqchip/irq-ingenic-tcu.c b/drivers/irqchip/irq-ingenic-tcu.c
new file mode 100644
index 000000000000..add3e9cc6f82
--- /dev/null
+++ b/drivers/irqchip/irq-ingenic-tcu.c
@@ -0,0 +1,161 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * JZ47xx SoCs TCU IRQ driver
+ * Copyright (C) 2018 Paul Cercueil <paul@crapouillou.net>
+ */
+
+#include <linux/bitops.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/regmap.h>
+
+#include <linux/mfd/syscon/ingenic-tcu.h>
+
+static void ingenic_tcu_intc_cascade(struct irq_desc *desc)
+{
+	struct irq_chip *irq_chip = irq_data_get_irq_chip(&desc->irq_data);
+	struct irq_domain *domain = irq_desc_get_handler_data(desc);
+	struct irq_chip_generic *gc = irq_get_domain_generic_chip(domain, 0);
+	struct regmap *map = gc->private;
+	uint32_t irq_reg, irq_mask;
+	unsigned int i;
+
+	regmap_read(map, TCU_REG_TFR, &irq_reg);
+	regmap_read(map, TCU_REG_TMR, &irq_mask);
+
+	chained_irq_enter(irq_chip, desc);
+
+	irq_reg &= ~irq_mask;
+
+	for (i = 0; i < 32; i++) {
+		if (irq_reg & BIT(i))
+			generic_handle_irq(irq_linear_revmap(domain, i));
+	}
+
+	chained_irq_exit(irq_chip, desc);
+}
+
+static void ingenic_tcu_gc_unmask_enable_reg(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct irq_chip_type *ct = irq_data_get_chip_type(d);
+	struct regmap *map = gc->private;
+	u32 mask = d->mask;
+
+	irq_gc_lock(gc);
+	regmap_write(map, ct->regs.ack, mask);
+	regmap_write(map, ct->regs.enable, mask);
+	*ct->mask_cache |= mask;
+	irq_gc_unlock(gc);
+}
+
+static void ingenic_tcu_gc_mask_disable_reg(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct irq_chip_type *ct = irq_data_get_chip_type(d);
+	struct regmap *map = gc->private;
+	u32 mask = d->mask;
+
+	irq_gc_lock(gc);
+	regmap_write(map, ct->regs.disable, mask);
+	*ct->mask_cache &= ~mask;
+	irq_gc_unlock(gc);
+}
+
+static void ingenic_tcu_gc_mask_disable_reg_and_ack(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct irq_chip_type *ct = irq_data_get_chip_type(d);
+	struct regmap *map = gc->private;
+	u32 mask = d->mask;
+
+	irq_gc_lock(gc);
+	regmap_write(map, ct->regs.ack, mask);
+	regmap_write(map, ct->regs.disable, mask);
+	irq_gc_unlock(gc);
+}
+
+static int __init ingenic_tcu_intc_of_init(struct device_node *node,
+	struct device_node *parent)
+{
+	struct irq_domain *domain;
+	struct irq_chip_generic *gc;
+	struct irq_chip_type *ct;
+	int err, i, irqs;
+	u32 parent_irqs[3];
+	struct regmap *map;
+
+	irqs = of_property_count_elems_of_size(node, "interrupts", sizeof(u32));
+	if (irqs < 0 || irqs > ARRAY_SIZE(parent_irqs))
+		return -EINVAL;
+
+	map = syscon_node_to_regmap(node->parent);
+	if (IS_ERR(map))
+		return PTR_ERR(map);
+
+	domain = irq_domain_add_linear(node, 32, &irq_generic_chip_ops, NULL);
+	if (!domain)
+		return -ENOMEM;
+
+	err = irq_alloc_domain_generic_chips(domain, 32, 1, "TCU",
+			handle_level_irq, 0, IRQ_NOPROBE | IRQ_LEVEL, 0);
+	if (err)
+		goto out_domain_remove;
+
+	gc = irq_get_domain_generic_chip(domain, 0);
+	ct = gc->chip_types;
+
+	gc->wake_enabled = IRQ_MSK(32);
+	gc->private = map;
+
+	ct->regs.disable = TCU_REG_TMSR;
+	ct->regs.enable = TCU_REG_TMCR;
+	ct->regs.ack = TCU_REG_TFCR;
+	ct->chip.irq_unmask = ingenic_tcu_gc_unmask_enable_reg;
+	ct->chip.irq_mask = ingenic_tcu_gc_mask_disable_reg;
+	ct->chip.irq_mask_ack = ingenic_tcu_gc_mask_disable_reg_and_ack;
+	ct->chip.flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE;
+
+	/* Mask all IRQs by default */
+	regmap_write(map, TCU_REG_TMSR, IRQ_MSK(32));
+
+	/* On JZ4740, timer 0 and timer 1 have their own interrupt line;
+	 * timers 2-7 share one interrupt.
+	 * On SoCs >= JZ4770, timer 5 has its own interrupt line;
+	 * timers 0-4 and 6-7 share one single interrupt.
+	 *
+	 * To keep things simple, we just register the same handler to
+	 * all parent interrupts. The handler will properly detect which
+	 * channel fired the interrupt.
+	 */
+	for (i = 0; i < irqs; i++) {
+		parent_irqs[i] = irq_of_parse_and_map(node, i);
+		if (!parent_irqs[i]) {
+			err = -EINVAL;
+			goto out_unmap_irqs;
+		}
+
+		irq_set_chained_handler_and_data(parent_irqs[i],
+				ingenic_tcu_intc_cascade, domain);
+	}
+
+	return 0;
+
+out_unmap_irqs:
+	for (; i > 0; i--)
+		irq_dispose_mapping(parent_irqs[i - 1]);
+out_domain_remove:
+	irq_domain_remove(domain);
+	return err;
+}
+
+/* We only probe via devicetree, no need for a platform driver */
+IRQCHIP_DECLARE(jz4740_tcu_intc, "ingenic,jz4740-tcu-intc",
+		ingenic_tcu_intc_of_init);
+IRQCHIP_DECLARE(jz4770_tcu_intc, "ingenic,jz4770-tcu-intc",
+		ingenic_tcu_intc_of_init);
+IRQCHIP_DECLARE(jz4780_tcu_intc, "ingenic,jz4780-tcu-intc",
+		ingenic_tcu_intc_of_init);
-- 
2.11.0

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

[PATCH v4 6/8] clk: ingenic: Add JZ47xx TCU clocks driver

[Paul Cercueil]

The TCU (Timer Counter Unit) of the Ingenic JZ47xx SoCs features 8
channels, each one having its own clock, that can be started and
stopped, reparented, and reclocked.

This driver only modifies the bits of the registers of the TCU that are
related to clocks control. It provides one clock per TCU channel (plus
one for the watchdog and one for the OS timer) that can be used by other
drivers.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Acked-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/clk/ingenic/Makefile |   2 +-
 drivers/clk/ingenic/tcu.c    | 319 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 320 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/ingenic/tcu.c

 v2: - Use SPDX identifier for the license
     - Fix broken build caused by typo when correcting patch
 v3: - Move documentation to its own patch
     - Get rid of ingenic_tcu structure. The "struct regmap *map" was
	   added to struct ingenic_tcu_clk, the other fields are gone.
	 - Replace clk_register / clk_register_clockdev /
	   of_clk_add_provider with their "clk_hw" variant.
 v4: No change

diff --git a/drivers/clk/ingenic/Makefile b/drivers/clk/ingenic/Makefile
index 1456e4cdb562..9dcadd4fed4c 100644
--- a/drivers/clk/ingenic/Makefile
+++ b/drivers/clk/ingenic/Makefile
@@ -1,4 +1,4 @@
-obj-y				+= cgu.o
+obj-y				+= cgu.o tcu.o
 obj-$(CONFIG_MACH_JZ4740)	+= jz4740-cgu.o
 obj-$(CONFIG_MACH_JZ4770)	+= jz4770-cgu.o
 obj-$(CONFIG_MACH_JZ4780)	+= jz4780-cgu.o
diff --git a/drivers/clk/ingenic/tcu.c b/drivers/clk/ingenic/tcu.c
new file mode 100644
index 000000000000..b550b6ac8fb4
--- /dev/null
+++ b/drivers/clk/ingenic/tcu.c
@@ -0,0 +1,319 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Ingenic JZ47xx SoC TCU clocks driver
+ * Copyright (C) 2018 Paul Cercueil <paul@crapouillou.net>
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mfd/syscon/ingenic-tcu.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/regmap.h>
+
+#include <dt-bindings/clock/ingenic,tcu.h>
+
+enum ingenic_version {
+	ID_JZ4740,
+	ID_JZ4770,
+	ID_JZ4780,
+};
+
+struct ingenic_tcu_clk_info {
+	struct clk_init_data init_data;
+	u8 gate_bit;
+	u8 tcsr_reg;
+};
+
+struct ingenic_tcu_clk {
+	struct clk_hw hw;
+
+	struct regmap *map;
+	const struct ingenic_tcu_clk_info *info;
+
+	unsigned int idx;
+};
+
+#define to_tcu_clk(_hw) container_of(_hw, struct ingenic_tcu_clk, hw)
+
+static int ingenic_tcu_enable(struct clk_hw *hw)
+{
+	struct ingenic_tcu_clk *tcu_clk = to_tcu_clk(hw);
+	const struct ingenic_tcu_clk_info *info = tcu_clk->info;
+
+	regmap_write(tcu_clk->map, TCU_REG_TSCR, BIT(info->gate_bit));
+	return 0;
+}
+
+static void ingenic_tcu_disable(struct clk_hw *hw)
+{
+	struct ingenic_tcu_clk *tcu_clk = to_tcu_clk(hw);
+	const struct ingenic_tcu_clk_info *info = tcu_clk->info;
+
+	regmap_write(tcu_clk->map, TCU_REG_TSSR, BIT(info->gate_bit));
+}
+
+static int ingenic_tcu_is_enabled(struct clk_hw *hw)
+{
+	struct ingenic_tcu_clk *tcu_clk = to_tcu_clk(hw);
+	const struct ingenic_tcu_clk_info *info = tcu_clk->info;
+	unsigned int value;
+
+	regmap_read(tcu_clk->map, TCU_REG_TSR, &value);
+
+	return !(value & BIT(info->gate_bit));
+}
+
+static u8 ingenic_tcu_get_parent(struct clk_hw *hw)
+{
+	struct ingenic_tcu_clk *tcu_clk = to_tcu_clk(hw);
+	const struct ingenic_tcu_clk_info *info = tcu_clk->info;
+	unsigned int val = 0;
+	int ret;
+
+	ret = regmap_read(tcu_clk->map, info->tcsr_reg, &val);
+	WARN_ONCE(ret < 0, "Unable to read TCSR %i", tcu_clk->idx);
+
+	return ffs(val & TCU_TCSR_PARENT_CLOCK_MASK) - 1;
+}
+
+static int ingenic_tcu_set_parent(struct clk_hw *hw, u8 idx)
+{
+	struct ingenic_tcu_clk *tcu_clk = to_tcu_clk(hw);
+	const struct ingenic_tcu_clk_info *info = tcu_clk->info;
+	struct regmap *map = tcu_clk->map;
+	int ret;
+
+	/*
+	 * Our clock provider has the CLK_SET_PARENT_GATE flag set, so we know
+	 * that the clk is in unprepared state. To be able to access TCSR
+	 * we must ungate the clock supply and we gate it again when done.
+	 */
+
+	regmap_write(map, TCU_REG_TSCR, BIT(info->gate_bit));
+
+	ret = regmap_update_bits(map, info->tcsr_reg,
+			TCU_TCSR_PARENT_CLOCK_MASK, BIT(idx));
+	WARN_ONCE(ret < 0, "Unable to update TCSR %i", tcu_clk->idx);
+
+	regmap_write(map, TCU_REG_TSSR, BIT(info->gate_bit));
+
+	return 0;
+}
+
+static unsigned long ingenic_tcu_recalc_rate(struct clk_hw *hw,
+		unsigned long parent_rate)
+{
+	struct ingenic_tcu_clk *tcu_clk = to_tcu_clk(hw);
+	const struct ingenic_tcu_clk_info *info = tcu_clk->info;
+	unsigned int prescale;
+	int ret;
+
+	ret = regmap_read(tcu_clk->map, info->tcsr_reg, &prescale);
+	WARN_ONCE(ret < 0, "Unable to read TCSR %i", tcu_clk->idx);
+
+	prescale = (prescale & TCU_TCSR_PRESCALE_MASK) >> TCU_TCSR_PRESCALE_LSB;
+
+	return parent_rate >> (prescale * 2);
+}
+
+static long ingenic_tcu_round_rate(struct clk_hw *hw, unsigned long req_rate,
+		unsigned long *parent_rate)
+{
+	unsigned long rate = *parent_rate;
+	unsigned int shift;
+
+	if (req_rate > rate)
+		return -EINVAL;
+
+	for (shift = 0; shift < 10; shift += 2)
+		if ((rate >> shift) <= req_rate)
+			break;
+
+	return rate >> shift;
+}
+
+static int ingenic_tcu_set_rate(struct clk_hw *hw, unsigned long req_rate,
+		unsigned long parent_rate)
+{
+	struct ingenic_tcu_clk *tcu_clk = to_tcu_clk(hw);
+	const struct ingenic_tcu_clk_info *info = tcu_clk->info;
+	struct regmap *map = tcu_clk->map;
+	u8 prescale = (ffs(parent_rate / req_rate) / 2)
+			<< TCU_TCSR_PRESCALE_LSB;
+	int ret;
+
+	/*
+	 * Our clock provider has the CLK_SET_RATE_GATE flag set, so we know
+	 * that the clk is in unprepared state. To be able to access TCSR
+	 * we must ungate the clock supply and we gate it again when done.
+	 */
+
+	regmap_write(map, TCU_REG_TSCR, BIT(info->gate_bit));
+
+	ret = regmap_update_bits(map, info->tcsr_reg,
+				TCU_TCSR_PRESCALE_MASK, prescale);
+	WARN_ONCE(ret < 0, "Unable to update TCSR %i", tcu_clk->idx);
+
+	regmap_write(map, TCU_REG_TSSR, BIT(info->gate_bit));
+
+	return 0;
+}
+
+static const struct clk_ops ingenic_tcu_clk_ops = {
+	.get_parent	= ingenic_tcu_get_parent,
+	.set_parent	= ingenic_tcu_set_parent,
+
+	.recalc_rate	= ingenic_tcu_recalc_rate,
+	.round_rate	= ingenic_tcu_round_rate,
+	.set_rate	= ingenic_tcu_set_rate,
+
+	.enable		= ingenic_tcu_enable,
+	.disable	= ingenic_tcu_disable,
+	.is_enabled	= ingenic_tcu_is_enabled,
+};
+
+static const char * const ingenic_tcu_timer_parents[] = {
+	"pclk",
+	"rtc",
+	"ext",
+};
+
+static const struct ingenic_tcu_clk_info ingenic_tcu_clk_info[] = {
+#define DEF_TIMER(_name, _gate_bit, _tcsr)				\
+	{								\
+		.init_data = {						\
+			.name = _name,					\
+			.parent_names = ingenic_tcu_timer_parents,	\
+			.num_parents = ARRAY_SIZE(ingenic_tcu_timer_parents),\
+			.ops = &ingenic_tcu_clk_ops,			\
+			.flags = CLK_SET_RATE_GATE | CLK_SET_PARENT_GATE,\
+		},							\
+		.gate_bit = _gate_bit,					\
+		.tcsr_reg = _tcsr,					\
+	}
+	[JZ4740_CLK_TIMER0] = DEF_TIMER("timer0", 0, TCU_REG_TCSRc(0)),
+	[JZ4740_CLK_TIMER1] = DEF_TIMER("timer1", 1, TCU_REG_TCSRc(1)),
+	[JZ4740_CLK_TIMER2] = DEF_TIMER("timer2", 2, TCU_REG_TCSRc(2)),
+	[JZ4740_CLK_TIMER3] = DEF_TIMER("timer3", 3, TCU_REG_TCSRc(3)),
+	[JZ4740_CLK_TIMER4] = DEF_TIMER("timer4", 4, TCU_REG_TCSRc(4)),
+	[JZ4740_CLK_TIMER5] = DEF_TIMER("timer5", 5, TCU_REG_TCSRc(5)),
+	[JZ4740_CLK_TIMER6] = DEF_TIMER("timer6", 6, TCU_REG_TCSRc(6)),
+	[JZ4740_CLK_TIMER7] = DEF_TIMER("timer7", 7, TCU_REG_TCSRc(7)),
+	[JZ4740_CLK_WDT]    = DEF_TIMER("wdt",   16, TCU_REG_WDT_TCSR),
+	[JZ4770_CLK_OST]    = DEF_TIMER("ost",   15, TCU_REG_OST_TCSR),
+#undef DEF_TIMER
+};
+
+static int ingenic_tcu_register_clock(struct regmap *map, unsigned int idx,
+		const struct ingenic_tcu_clk_info *info,
+		struct clk_hw_onecell_data *clocks)
+{
+	struct ingenic_tcu_clk *tcu_clk;
+	int err;
+
+	tcu_clk = kzalloc(sizeof(*tcu_clk), GFP_KERNEL);
+	if (!tcu_clk)
+		return -ENOMEM;
+
+	tcu_clk->hw.init = &info->init_data;
+	tcu_clk->idx = idx;
+	tcu_clk->info = info;
+	tcu_clk->map = map;
+
+	/* Set EXT as the default parent clock */
+	ingenic_tcu_set_parent(&tcu_clk->hw, 2);
+
+	ingenic_tcu_disable(&tcu_clk->hw);
+
+	err = clk_hw_register(NULL, &tcu_clk->hw);
+	if (err)
+		goto err_free_tcu_clk;
+
+	err = clk_hw_register_clkdev(&tcu_clk->hw, info->init_data.name, NULL);
+	if (err)
+		goto err_clk_unregister;
+
+	clocks->hws[idx] = &tcu_clk->hw;
+	return 0;
+
+err_clk_unregister:
+	clk_hw_unregister(&tcu_clk->hw);
+err_free_tcu_clk:
+	kfree(tcu_clk);
+	return err;
+}
+
+static void __init ingenic_tcu_init(struct device_node *np,
+		enum ingenic_version id)
+{
+	struct clk_hw_onecell_data *clocks;
+	struct regmap *map;
+	size_t i, nb_clks;
+	int ret = -ENOMEM;
+
+	if (id >= ID_JZ4770)
+		nb_clks = (JZ4770_CLK_LAST - JZ4740_CLK_TIMER0) + 1;
+	else
+		nb_clks = (JZ4740_CLK_LAST - JZ4740_CLK_TIMER0) + 1;
+
+	map = syscon_node_to_regmap(np->parent);
+	if (IS_ERR(map)) {
+		pr_err("%s: failed to map TCU registers\n", __func__);
+		return;
+	}
+
+	clocks = kzalloc(sizeof(*clocks) +
+					 sizeof(*clocks->hws) * nb_clks,
+					 GFP_KERNEL);
+	if (!clocks)
+		return;
+
+	clocks->num = nb_clks;
+
+	for (i = 0; i < nb_clks; i++) {
+		ret = ingenic_tcu_register_clock(map, i,
+				&ingenic_tcu_clk_info[JZ4740_CLK_TIMER0 + i],
+				clocks);
+		if (ret) {
+			pr_err("%s: cannot register clocks\n", __func__);
+			goto err_unregister;
+		}
+	}
+
+	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, clocks);
+	if (ret) {
+		pr_err("%s: cannot add OF clock provider\n", __func__);
+		goto err_unregister;
+	}
+
+	return;
+
+err_unregister:
+	for (i = 0; i < clocks->num; i++)
+		if (clocks->hws[i])
+			clk_hw_unregister(clocks->hws[i]);
+	kfree(clocks);
+}
+
+static void __init jz4740_tcu_init(struct device_node *np)
+{
+	ingenic_tcu_init(np, ID_JZ4740);
+}
+
+static void __init jz4770_tcu_init(struct device_node *np)
+{
+	ingenic_tcu_init(np, ID_JZ4770);
+}
+
+static void __init jz4780_tcu_init(struct device_node *np)
+{
+	ingenic_tcu_init(np, ID_JZ4780);
+}
+
+/* We only probe via devicetree, no need for a platform driver */
+CLK_OF_DECLARE(jz4740_tcu, "ingenic,jz4740-tcu-clocks", jz4740_tcu_init);
+CLK_OF_DECLARE(jz4770_tcu, "ingenic,jz4770-tcu-clocks", jz4770_tcu_init);
+CLK_OF_DECLARE(jz4780_tcu, "ingenic,jz4780-tcu-clocks", jz4780_tcu_init);
-- 
2.11.0

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

[PATCH v4 7/8] clocksource: Add a new timer-ingenic driver

[Paul Cercueil]

This driver will use the TCU (Timer Counter Unit) present on the Ingenic
JZ47xx SoCs to provide the kernel with a clocksource and timers.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/clocksource/Kconfig         |   8 ++
 drivers/clocksource/Makefile        |   1 +
 drivers/clocksource/timer-ingenic.c | 278 ++++++++++++++++++++++++++++++++++++
 3 files changed, 287 insertions(+)
 create mode 100644 drivers/clocksource/timer-ingenic.c

 v2: Use SPDX identifier for the license
 v3: - Move documentation to its own patch
     - Search the devicetree for PWM clients, and use all the TCU
	   channels that won't be used for PWM
 v4: - Add documentation about why we search for PWM clients
     - Verify that the PWM clients are for the TCU PWM driver

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index d2e5382821a4..481422145fb4 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -592,4 +592,12 @@ config CLKSRC_ST_LPC
 	  Enable this option to use the Low Power controller timer
 	  as clocksource.
 
+config INGENIC_TIMER
+	bool "Clocksource/timer using the TCU in Ingenic JZ SoCs"
+	depends on MACH_INGENIC || COMPILE_TEST
+	select CLKSRC_OF
+	default y
+	help
+	  Support for the timer/counter unit of the Ingenic JZ SoCs.
+
 endmenu
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index d6dec4489d66..98691e8999fe 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -74,5 +74,6 @@ obj-$(CONFIG_ASM9260_TIMER)		+= asm9260_timer.o
 obj-$(CONFIG_H8300_TMR8)		+= h8300_timer8.o
 obj-$(CONFIG_H8300_TMR16)		+= h8300_timer16.o
 obj-$(CONFIG_H8300_TPU)			+= h8300_tpu.o
+obj-$(CONFIG_INGENIC_TIMER)		+= timer-ingenic.o
 obj-$(CONFIG_CLKSRC_ST_LPC)		+= clksrc_st_lpc.o
 obj-$(CONFIG_X86_NUMACHIP)		+= numachip.o
diff --git a/drivers/clocksource/timer-ingenic.c b/drivers/clocksource/timer-ingenic.c
new file mode 100644
index 000000000000..8c777c0c0023
--- /dev/null
+++ b/drivers/clocksource/timer-ingenic.c
@@ -0,0 +1,278 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Ingenic JZ47xx SoC TCU clocksource driver
+ * Copyright (C) 2018 Paul Cercueil <paul@crapouillou.net>
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/clockchips.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mfd/syscon/ingenic-tcu.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#define NUM_CHANNELS	8
+
+struct ingenic_tcu;
+
+struct ingenic_tcu_channel {
+	unsigned int idx;
+	struct clk *clk;
+};
+
+struct ingenic_tcu {
+	struct ingenic_tcu_channel channels[NUM_CHANNELS];
+	unsigned long requested;
+	struct regmap *map;
+};
+
+struct ingenic_clock_event_device {
+	struct clock_event_device cevt;
+	struct ingenic_tcu_channel *channel;
+	char name[32];
+};
+
+#define ingenic_cevt(_evt) \
+	container_of(_evt, struct ingenic_clock_event_device, cevt)
+
+static inline struct ingenic_tcu *to_ingenic_tcu(struct ingenic_tcu_channel *ch)
+{
+	return container_of(ch, struct ingenic_tcu, channels[ch->idx]);
+}
+
+static int ingenic_tcu_cevt_set_state_shutdown(struct clock_event_device *evt)
+{
+	struct ingenic_clock_event_device *jzcevt = ingenic_cevt(evt);
+	struct ingenic_tcu_channel *channel = jzcevt->channel;
+	struct ingenic_tcu *tcu = to_ingenic_tcu(channel);
+	unsigned int idx = channel->idx;
+
+	regmap_write(tcu->map, TCU_REG_TECR, BIT(idx));
+	return 0;
+}
+
+static int ingenic_tcu_cevt_set_next(unsigned long next,
+		struct clock_event_device *evt)
+{
+	struct ingenic_clock_event_device *jzcevt = ingenic_cevt(evt);
+	struct ingenic_tcu_channel *channel = jzcevt->channel;
+	struct ingenic_tcu *tcu = to_ingenic_tcu(channel);
+	unsigned int idx = channel->idx;
+
+	if (next > 0xffff)
+		return -EINVAL;
+
+	regmap_write(tcu->map, TCU_REG_TDFRc(idx), (unsigned int) next);
+	regmap_write(tcu->map, TCU_REG_TCNTc(idx), 0);
+	regmap_write(tcu->map, TCU_REG_TESR, BIT(idx));
+
+	return 0;
+}
+
+static irqreturn_t ingenic_tcu_cevt_cb(int irq, void *dev_id)
+{
+	struct clock_event_device *cevt = dev_id;
+	struct ingenic_clock_event_device *jzcevt = ingenic_cevt(cevt);
+	struct ingenic_tcu_channel *channel = jzcevt->channel;
+	struct ingenic_tcu *tcu = to_ingenic_tcu(channel);
+	unsigned int idx = channel->idx;
+
+	regmap_write(tcu->map, TCU_REG_TECR, BIT(idx));
+
+	if (cevt->event_handler)
+		cevt->event_handler(cevt);
+
+	return IRQ_HANDLED;
+}
+
+static int __init ingenic_tcu_req_channel(struct ingenic_tcu_channel *channel)
+{
+	struct ingenic_tcu *tcu = to_ingenic_tcu(channel);
+	char buf[16];
+	int err;
+
+	if (test_and_set_bit(channel->idx, &tcu->requested))
+		return -EBUSY;
+
+	snprintf(buf, sizeof(buf), "timer%u", channel->idx);
+	channel->clk = clk_get(NULL, buf);
+	if (IS_ERR(channel->clk)) {
+		err = PTR_ERR(channel->clk);
+		goto out_release;
+	}
+
+	err = clk_prepare_enable(channel->clk);
+	if (err)
+		goto out_clk_put;
+
+	return 0;
+
+out_clk_put:
+	clk_put(channel->clk);
+out_release:
+	clear_bit(channel->idx, &tcu->requested);
+	return err;
+}
+
+static int __init ingenic_tcu_reset_channel(struct device_node *np,
+		struct ingenic_tcu_channel *channel)
+{
+	struct ingenic_tcu *tcu = to_ingenic_tcu(channel);
+
+	return regmap_update_bits(tcu->map, TCU_REG_TCSRc(channel->idx),
+				0xffff & ~TCU_TCSR_RESERVED_BITS, 0);
+}
+
+static void __init ingenic_tcu_free_channel(struct ingenic_tcu_channel *channel)
+{
+	struct ingenic_tcu *tcu = to_ingenic_tcu(channel);
+
+	clk_disable_unprepare(channel->clk);
+	clk_put(channel->clk);
+	clear_bit(channel->idx, &tcu->requested);
+}
+
+static const char * const ingenic_tcu_timer_names[] = {
+	"TCU0", "TCU1", "TCU2", "TCU3", "TCU4", "TCU5", "TCU6", "TCU7",
+};
+
+static int __init ingenic_tcu_setup_cevt(struct device_node *np,
+		struct ingenic_tcu *tcu, unsigned int idx)
+{
+	struct ingenic_tcu_channel *channel = &tcu->channels[idx];
+	struct ingenic_clock_event_device *jzcevt;
+	unsigned long rate;
+	int err, virq;
+
+	err = ingenic_tcu_req_channel(channel);
+	if (err)
+		return err;
+
+	err = ingenic_tcu_reset_channel(np, channel);
+	if (err)
+		goto err_out_free_channel;
+
+	rate = clk_get_rate(channel->clk);
+	if (!rate) {
+		err = -EINVAL;
+		goto err_out_free_channel;
+	}
+
+	jzcevt = kzalloc(sizeof(*jzcevt), GFP_KERNEL);
+	if (!jzcevt) {
+		err = -ENOMEM;
+		goto err_out_free_channel;
+	}
+
+	virq = irq_of_parse_and_map(np, idx);
+	if (!virq) {
+		err = -EINVAL;
+		goto err_out_kfree_jzcevt;
+	}
+
+	err = request_irq(virq, ingenic_tcu_cevt_cb, IRQF_TIMER,
+			ingenic_tcu_timer_names[idx], &jzcevt->cevt);
+	if (err)
+		goto err_out_irq_dispose_mapping;
+
+	jzcevt->channel = channel;
+	snprintf(jzcevt->name, sizeof(jzcevt->name), "ingenic-tcu-chan%u",
+		 channel->idx);
+
+	jzcevt->cevt.cpumask = cpumask_of(smp_processor_id());
+	jzcevt->cevt.features = CLOCK_EVT_FEAT_ONESHOT;
+	jzcevt->cevt.name = jzcevt->name;
+	jzcevt->cevt.rating = 200;
+	jzcevt->cevt.set_state_shutdown = ingenic_tcu_cevt_set_state_shutdown;
+	jzcevt->cevt.set_next_event = ingenic_tcu_cevt_set_next;
+
+	clockevents_config_and_register(&jzcevt->cevt, rate, 10, (1 << 16) - 1);
+
+	return 0;
+
+err_out_irq_dispose_mapping:
+	irq_dispose_mapping(virq);
+err_out_kfree_jzcevt:
+	kfree(jzcevt);
+err_out_free_channel:
+	ingenic_tcu_free_channel(channel);
+	return err;
+}
+
+static int __init ingenic_tcu_init(struct device_node *np)
+{
+	unsigned long available_channels = GENMASK(NUM_CHANNELS - 1, 0);
+	struct device_node *node, *pwm_driver_node;
+	struct ingenic_tcu *tcu;
+	unsigned int i, channel;
+	int err;
+	u32 val;
+
+	/* Parse the devicetree for clients of the TCU PWM driver;
+	 * every TCU channel not requested for PWM will be used as
+	 * a timer.
+	 */
+	for_each_node_with_property(node, "pwms") {
+		/* Get the PWM channel ID (field 1 of the "pwms" node) */
+		err = of_property_read_u32_index(node, "pwms", 1, &val);
+		if (!err && val >= NUM_CHANNELS)
+			err = -EINVAL;
+		if (err) {
+			pr_err("timer-ingenic: Unable to parse PWM nodes!");
+			break;
+		}
+
+		/* Get the PWM driver node (field 0 of the "pwms" node) */
+		pwm_driver_node = of_parse_phandle(node, "pwms", 0);
+		if (!pwm_driver_node) {
+			pr_err("timer-ingenic: Unable to find PWM driver node");
+			break;
+		}
+
+		/* Verify that the node we found is for the TCU PWM driver,
+		 * by checking that this driver and the PWM driver passed
+		 * as phandle share the same parent (the "ingenic,tcu"
+		 * compatible MFD/syscon node).
+		 */
+		if (pwm_driver_node->parent != np->parent)
+			continue;
+
+		pr_info("timer-ingenic: Reserving channel %u for PWM", val);
+		available_channels &= ~BIT(val);
+	}
+
+	tcu = kzalloc(sizeof(*tcu), GFP_KERNEL);
+	if (!tcu)
+		return -ENOMEM;
+
+	tcu->map = syscon_node_to_regmap(np->parent);
+	if (IS_ERR(tcu->map)) {
+		err = PTR_ERR(tcu->map);
+		kfree(tcu);
+		return err;
+	}
+
+	for (i = 0; i < NUM_CHANNELS; i++)
+		tcu->channels[i].idx = i;
+
+	for_each_set_bit(channel, &available_channels, NUM_CHANNELS) {
+		err = ingenic_tcu_setup_cevt(np, tcu, channel);
+		if (err) {
+			pr_warn("timer-ingenic: Unable to init TCU channel %u: %i",
+					channel, err);
+			continue;
+		}
+	}
+
+	return 0;
+}
+
+/* We only probe via devicetree, no need for a platform driver */
+CLOCKSOURCE_OF_DECLARE(jz4740_tcu, "ingenic,jz4740-tcu", ingenic_tcu_init);
+CLOCKSOURCE_OF_DECLARE(jz4770_tcu, "ingenic,jz4770-tcu", ingenic_tcu_init);
+CLOCKSOURCE_OF_DECLARE(jz4780_tcu, "ingenic,jz4780-tcu", ingenic_tcu_init);
-- 
2.11.0

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

Re: [PATCH v4 7/8] clocksource: Add a new timer-ingenic driver

[Daniel Lezcano]

On 18/03/2018 00:29, Paul Cercueil wrote:
> This driver will use the TCU (Timer Counter Unit) present on the Ingenic
> JZ47xx SoCs to provide the kernel with a clocksource and timers.

Please provide a more detailed description about the timer.

Where is the clocksource ?

I don't see the point of using channel idx and pwm checking here.

There is one clockevent, why create multiple channels ? Can't you stick
to the usual init routine for a timer.

> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/clocksource/Kconfig         |   8 ++
>  drivers/clocksource/Makefile        |   1 +
>  drivers/clocksource/timer-ingenic.c | 278 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 287 insertions(+)
>  create mode 100644 drivers/clocksource/timer-ingenic.c
> 
>  v2: Use SPDX identifier for the license
>  v3: - Move documentation to its own patch
>      - Search the devicetree for PWM clients, and use all the TCU
> 	   channels that won't be used for PWM
>  v4: - Add documentation about why we search for PWM clients
>      - Verify that the PWM clients are for the TCU PWM driver
> 
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index d2e5382821a4..481422145fb4 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -592,4 +592,12 @@ config CLKSRC_ST_LPC
>  	  Enable this option to use the Low Power controller timer
>  	  as clocksource.
>  
> +config INGENIC_TIMER
> +	bool "Clocksource/timer using the TCU in Ingenic JZ SoCs"
> +	depends on MACH_INGENIC || COMPILE_TEST

bool "Clocksource/timer using the TCU in Ingenic JZ SoCs" if COMPILE_TEST

Remove the depends MACH_INGENIC.

> +	select CLKSRC_OF
> +	default y

No default, Kconfig platform selects the timer.

> +	help
> +	  Support for the timer/counter unit of the Ingenic JZ SoCs.
> +
>  endmenu
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index d6dec4489d66..98691e8999fe 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -74,5 +74,6 @@ obj-$(CONFIG_ASM9260_TIMER)		+= asm9260_timer.o
>  obj-$(CONFIG_H8300_TMR8)		+= h8300_timer8.o
>  obj-$(CONFIG_H8300_TMR16)		+= h8300_timer16.o
>  obj-$(CONFIG_H8300_TPU)			+= h8300_tpu.o
> +obj-$(CONFIG_INGENIC_TIMER)		+= timer-ingenic.o
>  obj-$(CONFIG_CLKSRC_ST_LPC)		+= clksrc_st_lpc.o
>  obj-$(CONFIG_X86_NUMACHIP)		+= numachip.o
> diff --git a/drivers/clocksource/timer-ingenic.c b/drivers/clocksource/timer-ingenic.c
> new file mode 100644
> index 000000000000..8c777c0c0023
> --- /dev/null
> +++ b/drivers/clocksource/timer-ingenic.c
> @@ -0,0 +1,278 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Ingenic JZ47xx SoC TCU clocksource driver
> + * Copyright (C) 2018 Paul Cercueil <paul@crapouillou.net>
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/clockchips.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/mfd/syscon/ingenic-tcu.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#define NUM_CHANNELS	8
> +
> +struct ingenic_tcu;
> +
> +struct ingenic_tcu_channel {
> +	unsigned int idx;
> +	struct clk *clk;
> +};
> +
> +struct ingenic_tcu {
> +	struct ingenic_tcu_channel channels[NUM_CHANNELS];
> +	unsigned long requested;
> +	struct regmap *map;
> +};
> +
> +struct ingenic_clock_event_device {
> +	struct clock_event_device cevt;
> +	struct ingenic_tcu_channel *channel;
> +	char name[32];
> +};
> +
> +#define ingenic_cevt(_evt) \
> +	container_of(_evt, struct ingenic_clock_event_device, cevt)
> +
> +static inline struct ingenic_tcu *to_ingenic_tcu(struct ingenic_tcu_channel *ch)
> +{
> +	return container_of(ch, struct ingenic_tcu, channels[ch->idx]);
> +}
> +
> +static int ingenic_tcu_cevt_set_state_shutdown(struct clock_event_device *evt)
> +{
> +	struct ingenic_clock_event_device *jzcevt = ingenic_cevt(evt);
> +	struct ingenic_tcu_channel *channel = jzcevt->channel;
> +	struct ingenic_tcu *tcu = to_ingenic_tcu(channel);
> +	unsigned int idx = channel->idx;
> +
> +	regmap_write(tcu->map, TCU_REG_TECR, BIT(idx));
> +	return 0;
> +}
> +
> +static int ingenic_tcu_cevt_set_next(unsigned long next,
> +		struct clock_event_device *evt)
> +{
> +	struct ingenic_clock_event_device *jzcevt = ingenic_cevt(evt);
> +	struct ingenic_tcu_channel *channel = jzcevt->channel;
> +	struct ingenic_tcu *tcu = to_ingenic_tcu(channel);
> +	unsigned int idx = channel->idx;
> +
> +	if (next > 0xffff)
> +		return -EINVAL;
> +
> +	regmap_write(tcu->map, TCU_REG_TDFRc(idx), (unsigned int) next);
> +	regmap_write(tcu->map, TCU_REG_TCNTc(idx), 0);
> +	regmap_write(tcu->map, TCU_REG_TESR, BIT(idx));
> +
> +	return 0;
> +}
> +
> +static irqreturn_t ingenic_tcu_cevt_cb(int irq, void *dev_id)
> +{
> +	struct clock_event_device *cevt = dev_id;
> +	struct ingenic_clock_event_device *jzcevt = ingenic_cevt(cevt);
> +	struct ingenic_tcu_channel *channel = jzcevt->channel;
> +	struct ingenic_tcu *tcu = to_ingenic_tcu(channel);
> +	unsigned int idx = channel->idx;
> +
> +	regmap_write(tcu->map, TCU_REG_TECR, BIT(idx));
> +
> +	if (cevt->event_handler)
> +		cevt->event_handler(cevt);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int __init ingenic_tcu_req_channel(struct ingenic_tcu_channel *channel)
> +{
> +	struct ingenic_tcu *tcu = to_ingenic_tcu(channel);
> +	char buf[16];
> +	int err;
> +
> +	if (test_and_set_bit(channel->idx, &tcu->requested))
> +		return -EBUSY;
> +
> +	snprintf(buf, sizeof(buf), "timer%u", channel->idx);
> +	channel->clk = clk_get(NULL, buf);
> +	if (IS_ERR(channel->clk)) {
> +		err = PTR_ERR(channel->clk);
> +		goto out_release;
> +	}
> +
> +	err = clk_prepare_enable(channel->clk);
> +	if (err)
> +		goto out_clk_put;
> +
> +	return 0;
> +
> +out_clk_put:
> +	clk_put(channel->clk);
> +out_release:
> +	clear_bit(channel->idx, &tcu->requested);
> +	return err;
> +}
> +
> +static int __init ingenic_tcu_reset_channel(struct device_node *np,
> +		struct ingenic_tcu_channel *channel)
> +{
> +	struct ingenic_tcu *tcu = to_ingenic_tcu(channel);
> +
> +	return regmap_update_bits(tcu->map, TCU_REG_TCSRc(channel->idx),
> +				0xffff & ~TCU_TCSR_RESERVED_BITS, 0);
> +}
> +
> +static void __init ingenic_tcu_free_channel(struct ingenic_tcu_channel *channel)
> +{
> +	struct ingenic_tcu *tcu = to_ingenic_tcu(channel);
> +
> +	clk_disable_unprepare(channel->clk);
> +	clk_put(channel->clk);
> +	clear_bit(channel->idx, &tcu->requested);
> +}
> +
> +static const char * const ingenic_tcu_timer_names[] = {
> +	"TCU0", "TCU1", "TCU2", "TCU3", "TCU4", "TCU5", "TCU6", "TCU7",
> +};
> +
> +static int __init ingenic_tcu_setup_cevt(struct device_node *np,
> +		struct ingenic_tcu *tcu, unsigned int idx)
> +{
> +	struct ingenic_tcu_channel *channel = &tcu->channels[idx];
> +	struct ingenic_clock_event_device *jzcevt;
> +	unsigned long rate;
> +	int err, virq;
> +
> +	err = ingenic_tcu_req_channel(channel);
> +	if (err)
> +		return err;
> +
> +	err = ingenic_tcu_reset_channel(np, channel);
> +	if (err)
> +		goto err_out_free_channel;
> +
> +	rate = clk_get_rate(channel->clk);
> +	if (!rate) {
> +		err = -EINVAL;
> +		goto err_out_free_channel;
> +	}
> +
> +	jzcevt = kzalloc(sizeof(*jzcevt), GFP_KERNEL);
> +	if (!jzcevt) {
> +		err = -ENOMEM;
> +		goto err_out_free_channel;
> +	}
> +
> +	virq = irq_of_parse_and_map(np, idx);
> +	if (!virq) {
> +		err = -EINVAL;
> +		goto err_out_kfree_jzcevt;
> +	}
> +
> +	err = request_irq(virq, ingenic_tcu_cevt_cb, IRQF_TIMER,
> +			ingenic_tcu_timer_names[idx], &jzcevt->cevt);
> +	if (err)
> +		goto err_out_irq_dispose_mapping;
> +
> +	jzcevt->channel = channel;
> +	snprintf(jzcevt->name, sizeof(jzcevt->name), "ingenic-tcu-chan%u",
> +		 channel->idx);
> +
> +	jzcevt->cevt.cpumask = cpumask_of(smp_processor_id());
> +	jzcevt->cevt.features = CLOCK_EVT_FEAT_ONESHOT;
> +	jzcevt->cevt.name = jzcevt->name;
> +	jzcevt->cevt.rating = 200;
> +	jzcevt->cevt.set_state_shutdown = ingenic_tcu_cevt_set_state_shutdown;
> +	jzcevt->cevt.set_next_event = ingenic_tcu_cevt_set_next;
> +
> +	clockevents_config_and_register(&jzcevt->cevt, rate, 10, (1 << 16) - 1);
> +
> +	return 0;
> +
> +err_out_irq_dispose_mapping:
> +	irq_dispose_mapping(virq);
> +err_out_kfree_jzcevt:
> +	kfree(jzcevt);
> +err_out_free_channel:
> +	ingenic_tcu_free_channel(channel);
> +	return err;
> +}
> +
> +static int __init ingenic_tcu_init(struct device_node *np)
> +{
> +	unsigned long available_channels = GENMASK(NUM_CHANNELS - 1, 0);
> +	struct device_node *node, *pwm_driver_node;
> +	struct ingenic_tcu *tcu;
> +	unsigned int i, channel;
> +	int err;
> +	u32 val;
> +
> +	/* Parse the devicetree for clients of the TCU PWM driver;
> +	 * every TCU channel not requested for PWM will be used as
> +	 * a timer.
> +	 */



> +	for_each_node_with_property(node, "pwms") {
> +		/* Get the PWM channel ID (field 1 of the "pwms" node) */
> +		err = of_property_read_u32_index(node, "pwms", 1, &val);
> +		if (!err && val >= NUM_CHANNELS)
> +			err = -EINVAL;
> +		if (err) {
> +			pr_err("timer-ingenic: Unable to parse PWM nodes!");
> +			break;
> +		}
> +
> +		/* Get the PWM driver node (field 0 of the "pwms" node) */
> +		pwm_driver_node = of_parse_phandle(node, "pwms", 0);
> +		if (!pwm_driver_node) {
> +			pr_err("timer-ingenic: Unable to find PWM driver node");
> +			break;
> +		}
> +
> +		/* Verify that the node we found is for the TCU PWM driver,
> +		 * by checking that this driver and the PWM driver passed
> +		 * as phandle share the same parent (the "ingenic,tcu"
> +		 * compatible MFD/syscon node).
> +		 */
> +		if (pwm_driver_node->parent != np->parent)
> +			continue;
> +
> +		pr_info("timer-ingenic: Reserving channel %u for PWM", val);
> +		available_channels &= ~BIT(val);
> +	}
> +
> +	tcu = kzalloc(sizeof(*tcu), GFP_KERNEL);
> +	if (!tcu)
> +		return -ENOMEM;
> +
> +	tcu->map = syscon_node_to_regmap(np->parent);
> +	if (IS_ERR(tcu->map)) {
> +		err = PTR_ERR(tcu->map);
> +		kfree(tcu);
> +		return err;
> +	}
> +
> +	for (i = 0; i < NUM_CHANNELS; i++)
> +		tcu->channels[i].idx = i;

I'm pretty sure you can do better thaningenic_tcu_setup that :)

> +	for_each_set_bit(channel, &available_channels, NUM_CHANNELS) {
> +		err = _cevt(np, tcu, channel);
> +		if (err) {
> +			pr_warn("timer-ingenic: Unable to init TCU channel %u: %i",
> +					channel, err);
> +			continue;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/* We only probe via devicetree, no need for a platform driver */
> +CLOCKSOURCE_OF_DECLARE(jz4740_tcu, "ingenic,jz4740-tcu", ingenic_tcu_init);
> +CLOCKSOURCE_OF_DECLARE(jz4770_tcu, "ingenic,jz4770-tcu", ingenic_tcu_init);
> +CLOCKSOURCE_OF_DECLARE(jz4780_tcu, "ingenic,jz4780-tcu", ingenic_tcu_init);

s/CLOCKSOURCE_OF_DECLARE/TIMER_OF_DECLARE/

> 


-- 
 <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 linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 7/8] clocksource: Add a new timer-ingenic driver,

[Paul Cercueil]

Le 2018-03-24 07:26, Daniel Lezcano a écrit :
> On 18/03/2018 00:29, Paul Cercueil wrote:
>> This driver will use the TCU (Timer Counter Unit) present on the 
>> Ingenic
>> JZ47xx SoCs to provide the kernel with a clocksource and timers.
> 
> Please provide a more detailed description about the timer.

There's a doc file for that :)

> Where is the clocksource ?

Right, there is no clocksource, just timers.

> I don't see the point of using channel idx and pwm checking here.
> 
> There is one clockevent, why create multiple channels ? Can't you stick
> to the usual init routine for a timer.

So the idea is that we use all the TCU channels that won't be used for 
PWM
as timers. Hence the PWM checking. Why is this bad?

>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>> ---
>>  drivers/clocksource/Kconfig         |   8 ++
>>  drivers/clocksource/Makefile        |   1 +
>>  drivers/clocksource/timer-ingenic.c | 278 
>> ++++++++++++++++++++++++++++++++++++
>>  3 files changed, 287 insertions(+)
>>  create mode 100644 drivers/clocksource/timer-ingenic.c
>> 
>>  v2: Use SPDX identifier for the license
>>  v3: - Move documentation to its own patch
>>      - Search the devicetree for PWM clients, and use all the TCU
>> 	   channels that won't be used for PWM
>>  v4: - Add documentation about why we search for PWM clients
>>      - Verify that the PWM clients are for the TCU PWM driver
>> 
>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>> index d2e5382821a4..481422145fb4 100644
>> --- a/drivers/clocksource/Kconfig
>> +++ b/drivers/clocksource/Kconfig
>> @@ -592,4 +592,12 @@ config CLKSRC_ST_LPC
>>  	  Enable this option to use the Low Power controller timer
>>  	  as clocksource.
>> 
>> +config INGENIC_TIMER
>> +	bool "Clocksource/timer using the TCU in Ingenic JZ SoCs"
>> +	depends on MACH_INGENIC || COMPILE_TEST
> 
> bool "Clocksource/timer using the TCU in Ingenic JZ SoCs" if 
> COMPILE_TEST
> 
> Remove the depends MACH_INGENIC.

This driver is not useful on anything else than Ingenic SoCs, why should 
I
remove MACH_INGENIC then?

>> +	select CLKSRC_OF
>> +	default y
> 
> No default, Kconfig platform selects the timer.

Alright.

>> +	help
>> +	  Support for the timer/counter unit of the Ingenic JZ SoCs.
>> +
>>  endmenu
>> diff --git a/drivers/clocksource/Makefile 
>> b/drivers/clocksource/Makefile
>> index d6dec4489d66..98691e8999fe 100644
>> --- a/drivers/clocksource/Makefile
>> +++ b/drivers/clocksource/Makefile
>> @@ -74,5 +74,6 @@ obj-$(CONFIG_ASM9260_TIMER)		+= asm9260_timer.o
>>  obj-$(CONFIG_H8300_TMR8)		+= h8300_timer8.o
>>  obj-$(CONFIG_H8300_TMR16)		+= h8300_timer16.o
>>  obj-$(CONFIG_H8300_TPU)			+= h8300_tpu.o
>> +obj-$(CONFIG_INGENIC_TIMER)		+= timer-ingenic.o
>>  obj-$(CONFIG_CLKSRC_ST_LPC)		+= clksrc_st_lpc.o
>>  obj-$(CONFIG_X86_NUMACHIP)		+= numachip.o
>> diff --git a/drivers/clocksource/timer-ingenic.c 
>> b/drivers/clocksource/timer-ingenic.c
>> new file mode 100644
>> index 000000000000..8c777c0c0023
>> --- /dev/null
>> +++ b/drivers/clocksource/timer-ingenic.c
>> @@ -0,0 +1,278 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Ingenic JZ47xx SoC TCU clocksource driver
>> + * Copyright (C) 2018 Paul Cercueil <paul@crapouillou.net>
>> + */
>> +
>> +#include <linux/bitops.h>
>> +#include <linux/clk.h>
>> +#include <linux/clockchips.h>
>> +#include <linux/err.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/mfd/syscon/ingenic-tcu.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +
>> +#define NUM_CHANNELS	8
>> +
>> +struct ingenic_tcu;
>> +
>> +struct ingenic_tcu_channel {
>> +	unsigned int idx;
>> +	struct clk *clk;
>> +};
>> +
>> +struct ingenic_tcu {
>> +	struct ingenic_tcu_channel channels[NUM_CHANNELS];
>> +	unsigned long requested;
>> +	struct regmap *map;
>> +};
>> +
>> +struct ingenic_clock_event_device {
>> +	struct clock_event_device cevt;
>> +	struct ingenic_tcu_channel *channel;
>> +	char name[32];
>> +};
>> +
>> +#define ingenic_cevt(_evt) \
>> +	container_of(_evt, struct ingenic_clock_event_device, cevt)
>> +
>> +static inline struct ingenic_tcu *to_ingenic_tcu(struct 
>> ingenic_tcu_channel *ch)
>> +{
>> +	return container_of(ch, struct ingenic_tcu, channels[ch->idx]);
>> +}
>> +
>> +static int ingenic_tcu_cevt_set_state_shutdown(struct 
>> clock_event_device *evt)
>> +{
>> +	struct ingenic_clock_event_device *jzcevt = ingenic_cevt(evt);
>> +	struct ingenic_tcu_channel *channel = jzcevt->channel;
>> +	struct ingenic_tcu *tcu = to_ingenic_tcu(channel);
>> +	unsigned int idx = channel->idx;
>> +
>> +	regmap_write(tcu->map, TCU_REG_TECR, BIT(idx));
>> +	return 0;
>> +}
>> +
>> +static int ingenic_tcu_cevt_set_next(unsigned long next,
>> +		struct clock_event_device *evt)
>> +{
>> +	struct ingenic_clock_event_device *jzcevt = ingenic_cevt(evt);
>> +	struct ingenic_tcu_channel *channel = jzcevt->channel;
>> +	struct ingenic_tcu *tcu = to_ingenic_tcu(channel);
>> +	unsigned int idx = channel->idx;
>> +
>> +	if (next > 0xffff)
>> +		return -EINVAL;
>> +
>> +	regmap_write(tcu->map, TCU_REG_TDFRc(idx), (unsigned int) next);
>> +	regmap_write(tcu->map, TCU_REG_TCNTc(idx), 0);
>> +	regmap_write(tcu->map, TCU_REG_TESR, BIT(idx));
>> +
>> +	return 0;
>> +}
>> +
>> +static irqreturn_t ingenic_tcu_cevt_cb(int irq, void *dev_id)
>> +{
>> +	struct clock_event_device *cevt = dev_id;
>> +	struct ingenic_clock_event_device *jzcevt = ingenic_cevt(cevt);
>> +	struct ingenic_tcu_channel *channel = jzcevt->channel;
>> +	struct ingenic_tcu *tcu = to_ingenic_tcu(channel);
>> +	unsigned int idx = channel->idx;
>> +
>> +	regmap_write(tcu->map, TCU_REG_TECR, BIT(idx));
>> +
>> +	if (cevt->event_handler)
>> +		cevt->event_handler(cevt);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int __init ingenic_tcu_req_channel(struct ingenic_tcu_channel 
>> *channel)
>> +{
>> +	struct ingenic_tcu *tcu = to_ingenic_tcu(channel);
>> +	char buf[16];
>> +	int err;
>> +
>> +	if (test_and_set_bit(channel->idx, &tcu->requested))
>> +		return -EBUSY;
>> +
>> +	snprintf(buf, sizeof(buf), "timer%u", channel->idx);
>> +	channel->clk = clk_get(NULL, buf);
>> +	if (IS_ERR(channel->clk)) {
>> +		err = PTR_ERR(channel->clk);
>> +		goto out_release;
>> +	}
>> +
>> +	err = clk_prepare_enable(channel->clk);
>> +	if (err)
>> +		goto out_clk_put;
>> +
>> +	return 0;
>> +
>> +out_clk_put:
>> +	clk_put(channel->clk);
>> +out_release:
>> +	clear_bit(channel->idx, &tcu->requested);
>> +	return err;
>> +}
>> +
>> +static int __init ingenic_tcu_reset_channel(struct device_node *np,
>> +		struct ingenic_tcu_channel *channel)
>> +{
>> +	struct ingenic_tcu *tcu = to_ingenic_tcu(channel);
>> +
>> +	return regmap_update_bits(tcu->map, TCU_REG_TCSRc(channel->idx),
>> +				0xffff & ~TCU_TCSR_RESERVED_BITS, 0);
>> +}
>> +
>> +static void __init ingenic_tcu_free_channel(struct 
>> ingenic_tcu_channel *channel)
>> +{
>> +	struct ingenic_tcu *tcu = to_ingenic_tcu(channel);
>> +
>> +	clk_disable_unprepare(channel->clk);
>> +	clk_put(channel->clk);
>> +	clear_bit(channel->idx, &tcu->requested);
>> +}
>> +
>> +static const char * const ingenic_tcu_timer_names[] = {
>> +	"TCU0", "TCU1", "TCU2", "TCU3", "TCU4", "TCU5", "TCU6", "TCU7",
>> +};
>> +
>> +static int __init ingenic_tcu_setup_cevt(struct device_node *np,
>> +		struct ingenic_tcu *tcu, unsigned int idx)
>> +{
>> +	struct ingenic_tcu_channel *channel = &tcu->channels[idx];
>> +	struct ingenic_clock_event_device *jzcevt;
>> +	unsigned long rate;
>> +	int err, virq;
>> +
>> +	err = ingenic_tcu_req_channel(channel);
>> +	if (err)
>> +		return err;
>> +
>> +	err = ingenic_tcu_reset_channel(np, channel);
>> +	if (err)
>> +		goto err_out_free_channel;
>> +
>> +	rate = clk_get_rate(channel->clk);
>> +	if (!rate) {
>> +		err = -EINVAL;
>> +		goto err_out_free_channel;
>> +	}
>> +
>> +	jzcevt = kzalloc(sizeof(*jzcevt), GFP_KERNEL);
>> +	if (!jzcevt) {
>> +		err = -ENOMEM;
>> +		goto err_out_free_channel;
>> +	}
>> +
>> +	virq = irq_of_parse_and_map(np, idx);
>> +	if (!virq) {
>> +		err = -EINVAL;
>> +		goto err_out_kfree_jzcevt;
>> +	}
>> +
>> +	err = request_irq(virq, ingenic_tcu_cevt_cb, IRQF_TIMER,
>> +			ingenic_tcu_timer_names[idx], &jzcevt->cevt);
>> +	if (err)
>> +		goto err_out_irq_dispose_mapping;
>> +
>> +	jzcevt->channel = channel;
>> +	snprintf(jzcevt->name, sizeof(jzcevt->name), "ingenic-tcu-chan%u",
>> +		 channel->idx);
>> +
>> +	jzcevt->cevt.cpumask = cpumask_of(smp_processor_id());
>> +	jzcevt->cevt.features = CLOCK_EVT_FEAT_ONESHOT;
>> +	jzcevt->cevt.name = jzcevt->name;
>> +	jzcevt->cevt.rating = 200;
>> +	jzcevt->cevt.set_state_shutdown = 
>> ingenic_tcu_cevt_set_state_shutdown;
>> +	jzcevt->cevt.set_next_event = ingenic_tcu_cevt_set_next;
>> +
>> +	clockevents_config_and_register(&jzcevt->cevt, rate, 10, (1 << 16) - 
>> 1);
>> +
>> +	return 0;
>> +
>> +err_out_irq_dispose_mapping:
>> +	irq_dispose_mapping(virq);
>> +err_out_kfree_jzcevt:
>> +	kfree(jzcevt);
>> +err_out_free_channel:
>> +	ingenic_tcu_free_channel(channel);
>> +	return err;
>> +}
>> +
>> +static int __init ingenic_tcu_init(struct device_node *np)
>> +{
>> +	unsigned long available_channels = GENMASK(NUM_CHANNELS - 1, 0);
>> +	struct device_node *node, *pwm_driver_node;
>> +	struct ingenic_tcu *tcu;
>> +	unsigned int i, channel;
>> +	int err;
>> +	u32 val;
>> +
>> +	/* Parse the devicetree for clients of the TCU PWM driver;
>> +	 * every TCU channel not requested for PWM will be used as
>> +	 * a timer.
>> +	 */
> 
> 
> 
>> +	for_each_node_with_property(node, "pwms") {
>> +		/* Get the PWM channel ID (field 1 of the "pwms" node) */
>> +		err = of_property_read_u32_index(node, "pwms", 1, &val);
>> +		if (!err && val >= NUM_CHANNELS)
>> +			err = -EINVAL;
>> +		if (err) {
>> +			pr_err("timer-ingenic: Unable to parse PWM nodes!");
>> +			break;
>> +		}
>> +
>> +		/* Get the PWM driver node (field 0 of the "pwms" node) */
>> +		pwm_driver_node = of_parse_phandle(node, "pwms", 0);
>> +		if (!pwm_driver_node) {
>> +			pr_err("timer-ingenic: Unable to find PWM driver node");
>> +			break;
>> +		}
>> +
>> +		/* Verify that the node we found is for the TCU PWM driver,
>> +		 * by checking that this driver and the PWM driver passed
>> +		 * as phandle share the same parent (the "ingenic,tcu"
>> +		 * compatible MFD/syscon node).
>> +		 */
>> +		if (pwm_driver_node->parent != np->parent)
>> +			continue;
>> +
>> +		pr_info("timer-ingenic: Reserving channel %u for PWM", val);
>> +		available_channels &= ~BIT(val);
>> +	}
>> +
>> +	tcu = kzalloc(sizeof(*tcu), GFP_KERNEL);
>> +	if (!tcu)
>> +		return -ENOMEM;
>> +
>> +	tcu->map = syscon_node_to_regmap(np->parent);
>> +	if (IS_ERR(tcu->map)) {
>> +		err = PTR_ERR(tcu->map);
>> +		kfree(tcu);
>> +		return err;
>> +	}
>> +
>> +	for (i = 0; i < NUM_CHANNELS; i++)
>> +		tcu->channels[i].idx = i;
> 
> I'm pretty sure you can do better thaningenic_tcu_setup that :)

I didn't think this would be a problem. I guess I can try.

>> +	for_each_set_bit(channel, &available_channels, NUM_CHANNELS) {
>> +		err = _cevt(np, tcu, channel);
>> +		if (err) {
>> +			pr_warn("timer-ingenic: Unable to init TCU channel %u: %i",
>> +					channel, err);
>> +			continue;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/* We only probe via devicetree, no need for a platform driver */
>> +CLOCKSOURCE_OF_DECLARE(jz4740_tcu, "ingenic,jz4740-tcu", 
>> ingenic_tcu_init);
>> +CLOCKSOURCE_OF_DECLARE(jz4770_tcu, "ingenic,jz4770-tcu", 
>> ingenic_tcu_init);
>> +CLOCKSOURCE_OF_DECLARE(jz4780_tcu, "ingenic,jz4780-tcu", 
>> ingenic_tcu_init);
> 
> s/CLOCKSOURCE_OF_DECLARE/TIMER_OF_DECLARE/

Sure, OK.

Thanks!
-Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 7/8] clocksource: Add a new timer-ingenic driver

[Daniel Lezcano]

On 28/03/2018 17:15, Paul Cercueil wrote:
> Le 2018-03-24 07:26, Daniel Lezcano a écrit :
>> On 18/03/2018 00:29, Paul Cercueil wrote:
>>> This driver will use the TCU (Timer Counter Unit) present on the Ingenic
>>> JZ47xx SoCs to provide the kernel with a clocksource and timers.
>>
>> Please provide a more detailed description about the timer.
> 
> There's a doc file for that :)

Usually, when there is a new driver I ask for a description in the
changelog for reference.

>> Where is the clocksource ?
> 
> Right, there is no clocksource, just timers.
> 
>> I don't see the point of using channel idx and pwm checking here.
>>
>> There is one clockevent, why create multiple channels ? Can't you stick
>> to the usual init routine for a timer.
> 
> So the idea is that we use all the TCU channels that won't be used for PWM
> as timers. Hence the PWM checking. Why is this bad?

It is not bad but arguable. By checking the channels used by the pwm in
the code, you introduce an adherence between two subsystems even if it
is just related to the DT parsing part.

As it is not needed to have more than one timer in the time framework
(at least with the same characteristics), the pwm channels check is
pointless. We can assume the author of the DT file is smart enough to
prevent conflicts and define a pwm and a timer properly instead of
adding more code complexity.

In addition, simplifying the code will allow you to use the timer-of
code and reduce very significantly the init function.

>>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>> ---
>>>  drivers/clocksource/Kconfig         |   8 ++
>>>  drivers/clocksource/Makefile        |   1 +
>>>  drivers/clocksource/timer-ingenic.c | 278
>>> ++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 287 insertions(+)
>>>  create mode 100644 drivers/clocksource/timer-ingenic.c
>>>
>>>  v2: Use SPDX identifier for the license
>>>  v3: - Move documentation to its own patch
>>>      - Search the devicetree for PWM clients, and use all the TCU
>>>        channels that won't be used for PWM
>>>  v4: - Add documentation about why we search for PWM clients
>>>      - Verify that the PWM clients are for the TCU PWM driver
>>>
>>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>>> index d2e5382821a4..481422145fb4 100644
>>> --- a/drivers/clocksource/Kconfig
>>> +++ b/drivers/clocksource/Kconfig
>>> @@ -592,4 +592,12 @@ config CLKSRC_ST_LPC
>>>        Enable this option to use the Low Power controller timer
>>>        as clocksource.
>>>
>>> +config INGENIC_TIMER
>>> +    bool "Clocksource/timer using the TCU in Ingenic JZ SoCs"
>>> +    depends on MACH_INGENIC || COMPILE_TEST
>>
>> bool "Clocksource/timer using the TCU in Ingenic JZ SoCs" if COMPILE_TEST
>>
>> Remove the depends MACH_INGENIC.
> 
> This driver is not useful on anything else than Ingenic SoCs, why should I
> remove MACH_INGENIC then?

For COMPILE_TEST on x86.

>>> +    select CLKSRC_OF
>>> +    default y
>>
>> No default, Kconfig platform selects the timer.
> 
> Alright.
[ ... ]

-- 
 <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 linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 7/8] clocksource: Add a new timer-ingenic driver

[Paul Cercueil]

Le mer. 28 mars 2018 à 18:25, Daniel Lezcano 
<daniel.lezcano@linaro.org> a écrit :
> On 28/03/2018 17:15, Paul Cercueil wrote:
>>  Le 2018-03-24 07:26, Daniel Lezcano a écrit :
>>>  On 18/03/2018 00:29, Paul Cercueil wrote:
>>>>  This driver will use the TCU (Timer Counter Unit) present on the 
>>>> Ingenic
>>>>  JZ47xx SoCs to provide the kernel with a clocksource and timers.
>>> 
>>>  Please provide a more detailed description about the timer.
>> 
>>  There's a doc file for that :)
> 
> Usually, when there is a new driver I ask for a description in the
> changelog for reference.
> 
>>>  Where is the clocksource ?
>> 
>>  Right, there is no clocksource, just timers.
>> 
>>>  I don't see the point of using channel idx and pwm checking here.
>>> 
>>>  There is one clockevent, why create multiple channels ? Can't you 
>>> stick
>>>  to the usual init routine for a timer.
>> 
>>  So the idea is that we use all the TCU channels that won't be used 
>> for PWM
>>  as timers. Hence the PWM checking. Why is this bad?
> 
> It is not bad but arguable. By checking the channels used by the pwm 
> in
> the code, you introduce an adherence between two subsystems even if it
> is just related to the DT parsing part.
> 
> As it is not needed to have more than one timer in the time framework
> (at least with the same characteristics), the pwm channels check is
> pointless. We can assume the author of the DT file is smart enough to
> prevent conflicts and define a pwm and a timer properly instead of
> adding more code complexity.
> 
> In addition, simplifying the code will allow you to use the timer-of
> code and reduce very significantly the init function.

That's what I had in my V1 and V2, my DT node for the timer-ingenic 
driver
had a "timers" property (e.g. "timers = <4 5>;") to select the channels 
that
should be used as timers. Then Rob told me I shouldn't do that, and 
instead
detect the channels that will be used for PWM.

>>>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>>>  ---
>>>>   drivers/clocksource/Kconfig         |   8 ++
>>>>   drivers/clocksource/Makefile        |   1 +
>>>>   drivers/clocksource/timer-ingenic.c | 278
>>>>  ++++++++++++++++++++++++++++++++++++
>>>>   3 files changed, 287 insertions(+)
>>>>   create mode 100644 drivers/clocksource/timer-ingenic.c
>>>> 
>>>>   v2: Use SPDX identifier for the license
>>>>   v3: - Move documentation to its own patch
>>>>       - Search the devicetree for PWM clients, and use all the TCU
>>>>         channels that won't be used for PWM
>>>>   v4: - Add documentation about why we search for PWM clients
>>>>       - Verify that the PWM clients are for the TCU PWM driver
>>>> 
>>>>  diff --git a/drivers/clocksource/Kconfig 
>>>> b/drivers/clocksource/Kconfig
>>>>  index d2e5382821a4..481422145fb4 100644
>>>>  --- a/drivers/clocksource/Kconfig
>>>>  +++ b/drivers/clocksource/Kconfig
>>>>  @@ -592,4 +592,12 @@ config CLKSRC_ST_LPC
>>>>         Enable this option to use the Low Power controller timer
>>>>         as clocksource.
>>>> 
>>>>  +config INGENIC_TIMER
>>>>  +    bool "Clocksource/timer using the TCU in Ingenic JZ SoCs"
>>>>  +    depends on MACH_INGENIC || COMPILE_TEST
>>> 
>>>  bool "Clocksource/timer using the TCU in Ingenic JZ SoCs" if 
>>> COMPILE_TEST
>>> 
>>>  Remove the depends MACH_INGENIC.
>> 
>>  This driver is not useful on anything else than Ingenic SoCs, why 
>> should I
>>  remove MACH_INGENIC then?
> 
> For COMPILE_TEST on x86.

Well that's a logical OR right here, so it will work...

>>>>  +    select CLKSRC_OF
>>>>  +    default y
>>> 
>>>  No default, Kconfig platform selects the timer.
>> 
>>  Alright.
> [ ... ]
> 
> --
>  <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 linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 7/8] clocksource: Add a new timer-ingenic driver

[Daniel Lezcano]

On 29/03/2018 16:52, Paul Cercueil wrote:
> 
> 
> Le mer. 28 mars 2018 à 18:25, Daniel Lezcano <daniel.lezcano@linaro.org>
> a écrit :
>> On 28/03/2018 17:15, Paul Cercueil wrote:
>>>  Le 2018-03-24 07:26, Daniel Lezcano a écrit :
>>>>  On 18/03/2018 00:29, Paul Cercueil wrote:
>>>>>  This driver will use the TCU (Timer Counter Unit) present on the
>>>>> Ingenic
>>>>>  JZ47xx SoCs to provide the kernel with a clocksource and timers.
>>>>
>>>>  Please provide a more detailed description about the timer.
>>>
>>>  There's a doc file for that :)
>>
>> Usually, when there is a new driver I ask for a description in the
>> changelog for reference.
>>
>>>>  Where is the clocksource ?
>>>
>>>  Right, there is no clocksource, just timers.
>>>
>>>>  I don't see the point of using channel idx and pwm checking here.
>>>>
>>>>  There is one clockevent, why create multiple channels ? Can't you
>>>> stick
>>>>  to the usual init routine for a timer.
>>>
>>>  So the idea is that we use all the TCU channels that won't be used
>>> for PWM
>>>  as timers. Hence the PWM checking. Why is this bad?
>>
>> It is not bad but arguable. By checking the channels used by the pwm in
>> the code, you introduce an adherence between two subsystems even if it
>> is just related to the DT parsing part.
>>
>> As it is not needed to have more than one timer in the time framework
>> (at least with the same characteristics), the pwm channels check is
>> pointless. We can assume the author of the DT file is smart enough to
>> prevent conflicts and define a pwm and a timer properly instead of
>> adding more code complexity.
>>
>> In addition, simplifying the code will allow you to use the timer-of
>> code and reduce very significantly the init function.
> 
> That's what I had in my V1 and V2, my DT node for the timer-ingenic driver
> had a "timers" property (e.g. "timers = <4 5>;") to select the channels
> that
> should be used as timers. Then Rob told me I shouldn't do that, and instead
> detect the channels that will be used for PWM.
> 

[ ... ]

How do you specify the channels used for PWM ?

>>>>>
>>>>>  +config INGENIC_TIMER
>>>>>  +    bool "Clocksource/timer using the TCU in Ingenic JZ SoCs"
>>>>>  +    depends on MACH_INGENIC || COMPILE_TEST
>>>>
>>>>  bool "Clocksource/timer using the TCU in Ingenic JZ SoCs" if
>>>> COMPILE_TEST
>>>>
>>>>  Remove the depends MACH_INGENIC.
>>>
>>>  This driver is not useful on anything else than Ingenic SoCs, why
>>> should I
>>>  remove MACH_INGENIC then?
>>
>> For COMPILE_TEST on x86.
> 
> Well that's a logical OR right here, so it will work...

Right, I missed the second part of the condition. For consistency
reason, we don't add a dependency on the platform. The platform will
select it. Look the other timer options and you will see there is no
MACH deps. I'm trying consolidating all these options to have same
format and hopefully factor them out.





-- 
 <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 linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 7/8] clocksource: Add a new timer-ingenic driver,

[Paul Cercueil]

Le 2018-03-31 10:10, Daniel Lezcano a écrit :
> On 29/03/2018 16:52, Paul Cercueil wrote:
>> 
>> 
>> Le mer. 28 mars 2018 à 18:25, Daniel Lezcano 
>> <daniel.lezcano@linaro.org>
>> a écrit :
>>> On 28/03/2018 17:15, Paul Cercueil wrote:
>>>>  Le 2018-03-24 07:26, Daniel Lezcano a écrit :
>>>>>  On 18/03/2018 00:29, Paul Cercueil wrote:
>>>>>>  This driver will use the TCU (Timer Counter Unit) present on the
>>>>>> Ingenic
>>>>>>  JZ47xx SoCs to provide the kernel with a clocksource and timers.
>>>>> 
>>>>>  Please provide a more detailed description about the timer.
>>>> 
>>>>  There's a doc file for that :)
>>> 
>>> Usually, when there is a new driver I ask for a description in the
>>> changelog for reference.
>>> 
>>>>>  Where is the clocksource ?
>>>> 
>>>>  Right, there is no clocksource, just timers.
>>>> 
>>>>>  I don't see the point of using channel idx and pwm checking here.
>>>>> 
>>>>>  There is one clockevent, why create multiple channels ? Can't you
>>>>> stick
>>>>>  to the usual init routine for a timer.
>>>> 
>>>>  So the idea is that we use all the TCU channels that won't be used
>>>> for PWM
>>>>  as timers. Hence the PWM checking. Why is this bad?
>>> 
>>> It is not bad but arguable. By checking the channels used by the pwm 
>>> in
>>> the code, you introduce an adherence between two subsystems even if 
>>> it
>>> is just related to the DT parsing part.
>>> 
>>> As it is not needed to have more than one timer in the time framework
>>> (at least with the same characteristics), the pwm channels check is
>>> pointless. We can assume the author of the DT file is smart enough to
>>> prevent conflicts and define a pwm and a timer properly instead of
>>> adding more code complexity.
>>> 
>>> In addition, simplifying the code will allow you to use the timer-of
>>> code and reduce very significantly the init function.
>> 
>> That's what I had in my V1 and V2, my DT node for the timer-ingenic 
>> driver
>> had a "timers" property (e.g. "timers = <4 5>;") to select the 
>> channels
>> that
>> should be used as timers. Then Rob told me I shouldn't do that, and 
>> instead
>> detect the channels that will be used for PWM.
>> 
> 
> [ ... ]
> 
> How do you specify the channels used for PWM ?

To detect the channels that will be used as PWM I parse the whole 
devicetree
searching for "pwms" properties; check that the PWM handle is for our 
TCU PWM
driver; then read the PWM number from there.

Of course it's hackish, and it only works for devicetree. I preferred 
the
method with the "timers" property.

>>>>>> 
>>>>>>  +config INGENIC_TIMER
>>>>>>  +    bool "Clocksource/timer using the TCU in Ingenic JZ SoCs"
>>>>>>  +    depends on MACH_INGENIC || COMPILE_TEST
>>>>> 
>>>>>  bool "Clocksource/timer using the TCU in Ingenic JZ SoCs" if
>>>>> COMPILE_TEST
>>>>> 
>>>>>  Remove the depends MACH_INGENIC.
>>>> 
>>>>  This driver is not useful on anything else than Ingenic SoCs, why
>>>> should I
>>>>  remove MACH_INGENIC then?
>>> 
>>> For COMPILE_TEST on x86.
>> 
>> Well that's a logical OR right here, so it will work...
> 
> Right, I missed the second part of the condition. For consistency
> reason, we don't add a dependency on the platform. The platform will
> select it. Look the other timer options and you will see there is no
> MACH deps. I'm trying consolidating all these options to have same
> format and hopefully factor them out.

I'm all for factorisation, but what I dislike with not depending on
MACH_INGENIC, is that the driver now appears in the menuconfig for
every arch, even if it only applies to one MIPS SoC.

Regards,
-Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 7/8] clocksource: Add a new timer-ingenic driver

[Daniel Lezcano]

On 31/03/2018 19:46, Paul Cercueil wrote:
> Le 2018-03-31 10:10, Daniel Lezcano a écrit :
>> On 29/03/2018 16:52, Paul Cercueil wrote:
>>>
>>>
>>> Le mer. 28 mars 2018 à 18:25, Daniel Lezcano <daniel.lezcano@linaro.org>
>>> a écrit :
>>>> On 28/03/2018 17:15, Paul Cercueil wrote:
>>>>>  Le 2018-03-24 07:26, Daniel Lezcano a écrit :
>>>>>>  On 18/03/2018 00:29, Paul Cercueil wrote:
>>>>>>>  This driver will use the TCU (Timer Counter Unit) present on the
>>>>>>> Ingenic
>>>>>>>  JZ47xx SoCs to provide the kernel with a clocksource and timers.
>>>>>>
>>>>>>  Please provide a more detailed description about the timer.
>>>>>
>>>>>  There's a doc file for that :)
>>>>
>>>> Usually, when there is a new driver I ask for a description in the
>>>> changelog for reference.
>>>>
>>>>>>  Where is the clocksource ?
>>>>>
>>>>>  Right, there is no clocksource, just timers.
>>>>>
>>>>>>  I don't see the point of using channel idx and pwm checking here.
>>>>>>
>>>>>>  There is one clockevent, why create multiple channels ? Can't you
>>>>>> stick
>>>>>>  to the usual init routine for a timer.
>>>>>
>>>>>  So the idea is that we use all the TCU channels that won't be used
>>>>> for PWM
>>>>>  as timers. Hence the PWM checking. Why is this bad?
>>>>
>>>> It is not bad but arguable. By checking the channels used by the pwm in
>>>> the code, you introduce an adherence between two subsystems even if it
>>>> is just related to the DT parsing part.
>>>>
>>>> As it is not needed to have more than one timer in the time framework
>>>> (at least with the same characteristics), the pwm channels check is
>>>> pointless. We can assume the author of the DT file is smart enough to
>>>> prevent conflicts and define a pwm and a timer properly instead of
>>>> adding more code complexity.
>>>>
>>>> In addition, simplifying the code will allow you to use the timer-of
>>>> code and reduce very significantly the init function.
>>>
>>> That's what I had in my V1 and V2, my DT node for the timer-ingenic
>>> driver
>>> had a "timers" property (e.g. "timers = <4 5>;") to select the channels
>>> that
>>> should be used as timers. Then Rob told me I shouldn't do that, and
>>> instead
>>> detect the channels that will be used for PWM.
>>>
>>
>> [ ... ]
>>
>> How do you specify the channels used for PWM ?
> 
> To detect the channels that will be used as PWM I parse the whole
> devicetree
> searching for "pwms" properties; check that the PWM handle is for our
> TCU PWM
> driver; then read the PWM number from there.
> 
> Of course it's hackish, and it only works for devicetree. I preferred the
> method with the "timers" property.

Do you have a DT portion describing that? Eg somewhere in the kernel's
git tree ?

From what I understood, we can specify the channel for a pwm but not for
a timer, there is certainly something I'm missing.

>>>>>>>
>>>>>>>  +config INGENIC_TIMER
>>>>>>>  +    bool "Clocksource/timer using the TCU in Ingenic JZ SoCs"
>>>>>>>  +    depends on MACH_INGENIC || COMPILE_TEST
>>>>>>
>>>>>>  bool "Clocksource/timer using the TCU in Ingenic JZ SoCs" if
>>>>>> COMPILE_TEST
>>>>>>
>>>>>>  Remove the depends MACH_INGENIC.
>>>>>
>>>>>  This driver is not useful on anything else than Ingenic SoCs, why
>>>>> should I
>>>>>  remove MACH_INGENIC then?
>>>>
>>>> For COMPILE_TEST on x86.
>>>
>>> Well that's a logical OR right here, so it will work...
>>
>> Right, I missed the second part of the condition. For consistency
>> reason, we don't add a dependency on the platform. The platform will
>> select it. Look the other timer options and you will see there is no
>> MACH deps. I'm trying consolidating all these options to have same
>> format and hopefully factor them out.
> 
> I'm all for factorisation, but what I dislike with not depending on
> MACH_INGENIC, is that the driver now appears in the menuconfig for
> every arch, even if it only applies to one MIPS SoC.

Can you do the following change?

bool "Clocksource/timer using the TCU in Ingenic JZ SoCs" if COMPILE_TEST

so it will appear only when the COMPILE_TEST option is set whatever the
platform which is the purpose of this option to increase compile test
coverage.


-- 
 <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 linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 8/8] MAINTAINERS: Add myself as maintainer for Ingenic TCU drivers

[Paul Cercueil]

Add myself as maintainer for the ingenic-tcu-intc interrupt controller
driver, the ingenic-tcu-clocks clock driver, and the ingenic-tcu
clocksource driver.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 MAINTAINERS | 9 +++++++++
 1 file changed, 9 insertions(+)

 v2: No change
 v3: No change
 v4: No change

diff --git a/MAINTAINERS b/MAINTAINERS
index 4623caf8d72d..46d3955b92aa 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6959,6 +6959,15 @@ L:	linux-mtd@lists.infradead.org
 S:	Maintained
 F:	drivers/mtd/nand/jz4780_*
 
+INGENIC JZ47xx TCU drivers
+M:	Paul Cercueil <paul@crapouillou.net>
+S:	Maintained
+F:	drivers/clk/ingenic/tcu.c
+F:	drivers/irqchip/irq-ingenic-tcu.c
+F:	drivers/clocksource/timer-ingenic.c
+F:	include/linux/mfd/syscon/ingenic-tcu.h
+F:	include/dt-bindings/clock/ingenic,tcu.h
+
 INOTIFY
 M:	Jan Kara <jack@suse.cz>
 R:	Amir Goldstein <amir73il@gmail.com>
-- 
2.11.0

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

Re: [PATCH v4 0/8] Ingenic JZ47xx Timer/Counter Unit drivers

[Daniel Lezcano]

On 18/03/2018 00:28, Paul Cercueil wrote:
> Hi,
> 
> This is the 4th version of my TCU patchset.
> 
> The major change is a greatly improved documentation, both in-code
> and as separate text files, to describe how the hardware works and
> how the devicetree bindings should be used.
> 
> There are also cosmetic changes in the irqchip driver, and the
> clocksource driver will now use as timers all TCU channels not
> requested by the TCU PWM driver.

Hi Paul,

I don't know why but you series appears in reply to [PATCH v3 2/9]. Not
sure if it is my mailer or how you are sending the patches but if it is
the latter can you in the future, when resending a new version, not use
the in-reply-to option. It will be easier to follow the versions.

Thanks.

 -- Daniel



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

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

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

Re: [PATCH v4 0/8] Ingenic JZ47xx Timer/Counter Unit drivers,

[Paul Cercueil]

Le 2018-03-18 23:13, Daniel Lezcano a écrit :
> On 18/03/2018 00:28, Paul Cercueil wrote:
>> Hi,
>> 
>> This is the 4th version of my TCU patchset.
>> 
>> The major change is a greatly improved documentation, both in-code
>> and as separate text files, to describe how the hardware works and
>> how the devicetree bindings should be used.
>> 
>> There are also cosmetic changes in the irqchip driver, and the
>> clocksource driver will now use as timers all TCU channels not
>> requested by the TCU PWM driver.
> 
> Hi Paul,
> 
> I don't know why but you series appears in reply to [PATCH v3 2/9]. Not
> sure if it is my mailer or how you are sending the patches but if it is
> the latter can you in the future, when resending a new version, not use
> the in-reply-to option. It will be easier to follow the versions.
> 
> Thanks.
> 
>  -- Daniel

Hi Daniel,

I guess I did a mistake. I always reply to the first patch of the 
previous
version of the patchset (is that correct?).

Thanks,
-Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 0/8] Ingenic JZ47xx Timer/Counter Unit drivers

[Daniel Lezcano]

On 28/03/2018 17:01, Paul Cercueil wrote:
> Le 2018-03-18 23:13, Daniel Lezcano a écrit :
>> On 18/03/2018 00:28, Paul Cercueil wrote:
>>> Hi,
>>>
>>> This is the 4th version of my TCU patchset.
>>>
>>> The major change is a greatly improved documentation, both in-code
>>> and as separate text files, to describe how the hardware works and
>>> how the devicetree bindings should be used.
>>>
>>> There are also cosmetic changes in the irqchip driver, and the
>>> clocksource driver will now use as timers all TCU channels not
>>> requested by the TCU PWM driver.
>>
>> Hi Paul,
>>
>> I don't know why but you series appears in reply to [PATCH v3 2/9]. Not
>> sure if it is my mailer or how you are sending the patches but if it is
>> the latter can you in the future, when resending a new version, not use
>> the in-reply-to option. It will be easier to follow the versions.
>>
>> Thanks.
>>
>>  -- Daniel
> 
> Hi Daniel,
> 
> I guess I did a mistake. I always reply to the first patch of the previous
> version of the patchset (is that correct?).

It depends, if you have a threaded view of emails, it is not easy to
review the patches when they are in several levels. Usually you can see
the patches is top posted without in-reply-to every version.

You can use in-reply-to to an email suggesting a change in order to give
context.

For the v4 series of these drivers, I'm lost :/


-- 
 <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 linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html