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

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4941 bytes --]

Hi,

On Tue, Oct 01, 2019 at 05:04:40PM +0800, Chen-Yu Tsai wrote:
> On Tue, Oct 1, 2019 at 4:52 PM Maxime Ripard <mripard@kernel.org> 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@csie.org> wrote:
> > > >
> > > > Hi,
> > > >
> > > > Sorry for chiming in so late.
> > > >
> > > > On Thu, Jul 11, 2019 at 8:15 PM Maxime Ripard <maxime.ripard@bootlin.com> 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@kernel.org>
> > > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > > > > ---
> > > > >  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@csie.org>
> > > > > +  - Maxime Ripard <maxime.ripard@bootlin.com>
> > > > > +
> > > > > +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.

Looking at the Allwinner BSP again, it looks like you're right. I'll
send some patches to remove that clock from the binding.

Thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]