Re: [linux-sunxi] [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.

From: "Ondřej Jirman" <megous@megous.com>
To: yong.deng@magewell.com, maxime.ripard@free-electrons.com
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>, Chen-Yu Tsai <wens@csie.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"David S. Miller" <davem@davemloft.net>,
	Hans Verkuil <hverkuil@xs4all.nl>, Arnd Bergmann <arnd@arndb.de>,
	Hugues Fruchet <hugues.fruchet@st.com>,
	Yannick Fertre <yannick.fertre@st.com>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Benoit Parrot <bparrot@ti.com>,
	Benjamin Gaignard <benjamin.gaignard@linaro.org>,
	Jean-Christophe Trotin <jean-christophe.trotin@st.com>,
	Ramesh Shanmugasundaram <ramesh.shanmugasundaram@bp.renesas.com>,
	Minghsiu Tsai <minghsiu.tsai@mediatek.com>,
	Krzysztof Kozlowski <krzk@kernel.org>,
	Robert Jarzmik <robert.jarzmik@free.fr>,
	linux-media@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-sunxi@googlegroups.com
Subject: Re: [linux-sunxi] [PATCH v2 1/3] media: V3s: Add support for Allwinner CSI.
Date: Thu, 21 Sep 2017 15:45:00 +0200	[thread overview]
Message-ID: <1506001500.29217.6.camel@megous.com> (raw)
In-Reply-To: <1501131697-1359-2-git-send-email-yong.deng@magewell.com>

[-- Attachment #1: Type: text/plain, Size: 4389 bytes --]

Hello Yong,

I noticed one issue in the register macros. See below.

Yong Deng píše v Čt 27. 07. 2017 v 13:01 +0800:
> Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
> and CSI1 is used for parallel interface. This is not documented in
> datasheet but by testing and guess.
> 
> This patch implement a v4l2 framework driver for it.
> 
> Currently, the driver only support the parallel interface. MIPI-CSI2,
> ISP's support are not included in this patch.
> 
> Signed-off-by: Yong Deng <yong.deng@magewell.com>
> ---
> 

[snip]

> +
> +#define CSI_CH_INT_EN_REG		0x70
> +#define CSI_CH_INT_EN_VS_INT_EN			BIT(7)
> +#define CSI_CH_INT_EN_HB_OF_INT_EN		BIT(6)
> +#define CSI_CH_INT_EN_MUL_ERR_INT_EN		BIT(5)
> +#define CSI_CH_INT_EN_FIFO2_OF_INT_EN		BIT(4)
> +#define CSI_CH_INT_EN_FIFO1_OF_INT_EN		BIT(3)
> +#define CSI_CH_INT_EN_FIFO0_OF_INT_EN		BIT(2)
> +#define CSI_CH_INT_EN_FD_INT_EN			BIT(1)
> +#define CSI_CH_INT_EN_CD_INT_EN			BIT(0)
> +
> +#define CSI_CH_INT_STA_REG		0x74
> +#define CSI_CH_INT_STA_VS_PD			BIT(7)
> +#define CSI_CH_INT_STA_HB_OF_PD			BIT(6)
> +#define CSI_CH_INT_STA_MUL_ERR_PD		BIT(5)
> +#define CSI_CH_INT_STA_FIFO2_OF_PD		BIT(4)
> +#define CSI_CH_INT_STA_FIFO1_OF_PD		BIT(3)
> +#define CSI_CH_INT_STA_FIFO0_OF_PD		BIT(2)
> +#define CSI_CH_INT_STA_FD_PD			BIT(1)
> +#define CSI_CH_INT_STA_CD_PD			BIT(0)
> +
> +#define CSI_CH_FLD1_VSIZE_REG		0x74

This register should be 0x78 according to the V3s manual. Though it's
not used in your driver yet, so it is not yet causing any issues.

> +#define CSI_CH_HSIZE_REG		0x80
> +#define CSI_CH_HSIZE_HOR_LEN_MASK		GENMASK(28, 16)
> +#define CSI_CH_HSIZE_HOR_LEN(len)		((len << 16) & CSI_CH_HSIZE_HOR_LEN_MASK)
> +#define CSI_CH_HSIZE_HOR_START_MASK		GENMASK(12, 0)
> +#define CSI_CH_HSIZE_HOR_START(start)		((start << 0) & CSI_CH_HSIZE_HOR_START_MASK)
> +
> +#define CSI_CH_VSIZE_REG		0x84
> +#define CSI_CH_VSIZE_VER_LEN_MASK		GENMASK(28, 16)
> +#define CSI_CH_VSIZE_VER_LEN(len)		((len << 16) & CSI_CH_VSIZE_VER_LEN_MASK)
> +#define CSI_CH_VSIZE_VER_START_MASK		GENMASK(12, 0)
> +#define CSI_CH_VSIZE_VER_START(start)		((start << 0) & CSI_CH_VSIZE_VER_START_MASK)
> +
> +#define CSI_CH_BUF_LEN_REG		0x88
> +#define CSI_CH_BUF_LEN_BUF_LEN_C_MASK		GENMASK(29, 16)
> +#define CSI_CH_BUF_LEN_BUF_LEN_C(len)		((len << 16) & CSI_CH_BUF_LEN_BUF_LEN_C_MASK)
> +#define CSI_CH_BUF_LEN_BUF_LEN_Y_MASK		GENMASK(13, 0)
> +#define CSI_CH_BUF_LEN_BUF_LEN_Y(len)		((len << 0) & CSI_CH_BUF_LEN_BUF_LEN_Y_MASK)
> +
> +#define CSI_CH_FLIP_SIZE_REG		0x8c
> +#define CSI_CH_FLIP_SIZE_VER_LEN_MASK		GENMASK(28, 16)
> +#define CSI_CH_FLIP_SIZE_VER_LEN(len)		((len << 16) & CSI_CH_FLIP_SIZE_VER_LEN_MASK)
> +#define CSI_CH_FLIP_SIZE_VALID_LEN_MASK		GENMASK(12, 0)
> +#define CSI_CH_FLIP_SIZE_VALID_LEN(len)		((len << 0) & CSI_CH_FLIP_SIZE_VALID_LEN_MASK)
> +
> +#define CSI_CH_FRM_CLK_CNT_REG		0x90
> +#define CSI_CH_ACC_ITNL_CLK_CNT_REG	0x94
> +#define CSI_CH_FIFO_STAT_REG		0x98
> +#define CSI_CH_PCLK_STAT_REG		0x9c
> +
> +/*
> + * csi input data format
> + */
> +enum csi_input_fmt
> +{
> +	CSI_INPUT_FORMAT_RAW		= 0,
> +	CSI_INPUT_FORMAT_YUV422		= 3,
> +	CSI_INPUT_FORMAT_YUV420		= 4,
> +};
> +
> +/*
> + * csi output data format
> + */
> +enum csi_output_fmt
> +{
> +	/* only when input format is RAW */
> +	CSI_FIELD_RAW_8			= 0,
> +	CSI_FIELD_RAW_10		= 1,
> +	CSI_FIELD_RAW_12		= 2,
> +	CSI_FIELD_RGB565		= 4,
> +	CSI_FIELD_RGB888		= 5,
> +	CSI_FIELD_PRGB888		= 6,
> +	CSI_FRAME_RAW_8			= 8,
> +	CSI_FRAME_RAW_10		= 9,
> +	CSI_FRAME_RAW_12		= 10,
> +	CSI_FRAME_RGB565		= 12,
> +	CSI_FRAME_RGB888		= 13,
> +	CSI_FRAME_PRGB888		= 14,
> +
> +	/* only when input format is YUV422/YUV420 */

Other limitation is that when input is YUV420 output can only be YUV420
and not YUV422.

> +	CSI_FIELD_PLANAR_YUV422		= 0,
> +	CSI_FIELD_PLANAR_YUV420		= 1,
> +	CSI_FRAME_PLANAR_YUV420		= 2,
> +	CSI_FRAME_PLANAR_YUV422		= 3,
> +	CSI_FIELD_UV_CB_YUV422		= 4,
> +	CSI_FIELD_UV_CB_YUV420		= 5,
> +	CSI_FRAME_UV_CB_YUV420		= 6,
> +	CSI_FRAME_UV_CB_YUV422		= 7,
> +	CSI_FIELD_MB_YUV422		= 8,
> +	CSI_FIELD_MB_YUV420		= 9,
> +	CSI_FRAME_MB_YUV420		= 10,
> +	CSI_FRAME_MB_YUV422		= 11,
> +	CSI_FIELD_UV_CB_YUV422_10	= 12,
> +	CSI_FIELD_UV_CB_YUV420_10	= 13,
> +};
> +

thank you and regards,
  Ondrej

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]