On 22 August 2014 10:36, Kuninori Morimoto
<kuninori.morimoto.gx@xxxxxxxxx> wrote:
> From: Shinobu Uehara <shinobu.uehara.xc@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> Tested-by: Hiep Cao Minh <cm-hiep@xxxxxxxxxxx>
> Signed-off-by: Shinobu Uehara <shinobu.uehara.xc@xxxxxxxxxxx>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
