Re: [PATCH 2/2] media: rcar-vin: Mask VNCSI_IFMD register

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

Hi Jacopo,

Thanks for your patch.

On 2020-11-12 17:08:51 +0100, Jacopo Mondi wrote:
> The VNCSI_IFMD register controls the data expansion mode and the
> channel routing between the CSI-2 recivers and VIN instances.
> 
> According to the chip manual revision 2.20 not all fields are available
> for all the SoCs:
> - V3M, V3H and E3 do not support the DES1 field has they do not feature
>   a CSI20 receiver.
> - D3 only supports parallel input, and the whole register shall always
>   be written as 0.
> 
> Add a bit mask to the per-SoC rcar_info structure and clear the register
> value before writing it to the hardware.
> 
> This patch upports the BSP change commit f54697394457
> ("media: rcar-vin: Fix VnCSI_IFMD register access for r8a77990")

I like that this issue is finally being addressed it's been in my list 
for a while. Unfortunately I'm not super keen on how it's solved here. I 
hoped the needed information could be extracted from the strcut 
rvin_info routes member inside rvin_set_channel_routing().

For D3 the length of the routes will be 0 and the driver should not 
attempt to write to the register at all. In this patch the register is 
still written the value of VNCSI_IFMD_DES1 | VNCSI_IFMD_DES0.

For the rest I was hoping the routes array would be examined and tested 
for if it contains RVIN_CSI40, RVIN_CSI41, RVIN_CSI20 and RVIN_CSI21 
routes and set DES0 and DES1 bits accordingly.

As rvin_set_channel_routing() is never called in a hot path the cost of 
iterating over the small array I think is worth it to guarantee the 
routes always are the authoritative source, just as it is for the media 
graph links.

> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 5 +++++
>  drivers/media/platform/rcar-vin/rcar-dma.c  | 7 +++----
>  drivers/media/platform/rcar-vin/rcar-vin.h  | 6 ++++++
>  3 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 0b67d58dd727..57ac43a93f5e 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -1241,6 +1241,7 @@ static const struct rvin_info rcar_info_r8a77970 = {
>  	.max_width = 4096,
>  	.max_height = 4096,
>  	.routes = rcar_info_r8a77970_routes,
> +	.ifmd_mask = VNCSI_IFMD_DES1,
>  };
>  
>  static const struct rvin_group_route rcar_info_r8a77980_routes[] = {
> @@ -1270,6 +1271,7 @@ static const struct rvin_info rcar_info_r8a77980 = {
>  	.max_width = 4096,
>  	.max_height = 4096,
>  	.routes = rcar_info_r8a77980_routes,
> +	.ifmd_mask = VNCSI_IFMD_DES1,
>  };
>  
>  static const struct rvin_group_route rcar_info_r8a77990_routes[] = {
> @@ -1287,6 +1289,7 @@ static const struct rvin_info rcar_info_r8a77990 = {
>  	.max_width = 4096,
>  	.max_height = 4096,
>  	.routes = rcar_info_r8a77990_routes,
> +	.ifmd_mask = VNCSI_IFMD_DES1,
>  };
>  
>  static const struct rvin_group_route rcar_info_r8a77995_routes[] = {
> @@ -1306,6 +1309,8 @@ static const struct rvin_info rcar_info_r8a77995 = {
>  	.max_height = 4096,
>  	.routes = rcar_info_r8a77995_routes,
>  	.scalers = rcar_info_r8a77995_scalers,
> +	/* VNCSI_IFMD_REG not available on R-Car D3. */
> +	.ifmd_mask = 0xff,
>  };
>  
>  static const struct of_device_id rvin_of_id_table[] = {
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index 378514a75bc2..c0e09c5d9c79 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -134,8 +134,6 @@
>  #define VNDMR2_VLV(n)		((n & 0xf) << 12)
>  
>  /* Video n CSI2 Interface Mode Register (Gen3) */
> -#define VNCSI_IFMD_DES1		(1 << 26)
> -#define VNCSI_IFMD_DES0		(1 << 25)
>  #define VNCSI_IFMD_CSI_CHSEL(n) (((n) & 0xf) << 0)
>  
>  /* Video n scaling control register (Gen3) */
> @@ -1583,8 +1581,9 @@ int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel)
>  	vnmc = rvin_read(vin, VNMC_REG);
>  	rvin_write(vin, vnmc & ~VNMC_VUP, VNMC_REG);
>  
> -	ifmd = VNCSI_IFMD_DES1 | VNCSI_IFMD_DES0 | VNCSI_IFMD_CSI_CHSEL(chsel);
> -
> +	/* Write only available fields to IFMD_REG. */
> +	ifmd = (VNCSI_IFMD_DES0 | VNCSI_IFMD_DES1 | VNCSI_IFMD_CSI_CHSEL(chsel))
> +	     & !vin->info->ifmd_mask;
>  	rvin_write(vin, ifmd, VNCSI_IFMD_REG);
>  
>  	vin_dbg(vin, "Set IFMD 0x%x\n", ifmd);
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index be10e06b0880..2cf8952faab1 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -31,6 +31,10 @@
>  /* Max number on VIN instances that can be in a system */
>  #define RCAR_VIN_NUM 8
>  
> +/* CSI_IFMD register bit fields to mask per-SoC. */
> +#define VNCSI_IFMD_DES1		BIT(26)
> +#define VNCSI_IFMD_DES0		BIT(25)
> +
>  struct rvin_group;
>  
>  enum model_id {
> @@ -174,6 +178,7 @@ struct rvin_group_scaler {
>   * @routes:		list of possible routes from the CSI-2 recivers to
>   *			all VINs. The list mush be NULL terminated.
>   * @scalers:		List of available scalers, must be NULL terminated.
> + * @ifmd_mask:		Mask of unavailable bit fields of the CSI_IFMD register
>   */
>  struct rvin_info {
>  	enum model_id model;
> @@ -184,6 +189,7 @@ struct rvin_info {
>  	unsigned int max_height;
>  	const struct rvin_group_route *routes;
>  	const struct rvin_group_scaler *scalers;
> +	u32 ifmd_mask;
>  };
>  
>  /**
> -- 
> 2.29.1
> 

-- 
Regards,
Niklas Söderlund