Hi Jacopo,

Thanks for reviewing this patch

On Tue, Oct 16, 2018 at 06:54:50PM +0200, jacopo mondi wrote:
> > +static unsigned long ov5640_compute_sys_clk(struct ov5640_dev *sensor,
> > +					    u8 pll_prediv, u8 pll_mult,
> > +					    u8 sysdiv)
> > +{
> > +	unsigned long rate = clk_get_rate(sensor->xclk);
> 
> The clock rate is stored in sensor->xclk at probe time, no need to
> query it every iteration.

From a clk API point of view though, there's nothing that guarantees
that the clock rate hasn't changed between the probe and the time
where this function is called.

I appreciate that we're probably connected to an oscillator, but even
then, on the Allwinner SoCs we've had the issue recently that one
oscillator feeding the BT chip was actually had a muxer, with each
option having a slightly different rate, which was bad enough for the
BT chip to be non-functional.

I can definitely imagine the same case happening here for some
SoCs. Plus, the clock framework will cache the rate as well when
possible, so we're not losing anything here.

> > +
> > +	return rate / pll_prediv * pll_mult / sysdiv;
> > +}
> > +
> > +static unsigned long ov5640_calc_sys_clk(struct ov5640_dev *sensor,
> > +					 unsigned long rate,
> > +					 u8 *pll_prediv, u8 *pll_mult,
> > +					 u8 *sysdiv)
> > +{
> > +	unsigned long best = ~0;
> > +	u8 best_sysdiv = 1, best_mult = 1;
> > +	u8 _sysdiv, _pll_mult;
> > +
> > +	for (_sysdiv = OV5640_SYSDIV_MIN;
> > +	     _sysdiv <= OV5640_SYSDIV_MAX;
> > +	     _sysdiv++) {
> > +		for (_pll_mult = OV5640_PLL_MULT_MIN;
> > +		     _pll_mult <= OV5640_PLL_MULT_MAX;
> > +		     _pll_mult++) {
> > +			unsigned long _rate;
> > +
> > +			/*
> > +			 * The PLL multiplier cannot be odd if above
> > +			 * 127.
> > +			 */
> > +			if (_pll_mult > 127 && (_pll_mult % 2))
> > +				continue;
> > +
> > +			_rate = ov5640_compute_sys_clk(sensor,
> > +						       OV5640_PLL_PREDIV,
> > +						       _pll_mult, _sysdiv);
> 
> I'm under the impression a system clock slower than the requested one, even
> if more accurate is not good.
> 
> I'm still working on understanding how all CSI-2 related timing
> parameters play together, but since the system clock is calculated
> from the pixel clock (which comes from the frame dimensions, bpp, and
> rate), and it is then used to calculate the MIPI BIT clock frequency,
> I think it would be better to be a little faster than a bit slower,
> otherwise the serial lane clock wouldn't be fast enough to output
> frames generated by the sensor core (or maybe it would just decrease
> the frame rate and that's it, but I don't think it is just this).
> 
> What do you think of adding the following here:
> 
>                 if (_rate < rate)
>                         continue

I really don't know MIPI-CSI2 enough to be able to comment on your
concerns, but when reaching the end of the operating limit of the
clock, it would prevent us from having any rate at all, which seems
bad too.

> > +			if (abs(rate - _rate) < abs(rate - best)) {
> > +				best = _rate;
> > +				best_sysdiv = _sysdiv;
> > +				best_mult = _pll_mult;
> > +			}
> > +
> > +			if (_rate == rate)
> > +				goto out;
> > +		}
> > +	}
> > +
> > +out:
> > +	*sysdiv = best_sysdiv;
> > +	*pll_prediv = OV5640_PLL_PREDIV;
> > +	*pll_mult = best_mult;
> > +	return best;
> > +}
> 
> These function gets called at s_stream time, and cycle for a while,
> and I'm under the impression the MIPI state machine doesn't like
> delays too much, as I see timeouts on the receiver side.
> 
> I have tried to move this function at set_fmt() time, every time a new
> mode is selected, sysdiv, pll_prediv and pll_mult gets recalculated
> (and stored in the ov5640_dev structure). I now have other timeouts on
> missing EOF, but not anymore at startup time it seems.

I have no objection caching the values if it solves issues with CSI :)

Can you send that patch?

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com