Re: [PATCH v6] mmc: dw_mmc: Add IDMAC 64-bit address mode support

[Jaehoon Chung]

On 10/16/2014 08:09 PM, Vivek Gautam wrote:
> Hi Prabu,
> 
> 
> On Thu, Oct 16, 2014 at 4:09 PM, Prabu Thangamuthu <Prabu.T@xxxxxxxxxxxx> wrote:
>>
>> Hi Jaehoon, Vivek, Alim,
>>
>> Thanks for your review comments.
>>
>> On Thu, Oct 16, 2014 at 12:20 PM, Jaehoon Chung <jh80.chung@xxxxxxxxxxx> wrote:
>>> Hi, Prabu.
>>>
>>> On 10/15/2014 09:05 PM, Vivek Gautam wrote:
>>>> Hi Prabu,
>>>>
>>>>
>>>> On Tue, Oct 14, 2014 at 5:41 PM, Alim Akhtar <alim.akhtar@xxxxxxxxx>
>>> wrote:
>>>>> Hi Prahu,
>>>>> Thanks for a quick re-spin o the patch.
>>>>> One last comment, this is more of a information seek.
>>>>> On Thu, Oct 9, 2014 at 1:09 PM, Prabu Thangamuthu
>>> <Prabu.T@xxxxxxxxxxxx> wrote:
>>>>>> Synopsys DW_MMC IP core supports Internal DMA Controller with 64-bit
>>> address mode from IP version 2.70a onwards.
>>>>>> Updated the driver to support IDMAC 64-bit addressing mode.
>>>>>>
>>>>>> Tested the features in DW_MMC core v2.70a and v2.40a with HAPS-51
>>> setup and driver is working fine.
>>>>>>
>>>>>> Signed-off-by: Prabu Thangamuthu <prabu.t@xxxxxxxxxxxx>
>>>>>> ---
>>>>>> Change log v6:
>>>>>>         - Updated with review comment.
>>>>>>
>>>>>> Change log v5:
>>>>>>         - Recreated the patch against linux-next as this patch is
>>>>>> required for another patch
>>>>>> http://www.spinics.net/lists/arm-kernel/msg357985.html
>>>>>>
>>>>>> Change log v4:
>>>>>>         - Add the dynamic support for 32-bit and 64-bit address mode based
>>> on hw configuration.
>>>>>>         - Removed the CONFIG_MMC_DW_IDMAC_64BIT_ADDRESS macro.
>>>>>>
>>>>>>  drivers/mmc/host/dw_mmc.c  |  195
>>> +++++++++++++++++++++++++++++++++++---------
>>>>>>  drivers/mmc/host/dw_mmc.h  |   11 +++
>>>>>>  include/linux/mmc/dw_mmc.h |    2 +
>>>>>>  3 files changed, 170 insertions(+), 38 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mmc/host/dw_mmc.c
>>> b/drivers/mmc/host/dw_mmc.c
>>>>>> index 69f0cc6..c3b75a5 100644
>>>>>> --- a/drivers/mmc/host/dw_mmc.c
>>>>>> +++ b/drivers/mmc/host/dw_mmc.c
>>>>>> @@ -62,6 +62,24 @@
>>>>>>                                  SDMMC_IDMAC_INT_FBE | SDMMC_IDMAC_INT_RI | \
>>>>>>                                  SDMMC_IDMAC_INT_TI)
>>>>>>
>>>>>> +struct idmac_desc_64addr {
>>>>>> +       u32             des0;   /* Control Descriptor */
>>>>>> +
>>>>>> +       u32             des1;   /* Reserved */
>>>>> What is the default value of these _Reserved_ descriptors? As these
>>>>> are pointers, do you thing initializing then to zero make sense?
>>
>> We don't think it's required to assign zero to reserved des field as mobster core not going to use this field's for any operations unless user has modified it.
>> Also it's working fine in our test set-up without initializing them to zero.

Right, it's not required to assign zero to there. And It's working fine, because it's not used or referred anywhere.
Just Recommend to initialize to zero.
Actually, i don't understand why des1 is reserved..

>>
>>>>
>>>> I think the default reset value of these descriptors will depend on
>>>> how dwmmc is integrated in h/w and how these descriptors are then
>>>> configured.
>>>>
>>>> So it makes sense to initialize these descriptors to zero before use.
>>>> We have seen on our exynos7 system, that we need to initialize the
>>>> descriptors des1 and des2 to zero before we use them further.
>>
>> It looks like exynos7 is using this reserved fields for some other purposes. To make it generic, we will initialize the reserved fields to zero.
>>
>> Does exynos7 requires des2 (Buffer sizes) also to be initialized to zero?
> 
> Yes, we need des2 to be initialized to zero.
> One question however, do we have some default reset value set to these
> descriptors ? If yes then how can we check this value ?

I knew it doesn't have the reset value for descriptors.

Best Regards,
Jaehoon Chung

> I just tried printing the value of des1 and des2 in dw_mci_translate_sglist()
> where we are assigning these descriptors.
> 
>>
>>>
>>> I agreed with Vivek and Alim's comment.
>>>
>>> And I think you can fix the below compile warning.
>>>
>>> drivers/mmc/host/dw_mmc.c: In function âdw_mci_idmac_initâ:
>>> drivers/mmc/host/dw_mmc.c:542:21: warning: right shift count >= width of
>>> type [enabled by default]
>>> drivers/mmc/host/dw_mmc.c:547:3: warning: right shift count >= width of
>>> type [enabled by default]
>>> drivers/mmc/host/dw_mmc.c:575:3: warning: right shift count >= width of
>>> type [enabled by default]
>>>
>>> I will check this patch with 2.70a and other version.
>>>
>>
>> We will fix the warnings and post the new patch.
>>
>> Regards,
>> Prabu Thangamuthu.
>>
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/