Re: Add clkdev support to HSMMC

Re: Add clkdev support to HSMMC

[Sylwester Nawrocki]

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

Re: Add clkdev support to HSMMC

[Thomas Abraham]

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
>

Re: Add clkdev support to HSMMC

[Sylwester Nawrocki]

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

Re: Add clkdev support to HSMMC

[Thomas Abraham]

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
>