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

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

Hi Niklas,

Thank you for the patch.

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.

>  	/* 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.

>  	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