[PATCH 0/2] Enable wifi on pandaboard

[PATCH 0/2] Enable wifi on pandaboard

[Stefan Assmann]

On the pandaboard the wifi isn't available because on the TWL6030 chip
the clock CLK32KG never gets enabled. Adding a clock driver for the
TWL6030 which performs the required steps. Moved some register defines
from regulator code to twl.h.

Stefan Assmann (2):
  mfd: twl-core: move TWL6030 defines to twl.h
  clk: initial clock driver for TWL6030

 .../devicetree/bindings/clock/ti/twl6030.txt       |   4 +
 drivers/clk/Kconfig                                |   7 +
 drivers/clk/ti/Makefile                            |   1 +
 drivers/clk/ti/clk-6030.c                          | 141 +++++++++++++++++++++
 drivers/mfd/twl-core.c                             |   3 +
 drivers/regulator/twl-regulator.c                  |  11 --
 include/linux/i2c/twl.h                            |  20 +++
 7 files changed, 176 insertions(+), 11 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/ti/twl6030.txt
 create mode 100644 drivers/clk/ti/clk-6030.c

-- 
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] mfd: twl-core: move TWL6030 defines to twl.h

[Stefan Assmann]

These defines should be available to all drivers. Also added register
offset for CLK32KG_CFG_STATE and GRP defines.

Signed-off-by: Stefan Assmann <sassmann-llIHtaV5axyzQB+pC5nmwQ@public.gmane.org>
---
 drivers/regulator/twl-regulator.c | 11 -----------
 include/linux/i2c/twl.h           | 20 ++++++++++++++++++++
 2 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
index fed28ab..ad0dd22 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -94,17 +94,6 @@ struct twlreg_info {
 #define VREG_BC_PROC		3
 #define VREG_BC_CLK_RST		4
 
-/* TWL6030 LDO register values for CFG_STATE */
-#define TWL6030_CFG_STATE_OFF	0x00
-#define TWL6030_CFG_STATE_ON	0x01
-#define TWL6030_CFG_STATE_OFF2	0x02
-#define TWL6030_CFG_STATE_SLEEP	0x03
-#define TWL6030_CFG_STATE_GRP_SHIFT	5
-#define TWL6030_CFG_STATE_APP_SHIFT	2
-#define TWL6030_CFG_STATE_APP_MASK	(0x03 << TWL6030_CFG_STATE_APP_SHIFT)
-#define TWL6030_CFG_STATE_APP(v)	(((v) & TWL6030_CFG_STATE_APP_MASK) >>\
-						TWL6030_CFG_STATE_APP_SHIFT)
-
 /* Flags for SMPS Voltage reading */
 #define SMPS_OFFSET_EN		BIT(0)
 #define SMPS_EXTENDED_EN	BIT(1)
diff --git a/include/linux/i2c/twl.h b/include/linux/i2c/twl.h
index 8cfb50f..8ad63a2 100644
--- a/include/linux/i2c/twl.h
+++ b/include/linux/i2c/twl.h
@@ -127,6 +127,20 @@ enum twl6030_module_ids {
 #define REG_INT_MSK_STS_B		0x07
 #define REG_INT_MSK_STS_C		0x08
 
+/* TWL6030 register values for CFG_STATE */
+#define TWL6030_GRP_APP			(1 << 0)
+#define TWL6030_GRP_CON			(1 << 1)
+#define TWL6030_GRP_MOD			(1 << 2)
+#define TWL6030_CFG_STATE_OFF		0x00
+#define TWL6030_CFG_STATE_ON		0x01
+#define TWL6030_CFG_STATE_OFF2		0x02
+#define TWL6030_CFG_STATE_SLEEP		0x03
+#define TWL6030_CFG_STATE_GRP_SHIFT	5
+#define TWL6030_CFG_STATE_APP_SHIFT	2
+#define TWL6030_CFG_STATE_APP_MASK	(0x03 << TWL6030_CFG_STATE_APP_SHIFT)
+#define TWL6030_CFG_STATE_APP(v)	(((v) & TWL6030_CFG_STATE_APP_MASK) >>\
+						TWL6030_CFG_STATE_APP_SHIFT)
+
 /* MASK INT REG GROUP A */
 #define TWL6030_PWR_INT_MASK 		0x07
 #define TWL6030_RTC_INT_MASK 		0x18
@@ -470,6 +484,12 @@ static inline int twl6030_mmc_card_detect(struct device *dev, int slot)
 
 #define TWL4030_PM_MASTER_GLOBAL_TST		0xb6
 
+/*
+ * PM Receiver module register offsets (use TWL_MODULE_PM_RECEIVER)
+ */
+
+#define TWL6030_PM_RECEIVER_CLK32KG_CFG_STATE	0x8e
+
 /*----------------------------------------------------------------------*/
 
 /* Power bus message definitions */
-- 
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] mfd: twl-core: move TWL6030 defines to twl.h

[Lee Jones]

On Wed, 30 Jul 2014, Stefan Assmann wrote:

> These defines should be available to all drivers. Also added register
> offset for CLK32KG_CFG_STATE and GRP defines.
> 
> Signed-off-by: Stefan Assmann <sassmann-llIHtaV5axyzQB+pC5nmwQ@public.gmane.org>
> ---
>  drivers/regulator/twl-regulator.c | 11 -----------
>  include/linux/i2c/twl.h           | 20 ++++++++++++++++++++
>  2 files changed, 20 insertions(+), 11 deletions(-)

This doesn't have anything to do with MFD?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] mfd: twl-core: move TWL6030 defines to twl.h

[Stefan Assmann]

On 31.07.2014 10:36, Lee Jones wrote:
> On Wed, 30 Jul 2014, Stefan Assmann wrote:
> 
>> These defines should be available to all drivers. Also added register
>> offset for CLK32KG_CFG_STATE and GRP defines.
>>
>> Signed-off-by: Stefan Assmann <sassmann-llIHtaV5axyzQB+pC5nmwQ@public.gmane.org>
>> ---
>>  drivers/regulator/twl-regulator.c | 11 -----------
>>  include/linux/i2c/twl.h           | 20 ++++++++++++++++++++
>>  2 files changed, 20 insertions(+), 11 deletions(-)
> 
> This doesn't have anything to do with MFD?
> 

I added mfd because many other commits in include/linux/i2c/twl.h
reference it. We can drop it if you like.

  Stefan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] clk: initial clock driver for TWL6030

[Stefan Assmann]

Adding a clock driver for the TI TWL6030. The driver prepares the
CLK32KG clock, which is required for the wireless LAN.

Signed-off-by: Stefan Assmann <sassmann-llIHtaV5axyzQB+pC5nmwQ@public.gmane.org>
---
 .../devicetree/bindings/clock/ti/twl6030.txt       |   4 +
 drivers/clk/Kconfig                                |   7 +
 drivers/clk/ti/Makefile                            |   1 +
 drivers/clk/ti/clk-6030.c                          | 141 +++++++++++++++++++++
 drivers/mfd/twl-core.c                             |   3 +
 5 files changed, 156 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/ti/twl6030.txt
 create mode 100644 drivers/clk/ti/clk-6030.c

diff --git a/Documentation/devicetree/bindings/clock/ti/twl6030.txt b/Documentation/devicetree/bindings/clock/ti/twl6030.txt
new file mode 100644
index 0000000..d290ad4
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/ti/twl6030.txt
@@ -0,0 +1,4 @@
+Binding for TI TWL6030.
+
+Required properties:
+- compatible: "ti,twl6030-clk32kg"
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 9f9c5ae..4e89e8b 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -65,6 +65,13 @@ config COMMON_CLK_S2MPS11
 	  clock. These multi-function devices have two (S2MPS14) or three
 	  (S2MPS11, S5M8767) fixed-rate oscillators, clocked at 32KHz each.
 
+config CLK_TWL6030
+	tristate "Clock driver for twl6030"
+	depends on TWL4030_CORE
+	---help---
+	  Enable the TWL6030 clock CLK32KG which is disabled by default.
+	  Needed on the Pandaboard for the wireless LAN.
+
 config CLK_TWL6040
 	tristate "External McPDM functional clock from twl6040"
 	depends on TWL6040_CORE
diff --git a/drivers/clk/ti/Makefile b/drivers/clk/ti/Makefile
index ed4d0aa..04f25ea 100644
--- a/drivers/clk/ti/Makefile
+++ b/drivers/clk/ti/Makefile
@@ -10,4 +10,5 @@ obj-$(CONFIG_SOC_OMAP5)			+= $(clk-common) clk-54xx.o
 obj-$(CONFIG_SOC_DRA7XX)		+= $(clk-common) clk-7xx.o \
 					   clk-dra7-atl.o
 obj-$(CONFIG_SOC_AM43XX)		+= $(clk-common) clk-43xx.o
+obj-$(CONFIG_CLK_TWL6030)		+= $(clk-common) clk-6030.o
 endif
diff --git a/drivers/clk/ti/clk-6030.c b/drivers/clk/ti/clk-6030.c
new file mode 100644
index 0000000..baa965b
--- /dev/null
+++ b/drivers/clk/ti/clk-6030.c
@@ -0,0 +1,141 @@
+/*
+ * drivers/clk/ti/clk-6030.c
+ *
+ *  Copyright (C) 2014 Stefan Assmann <sassmann-llIHtaV5axyzQB+pC5nmwQ@public.gmane.org>
+ *
+ * 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.
+ *
+ * Clock driver for ti twl6030.
+ */
+
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/clkdev.h>
+#include <linux/clk-provider.h>
+#include <linux/i2c/twl.h>
+#include <linux/platform_device.h>
+
+struct twl6030_desc {
+	struct clk *clk;
+	struct clk_hw hw;
+	bool enabled;
+};
+
+#define to_twl6030_desc(_hw) container_of(_hw, struct twl6030_desc, hw)
+
+static int twl6030_clk32kg_prepare(struct clk_hw *hw)
+{
+	struct twl6030_desc *desc = to_twl6030_desc(hw);
+	int ret;
+
+	ret = twl_i2c_write_u8(TWL_MODULE_PM_RECEIVER,
+			       TWL6030_GRP_CON << TWL6030_CFG_STATE_GRP_SHIFT |
+			       TWL6030_CFG_STATE_ON,
+			       TWL6030_PM_RECEIVER_CLK32KG_CFG_STATE);
+	if (ret == 0)
+		desc->enabled = true;
+
+	return ret;
+}
+void twl6030_clk32kg_unprepare(struct clk_hw *hw)
+{
+	struct twl6030_desc *desc = to_twl6030_desc(hw);
+
+	twl_i2c_write_u8(TWL_MODULE_PM_RECEIVER,
+			 TWL6030_GRP_CON << TWL6030_CFG_STATE_GRP_SHIFT |
+			 TWL6030_CFG_STATE_OFF,
+			 TWL6030_PM_RECEIVER_CLK32KG_CFG_STATE);
+	desc->enabled = false;
+}
+
+static int twl6030_clk32kg_is_prepared(struct clk_hw *hw)
+{
+	struct twl6030_desc *desc = to_twl6030_desc(hw);
+
+	return desc->enabled;
+}
+
+static const struct clk_ops twl6030_clk32kg_ops = {
+	.prepare	= twl6030_clk32kg_prepare,
+	.unprepare	= twl6030_clk32kg_unprepare,
+	.is_prepared	= twl6030_clk32kg_is_prepared,
+};
+
+static void __init of_ti_twl6030_clk32kg_setup(struct device_node *node)
+{
+	struct twl6030_desc *clk_hw = NULL;
+	struct clk_init_data init = { 0 };
+	struct clk_lookup *clookup;
+	struct clk *clk;
+
+	clookup = kzalloc(sizeof(*clookup), GFP_KERNEL);
+	if (!clookup) {
+		pr_err("%s: could not allocate clookup\n", __func__);
+		return;
+	}
+	clk_hw = kzalloc(sizeof(*clk_hw), GFP_KERNEL);
+	if (!clk_hw) {
+		pr_err("%s: could not allocate clk_hw\n", __func__);
+		goto err_clk_hw;
+	}
+
+	clk_hw->hw.init = &init;
+
+	init.name = node->name;
+	init.ops = &twl6030_clk32kg_ops;
+	init.flags = CLK_IS_ROOT;
+
+	clk = clk_register(NULL, &clk_hw->hw);
+	if (!IS_ERR(clk)) {
+		clookup->con_id = kstrdup("clk32kg", GFP_KERNEL);
+		clookup->clk = clk;
+		clkdev_add(clookup);
+
+		return;
+	}
+
+	kfree(clookup);
+err_clk_hw:
+	kfree(clk_hw);
+}
+CLK_OF_DECLARE(of_ti_twl6030_clk32kg, "ti,twl6030-clk32kg", of_ti_twl6030_clk32kg_setup);
+
+static int of_twl6030_clk32kg_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	struct clk *clk;
+	int ret = 0;
+
+	if (!node)
+		return -ENODEV;
+
+	clk = clk_get(&pdev->dev, "clk32kg");
+	if (IS_ERR(clk))
+		ret = -EPROBE_DEFER;
+	else
+		clk_prepare(clk);
+
+	return ret;
+}
+
+static struct of_device_id of_twl6030_clk32kg_match_tbl[] = {
+	{ .compatible = "ti,twl6030-clk32kg", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_twl6030_clk32kg_match_tbl);
+
+static struct platform_driver twl6030_clk_driver = {
+	.driver = {
+		.name = "twl6030-clk32kg",
+		.owner = THIS_MODULE,
+		.of_match_table = of_twl6030_clk32kg_match_tbl,
+	},
+	.probe = of_twl6030_clk32kg_probe,
+};
+module_platform_driver(twl6030_clk_driver);
+
+MODULE_AUTHOR("Stefan Assmann <sassmann-llIHtaV5axyzQB+pC5nmwQ@public.gmane.org>");
+MODULE_DESCRIPTION("clock driver for TI SoC based boards with twl6030");
+MODULE_LICENSE("GPL");
diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
index db11b4f..440fe4e 100644
--- a/drivers/mfd/twl-core.c
+++ b/drivers/mfd/twl-core.c
@@ -34,6 +34,7 @@
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/err.h>
 #include <linux/device.h>
 #include <linux/of.h>
@@ -1012,6 +1013,8 @@ static void clocks_init(struct device *dev,
 	u32 rate;
 	u8 ctrl = HFCLK_FREQ_26_MHZ;
 
+	of_clk_init(NULL);
+
 	osc = clk_get(dev, "fck");
 	if (IS_ERR(osc)) {
 		printk(KERN_WARNING "Skipping twl internal clock init and "
-- 
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] clk: initial clock driver for TWL6030

[Andreas Färber]

Hi Stefan,

Glad you got it working. :)

Small nit below:

Am 30.07.2014 16:02, schrieb Stefan Assmann:
> diff --git a/drivers/clk/ti/clk-6030.c b/drivers/clk/ti/clk-6030.c
> new file mode 100644
> index 0000000..baa965b
> --- /dev/null
> +++ b/drivers/clk/ti/clk-6030.c
> @@ -0,0 +1,141 @@
> +/*
> + * drivers/clk/ti/clk-6030.c
> + *
> + *  Copyright (C) 2014 Stefan Assmann <sassmann@kpanic.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.

Here you say version 2 ...

> + *
> + * Clock driver for ti twl6030.
> + */
[...]
> +MODULE_AUTHOR("Stefan Assmann <sassmann@kpanic.de>");
> +MODULE_DESCRIPTION("clock driver for TI SoC based boards with twl6030");
> +MODULE_LICENSE("GPL");

... but down here it's "GPL". Either use "GPL v2" here, or IIUC use the
"or (at your option) any later version" license above if you can.

Cheers,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [PATCH 2/2] clk: initial clock driver for TWL6030

[Stefan Assmann]

On 30.07.2014 16:29, Andreas Färber wrote:
> Hi Stefan,
>
> Glad you got it working. :)
>
> Small nit below:
>
> Am 30.07.2014 16:02, schrieb Stefan Assmann:
>> diff --git a/drivers/clk/ti/clk-6030.c b/drivers/clk/ti/clk-6030.c
>> new file mode 100644
>> index 0000000..baa965b
>> --- /dev/null
>> +++ b/drivers/clk/ti/clk-6030.c
>> @@ -0,0 +1,141 @@
>> +/*
>> + * drivers/clk/ti/clk-6030.c
>> + *
>> + *  Copyright (C) 2014 Stefan Assmann <sassmann-llIHtaV5axyzQB+pC5nmwQ@public.gmane.org>
>> + *
>> + * 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.
>
> Here you say version 2 ...
>
>> + *
>> + * Clock driver for ti twl6030.
>> + */
> [...]
>> +MODULE_AUTHOR("Stefan Assmann <sassmann-llIHtaV5axyzQB+pC5nmwQ@public.gmane.org>");
>> +MODULE_DESCRIPTION("clock driver for TI SoC based boards with twl6030");
>> +MODULE_LICENSE("GPL");
>
> ... but down here it's "GPL". Either use "GPL v2" here, or IIUC use the
> "or (at your option) any later version" license above if you can.

Thanks for the hint. I'll fix it in the next version. :)

   Stefan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] clk: initial clock driver for TWL6030

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]

On Wed, Jul 30, 2014 at 04:02:29PM +0200, Stefan Assmann wrote:

> +static int twl6030_clk32kg_is_prepared(struct clk_hw *hw)
> +{
> +	struct twl6030_desc *desc = to_twl6030_desc(hw);
> +
> +	return desc->enabled;
> +}

Why not just check the register map - can't the register be cached?  If
that's not possible a comment would be good.

> +static int of_twl6030_clk32kg_probe(struct platform_device *pdev)
> +{
> +	struct device_node *node = pdev->dev.of_node;
> +	struct clk *clk;
> +	int ret = 0;
> +
> +	if (!node)
> +		return -ENODEV;
> +
> +	clk = clk_get(&pdev->dev, "clk32kg");

devm_clk_get()?

> +	if (IS_ERR(clk))
> +		ret = -EPROBE_DEFER;

Shouldn't the provided return code be being used?

> +	else
> +		clk_prepare(clk);

Why is the clock driver defaulting to enabling the clock, and if it
needs to shouldn't it be doing a prepere_enable() even if the enable
happens not to do anything to the hardware?  Otherwise child clocks
might get confused.

The return value is also not being checked.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] clk: initial clock driver for TWL6030

[Stefan Assmann]

On 30.07.2014 19:50, Mark Brown wrote:
> On Wed, Jul 30, 2014 at 04:02:29PM +0200, Stefan Assmann wrote:
> 
>> +static int twl6030_clk32kg_is_prepared(struct clk_hw *hw)
>> +{
>> +	struct twl6030_desc *desc = to_twl6030_desc(hw);
>> +
>> +	return desc->enabled;
>> +}
> 
> Why not just check the register map - can't the register be cached?  If
> that's not possible a comment would be good.

I just took atl_clk_is_enabled() as template. If you say it's better
to read the value, that can be arranged.

> 
>> +static int of_twl6030_clk32kg_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *node = pdev->dev.of_node;
>> +	struct clk *clk;
>> +	int ret = 0;
>> +
>> +	if (!node)
>> +		return -ENODEV;
>> +
>> +	clk = clk_get(&pdev->dev, "clk32kg");
> 
> devm_clk_get()?

Changed.

> 
>> +	if (IS_ERR(clk))
>> +		ret = -EPROBE_DEFER;
> 
> Shouldn't the provided return code be being used?

Changed.

> 
>> +	else
>> +		clk_prepare(clk);
> 
> Why is the clock driver defaulting to enabling the clock, and if it
> needs to shouldn't it be doing a prepere_enable() even if the enable
> happens not to do anything to the hardware?  Otherwise child clocks
> might get confused.

Mike advised me to convert the functions from enable/disable to
prepare/unprepare because i2c transactions may sleep. That's what I did.
The code no longer enables the clock and just prepares it. So IIUC the
call to clk_prepare() should be fine.

> 
> The return value is also not being checked.
> 

Changed.

Thanks for your comments Mark, here's a new version of the patch.

  Stefan

>From 2c52040f33af9dbb41e3e3f355ae154843b277c6 Mon Sep 17 00:00:00 2001
From: Stefan Assmann <sassmann-llIHtaV5axyzQB+pC5nmwQ@public.gmane.org>
Date: Fri, 25 Jul 2014 09:42:27 +0200
Subject: [PATCH] clk: initial clock driver for TWL6030

Adding a clock driver for the TI TWL6030. The driver prepares the
CLK32KG clock, which is required for the wireless LAN.

v2:
- use MODULE_LICENSE("GPL v2")
- use devm_clk_get() instead of clk_get()
- propagate return value of clk_prepare()
- read prepared state instead of using enabled variable

Signed-off-by: Stefan Assmann <sassmann-llIHtaV5axyzQB+pC5nmwQ@public.gmane.org>
---
 .../devicetree/bindings/clock/ti/twl6030.txt       |   4 +
 drivers/clk/Kconfig                                |   7 ++
 drivers/clk/ti/Makefile                            |   1 +
 drivers/clk/ti/clk-6030.c                          | 133 +++++++++++++++++++++
 drivers/mfd/twl-core.c                             |   3 +
 5 files changed, 148 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/ti/twl6030.txt
 create mode 100644 drivers/clk/ti/clk-6030.c

diff --git a/Documentation/devicetree/bindings/clock/ti/twl6030.txt b/Documentation/devicetree/bindings/clock/ti/twl6030.txt
new file mode 100644
index 0000000..d290ad4
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/ti/twl6030.txt
@@ -0,0 +1,4 @@
+Binding for TI TWL6030.
+
+Required properties:
+- compatible: "ti,twl6030-clk32kg"
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 9f9c5ae..4e89e8b 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -65,6 +65,13 @@ config COMMON_CLK_S2MPS11
 	  clock. These multi-function devices have two (S2MPS14) or three
 	  (S2MPS11, S5M8767) fixed-rate oscillators, clocked at 32KHz each.

+config CLK_TWL6030
+	tristate "Clock driver for twl6030"
+	depends on TWL4030_CORE
+	---help---
+	  Enable the TWL6030 clock CLK32KG which is disabled by default.
+	  Needed on the Pandaboard for the wireless LAN.
+
 config CLK_TWL6040
 	tristate "External McPDM functional clock from twl6040"
 	depends on TWL6040_CORE
diff --git a/drivers/clk/ti/Makefile b/drivers/clk/ti/Makefile
index ed4d0aa..04f25ea 100644
--- a/drivers/clk/ti/Makefile
+++ b/drivers/clk/ti/Makefile
@@ -10,4 +10,5 @@ obj-$(CONFIG_SOC_OMAP5)			+= $(clk-common) clk-54xx.o
 obj-$(CONFIG_SOC_DRA7XX)		+= $(clk-common) clk-7xx.o \
 					   clk-dra7-atl.o
 obj-$(CONFIG_SOC_AM43XX)		+= $(clk-common) clk-43xx.o
+obj-$(CONFIG_CLK_TWL6030)		+= $(clk-common) clk-6030.o
 endif
diff --git a/drivers/clk/ti/clk-6030.c b/drivers/clk/ti/clk-6030.c
new file mode 100644
index 0000000..61d1834
--- /dev/null
+++ b/drivers/clk/ti/clk-6030.c
@@ -0,0 +1,133 @@
+/*
+ * drivers/clk/ti/clk-6030.c
+ *
+ *  Copyright (C) 2014 Stefan Assmann <sassmann-llIHtaV5axyzQB+pC5nmwQ@public.gmane.org>
+ *
+ * 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.
+ *
+ * Clock driver for TI twl6030.
+ */
+
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/clkdev.h>
+#include <linux/clk-provider.h>
+#include <linux/i2c/twl.h>
+#include <linux/platform_device.h>
+
+struct twl6030_desc {
+	struct clk *clk;
+	struct clk_hw hw;
+};
+
+static int twl6030_clk32kg_prepare(struct clk_hw *hw)
+{
+	int ret;
+
+	ret = twl_i2c_write_u8(TWL_MODULE_PM_RECEIVER,
+			       TWL6030_GRP_CON << TWL6030_CFG_STATE_GRP_SHIFT |
+			       TWL6030_CFG_STATE_ON,
+			       TWL6030_PM_RECEIVER_CLK32KG_CFG_STATE);
+
+	return ret;
+}
+
+void twl6030_clk32kg_unprepare(struct clk_hw *hw)
+{
+	twl_i2c_write_u8(TWL_MODULE_PM_RECEIVER,
+			 TWL6030_GRP_CON << TWL6030_CFG_STATE_GRP_SHIFT |
+			 TWL6030_CFG_STATE_OFF,
+			 TWL6030_PM_RECEIVER_CLK32KG_CFG_STATE);
+}
+
+static int twl6030_clk32kg_is_prepared(struct clk_hw *hw)
+{
+	u8 is_prepared;
+
+	twl_i2c_read_u8(TWL_MODULE_PM_RECEIVER, &is_prepared,
+			TWL6030_PM_RECEIVER_CLK32KG_CFG_STATE);
+
+	return is_prepared & TWL6030_CFG_STATE_ON;
+}
+
+static const struct clk_ops twl6030_clk32kg_ops = {
+	.prepare	= twl6030_clk32kg_prepare,
+	.unprepare	= twl6030_clk32kg_unprepare,
+	.is_prepared	= twl6030_clk32kg_is_prepared,
+};
+
+static void __init of_ti_twl6030_clk32kg_setup(struct device_node *node)
+{
+	struct twl6030_desc *clk_hw = NULL;
+	struct clk_init_data init = { 0 };
+	struct clk_lookup *clookup;
+	struct clk *clk;
+
+	clookup = kzalloc(sizeof(*clookup), GFP_KERNEL);
+	if (!clookup) {
+		pr_err("%s: could not allocate clookup\n", __func__);
+		return;
+	}
+	clk_hw = kzalloc(sizeof(*clk_hw), GFP_KERNEL);
+	if (!clk_hw) {
+		pr_err("%s: could not allocate clk_hw\n", __func__);
+		goto err_clk_hw;
+	}
+
+	clk_hw->hw.init = &init;
+
+	init.name = node->name;
+	init.ops = &twl6030_clk32kg_ops;
+	init.flags = CLK_IS_ROOT;
+
+	clk = clk_register(NULL, &clk_hw->hw);
+	if (!IS_ERR(clk)) {
+		clookup->con_id = kstrdup("clk32kg", GFP_KERNEL);
+		clookup->clk = clk;
+		clkdev_add(clookup);
+
+		return;
+	}
+
+	kfree(clookup);
+err_clk_hw:
+	kfree(clk_hw);
+}
+CLK_OF_DECLARE(of_ti_twl6030_clk32kg, "ti,twl6030-clk32kg", of_ti_twl6030_clk32kg_setup);
+
+static int of_twl6030_clk32kg_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	struct clk *clk;
+
+	if (!node)
+		return -ENODEV;
+
+	clk = devm_clk_get(&pdev->dev, "clk32kg");
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	return clk_prepare(clk);
+}
+
+static struct of_device_id of_twl6030_clk32kg_match_tbl[] = {
+	{ .compatible = "ti,twl6030-clk32kg", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_twl6030_clk32kg_match_tbl);
+
+static struct platform_driver twl6030_clk_driver = {
+	.driver = {
+		.name = "twl6030-clk32kg",
+		.owner = THIS_MODULE,
+		.of_match_table = of_twl6030_clk32kg_match_tbl,
+	},
+	.probe = of_twl6030_clk32kg_probe,
+};
+module_platform_driver(twl6030_clk_driver);
+
+MODULE_AUTHOR("Stefan Assmann <sassmann-llIHtaV5axyzQB+pC5nmwQ@public.gmane.org>");
+MODULE_DESCRIPTION("clock driver for TI SoC based boards with twl6030");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
index db11b4f..440fe4e 100644
--- a/drivers/mfd/twl-core.c
+++ b/drivers/mfd/twl-core.c
@@ -34,6 +34,7 @@
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/err.h>
 #include <linux/device.h>
 #include <linux/of.h>
@@ -1012,6 +1013,8 @@ static void clocks_init(struct device *dev,
 	u32 rate;
 	u8 ctrl = HFCLK_FREQ_26_MHZ;

+	of_clk_init(NULL);
+
 	osc = clk_get(dev, "fck");
 	if (IS_ERR(osc)) {
 		printk(KERN_WARNING "Skipping twl internal clock init and "
-- 
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] clk: initial clock driver for TWL6030

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1563 bytes --]

On Thu, Jul 31, 2014 at 11:56:15AM +0200, Stefan Assmann wrote:
> On 30.07.2014 19:50, Mark Brown wrote:
> > On Wed, Jul 30, 2014 at 04:02:29PM +0200, Stefan Assmann wrote:

> >> +static int twl6030_clk32kg_is_prepared(struct clk_hw *hw)
> >> +{
> >> +	struct twl6030_desc *desc = to_twl6030_desc(hw);
> >> +
> >> +	return desc->enabled;
> >> +}

> > Why not just check the register map - can't the register be cached?  If
> > that's not possible a comment would be good.

> I just took atl_clk_is_enabled() as template. If you say it's better
> to read the value, that can be arranged.

It might be worth doing this if you have to go to hardware to check the
status, if you can read a cache then just using the register is less
error prone.

> >> +	else
> >> +		clk_prepare(clk);

> > Why is the clock driver defaulting to enabling the clock, and if it
> > needs to shouldn't it be doing a prepere_enable() even if the enable
> > happens not to do anything to the hardware?  Otherwise child clocks
> > might get confused.

> Mike advised me to convert the functions from enable/disable to
> prepare/unprepare because i2c transactions may sleep. That's what I did.
> The code no longer enables the clock and just prepares it. So IIUC the
> call to clk_prepare() should be fine.

That's not going to help consumers of the clock, you do need to move the
operations to prepare() but users shouldn't need to know what happens in
prepare() and what happens in enable().

You've also not addressed the comment about defaulting to enabling the
clock in the first place.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] clk: initial clock driver for TWL6030

[Stefan Assmann]

On 31.07.2014 13:05, Mark Brown wrote:
> On Thu, Jul 31, 2014 at 11:56:15AM +0200, Stefan Assmann wrote:
>> On 30.07.2014 19:50, Mark Brown wrote:
>>> On Wed, Jul 30, 2014 at 04:02:29PM +0200, Stefan Assmann wrote:
> 
>>>> +static int twl6030_clk32kg_is_prepared(struct clk_hw *hw)
>>>> +{
>>>> +	struct twl6030_desc *desc = to_twl6030_desc(hw);
>>>> +
>>>> +	return desc->enabled;
>>>> +}
> 
>>> Why not just check the register map - can't the register be cached?  If
>>> that's not possible a comment would be good.
> 
>> I just took atl_clk_is_enabled() as template. If you say it's better
>> to read the value, that can be arranged.
> 
> It might be worth doing this if you have to go to hardware to check the
> status, if you can read a cache then just using the register is less
> error prone.

Ok.

> 
>>>> +	else
>>>> +		clk_prepare(clk);
> 
>>> Why is the clock driver defaulting to enabling the clock, and if it
>>> needs to shouldn't it be doing a prepere_enable() even if the enable
>>> happens not to do anything to the hardware?  Otherwise child clocks
>>> might get confused.
> 
>> Mike advised me to convert the functions from enable/disable to
>> prepare/unprepare because i2c transactions may sleep. That's what I did.
>> The code no longer enables the clock and just prepares it. So IIUC the
>> call to clk_prepare() should be fine.
> 
> That's not going to help consumers of the clock, you do need to move the
> operations to prepare() but users shouldn't need to know what happens in
> prepare() and what happens in enable().
> 
> You've also not addressed the comment about defaulting to enabling the
> clock in the first place.

Maybe I misinterpreted your previous comment, sorry. So if I got it right
this time you're saying that the prepare/enable, disable/unprepare should
be seen as a single step. If that's the case then using prepare_enable()
should be used indeed.

Tero suggested to look into making this a generic i2c clock driver. I'll
look into that and report back.

  Stefan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] clk: initial clock driver for TWL6030

[Mike Turquette]

Quoting Stefan Assmann (2014-07-31 05:04:29)
> On 31.07.2014 13:05, Mark Brown wrote:
> > On Thu, Jul 31, 2014 at 11:56:15AM +0200, Stefan Assmann wrote:
> >> On 30.07.2014 19:50, Mark Brown wrote:
> >>> On Wed, Jul 30, 2014 at 04:02:29PM +0200, Stefan Assmann wrote:
> > 
> >>>> +static int twl6030_clk32kg_is_prepared(struct clk_hw *hw)
> >>>> +{
> >>>> +  struct twl6030_desc *desc = to_twl6030_desc(hw);
> >>>> +
> >>>> +  return desc->enabled;
> >>>> +}
> > 
> >>> Why not just check the register map - can't the register be cached?  If
> >>> that's not possible a comment would be good.
> > 
> >> I just took atl_clk_is_enabled() as template. If you say it's better
> >> to read the value, that can be arranged.
> > 
> > It might be worth doing this if you have to go to hardware to check the
> > status, if you can read a cache then just using the register is less
> > error prone.
> 
> Ok.
> 
> > 
> >>>> +  else
> >>>> +          clk_prepare(clk);
> > 
> >>> Why is the clock driver defaulting to enabling the clock, and if it
> >>> needs to shouldn't it be doing a prepere_enable() even if the enable
> >>> happens not to do anything to the hardware?  Otherwise child clocks
> >>> might get confused.
> > 
> >> Mike advised me to convert the functions from enable/disable to
> >> prepare/unprepare because i2c transactions may sleep. That's what I did.
> >> The code no longer enables the clock and just prepares it. So IIUC the
> >> call to clk_prepare() should be fine.
> > 
> > That's not going to help consumers of the clock, you do need to move the
> > operations to prepare() but users shouldn't need to know what happens in
> > prepare() and what happens in enable().
> > 
> > You've also not addressed the comment about defaulting to enabling the
> > clock in the first place.
> 
> Maybe I misinterpreted your previous comment, sorry. So if I got it right
> this time you're saying that the prepare/enable, disable/unprepare should
> be seen as a single step. If that's the case then using prepare_enable()
> should be used indeed.

It's not so much that they are single step. The point is that the way we
ungate a clock signal using the Linux clk.h api is by calling
clk_enable().

If you read the code for clk_enable you'll see that it depends on the
clock already having been prepared via clk_prepare(). The fact that your
particular implementation of this clock driver actually ungates the
physical signal via clk_prepare is irrelevant from the api's
perspective.  clk_enable is how we turn gateable clocks on, even if the
hardware really gets affected by the prior call to clk_prepare.

It would be a real mess if drivers knew the details of their underlying
clock hardware and only called clk_prepare and never clk_enable to
gate/ungate their clocks.

Regards,
Mike

> 
> Tero suggested to look into making this a generic i2c clock driver. I'll
> look into that and report back.
> 
>   Stefan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] clk: initial clock driver for TWL6030

[Tero Kristo]

On 07/31/2014 12:56 PM, Stefan Assmann wrote:
> On 30.07.2014 19:50, Mark Brown wrote:
>> On Wed, Jul 30, 2014 at 04:02:29PM +0200, Stefan Assmann wrote:
>>
>>> +static int twl6030_clk32kg_is_prepared(struct clk_hw *hw)
>>> +{
>>> +	struct twl6030_desc *desc = to_twl6030_desc(hw);
>>> +
>>> +	return desc->enabled;
>>> +}
>>
>> Why not just check the register map - can't the register be cached?  If
>> that's not possible a comment would be good.
>
> I just took atl_clk_is_enabled() as template. If you say it's better
> to read the value, that can be arranged.
>
>>
>>> +static int of_twl6030_clk32kg_probe(struct platform_device *pdev)
>>> +{
>>> +	struct device_node *node = pdev->dev.of_node;
>>> +	struct clk *clk;
>>> +	int ret = 0;
>>> +
>>> +	if (!node)
>>> +		return -ENODEV;
>>> +
>>> +	clk = clk_get(&pdev->dev, "clk32kg");
>>
>> devm_clk_get()?
>
> Changed.
>
>>
>>> +	if (IS_ERR(clk))
>>> +		ret = -EPROBE_DEFER;
>>
>> Shouldn't the provided return code be being used?
>
> Changed.
>
>>
>>> +	else
>>> +		clk_prepare(clk);
>>
>> Why is the clock driver defaulting to enabling the clock, and if it
>> needs to shouldn't it be doing a prepere_enable() even if the enable
>> happens not to do anything to the hardware?  Otherwise child clocks
>> might get confused.
>
> Mike advised me to convert the functions from enable/disable to
> prepare/unprepare because i2c transactions may sleep. That's what I did.
> The code no longer enables the clock and just prepares it. So IIUC the
> call to clk_prepare() should be fine.
>
>>
>> The return value is also not being checked.
>>
>
> Changed.
>
> Thanks for your comments Mark, here's a new version of the patch.
>
>    Stefan
>
>  From 2c52040f33af9dbb41e3e3f355ae154843b277c6 Mon Sep 17 00:00:00 2001
> From: Stefan Assmann <sassmann-llIHtaV5axyzQB+pC5nmwQ@public.gmane.org>
> Date: Fri, 25 Jul 2014 09:42:27 +0200
> Subject: [PATCH] clk: initial clock driver for TWL6030
>
> Adding a clock driver for the TI TWL6030. The driver prepares the
> CLK32KG clock, which is required for the wireless LAN.
>
> v2:
> - use MODULE_LICENSE("GPL v2")
> - use devm_clk_get() instead of clk_get()
> - propagate return value of clk_prepare()
> - read prepared state instead of using enabled variable
>
> Signed-off-by: Stefan Assmann <sassmann-llIHtaV5axyzQB+pC5nmwQ@public.gmane.org>
> ---
>   .../devicetree/bindings/clock/ti/twl6030.txt       |   4 +
>   drivers/clk/Kconfig                                |   7 ++
>   drivers/clk/ti/Makefile                            |   1 +
>   drivers/clk/ti/clk-6030.c                          | 133 +++++++++++++++++++++
>   drivers/mfd/twl-core.c                             |   3 +
>   5 files changed, 148 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/clock/ti/twl6030.txt
>   create mode 100644 drivers/clk/ti/clk-6030.c
>
> diff --git a/Documentation/devicetree/bindings/clock/ti/twl6030.txt b/Documentation/devicetree/bindings/clock/ti/twl6030.txt
> new file mode 100644
> index 0000000..d290ad4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/ti/twl6030.txt
> @@ -0,0 +1,4 @@
> +Binding for TI TWL6030.
> +
> +Required properties:
> +- compatible: "ti,twl6030-clk32kg"
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 9f9c5ae..4e89e8b 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -65,6 +65,13 @@ config COMMON_CLK_S2MPS11
>   	  clock. These multi-function devices have two (S2MPS14) or three
>   	  (S2MPS11, S5M8767) fixed-rate oscillators, clocked at 32KHz each.
>
> +config CLK_TWL6030
> +	tristate "Clock driver for twl6030"
> +	depends on TWL4030_CORE
> +	---help---
> +	  Enable the TWL6030 clock CLK32KG which is disabled by default.
> +	  Needed on the Pandaboard for the wireless LAN.
> +
>   config CLK_TWL6040
>   	tristate "External McPDM functional clock from twl6040"
>   	depends on TWL6040_CORE
> diff --git a/drivers/clk/ti/Makefile b/drivers/clk/ti/Makefile
> index ed4d0aa..04f25ea 100644
> --- a/drivers/clk/ti/Makefile
> +++ b/drivers/clk/ti/Makefile
> @@ -10,4 +10,5 @@ obj-$(CONFIG_SOC_OMAP5)			+= $(clk-common) clk-54xx.o
>   obj-$(CONFIG_SOC_DRA7XX)		+= $(clk-common) clk-7xx.o \
>   					   clk-dra7-atl.o
>   obj-$(CONFIG_SOC_AM43XX)		+= $(clk-common) clk-43xx.o
> +obj-$(CONFIG_CLK_TWL6030)		+= $(clk-common) clk-6030.o
>   endif
> diff --git a/drivers/clk/ti/clk-6030.c b/drivers/clk/ti/clk-6030.c
> new file mode 100644
> index 0000000..61d1834
> --- /dev/null
> +++ b/drivers/clk/ti/clk-6030.c

Please change the filename to something like clk-twl6030.c. clk-\d+ 
filenames basically denote a SoC under TI clock driver structure.

> @@ -0,0 +1,133 @@
> +/*
> + * drivers/clk/ti/clk-6030.c

Replace this with some sort of short description instead, see other 
files under drivers/clk/ti. Having an absolute filename here doesn't 
really provide anything.

> + *
> + *  Copyright (C) 2014 Stefan Assmann <sassmann-llIHtaV5axyzQB+pC5nmwQ@public.gmane.org>
> + *
> + * 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.
> + *
> + * Clock driver for TI twl6030.
> + */

A basic comment about this driver, how about trying to make it more 
generic, as in making it an i2c-clock driver? I guess there are other 
external chips/boards outside twl6030 family that would be interested in 
using it.

> +
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/clkdev.h>
> +#include <linux/clk-provider.h>
> +#include <linux/i2c/twl.h>
> +#include <linux/platform_device.h>
> +
> +struct twl6030_desc {
> +	struct clk *clk;
> +	struct clk_hw hw;
> +};
> +
> +static int twl6030_clk32kg_prepare(struct clk_hw *hw)
> +{
> +	int ret;
> +
> +	ret = twl_i2c_write_u8(TWL_MODULE_PM_RECEIVER,
> +			       TWL6030_GRP_CON << TWL6030_CFG_STATE_GRP_SHIFT |
> +			       TWL6030_CFG_STATE_ON,
> +			       TWL6030_PM_RECEIVER_CLK32KG_CFG_STATE);
> +
> +	return ret;
> +}
> +
> +void twl6030_clk32kg_unprepare(struct clk_hw *hw)
> +{
> +	twl_i2c_write_u8(TWL_MODULE_PM_RECEIVER,
> +			 TWL6030_GRP_CON << TWL6030_CFG_STATE_GRP_SHIFT |
> +			 TWL6030_CFG_STATE_OFF,
> +			 TWL6030_PM_RECEIVER_CLK32KG_CFG_STATE);
> +}
> +
> +static int twl6030_clk32kg_is_prepared(struct clk_hw *hw)
> +{
> +	u8 is_prepared;
> +
> +	twl_i2c_read_u8(TWL_MODULE_PM_RECEIVER, &is_prepared,
> +			TWL6030_PM_RECEIVER_CLK32KG_CFG_STATE);
> +
> +	return is_prepared & TWL6030_CFG_STATE_ON;
> +}
> +
> +static const struct clk_ops twl6030_clk32kg_ops = {
> +	.prepare	= twl6030_clk32kg_prepare,
> +	.unprepare	= twl6030_clk32kg_unprepare,
> +	.is_prepared	= twl6030_clk32kg_is_prepared,
> +};
> +
> +static void __init of_ti_twl6030_clk32kg_setup(struct device_node *node)
> +{
> +	struct twl6030_desc *clk_hw = NULL;
> +	struct clk_init_data init = { 0 };
> +	struct clk_lookup *clookup;
> +	struct clk *clk;
> +
> +	clookup = kzalloc(sizeof(*clookup), GFP_KERNEL);
> +	if (!clookup) {
> +		pr_err("%s: could not allocate clookup\n", __func__);
> +		return;
> +	}
> +	clk_hw = kzalloc(sizeof(*clk_hw), GFP_KERNEL);
> +	if (!clk_hw) {
> +		pr_err("%s: could not allocate clk_hw\n", __func__);
> +		goto err_clk_hw;
> +	}
> +
> +	clk_hw->hw.init = &init;
> +
> +	init.name = node->name;
> +	init.ops = &twl6030_clk32kg_ops;
> +	init.flags = CLK_IS_ROOT;
> +
> +	clk = clk_register(NULL, &clk_hw->hw);
> +	if (!IS_ERR(clk)) {
> +		clookup->con_id = kstrdup("clk32kg", GFP_KERNEL);
> +		clookup->clk = clk;
> +		clkdev_add(clookup);
> +
> +		return;
> +	}
> +
> +	kfree(clookup);
> +err_clk_hw:
> +	kfree(clk_hw);
> +}
> +CLK_OF_DECLARE(of_ti_twl6030_clk32kg, "ti,twl6030-clk32kg", of_ti_twl6030_clk32kg_setup);
> +
> +static int of_twl6030_clk32kg_probe(struct platform_device *pdev)
> +{
> +	struct device_node *node = pdev->dev.of_node;
> +	struct clk *clk;
> +
> +	if (!node)
> +		return -ENODEV;
> +
> +	clk = devm_clk_get(&pdev->dev, "clk32kg");
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	return clk_prepare(clk);

This is plain wrong as pointed out earlier. The driver that uses the 
clock must enable it.

> +}
> +
> +static struct of_device_id of_twl6030_clk32kg_match_tbl[] = {
> +	{ .compatible = "ti,twl6030-clk32kg", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, of_twl6030_clk32kg_match_tbl);
> +
> +static struct platform_driver twl6030_clk_driver = {
> +	.driver = {
> +		.name = "twl6030-clk32kg",
> +		.owner = THIS_MODULE,
> +		.of_match_table = of_twl6030_clk32kg_match_tbl,
> +	},
> +	.probe = of_twl6030_clk32kg_probe,
> +};
> +module_platform_driver(twl6030_clk_driver);
> +
> +MODULE_AUTHOR("Stefan Assmann <sassmann-llIHtaV5axyzQB+pC5nmwQ@public.gmane.org>");
> +MODULE_DESCRIPTION("clock driver for TI SoC based boards with twl6030");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
> index db11b4f..440fe4e 100644
> --- a/drivers/mfd/twl-core.c
> +++ b/drivers/mfd/twl-core.c
> @@ -34,6 +34,7 @@
>   #include <linux/platform_device.h>
>   #include <linux/regmap.h>
>   #include <linux/clk.h>
> +#include <linux/clk-provider.h>
>   #include <linux/err.h>
>   #include <linux/device.h>
>   #include <linux/of.h>
> @@ -1012,6 +1013,8 @@ static void clocks_init(struct device *dev,
>   	u32 rate;
>   	u8 ctrl = HFCLK_FREQ_26_MHZ;
>
> +	of_clk_init(NULL);
> +

Don't do this please, this is horrible. of_clk_init or alternatives 
should be called from board/SoC specific context, otherwise you end up 
calling the init functions multiple times. If you are facing an issue 
that TI boards do not initialize external clocks properly currently, 
this is because TI clock driver does its init hierarchically and does 
not parse the whole DT for clock nodes. I have an experimental patch 
available I can post for you to try out which provides external clock 
support on TI boards if you like.

-Tero


>   	osc = clk_get(dev, "fck");
>   	if (IS_ERR(osc)) {
>   		printk(KERN_WARNING "Skipping twl internal clock init and "
>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] clk: initial clock driver for TWL6030

[Stefan Assmann]

On 31.07.2014 13:28, Tero Kristo wrote:

[...]

>> diff --git a/drivers/clk/ti/clk-6030.c b/drivers/clk/ti/clk-6030.c
>> new file mode 100644
>> index 0000000..61d1834
>> --- /dev/null
>> +++ b/drivers/clk/ti/clk-6030.c
> 
> Please change the filename to something like clk-twl6030.c. clk-\d+ 
> filenames basically denote a SoC under TI clock driver structure.

Ok.

> 
>> @@ -0,0 +1,133 @@
>> +/*
>> + * drivers/clk/ti/clk-6030.c
> 
> Replace this with some sort of short description instead, see other 
> files under drivers/clk/ti. Having an absolute filename here doesn't 
> really provide anything.

Ok.

> 
>> + *
>> + *  Copyright (C) 2014 Stefan Assmann <sassmann-llIHtaV5axyzQB+pC5nmwQ@public.gmane.org>
>> + *
>> + * 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.
>> + *
>> + * Clock driver for TI twl6030.
>> + */
> 
> A basic comment about this driver, how about trying to make it more 
> generic, as in making it an i2c-clock driver? I guess there are other 
> external chips/boards outside twl6030 family that would be interested in 
> using it.

I'll look into that.

[...]

>> +static int of_twl6030_clk32kg_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *node = pdev->dev.of_node;
>> +	struct clk *clk;
>> +
>> +	if (!node)
>> +		return -ENODEV;
>> +
>> +	clk = devm_clk_get(&pdev->dev, "clk32kg");
>> +	if (IS_ERR(clk))
>> +		return PTR_ERR(clk);
>> +
>> +	return clk_prepare(clk);
> 
> This is plain wrong as pointed out earlier. The driver that uses the 
> clock must enable it.

Understood. I'll change it to clk_prepare_enable().

[...]

>> diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
>> index db11b4f..440fe4e 100644
>> --- a/drivers/mfd/twl-core.c
>> +++ b/drivers/mfd/twl-core.c
>> @@ -34,6 +34,7 @@
>>   #include <linux/platform_device.h>
>>   #include <linux/regmap.h>
>>   #include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>>   #include <linux/err.h>
>>   #include <linux/device.h>
>>   #include <linux/of.h>
>> @@ -1012,6 +1013,8 @@ static void clocks_init(struct device *dev,
>>   	u32 rate;
>>   	u8 ctrl = HFCLK_FREQ_26_MHZ;
>>
>> +	of_clk_init(NULL);
>> +
> 
> Don't do this please, this is horrible. of_clk_init or alternatives 
> should be called from board/SoC specific context, otherwise you end up 
> calling the init functions multiple times. If you are facing an issue 
> that TI boards do not initialize external clocks properly currently, 
> this is because TI clock driver does its init hierarchically and does 
> not parse the whole DT for clock nodes. I have an experimental patch 
> available I can post for you to try out which provides external clock 
> support on TI boards if you like.

If you could provide that patch that'll be great.

Thanks for the comments Tero.

  Stefan


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] clk: initial clock driver for TWL6030

[Tero Kristo]

<snip>

>>> +static int of_twl6030_clk32kg_probe(struct platform_device *pdev)
>>> +{
>>> +	struct device_node *node = pdev->dev.of_node;
>>> +	struct clk *clk;
>>> +
>>> +	if (!node)
>>> +		return -ENODEV;
>>> +
>>> +	clk = devm_clk_get(&pdev->dev, "clk32kg");
>>> +	if (IS_ERR(clk))
>>> +		return PTR_ERR(clk);
>>> +
>>> +	return clk_prepare(clk);
>>
>> This is plain wrong as pointed out earlier. The driver that uses the
>> clock must enable it.
>
> Understood. I'll change it to clk_prepare_enable().

Nono, remove it completely from here. The clock driver should not enable 
itself by default. The clock itself has some sort of customer somewhere 
(like am33xx-wifi driver), which should request for the clock and enable 
it only when needed.

-Tero

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] clk: initial clock driver for TWL6030

[Peter Ujfalusi]

On 07/30/2014 05:02 PM, Stefan Assmann wrote:
> Adding a clock driver for the TI TWL6030. The driver prepares the
> CLK32KG clock, which is required for the wireless LAN.
> 
> Signed-off-by: Stefan Assmann <sassmann-llIHtaV5axyzQB+pC5nmwQ@public.gmane.org>
> ---
>  .../devicetree/bindings/clock/ti/twl6030.txt       |   4 +
>  drivers/clk/Kconfig                                |   7 +
>  drivers/clk/ti/Makefile                            |   1 +
>  drivers/clk/ti/clk-6030.c                          | 141 +++++++++++++++++++++
>  drivers/mfd/twl-core.c                             |   3 +
>  5 files changed, 156 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/ti/twl6030.txt
>  create mode 100644 drivers/clk/ti/clk-6030.c
> 
> diff --git a/Documentation/devicetree/bindings/clock/ti/twl6030.txt b/Documentation/devicetree/bindings/clock/ti/twl6030.txt
> new file mode 100644
> index 0000000..d290ad4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/ti/twl6030.txt
> @@ -0,0 +1,4 @@
> +Binding for TI TWL6030.
> +
> +Required properties:
> +- compatible: "ti,twl6030-clk32kg"


This is not really helping out. Where would you put this compatible? How it is
related to twl6030?

> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 9f9c5ae..4e89e8b 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -65,6 +65,13 @@ config COMMON_CLK_S2MPS11
>  	  clock. These multi-function devices have two (S2MPS14) or three
>  	  (S2MPS11, S5M8767) fixed-rate oscillators, clocked at 32KHz each.
>  
> +config CLK_TWL6030
> +	tristate "Clock driver for twl6030"
> +	depends on TWL4030_CORE
> +	---help---
> +	  Enable the TWL6030 clock CLK32KG which is disabled by default.
> +	  Needed on the Pandaboard for the wireless LAN.

This is not relevant information. I'm sure the CLK32KG from twl6030 can be or
is used for other purposes on other boards.

> +
>  config CLK_TWL6040
>  	tristate "External McPDM functional clock from twl6040"
>  	depends on TWL6040_CORE
> diff --git a/drivers/clk/ti/Makefile b/drivers/clk/ti/Makefile
> index ed4d0aa..04f25ea 100644
> --- a/drivers/clk/ti/Makefile
> +++ b/drivers/clk/ti/Makefile
> @@ -10,4 +10,5 @@ obj-$(CONFIG_SOC_OMAP5)			+= $(clk-common) clk-54xx.o
>  obj-$(CONFIG_SOC_DRA7XX)		+= $(clk-common) clk-7xx.o \
>  					   clk-dra7-atl.o
>  obj-$(CONFIG_SOC_AM43XX)		+= $(clk-common) clk-43xx.o
> +obj-$(CONFIG_CLK_TWL6030)		+= $(clk-common) clk-6030.o
>  endif
> diff --git a/drivers/clk/ti/clk-6030.c b/drivers/clk/ti/clk-6030.c
> new file mode 100644
> index 0000000..baa965b
> --- /dev/null
> +++ b/drivers/clk/ti/clk-6030.c
> @@ -0,0 +1,141 @@
> +/*
> + * drivers/clk/ti/clk-6030.c
> + *
> + *  Copyright (C) 2014 Stefan Assmann <sassmann-llIHtaV5axyzQB+pC5nmwQ@public.gmane.org>
> + *
> + * 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.
> + *
> + * Clock driver for ti twl6030.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/clkdev.h>
> +#include <linux/clk-provider.h>
> +#include <linux/i2c/twl.h>
> +#include <linux/platform_device.h>
> +
> +struct twl6030_desc {
> +	struct clk *clk;
> +	struct clk_hw hw;
> +	bool enabled;
> +};
> +
> +#define to_twl6030_desc(_hw) container_of(_hw, struct twl6030_desc, hw)
> +
> +static int twl6030_clk32kg_prepare(struct clk_hw *hw)
> +{
> +	struct twl6030_desc *desc = to_twl6030_desc(hw);
> +	int ret;
> +
> +	ret = twl_i2c_write_u8(TWL_MODULE_PM_RECEIVER,
> +			       TWL6030_GRP_CON << TWL6030_CFG_STATE_GRP_SHIFT |
> +			       TWL6030_CFG_STATE_ON,
> +			       TWL6030_PM_RECEIVER_CLK32KG_CFG_STATE);

What is going to happen if someone ask for this clock before the twl-core is
initialized?


> +	if (ret == 0)
> +		desc->enabled = true;
> +
> +	return ret;
> +}
> +void twl6030_clk32kg_unprepare(struct clk_hw *hw)
> +{
> +	struct twl6030_desc *desc = to_twl6030_desc(hw);
> +
> +	twl_i2c_write_u8(TWL_MODULE_PM_RECEIVER,
> +			 TWL6030_GRP_CON << TWL6030_CFG_STATE_GRP_SHIFT |
> +			 TWL6030_CFG_STATE_OFF,
> +			 TWL6030_PM_RECEIVER_CLK32KG_CFG_STATE);
> +	desc->enabled = false;
> +}
> +
> +static int twl6030_clk32kg_is_prepared(struct clk_hw *hw)
> +{
> +	struct twl6030_desc *desc = to_twl6030_desc(hw);
> +
> +	return desc->enabled;
> +}
> +
> +static const struct clk_ops twl6030_clk32kg_ops = {
> +	.prepare	= twl6030_clk32kg_prepare,
> +	.unprepare	= twl6030_clk32kg_unprepare,
> +	.is_prepared	= twl6030_clk32kg_is_prepared,
> +};
> +
> +static void __init of_ti_twl6030_clk32kg_setup(struct device_node *node)
> +{
> +	struct twl6030_desc *clk_hw = NULL;
> +	struct clk_init_data init = { 0 };
> +	struct clk_lookup *clookup;
> +	struct clk *clk;
> +
> +	clookup = kzalloc(sizeof(*clookup), GFP_KERNEL);
> +	if (!clookup) {
> +		pr_err("%s: could not allocate clookup\n", __func__);
> +		return;
> +	}
> +	clk_hw = kzalloc(sizeof(*clk_hw), GFP_KERNEL);
> +	if (!clk_hw) {
> +		pr_err("%s: could not allocate clk_hw\n", __func__);
> +		goto err_clk_hw;
> +	}
> +
> +	clk_hw->hw.init = &init;
> +
> +	init.name = node->name;
> +	init.ops = &twl6030_clk32kg_ops;
> +	init.flags = CLK_IS_ROOT;
> +
> +	clk = clk_register(NULL, &clk_hw->hw);
> +	if (!IS_ERR(clk)) {
> +		clookup->con_id = kstrdup("clk32kg", GFP_KERNEL);
> +		clookup->clk = clk;
> +		clkdev_add(clookup);
> +
> +		return;
> +	}
> +
> +	kfree(clookup);
> +err_clk_hw:
> +	kfree(clk_hw);
> +}
> +CLK_OF_DECLARE(of_ti_twl6030_clk32kg, "ti,twl6030-clk32kg", of_ti_twl6030_clk32kg_setup);
> +
> +static int of_twl6030_clk32kg_probe(struct platform_device *pdev)
> +{
> +	struct device_node *node = pdev->dev.of_node;
> +	struct clk *clk;
> +	int ret = 0;
> +
> +	if (!node)
> +		return -ENODEV;
> +
> +	clk = clk_get(&pdev->dev, "clk32kg");
> +	if (IS_ERR(clk))
> +		ret = -EPROBE_DEFER;
> +	else
> +		clk_prepare(clk);

Why would you do this? The point of a clock provider is that you can
enable/disable the clock on demand. Here you enable the clock and leave it
enabled for the rest of the time...

clk-dra7-atl deals with similar issue

> +
> +	return ret;
> +}
> +
> +static struct of_device_id of_twl6030_clk32kg_match_tbl[] = {
> +	{ .compatible = "ti,twl6030-clk32kg", },

Hrm, not sure if this is correct. you have "ti,twl6030-clk32kg" as compatible
for the platform driver _and_ you have the same "ti,twl6030-clk32kg"
compatible for the declared clock.
This does not seams right.

I think you should not have compatible for the platform driver at all, rather
you need to create the needed platform device in the twl-core to probe the
driver, then look up for the clock node.

I think in terms of HW setup the Palmas clock is the closest to twl6030's 32K
clocks. It might worth taking a look at that for hints. It is not using
CLK_OF_DECLARE() since it is external chip, but it does the job.


> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, of_twl6030_clk32kg_match_tbl);
> +
> +static struct platform_driver twl6030_clk_driver = {
> +	.driver = {
> +		.name = "twl6030-clk32kg",
> +		.owner = THIS_MODULE,
> +		.of_match_table = of_twl6030_clk32kg_match_tbl,
> +	},
> +	.probe = of_twl6030_clk32kg_probe,
> +};
> +module_platform_driver(twl6030_clk_driver);
> +
> +MODULE_AUTHOR("Stefan Assmann <sassmann-llIHtaV5axyzQB+pC5nmwQ@public.gmane.org>");
> +MODULE_DESCRIPTION("clock driver for TI SoC based boards with twl6030");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
> index db11b4f..440fe4e 100644
> --- a/drivers/mfd/twl-core.c
> +++ b/drivers/mfd/twl-core.c
> @@ -34,6 +34,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  #include <linux/clk.h>
> +#include <linux/clk-provider.h>
>  #include <linux/err.h>
>  #include <linux/device.h>
>  #include <linux/of.h>
> @@ -1012,6 +1013,8 @@ static void clocks_init(struct device *dev,
>  	u32 rate;
>  	u8 ctrl = HFCLK_FREQ_26_MHZ;
>  
> +	of_clk_init(NULL);
> +

I don't think this is in the right place. twl-core should not call a generic
clk init function.

>  	osc = clk_get(dev, "fck");
>  	if (IS_ERR(osc)) {
>  		printk(KERN_WARNING "Skipping twl internal clock init and "
> 


-- 
Péter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] clk: initial clock driver for TWL6030

[Stefan Assmann]

On 31.07.2014 14:26, Peter Ujfalusi wrote:
> On 07/30/2014 05:02 PM, Stefan Assmann wrote:
>> Adding a clock driver for the TI TWL6030. The driver prepares the
>> CLK32KG clock, which is required for the wireless LAN.
>>
>> Signed-off-by: Stefan Assmann <sassmann-llIHtaV5axyzQB+pC5nmwQ@public.gmane.org>
>> ---
>>  .../devicetree/bindings/clock/ti/twl6030.txt       |   4 +
>>  drivers/clk/Kconfig                                |   7 +
>>  drivers/clk/ti/Makefile                            |   1 +
>>  drivers/clk/ti/clk-6030.c                          | 141 +++++++++++++++++++++
>>  drivers/mfd/twl-core.c                             |   3 +
>>  5 files changed, 156 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/clock/ti/twl6030.txt
>>  create mode 100644 drivers/clk/ti/clk-6030.c
>>
>> diff --git a/Documentation/devicetree/bindings/clock/ti/twl6030.txt b/Documentation/devicetree/bindings/clock/ti/twl6030.txt
>> new file mode 100644
>> index 0000000..d290ad4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/ti/twl6030.txt
>> @@ -0,0 +1,4 @@
>> +Binding for TI TWL6030.
>> +
>> +Required properties:
>> +- compatible: "ti,twl6030-clk32kg"
> 
> 
> This is not really helping out. Where would you put this compatible? How it is
> related to twl6030?

Just making a start, I'll see if I can make it more useful.

> 
>> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
>> index 9f9c5ae..4e89e8b 100644
>> --- a/drivers/clk/Kconfig
>> +++ b/drivers/clk/Kconfig
>> @@ -65,6 +65,13 @@ config COMMON_CLK_S2MPS11
>>  	  clock. These multi-function devices have two (S2MPS14) or three
>>  	  (S2MPS11, S5M8767) fixed-rate oscillators, clocked at 32KHz each.
>>  
>> +config CLK_TWL6030
>> +	tristate "Clock driver for twl6030"
>> +	depends on TWL4030_CORE
>> +	---help---
>> +	  Enable the TWL6030 clock CLK32KG which is disabled by default.
>> +	  Needed on the Pandaboard for the wireless LAN.
> 
> This is not relevant information. I'm sure the CLK32KG from twl6030 can be or
> is used for other purposes on other boards.

I'll remove it.

>> +static int twl6030_clk32kg_prepare(struct clk_hw *hw)
>> +{
>> +	struct twl6030_desc *desc = to_twl6030_desc(hw);
>> +	int ret;
>> +
>> +	ret = twl_i2c_write_u8(TWL_MODULE_PM_RECEIVER,
>> +			       TWL6030_GRP_CON << TWL6030_CFG_STATE_GRP_SHIFT |
>> +			       TWL6030_CFG_STATE_ON,
>> +			       TWL6030_PM_RECEIVER_CLK32KG_CFG_STATE);
> 
> What is going to happen if someone ask for this clock before the twl-core is
> initialized?

Originally in of_twl6030_clk32kg_probe() clk_get() would be called and
that would only succeed after of_clk_init() got called in twl_probe().
Tero already pointed out that it should not be done that way.

>> +static int of_twl6030_clk32kg_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *node = pdev->dev.of_node;
>> +	struct clk *clk;
>> +	int ret = 0;
>> +
>> +	if (!node)
>> +		return -ENODEV;
>> +
>> +	clk = clk_get(&pdev->dev, "clk32kg");
>> +	if (IS_ERR(clk))
>> +		ret = -EPROBE_DEFER;
>> +	else
>> +		clk_prepare(clk);
> 
> Why would you do this? The point of a clock provider is that you can
> enable/disable the clock on demand. Here you enable the clock and leave it
> enabled for the rest of the time...
> 
> clk-dra7-atl deals with similar issue

The idea is to enable the clock by default to get the wifi working.
Sorry if I got it wrong.

> 
>> +
>> +	return ret;
>> +}
>> +
>> +static struct of_device_id of_twl6030_clk32kg_match_tbl[] = {
>> +	{ .compatible = "ti,twl6030-clk32kg", },
> 
> Hrm, not sure if this is correct. you have "ti,twl6030-clk32kg" as compatible
> for the platform driver _and_ you have the same "ti,twl6030-clk32kg"
> compatible for the declared clock.
> This does not seams right.
> 
> I think you should not have compatible for the platform driver at all, rather
> you need to create the needed platform device in the twl-core to probe the
> driver, then look up for the clock node.
> 
> I think in terms of HW setup the Palmas clock is the closest to twl6030's 32K
> clocks. It might worth taking a look at that for hints. It is not using
> CLK_OF_DECLARE() since it is external chip, but it does the job.

Thanks for the hint I'll look at that driver for improvements.

>> @@ -1012,6 +1013,8 @@ static void clocks_init(struct device *dev,
>>  	u32 rate;
>>  	u8 ctrl = HFCLK_FREQ_26_MHZ;
>>  
>> +	of_clk_init(NULL);
>> +
> 
> I don't think this is in the right place. twl-core should not call a generic
> clk init function.

Tero pointed that out already. Problem is that the clock doesn't show up
otherwise. An experimental patch from Tero might help, he offered to
post it.

Thanks for the comments Peter.

  Stefan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] clk: initial clock driver for TWL6030

[Peter Ujfalusi]

On 07/31/2014 03:54 PM, Stefan Assmann wrote:
>> Why would you do this? The point of a clock provider is that you can
>> enable/disable the clock on demand. Here you enable the clock and leave it
>> enabled for the rest of the time...
>>
>> clk-dra7-atl deals with similar issue
> 
> The idea is to enable the clock by default to get the wifi working.
> Sorry if I got it wrong.

You should have a clock driver for the 32K clock. The wifi driver should
request and manage it's clocks via the clock API.

-- 
Péter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] clk: initial clock driver for TWL6030

[Stefan Assmann]

On 31.07.2014 14:58, Peter Ujfalusi wrote:
> On 07/31/2014 03:54 PM, Stefan Assmann wrote:
>>> Why would you do this? The point of a clock provider is that you can
>>> enable/disable the clock on demand. Here you enable the clock and leave it
>>> enabled for the rest of the time...
>>>
>>> clk-dra7-atl deals with similar issue
>>
>> The idea is to enable the clock by default to get the wifi working.
>> Sorry if I got it wrong.
> 
> You should have a clock driver for the 32K clock. The wifi driver should
> request and manage it's clocks via the clock API.
> 

If the clock does not get enabled the wifi driver wl12xx doesn't even
get probed. Which is my initial problem. Maybe I need to figure that out
first.

  Stefan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] clk: initial clock driver for TWL6030

[Mike Turquette]

Quoting Stefan Assmann (2014-07-31 07:05:43)
> On 31.07.2014 14:58, Peter Ujfalusi wrote:
> > On 07/31/2014 03:54 PM, Stefan Assmann wrote:
> >>> Why would you do this? The point of a clock provider is that you can
> >>> enable/disable the clock on demand. Here you enable the clock and leave it
> >>> enabled for the rest of the time...
> >>>
> >>> clk-dra7-atl deals with similar issue
> >>
> >> The idea is to enable the clock by default to get the wifi working.
> >> Sorry if I got it wrong.
> > 
> > You should have a clock driver for the 32K clock. The wifi driver should
> > request and manage it's clocks via the clock API.
> > 
> 
> If the clock does not get enabled the wifi driver wl12xx doesn't even
> get probed. Which is my initial problem. Maybe I need to figure that out
> first.

Sounds like the wifi driver's probe is missing something like:

"""
#include <linux/clk.h>

int ret;

struct clk *clk32k = clk_get(...);

if (IS_ERR(clk32k))
        explode();

ret = clk_prepare_enable(clk32k);

if (ret)
        explode();
"""

Regards,
Mike

> 
>   Stefan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] clk: initial clock driver for TWL6030

[Stefan Assmann]

On 31.07.2014 21:20, Mike Turquette wrote:
> Quoting Stefan Assmann (2014-07-31 07:05:43)
>> On 31.07.2014 14:58, Peter Ujfalusi wrote:
>>> On 07/31/2014 03:54 PM, Stefan Assmann wrote:
>>>>> Why would you do this? The point of a clock provider is that you can
>>>>> enable/disable the clock on demand. Here you enable the clock and leave it
>>>>> enabled for the rest of the time...
>>>>>
>>>>> clk-dra7-atl deals with similar issue
>>>>
>>>> The idea is to enable the clock by default to get the wifi working.
>>>> Sorry if I got it wrong.
>>>
>>> You should have a clock driver for the 32K clock. The wifi driver should
>>> request and manage it's clocks via the clock API.
>>>
>>
>> If the clock does not get enabled the wifi driver wl12xx doesn't even
>> get probed. Which is my initial problem. Maybe I need to figure that out
>> first.
> 
> Sounds like the wifi driver's probe is missing something like:

Thanks for the example Mike, but the issue is that the wifi drivers
probe function doesn't even get called without the clock being
powered/enabled. I've instrumented do_one_initcall() to verify this.
With the clock being enabled I see:
[   19.693511] init/main.c do_one_initcall:792 initcall wl1271_init+0x0/0x38 [wlcore_sdio]
[   20.993347] init/main.c do_one_initcall:792 initcall wl12xx_driver_init+0x0/0x14 [wl12xx]
If the clock is kept disabled none of the calls is made and we never get
to wl12xx_probe().
The device might not be discoverable without the clock.

Maybe we should rethink the idea of doing that single register write to
enable the device in twl-core code, if the TWL6030 is present.

Alternatively this could be done by u-boot. Seems like something that
should have been done by firmware upfront.

  Stefan

> 
> """
> #include <linux/clk.h>
> 
> int ret;
> 
> struct clk *clk32k = clk_get(...);
> 
> if (IS_ERR(clk32k))
>         explode();
> 
> ret = clk_prepare_enable(clk32k);
> 
> if (ret)
>         explode();
> """
> 
> Regards,
> Mike
> 
>>
>>   Stefan

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] clk: initial clock driver for TWL6030

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1007 bytes --]

On Fri, Aug 01, 2014 at 12:04:35PM +0200, Stefan Assmann wrote:

> probe function doesn't even get called without the clock being
> powered/enabled. I've instrumented do_one_initcall() to verify this.
> With the clock being enabled I see:
> [   19.693511] init/main.c do_one_initcall:792 initcall wl1271_init+0x0/0x38 [wlcore_sdio]
> [   20.993347] init/main.c do_one_initcall:792 initcall wl12xx_driver_init+0x0/0x14 [wl12xx]
> If the clock is kept disabled none of the calls is made and we never get
> to wl12xx_probe().
> The device might not be discoverable without the clock.

> Maybe we should rethink the idea of doing that single register write to
> enable the device in twl-core code, if the TWL6030 is present.

> Alternatively this could be done by u-boot. Seems like something that
> should have been done by firmware upfront.

If this is a SDIO device please search the lists for discussion on how
to handle enabling required resources for them - Ulf was working on
something generic for that.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]