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

[Tero Kristo <t-kristo@ti.com>]

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