linux-kernel - Re: [PATCH v3 21/24] media: imx: Add MIPI CSI-2 Receiver subdev driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 31 Jan 2017 17:02:40 -0800
From:   Steve Longerbeam <slongerbeam@...il.com>
To:     Philipp Zabel <p.zabel@...gutronix.de>,
        Russell King - ARM Linux <linux@...linux.org.uk>
Cc:     robh+dt@...nel.org, mark.rutland@....com, shawnguo@...nel.org,
        kernel@...gutronix.de, fabio.estevam@....com, mchehab@...nel.org,
        hverkuil@...all.nl, nick@...anahar.org, markus.heiser@...marIT.de,
        laurent.pinchart+renesas@...asonboard.com, bparrot@...com,
        geert@...ux-m68k.org, arnd@...db.de, sudipm.mukherjee@...il.com,
        minghsiu.tsai@...iatek.com, tiffany.lin@...iatek.com,
        jean-christophe.trotin@...com, horms+renesas@...ge.net.au,
        niklas.soderlund+renesas@...natech.se, robert.jarzmik@...e.fr,
        songjun.wu@...rochip.com, andrew-ct.chen@...iatek.com,
        gregkh@...uxfoundation.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-media@...r.kernel.org, devel@...verdev.osuosl.org,
        Steve Longerbeam <steve_longerbeam@...tor.com>
Subject: Re: [PATCH v3 21/24] media: imx: Add MIPI CSI-2 Receiver subdev
 driver

On 01/31/2017 01:49 AM, Philipp Zabel wrote:
> On Tue, 2017-01-31 at 00:01 +0000, Russell King - ARM Linux wrote:
> [...]
>> The iMX6 manuals call for a very specific seven sequence of initialisation
>> for CSI2, which begins with:
>>
>> 1. reset the D-PHY.
>> 2. place MIPI sensor in LP-11 state
>> 3. perform D-PHY initialisation
>> 4. configure CSI2 lanes and de-assert resets and shutdown signals
>>
>> Since you reset the CSI2 at power up and then release it, how do you
>> guarantee that the published sequence is followed?

Hi Russell,

In "40.3.1 Startup Sequence", it states that step 1 is to "De-assert
CSI2 presetn signal (global reset)". I can't find any description of
this signal in the manual, that statement is the only mention of it. I
don't know if this the D-PHY reset signal,  it sounds more like
CSI2_RESETN (CSI-2 host controller reset).

In any case, I re-reviewed the published sequence in the manual,
and it does look like there are a couple problems.

The pipeline power up sequence in imx-media driver is as follows:
s_power(ON) op is called first on the imx6-mipi-csi2, in which CSI2 and
D-PHY resets are asserted and then de-asserted via the CSI2_RESETN
and CSI2_DPHY_RSTZ registers, the D-PHY is initialized, and lanes set.

At this point the MIPI sensor may be powered down (in fact, in OV5640
case, the PWDN pin is asserted). So there could be a problem here,
I don't think the D-PHY is considered in the LP-11 stop state when the
D-PHY master is powered off :). A fix might simply be to reverse power
on, sensor first so that it can be placed in LP-11, then imx6-mipi-csi2.

The following steps are carried out by s_stream() calls. Sensor s_stream(ON)
is called first which starts a clock on the clock lane. Then imx6-mipi-csi2
s_stream(ON) in which the PHY_STATE register is polled to confirm the D-PHY
stop state, then looks for active clock on lock lane.

There could be a problem there too. Again should be fixed simply by
calling stream-on on the imx6-mipi-csi2 first, then sensor.

So I will try the following sequence:

1. sensor power on (put D-PHY in LP-11 stop state).
2. csi-2 power on (deassert CSI2 and D-PHY resets, D-PHY init, verify 
LP-11).
3. sensor stream on (starts clock on clock lane).
4. csi-2 stream on (confirm clock on clock lane).

That comes closest to meeting the sequence requirements.

But this also puts a requirement on MIPI sensors that s_power(ON)
should only place the D_PHY in LP-11, and _not_ start the clock lane.
But perhaps that is correct behavior anyway.

Steve

>> With Philipp's driver, this is easy, because there is a prepare_stream
>> callback which gives the sensor an opportunity to get everything
>> correctly configured according to the negotiated parameters, and place
>> the sensor in LP-11 state.
>>
>> Some sensors do not power up in LP-11 state, but need to be programmed
>> fully before being asked to momentarily stream.  Only at that point is
>> the sensor guaranteed to be in the required LP-11 state.
> Do you expect that 1. and 2. could depend on the negotiated parameters
> in any way on some hardware? I had removed the prepare_stream callback
> from my driver in v2 because for my use case at least the above sequence
> could be realized by
>
> 1. in imx-mipi-csi2 s_power(1)
> 2. in MIPI sensor s_power(1)
> 3./4. in imx-mipi-csi2 s_stream(1)
> 4. in MIPI sensor s_stream(1)
>
> as long as the sensor is correctly put back into LP-11 in s_stream(0).
>
> regards
> Philipp
>