diff mbox

[1/3] mmc: block: add block number limitation flag for lultiple block read

Message ID 87oayfpn14.wl%kuninori.morimoto.gx@renesas.com (mailing list archive)
State New, archived
Headers show

Commit Message

Kuninori Morimoto May 30, 2014, 10:40 a.m. UTC 
From: Kuninori Morimoto <kuninori.morimoto.gx@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.
Use single block read for this HW bug

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/mmc/card/block.c |   19 +++++++++++++++++--
 include/linux/mmc/host.h |    3 +++
 2 files changed, 20 insertions(+), 2 deletions(-)

Comments

Jaehoon Chung May 30, 2014, 11 a.m. UTC | #1 
Hi, Kuninori.

Subject has the typo. lultiple -> multiple?
I didn't know why you need this patch. Would you use the MMC_CAP2_NO_MULTI_READ?
And I think...it's not good that used the card's capability flags to fix your H/W bug.

Best Regards,
Jaehoon Chung

On 05/30/2014 07:40 PM, Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@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.
> Use single block read for this HW bug
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  drivers/mmc/card/block.c |   19 +++++++++++++++++--
>  include/linux/mmc/host.h |    3 +++
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index 452782b..f3cbe37 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) {
> +				/*
> +				 * 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/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index cb61ea4..5429cd7 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 | \
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kuninori Morimoto May 30, 2014, 11:14 a.m. UTC | #2 
Hi Jaehoon

> Subject has the typo. lultiple -> multiple?

grr..

> I didn't know why you need this patch. Would you use the MMC_CAP2_NO_MULTI_READ?
> And I think...it's not good that used the card's capability flags to fix your H/W bug.

Actually, current platform is using MMC_CAP2_NO_MULTI_READ at this point.
But, our HW bug happens when block size was 2.
Other block size is no problem.

I want to solve this issue somehow.
I'm not sure this is good method or not,
but, can I exchange block size under
mmc_host_ops :: request ?

Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Dooks May 30, 2014, 11:46 a.m. UTC | #3 
On Fri, May 30, 2014 at 08:00:17PM +0900, Jaehoon Chung wrote:
> Hi, Kuninori.
> 
> Subject has the typo. lultiple -> multiple?
> I didn't know why you need this patch. Would you use the MMC_CAP2_NO_MULTI_READ?
> And I think...it's not good that used the card's capability flags to fix your H/W bug.

If you do that, then there is around a 8-10x performance penalty.
Sergei Shtylyov May 30, 2014, 1:25 p.m. UTC | #4 
On 05/30/2014 02:40 PM, Kuninori Morimoto wrote:

> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

> In some controllers, when performing a multiple block read of

    You've typoed in the subject: "lultiple".

> 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

> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>   drivers/mmc/card/block.c |   19 +++++++++++++++++--
>   include/linux/mmc/host.h |    3 +++
>   2 files changed, 20 insertions(+), 2 deletions(-)

> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index 452782b..f3cbe37 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) {
> +				/*
> +				 * 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;

    I don't understand: previous code set 'brq->data.blocks' to 1 in any case 
without your extra flag. Why there's a need to set it specifically for 2 
blocks (and not set for a larger # of blocks)? This looks like an optimization 
of some sort, not a workaround?..

> +			} else {
> +				brq->data.blocks = 1;
> +			}
> +		}
>   	}
>
>   	if (brq->data.blocks > 1 || do_rel_wr) {

WBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kuninori Morimoto May 30, 2014, 1:40 p.m. UTC | #5 
Hi Sergei

> > +			if (card->host->caps2 & MMC_CAP2_2BLKS_LIMIT) {
> > +				/*
> > +				 * 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;
> 
>     I don't understand: previous code set 'brq->data.blocks' to 1 in any case 
> without your extra flag. Why there's a need to set it specifically for 2 
> blocks (and not set for a larger # of blocks)? This looks like an optimization 
> of some sort, not a workaround?..

Basically, we want to use multi block read.
Otherwise, the speed will be super slow
if it always use single block.
(Previous code force to single block)

But we need to switch to single block mode
if brq->data.blocks == 2.
Please read the above comment for detail.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 452782b..f3cbe37 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) {
+				/*
+				 * 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/include/linux/mmc/host.h b/include/linux/mmc/host.h
index cb61ea4..5429cd7 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 | \