[PATCH 1/1] [PATCH v4 1/1] mmc: Support of DUAL BUFFER DESC[ring] mode for dw_mmc

[PATCH 1/1] [PATCH v4 1/1] mmc: Support of DUAL BUFFER DESC[ring] mode for dw_mmc

[Shashidhar Hiremath]

This Patch adds the support for Dual Buffer Descriptor mode of
Operation for the dw_mmc driver.The patch also provides the configurability
Option for choosing DUAL_BUFFER mode or the chained modes through menuconfig.
The Menuconfig option for selecting Dual Buffer mode or chained mode
is selected only if Internal DMAC is enabled.

Signed-off-by: Shashidhar Hiremath <shashidharh@vayavyalabs.com>
---
v2:
* As per suggestions by Will Newton and James Hogan
-Config symbol Names prefixed with MMC_DW
-Added more Description for Config parameters added
-Removed unnecessary config opion IDMAC_DESC_MODE
-IDMAC_SET_BUFFER_SIZE chenged IDMAC_SET_BUFFER_SIZES
-fixed typos and indented commments correctly
-if ((i +1) <= sg_len changed to ((i +1) < sg_len
-duplication "desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC" line removed
-fixed bug in making DSL value zero
-removed ANDing the des1 with zero before writing buffer lengths to it
-Added proper multiline comments
v3:
* As per suggestions by James Hogan
-Modified Config Symbol Names in the Driver File
-Fixed Bug in Clearing the DSL field of BMOD register
-Fixed bug in IDMAC_SET_BUFFER_SIZES
v4:
* After Testing the Dual Buffer Support 
-Modified the dma_init sequence to support Dual Buffer Mode

 drivers/mmc/host/Kconfig  |   19 ++++++++++++++
 drivers/mmc/host/dw_mmc.c |   58 ++++++++++++++++++++++++++++++++++++++++----
 drivers/mmc/host/dw_mmc.h |    2 +
 3 files changed, 73 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 8c87096..dd0df83 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -534,6 +534,25 @@ config MMC_DW_IDMAC
 	  Designware Mobile Storage IP block. This disables the external DMA
 	  interface.
 
+choice
+	prompt "select  IDMAC Descriptors Mode"
+	depends on MMC_DW_IDMAC
+
+config MMC_DW_CHAIN_DESC
+	bool "Chain Descriptor Structure"
+	help
+	  Select this option to enable Chained Mode of Operation and the
+	  Chained Mode operates in a mode where only one Buffer will be used
+	  for each descriptor when the transfer is happening in DMA mode.
+
+config MMC_DW_DUAL_BUFFER_DESC
+	bool "Dual Buffer Descriptor Structure"
+	help
+	  Select this option to enable Dual Buffer Desc Mode of Operation and
+	  Dual Buffer Descriptor Mode or the Ring Mode indicates that two
+	  buffers can be used for each descriptor during DMA mode transfer.
+endchoice
+
 config MMC_SH_MMCIF
 	tristate "SuperH Internal MMCIF support"
 	depends on MMC_BLOCK && (SUPERH || ARCH_SHMOBILE)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index ff0f714..ba54bb8 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -63,6 +63,8 @@ struct idmac_desc {
 	u32		des1;	/* Buffer sizes */
 #define IDMAC_SET_BUFFER1_SIZE(d, s) \
 	((d)->des1 = ((d)->des1 & 0x03ffe000) | ((s) & 0x1fff))
+#define IDMAC_SET_BUFFER_SIZES(d, s1, s2) \
+	(d->des1 = (((s1) & 0x1fff) | (((s2) & 0x1fff) << 13)))
 
 	u32		des2;	/* buffer 1 physical address */
 
@@ -347,18 +349,45 @@ static void dw_mci_translate_sglist(struct dw_mci *host, struct mmc_data *data,
 	int i;
 	struct idmac_desc *desc = host->sg_cpu;
 
+#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
+	for (i = 0; i < sg_len; i += 2, desc++) {
+#endif
+#ifdef CONFIG_MMC_DW_CHAIN_DESC
 	for (i = 0; i < sg_len; i++, desc++) {
-		unsigned int length = sg_dma_len(&data->sg[i]);
-		u32 mem_addr = sg_dma_address(&data->sg[i]);
+#endif
+		unsigned int length1 = sg_dma_len(&data->sg[i]);
+		u32 mem_addr1 = sg_dma_address(&data->sg[i]);
+#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
+		unsigned int length2 = 0;
+		u32 mem_addr2;
+
+		if ((i+1) < sg_len) {
+			length2 = sg_dma_len(&data->sg[(i+1)]);
+			mem_addr2 = sg_dma_address(&data->sg[i+1]);
+		}
+
+		/* Set the OWN bit and disable interrupts for this descriptor
+		 * and disable the Chained Mode
+		 */
+		desc->des0 = (IDMAC_DES0_OWN | IDMAC_DES0_DIC) & (~IDMAC_DES0_CH);
+		/* Buffer length1 and length2 */
+		IDMAC_SET_BUFFER_SIZES(desc, length1, length2);
+#endif
+#ifdef CONFIG_MMC_DW_CHAIN_DESC
 
 		/* Set the OWN bit and disable interrupts for this descriptor */
 		desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC | IDMAC_DES0_CH;
 
-		/* Buffer length */
-		IDMAC_SET_BUFFER1_SIZE(desc, length);
+		/* Buffer length1 */
+		IDMAC_SET_BUFFER1_SIZE(desc, length1);
+#endif
 
 		/* Physical address to DMA to/from */
-		desc->des2 = mem_addr;
+		desc->des2 = mem_addr1;
+#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
+		if ((i+1) < sg_len)
+			desc->des3 = mem_addr2;
+#endif
 	}
 
 	/* Set first descriptor */
@@ -366,8 +395,14 @@ static void dw_mci_translate_sglist(struct dw_mci *host, struct mmc_data *data,
 	desc->des0 |= IDMAC_DES0_FD;
 
 	/* Set last descriptor */
+#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
+	desc = host->sg_cpu + ((i/2) - 1) * sizeof(struct idmac_desc);
+	desc->des0 &= (~IDMAC_DES0_DIC);
+#endif
+#ifdef CONFIG_MMC_DW_CHAIN_DESC
 	desc = host->sg_cpu + (i - 1) * sizeof(struct idmac_desc);
 	desc->des0 &= ~(IDMAC_DES0_CH | IDMAC_DES0_DIC);
+#endif
 	desc->des0 |= IDMAC_DES0_LD;
 
 	wmb();
@@ -388,6 +423,10 @@ static void dw_mci_idmac_start_dma(struct dw_mci *host, unsigned int sg_len)
 
 	/* Enable the IDMAC */
 	temp = mci_readl(host, BMOD);
+#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
+	/* The Descriptor Skip length is made zero */
+	temp &= ~(SDMMC_BMOD_DSL(0x1f));
+#endif
 	temp |= SDMMC_IDMAC_ENABLE | SDMMC_IDMAC_FB;
 	mci_writel(host, BMOD, temp);
 
@@ -402,13 +441,20 @@ static int dw_mci_idmac_init(struct dw_mci *host)
 
 	/* Number of descriptors in the ring buffer */
 	host->ring_size = PAGE_SIZE / sizeof(struct idmac_desc);
-
+#ifdef CONFIG_MMC_DW_CHAIN_DESC
 	/* Forward link the descriptor list */
 	for (i = 0, p = host->sg_cpu; i < host->ring_size - 1; i++, p++)
 		p->des3 = host->sg_dma + (sizeof(struct idmac_desc) * (i + 1));
+#endif
+#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
+	for (i = 0, p = host->sg_cpu; i < host->ring_size - 1; i++)
+		p++;
+#endif
 
 	/* Set the last descriptor as the end-of-ring descriptor */
+#ifdef CONFIG_MMC_DW_CHAIN_DESC
 	p->des3 = host->sg_dma;
+#endif
 	p->des0 = IDMAC_DES0_ER;
 
 	/* Mask out interrupts - get Tx & Rx complete only */
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index 027d377..0520dc8 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -72,6 +72,8 @@
 /* Clock Enable register defines */
 #define SDMMC_CLKEN_LOW_PWR		BIT(16)
 #define SDMMC_CLKEN_ENABLE		BIT(0)
+/* BMOD register defines */
+#define SDMMC_BMOD_DSL(n)		_SBF(2, (n))
 /* time-out register defines */
 #define SDMMC_TMOUT_DATA(n)		_SBF(8, (n))
 #define SDMMC_TMOUT_DATA_MSK		0xFFFFFF00
-- 
1.7.2.3

Re: [PATCH 1/1] [PATCH v4 1/1] mmc: Support of DUAL BUFFER DESC[ring] mode for dw_mmc

[Jaehoon Chung]

Hi Shashidhar.

I tested dual-buffer descriptor with applied your patch.
Actually, i didn't see to improve the performance. Dose it relate with
the performance? i didn't sure..

And you used #ifdef CHAIN_DESC and #ifdef DUAL_BUFFER_DESC.
I think if you use CHAIN_DESC by default, need not #ifdef CHAIN_DESC.
Because if you didn't select DUAL_BUFFER_DESC, should be work with
CHAIN_DESC. (i think good that using #ifdef..#else..#endif)

Regards,
Jaehoon Chung

On 09/27/2011 08:39 PM, Shashidhar Hiremath wrote:

> This Patch adds the support for Dual Buffer Descriptor mode of
> Operation for the dw_mmc driver.The patch also provides the configurability
> Option for choosing DUAL_BUFFER mode or the chained modes through menuconfig.
> The Menuconfig option for selecting Dual Buffer mode or chained mode
> is selected only if Internal DMAC is enabled.
> 
> Signed-off-by: Shashidhar Hiremath <shashidharh@vayavyalabs.com>
> ---
> v2:
> * As per suggestions by Will Newton and James Hogan
> -Config symbol Names prefixed with MMC_DW
> -Added more Description for Config parameters added
> -Removed unnecessary config opion IDMAC_DESC_MODE
> -IDMAC_SET_BUFFER_SIZE chenged IDMAC_SET_BUFFER_SIZES
> -fixed typos and indented commments correctly
> -if ((i +1) <= sg_len changed to ((i +1) < sg_len
> -duplication "desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC" line removed
> -fixed bug in making DSL value zero
> -removed ANDing the des1 with zero before writing buffer lengths to it
> -Added proper multiline comments
> v3:
> * As per suggestions by James Hogan
> -Modified Config Symbol Names in the Driver File
> -Fixed Bug in Clearing the DSL field of BMOD register
> -Fixed bug in IDMAC_SET_BUFFER_SIZES
> v4:
> * After Testing the Dual Buffer Support 
> -Modified the dma_init sequence to support Dual Buffer Mode
> 
>  drivers/mmc/host/Kconfig  |   19 ++++++++++++++
>  drivers/mmc/host/dw_mmc.c |   58 ++++++++++++++++++++++++++++++++++++++++----
>  drivers/mmc/host/dw_mmc.h |    2 +
>  3 files changed, 73 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 8c87096..dd0df83 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -534,6 +534,25 @@ config MMC_DW_IDMAC
>  	  Designware Mobile Storage IP block. This disables the external DMA
>  	  interface.
>  
> +choice
> +	prompt "select  IDMAC Descriptors Mode"
> +	depends on MMC_DW_IDMAC
> +
> +config MMC_DW_CHAIN_DESC
> +	bool "Chain Descriptor Structure"
> +	help
> +	  Select this option to enable Chained Mode of Operation and the
> +	  Chained Mode operates in a mode where only one Buffer will be used
> +	  for each descriptor when the transfer is happening in DMA mode.
> +
> +config MMC_DW_DUAL_BUFFER_DESC
> +	bool "Dual Buffer Descriptor Structure"
> +	help
> +	  Select this option to enable Dual Buffer Desc Mode of Operation and
> +	  Dual Buffer Descriptor Mode or the Ring Mode indicates that two
> +	  buffers can be used for each descriptor during DMA mode transfer.
> +endchoice
> +
>  config MMC_SH_MMCIF
>  	tristate "SuperH Internal MMCIF support"
>  	depends on MMC_BLOCK && (SUPERH || ARCH_SHMOBILE)
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index ff0f714..ba54bb8 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -63,6 +63,8 @@ struct idmac_desc {
>  	u32		des1;	/* Buffer sizes */
>  #define IDMAC_SET_BUFFER1_SIZE(d, s) \
>  	((d)->des1 = ((d)->des1 & 0x03ffe000) | ((s) & 0x1fff))
> +#define IDMAC_SET_BUFFER_SIZES(d, s1, s2) \
> +	(d->des1 = (((s1) & 0x1fff) | (((s2) & 0x1fff) << 13)))
>  
>  	u32		des2;	/* buffer 1 physical address */
>  
> @@ -347,18 +349,45 @@ static void dw_mci_translate_sglist(struct dw_mci *host, struct mmc_data *data,
>  	int i;
>  	struct idmac_desc *desc = host->sg_cpu;
>  
> +#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
> +	for (i = 0; i < sg_len; i += 2, desc++) {
> +#endif
> +#ifdef CONFIG_MMC_DW_CHAIN_DESC
>  	for (i = 0; i < sg_len; i++, desc++) {
> -		unsigned int length = sg_dma_len(&data->sg[i]);
> -		u32 mem_addr = sg_dma_address(&data->sg[i]);
> +#endif
> +		unsigned int length1 = sg_dma_len(&data->sg[i]);
> +		u32 mem_addr1 = sg_dma_address(&data->sg[i]);
> +#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
> +		unsigned int length2 = 0;
> +		u32 mem_addr2;
> +
> +		if ((i+1) < sg_len) {
> +			length2 = sg_dma_len(&data->sg[(i+1)]);
> +			mem_addr2 = sg_dma_address(&data->sg[i+1]);
> +		}
> +
> +		/* Set the OWN bit and disable interrupts for this descriptor
> +		 * and disable the Chained Mode
> +		 */
> +		desc->des0 = (IDMAC_DES0_OWN | IDMAC_DES0_DIC) & (~IDMAC_DES0_CH);
> +		/* Buffer length1 and length2 */
> +		IDMAC_SET_BUFFER_SIZES(desc, length1, length2);
> +#endif
> +#ifdef CONFIG_MMC_DW_CHAIN_DESC
>  
>  		/* Set the OWN bit and disable interrupts for this descriptor */
>  		desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC | IDMAC_DES0_CH;
>  
> -		/* Buffer length */
> -		IDMAC_SET_BUFFER1_SIZE(desc, length);
> +		/* Buffer length1 */
> +		IDMAC_SET_BUFFER1_SIZE(desc, length1);
> +#endif
>  
>  		/* Physical address to DMA to/from */
> -		desc->des2 = mem_addr;
> +		desc->des2 = mem_addr1;
> +#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
> +		if ((i+1) < sg_len)
> +			desc->des3 = mem_addr2;
> +#endif
>  	}
>  
>  	/* Set first descriptor */
> @@ -366,8 +395,14 @@ static void dw_mci_translate_sglist(struct dw_mci *host, struct mmc_data *data,
>  	desc->des0 |= IDMAC_DES0_FD;
>  
>  	/* Set last descriptor */
> +#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
> +	desc = host->sg_cpu + ((i/2) - 1) * sizeof(struct idmac_desc);
> +	desc->des0 &= (~IDMAC_DES0_DIC);
> +#endif
> +#ifdef CONFIG_MMC_DW_CHAIN_DESC
>  	desc = host->sg_cpu + (i - 1) * sizeof(struct idmac_desc);
>  	desc->des0 &= ~(IDMAC_DES0_CH | IDMAC_DES0_DIC);
> +#endif
>  	desc->des0 |= IDMAC_DES0_LD;
>  
>  	wmb();
> @@ -388,6 +423,10 @@ static void dw_mci_idmac_start_dma(struct dw_mci *host, unsigned int sg_len)
>  
>  	/* Enable the IDMAC */
>  	temp = mci_readl(host, BMOD);
> +#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
> +	/* The Descriptor Skip length is made zero */
> +	temp &= ~(SDMMC_BMOD_DSL(0x1f));
> +#endif
>  	temp |= SDMMC_IDMAC_ENABLE | SDMMC_IDMAC_FB;
>  	mci_writel(host, BMOD, temp);
>  
> @@ -402,13 +441,20 @@ static int dw_mci_idmac_init(struct dw_mci *host)
>  
>  	/* Number of descriptors in the ring buffer */
>  	host->ring_size = PAGE_SIZE / sizeof(struct idmac_desc);
> -
> +#ifdef CONFIG_MMC_DW_CHAIN_DESC
>  	/* Forward link the descriptor list */
>  	for (i = 0, p = host->sg_cpu; i < host->ring_size - 1; i++, p++)
>  		p->des3 = host->sg_dma + (sizeof(struct idmac_desc) * (i + 1));
> +#endif
> +#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
> +	for (i = 0, p = host->sg_cpu; i < host->ring_size - 1; i++)
> +		p++;
> +#endif
>  
>  	/* Set the last descriptor as the end-of-ring descriptor */
> +#ifdef CONFIG_MMC_DW_CHAIN_DESC
>  	p->des3 = host->sg_dma;
> +#endif
>  	p->des0 = IDMAC_DES0_ER;
>  
>  	/* Mask out interrupts - get Tx & Rx complete only */
> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
> index 027d377..0520dc8 100644
> --- a/drivers/mmc/host/dw_mmc.h
> +++ b/drivers/mmc/host/dw_mmc.h
> @@ -72,6 +72,8 @@
>  /* Clock Enable register defines */
>  #define SDMMC_CLKEN_LOW_PWR		BIT(16)
>  #define SDMMC_CLKEN_ENABLE		BIT(0)
> +/* BMOD register defines */
> +#define SDMMC_BMOD_DSL(n)		_SBF(2, (n))
>  /* time-out register defines */
>  #define SDMMC_TMOUT_DATA(n)		_SBF(8, (n))
>  #define SDMMC_TMOUT_DATA_MSK		0xFFFFFF00

Re: [PATCH 1/1] [PATCH v4 1/1] mmc: Support of DUAL BUFFER DESC[ring] mode for dw_mmc

[Shashidhar Hiremath]

Hi Jaehoon,
  First of all, thanks a lot for testing the patch.
I think you are right in saying that the patch does not increase the
performance considerably.
The reason I have added the patch is that this dual descriptor mode of
operation can be validated by this and its a Hardware feature
supported by the IP.

And, as far as #fdef scenario is concerned, the default value for
CHAIN_DESC is always yes .You can find this in the Kconfig file. So I
think it still makes Chain descriptor as the default mode.

On Wed, Oct 5, 2011 at 7:44 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> Hi Shashidhar.
>
> I tested dual-buffer descriptor with applied your patch.
> Actually, i didn't see to improve the performance. Dose it relate with
> the performance? i didn't sure..
>
> And you used #ifdef CHAIN_DESC and #ifdef DUAL_BUFFER_DESC.
> I think if you use CHAIN_DESC by default, need not #ifdef CHAIN_DESC.
> Because if you didn't select DUAL_BUFFER_DESC, should be work with
> CHAIN_DESC. (i think good that using #ifdef..#else..#endif)
>
> Regards,
> Jaehoon Chung
>
> On 09/27/2011 08:39 PM, Shashidhar Hiremath wrote:
>
>> This Patch adds the support for Dual Buffer Descriptor mode of
>> Operation for the dw_mmc driver.The patch also provides the configurability
>> Option for choosing DUAL_BUFFER mode or the chained modes through menuconfig.
>> The Menuconfig option for selecting Dual Buffer mode or chained mode
>> is selected only if Internal DMAC is enabled.
>>
>> Signed-off-by: Shashidhar Hiremath <shashidharh@vayavyalabs.com>
>> ---
>> v2:
>> * As per suggestions by Will Newton and James Hogan
>> -Config symbol Names prefixed with MMC_DW
>> -Added more Description for Config parameters added
>> -Removed unnecessary config opion IDMAC_DESC_MODE
>> -IDMAC_SET_BUFFER_SIZE chenged IDMAC_SET_BUFFER_SIZES
>> -fixed typos and indented commments correctly
>> -if ((i +1) <= sg_len changed to ((i +1) < sg_len
>> -duplication "desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC" line removed
>> -fixed bug in making DSL value zero
>> -removed ANDing the des1 with zero before writing buffer lengths to it
>> -Added proper multiline comments
>> v3:
>> * As per suggestions by James Hogan
>> -Modified Config Symbol Names in the Driver File
>> -Fixed Bug in Clearing the DSL field of BMOD register
>> -Fixed bug in IDMAC_SET_BUFFER_SIZES
>> v4:
>> * After Testing the Dual Buffer Support
>> -Modified the dma_init sequence to support Dual Buffer Mode
>>
>>  drivers/mmc/host/Kconfig  |   19 ++++++++++++++
>>  drivers/mmc/host/dw_mmc.c |   58 ++++++++++++++++++++++++++++++++++++++++----
>>  drivers/mmc/host/dw_mmc.h |    2 +
>>  3 files changed, 73 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>> index 8c87096..dd0df83 100644
>> --- a/drivers/mmc/host/Kconfig
>> +++ b/drivers/mmc/host/Kconfig
>> @@ -534,6 +534,25 @@ config MMC_DW_IDMAC
>>         Designware Mobile Storage IP block. This disables the external DMA
>>         interface.
>>
>> +choice
>> +     prompt "select  IDMAC Descriptors Mode"
>> +     depends on MMC_DW_IDMAC
>> +
>> +config MMC_DW_CHAIN_DESC
>> +     bool "Chain Descriptor Structure"
>> +     help
>> +       Select this option to enable Chained Mode of Operation and the
>> +       Chained Mode operates in a mode where only one Buffer will be used
>> +       for each descriptor when the transfer is happening in DMA mode.
>> +
>> +config MMC_DW_DUAL_BUFFER_DESC
>> +     bool "Dual Buffer Descriptor Structure"
>> +     help
>> +       Select this option to enable Dual Buffer Desc Mode of Operation and
>> +       Dual Buffer Descriptor Mode or the Ring Mode indicates that two
>> +       buffers can be used for each descriptor during DMA mode transfer.
>> +endchoice
>> +
>>  config MMC_SH_MMCIF
>>       tristate "SuperH Internal MMCIF support"
>>       depends on MMC_BLOCK && (SUPERH || ARCH_SHMOBILE)
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index ff0f714..ba54bb8 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -63,6 +63,8 @@ struct idmac_desc {
>>       u32             des1;   /* Buffer sizes */
>>  #define IDMAC_SET_BUFFER1_SIZE(d, s) \
>>       ((d)->des1 = ((d)->des1 & 0x03ffe000) | ((s) & 0x1fff))
>> +#define IDMAC_SET_BUFFER_SIZES(d, s1, s2) \
>> +     (d->des1 = (((s1) & 0x1fff) | (((s2) & 0x1fff) << 13)))
>>
>>       u32             des2;   /* buffer 1 physical address */
>>
>> @@ -347,18 +349,45 @@ static void dw_mci_translate_sglist(struct dw_mci *host, struct mmc_data *data,
>>       int i;
>>       struct idmac_desc *desc = host->sg_cpu;
>>
>> +#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
>> +     for (i = 0; i < sg_len; i += 2, desc++) {
>> +#endif
>> +#ifdef CONFIG_MMC_DW_CHAIN_DESC
>>       for (i = 0; i < sg_len; i++, desc++) {
>> -             unsigned int length = sg_dma_len(&data->sg[i]);
>> -             u32 mem_addr = sg_dma_address(&data->sg[i]);
>> +#endif
>> +             unsigned int length1 = sg_dma_len(&data->sg[i]);
>> +             u32 mem_addr1 = sg_dma_address(&data->sg[i]);
>> +#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
>> +             unsigned int length2 = 0;
>> +             u32 mem_addr2;
>> +
>> +             if ((i+1) < sg_len) {
>> +                     length2 = sg_dma_len(&data->sg[(i+1)]);
>> +                     mem_addr2 = sg_dma_address(&data->sg[i+1]);
>> +             }
>> +
>> +             /* Set the OWN bit and disable interrupts for this descriptor
>> +              * and disable the Chained Mode
>> +              */
>> +             desc->des0 = (IDMAC_DES0_OWN | IDMAC_DES0_DIC) & (~IDMAC_DES0_CH);
>> +             /* Buffer length1 and length2 */
>> +             IDMAC_SET_BUFFER_SIZES(desc, length1, length2);
>> +#endif
>> +#ifdef CONFIG_MMC_DW_CHAIN_DESC
>>
>>               /* Set the OWN bit and disable interrupts for this descriptor */
>>               desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC | IDMAC_DES0_CH;
>>
>> -             /* Buffer length */
>> -             IDMAC_SET_BUFFER1_SIZE(desc, length);
>> +             /* Buffer length1 */
>> +             IDMAC_SET_BUFFER1_SIZE(desc, length1);
>> +#endif
>>
>>               /* Physical address to DMA to/from */
>> -             desc->des2 = mem_addr;
>> +             desc->des2 = mem_addr1;
>> +#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
>> +             if ((i+1) < sg_len)
>> +                     desc->des3 = mem_addr2;
>> +#endif
>>       }
>>
>>       /* Set first descriptor */
>> @@ -366,8 +395,14 @@ static void dw_mci_translate_sglist(struct dw_mci *host, struct mmc_data *data,
>>       desc->des0 |= IDMAC_DES0_FD;
>>
>>       /* Set last descriptor */
>> +#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
>> +     desc = host->sg_cpu + ((i/2) - 1) * sizeof(struct idmac_desc);
>> +     desc->des0 &= (~IDMAC_DES0_DIC);
>> +#endif
>> +#ifdef CONFIG_MMC_DW_CHAIN_DESC
>>       desc = host->sg_cpu + (i - 1) * sizeof(struct idmac_desc);
>>       desc->des0 &= ~(IDMAC_DES0_CH | IDMAC_DES0_DIC);
>> +#endif
>>       desc->des0 |= IDMAC_DES0_LD;
>>
>>       wmb();
>> @@ -388,6 +423,10 @@ static void dw_mci_idmac_start_dma(struct dw_mci *host, unsigned int sg_len)
>>
>>       /* Enable the IDMAC */
>>       temp = mci_readl(host, BMOD);
>> +#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
>> +     /* The Descriptor Skip length is made zero */
>> +     temp &= ~(SDMMC_BMOD_DSL(0x1f));
>> +#endif
>>       temp |= SDMMC_IDMAC_ENABLE | SDMMC_IDMAC_FB;
>>       mci_writel(host, BMOD, temp);
>>
>> @@ -402,13 +441,20 @@ static int dw_mci_idmac_init(struct dw_mci *host)
>>
>>       /* Number of descriptors in the ring buffer */
>>       host->ring_size = PAGE_SIZE / sizeof(struct idmac_desc);
>> -
>> +#ifdef CONFIG_MMC_DW_CHAIN_DESC
>>       /* Forward link the descriptor list */
>>       for (i = 0, p = host->sg_cpu; i < host->ring_size - 1; i++, p++)
>>               p->des3 = host->sg_dma + (sizeof(struct idmac_desc) * (i + 1));
>> +#endif
>> +#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
>> +     for (i = 0, p = host->sg_cpu; i < host->ring_size - 1; i++)
>> +             p++;
>> +#endif
>>
>>       /* Set the last descriptor as the end-of-ring descriptor */
>> +#ifdef CONFIG_MMC_DW_CHAIN_DESC
>>       p->des3 = host->sg_dma;
>> +#endif
>>       p->des0 = IDMAC_DES0_ER;
>>
>>       /* Mask out interrupts - get Tx & Rx complete only */
>> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
>> index 027d377..0520dc8 100644
>> --- a/drivers/mmc/host/dw_mmc.h
>> +++ b/drivers/mmc/host/dw_mmc.h
>> @@ -72,6 +72,8 @@
>>  /* Clock Enable register defines */
>>  #define SDMMC_CLKEN_LOW_PWR          BIT(16)
>>  #define SDMMC_CLKEN_ENABLE           BIT(0)
>> +/* BMOD register defines */
>> +#define SDMMC_BMOD_DSL(n)            _SBF(2, (n))
>>  /* time-out register defines */
>>  #define SDMMC_TMOUT_DATA(n)          _SBF(8, (n))
>>  #define SDMMC_TMOUT_DATA_MSK         0xFFFFFF00
>
>
>



-- 
regards,
Shashidhar Hiremath

Re: [PATCH 1/1] [PATCH v4 1/1] mmc: Support of DUAL BUFFER DESC[ring] mode for dw_mmc

[Jaehoon Chung]

Hi Shashidhar.

Ok..that's dependence with Hardware feature..right? no problem..it's
working fine..

On 10/05/2011 01:54 PM, Shashidhar Hiremath wrote:

> Hi Jaehoon,
>   First of all, thanks a lot for testing the patch.
> I think you are right in saying that the patch does not increase the
> performance considerably.
> The reason I have added the patch is that this dual descriptor mode of
> operation can be validated by this and its a Hardware feature
> supported by the IP.
> 
> And, as far as #fdef scenario is concerned, the default value for
> CHAIN_DESC is always yes .You can find this in the Kconfig file. So I
> think it still makes Chain descriptor as the default mode.

My means how did you think that change the below..

for example,

#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
     for (i = 0; i < sg_len; i += 2, desc++) {
#else
     for (i = 0; i < sg_len; i++, desc++) {
#endif

Then code is more cleanable..just my opinion..

Regards,
Jaehoon Chung

> 
> On Wed, Oct 5, 2011 at 7:44 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>> Hi Shashidhar.
>>
>> I tested dual-buffer descriptor with applied your patch.
>> Actually, i didn't see to improve the performance. Dose it relate with
>> the performance? i didn't sure..
>>
>> And you used #ifdef CHAIN_DESC and #ifdef DUAL_BUFFER_DESC.
>> I think if you use CHAIN_DESC by default, need not #ifdef CHAIN_DESC.
>> Because if you didn't select DUAL_BUFFER_DESC, should be work with
>> CHAIN_DESC. (i think good that using #ifdef..#else..#endif)
>>
>> Regards,
>> Jaehoon Chung
>>
>> On 09/27/2011 08:39 PM, Shashidhar Hiremath wrote:
>>
>>> This Patch adds the support for Dual Buffer Descriptor mode of
>>> Operation for the dw_mmc driver.The patch also provides the configurability
>>> Option for choosing DUAL_BUFFER mode or the chained modes through menuconfig.
>>> The Menuconfig option for selecting Dual Buffer mode or chained mode
>>> is selected only if Internal DMAC is enabled.
>>>
>>> Signed-off-by: Shashidhar Hiremath <shashidharh@vayavyalabs.com>
>>> ---
>>> v2:
>>> * As per suggestions by Will Newton and James Hogan
>>> -Config symbol Names prefixed with MMC_DW
>>> -Added more Description for Config parameters added
>>> -Removed unnecessary config opion IDMAC_DESC_MODE
>>> -IDMAC_SET_BUFFER_SIZE chenged IDMAC_SET_BUFFER_SIZES
>>> -fixed typos and indented commments correctly
>>> -if ((i +1) <= sg_len changed to ((i +1) < sg_len
>>> -duplication "desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC" line removed
>>> -fixed bug in making DSL value zero
>>> -removed ANDing the des1 with zero before writing buffer lengths to it
>>> -Added proper multiline comments
>>> v3:
>>> * As per suggestions by James Hogan
>>> -Modified Config Symbol Names in the Driver File
>>> -Fixed Bug in Clearing the DSL field of BMOD register
>>> -Fixed bug in IDMAC_SET_BUFFER_SIZES
>>> v4:
>>> * After Testing the Dual Buffer Support
>>> -Modified the dma_init sequence to support Dual Buffer Mode
>>>
>>>  drivers/mmc/host/Kconfig  |   19 ++++++++++++++
>>>  drivers/mmc/host/dw_mmc.c |   58 ++++++++++++++++++++++++++++++++++++++++----
>>>  drivers/mmc/host/dw_mmc.h |    2 +
>>>  3 files changed, 73 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>>> index 8c87096..dd0df83 100644
>>> --- a/drivers/mmc/host/Kconfig
>>> +++ b/drivers/mmc/host/Kconfig
>>> @@ -534,6 +534,25 @@ config MMC_DW_IDMAC
>>>         Designware Mobile Storage IP block. This disables the external DMA
>>>         interface.
>>>
>>> +choice
>>> +     prompt "select  IDMAC Descriptors Mode"
>>> +     depends on MMC_DW_IDMAC
>>> +
>>> +config MMC_DW_CHAIN_DESC
>>> +     bool "Chain Descriptor Structure"
>>> +     help
>>> +       Select this option to enable Chained Mode of Operation and the
>>> +       Chained Mode operates in a mode where only one Buffer will be used
>>> +       for each descriptor when the transfer is happening in DMA mode.
>>> +
>>> +config MMC_DW_DUAL_BUFFER_DESC
>>> +     bool "Dual Buffer Descriptor Structure"
>>> +     help
>>> +       Select this option to enable Dual Buffer Desc Mode of Operation and
>>> +       Dual Buffer Descriptor Mode or the Ring Mode indicates that two
>>> +       buffers can be used for each descriptor during DMA mode transfer.
>>> +endchoice
>>> +
>>>  config MMC_SH_MMCIF
>>>       tristate "SuperH Internal MMCIF support"
>>>       depends on MMC_BLOCK && (SUPERH || ARCH_SHMOBILE)
>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>> index ff0f714..ba54bb8 100644
>>> --- a/drivers/mmc/host/dw_mmc.c
>>> +++ b/drivers/mmc/host/dw_mmc.c
>>> @@ -63,6 +63,8 @@ struct idmac_desc {
>>>       u32             des1;   /* Buffer sizes */
>>>  #define IDMAC_SET_BUFFER1_SIZE(d, s) \
>>>       ((d)->des1 = ((d)->des1 & 0x03ffe000) | ((s) & 0x1fff))
>>> +#define IDMAC_SET_BUFFER_SIZES(d, s1, s2) \
>>> +     (d->des1 = (((s1) & 0x1fff) | (((s2) & 0x1fff) << 13)))
>>>
>>>       u32             des2;   /* buffer 1 physical address */
>>>
>>> @@ -347,18 +349,45 @@ static void dw_mci_translate_sglist(struct dw_mci *host, struct mmc_data *data,
>>>       int i;
>>>       struct idmac_desc *desc = host->sg_cpu;
>>>
>>> +#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
>>> +     for (i = 0; i < sg_len; i += 2, desc++) {
>>> +#endif
>>> +#ifdef CONFIG_MMC_DW_CHAIN_DESC
>>>       for (i = 0; i < sg_len; i++, desc++) {
>>> -             unsigned int length = sg_dma_len(&data->sg[i]);
>>> -             u32 mem_addr = sg_dma_address(&data->sg[i]);
>>> +#endif
>>> +             unsigned int length1 = sg_dma_len(&data->sg[i]);
>>> +             u32 mem_addr1 = sg_dma_address(&data->sg[i]);
>>> +#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
>>> +             unsigned int length2 = 0;
>>> +             u32 mem_addr2;
>>> +
>>> +             if ((i+1) < sg_len) {
>>> +                     length2 = sg_dma_len(&data->sg[(i+1)]);
>>> +                     mem_addr2 = sg_dma_address(&data->sg[i+1]);
>>> +             }
>>> +
>>> +             /* Set the OWN bit and disable interrupts for this descriptor
>>> +              * and disable the Chained Mode
>>> +              */
>>> +             desc->des0 = (IDMAC_DES0_OWN | IDMAC_DES0_DIC) & (~IDMAC_DES0_CH);
>>> +             /* Buffer length1 and length2 */
>>> +             IDMAC_SET_BUFFER_SIZES(desc, length1, length2);
>>> +#endif
>>> +#ifdef CONFIG_MMC_DW_CHAIN_DESC
>>>
>>>               /* Set the OWN bit and disable interrupts for this descriptor */
>>>               desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC | IDMAC_DES0_CH;
>>>
>>> -             /* Buffer length */
>>> -             IDMAC_SET_BUFFER1_SIZE(desc, length);
>>> +             /* Buffer length1 */
>>> +             IDMAC_SET_BUFFER1_SIZE(desc, length1);
>>> +#endif
>>>
>>>               /* Physical address to DMA to/from */
>>> -             desc->des2 = mem_addr;
>>> +             desc->des2 = mem_addr1;
>>> +#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
>>> +             if ((i+1) < sg_len)
>>> +                     desc->des3 = mem_addr2;
>>> +#endif
>>>       }
>>>
>>>       /* Set first descriptor */
>>> @@ -366,8 +395,14 @@ static void dw_mci_translate_sglist(struct dw_mci *host, struct mmc_data *data,
>>>       desc->des0 |= IDMAC_DES0_FD;
>>>
>>>       /* Set last descriptor */
>>> +#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
>>> +     desc = host->sg_cpu + ((i/2) - 1) * sizeof(struct idmac_desc);
>>> +     desc->des0 &= (~IDMAC_DES0_DIC);
>>> +#endif
>>> +#ifdef CONFIG_MMC_DW_CHAIN_DESC
>>>       desc = host->sg_cpu + (i - 1) * sizeof(struct idmac_desc);
>>>       desc->des0 &= ~(IDMAC_DES0_CH | IDMAC_DES0_DIC);
>>> +#endif
>>>       desc->des0 |= IDMAC_DES0_LD;
>>>
>>>       wmb();
>>> @@ -388,6 +423,10 @@ static void dw_mci_idmac_start_dma(struct dw_mci *host, unsigned int sg_len)
>>>
>>>       /* Enable the IDMAC */
>>>       temp = mci_readl(host, BMOD);
>>> +#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
>>> +     /* The Descriptor Skip length is made zero */
>>> +     temp &= ~(SDMMC_BMOD_DSL(0x1f));
>>> +#endif
>>>       temp |= SDMMC_IDMAC_ENABLE | SDMMC_IDMAC_FB;
>>>       mci_writel(host, BMOD, temp);
>>>
>>> @@ -402,13 +441,20 @@ static int dw_mci_idmac_init(struct dw_mci *host)
>>>
>>>       /* Number of descriptors in the ring buffer */
>>>       host->ring_size = PAGE_SIZE / sizeof(struct idmac_desc);
>>> -
>>> +#ifdef CONFIG_MMC_DW_CHAIN_DESC
>>>       /* Forward link the descriptor list */
>>>       for (i = 0, p = host->sg_cpu; i < host->ring_size - 1; i++, p++)
>>>               p->des3 = host->sg_dma + (sizeof(struct idmac_desc) * (i + 1));
>>> +#endif
>>> +#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
>>> +     for (i = 0, p = host->sg_cpu; i < host->ring_size - 1; i++)
>>> +             p++;
>>> +#endif
>>>
>>>       /* Set the last descriptor as the end-of-ring descriptor */
>>> +#ifdef CONFIG_MMC_DW_CHAIN_DESC
>>>       p->des3 = host->sg_dma;
>>> +#endif
>>>       p->des0 = IDMAC_DES0_ER;
>>>
>>>       /* Mask out interrupts - get Tx & Rx complete only */
>>> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
>>> index 027d377..0520dc8 100644
>>> --- a/drivers/mmc/host/dw_mmc.h
>>> +++ b/drivers/mmc/host/dw_mmc.h
>>> @@ -72,6 +72,8 @@
>>>  /* Clock Enable register defines */
>>>  #define SDMMC_CLKEN_LOW_PWR          BIT(16)
>>>  #define SDMMC_CLKEN_ENABLE           BIT(0)
>>> +/* BMOD register defines */
>>> +#define SDMMC_BMOD_DSL(n)            _SBF(2, (n))
>>>  /* time-out register defines */
>>>  #define SDMMC_TMOUT_DATA(n)          _SBF(8, (n))
>>>  #define SDMMC_TMOUT_DATA_MSK         0xFFFFFF00
>>
>>
>>
> 
> 
> 

Re: [PATCH 1/1] [PATCH v4 1/1] mmc: Support of DUAL BUFFER DESC[ring] mode for dw_mmc

[Shashidhar Hiremath]

Hi Chris,
  Can this patch be accepted by criteria that its an additional
feature supported by the hardware and hence good to have the support
in the driver.Also note the patch has been tested.

On Wed, Oct 5, 2011 at 7:44 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> Hi Shashidhar.
>
> I tested dual-buffer descriptor with applied your patch.
> Actually, i didn't see to improve the performance. Dose it relate with
> the performance? i didn't sure..
>
> And you used #ifdef CHAIN_DESC and #ifdef DUAL_BUFFER_DESC.
> I think if you use CHAIN_DESC by default, need not #ifdef CHAIN_DESC.
> Because if you didn't select DUAL_BUFFER_DESC, should be work with
> CHAIN_DESC. (i think good that using #ifdef..#else..#endif)
>
> Regards,
> Jaehoon Chung
>
> On 09/27/2011 08:39 PM, Shashidhar Hiremath wrote:
>
>> This Patch adds the support for Dual Buffer Descriptor mode of
>> Operation for the dw_mmc driver.The patch also provides the configurability
>> Option for choosing DUAL_BUFFER mode or the chained modes through menuconfig.
>> The Menuconfig option for selecting Dual Buffer mode or chained mode
>> is selected only if Internal DMAC is enabled.
>>
>> Signed-off-by: Shashidhar Hiremath <shashidharh@vayavyalabs.com>
>> ---
>> v2:
>> * As per suggestions by Will Newton and James Hogan
>> -Config symbol Names prefixed with MMC_DW
>> -Added more Description for Config parameters added
>> -Removed unnecessary config opion IDMAC_DESC_MODE
>> -IDMAC_SET_BUFFER_SIZE chenged IDMAC_SET_BUFFER_SIZES
>> -fixed typos and indented commments correctly
>> -if ((i +1) <= sg_len changed to ((i +1) < sg_len
>> -duplication "desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC" line removed
>> -fixed bug in making DSL value zero
>> -removed ANDing the des1 with zero before writing buffer lengths to it
>> -Added proper multiline comments
>> v3:
>> * As per suggestions by James Hogan
>> -Modified Config Symbol Names in the Driver File
>> -Fixed Bug in Clearing the DSL field of BMOD register
>> -Fixed bug in IDMAC_SET_BUFFER_SIZES
>> v4:
>> * After Testing the Dual Buffer Support
>> -Modified the dma_init sequence to support Dual Buffer Mode
>>
>>  drivers/mmc/host/Kconfig  |   19 ++++++++++++++
>>  drivers/mmc/host/dw_mmc.c |   58 ++++++++++++++++++++++++++++++++++++++++----
>>  drivers/mmc/host/dw_mmc.h |    2 +
>>  3 files changed, 73 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>> index 8c87096..dd0df83 100644
>> --- a/drivers/mmc/host/Kconfig
>> +++ b/drivers/mmc/host/Kconfig
>> @@ -534,6 +534,25 @@ config MMC_DW_IDMAC
>>         Designware Mobile Storage IP block. This disables the external DMA
>>         interface.
>>
>> +choice
>> +     prompt "select  IDMAC Descriptors Mode"
>> +     depends on MMC_DW_IDMAC
>> +
>> +config MMC_DW_CHAIN_DESC
>> +     bool "Chain Descriptor Structure"
>> +     help
>> +       Select this option to enable Chained Mode of Operation and the
>> +       Chained Mode operates in a mode where only one Buffer will be used
>> +       for each descriptor when the transfer is happening in DMA mode.
>> +
>> +config MMC_DW_DUAL_BUFFER_DESC
>> +     bool "Dual Buffer Descriptor Structure"
>> +     help
>> +       Select this option to enable Dual Buffer Desc Mode of Operation and
>> +       Dual Buffer Descriptor Mode or the Ring Mode indicates that two
>> +       buffers can be used for each descriptor during DMA mode transfer.
>> +endchoice
>> +
>>  config MMC_SH_MMCIF
>>       tristate "SuperH Internal MMCIF support"
>>       depends on MMC_BLOCK && (SUPERH || ARCH_SHMOBILE)
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index ff0f714..ba54bb8 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -63,6 +63,8 @@ struct idmac_desc {
>>       u32             des1;   /* Buffer sizes */
>>  #define IDMAC_SET_BUFFER1_SIZE(d, s) \
>>       ((d)->des1 = ((d)->des1 & 0x03ffe000) | ((s) & 0x1fff))
>> +#define IDMAC_SET_BUFFER_SIZES(d, s1, s2) \
>> +     (d->des1 = (((s1) & 0x1fff) | (((s2) & 0x1fff) << 13)))
>>
>>       u32             des2;   /* buffer 1 physical address */
>>
>> @@ -347,18 +349,45 @@ static void dw_mci_translate_sglist(struct dw_mci *host, struct mmc_data *data,
>>       int i;
>>       struct idmac_desc *desc = host->sg_cpu;
>>
>> +#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
>> +     for (i = 0; i < sg_len; i += 2, desc++) {
>> +#endif
>> +#ifdef CONFIG_MMC_DW_CHAIN_DESC
>>       for (i = 0; i < sg_len; i++, desc++) {
>> -             unsigned int length = sg_dma_len(&data->sg[i]);
>> -             u32 mem_addr = sg_dma_address(&data->sg[i]);
>> +#endif
>> +             unsigned int length1 = sg_dma_len(&data->sg[i]);
>> +             u32 mem_addr1 = sg_dma_address(&data->sg[i]);
>> +#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
>> +             unsigned int length2 = 0;
>> +             u32 mem_addr2;
>> +
>> +             if ((i+1) < sg_len) {
>> +                     length2 = sg_dma_len(&data->sg[(i+1)]);
>> +                     mem_addr2 = sg_dma_address(&data->sg[i+1]);
>> +             }
>> +
>> +             /* Set the OWN bit and disable interrupts for this descriptor
>> +              * and disable the Chained Mode
>> +              */
>> +             desc->des0 = (IDMAC_DES0_OWN | IDMAC_DES0_DIC) & (~IDMAC_DES0_CH);
>> +             /* Buffer length1 and length2 */
>> +             IDMAC_SET_BUFFER_SIZES(desc, length1, length2);
>> +#endif
>> +#ifdef CONFIG_MMC_DW_CHAIN_DESC
>>
>>               /* Set the OWN bit and disable interrupts for this descriptor */
>>               desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC | IDMAC_DES0_CH;
>>
>> -             /* Buffer length */
>> -             IDMAC_SET_BUFFER1_SIZE(desc, length);
>> +             /* Buffer length1 */
>> +             IDMAC_SET_BUFFER1_SIZE(desc, length1);
>> +#endif
>>
>>               /* Physical address to DMA to/from */
>> -             desc->des2 = mem_addr;
>> +             desc->des2 = mem_addr1;
>> +#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
>> +             if ((i+1) < sg_len)
>> +                     desc->des3 = mem_addr2;
>> +#endif
>>       }
>>
>>       /* Set first descriptor */
>> @@ -366,8 +395,14 @@ static void dw_mci_translate_sglist(struct dw_mci *host, struct mmc_data *data,
>>       desc->des0 |= IDMAC_DES0_FD;
>>
>>       /* Set last descriptor */
>> +#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
>> +     desc = host->sg_cpu + ((i/2) - 1) * sizeof(struct idmac_desc);
>> +     desc->des0 &= (~IDMAC_DES0_DIC);
>> +#endif
>> +#ifdef CONFIG_MMC_DW_CHAIN_DESC
>>       desc = host->sg_cpu + (i - 1) * sizeof(struct idmac_desc);
>>       desc->des0 &= ~(IDMAC_DES0_CH | IDMAC_DES0_DIC);
>> +#endif
>>       desc->des0 |= IDMAC_DES0_LD;
>>
>>       wmb();
>> @@ -388,6 +423,10 @@ static void dw_mci_idmac_start_dma(struct dw_mci *host, unsigned int sg_len)
>>
>>       /* Enable the IDMAC */
>>       temp = mci_readl(host, BMOD);
>> +#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
>> +     /* The Descriptor Skip length is made zero */
>> +     temp &= ~(SDMMC_BMOD_DSL(0x1f));
>> +#endif
>>       temp |= SDMMC_IDMAC_ENABLE | SDMMC_IDMAC_FB;
>>       mci_writel(host, BMOD, temp);
>>
>> @@ -402,13 +441,20 @@ static int dw_mci_idmac_init(struct dw_mci *host)
>>
>>       /* Number of descriptors in the ring buffer */
>>       host->ring_size = PAGE_SIZE / sizeof(struct idmac_desc);
>> -
>> +#ifdef CONFIG_MMC_DW_CHAIN_DESC
>>       /* Forward link the descriptor list */
>>       for (i = 0, p = host->sg_cpu; i < host->ring_size - 1; i++, p++)
>>               p->des3 = host->sg_dma + (sizeof(struct idmac_desc) * (i + 1));
>> +#endif
>> +#ifdef CONFIG_MMC_DW_DUAL_BUFFER_DESC
>> +     for (i = 0, p = host->sg_cpu; i < host->ring_size - 1; i++)
>> +             p++;
>> +#endif
>>
>>       /* Set the last descriptor as the end-of-ring descriptor */
>> +#ifdef CONFIG_MMC_DW_CHAIN_DESC
>>       p->des3 = host->sg_dma;
>> +#endif
>>       p->des0 = IDMAC_DES0_ER;
>>
>>       /* Mask out interrupts - get Tx & Rx complete only */
>> diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
>> index 027d377..0520dc8 100644
>> --- a/drivers/mmc/host/dw_mmc.h
>> +++ b/drivers/mmc/host/dw_mmc.h
>> @@ -72,6 +72,8 @@
>>  /* Clock Enable register defines */
>>  #define SDMMC_CLKEN_LOW_PWR          BIT(16)
>>  #define SDMMC_CLKEN_ENABLE           BIT(0)
>> +/* BMOD register defines */
>> +#define SDMMC_BMOD_DSL(n)            _SBF(2, (n))
>>  /* time-out register defines */
>>  #define SDMMC_TMOUT_DATA(n)          _SBF(8, (n))
>>  #define SDMMC_TMOUT_DATA_MSK         0xFFFFFF00
>
>
>



-- 
regards,
Shashidhar Hiremath

Re: [PATCH 1/1] [PATCH v4 1/1] mmc: Support of DUAL BUFFER DESC[ring] mode for dw_mmc

[Chris Ball]

Hi,

On Thu, Nov 03 2011, Shashidhar Hiremath wrote:
> Hi Chris,
>   Can this patch be accepted by criteria that its an additional
> feature supported by the hardware and hence good to have the support
> in the driver.Also note the patch has been tested.

I think Will and James should make the call on that.

My own opinion is that it's not usually a good idea to merge code that
increases complexity for no performance gain; if the feature is actually
important, someone should find a way to finish it and measure a
performance gain (the gain can be in any of bandwidth, memory, or
lower CPU utilization) with it, to prove that the change is worthwhile.

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH 1/1] [PATCH v4 1/1] mmc: Support of DUAL BUFFER DESC[ring] mode for dw_mmc

[Will Newton]

On Thu, Nov 3, 2011 at 12:35 PM, Chris Ball <cjb@laptop.org> wrote:
> Hi,
>
> On Thu, Nov 03 2011, Shashidhar Hiremath wrote:
>> Hi Chris,
>>   Can this patch be accepted by criteria that its an additional
>> feature supported by the hardware and hence good to have the support
>> in the driver.Also note the patch has been tested.
>
> I think Will and James should make the call on that.
>
> My own opinion is that it's not usually a good idea to merge code that
> increases complexity for no performance gain; if the feature is actually
> important, someone should find a way to finish it and measure a
> performance gain (the gain can be in any of bandwidth, memory, or
> lower CPU utilization) with it, to prove that the change is worthwhile.

I'm inclined to agree. I don't want to feel like I am blocking
inclusion of anyone's hard work, but unless there is a clear advantage
for one option over the other I can't see a good reason for merging
it. At present it adds a question to the Kconfig that is pretty much
impossible for the user to answer (do I turn this feature on or off?
what is the advantage of choosing each option?).

Re: [PATCH 1/1] [PATCH v4 1/1] mmc: Support of DUAL BUFFER DESC[ring] mode for dw_mmc

[Jaehoon Chung]

On 11/04/2011 12:18 AM, Will Newton wrote:

> On Thu, Nov 3, 2011 at 12:35 PM, Chris Ball <cjb@laptop.org> wrote:
>> Hi,
>>
>> On Thu, Nov 03 2011, Shashidhar Hiremath wrote:
>>> Hi Chris,
>>>   Can this patch be accepted by criteria that its an additional
>>> feature supported by the hardware and hence good to have the support
>>> in the driver.Also note the patch has been tested.
>>
>> I think Will and James should make the call on that.
>>
>> My own opinion is that it's not usually a good idea to merge code that
>> increases complexity for no performance gain; if the feature is actually
>> important, someone should find a way to finish it and measure a
>> performance gain (the gain can be in any of bandwidth, memory, or
>> lower CPU utilization) with it, to prove that the change is worthwhile.
> 
> I'm inclined to agree. I don't want to feel like I am blocking
> inclusion of anyone's hard work, but unless there is a clear advantage
> for one option over the other I can't see a good reason for merging
> it. At present it adds a question to the Kconfig that is pretty much
> impossible for the user to answer (do I turn this feature on or off?
> what is the advantage of choosing each option?).


Maybe, i think we didn't achieve the any advantage with this patch.
But i understood that shashidhar's hardware is only supported dual buffer descriptor.
So he want to merge this patch. If that is not reason, i also think that didn't need to 
merge. I didn't see that improve the performance...memory/CPU utilization..

Regards,
Jaehoon Chung

> --
> 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
> 

Re: [PATCH 1/1] [PATCH v4 1/1] mmc: Support of DUAL BUFFER DESC[ring] mode for dw_mmc

[Shashidhar Hiremath]

Hi Will,
   I agree with you that it should not be merged  unless it improves
the performance. But I still feel that there might be some reason for
which the IP Vendor has provided an additional feature. So will this
not be a good feature to have as it will help in IP Validation for
different modes.
  As far as the Kconfig option is concerned, the user need not always
modify it since the default case will still be Chained Mode. Also Such
Kconfig options for selecting mode is already  used for stmmac
Ethernet Drivers.

On Thu, Nov 3, 2011 at 8:48 PM, Will Newton <will.newton@gmail.com> wrote:
> On Thu, Nov 3, 2011 at 12:35 PM, Chris Ball <cjb@laptop.org> wrote:
>> Hi,
>>
>> On Thu, Nov 03 2011, Shashidhar Hiremath wrote:
>>> Hi Chris,
>>>   Can this patch be accepted by criteria that its an additional
>>> feature supported by the hardware and hence good to have the support
>>> in the driver.Also note the patch has been tested.
>>
>> I think Will and James should make the call on that.
>>
>> My own opinion is that it's not usually a good idea to merge code that
>> increases complexity for no performance gain; if the feature is actually
>> important, someone should find a way to finish it and measure a
>> performance gain (the gain can be in any of bandwidth, memory, or
>> lower CPU utilization) with it, to prove that the change is worthwhile.
>
> I'm inclined to agree. I don't want to feel like I am blocking
> inclusion of anyone's hard work, but unless there is a clear advantage
> for one option over the other I can't see a good reason for merging
> it. At present it adds a question to the Kconfig that is pretty much
> impossible for the user to answer (do I turn this feature on or off?
> what is the advantage of choosing each option?).
>



-- 
regards,
Shashidhar Hiremath

Re: [PATCH 1/1] [PATCH v4 1/1] mmc: Support of DUAL BUFFER DESC[ring] mode for dw_mmc

[Shashidhar Hiremath]

Hi Will,

Just following up since i did not see any response from you on my
earlier mail.. In previous mail, im not sure whether i gave you enough
context/background.. So here it goes (apologies in advance if this
turns out to be a  lengthy mail...):

1>At Vayavya Labs, our work is to make sure that drivers for SNPS
designware IP is available in kernel.org. One of the
mandates/requirements from SNPS is that the driver submitted to
kernel.org should support all the features/configurations of the IP.
The bigger picture being that once this is done, SNPS customers dont
have to write code for any specific feature/configuration since it is
already available in the driver and they can start using it
immediately...
2>As such, it is required that code for dual descriptors be also
present in the driver submitted to kernel.org.. In future there, there
could be some person wanting to use this feature of the IP..
3>Also note that in kconfig, there does not have to be a choice
between this mode and chained mode since the default will always be
chained mode. STM Ethernet MAC driver already has such a kind of
support.. If this is not acceptable, do you feel the code should be
wrapped in some totally different preprocessor directive with comments
in the driver explaining the same?
4>With regards to your points on performance, I am open to look at it
and see where we can make improvements in the driver (if any... if its
a hardware thing, there is not much we can do)

Do let me know your views.

best regards
--Shashidhar Hiremath

On Fri, Nov 4, 2011 at 12:36 PM, Shashidhar Hiremath
<shashidharh@vayavyalabs.com> wrote:
> Hi Will,
>   I agree with you that it should not be merged  unless it improves
> the performance. But I still feel that there might be some reason for
> which the IP Vendor has provided an additional feature. So will this
> not be a good feature to have as it will help in IP Validation for
> different modes.
>  As far as the Kconfig option is concerned, the user need not always
> modify it since the default case will still be Chained Mode. Also Such
> Kconfig options for selecting mode is already  used for stmmac
> Ethernet Drivers.
>
> On Thu, Nov 3, 2011 at 8:48 PM, Will Newton <will.newton@gmail.com> wrote:
>> On Thu, Nov 3, 2011 at 12:35 PM, Chris Ball <cjb@laptop.org> wrote:
>>> Hi,
>>>
>>> On Thu, Nov 03 2011, Shashidhar Hiremath wrote:
>>>> Hi Chris,
>>>>   Can this patch be accepted by criteria that its an additional
>>>> feature supported by the hardware and hence good to have the support
>>>> in the driver.Also note the patch has been tested.
>>>
>>> I think Will and James should make the call on that.
>>>
>>> My own opinion is that it's not usually a good idea to merge code that
>>> increases complexity for no performance gain; if the feature is actually
>>> important, someone should find a way to finish it and measure a
>>> performance gain (the gain can be in any of bandwidth, memory, or
>>> lower CPU utilization) with it, to prove that the change is worthwhile.
>>
>> I'm inclined to agree. I don't want to feel like I am blocking
>> inclusion of anyone's hard work, but unless there is a clear advantage
>> for one option over the other I can't see a good reason for merging
>> it. At present it adds a question to the Kconfig that is pretty much
>> impossible for the user to answer (do I turn this feature on or off?
>> what is the advantage of choosing each option?).
>>
>
>
>
> --
> regards,
> Shashidhar Hiremath
>



-- 
regards,
Shashidhar Hiremath

Re: [PATCH 1/1] [PATCH v4 1/1] mmc: Support of DUAL BUFFER DESC[ring] mode for dw_mmc

[James Hogan]

Hi Shashidhar,

On Mon, Nov 14, 2011 at 09:32:25PM +0530, Shashidhar Hiremath wrote:
> Hi Will,
> 
> Just following up since i did not see any response from you on my
> earlier mail.. In previous mail, im not sure whether i gave you enough
> context/background.. So here it goes (apologies in advance if this
> turns out to be a  lengthy mail...):
> 
> 1>At Vayavya Labs, our work is to make sure that drivers for SNPS
> designware IP is available in kernel.org. One of the
> mandates/requirements from SNPS is that the driver submitted to
> kernel.org should support all the features/configurations of the IP.
> The bigger picture being that once this is done, SNPS customers dont
> have to write code for any specific feature/configuration since it is
> already available in the driver and they can start using it
> immediately...
> 2>As such, it is required that code for dual descriptors be also
> present in the driver submitted to kernel.org.. In future there, there
> could be some person wanting to use this feature of the IP..
> 3>Also note that in kconfig, there does not have to be a choice
> between this mode and chained mode since the default will always be
> chained mode. STM Ethernet MAC driver already has such a kind of
> support.. If this is not acceptable, do you feel the code should be
> wrapped in some totally different preprocessor directive with comments
> in the driver explaining the same?
> 4>With regards to your points on performance, I am open to look at it
> and see where we can make improvements in the driver (if any... if its
> a hardware thing, there is not much we can do)

Can I suggest you try running mmc_test with and without the dual
descriptor mode. It has a bunch of performance tests in it.

Cheers
James

> 
> Do let me know your views.
> 
> best regards
> --Shashidhar Hiremath
> 
> On Fri, Nov 4, 2011 at 12:36 PM, Shashidhar Hiremath
> <shashidharh@vayavyalabs.com> wrote:
> > Hi Will,
> >   I agree with you that it should not be merged  unless it improves
> > the performance. But I still feel that there might be some reason for
> > which the IP Vendor has provided an additional feature. So will this
> > not be a good feature to have as it will help in IP Validation for
> > different modes.
> >  As far as the Kconfig option is concerned, the user need not always
> > modify it since the default case will still be Chained Mode. Also Such
> > Kconfig options for selecting mode is already  used for stmmac
> > Ethernet Drivers.
> >
> > On Thu, Nov 3, 2011 at 8:48 PM, Will Newton <will.newton@gmail.com> wrote:
> >> On Thu, Nov 3, 2011 at 12:35 PM, Chris Ball <cjb@laptop.org> wrote:
> >>> Hi,
> >>>
> >>> On Thu, Nov 03 2011, Shashidhar Hiremath wrote:
> >>>> Hi Chris,
> >>>>   Can this patch be accepted by criteria that its an additional
> >>>> feature supported by the hardware and hence good to have the support
> >>>> in the driver.Also note the patch has been tested.
> >>>
> >>> I think Will and James should make the call on that.
> >>>
> >>> My own opinion is that it's not usually a good idea to merge code that
> >>> increases complexity for no performance gain; if the feature is actually
> >>> important, someone should find a way to finish it and measure a
> >>> performance gain (the gain can be in any of bandwidth, memory, or
> >>> lower CPU utilization) with it, to prove that the change is worthwhile.
> >>
> >> I'm inclined to agree. I don't want to feel like I am blocking
> >> inclusion of anyone's hard work, but unless there is a clear advantage
> >> for one option over the other I can't see a good reason for merging
> >> it. At present it adds a question to the Kconfig that is pretty much
> >> impossible for the user to answer (do I turn this feature on or off?
> >> what is the advantage of choosing each option?).
> >>
> >
> >
> >
> > --
> > regards,
> > Shashidhar Hiremath
> >
> 
> 
> 
> -- 
> regards,
> Shashidhar Hiremath
> --
> 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

Re: [PATCH 1/1] [PATCH v4 1/1] mmc: Support of DUAL BUFFER DESC[ring] mode for dw_mmc

[Shashidhar Hiremath]

Hi James,
  Sorry for delay in getting the performance numbers since there was
some issue with the Hardware.
Even though the Dual Buffer Patch does not improve the performance
considerably for Read operations, but for the write operations, I
found the improvement in speed of upto 200 KiB/s.
Please find the numbers for the tests with considerable improvement as below :-

Best Case Write Test:-
Chain Mode:-
Transfer of 1 x 256 sectors (1 x 128 KiB) took 0.034517837 seconds
(3797 kB/s, 3708 KiB/s, 28.97 IOPS, sg_len 32)

Dual Buffer Mode:-
Transfer of 1 x 256 sectors (1 x 128 KiB) took 0.033325325 seconds
(3933 kB/s, 3840 KiB/s, 30.00 IOPS, sg_len 32


Best Case Write Test for scattered Memory:-
Chain Mode:-
Transfer of 1 x 256 sectors (1 x 128 KiB) took 0.035052520 seconds
(3739 kB/s, 3651 KiB/s, 28.52 IOPS, sg_len 32

Dual Buffer Mode:-
Transfer of 1 x 256 sectors (1 x 128 KiB) took 0.033113106 seconds
(3958 kB/s, 3865 KiB/s, 30.19 IOPS, sg_len 32)


Single Write by Transfer Size:-
Chain Mode:-
Transfer of 1 x 256 sectors (1 x 128 KiB) took 0.034675358 seconds
(3779 kB/s, 3691 KiB/s, 28.83 IOPS, sg_len 32)

Dual Buffer Mode:-
Transfer of 1 x 256 sectors (1 x 128 KiB) took 0.033575460 seconds
(3903 kB/s, 3812 KiB/s, 29.78 IOPS, sg_len 32)

Write performance blocking req 1 to 512 sg elems:-
Chain Mode:-
Transfer of 1024 x 256 sectors (1024 x 128 KiB) took 38.029996867
seconds (3529 kB/s, 3446 KiB/s, 26.92 IOPS, sg_len 256)

Dual Buffer:-
Transfer of 1024 x 256 sectors (1024 x 128 KiB) took 37.489874739
seconds (3580 kB/s, 3496 KiB/s, 27.31 IOPS, sg_len 256)

Write performance non-blocking req 1 to 512 sg elems:-
Chain Mode:-
Transfer of 1024 x 256 sectors (1024 x 128 KiB) took 38.057999074
seconds (3526 kB/s, 3444 KiB/s, 26.90 IOPS, sg_len 256
Dual Buffer:-
Transfer of 1024 x 256 sectors (1024 x 128 KiB) took 37.489499346
seconds (3580 kB/s, 3496 KiB/s, 27.31 IOPS, sg_len 256


On Sat, Nov 19, 2011 at 7:04 AM, James Hogan <james@albanarts.com> wrote:
>
> Hi Shashidhar,
>
> On Mon, Nov 14, 2011 at 09:32:25PM +0530, Shashidhar Hiremath wrote:
> > Hi Will,
> >
> > Just following up since i did not see any response from you on my
> > earlier mail.. In previous mail, im not sure whether i gave you enough
> > context/background.. So here it goes (apologies in advance if this
> > turns out to be a  lengthy mail...):
> >
> > 1>At Vayavya Labs, our work is to make sure that drivers for SNPS
> > designware IP is available in kernel.org. One of the
> > mandates/requirements from SNPS is that the driver submitted to
> > kernel.org should support all the features/configurations of the IP.
> > The bigger picture being that once this is done, SNPS customers dont
> > have to write code for any specific feature/configuration since it is
> > already available in the driver and they can start using it
> > immediately...
> > 2>As such, it is required that code for dual descriptors be also
> > present in the driver submitted to kernel.org.. In future there, there
> > could be some person wanting to use this feature of the IP..
> > 3>Also note that in kconfig, there does not have to be a choice
> > between this mode and chained mode since the default will always be
> > chained mode. STM Ethernet MAC driver already has such a kind of
> > support.. If this is not acceptable, do you feel the code should be
> > wrapped in some totally different preprocessor directive with comments
> > in the driver explaining the same?
> > 4>With regards to your points on performance, I am open to look at it
> > and see where we can make improvements in the driver (if any... if its
> > a hardware thing, there is not much we can do)
>
> Can I suggest you try running mmc_test with and without the dual
> descriptor mode. It has a bunch of performance tests in it.
>
> Cheers
> James
>
> >
> > Do let me know your views.
> >
> > best regards
> > --Shashidhar Hiremath
> >
> > On Fri, Nov 4, 2011 at 12:36 PM, Shashidhar Hiremath
> > <shashidharh@vayavyalabs.com> wrote:
> > > Hi Will,
> > >   I agree with you that it should not be merged  unless it improves
> > > the performance. But I still feel that there might be some reason for
> > > which the IP Vendor has provided an additional feature. So will this
> > > not be a good feature to have as it will help in IP Validation for
> > > different modes.
> > >  As far as the Kconfig option is concerned, the user need not always
> > > modify it since the default case will still be Chained Mode. Also Such
> > > Kconfig options for selecting mode is already  used for stmmac
> > > Ethernet Drivers.
> > >
> > > On Thu, Nov 3, 2011 at 8:48 PM, Will Newton <will.newton@gmail.com> wrote:
> > >> On Thu, Nov 3, 2011 at 12:35 PM, Chris Ball <cjb@laptop.org> wrote:
> > >>> Hi,
> > >>>
> > >>> On Thu, Nov 03 2011, Shashidhar Hiremath wrote:
> > >>>> Hi Chris,
> > >>>>   Can this patch be accepted by criteria that its an additional
> > >>>> feature supported by the hardware and hence good to have the support
> > >>>> in the driver.Also note the patch has been tested.
> > >>>
> > >>> I think Will and James should make the call on that.
> > >>>
> > >>> My own opinion is that it's not usually a good idea to merge code that
> > >>> increases complexity for no performance gain; if the feature is actually
> > >>> important, someone should find a way to finish it and measure a
> > >>> performance gain (the gain can be in any of bandwidth, memory, or
> > >>> lower CPU utilization) with it, to prove that the change is worthwhile.
> > >>
> > >> I'm inclined to agree. I don't want to feel like I am blocking
> > >> inclusion of anyone's hard work, but unless there is a clear advantage
> > >> for one option over the other I can't see a good reason for merging
> > >> it. At present it adds a question to the Kconfig that is pretty much
> > >> impossible for the user to answer (do I turn this feature on or off?
> > >> what is the advantage of choosing each option?).
> > >>
> > >
> > >
> > >
> > > --
> > > regards,
> > > Shashidhar Hiremath
> > >
> >
> >
> >
> > --
> > regards,
> > Shashidhar Hiremath
> > --
> > 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




--
regards,
Shashidhar Hiremath

Re: [PATCH 1/1] [PATCH v4 1/1] mmc: Support of DUAL BUFFER DESC[ring] mode for dw_mmc

[Shashidhar Hiremath]

Hi ,
  Any update on this patch ?

On Wed, Jan 11, 2012 at 4:57 PM, Shashidhar Hiremath
<shashidharh@vayavyalabs.com> wrote:
> Hi James,
>   Sorry for delay in getting the performance numbers since there was
> some issue with the Hardware.
> Even though the Dual Buffer Patch does not improve the performance
> considerably for Read operations, but for the write operations, I
> found the improvement in speed of upto 200 KiB/s.
> Please find the numbers for the tests with considerable improvement as below :-
>
> Best Case Write Test:-
> Chain Mode:-
> Transfer of 1 x 256 sectors (1 x 128 KiB) took 0.034517837 seconds
> (3797 kB/s, 3708 KiB/s, 28.97 IOPS, sg_len 32)
>
> Dual Buffer Mode:-
> Transfer of 1 x 256 sectors (1 x 128 KiB) took 0.033325325 seconds
> (3933 kB/s, 3840 KiB/s, 30.00 IOPS, sg_len 32
>
>
> Best Case Write Test for scattered Memory:-
> Chain Mode:-
> Transfer of 1 x 256 sectors (1 x 128 KiB) took 0.035052520 seconds
> (3739 kB/s, 3651 KiB/s, 28.52 IOPS, sg_len 32
>
> Dual Buffer Mode:-
> Transfer of 1 x 256 sectors (1 x 128 KiB) took 0.033113106 seconds
> (3958 kB/s, 3865 KiB/s, 30.19 IOPS, sg_len 32)
>
>
> Single Write by Transfer Size:-
> Chain Mode:-
> Transfer of 1 x 256 sectors (1 x 128 KiB) took 0.034675358 seconds
> (3779 kB/s, 3691 KiB/s, 28.83 IOPS, sg_len 32)
>
> Dual Buffer Mode:-
> Transfer of 1 x 256 sectors (1 x 128 KiB) took 0.033575460 seconds
> (3903 kB/s, 3812 KiB/s, 29.78 IOPS, sg_len 32)
>
> Write performance blocking req 1 to 512 sg elems:-
> Chain Mode:-
> Transfer of 1024 x 256 sectors (1024 x 128 KiB) took 38.029996867
> seconds (3529 kB/s, 3446 KiB/s, 26.92 IOPS, sg_len 256)
>
> Dual Buffer:-
> Transfer of 1024 x 256 sectors (1024 x 128 KiB) took 37.489874739
> seconds (3580 kB/s, 3496 KiB/s, 27.31 IOPS, sg_len 256)
>
> Write performance non-blocking req 1 to 512 sg elems:-
> Chain Mode:-
> Transfer of 1024 x 256 sectors (1024 x 128 KiB) took 38.057999074
> seconds (3526 kB/s, 3444 KiB/s, 26.90 IOPS, sg_len 256
> Dual Buffer:-
> Transfer of 1024 x 256 sectors (1024 x 128 KiB) took 37.489499346
> seconds (3580 kB/s, 3496 KiB/s, 27.31 IOPS, sg_len 256
>
>
> On Sat, Nov 19, 2011 at 7:04 AM, James Hogan <james@albanarts.com> wrote:
>>
>> Hi Shashidhar,
>>
>> On Mon, Nov 14, 2011 at 09:32:25PM +0530, Shashidhar Hiremath wrote:
>> > Hi Will,
>> >
>> > Just following up since i did not see any response from you on my
>> > earlier mail.. In previous mail, im not sure whether i gave you enough
>> > context/background.. So here it goes (apologies in advance if this
>> > turns out to be a  lengthy mail...):
>> >
>> > 1>At Vayavya Labs, our work is to make sure that drivers for SNPS
>> > designware IP is available in kernel.org. One of the
>> > mandates/requirements from SNPS is that the driver submitted to
>> > kernel.org should support all the features/configurations of the IP.
>> > The bigger picture being that once this is done, SNPS customers dont
>> > have to write code for any specific feature/configuration since it is
>> > already available in the driver and they can start using it
>> > immediately...
>> > 2>As such, it is required that code for dual descriptors be also
>> > present in the driver submitted to kernel.org.. In future there, there
>> > could be some person wanting to use this feature of the IP..
>> > 3>Also note that in kconfig, there does not have to be a choice
>> > between this mode and chained mode since the default will always be
>> > chained mode. STM Ethernet MAC driver already has such a kind of
>> > support.. If this is not acceptable, do you feel the code should be
>> > wrapped in some totally different preprocessor directive with comments
>> > in the driver explaining the same?
>> > 4>With regards to your points on performance, I am open to look at it
>> > and see where we can make improvements in the driver (if any... if its
>> > a hardware thing, there is not much we can do)
>>
>> Can I suggest you try running mmc_test with and without the dual
>> descriptor mode. It has a bunch of performance tests in it.
>>
>> Cheers
>> James
>>
>> >
>> > Do let me know your views.
>> >
>> > best regards
>> > --Shashidhar Hiremath
>> >
>> > On Fri, Nov 4, 2011 at 12:36 PM, Shashidhar Hiremath
>> > <shashidharh@vayavyalabs.com> wrote:
>> > > Hi Will,
>> > >   I agree with you that it should not be merged  unless it improves
>> > > the performance. But I still feel that there might be some reason for
>> > > which the IP Vendor has provided an additional feature. So will this
>> > > not be a good feature to have as it will help in IP Validation for
>> > > different modes.
>> > >  As far as the Kconfig option is concerned, the user need not always
>> > > modify it since the default case will still be Chained Mode. Also Such
>> > > Kconfig options for selecting mode is already  used for stmmac
>> > > Ethernet Drivers.
>> > >
>> > > On Thu, Nov 3, 2011 at 8:48 PM, Will Newton <will.newton@gmail.com> wrote:
>> > >> On Thu, Nov 3, 2011 at 12:35 PM, Chris Ball <cjb@laptop.org> wrote:
>> > >>> Hi,
>> > >>>
>> > >>> On Thu, Nov 03 2011, Shashidhar Hiremath wrote:
>> > >>>> Hi Chris,
>> > >>>>   Can this patch be accepted by criteria that its an additional
>> > >>>> feature supported by the hardware and hence good to have the support
>> > >>>> in the driver.Also note the patch has been tested.
>> > >>>
>> > >>> I think Will and James should make the call on that.
>> > >>>
>> > >>> My own opinion is that it's not usually a good idea to merge code that
>> > >>> increases complexity for no performance gain; if the feature is actually
>> > >>> important, someone should find a way to finish it and measure a
>> > >>> performance gain (the gain can be in any of bandwidth, memory, or
>> > >>> lower CPU utilization) with it, to prove that the change is worthwhile.
>> > >>
>> > >> I'm inclined to agree. I don't want to feel like I am blocking
>> > >> inclusion of anyone's hard work, but unless there is a clear advantage
>> > >> for one option over the other I can't see a good reason for merging
>> > >> it. At present it adds a question to the Kconfig that is pretty much
>> > >> impossible for the user to answer (do I turn this feature on or off?
>> > >> what is the advantage of choosing each option?).
>> > >>
>> > >
>> > >
>> > >
>> > > --
>> > > regards,
>> > > Shashidhar Hiremath
>> > >
>> >
>> >
>> >
>> > --
>> > regards,
>> > Shashidhar Hiremath
>> > --
>> > 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
>
>
>
>
> --
> regards,
> Shashidhar Hiremath



-- 
regards,
Shashidhar Hiremath

Re: [PATCH 1/1] [PATCH v4 1/1] mmc: Support of DUAL BUFFER DESC[ring] mode for dw_mmc

[Will Newton]

On Tue, Jan 17, 2012 at 1:46 PM, Shashidhar Hiremath
<shashidharh@vayavyalabs.com> wrote:
> Hi ,
>  Any update on this patch ?

I'm happy to see it merged based on those numbers. It might be worth
adding the numbers to the commit message for future reference though?

Acked-by: Will Newton <will.newton@imgtec.com>

Re: [PATCH 1/1] [PATCH v4 1/1] mmc: Support of DUAL BUFFER DESC[ring] mode for dw_mmc

[Shashidhar Hiremath]

thanks James.
  Will add the Numbers to Commit Message

On Thu, Jan 19, 2012 at 3:59 PM, Will Newton <will.newton@gmail.com> wrote:
> On Tue, Jan 17, 2012 at 1:46 PM, Shashidhar Hiremath
> <shashidharh@vayavyalabs.com> wrote:
>> Hi ,
>>  Any update on this patch ?
>
> I'm happy to see it merged based on those numbers. It might be worth
> adding the numbers to the commit message for future reference though?
>
> Acked-by: Will Newton <will.newton@imgtec.com>



-- 
regards,
Shashidhar Hiremath

Re: [PATCH 1/1] [PATCH v4 1/1] mmc: Support of DUAL BUFFER DESC[ring] mode for dw_mmc

[Shashidhar Hiremath]

Hi Chris,
  Any updates on when this patch can be pushed ?

On Thu, Jan 19, 2012 at 3:59 PM, Will Newton <will.newton@gmail.com> wrote:
> On Tue, Jan 17, 2012 at 1:46 PM, Shashidhar Hiremath
> <shashidharh@vayavyalabs.com> wrote:
>> Hi ,
>>  Any update on this patch ?
>
> I'm happy to see it merged based on those numbers. It might be worth
> adding the numbers to the commit message for future reference though?
>
> Acked-by: Will Newton <will.newton@imgtec.com>



-- 
regards,
Shashidhar Hiremath