Hi Jaehoon, > -----Original Message----- > From: Jaehoon Chung [mailto:jh80.chung@xxxxxxxxxxx] > Sent: Thursday, November 27, 2014 11:02 AM > To: Alex Lemberg > Cc: ulf.hansson@xxxxxxxxxx; linux-mmc@xxxxxxxxxxxxxxx; chris@xxxxxxxxxx; Avi > Shchislowski > Subject: Re: [PATCH v2] mmc: Add Production State Awareness Support > > Hi, Alex. > > On 11/27/2014 01:38 AM, Alex Lemberg wrote: > > Hi Jaehoon, > > > > Thank you for reviewing the patch. > > > >> -----Original Message----- > >> From: Jaehoon Chung [mailto:jh80.chung@xxxxxxxxxxx] > >> Sent: Wednesday, November 26, 2014 6:33 AM > >> To: Alex Lemberg; ulf.hansson@xxxxxxxxxx > >> Cc: linux-mmc@xxxxxxxxxxxxxxx; chris@xxxxxxxxxx; Avi Shchislowski > >> Subject: Re: [PATCH v2] mmc: Add Production State Awareness Support > >> > >> Hi, > >> > >> On 11/26/2014 12:01 AM, Alex Lemberg wrote: > >>> In this patch driver should recognize if eMMC device (Rev >=5.0) was > >>> left in PRE_SOLDERING_POST_WRITES (0x02) state, and switch it to > >>> NORMAL (0x00). > >>> PRE_SOLDERING_POST_WRITES (0x02) state - represents a state where > >>> the device is in production process and the host (usually > >>> programmer) completed loading the content to the device. > >>> The host (usually programmer) sets the device to this state after > >>> loading the content and just before soldering. > >>> After soldering the device to the real host (not programmer), the > >>> device should be switched to NORMAL (0x00) mode. > >>> The NORMAL (0x00) mode of PSA register represents a state in which > >>> the device is running in the field and uses regular operations. > >>> Leaving device in PRE_SOLDERING_POST_WRITES (0x02) might cause > >>> unexpected behaviour of eMMC device. > >>> > >>> More details about PSA feature can be found in eMMC5.0 JEDEC spec > >> (JESD84-B50.pdf): > >>> http://www.jedec.org/standards-documents/technology-focus-areas/flas > >>> h- > >>> memory-ssds-ufs-emmc/e-mmc > >>> > >>> Signed-off-by: Alex Lemberg <alex.lemberg@xxxxxxxxxxx> > >>> --- > >>> Changes in v2: > >>> - Remove typo in patch code > >>> --- > >>> drivers/mmc/core/mmc.c | 28 ++++++++++++++++++++++++++++ > >>> include/linux/mmc/card.h | 2 ++ > >>> include/linux/mmc/mmc.h | 8 ++++++++ > >>> 3 files changed, 38 insertions(+) > >>> > >>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index > >>> 02ad792..3923c90 100644 > >>> --- a/drivers/mmc/core/mmc.c > >>> +++ b/drivers/mmc/core/mmc.c > >>> @@ -571,6 +571,16 @@ static int mmc_decode_ext_csd(struct mmc_card > >> *card, u8 *ext_csd) > >>> card->ext_csd.ffu_capable = > >>> (ext_csd[EXT_CSD_SUPPORTED_MODE] & 0x1) && > >>> !(ext_csd[EXT_CSD_FW_CONFIG] & 0x1); > >>> + card->ext_csd.psa = > >>> + ext_csd[EXT_CSD_PSA]; > >>> + if (ext_csd[EXT_CSD_PSA_TIMEOUT] > 0) { > >>> + card->ext_csd.psa_timeout = > >>> + 100 * > >>> + (1 << ext_csd[EXT_CSD_PSA_TIMEOUT]); > >>> + } else { > >>> + pr_warn("%s: EXT_CSD PSA Timeout is zero\n", > >>> + mmc_hostname(card->host)); > >> > >> I remembered Ulf's previous comment. Did you check it? > > Are you referring to psa_timeout? > > In case it is zero, we are printing warning, and let host controller > > to handle this. > > Every switch command timeout setting in mmc driver done in this way. > > pr_warn() doesn't need. OK > > >> > >>> + } > >>> } > >>> out: > >>> return err; > >>> @@ -1331,6 +1341,24 @@ static int mmc_init_card(struct mmc_host > >>> *host, > >> u32 ocr, > >>> mmc_set_dsr(host); > >>> > >>> /* > >>> + * eMMC v5.0 or later > >>> + * and Production State Awareness state is > >>> + * EXT_CSD_PSA_POST_SOLDERING_WRITES (0x02) > >>> + * The host should set the device to NORMAL mode > >>> + */ > >>> + if ((card->ext_csd.rev >= 7) && > >>> + (card->ext_csd.psa == > >> EXT_CSD_PSA_POST_SOLDERING_WRITES)) { > >> > >> Is it right? how did you get "revision"? > > > > In linux-next, the EXT_CSD revision set was moved to > > mmc_decode_ext_csd() function. > > Anyway, thanks for pointing me on this, I will move this check to the > > right place. > > > >> > >>> + unsigned int timeout; > >>> + > >>> + timeout = DIV_ROUND_UP(card->ext_csd.psa_timeout, 1000); > >>> + card->ext_csd.psa = EXT_CSD_PSA_NORMAL; > >> > >> Why did card->ext_csd.psa assign to EXT_CSD_PSA_NORMAL as hard- > coding? > >> > > I think this is a right way to set ext_csd structure in order to > > prevent additional read from device ext_csd register, which was already read > during the init. > > I think you assume that card is already initialized. > > In my understanding for your code. > a) init time > card->ext_csd.psa assigned to EXT_CSD_PSA_NORMAL. (hard coding) > And switch to EXT_CSD_PSA_NORMAL. > b) If oldcard doesn't present, try to read ext_csd register. > Then EXT_CSD_PSA was always set to EXT_CSD_PSA_NORMAL. isn't? > And card->ext_csd.psa is re-assigned to EXT_CSD_PSA register value. > (It's also EXT_CSD_PSA_NORMAL, because it's switched to > EXT_CSD_PSA_NORMA at 'a)' ) > > If i missed something, let me know. 'b)' is performed before 'a)' The card->ext_csd.psa is read from device before we check and set it to NORMAL. The code flow is: mmc_init_card(...) { ... 1) If oldcard doesn't present, try to read ext_csd register. The card->ext_csd.psa will be set with device register value ... 2) if card->ext_csd.psa is EXT_CSD_PSA_POST_SOLDERING_WRITES Set card->ext_csd.psa to EXT_CSD_PSA_NORMAL. (hard coding) And switch to EXT_CSD_PSA_NORMAL ... } Does it makes more sense now? > > > > > > >>> + err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, > >>> + EXT_CSD_PSA, card->ext_csd.psa, timeout); > > If you need to change, > err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, > EXT_CSD_PSA, > EXT_CSD_PSA_NORMAL, timeout); > > Anyway, this sequence is something strange. > > Best Regards, > Jaehoon Chung > >>> + if (err && err != -EBADMSG) > >>> + goto free_card; > >>> + } > >>> + > >>> + /* > >>> * Select card, as all following commands rely on that. > >>> */ > >>> if (!mmc_host_is_spi(host)) { > >>> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h > >>> index > >>> 4d69c00..09ac3b0 100644 > >>> --- a/include/linux/mmc/card.h > >>> +++ b/include/linux/mmc/card.h > >>> @@ -60,9 +60,11 @@ struct mmc_ext_csd { > >>> u8 packed_event_en; > >>> unsigned int part_time; /* Units: ms */ > >>> unsigned int sa_timeout; /* Units: 100ns */ > >>> + unsigned int psa_timeout; /* Units: 100us */ > >>> unsigned int generic_cmd6_time; /* Units: 10ms */ > >>> unsigned int power_off_longtime; /* Units: ms */ > >>> u8 power_off_notification; /* state */ > >>> + u8 psa; /* production state awareness */ > >>> unsigned int hs_max_dtr; > >>> unsigned int hs200_max_dtr; > >>> #define MMC_HIGH_26_MAX_DTR 26000000 > >>> diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h index > >>> 49ad7a9..458814d 100644 > >>> --- a/include/linux/mmc/mmc.h > >>> +++ b/include/linux/mmc/mmc.h > >>> @@ -285,6 +285,7 @@ struct _mmc_csd { > >>> #define EXT_CSD_EXP_EVENTS_STATUS 54 /* RO, 2 bytes */ > >>> #define EXT_CSD_EXP_EVENTS_CTRL 56 /* R/W, 2 > bytes */ > >>> #define EXT_CSD_DATA_SECTOR_SIZE 61 /* R */ > >>> +#define EXT_CSD_PSA 133 /* R/W/E */ > >>> #define EXT_CSD_GP_SIZE_MULT 143 /* R/W */ > >>> #define EXT_CSD_PARTITION_SETTING_COMPLETED 155 /* R/W */ > >>> #define EXT_CSD_PARTITION_ATTRIBUTE 156 /* R/W */ > >>> @@ -315,6 +316,7 @@ struct _mmc_csd { > >>> #define EXT_CSD_PWR_CL_26_360 203 /* RO */ > >>> #define EXT_CSD_SEC_CNT 212 /* RO, 4 bytes */ > >>> #define EXT_CSD_S_A_TIMEOUT 217 /* RO */ > >>> +#define EXT_CSD_PSA_TIMEOUT 218 /* RO */ > >>> #define EXT_CSD_REL_WR_SEC_C 222 /* RO */ > >>> #define EXT_CSD_HC_WP_GRP_SIZE 221 /* RO */ > >>> #define EXT_CSD_ERASE_TIMEOUT_MULT 223 /* RO */ > >>> @@ -433,6 +435,12 @@ struct _mmc_csd { > >>> #define EXT_CSD_BKOPS_LEVEL_2 0x2 > >>> > >>> /* > >>> + * PRODUCTION STATE AWARENESS fields */ > >>> +#define EXT_CSD_PSA_NORMAL 0x00 > >>> +#define EXT_CSD_PSA_POST_SOLDERING_WRITES 0x02 > >>> + > >>> +/* > >>> * MMC_SWITCH access modes > >>> */ > >>> > >>> > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
