diff mbox series

[RFC,v2,1/5] clk: mips: ralink: add Ralink MIPS gating clock driver

Message ID 20190405000129.19331-2-drvlabo@gmail.com (mailing list archive)
State RFC
Headers show
Series MIPS: ralink: peripheral clock gating driver | expand

Commit Message

NOGUCHI Hiroshi April 5, 2019, 12:01 a.m. UTC 
This patch adds clock gating driver for Ralink/Mediatek MIPS Socs.

Signed-off-by: NOGUCHI Hiroshi <drvlabo@gmail.com>
---
 drivers/clk/Kconfig      |   6 ++
 drivers/clk/Makefile     |   1 +
 drivers/clk/clk-rt2880.c | 199 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 206 insertions(+)
 create mode 100644 drivers/clk/clk-rt2880.c

Comments

Stephen Boyd April 25, 2019, 7:27 p.m. UTC | #1 
Quoting NOGUCHI Hiroshi (2019-04-04 17:01:25)
> This patch adds clock gating driver for Ralink/Mediatek MIPS Socs.

Please read Documentation/process/submitting-patches.rst and look at
this part of it in detail:

 Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
 instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
 to do frotz", as if you are giving orders to the codebase to change
 its behaviour.

> 
> Signed-off-by: NOGUCHI Hiroshi <drvlabo@gmail.com>
> ---
>  drivers/clk/Kconfig      |   6 ++
>  drivers/clk/Makefile     |   1 +
>  drivers/clk/clk-rt2880.c | 199 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 206 insertions(+)
>  create mode 100644 drivers/clk/clk-rt2880.c
> 
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index f96c7f39ab7e..dbfdf1bcdc6c 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -290,6 +290,12 @@ config COMMON_CLK_BD718XX
>           This driver supports ROHM BD71837 and ROHM BD71847
>           PMICs clock gates.
>  
> +config COMMON_CLK_RT2880
> +       bool "Clock driver for Mediatek/Ralink MIPS SoC Family"
> +       depends on COMMON_CLK && OF

Does it really depend on OF? Or just depends on it at runtime? If it's a
runtime requirement, perhaps make it just be depends on COMMON_CLK.

> +       help
> +         This driver support Mediatek/Ralink MIPS SoCs' clock gates.
> +
>  config COMMON_CLK_FIXED_MMIO
>         bool "Clock driver for Memory Mapped Fixed values"
> diff --git a/drivers/clk/clk-rt2880.c b/drivers/clk/clk-rt2880.c
> new file mode 100644
> index 000000000000..bcdb4c1486d3
> --- /dev/null
> +++ b/drivers/clk/clk-rt2880.c
> @@ -0,0 +1,199 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 NOGUCHI Hiroshi <drvlabo@gmail.com>
> + */
> +
> +#include <linux/slab.h>
> +#include <linux/clk-provider.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +#include <linux/platform_device.h>

Sort these includes alphabetically?

> +
> +
> +/* clock configuration 1 */

Comment seems useless.

> +#define        SYSC_REG_CLKCFG1        0x30
> +
> +#define GATE_CLK_NUM   32
> +
> +struct rt2880_gate {
> +       struct clk_hw   hw;
> +       u8      shift;
> +};
> +
> +#define        to_rt2880_gate(_hw)     container_of(_hw, struct rt2880_gate, hw)
> +
> +static struct clk_hw_onecell_data *clk_data;
> +static struct regmap *syscon_regmap;

Can these be per some driver instance instead of globals? If they have
to stay global, please make a better name so that they're not so
generic.

> +
> +static int rt2880_gate_enable(struct clk_hw *hw)
> +{
> +       struct rt2880_gate *clk_gate = to_rt2880_gate(hw);
> +       u32 val = BIT(clk_gate->shift);
> +
> +       return regmap_update_bits(syscon_regmap, SYSC_REG_CLKCFG1, val, val);
> +}
> +
> +static void rt2880_gate_disable(struct clk_hw *hw)
> +{
> +       struct rt2880_gate *clk_gate = to_rt2880_gate(hw);
> +       u32 val = BIT(clk_gate->shift);
> +
> +       regmap_update_bits(syscon_regmap, SYSC_REG_CLKCFG1, val, 0);
> +}
> +
> +static int rt2880_gate_is_enabled(struct clk_hw *hw)
> +{
> +       struct rt2880_gate *clk_gate = to_rt2880_gate(hw);
> +       unsigned int rdval;
> +
> +       if (regmap_read(syscon_regmap, SYSC_REG_CLKCFG1, &rdval))
> +               return 0;
> +
> +       return rdval & BIT(clk_gate->shift);
> +}
> +
> +static const struct clk_ops rt2880_gate_ops = {
> +       .enable = rt2880_gate_enable,
> +       .disable = rt2880_gate_disable,
> +       .is_enabled = rt2880_gate_is_enabled,
> +};
> +
> +static struct clk_hw * __init
> +rt2880_register_gate(const char *name, const char *parent_name, u8 shift)
> +{
> +       struct rt2880_gate      *clk_gate;
> +       struct clk_init_data    init;
> +       int err;
> +       const char *_parent_names[1] = { parent_name };

This isn't necessary.

> +
> +       clk_gate = kzalloc(sizeof(*clk_gate), GFP_KERNEL);
> +       if (!clk_gate)
> +               return ERR_PTR(-ENOMEM);
> +
> +       init.name = name;
> +       init.ops = &rt2880_gate_ops;
> +       init.flags = 0;
> +       init.parent_names = parent_name ? _parent_names : NULL;

Just use &parent_name instead.

> +       init.num_parents = parent_name ? 1 : 0;
> +
> +       clk_gate->hw.init = &init;
> +       clk_gate->shift = shift;
> +
> +       err = clk_hw_register(NULL, &clk_gate->hw);

Please pass in the device during registration.

> +       if (err) {
> +               kfree(clk_gate);
> +               return ERR_PTR(err);
> +       }
> +
> +       return &clk_gate->hw;
> +}
> +
> +static struct clk_hw *
> +rt2880_clk_hw_get(struct of_phandle_args *clkspec, void *data)
> +{
> +       struct clk_hw_onecell_data *hw_data = data;
> +       unsigned int idx = clkspec->args[0];
> +       int i;
> +
> +       if (idx >= GATE_CLK_NUM)
> +               goto err;
> +
> +       for (i = 0; i < hw_data->num; i++)
> +               if (idx == to_rt2880_gate(hw_data->hws[i])->shift)
> +                       break;
> +       if (i >= hw_data->num)
> +               goto err;
> +
> +       return hw_data->hws[i];
> +
> +err:
> +       pr_err("%s: invalid index %u\n", __func__, idx);
> +       return ERR_PTR(-EINVAL);
> +}
> +
> +static int __init _rt2880_clkctrl_init_dt(struct device_node *np)

Drop __init, this is called from probe(). Also, please just collapse it
into the driver probe and use platform device APIs throughout this
function.

> +{
> +       struct clk_hw *clk_hw;
> +       const char *name;
> +       const char *parent_name;
> +       u32 val;
> +       int cnt;
> +       int num;
> +       int i;
> +       int idx;
> +
> +       syscon_regmap = syscon_regmap_lookup_by_phandle(np, "ralink,sysctl");
> +       if (IS_ERR(syscon_regmap)) {
> +               pr_err("rt2880-clock: could not get syscon regmap.\n");
> +               return PTR_ERR(syscon_regmap);
> +       }
> +
> +       cnt = of_property_count_u32_elems(np, "clock-indices");
> +       if (cnt < 0) {
> +               pr_err("rt2880-clock: clock-indices property is invalid.\n");
> +               return cnt;
> +       }
> +
> +       num = 0;
> +       for (i = 0; i < cnt; i++) {
> +               if (of_property_read_u32_index(np, "clock-indices", i, &val))

I'm a little lost one what the indices are for? Why is the number space
being folded like this?

> +                       break;
> +               if (val < GATE_CLK_NUM)
> +                       num++;
> +       }
> +       if ((num <= 0) || (num > GATE_CLK_NUM)) {

Please remove useless parenthesis.

> +               pr_err("rt2880-clock: clock-indices property is invalid.\n");

Drop the full-stop on all error messages please.

> +               return -EINVAL;
> +       }
> +
> +       clk_data = kzalloc(struct_size(clk_data, hws, num), GFP_KERNEL);
> +       if (!clk_data)
> +               return -ENOMEM;
> +       clk_data->num = num;
> +
> +       idx = 0;
> +       for (i = 0; (i < cnt) && (idx < num); i++) {

Please drop useless parenthesis.

> +               if (of_property_read_u32_index(np, "clock-indices", i, &val))
> +                       break;
> +               if ((int)val >= GATE_CLK_NUM)

Why are we casting to int?

> +                       continue;
> +
> +               if (of_property_read_string_index(np, "clock-output-names",
> +                               i, &name))
> +                       break;
> +
> +               parent_name = of_clk_get_parent_name(np, i);
> +
> +               clk_hw = rt2880_register_gate(name, parent_name, val);
> +               if (IS_ERR_OR_NULL(clk_hw)) {

When does it return NULL? Should probably just be IS_ERR() check here.


> +                       pr_err("rt2880-clock: could not register clock for %s\n",

Consider dev_err() so that the prefix (rt2880-clock:) can be dropped.

> +                               name);
> +                       continue;
> +               }
> +               clk_data->hws[idx] = clk_hw;
> +               idx++;
> +       }
> +
> +       of_clk_add_hw_provider(np, rt2880_clk_hw_get, clk_data);

Why not return of_clk_add_hw_provider()?

> +
> +       return 0;
> +}
> +
> +static int rt2880_clkctrl_probe(struct platform_device *pdev)
> +{
> +       return _rt2880_clkctrl_init_dt(pdev->dev.of_node);
> +}
> +
> +static const struct of_device_id rt2880_clkctrl_ids[] = {
> +       { .compatible = "ralink,rt2880-clock" },
> +       { }
> +};
> +
> +static struct platform_driver rt2880_clkctrl_driver = {
> +       .probe = rt2880_clkctrl_probe,
> +       .driver = {
> +               .name = "rt2880-clock",
> +               .of_match_table = rt2880_clkctrl_ids,
> +       },
> +};
> +builtin_platform_driver(rt2880_clkctrl_driver);
> -- 
> 2.20.1
>
NOGUCHI Hiroshi May 1, 2019, 10:58 a.m. UTC | #2 
Thanks for suggestions.

>> +{
>> +       struct clk_hw *clk_hw;
>> +       const char *name;
>> +       const char *parent_name;
>> +       u32 val;
>> +       int cnt;
>> +       int num;
>> +       int i;
>> +       int idx;
>> +
>> +       syscon_regmap = syscon_regmap_lookup_by_phandle(np, "ralink,sysctl");
>> +       if (IS_ERR(syscon_regmap)) {
>> +               pr_err("rt2880-clock: could not get syscon regmap.\n");
>> +               return PTR_ERR(syscon_regmap);
>> +       }
>> +
>> +       cnt = of_property_count_u32_elems(np, "clock-indices");
>> +       if (cnt < 0) {
>> +               pr_err("rt2880-clock: clock-indices property is invalid.\n");
>> +               return cnt;
>> +       }
>> +
>> +       num = 0;
>> +       for (i = 0; i < cnt; i++) {
>> +               if (of_property_read_u32_index(np, "clock-indices", i, &val))
> 
> I'm a little lost one what the indices are for? Why is the number space
> being folded like this?

I want to let the clock cell index match  with the bit number in the 
gate control register.
Is my "clock-indices" usage wrong ?
diff mbox series

Patch

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index f96c7f39ab7e..dbfdf1bcdc6c 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -290,6 +290,12 @@  config COMMON_CLK_BD718XX
 	  This driver supports ROHM BD71837 and ROHM BD71847
 	  PMICs clock gates.
 
+config COMMON_CLK_RT2880
+	bool "Clock driver for Mediatek/Ralink MIPS SoC Family"
+	depends on COMMON_CLK && OF
+	help
+	  This driver support Mediatek/Ralink MIPS SoCs' clock gates.
+
 config COMMON_CLK_FIXED_MMIO
 	bool "Clock driver for Memory Mapped Fixed values"
 	depends on COMMON_CLK && OF
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 1db133652f0c..b1c24b99e848 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -43,6 +43,7 @@  obj-$(CONFIG_COMMON_CLK_PALMAS)		+= clk-palmas.o
 obj-$(CONFIG_COMMON_CLK_PWM)		+= clk-pwm.o
 obj-$(CONFIG_CLK_QORIQ)			+= clk-qoriq.o
 obj-$(CONFIG_COMMON_CLK_RK808)		+= clk-rk808.o
+obj-$(CONFIG_COMMON_CLK_RT2880)		+= clk-rt2880.o
 obj-$(CONFIG_COMMON_CLK_HI655X)		+= clk-hi655x.o
 obj-$(CONFIG_COMMON_CLK_S2MPS11)	+= clk-s2mps11.o
 obj-$(CONFIG_COMMON_CLK_SCMI)           += clk-scmi.o
diff --git a/drivers/clk/clk-rt2880.c b/drivers/clk/clk-rt2880.c
new file mode 100644
index 000000000000..bcdb4c1486d3
--- /dev/null
+++ b/drivers/clk/clk-rt2880.c
@@ -0,0 +1,199 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 NOGUCHI Hiroshi <drvlabo@gmail.com>
+ */
+
+#include <linux/slab.h>
+#include <linux/clk-provider.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+#include <linux/platform_device.h>
+
+
+/* clock configuration 1 */
+#define	SYSC_REG_CLKCFG1	0x30
+
+#define GATE_CLK_NUM	32
+
+struct rt2880_gate {
+	struct clk_hw	hw;
+	u8	shift;
+};
+
+#define	to_rt2880_gate(_hw)	container_of(_hw, struct rt2880_gate, hw)
+
+static struct clk_hw_onecell_data *clk_data;
+static struct regmap *syscon_regmap;
+
+static int rt2880_gate_enable(struct clk_hw *hw)
+{
+	struct rt2880_gate *clk_gate = to_rt2880_gate(hw);
+	u32 val = BIT(clk_gate->shift);
+
+	return regmap_update_bits(syscon_regmap, SYSC_REG_CLKCFG1, val, val);
+}
+
+static void rt2880_gate_disable(struct clk_hw *hw)
+{
+	struct rt2880_gate *clk_gate = to_rt2880_gate(hw);
+	u32 val = BIT(clk_gate->shift);
+
+	regmap_update_bits(syscon_regmap, SYSC_REG_CLKCFG1, val, 0);
+}
+
+static int rt2880_gate_is_enabled(struct clk_hw *hw)
+{
+	struct rt2880_gate *clk_gate = to_rt2880_gate(hw);
+	unsigned int rdval;
+
+	if (regmap_read(syscon_regmap, SYSC_REG_CLKCFG1, &rdval))
+		return 0;
+
+	return rdval & BIT(clk_gate->shift);
+}
+
+static const struct clk_ops rt2880_gate_ops = {
+	.enable = rt2880_gate_enable,
+	.disable = rt2880_gate_disable,
+	.is_enabled = rt2880_gate_is_enabled,
+};
+
+static struct clk_hw * __init
+rt2880_register_gate(const char *name, const char *parent_name, u8 shift)
+{
+	struct rt2880_gate	*clk_gate;
+	struct clk_init_data	init;
+	int err;
+	const char *_parent_names[1] = { parent_name };
+
+	clk_gate = kzalloc(sizeof(*clk_gate), GFP_KERNEL);
+	if (!clk_gate)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = name;
+	init.ops = &rt2880_gate_ops;
+	init.flags = 0;
+	init.parent_names = parent_name ? _parent_names : NULL;
+	init.num_parents = parent_name ? 1 : 0;
+
+	clk_gate->hw.init = &init;
+	clk_gate->shift = shift;
+
+	err = clk_hw_register(NULL, &clk_gate->hw);
+	if (err) {
+		kfree(clk_gate);
+		return ERR_PTR(err);
+	}
+
+	return &clk_gate->hw;
+}
+
+static struct clk_hw *
+rt2880_clk_hw_get(struct of_phandle_args *clkspec, void *data)
+{
+	struct clk_hw_onecell_data *hw_data = data;
+	unsigned int idx = clkspec->args[0];
+	int i;
+
+	if (idx >= GATE_CLK_NUM)
+		goto err;
+
+	for (i = 0; i < hw_data->num; i++)
+		if (idx == to_rt2880_gate(hw_data->hws[i])->shift)
+			break;
+	if (i >= hw_data->num)
+		goto err;
+
+	return hw_data->hws[i];
+
+err:
+	pr_err("%s: invalid index %u\n", __func__, idx);
+	return ERR_PTR(-EINVAL);
+}
+
+static int __init _rt2880_clkctrl_init_dt(struct device_node *np)
+{
+	struct clk_hw *clk_hw;
+	const char *name;
+	const char *parent_name;
+	u32 val;
+	int cnt;
+	int num;
+	int i;
+	int idx;
+
+	syscon_regmap = syscon_regmap_lookup_by_phandle(np, "ralink,sysctl");
+	if (IS_ERR(syscon_regmap)) {
+		pr_err("rt2880-clock: could not get syscon regmap.\n");
+		return PTR_ERR(syscon_regmap);
+	}
+
+	cnt = of_property_count_u32_elems(np, "clock-indices");
+	if (cnt < 0) {
+		pr_err("rt2880-clock: clock-indices property is invalid.\n");
+		return cnt;
+	}
+
+	num = 0;
+	for (i = 0; i < cnt; i++) {
+		if (of_property_read_u32_index(np, "clock-indices", i, &val))
+			break;
+		if (val < GATE_CLK_NUM)
+			num++;
+	}
+	if ((num <= 0) || (num > GATE_CLK_NUM)) {
+		pr_err("rt2880-clock: clock-indices property is invalid.\n");
+		return -EINVAL;
+	}
+
+	clk_data = kzalloc(struct_size(clk_data, hws, num), GFP_KERNEL);
+	if (!clk_data)
+		return -ENOMEM;
+	clk_data->num = num;
+
+	idx = 0;
+	for (i = 0; (i < cnt) && (idx < num); i++) {
+		if (of_property_read_u32_index(np, "clock-indices", i, &val))
+			break;
+		if ((int)val >= GATE_CLK_NUM)
+			continue;
+
+		if (of_property_read_string_index(np, "clock-output-names",
+				i, &name))
+			break;
+
+		parent_name = of_clk_get_parent_name(np, i);
+
+		clk_hw = rt2880_register_gate(name, parent_name, val);
+		if (IS_ERR_OR_NULL(clk_hw)) {
+			pr_err("rt2880-clock: could not register clock for %s\n",
+				name);
+			continue;
+		}
+		clk_data->hws[idx] = clk_hw;
+		idx++;
+	}
+
+	of_clk_add_hw_provider(np, rt2880_clk_hw_get, clk_data);
+
+	return 0;
+}
+
+static int rt2880_clkctrl_probe(struct platform_device *pdev)
+{
+	return _rt2880_clkctrl_init_dt(pdev->dev.of_node);
+}
+
+static const struct of_device_id rt2880_clkctrl_ids[] = {
+	{ .compatible = "ralink,rt2880-clock" },
+	{ }
+};
+
+static struct platform_driver rt2880_clkctrl_driver = {
+	.probe = rt2880_clkctrl_probe,
+	.driver = {
+		.name = "rt2880-clock",
+		.of_match_table = rt2880_clkctrl_ids,
+	},
+};
+builtin_platform_driver(rt2880_clkctrl_driver);