* Re: Add clkdev support to HSMMC [not found] <0LQO00HOLZVZDW20@ms6.samsung.com> @ 2011-08-31 8:42 ` Sylwester Nawrocki 2011-08-31 10:13 ` Thomas Abraham 0 siblings, 1 reply; 4+ messages in thread From: Sylwester Nawrocki @ 2011-08-31 8:42 UTC (permalink / raw) To: banajit.g Cc: RAJESHWARI S SHINDE, PADMAVATHI VENNA, Kukjin Kim, linux-samsung-soc Hi Banajit, (added Mr. Kim at cc) On 08/29/2011 03:52 PM, BANAJIT GOSWAMI wrote: > Hi Sylwester, > > Thanks once again for your suggestions and help. > > I have tried to put down some points (attached doc) regarding the need of > registering some clocks separately (with original names). > > > Kindly let me know your opinion regarding the same. Here is your original text as in the attachment: > Clkdev Changes Analysis > Following are few of the points due to which we are reluctant on changing the > current implementation of Clkdev in Samsung SoC’s: > > 1)Adding more lines of code: Currently Samsung uses a static clock array to > register the clocks. Breaking them up into individual clock structure object > and again registering them by creating a lookup table will add more lines of > code which might not be accepted. IMHO this is insignificant increase in LOC which will result in less code in the future. > 2) Confusion due to clock names: If we register the clock with generic > connection ids which are common across the platforms and then try to access > same for some generic functions like set_parent () or clk_get_rate (), which > would be confusing for the developer modifying the code. It's just a matter of chossing meaningfull names to avoid confusion. But all this require a look from wider perspective, not only problem of a single device driver. I'm not the Samsung clock code maintainer. I just expressed my concerns and want to let Mr. Kim judge and take care of further development. Unfortunately I can invest much time to this issue ATM. > Same is explained below with an example: > In SD/MMC driver we use a bus clock with name “sclk_mmc” which would be > registered with generic connection id “mmc_busclk.2” to clkdev and same is > used by driver for best source calculation. > In case we need to set parent for the same at kernel side we would have to do > the same using set_parent (“mmc_busclk.2”, parent) which would create > confusion to the person reading where as set_parent (“sclk_mmc”, parent) > would be clearer. > 3) In the proposed implementation where we create connection ids only for the > bus clocks we are creating an extra node for the same in the clkdev table, > where as the clock is getting registered only once with the kernel. I don't agree with this last statement, the same struct clk is added to the list of clocks known by the system twice, in s3c24xx_register_clocks() and clkdev_add_table(). This is only what I have been pointing out. Please correct me, if I missed anything. Here is an original patch: 8----------------------------------------------------------------------------- From 04e1949999ad4f3639e7266b4a8750b13a6d9e5e Mon Sep 17 00:00:00 2001 From: Rajeshwari Shinde <rajeshwari.s@samsung.com> Date: Fri, 19 Aug 2011 12:14:10 +0530 Subject: [PATCH 2] ARM: S5PV210: Add Clkdev support for SDHCI This patch registers the SDHCI bus clocks to Clkdev. Signed-off-by: Rajeshwari Shinde <rajeshwari.s@samsung.com> --- arch/arm/mach-s5pv210/clock.c | 178 ++++++++++++++++++---------- arch/arm/plat-samsung/include/plat/clock.h | 8 ++ 2 files changed, 122 insertions(+), 64 deletions(-) diff --git a/arch/arm/mach-s5pv210/clock.c b/arch/arm/mach-s5pv210/clock.c index 52a8e60..a95a14b 100644 --- a/arch/arm/mach-s5pv210/clock.c +++ b/arch/arm/mach-s5pv210/clock.c @@ -350,30 +350,6 @@ static struct clk init_clocks_off[] = { .enable = s5pv210_clk_ip1_ctrl, .ctrlbit = (1<<25), }, { - .name = "hsmmc", - .devname = "s3c-sdhci.0", - .parent = &clk_hclk_psys.clk, - .enable = s5pv210_clk_ip2_ctrl, - .ctrlbit = (1<<16), - }, { - .name = "hsmmc", - .devname = "s3c-sdhci.1", - .parent = &clk_hclk_psys.clk, - .enable = s5pv210_clk_ip2_ctrl, - .ctrlbit = (1<<17), - }, { - .name = "hsmmc", - .devname = "s3c-sdhci.2", - .parent = &clk_hclk_psys.clk, - .enable = s5pv210_clk_ip2_ctrl, - .ctrlbit = (1<<18), - }, { - .name = "hsmmc", - .devname = "s3c-sdhci.3", - .parent = &clk_hclk_psys.clk, - .enable = s5pv210_clk_ip2_ctrl, - .ctrlbit = (1<<19), - }, { .name = "systimer", .parent = &clk_pclk_psys.clk, .enable = s5pv210_clk_ip3_ctrl, @@ -844,46 +820,6 @@ static struct clksrc_clk clksrcs[] = { .reg_div = { .reg = S5P_CLK_DIV1, .shift = 20, .size = 4 }, }, { .clk = { - .name = "sclk_mmc", - .devname = "s3c-sdhci.0", - .enable = s5pv210_clk_mask0_ctrl, - .ctrlbit = (1 << 8), - }, - .sources = &clkset_group2, - .reg_src = { .reg = S5P_CLK_SRC4, .shift = 0, .size = 4 }, - .reg_div = { .reg = S5P_CLK_DIV4, .shift = 0, .size = 4 }, - }, { - .clk = { - .name = "sclk_mmc", - .devname = "s3c-sdhci.1", - .enable = s5pv210_clk_mask0_ctrl, - .ctrlbit = (1 << 9), - }, - .sources = &clkset_group2, - .reg_src = { .reg = S5P_CLK_SRC4, .shift = 4, .size = 4 }, - .reg_div = { .reg = S5P_CLK_DIV4, .shift = 4, .size = 4 }, - }, { - .clk = { - .name = "sclk_mmc", - .devname = "s3c-sdhci.2", - .enable = s5pv210_clk_mask0_ctrl, - .ctrlbit = (1 << 10), - }, - .sources = &clkset_group2, - .reg_src = { .reg = S5P_CLK_SRC4, .shift = 8, .size = 4 }, - .reg_div = { .reg = S5P_CLK_DIV4, .shift = 8, .size = 4 }, - }, { - .clk = { - .name = "sclk_mmc", - .devname = "s3c-sdhci.3", - .enable = s5pv210_clk_mask0_ctrl, - .ctrlbit = (1 << 11), - }, - .sources = &clkset_group2, - .reg_src = { .reg = S5P_CLK_SRC4, .shift = 12, .size = 4 }, - .reg_div = { .reg = S5P_CLK_DIV4, .shift = 12, .size = 4 }, - }, { - .clk = { .name = "sclk_mfc", .devname = "s5p-mfc", .enable = s5pv210_clk_ip0_ctrl, @@ -1011,6 +947,112 @@ static u32 epll_div[][6] = { { 50000000, 0, 50, 3, 3, 0 }, }; +static struct clk hsmmc_clk0 = { + .name = "hsmmc", + .devname = "s3c-sdhci.0", + .parent = &clk_hclk_psys.clk, + .enable = s5pv210_clk_ip2_ctrl, + .ctrlbit = (1<<16), +}; + +static struct clk hsmmc_clk1 = { + .name = "hsmmc", + .devname = "s3c-sdhci.1", + .parent = &clk_hclk_psys.clk, + .enable = s5pv210_clk_ip2_ctrl, + .ctrlbit = (1<<17), +}; + +static struct clk hsmmc_clk2 = { + .name = "hsmmc", + .devname = "s3c-sdhci.2", + .parent = &clk_hclk_psys.clk, + .enable = s5pv210_clk_ip2_ctrl, + .ctrlbit = (1<<18), +}; + +static struct clk hsmmc_clk3 = { + .name = "hsmmc", + .devname = "s3c-sdhci.3", + .parent = &clk_hclk_psys.clk, + .enable = s5pv210_clk_ip2_ctrl, + .ctrlbit = (1<<19), +}; + +static struct clksrc_clk sclk_mmc0 = { + .clk = { + .name = "sclk_mmc", + .devname = "s3c-sdhci.0", + .enable = s5pv210_clk_mask0_ctrl, + .ctrlbit = (1 << 8), + }, + .sources = &clkset_group2, + .reg_src = { .reg = S5P_CLK_SRC4, .shift = 0, .size = 4 }, + .reg_div = { .reg = S5P_CLK_DIV4, .shift = 0, .size = 4 }, + +}; + +static struct clksrc_clk sclk_mmc1 = { + .clk = { + .name = "sclk_mmc", + .devname = "s3c-sdhci.1", + .enable = s5pv210_clk_mask0_ctrl, + .ctrlbit = (1 << 9), + }, + .sources = &clkset_group2, + .reg_src = { .reg = S5P_CLK_SRC4, .shift = 4, .size = 4 }, + .reg_div = { .reg = S5P_CLK_DIV4, .shift = 4, .size = 4 }, +}; + +static struct clksrc_clk sclk_mmc2 = { + .clk = { + .name = "sclk_mmc", + .devname = "s3c-sdhci.2", + .enable = s5pv210_clk_mask0_ctrl, + .ctrlbit = (1 << 10), + }, + .sources = &clkset_group2, + .reg_src = { .reg = S5P_CLK_SRC4, .shift = 8, .size = 4 }, + .reg_div = { .reg = S5P_CLK_DIV4, .shift = 8, .size = 4 }, +}; + +static struct clksrc_clk sclk_mmc3 = { + .clk = { + .name = "sclk_mmc", + .devname = "s3c-sdhci.3", + .enable = s5pv210_clk_mask0_ctrl, + .ctrlbit = (1 << 11), + }, + .sources = &clkset_group2, + .reg_src = { .reg = S5P_CLK_SRC4, .shift = 12, .size = 4 }, + .reg_div = { .reg = S5P_CLK_DIV4, .shift = 12, .size = 4 }, +}; + +static struct clk_lookup s5pv210_clks[] = { + CLK("s3c-sdhci.0", "mmc_busclk.0", &hsmmc_clk0), + CLK("s3c-sdhci.1", "mmc_busclk.0", &hsmmc_clk1), + CLK("s3c-sdhci.2", "mmc_busclk.0", &hsmmc_clk2), + CLK("s3c-sdhci.3", "mmc_busclk.0", &hsmmc_clk3), + CLK("s3c-sdhci.0", "mmc_busclk.2", &sclk_mmc0.clk), + CLK("s3c-sdhci.1", "mmc_busclk.2", &sclk_mmc1.clk), + CLK("s3c-sdhci.2", "mmc_busclk.2", &sclk_mmc2.clk), + CLK("s3c-sdhci.3", "mmc_busclk.2", &sclk_mmc3.clk), +}; + +static struct clk *bus_clk[] = { + &hsmmc_clk0, + &hsmmc_clk1, + &hsmmc_clk2, + &hsmmc_clk3, +}; + +static struct clksrc_clk *bus_clksrc[] = { + &sclk_mmc0, + &sclk_mmc1, + &sclk_mmc2, + &sclk_mmc3, +}; + static int s5pv210_epll_set_rate(struct clk *clk, unsigned long rate) { unsigned int epll_con, epll_con_k; @@ -1156,10 +1198,18 @@ void __init s5pv210_register_clocks(void) s3c_register_clksrc(sysclks[ptr], 1); s3c_register_clksrc(clksrcs, ARRAY_SIZE(clksrcs)); + for (ptr = 0; ptr < ARRAY_SIZE(bus_clksrc); ptr++) + s3c_register_clksrc(bus_clksrc[ptr], 1); + s3c_register_clocks(init_clocks, ARRAY_SIZE(init_clocks)); s3c_register_clocks(init_clocks_off, ARRAY_SIZE(init_clocks_off)); s3c_disable_clocks(init_clocks_off, ARRAY_SIZE(init_clocks_off)); + s3c24xx_register_clocks(bus_clk, ARRAY_SIZE(bus_clk)); + for (ptr = 0; ptr < ARRAY_SIZE(bus_clk); ptr++) + s3c_disable_clocks(bus_clk[ptr], 1); + s3c_pwmclk_init(); + clkdev_add_table(s5pv210_clks, ARRAY_SIZE(s5pv210_clks)); } diff --git a/arch/arm/plat-samsung/include/plat/clock.h b/arch/arm/plat-samsung/include/plat/clock.h index 87d5b38..03f70ba 100644 --- a/arch/arm/plat-samsung/include/plat/clock.h +++ b/arch/arm/plat-samsung/include/plat/clock.h @@ -29,6 +29,14 @@ struct clk; * not be a problem as it is unlikely these operations are going * to need to be called quickly. */ + +#define CLK(dev, con, ck) \ + { \ + .dev_id = dev, \ + .con_id = con, \ + .clk = ck, \ + } \ + struct clk_ops { int (*set_rate)(struct clk *c, unsigned long rate); unsigned long (*get_rate)(struct clk *c); -- 1.7.4.4 8---------------------------------------------------------------------------- > > Thanks and Regards, > Banajit Goswami ... > > ------- *Original Message* ------- > *Sender* : Sylwester Nawrocki<s.nawrocki@samsung.com> Software Engineer/Poland > R&D Center-Linux (MSD)/Samsung Electronics > *Date* : Aug 24, 2011 22:29 (GMT+09:00) > *Title* : Re: Add clkdev support to HSMMC > > Hi Banajit, > > On 08/24/2011 07:47 AM, RAJESHWARI S SHINDE wrote: >> Hi Sylwester, >> >> >> This is with regards to your reply for Padmavathi's mail regarding double >> registration of clocks with clkdev in case of SPI. >> >> In the attached patch set registration of bus clocks is happening twice, once >> with actual clock name and another with generic connection id. This is being >> done to avoid issue as explained below. >> >> When trying to add Clkdev support for the HSMMC, was facing the following issue: >> >> We have a peripheral clock with name "hsmmc " which is also being used as a >> bus clock, for best source calculation in SoC's like S3C64XX, S3C2416, >> S5PC100 and S5PV210 as shown below: >> >> char *s5pv210_hsmmc_clksrcs[4] = { >> [0] = "hsmmc", /* HCLK */ >> /* [1] = "hsmmc", - duplicate HCLK entry */ >> [2] = "sclk_mmc", /* mmc_bus */ >> /* [3] = NULL, - reserved */ >> }; >> >> Hence when we try to add this "hsmmc" as a bus clock in the clkdev lookup table >> with a generic name across all the platforms, which will be used by the >> HSMMC driver, the problem arises as the driver also tries to search for this >> peripheral clock using the clock name "hsmmc" as shown below and fails: >> >> sc->clk_io = clk_get(dev, "hsmmc"); >> if (IS_ERR(sc->clk_io)) { >> dev_err(dev, "failed to get io clock\n"); >> ret = PTR_ERR(sc->clk_io); >> goto err_io_clk; >> } >> >> I cannot alone use a generic connection id for getting the peripheral clocks as >> SoC's like S5P64X0 and Exynos4 do not use this peripheral clock as bus clock. > > Could you create two separate clock 'connection id' name classes for each SoC ? > One for the 'bus clock' and one for the the 'other' (hsmmc) clock ? > Then each mach-* could assign proper struct clk to for the "clk_io" at the driver. > > Now you're registering both current "hsmmc" and "sclk_hsmmc" clocks under > "mmc_busclk.?" name only. > > For instance: > > > +static struct clk_lookup s5pv210_clks[] = { > + CLK("s3c-sdhci.0", "mmc_busclk.0", &hsmmc_clk0), > + CLK("s3c-sdhci.1", "mmc_busclk.0", &hsmmc_clk1), > + CLK("s3c-sdhci.2", "mmc_busclk.0", &hsmmc_clk2), > + CLK("s3c-sdhci.3", "mmc_busclk.0", &hsmmc_clk3), > + CLK("s3c-sdhci.0", "mmc_busclk.2", &sclk_mmc0.clk), > + CLK("s3c-sdhci.1", "mmc_busclk.2", &sclk_mmc1.clk), > + CLK("s3c-sdhci.2", "mmc_busclk.2", &sclk_mmc2.clk), > + CLK("s3c-sdhci.3", "mmc_busclk.2", &sclk_mmc3.clk), > > + CLK("s3c-sdhci.0", "mmc_ioclk", &hsmmc_clk0), > + CLK("s3c-sdhci.1", "mmc_ioclk", &hsmmc_clk1), > + CLK("s3c-sdhci.2", "mmc_ioclk", &hsmmc_clk2), > + CLK("s3c-sdhci.3", "mmc_ioclk", &hsmmc_clk3), > > +}; > > Then in the driver: > > > - sc->clk_io = clk_get(dev, "hsmmc"); > + sc->clk_io = clk_get(dev, "mmc_ioclk"); > if (IS_ERR(sc->clk_io)) { > dev_err(dev, "failed to get io clock\n"); > ret = PTR_ERR(sc->clk_io); > goto err_io_clk; > } > > Does it make sense? > > I guess you don't want to modify every mach-s* for this ? > > I'm mostly OK with your patches, as they are a step in good direction. > >> >> Hope you understood the problem; do refer the attached patch set for more >> clarification. >> >> Do suggest me if any better solution. >> > Regards, -- Sylwester Nawrocki Samsung Poland R&D Center ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: Add clkdev support to HSMMC 2011-08-31 8:42 ` Add clkdev support to HSMMC Sylwester Nawrocki @ 2011-08-31 10:13 ` Thomas Abraham 2011-08-31 12:56 ` Sylwester Nawrocki 0 siblings, 1 reply; 4+ messages in thread From: Thomas Abraham @ 2011-08-31 10:13 UTC (permalink / raw) To: Sylwester Nawrocki Cc: banajit.g, RAJESHWARI S SHINDE, PADMAVATHI VENNA, Kukjin Kim, linux-samsung-soc Hi Sylwester, On 31 August 2011 14:12, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote: > Hi Banajit, > > (added Mr. Kim at cc) > > > On 08/29/2011 03:52 PM, BANAJIT GOSWAMI wrote: >> Hi Sylwester, >> >> Thanks once again for your suggestions and help. >> >> I have tried to put down some points (attached doc) regarding the need of >> registering some clocks separately (with original names). >> >> >> Kindly let me know your opinion regarding the same. > > Here is your original text as in the attachment: [...] >> 3) In the proposed implementation where we create connection ids only for the >> bus clocks we are creating an extra node for the same in the clkdev table, > >> where as the clock is getting registered only once with the kernel. > > I don't agree with this last statement, the same struct clk is added to > the list of clocks known by the system twice, in s3c24xx_register_clocks() > and clkdev_add_table(). This is only what I have been pointing out. > Please correct me, if I missed anything. s3c24xx_register_clock and clkdev_add_table add one instance of "struct clk_lookup" each (with different connections ids) but both of these "struct clk_lookup" instances point to the same "struct clk" instance. So, is two instances of "struct clk_lookup" pointing to the same "struct clk" instance the concern in this case? Thanks, Thomas. > > > Here is an original patch: > > 8----------------------------------------------------------------------------- > > From 04e1949999ad4f3639e7266b4a8750b13a6d9e5e Mon Sep 17 00:00:00 2001 > From: Rajeshwari Shinde <rajeshwari.s@samsung.com> > Date: Fri, 19 Aug 2011 12:14:10 +0530 > Subject: [PATCH 2] ARM: S5PV210: Add Clkdev support for SDHCI > > This patch registers the SDHCI bus clocks to Clkdev. > > Signed-off-by: Rajeshwari Shinde <rajeshwari.s@samsung.com> > --- > arch/arm/mach-s5pv210/clock.c | 178 ++++++++++++++++++---------- > arch/arm/plat-samsung/include/plat/clock.h | 8 ++ > 2 files changed, 122 insertions(+), 64 deletions(-) > > diff --git a/arch/arm/mach-s5pv210/clock.c b/arch/arm/mach-s5pv210/clock.c > index 52a8e60..a95a14b 100644 > --- a/arch/arm/mach-s5pv210/clock.c > +++ b/arch/arm/mach-s5pv210/clock.c > @@ -350,30 +350,6 @@ static struct clk init_clocks_off[] = { > .enable = s5pv210_clk_ip1_ctrl, > .ctrlbit = (1<<25), > }, { > - .name = "hsmmc", > - .devname = "s3c-sdhci.0", > - .parent = &clk_hclk_psys.clk, > - .enable = s5pv210_clk_ip2_ctrl, > - .ctrlbit = (1<<16), > - }, { > - .name = "hsmmc", > - .devname = "s3c-sdhci.1", > - .parent = &clk_hclk_psys.clk, > - .enable = s5pv210_clk_ip2_ctrl, > - .ctrlbit = (1<<17), > - }, { > - .name = "hsmmc", > - .devname = "s3c-sdhci.2", > - .parent = &clk_hclk_psys.clk, > - .enable = s5pv210_clk_ip2_ctrl, > - .ctrlbit = (1<<18), > - }, { > - .name = "hsmmc", > - .devname = "s3c-sdhci.3", > - .parent = &clk_hclk_psys.clk, > - .enable = s5pv210_clk_ip2_ctrl, > - .ctrlbit = (1<<19), > - }, { > .name = "systimer", > .parent = &clk_pclk_psys.clk, > .enable = s5pv210_clk_ip3_ctrl, > @@ -844,46 +820,6 @@ static struct clksrc_clk clksrcs[] = { > .reg_div = { .reg = S5P_CLK_DIV1, .shift = 20, .size = 4 }, > }, { > .clk = { > - .name = "sclk_mmc", > - .devname = "s3c-sdhci.0", > - .enable = s5pv210_clk_mask0_ctrl, > - .ctrlbit = (1 << 8), > - }, > - .sources = &clkset_group2, > - .reg_src = { .reg = S5P_CLK_SRC4, .shift = 0, .size = 4 }, > - .reg_div = { .reg = S5P_CLK_DIV4, .shift = 0, .size = 4 }, > - }, { > - .clk = { > - .name = "sclk_mmc", > - .devname = "s3c-sdhci.1", > - .enable = s5pv210_clk_mask0_ctrl, > - .ctrlbit = (1 << 9), > - }, > - .sources = &clkset_group2, > - .reg_src = { .reg = S5P_CLK_SRC4, .shift = 4, .size = 4 }, > - .reg_div = { .reg = S5P_CLK_DIV4, .shift = 4, .size = 4 }, > - }, { > - .clk = { > - .name = "sclk_mmc", > - .devname = "s3c-sdhci.2", > - .enable = s5pv210_clk_mask0_ctrl, > - .ctrlbit = (1 << 10), > - }, > - .sources = &clkset_group2, > - .reg_src = { .reg = S5P_CLK_SRC4, .shift = 8, .size = 4 }, > - .reg_div = { .reg = S5P_CLK_DIV4, .shift = 8, .size = 4 }, > - }, { > - .clk = { > - .name = "sclk_mmc", > - .devname = "s3c-sdhci.3", > - .enable = s5pv210_clk_mask0_ctrl, > - .ctrlbit = (1 << 11), > - }, > - .sources = &clkset_group2, > - .reg_src = { .reg = S5P_CLK_SRC4, .shift = 12, .size = 4 }, > - .reg_div = { .reg = S5P_CLK_DIV4, .shift = 12, .size = 4 }, > - }, { > - .clk = { > .name = "sclk_mfc", > .devname = "s5p-mfc", > .enable = s5pv210_clk_ip0_ctrl, > @@ -1011,6 +947,112 @@ static u32 epll_div[][6] = { > { 50000000, 0, 50, 3, 3, 0 }, > }; > > +static struct clk hsmmc_clk0 = { > + .name = "hsmmc", > + .devname = "s3c-sdhci.0", > + .parent = &clk_hclk_psys.clk, > + .enable = s5pv210_clk_ip2_ctrl, > + .ctrlbit = (1<<16), > +}; > + > +static struct clk hsmmc_clk1 = { > + .name = "hsmmc", > + .devname = "s3c-sdhci.1", > + .parent = &clk_hclk_psys.clk, > + .enable = s5pv210_clk_ip2_ctrl, > + .ctrlbit = (1<<17), > +}; > + > +static struct clk hsmmc_clk2 = { > + .name = "hsmmc", > + .devname = "s3c-sdhci.2", > + .parent = &clk_hclk_psys.clk, > + .enable = s5pv210_clk_ip2_ctrl, > + .ctrlbit = (1<<18), > +}; > + > +static struct clk hsmmc_clk3 = { > + .name = "hsmmc", > + .devname = "s3c-sdhci.3", > + .parent = &clk_hclk_psys.clk, > + .enable = s5pv210_clk_ip2_ctrl, > + .ctrlbit = (1<<19), > +}; > + > +static struct clksrc_clk sclk_mmc0 = { > + .clk = { > + .name = "sclk_mmc", > + .devname = "s3c-sdhci.0", > + .enable = s5pv210_clk_mask0_ctrl, > + .ctrlbit = (1 << 8), > + }, > + .sources = &clkset_group2, > + .reg_src = { .reg = S5P_CLK_SRC4, .shift = 0, .size = 4 }, > + .reg_div = { .reg = S5P_CLK_DIV4, .shift = 0, .size = 4 }, > + > +}; > + > +static struct clksrc_clk sclk_mmc1 = { > + .clk = { > + .name = "sclk_mmc", > + .devname = "s3c-sdhci.1", > + .enable = s5pv210_clk_mask0_ctrl, > + .ctrlbit = (1 << 9), > + }, > + .sources = &clkset_group2, > + .reg_src = { .reg = S5P_CLK_SRC4, .shift = 4, .size = 4 }, > + .reg_div = { .reg = S5P_CLK_DIV4, .shift = 4, .size = 4 }, > +}; > + > +static struct clksrc_clk sclk_mmc2 = { > + .clk = { > + .name = "sclk_mmc", > + .devname = "s3c-sdhci.2", > + .enable = s5pv210_clk_mask0_ctrl, > + .ctrlbit = (1 << 10), > + }, > + .sources = &clkset_group2, > + .reg_src = { .reg = S5P_CLK_SRC4, .shift = 8, .size = 4 }, > + .reg_div = { .reg = S5P_CLK_DIV4, .shift = 8, .size = 4 }, > +}; > + > +static struct clksrc_clk sclk_mmc3 = { > + .clk = { > + .name = "sclk_mmc", > + .devname = "s3c-sdhci.3", > + .enable = s5pv210_clk_mask0_ctrl, > + .ctrlbit = (1 << 11), > + }, > + .sources = &clkset_group2, > + .reg_src = { .reg = S5P_CLK_SRC4, .shift = 12, .size = 4 }, > + .reg_div = { .reg = S5P_CLK_DIV4, .shift = 12, .size = 4 }, > +}; > + > +static struct clk_lookup s5pv210_clks[] = { > + CLK("s3c-sdhci.0", "mmc_busclk.0", &hsmmc_clk0), > + CLK("s3c-sdhci.1", "mmc_busclk.0", &hsmmc_clk1), > + CLK("s3c-sdhci.2", "mmc_busclk.0", &hsmmc_clk2), > + CLK("s3c-sdhci.3", "mmc_busclk.0", &hsmmc_clk3), > + CLK("s3c-sdhci.0", "mmc_busclk.2", &sclk_mmc0.clk), > + CLK("s3c-sdhci.1", "mmc_busclk.2", &sclk_mmc1.clk), > + CLK("s3c-sdhci.2", "mmc_busclk.2", &sclk_mmc2.clk), > + CLK("s3c-sdhci.3", "mmc_busclk.2", &sclk_mmc3.clk), > +}; > + > +static struct clk *bus_clk[] = { > + &hsmmc_clk0, > + &hsmmc_clk1, > + &hsmmc_clk2, > + &hsmmc_clk3, > +}; > + > +static struct clksrc_clk *bus_clksrc[] = { > + &sclk_mmc0, > + &sclk_mmc1, > + &sclk_mmc2, > + &sclk_mmc3, > +}; > + > static int s5pv210_epll_set_rate(struct clk *clk, unsigned long rate) > { > unsigned int epll_con, epll_con_k; > @@ -1156,10 +1198,18 @@ void __init s5pv210_register_clocks(void) > s3c_register_clksrc(sysclks[ptr], 1); > > s3c_register_clksrc(clksrcs, ARRAY_SIZE(clksrcs)); > + for (ptr = 0; ptr < ARRAY_SIZE(bus_clksrc); ptr++) > + s3c_register_clksrc(bus_clksrc[ptr], 1); > + > s3c_register_clocks(init_clocks, ARRAY_SIZE(init_clocks)); > > s3c_register_clocks(init_clocks_off, ARRAY_SIZE(init_clocks_off)); > s3c_disable_clocks(init_clocks_off, ARRAY_SIZE(init_clocks_off)); > > + s3c24xx_register_clocks(bus_clk, ARRAY_SIZE(bus_clk)); > + for (ptr = 0; ptr < ARRAY_SIZE(bus_clk); ptr++) > + s3c_disable_clocks(bus_clk[ptr], 1); > + > s3c_pwmclk_init(); > + clkdev_add_table(s5pv210_clks, ARRAY_SIZE(s5pv210_clks)); > > } > diff --git a/arch/arm/plat-samsung/include/plat/clock.h b/arch/arm/plat-samsung/include/plat/clock.h > index 87d5b38..03f70ba 100644 > --- a/arch/arm/plat-samsung/include/plat/clock.h > +++ b/arch/arm/plat-samsung/include/plat/clock.h > @@ -29,6 +29,14 @@ struct clk; > * not be a problem as it is unlikely these operations are going > * to need to be called quickly. > */ > + > +#define CLK(dev, con, ck) \ > + { \ > + .dev_id = dev, \ > + .con_id = con, \ > + .clk = ck, \ > + } \ > + > struct clk_ops { > int (*set_rate)(struct clk *c, unsigned long rate); > unsigned long (*get_rate)(struct clk *c); > -- > 1.7.4.4 > > 8---------------------------------------------------------------------------- > > >> >> Thanks and Regards, >> Banajit Goswami > ... >> >> ------- *Original Message* ------- >> *Sender* : Sylwester Nawrocki<s.nawrocki@samsung.com> Software Engineer/Poland >> R&D Center-Linux (MSD)/Samsung Electronics >> *Date* : Aug 24, 2011 22:29 (GMT+09:00) >> *Title* : Re: Add clkdev support to HSMMC >> >> Hi Banajit, >> >> On 08/24/2011 07:47 AM, RAJESHWARI S SHINDE wrote: >>> Hi Sylwester, >>> >>> >>> This is with regards to your reply for Padmavathi's mail regarding double >>> registration of clocks with clkdev in case of SPI. >>> >>> In the attached patch set registration of bus clocks is happening twice, once >>> with actual clock name and another with generic connection id. This is being >>> done to avoid issue as explained below. >>> >>> When trying to add Clkdev support for the HSMMC, was facing the following issue: >>> >>> We have a peripheral clock with name "hsmmc " which is also being used as a >>> bus clock, for best source calculation in SoC's like S3C64XX, S3C2416, >>> S5PC100 and S5PV210 as shown below: >>> >>> char *s5pv210_hsmmc_clksrcs[4] = { >>> [0] = "hsmmc", /* HCLK */ >>> /* [1] = "hsmmc", - duplicate HCLK entry */ >>> [2] = "sclk_mmc", /* mmc_bus */ >>> /* [3] = NULL, - reserved */ >>> }; >>> >>> Hence when we try to add this "hsmmc" as a bus clock in the clkdev lookup table >>> with a generic name across all the platforms, which will be used by the >>> HSMMC driver, the problem arises as the driver also tries to search for this >>> peripheral clock using the clock name "hsmmc" as shown below and fails: >>> >>> sc->clk_io = clk_get(dev, "hsmmc"); >>> if (IS_ERR(sc->clk_io)) { >>> dev_err(dev, "failed to get io clock\n"); >>> ret = PTR_ERR(sc->clk_io); >>> goto err_io_clk; >>> } >>> >>> I cannot alone use a generic connection id for getting the peripheral clocks as >>> SoC's like S5P64X0 and Exynos4 do not use this peripheral clock as bus clock. >> >> Could you create two separate clock 'connection id' name classes for each SoC ? >> One for the 'bus clock' and one for the the 'other' (hsmmc) clock ? >> Then each mach-* could assign proper struct clk to for the "clk_io" at the driver. >> >> Now you're registering both current "hsmmc" and "sclk_hsmmc" clocks under >> "mmc_busclk.?" name only. >> >> For instance: >> >> >> +static struct clk_lookup s5pv210_clks[] = { >> + CLK("s3c-sdhci.0", "mmc_busclk.0", &hsmmc_clk0), >> + CLK("s3c-sdhci.1", "mmc_busclk.0", &hsmmc_clk1), >> + CLK("s3c-sdhci.2", "mmc_busclk.0", &hsmmc_clk2), >> + CLK("s3c-sdhci.3", "mmc_busclk.0", &hsmmc_clk3), >> + CLK("s3c-sdhci.0", "mmc_busclk.2", &sclk_mmc0.clk), >> + CLK("s3c-sdhci.1", "mmc_busclk.2", &sclk_mmc1.clk), >> + CLK("s3c-sdhci.2", "mmc_busclk.2", &sclk_mmc2.clk), >> + CLK("s3c-sdhci.3", "mmc_busclk.2", &sclk_mmc3.clk), >> >> + CLK("s3c-sdhci.0", "mmc_ioclk", &hsmmc_clk0), >> + CLK("s3c-sdhci.1", "mmc_ioclk", &hsmmc_clk1), >> + CLK("s3c-sdhci.2", "mmc_ioclk", &hsmmc_clk2), >> + CLK("s3c-sdhci.3", "mmc_ioclk", &hsmmc_clk3), >> >> +}; >> >> Then in the driver: >> >> >> - sc->clk_io = clk_get(dev, "hsmmc"); >> + sc->clk_io = clk_get(dev, "mmc_ioclk"); >> if (IS_ERR(sc->clk_io)) { >> dev_err(dev, "failed to get io clock\n"); >> ret = PTR_ERR(sc->clk_io); >> goto err_io_clk; >> } >> >> Does it make sense? >> >> I guess you don't want to modify every mach-s* for this ? >> >> I'm mostly OK with your patches, as they are a step in good direction. >> >>> >>> Hope you understood the problem; do refer the attached patch set for more >>> clarification. >>> >>> Do suggest me if any better solution. >>> >> > > Regards, > -- > Sylwester Nawrocki > Samsung Poland R&D Center > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Add clkdev support to HSMMC 2011-08-31 10:13 ` Thomas Abraham @ 2011-08-31 12:56 ` Sylwester Nawrocki 2011-08-31 13:13 ` Thomas Abraham 0 siblings, 1 reply; 4+ messages in thread From: Sylwester Nawrocki @ 2011-08-31 12:56 UTC (permalink / raw) To: Thomas Abraham Cc: banajit.g, RAJESHWARI S SHINDE, PADMAVATHI VENNA, Kukjin Kim, linux-samsung-soc Hi Thomas, On 08/31/2011 12:13 PM, Thomas Abraham wrote: > > [...] > > >>> 3) In the proposed implementation where we create connection ids only for the >>> bus clocks we are creating an extra node for the same in the clkdev table, >> >>> where as the clock is getting registered only once with the kernel. >> >> I don't agree with this last statement, the same struct clk is added to >> the list of clocks known by the system twice, in s3c24xx_register_clocks() >> and clkdev_add_table(). This is only what I have been pointing out. >> Please correct me, if I missed anything. > > s3c24xx_register_clock and clkdev_add_table add one instance of > "struct clk_lookup" each (with different connections ids) but both of > these "struct clk_lookup" instances point to the same "struct clk" > instance. Yes, that's what happens in Banajit's code. > > So, is two instances of "struct clk_lookup" pointing to the same > "struct clk" instance the concern in this case? No, I don't see this as a problem. Just wanted to get things clarified. And to suggest that we could avoid having re-created struct clk and struct clk_clksrc arrays for s3c24xx_register_clocks() and s3c_register_clksrc() functions. However I don't consider this a significant issue. -- Regards, Sylwester -- Sylwester Nawrocki Samsung Poland R&D Center ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Add clkdev support to HSMMC 2011-08-31 12:56 ` Sylwester Nawrocki @ 2011-08-31 13:13 ` Thomas Abraham 0 siblings, 0 replies; 4+ messages in thread From: Thomas Abraham @ 2011-08-31 13:13 UTC (permalink / raw) To: Sylwester Nawrocki Cc: banajit.g, RAJESHWARI S SHINDE, PADMAVATHI VENNA, Kukjin Kim, linux-samsung-soc Hi Sylwester, On 31 August 2011 18:26, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote: > Hi Thomas, > > On 08/31/2011 12:13 PM, Thomas Abraham wrote: >> >> [...] >> >> >>>> 3) In the proposed implementation where we create connection ids only for the >>>> bus clocks we are creating an extra node for the same in the clkdev table, >>> >>>> where as the clock is getting registered only once with the kernel. >>> >>> I don't agree with this last statement, the same struct clk is added to >>> the list of clocks known by the system twice, in s3c24xx_register_clocks() >>> and clkdev_add_table(). This is only what I have been pointing out. >>> Please correct me, if I missed anything. >> >> s3c24xx_register_clock and clkdev_add_table add one instance of >> "struct clk_lookup" each (with different connections ids) but both of >> these "struct clk_lookup" instances point to the same "struct clk" >> instance. > > Yes, that's what happens in Banajit's code. > >> >> So, is two instances of "struct clk_lookup" pointing to the same >> "struct clk" instance the concern in this case? > > No, I don't see this as a problem. Just wanted to get things clarified. > > And to suggest that we could avoid having re-created struct clk and struct > clk_clksrc arrays for s3c24xx_register_clocks() and s3c_register_clksrc() > functions. However I don't consider this a significant issue. Thanks for letting us know your opinion. Regards, Thomas. > > -- > Regards, > Sylwester > -- > Sylwester Nawrocki > Samsung Poland R&D Center > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-08-31 13:13 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <0LQO00HOLZVZDW20@ms6.samsung.com>
2011-08-31 8:42 ` Add clkdev support to HSMMC Sylwester Nawrocki
2011-08-31 10:13 ` Thomas Abraham
2011-08-31 12:56 ` Sylwester Nawrocki
2011-08-31 13:13 ` Thomas Abraham
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.