* [early RFC] Device Tree bindings for OMAP3 Camera Subsystem
@ 2013-11-03 22:03 Sebastian Reichel
2013-11-04 19:49 ` jean-philippe francois
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Sebastian Reichel @ 2013-11-03 22:03 UTC (permalink / raw)
To: Laurent Pinchart, linux-media
Cc: devicetree, Rob Herring, Pawel Moll, Mark Rutland,
Stephen Warren, Ian Campbell, Sebastian Reichel
[-- Attachment #1: Type: text/plain, Size: 4324 bytes --]
Hi,
This is an early RFC for omap3isp DT support. For now i just created a potential DT
binding documentation based on the existing platform data:
Binding for the OMAP3 Camera subsystem with the image signal processor (ISP) feature.
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;
Optional properties:
- VDD_CSIPHY1-supply : regulator for csi phy1
- VDD_CSIPHY2-supply : regulator for csi phy2
- ti,isp-xclk-1 : device(s) attached to ISP's first external clock
- ti,isp-xclk-2 : device(s) attached to ISP's second external clock
device-group subnode
--------------------
Required properties:
- ti,isp-interface-type : Integer describing the interface type, one of the following
* 0 = ISP_INTERFACE_PARALLEL
* 1 = ISP_INTERFACE_CSI2A_PHY2
* 2 = ISP_INTERFACE_CCP2B_PHY1
* 3 = ISP_INTERFACE_CCP2B_PHY2
* 4 = ISP_INTERFACE_CSI2C_PHY1
- ti,isp-devices : Array of phandles to devices connected via the interface
- One of the following configuration nodes (depending on ti,isp-interface-type)
- ti,ccp2-bus-cfg : CCP2 bus configuration (needed for ISP_INTERFACE_CCP*)
- ti,parallel-bus-cfg : PARALLEL bus configuration (needed for ISP_INTERFACE_PARALLEL)
- ti,csi2-bus-cfg : CSI bus configuration (needed for ISP_INTERFACE_CSI*)
ccp2-bus-cfg subnode
--------------------
Required properties:
- ti,video-port-clock-divisor : integer; used for video port output clock control
Optional properties:
- ti,inverted-clock : boolean; clock/strobe signal is inverted
- ti,enable-crc : boolean; enable crc checking
- ti,ccp2-mode-mipi : boolean; port is used in MIPI-CSI1 mode (default: CCP2 mode)
- ti,phy-layer-is-strobe : boolean; use data/strobe physical layer (default: data/clock physical layer)
- ti,data-lane-configuration : integer array with position and polarity information for lane 1 and 2
- ti,clock-lane-configuration : integer array with position and polarity information for clock lane
parallel-bus-cfg subnode
------------------------
Required properties:
- ti,data-lane-shift : integer; shift data lanes by this amount
Optional properties:
- ti,clock-falling-edge : boolean; sample on falling edge (default: rising edge)
- ti,horizontal-synchronization-active-low : boolean; default: active high
- ti,vertical-synchronization-active-low : boolean; default: active high
- ti,data-polarity-ones-complement : boolean; data polarity is one's complement
csi2-bus-cfg subnode
--------------------
Required properties:
- ti,video-port-clock-divisor : integer; used for video port output clock control
Optional properties:
- ti,data-lane-configuration : integer array with position and polarity information for lane 1 and 2
- ti,clock-lane-configuration : integer array with position and polarity information for clock lane
- ti,enable-crc : boolean; enable crc checking
Example for Nokia N900
----------------------
omap3isp: isp@480BC000 {
compatible = "ti,omap3isp";
reg = <
/* OMAP3430+ */
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 */
>;
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>;
ti,isp-xclk-1 = <
&et8ek8
&smiapp_dfl
>;
group1: device-group@0 {
ti,isp-interface-type = <2>;
ti,isp-devices = <
&et8ek8
&ad5820
&adp1653
>;
ti,ccp2-bus-cfg {
ti,enable-crc;
ti,phy-layer-is-strobe;
ti,video-port-clock-divisor = <1>;
};
};
group2: device-group@1 {
ti,isp-interface-type = <2>;
ti,isp-devices = <
&smiapp_dfl
>;
ti,ccp2-bus-cfg {
ti,enable-crc;
ti,phy-layer-is-strobe;
ti,video-port-clock-divisor = <1>;
};
};
};
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [early RFC] Device Tree bindings for OMAP3 Camera Subsystem
2013-11-03 22:03 [early RFC] Device Tree bindings for OMAP3 Camera Subsystem Sebastian Reichel
@ 2013-11-04 19:49 ` jean-philippe francois
2013-11-06 0:48 ` Laurent Pinchart
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: jean-philippe francois @ 2013-11-04 19:49 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Laurent Pinchart, linux-media, devicetree, Rob Herring,
Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell
013/11/3 Sebastian Reichel <sre@debian.org>:
> Hi,
>
> This is an early RFC for omap3isp DT support. For now i just created a potential DT
> binding documentation based on the existing platform data:
>
> Binding for the OMAP3 Camera subsystem with the image signal processor (ISP) feature.
>
This is very interesting, I am in the process of transforming an (out
of tree) machine board file into
a device tree description, and I was precisely searching for "oma3isp
dt" when I saw your mail.
I would be happy to test or help develop any patch aiming at DT
support for omap3isp. I am new to DT, so I
will leave the DT bindings review to people that actually have a clue.
I am looking forward to testing patches and bugging you when things break ;)
Regards,
Jean-Philippe François
> 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;
>
> Optional properties:
>
> - VDD_CSIPHY1-supply : regulator for csi phy1
> - VDD_CSIPHY2-supply : regulator for csi phy2
> - ti,isp-xclk-1 : device(s) attached to ISP's first external clock
> - ti,isp-xclk-2 : device(s) attached to ISP's second external clock
>
> device-group subnode
> --------------------
>
> Required properties:
> - ti,isp-interface-type : Integer describing the interface type, one of the following
> * 0 = ISP_INTERFACE_PARALLEL
> * 1 = ISP_INTERFACE_CSI2A_PHY2
> * 2 = ISP_INTERFACE_CCP2B_PHY1
> * 3 = ISP_INTERFACE_CCP2B_PHY2
> * 4 = ISP_INTERFACE_CSI2C_PHY1
> - ti,isp-devices : Array of phandles to devices connected via the interface
> - One of the following configuration nodes (depending on ti,isp-interface-type)
> - ti,ccp2-bus-cfg : CCP2 bus configuration (needed for ISP_INTERFACE_CCP*)
> - ti,parallel-bus-cfg : PARALLEL bus configuration (needed for ISP_INTERFACE_PARALLEL)
> - ti,csi2-bus-cfg : CSI bus configuration (needed for ISP_INTERFACE_CSI*)
>
> ccp2-bus-cfg subnode
> --------------------
>
> Required properties:
> - ti,video-port-clock-divisor : integer; used for video port output clock control
>
> Optional properties:
> - ti,inverted-clock : boolean; clock/strobe signal is inverted
> - ti,enable-crc : boolean; enable crc checking
> - ti,ccp2-mode-mipi : boolean; port is used in MIPI-CSI1 mode (default: CCP2 mode)
> - ti,phy-layer-is-strobe : boolean; use data/strobe physical layer (default: data/clock physical layer)
> - ti,data-lane-configuration : integer array with position and polarity information for lane 1 and 2
> - ti,clock-lane-configuration : integer array with position and polarity information for clock lane
>
> parallel-bus-cfg subnode
> ------------------------
>
> Required properties:
> - ti,data-lane-shift : integer; shift data lanes by this amount
>
> Optional properties:
> - ti,clock-falling-edge : boolean; sample on falling edge (default: rising edge)
> - ti,horizontal-synchronization-active-low : boolean; default: active high
> - ti,vertical-synchronization-active-low : boolean; default: active high
> - ti,data-polarity-ones-complement : boolean; data polarity is one's complement
>
> csi2-bus-cfg subnode
> --------------------
>
> Required properties:
> - ti,video-port-clock-divisor : integer; used for video port output clock control
>
> Optional properties:
> - ti,data-lane-configuration : integer array with position and polarity information for lane 1 and 2
> - ti,clock-lane-configuration : integer array with position and polarity information for clock lane
> - ti,enable-crc : boolean; enable crc checking
>
> Example for Nokia N900
> ----------------------
>
> omap3isp: isp@480BC000 {
> compatible = "ti,omap3isp";
> reg = <
> /* OMAP3430+ */
> 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 */
> >;
>
> 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>;
>
> ti,isp-xclk-1 = <
> &et8ek8
> &smiapp_dfl
> >;
>
> group1: device-group@0 {
> ti,isp-interface-type = <2>;
>
> ti,isp-devices = <
> &et8ek8
> &ad5820
> &adp1653
> >;
>
> ti,ccp2-bus-cfg {
> ti,enable-crc;
> ti,phy-layer-is-strobe;
> ti,video-port-clock-divisor = <1>;
> };
> };
>
> group2: device-group@1 {
> ti,isp-interface-type = <2>;
>
> ti,isp-devices = <
> &smiapp_dfl
> >;
>
> ti,ccp2-bus-cfg {
> ti,enable-crc;
> ti,phy-layer-is-strobe;
> ti,video-port-clock-divisor = <1>;
> };
> };
> };
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [early RFC] Device Tree bindings for OMAP3 Camera Subsystem
2013-11-03 22:03 [early RFC] Device Tree bindings for OMAP3 Camera Subsystem Sebastian Reichel
2013-11-04 19:49 ` jean-philippe francois
@ 2013-11-06 0:48 ` Laurent Pinchart
2013-11-15 17:18 ` Mark Rutland
2014-01-15 19:41 ` [RFCv2] Device Tree bindings for OMAP3 Camera System Sebastian Reichel
3 siblings, 0 replies; 13+ messages in thread
From: Laurent Pinchart @ 2013-11-06 0:48 UTC (permalink / raw)
To: Sebastian Reichel
Cc: linux-media, devicetree, Rob Herring, Pawel Moll, Mark Rutland,
Stephen Warren, Ian Campbell
[-- Attachment #1: Type: text/plain, Size: 5903 bytes --]
Hi Sebastian,
Thank you for the aptch.
On Sunday 03 November 2013 23:03:15 Sebastian Reichel wrote:
> Hi,
>
> This is an early RFC for omap3isp DT support. For now i just created a
> potential DT binding documentation based on the existing platform data:
>
> Binding for the OMAP3 Camera subsystem with the image signal processor (ISP)
> feature.
>
> 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;
According to the OMAP36xx TRM, the ISP functional and interface clocks are
called CAM_FCLK and CAM_ICLK. They are driven by L3_ICLK and L4_ICLK
respectively, and both gated through a single bit.
The OMAP platform code instantiates a cam_ick clock for CAM_ICLK but doesn't
create any clock for CAM_FCLK. The reason is probably that such a clock wasn't
really needed, as enabling the interface clock enables the functional clock
anyway.
Now that we're moving to DT the clock names will be set in stone, so maybe we
should think about them a bit. Would it make sense to rename the clocks
according to the names used in the OMAP36xx TRM ? We should probably check the
documentation of the other SoCs in which the ISP is used to verify whether the
names match. Would it also make sense to create an FCLK clock and use it
instead of l3_ick ?
> - interrupts : must contain mmu interrupt;
> - ti,iommu : phandle to isp mmu;
Are there DT bindings for the IOMMU ? They don't seem to be present in
mainline.
> Optional properties:
>
> - VDD_CSIPHY1-supply : regulator for csi phy1
> - VDD_CSIPHY2-supply : regulator for csi phy2
Should the regulators be renamed to lower-case ?
> - ti,isp-xclk-1 : device(s) attached to ISP's first external clock
> - ti,isp-xclk-2 : device(s) attached to ISP's second external clock
That information should be present in the clock client node, not the ISP node.
> device-group subnode
> --------------------
>
> Required properties:
> - ti,isp-interface-type : Integer describing the interface type, one of the
> following * 0 = ISP_INTERFACE_PARALLEL
> * 1 = ISP_INTERFACE_CSI2A_PHY2
> * 2 = ISP_INTERFACE_CCP2B_PHY1
> * 3 = ISP_INTERFACE_CCP2B_PHY2
> * 4 = ISP_INTERFACE_CSI2C_PHY1
> - ti,isp-devices : Array of phandles to devices connected via the interface
Is there any reason why you don't use the V4L2 DT bindings to describe the
pipeline ?
> - One of the following configuration nodes (depending on
> ti,isp-interface-type) - ti,ccp2-bus-cfg : CCP2 bus configuration (needed
> for ISP_INTERFACE_CCP*) - ti,parallel-bus-cfg : PARALLEL bus configuration
> (needed for ISP_INTERFACE_PARALLEL) - ti,csi2-bus-cfg : CSI bus
> configuration (needed for ISP_INTERFACE_CSI*)
>
> ccp2-bus-cfg subnode
> --------------------
>
> Required properties:
> - ti,video-port-clock-divisor : integer; used for video port output clock
> control
>
> Optional properties:
> - ti,inverted-clock : boolean; clock/strobe signal is inverted
> - ti,enable-crc : boolean; enable crc checking
> - ti,ccp2-mode-mipi : boolean; port is used in MIPI-CSI1 mode (default:
> CCP2 mode) - ti,phy-layer-is-strobe : boolean; use data/strobe physical
> layer (default: data/clock physical layer) - ti,data-lane-configuration :
> integer array with position and polarity information for lane 1 and 2 -
> ti,clock-lane-configuration : integer array with position and polarity
> information for clock lane
>
> parallel-bus-cfg subnode
> ------------------------
>
> Required properties:
> - ti,data-lane-shift : integer; shift data lanes by this amount
>
> Optional properties:
> - ti,clock-falling-edge : boolean; sample on falling edge
(default:
> rising edge) - ti,horizontal-synchronization-active-low : boolean; default:
> active high - ti,vertical-synchronization-active-low : boolean; default:
> active high - ti,data-polarity-ones-complement : boolean; data polarity
is
> one's complement
>
> csi2-bus-cfg subnode
> --------------------
>
> Required properties:
> - ti,video-port-clock-divisor : integer; used for video port output clock
> control
>
> Optional properties:
> - ti,data-lane-configuration : integer array with position and polarity
> information for lane 1 and 2 - ti,clock-lane-configuration : integer
array
> with position and polarity information for clock lane - ti,enable-crc
:
> boolean; enable crc checking
>
> Example for Nokia N900
> ----------------------
>
> omap3isp: isp@480BC000 {
> compatible = "ti,omap3isp";
> reg = <
> /* OMAP3430+ */
> 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 */
> >;
>
> 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>;
>
> ti,isp-xclk-1 = <
> &et8ek8
> &smiapp_dfl
> >;
>
> group1: device-group@0 {
> ti,isp-interface-type = <2>;
>
> ti,isp-devices = <
> &et8ek8
> &ad5820
> &adp1653
> >;
>
> ti,ccp2-bus-cfg {
> ti,enable-crc;
> ti,phy-layer-is-strobe;
> ti,video-port-clock-divisor = <1>;
> };
> };
>
> group2: device-group@1 {
> ti,isp-interface-type = <2>;
>
> ti,isp-devices = <
> &smiapp_dfl
> >;
>
> ti,ccp2-bus-cfg {
> ti,enable-crc;
> ti,phy-layer-is-strobe;
> ti,video-port-clock-divisor = <1>;
> };
> };
> };
--
Regards,
Laurent Pinchart
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [early RFC] Device Tree bindings for OMAP3 Camera Subsystem
2013-11-03 22:03 [early RFC] Device Tree bindings for OMAP3 Camera Subsystem Sebastian Reichel
2013-11-04 19:49 ` jean-philippe francois
2013-11-06 0:48 ` Laurent Pinchart
@ 2013-11-15 17:18 ` Mark Rutland
2014-01-15 19:41 ` [RFCv2] Device Tree bindings for OMAP3 Camera System Sebastian Reichel
3 siblings, 0 replies; 13+ messages in thread
From: Mark Rutland @ 2013-11-15 17:18 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Laurent Pinchart, linux-media, devicetree, rob.herring,
Pawel Moll, Stephen Warren, Ian Campbell
On Sun, Nov 03, 2013 at 10:03:15PM +0000, Sebastian Reichel wrote:
> Hi,
>
> This is an early RFC for omap3isp DT support. For now i just created a potential DT
> binding documentation based on the existing platform data:
>
> Binding for the OMAP3 Camera subsystem with the image signal processor (ISP) feature.
>
> 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;
s/;/./ (or s/;//)
>
> Optional properties:
>
> - VDD_CSIPHY1-supply : regulator for csi phy1
> - VDD_CSIPHY2-supply : regulator for csi phy2
I'd make these lower-case. Upper case is unusual, and lower-case is
preferred.
> - ti,isp-xclk-1 : device(s) attached to ISP's first external clock
> - ti,isp-xclk-2 : device(s) attached to ISP's second external clock
If the ISP is acting as a clock controller, it should have #clock-cells,
and export clocks to the consumers. They can in turn refer to th ISP via
the standard clocks property.
>
> device-group subnode
> --------------------
>
> Required properties:
> - ti,isp-interface-type : Integer describing the interface type, one of the following
> * 0 = ISP_INTERFACE_PARALLEL
> * 1 = ISP_INTERFACE_CSI2A_PHY2
> * 2 = ISP_INTERFACE_CCP2B_PHY1
> * 3 = ISP_INTERFACE_CCP2B_PHY2
> * 4 = ISP_INTERFACE_CSI2C_PHY1
Are these PHYs always present?
Are they external to the ISP?
It's not possible for several of these to be valid simultaneously?
> - ti,isp-devices : Array of phandles to devices connected via the interface
Which devices are these? This looks backwards to me...
> - One of the following configuration nodes (depending on ti,isp-interface-type)
> - ti,ccp2-bus-cfg : CCP2 bus configuration (needed for ISP_INTERFACE_CCP*)
> - ti,parallel-bus-cfg : PARALLEL bus configuration (needed for ISP_INTERFACE_PARALLEL)
> - ti,csi2-bus-cfg : CSI bus configuration (needed for ISP_INTERFACE_CSI*)
>
> ccp2-bus-cfg subnode
> --------------------
>
> Required properties:
> - ti,video-port-clock-divisor : integer; used for video port output clock control
>
> Optional properties:
> - ti,inverted-clock : boolean; clock/strobe signal is inverted
> - ti,enable-crc : boolean; enable crc checking
Why can't this be a run-time option?
> - ti,ccp2-mode-mipi : boolean; port is used in MIPI-CSI1 mode (default: CCP2 mode)
> - ti,phy-layer-is-strobe : boolean; use data/strobe physical layer (default: data/clock physical layer)
> - ti,data-lane-configuration : integer array with position and polarity information for lane 1 and 2
> - ti,clock-lane-configuration : integer array with position and polarity information for clock lane
In what precise format?
>
> parallel-bus-cfg subnode
> ------------------------
>
> Required properties:
> - ti,data-lane-shift : integer; shift data lanes by this amount
>
> Optional properties:
> - ti,clock-falling-edge : boolean; sample on falling edge (default: rising edge)
> - ti,horizontal-synchronization-active-low : boolean; default: active high
> - ti,vertical-synchronization-active-low : boolean; default: active high
> - ti,data-polarity-ones-complement : boolean; data polarity is one's complement
>
> csi2-bus-cfg subnode
> --------------------
>
> Required properties:
> - ti,video-port-clock-divisor : integer; used for video port output clock control
>
> Optional properties:
> - ti,data-lane-configuration : integer array with position and polarity information for lane 1 and 2
> - ti,clock-lane-configuration : integer array with position and polarity information for clock lane
> - ti,enable-crc : boolean; enable crc checking
Similarly, run-time selectable?
>
> Example for Nokia N900
> ----------------------
>
> omap3isp: isp@480BC000 {
> compatible = "ti,omap3isp";
> reg = <
> /* OMAP3430+ */
> 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 */
> >;
The binding implied a single contiguous reg entry. These look like they
are in a contiguous register space, is it not possible to describe them
via a single large contiguous entry?
Also, please bracket individual entries in a list like so:
reg = <0x0 0x4>,
<0x44 0x27>,
<0x800 0x63>;
It's far easier to read arbitrary lists when they're bracketed
consistently.
>
> clocks = < &cam_ick &cam_mclk &csi2_96m_fck &l3_ick >;
Similarly here.
> clock-names = "cam_ick", "cam_mclk", "csi2_96m_fck", "l3_ick";
>
> interrupts = <24>;
>
> ti,iommu = <&mmu_isp>;
>
> ti,isp-xclk-1 = <
> &et8ek8
> &smiapp_dfl
> >;
And here (though I think this property is unnecessary).
Thanks,
Mark.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFCv2] Device Tree bindings for OMAP3 Camera System
2013-11-03 22:03 [early RFC] Device Tree bindings for OMAP3 Camera Subsystem Sebastian Reichel
` (2 preceding siblings ...)
2013-11-15 17:18 ` Mark Rutland
@ 2014-01-15 19:41 ` Sebastian Reichel
2014-01-20 4:19 ` Sakari Ailus
3 siblings, 1 reply; 13+ messages in thread
From: Sebastian Reichel @ 2014-01-15 19:41 UTC (permalink / raw)
To: Laurent Pinchart, linux-media
Cc: devicetree, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell
[-- Attachment #1: Type: text/plain, Size: 7562 bytes --]
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
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.
- #address-cells: Should be set to <1>.
- #size-cells : Should be set to <0>.
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>
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)
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
* <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
- 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.
- 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>.
- 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.
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 */
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>;
};
#address-cells = <1>;
#size-cells = <0>;
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";
gpios = <&gpio4 1>; /* 97 */
port@0 {
switch_in: endpoint {
remote-endpoint = <&csi1_ep>;
};
switch_out1: endpoint {
remote-endpoint = <&et8ek8>;
};
switch_out2: endpoint {
remote-endpoint = <&smiapp_dfl>;
};
};
};
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFCv2] Device Tree bindings for OMAP3 Camera System
2014-01-15 19:41 ` [RFCv2] Device Tree bindings for OMAP3 Camera System Sebastian Reichel
@ 2014-01-20 4:19 ` Sakari Ailus
2014-01-20 22:16 ` Sylwester Nawrocki
0 siblings, 1 reply; 13+ messages in thread
From: Sakari Ailus @ 2014-01-20 4:19 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Laurent Pinchart, linux-media, devicetree, Rob Herring,
Pawel Moll, Mark Rutland, Ian Campbell
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>;
> 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.
> 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.
> * <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.
> - 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?
> 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>;
> };
>
> #address-cells = <1>;
> #size-cells = <0>;
>
> 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.
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 */
>
> port@0 {
> switch_in: endpoint {
> remote-endpoint = <&csi1_ep>;
> };
>
> switch_out1: endpoint {
> remote-endpoint = <&et8ek8>;
> };
>
> switch_out2: endpoint {
> remote-endpoint = <&smiapp_dfl>;
> };
> };
> };
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFCv2] Device Tree bindings for OMAP3 Camera System
2014-01-20 4:19 ` Sakari Ailus
@ 2014-01-20 22:16 ` Sylwester Nawrocki
2014-01-20 23:27 ` Sebastian Reichel
0 siblings, 1 reply; 13+ messages in thread
From: Sylwester Nawrocki @ 2014-01-20 22:16 UTC (permalink / raw)
To: Sakari Ailus, Sebastian Reichel
Cc: Laurent Pinchart, linux-media, devicetree, Rob Herring,
Pawel Moll, Mark Rutland, Ian Campbell
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFCv2] Device Tree bindings for OMAP3 Camera System
2014-01-20 22:16 ` Sylwester Nawrocki
@ 2014-01-20 23:27 ` Sebastian Reichel
2014-01-22 22:27 ` Sylwester Nawrocki
2014-02-01 9:39 ` Sakari Ailus
0 siblings, 2 replies; 13+ messages in thread
From: Sebastian Reichel @ 2014-01-20 23:27 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: Sakari Ailus, Laurent Pinchart, linux-media, devicetree,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell
[-- Attachment #1: Type: text/plain, Size: 11109 bytes --]
Hi,
On Mon, Jan 20, 2014 at 11:16:43PM +0100, Sylwester Nawrocki wrote:
> On 01/20/2014 05:19 AM, Sakari Ailus wrote:
> >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.
> [...]
> >
> >Over 80 characters per line.
Will be fixed in the next revision.
> >>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.
> >>- #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.
> [...]
> >>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
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.
> >Hmm. I'm not entirely sure what does this last option mean. I could be
> >forgetting something, though.
I hope the above description helped.
> >> - 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.
> >> - 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.
> >>- 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
> >>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.
> >> 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>;
will do.
> >> 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.
> >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.
Obviously it should be mentioned in the n900-camera-switch binding
Documentation. This document was just the proposal for the omap3isp
node :)
> >> 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
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFCv2] Device Tree bindings for OMAP3 Camera System
2014-01-20 23:27 ` Sebastian Reichel
@ 2014-01-22 22:27 ` Sylwester Nawrocki
2014-01-22 22:57 ` Laurent Pinchart
2014-02-01 9:39 ` Sakari Ailus
1 sibling, 1 reply; 13+ messages in thread
From: Sylwester Nawrocki @ 2014-01-22 22:27 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Sakari Ailus, Laurent Pinchart, linux-media, devicetree,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell
On 01/21/2014 12:27 AM, Sebastian Reichel wrote:
> On Mon, Jan 20, 2014 at 11:16:43PM +0100, Sylwester Nawrocki wrote:
>> On 01/20/2014 05:19 AM, Sakari Ailus wrote:
[...]
>>>> - #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 = ...
> };
Yes, sorry, I think you're right. I guess it was just #clock-cells not
being used optimally.
>> 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.
>
>> [...]
>>>> 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
>
> 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.
Hmm, are there any other combinations involving MIPI CSI1 ?
Couldn't we just say that there are: MIPI CSI2, SMIA CCP2 and MIPI CSI1
protocols/bus types used ?
[...]
>>>> - 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
I guess it would be fine.
[...]
>>>> 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.
Indeed, that's a good argument, I forgot about it for a while.
>>> 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.
>
> Obviously it should be mentioned in the n900-camera-switch binding
> Documentation. This document was just the proposal for the omap3isp
> node :)
Huh, I wasn't reading carefully enough! Then since it is just about the
OMAP3 ISP it might be a good idea to drop the switch from the example,
it seems unrelated.
>>>> 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?
No, not really anything better at the moment.
--
Thanks,
Sylwester
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFCv2] Device Tree bindings for OMAP3 Camera System
2014-01-22 22:27 ` Sylwester Nawrocki
@ 2014-01-22 22:57 ` Laurent Pinchart
2014-01-23 0:11 ` Sebastian Reichel
0 siblings, 1 reply; 13+ messages in thread
From: Laurent Pinchart @ 2014-01-22 22:57 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: Sebastian Reichel, Sakari Ailus, linux-media, devicetree,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell
Hi,
On Wednesday 22 January 2014 23:27:42 Sylwester Nawrocki wrote:
> On 01/21/2014 12:27 AM, Sebastian Reichel wrote:
> > On Mon, Jan 20, 2014 at 11:16:43PM +0100, Sylwester Nawrocki wrote:
> >> On 01/20/2014 05:19 AM, Sakari Ailus wrote:
[snip]
> >>>> camera-switch {
> >>>>
> >>>> /*
> >>>> * TODO:
> >>>> * - check if the switching code is generic enough to use a
> >>>> * more generic name like "gpio-camera-switch".
I think you can use a more generic name. You could probably get some
inspiration from the i2c-mux-gpio DT bindings.
> >>>> * - 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.
>
> Indeed, that's a good argument, I forgot about it for a while.
>
> >>> 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.
> >
> > Obviously it should be mentioned in the n900-camera-switch binding
> > Documentation. This document was just the proposal for the omap3isp
> > node :)
>
> Huh, I wasn't reading carefully enough! Then since it is just about the
> OMAP3 ISP it might be a good idea to drop the switch from the example,
> it seems unrelated.
>
> >>>> 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?
>
> No, not really anything better at the moment.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFCv2] Device Tree bindings for OMAP3 Camera System
2014-01-22 22:57 ` Laurent Pinchart
@ 2014-01-23 0:11 ` Sebastian Reichel
2014-01-23 11:44 ` Laurent Pinchart
0 siblings, 1 reply; 13+ messages in thread
From: Sebastian Reichel @ 2014-01-23 0:11 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Sylwester Nawrocki, Sakari Ailus, linux-media, devicetree,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell
[-- Attachment #1: Type: text/plain, Size: 580 bytes --]
Hi,
On Wed, Jan 22, 2014 at 11:57:45PM +0100, Laurent Pinchart wrote:
> [...]
>> camera-switch {
>> /*
>> * TODO:
>> * - check if the switching code is generic enough to use a
>> * more generic name like "gpio-camera-switch".
>
> I think you can use a more generic name. You could probably get some
> inspiration from the i2c-mux-gpio DT bindings.
My main concern is, that the gpio used for switching is also
connected to the reset pin of one of the cameras. Maybe that
fact can just be neglected, though?
> [...]
-- Sebastian
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFCv2] Device Tree bindings for OMAP3 Camera System
2014-01-23 0:11 ` Sebastian Reichel
@ 2014-01-23 11:44 ` Laurent Pinchart
0 siblings, 0 replies; 13+ messages in thread
From: Laurent Pinchart @ 2014-01-23 11:44 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Sylwester Nawrocki, Sakari Ailus, linux-media, devicetree,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell
[-- Attachment #1: Type: text/plain, Size: 1310 bytes --]
Hi Sebastian,
On Thursday 23 January 2014 01:11:29 Sebastian Reichel wrote:
> On Wed, Jan 22, 2014 at 11:57:45PM +0100, Laurent Pinchart wrote:
> > [...]
> >
> >> camera-switch {
> >>
> >> /*
> >>
> >> * TODO:
> >> * - check if the switching code is generic enough to use a
> >> * more generic name like "gpio-camera-switch".
> >
> > I think you can use a more generic name. You could probably get some
> > inspiration from the i2c-mux-gpio DT bindings.
>
> My main concern is, that the gpio used for switching is also connected to
> the reset pin of one of the cameras. Maybe that fact can just be neglected,
> though?
I'm not the only one to wish we could change that, but alas! we'll have to
live with that stupid hardware design decision :-)
What we want to ensure here is that the two sensors won't be accessed at the
same time, as that would lead to errors. This was previously handled by
callback function to board code, but board code is now going away. The
challenge is to find a way to express the constraints in DT. I'm not sure
whether that's doable in a generic way, and this might be one of the rare
cases where board code is still needed.
Sakari, have you given this a thought ?
--
Regards,
Laurent Pinchart
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFCv2] Device Tree bindings for OMAP3 Camera System
2014-01-20 23:27 ` Sebastian Reichel
2014-01-22 22:27 ` Sylwester Nawrocki
@ 2014-02-01 9:39 ` Sakari Ailus
1 sibling, 0 replies; 13+ messages in thread
From: Sakari Ailus @ 2014-02-01 9:39 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Sylwester Nawrocki, Laurent Pinchart, linux-media, devicetree,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell
Hi Sebastian and Sylwester,
On Tue, Jan 21, 2014 at 12:27:21AM +0100, Sebastian Reichel wrote:
> Hi,
>
> On Mon, Jan 20, 2014 at 11:16:43PM +0100, Sylwester Nawrocki wrote:
> > On 01/20/2014 05:19 AM, Sakari Ailus wrote:
> > >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.
>
> > [...]
> > >
> > >Over 80 characters per line.
>
> Will be fixed in the next revision.
>
> > >>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.
Others will have the same so generic it is. It's part of Hiroshi's patches
he sent to linux-arm-kernel (and that I think I also applied to my branch).
> > >>- #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.
Ack.
> > [...]
> > >>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
>
> 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.
Quoting the spec,
0x0: MIPI CSI1-compatible mode. When this bit is set, all CCP2B
settings are ignored. If the settings are not set correctly to MIPI
CSI1 values, the behavior of the receiver is unpredictable.
I'm not entirely certain whether this is a compatibility mode intended for a
driver which cannot entirely control the CSI-1 block after the CCP2B
extensions (whatever they are) were implemented.
I would suggest leaving this out entirely. New cameras are all CSI-2 and the
Nokia N9 appears to be using value zero for this. I'm not aware of other
potential users.
> > >Hmm. I'm not entirely sure what does this last option mean. I could be
> > >forgetting something, though.
>
> I hope the above description helped.
>
> > >> - 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.
>
> > >> - 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 wonderer whether the default could or should be related to the bus type.
I'm not sure.
> I merged (isp_ccp2_platform_data.phy_layer) into the bus-type
> property described above.
>
> > >>- 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
Looks good to me.
> > >>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.
The MMU address range hasn't been there for a while. Which kernel did you
use? Could you use the ranges in my patch instead?
> > >> 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>;
>
> will do.
>
> > >> 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.
Streaming still wouldn't be possible since both streams would eventually
cross at the CCP2 receiver.
But for that matter, I think there would be benefits of being able to
implement simple drivers that do not touch the format and hide them from
user space also elsewhere. This will still need a little bit of thinking.
> > 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?
I think we must describe it in the DT as it's there. Otherwise at least one
driver must be aware of it. And as the switch matters for a number of
drivers, it likely wouldn't be even a single one.
I'd rather try to hide that driver (and the switch) from the user space, or
figure out an easy way to ignore it.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-02-01 9:39 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-03 22:03 [early RFC] Device Tree bindings for OMAP3 Camera Subsystem Sebastian Reichel
2013-11-04 19:49 ` jean-philippe francois
2013-11-06 0:48 ` Laurent Pinchart
2013-11-15 17:18 ` Mark Rutland
2014-01-15 19:41 ` [RFCv2] Device Tree bindings for OMAP3 Camera System Sebastian Reichel
2014-01-20 4:19 ` Sakari Ailus
2014-01-20 22:16 ` Sylwester Nawrocki
2014-01-20 23:27 ` Sebastian Reichel
2014-01-22 22:27 ` Sylwester Nawrocki
2014-01-22 22:57 ` Laurent Pinchart
2014-01-23 0:11 ` Sebastian Reichel
2014-01-23 11:44 ` Laurent Pinchart
2014-02-01 9:39 ` Sakari Ailus
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).