Hi Sakari, On Fri, Mar 09, 2018 at 01:16:24PM +0200, Sakari Ailus wrote: > > + * > > + * +--------------+ > > + * | Oscillator | > > I wonder if this should be simply called external clock, that's what the > sensor uses.
Ack
> > + * +------+-------+
> > + * |
> > + * +------+-------+
> > + * | System clock | - reg 0x3035, bits 4-7
> > + * +------+-------+
> > + * |
> > + * +------+-------+ - reg 0x3036, for the multiplier
> > + * | PLL | - reg 0x3037, bits 4 for the root divider
> > + * +------+-------+ - reg 0x3037, bits 0-3 for the pre-divider
> > + * |
> > + * +------+-------+
> > + * | SCLK | - reg 0x3108, bits 0-1 for the root divider
> > + * +------+-------+
> > + * |
> > + * +------+-------+
> > + * | PCLK | - reg 0x3108, bits 4-5 for the root divider
> > + * +--------------+
> > + *
> > + * This is deviating from the datasheet at least for the register
> > + * 0x3108, since it's said here that the PCLK would be clocked from
> > + * the PLL. However, changing the SCLK divider value has a direct
> > + * effect on the PCLK rate, which wouldn't be the case if both PCLK
> > + * and SCLK were to be sourced from the PLL.
> > + *
> > + * These parameters also match perfectly the rate that is output by
> > + * the sensor, so we shouldn't have too much factors missing (or they
> > + * would be set to 1).
> > + */
> > +
> > +/*
> > + * FIXME: This is supposed to be ranging from 1 to 16, but the value
> > + * is always set to either 1 or 2 in the vendor kernels.
>
> There could be limits for the clock rates after the first divider. In
> practice the clock rates are mostly one of the common frequencies (9,6; 12;
> 19,2 or 24 MHz) so there's no need to use the other values.
There's probably some limits on the various combinations as well. I
first tried to use the full range, and there was some combinations
that were not usable, even though the clock rate should have been
correct.
> > + */
> > +#define OV5640_SYSDIV_MIN 1
> > +#define OV5640_SYSDIV_MAX 2
> > +
> > +static unsigned long ov5640_calc_sysclk(struct ov5640_dev *sensor,
> > + unsigned long rate,
> > + u8 *sysdiv)
> > +{
> > + unsigned long best = ~0;
> > + u8 best_sysdiv = 1;
> > + u8 _sysdiv;
> > +
> > + for (_sysdiv = OV5640_SYSDIV_MIN;
> > + _sysdiv <= OV5640_SYSDIV_MAX;
> > + _sysdiv++) {
> > + unsigned long tmp;
> > +
> > + tmp = sensor->xclk_freq / _sysdiv;
> > + if (abs(rate - tmp) < abs(rate - best)) {
> > + best = tmp;
> > + best_sysdiv = _sysdiv;
> > + }
> > +
> > + if (tmp == rate)
> > + goto out;
> > + }
> > +
> > +out:
> > + *sysdiv = best_sysdiv;
> > + return best;
> > +}
> > +
> > +/*
> > + * FIXME: This is supposed to be ranging from 1 to 8, but the value is
> > + * always set to 3 in the vendor kernels.
> > + */
> > +#define OV5640_PLL_PREDIV_MIN 3
> > +#define OV5640_PLL_PREDIV_MAX 3
>
> Same reasoning here than above. I might leave a comment documenting the
> values the hardware supports, removing FIXME as this isn't really an issue
> as far as I see.
Ok, I'll do it then
> > +
> > +/*
> > + * FIXME: This is supposed to be ranging from 1 to 2, but the value is
> > + * always set to 1 in the vendor kernels.
> > + */
> > +#define OV5640_PLL_ROOT_DIV_MIN 1
> > +#define OV5640_PLL_ROOT_DIV_MAX 1
> > +
> > +#define OV5640_PLL_MULT_MIN 4
> > +#define OV5640_PLL_MULT_MAX 252
> > +
> > +static unsigned long ov5640_calc_pll(struct ov5640_dev *sensor,
> > + unsigned long rate,
> > + u8 *sysdiv, u8 *prediv, u8 *rdiv, u8 *mult)
> > +{
> > + unsigned long best = ~0;
> > + u8 best_sysdiv = 1, best_prediv = 1, best_mult = 1, best_rdiv = 1;
> > + u8 _prediv, _mult, _rdiv;
> > +
> > + for (_prediv = OV5640_PLL_PREDIV_MIN;
> > + _prediv <= OV5640_PLL_PREDIV_MAX;
> > + _prediv++) {
> > + for (_mult = OV5640_PLL_MULT_MIN;
> > + _mult <= OV5640_PLL_MULT_MAX;
> > + _mult++) {
> > + for (_rdiv = OV5640_PLL_ROOT_DIV_MIN;
> > + _rdiv <= OV5640_PLL_ROOT_DIV_MAX;
> > + _rdiv++) {
> > + unsigned long pll;
> > + unsigned long sysclk;
> > + u8 _sysdiv;
> > +
> > + /*
> > + * The PLL multiplier cannot be odd if
> > + * above 127.
> > + */
> > + if (_mult > 127 && !(_mult % 2))
> > + continue;
> > +
> > + sysclk = rate * _prediv * _rdiv / _mult;
> > + sysclk = ov5640_calc_sysclk(sensor, sysclk,
> > + &_sysdiv);
> > +
> > + pll = sysclk / _rdiv / _prediv * _mult;
> > + if (abs(rate - pll) < abs(rate - best)) {
> > + best = pll;
> > + best_sysdiv = _sysdiv;
> > + best_prediv = _prediv;
> > + best_mult = _mult;
> > + best_rdiv = _rdiv;
>
> The smiapp PLL calculator only accepts an exact match. That hasn't been an
> issue previously, I wonder if this would work here, too.
I don't recall which one exactly, but I think at least 480p@30fps
wasn't an exact match in our case. Given the insanely high blanking
periods, we might reduce them in order to fall back to something
exact, but I really wanted to be as close to what was already done in
the bytes array as possible, given the already quite intrusive nature
of the patches.
> I think you could remove the inner loop, too, if put what
> ov5640_calc_sysclk() does here. You know the desired clock rate so you can
> calculate the total divisor (_rdiv * sysclk divider).
I guess we can have two approaches here. Since most of the dividers
are fixed, I could have a much simpler code anyway, or I could keep it
that way if we ever want to extend it.
It seems you imply that you'd like something simpler, so I can even
simplify much more than what's already there if you want.
Maxime
--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
signature.asc
Description: PGP signature
