Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754470AbbL0VJx (ORCPT ); Sun, 27 Dec 2015 16:09:53 -0500 Received: from down.free-electrons.com ([37.187.137.238]:56788 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752514AbbL0VJt (ORCPT ); Sun, 27 Dec 2015 16:09:49 -0500 Date: Sun, 27 Dec 2015 22:09:46 +0100 From: Maxime Ripard To: Marcus Weseloh Cc: linux-sunxi@googlegroups.com, Chen-Yu Tsai , devicetree@vger.kernel.org, Ian Campbell , Kumar Gala , linux-arm-kernel@lists.infradead.org, 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: <20151227210946.GL30359@lukather> References: <1451145186-14235-1-git-send-email-mweseloh42@gmail.com> <1451145186-14235-3-git-send-email-mweseloh42@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="9+yvdhHLKXN64C4s" Content-Disposition: inline In-Reply-To: <1451145186-14235-3-git-send-email-mweseloh42@gmail.com> 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 Content-Length: 4717 Lines: 131 --9+yvdhHLKXN64C4s Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Dec 26, 2015 at 04:53:05PM +0100, Marcus Weseloh wrote: > This patch fixes multiple problems with the current clock calculations: >=20 > 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). >=20 > 2. The divisor calculations for CDR1 and CDR2 both rounded to the > nearest integer. This could lead to a transfer speed that is higher than > the requested speed. This patch changes both calculations to always > round down. >=20 > 3. The mclk frequency was only ever increased, never decreased. This could > 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 > Signed-off-by: Marcus Weseloh > --- > drivers/spi/spi-sun4i.c | 26 ++++++++++++++++---------- > 1 file changed, 16 insertions(+), 10 deletions(-) >=20 > diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c > index f60a6d6..d67e142 100644 > --- a/drivers/spi/spi-sun4i.c > +++ b/drivers/spi/spi-sun4i.c > @@ -79,6 +79,9 @@ struct sun4i_spi { > struct clk *hclk; > struct clk *mclk; > =20 > + int cur_max_speed; > + int cur_mclk; > + > struct completion done; > =20 > const u8 *tx_buf; > @@ -227,11 +230,17 @@ static int sun4i_spi_transfer_one(struct spi_master= *master, > =20 > sun4i_spi_write(sspi, SUN4I_CTL_REG, reg); > =20 > - /* 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. > + clk_set_rate(sspi->mclk, 2 * spi->max_speed_hz); > mclk_rate =3D clk_get_rate(sspi->mclk); > + sspi->cur_mclk =3D mclk_rate; > + sspi->cur_max_speed =3D spi->max_speed_hz; > } > =20 > /* > @@ -239,7 +248,7 @@ static int sun4i_spi_transfer_one(struct spi_master *= master, > * > * We have two choices there. Either we can use the clock > * divide rate 1, which is calculated thanks to this formula: > - * SPI_CLK =3D MOD_CLK / (2 ^ (cdr + 1)) > + * SPI_CLK =3D MOD_CLK / (2 ^ cdr) > > * Or we can use CDR2, which is calculated with the formula: > * SPI_CLK =3D MOD_CLK / (2 * (cdr + 1)) > * Wether we use the former or the latter is set through the > @@ -248,14 +257,11 @@ static int sun4i_spi_transfer_one(struct spi_master= *master, > * First try CDR2, and if we can't reach the expected > * frequency, fall back to CDR1. > */ > - 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) ? Thanks, Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com --9+yvdhHLKXN64C4s Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWgFOaAAoJEBx+YmzsjxAgs2IP/RFDIWQpP/Z2ZXbxvNz4Jmm4 +x8B8Nj66ZnzyXeiTIKkLLcVP8FAbcBZQaMWUdItgDhDGi348BrbL1Pu1RiB3JN1 XNsWGjaE3og6Kn+jGzw55b8J7O2+2SVuyH0Ig1mnzhXiDENwN2fU4/yw+zBEM5ZI gEiz91I7/XVIjCBSJoNtKQ2Hu6CrjVNua7gy3N+so1Gi0zi/S6fL3W5lPHgnOCXj uGQoZZaCvL26XmM3ebPaZp9+rY2hG2bFLQOgK1qCEED9wdM5l+AJS7ZTKvdyyIJk vsSW8cByYvh6jAdmelqNgITo7i23iEzpdWp4y6FNivMvcz1fQrJdSlGin0G+UeTv 96Tn8cs2y+GOjAB8j2QmlGR7Jc4juwl8C3JBgxgYOmSDiGF3KymHMf56vGm+Fkue Qxbmx5LaKnThMLJbY2S/PWh84aUCbj5Izv+rQmKZZ+vTrh/sQe2OUwdEVrLYhXtZ yMyXnNT55LgGSO1/kP8F5g1MjUP8XtW2ZEzCkbsCzs99aKxJIsR3zdsKpXjeMJH8 MJRb2ggZFZb/t80aXT9kssSV3rbMO8pBhMbiyuuKsYIistrkf9DxxEnQBMmrR/BO 9gQVeHWd48E5jbBoMezGMWfyxO5jxBoHsWkDR8GVwAjx85lCuaqqUdA8zafAmH7q xtZT3FWhdTBTweL98lKj =pBBh -----END PGP SIGNATURE----- --9+yvdhHLKXN64C4s-- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/