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.ch...@samsung.com]
>> Sent: Wednesday, November 26, 2014 6:33 AM
>> To: Alex Lemberg; ulf.hans...@linaro.org
>> Cc: linux-mmc@vger.kernel.org; ch...@printf.net; 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/flash-
>>> memory-ssds-ufs-emmc/e-mmc
>>>
>>> Signed-off-by: Alex Lemberg <alex.lemb...@sandisk.com>
>>> ---
>>> 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.
>>
>>> + }
>>> }
>>> 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.
>
>
>>> + 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
