[3/3] clk: ti: dra7xx: add timer_sys_ck clock alias

Message ID 1565183079-27798-4-git-send-email-t-kristo@ti.com
State New
Headers show
Series
  • clk: ti: couple of fixes towards 5.4
Related show

Commit Message

Tero Kristo Aug. 7, 2019, 1:04 p.m.
This is needed by the TI DM timer driver.

Signed-off-by: Tero Kristo <t-kristo@ti.com>

---
 drivers/clk/ti/clk-7xx.c | 1 +
 1 file changed, 1 insertion(+)

-- 
1.9.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Comments

Suman Anna Aug. 7, 2019, 10:56 p.m. | #1
Hi Tero,

On 8/7/19 8:04 AM, Tero Kristo wrote:
> This is needed by the TI DM timer driver.


Again can do with some better patch descriptions. Similar to the
previous patch, missing the equivalent patches for OMAP4 and OMAP5.
You can use my downstream patches for these - [1][2][3] that has all the
needed Fixes by details. Only difference is that you used a single line
change on DRA7, and this should suffice since all the sources are same,
but OMAP4 and OMAP5 needed different ones.

[1] OMAP4:
http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=9d45dc42fbed8395d733366dbf6c0fd5ec171e2f
[2] OMAP5:
http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=34f4682a91173386307b310d7f4955d46dcaaea2
[3] DRA7:
http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=2a662694437ae7192b5ef759ec40abe796d2a058

Technically, this data need to be added back for all OMAP2+ SoCs which
support dmtimer with any other drivers wanting to use the timers.

regards
Suman

> 

> Signed-off-by: Tero Kristo <t-kristo@ti.com>

> ---

>  drivers/clk/ti/clk-7xx.c | 1 +

>  1 file changed, 1 insertion(+)

> 

> diff --git a/drivers/clk/ti/clk-7xx.c b/drivers/clk/ti/clk-7xx.c

> index 5208eb8..64507b8 100644

> --- a/drivers/clk/ti/clk-7xx.c

> +++ b/drivers/clk/ti/clk-7xx.c

> @@ -792,6 +792,7 @@

>  static struct ti_dt_clk dra7xx_clks[] = {

>  	DT_CLK(NULL, "timer_32k_ck", "sys_32k_ck"),

>  	DT_CLK(NULL, "sys_clkin_ck", "timer_sys_clk_div"),

> +	DT_CLK(NULL, "timer_sys_ck", "timer_sys_clk_div"),

>  	DT_CLK(NULL, "sys_clkin", "sys_clkin1"),

>  	DT_CLK(NULL, "atl_dpll_clk_mux", "atl-clkctrl:0000:24"),

>  	DT_CLK(NULL, "atl_gfclk_mux", "atl-clkctrl:0000:26"),

>
Suman Anna Aug. 19, 2019, 9:14 p.m. | #2
Hi Tero,

On 8/7/19 5:56 PM, Anna, Suman wrote:
> Hi Tero,

> 

> On 8/7/19 8:04 AM, Tero Kristo wrote:

>> This is needed by the TI DM timer driver.

> 

> Again can do with some better patch descriptions. Similar to the

> previous patch, missing the equivalent patches for OMAP4 and OMAP5.

> You can use my downstream patches for these - [1][2][3] that has all the

> needed Fixes by details. Only difference is that you used a single line

> change on DRA7, and this should suffice since all the sources are same,

> but OMAP4 and OMAP5 needed different ones.

> 

> [1] OMAP4:

> http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=9d45dc42fbed8395d733366dbf6c0fd5ec171e2f

> [2] OMAP5:

> http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=34f4682a91173386307b310d7f4955d46dcaaea2

> [3] DRA7:

> http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=2a662694437ae7192b5ef759ec40abe796d2a058

> 

> Technically, this data need to be added back for all OMAP2+ SoCs which

> support dmtimer with any other drivers wanting to use the timers.


So, I checked and these aliases already are defined on OMAP2, OMAP3,
AM33xx, AM43xx, DM814x and DM816x SoCs. So, just include the OMAP4 and
OMAP5 ones along with the DRA7x one.

regards
Suman

> 

> regards

> Suman

> 

>>

>> Signed-off-by: Tero Kristo <t-kristo@ti.com>

>> ---

>>  drivers/clk/ti/clk-7xx.c | 1 +

>>  1 file changed, 1 insertion(+)

>>

>> diff --git a/drivers/clk/ti/clk-7xx.c b/drivers/clk/ti/clk-7xx.c

>> index 5208eb8..64507b8 100644

>> --- a/drivers/clk/ti/clk-7xx.c

>> +++ b/drivers/clk/ti/clk-7xx.c

>> @@ -792,6 +792,7 @@

>>  static struct ti_dt_clk dra7xx_clks[] = {

>>  	DT_CLK(NULL, "timer_32k_ck", "sys_32k_ck"),

>>  	DT_CLK(NULL, "sys_clkin_ck", "timer_sys_clk_div"),

>> +	DT_CLK(NULL, "timer_sys_ck", "timer_sys_clk_div"),

>>  	DT_CLK(NULL, "sys_clkin", "sys_clkin1"),

>>  	DT_CLK(NULL, "atl_dpll_clk_mux", "atl-clkctrl:0000:24"),

>>  	DT_CLK(NULL, "atl_gfclk_mux", "atl-clkctrl:0000:26"),

>>

>
Tero Kristo Aug. 23, 2019, 6:16 p.m. | #3
On 20.8.2019 0.14, Suman Anna wrote:
> Hi Tero,

> 

> On 8/7/19 5:56 PM, Anna, Suman wrote:

>> Hi Tero,

>>

>> On 8/7/19 8:04 AM, Tero Kristo wrote:

>>> This is needed by the TI DM timer driver.

>>

>> Again can do with some better patch descriptions. Similar to the

>> previous patch, missing the equivalent patches for OMAP4 and OMAP5.

>> You can use my downstream patches for these - [1][2][3] that has all the

>> needed Fixes by details. Only difference is that you used a single line

>> change on DRA7, and this should suffice since all the sources are same,

>> but OMAP4 and OMAP5 needed different ones.

>>

>> [1] OMAP4:

>> http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=9d45dc42fbed8395d733366dbf6c0fd5ec171e2f

>> [2] OMAP5:

>> http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=34f4682a91173386307b310d7f4955d46dcaaea2

>> [3] DRA7:

>> http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=2a662694437ae7192b5ef759ec40abe796d2a058

>>

>> Technically, this data need to be added back for all OMAP2+ SoCs which

>> support dmtimer with any other drivers wanting to use the timers.

> 

> So, I checked and these aliases already are defined on OMAP2, OMAP3,

> AM33xx, AM43xx, DM814x and DM816x SoCs. So, just include the OMAP4 and

> OMAP5 ones along with the DRA7x one.


Actually, all these alias definitions can be completely removed, and can 
be replaced with DT data. Here's sample how it can be done for dra7xx 
timer11:

diff --git a/arch/arm/boot/dts/dra7-l4.dtsi b/arch/arm/boot/dts/dra7-l4.dtsi
index bed67603c186..fafa0a131af0 100644
--- a/arch/arm/boot/dts/dra7-l4.dtsi
+++ b/arch/arm/boot/dts/dra7-l4.dtsi
@@ -1910,8 +1910,8 @@
                         timer11: timer@0 {
                                 compatible = "ti,omap5430-timer";
                                 reg = <0x0 0x80>;
-                               clocks = <&l4per_clkctrl 
DRA7_L4PER_TIMER11_CLKCTRL 24>;
-                               clock-names = "fck";
+                               clocks = <&l4per_clkctrl 
DRA7_L4PER_TIMER11_CLKCTRL 24>, <&timer_sys_clk_div>;
+                               clock-names = "fck", "timer_sys_ck";
                                 interrupts = <GIC_SPI 42 
IRQ_TYPE_LEVEL_HIGH>;
                         };
                 };

I will post these changes along with other DTS patches once the time is 
right. For now, I will just drop these aliases completely.

-Tero

> 

> regards

> Suman

> 

>>

>> regards

>> Suman

>>

>>>

>>> Signed-off-by: Tero Kristo <t-kristo@ti.com>

>>> ---

>>>   drivers/clk/ti/clk-7xx.c | 1 +

>>>   1 file changed, 1 insertion(+)

>>>

>>> diff --git a/drivers/clk/ti/clk-7xx.c b/drivers/clk/ti/clk-7xx.c

>>> index 5208eb8..64507b8 100644

>>> --- a/drivers/clk/ti/clk-7xx.c

>>> +++ b/drivers/clk/ti/clk-7xx.c

>>> @@ -792,6 +792,7 @@

>>>   static struct ti_dt_clk dra7xx_clks[] = {

>>>   	DT_CLK(NULL, "timer_32k_ck", "sys_32k_ck"),

>>>   	DT_CLK(NULL, "sys_clkin_ck", "timer_sys_clk_div"),

>>> +	DT_CLK(NULL, "timer_sys_ck", "timer_sys_clk_div"),

>>>   	DT_CLK(NULL, "sys_clkin", "sys_clkin1"),

>>>   	DT_CLK(NULL, "atl_dpll_clk_mux", "atl-clkctrl:0000:24"),

>>>   	DT_CLK(NULL, "atl_gfclk_mux", "atl-clkctrl:0000:26"),

>>>

>>

> 


--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Suman Anna Aug. 26, 2019, 10:26 p.m. | #4
Hi Tero,

On 8/23/19 1:16 PM, Tero Kristo wrote:
>>>

>>> On 8/7/19 8:04 AM, Tero Kristo wrote:

>>>> This is needed by the TI DM timer driver.

>>>

>>> Again can do with some better patch descriptions. Similar to the

>>> previous patch, missing the equivalent patches for OMAP4 and OMAP5.

>>> You can use my downstream patches for these - [1][2][3] that has all the

>>> needed Fixes by details. Only difference is that you used a single line

>>> change on DRA7, and this should suffice since all the sources are same,

>>> but OMAP4 and OMAP5 needed different ones.

>>>

>>> [1] OMAP4:

>>> http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=9d45dc42fbed8395d733366dbf6c0fd5ec171e2f

>>>

>>> [2] OMAP5:

>>> http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=34f4682a91173386307b310d7f4955d46dcaaea2

>>>

>>> [3] DRA7:

>>> http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=2a662694437ae7192b5ef759ec40abe796d2a058

>>>

>>>

>>> Technically, this data need to be added back for all OMAP2+ SoCs which

>>> support dmtimer with any other drivers wanting to use the timers.

>>

>> So, I checked and these aliases already are defined on OMAP2, OMAP3,

>> AM33xx, AM43xx, DM814x and DM816x SoCs. So, just include the OMAP4 and

>> OMAP5 ones along with the DRA7x one.

> 

> Actually, all these alias definitions can be completely removed, and can

> be replaced with DT data. Here's sample how it can be done for dra7xx

> timer11:

> 

> diff --git a/arch/arm/boot/dts/dra7-l4.dtsi

> b/arch/arm/boot/dts/dra7-l4.dtsi

> index bed67603c186..fafa0a131af0 100644

> --- a/arch/arm/boot/dts/dra7-l4.dtsi

> +++ b/arch/arm/boot/dts/dra7-l4.dtsi

> @@ -1910,8 +1910,8 @@

>                         timer11: timer@0 {

>                                 compatible = "ti,omap5430-timer";

>                                 reg = <0x0 0x80>;

> -                               clocks = <&l4per_clkctrl

> DRA7_L4PER_TIMER11_CLKCTRL 24>;

> -                               clock-names = "fck";

> +                               clocks = <&l4per_clkctrl

> DRA7_L4PER_TIMER11_CLKCTRL 24>, <&timer_sys_clk_div>;

> +                               clock-names = "fck", "timer_sys_ck";

>                                 interrupts = <GIC_SPI 42

> IRQ_TYPE_LEVEL_HIGH>;

>                         };

>                 };

> 

> I will post these changes along with other DTS patches once the time is

> right. For now, I will just drop these aliases completely.


I am not sure if this is gonna buy us anything and if it is scalable.
The added clock is neither a functional clock nor an optional clock of
the timer device, but is just a name to use to set the clock parent. Are
you going to add the aliases from clk-<soc>.h to all the device nodes?
DRA7 dmtimers can actually be parented from one of 13 clocks (driver was
never updated to support those).

Given that the dmtimers can only be requested using phandle on DT boots,
it is possible to eliminate the naming and rely on
assigned-clock-parents in either the dmtimer nodes or the client nodes
(provided all the clock parents are listed in dts), and eliminate this
set_source logic.

regards
Suman

> 

> -Tero

> 

>>

>> regards

>> Suman

>>

>>>

>>> regards

>>> Suman

>>>

>>>>

>>>> Signed-off-by: Tero Kristo <t-kristo@ti.com>

>>>> ---

>>>>   drivers/clk/ti/clk-7xx.c | 1 +

>>>>   1 file changed, 1 insertion(+)

>>>>

>>>> diff --git a/drivers/clk/ti/clk-7xx.c b/drivers/clk/ti/clk-7xx.c

>>>> index 5208eb8..64507b8 100644

>>>> --- a/drivers/clk/ti/clk-7xx.c

>>>> +++ b/drivers/clk/ti/clk-7xx.c

>>>> @@ -792,6 +792,7 @@

>>>>   static struct ti_dt_clk dra7xx_clks[] = {

>>>>       DT_CLK(NULL, "timer_32k_ck", "sys_32k_ck"),

>>>>       DT_CLK(NULL, "sys_clkin_ck", "timer_sys_clk_div"),

>>>> +    DT_CLK(NULL, "timer_sys_ck", "timer_sys_clk_div"),

>>>>       DT_CLK(NULL, "sys_clkin", "sys_clkin1"),

>>>>       DT_CLK(NULL, "atl_dpll_clk_mux", "atl-clkctrl:0000:24"),

>>>>       DT_CLK(NULL, "atl_gfclk_mux", "atl-clkctrl:0000:26"),

>>>>

>>>

>>

> 

> -- 

> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.

> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Tero Kristo Aug. 27, 2019, 5:57 a.m. | #5
On 27.8.2019 1.26, Suman Anna wrote:
> Hi Tero,

> 

> On 8/23/19 1:16 PM, Tero Kristo wrote:

>>>>

>>>> On 8/7/19 8:04 AM, Tero Kristo wrote:

>>>>> This is needed by the TI DM timer driver.

>>>>

>>>> Again can do with some better patch descriptions. Similar to the

>>>> previous patch, missing the equivalent patches for OMAP4 and OMAP5.

>>>> You can use my downstream patches for these - [1][2][3] that has all the

>>>> needed Fixes by details. Only difference is that you used a single line

>>>> change on DRA7, and this should suffice since all the sources are same,

>>>> but OMAP4 and OMAP5 needed different ones.

>>>>

>>>> [1] OMAP4:

>>>> http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=9d45dc42fbed8395d733366dbf6c0fd5ec171e2f

>>>>

>>>> [2] OMAP5:

>>>> http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=34f4682a91173386307b310d7f4955d46dcaaea2

>>>>

>>>> [3] DRA7:

>>>> http://git.ti.com/gitweb/?p=rpmsg/remoteproc.git;a=commit;h=2a662694437ae7192b5ef759ec40abe796d2a058

>>>>

>>>>

>>>> Technically, this data need to be added back for all OMAP2+ SoCs which

>>>> support dmtimer with any other drivers wanting to use the timers.

>>>

>>> So, I checked and these aliases already are defined on OMAP2, OMAP3,

>>> AM33xx, AM43xx, DM814x and DM816x SoCs. So, just include the OMAP4 and

>>> OMAP5 ones along with the DRA7x one.

>>

>> Actually, all these alias definitions can be completely removed, and can

>> be replaced with DT data. Here's sample how it can be done for dra7xx

>> timer11:

>>

>> diff --git a/arch/arm/boot/dts/dra7-l4.dtsi

>> b/arch/arm/boot/dts/dra7-l4.dtsi

>> index bed67603c186..fafa0a131af0 100644

>> --- a/arch/arm/boot/dts/dra7-l4.dtsi

>> +++ b/arch/arm/boot/dts/dra7-l4.dtsi

>> @@ -1910,8 +1910,8 @@

>>                          timer11: timer@0 {

>>                                  compatible = "ti,omap5430-timer";

>>                                  reg = <0x0 0x80>;

>> -                               clocks = <&l4per_clkctrl

>> DRA7_L4PER_TIMER11_CLKCTRL 24>;

>> -                               clock-names = "fck";

>> +                               clocks = <&l4per_clkctrl

>> DRA7_L4PER_TIMER11_CLKCTRL 24>, <&timer_sys_clk_div>;

>> +                               clock-names = "fck", "timer_sys_ck";

>>                                  interrupts = <GIC_SPI 42

>> IRQ_TYPE_LEVEL_HIGH>;

>>                          };

>>                  };

>>

>> I will post these changes along with other DTS patches once the time is

>> right. For now, I will just drop these aliases completely.

> 

> I am not sure if this is gonna buy us anything and if it is scalable.

> The added clock is neither a functional clock nor an optional clock of

> the timer device, but is just a name to use to set the clock parent. Are

> you going to add the aliases from clk-<soc>.h to all the device nodes?

> DRA7 dmtimers can actually be parented from one of 13 clocks (driver was

> never updated to support those).


No, adding all of these has no point.

> 

> Given that the dmtimers can only be requested using phandle on DT boots,

> it is possible to eliminate the naming and rely on

> assigned-clock-parents in either the dmtimer nodes or the client nodes

> (provided all the clock parents are listed in dts), and eliminate this

> set_source logic.


Either way works, however the alias mechanism provided inside the TI 
clock driver was meant to be temporary only when it was introduced a few 
years back... Any clock handles needed by drivers should be provided via DT.

If you need re-parenting of things, using assigned-clocks would be ideal.

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Patch

diff --git a/drivers/clk/ti/clk-7xx.c b/drivers/clk/ti/clk-7xx.c
index 5208eb8..64507b8 100644
--- a/drivers/clk/ti/clk-7xx.c
+++ b/drivers/clk/ti/clk-7xx.c
@@ -792,6 +792,7 @@ 
 static struct ti_dt_clk dra7xx_clks[] = {
 	DT_CLK(NULL, "timer_32k_ck", "sys_32k_ck"),
 	DT_CLK(NULL, "sys_clkin_ck", "timer_sys_clk_div"),
+	DT_CLK(NULL, "timer_sys_ck", "timer_sys_clk_div"),
 	DT_CLK(NULL, "sys_clkin", "sys_clkin1"),
 	DT_CLK(NULL, "atl_dpll_clk_mux", "atl-clkctrl:0000:24"),
 	DT_CLK(NULL, "atl_gfclk_mux", "atl-clkctrl:0000:26"),