| Message ID | 20210921125015.6977-1-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: imx: drop devlinks to reset-controller node | expand |
Hi Philipp, On Tue, Sep 21, 2021 at 9:51 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > Starting with commit 6b2117ad65f1 ("of: property: fw_devlink: Add > support for "resets" and "pwms""), the imx-drm driver fails to load > due to forever dormant devlinks to the reset-controller node. This > node is never associated with a struct device. > > Drop those links, the reset controller is always available. This deserves a Fixes tag, so that it can land in 5.15. Tested on a imx6dl-pico-pi board and after applying your patch the display controller is probed, thanks: Tested-by: Fabio Estevam <festevam@gmail.com> However, I get the following warning: [ 5.059769] ====================================================== [ 5.059773] WARNING: possible circular locking dependency detected [ 5.059778] 5.15.0-rc1-00003-g14cb023b6dee-dirty #132 Not tainted [ 5.059785] ------------------------------------------------------ [ 5.059789] kworker/u4:2/69 is trying to acquire lock: [ 5.059796] c1752358 (regulator_list_mutex){+.+.}-{3:3}, at: regulator_lock_dependent+0x40/0x294 [ 5.059837] [ 5.059837] but task is already holding lock: [ 5.059841] c176e478 (dma_fence_map){++++}-{0:0}, at: imx_drm_atomic_commit_tail+0x10/0x160 [ 5.059870] [ 5.059870] which lock already depends on the new lock. [ 5.059870] [ 5.059875] [ 5.059875] the existing dependency chain (in reverse order) is: [ 5.059878] [ 5.059878] -> #3 (dma_fence_map){++++}-{0:0}: [ 5.059896] dma_resv_lockdep+0x1c4/0x2b8 [ 5.059909] do_one_initcall+0x78/0x388 [ 5.059921] kernel_init_freeable+0x198/0x22c [ 5.059932] kernel_init+0x10/0x128 [ 5.059946] ret_from_fork+0x14/0x38 [ 5.059953] 0x0 [ 5.059961] [ 5.059961] -> #2 (fs_reclaim){+.+.}-{0:0}: [ 5.059979] kmem_cache_alloc+0x28/0x384 [ 5.059988] __d_alloc+0x20/0x224 [ 5.060001] d_alloc+0x10/0x60 [ 5.060009] d_alloc_parallel+0x48/0xa60 [ 5.060020] __lookup_slow+0x8c/0x178 [ 5.060034] lookup_one_len+0xa0/0xe8 [ 5.060043] start_creating+0x94/0x14c [ 5.060057] debugfs_create_dir+0x10/0x114 [ 5.060069] pinctrl_init+0x1c/0xd4 [ 5.060078] do_one_initcall+0x78/0x388 [ 5.060087] kernel_init_freeable+0x198/0x22c [ 5.060095] kernel_init+0x10/0x128 [ 5.060105] ret_from_fork+0x14/0x38 [ 5.060112] 0x0 [ 5.060117] [ 5.060117] -> #1 (&sb->s_type->i_mutex_key#2){+.+.}-{3:3}: [ 5.060141] simple_recursive_removal+0x54/0x35c [ 5.060154] debugfs_remove+0x30/0x4c [ 5.060165] _regulator_put.part.0+0x30/0x1d8 [ 5.060173] regulator_put+0x2c/0x3c [ 5.060179] release_nodes+0x50/0x178 [ 5.060189] devres_release_group+0xc0/0x13c [ 5.060197] i2c_device_probe+0x12c/0x2bc [ 5.060212] really_probe+0x14c/0x320 [ 5.060223] __driver_probe_device+0x80/0xe4 [ 5.060231] driver_probe_device+0x30/0xd4 [ 5.060240] __driver_attach_async_helper+0x20/0x38 [ 5.060250] async_run_entry_fn+0x20/0xb4 [ 5.060259] process_one_work+0x298/0x7cc [ 5.060272] worker_thread+0x30/0x50c [ 5.060280] kthread+0x154/0x17c [ 5.060290] ret_from_fork+0x14/0x38 [ 5.060297] 0x0 [ 5.060301] [ 5.060301] -> #0 (regulator_list_mutex){+.+.}-{3:3}: [ 5.060318] lock_acquire+0x130/0x400 [ 5.060327] __mutex_lock+0x94/0xa40 [ 5.060338] mutex_lock_nested+0x1c/0x24 [ 5.060347] regulator_lock_dependent+0x40/0x294 [ 5.060359] regulator_enable+0x2c/0xec [ 5.060369] panel_simple_resume+0x38/0x1f4 [ 5.060380] __rpm_callback+0x3c/0x108 [ 5.060391] rpm_callback+0x68/0x70 [ 5.060400] rpm_resume+0x5ec/0x7dc [ 5.060408] __pm_runtime_resume+0x64/0x90 [ 5.060416] panel_simple_prepare+0x2c/0x50 [ 5.060425] imx_pd_bridge_enable+0x10/0x1c [ 5.060433] drm_atomic_bridge_chain_enable+0x80/0xa4 [ 5.060444] drm_atomic_helper_commit_modeset_enables+0x12c/0x26c [ 5.060459] imx_drm_atomic_commit_tail+0x3c/0x160 [ 5.060467] commit_tail+0x9c/0x190 [ 5.060478] drm_atomic_helper_commit+0x158/0x18c [ 5.060487] drm_client_modeset_commit_atomic+0x238/0x284 [ 5.060500] drm_client_modeset_commit_locked+0x60/0x1cc [ 5.060508] drm_client_modeset_commit+0x24/0x40 [ 5.060517] __drm_fb_helper_restore_fbdev_mode_unlocked+0x9c/0xc8 [ 5.060531] drm_fb_helper_set_par+0x38/0x68 [ 5.060539] fbcon_init+0x2bc/0x550 [ 5.060552] visual_init+0xbc/0x104 [ 5.060566] do_bind_con_driver+0x1c8/0x3b0 [ 5.060575] do_take_over_console+0x134/0x1f0 [ 5.060585] do_fbcon_takeover+0x60/0xc0 [ 5.060593] register_framebuffer+0x1c8/0x2e8 [ 5.060603] __drm_fb_helper_initial_config_and_unlock+0x3c8/0x614 [ 5.060612] drm_fbdev_client_hotplug+0xf8/0x194 [ 5.060620] drm_fbdev_generic_setup+0x9c/0x194 [ 5.060629] imx_drm_bind+0xdc/0x120 [ 5.060637] try_to_bring_up_master+0x15c/0x1a8 [ 5.060650] __component_add+0x94/0x140 [ 5.060659] imx_pd_probe+0x130/0x18c [ 5.060669] platform_probe+0x58/0xb8 [ 5.060682] really_probe+0x14c/0x320 [ 5.060690] __driver_probe_device+0x80/0xe4 [ 5.060699] driver_probe_device+0x30/0xd4 [ 5.060708] __device_attach_driver+0x8c/0xb4 [ 5.060717] bus_for_each_drv+0x80/0xd0 [ 5.060727] __device_attach+0xec/0x160 [ 5.060734] bus_probe_device+0x88/0x90 [ 5.060743] deferred_probe_work_func+0x7c/0xac [ 5.060751] process_one_work+0x298/0x7cc [ 5.060761] worker_thread+0x30/0x50c [ 5.060769] kthread+0x154/0x17c [ 5.060779] ret_from_fork+0x14/0x38 [ 5.060787] 0x0 [ 5.060793] [ 5.060793] other info that might help us debug this: [ 5.060793] [ 5.060797] Chain exists of: [ 5.060797] regulator_list_mutex --> fs_reclaim --> dma_fence_map [ 5.060797] [ 5.060819] Possible unsafe locking scenario: [ 5.060819] [ 5.060823] CPU0 CPU1 [ 5.060826] ---- ---- [ 5.060829] lock(dma_fence_map); [ 5.060838] lock(fs_reclaim); [ 5.060848] lock(dma_fence_map); [ 5.060856] lock(regulator_list_mutex); [ 5.060864] [ 5.060864] *** DEADLOCK *** [ 5.060864] [ 5.060868] 13 locks held by kworker/u4:2/69: [ 5.060875] #0: c2006ca8 ((wq_completion)events_unbound){+.+.}-{0:0}, at: process_one_work+0x1ec/0x7cc [ 5.060905] #1: c22bbf28 (deferred_probe_work){+.+.}-{0:0}, at: process_one_work+0x1ec/0x7cc [ 5.060932] #2: c22664d8 (&dev->mutex){....}-{3:3}, at: __device_attach+0x30/0x160 [ 5.060958] #3: c176a0d0 (component_mutex){+.+.}-{3:3}, at: __component_add+0x44/0x140 [ 5.060988] #4: c174cb70 (registration_lock){+.+.}-{3:3}, at: register_framebuffer+0x28/0x2e8 [ 5.061015] #5: c1618504 (console_lock){+.+.}-{0:0}, at: register_framebuffer+0x248/0x2e8 [ 5.061043] #6: c28fa048 (&fb_info->lock){+.+.}-{3:3}, at: register_framebuffer+0x1c0/0x2e8 [ 5.061070] #7: c290d56c (&helper->lock){+.+.}-{3:3}, at: __drm_fb_helper_restore_fbdev_mode_unlocked+0x4c/0xc8 [ 5.061102] #8: c292f0e8 (&dev->master_mutex){+.+.}-{3:3}, at: drm_master_internal_acquire+0x18/0x3c [ 5.061130] #9: c290d450 (&client->modeset_mutex){+.+.}-{3:3}, at: drm_client_modeset_commit_locked+0x20/0x1cc [ 5.061163] #10: c22bbb18 (crtc_ww_class_acquire){+.+.}-{0:0}, at: drm_client_modeset_commit_atomic+0x34/0x284 [ 5.061189] #11: c2930094 (crtc_ww_class_mutex){+.+.}-{3:3}, at: drm_modeset_backoff+0x90/0x228 [ 5.061217] #12: c176e478 (dma_fence_map){++++}-{0:0}, at: imx_drm_atomic_commit_tail+0x10/0x160 [ 5.061242] [ 5.061242] stack backtrace: [ 5.061247] CPU: 0 PID: 69 Comm: kworker/u4:2 Not tainted 5.15.0-rc1-00003-g14cb023b6dee-dirty #132 [ 5.061259] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) [ 5.061267] Workqueue: events_unbound deferred_probe_work_func [ 5.061285] [<c0111938>] (unwind_backtrace) from [<c010be28>] (show_stack+0x10/0x14) [ 5.061307] [<c010be28>] (show_stack) from [<c0e812a8>] (dump_stack_lvl+0x58/0x70) [ 5.061328] [<c0e812a8>] (dump_stack_lvl) from [<c0188f14>] (check_noncircular+0xf4/0x168) [ 5.061345] [<c0188f14>] (check_noncircular) from [<c018cdcc>] (__lock_acquire+0x169c/0x3210) [ 5.061360] [<c018cdcc>] (__lock_acquire) from [<c018f3ac>] (lock_acquire+0x130/0x400) [ 5.061373] [<c018f3ac>] (lock_acquire) from [<c0e92ef8>] (__mutex_lock+0x94/0xa40) [ 5.061389] [<c0e92ef8>] (__mutex_lock) from [<c0e938c0>] (mutex_lock_nested+0x1c/0x24) [ 5.061403] [<c0e938c0>] (mutex_lock_nested) from [<c06af10c>] (regulator_lock_dependent+0x40/0x294) [ 5.061425] [<c06af10c>] (regulator_lock_dependent) from [<c06b2a9c>] (regulator_enable+0x2c/0xec) [ 5.061441] [<c06b2a9c>] (regulator_enable) from [<c07d0b18>] (panel_simple_resume+0x38/0x1f4) [ 5.061459] [<c07d0b18>] (panel_simple_resume) from [<c0808a2c>] (__rpm_callback+0x3c/0x108) [ 5.061476] [<c0808a2c>] (__rpm_callback) from [<c0808b60>] (rpm_callback+0x68/0x70) [ 5.061489] [<c0808b60>] (rpm_callback) from [<c0808684>] (rpm_resume+0x5ec/0x7dc) [ 5.061503] [<c0808684>] (rpm_resume) from [<c08088d8>] (__pm_runtime_resume+0x64/0x90) [ 5.061518] [<c08088d8>] (__pm_runtime_resume) from [<c07d09a8>] (panel_simple_prepare+0x2c/0x50) [ 5.061533] [<c07d09a8>] (panel_simple_prepare) from [<c07cd5ec>] (imx_pd_bridge_enable+0x10/0x1c) [ 5.061548] [<c07cd5ec>] (imx_pd_bridge_enable) from [<c072765c>] (drm_atomic_bridge_chain_enable+0x80/0xa4) [ 5.061565] [<c072765c>] (drm_atomic_bridge_chain_enable) from [<c0705fec>] (drm_atomic_helper_commit_modeset_enables+0x12c/0x26c) [ 5.061585] [<c0705fec>] (drm_atomic_helper_commit_modeset_enables) from [<c07cadac>] (imx_drm_atomic_commit_tail+0x3c/0x160) [ 5.061604] [<c07cadac>] (imx_drm_atomic_commit_tail) from [<c0707d7c>] (commit_tail+0x9c/0x190) [ 5.061621] [<c0707d7c>] (commit_tail) from [<c0707fd0>] (drm_atomic_helper_commit+0x158/0x18c) [ 5.061638] [<c0707fd0>] (drm_atomic_helper_commit) from [<c073d358>] (drm_client_modeset_commit_atomic+0x238/0x284) [ 5.061660] [<c073d358>] (drm_client_modeset_commit_atomic) from [<c073d474>] (drm_client_modeset_commit_locked+0x60/0x1cc) [ 5.061678] [<c073d474>] (drm_client_modeset_commit_locked) from [<c073d604>] (drm_client_modeset_commit+0x24/0x40) [ 5.061695] [<c073d604>] (drm_client_modeset_commit) from [<c070e40c>] (__drm_fb_helper_restore_fbdev_mode_unlocked+0x9c/0xc8) [ 5.061715] [<c070e40c>] (__drm_fb_helper_restore_fbdev_mode_unlocked) from [<c070e4a0>] (drm_fb_helper_set_par+0x38/0x68) [ 5.061730] [<c070e4a0>] (drm_fb_helper_set_par) from [<c0677dd4>] (fbcon_init+0x2bc/0x550) [ 5.061748] [<c0677dd4>] (fbcon_init) from [<c06d7cf0>] (visual_init+0xbc/0x104) [ 5.061768] [<c06d7cf0>] (visual_init) from [<c06d911c>] (do_bind_con_driver+0x1c8/0x3b0) [ 5.061786] [<c06d911c>] (do_bind_con_driver) from [<c06d9660>] (do_take_over_console+0x134/0x1f0) [ 5.061803] [<c06d9660>] (do_take_over_console) from [<c0675a18>] (do_fbcon_takeover+0x60/0xc0) [ 5.061820] [<c0675a18>] (do_fbcon_takeover) from [<c066dec4>] (register_framebuffer+0x1c8/0x2e8) [ 5.061834] [<c066dec4>] (register_framebuffer) from [<c070dda4>] (__drm_fb_helper_initial_config_and_unlock+0x3c8/0x614) [ 5.061851] [<c070dda4>] (__drm_fb_helper_initial_config_and_unlock) from [<c070e6dc>] (drm_fbdev_client_hotplug+0xf8/0x194) [ 5.061869] [<c070e6dc>] (drm_fbdev_client_hotplug) from [<c070e814>] (drm_fbdev_generic_setup+0x9c/0x194) [ 5.061885] [<c070e814>] (drm_fbdev_generic_setup) from [<c07cad2c>] (imx_drm_bind+0xdc/0x120) [ 5.061901] [<c07cad2c>] (imx_drm_bind) from [<c07f2f00>] (try_to_bring_up_master+0x15c/0x1a8) [ 5.061921] [<c07f2f00>] (try_to_bring_up_master) from [<c07f2fe0>] (__component_add+0x94/0x140) [ 5.061937] [<c07f2fe0>] (__component_add) from [<c07cd744>] (imx_pd_probe+0x130/0x18c) [ 5.061953] [<c07cd744>] (imx_pd_probe) from [<c07fd01c>] (platform_probe+0x58/0xb8) [ 5.061971] [<c07fd01c>] (platform_probe) from [<c07fa924>] (really_probe+0x14c/0x320) [ 5.061988] [<c07fa924>] (really_probe) from [<c07fab78>] (__driver_probe_device+0x80/0xe4) [ 5.062003] [<c07fab78>] (__driver_probe_device) from [<c07fac0c>] (driver_probe_device+0x30/0xd4) [ 5.062018] [<c07fac0c>] (driver_probe_device) from [<c07faf28>] (__device_attach_driver+0x8c/0xb4) [ 5.062034] [<c07faf28>] (__device_attach_driver) from [<c07f8ce8>] (bus_for_each_drv+0x80/0xd0) [ 5.062049] [<c07f8ce8>] (bus_for_each_drv) from [<c07fa75c>] (__device_attach+0xec/0x160) [ 5.062063] [<c07fa75c>] (__device_attach) from [<c07f99bc>] (bus_probe_device+0x88/0x90) [ 5.062078] [<c07f99bc>] (bus_probe_device) from [<c07f9e78>] (deferred_probe_work_func+0x7c/0xac) [ 5.062093] [<c07f9e78>] (deferred_probe_work_func) from [<c0147ea0>] (process_one_work+0x298/0x7cc) [ 5.062110] [<c0147ea0>] (process_one_work) from [<c0148404>] (worker_thread+0x30/0x50c) [ 5.062125] [<c0148404>] (worker_thread) from [<c01512b4>] (kthread+0x154/0x17c) [ 5.062142] [<c01512b4>] (kthread) from [<c010011c>] (ret_from_fork+0x14/0x38) [ 5.062156] Exception stack(0xc22bbfb0 to 0xc22bbff8) [ 5.062163] bfa0: ???????? ???????? ???????? ???????? [ 5.062169] bfc0: ???????? ???????? ???????? ???????? ???????? ???????? ???????? ???????? [ 5.062174] bfe0: ???????? ???????? ???????? ???????? ???????? ???????? [ 5.116118] Console: switching to colour frame buffer device 100x30 [ 6.569502] imx-drm display-subsystem: [drm] fb0: imx-drm frame buffer device
On Tue, Sep 21, 2021 at 5:50 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > Starting with commit 6b2117ad65f1 ("of: property: fw_devlink: Add > support for "resets" and "pwms""), the imx-drm driver fails to load > due to forever dormant devlinks to the reset-controller node. This > node is never associated with a struct device. > > Drop those links, the reset controller is always available. > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > arch/arm/mach-imx/src.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/mach-imx/src.c b/arch/arm/mach-imx/src.c > index 95fd1fbb0826..6924ac19a762 100644 > --- a/arch/arm/mach-imx/src.c > +++ b/arch/arm/mach-imx/src.c > @@ -178,9 +178,17 @@ void __init imx_src_init(void) > WARN_ON(!src_base); > > imx_reset_controller.of_node = np; > - if (IS_ENABLED(CONFIG_RESET_CONTROLLER)) > + if (IS_ENABLED(CONFIG_RESET_CONTROLLER)) { > reset_controller_register(&imx_reset_controller); > > + /* > + * The reset-controller node is never associated with a struct > + * device, which trips up fw_devlink=on. As a workaround, drop > + * all devlinks to/from this node. > + */ > + fw_devlink_purge_absent_suppliers(&np->fwnode); > + } > + Hey Philipp, Can you point me to the .dts file that corresponds to this platform? I want to take a closer look. -Saravana
Hi Saravana, On Tue, 2021-09-21 at 07:45 -0700, Saravana Kannan wrote: > On Tue, Sep 21, 2021 at 5:50 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > Starting with commit 6b2117ad65f1 ("of: property: fw_devlink: Add > > support for "resets" and "pwms""), the imx-drm driver fails to load > > due to forever dormant devlinks to the reset-controller node. This > > node is never associated with a struct device. > > > > Drop those links, the reset controller is always available. > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > --- > > arch/arm/mach-imx/src.c | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm/mach-imx/src.c b/arch/arm/mach-imx/src.c > > index 95fd1fbb0826..6924ac19a762 100644 > > --- a/arch/arm/mach-imx/src.c > > +++ b/arch/arm/mach-imx/src.c > > @@ -178,9 +178,17 @@ void __init imx_src_init(void) > > WARN_ON(!src_base); > > > > imx_reset_controller.of_node = np; > > - if (IS_ENABLED(CONFIG_RESET_CONTROLLER)) > > + if (IS_ENABLED(CONFIG_RESET_CONTROLLER)) { > > reset_controller_register(&imx_reset_controller); > > > > + /* > > + * The reset-controller node is never associated with a struct > > + * device, which trips up fw_devlink=on. As a workaround, drop > > + * all devlinks to/from this node. > > + */ > > + fw_devlink_purge_absent_suppliers(&np->fwnode); > > + } > > + > > Hey Philipp, > > Can you point me to the .dts file that corresponds to this platform? I > want to take a closer look. Thank you, the relevant file is arch/arm/boot/dts/imx6qdl.dtsi, device nodes labeled as &src and &ipu1. This is for example included by arch/arm/boot/dts/imx6q-nitrogen6x.dts. There's a second &ipu2 added in arch/arm/boot/dts/imx6qd.dtsi as well. I'm also seeing a few "Fixing up cyclic dependency with ldb/120000.hdmi" messages, presumably due to the various of_graph links between ipu_di and hdmi/ldb ports. regards Philipp
Hi Fabio, On Tue, 2021-09-21 at 10:19 -0300, Fabio Estevam wrote: > Hi Philipp, > > On Tue, Sep 21, 2021 at 9:51 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > Starting with commit 6b2117ad65f1 ("of: property: fw_devlink: Add > > support for "resets" and "pwms""), the imx-drm driver fails to load > > due to forever dormant devlinks to the reset-controller node. This > > node is never associated with a struct device. > > > > Drop those links, the reset controller is always available. > > This deserves a Fixes tag, so that it can land in 5.15. Will do. > Tested on a imx6dl-pico-pi board and after applying your patch the > display controller is probed, thanks: > > Tested-by: Fabio Estevam <festevam@gmail.com> > > However, I get the following warning: Thank you. This does not happen on HDMI-only boards, but on Nitrogen6X with LVDS panel I see the same. regards Philipp
On Tue, Sep 21, 2021 at 8:19 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > Hi Fabio, > > On Tue, 2021-09-21 at 10:19 -0300, Fabio Estevam wrote: > > Hi Philipp, > > > > On Tue, Sep 21, 2021 at 9:51 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > > Starting with commit 6b2117ad65f1 ("of: property: fw_devlink: Add > > > support for "resets" and "pwms""), the imx-drm driver fails to load > > > due to forever dormant devlinks to the reset-controller node. This > > > node is never associated with a struct device. > > > > > > Drop those links, the reset controller is always available. > > > > This deserves a Fixes tag, so that it can land in 5.15. > > Will do. > > > Tested on a imx6dl-pico-pi board and after applying your patch the > > display controller is probed, thanks: > > > > Tested-by: Fabio Estevam <festevam@gmail.com> > > > > However, I get the following warning: > > Thank you. This does not happen on HDMI-only boards, but on Nitrogen6X > with LVDS panel I see the same. Let's hold off this patch for a day or two please. I want to take a closer look to see if I have a more generic fix. -Saravana
On Tue, Sep 21, 2021 at 8:05 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > Hi Saravana, > > On Tue, 2021-09-21 at 07:45 -0700, Saravana Kannan wrote: > > On Tue, Sep 21, 2021 at 5:50 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > > Starting with commit 6b2117ad65f1 ("of: property: fw_devlink: Add > > > support for "resets" and "pwms""), the imx-drm driver fails to load > > > due to forever dormant devlinks to the reset-controller node. This > > > node is never associated with a struct device. > > > > > > Drop those links, the reset controller is always available. > > > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > > --- > > > arch/arm/mach-imx/src.c | 10 +++++++++- > > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/arm/mach-imx/src.c b/arch/arm/mach-imx/src.c > > > index 95fd1fbb0826..6924ac19a762 100644 > > > --- a/arch/arm/mach-imx/src.c > > > +++ b/arch/arm/mach-imx/src.c > > > @@ -178,9 +178,17 @@ void __init imx_src_init(void) > > > WARN_ON(!src_base); > > > > > > imx_reset_controller.of_node = np; > > > - if (IS_ENABLED(CONFIG_RESET_CONTROLLER)) > > > + if (IS_ENABLED(CONFIG_RESET_CONTROLLER)) { > > > reset_controller_register(&imx_reset_controller); > > > > > > + /* > > > + * The reset-controller node is never associated with a struct > > > + * device, which trips up fw_devlink=on. As a workaround, drop > > > + * all devlinks to/from this node. > > > + */ > > > + fw_devlink_purge_absent_suppliers(&np->fwnode); > > > + } > > > + > > > > Hey Philipp, > > > > Can you point me to the .dts file that corresponds to this platform? I > > want to take a closer look. > > Thank you, the relevant file is arch/arm/boot/dts/imx6qdl.dtsi, device > nodes labeled as &src and &ipu1. This is for example included by > arch/arm/boot/dts/imx6q-nitrogen6x.dts. There's a second &ipu2 added in > arch/arm/boot/dts/imx6qd.dtsi as well. Thanks. I'll try to look at it within this week. > I'm also seeing a few "Fixing up cyclic dependency with ldb/120000.hdmi" Those are fine. fw_devlink is just saying it's not enforcing probe ordering between those devices because it sees a dependency cycle and "gives up" trying to enforce probe ordering and lets the usual deferred probe process take over. -Saravana > messages, presumably due to the various of_graph links between ipu_di > and hdmi/ldb ports. > > regards > Philipp
Hi Philipp, On Tue, Sep 21, 2021 at 8:05 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > Hi Saravana, > > On Tue, 2021-09-21 at 07:45 -0700, Saravana Kannan wrote: > > On Tue, Sep 21, 2021 at 5:50 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > > Starting with commit 6b2117ad65f1 ("of: property: fw_devlink: Add > > > support for "resets" and "pwms""), the imx-drm driver fails to load > > > due to forever dormant devlinks to the reset-controller node. This > > > node is never associated with a struct device. > > > > > > Drop those links, the reset controller is always available. > > > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > > --- > > > arch/arm/mach-imx/src.c | 10 +++++++++- > > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/arm/mach-imx/src.c b/arch/arm/mach-imx/src.c > > > index 95fd1fbb0826..6924ac19a762 100644 > > > --- a/arch/arm/mach-imx/src.c > > > +++ b/arch/arm/mach-imx/src.c > > > @@ -178,9 +178,17 @@ void __init imx_src_init(void) > > > WARN_ON(!src_base); > > > > > > imx_reset_controller.of_node = np; > > > - if (IS_ENABLED(CONFIG_RESET_CONTROLLER)) > > > + if (IS_ENABLED(CONFIG_RESET_CONTROLLER)) { > > > reset_controller_register(&imx_reset_controller); > > > > > > + /* > > > + * The reset-controller node is never associated with a struct > > > + * device, which trips up fw_devlink=on. As a workaround, drop > > > + * all devlinks to/from this node. > > > + */ > > > + fw_devlink_purge_absent_suppliers(&np->fwnode); > > > + } > > > + > > > > Hey Philipp, > > > > Can you point me to the .dts file that corresponds to this platform? I > > want to take a closer look. > > Thank you, the relevant file is arch/arm/boot/dts/imx6qdl.dtsi, device > nodes labeled as &src and &ipu1. This is for example included by > arch/arm/boot/dts/imx6q-nitrogen6x.dts. There's a second &ipu2 added in > arch/arm/boot/dts/imx6qd.dtsi as well. I took a closer look at the DT. Is there really no struct device for this node? Based on the DT, it looks like a device would automatically get created for this node. So it looks like it's more of an issue with the device not being probed by a real driver? Why can't you convert the reset controller driver to a proper platform driver instead of this patch? I think that's the right fix. -Saravana
Hi Saravana, (Cc: Michael) On Tue, 2021-09-21 at 09:47 -0700, Saravana Kannan wrote: > Hi Philipp, > > On Tue, Sep 21, 2021 at 8:05 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > Hi Saravana, > > > > On Tue, 2021-09-21 at 07:45 -0700, Saravana Kannan wrote: > > > On Tue, Sep 21, 2021 at 5:50 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > > > Starting with commit 6b2117ad65f1 ("of: property: fw_devlink: Add > > > > support for "resets" and "pwms""), the imx-drm driver fails to load > > > > due to forever dormant devlinks to the reset-controller node. This > > > > node is never associated with a struct device. > > > > > > > > Drop those links, the reset controller is always available. > > > > > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > > > --- > > > > arch/arm/mach-imx/src.c | 10 +++++++++- > > > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/arch/arm/mach-imx/src.c b/arch/arm/mach-imx/src.c > > > > index 95fd1fbb0826..6924ac19a762 100644 > > > > --- a/arch/arm/mach-imx/src.c > > > > +++ b/arch/arm/mach-imx/src.c > > > > @@ -178,9 +178,17 @@ void __init imx_src_init(void) > > > > WARN_ON(!src_base); > > > > > > > > imx_reset_controller.of_node = np; > > > > - if (IS_ENABLED(CONFIG_RESET_CONTROLLER)) > > > > + if (IS_ENABLED(CONFIG_RESET_CONTROLLER)) { > > > > reset_controller_register(&imx_reset_controller); > > > > > > > > + /* > > > > + * The reset-controller node is never associated with a struct > > > > + * device, which trips up fw_devlink=on. As a workaround, drop > > > > + * all devlinks to/from this node. > > > > + */ > > > > + fw_devlink_purge_absent_suppliers(&np->fwnode); > > > > + } > > > > + > > > > > > Hey Philipp, > > > > > > Can you point me to the .dts file that corresponds to this platform? I > > > want to take a closer look. > > > > Thank you, the relevant file is arch/arm/boot/dts/imx6qdl.dtsi, device > > nodes labeled as &src and &ipu1. This is for example included by > > arch/arm/boot/dts/imx6q-nitrogen6x.dts. There's a second &ipu2 added in > > arch/arm/boot/dts/imx6qd.dtsi as well. > > I took a closer look at the DT. Is there really no struct device for > this node? Yes, this node is handled by platform code in arch/arm/mach-imx/src.c as the reset controller register also contains bits to bring up the CPUs. > Based on the DT, it looks like a device would automatically > get created for this node. So it looks like it's more of an issue with > the device not being probed by a real driver? Why can't you convert > the reset controller driver to a proper platform driver instead of > this patch? I think that's the right fix. In this case, I could indeed add a platform driver for the reset controller part. I'm not yet sure that this is always the right thing. I just learned from Michael that there are other cases where fw_devlink follows of_graph links from capture devices to connector nodes that have a compatible, such as "composite-video-connector" or "svideo-connector", but no associated driver. These are just parsed to determine the connector type in drivers/media/v4l2-core/v4l2-fwnode.c. Would you suggest to create platform devices for these as well? regards Philipp
On Wed, Sep 22, 2021 at 7:12 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > Hi Saravana, > > (Cc: Michael) > > On Tue, 2021-09-21 at 09:47 -0700, Saravana Kannan wrote: > > Hi Philipp, > > > > On Tue, Sep 21, 2021 at 8:05 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > > Hi Saravana, > > > > > > On Tue, 2021-09-21 at 07:45 -0700, Saravana Kannan wrote: > > > > On Tue, Sep 21, 2021 at 5:50 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > > > > Starting with commit 6b2117ad65f1 ("of: property: fw_devlink: Add > > > > > support for "resets" and "pwms""), the imx-drm driver fails to load > > > > > due to forever dormant devlinks to the reset-controller node. This > > > > > node is never associated with a struct device. > > > > > > > > > > Drop those links, the reset controller is always available. > > > > > > > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > > > > --- > > > > > arch/arm/mach-imx/src.c | 10 +++++++++- > > > > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/arch/arm/mach-imx/src.c b/arch/arm/mach-imx/src.c > > > > > index 95fd1fbb0826..6924ac19a762 100644 > > > > > --- a/arch/arm/mach-imx/src.c > > > > > +++ b/arch/arm/mach-imx/src.c > > > > > @@ -178,9 +178,17 @@ void __init imx_src_init(void) > > > > > WARN_ON(!src_base); > > > > > > > > > > imx_reset_controller.of_node = np; > > > > > - if (IS_ENABLED(CONFIG_RESET_CONTROLLER)) > > > > > + if (IS_ENABLED(CONFIG_RESET_CONTROLLER)) { > > > > > reset_controller_register(&imx_reset_controller); > > > > > > > > > > + /* > > > > > + * The reset-controller node is never associated with a struct > > > > > + * device, which trips up fw_devlink=on. As a workaround, drop > > > > > + * all devlinks to/from this node. > > > > > + */ > > > > > + fw_devlink_purge_absent_suppliers(&np->fwnode); > > > > > + } > > > > > + > > > > > > > > Hey Philipp, > > > > > > > > Can you point me to the .dts file that corresponds to this platform? I > > > > want to take a closer look. > > > > > > Thank you, the relevant file is arch/arm/boot/dts/imx6qdl.dtsi, device > > > nodes labeled as &src and &ipu1. This is for example included by > > > arch/arm/boot/dts/imx6q-nitrogen6x.dts. There's a second &ipu2 added in > > > arch/arm/boot/dts/imx6qd.dtsi as well. > > > > I took a closer look at the DT. Is there really no struct device for > > this node? > > Yes, this node is handled by platform code in arch/arm/mach-imx/src.c as Yes, I saw the node is handled in that file. I know it's not currently a platform driver. I wasn't asking about that. I'm asking if a struct device is created for the node or not. Creation of the struct device is done automatically by the OF code and I don't see any code that prevents the creation of the device. Can you please run this on your device and that should answer my question" # find /sys/devices/ -name *reset-controller* I'm asking this question because there is a cleaner way to fix this instead of what you are doing if there is actually a struct device created for the node. > the reset controller register also contains bits to bring up the CPUs. Ok, I initially assumed that's why you were doing this, but I didn't see this &src node (the reset controller) being referenced anywhere else in DT. So how does that work? > > > Based on the DT, it looks like a device would automatically > > get created for this node. So it looks like it's more of an issue with > > the device not being probed by a real driver? Why can't you convert > > the reset controller driver to a proper platform driver instead of > > this patch? I think that's the right fix. > > In this case, I could indeed add a platform driver for the reset > controller part. I'm not yet sure that this is always the right thing. Can you dig into that more please? Instead of just deleting the fwnode links? There are also ways to make sure you don't block early users (before driver core is up) while still doing proper ordering for later users (like ipu, etc). I'd like to fix this in the best way possibly so that fw_devlink can do the best ordering it can. > I just learned from Michael that there are other cases where fw_devlink > follows of_graph links from capture devices to connector nodes that have > a compatible, such as "composite-video-connector" or "svideo-connector", > but no associated driver. These are just parsed to determine the > connector type in drivers/media/v4l2-core/v4l2-fwnode.c. Would you > suggest to create platform devices for these as well? remote-endpoints always point to each other. So they create dependency cycles and fw_devlink stops enforcing ordering between them (because it's a cycle and it has no way to know which one depends on the other for probing). So for this specific case, you should get away without writing platform drivers. But in general, if you have a compatible string, there really aren't that many good reasons to not use a proper driver for it. -Saravana
On Wed, 2021-09-22 at 08:38 -0700, Saravana Kannan wrote: [...] > Yes, this node is handled by platform code in arch/arm/mach-imx/src.c as > > Yes, I saw the node is handled in that file. I know it's not currently > a platform driver. I wasn't asking about that. I'm asking if a struct > device is created for the node or not. Creation of the struct device > is done automatically by the OF code and I don't see any code that > prevents the creation of the device. > > Can you please run this on your device and that should answer my question" > # find /sys/devices/ -name *reset-controller* > > I'm asking this question because there is a cleaner way to fix this > instead of what you are doing if there is actually a struct device > created for the node. Oh, I wasn't aware. Yes, /sys/devices/platform/soc/2000000.bus/20d8000.reset-controller does exist. > > the reset controller register also contains bits to bring up the CPUs. > > Ok, I initially assumed that's why you were doing this, but I didn't > see this &src node (the reset controller) being referenced anywhere > else in DT. So how does that work? There are a few functions exported from src.c (imx_enable_cpu, imx_set_cpu_arg/jump) that are used in hotplug.c and platsmp.c. > > > Based on the DT, it looks like a device would automatically > > > get created for this node. So it looks like it's more of an issue with > > > the device not being probed by a real driver? Why can't you convert > > > the reset controller driver to a proper platform driver instead of > > > this patch? I think that's the right fix. > > > > In this case, I could indeed add a platform driver for the reset > > controller part. I'm not yet sure that this is always the right thing. > > Can you dig into that more please? Instead of just deleting the fwnode > links? There are also ways to make sure you don't block early users > (before driver core is up) while still doing proper ordering for later > users (like ipu, etc). I'd like to fix this in the best way possibly > so that fw_devlink can do the best ordering it can. I'll send a patch adding the platform driver, but could you elaborate on what could be done with the auto-generated reset-controller device instead? [...] > remote-endpoints always point to each other. So they create dependency > cycles and fw_devlink stops enforcing ordering between them (because > it's a cycle and it has no way to know which one depends on the other > for probing). So for this specific case, you should get away without > writing platform drivers. Right, thank you, I assume there must have been a missing backlink then. regards Philipp
On Wed, Sep 22, 2021 at 9:31 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > On Wed, 2021-09-22 at 08:38 -0700, Saravana Kannan wrote: > [...] > > Yes, this node is handled by platform code in arch/arm/mach-imx/src.c as > > > > Yes, I saw the node is handled in that file. I know it's not currently > > a platform driver. I wasn't asking about that. I'm asking if a struct > > device is created for the node or not. Creation of the struct device > > is done automatically by the OF code and I don't see any code that > > prevents the creation of the device. > > > > Can you please run this on your device and that should answer my question" > > # find /sys/devices/ -name *reset-controller* > > > > I'm asking this question because there is a cleaner way to fix this > > instead of what you are doing if there is actually a struct device > > created for the node. > > Oh, I wasn't aware. Yes, > /sys/devices/platform/soc/2000000.bus/20d8000.reset-controller > does exist. > > > > the reset controller register also contains bits to bring up the CPUs. > > > > Ok, I initially assumed that's why you were doing this, but I didn't > > see this &src node (the reset controller) being referenced anywhere > > else in DT. So how does that work? > > There are a few functions exported from src.c (imx_enable_cpu, > imx_set_cpu_arg/jump) that are used in hotplug.c and platsmp.c. > > > > > Based on the DT, it looks like a device would automatically > > > > get created for this node. So it looks like it's more of an issue with > > > > the device not being probed by a real driver? Why can't you convert > > > > the reset controller driver to a proper platform driver instead of > > > > this patch? I think that's the right fix. > > > > > > In this case, I could indeed add a platform driver for the reset > > > controller part. I'm not yet sure that this is always the right thing. > > > > Can you dig into that more please? Instead of just deleting the fwnode > > links? There are also ways to make sure you don't block early users > > (before driver core is up) while still doing proper ordering for later > > users (like ipu, etc). I'd like to fix this in the best way possibly > > so that fw_devlink can do the best ordering it can. > > I'll send a patch adding the platform driver, but could you elaborate on > what could be done with the auto-generated reset-controller device > instead? Your platform driver would basically match with this auto generated platform device. You would just change arch/arm/mach-imx/src.c to add platform_driver and the probe() would pretty much just do something like: ... probe(...) { WARN_ON(!src_base); imx_reset_controller.of_node = pdev->dev.of_node; return reset_controller_register(&imx_reset_controller); } I don't think you need the IS_ENABLED() check since reset_controller_register() is already stubbed out based on that. You'll also leave the rest of the code in imx_src_int() since you'll need those for your CPU reset to work. Another option you could do instead of writing a platform driver is something like this: https://lore.kernel.org/lkml/20210205222644.2357303-8-saravanak@google.com/ But the benefit of writing a platform driver is that you'll also get proper suspend/resume ordering (it might not matter in your case) between the reset controller and it's non-CPU consumers. I'm okay with you picking either option if you don't have any suspend/resume ordering requirements between src vs its consumers, but I think one of these options is better than the current patch that just deletes the fwnode links. -Saravana
On Wed, 22 Sep 2021 18:31:17 +0200, Philipp Zabel wrote: > On Wed, 2021-09-22 at 08:38 -0700, Saravana Kannan wrote: [...] > > remote-endpoints always point to each other. So they create dependency > > cycles and fw_devlink stops enforcing ordering between them (because > > it's a cycle and it has no way to know which one depends on the other > > for probing). So for this specific case, you should get away without > > writing platform drivers. > > Right, thank you, I assume there must have been a missing backlink then. I looked into this and found that remote-endpoints are actually working as expected. The issue that I was observing was also caused by the missing reset controller driver. Thanks, Michael
diff --git a/arch/arm/mach-imx/src.c b/arch/arm/mach-imx/src.c index 95fd1fbb0826..6924ac19a762 100644 --- a/arch/arm/mach-imx/src.c +++ b/arch/arm/mach-imx/src.c @@ -178,9 +178,17 @@ void __init imx_src_init(void) WARN_ON(!src_base); imx_reset_controller.of_node = np; - if (IS_ENABLED(CONFIG_RESET_CONTROLLER)) + if (IS_ENABLED(CONFIG_RESET_CONTROLLER)) { reset_controller_register(&imx_reset_controller); + /* + * The reset-controller node is never associated with a struct + * device, which trips up fw_devlink=on. As a workaround, drop + * all devlinks to/from this node. + */ + fw_devlink_purge_absent_suppliers(&np->fwnode); + } + /* * force warm reset sources to generate cold reset * for a more reliable restart
Starting with commit 6b2117ad65f1 ("of: property: fw_devlink: Add support for "resets" and "pwms""), the imx-drm driver fails to load due to forever dormant devlinks to the reset-controller node. This node is never associated with a struct device. Drop those links, the reset controller is always available. Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> --- arch/arm/mach-imx/src.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) base-commit: e4e737bb5c170df6135a127739a9e6148ee3da82