2019-12-15 17:01:35

by Chen-Yu Tsai

[permalink] [raw]
Subject: [PATCH 00/14] media: sun4i-csi: A10/A20 CSI1 and R40 CSI0 support

From: Chen-Yu Tsai <[email protected]>

Hi everyone,

This series adds basic support for CSI1 on Allwinner A10/A20 and CSI0 on
Allwinner R40. The CSI1 block has the same structure and layout as the
CSI0 block. Differences include:

- Only one channel in BT.656 instead of four in CSI0
- 10-bit raw data input vs 8-bit in CSI0
- 24-bit RGB888/YUV444 input vs 16-bit RGB565/YUV422 in CSI0
- No ISP hardware (CSI SCLK not needed)

The CSI0 block in the Allwinner R40 SoC looks to be the same as the one
in the A20. The register maps line up, and they support the same
features. The R40 appears to support BT.1120 based on the feature
overview, but it is not mentioned anywhere else. Also like the A20, the
ISP is not mentioned, but the CSI special clock needs to be enabled for
the hardware to function. The manual does state that the CSI special
clock is the TOP clock for all CSI hardware, but currently no hardware
exists for us to test if CSI1 also depends on it or not.

Included are a couple of fixes for signal polarity and DRAM offset
handling.

Patches 1 and 2 add compatible strings for the newly supported hardware.

Patches 3 and 4 fix the polarity setting of [HV]sync and data sampling.
Allwinner hardware uses [HV]ref semantics instead of [HV]sync.

Patch 5 deals with the DRAM offset when the CSI hardware does DMA. The
hardware does DMA directly to the memory bus, thus requiring the address
to not be offset like when DMA is done over the system bus.

Patch 6 add support for the CSI1 hardware block. For now this simply
means not requiring the ISP clock.

Patches 7 and 8 add CSI1 to A10 (sun4i) and A20 (sun7i) dtsi files.

Patch 9 adds I2C pixmuxing options for the R40. Used in the last example
patch.

Patch 10 adds a compatible string for the R40's MBUS (memory bus).

Patch 11 adds CSI0 to the R40 dtsi file

Patches 12 through 14 are examples of cameras hooked up to boards.

Please have a look. The MBUS compatible patch is likely to conflict
with a DT binding conversion patch Maxime sent out.

Also, I sent out an email asking about the polarity settings for
[HV]sync, how to signal the use of [HV]ref instead, and how to pass
timings from the camera to the capture interface. So far I haven't
heard back. In particular I think the OV7670 driver has inverted
polarity settings for HSYNC. Not sure about VSYNC.


Regards
ChenYu


Chen-Yu Tsai (14):
dt-bindings: media: sun4i-csi: Add compatible for CSI1 on A10/A20
dt-bindings: media: sun4i-csi: Add compatible for CSI0 on R40
media: sun4i-csi: Fix data sampling polarity handling
media: sun4i-csi: Fix [HV]sync polarity handling
media: sun4i-csi: Deal with DRAM offset
media: sun4i-csi: Add support for A10 CSI1 camera sensor interface
ARM: dts: sun4i: Add CSI1 controller and pinmux options
ARM: dts: sun7i: Add CSI1 controller and pinmux options
ARM: dts: sun8i: r40: Add I2C pinmux options
dt-bindings: bus: sunxi: Add R40 MBUS compatible
ARM: dts: sun8i: r40: Add device node for CSI0
[DO NOT MERGE] ARM: dts: sun4i: cubieboard: Enable OV7670 camera on
CSI1
[DO NOT MERGE] ARM: dts: sun7i: cubieboard2: Enable OV7670 camera on
CSI1
[DO NOT MERGE] ARM: dts: sun8i-r40: bananapi-m2-ultra: Enable OV5640
camera

.../bindings/arm/sunxi/sunxi-mbus.txt | 1 +
.../media/allwinner,sun4i-a10-csi.yaml | 14 +++-
arch/arm/boot/dts/sun4i-a10-cubieboard.dts | 42 ++++++++++++
arch/arm/boot/dts/sun4i-a10.dtsi | 35 ++++++++++
arch/arm/boot/dts/sun7i-a20-cubieboard2.dts | 42 ++++++++++++
arch/arm/boot/dts/sun7i-a20.dtsi | 36 ++++++++++
.../boot/dts/sun8i-r40-bananapi-m2-ultra.dts | 67 +++++++++++++++++++
arch/arm/boot/dts/sun8i-r40.dtsi | 64 ++++++++++++++++++
.../platform/sunxi/sun4i-csi/sun4i_csi.c | 57 ++++++++++++++--
.../platform/sunxi/sun4i-csi/sun4i_csi.h | 6 +-
.../platform/sunxi/sun4i-csi/sun4i_dma.c | 20 ++++--
11 files changed, 370 insertions(+), 14 deletions(-)

--
2.24.0


2019-12-15 17:01:52

by Chen-Yu Tsai

[permalink] [raw]
Subject: [PATCH 02/14] dt-bindings: media: sun4i-csi: Add compatible for CSI0 on R40

From: Chen-Yu Tsai <[email protected]>

The CSI0 block in the Allwinner R40 SoC looks to be the same as the one
in the A20. The register maps line up, and they support the same
features. The R40 appears to support BT.1120 based on the feature
overview, but it is not mentioned anywhere else. Also like the A20, the
ISP is not mentioned, but the CSI special clock needs to be enabled for
the hardware to function. The manual does state that the CSI special
clock is the TOP clock for all CSI hardware, but currently no hardware
exists for us to test if CSI1 also depends on it or not.

Add a compatible string for the CSI0 block in the R40, with the A20
compatible string as a fallback.

Signed-off-by: Chen-Yu Tsai <[email protected]>
---
.../devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml | 3 +++
1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml b/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml
index 221fe630c7d5..d486321b13f5 100644
--- a/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml
+++ b/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml
@@ -22,6 +22,9 @@ properties:
- items:
- const: allwinner,sun7i-a20-csi1
- const: allwinner,sun4i-a10-csi1
+ - items:
+ - const: allwinner,sun8i-r40-csi0
+ - const: allwinner,sun7i-a20-csi0

reg:
maxItems: 1
--
2.24.0

2019-12-15 17:02:06

by Chen-Yu Tsai

[permalink] [raw]
Subject: [PATCH 03/14] media: sun4i-csi: Fix data sampling polarity handling

From: Chen-Yu Tsai <[email protected]>

The CLK_POL field specifies whether data is sampled on the falling or
rising edge of PCLK, not whether the data lines are active high or low.
Evidence of this can be found in the timing diagram labeled "horizontal
size setting and pixel clock timing".

Fix the setting by checking the correct flag, V4L2_MBUS_PCLK_SAMPLE_RISING.
While at it, reorder the three polarity flag checks so HSYNC and VSYNC
are grouped together.

Fixes: 577bbf23b758 ("media: sunxi: Add A10 CSI driver")
Signed-off-by: Chen-Yu Tsai <[email protected]>
---
drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
index d6979e11a67b..8b567d0f019b 100644
--- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
+++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
@@ -279,8 +279,8 @@ static int sun4i_csi_start_streaming(struct vb2_queue *vq, unsigned int count)
csi->regs + CSI_WIN_CTRL_H_REG);

hsync_pol = !!(bus->flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH);
- pclk_pol = !!(bus->flags & V4L2_MBUS_DATA_ACTIVE_HIGH);
vsync_pol = !!(bus->flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH);
+ 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) |
--
2.24.0

2019-12-15 17:02:21

by Chen-Yu Tsai

[permalink] [raw]
Subject: [PATCH 13/14] [DO NOT MERGE] ARM: dts: sun7i: cubieboard2: Enable OV7670 camera on CSI1

From: Chen-Yu Tsai <[email protected]>

The Cubieboard2 has CSI1 pins exposed on one of its GPIO headers.
Combined with I2C1 on the same header, a connected OV7670 based
camera module can be used. Power is provided via the 5V rail on
the same header. The module has onboard LDOs for the sensor's
various power rails.

Add a device node for the sensor, enable CSI1 and I2C1, and hook
everything up.

Signed-off-by: Chen-Yu Tsai <[email protected]>
---
arch/arm/boot/dts/sun7i-a20-cubieboard2.dts | 42 +++++++++++++++++++++
1 file changed, 42 insertions(+)

diff --git a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
index b8203e4ef21c..0ff1593041eb 100644
--- a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
+++ b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
@@ -100,6 +100,25 @@ &cpu0 {
cpu-supply = <&reg_dcdc2>;
};

+&csi1 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&csi1_8bits_pg_pins>;
+ status = "okay";
+
+ port {
+ /* Parallel bus endpoint */
+ csi_from_ov7670: endpoint {
+ remote-endpoint = <&ov7670_to_csi>;
+ bus-width = <8>;
+ /* driver is broken */
+ hsync-active = <0>; /* Active high */
+ vsync-active = <1>; /* Active high */
+ data-active = <1>; /* Active high */
+ pclk-sample = <1>; /* Rising */
+ };
+ };
+};
+
&de {
status = "okay";
};
@@ -142,6 +161,29 @@ axp209: pmic@34 {

&i2c1 {
status = "okay";
+
+ ov7670: camera@21 {
+ compatible = "ovti,ov7670";
+ reg = <0x21>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&csi1_clk_pg_pin>;
+ clocks = <&ccu CLK_CSI1>;
+ clock-names = "xclk";
+
+ reset-gpios = <&pio 7 14 GPIO_ACTIVE_LOW>; /* PH14 */
+ powerdown-gpios = <&pio 7 15 GPIO_ACTIVE_HIGH>; /* PH15 */
+
+ port {
+ ov7670_to_csi: endpoint {
+ remote-endpoint = <&csi_from_ov7670>;
+ bus-width = <8>;
+ hsync-active = <1>; /* Active high */
+ vsync-active = <1>; /* Active high */
+ data-active = <1>; /* Active high */
+ pclk-sample = <1>; /* Rising */
+ };
+ };
+ };
};

&ir0 {
--
2.24.0

2019-12-15 17:02:30

by Chen-Yu Tsai

[permalink] [raw]
Subject: [PATCH 04/14] media: sun4i-csi: Fix [HV]sync polarity handling

From: Chen-Yu Tsai <[email protected]>

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 <[email protected]>
---
.../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(-)

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);

--
2.24.0

2019-12-15 17:02:48

by Chen-Yu Tsai

[permalink] [raw]
Subject: [PATCH 01/14] dt-bindings: media: sun4i-csi: Add compatible for CSI1 on A10/A20

From: Chen-Yu Tsai <[email protected]>

The CSI1 block has the same structure and layout as the CSI0 block.
Differences include:

- Only one channel in BT.656 instead of four in CSI0
- 10-bit raw data input vs 8-bit in CSI0
- 24-bit RGB888/YUV444 input vs 16-bit RGB565/YUV422 in CSI0
- No ISP hardware

The hardware found in the A20 is the same as in the A10.

Signed-off-by: Chen-Yu Tsai <[email protected]>
---
.../bindings/media/allwinner,sun4i-a10-csi.yaml | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml b/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml
index d3e423fcb6c2..221fe630c7d5 100644
--- a/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml
+++ b/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml
@@ -16,7 +16,12 @@ description: |-

properties:
compatible:
- const: allwinner,sun7i-a20-csi0
+ oneOf:
+ - const: allwinner,sun4i-a10-csi1
+ - const: allwinner,sun7i-a20-csi0
+ - items:
+ - const: allwinner,sun7i-a20-csi1
+ - const: allwinner,sun4i-a10-csi1

reg:
maxItems: 1
@@ -25,12 +30,16 @@ properties:
maxItems: 1

clocks:
+ minItems: 2
+ maxItems: 3
items:
- description: The CSI interface clock
- description: The CSI ISP clock
- description: The CSI DRAM clock

clock-names:
+ minItems: 2
+ maxItems: 3
items:
- const: bus
- const: isp
--
2.24.0

2019-12-19 23:57:07

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH 01/14] dt-bindings: media: sun4i-csi: Add compatible for CSI1 on A10/A20

On Mon, 16 Dec 2019 00:59:11 +0800, Chen-Yu Tsai wrote:
> From: Chen-Yu Tsai <[email protected]>
>
> The CSI1 block has the same structure and layout as the CSI0 block.
> Differences include:
>
> - Only one channel in BT.656 instead of four in CSI0
> - 10-bit raw data input vs 8-bit in CSI0
> - 24-bit RGB888/YUV444 input vs 16-bit RGB565/YUV422 in CSI0
> - No ISP hardware
>
> The hardware found in the A20 is the same as in the A10.
>
> Signed-off-by: Chen-Yu Tsai <[email protected]>
> ---
> .../bindings/media/allwinner,sun4i-a10-csi.yaml | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>

Reviewed-by: Rob Herring <[email protected]>

2019-12-19 23:59:12

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH 02/14] dt-bindings: media: sun4i-csi: Add compatible for CSI0 on R40

On Mon, 16 Dec 2019 00:59:12 +0800, Chen-Yu Tsai wrote:
> From: Chen-Yu Tsai <[email protected]>
>
> The CSI0 block in the Allwinner R40 SoC looks to be the same as the one
> in the A20. The register maps line up, and they support the same
> features. The R40 appears to support BT.1120 based on the feature
> overview, but it is not mentioned anywhere else. Also like the A20, the
> ISP is not mentioned, but the CSI special clock needs to be enabled for
> the hardware to function. The manual does state that the CSI special
> clock is the TOP clock for all CSI hardware, but currently no hardware
> exists for us to test if CSI1 also depends on it or not.
>
> Add a compatible string for the CSI0 block in the R40, with the A20
> compatible string as a fallback.
>
> Signed-off-by: Chen-Yu Tsai <[email protected]>
> ---
> .../devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml | 3 +++
> 1 file changed, 3 insertions(+)
>

Acked-by: Rob Herring <[email protected]>

2020-01-01 10:22:11

by Chen-Yu Tsai

[permalink] [raw]
Subject: Re: [PATCH 00/14] media: sun4i-csi: A10/A20 CSI1 and R40 CSI0 support

Hi Sakari,


On Mon, Dec 16, 2019 at 12:59 AM Chen-Yu Tsai <[email protected]> wrote:
>
> From: Chen-Yu Tsai <[email protected]>
>
> Hi everyone,
>
> This series adds basic support for CSI1 on Allwinner A10/A20 and CSI0 on
> Allwinner R40. The CSI1 block has the same structure and layout as the
> CSI0 block. Differences include:
>
> - Only one channel in BT.656 instead of four in CSI0
> - 10-bit raw data input vs 8-bit in CSI0
> - 24-bit RGB888/YUV444 input vs 16-bit RGB565/YUV422 in CSI0
> - No ISP hardware (CSI SCLK not needed)
>
> The CSI0 block in the Allwinner R40 SoC looks to be the same as the one
> in the A20. The register maps line up, and they support the same
> features. The R40 appears to support BT.1120 based on the feature
> overview, but it is not mentioned anywhere else. Also like the A20, the
> ISP is not mentioned, but the CSI special clock needs to be enabled for
> the hardware to function. The manual does state that the CSI special
> clock is the TOP clock for all CSI hardware, but currently no hardware
> exists for us to test if CSI1 also depends on it or not.
>
> Included are a couple of fixes for signal polarity and DRAM offset
> handling.
>
> Patches 1 and 2 add compatible strings for the newly supported hardware.
>
> Patches 3 and 4 fix the polarity setting of [HV]sync and data sampling.
> Allwinner hardware uses [HV]ref semantics instead of [HV]sync.
>
> Patch 5 deals with the DRAM offset when the CSI hardware does DMA. The
> hardware does DMA directly to the memory bus, thus requiring the address
> to not be offset like when DMA is done over the system bus.
>
> Patch 6 add support for the CSI1 hardware block. For now this simply
> means not requiring the ISP clock.
>
> Patches 7 and 8 add CSI1 to A10 (sun4i) and A20 (sun7i) dtsi files.
>
> Patch 9 adds I2C pixmuxing options for the R40. Used in the last example
> patch.
>
> Patch 10 adds a compatible string for the R40's MBUS (memory bus).
>
> Patch 11 adds CSI0 to the R40 dtsi file
>
> Patches 12 through 14 are examples of cameras hooked up to boards.
>
> Please have a look. The MBUS compatible patch is likely to conflict
> with a DT binding conversion patch Maxime sent out.
>
> Also, I sent out an email asking about the polarity settings for
> [HV]sync, how to signal the use of [HV]ref instead, and how to pass
> timings from the camera to the capture interface. So far I haven't
> heard back. In particular I think the OV7670 driver has inverted
> polarity settings for HSYNC. Not sure about VSYNC.
>
>
> Regards
> ChenYu
>
>
> Chen-Yu Tsai (14):
> dt-bindings: media: sun4i-csi: Add compatible for CSI1 on A10/A20
> dt-bindings: media: sun4i-csi: Add compatible for CSI0 on R40
> media: sun4i-csi: Fix data sampling polarity handling
> media: sun4i-csi: Fix [HV]sync polarity handling
> media: sun4i-csi: Deal with DRAM offset
> media: sun4i-csi: Add support for A10 CSI1 camera sensor interface

Any news on these 6 patches? I believe they have the required acks.

We (sunxi maintainers) will take the remain patches once these are in.

ChenYu

> ARM: dts: sun4i: Add CSI1 controller and pinmux options
> ARM: dts: sun7i: Add CSI1 controller and pinmux options
> ARM: dts: sun8i: r40: Add I2C pinmux options
> dt-bindings: bus: sunxi: Add R40 MBUS compatible
> ARM: dts: sun8i: r40: Add device node for CSI0
> [DO NOT MERGE] ARM: dts: sun4i: cubieboard: Enable OV7670 camera on
> CSI1
> [DO NOT MERGE] ARM: dts: sun7i: cubieboard2: Enable OV7670 camera on
> CSI1
> [DO NOT MERGE] ARM: dts: sun8i-r40: bananapi-m2-ultra: Enable OV5640
> camera
>
> .../bindings/arm/sunxi/sunxi-mbus.txt | 1 +
> .../media/allwinner,sun4i-a10-csi.yaml | 14 +++-
> arch/arm/boot/dts/sun4i-a10-cubieboard.dts | 42 ++++++++++++
> arch/arm/boot/dts/sun4i-a10.dtsi | 35 ++++++++++
> arch/arm/boot/dts/sun7i-a20-cubieboard2.dts | 42 ++++++++++++
> arch/arm/boot/dts/sun7i-a20.dtsi | 36 ++++++++++
> .../boot/dts/sun8i-r40-bananapi-m2-ultra.dts | 67 +++++++++++++++++++
> arch/arm/boot/dts/sun8i-r40.dtsi | 64 ++++++++++++++++++
> .../platform/sunxi/sun4i-csi/sun4i_csi.c | 57 ++++++++++++++--
> .../platform/sunxi/sun4i-csi/sun4i_csi.h | 6 +-
> .../platform/sunxi/sun4i-csi/sun4i_dma.c | 20 ++++--
> 11 files changed, 370 insertions(+), 14 deletions(-)
>
> --
> 2.24.0
>

2023-01-16 10:34:45

by Oleg Verych

[permalink] [raw]
Subject: Re: [PATCH 04/14] media: sun4i-csi: Fix [HV]sync polarity handling

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/[email protected]/

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

2023-01-16 11:12:44

by Maxime Ripard

[permalink] [raw]
Subject: Re: [PATCH 04/14] media: sun4i-csi: Fix [HV]sync polarity handling

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/[email protected]/
>
> > + 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


Attachments:
(No filename) (3.01 kB)
signature.asc (235.00 B)
Download all attachments

2023-01-16 19:12:49

by Oleg Verych

[permalink] [raw]
Subject: Re: [PATCH 04/14] media: sun4i-csi: Fix [HV]sync polarity handling

Hi!

On 1/16/23, Maxime Ripard <[email protected]> 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!
--
sed 'sh && sed && node.js + olecom = happiness and mirth' << ''
-o--=O`C
#oo'L O
<___=E M