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).
>
> > > + 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 :)
Thanks
j
>
> Thanks!
> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
signature.asc
Description: PGP signature
