Message-ID: <YxCyGgfNVHGIJlMz@aptenodytes>
Date:   Thu, 1 Sep 2022 15:22:34 +0200
From:   Paul Kocialkowski <paul.kocialkowski@...tlin.com>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-sunxi@...ts.linux.dev, linux-kernel@...r.kernel.org,
        linux-media@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
        Chen-Yu Tsai <wens@...e.org>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Samuel Holland <samuel@...lland.org>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        Kévin L'hôpital <kevin.lhopital@...tlin.com>
Subject: Re: [PATCH NOT FOR MERGE v5 6/6] ARM: dts: sun8i: a83t: bananapi-m3:
 Enable MIPI CSI-2 with OV8865

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@...tlin.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@...tlin.com>
> > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@...tlin.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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists