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

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

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

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.

The mipi divider however seems to be a bit more complicated than you
report here. It is indeed set to 1 when the scaler is enabled (all
resolutions > 1280 * 960), but it's also set to 4 in some cases
(640x480@30, 320x240@30, 176x144@30). I couldn't really find any
relationship between the resolution/framerate and whether to use a
divider of 2 or 4.

And the faster resolutions were working already, so I guess the ADC
clock is already fast enough with a 24MHz oscillator?

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