mbox series

[0/3] adv748x: Add support for s2ram

Message ID 20201122163637.3590465-1-niklas.soderlund+renesas@ragnatech.se (mailing list archive)
Headers show
Series adv748x: Add support for s2ram | expand

Message

Niklas Söderlund Nov. 22, 2020, 4:36 p.m. UTC
Hello,

This series enables usage of the ADV748x after the system have been 
suspended to ram. During s2ram the ADV748x may be powered down and thus 
lose its configuration from probe time. The configuration contains  
among other things the i2c slave address mappings for the different 
blocks inside the ADV748x. If this is lost the hardware listens to the 
"wrong" i2c addresses and becomes inaccessible.

Example trying to read the analog standard before and after s2ram with 
and without this this series.

Without this series,

  # subdev=$(grep -l "adv748x 4-0070 afe" /sys/class/video4linux/*/name | sed 's#.*video4linux\(.*\)/name#/dev\1#g')
  # v4l2-ctl --get-detected-standard -d $subdev
  Video Standard = 0x000000ff
          PAL-B/B1/G/H/I/D/D1/K
  # echo on > /sys/bus/i2c/drivers/bd9571mwv/*/bd9571mwv-regulator*/backup_mode
  ** flipp SW23 off **
  # echo mem > /sys/power/state
  ** flipp SW23 on **
  # v4l2-ctl --get-detected-standard -d $subdev
  [  502.753723] adv748x 4-0070: error reading 63, 02
  [  502.866437] adv748x 4-0070: error reading 63, 02
  VIDIOC_QUERYSTD: failed: No such device or address

With this series,

  # subdev=$(grep -l "adv748x 4-0070 afe" /sys/class/video4linux/*/name | sed 's#.*video4linux\(.*\)/name#/dev\1#g')
  # v4l2-ctl --get-detected-standard -d $subdev
  Video Standard = 0x000000ff
          PAL-B/B1/G/H/I/D/D1/K
  # echo on > /sys/bus/i2c/drivers/bd9571mwv/*/bd9571mwv-regulator*/backup_mode
  ** flipp SW23 off **
  # echo mem > /sys/power/state
  ** flipp SW23 on **
  # v4l2-ctl --get-detected-standard -d $subdev
  Video Standard = 0x000000ff
          PAL-B/B1/G/H/I/D/D1/K

Also any streaming while the system is suspended to ram fails to resume 
without this series due to the issue demonstrated above. This series is 
tested on R-Car M3-N on-top of latest media-tree.

Niklas Söderlund (3):
  adv748x: afe: Select input port when device is reset
  adv748x: csi2: Set virtual channel when device is reset
  adv748x: Configure device when resuming from sleep

 drivers/media/i2c/adv748x/adv748x-afe.c  |  6 +----
 drivers/media/i2c/adv748x/adv748x-core.c | 29 ++++++++++++++++++++++--
 drivers/media/i2c/adv748x/adv748x-csi2.c |  6 +----
 drivers/media/i2c/adv748x/adv748x.h      |  2 ++
 4 files changed, 31 insertions(+), 12 deletions(-)

Comments

Sergey Shtylyov Nov. 23, 2020, 8:05 a.m. UTC | #1
On 22.11.2020 19:36, Niklas Söderlund wrote:

> This series enables usage of the ADV748x after the system have been
> suspended to ram. During s2ram the ADV748x may be powered down and thus
> lose its configuration from probe time. The configuration contains
> among other things the i2c slave address mappings for the different
> blocks inside the ADV748x. If this is lost the hardware listens to the
> "wrong" i2c addresses and becomes inaccessible.
> 
> Example trying to read the analog standard before and after s2ram with
> and without this this series.
> 
> Without this series,
> 
>    # subdev=$(grep -l "adv748x 4-0070 afe" /sys/class/video4linux/*/name | sed 's#.*video4linux\(.*\)/name#/dev\1#g')
>    # v4l2-ctl --get-detected-standard -d $subdev
>    Video Standard = 0x000000ff
>            PAL-B/B1/G/H/I/D/D1/K
>    # echo on > /sys/bus/i2c/drivers/bd9571mwv/*/bd9571mwv-regulator*/backup_mode
>    ** flipp SW23 off **

    flip?

>    # echo mem > /sys/power/state
>    ** flipp SW23 on **

    flip?

>    # v4l2-ctl --get-detected-standard -d $subdev
>    [  502.753723] adv748x 4-0070: error reading 63, 02
>    [  502.866437] adv748x 4-0070: error reading 63, 02
>    VIDIOC_QUERYSTD: failed: No such device or address
> 
> With this series,
> 
>    # subdev=$(grep -l "adv748x 4-0070 afe" /sys/class/video4linux/*/name | sed 's#.*video4linux\(.*\)/name#/dev\1#g')
>    # v4l2-ctl --get-detected-standard -d $subdev
>    Video Standard = 0x000000ff
>            PAL-B/B1/G/H/I/D/D1/K
>    # echo on > /sys/bus/i2c/drivers/bd9571mwv/*/bd9571mwv-regulator*/backup_mode
>    ** flipp SW23 off **
>    # echo mem > /sys/power/state
>    ** flipp SW23 on **

    Here as well...

>    # v4l2-ctl --get-detected-standard -d $subdev
>    Video Standard = 0x000000ff
>            PAL-B/B1/G/H/I/D/D1/K
[...]

MBR, Sergei
Kieran Bingham Nov. 25, 2020, 1:09 p.m. UTC | #2
Hi Niklas,

On 22/11/2020 16:36, Niklas Söderlund wrote:
> Hello,
> 
> This series enables usage of the ADV748x after the system have been 
> suspended to ram. During s2ram the ADV748x may be powered down and thus 
> lose its configuration from probe time. The configuration contains  
> among other things the i2c slave address mappings for the different 
> blocks inside the ADV748x. If this is lost the hardware listens to the 
> "wrong" i2c addresses and becomes inaccessible.
> 
> Example trying to read the analog standard before and after s2ram with 
> and without this this series.
> 

Should we be considering runtime_pm for this instead?

--
Kieran


> Without this series,
> 
>   # subdev=$(grep -l "adv748x 4-0070 afe" /sys/class/video4linux/*/name | sed 's#.*video4linux\(.*\)/name#/dev\1#g')
>   # v4l2-ctl --get-detected-standard -d $subdev
>   Video Standard = 0x000000ff
>           PAL-B/B1/G/H/I/D/D1/K
>   # echo on > /sys/bus/i2c/drivers/bd9571mwv/*/bd9571mwv-regulator*/backup_mode
>   ** flipp SW23 off **
>   # echo mem > /sys/power/state
>   ** flipp SW23 on **
>   # v4l2-ctl --get-detected-standard -d $subdev
>   [  502.753723] adv748x 4-0070: error reading 63, 02
>   [  502.866437] adv748x 4-0070: error reading 63, 02
>   VIDIOC_QUERYSTD: failed: No such device or address
> 
> With this series,
> 
>   # subdev=$(grep -l "adv748x 4-0070 afe" /sys/class/video4linux/*/name | sed 's#.*video4linux\(.*\)/name#/dev\1#g')
>   # v4l2-ctl --get-detected-standard -d $subdev
>   Video Standard = 0x000000ff
>           PAL-B/B1/G/H/I/D/D1/K
>   # echo on > /sys/bus/i2c/drivers/bd9571mwv/*/bd9571mwv-regulator*/backup_mode
>   ** flipp SW23 off **
>   # echo mem > /sys/power/state
>   ** flipp SW23 on **
>   # v4l2-ctl --get-detected-standard -d $subdev
>   Video Standard = 0x000000ff
>           PAL-B/B1/G/H/I/D/D1/K
> 
> Also any streaming while the system is suspended to ram fails to resume 
> without this series due to the issue demonstrated above. This series is 
> tested on R-Car M3-N on-top of latest media-tree.
> 
> Niklas Söderlund (3):
>   adv748x: afe: Select input port when device is reset
>   adv748x: csi2: Set virtual channel when device is reset
>   adv748x: Configure device when resuming from sleep
> 
>  drivers/media/i2c/adv748x/adv748x-afe.c  |  6 +----
>  drivers/media/i2c/adv748x/adv748x-core.c | 29 ++++++++++++++++++++++--
>  drivers/media/i2c/adv748x/adv748x-csi2.c |  6 +----
>  drivers/media/i2c/adv748x/adv748x.h      |  2 ++
>  4 files changed, 31 insertions(+), 12 deletions(-)
>
Niklas Söderlund Nov. 25, 2020, 1:39 p.m. UTC | #3
Hi Kieran,

On 2020-11-25 13:09:39 +0000, Kieran Bingham wrote:
> Hi Niklas,
> 
> On 22/11/2020 16:36, Niklas Söderlund wrote:
> > Hello,
> > 
> > This series enables usage of the ADV748x after the system have been 
> > suspended to ram. During s2ram the ADV748x may be powered down and thus 
> > lose its configuration from probe time. The configuration contains  
> > among other things the i2c slave address mappings for the different 
> > blocks inside the ADV748x. If this is lost the hardware listens to the 
> > "wrong" i2c addresses and becomes inaccessible.
> > 
> > Example trying to read the analog standard before and after s2ram with 
> > and without this this series.
> > 
> 
> Should we be considering runtime_pm for this instead?

I don't think so, why do you think we should?

I opted for this solution because we need fine grain control of when the 
registers are restored when resuming from s2ram. If they are not 
restored before (in my case) the VIN driver is resumed and it was 
streaming at suspend time it will fail as the i2c address map is wrong 
at this time. For this reason the registers are restored in the early 
resume callback.

Second I'm unsure how we could properly test such a solution as I don't 
think we can powerdown the ADV7482 without also s2ram the whole SoC on 
our test platforms as it's power is not controllable by the SoC. For 
example it's not powered down in s2idel.

> 
> --
> Kieran
> 
> 
> > Without this series,
> > 
> >   # subdev=$(grep -l "adv748x 4-0070 afe" /sys/class/video4linux/*/name | sed 's#.*video4linux\(.*\)/name#/dev\1#g')
> >   # v4l2-ctl --get-detected-standard -d $subdev
> >   Video Standard = 0x000000ff
> >           PAL-B/B1/G/H/I/D/D1/K
> >   # echo on > /sys/bus/i2c/drivers/bd9571mwv/*/bd9571mwv-regulator*/backup_mode
> >   ** flipp SW23 off **
> >   # echo mem > /sys/power/state
> >   ** flipp SW23 on **
> >   # v4l2-ctl --get-detected-standard -d $subdev
> >   [  502.753723] adv748x 4-0070: error reading 63, 02
> >   [  502.866437] adv748x 4-0070: error reading 63, 02
> >   VIDIOC_QUERYSTD: failed: No such device or address
> > 
> > With this series,
> > 
> >   # subdev=$(grep -l "adv748x 4-0070 afe" /sys/class/video4linux/*/name | sed 's#.*video4linux\(.*\)/name#/dev\1#g')
> >   # v4l2-ctl --get-detected-standard -d $subdev
> >   Video Standard = 0x000000ff
> >           PAL-B/B1/G/H/I/D/D1/K
> >   # echo on > /sys/bus/i2c/drivers/bd9571mwv/*/bd9571mwv-regulator*/backup_mode
> >   ** flipp SW23 off **
> >   # echo mem > /sys/power/state
> >   ** flipp SW23 on **
> >   # v4l2-ctl --get-detected-standard -d $subdev
> >   Video Standard = 0x000000ff
> >           PAL-B/B1/G/H/I/D/D1/K
> > 
> > Also any streaming while the system is suspended to ram fails to resume 
> > without this series due to the issue demonstrated above. This series is 
> > tested on R-Car M3-N on-top of latest media-tree.
> > 
> > Niklas Söderlund (3):
> >   adv748x: afe: Select input port when device is reset
> >   adv748x: csi2: Set virtual channel when device is reset
> >   adv748x: Configure device when resuming from sleep
> > 
> >  drivers/media/i2c/adv748x/adv748x-afe.c  |  6 +----
> >  drivers/media/i2c/adv748x/adv748x-core.c | 29 ++++++++++++++++++++++--
> >  drivers/media/i2c/adv748x/adv748x-csi2.c |  6 +----
> >  drivers/media/i2c/adv748x/adv748x.h      |  2 ++
> >  4 files changed, 31 insertions(+), 12 deletions(-)
> > 
>
Geert Uytterhoeven Nov. 25, 2020, 1:58 p.m. UTC | #4
Hi Niklas, Kieran,

On Wed, Nov 25, 2020 at 2:40 PM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> On 2020-11-25 13:09:39 +0000, Kieran Bingham wrote:
> > On 22/11/2020 16:36, Niklas Söderlund wrote:
> > > This series enables usage of the ADV748x after the system have been
> > > suspended to ram. During s2ram the ADV748x may be powered down and thus
> > > lose its configuration from probe time. The configuration contains
> > > among other things the i2c slave address mappings for the different
> > > blocks inside the ADV748x. If this is lost the hardware listens to the
> > > "wrong" i2c addresses and becomes inaccessible.
> > >
> > > Example trying to read the analog standard before and after s2ram with
> > > and without this this series.
> >
> > Should we be considering runtime_pm for this instead?
>
> I don't think so, why do you think we should?
>
> I opted for this solution because we need fine grain control of when the
> registers are restored when resuming from s2ram. If they are not
> restored before (in my case) the VIN driver is resumed and it was
> streaming at suspend time it will fail as the i2c address map is wrong
> at this time. For this reason the registers are restored in the early
> resume callback.
>
> Second I'm unsure how we could properly test such a solution as I don't
> think we can powerdown the ADV7482 without also s2ram the whole SoC on
> our test platforms as it's power is not controllable by the SoC. For
> example it's not powered down in s2idel.

Also, there is no need to reset the ADV7482 every time it is
runtime-resumed.  In fact such needless reset may impact performance, as
runtime-resume happens much more frequently than system resume during
the system's lifetime.

Gr{oetje,eeting}s,

                        Geert