Re: Add clkdev support to HSMMC

[Sylwester Nawrocki <s.nawrocki@samsung.com>]

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