| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZbEB0TCTkzIYnLZD@skv.local>
Date: Wed, 24 Jan 2024 15:25:53 +0300
From: Andrey Skvortsov <andrej.skvortzov@...il.com>
To: Ondřej Jirman <megi@....cz>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>, Chen-Yu Tsai <wens@...e.org>,
Jernej Skrabec <jernej.skrabec@...il.com>,
Samuel Holland <samuel@...lland.org>, devicetree@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
linux-kernel@...r.kernel.org,
Alain Volmat <alain.volmat@...s.st.com>,
Pavel Machek <pavel@....cz>,
Arnaud Ferraris <arnaud.ferraris@...labora.com>
Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>,
Hans Verkuil <hverkuil-cisco@...all.nl>
Subject: Re: [PATCH] arm64: dts: sun50i-a64-pinephone: Add front/back cameras
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@....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@....cz>
> > Signed-off-by: Andrey Skvortsov <andrej.skvortzov@...il.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
--
Best regards,
Andrey Skvortsov
Powered by blists - more mailing lists