Enable wifi on pandaboard

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

Signed-off-by Stefan Assmann <[email protected]>

---
 drivers/regulator/twl-regulator.c | 11 -----------
 include/linux/i2c/twl.h           | 20 ++++++++++++++++++++
 2 files changed, 20 insertions(+), 11 deletions(-)

Quoted 3 lines · Stefan Assmann

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

Quoted 5 lines

Signed-off-by Stefan Assmann <[email protected]>

---
 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?

drivers/clk/ti/clk-6030.c new file +0 -0

new file mode 100644
index 0000000..baa965b

drivers/clk/ti/clk-6030.c new file +3 -0

+ *
+ * Clock driver for ti twl6030.
+ */

Hi Stefan,

Glad you got it working. :)

Small nit below:

Am 30.07.2014 16:02, schrieb Stefan Assmann:

Quoted 6 lines

drivers/clk/ti/clk-6030.c new file +0 -0

new file mode 100644
index 0000000..baa965b

drivers/clk/ti/clk-6030.c line 8

Quoted 8 lines

drivers/clk/ti/clk-6030.c new file +8 -0

@@ -0,0 +1,141 @@
+/*
+ * drivers/clk/ti/clk-6030.c
+ *
+ *  Copyright (C) 2014 Stefan Assmann <[email protected]>
+ *
+ * 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 ...

drivers/clk/ti/clk-6030.c line 11

Quoted 3 lines

drivers/clk/ti/clk-6030.c new file +3 -0

+ *
+ * Clock driver for ti twl6030.
+ */

[...]

drivers/clk/ti/clk-6030.c line 141

Quoted 3 lines

drivers/clk/ti/clk-6030.c new file +3 -0

+MODULE_AUTHOR("Stefan Assmann <[email protected]>");
+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.

Review

Quoted 6 lines · Stefan Assmann

(quoted diff) +6 -0

+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.

Review

Quoted 11 lines

(quoted diff) +10 -0

+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()?

Review

Quoted 3 lines

(quoted diff) +2 -0

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

Shouldn't the provided return code be being used?

Review

Quoted 3 lines

(quoted diff) +2 -0

+	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.

From 2c52040f33af9dbb41e3e3f355ae154843b277c6 Mon Sep 17 00:00:00 2001

Quoted 11 lines · Mark Brown

drivers/clk/ti/clk-6030.c line 53

Quoted 6 lines · Stefan Assmann

drivers/clk/ti/clk-6030.c new file +6 -0

+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.

Quoted 7 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+	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.

Quoted 12 lines · Stefan Assmann

Quoted 11 lines · Mark Brown

drivers/clk/ti/clk-6030.c line 53

Quoted 6 lines · Stefan Assmann

drivers/clk/ti/clk-6030.c new file +6 -0

+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.

Quoted 2 lines

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.

Quoted 9 lines

Quoted 8 lines

Review

Quoted 3 lines

(quoted diff) +2 -0

+	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.

Quoted 4 lines

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.

Quoted 14 lines · Stefan Assmann

Quoted 12 lines · Mark Brown

Quoted 11 lines · Stefan Assmann

drivers/clk/ti/clk-6030.c line 53

Quoted 7 lines

drivers/clk/ti/clk-6030.c new file +6 -0

+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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 18 lines

Quoted 9 lines

Quoted 8 lines

Review

Quoted 3 lines

(quoted diff) +2 -0

+  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.

Quoted 4 lines

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().

Quoted 2 lines

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

drivers/clk/ti/clk-6030.c new file +91 -0

+
+#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;

drivers/clk/ti/clk-6030.c new file +21 -0

+}
+
+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 <[email protected]>");
+MODULE_DESCRIPTION("clock driver for TI SoC based boards with twl6030");
+MODULE_LICENSE("GPL v2");

drivers/mfd/twl-core.c +1 -0

index db11b4f..440fe4e 100644
@@ -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>

drivers/mfd/twl-core.c +0 -0

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

drivers/clk/ti/clk-6030.c

Quoted 5 lines

drivers/clk/ti/clk-6030.c new file +0 -0

new file mode 100644
index 0000000..61d1834

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

drivers/clk/ti/clk-6030.c line 2

Quoted 4 lines

drivers/clk/ti/clk-6030.c new file +2 -0

@@ -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.

drivers/clk/ti/clk-6030.c line 11

Quoted 10 lines

drivers/clk/ti/clk-6030.c new file +9 -0

+ *
+ *  Copyright (C) 2014 Stefan Assmann <[email protected]>
+ *
+ * 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.

Quoted 8 lines

drivers/clk/ti/clk-6030.c new file +8 -0

+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;
+

drivers/clk/ti/clk-6030.c line 112

Quoted 5 lines

drivers/clk/ti/clk-6030.c new file +5 -0

+	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.

Quoted 16 lines

drivers/mfd/twl-core.c +1 -0

index db11b4f..440fe4e 100644
@@ -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;
 

drivers/mfd/twl-core.c static void clocks_init(struct device *dev, · line 1017

Quoted 2 lines

drivers/mfd/twl-core.c +2 -0

+	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.

Quoted 8 lines

Quoted 8 lines

drivers/clk/ti/clk-6030.c new file +8 -0

+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;
+

Quoted 8 lines

drivers/clk/ti/clk-6030.c line 112

Quoted 5 lines

drivers/clk/ti/clk-6030.c new file +5 -0

+	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().

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 <[email protected]>

---
 .../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

Documentation/devicetree/bindings/clock/ti/twl6030.txt new file +0 -0

new file mode 100644
index 0000000..d290ad4

drivers/clk/Kconfig +0 -0

index 9f9c5ae..4e89e8b 100644

drivers/clk/Kconfig +1 -0

+
 config CLK_TWL6040
 	tristate "External McPDM functional clock from twl6040"
 	depends on TWL6040_CORE

drivers/clk/ti/Makefile +1 -0

index ed4d0aa..04f25ea 100644
@@ -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

drivers/clk/ti/clk-6030.c new file +26 -0

new file mode 100644
index 0000000..baa965b
@@ -0,0 +1,141 @@
+/*
+ * drivers/clk/ti/clk-6030.c
+ *
+ *  Copyright (C) 2014 Stefan Assmann <[email protected]>
+ *
+ * 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)

drivers/clk/ti/clk-6030.c new file +72 -0

+	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;

drivers/clk/ti/clk-6030.c new file +17 -0

+	{},
+};
+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 <[email protected]>");
+MODULE_DESCRIPTION("clock driver for TI SoC based boards with twl6030");
+MODULE_LICENSE("GPL");

drivers/mfd/twl-core.c +1 -0

index db11b4f..440fe4e 100644
@@ -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>

drivers/mfd/twl-core.c +0 -0

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

Quoted 14 lines · 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 <[email protected]>

---
 .../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

Documentation/devicetree/bindings/clock/ti/twl6030.txt line 4

Quoted 10 lines

Documentation/devicetree/bindings/clock/ti/twl6030.txt new file +4 -0

new file mode 100644
index 0000000..d290ad4
@@ -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?

Quoted 9 lines

drivers/clk/Kconfig +0 -0

index 9f9c5ae..4e89e8b 100644
@@ -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.
 

drivers/clk/Kconfig config COMMON_CLK_S2MPS11 · line 73

Quoted 6 lines

drivers/clk/Kconfig +6 -0

+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.

Quoted 4 lines

drivers/clk/Kconfig +4 -0

+static int twl6030_clk32kg_prepare(struct clk_hw *hw)
+{
+	struct twl6030_desc *desc = to_twl6030_desc(hw);
+	int ret;

drivers/clk/Kconfig config COMMON_CLK_S2MPS11

Quoted 5 lines

drivers/clk/Kconfig +5 -0

+
+	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?

Quoted 4 lines

drivers/clk/Kconfig +4 -0

+static int of_twl6030_clk32kg_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	struct clk *clk;

drivers/clk/Kconfig

Quoted 10 lines

drivers/clk/Kconfig +10 -0

+	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

drivers/clk/Kconfig

Quoted 7 lines

drivers/clk/Kconfig +6 -0

+
+	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.

drivers/clk/Kconfig static void clocks_init(struct device *dev, · line 1017

Quoted 6 lines

drivers/clk/Kconfig +2 -0

@@ -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.

Quoted 5 lines

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.

Quoted 4 lines · Stefan Assmann

Quoted 4 lines

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...

Quoted 4 lines

Quoted 1 line

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.

Quoted 10 lines · Stefan Assmann

Quoted 8 lines

Quoted 5 lines

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.

Quoted 3 lines

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

Quoting Stefan Assmann (2014-07-31 07:05:43)

Quoted 15 lines · Peter Ujfalusi

Quoted 13 lines · Stefan Assmann

Quoted 8 lines

Quoted 5 lines

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.

Quoted 3 lines

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

Quoted 2 lines

Stefan

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.