* [PATCH 1/1] mmc: Support of DUAL BUFFER DESC[ring] mode for dw_mmc
@ 2011-08-26 10:31 Shashidhar Hiremath
2011-08-26 11:06 ` Will Newton
2011-08-26 13:39 ` James Hogan
0 siblings, 2 replies; 7+ messages in thread
From: Shashidhar Hiremath @ 2011-08-26 10:31 UTC (permalink / raw)
To: Chris Ball, Shawn Guo, Wolfram Sang, Philip Rakity, Zhangfei Gao, Wil
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>
---
drivers/mmc/host/Kconfig | 22 ++++++++++++++++++
drivers/mmc/host/dw_mmc.c | 55 ++++++++++++++++++++++++++++++++++++--------
2 files changed, 67 insertions(+), 10 deletions(-)
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 8c87096..e10f585 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -534,6 +534,28 @@ config MMC_DW_IDMAC
Designware Mobile Storage IP block. This disables the external DMA
interface.
+config IDMAC_DESC_MODE
+ bool "Internal DMAC Descriptor Operating Mode"
+ depends on MMC_DW_IDMAC
+ help
+ This selects the Operating mode for the Descriptors.
+
+choice
+ prompt "select configuration"
+ depends on IDMAC_DESC_MODE
+
+config CHAIN_DESC
+ bool "Chain Descriptor Structure"
+ help
+ Select this option to enable Chained Mode of Operation.
+
+config DUAL_BUFFER_DESC
+ bool "Dual Buffer Descriptor Structure"
+ help
+ Select this option to enable Dual Buffer Desc Mode of Operation.
+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..a590856 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -63,6 +63,9 @@ 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_SIZE(d, s1, s2) \
+ ((d)->des1 = ((d)->des1 & 0x0) | \
+ (((s1) & 0x1fff) | ((s2) & 0x1fff << 13)))
u32 des2; /* buffer 1 physical address */
@@ -348,17 +351,44 @@ static void dw_mci_translate_sglist(struct
dw_mci *host, struct mmc_data *data,
struct idmac_desc *desc = host->sg_cpu;
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]);
-
- /* Set the OWN bit and disable interrupts for this descriptor */
+ /*length and mem_aadress of first buffer*/
+ unsigned int length1 = sg_dma_len(&data->sg[i]);
+ u32 mem_addr1 = sg_dma_address(&data->sg[i]);
+#ifdef CONFIG_DUAL_BUFFER_DESC
+ unsigned int length2;
+ 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
+ */
+ desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC
+ desc->des0 &= (~IDMAC_DES0_CH);
+ /* Buffer length being set for Buffer1
+ * and Buffer2 being set
+ */
+ IDMAC_SET_BUFFER_SIZE(desc, length1, length2);
+ desc->des3 = mem_addr2;
+ /* Incrementing for the second buffer */
+ i++;
+ } else {
+ desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC
+ desc->des0 &= (~IDMAC_DES0_CH);
+ /* Buffer length being set for Buffer1
+ * and Buffer2 being set
+ */
+ IDMAC_SET_BUFFER_SIZE(desc, length1, 0);
+ }
+#elif CONFIG_CHAIN_DESC
+ /* Set 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 length for Buffer1*/
+ IDMAC_SET_BUFFER1_SIZE(desc, length1);
+#endif
/* Physical address to DMA to/from */
- desc->des2 = mem_addr;
+ desc->des2 = mem_addr1;
}
/* Set first descriptor */
@@ -369,6 +399,10 @@ static void dw_mci_translate_sglist(struct dw_mci
*host, struct mmc_data *data,
desc = host->sg_cpu + (i - 1) * sizeof(struct idmac_desc);
desc->des0 &= ~(IDMAC_DES0_CH | IDMAC_DES0_DIC);
desc->des0 |= IDMAC_DES0_LD;
+#ifdef CONFIG_DUAL_BUFFER_DESC
+ /*Set the End of Ring bit */
+ desc->des0 |= IDMAC_DES0_ER;
+#endif
wmb();
}
@@ -388,7 +422,8 @@ static void dw_mci_idmac_start_dma(struct dw_mci
*host, unsigned int sg_len)
/* Enable the IDMAC */
temp = mci_readl(host, BMOD);
- temp |= SDMMC_IDMAC_ENABLE | SDMMC_IDMAC_FB;
+ /* The Descriptor Skip length is made zero */
+ temp |= SDMMC_IDMAC_ENABLE | SDMMC_IDMAC_FB & SDMMC_BMOD_DSL(0);
mci_writel(host, BMOD, temp);
/* Start it running */
--
1.7.2.3
--
regards,
Shashidhar Hiremath
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] mmc: Support of DUAL BUFFER DESC[ring] mode for dw_mmc
2011-08-26 10:31 [PATCH 1/1] mmc: Support of DUAL BUFFER DESC[ring] mode for dw_mmc Shashidhar Hiremath
@ 2011-08-26 11:06 ` Will Newton
2011-08-26 11:20 ` Shashidhar Hiremath
2011-08-26 13:39 ` James Hogan
1 sibling, 1 reply; 7+ messages in thread
From: Will Newton @ 2011-08-26 11:06 UTC (permalink / raw)
To: Shashidhar Hiremath
Cc: Chris Ball, Shawn Guo, Wolfram Sang, Philip Rakity, Zhangfei Gao,
James Hogan, Jaehoon Chung, Kyungmin Park, linux-mmc
On Fri, Aug 26, 2011 at 11:31 AM, Shashidhar Hiremath
<shashidharh@vayavyalabs.com> wrote:
Hi Shashidhar,
Thanks for the patch! A few comments below:
> 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>
> ---
> drivers/mmc/host/Kconfig | 22 ++++++++++++++++++
> drivers/mmc/host/dw_mmc.c | 55 ++++++++++++++++++++++++++++++++++++--------
> 2 files changed, 67 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 8c87096..e10f585 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -534,6 +534,28 @@ config MMC_DW_IDMAC
> Designware Mobile Storage IP block. This disables the external DMA
> interface.
>
> +config IDMAC_DESC_MODE
> + bool "Internal DMAC Descriptor Operating Mode"
> + depends on MMC_DW_IDMAC
> + help
> + This selects the Operating mode for the Descriptors.
> +
> +choice
> + prompt "select configuration"
> + depends on IDMAC_DESC_MODE
> +
> +config CHAIN_DESC
> + bool "Chain Descriptor Structure"
> + help
> + Select this option to enable Chained Mode of Operation.
> +
> +config DUAL_BUFFER_DESC
> + bool "Dual Buffer Descriptor Structure"
> + help
> + Select this option to enable Dual Buffer Desc Mode of Operation.
> +endchoice
These config symbols go in the global config namespace so you should
probably prefix them with "MMC_DW" to prevent pollution of the
namespace. Also it would be good to add some more description text so
the user can understand which choice to make.
> +
> +
> 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..a590856 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -63,6 +63,9 @@ 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_SIZE(d, s1, s2) \
> + ((d)->des1 = ((d)->des1 & 0x0) | \
> + (((s1) & 0x1fff) | ((s2) & 0x1fff << 13)))
IDMAC_SET_BUFFER_SIZES might be a more descriptive name for this macro.
> u32 des2; /* buffer 1 physical address */
>
> @@ -348,17 +351,44 @@ static void dw_mci_translate_sglist(struct
> dw_mci *host, struct mmc_data *data,
> struct idmac_desc *desc = host->sg_cpu;
>
> 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]);
> -
> - /* Set the OWN bit and disable interrupts for this descriptor */
> + /*length and mem_aadress of first buffer*/
There's typo in this comment.
> + unsigned int length1 = sg_dma_len(&data->sg[i]);
> + u32 mem_addr1 = sg_dma_address(&data->sg[i]);
> +#ifdef CONFIG_DUAL_BUFFER_DESC
> + unsigned int length2;
> + u32 mem_addr2;
> + if ((i+1) <= sg_len) {
Won't this always be true as i < sg_len? Should it be 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
> + */
> + desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC
> + desc->des0 &= (~IDMAC_DES0_CH);
These two lines are identical in both branches of the if statement so
could be moved above it.
> + /* Buffer length being set for Buffer1
> + * and Buffer2 being set
> + */
> + IDMAC_SET_BUFFER_SIZE(desc, length1, length2);
> + desc->des3 = mem_addr2;
> + /* Incrementing for the second buffer */
> + i++;
> + } else {
> + desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC
> + desc->des0 &= (~IDMAC_DES0_CH);
> + /* Buffer length being set for Buffer1
> + * and Buffer2 being set
> + */
> + IDMAC_SET_BUFFER_SIZE(desc, length1, 0);
> + }
> +#elif CONFIG_CHAIN_DESC
> + /* Set 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 length for Buffer1*/
> + IDMAC_SET_BUFFER1_SIZE(desc, length1);
> +#endif
> /* Physical address to DMA to/from */
> - desc->des2 = mem_addr;
> + desc->des2 = mem_addr1;
> }
>
> /* Set first descriptor */
> @@ -369,6 +399,10 @@ static void dw_mci_translate_sglist(struct dw_mci
> *host, struct mmc_data *data,
> desc = host->sg_cpu + (i - 1) * sizeof(struct idmac_desc);
> desc->des0 &= ~(IDMAC_DES0_CH | IDMAC_DES0_DIC);
> desc->des0 |= IDMAC_DES0_LD;
> +#ifdef CONFIG_DUAL_BUFFER_DESC
> + /*Set the End of Ring bit */
> + desc->des0 |= IDMAC_DES0_ER;
> +#endif
>
> wmb();
> }
> @@ -388,7 +422,8 @@ static void dw_mci_idmac_start_dma(struct dw_mci
> *host, unsigned int sg_len)
>
> /* Enable the IDMAC */
> temp = mci_readl(host, BMOD);
> - temp |= SDMMC_IDMAC_ENABLE | SDMMC_IDMAC_FB;
> + /* The Descriptor Skip length is made zero */
> + temp |= SDMMC_IDMAC_ENABLE | SDMMC_IDMAC_FB & SDMMC_BMOD_DSL(0);
I'm not sure what this is trying to do exactly, but it looks like it
won't work. Setting DSL 0 just means having 0 in bits 6:2 of BMOD,
which would be "& ~SDMMC_BMOD_DSL(0)" I would have thought?
> mci_writel(host, BMOD, temp);
>
> /* Start it running */
> --
> 1.7.2.3
>
>
> --
> 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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] mmc: Support of DUAL BUFFER DESC[ring] mode for dw_mmc
2011-08-26 11:06 ` Will Newton
@ 2011-08-26 11:20 ` Shashidhar Hiremath
0 siblings, 0 replies; 7+ messages in thread
From: Shashidhar Hiremath @ 2011-08-26 11:20 UTC (permalink / raw)
To: Will Newton
Cc: Chris Ball, Shawn Guo, Wolfram Sang, Philip Rakity, Zhangfei Gao,
James Hogan, Jaehoon Chung, Kyungmin Park, linux-mmc
thanks Will,
I will make the changes soon.
On Fri, Aug 26, 2011 at 4:36 PM, Will Newton <will.newton@gmail.com> wrote:
> On Fri, Aug 26, 2011 at 11:31 AM, Shashidhar Hiremath
> <shashidharh@vayavyalabs.com> wrote:
>
> Hi Shashidhar,
>
> Thanks for the patch! A few comments below:
>
>> 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>
>> ---
>> drivers/mmc/host/Kconfig | 22 ++++++++++++++++++
>> drivers/mmc/host/dw_mmc.c | 55 ++++++++++++++++++++++++++++++++++++--------
>> 2 files changed, 67 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>> index 8c87096..e10f585 100644
>> --- a/drivers/mmc/host/Kconfig
>> +++ b/drivers/mmc/host/Kconfig
>> @@ -534,6 +534,28 @@ config MMC_DW_IDMAC
>> Designware Mobile Storage IP block. This disables the external DMA
>> interface.
>>
>> +config IDMAC_DESC_MODE
>> + bool "Internal DMAC Descriptor Operating Mode"
>> + depends on MMC_DW_IDMAC
>> + help
>> + This selects the Operating mode for the Descriptors.
>> +
>> +choice
>> + prompt "select configuration"
>> + depends on IDMAC_DESC_MODE
>> +
>> +config CHAIN_DESC
>> + bool "Chain Descriptor Structure"
>> + help
>> + Select this option to enable Chained Mode of Operation.
>> +
>> +config DUAL_BUFFER_DESC
>> + bool "Dual Buffer Descriptor Structure"
>> + help
>> + Select this option to enable Dual Buffer Desc Mode of Operation.
>> +endchoice
>
> These config symbols go in the global config namespace so you should
> probably prefix them with "MMC_DW" to prevent pollution of the
> namespace. Also it would be good to add some more description text so
> the user can understand which choice to make.
>
>> +
>> +
>> 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..a590856 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -63,6 +63,9 @@ 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_SIZE(d, s1, s2) \
>> + ((d)->des1 = ((d)->des1 & 0x0) | \
>> + (((s1) & 0x1fff) | ((s2) & 0x1fff << 13)))
>
> IDMAC_SET_BUFFER_SIZES might be a more descriptive name for this macro.
>
>> u32 des2; /* buffer 1 physical address */
>>
>> @@ -348,17 +351,44 @@ static void dw_mci_translate_sglist(struct
>> dw_mci *host, struct mmc_data *data,
>> struct idmac_desc *desc = host->sg_cpu;
>>
>> 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]);
>> -
>> - /* Set the OWN bit and disable interrupts for this descriptor */
>> + /*length and mem_aadress of first buffer*/
>
> There's typo in this comment.
>
>> + unsigned int length1 = sg_dma_len(&data->sg[i]);
>> + u32 mem_addr1 = sg_dma_address(&data->sg[i]);
>> +#ifdef CONFIG_DUAL_BUFFER_DESC
>> + unsigned int length2;
>> + u32 mem_addr2;
>> + if ((i+1) <= sg_len) {
>
> Won't this always be true as i < sg_len? Should it be 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
>> + */
>> + desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC
>> + desc->des0 &= (~IDMAC_DES0_CH);
>
> These two lines are identical in both branches of the if statement so
> could be moved above it.
>
>> + /* Buffer length being set for Buffer1
>> + * and Buffer2 being set
>> + */
>> + IDMAC_SET_BUFFER_SIZE(desc, length1, length2);
>> + desc->des3 = mem_addr2;
>> + /* Incrementing for the second buffer */
>> + i++;
>> + } else {
>> + desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC
>> + desc->des0 &= (~IDMAC_DES0_CH);
>> + /* Buffer length being set for Buffer1
>> + * and Buffer2 being set
>> + */
>> + IDMAC_SET_BUFFER_SIZE(desc, length1, 0);
>> + }
>> +#elif CONFIG_CHAIN_DESC
>> + /* Set 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 length for Buffer1*/
>> + IDMAC_SET_BUFFER1_SIZE(desc, length1);
>> +#endif
>> /* Physical address to DMA to/from */
>> - desc->des2 = mem_addr;
>> + desc->des2 = mem_addr1;
>> }
>>
>> /* Set first descriptor */
>> @@ -369,6 +399,10 @@ static void dw_mci_translate_sglist(struct dw_mci
>> *host, struct mmc_data *data,
>> desc = host->sg_cpu + (i - 1) * sizeof(struct idmac_desc);
>> desc->des0 &= ~(IDMAC_DES0_CH | IDMAC_DES0_DIC);
>> desc->des0 |= IDMAC_DES0_LD;
>> +#ifdef CONFIG_DUAL_BUFFER_DESC
>> + /*Set the End of Ring bit */
>> + desc->des0 |= IDMAC_DES0_ER;
>> +#endif
>>
>> wmb();
>> }
>> @@ -388,7 +422,8 @@ static void dw_mci_idmac_start_dma(struct dw_mci
>> *host, unsigned int sg_len)
>>
>> /* Enable the IDMAC */
>> temp = mci_readl(host, BMOD);
>> - temp |= SDMMC_IDMAC_ENABLE | SDMMC_IDMAC_FB;
>> + /* The Descriptor Skip length is made zero */
>> + temp |= SDMMC_IDMAC_ENABLE | SDMMC_IDMAC_FB & SDMMC_BMOD_DSL(0);
>
> I'm not sure what this is trying to do exactly, but it looks like it
> won't work. Setting DSL 0 just means having 0 in bits 6:2 of BMOD,
> which would be "& ~SDMMC_BMOD_DSL(0)" I would have thought?
>
>> mci_writel(host, BMOD, temp);
>>
>> /* Start it running */
>> --
>> 1.7.2.3
>>
>>
>> --
>> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] mmc: Support of DUAL BUFFER DESC[ring] mode for dw_mmc
2011-08-26 10:31 [PATCH 1/1] mmc: Support of DUAL BUFFER DESC[ring] mode for dw_mmc Shashidhar Hiremath
2011-08-26 11:06 ` Will Newton
@ 2011-08-26 13:39 ` James Hogan
2011-08-26 18:06 ` Shashidhar Hiremath
1 sibling, 1 reply; 7+ messages in thread
From: James Hogan @ 2011-08-26 13:39 UTC (permalink / raw)
To: Shashidhar Hiremath
Cc: Chris Ball, Shawn Guo, Wolfram Sang, Philip Rakity, Zhangfei Gao,
Will Newton, Jaehoon Chung, Kyungmin Park, linux-mmc
Hi,
Thanks for sending the patch.
On 08/26/2011 11:31 AM, 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>
> ---
> drivers/mmc/host/Kconfig | 22 ++++++++++++++++++
> drivers/mmc/host/dw_mmc.c | 55 ++++++++++++++++++++++++++++++++++++--------
> 2 files changed, 67 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 8c87096..e10f585 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -534,6 +534,28 @@ config MMC_DW_IDMAC
> Designware Mobile Storage IP block. This disables the external DMA
> interface.
>
> +config IDMAC_DESC_MODE
I think this config needs to be more specific, otherwise it could
conflict with drivers for other hardware with an internal DMAC, i.e.
something like MMC_DW_IDMAC_DESC_MODE.
> + bool "Internal DMAC Descriptor Operating Mode"
> + depends on MMC_DW_IDMAC
> + help
> + This selects the Operating mode for the Descriptors.
Am I right that there are 3 modes, dual buffer, chain descriptor, and
something else?
This config option doesn't seem necessary. I would put "optional" in the
choice below, or add a choice for whatever the mode is called when
neither option is selected.
If there are only the two options, don't have the choice depend on
IDMAC_DESC_MODE, otherwise if you don't select it, neither of the
choices will be set.
> +
> +choice
> + prompt "select configuration"
> + depends on IDMAC_DESC_MODE
> +
> +config CHAIN_DESC
same here (config name)
> + bool "Chain Descriptor Structure"
> + help
> + Select this option to enable Chained Mode of Operation.
> +
> +config DUAL_BUFFER_DESC
same here (config name)
> + bool "Dual Buffer Descriptor Structure"
> + help
> + Select this option to enable Dual Buffer Desc Mode of Operation.
> +endchoice
> +
> +
probably no need for the extra newline
> 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..a590856 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -63,6 +63,9 @@ 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_SIZE(d, s1, s2) \
> + ((d)->des1 = ((d)->des1 & 0x0) | \
Do you mean to & with 0 there?
> + (((s1) & 0x1fff) | ((s2) & 0x1fff << 13)))
>
> u32 des2; /* buffer 1 physical address */
>
> @@ -348,17 +351,44 @@ static void dw_mci_translate_sglist(struct
> dw_mci *host, struct mmc_data *data,
> struct idmac_desc *desc = host->sg_cpu;
>
> 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]);
> -
> - /* Set the OWN bit and disable interrupts for this descriptor */
> + /*length and mem_aadress of first buffer*/
it'd be nice to have a space after /* and before */ to be consistent.
> + unsigned int length1 = sg_dma_len(&data->sg[i]);
> + u32 mem_addr1 = sg_dma_address(&data->sg[i]);
> +#ifdef CONFIG_DUAL_BUFFER_DESC
> + unsigned int length2;
> + u32 mem_addr2;
> + if ((i+1) <= sg_len) {
should that be "< sg_len", it never hit sg[sg_len] before, but it will
below?
> + 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
> + */
the coding style says to have multiline comments with nothing on the
first line, i.e.
/*
* Set the OWN ....
* ....
*/
> + desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC
> + desc->des0 &= (~IDMAC_DES0_CH);
is this line necessary, since you only just set it without that bit.
> + /* Buffer length being set for Buffer1
> + * and Buffer2 being set
> + */
same here (multiline comment)
> + IDMAC_SET_BUFFER_SIZE(desc, length1, length2);
> + desc->des3 = mem_addr2;
> + /* Incrementing for the second buffer */
> + i++;
> + } else {
> + desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC
> + desc->des0 &= (~IDMAC_DES0_CH);
same here (necessary?)
Also, should this be the same as the non-dual case (also have
IDMAC_DES0_CH), since there is only one sg buffer left?
> + /* Buffer length being set for Buffer1
> + * and Buffer2 being set
> + */
same here (multiline comment)
> + IDMAC_SET_BUFFER_SIZE(desc, length1, 0);
> + }
> +#elif CONFIG_CHAIN_DESC
> + /* Set 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 length for Buffer1*/
same here (comment spacing)
> + IDMAC_SET_BUFFER1_SIZE(desc, length1);
> +#endif
> /* Physical address to DMA to/from */
> - desc->des2 = mem_addr;
> + desc->des2 = mem_addr1;
> }
>
> /* Set first descriptor */
> @@ -369,6 +399,10 @@ static void dw_mci_translate_sglist(struct dw_mci
> *host, struct mmc_data *data,
> desc = host->sg_cpu + (i - 1) * sizeof(struct idmac_desc);
> desc->des0 &= ~(IDMAC_DES0_CH | IDMAC_DES0_DIC);
> desc->des0 |= IDMAC_DES0_LD;
> +#ifdef CONFIG_DUAL_BUFFER_DESC
> + /*Set the End of Ring bit */
same here (comment spacing)
> + desc->des0 |= IDMAC_DES0_ER;
> +#endif
>
> wmb();
> }
> @@ -388,7 +422,8 @@ static void dw_mci_idmac_start_dma(struct dw_mci
> *host, unsigned int sg_len)
>
> /* Enable the IDMAC */
> temp = mci_readl(host, BMOD);
> - temp |= SDMMC_IDMAC_ENABLE | SDMMC_IDMAC_FB;
> + /* The Descriptor Skip length is made zero */
> + temp |= SDMMC_IDMAC_ENABLE | SDMMC_IDMAC_FB & SDMMC_BMOD_DSL(0);
should that be a | instead of &?
Also, where is SDMMC_BMOD_DSL defined?
> mci_writel(host, BMOD, temp);
>
> /* Start it running */
Thanks
James
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] mmc: Support of DUAL BUFFER DESC[ring] mode for dw_mmc
2011-08-26 13:39 ` James Hogan
@ 2011-08-26 18:06 ` Shashidhar Hiremath
2011-08-26 20:04 ` James Hogan
0 siblings, 1 reply; 7+ messages in thread
From: Shashidhar Hiremath @ 2011-08-26 18:06 UTC (permalink / raw)
To: James Hogan
Cc: Chris Ball, Shawn Guo, Wolfram Sang, Philip Rakity, Zhangfei Gao,
Will Newton, Jaehoon Chung, Kyungmin Park, linux-mmc
Hi James,
thanks a lot for the reply.
I will fix all the indentation issues as well.Please find my
comments/queries below:
On Fri, Aug 26, 2011 at 7:09 PM, James Hogan <james.hogan@imgtec.com> wrote:
> Hi,
>
> Thanks for sending the patch.
>
> On 08/26/2011 11:31 AM, 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>
>> ---
>> drivers/mmc/host/Kconfig | 22 ++++++++++++++++++
>> drivers/mmc/host/dw_mmc.c | 55 ++++++++++++++++++++++++++++++++++++--------
>> 2 files changed, 67 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>> index 8c87096..e10f585 100644
>> --- a/drivers/mmc/host/Kconfig
>> +++ b/drivers/mmc/host/Kconfig
>> @@ -534,6 +534,28 @@ config MMC_DW_IDMAC
>> Designware Mobile Storage IP block. This disables the external DMA
>> interface.
>>
>> +config IDMAC_DESC_MODE
>
> I think this config needs to be more specific, otherwise it could
> conflict with drivers for other hardware with an internal DMAC, i.e.
> something like MMC_DW_IDMAC_DESC_MODE.
>
>> + bool "Internal DMAC Descriptor Operating Mode"
>> + depends on MMC_DW_IDMAC
>> + help
>> + This selects the Operating mode for the Descriptors.
>
> Am I right that there are 3 modes, dual buffer, chain descriptor, and
> something else?
> This config option doesn't seem necessary. I would put "optional" in the
> choice below, or add a choice for whatever the mode is called when
> neither option is selected.
> If there are only the two options, don't have the choice depend on
> IDMAC_DESC_MODE, otherwise if you don't select it, neither of the
> choices will be set.
>
There are only two modes actually,and one of them has to be selected
is MMC_DW_IDMAC is seleced i'e If Internal DMAC is selected ,only
then ,the usage of descriptors and its modes used here come into
picture.
So, Is it OK if I add the choice options within MMC_DW_IDMAC option
which reprsents internal DMAC selected ?
>> +
>> +choice
>> + prompt "select configuration"
>> + depends on IDMAC_DESC_MODE
>> +
>> +config CHAIN_DESC
>
> same here (config name)
>
>> + bool "Chain Descriptor Structure"
>> + help
>> + Select this option to enable Chained Mode of Operation.
>> +
>> +config DUAL_BUFFER_DESC
>
> same here (config name)
>
>> + bool "Dual Buffer Descriptor Structure"
>> + help
>> + Select this option to enable Dual Buffer Desc Mode of Operation.
>> +endchoice
>> +
>> +
>
> probably no need for the extra newline
>
>> 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..a590856 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -63,6 +63,9 @@ 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_SIZE(d, s1, s2) \
>> + ((d)->des1 = ((d)->des1 & 0x0) | \
>
> Do you mean to & with 0 there?
Yes, I have made the desc1 '0' first and then I have ORed the length's
of Buffer1 and Buffer2 appropriately as below.The reason I have ANDed
with zero is to remove any junk value present in descriptors.
>
>> + (((s1) & 0x1fff) | ((s2) & 0x1fff << 13)))
>>
>> u32 des2; /* buffer 1 physical address */
>>
>> @@ -348,17 +351,44 @@ static void dw_mci_translate_sglist(struct
>> dw_mci *host, struct mmc_data *data,
>> struct idmac_desc *desc = host->sg_cpu;
>>
>> 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]);
>> -
>> - /* Set the OWN bit and disable interrupts for this descriptor */
>> + /*length and mem_aadress of first buffer*/
>
> it'd be nice to have a space after /* and before */ to be consistent.
>
>> + unsigned int length1 = sg_dma_len(&data->sg[i]);
>> + u32 mem_addr1 = sg_dma_address(&data->sg[i]);
>> +#ifdef CONFIG_DUAL_BUFFER_DESC
>> + unsigned int length2;
>> + u32 mem_addr2;
>> + if ((i+1) <= sg_len) {
>
> should that be "< sg_len", it never hit sg[sg_len] before, but it will
> below?
Yes, You are right, I will make the change.
>
>> + 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
>> + */
>
> the coding style says to have multiline comments with nothing on the
> first line, i.e.
> /*
> * Set the OWN ....
> * ....
> */
>
>> + desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC
>> + desc->des0 &= (~IDMAC_DES0_CH);
>
> is this line necessary, since you only just set it without that bit.
Yes, agreed.I will make the change.
>
>> + /* Buffer length being set for Buffer1
>> + * and Buffer2 being set
>> + */
>
> same here (multiline comment)
>
>> + IDMAC_SET_BUFFER_SIZE(desc, length1, length2);
>> + desc->des3 = mem_addr2;
>> + /* Incrementing for the second buffer */
>> + i++;
>> + } else {
>> + desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC
>> + desc->des0 &= (~IDMAC_DES0_CH);
>
> same here (necessary?)
Agreed,not necessary.I will make the change.
>
> Also, should this be the same as the non-dual case (also have
> IDMAC_DES0_CH), since there is only one sg buffer left?
Please correct me If I am wrong here.I feel since we select the mode
for one complee transfer.I have reset it and made the second buffer
length zero,If dynamic switching to chained mode is possible,I can
probably make it same as for non dual case.
>
>> + /* Buffer length being set for Buffer1
>> + * and Buffer2 being set
>> + */
>
> same here (multiline comment)
>
>> + IDMAC_SET_BUFFER_SIZE(desc, length1, 0);
>> + }
>> +#elif CONFIG_CHAIN_DESC
>> + /* Set 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 length for Buffer1*/
>
> same here (comment spacing)
>
>> + IDMAC_SET_BUFFER1_SIZE(desc, length1);
>> +#endif
>> /* Physical address to DMA to/from */
>> - desc->des2 = mem_addr;
>> + desc->des2 = mem_addr1;
>> }
>>
>> /* Set first descriptor */
>> @@ -369,6 +399,10 @@ static void dw_mci_translate_sglist(struct dw_mci
>> *host, struct mmc_data *data,
>> desc = host->sg_cpu + (i - 1) * sizeof(struct idmac_desc);
>> desc->des0 &= ~(IDMAC_DES0_CH | IDMAC_DES0_DIC);
>> desc->des0 |= IDMAC_DES0_LD;
>> +#ifdef CONFIG_DUAL_BUFFER_DESC
>> + /*Set the End of Ring bit */
>
> same here (comment spacing)
>
>> + desc->des0 |= IDMAC_DES0_ER;
>> +#endif
>>
>> wmb();
>> }
>> @@ -388,7 +422,8 @@ static void dw_mci_idmac_start_dma(struct dw_mci
>> *host, unsigned int sg_len)
>>
>> /* Enable the IDMAC */
>> temp = mci_readl(host, BMOD);
>> - temp |= SDMMC_IDMAC_ENABLE | SDMMC_IDMAC_FB;
>> + /* The Descriptor Skip length is made zero */
>> + temp |= SDMMC_IDMAC_ENABLE | SDMMC_IDMAC_FB & SDMMC_BMOD_DSL(0);
>
> should that be a | instead of &?
Yes, will make this change.
>
> Also, where is SDMMC_BMOD_DSL defined?
>
>> mci_writel(host, BMOD, temp);
>>
>> /* Start it running */
>
> Thanks
> James
>
>
--
regards,
Shashidhar Hiremath
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] mmc: Support of DUAL BUFFER DESC[ring] mode for dw_mmc
2011-08-26 18:06 ` Shashidhar Hiremath
@ 2011-08-26 20:04 ` James Hogan
2011-08-27 4:21 ` Shashidhar Hiremath
0 siblings, 1 reply; 7+ messages in thread
From: James Hogan @ 2011-08-26 20:04 UTC (permalink / raw)
To: Shashidhar Hiremath
Cc: James Hogan, Chris Ball, Shawn Guo, Wolfram Sang, Philip Rakity,
Zhangfei Gao, Will Newton, Jaehoon Chung, Kyungmin Park,
linux-mmc
Hi
On 26 August 2011 19:06, Shashidhar Hiremath
<shashidharh@vayavyalabs.com> wrote:
> Hi James,
> thanks a lot for the reply.
> I will fix all the indentation issues as well.Please find my
> comments/queries below:
>
> On Fri, Aug 26, 2011 at 7:09 PM, James Hogan <james.hogan@imgtec.com> wrote:
>> Hi,
>>
>> Thanks for sending the patch.
>>
>> On 08/26/2011 11:31 AM, 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>
>>> ---
>>> drivers/mmc/host/Kconfig | 22 ++++++++++++++++++
>>> drivers/mmc/host/dw_mmc.c | 55 ++++++++++++++++++++++++++++++++++++--------
>>> 2 files changed, 67 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>>> index 8c87096..e10f585 100644
>>> --- a/drivers/mmc/host/Kconfig
>>> +++ b/drivers/mmc/host/Kconfig
>>> @@ -534,6 +534,28 @@ config MMC_DW_IDMAC
>>> Designware Mobile Storage IP block. This disables the external DMA
>>> interface.
>>>
>>> +config IDMAC_DESC_MODE
>>
>> I think this config needs to be more specific, otherwise it could
>> conflict with drivers for other hardware with an internal DMAC, i.e.
>> something like MMC_DW_IDMAC_DESC_MODE.
>>
>>> + bool "Internal DMAC Descriptor Operating Mode"
>>> + depends on MMC_DW_IDMAC
>>> + help
>>> + This selects the Operating mode for the Descriptors.
>>
>> Am I right that there are 3 modes, dual buffer, chain descriptor, and
>> something else?
>> This config option doesn't seem necessary. I would put "optional" in the
>> choice below, or add a choice for whatever the mode is called when
>> neither option is selected.
>> If there are only the two options, don't have the choice depend on
>> IDMAC_DESC_MODE, otherwise if you don't select it, neither of the
>> choices will be set.
>>
> There are only two modes actually,and one of them has to be selected
> is MMC_DW_IDMAC is seleced i'e If Internal DMAC is selected ,only
> then ,the usage of descriptors and its modes used here come into
> picture.
> So, Is it OK if I add the choice options within MMC_DW_IDMAC option
> which reprsents internal DMAC selected ?
yes, that sounds good.
>>> +
>>> +choice
>>> + prompt "select configuration"
>>> + depends on IDMAC_DESC_MODE
>>> +
>>> +config CHAIN_DESC
>>
>> same here (config name)
>>
>>> + bool "Chain Descriptor Structure"
>>> + help
>>> + Select this option to enable Chained Mode of Operation.
>>> +
>>> +config DUAL_BUFFER_DESC
>>
>> same here (config name)
>>
>>> + bool "Dual Buffer Descriptor Structure"
>>> + help
>>> + Select this option to enable Dual Buffer Desc Mode of Operation.
>>> +endchoice
>>> +
>>> +
>>
>> probably no need for the extra newline
>>
>>> 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..a590856 100644
>>> --- a/drivers/mmc/host/dw_mmc.c
>>> +++ b/drivers/mmc/host/dw_mmc.c
>>> @@ -63,6 +63,9 @@ 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_SIZE(d, s1, s2) \
>>> + ((d)->des1 = ((d)->des1 & 0x0) | \
>>
>> Do you mean to & with 0 there?
> Yes, I have made the desc1 '0' first and then I have ORed the length's
> of Buffer1 and Buffer2 appropriately as below.The reason I have ANDed
> with zero is to remove any junk value present in descriptors.
But (((d)->des1 & 0x0) | X) == (0x0 | X) == X , so you don't need it.
>
>>
>>> + (((s1) & 0x1fff) | ((s2) & 0x1fff << 13)))
>>>
>>> u32 des2; /* buffer 1 physical address */
>>>
>>> @@ -348,17 +351,44 @@ static void dw_mci_translate_sglist(struct
>>> dw_mci *host, struct mmc_data *data,
>>> struct idmac_desc *desc = host->sg_cpu;
>>>
>>> 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]);
>>> -
>>> - /* Set the OWN bit and disable interrupts for this descriptor */
>>> + /*length and mem_aadress of first buffer*/
>>
>> it'd be nice to have a space after /* and before */ to be consistent.
>>
>>> + unsigned int length1 = sg_dma_len(&data->sg[i]);
>>> + u32 mem_addr1 = sg_dma_address(&data->sg[i]);
>>> +#ifdef CONFIG_DUAL_BUFFER_DESC
>>> + unsigned int length2;
>>> + u32 mem_addr2;
>>> + if ((i+1) <= sg_len) {
>>
>> should that be "< sg_len", it never hit sg[sg_len] before, but it will
>> below?
> Yes, You are right, I will make the change.
>>
>>> + 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
>>> + */
>>
>> the coding style says to have multiline comments with nothing on the
>> first line, i.e.
>> /*
>> * Set the OWN ....
>> * ....
>> */
>>
>>> + desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC
>>> + desc->des0 &= (~IDMAC_DES0_CH);
>>
>> is this line necessary, since you only just set it without that bit.
> Yes, agreed.I will make the change.
>>
>>> + /* Buffer length being set for Buffer1
>>> + * and Buffer2 being set
>>> + */
>>
>> same here (multiline comment)
>>
>>> + IDMAC_SET_BUFFER_SIZE(desc, length1, length2);
>>> + desc->des3 = mem_addr2;
>>> + /* Incrementing for the second buffer */
>>> + i++;
>>> + } else {
>>> + desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC
>>> + desc->des0 &= (~IDMAC_DES0_CH);
>>
>> same here (necessary?)
> Agreed,not necessary.I will make the change.
>>
>> Also, should this be the same as the non-dual case (also have
>> IDMAC_DES0_CH), since there is only one sg buffer left?
> Please correct me If I am wrong here.I feel since we select the mode
> for one complee transfer.I have reset it and made the second buffer
> length zero,If dynamic switching to chained mode is possible,I can
> probably make it same as for non dual case.
I don't know off the top of my head. I'd have to check the TRM.
>>
>>> + /* Buffer length being set for Buffer1
>>> + * and Buffer2 being set
>>> + */
>>
>> same here (multiline comment)
>>
>>> + IDMAC_SET_BUFFER_SIZE(desc, length1, 0);
>>> + }
>>> +#elif CONFIG_CHAIN_DESC
>>> + /* Set 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 length for Buffer1*/
>>
>> same here (comment spacing)
>>
>>> + IDMAC_SET_BUFFER1_SIZE(desc, length1);
>>> +#endif
>>> /* Physical address to DMA to/from */
>>> - desc->des2 = mem_addr;
>>> + desc->des2 = mem_addr1;
>>> }
>>>
>>> /* Set first descriptor */
>>> @@ -369,6 +399,10 @@ static void dw_mci_translate_sglist(struct dw_mci
>>> *host, struct mmc_data *data,
>>> desc = host->sg_cpu + (i - 1) * sizeof(struct idmac_desc);
>>> desc->des0 &= ~(IDMAC_DES0_CH | IDMAC_DES0_DIC);
>>> desc->des0 |= IDMAC_DES0_LD;
>>> +#ifdef CONFIG_DUAL_BUFFER_DESC
>>> + /*Set the End of Ring bit */
>>
>> same here (comment spacing)
>>
>>> + desc->des0 |= IDMAC_DES0_ER;
>>> +#endif
>>>
>>> wmb();
>>> }
>>> @@ -388,7 +422,8 @@ static void dw_mci_idmac_start_dma(struct dw_mci
>>> *host, unsigned int sg_len)
>>>
>>> /* Enable the IDMAC */
>>> temp = mci_readl(host, BMOD);
>>> - temp |= SDMMC_IDMAC_ENABLE | SDMMC_IDMAC_FB;
>>> + /* The Descriptor Skip length is made zero */
>>> + temp |= SDMMC_IDMAC_ENABLE | SDMMC_IDMAC_FB & SDMMC_BMOD_DSL(0);
>>
>> should that be a | instead of &?
> Yes, will make this change.
>>
>> Also, where is SDMMC_BMOD_DSL defined?
I couldn't find this definition in LXR, has the header file with it
been missed from the patch?
Chees
James
>>
>>> mci_writel(host, BMOD, temp);
>>>
>>> /* Start it running */
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] mmc: Support of DUAL BUFFER DESC[ring] mode for dw_mmc
2011-08-26 20:04 ` James Hogan
@ 2011-08-27 4:21 ` Shashidhar Hiremath
0 siblings, 0 replies; 7+ messages in thread
From: Shashidhar Hiremath @ 2011-08-27 4:21 UTC (permalink / raw)
To: James Hogan
Cc: Chris Ball, Shawn Guo, Wolfram Sang, Philip Rakity, Zhangfei Gao,
Will Newton, Jaehoon Chung, Kyungmin Park, linux-mmc
On Sat, Aug 27, 2011 at 1:34 AM, James Hogan <james.hogan@imgtec.com> wrote:
> Hi
>
> On 26 August 2011 19:06, Shashidhar Hiremath
> <shashidharh@vayavyalabs.com> wrote:
>> Hi James,
>> thanks a lot for the reply.
>> I will fix all the indentation issues as well.Please find my
>> comments/queries below:
>>
>> On Fri, Aug 26, 2011 at 7:09 PM, James Hogan <james.hogan@imgtec.com> wrote:
>>> Hi,
>>>
>>> Thanks for sending the patch.
>>>
>>> On 08/26/2011 11:31 AM, 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>
>>>> ---
>>>> drivers/mmc/host/Kconfig | 22 ++++++++++++++++++
>>>> drivers/mmc/host/dw_mmc.c | 55 ++++++++++++++++++++++++++++++++++++--------
>>>> 2 files changed, 67 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
>>>> index 8c87096..e10f585 100644
>>>> --- a/drivers/mmc/host/Kconfig
>>>> +++ b/drivers/mmc/host/Kconfig
>>>> @@ -534,6 +534,28 @@ config MMC_DW_IDMAC
>>>> Designware Mobile Storage IP block. This disables the external DMA
>>>> interface.
>>>>
>>>> +config IDMAC_DESC_MODE
>>>
>>> I think this config needs to be more specific, otherwise it could
>>> conflict with drivers for other hardware with an internal DMAC, i.e.
>>> something like MMC_DW_IDMAC_DESC_MODE.
>>>
>>>> + bool "Internal DMAC Descriptor Operating Mode"
>>>> + depends on MMC_DW_IDMAC
>>>> + help
>>>> + This selects the Operating mode for the Descriptors.
>>>
>>> Am I right that there are 3 modes, dual buffer, chain descriptor, and
>>> something else?
>>> This config option doesn't seem necessary. I would put "optional" in the
>>> choice below, or add a choice for whatever the mode is called when
>>> neither option is selected.
>>> If there are only the two options, don't have the choice depend on
>>> IDMAC_DESC_MODE, otherwise if you don't select it, neither of the
>>> choices will be set.
>>>
>> There are only two modes actually,and one of them has to be selected
>> is MMC_DW_IDMAC is seleced i'e If Internal DMAC is selected ,only
>> then ,the usage of descriptors and its modes used here come into
>> picture.
>> So, Is it OK if I add the choice options within MMC_DW_IDMAC option
>> which reprsents internal DMAC selected ?
>
> yes, that sounds good.
>
>>>> +
>>>> +choice
>>>> + prompt "select configuration"
>>>> + depends on IDMAC_DESC_MODE
>>>> +
>>>> +config CHAIN_DESC
>>>
>>> same here (config name)
>>>
>>>> + bool "Chain Descriptor Structure"
>>>> + help
>>>> + Select this option to enable Chained Mode of Operation.
>>>> +
>>>> +config DUAL_BUFFER_DESC
>>>
>>> same here (config name)
>>>
>>>> + bool "Dual Buffer Descriptor Structure"
>>>> + help
>>>> + Select this option to enable Dual Buffer Desc Mode of Operation.
>>>> +endchoice
>>>> +
>>>> +
>>>
>>> probably no need for the extra newline
>>>
>>>> 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..a590856 100644
>>>> --- a/drivers/mmc/host/dw_mmc.c
>>>> +++ b/drivers/mmc/host/dw_mmc.c
>>>> @@ -63,6 +63,9 @@ 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_SIZE(d, s1, s2) \
>>>> + ((d)->des1 = ((d)->des1 & 0x0) | \
>>>
>>> Do you mean to & with 0 there?
>> Yes, I have made the desc1 '0' first and then I have ORed the length's
>> of Buffer1 and Buffer2 appropriately as below.The reason I have ANDed
>> with zero is to remove any junk value present in descriptors.
>
> But (((d)->des1 & 0x0) | X) == (0x0 | X) == X , so you don't need it.
Agreed,Will make this change .
>
>>
>>>
>>>> + (((s1) & 0x1fff) | ((s2) & 0x1fff << 13)))
>>>>
>>>> u32 des2; /* buffer 1 physical address */
>>>>
>>>> @@ -348,17 +351,44 @@ static void dw_mci_translate_sglist(struct
>>>> dw_mci *host, struct mmc_data *data,
>>>> struct idmac_desc *desc = host->sg_cpu;
>>>>
>>>> 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]);
>>>> -
>>>> - /* Set the OWN bit and disable interrupts for this descriptor */
>>>> + /*length and mem_aadress of first buffer*/
>>>
>>> it'd be nice to have a space after /* and before */ to be consistent.
>>>
>>>> + unsigned int length1 = sg_dma_len(&data->sg[i]);
>>>> + u32 mem_addr1 = sg_dma_address(&data->sg[i]);
>>>> +#ifdef CONFIG_DUAL_BUFFER_DESC
>>>> + unsigned int length2;
>>>> + u32 mem_addr2;
>>>> + if ((i+1) <= sg_len) {
>>>
>>> should that be "< sg_len", it never hit sg[sg_len] before, but it will
>>> below?
>> Yes, You are right, I will make the change.
>>>
>>>> + 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
>>>> + */
>>>
>>> the coding style says to have multiline comments with nothing on the
>>> first line, i.e.
>>> /*
>>> * Set the OWN ....
>>> * ....
>>> */
>>>
>>>> + desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC
>>>> + desc->des0 &= (~IDMAC_DES0_CH);
>>>
>>> is this line necessary, since you only just set it without that bit.
>> Yes, agreed.I will make the change.
>>>
>>>> + /* Buffer length being set for Buffer1
>>>> + * and Buffer2 being set
>>>> + */
>>>
>>> same here (multiline comment)
>>>
>>>> + IDMAC_SET_BUFFER_SIZE(desc, length1, length2);
>>>> + desc->des3 = mem_addr2;
>>>> + /* Incrementing for the second buffer */
>>>> + i++;
>>>> + } else {
>>>> + desc->des0 = IDMAC_DES0_OWN | IDMAC_DES0_DIC
>>>> + desc->des0 &= (~IDMAC_DES0_CH);
>>>
>>> same here (necessary?)
>> Agreed,not necessary.I will make the change.
>>>
>>> Also, should this be the same as the non-dual case (also have
>>> IDMAC_DES0_CH), since there is only one sg buffer left?
>> Please correct me If I am wrong here.I feel since we select the mode
>> for one complee transfer.I have reset it and made the second buffer
>> length zero,If dynamic switching to chained mode is possible,I can
>> probably make it same as for non dual case.
>
> I don't know off the top of my head. I'd have to check the TRM.
>
>>>
>>>> + /* Buffer length being set for Buffer1
>>>> + * and Buffer2 being set
>>>> + */
>>>
>>> same here (multiline comment)
>>>
>>>> + IDMAC_SET_BUFFER_SIZE(desc, length1, 0);
>>>> + }
>>>> +#elif CONFIG_CHAIN_DESC
>>>> + /* Set 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 length for Buffer1*/
>>>
>>> same here (comment spacing)
>>>
>>>> + IDMAC_SET_BUFFER1_SIZE(desc, length1);
>>>> +#endif
>>>> /* Physical address to DMA to/from */
>>>> - desc->des2 = mem_addr;
>>>> + desc->des2 = mem_addr1;
>>>> }
>>>>
>>>> /* Set first descriptor */
>>>> @@ -369,6 +399,10 @@ static void dw_mci_translate_sglist(struct dw_mci
>>>> *host, struct mmc_data *data,
>>>> desc = host->sg_cpu + (i - 1) * sizeof(struct idmac_desc);
>>>> desc->des0 &= ~(IDMAC_DES0_CH | IDMAC_DES0_DIC);
>>>> desc->des0 |= IDMAC_DES0_LD;
>>>> +#ifdef CONFIG_DUAL_BUFFER_DESC
>>>> + /*Set the End of Ring bit */
>>>
>>> same here (comment spacing)
>>>
>>>> + desc->des0 |= IDMAC_DES0_ER;
>>>> +#endif
>>>>
>>>> wmb();
>>>> }
>>>> @@ -388,7 +422,8 @@ static void dw_mci_idmac_start_dma(struct dw_mci
>>>> *host, unsigned int sg_len)
>>>>
>>>> /* Enable the IDMAC */
>>>> temp = mci_readl(host, BMOD);
>>>> - temp |= SDMMC_IDMAC_ENABLE | SDMMC_IDMAC_FB;
>>>> + /* The Descriptor Skip length is made zero */
>>>> + temp |= SDMMC_IDMAC_ENABLE | SDMMC_IDMAC_FB & SDMMC_BMOD_DSL(0);
>>>
>>> should that be a | instead of &?
>> Yes, will make this change.
>>>
>>> Also, where is SDMMC_BMOD_DSL defined?
>
> I couldn't find this definition in LXR, has the header file with it
> been missed from the patch?
sorry,some how forgot to add the patch for header.Will do it.
>
> Chees
> James
>
>>>
>>>> mci_writel(host, BMOD, temp);
>>>>
>>>> /* Start it running */
>
--
regards,
Shashidhar Hiremath
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-08-27 4:21 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-08-26 10:31 [PATCH 1/1] mmc: Support of DUAL BUFFER DESC[ring] mode for dw_mmc Shashidhar Hiremath
2011-08-26 11:06 ` Will Newton
2011-08-26 11:20 ` Shashidhar Hiremath
2011-08-26 13:39 ` James Hogan
2011-08-26 18:06 ` Shashidhar Hiremath
2011-08-26 20:04 ` James Hogan
2011-08-27 4:21 ` Shashidhar Hiremath
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.