diff mbox

[1/2] clk: Add Oxford Semiconductor OXNAS Standard Clocks

Message ID 1459520791-13269-2-git-send-email-narmstrong@baylibre.com (mailing list archive)
State New, archived
Headers show

Commit Message

Neil Armstrong April 1, 2016, 2:26 p.m. UTC
Add Oxford Semiconductor OXNAS SoC Family Standard Clocks support.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/clk/Kconfig     |   6 ++
 drivers/clk/Makefile    |   1 +
 drivers/clk/clk-oxnas.c | 202 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 209 insertions(+)
 create mode 100644 drivers/clk/clk-oxnas.c

Comments

Stephen Boyd April 2, 2016, 12:50 a.m. UTC | #1
On 04/01, Neil Armstrong wrote:
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 16f7d33..2efdbab 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -197,6 +197,12 @@ config COMMON_CLK_PXA
>  	---help---
>  	  Support for the Marvell PXA SoC.
>  
> +config COMMON_CLK_OXNAS
> +	bool

Please add some text in quotes here so that this is user visible.

> +	select MFD_SYSCON
> +	---help---
> +	  Support for the OXNAS SoC Family clocks.
> +
> diff --git a/drivers/clk/clk-oxnas.c b/drivers/clk/clk-oxnas.c
> new file mode 100644
> index 0000000..5f02cfa
> --- /dev/null
> +++ b/drivers/clk/clk-oxnas.c
> @@ -0,0 +1,202 @@
> +
> +/* Clk init data declaration */
> +DECLARE_STD_CLK(leon);
> +DECLARE_STD_CLK(dma_sgdma);
> +DECLARE_STD_CLK(cipher);
> +DECLARE_STD_CLK(sata);
> +DECLARE_STD_CLK(audio);
> +DECLARE_STD_CLK(usbmph);
> +DECLARE_STD_CLKP(etha, eth_parents);
> +DECLARE_STD_CLK(pciea);
> +DECLARE_STD_CLK(nand);
> +
> +/* Bit - Name association */
> +static const struct clk_init_data *clk_oxnas_init[] = {
> +	[0] = &clk_leon_init,
> +	[1] = &clk_dma_sgdma_init,
> +	[2] = &clk_cipher_init,
> +	[3] = NULL,		/* Do not touch to DDR clock */

Why do we have this here then? The dt binding has different
numbers, so having this array with a hole in it seems fragile
when we're using the order of this array to map to the clk
indices.

> +	[4] = &clk_sata_init,
> +	[5] = &clk_audio_init,
> +	[6] = &clk_usbmph_init,
> +	[7] = &clk_etha_init,
> +	[8] = &clk_pciea_init,
> +	[9] = &clk_nand_init,
> +};
> +
> +static int oxnas_stdclk_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct regmap *regmap;
> +	struct clk_oxnas *clk_oxnas;
> +	struct clk_onecell_data *onecell_data;
> +	struct clk **clks;
> +	unsigned int clks_count = 0;
> +	int i;
> +
> +	clk_oxnas = devm_kzalloc(&pdev->dev,
> +				 sizeof(*clk_oxnas)*ARRAY_SIZE(clk_oxnas_init),
> +				 GFP_KERNEL);

devm_kcalloc()?

> +	if (!clk_oxnas)
> +		return -ENOMEM;
> +
> +	clks = devm_kzalloc(&pdev->dev,
> +			    sizeof(*clks)*ARRAY_SIZE(clk_oxnas_init),
> +			    GFP_KERNEL);

devm_kcalloc()? This can be one smaller because DDR is never
touched?

> +	if (!clks)
> +		return -ENOMEM;
> +
> +	onecell_data = devm_kzalloc(&pdev->dev, sizeof(*onecell_data),
> +				     GFP_KERNEL);

Maybe we should have one structure that we allocate all at once?

> +	if (!onecell_data)
> +		return -ENOMEM;
> +
> +	regmap = syscon_node_to_regmap(of_get_parent(np));

Can we use dev_get_regmap(&pdev->dev.parent) here instead? I'd
prefer device APIs over DT APIs here. 

> +	if (!regmap) {
> +		dev_err(&pdev->dev, "failed to have parent regmap\n");
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(clk_oxnas_init); i++) {
> +		struct clk_oxnas *_clk;
> +
> +		if (!clk_oxnas_init[i])
> +			continue;
> +
> +		_clk = &clk_oxnas[i];
> +		_clk->bit = i;

Ah I see now, the order is used to encode bit offset. The comment
above the init data array could be expanded a bit then please.

> +		_clk->hw.init = clk_oxnas_init[i];
> +		_clk->regmap = regmap;
> +
> +		clks[clks_count] = devm_clk_register(&pdev->dev, &_clk->hw);
> +		if (WARN_ON(IS_ERR(clks[clks_count])))
> +			return PTR_ERR(clks[clks_count]);
> +
> +		++clks_count;
> +	}
> +
> +	onecell_data->clks = clks;
> +	onecell_data->clk_num = clks_count;
> +
> +	return of_clk_add_provider(np, of_clk_src_onecell_get, onecell_data);
> +}
> +
> +static const struct of_device_id oxnas_stdclk_dt_ids[] = {
> +	{ .compatible = "oxsemi,ox810se-stdclk" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, oxnas_stdclk_dt_ids);
> +
> +static struct platform_driver oxnas_stdclk_driver = {
> +	.probe = oxnas_stdclk_probe,
> +	.remove = oxnas_stdclk_remove,
> +	.driver	= {
> +		.name = "oxnas-stdclk",
> +		.of_match_table = of_match_ptr(oxnas_stdclk_dt_ids),

You can drop of_match_ptr() here, it will just lead to unused
variable warnings.
Neil Armstrong April 2, 2016, 10:45 a.m. UTC | #2
On 04/02/2016 02:50 AM, Stephen Boyd wrote:
>> +
>> +/* Bit - Name association */
>> +static const struct clk_init_data *clk_oxnas_init[] = {
>> +	[0] = &clk_leon_init,
>> +	[1] = &clk_dma_sgdma_init,
>> +	[2] = &clk_cipher_init,
>> +	[3] = NULL,		/* Do not touch to DDR clock */
> 
> Why do we have this here then? The dt binding has different
> numbers, so having this array with a hole in it seems fragile
> when we're using the order of this array to map to the clk
> indices.

Hi Stephen,

I skipped the DDR clock since it must be touched in any case, I could have introduced a read-only clock....

I already posted an arm-soc dts pull request with the DTSI using the bindings indices.
Here the array indice is the register bit index, maybe I can order by bindings indice and add the hardware bit in an intermediate structure ?

>> +	clk_oxnas = devm_kzalloc(&pdev->dev,
>> +				 sizeof(*clk_oxnas)*ARRAY_SIZE(clk_oxnas_init),
>> +				 GFP_KERNEL);
> 
> devm_kcalloc()?
> 
>> +	if (!clk_oxnas)
>> +		return -ENOMEM;
>> +
>> +	clks = devm_kzalloc(&pdev->dev,
>> +			    sizeof(*clks)*ARRAY_SIZE(clk_oxnas_init),
>> +			    GFP_KERNEL);
> 
> devm_kcalloc()? This can be one smaller because DDR is never
> touched?

Sure
> 
>> +	if (!clks)
>> +		return -ENOMEM;
>> +
>> +	onecell_data = devm_kzalloc(&pdev->dev, sizeof(*onecell_data),
>> +				     GFP_KERNEL);
> 
> Maybe we should have one structure that we allocate all at once?
> 
>> +	if (!onecell_data)
>> +		return -ENOMEM;
>> +
>> +	regmap = syscon_node_to_regmap(of_get_parent(np));
> 
> Can we use dev_get_regmap(&pdev->dev.parent) here instead? I'd
> prefer device APIs over DT APIs here. 

Ok

>> +	if (!regmap) {
>> +		dev_err(&pdev->dev, "failed to have parent regmap\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	for (i = 0; i < ARRAY_SIZE(clk_oxnas_init); i++) {
>> +		struct clk_oxnas *_clk;
>> +
>> +		if (!clk_oxnas_init[i])
>> +			continue;
>> +
>> +		_clk = &clk_oxnas[i];
>> +		_clk->bit = i;
> 
> Ah I see now, the order is used to encode bit offset. The comment
> above the init data array could be expanded a bit then please.
Sure it need clarifications !

> 
>> +		_clk->hw.init = clk_oxnas_init[i];
>> +		_clk->regmap = regmap;
>> +
>> +		clks[clks_count] = devm_clk_register(&pdev->dev, &_clk->hw);
>> +		if (WARN_ON(IS_ERR(clks[clks_count])))
>> +			return PTR_ERR(clks[clks_count]);
>> +
>> +		++clks_count;
>> +	}
>> +
>> +	onecell_data->clks = clks;
>> +	onecell_data->clk_num = clks_count;
>> +
>> +	return of_clk_add_provider(np, of_clk_src_onecell_get, onecell_data);
>> +}
>> +
>> +static const struct of_device_id oxnas_stdclk_dt_ids[] = {
>> +	{ .compatible = "oxsemi,ox810se-stdclk" },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, oxnas_stdclk_dt_ids);
>> +
>> +static struct platform_driver oxnas_stdclk_driver = {
>> +	.probe = oxnas_stdclk_probe,
>> +	.remove = oxnas_stdclk_remove,
>> +	.driver	= {
>> +		.name = "oxnas-stdclk",
>> +		.of_match_table = of_match_ptr(oxnas_stdclk_dt_ids),
> 
> You can drop of_match_ptr() here, it will just lead to unused
> variable warnings.

OK, thanks for the review, I hope the next post will go throught 4.7 !

Regards,
Neil
Neil Armstrong April 3, 2016, 1:56 p.m. UTC | #3
On 04/02/2016 02:50 AM, Stephen Boyd wrote:
> On 04/01, Neil Armstrong wrote:
>> +	if (!onecell_data)
>> +		return -ENOMEM;
>> +
>> +	regmap = syscon_node_to_regmap(of_get_parent(np));
> 
> Can we use dev_get_regmap(&pdev->dev.parent) here instead? I'd
> prefer device APIs over DT APIs here. 
> 

It will not work here since the parent node is a syscon, the call to syscon_node_to_regmap() will call of_syscon_register() and create the regmap, the dev_get_regmap() needs a proper platform device registered as regmap here.

Neil
Stephen Boyd April 7, 2016, 11:31 p.m. UTC | #4
On 04/03, Neil Armstrong wrote:
> On 04/02/2016 02:50 AM, Stephen Boyd wrote:
> > On 04/01, Neil Armstrong wrote:
> >> +	if (!onecell_data)
> >> +		return -ENOMEM;
> >> +
> >> +	regmap = syscon_node_to_regmap(of_get_parent(np));
> > 
> > Can we use dev_get_regmap(&pdev->dev.parent) here instead? I'd
> > prefer device APIs over DT APIs here. 
> > 
> 
> It will not work here since the parent node is a syscon, the call to syscon_node_to_regmap() will call of_syscon_register() and create the regmap, the dev_get_regmap() needs a proper platform device registered as regmap here.
> 

Ok. I was hoping that we could make simple-mfd look to see if
there's a syscon and then attach it to the parent device, but it
seems that simple-mfd is not actually a driver and it might not
even make a parent device for the children nodes?
Neil Armstrong April 11, 2016, 2 p.m. UTC | #5
On 04/08/2016 01:31 AM, Stephen Boyd wrote:
> On 04/03, Neil Armstrong wrote:
>> On 04/02/2016 02:50 AM, Stephen Boyd wrote:
> 
> Ok. I was hoping that we could make simple-mfd look to see if
> there's a syscon and then attach it to the parent device, but it
> seems that simple-mfd is not actually a driver and it might not
> even make a parent device for the children nodes?
> 

It seems so, I will look at the device hierarchy next week.
Syscon is only used by these syscon_node_to_regmap() or similar calls, and they are the only APIs available.

This call is at least used by clk-mtk, nxp and at91 clocks, is there a plan for these to change to another API ?

Neil
Stephen Boyd April 11, 2016, 10:04 p.m. UTC | #6
On 04/11, Neil Armstrong wrote:
> On 04/08/2016 01:31 AM, Stephen Boyd wrote:
> > On 04/03, Neil Armstrong wrote:
> >> On 04/02/2016 02:50 AM, Stephen Boyd wrote:
> > 
> > Ok. I was hoping that we could make simple-mfd look to see if
> > there's a syscon and then attach it to the parent device, but it
> > seems that simple-mfd is not actually a driver and it might not
> > even make a parent device for the children nodes?
> > 
> 
> It seems so, I will look at the device hierarchy next week.
> Syscon is only used by these syscon_node_to_regmap() or similar calls, and they are the only APIs available.
> 
> This call is at least used by clk-mtk, nxp and at91 clocks, is there a plan for these to change to another API ?

No plans. I'm just complaining.
diff mbox

Patch

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 16f7d33..2efdbab 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -197,6 +197,12 @@  config COMMON_CLK_PXA
 	---help---
 	  Support for the Marvell PXA SoC.
 
+config COMMON_CLK_OXNAS
+	bool
+	select MFD_SYSCON
+	---help---
+	  Support for the OXNAS SoC Family clocks.
+
 source "drivers/clk/bcm/Kconfig"
 source "drivers/clk/hisilicon/Kconfig"
 source "drivers/clk/mvebu/Kconfig"
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 46869d6..627da26 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -33,6 +33,7 @@  obj-$(CONFIG_ARCH_MB86S7X)		+= clk-mb86s7x.o
 obj-$(CONFIG_ARCH_MOXART)		+= clk-moxart.o
 obj-$(CONFIG_ARCH_NOMADIK)		+= clk-nomadik.o
 obj-$(CONFIG_ARCH_NSPIRE)		+= clk-nspire.o
+obj-$(CONFIG_COMMON_CLK_OXNAS)		+= clk-oxnas.o
 obj-$(CONFIG_COMMON_CLK_PALMAS)		+= clk-palmas.o
 obj-$(CONFIG_CLK_QORIQ)			+= clk-qoriq.o
 obj-$(CONFIG_COMMON_CLK_RK808)		+= clk-rk808.o
diff --git a/drivers/clk/clk-oxnas.c b/drivers/clk/clk-oxnas.c
new file mode 100644
index 0000000..5f02cfa
--- /dev/null
+++ b/drivers/clk/clk-oxnas.c
@@ -0,0 +1,202 @@ 
+/*
+ * Copyright (C) 2010 Broadcom
+ * Copyright (C) 2012 Stephen Warren
+ * Copyright (C) 2016 Neil Armstrong <narmstrong@baylibre.com>
+ *
+ * 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/clk-provider.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/stringify.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+
+/* Standard regmap gate clocks */
+struct clk_oxnas {
+	struct clk_hw hw;
+	signed char bit;
+	struct regmap *regmap;
+};
+
+/* Regmap offsets */
+#define CLK_STAT_REGOFFSET	0x24
+#define CLK_SET_REGOFFSET	0x2c
+#define CLK_CLR_REGOFFSET	0x30
+
+static inline struct clk_oxnas *to_clk_oxnas(struct clk_hw *hw)
+{
+	return container_of(hw, struct clk_oxnas, hw);
+}
+
+static int oxnas_clk_is_enabled(struct clk_hw *hw)
+{
+	struct clk_oxnas *std = to_clk_oxnas(hw);
+	int ret;
+	unsigned int val;
+
+	ret = regmap_read(std->regmap, CLK_STAT_REGOFFSET, &val);
+	if (ret < 0)
+		return ret;
+
+	return val & BIT(std->bit);
+}
+
+static int oxnas_clk_enable(struct clk_hw *hw)
+{
+	struct clk_oxnas *std = to_clk_oxnas(hw);
+
+	regmap_write(std->regmap, CLK_SET_REGOFFSET, BIT(std->bit));
+
+	return 0;
+}
+
+static void oxnas_clk_disable(struct clk_hw *hw)
+{
+	struct clk_oxnas *std = to_clk_oxnas(hw);
+
+	regmap_write(std->regmap, CLK_CLR_REGOFFSET, BIT(std->bit));
+}
+
+static const struct clk_ops oxnas_clk_ops = {
+	.enable = oxnas_clk_enable,
+	.disable = oxnas_clk_disable,
+	.is_enabled = oxnas_clk_is_enabled,
+};
+
+static const char *const oxnas_clk_parents[] = {
+	"oscillator",
+};
+
+static const char *const eth_parents[] = {
+	"gmacclk",
+};
+
+#define DECLARE_STD_CLKP(__clk, __parent)			\
+static const struct clk_init_data clk_##__clk##_init = {	\
+	.name = __stringify(__clk),				\
+	.ops = &oxnas_clk_ops,					\
+	.parent_names = __parent,				\
+	.num_parents = ARRAY_SIZE(__parent),			\
+}
+
+#define DECLARE_STD_CLK(__clk) DECLARE_STD_CLKP(__clk, oxnas_clk_parents)
+
+/* Clk init data declaration */
+DECLARE_STD_CLK(leon);
+DECLARE_STD_CLK(dma_sgdma);
+DECLARE_STD_CLK(cipher);
+DECLARE_STD_CLK(sata);
+DECLARE_STD_CLK(audio);
+DECLARE_STD_CLK(usbmph);
+DECLARE_STD_CLKP(etha, eth_parents);
+DECLARE_STD_CLK(pciea);
+DECLARE_STD_CLK(nand);
+
+/* Bit - Name association */
+static const struct clk_init_data *clk_oxnas_init[] = {
+	[0] = &clk_leon_init,
+	[1] = &clk_dma_sgdma_init,
+	[2] = &clk_cipher_init,
+	[3] = NULL,		/* Do not touch to DDR clock */
+	[4] = &clk_sata_init,
+	[5] = &clk_audio_init,
+	[6] = &clk_usbmph_init,
+	[7] = &clk_etha_init,
+	[8] = &clk_pciea_init,
+	[9] = &clk_nand_init,
+};
+
+static int oxnas_stdclk_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct regmap *regmap;
+	struct clk_oxnas *clk_oxnas;
+	struct clk_onecell_data *onecell_data;
+	struct clk **clks;
+	unsigned int clks_count = 0;
+	int i;
+
+	clk_oxnas = devm_kzalloc(&pdev->dev,
+				 sizeof(*clk_oxnas)*ARRAY_SIZE(clk_oxnas_init),
+				 GFP_KERNEL);
+	if (!clk_oxnas)
+		return -ENOMEM;
+
+	clks = devm_kzalloc(&pdev->dev,
+			    sizeof(*clks)*ARRAY_SIZE(clk_oxnas_init),
+			    GFP_KERNEL);
+	if (!clks)
+		return -ENOMEM;
+
+	onecell_data = devm_kzalloc(&pdev->dev, sizeof(*onecell_data),
+				     GFP_KERNEL);
+	if (!onecell_data)
+		return -ENOMEM;
+
+	regmap = syscon_node_to_regmap(of_get_parent(np));
+	if (!regmap) {
+		dev_err(&pdev->dev, "failed to have parent regmap\n");
+		return -EINVAL;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(clk_oxnas_init); i++) {
+		struct clk_oxnas *_clk;
+
+		if (!clk_oxnas_init[i])
+			continue;
+
+		_clk = &clk_oxnas[i];
+		_clk->bit = i;
+		_clk->hw.init = clk_oxnas_init[i];
+		_clk->regmap = regmap;
+
+		clks[clks_count] = devm_clk_register(&pdev->dev, &_clk->hw);
+		if (WARN_ON(IS_ERR(clks[clks_count])))
+			return PTR_ERR(clks[clks_count]);
+
+		++clks_count;
+	}
+
+	onecell_data->clks = clks;
+	onecell_data->clk_num = clks_count;
+
+	return of_clk_add_provider(np, of_clk_src_onecell_get, onecell_data);
+}
+
+static int oxnas_stdclk_remove(struct platform_device *pdev)
+{
+	of_clk_del_provider(pdev->dev.of_node);
+
+	return 0;
+}
+
+static const struct of_device_id oxnas_stdclk_dt_ids[] = {
+	{ .compatible = "oxsemi,ox810se-stdclk" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, oxnas_stdclk_dt_ids);
+
+static struct platform_driver oxnas_stdclk_driver = {
+	.probe = oxnas_stdclk_probe,
+	.remove = oxnas_stdclk_remove,
+	.driver	= {
+		.name = "oxnas-stdclk",
+		.of_match_table = of_match_ptr(oxnas_stdclk_dt_ids),
+	},
+};
+
+module_platform_driver(oxnas_stdclk_driver);