Hi Sam,
On Fri, Jun 01, 2018 at 04:05:58PM -0700, Sam Bobrowicz wrote:
> >> On May 21, 2018, at 12:39 AM, Maxime Ripard <maxime.ripard@xxxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxx>
> >>>> ---
> >>>
> >>>
> >>> [...]
> >>>
> >>>> @@ -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?
> >>
> >> 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 agree that 1-4 are separate improvements to MIPI mode that may not
> affect all modules. They do break mine, but that has been true since
> the driver was released (mainly because of my 12 MHz clock and more
> stringent CSI RX requirements). So it makes sense for me to address
> them in a follow-up series. But I do think that we should get the
> clock generation a little closer to what I know works for MIPI so we
> don't break things for people that do have MIPI working.
I guess it's already a bit too late for that :/
> > 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.
>
> I didn't notice the divide by 4, interesting. I have a theory
> though... it could be that the constraint of PCLK relative to SCLK is:
>
> SCLK*(cpp/scaler ratio)<=PCLK<= ?
> cpp=Components/pixel (1 for JPEG, 2 for YUV, e.g.)
>
> Since the scaler is in auto mode, the scaler ratio might automatically
> change depending on the resolution selected, something like (using int
> math):
>
> (hTotal/hActive) * (vTotal/vActive) = scaler ratio
>
> If SCLK is responsible for reading the data into the scaler, and PCLK
> is responsible for reading data out to the physical interface, this
> would make sense. It will require more experiments to verify any of
> this, though, and unfortunately I don't have a lot of time to put into
> this right now.
To be honest, I really couldn't find any correlation. Some have
different dividers for the two framerates (15 and 30), which wouldn't
make sense in your case, and doubling the resolution doesn't always
result in increasing that divider either.
> On my platform I have also run into an upper bound for PCLK, where it
> seems that PCLK must be <= SCLK when the scaler is enabled. I think
> this may have to do with some of my platform's idiosyncrasies, so I'm
> not ready to say that it needs to be addressed in this series. But if
> others run into it while testing MIPI, you should consider
> implementing #3 above to address it.
>
> > And the faster resolutions were working already, so I guess the ADC
> > clock is already fast enough with a 24MHz oscillator?
>
> That's my theory. It seems to have pretty loose requirements as long
> as it is fast enough, which is why I did the simple 2x solution. It
> doesn't need to be addressed here though. If anyone runs into images
> that are all black or bluish, this is a possible culprit.
>
> I'm back, sorry for the delay. Here is a patch that should fix a few
> things for MIPI users. Just apply it directly after applying the
> series. Someone else should definitely verify this on a different MIPI
> platform.
>
> https://nofile.io/f/W8J3thK7pOp/clock_fixes.patch
>
> These are the noteworthy changes I made.
>
> *Added writes to init blob for 0x3034 (bit div) and 0x3037 (pll r
> div). This is because bit div and pll root div never get written to
> the expected values (8 and 2), so they remain as defaults (10 and 1).
> It would also be possible to modify set_mipi_pclk to just write these
> values there, but I didn't wan't to mess with your functions too much.
>
> *Change MIPI SCLK constraint in comments to match the notes found
> here: https://community.nxp.com/servlet/JiveServlet/downloadImage/105-32914-99951/ov5640_diagram.jpg.
> It seems that the pixels are serialized into components when they
> cross from SCLK to PCLK, so the MIPI serial clock does not care about
> cpp, only bpc (bits/component).
>
> *Lower MIPI DIV to 1 for now. It may be necessary to conditionally set
> it later if people are still having trouble, but always using 2 will
> make PCLK<SCLK*cpp, and definitely break non-scaled resolutions.
>
> *MIPI div register doesn't need a -1. When set to zero, it actually
> divides by 16.
>
> Also, FYI, I've made some improvements to my clock configuration
> spreadsheet and incorporated a register dump function into set_mode
> that prints the relevant registers so they can be copied into the
> spreadsheet for interpretation. Just let me know if anyone wants it.
Thanks!
Daniel, is this fixing your issue?
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
Attachment:
signature.asc
Description: PGP signature
