Hi Sylwester,
On 31 August 2011 14:12, Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx> 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@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
>
--
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
