From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752124AbcAQSvt (ORCPT ); Sun, 17 Jan 2016 13:51:49 -0500 Received: from down.free-electrons.com ([37.187.137.238]:42656 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751841AbcAQSvr (ORCPT ); Sun, 17 Jan 2016 13:51:47 -0500 Date: Sun, 17 Jan 2016 19:51:43 +0100 From: Maxime Ripard To: Marcus Weseloh Cc: linux-sunxi , Chen-Yu Tsai , devicetree , Ian Campbell , Kumar Gala , "Mailing List, Arm" , linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org, Mark Brown , Mark Rutland , Pawel Moll , Rob Herring Subject: Re: [PATCH v6 2/3] spi: sun4i: Fix clock calculations to be predictable and never exceed the requested rate Message-ID: <20160117185143.GL4581@lukather> References: <1451145186-14235-1-git-send-email-mweseloh42@gmail.com> <1451145186-14235-3-git-send-email-mweseloh42@gmail.com> <20151227210946.GL30359@lukather> <20160110181428.GH9631@lukather> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="dZihrQ6eCIduWT38" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --dZihrQ6eCIduWT38 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi, On Sun, Jan 10, 2016 at 10:11:11PM +0100, Marcus Weseloh wrote: > >> >> - /* Ensure that we have a parent clock fast enough */ > >> >> + /* > >> >> + * Ensure that the parent clock is set to twice the max speed > >> >> + * of the spi device (possibly rounded up by the clk driver) > >> >> + */ > >> >> mclk_rate =3D clk_get_rate(sspi->mclk); > >> >> - if (mclk_rate < (2 * tfr->speed_hz)) { > >> >> - clk_set_rate(sspi->mclk, 2 * tfr->speed_hz); > >> >> + if (spi->max_speed_hz !=3D sspi->cur_max_speed || > >> >> + mclk_rate !=3D sspi->cur_mclk) { > >> > > >> > Do you need to cache the values? As far as I'm aware, you end up doi= ng > >> > the computation all the time anyway. > >> > >> By caching the values we optimize the case when a single SPI slave > >> device (or even multiple slave devices with the same max_speed) are > >> used multiple times in a row. In that case, we're saving two calls: > >> clk_set_rate and clk_get_rate. I was unsure about how expensive the > >> clk_* calls were, so I thought it would be safer use caching. But > >> maybe it's not worth the extra code? > > > > Unless you can point that there's a significant performance > > difference, I'm not sure it's worth it. >=20 > I did actually notice a significant transfer latency when a new mod0 > clock frequency is set, probably due to the __delay in > drivers/clk/sunxi/clk-factors.c (line 147). So my feeling is that the > caching is worth it... at least for the case when there are two slave > devices with different transfer speeds accessing the same SPI module. I'm not sure we even need that delay in the first place, at least not for all the clocks using factor. AFAIK, the only case where it's useful is for PLL, and all of them have a bit you can busy-wait on that will let you know when the clock has stabilized. > >> [...] > >> >> - div =3D mclk_rate / (2 * tfr->speed_hz); > >> >> - if (div <=3D (SUN4I_CLK_CTL_CDR2_MASK + 1)) { > >> >> - if (div > 0) > >> >> - div--; > >> >> - > >> >> + div =3D DIV_ROUND_UP(mclk_rate, 2 * tfr->speed_hz) - 1; > >> > > >> > Isn't it exactly the same thing as mclk_rate / (2 * tfr->speed_hz) ? > >> > >> It is quite often, but not in all cases. The plain division rounds to > >> the nearest integer, so it rounds down sometimes. Consider the > >> following case: we have a slow SPI device with a spi-max-frequency of > >> 50kHz. Our clock driver can't find a clock as slow as 100kHz, so it > >> sets mclk to 214,285Hz. > >> > >> Using the old calculation we get: 214,285 / (2 * 50,000) =3D 2, so div= =3D > >> 1 as the old code subtracts 1 two lines further down > >> The new calculation results in: DIV_ROUND_UP(214,285, 2 * 50,000) =3D > >> 3, so div =3D 2 if we add the -1 > > > > Except that you have that extra - 1 after your DIV_ROUND_UP > > calculation in the line you add. so you have to remove 1 from that > > line above, and then 1 again when we set the register, which ends up > > being the exact same thing, or am I missing something? >=20 > The -1 after the DIV_ROUND_UP is already the -1 that is needed to set > the register. There's no need for another -1 after that (and there > isn't one in the code). I was probably hallucinating :) My bad. That being said, I still have a hard time figuring out what would not be solved simply by removing the div--, which seems to match what your commit log says. Thanks! Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com --dZihrQ6eCIduWT38 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWm+K/AAoJEBx+YmzsjxAgj6EQALLw0U7Mo36atet6dStaVWba Lbx0ZjlYSYq1iqBopObR5iclyjWBlHBYf6ZgS8ZbJsLjHgdJVZY/M/GS9VAq3QS9 16Mikgr45pmE2FuUKWiTTsbgB36EhNI4L99qXJRpCdePtqJkWBlFuBajgyrD5e0Q LHb5gp0FDB07legWdLCi0be8c1zWCRuI5dbw8gNZiEzQrdgB62EogqwUpkt8tmaZ sTtYjhM2lGo8IgTAZ1LnoqteneVQMv19FcQnO2rRG66Gwr8wOmBsSK1b2IdtNi55 GrxMIYehTRyTyfwyB/bRWU1SE4kgX7n77m0guoF5b7dOAiRAvSJZzYQhy8zmeL+0 ThFsoE8HQoOiBLLQfqpaF/zKQrm7D30MmmVeuSemiq2nhOXLuBh8F/3Ei+YR4+8o dviTMif2zPUPt9lYNQmRDokpBs4SSmjj0Svndno2bCVd3ZesauykDGwg8jldBYYV AtQU+v850TYy2EsOOaq1xh+ww3Vp/Vy7jeH4W6R8EhPYNBiaIVevCmChbFTEgDyI +jYWBMViNfbLQAh28XXUxBCB/vvTOKzclTGkQVxNX/mDOBR12LBD3+m1oOKWoNEN 6FCS2U0Pseg/GgXjedugFKTKZSoGRGuu/jVsZgey/wkvNkefADDvNHF8NWYD5Izn w9mRFLbMgGEjYnfYjLG0 =/6QS -----END PGP SIGNATURE----- --dZihrQ6eCIduWT38--