| 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 |
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
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 */ > + }; > + };
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
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 --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);