[PATCH v5 0/6] Allwinner A31/A83T MIPI CSI-2 and A31 ISP / Platform Support

[PATCH v5 0/6] Allwinner A31/A83T MIPI CSI-2 and A31 ISP / Platform Support

[Paul Kocialkowski]

This part only concerns Allwinner platform support changes.

Though some patches still need reviewing, they feel ready to go since the
MIPI CSI-2 bindings were merged, the updated CSI bindings were reviewed by
Rob and the new ISP bindings as well. Although they were not merged yet,
they seem ready to go at this point.

Since this multi-part series has been going on for a while, it would be
great to see it merged soon!

Changes since v4:
- Removed mbus bindings patch: an equivalent change was merged;
- Added collected tags;
- Rebased on latest media tree.

Changes since v3:
- Reordered v3s mbus compatible in binding;
- Added collected tag;
- Removed rejected interconnects fix.

Changes since all-in-one v2:
- Corrected mbus index used for the interconnects;
- Used extended mbus binding and exported the DRAM clock for that;
- Reworked the description of the core openfirmware change to give
  more insight about the situation.

Kévin L'hôpital (1):
  ARM: dts: sun8i: a83t: bananapi-m3: Enable MIPI CSI-2 with OV8865

Paul Kocialkowski (5):
  clk: sunxi-ng: v3s: Export MBUS and DRAM clocks to the public header
  ARM: dts: sun8i: v3s: Add mbus node to represent the interconnect
  ARM: dts: sun8i: v3s: Add nodes for MIPI CSI-2 support
  ARM: dts: sun8i: v3s: Add support for the ISP
  ARM: dts: sun8i: a83t: Add MIPI CSI-2 controller node

 arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts | 102 ++++++++++++++++
 arch/arm/boot/dts/sun8i-a83t.dtsi            |  26 ++++
 arch/arm/boot/dts/sun8i-v3s.dtsi             | 121 +++++++++++++++++++
 drivers/clk/sunxi-ng/ccu-sun8i-v3s.h         |   4 -
 include/dt-bindings/clock/sun8i-v3s-ccu.h    |   4 +-
 5 files changed, 251 insertions(+), 6 deletions(-)

-- 
2.37.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v5 1/6] clk: sunxi-ng: v3s: Export MBUS and DRAM clocks to the public header

[Paul Kocialkowski]

In order to declare a mbus node for the v3s, expose its associated
clocks to the public header.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Reviewed-by: Samuel Holland <samuel@sholland.org>
Acked-by: Rob Herring <robh@kernel.org>
---
 drivers/clk/sunxi-ng/ccu-sun8i-v3s.h      | 4 ----
 include/dt-bindings/clock/sun8i-v3s-ccu.h | 4 ++--
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-v3s.h b/drivers/clk/sunxi-ng/ccu-sun8i-v3s.h
index 108eeeedcbf7..8ed4eff86ca1 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-v3s.h
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-v3s.h
@@ -39,14 +39,10 @@
 
 /* The first bunch of module clocks are exported */
 
-#define CLK_DRAM		58
-
 /* All the DRAM gates are exported */
 
 /* Some more module clocks are exported */
 
-#define CLK_MBUS		72
-
 /* And the GPU module clock is exported */
 
 #define CLK_PLL_DDR1		74
diff --git a/include/dt-bindings/clock/sun8i-v3s-ccu.h b/include/dt-bindings/clock/sun8i-v3s-ccu.h
index 014ac6123d17..4231f23bc53b 100644
--- a/include/dt-bindings/clock/sun8i-v3s-ccu.h
+++ b/include/dt-bindings/clock/sun8i-v3s-ccu.h
@@ -87,7 +87,7 @@
 #define CLK_SPI0		55
 #define CLK_USB_PHY0		56
 #define CLK_USB_OHCI0		57
-
+#define CLK_DRAM		58
 #define CLK_DRAM_VE		59
 #define CLK_DRAM_CSI		60
 #define CLK_DRAM_EHCI		61
@@ -101,7 +101,7 @@
 #define CLK_VE			69
 #define CLK_AC_DIG		70
 #define CLK_AVS			71
-
+#define CLK_MBUS		72
 #define CLK_MIPI_CSI		73
 
 /* Clocks not available on V3s */
-- 
2.37.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v5 2/6] ARM: dts: sun8i: v3s: Add mbus node to represent the interconnect

[Paul Kocialkowski]

The V3s uses the mbus interconnect to provide DRAM access for a
number of blocks. The SoC can only map 2 GiB of DRAM, which is
reflected in the dma-ranges property.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Reviewed-by: Samuel Holland <samuel@sholland.org>
---
 arch/arm/boot/dts/sun8i-v3s.dtsi | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-v3s.dtsi b/arch/arm/boot/dts/sun8i-v3s.dtsi
index db194c606fdc..34f7df6b3960 100644
--- a/arch/arm/boot/dts/sun8i-v3s.dtsi
+++ b/arch/arm/boot/dts/sun8i-v3s.dtsi
@@ -579,6 +579,21 @@ int_mii_phy: ethernet-phy@1 {
 			};
 		};
 
+		mbus: dram-controller@1c62000 {
+			compatible = "allwinner,sun8i-v3s-mbus";
+			reg = <0x01c62000 0x1000>,
+			      <0x01c63000 0x1000>;
+			reg-names = "mbus", "dram";
+			clocks = <&ccu CLK_MBUS>,
+				 <&ccu CLK_DRAM>,
+				 <&ccu CLK_BUS_DRAM>;
+			clock-names = "mbus", "dram", "bus";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			dma-ranges = <0x00000000 0x40000000 0x80000000>;
+			#interconnect-cells = <1>;
+		};
+
 		spi0: spi@1c68000 {
 			compatible = "allwinner,sun8i-h3-spi";
 			reg = <0x01c68000 0x1000>;
-- 
2.37.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v5 3/6] ARM: dts: sun8i: v3s: Add nodes for MIPI CSI-2 support

[Paul Kocialkowski]

MIPI CSI-2 is supported on the V3s with an A31-based MIPI CSI-2 bridge
controller. The controller uses a separate D-PHY, which is the same
that is otherwise used for MIPI DSI, but used in Rx mode.

On the V3s, the CSI0 controller is dedicated to MIPI CSI-2 as it does
not have access to any parallel interface pins.

Add all the necessary nodes (CSI0, MIPI CSI-2 bridge and D-PHY) to
support the MIPI CSI-2 interface.

Note that a fwnode graph link is created between CSI0 and MIPI CSI-2
even when no sensor is connected. This will result in a probe failure
for the controller as long as no sensor is connected but this is fine
since no other interface is available.

The interconnects property is used to inherit the proper DMA offset.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 arch/arm/boot/dts/sun8i-v3s.dtsi | 71 ++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-v3s.dtsi b/arch/arm/boot/dts/sun8i-v3s.dtsi
index 34f7df6b3960..99966f11d061 100644
--- a/arch/arm/boot/dts/sun8i-v3s.dtsi
+++ b/arch/arm/boot/dts/sun8i-v3s.dtsi
@@ -621,6 +621,77 @@ gic: interrupt-controller@1c81000 {
 			interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
 		};
 
+		csi0: camera@1cb0000 {
+			compatible = "allwinner,sun8i-v3s-csi";
+			reg = <0x01cb0000 0x1000>;
+			interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_CSI>,
+				 <&ccu CLK_CSI1_SCLK>,
+				 <&ccu CLK_DRAM_CSI>;
+			clock-names = "bus", "mod", "ram";
+			resets = <&ccu RST_BUS_CSI>;
+			interconnects = <&mbus 5>;
+			interconnect-names = "dma-mem";
+			status = "disabled";
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				port@1 {
+					reg = <1>;
+
+					csi0_in_mipi_csi2: endpoint {
+						remote-endpoint = <&mipi_csi2_out_csi0>;
+					};
+				};
+			};
+		};
+
+		mipi_csi2: csi@1cb1000 {
+			compatible = "allwinner,sun8i-v3s-mipi-csi2",
+				     "allwinner,sun6i-a31-mipi-csi2";
+			reg = <0x01cb1000 0x1000>;
+			interrupts = <GIC_SPI 90 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_CSI>,
+				 <&ccu CLK_CSI1_SCLK>;
+			clock-names = "bus", "mod";
+			resets = <&ccu RST_BUS_CSI>;
+			status = "disabled";
+
+			phys = <&dphy>;
+			phy-names = "dphy";
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				mipi_csi2_in: port@0 {
+					reg = <0>;
+				};
+
+				mipi_csi2_out: port@1 {
+					reg = <1>;
+
+					mipi_csi2_out_csi0: endpoint {
+						remote-endpoint = <&csi0_in_mipi_csi2>;
+					};
+				};
+			};
+		};
+
+		dphy: d-phy@1cb2000 {
+			compatible = "allwinner,sun6i-a31-mipi-dphy";
+			reg = <0x01cb2000 0x1000>;
+			clocks = <&ccu CLK_BUS_CSI>,
+				 <&ccu CLK_MIPI_CSI>;
+			clock-names = "bus", "mod";
+			resets = <&ccu RST_BUS_CSI>;
+			allwinner,direction = "rx";
+			status = "disabled";
+			#phy-cells = <0>;
+		};
+
 		csi1: camera@1cb4000 {
 			compatible = "allwinner,sun8i-v3s-csi";
 			reg = <0x01cb4000 0x3000>;
-- 
2.37.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v5 4/6] ARM: dts: sun8i: v3s: Add support for the ISP

[Paul Kocialkowski]

The V3s (and related platforms) come with an instance of the A31 ISP.
Even though it is very close to the A31 ISP, it is not exactly
register-compatible and a dedicated compatible only is used as a
result.

Just like most other blocks of the camera pipeline, the ISP uses
the common CSI bus, module and ram clock as well as reset.

A port connection to the ISP is added to CSI0 for convenience since
CSI0 serves for MIPI CSI-2 interface support, which is likely to
receive raw data that will need to be processed by the ISP to produce
a final image.

The interconnects property is used to inherit the proper DMA offset.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 arch/arm/boot/dts/sun8i-v3s.dtsi | 35 ++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-v3s.dtsi b/arch/arm/boot/dts/sun8i-v3s.dtsi
index 99966f11d061..a249b825cfe7 100644
--- a/arch/arm/boot/dts/sun8i-v3s.dtsi
+++ b/arch/arm/boot/dts/sun8i-v3s.dtsi
@@ -645,6 +645,14 @@ csi0_in_mipi_csi2: endpoint {
 						remote-endpoint = <&mipi_csi2_out_csi0>;
 					};
 				};
+
+				port@2 {
+					reg = <2>;
+
+					csi0_out_isp: endpoint {
+						remote-endpoint = <&isp_in_csi0>;
+					};
+				};
 			};
 		};
 
@@ -703,5 +711,32 @@ csi1: camera@1cb4000 {
 			resets = <&ccu RST_BUS_CSI>;
 			status = "disabled";
 		};
+
+		isp: isp@1cb8000 {
+			compatible = "allwinner,sun8i-v3s-isp";
+			reg = <0x01cb8000 0x1000>;
+			interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_CSI>,
+				 <&ccu CLK_CSI1_SCLK>,
+				 <&ccu CLK_DRAM_CSI>;
+			clock-names = "bus", "mod", "ram";
+			resets = <&ccu RST_BUS_CSI>;
+			interconnects = <&mbus 5>;
+			interconnect-names = "dma-mem";
+			status = "disabled";
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				port@0 {
+					reg = <0>;
+
+					isp_in_csi0: endpoint {
+						remote-endpoint = <&csi0_out_isp>;
+					};
+				};
+			};
+		};
 	};
 };
-- 
2.37.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v5 5/6] ARM: dts: sun8i: a83t: Add MIPI CSI-2 controller node

[Paul Kocialkowski]

MIPI CSI-2 is supported on the A83T with a dedicated controller that
covers both the protocol and D-PHY. It can be connected to the CSI
interface as a V4L2 subdev through the fwnode graph.

This is not done by default since connecting the bridge without a
subdev attached to it will cause a failure on the CSI driver.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 arch/arm/boot/dts/sun8i-a83t.dtsi | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-a83t.dtsi b/arch/arm/boot/dts/sun8i-a83t.dtsi
index 82fdb04122ca..ecf9f3b2c0c0 100644
--- a/arch/arm/boot/dts/sun8i-a83t.dtsi
+++ b/arch/arm/boot/dts/sun8i-a83t.dtsi
@@ -1064,6 +1064,32 @@ csi: camera@1cb0000 {
 			status = "disabled";
 		};
 
+		mipi_csi2: csi@1cb1000 {
+			compatible = "allwinner,sun8i-a83t-mipi-csi2";
+			reg = <0x01cb1000 0x1000>;
+			interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_CSI>,
+				 <&ccu CLK_CSI_SCLK>,
+				 <&ccu CLK_MIPI_CSI>,
+				 <&ccu CLK_CSI_MISC>;
+			clock-names = "bus", "mod", "mipi", "misc";
+			resets = <&ccu RST_BUS_CSI>;
+			status = "disabled";
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				mipi_csi2_in: port@0 {
+					reg = <0>;
+				};
+
+				mipi_csi2_out: port@1 {
+					reg = <1>;
+				};
+			};
+		};
+
 		hdmi: hdmi@1ee0000 {
 			compatible = "allwinner,sun8i-a83t-dw-hdmi";
 			reg = <0x01ee0000 0x10000>;
-- 
2.37.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 5/6] ARM: dts: sun8i: a83t: Add MIPI CSI-2 controller node

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Fri, Aug 26, 2022 at 08:28:02PM +0200, Paul Kocialkowski wrote:
> MIPI CSI-2 is supported on the A83T with a dedicated controller that
> covers both the protocol and D-PHY. It can be connected to the CSI
> interface as a V4L2 subdev through the fwnode graph.
> 
> This is not done by default since connecting the bridge without a
> subdev attached to it will cause a failure on the CSI driver.

No urgency, but would it be possible to fix this so that the CSI-2
receiver can be connected to the CSI unconditionally in DT ? The
connection exists at the hardware level in the SoC, and should thus
exist here too, regardless of whether or not a sensor is connected.

> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  arch/arm/boot/dts/sun8i-a83t.dtsi | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun8i-a83t.dtsi b/arch/arm/boot/dts/sun8i-a83t.dtsi
> index 82fdb04122ca..ecf9f3b2c0c0 100644
> --- a/arch/arm/boot/dts/sun8i-a83t.dtsi
> +++ b/arch/arm/boot/dts/sun8i-a83t.dtsi
> @@ -1064,6 +1064,32 @@ csi: camera@1cb0000 {
>  			status = "disabled";
>  		};
>  
> +		mipi_csi2: csi@1cb1000 {
> +			compatible = "allwinner,sun8i-a83t-mipi-csi2";
> +			reg = <0x01cb1000 0x1000>;
> +			interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&ccu CLK_BUS_CSI>,
> +				 <&ccu CLK_CSI_SCLK>,
> +				 <&ccu CLK_MIPI_CSI>,
> +				 <&ccu CLK_CSI_MISC>;
> +			clock-names = "bus", "mod", "mipi", "misc";
> +			resets = <&ccu RST_BUS_CSI>;
> +			status = "disabled";
> +
> +			ports {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				mipi_csi2_in: port@0 {
> +					reg = <0>;
> +				};
> +
> +				mipi_csi2_out: port@1 {
> +					reg = <1>;
> +				};
> +			};
> +		};
> +
>  		hdmi: hdmi@1ee0000 {
>  			compatible = "allwinner,sun8i-a83t-dw-hdmi";
>  			reg = <0x01ee0000 0x10000>;

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 5/6] ARM: dts: sun8i: a83t: Add MIPI CSI-2 controller node

[Paul Kocialkowski]

[-- Attachment #1.1: Type: text/plain, Size: 2933 bytes --]

Hi Laurent,

On Fri 26 Aug 22, 21:59, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Fri, Aug 26, 2022 at 08:28:02PM +0200, Paul Kocialkowski wrote:
> > MIPI CSI-2 is supported on the A83T with a dedicated controller that
> > covers both the protocol and D-PHY. It can be connected to the CSI
> > interface as a V4L2 subdev through the fwnode graph.
> > 
> > This is not done by default since connecting the bridge without a
> > subdev attached to it will cause a failure on the CSI driver.
> 
> No urgency, but would it be possible to fix this so that the CSI-2
> receiver can be connected to the CSI unconditionally in DT ? The
> connection exists at the hardware level in the SoC, and should thus
> exist here too, regardless of whether or not a sensor is connected.

Yes it's true that having the link always would be legitimate.

For the context, this CSI controller can be switched between the MIPI CSI-2
controller and a parallel sensor input (i.e. it's not dedicated to one or the
other like on the V3).

Last time I tried, having the connection between the two always there resulted
in the unability to use a parallel sensor when no sensor is attached to the
mipi csi-2 receiver. Probably because the async notifier never completes since
the mipi csi-2's subdev is never registered without a sensor subdev attached.

Do you see a way to handle this case properly?

Thanks,

Paul

> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > ---
> >  arch/arm/boot/dts/sun8i-a83t.dtsi | 26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/sun8i-a83t.dtsi b/arch/arm/boot/dts/sun8i-a83t.dtsi
> > index 82fdb04122ca..ecf9f3b2c0c0 100644
> > --- a/arch/arm/boot/dts/sun8i-a83t.dtsi
> > +++ b/arch/arm/boot/dts/sun8i-a83t.dtsi
> > @@ -1064,6 +1064,32 @@ csi: camera@1cb0000 {
> >  			status = "disabled";
> >  		};
> >  
> > +		mipi_csi2: csi@1cb1000 {
> > +			compatible = "allwinner,sun8i-a83t-mipi-csi2";
> > +			reg = <0x01cb1000 0x1000>;
> > +			interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>;
> > +			clocks = <&ccu CLK_BUS_CSI>,
> > +				 <&ccu CLK_CSI_SCLK>,
> > +				 <&ccu CLK_MIPI_CSI>,
> > +				 <&ccu CLK_CSI_MISC>;
> > +			clock-names = "bus", "mod", "mipi", "misc";
> > +			resets = <&ccu RST_BUS_CSI>;
> > +			status = "disabled";
> > +
> > +			ports {
> > +				#address-cells = <1>;
> > +				#size-cells = <0>;
> > +
> > +				mipi_csi2_in: port@0 {
> > +					reg = <0>;
> > +				};
> > +
> > +				mipi_csi2_out: port@1 {
> > +					reg = <1>;
> > +				};
> > +			};
> > +		};
> > +
> >  		hdmi: hdmi@1ee0000 {
> >  			compatible = "allwinner,sun8i-a83t-dw-hdmi";
> >  			reg = <0x01ee0000 0x10000>;
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 5/6] ARM: dts: sun8i: a83t: Add MIPI CSI-2 controller node

[Laurent Pinchart]

On Thu, Sep 01, 2022 at 04:04:40PM +0200, Paul Kocialkowski wrote:
> Hi Laurent,
> 
> On Fri 26 Aug 22, 21:59, Laurent Pinchart wrote:
> > Hi Paul,
> > 
> > Thank you for the patch.
> > 
> > On Fri, Aug 26, 2022 at 08:28:02PM +0200, Paul Kocialkowski wrote:
> > > MIPI CSI-2 is supported on the A83T with a dedicated controller that
> > > covers both the protocol and D-PHY. It can be connected to the CSI
> > > interface as a V4L2 subdev through the fwnode graph.
> > > 
> > > This is not done by default since connecting the bridge without a
> > > subdev attached to it will cause a failure on the CSI driver.
> > 
> > No urgency, but would it be possible to fix this so that the CSI-2
> > receiver can be connected to the CSI unconditionally in DT ? The
> > connection exists at the hardware level in the SoC, and should thus
> > exist here too, regardless of whether or not a sensor is connected.
> 
> Yes it's true that having the link always would be legitimate.
> 
> For the context, this CSI controller can be switched between the MIPI CSI-2
> controller and a parallel sensor input (i.e. it's not dedicated to one or the
> other like on the V3).
> 
> Last time I tried, having the connection between the two always there resulted
> in the unability to use a parallel sensor when no sensor is attached to the
> mipi csi-2 receiver. Probably because the async notifier never completes since
> the mipi csi-2's subdev is never registered without a sensor subdev attached.
> 
> Do you see a way to handle this case properly?

It sounds like an issue in the CSI-2 receiver driver. If there's no
input device attached to it, it should register its subdev directly,
without its own async notifier.

> > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > ---
> > >  arch/arm/boot/dts/sun8i-a83t.dtsi | 26 ++++++++++++++++++++++++++
> > >  1 file changed, 26 insertions(+)
> > > 
> > > diff --git a/arch/arm/boot/dts/sun8i-a83t.dtsi b/arch/arm/boot/dts/sun8i-a83t.dtsi
> > > index 82fdb04122ca..ecf9f3b2c0c0 100644
> > > --- a/arch/arm/boot/dts/sun8i-a83t.dtsi
> > > +++ b/arch/arm/boot/dts/sun8i-a83t.dtsi
> > > @@ -1064,6 +1064,32 @@ csi: camera@1cb0000 {
> > >  			status = "disabled";
> > >  		};
> > >  
> > > +		mipi_csi2: csi@1cb1000 {
> > > +			compatible = "allwinner,sun8i-a83t-mipi-csi2";
> > > +			reg = <0x01cb1000 0x1000>;
> > > +			interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>;
> > > +			clocks = <&ccu CLK_BUS_CSI>,
> > > +				 <&ccu CLK_CSI_SCLK>,
> > > +				 <&ccu CLK_MIPI_CSI>,
> > > +				 <&ccu CLK_CSI_MISC>;
> > > +			clock-names = "bus", "mod", "mipi", "misc";
> > > +			resets = <&ccu RST_BUS_CSI>;
> > > +			status = "disabled";
> > > +
> > > +			ports {
> > > +				#address-cells = <1>;
> > > +				#size-cells = <0>;
> > > +
> > > +				mipi_csi2_in: port@0 {
> > > +					reg = <0>;
> > > +				};
> > > +
> > > +				mipi_csi2_out: port@1 {
> > > +					reg = <1>;
> > > +				};
> > > +			};
> > > +		};
> > > +
> > >  		hdmi: hdmi@1ee0000 {
> > >  			compatible = "allwinner,sun8i-a83t-dw-hdmi";
> > >  			reg = <0x01ee0000 0x10000>;

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 5/6] ARM: dts: sun8i: a83t: Add MIPI CSI-2 controller node

[Paul Kocialkowski]

[-- Attachment #1.1: Type: text/plain, Size: 3898 bytes --]

Hi Laurent,

On Thu 01 Sep 22, 17:21, Laurent Pinchart wrote:
> On Thu, Sep 01, 2022 at 04:04:40PM +0200, Paul Kocialkowski wrote:
> > Hi Laurent,
> > 
> > On Fri 26 Aug 22, 21:59, Laurent Pinchart wrote:
> > > Hi Paul,
> > > 
> > > Thank you for the patch.
> > > 
> > > On Fri, Aug 26, 2022 at 08:28:02PM +0200, Paul Kocialkowski wrote:
> > > > MIPI CSI-2 is supported on the A83T with a dedicated controller that
> > > > covers both the protocol and D-PHY. It can be connected to the CSI
> > > > interface as a V4L2 subdev through the fwnode graph.
> > > > 
> > > > This is not done by default since connecting the bridge without a
> > > > subdev attached to it will cause a failure on the CSI driver.
> > > 
> > > No urgency, but would it be possible to fix this so that the CSI-2
> > > receiver can be connected to the CSI unconditionally in DT ? The
> > > connection exists at the hardware level in the SoC, and should thus
> > > exist here too, regardless of whether or not a sensor is connected.
> > 
> > Yes it's true that having the link always would be legitimate.
> > 
> > For the context, this CSI controller can be switched between the MIPI CSI-2
> > controller and a parallel sensor input (i.e. it's not dedicated to one or the
> > other like on the V3).
> > 
> > Last time I tried, having the connection between the two always there resulted
> > in the unability to use a parallel sensor when no sensor is attached to the
> > mipi csi-2 receiver. Probably because the async notifier never completes since
> > the mipi csi-2's subdev is never registered without a sensor subdev attached.
> > 
> > Do you see a way to handle this case properly?
> 
> It sounds like an issue in the CSI-2 receiver driver. If there's no
> input device attached to it, it should register its subdev directly,
> without its own async notifier.

Yes it turns out there was an error on that side, thanks for bringing it up!
I have sent a fixup series which takes care of it.

Now it becomes possible to always describe the links without downsides.
Well, the CSI driver will still wait for the MIPI CSI-2 bridge's subdev
when its node is enabled in device-tree, but I think that is expected.

Cheers,

Paul

> > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > ---
> > > >  arch/arm/boot/dts/sun8i-a83t.dtsi | 26 ++++++++++++++++++++++++++
> > > >  1 file changed, 26 insertions(+)
> > > > 
> > > > diff --git a/arch/arm/boot/dts/sun8i-a83t.dtsi b/arch/arm/boot/dts/sun8i-a83t.dtsi
> > > > index 82fdb04122ca..ecf9f3b2c0c0 100644
> > > > --- a/arch/arm/boot/dts/sun8i-a83t.dtsi
> > > > +++ b/arch/arm/boot/dts/sun8i-a83t.dtsi
> > > > @@ -1064,6 +1064,32 @@ csi: camera@1cb0000 {
> > > >  			status = "disabled";
> > > >  		};
> > > >  
> > > > +		mipi_csi2: csi@1cb1000 {
> > > > +			compatible = "allwinner,sun8i-a83t-mipi-csi2";
> > > > +			reg = <0x01cb1000 0x1000>;
> > > > +			interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>;
> > > > +			clocks = <&ccu CLK_BUS_CSI>,
> > > > +				 <&ccu CLK_CSI_SCLK>,
> > > > +				 <&ccu CLK_MIPI_CSI>,
> > > > +				 <&ccu CLK_CSI_MISC>;
> > > > +			clock-names = "bus", "mod", "mipi", "misc";
> > > > +			resets = <&ccu RST_BUS_CSI>;
> > > > +			status = "disabled";
> > > > +
> > > > +			ports {
> > > > +				#address-cells = <1>;
> > > > +				#size-cells = <0>;
> > > > +
> > > > +				mipi_csi2_in: port@0 {
> > > > +					reg = <0>;
> > > > +				};
> > > > +
> > > > +				mipi_csi2_out: port@1 {
> > > > +					reg = <1>;
> > > > +				};
> > > > +			};
> > > > +		};
> > > > +
> > > >  		hdmi: hdmi@1ee0000 {
> > > >  			compatible = "allwinner,sun8i-a83t-dw-hdmi";
> > > >  			reg = <0x01ee0000 0x10000>;
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH NOT FOR MERGE v5 6/6] ARM: dts: sun8i: a83t: bananapi-m3: Enable MIPI CSI-2 with OV8865

[Paul Kocialkowski]

From: Kévin L'hôpital <kevin.lhopital@bootlin.com>

The Bananapi M3 supports a camera module which includes an OV8865 sensor
connected via the parallel CSI interface and an OV8865 sensor connected
via MIPI CSI-2.

The I2C2 bus is shared by the two sensors as well as the (active-low)
reset signal, but each sensor has it own shutdown line.

Signed-off-by: Kévin L'hôpital <kevin.lhopital@bootlin.com>
Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts | 102 +++++++++++++++++++
 1 file changed, 102 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts b/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
index 5a7e1bd5f825..80fd99cf24b2 100644
--- a/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
+++ b/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
@@ -85,6 +85,30 @@ led-1 {
 		};
 	};
 
+	reg_ov8865_avdd: ov8865-avdd {
+		compatible = "regulator-fixed";
+		regulator-name = "ov8865-avdd";
+		regulator-min-microvolt = <2800000>;
+		regulator-max-microvolt = <2800000>;
+		vin-supply = <&reg_dldo4>;
+	};
+
+	reg_ov8865_dovdd: ov8865-dovdd {
+		compatible = "regulator-fixed";
+		regulator-name = "ov8865-dovdd";
+		regulator-min-microvolt = <2800000>;
+		regulator-max-microvolt = <2800000>;
+		vin-supply = <&reg_dldo4>;
+	};
+
+	reg_ov8865_dvdd: ov8865-dvdd {
+		compatible = "regulator-fixed";
+		regulator-name = "ov8865-dvdd";
+		regulator-min-microvolt = <1200000>;
+		regulator-max-microvolt = <1200000>;
+		vin-supply = <&reg_eldo1>;
+	};
+
 	reg_usb1_vbus: reg-usb1-vbus {
 		compatible = "regulator-fixed";
 		regulator-name = "usb1-vbus";
@@ -115,6 +139,23 @@ &cpu100 {
 	cpu-supply = <&reg_dcdc3>;
 };
 
+&csi {
+	status = "okay";
+
+	ports {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		port@1 {
+			reg = <1>;
+
+			csi_in_mipi_csi2: endpoint {
+				remote-endpoint = <&mipi_csi2_out_csi>;
+			};
+		};
+	};
+};
+
 &de {
 	status = "okay";
 };
@@ -147,6 +188,36 @@ hdmi_out_con: endpoint {
 	};
 };
 
+&i2c2 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&i2c2_pe_pins>;
+	status = "okay";
+
+	ov8865: camera@36 {
+		compatible = "ovti,ov8865";
+		reg = <0x36>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&csi_mclk_pin>;
+		clocks = <&ccu CLK_CSI_MCLK>;
+		assigned-clocks = <&ccu CLK_CSI_MCLK>;
+		assigned-clock-rates = <24000000>;
+		avdd-supply = <&reg_ov8865_avdd>;
+		dovdd-supply = <&reg_ov8865_dovdd>;
+		dvdd-supply = <&reg_ov8865_dvdd>;
+		powerdown-gpios = <&pio 4 17 GPIO_ACTIVE_LOW>; /* PE17 */
+		reset-gpios = <&pio 4 16 GPIO_ACTIVE_LOW>; /* PE16 */
+
+		port {
+			ov8865_out_mipi_csi2: endpoint {
+				data-lanes = <1 2 3 4>;
+				link-frequencies = /bits/ 64 <360000000>;
+
+				remote-endpoint = <&mipi_csi2_in_ov8865>;
+			};
+		};
+	};
+};
+
 &mdio {
 	rgmii_phy: ethernet-phy@1 {
 		compatible = "ethernet-phy-ieee802.3-c22";
@@ -154,6 +225,24 @@ rgmii_phy: ethernet-phy@1 {
 	};
 };
 
+&mipi_csi2 {
+	status = "okay";
+};
+
+&mipi_csi2_in {
+	mipi_csi2_in_ov8865: endpoint {
+		data-lanes = <1 2 3 4>;
+
+		remote-endpoint = <&ov8865_out_mipi_csi2>;
+	};
+};
+
+&mipi_csi2_out {
+	mipi_csi2_out_csi: endpoint {
+		remote-endpoint = <&csi_in_mipi_csi2>;
+	};
+};
+
 &mmc0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&mmc0_pins>;
@@ -327,11 +416,24 @@ &reg_dldo3 {
 	regulator-name = "vcc-pd";
 };
 
+&reg_dldo4 {
+	regulator-always-on;
+	regulator-min-microvolt = <2800000>;
+	regulator-max-microvolt = <2800000>;
+	regulator-name = "avdd-csi";
+};
+
 &reg_drivevbus {
 	regulator-name = "usb0-vbus";
 	status = "okay";
 };
 
+&reg_eldo1 {
+	regulator-min-microvolt = <1200000>;
+	regulator-max-microvolt = <1200000>;
+	regulator-name = "dvdd-csi-r";
+};
+
 &reg_fldo1 {
 	regulator-min-microvolt = <1080000>;
 	regulator-max-microvolt = <1320000>;
-- 
2.37.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH NOT FOR MERGE v5 6/6] ARM: dts: sun8i: a83t: bananapi-m3: Enable MIPI CSI-2 with OV8865

[Laurent Pinchart]

Hi Paul and Kévin,

Thank you for the patch.

On Fri, Aug 26, 2022 at 08:28:03PM +0200, Paul Kocialkowski wrote:
> From: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> 
> The Bananapi M3 supports a camera module which includes an OV8865 sensor
> connected via the parallel CSI interface and an OV8865 sensor connected
> via MIPI CSI-2.
> 
> The I2C2 bus is shared by the two sensors as well as the (active-low)
> reset signal, but each sensor has it own shutdown line.

I see a single sensor in this file, am I missing something ?

Sounds like a perfect candidate for an overlay, it could then be merged
upstream.

> Signed-off-by: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts | 102 +++++++++++++++++++
>  1 file changed, 102 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts b/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
> index 5a7e1bd5f825..80fd99cf24b2 100644
> --- a/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
> +++ b/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
> @@ -85,6 +85,30 @@ led-1 {
>  		};
>  	};
>  
> +	reg_ov8865_avdd: ov8865-avdd {
> +		compatible = "regulator-fixed";
> +		regulator-name = "ov8865-avdd";
> +		regulator-min-microvolt = <2800000>;
> +		regulator-max-microvolt = <2800000>;
> +		vin-supply = <&reg_dldo4>;
> +	};
> +
> +	reg_ov8865_dovdd: ov8865-dovdd {
> +		compatible = "regulator-fixed";
> +		regulator-name = "ov8865-dovdd";
> +		regulator-min-microvolt = <2800000>;
> +		regulator-max-microvolt = <2800000>;
> +		vin-supply = <&reg_dldo4>;
> +	};
> +
> +	reg_ov8865_dvdd: ov8865-dvdd {
> +		compatible = "regulator-fixed";
> +		regulator-name = "ov8865-dvdd";
> +		regulator-min-microvolt = <1200000>;
> +		regulator-max-microvolt = <1200000>;
> +		vin-supply = <&reg_eldo1>;
> +	};

Are those three regulators on the Banana Pi, or on the camera module ?

> +
>  	reg_usb1_vbus: reg-usb1-vbus {
>  		compatible = "regulator-fixed";
>  		regulator-name = "usb1-vbus";
> @@ -115,6 +139,23 @@ &cpu100 {
>  	cpu-supply = <&reg_dcdc3>;
>  };
>  
> +&csi {
> +	status = "okay";
> +
> +	ports {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		port@1 {
> +			reg = <1>;

All of this (except the status = "okay") should go to sun8i-a83t.dtsi.

> +
> +			csi_in_mipi_csi2: endpoint {
> +				remote-endpoint = <&mipi_csi2_out_csi>;
> +			};

This too actually, once the issue mentioned in patch 5/6 gets fixed.

> +		};
> +	};
> +};
> +
>  &de {
>  	status = "okay";
>  };
> @@ -147,6 +188,36 @@ hdmi_out_con: endpoint {
>  	};
>  };
>  
> +&i2c2 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&i2c2_pe_pins>;

This looks like a candidate for upstreaming in
sun8i-a83t-bananapi-m3.dts, it shouldn't be in the overlay.

> +	status = "okay";
> +
> +	ov8865: camera@36 {
> +		compatible = "ovti,ov8865";
> +		reg = <0x36>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&csi_mclk_pin>;
> +		clocks = <&ccu CLK_CSI_MCLK>;
> +		assigned-clocks = <&ccu CLK_CSI_MCLK>;
> +		assigned-clock-rates = <24000000>;
> +		avdd-supply = <&reg_ov8865_avdd>;
> +		dovdd-supply = <&reg_ov8865_dovdd>;
> +		dvdd-supply = <&reg_ov8865_dvdd>;
> +		powerdown-gpios = <&pio 4 17 GPIO_ACTIVE_LOW>; /* PE17 */
> +		reset-gpios = <&pio 4 16 GPIO_ACTIVE_LOW>; /* PE16 */
> +
> +		port {
> +			ov8865_out_mipi_csi2: endpoint {
> +				data-lanes = <1 2 3 4>;
> +				link-frequencies = /bits/ 64 <360000000>;
> +
> +				remote-endpoint = <&mipi_csi2_in_ov8865>;
> +			};
> +		};
> +	};
> +};
> +
>  &mdio {
>  	rgmii_phy: ethernet-phy@1 {
>  		compatible = "ethernet-phy-ieee802.3-c22";
> @@ -154,6 +225,24 @@ rgmii_phy: ethernet-phy@1 {
>  	};
>  };
>  
> +&mipi_csi2 {
> +	status = "okay";
> +};
> +
> +&mipi_csi2_in {
> +	mipi_csi2_in_ov8865: endpoint {
> +		data-lanes = <1 2 3 4>;
> +
> +		remote-endpoint = <&ov8865_out_mipi_csi2>;
> +	};
> +};
> +
> +&mipi_csi2_out {
> +	mipi_csi2_out_csi: endpoint {
> +		remote-endpoint = <&csi_in_mipi_csi2>;
> +	};
> +};
> +
>  &mmc0 {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&mmc0_pins>;
> @@ -327,11 +416,24 @@ &reg_dldo3 {
>  	regulator-name = "vcc-pd";
>  };
>  
> +&reg_dldo4 {
> +	regulator-always-on;

Does it have to be always on ?

> +	regulator-min-microvolt = <2800000>;
> +	regulator-max-microvolt = <2800000>;
> +	regulator-name = "avdd-csi";

Doesn't this belong to the base board .dts ? Same below.

> +};
> +
>  &reg_drivevbus {
>  	regulator-name = "usb0-vbus";
>  	status = "okay";
>  };
>  
> +&reg_eldo1 {
> +	regulator-min-microvolt = <1200000>;
> +	regulator-max-microvolt = <1200000>;
> +	regulator-name = "dvdd-csi-r";
> +};
> +
>  &reg_fldo1 {
>  	regulator-min-microvolt = <1080000>;
>  	regulator-max-microvolt = <1320000>;

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH NOT FOR MERGE v5 6/6] ARM: dts: sun8i: a83t: bananapi-m3: Enable MIPI CSI-2 with OV8865

[Paul Kocialkowski]

[-- Attachment #1.1: Type: text/plain, Size: 8127 bytes --]

Hi Laurent,

Thansk for the review,

On Fri 26 Aug 22, 22:08, Laurent Pinchart wrote:
> Hi Paul and Kévin,
> 
> Thank you for the patch.
> 
> On Fri, Aug 26, 2022 at 08:28:03PM +0200, Paul Kocialkowski wrote:
> > From: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> > 
> > The Bananapi M3 supports a camera module which includes an OV8865 sensor
> > connected via the parallel CSI interface and an OV8865 sensor connected
> > via MIPI CSI-2.
> > 
> > The I2C2 bus is shared by the two sensors as well as the (active-low)
> > reset signal, but each sensor has it own shutdown line.
> 
> I see a single sensor in this file, am I missing something ?

Indeed this patch only adds the OV8865, not the OV5640 which is also present
on the same camera board extension.

A patch was submitted some time ago adding support for (only) the OV5640:
https://lore.kernel.org/lkml/20190408165744.11672-7-wens@kernel.org/

> Sounds like a perfect candidate for an overlay, it could then be merged
> upstream.

Do we have an upstream place to merge device-tree overlays now?

I'll check if it's possible to describe both sensors together and actually
be able to select one or the other properly from userspace. Last time I tried
this wasn't possible: there's at least the shared reset GPIO used by both
sensors, which cannot be requested by the two drivers in parallel.

To be honest this is very poor hardware design and it was almost certainly
designed with the idea that only one sensor will be configured per boot.
See https://wiki.banana-pi.org/Camera and
https://drive.google.com/file/d/0B4PAo2nW2KfnOEFTMjZ2aEVGUVU/view?usp=sharing
for the schematics pdf

> > Signed-off-by: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > ---
> >  arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts | 102 +++++++++++++++++++
> >  1 file changed, 102 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts b/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
> > index 5a7e1bd5f825..80fd99cf24b2 100644
> > --- a/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
> > +++ b/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
> > @@ -85,6 +85,30 @@ led-1 {
> >  		};
> >  	};
> >  
> > +	reg_ov8865_avdd: ov8865-avdd {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "ov8865-avdd";
> > +		regulator-min-microvolt = <2800000>;
> > +		regulator-max-microvolt = <2800000>;
> > +		vin-supply = <&reg_dldo4>;
> > +	};
> > +
> > +	reg_ov8865_dovdd: ov8865-dovdd {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "ov8865-dovdd";
> > +		regulator-min-microvolt = <2800000>;
> > +		regulator-max-microvolt = <2800000>;
> > +		vin-supply = <&reg_dldo4>;
> > +	};
> > +
> > +	reg_ov8865_dvdd: ov8865-dvdd {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "ov8865-dvdd";
> > +		regulator-min-microvolt = <1200000>;
> > +		regulator-max-microvolt = <1200000>;
> > +		vin-supply = <&reg_eldo1>;
> > +	};
> 
> Are those three regulators on the Banana Pi, or on the camera module ?

That's on the camera module.

> > +
> >  	reg_usb1_vbus: reg-usb1-vbus {
> >  		compatible = "regulator-fixed";
> >  		regulator-name = "usb1-vbus";
> > @@ -115,6 +139,23 @@ &cpu100 {
> >  	cpu-supply = <&reg_dcdc3>;
> >  };
> >  
> > +&csi {
> > +	status = "okay";
> > +
> > +	ports {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		port@1 {
> > +			reg = <1>;
> 
> All of this (except the status = "okay") should go to sun8i-a83t.dtsi.
> 
> > +
> > +			csi_in_mipi_csi2: endpoint {
> > +				remote-endpoint = <&mipi_csi2_out_csi>;
> > +			};
> 
> This too actually, once the issue mentioned in patch 5/6 gets fixed.
> 
> > +		};
> > +	};
> > +};
> > +
> >  &de {
> >  	status = "okay";
> >  };
> > @@ -147,6 +188,36 @@ hdmi_out_con: endpoint {
> >  	};
> >  };
> >  
> > +&i2c2 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&i2c2_pe_pins>;
> 
> This looks like a candidate for upstreaming in
> sun8i-a83t-bananapi-m3.dts, it shouldn't be in the overlay.

I2C2 is actually also exported in the PH pins, which is available on the board
header, so it's not obvious that using the PE pins should be the default.

The context is that Allwinner SoCs usually have a dedicated I2C controller for
cameras, but also route a "generic" I2C controller on the same pins.
Here that's on the PE14/15 pins. Since we don't have mainline support for this
camera-dedicated I2C controller, we end up routing the generic one to the PE
pins, which are routed to the camera connector.

In the future we could add support for this camera-dedicated controller, which
would then allow routing i2c2 to the PH pins independently. This is what the
downstream Allwinner BSP kernel does.

> > +	status = "okay";
> > +
> > +	ov8865: camera@36 {
> > +		compatible = "ovti,ov8865";
> > +		reg = <0x36>;
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&csi_mclk_pin>;
> > +		clocks = <&ccu CLK_CSI_MCLK>;
> > +		assigned-clocks = <&ccu CLK_CSI_MCLK>;
> > +		assigned-clock-rates = <24000000>;
> > +		avdd-supply = <&reg_ov8865_avdd>;
> > +		dovdd-supply = <&reg_ov8865_dovdd>;
> > +		dvdd-supply = <&reg_ov8865_dvdd>;
> > +		powerdown-gpios = <&pio 4 17 GPIO_ACTIVE_LOW>; /* PE17 */
> > +		reset-gpios = <&pio 4 16 GPIO_ACTIVE_LOW>; /* PE16 */
> > +
> > +		port {
> > +			ov8865_out_mipi_csi2: endpoint {
> > +				data-lanes = <1 2 3 4>;
> > +				link-frequencies = /bits/ 64 <360000000>;
> > +
> > +				remote-endpoint = <&mipi_csi2_in_ov8865>;
> > +			};
> > +		};
> > +	};
> > +};
> > +
> >  &mdio {
> >  	rgmii_phy: ethernet-phy@1 {
> >  		compatible = "ethernet-phy-ieee802.3-c22";
> > @@ -154,6 +225,24 @@ rgmii_phy: ethernet-phy@1 {
> >  	};
> >  };
> >  
> > +&mipi_csi2 {
> > +	status = "okay";
> > +};
> > +
> > +&mipi_csi2_in {
> > +	mipi_csi2_in_ov8865: endpoint {
> > +		data-lanes = <1 2 3 4>;
> > +
> > +		remote-endpoint = <&ov8865_out_mipi_csi2>;
> > +	};
> > +};
> > +
> > +&mipi_csi2_out {
> > +	mipi_csi2_out_csi: endpoint {
> > +		remote-endpoint = <&csi_in_mipi_csi2>;
> > +	};
> > +};
> > +
> >  &mmc0 {
> >  	pinctrl-names = "default";
> >  	pinctrl-0 = <&mmc0_pins>;
> > @@ -327,11 +416,24 @@ &reg_dldo3 {
> >  	regulator-name = "vcc-pd";
> >  };
> >  
> > +&reg_dldo4 {
> > +	regulator-always-on;
> 
> Does it have to be always on ?

Mhh so I realize the regulators handling here is quite messy.
I guess I didn't do such a good review of this patch internally.

The situation is that the regulators on the camera board all have their
enable pin tied to the DLDO4 output, but that would be best described as
a vin-supply of the ov8865 regulators above. Their real vin supply is an
always-on board-wide power source but I don't think we can represent another
regulator acting as enable signal in a better way.

Now beware that the camera board schematics are confusing as they show resistors
(R17, R18, R19, R20, R23) connecting some power lines together, but they are not
populated on the board (I guess the point is to make a variant of the design
without regulators on the camera board and to use ones from the PMU instead).
This probably confused Kevin and I back when this patch was made.

> > +	regulator-min-microvolt = <2800000>;
> > +	regulator-max-microvolt = <2800000>;
> > +	regulator-name = "avdd-csi";
> 
> Doesn't this belong to the base board .dts ? Same below.
> 
> > +};
> > +
> >  &reg_drivevbus {
> >  	regulator-name = "usb0-vbus";AVDD-CSI
> >  	status = "okay";
> >  };
> >  
> > +&reg_eldo1 {
> > +	regulator-min-microvolt = <1200000>;
> > +	regulator-max-microvolt = <1200000>;
> > +	regulator-name = "dvdd-csi-r";
> > +};
> > +
> >  &reg_fldo1 {
> >  	regulator-min-microvolt = <1080000>;
> >  	regulator-max-microvolt = <1320000>;
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH NOT FOR MERGE v5 6/6] ARM: dts: sun8i: a83t: bananapi-m3: Enable MIPI CSI-2 with OV8865

[Laurent Pinchart]

On Thu, Sep 01, 2022 at 02:49:43PM +0200, Paul Kocialkowski wrote:
> On Fri 26 Aug 22, 22:08, Laurent Pinchart wrote:
> > On Fri, Aug 26, 2022 at 08:28:03PM +0200, Paul Kocialkowski wrote:
> > > From: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> > > 
> > > The Bananapi M3 supports a camera module which includes an OV8865 sensor
> > > connected via the parallel CSI interface and an OV8865 sensor connected
> > > via MIPI CSI-2.
> > > 
> > > The I2C2 bus is shared by the two sensors as well as the (active-low)
> > > reset signal, but each sensor has it own shutdown line.
> > 
> > I see a single sensor in this file, am I missing something ?
> 
> Indeed this patch only adds the OV8865, not the OV5640 which is also present
> on the same camera board extension.
> 
> A patch was submitted some time ago adding support for (only) the OV5640:
> https://lore.kernel.org/lkml/20190408165744.11672-7-wens@kernel.org/

OK. That's fine, let's just reword the commit message.

> > Sounds like a perfect candidate for an overlay, it could then be merged
> > upstream.
> 
> Do we have an upstream place to merge device-tree overlays now?

They're accepted in the upstream kernel as far as I know, a git grep for
/plugin/ in .dts files produces 18 matches.

> I'll check if it's possible to describe both sensors together and actually
> be able to select one or the other properly from userspace. Last time I tried
> this wasn't possible: there's at least the shared reset GPIO used by both
> sensors, which cannot be requested by the two drivers in parallel.
> 
> To be honest this is very poor hardware design and it was almost certainly
> designed with the idea that only one sensor will be configured per boot.
> See https://wiki.banana-pi.org/Camera and
> https://drive.google.com/file/d/0B4PAo2nW2KfnOEFTMjZ2aEVGUVU/view?usp=sharing
> for the schematics pdf

It's not great indeed, but not that uncommon either (unfortunately). I
wonder if we need some kind of reset GPIO API, but that would be a hack
at most I think.

> > > Signed-off-by: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > ---
> > >  arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts | 102 +++++++++++++++++++
> > >  1 file changed, 102 insertions(+)
> > > 
> > > diff --git a/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts b/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
> > > index 5a7e1bd5f825..80fd99cf24b2 100644
> > > --- a/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
> > > +++ b/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
> > > @@ -85,6 +85,30 @@ led-1 {
> > >  		};
> > >  	};
> > >  
> > > +	reg_ov8865_avdd: ov8865-avdd {
> > > +		compatible = "regulator-fixed";
> > > +		regulator-name = "ov8865-avdd";
> > > +		regulator-min-microvolt = <2800000>;
> > > +		regulator-max-microvolt = <2800000>;
> > > +		vin-supply = <&reg_dldo4>;
> > > +	};
> > > +
> > > +	reg_ov8865_dovdd: ov8865-dovdd {
> > > +		compatible = "regulator-fixed";
> > > +		regulator-name = "ov8865-dovdd";
> > > +		regulator-min-microvolt = <2800000>;
> > > +		regulator-max-microvolt = <2800000>;
> > > +		vin-supply = <&reg_dldo4>;
> > > +	};
> > > +
> > > +	reg_ov8865_dvdd: ov8865-dvdd {
> > > +		compatible = "regulator-fixed";
> > > +		regulator-name = "ov8865-dvdd";
> > > +		regulator-min-microvolt = <1200000>;
> > > +		regulator-max-microvolt = <1200000>;
> > > +		vin-supply = <&reg_eldo1>;
> > > +	};
> > 
> > Are those three regulators on the Banana Pi, or on the camera module ?
> 
> That's on the camera module.
> 
> > > +
> > >  	reg_usb1_vbus: reg-usb1-vbus {
> > >  		compatible = "regulator-fixed";
> > >  		regulator-name = "usb1-vbus";
> > > @@ -115,6 +139,23 @@ &cpu100 {
> > >  	cpu-supply = <&reg_dcdc3>;
> > >  };
> > >  
> > > +&csi {
> > > +	status = "okay";
> > > +
> > > +	ports {
> > > +		#address-cells = <1>;
> > > +		#size-cells = <0>;
> > > +
> > > +		port@1 {
> > > +			reg = <1>;
> > 
> > All of this (except the status = "okay") should go to sun8i-a83t.dtsi.
> > 
> > > +
> > > +			csi_in_mipi_csi2: endpoint {
> > > +				remote-endpoint = <&mipi_csi2_out_csi>;
> > > +			};
> > 
> > This too actually, once the issue mentioned in patch 5/6 gets fixed.
> > 
> > > +		};
> > > +	};
> > > +};
> > > +
> > >  &de {
> > >  	status = "okay";
> > >  };
> > > @@ -147,6 +188,36 @@ hdmi_out_con: endpoint {
> > >  	};
> > >  };
> > >  
> > > +&i2c2 {
> > > +	pinctrl-names = "default";
> > > +	pinctrl-0 = <&i2c2_pe_pins>;
> > 
> > This looks like a candidate for upstreaming in
> > sun8i-a83t-bananapi-m3.dts, it shouldn't be in the overlay.
> 
> I2C2 is actually also exported in the PH pins, which is available on the board
> header, so it's not obvious that using the PE pins should be the default.
> 
> The context is that Allwinner SoCs usually have a dedicated I2C controller for
> cameras, but also route a "generic" I2C controller on the same pins.

Out of curiosity, what features do those dedicated camera I2C
controllers provide, compared to "normal" I2C controllers ?

> Here that's on the PE14/15 pins. Since we don't have mainline support for this
> camera-dedicated I2C controller, we end up routing the generic one to the PE
> pins, which are routed to the camera connector.

OK.

> In the future we could add support for this camera-dedicated controller, which
> would then allow routing i2c2 to the PH pins independently. This is what the
> downstream Allwinner BSP kernel does.
> 
> > > +	status = "okay";
> > > +
> > > +	ov8865: camera@36 {
> > > +		compatible = "ovti,ov8865";
> > > +		reg = <0x36>;
> > > +		pinctrl-names = "default";
> > > +		pinctrl-0 = <&csi_mclk_pin>;
> > > +		clocks = <&ccu CLK_CSI_MCLK>;
> > > +		assigned-clocks = <&ccu CLK_CSI_MCLK>;
> > > +		assigned-clock-rates = <24000000>;
> > > +		avdd-supply = <&reg_ov8865_avdd>;
> > > +		dovdd-supply = <&reg_ov8865_dovdd>;
> > > +		dvdd-supply = <&reg_ov8865_dvdd>;
> > > +		powerdown-gpios = <&pio 4 17 GPIO_ACTIVE_LOW>; /* PE17 */
> > > +		reset-gpios = <&pio 4 16 GPIO_ACTIVE_LOW>; /* PE16 */
> > > +
> > > +		port {
> > > +			ov8865_out_mipi_csi2: endpoint {
> > > +				data-lanes = <1 2 3 4>;
> > > +				link-frequencies = /bits/ 64 <360000000>;
> > > +
> > > +				remote-endpoint = <&mipi_csi2_in_ov8865>;
> > > +			};
> > > +		};
> > > +	};
> > > +};
> > > +
> > >  &mdio {
> > >  	rgmii_phy: ethernet-phy@1 {
> > >  		compatible = "ethernet-phy-ieee802.3-c22";
> > > @@ -154,6 +225,24 @@ rgmii_phy: ethernet-phy@1 {
> > >  	};
> > >  };
> > >  
> > > +&mipi_csi2 {
> > > +	status = "okay";
> > > +};
> > > +
> > > +&mipi_csi2_in {
> > > +	mipi_csi2_in_ov8865: endpoint {
> > > +		data-lanes = <1 2 3 4>;
> > > +
> > > +		remote-endpoint = <&ov8865_out_mipi_csi2>;
> > > +	};
> > > +};
> > > +
> > > +&mipi_csi2_out {
> > > +	mipi_csi2_out_csi: endpoint {
> > > +		remote-endpoint = <&csi_in_mipi_csi2>;
> > > +	};
> > > +};
> > > +
> > >  &mmc0 {
> > >  	pinctrl-names = "default";
> > >  	pinctrl-0 = <&mmc0_pins>;
> > > @@ -327,11 +416,24 @@ &reg_dldo3 {
> > >  	regulator-name = "vcc-pd";
> > >  };
> > >  
> > > +&reg_dldo4 {
> > > +	regulator-always-on;
> > 
> > Does it have to be always on ?
> 
> Mhh so I realize the regulators handling here is quite messy.
> I guess I didn't do such a good review of this patch internally.
> 
> The situation is that the regulators on the camera board all have their
> enable pin tied to the DLDO4 output, but that would be best described as
> a vin-supply of the ov8865 regulators above. Their real vin supply is an
> always-on board-wide power source but I don't think we can represent another
> regulator acting as enable signal in a better way.

That's right. I've modeled that as a parent regulator in the past, even
if it doesn't reflect the exact hardware topology, it's functionally
equivalent.

> Now beware that the camera board schematics are confusing as they show resistors
> (R17, R18, R19, R20, R23) connecting some power lines together, but they are not
> populated on the board (I guess the point is to make a variant of the design
> without regulators on the camera board and to use ones from the PMU instead).
> This probably confused Kevin and I back when this patch was made.
> 
> > > +	regulator-min-microvolt = <2800000>;
> > > +	regulator-max-microvolt = <2800000>;
> > > +	regulator-name = "avdd-csi";
> > 
> > Doesn't this belong to the base board .dts ? Same below.
> > 
> > > +};
> > > +
> > >  &reg_drivevbus {
> > >  	regulator-name = "usb0-vbus";AVDD-CSI
> > >  	status = "okay";
> > >  };
> > >  
> > > +&reg_eldo1 {
> > > +	regulator-min-microvolt = <1200000>;
> > > +	regulator-max-microvolt = <1200000>;
> > > +	regulator-name = "dvdd-csi-r";
> > > +};
> > > +
> > >  &reg_fldo1 {
> > >  	regulator-min-microvolt = <1080000>;
> > >  	regulator-max-microvolt = <1320000>;

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH NOT FOR MERGE v5 6/6] ARM: dts: sun8i: a83t: bananapi-m3: Enable MIPI CSI-2 with OV8865

[Paul Kocialkowski]

[-- Attachment #1.1: Type: text/plain, Size: 10334 bytes --]

Hi Laurent,

On Thu 01 Sep 22, 16:00, Laurent Pinchart wrote:
> On Thu, Sep 01, 2022 at 02:49:43PM +0200, Paul Kocialkowski wrote:
> > On Fri 26 Aug 22, 22:08, Laurent Pinchart wrote:
> > > On Fri, Aug 26, 2022 at 08:28:03PM +0200, Paul Kocialkowski wrote:
> > > > From: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> > > > 
> > > > The Bananapi M3 supports a camera module which includes an OV8865 sensor
> > > > connected via the parallel CSI interface and an OV8865 sensor connected
> > > > via MIPI CSI-2.
> > > > 
> > > > The I2C2 bus is shared by the two sensors as well as the (active-low)
> > > > reset signal, but each sensor has it own shutdown line.
> > > 
> > > I see a single sensor in this file, am I missing something ?
> > 
> > Indeed this patch only adds the OV8865, not the OV5640 which is also present
> > on the same camera board extension.
> > 
> > A patch was submitted some time ago adding support for (only) the OV5640:
> > https://lore.kernel.org/lkml/20190408165744.11672-7-wens@kernel.org/
> 
> OK. That's fine, let's just reword the commit message.

Sure.

> > > Sounds like a perfect candidate for an overlay, it could then be merged
> > > upstream.
> > 
> > Do we have an upstream place to merge device-tree overlays now?
> 
> They're accepted in the upstream kernel as far as I know, a git grep for
> /plugin/ in .dts files produces 18 matches.

Oh okay, definitely good to know thanks!

> > I'll check if it's possible to describe both sensors together and actually
> > be able to select one or the other properly from userspace. Last time I tried
> > this wasn't possible: there's at least the shared reset GPIO used by both
> > sensors, which cannot be requested by the two drivers in parallel.
> > 
> > To be honest this is very poor hardware design and it was almost certainly
> > designed with the idea that only one sensor will be configured per boot.
> > See https://wiki.banana-pi.org/Camera and
> > https://drive.google.com/file/d/0B4PAo2nW2KfnOEFTMjZ2aEVGUVU/view?usp=sharing
> > for the schematics pdf
> 
> It's not great indeed, but not that uncommon either (unfortunately). I
> wonder if we need some kind of reset GPIO API, but that would be a hack
> at most I think.

Yeah I don't see any obvious clean solution here.
But that could still be two separate dt overlays for now.

> > > > Signed-off-by: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > ---
> > > >  arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts | 102 +++++++++++++++++++
> > > >  1 file changed, 102 insertions(+)
> > > > 
> > > > diff --git a/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts b/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
> > > > index 5a7e1bd5f825..80fd99cf24b2 100644
> > > > --- a/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
> > > > +++ b/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
> > > > @@ -85,6 +85,30 @@ led-1 {
> > > >  		};
> > > >  	};
> > > >  
> > > > +	reg_ov8865_avdd: ov8865-avdd {
> > > > +		compatible = "regulator-fixed";
> > > > +		regulator-name = "ov8865-avdd";
> > > > +		regulator-min-microvolt = <2800000>;
> > > > +		regulator-max-microvolt = <2800000>;
> > > > +		vin-supply = <&reg_dldo4>;
> > > > +	};
> > > > +
> > > > +	reg_ov8865_dovdd: ov8865-dovdd {
> > > > +		compatible = "regulator-fixed";
> > > > +		regulator-name = "ov8865-dovdd";
> > > > +		regulator-min-microvolt = <2800000>;
> > > > +		regulator-max-microvolt = <2800000>;
> > > > +		vin-supply = <&reg_dldo4>;
> > > > +	};
> > > > +
> > > > +	reg_ov8865_dvdd: ov8865-dvdd {
> > > > +		compatible = "regulator-fixed";
> > > > +		regulator-name = "ov8865-dvdd";
> > > > +		regulator-min-microvolt = <1200000>;
> > > > +		regulator-max-microvolt = <1200000>;
> > > > +		vin-supply = <&reg_eldo1>;
> > > > +	};
> > > 
> > > Are those three regulators on the Banana Pi, or on the camera module ?
> > 
> > That's on the camera module.
> > 
> > > > +
> > > >  	reg_usb1_vbus: reg-usb1-vbus {
> > > >  		compatible = "regulator-fixed";
> > > >  		regulator-name = "usb1-vbus";
> > > > @@ -115,6 +139,23 @@ &cpu100 {
> > > >  	cpu-supply = <&reg_dcdc3>;
> > > >  };
> > > >  
> > > > +&csi {
> > > > +	status = "okay";
> > > > +
> > > > +	ports {
> > > > +		#address-cells = <1>;
> > > > +		#size-cells = <0>;
> > > > +
> > > > +		port@1 {
> > > > +			reg = <1>;
> > > 
> > > All of this (except the status = "okay") should go to sun8i-a83t.dtsi.
> > > 
> > > > +
> > > > +			csi_in_mipi_csi2: endpoint {
> > > > +				remote-endpoint = <&mipi_csi2_out_csi>;
> > > > +			};
> > > 
> > > This too actually, once the issue mentioned in patch 5/6 gets fixed.
> > > 
> > > > +		};
> > > > +	};
> > > > +};
> > > > +
> > > >  &de {
> > > >  	status = "okay";
> > > >  };
> > > > @@ -147,6 +188,36 @@ hdmi_out_con: endpoint {
> > > >  	};
> > > >  };
> > > >  
> > > > +&i2c2 {
> > > > +	pinctrl-names = "default";
> > > > +	pinctrl-0 = <&i2c2_pe_pins>;
> > > 
> > > This looks like a candidate for upstreaming in
> > > sun8i-a83t-bananapi-m3.dts, it shouldn't be in the overlay.
> > 
> > I2C2 is actually also exported in the PH pins, which is available on the board
> > header, so it's not obvious that using the PE pins should be the default.
> > 
> > The context is that Allwinner SoCs usually have a dedicated I2C controller for
> > cameras, but also route a "generic" I2C controller on the same pins.
> 
> Out of curiosity, what features do those dedicated camera I2C
> controllers provide, compared to "normal" I2C controllers ?

IIRC there's some feature to send i2c messages synced with the camera interface
vsync signal, to have some kind of hardware atomic mechanism for reconfiguring
a sensor.

Not sure that's very relevant for us and an implementation for it would probably
just support regular I2C. We can probably achieve simialr results with the
request API.

> > Here that's on the PE14/15 pins. Since we don't have mainline support for this
> > camera-dedicated I2C controller, we end up routing the generic one to the PE
> > pins, which are routed to the camera connector.
> 
> OK.
> 
> > In the future we could add support for this camera-dedicated controller, which
> > would then allow routing i2c2 to the PH pins independently. This is what the
> > downstream Allwinner BSP kernel does.
> > 
> > > > +	status = "okay";
> > > > +
> > > > +	ov8865: camera@36 {
> > > > +		compatible = "ovti,ov8865";
> > > > +		reg = <0x36>;
> > > > +		pinctrl-names = "default";
> > > > +		pinctrl-0 = <&csi_mclk_pin>;
> > > > +		clocks = <&ccu CLK_CSI_MCLK>;
> > > > +		assigned-clocks = <&ccu CLK_CSI_MCLK>;
> > > > +		assigned-clock-rates = <24000000>;
> > > > +		avdd-supply = <&reg_ov8865_avdd>;
> > > > +		dovdd-supply = <&reg_ov8865_dovdd>;
> > > > +		dvdd-supply = <&reg_ov8865_dvdd>;
> > > > +		powerdown-gpios = <&pio 4 17 GPIO_ACTIVE_LOW>; /* PE17 */
> > > > +		reset-gpios = <&pio 4 16 GPIO_ACTIVE_LOW>; /* PE16 */
> > > > +
> > > > +		port {
> > > > +			ov8865_out_mipi_csi2: endpoint {
> > > > +				data-lanes = <1 2 3 4>;
> > > > +				link-frequencies = /bits/ 64 <360000000>;
> > > > +
> > > > +				remote-endpoint = <&mipi_csi2_in_ov8865>;
> > > > +			};
> > > > +		};
> > > > +	};
> > > > +};
> > > > +
> > > >  &mdio {
> > > >  	rgmii_phy: ethernet-phy@1 {
> > > >  		compatible = "ethernet-phy-ieee802.3-c22";
> > > > @@ -154,6 +225,24 @@ rgmii_phy: ethernet-phy@1 {
> > > >  	};
> > > >  };
> > > >  
> > > > +&mipi_csi2 {
> > > > +	status = "okay";
> > > > +};
> > > > +
> > > > +&mipi_csi2_in {
> > > > +	mipi_csi2_in_ov8865: endpoint {
> > > > +		data-lanes = <1 2 3 4>;
> > > > +
> > > > +		remote-endpoint = <&ov8865_out_mipi_csi2>;
> > > > +	};
> > > > +};
> > > > +
> > > > +&mipi_csi2_out {
> > > > +	mipi_csi2_out_csi: endpoint {
> > > > +		remote-endpoint = <&csi_in_mipi_csi2>;
> > > > +	};
> > > > +};
> > > > +
> > > >  &mmc0 {
> > > >  	pinctrl-names = "default";
> > > >  	pinctrl-0 = <&mmc0_pins>;
> > > > @@ -327,11 +416,24 @@ &reg_dldo3 {
> > > >  	regulator-name = "vcc-pd";
> > > >  };
> > > >  
> > > > +&reg_dldo4 {
> > > > +	regulator-always-on;
> > > 
> > > Does it have to be always on ?
> > 
> > Mhh so I realize the regulators handling here is quite messy.
> > I guess I didn't do such a good review of this patch internally.
> > 
> > The situation is that the regulators on the camera board all have their
> > enable pin tied to the DLDO4 output, but that would be best described as
> > a vin-supply of the ov8865 regulators above. Their real vin supply is an
> > always-on board-wide power source but I don't think we can represent another
> > regulator acting as enable signal in a better way.
> 
> That's right. I've modeled that as a parent regulator in the past, even
> if it doesn't reflect the exact hardware topology, it's functionally
> equivalent.

Sounds good, I'll change it that way.

Cheers,

Paul

> > Now beware that the camera board schematics are confusing as they show resistors
> > (R17, R18, R19, R20, R23) connecting some power lines together, but they are not
> > populated on the board (I guess the point is to make a variant of the design
> > without regulators on the camera board and to use ones from the PMU instead).
> > This probably confused Kevin and I back when this patch was made.
> > 
> > > > +	regulator-min-microvolt = <2800000>;
> > > > +	regulator-max-microvolt = <2800000>;
> > > > +	regulator-name = "avdd-csi";
> > > 
> > > Doesn't this belong to the base board .dts ? Same below.
> > > 
> > > > +};
> > > > +
> > > >  &reg_drivevbus {
> > > >  	regulator-name = "usb0-vbus";AVDD-CSI
> > > >  	status = "okay";
> > > >  };
> > > >  
> > > > +&reg_eldo1 {
> > > > +	regulator-min-microvolt = <1200000>;
> > > > +	regulator-max-microvolt = <1200000>;
> > > > +	regulator-name = "dvdd-csi-r";
> > > > +};
> > > > +
> > > >  &reg_fldo1 {
> > > >  	regulator-min-microvolt = <1080000>;
> > > >  	regulator-max-microvolt = <1320000>;
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH NOT FOR MERGE v5 6/6] ARM: dts: sun8i: a83t: bananapi-m3: Enable MIPI CSI-2 with OV8865

[Dave Stevenson]

Hi Laurent and Paul

On Thu, 1 Sept 2022 at 14:23, Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> Hi Laurent,
>
> On Thu 01 Sep 22, 16:00, Laurent Pinchart wrote:
> > On Thu, Sep 01, 2022 at 02:49:43PM +0200, Paul Kocialkowski wrote:
> > > On Fri 26 Aug 22, 22:08, Laurent Pinchart wrote:
> > > > On Fri, Aug 26, 2022 at 08:28:03PM +0200, Paul Kocialkowski wrote:
> > > > > From: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> > > > >
> > > > > The Bananapi M3 supports a camera module which includes an OV8865 sensor
> > > > > connected via the parallel CSI interface and an OV8865 sensor connected
> > > > > via MIPI CSI-2.
> > > > >
> > > > > The I2C2 bus is shared by the two sensors as well as the (active-low)
> > > > > reset signal, but each sensor has it own shutdown line.
> > > >
> > > > I see a single sensor in this file, am I missing something ?
> > >
> > > Indeed this patch only adds the OV8865, not the OV5640 which is also present
> > > on the same camera board extension.
> > >
> > > A patch was submitted some time ago adding support for (only) the OV5640:
> > > https://lore.kernel.org/lkml/20190408165744.11672-7-wens@kernel.org/
> >
> > OK. That's fine, let's just reword the commit message.
>
> Sure.
>
> > > > Sounds like a perfect candidate for an overlay, it could then be merged
> > > > upstream.
> > >
> > > Do we have an upstream place to merge device-tree overlays now?
> >
> > They're accepted in the upstream kernel as far as I know, a git grep for
> > /plugin/ in .dts files produces 18 matches.
>
> Oh okay, definitely good to know thanks!
>
> > > I'll check if it's possible to describe both sensors together and actually
> > > be able to select one or the other properly from userspace. Last time I tried
> > > this wasn't possible: there's at least the shared reset GPIO used by both
> > > sensors, which cannot be requested by the two drivers in parallel.
> > >
> > > To be honest this is very poor hardware design and it was almost certainly
> > > designed with the idea that only one sensor will be configured per boot.
> > > See https://wiki.banana-pi.org/Camera and
> > > https://drive.google.com/file/d/0B4PAo2nW2KfnOEFTMjZ2aEVGUVU/view?usp=sharing
> > > for the schematics pdf
> >
> > It's not great indeed, but not that uncommon either (unfortunately). I
> > wonder if we need some kind of reset GPIO API, but that would be a hack
> > at most I think.
>
> Yeah I don't see any obvious clean solution here.
> But that could still be two separate dt overlays for now.

Just to throw the idea out there, (ab)use the regulator API so that
they share a regulator-gpio device that claims that GPIO.
When either sensor requests the regulator, the reset line will get
pulled to the appropriate state.

  Dave

> > > > > Signed-off-by: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > ---
> > > > >  arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts | 102 +++++++++++++++++++
> > > > >  1 file changed, 102 insertions(+)
> > > > >
> > > > > diff --git a/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts b/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
> > > > > index 5a7e1bd5f825..80fd99cf24b2 100644
> > > > > --- a/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
> > > > > +++ b/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
> > > > > @@ -85,6 +85,30 @@ led-1 {
> > > > >                 };
> > > > >         };
> > > > >
> > > > > +       reg_ov8865_avdd: ov8865-avdd {
> > > > > +               compatible = "regulator-fixed";
> > > > > +               regulator-name = "ov8865-avdd";
> > > > > +               regulator-min-microvolt = <2800000>;
> > > > > +               regulator-max-microvolt = <2800000>;
> > > > > +               vin-supply = <&reg_dldo4>;
> > > > > +       };
> > > > > +
> > > > > +       reg_ov8865_dovdd: ov8865-dovdd {
> > > > > +               compatible = "regulator-fixed";
> > > > > +               regulator-name = "ov8865-dovdd";
> > > > > +               regulator-min-microvolt = <2800000>;
> > > > > +               regulator-max-microvolt = <2800000>;
> > > > > +               vin-supply = <&reg_dldo4>;
> > > > > +       };
> > > > > +
> > > > > +       reg_ov8865_dvdd: ov8865-dvdd {
> > > > > +               compatible = "regulator-fixed";
> > > > > +               regulator-name = "ov8865-dvdd";
> > > > > +               regulator-min-microvolt = <1200000>;
> > > > > +               regulator-max-microvolt = <1200000>;
> > > > > +               vin-supply = <&reg_eldo1>;
> > > > > +       };
> > > >
> > > > Are those three regulators on the Banana Pi, or on the camera module ?
> > >
> > > That's on the camera module.
> > >
> > > > > +
> > > > >         reg_usb1_vbus: reg-usb1-vbus {
> > > > >                 compatible = "regulator-fixed";
> > > > >                 regulator-name = "usb1-vbus";
> > > > > @@ -115,6 +139,23 @@ &cpu100 {
> > > > >         cpu-supply = <&reg_dcdc3>;
> > > > >  };
> > > > >
> > > > > +&csi {
> > > > > +       status = "okay";
> > > > > +
> > > > > +       ports {
> > > > > +               #address-cells = <1>;
> > > > > +               #size-cells = <0>;
> > > > > +
> > > > > +               port@1 {
> > > > > +                       reg = <1>;
> > > >
> > > > All of this (except the status = "okay") should go to sun8i-a83t.dtsi.
> > > >
> > > > > +
> > > > > +                       csi_in_mipi_csi2: endpoint {
> > > > > +                               remote-endpoint = <&mipi_csi2_out_csi>;
> > > > > +                       };
> > > >
> > > > This too actually, once the issue mentioned in patch 5/6 gets fixed.
> > > >
> > > > > +               };
> > > > > +       };
> > > > > +};
> > > > > +
> > > > >  &de {
> > > > >         status = "okay";
> > > > >  };
> > > > > @@ -147,6 +188,36 @@ hdmi_out_con: endpoint {
> > > > >         };
> > > > >  };
> > > > >
> > > > > +&i2c2 {
> > > > > +       pinctrl-names = "default";
> > > > > +       pinctrl-0 = <&i2c2_pe_pins>;
> > > >
> > > > This looks like a candidate for upstreaming in
> > > > sun8i-a83t-bananapi-m3.dts, it shouldn't be in the overlay.
> > >
> > > I2C2 is actually also exported in the PH pins, which is available on the board
> > > header, so it's not obvious that using the PE pins should be the default.
> > >
> > > The context is that Allwinner SoCs usually have a dedicated I2C controller for
> > > cameras, but also route a "generic" I2C controller on the same pins.
> >
> > Out of curiosity, what features do those dedicated camera I2C
> > controllers provide, compared to "normal" I2C controllers ?
>
> IIRC there's some feature to send i2c messages synced with the camera interface
> vsync signal, to have some kind of hardware atomic mechanism for reconfiguring
> a sensor.
>
> Not sure that's very relevant for us and an implementation for it would probably
> just support regular I2C. We can probably achieve simialr results with the
> request API.
>
> > > Here that's on the PE14/15 pins. Since we don't have mainline support for this
> > > camera-dedicated I2C controller, we end up routing the generic one to the PE
> > > pins, which are routed to the camera connector.
> >
> > OK.
> >
> > > In the future we could add support for this camera-dedicated controller, which
> > > would then allow routing i2c2 to the PH pins independently. This is what the
> > > downstream Allwinner BSP kernel does.
> > >
> > > > > +       status = "okay";
> > > > > +
> > > > > +       ov8865: camera@36 {
> > > > > +               compatible = "ovti,ov8865";
> > > > > +               reg = <0x36>;
> > > > > +               pinctrl-names = "default";
> > > > > +               pinctrl-0 = <&csi_mclk_pin>;
> > > > > +               clocks = <&ccu CLK_CSI_MCLK>;
> > > > > +               assigned-clocks = <&ccu CLK_CSI_MCLK>;
> > > > > +               assigned-clock-rates = <24000000>;
> > > > > +               avdd-supply = <&reg_ov8865_avdd>;
> > > > > +               dovdd-supply = <&reg_ov8865_dovdd>;
> > > > > +               dvdd-supply = <&reg_ov8865_dvdd>;
> > > > > +               powerdown-gpios = <&pio 4 17 GPIO_ACTIVE_LOW>; /* PE17 */
> > > > > +               reset-gpios = <&pio 4 16 GPIO_ACTIVE_LOW>; /* PE16 */
> > > > > +
> > > > > +               port {
> > > > > +                       ov8865_out_mipi_csi2: endpoint {
> > > > > +                               data-lanes = <1 2 3 4>;
> > > > > +                               link-frequencies = /bits/ 64 <360000000>;
> > > > > +
> > > > > +                               remote-endpoint = <&mipi_csi2_in_ov8865>;
> > > > > +                       };
> > > > > +               };
> > > > > +       };
> > > > > +};
> > > > > +
> > > > >  &mdio {
> > > > >         rgmii_phy: ethernet-phy@1 {
> > > > >                 compatible = "ethernet-phy-ieee802.3-c22";
> > > > > @@ -154,6 +225,24 @@ rgmii_phy: ethernet-phy@1 {
> > > > >         };
> > > > >  };
> > > > >
> > > > > +&mipi_csi2 {
> > > > > +       status = "okay";
> > > > > +};
> > > > > +
> > > > > +&mipi_csi2_in {
> > > > > +       mipi_csi2_in_ov8865: endpoint {
> > > > > +               data-lanes = <1 2 3 4>;
> > > > > +
> > > > > +               remote-endpoint = <&ov8865_out_mipi_csi2>;
> > > > > +       };
> > > > > +};
> > > > > +
> > > > > +&mipi_csi2_out {
> > > > > +       mipi_csi2_out_csi: endpoint {
> > > > > +               remote-endpoint = <&csi_in_mipi_csi2>;
> > > > > +       };
> > > > > +};
> > > > > +
> > > > >  &mmc0 {
> > > > >         pinctrl-names = "default";
> > > > >         pinctrl-0 = <&mmc0_pins>;
> > > > > @@ -327,11 +416,24 @@ &reg_dldo3 {
> > > > >         regulator-name = "vcc-pd";
> > > > >  };
> > > > >
> > > > > +&reg_dldo4 {
> > > > > +       regulator-always-on;
> > > >
> > > > Does it have to be always on ?
> > >
> > > Mhh so I realize the regulators handling here is quite messy.
> > > I guess I didn't do such a good review of this patch internally.
> > >
> > > The situation is that the regulators on the camera board all have their
> > > enable pin tied to the DLDO4 output, but that would be best described as
> > > a vin-supply of the ov8865 regulators above. Their real vin supply is an
> > > always-on board-wide power source but I don't think we can represent another
> > > regulator acting as enable signal in a better way.
> >
> > That's right. I've modeled that as a parent regulator in the past, even
> > if it doesn't reflect the exact hardware topology, it's functionally
> > equivalent.
>
> Sounds good, I'll change it that way.
>
> Cheers,
>
> Paul
>
> > > Now beware that the camera board schematics are confusing as they show resistors
> > > (R17, R18, R19, R20, R23) connecting some power lines together, but they are not
> > > populated on the board (I guess the point is to make a variant of the design
> > > without regulators on the camera board and to use ones from the PMU instead).
> > > This probably confused Kevin and I back when this patch was made.
> > >
> > > > > +       regulator-min-microvolt = <2800000>;
> > > > > +       regulator-max-microvolt = <2800000>;
> > > > > +       regulator-name = "avdd-csi";
> > > >
> > > > Doesn't this belong to the base board .dts ? Same below.
> > > >
> > > > > +};
> > > > > +
> > > > >  &reg_drivevbus {
> > > > >         regulator-name = "usb0-vbus";AVDD-CSI
> > > > >         status = "okay";
> > > > >  };
> > > > >
> > > > > +&reg_eldo1 {
> > > > > +       regulator-min-microvolt = <1200000>;
> > > > > +       regulator-max-microvolt = <1200000>;
> > > > > +       regulator-name = "dvdd-csi-r";
> > > > > +};
> > > > > +
> > > > >  &reg_fldo1 {
> > > > >         regulator-min-microvolt = <1080000>;
> > > > >         regulator-max-microvolt = <1320000>;
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
>
> --
> Paul Kocialkowski, Bootlin
> Embedded Linux and kernel engineering
> https://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH NOT FOR MERGE v5 6/6] ARM: dts: sun8i: a83t: bananapi-m3: Enable MIPI CSI-2 with OV8865

[Paul Kocialkowski]

[-- Attachment #1.1: Type: text/plain, Size: 13532 bytes --]

Hi Dave,

On Thu 01 Sep 22, 14:34, Dave Stevenson wrote:
> Hi Laurent and Paul
> 
> On Thu, 1 Sept 2022 at 14:23, Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
> >
> > Hi Laurent,
> >
> > On Thu 01 Sep 22, 16:00, Laurent Pinchart wrote:
> > > On Thu, Sep 01, 2022 at 02:49:43PM +0200, Paul Kocialkowski wrote:
> > > > On Fri 26 Aug 22, 22:08, Laurent Pinchart wrote:
> > > > > On Fri, Aug 26, 2022 at 08:28:03PM +0200, Paul Kocialkowski wrote:
> > > > > > From: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> > > > > >
> > > > > > The Bananapi M3 supports a camera module which includes an OV8865 sensor
> > > > > > connected via the parallel CSI interface and an OV8865 sensor connected
> > > > > > via MIPI CSI-2.
> > > > > >
> > > > > > The I2C2 bus is shared by the two sensors as well as the (active-low)
> > > > > > reset signal, but each sensor has it own shutdown line.
> > > > >
> > > > > I see a single sensor in this file, am I missing something ?
> > > >
> > > > Indeed this patch only adds the OV8865, not the OV5640 which is also present
> > > > on the same camera board extension.
> > > >
> > > > A patch was submitted some time ago adding support for (only) the OV5640:
> > > > https://lore.kernel.org/lkml/20190408165744.11672-7-wens@kernel.org/
> > >
> > > OK. That's fine, let's just reword the commit message.
> >
> > Sure.
> >
> > > > > Sounds like a perfect candidate for an overlay, it could then be merged
> > > > > upstream.
> > > >
> > > > Do we have an upstream place to merge device-tree overlays now?
> > >
> > > They're accepted in the upstream kernel as far as I know, a git grep for
> > > /plugin/ in .dts files produces 18 matches.
> >
> > Oh okay, definitely good to know thanks!
> >
> > > > I'll check if it's possible to describe both sensors together and actually
> > > > be able to select one or the other properly from userspace. Last time I tried
> > > > this wasn't possible: there's at least the shared reset GPIO used by both
> > > > sensors, which cannot be requested by the two drivers in parallel.
> > > >
> > > > To be honest this is very poor hardware design and it was almost certainly
> > > > designed with the idea that only one sensor will be configured per boot.
> > > > See https://wiki.banana-pi.org/Camera and
> > > > https://drive.google.com/file/d/0B4PAo2nW2KfnOEFTMjZ2aEVGUVU/view?usp=sharing
> > > > for the schematics pdf
> > >
> > > It's not great indeed, but not that uncommon either (unfortunately). I
> > > wonder if we need some kind of reset GPIO API, but that would be a hack
> > > at most I think.
> >
> > Yeah I don't see any obvious clean solution here.
> > But that could still be two separate dt overlays for now.
> 
> Just to throw the idea out there, (ab)use the regulator API so that
> they share a regulator-gpio device that claims that GPIO.
> When either sensor requests the regulator, the reset line will get
> pulled to the appropriate state.

Yeah I guess that would solve the single-gpio/multiple-users issue.
But besides abusing one of the regulators for the job, that would also make
it hard to respect the power sequence of the sensor.

I think I would be happier with a dedicated GPIO reset driver that consumes a
single GPIO and provides as many GPIOs as needed with a behavior similar to
a reset controller: go out of reset as soon as one of the provided GPIOs
requests it and get back to reset as soon as all the provided GPIOs need
reset.

It's a bit weird but maybe that's legit enough to make its way upstream.

Paul

>   Dave
> 
> > > > > > Signed-off-by: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> > > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > > ---
> > > > > >  arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts | 102 +++++++++++++++++++
> > > > > >  1 file changed, 102 insertions(+)
> > > > > >
> > > > > > diff --git a/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts b/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
> > > > > > index 5a7e1bd5f825..80fd99cf24b2 100644
> > > > > > --- a/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
> > > > > > +++ b/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
> > > > > > @@ -85,6 +85,30 @@ led-1 {
> > > > > >                 };
> > > > > >         };
> > > > > >
> > > > > > +       reg_ov8865_avdd: ov8865-avdd {
> > > > > > +               compatible = "regulator-fixed";
> > > > > > +               regulator-name = "ov8865-avdd";
> > > > > > +               regulator-min-microvolt = <2800000>;
> > > > > > +               regulator-max-microvolt = <2800000>;
> > > > > > +               vin-supply = <&reg_dldo4>;
> > > > > > +       };
> > > > > > +
> > > > > > +       reg_ov8865_dovdd: ov8865-dovdd {
> > > > > > +               compatible = "regulator-fixed";
> > > > > > +               regulator-name = "ov8865-dovdd";
> > > > > > +               regulator-min-microvolt = <2800000>;
> > > > > > +               regulator-max-microvolt = <2800000>;
> > > > > > +               vin-supply = <&reg_dldo4>;
> > > > > > +       };
> > > > > > +
> > > > > > +       reg_ov8865_dvdd: ov8865-dvdd {
> > > > > > +               compatible = "regulator-fixed";
> > > > > > +               regulator-name = "ov8865-dvdd";
> > > > > > +               regulator-min-microvolt = <1200000>;
> > > > > > +               regulator-max-microvolt = <1200000>;
> > > > > > +               vin-supply = <&reg_eldo1>;
> > > > > > +       };
> > > > >
> > > > > Are those three regulators on the Banana Pi, or on the camera module ?
> > > >
> > > > That's on the camera module.
> > > >
> > > > > > +
> > > > > >         reg_usb1_vbus: reg-usb1-vbus {
> > > > > >                 compatible = "regulator-fixed";
> > > > > >                 regulator-name = "usb1-vbus";
> > > > > > @@ -115,6 +139,23 @@ &cpu100 {
> > > > > >         cpu-supply = <&reg_dcdc3>;
> > > > > >  };
> > > > > >
> > > > > > +&csi {
> > > > > > +       status = "okay";
> > > > > > +
> > > > > > +       ports {
> > > > > > +               #address-cells = <1>;
> > > > > > +               #size-cells = <0>;
> > > > > > +
> > > > > > +               port@1 {
> > > > > > +                       reg = <1>;
> > > > >
> > > > > All of this (except the status = "okay") should go to sun8i-a83t.dtsi.
> > > > >
> > > > > > +
> > > > > > +                       csi_in_mipi_csi2: endpoint {
> > > > > > +                               remote-endpoint = <&mipi_csi2_out_csi>;
> > > > > > +                       };
> > > > >
> > > > > This too actually, once the issue mentioned in patch 5/6 gets fixed.
> > > > >
> > > > > > +               };
> > > > > > +       };
> > > > > > +};
> > > > > > +
> > > > > >  &de {
> > > > > >         status = "okay";
> > > > > >  };
> > > > > > @@ -147,6 +188,36 @@ hdmi_out_con: endpoint {
> > > > > >         };
> > > > > >  };
> > > > > >
> > > > > > +&i2c2 {
> > > > > > +       pinctrl-names = "default";
> > > > > > +       pinctrl-0 = <&i2c2_pe_pins>;
> > > > >
> > > > > This looks like a candidate for upstreaming in
> > > > > sun8i-a83t-bananapi-m3.dts, it shouldn't be in the overlay.
> > > >
> > > > I2C2 is actually also exported in the PH pins, which is available on the board
> > > > header, so it's not obvious that using the PE pins should be the default.
> > > >
> > > > The context is that Allwinner SoCs usually have a dedicated I2C controller for
> > > > cameras, but also route a "generic" I2C controller on the same pins.
> > >
> > > Out of curiosity, what features do those dedicated camera I2C
> > > controllers provide, compared to "normal" I2C controllers ?
> >
> > IIRC there's some feature to send i2c messages synced with the camera interface
> > vsync signal, to have some kind of hardware atomic mechanism for reconfiguring
> > a sensor.
> >
> > Not sure that's very relevant for us and an implementation for it would probably
> > just support regular I2C. We can probably achieve simialr results with the
> > request API.
> >
> > > > Here that's on the PE14/15 pins. Since we don't have mainline support for this
> > > > camera-dedicated I2C controller, we end up routing the generic one to the PE
> > > > pins, which are routed to the camera connector.
> > >
> > > OK.
> > >
> > > > In the future we could add support for this camera-dedicated controller, which
> > > > would then allow routing i2c2 to the PH pins independently. This is what the
> > > > downstream Allwinner BSP kernel does.
> > > >
> > > > > > +       status = "okay";
> > > > > > +
> > > > > > +       ov8865: camera@36 {
> > > > > > +               compatible = "ovti,ov8865";
> > > > > > +               reg = <0x36>;
> > > > > > +               pinctrl-names = "default";
> > > > > > +               pinctrl-0 = <&csi_mclk_pin>;
> > > > > > +               clocks = <&ccu CLK_CSI_MCLK>;
> > > > > > +               assigned-clocks = <&ccu CLK_CSI_MCLK>;
> > > > > > +               assigned-clock-rates = <24000000>;
> > > > > > +               avdd-supply = <&reg_ov8865_avdd>;
> > > > > > +               dovdd-supply = <&reg_ov8865_dovdd>;
> > > > > > +               dvdd-supply = <&reg_ov8865_dvdd>;
> > > > > > +               powerdown-gpios = <&pio 4 17 GPIO_ACTIVE_LOW>; /* PE17 */
> > > > > > +               reset-gpios = <&pio 4 16 GPIO_ACTIVE_LOW>; /* PE16 */
> > > > > > +
> > > > > > +               port {
> > > > > > +                       ov8865_out_mipi_csi2: endpoint {
> > > > > > +                               data-lanes = <1 2 3 4>;
> > > > > > +                               link-frequencies = /bits/ 64 <360000000>;
> > > > > > +
> > > > > > +                               remote-endpoint = <&mipi_csi2_in_ov8865>;
> > > > > > +                       };
> > > > > > +               };
> > > > > > +       };
> > > > > > +};
> > > > > > +
> > > > > >  &mdio {
> > > > > >         rgmii_phy: ethernet-phy@1 {
> > > > > >                 compatible = "ethernet-phy-ieee802.3-c22";
> > > > > > @@ -154,6 +225,24 @@ rgmii_phy: ethernet-phy@1 {
> > > > > >         };
> > > > > >  };
> > > > > >
> > > > > > +&mipi_csi2 {
> > > > > > +       status = "okay";
> > > > > > +};
> > > > > > +
> > > > > > +&mipi_csi2_in {
> > > > > > +       mipi_csi2_in_ov8865: endpoint {
> > > > > > +               data-lanes = <1 2 3 4>;
> > > > > > +
> > > > > > +               remote-endpoint = <&ov8865_out_mipi_csi2>;
> > > > > > +       };
> > > > > > +};
> > > > > > +
> > > > > > +&mipi_csi2_out {
> > > > > > +       mipi_csi2_out_csi: endpoint {
> > > > > > +               remote-endpoint = <&csi_in_mipi_csi2>;
> > > > > > +       };
> > > > > > +};
> > > > > > +
> > > > > >  &mmc0 {
> > > > > >         pinctrl-names = "default";
> > > > > >         pinctrl-0 = <&mmc0_pins>;
> > > > > > @@ -327,11 +416,24 @@ &reg_dldo3 {
> > > > > >         regulator-name = "vcc-pd";
> > > > > >  };
> > > > > >
> > > > > > +&reg_dldo4 {
> > > > > > +       regulator-always-on;
> > > > >
> > > > > Does it have to be always on ?
> > > >
> > > > Mhh so I realize the regulators handling here is quite messy.
> > > > I guess I didn't do such a good review of this patch internally.
> > > >
> > > > The situation is that the regulators on the camera board all have their
> > > > enable pin tied to the DLDO4 output, but that would be best described as
> > > > a vin-supply of the ov8865 regulators above. Their real vin supply is an
> > > > always-on board-wide power source but I don't think we can represent another
> > > > regulator acting as enable signal in a better way.
> > >
> > > That's right. I've modeled that as a parent regulator in the past, even
> > > if it doesn't reflect the exact hardware topology, it's functionally
> > > equivalent.
> >
> > Sounds good, I'll change it that way.
> >
> > Cheers,
> >
> > Paul
> >
> > > > Now beware that the camera board schematics are confusing as they show resistors
> > > > (R17, R18, R19, R20, R23) connecting some power lines together, but they are not
> > > > populated on the board (I guess the point is to make a variant of the design
> > > > without regulators on the camera board and to use ones from the PMU instead).
> > > > This probably confused Kevin and I back when this patch was made.
> > > >
> > > > > > +       regulator-min-microvolt = <2800000>;
> > > > > > +       regulator-max-microvolt = <2800000>;
> > > > > > +       regulator-name = "avdd-csi";
> > > > >
> > > > > Doesn't this belong to the base board .dts ? Same below.
> > > > >
> > > > > > +};
> > > > > > +
> > > > > >  &reg_drivevbus {
> > > > > >         regulator-name = "usb0-vbus";AVDD-CSI
> > > > > >         status = "okay";
> > > > > >  };
> > > > > >
> > > > > > +&reg_eldo1 {
> > > > > > +       regulator-min-microvolt = <1200000>;
> > > > > > +       regulator-max-microvolt = <1200000>;
> > > > > > +       regulator-name = "dvdd-csi-r";
> > > > > > +};
> > > > > > +
> > > > > >  &reg_fldo1 {
> > > > > >         regulator-min-microvolt = <1080000>;
> > > > > >         regulator-max-microvolt = <1320000>;
> > >
> > > --
> > > Regards,
> > >
> > > Laurent Pinchart
> >
> > --
> > Paul Kocialkowski, Bootlin
> > Embedded Linux and kernel engineering
> > https://bootlin.com

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH NOT FOR MERGE v5 6/6] ARM: dts: sun8i: a83t: bananapi-m3: Enable MIPI CSI-2 with OV8865

[Laurent Pinchart]

Hi Paul,

On Thu, Sep 01, 2022 at 03:22:34PM +0200, Paul Kocialkowski wrote:
> On Thu 01 Sep 22, 16:00, Laurent Pinchart wrote:
> > On Thu, Sep 01, 2022 at 02:49:43PM +0200, Paul Kocialkowski wrote:
> > > On Fri 26 Aug 22, 22:08, Laurent Pinchart wrote:
> > > > On Fri, Aug 26, 2022 at 08:28:03PM +0200, Paul Kocialkowski wrote:
> > > > > From: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> > > > > 
> > > > > The Bananapi M3 supports a camera module which includes an OV8865 sensor
> > > > > connected via the parallel CSI interface and an OV8865 sensor connected
> > > > > via MIPI CSI-2.
> > > > > 
> > > > > The I2C2 bus is shared by the two sensors as well as the (active-low)
> > > > > reset signal, but each sensor has it own shutdown line.
> > > > 
> > > > I see a single sensor in this file, am I missing something ?
> > > 
> > > Indeed this patch only adds the OV8865, not the OV5640 which is also present
> > > on the same camera board extension.
> > > 
> > > A patch was submitted some time ago adding support for (only) the OV5640:
> > > https://lore.kernel.org/lkml/20190408165744.11672-7-wens@kernel.org/
> > 
> > OK. That's fine, let's just reword the commit message.
> 
> Sure.
> 
> > > > Sounds like a perfect candidate for an overlay, it could then be merged
> > > > upstream.
> > > 
> > > Do we have an upstream place to merge device-tree overlays now?
> > 
> > They're accepted in the upstream kernel as far as I know, a git grep for
> > /plugin/ in .dts files produces 18 matches.
> 
> Oh okay, definitely good to know thanks!
> 
> > > I'll check if it's possible to describe both sensors together and actually
> > > be able to select one or the other properly from userspace. Last time I tried
> > > this wasn't possible: there's at least the shared reset GPIO used by both
> > > sensors, which cannot be requested by the two drivers in parallel.
> > > 
> > > To be honest this is very poor hardware design and it was almost certainly
> > > designed with the idea that only one sensor will be configured per boot.
> > > See https://wiki.banana-pi.org/Camera and
> > > https://drive.google.com/file/d/0B4PAo2nW2KfnOEFTMjZ2aEVGUVU/view?usp=sharing
> > > for the schematics pdf
> > 
> > It's not great indeed, but not that uncommon either (unfortunately). I
> > wonder if we need some kind of reset GPIO API, but that would be a hack
> > at most I think.
> 
> Yeah I don't see any obvious clean solution here.
> But that could still be two separate dt overlays for now.
> 
> > > > > Signed-off-by: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > > ---
> > > > >  arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts | 102 +++++++++++++++++++
> > > > >  1 file changed, 102 insertions(+)
> > > > > 
> > > > > diff --git a/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts b/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
> > > > > index 5a7e1bd5f825..80fd99cf24b2 100644
> > > > > --- a/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
> > > > > +++ b/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts
> > > > > @@ -85,6 +85,30 @@ led-1 {
> > > > >  		};
> > > > >  	};
> > > > >  
> > > > > +	reg_ov8865_avdd: ov8865-avdd {
> > > > > +		compatible = "regulator-fixed";
> > > > > +		regulator-name = "ov8865-avdd";
> > > > > +		regulator-min-microvolt = <2800000>;
> > > > > +		regulator-max-microvolt = <2800000>;
> > > > > +		vin-supply = <&reg_dldo4>;
> > > > > +	};
> > > > > +
> > > > > +	reg_ov8865_dovdd: ov8865-dovdd {
> > > > > +		compatible = "regulator-fixed";
> > > > > +		regulator-name = "ov8865-dovdd";
> > > > > +		regulator-min-microvolt = <2800000>;
> > > > > +		regulator-max-microvolt = <2800000>;
> > > > > +		vin-supply = <&reg_dldo4>;
> > > > > +	};
> > > > > +
> > > > > +	reg_ov8865_dvdd: ov8865-dvdd {
> > > > > +		compatible = "regulator-fixed";
> > > > > +		regulator-name = "ov8865-dvdd";
> > > > > +		regulator-min-microvolt = <1200000>;
> > > > > +		regulator-max-microvolt = <1200000>;
> > > > > +		vin-supply = <&reg_eldo1>;
> > > > > +	};
> > > > 
> > > > Are those three regulators on the Banana Pi, or on the camera module ?
> > > 
> > > That's on the camera module.
> > > 
> > > > > +
> > > > >  	reg_usb1_vbus: reg-usb1-vbus {
> > > > >  		compatible = "regulator-fixed";
> > > > >  		regulator-name = "usb1-vbus";
> > > > > @@ -115,6 +139,23 @@ &cpu100 {
> > > > >  	cpu-supply = <&reg_dcdc3>;
> > > > >  };
> > > > >  
> > > > > +&csi {
> > > > > +	status = "okay";
> > > > > +
> > > > > +	ports {
> > > > > +		#address-cells = <1>;
> > > > > +		#size-cells = <0>;
> > > > > +
> > > > > +		port@1 {
> > > > > +			reg = <1>;
> > > > 
> > > > All of this (except the status = "okay") should go to sun8i-a83t.dtsi.
> > > > 
> > > > > +
> > > > > +			csi_in_mipi_csi2: endpoint {
> > > > > +				remote-endpoint = <&mipi_csi2_out_csi>;
> > > > > +			};
> > > > 
> > > > This too actually, once the issue mentioned in patch 5/6 gets fixed.
> > > > 
> > > > > +		};
> > > > > +	};
> > > > > +};
> > > > > +
> > > > >  &de {
> > > > >  	status = "okay";
> > > > >  };
> > > > > @@ -147,6 +188,36 @@ hdmi_out_con: endpoint {
> > > > >  	};
> > > > >  };
> > > > >  
> > > > > +&i2c2 {
> > > > > +	pinctrl-names = "default";
> > > > > +	pinctrl-0 = <&i2c2_pe_pins>;
> > > > 
> > > > This looks like a candidate for upstreaming in
> > > > sun8i-a83t-bananapi-m3.dts, it shouldn't be in the overlay.
> > > 
> > > I2C2 is actually also exported in the PH pins, which is available on the board
> > > header, so it's not obvious that using the PE pins should be the default.
> > > 
> > > The context is that Allwinner SoCs usually have a dedicated I2C controller for
> > > cameras, but also route a "generic" I2C controller on the same pins.
> > 
> > Out of curiosity, what features do those dedicated camera I2C
> > controllers provide, compared to "normal" I2C controllers ?
> 
> IIRC there's some feature to send i2c messages synced with the camera interface
> vsync signal, to have some kind of hardware atomic mechanism for reconfiguring
> a sensor.
> 
> Not sure that's very relevant for us and an implementation for it would probably
> just support regular I2C. We can probably achieve simialr results with the
> request API.

This hardware feature would be very useful to improve compliance with
the real-time requirements of the camera sensor. It's something that I
would really like to be supported in Linux, but we'll need to design a
new API for it.

> > > Here that's on the PE14/15 pins. Since we don't have mainline support for this
> > > camera-dedicated I2C controller, we end up routing the generic one to the PE
> > > pins, which are routed to the camera connector.
> > 
> > OK.
> > 
> > > In the future we could add support for this camera-dedicated controller, which
> > > would then allow routing i2c2 to the PH pins independently. This is what the
> > > downstream Allwinner BSP kernel does.
> > > 
> > > > > +	status = "okay";
> > > > > +
> > > > > +	ov8865: camera@36 {
> > > > > +		compatible = "ovti,ov8865";
> > > > > +		reg = <0x36>;
> > > > > +		pinctrl-names = "default";
> > > > > +		pinctrl-0 = <&csi_mclk_pin>;
> > > > > +		clocks = <&ccu CLK_CSI_MCLK>;
> > > > > +		assigned-clocks = <&ccu CLK_CSI_MCLK>;
> > > > > +		assigned-clock-rates = <24000000>;
> > > > > +		avdd-supply = <&reg_ov8865_avdd>;
> > > > > +		dovdd-supply = <&reg_ov8865_dovdd>;
> > > > > +		dvdd-supply = <&reg_ov8865_dvdd>;
> > > > > +		powerdown-gpios = <&pio 4 17 GPIO_ACTIVE_LOW>; /* PE17 */
> > > > > +		reset-gpios = <&pio 4 16 GPIO_ACTIVE_LOW>; /* PE16 */
> > > > > +
> > > > > +		port {
> > > > > +			ov8865_out_mipi_csi2: endpoint {
> > > > > +				data-lanes = <1 2 3 4>;
> > > > > +				link-frequencies = /bits/ 64 <360000000>;
> > > > > +
> > > > > +				remote-endpoint = <&mipi_csi2_in_ov8865>;
> > > > > +			};
> > > > > +		};
> > > > > +	};
> > > > > +};
> > > > > +
> > > > >  &mdio {
> > > > >  	rgmii_phy: ethernet-phy@1 {
> > > > >  		compatible = "ethernet-phy-ieee802.3-c22";
> > > > > @@ -154,6 +225,24 @@ rgmii_phy: ethernet-phy@1 {
> > > > >  	};
> > > > >  };
> > > > >  
> > > > > +&mipi_csi2 {
> > > > > +	status = "okay";
> > > > > +};
> > > > > +
> > > > > +&mipi_csi2_in {
> > > > > +	mipi_csi2_in_ov8865: endpoint {
> > > > > +		data-lanes = <1 2 3 4>;
> > > > > +
> > > > > +		remote-endpoint = <&ov8865_out_mipi_csi2>;
> > > > > +	};
> > > > > +};
> > > > > +
> > > > > +&mipi_csi2_out {
> > > > > +	mipi_csi2_out_csi: endpoint {
> > > > > +		remote-endpoint = <&csi_in_mipi_csi2>;
> > > > > +	};
> > > > > +};
> > > > > +
> > > > >  &mmc0 {
> > > > >  	pinctrl-names = "default";
> > > > >  	pinctrl-0 = <&mmc0_pins>;
> > > > > @@ -327,11 +416,24 @@ &reg_dldo3 {
> > > > >  	regulator-name = "vcc-pd";
> > > > >  };
> > > > >  
> > > > > +&reg_dldo4 {
> > > > > +	regulator-always-on;
> > > > 
> > > > Does it have to be always on ?
> > > 
> > > Mhh so I realize the regulators handling here is quite messy.
> > > I guess I didn't do such a good review of this patch internally.
> > > 
> > > The situation is that the regulators on the camera board all have their
> > > enable pin tied to the DLDO4 output, but that would be best described as
> > > a vin-supply of the ov8865 regulators above. Their real vin supply is an
> > > always-on board-wide power source but I don't think we can represent another
> > > regulator acting as enable signal in a better way.
> > 
> > That's right. I've modeled that as a parent regulator in the past, even
> > if it doesn't reflect the exact hardware topology, it's functionally
> > equivalent.
> 
> Sounds good, I'll change it that way.
> 
> > > Now beware that the camera board schematics are confusing as they show resistors
> > > (R17, R18, R19, R20, R23) connecting some power lines together, but they are not
> > > populated on the board (I guess the point is to make a variant of the design
> > > without regulators on the camera board and to use ones from the PMU instead).
> > > This probably confused Kevin and I back when this patch was made.
> > > 
> > > > > +	regulator-min-microvolt = <2800000>;
> > > > > +	regulator-max-microvolt = <2800000>;
> > > > > +	regulator-name = "avdd-csi";
> > > > 
> > > > Doesn't this belong to the base board .dts ? Same below.
> > > > 
> > > > > +};
> > > > > +
> > > > >  &reg_drivevbus {
> > > > >  	regulator-name = "usb0-vbus";AVDD-CSI
> > > > >  	status = "okay";
> > > > >  };
> > > > >  
> > > > > +&reg_eldo1 {
> > > > > +	regulator-min-microvolt = <1200000>;
> > > > > +	regulator-max-microvolt = <1200000>;
> > > > > +	regulator-name = "dvdd-csi-r";
> > > > > +};
> > > > > +
> > > > >  &reg_fldo1 {
> > > > >  	regulator-min-microvolt = <1080000>;
> > > > >  	regulator-max-microvolt = <1320000>;

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel