RE: [PATCH v1 1/1] mmc: sd: UHS-I bus speed should be set last in UHS initialization

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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?

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


[Index of Archives]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux