* Re: [PATCH v2] clk: samsung: exynos5420: Keep top G3D clocks enabled
2019-12-16 13:14 ` [PATCH v2] clk: samsung: exynos5420: Keep top G3D clocks enabled Marek Szyprowski
@ 2019-12-18 9:44 ` Krzysztof Kozlowski
2019-12-19 2:26 ` Chanwoo Choi
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Krzysztof Kozlowski @ 2019-12-18 9:44 UTC (permalink / raw)
To: Marek Szyprowski
Cc: linux-clk, linux-samsung-soc, Sylwester Nawrocki, Chanwoo Choi,
Bartlomiej Zolnierkiewicz, Marian Mihailescu, Stephen Boyd
On Mon, Dec 16, 2019 at 02:14:07PM +0100, Marek Szyprowski wrote:
> In Exynos542x/5800 SoCs, the G3D leaf clocks are located in the G3D power
> domain. This is similar to the other hardware modules and their power
> domains. However there is one thing specific to G3D clocks hierarchy.
> Unlike other hardware modules, the G3D clocks hierarchy doesn't have any
> gate clock between the TOP part of the hierarchy and the part located in
> the power domain and some SoC internal busses are sourced directly from
> the TOP muxes. The consequence of this design if the fact that the TOP
> part of the hierarchy has to be enabled permanently to ensure proper
> operation of the SoC power related components (G3D power domain and
> Exynos Power Management Unit for system suspend/resume).
>
> This patch adds an explicit call to clk_prepare_enable() on the last MUX
> in the TOP part of G3D clock hierarchy to keep it enabled permanently to
> ensure that the internal busses get their clock regardless of the main
> G3D clock enablement status.
>
> This fixes following imprecise abort issue observed on Odroid XU3/XU4
> after enabling Panfrost driver by commit 1a5a85c56402 "ARM: dts: exynos:
> Add Mali/GPU node on Exynos5420 and enable it on Odroid XU3/4"):
>
> panfrost 11800000.gpu: clock rate = 400000000
> panfrost 11800000.gpu: failed to get regulator: -517
> panfrost 11800000.gpu: regulator init failed -517
> Power domain G3D disable failed
> ...
> panfrost 11800000.gpu: clock rate = 400000000
> 8<--- cut here ---
> Unhandled fault: imprecise external abort (0x1406) at 0x00000000
> pgd = (ptrval)
> [00000000] *pgd=00000000
> Internal error: : 1406 [#1] PREEMPT SMP ARM
> Modules linked in:
> CPU: 7 PID: 53 Comm: kworker/7:1 Not tainted 5.4.0-rc8-next-20191119-00032-g56f1001191a6 #6923
> Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> Workqueue: events deferred_probe_work_func
> PC is at panfrost_gpu_soft_reset+0x94/0x110
> LR is at ___might_sleep+0x128/0x2dc
> ...
> [<c05c231c>] (panfrost_gpu_soft_reset) from [<c05c2704>] (panfrost_gpu_init+0x10/0x67c)
> [<c05c2704>] (panfrost_gpu_init) from [<c05c15d0>] (panfrost_device_init+0x158/0x2cc)
> [<c05c15d0>] (panfrost_device_init) from [<c05c0cb0>] (panfrost_probe+0x80/0x178)
> [<c05c0cb0>] (panfrost_probe) from [<c05cfaa0>] (platform_drv_probe+0x48/0x9c)
> [<c05cfaa0>] (platform_drv_probe) from [<c05cd20c>] (really_probe+0x1c4/0x474)
> [<c05cd20c>] (really_probe) from [<c05cd694>] (driver_probe_device+0x78/0x1bc)
> [<c05cd694>] (driver_probe_device) from [<c05cb374>] (bus_for_each_drv+0x74/0xb8)
> [<c05cb374>] (bus_for_each_drv) from [<c05ccfa8>] (__device_attach+0xd4/0x16c)
> [<c05ccfa8>] (__device_attach) from [<c05cc110>] (bus_probe_device+0x88/0x90)
> [<c05cc110>] (bus_probe_device) from [<c05cc634>] (deferred_probe_work_func+0x4c/0xd0)
> [<c05cc634>] (deferred_probe_work_func) from [<c0149df0>] (process_one_work+0x300/0x864)
> [<c0149df0>] (process_one_work) from [<c014a3ac>] (worker_thread+0x58/0x5a0)
> [<c014a3ac>] (worker_thread) from [<c0151174>] (kthread+0x12c/0x160)
> [<c0151174>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20)
> Exception stack(0xee03dfb0 to 0xee03dff8)
> ...
> Code: e594300c e5933020 e3130c01 1a00000f (ebefff50).
> ---[ end trace badde2b74a65a540 ]---
>
> In the above case, the Panfrost driver disables G3D clocks after failure
> of getting the needed regulator and return with -EPROVE_DEFER code. This
> causes G3D power domain disable failure and then, during second probe
> an imprecise abort is triggered due to undefined power domain state.
>
> Fixes: 45f10dabb56b ("clk: samsung: exynos5420: Add SET_RATE_PARENT flag to clocks on G3D path")
> Fixes: c9f7567aff31 ("clk: samsung: exynos542x: Move G3D subsystem clocks to its sub-CMU")
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> drivers/clk/samsung/clk-exynos5420.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
Acked-by: Krzysztof Kozlowski <krzk@kernel.org>
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] clk: samsung: exynos5420: Keep top G3D clocks enabled
2019-12-16 13:14 ` [PATCH v2] clk: samsung: exynos5420: Keep top G3D clocks enabled Marek Szyprowski
2019-12-18 9:44 ` Krzysztof Kozlowski
@ 2019-12-19 2:26 ` Chanwoo Choi
2019-12-19 10:05 ` Sylwester Nawrocki
2019-12-24 7:53 ` Stephen Boyd
3 siblings, 0 replies; 6+ messages in thread
From: Chanwoo Choi @ 2019-12-19 2:26 UTC (permalink / raw)
To: Marek Szyprowski, linux-clk, linux-samsung-soc
Cc: Sylwester Nawrocki, Krzysztof Kozlowski,
Bartlomiej Zolnierkiewicz, Marian Mihailescu, Stephen Boyd
On 12/16/19 10:14 PM, Marek Szyprowski wrote:
> In Exynos542x/5800 SoCs, the G3D leaf clocks are located in the G3D power
> domain. This is similar to the other hardware modules and their power
> domains. However there is one thing specific to G3D clocks hierarchy.
> Unlike other hardware modules, the G3D clocks hierarchy doesn't have any
> gate clock between the TOP part of the hierarchy and the part located in
> the power domain and some SoC internal busses are sourced directly from
> the TOP muxes. The consequence of this design if the fact that the TOP
> part of the hierarchy has to be enabled permanently to ensure proper
> operation of the SoC power related components (G3D power domain and
> Exynos Power Management Unit for system suspend/resume).
>
> This patch adds an explicit call to clk_prepare_enable() on the last MUX
> in the TOP part of G3D clock hierarchy to keep it enabled permanently to
> ensure that the internal busses get their clock regardless of the main
> G3D clock enablement status.
>
> This fixes following imprecise abort issue observed on Odroid XU3/XU4
> after enabling Panfrost driver by commit 1a5a85c56402 "ARM: dts: exynos:
> Add Mali/GPU node on Exynos5420 and enable it on Odroid XU3/4"):
>
> panfrost 11800000.gpu: clock rate = 400000000
> panfrost 11800000.gpu: failed to get regulator: -517
> panfrost 11800000.gpu: regulator init failed -517
> Power domain G3D disable failed
> ...
> panfrost 11800000.gpu: clock rate = 400000000
> 8<--- cut here ---
> Unhandled fault: imprecise external abort (0x1406) at 0x00000000
> pgd = (ptrval)
> [00000000] *pgd=00000000
> Internal error: : 1406 [#1] PREEMPT SMP ARM
> Modules linked in:
> CPU: 7 PID: 53 Comm: kworker/7:1 Not tainted 5.4.0-rc8-next-20191119-00032-g56f1001191a6 #6923
> Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> Workqueue: events deferred_probe_work_func
> PC is at panfrost_gpu_soft_reset+0x94/0x110
> LR is at ___might_sleep+0x128/0x2dc
> ...
> [<c05c231c>] (panfrost_gpu_soft_reset) from [<c05c2704>] (panfrost_gpu_init+0x10/0x67c)
> [<c05c2704>] (panfrost_gpu_init) from [<c05c15d0>] (panfrost_device_init+0x158/0x2cc)
> [<c05c15d0>] (panfrost_device_init) from [<c05c0cb0>] (panfrost_probe+0x80/0x178)
> [<c05c0cb0>] (panfrost_probe) from [<c05cfaa0>] (platform_drv_probe+0x48/0x9c)
> [<c05cfaa0>] (platform_drv_probe) from [<c05cd20c>] (really_probe+0x1c4/0x474)
> [<c05cd20c>] (really_probe) from [<c05cd694>] (driver_probe_device+0x78/0x1bc)
> [<c05cd694>] (driver_probe_device) from [<c05cb374>] (bus_for_each_drv+0x74/0xb8)
> [<c05cb374>] (bus_for_each_drv) from [<c05ccfa8>] (__device_attach+0xd4/0x16c)
> [<c05ccfa8>] (__device_attach) from [<c05cc110>] (bus_probe_device+0x88/0x90)
> [<c05cc110>] (bus_probe_device) from [<c05cc634>] (deferred_probe_work_func+0x4c/0xd0)
> [<c05cc634>] (deferred_probe_work_func) from [<c0149df0>] (process_one_work+0x300/0x864)
> [<c0149df0>] (process_one_work) from [<c014a3ac>] (worker_thread+0x58/0x5a0)
> [<c014a3ac>] (worker_thread) from [<c0151174>] (kthread+0x12c/0x160)
> [<c0151174>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20)
> Exception stack(0xee03dfb0 to 0xee03dff8)
> ...
> Code: e594300c e5933020 e3130c01 1a00000f (ebefff50).
> ---[ end trace badde2b74a65a540 ]---
>
> In the above case, the Panfrost driver disables G3D clocks after failure
> of getting the needed regulator and return with -EPROVE_DEFER code. This
> causes G3D power domain disable failure and then, during second probe
> an imprecise abort is triggered due to undefined power domain state.
>
> Fixes: 45f10dabb56b ("clk: samsung: exynos5420: Add SET_RATE_PARENT flag to clocks on G3D path")
> Fixes: c9f7567aff31 ("clk: samsung: exynos542x: Move G3D subsystem clocks to its sub-CMU")
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> drivers/clk/samsung/clk-exynos5420.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
> index 3a991ca1ee36..c9e5a1fb6653 100644
> --- a/drivers/clk/samsung/clk-exynos5420.c
> +++ b/drivers/clk/samsung/clk-exynos5420.c
> @@ -12,6 +12,7 @@
> #include <linux/clk-provider.h>
> #include <linux/of.h>
> #include <linux/of_address.h>
> +#include <linux/clk.h>
>
> #include "clk.h"
> #include "clk-cpu.h"
> @@ -1646,6 +1647,13 @@ static void __init exynos5x_clk_init(struct device_node *np,
> exynos5x_subcmus);
> }
>
> + /*
> + * Keep top part of G3D clock path enabled permanently to ensure
> + * that the internal busses get their clock regardless of the
> + * main G3D clock enablement status.
> + */
> + clk_prepare_enable(__clk_lookup("mout_sw_aclk_g3d"));
> +
> samsung_clk_of_add_provider(np, ctx);
> }
>
>
I tested it by checking the enable count of 'mout_sw_aclk_g3d'.
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] clk: samsung: exynos5420: Keep top G3D clocks enabled
2019-12-16 13:14 ` [PATCH v2] clk: samsung: exynos5420: Keep top G3D clocks enabled Marek Szyprowski
2019-12-18 9:44 ` Krzysztof Kozlowski
2019-12-19 2:26 ` Chanwoo Choi
@ 2019-12-19 10:05 ` Sylwester Nawrocki
2019-12-24 7:53 ` Stephen Boyd
2019-12-24 7:53 ` Stephen Boyd
3 siblings, 1 reply; 6+ messages in thread
From: Sylwester Nawrocki @ 2019-12-19 10:05 UTC (permalink / raw)
To: Marek Szyprowski, linux-clk, Stephen Boyd
Cc: linux-samsung-soc, Sylwester Nawrocki, Chanwoo Choi,
Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
Marian Mihailescu
On 12/16/19 14:14, Marek Szyprowski wrote:
> In Exynos542x/5800 SoCs, the G3D leaf clocks are located in the G3D power
> domain. This is similar to the other hardware modules and their power
> domains. However there is one thing specific to G3D clocks hierarchy.
> Unlike other hardware modules, the G3D clocks hierarchy doesn't have any
> gate clock between the TOP part of the hierarchy and the part located in
> the power domain and some SoC internal busses are sourced directly from
> the TOP muxes. The consequence of this design if the fact that the TOP
> part of the hierarchy has to be enabled permanently to ensure proper
> operation of the SoC power related components (G3D power domain and
> Exynos Power Management Unit for system suspend/resume).
>
> This patch adds an explicit call to clk_prepare_enable() on the last MUX
> in the TOP part of G3D clock hierarchy to keep it enabled permanently to
> ensure that the internal busses get their clock regardless of the main
> G3D clock enablement status.
> ---
> diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
> index 3a991ca1ee36..c9e5a1fb6653 100644
> --- a/drivers/clk/samsung/clk-exynos5420.c
> +++ b/drivers/clk/samsung/clk-exynos5420.c
> @@ -1646,6 +1647,13 @@ static void __init exynos5x_clk_init(struct device_node *np,
> exynos5x_subcmus);
> }
>
> + /*
> + * Keep top part of G3D clock path enabled permanently to ensure
> + * that the internal busses get their clock regardless of the
> + * main G3D clock enablement status.
> + */
> + clk_prepare_enable(__clk_lookup("mout_sw_aclk_g3d"));
> +
> samsung_clk_of_add_provider(np, ctx);
> }
It's a bit unfortunate we don't store that clock in the driver internally,
which would let us avoid the __clk_lookup() call. I can't come up with any
simple alternative though so let's apply it for now.
Acked-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
I don't have any other bug fix patches for clk/samsung, could you take it
directly into your tree Stephen?
--
Thanks,
Sylwester
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] clk: samsung: exynos5420: Keep top G3D clocks enabled
2019-12-19 10:05 ` Sylwester Nawrocki
@ 2019-12-24 7:53 ` Stephen Boyd
0 siblings, 0 replies; 6+ messages in thread
From: Stephen Boyd @ 2019-12-24 7:53 UTC (permalink / raw)
To: Marek Szyprowski, Sylwester Nawrocki, linux-clk
Cc: linux-samsung-soc, Sylwester Nawrocki, Chanwoo Choi,
Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
Marian Mihailescu
Quoting Sylwester Nawrocki (2019-12-19 02:05:04)
> On 12/16/19 14:14, Marek Szyprowski wrote:
> > In Exynos542x/5800 SoCs, the G3D leaf clocks are located in the G3D power
> > domain. This is similar to the other hardware modules and their power
> > domains. However there is one thing specific to G3D clocks hierarchy.
> > Unlike other hardware modules, the G3D clocks hierarchy doesn't have any
> > gate clock between the TOP part of the hierarchy and the part located in
> > the power domain and some SoC internal busses are sourced directly from
> > the TOP muxes. The consequence of this design if the fact that the TOP
> > part of the hierarchy has to be enabled permanently to ensure proper
> > operation of the SoC power related components (G3D power domain and
> > Exynos Power Management Unit for system suspend/resume).
> >
> > This patch adds an explicit call to clk_prepare_enable() on the last MUX
> > in the TOP part of G3D clock hierarchy to keep it enabled permanently to
> > ensure that the internal busses get their clock regardless of the main
> > G3D clock enablement status.
>
> > ---
> > diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
> > index 3a991ca1ee36..c9e5a1fb6653 100644
> > --- a/drivers/clk/samsung/clk-exynos5420.c
> > +++ b/drivers/clk/samsung/clk-exynos5420.c
>
> > @@ -1646,6 +1647,13 @@ static void __init exynos5x_clk_init(struct device_node *np,
> > exynos5x_subcmus);
> > }
> >
> > + /*
> > + * Keep top part of G3D clock path enabled permanently to ensure
> > + * that the internal busses get their clock regardless of the
> > + * main G3D clock enablement status.
> > + */
> > + clk_prepare_enable(__clk_lookup("mout_sw_aclk_g3d"));
> > +
> > samsung_clk_of_add_provider(np, ctx);
> > }
>
> It's a bit unfortunate we don't store that clock in the driver internally,
> which would let us avoid the __clk_lookup() call. I can't come up with any
> simple alternative though so let's apply it for now.
Yes it would be better to not use __clk_lookup(). I'll apply it for now
but please avoid it somehow in a future patch.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] clk: samsung: exynos5420: Keep top G3D clocks enabled
2019-12-16 13:14 ` [PATCH v2] clk: samsung: exynos5420: Keep top G3D clocks enabled Marek Szyprowski
` (2 preceding siblings ...)
2019-12-19 10:05 ` Sylwester Nawrocki
@ 2019-12-24 7:53 ` Stephen Boyd
3 siblings, 0 replies; 6+ messages in thread
From: Stephen Boyd @ 2019-12-24 7:53 UTC (permalink / raw)
To: Marek Szyprowski, linux-clk, linux-samsung-soc
Cc: Marek Szyprowski, Sylwester Nawrocki, Chanwoo Choi,
Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz,
Marian Mihailescu
Quoting Marek Szyprowski (2019-12-16 05:14:07)
> In Exynos542x/5800 SoCs, the G3D leaf clocks are located in the G3D power
> domain. This is similar to the other hardware modules and their power
> domains. However there is one thing specific to G3D clocks hierarchy.
> Unlike other hardware modules, the G3D clocks hierarchy doesn't have any
> gate clock between the TOP part of the hierarchy and the part located in
> the power domain and some SoC internal busses are sourced directly from
> the TOP muxes. The consequence of this design if the fact that the TOP
> part of the hierarchy has to be enabled permanently to ensure proper
> operation of the SoC power related components (G3D power domain and
> Exynos Power Management Unit for system suspend/resume).
>
> This patch adds an explicit call to clk_prepare_enable() on the last MUX
> in the TOP part of G3D clock hierarchy to keep it enabled permanently to
> ensure that the internal busses get their clock regardless of the main
> G3D clock enablement status.
>
> This fixes following imprecise abort issue observed on Odroid XU3/XU4
> after enabling Panfrost driver by commit 1a5a85c56402 "ARM: dts: exynos:
> Add Mali/GPU node on Exynos5420 and enable it on Odroid XU3/4"):
>
> panfrost 11800000.gpu: clock rate = 400000000
> panfrost 11800000.gpu: failed to get regulator: -517
> panfrost 11800000.gpu: regulator init failed -517
> Power domain G3D disable failed
> ...
> panfrost 11800000.gpu: clock rate = 400000000
> 8<--- cut here ---
Applied to clk-fixes
^ permalink raw reply [flat|nested] 6+ messages in thread