| Message ID | 20240123214729.2852346-1-andrej.skvortzov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: dts: sun50i-a64-pinephone: Add front/back cameras | expand |
Hi Andrey, On Wed, Jan 24, 2024 at 12:47:29AM +0300, Andrey Skvortsov wrote: > From: Ondřej Jirman <megi@xff.cz> > > Pinephone has OV5640 back camera and GC2145 front camera. Add support > for both. The upstream driver doesn't support multiple endpoints per port. See: https://elixir.bootlin.com/linux/v6.8-rc1/source/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml#L43 Only one endpoint is allowed/supported. Looking throught LKML, I don't see the support for multiple parallel interface endpoints being added recently... So this patch will not work, and will cause DTS validation errors. Kind regards, o. > Signed-off-by: Ondrej Jirman <megi@xff.cz> > Signed-off-by: Andrey Skvortsov <andrej.skvortzov@gmail.com> > --- > .../dts/allwinner/sun50i-a64-pinephone.dtsi | 91 +++++++++++++++++++ > 1 file changed, 91 insertions(+) > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi > index 87847116ab6d..4104a136ff75 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi > @@ -36,6 +36,15 @@ chosen { > stdout-path = "serial0:115200n8"; > }; > > + i2c_csi: i2c-csi { > + compatible = "i2c-gpio"; > + sda-gpios = <&pio 4 13 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>; /* PE13 */ > + scl-gpios = <&pio 4 12 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>; /* PE12 */ > + i2c-gpio,delay-us = <3>; > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + > leds { > compatible = "gpio-leds"; > > @@ -124,6 +133,36 @@ &cpu3 { > cpu-supply = <®_dcdc2>; > }; > > +&csi { > + pinctrl-0 = <&csi_pins>, <&csi_mclk_pin>; > + status = "okay"; > + > + port { > + #address-cells = <1>; > + #size-cells = <0>; > + > + csi_ov5640_ep: endpoint@0 { > + reg = <0>; > + remote-endpoint = <&ov5640_ep>; > + bus-width = <8>; > + hsync-active = <1>; /* Active high */ > + vsync-active = <0>; /* Active low */ > + data-active = <1>; /* Active high */ > + pclk-sample = <1>; /* Rising */ > + }; > + > + csi_gc2145_ep: endpoint@1 { > + reg = <1>; > + remote-endpoint = <&gc2145_ep>; > + bus-width = <8>; > + hsync-active = <1>; > + vsync-active = <1>; > + data-active = <1>; > + pclk-sample = <1>; > + }; > + }; > +}; > + > &dai { > status = "okay"; > }; > @@ -158,6 +197,58 @@ &ehci1 { > status = "okay"; > }; > > +&i2c_csi { > + gc2145: front-camera@3c { > + compatible = "galaxycore,gc2145"; > + reg = <0x3c>; > + clocks = <&ccu CLK_CSI_MCLK>; > + clock-names = "xclk"; > + avdd-supply = <®_dldo3>; > + dvdd-supply = <®_aldo1>; > + iovdd-supply = <®_eldo3>; > + reset-gpios = <&pio 4 16 (GPIO_ACTIVE_LOW | GPIO_OPEN_DRAIN)>; /* PE16 */ > + powerdown-gpios = <&pio 4 17 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>; /* PE17 */ > + rotation = <270>; > + orientation = <0>; > + > + port { > + gc2145_ep: endpoint { > + remote-endpoint = <&csi_gc2145_ep>; > + bus-width = <8>; > + hsync-active = <1>; > + vsync-active = <1>; > + data-active = <1>; > + pclk-sample = <1>; > + }; > + }; > + }; > + > + ov5640: rear-camera@4c { > + compatible = "ovti,ov5640"; > + reg = <0x4c>; > + clocks = <&ccu CLK_CSI_MCLK>; > + clock-names = "xclk"; > + AVDD-supply = <®_dldo3>; > + DOVDD-supply = <®_aldo1>; /* shared with AFVCC */ > + DVDD-supply = <®_eldo3>; > + reset-gpios = <&pio 3 3 (GPIO_ACTIVE_LOW | GPIO_OPEN_DRAIN)>; /* PD3 */ > + powerdown-gpios = <&pio 2 0 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>; /* PC0 */ > + rotation = <90>; > + orientation = <1>; > + > + port { > + ov5640_ep: endpoint { > + remote-endpoint = <&csi_ov5640_ep>; > + bus-width = <8>; > + hsync-active = <1>; /* Active high */ > + vsync-active = <0>; /* Active low */ > + data-active = <1>; /* Active high */ > + pclk-sample = <1>; /* Rising */ > + }; > + }; > + }; > +}; > + > &i2c0 { > status = "okay"; > > -- > 2.43.0 >
Hi Ondrey, On 24-01-23 23:01, Ondřej Jirman wrote: > Hi Andrey, > > On Wed, Jan 24, 2024 at 12:47:29AM +0300, Andrey Skvortsov wrote: > > From: Ondřej Jirman <megi@xff.cz> > > > > Pinephone has OV5640 back camera and GC2145 front camera. Add support > > for both. > > The upstream driver doesn't support multiple endpoints per port. See: > > https://elixir.bootlin.com/linux/v6.8-rc1/source/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml#L43 > > Only one endpoint is allowed/supported. Looking throught LKML, I don't > see the support for multiple parallel interface endpoints being added > recently... > > So this patch will not work, and will cause DTS validation errors. Thank you! I've not run dtb validation before submission, sorry for that. I've ran 'make dtbs_check' and have several question now. 1. I don't see any complaints about multiple endpoints definitions. IMHO, it looks okay from binding point of view according to that video-interfaces.yaml [1]. I know that currently multiple endpoint implementation for parallel interface is missing in sun6i_csi. Current out-of-tree implementation doesn't require any change in bindings (Hopefully it will be upstream one day) Or do you mean this change has to be submitted upstream only once sun6i_csi gets fixed? 2. dtbs_check complaints about missing link-frequencies for recently submitted gc2145. [2] ``` front-camera@3c: port:endpoint: 'link-frequencies' is a required property ``` I've looked at other drivers and link-frequencies are used only mostly for CSI-2 endpoints. Should it be required for CPI/DVP ? Not many mainline drivers support CSI-2 and DVP: ov5640, s5k5baf, mt9mt114. Only mt9mt114 uses link-frequencies property for DVP and it should match PCLK (double pixelrate). [3] Should I define link-frequencies for DVP as a double pixelrate here as well and handle that in the driver? Currently gc2145 doesn't support DVP, but I have basic working support for DVP for the upstreamed driver for a long time and waited for it to be merged into mainline. I'd like to submit it for review. Until now I thought, that submitted gc2145 bindings will be the same for DVP and CSI-2 support and therefore submitted this change. Are they? Or should I introduce bus-type and conditionally handle requirements in yaml if link-frequencies can be ignored for DVP? Something link this ``` properties: link-frequencies: true bus-type: enum: - 4 # CSI-2 D-PHY - 5 # Parallel required: - bus-type allOf: - if: properties: bus-type: const: 4 then: required: - link-frequencies ``` Should I better submit DVP support to the gc2145 driver first and only then submit this change? > > Kind regards, > o. > > > Signed-off-by: Ondrej Jirman <megi@xff.cz> > > Signed-off-by: Andrey Skvortsov <andrej.skvortzov@gmail.com> > > --- > > .../dts/allwinner/sun50i-a64-pinephone.dtsi | 91 +++++++++++++++++++ > > 1 file changed, 91 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi > > index 87847116ab6d..4104a136ff75 100644 > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi > > @@ -36,6 +36,15 @@ chosen { > > stdout-path = "serial0:115200n8"; > > }; > > > > + i2c_csi: i2c-csi { > > + compatible = "i2c-gpio"; > > + sda-gpios = <&pio 4 13 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>; /* PE13 */ > > + scl-gpios = <&pio 4 12 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>; /* PE12 */ > > + i2c-gpio,delay-us = <3>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + }; > > + > > leds { > > compatible = "gpio-leds"; > > > > @@ -124,6 +133,36 @@ &cpu3 { > > cpu-supply = <®_dcdc2>; > > }; > > > > +&csi { > > + pinctrl-0 = <&csi_pins>, <&csi_mclk_pin>; > > + status = "okay"; > > + > > + port { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + csi_ov5640_ep: endpoint@0 { > > + reg = <0>; > > + remote-endpoint = <&ov5640_ep>; > > + bus-width = <8>; > > + hsync-active = <1>; /* Active high */ > > + vsync-active = <0>; /* Active low */ > > + data-active = <1>; /* Active high */ > > + pclk-sample = <1>; /* Rising */ > > + }; > > + > > + csi_gc2145_ep: endpoint@1 { > > + reg = <1>; > > + remote-endpoint = <&gc2145_ep>; > > + bus-width = <8>; > > + hsync-active = <1>; > > + vsync-active = <1>; > > + data-active = <1>; > > + pclk-sample = <1>; > > + }; > > + }; > > +}; > > + > > &dai { > > status = "okay"; > > }; > > @@ -158,6 +197,58 @@ &ehci1 { > > status = "okay"; > > }; > > > > +&i2c_csi { > > + gc2145: front-camera@3c { > > + compatible = "galaxycore,gc2145"; > > + reg = <0x3c>; > > + clocks = <&ccu CLK_CSI_MCLK>; > > + clock-names = "xclk"; That should be removed to fix ``` front-camera@3c: 'clock-names' does not match any of the regexes: 'pinctrl-[0-9]+' ``` when running 'make dtbs_check' > > + avdd-supply = <®_dldo3>; > > + dvdd-supply = <®_aldo1>; > > + iovdd-supply = <®_eldo3>; > > + reset-gpios = <&pio 4 16 (GPIO_ACTIVE_LOW | GPIO_OPEN_DRAIN)>; /* PE16 */ > > + powerdown-gpios = <&pio 4 17 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>; /* PE17 */ > > + rotation = <270>; > > + orientation = <0>; > > + > > + port { > > + gc2145_ep: endpoint { > > + remote-endpoint = <&csi_gc2145_ep>; > > + bus-width = <8>; > > + hsync-active = <1>; > > + vsync-active = <1>; > > + data-active = <1>; > > + pclk-sample = <1>; > > + }; > > + }; > > + }; > > + > > + ov5640: rear-camera@4c { > > + compatible = "ovti,ov5640"; > > + reg = <0x4c>; > > + clocks = <&ccu CLK_CSI_MCLK>; > > + clock-names = "xclk"; > > + AVDD-supply = <®_dldo3>; > > + DOVDD-supply = <®_aldo1>; /* shared with AFVCC */ > > + DVDD-supply = <®_eldo3>; > > + reset-gpios = <&pio 3 3 (GPIO_ACTIVE_LOW | GPIO_OPEN_DRAIN)>; /* PD3 */ > > + powerdown-gpios = <&pio 2 0 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>; /* PC0 */ > > + rotation = <90>; > > + orientation = <1>; > > + > > + port { > > + ov5640_ep: endpoint { > > + remote-endpoint = <&csi_ov5640_ep>; > > + bus-width = <8>; > > + hsync-active = <1>; /* Active high */ > > + vsync-active = <0>; /* Active low */ > > + data-active = <1>; /* Active high */ > > + pclk-sample = <1>; /* Rising */ > > + }; > > + }; > > + }; > > +}; > > + > > &i2c0 { > > status = "okay"; > > > > -- > > 2.43.0 > > 1. https://elixir.bootlin.com/linux/v6.8-rc1/source/Documentation/devicetree/bindings/media/video-interfaces.yaml#L41 2. https://elixir.bootlin.com/linux/v6.8-rc1/source/Documentation/devicetree/bindings/media/i2c/galaxycore,gc2145.yaml#L65 3. https://elixir.bootlin.com/linux/v6.8-rc1/source/drivers/media/i2c/mt9m114.c#L2256
Hey Andrey, On Wed, Jan 24, 2024 at 03:25:53PM +0300, Andrey Skvortsov wrote: > Hi Ondrey, > > On 24-01-23 23:01, Ondřej Jirman wrote: > > Hi Andrey, > > > > On Wed, Jan 24, 2024 at 12:47:29AM +0300, Andrey Skvortsov wrote: > > > From: Ondřej Jirman <megi@xff.cz> > > > > > > Pinephone has OV5640 back camera and GC2145 front camera. Add support > > > for both. > > > > The upstream driver doesn't support multiple endpoints per port. See: > > > > https://elixir.bootlin.com/linux/v6.8-rc1/source/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-csi.yaml#L43 > > > > Only one endpoint is allowed/supported. Looking throught LKML, I don't > > see the support for multiple parallel interface endpoints being added > > recently... > > > > So this patch will not work, and will cause DTS validation errors. > > Thank you! I've not run dtb validation before submission, sorry for that. > I've ran 'make dtbs_check' and have several question now. > > 1. I don't see any complaints about multiple endpoints definitions. > > IMHO, it looks okay from binding point of view according to that > video-interfaces.yaml [1]. Allright, I've checked allwinner,sun6i-a31-csi.yaml and there I see: port: $ref: /schemas/graph.yaml#/$defs/port-base description: Parallel input port, connect to a parallel sensor properties: endpoint: $ref: video-interfaces.yaml# unevaluatedProperties: false properties: bus-width: enum: [ 8, 10, 12, 16 ] pclk-sample: true hsync-active: true vsync-active: true required: - bus-width unevaluatedProperties: false Which looks like a single endpoint node reference without any @somethnig suffix possible, but I guess in reality it means any number of those with arbitrary @something suffix. My bad. > I know that currently multiple endpoint implementation for parallel > interface is missing in sun6i_csi. Current out-of-tree implementation > doesn't require any change in bindings (Hopefully it will be upstream one > day) Or do you mean this change has to be submitted upstream only once > sun6i_csi gets fixed? If bindings are proper, then it's ok. I don't see a reason for them changing. It's a single physical port that has two cameras connected to it. So having two enpoints under one port describes reality well. A good thing is that the patch for multiple cameras is quite small against 6.7/6.8. It should be easy to merge, especially when it looks like now that I see there are not even DT changes needed. > > 2. dtbs_check complaints about missing link-frequencies for recently > submitted gc2145. [2] > ``` > front-camera@3c: port:endpoint: 'link-frequencies' is a required property > ``` > > I've looked at other drivers and link-frequencies are used only > mostly for CSI-2 endpoints. Should it be required for CPI/DVP ? > > Not many mainline drivers support CSI-2 and DVP: ov5640, s5k5baf, > mt9mt114. Only mt9mt114 uses link-frequencies property for DVP and it > should match PCLK (double pixelrate). [3] > > > Should I define link-frequencies for DVP as a double pixelrate here as > well and handle that in the driver? No. Pixel clock is generated by the sensor via some PLL based on master clock generated from the SoC (in this case). It will depend on pixel format and pixel rate, which in turn depends on selected format and frame rate. I don't see why this should be a predefined list of values in DT. The driver should use the lowest PCLK possible for a selected mode, to save power. Parallel interface on Pinephone goes up to maybe 50 MHz and is a slave interface with no adjustables based on PCLK frequency. So there's no in-kernel consumer for the sensor chosen PCLK (V4L2_CID_LINK_FREQ) information. What makes sense to me practically is to have a per board ability to specify a maximum PCLK frequency in DT, based on how well the routing and termination of signals went on a particular SBC/Phone/whatever or what SoC receiver port supports. The sensor driver could use this to limit the top PCLK frequency it uses, becuase it's a bus master. (eg. by limiting the modes or framerates it offers to userspace, because if mode would require say 70MHz PCLK, it would be unrealizable on Pinephone, even if sensor can happily produce 70MHz PCLK and other signals). Fixed list of link-frequencies in DT is meaningless for this situation. You can have sensor use any frequency up to a certain limit. What matters is the top bus frequency limit. (Around 50MHz on Pinephone) > Currently gc2145 doesn't support DVP, but I have basic working support > for DVP for the upstreamed driver for a long time and waited for it to be merged > into mainline. I'd like to submit it for review. Until now I thought, > that submitted gc2145 bindings will be the same for DVP and CSI-2 > support and therefore submitted this change. > > Are they? Or should I introduce bus-type and conditionally > handle requirements in yaml if link-frequencies can be ignored for DVP? > > Something link this > ``` > properties: > link-frequencies: true > > bus-type: > enum: > - 4 # CSI-2 D-PHY > - 5 # Parallel > > required: > - bus-type > > allOf: > - if: > properties: > bus-type: > const: 4 > then: > required: > - link-frequencies > ``` > > Should I better submit DVP support to the gc2145 driver first and only then > submit this change? That sounds like a good approach. > > > > > > [...] > > > > > > +&i2c_csi { > > > + gc2145: front-camera@3c { > > > + compatible = "galaxycore,gc2145"; > > > + reg = <0x3c>; > > > + clocks = <&ccu CLK_CSI_MCLK>; > > > + clock-names = "xclk"; > > That should be removed to fix > ``` > front-camera@3c: 'clock-names' does not match any of the regexes: 'pinctrl-[0-9]+' > ``` Yes. The driver uses gc2145->xclk = devm_clk_get(dev, NULL); without a clock name. So setting one here is superfluous. kind regards, o.
Hi! > Pinephone has OV5640 back camera and GC2145 front camera. Add support > for both. Thanks for doing this. I'm currently fighting with kexec on pinephone (it crashes as soon as I pass dtb) so I did not test this, but hw description looks fine. Signed-off-by: Pavel Machek <pavel@ucw.cz>
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi index 87847116ab6d..4104a136ff75 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinephone.dtsi @@ -36,6 +36,15 @@ chosen { stdout-path = "serial0:115200n8"; }; + i2c_csi: i2c-csi { + compatible = "i2c-gpio"; + sda-gpios = <&pio 4 13 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>; /* PE13 */ + scl-gpios = <&pio 4 12 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>; /* PE12 */ + i2c-gpio,delay-us = <3>; + #address-cells = <1>; + #size-cells = <0>; + }; + leds { compatible = "gpio-leds"; @@ -124,6 +133,36 @@ &cpu3 { cpu-supply = <®_dcdc2>; }; +&csi { + pinctrl-0 = <&csi_pins>, <&csi_mclk_pin>; + status = "okay"; + + port { + #address-cells = <1>; + #size-cells = <0>; + + csi_ov5640_ep: endpoint@0 { + reg = <0>; + remote-endpoint = <&ov5640_ep>; + bus-width = <8>; + hsync-active = <1>; /* Active high */ + vsync-active = <0>; /* Active low */ + data-active = <1>; /* Active high */ + pclk-sample = <1>; /* Rising */ + }; + + csi_gc2145_ep: endpoint@1 { + reg = <1>; + remote-endpoint = <&gc2145_ep>; + bus-width = <8>; + hsync-active = <1>; + vsync-active = <1>; + data-active = <1>; + pclk-sample = <1>; + }; + }; +}; + &dai { status = "okay"; }; @@ -158,6 +197,58 @@ &ehci1 { status = "okay"; }; +&i2c_csi { + gc2145: front-camera@3c { + compatible = "galaxycore,gc2145"; + reg = <0x3c>; + clocks = <&ccu CLK_CSI_MCLK>; + clock-names = "xclk"; + avdd-supply = <®_dldo3>; + dvdd-supply = <®_aldo1>; + iovdd-supply = <®_eldo3>; + reset-gpios = <&pio 4 16 (GPIO_ACTIVE_LOW | GPIO_OPEN_DRAIN)>; /* PE16 */ + powerdown-gpios = <&pio 4 17 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>; /* PE17 */ + rotation = <270>; + orientation = <0>; + + port { + gc2145_ep: endpoint { + remote-endpoint = <&csi_gc2145_ep>; + bus-width = <8>; + hsync-active = <1>; + vsync-active = <1>; + data-active = <1>; + pclk-sample = <1>; + }; + }; + }; + + ov5640: rear-camera@4c { + compatible = "ovti,ov5640"; + reg = <0x4c>; + clocks = <&ccu CLK_CSI_MCLK>; + clock-names = "xclk"; + AVDD-supply = <®_dldo3>; + DOVDD-supply = <®_aldo1>; /* shared with AFVCC */ + DVDD-supply = <®_eldo3>; + reset-gpios = <&pio 3 3 (GPIO_ACTIVE_LOW | GPIO_OPEN_DRAIN)>; /* PD3 */ + powerdown-gpios = <&pio 2 0 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>; /* PC0 */ + rotation = <90>; + orientation = <1>; + + port { + ov5640_ep: endpoint { + remote-endpoint = <&csi_ov5640_ep>; + bus-width = <8>; + hsync-active = <1>; /* Active high */ + vsync-active = <0>; /* Active low */ + data-active = <1>; /* Active high */ + pclk-sample = <1>; /* Rising */ + }; + }; + }; +}; + &i2c0 { status = "okay";