* Re: [PATCH 2/2] mmc: sdhci-pci: Add support for HS200 tuning mode on AMD eMMC-4.5.1
From: Adrian Hunter @ 2016-11-16 12:01 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: Ulf Hansson, linux-mmc, Sen, Pankaj, Shah, Nehal-bakulchandra,
Agrawal, Nitesh-kumar
In-Reply-To: <13d2d300-de73-cdc8-3d3a-1fdc2cfcd205@amd.com>
On 15/11/16 08:57, Shyam Sundar S K wrote:
> This patch adds support for HS200 tuning mode on AMD eMMC-4.5.1
>
> Reviewed-by: Sen, Pankaj <Pankaj.Sen@amd.com>
> Reviewed-by: Shah, Nehal-bakulchandra <Nehal-bakulchandra.Shah@amd.com>
> Signed-off-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
> ---
> drivers/mmc/host/sdhci-pci-core.c | 177 +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 176 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c
> index 782c8d2..1a31bdf 100644
> --- a/drivers/mmc/host/sdhci-pci-core.c
> +++ b/drivers/mmc/host/sdhci-pci-core.c
> @@ -817,6 +817,170 @@ enum amd_chipset_gen {
> AMD_CHIPSET_UNKNOWN,
> };
>
> +static const struct sdhci_ops amd_sdhci_pci_ops;
You shouldn't need this. Just put amd_sdhci_pci_ops in the right place.
> +
> +struct amd_tuning_descriptor {
> + u8 tune_around;
> + bool this_tune_ok;
> + bool last_tune_ok;
> + u8 valid_front;
> + u8 valid_window_max;
> + u8 tune_low_max;
> + u8 tune_low;
> + u8 valid_window;
> + u8 tune_result;
> +};
> +
> +static int amd_tuning_reset(struct sdhci_host *host)
> +{
> + unsigned int val;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&host->lock, flags);
> +
> + val = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> + val |= SDHCI_CTRL_PRESET_VAL_ENABLE | SDHCI_CTRL_EXEC_TUNING;
> + sdhci_writew(host, val, SDHCI_HOST_CONTROL2);
> +
> + val = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> + val &= ~SDHCI_CTRL_EXEC_TUNING;
> + sdhci_writew(host, val, SDHCI_HOST_CONTROL2);
> +
> + spin_unlock_irqrestore(&host->lock, flags);
> +
> + return 0;
> +}
> +
> +static int amd_config_tuning_phase(struct sdhci_host *host, u8 phase)
> +{
> + struct sdhci_pci_slot *slot = sdhci_priv(host);
> + struct pci_dev *pdev = slot->chip->pdev;
> + unsigned int val;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&host->lock, flags);
> +
> + pci_read_config_dword(pdev, 0xb8, &val);
> + val &= ~0x1f;
> + val |= (0x10800 | (phase << 1));
> + pci_write_config_dword(pdev, 0xb8, val);
It wouldn't hurt to #define registers and bit fields.
> +
> + spin_unlock_irqrestore(&host->lock, flags);
> +
> + return 0;
> +}
> +
> +static int amd_find_good_phase(struct sdhci_host *host)
> +{
> + struct sdhci_pci_slot *slot = sdhci_priv(host);
> + struct pci_dev *pdev = slot->chip->pdev;
> + struct amd_tuning_descriptor *td = sdhci_pci_priv(slot);
> +
> + unsigned int val;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&host->lock, flags);
> +
> + if (td->this_tune_ok)
> + td->valid_front += 1;
So now valid_front is never reset ?
> + if ((!td->this_tune_ok && td->last_tune_ok) ||
> + (td->tune_around == 11)) {
> + if (td->valid_window > td->valid_window_max) {
> + td->valid_window_max = td->valid_window;
> + td->tune_low_max = td->tune_low;
> + }
> + }
> + if (td->this_tune_ok && (!td->last_tune_ok))
> + td->tune_low = td->tune_around;
> + if (!td->this_tune_ok && td->last_tune_ok)
> + td->valid_window = 0;
> + else if (td->this_tune_ok)
> + td->valid_window += 1;
> +
> + td->last_tune_ok = td->this_tune_ok;
> +
> + if (td->tune_around == 11) {
> + if ((td->valid_front + td->valid_window) >
> + td->valid_window_max) {
> + if (td->valid_front > td->valid_window)
> + td->tune_result = ((td->valid_front -
> + td->valid_window) >> 1);
> + else
> + td->tune_result = td->tune_low +
> + ((td->valid_window + td->valid_front) >> 1);
> + } else {
> + td->tune_result = td->tune_low_max +
> + (td->valid_window_max >> 1);
> + }
So tune_result doesn't need to be in amd_tuning_descriptor.
It is not at all obvious why you need to retain amd_tuning_descriptor
between calls to amd_execute_tuning. Please explain.
> +
> + if (td->tune_result > 0x0b)
> + td->tune_result = 0x0b;
> +
> + pci_read_config_dword(pdev, 0xb8, &val);
> + val &= ~0x1f;
> + val |= (0x10800 | (td->tune_result << 1));
> + pci_write_config_dword(pdev, 0xb8, val);
> + }
> +
> + spin_unlock_irqrestore(&host->lock, flags);
> +
> + return 0;
> +}
> +
> +static int amd_enable_manual_tuning(struct sdhci_pci_slot *slot)
> +{
> + struct pci_dev *pdev = slot->chip->pdev;
> + unsigned int val;
> +
> + pci_read_config_dword(pdev, 0xd0, &val);
> + val &= 0xffffffcf;
No need to set the bits to 0 if you are then going to OR them with 1.
i.e. (val & 0xffffffcf) | 0x30 == val | 0x30
> + val |= 0x30;
> + pci_write_config_dword(pdev, 0xd0, val);
> +
> + return 0;
> +}
> +
> +static int amd_execute_tuning(struct sdhci_host *host, u32 opcode)
> +{
> + struct sdhci_pci_slot *slot = sdhci_priv(host);
> + struct amd_tuning_descriptor *td = sdhci_pci_priv(slot);
> + u8 ctrl;
> +
> + amd_tuning_reset(host);
> +
> + /*********************************************************************/
> + /* Enabling Software Tuning */
> + /********************************************************************/
> + /* 1. First switch the eMMC to HS200 Mode
> + * 2. Prepare the registers by using the sampling clock select
> + * 3. Send the CMD21 12 times with block length of 64 bytes
> + * 4. Everytime change the clk phase and check for CRC error
> + * (CMD and DATA),if error, soft reset the CMD and DATA line
> + * 5. Calculate the window and set the clock phase.
> + */
> +
> + for (td->tune_around = 0; td->tune_around < 12; td->tune_around++) {
tune_around does not need to be in amd_tuning_descriptor
> + amd_config_tuning_phase(host, td->tune_around);
> +
> + if (mmc_send_tuning(host->mmc, opcode, NULL)) {
> + td->this_tune_ok = false;
> + host->mmc->need_retune = 0;
> + mdelay(4);
Use msleep not mdelay
> + ctrl = SDHCI_RESET_CMD | SDHCI_RESET_DATA;
> + sdhci_writeb(host, ctrl, SDHCI_SOFTWARE_RESET);
> + } else {
> + td->this_tune_ok = true;
this_tune_ok does not need to be in amd_tuning_descriptor
> + }
> +
> + amd_find_good_phase(host);
Pass tune_around and this_tune_ok to amd_find_good_phase
> + }
> +
> + host->mmc->retune_period = 0;
> +
> + amd_enable_manual_tuning(slot);
> + return 0;
> +}
> +
> static int amd_probe(struct sdhci_pci_chip *chip)
> {
> struct pci_dev *smbus_dev;
> @@ -841,7 +1005,6 @@ static int amd_probe(struct sdhci_pci_chip *chip)
>
> if ((gen == AMD_CHIPSET_BEFORE_ML) || (gen == AMD_CHIPSET_CZ)) {
> chip->quirks2 |= SDHCI_QUIRK2_CLEAR_TRANSFERMODE_REG_BEFORE_CMD;
> - chip->quirks2 |= SDHCI_QUIRK2_BROKEN_HS200;
> }
>
> return 0;
> @@ -849,6 +1012,7 @@ static int amd_probe(struct sdhci_pci_chip *chip)
>
Put amd_sdhci_pci_ops here.
> static const struct sdhci_pci_fixes sdhci_amd = {
> .probe = amd_probe,
> + .ops = &amd_sdhci_pci_ops,
Need to define the private size i.e.
.priv_size = sizeof(struct amd_tuning_descriptor),
> };
>
> static const struct pci_device_id pci_ids[] = {
> @@ -1469,6 +1633,17 @@ static const struct sdhci_ops sdhci_pci_ops = {
> .select_drive_strength = sdhci_pci_select_drive_strength,
> };
>
> +static const struct sdhci_ops amd_sdhci_pci_ops = {
> + .set_clock = sdhci_set_clock,
> + .enable_dma = sdhci_pci_enable_dma,
> + .set_bus_width = sdhci_pci_set_bus_width,
> + .reset = sdhci_reset,
> + .set_uhs_signaling = sdhci_set_uhs_signaling,
> + .hw_reset = sdhci_pci_hw_reset,
> + .select_drive_strength = sdhci_pci_select_drive_strength,
> + .platform_execute_tuning = amd_execute_tuning,
> +};
Please try to line up the '='
> +
> /*****************************************************************************\
> * *
> * Suspend/resume *
>
^ permalink raw reply
* [PATCH 9/9] mmc: core: Allow CMD13 polling when switch to HS400ES mode
From: Ulf Hansson @ 2016-11-16 10:51 UTC (permalink / raw)
To: linux-mmc, Ulf Hansson
Cc: Jaehoon Chung, Adrian Hunter, Linus Walleij, Chaotian Jing,
Stephen Boyd, Michael Walle, Yong Mao, Shawn Lin
In-Reply-To: <1479293481-20186-1-git-send-email-ulf.hansson@linaro.org>
In cases when the mmc host doesn't support HW busy detection, polling for
busy by using CMD13 is beneficial. The reasons have already been explained
in earlier change logs.
Moreover, when polling with CMD13 during bus timing changes, we should
retry instead of fail when we get CRC errors.
Switching to HS400ES includes several steps, where each step changes the
bus speed timing. Let's improve the behaviour during these sequences, by
allowing CMD13 polling for each of the step. Let's also make sure the CMD13
polling becomes retried, while receiving a CRC error.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/mmc/core/mmc.c | 35 +++++++++++------------------------
1 file changed, 11 insertions(+), 24 deletions(-)
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 24b9e72..b6f0035 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1227,31 +1227,24 @@ static int mmc_select_hs400es(struct mmc_card *card)
goto out_err;
/* Switch card to HS mode */
- err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
- EXT_CSD_HS_TIMING, EXT_CSD_TIMING_HS,
- card->ext_csd.generic_cmd6_time, 0,
- true, false, true);
+ err = mmc_select_hs(card);
if (err) {
pr_err("%s: switch to hs for hs400es failed, err:%d\n",
mmc_hostname(host), err);
goto out_err;
}
- mmc_set_timing(host, MMC_TIMING_MMC_HS);
- err = mmc_switch_status(card);
- if (err)
- goto out_err;
-
mmc_set_clock(host, card->ext_csd.hs_max_dtr);
- /* Switch card to DDR with strobe bit */
+ /* Switch card to HS DDR with strobe bit */
val = EXT_CSD_DDR_BUS_WIDTH_8 | EXT_CSD_BUS_WIDTH_STROBE;
- err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
- EXT_CSD_BUS_WIDTH,
- val,
- card->ext_csd.generic_cmd6_time);
+ err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
+ EXT_CSD_BUS_WIDTH, val,
+ card->ext_csd.generic_cmd6_time,
+ MMC_TIMING_MMC_DDR52,
+ true, true, true);
if (err) {
- pr_err("%s: switch to bus width for hs400es failed, err:%d\n",
+ pr_err("%s: switch to hs ddr for hs400es failed, err:%d\n",
mmc_hostname(host), err);
goto out_err;
}
@@ -1261,26 +1254,20 @@ static int mmc_select_hs400es(struct mmc_card *card)
card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
EXT_CSD_HS_TIMING, val,
- card->ext_csd.generic_cmd6_time, 0,
- true, false, true);
+ card->ext_csd.generic_cmd6_time,
+ MMC_TIMING_MMC_HS400,
+ true, true, true);
if (err) {
pr_err("%s: switch to hs400es failed, err:%d\n",
mmc_hostname(host), err);
goto out_err;
}
- /* Set host controller to HS400 timing and frequency */
- mmc_set_timing(host, MMC_TIMING_MMC_HS400);
-
/* Controller enable enhanced strobe function */
host->ios.enhanced_strobe = true;
if (host->ops->hs400_enhanced_strobe)
host->ops->hs400_enhanced_strobe(host, &host->ios);
- err = mmc_switch_status(card);
- if (err)
- goto out_err;
-
return 0;
out_err:
--
1.9.1
^ permalink raw reply related
* [PATCH 8/9] mmc: core: Allow CMD13 polling when switch to HS400 mode
From: Ulf Hansson @ 2016-11-16 10:51 UTC (permalink / raw)
To: linux-mmc, Ulf Hansson
Cc: Jaehoon Chung, Adrian Hunter, Linus Walleij, Chaotian Jing,
Stephen Boyd, Michael Walle, Yong Mao, Shawn Lin
In-Reply-To: <1479293481-20186-1-git-send-email-ulf.hansson@linaro.org>
In cases when the mmc host doesn't support HW busy detection, polling for
busy by using CMD13 is beneficial. The reasons have already been explained
in earlier change logs.
Moreover, when polling with CMD13 during bus timing changes, we should
retry instead of fail when we get CRC errors.
Switching from HS200 to HS400 and reverse includes several steps, where
each step changes the bus speed timing. Let's improve the behaviour during
these sequences, by allowing CMD13 polling for each of the step. Let's also
make sure the CMD13 polling becomes retried, while receiving a CRC error.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/mmc/core/mmc.c | 87 +++++++++++++++-----------------------------------
1 file changed, 26 insertions(+), 61 deletions(-)
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 0b26383..24b9e72 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1094,8 +1094,7 @@ static int mmc_select_hs_ddr(struct mmc_card *card)
static int mmc_select_hs400(struct mmc_card *card)
{
struct mmc_host *host = card->host;
- unsigned int max_dtr;
- int err = 0;
+ int err;
u8 val;
/*
@@ -1105,34 +1104,26 @@ static int mmc_select_hs400(struct mmc_card *card)
host->ios.bus_width == MMC_BUS_WIDTH_8))
return 0;
- /* Switch card to HS mode */
- val = EXT_CSD_TIMING_HS;
- err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
- EXT_CSD_HS_TIMING, val,
- card->ext_csd.generic_cmd6_time, 0,
- true, false, true);
+ /*
+ * Switch card to HS mode.
+ * First reduce to the HS frequency as CMD13 are sent to poll for busy
+ * and/or to validate the switch status.
+ */
+ mmc_set_clock(host, card->ext_csd.hs_max_dtr);
+ err = mmc_select_hs(card);
if (err) {
pr_err("%s: switch to high-speed from hs200 failed, err:%d\n",
mmc_hostname(host), err);
return err;
}
- /* Set host controller to HS timing */
- mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
-
- /* Reduce frequency to HS frequency */
- max_dtr = card->ext_csd.hs_max_dtr;
- mmc_set_clock(host, max_dtr);
-
- err = mmc_switch_status(card);
- if (err)
- goto out_err;
-
- /* Switch card to DDR */
- err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
- EXT_CSD_BUS_WIDTH,
- EXT_CSD_DDR_BUS_WIDTH_8,
- card->ext_csd.generic_cmd6_time);
+ /* Switch card to HS DDR */
+ err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
+ EXT_CSD_BUS_WIDTH,
+ EXT_CSD_DDR_BUS_WIDTH_8,
+ card->ext_csd.generic_cmd6_time,
+ MMC_TIMING_MMC_DDR52,
+ true, true, true);
if (err) {
pr_err("%s: switch to bus width for hs400 failed, err:%d\n",
mmc_hostname(host), err);
@@ -1144,28 +1135,17 @@ static int mmc_select_hs400(struct mmc_card *card)
card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
EXT_CSD_HS_TIMING, val,
- card->ext_csd.generic_cmd6_time, 0,
- true, false, true);
+ card->ext_csd.generic_cmd6_time,
+ MMC_TIMING_MMC_HS400,
+ true, true, true);
if (err) {
pr_err("%s: switch to hs400 failed, err:%d\n",
mmc_hostname(host), err);
return err;
}
- /* Set host controller to HS400 timing and frequency */
- mmc_set_timing(host, MMC_TIMING_MMC_HS400);
mmc_set_bus_speed(card);
-
- err = mmc_switch_status(card);
- if (err)
- goto out_err;
-
return 0;
-
-out_err:
- pr_err("%s: %s failed, error %d\n", mmc_hostname(card->host),
- __func__, err);
- return err;
}
int mmc_hs200_to_hs400(struct mmc_card *card)
@@ -1187,27 +1167,17 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
/* Switch HS400 to HS DDR */
val = EXT_CSD_TIMING_HS;
err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING,
- val, card->ext_csd.generic_cmd6_time, 0,
- true, false, true);
- if (err)
- goto out_err;
-
- mmc_set_timing(host, MMC_TIMING_MMC_DDR52);
-
- err = mmc_switch_status(card);
+ val, card->ext_csd.generic_cmd6_time,
+ MMC_TIMING_MMC_DDR52,
+ true, true, true);
if (err)
goto out_err;
/* Switch HS DDR to HS */
err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BUS_WIDTH,
EXT_CSD_BUS_WIDTH_8, card->ext_csd.generic_cmd6_time,
- 0, true, false, true);
- if (err)
- goto out_err;
-
- mmc_set_timing(host, MMC_TIMING_MMC_HS);
-
- err = mmc_switch_status(card);
+ MMC_TIMING_MMC_HS,
+ true, true, true);
if (err)
goto out_err;
@@ -1215,14 +1185,9 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
val = EXT_CSD_TIMING_HS200 |
card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING,
- val, card->ext_csd.generic_cmd6_time, 0,
- true, false, true);
- if (err)
- goto out_err;
-
- mmc_set_timing(host, MMC_TIMING_MMC_HS200);
-
- err = mmc_switch_status(card);
+ val, card->ext_csd.generic_cmd6_time,
+ MMC_TIMING_MMC_HS200,
+ true, true, true);
if (err)
goto out_err;
--
1.9.1
^ permalink raw reply related
* [PATCH 7/9] mmc: core: Allow CMD13 polling when switch to HS200 mode
From: Ulf Hansson @ 2016-11-16 10:51 UTC (permalink / raw)
To: linux-mmc, Ulf Hansson
Cc: Jaehoon Chung, Adrian Hunter, Linus Walleij, Chaotian Jing,
Stephen Boyd, Michael Walle, Yong Mao, Shawn Lin
In-Reply-To: <1479293481-20186-1-git-send-email-ulf.hansson@linaro.org>
In cases when the mmc host doesn't support HW busy detection, polling for
busy by using CMD13 is beneficial. The reasons have already been explained
in earlier change logs.
To allow polling with CMD13, let's provide MMC_TIMING_MMC_HS200 as the
timing parameter to __mmc_switch(), which makes sure the mmc host and the
mmc card operates at the same bus timing during the polling.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/mmc/core/mmc.c | 21 +++++----------------
1 file changed, 5 insertions(+), 16 deletions(-)
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 3268fcd..0b26383 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1351,7 +1351,7 @@ static void mmc_select_driver_type(struct mmc_card *card)
static int mmc_select_hs200(struct mmc_card *card)
{
struct mmc_host *host = card->host;
- unsigned int old_timing, old_signal_voltage;
+ unsigned int old_signal_voltage;
int err = -EINVAL;
u8 val;
@@ -1378,22 +1378,11 @@ static int mmc_select_hs200(struct mmc_card *card)
card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
EXT_CSD_HS_TIMING, val,
- card->ext_csd.generic_cmd6_time, 0,
- true, false, true);
- if (err)
- goto err;
- old_timing = host->ios.timing;
- mmc_set_timing(host, MMC_TIMING_MMC_HS200);
-
- err = mmc_switch_status(card);
- /*
- * mmc_select_timing() assumes timing has not changed if
- * it is a switch error.
- */
- if (err == -EBADMSG)
- mmc_set_timing(host, old_timing);
+ card->ext_csd.generic_cmd6_time,
+ MMC_TIMING_MMC_HS200,
+ true, true, true);
}
-err:
+
if (err) {
/* fall back to the old signal voltage, if fails report error */
if (__mmc_set_signal_voltage(host, old_signal_voltage))
--
1.9.1
^ permalink raw reply related
* [PATCH 6/9] mmc: core: Update CMD13 polling policy when switch to HS DDR mode
From: Ulf Hansson @ 2016-11-16 10:51 UTC (permalink / raw)
To: linux-mmc, Ulf Hansson
Cc: Jaehoon Chung, Adrian Hunter, Linus Walleij, Chaotian Jing,
Stephen Boyd, Michael Walle, Yong Mao, Shawn Lin
In-Reply-To: <1479293481-20186-1-git-send-email-ulf.hansson@linaro.org>
According to the JEDEC specification, during bus timing change operations
for mmc, sending a CMD13 could trigger CRC errors.
As switching to HS DDR mode indeed causes a bus timing change, polling with
CMD13 to detect card busy, may thus potentially trigger CRC errors.
Currently these errors are treated as the switch to HS DDR mode failed.
To improve this behaviour, let's instead tell __mmc_switch() to retry when
it encounters CRC errors during polling.
Moreover, when switching to HS DDR mode, let's make sure the CMD13 polling
is done by having the mmc host and the mmc card, being configured to
operate at the same selected bus speed timing. Fix this by providing
MMC_TIMING_MMC_DDR52 as the timing parameter to __mmc_switch().
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/mmc/core/mmc.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 15dd51c..3268fcd 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1040,10 +1040,12 @@ static int mmc_select_hs_ddr(struct mmc_card *card)
ext_csd_bits = (bus_width == MMC_BUS_WIDTH_8) ?
EXT_CSD_DDR_BUS_WIDTH_8 : EXT_CSD_DDR_BUS_WIDTH_4;
- err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
- EXT_CSD_BUS_WIDTH,
- ext_csd_bits,
- card->ext_csd.generic_cmd6_time);
+ err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
+ EXT_CSD_BUS_WIDTH,
+ ext_csd_bits,
+ card->ext_csd.generic_cmd6_time,
+ MMC_TIMING_MMC_DDR52,
+ true, true, true);
if (err) {
pr_err("%s: switch to bus width %d ddr failed\n",
mmc_hostname(host), 1 << bus_width);
@@ -1086,9 +1088,6 @@ static int mmc_select_hs_ddr(struct mmc_card *card)
if (err)
err = __mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_330);
- if (!err)
- mmc_set_timing(host, MMC_TIMING_MMC_DDR52);
-
return err;
}
--
1.9.1
^ permalink raw reply related
* [PATCH 5/9] mmc: core: Allow CMD13 polling when switching to HS mode for mmc
From: Ulf Hansson @ 2016-11-16 10:51 UTC (permalink / raw)
To: linux-mmc, Ulf Hansson
Cc: Jaehoon Chung, Adrian Hunter, Linus Walleij, Chaotian Jing,
Stephen Boyd, Michael Walle, Yong Mao, Shawn Lin
In-Reply-To: <1479293481-20186-1-git-send-email-ulf.hansson@linaro.org>
In cases when the mmc host doesn't support HW busy detection, polling for a
card being busy by using CMD13 is beneficial. That is because, instead of
waiting a fixed amount of time, 500ms or the generic CMD6 time from
EXT_CSD, we find out a lot sooner when the card stops signaling busy. This
leads to a significant decreased total initialization time for the mmc
card.
However, to allow polling with CMD13 during a bus timing change operation,
such as switching to HS mode, we first need to update the mmc host's bus
timing before starting to poll. Deal with that, simply by providing
MMC_TIMING_MMC_HS as the timing parameter to __mmc_switch() from
mmc_select_hs().
By telling __mmc_switch() to allow polling with CMD13, also makes it
validate the CMD6 status, thus we can remove the corresponding checks.
When switching to HS400ES, the mmc_select_hs() function is called in one of
the intermediate steps. To still prevent CMD13 polling for HS400ES, let's
call the __mmc_switch() function in this path as it enables us to keep
using the existing method.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/mmc/core/mmc.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index cb1cf4e..15dd51c 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1012,13 +1012,8 @@ static int mmc_select_hs(struct mmc_card *card)
err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
EXT_CSD_HS_TIMING, EXT_CSD_TIMING_HS,
- card->ext_csd.generic_cmd6_time, 0,
- true, false, true);
- if (!err) {
- mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
- err = mmc_switch_status(card);
- }
-
+ card->ext_csd.generic_cmd6_time, MMC_TIMING_MMC_HS,
+ true, true, true);
if (err)
pr_warn("%s: switch to high-speed failed, err:%d\n",
mmc_hostname(card->host), err);
@@ -1268,16 +1263,23 @@ static int mmc_select_hs400es(struct mmc_card *card)
goto out_err;
/* Switch card to HS mode */
- err = mmc_select_hs(card);
- if (err)
+ err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
+ EXT_CSD_HS_TIMING, EXT_CSD_TIMING_HS,
+ card->ext_csd.generic_cmd6_time, 0,
+ true, false, true);
+ if (err) {
+ pr_err("%s: switch to hs for hs400es failed, err:%d\n",
+ mmc_hostname(host), err);
goto out_err;
+ }
- mmc_set_clock(host, card->ext_csd.hs_max_dtr);
-
+ mmc_set_timing(host, MMC_TIMING_MMC_HS);
err = mmc_switch_status(card);
if (err)
goto out_err;
+ mmc_set_clock(host, card->ext_csd.hs_max_dtr);
+
/* Switch card to DDR with strobe bit */
val = EXT_CSD_DDR_BUS_WIDTH_8 | EXT_CSD_BUS_WIDTH_STROBE;
err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
--
1.9.1
^ permalink raw reply related
* [PATCH 4/9] mmc: core: Enable __mmc_switch() to change bus speed timing for the host
From: Ulf Hansson @ 2016-11-16 10:51 UTC (permalink / raw)
To: linux-mmc, Ulf Hansson
Cc: Jaehoon Chung, Adrian Hunter, Linus Walleij, Chaotian Jing,
Stephen Boyd, Michael Walle, Yong Mao, Shawn Lin
In-Reply-To: <1479293481-20186-1-git-send-email-ulf.hansson@linaro.org>
In cases when a speed mode change is requested for mmc cards, a CMD6 is
sent by calling __mmc_switch() during the card initialization. The CMD6
leads to the card entering a busy period. When that is completed, the host
must parse the CMD6 status to find out whether the change of the speed mode
succeeded.
To enable the mmc core to poll the card by using CMD13 to find out when the
busy period is completed, it's reasonable to make sure polling is done by
having the mmc host and the mmc card, being configured to operate at the
same selected bus speed timing.
Therefore, let's extend __mmc_switch() to take yet another parameter, which
allow its callers to update the bus speed timing of the mmc host. In this
way, __mmc_switch() also becomes capable of reading and validating the CMD6
status by sending a CMD13, in cases when that's desired.
If __mmc_switch() encounters a failure, we make sure to restores the old
bus speed timing for the mmc host, before propagating the error code.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/mmc/core/core.c | 2 +-
drivers/mmc/core/mmc.c | 18 +++++++++---------
drivers/mmc/core/mmc_ops.c | 20 +++++++++++++++-----
drivers/mmc/core/mmc_ops.h | 4 ++--
4 files changed, 27 insertions(+), 17 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 50bb9a1..060f767 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -380,7 +380,7 @@ void mmc_start_bkops(struct mmc_card *card, bool from_exception)
mmc_retune_hold(card->host);
err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
- EXT_CSD_BKOPS_START, 1, timeout,
+ EXT_CSD_BKOPS_START, 1, timeout, 0,
use_busy_signal, true, false);
if (err) {
pr_warn("%s: Error %d starting bkops\n",
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 9355366..cb1cf4e 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1012,7 +1012,7 @@ static int mmc_select_hs(struct mmc_card *card)
err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
EXT_CSD_HS_TIMING, EXT_CSD_TIMING_HS,
- card->ext_csd.generic_cmd6_time,
+ card->ext_csd.generic_cmd6_time, 0,
true, false, true);
if (!err) {
mmc_set_timing(card->host, MMC_TIMING_MMC_HS);
@@ -1115,7 +1115,7 @@ static int mmc_select_hs400(struct mmc_card *card)
val = EXT_CSD_TIMING_HS;
err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
EXT_CSD_HS_TIMING, val,
- card->ext_csd.generic_cmd6_time,
+ card->ext_csd.generic_cmd6_time, 0,
true, false, true);
if (err) {
pr_err("%s: switch to high-speed from hs200 failed, err:%d\n",
@@ -1150,7 +1150,7 @@ static int mmc_select_hs400(struct mmc_card *card)
card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
EXT_CSD_HS_TIMING, val,
- card->ext_csd.generic_cmd6_time,
+ card->ext_csd.generic_cmd6_time, 0,
true, false, true);
if (err) {
pr_err("%s: switch to hs400 failed, err:%d\n",
@@ -1193,7 +1193,7 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
/* Switch HS400 to HS DDR */
val = EXT_CSD_TIMING_HS;
err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING,
- val, card->ext_csd.generic_cmd6_time,
+ val, card->ext_csd.generic_cmd6_time, 0,
true, false, true);
if (err)
goto out_err;
@@ -1207,7 +1207,7 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
/* Switch HS DDR to HS */
err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BUS_WIDTH,
EXT_CSD_BUS_WIDTH_8, card->ext_csd.generic_cmd6_time,
- true, false, true);
+ 0, true, false, true);
if (err)
goto out_err;
@@ -1221,7 +1221,7 @@ int mmc_hs400_to_hs200(struct mmc_card *card)
val = EXT_CSD_TIMING_HS200 |
card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_HS_TIMING,
- val, card->ext_csd.generic_cmd6_time,
+ val, card->ext_csd.generic_cmd6_time, 0,
true, false, true);
if (err)
goto out_err;
@@ -1295,7 +1295,7 @@ static int mmc_select_hs400es(struct mmc_card *card)
card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
EXT_CSD_HS_TIMING, val,
- card->ext_csd.generic_cmd6_time,
+ card->ext_csd.generic_cmd6_time, 0,
true, false, true);
if (err) {
pr_err("%s: switch to hs400es failed, err:%d\n",
@@ -1377,7 +1377,7 @@ static int mmc_select_hs200(struct mmc_card *card)
card->drive_strength << EXT_CSD_DRV_STR_SHIFT;
err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
EXT_CSD_HS_TIMING, val,
- card->ext_csd.generic_cmd6_time,
+ card->ext_csd.generic_cmd6_time, 0,
true, false, true);
if (err)
goto err;
@@ -1841,7 +1841,7 @@ static int mmc_poweroff_notify(struct mmc_card *card, unsigned int notify_type)
err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
EXT_CSD_POWER_OFF_NOTIFICATION,
- notify_type, timeout, true, false, false);
+ notify_type, timeout, 0, true, false, false);
if (err)
pr_err("%s: Power Off Notification timed out, %u\n",
mmc_hostname(card->host), timeout);
diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 214e734..ab77fc3 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -526,6 +526,7 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
* @value: value to program into EXT_CSD register
* @timeout_ms: timeout (ms) for operation performed by register write,
* timeout of zero implies maximum possible timeout
+ * @timing: new timing to change to
* @use_busy_signal: use the busy signal as response type
* @send_status: send status cmd to poll for busy
* @retry_crc_err: retry when CRC errors when polling with CMD13 for busy
@@ -533,13 +534,14 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
* Modifies the EXT_CSD register for selected card.
*/
int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
- unsigned int timeout_ms, bool use_busy_signal, bool send_status,
- bool retry_crc_err)
+ unsigned int timeout_ms, unsigned char timing,
+ bool use_busy_signal, bool send_status, bool retry_crc_err)
{
struct mmc_host *host = card->host;
int err;
struct mmc_command cmd = {0};
bool use_r1b_resp = use_busy_signal;
+ unsigned char old_timing = host->ios.timing;
mmc_retune_hold(host);
@@ -581,16 +583,24 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
if (!use_busy_signal)
goto out;
+ /* Switch to new timing before poll and check switch status. */
+ if (timing)
+ mmc_set_timing(host, timing);
+
/*If SPI or used HW busy detection above, then we don't need to poll. */
if (((host->caps & MMC_CAP_WAIT_WHILE_BUSY) && use_r1b_resp) ||
mmc_host_is_spi(host)) {
if (send_status)
err = mmc_switch_status(card);
- goto out;
+ goto out_tim;
}
/* Let's try to poll to find out when the command is completed. */
err = mmc_poll_for_busy(card, timeout_ms, send_status, retry_crc_err);
+
+out_tim:
+ if (err && timing)
+ mmc_set_timing(host, old_timing);
out:
mmc_retune_release(host);
@@ -600,8 +610,8 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
int mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
unsigned int timeout_ms)
{
- return __mmc_switch(card, set, index, value, timeout_ms, true, true,
- false);
+ return __mmc_switch(card, set, index, value, timeout_ms, 0,
+ true, true, false);
}
EXPORT_SYMBOL_GPL(mmc_switch);
diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
index 9fccddb..761cb69 100644
--- a/drivers/mmc/core/mmc_ops.h
+++ b/drivers/mmc/core/mmc_ops.h
@@ -29,8 +29,8 @@
int mmc_can_ext_csd(struct mmc_card *card);
int mmc_switch_status(struct mmc_card *card);
int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
- unsigned int timeout_ms, bool use_busy_signal, bool send_status,
- bool retry_crc_err);
+ unsigned int timeout_ms, unsigned char timing,
+ bool use_busy_signal, bool send_status, bool retry_crc_err);
#endif
--
1.9.1
^ permalink raw reply related
* [PATCH 3/9] mmc: core: Rename ignore_crc to retry_crc_err to reflect its purpose
From: Ulf Hansson @ 2016-11-16 10:51 UTC (permalink / raw)
To: linux-mmc, Ulf Hansson
Cc: Jaehoon Chung, Adrian Hunter, Linus Walleij, Chaotian Jing,
Stephen Boyd, Michael Walle, Yong Mao, Shawn Lin
In-Reply-To: <1479293481-20186-1-git-send-email-ulf.hansson@linaro.org>
The ignore_crc parameter/variable name is used at a couple of places in the
mmc core. Let's rename it to retry_crc_err to reflect its new purpose.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/mmc/core/mmc_ops.c | 10 +++++-----
drivers/mmc/core/mmc_ops.h | 2 +-
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 0515748..214e734 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -461,7 +461,7 @@ int mmc_switch_status(struct mmc_card *card)
}
static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
- bool send_status, bool ignore_crc)
+ bool send_status, bool retry_crc_err)
{
struct mmc_host *host = card->host;
int err;
@@ -496,7 +496,7 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
busy = host->ops->card_busy(host);
} else {
err = mmc_send_status(card, &status);
- if (ignore_crc && err == -EILSEQ)
+ if (retry_crc_err && err == -EILSEQ)
busy = true;
else if (err)
return err;
@@ -528,13 +528,13 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
* timeout of zero implies maximum possible timeout
* @use_busy_signal: use the busy signal as response type
* @send_status: send status cmd to poll for busy
- * @ignore_crc: ignore CRC errors when sending status cmd to poll for busy
+ * @retry_crc_err: retry when CRC errors when polling with CMD13 for busy
*
* Modifies the EXT_CSD register for selected card.
*/
int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
unsigned int timeout_ms, bool use_busy_signal, bool send_status,
- bool ignore_crc)
+ bool retry_crc_err)
{
struct mmc_host *host = card->host;
int err;
@@ -590,7 +590,7 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
}
/* Let's try to poll to find out when the command is completed. */
- err = mmc_poll_for_busy(card, timeout_ms, send_status, ignore_crc);
+ err = mmc_poll_for_busy(card, timeout_ms, send_status, retry_crc_err);
out:
mmc_retune_release(host);
diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
index 7f6c0e9..9fccddb 100644
--- a/drivers/mmc/core/mmc_ops.h
+++ b/drivers/mmc/core/mmc_ops.h
@@ -30,7 +30,7 @@
int mmc_switch_status(struct mmc_card *card);
int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
unsigned int timeout_ms, bool use_busy_signal, bool send_status,
- bool ignore_crc);
+ bool retry_crc_err);
#endif
--
1.9.1
^ permalink raw reply related
* [PATCH 2/9] mmc: core: Remove redundant __mmc_send_status()
From: Ulf Hansson @ 2016-11-16 10:51 UTC (permalink / raw)
To: linux-mmc, Ulf Hansson
Cc: Jaehoon Chung, Adrian Hunter, Linus Walleij, Chaotian Jing,
Stephen Boyd, Michael Walle, Yong Mao, Shawn Lin
In-Reply-To: <1479293481-20186-1-git-send-email-ulf.hansson@linaro.org>
There are only one users left which calls __mmc_send_status(). Moreover,
the ignore_crc parameter isn't being used, so let's just remove these
redundant parts.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/mmc/core/mmc_ops.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 4773c56..0515748 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -54,8 +54,7 @@
0xff, 0x77, 0x77, 0xff, 0x77, 0xbb, 0xdd, 0xee,
};
-static inline int __mmc_send_status(struct mmc_card *card, u32 *status,
- bool ignore_crc)
+int mmc_send_status(struct mmc_card *card, u32 *status)
{
int err;
struct mmc_command cmd = {0};
@@ -67,8 +66,6 @@ static inline int __mmc_send_status(struct mmc_card *card, u32 *status,
if (!mmc_host_is_spi(card->host))
cmd.arg = card->rca << 16;
cmd.flags = MMC_RSP_SPI_R2 | MMC_RSP_R1 | MMC_CMD_AC;
- if (ignore_crc)
- cmd.flags &= ~MMC_RSP_CRC;
err = mmc_wait_for_cmd(card->host, &cmd, MMC_CMD_RETRIES);
if (err)
@@ -83,11 +80,6 @@ static inline int __mmc_send_status(struct mmc_card *card, u32 *status,
return 0;
}
-int mmc_send_status(struct mmc_card *card, u32 *status)
-{
- return __mmc_send_status(card, status, false);
-}
-
static int _mmc_select_card(struct mmc_host *host, struct mmc_card *card)
{
struct mmc_command cmd = {0};
--
1.9.1
^ permalink raw reply related
* [PATCH 1/9] mmc: core: Retry instead of ignore at CRC errors when polling for busy
From: Ulf Hansson @ 2016-11-16 10:51 UTC (permalink / raw)
To: linux-mmc, Ulf Hansson
Cc: Jaehoon Chung, Adrian Hunter, Linus Walleij, Chaotian Jing,
Stephen Boyd, Michael Walle, Yong Mao, Shawn Lin
In-Reply-To: <1479293481-20186-1-git-send-email-ulf.hansson@linaro.org>
After a CMD6 command has been sent, the __mmc_switch() function might be
advised to poll the card for busy by using CMD13 and also by ignoring CRC
errors.
In the case of ignoring CRC errors, the mmc core tells the mmc host to also
ignore these errors via masking the MMC_RSP_CRC response flag. This seems
wrong, as it leads to that the mmc host could propagate an unreliable
response, instead of a proper error code.
What we really want, is not to ignore CRC errors but instead retry the
polling attempt. So, let's change this by treating a CRC error as the card
is still being busy and thus continue to run the polling loop.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/mmc/core/mmc_ops.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 481bbdb..4773c56 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -503,10 +503,13 @@ static int mmc_poll_for_busy(struct mmc_card *card, unsigned int timeout_ms,
if (host->ops->card_busy) {
busy = host->ops->card_busy(host);
} else {
- err = __mmc_send_status(card, &status, ignore_crc);
- if (err)
+ err = mmc_send_status(card, &status);
+ if (ignore_crc && err == -EILSEQ)
+ busy = true;
+ else if (err)
return err;
- busy = R1_CURRENT_STATE(status) == R1_STATE_PRG;
+ else
+ busy = R1_CURRENT_STATE(status) == R1_STATE_PRG;
}
/* Timeout if the device still remains busy. */
--
1.9.1
^ permalink raw reply related
* [PATCH 0/9] mmc: core: Re-work CMD13 polling method for CMD6 for mmc
From: Ulf Hansson @ 2016-11-16 10:51 UTC (permalink / raw)
To: linux-mmc, Ulf Hansson
Cc: Jaehoon Chung, Adrian Hunter, Linus Walleij, Chaotian Jing,
Stephen Boyd, Michael Walle, Yong Mao, Shawn Lin
Several changes has been made for how and when to use CMD13 as a
polling-method, to find out when the mmc cards stops signaling busy after
sending a CMD6. This particularly concerns the cases when switching to new bus
speed modes, such as HS (high-speed), HS DDR, HS200, HS400 and HS400ES.
Currently CMD13 polling is *not* allowed for these cases, but this was recently
changed - and which did cause regressions for card detection for some
platforms.
A simple fix was made to solve these regressions, simply by using a default
CMD6 generic timeout of 500ms, as to avoid using the fall-back timeout in
__mmc_switch() of 10min. That greatly improve the situation and one could
consider the regressions as "solved".
However, this simple fix is still causing unnecessary long card initialization
times, especially for those mmc hosts that doesn't support HW busy detection
(using MMC_CAP_WAIT_WHILE_BUSY and/or implements the ->card_busy() host ops).
This because we wait a fixed amount of time (CMD6 generic timeout) to make sure
the card is done signaling busy, while in practice this happens a lot sooner.
A couple of tests for HS and HS DDR eMMC cards, shows the card only need a few
ms to complete the bus speed mode changes, thus it's far less than the CMD6
generic timeout.
To improve this behaviour and shorten the card initialization time, we need to
allow using CMD13 as polling method to find out when the card stops signaling
busy. Although, enabling CMD13 polling may also introduce other issues as
according to the JEDEC spec, it's not the recommended method. Especially it
mentions that CRC errors may be triggered when sending a CMD13 command during a
bus timing change.
To deal with these issues, we need to change from ignoring those CRC errors but
instead continue to treat the card as being busy and continue to poll with
CMD13. Perhaps this behaviour is one of reasons to why the earlier CMD13 polling
method didn't work out well.
This series requires extensive testing, please help with that!
I have currently tested it with HS and HS DDR eMMC cards, and for combinations
of an mmc hosts (mmci) supporting HW busy detection and not (local hacks in
mmci.c).
Ulf Hansson (9):
mmc: core: Retry instead of ignore at CRC errors when polling for busy
mmc: core: Remove redundant __mmc_send_status()
mmc: core: Rename ignore_crc to retry_crc_err to reflect its purpose
mmc: core: Enable __mmc_switch() to change bus speed timing for the
host
mmc: core: Allow CMD13 polling when switching to HS mode for mmc
mmc: core: Update CMD13 polling policy when switch to HS DDR mode
mmc: core: Allow CMD13 polling when switch to HS200 mode
mmc: core: Allow CMD13 polling when switch to HS400 mode
mmc: core: Allow CMD13 polling when switch to HS400ES mode
drivers/mmc/core/core.c | 2 +-
drivers/mmc/core/mmc.c | 156 ++++++++++++++-------------------------------
drivers/mmc/core/mmc_ops.c | 45 +++++++------
drivers/mmc/core/mmc_ops.h | 4 +-
4 files changed, 77 insertions(+), 130 deletions(-)
--
1.9.1
^ permalink raw reply
* [PATCH] RFC: mmc: block: replace semaphore with freezing
From: Linus Walleij @ 2016-11-16 10:51 UTC (permalink / raw)
To: linux-mmc, Ulf Hansson
Cc: Chunyan Zhang, Baolin Wang, Linus Walleij, linux-pm,
Rafael J . Wysocki, Russell King, Arnd Bergmann
The MMC/SD block core request processing thread is taking a
semaphore in the request processing section and the same
semaphore is taken around suspend/resume operations.
The purpose of the semaphore is to block any request from being
issued to the MMC/SD host controllers when the system is
suspended. A semaphore is used in place of a mutex since the
calls are coming from different threads.
This construction predates the kernel thread freezer mechanism:
we can easily replace the semaphore by instead activating the
freezer with set_freezable() and call try_to_freeze() instead
of the up(); schedule(); down(); construction that is devised
to let the suspend/resume calls get in and grab the semaphore.
Tested with a few suspend/resume to memory cycles on the Ux500
when doing intense dd operations in the background: the
thread thaws and resumes operation after resume.
Cc: linux-pm@vger.kernel.org
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
I haven't seen any need to preserve the call to schedule()
in the request processing thread, but I want advice on whether
that has a point. I would guess the thread will just anyway
be preempted if needed anyway as it is marked interruptible?
---
drivers/mmc/card/queue.c | 13 ++-----------
drivers/mmc/card/queue.h | 1 -
2 files changed, 2 insertions(+), 12 deletions(-)
diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
index 8037f73a109a..09a932ffe46e 100644
--- a/drivers/mmc/card/queue.c
+++ b/drivers/mmc/card/queue.c
@@ -55,8 +55,8 @@ static int mmc_queue_thread(void *d)
struct request_queue *q = mq->queue;
current->flags |= PF_MEMALLOC;
+ set_freezable();
- down(&mq->thread_sem);
do {
struct request *req = NULL;
@@ -95,12 +95,9 @@ static int mmc_queue_thread(void *d)
set_current_state(TASK_RUNNING);
break;
}
- up(&mq->thread_sem);
- schedule();
- down(&mq->thread_sem);
+ try_to_freeze();
}
} while (1);
- up(&mq->thread_sem);
return 0;
}
@@ -289,8 +286,6 @@ int mmc_init_queue(struct mmc_queue *mq, struct mmc_card *card,
goto cleanup_queue;
}
- sema_init(&mq->thread_sem, 1);
-
mq->thread = kthread_run(mmc_queue_thread, mq, "mmcqd/%d%s",
host->index, subname ? subname : "");
@@ -424,8 +419,6 @@ void mmc_queue_suspend(struct mmc_queue *mq)
spin_lock_irqsave(q->queue_lock, flags);
blk_stop_queue(q);
spin_unlock_irqrestore(q->queue_lock, flags);
-
- down(&mq->thread_sem);
}
}
@@ -441,8 +434,6 @@ void mmc_queue_resume(struct mmc_queue *mq)
if (mq->flags & MMC_QUEUE_SUSPENDED) {
mq->flags &= ~MMC_QUEUE_SUSPENDED;
- up(&mq->thread_sem);
-
spin_lock_irqsave(q->queue_lock, flags);
blk_start_queue(q);
spin_unlock_irqrestore(q->queue_lock, flags);
diff --git a/drivers/mmc/card/queue.h b/drivers/mmc/card/queue.h
index 3c15a75bae86..fe10f94795de 100644
--- a/drivers/mmc/card/queue.h
+++ b/drivers/mmc/card/queue.h
@@ -53,7 +53,6 @@ struct mmc_queue_req {
struct mmc_queue {
struct mmc_card *card;
struct task_struct *thread;
- struct semaphore thread_sem;
unsigned int flags;
#define MMC_QUEUE_SUSPENDED (1 << 0)
#define MMC_QUEUE_NEW_REQUEST (1 << 1)
--
2.7.4
^ permalink raw reply related
* Re: [PATCHv2 06/10] mmc: core: move the cookie's enum values from sdhci.h to mmc.h
From: Jaehoon Chung @ 2016-11-16 9:23 UTC (permalink / raw)
To: Adrian Hunter, linux-mmc-u79uwXL29TY76Z2rM5mHXA
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, Ulf Hansson,
heiko-4mtYJXux2i+zQB+pC5nmwQ, shawn.lin-TNX95d0MmH7DzftRWevZcw,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <3e05ec88-af1d-9996-bbdb-c453f7e4bb07-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
On 11/16/2016 06:04 PM, Adrian Hunter wrote:
> On 16/11/16 10:58, Jaehoon Chung wrote:
>> On 11/16/2016 05:28 PM, Adrian Hunter wrote:
>>> On 16/11/16 10:25, Jaehoon Chung wrote:
>>>> On 11/16/2016 05:09 PM, Adrian Hunter wrote:
>>>>> On 16/11/16 09:53, Jaehoon Chung wrote:
>>>>>> Added Adrian for sdhci.h
>>>>>>
>>>>>> On 11/15/2016 07:12 PM, Jaehoon Chung wrote:
>>>>>>> It's not for only sdhci controller.
>>>>>>> So it can be moved from sdhci.h to mmc.h. And renamed from sdhci_cookie
>>>>>>> to mmc_cookie.
>>>>>
>>>>> The cookie is currently host private data, so I don't understand the
>>>>> motivation behind this.
>>>>
>>>> dwmmc controller can also use the data->host_cookie. because it's working with post/pre_req().
>>>>
>>>> So i think it can be used about both sdhci and dwmmc.
>>>> Is there no reason that add the private dwmmc data?
>>>>
>>>> With these cookie value, update the dwmmc controller for post/pre_req().
>>>>
>>>> https://patchwork.kernel.org/patch/9429287/
>>>
>>> So why not define dwmmc cookies in dw_mmc.c ?
>>
>> Because I understood that it's not sdhci specific cookies. It can be used generally, doesn't?
>>
>> Role of post/pre_req() in host controller is the doing dma_map/unmap().
>> And data's cookies only needs to notice whether dma is mapped or unmapped, etc.
>>
>> Well, If it's really private value and should be added other cookies values in future, i will put cookies in dw_mmc.h.
>> (it should be duplicated in sdhci and dwmmc.)
>
> Probably all host controllers should do cookies the same way, but they don't
> at the moment, so there is no reason to share definitions.
Ok. Then I will put dwmmc_cookie in dw_mmc.h on next version.
Thanks for comments.
Best Regards,
Jaehoon Chung
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCHv2 10/10] Documentation: synopsys-dw-mshc: remove the unused properties
From: Shawn Lin @ 2016-11-16 9:21 UTC (permalink / raw)
To: Jaehoon Chung
Cc: linux-mmc, shawn.lin, devicetree, ulf.hansson, heiko, robh+dt
In-Reply-To: <20161115101232.3854-11-jh80.chung@samsung.com>
在 2016/11/15 18:12, Jaehoon Chung 写道:
> "support-highspeed" was the obsoleted property.
> And "broken-cd" is not synopsys specific property.
> It can be referred to mmc.txt binding Documentation.
>
Ahh, I forgot to remove broken-cd from doc of dwmmc
when removing it from code.
Reviewed-by: Shawn Lin <shawn.lin@rock-chips.com>
> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
> Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> index 1279a22..7fd17c3 100644
> --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> @@ -75,11 +75,6 @@ Optional properties:
> * card-detect-delay: Delay in milli-seconds before detecting card after card
> insert event. The default value is 0.
>
> -* supports-highspeed (DEPRECATED): Enables support for high speed cards (up to 50MHz)
> - (use "cap-mmc-highspeed" or "cap-sd-highspeed" instead)
> -
> -* broken-cd: as documented in mmc core bindings.
> -
> * vmmc-supply: The phandle to the regulator to use for vmmc. If this is
> specified we'll defer probe until we can find this regulator.
>
>
--
Best Regards
Shawn Lin
^ permalink raw reply
* Re: [PATCHv2 09/10] mmc: dw_mmc: The "clock-freq-min-max" property was deprecated
From: Shawn Lin @ 2016-11-16 9:19 UTC (permalink / raw)
To: Jaehoon Chung
Cc: linux-mmc-u79uwXL29TY76Z2rM5mHXA,
shawn.lin-TNX95d0MmH7DzftRWevZcw,
devicetree-u79uwXL29TY76Z2rM5mHXA,
ulf.hansson-QSEj5FYQhm4dnm+yROfE0A, heiko-4mtYJXux2i+zQB+pC5nmwQ,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <20161115101232.3854-10-jh80.chung-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
在 2016/11/15 18:12, Jaehoon Chung 写道:
> The "clock-freq-min-max" property was deprecated.
> There is "max-frequency" property in drivers/mmc/core/host.c
> "max-frequency" can be replaced with "clock-freq-min-max".
> Minimum clock value might be set to 100K by default.
> Then MMC core should try to find the correct value from 400K to 100K.
> So it just needs to set Maximum clock value.
>
This is what we talked about to do :-)
Reviewed-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> Signed-off-by: Jaehoon Chung <jh80.chung-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
> Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt | 3 ++-
> drivers/mmc/host/dw_mmc.c | 2 ++
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> index bfa461a..1279a22 100644
> --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> @@ -59,8 +59,9 @@ Optional properties:
> is specified and the ciu clock is specified then we'll try to set the ciu
> clock to this at probe time.
>
> -* clock-freq-min-max: Minimum and Maximum clock frequency for card output
> +* clock-freq-min-max (DEPRECATED): Minimum and Maximum clock frequency for card output
> clock(cclk_out). If it's not specified, max is 200MHZ and min is 400KHz by default.
> + (Use the "max-frequency" instead of "clock-freq-min-max".)
>
> * num-slots: specifies the number of slots supported by the controller.
> The number of physical slots actually used could be equal or less than the
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index ec0ba79..48e968a 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -2608,6 +2608,8 @@ static int dw_mci_init_slot(struct dw_mci *host, unsigned int id)
> mmc->f_min = DW_MCI_FREQ_MIN;
> mmc->f_max = DW_MCI_FREQ_MAX;
> } else {
> + dev_info(host->dev,
> + "'clock-freq-min-max' property was deprecated.\n");
> mmc->f_min = freq[0];
> mmc->f_max = freq[1];
> }
>
--
Best Regards
Shawn Lin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCHv2 08/10] mmc: dw_mmc: remove the unnecessary mmc_data structure
From: Shawn Lin @ 2016-11-16 9:18 UTC (permalink / raw)
To: Jaehoon Chung
Cc: linux-mmc-u79uwXL29TY76Z2rM5mHXA,
shawn.lin-TNX95d0MmH7DzftRWevZcw,
devicetree-u79uwXL29TY76Z2rM5mHXA,
ulf.hansson-QSEj5FYQhm4dnm+yROfE0A, heiko-4mtYJXux2i+zQB+pC5nmwQ,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <20161115101232.3854-9-jh80.chung-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
在 2016/11/15 18:12, Jaehoon Chung 写道:
> Remove the unnecessary mmc_data structure.
> Instead, cmd->data can be used.
>
Looks good to me,
Reviewed-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> Signed-off-by: Jaehoon Chung <jh80.chung-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Tested-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
> ---
> drivers/mmc/host/dw_mmc.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index e912de6..ec0ba79 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -236,7 +236,6 @@ 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;
> u32 cmdr;
> @@ -291,10 +290,9 @@ static u32 dw_mci_prepare_command(struct mmc_host *mmc, struct mmc_command *cmd)
> if (cmd->flags & MMC_RSP_CRC)
> cmdr |= SDMMC_CMD_RESP_CRC;
>
> - data = cmd->data;
> - if (data) {
> + if (cmd->data) {
> cmdr |= SDMMC_CMD_DAT_EXP;
> - if (data->flags & MMC_DATA_WRITE)
> + if (cmd->data->flags & MMC_DATA_WRITE)
> cmdr |= SDMMC_CMD_DAT_WR;
> }
>
>
--
Best Regards
Shawn Lin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCHv2 05/10] mmc: dw_mmc: call the dw_mci_prep_stop_abort() by default
From: Shawn Lin @ 2016-11-16 9:16 UTC (permalink / raw)
To: Jaehoon Chung, linux-mmc-u79uwXL29TY76Z2rM5mHXA
Cc: shawn.lin-TNX95d0MmH7DzftRWevZcw,
devicetree-u79uwXL29TY76Z2rM5mHXA,
ulf.hansson-QSEj5FYQhm4dnm+yROfE0A, heiko-4mtYJXux2i+zQB+pC5nmwQ,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <20161115101232.3854-6-jh80.chung-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
在 2016/11/15 18:12, Jaehoon Chung 写道:
> stop_cmdr should be set to values relevant to stop command.
> It migth be assigned to values whatever there is mrq->stop or not.
> Then it doesn't need to use dw_mci_prepare_command().
> It's enough to use the prep_stop_abort for preparing stop command.
>
Have you considered to clean up the logic of preparing abort cmd
within dw_mci_prepare_command?
if (cmd->opcode == MMC_STOP_TRANSMISSION ||
cmd->opcode == MMC_GO_IDLE_STATE ||
cmd->opcode == MMC_GO_INACTIVE_STATE ||
(cmd->opcode == SD_IO_RW_DIRECT &&
((cmd->arg >> 9) & 0x1FFFF) == SDIO_CCCR_ABORT))
cmdr |= SDMMC_CMD_STOP;
> Signed-off-by: Jaehoon Chung <jh80.chung-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Tested-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
> ---
> drivers/mmc/host/dw_mmc.c | 15 +++++----------
> 1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 3cda68c..12e1107 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -385,7 +385,7 @@ static void dw_mci_start_command(struct dw_mci *host,
>
> static inline void send_stop_abort(struct dw_mci *host, struct mmc_data *data)
> {
> - struct mmc_command *stop = data->stop ? data->stop : &host->stop_abort;
> + struct mmc_command *stop = &host->stop_abort;
>
> dw_mci_start_command(host, stop, host->stop_cmdr);
> }
> @@ -1277,10 +1277,7 @@ static void __dw_mci_start_request(struct dw_mci *host,
> spin_unlock_irqrestore(&host->irq_lock, irqflags);
> }
>
> - if (mrq->stop)
> - host->stop_cmdr = dw_mci_prepare_command(slot->mmc, mrq->stop);
> - else
> - host->stop_cmdr = dw_mci_prep_stop_abort(host, cmd);
> + host->stop_cmdr = dw_mci_prep_stop_abort(host, cmd);
> }
>
> static void dw_mci_start_request(struct dw_mci *host,
> @@ -1890,8 +1887,7 @@ static void dw_mci_tasklet_func(unsigned long priv)
> if (test_and_clear_bit(EVENT_DATA_ERROR,
> &host->pending_events)) {
> dw_mci_stop_dma(host);
> - if (data->stop ||
> - !(host->data_status & (SDMMC_INT_DRTO |
> + if (!(host->data_status & (SDMMC_INT_DRTO |
> SDMMC_INT_EBE)))
> send_stop_abort(host, data);
> state = STATE_DATA_ERROR;
> @@ -1927,8 +1923,7 @@ static void dw_mci_tasklet_func(unsigned long priv)
> if (test_and_clear_bit(EVENT_DATA_ERROR,
> &host->pending_events)) {
> dw_mci_stop_dma(host);
> - if (data->stop ||
> - !(host->data_status & (SDMMC_INT_DRTO |
> + if (!(host->data_status & (SDMMC_INT_DRTO |
> SDMMC_INT_EBE)))
> send_stop_abort(host, data);
> state = STATE_DATA_ERROR;
> @@ -2004,7 +1999,7 @@ static void dw_mci_tasklet_func(unsigned long priv)
> host->cmd = NULL;
> host->data = NULL;
>
> - if (mrq->stop)
> + if (!mrq->sbc && mrq->stop)
> dw_mci_command_complete(host, mrq->stop);
> else
> host->cmd_status = 0;
>
--
Best Regards
Shawn Lin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCHv2 04/10] mmc: dw_mmc: use the hold register when send stop command
From: Shawn Lin @ 2016-11-16 9:09 UTC (permalink / raw)
To: Jaehoon Chung, linux-mmc-u79uwXL29TY76Z2rM5mHXA
Cc: shawn.lin-TNX95d0MmH7DzftRWevZcw,
devicetree-u79uwXL29TY76Z2rM5mHXA,
ulf.hansson-QSEj5FYQhm4dnm+yROfE0A, heiko-4mtYJXux2i+zQB+pC5nmwQ,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <20161115101232.3854-5-jh80.chung-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
在 2016/11/15 18:12, Jaehoon Chung 写道:
> If DW_MMC_CARD_NO_USE_HOLD isn't set, it's usesd by default.
> Enve if SDMMC_CMD_USB_HOLD_REG is set in prepare_command(), but it
> doesn't set in pre_stop_abort().
>
> To maintain the consistency, add the checking condition for this.
Actually I hacked the driver w/ or w/o this patch to see how the
controller would do when looking at your V1.
Interesting, I didn't see failure when missing to set this...But
definitely it should be fixed.
Reviewed-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>
> Signed-off-by: Jaehoon Chung <jh80.chung-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Tested-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
> ---
> drivers/mmc/host/dw_mmc.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index be36f48..3cda68c 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -337,6 +337,9 @@ static u32 dw_mci_prep_stop_abort(struct dw_mci *host, struct mmc_command *cmd)
> cmdr = stop->opcode | SDMMC_CMD_STOP |
> SDMMC_CMD_RESP_CRC | SDMMC_CMD_RESP_EXP;
>
> + if (!test_bit(DW_MMC_CARD_NO_USE_HOLD, &host->cur_slot->flags))
> + cmdr |= SDMMC_CMD_USE_HOLD_REG;
> +
> return cmdr;
> }
>
>
--
Best Regards
Shawn Lin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCHv2 06/10] mmc: core: move the cookie's enum values from sdhci.h to mmc.h
From: Adrian Hunter @ 2016-11-16 9:04 UTC (permalink / raw)
To: Jaehoon Chung, linux-mmc
Cc: devicetree, Ulf Hansson, heiko, shawn.lin, robh+dt
In-Reply-To: <4168313a-07bc-00a7-ecc8-1ca4978c4d8c@samsung.com>
On 16/11/16 10:58, Jaehoon Chung wrote:
> On 11/16/2016 05:28 PM, Adrian Hunter wrote:
>> On 16/11/16 10:25, Jaehoon Chung wrote:
>>> On 11/16/2016 05:09 PM, Adrian Hunter wrote:
>>>> On 16/11/16 09:53, Jaehoon Chung wrote:
>>>>> Added Adrian for sdhci.h
>>>>>
>>>>> On 11/15/2016 07:12 PM, Jaehoon Chung wrote:
>>>>>> It's not for only sdhci controller.
>>>>>> So it can be moved from sdhci.h to mmc.h. And renamed from sdhci_cookie
>>>>>> to mmc_cookie.
>>>>
>>>> The cookie is currently host private data, so I don't understand the
>>>> motivation behind this.
>>>
>>> dwmmc controller can also use the data->host_cookie. because it's working with post/pre_req().
>>>
>>> So i think it can be used about both sdhci and dwmmc.
>>> Is there no reason that add the private dwmmc data?
>>>
>>> With these cookie value, update the dwmmc controller for post/pre_req().
>>>
>>> https://patchwork.kernel.org/patch/9429287/
>>
>> So why not define dwmmc cookies in dw_mmc.c ?
>
> Because I understood that it's not sdhci specific cookies. It can be used generally, doesn't?
>
> Role of post/pre_req() in host controller is the doing dma_map/unmap().
> And data's cookies only needs to notice whether dma is mapped or unmapped, etc.
>
> Well, If it's really private value and should be added other cookies values in future, i will put cookies in dw_mmc.h.
> (it should be duplicated in sdhci and dwmmc.)
Probably all host controllers should do cookies the same way, but they don't
at the moment, so there is no reason to share definitions.
^ permalink raw reply
* Re: [PATCHv2 03/10] mmc: dw_mmc: change the DW_MCI_FREQ_MIN from 400K to 100K
From: Shawn Lin @ 2016-11-16 9:06 UTC (permalink / raw)
To: Jaehoon Chung, linux-mmc-u79uwXL29TY76Z2rM5mHXA
Cc: shawn.lin-TNX95d0MmH7DzftRWevZcw,
devicetree-u79uwXL29TY76Z2rM5mHXA,
ulf.hansson-QSEj5FYQhm4dnm+yROfE0A, heiko-4mtYJXux2i+zQB+pC5nmwQ,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <20161115101232.3854-4-jh80.chung-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
在 2016/11/15 18:12, Jaehoon Chung 写道:
> If there is no property "clock-freq-min-max", mmc->f_min should be set
> to 400K by default. But Some SoC can be used 100K.
> When 100K is used, MMC core will try to check from 400K to 100K.
>
> Reported-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> Signed-off-by: Jaehoon Chung <jh80.chung-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Tested-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
It makes more sense that the original 400KHz, thanks for improving
that.
Reviewed-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> ---
> drivers/mmc/host/dw_mmc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 6c0c4c5..be36f48 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -54,7 +54,7 @@
> #define DW_MCI_DMA_THRESHOLD 16
>
> #define DW_MCI_FREQ_MAX 200000000 /* unit: HZ */
> -#define DW_MCI_FREQ_MIN 400000 /* unit: HZ */
> +#define DW_MCI_FREQ_MIN 100000 /* unit: HZ */
>
> #define IDMAC_INT_CLR (SDMMC_IDMAC_INT_AI | SDMMC_IDMAC_INT_NI | \
> SDMMC_IDMAC_INT_CES | SDMMC_IDMAC_INT_DU | \
>
--
Best Regards
Shawn Lin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCHv2 01/10] mmc: dw_mmc: display the real register value on debugfs
From: Shawn Lin @ 2016-11-16 9:04 UTC (permalink / raw)
To: Jaehoon Chung
Cc: linux-mmc-u79uwXL29TY76Z2rM5mHXA,
shawn.lin-TNX95d0MmH7DzftRWevZcw,
devicetree-u79uwXL29TY76Z2rM5mHXA,
ulf.hansson-QSEj5FYQhm4dnm+yROfE0A, heiko-4mtYJXux2i+zQB+pC5nmwQ,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <20161115101232.3854-2-jh80.chung-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
在 2016/11/15 18:12, Jaehoon Chung 写道:
> Developer wants to see the real register value, not register offset.
> This patch fixed to display the real value of register.
>
IIRC, you had submited some similar patch but I don't remember
why it wasn't merged?
Anyway,
Reviewed-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> Signed-off-by: Jaehoon Chung <jh80.chung-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Tested-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
> ---
> drivers/mmc/host/dw_mmc.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index a16c537..e53899e 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -165,12 +165,14 @@ static const struct file_operations dw_mci_req_fops = {
>
> static int dw_mci_regs_show(struct seq_file *s, void *v)
> {
> - seq_printf(s, "STATUS:\t0x%08x\n", SDMMC_STATUS);
> - seq_printf(s, "RINTSTS:\t0x%08x\n", SDMMC_RINTSTS);
> - seq_printf(s, "CMD:\t0x%08x\n", SDMMC_CMD);
> - seq_printf(s, "CTRL:\t0x%08x\n", SDMMC_CTRL);
> - seq_printf(s, "INTMASK:\t0x%08x\n", SDMMC_INTMASK);
> - seq_printf(s, "CLKENA:\t0x%08x\n", SDMMC_CLKENA);
> + struct dw_mci *host = s->private;
> +
> + seq_printf(s, "STATUS:\t0x%08x\n", mci_readl(host, STATUS));
> + seq_printf(s, "RINTSTS:\t0x%08x\n", mci_readl(host, RINTSTS));
> + seq_printf(s, "CMD:\t0x%08x\n", mci_readl(host, CMD));
> + seq_printf(s, "CTRL:\t0x%08x\n", mci_readl(host, CTRL));
> + seq_printf(s, "INTMASK:\t0x%08x\n", mci_readl(host, INTMASK));
> + seq_printf(s, "CLKENA:\t0x%08x\n", mci_readl(host, CLKENA));
>
> return 0;
> }
>
--
Best Regards
Shawn Lin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCHv2 02/10] mmc: dw_mmc: fix the debug message for checking card's present
From: Shawn Lin @ 2016-11-16 9:01 UTC (permalink / raw)
To: Jaehoon Chung
Cc: linux-mmc-u79uwXL29TY76Z2rM5mHXA,
shawn.lin-TNX95d0MmH7DzftRWevZcw,
devicetree-u79uwXL29TY76Z2rM5mHXA,
ulf.hansson-QSEj5FYQhm4dnm+yROfE0A, heiko-4mtYJXux2i+zQB+pC5nmwQ,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <20161115101232.3854-3-jh80.chung-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
On 2016/11/15 18:12, Jaehoon Chung wrote:
> If display the debug message, this message should be spamming.
> If flags is maintained the previous value, didn't display the debug
> message.
>
> Signed-off-by: Jaehoon Chung <jh80.chung-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Tested-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
Looks correct to me,
Reviewed-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> ---
> drivers/mmc/host/dw_mmc.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index e53899e..6c0c4c5 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1537,13 +1537,10 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
> == 0 ? 1 : 0;
>
> spin_lock_bh(&host->lock);
> - if (present) {
> - set_bit(DW_MMC_CARD_PRESENT, &slot->flags);
> + if (present && !test_and_set_bit(DW_MMC_CARD_PRESENT, &slot->flags))
> dev_dbg(&mmc->class_dev, "card is present\n");
> - } else {
> - clear_bit(DW_MMC_CARD_PRESENT, &slot->flags);
> + else if (!test_and_clear_bit(DW_MMC_CARD_PRESENT, &slot->flags))
> dev_dbg(&mmc->class_dev, "card is not present\n");
> - }
> spin_unlock_bh(&host->lock);
>
> return present;
>
--
Best Regards
Shawn Lin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCHv2 06/10] mmc: core: move the cookie's enum values from sdhci.h to mmc.h
From: Jaehoon Chung @ 2016-11-16 8:58 UTC (permalink / raw)
To: Adrian Hunter, linux-mmc-u79uwXL29TY76Z2rM5mHXA
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, Ulf Hansson,
heiko-4mtYJXux2i+zQB+pC5nmwQ, shawn.lin-TNX95d0MmH7DzftRWevZcw,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <d5321daf-b449-e556-ccf1-cd82d497ea85-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
On 11/16/2016 05:28 PM, Adrian Hunter wrote:
> On 16/11/16 10:25, Jaehoon Chung wrote:
>> On 11/16/2016 05:09 PM, Adrian Hunter wrote:
>>> On 16/11/16 09:53, Jaehoon Chung wrote:
>>>> Added Adrian for sdhci.h
>>>>
>>>> On 11/15/2016 07:12 PM, Jaehoon Chung wrote:
>>>>> It's not for only sdhci controller.
>>>>> So it can be moved from sdhci.h to mmc.h. And renamed from sdhci_cookie
>>>>> to mmc_cookie.
>>>
>>> The cookie is currently host private data, so I don't understand the
>>> motivation behind this.
>>
>> dwmmc controller can also use the data->host_cookie. because it's working with post/pre_req().
>>
>> So i think it can be used about both sdhci and dwmmc.
>> Is there no reason that add the private dwmmc data?
>>
>> With these cookie value, update the dwmmc controller for post/pre_req().
>>
>> https://patchwork.kernel.org/patch/9429287/
>
> So why not define dwmmc cookies in dw_mmc.c ?
Because I understood that it's not sdhci specific cookies. It can be used generally, doesn't?
Role of post/pre_req() in host controller is the doing dma_map/unmap().
And data's cookies only needs to notice whether dma is mapped or unmapped, etc.
Well, If it's really private value and should be added other cookies values in future, i will put cookies in dw_mmc.h.
(it should be duplicated in sdhci and dwmmc.)
Best Regards,
Jaehoon Chung
>
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v7 08/14] mmc: sdhci-msm: Implement set_clock callback for sdhci-msm
From: Ritesh Harjani @ 2016-11-16 8:53 UTC (permalink / raw)
To: Adrian Hunter, Stephen Boyd, ulf.hansson
Cc: linux-mmc, shawn.lin, andy.gross, devicetree, linux-clk,
david.brown, linux-arm-msm, georgi.djakov, alex.lemberg,
mateusz.nowak, Yuliy.Izrailov, asutoshd, kdorfman, david.griego,
stummala, venkatg, rnayak, pramod.gurav
In-Reply-To: <875c9051-bd8b-60cf-82e4-3e26612a8623@intel.com>
On 11/16/2016 1:12 PM, Adrian Hunter wrote:
> On 16/11/16 06:42, Ritesh Harjani wrote:
>> Hi,
>>
>> On 11/15/2016 10:40 AM, Ritesh Harjani wrote:
>>> Hi Stephen/Adrian,
>>>
>>> On 11/15/2016 1:07 AM, Stephen Boyd wrote:
>>>> On 11/14, Ritesh Harjani wrote:
>>>>> @@ -577,6 +578,90 @@ static unsigned int
>>>>> sdhci_msm_get_min_clock(struct sdhci_host *host)
>>>>> return SDHCI_MSM_MIN_CLOCK;
>>>>> }
>>>>>
>>>>> +/**
>>>>> + * __sdhci_msm_set_clock - sdhci_msm clock control.
>>>>> + *
>>>>> + * Description:
>>>>> + * Implement MSM version of sdhci_set_clock.
>>>>> + * This is required since MSM controller does not
>>>>> + * use internal divider and instead directly control
>>>>> + * the GCC clock as per HW recommendation.
>>>>> + **/
>>>>> +void __sdhci_msm_set_clock(struct sdhci_host *host, unsigned int clock)
>>>>> +{
>>>>> + u16 clk;
>>>>> + unsigned long timeout;
>>>>> +
>>>>> + /*
>>>>> + * Keep actual_clock as zero -
>>>>> + * - since there is no divider used so no need of having
>>>>> actual_clock.
>>>>> + * - MSM controller uses SDCLK for data timeout calculation. If
>>>>> + * actual_clock is zero, host->clock is taken for calculation.
>>>>> + */
>>>>> + host->mmc->actual_clock = 0;
>>>>> +
>>>>> + sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
>>>>> +
>>>>> + if (clock == 0)
>>>>> + return;
>>>>> +
>>>>> + /*
>>>>> + * MSM controller do not use clock divider.
>>>>> + * Thus read SDHCI_CLOCK_CONTROL and only enable
>>>>> + * clock with no divider value programmed.
>>>>> + */
>>>>> + clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
>>>>> +
>>>>> + clk |= SDHCI_CLOCK_INT_EN;
>>>>> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>>>>> +
>>>>> + /* Wait max 20 ms */
>>>>> + timeout = 20;
>>>>> + while (!((clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
>>>>> + & SDHCI_CLOCK_INT_STABLE)) {
>>>>> + if (timeout == 0) {
>>>>> + pr_err("%s: Internal clock never stabilised\n",
>>>>> + mmc_hostname(host->mmc));
>>>>> + return;
>>>>> + }
>>>>> + timeout--;
>>>>> + mdelay(1);
>>>>> + }
>>>>> +
>>>>> + clk |= SDHCI_CLOCK_CARD_EN;
>>>>> + sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>>>>
>>>> This is almost a copy/paste of sdhci_set_clock(). Can we make
>>>> sdhci_set_clock() call a __sdhci_set_clock() function that takes
>>>> unsigned int clock, and also a flag indicating if we want to set
>>>> the internal clock divider or not? Then we can call
>>>> __sdhci_set_clock() from sdhci_set_clock() with (clock, true) as
>>>> arguments and (clock, false).
>> Actually what you may be referring here is some sort of quirks which is not
>> entertained any more for sdhci driver.
>> sdhci is tending towards becoming a library and hence such changes are
>> restricted.
>>
>> But I think we may do below changes to avoid duplication of code which
>> enables the sdhci internal clock and waits for internal clock to be stable.
>>
>> Adrian, could you please tell if this should be ok?
>
> That seems fine, but the name seems too long - how about changing
> sdhci_set_clock_enable to sdhci_enable_clk.
Sure. Will make the changes then.
>
>> Then we may be able to call for sdhci_set_clock_enable function from
>> sdhci_msm_set_clock.
>>
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 42ef3eb..28e605c 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -1343,19 +1343,8 @@ u16 sdhci_calc_clk(struct sdhci_host *host, unsigned
>> int clock,
>> }
>> EXPORT_SYMBOL_GPL(sdhci_calc_clk);
>>
>> -void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
>> +void sdhci_set_clock_enable(struct sdhci_host *host, unsigned short clk)
>> {
>> - u16 clk;
>> - unsigned long timeout;
>> -
>> - host->mmc->actual_clock = 0;
>> -
>> - sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
>> -
>> - if (clock == 0)
>> - return;
>> -
>> - clk = sdhci_calc_clk(host, clock, &host->mmc->actual_clock);
>>
>> clk |= SDHCI_CLOCK_INT_EN;
>> sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>> @@ -1377,6 +1366,24 @@ void sdhci_set_clock(struct sdhci_host *host,
>> unsigned int clock)
>> clk |= SDHCI_CLOCK_CARD_EN;
>> sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>> }
>> +EXPORT_SYMBOL_GPL(sdhci_set_clock_enable);
>> +
>> +void sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
>> +{
>> + u16 clk;
>> + unsigned long timeout;
>> +
>> + host->mmc->actual_clock = 0;
>> +
>> + sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
>> +
>> + if (clock == 0)
>> + return;
>> +
>> + clk = sdhci_calc_clk(host, clock, &host->mmc->actual_clock);
>> +
>> + sdhci_set_clock_enable(host, clk);
>> +}
>> EXPORT_SYMBOL_GPL(sdhci_set_clock);
>>
>> static void sdhci_set_power_reg(struct sdhci_host *host, unsigned char mode,
>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>> index 766df17..43382e1 100644
>> --- a/drivers/mmc/host/sdhci.h
>> +++ b/drivers/mmc/host/sdhci.h
>> @@ -681,6 +681,7 @@ static inline bool sdhci_sdio_irq_enabled(struct
>> sdhci_host *host)
>> u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
>> unsigned int *actual_clock);
>> void sdhci_set_clock(struct sdhci_host *host, unsigned int clock);
>> +void sdhci_set_clock_enable(struct sdhci_host *host, unsigned short clk);
>> void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
>> unsigned short vdd);
>> void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode,
>>
>>
>>
>>> Adrian,
>>> Could you please comment here ?
>>>
>>>>
>>>>> +}
>>>>> +
>>>>> +/* sdhci_msm_set_clock - Called with (host->lock) spinlock held. */
>>>>> +static void sdhci_msm_set_clock(struct sdhci_host *host, unsigned
>>>>> int clock)
>>>>> +{
>>>>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>>> + struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
>>>>> + int rc;
>>>>> +
>>>>> + if (!clock) {
>>>>> + msm_host->clk_rate = clock;
>>>>> + goto out;
>>>>> + }
>>>>> +
>>>>> + spin_unlock_irq(&host->lock);
>>>>> + if (clock != msm_host->clk_rate) {
>>>>
>>>> Why do we need to check here? Can't we call clk_set_rate()
>>>> Unconditionally?
>>> Since it may so happen that above layers may call for ->set_clock
>>> function with same requested clock more than once, hence we cache the
>>> host->clock here.
>>> Also, since requested clock (host->clock) can be say 400Mhz but the
>>> actual pltfm supported clock would be say 384MHz.
>>>
>>>
>>>>
>>>>> + rc = clk_set_rate(msm_host->clk, clock);
>>>>> + if (rc) {
>>>>> + pr_err("%s: Failed to set clock at rate %u\n",
>>>>> + mmc_hostname(host->mmc), clock);
>>>>> + spin_lock_irq(&host->lock);
>>>>> + goto out;
>>>>
>>>> Or replace the above two lines with goto err;
>>> Ok, I will have another label out_lock instead of err.
>>>>
>>>>> + }
>>>>> + msm_host->clk_rate = clock;
>>>>> + pr_debug("%s: Setting clock at rate %lu\n",
>>>>> + mmc_hostname(host->mmc), clk_get_rate(msm_host->clk));
>>>>> + }
>>>>
>>>> And put an err label here.
>>> will put the label here as out_lock;
>>>>
>>>>> + spin_lock_irq(&host->lock);
>>>>> +out:
>>>>> + __sdhci_msm_set_clock(host, clock);
>>>>> +}
>>>>> +
>>>>> static const struct of_device_id sdhci_msm_dt_match[] = {
>>>>> { .compatible = "qcom,sdhci-msm-v4" },
>>>>> {},
>>>>
>>>
>>
>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCHv2 06/10] mmc: core: move the cookie's enum values from sdhci.h to mmc.h
From: Jaehoon Chung @ 2016-11-16 8:25 UTC (permalink / raw)
To: Adrian Hunter, linux-mmc
Cc: devicetree, Ulf Hansson, heiko, shawn.lin, robh+dt
In-Reply-To: <ecb4b5d0-4cbf-bf44-3287-f1580b2e990a@intel.com>
On 11/16/2016 05:09 PM, Adrian Hunter wrote:
> On 16/11/16 09:53, Jaehoon Chung wrote:
>> Added Adrian for sdhci.h
>>
>> On 11/15/2016 07:12 PM, Jaehoon Chung wrote:
>>> It's not for only sdhci controller.
>>> So it can be moved from sdhci.h to mmc.h. And renamed from sdhci_cookie
>>> to mmc_cookie.
>
> The cookie is currently host private data, so I don't understand the
> motivation behind this.
dwmmc controller can also use the data->host_cookie. because it's working with post/pre_req().
So i think it can be used about both sdhci and dwmmc.
Is there no reason that add the private dwmmc data?
With these cookie value, update the dwmmc controller for post/pre_req().
https://patchwork.kernel.org/patch/9429287/
Best Regards,
Jaehoon Chung
>
>>>
>>> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
>>> Tested-by: Heiko Stuebner <heiko@sntech.de>
>>> ---
>>> drivers/mmc/host/sdhci.h | 6 ------
>>> include/linux/mmc/core.h | 6 ++++++
>>> 2 files changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>>> index 766df17..325663b 100644
>>> --- a/drivers/mmc/host/sdhci.h
>>> +++ b/drivers/mmc/host/sdhci.h
>>> @@ -321,12 +321,6 @@ struct sdhci_adma2_64_desc {
>>> /* Allow for a a command request and a data request at the same time */
>>> #define SDHCI_MAX_MRQS 2
>>>
>>> -enum sdhci_cookie {
>>> - COOKIE_UNMAPPED,
>>> - COOKIE_PRE_MAPPED, /* mapped by sdhci_pre_req() */
>>> - COOKIE_MAPPED, /* mapped by sdhci_prepare_data() */
>>> -};
>>> -
>>> struct sdhci_host {
>>> /* Data set by hardware interface driver */
>>> const char *hw_name; /* Hardware bus name */
>>> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
>>> index 0ce928b..82d707f 100644
>>> --- a/include/linux/mmc/core.h
>>> +++ b/include/linux/mmc/core.h
>>> @@ -118,6 +118,12 @@ struct mmc_command {
>>> struct mmc_request *mrq; /* associated request */
>>> };
>>>
>>> +enum mmc_cookie {
>>> + COOKIE_UNMAPPED,
>>> + COOKIE_PRE_MAPPED, /* mapped by pre_req() of controller */
>>> + COOKIE_MAPPED, /* mapped by prepare_data() of controller */
>>> +};
>>> +
>>> struct mmc_data {
>>> unsigned int timeout_ns; /* data timeout (in ns, max 80ms) */
>>> unsigned int timeout_clks; /* data timeout (in clocks) */
>>>
>>
>>
>
> --
> 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
>
>
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox