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

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

[Subhash Jadavani]

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@codeaurora.org>
---
 drivers/mmc/core/sd.c |   57 ++++++++++++++++++++++++++++++++----------------
 1 files changed, 38 insertions(+), 19 deletions(-)

diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index ff27741..d257496 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
@@ -470,37 +469,56 @@ static int sd_set_bus_speed_mode(struct mmc_card *card, u8 *status)
 	 */
 	if (!(card->host->caps & (MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 |
 	    MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR104 | MMC_CAP_UHS_DDR50)))
-		return 0;
+		return -EINVAL;
 
 	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;
+	}
+
+	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);
+
+	if (bus_speed == UHS_SDR104_BUS_SPEED) {
+			timing = MMC_TIMING_UHS_SDR104;
+			card->sw_caps.uhs_max_dtr = UHS_SDR104_MAX_DTR;
+	} else if (bus_speed == UHS_DDR50_BUS_SPEED) {
+			timing = MMC_TIMING_UHS_DDR50;
+			card->sw_caps.uhs_max_dtr = UHS_DDR50_MAX_DTR;
+	} else if (bus_speed == UHS_SDR50_BUS_SPEED) {
+			timing = MMC_TIMING_UHS_SDR50;
+			card->sw_caps.uhs_max_dtr = UHS_SDR50_MAX_DTR;
+	} else if (bus_speed == UHS_SDR25_BUS_SPEED) {
+			timing = MMC_TIMING_UHS_SDR25;
+			card->sw_caps.uhs_max_dtr = UHS_SDR25_MAX_DTR;
+	} else if (bus_speed == UHS_SDR12_BUS_SPEED) {
 			timing = MMC_TIMING_UHS_SDR12;
 			card->sw_caps.uhs_max_dtr = UHS_SDR12_MAX_DTR;
+	} else {
+		return -EINVAL;
 	}
 
 	card->sd_bus_speed = bus_speed;
@@ -522,6 +540,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 +548,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 +632,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.

Re: [PATCH] mmc: sd: UHS-I bus speed should be set last in UHS initialization

[Chris Ball]

Hi Subhash, adding Arindam for review,

On Mon, Aug 01 2011, Subhash Jadavani wrote:
> 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@codeaurora.org>
> ---
>  drivers/mmc/core/sd.c |   57 ++++++++++++++++++++++++++++++++----------------
>  1 files changed, 38 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index ff27741..d257496 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
> @@ -470,37 +469,56 @@ static int sd_set_bus_speed_mode(struct mmc_card *card, u8 *status)
>  	 */
>  	if (!(card->host->caps & (MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 |
>  	    MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR104 | MMC_CAP_UHS_DDR50)))
> -		return 0;
> +		return -EINVAL;
>  
>  	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;
> +	}
> +
> +	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);
> +
> +	if (bus_speed == UHS_SDR104_BUS_SPEED) {
> +			timing = MMC_TIMING_UHS_SDR104;
> +			card->sw_caps.uhs_max_dtr = UHS_SDR104_MAX_DTR;
> +	} else if (bus_speed == UHS_DDR50_BUS_SPEED) {
> +			timing = MMC_TIMING_UHS_DDR50;
> +			card->sw_caps.uhs_max_dtr = UHS_DDR50_MAX_DTR;
> +	} else if (bus_speed == UHS_SDR50_BUS_SPEED) {
> +			timing = MMC_TIMING_UHS_SDR50;
> +			card->sw_caps.uhs_max_dtr = UHS_SDR50_MAX_DTR;
> +	} else if (bus_speed == UHS_SDR25_BUS_SPEED) {
> +			timing = MMC_TIMING_UHS_SDR25;
> +			card->sw_caps.uhs_max_dtr = UHS_SDR25_MAX_DTR;
> +	} else if (bus_speed == UHS_SDR12_BUS_SPEED) {
>  			timing = MMC_TIMING_UHS_SDR12;
>  			card->sw_caps.uhs_max_dtr = UHS_SDR12_MAX_DTR;
> +	} else {
> +		return -EINVAL;
>  	}

Minor change: I think "switch (sd_get_bus_speed_mode(card))" would be cleaner.

>  
>  	card->sd_bus_speed = bus_speed;
> @@ -522,6 +540,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 +548,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 +632,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;
>  

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

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

[Nath, Arindam]

Hi Subhash,


> -----Original Message-----
> From: Chris Ball [mailto:cjb@laptop.org]
> Sent: Tuesday, August 02, 2011 1:40 AM
> To: Subhash Jadavani
> Cc: linux-mmc@vger.kernel.org; linux-arm-msm@vger.kernel.org; Nath,
> Arindam
> Subject: Re: [PATCH] mmc: sd: UHS-I bus speed should be set last in UHS
> initialization
> 
> Hi Subhash, adding Arindam for review,
> 
> On Mon, Aug 01 2011, Subhash Jadavani wrote:
> > 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@codeaurora.org>
> > ---
> >  drivers/mmc/core/sd.c |   57 ++++++++++++++++++++++++++++++++-------
> ---------
> >  1 files changed, 38 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> > index ff27741..d257496 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
> > @@ -470,37 +469,56 @@ static int sd_set_bus_speed_mode(struct
> mmc_card *card, u8 *status)
> >  	 */
> >  	if (!(card->host->caps & (MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 |
> >  	    MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR104 | MMC_CAP_UHS_DDR50)))
> > -		return 0;
> > +		return -EINVAL;

We should not return -EINVAL here, because this is not an error condition. This would simply mean that the card and host controller will fall back to using the default bus speed, and should still be operable. Check the comment below too.

> >
> >  	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;
> > +	}
> > +
> > +	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);
> > +
> > +	if (bus_speed == UHS_SDR104_BUS_SPEED) {
> > +			timing = MMC_TIMING_UHS_SDR104;
> > +			card->sw_caps.uhs_max_dtr = UHS_SDR104_MAX_DTR;
> > +	} else if (bus_speed == UHS_DDR50_BUS_SPEED) {
> > +			timing = MMC_TIMING_UHS_DDR50;
> > +			card->sw_caps.uhs_max_dtr = UHS_DDR50_MAX_DTR;
> > +	} else if (bus_speed == UHS_SDR50_BUS_SPEED) {
> > +			timing = MMC_TIMING_UHS_SDR50;
> > +			card->sw_caps.uhs_max_dtr = UHS_SDR50_MAX_DTR;
> > +	} else if (bus_speed == UHS_SDR25_BUS_SPEED) {
> > +			timing = MMC_TIMING_UHS_SDR25;
> > +			card->sw_caps.uhs_max_dtr = UHS_SDR25_MAX_DTR;
> > +	} else if (bus_speed == UHS_SDR12_BUS_SPEED) {
> >  			timing = MMC_TIMING_UHS_SDR12;
> >  			card->sw_caps.uhs_max_dtr = UHS_SDR12_MAX_DTR;
> > +	} else {
> > +		return -EINVAL;
> >  	}
> 
> Minor change: I think "switch (sd_get_bus_speed_mode(card))" would be
> cleaner.
> 
> >
> >  	card->sd_bus_speed = bus_speed;
> > @@ -522,6 +540,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 +548,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 +632,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);

Now supposing that we return -EINVAL here, then that would imply that mmc_sd_init_uhs_card() failed, but that should not be the case. Like I mentioned before, the card should still be operable using default speed mode.

Thanks,
Arindam

> >  	if (err)
> >  		goto out;
> >
> 
> Thanks,
> 
> - Chris.
> --
> Chris Ball   <cjb@laptop.org>   <http://printf.net/>
> One Laptop Per Child

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

[Subhash Jadavani]

Hi Arindam,

Thank you for the review. Please find few comments inline.

> -----Original Message-----
> From: Nath, Arindam [mailto:Arindam.Nath@amd.com]
> Sent: Tuesday, August 02, 2011 12:38 PM
> To: Chris Ball; Subhash Jadavani
> Cc: linux-mmc@vger.kernel.org; linux-arm-msm@vger.kernel.org; Philip
> Rakity; zhangfei gao
> Subject: RE: [PATCH] mmc: sd: UHS-I bus speed should be set last in UHS
> initialization
> 
> Hi Subhash,
> 
> 
> > -----Original Message-----
> > From: Chris Ball [mailto:cjb@laptop.org]
> > Sent: Tuesday, August 02, 2011 1:40 AM
> > To: Subhash Jadavani
> > Cc: linux-mmc@vger.kernel.org; linux-arm-msm@vger.kernel.org; Nath,
> > Arindam
> > Subject: Re: [PATCH] mmc: sd: UHS-I bus speed should be set last in
> UHS
> > initialization
> >
> > Hi Subhash, adding Arindam for review,
> >
> > On Mon, Aug 01 2011, Subhash Jadavani wrote:
> > > 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@codeaurora.org>
> > > ---
> > >  drivers/mmc/core/sd.c |   57 ++++++++++++++++++++++++++++++++-----
> --
> > ---------
> > >  1 files changed, 38 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> > > index ff27741..d257496 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
> > > @@ -470,37 +469,56 @@ static int sd_set_bus_speed_mode(struct
> > mmc_card *card, u8 *status)
> > >  	 */
> > >  	if (!(card->host->caps & (MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 |
> > >  	    MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR104 | MMC_CAP_UHS_DDR50)))
> > > -		return 0;
> > > +		return -EINVAL;
> 
> We should not return -EINVAL here, because this is not an error
> condition. This would simply mean that the card and host controller
> will fall back to using the default bus speed, and should still be
> operable. Check the comment below too.

Return value of this function gets used in sd_set_bus_speed_mode() function
and compared against all the valid bus speed modes. If we choose to return 0
from here then '0' corresponds to valid bus speed mode
'UHS_SDR12_BUS_SPEED'.

#define UHS_SDR12_BUS_SPEED     0
#define UHS_SDR25_BUS_SPEED     1
#define UHS_SDR50_BUS_SPEED     2
#define UHS_SDR104_BUS_SPEED    3
#define UHS_DDR50_BUS_SPEED     4

If we don't want to return the error value from this function then we can
basically remove following check from this *get* function and move it to
each *set* function before calling *get*. Please see more comments inline in
*set* functions below.

if (!(card->host->caps & (MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 |
	    MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR104 | MMC_CAP_UHS_DDR50)))
		return -EINVAL;

> 
> > >
> > >  	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;
> > > +	}
> > > +
> > > +	return bus_speed;
> > > +}
> > > +
> > > +static int sd_set_bus_speed_mode(struct mmc_card *card, u8
> *status)
> > > +{
> > > +	int err, bus_speed;
> > > +	unsigned int timing = 0;
> > > +

We need add this check here.

	if (!(card->host->caps & (MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 |
  		    MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR104 |
MMC_CAP_UHS_DDR50)))
			return 0;


> > > +	bus_speed = sd_get_bus_speed_mode(card);
> > > +
> > > +	if (bus_speed == UHS_SDR104_BUS_SPEED) {
> > > +			timing = MMC_TIMING_UHS_SDR104;
> > > +			card->sw_caps.uhs_max_dtr = UHS_SDR104_MAX_DTR;
> > > +	} else if (bus_speed == UHS_DDR50_BUS_SPEED) {
> > > +			timing = MMC_TIMING_UHS_DDR50;
> > > +			card->sw_caps.uhs_max_dtr = UHS_DDR50_MAX_DTR;
> > > +	} else if (bus_speed == UHS_SDR50_BUS_SPEED) {
> > > +			timing = MMC_TIMING_UHS_SDR50;
> > > +			card->sw_caps.uhs_max_dtr = UHS_SDR50_MAX_DTR;
> > > +	} else if (bus_speed == UHS_SDR25_BUS_SPEED) {
> > > +			timing = MMC_TIMING_UHS_SDR25;
> > > +			card->sw_caps.uhs_max_dtr = UHS_SDR25_MAX_DTR;
> > > +	} else if (bus_speed == UHS_SDR12_BUS_SPEED) {
> > >  			timing = MMC_TIMING_UHS_SDR12;
> > >  			card->sw_caps.uhs_max_dtr = UHS_SDR12_MAX_DTR;
> > > +	} else {
> > > +		return -EINVAL;
> > >  	}
> >
> > Minor change: I think "switch (sd_get_bus_speed_mode(card))" would be
> > cleaner.

Yes, we can replace it with switch. I will do it next patch rev.

> >
> > >
> > >  	card->sd_bus_speed = bus_speed;
> > > @@ -522,6 +540,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;

We need add this check here.

	if (!(card->host->caps & (MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 |
  		    MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR104 |
MMC_CAP_UHS_DDR50)))
			return 0;

> > >
> > >  	/*
> > > @@ -529,9 +548,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 +632,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);
> 
> Now supposing that we return -EINVAL here, then that would imply that
> mmc_sd_init_uhs_card() failed, but that should not be the case. Like I
> mentioned before, the card should still be operable using default speed
> mode.

When you say Default speed mode, does it mean SDR12 mode?

Thanks,
Subhash

> 
> Thanks,
> Arindam
> 
> > >  	if (err)
> > >  		goto out;
> > >
> >
> > Thanks,
> >
> > - Chris.
> > --
> > Chris Ball   <cjb@laptop.org>   <http://printf.net/>
> > One Laptop Per Child
> 

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

[Nath, Arindam]

> -----Original Message-----
> From: Subhash Jadavani [mailto:subhashj@codeaurora.org]
> Sent: Tuesday, August 02, 2011 2:43 PM
> To: Nath, Arindam; 'Chris Ball'
> Cc: linux-mmc@vger.kernel.org; linux-arm-msm@vger.kernel.org; 'Philip
> Rakity'; 'zhangfei gao'
> Subject: RE: [PATCH] mmc: sd: UHS-I bus speed should be set last in UHS
> initialization
> 
> Hi Arindam,
> 
> Thank you for the review. Please find few comments inline.
> 
> > -----Original Message-----
> > From: Nath, Arindam [mailto:Arindam.Nath@amd.com]
> > Sent: Tuesday, August 02, 2011 12:38 PM
> > To: Chris Ball; Subhash Jadavani
> > Cc: linux-mmc@vger.kernel.org; linux-arm-msm@vger.kernel.org; Philip
> > Rakity; zhangfei gao
> > Subject: RE: [PATCH] mmc: sd: UHS-I bus speed should be set last in
> UHS
> > initialization
> >
> > Hi Subhash,
> >
> >
> > > -----Original Message-----
> > > From: Chris Ball [mailto:cjb@laptop.org]
> > > Sent: Tuesday, August 02, 2011 1:40 AM
> > > To: Subhash Jadavani
> > > Cc: linux-mmc@vger.kernel.org; linux-arm-msm@vger.kernel.org; Nath,
> > > Arindam
> > > Subject: Re: [PATCH] mmc: sd: UHS-I bus speed should be set last in
> > UHS
> > > initialization
> > >
> > > Hi Subhash, adding Arindam for review,
> > >
> > > On Mon, Aug 01 2011, Subhash Jadavani wrote:
> > > > 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@codeaurora.org>
> > > > ---
> > > >  drivers/mmc/core/sd.c |   57 ++++++++++++++++++++++++++++++++---
> --
> > --
> > > ---------
> > > >  1 files changed, 38 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> > > > index ff27741..d257496 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
> > > > @@ -470,37 +469,56 @@ static int sd_set_bus_speed_mode(struct
> > > mmc_card *card, u8 *status)
> > > >  	 */
> > > >  	if (!(card->host->caps & (MMC_CAP_UHS_SDR12 |
> MMC_CAP_UHS_SDR25 |
> > > >  	    MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR104 |
> MMC_CAP_UHS_DDR50)))
> > > > -		return 0;
> > > > +		return -EINVAL;
> >
> > We should not return -EINVAL here, because this is not an error
> > condition. This would simply mean that the card and host controller
> > will fall back to using the default bus speed, and should still be
> > operable. Check the comment below too.
> 
> Return value of this function gets used in sd_set_bus_speed_mode()
> function
> and compared against all the valid bus speed modes. If we choose to
> return 0
> from here then '0' corresponds to valid bus speed mode
> 'UHS_SDR12_BUS_SPEED'.
> 
> #define UHS_SDR12_BUS_SPEED     0
> #define UHS_SDR25_BUS_SPEED     1
> #define UHS_SDR50_BUS_SPEED     2
> #define UHS_SDR104_BUS_SPEED    3
> #define UHS_DDR50_BUS_SPEED     4
> 
> If we don't want to return the error value from this function then we
> can
> basically remove following check from this *get* function and move it
> to
> each *set* function before calling *get*. Please see more comments
> inline in
> *set* functions below.
> 
> if (!(card->host->caps & (MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 |
> 	    MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR104 | MMC_CAP_UHS_DDR50)))
> 		return -EINVAL;

By *Default Bus Speed*, I meant speed corresponding to SDR12. So if we return 0 from sd_get_bus_speed_mode(), that would make sure that your modified sd_set_bus_speed_mode()  correctly executes the last *else if* condition, and we set the default speed. So I don't think we will need to return -EINVAL.

> 
> >
> > > >
> > > >  	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;
> > > > +	}
> > > > +
> > > > +	return bus_speed;
> > > > +}
> > > > +
> > > > +static int sd_set_bus_speed_mode(struct mmc_card *card, u8
> > *status)
> > > > +{
> > > > +	int err, bus_speed;
> > > > +	unsigned int timing = 0;
> > > > +
> 
> We need add this check here.
> 
> 	if (!(card->host->caps & (MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 |
>   		    MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR104 |
> MMC_CAP_UHS_DDR50)))
> 			return 0;

Actually my very first patches returned directly from this function for the above condition. But if I remember correctly, Philip suggested that we should set the default speed too, so the *else if* clause below would take care of that.

> 
> 
> > > > +	bus_speed = sd_get_bus_speed_mode(card);
> > > > +
> > > > +	if (bus_speed == UHS_SDR104_BUS_SPEED) {
> > > > +			timing = MMC_TIMING_UHS_SDR104;
> > > > +			card->sw_caps.uhs_max_dtr = UHS_SDR104_MAX_DTR;
> > > > +	} else if (bus_speed == UHS_DDR50_BUS_SPEED) {
> > > > +			timing = MMC_TIMING_UHS_DDR50;
> > > > +			card->sw_caps.uhs_max_dtr = UHS_DDR50_MAX_DTR;
> > > > +	} else if (bus_speed == UHS_SDR50_BUS_SPEED) {
> > > > +			timing = MMC_TIMING_UHS_SDR50;
> > > > +			card->sw_caps.uhs_max_dtr = UHS_SDR50_MAX_DTR;
> > > > +	} else if (bus_speed == UHS_SDR25_BUS_SPEED) {
> > > > +			timing = MMC_TIMING_UHS_SDR25;
> > > > +			card->sw_caps.uhs_max_dtr = UHS_SDR25_MAX_DTR;
> > > > +	} else if (bus_speed == UHS_SDR12_BUS_SPEED) {
> > > >  			timing = MMC_TIMING_UHS_SDR12;
> > > >  			card->sw_caps.uhs_max_dtr = UHS_SDR12_MAX_DTR;
> > > > +	} else {
> > > > +		return -EINVAL;
> > > >  	}
> > >
> > > Minor change: I think "switch (sd_get_bus_speed_mode(card))" would
> be
> > > cleaner.
> 
> Yes, we can replace it with switch. I will do it next patch rev.
> 
> > >
> > > >
> > > >  	card->sd_bus_speed = bus_speed;
> > > > @@ -522,6 +540,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;
> 
> We need add this check here.
> 
> 	if (!(card->host->caps & (MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 |
>   		    MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR104 |
> MMC_CAP_UHS_DDR50)))
> 			return 0;

Setting current limit is little different. As per section 4.3.10.3 of the Physical Layer Spec v3.01, Current Limit is only set for SDR50, SDR104, and DDR50 modes. For SDR12 and SDR25, we need to set the current limit of 200mA. So I don't think we should directly return from here.

> 
> > > >
> > > >  	/*
> > > > @@ -529,9 +548,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 +632,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);
> >
> > Now supposing that we return -EINVAL here, then that would imply that
> > mmc_sd_init_uhs_card() failed, but that should not be the case. Like
> I
> > mentioned before, the card should still be operable using default
> speed
> > mode.
> 
> When you say Default speed mode, does it mean SDR12 mode?

Yes.

> 
> Thanks,
> Subhash
> 
> >
> > Thanks,
> > Arindam
> >
> > > >  	if (err)
> > > >  		goto out;
> > > >
> > >
> > > Thanks,
> > >
> > > - Chris.
> > > --
> > > Chris Ball   <cjb@laptop.org>   <http://printf.net/>
> > > One Laptop Per Child
> >
> 
> 

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

[Subhash Jadavani]

> -----Original Message-----
> From: linux-arm-msm-owner@vger.kernel.org [mailto:linux-arm-msm-
> owner@vger.kernel.org] On Behalf Of Nath, Arindam
> Sent: Tuesday, August 02, 2011 3:21 PM
> To: Subhash Jadavani; 'Chris Ball'
> Cc: linux-mmc@vger.kernel.org; linux-arm-msm@vger.kernel.org; 'Philip
> Rakity'; 'zhangfei gao'
> Subject: RE: [PATCH] mmc: sd: UHS-I bus speed should be set last in UHS
> initialization
> 
> 
> 
> > -----Original Message-----
> > From: Subhash Jadavani [mailto:subhashj@codeaurora.org]
> > Sent: Tuesday, August 02, 2011 2:43 PM
> > To: Nath, Arindam; 'Chris Ball'
> > Cc: linux-mmc@vger.kernel.org; linux-arm-msm@vger.kernel.org; 'Philip
> > Rakity'; 'zhangfei gao'
> > Subject: RE: [PATCH] mmc: sd: UHS-I bus speed should be set last in
> UHS
> > initialization
> >
> > Hi Arindam,
> >
> > Thank you for the review. Please find few comments inline.
> >
> > > -----Original Message-----
> > > From: Nath, Arindam [mailto:Arindam.Nath@amd.com]
> > > Sent: Tuesday, August 02, 2011 12:38 PM
> > > To: Chris Ball; Subhash Jadavani
> > > Cc: linux-mmc@vger.kernel.org; linux-arm-msm@vger.kernel.org;
> Philip
> > > Rakity; zhangfei gao
> > > Subject: RE: [PATCH] mmc: sd: UHS-I bus speed should be set last in
> > UHS
> > > initialization
> > >
> > > Hi Subhash,
> > >
> > >
> > > > -----Original Message-----
> > > > From: Chris Ball [mailto:cjb@laptop.org]
> > > > Sent: Tuesday, August 02, 2011 1:40 AM
> > > > To: Subhash Jadavani
> > > > Cc: linux-mmc@vger.kernel.org; linux-arm-msm@vger.kernel.org;
> Nath,
> > > > Arindam
> > > > Subject: Re: [PATCH] mmc: sd: UHS-I bus speed should be set last
> in
> > > UHS
> > > > initialization
> > > >
> > > > Hi Subhash, adding Arindam for review,
> > > >
> > > > On Mon, Aug 01 2011, Subhash Jadavani wrote:
> > > > > 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@codeaurora.org>
> > > > > ---
> > > > >  drivers/mmc/core/sd.c |   57 ++++++++++++++++++++++++++++++++-
> --
> > --
> > > --
> > > > ---------
> > > > >  1 files changed, 38 insertions(+), 19 deletions(-)
> > > > >
> > > > > diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> > > > > index ff27741..d257496 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
> > > > > @@ -470,37 +469,56 @@ static int sd_set_bus_speed_mode(struct
> > > > mmc_card *card, u8 *status)
> > > > >  	 */
> > > > >  	if (!(card->host->caps & (MMC_CAP_UHS_SDR12 |
> > MMC_CAP_UHS_SDR25 |
> > > > >  	    MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR104 |
> > MMC_CAP_UHS_DDR50)))
> > > > > -		return 0;
> > > > > +		return -EINVAL;
> > >
> > > We should not return -EINVAL here, because this is not an error
> > > condition. This would simply mean that the card and host controller
> > > will fall back to using the default bus speed, and should still be
> > > operable. Check the comment below too.
> >
> > Return value of this function gets used in sd_set_bus_speed_mode()
> > function
> > and compared against all the valid bus speed modes. If we choose to
> > return 0
> > from here then '0' corresponds to valid bus speed mode
> > 'UHS_SDR12_BUS_SPEED'.
> >
> > #define UHS_SDR12_BUS_SPEED     0
> > #define UHS_SDR25_BUS_SPEED     1
> > #define UHS_SDR50_BUS_SPEED     2
> > #define UHS_SDR104_BUS_SPEED    3
> > #define UHS_DDR50_BUS_SPEED     4
> >
> > If we don't want to return the error value from this function then we
> > can
> > basically remove following check from this *get* function and move it
> > to
> > each *set* function before calling *get*. Please see more comments
> > inline in
> > *set* functions below.
> >
> > if (!(card->host->caps & (MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 |
> > 	    MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR104 | MMC_CAP_UHS_DDR50)))
> > 		return -EINVAL;
> 
> By *Default Bus Speed*, I meant speed corresponding to SDR12. So if we
> return 0 from sd_get_bus_speed_mode(), that would make sure that your
> modified sd_set_bus_speed_mode()  correctly executes the last *else if*
> condition, and we set the default speed. So I don't think we will need
> to return -EINVAL.

Ok. Got your points. Will address this in next rev. for this patch.

Thanks,
Subhash

> 
> >
> > >
> > > > >
> > > > >  	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;
> > > > > +	}
> > > > > +
> > > > > +	return bus_speed;
> > > > > +}
> > > > > +
> > > > > +static int sd_set_bus_speed_mode(struct mmc_card *card, u8
> > > *status)
> > > > > +{
> > > > > +	int err, bus_speed;
> > > > > +	unsigned int timing = 0;
> > > > > +
> >
> > We need add this check here.
> >
> > 	if (!(card->host->caps & (MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 |
> >   		    MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR104 |
> > MMC_CAP_UHS_DDR50)))
> > 			return 0;
> 
> Actually my very first patches returned directly from this function for
> the above condition. But if I remember correctly, Philip suggested that
> we should set the default speed too, so the *else if* clause below
> would take care of that.
> 
> >
> >
> > > > > +	bus_speed = sd_get_bus_speed_mode(card);
> > > > > +
> > > > > +	if (bus_speed == UHS_SDR104_BUS_SPEED) {
> > > > > +			timing = MMC_TIMING_UHS_SDR104;
> > > > > +			card->sw_caps.uhs_max_dtr =
UHS_SDR104_MAX_DTR;
> > > > > +	} else if (bus_speed == UHS_DDR50_BUS_SPEED) {
> > > > > +			timing = MMC_TIMING_UHS_DDR50;
> > > > > +			card->sw_caps.uhs_max_dtr =
UHS_DDR50_MAX_DTR;
> > > > > +	} else if (bus_speed == UHS_SDR50_BUS_SPEED) {
> > > > > +			timing = MMC_TIMING_UHS_SDR50;
> > > > > +			card->sw_caps.uhs_max_dtr =
UHS_SDR50_MAX_DTR;
> > > > > +	} else if (bus_speed == UHS_SDR25_BUS_SPEED) {
> > > > > +			timing = MMC_TIMING_UHS_SDR25;
> > > > > +			card->sw_caps.uhs_max_dtr =
UHS_SDR25_MAX_DTR;
> > > > > +	} else if (bus_speed == UHS_SDR12_BUS_SPEED) {
> > > > >  			timing = MMC_TIMING_UHS_SDR12;
> > > > >  			card->sw_caps.uhs_max_dtr =
UHS_SDR12_MAX_DTR;
> > > > > +	} else {
> > > > > +		return -EINVAL;
> > > > >  	}
> > > >
> > > > Minor change: I think "switch (sd_get_bus_speed_mode(card))"
> would
> > be
> > > > cleaner.
> >
> > Yes, we can replace it with switch. I will do it next patch rev.
> >
> > > >
> > > > >
> > > > >  	card->sd_bus_speed = bus_speed;
> > > > > @@ -522,6 +540,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;
> >
> > We need add this check here.
> >
> > 	if (!(card->host->caps & (MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 |
> >   		    MMC_CAP_UHS_SDR50 | MMC_CAP_UHS_SDR104 |
> > MMC_CAP_UHS_DDR50)))
> > 			return 0;
> 
> Setting current limit is little different. As per section 4.3.10.3 of
> the Physical Layer Spec v3.01, Current Limit is only set for SDR50,
> SDR104, and DDR50 modes. For SDR12 and SDR25, we need to set the
> current limit of 200mA. So I don't think we should directly return from
> here.
> 
> >
> > > > >
> > > > >  	/*
> > > > > @@ -529,9 +548,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 +632,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);
> > >
> > > Now supposing that we return -EINVAL here, then that would imply
> that
> > > mmc_sd_init_uhs_card() failed, but that should not be the case.
> Like
> > I
> > > mentioned before, the card should still be operable using default
> > speed
> > > mode.
> >
> > When you say Default speed mode, does it mean SDR12 mode?
> 
> Yes.
> 
> >
> > Thanks,
> > Subhash
> >
> > >
> > > Thanks,
> > > Arindam
> > >
> > > > >  	if (err)
> > > > >  		goto out;
> > > > >
> > > >
> > > > Thanks,
> > > >
> > > > - Chris.
> > > > --
> > > > Chris Ball   <cjb@laptop.org>   <http://printf.net/>
> > > > One Laptop Per Child
> > >
> >
> >
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-
> msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Issue with non-UHS-I SD3.0 cards

[Subhash Jadavani]

Hi,

I am seeing the issue where "SanDisk Extreme SDHC 8GB Class 10 card" is
running in "Default Speed" mode even though it supports "High Speed" mode.
After analysis, I found that this card is advertising itself as SD3.0 card
(with SD_SPEC=2 and SD_SPEC3=1 in SCR). But current mmc core later
initialization sequence for SD3.0 cards is such that these non-UHS-I SD3.0
cards runs in Default Speed mode @25MHz.

Here is Essential conditions to indicate Version 3.00 Card (from SD3.01
spec)
    (SD_SPEC=2 and SD_SPEC3=1) :
    (1) The card shall support CMD6
    (2) The card shall support CMD8
    (3) The card shall support CMD42
    (4) User area capacity shall be up to 2GB (SDSC) or 32GB (SDHC)
        User area capacity shall be more than or equal to 32GB and
        up to 2TB (SDXC)
    (5) Speed Class shall be supported (SDHC or SDXC)


So even if SD card doesn't support any of the newly defined UHS-I bus speed
mode, it can advertise itself as SD3.0 cards as long as it supports all the
essential conditions of SD3.0 cards. Given this, these type of cards should
at least run in High Speed mode @50MHZ if it supports HS.

Here are the changes you have to do to make non-UHS-I SD3.0 cards run in
High Speed Mode @50MHz. Do let me know if looks fine or not, so I can post
the formal patch.

---
 drivers/mmc/core/sd.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 907662a..3fab676 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -305,6 +305,9 @@ static int mmc_read_switch(struct mmc_card *card)
                goto out;
        }

+       if (status[13] & 0x02)
+               card->sw_caps.hs_max_dtr = 50000000;
+
        if (card->scr.sda_spec3) {
                card->sw_caps.sd3_bus_mode = status[13];

@@ -347,9 +350,6 @@ static int mmc_read_switch(struct mmc_card *card)
                }

                card->sw_caps.sd3_curr_limit = status[7];
-       } else {
-               if (status[13] & 0x02)
-                       card->sw_caps.hs_max_dtr = 50000000;
        }

 out:
--


Regards,
Subhash

RE: Issue with non-UHS-I SD3.0 cards

[Nath, Arindam]

Hi Subhash,


> -----Original Message-----
> From: Subhash Jadavani [mailto:subhashj@codeaurora.org]
> Sent: Friday, August 05, 2011 7:01 PM
> To: linux-mmc@vger.kernel.org
> Cc: Nath, Arindam; 'Chris Ball'; 'Philip Rakity'; 'zhangfei gao'
> Subject: Issue with non-UHS-I SD3.0 cards
> 
> Hi,
> 
> I am seeing the issue where "SanDisk Extreme SDHC 8GB Class 10 card" is
> running in "Default Speed" mode even though it supports "High Speed"
> mode.
> After analysis, I found that this card is advertising itself as SD3.0
> card
> (with SD_SPEC=2 and SD_SPEC3=1 in SCR). But current mmc core later
> initialization sequence for SD3.0 cards is such that these non-UHS-I
> SD3.0
> cards runs in Default Speed mode @25MHz.
> 
> Here is Essential conditions to indicate Version 3.00 Card (from SD3.01
> spec)
>     (SD_SPEC=2 and SD_SPEC3=1) :
>     (1) The card shall support CMD6
>     (2) The card shall support CMD8
>     (3) The card shall support CMD42
>     (4) User area capacity shall be up to 2GB (SDSC) or 32GB (SDHC)
>         User area capacity shall be more than or equal to 32GB and
>         up to 2TB (SDXC)
>     (5) Speed Class shall be supported (SDHC or SDXC)

I went through the particular section of the spec again, and IMO, the essential conditions are for the manufacturer of the card to advertise his card as SD3.0 card or otherwise. From software point of view, the SD_SPEC=2 _and_ SD_SPEC3=1 will indicate a SD3.0 card.

Thanks,
Arindam

> 
> 
> So even if SD card doesn't support any of the newly defined UHS-I bus
> speed
> mode, it can advertise itself as SD3.0 cards as long as it supports all
> the
> essential conditions of SD3.0 cards. Given this, these type of cards
> should
> at least run in High Speed mode @50MHZ if it supports HS.
> 
> Here are the changes you have to do to make non-UHS-I SD3.0 cards run
> in
> High Speed Mode @50MHz. Do let me know if looks fine or not, so I can
> post
> the formal patch.
> 
> ---
>  drivers/mmc/core/sd.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 907662a..3fab676 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -305,6 +305,9 @@ static int mmc_read_switch(struct mmc_card *card)
>                 goto out;
>         }
> 
> +       if (status[13] & 0x02)
> +               card->sw_caps.hs_max_dtr = 50000000;
> +
>         if (card->scr.sda_spec3) {
>                 card->sw_caps.sd3_bus_mode = status[13];
> 
> @@ -347,9 +350,6 @@ static int mmc_read_switch(struct mmc_card *card)
>                 }
> 
>                 card->sw_caps.sd3_curr_limit = status[7];
> -       } else {
> -               if (status[13] & 0x02)
> -                       card->sw_caps.hs_max_dtr = 50000000;
>         }
> 
>  out:
> --
> 
> 
> Regards,
> Subhash
> 
> 

RE: Issue with non-UHS-I SD3.0 cards

[Nath, Arindam]

> -----Original Message-----
> From: Nath, Arindam
> Sent: Saturday, August 06, 2011 12:15 AM
> To: 'Subhash Jadavani'; linux-mmc@vger.kernel.org
> Cc: 'Chris Ball'; 'Philip Rakity'; 'zhangfei gao'
> Subject: RE: Issue with non-UHS-I SD3.0 cards
> 
> Hi Subhash,
> 
> 
> > -----Original Message-----
> > From: Subhash Jadavani [mailto:subhashj@codeaurora.org]
> > Sent: Friday, August 05, 2011 7:01 PM
> > To: linux-mmc@vger.kernel.org
> > Cc: Nath, Arindam; 'Chris Ball'; 'Philip Rakity'; 'zhangfei gao'
> > Subject: Issue with non-UHS-I SD3.0 cards
> >
> > Hi,
> >
> > I am seeing the issue where "SanDisk Extreme SDHC 8GB Class 10 card"
> is
> > running in "Default Speed" mode even though it supports "High Speed"
> > mode.
> > After analysis, I found that this card is advertising itself as SD3.0
> > card
> > (with SD_SPEC=2 and SD_SPEC3=1 in SCR). But current mmc core later
> > initialization sequence for SD3.0 cards is such that these non-UHS-I
> > SD3.0
> > cards runs in Default Speed mode @25MHz.
> >
> > Here is Essential conditions to indicate Version 3.00 Card (from
> SD3.01
> > spec)
> >     (SD_SPEC=2 and SD_SPEC3=1) :
> >     (1) The card shall support CMD6
> >     (2) The card shall support CMD8
> >     (3) The card shall support CMD42
> >     (4) User area capacity shall be up to 2GB (SDSC) or 32GB (SDHC)
> >         User area capacity shall be more than or equal to 32GB and
> >         up to 2TB (SDXC)
> >     (5) Speed Class shall be supported (SDHC or SDXC)
> 
> I went through the particular section of the spec again, and IMO, the
> essential conditions are for the manufacturer of the card to advertise
> his card as SD3.0 card or otherwise. From software point of view, the
> SD_SPEC=2 _and_ SD_SPEC3=1 will indicate a SD3.0 card.

I misread your comment, so please ignore what you mentioned above. So if I understand correctly, in your case *sd_spec3* is set for the card, even though it is a non UHS-I card. In that case, what you tried in your patch looks fine, except that I would suggest to move it inside the

if (card->scr.sda_spec3) {
	...
}

condition.

> 
> Thanks,
> Arindam
> 
> >
> >
> > So even if SD card doesn't support any of the newly defined UHS-I bus
> > speed
> > mode, it can advertise itself as SD3.0 cards as long as it supports
> all
> > the
> > essential conditions of SD3.0 cards. Given this, these type of cards
> > should
> > at least run in High Speed mode @50MHZ if it supports HS.
> >
> > Here are the changes you have to do to make non-UHS-I SD3.0 cards run
> > in
> > High Speed Mode @50MHz. Do let me know if looks fine or not, so I can
> > post
> > the formal patch.
> >
> > ---
> >  drivers/mmc/core/sd.c |    6 +++---
> >  1 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> > index 907662a..3fab676 100644
> > --- a/drivers/mmc/core/sd.c
> > +++ b/drivers/mmc/core/sd.c
> > @@ -305,6 +305,9 @@ static int mmc_read_switch(struct mmc_card *card)
> >                 goto out;
> >         }
> >
> > +       if (status[13] & 0x02)
> > +               card->sw_caps.hs_max_dtr = 50000000;
> > +
> >         if (card->scr.sda_spec3) {
> >                 card->sw_caps.sd3_bus_mode = status[13];
> >
> > @@ -347,9 +350,6 @@ static int mmc_read_switch(struct mmc_card *card)
> >                 }
> >
> >                 card->sw_caps.sd3_curr_limit = status[7];
> > -       } else {
> > -               if (status[13] & 0x02)
> > -                       card->sw_caps.hs_max_dtr = 50000000;
> >         }
> >
> >  out:
> > --
> >
> >
> > Regards,
> > Subhash
> >
> >

RE: Issue with non-UHS-I SD3.0 cards

[Nath, Arindam]

> -----Original Message-----
> From: Nath, Arindam
> Sent: Saturday, August 06, 2011 12:27 AM
> To: 'Subhash Jadavani'; 'linux-mmc@vger.kernel.org'
> Cc: 'Chris Ball'; 'Philip Rakity'; 'zhangfei gao'
> Subject: RE: Issue with non-UHS-I SD3.0 cards
> 
> 
> 
> > -----Original Message-----
> > From: Nath, Arindam
> > Sent: Saturday, August 06, 2011 12:15 AM
> > To: 'Subhash Jadavani'; linux-mmc@vger.kernel.org
> > Cc: 'Chris Ball'; 'Philip Rakity'; 'zhangfei gao'
> > Subject: RE: Issue with non-UHS-I SD3.0 cards
> >
> > Hi Subhash,
> >
> >
> > > -----Original Message-----
> > > From: Subhash Jadavani [mailto:subhashj@codeaurora.org]
> > > Sent: Friday, August 05, 2011 7:01 PM
> > > To: linux-mmc@vger.kernel.org
> > > Cc: Nath, Arindam; 'Chris Ball'; 'Philip Rakity'; 'zhangfei gao'
> > > Subject: Issue with non-UHS-I SD3.0 cards
> > >
> > > Hi,
> > >
> > > I am seeing the issue where "SanDisk Extreme SDHC 8GB Class 10
> card"
> > is
> > > running in "Default Speed" mode even though it supports "High
> Speed"
> > > mode.
> > > After analysis, I found that this card is advertising itself as
> SD3.0
> > > card
> > > (with SD_SPEC=2 and SD_SPEC3=1 in SCR). But current mmc core later
> > > initialization sequence for SD3.0 cards is such that these non-UHS-
> I
> > > SD3.0
> > > cards runs in Default Speed mode @25MHz.
> > >
> > > Here is Essential conditions to indicate Version 3.00 Card (from
> > SD3.01
> > > spec)
> > >     (SD_SPEC=2 and SD_SPEC3=1) :
> > >     (1) The card shall support CMD6
> > >     (2) The card shall support CMD8
> > >     (3) The card shall support CMD42
> > >     (4) User area capacity shall be up to 2GB (SDSC) or 32GB (SDHC)
> > >         User area capacity shall be more than or equal to 32GB and
> > >         up to 2TB (SDXC)
> > >     (5) Speed Class shall be supported (SDHC or SDXC)
> >
> > I went through the particular section of the spec again, and IMO, the
> > essential conditions are for the manufacturer of the card to
> advertise
> > his card as SD3.0 card or otherwise. From software point of view, the
> > SD_SPEC=2 _and_ SD_SPEC3=1 will indicate a SD3.0 card.
> 
> I misread your comment, so please ignore what you mentioned above. So

*so please ignore what I mentioned above*

Sorry for the typo.

> if I understand correctly, in your case *sd_spec3* is set for the card,
> even though it is a non UHS-I card. In that case, what you tried in
> your patch looks fine, except that I would suggest to move it inside
> the
> 
> if (card->scr.sda_spec3) {
> 	...
> }
> 
> condition.
> 
> >
> > Thanks,
> > Arindam
> >
> > >
> > >
> > > So even if SD card doesn't support any of the newly defined UHS-I
> bus
> > > speed
> > > mode, it can advertise itself as SD3.0 cards as long as it supports
> > all
> > > the
> > > essential conditions of SD3.0 cards. Given this, these type of
> cards
> > > should
> > > at least run in High Speed mode @50MHZ if it supports HS.
> > >
> > > Here are the changes you have to do to make non-UHS-I SD3.0 cards
> run
> > > in
> > > High Speed Mode @50MHz. Do let me know if looks fine or not, so I
> can
> > > post
> > > the formal patch.
> > >
> > > ---
> > >  drivers/mmc/core/sd.c |    6 +++---
> > >  1 files changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> > > index 907662a..3fab676 100644
> > > --- a/drivers/mmc/core/sd.c
> > > +++ b/drivers/mmc/core/sd.c
> > > @@ -305,6 +305,9 @@ static int mmc_read_switch(struct mmc_card
> *card)
> > >                 goto out;
> > >         }
> > >
> > > +       if (status[13] & 0x02)
> > > +               card->sw_caps.hs_max_dtr = 50000000;
> > > +
> > >         if (card->scr.sda_spec3) {
> > >                 card->sw_caps.sd3_bus_mode = status[13];
> > >
> > > @@ -347,9 +350,6 @@ static int mmc_read_switch(struct mmc_card
> *card)
> > >                 }
> > >
> > >                 card->sw_caps.sd3_curr_limit = status[7];
> > > -       } else {
> > > -               if (status[13] & 0x02)
> > > -                       card->sw_caps.hs_max_dtr = 50000000;
> > >         }
> > >
> > >  out:
> > > --
> > >
> > >
> > > Regards,
> > > Subhash
> > >
> > >

RE: Issue with non-UHS-I SD3.0 cards

[Subhash Jadavani]

Hi Arindam,

> -----Original Message-----
> From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-
> owner@vger.kernel.org] On Behalf Of Nath, Arindam
> Sent: Saturday, August 06, 2011 12:27 AM
> To: Subhash Jadavani; linux-mmc@vger.kernel.org
> Cc: 'Chris Ball'; 'Philip Rakity'; 'zhangfei gao'
> Subject: RE: Issue with non-UHS-I SD3.0 cards
> 
> 
> 
> > -----Original Message-----
> > From: Nath, Arindam
> > Sent: Saturday, August 06, 2011 12:15 AM
> > To: 'Subhash Jadavani'; linux-mmc@vger.kernel.org
> > Cc: 'Chris Ball'; 'Philip Rakity'; 'zhangfei gao'
> > Subject: RE: Issue with non-UHS-I SD3.0 cards
> >
> > Hi Subhash,
> >
> >
> > > -----Original Message-----
> > > From: Subhash Jadavani [mailto:subhashj@codeaurora.org]
> > > Sent: Friday, August 05, 2011 7:01 PM
> > > To: linux-mmc@vger.kernel.org
> > > Cc: Nath, Arindam; 'Chris Ball'; 'Philip Rakity'; 'zhangfei gao'
> > > Subject: Issue with non-UHS-I SD3.0 cards
> > >
> > > Hi,
> > >
> > > I am seeing the issue where "SanDisk Extreme SDHC 8GB Class 10
> card"
> > is
> > > running in "Default Speed" mode even though it supports "High
> Speed"
> > > mode.
> > > After analysis, I found that this card is advertising itself as
> SD3.0
> > > card
> > > (with SD_SPEC=2 and SD_SPEC3=1 in SCR). But current mmc core later
> > > initialization sequence for SD3.0 cards is such that these non-UHS-
> I
> > > SD3.0
> > > cards runs in Default Speed mode @25MHz.
> > >
> > > Here is Essential conditions to indicate Version 3.00 Card (from
> > SD3.01
> > > spec)
> > >     (SD_SPEC=2 and SD_SPEC3=1) :
> > >     (1) The card shall support CMD6
> > >     (2) The card shall support CMD8
> > >     (3) The card shall support CMD42
> > >     (4) User area capacity shall be up to 2GB (SDSC) or 32GB (SDHC)
> > >         User area capacity shall be more than or equal to 32GB and
> > >         up to 2TB (SDXC)
> > >     (5) Speed Class shall be supported (SDHC or SDXC)
> >
> > I went through the particular section of the spec again, and IMO, the
> > essential conditions are for the manufacturer of the card to
> advertise
> > his card as SD3.0 card or otherwise. From software point of view, the
> > SD_SPEC=2 _and_ SD_SPEC3=1 will indicate a SD3.0 card.
> 
> I misread your comment, so please ignore what you mentioned above. So
> if I understand correctly, in your case *sd_spec3* is set for the card,
> even though it is a non UHS-I card. In that case, what you tried in
> your patch looks fine, except that I would suggest to move it inside
> the
> 
> if (card->scr.sda_spec3) {
> 	...
> }
> 
> condition.
> 

You mean moving the " card->sw_caps.hs_max_dtr = 50000000;" under if
(card->scr.sda_spec3) condition and still retaining the else condition for "
if (card->scr.sda_spec3) ".

Something like this:

if (card->scr.sda_spec3) {
       if (status[13] & 0x02)
               card->sw_caps.hs_max_dtr = 50000000;
	...
	...
	
} else {
       if (status[13] & 0x02)
               card->sw_caps.hs_max_dtr = 50000000;
}

I thought it's duplication of code as both if & else condition is executing
the same code so thought of putting it outside of if, else. 
I am even ok with putting it inside "if (card->scr.sda_spec3)", if you think
it's better.

Regards,
Subhash


> >
> > Thanks,
> > Arindam
> >
> > >
> > >
> > > So even if SD card doesn't support any of the newly defined UHS-I
> bus
> > > speed
> > > mode, it can advertise itself as SD3.0 cards as long as it supports
> > all
> > > the
> > > essential conditions of SD3.0 cards. Given this, these type of
> cards
> > > should
> > > at least run in High Speed mode @50MHZ if it supports HS.
> > >
> > > Here are the changes you have to do to make non-UHS-I SD3.0 cards
> run
> > > in
> > > High Speed Mode @50MHz. Do let me know if looks fine or not, so I
> can
> > > post
> > > the formal patch.
> > >
> > > ---
> > >  drivers/mmc/core/sd.c |    6 +++---
> > >  1 files changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> > > index 907662a..3fab676 100644
> > > --- a/drivers/mmc/core/sd.c
> > > +++ b/drivers/mmc/core/sd.c
> > > @@ -305,6 +305,9 @@ static int mmc_read_switch(struct mmc_card
> *card)
> > >                 goto out;
> > >         }
> > >
> > > +       if (status[13] & 0x02)
> > > +               card->sw_caps.hs_max_dtr = 50000000;
> > > +
> > >         if (card->scr.sda_spec3) {
> > >                 card->sw_caps.sd3_bus_mode = status[13];
> > >
> > > @@ -347,9 +350,6 @@ static int mmc_read_switch(struct mmc_card
> *card)
> > >                 }
> > >
> > >                 card->sw_caps.sd3_curr_limit = status[7];
> > > -       } else {
> > > -               if (status[13] & 0x02)
> > > -                       card->sw_caps.hs_max_dtr = 50000000;
> > >         }
> > >
> > >  out:
> > > --
> > >
> > >
> > > Regards,
> > > Subhash
> > >
> > >
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: Issue with non-UHS-I SD3.0 cards

[Nath, Arindam]

Hi Subhash,


> -----Original Message-----
> From: Subhash Jadavani [mailto:subhashj@codeaurora.org]
> Sent: Monday, August 08, 2011 11:51 AM
> To: Nath, Arindam; linux-mmc@vger.kernel.org
> Cc: 'Chris Ball'; 'Philip Rakity'; 'zhangfei gao'
> Subject: RE: Issue with non-UHS-I SD3.0 cards
> 
> Hi Arindam,
> 
> > -----Original Message-----
> > From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-
> > owner@vger.kernel.org] On Behalf Of Nath, Arindam
> > Sent: Saturday, August 06, 2011 12:27 AM
> > To: Subhash Jadavani; linux-mmc@vger.kernel.org
> > Cc: 'Chris Ball'; 'Philip Rakity'; 'zhangfei gao'
> > Subject: RE: Issue with non-UHS-I SD3.0 cards
> >
> >
> >
> > > -----Original Message-----
> > > From: Nath, Arindam
> > > Sent: Saturday, August 06, 2011 12:15 AM
> > > To: 'Subhash Jadavani'; linux-mmc@vger.kernel.org
> > > Cc: 'Chris Ball'; 'Philip Rakity'; 'zhangfei gao'
> > > Subject: RE: Issue with non-UHS-I SD3.0 cards
> > >
> > > Hi Subhash,
> > >
> > >
> > > > -----Original Message-----
> > > > From: Subhash Jadavani [mailto:subhashj@codeaurora.org]
> > > > Sent: Friday, August 05, 2011 7:01 PM
> > > > To: linux-mmc@vger.kernel.org
> > > > Cc: Nath, Arindam; 'Chris Ball'; 'Philip Rakity'; 'zhangfei gao'
> > > > Subject: Issue with non-UHS-I SD3.0 cards
> > > >
> > > > Hi,
> > > >
> > > > I am seeing the issue where "SanDisk Extreme SDHC 8GB Class 10
> > card"
> > > is
> > > > running in "Default Speed" mode even though it supports "High
> > Speed"
> > > > mode.
> > > > After analysis, I found that this card is advertising itself as
> > SD3.0
> > > > card
> > > > (with SD_SPEC=2 and SD_SPEC3=1 in SCR). But current mmc core
> later
> > > > initialization sequence for SD3.0 cards is such that these non-
> UHS-
> > I
> > > > SD3.0
> > > > cards runs in Default Speed mode @25MHz.
> > > >
> > > > Here is Essential conditions to indicate Version 3.00 Card (from
> > > SD3.01
> > > > spec)
> > > >     (SD_SPEC=2 and SD_SPEC3=1) :
> > > >     (1) The card shall support CMD6
> > > >     (2) The card shall support CMD8
> > > >     (3) The card shall support CMD42
> > > >     (4) User area capacity shall be up to 2GB (SDSC) or 32GB
> (SDHC)
> > > >         User area capacity shall be more than or equal to 32GB
> and
> > > >         up to 2TB (SDXC)
> > > >     (5) Speed Class shall be supported (SDHC or SDXC)
> > >
> > > I went through the particular section of the spec again, and IMO,
> the
> > > essential conditions are for the manufacturer of the card to
> > advertise
> > > his card as SD3.0 card or otherwise. From software point of view,
> the
> > > SD_SPEC=2 _and_ SD_SPEC3=1 will indicate a SD3.0 card.
> >
> > I misread your comment, so please ignore what you mentioned above. So
> > if I understand correctly, in your case *sd_spec3* is set for the
> card,
> > even though it is a non UHS-I card. In that case, what you tried in
> > your patch looks fine, except that I would suggest to move it inside
> > the
> >
> > if (card->scr.sda_spec3) {
> > 	...
> > }
> >
> > condition.
> >
> 
> You mean moving the " card->sw_caps.hs_max_dtr = 50000000;" under if
> (card->scr.sda_spec3) condition and still retaining the else condition
> for "
> if (card->scr.sda_spec3) ".
> 
> Something like this:
> 
> if (card->scr.sda_spec3) {
>        if (status[13] & 0x02)
>                card->sw_caps.hs_max_dtr = 50000000;
> 	...
> 	...
> 
> } else {
>        if (status[13] & 0x02)
>                card->sw_caps.hs_max_dtr = 50000000;
> }
> 
> I thought it's duplication of code as both if & else condition is
> executing
> the same code so thought of putting it outside of if, else.
> I am even ok with putting it inside "if (card->scr.sda_spec3)", if you
> think
> it's better.

Your argument makes sense. I am OK with your original patch.

Reviewed-by: Arindam Nath <arindam.nath@amd.com>

> 
> Regards,
> Subhash
> 
> 
> > >
> > > Thanks,
> > > Arindam
> > >
> > > >
> > > >
> > > > So even if SD card doesn't support any of the newly defined UHS-I
> > bus
> > > > speed
> > > > mode, it can advertise itself as SD3.0 cards as long as it
> supports
> > > all
> > > > the
> > > > essential conditions of SD3.0 cards. Given this, these type of
> > cards
> > > > should
> > > > at least run in High Speed mode @50MHZ if it supports HS.
> > > >
> > > > Here are the changes you have to do to make non-UHS-I SD3.0 cards
> > run
> > > > in
> > > > High Speed Mode @50MHz. Do let me know if looks fine or not, so I
> > can
> > > > post
> > > > the formal patch.
> > > >
> > > > ---
> > > >  drivers/mmc/core/sd.c |    6 +++---
> > > >  1 files changed, 3 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> > > > index 907662a..3fab676 100644
> > > > --- a/drivers/mmc/core/sd.c
> > > > +++ b/drivers/mmc/core/sd.c
> > > > @@ -305,6 +305,9 @@ static int mmc_read_switch(struct mmc_card
> > *card)
> > > >                 goto out;
> > > >         }
> > > >
> > > > +       if (status[13] & 0x02)
> > > > +               card->sw_caps.hs_max_dtr = 50000000;
> > > > +
> > > >         if (card->scr.sda_spec3) {
> > > >                 card->sw_caps.sd3_bus_mode = status[13];
> > > >
> > > > @@ -347,9 +350,6 @@ static int mmc_read_switch(struct mmc_card
> > *card)
> > > >                 }
> > > >
> > > >                 card->sw_caps.sd3_curr_limit = status[7];
> > > > -       } else {
> > > > -               if (status[13] & 0x02)
> > > > -                       card->sw_caps.hs_max_dtr = 50000000;
> > > >         }
> > > >
> > > >  out:
> > > > --
> > > >
> > > >
> > > > Regards,
> > > > Subhash
> > > >
> > > >
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-mmc"
> in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>