Re: [PATCH] arm64: dts: sun50i-a64-pinephone: Add front/back cameras

[Ondřej Jirman]

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@xxxxxx>
> > > 
> > > 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.