All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] adv748x: Add support for s2ram
@ 2020-11-22 16:36 Niklas Söderlund
  2020-11-22 16:36 ` [PATCH 1/3] adv748x: afe: Select input port when device is reset Niklas Söderlund
                   ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Niklas Söderlund @ 2020-11-22 16:36 UTC (permalink / raw)
  To: linux-media; +Cc: linux-renesas-soc, Niklas Söderlund

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(-)

-- 
2.29.2


^ permalink raw reply	[flat|nested] 13+ messages in thread
* [PATCH 1/3] adv748x: afe: Select input port when device is reset
  2020-11-22 16:36 [PATCH 0/3] adv748x: Add support for s2ram Niklas Söderlund
@ 2020-11-22 16:36 ` Niklas Söderlund
  2020-11-23  8:00   ` Sergei Shtylyov
  2020-11-25 12:10   ` Kieran Bingham
  2020-11-22 16:36 ` [PATCH 2/3] adv748x: csi2: Set virtual channel " Niklas Söderlund
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 13+ messages in thread
From: Niklas Söderlund @ 2020-11-22 16:36 UTC (permalink / raw)
  To: linux-media; +Cc: linux-renesas-soc, Niklas Söderlund

It's not enough to select the AFE input port during probe it also needs
to be set when the device is reset. Move the port selection to
adv748x_reset() that is called during probe and when the device needs to
be reset.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/i2c/adv748x/adv748x-afe.c  | 6 +-----
 drivers/media/i2c/adv748x/adv748x-core.c | 4 ++++
 drivers/media/i2c/adv748x/adv748x.h      | 1 +
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-afe.c b/drivers/media/i2c/adv748x/adv748x-afe.c
index dbbb1e4d63637a33..4052cf67bf16c7fb 100644
--- a/drivers/media/i2c/adv748x/adv748x-afe.c
+++ b/drivers/media/i2c/adv748x/adv748x-afe.c
@@ -154,7 +154,7 @@ static void adv748x_afe_set_video_standard(struct adv748x_state *state,
 		   (sdpstd & 0xf) << ADV748X_SDP_VID_SEL_SHIFT);
 }
 
-static int adv748x_afe_s_input(struct adv748x_afe *afe, unsigned int input)
+int adv748x_afe_s_input(struct adv748x_afe *afe, unsigned int input)
 {
 	struct adv748x_state *state = adv748x_afe_to_state(afe);
 
@@ -520,10 +520,6 @@ int adv748x_afe_init(struct adv748x_afe *afe)
 		}
 	}
 
-	adv748x_afe_s_input(afe, afe->input);
-
-	adv_dbg(state, "AFE Default input set to %d\n", afe->input);
-
 	/* Entity pads and sinks are 0-indexed to match the pads */
 	for (i = ADV748X_AFE_SINK_AIN0; i <= ADV748X_AFE_SINK_AIN7; i++)
 		afe->pads[i].flags = MEDIA_PAD_FL_SINK;
diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 00966fe104881a14..8676ad2428856dd3 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -516,6 +516,10 @@ static int adv748x_reset(struct adv748x_state *state)
 	if (ret)
 		return ret;
 
+	adv748x_afe_s_input(&state->afe, state->afe.input);
+
+	adv_dbg(state, "AFE Default input set to %d\n", state->afe.input);
+
 	/* Reset TXA and TXB */
 	adv748x_tx_power(&state->txa, 1);
 	adv748x_tx_power(&state->txa, 0);
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index 1061f425ece5989e..747947ea3e316451 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -435,6 +435,7 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on);
 
 int adv748x_afe_init(struct adv748x_afe *afe);
 void adv748x_afe_cleanup(struct adv748x_afe *afe);
+int adv748x_afe_s_input(struct adv748x_afe *afe, unsigned int input);
 
 int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx);
 void adv748x_csi2_cleanup(struct adv748x_csi2 *tx);
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread
* [PATCH 2/3] adv748x: csi2: Set virtual channel when device is reset
  2020-11-22 16:36 [PATCH 0/3] adv748x: Add support for s2ram Niklas Söderlund
  2020-11-22 16:36 ` [PATCH 1/3] adv748x: afe: Select input port when device is reset Niklas Söderlund
@ 2020-11-22 16:36 ` Niklas Söderlund
  2020-11-23  8:06   ` Sergei Shtylyov
  2020-11-22 16:36 ` [PATCH 3/3] adv748x: Configure device when resuming from sleep Niklas Söderlund
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Niklas Söderlund @ 2020-11-22 16:36 UTC (permalink / raw)
  To: linux-media; +Cc: linux-renesas-soc, Niklas Söderlund

It's not enough to set the CSI-2 virtual channel for TXA and TXB during
probe it also needs to be set when the device is reset. Move the virtual
channel selection to adv748x_reset() that is called during probe and
when the device needs to be reset.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/i2c/adv748x/adv748x-core.c | 8 ++++++--
 drivers/media/i2c/adv748x/adv748x-csi2.c | 6 +-----
 drivers/media/i2c/adv748x/adv748x.h      | 1 +
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index 8676ad2428856dd3..b578a01cfebe7217 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -530,10 +530,14 @@ static int adv748x_reset(struct adv748x_state *state)
 	io_write(state, ADV748X_IO_PD, ADV748X_IO_PD_RX_EN);
 
 	/* Conditionally enable TXa and TXb. */
-	if (is_tx_enabled(&state->txa))
+	if (is_tx_enabled(&state->txa)) {
 		regval |= ADV748X_IO_10_CSI4_EN;
-	if (is_tx_enabled(&state->txb))
+		adv748x_csi2_set_virtual_channel(&state->txa, 0);
+	}
+	if (is_tx_enabled(&state->txb)) {
 		regval |= ADV748X_IO_10_CSI1_EN;
+		adv748x_csi2_set_virtual_channel(&state->txb, 0);
+	}
 	io_write(state, ADV748X_IO_10, regval);
 
 	/* Use vid_std and v_freq as freerun resolution for CP */
diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
index 99bb63d05eef1cd5..fa9278a08fdee3bb 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -14,8 +14,7 @@
 
 #include "adv748x.h"
 
-static int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx,
-					    unsigned int vc)
+int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx, unsigned int vc)
 {
 	return tx_write(tx, ADV748X_CSI_VC_REF, vc << ADV748X_CSI_VC_REF_SHIFT);
 }
@@ -313,9 +312,6 @@ int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx)
 	if (!is_tx_enabled(tx))
 		return 0;
 
-	/* Initialise the virtual channel */
-	adv748x_csi2_set_virtual_channel(tx, 0);
-
 	adv748x_subdev_init(&tx->sd, state, &adv748x_csi2_ops,
 			    MEDIA_ENT_F_VID_IF_BRIDGE,
 			    is_txa(tx) ? "txa" : "txb");
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index 747947ea3e316451..56256c1e8b0d3f01 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -439,6 +439,7 @@ int adv748x_afe_s_input(struct adv748x_afe *afe, unsigned int input);
 
 int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx);
 void adv748x_csi2_cleanup(struct adv748x_csi2 *tx);
+int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx, unsigned int vc);
 int adv748x_csi2_set_pixelrate(struct v4l2_subdev *sd, s64 rate);
 
 int adv748x_hdmi_init(struct adv748x_hdmi *hdmi);
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread
* [PATCH 3/3] adv748x: Configure device when resuming from sleep
  2020-11-22 16:36 [PATCH 0/3] adv748x: Add support for s2ram Niklas Söderlund
  2020-11-22 16:36 ` [PATCH 1/3] adv748x: afe: Select input port when device is reset Niklas Söderlund
  2020-11-22 16:36 ` [PATCH 2/3] adv748x: csi2: Set virtual channel " Niklas Söderlund
@ 2020-11-22 16:36 ` Niklas Söderlund
  2020-11-23  8:09   ` Sergei Shtylyov
  2020-11-23  8:05 ` [PATCH 0/3] adv748x: Add support for s2ram Sergei Shtylyov
  2020-11-25 13:09 ` Kieran Bingham
  4 siblings, 1 reply; 13+ messages in thread
From: Niklas Söderlund @ 2020-11-22 16:36 UTC (permalink / raw)
  To: linux-media; +Cc: linux-renesas-soc, Niklas Söderlund

If the device is powered off for example during system suspend to ram
the devices loses its configuration, specially the slave i2c mappings
and other configuration set at probe time. This renders the device
unusable and only way to recover is to unbind and rebind the device to
the driver to run the probe setup again.

Add an early resume callback that reinitializes the device and setup the
slave i2c address mappings and other probe time configuration.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/i2c/adv748x/adv748x-core.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
index b578a01cfebe7217..4e54148147b9adec 100644
--- a/drivers/media/i2c/adv748x/adv748x-core.c
+++ b/drivers/media/i2c/adv748x/adv748x-core.c
@@ -565,6 +565,18 @@ static int adv748x_identify_chip(struct adv748x_state *state)
 	return 0;
 }
 
+/* -----------------------------------------------------------------------------
+ * Suspend / Resume
+ */
+
+static int __maybe_unused adv748x_resume_early(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct adv748x_state *state = i2c_get_clientdata(client);
+
+	return adv748x_reset(state);
+}
+
 /* -----------------------------------------------------------------------------
  * i2c driver
  */
@@ -827,10 +839,15 @@ static const struct of_device_id adv748x_of_table[] = {
 };
 MODULE_DEVICE_TABLE(of, adv748x_of_table);
 
+static const struct dev_pm_ops adv748x_pm_ops = {
+	SET_LATE_SYSTEM_SLEEP_PM_OPS(NULL, adv748x_resume_early)
+};
+
 static struct i2c_driver adv748x_driver = {
 	.driver = {
 		.name = "adv748x",
 		.of_match_table = adv748x_of_table,
+		.pm = &adv748x_pm_ops,
 	},
 	.probe_new = adv748x_probe,
 	.remove = adv748x_remove,
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] adv748x: afe: Select input port when device is reset
  2020-11-22 16:36 ` [PATCH 1/3] adv748x: afe: Select input port when device is reset Niklas Söderlund
@ 2020-11-23  8:00   ` Sergei Shtylyov
  2020-11-25 12:10   ` Kieran Bingham
  1 sibling, 0 replies; 13+ messages in thread
From: Sergei Shtylyov @ 2020-11-23  8:00 UTC (permalink / raw)
  To: Niklas Söderlund, linux-media; +Cc: linux-renesas-soc

Hello!

On 22.11.2020 19:36, Niklas Söderlund wrote:

> It's not enough to select the AFE input port during probe it also needs
                                                            ^ : or --?

> to be set when the device is reset. Move the port selection to
> adv748x_reset() that is called during probe and when the device needs to
> be reset.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
[...]

MBR, Sergei

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH 0/3] adv748x: Add support for s2ram
  2020-11-22 16:36 [PATCH 0/3] adv748x: Add support for s2ram Niklas Söderlund
                   ` (2 preceding siblings ...)
  2020-11-22 16:36 ` [PATCH 3/3] adv748x: Configure device when resuming from sleep Niklas Söderlund
@ 2020-11-23  8:05 ` Sergei Shtylyov
  2020-11-25 13:09 ` Kieran Bingham
  4 siblings, 0 replies; 13+ messages in thread
From: Sergei Shtylyov @ 2020-11-23  8:05 UTC (permalink / raw)
  To: Niklas Söderlund, linux-media; +Cc: linux-renesas-soc

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

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] adv748x: csi2: Set virtual channel when device is reset
  2020-11-22 16:36 ` [PATCH 2/3] adv748x: csi2: Set virtual channel " Niklas Söderlund
@ 2020-11-23  8:06   ` Sergei Shtylyov
  0 siblings, 0 replies; 13+ messages in thread
From: Sergei Shtylyov @ 2020-11-23  8:06 UTC (permalink / raw)
  To: Niklas Söderlund, linux-media; +Cc: linux-renesas-soc

On 22.11.2020 19:36, Niklas Söderlund wrote:

> It's not enough to set the CSI-2 virtual channel for TXA and TXB during
> probe it also needs to be set when the device is reset. Move the virtual
        ^ : or --?

> channel selection to adv748x_reset() that is called during probe and
> when the device needs to be reset.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
[...]

MBR, Sergei

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] adv748x: Configure device when resuming from sleep
  2020-11-22 16:36 ` [PATCH 3/3] adv748x: Configure device when resuming from sleep Niklas Söderlund
@ 2020-11-23  8:09   ` Sergei Shtylyov
  0 siblings, 0 replies; 13+ messages in thread
From: Sergei Shtylyov @ 2020-11-23  8:09 UTC (permalink / raw)
  To: Niklas Söderlund, linux-media; +Cc: linux-renesas-soc

On 22.11.2020 19:36, Niklas Söderlund wrote:

> If the device is powered off for example during system suspend to ram
> the devices loses its configuration, specially the slave i2c mappings

    Especially?

> and other configuration set at probe time. This renders the device
> unusable and only way to recover is to unbind and rebind the device to
> the driver to run the probe setup again.
> 
> Add an early resume callback that reinitializes the device and setup the
> slave i2c address mappings and other probe time configuration.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
[...]

MBR, Sergei

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] adv748x: afe: Select input port when device is reset
  2020-11-22 16:36 ` [PATCH 1/3] adv748x: afe: Select input port when device is reset Niklas Söderlund
  2020-11-23  8:00   ` Sergei Shtylyov
@ 2020-11-25 12:10   ` Kieran Bingham
  2020-11-25 13:16     ` Niklas Söderlund
  1 sibling, 1 reply; 13+ messages in thread
From: Kieran Bingham @ 2020-11-25 12:10 UTC (permalink / raw)
  To: Niklas Söderlund, linux-media; +Cc: linux-renesas-soc

Hi Niklas,

On 22/11/2020 16:36, Niklas Söderlund wrote:
> It's not enough to select the AFE input port during probe it also needs
> to be set when the device is reset. Move the port selection to
> adv748x_reset() that is called during probe and when the device needs to
> be reset.
> 

Should we instead have an adv748x_afe_reset(), rather than expose the
AFE internals to the top level core?

That said, shouldn't we be able to take advantage of regmap to restore
registers in this instance?

--
Kieran


> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/i2c/adv748x/adv748x-afe.c  | 6 +-----
>  drivers/media/i2c/adv748x/adv748x-core.c | 4 ++++
>  drivers/media/i2c/adv748x/adv748x.h      | 1 +
>  3 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-afe.c b/drivers/media/i2c/adv748x/adv748x-afe.c
> index dbbb1e4d63637a33..4052cf67bf16c7fb 100644
> --- a/drivers/media/i2c/adv748x/adv748x-afe.c
> +++ b/drivers/media/i2c/adv748x/adv748x-afe.c
> @@ -154,7 +154,7 @@ static void adv748x_afe_set_video_standard(struct adv748x_state *state,
>  		   (sdpstd & 0xf) << ADV748X_SDP_VID_SEL_SHIFT);
>  }
>  
> -static int adv748x_afe_s_input(struct adv748x_afe *afe, unsigned int input)
> +int adv748x_afe_s_input(struct adv748x_afe *afe, unsigned int input)
>  {
>  	struct adv748x_state *state = adv748x_afe_to_state(afe);
>  
> @@ -520,10 +520,6 @@ int adv748x_afe_init(struct adv748x_afe *afe)
>  		}
>  	}
>  
> -	adv748x_afe_s_input(afe, afe->input);
> -
> -	adv_dbg(state, "AFE Default input set to %d\n", afe->input);
> -
>  	/* Entity pads and sinks are 0-indexed to match the pads */
>  	for (i = ADV748X_AFE_SINK_AIN0; i <= ADV748X_AFE_SINK_AIN7; i++)
>  		afe->pads[i].flags = MEDIA_PAD_FL_SINK;
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index 00966fe104881a14..8676ad2428856dd3 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -516,6 +516,10 @@ static int adv748x_reset(struct adv748x_state *state)
>  	if (ret)
>  		return ret;
>  
> +	adv748x_afe_s_input(&state->afe, state->afe.input);
> +
> +	adv_dbg(state, "AFE Default input set to %d\n", state->afe.input);
> +
>  	/* Reset TXA and TXB */
>  	adv748x_tx_power(&state->txa, 1);
>  	adv748x_tx_power(&state->txa, 0);
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index 1061f425ece5989e..747947ea3e316451 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -435,6 +435,7 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on);
>  
>  int adv748x_afe_init(struct adv748x_afe *afe);
>  void adv748x_afe_cleanup(struct adv748x_afe *afe);
> +int adv748x_afe_s_input(struct adv748x_afe *afe, unsigned int input);
>  
>  int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx);
>  void adv748x_csi2_cleanup(struct adv748x_csi2 *tx);
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH 0/3] adv748x: Add support for s2ram
  2020-11-22 16:36 [PATCH 0/3] adv748x: Add support for s2ram Niklas Söderlund
                   ` (3 preceding siblings ...)
  2020-11-23  8:05 ` [PATCH 0/3] adv748x: Add support for s2ram Sergei Shtylyov
@ 2020-11-25 13:09 ` Kieran Bingham
  2020-11-25 13:39   ` Niklas Söderlund
  4 siblings, 1 reply; 13+ messages in thread
From: Kieran Bingham @ 2020-11-25 13:09 UTC (permalink / raw)
  To: Niklas Söderlund, linux-media; +Cc: linux-renesas-soc

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(-)
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] adv748x: afe: Select input port when device is reset
  2020-11-25 12:10   ` Kieran Bingham
@ 2020-11-25 13:16     ` Niklas Söderlund
  0 siblings, 0 replies; 13+ messages in thread
From: Niklas Söderlund @ 2020-11-25 13:16 UTC (permalink / raw)
  To: Kieran Bingham; +Cc: linux-media, linux-renesas-soc

Hi Kieran,

On 2020-11-25 12:10:08 +0000, Kieran Bingham wrote:
> Hi Niklas,
> 
> On 22/11/2020 16:36, Niklas Söderlund wrote:
> > It's not enough to select the AFE input port during probe it also needs
> > to be set when the device is reset. Move the port selection to
> > adv748x_reset() that is called during probe and when the device needs to
> > be reset.
> > 
> 
> Should we instead have an adv748x_afe_reset(), rather than expose the
> AFE internals to the top level core?

We could, I have no real preference. But in this case all 
adv748x_afe_reset() would do is call adv748x_afe_s_input() so unless we 
foresee more work to be done at reset time my preference would be like 
this but it's your call.

> 
> That said, shouldn't we be able to take advantage of regmap to restore
> registers in this instance?

I'm no regmap expert so I don't know. But if so we need to be sure the 
order of registers match what is needed as we need to restore the i2c 
addresses for all none core "pages".

> 
> --
> Kieran
> 
> 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/i2c/adv748x/adv748x-afe.c  | 6 +-----
> >  drivers/media/i2c/adv748x/adv748x-core.c | 4 ++++
> >  drivers/media/i2c/adv748x/adv748x.h      | 1 +
> >  3 files changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/adv748x/adv748x-afe.c b/drivers/media/i2c/adv748x/adv748x-afe.c
> > index dbbb1e4d63637a33..4052cf67bf16c7fb 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-afe.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-afe.c
> > @@ -154,7 +154,7 @@ static void adv748x_afe_set_video_standard(struct adv748x_state *state,
> >  		   (sdpstd & 0xf) << ADV748X_SDP_VID_SEL_SHIFT);
> >  }
> >  
> > -static int adv748x_afe_s_input(struct adv748x_afe *afe, unsigned int input)
> > +int adv748x_afe_s_input(struct adv748x_afe *afe, unsigned int input)
> >  {
> >  	struct adv748x_state *state = adv748x_afe_to_state(afe);
> >  
> > @@ -520,10 +520,6 @@ int adv748x_afe_init(struct adv748x_afe *afe)
> >  		}
> >  	}
> >  
> > -	adv748x_afe_s_input(afe, afe->input);
> > -
> > -	adv_dbg(state, "AFE Default input set to %d\n", afe->input);
> > -
> >  	/* Entity pads and sinks are 0-indexed to match the pads */
> >  	for (i = ADV748X_AFE_SINK_AIN0; i <= ADV748X_AFE_SINK_AIN7; i++)
> >  		afe->pads[i].flags = MEDIA_PAD_FL_SINK;
> > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> > index 00966fe104881a14..8676ad2428856dd3 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-core.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> > @@ -516,6 +516,10 @@ static int adv748x_reset(struct adv748x_state *state)
> >  	if (ret)
> >  		return ret;
> >  
> > +	adv748x_afe_s_input(&state->afe, state->afe.input);
> > +
> > +	adv_dbg(state, "AFE Default input set to %d\n", state->afe.input);
> > +
> >  	/* Reset TXA and TXB */
> >  	adv748x_tx_power(&state->txa, 1);
> >  	adv748x_tx_power(&state->txa, 0);
> > diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> > index 1061f425ece5989e..747947ea3e316451 100644
> > --- a/drivers/media/i2c/adv748x/adv748x.h
> > +++ b/drivers/media/i2c/adv748x/adv748x.h
> > @@ -435,6 +435,7 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on);
> >  
> >  int adv748x_afe_init(struct adv748x_afe *afe);
> >  void adv748x_afe_cleanup(struct adv748x_afe *afe);
> > +int adv748x_afe_s_input(struct adv748x_afe *afe, unsigned int input);
> >  
> >  int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx);
> >  void adv748x_csi2_cleanup(struct adv748x_csi2 *tx);
> > 
> 

-- 
Regards,
Niklas Söderlund

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH 0/3] adv748x: Add support for s2ram
  2020-11-25 13:09 ` Kieran Bingham
@ 2020-11-25 13:39   ` Niklas Söderlund
  2020-11-25 13:58     ` Geert Uytterhoeven
  0 siblings, 1 reply; 13+ messages in thread
From: Niklas Söderlund @ 2020-11-25 13:39 UTC (permalink / raw)
  To: Kieran Bingham; +Cc: linux-media, linux-renesas-soc

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(-)
> > 
> 

-- 
Regards,
Niklas Söderlund

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH 0/3] adv748x: Add support for s2ram
  2020-11-25 13:39   ` Niklas Söderlund
@ 2020-11-25 13:58     ` Geert Uytterhoeven
  0 siblings, 0 replies; 13+ messages in thread
From: Geert Uytterhoeven @ 2020-11-25 13:58 UTC (permalink / raw)
  To: Niklas Söderlund, Kieran Bingham
  Cc: Linux Media Mailing List, Linux-Renesas

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

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

^ permalink raw reply	[flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-11-25 13:58 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-22 16:36 [PATCH 0/3] adv748x: Add support for s2ram Niklas Söderlund
2020-11-22 16:36 ` [PATCH 1/3] adv748x: afe: Select input port when device is reset Niklas Söderlund
2020-11-23  8:00   ` Sergei Shtylyov
2020-11-25 12:10   ` Kieran Bingham
2020-11-25 13:16     ` Niklas Söderlund
2020-11-22 16:36 ` [PATCH 2/3] adv748x: csi2: Set virtual channel " Niklas Söderlund
2020-11-23  8:06   ` Sergei Shtylyov
2020-11-22 16:36 ` [PATCH 3/3] adv748x: Configure device when resuming from sleep Niklas Söderlund
2020-11-23  8:09   ` Sergei Shtylyov
2020-11-23  8:05 ` [PATCH 0/3] adv748x: Add support for s2ram Sergei Shtylyov
2020-11-25 13:09 ` Kieran Bingham
2020-11-25 13:39   ` Niklas Söderlund
2020-11-25 13:58     ` Geert Uytterhoeven

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.