Re: [PATCH v2 09/13] ARM: davinci - update the dm644x soc code to use common clk drivers

[Sekhar Nori <nsekhar@ti.com>]

Hi Murali,

On 10/11/2012 8:28 PM, Karicheri, Muralidharan wrote:
>>> -----Original Message-----
>>> From: Nori, Sekhar
>>> Sent: Thursday, October 11, 2012 8:25 AM
>>> To: Karicheri, Muralidharan
>>> Cc: mturquette@linaro.org; arnd@arndb.de; akpm@linux-foundation.org;
>>> shawn.guo@linaro.org; rob.herring@calxeda.com; linus.walleij@linaro.org;
>>> viresh.linux@gmail.com; linux-kernel@vger.kernel.org; Hilman, Kevin;
>>> linux@arm.linux.org.uk; davinci-linux-open-source@linux.davincidsp.com; linux-arm-
>>> kernel@lists.infradead.org; linux-keystone@list.ti.com - Linux developers for Keystone
>>> family of devices (May contain non-TIers); linux-c6x-dev@linux-c6x.org; Chemparathy,
>>> Cyril
>>> Subject: Re: [PATCH v2 09/13] ARM: davinci - update the dm644x soc code to use
>>> common clk drivers
>>>
>>> Murali,
>>>
>>> On 9/26/2012 11:40 PM, Murali Karicheri wrote:
>>>> The clock tree for dm644x is defined using the new structure davinci_clk.
>>>> The SoC specific code re-uses clk-fixed-rate, clk-divider and clk-mux
>>>> drivers in addition to the davinci specific clk drivers,
>>>> clk-davinci-pll and clk-davinci-psc. Macros are defined to define the
>>>> various clocks in the SoC.
>>>>
>>>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
>>>
>>> You have chosen to keep all clock related data in platform files while using the common
>>> clock framework to provide just the infrastructure. If you look at how mxs and spear
>>> have been migrated, they have migrated the soc specific clock data to drivers/clk as well.
>>> See "drivers/clk/spear/spear3xx_clock.c" or "drivers/clk/mxs/clk-imx23.c 
> 
> I have to disagree on this one. I had investigated these code already and came
> up with a way that we can re-use code across all of the davinci platforms as 
> well as other architectures that re-uses the clk hardware IPs.

Which code you are talking about here? Even if you introduce
clk-dm644x.c, clk-keystone.c etc in drivers/clk/davinci/ you can reuse
the code you introduce in patches 1-3. I cant see how that will be
prevented.

> spear3xx_clock.c has initialization code for each of the platforms
> and so is the case with imx23.c.

By each of the platforms, you mean they all cater to a family of
devices? This depends on how close together the family of devices are.
Otherwise, there would be a file per soc. DM644x also represents a
family for that matter.

> By using platform_data approach, we are able to define clks for each of the SoC and then use davinci_common_clk_init() to do initialize the clk drivers based on platform data.

You need to define and register the clocks present on each SoC either
which way. I don't see why just the platform_data approach allows this.
And looking closely, you have defined platform data, but don't actually
have a platform device, making things more confusing.

> Later once we migrate to device tree, davinci_common_clk_init() will go way and also the clk structures defined in the SoC file. I have prototyped this on one of the device that I am working on. davinci_common_clk_init() will be replaced with a of_davinci_clk_init() that will use device tree to get all of the platform data for the clk providers and do the initialization based on that. See highbank_clocks_init() in clk-highbank.c. I have used this model for device
> tree based clk initialization.

I don't think we should wait till DT migration to get rid of clock data
from platform code. For many of the older DaVinci platforms, DT
migration is a big if and when. This approach you gave above might work
for newer DT-only platforms, but even if there is one board that is not
migrated to DT, the entire clock data will have to stay. I have very
less hope this will happen for DaVinci (at least in the near term). So,
I would rather take the opportunity of common clock tree migration to
move clock data out of mach-davinci.

Also, just moving soc-specific clk data to drivers/clk/davinci/* does
not impede a future DT conversion, no?

> So it make sense to keep the design the way it is. Otherwise we will end up writing dm644x_clk_init(), dm355_clk_init(), etc for each of the platforms and these code will get thrown away once we migrate to
> device tree. 

I still don't see why davinci/keystone cannot follow the same approach
taken by multiple other socs - spear, mxs and ux500. I am unconvinced
that we have a significantly different case.

>>> ". I feel the
>>> latter way is better and I also think it will simplify some of the look-up infrastructure you
>>> had to build. This will also help some real code reduction from arch/arm/mach-davinci/.
>>>
> 
> The look-up infrastructure is pretty much re-use of the existing code base in SoC specific file.

Yes, but that's no reason to keep maintaining it.

> About code reduction, I can't say I agree, as we need to add platform_specific clock initialization code if we follow spear3xx_clock.c model and end up probably adding more code.
> SoC specific file (for example dm644x.c) has only data structures and all of SoC will re-use davinci_common_clk_init() to do the initialization. So I am not sure how you conclude we will have code reduction?

Is about code reduction from arch/arm/. That's what ARM community is
working towards.

Thanks,
Sekhar

PS: When replying, can you please hit an enter after every 70 or so
characters. Otherwise quoting from your mails is becoming very
difficult. I tried manually adjusting it but finally gave up.