From: Ondřej Jirman <[email protected]>
Pinephone has OV5640 back camera and GC2145 front camera. Add support
for both.
Signed-off-by: Ondrej Jirman <[email protected]>
Signed-off-by: Andrey Skvortsov <[email protected]>
---
.../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 Andrey,
On Wed, Jan 24, 2024 at 12:47:29AM +0300, Andrey Skvortsov wrote:
> From: Ondřej Jirman <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Andrey Skvortsov <[email protected]>
> ---
> .../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 <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Andrey Skvortsov <[email protected]>
> > ---
> > .../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
--
Best regards,
Andrey Skvortsov
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 <[email protected]>
> > >
> > > 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 <[email protected]>