| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <944f9a21-8296-c29d-cccf-f9588aceab4a@rock-chips.com>
Date: Fri, 27 May 2016 14:45:48 +0800
From: Shawn Lin <shawn.lin@...k-chips.com>
To: Adrian Hunter <adrian.hunter@...el.com>
Cc: shawn.lin@...k-chips.com, Ulf Hansson <ulf.hansson@...aro.org>,
Jaehoon Chung <jh80.chung@...sung.com>,
Michal Simek <michal.simek@...inx.com>,
soren.brinkmann@...inx.com, Rob Herring <robh+dt@...nel.org>,
linux-mmc@...r.kernel.org, linux-kernel@...r.kernel.org,
Doug Anderson <dianders@...omium.org>,
Brian Norris <briannorris@...omium.org>,
Heiko Stuebner <heiko@...ech.de>,
linux-rockchip@...ts.infradead.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v6 3/5] mmc: core: implement enhanced strobe support
在 2016/5/26 19:04, Adrian Hunter 写道:
> On 26/05/16 04:56, Shawn Lin wrote:
>> Controllers use data strobe line to latch data from devices
>> under hs400 mode, but not for cmd line. So since emmc 5.1, JEDEC
>> introduces enhanced strobe mode for latching cmd response from
>> emmc devices to host controllers. This new feature is optional,
>> so it depends both on device's cap and host's cap to decide
>> whether to use it or not.
>>
>> Signed-off-by: Shawn Lin <shawn.lin@...k-chips.com>
>
> I have a couple of comments below, but they don't necessarily require any
> action, so:
>
> Acked-by: Adrian Hunter <adrian.hunter@...el.com>
>
>>
>> Reviewed-by: Jaehoon Chung <jh80.chung@...sung.com>
>> Tested-by: Douglas Anderson <dianders@...omium.org>
>> Tested-by: Jaehoon Chung <jh80.chung@...sung.com>
>> ---
>>
>> Changes in v6:
>> - add Doug's test-tag and Jaehoon's review-test-tag
>>
>> Changes in v5: None
>> Changes in v4: None
>> Changes in v3: None
>> Changes in v2: None
>>
>> drivers/mmc/core/bus.c | 3 +-
>> drivers/mmc/core/core.c | 9 ++++++
>> drivers/mmc/core/mmc.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++--
>> include/linux/mmc/card.h | 1 +
>> include/linux/mmc/host.h | 11 +++++++
>> include/linux/mmc/mmc.h | 3 ++
>> 6 files changed, 108 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
>> index 4bc48f1..c64266f 100644
>> --- a/drivers/mmc/core/bus.c
>> +++ b/drivers/mmc/core/bus.c
>> @@ -332,12 +332,13 @@ int mmc_add_card(struct mmc_card *card)
>> mmc_card_ddr52(card) ? "DDR " : "",
>> type);
>> } else {
>> - pr_info("%s: new %s%s%s%s%s card at address %04x\n",
>> + pr_info("%s: new %s%s%s%s%s%s card at address %04x\n",
>> mmc_hostname(card->host),
>> mmc_card_uhs(card) ? "ultra high speed " :
>> (mmc_card_hs(card) ? "high speed " : ""),
>> mmc_card_hs400(card) ? "HS400 " :
>> (mmc_card_hs200(card) ? "HS200 " : ""),
>> + mmc_card_hs400es(card) ? "Enhanced strobe " : "",
>> mmc_card_ddr52(card) ? "DDR " : "",
>> uhs_bus_speed_mode, type, card->rca);
>> }
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index 99275e4..b0c1f07 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -1127,6 +1127,15 @@ void mmc_set_initial_state(struct mmc_host *host)
>> host->ios.bus_width = MMC_BUS_WIDTH_1;
>> host->ios.timing = MMC_TIMING_LEGACY;
>> host->ios.drv_type = 0;
>> + host->ios.enhanced_strobe = false;
>> +
>> + /*
>> + * Make sure we are in non-enhanced strobe mode before we
>> + * actually enable it in ext_csd.
>> + */
>> + if ((host->caps2 & MMC_CAP2_HS400_ES) &&
>> + host->ops->hs400_enhanced_strobe)
>> + host->ops->hs400_enhanced_strobe(host, &host->ios);
>>
>> mmc_set_ios(host);
>> }
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index f99c47e..9dc91ba 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -235,6 +235,11 @@ static void mmc_select_card_type(struct mmc_card *card)
>> avail_type |= EXT_CSD_CARD_TYPE_HS400_1_2V;
>> }
>>
>> + if ((caps2 & MMC_CAP2_HS400_ES) &&
>> + card->ext_csd.strobe_support &&
>> + (avail_type & EXT_CSD_CARD_TYPE_HS400))
>> + avail_type |= EXT_CSD_CARD_TYPE_HS400ES;
>> +
>> card->ext_csd.hs_max_dtr = hs_max_dtr;
>> card->ext_csd.hs200_max_dtr = hs200_max_dtr;
>> card->mmc_avail_type = avail_type;
>> @@ -383,6 +388,7 @@ static int mmc_decode_ext_csd(struct mmc_card *card, u8 *ext_csd)
>> mmc_card_set_blockaddr(card);
>> }
>>
>> + card->ext_csd.strobe_support = ext_csd[EXT_CSD_STROBE_SUPPORT];
>> card->ext_csd.raw_card_type = ext_csd[EXT_CSD_CARD_TYPE];
>> mmc_select_card_type(card);
>>
>> @@ -1216,6 +1222,78 @@ out_err:
>> return err;
>> }
>>
>> +static int mmc_select_hs400es(struct mmc_card *card)
>> +{
>> + struct mmc_host *host = card->host;
>> + int err = 0;
>> + u8 val;
>> +
>> + if (!(host->caps & MMC_CAP_8_BIT_DATA)) {
>> + err = -ENOTSUPP;
>> + goto out_err;
>> + }
>> +
>> + err = mmc_select_bus_width(card);
>> + if (IS_ERR_VALUE(err))
>> + goto out_err;
>> +
>> + /* Switch card to HS mode */
>> + err = mmc_select_hs(card);
>> + if (err) {
>> + pr_err("%s: switch to high-speed failed, err:%d\n",
>> + mmc_hostname(host), err);
>> + goto out_err;
>> + }
>> +
>> + err = mmc_switch_status(card);
>> + if (err)
>> + goto out_err;
>
> The spec. says "switch to High Speed mode and then set the clock frequency
> to a value not greater than 52 MHz" which would tend to imply changing to
> the High Speed frequency here. It shouldn't make any difference, so I don't
> know if it is worth doing.
yes, it should't make any difference as we are already assign a clock
freq not greater than 52M(400k should also work for Highspeed).
>
>> +
>> + /* Switch card to DDR with strobe bit */
>> + val = EXT_CSD_DDR_BUS_WIDTH_8 | EXT_CSD_BUS_WIDTH_STROBE;
>> + err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>> + EXT_CSD_BUS_WIDTH,
>> + val,
>> + card->ext_csd.generic_cmd6_time);
>> + if (err) {
>> + pr_err("%s: switch to bus width for hs400es failed, err:%d\n",
>> + mmc_hostname(host), err);
>> + goto out_err;
>> + }
>> +
>> + /* Switch card to HS400 */
>> + val = EXT_CSD_TIMING_HS400 |
>> + card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
>> + err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>> + EXT_CSD_HS_TIMING, val,
>> + card->ext_csd.generic_cmd6_time,
>> + true, false, true);
>> + if (err) {
>> + pr_err("%s: switch to hs400es failed, err:%d\n",
>> + mmc_hostname(host), err);
>> + goto out_err;
>> + }
>> +
>> + /* Set host controller to HS400 timing and frequency */
>> + mmc_set_timing(host, MMC_TIMING_MMC_HS400);
>> +
>> + /* Controller enable enhanced strobe function */
>> + host->ios.enhanced_strobe = true;
>> + if (host->ops->hs400_enhanced_strobe)
>> + host->ops->hs400_enhanced_strobe(host, &host->ios);
>> +
>
> And again here, if we followed the spec. exactly we would change to the
> HS400 frequency here rather than after sending CMD13. Again, it shouldn't
> make any difference, so I don't if it is worth doing.
I delayed the response to your comment as I have been tring some
different eMMC devices to see whether it makes any difference. Now I
it's OK. :)
Thanks.
>
>> + err = mmc_switch_status(card);
>> + if (err)
>> + goto out_err;
>> +
>> + return 0;
>> +
>> +out_err:
>> + pr_err("%s: %s failed, error %d\n", mmc_hostname(card->host),
>> + __func__, err);
>> + return err;
>> +}
>> +
>> static void mmc_select_driver_type(struct mmc_card *card)
>> {
>> int card_drv_type, drive_strength, drv_type;
>> @@ -1297,7 +1375,7 @@ err:
>> }
>>
>> /*
>> - * Activate High Speed or HS200 mode if supported.
>> + * Activate High Speed, HS200 or HS400ES mode if supported.
>> */
>> static int mmc_select_timing(struct mmc_card *card)
>> {
>> @@ -1306,7 +1384,9 @@ static int mmc_select_timing(struct mmc_card *card)
>> if (!mmc_can_ext_csd(card))
>> goto bus_speed;
>>
>> - if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200)
>> + if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES)
>> + err = mmc_select_hs400es(card);
>> + else if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS200)
>> err = mmc_select_hs200(card);
>> else if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS)
>> err = mmc_select_hs(card);
>> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
>> index eb0151b..22defc2 100644
>> --- a/include/linux/mmc/card.h
>> +++ b/include/linux/mmc/card.h
>> @@ -95,6 +95,7 @@ struct mmc_ext_csd {
>> u8 raw_partition_support; /* 160 */
>> u8 raw_rpmb_size_mult; /* 168 */
>> u8 raw_erased_mem_count; /* 181 */
>> + u8 strobe_support; /* 184 */
>> u8 raw_ext_csd_structure; /* 194 */
>> u8 raw_card_type; /* 196 */
>> u8 raw_driver_strength; /* 197 */
>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>> index 2a06fb0..0eabb72 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -19,6 +19,7 @@
>>
>> #include <linux/mmc/core.h>
>> #include <linux/mmc/card.h>
>> +#include <linux/mmc/mmc.h>
>> #include <linux/mmc/pm.h>
>>
>> struct mmc_ios {
>> @@ -77,6 +78,8 @@ struct mmc_ios {
>> #define MMC_SET_DRIVER_TYPE_A 1
>> #define MMC_SET_DRIVER_TYPE_C 2
>> #define MMC_SET_DRIVER_TYPE_D 3
>> +
>> + bool enhanced_strobe; /* hs400es selection */
>> };
>>
>> struct mmc_host_ops {
>> @@ -143,6 +146,9 @@ struct mmc_host_ops {
>>
>> /* Prepare HS400 target operating frequency depending host driver */
>> int (*prepare_hs400_tuning)(struct mmc_host *host, struct mmc_ios *ios);
>> + /* Prepare enhanced strobe depending host driver */
>> + void (*hs400_enhanced_strobe)(struct mmc_host *host,
>> + struct mmc_ios *ios);
>> int (*select_drive_strength)(struct mmc_card *card,
>> unsigned int max_dtr, int host_drv,
>> int card_drv, int *drv_type);
>> @@ -513,6 +519,11 @@ static inline bool mmc_card_hs400(struct mmc_card *card)
>> return card->host->ios.timing == MMC_TIMING_MMC_HS400;
>> }
>>
>> +static inline bool mmc_card_hs400es(struct mmc_card *card)
>> +{
>> + return card->host->ios.enhanced_strobe;
>> +}
>> +
>> void mmc_retune_timer_stop(struct mmc_host *host);
>>
>> static inline void mmc_retune_needed(struct mmc_host *host)
>> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h
>> index 15f2c4a..c376209 100644
>> --- a/include/linux/mmc/mmc.h
>> +++ b/include/linux/mmc/mmc.h
>> @@ -297,6 +297,7 @@ struct _mmc_csd {
>> #define EXT_CSD_PART_CONFIG 179 /* R/W */
>> #define EXT_CSD_ERASED_MEM_CONT 181 /* RO */
>> #define EXT_CSD_BUS_WIDTH 183 /* R/W */
>> +#define EXT_CSD_STROBE_SUPPORT 184 /* RO */
>> #define EXT_CSD_HS_TIMING 185 /* R/W */
>> #define EXT_CSD_POWER_CLASS 187 /* R/W */
>> #define EXT_CSD_REV 192 /* RO */
>> @@ -380,12 +381,14 @@ struct _mmc_csd {
>> #define EXT_CSD_CARD_TYPE_HS400_1_2V (1<<7) /* Card can run at 200MHz DDR, 1.2V */
>> #define EXT_CSD_CARD_TYPE_HS400 (EXT_CSD_CARD_TYPE_HS400_1_8V | \
>> EXT_CSD_CARD_TYPE_HS400_1_2V)
>> +#define EXT_CSD_CARD_TYPE_HS400ES (1<<8) /* Card can run at HS400ES */
>>
>> #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 */
>> #define EXT_CSD_DDR_BUS_WIDTH_4 5 /* Card is in 4 bit DDR mode */
>> #define EXT_CSD_DDR_BUS_WIDTH_8 6 /* Card is in 8 bit DDR mode */
>> +#define EXT_CSD_BUS_WIDTH_STROBE BIT(7) /* Enhanced strobe mode */
>>
>> #define EXT_CSD_TIMING_BC 0 /* Backwards compatility */
>> #define EXT_CSD_TIMING_HS 1 /* High speed */
>>
>
>
>
>
--
Best Regards
Shawn Lin
Powered by blists - more mailing lists