Hi,
On 01/20/2014 05:19 AM, Sakari Ailus wrote:
Hi Sebastian,
I've also been working on this (besides others); what I have however are
mostly experimental patches. I'm also using patches from Laurent, Florian
Vaussard (IOMMU) and Hiroshi Doyu (IOMMU as well). My tree is here:
<URL:http://vihersipuli.retiisi.org.uk/cgi-bin/gitweb.cgi?p=~sailus/linux.git;a=summary>
The branch is rm696-035-dt. The intent is to get the N9/N950 camera support
to mainline. Most patches in that branch are experimental. What is missing
entirely is constucting subdev groups off of the devices found. The rest
will likely contain lots of bugs.
Seems like you've started with documentation which I'm lacking entirely.
Good! :)
You might want to take a look at least at this one as well:
<URL:http://vihersipuli.retiisi.org.uk/cgi-bin/gitweb.cgi?p=~sailus/linux.git;a=commitdiff;h=58f7754099539178f47dcca97981bf2ba4c73e54>
On Wed, Jan 15, 2014 at 08:41:28PM +0100, Sebastian Reichel wrote:
Hi,
I finally found some time to update the proposed binding
documentation for omap3isp according to the comments on RFCv1.
Changes since v1:
* Use common DT clock bindings to provide isp-xclk
* Use common DT video-interface bindings to specify device connections
* Apply style updates suggested by Mark Rutland
* I renamed ti,enable-crc to ti,disable-crc, since I think its supposed
to be used for remote hw adding broken crc values. I can't see other
reasons for disabling it :)
* I've kept the clocks the same for now. I think somebody else should look
over them. Changing the actual referenced clocks / renaming them is just
a small change and can be done at a later time in the omap3isp DT process
IMHO.
* I've kept the reg reference as a list for now, since that's how the
omap3isp driver currently works and I can't see any disadvantages
in making the memory segmentation visible in the DTS file.
So here is the proposed DT binding documentation:
Binding for the OMAP3 Camera subsystem with the image signal processor (ISP)
feature. The binding follows the common video-interfaces Device Tree binding,
so it is recommended to read the common binding description first. This description
Over 80 characters per line.
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.
- #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>;
If we treat the omap3isp node as the clock provider node then yes, it
should be added. And I think it is reasonable to do so, instead of e.g.
adding a separate DT node for the clocks provider.
Optional properties:
- vdd-csiphy1-supply : regulator for csi phy1
- vdd-csiphy2-supply : regulator for csi phy2
isp-xclk subnode
----------------
The omap3 ISP provides two external clocks, which are available as subnodes of
the omap3isp node. Devices using one of these clock devices are supposed to follow
the common Device Tree clock bindings described in
Documentation/devicetree/bindings/clock/clock-bindings.txt
Required properties:
- compatible : should contain "ti,omap3-cam-xclk"
- reg : should be set to
*<0> for cam_xclka
*<1> for cam_xclkb
- #clock-cells : should be set to<0>
Or to do this instead. I'm not an expert (or perhaps not even a little less)
in DT so I don't know. The ISP is still a single device, and I think you
might not need these to just refer to the clocks from elsewhere in the DT.
This doesn't seem to follow the common clock bindings.
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.:
/* clk provider node */
omap3isp-clocks {
#clock-cells = <1>;
};
camera-sensor-0 {
clocks = <&omap3isp-clocks 0>; /* XCLK A */
clock-names = ...
};
camera-sensor-1 {
clocks = <&omap3isp-clocks 1>; /* XCLK B */
clock-names = ...
};
port subnode
------------
The omap3 ISP provides three different physical interfaces for camera
connections. Each of them is described using a port node.
Required properties:
- reg : Should be set to one of the following values
*<0> for the parallel interface (CPI)
*<1> for the first serial interface (CSI1)
*<2> for the second serial interface (CSI2)
There are two PHYs in 3630 but three receivers. Currently the two are
combined in the configuration. I think I agree with your approach.
endpoint subnode for parallel interface
---------------------------------------
The endpoint subnode describes the connection between the port and the remote
camera device.
Required properties:
- remote-endpoint : phandle to remote device
Optional properties:
- data-shift : integer describing how far the data lanes are shifted.
- pclk-sample : integer describing if clk should be interpreted on
rising (<1>) or falling edge (<0>). Default is<1>.
- hsync-active : integer describing if hsync is active high (<1>) or
active low (<0>). Default is<1>.
- vsync-active : integer describing if vsync is active high (<1>) or
active low (<0>). Default is<1>.
- ti,data-ones-complement : boolean, describing that the data's polarity is
one's complement.
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".
*<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
Hmm. I'm not entirely sure what does this last option mean. I could be
forgetting something, though.
- 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.
- 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.
- 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
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.)
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>;
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...
#address-cells =<1>;
#size-cells =<0>;
.. if you add here:
#clock-cells =<1>;
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.
I'm still trying to get N9 support working first, the drivers are in a
better shape and there are no such hardware hacks.
gpios =<&gpio4 1>; /* 97 */
I think the binding should be defining how state of the GPIO corresponds
to state of the mux.
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:
port@0 {
#address-cells = <1>;
#size-cells = <0>;
switch_in: endpoint@0 {
reg = <0>;
remote-endpoint =<&csi1_ep>;
};
switch_out1: endpoint@1 {
reg = <1>;
remote-endpoint =<&et8ek8>;
};
switch_out2: endpoint@2 {
reg = <2>;
remote-endpoint =<&smiapp_dfl>;
};
};
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>;
};
};
};
I'm just wondering if we need to be describing this in DT in such detail.
--
Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
