Hi Sakari,
On 12/13/2017 08:47 PM, Sakari Ailus wrote:
> Hi Hugues,
>
> Hugues FRUCHET wrote:
>> Hi Sakari,
>>
>> On 12/07/2017 02:59 PM, Sakari Ailus wrote:
>>> Hi Hugues,
>>>
>>> On Thu, Dec 07, 2017 at 01:40:51PM +0100, Hugues Fruchet wrote:
>>>> Add bindings for OV5640 DVP parallel interface support.
>>>>
>>>> Signed-off-by: Hugues Fruchet <hugues.fruchet@xxxxxx>
>>>> ---
>>>> .../devicetree/bindings/media/i2c/ov5640.txt | 27 ++++++++++++++++++++--
>>>> 1 file changed, 25 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5640.txt b/Documentation/devicetree/bindings/media/i2c/ov5640.txt
>>>> index 540b36c..04e2a91 100644
>>>> --- a/Documentation/devicetree/bindings/media/i2c/ov5640.txt
>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5640.txt
>>>> @@ -1,4 +1,4 @@
>>>> -* Omnivision OV5640 MIPI CSI-2 sensor
>>>> +* Omnivision OV5640 MIPI CSI-2 / parallel sensor
>>>>
>>>> Required Properties:
>>>> - compatible: should be "ovti,ov5640"
>>>> @@ -18,7 +18,11 @@ The device node must contain one 'port' child node for its digital output
>>>> video port, in accordance with the video interface bindings defined in
>>>> Documentation/devicetree/bindings/media/video-interfaces.txt.
>>>>
>>>> -Example:
>>>> +Parallel or CSI mode is selected according to optional endpoint properties.
>>>> +Without properties (or bus properties), parallel mode is selected.
>>>> +Specifying any CSI properties such as lanes will enable CSI mode.
>>>
>>> These bindings never documented what which endpoint properties were needed.
>>
>> Ok I will add a section related to endpoint properties for both CSI and
>> parallel.
>>
>>>
>>> Beyond that, the sensor supports two CSI-2 lanes. You should explicitly
>>> specify that, in other words, you'll need "data-lanes" property. Could you
>>> add that?
>> Ok I will add it to required endpoint property in case of CSI mode.
>> I will change commit header to reflect changes on parallel but also CSI
>> documentation.
>>
>>>
>>> Long time ago when the video-interfaces.txt and the V4L2 OF framework were
>>> written, the bus type selection was made implicit and only later on
>>> explicit. This is still reflected in how the bus type gets set between
>>> CSI-2 D-PHY, parallel and Bt.656.
>>>
>> I'm a little bit confused, must I explicitly add as required property
>> "bus-type=0" (autodetect) for both cases ? Or must I require
>> "bus-type=1" for CSI and "bus-type=3" for parallel ?
>
> Yes, the confusion will stay for some time to come in some way. :-)
>
> What I wanted to say that the original decision to make this implicit
> from the bindings wasn't great --- we have here the device that does
> both parallel and CSI-2 D-PHY.
>
> But due to data-lanes, you can rely on implicit bus type selection
> working. So no bus-type is needed.
>
OK, got it now:
- "bus-type=0" (autodetect) => V4L2_MBUS_PARALLEL or V4L2_MBUS_BT656 or
V4L2_MBUS_CSI2 depending on properties
- "bus-type=1" => MIPI CSI-2 C-PHY
- "bus-type=2" => MIPI CSI1
- "bus-type=3" => CCP2
/**
* enum v4l2_mbus_type - media bus type
* @V4L2_MBUS_PARALLEL: parallel interface with hsync and vsync
* @V4L2_MBUS_BT656: parallel interface with embedded synchronisation, can
* also be used for BT.1120
* @V4L2_MBUS_CSI1: MIPI CSI-1 serial interface
* @V4L2_MBUS_CCP2: CCP2 (Compact Camera Port 2)
* @V4L2_MBUS_CSI2: MIPI CSI-2 serial interface
*/
enum v4l2_mbus_type {
V4L2_MBUS_PARALLEL,
V4L2_MBUS_BT656,
V4L2_MBUS_CSI1,
V4L2_MBUS_CCP2,
V4L2_MBUS_CSI2,
};
This explain my confusion on CSI-2 CPHY and CCP2 below...
>>
>>
>> Talking bindings, I feel that it could be of great help to document also
>> the polarity of control signals (hsync/vsync/pclk), they are currently
>> set by ov5640 init sequence and not configurable.
>> Moreover, should some checks be added in probe sequence to verify that
>> the defined control signals polarity are aligned with default ones from
>> init sequence ?
>
> Yes, that's a very good idea. It should have been there all along.
>
>>
>>
>> Here is a proposal:
>>
>> "
>> The device node must contain one 'port' child node for its digital
>> output video port with a single 'endpoint' subnode, in accordance
>> with the video interface bindings defined in
>> Documentation/devicetree/bindings/media/video-interfaces.txt.
>>
>> OV5640 can be connected to a MIPI CSI bus or a parallel bus endpoint:
>
> CSI-2, please.
>
OK
>>
>> Endpoint node required properties for CSI connection are:
>> - remote-endpoint: a phandle to the bus receiver's endpoint node.
>> - bus-type: should be set to <1> (MIPI CSI-2 C-PHY)
>
> You can omit bus-type. Or is this really C-PHY?? We don't actually have
> any other devices that support C-PHY yet AFAIK.
>
No it's D-PHY, confusion on my side...
>> - clock-lanes: should be set to <0> (clock lane on hardware lane 0)
>
> But clock-lanes isn't relevant for C-PHY. So you have D-PHY, I presume.
>
OK I'll remove.
>> - data-lanes: should be set to <1 2> (two CSI-2 lanes supported)
>
> This should document what the hardware can do, not what the driver
> supports. So <1> or <1 2> it should be.
OK
>
>>
>> Endpoint node required properties for parallel connection are:
>> - remote-endpoint: a phandle to the bus receiver's endpoint node.
>> - bus-type: should be set to <3> (parallel CCP2)
>
> CCP2 is Compact Camera Port 2, an older serial bus (between CSI and CSI-2).
No it's parallel, confusion on my side...
>
> (I actually wonder if we could fix the bus-type property by giving
> separate entries for parallel and CSI-2 D-PHY; I'll still need to check
> whether it's used somewhere. I think not. Not relevant for this patchset
> though.)
>
>> - bus-width: should be set to <8> for 8 bits parallel bus
>> or <10> for 10 bits parallel bus
>> - data-shift: should be set to <2> for 8 bits parallel bus
>> (lines 9:2 are used) or <0> for 10 bits parallel bus
>
> s/should/shall/ for both.
>
OK
>> - hsync-active: should be set to <0> (Horizontal synchronization
>> polarity is active low).
>> - vsync-active: should be set to <1> (active high) (Horizontal
>> synchronization polarity is active low).
>> - pclk-sample: should be set to <1> (data are sampled on the rising
>> edge of the pixel clock signal).
>
> Is this configurable on hardware? If so, no recommendation should be
> made for hardware configuration as it's board specific.
>
As explained, above:
>> Talking bindings, I feel that it could be of great help to document
>> also the polarity of control signals (hsync/vsync/pclk), they are
>> currently set by ov5640 init sequence and not configurable.
So this is configurable by some sensor registers but currently hardcoded
in sensor init sequence inside driver.
So in order to make it functional, the remote side (ISP endpoint
"parallel_from_ov5640") have to be configured to match those signal levels.
So here we have 3 options:
1) no recommendation
In this case no information can be found in binding on how to set the
signal levels on ISP side, information is given in ov5640 source code:
+static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
<...>
+ * The init sequence initializes control lines as defined below:
+ * - VSYNC: active high
+ * - HREF: active low
+ * - PCLK: active high
+ */
From this source code information and schematics study for potential
inverters in-between, we can deduce the ISP endpoint
"parallel_from_ov5640" signal levels properties.
2) hsync/vsync/pclk signal levels are optional endpoint properties
They affect ov5640 hardware signals polarity. ov5640 sensor driver code
must be updated to send the right i2c sequences to program signal
polarities depending on those devicetree endpoint values (not currently
done).
With schematics study for potential inverters in-between, we can easily
set the ISP endpoint "parallel_from_ov5640" signal levels properties.
No source code check is required.
3) hsync/vsync/pclk signal levels are fixed required endpoint properties
This is my current proposal.
They reflect in binding documentation the hardware signal levels of
ov5640 set by init sequence.
I can update the ov5640 sensor driver code in order to check that those
mandatory properties are set to the right value.
With schematics study for potential inverters in-between, we can easily
set the ISP endpoint "parallel_from_ov5640" signal levels properties.
No source code check is required.
>>
>>
>>>> +
>>>> +Examples:
>>>>
>>>> &i2c1 {
>>>> ov5640: camera@3c {
>>>> @@ -35,6 +39,7 @@ Example:
>>>> reset-gpios = <&gpio1 20 GPIO_ACTIVE_LOW>;
>>>>
>>>> port {
>>>> + /* MIPI CSI-2 bus endpoint */
>>>> ov5640_to_mipi_csi2: endpoint {
>>>> remote-endpoint = <&mipi_csi2_from_ov5640>;
>>>> clock-lanes = <0>;
>>>> @@ -43,3 +48,21 @@ Example:
>>>> };
>>>> };
>>>> };
>>>> +
>>>> +&i2c1 {
>>>> + ov5640: camera@3c {
>>>> + compatible = "ovti,ov5640";
>>>> + pinctrl-names = "default";
>>>> + pinctrl-0 = <&pinctrl_ov5640>;
>>>> + reg = <0x3c>;
>>>> + clocks = <&clk_ext_camera>;
>>>> + clock-names = "xclk";
>>>> +
>>>> + port {
>>>> + /* Parallel bus endpoint */
>>>> + ov5640_to_parallel: endpoint {
>>>> + remote-endpoint = <¶llel_from_ov5640>;
>>>> + };
>>>> + };
>>>> + };
>>>> +};
>>>> --
>>>> 1.9.1
>>>>
>>>
>>
>> Best regards,
>> Hugues.
>>
>
>
Best regards,
Hugues
