Re: [PATCH] mmc: renesas_sdhi: use extra flag for CBSY usage

[Wolfram Sang]

> Is this supposed to have any impact on the issue on Magnus' r8a7794/alt, where
> tuning failed before?

Nope, I'd be very surprised if it does. I wanted to look at this issue
with my next free time slot.



signature.asc
Description: PGP signature

Re: [PATCH] mmc: renesas_sdhi: use extra flag for CBSY usage

[Geert Uytterhoeven]

Hi Wolfram,

On Wed, Aug 9, 2017 at 9:00 PM, Wolfram Sang
 wrote:
> There is one SDHI instance on Gen2 which does not have the CBSY bit.
> So, turn CBSY usage into an extra flag and set it accordingly. This has
> the additional advantage that we can also set it for other incarnations
> later.
>
> Signed-off-by: Wolfram Sang 
> ---
>
> Tested on a M3-W Salvator-X and H2 Lager (with both SDHI instances). I could
> move large files around without problems. Note that on the special incarnation
> without the CBSY bit, the old code still worked, so there might be a CBSY bit.
> But it is not documented, so we better play safe.

Is this supposed to have any impact on the issue on Magnus' r8a7794/alt, where
tuning failed before?

sh_mobile_sdhi ee10.sd: timeout waiting for hardware interrupt (CMD19)
sh_mobile_sdhi ee10.sd: timeout waiting for hardware interrupt (CMD19)
sh_mobile_sdhi ee10.sd: Tuning procedure failed
mmc1: tuning execution failed: -110
mmc1: error -110 whilst initialising SD card

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH] mmc: renesas_sdhi: use extra flag for CBSY usage

[Wolfram Sang]

On Thu, Aug 10, 2017 at 09:58:50AM +0200, Simon Horman wrote:
> On Wed, Aug 09, 2017 at 09:00:41PM +0200, Wolfram Sang wrote:
> > There is one SDHI instance on Gen2 which does not have the CBSY bit.
> > So, turn CBSY usage into an extra flag and set it accordingly. This has
> > the additional advantage that we can also set it for other incarnations
> > later.
> 
> What is the run-time effect, if any, of this change?

The run-time effect is that a few cycles are saved because CBSY signals
ready a bit earlier than SCLKDIVEN. Though, the more important aspect is
that docs explicitly say to wait for CBSY. That's why this change was
requested although I don't think it will matter much in practice.

> > Chris: according to the specs I have, we can enable it for RZ as well. I'll
> > send a patch for that in a minute. If you could test this on top of this 
> > one,
> > that would be much appreciated! Thanks.
> 
> I take it from the above you did a pass of the available documentation to
> see which SoCs support this feature. If so, thanks!

I don't have docs for R-Car Gen1 SDHI. But given that even Gen2 has one
SDHI instance without CBSY and Gen1 is older, my educated guess is that
it does not have CBSY.



signature.asc
Description: PGP signature

Re: [PATCH] mmc: renesas_sdhi: use extra flag for CBSY usage

[Simon Horman]

On Wed, Aug 09, 2017 at 09:00:41PM +0200, Wolfram Sang wrote:
> There is one SDHI instance on Gen2 which does not have the CBSY bit.
> So, turn CBSY usage into an extra flag and set it accordingly. This has
> the additional advantage that we can also set it for other incarnations
> later.

What is the run-time effect, if any, of this change?

> Signed-off-by: Wolfram Sang 

Code looks good:

Reviewed-by: Simon Horman 

> ---
> 
> Tested on a M3-W Salvator-X and H2 Lager (with both SDHI instances). I could
> move large files around without problems. Note that on the special incarnation
> without the CBSY bit, the old code still worked, so there might be a CBSY bit.
> But it is not documented, so we better play safe.
> 
> Chris: according to the specs I have, we can enable it for RZ as well. I'll
> send a patch for that in a minute. If you could test this on top of this one,
> that would be much appreciated! Thanks.

I take it from the above you did a pass of the available documentation to
see which SoCs support this feature. If so, thanks!

>  drivers/mmc/host/renesas_sdhi_core.c  | 6 +-
>  drivers/mmc/host/renesas_sdhi_internal_dmac.c | 3 ++-
>  drivers/mmc/host/renesas_sdhi_sys_dmac.c  | 6 --
>  include/linux/mfd/tmio.h  | 3 +++
>  4 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/host/renesas_sdhi_core.c 
> b/drivers/mmc/host/renesas_sdhi_core.c
> index a252145097d6a5..e46504edc9fcb4 100644
> --- a/drivers/mmc/host/renesas_sdhi_core.c
> +++ b/drivers/mmc/host/renesas_sdhi_core.c
> @@ -429,7 +429,7 @@ static int renesas_sdhi_write16_hook(struct tmio_mmc_host 
> *host, int addr)
>   case CTL_TRANSACTION_CTL:
>   case CTL_DMA_ENABLE:
>   case EXT_ACC:
> - if (host->pdata->flags & TMIO_MMC_MIN_RCAR2)
> + if (host->pdata->flags & TMIO_MMC_HAVE_CBSY)
>   bit = TMIO_STAT_CMD_BUSY;
>   /* fallthrough */
>   case CTL_SD_CARD_CLK_CTL:
> @@ -589,6 +589,10 @@ int renesas_sdhi_probe(struct platform_device *pdev,
>   if (ret < 0)
>   goto efree;
>  
> + /* One Gen2 SDHI incarnation does NOT have a CBSY bit */
> + if (sd_ctrl_read16(host, CTL_VERSION) == SDHI_VER_GEN2_SDR50)
> + mmc_data->flags &= ~TMIO_MMC_HAVE_CBSY;
> +
>   /* Enable tuning iff we have an SCC and a supported mode */
>   if (of_data && of_data->scc_offset &&
>   (host->mmc->caps & MMC_CAP_UHS_SDR104 ||
> diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c 
> b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> index 6717003888e292..fa4e71d3b47d47 100644
> --- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> +++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> @@ -72,7 +72,8 @@ static struct renesas_sdhi_scc rcar_gen3_scc_taps[] = {
>  
>  static const struct renesas_sdhi_of_data of_rcar_gen3_compatible = {
>   .tmio_flags = TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_WRPROTECT_DISABLE |
> -   TMIO_MMC_CLK_ACTUAL | TMIO_MMC_MIN_RCAR2,
> +   TMIO_MMC_CLK_ACTUAL | TMIO_MMC_HAVE_CBSY |
> +   TMIO_MMC_MIN_RCAR2,
>   .capabilities   = MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
> MMC_CAP_CMD23,
>   .bus_shift  = 2,

Keeping the above of_rcar_gen3_compatible structure in sync with the one
below may get painful at some point. Perhaps we should consider a common
location for it? (Yes, I know this is my handiwork)

> diff --git a/drivers/mmc/host/renesas_sdhi_sys_dmac.c 
> b/drivers/mmc/host/renesas_sdhi_sys_dmac.c
> index 718cb8a9d2ce8d..9b77f521cd2c2f 100644
> --- a/drivers/mmc/host/renesas_sdhi_sys_dmac.c
> +++ b/drivers/mmc/host/renesas_sdhi_sys_dmac.c
> @@ -58,7 +58,8 @@ static struct renesas_sdhi_scc rcar_gen2_scc_taps[] = {
>  
>  static const struct renesas_sdhi_of_data of_rcar_gen2_compatible = {
>   .tmio_flags = TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_WRPROTECT_DISABLE |
> -   TMIO_MMC_CLK_ACTUAL | TMIO_MMC_MIN_RCAR2,
> +   TMIO_MMC_CLK_ACTUAL | TMIO_MMC_HAVE_CBSY |
> +   TMIO_MMC_MIN_RCAR2,
>   .capabilities   = MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
> MMC_CAP_CMD23,
>   .dma_buswidth   = DMA_SLAVE_BUSWIDTH_4_BYTES,
> @@ -78,7 +79,8 @@ static struct renesas_sdhi_scc rcar_gen3_scc_taps[] = {
>  
>  static const struct renesas_sdhi_of_data of_rcar_gen3_compatible = {
>   .tmio_flags = TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_WRPROTECT_DISABLE |
> -   TMIO_MMC_CLK_ACTUAL | TMIO_MMC_MIN_RCAR2,
> +   TMIO_MMC_CLK_ACTUAL | TMIO_MMC_HAVE_CBSY |
> +   TMIO_MMC_MIN_RCAR2,
>   .capabilities   = MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
> MMC_CAP_CMD23,
>   .bus_shift  = 2,
> diff --git a/include/linux/mfd/tmio.h