From mboxrd@z Thu Jan 1 00:00:00 1970 From: tomasz.figa@gmail.com (Tomasz Figa) Date: Sun, 09 Feb 2014 03:25:21 +0100 Subject: [PATCH 03/12] clk: samsung: add clock driver for external clock outputs In-Reply-To: <201312131359.36576.heiko@sntech.de> References: <201312131356.40755.heiko@sntech.de> <201312131359.36576.heiko@sntech.de> Message-ID: <52F6E711.4020109@gmail.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Heiko, On 13.12.2013 13:59, Heiko St?bner wrote: > This adds a driver for controlling the external clock outputs of > s3c24xx architectures including the dclk muxes and dividers. > > Signed-off-by: Heiko Stuebner > --- > drivers/clk/samsung/Makefile | 1 + > drivers/clk/samsung/clk-s3c2410-dclk.c | 517 ++++++++++++++++++++++ > include/dt-bindings/clock/samsung,s3c2410-dclk.h | 28 ++ > 3 files changed, 546 insertions(+) > create mode 100644 drivers/clk/samsung/clk-s3c2410-dclk.c > create mode 100644 include/dt-bindings/clock/samsung,s3c2410-dclk.h > > diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile > index 4c892c6..568683c 100644 > --- a/drivers/clk/samsung/Makefile > +++ b/drivers/clk/samsung/Makefile > @@ -8,5 +8,6 @@ obj-$(CONFIG_SOC_EXYNOS5250) += clk-exynos5250.o > obj-$(CONFIG_SOC_EXYNOS5420) += clk-exynos5420.o > obj-$(CONFIG_SOC_EXYNOS5440) += clk-exynos5440.o > obj-$(CONFIG_ARCH_EXYNOS) += clk-exynos-audss.o > +obj-$(CONFIG_S3C2410_COMMON_DCLK)+= clk-s3c2410-dclk.o > obj-$(CONFIG_S3C2443_COMMON_CLK)+= clk-s3c2443.o > obj-$(CONFIG_ARCH_S3C64XX) += clk-s3c64xx.o > diff --git a/drivers/clk/samsung/clk-s3c2410-dclk.c b/drivers/clk/samsung/clk-s3c2410-dclk.c > new file mode 100644 > index 0000000..de10e5c > --- /dev/null > +++ b/drivers/clk/samsung/clk-s3c2410-dclk.c > @@ -0,0 +1,517 @@ > +/* > + * Copyright (c) 2013 Heiko Stuebner > + * > + * 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 s3c24xx external clock output. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include "clk.h" > + > +/* legacy access to misccr, until dt conversion is finished */ > +#include > +#include > + > +enum supported_socs { > + S3C2410, > + S3C2412, > + S3C2440, > + S3C2443, > +}; > + > +struct s3c24xx_dclk_drv_data { > + int cpu_type; > +}; > + > +/* > + * Clock for output-parent selection in misccr > + */ > + > +struct s3c24xx_clkout { > + struct clk_hw hw; > + struct regmap *misccr; > + u32 mask; > + u8 shift; > +}; > + > +#define to_s3c24xx_clkout(_hw) container_of(_hw, struct s3c24xx_clkout, hw) > + > +static u8 s3c24xx_clkout_get_parent(struct clk_hw *hw) > +{ > + struct s3c24xx_clkout *clkout = to_s3c24xx_clkout(hw); > + int num_parents = __clk_get_num_parents(hw->clk); > + u32 val; > + int ret = 0; > + > + if (clkout->misccr) > + ret = regmap_read(clkout->misccr, 0, &val); > + else > + val = readl_relaxed(S3C24XX_MISCCR) >> clkout->shift; I wonder if this couldn't be simplified by always providing a regmap. > + > + if (ret) > + return ret; > + > + val >>= clkout->shift; > + val &= clkout->mask; > + > + if (val >= num_parents) > + return -EINVAL; > + > + return val; > +} [snip] > +#define to_s3c24xx_dclk0(x) \ > + container_of(x, struct s3c24xx_dclk, dclk0_div_change_nb) > + > +#define to_s3c24xx_dclk1(x) \ > + container_of(x, struct s3c24xx_dclk, dclk1_div_change_nb) > + > +static const char dummy_nm[] __initconst = "dummy_name"; What's the advantage of having it defined this way instead of using "dummy_name" (or probably "reserved" or "none", as in Samsung clock drivers) directly in parent lists? > + > +PNAME(dclk_s3c2410_p) = { "pclk", "uclk" }; > +PNAME(clkout0_s3c2410_p) = { "mpll", "upll", "fclk", "hclk", "pclk", > + "gate_dclk0" }; > +PNAME(clkout1_s3c2410_p) = { "mpll", "upll", "fclk", "hclk", "pclk", > + "gate_dclk1" }; > + > +PNAME(clkout0_s3c2412_p) = { "mpll", "upll", dummy_nm /* rtc clock output */, > + "hclk", "pclk", "gate_dclk0" }; Hmm, this would suggest that instead of dummy_nm, a real name should be used here, even if such clock doesn't exist yet. CCF will handle this fine. > +PNAME(clkout1_s3c2412_p) = { "xti", "upll", "fclk", "hclk", "pclk", > + "gate_dclk1" }; > + > +PNAME(clkout0_s3c2440_p) = { "xti", "upll", "fclk", "hclk", "pclk", > + "gate_dclk0" }; > +PNAME(clkout1_s3c2440_p) = { "mpll", "upll", dummy_nm /* rtc clock output */, > + "hclk", "pclk", "gate_dclk1" }; [snip] > +static int s3c24xx_dclk_probe(struct platform_device *pdev) > +{ > + struct s3c24xx_dclk *s3c24xx_dclk; > + struct device_node *np = pdev->dev.of_node; > + struct regmap *misccr = NULL; > + struct resource *mem; > + struct clk **clk_table; > + const char **clkout0_parent_names, **clkout1_parent_names; > + u8 clkout0_num_parents, clkout1_num_parents; > + int current_soc, ret, i; > + > + s3c24xx_dclk = devm_kzalloc(&pdev->dev, sizeof(*s3c24xx_dclk), > + GFP_KERNEL); > + if (!s3c24xx_dclk) > + return -ENOMEM; > + > + s3c24xx_dclk->dev = &pdev->dev; > + platform_set_drvdata(pdev, s3c24xx_dclk); > + spin_lock_init(&s3c24xx_dclk->dclk_lock); > + > + clk_table = devm_kzalloc(&pdev->dev, > + sizeof(struct clk *) * DCLK_MAX_CLKS, > + GFP_KERNEL); > + if (!clk_table) > + return -ENOMEM; > + > + s3c24xx_dclk->clk_data.clks = clk_table; > + s3c24xx_dclk->clk_data.clk_num = DCLK_MAX_CLKS; > + > + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + s3c24xx_dclk->base = devm_ioremap_resource(&pdev->dev, mem); > + if (IS_ERR(s3c24xx_dclk->base)) > + return PTR_ERR(s3c24xx_dclk->base); > + > + /* when run from devicetree, get the misccr through a syscon-regmap */ > + if (np) { > + misccr = syscon_regmap_lookup_by_phandle(np, "samsung,misccr"); > + if (IS_ERR(misccr)) { > + dev_err(&pdev->dev, "could not get misccr syscon, %ld\n", > + PTR_ERR(misccr)); > + return PTR_ERR(misccr); > + } > + } > + > + current_soc = s3c24xx_dclk_get_driver_data(pdev); > + > + if (current_soc == S3C2443) { > + clk_table[MUX_DCLK0] = clk_register_mux(&pdev->dev, > + "mux_dclk0", dclk_s3c2443_p, > + ARRAY_SIZE(dclk_s3c2443_p), 0, > + s3c24xx_dclk->base, 1, 1, 0, > + &s3c24xx_dclk->dclk_lock); > + clk_table[MUX_DCLK1] = clk_register_mux(&pdev->dev, > + "mux_dclk1", dclk_s3c2443_p, > + ARRAY_SIZE(dclk_s3c2443_p), 0, > + s3c24xx_dclk->base, 17, 1, 0, > + &s3c24xx_dclk->dclk_lock); > + } else { > + clk_table[MUX_DCLK0] = clk_register_mux(&pdev->dev, > + "mux_dclk0", dclk_s3c2410_p, > + ARRAY_SIZE(dclk_s3c2410_p), 0, > + s3c24xx_dclk->base, 1, 1, 0, > + &s3c24xx_dclk->dclk_lock); > + clk_table[MUX_DCLK1] = clk_register_mux(&pdev->dev, > + "mux_dclk1", dclk_s3c2410_p, > + ARRAY_SIZE(dclk_s3c2410_p), 0, > + s3c24xx_dclk->base, 17, 1, 0, > + &s3c24xx_dclk->dclk_lock); > + } What about using a variant struct instead? Match tables would simply contain pointers to respective structs and here the code would refer to appropriate fields in a struct returned by s3c24xx_dclk_get_driver_data(). > + > + clk_table[DIV_DCLK0] = clk_register_divider(&pdev->dev, "div_dclk0", > + "mux_dclk0", 0, s3c24xx_dclk->base, > + 4, 4, 0, &s3c24xx_dclk->dclk_lock); > + clk_table[DIV_DCLK1] = clk_register_divider(&pdev->dev, "div_dclk1", > + "mux_dclk1", 0, s3c24xx_dclk->base, > + 20, 4, 0, &s3c24xx_dclk->dclk_lock); > + > + clk_table[GATE_DCLK0] = clk_register_gate(&pdev->dev, "gate_dclk0", > + "div_dclk0", CLK_SET_RATE_PARENT, > + s3c24xx_dclk->base, 0, 0, > + &s3c24xx_dclk->dclk_lock); > + clk_table[GATE_DCLK1] = clk_register_gate(&pdev->dev, "gate_dclk1", > + "div_dclk1", CLK_SET_RATE_PARENT, > + s3c24xx_dclk->base, 16, 0, > + &s3c24xx_dclk->dclk_lock); > + > + switch (current_soc) { > + case S3C2410: > + clkout0_parent_names = clkout0_s3c2410_p; > + clkout0_num_parents = ARRAY_SIZE(clkout0_s3c2410_p); > + clkout1_parent_names = clkout1_s3c2410_p; > + clkout1_num_parents = ARRAY_SIZE(clkout1_s3c2410_p); > + break; > + case S3C2412: > + clkout0_parent_names = clkout0_s3c2412_p; > + clkout0_num_parents = ARRAY_SIZE(clkout0_s3c2412_p); > + clkout1_parent_names = clkout1_s3c2412_p; > + clkout1_num_parents = ARRAY_SIZE(clkout1_s3c2412_p); > + break; > + case S3C2440: > + clkout0_parent_names = clkout0_s3c2440_p; > + clkout0_num_parents = ARRAY_SIZE(clkout0_s3c2440_p); > + clkout1_parent_names = clkout1_s3c2440_p; > + clkout1_num_parents = ARRAY_SIZE(clkout1_s3c2440_p); > + break; > + case S3C2443: > + clkout0_parent_names = clkout0_s3c2443_p; > + clkout0_num_parents = ARRAY_SIZE(clkout0_s3c2443_p); > + clkout1_parent_names = clkout1_s3c2443_p; > + clkout1_num_parents = ARRAY_SIZE(clkout1_s3c2443_p); > + break; > + default: > + dev_err(&pdev->dev, "unsupported soc %d\n", current_soc); > + ret = -EINVAL; > + goto err_clk_register; > + } Ditto. > + > + clk_table[MUX_CLKOUT0] = s3c24xx_register_clkout(&pdev->dev, > + "clkout0", clkout0_parent_names, > + clkout0_num_parents, misccr, 4, 7); > + clk_table[MUX_CLKOUT1] = s3c24xx_register_clkout(&pdev->dev, "clkout1", > + clkout1_parent_names, > + clkout1_num_parents, misccr, 8, 7); > + > + for (i = 0; i < DCLK_MAX_CLKS; i++) > + if (IS_ERR(clk_table[i])) { > + dev_err(&pdev->dev, "clock %d failed to register\n", i); > + ret = PTR_ERR(clk_table[i]); > + goto err_clk_register; > + } > + > + ret = clk_register_clkdev(clk_table[MUX_DCLK0], "dclk0", NULL); > + ret |= clk_register_clkdev(clk_table[MUX_DCLK1], "dclk1", NULL); > + ret |= clk_register_clkdev(clk_table[MUX_CLKOUT0], "clkout0", NULL); > + ret |= clk_register_clkdev(clk_table[MUX_CLKOUT1], "clkout1", NULL); Hmm, won't it break the error value if two calls return different error codes? I guess that if (!ret) ret = ... if (!ret) ret = ... construct would be more appropriate here, if you don't want error message per call. > + if (ret) { > + dev_err(&pdev->dev, "failed to register aliases\n"); > + goto err_clk_register; > + } > + > + s3c24xx_dclk->dclk0_div_change_nb.notifier_call = > + s3c24xx_dclk0_div_notify; > + s3c24xx_dclk->dclk0_div_change_nb.next = NULL; Do you need to set this field to NULL explicitly? Best regards, Tomasz