Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1570193 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 21/24] media: imx: Add MIPI CSI-2 Receiver subdev  driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-01-31 01:10 +0100
    Re: [PATCH v3 21/24] media: imx: Add MIPI CSI-2 Receiver subdev  driver Philipp Zabel <p.zabel@pengutronix.de> - 2017-01-31 11:00 +0100
      Re: [PATCH v3 21/24] media: imx: Add MIPI CSI-2 Receiver subdev  driver Steve Longerbeam <slongerbeam@gmail.com> - 2017-02-01 02:10 +0100
        Re: [PATCH v3 21/24] media: imx: Add MIPI CSI-2 Receiver subdev  driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-02-01 02:20 +0100

#1570193 — Re: [PATCH v3 21/24] media: imx: Add MIPI CSI-2 Receiver subdev driver

On Fri, Jan 06, 2017 at 06:11:39PM -0800, Steve Longerbeam wrote:
> +static void imxcsi2_enable(struct imxcsi2_dev *csi2, bool enable)
> +{
> +	if (enable) {
> +		imxcsi2_write(csi2, 0xffffffff, CSI2_PHY_SHUTDOWNZ);
> +		imxcsi2_write(csi2, 0xffffffff, CSI2_DPHY_RSTZ);
> +		imxcsi2_write(csi2, 0xffffffff, CSI2_RESETN);
> +	} else {
> +		imxcsi2_write(csi2, 0x0, CSI2_PHY_SHUTDOWNZ);
> +		imxcsi2_write(csi2, 0x0, CSI2_DPHY_RSTZ);
> +		imxcsi2_write(csi2, 0x0, CSI2_RESETN);
> +	}
> +}
> +
> +static void imxcsi2_reset(struct imxcsi2_dev *csi2)
> +{
> +	imxcsi2_enable(csi2, false);
> +
> +	imxcsi2_write(csi2, 0x00000001, CSI2_PHY_TST_CTRL0);
> +	imxcsi2_write(csi2, 0x00000000, CSI2_PHY_TST_CTRL1);
> +	imxcsi2_write(csi2, 0x00000000, CSI2_PHY_TST_CTRL0);
> +	imxcsi2_write(csi2, 0x00000002, CSI2_PHY_TST_CTRL0);
> +	imxcsi2_write(csi2, 0x00010044, CSI2_PHY_TST_CTRL1);
> +	imxcsi2_write(csi2, 0x00000000, CSI2_PHY_TST_CTRL0);
> +	imxcsi2_write(csi2, 0x00000014, CSI2_PHY_TST_CTRL1);
> +	imxcsi2_write(csi2, 0x00000002, CSI2_PHY_TST_CTRL0);
> +	imxcsi2_write(csi2, 0x00000000, CSI2_PHY_TST_CTRL0);
> +
> +	imxcsi2_enable(csi2, true);
> +}
> +
> +static int imxcsi2_dphy_wait(struct imxcsi2_dev *csi2)
> +{
> +	u32 reg;
> +	int i;
> +
> +	/* wait for mipi sensor ready */
> +	for (i = 0; i < 50; i++) {
> +		reg = imxcsi2_read(csi2, CSI2_PHY_STATE);
> +		if (reg != 0x200)
> +			break;
> +		usleep_range(10000, 20000);
> +	}
> +
> +	if (i >= 50) {
> +		v4l2_err(&csi2->sd,
> +			 "wait for clock lane timeout, phy_state = 0x%08x\n",
> +			 reg);
> +		return -ETIME;
> +	}
> +
> +	/* wait for mipi stable */
> +	for (i = 0; i < 50; i++) {
> +		reg = imxcsi2_read(csi2, CSI2_ERR1);
> +		if (reg == 0x0)
> +			break;
> +		usleep_range(10000, 20000);
> +	}
> +
> +	if (i >= 50) {
> +		v4l2_err(&csi2->sd,
> +			 "wait for controller timeout, err1 = 0x%08x\n",
> +			 reg);
> +		return -ETIME;
> +	}
> +
> +	/* finally let's wait for active clock on the clock lane */
> +	for (i = 0; i < 50; i++) {
> +		reg = imxcsi2_read(csi2, CSI2_PHY_STATE);
> +		if (reg & (1 << 8))
> +			break;
> +		usleep_range(10000, 20000);
> +	}
> +
> +	if (i >= 50) {
> +		v4l2_err(&csi2->sd,
> +			 "wait for active clock timeout, phy_state = 0x%08x\n",
> +			 reg);
> +		return -ETIME;
> +	}
> +
> +	v4l2_info(&csi2->sd, "ready, dphy version 0x%x\n",
> +		  imxcsi2_read(csi2, CSI2_VERSION));
> +
> +	return 0;
> +}
...
> +static int imxcsi2_s_power(struct v4l2_subdev *sd, int on)
> +{
> +	struct imxcsi2_dev *csi2 = sd_to_dev(sd);
> +
> +	if (on && !csi2->on) {
> +		v4l2_info(&csi2->sd, "power ON\n");
> +		clk_prepare_enable(csi2->cfg_clk);
> +		clk_prepare_enable(csi2->dphy_clk);
> +		imxcsi2_set_lanes(csi2);
> +		imxcsi2_reset(csi2);

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?

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.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [next] | [standalone]

#1570538

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?
> 
> 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

[toc] | [prev] | [next] | [standalone]

#1571232

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
>

[toc] | [prev] | [next] | [standalone]

#1571234

Hi Steve,

On Tue, Jan 31, 2017 at 05:02:40PM -0800, Steve Longerbeam wrote:
> 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.

If the CSI2 DPHY is held in reset state, it shouldn't matter what the
sensor does.  In the case of IMX219, it needs a full setup of the
device, including enabling it to stream (so it starts the clock lane
etc) in order to get it into LP-11 state.  Merely disabling the XCLR
signal leaves the lanes grounded.

I do seem to remember reading in one of the MIPI specs that this is
rather expected behaviour, though I can't point at a paragraph this
late in the night.

So, the only way to satisfy these requirements is this order:

- assert PHY reset signals (so blocking any activity on the CSI lanes)
- initialise the sensor (including allowing it to start streaming and
  then stopping the stream - at that point, the lanes will be in LP-11.)
- deassert the resets as per the iMX6 documentation and follow the
  remaining procedure.

I'll look at your other points tomorrow.

Thanks.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web