From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751557AbdBMXUM (ORCPT ); Mon, 13 Feb 2017 18:20:12 -0500 Received: from mail-it0-f68.google.com ([209.85.214.68]:34623 "EHLO mail-it0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751079AbdBMXUJ (ORCPT ); Mon, 13 Feb 2017 18:20:09 -0500 From: Steve Longerbeam Subject: Re: [PATCH v3 21/24] media: imx: Add MIPI CSI-2 Receiver subdev driver To: Philipp Zabel References: <1483755102-24785-1-git-send-email-steve_longerbeam@mentor.com> <1483755102-24785-22-git-send-email-steve_longerbeam@mentor.com> <1486036237.2289.37.camel@pengutronix.de> <20170208234235.GA27312@n2100.armlinux.org.uk> <1486977617.2873.29.camel@pengutronix.de> Cc: Russell King - ARM Linux , robh+dt@kernel.org, mark.rutland@arm.com, shawnguo@kernel.org, kernel@pengutronix.de, fabio.estevam@nxp.com, mchehab@kernel.org, hverkuil@xs4all.nl, nick@shmanahar.org, markus.heiser@darmarIT.de, laurent.pinchart+renesas@ideasonboard.com, bparrot@ti.com, geert@linux-m68k.org, arnd@arndb.de, sudipm.mukherjee@gmail.com, minghsiu.tsai@mediatek.com, tiffany.lin@mediatek.com, jean-christophe.trotin@st.com, horms+renesas@verge.net.au, niklas.soderlund+renesas@ragnatech.se, robert.jarzmik@free.fr, songjun.wu@microchip.com, andrew-ct.chen@mediatek.com, gregkh@linuxfoundation.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org, devel@driverdev.osuosl.org, Steve Longerbeam Message-ID: <04a4d130-0259-cbba-7815-e41c1c80c3c7@gmail.com> Date: Mon, 13 Feb 2017 15:20:05 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <1486977617.2873.29.camel@pengutronix.de> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org (#!##* 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 > 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 > --- > 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)