Hi, Alex. On 11/27/2014 10:59 PM, Alex Lemberg wrote: > 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? Sorry, i missed that checking EXT_CSD_PSA_POST_SOLDERING_WIRTES. Then you need to relocated this at correct place.(To get ext_csd value), right? Best Regards, Jaehoon Chung > >> >>> >>> >>>>> + 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
