Re: [RFC 2/5] media: adv748x: Post-pone IO10 write to power up

[Kieran Bingham <kieran.bingham@ideasonboard.com>]

Hi Jacopo,

On 16/03/2019 15:47, Jacopo Mondi wrote:
> Post-pone the write of the ADV748X_IO_10 register that controls the active
> routing between the CP and AFE cores to the 4-lanes CSI-2 TXA at TX
> power-up time.

"by caching its configuration in the driver state."

> 
> While at there, use the 'csi4_in_sel' name, which matches the register

'While at it, use the...'

Except I'm not sure csi4_in_sel is the right name for the cached values
as below...


> field description in the manual, in place of 'io_10' which is the full
> register name.
> 

This has a fixes tag, but doesn't state what the actual problem is?

Can I assume that the problem is that the configuration here is being
written to the hardware before it is powered up or such?

Or perhaps reading through the patch again, is it that the call to
link_setup can affect running streams?



> Fixes: 9423ca350df7 ("media: adv748x: Implement TX link_setup callback")
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/adv748x/adv748x-core.c | 53 ++++++++++++++----------
>  drivers/media/i2c/adv748x/adv748x.h      |  2 +
>  2 files changed, 33 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c
> index 0e5a75eb6d75..02135741b1a6 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -305,23 +305,35 @@ static int adv748x_power_down_tx(struct adv748x_csi2 *tx)
> 
>  int adv748x_tx_power(struct adv748x_csi2 *tx, bool on)
>  {
> -	int val;
> +	u8 io10_mask = ADV748X_IO_10_CSI1_EN | ADV748X_IO_10_CSI4_EN |
> +		       ADV748X_IO_10_CSI4_IN_SEL_AFE;
> +	struct adv748x_state *state = tx->state;
> +	int ret;
> 
>  	if (!is_tx_enabled(tx))
>  		return 0;
> 
> -	val = tx_read(tx, ADV748X_CSI_FS_AS_LS);
> -	if (val < 0)
> -		return val;
> +	ret = tx_read(tx, ADV748X_CSI_FS_AS_LS);
> +	if (ret < 0)
> +		return ret;
> 
>  	/*
>  	 * This test against BIT(6) is not documented by the datasheet, but was
>  	 * specified in the downstream driver.
>  	 * Track with a WARN_ONCE to determine if it is ever set by HW.
>  	 */
> -	WARN_ONCE((on && val & ADV748X_CSI_FS_AS_LS_UNKNOWN),
> +	WARN_ONCE((on && ret & ADV748X_CSI_FS_AS_LS_UNKNOWN),
>  			"Enabling with unknown bit set");
> 
> +	/* Configure TXA routing. */

Should TXA routing be configured even on TXB power up? This function
handles both TX code paths. (Edit: possibly yes)

Can the logic that determines state->csi4_in_sel value simply be moved
here (or to an independent adv748x_configure_routing() function)?

I think this patch means that changes to routing will now only take
effect when starting or stopping a stream, is that right? (If so - could
that go into the commit message please?)




> +	if (on) {
> +		ret = io_clrset(state, ADV748X_IO_10, io10_mask,
> +				state->csi4_in_sel);
> +		if (ret)
> +			return ret;
> +	}
> +
> +
>  	return on ? adv748x_power_up_tx(tx) : adv748x_power_down_tx(tx);
>  }
> 
> @@ -337,10 +349,7 @@ static int adv748x_link_setup(struct media_entity *entity,
>  	struct adv748x_state *state = v4l2_get_subdevdata(sd);
>  	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
>  	bool enable = flags & MEDIA_LNK_FL_ENABLED;
> -	u8 io10_mask = ADV748X_IO_10_CSI1_EN |
> -		       ADV748X_IO_10_CSI4_EN |
> -		       ADV748X_IO_10_CSI4_IN_SEL_AFE;
> -	u8 io10 = 0;
> +	u8 csi4_in_sel = 0;
> 
>  	/* Refuse to enable multiple links to the same TX at the same time. */
>  	if (enable && tx->src)
> @@ -359,17 +368,19 @@ static int adv748x_link_setup(struct media_entity *entity,
> 
>  	if (state->afe.tx) {
>  		/* AFE Requires TXA enabled, even when output to TXB */
> -		io10 |= ADV748X_IO_10_CSI4_EN;
> +		csi4_in_sel |= ADV748X_IO_10_CSI4_EN;
>  		if (is_txa(tx))
> -			io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE;
> +			csi4_in_sel |= ADV748X_IO_10_CSI4_IN_SEL_AFE;

Hrm ... this is the one part that I think can't be determined without
caching some sort of value to state the routing.

>  		else
> -			io10 |= ADV748X_IO_10_CSI1_EN;
> +			csi4_in_sel |= ADV748X_IO_10_CSI1_EN;
>  	}
> 
>  	if (state->hdmi.tx)
> -		io10 |= ADV748X_IO_10_CSI4_EN;
> +		csi4_in_sel |= ADV748X_IO_10_CSI4_EN;
> 
> -	return io_clrset(state, ADV748X_IO_10, io10_mask, io10);
> +	state->csi4_in_sel = csi4_in_sel;
> +
> +	return 0;
>  }
> 
>  static const struct media_entity_operations adv748x_tx_media_ops = {
> @@ -485,7 +496,6 @@ static int adv748x_sw_reset(struct adv748x_state *state)
>  static int adv748x_reset(struct adv748x_state *state)
>  {
>  	int ret;
> -	u8 regval = 0;
> 
>  	ret = adv748x_sw_reset(state);
>  	if (ret < 0)
> @@ -504,6 +514,12 @@ static int adv748x_reset(struct adv748x_state *state)
>  	if (ret)
>  		return ret;
> 

Should adv748x_reset() reset the state->csi4_in_sel cached value to 0
before setting it?


> +	/* Conditionally enable TXa and TXb. */
> +	if (is_tx_enabled(&state->txa))
> +		state->csi4_in_sel |= ADV748X_IO_10_CSI4_EN;
> +	if (is_tx_enabled(&state->txb))
> +		state->csi4_in_sel |= ADV748X_IO_10_CSI1_EN;

This makes it looks like the naming "csi4_in_sel" is less appropriate,
as it covers both CSI4 and CSI1...

Also, this is confusing two terms, between the 'enable' and the 'select'

The _EN bits looks like they control the activation of the CSI
transmitter, where as the 'select' bits control the routing.

As the is_tx_enabled($TX) state is constant, perhaps that bit could be
inferred later when the register is written, and doesn't need to be
cached here?


> +
>  	/* Reset TXA and TXB */
>  	adv748x_tx_power(&state->txa, 1);
>  	adv748x_tx_power(&state->txa, 0);
> @@ -513,13 +529,6 @@ static int adv748x_reset(struct adv748x_state *state)
>  	/* Disable chip powerdown & Enable HDMI Rx block */
>  	io_write(state, ADV748X_IO_PD, ADV748X_IO_PD_RX_EN);
> 
> -	/* Conditionally enable TXa and TXb. */
> -	if (is_tx_enabled(&state->txa))
> -		regval |= ADV748X_IO_10_CSI4_EN;
> -	if (is_tx_enabled(&state->txb))
> -		regval |= ADV748X_IO_10_CSI1_EN;
> -	io_write(state, ADV748X_IO_10, regval);
> -
>  	/* Use vid_std and v_freq as freerun resolution for CP */
>  	cp_clrset(state, ADV748X_CP_CLMP_POS, ADV748X_CP_CLMP_POS_DIS_AUTO,
>  					      ADV748X_CP_CLMP_POS_DIS_AUTO);
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index 4a5a6445604f..27c116d09284 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -196,6 +196,8 @@ struct adv748x_state {
>  	struct adv748x_afe afe;
>  	struct adv748x_csi2 txa;
>  	struct adv748x_csi2 txb;
> +
> +	unsigned int csi4_in_sel;
>  };
> 
>  #define adv748x_hdmi_to_state(h) container_of(h, struct adv748x_state, hdmi)
> --
> 2.21.0
> 

-- 
Regards
--
Kieran