Re: [PATCH v2 01/13] clk: davinci - add Main PLL clock driver

Re: [PATCH v2 01/13] clk: davinci - add Main PLL clock driver

[Sekhar Nori]

On 9/26/2012 11:40 PM, Murali Karicheri wrote:

> +struct clk_davinci_pll_data {
> +	/* physical addresses set by platform code */
> +	u32 phy_pllm;
> +	/* if PLL has a prediv register this should be non zero */
> +	u32 phy_prediv;
> +	/* if PLL has a postdiv register this should be non zero */
> +	u32 phy_postdiv;

"phys" is typically used for physical addresses. I couldn't immediately
related this to physical address when I looked at this variable outside
of this file.

Also, physical addresses should be of type phys_addr_t.

> +	/* mapped addresses. should be initialized by  */
> +	void __iomem *pllm;
> +	void __iomem *prediv;
> +	void __iomem *postdiv;
> +	u32 pllm_mask;
> +	u32 prediv_mask;
> +	u32 postdiv_mask;
> +	u32 num;

Wondering what num is for? May be it will be clear once you document the
complete structure as Linus suggested.

> +	/* framework flags */
> +	u32 flags;
> +	/* pll flags */
> +	u32 pll_flags;
> +       /* use this value for prediv */
> +	u32 fixed_prediv;

Why is this called "fixed". Isn't this the pre-divider value that user
can configure? Also, shouldn't there be a post-divider value that you
need as well.

> +	/* multiply PLLM by this factor. By default most SOC set this to zero
> +	 * that translates to a multiplier of 1 and incrementer of 1.
> +	 * To override default, set this factor
> +	 */
> +	u32 pllm_multiplier;

'm' in pllm stands for multiplier. So, instead of calling it
"pllm_multiplier", call it "pllm_val" instead?

Sorry about late comments on this, looks like I missed looking at this
structure altogether yesterday and had to go back to this only when I
tried understanding whats going on in 4/13

Thanks,
Sekhar

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

[Sekhar Nori]

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
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/.

Thanks,
Sekhar

Re: [PATCH v2 04/13] clk: davinci - common clk driver initialization

[Sekhar Nori]

Hi Murali,

I have given this patch a partial review. I suspect this patch will
change much based on the comment I gave for 9/13.

On 9/26/2012 11:40 PM, Murali Karicheri wrote:
> This is the common clk driver initialization function for DaVinci
> SoCs and other SoCs that uses similar hardware architecture.
> Different clocks present in the SoC are passed to this function
> using a clk table and this function initialize the various drivers.
> Existing driver such as clk-fixed-rate, clk-divider, clk-mux and
> DaVinci specific clk drivers are initialized by this function.
> Also adds clock lookup for shared clocks used by various drivers.
> davinci-clock.h declares the various structures used for defining
> the DaVinci clocks.
> 
> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
> 
> diff --git a/drivers/clk/davinci/davinci-clock.c b/drivers/clk/davinci/davinci-clock.c
> new file mode 100644
> index 0000000..cbd5201
> --- /dev/null
> +++ b/drivers/clk/davinci/davinci-clock.c
> @@ -0,0 +1,232 @@
> +/*
> + * Clock initialization code for DaVinci devices
> + *
> + * Copyright (C) 2006-2012 Texas Instruments.
> + * Copyright (C) 2008-2009 Deep Root Systems, LLC
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include <linux/init.h>
> +#include <linux/clk-provider.h>
> +#include <linux/clkdev.h>
> +#include <linux/slab.h>
> +#include <linux/io.h>
> +#include <linux/platform_data/clk-davinci-pll.h>
> +#include <linux/platform_data/clk-keystone-pll.h>
> +#include <linux/platform_data/clk-davinci-psc.h>
> +#include <linux/platform_data/davinci-clock.h>

Needs to be kept sorted.

> +
> +static DEFINE_SPINLOCK(_lock);

No need of the underscore prefix.

> +
> +struct clk *davinci_lookup_clk(struct davinci_clk_lookup *clocks,
> +				const char *con_id)
> +{
> +	struct davinci_clk_lookup *c;
> +
> +	for (c = clocks; c->_clk; c++) {
> +		if (c->con_id && !strcmp(c->con_id, con_id))
> +			return c->clk;
> +	}
> +	return NULL;
> +}

This is a global function, but it is not exported in a header file.
Should this be static instead?

> +
> +#ifdef	CONFIG_CLK_DAVINCI_PLL
> +static void register_davinci_pll_clk(struct davinci_clk_lookup *c,
> +			struct clk_davinci_pll_data *pll_data)
> +{
> +
> +	WARN_ON(!pll_data->phy_pllm);

I don't think it is right to check for zero physical address. It is not
true on any of the DaVinci platforms, but it is possible to design
hardware where physical address of the PLL multipler is zero.

> +	pll_data->pllm = ioremap(pll_data->phy_pllm, 4);
> +	WARN_ON(!pll_data->pllm);

Along with the warning for users, you also need to return an -ENOMEM so
callers are aware? Or you can skip the WARN_ON and simply return -ENOMEM.

> +	if (pll_data->phy_prediv) {
> +		pll_data->prediv = ioremap(pll_data->phy_prediv, 4);
> +		WARN_ON(!pll_data->prediv);
> +	}
> +	if (pll_data->phy_postdiv) {
> +		pll_data->postdiv = ioremap(pll_data->phy_postdiv, 4);
> +		WARN_ON(!pll_data->postdiv);
> +	}

Comments above apply here as well.

> +	c->clk = clk_register_davinci_pll(NULL,
> +			c->_clk->name, c->_clk->parent->name,
> +			pll_data);
> +}
> +#else
> +static void register_davinci_pll_clk(struct davinci_clk_lookup *c,
> +			struct clk_davinci_pll_data *pll_data)
> +{
> +	return;
> +}
> +#endif
> +
> +#ifdef	CONFIG_CLK_KEYSTONE_PLL
> +static void register_keystone_pll_clk(struct davinci_clk_lookup *c,
> +			struct clk_keystone_pll_data *pll_data)
> +{
> +	WARN_ON(!pll_data->phy_pllm);
> +	pll_data->pllm = ioremap(pll_data->phy_pllm, 4);
> +	WARN_ON(!pll_data->pllm);
> +	WARN_ON(!pll_data->phy_main_pll_ctl0);
> +	pll_data->main_pll_ctl0 =
> +		ioremap(pll_data->phy_main_pll_ctl0, 4);
> +	WARN_ON(!pll_data->main_pll_ctl0);
> +	c->clk = clk_register_keystone_pll(NULL,
> +			c->_clk->name, c->_clk->parent->name,
> +			 pll_data);
> +}
> +#else
> +static void register_keystone_pll_clk(struct davinci_clk_lookup *c,
> +			struct clk_keystone_pll_data *pll_data)
> +{
> +	return;
> +}
> +#endif
> +
> +int __init davinci_common_clk_init(struct davinci_clk_lookup *clocks,
> +				struct davinci_dev_lookup *dev_clk_lookups,
> +				u8 num_gpscs, u32 *psc_bases)

psc_bases should be of type phys_add_t.

> +{
> +	void __iomem **base = NULL, *reg;
> +	struct davinci_clk_lookup *c;
> +	struct davinci_clk *_clk;
> +	unsigned long rate;
> +	int i, skip;
> +
> +	WARN_ON(!num_gpscs);

Need to return error if this is hit. In general revisit the need to
return an error wherever you have WARN_ON().

Thanks,
Sekhar

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

[Karicheri, Muralidharan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3635 bytes --]

>> -----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. spear3xx_clock.c has initialization code for each of the platforms and so is the case with imx23.c. 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. 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.

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 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. 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?

- Murali

>> Thanks,
>> Sekhar
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[Sekhar Nori]

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.

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

[Karicheri, Muralidharan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 5821 bytes --]

--Cut----

>> Subject: Re: [PATCH v2 09/13] ARM: davinci - update the dm644x soc code to use
>> common clk drivers
>> 
>> >>> 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.

I was talking about re-use of davinci_common_clk_init in drivers/clk/davinci/davinci-clock.c.
This is meant to be re-used across all of the DaVinci devices.

>> 
>> > 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.
>> 

Ok. There are multiple ways to implement this software. We had discussed this
internally and picked the platform_data approach. The clk drivers are written not
following the platform driver model. But I don't see why we can't use platform data
to configure this drivers. Down below, you have made two interesting points, one is
ARM code reduction. This patch already does this by moving the API that initializes
the clk drivers (davinci_common_clk_init()) out of ARM to drivers/clk/davinci. So
this + removal of existing clk driver under arm/mach-davinci/clock.[ch], we have
achieved this goal. The second point is the moving of SoC specific clk data out of SoC
code to drive. Are you 100% sure this is the right thing to do for these drivers. If so,
I can start working on this change right away. As I am working on this as a background
activity, I want to be double or triple sure before doing the rework of these patches :).
So please confirm.

>> > 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.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[Sekhar Nori]

Hi Murali,

On 10/15/2012 9:21 PM, Karicheri, Muralidharan wrote:
> --Cut----
> 
>>> Subject: Re: [PATCH v2 09/13] ARM: davinci - update the dm644x soc code to use
>>> common clk drivers
>>>
>>>>>> 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.
> 
> I was talking about re-use of davinci_common_clk_init in drivers/clk/davinci/davinci-clock.c.
> This is meant to be re-used across all of the DaVinci devices.
> 
>>>
>>>> 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.
>>>
> 
> Ok. There are multiple ways to implement this software. We had discussed this
> internally and picked the platform_data approach. The clk drivers are written not
> following the platform driver model. But I don't see why we can't use platform data
> to configure this drivers. Down below, you have made two interesting points, one is
> ARM code reduction. This patch already does this by moving the API that initializes
> the clk drivers (davinci_common_clk_init()) out of ARM to drivers/clk/davinci. So
> this + removal of existing clk driver under arm/mach-davinci/clock.[ch], we have
> achieved this goal. The second point is the moving of SoC specific clk data out of SoC
> code to drive. Are you 100% sure this is the right thing to do for these drivers. If so,
> I can start working on this change right away. As I am working on this as a background
> activity, I want to be double or triple sure before doing the rework of these patches :).
> So please confirm.

Yes, this is the right way to go. And I don't see it as something
breaking new ground since there are already multiple SoCs in mainline
which are following this same approach. May be to start with just
convert one SoC and send for review.

Thanks for taking this up and helping clean-up mach-davinci.

Regards,
Sekhar