On 14 March 2014 03:49, Seungwon Jeon <[email protected]> wrote:
> On Thu, March 13, 2014, Ulf Hansson wrote:
>> On 7 March 2014 15:36, Seungwon Jeon <[email protected]> wrote:
>> > Device types which are supported by both host and device
>> > can be identified when EXT_CSD is read. There is no need to
>> > check host's capability anymore.
>> >
>> > Signed-off-by: Seungwon Jeon <[email protected]>
>> > ---
>> > Changes in v2:
>> > Just rebased with latest one.
>> >
>> > drivers/mmc/core/mmc.c | 77
>> > ++++++++++++++++++++++++++-------------------
>> > include/linux/mmc/card.h | 6 ++-
>> > include/linux/mmc/host.h | 6 ---
>> > include/linux/mmc/mmc.h | 12 +++++--
>> > 4 files changed, 56 insertions(+), 45 deletions(-)
>> >
>> > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> > index db9655f..0abece0 100644
>> > --- a/drivers/mmc/core/mmc.c
>> > +++ b/drivers/mmc/core/mmc.c
>> > @@ -243,28 +243,46 @@ static void mmc_select_card_type(struct mmc_card
>> > *card)
>> > u8 card_type = card->ext_csd.raw_card_type &
>> > EXT_CSD_CARD_TYPE_MASK;
>> > u32 caps = host->caps, caps2 = host->caps2;
>> > unsigned int hs_max_dtr = 0;
>> > + unsigned int avail_type = 0;
>> >
>> > - if (card_type & EXT_CSD_CARD_TYPE_26)
>> > + if (caps & MMC_CAP_MMC_HIGHSPEED &&
>> > + card_type & EXT_CSD_CARD_TYPE_HS_26) {
>> > hs_max_dtr = MMC_HIGH_26_MAX_DTR;
>> > + avail_type |= EXT_CSD_CARD_TYPE_HS_26;
>> > + }
>> >
>> > if (caps & MMC_CAP_MMC_HIGHSPEED &&
>> > - card_type & EXT_CSD_CARD_TYPE_52)
>> > + card_type & EXT_CSD_CARD_TYPE_HS_52) {
>> > hs_max_dtr = MMC_HIGH_52_MAX_DTR;
>> > + avail_type |= EXT_CSD_CARD_TYPE_HS_52;
>> > + }
>> >
>> > - if ((caps & MMC_CAP_1_8V_DDR &&
>> > - card_type & EXT_CSD_CARD_TYPE_DDR_1_8V) ||
>> > - (caps & MMC_CAP_1_2V_DDR &&
>> > - card_type & EXT_CSD_CARD_TYPE_DDR_1_2V))
>> > + if (caps & MMC_CAP_1_8V_DDR &&
>> > + card_type & EXT_CSD_CARD_TYPE_DDR_1_8V) {
>> > hs_max_dtr = MMC_HIGH_DDR_MAX_DTR;
>> > + avail_type |= EXT_CSD_CARD_TYPE_DDR_1_8V;
>> > + }
>> >
>> > - if ((caps2 & MMC_CAP2_HS200_1_8V_SDR &&
>> > - card_type & EXT_CSD_CARD_TYPE_SDR_1_8V) ||
>> > - (caps2 & MMC_CAP2_HS200_1_2V_SDR &&
>> > - card_type & EXT_CSD_CARD_TYPE_SDR_1_2V))
>> > + if (caps & MMC_CAP_1_2V_DDR &&
>> > + card_type & EXT_CSD_CARD_TYPE_DDR_1_2V) {
>> > + hs_max_dtr = MMC_HIGH_DDR_MAX_DTR;
>> > + avail_type |= EXT_CSD_CARD_TYPE_DDR_1_2V;
>> > + }
>> > +
>> > + if (caps2 & MMC_CAP2_HS200_1_8V_SDR &&
>> > + card_type & EXT_CSD_CARD_TYPE_HS200_1_8V) {
>> > hs_max_dtr = MMC_HS200_MAX_DTR;
>> > + avail_type |= EXT_CSD_CARD_TYPE_HS200_1_8V;
>> > + }
>> > +
>> > + if (caps2 & MMC_CAP2_HS200_1_2V_SDR &&
>> > + card_type & EXT_CSD_CARD_TYPE_HS200_1_2V) {
>> > + hs_max_dtr = MMC_HS200_MAX_DTR;
>> > + avail_type |= EXT_CSD_CARD_TYPE_HS200_1_2V;
>> > + }
>> >
>> > card->ext_csd.hs_max_dtr = hs_max_dtr;
>> > - card->ext_csd.card_type = card_type;
>> > + card->mmc_avail_type = avail_type;
>> > }
>> >
>> > /*
>> > @@ -708,6 +726,11 @@ static struct device_type mmc_type = {
>> > .groups = mmc_attr_groups,
>> > };
>> >
>> > +static inline unsigned int mmc_snoop_ddr(struct mmc_card *card)
>> > +{
>> > + return card->mmc_avail_type & EXT_CSD_CARD_TYPE_DDR_52;
>> > +}
>> > +
>>
>> Having a separate function for this seem a bit silly. Similar checks
>> is performed all over the code. I suggest you remove this.
> I understand your meaning.
> Yes, actually similar checking card type is done.
> But checking DDR type is required several times unlike other type case.
> I considered for that. I think it's pretty useful in terms of avoiding
> duplication and enhancement of readability.
>
As we don't have functions that performs the similar check for
"mmc_avail_type", I don't think we should have it for ddr either. I
prefer the symmetry in the code, so please remove.
Kind regards
Ulf Hansson
>>
>> > /*
>> > * Select the PowerClass for the current bus width
>> > * If power class is defined for 4/8 bit bus in the
>> > @@ -808,12 +831,10 @@ static int mmc_select_hs200(struct mmc_card *card)
>> >
>> > host = card->host;
>> >
>> > - if (card->ext_csd.card_type & EXT_CSD_CARD_TYPE_SDR_1_2V &&
>> > - host->caps2 & MMC_CAP2_HS200_1_2V_SDR)
>> > + if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200_1_2V)
>> > err = __mmc_set_signal_voltage(host,
>> > MMC_SIGNAL_VOLTAGE_120);
>> >
>> > - if (err && card->ext_csd.card_type & EXT_CSD_CARD_TYPE_SDR_1_8V &&
>> > - host->caps2 & MMC_CAP2_HS200_1_8V_SDR)
>> > + if (err && card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200_1_8V)
>> > err = __mmc_set_signal_voltage(host,
>> > MMC_SIGNAL_VOLTAGE_180);
>> >
>> > /* If fails try again during next card power cycle */
>> > @@ -1072,10 +1093,9 @@ static int mmc_init_card(struct mmc_host *host, u32
>> > ocr,
>> > */
>> > if (card->ext_csd.hs_max_dtr != 0) {
>> > err = 0;
>> > - if (card->ext_csd.hs_max_dtr > 52000000 &&
>> > - host->caps2 & MMC_CAP2_HS200)
>> > + if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200)
>> > err = mmc_select_hs200(card);
>> > - else if (host->caps & MMC_CAP_MMC_HIGHSPEED)
>> > + else if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS)
>> > err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>> > EXT_CSD_HS_TIMING, 1,
>> > card->ext_csd.generic_cmd6_time,
>> > @@ -1089,13 +1109,11 @@ static int mmc_init_card(struct mmc_host *host,
>> > u32 ocr,
>> > mmc_hostname(card->host));
>> > err = 0;
>> > } else {
>> > - if (card->ext_csd.hs_max_dtr > 52000000 &&
>> > - host->caps2 & MMC_CAP2_HS200) {
>> > + if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200)
>> > mmc_set_timing(card->host,
>> > MMC_TIMING_MMC_HS200);
>> > - } else {
>> > + else
>> > mmc_set_timing(card->host,
>> > MMC_TIMING_MMC_HS);
>> > - }
>> > }
>> > }
>> >
>> > @@ -1118,14 +1136,8 @@ static int mmc_init_card(struct mmc_host *host, u32
>> > ocr,
>> > /*
>> > * Indicate DDR mode (if supported).
>> > */
>> > - if (mmc_card_hs(card)) {
>> > - if ((card->ext_csd.card_type & EXT_CSD_CARD_TYPE_DDR_1_8V)
>> > - && (host->caps & MMC_CAP_1_8V_DDR))
>> > - ddr = MMC_1_8V_DDR_MODE;
>> > - else if ((card->ext_csd.card_type &
>> > EXT_CSD_CARD_TYPE_DDR_1_2V)
>> > - && (host->caps & MMC_CAP_1_2V_DDR))
>> > - ddr = MMC_1_2V_DDR_MODE;
>> > - }
>> > + if (mmc_card_hs(card))
>> > + ddr = mmc_snoop_ddr(card);
>> >
>> > /*
>> > * Indicate HS200 SDR mode (if supported).
>> > @@ -1145,8 +1157,7 @@ static int mmc_init_card(struct mmc_host *host, u32
>> > ocr,
>> > * 3. set the clock to > 52Mhz <=200MHz and
>> > * 4. execute tuning for HS200
>> > */
>> > - if ((host->caps2 & MMC_CAP2_HS200) &&
>> > - card->host->ops->execute_tuning) {
>> > + if (card->host->ops->execute_tuning) {
>> > mmc_host_clk_hold(card->host);
>> > err = card->host->ops->execute_tuning(card->host,
>> > MMC_SEND_TUNING_BLOCK_HS200);
>> > @@ -1255,7 +1266,7 @@ static int mmc_init_card(struct mmc_host *host, u32
>> > ocr,
>> > *
>> > * WARNING: eMMC rules are NOT the same as SD DDR
>> > */
>> > - if (ddr == MMC_1_2V_DDR_MODE) {
>> > + if (ddr & EXT_CSD_CARD_TYPE_DDR_1_2V) {
>> > err = __mmc_set_signal_voltage(host,
>> > MMC_SIGNAL_VOLTAGE_120);
>> > if (err)
>> > diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
>> > index 5473133..c232b10 100644
>> > --- a/include/linux/mmc/card.h
>> > +++ b/include/linux/mmc/card.h
>> > @@ -68,7 +68,6 @@ struct mmc_ext_csd {
>> > #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 */
>> > unsigned int hc_erase_timeout; /* In milliseconds
>> > */
>> > unsigned int sec_trim_mult; /* Secure trim multiplier
>> > */
>> > @@ -297,7 +296,10 @@ struct mmc_card {
>> > const char **info; /* info strings */
>> > struct sdio_func_tuple *tuples; /* unknown common tuples */
>> >
>> > - unsigned int sd_bus_speed; /* Bus Speed Mode set for
>> > the card */
>> > + union {
>> > + unsigned int sd_bus_speed; /* Bus Speed Mode
>> > set for the card */
>> > + unsigned int mmc_avail_type; /* supported
>> > device type by both host and card */
>>
>> Using a union here won't be that much of a gain since there are only
>> few instances of the struct. Please remove.
> Yes, you're right. It's not much in gain.
> I intended to distinguish these two similar members respectively.
> It's used only in each specific type domain and not used at the same time.
> If no meaningful, I can remove as your suggestion.
>
> Thanks,
> Seungwon Jeon
>
>>
>> > + };
>> >
>> > struct dentry *debugfs_root;
>> > struct mmc_part part[MMC_NUM_PHY_PARTITION]; /* physical
>> > partitions */
>> > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>> > index 2f263ae..1ee3c10 100644
>> > --- a/include/linux/mmc/host.h
>> > +++ b/include/linux/mmc/host.h
>> > @@ -62,12 +62,6 @@ struct mmc_ios {
>> > #define MMC_TIMING_MMC_DDR52 8
>> > #define MMC_TIMING_MMC_HS200 9
>> >
>> > -#define MMC_SDR_MODE 0
>> > -#define MMC_1_2V_DDR_MODE 1
>> > -#define MMC_1_8V_DDR_MODE 2
>> > -#define MMC_1_2V_SDR_MODE 3
>> > -#define MMC_1_8V_SDR_MODE 4
>> > -
>> > unsigned char signal_voltage; /* signalling voltage
>> > (1.8V or 3.3V) */
>> >
>> > #define MMC_SIGNAL_VOLTAGE_330 0
>> > diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
>> > index 50bcde3..f734c0c 100644
>> > --- a/include/linux/mmc/mmc.h
>> > +++ b/include/linux/mmc/mmc.h
>> > @@ -354,18 +354,22 @@ struct _mmc_csd {
>> > #define EXT_CSD_CMD_SET_SECURE (1<<1)
>> > #define EXT_CSD_CMD_SET_CPSECURE (1<<2)
>> >
>> > -#define EXT_CSD_CARD_TYPE_26 (1<<0) /* Card can run at 26MHz */
>> > -#define EXT_CSD_CARD_TYPE_52 (1<<1) /* Card can run at 52MHz */
>> > #define EXT_CSD_CARD_TYPE_MASK 0x3F /* Mask out reserved bits */
>> > +#define EXT_CSD_CARD_TYPE_HS_26 (1<<0) /* Card can run at 26MHz */
>> > +#define EXT_CSD_CARD_TYPE_HS_52 (1<<1) /* Card can run at 52MHz */
>> > +#define EXT_CSD_CARD_TYPE_HS (EXT_CSD_CARD_TYPE_HS_26 | \
>> > + EXT_CSD_CARD_TYPE_HS_52)
>> > #define EXT_CSD_CARD_TYPE_DDR_1_8V (1<<2) /* Card can run at 52MHz */
>> > /* DDR mode @1.8V or 3V I/O */
>> > #define EXT_CSD_CARD_TYPE_DDR_1_2V (1<<3) /* Card can run at 52MHz */
>> > /* DDR mode @1.2V I/O */
>> > #define EXT_CSD_CARD_TYPE_DDR_52 (EXT_CSD_CARD_TYPE_DDR_1_8V \
>> > | EXT_CSD_CARD_TYPE_DDR_1_2V)
>> > -#define EXT_CSD_CARD_TYPE_SDR_1_8V (1<<4) /* Card can run at 200MHz
>> > */
>> > -#define EXT_CSD_CARD_TYPE_SDR_1_2V (1<<5) /* Card can run at 200MHz
>> > */
>> > +#define EXT_CSD_CARD_TYPE_HS200_1_8V (1<<4) /* Card can run at 200MHz
>> > */
>> > +#define EXT_CSD_CARD_TYPE_HS200_1_2V (1<<5) /* Card can run at 200MHz
>> > */
>> > /* SDR mode @1.2V I/O */
>> > +#define EXT_CSD_CARD_TYPE_HS200
>> > (EXT_CSD_CARD_TYPE_HS200_1_8V | \
>> > + EXT_CSD_CARD_TYPE_HS200_1_2V)
>> >
>> > #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 */
>> > --
>> > 1.7.0.4
>> >
>> >
>>
>> Besides the minor stuff, looks good.
>>
>> Kind regards
>> Ulf Hansson
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
