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

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

Hi Jacopo,

Sorry for dragging up an old thread...

On 15/04/2019 08:00, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Fri, Apr 12, 2019 at 04:57:45PM +0100, Kieran Bingham wrote:
>> Hi Jacopo,
>>
>> On 12/04/2019 15:45, Jacopo Mondi wrote:
>>> Hi Kieran,
>>>
>>> On Fri, Apr 12, 2019 at 03:15:46PM +0100, Kieran Bingham wrote:
>>>> 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?
>>>>
>>>
>>> As reported in the cover letter, please see:
>>> "[PATCH] media: adv748x: Don't disable CSI-2 on link_setup"
>>
>> Ah I see, I had missed that.
>>
>> The patch itself should still describe the problem if it's fixing
>> something. The cover letter will not be available in the git-log.
>>
> 
> You're right, I'll expand the commit message.
> 
>> Ok, so this patch supersedes "[PATCH] media: adv748x: Don't disable
>> CSI-2 on link_setup" ?
>>
> 
> Yes
> 
>>>> 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?
>>>
>>> The issue I was trying to solve, even with the first patch is that
>>> going through a link disable (eg. at media graph reset time) wrote to
>>> the csi4_in_sel register a 0 value, when both TXA and TXB routing
>>> where disabled and this causes issues on some HDMI transmiters, for
>>> reason Ian and Hans tried to expand but about which I'm not yet sure
>>> about.
>>
>> Ok, I found that patch and read their comments. So disabling the CSI2
>> might trigger a hot-plug event to the transmitter which makes them think
>> they have been (physically) disconnected in some way, or triggers them
>> to re-read the EDID which will fail. But we don't really know what the
>> fault is yet.
>>
> 
> Correct. Please note that I've seen this happening with one HDMI
> transmitter (a chromecast device), while if HDMI source is a laptop
> everything's fine...
> 
>>
>>> The idea here is to cache the routing settings at link_setup time
>>> (upon activation or deactivation of a link) and apply them to hardware
>>> at tx power up time, which happens at s_stream(1).
>>>
>>> In this way, if streaming is started, we know at least one link is
>>> enabled and we can write to csi4_in_sel.
>>
>> Overall this seems reasonable, only making a change to io10 when either
>> of the stream states are changed.
>>
> 
> Thanks, I'll re-send (maybe even out of this series if it gets stale).
> Could I have your tag on the next iteration?

I found this patch on a branch while rebasing old patches to latest.

It still applies, and seems to make sense - and it looks like I agreed
with you back when you posted it.

It seems it only needed me to say:

"Yes,

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
"

To unblock this particular patch (separate from the rest of the series
it currently resides in.

So there you have it ;-)

With the only action remaining being to try to briefly describe the
problem it fixes in the commit message.


Thanks,
--
Kieran



> 
> Thanks
>   j
> 
>>
>>>>> 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)
>>>>
>>>
>>> The comment is wrong, thanks for noticing.
>>>
>>>> Can the logic that determines state->csi4_in_sel value simply be moved
>>>> here (or to an independent adv748x_configure_routing() function)?
>>>>
>>>
>>> It has to be changed at link_setup time in rensponse to a media link
>>> activation or deactivation.
>>>
>>>> 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?)
>>>>
>>>
>>> Well...
>>>
>>>  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.
>>>
>>> Doesn't it say that? Or what confused you is that s_stream->s_power(1)
>>> and I should mention streaming instead of power up?
>>
>> I think of "Power up" meaning at device probe time (or possibly some
>> runtime-pm event time). But yes, I think the distinction that it now
>> happens at stream_on/stream_off is important.
>>
>>
>>
>>>
>>>>
>>>>
>>>>
>>>>> +	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.
>>>>
>>>
>>> Yes
>>
>> But the actual hardware shouldn't be updated if the stream is running
>> right? (I wonder if the media-controller would prevent that anyway).
>>
>>
>>
>>>
>>>>>  		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?
>>>
>>> I should better check when reset happens, and if it happens only when
>>> links have been disabled or not.
>>> If links are disabled, the variable gets zeroed by the link_setup
>>> callback. If reset happens with links active, we should not zero it
>>> otherwise we lose the configuration
>>
>>
>> Ah yes, I missed that the local variable is initialised to zero, and
>> this state variable is set to the result at the end of the call...
>>
>> It does mean that the function ordering will be important here.
>>
>> It becomes irrelevant if these conditionals move to the same point
>> anyway though.
>>
>>
>>
>>>
>>>>
>>>>
>>>>> +	/* 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...
>>>>
>>>
>>> Blame the hw designers :)
>>
>> Always. ... of course they keep blaming us back :D
>>
>>>
>>>> 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.
>>>>
>>>
>>> You are righ. csi4_in_sel should only control the 3 bits used for
>>> routing, while enabling and disabling of the TXes is controlled by
>>> other bits of the io_10 register.
>>> I'll look into changing the name back
>>>
>>>> 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?
>>>
>>> I'll consider that, thanks
>>
>> I think it's only the routing choice that needs to be stored in the
>> state. That would minimise being stored 'globally', and the values could
>> be determined at the time of programming the register.
>>
>>>
>>> Thanks
>>>   j
>>>
>>>>
>>>>
>>>>> +
>>>>>  	/* 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

-- 
Regards
--
Kieran