From mboxrd@z Thu Jan 1 00:00:00 1970 From: Seungwon Jeon Subject: RE: [PATCH 3/3] mmc: dw_mmc: Support voltage changes Date: Tue, 08 Jul 2014 18:56:48 +0900 Message-ID: <003701cf9a92$ef2f2220$cd8d6660$%jun@samsung.com> References: <1403520321-2431-1-git-send-email-yuvaraj.cd@samsung.com> <1403520321-2431-4-git-send-email-yuvaraj.cd@samsung.com> <001501cf9076$9de45000$d9acf000$%jun@samsung.com> <005001cf912b$27fb8510$77f28f30$%jun@samsung.com> <001101cf91f9$84111530$8c333f90$%jun@samsung.com> <001b01cf976f$e91671c0$bb435540$%jun@samsung.com> <000b01cf99a3$85a2c630$90e85290$%jun@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Return-path: Received: from mailout2.samsung.com ([203.254.224.25]:59597 "EHLO mailout2.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753833AbaGHJ4v (ORCPT ); Tue, 8 Jul 2014 05:56:51 -0400 In-reply-to: Content-language: ko Sender: linux-mmc-owner@vger.kernel.org List-Id: linux-mmc@vger.kernel.org To: 'Yuvaraj Kumar' Cc: 'Doug Anderson' , 'linux-samsung-soc' , linux-arm-kernel@lists.infradead.org, 'Jaehoon Chung' , 'Chris Ball' , linux-mmc@vger.kernel.org, 'Ulf Hansson' , 'Sonny Rao' , 'Tomasz Figa' , 'Kukjin Kim' , 'sunil joshi' , ks.giri@samsung.com, 'Prashanth G' , 'Alim Akhtar' , 'Yuvaraj Kumar C D' On Tue, July 08, 2014,Yuvaraj Kumar wrote: > On Mon, Jul 7, 2014 at 10:53 AM, Seungwon Jeon wrote: > > On Fri, July 04, 2014, Seungwon Jeon wrote: > >> On Tue, July 01, 2014. Yuvaraj Kumar wrote: > >> > On Fri, Jun 27, 2014 at 4:48 PM, Seungwon Jeon wrote: > >> > > Hi Yuvaraj, > >> > > > >> > > On Fri, June 27, 2014, Yuvaraj Kumar wrote: > >> > >> On Thu, Jun 26, 2014 at 10:20 PM, Doug Anderson wrote: > >> > >> > Seungwon, > >> > >> > > >> > >> > On Thu, Jun 26, 2014 at 3:41 AM, Seungwon Jeon wrote: > >> > >> >> On Thu, June 26, 2014, Doug Anderson wrote: > >> > >> >>> Seungwon, > >> > >> >>> > >> > >> >>> On Wed, Jun 25, 2014 at 6:08 AM, Seungwon Jeon wrote: > >> > >> >>> > On Mon, June 23, 2014, Yuvaraj Kumar C D wrote: > >> > >> >>> >> Subject: [PATCH 3/3] mmc: dw_mmc: Support voltage changes > >> > >> >>> >> > >> > >> >>> >> From: Doug Anderson > >> > >> >>> >> > >> > >> >>> >> For UHS cards we need the ability to switch voltages from 3.3V to > >> > >> >>> >> 1.8V. Add support to the dw_mmc driver to handle this. Note that > >> > >> >>> >> dw_mmc needs a little bit of extra code since the interface needs a > >> > >> >>> >> special bit programmed to the CMD register while CMD11 is progressing. > >> > >> >>> >> This means adding a few extra states to the state machine to track. > >> > >> >>> > > >> > >> >>> > Overall new additional states makes it complicated. > >> > >> >>> > Can we do that in other way? > >> > >> >>> > >> > >> >>> That was the best I was able to figure out when I thought this > >> > >> >>> through. If you have ideas for doing it another way I'd imagine that > >> > >> >>> Yuvaraj would be happy to take your feedback. > >> > >> >> Let's clean up SDMMC_CMD_VOLT_SWITCH. > >> > >> >> In turn, we may remove state-handling simply. > >> > >> >> > >> > >> >>> > >> > >> >>> > >> > >> >>> >> Signed-off-by: Doug Anderson > >> > >> >>> >> Signed-off-by: Yuvaraj Kumar C D > >> > >> >>> >> > >> > >> >>> >> --- > >> > >> >>> >> drivers/mmc/host/dw_mmc.c | 145 +++++++++++++++++++++++++++++++++++++++++--- > >> > >> >>> >> drivers/mmc/host/dw_mmc.h | 5 +- > >> > >> >>> >> include/linux/mmc/dw_mmc.h | 2 + > >> > >> >>> >> 3 files changed, 142 insertions(+), 10 deletions(-) > >> > >> >>> >> > >> > >> >>> >> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > >> > >> >>> >> index e034bce..38eb548 100644 > >> > >> >>> >> --- a/drivers/mmc/host/dw_mmc.c > >> > >> >>> >> +++ b/drivers/mmc/host/dw_mmc.c > >> > >> >>> >> @@ -29,6 +29,7 @@ > >> > >> >>> >> #include > >> > >> >>> >> #include > >> > >> >>> >> #include > >> > >> >>> >> +#include > >> > >> >>> >> #include > >> > >> >>> >> #include > >> > >> >>> >> #include > >> > >> >>> >> @@ -235,10 +236,13 @@ err: > >> > >> >>> >> } > >> > >> >>> >> #endif /* defined(CONFIG_DEBUG_FS) */ > >> > >> >>> >> > >> > >> >>> >> +static void mci_send_cmd(struct dw_mci_slot *slot, u32 cmd, u32 arg); > >> > >> >>> >> + > >> > >> >>> >> static u32 dw_mci_prepare_command(struct mmc_host *mmc, struct mmc_command *cmd) > >> > >> >>> >> { > >> > >> >>> >> struct mmc_data *data; > >> > >> >>> >> struct dw_mci_slot *slot = mmc_priv(mmc); > >> > >> >>> >> + struct dw_mci *host = slot->host; > >> > >> >>> >> const struct dw_mci_drv_data *drv_data = slot->host->drv_data; > >> > >> >>> >> u32 cmdr; > >> > >> >>> >> cmd->error = -EINPROGRESS; > >> > >> >>> >> @@ -254,6 +258,32 @@ static u32 dw_mci_prepare_command(struct mmc_host *mmc, struct > >> > mmc_command > >> > >> >>> *cmd) > >> > >> >>> >> else if (cmd->opcode != MMC_SEND_STATUS && cmd->data) > >> > >> >>> >> cmdr |= SDMMC_CMD_PRV_DAT_WAIT; > >> > >> >>> >> > >> > >> >>> >> + if (cmd->opcode == SD_SWITCH_VOLTAGE) { > >> > >> >>> >> + u32 clk_en_a; > >> > >> >>> >> + > >> > >> >>> >> + /* Special bit makes CMD11 not die */ > >> > >> >>> >> + cmdr |= SDMMC_CMD_VOLT_SWITCH; > >> > >> >>> >> + > >> > >> >>> >> + /* Change state to continue to handle CMD11 weirdness */ > >> > >> >>> >> + WARN_ON(slot->host->state != STATE_SENDING_CMD); > >> > >> >>> >> + slot->host->state = STATE_SENDING_CMD11; > >> > >> >>> >> + > >> > >> >>> >> + /* > >> > >> >>> >> + * We need to disable clock stop while doing voltage switch > >> > >> >>> >> + * according to 7.4.1.2 Voltage Switch Normal Scenario. > >> > >> >>> >> + * > >> > >> >>> >> + * It's assumed that by the next time the CLKENA is updated > >> > >> >>> >> + * (when we set the clock next) that the voltage change will > >> > >> >>> >> + * be over, so we don't bother setting any bits to synchronize > >> > >> >>> >> + * with dw_mci_setup_bus(). > >> > >> >>> >> + */ > >> > >> >>> >> + clk_en_a = mci_readl(host, CLKENA); > >> > >> >>> >> + clk_en_a &= ~(SDMMC_CLKEN_LOW_PWR << slot->id); > >> > >> >>> >> + mci_writel(host, CLKENA, clk_en_a); > >> > >> >>> >> + mci_send_cmd(slot, SDMMC_CMD_UPD_CLK | > >> > >> >>> >> + SDMMC_CMD_PRV_DAT_WAIT, 0); > >> > >> >>> > dw_mci_disable_low_power() can be used here. > >> > >> >>> > >> > >> >>> Ah. I guess we don't have that locally anymore. Locally we have variants on: > >> > >> >> I'm checking on cjb/mmc branch. > >> > >> >> > >> > >> >>> * https://patchwork.kernel.org/patch/3070311/ > >> > >> >>> * https://patchwork.kernel.org/patch/3070251/ > >> > >> >>> * https://patchwork.kernel.org/patch/3070221/ > >> > >> >>> > >> > >> >>> ...which removed that function. ...but I guess upstream never picked > >> > >> >>> up those patches, huh? Looking back it looks like you had some > >> > >> >>> feedback and it needed another spin but somehow fell off my plate. :( > >> > >> >>> > >> > >> >>> Maybe this is something Yuvaraj would like to pick up? > >> > >> >> It's long ago. I remember that there is no progress since my last comment. > >> > >> >> In case of patch "3070221", I want to pick up for next Kernel. > >> > >> > > >> > >> > Sounds like Yuvaraj has agreed to look at addressing your comments. Thanks! > >> > >> > > >> > >> > > >> > >> >>> >> + } > >> > >> >>> >> + > >> > >> >>> >> if (cmd->flags & MMC_RSP_PRESENT) { > >> > >> >>> >> /* We expect a response, so set this bit */ > >> > >> >>> >> cmdr |= SDMMC_CMD_RESP_EXP; > >> > >> >>> >> @@ -776,11 +806,15 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool > >> > force_clkinit) > >> > >> >>> >> unsigned int clock = slot->clock; > >> > >> >>> >> u32 div; > >> > >> >>> >> u32 clk_en_a; > >> > >> >>> >> + u32 sdmmc_cmd_bits = SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT; > >> > >> >>> >> + > >> > >> >>> >> + /* We must continue to set bit 28 in CMD until the change is complete */ > >> > >> >>> >> + if (host->state == STATE_WAITING_CMD11_DONE) > >> > >> >>> >> + sdmmc_cmd_bits |= SDMMC_CMD_VOLT_SWITCH; > >> > >> >>> > I didn't get the reason SDMMC_CMD_VOLT_SWITCH is needed during clock > update(enable/disable) > >> > >> >>> > Can you explain it in details? > >> > >> >>> > >> > >> >>> Simply put: if I didn't do this then the system hung during voltage > >> > >> >>> switch. It was not documented in the version of the IP manual that I > >> > >> >>> had access to and it took me a bunch of digging / trial and error to > >> > >> >>> figure this out. Whatever this bit does internally it's important to > >> > >> >>> set it while the voltage change is happening. Note that this need was > >> > >> >>> the whole reason for adding the extra state to the state machine. > >> > >> >>> > >> > >> >>> Perhaps Yuvaraj can try without it and I'd assume he'll report the > >> > >> >>> same freeze. > >> > >> >> Clarify the necessity of SDMMC_CMD_VOLT_SWITCH. > >> > >> >> As far as I experience, we didn't apply this bit for several projects. > >> > >> > > >> > >> > I just tried this locally on our ChromeOS 3.8 kernel (which has quite > >> > >> > a few backports and matches dw_mmc upstream pretty closely). When I > >> > >> > take out "sdmmc_cmd_bits |= SDMMC_CMD_VOLT_SWITCH" and bootup it fails > >> > >> > to bringup WiFi chip. It prints messages like: > >> > >> > > >> > >> > [ 4.400842] mmc_host mmc1: Timeout sending command (cmd 0x202000 > >> > >> > arg 0x0 status 0x80202000) > >> > >> > > >> > >> > I will let Yuvaraj comment about his testing too. > >> > >> To make it simple and verify, i have just enabled eMMC and SD channels > >> > >> on 3.16-rc1 and tested. > >> > >> Without "sdmmc_cmd_bits |= SDMMC_CMD_VOLT_SWITCH" bootup fails. > >> > >> [ 3.015971] mmc_host mmc1: Timeout sending command (cmd 0x202000 > >> > >> arg 0x0 status 0x80202000) > >> > >> [ 3.530974] mmc_host mmc1: Timeout sending command (cmd 0x202000 > >> > >> arg 0x0 status 0x80202000) > >> > > Thank you for test. > >> > > Hmm, it failed during clock update, right?. > >> > > Can you check HLE interrupt at this time? > >> > > I'll investigate for this. > >> > Yes, it failed during clock update.I am dumping the register contents > >> > just before the dev_err statement.I can observe that 1st timeout does > >> > not have HLE interrupt.But once we set bit 0 of UHS_REG subsequent > >> > timeout's have HLE bit set.I am attaching the complete log. > >> > >> Before disabling the clocks, we ensure that the card is not busy.(It's from Synopsys's databook) > >> I guess SDMMC_CMD_VOLT_SWITCH flag seems to affect to avoid this limitation if you have no problem. > >> But, I met the following log with HEL when I apply and test this patch . > >> > >> "mmc_host mmc1: Timeout sending command (cmd 0x202000 arg 0x0 status 0x80202000)" > >> I need to look into it more deeply. > > I confirmed that SDMMC_CMD_VOLT_SWITCH is needed during clock-off/on in voltage switching sequence > > even though there is no description for this in Synopsys's databook. > > And the above-mentioned message("mmc_host mmc1: Timeout sending command (cmd 0x202000 arg 0x0 status > 0x80202000)") > > is happened on mmc_power_cycle(), which should be done because of busy status of DAT[3:0]. > > It indicates clock-off/on sequence fails when executing mmc_power_cycle(). Do you have any idea for this error case? > > > >> > >> > > > >> > >> > > >> > >> > > >> > >> >>> >> +static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios) > >> > >> >>> >> +{ > >> > >> >>> >> + struct dw_mci_slot *slot = mmc_priv(mmc); > >> > >> >>> >> + struct dw_mci *host = slot->host; > >> > >> >>> >> + u32 uhs; > >> > >> >>> >> + u32 v18 = SDMMC_UHS_18V << slot->id; > >> > >> >>> >> + int min_uv, max_uv; > >> > >> >>> >> + int ret; > >> > >> >>> >> + > >> > >> >>> >> + /* > >> > >> >>> >> + * Program the voltage. Note that some instances of dw_mmc may use > >> > >> >>> >> + * the UHS_REG for this. For other instances (like exynos) the UHS_REG > >> > >> >>> >> + * does no harm but you need to set the regulator directly. Try both. > >> > >> >>> >> + */ > >> > >> >>> >> + uhs = mci_readl(host, UHS_REG); > >> > >> >>> >> + if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330) { > >> > >> >>> >> + min_uv = 2700000; > >> > >> >>> >> + max_uv = 3600000; > >> > >> >>> >> + uhs &= ~v18; > >> > >> >>> >> + } else { > >> > >> >>> >> + min_uv = 1700000; > >> > >> >>> >> + max_uv = 1950000; > >> > >> >>> >> + uhs |= v18; > >> > >> >>> >> + } > >> > >> >>> >> + if (!IS_ERR(mmc->supply.vqmmc)) { > >> > >> >>> >> + ret = regulator_set_voltage(mmc->supply.vqmmc, min_uv, max_uv); > >> > >> >>> >> + > >> > >> >>> >> + /* > >> > >> >>> >> + * Only complain if regulator claims that it's not in the 1.8V > >> > >> >>> >> + * range. This avoids a bunch of errors in the case that > >> > >> >>> >> + * we've got a fixed 1.8V regulator but the core SD code still > >> > >> >>> >> + * thinks it ought to try to switch to 3.3 and then back to 1.8 > >> > >> >>> >> + */ > >> > >> >>> >> + if (ret) { > >> > >> >>> > I think if ret is error, printing message and returning error is good. > >> > >> >>> > Currently, just returning '0' though it fails. > >> > >> >> Any feedback? > >> > >> > > >> > >> > Whoops, right. I think you're right that in the case it warns it > >> > >> > should also return the error code. In the case it doesn't warn it > >> > >> > shouldn't. Also: possibly we should use a trick like the mmc core > >> > >> > does and use "regulator_count_voltages" to figure out if we're going > >> > >> > to be able to switch voltages... > >> > >> > > >> > >> >>> > Artificially adding SDMMC_INT_CMD_DONE to pending is needed to complete cmd handling? > >> > >> >>> > Is there any reason? > >> > >> >>> > >> > >> >>> I don't remember this for sure since I wrote this a long time ago. > >> > >> >>> Maybe it's not needed? I may have just been modeling on other > >> > >> >>> interrupts. > >> > >> >> If there is no specific reason, please remove it. > >> > >> > > >> > >> > OK, we'll see how Yuvaraj's testing goes without it. My incredibly > >> > >> > brief testing in our local Chrome OS 3.8 tree shows that the WiFi is > >> > >> > not detected properly if I comment out the line: > >> > >> > dw_mci_cmd_interrupt(host, > >> > >> > pending | SDMMC_INT_CMD_DONE); > >> > >> > > >> > >> > It may be sufficient to simply schedule the tasklet or to do some > >> > >> > other subset of dw_mci_cmd_interrupt(). Yuvaraj can confirm on the > >> > >> > latest kernel and also investigate further. > >> > > How about completing CMD without SDMMC_INT_CMD_DONE? > >> > Without this tuning fails. > >> > [ 2.525284] mmc_host mmc0: Bus speed (slot 0) = 200000000Hz (slot > >> > req 200000000Hz, actual 200000000HZ div = 0) > >> > [ 198.556573] random: nonblocking pool is initialized > >> > [ 240.226044] INFO: task kworker/u8:0:6 blocked for more than 120 seconds. > >> > [ 240.231276] Not tainted 3.16.0-rc1-00010-g2f7a756-dirty #55 > >> > [ 240.237359] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" > >> > disables this message. > >> > [ 240.245150] kworker/u8:0 D c03a58a0 0 6 2 0x00000000 > >> > [ 240.251487] Workqueue: kmmcd mmc_rescan > >> > [ 240.255297] [] (__schedule) from [] > >> > (schedule_timeout+0x130/0x170) > >> > [ 240.263201] [] (schedule_timeout) from [] > >> > (wait_for_common+0xbc/0x14c) > >> > [ 240.271441] [] (wait_for_common) from [] > >> > (mmc_wait_for_req_done+0x6c/0xf0) > >> > [ 240.280031] [] (mmc_wait_for_req_done) from [] > >> > (dw_mci_exynos_execute_tuning+0x1d4/0x2c4) > >> > [ 240.289919] [] (dw_mci_exynos_execute_tuning) from > >> > [] (dw_mci_execute_tuning+0x54/0xb4) > >> > [ 240.299635] [] (dw_mci_execute_tuning) from [] > >> > (mmc_init_card+0xeec/0x14ac) > >> > [ 240.308309] [] (mmc_init_card) from [] > >> > (mmc_attach_mmc+0x8c/0x160) > >> > [ 240.316202] [] (mmc_attach_mmc) from [] > >> > (mmc_rescan+0x2b0/0x308) > >> > [ 240.323924] [] (mmc_rescan) from [] > >> > (process_one_work+0xf8/0x364) > >> > [ 240.331732] [] (process_one_work) from [] > >> > (worker_thread+0x50/0x5a0) > >> > [ 240.339799] [] (worker_thread) from [] > >> > (kthread+0xd8/0xf0) > >> > [ 240.346999] [] (kthread) from [] > >> > (ret_from_fork+0x14/0x3c) > >> > >> Thank you for test. > >> I want to clear it why CMD_DONE is related to tuning. > > In my case, SDMMC_INT_CMD_DONE doesn't affect. > I rechecked this.SDMMC_INT_CMD_DONE flag doesn't affect.Simply > sheduling the tasklet is enough.Sorry,I misunderstood your comments > earlier. > > And do you check VOLT_SWITCH interrupt occurs twice during voltage-switching. > No.Only once. VOLT_SWITCH interrupt occurs totally twice. First is under CMD11. And second is after 1.8V voltage completed. I actually found twice. Please check Voltage Switch Normal Scenario in the manual. Thanks, Seungwon Jeon > > For last one, if-statement in dw_mci_interrupt() disturbs clearing interrupt. > > if ((host->state == STATE_SENDING_CMD11) && > > > > In addition, I suggest adding another flag instead of using host's state. > > Can you consider it? > Sure.I will check this. > > > > Thanks, > > Seungwon Jeon > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 From: tgih.jun@samsung.com (Seungwon Jeon) Date: Tue, 08 Jul 2014 18:56:48 +0900 Subject: [PATCH 3/3] mmc: dw_mmc: Support voltage changes In-Reply-To: References: <1403520321-2431-1-git-send-email-yuvaraj.cd@samsung.com> <1403520321-2431-4-git-send-email-yuvaraj.cd@samsung.com> <001501cf9076$9de45000$d9acf000$%jun@samsung.com> <005001cf912b$27fb8510$77f28f30$%jun@samsung.com> <001101cf91f9$84111530$8c333f90$%jun@samsung.com> <001b01cf976f$e91671c0$bb435540$%jun@samsung.com> <000b01cf99a3$85a2c630$90e85290$%jun@samsung.com> Message-ID: <003701cf9a92$ef2f2220$cd8d6660$%jun@samsung.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Tue, July 08, 2014,Yuvaraj Kumar wrote: > On Mon, Jul 7, 2014 at 10:53 AM, Seungwon Jeon wrote: > > On Fri, July 04, 2014, Seungwon Jeon wrote: > >> On Tue, July 01, 2014. Yuvaraj Kumar wrote: > >> > On Fri, Jun 27, 2014 at 4:48 PM, Seungwon Jeon wrote: > >> > > Hi Yuvaraj, > >> > > > >> > > On Fri, June 27, 2014, Yuvaraj Kumar wrote: > >> > >> On Thu, Jun 26, 2014 at 10:20 PM, Doug Anderson wrote: > >> > >> > Seungwon, > >> > >> > > >> > >> > On Thu, Jun 26, 2014 at 3:41 AM, Seungwon Jeon wrote: > >> > >> >> On Thu, June 26, 2014, Doug Anderson wrote: > >> > >> >>> Seungwon, > >> > >> >>> > >> > >> >>> On Wed, Jun 25, 2014 at 6:08 AM, Seungwon Jeon wrote: > >> > >> >>> > On Mon, June 23, 2014, Yuvaraj Kumar C D wrote: > >> > >> >>> >> Subject: [PATCH 3/3] mmc: dw_mmc: Support voltage changes > >> > >> >>> >> > >> > >> >>> >> From: Doug Anderson > >> > >> >>> >> > >> > >> >>> >> For UHS cards we need the ability to switch voltages from 3.3V to > >> > >> >>> >> 1.8V. Add support to the dw_mmc driver to handle this. Note that > >> > >> >>> >> dw_mmc needs a little bit of extra code since the interface needs a > >> > >> >>> >> special bit programmed to the CMD register while CMD11 is progressing. > >> > >> >>> >> This means adding a few extra states to the state machine to track. > >> > >> >>> > > >> > >> >>> > Overall new additional states makes it complicated. > >> > >> >>> > Can we do that in other way? > >> > >> >>> > >> > >> >>> That was the best I was able to figure out when I thought this > >> > >> >>> through. If you have ideas for doing it another way I'd imagine that > >> > >> >>> Yuvaraj would be happy to take your feedback. > >> > >> >> Let's clean up SDMMC_CMD_VOLT_SWITCH. > >> > >> >> In turn, we may remove state-handling simply. > >> > >> >> > >> > >> >>> > >> > >> >>> > >> > >> >>> >> Signed-off-by: Doug Anderson > >> > >> >>> >> Signed-off-by: Yuvaraj Kumar C D > >> > >> >>> >> > >> > >> >>> >> --- > >> > >> >>> >> drivers/mmc/host/dw_mmc.c | 145 +++++++++++++++++++++++++++++++++++++++++--- > >> > >> >>> >> drivers/mmc/host/dw_mmc.h | 5 +- > >> > >> >>> >> include/linux/mmc/dw_mmc.h | 2 + > >> > >> >>> >> 3 files changed, 142 insertions(+), 10 deletions(-) > >> > >> >>> >> > >> > >> >>> >> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > >> > >> >>> >> index e034bce..38eb548 100644 > >> > >> >>> >> --- a/drivers/mmc/host/dw_mmc.c > >> > >> >>> >> +++ b/drivers/mmc/host/dw_mmc.c > >> > >> >>> >> @@ -29,6 +29,7 @@ > >> > >> >>> >> #include > >> > >> >>> >> #include > >> > >> >>> >> #include > >> > >> >>> >> +#include > >> > >> >>> >> #include > >> > >> >>> >> #include > >> > >> >>> >> #include > >> > >> >>> >> @@ -235,10 +236,13 @@ err: > >> > >> >>> >> } > >> > >> >>> >> #endif /* defined(CONFIG_DEBUG_FS) */ > >> > >> >>> >> > >> > >> >>> >> +static void mci_send_cmd(struct dw_mci_slot *slot, u32 cmd, u32 arg); > >> > >> >>> >> + > >> > >> >>> >> static u32 dw_mci_prepare_command(struct mmc_host *mmc, struct mmc_command *cmd) > >> > >> >>> >> { > >> > >> >>> >> struct mmc_data *data; > >> > >> >>> >> struct dw_mci_slot *slot = mmc_priv(mmc); > >> > >> >>> >> + struct dw_mci *host = slot->host; > >> > >> >>> >> const struct dw_mci_drv_data *drv_data = slot->host->drv_data; > >> > >> >>> >> u32 cmdr; > >> > >> >>> >> cmd->error = -EINPROGRESS; > >> > >> >>> >> @@ -254,6 +258,32 @@ static u32 dw_mci_prepare_command(struct mmc_host *mmc, struct > >> > mmc_command > >> > >> >>> *cmd) > >> > >> >>> >> else if (cmd->opcode != MMC_SEND_STATUS && cmd->data) > >> > >> >>> >> cmdr |= SDMMC_CMD_PRV_DAT_WAIT; > >> > >> >>> >> > >> > >> >>> >> + if (cmd->opcode == SD_SWITCH_VOLTAGE) { > >> > >> >>> >> + u32 clk_en_a; > >> > >> >>> >> + > >> > >> >>> >> + /* Special bit makes CMD11 not die */ > >> > >> >>> >> + cmdr |= SDMMC_CMD_VOLT_SWITCH; > >> > >> >>> >> + > >> > >> >>> >> + /* Change state to continue to handle CMD11 weirdness */ > >> > >> >>> >> + WARN_ON(slot->host->state != STATE_SENDING_CMD); > >> > >> >>> >> + slot->host->state = STATE_SENDING_CMD11; > >> > >> >>> >> + > >> > >> >>> >> + /* > >> > >> >>> >> + * We need to disable clock stop while doing voltage switch > >> > >> >>> >> + * according to 7.4.1.2 Voltage Switch Normal Scenario. > >> > >> >>> >> + * > >> > >> >>> >> + * It's assumed that by the next time the CLKENA is updated > >> > >> >>> >> + * (when we set the clock next) that the voltage change will > >> > >> >>> >> + * be over, so we don't bother setting any bits to synchronize > >> > >> >>> >> + * with dw_mci_setup_bus(). > >> > >> >>> >> + */ > >> > >> >>> >> + clk_en_a = mci_readl(host, CLKENA); > >> > >> >>> >> + clk_en_a &= ~(SDMMC_CLKEN_LOW_PWR << slot->id); > >> > >> >>> >> + mci_writel(host, CLKENA, clk_en_a); > >> > >> >>> >> + mci_send_cmd(slot, SDMMC_CMD_UPD_CLK | > >> > >> >>> >> + SDMMC_CMD_PRV_DAT_WAIT, 0); > >> > >> >>> > dw_mci_disable_low_power() can be used here. > >> > >> >>> > >> > >> >>> Ah. I guess we don't have that locally anymore. Locally we have variants on: > >> > >> >> I'm checking on cjb/mmc branch. > >> > >> >> > >> > >> >>> * https://patchwork.kernel.org/patch/3070311/ > >> > >> >>> * https://patchwork.kernel.org/patch/3070251/ > >> > >> >>> * https://patchwork.kernel.org/patch/3070221/ > >> > >> >>> > >> > >> >>> ...which removed that function. ...but I guess upstream never picked > >> > >> >>> up those patches, huh? Looking back it looks like you had some > >> > >> >>> feedback and it needed another spin but somehow fell off my plate. :( > >> > >> >>> > >> > >> >>> Maybe this is something Yuvaraj would like to pick up? > >> > >> >> It's long ago. I remember that there is no progress since my last comment. > >> > >> >> In case of patch "3070221", I want to pick up for next Kernel. > >> > >> > > >> > >> > Sounds like Yuvaraj has agreed to look at addressing your comments. Thanks! > >> > >> > > >> > >> > > >> > >> >>> >> + } > >> > >> >>> >> + > >> > >> >>> >> if (cmd->flags & MMC_RSP_PRESENT) { > >> > >> >>> >> /* We expect a response, so set this bit */ > >> > >> >>> >> cmdr |= SDMMC_CMD_RESP_EXP; > >> > >> >>> >> @@ -776,11 +806,15 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool > >> > force_clkinit) > >> > >> >>> >> unsigned int clock = slot->clock; > >> > >> >>> >> u32 div; > >> > >> >>> >> u32 clk_en_a; > >> > >> >>> >> + u32 sdmmc_cmd_bits = SDMMC_CMD_UPD_CLK | SDMMC_CMD_PRV_DAT_WAIT; > >> > >> >>> >> + > >> > >> >>> >> + /* We must continue to set bit 28 in CMD until the change is complete */ > >> > >> >>> >> + if (host->state == STATE_WAITING_CMD11_DONE) > >> > >> >>> >> + sdmmc_cmd_bits |= SDMMC_CMD_VOLT_SWITCH; > >> > >> >>> > I didn't get the reason SDMMC_CMD_VOLT_SWITCH is needed during clock > update(enable/disable) > >> > >> >>> > Can you explain it in details? > >> > >> >>> > >> > >> >>> Simply put: if I didn't do this then the system hung during voltage > >> > >> >>> switch. It was not documented in the version of the IP manual that I > >> > >> >>> had access to and it took me a bunch of digging / trial and error to > >> > >> >>> figure this out. Whatever this bit does internally it's important to > >> > >> >>> set it while the voltage change is happening. Note that this need was > >> > >> >>> the whole reason for adding the extra state to the state machine. > >> > >> >>> > >> > >> >>> Perhaps Yuvaraj can try without it and I'd assume he'll report the > >> > >> >>> same freeze. > >> > >> >> Clarify the necessity of SDMMC_CMD_VOLT_SWITCH. > >> > >> >> As far as I experience, we didn't apply this bit for several projects. > >> > >> > > >> > >> > I just tried this locally on our ChromeOS 3.8 kernel (which has quite > >> > >> > a few backports and matches dw_mmc upstream pretty closely). When I > >> > >> > take out "sdmmc_cmd_bits |= SDMMC_CMD_VOLT_SWITCH" and bootup it fails > >> > >> > to bringup WiFi chip. It prints messages like: > >> > >> > > >> > >> > [ 4.400842] mmc_host mmc1: Timeout sending command (cmd 0x202000 > >> > >> > arg 0x0 status 0x80202000) > >> > >> > > >> > >> > I will let Yuvaraj comment about his testing too. > >> > >> To make it simple and verify, i have just enabled eMMC and SD channels > >> > >> on 3.16-rc1 and tested. > >> > >> Without "sdmmc_cmd_bits |= SDMMC_CMD_VOLT_SWITCH" bootup fails. > >> > >> [ 3.015971] mmc_host mmc1: Timeout sending command (cmd 0x202000 > >> > >> arg 0x0 status 0x80202000) > >> > >> [ 3.530974] mmc_host mmc1: Timeout sending command (cmd 0x202000 > >> > >> arg 0x0 status 0x80202000) > >> > > Thank you for test. > >> > > Hmm, it failed during clock update, right?. > >> > > Can you check HLE interrupt at this time? > >> > > I'll investigate for this. > >> > Yes, it failed during clock update.I am dumping the register contents > >> > just before the dev_err statement.I can observe that 1st timeout does > >> > not have HLE interrupt.But once we set bit 0 of UHS_REG subsequent > >> > timeout's have HLE bit set.I am attaching the complete log. > >> > >> Before disabling the clocks, we ensure that the card is not busy.(It's from Synopsys's databook) > >> I guess SDMMC_CMD_VOLT_SWITCH flag seems to affect to avoid this limitation if you have no problem. > >> But, I met the following log with HEL when I apply and test this patch . > >> > >> "mmc_host mmc1: Timeout sending command (cmd 0x202000 arg 0x0 status 0x80202000)" > >> I need to look into it more deeply. > > I confirmed that SDMMC_CMD_VOLT_SWITCH is needed during clock-off/on in voltage switching sequence > > even though there is no description for this in Synopsys's databook. > > And the above-mentioned message("mmc_host mmc1: Timeout sending command (cmd 0x202000 arg 0x0 status > 0x80202000)") > > is happened on mmc_power_cycle(), which should be done because of busy status of DAT[3:0]. > > It indicates clock-off/on sequence fails when executing mmc_power_cycle(). Do you have any idea for this error case? > > > >> > >> > > > >> > >> > > >> > >> > > >> > >> >>> >> +static int dw_mci_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios) > >> > >> >>> >> +{ > >> > >> >>> >> + struct dw_mci_slot *slot = mmc_priv(mmc); > >> > >> >>> >> + struct dw_mci *host = slot->host; > >> > >> >>> >> + u32 uhs; > >> > >> >>> >> + u32 v18 = SDMMC_UHS_18V << slot->id; > >> > >> >>> >> + int min_uv, max_uv; > >> > >> >>> >> + int ret; > >> > >> >>> >> + > >> > >> >>> >> + /* > >> > >> >>> >> + * Program the voltage. Note that some instances of dw_mmc may use > >> > >> >>> >> + * the UHS_REG for this. For other instances (like exynos) the UHS_REG > >> > >> >>> >> + * does no harm but you need to set the regulator directly. Try both. > >> > >> >>> >> + */ > >> > >> >>> >> + uhs = mci_readl(host, UHS_REG); > >> > >> >>> >> + if (ios->signal_voltage == MMC_SIGNAL_VOLTAGE_330) { > >> > >> >>> >> + min_uv = 2700000; > >> > >> >>> >> + max_uv = 3600000; > >> > >> >>> >> + uhs &= ~v18; > >> > >> >>> >> + } else { > >> > >> >>> >> + min_uv = 1700000; > >> > >> >>> >> + max_uv = 1950000; > >> > >> >>> >> + uhs |= v18; > >> > >> >>> >> + } > >> > >> >>> >> + if (!IS_ERR(mmc->supply.vqmmc)) { > >> > >> >>> >> + ret = regulator_set_voltage(mmc->supply.vqmmc, min_uv, max_uv); > >> > >> >>> >> + > >> > >> >>> >> + /* > >> > >> >>> >> + * Only complain if regulator claims that it's not in the 1.8V > >> > >> >>> >> + * range. This avoids a bunch of errors in the case that > >> > >> >>> >> + * we've got a fixed 1.8V regulator but the core SD code still > >> > >> >>> >> + * thinks it ought to try to switch to 3.3 and then back to 1.8 > >> > >> >>> >> + */ > >> > >> >>> >> + if (ret) { > >> > >> >>> > I think if ret is error, printing message and returning error is good. > >> > >> >>> > Currently, just returning '0' though it fails. > >> > >> >> Any feedback? > >> > >> > > >> > >> > Whoops, right. I think you're right that in the case it warns it > >> > >> > should also return the error code. In the case it doesn't warn it > >> > >> > shouldn't. Also: possibly we should use a trick like the mmc core > >> > >> > does and use "regulator_count_voltages" to figure out if we're going > >> > >> > to be able to switch voltages... > >> > >> > > >> > >> >>> > Artificially adding SDMMC_INT_CMD_DONE to pending is needed to complete cmd handling? > >> > >> >>> > Is there any reason? > >> > >> >>> > >> > >> >>> I don't remember this for sure since I wrote this a long time ago. > >> > >> >>> Maybe it's not needed? I may have just been modeling on other > >> > >> >>> interrupts. > >> > >> >> If there is no specific reason, please remove it. > >> > >> > > >> > >> > OK, we'll see how Yuvaraj's testing goes without it. My incredibly > >> > >> > brief testing in our local Chrome OS 3.8 tree shows that the WiFi is > >> > >> > not detected properly if I comment out the line: > >> > >> > dw_mci_cmd_interrupt(host, > >> > >> > pending | SDMMC_INT_CMD_DONE); > >> > >> > > >> > >> > It may be sufficient to simply schedule the tasklet or to do some > >> > >> > other subset of dw_mci_cmd_interrupt(). Yuvaraj can confirm on the > >> > >> > latest kernel and also investigate further. > >> > > How about completing CMD without SDMMC_INT_CMD_DONE? > >> > Without this tuning fails. > >> > [ 2.525284] mmc_host mmc0: Bus speed (slot 0) = 200000000Hz (slot > >> > req 200000000Hz, actual 200000000HZ div = 0) > >> > [ 198.556573] random: nonblocking pool is initialized > >> > [ 240.226044] INFO: task kworker/u8:0:6 blocked for more than 120 seconds. > >> > [ 240.231276] Not tainted 3.16.0-rc1-00010-g2f7a756-dirty #55 > >> > [ 240.237359] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" > >> > disables this message. > >> > [ 240.245150] kworker/u8:0 D c03a58a0 0 6 2 0x00000000 > >> > [ 240.251487] Workqueue: kmmcd mmc_rescan > >> > [ 240.255297] [] (__schedule) from [] > >> > (schedule_timeout+0x130/0x170) > >> > [ 240.263201] [] (schedule_timeout) from [] > >> > (wait_for_common+0xbc/0x14c) > >> > [ 240.271441] [] (wait_for_common) from [] > >> > (mmc_wait_for_req_done+0x6c/0xf0) > >> > [ 240.280031] [] (mmc_wait_for_req_done) from [] > >> > (dw_mci_exynos_execute_tuning+0x1d4/0x2c4) > >> > [ 240.289919] [] (dw_mci_exynos_execute_tuning) from > >> > [] (dw_mci_execute_tuning+0x54/0xb4) > >> > [ 240.299635] [] (dw_mci_execute_tuning) from [] > >> > (mmc_init_card+0xeec/0x14ac) > >> > [ 240.308309] [] (mmc_init_card) from [] > >> > (mmc_attach_mmc+0x8c/0x160) > >> > [ 240.316202] [] (mmc_attach_mmc) from [] > >> > (mmc_rescan+0x2b0/0x308) > >> > [ 240.323924] [] (mmc_rescan) from [] > >> > (process_one_work+0xf8/0x364) > >> > [ 240.331732] [] (process_one_work) from [] > >> > (worker_thread+0x50/0x5a0) > >> > [ 240.339799] [] (worker_thread) from [] > >> > (kthread+0xd8/0xf0) > >> > [ 240.346999] [] (kthread) from [] > >> > (ret_from_fork+0x14/0x3c) > >> > >> Thank you for test. > >> I want to clear it why CMD_DONE is related to tuning. > > In my case, SDMMC_INT_CMD_DONE doesn't affect. > I rechecked this.SDMMC_INT_CMD_DONE flag doesn't affect.Simply > sheduling the tasklet is enough.Sorry,I misunderstood your comments > earlier. > > And do you check VOLT_SWITCH interrupt occurs twice during voltage-switching. > No.Only once. VOLT_SWITCH interrupt occurs totally twice. First is under CMD11. And second is after 1.8V voltage completed. I actually found twice. Please check Voltage Switch Normal Scenario in the manual. Thanks, Seungwon Jeon > > For last one, if-statement in dw_mci_interrupt() disturbs clearing interrupt. > > if ((host->state == STATE_SENDING_CMD11) && > > > > In addition, I suggest adding another flag instead of using host's state. > > Can you consider it? > Sure.I will check this. > > > > Thanks, > > Seungwon Jeon > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html