Re: [PATCH v4 01/12] media: ov5640: Adjust the clock based on the expected rate

[Maxime Ripard <maxime.ripard@bootlin.com>]

Hi!

On Thu, Oct 18, 2018 at 03:46:05PM +0200, jacopo mondi wrote:
> Hello Maxime,
> 
> On Thu, Oct 18, 2018 at 11:29:12AM +0200, Maxime Ripard wrote:
> > 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.
> 
> Correct, bell, it can be queried in the caller and re-used here :)
> >
> > 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.
> 
> I see, so please ignore this comment :)
> 
> >
> > > > +
> > > > +	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.
> 
> Are you referring to the 1GHz limit of the (xvlkc / pre_div * mult)
> output here? If that's your concern we should adjust the requested
> SYSCLK rate then (and I added a check for that in my patches on top of
> yours, but it could be improved to be honest, as it just refuses the
> current rate, while it should increment the pre_divider instead, now
> that I think better about that).

I meant to the limits of the PCLK / MIPI bit clock, so the rate we are
expected to reach. But I really don't have any opinion on this, so
I'll just merge your suggestion for the next version.

> >
> > > > +			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?
> 
> Actually I think I was wrong. The timeout I saw have gone with the
> last version I sent, even with this computation performed at
> s_stream() time. And re-thinking this, the MIPI state machine should
> get started after the data lanes are put in LP11 state, which happens
> after this function ends.
> 
> We can discuss however if it is better to do this calculations at
> s_fmt time or s_stream time as a general thing, but I think (also)
> this comment might be ignored for now :)

That works for me :)

Thanks!
Maxime

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