Re: [PATCH v6 1/5] dt-bindings: media: Add Allwinner A10 CSI binding

[Chen-Yu Tsai]

On Tue, Oct 1, 2019 at 4:52 PM Maxime Ripard <mripard@xxxxxxxxxx> wrote:
>
> Hi,
>
> Thanks for looking into this.
>
> On Sun, Sep 15, 2019 at 04:54:16PM +0800, Chen-Yu Tsai wrote:
> > On Thu, Aug 15, 2019 at 4:34 PM Chen-Yu Tsai <wens@xxxxxxxx> wrote:
> > >
> > > Hi,
> > >
> > > Sorry for chiming in so late.
> > >
> > > On Thu, Jul 11, 2019 at 8:15 PM Maxime Ripard <maxime.ripard@xxxxxxxxxxx> wrote:
> > > >
> > > > The Allwinner A10 CMOS Sensor Interface is a camera capture interface also
> > > > used in later (A10s, A13, A20, R8 and GR8) SoCs.
> > > >
> > > > On some SoCs, like the A10, there's multiple instances of that controller,
> > > > with one instance supporting more channels and having an ISP.
> > > >
> > > > Reviewed-by: Rob Herring <robh@xxxxxxxxxx>
> > > > Signed-off-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxx>
> > > > ---
> > > >  Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 94 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml b/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml
> > > > new file mode 100644
> > > > index 000000000000..97c9fc3b5050
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml
> > > > @@ -0,0 +1,94 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/arm/allwinner,sun4i-a10-csi.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Allwinner A10 CMOS Sensor Interface (CSI) Device Tree Bindings
> > > > +
> > > > +maintainers:
> > > > +  - Chen-Yu Tsai <wens@xxxxxxxx>
> > > > +  - Maxime Ripard <maxime.ripard@xxxxxxxxxxx>
> > > > +
> > > > +description: |-
> > > > +  The Allwinner A10 and later has a CMOS Sensor Interface to retrieve
> > > > +  frames from a parallel or BT656 sensor.
> > > > +
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    oneOf:
> > > > +      - items:
> > > > +          - enum:
> > > > +              - allwinner,sun7i-a20-csi0
> > > > +          - const: allwinner,sun4i-a10-csi0
> > >
> > > CSI0 on the A10 has an ISP. Do we know if the one in the A20 does
> > > as well? It certainly doesn't say so in the user manual. If not,
> > > then we can't claim that A20 CSI0 is compatible with A10 CSI0.
> > >
> > > > +
> > > > +      - items:
> > > > +          - const: allwinner,sun4i-a10-csi0
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  interrupts:
> > > > +    maxItems: 1
> > > > +
> > > > +  clocks:
> > > > +    items:
> > > > +      - description: The CSI interface clock
> > > > +      - description: The CSI module clock
> > > > +      - description: The CSI ISP clock
> > > > +      - description: The CSI DRAM clock
> > > > +
> > > > +  clock-names:
> > > > +    items:
> > > > +      - const: bus
> > > > +      - const: mod
> > >
> > > I doubt this actually is a module clock. Based on the usage in your
> > > device tree patch, and the csi driver in the old linux-sunxi kernel,
> > > the clock rate is set to 24 MHz, or whatever the sensor requires for
> > > MCLK.
> >
> > I'm working on adding support for this on the R40, and it seems with
> > this SoC the picture is much clearer. It has the same CSI interface
> > block, but the CCU has the clocks correctly named. We have:
> >
> >   - CSI_MCLK0
> >   - CSI_MCLK1
> >   - CSI_SCLK
> >
> > in addition to the bus clocks.
> >
> > The CSI section also explains the clock signals:
> >
> >     6.1.3.2. Clock Sources
> >     Two Clocks need to be configured for CSI controller. CSI0/1_MCLK
> >     provides the master clock for sensor and other devices. CSI_SCLK
> >     is the top clock for the whole CSI module.
> >
> > So it would seem the ISP clock we currently have in the DT is simply
> > the module clock shared by all CSI-related hardware blocks, and the
> > module clock is bogus.
>
> I don't think it is. It looks like there's no ISP in the R40 CSI
> controllers, so that would mean that we don't have an ISP clock, and
> the SCLK is the module clock.
>
> Does that make sense?

Right. That's another way to put it. The point is I believe the
CSI[01]_CLK clocks on the A10/A20 are simply the MCLK outputs.

ChenYu