Hi Tomasz,
On Fri, May 10, 2013 at 5:51 AM, Tomasz Figa <tomasz.figa@xxxxxxxxx> wrote:
> Hi Padmavathi,
>
> I managed to review the patch a bit more thoroughly and I had few more
> comments. You can find them inline.
Thanks for the review.
>
> On Tuesday 07 of May 2013 12:13:34 Padmavathi Venna wrote:
>> Audio subsystem is introduced in s5pv210 and 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>
>> ---
>> .../bindings/clock/clk-samsung-audss.txt | 64 +++++++++
>> drivers/clk/samsung/Makefile | 1 +
>> drivers/clk/samsung/clk-samsung-audss.c | 137
>> ++++++++++++++++++++ include/dt-bindings/clk/samsung-audss-clk.h
>> | 25 ++++ 4 files changed, 227 insertions(+), 0 deletions(-)
>> create mode 100644
>> Documentation/devicetree/bindings/clock/clk-samsung-audss.txt create
>> mode 100644 drivers/clk/samsung/clk-samsung-audss.c
>> create mode 100644 include/dt-bindings/clk/samsung-audss-clk.h
>>
>> diff --git
>> a/Documentation/devicetree/bindings/clock/clk-samsung-audss.txt
>> b/Documentation/devicetree/bindings/clock/clk-samsung-audss.txt new
>> file mode 100644
>> index 0000000..ec2cd0b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/clk-samsung-audss.txt
>> @@ -0,0 +1,64 @@
>> +* Samsung Audio Subsystem Clock Controller
>> +
>> +The Samsung Audio Subsystem clock controller generates and supplies
>> clocks +to Audio Subsystem block available in the S5PV210 and Exynos
>> SoCs. The clock +binding described here is applicable to all SoC's in
>> the S5PV210 and Exynos +family.
>> +
>> +Required Properties:
>> +
>> +- compatible: should be one of the following:
>> + - "samsung,s5pv210-audss-clock" - controller compatible with all
>> S5PV210 SoCs. + - "samsung,exynos4210-audss-clock" - controller
>> compatible with all Exynos4 SoCs. + - "samsung,exynos5250-audss-clock"
>> - controller compatible with all Exynos5 SoCs. +
>> +- reg: physical base address and length of the controller's register
>> set. +
>> +- #clock-cells: should be 1.
>> +
>> +The following is the list of clocks generated by the controller. Each
>> clock is +assigned an identifier and client nodes use this identifier
>> to specify the +clock which they consume. Some of the clocks are
>> available only on a particular +Exynos4 SoC and this is specified where
>> applicable.
>> +
>> +Provided clocks:
>> +
>> +Clock ID SoC (if specific)
>> +-----------------------------------------------
>> +
>> +mout_audss 0
>> +mout_i2s 1
>> +dout_srp 2
>> +dout_bus 3
>> +dout_i2s 4
>> +srp_clk 5
>> +i2s_bus 6
>> +sclk_i2s 7
>> +pcm_bus 8
>> +sclk_pcm 9
>> +
>> +Example 1: An example of a clock controller node is listed below.
>> +
>> +clock_audss: audss-clock-controller@03810000 {
>> + compatible = "samsung,exynos5250-audss-clock";
>> + reg = <0x03810000 0x0C>;
>> + #clock-cells = <1>;
>> +};
>> +
>> +Example 2: I2S controller node that consumes the clock generated by the
>> clock + controller. Refer to the standard clock bindings for
>> information + about 'clocks' and 'clock-names' property.
>> +
>> + i2s0: i2s@03830000 {
>> + compatible = "samsung,i2s-v5";
>> + reg = <0x03830000 0x100>;
>> + dmas = <&pdma0 10
>> + &pdma0 9
>> + &pdma0 8>;
>> + dma-names = "tx", "rx", "tx-sec";
>> + clocks = <&clock_audss SAMSUNG_I2S_BUS>,
>> + <&clock_audss SAMSUNG_I2S_BUS>,
>> + <&clock_audss SAMSUNG_SCLK_I2S>;
>> + clock-names = "iis", "i2s_opclk0", "i2s_opclk1";
>> +};
>> +
>> diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
>> index b7c232e..5425fa8 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_PLAT_SAMSUNG) += clk-samsung-audss.o
>> diff --git a/drivers/clk/samsung/clk-samsung-audss.c
>> b/drivers/clk/samsung/clk-samsung-audss.c new file mode 100644
>> index 0000000..97526b7
>> --- /dev/null
>> +++ b/drivers/clk/samsung/clk-samsung-audss.c
>> @@ -0,0 +1,137 @@
>> +/*
>> + * 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 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>
>> +
>> +#include <dt-bindings/clk/samsung-audss-clk.h>
>> +
>> +static DEFINE_SPINLOCK(lock);
>> +static struct clk **clk_table;
>> +static void __iomem *reg_base;
>> +static struct clk_onecell_data clk_data;
>> +
>> +#define ASS_CLK_SRC 0x0
>> +#define ASS_CLK_DIV 0x4
>> +#define ASS_CLK_GATE 0x8
>> +
>> +static unsigned long reg_save[][2] = {
>> + {ASS_CLK_SRC, 0},
>> + {ASS_CLK_DIV, 0},
>> + {ASS_CLK_GATE, 0},
>> +};
>> +
>> +/* 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 samsung_audss_clk_suspend(void)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < 3; i++)
>> + reg_save[i][1] = readl(reg_base + reg_save[i][0]);
>> +
>> + return 0;
>> +}
>> +
>> +static void samsung_audss_clk_resume(void)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < 3; i++)
>> + writel(reg_save[i][1], reg_base + reg_save[i][0]);
>> +}
>> +
>> +static struct syscore_ops samsung_audss_clk_syscore_ops = {
>> + .suspend = samsung_audss_clk_suspend,
>> + .resume = samsung_audss_clk_resume,
>> +};
>> +#endif /* CONFIG_PM_SLEEP */
>> +
>> +/* register samsung_audss clocks */
>> +void __init samsung_audss_clk_init(struct device_node *np)
>> +{
>> + reg_base = of_iomap(np, 0);
>> + if (!reg_base) {
>> + pr_err("%s: failed to map audss registers\n", __func__);
>> + return;
>> + }
>> +
>> + clk_table = kzalloc(sizeof(struct clk *) * SAMSUNG_AUDSS_MAX_CLKS,
>> + GFP_KERNEL);
>> + if (!clk_table) {
>> + pr_err("%s: could not allocate clock lookup table\n",
> __func__);
>> + return;
>> + }
>> +
>> + clk_data.clks = clk_table;
>> + clk_data.clk_num = SAMSUNG_AUDSS_MAX_CLKS;
>> + of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
>> +
>> + clk_table[SAMSUNG_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[SAMSUNG_MOUT_AUDSS], "mout_audss",
>> NULL); +
>
> Is there are a reason for this clkdev lookup to be registered?
>
> This driver seems to be used only in DT-case, so DT-based lookup should be
> enough.
Ok. I will not register these clocks with clkdev. I will add as clock
entries into i2s0 node.
>
>> + clk_table[SAMSUNG_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[SAMSUNG_MOUT_I2S], "mout_i2s",
> NULL);
>
> Same here.
>
>> +
>> + clk_table[SAMSUNG_DOUT_SRP] = clk_register_divider(NULL,
> "dout_srp",
>> + "mout_audss", 0, reg_base + ASS_CLK_DIV,
> 0, 4,
>> + 0, &lock);
>> +
>> + clk_table[SAMSUNG_DOUT_BUS] = clk_register_divider(NULL,
> "dout_bus",
>> + "dout_srp", 0, reg_base + ASS_CLK_DIV, 4,
> 4, 0,
>> + &lock);
>> +
>> + clk_table[SAMSUNG_DOUT_I2S] = clk_register_divider(NULL,
> "dout_i2s",
>> + "mout_i2s", 0, reg_base + ASS_CLK_DIV, 8,
> 4, 0,
>> + &lock);
>> +
>> + clk_table[SAMSUNG_SRP_CLK] = clk_register_gate(NULL, "srp_clk",
>> + "dout_srp", CLK_SET_RATE_PARENT,
>> + reg_base + ASS_CLK_GATE, 0, 0, &lock);
>> +
>> + clk_table[SAMSUNG_I2S_BUS] = clk_register_gate(NULL, "i2s_bus",
>> + "dout_bus", CLK_SET_RATE_PARENT,
>> + reg_base + ASS_CLK_GATE, 2, 0, &lock);
>> +
>> + clk_table[SAMSUNG_SCLK_I2S] = clk_register_gate(NULL, "sclk_i2s",
>> + "dout_i2s", CLK_SET_RATE_PARENT,
>> + reg_base + ASS_CLK_GATE, 3, 0, &lock);
>> +
>> + clk_table[SAMSUNG_PCM_BUS] = clk_register_gate(NULL, "pcm_bus",
>> + "sclk_pcm", CLK_SET_RATE_PARENT,
>> + reg_base + ASS_CLK_GATE, 4, 0, &lock);
>> +
>> + clk_table[SAMSUNG_SCLK_PCM] = clk_register_gate(NULL, "sclk_pcm",
>> + "div_pcm0", CLK_SET_RATE_PARENT,
>> + reg_base + ASS_CLK_GATE, 5, 0, &lock);
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> + register_syscore_ops(&samsung_audss_clk_syscore_ops);
>> +#endif
>> +
>> + pr_info("Exynos: Audss: clock setup completed\n");
>> +}
>> +CLK_OF_DECLARE(s5pv210_audss_clk, "samsung,s5pv210-audss-clock",
>> + samsung_audss_clk_init);
>
> I'm wondering if this driver in its current state is really compatible
> with S5PV210. Quick look at documentation shows that CLK_GATE and CLK_DIV
> registers on this SoC have different layout than on Exynos SoCs.
Yes. There are some differences. As of now I will remove v210.
>
> I guess that for now the best choice would be to remove any mentions about
> S5PV210 from the patch and add them back after the driver gets proper
> support for this SoC.
>
>> +CLK_OF_DECLARE(exynos4210_audss_clk, "samsung,exynos4210-audss-clock",
>> + samsung_audss_clk_init);
>> +CLK_OF_DECLARE(exynos5250_audss_clk, "samsung,exynos5250-audss-clock",
>> + samsung_audss_clk_init);
>
> Also if both Exynos4210 and Exynos5250 have exactly the same audss clock
> layout, there is no reason to have two compatibles for them - the
> convention is that just the first model that had this hardware is enough -
> in this case Exynos4210.
>
> Having two different compatibles suggests that those two SoCs differ in a
> way that needs special handling, which is misleading, based on the fact
> that there is no such special handling in the driver.
There is only one difference between Exynos4 and Exynos5 is bit 1 of
CLK_GATE register where in Exynos5 it is reserved and Exynos4 it is
gate to IntMEM. I am not sure if we use this bit some where? So is it
okey to have same compatible with this diff?
I will post next version of patches after the dependencies are merged
into mainline(dtc+cpp patches by Stephen).
>
> Best regards,
> Tomasz
>
Thanks
Padma
--
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
