| Message ID | 20191216131407.17225-1-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [v2] clk: samsung: exynos5420: Keep top G3D clocks enabled | expand |
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
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>
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?
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.
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
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); }
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(+)