diff mbox series

[04/14] media: sun4i-csi: Fix [HV]sync polarity handling

Message ID 20191215165924.28314-5-wens@kernel.org (mailing list archive)
State Mainlined
Headers show
Series media: sun4i-csi: A10/A20 CSI1 and R40 CSI0 support | expand

Commit Message

Chen-Yu Tsai Dec. 15, 2019, 4:59 p.m. UTC
From: Chen-Yu Tsai <wens@csie.org>

The Allwinner camera sensor interface has a different definition of
[HV]sync. While the timing diagram uses the names HSYNC and VSYNC,
the note following the diagram and register names use HREF and VREF.
Combined they imply the hardware uses either [HV]REF or inverted
[HV]SYNC. There are also registers to set horizontal skip lengths
in pixels and vertical skip lengths in lines, also known as back
porches.

Fix the polarity handling by using the opposite polarity flag for
the checks. Also rename `[hv]sync_pol` to `[hv]ref_pol` to better
match the hardware register description.

Fixes: 577bbf23b758 ("media: sunxi: Add A10 CSI driver")
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 .../media/platform/sunxi/sun4i-csi/sun4i_csi.h |  4 ++--
 .../media/platform/sunxi/sun4i-csi/sun4i_dma.c | 18 +++++++++++++-----
 2 files changed, 15 insertions(+), 7 deletions(-)

Comments

Maxime Ripard Dec. 16, 2019, 1:37 p.m. UTC | #1
On Mon, Dec 16, 2019 at 12:59:14AM +0800, Chen-Yu Tsai wrote:
> From: Chen-Yu Tsai <wens@csie.org>
>
> The Allwinner camera sensor interface has a different definition of
> [HV]sync. While the timing diagram uses the names HSYNC and VSYNC,
> the note following the diagram and register names use HREF and VREF.
> Combined they imply the hardware uses either [HV]REF or inverted
> [HV]SYNC. There are also registers to set horizontal skip lengths
> in pixels and vertical skip lengths in lines, also known as back
> porches.
>
> Fix the polarity handling by using the opposite polarity flag for
> the checks. Also rename `[hv]sync_pol` to `[hv]ref_pol` to better
> match the hardware register description.
>
> Fixes: 577bbf23b758 ("media: sunxi: Add A10 CSI driver")
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime
Oleg Verych Jan. 16, 2023, 10:03 a.m. UTC | #2
Hi, Chen-Yu Tsai

> -	hsync_pol = !!(bus->flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH);
> -	vsync_pol = !!(bus->flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH);
> +	/*
> +	 * This hardware uses [HV]REF instead of [HV]SYNC. Based on the
> +	 * provided timing diagrams in the manual, positive polarity
> +	 * equals active high [HV]REF.
> +	 *
> +	 * When the back porch is 0, [HV]REF is more or less equivalent
> +	 * to [HV]SYNC inverted.
> +	 */
> +	href_pol = !!(bus->flags & V4L2_MBUS_HSYNC_ACTIVE_LOW);
> +	vref_pol = !!(bus->flags & V4L2_MBUS_VSYNC_ACTIVE_LOW);

After this change has been made there is a need of explicit explanation
of what "Active high" / "Active low" in dts really mean.

Currently physical high/low voltage levels are like that:
(I'm not sure about vsync-active)

* hsync-active = <0>; /* HSYNC active 'low' => wire active is 'high' */
  CSI register setting: href_pol: 1,

That is confusing:

[PATCH v6 5/5] DO NOT MERGE: ARM: dts: bananapi: Add Camera support
https://lore.kernel.org/linux-arm-kernel/cf0e40b0bca9219d2bb023a5b7f23bad8baba1e5.1562847292.git-series.maxime.ripard@bootlin.com/#r

> +	port {
> +		csi_from_ov5640: endpoint {
> +                        remote-endpoint = <&ov5640_to_csi>;
> +                        bus-width = <8>;
> +                        hsync-active = <1>; /* Active high */

original CSI driver

> +                        vsync-active = <0>; /* Active low */
> +                        data-active = <1>;  /* Active high */
> +                        pclk-sample = <1>;  /* Rising */
> +                };
> +	};

[PATCH 13/14] [DO NOT MERGE] ARM: dts: sun7i: cubieboard2: Enable OV7670 camera on CSI1
https://lore.kernel.org/linux-arm-kernel/20191215165924.28314-14-wens@kernel.org/

> +	port {
> +		/* Parallel bus endpoint */
> +		csi_from_ov7670: endpoint {
> +			remote-endpoint = <&ov7670_to_csi>;
> +			bus-width = <8>;
> +			/* driver is broken */
> +			hsync-active = <0>; /* Active high */

this change patchset

> +			vsync-active = <1>; /* Active high */
> +			data-active = <1>;  /* Active high */
> +			pclk-sample = <1>;  /* Rising */
> +		};

> +			ov7670_to_csi: endpoint {
> +				remote-endpoint = <&csi_from_ov7670>;
> +				bus-width = <8>;
> +				hsync-active = <1>; /* Active high */

this patcheset

> +				vsync-active = <1>; /* Active high */
> +				data-active = <1>;  /* Active high */
> +				pclk-sample = <1>;  /* Rising */
> +			};
> +		};
Maxime Ripard Jan. 16, 2023, 10:16 a.m. UTC | #3
Hi,

On Mon, Jan 16, 2023 at 01:03:59PM +0300, Oleg Verych wrote:
> > -	hsync_pol = !!(bus->flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH);
> > -	vsync_pol = !!(bus->flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH);
> > +	/*
> > +	 * This hardware uses [HV]REF instead of [HV]SYNC. Based on the
> > +	 * provided timing diagrams in the manual, positive polarity
> > +	 * equals active high [HV]REF.
> > +	 *
> > +	 * When the back porch is 0, [HV]REF is more or less equivalent
> > +	 * to [HV]SYNC inverted.
> > +	 */
> > +	href_pol = !!(bus->flags & V4L2_MBUS_HSYNC_ACTIVE_LOW);
> > +	vref_pol = !!(bus->flags & V4L2_MBUS_VSYNC_ACTIVE_LOW);
> 
> After this change has been made there is a need of explicit explanation
> of what "Active high" / "Active low" in dts really mean.

Why?

Active high means that the signal is considered active when it is held
high. Active low means that the signal is considered active when it is
low.

> Currently physical high/low voltage levels are like that:
> (I'm not sure about vsync-active)
> 
> * hsync-active = <0>; /* HSYNC active 'low' => wire active is 'high' */

Yes

>   CSI register setting: href_pol: 1,

Not really, no. It's what this patch commit log is saying: HREF is
!HSYNC, so in order to get a hsync pulse active high, you need to set
href_pol to 0.

> That is confusing:
> 
> [PATCH v6 5/5] DO NOT MERGE: ARM: dts: bananapi: Add Camera support
> https://lore.kernel.org/linux-arm-kernel/cf0e40b0bca9219d2bb023a5b7f23bad8baba1e5.1562847292.git-series.maxime.ripard@bootlin.com/#r
> 
> > +	port {
> > +		csi_from_ov5640: endpoint {
> > +                        remote-endpoint = <&ov5640_to_csi>;
> > +                        bus-width = <8>;
> > +                        hsync-active = <1>; /* Active high */
> 
> original CSI driver
> 
> > +                        vsync-active = <0>; /* Active low */
> > +                        data-active = <1>;  /* Active high */
> > +                        pclk-sample = <1>;  /* Rising */
> > +                };
> > +	};
> 
> [PATCH 13/14] [DO NOT MERGE] ARM: dts: sun7i: cubieboard2: Enable OV7670 camera on CSI1
> https://lore.kernel.org/linux-arm-kernel/20191215165924.28314-14-wens@kernel.org/
> 
> > +	port {
> > +		/* Parallel bus endpoint */
> > +		csi_from_ov7670: endpoint {
> > +			remote-endpoint = <&ov7670_to_csi>;
> > +			bus-width = <8>;
> > +			/* driver is broken */
> > +			hsync-active = <0>; /* Active high */
> 
> this change patchset
> 
> > +			vsync-active = <1>; /* Active high */
> > +			data-active = <1>;  /* Active high */
> > +			pclk-sample = <1>;  /* Rising */
> > +		};
> 
> > +			ov7670_to_csi: endpoint {
> > +				remote-endpoint = <&csi_from_ov7670>;
> > +				bus-width = <8>;
> > +				hsync-active = <1>; /* Active high */
> 
> this patcheset
> 
> > +				vsync-active = <1>; /* Active high */
> > +				data-active = <1>;  /* Active high */
> > +				pclk-sample = <1>;  /* Rising */
> > +			};
> > +		};

I'm sorry, it's not clear to me what is confusing in those excerpts?

Maxime
Oleg Verych Jan. 16, 2023, 6:43 p.m. UTC | #4
Hi!

On 1/16/23, Maxime Ripard <maxime@cerno.tech> wrote:
> Hi,
>
> On Mon, Jan 16, 2023 at 01:03:59PM +0300, Oleg Verych wrote:
>> > -	hsync_pol = !!(bus->flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH);
>> > -	vsync_pol = !!(bus->flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH);
>> > +	/*
>> > +	 * This hardware uses [HV]REF instead of [HV]SYNC. Based on the
>> > +	 * provided timing diagrams in the manual, positive polarity
>> > +	 * equals active high [HV]REF.
>> > +	 *
>> > +	 * When the back porch is 0, [HV]REF is more or less equivalent
>> > +	 * to [HV]SYNC inverted.
>> > +	 */
>> > +	href_pol = !!(bus->flags & V4L2_MBUS_HSYNC_ACTIVE_LOW);
>> > +	vref_pol = !!(bus->flags & V4L2_MBUS_VSYNC_ACTIVE_LOW);
>>
>> After this change has been made there is a need of explicit explanation
>> of what "Active high" / "Active low" in dts really mean.
>
> Why?

It will be better understood by a person behind an oscilloscope who is
trying to figure out the logic behind dts, csi driver, csi controller,
wire voltage levels by just reading device tree definitions. Because
dts must be changed in order to connect source / sink devices.

>
> I'm sorry, it's not clear to me what is confusing in those excerpts?

I'm sorry too, maybe that is not clear. Confusion is here:

>> > +                        hsync-active = <1>; /* Active high */
>>
>> original CSI driver

i.e. <1> - active high

>> > +			hsync-active = <0>; /* Active high */
>>
>> this change patchset

i.e. <0> - active high

>> > +				hsync-active = <1>; /* Active high */
>>
>> this patcheset

i.e. <1> - active high


>> Currently physical high/low voltage levels are like that:
>> (I'm not sure about vsync-active)
>>
>> * hsync-active = <0>; /* HSYNC active 'low' => wire active is 'high' */
>
> Yes
>
>>   CSI register setting: href_pol: 1,
>
> Not really, no. It's what this patch commit log is saying: HREF is
> !HSYNC, so in order to get a hsync pulse active high, you need to set
> href_pol to 0.

I'm totally confused here. That `hsync-active = <0>` -> `href_pol: 1`
was found by `printk()`-like debugging.

(This can be not relevant or incorrect) What was found also is that
active high horizontal wire (whatever it is called in datasheet, PCB,
dts or driver) from e.g. FPGA corresponds to `href_pol: 1` to
correctly read image lines sent.

Thanks!
diff mbox series

Patch

diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h
index 001c8bde006c..88d39b3554c4 100644
--- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h
+++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h
@@ -22,8 +22,8 @@ 
 #define CSI_CFG_INPUT_FMT(fmt)			((fmt) << 20)
 #define CSI_CFG_OUTPUT_FMT(fmt)			((fmt) << 16)
 #define CSI_CFG_YUV_DATA_SEQ(seq)		((seq) << 8)
-#define CSI_CFG_VSYNC_POL(pol)			((pol) << 2)
-#define CSI_CFG_HSYNC_POL(pol)			((pol) << 1)
+#define CSI_CFG_VREF_POL(pol)			((pol) << 2)
+#define CSI_CFG_HREF_POL(pol)			((pol) << 1)
 #define CSI_CFG_PCLK_POL(pol)			((pol) << 0)
 
 #define CSI_CPT_CTRL_REG		0x08
diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
index 8b567d0f019b..78fa1c535ac6 100644
--- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
+++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
@@ -228,7 +228,7 @@  static int sun4i_csi_start_streaming(struct vb2_queue *vq, unsigned int count)
 	struct sun4i_csi *csi = vb2_get_drv_priv(vq);
 	struct v4l2_fwnode_bus_parallel *bus = &csi->bus;
 	const struct sun4i_csi_format *csi_fmt;
-	unsigned long hsync_pol, pclk_pol, vsync_pol;
+	unsigned long href_pol, pclk_pol, vref_pol;
 	unsigned long flags;
 	unsigned int i;
 	int ret;
@@ -278,13 +278,21 @@  static int sun4i_csi_start_streaming(struct vb2_queue *vq, unsigned int count)
 	writel(CSI_WIN_CTRL_H_ACTIVE(csi->fmt.height),
 	       csi->regs + CSI_WIN_CTRL_H_REG);
 
-	hsync_pol = !!(bus->flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH);
-	vsync_pol = !!(bus->flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH);
+	/*
+	 * This hardware uses [HV]REF instead of [HV]SYNC. Based on the
+	 * provided timing diagrams in the manual, positive polarity
+	 * equals active high [HV]REF.
+	 *
+	 * When the back porch is 0, [HV]REF is more or less equivalent
+	 * to [HV]SYNC inverted.
+	 */
+	href_pol = !!(bus->flags & V4L2_MBUS_HSYNC_ACTIVE_LOW);
+	vref_pol = !!(bus->flags & V4L2_MBUS_VSYNC_ACTIVE_LOW);
 	pclk_pol = !!(bus->flags & V4L2_MBUS_PCLK_SAMPLE_RISING);
 	writel(CSI_CFG_INPUT_FMT(csi_fmt->input) |
 	       CSI_CFG_OUTPUT_FMT(csi_fmt->output) |
-	       CSI_CFG_VSYNC_POL(vsync_pol) |
-	       CSI_CFG_HSYNC_POL(hsync_pol) |
+	       CSI_CFG_VREF_POL(vref_pol) |
+	       CSI_CFG_HREF_POL(href_pol) |
 	       CSI_CFG_PCLK_POL(pclk_pol),
 	       csi->regs + CSI_CFG_REG);