On Tue, May 03, 2022 at 04:02:56PM +0000, Biju Das wrote:
> Hi Geert,
>
> Thanks for the feedback.
>
> > Subject: Re: [PATCH v9 2/5] media: renesas: vsp1: Add support to
> > deassert/assert reset line
> >
> > 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?
>
> Looks like reset behaviour is different from R-Car Gen2 and Gen3,
> As one uses memory to display and later one uses VSPD to display.
>
> I can see 2 options:
>
> Option 1) move the deassertion of the reset to vsp1_pm_runtime_resume(), as you said.
>
> Or
>
> Option 2) Use reset calls only for Gen3.
>
> I will go with option 1, if there is no issue.
Sounds good to me.
> > > +
> > > + 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()?
>
> Ok.
>
> > > }
> > >
> > > /* -----------------------------------------------------------------------------
--
Regards,
Laurent Pinchart
