Re: [PATCH v2 00/12] media: ov5640: Misc cleanup and improvements

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

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

Hi,

On Wed, May 02, 2018 at 11:11:55AM -0700, Sam Bobrowicz wrote:
> > On Wednesday, 25 April 2018 01:11:19 EEST Sam Bobrowicz wrote:
> >> FYI, still hard at work on this. Did some more experiments last week
> >> that seemed to corroborate the clock tree in the spreadsheet. It also
> >> seems that the output of the P divider cell, SCLK cell and MIPI Rate
> >> cell in the spreadsheet must have a ratio of 2x:1x:8x (respectively)
> >> in order for the sensor to work properly on my platform, and that the
> >> SCLK value must be close to the "rate" variable that you calculate and
> >> pass to set_mipi_pclk. Unfortunately, I've only got the sensor working
> >> well for 1080p@15Hz and 720p@30Hz, both with a SCLK of 42MHz (aka
> >> 84:42:336). I'm running experiments now trying to adjust the htot and
> >> vtot values to create different required rates, and also to try to get
> >> faster Mipi rates working. Any information you have on the
> >> requirements of the htot and vtot values with respect to vact and hact
> >> values would likely be helpful.
> >>
> >> I'm also keeping an eye on the scaler clock, which I think may be
> >> affecting certain resolutions, but haven't been able to see it make a
> >> difference yet (see register 0x3824 and 0x460c)
> >>
> >> I plan on pushing a set of patches once I get this figured out, we can
> >> discuss what I should base them on when I get closer to that point.
> >> I'm new to this process :)
> >
> > I'm also interested in getting the ov5640 driver working with MIPI CSI-2.
> > Studying the datasheet and the driver code, I found the stream on sequence to
> > be a bit weird. In particular the configuration of OV5640_REG_IO_MIPI_CTRL00,
> > OV5640_REG_PAD_OUTPUT00 and OV5640_REG_MIPI_CTRL00 caught my attention.
> >
> > OV5640_REG_IO_MIPI_CTRL00 (0x300e) is set to 0x45 in the large array of init
> > mode data and never touched for MIPI CSI-2 (the register is only touched in
> > ov5640_set_stream_dvp). The value means
> >
> > - mipi_lane_mode: 010 is documented as "debug mode", I would have expected 000
> > for one lane or 001 for two lanes.
> 
> I noticed this too, but it seems that 010 is the correct value for two
> lane mode. I think this is a typo in the datasheet.
> 
> >
> > - MIPI TX PHY power down: 0 is documented as "debug mode" and 1 as "Power down
> > PHY HS TX", so I suppose 0 is correct.
> >
> > - MIPI RX PHY power down: 0 is documented as "debug mode" and 1 as "Power down
> > PHY LP RX module", so I suppose 0 is correct. I however wonder why there's a
> > RX PHY, it could be a typo.
> >
> > - mipi_en: 1 means MIPI enable, which should be correct.
> >
> > - BIT(0) is undocumented.
> >
> > OV5640_REG_PAD_OUTPUT00 (0x3019) isn't initialized explicitly and thus retains
> > its default value of 0x00, and is controlled when starting and stopping the
> > stream where it's set to 0x00 and 0x70 respectively. Bits 6:4 control the
> > "sleep mode" state of lane 2, lane 1 and clock lane respectively, and should
> > be LP11 in my opinion (that's the PHY stop state). However, setting them to
> > 0x00 when starting the stream mean that LP00 is selected as the sleep state at
> > stream start, and LP11 when stopping the stream. Maybe "sleep mode" means
> > LPDT, but I would expect that to be controlled by the idle status bit in
> > register 0x4800.
> >
> 
> I did not need to mess with the accesses to 0x3019 in order to get
> things working on my system. I'm not sure of this registers actual
> behavior, but it might affect idling while not streaming (after power
> on). My pipeline currently only powers the sensor while streaming, so
> I might be missing some ramifications of this register.
> 
> > OV5640_REG_MIPI_CTRL00 (0x4800) is set to 0x04 in the large array of init mode
> > data, and BIT(5) is then cleared at stream on time and set at stream off time.
> > This means:
> >
> > - Clock lane gate enable: Clock lane is free running
> > - Line sync enable: Do not send line short packets for each line (I assume
> > that's LS/LE)
> > - Lane select: Use lane1 as default data lane.
> > - Idle status: MIPI bus will be LP11 when no packet to transmit. I would have
> > expected the idle status to correspond to LPDT, and thus be LP00 (as opposed
> > to the stop state that should be LP11, which I believe is named "sleep mode"
> > in the datasheet and controlled in register 0x3019).
> >
> > BIT(5) is the clock lane gate enable, so at stream on time the clock is set to
> > free running, and at stream off time to "Gate clock lane when no packet to
> > transmit". Couldn't we always enable clock gating ?
> 
> Good question, it might be worth testing. Same as above, I didn't need
> to mess with this reg either.
> 
> > Do you have any insight on this ? Have you modified the MIPI CSI-2
> > configuration to get the CSI-2 output working ?
> 
> Good news, MIPI is now working on my platform. I had to make several
> changes to how the mipi clocking is calculated in order to get things
> stable, but I think I got it figured out. Maxime's changes were really
> helpful.
> 
> I will try to get some patches out today or tomorrow that should get
> you up and running.
> 
> Maxime, I'd prefer to create the patches myself for the experience.
> I've read all of the submission guidelines and I understand the
> general process, but how should I submit them to the mailing list
> since they are based to your patches, which are still under review?
> Should I send the patch series to the mailing list myself and just
> mention this patch series, maybe with the In-Reply-To header? Or
> should I just post a link to them here so you can review them and add
> them to a new version of your patch series?

That's definitely something you can do if you want to try it out :)

However, we shouldn't break the bisectability either, so this will
need to be folded into this serie eventually.

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