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:
signature.asc
Description: Digital signature
