Hi Biju,
On Thu, Apr 28, 2022 at 8:53 AM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> As the resets DT property is mandatory, and is present in all .dtsi
> in mainline, add support to perform deassert/assert using reference
> counted reset handle.
>
> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> Reviewed-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
Thanks for your patch!
Unfortunately this patch causes a lock-up during boot on the Koelsch
development board.
Adding some debug code reveals that the VSP1 registers are accessed
while the reset is still asserted:
| WARNING: CPU: 0 PID: 1 at
drivers/media/platform/renesas/vsp1/vsp1.h:121 vsp1_read+0x48/0x74
| reset not deasserted
| CPU: 0 PID: 1 Comm: swapper/0 Tainted: G W
5.18.0-rc5-shmobile-04787-g175dd1b77531-dirty #1230
| Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
| unwind_backtrace from show_stack+0x10/0x14
| show_stack from dump_stack_lvl+0x40/0x4c
| dump_stack_lvl from __warn+0xa0/0x124
| __warn from warn_slowpath_fmt+0x78/0xb0
| warn_slowpath_fmt from vsp1_read+0x48/0x74
| vsp1_read from vsp1_reset_wpf+0x14/0x90
| vsp1_reset_wpf from vsp1_pm_runtime_resume+0x3c/0x1c0
| vsp1_pm_runtime_resume from genpd_runtime_resume+0xfc/0x1bc
vsp1_pm_runtime_resume() initializes the VSP1.
| genpd_runtime_resume from __rpm_callback+0x3c/0x114
| __rpm_callback from rpm_callback+0x50/0x54
| rpm_callback from rpm_resume+0x3e4/0x47c
| rpm_resume from __pm_runtime_resume+0x38/0x50
| __pm_runtime_resume from __device_attach+0xbc/0x148
| __device_attach from bus_probe_device+0x28/0x80
__device_attach() calls "pm_runtime_get_sync(dev->parent)",
bypassing vsp1_device_get().
Hence it wakes the parent, but does not deassert reset.
| bus_probe_device from device_add+0x560/0x784
| device_add from cdev_device_add+0x20/0x58
| cdev_device_add from media_devnode_register+0x1b8/0x28c
| media_devnode_register from __media_device_register+0xb0/0x198
| __media_device_register from vsp1_probe+0xf74/0xfe0
| vsp1_probe from platform_probe+0x58/0xa8
| platform_probe from really_probe+0x138/0x29c
| really_probe from __driver_probe_device+0xc4/0xd8
| __driver_probe_device from driver_probe_device+0x40/0xc0
| driver_probe_device from __driver_attach+0xd4/0xe8
| __driver_attach from bus_for_each_dev+0x64/0xa8
| bus_for_each_dev from bus_add_driver+0x148/0x1a8
| bus_add_driver from driver_register+0xac/0xf0
| driver_register from do_one_initcall+0x70/0x16c
| do_one_initcall from kernel_init_freeable+0x1ac/0x1f8
| kernel_init_freeable from kernel_init+0x14/0x12c
| kernel_init from ret_from_fork+0x14/0x2c
> --- a/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_drv.c
> @@ -567,7 +568,17 @@ static void vsp1_mask_all_interrupts(struct vsp1_device *vsp1)
> */
> int vsp1_device_get(struct vsp1_device *vsp1)
> {
> - return pm_runtime_resume_and_get(vsp1->dev);
> + int ret;
> +
> + ret = reset_control_deassert(vsp1->rstc);
> + if (ret < 0)
> + return ret;
Perhaps you can move the deassertion of the reset to
vsp1_pm_runtime_resume(), so it is called automatically on every
resume?
> +
> + ret = pm_runtime_resume_and_get(vsp1->dev);
> + if (ret < 0)
> + reset_control_assert(vsp1->rstc);
> +
> + return ret;
> }
>
> /*
> @@ -579,6 +590,7 @@ int vsp1_device_get(struct vsp1_device *vsp1)
> void vsp1_device_put(struct vsp1_device *vsp1)
> {
> pm_runtime_put_sync(vsp1->dev);
> + reset_control_assert(vsp1->rstc);
Likewise, move to vsp1_pm_runtime_suspend()?
> }
>
> /* -----------------------------------------------------------------------------
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
