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. 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
