[PATCH V4 3/3] ARM: SAMSUNG: Add lookup of sdhci-s3c clocks using generic names

[s.nawrocki@samsung.com (Sylwester Nawrocki)]

On 10/12/2011 02:36 PM, Rajeshwari Birje wrote:
> On Wed, Oct 12, 2011 at 3:54 PM, Sylwester Nawrocki
> <s.nawrocki@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@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.

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