From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757299AbcAJSOe (ORCPT ); Sun, 10 Jan 2016 13:14:34 -0500 Received: from down.free-electrons.com ([37.187.137.238]:58356 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1757070AbcAJSOc (ORCPT ); Sun, 10 Jan 2016 13:14:32 -0500 Date: Sun, 10 Jan 2016 19:14:28 +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: <20160110181428.GH9631@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="0OWHXb1mYLuhj1Ox" 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 --0OWHXb1mYLuhj1Ox Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi, On Mon, Dec 28, 2015 at 12:29:16AM +0100, Marcus Weseloh wrote: > Hi, >=20 > 2015-12-27 22:09 GMT+01:00 Maxime Ripard : > > On Sat, Dec 26, 2015 at 04:53:05PM +0100, Marcus Weseloh wrote: > >> This patch fixes multiple problems with the current clock calculations: > >> > >> 1. The A10/A20 datasheet contains the formula AHB_CLK / (2^(n+1)) to > >> calculate SPI_CLK from CDR1, but this formula is wrong. The actual > >> formula - determined by analyzing the actual waveforms - is > >> AHB_CLK / (2^n). > >> > >> 2. The divisor calculations for CDR1 and CDR2 both rounded to the > >> nearest integer. This could lead to a transfer speed that is higher th= an > >> the requested speed. This patch changes both calculations to always > >> round down. > >> > >> 3. The mclk frequency was only ever increased, never decreased. This c= ould > >> lead to unpredictable transfer speeds, depending on the order in which > >> transfers with different speeds where serviced by the SPI driver. > > > > These 3 should probably be separate patches (and be Cc'd to stable >=20 > Will do. But I have the feeling that at least 1. and 2. should be in > the same patch as they touch the same lines of code. Do you think that > would be ok? It can also be two subsequent patches that are part of the same serie. > And before CC'ing stable, I would love to have someone with access to > A10 hardware and a scope (or even a bus pirate) check that the A10 SPI > controller does indeed have the same "bug". I strongly think so, but > would sleep better if it could be confirmed. We never noticed any significant difference between the two. By now, if there was any, we probably would be aware of it. And if there's any, we can always send a subsequent patch. > >> - /* 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. >=20 > 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. > Oh, and I just noticed a mistake in the comment: the clock driver > rounds up _or_ down, so I should drop the "up". >=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) ? >=20 > 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 > 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? Thanks! Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com --0OWHXb1mYLuhj1Ox Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWkp+EAAoJEBx+YmzsjxAgBGgP/jkyef3fZVCFs1SeNDo177Ff o3JDGTNnb+vkdkBsBnPzGpZwrzccrNYiG9FgUC8FDOZQNc5n2qZZ1sh7Aotb1Z9W WFhSqDgs8nFRHRYhZSy96F+7ux5gS9KXHHmSSwd3d+5MfBaSVskls1zpO6B61YMl hOnl9I9CdnCEYOHC5+VYAPGDkSm56b77bqKFAY9FVmXakqfvVp65R4Duw3uQ8x6T DXL+GVZEnzFaZASsTEW8+wjLbjF6biwJDqQsPI96NgPXEUNtMaDhIe6kXJIRUm/a nE0L7Tgl8zop25jfo3JfaHofYyqQRGeqvkcq4yhjzDbueIYjhzVhzSNHKrbIiXpN pNEPM1CvwUcw5wM3Q5hcFb3MBkG4aFuY8ahpxMsjBfecSq+xNRWTojaT+4najdYD tpPu4i2BJFYtcblX1Vh80IFAg9x/vCmv3H1cd/g5lpwML/kXxbgdNayMWR38d5bN J1k4UwAHuHORBnTvrn3B898U4/zB2JAQsYuui7LnGiWirdfP0/ssh9IhkbS6UYO0 cMMM2hsU1ZH67XaT+KuFgH4B7mRAgoEzF3nLanxD+OWt5NF2Qtp4kD+dO2eZQFiY SbugLhBvWmmbKGf99ZHhD425MNitTfk2VJToxi5rxNnOohxdjUcoq++HNW3d1qm/ TUN7XkQwlr8IUE2oyGGQ =afZh -----END PGP SIGNATURE----- --0OWHXb1mYLuhj1Ox--