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