[RFC PATCH 1/1] dw_mmc: add quirk about using only one slot

[RFC PATCH 1/1] dw_mmc: add quirk about using only one slot

[Jaehoon Chung]

If assume only using one slot, i think that dw_mci_queue_request() need not.

Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com
---
 drivers/mmc/host/dw_mmc.c  |    9 +++++++--
 include/linux/mmc/dw_mmc.h |    3 ++-
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 882d004..e3f26ea 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -673,8 +673,13 @@ static void dw_mci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 		return;
 	}
 
-	/* We don't support multiple blocks of weird lengths. */
-	dw_mci_queue_request(host, slot, mrq);
+	if (host->quirks & DW_MCI_QUIRK_FORCE_ONE_SLOT) {
+		slot->mrq = mrq;
+		host->state = STATE_SENDING_CMD;
+		dw_mci_start_request(host, slot);
+	} else
+		/* We don't support multiple blocks of weird lengths. */
+		dw_mci_queue_request(host, slot, mrq);
 }
 
 static void dw_mci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
index c0207a7..82f59c2 100644
--- a/include/linux/mmc/dw_mmc.h
+++ b/include/linux/mmc/dw_mmc.h
@@ -176,7 +176,8 @@ struct dw_mci_dma_ops {
 #define DW_MCI_QUIRK_HIGHSPEED			BIT(2)
 /* Unreliable card detection */
 #define DW_MCI_QUIRK_BROKEN_CARD_DETECTION	BIT(3)
-
+/* Force using only one slot */
+#define DW_MCI_QUIRK_FORCE_ONE_SLOT		BIT(4)
 
 struct dma_pdata;
 

Re: [RFC PATCH 1/1] dw_mmc: add quirk about using only one slot

[Will Newton]

On Fri, Mar 25, 2011 at 6:00 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> If assume only using one slot, i think that dw_mci_queue_request() need not.
>
> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com
> ---
>  drivers/mmc/host/dw_mmc.c  |    9 +++++++--
>  include/linux/mmc/dw_mmc.h |    3 ++-
>  2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 882d004..e3f26ea 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -673,8 +673,13 @@ static void dw_mci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>                return;
>        }
>
> -       /* We don't support multiple blocks of weird lengths. */
> -       dw_mci_queue_request(host, slot, mrq);
> +       if (host->quirks & DW_MCI_QUIRK_FORCE_ONE_SLOT) {

Do we really need a quirk for this? Why not use host->num_slots?

> +               slot->mrq = mrq;
> +               host->state = STATE_SENDING_CMD;

I don't think it is safe to manipulate these structures without taking
the host->lock. If we are to do this then I think I would like to know
why (e.g. do we have performance numbers to support this change) and
some analysis of what is protected by host->lock and which functions
need the lock to be held. For example I do not think it is safe to
call dw_mci_start_request without taking the lock.

> +               dw_mci_start_request(host, slot);
> +       } else
> +               /* We don't support multiple blocks of weird lengths. */

This comment is obsolete I think and can be removed.

Re: [RFC PATCH 1/1] dw_mmc: add quirk about using only one slot

[Jaehoon Chung]

Hi Will

Will Newton wrote:
> On Fri, Mar 25, 2011 at 6:00 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>> If assume only using one slot, i think that dw_mci_queue_request() need not.
>>
>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com
>> ---
>>  drivers/mmc/host/dw_mmc.c  |    9 +++++++--
>>  include/linux/mmc/dw_mmc.h |    3 ++-
>>  2 files changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 882d004..e3f26ea 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -673,8 +673,13 @@ static void dw_mci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>>                return;
>>        }
>>
>> -       /* We don't support multiple blocks of weird lengths. */
>> -       dw_mci_queue_request(host, slot, mrq);
>> +       if (host->quirks & DW_MCI_QUIRK_FORCE_ONE_SLOT) {
> 
> Do we really need a quirk for this? Why not use host->num_slots

Yes, you'r right. if using this patch, can use host->num_slots.

> 
>> +               slot->mrq = mrq;
>> +               host->state = STATE_SENDING_CMD;
> 
> I don't think it is safe to manipulate these structures without taking
> the host->lock. If we are to do this then I think I would like to know
> why (e.g. do we have performance numbers to support this change) and
> some analysis of what is protected by host->lock and which functions
> need the lock to be held. For example I do not think it is safe to
> call dw_mci_start_request without taking the lock.
> 

I know this patch didn't increase the performance.
BUt If we want to know some analysis..I should analysis what protected by host->lock.
Did you analysis what protect by host->lock?

>> +               dw_mci_start_request(host, slot);
>> +       } else
>> +               /* We don't support multiple blocks of weird lengths. */
> 
> This comment is obsolete I think and can be removed.

Anyway, Thanks for your comment.
 

Regards,
Jaehoon Chung