diff mbox

[1/3] clk: exynos: register audio subsystem clocks using common clock framework

Message ID 1365143029-16936-1-git-send-email-padma.v@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Padmavathi Venna April 5, 2013, 6:23 a.m. UTC
Audio subsystem is introduced in exynos platforms. This has seperate
clock controller which can control i2s0 and pcm0 clocks. This patch
registers the audio subsystem clocks with the common clock framework.

Signed-off-by: Padmavathi Venna <padma.v@samsung.com>
---
 drivers/clk/samsung/Makefile           |    1 +
 drivers/clk/samsung/clk-exynos-audss.c |  139 ++++++++++++++++++++++++++++++++
 2 files changed, 140 insertions(+), 0 deletions(-)
 create mode 100644 drivers/clk/samsung/clk-exynos-audss.c

Comments

On 04/05/2013 08:23 AM, Padmavathi Venna wrote:
> Audio subsystem is introduced in exynos platforms. This has seperate
> clock controller which can control i2s0 and pcm0 clocks. This patch
> registers the audio subsystem clocks with the common clock framework.
> 
> Signed-off-by: Padmavathi Venna <padma.v@samsung.com>
> ---
>  drivers/clk/samsung/Makefile           |    1 +
>  drivers/clk/samsung/clk-exynos-audss.c |  139 ++++++++++++++++++++++++++++++++
>  2 files changed, 140 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/clk/samsung/clk-exynos-audss.c
> 
> diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
> index b7c232e..1876810 100644
> --- a/drivers/clk/samsung/Makefile
> +++ b/drivers/clk/samsung/Makefile
> @@ -6,3 +6,4 @@ obj-$(CONFIG_COMMON_CLK)	+= clk.o clk-pll.o
>  obj-$(CONFIG_ARCH_EXYNOS4)	+= clk-exynos4.o
>  obj-$(CONFIG_SOC_EXYNOS5250)	+= clk-exynos5250.o
>  obj-$(CONFIG_SOC_EXYNOS5440)	+= clk-exynos5440.o
> +obj-$(CONFIG_ARCH_EXYNOS)	+= clk-exynos-audss.o
> diff --git a/drivers/clk/samsung/clk-exynos-audss.c b/drivers/clk/samsung/clk-exynos-audss.c
> new file mode 100644
> index 0000000..d7f9aa8
> --- /dev/null
> +++ b/drivers/clk/samsung/clk-exynos-audss.c
> @@ -0,0 +1,139 @@
> +/*
> + * Copyright (c) 2013 Samsung Electronics Co., Ltd.
> + * Author: Padmavathi Venna <padma.v@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Common Clock Framework support for Audio clks.

s/clks/Subsystem Clock Controller ?

> +*/
> +
> +#include <linux/clkdev.h>
> +#include <linux/io.h>
> +#include <linux/clk-provider.h>
> +#include <linux/of_address.h>
> +#include <linux/syscore_ops.h>
> +
> +static DEFINE_SPINLOCK(lock);
> +static struct clk **clk_table;
> +static void __iomem *reg_base;
> +#ifdef CONFIG_OF

Is this really required ? This driver is for ARCH_EXYNOS which is
going to be DT only anyway - presumably it would depend on OF.

> +static struct clk_onecell_data clk_data;
> +#endif
> +
> +#define ASS_CLK_SRC 0x0
> +#define ASS_CLK_DIV 0x4
> +#define ASS_CLK_GATE 0x8
> +
> +static unsigned long reg_save[][2] = {
> +	{ASS_CLK_SRC,  0x0},

nit: '0x' could be probably omitted.

> +	{ASS_CLK_DIV,  0x0},
> +	{ASS_CLK_GATE, 0x0},
> +};
> +
> +/*
> + * Let each supported clock get a unique id. This id is used to lookup the clock
> + * for device tree based platforms.
> + *
> + * When adding a new clock to this list, it is advised to add it to the end.
> + * That is because the device tree source file is referring to these ids and
> + * any change in the sequence number of existing clocks will require
> + * corresponding change in the device tree files. This limitation would go away
> + * when pre-processor support for dtc would be available.

It's already available. Also please see [1]. IMO the best would be to 
create a header file including #defines for all the clocks indexes as 
per enum exynos_audss_clks. This header would be put in a common location
so it can be included by the driver and the dts files.

> + */
> +enum exynos_audss_clks {
> +	/* audss clocks */
> +	mout_audss, mout_i2s,
> +	dout_srp, dout_bus, dout_i2s,
> +	srp_clk, i2s_bus, sclk_i2s0,
> +
> +	nr_clks,
> +};
> +
> +/* list of all parent clock list */
> +static const char *mout_audss_p[] = { "fin_pll", "fout_epll" };
> +static const char *mout_i2s_p[] = { "mout_audss", "cdclk0", "sclk_audio0" };
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int exynos_audss_clk_suspend(void)
> +{
> +	int i;
> +
> +	for (i = 0; i < 3; i++)
> +		reg_save[i][1] = __raw_readl(reg_base + reg_save[i][0]);

Why using the __raw_* accessors ? I think it should be readl().

> +
> +	return 0;
> +}
> +
> +static void exynos_audss_clk_resume(void)
> +{
> +	int i;
> +
> +	for (i = 0; i < 3; i++)
> +		__raw_writel(reg_save[i][1], reg_base + reg_save[i][0]);

Same here, writel().

> +}
> +
> +static struct syscore_ops exynos_audss_clk_syscore_ops = {
> +	.suspend	= exynos_audss_clk_suspend,
> +	.resume		= exynos_audss_clk_resume,
> +};
> +#endif /* CONFIG_PM_SLEEP */
> +
> +/* register exynos_audss clocks */
> +void __init exynos_audss_clk_init(struct device_node *np)
> +{

Isn't it better to just do

	if (np == NULL)
		return;	

i.e. just skip the initialization altogether ? panic() seems 
unreasonable here.

> +	if (np) {
> +		reg_base = of_iomap(np, 0);
> +		if (!reg_base)
> +			panic("%s: failed to map registers\n", __func__);
> +	} else
> +		panic("%s: unable to determine soc\n", __func__);
> +
> +	clk_table = kzalloc(sizeof(struct clk *) * nr_clks, GFP_KERNEL);
> +	if (!clk_table)
> +		panic("could not allocate clock lookup table\n");
> +
> +	clk_data.clks = clk_table;
> +	clk_data.clk_num = nr_clks;
> +	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
> +
> +	clk_table[mout_audss] = clk_register_mux(NULL, "mout_audss",
> +				mout_audss_p, ARRAY_SIZE(mout_audss_p), 0,
> +				reg_base + ASS_CLK_SRC, 0, 1, 0, &lock);
> +	clk_register_clkdev(clk_table[mout_audss], "mout_audss", NULL);
> +
> +	clk_table[mout_i2s] = clk_register_mux(NULL, "mout_i2s", mout_i2s_p,
> +				ARRAY_SIZE(mout_i2s_p), 0,
> +				reg_base + ASS_CLK_SRC, 2, 2, 0, &lock);
> +	clk_register_clkdev(clk_table[mout_i2s], "mout_i2s", NULL);
> +
> +	clk_table[dout_srp] = clk_register_divider(NULL, "dout_srp",
> +				"mout_audss", 0, reg_base + ASS_CLK_DIV, 0, 4,
> +				0, &lock);
> +
> +	clk_table[dout_bus] = clk_register_divider(NULL, "dout_bus",
> +				"dout_srp", 0, reg_base + ASS_CLK_DIV, 4, 4, 0,
> +				&lock);
> +
> +	clk_table[dout_i2s] = clk_register_divider(NULL, "dout_i2s",
> +				"mout_i2s", 0, reg_base + ASS_CLK_DIV, 8, 4, 0,
> +				&lock);
> +
> +	clk_table[srp_clk] = clk_register_gate(NULL, "srp_clk", "dout_srp",
> +				CLK_SET_RATE_PARENT, reg_base + ASS_CLK_GATE, 0,
> +				0, &lock);
> +
> +	clk_table[i2s_bus] = clk_register_gate(NULL, "i2s_bus", "dout_bus",
> +				CLK_SET_RATE_PARENT, reg_base + ASS_CLK_GATE, 2,

Don't we need to handle also bit 3 of the ASS_CLK_GATE register ?

In some kernels I saw PCM special/PCM Bus and I2S special/I2S Bus bits
associated with same clocks. So there were 2-bit wide masks for each clock.

Tomasz had an idea to add a flag to the clock framework that would be passed
to clk_register_gate() and would indicate that the sixth argument is a bitmask,
rather than a bit index.

Perhaps we could just specify each bit of ASS_CLK_GATE as a separate clock.
Then the drivers would need to be modified. I'm not sure what's best approach.
And if it is really necessary to be enabling/disabling both special/bus clock
gate bits simultaneously. I'm adding Mike to Cc, so perhaps he could provide
us with some pointers.

> +				0, &lock);
> +
> +	clk_table[sclk_i2s0] = clk_register_gate(NULL, "sclk_i2s0", "dout_i2s",
> +				CLK_SET_RATE_PARENT, reg_base + ASS_CLK_GATE, 3,
> +				0, &lock);
> +#ifdef CONFIG_PM_SLEEP
> +	register_syscore_ops(&exynos_audss_clk_syscore_ops);
> +#endif
> +	pr_info("Exynos: Audss: clock setup completed\n");
> +}
> +CLK_OF_DECLARE(exynos_audss_clk, "samsung,exynos-audss-clock", exynos_audss_clk_init);


[1] http://www.spinics.net/lists/linux-kbuild/msg07616.html
--
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
padma venkat April 6, 2013, 10:13 a.m. UTC | #2
Hi Sylwester,

On Fri, Apr 5, 2013 at 7:23 PM, Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
> On 04/05/2013 08:23 AM, Padmavathi Venna wrote:
>> Audio subsystem is introduced in exynos platforms. This has seperate
>> clock controller which can control i2s0 and pcm0 clocks. This patch
>> registers the audio subsystem clocks with the common clock framework.
>>
>> Signed-off-by: Padmavathi Venna <padma.v@samsung.com>
>> ---
>>  drivers/clk/samsung/Makefile           |    1 +
>>  drivers/clk/samsung/clk-exynos-audss.c |  139 ++++++++++++++++++++++++++++++++
>>  2 files changed, 140 insertions(+), 0 deletions(-)
>>  create mode 100644 drivers/clk/samsung/clk-exynos-audss.c
>>
>> diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
>> index b7c232e..1876810 100644
>> --- a/drivers/clk/samsung/Makefile
>> +++ b/drivers/clk/samsung/Makefile
>> @@ -6,3 +6,4 @@ obj-$(CONFIG_COMMON_CLK)      += clk.o clk-pll.o
>>  obj-$(CONFIG_ARCH_EXYNOS4)   += clk-exynos4.o
>>  obj-$(CONFIG_SOC_EXYNOS5250) += clk-exynos5250.o
>>  obj-$(CONFIG_SOC_EXYNOS5440) += clk-exynos5440.o
>> +obj-$(CONFIG_ARCH_EXYNOS)    += clk-exynos-audss.o
>> diff --git a/drivers/clk/samsung/clk-exynos-audss.c b/drivers/clk/samsung/clk-exynos-audss.c
>> new file mode 100644
>> index 0000000..d7f9aa8
>> --- /dev/null
>> +++ b/drivers/clk/samsung/clk-exynos-audss.c
>> @@ -0,0 +1,139 @@
>> +/*
>> + * Copyright (c) 2013 Samsung Electronics Co., Ltd.
>> + * Author: Padmavathi Venna <padma.v@samsung.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * Common Clock Framework support for Audio clks.
>
> s/clks/Subsystem Clock Controller ?

Ok. I will change this.

>
>> +*/
>> +
>> +#include <linux/clkdev.h>
>> +#include <linux/io.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/of_address.h>
>> +#include <linux/syscore_ops.h>
>> +
>> +static DEFINE_SPINLOCK(lock);
>> +static struct clk **clk_table;
>> +static void __iomem *reg_base;
>> +#ifdef CONFIG_OF
>
> Is this really required ? This driver is for ARCH_EXYNOS which is
> going to be DT only anyway - presumably it would depend on OF.

I missed out to delete this. It's not required.

>
>> +static struct clk_onecell_data clk_data;
>> +#endif
>> +
>> +#define ASS_CLK_SRC 0x0
>> +#define ASS_CLK_DIV 0x4
>> +#define ASS_CLK_GATE 0x8
>> +
>> +static unsigned long reg_save[][2] = {
>> +     {ASS_CLK_SRC,  0x0},
>
> nit: '0x' could be probably omitted.

Ok.

>
>> +     {ASS_CLK_DIV,  0x0},
>> +     {ASS_CLK_GATE, 0x0},
>> +};
>> +
>> +/*
>> + * Let each supported clock get a unique id. This id is used to lookup the clock
>> + * for device tree based platforms.
>> + *
>> + * When adding a new clock to this list, it is advised to add it to the end.
>> + * That is because the device tree source file is referring to these ids and
>> + * any change in the sequence number of existing clocks will require
>> + * corresponding change in the device tree files. This limitation would go away
>> + * when pre-processor support for dtc would be available.
>
> It's already available. Also please see [1]. IMO the best would be to
> create a header file including #defines for all the clocks indexes as
> per enum exynos_audss_clks. This header would be put in a common location
> so it can be included by the driver and the dts files.

Ok. I will include it.

>
>> + */
>> +enum exynos_audss_clks {
>> +     /* audss clocks */
>> +     mout_audss, mout_i2s,
>> +     dout_srp, dout_bus, dout_i2s,
>> +     srp_clk, i2s_bus, sclk_i2s0,
>> +
>> +     nr_clks,
>> +};
>> +
>> +/* list of all parent clock list */
>> +static const char *mout_audss_p[] = { "fin_pll", "fout_epll" };
>> +static const char *mout_i2s_p[] = { "mout_audss", "cdclk0", "sclk_audio0" };
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int exynos_audss_clk_suspend(void)
>> +{
>> +     int i;
>> +
>> +     for (i = 0; i < 3; i++)
>> +             reg_save[i][1] = __raw_readl(reg_base + reg_save[i][0]);
>
> Why using the __raw_* accessors ? I think it should be readl().

Ok. I will change this.

>
>> +
>> +     return 0;
>> +}
>> +
>> +static void exynos_audss_clk_resume(void)
>> +{
>> +     int i;
>> +
>> +     for (i = 0; i < 3; i++)
>> +             __raw_writel(reg_save[i][1], reg_base + reg_save[i][0]);
>
> Same here, writel().
>
>> +}
>> +
>> +static struct syscore_ops exynos_audss_clk_syscore_ops = {
>> +     .suspend        = exynos_audss_clk_suspend,
>> +     .resume         = exynos_audss_clk_resume,
>> +};
>> +#endif /* CONFIG_PM_SLEEP */
>> +
>> +/* register exynos_audss clocks */
>> +void __init exynos_audss_clk_init(struct device_node *np)
>> +{
>
> Isn't it better to just do
>
>         if (np == NULL)
>                 return;
>
> i.e. just skip the initialization altogether ? panic() seems
> unreasonable here.

Ok. I will change this.

>
>> +     if (np) {
>> +             reg_base = of_iomap(np, 0);
>> +             if (!reg_base)
>> +                     panic("%s: failed to map registers\n", __func__);
>> +     } else
>> +             panic("%s: unable to determine soc\n", __func__);
>> +
>> +     clk_table = kzalloc(sizeof(struct clk *) * nr_clks, GFP_KERNEL);
>> +     if (!clk_table)
>> +             panic("could not allocate clock lookup table\n");
>> +
>> +     clk_data.clks = clk_table;
>> +     clk_data.clk_num = nr_clks;
>> +     of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
>> +
>> +     clk_table[mout_audss] = clk_register_mux(NULL, "mout_audss",
>> +                             mout_audss_p, ARRAY_SIZE(mout_audss_p), 0,
>> +                             reg_base + ASS_CLK_SRC, 0, 1, 0, &lock);
>> +     clk_register_clkdev(clk_table[mout_audss], "mout_audss", NULL);
>> +
>> +     clk_table[mout_i2s] = clk_register_mux(NULL, "mout_i2s", mout_i2s_p,
>> +                             ARRAY_SIZE(mout_i2s_p), 0,
>> +                             reg_base + ASS_CLK_SRC, 2, 2, 0, &lock);
>> +     clk_register_clkdev(clk_table[mout_i2s], "mout_i2s", NULL);
>> +
>> +     clk_table[dout_srp] = clk_register_divider(NULL, "dout_srp",
>> +                             "mout_audss", 0, reg_base + ASS_CLK_DIV, 0, 4,
>> +                             0, &lock);
>> +
>> +     clk_table[dout_bus] = clk_register_divider(NULL, "dout_bus",
>> +                             "dout_srp", 0, reg_base + ASS_CLK_DIV, 4, 4, 0,
>> +                             &lock);
>> +
>> +     clk_table[dout_i2s] = clk_register_divider(NULL, "dout_i2s",
>> +                             "mout_i2s", 0, reg_base + ASS_CLK_DIV, 8, 4, 0,
>> +                             &lock);
>> +
>> +     clk_table[srp_clk] = clk_register_gate(NULL, "srp_clk", "dout_srp",
>> +                             CLK_SET_RATE_PARENT, reg_base + ASS_CLK_GATE, 0,
>> +                             0, &lock);
>> +
>> +     clk_table[i2s_bus] = clk_register_gate(NULL, "i2s_bus", "dout_bus",
>> +                             CLK_SET_RATE_PARENT, reg_base + ASS_CLK_GATE, 2,
>
> Don't we need to handle also bit 3 of the ASS_CLK_GATE register ?

Bit 3 is handled below as sclk_i2s0.

>
> In some kernels I saw PCM special/PCM Bus and I2S special/I2S Bus bits
> associated with same clocks. So there were 2-bit wide masks for each clock.
>
> Tomasz had an idea to add a flag to the clock framework that would be passed
> to clk_register_gate() and would indicate that the sixth argument is a bitmask,
> rather than a bit index.

For I2S there are separate clock instances and bits in the clock
controller named as busclk[bit:2] and i2s_clk[bit:3].
But PCM case there are two bits available[bit 4 and 5] but I think
only one clock instance available(need to check
clk controller diagram). But anyway having the bitmask in the
clk_register_gate may be useful.

>
> Perhaps we could just specify each bit of ASS_CLK_GATE as a separate clock.
> Then the drivers would need to be modified. I'm not sure what's best approach.
> And if it is really necessary to be enabling/disabling both special/bus clock
> gate bits simultaneously. I'm adding Mike to Cc, so perhaps he could provide
> us with some pointers.
>
>> +                             0, &lock);
>> +
>> +     clk_table[sclk_i2s0] = clk_register_gate(NULL, "sclk_i2s0", "dout_i2s",
>> +                             CLK_SET_RATE_PARENT, reg_base + ASS_CLK_GATE, 3,
>> +                             0, &lock);
>> +#ifdef CONFIG_PM_SLEEP
>> +     register_syscore_ops(&exynos_audss_clk_syscore_ops);
>> +#endif
>> +     pr_info("Exynos: Audss: clock setup completed\n");
>> +}
>> +CLK_OF_DECLARE(exynos_audss_clk, "samsung,exynos-audss-clock", exynos_audss_clk_init);
>
>
> [1] http://www.spinics.net/lists/linux-kbuild/msg07616.html
--
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
Hi,

On 04/06/2013 12:13 PM, Padma Venkat wrote:
> On Fri, Apr 5, 2013 at 7:23 PM, Sylwester Nawrocki
> <s.nawrocki@samsung.com> wrote:
>> On 04/05/2013 08:23 AM, Padmavathi Venna wrote:
>>> Audio subsystem is introduced in exynos platforms. This has seperate
>>> clock controller which can control i2s0 and pcm0 clocks. This patch
>>> registers the audio subsystem clocks with the common clock framework.
>>>
>>> Signed-off-by: Padmavathi Venna <padma.v@samsung.com>
>>> ---
>>>  drivers/clk/samsung/Makefile           |    1 +
>>>  drivers/clk/samsung/clk-exynos-audss.c |  139 ++++++++++++++++++++++++++++++++
>>>  2 files changed, 140 insertions(+), 0 deletions(-)
>>>  create mode 100644 drivers/clk/samsung/clk-exynos-audss.c
[...]
>>> diff --git a/drivers/clk/samsung/clk-exynos-audss.c b/drivers/clk/samsung/clk-exynos-audss.c
>>> new file mode 100644
>>> index 0000000..d7f9aa8
>>> --- /dev/null
>>> +++ b/drivers/clk/samsung/clk-exynos-audss.c
[...]
>>> +/* register exynos_audss clocks */
>>> +void __init exynos_audss_clk_init(struct device_node *np)
>>> +{
>>
>> Isn't it better to just do
>>
>>         if (np == NULL)
>>                 return;
>>
>> i.e. just skip the initialization altogether ? panic() seems
>> unreasonable here.
> 
> Ok. I will change this.
> 
>>
>>> +     if (np) {
>>> +             reg_base = of_iomap(np, 0);
>>> +             if (!reg_base)
>>> +                     panic("%s: failed to map registers\n", __func__);
>>> +     } else
>>> +             panic("%s: unable to determine soc\n", __func__);
>>> +
>>> +     clk_table = kzalloc(sizeof(struct clk *) * nr_clks, GFP_KERNEL);
>>> +     if (!clk_table)
>>> +             panic("could not allocate clock lookup table\n");
>>> +
>>> +     clk_data.clks = clk_table;
>>> +     clk_data.clk_num = nr_clks;
>>> +     of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
>>> +
>>> +     clk_table[mout_audss] = clk_register_mux(NULL, "mout_audss",
>>> +                             mout_audss_p, ARRAY_SIZE(mout_audss_p), 0,
>>> +                             reg_base + ASS_CLK_SRC, 0, 1, 0, &lock);
>>> +     clk_register_clkdev(clk_table[mout_audss], "mout_audss", NULL);
>>> +
>>> +     clk_table[mout_i2s] = clk_register_mux(NULL, "mout_i2s", mout_i2s_p,
>>> +                             ARRAY_SIZE(mout_i2s_p), 0,
>>> +                             reg_base + ASS_CLK_SRC, 2, 2, 0, &lock);
>>> +     clk_register_clkdev(clk_table[mout_i2s], "mout_i2s", NULL);
>>> +
>>> +     clk_table[dout_srp] = clk_register_divider(NULL, "dout_srp",
>>> +                             "mout_audss", 0, reg_base + ASS_CLK_DIV, 0, 4,
>>> +                             0, &lock);
>>> +
>>> +     clk_table[dout_bus] = clk_register_divider(NULL, "dout_bus",
>>> +                             "dout_srp", 0, reg_base + ASS_CLK_DIV, 4, 4, 0,
>>> +                             &lock);
>>> +
>>> +     clk_table[dout_i2s] = clk_register_divider(NULL, "dout_i2s",
>>> +                             "mout_i2s", 0, reg_base + ASS_CLK_DIV, 8, 4, 0,
>>> +                             &lock);
>>> +
>>> +     clk_table[srp_clk] = clk_register_gate(NULL, "srp_clk", "dout_srp",
>>> +                             CLK_SET_RATE_PARENT, reg_base + ASS_CLK_GATE, 0,
>>> +                             0, &lock);
>>> +
>>> +     clk_table[i2s_bus] = clk_register_gate(NULL, "i2s_bus", "dout_bus",
>>> +                             CLK_SET_RATE_PARENT, reg_base + ASS_CLK_GATE, 2,
>>
>> Don't we need to handle also bit 3 of the ASS_CLK_GATE register ?
> 
> Bit 3 is handled below as sclk_i2s0.

Oops, sorry, my fault.

>> In some kernels I saw PCM special/PCM Bus and I2S special/I2S Bus bits
>> associated with same clocks. So there were 2-bit wide masks for each clock.
>>
>> Tomasz had an idea to add a flag to the clock framework that would be passed
>> to clk_register_gate() and would indicate that the sixth argument is a bitmask,
>> rather than a bit index.
> 
> For I2S there are separate clock instances and bits in the clock
> controller named as busclk[bit:2] and i2s_clk[bit:3].

Indeed.

> But PCM case there are two bits available[bit 4 and 5] but I think
> only one clock instance available(need to check
> clk controller diagram). But anyway having the bitmask in the
> clk_register_gate may be useful.

Yes, from the diagram there seems to be only one clock (SCLK_PCM0) but there 
are 2 in the clock gate register (PCM Special, PCM bus). I guess it's best to
describe those as 2 clocks, and handle them in the driver this way. Instead 
of merging presumably two separate physical clocks in a single clock object.

I suppose "PCM Special" (SCLK_PCMx) is only shown on the Audio Subsystem
diagrams, since it's mostly related to the sound data flow. And the PCM bus
is the PCM IP block bus clock, which is important for inter-working of the
PCM bus with rest of the system.

One option would be to provide a dummy clock for the second PCM clock, for 
systems where it is not available. So the driver always handles two clocks.

>> Perhaps we could just specify each bit of ASS_CLK_GATE as a separate clock.
>> Then the drivers would need to be modified. I'm not sure what's best approach.
>> And if it is really necessary to be enabling/disabling both special/bus clock
>> gate bits simultaneously. I'm adding Mike to Cc, so perhaps he could provide
>> us with some pointers.
>>
>>> +                             0, &lock);
>>> +
>>> +     clk_table[sclk_i2s0] = clk_register_gate(NULL, "sclk_i2s0", "dout_i2s",
>>> +                             CLK_SET_RATE_PARENT, reg_base + ASS_CLK_GATE, 3,
>>> +                             0, &lock);
>>> +#ifdef CONFIG_PM_SLEEP
>>> +     register_syscore_ops(&exynos_audss_clk_syscore_ops);
>>> +#endif
>>> +     pr_info("Exynos: Audss: clock setup completed\n");
>>> +}
>>> +CLK_OF_DECLARE(exynos_audss_clk, "samsung,exynos-audss-clock", exynos_audss_clk_init);
>>
>>
>> [1] http://www.spinics.net/lists/linux-kbuild/msg07616.html

Regards,
Sylwester
--
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
diff mbox

Patch

diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
index b7c232e..1876810 100644
--- a/drivers/clk/samsung/Makefile
+++ b/drivers/clk/samsung/Makefile
@@ -6,3 +6,4 @@  obj-$(CONFIG_COMMON_CLK)	+= clk.o clk-pll.o
 obj-$(CONFIG_ARCH_EXYNOS4)	+= clk-exynos4.o
 obj-$(CONFIG_SOC_EXYNOS5250)	+= clk-exynos5250.o
 obj-$(CONFIG_SOC_EXYNOS5440)	+= clk-exynos5440.o
+obj-$(CONFIG_ARCH_EXYNOS)	+= clk-exynos-audss.o
diff --git a/drivers/clk/samsung/clk-exynos-audss.c b/drivers/clk/samsung/clk-exynos-audss.c
new file mode 100644
index 0000000..d7f9aa8
--- /dev/null
+++ b/drivers/clk/samsung/clk-exynos-audss.c
@@ -0,0 +1,139 @@ 
+/*
+ * Copyright (c) 2013 Samsung Electronics Co., Ltd.
+ * Author: Padmavathi Venna <padma.v@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Common Clock Framework support for Audio clks.
+*/
+
+#include <linux/clkdev.h>
+#include <linux/io.h>
+#include <linux/clk-provider.h>
+#include <linux/of_address.h>
+#include <linux/syscore_ops.h>
+
+static DEFINE_SPINLOCK(lock);
+static struct clk **clk_table;
+static void __iomem *reg_base;
+#ifdef CONFIG_OF
+static struct clk_onecell_data clk_data;
+#endif
+
+#define ASS_CLK_SRC 0x0
+#define ASS_CLK_DIV 0x4
+#define ASS_CLK_GATE 0x8
+
+static unsigned long reg_save[][2] = {
+	{ASS_CLK_SRC,  0x0},
+	{ASS_CLK_DIV,  0x0},
+	{ASS_CLK_GATE, 0x0},
+};
+
+/*
+ * Let each supported clock get a unique id. This id is used to lookup the clock
+ * for device tree based platforms.
+ *
+ * When adding a new clock to this list, it is advised to add it to the end.
+ * That is because the device tree source file is referring to these ids and
+ * any change in the sequence number of existing clocks will require
+ * corresponding change in the device tree files. This limitation would go away
+ * when pre-processor support for dtc would be available.
+ */
+enum exynos_audss_clks {
+	/* audss clocks */
+	mout_audss, mout_i2s,
+	dout_srp, dout_bus, dout_i2s,
+	srp_clk, i2s_bus, sclk_i2s0,
+
+	nr_clks,
+};
+
+/* list of all parent clock list */
+static const char *mout_audss_p[] = { "fin_pll", "fout_epll" };
+static const char *mout_i2s_p[] = { "mout_audss", "cdclk0", "sclk_audio0" };
+
+#ifdef CONFIG_PM_SLEEP
+static int exynos_audss_clk_suspend(void)
+{
+	int i;
+
+	for (i = 0; i < 3; i++)
+		reg_save[i][1] = __raw_readl(reg_base + reg_save[i][0]);
+
+	return 0;
+}
+
+static void exynos_audss_clk_resume(void)
+{
+	int i;
+
+	for (i = 0; i < 3; i++)
+		__raw_writel(reg_save[i][1], reg_base + reg_save[i][0]);
+}
+
+static struct syscore_ops exynos_audss_clk_syscore_ops = {
+	.suspend	= exynos_audss_clk_suspend,
+	.resume		= exynos_audss_clk_resume,
+};
+#endif /* CONFIG_PM_SLEEP */
+
+/* register exynos_audss clocks */
+void __init exynos_audss_clk_init(struct device_node *np)
+{
+	if (np) {
+		reg_base = of_iomap(np, 0);
+		if (!reg_base)
+			panic("%s: failed to map registers\n", __func__);
+	} else
+		panic("%s: unable to determine soc\n", __func__);
+
+	clk_table = kzalloc(sizeof(struct clk *) * nr_clks, GFP_KERNEL);
+	if (!clk_table)
+		panic("could not allocate clock lookup table\n");
+
+	clk_data.clks = clk_table;
+	clk_data.clk_num = nr_clks;
+	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
+
+	clk_table[mout_audss] = clk_register_mux(NULL, "mout_audss",
+				mout_audss_p, ARRAY_SIZE(mout_audss_p), 0,
+				reg_base + ASS_CLK_SRC, 0, 1, 0, &lock);
+	clk_register_clkdev(clk_table[mout_audss], "mout_audss", NULL);
+
+	clk_table[mout_i2s] = clk_register_mux(NULL, "mout_i2s", mout_i2s_p,
+				ARRAY_SIZE(mout_i2s_p), 0,
+				reg_base + ASS_CLK_SRC, 2, 2, 0, &lock);
+	clk_register_clkdev(clk_table[mout_i2s], "mout_i2s", NULL);
+
+	clk_table[dout_srp] = clk_register_divider(NULL, "dout_srp",
+				"mout_audss", 0, reg_base + ASS_CLK_DIV, 0, 4,
+				0, &lock);
+
+	clk_table[dout_bus] = clk_register_divider(NULL, "dout_bus",
+				"dout_srp", 0, reg_base + ASS_CLK_DIV, 4, 4, 0,
+				&lock);
+
+	clk_table[dout_i2s] = clk_register_divider(NULL, "dout_i2s",
+				"mout_i2s", 0, reg_base + ASS_CLK_DIV, 8, 4, 0,
+				&lock);
+
+	clk_table[srp_clk] = clk_register_gate(NULL, "srp_clk", "dout_srp",
+				CLK_SET_RATE_PARENT, reg_base + ASS_CLK_GATE, 0,
+				0, &lock);
+
+	clk_table[i2s_bus] = clk_register_gate(NULL, "i2s_bus", "dout_bus",
+				CLK_SET_RATE_PARENT, reg_base + ASS_CLK_GATE, 2,
+				0, &lock);
+
+	clk_table[sclk_i2s0] = clk_register_gate(NULL, "sclk_i2s0", "dout_i2s",
+				CLK_SET_RATE_PARENT, reg_base + ASS_CLK_GATE, 3,
+				0, &lock);
+#ifdef CONFIG_PM_SLEEP
+	register_syscore_ops(&exynos_audss_clk_syscore_ops);
+#endif
+	pr_info("Exynos: Audss: clock setup completed\n");
+}
+CLK_OF_DECLARE(exynos_audss_clk, "samsung,exynos-audss-clock", exynos_audss_clk_init);