Re: [PATCH] rcar-csi2: update stream start for V3M

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Niklas,

On Friday, 27 July 2018 14:51:40 EEST Niklas Söderlund wrote:
> On 2018-07-27 12:25:13 +0300, Laurent Pinchart wrote:
> > On Friday, 27 July 2018 01:36:57 EEST Niklas Söderlund wrote:
> >> Latest errata document updates the start procedure for V3M. This change
> >> in addition to adhering to the datasheet update fixes capture on early
> >> revisions of V3M.
> >> 
> >> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >> ---
> >> 
> >>  drivers/media/platform/rcar-vin/rcar-csi2.c | 20 ++++++++++++++------
> >>  1 file changed, 14 insertions(+), 6 deletions(-)
> >> 
> >> ---
> >> 
> >> Hi Hans, Mauro and Sakari
> >> 
> >> I know this is late for v4.19 but if possible can it be considered? It
> >> fixes a real issue on R-Car V3M boards. I'm sorry for the late
> >> submission, the errata document accesses unfortunate did not align with
> >> the release schedule.
> >> 
> >> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> >> b/drivers/media/platform/rcar-vin/rcar-csi2.c index
> >> daef72d410a3425d..dc5ae8025832ab6e 100644
> >> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> >> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> >> @@ -339,6 +339,7 @@ enum rcar_csi2_pads {
> >> 
> >>  struct rcar_csi2_info {
> >>  	int (*init_phtw)(struct rcar_csi2 *priv, unsigned int mbps);
> >> +	int (*confirm_start)(struct rcar_csi2 *priv);
> >>  	const struct rcsi2_mbps_reg *hsfreqrange;
> >>  	unsigned int csi0clkfreqrange;
> >>  	bool clear_ulps;
> >> @@ -545,6 +546,13 @@ static int rcsi2_start(struct rcar_csi2 *priv)
> >>  	if (ret)
> >>  		return ret;
> >> 
> >> +	/* Confirm start */
> >> +	if (priv->info->confirm_start) {
> >> +		ret = priv->info->confirm_start(priv);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +
> > 
> > While PHTW has to be written in the "Confirmation of PHY start" sequence,
> > the operation doesn't seem to be related to confirmation of PHY start, it
> > instead looks like a shuffle of the configuration sequence. I would thus
> > not name the operation .confirm_start() as that's not what it does.
> 
> I think the hook name being .confirm_start() is good as it is where in
> stream start procedure it is called. What the operation do in the V3M
> case is for me hidden as the datasheet only lists register writes and
> instructs you to check what I believe is the result of each "operation".

The PHTW register is used to communicate with the PHY through an internal bus 
(could it be I2C ? That would be similar to what a well known HDMI TX IP 
provider does). I believe that the read following the write is just a way to 
way for the internal write to complete. The whole sequence is thus in my 
opinion really a set of writes to PHY registers, and thus a configuration 
sequence. I don't have more insight on this particular IP core though, all 
this is based on a combination of knowledge of other multimedia IPs and some 
guesswork. It would be worth asking Renesas for clarification.

> For all I know it might be a configuration sequence or a method to
> confirm that the stream is started. Do you have anymore insight to what
> it does? All I know is prior to datasheet v1.0 it was not documented for
> V3M and streaming worked fine without it, and still do.
> 
> >>  	/* Clear Ultra Low Power interrupt. */
> >>  	if (priv->info->clear_ulps)
> >>  		rcsi2_write(priv, INTSTATE_REG,
> >> @@ -880,6 +888,11 @@ static int rcsi2_init_phtw_h3_v3h_m3n(struct
> >> rcar_csi2 *priv, unsigned int mbps)
> >>  }
> >> 
> >>  static int rcsi2_init_phtw_v3m_e3(struct rcar_csi2 *priv, unsigned int
> >> mbps)
> >> +{
> >> +	return rcsi2_phtw_write_mbps(priv, mbps, phtw_mbps_v3m_e3, 0x44);
> >> +}
> >> +
> >> +static int rcsi2_confirm_start_v3m_e3(struct rcar_csi2 *priv)
> >>  {
> >>  	static const struct phtw_value step1[] = {
> >>  		{ .data = 0xed, .code = 0x34 },
> >> @@ -890,12 +903,6 @@ static int rcsi2_init_phtw_v3m_e3(struct rcar_csi2
> >> *priv, unsigned int mbps) { /* sentinel */
> >>  		},
> >>  	};
> >> 
> >> -	int ret;
> >> -
> >> -	ret = rcsi2_phtw_write_mbps(priv, mbps, phtw_mbps_v3m_e3, 0x44);
> >> -	if (ret)
> >> -		return ret;
> >> -
> > 
> > There's something I don't get here. According to the errata, it's the
> > step1 array write sequence that need to be moved from "Start of PHY" to
> > "Confirmation of PHY start". This patch moves the PHTW frequency
> > configuration instead.
> 
> Is this not what this patch do? I agree the diff is hard to read. The
> result however is more clear.

Of course, my bad. I had missed the fact that the patch also turned 
rcsi2_init_phtw_v3m_e3() into rcsi2_confirm_start_v3m_e3(). Sorry for the 
noise.

Given that the operation name isn't a blocker as it can be renamed later if 
needed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>     static int rcsi2_init_phtw_v3m_e3(struct rcar_csi2 *priv, unsigned int
> mbps) {
> 	    return rcsi2_phtw_write_mbps(priv, mbps, phtw_mbps_v3m_e3, 0x44);
>     }
> 
>     static int rcsi2_confirm_start_v3m_e3(struct rcar_csi2 *priv)
>     {
> 	    static const struct phtw_value step1[] = {
> 		    { .data = 0xed, .code = 0x34 },
> 		    { .data = 0xed, .code = 0x44 },
> 		    { .data = 0xed, .code = 0x54 },
> 		    { .data = 0xed, .code = 0x84 },
> 		    { .data = 0xed, .code = 0x94 },
> 		    { /* sentinel */ },
> 	    };
> 
> 	    return rcsi2_phtw_write_array(priv, step1);
>     }
> 
>     ...
> 
>     static const struct rcar_csi2_info rcar_csi2_info_r8a77970 = {
> 	    .init_phtw = rcsi2_init_phtw_v3m_e3,
> 	    .confirm_start = rcsi2_confirm_start_v3m_e3,
>     };
> 
> >>  	return rcsi2_phtw_write_array(priv, step1);
> >>  }
> >> 
> >> @@ -949,6 +956,7 @@ static const struct rcar_csi2_info
> >> rcar_csi2_info_r8a77965 = {
> >> 
> >>  static const struct rcar_csi2_info rcar_csi2_info_r8a77970 = {
> >>  	.init_phtw = rcsi2_init_phtw_v3m_e3,
> >> +	.confirm_start = rcsi2_confirm_start_v3m_e3,
> >>  };
> >>  
> >>  static const struct of_device_id rcar_csi2_of_table[] = {

-- 
Regards,

Laurent Pinchart