Re: [PATCH v9 15/28] rcar-vin: enable Gen3 hardware configuration

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

Hi Niklas,

Thank you for the patch.

On Friday, 8 December 2017 03:08:29 EET Niklas Söderlund wrote:
> Add the register needed to work with Gen3 hardware. This patch adds
> the logic for how to work with the Gen3 hardware. More work is required
> to enable the subdevice structure needed to configure capturing.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 94 ++++++++++++++++++---------
>  drivers/media/platform/rcar-vin/rcar-vin.h |  1 +
>  2 files changed, 64 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> b/drivers/media/platform/rcar-vin/rcar-dma.c index
> d7660f485a2df9e4..ace95d5b543a17e3 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -33,21 +33,23 @@
>  #define VNELPRC_REG	0x10	/* Video n End Line Pre-Clip Register */
>  #define VNSPPRC_REG	0x14	/* Video n Start Pixel Pre-Clip Register */
>  #define VNEPPRC_REG	0x18	/* Video n End Pixel Pre-Clip Register */
> -#define VNSLPOC_REG	0x1C	/* Video n Start Line Post-Clip Register */
> -#define VNELPOC_REG	0x20	/* Video n End Line Post-Clip Register */
> -#define VNSPPOC_REG	0x24	/* Video n Start Pixel Post-Clip Register */
> -#define VNEPPOC_REG	0x28	/* Video n End Pixel Post-Clip Register */
>  #define VNIS_REG	0x2C	/* Video n Image Stride Register */
>  #define VNMB_REG(m)	(0x30 + ((m) << 2)) /* Video n Memory Base m Register
> */ #define VNIE_REG	0x40	/* Video n Interrupt Enable Register */
>  #define VNINTS_REG	0x44	/* Video n Interrupt Status Register */
>  #define VNSI_REG	0x48	/* Video n Scanline Interrupt Register */
>  #define VNMTC_REG	0x4C	/* Video n Memory Transfer Control Register */
> -#define VNYS_REG	0x50	/* Video n Y Scale Register */
> -#define VNXS_REG	0x54	/* Video n X Scale Register */
>  #define VNDMR_REG	0x58	/* Video n Data Mode Register */
>  #define VNDMR2_REG	0x5C	/* Video n Data Mode Register 2 */
>  #define VNUVAOF_REG	0x60	/* Video n UV Address Offset Register */
> +
> +/* Register offsets specific for Gen2 */
> +#define VNSLPOC_REG	0x1C	/* Video n Start Line Post-Clip Register */
> +#define VNELPOC_REG	0x20	/* Video n End Line Post-Clip Register */
> +#define VNSPPOC_REG	0x24	/* Video n Start Pixel Post-Clip Register */
> +#define VNEPPOC_REG	0x28	/* Video n End Pixel Post-Clip Register */
> +#define VNYS_REG	0x50	/* Video n Y Scale Register */
> +#define VNXS_REG	0x54	/* Video n X Scale Register */
>  #define VNC1A_REG	0x80	/* Video n Coefficient Set C1A Register */
>  #define VNC1B_REG	0x84	/* Video n Coefficient Set C1B Register */
>  #define VNC1C_REG	0x88	/* Video n Coefficient Set C1C Register */
> @@ -73,9 +75,13 @@
>  #define VNC8B_REG	0xF4	/* Video n Coefficient Set C8B Register */
>  #define VNC8C_REG	0xF8	/* Video n Coefficient Set C8C Register */
> 
> +/* Register offsets specific for Gen3 */
> +#define VNCSI_IFMD_REG		0x20 /* Video n CSI2 Interface Mode Register */
> 
>  /* Register bit fields for R-Car VIN */
>  /* Video n Main Control Register bits */
> +#define VNMC_DPINE		(1 << 27) /* Gen3 specific */
> +#define VNMC_SCLE		(1 << 26) /* Gen3 specific */
>  #define VNMC_FOC		(1 << 21)
>  #define VNMC_YCAL		(1 << 19)
>  #define VNMC_INF_YUV8_BT656	(0 << 16)
> @@ -119,6 +125,13 @@
>  #define VNDMR2_FTEV		(1 << 17)
>  #define VNDMR2_VLV(n)		((n & 0xf) << 12)
> 
> +/* Video n CSI2 Interface Mode Register (Gen3) */
> +#define VNCSI_IFMD_DES2		(1 << 27)
> +#define VNCSI_IFMD_DES1		(1 << 26)
> +#define VNCSI_IFMD_DES0		(1 << 25)
> +#define VNCSI_IFMD_CSI_CHSEL(n) ((n & 0xf) << 0)

*Always* enclose macro arguments in parentheses otherwise they are subject to 
side effects.

#define VNCSI_IFMD_CSI_CHSEL(n) (((n) & 0xf) << 0)

> +#define VNCSI_IFMD_CSI_CHSEL_MASK 0xf
> +
>  struct rvin_buffer {
>  	struct vb2_v4l2_buffer vb;
>  	struct list_head list;
> @@ -514,28 +527,10 @@ static void rvin_set_coeff(struct rvin_dev *vin,
> unsigned short xs) rvin_write(vin, p_set->coeff_set[23], VNC8C_REG);
>  }
> 
> -static void rvin_crop_scale_comp(struct rvin_dev *vin)
> +static void rvin_crop_scale_comp_gen2(struct rvin_dev *vin)
>  {
>  	u32 xs, ys;
> 
> -	/* Set Start/End Pixel/Line Pre-Clip */
> -	rvin_write(vin, vin->crop.left, VNSPPRC_REG);
> -	rvin_write(vin, vin->crop.left + vin->crop.width - 1, VNEPPRC_REG);
> -	switch (vin->format.field) {
> -	case V4L2_FIELD_INTERLACED:
> -	case V4L2_FIELD_INTERLACED_TB:
> -	case V4L2_FIELD_INTERLACED_BT:
> -		rvin_write(vin, vin->crop.top / 2, VNSLPRC_REG);
> -		rvin_write(vin, (vin->crop.top + vin->crop.height) / 2 - 1,
> -			   VNELPRC_REG);
> -		break;
> -	default:
> -		rvin_write(vin, vin->crop.top, VNSLPRC_REG);
> -		rvin_write(vin, vin->crop.top + vin->crop.height - 1,
> -			   VNELPRC_REG);
> -		break;
> -	}
> -
>  	/* Set scaling coefficient */
>  	ys = 0;
>  	if (vin->crop.height != vin->compose.height)
> @@ -573,11 +568,6 @@ static void rvin_crop_scale_comp(struct rvin_dev *vin)
>  		break;
>  	}
> 
> -	if (vin->format.pixelformat == V4L2_PIX_FMT_NV16)
> -		rvin_write(vin, ALIGN(vin->format.width, 0x20), VNIS_REG);
> -	else
> -		rvin_write(vin, ALIGN(vin->format.width, 0x10), VNIS_REG);
> -
>  	vin_dbg(vin,
>  		"Pre-Clip: %ux%u@%u:%u YS: %d XS: %d Post-Clip: %ux%u@%u:%u\n",
>  		vin->crop.width, vin->crop.height, vin->crop.left,

Would it make sense to keep the debug message in the common function, or ill 
cropping and composing be handled through subdevs for Gen3 ?

With these two issues addressed,

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

> @@ -585,6 +575,37 @@ static void rvin_crop_scale_comp(struct rvin_dev *vin)
>  		0, 0);
>  }
> 
> +static void rvin_crop_scale_comp(struct rvin_dev *vin)
> +{
> +	/* Set Start/End Pixel/Line Pre-Clip */
> +	rvin_write(vin, vin->crop.left, VNSPPRC_REG);
> +	rvin_write(vin, vin->crop.left + vin->crop.width - 1, VNEPPRC_REG);
> +
> +	switch (vin->format.field) {
> +	case V4L2_FIELD_INTERLACED:
> +	case V4L2_FIELD_INTERLACED_TB:
> +	case V4L2_FIELD_INTERLACED_BT:
> +		rvin_write(vin, vin->crop.top / 2, VNSLPRC_REG);
> +		rvin_write(vin, (vin->crop.top + vin->crop.height) / 2 - 1,
> +			   VNELPRC_REG);
> +		break;
> +	default:
> +		rvin_write(vin, vin->crop.top, VNSLPRC_REG);
> +		rvin_write(vin, vin->crop.top + vin->crop.height - 1,
> +			   VNELPRC_REG);
> +		break;
> +	}
> +
> +	/* TODO: Add support for the UDS scaler. */
> +	if (vin->info->chip != RCAR_GEN3)
> +		rvin_crop_scale_comp_gen2(vin);
> +
> +	if (vin->format.pixelformat == V4L2_PIX_FMT_NV16)
> +		rvin_write(vin, ALIGN(vin->format.width, 0x20), VNIS_REG);
> +	else
> +		rvin_write(vin, ALIGN(vin->format.width, 0x10), VNIS_REG);
> +}
> +
>  /* ------------------------------------------------------------------------
>   * Hardware setup
>   */
> @@ -659,7 +680,10 @@ static int rvin_setup(struct rvin_dev *vin)
>  	}
> 
>  	/* Enable VSYNC Field Toogle mode after one VSYNC input */
> -	dmr2 = VNDMR2_FTEV | VNDMR2_VLV(1);
> +	if (vin->info->chip == RCAR_GEN3)
> +		dmr2 = VNDMR2_FTEV;
> +	else
> +		dmr2 = VNDMR2_FTEV | VNDMR2_VLV(1);
> 
>  	/* Hsync Signal Polarity Select */
>  	if (!(vin->mbus_cfg.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
> @@ -711,6 +735,14 @@ static int rvin_setup(struct rvin_dev *vin)
>  	if (input_is_yuv == output_is_yuv)
>  		vnmc |= VNMC_BPS;
> 
> +	if (vin->info->chip == RCAR_GEN3) {
> +		/* Select between CSI-2 and Digital input */
> +		if (vin->mbus_cfg.type == V4L2_MBUS_CSI2)
> +			vnmc &= ~VNMC_DPINE;
> +		else
> +			vnmc |= VNMC_DPINE;
> +	}
> +
>  	/* Progressive or interlaced mode */
>  	interrupts = progressive ? VNIE_FIE : VNIE_EFE;
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h
> b/drivers/media/platform/rcar-vin/rcar-vin.h index
> 118f45b656920d39..a440effe4b86af31 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -33,6 +33,7 @@ enum chip_id {
>  	RCAR_H1,
>  	RCAR_M1,
>  	RCAR_GEN2,
> +	RCAR_GEN3,
>  };
> 
>  /**

-- 
Regards,

Laurent Pinchart