Hi Sylwester, On Wed, Oct 12, 2011 at 9:49 PM, Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx> wrote: > On 10/12/2011 02:36 PM, Rajeshwari Birje wrote: >> On Wed, Oct 12, 2011 at 3:54 PM, Sylwester Nawrocki >> <s.nawrocki@xxxxxxxxxxx> 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@xxxxxxxxxxx> >>>> --- >>>> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html