[PATCH v2 0/5] Fix Arndale Octa/Peach Pi boot on Audio subsystem clocks

[PATCH v2 0/5] Fix Arndale Octa/Peach Pi boot on Audio subsystem clocks

[Krzysztof Kozlowski]

Hi,

Changes since v1
================
1. clocks-audss: Reimplement own clock register functions instead
   changing clk API. Minor fixes. (after idea from Tomasz Figa)
2. Add new patches: fix for pinctrl and minor fixes in clk-audss.

Description
===========
This patchset tries to solve dependency between AudioSS components
(clocks and GPIO) and main clock controller on Exynos platform.

This solves boot failure of Peach Pi/Pit and Arndale Octa [1].

Any access to memory of audss block (like checking if clock is enabled
or configuring GPIO) will hang if main audss clock is gated.

Tested on Arndale Octa board.

[1] http://www.spinics.net/lists/linux-samsung-soc/msg39331.html

Best regards,
Krzysztof Kozlowski


Krzysztof Kozlowski (3):
  clk: samsung: Fix clock disable failure because domain being gated
  pinctrl: exynos: Fix GPIO setup failure because domain clock being
    gated
  ARM: dts: exynos5420: Add clock for audss pinctrl

 .../bindings/pinctrl/samsung-pinctrl.txt           |   6 +
 arch/arm/boot/dts/exynos5420-pinctrl.dtsi          |   3 +
 drivers/clk/samsung/clk-exynos-audss.c             | 367 +++++++++++++++++++--
 drivers/pinctrl/samsung/pinctrl-samsung.c          | 110 +++++-
 drivers/pinctrl/samsung/pinctrl-samsung.h          |   2 +
 5 files changed, 446 insertions(+), 42 deletions(-)

-- 
1.9.1

[PATCH v2 0/5] Fix Arndale Octa/Peach Pi boot on Audio subsystem clocks

[Krzysztof Kozlowski]

Hi,

Changes since v1
================
1. clocks-audss: Reimplement own clock register functions instead
   changing clk API. Minor fixes. (after idea from Tomasz Figa)
2. Add new patches: fix for pinctrl and minor fixes in clk-audss.

Description
===========
This patchset tries to solve dependency between AudioSS components
(clocks and GPIO) and main clock controller on Exynos platform.

This solves boot failure of Peach Pi/Pit and Arndale Octa [1].

Any access to memory of audss block (like checking if clock is enabled
or configuring GPIO) will hang if main audss clock is gated.

Tested on Arndale Octa board.

[1] http://www.spinics.net/lists/linux-samsung-soc/msg39331.html

Best regards,
Krzysztof Kozlowski


Krzysztof Kozlowski (3):
  clk: samsung: Fix clock disable failure because domain being gated
  pinctrl: exynos: Fix GPIO setup failure because domain clock being
    gated
  ARM: dts: exynos5420: Add clock for audss pinctrl

 .../bindings/pinctrl/samsung-pinctrl.txt           |   6 +
 arch/arm/boot/dts/exynos5420-pinctrl.dtsi          |   3 +
 drivers/clk/samsung/clk-exynos-audss.c             | 367 +++++++++++++++++++--
 drivers/pinctrl/samsung/pinctrl-samsung.c          | 110 +++++-
 drivers/pinctrl/samsung/pinctrl-samsung.h          |   2 +
 5 files changed, 446 insertions(+), 42 deletions(-)

-- 
1.9.1

[PATCH v2 1/5] clk: samsung: Fix double add of syscore ops after driver rebind

[Krzysztof Kozlowski]

During driver unbind the syscore ops were not unregistered which lead to
double add on syscore list:

$ echo "3810000.audss-clock-controller" > /sys/bus/platform/drivers/exynos-audss-clk/unbind
$ echo "3810000.audss-clock-controller" > /sys/bus/platform/drivers/exynos-audss-clk/bind
[ 1463.044061] ------------[ cut here ]------------
[ 1463.047255] WARNING: CPU: 0 PID: 1 at lib/list_debug.c:36 __list_add+0x8c/0xc0()
[ 1463.054613] list_add double add: new=c06e52c0, prev=c06e52c0, next=c06d5f84.
[ 1463.061625] Modules linked in:
[ 1463.064623] CPU: 0 PID: 1 Comm: bash Tainted: G        W      3.18.0-rc5-next-20141121-00005-ga8fab06eab42-dirty #1022
[ 1463.075338] [<c0014e2c>] (unwind_backtrace) from [<c0011d80>] (show_stack+0x10/0x14)
[ 1463.083046] [<c0011d80>] (show_stack) from [<c048bb70>] (dump_stack+0x70/0xbc)
[ 1463.090236] [<c048bb70>] (dump_stack) from [<c00233d4>] (warn_slowpath_common+0x74/0xb0)
[ 1463.098295] [<c00233d4>] (warn_slowpath_common) from [<c00234a4>] (warn_slowpath_fmt+0x30/0x40)
[ 1463.106962] [<c00234a4>] (warn_slowpath_fmt) from [<c020fe80>] (__list_add+0x8c/0xc0)
[ 1463.114760] [<c020fe80>] (__list_add) from [<c0282094>] (register_syscore_ops+0x30/0x3c)
[ 1463.122819] [<c0282094>] (register_syscore_ops) from [<c0392f20>] (exynos_audss_clk_probe+0x36c/0x460)
[ 1463.132091] [<c0392f20>] (exynos_audss_clk_probe) from [<c0283084>] (platform_drv_probe+0x48/0xa4)
[ 1463.141013] [<c0283084>] (platform_drv_probe) from [<c0281a14>] (driver_probe_device+0x13c/0x37c)
[ 1463.149852] [<c0281a14>] (driver_probe_device) from [<c0280560>] (bind_store+0x90/0xe0)
[ 1463.157822] [<c0280560>] (bind_store) from [<c027fd10>] (drv_attr_store+0x20/0x2c)
[ 1463.165363] [<c027fd10>] (drv_attr_store) from [<c0143898>] (sysfs_kf_write+0x4c/0x50)
[ 1463.173252] [<c0143898>] (sysfs_kf_write) from [<c0142c80>] (kernfs_fop_write+0xbc/0x198)
[ 1463.181395] [<c0142c80>] (kernfs_fop_write) from [<c00e2be0>] (vfs_write+0xa0/0x1a8)
[ 1463.189104] [<c00e2be0>] (vfs_write) from [<c00e2f00>] (SyS_write+0x40/0x8c)
[ 1463.196122] [<c00e2f00>] (SyS_write) from [<c000f2a0>] (ret_fast_syscall+0x0/0x48)
[ 1463.203655] ---[ end trace 08f6710c9bc8d8f3 ]---
[ 1463.208244] exynos-audss-clk 3810000.audss-clock-controller: setup completed

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Fixes: 1241ef94ccc3 ("clk: samsung: register audio subsystem clocks using common clock framework")
Cc: <stable@vger.kernel.org>
---
 drivers/clk/samsung/clk-exynos-audss.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/clk/samsung/clk-exynos-audss.c b/drivers/clk/samsung/clk-exynos-audss.c
index acce708ace18..7c4368e75ede 100644
--- a/drivers/clk/samsung/clk-exynos-audss.c
+++ b/drivers/clk/samsung/clk-exynos-audss.c
@@ -210,6 +210,10 @@ static int exynos_audss_clk_remove(struct platform_device *pdev)
 {
 	int i;
 
+#ifdef CONFIG_PM_SLEEP
+	unregister_syscore_ops(&exynos_audss_clk_syscore_ops);
+#endif
+
 	of_clk_del_provider(pdev->dev.of_node);
 
 	for (i = 0; i < clk_data.clk_num; i++) {
-- 
1.9.1

[PATCH v2 1/5] clk: samsung: Fix double add of syscore ops after driver rebind

[Krzysztof Kozlowski]

During driver unbind the syscore ops were not unregistered which lead to
double add on syscore list:

$ echo "3810000.audss-clock-controller" > /sys/bus/platform/drivers/exynos-audss-clk/unbind
$ echo "3810000.audss-clock-controller" > /sys/bus/platform/drivers/exynos-audss-clk/bind
[ 1463.044061] ------------[ cut here ]------------
[ 1463.047255] WARNING: CPU: 0 PID: 1 at lib/list_debug.c:36 __list_add+0x8c/0xc0()
[ 1463.054613] list_add double add: new=c06e52c0, prev=c06e52c0, next=c06d5f84.
[ 1463.061625] Modules linked in:
[ 1463.064623] CPU: 0 PID: 1 Comm: bash Tainted: G        W      3.18.0-rc5-next-20141121-00005-ga8fab06eab42-dirty #1022
[ 1463.075338] [<c0014e2c>] (unwind_backtrace) from [<c0011d80>] (show_stack+0x10/0x14)
[ 1463.083046] [<c0011d80>] (show_stack) from [<c048bb70>] (dump_stack+0x70/0xbc)
[ 1463.090236] [<c048bb70>] (dump_stack) from [<c00233d4>] (warn_slowpath_common+0x74/0xb0)
[ 1463.098295] [<c00233d4>] (warn_slowpath_common) from [<c00234a4>] (warn_slowpath_fmt+0x30/0x40)
[ 1463.106962] [<c00234a4>] (warn_slowpath_fmt) from [<c020fe80>] (__list_add+0x8c/0xc0)
[ 1463.114760] [<c020fe80>] (__list_add) from [<c0282094>] (register_syscore_ops+0x30/0x3c)
[ 1463.122819] [<c0282094>] (register_syscore_ops) from [<c0392f20>] (exynos_audss_clk_probe+0x36c/0x460)
[ 1463.132091] [<c0392f20>] (exynos_audss_clk_probe) from [<c0283084>] (platform_drv_probe+0x48/0xa4)
[ 1463.141013] [<c0283084>] (platform_drv_probe) from [<c0281a14>] (driver_probe_device+0x13c/0x37c)
[ 1463.149852] [<c0281a14>] (driver_probe_device) from [<c0280560>] (bind_store+0x90/0xe0)
[ 1463.157822] [<c0280560>] (bind_store) from [<c027fd10>] (drv_attr_store+0x20/0x2c)
[ 1463.165363] [<c027fd10>] (drv_attr_store) from [<c0143898>] (sysfs_kf_write+0x4c/0x50)
[ 1463.173252] [<c0143898>] (sysfs_kf_write) from [<c0142c80>] (kernfs_fop_write+0xbc/0x198)
[ 1463.181395] [<c0142c80>] (kernfs_fop_write) from [<c00e2be0>] (vfs_write+0xa0/0x1a8)
[ 1463.189104] [<c00e2be0>] (vfs_write) from [<c00e2f00>] (SyS_write+0x40/0x8c)
[ 1463.196122] [<c00e2f00>] (SyS_write) from [<c000f2a0>] (ret_fast_syscall+0x0/0x48)
[ 1463.203655] ---[ end trace 08f6710c9bc8d8f3 ]---
[ 1463.208244] exynos-audss-clk 3810000.audss-clock-controller: setup completed

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Fixes: 1241ef94ccc3 ("clk: samsung: register audio subsystem clocks using common clock framework")
Cc: <stable@vger.kernel.org>
---
 drivers/clk/samsung/clk-exynos-audss.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/clk/samsung/clk-exynos-audss.c b/drivers/clk/samsung/clk-exynos-audss.c
index acce708ace18..7c4368e75ede 100644
--- a/drivers/clk/samsung/clk-exynos-audss.c
+++ b/drivers/clk/samsung/clk-exynos-audss.c
@@ -210,6 +210,10 @@ static int exynos_audss_clk_remove(struct platform_device *pdev)
 {
 	int i;
 
+#ifdef CONFIG_PM_SLEEP
+	unregister_syscore_ops(&exynos_audss_clk_syscore_ops);
+#endif
+
 	of_clk_del_provider(pdev->dev.of_node);
 
 	for (i = 0; i < clk_data.clk_num; i++) {
-- 
1.9.1

[PATCH v2 1/5] clk: samsung: Fix double add of syscore ops after driver rebind

[Krzysztof Kozlowski]

During driver unbind the syscore ops were not unregistered which lead to
double add on syscore list:

$ echo "3810000.audss-clock-controller" > /sys/bus/platform/drivers/exynos-audss-clk/unbind
$ echo "3810000.audss-clock-controller" > /sys/bus/platform/drivers/exynos-audss-clk/bind
[ 1463.044061] ------------[ cut here ]------------
[ 1463.047255] WARNING: CPU: 0 PID: 1 at lib/list_debug.c:36 __list_add+0x8c/0xc0()
[ 1463.054613] list_add double add: new=c06e52c0, prev=c06e52c0, next=c06d5f84.
[ 1463.061625] Modules linked in:
[ 1463.064623] CPU: 0 PID: 1 Comm: bash Tainted: G        W      3.18.0-rc5-next-20141121-00005-ga8fab06eab42-dirty #1022
[ 1463.075338] [<c0014e2c>] (unwind_backtrace) from [<c0011d80>] (show_stack+0x10/0x14)
[ 1463.083046] [<c0011d80>] (show_stack) from [<c048bb70>] (dump_stack+0x70/0xbc)
[ 1463.090236] [<c048bb70>] (dump_stack) from [<c00233d4>] (warn_slowpath_common+0x74/0xb0)
[ 1463.098295] [<c00233d4>] (warn_slowpath_common) from [<c00234a4>] (warn_slowpath_fmt+0x30/0x40)
[ 1463.106962] [<c00234a4>] (warn_slowpath_fmt) from [<c020fe80>] (__list_add+0x8c/0xc0)
[ 1463.114760] [<c020fe80>] (__list_add) from [<c0282094>] (register_syscore_ops+0x30/0x3c)
[ 1463.122819] [<c0282094>] (register_syscore_ops) from [<c0392f20>] (exynos_audss_clk_probe+0x36c/0x460)
[ 1463.132091] [<c0392f20>] (exynos_audss_clk_probe) from [<c0283084>] (platform_drv_probe+0x48/0xa4)
[ 1463.141013] [<c0283084>] (platform_drv_probe) from [<c0281a14>] (driver_probe_device+0x13c/0x37c)
[ 1463.149852] [<c0281a14>] (driver_probe_device) from [<c0280560>] (bind_store+0x90/0xe0)
[ 1463.157822] [<c0280560>] (bind_store) from [<c027fd10>] (drv_attr_store+0x20/0x2c)
[ 1463.165363] [<c027fd10>] (drv_attr_store) from [<c0143898>] (sysfs_kf_write+0x4c/0x50)
[ 1463.173252] [<c0143898>] (sysfs_kf_write) from [<c0142c80>] (kernfs_fop_write+0xbc/0x198)
[ 1463.181395] [<c0142c80>] (kernfs_fop_write) from [<c00e2be0>] (vfs_write+0xa0/0x1a8)
[ 1463.189104] [<c00e2be0>] (vfs_write) from [<c00e2f00>] (SyS_write+0x40/0x8c)
[ 1463.196122] [<c00e2f00>] (SyS_write) from [<c000f2a0>] (ret_fast_syscall+0x0/0x48)
[ 1463.203655] ---[ end trace 08f6710c9bc8d8f3 ]---
[ 1463.208244] exynos-audss-clk 3810000.audss-clock-controller: setup completed

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Fixes: 1241ef94ccc3 ("clk: samsung: register audio subsystem clocks using common clock framework")
Cc: <stable@vger.kernel.org>
---
 drivers/clk/samsung/clk-exynos-audss.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/clk/samsung/clk-exynos-audss.c b/drivers/clk/samsung/clk-exynos-audss.c
index acce708ace18..7c4368e75ede 100644
--- a/drivers/clk/samsung/clk-exynos-audss.c
+++ b/drivers/clk/samsung/clk-exynos-audss.c
@@ -210,6 +210,10 @@ static int exynos_audss_clk_remove(struct platform_device *pdev)
 {
 	int i;
 
+#ifdef CONFIG_PM_SLEEP
+	unregister_syscore_ops(&exynos_audss_clk_syscore_ops);
+#endif
+
 	of_clk_del_provider(pdev->dev.of_node);
 
 	for (i = 0; i < clk_data.clk_num; i++) {
-- 
1.9.1

[PATCH v2 2/5] clk: samsung: Fix clock disable failure because domain being gated

[Krzysztof Kozlowski]

Audio subsystem clocks are located in separate block. If clock for this
block (from main clock domain) 'mau_epll' is gated then any read or
write to audss registers will block.

This was observed on Exynos 5420 platforms (Arndale Octa and Peach
Pi/Pit) after introducing runtime PM to pl330 DMA driver. After that
commit the 'mau_epll' was gated, because the "amba" clock was disabled
and there were no more users of mau_epll. The system hang on disabling
unused clocks from audss block.

Unfortunately the 'mau_epll' clock is not parent of some of audss clocks.

Whenever system wants to operate on audss clocks it has to enable epll
clock. The solution reuses common clk-gate/divider/mux code and duplicates
clk_register_*() functions. In the same time the patch tries to limit
functional changes of the driver so it does not fix minor issues with existing
code (like leaking memory allocated for clk-gate/clk-mux/clk-divider code).
This is addressed later.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Reported-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Reported-by: Kevin Hilman <khilman@kernel.org>
---
 drivers/clk/samsung/clk-exynos-audss.c | 346 +++++++++++++++++++++++++++++----
 1 file changed, 311 insertions(+), 35 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos-audss.c b/drivers/clk/samsung/clk-exynos-audss.c
index 7c4368e75ede..9ec7de866ab4 100644
--- a/drivers/clk/samsung/clk-exynos-audss.c
+++ b/drivers/clk/samsung/clk-exynos-audss.c
@@ -29,6 +29,7 @@ static DEFINE_SPINLOCK(lock);
 static struct clk **clk_table;
 static void __iomem *reg_base;
 static struct clk_onecell_data clk_data;
+static struct clk *pll_in;
 
 #define ASS_CLK_SRC 0x0
 #define ASS_CLK_DIV 0x4
@@ -75,6 +76,276 @@ static const struct of_device_id exynos_audss_clk_of_match[] = {
 	{},
 };
 
+static int pll_clk_enable(void)
+{
+	if (!IS_ERR(pll_in))
+		return clk_enable(pll_in);
+
+	return 0;
+}
+
+static void pll_clk_disable(void)
+{
+	if (!IS_ERR(pll_in))
+		clk_disable(pll_in);
+}
+
+static int audss_clk_gate_enable(struct clk_hw *hw)
+{
+	int ret;
+
+	ret = pll_clk_enable();
+	if (ret)
+		return ret;
+
+	ret = clk_gate_ops.enable(hw);
+
+	pll_clk_disable();
+
+	return ret;
+}
+
+static void audss_clk_gate_disable(struct clk_hw *hw)
+{
+	int ret;
+
+	ret = pll_clk_enable();
+	if (ret)
+		return;
+
+	clk_gate_ops.disable(hw);
+
+	pll_clk_disable();
+}
+
+static int audss_clk_gate_is_enabled(struct clk_hw *hw)
+{
+	int ret;
+
+	ret = pll_clk_enable();
+	if (ret)
+		return ret;
+
+	ret = clk_gate_ops.is_enabled(hw);
+
+	pll_clk_disable();
+
+	return ret;
+}
+
+static const struct clk_ops audss_clk_gate_ops = {
+	.enable = audss_clk_gate_enable,
+	.disable = audss_clk_gate_disable,
+	.is_enabled = audss_clk_gate_is_enabled,
+};
+
+/*
+ * A simplified copy of clk-gate.c:clk_register_gate() to mimic
+ * clk-gate behavior while using customized ops.
+ *
+ * TODO: just like clk-gate it leaks memory for struct clk_gate.
+ */
+static struct clk *audss_clk_register_gate(struct device *dev, const char *name,
+		const char *parent_name, unsigned long flags, u8 bit_idx)
+{
+	struct clk_gate *gate;
+	struct clk *clk;
+	struct clk_init_data init;
+
+	/* allocate the gate */
+	gate = kzalloc(sizeof(struct clk_gate), GFP_KERNEL);
+	if (!gate)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = name;
+	init.ops = &audss_clk_gate_ops;
+	init.flags = flags | CLK_IS_BASIC;
+	init.parent_names = (parent_name ? &parent_name : NULL);
+	init.num_parents = (parent_name ? 1 : 0);
+
+	/* struct clk_gate assignments */
+	gate->reg = reg_base + ASS_CLK_GATE;
+	gate->bit_idx = bit_idx;
+	gate->flags = 0;
+	gate->lock = &lock;
+	gate->hw.init = &init;
+
+	clk = clk_register(dev, &gate->hw);
+
+	if (IS_ERR(clk))
+		kfree(gate);
+
+	return clk;
+}
+
+static unsigned long audss_clk_divider_recalc_rate(struct clk_hw *hw,
+		unsigned long parent_rate)
+{
+	unsigned long ret;
+
+	ret = pll_clk_enable();
+	if (ret) {
+		WARN(ret, "Could not enable pll_in clock\n");
+		return parent_rate;
+	}
+
+	ret = clk_divider_ops.recalc_rate(hw, parent_rate);
+
+	pll_clk_disable();
+
+	return ret;
+}
+
+static long audss_clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
+				unsigned long *prate)
+{
+	return clk_divider_ops.round_rate(hw, rate, prate);
+}
+
+static int audss_clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
+				unsigned long parent_rate)
+{
+	int ret;
+
+	ret = pll_clk_enable();
+	if (ret)
+		return ret;
+
+	ret = clk_divider_ops.set_rate(hw, rate, parent_rate);
+
+	pll_clk_disable();
+
+	return ret;
+}
+
+static const struct clk_ops audss_clk_divider_ops = {
+	.recalc_rate = audss_clk_divider_recalc_rate,
+	.round_rate = audss_clk_divider_round_rate,
+	.set_rate = audss_clk_divider_set_rate,
+};
+
+/*
+ * A simplified copy of clk-divider.c:clk_register_divider() to mimic
+ * clk-divider behavior while using customized ops.
+ */
+static struct clk *audss_clk_register_divider(struct device *dev,
+		const char *name,
+		const char *parent_name, unsigned long flags,
+		u8 shift, u8 width)
+{
+	struct clk_divider *div;
+	struct clk *clk;
+	struct clk_init_data init;
+
+	/* allocate the divider */
+	div = kzalloc(sizeof(struct clk_divider), GFP_KERNEL);
+	if (!div)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = name;
+	init.ops = &audss_clk_divider_ops;
+	init.flags = flags | CLK_IS_BASIC;
+	init.parent_names = (parent_name ? &parent_name : NULL);
+	init.num_parents = (parent_name ? 1 : 0);
+
+	/* struct clk_divider assignments */
+	div->reg = reg_base + ASS_CLK_DIV;
+	div->shift = shift;
+	div->width = width;
+	div->flags = 0;
+	div->lock = &lock;
+	div->hw.init = &init;
+	div->table = NULL;
+
+	/* register the clock */
+	clk = clk_register(dev, &div->hw);
+
+	if (IS_ERR(clk))
+		kfree(div);
+
+	return clk;
+}
+
+static u8 audss_clk_mux_get_parent(struct clk_hw *hw)
+{
+	u8 parent;
+	int ret;
+
+	ret = pll_clk_enable();
+	if (ret) {
+		WARN(ret, "Could not enable pll_in clock\n");
+		return -EINVAL; /* Just like clk_mux_get_parent() */
+	}
+
+	parent = clk_mux_ops.get_parent(hw);
+
+	pll_clk_disable();
+
+	return parent;
+}
+
+static int audss_clk_mux_set_parent(struct clk_hw *hw, u8 index)
+{
+	int ret;
+
+	ret = pll_clk_enable();
+	if (ret)
+		return ret;
+
+	ret = clk_mux_ops.set_parent(hw, index);
+
+	pll_clk_disable();
+
+	return ret;
+}
+
+static const struct clk_ops audss_clk_mux_ops = {
+	.get_parent = audss_clk_mux_get_parent,
+	.set_parent = audss_clk_mux_set_parent,
+	.determine_rate = __clk_mux_determine_rate,
+};
+
+/*
+ * A simplified copy of clk-mux.c:clk_register_mux_table() to mimic
+ * clk-mux behavior while using customized ops.
+ */
+static struct clk *audss_clk_register_mux(struct device *dev, const char *name,
+		const char **parent_names, u8 num_parents, unsigned long flags,
+		u8 shift, u8 width)
+{
+	struct clk_mux *mux;
+	struct clk *clk;
+	struct clk_init_data init;
+	u32 mask = BIT(width) - 1;
+
+	/* allocate the mux */
+	mux = kzalloc(sizeof(struct clk_mux), GFP_KERNEL);
+	if (!mux)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = name;
+	init.ops = &audss_clk_mux_ops;
+	init.flags = flags | CLK_IS_BASIC;
+	init.parent_names = parent_names;
+	init.num_parents = num_parents;
+
+	/* struct clk_mux assignments */
+	mux->reg = reg_base + ASS_CLK_SRC;
+	mux->shift = shift;
+	mux->mask = mask;
+	mux->flags = 0;
+	mux->lock = &lock;
+	mux->table = NULL;
+	mux->hw.init = &init;
+
+	clk = clk_register(dev, &mux->hw);
+
+	if (IS_ERR(clk))
+		kfree(mux);
+
+	return clk;
+}
+
 /* register exynos_audss clocks */
 static int exynos_audss_clk_probe(struct platform_device *pdev)
 {
@@ -83,7 +354,7 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
 	const char *mout_audss_p[] = {"fin_pll", "fout_epll"};
 	const char *mout_i2s_p[] = {"mout_audss", "cdclk0", "sclk_audio0"};
 	const char *sclk_pcm_p = "sclk_pcm0";
-	struct clk *pll_ref, *pll_in, *cdclk, *sclk_audio, *sclk_pcm_in;
+	struct clk *pll_ref, *cdclk, *sclk_audio, *sclk_pcm_in;
 	const struct of_device_id *match;
 	enum exynos_audss_clk_type variant;
 
@@ -115,12 +386,21 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
 	pll_in = devm_clk_get(&pdev->dev, "pll_in");
 	if (!IS_ERR(pll_ref))
 		mout_audss_p[0] = __clk_get_name(pll_ref);
-	if (!IS_ERR(pll_in))
+	if (!IS_ERR(pll_in)) {
 		mout_audss_p[1] = __clk_get_name(pll_in);
-	clk_table[EXYNOS_MOUT_AUDSS] = clk_register_mux(NULL, "mout_audss",
+
+		ret = clk_prepare(pll_in);
+		if (ret) {
+			dev_err(&pdev->dev,
+				"failed to prepare the pll_in clock\n");
+			return ret;
+		}
+
+	}
+
+	clk_table[EXYNOS_MOUT_AUDSS] = audss_clk_register_mux(NULL, "mout_audss",
 				mout_audss_p, ARRAY_SIZE(mout_audss_p),
-				CLK_SET_RATE_NO_REPARENT,
-				reg_base + ASS_CLK_SRC, 0, 1, 0, &lock);
+				CLK_SET_RATE_NO_REPARENT, 0, 1);
 
 	cdclk = devm_clk_get(&pdev->dev, "cdclk");
 	sclk_audio = devm_clk_get(&pdev->dev, "sclk_audio");
@@ -128,50 +408,40 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
 		mout_i2s_p[1] = __clk_get_name(cdclk);
 	if (!IS_ERR(sclk_audio))
 		mout_i2s_p[2] = __clk_get_name(sclk_audio);
-	clk_table[EXYNOS_MOUT_I2S] = clk_register_mux(NULL, "mout_i2s",
+	clk_table[EXYNOS_MOUT_I2S] = audss_clk_register_mux(NULL, "mout_i2s",
 				mout_i2s_p, ARRAY_SIZE(mout_i2s_p),
-				CLK_SET_RATE_NO_REPARENT,
-				reg_base + ASS_CLK_SRC, 2, 2, 0, &lock);
+				CLK_SET_RATE_NO_REPARENT, 2, 2);
 
-	clk_table[EXYNOS_DOUT_SRP] = clk_register_divider(NULL, "dout_srp",
-				"mout_audss", 0, reg_base + ASS_CLK_DIV, 0, 4,
-				0, &lock);
+	clk_table[EXYNOS_DOUT_SRP] = audss_clk_register_divider(NULL, "dout_srp",
+				"mout_audss", 0, 0, 4);
 
-	clk_table[EXYNOS_DOUT_AUD_BUS] = clk_register_divider(NULL,
-				"dout_aud_bus", "dout_srp", 0,
-				reg_base + ASS_CLK_DIV, 4, 4, 0, &lock);
+	clk_table[EXYNOS_DOUT_AUD_BUS] = audss_clk_register_divider(NULL,
+				"dout_aud_bus", "dout_srp", 0, 4, 4);
 
-	clk_table[EXYNOS_DOUT_I2S] = clk_register_divider(NULL, "dout_i2s",
-				"mout_i2s", 0, reg_base + ASS_CLK_DIV, 8, 4, 0,
-				&lock);
+	clk_table[EXYNOS_DOUT_I2S] = audss_clk_register_divider(NULL, "dout_i2s",
+				"mout_i2s", 0, 8, 4);
 
-	clk_table[EXYNOS_SRP_CLK] = clk_register_gate(NULL, "srp_clk",
-				"dout_srp", CLK_SET_RATE_PARENT,
-				reg_base + ASS_CLK_GATE, 0, 0, &lock);
+	clk_table[EXYNOS_SRP_CLK] = audss_clk_register_gate(NULL, "srp_clk",
+				"dout_srp", CLK_SET_RATE_PARENT, 0);
 
-	clk_table[EXYNOS_I2S_BUS] = clk_register_gate(NULL, "i2s_bus",
-				"dout_aud_bus", CLK_SET_RATE_PARENT,
-				reg_base + ASS_CLK_GATE, 2, 0, &lock);
+	clk_table[EXYNOS_I2S_BUS] = audss_clk_register_gate(NULL, "i2s_bus",
+				"dout_aud_bus", CLK_SET_RATE_PARENT, 2);
 
-	clk_table[EXYNOS_SCLK_I2S] = clk_register_gate(NULL, "sclk_i2s",
-				"dout_i2s", CLK_SET_RATE_PARENT,
-				reg_base + ASS_CLK_GATE, 3, 0, &lock);
+	clk_table[EXYNOS_SCLK_I2S] = audss_clk_register_gate(NULL, "sclk_i2s",
+				"dout_i2s", CLK_SET_RATE_PARENT, 3);
 
-	clk_table[EXYNOS_PCM_BUS] = clk_register_gate(NULL, "pcm_bus",
-				 "sclk_pcm", CLK_SET_RATE_PARENT,
-				reg_base + ASS_CLK_GATE, 4, 0, &lock);
+	clk_table[EXYNOS_PCM_BUS] = audss_clk_register_gate(NULL, "pcm_bus",
+				 "sclk_pcm", CLK_SET_RATE_PARENT, 4);
 
 	sclk_pcm_in = devm_clk_get(&pdev->dev, "sclk_pcm_in");
 	if (!IS_ERR(sclk_pcm_in))
 		sclk_pcm_p = __clk_get_name(sclk_pcm_in);
-	clk_table[EXYNOS_SCLK_PCM] = clk_register_gate(NULL, "sclk_pcm",
-				sclk_pcm_p, CLK_SET_RATE_PARENT,
-				reg_base + ASS_CLK_GATE, 5, 0, &lock);
+	clk_table[EXYNOS_SCLK_PCM] = audss_clk_register_gate(NULL, "sclk_pcm",
+				sclk_pcm_p, CLK_SET_RATE_PARENT, 5);
 
 	if (variant == TYPE_EXYNOS5420) {
-		clk_table[EXYNOS_ADMA] = clk_register_gate(NULL, "adma",
-				"dout_srp", CLK_SET_RATE_PARENT,
-				reg_base + ASS_CLK_GATE, 9, 0, &lock);
+		clk_table[EXYNOS_ADMA] = audss_clk_register_gate(NULL, "adma",
+				"dout_srp", CLK_SET_RATE_PARENT, 9);
 	}
 
 	for (i = 0; i < clk_data.clk_num; i++) {
@@ -198,6 +468,9 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
 	return 0;
 
 unregister:
+	if (!IS_ERR(pll_in))
+		clk_unprepare(pll_in);
+
 	for (i = 0; i < clk_data.clk_num; i++) {
 		if (!IS_ERR(clk_table[i]))
 			clk_unregister(clk_table[i]);
@@ -214,6 +487,9 @@ static int exynos_audss_clk_remove(struct platform_device *pdev)
 	unregister_syscore_ops(&exynos_audss_clk_syscore_ops);
 #endif
 
+	if (!IS_ERR(pll_in))
+		clk_unprepare(pll_in);
+
 	of_clk_del_provider(pdev->dev.of_node);
 
 	for (i = 0; i < clk_data.clk_num; i++) {
-- 
1.9.1

[PATCH v2 2/5] clk: samsung: Fix clock disable failure because domain being gated

[Krzysztof Kozlowski]

Audio subsystem clocks are located in separate block. If clock for this
block (from main clock domain) 'mau_epll' is gated then any read or
write to audss registers will block.

This was observed on Exynos 5420 platforms (Arndale Octa and Peach
Pi/Pit) after introducing runtime PM to pl330 DMA driver. After that
commit the 'mau_epll' was gated, because the "amba" clock was disabled
and there were no more users of mau_epll. The system hang on disabling
unused clocks from audss block.

Unfortunately the 'mau_epll' clock is not parent of some of audss clocks.

Whenever system wants to operate on audss clocks it has to enable epll
clock. The solution reuses common clk-gate/divider/mux code and duplicates
clk_register_*() functions. In the same time the patch tries to limit
functional changes of the driver so it does not fix minor issues with existing
code (like leaking memory allocated for clk-gate/clk-mux/clk-divider code).
This is addressed later.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Reported-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Reported-by: Kevin Hilman <khilman@kernel.org>
---
 drivers/clk/samsung/clk-exynos-audss.c | 346 +++++++++++++++++++++++++++++----
 1 file changed, 311 insertions(+), 35 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos-audss.c b/drivers/clk/samsung/clk-exynos-audss.c
index 7c4368e75ede..9ec7de866ab4 100644
--- a/drivers/clk/samsung/clk-exynos-audss.c
+++ b/drivers/clk/samsung/clk-exynos-audss.c
@@ -29,6 +29,7 @@ static DEFINE_SPINLOCK(lock);
 static struct clk **clk_table;
 static void __iomem *reg_base;
 static struct clk_onecell_data clk_data;
+static struct clk *pll_in;
 
 #define ASS_CLK_SRC 0x0
 #define ASS_CLK_DIV 0x4
@@ -75,6 +76,276 @@ static const struct of_device_id exynos_audss_clk_of_match[] = {
 	{},
 };
 
+static int pll_clk_enable(void)
+{
+	if (!IS_ERR(pll_in))
+		return clk_enable(pll_in);
+
+	return 0;
+}
+
+static void pll_clk_disable(void)
+{
+	if (!IS_ERR(pll_in))
+		clk_disable(pll_in);
+}
+
+static int audss_clk_gate_enable(struct clk_hw *hw)
+{
+	int ret;
+
+	ret = pll_clk_enable();
+	if (ret)
+		return ret;
+
+	ret = clk_gate_ops.enable(hw);
+
+	pll_clk_disable();
+
+	return ret;
+}
+
+static void audss_clk_gate_disable(struct clk_hw *hw)
+{
+	int ret;
+
+	ret = pll_clk_enable();
+	if (ret)
+		return;
+
+	clk_gate_ops.disable(hw);
+
+	pll_clk_disable();
+}
+
+static int audss_clk_gate_is_enabled(struct clk_hw *hw)
+{
+	int ret;
+
+	ret = pll_clk_enable();
+	if (ret)
+		return ret;
+
+	ret = clk_gate_ops.is_enabled(hw);
+
+	pll_clk_disable();
+
+	return ret;
+}
+
+static const struct clk_ops audss_clk_gate_ops = {
+	.enable = audss_clk_gate_enable,
+	.disable = audss_clk_gate_disable,
+	.is_enabled = audss_clk_gate_is_enabled,
+};
+
+/*
+ * A simplified copy of clk-gate.c:clk_register_gate() to mimic
+ * clk-gate behavior while using customized ops.
+ *
+ * TODO: just like clk-gate it leaks memory for struct clk_gate.
+ */
+static struct clk *audss_clk_register_gate(struct device *dev, const char *name,
+		const char *parent_name, unsigned long flags, u8 bit_idx)
+{
+	struct clk_gate *gate;
+	struct clk *clk;
+	struct clk_init_data init;
+
+	/* allocate the gate */
+	gate = kzalloc(sizeof(struct clk_gate), GFP_KERNEL);
+	if (!gate)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = name;
+	init.ops = &audss_clk_gate_ops;
+	init.flags = flags | CLK_IS_BASIC;
+	init.parent_names = (parent_name ? &parent_name : NULL);
+	init.num_parents = (parent_name ? 1 : 0);
+
+	/* struct clk_gate assignments */
+	gate->reg = reg_base + ASS_CLK_GATE;
+	gate->bit_idx = bit_idx;
+	gate->flags = 0;
+	gate->lock = &lock;
+	gate->hw.init = &init;
+
+	clk = clk_register(dev, &gate->hw);
+
+	if (IS_ERR(clk))
+		kfree(gate);
+
+	return clk;
+}
+
+static unsigned long audss_clk_divider_recalc_rate(struct clk_hw *hw,
+		unsigned long parent_rate)
+{
+	unsigned long ret;
+
+	ret = pll_clk_enable();
+	if (ret) {
+		WARN(ret, "Could not enable pll_in clock\n");
+		return parent_rate;
+	}
+
+	ret = clk_divider_ops.recalc_rate(hw, parent_rate);
+
+	pll_clk_disable();
+
+	return ret;
+}
+
+static long audss_clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
+				unsigned long *prate)
+{
+	return clk_divider_ops.round_rate(hw, rate, prate);
+}
+
+static int audss_clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
+				unsigned long parent_rate)
+{
+	int ret;
+
+	ret = pll_clk_enable();
+	if (ret)
+		return ret;
+
+	ret = clk_divider_ops.set_rate(hw, rate, parent_rate);
+
+	pll_clk_disable();
+
+	return ret;
+}
+
+static const struct clk_ops audss_clk_divider_ops = {
+	.recalc_rate = audss_clk_divider_recalc_rate,
+	.round_rate = audss_clk_divider_round_rate,
+	.set_rate = audss_clk_divider_set_rate,
+};
+
+/*
+ * A simplified copy of clk-divider.c:clk_register_divider() to mimic
+ * clk-divider behavior while using customized ops.
+ */
+static struct clk *audss_clk_register_divider(struct device *dev,
+		const char *name,
+		const char *parent_name, unsigned long flags,
+		u8 shift, u8 width)
+{
+	struct clk_divider *div;
+	struct clk *clk;
+	struct clk_init_data init;
+
+	/* allocate the divider */
+	div = kzalloc(sizeof(struct clk_divider), GFP_KERNEL);
+	if (!div)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = name;
+	init.ops = &audss_clk_divider_ops;
+	init.flags = flags | CLK_IS_BASIC;
+	init.parent_names = (parent_name ? &parent_name : NULL);
+	init.num_parents = (parent_name ? 1 : 0);
+
+	/* struct clk_divider assignments */
+	div->reg = reg_base + ASS_CLK_DIV;
+	div->shift = shift;
+	div->width = width;
+	div->flags = 0;
+	div->lock = &lock;
+	div->hw.init = &init;
+	div->table = NULL;
+
+	/* register the clock */
+	clk = clk_register(dev, &div->hw);
+
+	if (IS_ERR(clk))
+		kfree(div);
+
+	return clk;
+}
+
+static u8 audss_clk_mux_get_parent(struct clk_hw *hw)
+{
+	u8 parent;
+	int ret;
+
+	ret = pll_clk_enable();
+	if (ret) {
+		WARN(ret, "Could not enable pll_in clock\n");
+		return -EINVAL; /* Just like clk_mux_get_parent() */
+	}
+
+	parent = clk_mux_ops.get_parent(hw);
+
+	pll_clk_disable();
+
+	return parent;
+}
+
+static int audss_clk_mux_set_parent(struct clk_hw *hw, u8 index)
+{
+	int ret;
+
+	ret = pll_clk_enable();
+	if (ret)
+		return ret;
+
+	ret = clk_mux_ops.set_parent(hw, index);
+
+	pll_clk_disable();
+
+	return ret;
+}
+
+static const struct clk_ops audss_clk_mux_ops = {
+	.get_parent = audss_clk_mux_get_parent,
+	.set_parent = audss_clk_mux_set_parent,
+	.determine_rate = __clk_mux_determine_rate,
+};
+
+/*
+ * A simplified copy of clk-mux.c:clk_register_mux_table() to mimic
+ * clk-mux behavior while using customized ops.
+ */
+static struct clk *audss_clk_register_mux(struct device *dev, const char *name,
+		const char **parent_names, u8 num_parents, unsigned long flags,
+		u8 shift, u8 width)
+{
+	struct clk_mux *mux;
+	struct clk *clk;
+	struct clk_init_data init;
+	u32 mask = BIT(width) - 1;
+
+	/* allocate the mux */
+	mux = kzalloc(sizeof(struct clk_mux), GFP_KERNEL);
+	if (!mux)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = name;
+	init.ops = &audss_clk_mux_ops;
+	init.flags = flags | CLK_IS_BASIC;
+	init.parent_names = parent_names;
+	init.num_parents = num_parents;
+
+	/* struct clk_mux assignments */
+	mux->reg = reg_base + ASS_CLK_SRC;
+	mux->shift = shift;
+	mux->mask = mask;
+	mux->flags = 0;
+	mux->lock = &lock;
+	mux->table = NULL;
+	mux->hw.init = &init;
+
+	clk = clk_register(dev, &mux->hw);
+
+	if (IS_ERR(clk))
+		kfree(mux);
+
+	return clk;
+}
+
 /* register exynos_audss clocks */
 static int exynos_audss_clk_probe(struct platform_device *pdev)
 {
@@ -83,7 +354,7 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
 	const char *mout_audss_p[] = {"fin_pll", "fout_epll"};
 	const char *mout_i2s_p[] = {"mout_audss", "cdclk0", "sclk_audio0"};
 	const char *sclk_pcm_p = "sclk_pcm0";
-	struct clk *pll_ref, *pll_in, *cdclk, *sclk_audio, *sclk_pcm_in;
+	struct clk *pll_ref, *cdclk, *sclk_audio, *sclk_pcm_in;
 	const struct of_device_id *match;
 	enum exynos_audss_clk_type variant;
 
@@ -115,12 +386,21 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
 	pll_in = devm_clk_get(&pdev->dev, "pll_in");
 	if (!IS_ERR(pll_ref))
 		mout_audss_p[0] = __clk_get_name(pll_ref);
-	if (!IS_ERR(pll_in))
+	if (!IS_ERR(pll_in)) {
 		mout_audss_p[1] = __clk_get_name(pll_in);
-	clk_table[EXYNOS_MOUT_AUDSS] = clk_register_mux(NULL, "mout_audss",
+
+		ret = clk_prepare(pll_in);
+		if (ret) {
+			dev_err(&pdev->dev,
+				"failed to prepare the pll_in clock\n");
+			return ret;
+		}
+
+	}
+
+	clk_table[EXYNOS_MOUT_AUDSS] = audss_clk_register_mux(NULL, "mout_audss",
 				mout_audss_p, ARRAY_SIZE(mout_audss_p),
-				CLK_SET_RATE_NO_REPARENT,
-				reg_base + ASS_CLK_SRC, 0, 1, 0, &lock);
+				CLK_SET_RATE_NO_REPARENT, 0, 1);
 
 	cdclk = devm_clk_get(&pdev->dev, "cdclk");
 	sclk_audio = devm_clk_get(&pdev->dev, "sclk_audio");
@@ -128,50 +408,40 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
 		mout_i2s_p[1] = __clk_get_name(cdclk);
 	if (!IS_ERR(sclk_audio))
 		mout_i2s_p[2] = __clk_get_name(sclk_audio);
-	clk_table[EXYNOS_MOUT_I2S] = clk_register_mux(NULL, "mout_i2s",
+	clk_table[EXYNOS_MOUT_I2S] = audss_clk_register_mux(NULL, "mout_i2s",
 				mout_i2s_p, ARRAY_SIZE(mout_i2s_p),
-				CLK_SET_RATE_NO_REPARENT,
-				reg_base + ASS_CLK_SRC, 2, 2, 0, &lock);
+				CLK_SET_RATE_NO_REPARENT, 2, 2);
 
-	clk_table[EXYNOS_DOUT_SRP] = clk_register_divider(NULL, "dout_srp",
-				"mout_audss", 0, reg_base + ASS_CLK_DIV, 0, 4,
-				0, &lock);
+	clk_table[EXYNOS_DOUT_SRP] = audss_clk_register_divider(NULL, "dout_srp",
+				"mout_audss", 0, 0, 4);
 
-	clk_table[EXYNOS_DOUT_AUD_BUS] = clk_register_divider(NULL,
-				"dout_aud_bus", "dout_srp", 0,
-				reg_base + ASS_CLK_DIV, 4, 4, 0, &lock);
+	clk_table[EXYNOS_DOUT_AUD_BUS] = audss_clk_register_divider(NULL,
+				"dout_aud_bus", "dout_srp", 0, 4, 4);
 
-	clk_table[EXYNOS_DOUT_I2S] = clk_register_divider(NULL, "dout_i2s",
-				"mout_i2s", 0, reg_base + ASS_CLK_DIV, 8, 4, 0,
-				&lock);
+	clk_table[EXYNOS_DOUT_I2S] = audss_clk_register_divider(NULL, "dout_i2s",
+				"mout_i2s", 0, 8, 4);
 
-	clk_table[EXYNOS_SRP_CLK] = clk_register_gate(NULL, "srp_clk",
-				"dout_srp", CLK_SET_RATE_PARENT,
-				reg_base + ASS_CLK_GATE, 0, 0, &lock);
+	clk_table[EXYNOS_SRP_CLK] = audss_clk_register_gate(NULL, "srp_clk",
+				"dout_srp", CLK_SET_RATE_PARENT, 0);
 
-	clk_table[EXYNOS_I2S_BUS] = clk_register_gate(NULL, "i2s_bus",
-				"dout_aud_bus", CLK_SET_RATE_PARENT,
-				reg_base + ASS_CLK_GATE, 2, 0, &lock);
+	clk_table[EXYNOS_I2S_BUS] = audss_clk_register_gate(NULL, "i2s_bus",
+				"dout_aud_bus", CLK_SET_RATE_PARENT, 2);
 
-	clk_table[EXYNOS_SCLK_I2S] = clk_register_gate(NULL, "sclk_i2s",
-				"dout_i2s", CLK_SET_RATE_PARENT,
-				reg_base + ASS_CLK_GATE, 3, 0, &lock);
+	clk_table[EXYNOS_SCLK_I2S] = audss_clk_register_gate(NULL, "sclk_i2s",
+				"dout_i2s", CLK_SET_RATE_PARENT, 3);
 
-	clk_table[EXYNOS_PCM_BUS] = clk_register_gate(NULL, "pcm_bus",
-				 "sclk_pcm", CLK_SET_RATE_PARENT,
-				reg_base + ASS_CLK_GATE, 4, 0, &lock);
+	clk_table[EXYNOS_PCM_BUS] = audss_clk_register_gate(NULL, "pcm_bus",
+				 "sclk_pcm", CLK_SET_RATE_PARENT, 4);
 
 	sclk_pcm_in = devm_clk_get(&pdev->dev, "sclk_pcm_in");
 	if (!IS_ERR(sclk_pcm_in))
 		sclk_pcm_p = __clk_get_name(sclk_pcm_in);
-	clk_table[EXYNOS_SCLK_PCM] = clk_register_gate(NULL, "sclk_pcm",
-				sclk_pcm_p, CLK_SET_RATE_PARENT,
-				reg_base + ASS_CLK_GATE, 5, 0, &lock);
+	clk_table[EXYNOS_SCLK_PCM] = audss_clk_register_gate(NULL, "sclk_pcm",
+				sclk_pcm_p, CLK_SET_RATE_PARENT, 5);
 
 	if (variant == TYPE_EXYNOS5420) {
-		clk_table[EXYNOS_ADMA] = clk_register_gate(NULL, "adma",
-				"dout_srp", CLK_SET_RATE_PARENT,
-				reg_base + ASS_CLK_GATE, 9, 0, &lock);
+		clk_table[EXYNOS_ADMA] = audss_clk_register_gate(NULL, "adma",
+				"dout_srp", CLK_SET_RATE_PARENT, 9);
 	}
 
 	for (i = 0; i < clk_data.clk_num; i++) {
@@ -198,6 +468,9 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
 	return 0;
 
 unregister:
+	if (!IS_ERR(pll_in))
+		clk_unprepare(pll_in);
+
 	for (i = 0; i < clk_data.clk_num; i++) {
 		if (!IS_ERR(clk_table[i]))
 			clk_unregister(clk_table[i]);
@@ -214,6 +487,9 @@ static int exynos_audss_clk_remove(struct platform_device *pdev)
 	unregister_syscore_ops(&exynos_audss_clk_syscore_ops);
 #endif
 
+	if (!IS_ERR(pll_in))
+		clk_unprepare(pll_in);
+
 	of_clk_del_provider(pdev->dev.of_node);
 
 	for (i = 0; i < clk_data.clk_num; i++) {
-- 
1.9.1

[PATCH v2 2/5] clk: samsung: Fix clock disable failure because domain being gated

[Krzysztof Kozlowski]

Audio subsystem clocks are located in separate block. If clock for this
block (from main clock domain) 'mau_epll' is gated then any read or
write to audss registers will block.

This was observed on Exynos 5420 platforms (Arndale Octa and Peach
Pi/Pit) after introducing runtime PM to pl330 DMA driver. After that
commit the 'mau_epll' was gated, because the "amba" clock was disabled
and there were no more users of mau_epll. The system hang on disabling
unused clocks from audss block.

Unfortunately the 'mau_epll' clock is not parent of some of audss clocks.

Whenever system wants to operate on audss clocks it has to enable epll
clock. The solution reuses common clk-gate/divider/mux code and duplicates
clk_register_*() functions. In the same time the patch tries to limit
functional changes of the driver so it does not fix minor issues with existing
code (like leaking memory allocated for clk-gate/clk-mux/clk-divider code).
This is addressed later.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Reported-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Reported-by: Kevin Hilman <khilman@kernel.org>
---
 drivers/clk/samsung/clk-exynos-audss.c | 346 +++++++++++++++++++++++++++++----
 1 file changed, 311 insertions(+), 35 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos-audss.c b/drivers/clk/samsung/clk-exynos-audss.c
index 7c4368e75ede..9ec7de866ab4 100644
--- a/drivers/clk/samsung/clk-exynos-audss.c
+++ b/drivers/clk/samsung/clk-exynos-audss.c
@@ -29,6 +29,7 @@ static DEFINE_SPINLOCK(lock);
 static struct clk **clk_table;
 static void __iomem *reg_base;
 static struct clk_onecell_data clk_data;
+static struct clk *pll_in;
 
 #define ASS_CLK_SRC 0x0
 #define ASS_CLK_DIV 0x4
@@ -75,6 +76,276 @@ static const struct of_device_id exynos_audss_clk_of_match[] = {
 	{},
 };
 
+static int pll_clk_enable(void)
+{
+	if (!IS_ERR(pll_in))
+		return clk_enable(pll_in);
+
+	return 0;
+}
+
+static void pll_clk_disable(void)
+{
+	if (!IS_ERR(pll_in))
+		clk_disable(pll_in);
+}
+
+static int audss_clk_gate_enable(struct clk_hw *hw)
+{
+	int ret;
+
+	ret = pll_clk_enable();
+	if (ret)
+		return ret;
+
+	ret = clk_gate_ops.enable(hw);
+
+	pll_clk_disable();
+
+	return ret;
+}
+
+static void audss_clk_gate_disable(struct clk_hw *hw)
+{
+	int ret;
+
+	ret = pll_clk_enable();
+	if (ret)
+		return;
+
+	clk_gate_ops.disable(hw);
+
+	pll_clk_disable();
+}
+
+static int audss_clk_gate_is_enabled(struct clk_hw *hw)
+{
+	int ret;
+
+	ret = pll_clk_enable();
+	if (ret)
+		return ret;
+
+	ret = clk_gate_ops.is_enabled(hw);
+
+	pll_clk_disable();
+
+	return ret;
+}
+
+static const struct clk_ops audss_clk_gate_ops = {
+	.enable = audss_clk_gate_enable,
+	.disable = audss_clk_gate_disable,
+	.is_enabled = audss_clk_gate_is_enabled,
+};
+
+/*
+ * A simplified copy of clk-gate.c:clk_register_gate() to mimic
+ * clk-gate behavior while using customized ops.
+ *
+ * TODO: just like clk-gate it leaks memory for struct clk_gate.
+ */
+static struct clk *audss_clk_register_gate(struct device *dev, const char *name,
+		const char *parent_name, unsigned long flags, u8 bit_idx)
+{
+	struct clk_gate *gate;
+	struct clk *clk;
+	struct clk_init_data init;
+
+	/* allocate the gate */
+	gate = kzalloc(sizeof(struct clk_gate), GFP_KERNEL);
+	if (!gate)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = name;
+	init.ops = &audss_clk_gate_ops;
+	init.flags = flags | CLK_IS_BASIC;
+	init.parent_names = (parent_name ? &parent_name : NULL);
+	init.num_parents = (parent_name ? 1 : 0);
+
+	/* struct clk_gate assignments */
+	gate->reg = reg_base + ASS_CLK_GATE;
+	gate->bit_idx = bit_idx;
+	gate->flags = 0;
+	gate->lock = &lock;
+	gate->hw.init = &init;
+
+	clk = clk_register(dev, &gate->hw);
+
+	if (IS_ERR(clk))
+		kfree(gate);
+
+	return clk;
+}
+
+static unsigned long audss_clk_divider_recalc_rate(struct clk_hw *hw,
+		unsigned long parent_rate)
+{
+	unsigned long ret;
+
+	ret = pll_clk_enable();
+	if (ret) {
+		WARN(ret, "Could not enable pll_in clock\n");
+		return parent_rate;
+	}
+
+	ret = clk_divider_ops.recalc_rate(hw, parent_rate);
+
+	pll_clk_disable();
+
+	return ret;
+}
+
+static long audss_clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
+				unsigned long *prate)
+{
+	return clk_divider_ops.round_rate(hw, rate, prate);
+}
+
+static int audss_clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
+				unsigned long parent_rate)
+{
+	int ret;
+
+	ret = pll_clk_enable();
+	if (ret)
+		return ret;
+
+	ret = clk_divider_ops.set_rate(hw, rate, parent_rate);
+
+	pll_clk_disable();
+
+	return ret;
+}
+
+static const struct clk_ops audss_clk_divider_ops = {
+	.recalc_rate = audss_clk_divider_recalc_rate,
+	.round_rate = audss_clk_divider_round_rate,
+	.set_rate = audss_clk_divider_set_rate,
+};
+
+/*
+ * A simplified copy of clk-divider.c:clk_register_divider() to mimic
+ * clk-divider behavior while using customized ops.
+ */
+static struct clk *audss_clk_register_divider(struct device *dev,
+		const char *name,
+		const char *parent_name, unsigned long flags,
+		u8 shift, u8 width)
+{
+	struct clk_divider *div;
+	struct clk *clk;
+	struct clk_init_data init;
+
+	/* allocate the divider */
+	div = kzalloc(sizeof(struct clk_divider), GFP_KERNEL);
+	if (!div)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = name;
+	init.ops = &audss_clk_divider_ops;
+	init.flags = flags | CLK_IS_BASIC;
+	init.parent_names = (parent_name ? &parent_name : NULL);
+	init.num_parents = (parent_name ? 1 : 0);
+
+	/* struct clk_divider assignments */
+	div->reg = reg_base + ASS_CLK_DIV;
+	div->shift = shift;
+	div->width = width;
+	div->flags = 0;
+	div->lock = &lock;
+	div->hw.init = &init;
+	div->table = NULL;
+
+	/* register the clock */
+	clk = clk_register(dev, &div->hw);
+
+	if (IS_ERR(clk))
+		kfree(div);
+
+	return clk;
+}
+
+static u8 audss_clk_mux_get_parent(struct clk_hw *hw)
+{
+	u8 parent;
+	int ret;
+
+	ret = pll_clk_enable();
+	if (ret) {
+		WARN(ret, "Could not enable pll_in clock\n");
+		return -EINVAL; /* Just like clk_mux_get_parent() */
+	}
+
+	parent = clk_mux_ops.get_parent(hw);
+
+	pll_clk_disable();
+
+	return parent;
+}
+
+static int audss_clk_mux_set_parent(struct clk_hw *hw, u8 index)
+{
+	int ret;
+
+	ret = pll_clk_enable();
+	if (ret)
+		return ret;
+
+	ret = clk_mux_ops.set_parent(hw, index);
+
+	pll_clk_disable();
+
+	return ret;
+}
+
+static const struct clk_ops audss_clk_mux_ops = {
+	.get_parent = audss_clk_mux_get_parent,
+	.set_parent = audss_clk_mux_set_parent,
+	.determine_rate = __clk_mux_determine_rate,
+};
+
+/*
+ * A simplified copy of clk-mux.c:clk_register_mux_table() to mimic
+ * clk-mux behavior while using customized ops.
+ */
+static struct clk *audss_clk_register_mux(struct device *dev, const char *name,
+		const char **parent_names, u8 num_parents, unsigned long flags,
+		u8 shift, u8 width)
+{
+	struct clk_mux *mux;
+	struct clk *clk;
+	struct clk_init_data init;
+	u32 mask = BIT(width) - 1;
+
+	/* allocate the mux */
+	mux = kzalloc(sizeof(struct clk_mux), GFP_KERNEL);
+	if (!mux)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = name;
+	init.ops = &audss_clk_mux_ops;
+	init.flags = flags | CLK_IS_BASIC;
+	init.parent_names = parent_names;
+	init.num_parents = num_parents;
+
+	/* struct clk_mux assignments */
+	mux->reg = reg_base + ASS_CLK_SRC;
+	mux->shift = shift;
+	mux->mask = mask;
+	mux->flags = 0;
+	mux->lock = &lock;
+	mux->table = NULL;
+	mux->hw.init = &init;
+
+	clk = clk_register(dev, &mux->hw);
+
+	if (IS_ERR(clk))
+		kfree(mux);
+
+	return clk;
+}
+
 /* register exynos_audss clocks */
 static int exynos_audss_clk_probe(struct platform_device *pdev)
 {
@@ -83,7 +354,7 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
 	const char *mout_audss_p[] = {"fin_pll", "fout_epll"};
 	const char *mout_i2s_p[] = {"mout_audss", "cdclk0", "sclk_audio0"};
 	const char *sclk_pcm_p = "sclk_pcm0";
-	struct clk *pll_ref, *pll_in, *cdclk, *sclk_audio, *sclk_pcm_in;
+	struct clk *pll_ref, *cdclk, *sclk_audio, *sclk_pcm_in;
 	const struct of_device_id *match;
 	enum exynos_audss_clk_type variant;
 
@@ -115,12 +386,21 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
 	pll_in = devm_clk_get(&pdev->dev, "pll_in");
 	if (!IS_ERR(pll_ref))
 		mout_audss_p[0] = __clk_get_name(pll_ref);
-	if (!IS_ERR(pll_in))
+	if (!IS_ERR(pll_in)) {
 		mout_audss_p[1] = __clk_get_name(pll_in);
-	clk_table[EXYNOS_MOUT_AUDSS] = clk_register_mux(NULL, "mout_audss",
+
+		ret = clk_prepare(pll_in);
+		if (ret) {
+			dev_err(&pdev->dev,
+				"failed to prepare the pll_in clock\n");
+			return ret;
+		}
+
+	}
+
+	clk_table[EXYNOS_MOUT_AUDSS] = audss_clk_register_mux(NULL, "mout_audss",
 				mout_audss_p, ARRAY_SIZE(mout_audss_p),
-				CLK_SET_RATE_NO_REPARENT,
-				reg_base + ASS_CLK_SRC, 0, 1, 0, &lock);
+				CLK_SET_RATE_NO_REPARENT, 0, 1);
 
 	cdclk = devm_clk_get(&pdev->dev, "cdclk");
 	sclk_audio = devm_clk_get(&pdev->dev, "sclk_audio");
@@ -128,50 +408,40 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
 		mout_i2s_p[1] = __clk_get_name(cdclk);
 	if (!IS_ERR(sclk_audio))
 		mout_i2s_p[2] = __clk_get_name(sclk_audio);
-	clk_table[EXYNOS_MOUT_I2S] = clk_register_mux(NULL, "mout_i2s",
+	clk_table[EXYNOS_MOUT_I2S] = audss_clk_register_mux(NULL, "mout_i2s",
 				mout_i2s_p, ARRAY_SIZE(mout_i2s_p),
-				CLK_SET_RATE_NO_REPARENT,
-				reg_base + ASS_CLK_SRC, 2, 2, 0, &lock);
+				CLK_SET_RATE_NO_REPARENT, 2, 2);
 
-	clk_table[EXYNOS_DOUT_SRP] = clk_register_divider(NULL, "dout_srp",
-				"mout_audss", 0, reg_base + ASS_CLK_DIV, 0, 4,
-				0, &lock);
+	clk_table[EXYNOS_DOUT_SRP] = audss_clk_register_divider(NULL, "dout_srp",
+				"mout_audss", 0, 0, 4);
 
-	clk_table[EXYNOS_DOUT_AUD_BUS] = clk_register_divider(NULL,
-				"dout_aud_bus", "dout_srp", 0,
-				reg_base + ASS_CLK_DIV, 4, 4, 0, &lock);
+	clk_table[EXYNOS_DOUT_AUD_BUS] = audss_clk_register_divider(NULL,
+				"dout_aud_bus", "dout_srp", 0, 4, 4);
 
-	clk_table[EXYNOS_DOUT_I2S] = clk_register_divider(NULL, "dout_i2s",
-				"mout_i2s", 0, reg_base + ASS_CLK_DIV, 8, 4, 0,
-				&lock);
+	clk_table[EXYNOS_DOUT_I2S] = audss_clk_register_divider(NULL, "dout_i2s",
+				"mout_i2s", 0, 8, 4);
 
-	clk_table[EXYNOS_SRP_CLK] = clk_register_gate(NULL, "srp_clk",
-				"dout_srp", CLK_SET_RATE_PARENT,
-				reg_base + ASS_CLK_GATE, 0, 0, &lock);
+	clk_table[EXYNOS_SRP_CLK] = audss_clk_register_gate(NULL, "srp_clk",
+				"dout_srp", CLK_SET_RATE_PARENT, 0);
 
-	clk_table[EXYNOS_I2S_BUS] = clk_register_gate(NULL, "i2s_bus",
-				"dout_aud_bus", CLK_SET_RATE_PARENT,
-				reg_base + ASS_CLK_GATE, 2, 0, &lock);
+	clk_table[EXYNOS_I2S_BUS] = audss_clk_register_gate(NULL, "i2s_bus",
+				"dout_aud_bus", CLK_SET_RATE_PARENT, 2);
 
-	clk_table[EXYNOS_SCLK_I2S] = clk_register_gate(NULL, "sclk_i2s",
-				"dout_i2s", CLK_SET_RATE_PARENT,
-				reg_base + ASS_CLK_GATE, 3, 0, &lock);
+	clk_table[EXYNOS_SCLK_I2S] = audss_clk_register_gate(NULL, "sclk_i2s",
+				"dout_i2s", CLK_SET_RATE_PARENT, 3);
 
-	clk_table[EXYNOS_PCM_BUS] = clk_register_gate(NULL, "pcm_bus",
-				 "sclk_pcm", CLK_SET_RATE_PARENT,
-				reg_base + ASS_CLK_GATE, 4, 0, &lock);
+	clk_table[EXYNOS_PCM_BUS] = audss_clk_register_gate(NULL, "pcm_bus",
+				 "sclk_pcm", CLK_SET_RATE_PARENT, 4);
 
 	sclk_pcm_in = devm_clk_get(&pdev->dev, "sclk_pcm_in");
 	if (!IS_ERR(sclk_pcm_in))
 		sclk_pcm_p = __clk_get_name(sclk_pcm_in);
-	clk_table[EXYNOS_SCLK_PCM] = clk_register_gate(NULL, "sclk_pcm",
-				sclk_pcm_p, CLK_SET_RATE_PARENT,
-				reg_base + ASS_CLK_GATE, 5, 0, &lock);
+	clk_table[EXYNOS_SCLK_PCM] = audss_clk_register_gate(NULL, "sclk_pcm",
+				sclk_pcm_p, CLK_SET_RATE_PARENT, 5);
 
 	if (variant == TYPE_EXYNOS5420) {
-		clk_table[EXYNOS_ADMA] = clk_register_gate(NULL, "adma",
-				"dout_srp", CLK_SET_RATE_PARENT,
-				reg_base + ASS_CLK_GATE, 9, 0, &lock);
+		clk_table[EXYNOS_ADMA] = audss_clk_register_gate(NULL, "adma",
+				"dout_srp", CLK_SET_RATE_PARENT, 9);
 	}
 
 	for (i = 0; i < clk_data.clk_num; i++) {
@@ -198,6 +468,9 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
 	return 0;
 
 unregister:
+	if (!IS_ERR(pll_in))
+		clk_unprepare(pll_in);
+
 	for (i = 0; i < clk_data.clk_num; i++) {
 		if (!IS_ERR(clk_table[i]))
 			clk_unregister(clk_table[i]);
@@ -214,6 +487,9 @@ static int exynos_audss_clk_remove(struct platform_device *pdev)
 	unregister_syscore_ops(&exynos_audss_clk_syscore_ops);
 #endif
 
+	if (!IS_ERR(pll_in))
+		clk_unprepare(pll_in);
+
 	of_clk_del_provider(pdev->dev.of_node);
 
 	for (i = 0; i < clk_data.clk_num; i++) {
-- 
1.9.1

[PATCH v2 3/5] pinctrl: exynos: Fix GPIO setup failure because domain clock being gated

[Krzysztof Kozlowski]

The audio subsystem on Exynos 5420 has separate clocks and GPIO. To
operate properly on GPIOs the main block clock 'mau_epll' must be
enabled.

This was observed on Peach Pi/Pit and Arndale Octa (after enabling i2s0)
after introducing runtime PM to pl330 DMA driver. After that commit the
'mau_epll' was gated, because the "amba" clock was disabled and there
were no more users of mau_epll.

The system hang just before probing i2s0 because
samsung_pinmux_setup() tried to access memory from audss block which was
gated.

Add a clock property to the pinctrl driver and enable the clock during
GPIO setup. During normal GPIO operations (set, get, set_direction) the
clock is not enabled.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 .../bindings/pinctrl/samsung-pinctrl.txt           |   6 ++
 drivers/pinctrl/samsung/pinctrl-samsung.c          | 110 +++++++++++++++++++--
 drivers/pinctrl/samsung/pinctrl-samsung.h          |   2 +
 3 files changed, 111 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
index 8425838a6dff..eb121daabe9d 100644
--- a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
@@ -93,6 +93,12 @@ Required Properties:
   pin configuration should use the bindings listed in the "pinctrl-bindings.txt"
   file.
 
+Optional Properties:
+- clocks: Optional clock needed to access the block. Will be enabled/disabled
+  during GPIO configuration, suspend and resume but not during GPIO operations
+  (like set, get, set direction).
+- clock-names: Must be "block".
+
 External GPIO and Wakeup Interrupts:
 
 The controller supports two types of external interrupts over gpio. The first
diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index ec580af35856..96419aba7650 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -24,6 +24,7 @@
 #include <linux/platform_device.h>
 #include <linux/io.h>
 #include <linux/slab.h>
+#include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/gpio.h>
 #include <linux/irqdomain.h>
@@ -55,6 +56,32 @@ static LIST_HEAD(drvdata_list);
 
 static unsigned int pin_base;
 
+static int pctl_clk_enable(struct pinctrl_dev *pctldev)
+{
+	struct samsung_pinctrl_drv_data *drvdata;
+	int ret;
+
+	drvdata = pinctrl_dev_get_drvdata(pctldev);
+	if (!drvdata->clk)
+		return 0;
+
+	ret = clk_enable(drvdata->clk);
+	if (ret)
+		dev_err(pctldev->dev, "failed to enable clock: %d\n", ret);
+
+	return ret;
+}
+
+static void pctl_clk_disable(struct pinctrl_dev *pctldev)
+{
+	struct samsung_pinctrl_drv_data *drvdata;
+
+	drvdata = pinctrl_dev_get_drvdata(pctldev);
+
+	/* clk/core.c does the check if clk != NULL */
+	clk_disable(drvdata->clk);
+}
+
 static inline struct samsung_pin_bank *gc_to_pin_bank(struct gpio_chip *gc)
 {
 	return container_of(gc, struct samsung_pin_bank, gpio_chip);
@@ -374,7 +401,9 @@ static void samsung_pinmux_setup(struct pinctrl_dev *pctldev, unsigned selector,
 	const struct samsung_pmx_func *func;
 	const struct samsung_pin_group *grp;
 
+	pctl_clk_enable(pctldev);
 	drvdata = pinctrl_dev_get_drvdata(pctldev);
+
 	func = &drvdata->pmx_functions[selector];
 	grp = &drvdata->pin_groups[group];
 
@@ -398,6 +427,8 @@ static void samsung_pinmux_setup(struct pinctrl_dev *pctldev, unsigned selector,
 	writel(data, reg + type->reg_offset[PINCFG_TYPE_FUNC]);
 
 	spin_unlock_irqrestore(&bank->slock, flags);
+
+	pctl_clk_disable(pctldev);
 }
 
 /* enable a specified pinmux by writing to registers */
@@ -469,20 +500,37 @@ static int samsung_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
 {
 	int i, ret;
 
+	ret = pctl_clk_enable(pctldev);
+	if (ret)
+		goto out;
+
 	for (i = 0; i < num_configs; i++) {
 		ret = samsung_pinconf_rw(pctldev, pin, &configs[i], true);
 		if (ret < 0)
-			return ret;
+			goto out;
 	} /* for each config */
 
-	return 0;
+out:
+	pctl_clk_disable(pctldev);
+
+	return ret;
 }
 
 /* get the pin config settings for a specified pin */
 static int samsung_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
 					unsigned long *config)
 {
-	return samsung_pinconf_rw(pctldev, pin, config, false);
+	int ret;
+
+	ret = pctl_clk_enable(pctldev);
+	if (ret)
+		return ret;
+
+	ret = samsung_pinconf_rw(pctldev, pin, config, false);
+
+	pctl_clk_disable(pctldev);
+
+	return ret;
 }
 
 /* set the pin config settings for a specified pin group */
@@ -1057,10 +1105,23 @@ static int samsung_pinctrl_probe(struct platform_device *pdev)
 	}
 	drvdata->dev = dev;
 
+	drvdata->clk = clk_get(&pdev->dev, "block");
+	if (!IS_ERR(drvdata->clk)) {
+		ret = clk_prepare_enable(drvdata->clk);
+		if (ret) {
+			dev_err(dev, "failed to enable clk: %d\n", ret);
+			return ret;
+		}
+	} else {
+		drvdata->clk = NULL;
+	}
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	drvdata->virt_base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(drvdata->virt_base))
-		return PTR_ERR(drvdata->virt_base);
+	if (IS_ERR(drvdata->virt_base)) {
+		ret = PTR_ERR(drvdata->virt_base);
+		goto err;
+	}
 
 	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	if (res)
@@ -1068,12 +1129,12 @@ static int samsung_pinctrl_probe(struct platform_device *pdev)
 
 	ret = samsung_gpiolib_register(pdev, drvdata);
 	if (ret)
-		return ret;
+		goto err;
 
 	ret = samsung_pinctrl_register(pdev, drvdata);
 	if (ret) {
 		samsung_gpiolib_unregister(pdev, drvdata);
-		return ret;
+		goto err;
 	}
 
 	if (ctrl->eint_gpio_init)
@@ -1085,6 +1146,22 @@ static int samsung_pinctrl_probe(struct platform_device *pdev)
 
 	/* Add to the global list */
 	list_add_tail(&drvdata->node, &drvdata_list);
+	clk_disable(drvdata->clk); /* Leave prepared */
+
+	return 0;
+
+err:
+	if (drvdata->clk)
+		clk_disable_unprepare(drvdata->clk);
+
+	return ret;
+}
+
+static int samsung_pinctrl_remove(struct platform_device *pdev)
+{
+	struct samsung_pinctrl_drv_data *drvdata = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(drvdata->clk);
 
 	return 0;
 }
@@ -1102,6 +1179,13 @@ static void samsung_pinctrl_suspend_dev(
 	void __iomem *virt_base = drvdata->virt_base;
 	int i;
 
+	if (drvdata->clk) {
+		if (clk_enable(drvdata->clk)) {
+			dev_err(drvdata->dev, "failed to enable clock\n");
+			return;
+		}
+	}
+
 	for (i = 0; i < drvdata->nr_banks; i++) {
 		struct samsung_pin_bank *bank = &drvdata->pin_banks[i];
 		void __iomem *reg = virt_base + bank->pctl_offset;
@@ -1133,6 +1217,8 @@ static void samsung_pinctrl_suspend_dev(
 
 	if (drvdata->suspend)
 		drvdata->suspend(drvdata);
+
+	clk_disable(drvdata->clk);
 }
 
 /**
@@ -1148,6 +1234,13 @@ static void samsung_pinctrl_resume_dev(struct samsung_pinctrl_drv_data *drvdata)
 	void __iomem *virt_base = drvdata->virt_base;
 	int i;
 
+	if (drvdata->clk) {
+		if (clk_enable(drvdata->clk)) {
+			dev_err(drvdata->dev, "failed to enable clock\n");
+			return;
+		}
+	}
+
 	if (drvdata->resume)
 		drvdata->resume(drvdata);
 
@@ -1181,6 +1274,8 @@ static void samsung_pinctrl_resume_dev(struct samsung_pinctrl_drv_data *drvdata)
 			if (widths[type])
 				writel(bank->pm_save[type], reg + offs[type]);
 	}
+
+	clk_disable(drvdata->clk);
 }
 
 /**
@@ -1264,6 +1359,7 @@ MODULE_DEVICE_TABLE(of, samsung_pinctrl_dt_match);
 
 static struct platform_driver samsung_pinctrl_driver = {
 	.probe		= samsung_pinctrl_probe,
+	.remove		= samsung_pinctrl_remove,
 	.driver = {
 		.name	= "samsung-pinctrl",
 		.of_match_table = samsung_pinctrl_dt_match,
diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h
index 1b8c0139d604..666cb23eb9f2 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.h
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.h
@@ -201,6 +201,7 @@ struct samsung_pin_ctrl {
  * struct samsung_pinctrl_drv_data: wrapper for holding driver data together.
  * @node: global list node
  * @virt_base: register base address of the controller.
+ * @clk: Optional clock to enable/disable during setup. May be NULL.
  * @dev: device instance representing the controller.
  * @irq: interrpt number used by the controller to notify gpio interrupts.
  * @ctrl: pin controller instance managed by the driver.
@@ -218,6 +219,7 @@ struct samsung_pinctrl_drv_data {
 	void __iomem			*virt_base;
 	struct device			*dev;
 	int				irq;
+	struct clk			*clk;
 
 	struct pinctrl_desc		pctl;
 	struct pinctrl_dev		*pctl_dev;
-- 
1.9.1

[PATCH v2 3/5] pinctrl: exynos: Fix GPIO setup failure because domain clock being gated

[Krzysztof Kozlowski]

The audio subsystem on Exynos 5420 has separate clocks and GPIO. To
operate properly on GPIOs the main block clock 'mau_epll' must be
enabled.

This was observed on Peach Pi/Pit and Arndale Octa (after enabling i2s0)
after introducing runtime PM to pl330 DMA driver. After that commit the
'mau_epll' was gated, because the "amba" clock was disabled and there
were no more users of mau_epll.

The system hang just before probing i2s0 because
samsung_pinmux_setup() tried to access memory from audss block which was
gated.

Add a clock property to the pinctrl driver and enable the clock during
GPIO setup. During normal GPIO operations (set, get, set_direction) the
clock is not enabled.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 .../bindings/pinctrl/samsung-pinctrl.txt           |   6 ++
 drivers/pinctrl/samsung/pinctrl-samsung.c          | 110 +++++++++++++++++++--
 drivers/pinctrl/samsung/pinctrl-samsung.h          |   2 +
 3 files changed, 111 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
index 8425838a6dff..eb121daabe9d 100644
--- a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
@@ -93,6 +93,12 @@ Required Properties:
   pin configuration should use the bindings listed in the "pinctrl-bindings.txt"
   file.
 
+Optional Properties:
+- clocks: Optional clock needed to access the block. Will be enabled/disabled
+  during GPIO configuration, suspend and resume but not during GPIO operations
+  (like set, get, set direction).
+- clock-names: Must be "block".
+
 External GPIO and Wakeup Interrupts:
 
 The controller supports two types of external interrupts over gpio. The first
diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index ec580af35856..96419aba7650 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -24,6 +24,7 @@
 #include <linux/platform_device.h>
 #include <linux/io.h>
 #include <linux/slab.h>
+#include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/gpio.h>
 #include <linux/irqdomain.h>
@@ -55,6 +56,32 @@ static LIST_HEAD(drvdata_list);
 
 static unsigned int pin_base;
 
+static int pctl_clk_enable(struct pinctrl_dev *pctldev)
+{
+	struct samsung_pinctrl_drv_data *drvdata;
+	int ret;
+
+	drvdata = pinctrl_dev_get_drvdata(pctldev);
+	if (!drvdata->clk)
+		return 0;
+
+	ret = clk_enable(drvdata->clk);
+	if (ret)
+		dev_err(pctldev->dev, "failed to enable clock: %d\n", ret);
+
+	return ret;
+}
+
+static void pctl_clk_disable(struct pinctrl_dev *pctldev)
+{
+	struct samsung_pinctrl_drv_data *drvdata;
+
+	drvdata = pinctrl_dev_get_drvdata(pctldev);
+
+	/* clk/core.c does the check if clk != NULL */
+	clk_disable(drvdata->clk);
+}
+
 static inline struct samsung_pin_bank *gc_to_pin_bank(struct gpio_chip *gc)
 {
 	return container_of(gc, struct samsung_pin_bank, gpio_chip);
@@ -374,7 +401,9 @@ static void samsung_pinmux_setup(struct pinctrl_dev *pctldev, unsigned selector,
 	const struct samsung_pmx_func *func;
 	const struct samsung_pin_group *grp;
 
+	pctl_clk_enable(pctldev);
 	drvdata = pinctrl_dev_get_drvdata(pctldev);
+
 	func = &drvdata->pmx_functions[selector];
 	grp = &drvdata->pin_groups[group];
 
@@ -398,6 +427,8 @@ static void samsung_pinmux_setup(struct pinctrl_dev *pctldev, unsigned selector,
 	writel(data, reg + type->reg_offset[PINCFG_TYPE_FUNC]);
 
 	spin_unlock_irqrestore(&bank->slock, flags);
+
+	pctl_clk_disable(pctldev);
 }
 
 /* enable a specified pinmux by writing to registers */
@@ -469,20 +500,37 @@ static int samsung_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
 {
 	int i, ret;
 
+	ret = pctl_clk_enable(pctldev);
+	if (ret)
+		goto out;
+
 	for (i = 0; i < num_configs; i++) {
 		ret = samsung_pinconf_rw(pctldev, pin, &configs[i], true);
 		if (ret < 0)
-			return ret;
+			goto out;
 	} /* for each config */
 
-	return 0;
+out:
+	pctl_clk_disable(pctldev);
+
+	return ret;
 }
 
 /* get the pin config settings for a specified pin */
 static int samsung_pinconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
 					unsigned long *config)
 {
-	return samsung_pinconf_rw(pctldev, pin, config, false);
+	int ret;
+
+	ret = pctl_clk_enable(pctldev);
+	if (ret)
+		return ret;
+
+	ret = samsung_pinconf_rw(pctldev, pin, config, false);
+
+	pctl_clk_disable(pctldev);
+
+	return ret;
 }
 
 /* set the pin config settings for a specified pin group */
@@ -1057,10 +1105,23 @@ static int samsung_pinctrl_probe(struct platform_device *pdev)
 	}
 	drvdata->dev = dev;
 
+	drvdata->clk = clk_get(&pdev->dev, "block");
+	if (!IS_ERR(drvdata->clk)) {
+		ret = clk_prepare_enable(drvdata->clk);
+		if (ret) {
+			dev_err(dev, "failed to enable clk: %d\n", ret);
+			return ret;
+		}
+	} else {
+		drvdata->clk = NULL;
+	}
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	drvdata->virt_base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(drvdata->virt_base))
-		return PTR_ERR(drvdata->virt_base);
+	if (IS_ERR(drvdata->virt_base)) {
+		ret = PTR_ERR(drvdata->virt_base);
+		goto err;
+	}
 
 	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	if (res)
@@ -1068,12 +1129,12 @@ static int samsung_pinctrl_probe(struct platform_device *pdev)
 
 	ret = samsung_gpiolib_register(pdev, drvdata);
 	if (ret)
-		return ret;
+		goto err;
 
 	ret = samsung_pinctrl_register(pdev, drvdata);
 	if (ret) {
 		samsung_gpiolib_unregister(pdev, drvdata);
-		return ret;
+		goto err;
 	}
 
 	if (ctrl->eint_gpio_init)
@@ -1085,6 +1146,22 @@ static int samsung_pinctrl_probe(struct platform_device *pdev)
 
 	/* Add to the global list */
 	list_add_tail(&drvdata->node, &drvdata_list);
+	clk_disable(drvdata->clk); /* Leave prepared */
+
+	return 0;
+
+err:
+	if (drvdata->clk)
+		clk_disable_unprepare(drvdata->clk);
+
+	return ret;
+}
+
+static int samsung_pinctrl_remove(struct platform_device *pdev)
+{
+	struct samsung_pinctrl_drv_data *drvdata = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(drvdata->clk);
 
 	return 0;
 }
@@ -1102,6 +1179,13 @@ static void samsung_pinctrl_suspend_dev(
 	void __iomem *virt_base = drvdata->virt_base;
 	int i;
 
+	if (drvdata->clk) {
+		if (clk_enable(drvdata->clk)) {
+			dev_err(drvdata->dev, "failed to enable clock\n");
+			return;
+		}
+	}
+
 	for (i = 0; i < drvdata->nr_banks; i++) {
 		struct samsung_pin_bank *bank = &drvdata->pin_banks[i];
 		void __iomem *reg = virt_base + bank->pctl_offset;
@@ -1133,6 +1217,8 @@ static void samsung_pinctrl_suspend_dev(
 
 	if (drvdata->suspend)
 		drvdata->suspend(drvdata);
+
+	clk_disable(drvdata->clk);
 }
 
 /**
@@ -1148,6 +1234,13 @@ static void samsung_pinctrl_resume_dev(struct samsung_pinctrl_drv_data *drvdata)
 	void __iomem *virt_base = drvdata->virt_base;
 	int i;
 
+	if (drvdata->clk) {
+		if (clk_enable(drvdata->clk)) {
+			dev_err(drvdata->dev, "failed to enable clock\n");
+			return;
+		}
+	}
+
 	if (drvdata->resume)
 		drvdata->resume(drvdata);
 
@@ -1181,6 +1274,8 @@ static void samsung_pinctrl_resume_dev(struct samsung_pinctrl_drv_data *drvdata)
 			if (widths[type])
 				writel(bank->pm_save[type], reg + offs[type]);
 	}
+
+	clk_disable(drvdata->clk);
 }
 
 /**
@@ -1264,6 +1359,7 @@ MODULE_DEVICE_TABLE(of, samsung_pinctrl_dt_match);
 
 static struct platform_driver samsung_pinctrl_driver = {
 	.probe		= samsung_pinctrl_probe,
+	.remove		= samsung_pinctrl_remove,
 	.driver = {
 		.name	= "samsung-pinctrl",
 		.of_match_table = samsung_pinctrl_dt_match,
diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h
index 1b8c0139d604..666cb23eb9f2 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.h
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.h
@@ -201,6 +201,7 @@ struct samsung_pin_ctrl {
  * struct samsung_pinctrl_drv_data: wrapper for holding driver data together.
  * @node: global list node
  * @virt_base: register base address of the controller.
+ * @clk: Optional clock to enable/disable during setup. May be NULL.
  * @dev: device instance representing the controller.
  * @irq: interrpt number used by the controller to notify gpio interrupts.
  * @ctrl: pin controller instance managed by the driver.
@@ -218,6 +219,7 @@ struct samsung_pinctrl_drv_data {
 	void __iomem			*virt_base;
 	struct device			*dev;
 	int				irq;
+	struct clk			*clk;
 
 	struct pinctrl_desc		pctl;
 	struct pinctrl_dev		*pctl_dev;
-- 
1.9.1

[PATCH v2 4/5] ARM: dts: exynos5420: Add clock for audss pinctrl

[Krzysztof Kozlowski]

The pinctrl for audio subsystem needs 'mau_epll' clock to be enabled in
order to properly access memory during GPIO setup.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 arch/arm/boot/dts/exynos5420-pinctrl.dtsi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5420-pinctrl.dtsi b/arch/arm/boot/dts/exynos5420-pinctrl.dtsi
index ba686e40eac7..c0ca0da36ade 100644
--- a/arch/arm/boot/dts/exynos5420-pinctrl.dtsi
+++ b/arch/arm/boot/dts/exynos5420-pinctrl.dtsi
@@ -696,6 +696,9 @@
 	};
 
 	pinctrl@03860000 {
+		clocks = <&clock CLK_MAU_EPLL>;
+		clock-names = "block";
+
 		gpz: gpz {
 			gpio-controller;
 			#gpio-cells = <2>;
-- 
1.9.1

[PATCH v2 4/5] ARM: dts: exynos5420: Add clock for audss pinctrl

[Krzysztof Kozlowski]

The pinctrl for audio subsystem needs 'mau_epll' clock to be enabled in
order to properly access memory during GPIO setup.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 arch/arm/boot/dts/exynos5420-pinctrl.dtsi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5420-pinctrl.dtsi b/arch/arm/boot/dts/exynos5420-pinctrl.dtsi
index ba686e40eac7..c0ca0da36ade 100644
--- a/arch/arm/boot/dts/exynos5420-pinctrl.dtsi
+++ b/arch/arm/boot/dts/exynos5420-pinctrl.dtsi
@@ -696,6 +696,9 @@
 	};
 
 	pinctrl at 03860000 {
+		clocks = <&clock CLK_MAU_EPLL>;
+		clock-names = "block";
+
 		gpz: gpz {
 			gpio-controller;
 			#gpio-cells = <2>;
-- 
1.9.1

[PATCH v2 5/5] clk: samsung: Fix memory leak of clock gate/divider/mux structures

[Krzysztof Kozlowski]

While fixing audss clock access when domain is gated (commit "clk:
samsung: Fix clock disable failure because domain being gated") generic
code from clk-gate/divider/mux was taken and modified.

This generic code leaks memory allocated for internal structures (struct
clk_gate/clk_divider/clk_mux). Fix the leak by using resourced managed
allocations.

The audss clocks are now attached to platform device.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/clk/samsung/clk-exynos-audss.c | 63 ++++++++++++++--------------------
 1 file changed, 26 insertions(+), 37 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos-audss.c b/drivers/clk/samsung/clk-exynos-audss.c
index 9ec7de866ab4..229d54981825 100644
--- a/drivers/clk/samsung/clk-exynos-audss.c
+++ b/drivers/clk/samsung/clk-exynos-audss.c
@@ -142,8 +142,6 @@ static const struct clk_ops audss_clk_gate_ops = {
 /*
  * A simplified copy of clk-gate.c:clk_register_gate() to mimic
  * clk-gate behavior while using customized ops.
- *
- * TODO: just like clk-gate it leaks memory for struct clk_gate.
  */
 static struct clk *audss_clk_register_gate(struct device *dev, const char *name,
 		const char *parent_name, unsigned long flags, u8 bit_idx)
@@ -153,7 +151,7 @@ static struct clk *audss_clk_register_gate(struct device *dev, const char *name,
 	struct clk_init_data init;
 
 	/* allocate the gate */
-	gate = kzalloc(sizeof(struct clk_gate), GFP_KERNEL);
+	gate = devm_kzalloc(dev, sizeof(struct clk_gate), GFP_KERNEL);
 	if (!gate)
 		return ERR_PTR(-ENOMEM);
 
@@ -172,9 +170,6 @@ static struct clk *audss_clk_register_gate(struct device *dev, const char *name,
 
 	clk = clk_register(dev, &gate->hw);
 
-	if (IS_ERR(clk))
-		kfree(gate);
-
 	return clk;
 }
 
@@ -238,7 +233,7 @@ static struct clk *audss_clk_register_divider(struct device *dev,
 	struct clk_init_data init;
 
 	/* allocate the divider */
-	div = kzalloc(sizeof(struct clk_divider), GFP_KERNEL);
+	div = devm_kzalloc(dev, sizeof(struct clk_divider), GFP_KERNEL);
 	if (!div)
 		return ERR_PTR(-ENOMEM);
 
@@ -260,9 +255,6 @@ static struct clk *audss_clk_register_divider(struct device *dev,
 	/* register the clock */
 	clk = clk_register(dev, &div->hw);
 
-	if (IS_ERR(clk))
-		kfree(div);
-
 	return clk;
 }
 
@@ -319,7 +311,7 @@ static struct clk *audss_clk_register_mux(struct device *dev, const char *name,
 	u32 mask = BIT(width) - 1;
 
 	/* allocate the mux */
-	mux = kzalloc(sizeof(struct clk_mux), GFP_KERNEL);
+	mux = devm_kzalloc(dev, sizeof(struct clk_mux), GFP_KERNEL);
 	if (!mux)
 		return ERR_PTR(-ENOMEM);
 
@@ -340,9 +332,6 @@ static struct clk *audss_clk_register_mux(struct device *dev, const char *name,
 
 	clk = clk_register(dev, &mux->hw);
 
-	if (IS_ERR(clk))
-		kfree(mux);
-
 	return clk;
 }
 
@@ -398,9 +387,9 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
 
 	}
 
-	clk_table[EXYNOS_MOUT_AUDSS] = audss_clk_register_mux(NULL, "mout_audss",
-				mout_audss_p, ARRAY_SIZE(mout_audss_p),
-				CLK_SET_RATE_NO_REPARENT, 0, 1);
+	clk_table[EXYNOS_MOUT_AUDSS] = audss_clk_register_mux(&pdev->dev,
+			"mout_audss", mout_audss_p, ARRAY_SIZE(mout_audss_p),
+			CLK_SET_RATE_NO_REPARENT, 0, 1);
 
 	cdclk = devm_clk_get(&pdev->dev, "cdclk");
 	sclk_audio = devm_clk_get(&pdev->dev, "sclk_audio");
@@ -408,40 +397,40 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
 		mout_i2s_p[1] = __clk_get_name(cdclk);
 	if (!IS_ERR(sclk_audio))
 		mout_i2s_p[2] = __clk_get_name(sclk_audio);
-	clk_table[EXYNOS_MOUT_I2S] = audss_clk_register_mux(NULL, "mout_i2s",
-				mout_i2s_p, ARRAY_SIZE(mout_i2s_p),
-				CLK_SET_RATE_NO_REPARENT, 2, 2);
+	clk_table[EXYNOS_MOUT_I2S] = audss_clk_register_mux(&pdev->dev,
+			"mout_i2s", mout_i2s_p, ARRAY_SIZE(mout_i2s_p),
+			CLK_SET_RATE_NO_REPARENT, 2, 2);
 
-	clk_table[EXYNOS_DOUT_SRP] = audss_clk_register_divider(NULL, "dout_srp",
-				"mout_audss", 0, 0, 4);
+	clk_table[EXYNOS_DOUT_SRP] = audss_clk_register_divider(&pdev->dev,
+			"dout_srp", "mout_audss", 0, 0, 4);
 
-	clk_table[EXYNOS_DOUT_AUD_BUS] = audss_clk_register_divider(NULL,
+	clk_table[EXYNOS_DOUT_AUD_BUS] = audss_clk_register_divider(&pdev->dev,
 				"dout_aud_bus", "dout_srp", 0, 4, 4);
 
-	clk_table[EXYNOS_DOUT_I2S] = audss_clk_register_divider(NULL, "dout_i2s",
-				"mout_i2s", 0, 8, 4);
+	clk_table[EXYNOS_DOUT_I2S] = audss_clk_register_divider(&pdev->dev,
+			"dout_i2s", "mout_i2s", 0, 8, 4);
 
-	clk_table[EXYNOS_SRP_CLK] = audss_clk_register_gate(NULL, "srp_clk",
-				"dout_srp", CLK_SET_RATE_PARENT, 0);
+	clk_table[EXYNOS_SRP_CLK] = audss_clk_register_gate(&pdev->dev,
+			"srp_clk", "dout_srp", CLK_SET_RATE_PARENT, 0);
 
-	clk_table[EXYNOS_I2S_BUS] = audss_clk_register_gate(NULL, "i2s_bus",
-				"dout_aud_bus", CLK_SET_RATE_PARENT, 2);
+	clk_table[EXYNOS_I2S_BUS] = audss_clk_register_gate(&pdev->dev,
+			"i2s_bus", "dout_aud_bus", CLK_SET_RATE_PARENT, 2);
 
-	clk_table[EXYNOS_SCLK_I2S] = audss_clk_register_gate(NULL, "sclk_i2s",
-				"dout_i2s", CLK_SET_RATE_PARENT, 3);
+	clk_table[EXYNOS_SCLK_I2S] = audss_clk_register_gate(&pdev->dev,
+			"sclk_i2s", "dout_i2s", CLK_SET_RATE_PARENT, 3);
 
-	clk_table[EXYNOS_PCM_BUS] = audss_clk_register_gate(NULL, "pcm_bus",
-				 "sclk_pcm", CLK_SET_RATE_PARENT, 4);
+	clk_table[EXYNOS_PCM_BUS] = audss_clk_register_gate(&pdev->dev,
+			"pcm_bus", "sclk_pcm", CLK_SET_RATE_PARENT, 4);
 
 	sclk_pcm_in = devm_clk_get(&pdev->dev, "sclk_pcm_in");
 	if (!IS_ERR(sclk_pcm_in))
 		sclk_pcm_p = __clk_get_name(sclk_pcm_in);
-	clk_table[EXYNOS_SCLK_PCM] = audss_clk_register_gate(NULL, "sclk_pcm",
-				sclk_pcm_p, CLK_SET_RATE_PARENT, 5);
+	clk_table[EXYNOS_SCLK_PCM] = audss_clk_register_gate(&pdev->dev,
+			"sclk_pcm", sclk_pcm_p, CLK_SET_RATE_PARENT, 5);
 
 	if (variant == TYPE_EXYNOS5420) {
-		clk_table[EXYNOS_ADMA] = audss_clk_register_gate(NULL, "adma",
-				"dout_srp", CLK_SET_RATE_PARENT, 9);
+		clk_table[EXYNOS_ADMA] = audss_clk_register_gate(&pdev->dev,
+				"adma", "dout_srp", CLK_SET_RATE_PARENT, 9);
 	}
 
 	for (i = 0; i < clk_data.clk_num; i++) {
-- 
1.9.1

[PATCH v2 5/5] clk: samsung: Fix memory leak of clock gate/divider/mux structures

[Krzysztof Kozlowski]

While fixing audss clock access when domain is gated (commit "clk:
samsung: Fix clock disable failure because domain being gated") generic
code from clk-gate/divider/mux was taken and modified.

This generic code leaks memory allocated for internal structures (struct
clk_gate/clk_divider/clk_mux). Fix the leak by using resourced managed
allocations.

The audss clocks are now attached to platform device.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/clk/samsung/clk-exynos-audss.c | 63 ++++++++++++++--------------------
 1 file changed, 26 insertions(+), 37 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos-audss.c b/drivers/clk/samsung/clk-exynos-audss.c
index 9ec7de866ab4..229d54981825 100644
--- a/drivers/clk/samsung/clk-exynos-audss.c
+++ b/drivers/clk/samsung/clk-exynos-audss.c
@@ -142,8 +142,6 @@ static const struct clk_ops audss_clk_gate_ops = {
 /*
  * A simplified copy of clk-gate.c:clk_register_gate() to mimic
  * clk-gate behavior while using customized ops.
- *
- * TODO: just like clk-gate it leaks memory for struct clk_gate.
  */
 static struct clk *audss_clk_register_gate(struct device *dev, const char *name,
 		const char *parent_name, unsigned long flags, u8 bit_idx)
@@ -153,7 +151,7 @@ static struct clk *audss_clk_register_gate(struct device *dev, const char *name,
 	struct clk_init_data init;
 
 	/* allocate the gate */
-	gate = kzalloc(sizeof(struct clk_gate), GFP_KERNEL);
+	gate = devm_kzalloc(dev, sizeof(struct clk_gate), GFP_KERNEL);
 	if (!gate)
 		return ERR_PTR(-ENOMEM);
 
@@ -172,9 +170,6 @@ static struct clk *audss_clk_register_gate(struct device *dev, const char *name,
 
 	clk = clk_register(dev, &gate->hw);
 
-	if (IS_ERR(clk))
-		kfree(gate);
-
 	return clk;
 }
 
@@ -238,7 +233,7 @@ static struct clk *audss_clk_register_divider(struct device *dev,
 	struct clk_init_data init;
 
 	/* allocate the divider */
-	div = kzalloc(sizeof(struct clk_divider), GFP_KERNEL);
+	div = devm_kzalloc(dev, sizeof(struct clk_divider), GFP_KERNEL);
 	if (!div)
 		return ERR_PTR(-ENOMEM);
 
@@ -260,9 +255,6 @@ static struct clk *audss_clk_register_divider(struct device *dev,
 	/* register the clock */
 	clk = clk_register(dev, &div->hw);
 
-	if (IS_ERR(clk))
-		kfree(div);
-
 	return clk;
 }
 
@@ -319,7 +311,7 @@ static struct clk *audss_clk_register_mux(struct device *dev, const char *name,
 	u32 mask = BIT(width) - 1;
 
 	/* allocate the mux */
-	mux = kzalloc(sizeof(struct clk_mux), GFP_KERNEL);
+	mux = devm_kzalloc(dev, sizeof(struct clk_mux), GFP_KERNEL);
 	if (!mux)
 		return ERR_PTR(-ENOMEM);
 
@@ -340,9 +332,6 @@ static struct clk *audss_clk_register_mux(struct device *dev, const char *name,
 
 	clk = clk_register(dev, &mux->hw);
 
-	if (IS_ERR(clk))
-		kfree(mux);
-
 	return clk;
 }
 
@@ -398,9 +387,9 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
 
 	}
 
-	clk_table[EXYNOS_MOUT_AUDSS] = audss_clk_register_mux(NULL, "mout_audss",
-				mout_audss_p, ARRAY_SIZE(mout_audss_p),
-				CLK_SET_RATE_NO_REPARENT, 0, 1);
+	clk_table[EXYNOS_MOUT_AUDSS] = audss_clk_register_mux(&pdev->dev,
+			"mout_audss", mout_audss_p, ARRAY_SIZE(mout_audss_p),
+			CLK_SET_RATE_NO_REPARENT, 0, 1);
 
 	cdclk = devm_clk_get(&pdev->dev, "cdclk");
 	sclk_audio = devm_clk_get(&pdev->dev, "sclk_audio");
@@ -408,40 +397,40 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
 		mout_i2s_p[1] = __clk_get_name(cdclk);
 	if (!IS_ERR(sclk_audio))
 		mout_i2s_p[2] = __clk_get_name(sclk_audio);
-	clk_table[EXYNOS_MOUT_I2S] = audss_clk_register_mux(NULL, "mout_i2s",
-				mout_i2s_p, ARRAY_SIZE(mout_i2s_p),
-				CLK_SET_RATE_NO_REPARENT, 2, 2);
+	clk_table[EXYNOS_MOUT_I2S] = audss_clk_register_mux(&pdev->dev,
+			"mout_i2s", mout_i2s_p, ARRAY_SIZE(mout_i2s_p),
+			CLK_SET_RATE_NO_REPARENT, 2, 2);
 
-	clk_table[EXYNOS_DOUT_SRP] = audss_clk_register_divider(NULL, "dout_srp",
-				"mout_audss", 0, 0, 4);
+	clk_table[EXYNOS_DOUT_SRP] = audss_clk_register_divider(&pdev->dev,
+			"dout_srp", "mout_audss", 0, 0, 4);
 
-	clk_table[EXYNOS_DOUT_AUD_BUS] = audss_clk_register_divider(NULL,
+	clk_table[EXYNOS_DOUT_AUD_BUS] = audss_clk_register_divider(&pdev->dev,
 				"dout_aud_bus", "dout_srp", 0, 4, 4);
 
-	clk_table[EXYNOS_DOUT_I2S] = audss_clk_register_divider(NULL, "dout_i2s",
-				"mout_i2s", 0, 8, 4);
+	clk_table[EXYNOS_DOUT_I2S] = audss_clk_register_divider(&pdev->dev,
+			"dout_i2s", "mout_i2s", 0, 8, 4);
 
-	clk_table[EXYNOS_SRP_CLK] = audss_clk_register_gate(NULL, "srp_clk",
-				"dout_srp", CLK_SET_RATE_PARENT, 0);
+	clk_table[EXYNOS_SRP_CLK] = audss_clk_register_gate(&pdev->dev,
+			"srp_clk", "dout_srp", CLK_SET_RATE_PARENT, 0);
 
-	clk_table[EXYNOS_I2S_BUS] = audss_clk_register_gate(NULL, "i2s_bus",
-				"dout_aud_bus", CLK_SET_RATE_PARENT, 2);
+	clk_table[EXYNOS_I2S_BUS] = audss_clk_register_gate(&pdev->dev,
+			"i2s_bus", "dout_aud_bus", CLK_SET_RATE_PARENT, 2);
 
-	clk_table[EXYNOS_SCLK_I2S] = audss_clk_register_gate(NULL, "sclk_i2s",
-				"dout_i2s", CLK_SET_RATE_PARENT, 3);
+	clk_table[EXYNOS_SCLK_I2S] = audss_clk_register_gate(&pdev->dev,
+			"sclk_i2s", "dout_i2s", CLK_SET_RATE_PARENT, 3);
 
-	clk_table[EXYNOS_PCM_BUS] = audss_clk_register_gate(NULL, "pcm_bus",
-				 "sclk_pcm", CLK_SET_RATE_PARENT, 4);
+	clk_table[EXYNOS_PCM_BUS] = audss_clk_register_gate(&pdev->dev,
+			"pcm_bus", "sclk_pcm", CLK_SET_RATE_PARENT, 4);
 
 	sclk_pcm_in = devm_clk_get(&pdev->dev, "sclk_pcm_in");
 	if (!IS_ERR(sclk_pcm_in))
 		sclk_pcm_p = __clk_get_name(sclk_pcm_in);
-	clk_table[EXYNOS_SCLK_PCM] = audss_clk_register_gate(NULL, "sclk_pcm",
-				sclk_pcm_p, CLK_SET_RATE_PARENT, 5);
+	clk_table[EXYNOS_SCLK_PCM] = audss_clk_register_gate(&pdev->dev,
+			"sclk_pcm", sclk_pcm_p, CLK_SET_RATE_PARENT, 5);
 
 	if (variant == TYPE_EXYNOS5420) {
-		clk_table[EXYNOS_ADMA] = audss_clk_register_gate(NULL, "adma",
-				"dout_srp", CLK_SET_RATE_PARENT, 9);
+		clk_table[EXYNOS_ADMA] = audss_clk_register_gate(&pdev->dev,
+				"adma", "dout_srp", CLK_SET_RATE_PARENT, 9);
 	}
 
 	for (i = 0; i < clk_data.clk_num; i++) {
-- 
1.9.1

[PATCH v2 5/5] clk: samsung: Fix memory leak of clock gate/divider/mux structures

[Krzysztof Kozlowski]

While fixing audss clock access when domain is gated (commit "clk:
samsung: Fix clock disable failure because domain being gated") generic
code from clk-gate/divider/mux was taken and modified.

This generic code leaks memory allocated for internal structures (struct
clk_gate/clk_divider/clk_mux). Fix the leak by using resourced managed
allocations.

The audss clocks are now attached to platform device.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/clk/samsung/clk-exynos-audss.c | 63 ++++++++++++++--------------------
 1 file changed, 26 insertions(+), 37 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos-audss.c b/drivers/clk/samsung/clk-exynos-audss.c
index 9ec7de866ab4..229d54981825 100644
--- a/drivers/clk/samsung/clk-exynos-audss.c
+++ b/drivers/clk/samsung/clk-exynos-audss.c
@@ -142,8 +142,6 @@ static const struct clk_ops audss_clk_gate_ops = {
 /*
  * A simplified copy of clk-gate.c:clk_register_gate() to mimic
  * clk-gate behavior while using customized ops.
- *
- * TODO: just like clk-gate it leaks memory for struct clk_gate.
  */
 static struct clk *audss_clk_register_gate(struct device *dev, const char *name,
 		const char *parent_name, unsigned long flags, u8 bit_idx)
@@ -153,7 +151,7 @@ static struct clk *audss_clk_register_gate(struct device *dev, const char *name,
 	struct clk_init_data init;
 
 	/* allocate the gate */
-	gate = kzalloc(sizeof(struct clk_gate), GFP_KERNEL);
+	gate = devm_kzalloc(dev, sizeof(struct clk_gate), GFP_KERNEL);
 	if (!gate)
 		return ERR_PTR(-ENOMEM);
 
@@ -172,9 +170,6 @@ static struct clk *audss_clk_register_gate(struct device *dev, const char *name,
 
 	clk = clk_register(dev, &gate->hw);
 
-	if (IS_ERR(clk))
-		kfree(gate);
-
 	return clk;
 }
 
@@ -238,7 +233,7 @@ static struct clk *audss_clk_register_divider(struct device *dev,
 	struct clk_init_data init;
 
 	/* allocate the divider */
-	div = kzalloc(sizeof(struct clk_divider), GFP_KERNEL);
+	div = devm_kzalloc(dev, sizeof(struct clk_divider), GFP_KERNEL);
 	if (!div)
 		return ERR_PTR(-ENOMEM);
 
@@ -260,9 +255,6 @@ static struct clk *audss_clk_register_divider(struct device *dev,
 	/* register the clock */
 	clk = clk_register(dev, &div->hw);
 
-	if (IS_ERR(clk))
-		kfree(div);
-
 	return clk;
 }
 
@@ -319,7 +311,7 @@ static struct clk *audss_clk_register_mux(struct device *dev, const char *name,
 	u32 mask = BIT(width) - 1;
 
 	/* allocate the mux */
-	mux = kzalloc(sizeof(struct clk_mux), GFP_KERNEL);
+	mux = devm_kzalloc(dev, sizeof(struct clk_mux), GFP_KERNEL);
 	if (!mux)
 		return ERR_PTR(-ENOMEM);
 
@@ -340,9 +332,6 @@ static struct clk *audss_clk_register_mux(struct device *dev, const char *name,
 
 	clk = clk_register(dev, &mux->hw);
 
-	if (IS_ERR(clk))
-		kfree(mux);
-
 	return clk;
 }
 
@@ -398,9 +387,9 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
 
 	}
 
-	clk_table[EXYNOS_MOUT_AUDSS] = audss_clk_register_mux(NULL, "mout_audss",
-				mout_audss_p, ARRAY_SIZE(mout_audss_p),
-				CLK_SET_RATE_NO_REPARENT, 0, 1);
+	clk_table[EXYNOS_MOUT_AUDSS] = audss_clk_register_mux(&pdev->dev,
+			"mout_audss", mout_audss_p, ARRAY_SIZE(mout_audss_p),
+			CLK_SET_RATE_NO_REPARENT, 0, 1);
 
 	cdclk = devm_clk_get(&pdev->dev, "cdclk");
 	sclk_audio = devm_clk_get(&pdev->dev, "sclk_audio");
@@ -408,40 +397,40 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
 		mout_i2s_p[1] = __clk_get_name(cdclk);
 	if (!IS_ERR(sclk_audio))
 		mout_i2s_p[2] = __clk_get_name(sclk_audio);
-	clk_table[EXYNOS_MOUT_I2S] = audss_clk_register_mux(NULL, "mout_i2s",
-				mout_i2s_p, ARRAY_SIZE(mout_i2s_p),
-				CLK_SET_RATE_NO_REPARENT, 2, 2);
+	clk_table[EXYNOS_MOUT_I2S] = audss_clk_register_mux(&pdev->dev,
+			"mout_i2s", mout_i2s_p, ARRAY_SIZE(mout_i2s_p),
+			CLK_SET_RATE_NO_REPARENT, 2, 2);
 
-	clk_table[EXYNOS_DOUT_SRP] = audss_clk_register_divider(NULL, "dout_srp",
-				"mout_audss", 0, 0, 4);
+	clk_table[EXYNOS_DOUT_SRP] = audss_clk_register_divider(&pdev->dev,
+			"dout_srp", "mout_audss", 0, 0, 4);
 
-	clk_table[EXYNOS_DOUT_AUD_BUS] = audss_clk_register_divider(NULL,
+	clk_table[EXYNOS_DOUT_AUD_BUS] = audss_clk_register_divider(&pdev->dev,
 				"dout_aud_bus", "dout_srp", 0, 4, 4);
 
-	clk_table[EXYNOS_DOUT_I2S] = audss_clk_register_divider(NULL, "dout_i2s",
-				"mout_i2s", 0, 8, 4);
+	clk_table[EXYNOS_DOUT_I2S] = audss_clk_register_divider(&pdev->dev,
+			"dout_i2s", "mout_i2s", 0, 8, 4);
 
-	clk_table[EXYNOS_SRP_CLK] = audss_clk_register_gate(NULL, "srp_clk",
-				"dout_srp", CLK_SET_RATE_PARENT, 0);
+	clk_table[EXYNOS_SRP_CLK] = audss_clk_register_gate(&pdev->dev,
+			"srp_clk", "dout_srp", CLK_SET_RATE_PARENT, 0);
 
-	clk_table[EXYNOS_I2S_BUS] = audss_clk_register_gate(NULL, "i2s_bus",
-				"dout_aud_bus", CLK_SET_RATE_PARENT, 2);
+	clk_table[EXYNOS_I2S_BUS] = audss_clk_register_gate(&pdev->dev,
+			"i2s_bus", "dout_aud_bus", CLK_SET_RATE_PARENT, 2);
 
-	clk_table[EXYNOS_SCLK_I2S] = audss_clk_register_gate(NULL, "sclk_i2s",
-				"dout_i2s", CLK_SET_RATE_PARENT, 3);
+	clk_table[EXYNOS_SCLK_I2S] = audss_clk_register_gate(&pdev->dev,
+			"sclk_i2s", "dout_i2s", CLK_SET_RATE_PARENT, 3);
 
-	clk_table[EXYNOS_PCM_BUS] = audss_clk_register_gate(NULL, "pcm_bus",
-				 "sclk_pcm", CLK_SET_RATE_PARENT, 4);
+	clk_table[EXYNOS_PCM_BUS] = audss_clk_register_gate(&pdev->dev,
+			"pcm_bus", "sclk_pcm", CLK_SET_RATE_PARENT, 4);
 
 	sclk_pcm_in = devm_clk_get(&pdev->dev, "sclk_pcm_in");
 	if (!IS_ERR(sclk_pcm_in))
 		sclk_pcm_p = __clk_get_name(sclk_pcm_in);
-	clk_table[EXYNOS_SCLK_PCM] = audss_clk_register_gate(NULL, "sclk_pcm",
-				sclk_pcm_p, CLK_SET_RATE_PARENT, 5);
+	clk_table[EXYNOS_SCLK_PCM] = audss_clk_register_gate(&pdev->dev,
+			"sclk_pcm", sclk_pcm_p, CLK_SET_RATE_PARENT, 5);
 
 	if (variant == TYPE_EXYNOS5420) {
-		clk_table[EXYNOS_ADMA] = audss_clk_register_gate(NULL, "adma",
-				"dout_srp", CLK_SET_RATE_PARENT, 9);
+		clk_table[EXYNOS_ADMA] = audss_clk_register_gate(&pdev->dev,
+				"adma", "dout_srp", CLK_SET_RATE_PARENT, 9);
 	}
 
 	for (i = 0; i < clk_data.clk_num; i++) {
-- 
1.9.1

Re: [PATCH v2 0/5] Fix Arndale Octa/Peach Pi boot on Audio subsystem clocks

[Javier Martinez Canillas]

Hello Krzysztof,

On 11/26/2014 03:24 PM, Krzysztof Kozlowski wrote:
> Description
> ===========
> This patchset tries to solve dependency between AudioSS components
> (clocks and GPIO) and main clock controller on Exynos platform.
> 
> This solves boot failure of Peach Pi/Pit and Arndale Octa [1].
> 

I tested and patches #2, #3, #4 solves the boot failure I was facing.
So for the whole series on an Exynos5420 Peach Pit:

Tested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>

Thanks a lot for solving this issue.

Best regards,
Javier

[PATCH v2 0/5] Fix Arndale Octa/Peach Pi boot on Audio subsystem clocks

[Javier Martinez Canillas]

Hello Krzysztof,

On 11/26/2014 03:24 PM, Krzysztof Kozlowski wrote:
> Description
> ===========
> This patchset tries to solve dependency between AudioSS components
> (clocks and GPIO) and main clock controller on Exynos platform.
> 
> This solves boot failure of Peach Pi/Pit and Arndale Octa [1].
> 

I tested and patches #2, #3, #4 solves the boot failure I was facing.
So for the whole series on an Exynos5420 Peach Pit:

Tested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>

Thanks a lot for solving this issue.

Best regards,
Javier

Re: [PATCH v2 3/5] pinctrl: exynos: Fix GPIO setup failure because domain clock being gated

[Linus Walleij]

On Wed, Nov 26, 2014 at 3:24 PM, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:

> The audio subsystem on Exynos 5420 has separate clocks and GPIO. To
> operate properly on GPIOs the main block clock 'mau_epll' must be
> enabled.
>
> This was observed on Peach Pi/Pit and Arndale Octa (after enabling i2s0)
> after introducing runtime PM to pl330 DMA driver. After that commit the
> 'mau_epll' was gated, because the "amba" clock was disabled and there
> were no more users of mau_epll.
>
> The system hang just before probing i2s0 because
> samsung_pinmux_setup() tried to access memory from audss block which was
> gated.
>
> Add a clock property to the pinctrl driver and enable the clock during
> GPIO setup. During normal GPIO operations (set, get, set_direction) the
> clock is not enabled.
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Waiting for Tomasz to review this.

Can this patch be applied in separation from the others?

Yours,
Linus Walleij

Re: [PATCH v2 3/5] pinctrl: exynos: Fix GPIO setup failure because domain clock being gated

[Linus Walleij]

On Wed, Nov 26, 2014 at 3:24 PM, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:

> The audio subsystem on Exynos 5420 has separate clocks and GPIO. To
> operate properly on GPIOs the main block clock 'mau_epll' must be
> enabled.
>
> This was observed on Peach Pi/Pit and Arndale Octa (after enabling i2s0)
> after introducing runtime PM to pl330 DMA driver. After that commit the
> 'mau_epll' was gated, because the "amba" clock was disabled and there
> were no more users of mau_epll.
>
> The system hang just before probing i2s0 because
> samsung_pinmux_setup() tried to access memory from audss block which was
> gated.
>
> Add a clock property to the pinctrl driver and enable the clock during
> GPIO setup. During normal GPIO operations (set, get, set_direction) the
> clock is not enabled.
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Waiting for Tomasz to review this.

Can this patch be applied in separation from the others?

Yours,
Linus Walleij

[PATCH v2 3/5] pinctrl: exynos: Fix GPIO setup failure because domain clock being gated

[Linus Walleij]

On Wed, Nov 26, 2014 at 3:24 PM, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:

> The audio subsystem on Exynos 5420 has separate clocks and GPIO. To
> operate properly on GPIOs the main block clock 'mau_epll' must be
> enabled.
>
> This was observed on Peach Pi/Pit and Arndale Octa (after enabling i2s0)
> after introducing runtime PM to pl330 DMA driver. After that commit the
> 'mau_epll' was gated, because the "amba" clock was disabled and there
> were no more users of mau_epll.
>
> The system hang just before probing i2s0 because
> samsung_pinmux_setup() tried to access memory from audss block which was
> gated.
>
> Add a clock property to the pinctrl driver and enable the clock during
> GPIO setup. During normal GPIO operations (set, get, set_direction) the
> clock is not enabled.
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Waiting for Tomasz to review this.

Can this patch be applied in separation from the others?

Yours,
Linus Walleij

Re: [PATCH v2 3/5] pinctrl: exynos: Fix GPIO setup failure because domain clock being gated

[Krzysztof Kozlowski]

On pią, 2014-11-28 at 15:04 +0100, Linus Walleij wrote:
> On Wed, Nov 26, 2014 at 3:24 PM, Krzysztof Kozlowski
> <k.kozlowski@samsung.com> wrote:
> 
> > The audio subsystem on Exynos 5420 has separate clocks and GPIO. To
> > operate properly on GPIOs the main block clock 'mau_epll' must be
> > enabled.
> >
> > This was observed on Peach Pi/Pit and Arndale Octa (after enabling i2s0)
> > after introducing runtime PM to pl330 DMA driver. After that commit the
> > 'mau_epll' was gated, because the "amba" clock was disabled and there
> > were no more users of mau_epll.
> >
> > The system hang just before probing i2s0 because
> > samsung_pinmux_setup() tried to access memory from audss block which was
> > gated.
> >
> > Add a clock property to the pinctrl driver and enable the clock during
> > GPIO setup. During normal GPIO operations (set, get, set_direction) the
> > clock is not enabled.
> >
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> 
> Waiting for Tomasz to review this.
> 
> Can this patch be applied in separation from the others?

Yes, it can be picked independently.

The commit message is somehow misleading because issue is actually fixed
by enabling this in DTS. So the next patch (4/5: ARM: dts: exynos5420:
Add clock for audss pinctrl) actually fixes the issue on Arndale Octa
board from pinctrl perspective. Unfortunately I spot that mistake (in
commit msg) later...

Best regards,
Krzysztof

Re: [PATCH v2 3/5] pinctrl: exynos: Fix GPIO setup failure because domain clock being gated

[Krzysztof Kozlowski]

On pią, 2014-11-28 at 15:04 +0100, Linus Walleij wrote:
> On Wed, Nov 26, 2014 at 3:24 PM, Krzysztof Kozlowski
> <k.kozlowski@samsung.com> wrote:
> 
> > The audio subsystem on Exynos 5420 has separate clocks and GPIO. To
> > operate properly on GPIOs the main block clock 'mau_epll' must be
> > enabled.
> >
> > This was observed on Peach Pi/Pit and Arndale Octa (after enabling i2s0)
> > after introducing runtime PM to pl330 DMA driver. After that commit the
> > 'mau_epll' was gated, because the "amba" clock was disabled and there
> > were no more users of mau_epll.
> >
> > The system hang just before probing i2s0 because
> > samsung_pinmux_setup() tried to access memory from audss block which was
> > gated.
> >
> > Add a clock property to the pinctrl driver and enable the clock during
> > GPIO setup. During normal GPIO operations (set, get, set_direction) the
> > clock is not enabled.
> >
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> 
> Waiting for Tomasz to review this.
> 
> Can this patch be applied in separation from the others?

Yes, it can be picked independently.

The commit message is somehow misleading because issue is actually fixed
by enabling this in DTS. So the next patch (4/5: ARM: dts: exynos5420:
Add clock for audss pinctrl) actually fixes the issue on Arndale Octa
board from pinctrl perspective. Unfortunately I spot that mistake (in
commit msg) later...

Best regards,
Krzysztof

[PATCH v2 3/5] pinctrl: exynos: Fix GPIO setup failure because domain clock being gated

[Krzysztof Kozlowski]

On pi?, 2014-11-28 at 15:04 +0100, Linus Walleij wrote:
> On Wed, Nov 26, 2014 at 3:24 PM, Krzysztof Kozlowski
> <k.kozlowski@samsung.com> wrote:
> 
> > The audio subsystem on Exynos 5420 has separate clocks and GPIO. To
> > operate properly on GPIOs the main block clock 'mau_epll' must be
> > enabled.
> >
> > This was observed on Peach Pi/Pit and Arndale Octa (after enabling i2s0)
> > after introducing runtime PM to pl330 DMA driver. After that commit the
> > 'mau_epll' was gated, because the "amba" clock was disabled and there
> > were no more users of mau_epll.
> >
> > The system hang just before probing i2s0 because
> > samsung_pinmux_setup() tried to access memory from audss block which was
> > gated.
> >
> > Add a clock property to the pinctrl driver and enable the clock during
> > GPIO setup. During normal GPIO operations (set, get, set_direction) the
> > clock is not enabled.
> >
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> 
> Waiting for Tomasz to review this.
> 
> Can this patch be applied in separation from the others?

Yes, it can be picked independently.

The commit message is somehow misleading because issue is actually fixed
by enabling this in DTS. So the next patch (4/5: ARM: dts: exynos5420:
Add clock for audss pinctrl) actually fixes the issue on Arndale Octa
board from pinctrl perspective. Unfortunately I spot that mistake (in
commit msg) later...

Best regards,
Krzysztof

Re: [PATCH v2 3/5] pinctrl: exynos: Fix GPIO setup failure because domain clock being gated

[Tomasz Figa]

Hi Krzysztof,

2014-11-28 23:08 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@samsung.com>:
> On pią, 2014-11-28 at 15:04 +0100, Linus Walleij wrote:
>> On Wed, Nov 26, 2014 at 3:24 PM, Krzysztof Kozlowski
>> <k.kozlowski@samsung.com> wrote:
>>
>> > The audio subsystem on Exynos 5420 has separate clocks and GPIO. To
>> > operate properly on GPIOs the main block clock 'mau_epll' must be
>> > enabled.
>> >
>> > This was observed on Peach Pi/Pit and Arndale Octa (after enabling i2s0)
>> > after introducing runtime PM to pl330 DMA driver. After that commit the
>> > 'mau_epll' was gated, because the "amba" clock was disabled and there
>> > were no more users of mau_epll.
>> >
>> > The system hang just before probing i2s0 because
>> > samsung_pinmux_setup() tried to access memory from audss block which was
>> > gated.
>> >
>> > Add a clock property to the pinctrl driver and enable the clock during
>> > GPIO setup. During normal GPIO operations (set, get, set_direction) the
>> > clock is not enabled.

Could you make sure that possibility of gating this clock is worth the
effort of adding gating code to all affected drivers? If there is no
significant change in power consumption maybe it could be simply keep
running all the time?

Also isn't a similar problem happening due to power domains? I believe
the whole maudio block is located in a separate power domain but
somehow it doesn't get turned off?

Could you investigate the above, please?

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/5] pinctrl: exynos: Fix GPIO setup failure because domain clock being gated

[Tomasz Figa]

Hi Krzysztof,

2014-11-28 23:08 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@samsung.com>:
> On pią, 2014-11-28 at 15:04 +0100, Linus Walleij wrote:
>> On Wed, Nov 26, 2014 at 3:24 PM, Krzysztof Kozlowski
>> <k.kozlowski@samsung.com> wrote:
>>
>> > The audio subsystem on Exynos 5420 has separate clocks and GPIO. To
>> > operate properly on GPIOs the main block clock 'mau_epll' must be
>> > enabled.
>> >
>> > This was observed on Peach Pi/Pit and Arndale Octa (after enabling i2s0)
>> > after introducing runtime PM to pl330 DMA driver. After that commit the
>> > 'mau_epll' was gated, because the "amba" clock was disabled and there
>> > were no more users of mau_epll.
>> >
>> > The system hang just before probing i2s0 because
>> > samsung_pinmux_setup() tried to access memory from audss block which was
>> > gated.
>> >
>> > Add a clock property to the pinctrl driver and enable the clock during
>> > GPIO setup. During normal GPIO operations (set, get, set_direction) the
>> > clock is not enabled.

Could you make sure that possibility of gating this clock is worth the
effort of adding gating code to all affected drivers? If there is no
significant change in power consumption maybe it could be simply keep
running all the time?

Also isn't a similar problem happening due to power domains? I believe
the whole maudio block is located in a separate power domain but
somehow it doesn't get turned off?

Could you investigate the above, please?

Best regards,
Tomasz

[PATCH v2 3/5] pinctrl: exynos: Fix GPIO setup failure because domain clock being gated

[Tomasz Figa]

Hi Krzysztof,

2014-11-28 23:08 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@samsung.com>:
> On pi?, 2014-11-28 at 15:04 +0100, Linus Walleij wrote:
>> On Wed, Nov 26, 2014 at 3:24 PM, Krzysztof Kozlowski
>> <k.kozlowski@samsung.com> wrote:
>>
>> > The audio subsystem on Exynos 5420 has separate clocks and GPIO. To
>> > operate properly on GPIOs the main block clock 'mau_epll' must be
>> > enabled.
>> >
>> > This was observed on Peach Pi/Pit and Arndale Octa (after enabling i2s0)
>> > after introducing runtime PM to pl330 DMA driver. After that commit the
>> > 'mau_epll' was gated, because the "amba" clock was disabled and there
>> > were no more users of mau_epll.
>> >
>> > The system hang just before probing i2s0 because
>> > samsung_pinmux_setup() tried to access memory from audss block which was
>> > gated.
>> >
>> > Add a clock property to the pinctrl driver and enable the clock during
>> > GPIO setup. During normal GPIO operations (set, get, set_direction) the
>> > clock is not enabled.

Could you make sure that possibility of gating this clock is worth the
effort of adding gating code to all affected drivers? If there is no
significant change in power consumption maybe it could be simply keep
running all the time?

Also isn't a similar problem happening due to power domains? I believe
the whole maudio block is located in a separate power domain but
somehow it doesn't get turned off?

Could you investigate the above, please?

Best regards,
Tomasz

Re: [PATCH v2 3/5] pinctrl: exynos: Fix GPIO setup failure because domain clock being gated

[Krzysztof Kozlowski]

On nie, 2014-11-30 at 21:19 +0900, Tomasz Figa wrote:
> Hi Krzysztof,
> 
> 2014-11-28 23:08 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@samsung.com>:
> > On pią, 2014-11-28 at 15:04 +0100, Linus Walleij wrote:
> >> On Wed, Nov 26, 2014 at 3:24 PM, Krzysztof Kozlowski
> >> <k.kozlowski@samsung.com> wrote:
> >>
> >> > The audio subsystem on Exynos 5420 has separate clocks and GPIO. To
> >> > operate properly on GPIOs the main block clock 'mau_epll' must be
> >> > enabled.
> >> >
> >> > This was observed on Peach Pi/Pit and Arndale Octa (after enabling i2s0)
> >> > after introducing runtime PM to pl330 DMA driver. After that commit the
> >> > 'mau_epll' was gated, because the "amba" clock was disabled and there
> >> > were no more users of mau_epll.
> >> >
> >> > The system hang just before probing i2s0 because
> >> > samsung_pinmux_setup() tried to access memory from audss block which was
> >> > gated.
> >> >
> >> > Add a clock property to the pinctrl driver and enable the clock during
> >> > GPIO setup. During normal GPIO operations (set, get, set_direction) the
> >> > clock is not enabled.
> 
> Could you make sure that possibility of gating this clock is worth the
> effort of adding gating code to all affected drivers? If there is no
> significant change in power consumption maybe it could be simply keep
> running all the time?

I had an impression that last time you disliked such idea:
http://www.spinics.net/lists/arm-kernel/msg338127.html
That's why I developed these patches. Because keeping a clock always on,
even when it is unused, is undesirable.

Anyway, I did some simple measurements (after booting Arndale Octa
to /bin/sh, idle):
 - with mau_epll gated: ~523 mA
 - with mau_epll always on: ~531 mA

Keeping it on increases energy usage by 1.5% in idle (with measurement
uncertainty ~0.4%).


> Also isn't a similar problem happening due to power domains? I believe
> the whole maudio block is located in a separate power domain but
> somehow it doesn't get turned off?

There is Maudio power domain... but I think it is not related here.
Pinctrl driver does not have runtime PM and is not attached to a domain.
I thought about other solution to this problem (with utilization of
power domains):
 - add runtime PM to pinctrl and audss clocks,
 - attach pinctrl and audss clocks to maudio power domain,
 - enable the clock when power domain is turned on.
However almost the same changes had to be added to pinctrl and audss
clocks drivers (replace clock_enable() with pm_runtime_get_sync()).

Best regards,
Krzysztof


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

Re: [PATCH v2 3/5] pinctrl: exynos: Fix GPIO setup failure because domain clock being gated

[Krzysztof Kozlowski]

On nie, 2014-11-30 at 21:19 +0900, Tomasz Figa wrote:
> Hi Krzysztof,
> 
> 2014-11-28 23:08 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@samsung.com>:
> > On pią, 2014-11-28 at 15:04 +0100, Linus Walleij wrote:
> >> On Wed, Nov 26, 2014 at 3:24 PM, Krzysztof Kozlowski
> >> <k.kozlowski@samsung.com> wrote:
> >>
> >> > The audio subsystem on Exynos 5420 has separate clocks and GPIO. To
> >> > operate properly on GPIOs the main block clock 'mau_epll' must be
> >> > enabled.
> >> >
> >> > This was observed on Peach Pi/Pit and Arndale Octa (after enabling i2s0)
> >> > after introducing runtime PM to pl330 DMA driver. After that commit the
> >> > 'mau_epll' was gated, because the "amba" clock was disabled and there
> >> > were no more users of mau_epll.
> >> >
> >> > The system hang just before probing i2s0 because
> >> > samsung_pinmux_setup() tried to access memory from audss block which was
> >> > gated.
> >> >
> >> > Add a clock property to the pinctrl driver and enable the clock during
> >> > GPIO setup. During normal GPIO operations (set, get, set_direction) the
> >> > clock is not enabled.
> 
> Could you make sure that possibility of gating this clock is worth the
> effort of adding gating code to all affected drivers? If there is no
> significant change in power consumption maybe it could be simply keep
> running all the time?

I had an impression that last time you disliked such idea:
http://www.spinics.net/lists/arm-kernel/msg338127.html
That's why I developed these patches. Because keeping a clock always on,
even when it is unused, is undesirable.

Anyway, I did some simple measurements (after booting Arndale Octa
to /bin/sh, idle):
 - with mau_epll gated: ~523 mA
 - with mau_epll always on: ~531 mA

Keeping it on increases energy usage by 1.5% in idle (with measurement
uncertainty ~0.4%).


> Also isn't a similar problem happening due to power domains? I believe
> the whole maudio block is located in a separate power domain but
> somehow it doesn't get turned off?

There is Maudio power domain... but I think it is not related here.
Pinctrl driver does not have runtime PM and is not attached to a domain.
I thought about other solution to this problem (with utilization of
power domains):
 - add runtime PM to pinctrl and audss clocks,
 - attach pinctrl and audss clocks to maudio power domain,
 - enable the clock when power domain is turned on.
However almost the same changes had to be added to pinctrl and audss
clocks drivers (replace clock_enable() with pm_runtime_get_sync()).

Best regards,
Krzysztof

[PATCH v2 3/5] pinctrl: exynos: Fix GPIO setup failure because domain clock being gated

[Krzysztof Kozlowski]

On nie, 2014-11-30 at 21:19 +0900, Tomasz Figa wrote:
> Hi Krzysztof,
> 
> 2014-11-28 23:08 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@samsung.com>:
> > On pi?, 2014-11-28 at 15:04 +0100, Linus Walleij wrote:
> >> On Wed, Nov 26, 2014 at 3:24 PM, Krzysztof Kozlowski
> >> <k.kozlowski@samsung.com> wrote:
> >>
> >> > The audio subsystem on Exynos 5420 has separate clocks and GPIO. To
> >> > operate properly on GPIOs the main block clock 'mau_epll' must be
> >> > enabled.
> >> >
> >> > This was observed on Peach Pi/Pit and Arndale Octa (after enabling i2s0)
> >> > after introducing runtime PM to pl330 DMA driver. After that commit the
> >> > 'mau_epll' was gated, because the "amba" clock was disabled and there
> >> > were no more users of mau_epll.
> >> >
> >> > The system hang just before probing i2s0 because
> >> > samsung_pinmux_setup() tried to access memory from audss block which was
> >> > gated.
> >> >
> >> > Add a clock property to the pinctrl driver and enable the clock during
> >> > GPIO setup. During normal GPIO operations (set, get, set_direction) the
> >> > clock is not enabled.
> 
> Could you make sure that possibility of gating this clock is worth the
> effort of adding gating code to all affected drivers? If there is no
> significant change in power consumption maybe it could be simply keep
> running all the time?

I had an impression that last time you disliked such idea:
http://www.spinics.net/lists/arm-kernel/msg338127.html
That's why I developed these patches. Because keeping a clock always on,
even when it is unused, is undesirable.

Anyway, I did some simple measurements (after booting Arndale Octa
to /bin/sh, idle):
 - with mau_epll gated: ~523 mA
 - with mau_epll always on: ~531 mA

Keeping it on increases energy usage by 1.5% in idle (with measurement
uncertainty ~0.4%).


> Also isn't a similar problem happening due to power domains? I believe
> the whole maudio block is located in a separate power domain but
> somehow it doesn't get turned off?

There is Maudio power domain... but I think it is not related here.
Pinctrl driver does not have runtime PM and is not attached to a domain.
I thought about other solution to this problem (with utilization of
power domains):
 - add runtime PM to pinctrl and audss clocks,
 - attach pinctrl and audss clocks to maudio power domain,
 - enable the clock when power domain is turned on.
However almost the same changes had to be added to pinctrl and audss
clocks drivers (replace clock_enable() with pm_runtime_get_sync()).

Best regards,
Krzysztof

Re: [PATCH v2 3/5] pinctrl: exynos: Fix GPIO setup failure because domain clock being gated

[Tomasz Figa]

2014-12-01 17:37 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@samsung.com>:
>
> On nie, 2014-11-30 at 21:19 +0900, Tomasz Figa wrote:
>> Hi Krzysztof,
>>
>> 2014-11-28 23:08 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@samsung.com>:
>> > On pią, 2014-11-28 at 15:04 +0100, Linus Walleij wrote:
>> >> On Wed, Nov 26, 2014 at 3:24 PM, Krzysztof Kozlowski
>> >> <k.kozlowski@samsung.com> wrote:
>> >>
>> >> > The audio subsystem on Exynos 5420 has separate clocks and GPIO. To
>> >> > operate properly on GPIOs the main block clock 'mau_epll' must be
>> >> > enabled.
>> >> >
>> >> > This was observed on Peach Pi/Pit and Arndale Octa (after enabling i2s0)
>> >> > after introducing runtime PM to pl330 DMA driver. After that commit the
>> >> > 'mau_epll' was gated, because the "amba" clock was disabled and there
>> >> > were no more users of mau_epll.
>> >> >
>> >> > The system hang just before probing i2s0 because
>> >> > samsung_pinmux_setup() tried to access memory from audss block which was
>> >> > gated.
>> >> >
>> >> > Add a clock property to the pinctrl driver and enable the clock during
>> >> > GPIO setup. During normal GPIO operations (set, get, set_direction) the
>> >> > clock is not enabled.
>>
>> Could you make sure that possibility of gating this clock is worth the
>> effort of adding gating code to all affected drivers? If there is no
>> significant change in power consumption maybe it could be simply keep
>> running all the time?
>
> I had an impression that last time you disliked such idea:
> http://www.spinics.net/lists/arm-kernel/msg338127.html
> That's why I developed these patches. Because keeping a clock always on,
> even when it is unused, is undesirable.

I was mostly concerned about the particular solution implemented by
that patch that kept the clocks enabled on all platforms, not only the
affected one. However...

>
> Anyway, I did some simple measurements (after booting Arndale Octa
> to /bin/sh, idle):
>  - with mau_epll gated: ~523 mA
>  - with mau_epll always on: ~531 mA
>
> Keeping it on increases energy usage by 1.5% in idle (with measurement
> uncertainty ~0.4%).
>

...this definitely shows that the effort isn't worthless, which means
that your patch goes in the right direction.

>
>> Also isn't a similar problem happening due to power domains? I believe
>> the whole maudio block is located in a separate power domain but
>> somehow it doesn't get turned off?
>
> There is Maudio power domain... but I think it is not related here.

Could you somehow check what happens when the maudio power domain is
turned off and maudio pin controller is accessed? Could you also check
the difference in power consumption with this domain powered on and
off?

> Pinctrl driver does not have runtime PM and is not attached to a domain.
> I thought about other solution to this problem (with utilization of
> power domains):
>  - add runtime PM to pinctrl and audss clocks,
>  - attach pinctrl and audss clocks to maudio power domain,
>  - enable the clock when power domain is turned on.
> However almost the same changes had to be added to pinctrl and audss
> clocks drivers (replace clock_enable() with pm_runtime_get_sync()).

Well, if the dependency is there due to hardware design, I think it
needs to be reflected in the drivers as well.

Few other issues that came to my mind:

 - Your previous patch added clock control only to pinctrl operations.
Shouldn't GPIO operations be included as well?

 - If power domain dependency is there too, what happens with GPIO
pins if the domain is powered off? If they lose their states, wouldn't
it necessary to keep the domain powered on whenever there is some GPIO
pin requested (which usually = in use)? (I'd assume that special
functions shouldn't take a reference on runtime PM, because they are
related to IP blocks in the same PM domain, which will implicitly keep
the domain powered on.)

 - Doesn't the clock controller also lose its state whenever the power
domain is powered off? I guess that would be similar to ISP clock
controller, issues of which are still not resolved in mainline.
Sylwester could tell you more about this, I guess.

Best regards,
Tomasz

Re: [PATCH v2 3/5] pinctrl: exynos: Fix GPIO setup failure because domain clock being gated

[Tomasz Figa]

2014-12-01 17:37 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@samsung.com>:
>
> On nie, 2014-11-30 at 21:19 +0900, Tomasz Figa wrote:
>> Hi Krzysztof,
>>
>> 2014-11-28 23:08 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@samsung.com>:
>> > On pią, 2014-11-28 at 15:04 +0100, Linus Walleij wrote:
>> >> On Wed, Nov 26, 2014 at 3:24 PM, Krzysztof Kozlowski
>> >> <k.kozlowski@samsung.com> wrote:
>> >>
>> >> > The audio subsystem on Exynos 5420 has separate clocks and GPIO. To
>> >> > operate properly on GPIOs the main block clock 'mau_epll' must be
>> >> > enabled.
>> >> >
>> >> > This was observed on Peach Pi/Pit and Arndale Octa (after enabling i2s0)
>> >> > after introducing runtime PM to pl330 DMA driver. After that commit the
>> >> > 'mau_epll' was gated, because the "amba" clock was disabled and there
>> >> > were no more users of mau_epll.
>> >> >
>> >> > The system hang just before probing i2s0 because
>> >> > samsung_pinmux_setup() tried to access memory from audss block which was
>> >> > gated.
>> >> >
>> >> > Add a clock property to the pinctrl driver and enable the clock during
>> >> > GPIO setup. During normal GPIO operations (set, get, set_direction) the
>> >> > clock is not enabled.
>>
>> Could you make sure that possibility of gating this clock is worth the
>> effort of adding gating code to all affected drivers? If there is no
>> significant change in power consumption maybe it could be simply keep
>> running all the time?
>
> I had an impression that last time you disliked such idea:
> http://www.spinics.net/lists/arm-kernel/msg338127.html
> That's why I developed these patches. Because keeping a clock always on,
> even when it is unused, is undesirable.

I was mostly concerned about the particular solution implemented by
that patch that kept the clocks enabled on all platforms, not only the
affected one. However...

>
> Anyway, I did some simple measurements (after booting Arndale Octa
> to /bin/sh, idle):
>  - with mau_epll gated: ~523 mA
>  - with mau_epll always on: ~531 mA
>
> Keeping it on increases energy usage by 1.5% in idle (with measurement
> uncertainty ~0.4%).
>

...this definitely shows that the effort isn't worthless, which means
that your patch goes in the right direction.

>
>> Also isn't a similar problem happening due to power domains? I believe
>> the whole maudio block is located in a separate power domain but
>> somehow it doesn't get turned off?
>
> There is Maudio power domain... but I think it is not related here.

Could you somehow check what happens when the maudio power domain is
turned off and maudio pin controller is accessed? Could you also check
the difference in power consumption with this domain powered on and
off?

> Pinctrl driver does not have runtime PM and is not attached to a domain.
> I thought about other solution to this problem (with utilization of
> power domains):
>  - add runtime PM to pinctrl and audss clocks,
>  - attach pinctrl and audss clocks to maudio power domain,
>  - enable the clock when power domain is turned on.
> However almost the same changes had to be added to pinctrl and audss
> clocks drivers (replace clock_enable() with pm_runtime_get_sync()).

Well, if the dependency is there due to hardware design, I think it
needs to be reflected in the drivers as well.

Few other issues that came to my mind:

 - Your previous patch added clock control only to pinctrl operations.
Shouldn't GPIO operations be included as well?

 - If power domain dependency is there too, what happens with GPIO
pins if the domain is powered off? If they lose their states, wouldn't
it necessary to keep the domain powered on whenever there is some GPIO
pin requested (which usually = in use)? (I'd assume that special
functions shouldn't take a reference on runtime PM, because they are
related to IP blocks in the same PM domain, which will implicitly keep
the domain powered on.)

 - Doesn't the clock controller also lose its state whenever the power
domain is powered off? I guess that would be similar to ISP clock
controller, issues of which are still not resolved in mainline.
Sylwester could tell you more about this, I guess.

Best regards,
Tomasz

[PATCH v2 3/5] pinctrl: exynos: Fix GPIO setup failure because domain clock being gated

[Tomasz Figa]

2014-12-01 17:37 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@samsung.com>:
>
> On nie, 2014-11-30 at 21:19 +0900, Tomasz Figa wrote:
>> Hi Krzysztof,
>>
>> 2014-11-28 23:08 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@samsung.com>:
>> > On pi?, 2014-11-28 at 15:04 +0100, Linus Walleij wrote:
>> >> On Wed, Nov 26, 2014 at 3:24 PM, Krzysztof Kozlowski
>> >> <k.kozlowski@samsung.com> wrote:
>> >>
>> >> > The audio subsystem on Exynos 5420 has separate clocks and GPIO. To
>> >> > operate properly on GPIOs the main block clock 'mau_epll' must be
>> >> > enabled.
>> >> >
>> >> > This was observed on Peach Pi/Pit and Arndale Octa (after enabling i2s0)
>> >> > after introducing runtime PM to pl330 DMA driver. After that commit the
>> >> > 'mau_epll' was gated, because the "amba" clock was disabled and there
>> >> > were no more users of mau_epll.
>> >> >
>> >> > The system hang just before probing i2s0 because
>> >> > samsung_pinmux_setup() tried to access memory from audss block which was
>> >> > gated.
>> >> >
>> >> > Add a clock property to the pinctrl driver and enable the clock during
>> >> > GPIO setup. During normal GPIO operations (set, get, set_direction) the
>> >> > clock is not enabled.
>>
>> Could you make sure that possibility of gating this clock is worth the
>> effort of adding gating code to all affected drivers? If there is no
>> significant change in power consumption maybe it could be simply keep
>> running all the time?
>
> I had an impression that last time you disliked such idea:
> http://www.spinics.net/lists/arm-kernel/msg338127.html
> That's why I developed these patches. Because keeping a clock always on,
> even when it is unused, is undesirable.

I was mostly concerned about the particular solution implemented by
that patch that kept the clocks enabled on all platforms, not only the
affected one. However...

>
> Anyway, I did some simple measurements (after booting Arndale Octa
> to /bin/sh, idle):
>  - with mau_epll gated: ~523 mA
>  - with mau_epll always on: ~531 mA
>
> Keeping it on increases energy usage by 1.5% in idle (with measurement
> uncertainty ~0.4%).
>

...this definitely shows that the effort isn't worthless, which means
that your patch goes in the right direction.

>
>> Also isn't a similar problem happening due to power domains? I believe
>> the whole maudio block is located in a separate power domain but
>> somehow it doesn't get turned off?
>
> There is Maudio power domain... but I think it is not related here.

Could you somehow check what happens when the maudio power domain is
turned off and maudio pin controller is accessed? Could you also check
the difference in power consumption with this domain powered on and
off?

> Pinctrl driver does not have runtime PM and is not attached to a domain.
> I thought about other solution to this problem (with utilization of
> power domains):
>  - add runtime PM to pinctrl and audss clocks,
>  - attach pinctrl and audss clocks to maudio power domain,
>  - enable the clock when power domain is turned on.
> However almost the same changes had to be added to pinctrl and audss
> clocks drivers (replace clock_enable() with pm_runtime_get_sync()).

Well, if the dependency is there due to hardware design, I think it
needs to be reflected in the drivers as well.

Few other issues that came to my mind:

 - Your previous patch added clock control only to pinctrl operations.
Shouldn't GPIO operations be included as well?

 - If power domain dependency is there too, what happens with GPIO
pins if the domain is powered off? If they lose their states, wouldn't
it necessary to keep the domain powered on whenever there is some GPIO
pin requested (which usually = in use)? (I'd assume that special
functions shouldn't take a reference on runtime PM, because they are
related to IP blocks in the same PM domain, which will implicitly keep
the domain powered on.)

 - Doesn't the clock controller also lose its state whenever the power
domain is powered off? I guess that would be similar to ISP clock
controller, issues of which are still not resolved in mainline.
Sylwester could tell you more about this, I guess.

Best regards,
Tomasz

Re: [PATCH v2 3/5] pinctrl: exynos: Fix GPIO setup failure because domain clock being gated

[Krzysztof Kozlowski]

On pon, 2014-12-01 at 23:34 +0900, Tomasz Figa wrote:
> 2014-12-01 17:37 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@samsung.com>:
> >
> > On nie, 2014-11-30 at 21:19 +0900, Tomasz Figa wrote:
> >> Hi Krzysztof,
> >>
> >> 2014-11-28 23:08 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@samsung.com>:
> >> > On pią, 2014-11-28 at 15:04 +0100, Linus Walleij wrote:
> >> >> On Wed, Nov 26, 2014 at 3:24 PM, Krzysztof Kozlowski
> >> >> <k.kozlowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> >> >>
> >> >> > The audio subsystem on Exynos 5420 has separate clocks and GPIO. To
> >> >> > operate properly on GPIOs the main block clock 'mau_epll' must be
> >> >> > enabled.
> >> >> >
> >> >> > This was observed on Peach Pi/Pit and Arndale Octa (after enabling i2s0)
> >> >> > after introducing runtime PM to pl330 DMA driver. After that commit the
> >> >> > 'mau_epll' was gated, because the "amba" clock was disabled and there
> >> >> > were no more users of mau_epll.
> >> >> >
> >> >> > The system hang just before probing i2s0 because
> >> >> > samsung_pinmux_setup() tried to access memory from audss block which was
> >> >> > gated.
> >> >> >
> >> >> > Add a clock property to the pinctrl driver and enable the clock during
> >> >> > GPIO setup. During normal GPIO operations (set, get, set_direction) the
> >> >> > clock is not enabled.
> >>
> >> Could you make sure that possibility of gating this clock is worth the
> >> effort of adding gating code to all affected drivers? If there is no
> >> significant change in power consumption maybe it could be simply keep
> >> running all the time?
> >
> > I had an impression that last time you disliked such idea:
> > http://www.spinics.net/lists/arm-kernel/msg338127.html
> > That's why I developed these patches. Because keeping a clock always on,
> > even when it is unused, is undesirable.
> 
> I was mostly concerned about the particular solution implemented by
> that patch that kept the clocks enabled on all platforms, not only the
> affected one. However...
> 
> >
> > Anyway, I did some simple measurements (after booting Arndale Octa
> > to /bin/sh, idle):
> >  - with mau_epll gated: ~523 mA
> >  - with mau_epll always on: ~531 mA
> >
> > Keeping it on increases energy usage by 1.5% in idle (with measurement
> > uncertainty ~0.4%).
> >
> 
> ...this definitely shows that the effort isn't worthless, which means
> that your patch goes in the right direction.

Great!

> 
> >
> >> Also isn't a similar problem happening due to power domains? I believe
> >> the whole maudio block is located in a separate power domain but
> >> somehow it doesn't get turned off?
> >
> > There is Maudio power domain... but I think it is not related here.
> 
> Could you somehow check what happens when the maudio power domain is
> turned off and maudio pin controller is accessed? Could you also check
> the difference in power consumption with this domain powered on and
> off?

I can try. I'll let you know the results.

> 
> > Pinctrl driver does not have runtime PM and is not attached to a domain.
> > I thought about other solution to this problem (with utilization of
> > power domains):
> >  - add runtime PM to pinctrl and audss clocks,
> >  - attach pinctrl and audss clocks to maudio power domain,
> >  - enable the clock when power domain is turned on.
> > However almost the same changes had to be added to pinctrl and audss
> > clocks drivers (replace clock_enable() with pm_runtime_get_sync()).
> 
> Well, if the dependency is there due to hardware design, I think it
> needs to be reflected in the drivers as well.
> 
> Few other issues that came to my mind:
> 
>  - Your previous patch added clock control only to pinctrl operations.
> Shouldn't GPIO operations be included as well?

Yeah... but does it make any sense? For example imagine GPIO is
configured as input. Enabling the clock won't change anything because
already that clock should be enabled by peripheral which writes to such
GPIO.

>  - If power domain dependency is there too, what happens with GPIO
> pins if the domain is powered off? If they lose their states, wouldn't
> it necessary to keep the domain powered on whenever there is some GPIO
> pin requested (which usually = in use)? (I'd assume that special
> functions shouldn't take a reference on runtime PM, because they are
> related to IP blocks in the same PM domain, which will implicitly keep
> the domain powered on.)

What you're saying makes sense but that look like job for sound soc
driver? Anyway the domain is not present in DTS so by default it is
turned on.

>  - Doesn't the clock controller also lose its state whenever the power
> domain is powered off? I guess that would be similar to ISP clock
> controller, issues of which are still not resolved in mainline.
> Sylwester could tell you more about this, I guess.

Once again - the domain.

Best regards,
Krzysztof


--
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 v2 3/5] pinctrl: exynos: Fix GPIO setup failure because domain clock being gated

[Krzysztof Kozlowski]

On pon, 2014-12-01 at 23:34 +0900, Tomasz Figa wrote:
> 2014-12-01 17:37 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@samsung.com>:
> >
> > On nie, 2014-11-30 at 21:19 +0900, Tomasz Figa wrote:
> >> Hi Krzysztof,
> >>
> >> 2014-11-28 23:08 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@samsung.com>:
> >> > On pią, 2014-11-28 at 15:04 +0100, Linus Walleij wrote:
> >> >> On Wed, Nov 26, 2014 at 3:24 PM, Krzysztof Kozlowski
> >> >> <k.kozlowski@samsung.com> wrote:
> >> >>
> >> >> > The audio subsystem on Exynos 5420 has separate clocks and GPIO. To
> >> >> > operate properly on GPIOs the main block clock 'mau_epll' must be
> >> >> > enabled.
> >> >> >
> >> >> > This was observed on Peach Pi/Pit and Arndale Octa (after enabling i2s0)
> >> >> > after introducing runtime PM to pl330 DMA driver. After that commit the
> >> >> > 'mau_epll' was gated, because the "amba" clock was disabled and there
> >> >> > were no more users of mau_epll.
> >> >> >
> >> >> > The system hang just before probing i2s0 because
> >> >> > samsung_pinmux_setup() tried to access memory from audss block which was
> >> >> > gated.
> >> >> >
> >> >> > Add a clock property to the pinctrl driver and enable the clock during
> >> >> > GPIO setup. During normal GPIO operations (set, get, set_direction) the
> >> >> > clock is not enabled.
> >>
> >> Could you make sure that possibility of gating this clock is worth the
> >> effort of adding gating code to all affected drivers? If there is no
> >> significant change in power consumption maybe it could be simply keep
> >> running all the time?
> >
> > I had an impression that last time you disliked such idea:
> > http://www.spinics.net/lists/arm-kernel/msg338127.html
> > That's why I developed these patches. Because keeping a clock always on,
> > even when it is unused, is undesirable.
> 
> I was mostly concerned about the particular solution implemented by
> that patch that kept the clocks enabled on all platforms, not only the
> affected one. However...
> 
> >
> > Anyway, I did some simple measurements (after booting Arndale Octa
> > to /bin/sh, idle):
> >  - with mau_epll gated: ~523 mA
> >  - with mau_epll always on: ~531 mA
> >
> > Keeping it on increases energy usage by 1.5% in idle (with measurement
> > uncertainty ~0.4%).
> >
> 
> ...this definitely shows that the effort isn't worthless, which means
> that your patch goes in the right direction.

Great!

> 
> >
> >> Also isn't a similar problem happening due to power domains? I believe
> >> the whole maudio block is located in a separate power domain but
> >> somehow it doesn't get turned off?
> >
> > There is Maudio power domain... but I think it is not related here.
> 
> Could you somehow check what happens when the maudio power domain is
> turned off and maudio pin controller is accessed? Could you also check
> the difference in power consumption with this domain powered on and
> off?

I can try. I'll let you know the results.

> 
> > Pinctrl driver does not have runtime PM and is not attached to a domain.
> > I thought about other solution to this problem (with utilization of
> > power domains):
> >  - add runtime PM to pinctrl and audss clocks,
> >  - attach pinctrl and audss clocks to maudio power domain,
> >  - enable the clock when power domain is turned on.
> > However almost the same changes had to be added to pinctrl and audss
> > clocks drivers (replace clock_enable() with pm_runtime_get_sync()).
> 
> Well, if the dependency is there due to hardware design, I think it
> needs to be reflected in the drivers as well.
> 
> Few other issues that came to my mind:
> 
>  - Your previous patch added clock control only to pinctrl operations.
> Shouldn't GPIO operations be included as well?

Yeah... but does it make any sense? For example imagine GPIO is
configured as input. Enabling the clock won't change anything because
already that clock should be enabled by peripheral which writes to such
GPIO.

>  - If power domain dependency is there too, what happens with GPIO
> pins if the domain is powered off? If they lose their states, wouldn't
> it necessary to keep the domain powered on whenever there is some GPIO
> pin requested (which usually = in use)? (I'd assume that special
> functions shouldn't take a reference on runtime PM, because they are
> related to IP blocks in the same PM domain, which will implicitly keep
> the domain powered on.)

What you're saying makes sense but that look like job for sound soc
driver? Anyway the domain is not present in DTS so by default it is
turned on.

>  - Doesn't the clock controller also lose its state whenever the power
> domain is powered off? I guess that would be similar to ISP clock
> controller, issues of which are still not resolved in mainline.
> Sylwester could tell you more about this, I guess.

Once again - the domain.

Best regards,
Krzysztof

[PATCH v2 3/5] pinctrl: exynos: Fix GPIO setup failure because domain clock being gated

[Krzysztof Kozlowski]

On pon, 2014-12-01 at 23:34 +0900, Tomasz Figa wrote:
> 2014-12-01 17:37 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@samsung.com>:
> >
> > On nie, 2014-11-30 at 21:19 +0900, Tomasz Figa wrote:
> >> Hi Krzysztof,
> >>
> >> 2014-11-28 23:08 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@samsung.com>:
> >> > On pi?, 2014-11-28 at 15:04 +0100, Linus Walleij wrote:
> >> >> On Wed, Nov 26, 2014 at 3:24 PM, Krzysztof Kozlowski
> >> >> <k.kozlowski@samsung.com> wrote:
> >> >>
> >> >> > The audio subsystem on Exynos 5420 has separate clocks and GPIO. To
> >> >> > operate properly on GPIOs the main block clock 'mau_epll' must be
> >> >> > enabled.
> >> >> >
> >> >> > This was observed on Peach Pi/Pit and Arndale Octa (after enabling i2s0)
> >> >> > after introducing runtime PM to pl330 DMA driver. After that commit the
> >> >> > 'mau_epll' was gated, because the "amba" clock was disabled and there
> >> >> > were no more users of mau_epll.
> >> >> >
> >> >> > The system hang just before probing i2s0 because
> >> >> > samsung_pinmux_setup() tried to access memory from audss block which was
> >> >> > gated.
> >> >> >
> >> >> > Add a clock property to the pinctrl driver and enable the clock during
> >> >> > GPIO setup. During normal GPIO operations (set, get, set_direction) the
> >> >> > clock is not enabled.
> >>
> >> Could you make sure that possibility of gating this clock is worth the
> >> effort of adding gating code to all affected drivers? If there is no
> >> significant change in power consumption maybe it could be simply keep
> >> running all the time?
> >
> > I had an impression that last time you disliked such idea:
> > http://www.spinics.net/lists/arm-kernel/msg338127.html
> > That's why I developed these patches. Because keeping a clock always on,
> > even when it is unused, is undesirable.
> 
> I was mostly concerned about the particular solution implemented by
> that patch that kept the clocks enabled on all platforms, not only the
> affected one. However...
> 
> >
> > Anyway, I did some simple measurements (after booting Arndale Octa
> > to /bin/sh, idle):
> >  - with mau_epll gated: ~523 mA
> >  - with mau_epll always on: ~531 mA
> >
> > Keeping it on increases energy usage by 1.5% in idle (with measurement
> > uncertainty ~0.4%).
> >
> 
> ...this definitely shows that the effort isn't worthless, which means
> that your patch goes in the right direction.

Great!

> 
> >
> >> Also isn't a similar problem happening due to power domains? I believe
> >> the whole maudio block is located in a separate power domain but
> >> somehow it doesn't get turned off?
> >
> > There is Maudio power domain... but I think it is not related here.
> 
> Could you somehow check what happens when the maudio power domain is
> turned off and maudio pin controller is accessed? Could you also check
> the difference in power consumption with this domain powered on and
> off?

I can try. I'll let you know the results.

> 
> > Pinctrl driver does not have runtime PM and is not attached to a domain.
> > I thought about other solution to this problem (with utilization of
> > power domains):
> >  - add runtime PM to pinctrl and audss clocks,
> >  - attach pinctrl and audss clocks to maudio power domain,
> >  - enable the clock when power domain is turned on.
> > However almost the same changes had to be added to pinctrl and audss
> > clocks drivers (replace clock_enable() with pm_runtime_get_sync()).
> 
> Well, if the dependency is there due to hardware design, I think it
> needs to be reflected in the drivers as well.
> 
> Few other issues that came to my mind:
> 
>  - Your previous patch added clock control only to pinctrl operations.
> Shouldn't GPIO operations be included as well?

Yeah... but does it make any sense? For example imagine GPIO is
configured as input. Enabling the clock won't change anything because
already that clock should be enabled by peripheral which writes to such
GPIO.

>  - If power domain dependency is there too, what happens with GPIO
> pins if the domain is powered off? If they lose their states, wouldn't
> it necessary to keep the domain powered on whenever there is some GPIO
> pin requested (which usually = in use)? (I'd assume that special
> functions shouldn't take a reference on runtime PM, because they are
> related to IP blocks in the same PM domain, which will implicitly keep
> the domain powered on.)

What you're saying makes sense but that look like job for sound soc
driver? Anyway the domain is not present in DTS so by default it is
turned on.

>  - Doesn't the clock controller also lose its state whenever the power
> domain is powered off? I guess that would be similar to ISP clock
> controller, issues of which are still not resolved in mainline.
> Sylwester could tell you more about this, I guess.

Once again - the domain.

Best regards,
Krzysztof

Re: [PATCH v2 1/5] clk: samsung: Fix double add of syscore ops after driver rebind

[Sylwester Nawrocki]

On 26/11/14 15:24, Krzysztof Kozlowski wrote:
> During driver unbind the syscore ops were not unregistered which lead to
> double add on syscore list:
> 
> $ echo "3810000.audss-clock-controller" > /sys/bus/platform/drivers/exynos-audss-clk/unbind
> $ echo "3810000.audss-clock-controller" > /sys/bus/platform/drivers/exynos-audss-clk/bind
> [ 1463.044061] ------------[ cut here ]------------
> [ 1463.047255] WARNING: CPU: 0 PID: 1 at lib/list_debug.c:36 __list_add+0x8c/0xc0()
> [ 1463.054613] list_add double add: new=c06e52c0, prev=c06e52c0, next=c06d5f84.
> [ 1463.061625] Modules linked in:
> [ 1463.064623] CPU: 0 PID: 1 Comm: bash Tainted: G        W      3.18.0-rc5-next-20141121-00005-ga8fab06eab42-dirty #1022
> [ 1463.075338] [<c0014e2c>] (unwind_backtrace) from [<c0011d80>] (show_stack+0x10/0x14)
> [ 1463.083046] [<c0011d80>] (show_stack) from [<c048bb70>] (dump_stack+0x70/0xbc)
> [ 1463.090236] [<c048bb70>] (dump_stack) from [<c00233d4>] (warn_slowpath_common+0x74/0xb0)
> [ 1463.098295] [<c00233d4>] (warn_slowpath_common) from [<c00234a4>] (warn_slowpath_fmt+0x30/0x40)
> [ 1463.106962] [<c00234a4>] (warn_slowpath_fmt) from [<c020fe80>] (__list_add+0x8c/0xc0)
> [ 1463.114760] [<c020fe80>] (__list_add) from [<c0282094>] (register_syscore_ops+0x30/0x3c)
> [ 1463.122819] [<c0282094>] (register_syscore_ops) from [<c0392f20>] (exynos_audss_clk_probe+0x36c/0x460)
> [ 1463.132091] [<c0392f20>] (exynos_audss_clk_probe) from [<c0283084>] (platform_drv_probe+0x48/0xa4)
> [ 1463.141013] [<c0283084>] (platform_drv_probe) from [<c0281a14>] (driver_probe_device+0x13c/0x37c)
> [ 1463.149852] [<c0281a14>] (driver_probe_device) from [<c0280560>] (bind_store+0x90/0xe0)
> [ 1463.157822] [<c0280560>] (bind_store) from [<c027fd10>] (drv_attr_store+0x20/0x2c)
> [ 1463.165363] [<c027fd10>] (drv_attr_store) from [<c0143898>] (sysfs_kf_write+0x4c/0x50)
> [ 1463.173252] [<c0143898>] (sysfs_kf_write) from [<c0142c80>] (kernfs_fop_write+0xbc/0x198)
> [ 1463.181395] [<c0142c80>] (kernfs_fop_write) from [<c00e2be0>] (vfs_write+0xa0/0x1a8)
> [ 1463.189104] [<c00e2be0>] (vfs_write) from [<c00e2f00>] (SyS_write+0x40/0x8c)
> [ 1463.196122] [<c00e2f00>] (SyS_write) from [<c000f2a0>] (ret_fast_syscall+0x0/0x48)
> [ 1463.203655] ---[ end trace 08f6710c9bc8d8f3 ]---
> [ 1463.208244] exynos-audss-clk 3810000.audss-clock-controller: setup completed
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Fixes: 1241ef94ccc3 ("clk: samsung: register audio subsystem clocks using common clock framework")
> Cc: <stable@vger.kernel.org>

Thanks for the fix, I applied the patch to my tree.

> ---
>  drivers/clk/samsung/clk-exynos-audss.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/clk/samsung/clk-exynos-audss.c b/drivers/clk/samsung/clk-exynos-audss.c
> index acce708ace18..7c4368e75ede 100644
> --- a/drivers/clk/samsung/clk-exynos-audss.c
> +++ b/drivers/clk/samsung/clk-exynos-audss.c
> @@ -210,6 +210,10 @@ static int exynos_audss_clk_remove(struct platform_device *pdev)
>  {
>  	int i;
>  
> +#ifdef CONFIG_PM_SLEEP
> +	unregister_syscore_ops(&exynos_audss_clk_syscore_ops);
> +#endif
> +

--
Regards,
Sylwester

[PATCH v2 1/5] clk: samsung: Fix double add of syscore ops after driver rebind

[Sylwester Nawrocki]

On 26/11/14 15:24, Krzysztof Kozlowski wrote:
> During driver unbind the syscore ops were not unregistered which lead to
> double add on syscore list:
> 
> $ echo "3810000.audss-clock-controller" > /sys/bus/platform/drivers/exynos-audss-clk/unbind
> $ echo "3810000.audss-clock-controller" > /sys/bus/platform/drivers/exynos-audss-clk/bind
> [ 1463.044061] ------------[ cut here ]------------
> [ 1463.047255] WARNING: CPU: 0 PID: 1 at lib/list_debug.c:36 __list_add+0x8c/0xc0()
> [ 1463.054613] list_add double add: new=c06e52c0, prev=c06e52c0, next=c06d5f84.
> [ 1463.061625] Modules linked in:
> [ 1463.064623] CPU: 0 PID: 1 Comm: bash Tainted: G        W      3.18.0-rc5-next-20141121-00005-ga8fab06eab42-dirty #1022
> [ 1463.075338] [<c0014e2c>] (unwind_backtrace) from [<c0011d80>] (show_stack+0x10/0x14)
> [ 1463.083046] [<c0011d80>] (show_stack) from [<c048bb70>] (dump_stack+0x70/0xbc)
> [ 1463.090236] [<c048bb70>] (dump_stack) from [<c00233d4>] (warn_slowpath_common+0x74/0xb0)
> [ 1463.098295] [<c00233d4>] (warn_slowpath_common) from [<c00234a4>] (warn_slowpath_fmt+0x30/0x40)
> [ 1463.106962] [<c00234a4>] (warn_slowpath_fmt) from [<c020fe80>] (__list_add+0x8c/0xc0)
> [ 1463.114760] [<c020fe80>] (__list_add) from [<c0282094>] (register_syscore_ops+0x30/0x3c)
> [ 1463.122819] [<c0282094>] (register_syscore_ops) from [<c0392f20>] (exynos_audss_clk_probe+0x36c/0x460)
> [ 1463.132091] [<c0392f20>] (exynos_audss_clk_probe) from [<c0283084>] (platform_drv_probe+0x48/0xa4)
> [ 1463.141013] [<c0283084>] (platform_drv_probe) from [<c0281a14>] (driver_probe_device+0x13c/0x37c)
> [ 1463.149852] [<c0281a14>] (driver_probe_device) from [<c0280560>] (bind_store+0x90/0xe0)
> [ 1463.157822] [<c0280560>] (bind_store) from [<c027fd10>] (drv_attr_store+0x20/0x2c)
> [ 1463.165363] [<c027fd10>] (drv_attr_store) from [<c0143898>] (sysfs_kf_write+0x4c/0x50)
> [ 1463.173252] [<c0143898>] (sysfs_kf_write) from [<c0142c80>] (kernfs_fop_write+0xbc/0x198)
> [ 1463.181395] [<c0142c80>] (kernfs_fop_write) from [<c00e2be0>] (vfs_write+0xa0/0x1a8)
> [ 1463.189104] [<c00e2be0>] (vfs_write) from [<c00e2f00>] (SyS_write+0x40/0x8c)
> [ 1463.196122] [<c00e2f00>] (SyS_write) from [<c000f2a0>] (ret_fast_syscall+0x0/0x48)
> [ 1463.203655] ---[ end trace 08f6710c9bc8d8f3 ]---
> [ 1463.208244] exynos-audss-clk 3810000.audss-clock-controller: setup completed
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Fixes: 1241ef94ccc3 ("clk: samsung: register audio subsystem clocks using common clock framework")
> Cc: <stable@vger.kernel.org>

Thanks for the fix, I applied the patch to my tree.

> ---
>  drivers/clk/samsung/clk-exynos-audss.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/clk/samsung/clk-exynos-audss.c b/drivers/clk/samsung/clk-exynos-audss.c
> index acce708ace18..7c4368e75ede 100644
> --- a/drivers/clk/samsung/clk-exynos-audss.c
> +++ b/drivers/clk/samsung/clk-exynos-audss.c
> @@ -210,6 +210,10 @@ static int exynos_audss_clk_remove(struct platform_device *pdev)
>  {
>  	int i;
>  
> +#ifdef CONFIG_PM_SLEEP
> +	unregister_syscore_ops(&exynos_audss_clk_syscore_ops);
> +#endif
> +

--
Regards,
Sylwester

Re: [PATCH v2 2/5] clk: samsung: Fix clock disable failure because domain being gated

[Sylwester Nawrocki]

On 26/11/14 15:24, Krzysztof Kozlowski wrote:
> Audio subsystem clocks are located in separate block. If clock for this
> block (from main clock domain) 'mau_epll' is gated then any read or
> write to audss registers will block.
> 
> This was observed on Exynos 5420 platforms (Arndale Octa and Peach
> Pi/Pit) after introducing runtime PM to pl330 DMA driver. After that
> commit the 'mau_epll' was gated, because the "amba" clock was disabled
> and there were no more users of mau_epll. The system hang on disabling
> unused clocks from audss block.
> 
> Unfortunately the 'mau_epll' clock is not parent of some of audss clocks.
> 
> Whenever system wants to operate on audss clocks it has to enable epll
> clock. The solution reuses common clk-gate/divider/mux code and duplicates
> clk_register_*() functions. In the same time the patch tries to limit
> functional changes of the driver so it does not fix minor issues with existing
> code (like leaking memory allocated for clk-gate/clk-mux/clk-divider code).
> This is addressed later.

It seems we need separate functions for unregistering the standard
mux/gate/div clocks.

> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Reported-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> Reported-by: Kevin Hilman <khilman@kernel.org>
> ---
>  drivers/clk/samsung/clk-exynos-audss.c | 346 +++++++++++++++++++++++++++++----
>  1 file changed, 311 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/clk/samsung/clk-exynos-audss.c b/drivers/clk/samsung/clk-exynos-audss.c
> index 7c4368e75ede..9ec7de866ab4 100644
> --- a/drivers/clk/samsung/clk-exynos-audss.c
> +++ b/drivers/clk/samsung/clk-exynos-audss.c
> @@ -29,6 +29,7 @@ static DEFINE_SPINLOCK(lock);
>  static struct clk **clk_table;
>  static void __iomem *reg_base;
>  static struct clk_onecell_data clk_data;
> +static struct clk *pll_in;
>  
>  #define ASS_CLK_SRC 0x0
>  #define ASS_CLK_DIV 0x4
> @@ -75,6 +76,276 @@ static const struct of_device_id exynos_audss_clk_of_match[] = {
>  	{},
>  };
>  
> +static int pll_clk_enable(void)
> +{
> +	if (!IS_ERR(pll_in))
> +		return clk_enable(pll_in);
> +
> +	return 0;
> +}
> +
> +static void pll_clk_disable(void)
> +{
> +	if (!IS_ERR(pll_in))
> +		clk_disable(pll_in);
> +}
> +
> +static int audss_clk_gate_enable(struct clk_hw *hw)
> +{
> +	int ret;
> +
> +	ret = pll_clk_enable();
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_gate_ops.enable(hw);
> +
> +	pll_clk_disable();
> +
> +	return ret;
> +}
> +
> +static void audss_clk_gate_disable(struct clk_hw *hw)
> +{
> +	int ret;
> +
> +	ret = pll_clk_enable();
> +	if (ret)
> +		return;
> +
> +	clk_gate_ops.disable(hw);
> +
> +	pll_clk_disable();
> +}
> +
> +static int audss_clk_gate_is_enabled(struct clk_hw *hw)
> +{
> +	int ret;
> +
> +	ret = pll_clk_enable();
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_gate_ops.is_enabled(hw);
> +
> +	pll_clk_disable();
> +
> +	return ret;
> +}
> +
> +static const struct clk_ops audss_clk_gate_ops = {
> +	.enable = audss_clk_gate_enable,
> +	.disable = audss_clk_gate_disable,
> +	.is_enabled = audss_clk_gate_is_enabled,
> +};

As Tomasz suggested a better approach could be to use regmap
and let it handle the PLL clock. Unfortunately there the regmap
is not supported for the base clock types in the clock core and
that would require even more work and more added code.

> +/*
> + * A simplified copy of clk-gate.c:clk_register_gate() to mimic
> + * clk-gate behavior while using customized ops.
> + *
> + * TODO: just like clk-gate it leaks memory for struct clk_gate.

Please squash patch 5/5 into this one for the next iteration.

> + */
> +static struct clk *audss_clk_register_gate(struct device *dev, const char *name,
> +		const char *parent_name, unsigned long flags, u8 bit_idx)
> +{
> +	struct clk_gate *gate;
> +	struct clk *clk;
> +	struct clk_init_data init;
> +
> +	/* allocate the gate */
> +	gate = kzalloc(sizeof(struct clk_gate), GFP_KERNEL);
> +	if (!gate)
> +		return ERR_PTR(-ENOMEM);
> +
> +	init.name = name;
> +	init.ops = &audss_clk_gate_ops;
> +	init.flags = flags | CLK_IS_BASIC;
> +	init.parent_names = (parent_name ? &parent_name : NULL);
> +	init.num_parents = (parent_name ? 1 : 0);
> +
> +	/* struct clk_gate assignments */
> +	gate->reg = reg_base + ASS_CLK_GATE;
> +	gate->bit_idx = bit_idx;
> +	gate->flags = 0;
> +	gate->lock = &lock;
> +	gate->hw.init = &init;
> +
> +	clk = clk_register(dev, &gate->hw);
> +
> +	if (IS_ERR(clk))
> +		kfree(gate);
> +
> +	return clk;
> +}
> +

>  /* register exynos_audss clocks */
>  static int exynos_audss_clk_probe(struct platform_device *pdev)
>  {
> @@ -83,7 +354,7 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
>  	const char *mout_audss_p[] = {"fin_pll", "fout_epll"};
>  	const char *mout_i2s_p[] = {"mout_audss", "cdclk0", "sclk_audio0"};
>  	const char *sclk_pcm_p = "sclk_pcm0";
> -	struct clk *pll_ref, *pll_in, *cdclk, *sclk_audio, *sclk_pcm_in;

How about pll_in locally, using a different name for the global pointer
and ensuring the global pointer is properly initialized to ERR_PTR value
for cases where we don't need to touch the APLL clock ?

> +	struct clk *pll_ref, *cdclk, *sclk_audio, *sclk_pcm_in;
>  	const struct of_device_id *match;
>  	enum exynos_audss_clk_type variant;
>  
> @@ -115,12 +386,21 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
>  	pll_in = devm_clk_get(&pdev->dev, "pll_in");
>  	if (!IS_ERR(pll_ref))
>  		mout_audss_p[0] = __clk_get_name(pll_ref);
> -	if (!IS_ERR(pll_in))
> +	if (!IS_ERR(pll_in)) {
>  		mout_audss_p[1] = __clk_get_name(pll_in);
> -	clk_table[EXYNOS_MOUT_AUDSS] = clk_register_mux(NULL, "mout_audss",
> +
> +		ret = clk_prepare(pll_in);
> +		if (ret) {
> +			dev_err(&pdev->dev,
> +				"failed to prepare the pll_in clock\n");
> +			return ret;
> +		}

Let's introduce such chnages only for SoC's where that's really necessary,
AFAICS it seems to be needed only for "samsung,exynos5420-audss-clock"
compatible.

> +	}
> +
> +	clk_table[EXYNOS_MOUT_AUDSS] = audss_clk_register_mux(NULL, "mout_audss",
>  				mout_audss_p, ARRAY_SIZE(mout_audss_p),
> -				CLK_SET_RATE_NO_REPARENT,
> -				reg_base + ASS_CLK_SRC, 0, 1, 0, &lock);
> +				CLK_SET_RATE_NO_REPARENT, 0, 1);

I would prefer leaving the register's address in the arguments list.
Now you're passing the bit index but not the actual register.

>  	cdclk = devm_clk_get(&pdev->dev, "cdclk");
>  	sclk_audio = devm_clk_get(&pdev->dev, "sclk_audio");

--
Regards,
Sylwester

[PATCH v2 2/5] clk: samsung: Fix clock disable failure because domain being gated

[Sylwester Nawrocki]

On 26/11/14 15:24, Krzysztof Kozlowski wrote:
> Audio subsystem clocks are located in separate block. If clock for this
> block (from main clock domain) 'mau_epll' is gated then any read or
> write to audss registers will block.
> 
> This was observed on Exynos 5420 platforms (Arndale Octa and Peach
> Pi/Pit) after introducing runtime PM to pl330 DMA driver. After that
> commit the 'mau_epll' was gated, because the "amba" clock was disabled
> and there were no more users of mau_epll. The system hang on disabling
> unused clocks from audss block.
> 
> Unfortunately the 'mau_epll' clock is not parent of some of audss clocks.
> 
> Whenever system wants to operate on audss clocks it has to enable epll
> clock. The solution reuses common clk-gate/divider/mux code and duplicates
> clk_register_*() functions. In the same time the patch tries to limit
> functional changes of the driver so it does not fix minor issues with existing
> code (like leaking memory allocated for clk-gate/clk-mux/clk-divider code).
> This is addressed later.

It seems we need separate functions for unregistering the standard
mux/gate/div clocks.

> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Reported-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> Reported-by: Kevin Hilman <khilman@kernel.org>
> ---
>  drivers/clk/samsung/clk-exynos-audss.c | 346 +++++++++++++++++++++++++++++----
>  1 file changed, 311 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/clk/samsung/clk-exynos-audss.c b/drivers/clk/samsung/clk-exynos-audss.c
> index 7c4368e75ede..9ec7de866ab4 100644
> --- a/drivers/clk/samsung/clk-exynos-audss.c
> +++ b/drivers/clk/samsung/clk-exynos-audss.c
> @@ -29,6 +29,7 @@ static DEFINE_SPINLOCK(lock);
>  static struct clk **clk_table;
>  static void __iomem *reg_base;
>  static struct clk_onecell_data clk_data;
> +static struct clk *pll_in;
>  
>  #define ASS_CLK_SRC 0x0
>  #define ASS_CLK_DIV 0x4
> @@ -75,6 +76,276 @@ static const struct of_device_id exynos_audss_clk_of_match[] = {
>  	{},
>  };
>  
> +static int pll_clk_enable(void)
> +{
> +	if (!IS_ERR(pll_in))
> +		return clk_enable(pll_in);
> +
> +	return 0;
> +}
> +
> +static void pll_clk_disable(void)
> +{
> +	if (!IS_ERR(pll_in))
> +		clk_disable(pll_in);
> +}
> +
> +static int audss_clk_gate_enable(struct clk_hw *hw)
> +{
> +	int ret;
> +
> +	ret = pll_clk_enable();
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_gate_ops.enable(hw);
> +
> +	pll_clk_disable();
> +
> +	return ret;
> +}
> +
> +static void audss_clk_gate_disable(struct clk_hw *hw)
> +{
> +	int ret;
> +
> +	ret = pll_clk_enable();
> +	if (ret)
> +		return;
> +
> +	clk_gate_ops.disable(hw);
> +
> +	pll_clk_disable();
> +}
> +
> +static int audss_clk_gate_is_enabled(struct clk_hw *hw)
> +{
> +	int ret;
> +
> +	ret = pll_clk_enable();
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_gate_ops.is_enabled(hw);
> +
> +	pll_clk_disable();
> +
> +	return ret;
> +}
> +
> +static const struct clk_ops audss_clk_gate_ops = {
> +	.enable = audss_clk_gate_enable,
> +	.disable = audss_clk_gate_disable,
> +	.is_enabled = audss_clk_gate_is_enabled,
> +};

As Tomasz suggested a better approach could be to use regmap
and let it handle the PLL clock. Unfortunately there the regmap
is not supported for the base clock types in the clock core and
that would require even more work and more added code.

> +/*
> + * A simplified copy of clk-gate.c:clk_register_gate() to mimic
> + * clk-gate behavior while using customized ops.
> + *
> + * TODO: just like clk-gate it leaks memory for struct clk_gate.

Please squash patch 5/5 into this one for the next iteration.

> + */
> +static struct clk *audss_clk_register_gate(struct device *dev, const char *name,
> +		const char *parent_name, unsigned long flags, u8 bit_idx)
> +{
> +	struct clk_gate *gate;
> +	struct clk *clk;
> +	struct clk_init_data init;
> +
> +	/* allocate the gate */
> +	gate = kzalloc(sizeof(struct clk_gate), GFP_KERNEL);
> +	if (!gate)
> +		return ERR_PTR(-ENOMEM);
> +
> +	init.name = name;
> +	init.ops = &audss_clk_gate_ops;
> +	init.flags = flags | CLK_IS_BASIC;
> +	init.parent_names = (parent_name ? &parent_name : NULL);
> +	init.num_parents = (parent_name ? 1 : 0);
> +
> +	/* struct clk_gate assignments */
> +	gate->reg = reg_base + ASS_CLK_GATE;
> +	gate->bit_idx = bit_idx;
> +	gate->flags = 0;
> +	gate->lock = &lock;
> +	gate->hw.init = &init;
> +
> +	clk = clk_register(dev, &gate->hw);
> +
> +	if (IS_ERR(clk))
> +		kfree(gate);
> +
> +	return clk;
> +}
> +

>  /* register exynos_audss clocks */
>  static int exynos_audss_clk_probe(struct platform_device *pdev)
>  {
> @@ -83,7 +354,7 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
>  	const char *mout_audss_p[] = {"fin_pll", "fout_epll"};
>  	const char *mout_i2s_p[] = {"mout_audss", "cdclk0", "sclk_audio0"};
>  	const char *sclk_pcm_p = "sclk_pcm0";
> -	struct clk *pll_ref, *pll_in, *cdclk, *sclk_audio, *sclk_pcm_in;

How about pll_in locally, using a different name for the global pointer
and ensuring the global pointer is properly initialized to ERR_PTR value
for cases where we don't need to touch the APLL clock ?

> +	struct clk *pll_ref, *cdclk, *sclk_audio, *sclk_pcm_in;
>  	const struct of_device_id *match;
>  	enum exynos_audss_clk_type variant;
>  
> @@ -115,12 +386,21 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
>  	pll_in = devm_clk_get(&pdev->dev, "pll_in");
>  	if (!IS_ERR(pll_ref))
>  		mout_audss_p[0] = __clk_get_name(pll_ref);
> -	if (!IS_ERR(pll_in))
> +	if (!IS_ERR(pll_in)) {
>  		mout_audss_p[1] = __clk_get_name(pll_in);
> -	clk_table[EXYNOS_MOUT_AUDSS] = clk_register_mux(NULL, "mout_audss",
> +
> +		ret = clk_prepare(pll_in);
> +		if (ret) {
> +			dev_err(&pdev->dev,
> +				"failed to prepare the pll_in clock\n");
> +			return ret;
> +		}

Let's introduce such chnages only for SoC's where that's really necessary,
AFAICS it seems to be needed only for "samsung,exynos5420-audss-clock"
compatible.

> +	}
> +
> +	clk_table[EXYNOS_MOUT_AUDSS] = audss_clk_register_mux(NULL, "mout_audss",
>  				mout_audss_p, ARRAY_SIZE(mout_audss_p),
> -				CLK_SET_RATE_NO_REPARENT,
> -				reg_base + ASS_CLK_SRC, 0, 1, 0, &lock);
> +				CLK_SET_RATE_NO_REPARENT, 0, 1);

I would prefer leaving the register's address in the arguments list.
Now you're passing the bit index but not the actual register.

>  	cdclk = devm_clk_get(&pdev->dev, "cdclk");
>  	sclk_audio = devm_clk_get(&pdev->dev, "sclk_audio");

--
Regards,
Sylwester

Re: [PATCH v2 2/5] clk: samsung: Fix clock disable failure because domain being gated

[Krzysztof Kozlowski]

On śro, 2014-12-03 at 15:12 +0100, Sylwester Nawrocki wrote:
> On 26/11/14 15:24, Krzysztof Kozlowski wrote:
> > Audio subsystem clocks are located in separate block. If clock for this
> > block (from main clock domain) 'mau_epll' is gated then any read or
> > write to audss registers will block.
> > 
> > This was observed on Exynos 5420 platforms (Arndale Octa and Peach
> > Pi/Pit) after introducing runtime PM to pl330 DMA driver. After that
> > commit the 'mau_epll' was gated, because the "amba" clock was disabled
> > and there were no more users of mau_epll. The system hang on disabling
> > unused clocks from audss block.
> > 
> > Unfortunately the 'mau_epll' clock is not parent of some of audss clocks.
> > 
> > Whenever system wants to operate on audss clocks it has to enable epll
> > clock. The solution reuses common clk-gate/divider/mux code and duplicates
> > clk_register_*() functions. In the same time the patch tries to limit
> > functional changes of the driver so it does not fix minor issues with existing
> > code (like leaking memory allocated for clk-gate/clk-mux/clk-divider code).
> > This is addressed later.
> 
> It seems we need separate functions for unregistering the standard
> mux/gate/div clocks.

Yep, I put it on our TODO list...

> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > Reported-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> > Reported-by: Kevin Hilman <khilman@kernel.org>
> > ---
> >  drivers/clk/samsung/clk-exynos-audss.c | 346 +++++++++++++++++++++++++++++----
> >  1 file changed, 311 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/clk/samsung/clk-exynos-audss.c b/drivers/clk/samsung/clk-exynos-audss.c
> > index 7c4368e75ede..9ec7de866ab4 100644
> > --- a/drivers/clk/samsung/clk-exynos-audss.c
> > +++ b/drivers/clk/samsung/clk-exynos-audss.c
> > @@ -29,6 +29,7 @@ static DEFINE_SPINLOCK(lock);
> >  static struct clk **clk_table;
> >  static void __iomem *reg_base;
> >  static struct clk_onecell_data clk_data;
> > +static struct clk *pll_in;
> >  
> >  #define ASS_CLK_SRC 0x0
> >  #define ASS_CLK_DIV 0x4
> > @@ -75,6 +76,276 @@ static const struct of_device_id exynos_audss_clk_of_match[] = {
> >  	{},
> >  };
> >  
> > +static int pll_clk_enable(void)
> > +{
> > +	if (!IS_ERR(pll_in))
> > +		return clk_enable(pll_in);
> > +
> > +	return 0;
> > +}
> > +
> > +static void pll_clk_disable(void)
> > +{
> > +	if (!IS_ERR(pll_in))
> > +		clk_disable(pll_in);
> > +}
> > +
> > +static int audss_clk_gate_enable(struct clk_hw *hw)
> > +{
> > +	int ret;
> > +
> > +	ret = pll_clk_enable();
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = clk_gate_ops.enable(hw);
> > +
> > +	pll_clk_disable();
> > +
> > +	return ret;
> > +}
> > +
> > +static void audss_clk_gate_disable(struct clk_hw *hw)
> > +{
> > +	int ret;
> > +
> > +	ret = pll_clk_enable();
> > +	if (ret)
> > +		return;
> > +
> > +	clk_gate_ops.disable(hw);
> > +
> > +	pll_clk_disable();
> > +}
> > +
> > +static int audss_clk_gate_is_enabled(struct clk_hw *hw)
> > +{
> > +	int ret;
> > +
> > +	ret = pll_clk_enable();
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = clk_gate_ops.is_enabled(hw);
> > +
> > +	pll_clk_disable();
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct clk_ops audss_clk_gate_ops = {
> > +	.enable = audss_clk_gate_enable,
> > +	.disable = audss_clk_gate_disable,
> > +	.is_enabled = audss_clk_gate_is_enabled,
> > +};
> 
> As Tomasz suggested a better approach could be to use regmap
> and let it handle the PLL clock. Unfortunately there the regmap
> is not supported for the base clock types in the clock core and
> that would require even more work and more added code.
> 
> > +/*
> > + * A simplified copy of clk-gate.c:clk_register_gate() to mimic
> > + * clk-gate behavior while using customized ops.
> > + *
> > + * TODO: just like clk-gate it leaks memory for struct clk_gate.
> 
> Please squash patch 5/5 into this one for the next iteration.

Sure.

> 
> > + */
> > +static struct clk *audss_clk_register_gate(struct device *dev, const char *name,
> > +		const char *parent_name, unsigned long flags, u8 bit_idx)
> > +{
> > +	struct clk_gate *gate;
> > +	struct clk *clk;
> > +	struct clk_init_data init;
> > +
> > +	/* allocate the gate */
> > +	gate = kzalloc(sizeof(struct clk_gate), GFP_KERNEL);
> > +	if (!gate)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	init.name = name;
> > +	init.ops = &audss_clk_gate_ops;
> > +	init.flags = flags | CLK_IS_BASIC;
> > +	init.parent_names = (parent_name ? &parent_name : NULL);
> > +	init.num_parents = (parent_name ? 1 : 0);
> > +
> > +	/* struct clk_gate assignments */
> > +	gate->reg = reg_base + ASS_CLK_GATE;
> > +	gate->bit_idx = bit_idx;
> > +	gate->flags = 0;
> > +	gate->lock = &lock;
> > +	gate->hw.init = &init;
> > +
> > +	clk = clk_register(dev, &gate->hw);
> > +
> > +	if (IS_ERR(clk))
> > +		kfree(gate);
> > +
> > +	return clk;
> > +}
> > +
> 
> >  /* register exynos_audss clocks */
> >  static int exynos_audss_clk_probe(struct platform_device *pdev)
> >  {
> > @@ -83,7 +354,7 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
> >  	const char *mout_audss_p[] = {"fin_pll", "fout_epll"};
> >  	const char *mout_i2s_p[] = {"mout_audss", "cdclk0", "sclk_audio0"};
> >  	const char *sclk_pcm_p = "sclk_pcm0";
> > -	struct clk *pll_ref, *pll_in, *cdclk, *sclk_audio, *sclk_pcm_in;
> 
> How about pll_in locally, using a different name for the global pointer
> and ensuring the global pointer is properly initialized to ERR_PTR value
> for cases where we don't need to touch the APLL clock ?

OK.

> 
> > +	struct clk *pll_ref, *cdclk, *sclk_audio, *sclk_pcm_in;
> >  	const struct of_device_id *match;
> >  	enum exynos_audss_clk_type variant;
> >  
> > @@ -115,12 +386,21 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
> >  	pll_in = devm_clk_get(&pdev->dev, "pll_in");
> >  	if (!IS_ERR(pll_ref))
> >  		mout_audss_p[0] = __clk_get_name(pll_ref);
> > -	if (!IS_ERR(pll_in))
> > +	if (!IS_ERR(pll_in)) {
> >  		mout_audss_p[1] = __clk_get_name(pll_in);
> > -	clk_table[EXYNOS_MOUT_AUDSS] = clk_register_mux(NULL, "mout_audss",
> > +
> > +		ret = clk_prepare(pll_in);
> > +		if (ret) {
> > +			dev_err(&pdev->dev,
> > +				"failed to prepare the pll_in clock\n");
> > +			return ret;
> > +		}
> 
> Let's introduce such chnages only for SoC's where that's really necessary,
> AFAICS it seems to be needed only for "samsung,exynos5420-audss-clock"
> compatible.

Yes, it turned out that only Exynos 5420 has mau_epll clock. On
Exynos4412 for example such problem does not exist.

> > +	}
> > +
> > +	clk_table[EXYNOS_MOUT_AUDSS] = audss_clk_register_mux(NULL, "mout_audss",
> >  				mout_audss_p, ARRAY_SIZE(mout_audss_p),
> > -				CLK_SET_RATE_NO_REPARENT,
> > -				reg_base + ASS_CLK_SRC, 0, 1, 0, &lock);
> > +				CLK_SET_RATE_NO_REPARENT, 0, 1);
> 
> I would prefer leaving the register's address in the arguments list.
> Now you're passing the bit index but not the actual register.

Hmm... that would extend the arguments list without any information (the
register is the same)... but sure, I'll add it.

> 
> >  	cdclk = devm_clk_get(&pdev->dev, "cdclk");
> >  	sclk_audio = devm_clk_get(&pdev->dev, "sclk_audio");
> 

Thanks for reviewing!
Krzysztof

[PATCH v2 2/5] clk: samsung: Fix clock disable failure because domain being gated

[Krzysztof Kozlowski]

On ?ro, 2014-12-03 at 15:12 +0100, Sylwester Nawrocki wrote:
> On 26/11/14 15:24, Krzysztof Kozlowski wrote:
> > Audio subsystem clocks are located in separate block. If clock for this
> > block (from main clock domain) 'mau_epll' is gated then any read or
> > write to audss registers will block.
> > 
> > This was observed on Exynos 5420 platforms (Arndale Octa and Peach
> > Pi/Pit) after introducing runtime PM to pl330 DMA driver. After that
> > commit the 'mau_epll' was gated, because the "amba" clock was disabled
> > and there were no more users of mau_epll. The system hang on disabling
> > unused clocks from audss block.
> > 
> > Unfortunately the 'mau_epll' clock is not parent of some of audss clocks.
> > 
> > Whenever system wants to operate on audss clocks it has to enable epll
> > clock. The solution reuses common clk-gate/divider/mux code and duplicates
> > clk_register_*() functions. In the same time the patch tries to limit
> > functional changes of the driver so it does not fix minor issues with existing
> > code (like leaking memory allocated for clk-gate/clk-mux/clk-divider code).
> > This is addressed later.
> 
> It seems we need separate functions for unregistering the standard
> mux/gate/div clocks.

Yep, I put it on our TODO list...

> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > Reported-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> > Reported-by: Kevin Hilman <khilman@kernel.org>
> > ---
> >  drivers/clk/samsung/clk-exynos-audss.c | 346 +++++++++++++++++++++++++++++----
> >  1 file changed, 311 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/clk/samsung/clk-exynos-audss.c b/drivers/clk/samsung/clk-exynos-audss.c
> > index 7c4368e75ede..9ec7de866ab4 100644
> > --- a/drivers/clk/samsung/clk-exynos-audss.c
> > +++ b/drivers/clk/samsung/clk-exynos-audss.c
> > @@ -29,6 +29,7 @@ static DEFINE_SPINLOCK(lock);
> >  static struct clk **clk_table;
> >  static void __iomem *reg_base;
> >  static struct clk_onecell_data clk_data;
> > +static struct clk *pll_in;
> >  
> >  #define ASS_CLK_SRC 0x0
> >  #define ASS_CLK_DIV 0x4
> > @@ -75,6 +76,276 @@ static const struct of_device_id exynos_audss_clk_of_match[] = {
> >  	{},
> >  };
> >  
> > +static int pll_clk_enable(void)
> > +{
> > +	if (!IS_ERR(pll_in))
> > +		return clk_enable(pll_in);
> > +
> > +	return 0;
> > +}
> > +
> > +static void pll_clk_disable(void)
> > +{
> > +	if (!IS_ERR(pll_in))
> > +		clk_disable(pll_in);
> > +}
> > +
> > +static int audss_clk_gate_enable(struct clk_hw *hw)
> > +{
> > +	int ret;
> > +
> > +	ret = pll_clk_enable();
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = clk_gate_ops.enable(hw);
> > +
> > +	pll_clk_disable();
> > +
> > +	return ret;
> > +}
> > +
> > +static void audss_clk_gate_disable(struct clk_hw *hw)
> > +{
> > +	int ret;
> > +
> > +	ret = pll_clk_enable();
> > +	if (ret)
> > +		return;
> > +
> > +	clk_gate_ops.disable(hw);
> > +
> > +	pll_clk_disable();
> > +}
> > +
> > +static int audss_clk_gate_is_enabled(struct clk_hw *hw)
> > +{
> > +	int ret;
> > +
> > +	ret = pll_clk_enable();
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = clk_gate_ops.is_enabled(hw);
> > +
> > +	pll_clk_disable();
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct clk_ops audss_clk_gate_ops = {
> > +	.enable = audss_clk_gate_enable,
> > +	.disable = audss_clk_gate_disable,
> > +	.is_enabled = audss_clk_gate_is_enabled,
> > +};
> 
> As Tomasz suggested a better approach could be to use regmap
> and let it handle the PLL clock. Unfortunately there the regmap
> is not supported for the base clock types in the clock core and
> that would require even more work and more added code.
> 
> > +/*
> > + * A simplified copy of clk-gate.c:clk_register_gate() to mimic
> > + * clk-gate behavior while using customized ops.
> > + *
> > + * TODO: just like clk-gate it leaks memory for struct clk_gate.
> 
> Please squash patch 5/5 into this one for the next iteration.

Sure.

> 
> > + */
> > +static struct clk *audss_clk_register_gate(struct device *dev, const char *name,
> > +		const char *parent_name, unsigned long flags, u8 bit_idx)
> > +{
> > +	struct clk_gate *gate;
> > +	struct clk *clk;
> > +	struct clk_init_data init;
> > +
> > +	/* allocate the gate */
> > +	gate = kzalloc(sizeof(struct clk_gate), GFP_KERNEL);
> > +	if (!gate)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	init.name = name;
> > +	init.ops = &audss_clk_gate_ops;
> > +	init.flags = flags | CLK_IS_BASIC;
> > +	init.parent_names = (parent_name ? &parent_name : NULL);
> > +	init.num_parents = (parent_name ? 1 : 0);
> > +
> > +	/* struct clk_gate assignments */
> > +	gate->reg = reg_base + ASS_CLK_GATE;
> > +	gate->bit_idx = bit_idx;
> > +	gate->flags = 0;
> > +	gate->lock = &lock;
> > +	gate->hw.init = &init;
> > +
> > +	clk = clk_register(dev, &gate->hw);
> > +
> > +	if (IS_ERR(clk))
> > +		kfree(gate);
> > +
> > +	return clk;
> > +}
> > +
> 
> >  /* register exynos_audss clocks */
> >  static int exynos_audss_clk_probe(struct platform_device *pdev)
> >  {
> > @@ -83,7 +354,7 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
> >  	const char *mout_audss_p[] = {"fin_pll", "fout_epll"};
> >  	const char *mout_i2s_p[] = {"mout_audss", "cdclk0", "sclk_audio0"};
> >  	const char *sclk_pcm_p = "sclk_pcm0";
> > -	struct clk *pll_ref, *pll_in, *cdclk, *sclk_audio, *sclk_pcm_in;
> 
> How about pll_in locally, using a different name for the global pointer
> and ensuring the global pointer is properly initialized to ERR_PTR value
> for cases where we don't need to touch the APLL clock ?

OK.

> 
> > +	struct clk *pll_ref, *cdclk, *sclk_audio, *sclk_pcm_in;
> >  	const struct of_device_id *match;
> >  	enum exynos_audss_clk_type variant;
> >  
> > @@ -115,12 +386,21 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
> >  	pll_in = devm_clk_get(&pdev->dev, "pll_in");
> >  	if (!IS_ERR(pll_ref))
> >  		mout_audss_p[0] = __clk_get_name(pll_ref);
> > -	if (!IS_ERR(pll_in))
> > +	if (!IS_ERR(pll_in)) {
> >  		mout_audss_p[1] = __clk_get_name(pll_in);
> > -	clk_table[EXYNOS_MOUT_AUDSS] = clk_register_mux(NULL, "mout_audss",
> > +
> > +		ret = clk_prepare(pll_in);
> > +		if (ret) {
> > +			dev_err(&pdev->dev,
> > +				"failed to prepare the pll_in clock\n");
> > +			return ret;
> > +		}
> 
> Let's introduce such chnages only for SoC's where that's really necessary,
> AFAICS it seems to be needed only for "samsung,exynos5420-audss-clock"
> compatible.

Yes, it turned out that only Exynos 5420 has mau_epll clock. On
Exynos4412 for example such problem does not exist.

> > +	}
> > +
> > +	clk_table[EXYNOS_MOUT_AUDSS] = audss_clk_register_mux(NULL, "mout_audss",
> >  				mout_audss_p, ARRAY_SIZE(mout_audss_p),
> > -				CLK_SET_RATE_NO_REPARENT,
> > -				reg_base + ASS_CLK_SRC, 0, 1, 0, &lock);
> > +				CLK_SET_RATE_NO_REPARENT, 0, 1);
> 
> I would prefer leaving the register's address in the arguments list.
> Now you're passing the bit index but not the actual register.

Hmm... that would extend the arguments list without any information (the
register is the same)... but sure, I'll add it.

> 
> >  	cdclk = devm_clk_get(&pdev->dev, "cdclk");
> >  	sclk_audio = devm_clk_get(&pdev->dev, "sclk_audio");
> 

Thanks for reviewing!
Krzysztof

Re: [PATCH v2 3/5] pinctrl: exynos: Fix GPIO setup failure because domain clock being gated

[Krzysztof Kozlowski]

On pon, 2014-12-01 at 23:34 +0900, Tomasz Figa wrote:
> 2014-12-01 17:37 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@samsung.com>:
> >
> > On nie, 2014-11-30 at 21:19 +0900, Tomasz Figa wrote:
> >> Hi Krzysztof,
> >>
(...)
> >
> >> Also isn't a similar problem happening due to power domains? I believe
> >> the whole maudio block is located in a separate power domain but
> >> somehow it doesn't get turned off?
> >
> > There is Maudio power domain... but I think it is not related here.
> 
> Could you somehow check what happens when the maudio power domain is
> turned off and maudio pin controller is accessed? Could you also check
> the difference in power consumption with this domain powered on and
> off?

Hi Tomasz,

Some time ago you asked for some details of Exynos5420 Maudio domain
behavior. Here it goes:

1. mau domain ON, EPL: OFF: boot hang (this was behavior after adding
runtime PM to pl330 driver)

2. maud domain OFF, EPLL OFF: boot hang

3. maud domain ON, EPLL ON: works (current linux-next and mainline)

4. maud domain OFF, EPLL ON: works, almost the same energy consumption
as in (3) (diff: 0.1% so it is smaller than measuring accuracy).

However an unspecified imprecise abort shows up in (4):
[   11.911628] Freeing unused kernel memory: 328K (c0674000 - c06c6000)
[   12.122695] usb 5-1.4: new high-speed USB device number 3 using exynos-ehci
[   12.190011] Unhandled fault: imprecise external abort (0x1406) at 0x00000000
[   12.219062] usb 5-1.4: New USB device found, idVendor=0b95, idProduct=772a
[   12.224548] usb 5-1.4: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[   12.231794] usb 5-1.4: Product: AX88772
[   12.235681] usb 5-1.4: Manufacturer: ASIX Elec. Corp.
[   12.240707] usb 5-1.4: SerialNumber: 000001
[   12.248835] asix 5-1.4:1.0 (unnamed net_device) (uninitialized): invalid hw address, using random
root@(none):/#

Best regards,
Krzysztof

> 
> > Pinctrl driver does not have runtime PM and is not attached to a domain.
> > I thought about other solution to this problem (with utilization of
> > power domains):
> >  - add runtime PM to pinctrl and audss clocks,
> >  - attach pinctrl and audss clocks to maudio power domain,
> >  - enable the clock when power domain is turned on.
> > However almost the same changes had to be added to pinctrl and audss
> > clocks drivers (replace clock_enable() with pm_runtime_get_sync()).
> 
> Well, if the dependency is there due to hardware design, I think it
> needs to be reflected in the drivers as well.
> 
> Few other issues that came to my mind:
> 
>  - Your previous patch added clock control only to pinctrl operations.
> Shouldn't GPIO operations be included as well?
> 
>  - If power domain dependency is there too, what happens with GPIO
> pins if the domain is powered off? If they lose their states, wouldn't
> it necessary to keep the domain powered on whenever there is some GPIO
> pin requested (which usually = in use)? (I'd assume that special
> functions shouldn't take a reference on runtime PM, because they are
> related to IP blocks in the same PM domain, which will implicitly keep
> the domain powered on.)
> 
>  - Doesn't the clock controller also lose its state whenever the power
> domain is powered off? I guess that would be similar to ISP clock
> controller, issues of which are still not resolved in mainline.
> Sylwester could tell you more about this, I guess.
> 
> Best regards,
> Tomasz

Re: [PATCH v2 3/5] pinctrl: exynos: Fix GPIO setup failure because domain clock being gated

[Krzysztof Kozlowski]

On pon, 2014-12-01 at 23:34 +0900, Tomasz Figa wrote:
> 2014-12-01 17:37 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@samsung.com>:
> >
> > On nie, 2014-11-30 at 21:19 +0900, Tomasz Figa wrote:
> >> Hi Krzysztof,
> >>
(...)
> >
> >> Also isn't a similar problem happening due to power domains? I believe
> >> the whole maudio block is located in a separate power domain but
> >> somehow it doesn't get turned off?
> >
> > There is Maudio power domain... but I think it is not related here.
> 
> Could you somehow check what happens when the maudio power domain is
> turned off and maudio pin controller is accessed? Could you also check
> the difference in power consumption with this domain powered on and
> off?

Hi Tomasz,

Some time ago you asked for some details of Exynos5420 Maudio domain
behavior. Here it goes:

1. mau domain ON, EPL: OFF: boot hang (this was behavior after adding
runtime PM to pl330 driver)

2. maud domain OFF, EPLL OFF: boot hang

3. maud domain ON, EPLL ON: works (current linux-next and mainline)

4. maud domain OFF, EPLL ON: works, almost the same energy consumption
as in (3) (diff: 0.1% so it is smaller than measuring accuracy).

However an unspecified imprecise abort shows up in (4):
[   11.911628] Freeing unused kernel memory: 328K (c0674000 - c06c6000)
[   12.122695] usb 5-1.4: new high-speed USB device number 3 using exynos-ehci
[   12.190011] Unhandled fault: imprecise external abort (0x1406) at 0x00000000
[   12.219062] usb 5-1.4: New USB device found, idVendor=0b95, idProduct=772a
[   12.224548] usb 5-1.4: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[   12.231794] usb 5-1.4: Product: AX88772
[   12.235681] usb 5-1.4: Manufacturer: ASIX Elec. Corp.
[   12.240707] usb 5-1.4: SerialNumber: 000001
[   12.248835] asix 5-1.4:1.0 (unnamed net_device) (uninitialized): invalid hw address, using random
root@(none):/#

Best regards,
Krzysztof

> 
> > Pinctrl driver does not have runtime PM and is not attached to a domain.
> > I thought about other solution to this problem (with utilization of
> > power domains):
> >  - add runtime PM to pinctrl and audss clocks,
> >  - attach pinctrl and audss clocks to maudio power domain,
> >  - enable the clock when power domain is turned on.
> > However almost the same changes had to be added to pinctrl and audss
> > clocks drivers (replace clock_enable() with pm_runtime_get_sync()).
> 
> Well, if the dependency is there due to hardware design, I think it
> needs to be reflected in the drivers as well.
> 
> Few other issues that came to my mind:
> 
>  - Your previous patch added clock control only to pinctrl operations.
> Shouldn't GPIO operations be included as well?
> 
>  - If power domain dependency is there too, what happens with GPIO
> pins if the domain is powered off? If they lose their states, wouldn't
> it necessary to keep the domain powered on whenever there is some GPIO
> pin requested (which usually = in use)? (I'd assume that special
> functions shouldn't take a reference on runtime PM, because they are
> related to IP blocks in the same PM domain, which will implicitly keep
> the domain powered on.)
> 
>  - Doesn't the clock controller also lose its state whenever the power
> domain is powered off? I guess that would be similar to ISP clock
> controller, issues of which are still not resolved in mainline.
> Sylwester could tell you more about this, I guess.
> 
> Best regards,
> Tomasz

[PATCH v2 3/5] pinctrl: exynos: Fix GPIO setup failure because domain clock being gated

[Krzysztof Kozlowski]

On pon, 2014-12-01 at 23:34 +0900, Tomasz Figa wrote:
> 2014-12-01 17:37 GMT+09:00 Krzysztof Kozlowski <k.kozlowski@samsung.com>:
> >
> > On nie, 2014-11-30 at 21:19 +0900, Tomasz Figa wrote:
> >> Hi Krzysztof,
> >>
(...)
> >
> >> Also isn't a similar problem happening due to power domains? I believe
> >> the whole maudio block is located in a separate power domain but
> >> somehow it doesn't get turned off?
> >
> > There is Maudio power domain... but I think it is not related here.
> 
> Could you somehow check what happens when the maudio power domain is
> turned off and maudio pin controller is accessed? Could you also check
> the difference in power consumption with this domain powered on and
> off?

Hi Tomasz,

Some time ago you asked for some details of Exynos5420 Maudio domain
behavior. Here it goes:

1. mau domain ON, EPL: OFF: boot hang (this was behavior after adding
runtime PM to pl330 driver)

2. maud domain OFF, EPLL OFF: boot hang

3. maud domain ON, EPLL ON: works (current linux-next and mainline)

4. maud domain OFF, EPLL ON: works, almost the same energy consumption
as in (3) (diff: 0.1% so it is smaller than measuring accuracy).

However an unspecified imprecise abort shows up in (4):
[   11.911628] Freeing unused kernel memory: 328K (c0674000 - c06c6000)
[   12.122695] usb 5-1.4: new high-speed USB device number 3 using exynos-ehci
[   12.190011] Unhandled fault: imprecise external abort (0x1406) at 0x00000000
[   12.219062] usb 5-1.4: New USB device found, idVendor=0b95, idProduct=772a
[   12.224548] usb 5-1.4: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[   12.231794] usb 5-1.4: Product: AX88772
[   12.235681] usb 5-1.4: Manufacturer: ASIX Elec. Corp.
[   12.240707] usb 5-1.4: SerialNumber: 000001
[   12.248835] asix 5-1.4:1.0 (unnamed net_device) (uninitialized): invalid hw address, using random
root@(none):/#

Best regards,
Krzysztof

> 
> > Pinctrl driver does not have runtime PM and is not attached to a domain.
> > I thought about other solution to this problem (with utilization of
> > power domains):
> >  - add runtime PM to pinctrl and audss clocks,
> >  - attach pinctrl and audss clocks to maudio power domain,
> >  - enable the clock when power domain is turned on.
> > However almost the same changes had to be added to pinctrl and audss
> > clocks drivers (replace clock_enable() with pm_runtime_get_sync()).
> 
> Well, if the dependency is there due to hardware design, I think it
> needs to be reflected in the drivers as well.
> 
> Few other issues that came to my mind:
> 
>  - Your previous patch added clock control only to pinctrl operations.
> Shouldn't GPIO operations be included as well?
> 
>  - If power domain dependency is there too, what happens with GPIO
> pins if the domain is powered off? If they lose their states, wouldn't
> it necessary to keep the domain powered on whenever there is some GPIO
> pin requested (which usually = in use)? (I'd assume that special
> functions shouldn't take a reference on runtime PM, because they are
> related to IP blocks in the same PM domain, which will implicitly keep
> the domain powered on.)
> 
>  - Doesn't the clock controller also lose its state whenever the power
> domain is powered off? I guess that would be similar to ISP clock
> controller, issues of which are still not resolved in mainline.
> Sylwester could tell you more about this, I guess.
> 
> Best regards,
> Tomasz