Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1375679 > unrolled thread

[RESEND PATCH v4] clk/axs10x: Add I2S PLL clock driver

Back to article view | Back to linux.kernel

Contents

  [RESEND PATCH v4] clk/axs10x: Add I2S PLL clock driver Jose Abreu <Jose.Abreu@synopsys.com> - 2016-04-11 12:50 +0200
    Re: [RESEND PATCH v4] clk/axs10x: Add I2S PLL clock driver Alexey Brodkin <Alexey.Brodkin@synopsys.com> - 2016-04-11 18:50 +0200
      Re: [RESEND PATCH v4] clk/axs10x: Add I2S PLL clock driver "sboyd@codeaurora.org" <sboyd@codeaurora.org> - 2016-04-12 00:10 +0200
        Re: [RESEND PATCH v4] clk/axs10x: Add I2S PLL clock driver Alexey Brodkin <Alexey.Brodkin@synopsys.com> - 2016-04-15 14:10 +0200

#1375679 — [RESEND PATCH v4] clk/axs10x: Add I2S PLL clock driver

The ARC SDP I2S clock can be programmed using a
specific PLL.

This patch has the goal of adding a clock driver
that programs this PLL.

At this moment the rate values are hardcoded in
a table but in the future it would be ideal to
use a function which determines the PLL values
given the desired rate.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
---

Changes v3 -> v4:
* Added binding document (as suggested by Stephen Boyd)
* Minor code style fixes (as suggested by Stephen Boyd)
* Use ioremap (as suggested by Stephen Boyd)
* Implement round_rate (as suggested by Stephen Boyd)
* Change to platform driver (as suggested by Stephen Boyd)
* Use {readl/writel}_relaxed (as suggested by Vineet Gupta)

Changes v2 -> v3:
* Implemented recalc_rate

Changes v1 -> v2:
* Renamed folder to axs10x (as suggested by Alexey Brodkin)
* Added more supported rates

 .../devicetree/bindings/clock/i2s-pll-clock.txt    |  17 ++
 drivers/clk/Makefile                               |   1 +
 drivers/clk/axs10x/Makefile                        |   1 +
 drivers/clk/axs10x/i2s_pll_clock.c                 | 217 +++++++++++++++++++++
 4 files changed, 236 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/i2s-pll-clock.txt
 create mode 100644 drivers/clk/axs10x/Makefile
 create mode 100644 drivers/clk/axs10x/i2s_pll_clock.c

diff --git a/Documentation/devicetree/bindings/clock/i2s-pll-clock.txt b/Documentation/devicetree/bindings/clock/i2s-pll-clock.txt
new file mode 100644
index 0000000..ff86a41
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/i2s-pll-clock.txt
@@ -0,0 +1,17 @@
+Binding for the AXS10X I2S PLL clock
+
+This binding uses the common clock binding[1].
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible: shall be "snps,i2s-pll-clock"
+- #clock-cells: from common clock binding; Should always be set to 0.
+- reg : Address and length of the I2S PLL register set.
+
+Example:
+	clock@0x100a0 {
+		compatible = "snps,i2s-pll-clock";
+		reg = <0x100a0 0x10>;
+		#clock-cells = <0>;
+	};
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 46869d6..2ca62dc6 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -84,3 +84,4 @@ obj-$(CONFIG_X86)			+= x86/
 obj-$(CONFIG_ARCH_ZX)			+= zte/
 obj-$(CONFIG_ARCH_ZYNQ)			+= zynq/
 obj-$(CONFIG_H8300)		+= h8300/
+obj-$(CONFIG_ARC_PLAT_AXS10X)		+= axs10x/
diff --git a/drivers/clk/axs10x/Makefile b/drivers/clk/axs10x/Makefile
new file mode 100644
index 0000000..01996b8
--- /dev/null
+++ b/drivers/clk/axs10x/Makefile
@@ -0,0 +1 @@
+obj-y += i2s_pll_clock.o
diff --git a/drivers/clk/axs10x/i2s_pll_clock.c b/drivers/clk/axs10x/i2s_pll_clock.c
new file mode 100644
index 0000000..3ba4e2f
--- /dev/null
+++ b/drivers/clk/axs10x/i2s_pll_clock.c
@@ -0,0 +1,217 @@
+/*
+ * Synopsys AXS10X SDP I2S PLL clock driver
+ *
+ * Copyright (C) 2016 Synopsys
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/device.h>
+#include <linux/of_address.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+
+/* FPGA Version Info */
+#define FPGA_VER_INFO	0xE0011230
+#define FPGA_VER_27M	0x000FBED9
+
+/* PLL registers addresses */
+#define PLL_IDIV_REG	0x0
+#define PLL_FBDIV_REG	0x4
+#define PLL_ODIV0_REG	0x8
+#define PLL_ODIV1_REG	0xC
+
+struct i2s_pll_cfg {
+	unsigned int rate;
+	unsigned int idiv;
+	unsigned int fbdiv;
+	unsigned int odiv0;
+	unsigned int odiv1;
+};
+
+static const struct i2s_pll_cfg i2s_pll_cfg_27m[] = {
+	/* 27 Mhz */
+	{ 1024000, 0x104, 0x451, 0x10E38, 0x2000 },
+	{ 1411200, 0x104, 0x596, 0x10D35, 0x2000 },
+	{ 1536000, 0x208, 0xA28, 0x10B2C, 0x2000 },
+	{ 2048000, 0x82, 0x451, 0x10E38, 0x2000 },
+	{ 2822400, 0x82, 0x596, 0x10D35, 0x2000 },
+	{ 3072000, 0x104, 0xA28, 0x10B2C, 0x2000 },
+	{ 2116800, 0x82, 0x3CF, 0x10C30, 0x2000 },
+	{ 2304000, 0x104, 0x79E, 0x10B2C, 0x2000 },
+	{ 0, 0, 0, 0, 0 },
+};
+
+static const struct i2s_pll_cfg i2s_pll_cfg_28m[] = {
+	/* 28.224 Mhz */
+	{ 1024000, 0x82, 0x105, 0x107DF, 0x2000 },
+	{ 1411200, 0x28A, 0x1, 0x10001, 0x2000 },
+	{ 1536000, 0xA28, 0x187, 0x10042, 0x2000 },
+	{ 2048000, 0x41, 0x105, 0x107DF, 0x2000 },
+	{ 2822400, 0x145, 0x1, 0x10001, 0x2000 },
+	{ 3072000, 0x514, 0x187, 0x10042, 0x2000 },
+	{ 2116800, 0x514, 0x42, 0x10001, 0x2000 },
+	{ 2304000, 0x619, 0x82, 0x10001, 0x2000 },
+	{ 0, 0, 0, 0, 0 },
+};
+
+struct i2s_pll_clk {
+	void __iomem *base;
+	struct clk_hw hw;
+	unsigned int ref_clk;
+	const struct i2s_pll_cfg *pll_cfg;
+};
+
+static inline void i2s_pll_write(struct i2s_pll_clk *clk, unsigned int reg,
+		unsigned int val)
+{
+	writel_relaxed(val, clk->base + reg);
+}
+
+static inline unsigned int i2s_pll_read(struct i2s_pll_clk *clk,
+		unsigned int reg)
+{
+	return readl_relaxed(clk->base + reg);
+}
+
+static inline struct i2s_pll_clk *to_i2s_pll_clk(struct clk_hw *hw)
+{
+	return container_of(hw, struct i2s_pll_clk, hw);
+}
+
+static unsigned int i2s_pll_get_value(unsigned int val)
+{
+	return (val & 0x3F) + ((val >> 6) & 0x3F);
+}
+
+static unsigned long i2s_pll_recalc_rate(struct clk_hw *hw,
+			unsigned long parent_rate)
+{
+	struct i2s_pll_clk *clk = to_i2s_pll_clk(hw);
+	unsigned int idiv, fbdiv, odiv;
+
+	idiv = i2s_pll_get_value(i2s_pll_read(clk, PLL_IDIV_REG));
+	fbdiv = i2s_pll_get_value(i2s_pll_read(clk, PLL_FBDIV_REG));
+	odiv = i2s_pll_get_value(i2s_pll_read(clk, PLL_ODIV0_REG));
+
+	return ((clk->ref_clk / idiv) * fbdiv) / odiv;
+}
+
+static long i2s_pll_round_rate(struct clk_hw *hw, unsigned long rate,
+			unsigned long *prate)
+{
+	const struct i2s_pll_cfg *pll_cfg = to_i2s_pll_clk(hw)->pll_cfg;
+	int i;
+
+	for (i = 0; pll_cfg[i].rate != 0; i++)
+		if (pll_cfg[i].rate == rate)
+			return rate;
+
+	return -EINVAL;
+}
+
+static int i2s_pll_set_rate(struct clk_hw *hw, unsigned long rate,
+			unsigned long parent_rate)
+{
+	struct i2s_pll_clk *clk = to_i2s_pll_clk(hw);
+	const struct i2s_pll_cfg *pll_cfg = clk->pll_cfg;
+	int i;
+
+	for (i = 0; pll_cfg[i].rate != 0; i++) {
+		if (pll_cfg[i].rate == rate) {
+			i2s_pll_write(clk, PLL_IDIV_REG, pll_cfg[i].idiv);
+			i2s_pll_write(clk, PLL_FBDIV_REG, pll_cfg[i].fbdiv);
+			i2s_pll_write(clk, PLL_ODIV0_REG, pll_cfg[i].odiv0);
+			i2s_pll_write(clk, PLL_ODIV1_REG, pll_cfg[i].odiv1);
+			return 0;
+		}
+	}
+
+	pr_err("%s: invalid rate=%ld, parent_rate=%ld\n", __func__,
+			rate, parent_rate);
+	return -EINVAL;
+}
+
+static const struct clk_ops i2s_pll_ops = {
+	.recalc_rate = i2s_pll_recalc_rate,
+	.round_rate = i2s_pll_round_rate,
+	.set_rate = i2s_pll_set_rate,
+};
+
+static int i2s_pll_clk_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *node = dev->of_node;
+	const char *clk_name;
+	struct clk *clk;
+	struct i2s_pll_clk *pll_clk;
+	struct clk_init_data init;
+	struct resource *mem;
+
+	if (!node)
+		return -ENODEV;
+
+	pll_clk = devm_kzalloc(dev, sizeof(*pll_clk), GFP_KERNEL);
+	if (!pll_clk)
+		return -ENOMEM;
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	pll_clk->base = devm_ioremap_resource(dev, mem);
+	if (IS_ERR(pll_clk->base))
+		return PTR_ERR(pll_clk->base);
+
+	clk_name = node->name;
+	init.name = clk_name;
+	init.ops = &i2s_pll_ops;
+	init.num_parents = 0;
+	pll_clk->hw.init = &init;
+
+	clk = clk_register(NULL, &pll_clk->hw);
+	if (IS_ERR(clk)) {
+		dev_err(dev, "failed to register %s div clock (%ld)\n",
+				clk_name, PTR_ERR(clk));
+		return PTR_ERR(clk);
+	}
+
+	if (readl((void *)FPGA_VER_INFO) <= FPGA_VER_27M) {
+		pll_clk->ref_clk = 27000000;
+		pll_clk->pll_cfg = i2s_pll_cfg_27m;
+	} else {
+		pll_clk->ref_clk = 28224000;
+		pll_clk->pll_cfg = i2s_pll_cfg_28m;
+	}
+
+	return of_clk_add_provider(node, of_clk_src_simple_get, clk);
+}
+
+static int i2s_pll_clk_remove(struct platform_device *pdev)
+{
+	of_clk_del_provider(pdev->dev.of_node);
+	return 0;
+}
+
+static const struct of_device_id i2s_pll_clk_id[] = {
+	{ .compatible = "snps,i2s-pll-clock", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, i2s_pll_clk_id);
+
+static struct platform_driver i2s_pll_clk_driver = {
+	.driver = {
+		.name = "i2s-pll-clock",
+		.of_match_table = of_match_ptr(i2s_pll_clk_id),
+	},
+	.probe = i2s_pll_clk_probe,
+	.remove = i2s_pll_clk_remove,
+};
+module_platform_driver(i2s_pll_clk_driver);
+
+MODULE_AUTHOR("Jose Abreu <joabreu@synopsys.com>");
+MODULE_DESCRIPTION("Synopsys AXS10X SDP I2S PLL Clock Driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

[toc] | [next] | [standalone]

#1376159

Hi Jose,

On Mon, 2016-04-11 at 11:41 +-0100, Jose Abreu wrote:
+AD4- The ARC SDP I2S clock can be programmed using a
+AD4- specific PLL.
+AD4- 
+AD4- This patch has the goal of adding a clock driver
+AD4- that programs this PLL.
+AD4- 
+AD4- At this moment the rate values are hardcoded in
+AD4- a table but in the future it would be ideal to
+AD4- use a function which determines the PLL values
+AD4- given the desired rate.
+AD4- 
+AD4- Signed-off-by: Jose Abreu +ADw-joabreu+AEA-synopsys.com+AD4-
+AD4- ---
+AD4- 
+AD4- Changes v3 -+AD4- v4:
+AD4- +ACo- Added binding document (as suggested by Stephen Boyd)
+AD4- +ACo- Minor code style fixes (as suggested by Stephen Boyd)
+AD4- +ACo- Use ioremap (as suggested by Stephen Boyd)
+AD4- +ACo- Implement round+AF8-rate (as suggested by Stephen Boyd)
+AD4- +ACo- Change to platform driver (as suggested by Stephen Boyd)
+AD4- +ACo- Use +AHs-readl/writel+AH0AXw-relaxed (as suggested by Vineet Gupta)
+AD4- 
+AD4- Changes v2 -+AD4- v3:
+AD4- +ACo- Implemented recalc+AF8-rate
+AD4- 
+AD4- Changes v1 -+AD4- v2:
+AD4- +ACo- Renamed folder to axs10x (as suggested by Alexey Brodkin)
+AD4- +ACo- Added more supported rates

+AFs-snip+AF0-

+AD4- diff --git a/Documentation/devicetree/bindings/clock/i2s-pll-clock.txt b/Documentation/devicetree/bindings/clock/i2s-
+AD4- pll-clock.txt
+AD4- new file mode 100644
+AD4- index 0000000..ff86a41
+AD4- --- /dev/null
+AD4- +-+-+- b/Documentation/devicetree/bindings/clock/i2s-pll-clock.txt
+AD4- +AEAAQA- -0,0 +-1,17 +AEAAQA-
+AD4- +-Binding for the AXS10X I2S PLL clock
+AD4- +-
+AD4- +-This binding uses the common clock binding+AFs-1+AF0-.
+AD4- +-
+AD4- +-+AFs-1+AF0- Documentation/devicetree/bindings/clock/clock-bindings.txt
+AD4- +-
+AD4- +-Required properties:
+AD4- +-- compatible: shall be +ACI-snps,i2s-pll-clock+ACI-
+AD4- +-- +ACM-clock-cells: from common clock binding+ADs- Should always be set to 0.
+AD4- +-- reg : Address and length of the I2S PLL register set.
+AD4- +-
+AD4- +-Example:
+AD4- +-	clock+AEA-0x100a0 +AHs-

Please remove +ACI-0x+ACI- from node name.

+AD4- +-		compatible +AD0- +ACI-snps,i2s-pll-clock+ACIAOw-
+AD4- +-		reg +AD0- +ADw-0x100a0 0x10+AD4AOw-
+AD4- +-		+ACM-clock-cells +AD0- +ADw-0+AD4AOw-
+AD4- +-	+AH0AOw-
+AD4- diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
+AD4- index 46869d6..2ca62dc6 100644
+AD4- --- a/drivers/clk/Makefile
+AD4- +-+-+- b/drivers/clk/Makefile
+AD4- +AEAAQA- -84,3 +-84,4 +AEAAQA- obj-+ACQ-(CONFIG+AF8-X86)			+-+AD0- x86/
+AD4- +AKA-obj-+ACQ-(CONFIG+AF8-ARCH+AF8-ZX)			+-+AD0- zte/
+AD4- +AKA-obj-+ACQ-(CONFIG+AF8-ARCH+AF8-ZYNQ)			+-+AD0- zynq/
+AD4- +AKA-obj-+ACQ-(CONFIG+AF8-H8300)		+-+AD0- h8300/
+AD4- +-obj-+ACQ-(CONFIG+AF8-ARC+AF8-PLAT+AF8-AXS10X)		+-+AD0- axs10x/
+AD4- diff --git a/drivers/clk/axs10x/Makefile b/drivers/clk/axs10x/Makefile
+AD4- new file mode 100644
+AD4- index 0000000..01996b8
+AD4- --- /dev/null
+AD4- +-+-+- b/drivers/clk/axs10x/Makefile
+AD4- +AEAAQA- -0,0 +-1 +AEAAQA-
+AD4- +-obj-y +-+AD0- i2s+AF8-pll+AF8-clock.o
+AD4- diff --git a/drivers/clk/axs10x/i2s+AF8-pll+AF8-clock.c b/drivers/clk/axs10x/i2s+AF8-pll+AF8-clock.c
+AD4- new file mode 100644
+AD4- index 0000000..3ba4e2f
+AD4- --- /dev/null
+AD4- +-+-+- b/drivers/clk/axs10x/i2s+AF8-pll+AF8-clock.c
+AD4- +AEAAQA- -0,0 +-1,217 +AEAAQA-
+AD4- +-/+ACo-
+AD4- +- +ACo- Synopsys AXS10X SDP I2S PLL clock driver
+AD4- +- +ACo-
+AD4- +- +ACo- Copyright (C) 2016 Synopsys
+AD4- +- +ACo-
+AD4- +- +ACo- This file is licensed under the terms of the GNU General Public
+AD4- +- +ACo- License version 2. This program is licensed +ACI-as is+ACI- without any
+AD4- +- +ACo- warranty of any kind, whether express or implied.
+AD4- +- +ACo-/
+AD4- +-
+AD4- +-+ACM-include +ADw-linux/platform+AF8-device.h+AD4-
+AD4- +-+ACM-include +ADw-linux/module.h+AD4-
+AD4- +-+ACM-include +ADw-linux/clk-provider.h+AD4-
+AD4- +-+ACM-include +ADw-linux/err.h+AD4-
+AD4- +-+ACM-include +ADw-linux/device.h+AD4-

+ACI-linux/platform+AF8-device.h+ACI- includes +ACI-linux/device.h+ACI- so you may make this list of headers
a little bit shorter.

+AD4- +-+ACM-include +ADw-linux/of+AF8-address.h+AD4-
+AD4- +-+ACM-include +ADw-linux/slab.h+AD4-
+AD4- +-+ACM-include +ADw-linux/of.h+AD4-

+ACI-linux/of+AF8-address.h+ACI- already includes +ACI-linux/of.h+ACI-.

+AFs-snip+AF0-

+AD4- +-
+AD4- +-static const struct of+AF8-device+AF8-id i2s+AF8-pll+AF8-clk+AF8-id+AFsAXQ- +AD0- +AHs-
+AD4- +-	+AHs- .compatible +AD0- +ACI-snps,i2s-pll-clock+ACI-, +AH0-,

I would think that it makes sense to add the board name in
this compatible string. So something like+AKAAIg-snps,axs10x-i2s-pll-clock+ACI-
IMHO looks much more informative.

Also adding Rob Herring and DT mailing list in Cc.
Please make sure Rod acks your bindings and corresponding docs.

-Alexey

[toc] | [prev] | [next] | [standalone]

#1376303

On 04/11, Alexey Brodkin wrote:
> On Mon, 2016-04-11 at 11:41 +0100, Jose Abreu wrote:
> > + * warranty of any kind, whether express or implied.
> > + */
> > +
> > +#include <linux/platform_device.h>
> > +#include <linux/module.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/err.h>
> > +#include <linux/device.h>
> 
> "linux/platform_device.h" includes "linux/device.h" so you may make this list of headers
> a little bit shorter.
> 
> > +#include <linux/of_address.h>
> > +#include <linux/slab.h>
> > +#include <linux/of.h>
> 
> "linux/of_address.h" already includes "linux/of.h".

It's ok to include things twice. In fact, its better to avoid any
implicit includes so that if we ever want to remove includes from
other headers we can do so without disturbing this driver.

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

[toc] | [prev] | [next] | [standalone]

#1379775

Hi+AKA-Stephen,

On Mon, 2016-04-11 at 15:03 -0700, sboyd+AEA-codeaurora.org wrote:
+AD4- On 04/11, Alexey Brodkin wrote:
+AD4- +AD4- 
+AD4- +AD4- On Mon, 2016-04-11 at 11:41 +-0100, Jose Abreu wrote:
+AD4- +AD4- +AD4- 
+AD4- +AD4- +AD4- +- +ACo- warranty of any kind, whether express or implied.
+AD4- +AD4- +AD4- +- +ACo-/
+AD4- +AD4- +AD4- +-
+AD4- +AD4- +AD4- +-+ACM-include +ADw-linux/platform+AF8-device.h+AD4-
+AD4- +AD4- +AD4- +-+ACM-include +ADw-linux/module.h+AD4-
+AD4- +AD4- +AD4- +-+ACM-include +ADw-linux/clk-provider.h+AD4-
+AD4- +AD4- +AD4- +-+ACM-include +ADw-linux/err.h+AD4-
+AD4- +AD4- +AD4- +-+ACM-include +ADw-linux/device.h+AD4-
+AD4- +AD4- +ACI-linux/platform+AF8-device.h+ACI- includes +ACI-linux/device.h+ACI- so you may make this list of headers
+AD4- +AD4- a little bit shorter.
+AD4- +AD4- 
+AD4- +AD4- +AD4- 
+AD4- +AD4- +AD4- +-+ACM-include +ADw-linux/of+AF8-address.h+AD4-
+AD4- +AD4- +AD4- +-+ACM-include +ADw-linux/slab.h+AD4-
+AD4- +AD4- +AD4- +-+ACM-include +ADw-linux/of.h+AD4-
+AD4- +AD4- +ACI-linux/of+AF8-address.h+ACI- already includes +ACI-linux/of.h+ACI-.
+AD4- It's ok to include things twice. In fact, its better to avoid any
+AD4- implicit includes so that if we ever want to remove includes from
+AD4- other headers we can do so without disturbing this driver.

IMHO approach with minimal amount of headers is nice just because
it's easier to check if everything is in place. I mean attempt to compile
will immediately reveal a missing header.

So that's what I do - I remove as many inclusions as I may until stuff compiles.

But with approach of explicit inclusion it's much easier to include
much more headers than really needed. The only way to figure out if header is really
required is to manually check all used functions in the current source
which is way too unreliable and probably nobody will do it ever anyways.
And that's how we'll get more stale and pointless inclusions.

-Alexey

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web