Hi,
This is initial idea to solve dependency between AudioSS clocks
and main clock controller on Exynos platform.
This solves boot failure of Peach Pi/Pit and Arndale Octa [1].
It is only RFC/proof of concept because I would like to hear whether
this is good direction to solve this particular issue (especially that
it touches main clk api).
[1] http://www.spinics.net/lists/linux-samsung-soc/msg39331.html
Best regards,
Krzysztof Kozlowski
Krzysztof Kozlowski (2):
clk: Allow overriding generic ops for clocks
clk: samsung: Fix clock disable failure because domain being gated
drivers/clk/clk-gate.c | 16 ++++++--
drivers/clk/samsung/clk-exynos-audss.c | 69 +++++++++++++++++++++++++++-------
include/linux/clk-provider.h | 5 +++
3 files changed, 74 insertions(+), 16 deletions(-)
--
1.9.1
For clocks depending on some other clock domain one may want to perform
specific ops before actual enable/disable of gate clock. Allow such case
by accepting supplied ops in new exported function:
clk_register_gate_ops().
Signed-off-by: Krzysztof Kozlowski <[email protected]>
---
drivers/clk/clk-gate.c | 16 +++++++++++++---
include/linux/clk-provider.h | 5 +++++
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c
index 51fd87fb7ba6..51802af2d9e7 100644
--- a/drivers/clk/clk-gate.c
+++ b/drivers/clk/clk-gate.c
@@ -117,11 +117,12 @@ EXPORT_SYMBOL_GPL(clk_gate_ops);
* @bit_idx: which bit in the register controls gating of this clock
* @clk_gate_flags: gate-specific flags for this clock
* @lock: shared register lock for this clock
+ * @ops: clk_ops to use for this clock
*/
-struct clk *clk_register_gate(struct device *dev, const char *name,
+struct clk *clk_register_gate_ops(struct device *dev, const char *name,
const char *parent_name, unsigned long flags,
void __iomem *reg, u8 bit_idx,
- u8 clk_gate_flags, spinlock_t *lock)
+ u8 clk_gate_flags, spinlock_t *lock, const struct clk_ops *ops)
{
struct clk_gate *gate;
struct clk *clk;
@@ -142,7 +143,7 @@ struct clk *clk_register_gate(struct device *dev, const char *name,
}
init.name = name;
- init.ops = &clk_gate_ops;
+ init.ops = ops;
init.flags = flags | CLK_IS_BASIC;
init.parent_names = (parent_name ? &parent_name: NULL);
init.num_parents = (parent_name ? 1 : 0);
@@ -161,4 +162,13 @@ struct clk *clk_register_gate(struct device *dev, const char *name,
return clk;
}
+EXPORT_SYMBOL_GPL(clk_register_gate_ops);
+
+struct clk *clk_register_gate(struct device *dev, const char *name,
+ const char *parent_name, unsigned long flags,
+ void __iomem *reg, u8 bit_idx,
+ u8 clk_gate_flags, spinlock_t *lock)
+{
+ return clk_register_gate_ops(dev, name, parent_name, flags, reg, bit_idx, clk_gate_flags, lock, &clk_gate_ops);
+}
EXPORT_SYMBOL_GPL(clk_register_gate);
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index be21af149f11..6cfc77a9f339 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -290,11 +290,16 @@ struct clk_gate {
#define CLK_GATE_HIWORD_MASK BIT(1)
extern const struct clk_ops clk_gate_ops;
+struct clk *clk_register_gate_ops(struct device *dev, const char *name,
+ const char *parent_name, unsigned long flags,
+ void __iomem *reg, u8 bit_idx,
+ u8 clk_gate_flags, spinlock_t *lock, const struct clk_ops *ops);
struct clk *clk_register_gate(struct device *dev, const char *name,
const char *parent_name, unsigned long flags,
void __iomem *reg, u8 bit_idx,
u8 clk_gate_flags, spinlock_t *lock);
+
struct clk_div_table {
unsigned int val;
unsigned int div;
--
1.9.1
Audio subsystem clocks are located in separate block. If parent clock
(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 (no users). The system hang on disabling
unused clocks from audss block.
Whenever system wants to operate on audss clock it has to enable epll
clock.
Signed-off-by: Krzysztof Kozlowski <[email protected]>
Reported-by: Javier Martinez Canillas <[email protected]>
Reported-by: Kevin Hilman <[email protected]>
---
drivers/clk/samsung/clk-exynos-audss.c | 69 +++++++++++++++++++++++++++-------
1 file changed, 56 insertions(+), 13 deletions(-)
diff --git a/drivers/clk/samsung/clk-exynos-audss.c b/drivers/clk/samsung/clk-exynos-audss.c
index acce708ace18..d10286f30b4f 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;
+struct clk *pll_in;
#define ASS_CLK_SRC 0x0
#define ASS_CLK_DIV 0x4
@@ -75,6 +76,48 @@ static const struct of_device_id exynos_audss_clk_of_match[] = {
{},
};
+static int audss_clk_gate_enable(struct clk_hw *hw)
+{
+ int ret;
+
+ if (!IS_ERR(pll_in))
+ clk_prepare_enable(pll_in);
+ ret = clk_gate_ops.enable(hw);
+ if (!IS_ERR(pll_in))
+ clk_disable_unprepare(pll_in);
+
+ return ret;
+}
+
+static void audss_clk_gate_disable(struct clk_hw *hw)
+{
+ if (!IS_ERR(pll_in))
+ clk_prepare_enable(pll_in);
+ clk_gate_ops.disable(hw);
+ if (!IS_ERR(pll_in))
+ clk_disable_unprepare(pll_in);
+}
+
+static int audss_clk_gate_is_enabled(struct clk_hw *hw)
+{
+ int ret;
+
+ if (!IS_ERR(pll_in))
+ clk_prepare_enable(pll_in);
+ ret = clk_gate_ops.is_enabled(hw);
+ if (!IS_ERR(pll_in))
+ clk_disable_unprepare(pll_in);
+
+ return ret;
+}
+
+/* TODO: Also mux and div */
+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,
+};
+
/* register exynos_audss clocks */
static int exynos_audss_clk_probe(struct platform_device *pdev)
{
@@ -83,7 +126,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;
@@ -145,33 +188,33 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
"mout_i2s", 0, reg_base + ASS_CLK_DIV, 8, 4, 0,
&lock);
- clk_table[EXYNOS_SRP_CLK] = clk_register_gate(NULL, "srp_clk",
+ clk_table[EXYNOS_SRP_CLK] = clk_register_gate_ops(NULL, "srp_clk",
"dout_srp", CLK_SET_RATE_PARENT,
- reg_base + ASS_CLK_GATE, 0, 0, &lock);
+ reg_base + ASS_CLK_GATE, 0, 0, &lock, &audss_clk_gate_ops);
- clk_table[EXYNOS_I2S_BUS] = clk_register_gate(NULL, "i2s_bus",
+ clk_table[EXYNOS_I2S_BUS] = clk_register_gate_ops(NULL, "i2s_bus",
"dout_aud_bus", CLK_SET_RATE_PARENT,
- reg_base + ASS_CLK_GATE, 2, 0, &lock);
+ reg_base + ASS_CLK_GATE, 2, 0, &lock, &audss_clk_gate_ops);
- clk_table[EXYNOS_SCLK_I2S] = clk_register_gate(NULL, "sclk_i2s",
+ clk_table[EXYNOS_SCLK_I2S] = clk_register_gate_ops(NULL, "sclk_i2s",
"dout_i2s", CLK_SET_RATE_PARENT,
- reg_base + ASS_CLK_GATE, 3, 0, &lock);
+ reg_base + ASS_CLK_GATE, 3, 0, &lock, &audss_clk_gate_ops);
- clk_table[EXYNOS_PCM_BUS] = clk_register_gate(NULL, "pcm_bus",
+ clk_table[EXYNOS_PCM_BUS] = clk_register_gate_ops(NULL, "pcm_bus",
"sclk_pcm", CLK_SET_RATE_PARENT,
- reg_base + ASS_CLK_GATE, 4, 0, &lock);
+ reg_base + ASS_CLK_GATE, 4, 0, &lock, &audss_clk_gate_ops);
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",
+ clk_table[EXYNOS_SCLK_PCM] = clk_register_gate_ops(NULL, "sclk_pcm",
sclk_pcm_p, CLK_SET_RATE_PARENT,
- reg_base + ASS_CLK_GATE, 5, 0, &lock);
+ reg_base + ASS_CLK_GATE, 5, 0, &lock, &audss_clk_gate_ops);
if (variant == TYPE_EXYNOS5420) {
- clk_table[EXYNOS_ADMA] = clk_register_gate(NULL, "adma",
+ clk_table[EXYNOS_ADMA] = clk_register_gate_ops(NULL, "adma",
"dout_srp", CLK_SET_RATE_PARENT,
- reg_base + ASS_CLK_GATE, 9, 0, &lock);
+ reg_base + ASS_CLK_GATE, 9, 0, &lock, &audss_clk_gate_ops);
}
for (i = 0; i < clk_data.clk_num; i++) {
--
1.9.1
Hello Krzysztof,
On 11/24/2014 04:18 PM, Krzysztof Kozlowski wrote:
> Hi,
>
>
> This is initial idea to solve dependency between AudioSS clocks
> and main clock controller on Exynos platform.
>
> This solves boot failure of Peach Pi/Pit and Arndale Octa [1].
>
I tested this series on an Exynos5420 Peach Pit but I still
have the boot hang so it doesn't solve the issue on this board.
I'm using linux-next + your patches with exynos_defconfig and
without the clk_ignore_unused command line parameter.
Best regards,
Javier
Quoting Krzysztof Kozlowski (2014-11-24 07:18:31)
> For clocks depending on some other clock domain one may want to perform
> specific ops before actual enable/disable of gate clock. Allow such case
> by accepting supplied ops in new exported function:
> clk_register_gate_ops().
If you are not going to use the gate_ops, why use the gate clock at all?
You can create a platform-specific clock type which uses the ops you
specify.
Regards,
Mike
>
> Signed-off-by: Krzysztof Kozlowski <[email protected]>
> ---
> drivers/clk/clk-gate.c | 16 +++++++++++++---
> include/linux/clk-provider.h | 5 +++++
> 2 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c
> index 51fd87fb7ba6..51802af2d9e7 100644
> --- a/drivers/clk/clk-gate.c
> +++ b/drivers/clk/clk-gate.c
> @@ -117,11 +117,12 @@ EXPORT_SYMBOL_GPL(clk_gate_ops);
> * @bit_idx: which bit in the register controls gating of this clock
> * @clk_gate_flags: gate-specific flags for this clock
> * @lock: shared register lock for this clock
> + * @ops: clk_ops to use for this clock
> */
> -struct clk *clk_register_gate(struct device *dev, const char *name,
> +struct clk *clk_register_gate_ops(struct device *dev, const char *name,
> const char *parent_name, unsigned long flags,
> void __iomem *reg, u8 bit_idx,
> - u8 clk_gate_flags, spinlock_t *lock)
> + u8 clk_gate_flags, spinlock_t *lock, const struct clk_ops *ops)
> {
> struct clk_gate *gate;
> struct clk *clk;
> @@ -142,7 +143,7 @@ struct clk *clk_register_gate(struct device *dev, const char *name,
> }
>
> init.name = name;
> - init.ops = &clk_gate_ops;
> + init.ops = ops;
> init.flags = flags | CLK_IS_BASIC;
> init.parent_names = (parent_name ? &parent_name: NULL);
> init.num_parents = (parent_name ? 1 : 0);
> @@ -161,4 +162,13 @@ struct clk *clk_register_gate(struct device *dev, const char *name,
>
> return clk;
> }
> +EXPORT_SYMBOL_GPL(clk_register_gate_ops);
> +
> +struct clk *clk_register_gate(struct device *dev, const char *name,
> + const char *parent_name, unsigned long flags,
> + void __iomem *reg, u8 bit_idx,
> + u8 clk_gate_flags, spinlock_t *lock)
> +{
> + return clk_register_gate_ops(dev, name, parent_name, flags, reg, bit_idx, clk_gate_flags, lock, &clk_gate_ops);
> +}
> EXPORT_SYMBOL_GPL(clk_register_gate);
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index be21af149f11..6cfc77a9f339 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -290,11 +290,16 @@ struct clk_gate {
> #define CLK_GATE_HIWORD_MASK BIT(1)
>
> extern const struct clk_ops clk_gate_ops;
> +struct clk *clk_register_gate_ops(struct device *dev, const char *name,
> + const char *parent_name, unsigned long flags,
> + void __iomem *reg, u8 bit_idx,
> + u8 clk_gate_flags, spinlock_t *lock, const struct clk_ops *ops);
> struct clk *clk_register_gate(struct device *dev, const char *name,
> const char *parent_name, unsigned long flags,
> void __iomem *reg, u8 bit_idx,
> u8 clk_gate_flags, spinlock_t *lock);
>
> +
> struct clk_div_table {
> unsigned int val;
> unsigned int div;
> --
> 1.9.1
>
On pon, 2014-11-24 at 17:34 -0800, Mike Turquette wrote:
> Quoting Krzysztof Kozlowski (2014-11-24 07:18:31)
> > For clocks depending on some other clock domain one may want to perform
> > specific ops before actual enable/disable of gate clock. Allow such case
> > by accepting supplied ops in new exported function:
> > clk_register_gate_ops().
>
> If you are not going to use the gate_ops, why use the gate clock at all?
> You can create a platform-specific clock type which uses the ops you
> specify.
Actually I want to use gate_ops (and as much as possible from current
code) but I need to encapsulate gate_ops calls in other enable/disable
of other clock. In 2nd patch:
+static int audss_clk_gate_enable(struct clk_hw *hw)
+{
+ int ret;
+
+ if (!IS_ERR(pll_in))
+ clk_prepare_enable(pll_in);
+ ret = clk_gate_ops.enable(hw);
+ if (!IS_ERR(pll_in))
+ clk_disable_unprepare(pll_in);
+
+ return ret;
+}
Other idea is to add some kind of dependency between clocks... but
wait... there is already parent-child dependency. However these audss
clocks need two parents :)
Best regards,
Krzysztof
>
> Regards,
> Mike
>
> >
> > Signed-off-by: Krzysztof Kozlowski <[email protected]>
> > ---
> > drivers/clk/clk-gate.c | 16 +++++++++++++---
> > include/linux/clk-provider.h | 5 +++++
> > 2 files changed, 18 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c
> > index 51fd87fb7ba6..51802af2d9e7 100644
> > --- a/drivers/clk/clk-gate.c
> > +++ b/drivers/clk/clk-gate.c
> > @@ -117,11 +117,12 @@ EXPORT_SYMBOL_GPL(clk_gate_ops);
> > * @bit_idx: which bit in the register controls gating of this clock
> > * @clk_gate_flags: gate-specific flags for this clock
> > * @lock: shared register lock for this clock
> > + * @ops: clk_ops to use for this clock
> > */
> > -struct clk *clk_register_gate(struct device *dev, const char *name,
> > +struct clk *clk_register_gate_ops(struct device *dev, const char *name,
> > const char *parent_name, unsigned long flags,
> > void __iomem *reg, u8 bit_idx,
> > - u8 clk_gate_flags, spinlock_t *lock)
> > + u8 clk_gate_flags, spinlock_t *lock, const struct clk_ops *ops)
> > {
> > struct clk_gate *gate;
> > struct clk *clk;
> > @@ -142,7 +143,7 @@ struct clk *clk_register_gate(struct device *dev, const char *name,
> > }
> >
> > init.name = name;
> > - init.ops = &clk_gate_ops;
> > + init.ops = ops;
> > init.flags = flags | CLK_IS_BASIC;
> > init.parent_names = (parent_name ? &parent_name: NULL);
> > init.num_parents = (parent_name ? 1 : 0);
> > @@ -161,4 +162,13 @@ struct clk *clk_register_gate(struct device *dev, const char *name,
> >
> > return clk;
> > }
> > +EXPORT_SYMBOL_GPL(clk_register_gate_ops);
> > +
> > +struct clk *clk_register_gate(struct device *dev, const char *name,
> > + const char *parent_name, unsigned long flags,
> > + void __iomem *reg, u8 bit_idx,
> > + u8 clk_gate_flags, spinlock_t *lock)
> > +{
> > + return clk_register_gate_ops(dev, name, parent_name, flags, reg, bit_idx, clk_gate_flags, lock, &clk_gate_ops);
> > +}
> > EXPORT_SYMBOL_GPL(clk_register_gate);
> > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > index be21af149f11..6cfc77a9f339 100644
> > --- a/include/linux/clk-provider.h
> > +++ b/include/linux/clk-provider.h
> > @@ -290,11 +290,16 @@ struct clk_gate {
> > #define CLK_GATE_HIWORD_MASK BIT(1)
> >
> > extern const struct clk_ops clk_gate_ops;
> > +struct clk *clk_register_gate_ops(struct device *dev, const char *name,
> > + const char *parent_name, unsigned long flags,
> > + void __iomem *reg, u8 bit_idx,
> > + u8 clk_gate_flags, spinlock_t *lock, const struct clk_ops *ops);
> > struct clk *clk_register_gate(struct device *dev, const char *name,
> > const char *parent_name, unsigned long flags,
> > void __iomem *reg, u8 bit_idx,
> > u8 clk_gate_flags, spinlock_t *lock);
> >
> > +
> > struct clk_div_table {
> > unsigned int val;
> > unsigned int div;
> > --
> > 1.9.1
> >
On pon, 2014-11-24 at 18:28 +0100, Javier Martinez Canillas wrote:
> Hello Krzysztof,
>
> On 11/24/2014 04:18 PM, Krzysztof Kozlowski wrote:
> > Hi,
> >
> >
> > This is initial idea to solve dependency between AudioSS clocks
> > and main clock controller on Exynos platform.
> >
> > This solves boot failure of Peach Pi/Pit and Arndale Octa [1].
> >
>
> I tested this series on an Exynos5420 Peach Pit but I still
> have the boot hang so it doesn't solve the issue on this board.
>
> I'm using linux-next + your patches with exynos_defconfig and
> without the clk_ignore_unused command line parameter.
Hmmm, that's strange. I've looked again at your dmesg and it is little
different than mine (when system fails).
1. Does your PMIC come up? In your dmesg:
[ 2.687297] max77802-pmic max77802-pmic: regulator init failed for 0
[ 2.693539] platform max77802-pmic: Driver max77802-pmic requests probe deferral
2. In my case booting fails just after disabling unused clocks which is
quite early, my dmesg:
[ 11.700074] input: gpio_keys as /devices/platform/gpio_keys/input/input0
[ 11.704282] s3c-rtc 101e0000.rtc: setting system clock to 2029-01-11 19:19:17 UTC (1862853557)
[ 11.770602] mmcblk0: mmc0:0001 M8G1WA 7.28 GiB
[ 11.776201] mmcblk0boot0: mmc0:0001 M8G1WA partition 1 2.00 MiB
[ 11.780968] mmcblk0boot1: mmc0:0001 M8G1WA partition 2 2.00 MiB
[ 11.786823] mmcblk0rpmb: mmc0:0001 M8G1WA partition 3 128 KiB
[ 11.793815] mmcblk0: p1 p2 p3 p4
[ 11.824165] PVDD_G3D_1V0: disabling
[ 11.830833] PVDD_G3DS_1V0: disabling
[ 11.837771] PVDD_HSIC_1V8: disabling
[ 11.841172] PVDD_ABB_1V8: disabling
[ 11.844866] PVDD_ANAIP_1V8: disabling
[ 11.847157] usb 5-1.4: new high-speed USB device number 3 using exynos-ehci
[ 11.848778] PVDD
In your case you have a gap here:
[ 3.624142] platform 12d10000.adc: Driver exynos-adc requests probe deferral
[ 24.188722] random: nonblocking pool is initialized
Could you try my patches once again but this time with enabled:
DEBUG_EXYNOS_UART
EARLY_PRINTK
DEBUG_S3C_UART3
DEBUG_LL
and with following change:
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 4896ae9e23da..316856e55784 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -478,7 +478,9 @@ static void clk_disable_unused_subtree(struct clk *clk)
* sequence. call .disable_unused if available, otherwise fall
* back to .disable
*/
+ pr_err("clk: checking %s\n", clk->name);
if (__clk_is_enabled(clk)) {
+ pr_err("clk: enabled %s, disabling\n", clk->name);
if (clk->ops->disable_unused)
clk->ops->disable_unused(clk->hw);
else if (clk->ops->disable)
Without my patch the boot hangs:
[ 12.230666] clk: checking dout_unipro
[ 12.234304] clk: checking mout_unipro
[ 12.237913] clk: checking sclk_usbd300
[ 12.241638] clk: enabled sclk_usbd300, disabling
[ 12.246264] clk: checking dout_usbd300
[ 12.249992] clk: checking sclk_usbd301
[ 12.253683] clk: enabled sclk_usbd301, disabling
[ 12.258309] clk: checking dout_usbd301
[ 12.262007] clk: checking sclk_maudio0
[ 12.265728] clk: enabled sclk_maudio0, disabling
[ 12.270355] clk: checking pcm_bus
because audss seems to be gated so kernel cannot access status of
pcm_bus clock.
Best regards,
Krzysztof
Hi Krzysztof,
Please see my comments inline.
2014-11-25 0:18 GMT+09:00 Krzysztof Kozlowski <[email protected]>:
> +static int audss_clk_gate_enable(struct clk_hw *hw)
> +{
> + int ret;
> +
> + if (!IS_ERR(pll_in))
> + clk_prepare_enable(pll_in);
Calling clk_prepare_enable() from enable() callback doesn't look like
a good idea, because enabling is not supposed to sleep, while
preparing might do so.
I guess you have to pre-prepare this clock in probe and then only call
enable here.
> + ret = clk_gate_ops.enable(hw);
> + if (!IS_ERR(pll_in))
> + clk_disable_unprepare(pll_in);
> +
> + return ret;
> +}
[snip]
> +/* TODO: Also mux and div */
> +const struct clk_ops audss_clk_gate_ops = {
nit: static const probably?
> + .enable = audss_clk_gate_enable,
> + .disable = audss_clk_gate_disable,
> + .is_enabled = audss_clk_gate_is_enabled,
> +};
As for the approach itself, maybe you should simply register fully
custom clocks with clk_register(), without altering
clk_register_gate() at all and simply calling gate ops whenever
necessary? I don't know, just a loose idea.
By the way, this issue could be probably solved by integrating generic
clocks with regmap API, since regmap-mmio can automatically control a
clock.
Best regards,
Tomasz
On wto, 2014-11-25 at 20:35 +0900, Tomasz Figa wrote:
> Hi Krzysztof,
>
> Please see my comments inline.
>
> 2014-11-25 0:18 GMT+09:00 Krzysztof Kozlowski <[email protected]>:
> > +static int audss_clk_gate_enable(struct clk_hw *hw)
> > +{
> > + int ret;
> > +
> > + if (!IS_ERR(pll_in))
> > + clk_prepare_enable(pll_in);
>
> Calling clk_prepare_enable() from enable() callback doesn't look like
> a good idea, because enabling is not supposed to sleep, while
> preparing might do so.
Right.
> I guess you have to pre-prepare this clock in probe and then only call
> enable here.
Yes, the prepare won't have any negative effect on energy usage anyway.
>
> > + ret = clk_gate_ops.enable(hw);
> > + if (!IS_ERR(pll_in))
> > + clk_disable_unprepare(pll_in);
> > +
> > + return ret;
> > +}
>
> [snip]
>
> > +/* TODO: Also mux and div */
> > +const struct clk_ops audss_clk_gate_ops = {
>
> nit: static const probably?
Yes.
>
> > + .enable = audss_clk_gate_enable,
> > + .disable = audss_clk_gate_disable,
> > + .is_enabled = audss_clk_gate_is_enabled,
> > +};
>
> As for the approach itself, maybe you should simply register fully
> custom clocks with clk_register(), without altering
> clk_register_gate() at all and simply calling gate ops whenever
> necessary? I don't know, just a loose idea.
Initially that seemed to me the simplest way to encapsulate gate_ops
calls. However in that approach I should also change clk_register_mux
and clk_register_divider... which complicates this patch and maybe your
idea will be simpler overall.
> By the way, this issue could be probably solved by integrating generic
> clocks with regmap API, since regmap-mmio can automatically control a
> clock.
Indeed but this looks like much bigger task.
Anyway thanks for feedback. I'll prepare another version.
Best regards,
Krzysztof
Hello Krzysztof,
On 11/25/2014 10:22 AM, Krzysztof Kozlowski wrote:
>
> Hmmm, that's strange. I've looked again at your dmesg and it is little
> different than mine (when system fails).
> 1. Does your PMIC come up? In your dmesg:
> [ 2.687297] max77802-pmic max77802-pmic: regulator init failed for 0
> [ 2.693539] platform max77802-pmic: Driver max77802-pmic requests probe deferral
>
The Peach Pit/Pi DTS define the parent input supplies for the max77802 PMIC
regulators and these are regulators from the tps65090 PMU but the max77802
is probed before. When the system does not fail, the probe is deferred and
succeed later but I don't know when the system fails (more on that below).
> 2. In my case booting fails just after disabling unused clocks which is
> quite early, my dmesg:
> [ 11.700074] input: gpio_keys as /devices/platform/gpio_keys/input/input0
> [ 11.704282] s3c-rtc 101e0000.rtc: setting system clock to 2029-01-11 19:19:17 UTC (1862853557)
> [ 11.770602] mmcblk0: mmc0:0001 M8G1WA 7.28 GiB
> [ 11.776201] mmcblk0boot0: mmc0:0001 M8G1WA partition 1 2.00 MiB
> [ 11.780968] mmcblk0boot1: mmc0:0001 M8G1WA partition 2 2.00 MiB
> [ 11.786823] mmcblk0rpmb: mmc0:0001 M8G1WA partition 3 128 KiB
> [ 11.793815] mmcblk0: p1 p2 p3 p4
> [ 11.824165] PVDD_G3D_1V0: disabling
> [ 11.830833] PVDD_G3DS_1V0: disabling
> [ 11.837771] PVDD_HSIC_1V8: disabling
> [ 11.841172] PVDD_ABB_1V8: disabling
> [ 11.844866] PVDD_ANAIP_1V8: disabling
> [ 11.847157] usb 5-1.4: new high-speed USB device number 3 using exynos-ehci
> [ 11.848778] PVDD
>
> In your case you have a gap here:
> [ 3.624142] platform 12d10000.adc: Driver exynos-adc requests probe deferral
> [ 24.188722] random: nonblocking pool is initialized
>
> Could you try my patches once again but this time with enabled:
> DEBUG_EXYNOS_UART
> EARLY_PRINTK
> DEBUG_S3C_UART3
> DEBUG_LL
>
Yes, there is a gap and I don't know why. Even after enabling all those
config options, nothing is printed on the serial console after the exynos-adc
probe deferral. Only that message in drivers/char/random.c is printed but
nothing else is printed after all...
> and with following change:
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 4896ae9e23da..316856e55784 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -478,7 +478,9 @@ static void clk_disable_unused_subtree(struct clk *clk)
> * sequence. call .disable_unused if available, otherwise fall
> * back to .disable
> */
> + pr_err("clk: checking %s\n", clk->name);
> if (__clk_is_enabled(clk)) {
> + pr_err("clk: enabled %s, disabling\n", clk->name);
> if (clk->ops->disable_unused)
> clk->ops->disable_unused(clk->hw);
> else if (clk->ops->disable)
>
> Without my patch the boot hangs:
> [ 12.230666] clk: checking dout_unipro
> [ 12.234304] clk: checking mout_unipro
> [ 12.237913] clk: checking sclk_usbd300
> [ 12.241638] clk: enabled sclk_usbd300, disabling
> [ 12.246264] clk: checking dout_usbd300
> [ 12.249992] clk: checking sclk_usbd301
> [ 12.253683] clk: enabled sclk_usbd301, disabling
> [ 12.258309] clk: checking dout_usbd301
> [ 12.262007] clk: checking sclk_maudio0
> [ 12.265728] clk: enabled sclk_maudio0, disabling
> [ 12.270355] clk: checking pcm_bus
>
> because audss seems to be gated so kernel cannot access status of
> pcm_bus clock.
>
I added those printk's but clk_disable_unused() is executed after the
exynos-adc probe function so the logs are not shown on the serial console.
As mentioned previously in the thread, Tushar's fix [0] works for me and with
that patch I see the clocks disabled by clk_disable_unused_subtree() and the
kernel is able to access pcm_bus and other audio clocks:
[ 5.007368] clk: checking mout_spdif
[ 5.010908] clk: checking dout_audio0
[ 5.014566] clk: checking mout_audio0
[ 5.018208] clk: checking sclk_maudio0
[ 5.021926] clk: enabled sclk_maudio0, disabling
[ 5.026541] clk: checking pcm_bus
[ 5.029820] clk: enabled pcm_bus, disabling
[ 5.034001] clk: checking sclk_pcm
[ 5.037367] clk: enabled sclk_pcm, disabling
Any ideas?
Best regards,
Javier
[0]: http://www.spinics.net/lists/arm-kernel/msg337970.html
On wto, 2014-11-25 at 14:36 +0100, Javier Martinez Canillas wrote:
> Hello Krzysztof,
>
> On 11/25/2014 10:22 AM, Krzysztof Kozlowski wrote:
> >
> > Hmmm, that's strange. I've looked again at your dmesg and it is little
> > different than mine (when system fails).
> > 1. Does your PMIC come up? In your dmesg:
> > [ 2.687297] max77802-pmic max77802-pmic: regulator init failed for 0
> > [ 2.693539] platform max77802-pmic: Driver max77802-pmic requests probe deferral
> >
>
> The Peach Pit/Pi DTS define the parent input supplies for the max77802 PMIC
> regulators and these are regulators from the tps65090 PMU but the max77802
> is probed before. When the system does not fail, the probe is deferred and
> succeed later but I don't know when the system fails (more on that below).
OK.
>
> > 2. In my case booting fails just after disabling unused clocks which is
> > quite early, my dmesg:
> > [ 11.700074] input: gpio_keys as /devices/platform/gpio_keys/input/input0
> > [ 11.704282] s3c-rtc 101e0000.rtc: setting system clock to 2029-01-11 19:19:17 UTC (1862853557)
> > [ 11.770602] mmcblk0: mmc0:0001 M8G1WA 7.28 GiB
> > [ 11.776201] mmcblk0boot0: mmc0:0001 M8G1WA partition 1 2.00 MiB
> > [ 11.780968] mmcblk0boot1: mmc0:0001 M8G1WA partition 2 2.00 MiB
> > [ 11.786823] mmcblk0rpmb: mmc0:0001 M8G1WA partition 3 128 KiB
> > [ 11.793815] mmcblk0: p1 p2 p3 p4
> > [ 11.824165] PVDD_G3D_1V0: disabling
> > [ 11.830833] PVDD_G3DS_1V0: disabling
> > [ 11.837771] PVDD_HSIC_1V8: disabling
> > [ 11.841172] PVDD_ABB_1V8: disabling
> > [ 11.844866] PVDD_ANAIP_1V8: disabling
> > [ 11.847157] usb 5-1.4: new high-speed USB device number 3 using exynos-ehci
> > [ 11.848778] PVDD
> >
> > In your case you have a gap here:
> > [ 3.624142] platform 12d10000.adc: Driver exynos-adc requests probe deferral
> > [ 24.188722] random: nonblocking pool is initialized
> >
> > Could you try my patches once again but this time with enabled:
> > DEBUG_EXYNOS_UART
> > EARLY_PRINTK
> > DEBUG_S3C_UART3
> > DEBUG_LL
> >
>
> Yes, there is a gap and I don't know why. Even after enabling all those
> config options, nothing is printed on the serial console after the exynos-adc
> probe deferral. Only that message in drivers/char/random.c is printed but
> nothing else is printed after all...
You may try attached patch (very early printk) if you suspect that some
logs are missing.
>
> > and with following change:
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 4896ae9e23da..316856e55784 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -478,7 +478,9 @@ static void clk_disable_unused_subtree(struct clk *clk)
> > * sequence. call .disable_unused if available, otherwise fall
> > * back to .disable
> > */
> > + pr_err("clk: checking %s\n", clk->name);
> > if (__clk_is_enabled(clk)) {
> > + pr_err("clk: enabled %s, disabling\n", clk->name);
> > if (clk->ops->disable_unused)
> > clk->ops->disable_unused(clk->hw);
> > else if (clk->ops->disable)
> >
> > Without my patch the boot hangs:
> > [ 12.230666] clk: checking dout_unipro
> > [ 12.234304] clk: checking mout_unipro
> > [ 12.237913] clk: checking sclk_usbd300
> > [ 12.241638] clk: enabled sclk_usbd300, disabling
> > [ 12.246264] clk: checking dout_usbd300
> > [ 12.249992] clk: checking sclk_usbd301
> > [ 12.253683] clk: enabled sclk_usbd301, disabling
> > [ 12.258309] clk: checking dout_usbd301
> > [ 12.262007] clk: checking sclk_maudio0
> > [ 12.265728] clk: enabled sclk_maudio0, disabling
> > [ 12.270355] clk: checking pcm_bus
> >
> > because audss seems to be gated so kernel cannot access status of
> > pcm_bus clock.
> >
>
> I added those printk's but clk_disable_unused() is executed after the
> exynos-adc probe function so the logs are not shown on the serial console.
So this would mean that something fails before clk_disable_unused().
> As mentioned previously in the thread, Tushar's fix [0] works for me and with
> that patch I see the clocks disabled by clk_disable_unused_subtree() and the
> kernel is able to access pcm_bus and other audio clocks:
>
> [ 5.007368] clk: checking mout_spdif
> [ 5.010908] clk: checking dout_audio0
> [ 5.014566] clk: checking mout_audio0
> [ 5.018208] clk: checking sclk_maudio0
> [ 5.021926] clk: enabled sclk_maudio0, disabling
> [ 5.026541] clk: checking pcm_bus
> [ 5.029820] clk: enabled pcm_bus, disabling
> [ 5.034001] clk: checking sclk_pcm
> [ 5.037367] clk: enabled sclk_pcm, disabling
>
> Any ideas?
Yes, I got. On Peach board the i2s0 is enabled in DTS. Probing it could
fail because it relies on enabling audss clocks (which cannot be
accessed).
I reproduced another hang on Arndale Octa after enabling i2s0 in DTS.
Maybe that is the cause also on Peach.
Best regards,
Krzysztof
Hello Krzysztof,
On 11/25/2014 03:22 PM, Krzysztof Kozlowski wrote:
>> Any ideas?
>
> Yes, I got. On Peach board the i2s0 is enabled in DTS. Probing it could
> fail because it relies on enabling audss clocks (which cannot be
> accessed).
>
> I reproduced another hang on Arndale Octa after enabling i2s0 in DTS.
> Maybe that is the cause also on Peach.
>
You are correct, if I disable i2s0 then I see the logs from
clk_disable_unused_subtree() and boot hangs when accessing the adma clock:
clk: checking dout_fimd1
clk: checking mout_fimd1_final
clk: checking mout_fimd1
clk: checking mout_sclk_rpll
clk: checking fout_rpll
clk: checking adma
With i2s0 disabled, your series are indeed enough to make it boot again but
as you said enabling i2s0 in the DTS makes it hang even with your patches.
Thanks a lot for digging into this!
Best regards,
Javier
On wto, 2014-11-25 at 15:52 +0100, Javier Martinez Canillas wrote:
> Hello Krzysztof,
>
> On 11/25/2014 03:22 PM, Krzysztof Kozlowski wrote:
> >> Any ideas?
> >
> > Yes, I got. On Peach board the i2s0 is enabled in DTS. Probing it could
> > fail because it relies on enabling audss clocks (which cannot be
> > accessed).
> >
> > I reproduced another hang on Arndale Octa after enabling i2s0 in DTS.
> > Maybe that is the cause also on Peach.
> >
>
> You are correct, if I disable i2s0 then I see the logs from
> clk_disable_unused_subtree() and boot hangs when accessing the adma clock:
>
> clk: checking dout_fimd1
> clk: checking mout_fimd1_final
> clk: checking mout_fimd1
> clk: checking mout_sclk_rpll
> clk: checking fout_rpll
> clk: checking adma
>
> With i2s0 disabled, your series are indeed enough to make it boot again but
> as you said enabling i2s0 in the DTS makes it hang even with your patches.
>
> Thanks a lot for digging into this!
My patch only fixed the gate clocks but it didn't touched div and mux.
I'll prepare a v2 of it (I got some feedback) which I hope will fix both
cases: i2s and disabling unused clocks.
Best regards,
Krzysztof
Hello Krzysztof,
On 11/25/2014 03:54 PM, Krzysztof Kozlowski wrote:
>
> My patch only fixed the gate clocks but it didn't touched div and mux.
> I'll prepare a v2 of it (I got some feedback) which I hope will fix both
> cases: i2s and disabling unused clocks.
>
Perfect, I will gladly test on Peach boards once you post it.
Thanks a lot for your help.
Best regards,
Javier