From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Horman Subject: Re: [PATCH 3/5] mmc: sdhi: Add write16_hook Date: Mon, 20 Jun 2011 22:40:51 +0900 Message-ID: <20110620134045.GD9325@verge.net.au> References: <1308550015-15717-1-git-send-email-horms@verge.net.au> <1308550015-15717-4-git-send-email-horms@verge.net.au> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-sh-owner@vger.kernel.org To: Magnus Damm Cc: linux-mmc@vger.kernel.org, linux-sh@vger.kernel.org, Guennadi Liakhovetski , Paul Mundt , Chris Ball List-Id: linux-mmc@vger.kernel.org On Mon, Jun 20, 2011 at 04:04:04PM +0900, Magnus Damm wrote: > On Mon, Jun 20, 2011 at 3:06 PM, Simon Horman wr= ote: > > Some controllers require waiting for the bus to become idle > > before writing to some registers. I have implemented this > > by adding a hook to sd_ctrl_write16() and implementing > > a hook for SDHI which waits for the bus to become idle. > > > > Cc: Guennadi Liakhovetski > > Cc: Magnus Damm > > Signed-off-by: Simon Horman > > > > --- >=20 > Hi Simon, >=20 > Thanks for your work on this! >=20 > > --- a/drivers/mmc/host/sh_mobile_sdhi.c > > +++ b/drivers/mmc/host/sh_mobile_sdhi.c > > @@ -55,6 +57,37 @@ static int sh_mobile_sdhi_get_cd(struct platform= _device *pdev) > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -ENOS= YS; > > =C2=A0} > > > > +static void sh_mobile_sdhi_wait_idle(struct tmio_mmc_host *host) > > +{ > > + =C2=A0 =C2=A0 =C2=A0 int timeout =3D 1000; > > + > > + =C2=A0 =C2=A0 =C2=A0 while (--timeout && !(sd_ctrl_read16(host, C= TL_STATUS2) & (1 << 13))) > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 udelay(1); > > + > > + =C2=A0 =C2=A0 =C2=A0 if (!timeout) > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 dev_warn(host->p= data->dev, "timeout waiting for SD bus idle\n"); > > + > > +} > > + > > +static void sh_mobile_sdhi_write16_hook(struct tmio_mmc_host *host= , int addr) > > +{ > > + =C2=A0 =C2=A0 =C2=A0 if (!(host->pdata->flags & TMIO_MMC_HAS_IDLE= _WAIT)) > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return; >=20 > and >=20 > > @@ -86,6 +119,7 @@ static int __devinit sh_mobile_sdhi_probe(struct= platform_device *pdev) > > =C2=A0 =C2=A0 =C2=A0 =C2=A0mmc_data->hclk =3D clk_get_rate(priv->cl= k); > > =C2=A0 =C2=A0 =C2=A0 =C2=A0mmc_data->set_pwr =3D sh_mobile_sdhi_set= _pwr; > > =C2=A0 =C2=A0 =C2=A0 =C2=A0mmc_data->get_cd =3D sh_mobile_sdhi_get_= cd; > > + =C2=A0 =C2=A0 =C2=A0 mmc_data->write16_hook =3D sh_mobile_sdhi_wr= ite16_hook; > > =C2=A0 =C2=A0 =C2=A0 =C2=A0mmc_data->capabilities =3D MMC_CAP_MMC_H= IGHSPEED; > > =C2=A0 =C2=A0 =C2=A0 =C2=A0if (p) { > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0mmc_data->fl= ags =3D p->tmio_flags; >=20 > Doesn't it make more sense to do: >=20 > if (!(host->pdata->flags & TMIO_MMC_HAS_IDLE_WAIT)) > mmc_data->write16_hook =3D sh_mobile_sdhi_write16_hook; >=20 > and skip the check inside the sh_mobile_sdhi_write16_hook() function? >=20 > No need to do that check on every register access unless really neede= d. Sure, good idea. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Horman Date: Mon, 20 Jun 2011 13:40:51 +0000 Subject: Re: [PATCH 3/5] mmc: sdhi: Add write16_hook Message-Id: <20110620134045.GD9325@verge.net.au> List-Id: References: <1308550015-15717-1-git-send-email-horms@verge.net.au> <1308550015-15717-4-git-send-email-horms@verge.net.au> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: Magnus Damm Cc: linux-mmc@vger.kernel.org, linux-sh@vger.kernel.org, Guennadi Liakhovetski , Paul Mundt , Chris Ball On Mon, Jun 20, 2011 at 04:04:04PM +0900, Magnus Damm wrote: > On Mon, Jun 20, 2011 at 3:06 PM, Simon Horman wrote: > > Some controllers require waiting for the bus to become idle > > before writing to some registers. I have implemented this > > by adding a hook to sd_ctrl_write16() and implementing > > a hook for SDHI which waits for the bus to become idle. > > > > Cc: Guennadi Liakhovetski > > Cc: Magnus Damm > > Signed-off-by: Simon Horman > > > > --- > > Hi Simon, > > Thanks for your work on this! > > > --- a/drivers/mmc/host/sh_mobile_sdhi.c > > +++ b/drivers/mmc/host/sh_mobile_sdhi.c > > @@ -55,6 +57,37 @@ static int sh_mobile_sdhi_get_cd(struct platform_device *pdev) > >                return -ENOSYS; > >  } > > > > +static void sh_mobile_sdhi_wait_idle(struct tmio_mmc_host *host) > > +{ > > +       int timeout = 1000; > > + > > +       while (--timeout && !(sd_ctrl_read16(host, CTL_STATUS2) & (1 << 13))) > > +               udelay(1); > > + > > +       if (!timeout) > > +               dev_warn(host->pdata->dev, "timeout waiting for SD bus idle\n"); > > + > > +} > > + > > +static void sh_mobile_sdhi_write16_hook(struct tmio_mmc_host *host, int addr) > > +{ > > +       if (!(host->pdata->flags & TMIO_MMC_HAS_IDLE_WAIT)) > > +               return; > > and > > > @@ -86,6 +119,7 @@ static int __devinit sh_mobile_sdhi_probe(struct platform_device *pdev) > >        mmc_data->hclk = clk_get_rate(priv->clk); > >        mmc_data->set_pwr = sh_mobile_sdhi_set_pwr; > >        mmc_data->get_cd = sh_mobile_sdhi_get_cd; > > +       mmc_data->write16_hook = sh_mobile_sdhi_write16_hook; > >        mmc_data->capabilities = MMC_CAP_MMC_HIGHSPEED; > >        if (p) { > >                mmc_data->flags = p->tmio_flags; > > Doesn't it make more sense to do: > > if (!(host->pdata->flags & TMIO_MMC_HAS_IDLE_WAIT)) > mmc_data->write16_hook = sh_mobile_sdhi_write16_hook; > > and skip the check inside the sh_mobile_sdhi_write16_hook() function? > > No need to do that check on every register access unless really needed. Sure, good idea.