From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757398AbcAJTo4 (ORCPT ); Sun, 10 Jan 2016 14:44:56 -0500 Received: from down.free-electrons.com ([37.187.137.238]:59166 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1757131AbcAJTox (ORCPT ); Sun, 10 Jan 2016 14:44:53 -0500 Date: Sun, 10 Jan 2016 20:44:50 +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: <20160110194450.GI9631@lukather> References: <1451145186-14235-1-git-send-email-mweseloh42@gmail.com> <1451145186-14235-3-git-send-email-mweseloh42@gmail.com> <20151227210946.GL30359@lukather> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="fmvA4kSBHQVZhkR6" 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 --fmvA4kSBHQVZhkR6 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Dec 28, 2015 at 05:22:35PM +0100, Marcus Weseloh wrote: > Hi again, >=20 > 2015-12-28 0:29 GMT+01:00 Marcus Weseloh : > > 2015-12-27 22:09 GMT+01:00 Maxime Ripard : > [...] > > [...] > >>> - /* 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 doing > >> 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? > > > > Oh, and I just noticed a mistake in the comment: the clock driver > > rounds up _or_ down, so I should drop the "up". >=20 > As I'm looking further into this, I think the comment should read > "possibly rounded down", as the clk framework is expected to set a > frequency that is less or equal to the requested frequency. AFAIK, there's no such expectation in the clock framework. You treating this from a maximum frequency perspective, but it would be the exact opposite if you were talking about a minimum frequency, as might be the case for other consumers. > So the effect I was seeing (that I got a frequency higher than the > requested one) is actually a bug!? Further details below. >=20 > > [...] > >>> - 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. >=20 > That clk_set_rate sets a higher frequency than requested seems to be a > problem in itself. I had a look at clk/sunxi/clk-mod0.c and noticed a > few small problems there. Will post an RFC patch in a couple of > minutes. Did you post these patches already? I think I missed them if that's the case. Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com --fmvA4kSBHQVZhkR6 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWkrSyAAoJEBx+YmzsjxAgpUEQAMEG0AB0hIufkuCvnbhb4cns ysBNh+tFQWuuCHzb95Yi376f/rcKJjEg+50F3oiY2Id91oUlbJAP4z+8VXD5VKkP f3yH5n5ykqZG1v88ouTV/r8hn1HIq+Xxs7ZSAR9OPJSxTVbnq6U/3pEMPmJ9Fe9m U7PnwmD954c8LCve95aLUJfVaJQ/SK0okJDU5khItzbCLEVzF+5o8Nf08jVVkfwe vK47FrIVgkxADQgn3J0fOuqmcDGDbbmuykzpWLuvrXtu3DHSQ+no0nfDCVTrUHuM VXbqoVnm8kD3kng23qpGaMyti2QLVTtNqQA47TYNUGxqKmzLh8Tts8J5C2Crx9Bn RSw6An/JfPbNRY+1Xr9z9FHpACvVprv+se7+s1ScgVfltrcRMnYZRsfHVFQrJ/rN kX7IxnQmz44R7eiUnzeG6c2L7UGRIqlDAZ4wyl2piKWc4JwDBFAkHOh8RuLTZH9S WHoZUPqS5RCYEUvf3dQQueQItVcLYQBTyAIxUicEPuWoR2vID+eYBLzwDgmSukQu SHGdRiwsDpfPwBAz9aRiVk53zCOucegUXsUoXWyUrpcLGXxh+a+XvRUKCHMxTUmr aSbaNKAsbPvEFNo5sdXgJo8bWXmnV2OfZuSsAse7tEd+vN0cTkwx1Mo4BSLDTVUe MCSvCS03R0j3fR7n/shQ =fg1s -----END PGP SIGNATURE----- --fmvA4kSBHQVZhkR6--