Re: [PATCH v5 3/5] mmc: core: implement enhanced strobe support

[Jaehoon Chung]

On 05/23/2016 01:13 PM, 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@xxxxxxxxxxxxxx>
> ---
> 
> 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);


I wonder what is enhanced_strobe's purpose.
When i saw your patches, this is used to check whether timing is strobe or not in sdhci-of-arasan, right?
If ios.enhanced_strobe is used only this purpose, is it possible to use the below?

host->ops->hs400_enhanced_strobe(host, false/true); ?

This callback is also called only two locations..init (false) and switch hs400es (true).

>  
>  	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;
> +
> +	/* 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);
> +
> +	err = mmc_switch_status(card);
> +	if (err)
> +		goto out_err;
> +
> +	return 0;
> +
> +out_err:

If ios.ehanced_strobe is really needs, you need to set from true to false at here?

Best Regards,
Jaehoon Chung

> +	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 */
>