On Aug 7, 2011, at 11:46 PM, Nath, Arindam wrote:
> Hi Subhash,
>
>
>> -----Original Message-----
>> From: Subhash Jadavani [mailto:[email protected]]
>> Sent: Monday, August 08, 2011 12:09 PM
>> To: Nath, Arindam; [email protected]; 'Philip Rakity'
>> Cc: [email protected]
>> Subject: RE: [PATCH v1 1/1] mmc: sd: UHS-I bus speed should be set last
>> in UHS initialization
>>
>> Hi Arindam,
>>
>>> -----Original Message-----
>>> From: Nath, Arindam [mailto:[email protected]]
>>> Sent: Friday, August 05, 2011 10:54 PM
>>> To: Subhash Jadavani; [email protected]; 'Philip Rakity'
>>> Cc: [email protected]
>>> Subject: RE: [PATCH v1 1/1] mmc: sd: UHS-I bus speed should be set
>> last
>>> in UHS initialization
>>>
>>> Hi Subhash,
>>>
>>> The patch in general looks good to me, I have one comment below.
>>>
>>>> -----Original Message-----
>>>> From: Subhash Jadavani [mailto:[email protected]]
>>>> Sent: Friday, August 05, 2011 9:55 PM
>>>> To: [email protected]; Nath, Arindam; 'Philip Rakity'
>>>> Cc: [email protected]
>>>> Subject: RE: [PATCH v1 1/1] mmc: sd: UHS-I bus speed should be set
>>> last
>>>> in UHS initialization
>>>>
>>>> Chris, Arindam, Philip,
>>>>
>>>> Please let me know you comments on this reworked patch.
>>>>
>>>> Regards,
>>>> Subhash
>>>>
>>>>> -----Original Message-----
>>>>> From: [email protected] [mailto:linux-arm-msm-
>>>>> [email protected]] On Behalf Of Subhash Jadavani
>>>>> Sent: Thursday, August 04, 2011 4:08 PM
>>>>> To: [email protected]
>>>>> Cc: [email protected]; Subhash Jadavani
>>>>> Subject: [PATCH v1 1/1] mmc: sd: UHS-I bus speed should be set
>> last
>>>> in
>>>>> UHS initialization
>>>>>
>>>>> mmc_sd_init_uhs_card function sets the driver type, current limit
>>>>> and bus speed mode on card as well as on host controller side.
>>>>>
>>>>> Currently bus speed mode is set by sending CMD6 to card and
>>>>> immediately setting the timing mode in host controller. But
>>>>> then before initiating tuning sequence, it also tries to set
>>>>> current limit by sending CMD6 to card which results in data
>>>>> timeout errors in controller if bus speed mode is SDR50/SDR104
>>> mode.
>>>>>
>>>>> So basically bus speed mode should be set only after current
>> limit
>>>>> is set in the card and immediately after setting the bus speed
>>> mode,
>>>>> tuning sequence should be initiated.
>>>>>
>>>>> Signed-off-by: Subhash Jadavani <[email protected]>
>>>>> ---
>>>>> drivers/mmc/core/sd.c | 65 ++++++++++++++++++++++++++++++++++-
>> --
>>> --
>>>> --
>>>>> --------
>>>>> 1 files changed, 45 insertions(+), 20 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
>>>>> index ff27741..769e587 100644
>>>>> --- a/drivers/mmc/core/sd.c
>>>>> +++ b/drivers/mmc/core/sd.c
>>>>> @@ -459,10 +459,9 @@ static int sd_select_driver_type(struct
>>> mmc_card
>>>>> *card, u8 *status)
>>>>> return 0;
>>>>> }
>>>>>
>>>>> -static int sd_set_bus_speed_mode(struct mmc_card *card, u8
>>> *status)
>>>>> +static int sd_get_bus_speed_mode(struct mmc_card *card)
>>>>> {
>>>>> - unsigned int bus_speed = 0, timing = 0;
>>>>> - int err;
>>>>> + int bus_speed = 0;
>>>>>
>>>>> /*
>>>>> * If the host doesn't support any of the UHS-I modes,
>> fallback
>>>>> on
>>>>> @@ -475,32 +474,57 @@ static int sd_set_bus_speed_mode(struct
>>>> mmc_card
>>>>> *card, u8 *status)
>>>>> if ((card->host->caps & MMC_CAP_UHS_SDR104) &&
>>>>> (card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR104)) {
>>>>> bus_speed = UHS_SDR104_BUS_SPEED;
>>>>> - timing = MMC_TIMING_UHS_SDR104;
>>>>> - card->sw_caps.uhs_max_dtr = UHS_SDR104_MAX_DTR;
>>>>> } else if ((card->host->caps & MMC_CAP_UHS_DDR50) &&
>>>>> (card->sw_caps.sd3_bus_mode & SD_MODE_UHS_DDR50))
>> {
>>>>> bus_speed = UHS_DDR50_BUS_SPEED;
>>>>> - timing = MMC_TIMING_UHS_DDR50;
>>>>> - card->sw_caps.uhs_max_dtr = UHS_DDR50_MAX_DTR;
>>>>> } else if ((card->host->caps & (MMC_CAP_UHS_SDR104 |
>>>>> MMC_CAP_UHS_SDR50)) && (card-
>>> sw_caps.sd3_bus_mode &
>>>>> SD_MODE_UHS_SDR50)) {
>>>>> bus_speed = UHS_SDR50_BUS_SPEED;
>>>>> - timing = MMC_TIMING_UHS_SDR50;
>>>>> - card->sw_caps.uhs_max_dtr = UHS_SDR50_MAX_DTR;
>>>>> } else if ((card->host->caps & (MMC_CAP_UHS_SDR104 |
>>>>> MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR25)) &&
>>>>> (card->sw_caps.sd3_bus_mode & SD_MODE_UHS_SDR25))
>> {
>>>>> bus_speed = UHS_SDR25_BUS_SPEED;
>>>>> - timing = MMC_TIMING_UHS_SDR25;
>>>>> - card->sw_caps.uhs_max_dtr = UHS_SDR25_MAX_DTR;
>>>>> } else if ((card->host->caps & (MMC_CAP_UHS_SDR104 |
>>>>> MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR25 |
>>>>> MMC_CAP_UHS_SDR12)) && (card-
>>> sw_caps.sd3_bus_mode &
>>>>> SD_MODE_UHS_SDR12)) {
>>>>> bus_speed = UHS_SDR12_BUS_SPEED;
>>>>> - timing = MMC_TIMING_UHS_SDR12;
>>>>> - card->sw_caps.uhs_max_dtr = UHS_SDR12_MAX_DTR;
>>>>> + }
>>>>> +
>>>>> + return bus_speed;
>>>>> +}
>>>>> +
>>>>> +static int sd_set_bus_speed_mode(struct mmc_card *card, u8
>>> *status)
>>>>> +{
>>>>> + int err, bus_speed;
>>>>> + unsigned int timing = 0;
>>>>> +
>>>>> + bus_speed = sd_get_bus_speed_mode(card);
>>>>> +
>>>>> + switch (bus_speed) {
>>>>> + case UHS_SDR104_BUS_SPEED:
>>>>> + timing = MMC_TIMING_UHS_SDR104;
>>>>> + card->sw_caps.uhs_max_dtr = UHS_SDR104_MAX_DTR;
>>>>> + break;
>>>>> + case UHS_DDR50_BUS_SPEED:
>>>>> + timing = MMC_TIMING_UHS_DDR50;
>>>>> + card->sw_caps.uhs_max_dtr = UHS_DDR50_MAX_DTR;
>>>>> + break;
>>>>> + case UHS_SDR50_BUS_SPEED:
>>>>> + timing = MMC_TIMING_UHS_SDR50;
>>>>> + card->sw_caps.uhs_max_dtr = UHS_SDR50_MAX_DTR;
>>>>> + break;
>>>>> + case UHS_SDR25_BUS_SPEED:
>>>>> + timing = MMC_TIMING_UHS_SDR25;
>>>>> + card->sw_caps.uhs_max_dtr = UHS_SDR25_MAX_DTR;
>>>>> + break;
>>>>> + case UHS_SDR12_BUS_SPEED:
>>>>> + timing = MMC_TIMING_UHS_SDR12;
>>>>> + card->sw_caps.uhs_max_dtr = UHS_SDR12_MAX_DTR;
>>>>> + break;
>>>>> + default:
>>>>> + return 0;
>>>>> }
>>>>>
>>>>> card->sd_bus_speed = bus_speed;
>>>
>>> There is redundancy in the use of a variable here. We have
>> *bus_speed*,
>>> and we have *card->sd_bus_speed* which will eventually have the same
>>> value. The point to consider is this, if you use
>>> sd_get_bus_speed_mode() to return the current bus speed, then there
>> is
>>> a function call overhead, may be minor, associated with the call.
>> Right
>>> now we only use the return value from this function in
>>> sd_set_current_limit(), but in future there might be multiple calls
>>> from other functions. So rather than returning the *bus_speed* from
>>> sd_get_bus_speed() everytime, IMO it's better to have it stored in
>>> mmc_card structure, which can be used anytime later without any
>>> overhead. I would like to have another opinion on this.
>>>
>>
>> Yes, that is also fine. then I can do something like this: Rename
>> sd_get_bus_speed_mode to sd_update_bus_speed_mode() and this function
>> will
>> just set the "card->sd_bus_speed" mode depending on card and host
>> capability. Then call sd_update_bus_speed_mode() in
>> mmc_sd_init_uhs_card()
>> before calling
>> sd_select_driver_type(),sd_set_current_limit(),sd_set_bus_speed_mode()
>> functions and in all of these *set* functions, directly use
>> "card->sd_bus_speed".
>>
>> Is this fine? if yes, then I can post the new patch with these changes.
>
> I am OK with your suggestion.
>
> Philip, do you want to add anything?
I am okay with this. I need to see the final patch since I have code for sdio
cards ready
that I will submit and would like to check things with the change. Might be a
case to
refactor the code to a one common routine and specific code for sd/sdio.
Philip
>
>>
>> Regards,
>> Subhash
>>
>>
>>> Thanks,
>>> Arindam
>>>
>>>>> @@ -522,6 +546,7 @@ static int sd_set_bus_speed_mode(struct
>>> mmc_card
>>>>> *card, u8 *status)
>>>>> static int sd_set_current_limit(struct mmc_card *card, u8
>> *status)
>>>>> {
>>>>> int current_limit = 0;
>>>>> + unsigned int bus_speed = sd_get_bus_speed_mode(card);
>>>>> int err;
>>>>>
>>>>> /*
>>>>> @@ -529,9 +554,9 @@ static int sd_set_current_limit(struct
>> mmc_card
>>>>> *card, u8 *status)
>>>>> * bus speed modes. For other bus speed modes, we set the
>> default
>>>>> * current limit of 200mA.
>>>>> */
>>>>> - if ((card->sd_bus_speed == UHS_SDR50_BUS_SPEED) ||
>>>>> - (card->sd_bus_speed == UHS_SDR104_BUS_SPEED) ||
>>>>> - (card->sd_bus_speed == UHS_DDR50_BUS_SPEED)) {
>>>>> + if ((bus_speed == UHS_SDR50_BUS_SPEED) ||
>>>>> + (bus_speed == UHS_SDR104_BUS_SPEED) ||
>>>>> + (bus_speed == UHS_DDR50_BUS_SPEED)) {
>>>>> if (card->host->caps & MMC_CAP_MAX_CURRENT_800) {
>>>>> if (card->sw_caps.sd3_curr_limit &
>>>>> SD_MAX_CURRENT_800)
>>>>> current_limit = SD_SET_CURRENT_LIMIT_800;
>>>>> @@ -613,13 +638,13 @@ static int mmc_sd_init_uhs_card(struct
>>> mmc_card
>>>>> *card)
>>>>> if (err)
>>>>> goto out;
>>>>>
>>>>> - /* Set bus speed mode of the card */
>>>>> - err = sd_set_bus_speed_mode(card, status);
>>>>> + /* Set current limit for the card */
>>>>> + err = sd_set_current_limit(card, status);
>>>>> if (err)
>>>>> goto out;
>>>>>
>>>>> - /* Set current limit for the card */
>>>>> - err = sd_set_current_limit(card, status);
>>>>> + /* Set bus speed mode of the card */
>>>>> + err = sd_set_bus_speed_mode(card, status);
>>>>> if (err)
>>>>> goto out;
>>>>>
>>>>> --
>>>>> 1.7.1.1
>>>>>
>>>>> --
>>>>> Sent by a consultant of the Qualcomm Innovation Center, Inc.
>>>>> The Qualcomm Innovation Center, Inc. is a member of the Code
>> Aurora
>>>>> Forum.
>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-
>>> arm-
>>>>> msm" in
>>>>> the body of a message to [email protected]
>>>>> More majordomo info at http://vger.kernel.org/majordomo-
>> info.html
>>>>
>>>
>>
>>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
