On Fri, Dec 7, 2018 at 1:17 PM Maxime Ripard <maxime.ripard@xxxxxxxxxxx> wrote:
>
> On Thu, Dec 06, 2018 at 06:07:59PM +0100, Michael Nazzareno Trimarchi wrote:
> > On Thu, Dec 6, 2018 at 4:34 PM Maxime Ripard <maxime.ripard@xxxxxxxxxxx> wrote:
> > > On Thu, Dec 06, 2018 at 06:53:05PM +0530, Jagan Teki wrote:
> > > > Allwinner A64 CSI controller has similar features as like in
> > > > H3, So add support for A64 via H3 fallback.
> > > >
> > > > Also updated CSI_SCLK to use 300MHz via assigned-clocks, since
> > > > the default clock 600MHz seems unable to drive the sensor(ov5640)
> > > > to capture the image.
> > > >
> > > > Signed-off-by: Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx>
> > > > ---
> > > > Changes for v2:
> > > > - Use CSI_SCLK to 300MHz
> > > >
> > > > arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 23 +++++++++++++++++++
> > > > 1 file changed, 23 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > index 384c417cb7a2..d7ab0006ebce 100644
> > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > @@ -532,6 +532,12 @@
> > > > interrupt-controller;
> > > > #interrupt-cells = <3>;
> > > >
> > > > + csi_pins: csi-pins {
> > > > + pins = "PE0", "PE2", "PE3", "PE4", "PE5", "PE6",
> > > > + "PE7", "PE8", "PE9", "PE10", "PE11";
> > > > + function = "csi0";
> > > > + };
> > > > +
> > > > i2c0_pins: i2c0_pins {
> > > > pins = "PH0", "PH1";
> > > > function = "i2c0";
> > > > @@ -899,6 +905,23 @@
> > > > status = "disabled";
> > > > };
> > > >
> > > > + csi: csi@1cb0000 {
> > > > + compatible = "allwinner,sun50i-a64-csi",
> > > > + "allwinner,sun8i-h3-csi";
> > > > + reg = <0x01cb0000 0x1000>;
> > > > + interrupts = <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>;
> > > > + clocks = <&ccu CLK_BUS_CSI>,
> > > > + <&ccu CLK_CSI_SCLK>,
> > > > + <&ccu CLK_DRAM_CSI>;
> > > > + clock-names = "bus", "mod", "ram";
> > > > + resets = <&ccu RST_BUS_CSI>;
> > > > + pinctrl-names = "default";
> > > > + pinctrl-0 = <&csi_pins>;
> > > > + assigned-clocks = <&ccu CLK_CSI_SCLK>;
> > > > + assigned-clock-rates = <300000000>;
> > >
> > > That should be enforced in the driver.
> > >
> >
> > We are not really sure what is the best here. Our first idea was to
> > put in the board file and then Jagan
> > decide to put in dtsi. We don't have enough coverage of camera on this
> > CPU and I prefer to stay with this
> > minimal change that does not impact the driver.
>
> The thing is that:
> - in this commit log, you're stating that it depends on the sensor,
> which indicates that this would be a board level addition
> - In another patch series, Jagan reported IIRC that it actually
> depends on the resolution, so it doesn't belong in the DT at all
> - And then, you don't even have any guarantee on the clock rate. The
> sole guarantee you have is that when your driver will probe, the
> rate will be close to those 300MHz. That's it. It might completely
> change after the driver has probed, or be rounded to something
> else entirely, who knows.
Let's to be clear.
- with default clock(600MHz) the sensor get probed but image capture
has an issue.
- with 300MHz the image capture working with 320x240@30, 640x480@30,
640x480@60, 1280x720@30 with UYVY8_2X8 and YUYV8_2X8 formats but
1080p@30 seems broken (the same I have mentioned in another mail)
- since all this is verified on ov5640, I have mentioned the same
thing on commit message for future reference.
