Re: [PATCH v2 0/2] media: i2c: ov5640: Re-work MIPI startup sequence

[Steve Longerbeam <slongerbeam@gmail.com>]

On 07/16/2018 01:29 AM, jacopo mondi wrote:
> Hi Steve,
>     thanks for keep testing it
>
> On Sat, Jul 14, 2018 at 01:02:32PM -0700, Steve Longerbeam wrote:
>>
>> On 07/14/2018 12:41 PM, Steve Longerbeam wrote:
>>> Hi Jacopo,
>>>
>>>
>>> On 07/14/2018 11:57 AM, Steve Longerbeam wrote:
>>>> Hi Jacopo,
>>>>
>>>> Pardon the late reply, see below.
>>>>
>>>> On 07/11/2018 12:21 AM, jacopo mondi wrote:
>>>>> Hi Steve,
>>>>>
>>>>> On Tue, Jul 10, 2018 at 02:10:54PM -0700, Steve Longerbeam wrote:
>>>>>> Hi Jacopo,
>>>>>>
>>>>>> Sorry to report my testing on SabreSD has same result
>>>>>> as last time. This series fixes the LP-11 timeout at stream
>>>>>> on but captured images are still blank. I tried the 640x480
>>>>>> mode with UYVY2X8. Here is the pad config:
>>>>> This saddens me :(
>>>>>
>>>>> I'm capturing with the same format and sizes... this shouldn't be the
>>>>> issue
>>>>>
>>>>> Could you confirm this matches what you have in your tree?
>>>>> 5dc2c80 media: ov5640: Fix timings setup code
>>>>> b35e757 media: i2c: ov5640: Re-work MIPI startup sequence
>>>>> 3c4a737 media: ov5640: fix frame interval enumeration
>>>>> 41cb1c7 media: ov5640: adjust xclk_max
>>>>> c3f3ba3 media: ov5640: add support of module orientation
>>>>> ce85705 media: ov5640: add HFLIP/VFLIP controls support
>>>>> 8663341 media: ov5640: Program the visible resolution
>>>>> 476dec0 media: ov5640: Add horizontal and vertical totals
>>>>> dba13a0 media: ov5640: Change horizontal and vertical resolutions name
>>>>> 8f57c2f media: ov5640: Init properly the SCLK dividers
>>>> Yes, I have that commit sequence.
>>>>
>>>> FWIW, I can verify what Jagan Teki reported earlier, that the driver
>>>> still
>>>> works on the SabreSD platform at:
>>>>
>>>> dba13a0 media: ov5640: Change horizontal and vertical resolutions name
>>>>
>>>> and is broken at:
>>>>
>>>> 476dec0 media: ov5640: Add horizontal and vertical totals
>>>>
>>>> with LP-11 timeout at the mipi csi-2 receiver:
>>>>
>>>> [   80.763189] imx6-mipi-csi2: LP-11 timeout, phy_state = 0x00000230
>>>> [   80.769599] ipu1_csi1: pipeline start failed with -110
>>> And I discovered the bug in 476dec0 "media: ov5640: Add horizontal and
>>> vertical totals". The call to ov5640_set_timings() needs to be moved
>>> before the
>>> calls to ov5640_get_vts() and ov5640_get_hts(). But I see you have
>>> discovered
>>> that as well, and fixed in the second patch in your series.
>>>
> I'm sorry I'm not sur I'm following. Does this mean that with that bug
> you are referring to up here fixed by my last patch you have capture
> working?

No, capture still not working for me on SabreSD, even after fixing
the bug in 476dec0 "media: ov5640: Add horizontal and vertical totals",
by either using your patchset, or by running version 476dec0 of ov5640.c
with the call to ov5640_set_timings() moved to the correct places as
described below.

Steve

>> But strangely, if I revert to 476dec0, and then move the call to
>> ov5640_set_timings()
>> to just after ov5640_load_regs() in ov5640_set_mode_exposure_calc() and
>> ov5640_set_mode_direct(), the LP-11 timeouts are still present. So I can
>> confirm
>> this strangeness which you already pointed out below [1].
>>
>>
>>>>>>> The version I'm sending here re-introduces some of the timings
>>>>>>> parameters in the
>>>>>>> initial configuration blob (not in the single mode ones), which
>>>>>>> apparently has
>>>>>>> to be at least initially programmed to allow the driver to later
>>>>>>> program them
>>>>>>> singularly in the 'set_timings()' function. Unfortunately I do not
>>>>>>> have a real
>>>>>>> rationale behind this which explains why it has to be done this
>>>>>>> way :(
>>>>>>>
>> [1] here :)
>>
>> Steve
>>
>>