[PATCH 2/4] clk: add support for clocks provided by SCP(System Control Processor)

[PATCH 2/4] clk: add support for clocks provided by SCP(System Control Processor)

[Sudeep Holla]

On some ARM based systems, a separate Cortex-M based System Control
Processor(SCP) provides the overall power, clock, reset and system
control. System Control and Power Interface(SCPI) Message Protocol
is defined for the communication between the Application Cores(AP)
and the SCP.

This patch adds support for the clocks provided by SCP using SCPI
protocol.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
Cc: Mike Turquette <mturquette@linaro.org>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Liviu Dudau <Liviu.Dudau@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Jon Medhurst (Tixy) <tixy@linaro.org>
Cc: linux-clk@vger.kernel.org
---
 drivers/clk/Kconfig    |  10 ++
 drivers/clk/Makefile   |   1 +
 drivers/clk/clk-scpi.c | 344 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 355 insertions(+)
 create mode 100644 drivers/clk/clk-scpi.c

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 9897f353bf1a..906bf7dd72a2 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -59,6 +59,16 @@ config COMMON_CLK_RK808
 	  clocked at 32KHz each. Clkout1 is always on, Clkout2 can off
 	  by control register.
 
+config COMMON_CLK_SCPI
+        tristate "Clock driver controlled via SCPI interface"
+        depends on ARM_SCPI_PROTOCOL
+        ---help---
+          This driver provides support for clocks that are controlled
+          by firmware that implements the SCPI interface.
+
+	  This driver uses SCPI Message Protocol to interact with the
+	  firware providing all the clock controls.
+
 config COMMON_CLK_SI5351
 	tristate "Clock driver for SiLabs 5351A/B/C"
 	depends on I2C
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 3d00c25382c5..442ab6ebd5b1 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -36,6 +36,7 @@ obj-$(CONFIG_COMMON_CLK_PALMAS)		+= clk-palmas.o
 obj-$(CONFIG_CLK_QORIQ)			+= clk-qoriq.o
 obj-$(CONFIG_COMMON_CLK_RK808)		+= clk-rk808.o
 obj-$(CONFIG_COMMON_CLK_S2MPS11)	+= clk-s2mps11.o
+obj-$(CONFIG_COMMON_CLK_SCPI)           += clk-scpi.o
 obj-$(CONFIG_COMMON_CLK_SI5351)		+= clk-si5351.o
 obj-$(CONFIG_COMMON_CLK_SI570)		+= clk-si570.o
 obj-$(CONFIG_CLK_TWL6040)		+= clk-twl6040.o
diff --git a/drivers/clk/clk-scpi.c b/drivers/clk/clk-scpi.c
new file mode 100644
index 000000000000..70a2d57da32d
--- /dev/null
+++ b/drivers/clk/clk-scpi.c
@@ -0,0 +1,344 @@
+/*
+ * System Control and Power Interface (SCPI) Protocol based clock driver
+ *
+ * Copyright (C) 2015 ARM Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/clkdev.h>
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/of.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/scpi_protocol.h>
+
+struct scpi_clk {
+	u32 id;
+	const char *name;
+	struct clk_hw hw;
+	struct scpi_dvfs_info *info;
+	unsigned long rate_min;
+	unsigned long rate_max;
+};
+
+#define to_scpi_clk(clk) container_of(clk, struct scpi_clk, hw)
+
+static struct scpi_ops *scpi_ops;
+
+static unsigned long scpi_clk_recalc_rate(struct clk_hw *hw,
+					  unsigned long parent_rate)
+{
+	struct scpi_clk *clk = to_scpi_clk(hw);
+
+	return scpi_ops->clk_get_val(clk->id);
+}
+
+static long scpi_clk_round_rate(struct clk_hw *hw, unsigned long rate,
+				unsigned long *parent_rate)
+{
+	struct scpi_clk *clk = to_scpi_clk(hw);
+
+	if (WARN_ON(clk->rate_min && rate < clk->rate_min))
+		rate = clk->rate_min;
+	if (WARN_ON(clk->rate_max && rate > clk->rate_max))
+		rate = clk->rate_max;
+
+	return rate;
+}
+
+static int scpi_clk_set_rate(struct clk_hw *hw, unsigned long rate,
+			     unsigned long parent_rate)
+{
+	struct scpi_clk *clk = to_scpi_clk(hw);
+
+	return scpi_ops->clk_set_val(clk->id, rate);
+}
+
+static struct clk_ops scpi_clk_ops = {
+	.recalc_rate = scpi_clk_recalc_rate,
+	.round_rate = scpi_clk_round_rate,
+	.set_rate = scpi_clk_set_rate,
+};
+
+/* find closest match to given frequency in OPP table */
+static int __scpi_dvfs_round_rate(struct scpi_clk *clk, unsigned long rate)
+{
+	int idx;
+	u32 fmin = 0, fmax = ~0, ftmp;
+	struct scpi_opp *opp = clk->info->opps;
+
+	for (idx = 0; idx < clk->info->count; idx++, opp++) {
+		ftmp = opp->freq;
+		if (ftmp >= (u32)rate) {
+			if (ftmp <= fmax)
+				fmax = ftmp;
+		} else {
+			if (ftmp >= fmin)
+				fmin = ftmp;
+		}
+	}
+	if (fmax != ~0)
+		return fmax;
+	return fmin;
+}
+
+static unsigned long scpi_dvfs_recalc_rate(struct clk_hw *hw,
+					   unsigned long parent_rate)
+{
+	struct scpi_clk *clk = to_scpi_clk(hw);
+	int idx = scpi_ops->dvfs_get_idx(clk->id);
+	struct scpi_opp *opp;
+
+	if (idx < 0)
+		return 0;
+
+	opp = clk->info->opps + idx;
+	return opp->freq;
+}
+
+static long scpi_dvfs_round_rate(struct clk_hw *hw, unsigned long rate,
+				 unsigned long *parent_rate)
+{
+	struct scpi_clk *clk = to_scpi_clk(hw);
+
+	return __scpi_dvfs_round_rate(clk, rate);
+}
+
+static int __scpi_find_dvfs_index(struct scpi_clk *clk, unsigned long rate)
+{
+	int idx, max_opp = clk->info->count;
+	struct scpi_opp *opp = clk->info->opps;
+
+	for (idx = 0; idx < max_opp; idx++, opp++)
+		if (opp->freq == rate)
+			break;
+	return (idx == max_opp) ? -EINVAL : idx;
+}
+
+static int scpi_dvfs_set_rate(struct clk_hw *hw, unsigned long rate,
+			      unsigned long parent_rate)
+{
+	struct scpi_clk *clk = to_scpi_clk(hw);
+	int ret = __scpi_find_dvfs_index(clk, rate);
+
+	if (ret < 0)
+		return ret;
+	return scpi_ops->dvfs_set_idx(clk->id, (u8)ret);
+}
+
+static struct clk_ops scpi_dvfs_ops = {
+	.recalc_rate = scpi_dvfs_recalc_rate,
+	.round_rate = scpi_dvfs_round_rate,
+	.set_rate = scpi_dvfs_set_rate,
+};
+
+static struct clk *
+scpi_dvfs_ops_init(struct device *dev, struct device_node *np,
+		   struct scpi_clk *sclk)
+{
+	struct clk_init_data init;
+	struct scpi_dvfs_info *info;
+
+	init.name = sclk->name;
+	init.flags = CLK_IS_ROOT;
+	init.num_parents = 0;
+	init.ops = &scpi_dvfs_ops;
+	sclk->hw.init = &init;
+
+	info = scpi_ops->dvfs_get_info(sclk->id);
+	if (IS_ERR(info))
+		return (struct clk *)info;
+
+	sclk->info = info;
+
+	return devm_clk_register(dev, &sclk->hw);
+}
+
+static struct clk *
+scpi_clk_ops_init(struct device *dev, struct device_node *np,
+		  struct scpi_clk *sclk)
+{
+	struct clk_init_data init;
+	int ret;
+
+	init.name = sclk->name;
+	init.flags = CLK_IS_ROOT;
+	init.num_parents = 0;
+	init.ops = &scpi_clk_ops;
+	sclk->hw.init = &init;
+
+	ret = scpi_ops->clk_get_range(sclk->id, &sclk->rate_min,
+				      &sclk->rate_max);
+	if (!sclk->rate_max)
+		ret = -EINVAL;
+	if (ret)
+		return ERR_PTR(ret);
+
+	return devm_clk_register(dev, &sclk->hw);
+}
+
+static const struct of_device_id scpi_clk_match[] = {
+	{ .compatible = "arm,scpi-dvfs", .data = scpi_dvfs_ops_init, },
+	{ .compatible = "arm,scpi-clk", .data = scpi_clk_ops_init, },
+	{}
+};
+
+static int scpi_clk_probe(struct platform_device *pdev)
+{
+	struct clk **clks;
+	int idx, count;
+	struct clk_onecell_data *clk_data;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct clk *(*clk_ops_init)(struct device *, struct device_node *,
+				    struct scpi_clk *);
+
+	if (!of_device_is_available(np))
+		return -ENODEV;
+
+	clk_ops_init = of_match_node(scpi_clk_match, np)->data;
+	if (!clk_ops_init)
+		return -ENODEV;
+
+	count = of_property_count_strings(np, "clock-output-names");
+	if (count < 0) {
+		dev_err(dev, "%s: invalid clock output count\n", np->name);
+		return -EINVAL;
+	}
+
+	clk_data = devm_kmalloc(dev, sizeof(*clk_data), GFP_KERNEL);
+	if (!clk_data) {
+		dev_err(dev, "failed to allocate clock provider data\n");
+		return -ENOMEM;
+	}
+
+	clks = devm_kmalloc(dev, count * sizeof(*clks), GFP_KERNEL);
+	if (!clks) {
+		dev_err(dev, "failed to allocate clock providers\n");
+		return -ENOMEM;
+	}
+
+	for (idx = 0; idx < count; idx++) {
+		struct scpi_clk *sclk;
+		u32 val;
+
+		sclk = devm_kzalloc(dev, sizeof(*sclk), GFP_KERNEL);
+		if (!sclk) {
+			dev_err(dev, "failed to allocate scpi clocks\n");
+			return -ENOMEM;
+		}
+
+		if (of_property_read_string_index(np, "clock-output-names",
+						  idx, &sclk->name)) {
+			dev_err(dev, "invalid clock name @ %s\n", np->name);
+			return -EINVAL;
+		}
+
+		if (of_property_read_u32_index(np, "clock-indices",
+					       idx, &val)) {
+			dev_err(dev, "invalid clock index @ %s\n", np->name);
+			return -EINVAL;
+		}
+
+		sclk->id = val;
+
+		clks[idx] = clk_ops_init(dev, np, sclk);
+		if (IS_ERR(clks[idx])) {
+			dev_err(dev, "failed to register clock '%s'\n",
+				sclk->name);
+		}
+
+		dev_dbg(dev, "Registered clock '%s'\n", sclk->name);
+	}
+
+	clk_data->clks = clks;
+	clk_data->clk_num = idx;
+	of_clk_add_provider(np, of_clk_src_onecell_get, clk_data);
+
+	platform_set_drvdata(pdev, clk_data);
+
+	return 0;
+}
+
+static int scpi_clk_remove(struct platform_device *pdev)
+{
+	of_clk_del_provider(pdev->dev.of_node);
+	platform_set_drvdata(pdev, NULL);
+	return 0;
+}
+
+static struct platform_driver scpi_clk_driver = {
+	.driver	= {
+		.name = "scpi_clk",
+		.of_match_table = scpi_clk_match,
+	},
+	.probe = scpi_clk_probe,
+	.remove = scpi_clk_remove,
+};
+
+static int scpi_clocks_probe(struct platform_device *pdev)
+{
+	scpi_ops = get_scpi_ops();
+	if (!scpi_ops)
+		return -ENXIO;
+
+	return of_platform_populate(pdev->dev.of_node, scpi_clk_match,
+				    NULL, &pdev->dev);
+}
+
+static int scpi_clocks_remove(struct platform_device *pdev)
+{
+	of_platform_depopulate(&pdev->dev);
+	scpi_ops = NULL;
+	return 0;
+}
+
+static const struct of_device_id scpi_clocks_ids[] = {
+	{ .compatible = "arm,scpi-clocks", },
+	{}
+};
+
+static struct platform_driver scpi_clocks_driver = {
+	.driver	= {
+		.name = "scpi_clocks",
+		.of_match_table = scpi_clocks_ids,
+	},
+	.probe = scpi_clocks_probe,
+	.remove = scpi_clocks_remove,
+};
+
+static int __init scpi_clocks_init(void)
+{
+	int ret;
+
+	ret = platform_driver_register(&scpi_clk_driver);
+	if (ret)
+		return ret;
+
+	return platform_driver_register(&scpi_clocks_driver);
+}
+module_init(scpi_clocks_init);
+
+static void __exit scpi_clocks_exit(void)
+{
+	platform_driver_unregister(&scpi_clk_driver);
+	platform_driver_unregister(&scpi_clocks_driver);
+}
+module_exit(scpi_clocks_exit);
+
+MODULE_AUTHOR("Sudeep Holla <sudeep.holla@arm.com>");
+MODULE_DESCRIPTION("ARM SCPI clock driver");
+MODULE_LICENSE("GPL");
-- 
1.9.1

Re: [PATCH 2/4] clk: add support for clocks provided by SCP(System Control Processor)

[Lorenzo Pieralisi]

On Mon, Apr 27, 2015 at 12:40:44PM +0100, Sudeep Holla wrote:
> On some ARM based systems, a separate Cortex-M based System Control
> Processor(SCP) provides the overall power, clock, reset and system
> control. System Control and Power Interface(SCPI) Message Protocol
> is defined for the communication between the Application Cores(AP)
> and the SCP.
> 
> This patch adds support for the clocks provided by SCP using SCPI
> protocol.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Mike Turquette <mturquette@linaro.org>
> Cc: Stephen Boyd <sboyd@codeaurora.org>
> Cc: Liviu Dudau <Liviu.Dudau@arm.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Jon Medhurst (Tixy) <tixy@linaro.org>
> Cc: linux-clk@vger.kernel.org
> ---
>  drivers/clk/Kconfig    |  10 ++
>  drivers/clk/Makefile   |   1 +
>  drivers/clk/clk-scpi.c | 344 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 355 insertions(+)
>  create mode 100644 drivers/clk/clk-scpi.c
> 
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 9897f353bf1a..906bf7dd72a2 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -59,6 +59,16 @@ config COMMON_CLK_RK808
>  	  clocked at 32KHz each. Clkout1 is always on, Clkout2 can off
>  	  by control register.
>  
> +config COMMON_CLK_SCPI
> +        tristate "Clock driver controlled via SCPI interface"
> +        depends on ARM_SCPI_PROTOCOL
> +        ---help---
> +          This driver provides support for clocks that are controlled
> +          by firmware that implements the SCPI interface.
> +
> +	  This driver uses SCPI Message Protocol to interact with the
> +	  firware providing all the clock controls.

/s/firware/firmware

> +
>  config COMMON_CLK_SI5351
>  	tristate "Clock driver for SiLabs 5351A/B/C"
>  	depends on I2C
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 3d00c25382c5..442ab6ebd5b1 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -36,6 +36,7 @@ obj-$(CONFIG_COMMON_CLK_PALMAS)		+= clk-palmas.o
>  obj-$(CONFIG_CLK_QORIQ)			+= clk-qoriq.o
>  obj-$(CONFIG_COMMON_CLK_RK808)		+= clk-rk808.o
>  obj-$(CONFIG_COMMON_CLK_S2MPS11)	+= clk-s2mps11.o
> +obj-$(CONFIG_COMMON_CLK_SCPI)           += clk-scpi.o
>  obj-$(CONFIG_COMMON_CLK_SI5351)		+= clk-si5351.o
>  obj-$(CONFIG_COMMON_CLK_SI570)		+= clk-si570.o
>  obj-$(CONFIG_CLK_TWL6040)		+= clk-twl6040.o
> diff --git a/drivers/clk/clk-scpi.c b/drivers/clk/clk-scpi.c
> new file mode 100644
> index 000000000000..70a2d57da32d
> --- /dev/null
> +++ b/drivers/clk/clk-scpi.c
> @@ -0,0 +1,344 @@
> +/*
> + * System Control and Power Interface (SCPI) Protocol based clock driver
> + *
> + * Copyright (C) 2015 ARM Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/clkdev.h>
> +#include <linux/clk-provider.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/scpi_protocol.h>
> +
> +struct scpi_clk {
> +	u32 id;
> +	const char *name;
> +	struct clk_hw hw;
> +	struct scpi_dvfs_info *info;
> +	unsigned long rate_min;
> +	unsigned long rate_max;
> +};
> +
> +#define to_scpi_clk(clk) container_of(clk, struct scpi_clk, hw)
> +
> +static struct scpi_ops *scpi_ops;
> +
> +static unsigned long scpi_clk_recalc_rate(struct clk_hw *hw,
> +					  unsigned long parent_rate)
> +{
> +	struct scpi_clk *clk = to_scpi_clk(hw);
> +
> +	return scpi_ops->clk_get_val(clk->id);
> +}
> +
> +static long scpi_clk_round_rate(struct clk_hw *hw, unsigned long rate,
> +				unsigned long *parent_rate)
> +{
> +	struct scpi_clk *clk = to_scpi_clk(hw);
> +
> +	if (WARN_ON(clk->rate_min && rate < clk->rate_min))
> +		rate = clk->rate_min;
> +	if (WARN_ON(clk->rate_max && rate > clk->rate_max))
> +		rate = clk->rate_max;
> +
> +	return rate;
> +}
> +
> +static int scpi_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> +			     unsigned long parent_rate)
> +{
> +	struct scpi_clk *clk = to_scpi_clk(hw);
> +
> +	return scpi_ops->clk_set_val(clk->id, rate);
> +}
> +
> +static struct clk_ops scpi_clk_ops = {
> +	.recalc_rate = scpi_clk_recalc_rate,
> +	.round_rate = scpi_clk_round_rate,
> +	.set_rate = scpi_clk_set_rate,
> +};
> +
> +/* find closest match to given frequency in OPP table */
> +static int __scpi_dvfs_round_rate(struct scpi_clk *clk, unsigned long rate)
> +{
> +	int idx;
> +	u32 fmin = 0, fmax = ~0, ftmp;
> +	struct scpi_opp *opp = clk->info->opps;
> +
> +	for (idx = 0; idx < clk->info->count; idx++, opp++) {
> +		ftmp = opp->freq;
> +		if (ftmp >= (u32)rate) {
> +			if (ftmp <= fmax)
> +				fmax = ftmp;
> +		} else {
> +			if (ftmp >= fmin)
> +				fmin = ftmp;
> +		}
> +	}

Is the opp list sorted ? Can't you break instead of going through
the whole opp set ?

> +	if (fmax != ~0)
> +		return fmax;
> +	return fmin;

return fmax != ~0 ? fmax : fmin;

> +}
> +
> +static unsigned long scpi_dvfs_recalc_rate(struct clk_hw *hw,
> +					   unsigned long parent_rate)
> +{
> +	struct scpi_clk *clk = to_scpi_clk(hw);
> +	int idx = scpi_ops->dvfs_get_idx(clk->id);
> +	struct scpi_opp *opp;
> +
> +	if (idx < 0)
> +		return 0;
> +
> +	opp = clk->info->opps + idx;
> +	return opp->freq;
> +}
> +
> +static long scpi_dvfs_round_rate(struct clk_hw *hw, unsigned long rate,
> +				 unsigned long *parent_rate)
> +{
> +	struct scpi_clk *clk = to_scpi_clk(hw);
> +
> +	return __scpi_dvfs_round_rate(clk, rate);
> +}
> +
> +static int __scpi_find_dvfs_index(struct scpi_clk *clk, unsigned long rate)
> +{
> +	int idx, max_opp = clk->info->count;
> +	struct scpi_opp *opp = clk->info->opps;
> +
> +	for (idx = 0; idx < max_opp; idx++, opp++)
> +		if (opp->freq == rate)
> +			break;

return idx;

> +	return (idx == max_opp) ? -EINVAL : idx;

return -EINVAL;

> +}
> +
> +static int scpi_dvfs_set_rate(struct clk_hw *hw, unsigned long rate,
> +			      unsigned long parent_rate)
> +{
> +	struct scpi_clk *clk = to_scpi_clk(hw);
> +	int ret = __scpi_find_dvfs_index(clk, rate);
> +
> +	if (ret < 0)
> +		return ret;
> +	return scpi_ops->dvfs_set_idx(clk->id, (u8)ret);
> +}
> +
> +static struct clk_ops scpi_dvfs_ops = {
> +	.recalc_rate = scpi_dvfs_recalc_rate,
> +	.round_rate = scpi_dvfs_round_rate,
> +	.set_rate = scpi_dvfs_set_rate,
> +};
> +
> +static struct clk *
> +scpi_dvfs_ops_init(struct device *dev, struct device_node *np,
> +		   struct scpi_clk *sclk)
> +{
> +	struct clk_init_data init;
> +	struct scpi_dvfs_info *info;
> +
> +	init.name = sclk->name;
> +	init.flags = CLK_IS_ROOT;
> +	init.num_parents = 0;
> +	init.ops = &scpi_dvfs_ops;
> +	sclk->hw.init = &init;
> +
> +	info = scpi_ops->dvfs_get_info(sclk->id);
> +	if (IS_ERR(info))
> +		return (struct clk *)info;

Can't you just return NULL ?

> +
> +	sclk->info = info;
> +
> +	return devm_clk_register(dev, &sclk->hw);
> +}
> +
> +static struct clk *
> +scpi_clk_ops_init(struct device *dev, struct device_node *np,
> +		  struct scpi_clk *sclk)
> +{
> +	struct clk_init_data init;
> +	int ret;
> +
> +	init.name = sclk->name;
> +	init.flags = CLK_IS_ROOT;
> +	init.num_parents = 0;
> +	init.ops = &scpi_clk_ops;
> +	sclk->hw.init = &init;
> +
> +	ret = scpi_ops->clk_get_range(sclk->id, &sclk->rate_min,
> +				      &sclk->rate_max);
> +	if (!sclk->rate_max)
> +		ret = -EINVAL;
> +	if (ret)
> +		return ERR_PTR(ret);

Ditto, this ERR_PTR song and dance is not nice at all.

> +
> +	return devm_clk_register(dev, &sclk->hw);
> +}

These two functions are identical apart from the ops, can't we have
just one function and differentiate behavior according to the matching
compatible string ?

> +static const struct of_device_id scpi_clk_match[] = {
> +	{ .compatible = "arm,scpi-dvfs", .data = scpi_dvfs_ops_init, },
> +	{ .compatible = "arm,scpi-clk", .data = scpi_clk_ops_init, },
> +	{}
> +};
> +
> +static int scpi_clk_probe(struct platform_device *pdev)
> +{
> +	struct clk **clks;
> +	int idx, count;
> +	struct clk_onecell_data *clk_data;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct clk *(*clk_ops_init)(struct device *, struct device_node *,
> +				    struct scpi_clk *);
> +
> +	if (!of_device_is_available(np))
> +		return -ENODEV;
> +
> +	clk_ops_init = of_match_node(scpi_clk_match, np)->data;
> +	if (!clk_ops_init)
> +		return -ENODEV;
> +
> +	count = of_property_count_strings(np, "clock-output-names");
> +	if (count < 0) {
> +		dev_err(dev, "%s: invalid clock output count\n", np->name);
> +		return -EINVAL;
> +	}
> +
> +	clk_data = devm_kmalloc(dev, sizeof(*clk_data), GFP_KERNEL);
> +	if (!clk_data) {
> +		dev_err(dev, "failed to allocate clock provider data\n");
> +		return -ENOMEM;
> +	}
> +
> +	clks = devm_kmalloc(dev, count * sizeof(*clks), GFP_KERNEL);
> +	if (!clks) {
> +		dev_err(dev, "failed to allocate clock providers\n");
> +		return -ENOMEM;
> +	}
> +
> +	for (idx = 0; idx < count; idx++) {
> +		struct scpi_clk *sclk;
> +		u32 val;
> +
> +		sclk = devm_kzalloc(dev, sizeof(*sclk), GFP_KERNEL);
> +		if (!sclk) {
> +			dev_err(dev, "failed to allocate scpi clocks\n");
> +			return -ENOMEM;
> +		}
> +
> +		if (of_property_read_string_index(np, "clock-output-names",
> +						  idx, &sclk->name)) {
> +			dev_err(dev, "invalid clock name @ %s\n", np->name);
> +			return -EINVAL;
> +		}
> +
> +		if (of_property_read_u32_index(np, "clock-indices",
> +					       idx, &val)) {
> +			dev_err(dev, "invalid clock index @ %s\n", np->name);
> +			return -EINVAL;
> +		}
> +
> +		sclk->id = val;
> +
> +		clks[idx] = clk_ops_init(dev, np, sclk);
> +		if (IS_ERR(clks[idx])) {

See above.

> +			dev_err(dev, "failed to register clock '%s'\n",
> +				sclk->name);
> +		}
> +
> +		dev_dbg(dev, "Registered clock '%s'\n", sclk->name);
> +	}
> +
> +	clk_data->clks = clks;
> +	clk_data->clk_num = idx;
> +	of_clk_add_provider(np, of_clk_src_onecell_get, clk_data);
> +
> +	platform_set_drvdata(pdev, clk_data);

Where is this used ?

> +
> +	return 0;
> +}
> +
> +static int scpi_clk_remove(struct platform_device *pdev)
> +{
> +	of_clk_del_provider(pdev->dev.of_node);
> +	platform_set_drvdata(pdev, NULL);
> +	return 0;
> +}
> +
> +static struct platform_driver scpi_clk_driver = {
> +	.driver	= {
> +		.name = "scpi_clk",
> +		.of_match_table = scpi_clk_match,
> +	},
> +	.probe = scpi_clk_probe,
> +	.remove = scpi_clk_remove,
> +};
> +
> +static int scpi_clocks_probe(struct platform_device *pdev)
> +{
> +	scpi_ops = get_scpi_ops();
> +	if (!scpi_ops)
> +		return -ENXIO;
> +
> +	return of_platform_populate(pdev->dev.of_node, scpi_clk_match,
> +				    NULL, &pdev->dev);

Do you really need to create a platform_device per sub-node ?

> +}
> +
> +static int scpi_clocks_remove(struct platform_device *pdev)
> +{
> +	of_platform_depopulate(&pdev->dev);
> +	scpi_ops = NULL;
> +	return 0;
> +}
> +
> +static const struct of_device_id scpi_clocks_ids[] = {
> +	{ .compatible = "arm,scpi-clocks", },
> +	{}
> +};
> +
> +static struct platform_driver scpi_clocks_driver = {
> +	.driver	= {
> +		.name = "scpi_clocks",
> +		.of_match_table = scpi_clocks_ids,
> +	},
> +	.probe = scpi_clocks_probe,
> +	.remove = scpi_clocks_remove,
> +};

Why do you need two drivers for the clocks ? Isn't the parent device
tree node and respective platform device enough to initialize all
the subnodes clocks ?

> +static int __init scpi_clocks_init(void)
> +{
> +	int ret;
> +
> +	ret = platform_driver_register(&scpi_clk_driver);
> +	if (ret)
> +		return ret;
> +
> +	return platform_driver_register(&scpi_clocks_driver);

See above.

Thanks,
Lorenzo

> +}
> +module_init(scpi_clocks_init);
> +
> +static void __exit scpi_clocks_exit(void)
> +{
> +	platform_driver_unregister(&scpi_clk_driver);
> +	platform_driver_unregister(&scpi_clocks_driver);
> +}
> +module_exit(scpi_clocks_exit);
> +
> +MODULE_AUTHOR("Sudeep Holla <sudeep.holla@arm.com>");
> +MODULE_DESCRIPTION("ARM SCPI clock driver");
> +MODULE_LICENSE("GPL");
> -- 
> 1.9.1
> 

Re: [PATCH 2/4] clk: add support for clocks provided by SCP(System Control Processor)

[Stephen Boyd]

On 04/27, Sudeep Holla wrote:
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 9897f353bf1a..906bf7dd72a2 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -59,6 +59,16 @@ config COMMON_CLK_RK808
>  	  clocked at 32KHz each. Clkout1 is always on, Clkout2 can off
>  	  by control register.
>  
> +config COMMON_CLK_SCPI
> +        tristate "Clock driver controlled via SCPI interface"
> +        depends on ARM_SCPI_PROTOCOL

|| COMPILE_TEST ?

> +        ---help---
> +          This driver provides support for clocks that are controlled
> +          by firmware that implements the SCPI interface.
> +
> +	  This driver uses SCPI Message Protocol to interact with the
> +	  firware providing all the clock controls.
> +
>  config COMMON_CLK_SI5351
>  	tristate "Clock driver for SiLabs 5351A/B/C"
>  	depends on I2C
> diff --git a/drivers/clk/clk-scpi.c b/drivers/clk/clk-scpi.c
> new file mode 100644
> index 000000000000..70a2d57da32d
> --- /dev/null
> +++ b/drivers/clk/clk-scpi.c
> @@ -0,0 +1,344 @@
> +#include <linux/clkdev.h>

Is clkdev used here?

> +#include <linux/clk-provider.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/scpi_protocol.h>
> +
[...]
> +
> +static int scpi_dvfs_set_rate(struct clk_hw *hw, unsigned long rate,
> +			      unsigned long parent_rate)
> +{
> +	struct scpi_clk *clk = to_scpi_clk(hw);
> +	int ret = __scpi_find_dvfs_index(clk, rate);
> +
> +	if (ret < 0)
> +		return ret;
> +	return scpi_ops->dvfs_set_idx(clk->id, (u8)ret);
> +}
> +
> +static struct clk_ops scpi_dvfs_ops = {

const?

> +	.recalc_rate = scpi_dvfs_recalc_rate,
> +	.round_rate = scpi_dvfs_round_rate,
> +	.set_rate = scpi_dvfs_set_rate,
> +};
> +
> +static struct clk *
> +scpi_dvfs_ops_init(struct device *dev, struct device_node *np,
> +		   struct scpi_clk *sclk)
> +{
> +	struct clk_init_data init;
> +	struct scpi_dvfs_info *info;
> +
> +	init.name = sclk->name;
> +	init.flags = CLK_IS_ROOT;
> +	init.num_parents = 0;
> +	init.ops = &scpi_dvfs_ops;
> +	sclk->hw.init = &init;
> +
> +	info = scpi_ops->dvfs_get_info(sclk->id);
> +	if (IS_ERR(info))
> +		return (struct clk *)info;

return ERR_CAST(info)?

> +
> +	sclk->info = info;
> +
> +	return devm_clk_register(dev, &sclk->hw);
> +}
> +
> +
[...]
> +static int scpi_clk_probe(struct platform_device *pdev)
> +{
> +	struct clk **clks;
> +	int idx, count;
> +	struct clk_onecell_data *clk_data;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct clk *(*clk_ops_init)(struct device *, struct device_node *,
> +				    struct scpi_clk *);
> +
> +	if (!of_device_is_available(np))
> +		return -ENODEV;

Is this because of_platform_populate() populates disabled nodes?

> +
> +	clk_ops_init = of_match_node(scpi_clk_match, np)->data;
> +	if (!clk_ops_init)
> +		return -ENODEV;
> +
> +	count = of_property_count_strings(np, "clock-output-names");
> +	if (count < 0) {
> +		dev_err(dev, "%s: invalid clock output count\n", np->name);
> +		return -EINVAL;
> +	}
> +
> +	clk_data = devm_kmalloc(dev, sizeof(*clk_data), GFP_KERNEL);
> +	if (!clk_data) {
> +		dev_err(dev, "failed to allocate clock provider data\n");
> +		return -ENOMEM;
> +	}
> +
> +	clks = devm_kmalloc(dev, count * sizeof(*clks), GFP_KERNEL);

devm_kcalloc()?

> +	if (!clks) {
> +		dev_err(dev, "failed to allocate clock providers\n");

Error messages on allocation failures are useless. Please remove.

> +		return -ENOMEM;
> +	}
> +
> +	for (idx = 0; idx < count; idx++) {
> +		struct scpi_clk *sclk;
[...]
> +static int scpi_clk_remove(struct platform_device *pdev)
> +{
> +	of_clk_del_provider(pdev->dev.of_node);
> +	platform_set_drvdata(pdev, NULL);

We don't need to set platform_drvdata to NULL here.

> +	return 0;
> +}
> +

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/4] clk: add support for clocks provided by SCP(System Control Processor)

[Sudeep Holla]

Hi Stephen,

On 21/05/15 00:43, Stephen Boyd wrote:
> On 04/27, Sudeep Holla wrote:
>> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
>> index 9897f353bf1a..906bf7dd72a2 100644
>> --- a/drivers/clk/Kconfig
>> +++ b/drivers/clk/Kconfig
>> @@ -59,6 +59,16 @@ config COMMON_CLK_RK808
>>   	  clocked at 32KHz each. Clkout1 is always on, Clkout2 can off
>>   	  by control register.
>>
>> +config COMMON_CLK_SCPI
>> +        tristate "Clock driver controlled via SCPI interface"
>> +        depends on ARM_SCPI_PROTOCOL
>
> || COMPILE_TEST ?
>

[...]

>
>> +		return -ENOMEM;
>> +	}
>> +
>> +	for (idx = 0; idx < count; idx++) {
>> +		struct scpi_clk *sclk;
> [...]
>> +static int scpi_clk_remove(struct platform_device *pdev)
>> +{
>> +	of_clk_del_provider(pdev->dev.of_node);
>> +	platform_set_drvdata(pdev, NULL);
>
> We don't need to set platform_drvdata to NULL here.
>

Thanks for the review. I have posted v2 of this series and some of the
comments are not applicable(already done or code removed). I have
fixed all the remaining comments locally for v3.

Regards,
Sudeep

[PATCH 3/4] clk: scpi: add support for cpufreq virtual device

[Sudeep Holla]

The clocks for the CPUs are provided by SCP and are managed by this
clock driver. So the cpufreq device needs to be added only after the
clock get registered and removed when this driver is unloaded.

This patch manages the cpufreq virtual device based on the clock
availability.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
Cc: Mike Turquette <mturquette@linaro.org>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Liviu Dudau <Liviu.Dudau@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Jon Medhurst (Tixy) <tixy@linaro.org>
Cc: linux-clk@vger.kernel.org
---
 drivers/clk/clk-scpi.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/clk/clk-scpi.c b/drivers/clk/clk-scpi.c
index 70a2d57da32d..0c048f7d0c28 100644
--- a/drivers/clk/clk-scpi.c
+++ b/drivers/clk/clk-scpi.c
@@ -37,6 +37,7 @@ struct scpi_clk {
 #define to_scpi_clk(clk) container_of(clk, struct scpi_clk, hw)
 
 static struct scpi_ops *scpi_ops;
+static struct platform_device *cpufreq_dev;
 
 static unsigned long scpi_clk_recalc_rate(struct clk_hw *hw,
 					  unsigned long parent_rate)
@@ -270,11 +271,23 @@ static int scpi_clk_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, clk_data);
 
+	if (clk_ops_init == scpi_dvfs_ops_init) {
+		/* Add virtual cpufreq device depending SCPI clock */
+		cpufreq_dev = platform_device_register_simple("scpi-cpufreq",
+							      -1, NULL, 0);
+		if (!cpufreq_dev)
+			pr_warn("unable to register cpufreq device");
+	}
 	return 0;
 }
 
 static int scpi_clk_remove(struct platform_device *pdev)
 {
+	if (cpufreq_dev) {
+		platform_device_unregister(cpufreq_dev);
+		cpufreq_dev = NULL;
+	}
+
 	of_clk_del_provider(pdev->dev.of_node);
 	platform_set_drvdata(pdev, NULL);
 	return 0;
-- 
1.9.1

Re: [PATCH 3/4] clk: scpi: add support for cpufreq virtual device

[Stephen Boyd]

On 04/27, Sudeep Holla wrote:
> The clocks for the CPUs are provided by SCP and are managed by this
> clock driver. So the cpufreq device needs to be added only after the
> clock get registered and removed when this driver is unloaded.
> 
> This patch manages the cpufreq virtual device based on the clock
> availability.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

The cpufreq device can't handle probe defer? I suppose we do it
this way because we need to create a platform device somewhere
and this is the best place to do so?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/4] clk: scpi: add support for cpufreq virtual device

[Sudeep Holla]

On 21/05/15 00:45, Stephen Boyd wrote:
> On 04/27, Sudeep Holla wrote:
>> The clocks for the CPUs are provided by SCP and are managed by this
>> clock driver. So the cpufreq device needs to be added only after the
>> clock get registered and removed when this driver is unloaded.
>>
>> This patch manages the cpufreq virtual device based on the clock
>> availability.
>>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>
> The cpufreq device can't handle probe defer? I suppose we do it
> this way because we need to create a platform device somewhere
> and this is the best place to do so?
>

Correct, unless the communication with the SCP firmware is established
and the CPU clocks are registered, it makes no sense to register CPUFreq
virtual device. This seems to be best place IMO.

And yes, thanks to the absence of platform code on ARM64, where such
code was traditionally stashed away and got away with such scenarios.
Let me if you disagree with this or have any other suggestions.

Regards,
Sudeep