Message-ID: <d566fb19-2565-66d8-023a-bd9a7ef987b6@kernel-upstream.org>
Date:	Tue, 10 May 2016 16:29:52 +0800
From:	Shawn Lin <shawn.lin@...nel-upstream.org>
To:	Adrian Hunter <adrian.hunter@...el.com>,
	Shawn Lin <shawn.lin@...k-chips.com>,
	Ulf Hansson <ulf.hansson@...aro.org>
Cc:	devicetree@...r.kernel.org, Doug Anderson <dianders@...omium.org>,
	Heiko Stuebner <heiko@...ech.de>, linux-mmc@...r.kernel.org,
	Michal Simek <michal.simek@...inx.com>,
	linux-kernel@...r.kernel.org,
	Jaehoon Chung <jh80.chung@...sung.com>,
	linux-rockchip@...ts.infradead.org,
	Rob Herring <robh+dt@...nel.org>, soren.brinkmann@...inx.com
Subject: Re: [PATCH v2 3/6] mmc: core: implement enhanced strobe support

On 2016/5/9 19:56, Adrian Hunter wrote:
> On 29/04/16 05:47, 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>
>
> Thanks for doing this.  There are a couple of comments below, but generally
> I tend to think you should split out selecting HS400ES as a separate function.
>
>> ---
>>
>> Changes in v2: None
>>
>>  drivers/mmc/core/bus.c   |  3 +-
>>  drivers/mmc/core/mmc.c   | 77 ++++++++++++++++++++++++++++++++++++++++++++----
>>  include/linux/mmc/card.h |  1 +
>>  include/linux/mmc/host.h | 12 ++++++++
>>  include/linux/mmc/mmc.h  |  3 ++
>>  5 files changed, 89 insertions(+), 7 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" : "",
>>  			mmc_card_ddr52(card) ? "DDR " : "",
>>  			uhs_bus_speed_mode, type, card->rca);
>>  	}
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 28b477d..f45ed10 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -235,6 +235,12 @@ static void mmc_select_card_type(struct mmc_card *card)
>>  		avail_type |= EXT_CSD_CARD_TYPE_HS400_1_2V;
>>  	}
>>
>> +	if ((caps2 & MMC_CAP2_HS400_ENHANCED_STROBE) &&
>> +	    card->ext_csd.strobe_support &&
>> +	    ((card_type & EXT_CSD_CARD_TYPE_HS400_1_2V) ||
>> +	     (card_type & EXT_CSD_CARD_TYPE_HS400_1_8V)))
>
> Could use just "card_type & EXT_CSD_CARD_TYPE_HS400" here

okay, EXT_CSD_CARD_TYPE_HS400 contains 1_2V & 1_8V, so I will
use it.

>
>> +		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 +389,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);
>>
>> @@ -1062,9 +1069,10 @@ static int mmc_select_hs400(struct mmc_card *card)
>>  	u8 val;
>>
>>  	/*
>> -	 * HS400 mode requires 8-bit bus width
>> +	 * HS400(ES) mode requires 8-bit bus width
>>  	 */
>> -	if (!(card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400 &&
>> +	if (!(((card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400) ||
>> +		(card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES)) &&
>>  	      host->ios.bus_width == MMC_BUS_WIDTH_8))
>>  		return 0;
>>
>> @@ -1097,9 +1105,26 @@ static int mmc_select_hs400(struct mmc_card *card)
>>  	}
>>
>>  	/* Switch card to DDR */
>> +	val = EXT_CSD_DDR_BUS_WIDTH_8;
>> +	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
>> +		val |= EXT_CSD_BUS_WIDTH_STROBE;
>> +		/*
>> +		* Make sure we are in non-enhanced strobe mode before we
>> +		* actually enable it in ext_csd.
>> +		*/
>> +		if (host->ops->prepare_enhanced_strobe)
>> +			err = host->ops->prepare_enhanced_strobe(host, false);
>> +
>> +		if (err) {
>> +			pr_err("%s: unprepare_enhanced strobe failed, err:%d\n",
>> +				mmc_hostname(host), err);
>> +			return err;
>> +		}
>> +	}
>> +
>>  	err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
>>  			 EXT_CSD_BUS_WIDTH,
>> -			 EXT_CSD_DDR_BUS_WIDTH_8,
>> +			 val,
>>  			 card->ext_csd.generic_cmd6_time);
>>  	if (err) {
>>  		pr_err("%s: switch to bus width for hs400 failed, err:%d\n",
>> @@ -1124,6 +1149,35 @@ static int mmc_select_hs400(struct mmc_card *card)
>>  	mmc_set_timing(host, MMC_TIMING_MMC_HS400);
>>  	mmc_set_bus_speed(card);
>>
>> +	if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
>> +		/* Controller enable enhanced strobe function */
>> +		if (host->ops->prepare_enhanced_strobe)
>> +			err = host->ops->prepare_enhanced_strobe(host, true);
>> +
>> +		if (err) {
>> +			pr_err("%s: prepare enhanced strobe failed, err:%d\n",
>> +				mmc_hostname(host), err);
>> +			return err;
>> +		}
>> +
>> +		/*
>> +		 * After switching to hs400 enhanced strobe mode, we expect to
>> +		 * verify whether it works or not. If controller can't handle
>> +		 * bus width test, compare ext_csd previously read in 1 bit mode
>> +		 * against ext_csd at new bus width
>> +		 */
>
> I don't think we need this.  Just checking (host->caps & MMC_CAP_8_BIT_DATA)
> ought to be enough.  But if you want to play if safe, you could use
> mmc_select_bus_width() in advance.

Ulf also questioned the requirment of doing this.
I consider droping this checking for the next version.

>
>> +		if (!(host->caps & MMC_CAP_BUS_WIDTH_TEST))
>> +			err = mmc_compare_ext_csds(card, MMC_BUS_WIDTH_8);
>> +		else
>> +			err = mmc_bus_test(card, MMC_BUS_WIDTH_8);
>> +
>> +		if (err) {
>> +			pr_warn("%s: switch to enhanced strobe failed\n",
>> +				mmc_hostname(host));
>> +			return err;
>> +		}
>> +	}
>> +
>>  	if (!send_status) {
>>  		err = mmc_switch_status(card);
>>  		if (err)
>> @@ -1297,7 +1351,8 @@ err:
>>  }
>>
>>  /*
>> - * Activate High Speed or HS200 mode if supported.
>> + * Activate High Speed or HS200 mode if supported. For HS400
>> + * with enhanced strobe mode, we should activate High Speed.
>>   */
>>  static int mmc_select_timing(struct mmc_card *card)
>>  {
>> @@ -1306,7 +1361,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_hs(card);
>
> In my view, you should just make a new function: mmc_select_hs400es()

I will spilt a new function to handle the hs400es timing slection.

>
>> +	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);
>> @@ -1578,7 +1635,15 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
>>  	} else if (mmc_card_hs(card)) {
>>  		/* Select the desired bus width optionally */
>>  		err = mmc_select_bus_width(card);
>> -		if (!IS_ERR_VALUE(err)) {
>> +		if (IS_ERR_VALUE(err))
>> +			goto free_card;
>> +
>> +		if (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES) {
>> +			/* Directly from HS to HS400-ES */
>> +			err = mmc_select_hs400(card);
>> +			if (err)
>> +				goto free_card;
>> +		} else {
>>  			err = mmc_select_hs_ddr(card);
>>  			if (err)
>>  				goto free_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 11e89d3..19de56c 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 {
>> @@ -143,6 +144,8 @@ 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 */
>> +	int	(*prepare_enhanced_strobe)(struct mmc_host *host, bool enable);
>
> I don't see any need for an error return, so maybe make this return void.
> Also, we could put "bool enhanced_strobe" in struct mmc_ios and pass ios
> instead of enable.

yep, currently there's no need to check it.
I will change the name to hs400_enhacned_strobe as Ulf's suggestion,
and pass mmc_ios to this callback as well.

>
>>  	int	(*select_drive_strength)(struct mmc_card *card,
>>  					 unsigned int max_dtr, int host_drv,
>>  					 int card_drv, int *drv_type);
>> @@ -518,6 +521,15 @@ 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)
>> +{
>> +	if (mmc_card_hs400(card) &&
>> +	    (card->mmc_avail_type & EXT_CSD_CARD_TYPE_HS400ES))
>> +		return 1;
>
> We shouldn't assume that we are using it, just because it is available.
> How about putting "bool enhanced_strobe" in struct mmc_ios and checking that.
>

Good catch. Thanks.

>> +
>> +	return 0;
>> +}
>> +
>>  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 */
>>
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>

Powered by blists - more mailing lists