Hi Tomi,
On Mon, Jan 18, 2021 at 04:04:47PM +0200, Tomi Valkeinen wrote:
> On 04/01/2021 07:39, Laurent Pinchart wrote:
>
> > +static int ov1063x_pll_setup(unsigned int clk_rate,
> > + unsigned int *htsmin, unsigned int vts,
> > + unsigned int fps_numerator,
> > + unsigned int fps_denominator,
> > + struct ov1063x_pll_config *cfg)
> > +{
> > + static const unsigned int pre_divs[] = { 2, 3, 4, 6, 8, 10, 12, 14 };
> > +
> > + unsigned int best_pclk = UINT_MAX;
> > + unsigned int best_pre_div;
> > + unsigned int best_mult;
> > + unsigned int best_div;
> > + unsigned int best_hts;
> > + unsigned int max_pre_div;
> > + unsigned int pre_div;
> > + unsigned int hts;
> > +
> > + /*
> > + * XVCLK --> pre-div -------> mult ----------> div --> output
> > + * 6-27 MHz 3-27 MHz 200-500 MHz Max 96 MHz
> > + *
> > + * Valid pre-divider values are 1, 1.5, 2, 3, 4, 5, 6 and 7. The
> > + * pre_divs array stores the pre-dividers multiplied by two, indexed by
> > + * register values.
> > + *
> > + * Valid multiplier values are [1, 63], stored as-is in registers.
> > + *
> > + * Valid divider values are 2 to 16 with a step of 2, stored in
> > + * registers as (div / 2) - 1.
> > + */
> > +
> > + if (clk_rate < 6 * 1000 * 1000 || clk_rate > 27 * 1000 * 1000)
> > + return -EINVAL;
> > +
> > + /*
> > + * We try all valid combinations of settings for the 3 blocks to get
> > + * the pixel clock, and from that calculate the actual hts/vts to use.
> > + * The vts is extended so as to achieve the required frame rate.
> > + */
> > +
> > + max_pre_div = max(clk_rate / (3 * 1000 * 1000),
> > + ARRAY_SIZE(pre_divs) - 1);
> > +
> > + for (pre_div = 0; pre_div <= max_pre_div; pre_div++) {
> > + unsigned int clk1 = clk_rate * 2 / pre_divs[pre_div];
> > + unsigned int min_mult;
> > + unsigned int max_mult;
> > + unsigned int mult;
>
> This PLL setup is a bit confusing to understand, because:
>
> - "pre_div" is not the divider value, but an index to the pre_divs array
> and a value to be written into the register, and pre_div is also stored
> into the pll_cfg.
Correct.
> - "div" _is_ the divider value but it's not stored into the pll_cfg,
> instead (div / 2) - 1 is stored there.
Correct too. cfg->div stores the register value.
> And calculating max_pre_div is probably not right above, I think it
> should be min(),
Indeed. Good catch, I'll fix that.
> and additionally "clk_rate / (3 * 1000 * 1000)" is
> calculating the divider value, not the index, but it's then used as a
> max to the index loop... And even if it were the index, it should be -1,
> as the loop check uses <=.
Those are real issues too, I'll address them.
> Suggestions:
>
> - Redefine the PLL formula. Instead of having fractional pre_divider (I
> wonder if it's actually fractional in the HW... Aren't dividers usually
> integer dividers?), redefine the formula as pclk = xvclk * 2 / pre_div *
> mul / div, and say that the possible pre_dividers are what's currently
> in pre_divs array. Or pclk = xvclk / pre_div * 2 * mul / div, which
> gives a different result with integer divisions. I don't know which one
> is the correct one (or is it either of those... If the HW handles
> fractionals, both are wrong).
>
> - Clearly separate divider value and index/regvalue variables. The
> iteration loop should use the plain divider values, and I think it would
> be best to store the values as such to pll_config. And map the divider
> values to register values only when writing to the register.
I'd agree if it wasn't for the pre-div value. Storing the pre-div value
in pll_config would make it more complicated to calculate the register
value in the caller. And mixing register values and multiplier/divider
values in the structure isn't nice :-S That's why I've stored register
values only in pll_config.
How about this ?
commit b2356f1b87576a540ca99fbcd2ad2f0b81c494b7
Author: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
Date: Wed Jan 20 10:25:43 2021 +0200
media: i2c: ov1063x: Fix PLL calculation
Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
diff --git a/drivers/media/i2c/ov1063x.c b/drivers/media/i2c/ov1063x.c
index cfd852b3eb2f..cd8d6bba3d70 100644
--- a/drivers/media/i2c/ov1063x.c
+++ b/drivers/media/i2c/ov1063x.c
@@ -642,24 +642,21 @@ static int ov1063x_pll_setup(unsigned int clk_rate,
unsigned int fps_denominator,
struct ov1063x_pll_config *cfg)
{
- static const unsigned int pre_divs[] = { 2, 3, 4, 6, 8, 10, 12, 14 };
-
unsigned int best_pclk = UINT_MAX;
- unsigned int best_pre_div;
+ unsigned int best_pre_div_x2;
unsigned int best_mult;
unsigned int best_div;
unsigned int best_hts;
- unsigned int max_pre_div;
- unsigned int pre_div;
+ unsigned int max_pre_div_x2;
+ unsigned int pre_div_x2;
unsigned int hts;
/*
* XVCLK --> pre-div -------> mult ----------> div --> output
* 6-27 MHz 3-27 MHz 200-500 MHz Max 96 MHz
*
- * Valid pre-divider values are 1, 1.5, 2, 3, 4, 5, 6 and 7. The
- * pre_divs array stores the pre-dividers multiplied by two, indexed by
- * register values.
+ * Valid pre-divider values are 1, 1.5, 2, 3, 4, 5, 6 and 7, stored in
+ * registers as the index in this list of values.
*
* Valid multiplier values are [1, 63], stored as-is in registers.
*
@@ -676,11 +673,15 @@ static int ov1063x_pll_setup(unsigned int clk_rate,
* The vts is extended so as to achieve the required frame rate.
*/
- max_pre_div = max(clk_rate / (3 * 1000 * 1000),
- ARRAY_SIZE(pre_divs) - 1);
+ /*
+ * The pre_div_x2 variable stores the pre-div value multiplied by 2, to
+ * support the fractional divider 1.5.
+ */
+ max_pre_div_x2 = min(clk_rate * 2 / (3 * 1000 * 1000), 14U);
- for (pre_div = 0; pre_div <= max_pre_div; pre_div++) {
- unsigned int clk1 = clk_rate * 2 / pre_divs[pre_div];
+ for (pre_div_x2 = 2; pre_div_x2 <= max_pre_div_x2;
+ pre_div_x2 += (pre_div_x2 < 4 ? 1 : 2)) {
+ unsigned int clk1 = clk_rate * 2 / pre_div_x2;
unsigned int min_mult;
unsigned int max_mult;
unsigned int mult;
@@ -720,7 +721,7 @@ static int ov1063x_pll_setup(unsigned int clk_rate,
if (pclk < best_pclk) {
best_pclk = pclk;
best_hts = hts;
- best_pre_div = pre_div;
+ best_pre_div_x2 = pre_div_x2;
best_mult = mult;
best_div = div;
}
@@ -731,8 +732,10 @@ static int ov1063x_pll_setup(unsigned int clk_rate,
if (best_pclk == UINT_MAX)
return -EINVAL;
+ /* Store the mult, pre_div and div as register values. */
cfg->mult = best_mult;
- cfg->pre_div = best_pre_div;
+ cfg->pre_div = best_pre_div_x2 < 4 ? best_pre_div_x2 - 2
+ : best_pre_div_x2 / 2;
cfg->div = (best_div / 2) - 1;
cfg->clk_out = best_pclk;
--
Regards,
Laurent Pinchart
