Re: [Patch 1/3] ARM: dts: am43xx: add support for clkout1 clock

From: Tero Kristo <t-kristo@ti.com>
To: Benoit Parrot <bparrot@ti.com>, Tony Lindgren <tony@atomide.com>
Cc: Rob Herring <robh+dt@kernel.org>, <linux-omap@vger.kernel.org>,
	<devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [Patch 1/3] ARM: dts: am43xx: add support for clkout1 clock
Date: Thu, 31 Oct 2019 16:06:59 +0200	[thread overview]
Message-ID: <39b79438-ad82-0840-b2a5-36856d0ac520@ti.com> (raw)
In-Reply-To: <20191030195946.ouexmis632nb7lqj@ti.com>

On 30/10/2019 21:59, Benoit Parrot wrote:
> Tony Lindgren <tony@atomide.com> wrote on Wed [2019-Oct-23 08:56:57 -0700]:
>> * Tony Lindgren <tony@atomide.com> [191022 16:56]:
>>> * Tero Kristo <t-kristo@ti.com> [191022 16:48]:
>>>> On 22/10/2019 19:21, Benoit Parrot wrote:
>>>>> Tony Lindgren <tony@atomide.com> wrote on Tue [2019-Oct-22 08:48:16 -0700]:
>>>>>> * Benoit Parrot <bparrot@ti.com> [191016 18:47]:
>>>>>>> --- a/arch/arm/boot/dts/am43xx-clocks.dtsi
>>>>>>> +++ b/arch/arm/boot/dts/am43xx-clocks.dtsi
>>>>>>> @@ -704,6 +704,60 @@
>>>>>>>    		ti,bit-shift = <8>;
>>>>>>>    		reg = <0x2a48>;
>>>>>>>    	};
>>>>>>> +
>>>>>>> +	clkout1_osc_div_ck: clkout1_osc_div_ck {
>>>>>>> +		#clock-cells = <0>;
>>>>>>> +		compatible = "ti,divider-clock";
>>>>>>> +		clocks = <&sys_clkin_ck>;
>>>>>>> +		ti,bit-shift = <20>;
>>>>>>> +		ti,max-div = <4>;
>>>>>>> +		reg = <0x4100>;
>>>>>>> +	};
>>>>>>
>>>>>> Here too please describe why the clock names are not generic.
>>>>>
>>>>> Tero originally had this patch in the kernel so this is somewhat of a
>>>>> revert. Since these "clock" were removed. If the name syntax is no longer
>>>>> valid for some reason, then I will need a little more informations to
>>>>> proceed.
>>>>>
>>>>> Tero, can you assist here?
>>>>
>>>> This one is just following the naming convention of the rest of the clocks
>>>> atm.
>>>>
>>>> If we need to fix all the underscore name clocks, that requires pretty much
>>>> complete revamp of both the dts data + clock data under the clock driver,
>>>> and it is not backwards compatible either. How should we tackle that one?
>>>>
>>>> We could maybe add support code in kernel to do s/-/_/g for the "new" clocks
>>>> so that their parent-child relationships would be retained, and then convert
>>>> the clocks in phases.
>>>
>>> Well some of them can be fixed by configuring things based
>>> on the compatible value and then the node name can be just
>>> clock like it should be.
>>>
>>> Here too one option would be to add custom compatibles like:
>>>
>>> compatible = "ti,clkout1-osc-div", "ti,divider-clock";
>>>
>>> And then have match data configure the rest.
>>>
>>> The other option would be to have lookup tables in the clock
>>> driver based on the SoC and reg address.
>>>
>>> This is a hidden mine though.. We've hit it already several times,
>>> and any dts clean-up effort has a chance of breaking things.
>>
>> Hmm maybe in this case just doing this is enough:
>>
>> clkout1_osc_div_ck: clock@4100 {
>> 	...
>> }
> 
> But then we would end up with 6 clock node with the same name "clock@4100",
> doesn't pose a problem somewhere?

Yeah, clk core would not know which one to use then.

> 
> Tero?
> 
> Benoit
> 
>>
>> Or do all the TI clocks we have have a dependency to the
>> node naming?

This is a feature of clock core. Clock parents need to have distinct 
names, otherwise it won't work.

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