[PATCH V4 3/3] ARM: SAMSUNG: Add lookup of sdhci-s3c clocks using generic names
Rajeshwari Birje
rajeshwari.birje at gmail.com
Thu Oct 13 03:16:19 EDT 2011
Hi Sylwester,
On Wed, Oct 12, 2011 at 9:49 PM, Sylwester Nawrocki
<s.nawrocki at samsung.com> wrote:
> On 10/12/2011 02:36 PM, Rajeshwari Birje wrote:
>> On Wed, Oct 12, 2011 at 3:54 PM, Sylwester Nawrocki
>> <s.nawrocki at samsung.com> wrote:
>>> Hi Rajeshwari,
>>>
>>> On 10/12/2011 11:43 AM, Rajeshwari Shinde wrote:
>>>> Add support for lookup of sdhci-s3c controller clocks using generic names
>>>> for s3c2416, s3c64xx, s5pc100, s5pv210 and exynos4 SoC's.
>>>>
>>>> Signed-off-by: Rajeshwari Shinde <rajeshwari.s at samsung.com>
>>>> ---
>>>> arch/arm/mach-exynos4/clock.c | 88 ++++++++++-------
>>>> arch/arm/mach-s3c2416/clock.c | 68 +++++++------
>>>> arch/arm/mach-s3c64xx/clock.c | 126 +++++++++++++++----------
>>>> arch/arm/mach-s5pc100/clock.c | 130 ++++++++++++++++----------
>>>> arch/arm/mach-s5pv210/clock.c | 167 ++++++++++++++++++++-------------
>>>> arch/arm/plat-s3c24xx/s3c2443-clock.c | 15 ++-
>>>> 6 files changed, 359 insertions(+), 235 deletions(-)
>>>>
>>>> diff --git a/arch/arm/mach-exynos4/clock.c b/arch/arm/mach-exynos4/clock.c
>>>> index 9f50e33..c6383b9 100644
>>>> --- a/arch/arm/mach-exynos4/clock.c
>>>> +++ b/arch/arm/mach-exynos4/clock.c
>>>> @@ -1157,42 +1157,6 @@ static struct clksrc_clk clksrcs[] = {
>>>> .reg_div = { .reg = S5P_CLKDIV_MFC, .shift = 0, .size = 4 },
> ...
>>>> +static struct clksrc_clk clk_sclk_mmc0 = {
>>>> + .clk = {
>>>> + .name = "sclk_mmc",
>>>> + .devname = "s3c-sdhci.0",
>>>
>>> Would it make sense to drop this 'devname' field here and others
>>> until sclk_mmc3 ....
>>
>> *** The devname here distinguishes these clocks. So it should be okay
>> to have a devname for these clocks.
>
> I'm not sure what's Mr Kukjin's opinion on that, but I personally would really
> like to see all the devname fields disappear from samsung clk data structures.
> Possibly if all involved people would keep that in mind we could achieve this
> over time.
**** Devname field is still required for clocks like hsmmc for driver
to work fine. Hence it would be tough to remove the devname
completely.
>
>>
>>>
>>>> + .parent = &clk_dout_mmc0.clk,
>>>> + .enable = exynos4_clksrc_mask_fsys_ctrl,
>>>> + .ctrlbit = (1 << 0),
>>>> + },
>>>> + .reg_div = { .reg = S5P_CLKDIV_FSYS1, .shift = 8, .size = 8 },
>>>> +};
>>>> +
>>>> +static struct clksrc_clk clk_sclk_mmc1 = {
>>>> + .clk = {
>>>> + .name = "sclk_mmc",
>>>> + .devname = "s3c-sdhci.1",
>>>
>>>> + .parent = &clk_dout_mmc1.clk,
>>>> + .enable = exynos4_clksrc_mask_fsys_ctrl,
>>>> + .ctrlbit = (1 << 4),
>>>> + },
>>>> + .reg_div = { .reg = S5P_CLKDIV_FSYS1, .shift = 24, .size = 8 },
>>>> +};
>>>> +
>>>> +static struct clksrc_clk clk_sclk_mmc2 = {
>>>> + .clk = {
>>>> + .name = "sclk_mmc",
>>>> + .devname = "s3c-sdhci.2",
>>>
>>>> + .parent = &clk_dout_mmc2.clk,
>>>> + .enable = exynos4_clksrc_mask_fsys_ctrl,
>>>> + .ctrlbit = (1 << 8),
>>>> + },
>>>> + .reg_div = { .reg = S5P_CLKDIV_FSYS2, .shift = 8, .size = 8 },
>>>> +};
>>>> +
>>>> +static struct clksrc_clk clk_sclk_mmc3 = {
>>>> + .clk = {
>>>> + .name = "sclk_mmc",
>>>> + .devname = "s3c-sdhci.3",
>>>
>>>> + .parent = &clk_dout_mmc3.clk,
>>>> + .enable = exynos4_clksrc_mask_fsys_ctrl,
>>>> + .ctrlbit = (1 << 12),
>>>> + },
>>>> + .reg_div = { .reg = S5P_CLKDIV_FSYS2, .shift = 24, .size = 8 },
>>>> +};
>>>> +
>>>> /* Clock initialization code */
>>>> static struct clksrc_clk *sysclks[] = {
>>>> &clk_mout_apll,
>>>> @@ -1289,6 +1297,10 @@ static struct clksrc_clk *clksrc_cdev[] = {
>>>> &clk_sclk_uart1,
>>>> &clk_sclk_uart2,
>>>> &clk_sclk_uart3,
>>>> + &clk_sclk_mmc0,
>>>> + &clk_sclk_mmc1,
>>>> + &clk_sclk_mmc2,
>>>> + &clk_sclk_mmc3,
>>>
>>> ..then drop the above 4 lines...
>>
>> **** The registration for these clocks are important. The
>> s3c_register_clksrc() function sets the .ops of this clock and also
>> its parent. So the registration cannot be dropped.
>
> OK.
>
>>
>>>
>>>> };
>>>>
>>>> static struct clk_lookup exynos4_clk_lookup[] = {
>>>> @@ -1296,6 +1308,10 @@ static struct clk_lookup exynos4_clk_lookup[] = {
>>>> CLKDEV_INIT("exynos4210-uart.1", "clk_uart_baud0", &clk_sclk_uart1.clk),
>>>> CLKDEV_INIT("exynos4210-uart.2", "clk_uart_baud0", &clk_sclk_uart2.clk),
>>>> CLKDEV_INIT("exynos4210-uart.3", "clk_uart_baud0", &clk_sclk_uart3.clk),
>>>> + CLKDEV_INIT("exynos4-sdhci.0", "mmc_busclk.2", &clk_sclk_mmc0.clk),
>>>> + CLKDEV_INIT("exynos4-sdhci.1", "mmc_busclk.2", &clk_sclk_mmc1.clk),
>>>> + CLKDEV_INIT("exynos4-sdhci.2", "mmc_busclk.2", &clk_sclk_mmc2.clk),
>>>> + CLKDEV_INIT("exynos4-sdhci.3", "mmc_busclk.2", &clk_sclk_mmc3.clk),
>>>
>>> ..and add something like:
>>>
>>> + CLKDEV_INIT("s3c-sdhci.0", "sclk_mmc", &clk_sclk_mmc0.clk),
>>> + CLKDEV_INIT("s3c-sdhci.1", "sclk_mmc", &clk_sclk_mmc1.clk),
>>> + CLKDEV_INIT("s3c-sdhci.2", "sclk_mmc", &clk_sclk_mmc2.clk),
>>> + CLKDEV_INIT("s3c-sdhci.3", "sclk_mmc", &clk_sclk_mmc3.clk),
>>>
>>> ?
>>
>> **** The driver uses a common name for the possible bus clock sources,
>> that is “mmc_busclk”. This keeps the clock lookup code in the driver
>> simple. Also, there could be SoC’s which do no use sclk_mmc as the bus
>> clock name as per the user manual
>
> OK, I was aware of that. I didn't mean removing the "mmc_busclk.2" entries,
> just adding another 4, which are created anyway by s3c_register_clksrc()
> function. But for now I think the patch is OK.
>
>>
>>
>>>
>>> Also I'm wondering why we're using different device names for clk_sclk_mmc0..3
>>> clocks, i.e. exynos4-sdhci.? and s3c-sdhci.? ?
>>>
>>> Does it all work on exynos ? I would expect the device name to be same
>>> across all the clock definitions, otherwise clk_get(dev, ..) will fail.
>>
>> **** There was a patch submitted to rename the device name of sdhci
>> for Exynos to exynos4-sdhci. I will remove this change from this patch
>> and let that patch handle this change.
>
> I wouldn't do that. IMHO it's better to keep this patch as is, to make the final
> diff size lower.
**** Will modify the exynos4-sdhci. to s3c-sdhci. and send the
updated patch for review.
> Perhaps it's even worth to consider doing the clock rename altogether in this patch.
> But it's of course up to you.
>
>
> Regards,
> --
> Sylwester Nawrocki
> Samsung Poland R&D Center
>
Regards,
Rajeshwari Shinde.
More information about the linux-arm-kernel
mailing list