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

  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>
---
v8->v9:
 * No change
v7->v8:
 * No Change
v6->v7:
 * No change
v5->v6:
 * Rebased to media_staging and updated commit header
 * Added Rb tag from Laurent
 * Added forward declaration for struct reset_control
 * Updated vsp1_device_get() with changes suggested by Laurent
 * Updated error message for reset_control_get form ctrl->control.
v4->v5:
 * Added Rb tag from Geert
v3->v4:
 * Restored error check for pm_runtime_resume_and_get and calls
   assert() in case of failure.
v2->v3:
 * Added Rb tag from Philipp
 * If reset_control_deassert() failed, return ret directly. 
v1->v2:
 * Used reference counted reset handle to perform deassert/assert
RFC->v1:
 * Added reset support as separate patch
 * Moved rstc just after the bus_master field in struct vsp1_device
RFC:
 * https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220112174612.10773-21-biju.das.jz@bp.renesas.com/
---
 drivers/media/platform/renesas/vsp1/vsp1.h    |  2 ++
 .../media/platform/renesas/vsp1/vsp1_drv.c    | 19 ++++++++++++++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)
  

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?

> +
> +       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@linux-m68k.org

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
  

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.

> 
> > +
> > +       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.

Cheers,
Biju

> 
> >  }
> >
> >  /* ---------------------------------------------------------------------
> --------
> 
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
> 
> 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
  

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.
> 
> > >  }
> > >
> > >  /* -----------------------------------------------------------------------------
  

Hi Geert and Laurent,


> 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?

I moved the calls vsp1_pm_runtime_* and tested the changes on RZ/G1N(R-Car Gen2), RZ/G2M(R-Car M3 tested STR as well) and RZ/G2L, All works OK. 

But for RZ/G1N(R-Car Gen2) clk driver needs an additional 35 microseconds delay
after de-asserting reset, otherwise system hangs.

I will post a patch with the clk changes.

Thanks and regards,
Biju