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

[jacopo mondi]

Hi ov5640 people,
I'm happy to finally jump on the bandwagon...

I had a few days to test the sensor driver (on 2 platforms) and unfortunately
this series breaks MIPI capture for me as well..

On Fri, Jun 01, 2018 at 04:05:58PM -0700, Sam Bobrowicz wrote:
> >> On May 21, 2018, at 12:39 AM, 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  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 
>  ---
> >>>
> >>>
> >>> [...]
> >>>
>  @@ -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 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.
>
> > 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.
>
> 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 

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

[Maxime Ripard]

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  
> >> 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  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 
>  ---
> >>>
> >>>
> >>> [...]
> >>>
>  @@ -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 

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

[Sam Bobrowicz]

>> On May 21, 2018, at 12:39 AM, 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  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 
 ---
>>>
>>>
>>> [...]
>>>
 @@ -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 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.

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

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 

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

[Maxime Ripard]

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


signature.asc
Description: PGP signature

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

[Daniel Mack]

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]

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 16660 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?


Thanks for all your efforts,
Daniel

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

[Maxime Ripard]

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  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 
> > >> ---
> > >
> > >
> > > [...]
> > >
> > >> @@ -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

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

[Maxime Ripard]

On Fri, May 18, 2018 at 07:42:34PM -0700, Sam Bobrowicz wrote:
> On Fri, May 18, 2018 at 3:35 AM, Daniel Mack  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 
> >> ---
> >
> >
> > [...]
> >
> >> @@ -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


signature.asc
Description: PGP signature

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

[Daniel Mack]

Hi,

On Saturday, May 19, 2018 04:42 AM, 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.

I will run some more tests to see if anything else is broken and come
back with some suggestions.

I should mention that the upstream driver has never worked with my
platform. I suspect that the driver only ever worked previously with
MIPI platforms that have loose DPHY timing requirements and a specific
xclk (24MHz maybe?). Out of the interest of collecting more data, can
you provide the following info on your platform?

a) External clock frequency


Mine has a 24MHz oscillator.


b) List of resolutions (including framerates) that are working with
these patches (and your fix) applied


I have a somewhat limited support in userspace which is currently 
hard-coded to 1920x1080@30fps. I haven't tested any other resolution, 
but this one is not working with this patch set.



c) List of resolutions that were working prior to the regression you
experienced with the set_timings function


The one mentioned above did work before, except for one things: I need a 
patch on top that adds a V4L2_CID_PIXEL_RATE control. The qcom camss 
platform needs that in order to calculate its own clock rates. When I 
tested this patch set, I hard-coded the setting the camss driver.


I can send a patch that adds this control once this patch set has landed.


Thanks,
Daniel

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

[Sam Bobrowicz]

On Fri, May 18, 2018 at 3:35 AM, Daniel Mack  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 
>> ---
>
>
> [...]
>
>> @@ -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.

I will run some more tests to see if anything else is broken and come
back with some suggestions.

I should mention that the upstream driver has never worked with my
platform. I suspect that the driver only ever worked previously with
MIPI platforms that have loose DPHY timing requirements and a specific
xclk (24MHz maybe?). Out of the interest of collecting more data, can
you provide the following info on your platform?

a) External clock frequency
b) List of resolutions (including framerates) that are working with
these patches (and your fix) applied
c) List of resolutions that were working prior to the regression you
experienced with the set_timings function

Sam

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

[Daniel Mack]

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


[...]


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