Seungwon,
On Thu, Jun 26, 2014 at 3:41 AM, Seungwon Jeon <tgih.jun@xxxxxxxxxxx> wrote:
> On Thu, June 26, 2014, Doug Anderson wrote:
>> Seungwon,
>>
>> On Wed, Jun 25, 2014 at 6:08 AM, Seungwon Jeon <tgih.jun@xxxxxxxxxxx> wrote:
>> > On Mon, June 23, 2014, Yuvaraj Kumar C D wrote:
>> >> Subject: [PATCH 3/3] mmc: dw_mmc: Support voltage changes
>> >>
>> >> From: Doug Anderson <dianders@xxxxxxxxxxxx>
>> >>
>> >> 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 <dianders@xxxxxxxxxxxx>
>> >> Signed-off-by: Yuvaraj Kumar C D <yuvaraj.cd@xxxxxxxxxxx>
>> >>
>> >> ---
>> >> 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 <linux/irq.h>
>> >> #include <linux/mmc/host.h>
>> >> #include <linux/mmc/mmc.h>
>> >> +#include <linux/mmc/sd.h>
>> >> #include <linux/mmc/sdio.h>
>> >> #include <linux/mmc/dw_mmc.h>
>> >> #include <linux/bitops.h>
>> >> @@ -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.
>> >> +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.
-Doug
--
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
