On Thu, Jun 21, 2012 at 4:38 PM, Subhash Jadavani <subhashj@xxxxxxxxxxxxxx> wrote: > > >> -----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, it has CMD13 - and it makes sense. > 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 I think that's the case with OMAP, and I assumed it to be common. > 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. > Yes - that was atleast the intention. I didn't assume DAT0 de assertation was auto detectable, and even otherwise, PRG state was to be polled later anyway. > 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. Ok- I'll send out an updated version. -- 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
