| Message ID | 1486977617.2873.29.camel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
(#!##* Thunderbird! resending text only) Hi Philipp, On 02/13/2017 01:20 AM, Philipp Zabel wrote: > Hi Steve, > > On Thu, 2017-02-09 at 15:51 -0800, Steve Longerbeam wrote: >> On 02/09/2017 03:49 PM, Steve Longerbeam wrote: >>> On 02/08/2017 03:42 PM, Russell King - ARM Linux wrote: >>>> On Wed, Feb 08, 2017 at 03:23:53PM -0800, Steve Longerbeam wrote: >>>>>> Actually, this exact function already exists as >>>>>> dw_mipi_dsi_phy_write in >>>>>> drivers/gpu/drm/rockchip/dw-mipi-dsi.c, and it looks like the D-PHY >>>>>> register 0x44 might contain a field called HSFREQRANGE_SEL. >>>>> Thanks for pointing out drivers/gpu/drm/rockchip/dw-mipi-dsi.c. >>>>> It's clear from that driver that there probably needs to be a fuller >>>>> treatment of the D-PHY programming here, but I don't know where >>>>> to find the MIPI CSI-2 D-PHY documentation for the i.MX6. The code >>>>> in imxcsi2_reset() was also pulled from FSL, and that's all I really >>>>> have >>>>> to go on for the D-PHY programming. I assume the D-PHY is also a >>>>> Synopsys core, like the host controller, but the i.MX6 manual doesn't >>>>> cover it. >>>> Why exactly? What problems are you seeing that would necessitate a >>>> more detailed programming of the D-PHY? From my testing, I can wind >>>> a 2-lane MIPI bus on iMX6D from 912Mbps per lane down to (eg) 308Mbps >>>> per lane with your existing code without any issues. >>> That's good to hear. >>> >>> Just from my experience with struggles to get the CSI2 receiver >>> up and running with an active clock lane, and my suspicions that >>> some of that could be due to my lack of understanding of the D-PHY >>> programming. >> But I should add that after a re-org of the sequence, it looks more stable >> now. Tested on both the SabreSD and SabreLite with the OV5640. > It seems the OV5640 driver never puts its the CSI-2 lanes into stop > state while not streaming. Yes I found that as well. But good news, I finally managed to coax the OV5640's clock lane into LP-11 state! It was accomplished by setting bit 5 in OV5640 register 0x4800. The datasheet describes this bit as "Gate clock lane when no packet to transmit". But I may have also got this to work with the additional write 1 to bits 4-6 in register 0x3019 ("MIPI CLK/data lane state in sleep mode" - setting 1 to these bits selects LP-11 for sleep mode). So I am now finally able to call csi2_dphy_wait_stopstate() in csi2_s_stream(ON). So for the TC35874, you shouldn't see a timeout in csi2_s_stream(ON) any longer. I have updated both ov5640.c and imx6-mipi-csi2.c in the wip branch. Can you try again? I have not applied your patch below, because I don't think that is needed anymore. And speaking of the TC35874, I received this module for the SabreLite from Boundary Devices (thanks BD!). So I can finally help you with debug/test. Are there any patches you need to send to me (against wip branch) for this support, or can I use what exists now in media tree? Also any scripts or help with running. > With the recent s_stream reordering, > streaming from TC358743 does not work anymore, since imx6-mipi-csi2 > s_stream is called before tc358743 s_stream, while all lanes are still > in stop state. Then it waits for the clock lane to become active and > fails. I have applied the following patch to revert the reordering > locally to get it to work again. > > The initialization order, as Russell pointed out, should be: > > 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 > > So csi2_s_stream should wait for stop state on all lanes (the result of > 2.) before dphy_init (3.), not wait for active clock afterwards. That > should happen only after sensor_s_stream was called. So unless we are > allowed to reorder steps 1. and 2., we might need the prepare_stream > callback after all. Agreed! See my new FIXME comment in imx6-mipi-csi2.c. I agree we might need a new subdev op .prepare_stream(). This op would be implemented by imx6-mipi-csi2.c, and would carry out steps 3, 4, 5 (instead of currently by csi2_s_stream()). Then step 6 would finally become available as csi2_s_stream(). And then we must re-order stream on to start sensor first, then csi2, as in your patch below. Steve > More specifically, the chapter 40.3.1 "Startup Sequence" in the i.MX6DQ > reference manual states: > > 1. Deassert presetn signal (global reset) > - This is probably the APB global reset, so not something we have to > care about. > 2. Configure MIPI Camera Sensor to put all Tx lanes in PL-11 state > 3. D-PHY initialization (write 0x14 to address 0x44) > 4. CSI2 Controller programming > - Set N_LANES > - Deassert PHY_SHUTDOWNZ > - Deassert PHY_RSTZ > - Deassert CSI2_RESETN > 5. Read the PHY status register (PHY_STATE) to confirm that all data and > clock lanes of the D-PHY are in Stop State > 6. Configure the MIPI Camera Sensor to start transmitting a clock on the > D-PHY clock lane > 7. CSI2 Controller programming - Read the PHY status register > (PHY_STATE) to confirm that the D-PHY is receiving a clock on the > D-PHY clock lane > > 2. can be done in sensor s_power (which tc358743 currently does) only if > the sensor can still be configured in step 6. > Also, 3./4./5. are csi2 code, 6. is sensor code, and 7. is csi2 code > again. For reliable startup we need csi2 receiver code to be called on > both sides of the sensor enabling its clock lane. > ----------8<---------- > From f12758caa3e1d05980cd2ac07587d125449fc860 Mon Sep 17 00:00:00 2001 > From: Philipp Zabel<p.zabel@pengutronix.de> > Date: Mon, 13 Feb 2017 09:28:27 +0100 > Subject: [PATCH] media: imx: revert streamon sequence change > > Without this patch, starting streaming from a TC358743 MIPI CSI-2 source > fails with the following error messages: > > imx6-mipi-csi2: clock lane timeout, phy_state = 0x000006f0 > ipu1_csi0: pipeline_set_stream failed with -110 > > The phy state above has the stopstateclk, rxulpsclknot, and > stopstatedata[3:0] bits set: at this point all lanes are in stop state, > no lane is in ultra low power state or active. > This is no suprise, since tc358743 s_stream(sd, 1) has not been called > due to the recently changed ordering. The imx6-mipi-csi2 s_stream does > wait for the clock lane to become active, so csi2_s_stream must happen > after tc358743_s_stream. > > Signed-off-by: Philipp Zabel<p.zabel@pengutronix.de> > --- > drivers/staging/media/imx/imx-media-utils.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c > index 81eabcf76a66f..495ccefa3cd2a 100644 > --- a/drivers/staging/media/imx/imx-media-utils.c > +++ b/drivers/staging/media/imx/imx-media-utils.c > @@ -782,8 +782,8 @@ static const u32 stream_on_seq[] = { > IMX_MEDIA_GRP_ID_IC_PRPENC, > IMX_MEDIA_GRP_ID_IC_PRP, > IMX_MEDIA_GRP_ID_VDIC, > - IMX_MEDIA_GRP_ID_CSI2, > IMX_MEDIA_GRP_ID_SENSOR, > + IMX_MEDIA_GRP_ID_CSI2, > IMX_MEDIA_GRP_ID_VIDMUX, > IMX_MEDIA_GRP_ID_CSI, > }; > @@ -795,8 +795,8 @@ static const u32 stream_off_seq[] = { > IMX_MEDIA_GRP_ID_VDIC, > IMX_MEDIA_GRP_ID_CSI, > IMX_MEDIA_GRP_ID_VIDMUX, > - IMX_MEDIA_GRP_ID_SENSOR, > IMX_MEDIA_GRP_ID_CSI2, > + IMX_MEDIA_GRP_ID_SENSOR, > }; > > #define NUM_STREAM_ENTITIES ARRAY_SIZE(stream_on_seq)
Hi Steve, On Mon, 2017-02-13 at 15:20 -0800, Steve Longerbeam wrote: [...] > > It seems the OV5640 driver never puts its the CSI-2 lanes into stop > > state while not streaming. > > Yes I found that as well. > > But good news, I finally managed to coax the OV5640's clock lane > into LP-11 state! It was accomplished by setting bit 5 in OV5640 register > 0x4800. The datasheet describes this bit as "Gate clock lane when no > packet to transmit". But I may have also got this to work with the > additional > write 1 to bits 4-6 in register 0x3019 ("MIPI CLK/data lane state in sleep > mode" - setting 1 to these bits selects LP-11 for sleep mode). > > So I am now finally able to call csi2_dphy_wait_stopstate() in > csi2_s_stream(ON). That's good news. > So for the TC35874, you shouldn't see a timeout in csi2_s_stream(ON) > any longer. > > I have updated both ov5640.c and imx6-mipi-csi2.c in the wip branch. > Can you try again? I have not applied your patch below, because I > don't think that is needed anymore. Thanks, I'll test tomorrow. > And speaking of the TC35874, I received this module for the SabreLite > from Boundary Devices (thanks BD!). So I can finally help you with > debug/test. Are there any patches you need to send to me (against > wip branch) for this support, or can I use what exists now in media > tree? Also any scripts or help with running. That's even better news. I have pushed my my wip branch, which contains some colorspace work and experiments to pass through query/g_/s_std subdev calls so bypassing the pipeline can be avoided. Also, there's the Nitrogen6X device tree that I've been using to test: https://git.pengutronix.de/git/pza/linux imx-media-staging-md-wip > > With the recent s_stream reordering, > > streaming from TC358743 does not work anymore, since imx6-mipi-csi2 > > s_stream is called before tc358743 s_stream, while all lanes are still > > in stop state. Then it waits for the clock lane to become active and > > fails. I have applied the following patch to revert the reordering > > locally to get it to work again. > > > > The initialization order, as Russell pointed out, should be: > > > > 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 > > > > So csi2_s_stream should wait for stop state on all lanes (the result of > > 2.) before dphy_init (3.), not wait for active clock afterwards. That > > should happen only after sensor_s_stream was called. So unless we are > > allowed to reorder steps 1. and 2., we might need the prepare_stream > > callback after all. > > Agreed! > > See my new FIXME comment in imx6-mipi-csi2.c. Looks good. I wonder if enabling the phy clock isn't part of step 3. though. > I agree we might need a new subdev op .prepare_stream(). This > op would be implemented by imx6-mipi-csi2.c, and would carry > out steps 3, 4, 5 (instead of currently by csi2_s_stream()). Then > step 6 would finally become available as csi2_s_stream(). > > And then we must re-order stream on to start sensor first, then > csi2, as in your patch below. regards Philipp
diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c index 81eabcf76a66f..495ccefa3cd2a 100644 --- a/drivers/staging/media/imx/imx-media-utils.c +++ b/drivers/staging/media/imx/imx-media-utils.c @@ -782,8 +782,8 @@ static const u32 stream_on_seq[] = { IMX_MEDIA_GRP_ID_IC_PRPENC, IMX_MEDIA_GRP_ID_IC_PRP, IMX_MEDIA_GRP_ID_VDIC, - IMX_MEDIA_GRP_ID_CSI2, IMX_MEDIA_GRP_ID_SENSOR, + IMX_MEDIA_GRP_ID_CSI2, IMX_MEDIA_GRP_ID_VIDMUX, IMX_MEDIA_GRP_ID_CSI, }; @@ -795,8 +795,8 @@ static const u32 stream_off_seq[] = { IMX_MEDIA_GRP_ID_VDIC, IMX_MEDIA_GRP_ID_CSI, IMX_MEDIA_GRP_ID_VIDMUX, - IMX_MEDIA_GRP_ID_SENSOR, IMX_MEDIA_GRP_ID_CSI2, + IMX_MEDIA_GRP_ID_SENSOR, }; #define NUM_STREAM_ENTITIES ARRAY_SIZE(stream_on_seq)