* [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox