Ingenic Timer/Counter Unit (TCU) patchset v12

Ingenic Timer/Counter Unit (TCU) patchset v12

[Paul Cercueil]

Hi,

Here's the V12 of my patchset to add support for the Timer/Counter Unit
(TCU) present on the JZ47xx SoCs from Ingenic.

This patchset is much shorter at only 13 patches vs. 27 patches in V11;
the remaining patches will be sent in parallel (if applicable) or as a
follow-up patchset once this one is merged.

In V11 the clocksource maintainers weren't happy with the size of the
ingenic-timer driver, which included clocks and irqchip setup code.
On the other hand, devicetree maintainers wanted one single node for
the TCU hardware since it's effectively just one hardware block.

In this patchset the functionality is cut in four different drivers:
a MFD one to provide the regmap, probe the children and which provides
several API functions; a clocks driver; a irqchip driver; a clocksource
driver. All these drivers work with the same regmap, have the same
compatible strings, and will probe _with the same devicetree node_.

Regards,
-Paul

[PATCH v12 01/13] 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>
Acked-by: Stephen Boyd <sboyd@kernel.org>
---

Notes:
    v2: Use SPDX identifier for the license
    
    v3/v4: No change
    
    v5: s/JZ47*_/TCU_/ and dropped *_CLK_LAST defines
    
    v6-v12: No change

 include/dt-bindings/clock/ingenic,tcu.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100644 include/dt-bindings/clock/ingenic,tcu.h

diff --git a/include/dt-bindings/clock/ingenic,tcu.h b/include/dt-bindings/clock/ingenic,tcu.h
new file mode 100644
index 000000000000..d569650a7945
--- /dev/null
+++ b/include/dt-bindings/clock/ingenic,tcu.h
@@ -0,0 +1,20 @@
+/* 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 TCU_CLK_TIMER0	0
+#define TCU_CLK_TIMER1	1
+#define TCU_CLK_TIMER2	2
+#define TCU_CLK_TIMER3	3
+#define TCU_CLK_TIMER4	4
+#define TCU_CLK_TIMER5	5
+#define TCU_CLK_TIMER6	6
+#define TCU_CLK_TIMER7	7
+#define TCU_CLK_WDT	8
+#define TCU_CLK_OST	9
+
+#endif /* __DT_BINDINGS_CLOCK_INGENIC_TCU_H__ */
-- 
2.21.0.593.g511ec345e18

[PATCH v12 02/13] 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.

The Timer/Counter Unit (TCU) in Ingenic JZ47xx SoCs is a multi-function
hardware block. It features up to to eight channels, that can be used as
counters, timers, or PWM.

- JZ4725B, JZ4750, JZ4755 only have six TCU channels. The other SoCs all
  have eight channels.

- JZ4725B introduced a separate channel, called Operating System Timer
  (OST). It is a 32-bit programmable timer. On JZ4770 and above, it is
  64-bit.

- Each one of the TCU 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.

- Each TCU channel works in one of two modes:
  * mode TCU1: channels cannot work in sleep mode, but are easier to
    operate.
  * mode TCU2: channels can work in sleep mode, but the operation is a bit
    more complicated than with TCU1 channels.

- The mode of each TCU channel depends on the SoC used:
  * On the oldest SoCs (up to JZ4740), all of the eight channels operate in
    TCU1 mode.
  * On JZ4725B, channel 5 operates as TCU2, the others operate as TCU1.
  * On newest SoCs (JZ4750 and above), channels 1-2 operate as TCU2, the
    others operate as TCU1.

- Each channel can generate an interrupt. Some channels share an interrupt
  line, some don't, and this changes between SoC versions:
  * on older SoCs (JZ4740 and below), channel 0 and channel 1 have their
    own interrupt line; channels 2-7 share the last interrupt line.
  * On JZ4725B, channel 0 has its own interrupt; channels 1-5 share one
    interrupt line; the OST uses the last interrupt line.
  * on newer SoCs (JZ4750 and above), channel 5 has its own interrupt;
    channels 0-4 and (if eight channels) 6-7 all share one interrupt line;
    the OST uses the last interrupt line.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    v4: New patch in this series
    
    v5: Added information about number of channels, and improved
     documentation about channel modes
    
    v6: Add info about OST (can be 32-bit on older SoCs)
    
    v7-v11: No change
    
    v12: Add details about new implementation

 Documentation/mips/ingenic-tcu.txt | 63 ++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)
 create mode 100644 Documentation/mips/ingenic-tcu.txt

diff --git a/Documentation/mips/ingenic-tcu.txt b/Documentation/mips/ingenic-tcu.txt
new file mode 100644
index 000000000000..1a753805779c
--- /dev/null
+++ b/Documentation/mips/ingenic-tcu.txt
@@ -0,0 +1,63 @@
+Ingenic JZ47xx SoCs Timer/Counter Unit hardware
+-----------------------------------------------
+
+The Timer/Counter Unit (TCU) in Ingenic JZ47xx SoCs is a multi-function
+hardware block. It features up to to eight channels, that can be used as
+counters, timers, or PWM.
+
+- JZ4725B, JZ4750, JZ4755 only have six TCU channels. The other SoCs all
+  have eight channels.
+
+- JZ4725B introduced a separate channel, called Operating System Timer
+  (OST). It is a 32-bit programmable timer. On JZ4760B and above, it is
+  64-bit.
+
+- Each one of the TCU 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.
+
+- Each TCU channel works in one of two modes:
+  * mode TCU1: channels cannot work in sleep mode, but are easier to
+    operate.
+  * mode TCU2: channels can work in sleep mode, but the operation is a bit
+    more complicated than with TCU1 channels.
+
+- The mode of each TCU channel depends on the SoC used:
+  * On the oldest SoCs (up to JZ4740), all of the eight channels operate in
+    TCU1 mode.
+  * On JZ4725B, channel 5 operates as TCU2, the others operate as TCU1.
+  * On newest SoCs (JZ4750 and above), channels 1-2 operate as TCU2, the
+    others operate as TCU1.
+
+- Each channel can generate an interrupt. Some channels share an interrupt
+  line, some don't, and this changes between SoC versions:
+  * on older SoCs (JZ4740 and below), channel 0 and channel 1 have their
+    own interrupt line; channels 2-7 share the last interrupt line.
+  * On JZ4725B, channel 0 has its own interrupt; channels 1-5 share one
+    interrupt line; the OST uses the last interrupt line.
+  * on newer SoCs (JZ4750 and above), channel 5 has its own interrupt;
+    channels 0-4 and (if eight channels) 6-7 all share one interrupt line;
+    the OST uses the last interrupt line.
+
+Implementation
+--------------
+
+The functionalities of the TCU hardware are spread across multiple drivers:
+- boilerplate:      drivers/mfd/ingenic-tcu.c
+- clocks:           drivers/clk/ingenic/tcu.c
+- interrupts:       drivers/irqchip/irq-ingenic-tcu.c
+- timers:           drivers/clocksource/ingenic-timer.c
+- PWM:              drivers/pwm/pwm-jz4740.c
+- watchdog:         drivers/watchdog/jz4740_wdt.c
+- OST:              drivers/clocksource/ingenic-ost.c
+
+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.
+
+For more information regarding the devicetree bindings of the TCU drivers,
+have a look at Documentation/devicetree/bindings/mfd/ingenic,tcu.txt.
-- 
2.21.0.593.g511ec345e18

[PATCH v12 03/13] 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>
---

Notes:
    v4: New patch in this series. Corresponds to V2 patches 3-4-5 with
     added content.
    
    v5: - Edited PWM/watchdog DT bindings documentation to point to the new
       document.
     - Moved main document to
       Documentation/devicetree/bindings/timer/ingenic,tcu.txt
     - Updated documentation to reflect the new devicetree bindings.
    
    v6: - Removed PWM/watchdog documentation files as asked by upstream
     - Removed doc about properties that should be implicit
     - Removed doc about ingenic,timer-channel /
       ingenic,clocksource-channel as they are gone
     - Fix WDT clock name in the binding doc
     - Fix lengths of register areas in watchdog/pwm nodes
    
    v7: No change
    
    v8: - Fix address of the PWM node
     - Added doc about system timer and clocksource children nodes
    
    v9: - Remove doc about system timer and clocksource children
       nodes...
    - Add doc about ingenic,pwm-channels-mask property
    
    v10: No change
    
    v11: Fix info about default value of ingenic,pwm-channels-mask
    
    v12: Drop sub-nodes for now; they will be introduced in a follow-up
    	 patchset.

 .../devicetree/bindings/timer/ingenic,tcu.txt | 59 +++++++++++++++++++
 1 file changed, 59 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/ingenic,tcu.txt

diff --git a/Documentation/devicetree/bindings/timer/ingenic,tcu.txt b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
new file mode 100644
index 000000000000..d101cd72c9b0
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
@@ -0,0 +1,59 @@
+Ingenic JZ47xx SoCs Timer/Counter Unit devicetree bindings
+==========================================================
+
+For a description of the TCU hardware and drivers, have a look at
+Documentation/mips/ingenic-tcu.txt.
+
+Required properties:
+
+- compatible: Must be one of:
+  * "ingenic,jz4740-tcu"
+  * "ingenic,jz4725b-tcu"
+  * "ingenic,jz4770-tcu"
+- reg: Should be the offset/length value corresponding to the TCU registers
+- clocks: List of phandle & clock specifiers for clocks external to the TCU.
+  The "pclk", "rtc" and "ext" clocks should be provided. The "tcu" clock
+  should be provided if the SoC has it.
+- clock-names: List of name strings for the external clocks.
+- #clock-cells: Should be <1>;
+  Clock consumers specify this argument to identify a clock. The valid values
+  may be found in <dt-bindings/clock/ingenic,tcu.h>.
+- interrupt-controller : Identifies the node as an interrupt controller
+- #interrupt-cells : Specifies the number of cells needed to encode an
+  interrupt source. The value should be 1.
+- interrupt-parent : phandle of the interrupt controller.
+- interrupts : Specifies the interrupt the controller is connected to.
+
+Optional properties:
+
+- ingenic,pwm-channels-mask: Bitmask of TCU channels reserved for PWM use.
+  Default value is 0xfc.
+
+
+Example
+==========================================================
+
+#include <dt-bindings/clock/jz4770-cgu.h>
+
+/ {
+	tcu: timer@10002000 {
+		compatible = "ingenic,jz4770-tcu";
+		reg = <0x10002000 0x1000>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0x0 0x10002000 0x1000>;
+
+		#clock-cells = <1>;
+
+		clocks = <&cgu JZ4770_CLK_RTC
+			  &cgu JZ4770_CLK_EXT
+			  &cgu JZ4770_CLK_PCLK>;
+		clock-names = "rtc", "ext", "pclk";
+
+		interrupt-controller;
+		#interrupt-cells = <1>;
+
+		interrupt-parent = <&intc>;
+		interrupts = <27 26 25>;
+	};
+};
-- 
2.21.0.593.g511ec345e18

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

[Rob Herring]

On Tue, May 21, 2019 at 04:51:31PM +0200, Paul Cercueil wrote:
> Add documentation about how to properly use the Ingenic TCU
> (Timer/Counter Unit) drivers from devicetree.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
> 
> Notes:
>     v4: New patch in this series. Corresponds to V2 patches 3-4-5 with
>      added content.
>     
>     v5: - Edited PWM/watchdog DT bindings documentation to point to the new
>        document.
>      - Moved main document to
>        Documentation/devicetree/bindings/timer/ingenic,tcu.txt
>      - Updated documentation to reflect the new devicetree bindings.
>     
>     v6: - Removed PWM/watchdog documentation files as asked by upstream
>      - Removed doc about properties that should be implicit
>      - Removed doc about ingenic,timer-channel /
>        ingenic,clocksource-channel as they are gone
>      - Fix WDT clock name in the binding doc
>      - Fix lengths of register areas in watchdog/pwm nodes
>     
>     v7: No change
>     
>     v8: - Fix address of the PWM node
>      - Added doc about system timer and clocksource children nodes
>     
>     v9: - Remove doc about system timer and clocksource children
>        nodes...
>     - Add doc about ingenic,pwm-channels-mask property
>     
>     v10: No change
>     
>     v11: Fix info about default value of ingenic,pwm-channels-mask
>     
>     v12: Drop sub-nodes for now; they will be introduced in a follow-up
>     	 patchset.

Why? I believe I acked them.

> 
>  .../devicetree/bindings/timer/ingenic,tcu.txt | 59 +++++++++++++++++++
>  1 file changed, 59 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/timer/ingenic,tcu.txt
> 
> diff --git a/Documentation/devicetree/bindings/timer/ingenic,tcu.txt b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
> new file mode 100644
> index 000000000000..d101cd72c9b0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
> @@ -0,0 +1,59 @@
> +Ingenic JZ47xx SoCs Timer/Counter Unit devicetree bindings
> +==========================================================
> +
> +For a description of the TCU hardware and drivers, have a look at
> +Documentation/mips/ingenic-tcu.txt.
> +
> +Required properties:
> +
> +- compatible: Must be one of:
> +  * "ingenic,jz4740-tcu"
> +  * "ingenic,jz4725b-tcu"
> +  * "ingenic,jz4770-tcu"
> +- reg: Should be the offset/length value corresponding to the TCU registers
> +- clocks: List of phandle & clock specifiers for clocks external to the TCU.
> +  The "pclk", "rtc" and "ext" clocks should be provided. The "tcu" clock
> +  should be provided if the SoC has it.
> +- clock-names: List of name strings for the external clocks.
> +- #clock-cells: Should be <1>;
> +  Clock consumers specify this argument to identify a clock. The valid values
> +  may be found in <dt-bindings/clock/ingenic,tcu.h>.
> +- interrupt-controller : Identifies the node as an interrupt controller
> +- #interrupt-cells : Specifies the number of cells needed to encode an
> +  interrupt source. The value should be 1.
> +- interrupt-parent : phandle of the interrupt controller.

Drop this 'interrupt-parent' is implied and could be in a parent node.

> +- interrupts : Specifies the interrupt the controller is connected to.
> +
> +Optional properties:
> +
> +- ingenic,pwm-channels-mask: Bitmask of TCU channels reserved for PWM use.
> +  Default value is 0xfc.
> +
> +
> +Example
> +==========================================================
> +
> +#include <dt-bindings/clock/jz4770-cgu.h>
> +
> +/ {
> +	tcu: timer@10002000 {
> +		compatible = "ingenic,jz4770-tcu";
> +		reg = <0x10002000 0x1000>;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges = <0x0 0x10002000 0x1000>;
> +
> +		#clock-cells = <1>;
> +
> +		clocks = <&cgu JZ4770_CLK_RTC
> +			  &cgu JZ4770_CLK_EXT
> +			  &cgu JZ4770_CLK_PCLK>;
> +		clock-names = "rtc", "ext", "pclk";
> +
> +		interrupt-controller;
> +		#interrupt-cells = <1>;
> +
> +		interrupt-parent = <&intc>;
> +		interrupts = <27 26 25>;
> +	};
> +};
> -- 
> 2.21.0.593.g511ec345e18
> 

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

[Paul Cercueil]

Le ven. 24 mai 2019 à 22:21, Rob Herring <robh@kernel.org> a écrit :
> On Tue, May 21, 2019 at 04:51:31PM +0200, Paul Cercueil wrote:
>>  Add documentation about how to properly use the Ingenic TCU
>>  (Timer/Counter Unit) drivers from devicetree.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>> 
>>  Notes:
>>      v4: New patch in this series. Corresponds to V2 patches 3-4-5 
>> with
>>       added content.
>> 
>>      v5: - Edited PWM/watchdog DT bindings documentation to point to 
>> the new
>>         document.
>>       - Moved main document to
>>         Documentation/devicetree/bindings/timer/ingenic,tcu.txt
>>       - Updated documentation to reflect the new devicetree bindings.
>> 
>>      v6: - Removed PWM/watchdog documentation files as asked by 
>> upstream
>>       - Removed doc about properties that should be implicit
>>       - Removed doc about ingenic,timer-channel /
>>         ingenic,clocksource-channel as they are gone
>>       - Fix WDT clock name in the binding doc
>>       - Fix lengths of register areas in watchdog/pwm nodes
>> 
>>      v7: No change
>> 
>>      v8: - Fix address of the PWM node
>>       - Added doc about system timer and clocksource children nodes
>> 
>>      v9: - Remove doc about system timer and clocksource children
>>         nodes...
>>      - Add doc about ingenic,pwm-channels-mask property
>> 
>>      v10: No change
>> 
>>      v11: Fix info about default value of ingenic,pwm-channels-mask
>> 
>>      v12: Drop sub-nodes for now; they will be introduced in a 
>> follow-up
>>      	 patchset.
> 
> Why? I believe I acked them.

The patchset was too big, and I've already been trying to get it 
upstream for
more than one year now. So I cut it in half in hope that it'll be 
easier to
upstream it that way.

>> 
>>   .../devicetree/bindings/timer/ingenic,tcu.txt | 59 
>> +++++++++++++++++++
>>   1 file changed, 59 insertions(+)
>>   create mode 100644 
>> Documentation/devicetree/bindings/timer/ingenic,tcu.txt
>> 
>>  diff --git 
>> a/Documentation/devicetree/bindings/timer/ingenic,tcu.txt 
>> b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
>>  new file mode 100644
>>  index 000000000000..d101cd72c9b0
>>  --- /dev/null
>>  +++ b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
>>  @@ -0,0 +1,59 @@
>>  +Ingenic JZ47xx SoCs Timer/Counter Unit devicetree bindings
>>  +==========================================================
>>  +
>>  +For a description of the TCU hardware and drivers, have a look at
>>  +Documentation/mips/ingenic-tcu.txt.
>>  +
>>  +Required properties:
>>  +
>>  +- compatible: Must be one of:
>>  +  * "ingenic,jz4740-tcu"
>>  +  * "ingenic,jz4725b-tcu"
>>  +  * "ingenic,jz4770-tcu"
>>  +- reg: Should be the offset/length value corresponding to the TCU 
>> registers
>>  +- clocks: List of phandle & clock specifiers for clocks external 
>> to the TCU.
>>  +  The "pclk", "rtc" and "ext" clocks should be provided. The "tcu" 
>> clock
>>  +  should be provided if the SoC has it.
>>  +- clock-names: List of name strings for the external clocks.
>>  +- #clock-cells: Should be <1>;
>>  +  Clock consumers specify this argument to identify a clock. The 
>> valid values
>>  +  may be found in <dt-bindings/clock/ingenic,tcu.h>.
>>  +- interrupt-controller : Identifies the node as an interrupt 
>> controller
>>  +- #interrupt-cells : Specifies the number of cells needed to 
>> encode an
>>  +  interrupt source. The value should be 1.
>>  +- interrupt-parent : phandle of the interrupt controller.
> 
> Drop this 'interrupt-parent' is implied and could be in a parent node.
> 
>>  +- interrupts : Specifies the interrupt the controller is connected 
>> to.
>>  +
>>  +Optional properties:
>>  +
>>  +- ingenic,pwm-channels-mask: Bitmask of TCU channels reserved for 
>> PWM use.
>>  +  Default value is 0xfc.
>>  +
>>  +
>>  +Example
>>  +==========================================================
>>  +
>>  +#include <dt-bindings/clock/jz4770-cgu.h>
>>  +
>>  +/ {
>>  +	tcu: timer@10002000 {
>>  +		compatible = "ingenic,jz4770-tcu";
>>  +		reg = <0x10002000 0x1000>;
>>  +		#address-cells = <1>;
>>  +		#size-cells = <1>;
>>  +		ranges = <0x0 0x10002000 0x1000>;
>>  +
>>  +		#clock-cells = <1>;
>>  +
>>  +		clocks = <&cgu JZ4770_CLK_RTC
>>  +			  &cgu JZ4770_CLK_EXT
>>  +			  &cgu JZ4770_CLK_PCLK>;
>>  +		clock-names = "rtc", "ext", "pclk";
>>  +
>>  +		interrupt-controller;
>>  +		#interrupt-cells = <1>;
>>  +
>>  +		interrupt-parent = <&intc>;
>>  +		interrupts = <27 26 25>;
>>  +	};
>>  +};
>>  --
>>  2.21.0.593.g511ec345e18
>> 

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

[Rob Herring]

On Sat, May 25, 2019 at 1:13 PM Paul Cercueil <paul@crapouillou.net> wrote:
>
>
>
> Le ven. 24 mai 2019 à 22:21, Rob Herring <robh@kernel.org> a écrit :
> > On Tue, May 21, 2019 at 04:51:31PM +0200, Paul Cercueil wrote:
> >>  Add documentation about how to properly use the Ingenic TCU
> >>  (Timer/Counter Unit) drivers from devicetree.
> >>
> >>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> >>  ---
> >>
> >>  Notes:
> >>      v4: New patch in this series. Corresponds to V2 patches 3-4-5
> >> with
> >>       added content.
> >>
> >>      v5: - Edited PWM/watchdog DT bindings documentation to point to
> >> the new
> >>         document.
> >>       - Moved main document to
> >>         Documentation/devicetree/bindings/timer/ingenic,tcu.txt
> >>       - Updated documentation to reflect the new devicetree bindings.
> >>
> >>      v6: - Removed PWM/watchdog documentation files as asked by
> >> upstream
> >>       - Removed doc about properties that should be implicit
> >>       - Removed doc about ingenic,timer-channel /
> >>         ingenic,clocksource-channel as they are gone
> >>       - Fix WDT clock name in the binding doc
> >>       - Fix lengths of register areas in watchdog/pwm nodes
> >>
> >>      v7: No change
> >>
> >>      v8: - Fix address of the PWM node
> >>       - Added doc about system timer and clocksource children nodes
> >>
> >>      v9: - Remove doc about system timer and clocksource children
> >>         nodes...
> >>      - Add doc about ingenic,pwm-channels-mask property
> >>
> >>      v10: No change
> >>
> >>      v11: Fix info about default value of ingenic,pwm-channels-mask
> >>
> >>      v12: Drop sub-nodes for now; they will be introduced in a
> >> follow-up
> >>               patchset.
> >
> > Why? I believe I acked them.
>
> The patchset was too big, and I've already been trying to get it
> upstream for
> more than one year now. So I cut it in half in hope that it'll be
> easier to
> upstream it that way.

You can drop the driver part and keep the binding. Unlike drivers, we
don't want bindings to needlessly evolve, and you don't have to wait
til a driver implements some functionality to add that to the binding.

Rob

[PATCH v12 04/13] mfd: Add Ingenic TCU driver

[Paul Cercueil]

This driver will provide a regmap that can be retrieved very early in
the boot process through the API function ingenic_tcu_get_regmap().

Additionally, it will call devm_of_platform_populate() so that all the
children devices will be probed.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    v12: New patch

 drivers/mfd/Kconfig             |   8 +++
 drivers/mfd/Makefile            |   1 +
 drivers/mfd/ingenic-tcu.c       | 113 ++++++++++++++++++++++++++++++++
 include/linux/mfd/ingenic-tcu.h |   8 +++
 4 files changed, 130 insertions(+)
 create mode 100644 drivers/mfd/ingenic-tcu.c

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 294d9567cc71..a13544474e05 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -494,6 +494,14 @@ config HTC_I2CPLD
 	  This device provides input and output GPIOs through an I2C
 	  interface to one or more sub-chips.
 
+config INGENIC_TCU
+	bool "Ingenic Timer/Counter Unit (TCU) support"
+	depends on MIPS || COMPILE_TEST
+	select REGMAP_MMIO
+	help
+	  Say yes here to support the Timer/Counter Unit (TCU) IP present
+	  in the JZ47xx SoCs from Ingenic.
+
 config MFD_INTEL_QUARK_I2C_GPIO
 	tristate "Intel Quark MFD I2C GPIO"
 	depends on PCI
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 52b1a90ff515..fb89e131ae98 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -180,6 +180,7 @@ obj-$(CONFIG_AB8500_CORE)	+= ab8500-core.o ab8500-sysctrl.o
 obj-$(CONFIG_MFD_TIMBERDALE)    += timberdale.o
 obj-$(CONFIG_PMIC_ADP5520)	+= adp5520.o
 obj-$(CONFIG_MFD_KEMPLD)	+= kempld-core.o
+obj-$(CONFIG_INGENIC_TCU)	+= ingenic-tcu.o
 obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO)	+= intel_quark_i2c_gpio.o
 obj-$(CONFIG_LPC_SCH)		+= lpc_sch.o
 obj-$(CONFIG_LPC_ICH)		+= lpc_ich.o
diff --git a/drivers/mfd/ingenic-tcu.c b/drivers/mfd/ingenic-tcu.c
new file mode 100644
index 000000000000..6c1d5e4310c1
--- /dev/null
+++ b/drivers/mfd/ingenic-tcu.c
@@ -0,0 +1,113 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * JZ47xx SoCs TCU MFD driver
+ * Copyright (C) 2019 Paul Cercueil <paul@crapouillou.net>
+ */
+
+#include <linux/mfd/ingenic-tcu.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+struct ingenic_soc_info {
+	unsigned int num_channels;
+};
+
+static struct regmap *tcu_regmap __initdata;
+
+static const struct regmap_config ingenic_tcu_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.max_register = TCU_REG_OST_CNTHBUF,
+};
+
+static const struct ingenic_soc_info jz4740_soc_info = {
+	.num_channels = 8,
+};
+
+static const struct ingenic_soc_info jz4725b_soc_info = {
+	.num_channels = 6,
+};
+
+static const struct of_device_id ingenic_tcu_of_match[] = {
+	{ .compatible = "ingenic,jz4740-tcu", .data = &jz4740_soc_info, },
+	{ .compatible = "ingenic,jz4725b-tcu", .data = &jz4725b_soc_info, },
+	{ .compatible = "ingenic,jz4770-tcu", .data = &jz4740_soc_info, },
+	{ }
+};
+
+static struct regmap * __init ingenic_tcu_create_regmap(struct device_node *np)
+{
+	struct resource res;
+	void __iomem *base;
+	struct regmap *map;
+
+	if (!of_match_node(ingenic_tcu_of_match, np))
+		return ERR_PTR(-EINVAL);
+
+	base = of_io_request_and_map(np, 0, "TCU");
+	if (IS_ERR(base))
+		return ERR_PTR(PTR_ERR(base));
+
+	map = regmap_init_mmio(NULL, base, &ingenic_tcu_regmap_config);
+	if (IS_ERR(map))
+		goto err_iounmap;
+
+	return map;
+
+err_iounmap:
+	iounmap(base);
+	of_address_to_resource(np, 0, &res);
+	release_mem_region(res.start, resource_size(&res));
+
+	return map;
+}
+
+static int __init ingenic_tcu_probe(struct platform_device *pdev)
+{
+	struct regmap *map = ingenic_tcu_get_regmap(pdev->dev.of_node);
+
+	platform_set_drvdata(pdev, map);
+
+	regmap_attach_dev(&pdev->dev, map, &ingenic_tcu_regmap_config);
+
+	return devm_of_platform_populate(&pdev->dev);
+}
+
+static struct platform_driver ingenic_tcu_driver = {
+	.driver = {
+		.name = "ingenic-tcu",
+		.of_match_table = ingenic_tcu_of_match,
+	},
+};
+
+static int __init ingenic_tcu_platform_init(void)
+{
+	return platform_driver_probe(&ingenic_tcu_driver,
+				     ingenic_tcu_probe);
+}
+subsys_initcall(ingenic_tcu_platform_init);
+
+struct regmap * __init ingenic_tcu_get_regmap(struct device_node *np)
+{
+	if (!tcu_regmap)
+		tcu_regmap = ingenic_tcu_create_regmap(np);
+
+	return tcu_regmap;
+}
+
+bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned int channel)
+{
+	const struct ingenic_soc_info *soc = device_get_match_data(dev->parent);
+
+	/* Enable all TCU channels for PWM use by default except channels 0/1 */
+	u32 pwm_channels_mask = GENMASK(soc->num_channels - 1, 2);
+
+	device_property_read_u32(dev->parent, "ingenic,pwm-channels-mask",
+				 &pwm_channels_mask);
+
+	return !!(pwm_channels_mask & BIT(channel));
+}
+EXPORT_SYMBOL_GPL(ingenic_tcu_pwm_can_use_chn);
diff --git a/include/linux/mfd/ingenic-tcu.h b/include/linux/mfd/ingenic-tcu.h
index 2083fa20821d..21df23916cd2 100644
--- a/include/linux/mfd/ingenic-tcu.h
+++ b/include/linux/mfd/ingenic-tcu.h
@@ -6,6 +6,11 @@
 #define __LINUX_MFD_INGENIC_TCU_H_
 
 #include <linux/bitops.h>
+#include <linux/init.h>
+
+struct device;
+struct device_node;
+struct regmap;
 
 #define TCU_REG_WDT_TDR		0x00
 #define TCU_REG_WDT_TCER	0x04
@@ -53,4 +58,7 @@
 #define TCU_REG_TCNTc(c)	(TCU_REG_TCNT0 + ((c) * TCU_CHANNEL_STRIDE))
 #define TCU_REG_TCSRc(c)	(TCU_REG_TCSR0 + ((c) * TCU_CHANNEL_STRIDE))
 
+struct regmap * __init ingenic_tcu_get_regmap(struct device_node *np);
+bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned int channel);
+
 #endif /* __LINUX_MFD_INGENIC_TCU_H_ */
-- 
2.21.0.593.g511ec345e18

Re: [PATCH v12 04/13] mfd: Add Ingenic TCU driver

[Paul Cercueil]

Hi,

I'll make a V13 of this patchset on top of -rc6, any feedback
for this MFD driver? It's been already one month.

Thanks,
-Paul



Le mar. 21 mai 2019 à 16:51, Paul Cercueil <paul@crapouillou.net> a 
écrit :
> This driver will provide a regmap that can be retrieved very early in
> the boot process through the API function ingenic_tcu_get_regmap().
> 
> Additionally, it will call devm_of_platform_populate() so that all the
> children devices will be probed.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
> 
> Notes:
>     v12: New patch
> 
>  drivers/mfd/Kconfig             |   8 +++
>  drivers/mfd/Makefile            |   1 +
>  drivers/mfd/ingenic-tcu.c       | 113 
> ++++++++++++++++++++++++++++++++
>  include/linux/mfd/ingenic-tcu.h |   8 +++
>  4 files changed, 130 insertions(+)
>  create mode 100644 drivers/mfd/ingenic-tcu.c
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 294d9567cc71..a13544474e05 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -494,6 +494,14 @@ config HTC_I2CPLD
>  	  This device provides input and output GPIOs through an I2C
>  	  interface to one or more sub-chips.
> 
> +config INGENIC_TCU
> +	bool "Ingenic Timer/Counter Unit (TCU) support"
> +	depends on MIPS || COMPILE_TEST
> +	select REGMAP_MMIO
> +	help
> +	  Say yes here to support the Timer/Counter Unit (TCU) IP present
> +	  in the JZ47xx SoCs from Ingenic.
> +
>  config MFD_INTEL_QUARK_I2C_GPIO
>  	tristate "Intel Quark MFD I2C GPIO"
>  	depends on PCI
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 52b1a90ff515..fb89e131ae98 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -180,6 +180,7 @@ obj-$(CONFIG_AB8500_CORE)	+= ab8500-core.o 
> ab8500-sysctrl.o
>  obj-$(CONFIG_MFD_TIMBERDALE)    += timberdale.o
>  obj-$(CONFIG_PMIC_ADP5520)	+= adp5520.o
>  obj-$(CONFIG_MFD_KEMPLD)	+= kempld-core.o
> +obj-$(CONFIG_INGENIC_TCU)	+= ingenic-tcu.o
>  obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO)	+= intel_quark_i2c_gpio.o
>  obj-$(CONFIG_LPC_SCH)		+= lpc_sch.o
>  obj-$(CONFIG_LPC_ICH)		+= lpc_ich.o
> diff --git a/drivers/mfd/ingenic-tcu.c b/drivers/mfd/ingenic-tcu.c
> new file mode 100644
> index 000000000000..6c1d5e4310c1
> --- /dev/null
> +++ b/drivers/mfd/ingenic-tcu.c
> @@ -0,0 +1,113 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * JZ47xx SoCs TCU MFD driver
> + * Copyright (C) 2019 Paul Cercueil <paul@crapouillou.net>
> + */
> +
> +#include <linux/mfd/ingenic-tcu.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +struct ingenic_soc_info {
> +	unsigned int num_channels;
> +};
> +
> +static struct regmap *tcu_regmap __initdata;
> +
> +static const struct regmap_config ingenic_tcu_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.max_register = TCU_REG_OST_CNTHBUF,
> +};
> +
> +static const struct ingenic_soc_info jz4740_soc_info = {
> +	.num_channels = 8,
> +};
> +
> +static const struct ingenic_soc_info jz4725b_soc_info = {
> +	.num_channels = 6,
> +};
> +
> +static const struct of_device_id ingenic_tcu_of_match[] = {
> +	{ .compatible = "ingenic,jz4740-tcu", .data = &jz4740_soc_info, },
> +	{ .compatible = "ingenic,jz4725b-tcu", .data = &jz4725b_soc_info, },
> +	{ .compatible = "ingenic,jz4770-tcu", .data = &jz4740_soc_info, },
> +	{ }
> +};
> +
> +static struct regmap * __init ingenic_tcu_create_regmap(struct 
> device_node *np)
> +{
> +	struct resource res;
> +	void __iomem *base;
> +	struct regmap *map;
> +
> +	if (!of_match_node(ingenic_tcu_of_match, np))
> +		return ERR_PTR(-EINVAL);
> +
> +	base = of_io_request_and_map(np, 0, "TCU");
> +	if (IS_ERR(base))
> +		return ERR_PTR(PTR_ERR(base));
> +
> +	map = regmap_init_mmio(NULL, base, &ingenic_tcu_regmap_config);
> +	if (IS_ERR(map))
> +		goto err_iounmap;
> +
> +	return map;
> +
> +err_iounmap:
> +	iounmap(base);
> +	of_address_to_resource(np, 0, &res);
> +	release_mem_region(res.start, resource_size(&res));
> +
> +	return map;
> +}
> +
> +static int __init ingenic_tcu_probe(struct platform_device *pdev)
> +{
> +	struct regmap *map = ingenic_tcu_get_regmap(pdev->dev.of_node);
> +
> +	platform_set_drvdata(pdev, map);
> +
> +	regmap_attach_dev(&pdev->dev, map, &ingenic_tcu_regmap_config);
> +
> +	return devm_of_platform_populate(&pdev->dev);
> +}
> +
> +static struct platform_driver ingenic_tcu_driver = {
> +	.driver = {
> +		.name = "ingenic-tcu",
> +		.of_match_table = ingenic_tcu_of_match,
> +	},
> +};
> +
> +static int __init ingenic_tcu_platform_init(void)
> +{
> +	return platform_driver_probe(&ingenic_tcu_driver,
> +				     ingenic_tcu_probe);
> +}
> +subsys_initcall(ingenic_tcu_platform_init);
> +
> +struct regmap * __init ingenic_tcu_get_regmap(struct device_node *np)
> +{
> +	if (!tcu_regmap)
> +		tcu_regmap = ingenic_tcu_create_regmap(np);
> +
> +	return tcu_regmap;
> +}
> +
> +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned int 
> channel)
> +{
> +	const struct ingenic_soc_info *soc = 
> device_get_match_data(dev->parent);
> +
> +	/* Enable all TCU channels for PWM use by default except channels 
> 0/1 */
> +	u32 pwm_channels_mask = GENMASK(soc->num_channels - 1, 2);
> +
> +	device_property_read_u32(dev->parent, "ingenic,pwm-channels-mask",
> +				 &pwm_channels_mask);
> +
> +	return !!(pwm_channels_mask & BIT(channel));
> +}
> +EXPORT_SYMBOL_GPL(ingenic_tcu_pwm_can_use_chn);
> diff --git a/include/linux/mfd/ingenic-tcu.h 
> b/include/linux/mfd/ingenic-tcu.h
> index 2083fa20821d..21df23916cd2 100644
> --- a/include/linux/mfd/ingenic-tcu.h
> +++ b/include/linux/mfd/ingenic-tcu.h
> @@ -6,6 +6,11 @@
>  #define __LINUX_MFD_INGENIC_TCU_H_
> 
>  #include <linux/bitops.h>
> +#include <linux/init.h>
> +
> +struct device;
> +struct device_node;
> +struct regmap;
> 
>  #define TCU_REG_WDT_TDR		0x00
>  #define TCU_REG_WDT_TCER	0x04
> @@ -53,4 +58,7 @@
>  #define TCU_REG_TCNTc(c)	(TCU_REG_TCNT0 + ((c) * TCU_CHANNEL_STRIDE))
>  #define TCU_REG_TCSRc(c)	(TCU_REG_TCSR0 + ((c) * TCU_CHANNEL_STRIDE))
> 
> +struct regmap * __init ingenic_tcu_get_regmap(struct device_node 
> *np);
> +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned int 
> channel);
> +
>  #endif /* __LINUX_MFD_INGENIC_TCU_H_ */
> --
> 2.21.0.593.g511ec345e18
> 

Re: [PATCH v12 04/13] mfd: Add Ingenic TCU driver

[Lee Jones]

On Tue, 21 May 2019, Paul Cercueil wrote:

> This driver will provide a regmap that can be retrieved very early in
> the boot process through the API function ingenic_tcu_get_regmap().
> 
> Additionally, it will call devm_of_platform_populate() so that all the
> children devices will be probed.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
> 
> Notes:
>     v12: New patch
> 
>  drivers/mfd/Kconfig             |   8 +++
>  drivers/mfd/Makefile            |   1 +
>  drivers/mfd/ingenic-tcu.c       | 113 ++++++++++++++++++++++++++++++++
>  include/linux/mfd/ingenic-tcu.h |   8 +++
>  4 files changed, 130 insertions(+)
>  create mode 100644 drivers/mfd/ingenic-tcu.c
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 294d9567cc71..a13544474e05 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -494,6 +494,14 @@ config HTC_I2CPLD
>  	  This device provides input and output GPIOs through an I2C
>  	  interface to one or more sub-chips.
>  
> +config INGENIC_TCU
> +	bool "Ingenic Timer/Counter Unit (TCU) support"
> +	depends on MIPS || COMPILE_TEST
> +	select REGMAP_MMIO
> +	help
> +	  Say yes here to support the Timer/Counter Unit (TCU) IP present
> +	  in the JZ47xx SoCs from Ingenic.
> +
>  config MFD_INTEL_QUARK_I2C_GPIO
>  	tristate "Intel Quark MFD I2C GPIO"
>  	depends on PCI
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 52b1a90ff515..fb89e131ae98 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -180,6 +180,7 @@ obj-$(CONFIG_AB8500_CORE)	+= ab8500-core.o ab8500-sysctrl.o
>  obj-$(CONFIG_MFD_TIMBERDALE)    += timberdale.o
>  obj-$(CONFIG_PMIC_ADP5520)	+= adp5520.o
>  obj-$(CONFIG_MFD_KEMPLD)	+= kempld-core.o
> +obj-$(CONFIG_INGENIC_TCU)	+= ingenic-tcu.o
>  obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO)	+= intel_quark_i2c_gpio.o
>  obj-$(CONFIG_LPC_SCH)		+= lpc_sch.o
>  obj-$(CONFIG_LPC_ICH)		+= lpc_ich.o
> diff --git a/drivers/mfd/ingenic-tcu.c b/drivers/mfd/ingenic-tcu.c
> new file mode 100644
> index 000000000000..6c1d5e4310c1
> --- /dev/null
> +++ b/drivers/mfd/ingenic-tcu.c
> @@ -0,0 +1,113 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * JZ47xx SoCs TCU MFD driver

Nit: Another line here please.

> + * Copyright (C) 2019 Paul Cercueil <paul@crapouillou.net>
> + */
> +
> +#include <linux/mfd/ingenic-tcu.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +struct ingenic_soc_info {
> +	unsigned int num_channels;
> +};
> +
> +static struct regmap *tcu_regmap __initdata;
> +
> +static const struct regmap_config ingenic_tcu_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.max_register = TCU_REG_OST_CNTHBUF,
> +};
> +
> +static const struct ingenic_soc_info jz4740_soc_info = {
> +	.num_channels = 8,
> +};
> +
> +static const struct ingenic_soc_info jz4725b_soc_info = {
> +	.num_channels = 6,
> +};
> +
> +static const struct of_device_id ingenic_tcu_of_match[] = {
> +	{ .compatible = "ingenic,jz4740-tcu", .data = &jz4740_soc_info, },
> +	{ .compatible = "ingenic,jz4725b-tcu", .data = &jz4725b_soc_info, },
> +	{ .compatible = "ingenic,jz4770-tcu", .data = &jz4740_soc_info, },
> +	{ }
> +};
> +
> +static struct regmap * __init ingenic_tcu_create_regmap(struct device_node *np)
> +{
> +	struct resource res;
> +	void __iomem *base;
> +	struct regmap *map;
> +
> +	if (!of_match_node(ingenic_tcu_of_match, np))
> +		return ERR_PTR(-EINVAL);
> +
> +	base = of_io_request_and_map(np, 0, "TCU");
> +	if (IS_ERR(base))
> +		return ERR_PTR(PTR_ERR(base));
> +
> +	map = regmap_init_mmio(NULL, base, &ingenic_tcu_regmap_config);
> +	if (IS_ERR(map))
> +		goto err_iounmap;
> +
> +	return map;
> +
> +err_iounmap:
> +	iounmap(base);
> +	of_address_to_resource(np, 0, &res);
> +	release_mem_region(res.start, resource_size(&res));
> +
> +	return map;
> +}

Why does this need to be set-up earlier than probe()?

> +static int __init ingenic_tcu_probe(struct platform_device *pdev)
> +{
> +	struct regmap *map = ingenic_tcu_get_regmap(pdev->dev.of_node);
> +
> +	platform_set_drvdata(pdev, map);
> +
> +	regmap_attach_dev(&pdev->dev, map, &ingenic_tcu_regmap_config);
> +
> +	return devm_of_platform_populate(&pdev->dev);
> +}
> +
> +static struct platform_driver ingenic_tcu_driver = {
> +	.driver = {
> +		.name = "ingenic-tcu",
> +		.of_match_table = ingenic_tcu_of_match,
> +	},
> +};
> +
> +static int __init ingenic_tcu_platform_init(void)
> +{
> +	return platform_driver_probe(&ingenic_tcu_driver,
> +				     ingenic_tcu_probe);

What?  Why?

> +}
> +subsys_initcall(ingenic_tcu_platform_init);
> +
> +struct regmap * __init ingenic_tcu_get_regmap(struct device_node *np)
> +{
> +	if (!tcu_regmap)
> +		tcu_regmap = ingenic_tcu_create_regmap(np);
> +
> +	return tcu_regmap;
> +}

This makes me pretty uncomfortable.

What calls it?

> +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned int channel)
> +{
> +	const struct ingenic_soc_info *soc = device_get_match_data(dev->parent);
> +
> +	/* Enable all TCU channels for PWM use by default except channels 0/1 */
> +	u32 pwm_channels_mask = GENMASK(soc->num_channels - 1, 2);
> +
> +	device_property_read_u32(dev->parent, "ingenic,pwm-channels-mask",
> +				 &pwm_channels_mask);
> +
> +	return !!(pwm_channels_mask & BIT(channel));
> +}
> +EXPORT_SYMBOL_GPL(ingenic_tcu_pwm_can_use_chn);
> diff --git a/include/linux/mfd/ingenic-tcu.h b/include/linux/mfd/ingenic-tcu.h
> index 2083fa20821d..21df23916cd2 100644
> --- a/include/linux/mfd/ingenic-tcu.h
> +++ b/include/linux/mfd/ingenic-tcu.h
> @@ -6,6 +6,11 @@
>  #define __LINUX_MFD_INGENIC_TCU_H_
>  
>  #include <linux/bitops.h>
> +#include <linux/init.h>
> +
> +struct device;
> +struct device_node;
> +struct regmap;
>  
>  #define TCU_REG_WDT_TDR		0x00
>  #define TCU_REG_WDT_TCER	0x04
> @@ -53,4 +58,7 @@
>  #define TCU_REG_TCNTc(c)	(TCU_REG_TCNT0 + ((c) * TCU_CHANNEL_STRIDE))
>  #define TCU_REG_TCSRc(c)	(TCU_REG_TCSR0 + ((c) * TCU_CHANNEL_STRIDE))
>  
> +struct regmap * __init ingenic_tcu_get_regmap(struct device_node *np);
> +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned int channel);
> +
>  #endif /* __LINUX_MFD_INGENIC_TCU_H_ */

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

Re: [PATCH v12 04/13] mfd: Add Ingenic TCU driver

[Paul Cercueil]

Hi Lee,

Le mer. 26 juin 2019 à 15:18, Lee Jones <lee.jones@linaro.org> a 
écrit :
> On Tue, 21 May 2019, Paul Cercueil wrote:
> 
>>  This driver will provide a regmap that can be retrieved very early 
>> in
>>  the boot process through the API function ingenic_tcu_get_regmap().
>> 
>>  Additionally, it will call devm_of_platform_populate() so that all 
>> the
>>  children devices will be probed.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>> 
>>  Notes:
>>      v12: New patch
>> 
>>   drivers/mfd/Kconfig             |   8 +++
>>   drivers/mfd/Makefile            |   1 +
>>   drivers/mfd/ingenic-tcu.c       | 113 
>> ++++++++++++++++++++++++++++++++
>>   include/linux/mfd/ingenic-tcu.h |   8 +++
>>   4 files changed, 130 insertions(+)
>>   create mode 100644 drivers/mfd/ingenic-tcu.c
>> 
>>  diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>>  index 294d9567cc71..a13544474e05 100644
>>  --- a/drivers/mfd/Kconfig
>>  +++ b/drivers/mfd/Kconfig
>>  @@ -494,6 +494,14 @@ config HTC_I2CPLD
>>   	  This device provides input and output GPIOs through an I2C
>>   	  interface to one or more sub-chips.
>> 
>>  +config INGENIC_TCU
>>  +	bool "Ingenic Timer/Counter Unit (TCU) support"
>>  +	depends on MIPS || COMPILE_TEST
>>  +	select REGMAP_MMIO
>>  +	help
>>  +	  Say yes here to support the Timer/Counter Unit (TCU) IP present
>>  +	  in the JZ47xx SoCs from Ingenic.
>>  +
>>   config MFD_INTEL_QUARK_I2C_GPIO
>>   	tristate "Intel Quark MFD I2C GPIO"
>>   	depends on PCI
>>  diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>>  index 52b1a90ff515..fb89e131ae98 100644
>>  --- a/drivers/mfd/Makefile
>>  +++ b/drivers/mfd/Makefile
>>  @@ -180,6 +180,7 @@ obj-$(CONFIG_AB8500_CORE)	+= ab8500-core.o 
>> ab8500-sysctrl.o
>>   obj-$(CONFIG_MFD_TIMBERDALE)    += timberdale.o
>>   obj-$(CONFIG_PMIC_ADP5520)	+= adp5520.o
>>   obj-$(CONFIG_MFD_KEMPLD)	+= kempld-core.o
>>  +obj-$(CONFIG_INGENIC_TCU)	+= ingenic-tcu.o
>>   obj-$(CONFIG_MFD_INTEL_QUARK_I2C_GPIO)	+= intel_quark_i2c_gpio.o
>>   obj-$(CONFIG_LPC_SCH)		+= lpc_sch.o
>>   obj-$(CONFIG_LPC_ICH)		+= lpc_ich.o
>>  diff --git a/drivers/mfd/ingenic-tcu.c b/drivers/mfd/ingenic-tcu.c
>>  new file mode 100644
>>  index 000000000000..6c1d5e4310c1
>>  --- /dev/null
>>  +++ b/drivers/mfd/ingenic-tcu.c
>>  @@ -0,0 +1,113 @@
>>  +// SPDX-License-Identifier: GPL-2.0
>>  +/*
>>  + * JZ47xx SoCs TCU MFD driver
> 
> Nit: Another line here please.
> 
>>  + * Copyright (C) 2019 Paul Cercueil <paul@crapouillou.net>
>>  + */
>>  +
>>  +#include <linux/mfd/ingenic-tcu.h>
>>  +#include <linux/of_address.h>
>>  +#include <linux/of_platform.h>
>>  +#include <linux/platform_device.h>
>>  +#include <linux/regmap.h>
>>  +
>>  +struct ingenic_soc_info {
>>  +	unsigned int num_channels;
>>  +};
>>  +
>>  +static struct regmap *tcu_regmap __initdata;
>>  +
>>  +static const struct regmap_config ingenic_tcu_regmap_config = {
>>  +	.reg_bits = 32,
>>  +	.val_bits = 32,
>>  +	.reg_stride = 4,
>>  +	.max_register = TCU_REG_OST_CNTHBUF,
>>  +};
>>  +
>>  +static const struct ingenic_soc_info jz4740_soc_info = {
>>  +	.num_channels = 8,
>>  +};
>>  +
>>  +static const struct ingenic_soc_info jz4725b_soc_info = {
>>  +	.num_channels = 6,
>>  +};
>>  +
>>  +static const struct of_device_id ingenic_tcu_of_match[] = {
>>  +	{ .compatible = "ingenic,jz4740-tcu", .data = &jz4740_soc_info, },
>>  +	{ .compatible = "ingenic,jz4725b-tcu", .data = &jz4725b_soc_info, 
>> },
>>  +	{ .compatible = "ingenic,jz4770-tcu", .data = &jz4740_soc_info, },
>>  +	{ }
>>  +};
>>  +
>>  +static struct regmap * __init ingenic_tcu_create_regmap(struct 
>> device_node *np)
>>  +{
>>  +	struct resource res;
>>  +	void __iomem *base;
>>  +	struct regmap *map;
>>  +
>>  +	if (!of_match_node(ingenic_tcu_of_match, np))
>>  +		return ERR_PTR(-EINVAL);
>>  +
>>  +	base = of_io_request_and_map(np, 0, "TCU");
>>  +	if (IS_ERR(base))
>>  +		return ERR_PTR(PTR_ERR(base));
>>  +
>>  +	map = regmap_init_mmio(NULL, base, &ingenic_tcu_regmap_config);
>>  +	if (IS_ERR(map))
>>  +		goto err_iounmap;
>>  +
>>  +	return map;
>>  +
>>  +err_iounmap:
>>  +	iounmap(base);
>>  +	of_address_to_resource(np, 0, &res);
>>  +	release_mem_region(res.start, resource_size(&res));
>>  +
>>  +	return map;
>>  +}
> 
> Why does this need to be set-up earlier than probe()?

See the explanation below.

>>  +static int __init ingenic_tcu_probe(struct platform_device *pdev)
>>  +{
>>  +	struct regmap *map = ingenic_tcu_get_regmap(pdev->dev.of_node);
>>  +
>>  +	platform_set_drvdata(pdev, map);
>>  +
>>  +	regmap_attach_dev(&pdev->dev, map, &ingenic_tcu_regmap_config);
>>  +
>>  +	return devm_of_platform_populate(&pdev->dev);
>>  +}
>>  +
>>  +static struct platform_driver ingenic_tcu_driver = {
>>  +	.driver = {
>>  +		.name = "ingenic-tcu",
>>  +		.of_match_table = ingenic_tcu_of_match,
>>  +	},
>>  +};
>>  +
>>  +static int __init ingenic_tcu_platform_init(void)
>>  +{
>>  +	return platform_driver_probe(&ingenic_tcu_driver,
>>  +				     ingenic_tcu_probe);
> 
> What?  Why?

The device driver probed here will populate the children devices,
which will be able to retrieve the pointer to the regmap through
device_get_regmap(dev->parent).

The children devices are normal platform drivers that can be probed
the normal way. These are the PWM driver, the watchdog driver, and the
OST (OS Timer) clocksource driver, all part of the same hardware block
(the Timer/Counter Unit or TCU).

>>  +}
>>  +subsys_initcall(ingenic_tcu_platform_init);
>>  +
>>  +struct regmap * __init ingenic_tcu_get_regmap(struct device_node 
>> *np)
>>  +{
>>  +	if (!tcu_regmap)
>>  +		tcu_regmap = ingenic_tcu_create_regmap(np);
>>  +
>>  +	return tcu_regmap;
>>  +}
> 
> This makes me pretty uncomfortable.
> 
> What calls it?

The TCU IRQ driver (patch [06/13]), clocks driver (patch [05/13]), and 
the
non-OST clocksource driver (patch [07/13]) all probe very early in the 
boot
process, and share the same devicetree node. They call this function to 
get
a pointer to the regmap.

>>  +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned int 
>> channel)
>>  +{
>>  +	const struct ingenic_soc_info *soc = 
>> device_get_match_data(dev->parent);
>>  +
>>  +	/* Enable all TCU channels for PWM use by default except channels 
>> 0/1 */
>>  +	u32 pwm_channels_mask = GENMASK(soc->num_channels - 1, 2);
>>  +
>>  +	device_property_read_u32(dev->parent, "ingenic,pwm-channels-mask",
>>  +				 &pwm_channels_mask);
>>  +
>>  +	return !!(pwm_channels_mask & BIT(channel));
>>  +}
>>  +EXPORT_SYMBOL_GPL(ingenic_tcu_pwm_can_use_chn);
>>  diff --git a/include/linux/mfd/ingenic-tcu.h 
>> b/include/linux/mfd/ingenic-tcu.h
>>  index 2083fa20821d..21df23916cd2 100644
>>  --- a/include/linux/mfd/ingenic-tcu.h
>>  +++ b/include/linux/mfd/ingenic-tcu.h
>>  @@ -6,6 +6,11 @@
>>   #define __LINUX_MFD_INGENIC_TCU_H_
>> 
>>   #include <linux/bitops.h>
>>  +#include <linux/init.h>
>>  +
>>  +struct device;
>>  +struct device_node;
>>  +struct regmap;
>> 
>>   #define TCU_REG_WDT_TDR		0x00
>>   #define TCU_REG_WDT_TCER	0x04
>>  @@ -53,4 +58,7 @@
>>   #define TCU_REG_TCNTc(c)	(TCU_REG_TCNT0 + ((c) * 
>> TCU_CHANNEL_STRIDE))
>>   #define TCU_REG_TCSRc(c)	(TCU_REG_TCSR0 + ((c) * 
>> TCU_CHANNEL_STRIDE))
>> 
>>  +struct regmap * __init ingenic_tcu_get_regmap(struct device_node 
>> *np);
>>  +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned int 
>> channel);
>>  +
>>   #endif /* __LINUX_MFD_INGENIC_TCU_H_ */
> 
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v12 04/13] mfd: Add Ingenic TCU driver

[Lee Jones]

On Wed, 26 Jun 2019, Paul Cercueil wrote:
> Le mer. 26 juin 2019 à 15:18, Lee Jones <lee.jones@linaro.org> a écrit :
> > On Tue, 21 May 2019, Paul Cercueil wrote:
> > 
> > >  This driver will provide a regmap that can be retrieved very early
> > > in
> > >  the boot process through the API function ingenic_tcu_get_regmap().
> > > 
> > >  Additionally, it will call devm_of_platform_populate() so that all
> > > the
> > >  children devices will be probed.
> > > 
> > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > >  ---
> > > 
> > >  Notes:
> > >      v12: New patch
> > > 
> > >   drivers/mfd/Kconfig             |   8 +++
> > >   drivers/mfd/Makefile            |   1 +
> > >   drivers/mfd/ingenic-tcu.c       | 113
> > > ++++++++++++++++++++++++++++++++
> > >   include/linux/mfd/ingenic-tcu.h |   8 +++
> > >   4 files changed, 130 insertions(+)
> > >   create mode 100644 drivers/mfd/ingenic-tcu.c

[...]

> > >  +static struct regmap * __init ingenic_tcu_create_regmap(struct
> > > device_node *np)
> > >  +{
> > >  +	struct resource res;
> > >  +	void __iomem *base;
> > >  +	struct regmap *map;
> > >  +
> > >  +	if (!of_match_node(ingenic_tcu_of_match, np))
> > >  +		return ERR_PTR(-EINVAL);

Drop this check.

> > >  +	base = of_io_request_and_map(np, 0, "TCU");
> > >  +	if (IS_ERR(base))
> > >  +		return ERR_PTR(PTR_ERR(base));
> > >  +
> > >  +	map = regmap_init_mmio(NULL, base, &ingenic_tcu_regmap_config);
> > >  +	if (IS_ERR(map))
> > >  +		goto err_iounmap;

Place this inside probe().

> > >  +	return map;
> > >  +
> > >  +err_iounmap:
> > >  +	iounmap(base);
> > >  +	of_address_to_resource(np, 0, &res);
> > >  +	release_mem_region(res.start, resource_size(&res));
> > >  +
> > >  +	return map;
> > >  +}
> > 
> > Why does this need to be set-up earlier than probe()?
> 
> See the explanation below.

I think the answer is, it doesn't.

> > >  +static int __init ingenic_tcu_probe(struct platform_device *pdev)
> > >  +{
> > >  +	struct regmap *map = ingenic_tcu_get_regmap(pdev->dev.of_node);
> > >  +
> > >  +	platform_set_drvdata(pdev, map);
> > >  +
> > >  +	regmap_attach_dev(&pdev->dev, map, &ingenic_tcu_regmap_config);
> > >  +
> > >  +	return devm_of_platform_populate(&pdev->dev);
> > >  +}
> > >  +
> > >  +static struct platform_driver ingenic_tcu_driver = {
> > >  +	.driver = {
> > >  +		.name = "ingenic-tcu",
> > >  +		.of_match_table = ingenic_tcu_of_match,
> > >  +	},
> > >  +};
> > >  +
> > >  +static int __init ingenic_tcu_platform_init(void)
> > >  +{
> > >  +	return platform_driver_probe(&ingenic_tcu_driver,
> > >  +				     ingenic_tcu_probe);
> > 
> > What?  Why?
> 
> The device driver probed here will populate the children devices,
> which will be able to retrieve the pointer to the regmap through
> device_get_regmap(dev->parent).

I've never heard of this call.  Where is it?

> The children devices are normal platform drivers that can be probed
> the normal way. These are the PWM driver, the watchdog driver, and the
> OST (OS Timer) clocksource driver, all part of the same hardware block
> (the Timer/Counter Unit or TCU).

If they are normal devices, then there is no need to roll your own
regmap-getter implementation like this.

> > >  +}
> > >  +subsys_initcall(ingenic_tcu_platform_init);
> > >  +
> > >  +struct regmap * __init ingenic_tcu_get_regmap(struct device_node
> > > *np)
> > >  +{
> > >  +	if (!tcu_regmap)
> > >  +		tcu_regmap = ingenic_tcu_create_regmap(np);
> > >  +
> > >  +	return tcu_regmap;
> > >  +}
> > 
> > This makes me pretty uncomfortable.
> > 
> > What calls it?
> 
> The TCU IRQ driver (patch [06/13]), clocks driver (patch [05/13]), and the
> non-OST clocksource driver (patch [07/13]) all probe very early in the boot
> process, and share the same devicetree node. They call this function to get
> a pointer to the regmap.

Horrible!

Instead, you should send it through platform_set_drvdata() and collect
it in the child drivers with platform_get_drvdata(dev->parent).

> > >  +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned int
> > > channel)
> > >  +{
> > >  +	const struct ingenic_soc_info *soc =
> > > device_get_match_data(dev->parent);
> > >  +
> > >  +	/* Enable all TCU channels for PWM use by default except channels
> > > 0/1 */
> > >  +	u32 pwm_channels_mask = GENMASK(soc->num_channels - 1, 2);
> > >  +
> > >  +	device_property_read_u32(dev->parent, "ingenic,pwm-channels-mask",
> > >  +				 &pwm_channels_mask);

Doesn't this call overwrite the previous assignment above?

> > >  +	return !!(pwm_channels_mask & BIT(channel));
> > >  +}
> > >  +EXPORT_SYMBOL_GPL(ingenic_tcu_pwm_can_use_chn);

Where is this called from?

I think this needs a review by the DT guys.

> > >  diff --git a/include/linux/mfd/ingenic-tcu.h
> > > b/include/linux/mfd/ingenic-tcu.h
> > >  index 2083fa20821d..21df23916cd2 100644
> > >  --- a/include/linux/mfd/ingenic-tcu.h
> > >  +++ b/include/linux/mfd/ingenic-tcu.h
> > >  @@ -6,6 +6,11 @@
> > >   #define __LINUX_MFD_INGENIC_TCU_H_
> > > 
> > >   #include <linux/bitops.h>
> > >  +#include <linux/init.h>
> > >  +
> > >  +struct device;
> > >  +struct device_node;
> > >  +struct regmap;
> > > 
> > >   #define TCU_REG_WDT_TDR		0x00
> > >   #define TCU_REG_WDT_TCER	0x04
> > >  @@ -53,4 +58,7 @@
> > >   #define TCU_REG_TCNTc(c)	(TCU_REG_TCNT0 + ((c) *
> > > TCU_CHANNEL_STRIDE))
> > >   #define TCU_REG_TCSRc(c)	(TCU_REG_TCSR0 + ((c) *
> > > TCU_CHANNEL_STRIDE))
> > > 
> > >  +struct regmap * __init ingenic_tcu_get_regmap(struct device_node
> > > *np);
> > >  +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned int
> > > channel);
> > >  +
> > >   #endif /* __LINUX_MFD_INGENIC_TCU_H_ */
> > 
> 
> 

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

Re: [PATCH v12 04/13] mfd: Add Ingenic TCU driver

[Paul Cercueil]

Le jeu. 27 juin 2019 à 8:58, Lee Jones <lee.jones@linaro.org> a écrit 
:
> On Wed, 26 Jun 2019, Paul Cercueil wrote:
>>  Le mer. 26 juin 2019 à 15:18, Lee Jones <lee.jones@linaro.org> a 
>> écrit :
>>  > On Tue, 21 May 2019, Paul Cercueil wrote:
>>  >
>>  > >  This driver will provide a regmap that can be retrieved very 
>> early
>>  > > in
>>  > >  the boot process through the API function 
>> ingenic_tcu_get_regmap().
>>  > >
>>  > >  Additionally, it will call devm_of_platform_populate() so that 
>> all
>>  > > the
>>  > >  children devices will be probed.
>>  > >
>>  > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  > >  ---
>>  > >
>>  > >  Notes:
>>  > >      v12: New patch
>>  > >
>>  > >   drivers/mfd/Kconfig             |   8 +++
>>  > >   drivers/mfd/Makefile            |   1 +
>>  > >   drivers/mfd/ingenic-tcu.c       | 113
>>  > > ++++++++++++++++++++++++++++++++
>>  > >   include/linux/mfd/ingenic-tcu.h |   8 +++
>>  > >   4 files changed, 130 insertions(+)
>>  > >   create mode 100644 drivers/mfd/ingenic-tcu.c
> 
> [...]
> 
>>  > >  +static struct regmap * __init ingenic_tcu_create_regmap(struct
>>  > > device_node *np)
>>  > >  +{
>>  > >  +	struct resource res;
>>  > >  +	void __iomem *base;
>>  > >  +	struct regmap *map;
>>  > >  +
>>  > >  +	if (!of_match_node(ingenic_tcu_of_match, np))
>>  > >  +		return ERR_PTR(-EINVAL);
> 
> Drop this check.
> 
>>  > >  +	base = of_io_request_and_map(np, 0, "TCU");
>>  > >  +	if (IS_ERR(base))
>>  > >  +		return ERR_PTR(PTR_ERR(base));
>>  > >  +
>>  > >  +	map = regmap_init_mmio(NULL, base, 
>> &ingenic_tcu_regmap_config);
>>  > >  +	if (IS_ERR(map))
>>  > >  +		goto err_iounmap;
> 
> Place this inside probe().
> 
>>  > >  +	return map;
>>  > >  +
>>  > >  +err_iounmap:
>>  > >  +	iounmap(base);
>>  > >  +	of_address_to_resource(np, 0, &res);
>>  > >  +	release_mem_region(res.start, resource_size(&res));
>>  > >  +
>>  > >  +	return map;
>>  > >  +}
>>  >
>>  > Why does this need to be set-up earlier than probe()?
>> 
>>  See the explanation below.
> 
> I think the answer is, it doesn't.
> 
>>  > >  +static int __init ingenic_tcu_probe(struct platform_device 
>> *pdev)
>>  > >  +{
>>  > >  +	struct regmap *map = 
>> ingenic_tcu_get_regmap(pdev->dev.of_node);
>>  > >  +
>>  > >  +	platform_set_drvdata(pdev, map);
>>  > >  +
>>  > >  +	regmap_attach_dev(&pdev->dev, map, 
>> &ingenic_tcu_regmap_config);
>>  > >  +
>>  > >  +	return devm_of_platform_populate(&pdev->dev);
>>  > >  +}
>>  > >  +
>>  > >  +static struct platform_driver ingenic_tcu_driver = {
>>  > >  +	.driver = {
>>  > >  +		.name = "ingenic-tcu",
>>  > >  +		.of_match_table = ingenic_tcu_of_match,
>>  > >  +	},
>>  > >  +};
>>  > >  +
>>  > >  +static int __init ingenic_tcu_platform_init(void)
>>  > >  +{
>>  > >  +	return platform_driver_probe(&ingenic_tcu_driver,
>>  > >  +				     ingenic_tcu_probe);
>>  >
>>  > What?  Why?
>> 
>>  The device driver probed here will populate the children devices,
>>  which will be able to retrieve the pointer to the regmap through
>>  device_get_regmap(dev->parent).
> 
> I've never heard of this call.  Where is it?

dev_get_regmap, in <linux/regmap.h>.

>>  The children devices are normal platform drivers that can be probed
>>  the normal way. These are the PWM driver, the watchdog driver, and 
>> the
>>  OST (OS Timer) clocksource driver, all part of the same hardware 
>> block
>>  (the Timer/Counter Unit or TCU).
> 
> If they are normal devices, then there is no need to roll your own
> regmap-getter implementation like this.
> 
>>  > >  +}
>>  > >  +subsys_initcall(ingenic_tcu_platform_init);
>>  > >  +
>>  > >  +struct regmap * __init ingenic_tcu_get_regmap(struct 
>> device_node
>>  > > *np)
>>  > >  +{
>>  > >  +	if (!tcu_regmap)
>>  > >  +		tcu_regmap = ingenic_tcu_create_regmap(np);
>>  > >  +
>>  > >  +	return tcu_regmap;
>>  > >  +}
>>  >
>>  > This makes me pretty uncomfortable.
>>  >
>>  > What calls it?
>> 
>>  The TCU IRQ driver (patch [06/13]), clocks driver (patch [05/13]), 
>> and the
>>  non-OST clocksource driver (patch [07/13]) all probe very early in 
>> the boot
>>  process, and share the same devicetree node. They call this 
>> function to get
>>  a pointer to the regmap.
> 
> Horrible!
> 
> Instead, you should send it through platform_set_drvdata() and collect
> it in the child drivers with platform_get_drvdata(dev->parent).

The IRQ, clocks and clocksource driver do NOT have a "struct device" to
begin with. They are not platform drivers, and cannot be platform 
drivers,
as they must register so early in the boot process, before "struct 
device"
is even a thing.

All they get is a pointer to the same devicetree node. Since all of 
these
have to use the same registers, they need to use a shared regmap, which
they obtain by calling ingenic_tcu_get_regmap() below.

Then, when this driver's probe gets called, the regmap is retrieved and
attached to the struct device, and then the children devices will be
probed: the watchdog device, the PWM device, the OST device. These three
will retrieve the regmap by calling dev_get_regmap(dev->parent, NULL).

>>  > >  +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned 
>> int
>>  > > channel)
>>  > >  +{
>>  > >  +	const struct ingenic_soc_info *soc =
>>  > > device_get_match_data(dev->parent);
>>  > >  +
>>  > >  +	/* Enable all TCU channels for PWM use by default except 
>> channels
>>  > > 0/1 */
>>  > >  +	u32 pwm_channels_mask = GENMASK(soc->num_channels - 1, 2);
>>  > >  +
>>  > >  +	device_property_read_u32(dev->parent, 
>> "ingenic,pwm-channels-mask",
>>  > >  +				 &pwm_channels_mask);
> 
> Doesn't this call overwrite the previous assignment above?

Yes, that's intended. You have a default value, that can be overriden
by a device property.

>>  > >  +	return !!(pwm_channels_mask & BIT(channel));
>>  > >  +}
>>  > >  +EXPORT_SYMBOL_GPL(ingenic_tcu_pwm_can_use_chn);
> 
> Where is this called from?

This is called from the PWM driver.

> I think this needs a review by the DT guys.

Rob already acked the bindings, which describe this property.

>>  > >  diff --git a/include/linux/mfd/ingenic-tcu.h
>>  > > b/include/linux/mfd/ingenic-tcu.h
>>  > >  index 2083fa20821d..21df23916cd2 100644
>>  > >  --- a/include/linux/mfd/ingenic-tcu.h
>>  > >  +++ b/include/linux/mfd/ingenic-tcu.h
>>  > >  @@ -6,6 +6,11 @@
>>  > >   #define __LINUX_MFD_INGENIC_TCU_H_
>>  > >
>>  > >   #include <linux/bitops.h>
>>  > >  +#include <linux/init.h>
>>  > >  +
>>  > >  +struct device;
>>  > >  +struct device_node;
>>  > >  +struct regmap;
>>  > >
>>  > >   #define TCU_REG_WDT_TDR		0x00
>>  > >   #define TCU_REG_WDT_TCER	0x04
>>  > >  @@ -53,4 +58,7 @@
>>  > >   #define TCU_REG_TCNTc(c)	(TCU_REG_TCNT0 + ((c) *
>>  > > TCU_CHANNEL_STRIDE))
>>  > >   #define TCU_REG_TCSRc(c)	(TCU_REG_TCSR0 + ((c) *
>>  > > TCU_CHANNEL_STRIDE))
>>  > >
>>  > >  +struct regmap * __init ingenic_tcu_get_regmap(struct 
>> device_node
>>  > > *np);
>>  > >  +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned 
>> int
>>  > > channel);
>>  > >  +
>>  > >   #endif /* __LINUX_MFD_INGENIC_TCU_H_ */
>>  >
>> 
>> 
> 
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v12 04/13] mfd: Add Ingenic TCU driver

[Lee Jones]

On Thu, 27 Jun 2019, Paul Cercueil wrote:
> Le jeu. 27 juin 2019 à 8:58, Lee Jones <lee.jones@linaro.org> a écrit :
> > On Wed, 26 Jun 2019, Paul Cercueil wrote:
> > >  Le mer. 26 juin 2019 à 15:18, Lee Jones <lee.jones@linaro.org> a
> > > écrit :
> > >  > On Tue, 21 May 2019, Paul Cercueil wrote:
> > >  >
> > >  > >  This driver will provide a regmap that can be retrieved very
> > > early
> > >  > > in
> > >  > >  the boot process through the API function
> > > ingenic_tcu_get_regmap().
> > >  > >
> > >  > >  Additionally, it will call devm_of_platform_populate() so that
> > > all
> > >  > > the
> > >  > >  children devices will be probed.
> > >  > >
> > >  > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > >  > >  ---
> > >  > >
> > >  > >  Notes:
> > >  > >      v12: New patch
> > >  > >
> > >  > >   drivers/mfd/Kconfig             |   8 +++
> > >  > >   drivers/mfd/Makefile            |   1 +
> > >  > >   drivers/mfd/ingenic-tcu.c       | 113
> > >  > > ++++++++++++++++++++++++++++++++
> > >  > >   include/linux/mfd/ingenic-tcu.h |   8 +++
> > >  > >   4 files changed, 130 insertions(+)
> > >  > >   create mode 100644 drivers/mfd/ingenic-tcu.c
> > 
> > [...]
> > 
> > >  > >  +static struct regmap * __init ingenic_tcu_create_regmap(struct
> > >  > > device_node *np)
> > >  > >  +{
> > >  > >  +	struct resource res;
> > >  > >  +	void __iomem *base;
> > >  > >  +	struct regmap *map;
> > >  > >  +
> > >  > >  +	if (!of_match_node(ingenic_tcu_of_match, np))
> > >  > >  +		return ERR_PTR(-EINVAL);
> > 
> > Drop this check.
> > 
> > >  > >  +	base = of_io_request_and_map(np, 0, "TCU");
> > >  > >  +	if (IS_ERR(base))
> > >  > >  +		return ERR_PTR(PTR_ERR(base));
> > >  > >  +
> > >  > >  +	map = regmap_init_mmio(NULL, base,
> > > &ingenic_tcu_regmap_config);
> > >  > >  +	if (IS_ERR(map))
> > >  > >  +		goto err_iounmap;
> > 
> > Place this inside probe().
> > 
> > >  > >  +	return map;
> > >  > >  +
> > >  > >  +err_iounmap:
> > >  > >  +	iounmap(base);
> > >  > >  +	of_address_to_resource(np, 0, &res);
> > >  > >  +	release_mem_region(res.start, resource_size(&res));
> > >  > >  +
> > >  > >  +	return map;
> > >  > >  +}
> > >  >
> > >  > Why does this need to be set-up earlier than probe()?
> > > 
> > >  See the explanation below.
> > 
> > I think the answer is, it doesn't.
> > 
> > >  > >  +static int __init ingenic_tcu_probe(struct platform_device
> > > *pdev)
> > >  > >  +{
> > >  > >  +	struct regmap *map =
> > > ingenic_tcu_get_regmap(pdev->dev.of_node);
> > >  > >  +
> > >  > >  +	platform_set_drvdata(pdev, map);
> > >  > >  +
> > >  > >  +	regmap_attach_dev(&pdev->dev, map,
> > > &ingenic_tcu_regmap_config);
> > >  > >  +
> > >  > >  +	return devm_of_platform_populate(&pdev->dev);
> > >  > >  +}
> > >  > >  +
> > >  > >  +static struct platform_driver ingenic_tcu_driver = {
> > >  > >  +	.driver = {
> > >  > >  +		.name = "ingenic-tcu",
> > >  > >  +		.of_match_table = ingenic_tcu_of_match,
> > >  > >  +	},
> > >  > >  +};
> > >  > >  +
> > >  > >  +static int __init ingenic_tcu_platform_init(void)
> > >  > >  +{
> > >  > >  +	return platform_driver_probe(&ingenic_tcu_driver,
> > >  > >  +				     ingenic_tcu_probe);
> > >  >
> > >  > What?  Why?
> > > 
> > >  The device driver probed here will populate the children devices,
> > >  which will be able to retrieve the pointer to the regmap through
> > >  device_get_regmap(dev->parent).
> > 
> > I've never heard of this call.  Where is it?
> 
> dev_get_regmap, in <linux/regmap.h>.
> 
> > >  The children devices are normal platform drivers that can be probed
> > >  the normal way. These are the PWM driver, the watchdog driver, and
> > > the
> > >  OST (OS Timer) clocksource driver, all part of the same hardware
> > > block
> > >  (the Timer/Counter Unit or TCU).
> > 
> > If they are normal devices, then there is no need to roll your own
> > regmap-getter implementation like this.
> > 
> > >  > >  +}
> > >  > >  +subsys_initcall(ingenic_tcu_platform_init);
> > >  > >  +
> > >  > >  +struct regmap * __init ingenic_tcu_get_regmap(struct
> > > device_node
> > >  > > *np)
> > >  > >  +{
> > >  > >  +	if (!tcu_regmap)
> > >  > >  +		tcu_regmap = ingenic_tcu_create_regmap(np);
> > >  > >  +
> > >  > >  +	return tcu_regmap;
> > >  > >  +}
> > >  >
> > >  > This makes me pretty uncomfortable.
> > >  >
> > >  > What calls it?
> > > 
> > >  The TCU IRQ driver (patch [06/13]), clocks driver (patch [05/13]),
> > > and the
> > >  non-OST clocksource driver (patch [07/13]) all probe very early in
> > > the boot
> > >  process, and share the same devicetree node. They call this
> > > function to get
> > >  a pointer to the regmap.
> > 
> > Horrible!
> > 
> > Instead, you should send it through platform_set_drvdata() and collect
> > it in the child drivers with platform_get_drvdata(dev->parent).
> 
> The IRQ, clocks and clocksource driver do NOT have a "struct device" to
> begin with. They are not platform drivers, and cannot be platform drivers,
> as they must register so early in the boot process, before "struct device"
> is even a thing.
> 
> All they get is a pointer to the same devicetree node. Since all of these
> have to use the same registers, they need to use a shared regmap, which
> they obtain by calling ingenic_tcu_get_regmap() below.
> 
> Then, when this driver's probe gets called, the regmap is retrieved and
> attached to the struct device, and then the children devices will be
> probed: the watchdog device, the PWM device, the OST device. These three
> will retrieve the regmap by calling dev_get_regmap(dev->parent, NULL).

That makes sense.

This explanation certainly belongs in the commit log.

Can you send your v14, as you intended.  I will re-review it with new
eyes when you do.

> > >  > >  +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, unsigned
> > > int
> > >  > > channel)
> > >  > >  +{
> > >  > >  +	const struct ingenic_soc_info *soc =
> > >  > > device_get_match_data(dev->parent);
> > >  > >  +
> > >  > >  +	/* Enable all TCU channels for PWM use by default except
> > > channels
> > >  > > 0/1 */
> > >  > >  +	u32 pwm_channels_mask = GENMASK(soc->num_channels - 1, 2);
> > >  > >  +
> > >  > >  +	device_property_read_u32(dev->parent,
> > > "ingenic,pwm-channels-mask",
> > >  > >  +				 &pwm_channels_mask);
> > 
> > Doesn't this call overwrite the previous assignment above?
> 
> Yes, that's intended. You have a default value, that can be overriden
> by a device property.

You should provide a comment here to make your intentions clear.

> > >  > >  +	return !!(pwm_channels_mask & BIT(channel));
> > >  > >  +}
> > >  > >  +EXPORT_SYMBOL_GPL(ingenic_tcu_pwm_can_use_chn);
> > 
> > Where is this called from?
> 
> This is called from the PWM driver.

Why can't it live in the PWM driver?

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

Re: [PATCH v12 04/13] mfd: Add Ingenic TCU driver

[Paul Cercueil]

Le jeu. 27 juin 2019 à 11:01, Lee Jones <lee.jones@linaro.org> a 
écrit :
> On Thu, 27 Jun 2019, Paul Cercueil wrote:
>>  Le jeu. 27 juin 2019 à 8:58, Lee Jones <lee.jones@linaro.org> a 
>> écrit :
>>  > On Wed, 26 Jun 2019, Paul Cercueil wrote:
>>  > >  Le mer. 26 juin 2019 à 15:18, Lee Jones 
>> <lee.jones@linaro.org> a
>>  > > écrit :
>>  > >  > On Tue, 21 May 2019, Paul Cercueil wrote:
>>  > >  >
>>  > >  > >  This driver will provide a regmap that can be retrieved 
>> very
>>  > > early
>>  > >  > > in
>>  > >  > >  the boot process through the API function
>>  > > ingenic_tcu_get_regmap().
>>  > >  > >
>>  > >  > >  Additionally, it will call devm_of_platform_populate() so 
>> that
>>  > > all
>>  > >  > > the
>>  > >  > >  children devices will be probed.
>>  > >  > >
>>  > >  > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  > >  > >  ---
>>  > >  > >
>>  > >  > >  Notes:
>>  > >  > >      v12: New patch
>>  > >  > >
>>  > >  > >   drivers/mfd/Kconfig             |   8 +++
>>  > >  > >   drivers/mfd/Makefile            |   1 +
>>  > >  > >   drivers/mfd/ingenic-tcu.c       | 113
>>  > >  > > ++++++++++++++++++++++++++++++++
>>  > >  > >   include/linux/mfd/ingenic-tcu.h |   8 +++
>>  > >  > >   4 files changed, 130 insertions(+)
>>  > >  > >   create mode 100644 drivers/mfd/ingenic-tcu.c
>>  >
>>  > [...]
>>  >
>>  > >  > >  +static struct regmap * __init 
>> ingenic_tcu_create_regmap(struct
>>  > >  > > device_node *np)
>>  > >  > >  +{
>>  > >  > >  +	struct resource res;
>>  > >  > >  +	void __iomem *base;
>>  > >  > >  +	struct regmap *map;
>>  > >  > >  +
>>  > >  > >  +	if (!of_match_node(ingenic_tcu_of_match, np))
>>  > >  > >  +		return ERR_PTR(-EINVAL);
>>  >
>>  > Drop this check.
>>  >
>>  > >  > >  +	base = of_io_request_and_map(np, 0, "TCU");
>>  > >  > >  +	if (IS_ERR(base))
>>  > >  > >  +		return ERR_PTR(PTR_ERR(base));
>>  > >  > >  +
>>  > >  > >  +	map = regmap_init_mmio(NULL, base,
>>  > > &ingenic_tcu_regmap_config);
>>  > >  > >  +	if (IS_ERR(map))
>>  > >  > >  +		goto err_iounmap;
>>  >
>>  > Place this inside probe().
>>  >
>>  > >  > >  +	return map;
>>  > >  > >  +
>>  > >  > >  +err_iounmap:
>>  > >  > >  +	iounmap(base);
>>  > >  > >  +	of_address_to_resource(np, 0, &res);
>>  > >  > >  +	release_mem_region(res.start, resource_size(&res));
>>  > >  > >  +
>>  > >  > >  +	return map;
>>  > >  > >  +}
>>  > >  >
>>  > >  > Why does this need to be set-up earlier than probe()?
>>  > >
>>  > >  See the explanation below.
>>  >
>>  > I think the answer is, it doesn't.
>>  >
>>  > >  > >  +static int __init ingenic_tcu_probe(struct 
>> platform_device
>>  > > *pdev)
>>  > >  > >  +{
>>  > >  > >  +	struct regmap *map =
>>  > > ingenic_tcu_get_regmap(pdev->dev.of_node);
>>  > >  > >  +
>>  > >  > >  +	platform_set_drvdata(pdev, map);
>>  > >  > >  +
>>  > >  > >  +	regmap_attach_dev(&pdev->dev, map,
>>  > > &ingenic_tcu_regmap_config);
>>  > >  > >  +
>>  > >  > >  +	return devm_of_platform_populate(&pdev->dev);
>>  > >  > >  +}
>>  > >  > >  +
>>  > >  > >  +static struct platform_driver ingenic_tcu_driver = {
>>  > >  > >  +	.driver = {
>>  > >  > >  +		.name = "ingenic-tcu",
>>  > >  > >  +		.of_match_table = ingenic_tcu_of_match,
>>  > >  > >  +	},
>>  > >  > >  +};
>>  > >  > >  +
>>  > >  > >  +static int __init ingenic_tcu_platform_init(void)
>>  > >  > >  +{
>>  > >  > >  +	return platform_driver_probe(&ingenic_tcu_driver,
>>  > >  > >  +				     ingenic_tcu_probe);
>>  > >  >
>>  > >  > What?  Why?
>>  > >
>>  > >  The device driver probed here will populate the children 
>> devices,
>>  > >  which will be able to retrieve the pointer to the regmap 
>> through
>>  > >  device_get_regmap(dev->parent).
>>  >
>>  > I've never heard of this call.  Where is it?
>> 
>>  dev_get_regmap, in <linux/regmap.h>.
>> 
>>  > >  The children devices are normal platform drivers that can be 
>> probed
>>  > >  the normal way. These are the PWM driver, the watchdog driver, 
>> and
>>  > > the
>>  > >  OST (OS Timer) clocksource driver, all part of the same 
>> hardware
>>  > > block
>>  > >  (the Timer/Counter Unit or TCU).
>>  >
>>  > If they are normal devices, then there is no need to roll your own
>>  > regmap-getter implementation like this.
>>  >
>>  > >  > >  +}
>>  > >  > >  +subsys_initcall(ingenic_tcu_platform_init);
>>  > >  > >  +
>>  > >  > >  +struct regmap * __init ingenic_tcu_get_regmap(struct
>>  > > device_node
>>  > >  > > *np)
>>  > >  > >  +{
>>  > >  > >  +	if (!tcu_regmap)
>>  > >  > >  +		tcu_regmap = ingenic_tcu_create_regmap(np);
>>  > >  > >  +
>>  > >  > >  +	return tcu_regmap;
>>  > >  > >  +}
>>  > >  >
>>  > >  > This makes me pretty uncomfortable.
>>  > >  >
>>  > >  > What calls it?
>>  > >
>>  > >  The TCU IRQ driver (patch [06/13]), clocks driver (patch 
>> [05/13]),
>>  > > and the
>>  > >  non-OST clocksource driver (patch [07/13]) all probe very 
>> early in
>>  > > the boot
>>  > >  process, and share the same devicetree node. They call this
>>  > > function to get
>>  > >  a pointer to the regmap.
>>  >
>>  > Horrible!
>>  >
>>  > Instead, you should send it through platform_set_drvdata() and 
>> collect
>>  > it in the child drivers with platform_get_drvdata(dev->parent).
>> 
>>  The IRQ, clocks and clocksource driver do NOT have a "struct 
>> device" to
>>  begin with. They are not platform drivers, and cannot be platform 
>> drivers,
>>  as they must register so early in the boot process, before "struct 
>> device"
>>  is even a thing.
>> 
>>  All they get is a pointer to the same devicetree node. Since all of 
>> these
>>  have to use the same registers, they need to use a shared regmap, 
>> which
>>  they obtain by calling ingenic_tcu_get_regmap() below.
>> 
>>  Then, when this driver's probe gets called, the regmap is retrieved 
>> and
>>  attached to the struct device, and then the children devices will be
>>  probed: the watchdog device, the PWM device, the OST device. These 
>> three
>>  will retrieve the regmap by calling dev_get_regmap(dev->parent, 
>> NULL).
> 
> That makes sense.
> 
> This explanation certainly belongs in the commit log.

Right.

> Can you send your v14, as you intended.  I will re-review it with new
> eyes when you do.

Could you review v13 instead? v14 will be a v13 with tiny teeny
non-code fixes (delete some newlines, replace %i with %d, and
convert the documentation from .txt to .rst).

>>  > >  > >  +bool ingenic_tcu_pwm_can_use_chn(struct device *dev, 
>> unsigned
>>  > > int
>>  > >  > > channel)
>>  > >  > >  +{
>>  > >  > >  +	const struct ingenic_soc_info *soc =
>>  > >  > > device_get_match_data(dev->parent);
>>  > >  > >  +
>>  > >  > >  +	/* Enable all TCU channels for PWM use by default except
>>  > > channels
>>  > >  > > 0/1 */
>>  > >  > >  +	u32 pwm_channels_mask = GENMASK(soc->num_channels - 1, 
>> 2);
>>  > >  > >  +
>>  > >  > >  +	device_property_read_u32(dev->parent,
>>  > > "ingenic,pwm-channels-mask",
>>  > >  > >  +				 &pwm_channels_mask);
>>  >
>>  > Doesn't this call overwrite the previous assignment above?
>> 
>>  Yes, that's intended. You have a default value, that can be 
>> overriden
>>  by a device property.
> 
> You should provide a comment here to make your intentions clear.

Ok.

>>  > >  > >  +	return !!(pwm_channels_mask & BIT(channel));
>>  > >  > >  +}
>>  > >  > >  +EXPORT_SYMBOL_GPL(ingenic_tcu_pwm_can_use_chn);
>>  >
>>  > Where is this called from?
>> 
>>  This is called from the PWM driver.
> 
> Why can't it live in the PWM driver?

It totally can. I'll move it there.

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

[PATCH v12 05/13] clk: ingenic: Add driver for the TCU clocks

[Paul Cercueil]

Add driver to support the clocks provided by the Timer/Counter Unit
(TCU) of the JZ47xx SoCs from Ingenic.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    v12: New patch

 drivers/clk/ingenic/Kconfig  |  11 +-
 drivers/clk/ingenic/Makefile |   1 +
 drivers/clk/ingenic/tcu.c    | 458 +++++++++++++++++++++++++++++++++++
 3 files changed, 469 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/ingenic/tcu.c

diff --git a/drivers/clk/ingenic/Kconfig b/drivers/clk/ingenic/Kconfig
index 34dc0da79c39..434893133eb4 100644
--- a/drivers/clk/ingenic/Kconfig
+++ b/drivers/clk/ingenic/Kconfig
@@ -1,4 +1,4 @@
-menu "Ingenic JZ47xx CGU drivers"
+menu "Ingenic JZ47xx drivers"
 	depends on MIPS
 
 config INGENIC_CGU_COMMON
@@ -44,4 +44,13 @@ config INGENIC_CGU_JZ4780
 
 	  If building for a JZ4780 SoC, you want to say Y here.
 
+config INGENIC_TCU_CLK
+	bool "Ingenic JZ47xx TCU clocks driver"
+	default MACH_INGENIC
+	depends on COMMON_CLK
+	select INGENIC_TCU
+	help
+	  Support the clocks of the Timer/Counter Unit (TCU) of the Ingenic
+	  JZ47xx SoCs.
+
 endmenu
diff --git a/drivers/clk/ingenic/Makefile b/drivers/clk/ingenic/Makefile
index 00a79b2fba10..9ef8551926b7 100644
--- a/drivers/clk/ingenic/Makefile
+++ b/drivers/clk/ingenic/Makefile
@@ -3,3 +3,4 @@ obj-$(CONFIG_INGENIC_CGU_JZ4740)	+= jz4740-cgu.o
 obj-$(CONFIG_INGENIC_CGU_JZ4725B)	+= jz4725b-cgu.o
 obj-$(CONFIG_INGENIC_CGU_JZ4770)	+= jz4770-cgu.o
 obj-$(CONFIG_INGENIC_CGU_JZ4780)	+= jz4780-cgu.o
+obj-$(CONFIG_INGENIC_TCU_CLK)		+= tcu.o
diff --git a/drivers/clk/ingenic/tcu.c b/drivers/clk/ingenic/tcu.c
new file mode 100644
index 000000000000..7249225a6994
--- /dev/null
+++ b/drivers/clk/ingenic/tcu.c
@@ -0,0 +1,458 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * JZ47xx SoCs TCU clocks driver
+ * Copyright (C) 2019 Paul Cercueil <paul@crapouillou.net>
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+#include <linux/clockchips.h>
+#include <linux/mfd/ingenic-tcu.h>
+#include <linux/regmap.h>
+
+#include <dt-bindings/clock/ingenic,tcu.h>
+
+/* 8 channels max + watchdog + OST */
+#define TCU_CLK_COUNT	10
+
+#define TCU_ERR(...) pr_crit("ingenic-tcu-clk: " __VA_ARGS__)
+
+enum tcu_clk_parent {
+	TCU_PARENT_PCLK,
+	TCU_PARENT_RTC,
+	TCU_PARENT_EXT,
+};
+
+struct ingenic_soc_info {
+	unsigned int num_channels;
+	bool has_ost;
+	bool has_tcu_clk;
+};
+
+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;
+	unsigned int idx;
+	struct ingenic_tcu *tcu;
+	const struct ingenic_tcu_clk_info *info;
+};
+
+struct ingenic_tcu {
+	const struct ingenic_soc_info *soc_info;
+	struct regmap *map;
+	struct clk *clk;
+
+	struct clk_hw_onecell_data *clocks;
+};
+
+static inline struct ingenic_tcu_clk *to_tcu_clk(struct clk_hw *hw)
+{
+	return 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;
+	struct ingenic_tcu *tcu = tcu_clk->tcu;
+	int ret;
+
+	if (tcu->clk) {
+		ret = clk_prepare_enable(tcu->clk);
+		if (ret)
+			return ret;
+	}
+
+	regmap_write(tcu->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;
+	struct ingenic_tcu *tcu = tcu_clk->tcu;
+
+	regmap_write(tcu->map, TCU_REG_TSSR, BIT(info->gate_bit));
+
+	if (tcu->clk)
+		clk_disable_unprepare(tcu->clk);
+}
+
+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->tcu->map, TCU_REG_TSR, &value);
+
+	return !(value & BIT(info->gate_bit));
+}
+
+static bool ingenic_tcu_enable_regs(struct clk_hw *hw)
+{
+	struct ingenic_tcu_clk *tcu_clk = to_tcu_clk(hw);
+	const struct ingenic_tcu_clk_info *info = tcu_clk->info;
+	struct ingenic_tcu *tcu = tcu_clk->tcu;
+	bool enabled = false;
+
+	/*
+	 * If the SoC has no global TCU clock, we must ungate the channel's
+	 * clock to be able to access its registers.
+	 * If we have a TCU clock, it will be enabled automatically as it has
+	 * been attached to the regmap.
+	 */
+	if (!tcu->clk) {
+		enabled = !!ingenic_tcu_is_enabled(hw);
+		regmap_write(tcu->map, TCU_REG_TSCR, BIT(info->gate_bit));
+	}
+
+	return enabled;
+}
+
+static void ingenic_tcu_disable_regs(struct clk_hw *hw)
+{
+	struct ingenic_tcu_clk *tcu_clk = to_tcu_clk(hw);
+	const struct ingenic_tcu_clk_info *info = tcu_clk->info;
+	struct ingenic_tcu *tcu = tcu_clk->tcu;
+
+	if (!tcu->clk)
+		regmap_write(tcu->map, TCU_REG_TSSR, 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->tcu->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;
+	bool was_enabled;
+	int ret;
+
+	was_enabled = ingenic_tcu_enable_regs(hw);
+
+	ret = regmap_update_bits(tcu_clk->tcu->map, info->tcsr_reg,
+				 TCU_TCSR_PARENT_CLOCK_MASK, BIT(idx));
+	WARN_ONCE(ret < 0, "Unable to update TCSR %i", tcu_clk->idx);
+
+	if (!was_enabled)
+		ingenic_tcu_disable_regs(hw);
+
+	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->tcu->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 u8 ingenic_tcu_get_prescale(unsigned long rate, unsigned long req_rate)
+{
+	u8 prescale;
+
+	for (prescale = 0; prescale < 5; prescale++)
+		if ((rate >> (prescale * 2)) <= req_rate)
+			return prescale;
+
+	return 5; /* /1024 divider */
+}
+
+static long ingenic_tcu_round_rate(struct clk_hw *hw, unsigned long req_rate,
+		unsigned long *parent_rate)
+{
+	unsigned long rate = *parent_rate;
+	u8 prescale;
+
+	if (req_rate > rate)
+		return -EINVAL;
+
+	prescale = ingenic_tcu_get_prescale(rate, req_rate);
+
+	return rate >> (prescale * 2);
+}
+
+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;
+	u8 prescale = ingenic_tcu_get_prescale(parent_rate, req_rate);
+	bool was_enabled;
+	int ret;
+
+	was_enabled = ingenic_tcu_enable_regs(hw);
+
+	ret = regmap_update_bits(tcu_clk->tcu->map, info->tcsr_reg,
+				 TCU_TCSR_PRESCALE_MASK,
+				 prescale << TCU_TCSR_PRESCALE_LSB);
+	WARN_ONCE(ret < 0, "Unable to update TCSR %i", tcu_clk->idx);
+
+	if (!was_enabled)
+		ingenic_tcu_disable_regs(hw);
+
+	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[] = {
+	[TCU_PARENT_PCLK] = "pclk",
+	[TCU_PARENT_RTC]  = "rtc",
+	[TCU_PARENT_EXT]  = "ext",
+};
+
+#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_UNGATE,			\
+		},							\
+		.gate_bit = _gate_bit,					\
+		.tcsr_reg = _tcsr,					\
+	}
+static const struct ingenic_tcu_clk_info ingenic_tcu_clk_info[] = {
+	[TCU_CLK_TIMER0] = DEF_TIMER("timer0", 0, TCU_REG_TCSRc(0)),
+	[TCU_CLK_TIMER1] = DEF_TIMER("timer1", 1, TCU_REG_TCSRc(1)),
+	[TCU_CLK_TIMER2] = DEF_TIMER("timer2", 2, TCU_REG_TCSRc(2)),
+	[TCU_CLK_TIMER3] = DEF_TIMER("timer3", 3, TCU_REG_TCSRc(3)),
+	[TCU_CLK_TIMER4] = DEF_TIMER("timer4", 4, TCU_REG_TCSRc(4)),
+	[TCU_CLK_TIMER5] = DEF_TIMER("timer5", 5, TCU_REG_TCSRc(5)),
+	[TCU_CLK_TIMER6] = DEF_TIMER("timer6", 6, TCU_REG_TCSRc(6)),
+	[TCU_CLK_TIMER7] = DEF_TIMER("timer7", 7, TCU_REG_TCSRc(7)),
+};
+
+static const struct ingenic_tcu_clk_info ingenic_tcu_watchdog_clk_info =
+					 DEF_TIMER("wdt", 16, TCU_REG_WDT_TCSR);
+static const struct ingenic_tcu_clk_info ingenic_tcu_ost_clk_info =
+					 DEF_TIMER("ost", 15, TCU_REG_OST_TCSR);
+#undef DEF_TIMER
+
+static int __init ingenic_tcu_register_clock(struct ingenic_tcu *tcu,
+			unsigned int idx, enum tcu_clk_parent parent,
+			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->tcu = tcu;
+
+	/* Reset channel and clock divider, set default parent */
+	ingenic_tcu_enable_regs(&tcu_clk->hw);
+	regmap_update_bits(tcu->map, info->tcsr_reg, 0xffff, BIT(parent));
+	ingenic_tcu_disable_regs(&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 const struct ingenic_soc_info jz4740_soc_info = {
+	.num_channels = 8,
+	.has_ost = false,
+	.has_tcu_clk = true,
+};
+
+static const struct ingenic_soc_info jz4725b_soc_info = {
+	.num_channels = 6,
+	.has_ost = true,
+	.has_tcu_clk = true,
+};
+
+static const struct ingenic_soc_info jz4770_soc_info = {
+	.num_channels = 8,
+	.has_ost = true,
+	.has_tcu_clk = false,
+};
+
+static const struct of_device_id ingenic_tcu_of_match[] __initconst = {
+	{ .compatible = "ingenic,jz4740-tcu", .data = &jz4740_soc_info, },
+	{ .compatible = "ingenic,jz4725b-tcu", .data = &jz4725b_soc_info, },
+	{ .compatible = "ingenic,jz4770-tcu", .data = &jz4770_soc_info, },
+	{ }
+};
+
+static int __init ingenic_tcu_probe(struct device_node *np)
+{
+	const struct of_device_id *id = of_match_node(ingenic_tcu_of_match, np);
+	struct ingenic_tcu *tcu;
+	struct regmap *map;
+	unsigned int i;
+	int ret;
+
+	map = ingenic_tcu_get_regmap(np);
+	if (IS_ERR(map))
+		return PTR_ERR(map);
+
+	tcu = kzalloc(sizeof(*tcu), GFP_KERNEL);
+	if (!tcu)
+		return -ENOMEM;
+
+	tcu->map = map;
+	tcu->soc_info = id->data;
+
+	if (tcu->soc_info->has_tcu_clk) {
+		tcu->clk = of_clk_get_by_name(np, "tcu");
+		if (IS_ERR(tcu->clk)) {
+			ret = PTR_ERR(tcu->clk);
+			TCU_ERR("Cannot get TCU clock\n");
+			goto err_free_tcu;
+		}
+
+		ret = regmap_mmio_attach_clk(map, tcu->clk);
+		if (ret) {
+			TCU_ERR("Unable to attach TCU clock\n");
+			goto err_put_clk;
+		}
+	}
+
+	tcu->clocks = kzalloc(sizeof(*tcu->clocks) +
+			      sizeof(*tcu->clocks->hws) * TCU_CLK_COUNT,
+			      GFP_KERNEL);
+	if (!tcu->clocks) {
+		ret = -ENOMEM;
+		goto err_clk_detach;
+	}
+
+	tcu->clocks->num = TCU_CLK_COUNT;
+
+	for (i = 0; i < tcu->soc_info->num_channels; i++) {
+		ret = ingenic_tcu_register_clock(tcu, i, TCU_PARENT_EXT,
+						 &ingenic_tcu_clk_info[i],
+						 tcu->clocks);
+		if (ret) {
+			TCU_ERR("cannot register clock %i\n", i);
+			goto err_unregister_timer_clocks;
+		}
+	}
+
+	/* We set EXT as the default parent clock for all the TCU clocks
+	 * except for the watchdog one, where we set the RTC clock as the
+	 * parent. Since the EXT and PCLK are much faster than the RTC clock,
+	 * the watchdog would kick after a maximum time of 5s, and we might
+	 * want a slower kicking time.
+	 */
+	ret = ingenic_tcu_register_clock(tcu, TCU_CLK_WDT, TCU_PARENT_RTC,
+					 &ingenic_tcu_watchdog_clk_info,
+					 tcu->clocks);
+	if (ret) {
+		TCU_ERR("cannot register watchdog clock\n");
+		goto err_unregister_timer_clocks;
+	}
+
+	if (tcu->soc_info->has_ost) {
+		ret = ingenic_tcu_register_clock(tcu, TCU_CLK_OST,
+						 TCU_PARENT_EXT,
+						 &ingenic_tcu_ost_clk_info,
+						 tcu->clocks);
+		if (ret) {
+			TCU_ERR("cannot register ost clock\n");
+			goto err_unregister_watchdog_clock;
+		}
+	}
+
+	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, tcu->clocks);
+	if (ret) {
+		TCU_ERR("cannot add OF clock provider\n");
+		goto err_unregister_ost_clock;
+	}
+
+	return 0;
+
+err_unregister_ost_clock:
+	if (tcu->soc_info->has_ost)
+		clk_hw_unregister(tcu->clocks->hws[i + 1]);
+err_unregister_watchdog_clock:
+	clk_hw_unregister(tcu->clocks->hws[i]);
+err_unregister_timer_clocks:
+	for (i = 0; i < tcu->clocks->num; i++)
+		if (tcu->clocks->hws[i])
+			clk_hw_unregister(tcu->clocks->hws[i]);
+	kfree(tcu->clocks);
+err_clk_detach:
+	if (tcu->soc_info->has_tcu_clk)
+		regmap_mmio_detach_clk(map);
+err_put_clk:
+	if (tcu->soc_info->has_tcu_clk)
+		clk_put(tcu->clk);
+err_free_tcu:
+	kfree(tcu);
+	return ret;
+}
+
+static void __init ingenic_tcu_init(struct device_node *np)
+{
+	int ret = ingenic_tcu_probe(np);
+
+	if (ret)
+		TCU_ERR("Failed to initialize TCU clocks: %i\n", ret);
+}
+
+CLK_OF_DECLARE(jz4740_cgu, "ingenic,jz4740-tcu", ingenic_tcu_init);
+CLK_OF_DECLARE(jz4725b_cgu, "ingenic,jz4725b-tcu", ingenic_tcu_init);
+CLK_OF_DECLARE(jz4770_cgu, "ingenic,jz4770-tcu", ingenic_tcu_init);
-- 
2.21.0.593.g511ec345e18

Re: [PATCH v12 05/13] clk: ingenic: Add driver for the TCU clocks

[Stephen Boyd]

Quoting Paul Cercueil (2019-05-21 07:51:33)
> diff --git a/drivers/clk/ingenic/Kconfig b/drivers/clk/ingenic/Kconfig
> index 34dc0da79c39..434893133eb4 100644
> --- a/drivers/clk/ingenic/Kconfig
> +++ b/drivers/clk/ingenic/Kconfig
> @@ -1,4 +1,4 @@
> -menu "Ingenic JZ47xx CGU drivers"
> +menu "Ingenic JZ47xx drivers"
>         depends on MIPS
>  
>  config INGENIC_CGU_COMMON
> @@ -44,4 +44,13 @@ config INGENIC_CGU_JZ4780
>  
>           If building for a JZ4780 SoC, you want to say Y here.
>  
> +config INGENIC_TCU_CLK
> +       bool "Ingenic JZ47xx TCU clocks driver"
> +       default MACH_INGENIC
> +       depends on COMMON_CLK

Does the INGENIC_TCU_CLK config even exist if COMMON_CLK is disabled? I
suspect it's all part of the menuconfig so this depends is not useful?

> +       select INGENIC_TCU
> +       help
> +         Support the clocks of the Timer/Counter Unit (TCU) of the Ingenic
> +         JZ47xx SoCs.
> +
>  endmenu
> diff --git a/drivers/clk/ingenic/tcu.c b/drivers/clk/ingenic/tcu.c
> new file mode 100644
> index 000000000000..7249225a6994
> --- /dev/null
> +++ b/drivers/clk/ingenic/tcu.c
> @@ -0,0 +1,458 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * JZ47xx SoCs TCU clocks driver
> + * Copyright (C) 2019 Paul Cercueil <paul@crapouillou.net>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/clkdev.h>
> +#include <linux/clockchips.h>
> +#include <linux/mfd/ingenic-tcu.h>
> +#include <linux/regmap.h>
> +
> +#include <dt-bindings/clock/ingenic,tcu.h>
> +
> +/* 8 channels max + watchdog + OST */
> +#define TCU_CLK_COUNT  10
> +
> +#define TCU_ERR(...) pr_crit("ingenic-tcu-clk: " __VA_ARGS__)

Why is it pr_crit instead of pr_err()?

> +
> +enum tcu_clk_parent {
> +       TCU_PARENT_PCLK,
> +       TCU_PARENT_RTC,
> +       TCU_PARENT_EXT,
> +};
> +
[...]
> +
> +static int __init ingenic_tcu_register_clock(struct ingenic_tcu *tcu,
> +                       unsigned int idx, enum tcu_clk_parent parent,
> +                       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->tcu = tcu;
> +
> +       /* Reset channel and clock divider, set default parent */
> +       ingenic_tcu_enable_regs(&tcu_clk->hw);
> +       regmap_update_bits(tcu->map, info->tcsr_reg, 0xffff, BIT(parent));
> +       ingenic_tcu_disable_regs(&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);

Do you have a use for clkdev? If DT lookups work just as well it would
be better to skip clkdev registration.

> +       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 const struct ingenic_soc_info jz4740_soc_info = {
> +       .num_channels = 8,
> +       .has_ost = false,
> +       .has_tcu_clk = true,
> +};
> +
> +static const struct ingenic_soc_info jz4725b_soc_info = {
> +       .num_channels = 6,
> +       .has_ost = true,
> +       .has_tcu_clk = true,
> +};
> +
> +static const struct ingenic_soc_info jz4770_soc_info = {
> +       .num_channels = 8,
> +       .has_ost = true,
> +       .has_tcu_clk = false,
> +};
> +
> +static const struct of_device_id ingenic_tcu_of_match[] __initconst = {
> +       { .compatible = "ingenic,jz4740-tcu", .data = &jz4740_soc_info, },
> +       { .compatible = "ingenic,jz4725b-tcu", .data = &jz4725b_soc_info, },
> +       { .compatible = "ingenic,jz4770-tcu", .data = &jz4770_soc_info, },
> +       { }
> +};
> +
> +static int __init ingenic_tcu_probe(struct device_node *np)
> +{
> +       const struct of_device_id *id = of_match_node(ingenic_tcu_of_match, np);
> +       struct ingenic_tcu *tcu;
> +       struct regmap *map;
> +       unsigned int i;
> +       int ret;
> +
> +       map = ingenic_tcu_get_regmap(np);
> +       if (IS_ERR(map))
> +               return PTR_ERR(map);
> +
> +       tcu = kzalloc(sizeof(*tcu), GFP_KERNEL);
> +       if (!tcu)
> +               return -ENOMEM;
> +
> +       tcu->map = map;
> +       tcu->soc_info = id->data;
> +
> +       if (tcu->soc_info->has_tcu_clk) {
> +               tcu->clk = of_clk_get_by_name(np, "tcu");

Is this clk necessary to read/write registers in this clk driver? And
this clk isn't the parent of the clks? Why is it managed by Linux at
all? Will there be a time when it's turned off?

I'm asking because it looks like we're calling clk APIs from within clk
provider implementation. That works right now because of our locking
scheme, but this will put up another roadblock to making the prepare and
enable locks not recursive. I've seen some drivers take an approach of
enabling the clk when the provider is PM runtime active, and disable the
clk when the provider is runtime PM inactive. This gets it out of the
provider path and into the runtime PM path. If you take that approach
then when we move the runtime PM code in clk core outside of the prepare
lock we should be able to avoid any recursive locking scenarios.

> +               if (IS_ERR(tcu->clk)) {
> +                       ret = PTR_ERR(tcu->clk);
> +                       TCU_ERR("Cannot get TCU clock\n");
> +                       goto err_free_tcu;
> +               }
> +

Re: [PATCH v12 05/13] clk: ingenic: Add driver for the TCU clocks

[Paul Cercueil]

Hi Stephen, thanks for the review.

Le ven. 7 juin 2019 à 23:28, Stephen Boyd <sboyd@kernel.org> a écrit :
> Quoting Paul Cercueil (2019-05-21 07:51:33)
>>  diff --git a/drivers/clk/ingenic/Kconfig 
>> b/drivers/clk/ingenic/Kconfig
>>  index 34dc0da79c39..434893133eb4 100644
>>  --- a/drivers/clk/ingenic/Kconfig
>>  +++ b/drivers/clk/ingenic/Kconfig
>>  @@ -1,4 +1,4 @@
>>  -menu "Ingenic JZ47xx CGU drivers"
>>  +menu "Ingenic JZ47xx drivers"
>>          depends on MIPS
>> 
>>   config INGENIC_CGU_COMMON
>>  @@ -44,4 +44,13 @@ config INGENIC_CGU_JZ4780
>> 
>>            If building for a JZ4780 SoC, you want to say Y here.
>> 
>>  +config INGENIC_TCU_CLK
>>  +       bool "Ingenic JZ47xx TCU clocks driver"
>>  +       default MACH_INGENIC
>>  +       depends on COMMON_CLK
> 
> Does the INGENIC_TCU_CLK config even exist if COMMON_CLK is disabled? 
> I
> suspect it's all part of the menuconfig so this depends is not useful?

Right, it can be dropped.

>>  +       select INGENIC_TCU
>>  +       help
>>  +         Support the clocks of the Timer/Counter Unit (TCU) of the 
>> Ingenic
>>  +         JZ47xx SoCs.
>>  +
>>   endmenu
>>  diff --git a/drivers/clk/ingenic/tcu.c b/drivers/clk/ingenic/tcu.c
>>  new file mode 100644
>>  index 000000000000..7249225a6994
>>  --- /dev/null
>>  +++ b/drivers/clk/ingenic/tcu.c
>>  @@ -0,0 +1,458 @@
>>  +// SPDX-License-Identifier: GPL-2.0
>>  +/*
>>  + * JZ47xx SoCs TCU clocks driver
>>  + * Copyright (C) 2019 Paul Cercueil <paul@crapouillou.net>
>>  + */
>>  +
>>  +#include <linux/clk.h>
>>  +#include <linux/clk-provider.h>
>>  +#include <linux/clkdev.h>
>>  +#include <linux/clockchips.h>
>>  +#include <linux/mfd/ingenic-tcu.h>
>>  +#include <linux/regmap.h>
>>  +
>>  +#include <dt-bindings/clock/ingenic,tcu.h>
>>  +
>>  +/* 8 channels max + watchdog + OST */
>>  +#define TCU_CLK_COUNT  10
>>  +
>>  +#define TCU_ERR(...) pr_crit("ingenic-tcu-clk: " __VA_ARGS__)
> 
> Why is it pr_crit instead of pr_err()?

If the TCU timer clocks are not provided for any reason, the system
will have no timer, and the kernel will hang very early in the init
process. That's why I chose pr_crit().

>>  +
>>  +enum tcu_clk_parent {
>>  +       TCU_PARENT_PCLK,
>>  +       TCU_PARENT_RTC,
>>  +       TCU_PARENT_EXT,
>>  +};
>>  +
> [...]
>>  +
>>  +static int __init ingenic_tcu_register_clock(struct ingenic_tcu 
>> *tcu,
>>  +                       unsigned int idx, enum tcu_clk_parent 
>> parent,
>>  +                       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->tcu = tcu;
>>  +
>>  +       /* Reset channel and clock divider, set default parent */
>>  +       ingenic_tcu_enable_regs(&tcu_clk->hw);
>>  +       regmap_update_bits(tcu->map, info->tcsr_reg, 0xffff, 
>> BIT(parent));
>>  +       ingenic_tcu_disable_regs(&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);
> 
> Do you have a use for clkdev? If DT lookups work just as well it would
> be better to skip clkdev registration.

OK.

>>  +       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 const struct ingenic_soc_info jz4740_soc_info = {
>>  +       .num_channels = 8,
>>  +       .has_ost = false,
>>  +       .has_tcu_clk = true,
>>  +};
>>  +
>>  +static const struct ingenic_soc_info jz4725b_soc_info = {
>>  +       .num_channels = 6,
>>  +       .has_ost = true,
>>  +       .has_tcu_clk = true,
>>  +};
>>  +
>>  +static const struct ingenic_soc_info jz4770_soc_info = {
>>  +       .num_channels = 8,
>>  +       .has_ost = true,
>>  +       .has_tcu_clk = false,
>>  +};
>>  +
>>  +static const struct of_device_id ingenic_tcu_of_match[] 
>> __initconst = {
>>  +       { .compatible = "ingenic,jz4740-tcu", .data = 
>> &jz4740_soc_info, },
>>  +       { .compatible = "ingenic,jz4725b-tcu", .data = 
>> &jz4725b_soc_info, },
>>  +       { .compatible = "ingenic,jz4770-tcu", .data = 
>> &jz4770_soc_info, },
>>  +       { }
>>  +};
>>  +
>>  +static int __init ingenic_tcu_probe(struct device_node *np)
>>  +{
>>  +       const struct of_device_id *id = 
>> of_match_node(ingenic_tcu_of_match, np);
>>  +       struct ingenic_tcu *tcu;
>>  +       struct regmap *map;
>>  +       unsigned int i;
>>  +       int ret;
>>  +
>>  +       map = ingenic_tcu_get_regmap(np);
>>  +       if (IS_ERR(map))
>>  +               return PTR_ERR(map);
>>  +
>>  +       tcu = kzalloc(sizeof(*tcu), GFP_KERNEL);
>>  +       if (!tcu)
>>  +               return -ENOMEM;
>>  +
>>  +       tcu->map = map;
>>  +       tcu->soc_info = id->data;
>>  +
>>  +       if (tcu->soc_info->has_tcu_clk) {
>>  +               tcu->clk = of_clk_get_by_name(np, "tcu");
> 
> Is this clk necessary to read/write registers in this clk driver? And
> this clk isn't the parent of the clks? Why is it managed by Linux at
> all? Will there be a time when it's turned off?

For the SoCs which have the "tcu" clock, it has to be enabled for the
registers to be accessible, yes. And as you noticed, it is not the
parent of the timer clocks.

The "tcu" clock can be turned off during suspend, for instance.

> I'm asking because it looks like we're calling clk APIs from within 
> clk
> provider implementation. That works right now because of our locking
> scheme, but this will put up another roadblock to making the prepare 
> and
> enable locks not recursive. I've seen some drivers take an approach of
> enabling the clk when the provider is PM runtime active, and disable 
> the
> clk when the provider is runtime PM inactive. This gets it out of the
> provider path and into the runtime PM path. If you take that approach
> then when we move the runtime PM code in clk core outside of the 
> prepare
> lock we should be able to avoid any recursive locking scenarios.

Most of the code here works without a struct device, it wouldn't be 
easy to
get it to work with runtime PM.

I can enable the "tcu" clock in the probe and just gate/ungate it in the
suspend/resume callbacks, that would work just fine. We don't need 
anything
fancy here.

>>  +               if (IS_ERR(tcu->clk)) {
>>  +                       ret = PTR_ERR(tcu->clk);
>>  +                       TCU_ERR("Cannot get TCU clock\n");
>>  +                       goto err_free_tcu;
>>  +               }
>>  +

Re: [PATCH v12 05/13] clk: ingenic: Add driver for the TCU clocks

[Stephen Boyd]

Quoting Paul Cercueil (2019-06-07 14:59:54)
> Hi Stephen, thanks for the review.
> > Quoting Paul Cercueil (2019-05-21 07:51:33)
> >>  diff --git a/drivers/clk/ingenic/tcu.c b/drivers/clk/ingenic/tcu.c
> >>  new file mode 100644
> >>  index 000000000000..7249225a6994
> >>  --- /dev/null
> >>  +++ b/drivers/clk/ingenic/tcu.c
> >>  @@ -0,0 +1,458 @@
> >>  +// SPDX-License-Identifier: GPL-2.0
> >>  +/*
> >>  + * JZ47xx SoCs TCU clocks driver
> >>  + * Copyright (C) 2019 Paul Cercueil <paul@crapouillou.net>
> >>  + */
> >>  +
> >>  +#include <linux/clk.h>
> >>  +#include <linux/clk-provider.h>
> >>  +#include <linux/clkdev.h>
> >>  +#include <linux/clockchips.h>
> >>  +#include <linux/mfd/ingenic-tcu.h>
> >>  +#include <linux/regmap.h>
> >>  +
> >>  +#include <dt-bindings/clock/ingenic,tcu.h>
> >>  +
> >>  +/* 8 channels max + watchdog + OST */
> >>  +#define TCU_CLK_COUNT  10
> >>  +
> >>  +#define TCU_ERR(...) pr_crit("ingenic-tcu-clk: " __VA_ARGS__)
> > 
> > Why is it pr_crit instead of pr_err()?
> 
> If the TCU timer clocks are not provided for any reason, the system
> will have no timer, and the kernel will hang very early in the init
> process. That's why I chose pr_crit().

HMm. So maybe it should be TCU_CRIT() then? Or just drop the wrapper
macro and define a pr_fmt for this file that has ingenic-tcu-clk: for
it?

> 
> Most of the code here works without a struct device, it wouldn't be 
> easy to
> get it to work with runtime PM.
> 
> I can enable the "tcu" clock in the probe and just gate/ungate it in the
> suspend/resume callbacks, that would work just fine. We don't need 
> anything
> fancy here.

OK. That sounds like a better approach to gate and ungate in
suspend/resume.

[PATCH v12 06/13] irqchip: Add irq-ingenic-tcu driver

[Paul Cercueil]

This driver handles the interrupt controller built in the Timer/Counter
Unit (TCU) of the JZ47xx SoCs from Ingenic.

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

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 1c1f3f66dfd3..2d0700e52db7 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -293,6 +293,17 @@ config INGENIC_IRQ
 	depends on MACH_INGENIC
 	default y
 
+config INGENIC_TCU_IRQ
+	bool "Ingenic JZ47xx TCU interrupt controller"
+	default MACH_INGENIC
+	depends on MIPS || COMPILE_TEST
+	select INGENIC_TCU
+	help
+	  Support for interrupts in the Timer/Counter Unit (TCU) of the Ingenic
+	  JZ47xx SoCs.
+
+	  If unsure, say N.
+
 config RENESAS_H8300H_INTC
         bool
 	select IRQ_DOMAIN
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 606a003a0000..f403b2c221e4 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -73,6 +73,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_MSCC_OCELOT_IRQ)		+= irq-mscc-ocelot.o
diff --git a/drivers/irqchip/irq-ingenic-tcu.c b/drivers/irqchip/irq-ingenic-tcu.c
new file mode 100644
index 000000000000..738ed06c983f
--- /dev/null
+++ b/drivers/irqchip/irq-ingenic-tcu.c
@@ -0,0 +1,182 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * JZ47xx SoCs TCU IRQ driver
+ * Copyright (C) 2019 Paul Cercueil <paul@crapouillou.net>
+ */
+
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/mfd/ingenic-tcu.h>
+#include <linux/of_irq.h>
+#include <linux/regmap.h>
+
+struct ingenic_tcu {
+	struct regmap *map;
+	struct clk *clk;
+
+	struct irq_domain *domain;
+	unsigned int nb_parent_irqs;
+	u32 parent_irqs[3];
+};
+
+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_each_set_bit(i, (unsigned long *)&irq_reg, 32)
+		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_irq_init(struct device_node *np,
+				       struct device_node *parent)
+{
+	struct irq_chip_generic *gc;
+	struct irq_chip_type *ct;
+	struct ingenic_tcu *tcu;
+	struct regmap *map;
+	unsigned int i;
+	int ret, irqs;
+
+	map = ingenic_tcu_get_regmap(np);
+	if (IS_ERR(map))
+		return PTR_ERR(map);
+
+	tcu = kzalloc(sizeof(*tcu), GFP_KERNEL);
+	if (!tcu)
+		return -ENOMEM;
+
+	tcu->map = map;
+
+	irqs = of_property_count_elems_of_size(np, "interrupts", sizeof(u32));
+	if (irqs < 0 || irqs > ARRAY_SIZE(tcu->parent_irqs)) {
+		pr_crit("%s: Invalid 'interrupts' property\n", __func__);
+		ret = -EINVAL;
+		goto err_free_tcu;
+	}
+
+	tcu->nb_parent_irqs = irqs;
+
+	tcu->domain = irq_domain_add_linear(np, 32, &irq_generic_chip_ops,
+					    NULL);
+	if (!tcu->domain) {
+		ret = -ENOMEM;
+		goto err_free_tcu;
+	}
+
+	ret = irq_alloc_domain_generic_chips(tcu->domain, 32, 1, "TCU",
+					     handle_level_irq, 0,
+					     IRQ_NOPROBE | IRQ_LEVEL, 0);
+	if (ret) {
+		pr_crit("%s: Invalid 'interrupts' property\n", __func__);
+		goto out_domain_remove;
+	}
+
+	gc = irq_get_domain_generic_chip(tcu->domain, 0);
+	ct = gc->chip_types;
+
+	gc->wake_enabled = IRQ_MSK(32);
+	gc->private = tcu->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(tcu->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++) {
+		tcu->parent_irqs[i] = irq_of_parse_and_map(np, i);
+		if (!tcu->parent_irqs[i]) {
+			ret = -EINVAL;
+			goto out_unmap_irqs;
+		}
+
+		irq_set_chained_handler_and_data(tcu->parent_irqs[i],
+						 ingenic_tcu_intc_cascade,
+						 tcu->domain);
+	}
+
+	return 0;
+
+out_unmap_irqs:
+	for (; i > 0; i--)
+		irq_dispose_mapping(tcu->parent_irqs[i - 1]);
+out_domain_remove:
+	irq_domain_remove(tcu->domain);
+err_free_tcu:
+	kfree(tcu);
+	return ret;
+}
+IRQCHIP_DECLARE(jz4740_tcu_irq, "ingenic,jz4740-tcu", ingenic_tcu_irq_init);
+IRQCHIP_DECLARE(jz4725b_tcu_irq, "ingenic,jz4725b-tcu", ingenic_tcu_irq_init);
+IRQCHIP_DECLARE(jz4770_tcu_irq, "ingenic,jz4770-tcu", ingenic_tcu_irq_init);
-- 
2.21.0.593.g511ec345e18

Re: [PATCH v12 06/13] irqchip: Add irq-ingenic-tcu driver

[Paul Cercueil]

Hi,

I'll make a V13 of this patchset on top of -rc6, any feedback
for this IRQ driver? It's been already one month.

Thanks,
-Paul



Le mar. 21 mai 2019 à 16:51, Paul Cercueil <paul@crapouillou.net> a 
écrit :
> This driver handles the interrupt controller built in the 
> Timer/Counter
> Unit (TCU) of the JZ47xx SoCs from Ingenic.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/irqchip/Kconfig           |  11 ++
>  drivers/irqchip/Makefile          |   1 +
>  drivers/irqchip/irq-ingenic-tcu.c | 182 
> ++++++++++++++++++++++++++++++
>  3 files changed, 194 insertions(+)
>  create mode 100644 drivers/irqchip/irq-ingenic-tcu.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 1c1f3f66dfd3..2d0700e52db7 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -293,6 +293,17 @@ config INGENIC_IRQ
>  	depends on MACH_INGENIC
>  	default y
> 
> +config INGENIC_TCU_IRQ
> +	bool "Ingenic JZ47xx TCU interrupt controller"
> +	default MACH_INGENIC
> +	depends on MIPS || COMPILE_TEST
> +	select INGENIC_TCU
> +	help
> +	  Support for interrupts in the Timer/Counter Unit (TCU) of the 
> Ingenic
> +	  JZ47xx SoCs.
> +
> +	  If unsure, say N.
> +
>  config RENESAS_H8300H_INTC
>          bool
>  	select IRQ_DOMAIN
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 606a003a0000..f403b2c221e4 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -73,6 +73,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_MSCC_OCELOT_IRQ)		+= irq-mscc-ocelot.o
> diff --git a/drivers/irqchip/irq-ingenic-tcu.c 
> b/drivers/irqchip/irq-ingenic-tcu.c
> new file mode 100644
> index 000000000000..738ed06c983f
> --- /dev/null
> +++ b/drivers/irqchip/irq-ingenic-tcu.c
> @@ -0,0 +1,182 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * JZ47xx SoCs TCU IRQ driver
> + * Copyright (C) 2019 Paul Cercueil <paul@crapouillou.net>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/mfd/ingenic-tcu.h>
> +#include <linux/of_irq.h>
> +#include <linux/regmap.h>
> +
> +struct ingenic_tcu {
> +	struct regmap *map;
> +	struct clk *clk;
> +
> +	struct irq_domain *domain;
> +	unsigned int nb_parent_irqs;
> +	u32 parent_irqs[3];
> +};
> +
> +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_each_set_bit(i, (unsigned long *)&irq_reg, 32)
> +		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_irq_init(struct device_node *np,
> +				       struct device_node *parent)
> +{
> +	struct irq_chip_generic *gc;
> +	struct irq_chip_type *ct;
> +	struct ingenic_tcu *tcu;
> +	struct regmap *map;
> +	unsigned int i;
> +	int ret, irqs;
> +
> +	map = ingenic_tcu_get_regmap(np);
> +	if (IS_ERR(map))
> +		return PTR_ERR(map);
> +
> +	tcu = kzalloc(sizeof(*tcu), GFP_KERNEL);
> +	if (!tcu)
> +		return -ENOMEM;
> +
> +	tcu->map = map;
> +
> +	irqs = of_property_count_elems_of_size(np, "interrupts", 
> sizeof(u32));
> +	if (irqs < 0 || irqs > ARRAY_SIZE(tcu->parent_irqs)) {
> +		pr_crit("%s: Invalid 'interrupts' property\n", __func__);
> +		ret = -EINVAL;
> +		goto err_free_tcu;
> +	}
> +
> +	tcu->nb_parent_irqs = irqs;
> +
> +	tcu->domain = irq_domain_add_linear(np, 32, &irq_generic_chip_ops,
> +					    NULL);
> +	if (!tcu->domain) {
> +		ret = -ENOMEM;
> +		goto err_free_tcu;
> +	}
> +
> +	ret = irq_alloc_domain_generic_chips(tcu->domain, 32, 1, "TCU",
> +					     handle_level_irq, 0,
> +					     IRQ_NOPROBE | IRQ_LEVEL, 0);
> +	if (ret) {
> +		pr_crit("%s: Invalid 'interrupts' property\n", __func__);
> +		goto out_domain_remove;
> +	}
> +
> +	gc = irq_get_domain_generic_chip(tcu->domain, 0);
> +	ct = gc->chip_types;
> +
> +	gc->wake_enabled = IRQ_MSK(32);
> +	gc->private = tcu->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(tcu->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++) {
> +		tcu->parent_irqs[i] = irq_of_parse_and_map(np, i);
> +		if (!tcu->parent_irqs[i]) {
> +			ret = -EINVAL;
> +			goto out_unmap_irqs;
> +		}
> +
> +		irq_set_chained_handler_and_data(tcu->parent_irqs[i],
> +						 ingenic_tcu_intc_cascade,
> +						 tcu->domain);
> +	}
> +
> +	return 0;
> +
> +out_unmap_irqs:
> +	for (; i > 0; i--)
> +		irq_dispose_mapping(tcu->parent_irqs[i - 1]);
> +out_domain_remove:
> +	irq_domain_remove(tcu->domain);
> +err_free_tcu:
> +	kfree(tcu);
> +	return ret;
> +}
> +IRQCHIP_DECLARE(jz4740_tcu_irq, "ingenic,jz4740-tcu", 
> ingenic_tcu_irq_init);
> +IRQCHIP_DECLARE(jz4725b_tcu_irq, "ingenic,jz4725b-tcu", 
> ingenic_tcu_irq_init);
> +IRQCHIP_DECLARE(jz4770_tcu_irq, "ingenic,jz4770-tcu", 
> ingenic_tcu_irq_init);
> --
> 2.21.0.593.g511ec345e18
> 

[PATCH v12 07/13] clocksource: Add a new timer-ingenic driver

[Paul Cercueil]

This driver handles the TCU (Timer Counter Unit) present on the Ingenic
JZ47xx SoCs, and provides the kernel with a system timer, a clocksource
and a sched_clock.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    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
    
    v5: Major overhaul. Too many changes to list. Consider it's a new
    	patch.
    
    v6: - Add two API functions ingenic_tcu_request_channel and
    	  ingenic_tcu_release_channel. To be used by the PWM driver to
    	  request the use of a TCU channel. The driver will now dynamically
    	  move away the system timer or clocksource to a new TCU channel.
    	- The system timer now defaults to channel 0, the clocksource now
    	  defaults to channel 1 and is no more optional. The
    	  ingenic,timer-channel and ingenic,clocksource-channel devicetree
    	  properties are now gone.
    	- Fix round_rate / set_rate not calculating the prescale divider
    	  the same way. This caused problems when (parent_rate / div) would
    	  give a non-integer result. The behaviour is correct now.
    	- The clocksource clock is turned off on suspend now.
    
    v7: Fix section mismatch by using builtin_platform_driver_probe()
    
    v8: - Removed ingenic_tcu_[request,release]_channel, and the mechanism
    	  to dynamically change the TCU channel of the system timer or
    	  the clocksource.
    	- The driver's devicetree node can now have two more children
    	  nodes, that correspond to the system timer and clocksource.
    	  For these two, the driver will use the TCU timer that
    	  correspond to the memory resource supplied in their
    	  respective node.
    
    v9: - Removed support for clocksource / timer children devicetree
    	  nodes. Now, we use a property "ingenic,pwm-channels-mask" to
    	  know which PWM channels are reserved for PWM use and should
    	  not be used as OS timers.
    
    v10: - Use CLK_SET_RATE_UNGATE instead of CLK_SET_RATE_GATE + manually
    	   un-gating the clock before changing rate. Same for re-parenting.
    	 - Unconditionally create the clocksource and sched_clock even if
    	   the SoC possesses a OS Timer. That gives the choice back to the
    	   user which clocksource should be selected.
    	 - Use subsys_initcall() instead of builtin_platform_driver_probe().
    	   The OS Timer driver calls builtin_platform_driver_probe, which
    	   requires the device to be created before that.
    	 - Cosmetic cleanups
    
    v11: - Change prototype of exported function
    	   ingenic_tcu_pwm_can_use_chn(), use a struct device * as first
    	   argument.
    	 - Read clocksource using the regmap instead of bypassing it.
    	   Bypassing the regmap makes sense only for the sched_clock where
    	   the read operation must be as fast as possible.
    	 - Fix incorrect format in pr_crit() macro
    
    v12: - Clock handling and IRQ handling are gone, and are now handled
    	   in their own driver.
    	 - Obtain regmap from the ingenic-tcu MFD driver. As a result, we
    	   cannot bypass the regmap anymore for the sched_clock.

 drivers/clocksource/Kconfig         |  11 +
 drivers/clocksource/Makefile        |   1 +
 drivers/clocksource/ingenic-timer.c | 357 ++++++++++++++++++++++++++++
 3 files changed, 369 insertions(+)
 create mode 100644 drivers/clocksource/ingenic-timer.c

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 6bcaa4e2e72c..bb5d5c044341 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -672,4 +672,15 @@ config MILBEAUT_TIMER
 	help
 	  Enables the support for Milbeaut timer driver.
 
+config INGENIC_TIMER
+	bool "Clocksource/timer using the TCU in Ingenic JZ SoCs"
+	default MACH_INGENIC
+	depends on MIPS || COMPILE_TEST
+	depends on COMMON_CLK
+	select INGENIC_TCU
+	select TIMER_OF
+	select IRQ_DOMAIN
+	help
+	  Support for the timer/counter unit of the Ingenic JZ SoCs.
+
 endmenu
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 236858fa7fbf..553f3c61717a 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -78,6 +78,7 @@ 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)		+= ingenic-timer.o
 obj-$(CONFIG_CLKSRC_ST_LPC)		+= clksrc_st_lpc.o
 obj-$(CONFIG_X86_NUMACHIP)		+= numachip.o
 obj-$(CONFIG_ATCPIT100_TIMER)		+= timer-atcpit100.o
diff --git a/drivers/clocksource/ingenic-timer.c b/drivers/clocksource/ingenic-timer.c
new file mode 100644
index 000000000000..9f4df64390f4
--- /dev/null
+++ b/drivers/clocksource/ingenic-timer.c
@@ -0,0 +1,357 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * JZ47xx SoCs TCU IRQ driver
+ * Copyright (C) 2019 Paul Cercueil <paul@crapouillou.net>
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/clockchips.h>
+#include <linux/clocksource.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/ingenic-tcu.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/sched_clock.h>
+
+#include <dt-bindings/clock/ingenic,tcu.h>
+
+struct ingenic_soc_info {
+	unsigned int num_channels;
+};
+
+struct ingenic_tcu {
+	struct regmap *map;
+	struct clk *timer_clk, *cs_clk;
+
+	unsigned int timer_channel, cs_channel;
+	struct clock_event_device cevt;
+	struct clocksource cs;
+	char name[4];
+
+	unsigned long pwm_channels_mask;
+};
+
+static struct ingenic_tcu *ingenic_tcu;
+
+static u64 notrace ingenic_tcu_timer_read(void)
+{
+	struct ingenic_tcu *tcu = ingenic_tcu;
+	unsigned int count;
+
+	regmap_read(tcu->map, TCU_REG_TCNTc(tcu->cs_channel), &count);
+
+	return count;
+}
+
+static u64 notrace ingenic_tcu_timer_cs_read(struct clocksource *cs)
+{
+	return ingenic_tcu_timer_read();
+}
+
+static inline struct ingenic_tcu *to_ingenic_tcu(struct clock_event_device *evt)
+{
+	return container_of(evt, struct ingenic_tcu, cevt);
+}
+
+static int ingenic_tcu_cevt_set_state_shutdown(struct clock_event_device *evt)
+{
+	struct ingenic_tcu *tcu = to_ingenic_tcu(evt);
+
+	regmap_write(tcu->map, TCU_REG_TECR, BIT(tcu->timer_channel));
+
+	return 0;
+}
+
+static int ingenic_tcu_cevt_set_next(unsigned long next,
+				     struct clock_event_device *evt)
+{
+	struct ingenic_tcu *tcu = to_ingenic_tcu(evt);
+
+	if (next > 0xffff)
+		return -EINVAL;
+
+	regmap_write(tcu->map, TCU_REG_TDFRc(tcu->timer_channel), next);
+	regmap_write(tcu->map, TCU_REG_TCNTc(tcu->timer_channel), 0);
+	regmap_write(tcu->map, TCU_REG_TESR, BIT(tcu->timer_channel));
+
+	return 0;
+}
+
+static irqreturn_t ingenic_tcu_cevt_cb(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = dev_id;
+	struct ingenic_tcu *tcu = to_ingenic_tcu(evt);
+
+	regmap_write(tcu->map, TCU_REG_TECR, BIT(tcu->timer_channel));
+
+	if (evt->event_handler)
+		evt->event_handler(evt);
+
+	return IRQ_HANDLED;
+}
+
+static struct clk * __init ingenic_tcu_get_clock(struct device_node *np, int id)
+{
+	struct of_phandle_args args;
+
+	args.np = np;
+	args.args_count = 1;
+	args.args[0] = id;
+
+	return of_clk_get_from_provider(&args);
+}
+
+static int __init ingenic_tcu_timer_init(struct device_node *np,
+					 struct ingenic_tcu *tcu)
+{
+	unsigned int timer_virq, channel = tcu->timer_channel;
+	struct irq_domain *domain;
+	unsigned long rate;
+	int err;
+
+	tcu->timer_clk = ingenic_tcu_get_clock(np, channel);
+	if (IS_ERR(tcu->timer_clk))
+		return PTR_ERR(tcu->timer_clk);
+
+	err = clk_prepare_enable(tcu->timer_clk);
+	if (err)
+		goto err_clk_put;
+
+	rate = clk_get_rate(tcu->timer_clk);
+	if (!rate) {
+		err = -EINVAL;
+		goto err_clk_disable;
+	}
+
+	domain = irq_find_host(np);
+	if (!domain) {
+		err = -ENODEV;
+		goto err_clk_disable;
+	}
+
+	timer_virq = irq_create_mapping(domain, channel);
+	if (!timer_virq) {
+		err = -EINVAL;
+		goto err_clk_disable;
+	}
+
+	snprintf(tcu->name, sizeof(tcu->name), "TCU");
+
+	err = request_irq(timer_virq, ingenic_tcu_cevt_cb, IRQF_TIMER,
+			  tcu->name, &tcu->cevt);
+	if (err)
+		goto err_irq_dispose_mapping;
+
+	tcu->cevt.cpumask = cpumask_of(smp_processor_id());
+	tcu->cevt.features = CLOCK_EVT_FEAT_ONESHOT;
+	tcu->cevt.name = tcu->name;
+	tcu->cevt.rating = 200;
+	tcu->cevt.set_state_shutdown = ingenic_tcu_cevt_set_state_shutdown;
+	tcu->cevt.set_next_event = ingenic_tcu_cevt_set_next;
+
+	clockevents_config_and_register(&tcu->cevt, rate, 10, 0xffff);
+
+	return 0;
+
+err_irq_dispose_mapping:
+	irq_dispose_mapping(timer_virq);
+err_clk_disable:
+	clk_disable_unprepare(tcu->timer_clk);
+err_clk_put:
+	clk_put(tcu->timer_clk);
+	return err;
+}
+
+static int __init ingenic_tcu_clocksource_init(struct device_node *np,
+					       struct ingenic_tcu *tcu)
+{
+	unsigned int channel = tcu->cs_channel;
+	struct clocksource *cs = &tcu->cs;
+	unsigned long rate;
+	int err;
+
+	tcu->cs_clk = ingenic_tcu_get_clock(np, channel);
+	if (IS_ERR(tcu->cs_clk))
+		return PTR_ERR(tcu->cs_clk);
+
+	err = clk_prepare_enable(tcu->cs_clk);
+	if (err)
+		goto err_clk_put;
+
+	rate = clk_get_rate(tcu->cs_clk);
+	if (!rate) {
+		err = -EINVAL;
+		goto err_clk_disable;
+	}
+
+	/* Reset channel */
+	regmap_update_bits(tcu->map, TCU_REG_TCSRc(channel),
+			   0xffff & ~TCU_TCSR_RESERVED_BITS, 0);
+
+	/* Reset counter */
+	regmap_write(tcu->map, TCU_REG_TDFRc(channel), 0xffff);
+	regmap_write(tcu->map, TCU_REG_TCNTc(channel), 0);
+
+	/* Enable channel */
+	regmap_write(tcu->map, TCU_REG_TESR, BIT(channel));
+
+	cs->name = "ingenic-timer";
+	cs->rating = 200;
+	cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
+	cs->mask = CLOCKSOURCE_MASK(16);
+	cs->read = ingenic_tcu_timer_cs_read;
+
+	err = clocksource_register_hz(cs, rate);
+	if (err)
+		goto err_clk_disable;
+
+	return 0;
+
+err_clk_disable:
+	clk_disable_unprepare(tcu->cs_clk);
+err_clk_put:
+	clk_put(tcu->cs_clk);
+	return err;
+}
+
+static const struct ingenic_soc_info jz4740_soc_info = {
+	.num_channels = 8,
+};
+
+static const struct ingenic_soc_info jz4725b_soc_info = {
+	.num_channels = 6,
+};
+
+static const struct of_device_id ingenic_tcu_of_match[] = {
+	{ .compatible = "ingenic,jz4740-tcu", .data = &jz4740_soc_info, },
+	{ .compatible = "ingenic,jz4725b-tcu", .data = &jz4725b_soc_info, },
+	{ .compatible = "ingenic,jz4770-tcu", .data = &jz4740_soc_info, },
+	{ }
+};
+
+static int __init ingenic_tcu_init(struct device_node *np)
+{
+	const struct of_device_id *id = of_match_node(ingenic_tcu_of_match, np);
+	const struct ingenic_soc_info *soc_info = id->data;
+	struct ingenic_tcu *tcu;
+	struct regmap *map;
+	long rate;
+	int ret;
+
+	of_node_clear_flag(np, OF_POPULATED);
+
+	map = ingenic_tcu_get_regmap(np);
+	if (IS_ERR(map))
+		return PTR_ERR(map);
+
+	tcu = kzalloc(sizeof(*tcu), GFP_KERNEL);
+	if (!tcu)
+		return -ENOMEM;
+
+	/* Enable all TCU channels for PWM use by default except channels 0/1 */
+	tcu->pwm_channels_mask = GENMASK(soc_info->num_channels - 1, 2);
+	of_property_read_u32(np, "ingenic,pwm-channels-mask",
+			     (u32 *)&tcu->pwm_channels_mask);
+
+	/* Verify that we have at least two free channels */
+	if (hweight8(tcu->pwm_channels_mask) > soc_info->num_channels - 2) {
+		pr_crit("%s: Invalid PWM channel mask: 0x%02lx\n", __func__,
+			tcu->pwm_channels_mask);
+		ret = -EINVAL;
+		goto err_free_ingenic_tcu;
+	}
+
+	tcu->map = map;
+	ingenic_tcu = tcu;
+
+	tcu->timer_channel = find_first_zero_bit(&tcu->pwm_channels_mask,
+						 soc_info->num_channels);
+	tcu->cs_channel = find_next_zero_bit(&tcu->pwm_channels_mask,
+					     soc_info->num_channels,
+					     tcu->timer_channel + 1);
+
+	ret = ingenic_tcu_clocksource_init(np, tcu);
+	if (ret) {
+		pr_crit("%s: Unable to init clocksource: %i\n", __func__, ret);
+		goto err_free_ingenic_tcu;
+	}
+
+	ret = ingenic_tcu_timer_init(np, tcu);
+	if (ret)
+		goto err_tcu_clocksource_cleanup;
+
+	/* Register the sched_clock at the end as there's no way to undo it */
+	rate = clk_get_rate(tcu->cs_clk);
+	sched_clock_register(ingenic_tcu_timer_read, 16, rate);
+
+	return 0;
+
+err_tcu_clocksource_cleanup:
+	clocksource_unregister(&tcu->cs);
+	clk_disable_unprepare(tcu->cs_clk);
+	clk_put(tcu->cs_clk);
+err_free_ingenic_tcu:
+	kfree(tcu);
+	return ret;
+}
+
+TIMER_OF_DECLARE(jz4740_tcu_intc,  "ingenic,jz4740-tcu",  ingenic_tcu_init);
+TIMER_OF_DECLARE(jz4725b_tcu_intc, "ingenic,jz4725b-tcu", ingenic_tcu_init);
+TIMER_OF_DECLARE(jz4770_tcu_intc,  "ingenic,jz4770-tcu",  ingenic_tcu_init);
+
+
+static int __init ingenic_tcu_probe(struct platform_device *pdev)
+{
+	platform_set_drvdata(pdev, ingenic_tcu);
+
+	return 0;
+}
+
+static int __maybe_unused ingenic_tcu_suspend(struct device *dev)
+{
+	struct ingenic_tcu *tcu = dev_get_drvdata(dev);
+
+	clk_disable(tcu->cs_clk);
+	clk_disable(tcu->timer_clk);
+	return 0;
+}
+
+static int __maybe_unused ingenic_tcu_resume(struct device *dev)
+{
+	struct ingenic_tcu *tcu = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_enable(tcu->timer_clk);
+	if (ret)
+		return ret;
+
+	ret = clk_enable(tcu->cs_clk);
+	if (ret) {
+		clk_disable(tcu->timer_clk);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct dev_pm_ops __maybe_unused ingenic_tcu_pm_ops = {
+	/* _noirq: We want the TCU clocks to be gated last / ungated first */
+	.suspend_noirq = ingenic_tcu_suspend,
+	.resume_noirq  = ingenic_tcu_resume,
+};
+
+static struct platform_driver ingenic_tcu_driver = {
+	.driver = {
+		.name	= "ingenic-tcu-timer",
+#ifdef CONFIG_PM_SLEEP
+		.pm	= &ingenic_tcu_pm_ops,
+#endif
+		.of_match_table = ingenic_tcu_of_match,
+	},
+};
+builtin_platform_driver_probe(ingenic_tcu_driver, ingenic_tcu_probe);
-- 
2.21.0.593.g511ec345e18

Re: [PATCH v12 07/13] clocksource: Add a new timer-ingenic driver

[Paul Cercueil]

Hi,

I'll make a V13 of this patchset on top of -rc6, any feedback
for this clocksource driver? It's been already one month.

Thanks,
-Paul



Le mar. 21 mai 2019 à 16:51, Paul Cercueil <paul@crapouillou.net> a 
écrit :
> This driver handles the TCU (Timer Counter Unit) present on the 
> Ingenic
> JZ47xx SoCs, and provides the kernel with a system timer, a 
> clocksource
> and a sched_clock.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
> 
> Notes:
>     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
> 
>     v5: Major overhaul. Too many changes to list. Consider it's a new
>     	patch.
> 
>     v6: - Add two API functions ingenic_tcu_request_channel and
>     	  ingenic_tcu_release_channel. To be used by the PWM driver to
>     	  request the use of a TCU channel. The driver will now 
> dynamically
>     	  move away the system timer or clocksource to a new TCU channel.
>     	- The system timer now defaults to channel 0, the clocksource now
>     	  defaults to channel 1 and is no more optional. The
>     	  ingenic,timer-channel and ingenic,clocksource-channel 
> devicetree
>     	  properties are now gone.
>     	- Fix round_rate / set_rate not calculating the prescale divider
>     	  the same way. This caused problems when (parent_rate / div) 
> would
>     	  give a non-integer result. The behaviour is correct now.
>     	- The clocksource clock is turned off on suspend now.
> 
>     v7: Fix section mismatch by using builtin_platform_driver_probe()
> 
>     v8: - Removed ingenic_tcu_[request,release]_channel, and the 
> mechanism
>     	  to dynamically change the TCU channel of the system timer or
>     	  the clocksource.
>     	- The driver's devicetree node can now have two more children
>     	  nodes, that correspond to the system timer and clocksource.
>     	  For these two, the driver will use the TCU timer that
>     	  correspond to the memory resource supplied in their
>     	  respective node.
> 
>     v9: - Removed support for clocksource / timer children devicetree
>     	  nodes. Now, we use a property "ingenic,pwm-channels-mask" to
>     	  know which PWM channels are reserved for PWM use and should
>     	  not be used as OS timers.
> 
>     v10: - Use CLK_SET_RATE_UNGATE instead of CLK_SET_RATE_GATE + 
> manually
>     	   un-gating the clock before changing rate. Same for 
> re-parenting.
>     	 - Unconditionally create the clocksource and sched_clock even if
>     	   the SoC possesses a OS Timer. That gives the choice back to 
> the
>     	   user which clocksource should be selected.
>     	 - Use subsys_initcall() instead of 
> builtin_platform_driver_probe().
>     	   The OS Timer driver calls builtin_platform_driver_probe, which
>     	   requires the device to be created before that.
>     	 - Cosmetic cleanups
> 
>     v11: - Change prototype of exported function
>     	   ingenic_tcu_pwm_can_use_chn(), use a struct device * as first
>     	   argument.
>     	 - Read clocksource using the regmap instead of bypassing it.
>     	   Bypassing the regmap makes sense only for the sched_clock 
> where
>     	   the read operation must be as fast as possible.
>     	 - Fix incorrect format in pr_crit() macro
> 
>     v12: - Clock handling and IRQ handling are gone, and are now 
> handled
>     	   in their own driver.
>     	 - Obtain regmap from the ingenic-tcu MFD driver. As a result, we
>     	   cannot bypass the regmap anymore for the sched_clock.
> 
>  drivers/clocksource/Kconfig         |  11 +
>  drivers/clocksource/Makefile        |   1 +
>  drivers/clocksource/ingenic-timer.c | 357 
> ++++++++++++++++++++++++++++
>  3 files changed, 369 insertions(+)
>  create mode 100644 drivers/clocksource/ingenic-timer.c
> 
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 6bcaa4e2e72c..bb5d5c044341 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -672,4 +672,15 @@ config MILBEAUT_TIMER
>  	help
>  	  Enables the support for Milbeaut timer driver.
> 
> +config INGENIC_TIMER
> +	bool "Clocksource/timer using the TCU in Ingenic JZ SoCs"
> +	default MACH_INGENIC
> +	depends on MIPS || COMPILE_TEST
> +	depends on COMMON_CLK
> +	select INGENIC_TCU
> +	select TIMER_OF
> +	select IRQ_DOMAIN
> +	help
> +	  Support for the timer/counter unit of the Ingenic JZ SoCs.
> +
>  endmenu
> diff --git a/drivers/clocksource/Makefile 
> b/drivers/clocksource/Makefile
> index 236858fa7fbf..553f3c61717a 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -78,6 +78,7 @@ 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)		+= ingenic-timer.o
>  obj-$(CONFIG_CLKSRC_ST_LPC)		+= clksrc_st_lpc.o
>  obj-$(CONFIG_X86_NUMACHIP)		+= numachip.o
>  obj-$(CONFIG_ATCPIT100_TIMER)		+= timer-atcpit100.o
> diff --git a/drivers/clocksource/ingenic-timer.c 
> b/drivers/clocksource/ingenic-timer.c
> new file mode 100644
> index 000000000000..9f4df64390f4
> --- /dev/null
> +++ b/drivers/clocksource/ingenic-timer.c
> @@ -0,0 +1,357 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * JZ47xx SoCs TCU IRQ driver
> + * Copyright (C) 2019 Paul Cercueil <paul@crapouillou.net>
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/clockchips.h>
> +#include <linux/clocksource.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/ingenic-tcu.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/sched_clock.h>
> +
> +#include <dt-bindings/clock/ingenic,tcu.h>
> +
> +struct ingenic_soc_info {
> +	unsigned int num_channels;
> +};
> +
> +struct ingenic_tcu {
> +	struct regmap *map;
> +	struct clk *timer_clk, *cs_clk;
> +
> +	unsigned int timer_channel, cs_channel;
> +	struct clock_event_device cevt;
> +	struct clocksource cs;
> +	char name[4];
> +
> +	unsigned long pwm_channels_mask;
> +};
> +
> +static struct ingenic_tcu *ingenic_tcu;
> +
> +static u64 notrace ingenic_tcu_timer_read(void)
> +{
> +	struct ingenic_tcu *tcu = ingenic_tcu;
> +	unsigned int count;
> +
> +	regmap_read(tcu->map, TCU_REG_TCNTc(tcu->cs_channel), &count);
> +
> +	return count;
> +}
> +
> +static u64 notrace ingenic_tcu_timer_cs_read(struct clocksource *cs)
> +{
> +	return ingenic_tcu_timer_read();
> +}
> +
> +static inline struct ingenic_tcu *to_ingenic_tcu(struct 
> clock_event_device *evt)
> +{
> +	return container_of(evt, struct ingenic_tcu, cevt);
> +}
> +
> +static int ingenic_tcu_cevt_set_state_shutdown(struct 
> clock_event_device *evt)
> +{
> +	struct ingenic_tcu *tcu = to_ingenic_tcu(evt);
> +
> +	regmap_write(tcu->map, TCU_REG_TECR, BIT(tcu->timer_channel));
> +
> +	return 0;
> +}
> +
> +static int ingenic_tcu_cevt_set_next(unsigned long next,
> +				     struct clock_event_device *evt)
> +{
> +	struct ingenic_tcu *tcu = to_ingenic_tcu(evt);
> +
> +	if (next > 0xffff)
> +		return -EINVAL;
> +
> +	regmap_write(tcu->map, TCU_REG_TDFRc(tcu->timer_channel), next);
> +	regmap_write(tcu->map, TCU_REG_TCNTc(tcu->timer_channel), 0);
> +	regmap_write(tcu->map, TCU_REG_TESR, BIT(tcu->timer_channel));
> +
> +	return 0;
> +}
> +
> +static irqreturn_t ingenic_tcu_cevt_cb(int irq, void *dev_id)
> +{
> +	struct clock_event_device *evt = dev_id;
> +	struct ingenic_tcu *tcu = to_ingenic_tcu(evt);
> +
> +	regmap_write(tcu->map, TCU_REG_TECR, BIT(tcu->timer_channel));
> +
> +	if (evt->event_handler)
> +		evt->event_handler(evt);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static struct clk * __init ingenic_tcu_get_clock(struct device_node 
> *np, int id)
> +{
> +	struct of_phandle_args args;
> +
> +	args.np = np;
> +	args.args_count = 1;
> +	args.args[0] = id;
> +
> +	return of_clk_get_from_provider(&args);
> +}
> +
> +static int __init ingenic_tcu_timer_init(struct device_node *np,
> +					 struct ingenic_tcu *tcu)
> +{
> +	unsigned int timer_virq, channel = tcu->timer_channel;
> +	struct irq_domain *domain;
> +	unsigned long rate;
> +	int err;
> +
> +	tcu->timer_clk = ingenic_tcu_get_clock(np, channel);
> +	if (IS_ERR(tcu->timer_clk))
> +		return PTR_ERR(tcu->timer_clk);
> +
> +	err = clk_prepare_enable(tcu->timer_clk);
> +	if (err)
> +		goto err_clk_put;
> +
> +	rate = clk_get_rate(tcu->timer_clk);
> +	if (!rate) {
> +		err = -EINVAL;
> +		goto err_clk_disable;
> +	}
> +
> +	domain = irq_find_host(np);
> +	if (!domain) {
> +		err = -ENODEV;
> +		goto err_clk_disable;
> +	}
> +
> +	timer_virq = irq_create_mapping(domain, channel);
> +	if (!timer_virq) {
> +		err = -EINVAL;
> +		goto err_clk_disable;
> +	}
> +
> +	snprintf(tcu->name, sizeof(tcu->name), "TCU");
> +
> +	err = request_irq(timer_virq, ingenic_tcu_cevt_cb, IRQF_TIMER,
> +			  tcu->name, &tcu->cevt);
> +	if (err)
> +		goto err_irq_dispose_mapping;
> +
> +	tcu->cevt.cpumask = cpumask_of(smp_processor_id());
> +	tcu->cevt.features = CLOCK_EVT_FEAT_ONESHOT;
> +	tcu->cevt.name = tcu->name;
> +	tcu->cevt.rating = 200;
> +	tcu->cevt.set_state_shutdown = ingenic_tcu_cevt_set_state_shutdown;
> +	tcu->cevt.set_next_event = ingenic_tcu_cevt_set_next;
> +
> +	clockevents_config_and_register(&tcu->cevt, rate, 10, 0xffff);
> +
> +	return 0;
> +
> +err_irq_dispose_mapping:
> +	irq_dispose_mapping(timer_virq);
> +err_clk_disable:
> +	clk_disable_unprepare(tcu->timer_clk);
> +err_clk_put:
> +	clk_put(tcu->timer_clk);
> +	return err;
> +}
> +
> +static int __init ingenic_tcu_clocksource_init(struct device_node 
> *np,
> +					       struct ingenic_tcu *tcu)
> +{
> +	unsigned int channel = tcu->cs_channel;
> +	struct clocksource *cs = &tcu->cs;
> +	unsigned long rate;
> +	int err;
> +
> +	tcu->cs_clk = ingenic_tcu_get_clock(np, channel);
> +	if (IS_ERR(tcu->cs_clk))
> +		return PTR_ERR(tcu->cs_clk);
> +
> +	err = clk_prepare_enable(tcu->cs_clk);
> +	if (err)
> +		goto err_clk_put;
> +
> +	rate = clk_get_rate(tcu->cs_clk);
> +	if (!rate) {
> +		err = -EINVAL;
> +		goto err_clk_disable;
> +	}
> +
> +	/* Reset channel */
> +	regmap_update_bits(tcu->map, TCU_REG_TCSRc(channel),
> +			   0xffff & ~TCU_TCSR_RESERVED_BITS, 0);
> +
> +	/* Reset counter */
> +	regmap_write(tcu->map, TCU_REG_TDFRc(channel), 0xffff);
> +	regmap_write(tcu->map, TCU_REG_TCNTc(channel), 0);
> +
> +	/* Enable channel */
> +	regmap_write(tcu->map, TCU_REG_TESR, BIT(channel));
> +
> +	cs->name = "ingenic-timer";
> +	cs->rating = 200;
> +	cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
> +	cs->mask = CLOCKSOURCE_MASK(16);
> +	cs->read = ingenic_tcu_timer_cs_read;
> +
> +	err = clocksource_register_hz(cs, rate);
> +	if (err)
> +		goto err_clk_disable;
> +
> +	return 0;
> +
> +err_clk_disable:
> +	clk_disable_unprepare(tcu->cs_clk);
> +err_clk_put:
> +	clk_put(tcu->cs_clk);
> +	return err;
> +}
> +
> +static const struct ingenic_soc_info jz4740_soc_info = {
> +	.num_channels = 8,
> +};
> +
> +static const struct ingenic_soc_info jz4725b_soc_info = {
> +	.num_channels = 6,
> +};
> +
> +static const struct of_device_id ingenic_tcu_of_match[] = {
> +	{ .compatible = "ingenic,jz4740-tcu", .data = &jz4740_soc_info, },
> +	{ .compatible = "ingenic,jz4725b-tcu", .data = &jz4725b_soc_info, },
> +	{ .compatible = "ingenic,jz4770-tcu", .data = &jz4740_soc_info, },
> +	{ }
> +};
> +
> +static int __init ingenic_tcu_init(struct device_node *np)
> +{
> +	const struct of_device_id *id = of_match_node(ingenic_tcu_of_match, 
> np);
> +	const struct ingenic_soc_info *soc_info = id->data;
> +	struct ingenic_tcu *tcu;
> +	struct regmap *map;
> +	long rate;
> +	int ret;
> +
> +	of_node_clear_flag(np, OF_POPULATED);
> +
> +	map = ingenic_tcu_get_regmap(np);
> +	if (IS_ERR(map))
> +		return PTR_ERR(map);
> +
> +	tcu = kzalloc(sizeof(*tcu), GFP_KERNEL);
> +	if (!tcu)
> +		return -ENOMEM;
> +
> +	/* Enable all TCU channels for PWM use by default except channels 
> 0/1 */
> +	tcu->pwm_channels_mask = GENMASK(soc_info->num_channels - 1, 2);
> +	of_property_read_u32(np, "ingenic,pwm-channels-mask",
> +			     (u32 *)&tcu->pwm_channels_mask);
> +
> +	/* Verify that we have at least two free channels */
> +	if (hweight8(tcu->pwm_channels_mask) > soc_info->num_channels - 2) {
> +		pr_crit("%s: Invalid PWM channel mask: 0x%02lx\n", __func__,
> +			tcu->pwm_channels_mask);
> +		ret = -EINVAL;
> +		goto err_free_ingenic_tcu;
> +	}
> +
> +	tcu->map = map;
> +	ingenic_tcu = tcu;
> +
> +	tcu->timer_channel = find_first_zero_bit(&tcu->pwm_channels_mask,
> +						 soc_info->num_channels);
> +	tcu->cs_channel = find_next_zero_bit(&tcu->pwm_channels_mask,
> +					     soc_info->num_channels,
> +					     tcu->timer_channel + 1);
> +
> +	ret = ingenic_tcu_clocksource_init(np, tcu);
> +	if (ret) {
> +		pr_crit("%s: Unable to init clocksource: %i\n", __func__, ret);
> +		goto err_free_ingenic_tcu;
> +	}
> +
> +	ret = ingenic_tcu_timer_init(np, tcu);
> +	if (ret)
> +		goto err_tcu_clocksource_cleanup;
> +
> +	/* Register the sched_clock at the end as there's no way to undo it 
> */
> +	rate = clk_get_rate(tcu->cs_clk);
> +	sched_clock_register(ingenic_tcu_timer_read, 16, rate);
> +
> +	return 0;
> +
> +err_tcu_clocksource_cleanup:
> +	clocksource_unregister(&tcu->cs);
> +	clk_disable_unprepare(tcu->cs_clk);
> +	clk_put(tcu->cs_clk);
> +err_free_ingenic_tcu:
> +	kfree(tcu);
> +	return ret;
> +}
> +
> +TIMER_OF_DECLARE(jz4740_tcu_intc,  "ingenic,jz4740-tcu",  
> ingenic_tcu_init);
> +TIMER_OF_DECLARE(jz4725b_tcu_intc, "ingenic,jz4725b-tcu", 
> ingenic_tcu_init);
> +TIMER_OF_DECLARE(jz4770_tcu_intc,  "ingenic,jz4770-tcu",  
> ingenic_tcu_init);
> +
> +
> +static int __init ingenic_tcu_probe(struct platform_device *pdev)
> +{
> +	platform_set_drvdata(pdev, ingenic_tcu);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused ingenic_tcu_suspend(struct device *dev)
> +{
> +	struct ingenic_tcu *tcu = dev_get_drvdata(dev);
> +
> +	clk_disable(tcu->cs_clk);
> +	clk_disable(tcu->timer_clk);
> +	return 0;
> +}
> +
> +static int __maybe_unused ingenic_tcu_resume(struct device *dev)
> +{
> +	struct ingenic_tcu *tcu = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = clk_enable(tcu->timer_clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_enable(tcu->cs_clk);
> +	if (ret) {
> +		clk_disable(tcu->timer_clk);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops __maybe_unused ingenic_tcu_pm_ops = {
> +	/* _noirq: We want the TCU clocks to be gated last / ungated first 
> */
> +	.suspend_noirq = ingenic_tcu_suspend,
> +	.resume_noirq  = ingenic_tcu_resume,
> +};
> +
> +static struct platform_driver ingenic_tcu_driver = {
> +	.driver = {
> +		.name	= "ingenic-tcu-timer",
> +#ifdef CONFIG_PM_SLEEP
> +		.pm	= &ingenic_tcu_pm_ops,
> +#endif
> +		.of_match_table = ingenic_tcu_of_match,
> +	},
> +};
> +builtin_platform_driver_probe(ingenic_tcu_driver, ingenic_tcu_probe);
> --
> 2.21.0.593.g511ec345e18
> 

[PATCH v12 08/13] clk: jz4740: Add TCU clock

[Paul Cercueil]

Add the missing TCU clock to the list of clocks supplied by the CGU for
the JZ4740 SoC.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Acked-by: Stephen Boyd <sboyd@kernel.org>
Acked-by: Rob Herring <robh@kernel.org>
---

Notes:
    v5: New patch
    
    v6-v12: No change

 drivers/clk/ingenic/jz4740-cgu.c       | 6 ++++++
 include/dt-bindings/clock/jz4740-cgu.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/clk/ingenic/jz4740-cgu.c b/drivers/clk/ingenic/jz4740-cgu.c
index 25f7df028e67..fbc7319c2861 100644
--- a/drivers/clk/ingenic/jz4740-cgu.c
+++ b/drivers/clk/ingenic/jz4740-cgu.c
@@ -212,6 +212,12 @@ static const struct ingenic_cgu_clk_info jz4740_cgu_clocks[] = {
 		.parents = { JZ4740_CLK_EXT, -1, -1, -1 },
 		.gate = { CGU_REG_CLKGR, 5 },
 	},
+
+	[JZ4740_CLK_TCU] = {
+		"tcu", CGU_CLK_GATE,
+		.parents = { JZ4740_CLK_EXT, -1, -1, -1 },
+		.gate = { CGU_REG_CLKGR, 1 },
+	},
 };
 
 static void __init jz4740_cgu_init(struct device_node *np)
diff --git a/include/dt-bindings/clock/jz4740-cgu.h b/include/dt-bindings/clock/jz4740-cgu.h
index 6ed83f926ae7..e82d77028581 100644
--- a/include/dt-bindings/clock/jz4740-cgu.h
+++ b/include/dt-bindings/clock/jz4740-cgu.h
@@ -34,5 +34,6 @@
 #define JZ4740_CLK_ADC		19
 #define JZ4740_CLK_I2C		20
 #define JZ4740_CLK_AIC		21
+#define JZ4740_CLK_TCU		22
 
 #endif /* __DT_BINDINGS_CLOCK_JZ4740_CGU_H__ */
-- 
2.21.0.593.g511ec345e18

[PATCH v12 09/13] MIPS: jz4740: Add DTS nodes for the TCU drivers

[Paul Cercueil]

Add DTS nodes for the JZ4780, JZ4770 and JZ4740 devicetree files.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    v5: New patch
    
    v6: Fix register lengths in watchdog/pwm nodes
    
    v7: No change
    
    v8: - Fix wrong start address for PWM node
    	- Add system timer and clocksource sub-nodes
    
    v9: Drop timer and clocksource sub-nodes
    
    v10-v11: No change
    
    v12: Drop PWM/watchdog/OST sub-nodes, for now.

 arch/mips/boot/dts/ingenic/jz4740.dtsi | 22 ++++++++++++++++++++++
 arch/mips/boot/dts/ingenic/jz4770.dtsi | 21 +++++++++++++++++++++
 arch/mips/boot/dts/ingenic/jz4780.dtsi | 21 +++++++++++++++++++++
 3 files changed, 64 insertions(+)

diff --git a/arch/mips/boot/dts/ingenic/jz4740.dtsi b/arch/mips/boot/dts/ingenic/jz4740.dtsi
index 2beb78a62b7d..807d9702d4cf 100644
--- a/arch/mips/boot/dts/ingenic/jz4740.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4740.dtsi
@@ -53,6 +53,28 @@
 		clock-names = "rtc";
 	};
 
+	tcu: timer@10002000 {
+		compatible = "ingenic,jz4740-tcu";
+		reg = <0x10002000 0x1000>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0x0 0x10002000 0x1000>;
+
+		#clock-cells = <1>;
+
+		clocks = <&cgu JZ4740_CLK_RTC
+			  &cgu JZ4740_CLK_EXT
+			  &cgu JZ4740_CLK_PCLK
+			  &cgu JZ4740_CLK_TCU>;
+		clock-names = "rtc", "ext", "pclk", "tcu";
+
+		interrupt-controller;
+		#interrupt-cells = <1>;
+
+		interrupt-parent = <&intc>;
+		interrupts = <23 22 21>;
+	};
+
 	rtc_dev: rtc@10003000 {
 		compatible = "ingenic,jz4740-rtc";
 		reg = <0x10003000 0x40>;
diff --git a/arch/mips/boot/dts/ingenic/jz4770.dtsi b/arch/mips/boot/dts/ingenic/jz4770.dtsi
index 49ede6c14ff3..70932fd90902 100644
--- a/arch/mips/boot/dts/ingenic/jz4770.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4770.dtsi
@@ -46,6 +46,27 @@
 		#clock-cells = <1>;
 	};
 
+	tcu: timer@10002000 {
+		compatible = "ingenic,jz4770-tcu";
+		reg = <0x10002000 0x1000>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0x0 0x10002000 0x1000>;
+
+		#clock-cells = <1>;
+
+		clocks = <&cgu JZ4770_CLK_RTC
+			  &cgu JZ4770_CLK_EXT
+			  &cgu JZ4770_CLK_PCLK>;
+		clock-names = "rtc", "ext", "pclk";
+
+		interrupt-controller;
+		#interrupt-cells = <1>;
+
+		interrupt-parent = <&intc>;
+		interrupts = <27 26 25>;
+	};
+
 	pinctrl: pin-controller@10010000 {
 		compatible = "ingenic,jz4770-pinctrl";
 		reg = <0x10010000 0x600>;
diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index b03cdec56de9..495082ce7fc5 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -46,6 +46,27 @@
 		#clock-cells = <1>;
 	};
 
+	tcu: timer@10002000 {
+		compatible = "ingenic,jz4770-tcu";
+		reg = <0x10002000 0x1000>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0x0 0x10002000 0x1000>;
+
+		#clock-cells = <1>;
+
+		clocks = <&cgu JZ4780_CLK_RTCLK
+			  &cgu JZ4780_CLK_EXCLK
+			  &cgu JZ4780_CLK_PCLK>;
+		clock-names = "rtc", "ext", "pclk";
+
+		interrupt-controller;
+		#interrupt-cells = <1>;
+
+		interrupt-parent = <&intc>;
+		interrupts = <27 26 25>;
+	};
+
 	rtc_dev: rtc@10003000 {
 		compatible = "ingenic,jz4780-rtc";
 		reg = <0x10003000 0x4c>;
-- 
2.21.0.593.g511ec345e18

Re: [PATCH v12 09/13] MIPS: jz4740: Add DTS nodes for the TCU drivers

[Mathieu Malaterre]

On Tue, May 21, 2019 at 4:52 PM Paul Cercueil <paul@crapouillou.net> wrote:
>
> Add DTS nodes for the JZ4780, JZ4770 and JZ4740 devicetree files.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>
> Notes:
>     v5: New patch
>
>     v6: Fix register lengths in watchdog/pwm nodes
>
>     v7: No change
>
>     v8: - Fix wrong start address for PWM node
>         - Add system timer and clocksource sub-nodes
>
>     v9: Drop timer and clocksource sub-nodes
>
>     v10-v11: No change
>
>     v12: Drop PWM/watchdog/OST sub-nodes, for now.
>
>  arch/mips/boot/dts/ingenic/jz4740.dtsi | 22 ++++++++++++++++++++++
>  arch/mips/boot/dts/ingenic/jz4770.dtsi | 21 +++++++++++++++++++++
>  arch/mips/boot/dts/ingenic/jz4780.dtsi | 21 +++++++++++++++++++++
>  3 files changed, 64 insertions(+)
>
> diff --git a/arch/mips/boot/dts/ingenic/jz4740.dtsi b/arch/mips/boot/dts/ingenic/jz4740.dtsi
> index 2beb78a62b7d..807d9702d4cf 100644
> --- a/arch/mips/boot/dts/ingenic/jz4740.dtsi
> +++ b/arch/mips/boot/dts/ingenic/jz4740.dtsi
> @@ -53,6 +53,28 @@
>                 clock-names = "rtc";
>         };
>
> +       tcu: timer@10002000 {
> +               compatible = "ingenic,jz4740-tcu";
> +               reg = <0x10002000 0x1000>;
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +               ranges = <0x0 0x10002000 0x1000>;
> +
> +               #clock-cells = <1>;
> +
> +               clocks = <&cgu JZ4740_CLK_RTC
> +                         &cgu JZ4740_CLK_EXT
> +                         &cgu JZ4740_CLK_PCLK
> +                         &cgu JZ4740_CLK_TCU>;
> +               clock-names = "rtc", "ext", "pclk", "tcu";
> +
> +               interrupt-controller;
> +               #interrupt-cells = <1>;
> +
> +               interrupt-parent = <&intc>;
> +               interrupts = <23 22 21>;
> +       };
> +
>         rtc_dev: rtc@10003000 {
>                 compatible = "ingenic,jz4740-rtc";
>                 reg = <0x10003000 0x40>;
> diff --git a/arch/mips/boot/dts/ingenic/jz4770.dtsi b/arch/mips/boot/dts/ingenic/jz4770.dtsi
> index 49ede6c14ff3..70932fd90902 100644
> --- a/arch/mips/boot/dts/ingenic/jz4770.dtsi
> +++ b/arch/mips/boot/dts/ingenic/jz4770.dtsi
> @@ -46,6 +46,27 @@
>                 #clock-cells = <1>;
>         };
>
> +       tcu: timer@10002000 {
> +               compatible = "ingenic,jz4770-tcu";
> +               reg = <0x10002000 0x1000>;
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +               ranges = <0x0 0x10002000 0x1000>;
> +
> +               #clock-cells = <1>;
> +
> +               clocks = <&cgu JZ4770_CLK_RTC
> +                         &cgu JZ4770_CLK_EXT
> +                         &cgu JZ4770_CLK_PCLK>;
> +               clock-names = "rtc", "ext", "pclk";
> +
> +               interrupt-controller;
> +               #interrupt-cells = <1>;
> +
> +               interrupt-parent = <&intc>;
> +               interrupts = <27 26 25>;
> +       };
> +
>         pinctrl: pin-controller@10010000 {
>                 compatible = "ingenic,jz4770-pinctrl";
>                 reg = <0x10010000 0x600>;
> diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
> index b03cdec56de9..495082ce7fc5 100644
> --- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
> +++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
> @@ -46,6 +46,27 @@
>                 #clock-cells = <1>;
>         };
>
> +       tcu: timer@10002000 {

With W=1, I see:

../arch/mips/boot/dts/ingenic/jz4780.dtsi:64.22-83.4: Warning
(unique_unit_address): /timer@10002000: duplicate unit-address (also
used in node /watchdog@1000
2000)


> +               compatible = "ingenic,jz4770-tcu";
> +               reg = <0x10002000 0x1000>;
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +               ranges = <0x0 0x10002000 0x1000>;
> +
> +               #clock-cells = <1>;
> +
> +               clocks = <&cgu JZ4780_CLK_RTCLK
> +                         &cgu JZ4780_CLK_EXCLK
> +                         &cgu JZ4780_CLK_PCLK>;
> +               clock-names = "rtc", "ext", "pclk";
> +
> +               interrupt-controller;
> +               #interrupt-cells = <1>;
> +
> +               interrupt-parent = <&intc>;
> +               interrupts = <27 26 25>;
> +       };
> +
>         rtc_dev: rtc@10003000 {
>                 compatible = "ingenic,jz4780-rtc";
>                 reg = <0x10003000 0x4c>;
> --
> 2.21.0.593.g511ec345e18
>

Re: [PATCH v12 09/13] MIPS: jz4740: Add DTS nodes for the TCU drivers

[Paul Cercueil]

On Wed, May 22, 2019 at 11:21 AM, Mathieu Malaterre <malat@debian.org> 
wrote:
> On Tue, May 21, 2019 at 4:52 PM Paul Cercueil <paul@crapouillou.net> 
> wrote:
>> 
>>  Add DTS nodes for the JZ4780, JZ4770 and JZ4740 devicetree files.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>> 
>>  Notes:
>>      v5: New patch
>> 
>>      v6: Fix register lengths in watchdog/pwm nodes
>> 
>>      v7: No change
>> 
>>      v8: - Fix wrong start address for PWM node
>>          - Add system timer and clocksource sub-nodes
>> 
>>      v9: Drop timer and clocksource sub-nodes
>> 
>>      v10-v11: No change
>> 
>>      v12: Drop PWM/watchdog/OST sub-nodes, for now.
>> 
>>   arch/mips/boot/dts/ingenic/jz4740.dtsi | 22 ++++++++++++++++++++++
>>   arch/mips/boot/dts/ingenic/jz4770.dtsi | 21 +++++++++++++++++++++
>>   arch/mips/boot/dts/ingenic/jz4780.dtsi | 21 +++++++++++++++++++++
>>   3 files changed, 64 insertions(+)
>> 
>>  diff --git a/arch/mips/boot/dts/ingenic/jz4740.dtsi 
>> b/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>  index 2beb78a62b7d..807d9702d4cf 100644
>>  --- a/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>  +++ b/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>  @@ -53,6 +53,28 @@
>>                  clock-names = "rtc";
>>          };
>> 
>>  +       tcu: timer@10002000 {
>>  +               compatible = "ingenic,jz4740-tcu";
>>  +               reg = <0x10002000 0x1000>;
>>  +               #address-cells = <1>;
>>  +               #size-cells = <1>;
>>  +               ranges = <0x0 0x10002000 0x1000>;
>>  +
>>  +               #clock-cells = <1>;
>>  +
>>  +               clocks = <&cgu JZ4740_CLK_RTC
>>  +                         &cgu JZ4740_CLK_EXT
>>  +                         &cgu JZ4740_CLK_PCLK
>>  +                         &cgu JZ4740_CLK_TCU>;
>>  +               clock-names = "rtc", "ext", "pclk", "tcu";
>>  +
>>  +               interrupt-controller;
>>  +               #interrupt-cells = <1>;
>>  +
>>  +               interrupt-parent = <&intc>;
>>  +               interrupts = <23 22 21>;
>>  +       };
>>  +
>>          rtc_dev: rtc@10003000 {
>>                  compatible = "ingenic,jz4740-rtc";
>>                  reg = <0x10003000 0x40>;
>>  diff --git a/arch/mips/boot/dts/ingenic/jz4770.dtsi 
>> b/arch/mips/boot/dts/ingenic/jz4770.dtsi
>>  index 49ede6c14ff3..70932fd90902 100644
>>  --- a/arch/mips/boot/dts/ingenic/jz4770.dtsi
>>  +++ b/arch/mips/boot/dts/ingenic/jz4770.dtsi
>>  @@ -46,6 +46,27 @@
>>                  #clock-cells = <1>;
>>          };
>> 
>>  +       tcu: timer@10002000 {
>>  +               compatible = "ingenic,jz4770-tcu";
>>  +               reg = <0x10002000 0x1000>;
>>  +               #address-cells = <1>;
>>  +               #size-cells = <1>;
>>  +               ranges = <0x0 0x10002000 0x1000>;
>>  +
>>  +               #clock-cells = <1>;
>>  +
>>  +               clocks = <&cgu JZ4770_CLK_RTC
>>  +                         &cgu JZ4770_CLK_EXT
>>  +                         &cgu JZ4770_CLK_PCLK>;
>>  +               clock-names = "rtc", "ext", "pclk";
>>  +
>>  +               interrupt-controller;
>>  +               #interrupt-cells = <1>;
>>  +
>>  +               interrupt-parent = <&intc>;
>>  +               interrupts = <27 26 25>;
>>  +       };
>>  +
>>          pinctrl: pin-controller@10010000 {
>>                  compatible = "ingenic,jz4770-pinctrl";
>>                  reg = <0x10010000 0x600>;
>>  diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi 
>> b/arch/mips/boot/dts/ingenic/jz4780.dtsi
>>  index b03cdec56de9..495082ce7fc5 100644
>>  --- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
>>  +++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
>>  @@ -46,6 +46,27 @@
>>                  #clock-cells = <1>;
>>          };
>> 
>>  +       tcu: timer@10002000 {
> 
> With W=1, I see:
> 
> ../arch/mips/boot/dts/ingenic/jz4780.dtsi:64.22-83.4: Warning
> (unique_unit_address): /timer@10002000: duplicate unit-address (also
> used in node /watchdog@1000
> 2000)

That didn't happen in V11 because there I was also migrating the
watchdog and PWM drivers to children nodes of the TCU. It was more
atomic, but it also was a 27-patches bomb touching a lot of
subsystems that nobody was ever going to merge.

Is the address conflict OK, knowing that the watchdog node will
move out of the way as soon as this patchset is merged?

Should I add an extra patch to remove the watchdog node instead?

(and yes, devicetree ABI will break, which is sort-of OK in this
case - as on Ingenic boards the devicetree blobs are always
compiled within the kernel, so I think we should make this
much-needed change while we still can).


>>  +               compatible = "ingenic,jz4770-tcu";
>>  +               reg = <0x10002000 0x1000>;
>>  +               #address-cells = <1>;
>>  +               #size-cells = <1>;
>>  +               ranges = <0x0 0x10002000 0x1000>;
>>  +
>>  +               #clock-cells = <1>;
>>  +
>>  +               clocks = <&cgu JZ4780_CLK_RTCLK
>>  +                         &cgu JZ4780_CLK_EXCLK
>>  +                         &cgu JZ4780_CLK_PCLK>;
>>  +               clock-names = "rtc", "ext", "pclk";
>>  +
>>  +               interrupt-controller;
>>  +               #interrupt-cells = <1>;
>>  +
>>  +               interrupt-parent = <&intc>;
>>  +               interrupts = <27 26 25>;
>>  +       };
>>  +
>>          rtc_dev: rtc@10003000 {
>>                  compatible = "ingenic,jz4780-rtc";
>>                  reg = <0x10003000 0x4c>;
>>  --
>>  2.21.0.593.g511ec345e18
>> 

[PATCH v12 10/13] MIPS: qi_lb60: Reduce system timer and clocksource to 750 kHz

[Paul Cercueil]

The default clock (12 MHz) is too fast for the system timer, which fails
to report time accurately.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    v5: New patch
    
    v6: Remove ingenic,clocksource-channel property
    
    v7-v12: No change

 arch/mips/boot/dts/ingenic/qi_lb60.dts | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/mips/boot/dts/ingenic/qi_lb60.dts b/arch/mips/boot/dts/ingenic/qi_lb60.dts
index 76aaf8982554..01b8c917cb33 100644
--- a/arch/mips/boot/dts/ingenic/qi_lb60.dts
+++ b/arch/mips/boot/dts/ingenic/qi_lb60.dts
@@ -2,6 +2,7 @@
 /dts-v1/;
 
 #include "jz4740.dtsi"
+#include <dt-bindings/clock/ingenic,tcu.h>
 
 / {
 	compatible = "qi,lb60", "ingenic,jz4740";
@@ -31,3 +32,9 @@
 		bias-disable;
 	};
 };
+
+&tcu {
+	/* 750 kHz for the system timer and clocksource */
+	assigned-clocks = <&tcu TCU_CLK_TIMER0>, <&tcu TCU_CLK_TIMER1>;
+	assigned-clock-rates = <750000>, <750000>;
+};
-- 
2.21.0.593.g511ec345e18

[PATCH v12 11/13] MIPS: CI20: Reduce system timer and clocksource to 3 MHz

[Paul Cercueil]

The default clock (48 MHz) is too fast for the system timer.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    v5: New patch
    
    v6: Set also the rate for the clocksource channel's clock
    
    v7: No change
    
    v8: No change
    
    v9: Don't configure clock timer1, as the OS Timer is used as
    	clocksource on this SoC
    
    v10: Revert back to v8 bahaviour. Let the user choose what
    	 clocksource should be used.
    
    v11-v12: No change

 arch/mips/boot/dts/ingenic/ci20.dts | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/mips/boot/dts/ingenic/ci20.dts b/arch/mips/boot/dts/ingenic/ci20.dts
index 4f7b1fa31cf5..2e9952311ecd 100644
--- a/arch/mips/boot/dts/ingenic/ci20.dts
+++ b/arch/mips/boot/dts/ingenic/ci20.dts
@@ -2,6 +2,7 @@
 /dts-v1/;
 
 #include "jz4780.dtsi"
+#include <dt-bindings/clock/ingenic,tcu.h>
 #include <dt-bindings/gpio/gpio.h>
 
 / {
@@ -238,3 +239,9 @@
 		bias-disable;
 	};
 };
+
+&tcu {
+	/* 3 MHz for the system timer and clocksource */
+	assigned-clocks = <&tcu TCU_CLK_TIMER0>, <&tcu TCU_CLK_TIMER1>;
+	assigned-clock-rates = <3000000>, <3000000>;
+};
-- 
2.21.0.593.g511ec345e18

[PATCH v12 12/13] MIPS: GCW0: Reduce system timer and clocksource to 750 kHz

[Paul Cercueil]

The default clock (12 MHz) is too fast for the system timer.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    v8: New patch
    
    v9: Don't configure clock timer1, as the OS Timer is used as
    	clocksource on this SoC
    
    v10: Revert back to v8 bahaviour. Let the user choose what
    	 clocksource should be used.
    
    v11: No change
    
    v12: Move clocksource to channel 2, as channel 1 is used as PWM
    	 for the backlight.

 arch/mips/boot/dts/ingenic/gcw0.dts | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/mips/boot/dts/ingenic/gcw0.dts b/arch/mips/boot/dts/ingenic/gcw0.dts
index 35f0291e8d38..f58d239c2058 100644
--- a/arch/mips/boot/dts/ingenic/gcw0.dts
+++ b/arch/mips/boot/dts/ingenic/gcw0.dts
@@ -2,6 +2,7 @@
 /dts-v1/;
 
 #include "jz4770.dtsi"
+#include <dt-bindings/clock/ingenic,tcu.h>
 
 / {
 	compatible = "gcw,zero", "ingenic,jz4770";
@@ -60,3 +61,12 @@
 	/* The WiFi module is connected to the UHC. */
 	status = "okay";
 };
+
+&tcu {
+	/* 750 kHz for the system timer and clocksource */
+	assigned-clocks = <&tcu TCU_CLK_TIMER0>, <&tcu TCU_CLK_TIMER2>;
+	assigned-clock-rates = <750000>, <750000>;
+
+	/* PWM1 is in use, so reserve channel #2 for the clocksource */
+	ingenic,pwm-channels-mask = <0xfa>;
+};
-- 
2.21.0.593.g511ec345e18

[PATCH v12 13/13] MIPS: jz4740: Drop obsolete code

[Paul Cercueil]

The old clocksource/timer platform code is now obsoleted by the newly
introduced TCU drivers.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    v5: New patch
    
    v6-v11: No change
    
    v12: Only remove clocksource code. The rest will eventually be
    	 removed in a future patchset when the PWM/watchdog drivers
    	 are updated.

 arch/mips/jz4740/time.c | 154 +---------------------------------------
 1 file changed, 2 insertions(+), 152 deletions(-)

diff --git a/arch/mips/jz4740/time.c b/arch/mips/jz4740/time.c
index 2ca9160f642a..9a61bca8e9f0 100644
--- a/arch/mips/jz4740/time.c
+++ b/arch/mips/jz4740/time.c
@@ -13,164 +13,14 @@
  *
  */
 
-#include <linux/clk.h>
 #include <linux/clk-provider.h>
-#include <linux/interrupt.h>
-#include <linux/kernel.h>
-#include <linux/time.h>
+#include <linux/clocksource.h>
 
-#include <linux/clockchips.h>
-#include <linux/sched_clock.h>
-
-#include <asm/mach-jz4740/clock.h>
-#include <asm/mach-jz4740/irq.h>
 #include <asm/mach-jz4740/timer.h>
-#include <asm/time.h>
-
-#include "clock.h"
-
-#define TIMER_CLOCKEVENT 0
-#define TIMER_CLOCKSOURCE 1
-
-static uint16_t jz4740_jiffies_per_tick;
-
-static u64 jz4740_clocksource_read(struct clocksource *cs)
-{
-	return jz4740_timer_get_count(TIMER_CLOCKSOURCE);
-}
-
-static struct clocksource jz4740_clocksource = {
-	.name = "jz4740-timer",
-	.rating = 200,
-	.read = jz4740_clocksource_read,
-	.mask = CLOCKSOURCE_MASK(16),
-	.flags = CLOCK_SOURCE_IS_CONTINUOUS,
-};
-
-static u64 notrace jz4740_read_sched_clock(void)
-{
-	return jz4740_timer_get_count(TIMER_CLOCKSOURCE);
-}
-
-static irqreturn_t jz4740_clockevent_irq(int irq, void *devid)
-{
-	struct clock_event_device *cd = devid;
-
-	jz4740_timer_ack_full(TIMER_CLOCKEVENT);
-
-	if (!clockevent_state_periodic(cd))
-		jz4740_timer_disable(TIMER_CLOCKEVENT);
-
-	cd->event_handler(cd);
-
-	return IRQ_HANDLED;
-}
-
-static int jz4740_clockevent_set_periodic(struct clock_event_device *evt)
-{
-	jz4740_timer_set_count(TIMER_CLOCKEVENT, 0);
-	jz4740_timer_set_period(TIMER_CLOCKEVENT, jz4740_jiffies_per_tick);
-	jz4740_timer_irq_full_enable(TIMER_CLOCKEVENT);
-	jz4740_timer_enable(TIMER_CLOCKEVENT);
-
-	return 0;
-}
-
-static int jz4740_clockevent_resume(struct clock_event_device *evt)
-{
-	jz4740_timer_irq_full_enable(TIMER_CLOCKEVENT);
-	jz4740_timer_enable(TIMER_CLOCKEVENT);
-
-	return 0;
-}
-
-static int jz4740_clockevent_shutdown(struct clock_event_device *evt)
-{
-	jz4740_timer_disable(TIMER_CLOCKEVENT);
-
-	return 0;
-}
-
-static int jz4740_clockevent_set_next(unsigned long evt,
-	struct clock_event_device *cd)
-{
-	jz4740_timer_set_count(TIMER_CLOCKEVENT, 0);
-	jz4740_timer_set_period(TIMER_CLOCKEVENT, evt);
-	jz4740_timer_enable(TIMER_CLOCKEVENT);
-
-	return 0;
-}
-
-static struct clock_event_device jz4740_clockevent = {
-	.name = "jz4740-timer",
-	.features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
-	.set_next_event = jz4740_clockevent_set_next,
-	.set_state_shutdown = jz4740_clockevent_shutdown,
-	.set_state_periodic = jz4740_clockevent_set_periodic,
-	.set_state_oneshot = jz4740_clockevent_shutdown,
-	.tick_resume = jz4740_clockevent_resume,
-	.rating = 200,
-#ifdef CONFIG_MACH_JZ4740
-	.irq = JZ4740_IRQ_TCU0,
-#endif
-#if defined(CONFIG_MACH_JZ4770) || defined(CONFIG_MACH_JZ4780)
-	.irq = JZ4780_IRQ_TCU2,
-#endif
-};
-
-static struct irqaction timer_irqaction = {
-	.handler	= jz4740_clockevent_irq,
-	.flags		= IRQF_PERCPU | IRQF_TIMER,
-	.name		= "jz4740-timerirq",
-	.dev_id		= &jz4740_clockevent,
-};
 
 void __init plat_time_init(void)
 {
-	int ret;
-	uint32_t clk_rate;
-	uint16_t ctrl;
-	struct clk *ext_clk;
-
 	of_clk_init(NULL);
 	jz4740_timer_init();
-
-	ext_clk = clk_get(NULL, "ext");
-	if (IS_ERR(ext_clk))
-		panic("unable to get ext clock");
-	clk_rate = clk_get_rate(ext_clk) >> 4;
-	clk_put(ext_clk);
-
-	jz4740_jiffies_per_tick = DIV_ROUND_CLOSEST(clk_rate, HZ);
-
-	clockevent_set_clock(&jz4740_clockevent, clk_rate);
-	jz4740_clockevent.min_delta_ns = clockevent_delta2ns(100, &jz4740_clockevent);
-	jz4740_clockevent.min_delta_ticks = 100;
-	jz4740_clockevent.max_delta_ns = clockevent_delta2ns(0xffff, &jz4740_clockevent);
-	jz4740_clockevent.max_delta_ticks = 0xffff;
-	jz4740_clockevent.cpumask = cpumask_of(0);
-
-	clockevents_register_device(&jz4740_clockevent);
-
-	ret = clocksource_register_hz(&jz4740_clocksource, clk_rate);
-
-	if (ret)
-		printk(KERN_ERR "Failed to register clocksource: %d\n", ret);
-
-	sched_clock_register(jz4740_read_sched_clock, 16, clk_rate);
-
-	setup_irq(jz4740_clockevent.irq, &timer_irqaction);
-
-	ctrl = JZ_TIMER_CTRL_PRESCALE_16 | JZ_TIMER_CTRL_SRC_EXT;
-
-	jz4740_timer_set_ctrl(TIMER_CLOCKEVENT, ctrl);
-	jz4740_timer_set_ctrl(TIMER_CLOCKSOURCE, ctrl);
-
-	jz4740_timer_set_period(TIMER_CLOCKEVENT, jz4740_jiffies_per_tick);
-	jz4740_timer_irq_full_enable(TIMER_CLOCKEVENT);
-
-	jz4740_timer_set_period(TIMER_CLOCKSOURCE, 0xffff);
-
-	jz4740_timer_enable(TIMER_CLOCKEVENT);
-	jz4740_timer_enable(TIMER_CLOCKSOURCE);
+	timer_probe();
 }
-- 
2.21.0.593.g511ec345e18

Re: Ingenic Timer/Counter Unit (TCU) patchset v12

[Mathieu Malaterre]

On Tue, May 21, 2019 at 4:51 PM Paul Cercueil <paul@crapouillou.net> wrote:
>
> Hi,
>
> Here's the V12 of my patchset to add support for the Timer/Counter Unit
> (TCU) present on the JZ47xx SoCs from Ingenic.
>
> This patchset is much shorter at only 13 patches vs. 27 patches in V11;
> the remaining patches will be sent in parallel (if applicable) or as a
> follow-up patchset once this one is merged.
>
> In V11 the clocksource maintainers weren't happy with the size of the
> ingenic-timer driver, which included clocks and irqchip setup code.
> On the other hand, devicetree maintainers wanted one single node for
> the TCU hardware since it's effectively just one hardware block.
>
> In this patchset the functionality is cut in four different drivers:
> a MFD one to provide the regmap, probe the children and which provides
> several API functions; a clocks driver; a irqchip driver; a clocksource
> driver. All these drivers work with the same regmap, have the same
> compatible strings, and will probe _with the same devicetree node_.

For the series:

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

System: MIPS Creator CI20

For reference, here is my local patch:

diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi
b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index 1bfac58da5df..e7b7da32f278 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <dt-bindings/clock/jz4780-cgu.h>
+#include <dt-bindings/clock/ingenic,tcu.h>
 #include <dt-bindings/dma/jz4780-dma.h>

 / {
@@ -80,6 +81,15 @@

                interrupt-parent = <&intc>;
                interrupts = <27 26 25>;
+
+               watchdog: watchdog@0 {
+                       compatible = "ingenic,jz4780-watchdog";
+                       reg = <0x0 0xc>;
+
+                       clocks = <&tcu TCU_CLK_WDT>;
+                       clock-names = "wdt";
+               };
+
        };

        rtc_dev: rtc@10003000 {
@@ -287,14 +297,6 @@
                status = "disabled";
        };

-       watchdog: watchdog@10002000 {
-               compatible = "ingenic,jz4780-watchdog";
-               reg = <0x10002000 0x10>;
-
-               clocks = <&cgu JZ4780_CLK_RTCLK>;
-               clock-names = "rtc";
-       };
-
        nemc: nemc@13410000 {
                compatible = "ingenic,jz4780-nemc";
                reg = <0x13410000 0x10000>;



> Regards,
> -Paul
>
>