Hi Laurent,
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.
- "div" _is_ the divider value but it's not stored into the pll_cfg,
instead (div / 2) - 1 is stored there.
And calculating max_pre_div is probably not right above, I think it
should be min(), 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 <=.
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.
Tomi
