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

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

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:
> > On Fri, May 18, 2018 at 3:35 AM, Daniel Mack <daniel@zonque.org> wrote:
> > > On Thursday, May 17, 2018 10:53 AM, Maxime Ripard wrote:
> > >>
> > >> Part of the hardcoded initialization sequence is to set up the proper
> > >> clock
> > >> dividers. However, this is now done dynamically through proper code and as
> > >> such, the static one is now redundant.
> > >>
> > >> Let's remove it.
> > >>
> > >> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > >> ---
> > >
> > >
> > > [...]
> > >
> > >> @@ -625,8 +623,8 @@ static const struct reg_value
> > >> ov5640_setting_30fps_1080P_1920_1080[] = {
> > >>         {0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
> > >>         {0x4001, 0x02, 0, 0}, {0x4004, 0x06, 0, 0}, {0x4713, 0x03, 0, 0},
> > >>         {0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
> > >> -       {0x3824, 0x02, 0, 0}, {0x5001, 0x83, 0, 0}, {0x3035, 0x11, 0, 0},
> > >> -       {0x3036, 0x54, 0, 0}, {0x3c07, 0x07, 0, 0}, {0x3c08, 0x00, 0, 0},
> > >> +       {0x3824, 0x02, 0, 0}, {0x5001, 0x83, 0, 0},
> > >> +       {0x3c07, 0x07, 0, 0}, {0x3c08, 0x00, 0, 0},
> > >
> > >
> > > This is the mode that I'm testing with. Previously, the hard-coded registers
> > > here were:
> > >
> > >  OV5640_REG_SC_PLL_CTRL1 (0x3035) = 0x11
> > >  OV5640_REG_SC_PLL_CTRL2 (0x3036) = 0x54
> > >  OV5640_REG_SC_PLL_CTRL3 (0x3037) = 0x07
> > >
> > > Your new code that calculates the clock rates dynamically ends up with
> > > different values however:
> > >
> > >  OV5640_REG_SC_PLL_CTRL1 (0x3035) = 0x11
> > >  OV5640_REG_SC_PLL_CTRL2 (0x3036) = 0xa8
> > >  OV5640_REG_SC_PLL_CTRL3 (0x3037) = 0x03
> > >
> > > Interestingly, leaving the hard-coded values in the array *and* letting
> > > ov5640_set_mipi_pclk() do its thing later still works. So again it seems
> > > that writes to registers after 0x3035/0x3036/0x3037 seem to depend on the
> > > values of these timing registers. You might need to leave these values as
> > > dummies in the array. Confusing.
> > >
> > > Any idea?
> > >
> > >
> > > Thanks,
> > > Daniel
> > 
> > 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

And let us know the results?

Thanks!
Maxime

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