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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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@bp.renesas.com>
> > 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@bp.renesas.com>
> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > 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