Hi Subhash,
Subhash Jadavani <subhashj@xxxxxxxxxxxxxx> 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@xxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
