ARM: imx: make the imx timer driver implementation independent of SoCs.

PATCH New Shenwei Wang 3 participants 11 messages Apr 29, 2015 – Apr 30, 2015

Subsystems (3)

Arm (32-bit) Arm sub-architectures Arm i.MX and Layerscape

lore

Share & tools

lore

Shenwei Wang

author

2015-04-29 14:14 UTC #0 lore raw

PATCH 1/1 ARM: imx: make the imx timer driver implementation independent of SoCs.

There are 4 versions of the timer hardware on Freescale MXC hardware. --Version 0: MX1/MXL
--Version 1: MX21, MX27.
--Version 2: MX25, MX31, MX35, MX37, MX51, MX6Q
--Version 3: MX6DL, MX6SX

This patch has removed the SoC related codes, and implemented the driver directly upon the hardware timer IP version.

The new driver can be installed via device tree or the direct function call to mxc_timer_init in order to support imx legacy systems like MX21 and MX27.

For the device tree implementation, the driver is compatible with the current bindings like "fsl,imx6q-gpt", but for future dts file, the string like "fsl,imx-gpt-v2" without SoC information is recommended.

Signed-off-by Shenwei Wang <[email protected]>

---
 arch/arm/mach-imx/clk-imx1.c  |   2 +-
 arch/arm/mach-imx/clk-imx21.c |   3 +-
 arch/arm/mach-imx/clk-imx27.c |   3 +-
 arch/arm/mach-imx/clk-imx31.c |   3 +-
 arch/arm/mach-imx/clk-imx35.c |   5 +-
 arch/arm/mach-imx/common.h    |   2 +-
 arch/arm/mach-imx/time.c      | 435 ++++++++++++++++++++++++++++++------------
 7 files changed, 328 insertions(+), 125 deletions(-)

arch/arm/mach-imx/clk-imx1.c +1 -1

index 37c307a..57ba8d5 100644
@@ -98,7 +98,7 @@ int __init mx1_clocks_init(unsigned long fref)
 	clk_register_clkdev(clk[IMX1_CLK_DUMMY], "ipg", "imx1-fb.0");
 	clk_register_clkdev(clk[IMX1_CLK_DUMMY], "ahb", "imx1-fb.0");
 
-	mxc_timer_init(MX1_IO_ADDRESS(MX1_TIM1_BASE_ADDR), MX1_TIM1_INT);
+	mxc_timer_init(MX1_IO_ADDRESS(MX1_TIM1_BASE_ADDR), MX1_TIM1_INT, 0);
 
 	return 0;
 }

arch/arm/mach-imx/clk-imx21.c +2 -1

index 4b4c753..21a1061 100644
@@ -153,7 +153,8 @@ int __init mx21_clocks_init(unsigned long lref, unsigned long href)
 	clk_register_clkdev(clk[IMX21_CLK_I2C_GATE], NULL, "imx21-i2c.0");
 	clk_register_clkdev(clk[IMX21_CLK_OWIRE_GATE], NULL, "mxc_w1.0");
 
-	mxc_timer_init(MX21_IO_ADDRESS(MX21_GPT1_BASE_ADDR), MX21_INT_GPT1);
+	/*Timer version in MX21 is v1*/
+	mxc_timer_init(MX21_IO_ADDRESS(MX21_GPT1_BASE_ADDR), MX21_INT_GPT1, 1);
 
 	return 0;
 }

arch/arm/mach-imx/clk-imx27.c +2 -1

index ab6349e..0cf532c 100644
@@ -229,7 +229,8 @@ int __init mx27_clocks_init(unsigned long fref)
 	clk_register_clkdev(clk[IMX27_CLK_EMMA_AHB_GATE], "ahb", "m2m-emmaprp.0");
 	clk_register_clkdev(clk[IMX27_CLK_EMMA_IPG_GATE], "ipg", "m2m-emmaprp.0");
 
-	mxc_timer_init(MX27_IO_ADDRESS(MX27_GPT1_BASE_ADDR), MX27_INT_GPT1);
+	/*Timer version in MX27 is v1*/
+	mxc_timer_init(MX27_IO_ADDRESS(MX27_GPT1_BASE_ADDR), MX27_INT_GPT1, 1);
 
 	return 0;
 }

arch/arm/mach-imx/clk-imx31.c +2 -1

index 286ef422..c863390 100644
@@ -182,7 +182,8 @@ int __init mx31_clocks_init(unsigned long fref)
 	mx31_revision();
 	clk_disable_unprepare(clk[iim_gate]);
 
-	mxc_timer_init(MX31_IO_ADDRESS(MX31_GPT1_BASE_ADDR), MX31_INT_GPT);
+	/*Timer version in MX31 is v2*/
+	mxc_timer_init(MX31_IO_ADDRESS(MX31_GPT1_BASE_ADDR), MX31_INT_GPT, 2);
 
 	return 0;
 }

arch/arm/mach-imx/clk-imx35.c +3 -2

index a0d2b57..532fc21 100644
@@ -128,7 +128,7 @@ int __init mx35_clocks_init(void)
 	clk[esdhc3_div] = imx_clk_divider("esdhc3_div", "esdhc_sel", base + MX35_CCM_PDR3, 16, 6);
 
 	clk[spdif_sel] = imx_clk_mux("spdif_sel", base + MX35_CCM_PDR3, 22, 1, std_sel, ARRAY_SIZE(std_sel));
-	clk[spdif_div_pre] = imx_clk_divider("spdif_div_pre", "spdif_sel", base + MX35_CCM_PDR3, 29, 3); /* divide by 1 not allowed */ 
+	clk[spdif_div_pre] = imx_clk_divider("spdif_div_pre", "spdif_sel", base + MX35_CCM_PDR3, 29, 3); /* divide by 1 not allowed */
 	clk[spdif_div_post] = imx_clk_divider("spdif_div_post", "spdif_div_pre", base + MX35_CCM_PDR3, 23, 6);
 
 	clk[ssi_sel] = imx_clk_mux("ssi_sel", base + MX35_CCM_PDR2, 6, 1, std_sel, ARRAY_SIZE(std_sel));

@@ -279,7 +279,8 @@ int __init mx35_clocks_init(void)
 #ifdef CONFIG_MXC_USE_EPIT
 	epit_timer_init(MX35_IO_ADDRESS(MX35_EPIT1_BASE_ADDR), MX35_INT_EPIT1);
 #else
-	mxc_timer_init(MX35_IO_ADDRESS(MX35_GPT1_BASE_ADDR), MX35_INT_GPT);
+	/*Timer version in MX35 is v2*/
+	mxc_timer_init(MX35_IO_ADDRESS(MX35_GPT1_BASE_ADDR), MX35_INT_GPT, 2);
 #endif
 
 	return 0;

arch/arm/mach-imx/common.h +1 -1

index 0f04e30..851ae58 100644
@@ -44,7 +44,7 @@ void imx27_soc_init(void);
 void imx31_soc_init(void);
 void imx35_soc_init(void);
 void epit_timer_init(void __iomem *base, int irq);
-void mxc_timer_init(void __iomem *, int);
+void mxc_timer_init(void __iomem *, int, int);
 int mx1_clocks_init(unsigned long fref);
 int mx21_clocks_init(unsigned long lref, unsigned long fref);
 int mx27_clocks_init(unsigned long fref);

arch/arm/mach-imx/time.c +317 ~~-118~~

577 lines not on this page · raw message

Signature · 1 line

1.9.1

Baruch Siach

2015-04-29 14:26 UTC #1 lore raw

PATCH 1/1 ARM: imx: make the imx timer driver implementation independent of SoCs.

Hi Shenwei Wang,

Quoted 9 lines · Shenwei Wang

There are 4 versions of the timer hardware on Freescale MXC hardware. --Version 0: MX1/MXL
--Version 1: MX21, MX27.
--Version 2: MX25, MX31, MX35, MX37, MX51, MX6Q
--Version 3: MX6DL, MX6SX
This patch has removed the SoC related codes, and implemented the driver directly upon the hardware timer IP version.

Quoted 7 lines

The new driver can be installed via device tree or the direct function call to mxc_timer_init in order to support imx legacy systems like MX21 and MX27.
For the device tree implementation, the driver is compatible with the current bindings like "fsl,imx6q-gpt", but for future dts file, the string like "fsl,imx-gpt-v2" without SoC information is recommended.

That is not the usual convention for IP block versions.

Please Cc the devicetree list (added).

baruch

Signature · 3 lines

     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Shenwei Wang

author

2015-04-29 14:55 UTC #2 lore raw

PATCH 1/1 ARM: imx: make the imx timer driver implementation independent of SoCs.

Quoted 25 lines

-----Original Message-----
From: Baruch Siach [mailto:baruch at tkos.co.il]
Sent: 2015?4?29? 9:26
To: Wang Shenwei-B38339
Cc: shawn.guo at linaro.org; linux-arm-kernel at lists.infradead.org; devicetree at vger.kernel.org
Subject: Re: [PATCH 1/1] ARM: imx: make the imx timer driver implementation independent of SoCs.
Hi Shenwei Wang,
Quoted 13 lines · Shenwei Wang
There are 4 versions of the timer hardware on Freescale MXC hardware. --Version 0: MX1/MXL
--Version 1: MX21, MX27.
--Version 2: MX25, MX31, MX35, MX37, MX51, MX6Q --Version 3: MX6DL, MX6SX
This patch has removed the SoC related codes, and implemented the driver directly upon the hardware timer IP version.
The new driver can be installed via device tree or the direct function call to mxc_timer_init in order to support imx legacy systems like MX21 and MX27.

Quoted 8 lines

Quoted 3 lines
For the device tree implementation, the driver is compatible with the current bindings like "fsl,imx6q-gpt", but for future dts file, the string like "fsl,imx-gpt-v2" without SoC information is recommended.
That is not the usual convention for IP block versions.
Please Cc the devicetree list (added).

Thank you for the comments. What is the current naming rules for IP block version? It would be appreciated if you could provide an example.

Shenwei

Quoted 7 lines

baruch
--
http://baruch.siach.name/blog/ ~. .~ Tk Open
Systems =}------------------------------------------------ooO--U--Ooo------------{=
- baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Baruch Siach

2015-04-29 15:08 UTC #3 lore raw

PATCH 1/1 ARM: imx: make the imx timer driver implementation independent of SoCs.

Hi Shenwei Wang,

Quoted 27 lines · Shenwei Wang

Quoted 27 lines
-----Original Message-----
From: Baruch Siach [mailto:baruch at tkos.co.il]
Sent: 2015?4?29? 9:26
To: Wang Shenwei-B38339
Cc: shawn.guo at linaro.org; linux-arm-kernel at lists.infradead.org; devicetree at vger.kernel.org
Subject: Re: [PATCH 1/1] ARM: imx: make the imx timer driver implementation independent of SoCs.
Quoted 16 lines · Shenwei Wang
There are 4 versions of the timer hardware on Freescale MXC hardware. --Version 0: MX1/MXL
--Version 1: MX21, MX27.
--Version 2: MX25, MX31, MX35, MX37, MX51, MX6Q
--Version 3: MX6DL, MX6SX
This patch has removed the SoC related codes, and implemented the driver directly upon the hardware timer IP version.
The new driver can be installed via device tree or the direct function call to mxc_timer_init in order to support imx legacy systems like MX21 and MX27.
For the device tree implementation, the driver is compatible with the current bindings like "fsl,imx6q-gpt", but for future dts file, the string like "fsl,imx-gpt-v2" without SoC information is recommended.

Quoted 6 lines

Quoted 3 lines
That is not the usual convention for IP block versions.
Please Cc the devicetree list (added).
Thank you for the comments. What is the current naming rules for IP block version? It would be appreciated if you could provide an example.

When several SoC share the same IP block the usual convention is to name it in the compatible property string after the first SoC it appeared on. Just look at some binding documentation from Documentation/devicetree/bindings/timer/ to find examples. The allwinner,sun5i-a13-hstimer property is shared by A10s and A13 SoCs. The amlogic,meson6-timer is shared by Meson6 and Meson8 SoCs, and so on.

baruch

Signature · 3 lines

     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Shenwei Wang

author

2015-04-29 15:19 UTC #4 lore raw

PATCH 1/1 ARM: imx: make the imx timer driver implementation independent of SoCs.

Hi Baruch,

Quoted 48 lines

-----Original Message-----
From: Baruch Siach [mailto:baruch at tkos.co.il]
Sent: 2015?4?29? 10:08
To: Wang Shenwei-B38339
Cc: shawn.guo at linaro.org; linux-arm-kernel at lists.infradead.org; devicetree at vger.kernel.org
Subject: Re: [PATCH 1/1] ARM: imx: make the imx timer driver implementation independent of SoCs.
Hi Shenwei Wang,
Quoted 19 lines · Shenwei Wang
Quoted 19 lines
-----Original Message-----
From: Baruch Siach [mailto:baruch at tkos.co.il]
Sent: 2015?4?29? 9:26
To: Wang Shenwei-B38339
Cc: shawn.guo at linaro.org; linux-arm-kernel at lists.infradead.org; devicetree at vger.kernel.org
Subject: Re: [PATCH 1/1] ARM: imx: make the imx timer driver implementation independent of SoCs.
Quoted 9 lines · Shenwei Wang
There are 4 versions of the timer hardware on Freescale MXC hardware. --Version 0: MX1/MXL
--Version 1: MX21, MX27.
--Version 2: MX25, MX31, MX35, MX37, MX51, MX6Q --Version 3:
MX6DL, MX6SX
This patch has removed the SoC related codes, and implemented the driver directly upon the hardware timer IP version.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
The new driver can be installed via device tree or the direct function call to mxc_timer_init in order to support imx legacy systems like
MX21 and MX27.
For the device tree implementation, the driver is compatible with the current bindings like "fsl,imx6q-gpt", but for future dts file, the string like "fsl,imx-gpt-v2" without SoC information is
recommended.
Quoted 7 lines
Quoted 4 lines
That is not the usual convention for IP block versions.
Please Cc the devicetree list (added).
Thank you for the comments. What is the current naming rules for IP block version? It would be appreciated if you could provide an example.

Quoted 7 lines

When several SoC share the same IP block the usual convention is to name it in the compatible property string after the first SoC it appeared on. Just look at some binding documentation from Documentation/devicetree/bindings/timer/ to find examples. The allwinner,sun5i-a13-hstimer property is shared by A10s and A13 SoCs. The amlogic,meson6-timer is shared by Meson6 and Meson8 SoCs, and so on.

If the same IP block is shared with several SoCs, why we gave them different compatible strings? If no changes in an IP block, I assume no changes in the relating driver as well. In this assumption, I don't see any need to introduce a new compatible string for an unchanged IP block in a new SoC.

Shenwei

Quoted 7 lines

baruch
--
http://baruch.siach.name/blog/ ~. .~ Tk Open
Systems =}------------------------------------------------ooO--U--Ooo------------{=
- baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Baruch Siach

2015-04-29 15:25 UTC #5 lore raw

PATCH 1/1 ARM: imx: make the imx timer driver implementation independent of SoCs.

Hi Shenwei Wang,

Quoted 17 lines · Shenwei Wang

Quoted 9 lines
-----Original Message-----
From: Baruch Siach [mailto:baruch at tkos.co.il]
Sent: 2015?4?29? 10:08
To: Wang Shenwei-B38339
Cc: shawn.guo at linaro.org; linux-arm-kernel at lists.infradead.org; devicetree at vger.kernel.org
Subject: Re: [PATCH 1/1] ARM: imx: make the imx timer driver implementation independent of SoCs.
Quoted 7 lines
When several SoC share the same IP block the usual convention is to name it in
the compatible property string after the first SoC it appeared on. Just look at some binding documentation from Documentation/devicetree/bindings/timer/ to find examples. The allwinner,sun5i-a13-hstimer property is shared by A10s and A13 SoCs. The amlogic,meson6-timer is shared by Meson6 and Meson8 SoCs, and so on.

Quoted 4 lines

If the same IP block is shared with several SoCs, why we gave them different compatible strings? If no changes in an IP block, I assume no changes in the relating driver as well. In this assumption, I don't see any need to introduce a new compatible string for an unchanged IP block in a new SoC.

That is exactly what I meant to say. Sorry that I was not clear enough.

When the same IP block is used in a newer generation SoC the same compatible property string is used. For that reason you can find in sun5i-a10s.dtsi the line

	compatible = "allwinner,sun5i-a13-hstimer";

baruch

Signature · 3 lines

     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Shenwei Wang

author

2015-04-29 16:34 UTC #6 lore raw

PATCH 1/1 ARM: imx: make the imx timer driver implementation independent of SoCs.

Hi Baruch,

Quoted 34 lines

-----Original Message-----
From: Baruch Siach [mailto:baruch at tkos.co.il]
Sent: 2015?4?29? 10:25
To: Wang Shenwei-B38339
Cc: shawn.guo at linaro.org; linux-arm-kernel at lists.infradead.org; devicetree at vger.kernel.org
Subject: Re: [PATCH 1/1] ARM: imx: make the imx timer driver implementation independent of SoCs.
Hi Shenwei Wang,
Quoted 17 lines · Shenwei Wang
Quoted 9 lines
-----Original Message-----
From: Baruch Siach [mailto:baruch at tkos.co.il]
Sent: 2015?4?29? 10:08
To: Wang Shenwei-B38339
Cc: shawn.guo at linaro.org; linux-arm-kernel at lists.infradead.org; devicetree at vger.kernel.org
Subject: Re: [PATCH 1/1] ARM: imx: make the imx timer driver implementation independent of SoCs.
Quoted 7 lines
When several SoC share the same IP block the usual convention is to name it in the compatible property string after the first SoC it appeared on. Just look at some binding documentation from Documentation/devicetree/bindings/timer/ to find examples. The allwinner,sun5i-a13-hstimer property is shared by A10s and A13 SoCs. The amlogic,meson6-timer is shared by Meson6 and Meson8 SoCs, and so on.
Quoted 2 lines
If the same IP block is shared with several SoCs, why we gave them different compatible strings? If no changes in an IP block, I assume no changes
in the relating driver as well.
Quoted 2 lines
In this assumption, I don't see any need to introduce a new compatible string for an unchanged IP block in a new SoC.

Quoted 7 lines

That is exactly what I meant to say. Sorry that I was not clear enough.
When the same IP block is used in a newer generation SoC the same compatible property string is used. For that reason you can find in sun5i-a10s.dtsi the line
	compatible = "allwinner,sun5i-a13-hstimer";

That is what has been supported in this patch. Besides that, it can also support different IP block versions in a SoC with different revision. For example, Let's say:
hstimer version 1 in A13 revision 1.0
hstimer version 2 in A13 revision 1.1

Shenwei

Quoted 7 lines

baruch
--
http://baruch.siach.name/blog/ ~. .~ Tk Open
Systems =}------------------------------------------------ooO--U--Ooo------------{=
- baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Sascha Hauer

maintainer

2015-04-29 18:57 UTC #7 lore raw

PATCH 1/1 ARM: imx: make the imx timer driver implementation independent of SoCs.

In reply to Shenwei Wang

Quoted 16 lines · Shenwei Wang

There are 4 versions of the timer hardware on Freescale MXC hardware. --Version 0: MX1/MXL
--Version 1: MX21, MX27.
--Version 2: MX25, MX31, MX35, MX37, MX51, MX6Q
--Version 3: MX6DL, MX6SX
This patch has removed the SoC related codes, and implemented the driver directly upon the hardware timer IP version.
The new driver can be installed via device tree or the direct function call to mxc_timer_init in order to support imx legacy systems like MX21 and MX27.
For the device tree implementation, the driver is compatible with the current bindings like "fsl,imx6q-gpt", but for future dts file, the string like "fsl,imx-gpt-v2" without SoC information is recommended.

[...]

arch/arm/mach-imx/time.c static void __init mxc_timer_init_dt(struct device_node *np) · line 579

Quoted 8 lines

arch/arm/mach-imx/time.c +5 -0

+
+CLOCKSOURCE_OF_DECLARE(mx_timer_v0, "fsl,imx-gpt-v0", mxc_timer_init_dt);
+CLOCKSOURCE_OF_DECLARE(mx_timer_v1, "fsl,imx-gpt-v1", mxc_timer_init_dt);
+CLOCKSOURCE_OF_DECLARE(mx_timer_v2, "fsl,imx-gpt-v2", mxc_timer_init_dt);
+CLOCKSOURCE_OF_DECLARE(mx_timer_v3, "fsl,imx-gpt-v3", mxc_timer_init_dt);
 CLOCKSOURCE_OF_DECLARE(mx1_timer, "fsl,imx1-gpt", mxc_timer_init_dt);
 CLOCKSOURCE_OF_DECLARE(mx25_timer, "fsl,imx25-gpt", mxc_timer_init_dt);
 CLOCKSOURCE_OF_DECLARE(mx50_timer, "fsl,imx50-gpt", mxc_timer_init_dt);

Don't make it more complicated than it is. Instead of using mxc_timer_init_dt for all different timers and dispatch the different versions later introduce mxc_timer_init_v[0123] and put this directly into the function callbacks here.

Overall this patch has much too many changes in it. It changes the compatible entries, changes the implementation, splits the two timer versions into four, changes the scope of the clockevent_mode variable and renames it to last_mode. This should be split up into different patches.

I don't like these fsl,imx-gpt-vx compatibles at all. They seem to be pretty useless since all device trees already have a oldest compatible version as second compatible entry. There's no need to create new compatibles.

Sascha

Signature · 4 lines

Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Shenwei Wang

author

2015-04-29 19:56 UTC #8 lore raw

PATCH 1/1 ARM: imx: make the imx timer driver implementation independent of SoCs.

Hi Sacha,

Quoted 44 lines

-----Original Message-----
From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
Sent: 2015?4?29? 13:58
To: Wang Shenwei-B38339
Cc: shawn.guo at linaro.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH 1/1] ARM: imx: make the imx timer driver implementation independent of SoCs.
Quoted 16 lines · Shenwei Wang
There are 4 versions of the timer hardware on Freescale MXC hardware. --Version 0: MX1/MXL
--Version 1: MX21, MX27.
--Version 2: MX25, MX31, MX35, MX37, MX51, MX6Q --Version 3: MX6DL, MX6SX
This patch has removed the SoC related codes, and implemented the driver directly upon the hardware timer IP version.
The new driver can be installed via device tree or the direct function call to mxc_timer_init in order to support imx legacy systems like MX21 and MX27.
For the device tree implementation, the driver is compatible with the current bindings like "fsl,imx6q-gpt", but for future dts file, the string like "fsl,imx-gpt-v2" without SoC information is recommended.
[...]
Quoted 11 lines
(quoted diff) +7 -0
+
+CLOCKSOURCE_OF_DECLARE(mx_timer_v0, "fsl,imx-gpt-v0",
+mxc_timer_init_dt); CLOCKSOURCE_OF_DECLARE(mx_timer_v1,
+"fsl,imx-gpt-v1", mxc_timer_init_dt);
+CLOCKSOURCE_OF_DECLARE(mx_timer_v2, "fsl,imx-gpt-v2",
+mxc_timer_init_dt); CLOCKSOURCE_OF_DECLARE(mx_timer_v3,
+"fsl,imx-gpt-v3", mxc_timer_init_dt);
 CLOCKSOURCE_OF_DECLARE(mx1_timer, "fsl,imx1-gpt", mxc_timer_init_dt);
CLOCKSOURCE_OF_DECLARE(mx25_timer, "fsl,imx25-gpt", mxc_timer_init_dt); CLOCKSOURCE_OF_DECLARE(mx50_timer, "fsl,imx50-gpt", mxc_timer_init_dt);
Don't make it more complicated than it is. Instead of using mxc_timer_init_dt for all different timers and dispatch the different versions later introduce mxc_timer_init_v[0123] and put this directly into the function callbacks here.

Quoted 5 lines

Overall this patch has much too many changes in it. It changes the compatible entries, changes the implementation, splits the two timer versions into four, changes the scope of the clockevent_mode variable and renames it to last_mode. This should be split up into different patches.

The patch did create some trouble for review. I am going to split it into several small patches.

Quoted 4 lines

I don't like these fsl,imx-gpt-vx compatibles at all. They seem to be pretty useless since all device trees already have a oldest compatible version as second compatible entry. There's no need to create new compatibles.

The second compatible string is useless at all in this case. Because the driver will only run correctly on the specified version of IP block. For example, you can not specify a compatible string of "fsl,imx25-gpt" in imx6sx dts file and then suppose it could work on the imx6sx platform. There are still some real needs to let the version of the IP block. For example, there are two revisions of imx6q SoCs. One revision has a v2 timer IP block, and the other revision has a v3 one. Since both revisions are imx6q, one way has to be selected to identify them. So I don't think it is useless. "fsl,imx-gpt-vx" is not a beautiful name, but I think it is a better name than the current one.

Shenwei

Quoted 9 lines

Sascha

--
Pengutronix e.K. |
|

Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555

Sascha Hauer

maintainer

2015-04-30 05:15 UTC #9 lore raw

PATCH 1/1 ARM: imx: make the imx timer driver implementation independent of SoCs.

Quoted 58 lines · Shenwei Wang

Hi Sacha,
Quoted 44 lines
-----Original Message-----
From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
Sent: 2015?4?29? 13:58
To: Wang Shenwei-B38339
Cc: shawn.guo at linaro.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH 1/1] ARM: imx: make the imx timer driver implementation independent of SoCs.
Quoted 16 lines · Shenwei Wang
There are 4 versions of the timer hardware on Freescale MXC hardware. --Version 0: MX1/MXL
--Version 1: MX21, MX27.
--Version 2: MX25, MX31, MX35, MX37, MX51, MX6Q --Version 3: MX6DL, MX6SX
This patch has removed the SoC related codes, and implemented the driver directly upon the hardware timer IP version.
The new driver can be installed via device tree or the direct function call to mxc_timer_init in order to support imx legacy systems like MX21 and MX27.
For the device tree implementation, the driver is compatible with the current bindings like "fsl,imx6q-gpt", but for future dts file, the string like "fsl,imx-gpt-v2" without SoC information is recommended.
[...]
Quoted 11 lines
(quoted diff) +7 -0
+
+CLOCKSOURCE_OF_DECLARE(mx_timer_v0, "fsl,imx-gpt-v0",
+mxc_timer_init_dt); CLOCKSOURCE_OF_DECLARE(mx_timer_v1,
+"fsl,imx-gpt-v1", mxc_timer_init_dt);
+CLOCKSOURCE_OF_DECLARE(mx_timer_v2, "fsl,imx-gpt-v2",
+mxc_timer_init_dt); CLOCKSOURCE_OF_DECLARE(mx_timer_v3,
+"fsl,imx-gpt-v3", mxc_timer_init_dt);
 CLOCKSOURCE_OF_DECLARE(mx1_timer, "fsl,imx1-gpt", mxc_timer_init_dt);
CLOCKSOURCE_OF_DECLARE(mx25_timer, "fsl,imx25-gpt", mxc_timer_init_dt); CLOCKSOURCE_OF_DECLARE(mx50_timer, "fsl,imx50-gpt", mxc_timer_init_dt);
Don't make it more complicated than it is. Instead of using mxc_timer_init_dt for all different timers and dispatch the different versions later introduce mxc_timer_init_v[0123] and put this directly into the function callbacks here.
Quoted 5 lines
Overall this patch has much too many changes in it. It changes the compatible entries, changes the implementation, splits the two timer versions into four, changes the scope of the clockevent_mode variable and renames it to last_mode. This should be split up into different patches.
The patch did create some trouble for review. I am going to split it into several small patches.
Quoted 4 lines
I don't like these fsl,imx-gpt-vx compatibles at all. They seem to be pretty useless since all device trees already have a oldest compatible version as second compatible entry. There's no need to create new compatibles.

Quoted 7 lines

The second compatible string is useless at all in this case. Because the driver will only run correctly on the specified version of IP block. For example, you can not specify a compatible string of "fsl,imx25-gpt" in imx6sx dts file and then suppose it could work on the imx6sx platform. There are still some real needs to let the version of the IP block. For example, there are two revisions of imx6q SoCs. One revision has a v2 timer IP block, and the other revision has a v3 one. Since both revisions are imx6q, one way has to be selected to identify them. So I don't think it is useless. "fsl,imx-gpt-vx" is not a beautiful name, but I think it is a better name than the current one.

The policy so far is:

- use the exact SoC name as primary compatible string
- Use the oldest SoC we *think* is compatible at the time of writing as secondary compatible.

This way we can always match to the oldest compatible in the driver (Thus i.MX7 could get a "i.MX6sx" compatible or whatever). Should we realize later that there are slight differences between timers that we thought would be identical, then we can still change the driver to match against the exact compatible string.

Whether we use the SoC name or Vxy doesn't matter at all. With Vxy we still need to increase the counter with each new SoC since there might be differences discovered later. We can't really say for sure that there only four versions of the timer, only that by now we have identified four versions. On i.MX we chose to match to the SoC name not only for the timer but for all other devices aswell, so let's keep it that way.

That i.MX6q has two different versions of the timer is unfortunate, that needs a new compatible string, but whether that is fsl,imx-gpt-v3 or fsl,imx6q-gpt-newone again doesn't matter.

Sascha

Signature · 4 lines

Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Shenwei Wang

author

2015-04-30 15:16 UTC #10 lore raw

PATCH 1/1 ARM: imx: make the imx timer driver implementation independent of SoCs.

Quoted 29 lines

-----Original Message-----
From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
Sent: 2015?4?30? 0:16
To: Wang Shenwei-B38339
Cc: shawn.guo at linaro.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH 1/1] ARM: imx: make the imx timer driver implementation independent of SoCs.
The policy so far is:
- use the exact SoC name as primary compatible string
- Use the oldest SoC we *think* is compatible at the time of writing as secondary compatible.
This way we can always match to the oldest compatible in the driver (Thus i.MX7 could get a "i.MX6sx" compatible or whatever). Should we realize later that there are slight differences between timers that we thought would be identical, then we can still change the driver to match against the exact compatible string.
Whether we use the SoC name or Vxy doesn't matter at all. With Vxy we still need to increase the counter with each new SoC since there might be differences discovered later. We can't really say for sure that there only four versions of the timer, only that by now we have identified four versions. On i.MX we chose to match to the SoC name not only for the timer but for all other devices aswell, so let's keep it that way.

Quoted 3 lines

That i.MX6q has two different versions of the timer is unfortunate, that needs a new compatible string, but whether that is fsl,imx-gpt-v3 or fsl,imx6q-gpt-newone again doesn't matter.

As it is a rule, we should follow it. But I don?t' think it is a good idea to bind the SoC name with the IP block. It seems even ugly especially in this case. You will have to create a new string for each new SoC even the IP block has no changes at all. Otherwise, you will have to re-use a string containing another SoC's name.

Quoted 10 lines

Sascha

--
Pengutronix e.K. |
|

Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555