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 <heiko@xxxxxxxxx>
---
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 <heiko@xxxxxxxxx>
+ *
+ * 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 <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/mfd/syscon.h>
+#include <linux/clk-provider.h>
+#include <linux/regmap.h>
+#include <linux/of.h>
+#include <dt-bindings/clock/samsung,s3c2410-dclk.h>
+#include "clk.h"
+
+/* legacy access to misccr, until dt conversion is finished */
+#include <mach/hardware.h>
+#include <mach/regs-gpio.h>
+
+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
--
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
