Re: [PATCH 01/10 v2 resend] mmc: block: add block number limitation flag for multiple block read

[Ulf Hansson <ulf.hansson@linaro.org>]

On 22 August 2014 10:36, Kuninori Morimoto
<kuninori.morimoto.gx@gmail.com> wrote:
> From: Shinobu Uehara <shinobu.uehara.xc@renesas.com>
>
> In some controllers, when performing a multiple block read of
> one or two blocks, depending on the timing with which the
> response register is read, the response value may not
> be read properly.

Can't his be solved in software in the host driver? Or is it pure bug in the HW?

> MMC_CAP2_NO_MULTI_READ flags disable all multiple block read,
> but, it is over-kill for this issue.
> This patch adds new MMC_CAP2_2BLKS_LIMIT to use single block read
> when it was two blocks.
> This is additional option of MMC_CAP2_NO_MULTI_READ
>
> [Kuninori Morimoto: tidyup for upstreaming, and cared mach-shmobile]
>
> Tested-by: Nguyen Xuan Nui <nx-nui@jinso.co.jp>
> Tested-by: Hiep Cao Minh <cm-hiep@jinso.co.jp>
> Signed-off-by: Shinobu Uehara <shinobu.uehara.xc@renesas.com>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  arch/arm/mach-shmobile/board-koelsch.c |    6 +++---
>  arch/arm/mach-shmobile/board-lager.c   |    4 ++--

I prefer to split up patches into ARM patches and MMC patches if it's possible.
That makes it easier to review and collect acks from SoC maintainers.

>  drivers/mmc/card/block.c               |   19 +++++++++++++++++--
>  drivers/mmc/host/sh_mobile_sdhi.c      |    2 +-
>  include/linux/mmc/host.h               |    3 +++
>  5 files changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm/mach-shmobile/board-koelsch.c b/arch/arm/mach-shmobile/board-koelsch.c
> index b7d5bc7..32a2b2a 100644
> --- a/arch/arm/mach-shmobile/board-koelsch.c
> +++ b/arch/arm/mach-shmobile/board-koelsch.c
> @@ -331,7 +331,7 @@ SDHI_REGULATOR(2, RCAR_GP_PIN(7, 19), RCAR_GP_PIN(2, 26));
>  static struct sh_mobile_sdhi_info sdhi0_info __initdata = {
>         .tmio_caps      = MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
>                           MMC_CAP_POWER_OFF_CARD,
> -       .tmio_caps2     = MMC_CAP2_NO_MULTI_READ,
> +       .tmio_caps2     = MMC_CAP2_NO_2BLKS_READ,

Why are MMC_CAP2_NO_MULTI_READ /MMC_CAP2_NO_2BLKS_READ configured from
platform data. If this is HW bug, aren't that related to the actual
mmc controller and version of it - not the board/platform?

This applies to more caps and boards further down below in this patch as well.

To me, it seems like these belongs into the host driver instead!?

>         .tmio_flags     = TMIO_MMC_HAS_IDLE_WAIT,
>  };
>
> @@ -344,7 +344,7 @@ static struct resource sdhi0_resources[] __initdata = {
>  static struct sh_mobile_sdhi_info sdhi1_info __initdata = {
>         .tmio_caps      = MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
>                           MMC_CAP_POWER_OFF_CARD,
> -       .tmio_caps2     = MMC_CAP2_NO_MULTI_READ,
> +       .tmio_caps2     = MMC_CAP2_NO_2BLKS_READ,
>         .tmio_flags     = TMIO_MMC_HAS_IDLE_WAIT,
>  };
>
> @@ -357,7 +357,7 @@ static struct resource sdhi1_resources[] __initdata = {
>  static struct sh_mobile_sdhi_info sdhi2_info __initdata = {
>         .tmio_caps      = MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
>                           MMC_CAP_POWER_OFF_CARD,
> -       .tmio_caps2     = MMC_CAP2_NO_MULTI_READ,
> +       .tmio_caps2     = MMC_CAP2_NO_2BLKS_READ,
>         .tmio_flags     = TMIO_MMC_HAS_IDLE_WAIT |
>                           TMIO_MMC_WRPROTECT_DISABLE,
>  };
> diff --git a/arch/arm/mach-shmobile/board-lager.c b/arch/arm/mach-shmobile/board-lager.c
> index e1d8215..953f9c9 100644
> --- a/arch/arm/mach-shmobile/board-lager.c
> +++ b/arch/arm/mach-shmobile/board-lager.c
> @@ -630,7 +630,7 @@ static void __init lager_add_rsnd_device(void)
>  static struct sh_mobile_sdhi_info sdhi0_info __initdata = {
>         .tmio_caps      = MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
>                           MMC_CAP_POWER_OFF_CARD,
> -       .tmio_caps2     = MMC_CAP2_NO_MULTI_READ,
> +       .tmio_caps2     = MMC_CAP2_NO_2BLKS_READ,
>         .tmio_flags     = TMIO_MMC_HAS_IDLE_WAIT |
>                           TMIO_MMC_WRPROTECT_DISABLE,
>  };
> @@ -644,7 +644,7 @@ static struct resource sdhi0_resources[] __initdata = {
>  static struct sh_mobile_sdhi_info sdhi2_info __initdata = {
>         .tmio_caps      = MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
>                           MMC_CAP_POWER_OFF_CARD,
> -       .tmio_caps2     = MMC_CAP2_NO_MULTI_READ,
> +       .tmio_caps2     = MMC_CAP2_NO_2BLKS_READ,
>         .tmio_flags     = TMIO_MMC_HAS_IDLE_WAIT |
>                           TMIO_MMC_WRPROTECT_DISABLE,
>  };
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index ede41f0..98c7e8c 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -1400,8 +1400,23 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
>
>                 /* Some controllers can't do multiblock reads due to hw bugs */
>                 if (card->host->caps2 & MMC_CAP2_NO_MULTI_READ &&
> -                   rq_data_dir(req) = READ)
> -                       brq->data.blocks = 1;
> +                   rq_data_dir(req) = READ) {
> +
> +                       if (card->host->caps2 & MMC_CAP2_2BLKS_LIMIT) {

This conforms to what the commit message states, that
MMC_CAP2_2BLKS_LIMIT needs to be used together with
MMC_CAP2_NO_MULTI_READ to take effect. I think I would prefer to have
them separate.

Anyway, according to your cap configuration changes, you are replacing
MMC_CAP2_NO_MULTI_READ with MMC_CAP2_2BLKS_LIMIT, instead of adding
it...

> +                               /*
> +                                * In some controllers, when performing a
> +                                * multiple block read of one or two blocks,
> +                                * depending on the timing with which the
> +                                * response register is read, the response
> +                                * value may not be read properly.
> +                                * Use single block read for this HW bug
> +                                */
> +                               if (brq->data.blocks = 2)
> +                                       brq->data.blocks = 1;
> +                       } else {
> +                               brq->data.blocks = 1;
> +                       }
> +               }
>         }
>
>         if (brq->data.blocks > 1 || do_rel_wr) {
> diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c
> index 91058da..b3baa83 100644
> --- a/drivers/mmc/host/sh_mobile_sdhi.c
> +++ b/drivers/mmc/host/sh_mobile_sdhi.c
> @@ -55,7 +55,7 @@ static const struct sh_mobile_sdhi_of_data of_rcar_gen1_compatible = {
>  static const struct sh_mobile_sdhi_of_data of_rcar_gen2_compatible = {
>         .tmio_flags     = TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_WRPROTECT_DISABLE,
>         .capabilities   = MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ,
> -       .capabilities2  = MMC_CAP2_NO_MULTI_READ,
> +       .capabilities2  = MMC_CAP2_NO_2BLKS_READ,
>  };
>
>  static const struct of_device_id sh_mobile_sdhi_of_match[] = {
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 7960424..3510fba 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -266,6 +266,9 @@ struct mmc_host {
>  #define MMC_CAP2_BOOTPART_NOACC        (1 << 0)        /* Boot partition no access */
>  #define MMC_CAP2_FULL_PWR_CYCLE        (1 << 2)        /* Can do full power cycle */
>  #define MMC_CAP2_NO_MULTI_READ (1 << 3)        /* Multiblock reads don't work */
> +#define MMC_CAP2_2BLKS_LIMIT   (1 << 4)        /* 2 blocks limit for multi read */
> +#define MMC_CAP2_NO_2BLKS_READ (MMC_CAP2_NO_MULTI_READ | \
> +                                MMC_CAP2_2BLKS_LIMIT)
>  #define MMC_CAP2_HS200_1_8V_SDR        (1 << 5)        /* can support */
>  #define MMC_CAP2_HS200_1_2V_SDR        (1 << 6)        /* can support */
>  #define MMC_CAP2_HS200         (MMC_CAP2_HS200_1_8V_SDR | \
> --
> 1.7.9.5
>

Kind regards
Uffe