From: Doug Anderson <dianders@chromium.org>
To: Shawn Lin <shawn.lin@rock-chips.com>
Cc: Ulf Hansson <ulf.hansson@linaro.org>,
Adrian Hunter <adrian.hunter@intel.com>,
Rob Herring <robh+dt@kernel.org>,
"linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Heiko Stuebner <heiko@sntech.de>,
"open list:ARM/Rockchip SoC..."
<linux-rockchip@lists.infradead.org>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>
Subject: Re: [PATCH v3 3/5] mmc: core: implement enhanced strobe support
Date: Thu, 19 May 2016 17:18:10 -0700 [thread overview]
Message-ID: <CAD=FV=Xv-c2owh+dDT1EJYPrKsahxOoQUhJiZFprtk4FxHAK1A@mail.gmail.com> (raw)
In-Reply-To: <1462871391-32484-1-git-send-email-shawn.lin@rock-chips.com>
Hi,
On Tue, May 10, 2016 at 2:09 AM, Shawn Lin <shawn.lin@rock-chips.com> 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.
I don't totally understand it, but two boards I'm working with have
eMMC devices that claim to be eMMC 5.0 devices but also have support
for strobe. Did people just cherry-pick this feature from eMMC 5.1 to
their eMMC 5.0 devices?
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
>
> Changes in v3: None
> Changes in v2: None
>
> drivers/mmc/core/bus.c | 3 +-
> drivers/mmc/core/core.c | 8 +++++
> drivers/mmc/core/mmc.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++--
> include/linux/mmc/card.h | 1 +
> include/linux/mmc/host.h | 11 +++++++
> include/linux/mmc/mmc.h | 3 ++
> 6 files changed, 102 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
> index 4bc48f1..7e94b9d 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" : "",
Nit: there should be a space before the quote. Otherwise the message
runs together, like:
new HS400 Enhanced strobeMMC card at address 0001
> 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..a559501 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1127,6 +1127,14 @@ 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->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..c2d1981 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 &&
> + (card_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,73 @@ out_err:
> return err;
> }
>
> +static int mmc_select_hs400es(struct mmc_card *card)
> +{
> + struct mmc_host *host = card->host;
> + int err = 0;
> + u8 val;
> +
> + 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;
> +
> + /* Switch card to DDR with strobe bit */
> + val = EXT_CSD_DDR_BUS_WIDTH_8 | EXT_CSD_BUS_WIDTH_STROBE;
In the hs400 function earlier in the file the check to confirm that
the bus width is 8 before doing this. Should we do that here, too?
> + 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);
> +
> + 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 +1370,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 +1379,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..f98bd0e 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 == true;
nit: never need "== true". Just:
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 */
nit: while your code should be OK, it seems better to be BIT(3), since
above is really:
0
BIT(0)
BIT(1)
BIT(2) | BIT(0)
BIT(2) | BIT(1)
...at least that's my understanding...
next prev parent reply other threads:[~2016-05-20 0:18 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-10 9:09 [PATCH v3 0/5] Add enhanced strobe support for emmc version 5.1 or later Shawn Lin
2016-05-10 9:09 ` Shawn Lin
2016-05-10 9:09 ` Shawn Lin
2016-05-10 9:09 ` [PATCH v3 1/5] Documentation: mmc: add mmc-hs400-enhanced-strobe Shawn Lin
2016-05-10 9:09 ` [PATCH v3 2/5] mmc: core: add mmc-hs400-enhanced-strobe support Shawn Lin
2016-05-10 9:09 ` Shawn Lin
2016-05-10 9:09 ` [PATCH v3 3/5] mmc: core: implement enhanced strobe support Shawn Lin
2016-05-10 9:09 ` Shawn Lin
2016-05-20 0:18 ` Doug Anderson [this message]
2016-05-20 1:48 ` Shawn Lin
2016-05-20 1:48 ` Shawn Lin
2016-05-20 2:19 ` Doug Anderson
2016-05-20 2:45 ` Jaehoon Chung
2016-05-20 3:15 ` Shawn Lin
2016-05-20 3:54 ` Jaehoon Chung
2016-05-20 4:15 ` Shawn Lin
2016-05-10 9:09 ` [PATCH v3 4/5] mmc: debugfs: add HS400 enhanced strobe description Shawn Lin
2016-05-10 9:09 ` Shawn Lin
2016-05-10 9:10 ` [PATCH v3 5/5] mmc: sdhci-of-arasan: implement enhanced strobe callback Shawn Lin
2016-05-10 9:10 ` Shawn Lin
2016-05-20 0:11 ` Doug Anderson
2016-05-20 1:49 ` Shawn Lin
2016-05-20 1:49 ` Shawn Lin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAD=FV=Xv-c2owh+dDT1EJYPrKsahxOoQUhJiZFprtk4FxHAK1A@mail.gmail.com' \
--to=dianders@chromium.org \
--cc=adrian.hunter@intel.com \
--cc=devicetree@vger.kernel.org \
--cc=heiko@sntech.de \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mmc@vger.kernel.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=robh+dt@kernel.org \
--cc=shawn.lin@rock-chips.com \
--cc=ulf.hansson@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.