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@xxxxxxxxxxx>
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@xxxxxxxxxxx>
---
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@xxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
