* [PATCH 1/2] mmc: core: fix the decision of mmc card-type
@ 2012-04-23 9:11 Seungwon Jeon
2012-04-24 8:02 ` Subhash Jadavani
0 siblings, 1 reply; 4+ messages in thread
From: Seungwon Jeon @ 2012-04-23 9:11 UTC (permalink / raw)
To: linux-mmc; +Cc: 'Chris Ball', 'Girish K S'
Current implementation decides the card type exclusively. Even though
eMMC device can support both HS200 and DDR mode, card type will be
set only for HS200. If the host doesn't support HS200 but has DDR
capability, then DDR mode can't be selected.
Signed-off-by: Seungwon Jeon <tgih.jun@samsung.com>
---
drivers/mmc/core/mmc.c | 85 +++++++++++++++++++--------------------------
include/linux/mmc/card.h | 4 ++
include/linux/mmc/mmc.h | 60 --------------------------------
3 files changed, 40 insertions(+), 109 deletions(-)
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 54df5ad..ebb9522 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -235,6 +235,40 @@ static int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd)
return err;
}
+static void mmc_select_card_type(struct mmc_card *card)
+{
+ struct mmc_host *host = card->host;
+ u8 card_type = card->ext_csd.raw_card_type & EXT_CSD_CARD_TYPE_MASK;
+ unsigned int caps = host->caps, caps2 = host->caps2;
+ unsigned int hs_max_dtr = 0;
+
+ if (card_type & EXT_CSD_CARD_TYPE_26)
+ hs_max_dtr = MMC_HIGH_26_MAX_DTR;
+
+ if (caps & MMC_CAP_MMC_HIGHSPEED &&
+ card_type & EXT_CSD_CARD_TYPE_52)
+ hs_max_dtr = MMC_HIGH_52_MAX_DTR;
+
+ if (caps & MMC_CAP_1_8V_DDR &&
+ card_type & EXT_CSD_CARD_TYPE_DDR_1_8V)
+ hs_max_dtr = MMC_HIGH_DDR_MAX_DTR;
+
+ if (caps & MMC_CAP_1_2V_DDR &&
+ card_type & EXT_CSD_CARD_TYPE_DDR_1_2V)
+ hs_max_dtr = MMC_HIGH_DDR_MAX_DTR;
+
+ if (caps2 & MMC_CAP2_HS200_1_8V_SDR &&
+ card_type & EXT_CSD_CARD_TYPE_SDR_1_8V)
+ hs_max_dtr = MMC_HS200_MAX_DTR;
+
+ if (caps2 & MMC_CAP2_HS200_1_2V_SDR &&
+ card_type & EXT_CSD_CARD_TYPE_SDR_1_2V)
+ hs_max_dtr = MMC_HS200_MAX_DTR;
+
+ card->ext_csd.hs_max_dtr = hs_max_dtr;
+ card->ext_csd.card_type = card_type;
+}
+
/*
* Decode extended CSD.
*/
@@ -284,56 +318,9 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)
if (card->ext_csd.sectors > (2u * 1024 * 1024 * 1024) / 512)
mmc_card_set_blockaddr(card);
}
+
card->ext_csd.raw_card_type = ext_csd[EXT_CSD_CARD_TYPE];
- switch (ext_csd[EXT_CSD_CARD_TYPE] & EXT_CSD_CARD_TYPE_MASK) {
- case EXT_CSD_CARD_TYPE_SDR_ALL:
- case EXT_CSD_CARD_TYPE_SDR_ALL_DDR_1_8V:
- case EXT_CSD_CARD_TYPE_SDR_ALL_DDR_1_2V:
- case EXT_CSD_CARD_TYPE_SDR_ALL_DDR_52:
- card->ext_csd.hs_max_dtr = 200000000;
- card->ext_csd.card_type = EXT_CSD_CARD_TYPE_SDR_200;
- break;
- case EXT_CSD_CARD_TYPE_SDR_1_2V_ALL:
- case EXT_CSD_CARD_TYPE_SDR_1_2V_DDR_1_8V:
- case EXT_CSD_CARD_TYPE_SDR_1_2V_DDR_1_2V:
- case EXT_CSD_CARD_TYPE_SDR_1_2V_DDR_52:
- card->ext_csd.hs_max_dtr = 200000000;
- card->ext_csd.card_type = EXT_CSD_CARD_TYPE_SDR_1_2V;
- break;
- case EXT_CSD_CARD_TYPE_SDR_1_8V_ALL:
- case EXT_CSD_CARD_TYPE_SDR_1_8V_DDR_1_8V:
- case EXT_CSD_CARD_TYPE_SDR_1_8V_DDR_1_2V:
- case EXT_CSD_CARD_TYPE_SDR_1_8V_DDR_52:
- card->ext_csd.hs_max_dtr = 200000000;
- card->ext_csd.card_type = EXT_CSD_CARD_TYPE_SDR_1_8V;
- break;
- case EXT_CSD_CARD_TYPE_DDR_52 | EXT_CSD_CARD_TYPE_52 |
- EXT_CSD_CARD_TYPE_26:
- card->ext_csd.hs_max_dtr = 52000000;
- card->ext_csd.card_type = EXT_CSD_CARD_TYPE_DDR_52;
- break;
- case EXT_CSD_CARD_TYPE_DDR_1_2V | EXT_CSD_CARD_TYPE_52 |
- EXT_CSD_CARD_TYPE_26:
- card->ext_csd.hs_max_dtr = 52000000;
- card->ext_csd.card_type = EXT_CSD_CARD_TYPE_DDR_1_2V;
- break;
- case EXT_CSD_CARD_TYPE_DDR_1_8V | EXT_CSD_CARD_TYPE_52 |
- EXT_CSD_CARD_TYPE_26:
- card->ext_csd.hs_max_dtr = 52000000;
- card->ext_csd.card_type = EXT_CSD_CARD_TYPE_DDR_1_8V;
- break;
- case EXT_CSD_CARD_TYPE_52 | EXT_CSD_CARD_TYPE_26:
- card->ext_csd.hs_max_dtr = 52000000;
- break;
- case EXT_CSD_CARD_TYPE_26:
- card->ext_csd.hs_max_dtr = 26000000;
- break;
- default:
- /* MMC v4 spec says this cannot happen */
- pr_warning("%s: card is mmc v4 but doesn't "
- "support any high-speed modes.\n",
- mmc_hostname(card->host));
- }
+ mmc_select_card_type(card);
card->ext_csd.raw_s_a_timeout = ext_csd[EXT_CSD_S_A_TIMEOUT];
card->ext_csd.raw_erase_timeout_mult =
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 629b823..d76513b 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -58,6 +58,10 @@ struct mmc_ext_csd {
unsigned int generic_cmd6_time; /* Units: 10ms */
unsigned int power_off_longtime; /* Units: ms */
unsigned int hs_max_dtr;
+#define MMC_HIGH_26_MAX_DTR 26000000
+#define MMC_HIGH_52_MAX_DTR 52000000
+#define MMC_HIGH_DDR_MAX_DTR 52000000
+#define MMC_HS200_MAX_DTR 200000000
unsigned int sectors;
unsigned int card_type;
unsigned int hc_erase_size; /* In sectors */
diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
index b822a2c..d425cab 100644
--- a/include/linux/mmc/mmc.h
+++ b/include/linux/mmc/mmc.h
@@ -354,66 +354,6 @@ struct _mmc_csd {
#define EXT_CSD_CARD_TYPE_SDR_1_2V (1<<5) /* Card can run at 200MHz */
/* SDR mode @1.2V I/O */
-#define EXT_CSD_CARD_TYPE_SDR_200 (EXT_CSD_CARD_TYPE_SDR_1_8V | \
- EXT_CSD_CARD_TYPE_SDR_1_2V)
-
-#define EXT_CSD_CARD_TYPE_SDR_ALL (EXT_CSD_CARD_TYPE_SDR_200 | \
- EXT_CSD_CARD_TYPE_52 | \
- EXT_CSD_CARD_TYPE_26)
-
-#define EXT_CSD_CARD_TYPE_SDR_1_2V_ALL (EXT_CSD_CARD_TYPE_SDR_1_2V | \
- EXT_CSD_CARD_TYPE_52 | \
- EXT_CSD_CARD_TYPE_26)
-
-#define EXT_CSD_CARD_TYPE_SDR_1_8V_ALL (EXT_CSD_CARD_TYPE_SDR_1_8V | \
- EXT_CSD_CARD_TYPE_52 | \
- EXT_CSD_CARD_TYPE_26)
-
-#define EXT_CSD_CARD_TYPE_SDR_1_2V_DDR_1_8V (EXT_CSD_CARD_TYPE_SDR_1_2V | \
- EXT_CSD_CARD_TYPE_DDR_1_8V | \
- EXT_CSD_CARD_TYPE_52 | \
- EXT_CSD_CARD_TYPE_26)
-
-#define EXT_CSD_CARD_TYPE_SDR_1_8V_DDR_1_8V (EXT_CSD_CARD_TYPE_SDR_1_8V | \
- EXT_CSD_CARD_TYPE_DDR_1_8V | \
- EXT_CSD_CARD_TYPE_52 | \
- EXT_CSD_CARD_TYPE_26)
-
-#define EXT_CSD_CARD_TYPE_SDR_1_2V_DDR_1_2V (EXT_CSD_CARD_TYPE_SDR_1_2V | \
- EXT_CSD_CARD_TYPE_DDR_1_2V | \
- EXT_CSD_CARD_TYPE_52 | \
- EXT_CSD_CARD_TYPE_26)
-
-#define EXT_CSD_CARD_TYPE_SDR_1_8V_DDR_1_2V (EXT_CSD_CARD_TYPE_SDR_1_8V | \
- EXT_CSD_CARD_TYPE_DDR_1_2V | \
- EXT_CSD_CARD_TYPE_52 | \
- EXT_CSD_CARD_TYPE_26)
-
-#define EXT_CSD_CARD_TYPE_SDR_1_2V_DDR_52 (EXT_CSD_CARD_TYPE_SDR_1_2V | \
- EXT_CSD_CARD_TYPE_DDR_52 | \
- EXT_CSD_CARD_TYPE_52 | \
- EXT_CSD_CARD_TYPE_26)
-
-#define EXT_CSD_CARD_TYPE_SDR_1_8V_DDR_52 (EXT_CSD_CARD_TYPE_SDR_1_8V | \
- EXT_CSD_CARD_TYPE_DDR_52 | \
- EXT_CSD_CARD_TYPE_52 | \
- EXT_CSD_CARD_TYPE_26)
-
-#define EXT_CSD_CARD_TYPE_SDR_ALL_DDR_1_8V (EXT_CSD_CARD_TYPE_SDR_200 | \
- EXT_CSD_CARD_TYPE_DDR_1_8V | \
- EXT_CSD_CARD_TYPE_52 | \
- EXT_CSD_CARD_TYPE_26)
-
-#define EXT_CSD_CARD_TYPE_SDR_ALL_DDR_1_2V (EXT_CSD_CARD_TYPE_SDR_200 | \
- EXT_CSD_CARD_TYPE_DDR_1_2V | \
- EXT_CSD_CARD_TYPE_52 | \
- EXT_CSD_CARD_TYPE_26)
-
-#define EXT_CSD_CARD_TYPE_SDR_ALL_DDR_52 (EXT_CSD_CARD_TYPE_SDR_200 | \
- EXT_CSD_CARD_TYPE_DDR_52 | \
- EXT_CSD_CARD_TYPE_52 | \
- EXT_CSD_CARD_TYPE_26)
-
#define EXT_CSD_BUS_WIDTH_1 0 /* Card is in 1 bit mode */
#define EXT_CSD_BUS_WIDTH_4 1 /* Card is in 4 bit mode */
#define EXT_CSD_BUS_WIDTH_8 2 /* Card is in 8 bit mode */
--
1.7.0.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] mmc: core: fix the decision of mmc card-type
2012-04-23 9:11 [PATCH 1/2] mmc: core: fix the decision of mmc card-type Seungwon Jeon
@ 2012-04-24 8:02 ` Subhash Jadavani
2012-04-25 5:14 ` Seungwon Jeon
0 siblings, 1 reply; 4+ messages in thread
From: Subhash Jadavani @ 2012-04-24 8:02 UTC (permalink / raw)
To: Seungwon Jeon; +Cc: linux-mmc, 'Chris Ball', 'Girish K S'
Hi Seungwon,
On 4/23/2012 2:41 PM, Seungwon Jeon wrote:
> Current implementation decides the card type exclusively. Even though
> eMMC device can support both HS200 and DDR mode, card type will be
> set only for HS200. If the host doesn't support HS200 but has DDR
> capability, then DDR mode can't be selected.
Yes, there is a bug. This patch looks fine to me. There are few minor
comments inline below.
>
> Signed-off-by: Seungwon Jeon<tgih.jun@samsung.com>
> ---
> drivers/mmc/core/mmc.c | 85 +++++++++++++++++++--------------------------
> include/linux/mmc/card.h | 4 ++
> include/linux/mmc/mmc.h | 60 --------------------------------
> 3 files changed, 40 insertions(+), 109 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 54df5ad..ebb9522 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -235,6 +235,40 @@ static int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd)
> return err;
> }
>
> +static void mmc_select_card_type(struct mmc_card *card)
> +{
> + struct mmc_host *host = card->host;
> + u8 card_type = card->ext_csd.raw_card_type& EXT_CSD_CARD_TYPE_MASK;
> + unsigned int caps = host->caps, caps2 = host->caps2;
> + unsigned int hs_max_dtr = 0;
> +
> + if (card_type& EXT_CSD_CARD_TYPE_26)
> + hs_max_dtr = MMC_HIGH_26_MAX_DTR;
> +
> + if (caps& MMC_CAP_MMC_HIGHSPEED&&
> + card_type& EXT_CSD_CARD_TYPE_52)
Just from code readability point of view, can we add parenthesis for
each conditions within if?
if ( (xxx) && (xxx))
> + hs_max_dtr = MMC_HIGH_52_MAX_DTR;
> +
> + if (caps& MMC_CAP_1_8V_DDR&&
> + card_type& EXT_CSD_CARD_TYPE_DDR_1_8V)
> + hs_max_dtr = MMC_HIGH_DDR_MAX_DTR;
> +
> + if (caps& MMC_CAP_1_2V_DDR&&
> + card_type& EXT_CSD_CARD_TYPE_DDR_1_2V)
> + hs_max_dtr = MMC_HIGH_DDR_MAX_DTR;
Both of the above checks are assigning the same value to hs_max_dtr? can
we combine both the if conditions with OR?
> +
> + if (caps2& MMC_CAP2_HS200_1_8V_SDR&&
> + card_type& EXT_CSD_CARD_TYPE_SDR_1_8V)
> + hs_max_dtr = MMC_HS200_MAX_DTR;
> +
> + if (caps2& MMC_CAP2_HS200_1_2V_SDR&&
> + card_type& EXT_CSD_CARD_TYPE_SDR_1_2V)
> + hs_max_dtr = MMC_HS200_MAX_DTR;
Same comment as above for DDR.
> +
> + card->ext_csd.hs_max_dtr = hs_max_dtr;
> + card->ext_csd.card_type = card_type;
> +}
> +
> /*
> * Decode extended CSD.
> */
> @@ -284,56 +318,9 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)
> if (card->ext_csd.sectors> (2u * 1024 * 1024 * 1024) / 512)
> mmc_card_set_blockaddr(card);
> }
> +
> card->ext_csd.raw_card_type = ext_csd[EXT_CSD_CARD_TYPE];
> - switch (ext_csd[EXT_CSD_CARD_TYPE]& EXT_CSD_CARD_TYPE_MASK) {
> - case EXT_CSD_CARD_TYPE_SDR_ALL:
> - case EXT_CSD_CARD_TYPE_SDR_ALL_DDR_1_8V:
> - case EXT_CSD_CARD_TYPE_SDR_ALL_DDR_1_2V:
> - case EXT_CSD_CARD_TYPE_SDR_ALL_DDR_52:
> - card->ext_csd.hs_max_dtr = 200000000;
> - card->ext_csd.card_type = EXT_CSD_CARD_TYPE_SDR_200;
> - break;
> - case EXT_CSD_CARD_TYPE_SDR_1_2V_ALL:
> - case EXT_CSD_CARD_TYPE_SDR_1_2V_DDR_1_8V:
> - case EXT_CSD_CARD_TYPE_SDR_1_2V_DDR_1_2V:
> - case EXT_CSD_CARD_TYPE_SDR_1_2V_DDR_52:
> - card->ext_csd.hs_max_dtr = 200000000;
> - card->ext_csd.card_type = EXT_CSD_CARD_TYPE_SDR_1_2V;
> - break;
> - case EXT_CSD_CARD_TYPE_SDR_1_8V_ALL:
> - case EXT_CSD_CARD_TYPE_SDR_1_8V_DDR_1_8V:
> - case EXT_CSD_CARD_TYPE_SDR_1_8V_DDR_1_2V:
> - case EXT_CSD_CARD_TYPE_SDR_1_8V_DDR_52:
> - card->ext_csd.hs_max_dtr = 200000000;
> - card->ext_csd.card_type = EXT_CSD_CARD_TYPE_SDR_1_8V;
> - break;
> - case EXT_CSD_CARD_TYPE_DDR_52 | EXT_CSD_CARD_TYPE_52 |
> - EXT_CSD_CARD_TYPE_26:
> - card->ext_csd.hs_max_dtr = 52000000;
> - card->ext_csd.card_type = EXT_CSD_CARD_TYPE_DDR_52;
> - break;
> - case EXT_CSD_CARD_TYPE_DDR_1_2V | EXT_CSD_CARD_TYPE_52 |
> - EXT_CSD_CARD_TYPE_26:
> - card->ext_csd.hs_max_dtr = 52000000;
> - card->ext_csd.card_type = EXT_CSD_CARD_TYPE_DDR_1_2V;
> - break;
> - case EXT_CSD_CARD_TYPE_DDR_1_8V | EXT_CSD_CARD_TYPE_52 |
> - EXT_CSD_CARD_TYPE_26:
> - card->ext_csd.hs_max_dtr = 52000000;
> - card->ext_csd.card_type = EXT_CSD_CARD_TYPE_DDR_1_8V;
> - break;
> - case EXT_CSD_CARD_TYPE_52 | EXT_CSD_CARD_TYPE_26:
> - card->ext_csd.hs_max_dtr = 52000000;
> - break;
> - case EXT_CSD_CARD_TYPE_26:
> - card->ext_csd.hs_max_dtr = 26000000;
> - break;
> - default:
> - /* MMC v4 spec says this cannot happen */
> - pr_warning("%s: card is mmc v4 but doesn't "
> - "support any high-speed modes.\n",
> - mmc_hostname(card->host));
> - }
> + mmc_select_card_type(card);
>
> card->ext_csd.raw_s_a_timeout = ext_csd[EXT_CSD_S_A_TIMEOUT];
> card->ext_csd.raw_erase_timeout_mult =
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index 629b823..d76513b 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -58,6 +58,10 @@ struct mmc_ext_csd {
> unsigned int generic_cmd6_time; /* Units: 10ms */
> unsigned int power_off_longtime; /* Units: ms */
> unsigned int hs_max_dtr;
> +#define MMC_HIGH_26_MAX_DTR 26000000
> +#define MMC_HIGH_52_MAX_DTR 52000000
> +#define MMC_HIGH_DDR_MAX_DTR 52000000
> +#define MMC_HS200_MAX_DTR 200000000
> unsigned int sectors;
> unsigned int card_type;
> unsigned int hc_erase_size; /* In sectors */
> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
> index b822a2c..d425cab 100644
> --- a/include/linux/mmc/mmc.h
> +++ b/include/linux/mmc/mmc.h
> @@ -354,66 +354,6 @@ struct _mmc_csd {
> #define EXT_CSD_CARD_TYPE_SDR_1_2V (1<<5) /* Card can run at 200MHz */
> /* SDR mode @1.2V I/O */
>
> -#define EXT_CSD_CARD_TYPE_SDR_200 (EXT_CSD_CARD_TYPE_SDR_1_8V | \
> - EXT_CSD_CARD_TYPE_SDR_1_2V)
> -
> -#define EXT_CSD_CARD_TYPE_SDR_ALL (EXT_CSD_CARD_TYPE_SDR_200 | \
> - EXT_CSD_CARD_TYPE_52 | \
> - EXT_CSD_CARD_TYPE_26)
> -
> -#define EXT_CSD_CARD_TYPE_SDR_1_2V_ALL (EXT_CSD_CARD_TYPE_SDR_1_2V | \
> - EXT_CSD_CARD_TYPE_52 | \
> - EXT_CSD_CARD_TYPE_26)
> -
> -#define EXT_CSD_CARD_TYPE_SDR_1_8V_ALL (EXT_CSD_CARD_TYPE_SDR_1_8V | \
> - EXT_CSD_CARD_TYPE_52 | \
> - EXT_CSD_CARD_TYPE_26)
> -
> -#define EXT_CSD_CARD_TYPE_SDR_1_2V_DDR_1_8V (EXT_CSD_CARD_TYPE_SDR_1_2V | \
> - EXT_CSD_CARD_TYPE_DDR_1_8V | \
> - EXT_CSD_CARD_TYPE_52 | \
> - EXT_CSD_CARD_TYPE_26)
> -
> -#define EXT_CSD_CARD_TYPE_SDR_1_8V_DDR_1_8V (EXT_CSD_CARD_TYPE_SDR_1_8V | \
> - EXT_CSD_CARD_TYPE_DDR_1_8V | \
> - EXT_CSD_CARD_TYPE_52 | \
> - EXT_CSD_CARD_TYPE_26)
> -
> -#define EXT_CSD_CARD_TYPE_SDR_1_2V_DDR_1_2V (EXT_CSD_CARD_TYPE_SDR_1_2V | \
> - EXT_CSD_CARD_TYPE_DDR_1_2V | \
> - EXT_CSD_CARD_TYPE_52 | \
> - EXT_CSD_CARD_TYPE_26)
> -
> -#define EXT_CSD_CARD_TYPE_SDR_1_8V_DDR_1_2V (EXT_CSD_CARD_TYPE_SDR_1_8V | \
> - EXT_CSD_CARD_TYPE_DDR_1_2V | \
> - EXT_CSD_CARD_TYPE_52 | \
> - EXT_CSD_CARD_TYPE_26)
> -
> -#define EXT_CSD_CARD_TYPE_SDR_1_2V_DDR_52 (EXT_CSD_CARD_TYPE_SDR_1_2V | \
> - EXT_CSD_CARD_TYPE_DDR_52 | \
> - EXT_CSD_CARD_TYPE_52 | \
> - EXT_CSD_CARD_TYPE_26)
> -
> -#define EXT_CSD_CARD_TYPE_SDR_1_8V_DDR_52 (EXT_CSD_CARD_TYPE_SDR_1_8V | \
> - EXT_CSD_CARD_TYPE_DDR_52 | \
> - EXT_CSD_CARD_TYPE_52 | \
> - EXT_CSD_CARD_TYPE_26)
> -
> -#define EXT_CSD_CARD_TYPE_SDR_ALL_DDR_1_8V (EXT_CSD_CARD_TYPE_SDR_200 | \
> - EXT_CSD_CARD_TYPE_DDR_1_8V | \
> - EXT_CSD_CARD_TYPE_52 | \
> - EXT_CSD_CARD_TYPE_26)
> -
> -#define EXT_CSD_CARD_TYPE_SDR_ALL_DDR_1_2V (EXT_CSD_CARD_TYPE_SDR_200 | \
> - EXT_CSD_CARD_TYPE_DDR_1_2V | \
> - EXT_CSD_CARD_TYPE_52 | \
> - EXT_CSD_CARD_TYPE_26)
> -
> -#define EXT_CSD_CARD_TYPE_SDR_ALL_DDR_52 (EXT_CSD_CARD_TYPE_SDR_200 | \
> - EXT_CSD_CARD_TYPE_DDR_52 | \
> - EXT_CSD_CARD_TYPE_52 | \
> - EXT_CSD_CARD_TYPE_26)
> -
> #define EXT_CSD_BUS_WIDTH_1 0 /* Card is in 1 bit mode */
> #define EXT_CSD_BUS_WIDTH_4 1 /* Card is in 4 bit mode */
> #define EXT_CSD_BUS_WIDTH_8 2 /* Card is in 8 bit mode */
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH 1/2] mmc: core: fix the decision of mmc card-type
2012-04-24 8:02 ` Subhash Jadavani
@ 2012-04-25 5:14 ` Seungwon Jeon
2012-04-25 6:10 ` Subhash Jadavani
0 siblings, 1 reply; 4+ messages in thread
From: Seungwon Jeon @ 2012-04-25 5:14 UTC (permalink / raw)
To: 'Subhash Jadavani'
Cc: linux-mmc, 'Chris Ball', 'Girish K S'
Hi Subhash,
Subhash Jadavani <subhashj@codeaurora.org> wrote:
> Hi Seungwon,
>
> On 4/23/2012 2:41 PM, Seungwon Jeon wrote:
> > Current implementation decides the card type exclusively. Even though
> > eMMC device can support both HS200 and DDR mode, card type will be
> > set only for HS200. If the host doesn't support HS200 but has DDR
> > capability, then DDR mode can't be selected.
>
> Yes, there is a bug. This patch looks fine to me. There are few minor
> comments inline below.
>
> >
> > Signed-off-by: Seungwon Jeon<tgih.jun@samsung.com>
> > ---
> > drivers/mmc/core/mmc.c | 85 +++++++++++++++++++--------------------------
> > include/linux/mmc/card.h | 4 ++
> > include/linux/mmc/mmc.h | 60 --------------------------------
> > 3 files changed, 40 insertions(+), 109 deletions(-)
> >
> > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> > index 54df5ad..ebb9522 100644
> > --- a/drivers/mmc/core/mmc.c
> > +++ b/drivers/mmc/core/mmc.c
> > @@ -235,6 +235,40 @@ static int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd)
> > return err;
> > }
> >
> > +static void mmc_select_card_type(struct mmc_card *card)
> > +{
> > + struct mmc_host *host = card->host;
> > + u8 card_type = card->ext_csd.raw_card_type& EXT_CSD_CARD_TYPE_MASK;
> > + unsigned int caps = host->caps, caps2 = host->caps2;
> > + unsigned int hs_max_dtr = 0;
> > +
> > + if (card_type& EXT_CSD_CARD_TYPE_26)
> > + hs_max_dtr = MMC_HIGH_26_MAX_DTR;
> > +
> > + if (caps& MMC_CAP_MMC_HIGHSPEED&&
> > + card_type& EXT_CSD_CARD_TYPE_52)
> Just from code readability point of view, can we add parenthesis for
> each conditions within if?
> if ( (xxx) && (xxx))
Right but '&' is higher than '&&' in priority, so how about keeping on?
>
>
> > + hs_max_dtr = MMC_HIGH_52_MAX_DTR;
> > +
> > + if (caps& MMC_CAP_1_8V_DDR&&
> > + card_type& EXT_CSD_CARD_TYPE_DDR_1_8V)
> > + hs_max_dtr = MMC_HIGH_DDR_MAX_DTR;
> > +
> > + if (caps& MMC_CAP_1_2V_DDR&&
> > + card_type& EXT_CSD_CARD_TYPE_DDR_1_2V)
> > + hs_max_dtr = MMC_HIGH_DDR_MAX_DTR;
> Both of the above checks are assigning the same value to hs_max_dtr? can
> we combine both the if conditions with OR?
> > +
> > + if (caps2& MMC_CAP2_HS200_1_8V_SDR&&
> > + card_type& EXT_CSD_CARD_TYPE_SDR_1_8V)
> > + hs_max_dtr = MMC_HS200_MAX_DTR;
> > +
> > + if (caps2& MMC_CAP2_HS200_1_2V_SDR&&
> > + card_type& EXT_CSD_CARD_TYPE_SDR_1_2V)
> > + hs_max_dtr = MMC_HS200_MAX_DTR;
> Same comment as above for DDR.
Yes, we can. I'll apply your feedback.
Thanks.
> > +
> > + card->ext_csd.hs_max_dtr = hs_max_dtr;
> > + card->ext_csd.card_type = card_type;
> > +}
> > +
> > /*
> > * Decode extended CSD.
> > */
> > @@ -284,56 +318,9 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)
> > if (card->ext_csd.sectors> (2u * 1024 * 1024 * 1024) / 512)
> > mmc_card_set_blockaddr(card);
> > }
> > +
> > card->ext_csd.raw_card_type = ext_csd[EXT_CSD_CARD_TYPE];
> > - switch (ext_csd[EXT_CSD_CARD_TYPE]& EXT_CSD_CARD_TYPE_MASK) {
> > - case EXT_CSD_CARD_TYPE_SDR_ALL:
> > - case EXT_CSD_CARD_TYPE_SDR_ALL_DDR_1_8V:
> > - case EXT_CSD_CARD_TYPE_SDR_ALL_DDR_1_2V:
> > - case EXT_CSD_CARD_TYPE_SDR_ALL_DDR_52:
> > - card->ext_csd.hs_max_dtr = 200000000;
> > - card->ext_csd.card_type = EXT_CSD_CARD_TYPE_SDR_200;
> > - break;
> > - case EXT_CSD_CARD_TYPE_SDR_1_2V_ALL:
> > - case EXT_CSD_CARD_TYPE_SDR_1_2V_DDR_1_8V:
> > - case EXT_CSD_CARD_TYPE_SDR_1_2V_DDR_1_2V:
> > - case EXT_CSD_CARD_TYPE_SDR_1_2V_DDR_52:
> > - card->ext_csd.hs_max_dtr = 200000000;
> > - card->ext_csd.card_type = EXT_CSD_CARD_TYPE_SDR_1_2V;
> > - break;
> > - case EXT_CSD_CARD_TYPE_SDR_1_8V_ALL:
> > - case EXT_CSD_CARD_TYPE_SDR_1_8V_DDR_1_8V:
> > - case EXT_CSD_CARD_TYPE_SDR_1_8V_DDR_1_2V:
> > - case EXT_CSD_CARD_TYPE_SDR_1_8V_DDR_52:
> > - card->ext_csd.hs_max_dtr = 200000000;
> > - card->ext_csd.card_type = EXT_CSD_CARD_TYPE_SDR_1_8V;
> > - break;
> > - case EXT_CSD_CARD_TYPE_DDR_52 | EXT_CSD_CARD_TYPE_52 |
> > - EXT_CSD_CARD_TYPE_26:
> > - card->ext_csd.hs_max_dtr = 52000000;
> > - card->ext_csd.card_type = EXT_CSD_CARD_TYPE_DDR_52;
> > - break;
> > - case EXT_CSD_CARD_TYPE_DDR_1_2V | EXT_CSD_CARD_TYPE_52 |
> > - EXT_CSD_CARD_TYPE_26:
> > - card->ext_csd.hs_max_dtr = 52000000;
> > - card->ext_csd.card_type = EXT_CSD_CARD_TYPE_DDR_1_2V;
> > - break;
> > - case EXT_CSD_CARD_TYPE_DDR_1_8V | EXT_CSD_CARD_TYPE_52 |
> > - EXT_CSD_CARD_TYPE_26:
> > - card->ext_csd.hs_max_dtr = 52000000;
> > - card->ext_csd.card_type = EXT_CSD_CARD_TYPE_DDR_1_8V;
> > - break;
> > - case EXT_CSD_CARD_TYPE_52 | EXT_CSD_CARD_TYPE_26:
> > - card->ext_csd.hs_max_dtr = 52000000;
> > - break;
> > - case EXT_CSD_CARD_TYPE_26:
> > - card->ext_csd.hs_max_dtr = 26000000;
> > - break;
> > - default:
> > - /* MMC v4 spec says this cannot happen */
> > - pr_warning("%s: card is mmc v4 but doesn't "
> > - "support any high-speed modes.\n",
> > - mmc_hostname(card->host));
> > - }
> > + mmc_select_card_type(card);
> >
> > card->ext_csd.raw_s_a_timeout = ext_csd[EXT_CSD_S_A_TIMEOUT];
> > card->ext_csd.raw_erase_timeout_mult =
> > diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> > index 629b823..d76513b 100644
> > --- a/include/linux/mmc/card.h
> > +++ b/include/linux/mmc/card.h
> > @@ -58,6 +58,10 @@ struct mmc_ext_csd {
> > unsigned int generic_cmd6_time; /* Units: 10ms */
> > unsigned int power_off_longtime; /* Units: ms */
> > unsigned int hs_max_dtr;
> > +#define MMC_HIGH_26_MAX_DTR 26000000
> > +#define MMC_HIGH_52_MAX_DTR 52000000
> > +#define MMC_HIGH_DDR_MAX_DTR 52000000
> > +#define MMC_HS200_MAX_DTR 200000000
> > unsigned int sectors;
> > unsigned int card_type;
> > unsigned int hc_erase_size; /* In sectors */
> > diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
> > index b822a2c..d425cab 100644
> > --- a/include/linux/mmc/mmc.h
> > +++ b/include/linux/mmc/mmc.h
> > @@ -354,66 +354,6 @@ struct _mmc_csd {
> > #define EXT_CSD_CARD_TYPE_SDR_1_2V (1<<5) /* Card can run at 200MHz */
> > /* SDR mode @1.2V I/O */
> >
> > -#define EXT_CSD_CARD_TYPE_SDR_200 (EXT_CSD_CARD_TYPE_SDR_1_8V | \
> > - EXT_CSD_CARD_TYPE_SDR_1_2V)
> > -
> > -#define EXT_CSD_CARD_TYPE_SDR_ALL (EXT_CSD_CARD_TYPE_SDR_200 | \
> > - EXT_CSD_CARD_TYPE_52 | \
> > - EXT_CSD_CARD_TYPE_26)
> > -
> > -#define EXT_CSD_CARD_TYPE_SDR_1_2V_ALL (EXT_CSD_CARD_TYPE_SDR_1_2V | \
> > - EXT_CSD_CARD_TYPE_52 | \
> > - EXT_CSD_CARD_TYPE_26)
> > -
> > -#define EXT_CSD_CARD_TYPE_SDR_1_8V_ALL (EXT_CSD_CARD_TYPE_SDR_1_8V | \
> > - EXT_CSD_CARD_TYPE_52 | \
> > - EXT_CSD_CARD_TYPE_26)
> > -
> > -#define EXT_CSD_CARD_TYPE_SDR_1_2V_DDR_1_8V (EXT_CSD_CARD_TYPE_SDR_1_2V | \
> > - EXT_CSD_CARD_TYPE_DDR_1_8V | \
> > - EXT_CSD_CARD_TYPE_52 | \
> > - EXT_CSD_CARD_TYPE_26)
> > -
> > -#define EXT_CSD_CARD_TYPE_SDR_1_8V_DDR_1_8V (EXT_CSD_CARD_TYPE_SDR_1_8V | \
> > - EXT_CSD_CARD_TYPE_DDR_1_8V | \
> > - EXT_CSD_CARD_TYPE_52 | \
> > - EXT_CSD_CARD_TYPE_26)
> > -
> > -#define EXT_CSD_CARD_TYPE_SDR_1_2V_DDR_1_2V (EXT_CSD_CARD_TYPE_SDR_1_2V | \
> > - EXT_CSD_CARD_TYPE_DDR_1_2V | \
> > - EXT_CSD_CARD_TYPE_52 | \
> > - EXT_CSD_CARD_TYPE_26)
> > -
> > -#define EXT_CSD_CARD_TYPE_SDR_1_8V_DDR_1_2V (EXT_CSD_CARD_TYPE_SDR_1_8V | \
> > - EXT_CSD_CARD_TYPE_DDR_1_2V | \
> > - EXT_CSD_CARD_TYPE_52 | \
> > - EXT_CSD_CARD_TYPE_26)
> > -
> > -#define EXT_CSD_CARD_TYPE_SDR_1_2V_DDR_52 (EXT_CSD_CARD_TYPE_SDR_1_2V | \
> > - EXT_CSD_CARD_TYPE_DDR_52 | \
> > - EXT_CSD_CARD_TYPE_52 | \
> > - EXT_CSD_CARD_TYPE_26)
> > -
> > -#define EXT_CSD_CARD_TYPE_SDR_1_8V_DDR_52 (EXT_CSD_CARD_TYPE_SDR_1_8V | \
> > - EXT_CSD_CARD_TYPE_DDR_52 | \
> > - EXT_CSD_CARD_TYPE_52 | \
> > - EXT_CSD_CARD_TYPE_26)
> > -
> > -#define EXT_CSD_CARD_TYPE_SDR_ALL_DDR_1_8V (EXT_CSD_CARD_TYPE_SDR_200 | \
> > - EXT_CSD_CARD_TYPE_DDR_1_8V | \
> > - EXT_CSD_CARD_TYPE_52 | \
> > - EXT_CSD_CARD_TYPE_26)
> > -
> > -#define EXT_CSD_CARD_TYPE_SDR_ALL_DDR_1_2V (EXT_CSD_CARD_TYPE_SDR_200 | \
> > - EXT_CSD_CARD_TYPE_DDR_1_2V | \
> > - EXT_CSD_CARD_TYPE_52 | \
> > - EXT_CSD_CARD_TYPE_26)
> > -
> > -#define EXT_CSD_CARD_TYPE_SDR_ALL_DDR_52 (EXT_CSD_CARD_TYPE_SDR_200 | \
> > - EXT_CSD_CARD_TYPE_DDR_52 | \
> > - EXT_CSD_CARD_TYPE_52 | \
> > - EXT_CSD_CARD_TYPE_26)
> > -
> > #define EXT_CSD_BUS_WIDTH_1 0 /* Card is in 1 bit mode */
> > #define EXT_CSD_BUS_WIDTH_4 1 /* Card is in 4 bit mode */
> > #define EXT_CSD_BUS_WIDTH_8 2 /* Card is in 8 bit mode */
>
> --
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH 1/2] mmc: core: fix the decision of mmc card-type
2012-04-25 5:14 ` Seungwon Jeon
@ 2012-04-25 6:10 ` Subhash Jadavani
0 siblings, 0 replies; 4+ messages in thread
From: Subhash Jadavani @ 2012-04-25 6:10 UTC (permalink / raw)
To: 'Seungwon Jeon'
Cc: linux-mmc, 'Chris Ball', 'Girish K S'
> > > + if (caps& MMC_CAP_MMC_HIGHSPEED&&
> > > + card_type& EXT_CSD_CARD_TYPE_52)
> > Just from code readability point of view, can we add parenthesis for
> > each conditions within if?
> > if ( (xxx) && (xxx))
> Right but '&' is higher than '&&' in priority, so how about keeping on?
Ok.
Regards,
Subhash
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-04-25 6:10 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-23 9:11 [PATCH 1/2] mmc: core: fix the decision of mmc card-type Seungwon Jeon
2012-04-24 8:02 ` Subhash Jadavani
2012-04-25 5:14 ` Seungwon Jeon
2012-04-25 6:10 ` Subhash Jadavani
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.