> -----Original Message----- > From: linux-mmc-owner@xxxxxxxxxxxxxxx [mailto:linux-mmc- > owner@xxxxxxxxxxxxxxx] On Behalf Of S, Venkatraman > Sent: Thursday, June 21, 2012 12:55 PM > To: Subhash Jadavani > Cc: cjb@xxxxxxxxxx; linux-mmc@xxxxxxxxxxxxxxx; linkinjeon@xxxxxxxxx; > jh80.chung@xxxxxxxxxxx; alex.lemberg@xxxxxxxxxxx; Kostya > Subject: Re: [PATCH v3] mmc: core: Fix the HPI execution sequence > > On Wed, Jun 20, 2012 at 11:29 PM, Subhash Jadavani > <subhashj@xxxxxxxxxxxxxx> wrote: > > Hi Venkatraman, > > > >> -----Original Message----- > >> From: linux-mmc-owner@xxxxxxxxxxxxxxx [mailto:linux-mmc- > >> owner@xxxxxxxxxxxxxxx] On Behalf Of Venkatraman S > >> Sent: Wednesday, June 20, 2012 3:04 PM > >> To: cjb@xxxxxxxxxx > >> Cc: linux-mmc@xxxxxxxxxxxxxxx; linkinjeon@xxxxxxxxx; > >> jh80.chung@xxxxxxxxxxx; alex.lemberg@xxxxxxxxxxx; Venkatraman S; > >> Kostya > >> Subject: [PATCH v3] mmc: core: Fix the HPI execution sequence > >> > >> mmc_execute_hpi should send the HPI command only once, and only if > >> the card is in PRG state. > >> > >> According to eMMC spec, the command's completion time is not > >> dependent on OUT_OF_INTERRUPT_TIME. Only the transition out of PRG > >> STATE is guarded by OUT_OF_INTERRUPT_TIME - which is defined to begin > >> at the end of sending the command itself. > >> > >> Specify the default timeout for the actual sending of HPI command, > >> and > > then > >> use OUT_OF_INTERRUPT_TIME to wait for the transition out of PRG state. > > > > I guess this is not the correct interpretation. cmd.cmd_timeout_ms > > should specify for how much time host driver should wait for command + > > busy line deassertion. So why are you keeping the timeout calculation > > after the command is completed? > > > If what you stay is correct, the card should be in PRG state after > mmc_send_hpi_cmd() > returns. > > I understood and observed it as opposite. HPI cmd returns immediately, and the > device takes OUT_OF_INTERRUPT_TIME time to come out of PRG state. > > My experiments / tests confirm that behavior. It takes a few ms (and several > SEND_STATUS messages later), after HPI, for the card to be out of PRG state. What is the HPI command in your case? CMD12 or CMD13? For command CMD12, we set the R1B flag which would tell the host driver to wait for the DAT0 line to be deasserted (if host controller support the automatic detection of DAT0 de assertion). But HPI command is CMD13 then yes, driver will not wait for DAT0 line de assertion. Yes, so to support both CMD12 and CMD13 as HPI command, we can have 2 options: 1. - Set the R1B flag for CMD13 as well so controller waits for the PROG_DONE. - Once mmc_send_hpi_cmd() returns, you can poll (without any timeout) until the card is out of programming state. 2. - don't set the R1B flag for CMD13 - Once mmc_send_hpi_cmd() returns, if HPI command is CMD12, you can poll (without any timeout) until the card is out of programming state. But if HPI command is CMD13, poll until card is out of programming state with timeout. But in both of the above case, if host driver do not support automatic detection of DAT0 de assertion, we will have to send multiple CMD13 (send_status) before card comes out of programming state but yes, in case card gets stuck in programming state forever then we will end up sending CMD13 forever. In this case timeout based wait is fine. So if you want to take of case where card is stuck in programming state forever, your implementation is fine. Is this your intention here? I have few comments on your code below. Regards, Subhash > > > mmc_send_hpi_cmd() returns means the host controller driver have > > ensured that BUSY indication is removed so after mmc_send_hpi_cmd(), > > if you want to check if the card is still in PROGRAMMING state, it > > should be unconditional check in loop (rather than timeout based) > > until card is out of programming state. > > > > To me, mmc_send_hpi_cmd() implementation should not be anything > > different than mmc_switch(). In mmc_switch() also, after > > mmc_wait_for_cmd() returns it continuously keeps checking if card is > programming or not. > > > > Regards, > > Subhash > > > >> > >> Reported-by: Alex Lemberg <Alex.Lemberg@xxxxxxxxxxx> > >> Signed-off-by: Venkatraman S <svenkatr@xxxxxx> > >> Reviewed-by: Namjae Jeon <linkinjeon@xxxxxxxxx> > >> Acked-by: Jaehoon Chung <jh80.chung@xxxxxxxxxxx> > >> CC: Kostya <kdorfman@xxxxxxxxxxxxxx> > >> --- > >> v2 discussions: http://marc.info/?l=linux-mmc&m=133657255616390&w=2 > >> > >> v2->v3: > >> Return gracefully for card idle states (suggested by kdorfman) > >> > >> drivers/mmc/core/core.c | 57 > > ++++++++++++++++++++++++++----------------- > >> - > >> drivers/mmc/core/mmc_ops.c | 1 - > >> 2 files changed, 34 insertions(+), 24 deletions(-) > >> > >> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index > >> 80ec427..08ed144 100644 > >> --- a/drivers/mmc/core/core.c > >> +++ b/drivers/mmc/core/core.c > >> @@ -404,6 +404,7 @@ int mmc_interrupt_hpi(struct mmc_card *card) { > >> int err; > >> u32 status; > >> + unsigned long starttime; > >> > >> BUG_ON(!card); > >> > >> @@ -419,30 +420,40 @@ int mmc_interrupt_hpi(struct mmc_card *card) > >> goto out; > >> } > >> > >> - /* > >> - * If the card status is in PRG-state, we can send the HPI command. > >> - */ > >> - if (R1_CURRENT_STATE(status) == R1_STATE_PRG) { > >> - do { > >> - /* > >> - * We don't know when the HPI command will > >> finish > >> - * processing, so we need to resend HPI until > >> out > >> - * of prg-state, and keep checking the card > >> status > >> - * with SEND_STATUS. If a timeout error occurs > >> when > >> - * sending the HPI command, we are already out > >> of > >> - * prg-state. > >> - */ > >> - err = mmc_send_hpi_cmd(card, &status); > >> - if (err) > >> - pr_debug("%s: abort HPI (%d error)\n", > >> - mmc_hostname(card->host), > >> err); > >> + switch (R1_CURRENT_STATE(status)) { > >> > >> - err = mmc_send_status(card, &status); > >> - if (err) > >> - break; > >> - } while (R1_CURRENT_STATE(status) == R1_STATE_PRG); > >> - } else > >> - pr_debug("%s: Left prg-state\n", > >> mmc_hostname(card->host)); > >> + case R1_STATE_IDLE: > >> + case R1_STATE_READY: > >> + case R1_STATE_STBY: /* intentional fall through */ > >> + /* In idle states, HPI is not needed and the caller > >> + can issue the next intended command immediately */ > >> + goto out; > >> + break; > >> + case R1_STATE_PRG: > >> + break; > >> + default: > >> + /* In all other states, it's illegal to issue HPI */ > >> + pr_debug("%s: HPI cannot be sent. Card state=%d\n", > >> + mmc_hostname(card->host), > >> R1_CURRENT_STATE(status)); > >> + err = -EINVAL; > >> + goto out; > >> + break; > >> + } > >> + > >> + starttime = jiffies; > >> + err = mmc_send_hpi_cmd(card, &status); > >> + if (err) > >> + goto out; > >> + > >> + do { > >> + err = mmc_send_status(card, &status); > >> + > >> + if (!err && R1_CURRENT_STATE(status) == R1_STATE_TRAN) > >> + break; > >> + if (jiffies_to_msecs(jiffies - starttime) >= > >> + card->ext_csd.out_of_int_time) When you are comparing snapshot of 2 different jiffies, you should be using time_after() or time_before() macros others this will not take case of jiffies wraparound. > >> + err = -ETIMEDOUT; > >> + } while (!err); > >> > >> out: > >> mmc_release_host(card->host); > >> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c > >> index 69370f4..0ed2cc5 100644 > >> --- a/drivers/mmc/core/mmc_ops.c > >> +++ b/drivers/mmc/core/mmc_ops.c > >> @@ -569,7 +569,6 @@ int mmc_send_hpi_cmd(struct mmc_card *card, > u32 > >> *status) > >> > >> cmd.opcode = opcode; > >> cmd.arg = card->rca << 16 | 1; > >> - cmd.cmd_timeout_ms = card->ext_csd.out_of_int_time; > >> > >> err = mmc_wait_for_cmd(card->host, &cmd, 0); > >> if (err) { > >> -- > >> 1.7.10.rc2 > >> > >> -- > >> 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 > -- > 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 -- 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
