From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail.bootlin.com ([62.4.15.54]:59207 "EHLO mail.bootlin.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726903AbeJSSYD (ORCPT ); Fri, 19 Oct 2018 14:24:03 -0400 Date: Thu, 18 Oct 2018 18:56:54 +0200 From: Maxime Ripard To: jacopo mondi Cc: Mauro Carvalho Chehab , Laurent Pinchart , linux-media@vger.kernel.org, Thomas Petazzoni , Mylene Josserand , Hans Verkuil , Sakari Ailus , Hugues Fruchet , Loic Poulain , Samuel Bobrowicz , Steve Longerbeam , Daniel Mack Subject: Re: [PATCH v4 01/12] media: ov5640: Adjust the clock based on the expected rate Message-ID: <20181018165654.ecb5hpotay4qzurg@flea> References: <20181011092107.30715-1-maxime.ripard@bootlin.com> <20181011092107.30715-2-maxime.ripard@bootlin.com> <20181016165450.GB11703@w540> <20181018092912.u23arvx5ope24m5t@flea> <20181018134605.GH17549@w540> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: 8BIT In-Reply-To: <20181018134605.GH17549@w540> Sender: linux-media-owner@vger.kernel.org List-ID: 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