Re: [PATCH v9 2/5] media: renesas: vsp1: Add support to deassert/assert reset line

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux