Hi Jaehoon, > -----Original Message----- > From: Jaehoon Chung [mailto:jh80.chung@xxxxxxxxxxx] > Sent: Thursday, November 27, 2014 4:42 PM > 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 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/fl > >>>>> as > >>>>> 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? > Right, I already did this in "PATCH v3" and re-submitted. Thanks again for your review! > 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
