[PATCH 03/12] clk: samsung: add clock driver for external clock outputs
Tomasz Figa
tomasz.figa at gmail.com
Sat Feb 8 21:25:21 EST 2014
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 at sntech.de>
> ---
> 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 at sntech.de>
> + *
> + * 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
More information about the linux-arm-kernel
mailing list