From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751287AbbEYVFH (ORCPT ); Mon, 25 May 2015 17:05:07 -0400 Received: from down.free-electrons.com ([37.187.137.238]:41461 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751045AbbEYVFE (ORCPT ); Mon, 25 May 2015 17:05:04 -0400 Date: Mon, 25 May 2015 23:02:33 +0200 From: Maxime Ripard To: Michal Suchanek Cc: linux-sunxi@googlegroups.com, Ulf Hansson , David =?iso-8859-1?Q?Lanzend=F6rfer?= , Hans de Goede , Chen-Yu Tsai , Peter Griffin , linux-mmc@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/2] mmc: sunxi: fix timeout in sunxi_mmc_oclk_onoff Message-ID: <20150525210233.GN8557@lukather> References: <4011290915a42fce8b9c19d59f3ca9c3289a6817.1432491958.git.hramrach@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="OwXh6gFRjCd3qPCM" Content-Disposition: inline In-Reply-To: <4011290915a42fce8b9c19d59f3ca9c3289a6817.1432491958.git.hramrach@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 --OwXh6gFRjCd3qPCM Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Michal, The patch itself looks good, but there's a few style issues. On Sun, May 24, 2015 at 08:07:32PM +0200, Michal Suchanek wrote: > The 250ms timeout is too short. >=20 > On my system enabling the oclk takes under 50ms and disabling slightly > over 100ms when idle. Under load disabling the clock can take over > 350ms. >=20 > This does not make mmc clock gating look like good option to have on > sunxi but the system should not crash with mmc clock gating enabled > nonetheless. >=20 > This patch sets the timeout to 750ms and adds debug prints which show > how long enabling/disabling the clock took so more data can be collected > from other systems. >=20 > Signed-off-by: Michal Suchanek > --- > drivers/mmc/host/sunxi-mmc.c | 22 +++++++++++++++++++--- > 1 file changed, 19 insertions(+), 3 deletions(-) >=20 > diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c > index 4d3e1ff..7cdeecd 100644 > --- a/drivers/mmc/host/sunxi-mmc.c > +++ b/drivers/mmc/host/sunxi-mmc.c > @@ -595,7 +595,7 @@ static irqreturn_t sunxi_mmc_handle_manual_stop(int i= rq, void *dev_id) > =20 > static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 oclk_en) > { > - unsigned long expire =3D jiffies + msecs_to_jiffies(250); > + unsigned long start, end; > u32 rval; > =20 > rval =3D mmc_readl(host, REG_CLKCR); > @@ -604,6 +604,8 @@ static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host= *host, u32 oclk_en) > if (oclk_en) > rval |=3D SDXC_CARD_CLOCK_ON; > =20 > + start =3D jiffies; > + end =3D start + msecs_to_jiffies(750); > mmc_writel(host, REG_CLKCR, rval); > =20 > rval =3D SDXC_START | SDXC_UPCLK_ONLY | SDXC_WAIT_PRE_OVER; > @@ -611,15 +613,29 @@ static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_ho= st *host, u32 oclk_en) > =20 > do { > rval =3D mmc_readl(host, REG_CMDR); > - } while (time_before(jiffies, expire) && (rval & SDXC_START)); > + } while (time_before(jiffies, end) && (rval & SDXC_START)); > + end =3D jiffies; > =20 > /* clear irq status bits set by the command */ > mmc_writel(host, REG_RINTR, > mmc_readl(host, REG_RINTR) & ~SDXC_SDIO_INTERRUPT); > =20 > if (rval & SDXC_START) { > - dev_err(mmc_dev(host->mmc), "fatal err update clk timeout\n"); > + dev_err(mmc_dev(host->mmc), > + "fatal err update oclk timeout. Could not %s in %ims.\n", > + oclk_en ? "enable" : "disable", > + jiffies_to_msecs(end-start)); You should have spaces around that operator. > return -EIO; > + } else { > + int msecs =3D jiffies_to_msecs(end-start); Ditto. > + const char *ing =3D oclk_en ? "enabling" : "disabling"; > + > + if ((msecs > 150) || (oclk_en && (msecs > 50))) > + dev_warn(mmc_dev(host->mmc), > + "%s oclk took %ims", ing, msecs); > + else > + dev_dbg(mmc_dev(host->mmc), > + "%s oclk took %ims", ing, msecs); And this should be aligned with the opening parenthesis. Thanks! Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com --OwXh6gFRjCd3qPCM Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVY43pAAoJEBx+YmzsjxAg75IQAKY4EotKp9eCqg4RbN+4gWRf eAlnsRAnkiO/sa5aPK8k9g5wjUOfGIECg1Rh9rWNrOxLoObvmNSzfvJ2sE3P9Hrh li4pNwuyIte18iO7SLmKnWkIl3drVK91Law8jFP7EWrUM2rO+c0GpMy96b3+VflT TLg+at/6HhyToV4JKMw8PYh9o15scjq3GJ6GWopn6rANcUrEex8Nxnf6ftUDtNRp ks/K9VJCwSjQ9j2ybi+R0aEe9bUyKtPxQctP3MvcOmsIKO0ATJA+pX6ddeozWDA2 Rjl0FblocZSGjOPMXst4j+VDHLMFuAU4zRByt1Nit4Auf449NvgzMOORCB6/je/u aKRX0DJwq9GVfy+Qe/8w79fCHKbjYJtED1dwzbqIc+H/fmQNha9xO5Pd6Tml+6k6 VX5InSQDIvX2P6NUnodCAJ4cETDSPvbzliaX1euzXL2IFQfAMlPG7zrCdFNhFJHQ 5/+t/6Oj0YVNYPtm7OHObasRSjVwPBEPpoYAsW/QK4fqNi5A7q5LG30Su/CTNP+8 WRBmcmRr4tQeFJUtA7MPjWgzgGbKhZOJwZaAZgoYses8lSVmx1sZNnZgCeBtH1U9 Vlqq6lOWCQDyzumIejZm0i9pdMjURt9Z2GZT2siV8Gva7AiCw8Ms32izWfPfDsB+ gWIvgt7SNRUnFE7GJrxA =HkXB -----END PGP SIGNATURE----- --OwXh6gFRjCd3qPCM--