Hi
On 26 August 2011 19:06, Shashidhar Hiremath
<[email protected]> 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 <[email protected]> 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 <[email protected]>
>>> ---
>>> 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 */
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
