Re: [PATCH v3 03/12] media: ov5640: Remove the clocks registers initialization

[Maxime Ripard <maxime.ripard@bootlin.com>]

[-- Attachment #1: Type: text/plain, Size: 2691 bytes --]

On Wed, May 23, 2018 at 11:31:58AM +0200, Daniel Mack wrote:
> Hi Maxime,
> 
> On Tuesday, May 22, 2018 09:54 PM, Maxime Ripard wrote:
> > On Mon, May 21, 2018 at 09:39:02AM +0200, Maxime Ripard wrote:
> > > On Fri, May 18, 2018 at 07:42:34PM -0700, Sam Bobrowicz wrote:
> 
> > > > This set of patches is also not working for my MIPI platform (mine has
> > > > a 12 MHz external clock). I am pretty sure is isn't working because it
> > > > does not include the following, which my tests have found to be
> > > > necessary:
> > > > 
> > > > 1) Setting pclk period reg in order to correct DPHY timing.
> > > > 2) Disabling of MIPI lanes when streaming not enabled.
> > > > 3) setting mipi_div to 1 when the scaler is disabled
> > > > 4) Doubling ADC clock on faster resolutions.
> > > 
> > > Yeah, I left them out because I didn't think this was relevant to this
> > > patchset but should come as future improvements. However, given that
> > > it works with the parallel bus, maybe the two first are needed when
> > > adjusting the rate.
> > 
> > I've checked for the pclk period, and it's hardcoded to the same value
> > all the time, so I guess this is not the reason it doesn't work on
> > MIPI CSI anymore.
> > 
> > Daniel, could you test:
> > http://code.bulix.org/ki6kgz-339327?raw
> 
> [Note that there's a missing parenthesis in this snippet]

Sorry :/

> Hmm, no, that doesn't change anything. Streaming doesn't work here, even if
> I move ov5640_load_regs() before any other initialization.
> 
> One of my test setup is the following gst pipeline:
> 
>   gst-launch-1.0	\
> 	v4l2src device=/dev/video0 ! \
> 	videoconvert ! \
> 	video/x-raw,format=UYVY,width=1920,height=1080 ! \
> 	glimagesink
> 
> With the pixel clock hard-coded to 166600000 in qcom camss, the setup works
> on 4.14, but as I said, it broke already before this series with
> 5999f381e023 ("media: ov5640: Add horizontal and vertical
> totals").
> 
> Frankly, my understanding of these chips is currently limited, so I don't
> really know where to start digging. It seems clear though that the timing
> registers setup is necessary for other register writes to succeed.
> 
> Can I help in any other way?

If you feel like it, you could go through the various changes
(especially the pclk period I guess) changes Sam pushed in the
previous iteration to his dropbox. That's probably not going to be
quite easy to merge though, so that's going to require some manual
holding.

Sorry for not being able to help more than that :/

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]