On Aug 7, 2011, at 11:46 PM, Nath, Arindam wrote: > Hi Subhash, > > >> -----Original Message----- >> From: Subhash Jadavani [mailto:subhashj@xxxxxxxxxxxxxx] >> Sent: Monday, August 08, 2011 12:09 PM >> To: Nath, Arindam; linux-mmc@xxxxxxxxxxxxxxx; 'Philip Rakity' >> Cc: linux-arm-msm@xxxxxxxxxxxxxxx >> 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:Arindam.Nath@xxxxxxx] >>> Sent: Friday, August 05, 2011 10:54 PM >>> To: Subhash Jadavani; linux-mmc@xxxxxxxxxxxxxxx; 'Philip Rakity' >>> Cc: linux-arm-msm@xxxxxxxxxxxxxxx >>> 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:subhashj@xxxxxxxxxxxxxx] >>>> Sent: Friday, August 05, 2011 9:55 PM >>>> To: linux-mmc@xxxxxxxxxxxxxxx; Nath, Arindam; 'Philip Rakity' >>>> Cc: linux-arm-msm@xxxxxxxxxxxxxxx >>>> 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: linux-arm-msm-owner@xxxxxxxxxxxxxxx [mailto:linux-arm-msm- >>>>> owner@xxxxxxxxxxxxxxx] On Behalf Of Subhash Jadavani >>>>> Sent: Thursday, August 04, 2011 4:08 PM >>>>> To: linux-mmc@xxxxxxxxxxxxxxx >>>>> Cc: linux-arm-msm@xxxxxxxxxxxxxxx; 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 <subhashj@xxxxxxxxxxxxxx> >>>>> --- >>>>> 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 majordomo@xxxxxxxxxxxxxxx >>>>> More majordomo info at http://vger.kernel.org/majordomo- >> info.html >>>> >>> >> >> > > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
