Re: [RFCv2] Device Tree bindings for OMAP3 Camera System

Hi,

On Mon, Jan 20, 2014 at 11:16:43PM +0100, Sylwester Nawrocki wrote:

On 01/20/2014 05:19 AM, Sakari Ailus wrote:

quoted

I've also been working on this (besides others); what I have however are
mostly experimental patches. [...]

Thanks. I will have a look at it.

[...]

quoted

Over 80 characters per line.

Will be fixed in the next revision.

quoted

quoted

can be found in Documentation/devicetree/bindings/media/video-interfaces.txt

omap3isp node
-------------

Required properties:

- compatible    : should be "ti,omap3isp" for OMAP3.
- reg       : physical addresses and length of the registers set.
- clocks    : list of clock specifiers, corresponding to entries in
          clock-names property.
- clock-names   : must contain "cam_ick", "cam_mclk", "csi2_96m_fck",
          "l3_ick" entries, matching entries in the clocks property.
- interrupts    : must contain mmu interrupt.
- ti,iommu  : phandle to isp mmu.

Is the TI specific? I'd assume not. Hiroshi's patches assume that
at least.

I did not see any iommu standard in Documentation/devicetree/bindings,
so I added the vendor prefix, for now. I don't have strong feelings for
this. I saw you used iommus instead, which sounds reasonable for me.

quoted

quoted

- #address-cells: Should be set to<1>.
- #size-cells   : Should be set to<0>.

The ISP also exports clocks. Shouldn't you add

#clock-cells =<1>;

Ok. I already though about that possibility, but wasn't sure which
way is the cleaner one. Thanks for clarifying.

[...]

This doesn't seem to follow the common clock bindings.

I think it does follow common clock bindings at least. Clocks can
referenced with the following statement:

camera-sensor-0 {
    clocks = <&isp_xclk1>;
    clock-names = ...
};

Instead, you could define value of #clock-cells to be 1 and allow clocks
consumers to reference the provider node in a standard way, e.g.:

This also works and probably better. I will merge clock provider
into the main omap3isp node.

[...]

quoted

quoted

endpoint subnode for serial interfaces
--------------------------------------

Required properties:
 - ti,isp-interface-type    : should be one of the following values

I think the interface type should be standardised at V4L2 level. We
currently do not do that, but instead do a little bit of guessing.

I'm all for such a standard property. It seems much more clear to use such
a property. And I already run into issues with deriving the bus interface
type from existing properties, please see https://linuxtv.org/patch/19937

I assume it would be fine to add a string type property like
"interface-type"
or "bus-type".

quoted

quoted

  *<0>  to use the phy in CSI mode
  *<1>  to use the phy in CCP mode
  *<2>  to use the phy in CCP mode, but configured for MIPI CSI2

mh... from what I understand a port can be configured to be either
CSI2 or CPP2 type. If CCP2 type is chosen the port can be configured
to be CSI1 mode instead of actually being CPP2. See

see "struct isp_ccp2_platform_data" in include/media/omap3isp.h.

But actually I made a typo above. This is what I meant:

*<0>  to use the phy in MIPI CSI2 mode
*<1>  to use the phy in SMIA CCP2 mode
*<2>  to use the phy in SMIA CCP2 mode, but configured for MIPI CSI1

I'm not sure if this can be properly be described in a standardized
type property.

quoted

Hmm. I'm not entirely sure what does this last option mean. I could be
forgetting something, though.

I hope the above description helped.

quoted

quoted

 - ti,isp-clock-divisor     : integer used for configuration of the
                  video port output clock control.

Optional properties:
 - ti,disable-crc       : boolean, which disables crc checking.

I think crc should be standardised as well.

Definitely something we should have a common definition for.

ok.

quoted

quoted

 - ti,strobe-mode       : boolean, which setups data/strobe physical
                  layer instead of data/clock physical layer.
 - pclk-sample          : integer describing if clk should be interpreted on
                  rising (<1>) or falling edge (<0>). Default is<1>.

I see different values on the N9 platform data for CCP2 and CSI2 (front and
back camera). I'm not sure the bus type is related to this or not.

Not sure, what you mean here.

I merged (isp_ccp2_platform_data.phy_layer) into the bus-type
property described above.

quoted

quoted

- data-lanes: an array of physical data lane indexes. Position of an entry
  determines the logical lane number, while the value of an entry indicates
  physical lane, e.g. for 2-lane MIPI CSI-2 bus we could have
  "data-lanes =<1 2>;", assuming the clock lane is on hardware lane 0.
  This property is valid for serial busses only (e.g. MIPI CSI-2).
- clock-lanes: an array of physical clock lane indexes. Position of an entry
  determines the logical lane number, while the value of an entry indicates
  physical lane, e.g. for a MIPI CSI-2 bus we could have "clock-lanes =<0>;",
  which places the clock lane on hardware lane 0. This property is valid for
  serial busses only (e.g. MIPI CSI-2). Note that for the MIPI CSI-2 bus this
  array contains only one entry.

I'd rather refer to
Documentation/devicetree/bindings/media/video-interfaces.txt than copy from
it. It's important that there's a single definition for the standard
properties. Just mentioning the property by name should be enough. What do
you think?

+1

sounds fine to me. Something like this?

- data-lanes: see [0]
- clock-lanes: see [0]

[0] Documentation/devicetree/bindings/media/video-interfaces.txt

quoted

quoted

Example for Nokia N900
----------------------

omap3isp: isp@480BC000 {
    compatible = "ti,omap3isp";
    reg =<0x480BC000 0x070>, /* base */
        <0x480BC100 0x078>, /* cbuf */
        <0x480BC400 0x1F0>, /* cpp2 */
        <0x480BC600 0x0A8>, /* ccdc */
        <0x480BCA00 0x048>, /* hist */
        <0x480BCC00 0x060>, /* h3a  */
        <0x480BCE00 0x0A0>, /* prev */
        <0x480BD000 0x0AC>, /* resz */
        <0x480BD200 0x0FC>, /* sbl  */
        <0x480BD400 0x070>; /* mmu  */

Mmu is a separate device. (Please see my patches.)

Ok.

I simply took over the memory ranges currently defined in the
omap3isp driver.

quoted

quoted

    clocks =<&cam_ick>,
         <&cam_mclk>,
         <&csi2_96m_fck>,
         <&l3_ick>;
    clock-names = "cam_ick",
              "cam_mclk",
              "csi2_96m_fck",
              "l3_ick";

    interrupts =<24>;

    ti,iommu =<&mmu_isp>;

quoted

quoted

    isp_xclk1: isp-xclk@0 {
        compatible = "ti,omap3-isp-xclk";
        reg =<0>;
        #clock-cells =<0>;
    };

    isp_xclk2: isp-xclk@1 {
        compatible = "ti,omap3-isp-xclk";
        reg =<1>;
        #clock-cells =<0>;
    };

I think these whole 2 nodes could be omitted...

quoted

quoted

    #address-cells =<1>;
    #size-cells =<0>;

.. if you add here:

	#clock-cells =<1>;

will do.

quoted

quoted

    port@0 {
        reg =<0>;

        /* parallel interface is not used on Nokia N900 */
        parallel_ep: endpoint {};
    };

    port@1 {
        reg =<1>;

        csi1_ep: endpoint {
            remote-endpoint =<&switch_in>;
            ti,isp-clock-divisor =<1>;
            ti,strobe-mode;
        };
    }

    port@2 {
        reg =<2>;

        /* second serial interface is not used on Nokia N900 */
        csi2_ep: endpoint {};
    }
};

camera-switch {
    /*
     * TODO:
     *  - check if the switching code is generic enough to use a
     *    more generic name like "gpio-camera-switch".
     *  - document the camera-switch binding
     */
    compatible = "nokia,n900-camera-switch";

Indeed. I don't think the hardware engineers realised what kind of a long
standing issue they created for us when they chose that solution. ;)

Writing a small driver for this that exports a sub-device would probably be
the best option as this is hardly very generic. Should this be shown to the
user space or not? Probably it'd be nice to avoid showing the related sub-device
if there would be one.

Probably we should avoid exposing such a hardware detail to user space.
OTOH it would be easy to handle as a media entity through the media
controller API.

If this is exposed to the userspace, then a userspace application
"knows", that it cannot use both cameras at the same time. Otherwise
it can just react to error messages when it tries to use the second
camera.

quoted

I'm still trying to get N9 support working first, the drivers are in a
better shape and there are no such hardware hacks.

quoted

    gpios =<&gpio4 1>; /* 97 */

I think the binding should be defining how state of the GPIO corresponds
to state of the mux.

Obviously it should be mentioned in the n900-camera-switch binding
Documentation. This document was just the proposal for the omap3isp
node :)

quoted

quoted

    port@0 {
        switch_in: endpoint {
            remote-endpoint =<&csi1_ep>;
        };

        switch_out1: endpoint {
            remote-endpoint =<&et8ek8>;
        };

        switch_out2: endpoint {
            remote-endpoint =<&smiapp_dfl>;
        };
    };

This won't work, since names of the nodes are identical they will be
combined
by the dtc into a single 'endpoint' node with single
'remote-endpoint' property
- might not be exactly something that you want.
So it could be rewritten like:

right.

[...]
However, simplifying a bit, the 'endpoint' nodes are supposed to describe
the configuration of a bus interface (port) for a specific remote device.
Then what you need might be something like:

 camera-switch {
	compatible = "nokia,n900-camera-switch";

	#address-cells = <1>;
	#size-cells = <0>;

	switch_in: port@0 {
		reg = <0>;
		endpoint {
			remote-endpoint =<&csi1_ep>;
		};
	};

        switch_out1: port@1 {
		reg = <1>;
		endpoint {
			remote-endpoint =<&et8ek8>;
		};
	};

	switch_out2: port@2 {
		endpoint {
			reg = <2>;
			remote-endpoint =<&smiapp_dfl>;
		};
	};
 };

sounds fine to me.

I'm just wondering if we need to be describing this in DT in such
detail.

Do you have an alternative suggestion for the N900's bus switch
hack?

-- Sebastian