* RE: [PATCH v3 2/3] mmc: tmio-mmc: add support for 32bit data port
From: Chris Brandt @ 2016-11-01 15:13 UTC (permalink / raw)
To: Wolfram Sang
Cc: Ulf Hansson, Wolfram Sang, Sergei Shtylyov, Geert Uytterhoeven,
Simon Horman, linux-mmc@vger.kernel.org,
linux-renesas-soc@vger.kernel.org
In-Reply-To: <20161101150759.GA1436@katana>
> > I don't understand "TMIO_MMC_BIG_DATA_PORT" (How do you know 32-bit??
> How do you know 64-bit??)
>
> I think 16 << host->bus_shift should work; need to verify this, though.
Good idea.
^ permalink raw reply
* [PATCH] sdhci-esdhc-imx: fix bus-width for 1-bit operation.
From: Leonardo G. Veiga @ 2016-11-01 15:58 UTC (permalink / raw)
To: ulf.hansson; +Cc: linux-mmc, linux-kernel, stefan, raul.munoz, leonardo.veiga
From: Leonardo Graboski Veiga <leonardo.veiga@toradex.com>
The 1-bit operation mode, enabled by seeting the 'bus-width' property of
the device tree 'esdhc' node to <1>, not work while using SD card.
The behavior is only noticed when only the data pin 0 is connected to the
hardware. A series of kernel errors are printed to the console, all of them
returning the following error message followed by some explanation:
mmcblk0: error -84 transferring data
If four data lines are connected, it ignores the device-tree
property and works in 4-bit mode of operation without errors. The hardware
used for testing does not support 8-bit mode.
Check the 'bus-width' property and if set to <1>, enable the
SDHCI_QUIRK_FORCE_1_BIT_DATA quirk.
Signed-off-by: Leonardo Graboski Veiga <leonardo.veiga@toradex.com>
---
drivers/mmc/host/sdhci-esdhc-imx.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index c9fbc4c3..88d7d22 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -1003,6 +1003,10 @@ sdhci_esdhc_imx_probe_dt(struct platform_device *pdev,
host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
}
+ if (!of_property_read_u32(np, "bus-width", &boarddata->max_bus_width)
+ && boarddata->max_bus_width == 1)
+ host->quirks |= SDHCI_QUIRK_FORCE_1_BIT_DATA;
+
/* call to generic mmc_of_parse to support additional capabilities */
ret = mmc_of_parse(host->mmc);
if (ret)
--
2.7.4
^ permalink raw reply related
* [PATCH v2 0/2] mmc: sdhci-iproc: Add byte register access support
From: Scott Branden @ 2016-11-01 16:37 UTC (permalink / raw)
To: Ulf Hansson, Rob Herring, Mark Rutland, Ray Jui, Scott Branden,
Adrian Hunter
Cc: BCM Kernel Feedback, linux-mmc, devicetree, linux-arm-kernel,
linux-kernel, Scott Branden
Add brcm,sdhci-iproc compat string and code for support of newer versions of
sdhci-iproc controller that allow byte-wise register accesses.
Changes from v1:
- added details to bindings documentation to clarify usage of brcm,sdhci-iproc
compatibility string
Scott Branden (2):
mmc: sdhci-iproc: Add brcm,sdhci-iproc compat string in bindings
document
mmc: sdhci-iproc: support standard byte register accesses
.../devicetree/bindings/mmc/brcm,sdhci-iproc.txt | 9 ++++++
drivers/mmc/host/sdhci-iproc.c | 35 ++++++++++++++++++++--
2 files changed, 42 insertions(+), 2 deletions(-)
--
2.5.0
^ permalink raw reply
* [PATCH v2 2/2] mmc: sdhci-iproc: support standard byte register accesses
From: Scott Branden @ 2016-11-01 16:37 UTC (permalink / raw)
To: Ulf Hansson, Rob Herring, Mark Rutland, Ray Jui, Scott Branden,
Adrian Hunter
Cc: devicetree, Scott Branden, linux-mmc, linux-kernel,
BCM Kernel Feedback, Srinath Mannam, linux-arm-kernel
In-Reply-To: <1478018277-10097-1-git-send-email-scott.branden@broadcom.com>
Add bytewise register accesses support for newer versions of IPROC
SDHCI controllers.
Previous sdhci-iproc versions of SDIO controllers
(such as Raspberry Pi and Cygnus) only allowed for 32-bit register
accesses.
Signed-off-by: Srinath Mannam <srinath.mannam@broadcom.com>
Signed-off-by: Scott Branden <scott.branden@broadcom.com>
---
drivers/mmc/host/sdhci-iproc.c | 35 +++++++++++++++++++++++++++++++++--
1 file changed, 33 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
index 7262466..d7046d6 100644
--- a/drivers/mmc/host/sdhci-iproc.c
+++ b/drivers/mmc/host/sdhci-iproc.c
@@ -143,6 +143,14 @@ static void sdhci_iproc_writeb(struct sdhci_host *host, u8 val, int reg)
}
static const struct sdhci_ops sdhci_iproc_ops = {
+ .set_clock = sdhci_set_clock,
+ .get_max_clock = sdhci_pltfm_clk_get_max_clock,
+ .set_bus_width = sdhci_set_bus_width,
+ .reset = sdhci_reset,
+ .set_uhs_signaling = sdhci_set_uhs_signaling,
+};
+
+static const struct sdhci_ops sdhci_iproc_32only_ops = {
.read_l = sdhci_iproc_readl,
.read_w = sdhci_iproc_readw,
.read_b = sdhci_iproc_readb,
@@ -156,6 +164,28 @@ static const struct sdhci_ops sdhci_iproc_ops = {
.set_uhs_signaling = sdhci_set_uhs_signaling,
};
+static const struct sdhci_pltfm_data sdhci_iproc_cygnus_pltfm_data = {
+ .quirks = SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK,
+ .quirks2 = SDHCI_QUIRK2_ACMD23_BROKEN,
+ .ops = &sdhci_iproc_32only_ops,
+};
+
+static const struct sdhci_iproc_data iproc_cygnus_data = {
+ .pdata = &sdhci_iproc_cygnus_pltfm_data,
+ .caps = ((0x1 << SDHCI_MAX_BLOCK_SHIFT)
+ & SDHCI_MAX_BLOCK_MASK) |
+ SDHCI_CAN_VDD_330 |
+ SDHCI_CAN_VDD_180 |
+ SDHCI_CAN_DO_SUSPEND |
+ SDHCI_CAN_DO_HISPD |
+ SDHCI_CAN_DO_ADMA2 |
+ SDHCI_CAN_DO_SDMA,
+ .caps1 = SDHCI_DRIVER_TYPE_C |
+ SDHCI_DRIVER_TYPE_D |
+ SDHCI_SUPPORT_DDR50,
+ .mmc_caps = MMC_CAP_1_8V_DDR,
+};
+
static const struct sdhci_pltfm_data sdhci_iproc_pltfm_data = {
.quirks = SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK,
.quirks2 = SDHCI_QUIRK2_ACMD23_BROKEN,
@@ -182,7 +212,7 @@ static const struct sdhci_pltfm_data sdhci_bcm2835_pltfm_data = {
.quirks = SDHCI_QUIRK_BROKEN_CARD_DETECTION |
SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK |
SDHCI_QUIRK_MISSING_CAPS,
- .ops = &sdhci_iproc_ops,
+ .ops = &sdhci_iproc_32only_ops,
};
static const struct sdhci_iproc_data bcm2835_data = {
@@ -194,7 +224,8 @@ static const struct sdhci_iproc_data bcm2835_data = {
static const struct of_device_id sdhci_iproc_of_match[] = {
{ .compatible = "brcm,bcm2835-sdhci", .data = &bcm2835_data },
- { .compatible = "brcm,sdhci-iproc-cygnus", .data = &iproc_data },
+ { .compatible = "brcm,sdhci-iproc-cygnus", .data = &iproc_cygnus_data},
+ { .compatible = "brcm,sdhci-iproc", .data = &iproc_data },
{ }
};
MODULE_DEVICE_TABLE(of, sdhci_iproc_of_match);
--
2.5.0
^ permalink raw reply related
* [PATCH v2 1/2] mmc: sdhci-iproc: Add brcm,sdhci-iproc compat string in bindings document
From: Scott Branden @ 2016-11-01 16:37 UTC (permalink / raw)
To: Ulf Hansson, Rob Herring, Mark Rutland, Ray Jui, Scott Branden,
Adrian Hunter
Cc: BCM Kernel Feedback, linux-mmc, devicetree, linux-arm-kernel,
linux-kernel, Scott Branden, Anup Patel
In-Reply-To: <1478018277-10097-1-git-send-email-scott.branden@broadcom.com>
Adds brcm,sdhci-iproc compat string to DT bindings document for
the iProc SDHCI driver.
Signed-off-by: Anup Patel <anup.patel@broadcom.com>
Signed-off-by: Scott Branden <scott.branden@broadcom.com>
---
Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt b/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
index be56d2b..954561d 100644
--- a/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
+++ b/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
@@ -7,6 +7,15 @@ Required properties:
- compatible : Should be one of the following
"brcm,bcm2835-sdhci"
"brcm,sdhci-iproc-cygnus"
+ "brcm,sdhci-iproc"
+
+Use brcm2835-sdhci for Rasperry PI.
+
+Use sdhci-iproc-cygnus for Broadcom SDHCI Controllers
+restricted to 32bit host accesses to SDHCI registers.
+
+Use sdhci-iproc for Broadcom SDHCI Controllers that allow standard
+8, 16, 32-bit host access to SDHCI register.
- clocks : The clock feeding the SDHCI controller.
--
2.5.0
^ permalink raw reply related
* Re: [PATCH 1/2] mmc: sdhci-iproc: Add brcm,sdhci-iproc compat string in bindings document
From: Scott Branden @ 2016-11-01 16:40 UTC (permalink / raw)
To: Rob Herring
Cc: Ulf Hansson, Mark Rutland, Ray Jui, Scott Branden, Adrian Hunter,
BCM Kernel Feedback, linux-mmc-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Anup Patel
In-Reply-To: <93502f42-ae86-742f-a8b1-3645005fa98e-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Hi Rob,
On 16-10-18 01:08 PM, Scott Branden wrote:
> Hi Rob,
>
> On 16-10-18 06:16 AM, Rob Herring wrote:
>> On Wed, Oct 12, 2016 at 11:35:51AM -0700, Scott Branden wrote:
>>> Adds brcm,sdhci-iproc compat string to DT bindings document for
>>> the iProc SDHCI driver.
>>>
>>> Signed-off-by: Anup Patel <anup.patel-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>>> Signed-off-by: Scott Branden <scott.branden-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
>>> ---
>>> Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
>>> b/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
>>> index be56d2b..aa58b94 100644
>>> --- a/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
>>> +++ b/Documentation/devicetree/bindings/mmc/brcm,sdhci-iproc.txt
>>> @@ -7,6 +7,7 @@ Required properties:
>>> - compatible : Should be one of the following
>>> "brcm,bcm2835-sdhci"
>>> "brcm,sdhci-iproc-cygnus"
>>> + "brcm,sdhci-iproc"
>>
>> Seems kind of generic. SoC specific compatible strings please.
>>
> The compatibility string is generic on purpose as it is not intended to
> be SoC specific but work on all new iproc SoCs that have the proper
> fixes in place for this block (unlike bcm2835 and cygnus class devices
> which can only do 32-bit accesses). I could call it brcm,sdhci-iproc-v2
> if that is better or leave it as is. Please let me know your preferences.
>
I just sent out v2 of the patch with additional details in the device
tree bindings document. Please let me know if this covers your
concerns. The SDHCI controller binding is not SoC specific and is used
in multiple SoCs going forward with the same binding.
> Regards,
> Scott
--
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 0/2] mmc: dw_mmc: fix the wrong operation for reset controller
From: John Stultz @ 2016-11-01 21:56 UTC (permalink / raw)
To: Jaehoon Chung
Cc: linux-mmc,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Ulf Hansson, Rob Herring, Mark Rutland, lkml, Guodong Xu, Leo Yan,
Vincent Guittot
In-Reply-To: <20161031024942.4415-1-jh80.chung@samsung.com>
On Sun, Oct 30, 2016 at 7:49 PM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> This patch adds the "reset-names" as reset controller for dwmmc controller.
> Linaro guys had reported the issue about booting stuck.
>
> Some SoCs are enabled the CONFIG_RESET_CONTROLLER.
> then dwmmc controller are waiting for getting reset controller.
> But if doesn't define "resets" property in device-tree, it should be stuck.
>
> If use the reset-names as reset controller for dwmmc controller,
> it's more stable than now.
> This commit is related with the below commit.
>
> Fixes: d6786fefe816 ("mmc: dw_mmc: add reset support to dwmmc host controller")
So I wasn't hit by the issue, as I don't tend to boot from SD on my
hikey, but I did run these patches and I'm not seeing any issues with
my Android environment.
So for what its worth:
Acked-by: John Stultz <john.stultz@linaro.org>
thanks
-john
^ permalink raw reply
* [PATCH 1/8] mmc: core: remove BUG_ONs from sdio
From: Shawn Lin @ 2016-11-02 7:24 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Shawn Lin
BUG_ONs doesn't help anything except for stop the system from
running. If it occurs, it implies we should deploy proper error
handling for that. So this patch is gonna discard these meaningless
BUG_ONs and deploy error handling if needed.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
drivers/mmc/core/sdio.c | 17 ++---------------
drivers/mmc/core/sdio_cis.c | 3 ++-
drivers/mmc/core/sdio_irq.c | 12 +++++++-----
3 files changed, 11 insertions(+), 21 deletions(-)
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index bd44ba8..ecbc529 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -63,7 +63,8 @@ static int sdio_init_func(struct mmc_card *card, unsigned int fn)
int ret;
struct sdio_func *func;
- BUG_ON(fn > SDIO_MAX_FUNCS);
+ if (WARN_ON(fn > SDIO_MAX_FUNCS))
+ return -EINVAL;
func = sdio_alloc_func(card);
if (IS_ERR(func))
@@ -555,7 +556,6 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
u32 rocr = 0;
u32 ocr_card = ocr;
- BUG_ON(!host);
WARN_ON(!host->claimed);
/* to query card if 1.8V signalling is supported */
@@ -791,9 +791,6 @@ static void mmc_sdio_remove(struct mmc_host *host)
{
int i;
- BUG_ON(!host);
- BUG_ON(!host->card);
-
for (i = 0;i < host->card->sdio_funcs;i++) {
if (host->card->sdio_func[i]) {
sdio_remove_func(host->card->sdio_func[i]);
@@ -820,9 +817,6 @@ static void mmc_sdio_detect(struct mmc_host *host)
{
int err;
- BUG_ON(!host);
- BUG_ON(!host->card);
-
/* Make sure card is powered before detecting it */
if (host->caps & MMC_CAP_POWER_OFF_CARD) {
err = pm_runtime_get_sync(&host->card->dev);
@@ -916,9 +910,6 @@ static int mmc_sdio_resume(struct mmc_host *host)
{
int err = 0;
- BUG_ON(!host);
- BUG_ON(!host->card);
-
/* Basic card reinitialization. */
mmc_claim_host(host);
@@ -970,9 +961,6 @@ static int mmc_sdio_power_restore(struct mmc_host *host)
{
int ret;
- BUG_ON(!host);
- BUG_ON(!host->card);
-
mmc_claim_host(host);
/*
@@ -1063,7 +1051,6 @@ int mmc_attach_sdio(struct mmc_host *host)
u32 ocr, rocr;
struct mmc_card *card;
- BUG_ON(!host);
WARN_ON(!host->claimed);
err = mmc_send_io_op_cond(host, 0, &ocr);
diff --git a/drivers/mmc/core/sdio_cis.c b/drivers/mmc/core/sdio_cis.c
index dcb3dee..f8c3728 100644
--- a/drivers/mmc/core/sdio_cis.c
+++ b/drivers/mmc/core/sdio_cis.c
@@ -262,7 +262,8 @@ static int sdio_read_cis(struct mmc_card *card, struct sdio_func *func)
else
prev = &card->tuples;
- BUG_ON(*prev);
+ if (*prev)
+ return -EINVAL;
do {
unsigned char tpl_code, tpl_link;
diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
index 91bbbfb..f1faf9a 100644
--- a/drivers/mmc/core/sdio_irq.c
+++ b/drivers/mmc/core/sdio_irq.c
@@ -214,7 +214,9 @@ static int sdio_card_irq_put(struct mmc_card *card)
struct mmc_host *host = card->host;
WARN_ON(!host->claimed);
- BUG_ON(host->sdio_irqs < 1);
+
+ if (host->sdio_irqs < 1)
+ return -EINVAL;
if (!--host->sdio_irqs) {
if (!(host->caps2 & MMC_CAP2_SDIO_IRQ_NOTHREAD)) {
@@ -261,8 +263,8 @@ int sdio_claim_irq(struct sdio_func *func, sdio_irq_handler_t *handler)
int ret;
unsigned char reg;
- BUG_ON(!func);
- BUG_ON(!func->card);
+ if (!func)
+ return -EINVAL;
pr_debug("SDIO: Enabling IRQ for %s...\n", sdio_func_id(func));
@@ -304,8 +306,8 @@ int sdio_release_irq(struct sdio_func *func)
int ret;
unsigned char reg;
- BUG_ON(!func);
- BUG_ON(!func->card);
+ if (!func)
+ return -EINVAL;
pr_debug("SDIO: Disabling IRQ for %s...\n", sdio_func_id(func));
--
2.3.7
^ permalink raw reply related
* [PATCH 2/8] mmc: debugfs: remove BUG_ON from mmc_ext_csd_open
From: Shawn Lin @ 2016-11-02 7:24 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Shawn Lin
In-Reply-To: <1478071440-12689-1-git-send-email-shawn.lin@rock-chips.com>
Return error value for file_operations callback instead
of triggering BUG_ON which is meaningless. Personally I
don't believe n != EXT_CSD_STR_LEN could happen. Anyway,
propagate the error to the caller.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
drivers/mmc/core/debugfs.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
index c8451ce..30623b8 100644
--- a/drivers/mmc/core/debugfs.c
+++ b/drivers/mmc/core/debugfs.c
@@ -321,7 +321,11 @@ static int mmc_ext_csd_open(struct inode *inode, struct file *filp)
for (i = 0; i < 512; i++)
n += sprintf(buf + n, "%02x", ext_csd[i]);
n += sprintf(buf + n, "\n");
- BUG_ON(n != EXT_CSD_STR_LEN);
+
+ if (n != EXT_CSD_STR_LEN) {
+ err = -EINVAL;
+ goto out_free;
+ }
filp->private_data = buf;
kfree(ext_csd);
--
2.3.7
^ permalink raw reply related
* [PATCH 3/8] mmc: core: remove BUG_ONs from mmc
From: Shawn Lin @ 2016-11-02 7:25 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Shawn Lin
In-Reply-To: <1478071440-12689-1-git-send-email-shawn.lin@rock-chips.com>
BUG_ONs doesn't help anything except for stop the system from
running. If it occurs, it implies we should deploy proper error
handling for that. So this patch is gonna discard these meaningless
BUG_ONs and deploy error handling if needed.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
drivers/mmc/core/mmc.c | 14 --------------
drivers/mmc/core/mmc_ops.c | 17 -----------------
2 files changed, 31 deletions(-)
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index e811bd9..4763a35 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1464,7 +1464,6 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
u32 cid[4];
u32 rocr;
- BUG_ON(!host);
WARN_ON(!host->claimed);
/* Set correct bus mode for MMC before attempting init */
@@ -1854,9 +1853,6 @@ static int mmc_poweroff_notify(struct mmc_card *card, unsigned int notify_type)
*/
static void mmc_remove(struct mmc_host *host)
{
- BUG_ON(!host);
- BUG_ON(!host->card);
-
mmc_remove_card(host->card);
host->card = NULL;
}
@@ -1876,9 +1872,6 @@ static void mmc_detect(struct mmc_host *host)
{
int err;
- BUG_ON(!host);
- BUG_ON(!host->card);
-
mmc_get_card(host->card);
/*
@@ -1904,9 +1897,6 @@ static int _mmc_suspend(struct mmc_host *host, bool is_suspend)
unsigned int notify_type = is_suspend ? EXT_CSD_POWER_OFF_SHORT :
EXT_CSD_POWER_OFF_LONG;
- BUG_ON(!host);
- BUG_ON(!host->card);
-
mmc_claim_host(host);
if (mmc_card_suspended(host->card))
@@ -1963,9 +1953,6 @@ static int _mmc_resume(struct mmc_host *host)
{
int err = 0;
- BUG_ON(!host);
- BUG_ON(!host->card);
-
mmc_claim_host(host);
if (!mmc_card_suspended(host->card))
@@ -2098,7 +2085,6 @@ int mmc_attach_mmc(struct mmc_host *host)
int err;
u32 ocr, rocr;
- BUG_ON(!host);
WARN_ON(!host->claimed);
/* Set correct bus mode for MMC before attempting attach */
diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 481bbdb..2bc4780 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -60,9 +60,6 @@ static inline int __mmc_send_status(struct mmc_card *card, u32 *status,
int err;
struct mmc_command cmd = {0};
- BUG_ON(!card);
- BUG_ON(!card->host);
-
cmd.opcode = MMC_SEND_STATUS;
if (!mmc_host_is_spi(card->host))
cmd.arg = card->rca << 16;
@@ -92,8 +89,6 @@ static int _mmc_select_card(struct mmc_host *host, struct mmc_card *card)
{
struct mmc_command cmd = {0};
- BUG_ON(!host);
-
cmd.opcode = MMC_SELECT_CARD;
if (card) {
@@ -109,7 +104,6 @@ static int _mmc_select_card(struct mmc_host *host, struct mmc_card *card)
int mmc_select_card(struct mmc_card *card)
{
- BUG_ON(!card);
return _mmc_select_card(card->host, card);
}
@@ -181,8 +175,6 @@ int mmc_send_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr)
struct mmc_command cmd = {0};
int i, err = 0;
- BUG_ON(!host);
-
cmd.opcode = MMC_SEND_OP_COND;
cmd.arg = mmc_host_is_spi(host) ? 0 : ocr;
cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R3 | MMC_CMD_BCR;
@@ -221,9 +213,6 @@ int mmc_all_send_cid(struct mmc_host *host, u32 *cid)
int err;
struct mmc_command cmd = {0};
- BUG_ON(!host);
- BUG_ON(!cid);
-
cmd.opcode = MMC_ALL_SEND_CID;
cmd.arg = 0;
cmd.flags = MMC_RSP_R2 | MMC_CMD_BCR;
@@ -241,9 +230,6 @@ int mmc_set_relative_addr(struct mmc_card *card)
{
struct mmc_command cmd = {0};
- BUG_ON(!card);
- BUG_ON(!card->host);
-
cmd.opcode = MMC_SET_RELATIVE_ADDR;
cmd.arg = card->rca << 16;
cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
@@ -257,9 +243,6 @@ mmc_send_cxd_native(struct mmc_host *host, u32 arg, u32 *cxd, int opcode)
int err;
struct mmc_command cmd = {0};
- BUG_ON(!host);
- BUG_ON(!cxd);
-
cmd.opcode = opcode;
cmd.arg = arg;
cmd.flags = MMC_RSP_R2 | MMC_CMD_AC;
--
2.3.7
^ permalink raw reply related
* [PATCH 4/8] mmc: core: remove BUG_ONs from sd
From: Shawn Lin @ 2016-11-02 7:25 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Shawn Lin
In-Reply-To: <1478071440-12689-1-git-send-email-shawn.lin@rock-chips.com>
BUG_ONs doesn't help anything except for stop the system from
running. If it occurs, it implies we should deploy proper error
handling for that. So this patch is gonna discard these meaningless
BUG_ONs and deploy error handling if needed.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
drivers/mmc/core/sd.c | 14 --------------
drivers/mmc/core/sd_ops.c | 27 ++++-----------------------
2 files changed, 4 insertions(+), 37 deletions(-)
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 73c762a..deb90c2 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -927,7 +927,6 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
u32 cid[4];
u32 rocr = 0;
- BUG_ON(!host);
WARN_ON(!host->claimed);
err = mmc_sd_get_cid(host, ocr, cid, &rocr);
@@ -1043,9 +1042,6 @@ static int mmc_sd_init_card(struct mmc_host *host, u32 ocr,
*/
static void mmc_sd_remove(struct mmc_host *host)
{
- BUG_ON(!host);
- BUG_ON(!host->card);
-
mmc_remove_card(host->card);
host->card = NULL;
}
@@ -1065,9 +1061,6 @@ static void mmc_sd_detect(struct mmc_host *host)
{
int err;
- BUG_ON(!host);
- BUG_ON(!host->card);
-
mmc_get_card(host->card);
/*
@@ -1091,9 +1084,6 @@ static int _mmc_sd_suspend(struct mmc_host *host)
{
int err = 0;
- BUG_ON(!host);
- BUG_ON(!host->card);
-
mmc_claim_host(host);
if (mmc_card_suspended(host->card))
@@ -1136,9 +1126,6 @@ static int _mmc_sd_resume(struct mmc_host *host)
{
int err = 0;
- BUG_ON(!host);
- BUG_ON(!host->card);
-
mmc_claim_host(host);
if (!mmc_card_suspended(host->card))
@@ -1221,7 +1208,6 @@ int mmc_attach_sd(struct mmc_host *host)
int err;
u32 ocr, rocr;
- BUG_ON(!host);
WARN_ON(!host->claimed);
err = mmc_send_app_op_cond(host, 0, &ocr);
diff --git a/drivers/mmc/core/sd_ops.c b/drivers/mmc/core/sd_ops.c
index 16b774c..de125a4 100644
--- a/drivers/mmc/core/sd_ops.c
+++ b/drivers/mmc/core/sd_ops.c
@@ -27,8 +27,8 @@ int mmc_app_cmd(struct mmc_host *host, struct mmc_card *card)
int err;
struct mmc_command cmd = {0};
- BUG_ON(!host);
- BUG_ON(card && (card->host != host));
+ if (WARN_ON(card && card->host != host))
+ return -EINVAL;
cmd.opcode = MMC_APP_CMD;
@@ -72,8 +72,8 @@ int mmc_wait_for_app_cmd(struct mmc_host *host, struct mmc_card *card,
int i, err;
- BUG_ON(!cmd);
- BUG_ON(retries < 0);
+ if (retries < 0)
+ retries = MMC_CMD_RETRIES;
err = -EIO;
@@ -122,9 +122,6 @@ int mmc_app_set_bus_width(struct mmc_card *card, int width)
{
struct mmc_command cmd = {0};
- BUG_ON(!card);
- BUG_ON(!card->host);
-
cmd.opcode = SD_APP_SET_BUS_WIDTH;
cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
@@ -147,8 +144,6 @@ int mmc_send_app_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr)
struct mmc_command cmd = {0};
int i, err = 0;
- BUG_ON(!host);
-
cmd.opcode = SD_APP_OP_COND;
if (mmc_host_is_spi(host))
cmd.arg = ocr & (1 << 30); /* SPI only defines one bit */
@@ -224,9 +219,6 @@ int mmc_send_relative_addr(struct mmc_host *host, unsigned int *rca)
int err;
struct mmc_command cmd = {0};
- BUG_ON(!host);
- BUG_ON(!rca);
-
cmd.opcode = SD_SEND_RELATIVE_ADDR;
cmd.arg = 0;
cmd.flags = MMC_RSP_R6 | MMC_CMD_BCR;
@@ -249,10 +241,6 @@ int mmc_app_send_scr(struct mmc_card *card, u32 *scr)
struct scatterlist sg;
void *data_buf;
- BUG_ON(!card);
- BUG_ON(!card->host);
- BUG_ON(!scr);
-
/* NOTE: caller guarantees scr is heap-allocated */
err = mmc_app_cmd(card->host, card);
@@ -307,9 +295,6 @@ int mmc_sd_switch(struct mmc_card *card, int mode, int group,
struct mmc_data data = {0};
struct scatterlist sg;
- BUG_ON(!card);
- BUG_ON(!card->host);
-
/* NOTE: caller guarantees resp is heap-allocated */
mode = !!mode;
@@ -352,10 +337,6 @@ int mmc_app_sd_status(struct mmc_card *card, void *ssr)
struct mmc_data data = {0};
struct scatterlist sg;
- BUG_ON(!card);
- BUG_ON(!card->host);
- BUG_ON(!ssr);
-
/* NOTE: caller guarantees ssr is heap-allocated */
err = mmc_app_cmd(card->host, card);
--
2.3.7
^ permalink raw reply related
* [PATCH 5/8] mmc: core: remove BUG_ONs from core.c
From: Shawn Lin @ 2016-11-02 7:26 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Shawn Lin
In-Reply-To: <1478071440-12689-1-git-send-email-shawn.lin@rock-chips.com>
BUG_ONs doesn't help anything except for stop the system from
running. If it occurs, it implies we should deploy proper error
handling for that. So this patch is gonna discard these meaningless
BUG_ONs and deploy error handling if needed.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
drivers/mmc/core/core.c | 34 +++++++++-------------------------
1 file changed, 9 insertions(+), 25 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 2553d90..7fc4814 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -306,16 +306,16 @@ static int mmc_start_request(struct mmc_host *host, struct mmc_request *mrq)
mrq->sbc->mrq = mrq;
}
if (mrq->data) {
- BUG_ON(mrq->data->blksz > host->max_blk_size);
- BUG_ON(mrq->data->blocks > host->max_blk_count);
- BUG_ON(mrq->data->blocks * mrq->data->blksz >
- host->max_req_size);
-
+ if (mrq->data->blksz > host->max_blk_size ||
+ mrq->data->blocks > host->max_blk_count ||
+ mrq->data->blocks * mrq->data->blksz > host->max_req_size)
+ return -EINVAL;
#ifdef CONFIG_MMC_DEBUG
sz = 0;
for_each_sg(mrq->data->sg, sg, mrq->data->sg_len, i)
sz += sg->length;
- BUG_ON(sz != mrq->data->blocks * mrq->data->blksz);
+ if (sz != mrq->data->blocks * mrq->data->blksz)
+ return -EINVAL;
#endif
mrq->cmd->data = mrq->data;
@@ -349,8 +349,6 @@ void mmc_start_bkops(struct mmc_card *card, bool from_exception)
int timeout;
bool use_busy_signal;
- BUG_ON(!card);
-
if (!card->ext_csd.man_bkops_en || mmc_card_doing_bkops(card))
return;
@@ -754,8 +752,6 @@ int mmc_interrupt_hpi(struct mmc_card *card)
u32 status;
unsigned long prg_wait;
- BUG_ON(!card);
-
if (!card->ext_csd.hpi_en) {
pr_info("%s: HPI enable bit unset\n", mmc_hostname(card->host));
return 1;
@@ -850,7 +846,6 @@ int mmc_stop_bkops(struct mmc_card *card)
{
int err = 0;
- BUG_ON(!card);
err = mmc_interrupt_hpi(card);
/*
@@ -1666,8 +1661,6 @@ int mmc_set_signal_voltage(struct mmc_host *host, int signal_voltage, u32 ocr)
int err = 0;
u32 clock;
- BUG_ON(!host);
-
/*
* Send CMD11 only if the request is to switch the card to
* 1.8V signalling.
@@ -1884,9 +1877,7 @@ void mmc_power_cycle(struct mmc_host *host, u32 ocr)
*/
static void __mmc_release_bus(struct mmc_host *host)
{
- BUG_ON(!host);
- BUG_ON(host->bus_refs);
- BUG_ON(!host->bus_dead);
+ WARN_ON(!host->bus_dead);
host->bus_ops = NULL;
}
@@ -1926,15 +1917,12 @@ void mmc_attach_bus(struct mmc_host *host, const struct mmc_bus_ops *ops)
{
unsigned long flags;
- BUG_ON(!host);
- BUG_ON(!ops);
-
WARN_ON(!host->claimed);
spin_lock_irqsave(&host->lock, flags);
- BUG_ON(host->bus_ops);
- BUG_ON(host->bus_refs);
+ WARN_ON(host->bus_ops);
+ WARN_ON(host->bus_refs);
host->bus_ops = ops;
host->bus_refs = 1;
@@ -1950,8 +1938,6 @@ void mmc_detach_bus(struct mmc_host *host)
{
unsigned long flags;
- BUG_ON(!host);
-
WARN_ON(!host->claimed);
WARN_ON(!host->bus_ops);
@@ -2865,8 +2851,6 @@ void mmc_stop_host(struct mmc_host *host)
}
mmc_bus_put(host);
- BUG_ON(host->card);
-
mmc_claim_host(host);
mmc_power_off(host);
mmc_release_host(host);
--
2.3.7
^ permalink raw reply related
* [PATCH 6/8] mmc: sdio_uart: remove meaningless BUG_ON
From: Shawn Lin @ 2016-11-02 7:26 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Shawn Lin
In-Reply-To: <1478071440-12689-1-git-send-email-shawn.lin@rock-chips.com>
The code seems quite simple to maintain the sdio_uart_table,
and the insert/remove port from the table are symmetric. If
the BUG_ON occurs, which means serial_core modify the index
or mess up the port sequence anyway.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
drivers/mmc/card/sdio_uart.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mmc/card/sdio_uart.c b/drivers/mmc/card/sdio_uart.c
index 5af6fb9..491c187 100644
--- a/drivers/mmc/card/sdio_uart.c
+++ b/drivers/mmc/card/sdio_uart.c
@@ -135,8 +135,6 @@ static void sdio_uart_port_remove(struct sdio_uart_port *port)
{
struct sdio_func *func;
- BUG_ON(sdio_uart_table[port->index] != port);
-
spin_lock(&sdio_uart_table_lock);
sdio_uart_table[port->index] = NULL;
spin_unlock(&sdio_uart_table_lock);
--
2.3.7
^ permalink raw reply related
* [PATCH 7/8] mmc: queue: remove BUG_ON for bounce_sg
From: Shawn Lin @ 2016-11-02 7:26 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Shawn Lin
In-Reply-To: <1478071440-12689-1-git-send-email-shawn.lin@rock-chips.com>
bounce_sg for mqrq_cur and mqrq_pre are proper
allocated when initializing the queue and will not
be freed before explicitly cleaning the queue. So from
the code itself it should be quite confident to remove
this check. If that BUG_ON take effects, it is mostly
likely the memory is randomly oopsing.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
drivers/mmc/card/queue.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
index 8037f73..6c8978a 100644
--- a/drivers/mmc/card/queue.c
+++ b/drivers/mmc/card/queue.c
@@ -505,8 +505,6 @@ unsigned int mmc_queue_map_sg(struct mmc_queue *mq, struct mmc_queue_req *mqrq)
return blk_rq_map_sg(mq->queue, mqrq->req, mqrq->sg);
}
- BUG_ON(!mqrq->bounce_sg);
-
if (mmc_packed_cmd(cmd_type))
sg_len = mmc_queue_packed_map_sg(mq, mqrq->packed,
mqrq->bounce_sg, cmd_type);
--
2.3.7
^ permalink raw reply related
* [PATCH 8/8] mmc: mmc_test: remove BUG_ONs and deploy error handling
From: Shawn Lin @ 2016-11-02 7:26 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Shawn Lin
In-Reply-To: <1478071440-12689-1-git-send-email-shawn.lin@rock-chips.com>
It is unnecessary to panic the kernel when testing mmc. Instead,
cast a warning for folkz to debug and return the error code to
the caller to indicate the failure of this test should be enough.
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---
drivers/mmc/card/mmc_test.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/mmc/card/mmc_test.c b/drivers/mmc/card/mmc_test.c
index 5a8dc5a..db1a7ac 100644
--- a/drivers/mmc/card/mmc_test.c
+++ b/drivers/mmc/card/mmc_test.c
@@ -214,7 +214,8 @@ static void mmc_test_prepare_mrq(struct mmc_test_card *test,
struct mmc_request *mrq, struct scatterlist *sg, unsigned sg_len,
unsigned dev_addr, unsigned blocks, unsigned blksz, int write)
{
- BUG_ON(!mrq || !mrq->cmd || !mrq->data || !mrq->stop);
+ if (WARN_ON(!mrq || !mrq->cmd || !mrq->data || !mrq->stop))
+ return;
if (blocks > 1) {
mrq->cmd->opcode = write ?
@@ -694,7 +695,8 @@ static int mmc_test_cleanup(struct mmc_test_card *test)
static void mmc_test_prepare_broken_mrq(struct mmc_test_card *test,
struct mmc_request *mrq, int write)
{
- BUG_ON(!mrq || !mrq->cmd || !mrq->data);
+ if (WARN_ON(!mrq || !mrq->cmd || !mrq->data))
+ return;
if (mrq->data->blocks > 1) {
mrq->cmd->opcode = write ?
@@ -714,7 +716,8 @@ static int mmc_test_check_result(struct mmc_test_card *test,
{
int ret;
- BUG_ON(!mrq || !mrq->cmd || !mrq->data);
+ if (WARN_ON(!mrq || !mrq->cmd || !mrq->data))
+ return -EINVAL;
ret = 0;
@@ -755,7 +758,8 @@ static int mmc_test_check_broken_result(struct mmc_test_card *test,
{
int ret;
- BUG_ON(!mrq || !mrq->cmd || !mrq->data);
+ if (WARN_ON(!mrq || !mrq->cmd || !mrq->data))
+ return -EINVAL;
ret = 0;
--
2.3.7
^ permalink raw reply related
* Re: Regression after "do not use CMD13 to get status after speed mode switch"
From: Adrian Hunter @ 2016-11-02 8:19 UTC (permalink / raw)
To: Chaotian Jing
Cc: Ulf Hansson, Linus Walleij, linux-mmc@vger.kernel.org,
linux-arm-msm@vger.kernel.org, Bjorn Andersson, Stephen Boyd,
Andy Gross
In-Reply-To: <1477964625.4162.3.camel@mhfsdcap03>
On 01/11/16 03:43, Chaotian Jing wrote:
> On Mon, 2016-10-31 at 15:09 +0200, Adrian Hunter wrote:
>> On 27/10/16 13:04, Ulf Hansson wrote:
>>> On 20 October 2016 at 09:06, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>> On 20 October 2016 at 04:22, Chaotian Jing <chaotian.jing@mediatek.com> wrote:
>>>>> On Wed, 2016-10-19 at 18:41 +0200, Ulf Hansson wrote:
>>>>>> Adrian, Linus,
>>>>>>
>>>>>> Thanks for looking into this and reporting!
>>>>>>
>>>>>> On 18 October 2016 at 15:23, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>>> On 18/10/16 11:36, Linus Walleij wrote:
>>>>>>>> On Mon, Oct 17, 2016 at 4:32 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>>>>>
>>>>>>>>> Before this patch the eMMC is detected and all partitions enumerated
>>>>>>>>> immediately, but after the patch it doesn't come up at all, except
>>>>>>>>> sometimes, when it appears minutes (!) after boot, all of a sudden.
>>>>>>>>
>>>>>>>> FYI this is what it looks like when it eventually happens:
>>>>>>>> root@msm8660:/ [ 627.710175] mmc0: new high speed MMC card at address 0001
>>>>>>>> [ 627.711641] mmcblk0: mmc0:0001 SEM04G 3.69 GiB
>>>>>>>> [ 627.715485] mmcblk0boot0: mmc0:0001 SEM04G partition 1 1.00 MiB
>>>>>>>> [ 627.736654] mmcblk0boot1: mmc0:0001 SEM04G partition 2 1.00 MiB
>>>>>>>> [ 627.747397] mmcblk0rpmb: mmc0:0001 SEM04G partition 3 128 KiB
>>>>>>>> [ 627.756326] mmcblk0: p1 p2 p3 p4 < p5 p6 p7 p8 p9 p10 p11 p12 p13
>>>>>>>> p14 p15 p16 p17 p18 p19 p20 p21 >
>>>>>>>>
>>>>>>>> So after 627 seconds, a bit hard for users to wait this long for their
>>>>>>>> root filesystem.
>>>>>>>
>>>>>>> If the driver does not support busy detection and the eMMC card provides
>>>>>>> zero as the cmd6 generic timeout (which it may especially as cmd6 generic
>>>>>>> timeout wasn't added until eMMCv4.5), then __mmc_switch() defaults to
>>>>>>> waiting 10 minutes i.e.
>>>>>>>
>>>>>>> #define MMC_OPS_TIMEOUT_MS (10 * 60 * 1000) /* 10 minute timeout */
>>>>>>
>>>>>> Urgh! Yes, I have verified that this is exactly what happens.
>>>>>>
>>>>>>>
>>>>>>> So removal of CMD13 polling for HS mode (as per commit
>>>>>>> 08573eaf1a70104f83fdbee9b84e5be03480e9ed) is going to be a problem for some
>>>>>>> combinations of eMMC cards and host drivers.
>>>>>>
>>>>>> I was looking in the __mmc_switch() function, it's just a pain to walk
>>>>>> trough it :-) So first out I decided to clean it up and factor out the
>>>>>> polling parts. I will post the patches first out tomorrow morning,
>>>>>> running some final test right now.
>>>>>>
>>>>>> Although, that of course doesn't solve our problem. As I see it we
>>>>>> only have a few options here.
>>>>>>
>>>>>> 1) In case when cmd6 generic timeout isn't available, let's assign
>>>>>> another empirically selected value.
>>>>>> 2) Use a specific timeout when switching to HS mode.
>>>>>> 3) Even if we deploy 1 (and 2), perhaps we still should allow polling
>>>>>> with CMD13 for switching to HS mode - unless it causes issues for some
>>>>>> cards/drivers combination?
>>>>>>
>>>>>> BTW, I already tried 2) and it indeed solves the problem, although
>>>>>> depending on the selected timeout, it might delay the card detection
>>>>>> to process.
>>>>>>
>>>>>> Thoughts?
>>>>>
>>>>> I just have a try of switching to HS mode with Hynix EMMC, the first
>>>>> CMD13 gets response of 0x900, but the EMMC is still pull-low DAT0. so
>>>>> that CMD13 cannot indicate current card status in this case.
>>>>
>>>> Thanks for sharing that. Okay, so clearly we have some cards that
>>>> don't supports polling with CMD13 when switching to HS mode.
>>>> One could of course add quirks for these kind of cards and do a fixed
>>>> delay for them, but then to find out which these cards are is going to
>>>> be hard.
>>>>
>>>> It seems like we are left with using a fixed delay. Any ideas of what
>>>> such delay should be? And should we have one specific for switch to
>>>> the various speed modes and a different one that overrides the CMD6
>>>> generic timout, when it doesn't exist?
>>>>
>>>
>>> Replying to my own earlier response, as I believe the problem could
>>> also be related to another old commit, see below.
>>>
>>> commit a27fbf2f067b0cd6f172c8b696b9a44c58bfaa7a
>>> Author: Seungwon Jeon <tgih.jun@samsung.com>
>>> Date: Wed Sep 4 21:21:05 2013 +0900
>>>
>>> mmc: add ignorance case for CMD13 CRC error
>>>
>>> While speed mode is changed, CMD13 cannot be guaranteed.
>>> According to the spec., it is not recommended to use CMD13
>>> to check the busy completion of the timing change.
>>> If CMD13 is used in this case, CRC error must be ignored.
>>>
>>> Signed-off-by: Seungwon Jeon <tgih.jun@samsung.com>
>>> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> Signed-off-by: Chris Ball <cjb@laptop.org>
>>>
>>>
>>> The intent with this commit was not really correct. We don't want to
>>> ignore CRC errors, but instead we should *re-try* sending CMD13 once
>>> we get a CRC error.
>>>
>>> Unfortunate since this commit, instead we tell the host driver to
>>> *ignore* CRC errors and instead reads the status and returns 0
>>> (indicating success). In the mmc core, in __mmc_switch(), it will thus
>>> parse the status reply, even for a reply that might have been received
>>> with a CRC error. Not good!
>>
>> I agree: ignoring CRC errors and then expecting the status in the response
>> to be correct doesn't make sense.
>>
>> However, it raises the question of what to do if there are always CRC errors
>> e.g. if it only works without CRC errors once the mode and frequency are
>> changed in the host controller.
>>
>>> I am wondering whether this actually is the main problem to why we
>>> think polling isn't working for some cases. And perhaps that was the
>>> original problem Chaotian was trying to solve?
>>>
>>> Thoughts?
>>
>> Does Chaotian have a real problem since his driver has busy detection anyway?
>
> In fact, I have not encounter CRC errors of CMD13, I have tried several
> eMMC cards, after mode switch, CMD13 will only gets 0x800 response and
> we don't know if card is busy by 0x800 response.
Does it change to 0x900 when it is not busy?
But anyway the question was: do you have busy detection in your driver?
^ permalink raw reply
* Re: [PATCH 2/2] mmc: sdhci: Use sdhci-caps-mask and sdhci-caps to change the caps read during __sdhci_read_caps
From: Adrian Hunter @ 2016-11-02 8:39 UTC (permalink / raw)
To: Zach Brown, ulf.hansson-QSEj5FYQhm4dnm+yROfE0A
Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
linux-mmc-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1477670171-30015-3-git-send-email-zach.brown-acOepvfBmUk@public.gmane.org>
On 28/10/16 18:56, Zach Brown wrote:
> The sdhci capabilities register can be incorrect. The sdhci-caps-mask
> and sdhci-caps dt properties specify which bits of the register are
> incorrect and what their values should be. This patch makes the sdhci
> driver use those properties to correct the caps during
> __sdhci_read_caps.
>
> During __sdhci_read_caps
> Use the sdhci-caps-mask property to turn off the incorrect bits of the
> sdhci register after reading them.
> Use the sdhci-caps to turn on bits after using sdhci-caps-mask to turn
> off the incorrect ones.
>
> Signed-off-by: Zach Brown <zach.brown-acOepvfBmUk@public.gmane.org>
Apart from minor style issue:
Acked-by: Adrian Hunter <adrian.hunter-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
> drivers/mmc/host/sdhci.c | 24 ++++++++++++++++++++++--
> 1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 1e25b01..d5feae4 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -22,6 +22,7 @@
> #include <linux/scatterlist.h>
> #include <linux/regulator/consumer.h>
> #include <linux/pm_runtime.h>
> +#include <linux/of.h>
>
> #include <linux/leds.h>
>
> @@ -2991,6 +2992,8 @@ static int sdhci_set_dma_mask(struct sdhci_host *host)
> void __sdhci_read_caps(struct sdhci_host *host, u16 *ver, u32 *caps, u32 *caps1)
> {
> u16 v;
> + u64 dt_caps_mask = 0;
> + u64 dt_caps = 0;
>
> if (host->read_caps)
> return;
> @@ -3005,18 +3008,35 @@ void __sdhci_read_caps(struct sdhci_host *host, u16 *ver, u32 *caps, u32 *caps1)
>
> sdhci_do_reset(host, SDHCI_RESET_ALL);
>
> + of_property_read_u64(mmc_dev(host->mmc)->of_node,
> + "sdhci-caps-mask", &dt_caps_mask);
> + of_property_read_u64(mmc_dev(host->mmc)->of_node,
> + "sdhci-caps", &dt_caps);
> +
> v = ver ? *ver : sdhci_readw(host, SDHCI_HOST_VERSION);
> host->version = (v & SDHCI_SPEC_VER_MASK) >> SDHCI_SPEC_VER_SHIFT;
>
> if (host->quirks & SDHCI_QUIRK_MISSING_CAPS)
> return;
>
> - host->caps = caps ? *caps : sdhci_readl(host, SDHCI_CAPABILITIES);
> + if (caps)
> + host->caps = *caps;
> + else {
All branches of 'if' should have {}.
i.e. refer results of ./scripts/checkpatch.pl --strict
> + host->caps = sdhci_readl(host, SDHCI_CAPABILITIES);
> + host->caps &= ~lower_32_bits(dt_caps_mask);
> + host->caps |= lower_32_bits(dt_caps);
> + }
>
> if (host->version < SDHCI_SPEC_300)
> return;
>
> - host->caps1 = caps1 ? *caps1 : sdhci_readl(host, SDHCI_CAPABILITIES_1);
> + if (caps1)
> + host->caps1 = *caps1;
> + else {
Ditto
> + host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);
> + host->caps1 &= ~upper_32_bits(dt_caps_mask);
> + host->caps1 |= upper_32_bits(dt_caps);
> + }
> }
> EXPORT_SYMBOL_GPL(__sdhci_read_caps);
>
>
--
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] sdhci-esdhc-imx: fix bus-width for 1-bit operation.
From: Dong Aisheng @ 2016-11-02 8:42 UTC (permalink / raw)
To: Leonardo G. Veiga
Cc: Ulf Hansson, linux-mmc@vger.kernel.org,
linux-kernel@vger.kernel.org, Stefan Agner, raul.munoz,
leonardo.veiga, Haibo Chen
In-Reply-To: <1478015905-31262-1-git-send-email-leogveiga@gmail.com>
Hi Leonardo,
On Tue, Nov 1, 2016 at 11:58 PM, Leonardo G. Veiga <leogveiga@gmail.com> wrote:
> From: Leonardo Graboski Veiga <leonardo.veiga@toradex.com>
>
> The 1-bit operation mode, enabled by seeting the 'bus-width' property of
> the device tree 'esdhc' node to <1>, not work while using SD card.
>
> The behavior is only noticed when only the data pin 0 is connected to the
> hardware. A series of kernel errors are printed to the console, all of them
> returning the following error message followed by some explanation:
> mmcblk0: error -84 transferring data
>
> If four data lines are connected, it ignores the device-tree
> property and works in 4-bit mode of operation without errors. The hardware
> used for testing does not support 8-bit mode.
>
> Check the 'bus-width' property and if set to <1>, enable the
> SDHCI_QUIRK_FORCE_1_BIT_DATA quirk.
>
> Signed-off-by: Leonardo Graboski Veiga <leonardo.veiga@toradex.com>
> ---
> drivers/mmc/host/sdhci-esdhc-imx.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index c9fbc4c3..88d7d22 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -1003,6 +1003,10 @@ sdhci_esdhc_imx_probe_dt(struct platform_device *pdev,
> host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
> }
>
> + if (!of_property_read_u32(np, "bus-width", &boarddata->max_bus_width)
> + && boarddata->max_bus_width == 1)
> + host->quirks |= SDHCI_QUIRK_FORCE_1_BIT_DATA;
> +
This looks like a common SDHCI driver issue that it assumes the default
bus-width as 4 bit if no SDHCI_QUIRK_FORCE_1_BIT_DATA specified.
if (!(host->quirks & SDHCI_QUIRK_FORCE_1_BIT_DATA))
mmc->caps |= MMC_CAP_4_BIT_DATA;
And I'm not sure Andrian or Ulf would like to see people keep using this quirk.
IMHO we probably could totally remove it since bus-width already tells
what the driver needs.
Andrian & Ulf,
Comments?
> /* call to generic mmc_of_parse to support additional capabilities */
> ret = mmc_of_parse(host->mmc);
> if (ret)
> --
> 2.7.4
>
Regards
Dong Aisheng
^ permalink raw reply
* Re: [PATCH V5 00/25] mmc: mmc: Add Software Command Queuing
From: Ritesh Harjani @ 2016-11-02 9:30 UTC (permalink / raw)
To: Adrian Hunter, Ulf Hansson
Cc: linux-mmc, Alex Lemberg, Mateusz Nowak, Yuliy Izrailov,
Jaehoon Chung, Dong Aisheng, Das Asutosh, Zhangfei Gao,
Dorfman Konstantin, David Griego, Sahitya Tummala,
Venu Byravarasu
In-Reply-To: <23591370-9dcc-49f8-fa80-ce2d60c7d082@intel.com>
Hi Adrian,
Reply got delayed due to festive season holidays here.
On 10/28/2016 6:18 PM, Adrian Hunter wrote:
> On 28/10/16 14:48, Ritesh Harjani wrote:
>> Hi Adrian,
>>
>>
>> Thanks for the re-base. I am trying to apply this patch series and validate.
>> I am seeing some tuning related errors, it could be due to my setup/device.
>> I will be working on this.
>>
>>
>> Meanwhile below perf readout was showing some discrepancy (could be that I
>> am missing something).
>> In your last test both sequential and random read gives almost same
>> throughput and even the percentage increase is similar(legacy v/s SW cmdq).
>> Could be due to benchmark itself ?
>
> Normally sequential reading would result in large transfers due to
> readahead. But the test is using Direct-I/O (-I option) to prevent VFS
> caching distorting the results. So the sequential read case ends up being
> more like a random read case anyway.
>
>>
>> Do you think we should get some other benchmarks tested as well?
>> (may be tio?)
>>
>>
>> Also could you please also add some analysis on where we should expect to
>> see the score improvement in SW CMDQ and where we may see some decrements
>> due to SW CMDQ?
>
> I would expect random reads to be improved, but there is the overhead of the
> extra cmdq commands, so a small decrease in performance would be expected
> otherwise.
>
>> You did mention in original cover-letter that we mostly should see
>> improvement in Random read scores, but below here we are seeing similar or
>> higher improvement in sequential reads(4 threads). Which is a bit surprising.
>
> As explained above: the test is called "sequential" but the limited record
> size combined with Direct I/O makes it more like random I/O especially when
> it is mixed from multiple threads.
In that case if we see sequential read v/s random read for single thread
of SWCMDQ, both show the same data. Both SR and RR should not be same
ideally no?
>
>>
>>
>>
>>
>> On 10/24/2016 2:07 PM, Adrian Hunter wrote:
>>> Hi
>>>
>>> Here is an updated version of the Software Command Queuing patches,
>>> re-based on next, with patches 1-5 dropped because they have been applied,
>>> and 2 fixes that have been rolled in (refer Changes in V5 below).
>>>
>>> Performance results:
>>>
>>> Results can vary from run to run, but here are some results showing 1, 2 or 4
>>> processes with 4k and 32k record sizes. They show up to 40% improvement in
>>> read performance when there are multiple processes.
>>>
>>> iozone -s 8192k -r 4k -i 0 -i 1 -i 2 -i 8 -I -t 1 -F /mnt/mmc/iozone1.tmp
>>>
>>> Children see throughput for 1 initial writers = 27909.87
>>> kB/sec 24204.14 kB/sec -13.28 %
>>> Children see throughput for 1 rewriters = 28839.28 kB/sec
>>> 25531.92 kB/sec -11.47 %
>>> Children see throughput for 1 readers = 25889.65
>>> kB/sec 24883.23 kB/sec -3.89 %
>>> Children see throughput for 1 re-readers = 25558.23 kB/sec
>>> 24679.89 kB/sec -3.44 %
>>> Children see throughput for 1 random readers = 25571.48
>>> kB/sec 24689.52 kB/sec -3.45 %
>>> Children see throughput for 1 mixed workload = 25758.59
>>> kB/sec 24487.52 kB/sec -4.93 %
>>> Children see throughput for 1 random writers = 24787.51
>>> kB/sec 19368.99 kB/sec -21.86 %
>>>
>>> iozone -s 8192k -r 32k -i 0 -i 1 -i 2 -i 8 -I -t 1 -F /mnt/mmc/iozone1.tmp
>>>
>>> Children see throughput for 1 initial writers = 91344.61
>>> kB/sec 102008.56 kB/sec 11.67 %
>>> Children see throughput for 1 rewriters = 87932.36 kB/sec
>>> 96630.44 kB/sec 9.89 %
>>> Children see throughput for 1 readers = 134879.82
>>> kB/sec 110292.79 kB/sec -18.23 %
>>> Children see throughput for 1 re-readers = 147632.13 kB/sec
>>> 109053.33 kB/sec -26.13 %
>>> Children see throughput for 1 random readers = 93547.37
>>> kB/sec 112225.50 kB/sec 19.97 %
>>> Children see throughput for 1 mixed workload = 93560.04
>>> kB/sec 110515.21 kB/sec 18.12 %
>>> Children see throughput for 1 random writers = 92841.84
>>> kB/sec 81153.81 kB/sec -12.59 %
>>>
>>> iozone -s 8192k -r 4k -i 0 -i 1 -i 2 -i 8 -I -t 2 -F /mnt/mmc/iozone1.tmp
>>> /mnt/mmc/iozone2.tmp
>>>
>>> Children see throughput for 2 initial writers = 31145.43
>>> kB/sec 33771.25 kB/sec 8.43 %
>>> Children see throughput for 2 rewriters = 30592.57 kB/sec
>>> 35916.46 kB/sec 17.40 %
>>> Children see throughput for 2 readers = 31669.83
>>> kB/sec 37460.13 kB/sec 18.28 %
>>> Children see throughput for 2 re-readers = 32079.94 kB/sec
>>> 37373.33 kB/sec 16.50 %
>>> Children see throughput for 2 random readers = 27731.19
>>> kB/sec 37601.65 kB/sec 35.59 %
>>> Children see throughput for 2 mixed workload = 13927.50
>>> kB/sec 14617.06 kB/sec 4.95 %
>>> Children see throughput for 2 random writers = 31250.00
>>> kB/sec 33106.72 kB/sec 5.94 %
>>>
>>> iozone -s 8192k -r 32k -i 0 -i 1 -i 2 -i 8 -I -t 2 -F /mnt/mmc/iozone1.tmp
>>> /mnt/mmc/iozone2.tmp
>>>
>>> Children see throughput for 2 initial writers = 123255.84
>>> kB/sec 131252.22 kB/sec 6.49 %
>>> Children see throughput for 2 rewriters = 115234.91 kB/sec
>>> 107225.74 kB/sec -6.95 %
>>> Children see throughput for 2 readers = 128921.86
>>> kB/sec 148562.71 kB/sec 15.23 %
>>> Children see throughput for 2 re-readers = 127815.24 kB/sec
>>> 149304.32 kB/sec 16.81 %
>>> Children see throughput for 2 random readers = 125600.46
>>> kB/sec 148406.56 kB/sec 18.16 %
>>> Children see throughput for 2 mixed workload = 44006.94
>>> kB/sec 50937.36 kB/sec 15.75 %
>>> Children see throughput for 2 random writers = 120623.95
>>> kB/sec 103969.05 kB/sec -13.81 %
>>>
>>> iozone -s 8192k -r 4k -i 0 -i 1 -i 2 -i 8 -I -t 4 -F /mnt/mmc/iozone1.tmp
>>> /mnt/mmc/iozone2.tmp /mnt/mmc/iozone3.tmp /mnt/mmc/iozone4.tmp
>>>
>>> Children see throughput for 4 initial writers = 24100.96
>>> kB/sec 33336.58 kB/sec 38.32 %
>>> Children see throughput for 4 rewriters = 31650.20 kB/sec
>>> 33091.53 kB/sec 4.55 %
>>> Children see throughput for 4 readers = 33276.92
>>> kB/sec 41799.89 kB/sec 25.61 %
>>> Children see throughput for 4 re-readers = 31786.96 kB/sec
>>> 41501.74 kB/sec 30.56 %
>>> Children see throughput for 4 random readers = 31991.65
>>> kB/sec 40973.93 kB/sec 28.08 %
>>> Children see throughput for 4 mixed workload = 15804.80
>>> kB/sec 13581.32 kB/sec -14.07 %
>>> Children see throughput for 4 random writers = 31231.42
>>> kB/sec 34537.03 kB/sec 10.58 %
>>>
>>> iozone -s 8192k -r 32k -i 0 -i 1 -i 2 -i 8 -I -t 4 -F /mnt/mmc/iozone1.tmp
>>> /mnt/mmc/iozone2.tmp /mnt/mmc/iozone3.tmp /mnt/mmc/iozone4.tmp
>>>
>>> Children see throughput for 4 initial writers = 116567.42
>>> kB/sec 119280.35 kB/sec 2.33 %
>>> Children see throughput for 4 rewriters = 115010.96 kB/sec
>>> 120864.34 kB/sec 5.09 %
>>> Children see throughput for 4 readers = 130700.29
>>> kB/sec 177834.21 kB/sec 36.06 %
>> Do you think sequential read will increase more that of random read. It
>> should mostly benefit in random reads right. Any idea why it's behaving
>> differently here?
>
> Explained above.
>
>>
>>
>>> Children see throughput for 4 re-readers = 125392.58 kB/sec
>>> 175975.28 kB/sec 40.34 %
>>> Children see throughput for 4 random readers = 132194.57
>>> kB/sec 176630.46 kB/sec 33.61 %
>>> Children see throughput for 4 mixed workload = 56464.98
>>> kB/sec 54140.61 kB/sec -4.12 %
>>> Children see throughput for 4 random writers = 109128.36
>>> kB/sec 85359.80 kB/sec -21.78 %
>> Similarly, we don't expect random write scores to decrease here. Do you know
>> why this could be the case here?
>
> There is a lot of variation from one run to another
These run-to-run variations are due to what, any idea?
21% variations is huge.
Actually can we try and get some data which shows the difference between
SeqR and RandR and also RandW should not decrease this much.
May be by changing iozone parameters?
How about below data ?
For Sequential
./iozone -s 64m -r 32m -i 0 -i 1 -i 2 -i 8 -I -t 4 -F
/data/mmc/iozone1.tmp /data/mmc/iozone2.tmp /data/mmc/iozone3.tmp
/data/mmc/iozone4.tmp
For random -
./iozone -s 64m -r 4k -i 0 -i 1 -i 2 -i 8 -I -t 4 -F
/data/mmc/iozone1.tmp /data/mmc/iozone2.tmp /data/mmc/iozone3.tmp
/data/mmc/iozone4.tmp
Regards
Ritesh
>
>>
>>
>>>
>>>
>>> The current block driver supports 2 requests on the go at a time. Patches
>>> 1 - 8 make preparations for an arbitrary sized queue. Patches 9 - 12
>>> introduce Command Queue definitions and helpers. Patches 13 - 19
>>> complete the job of making the block driver use a queue. Patches 20 - 23
>>> finally add Software Command Queuing, and 24 - 25 enable it for Intel eMMC
>>> controllers. Most of the Software Command Queuing functionality is added
>>> in patch 22.
>>>
>>> As noted below, the patches can also be found here:
>>>
>>> http://git.infradead.org/users/ahunter/linux-sdhci.git/shortlog/refs/heads/swcmdq
>>>
>>>
>>> Changes in V5:
>>>
>>> Patches 1-5 dropped because they have been applied.
>>>
>>> Re-based on next.
>>>
>>> Fixed use of blk_end_request_cur() when it should have been
>>> blk_end_request_all() to error out requests during error recovery.
>>>
>>> Fixed unpaired retune_hold / retune_release in the error recovery path.
>>>
>>> Changes in V4:
>>>
>>> Re-based on next + v4.8-rc2 + "block: Fix secure erase" patch
>>>
>>> Changes in V3:
>>>
>>> Patches 1-25 dropped because they have been applied.
>>>
>>> Re-based on next.
>>>
>>> mmc: queue: Allocate queue of size qdepth
>>> Free queue during cleanup
>>>
>>> mmc: mmc: Add Command Queue definitions
>>> Add cmdq_en to mmc-dev-attrs.txt documentation
>>>
>>> mmc: queue: Share mmc request array between partitions
>>> New patch
>>>
>>> Changes in V2:
>>>
>>> Added 5 patches already sent here:
>>>
>>> http://marc.info/?l=linux-mmc&m=146712062816835
>>>
>>> Added 3 more new patches:
>>>
>>> mmc: sdhci-pci: Do not runtime suspend at the end of sdhci_pci_probe()
>>> mmc: sdhci: Avoid STOP cmd triggering warning in sdhci_send_command()
>>> mmc: sdhci: sdhci_execute_tuning() must delete timer
>>>
>>> Carried forward the V2 fix to:
>>>
>>> mmc: mmc_test: Disable Command Queue while mmc_test is used
>>>
>>> Also reset the cmd circuit for data timeout if it is processing the data
>>> cmd, in patch:
>>>
>>> mmc: sdhci: Do not reset cmd or data circuits that are in use
>>>
>>> There wasn't much comment on the RFC so there have been few changes.
>>> Venu Byravarasu commented that it may be more efficient to use Software
>>> Command Queuing only when there is more than 1 request queued - it isn't
>>> obvious how well that would work in practice, but it could be added later
>>> if it could be shown to be beneficial.
>>>
>>> Original Cover Letter:
>>>
>>> Chuanxiao Dong sent some patches last year relating to eMMC 5.1 Software
>>> Command Queuing. He did not follow-up but I have contacted him and he says
>>> it is OK if I take over upstreaming the patches.
>>>
>>> eMMC Command Queuing is a feature added in version 5.1. The card maintains
>>> a queue of up to 32 data transfers. Commands CMD45/CMD45 are sent to queue
>>> up transfers in advance, and then one of the transfers is selected to
>>> "execute" by CMD46/CMD47 at which point data transfer actually begins.
>>>
>>> The advantage of command queuing is that the card can prepare for transfers
>>> in advance. That makes a big difference in the case of random reads because
>>> the card can start reading into its cache in advance.
>>>
>>> A v5.1 host controller can manage the command queue itself, but it is also
>>> possible for software to manage the queue using an non-v5.1 host controller
>>> - that is what Software Command Queuing is.
>>>
>>> Refer to the JEDEC (http://www.jedec.org/) eMMC v5.1 Specification for more
>>> information about Command Queuing.
>>>
>>> While these patches are heavily based on Dong's patches, there are some
>>> changes:
>>>
>>> SDHCI has been amended to support commands during transfer. That is a
>>> generic change added in patches 1 - 5. [Those patches have now been applied]
>>> In principle, that would also support SDIO's CMD52 during data transfer.
>>>
>>> The original approach added multiple commands into the same request for
>>> sending CMD44, CMD45 and CMD13. That is not strictly necessary and has
>>> been omitted for now.
>>>
>>> The original approach also called blk_end_request() from the mrq->done()
>>> function, which means the upper layers learnt of completed requests
>>> slightly earlier. That is not strictly related to Software Command Queuing
>>> and is something that could potentially be done for all data requests.
>>> That has been omitted for now.
>>>
>>> The current block driver supports 2 requests on the go at a time. Patches
>>> 1 - 8 make preparations for an arbitrary sized queue. Patches 9 - 12
>>> introduce Command Queue definitions and helpers. Patches 13 - 19
>>> complete the job of making the block driver use a queue. Patches 20 - 23
>>> finally add Software Command Queuing, and 24 - 25 enable it for Intel eMMC
>>> controllers. Most of the Software Command Queuing functionality is added
>>> in patch 22.
>>>
>>> The patches can also be found here:
>>>
>>> http://git.infradead.org/users/ahunter/linux-sdhci.git/shortlog/refs/heads/swcmdq
>>>
>>>
>>> The patches have only had basic testing so far. Ad-hoc testing shows a
>>> degradation in sequential read performance of about 10% but an increase in
>>> throughput for mixed workload of multiple processes of about 90%. The
>>> reduction in sequential performance is due to the need to read the Queue
>>> Status register between each transfer.
>>>
>>> These patches should not conflict with Hardware Command Queuing which
>>> handles the queue in a completely different way and thus does not need
>>> to share code with Software Command Queuing. The exceptions being the
>>> Command Queue definitions and queue allocation which should be able to be
>>> used.
>>>
>>>
>>> Adrian Hunter (25):
>>> mmc: queue: Fix queue thread wake-up
>>> mmc: queue: Factor out mmc_queue_alloc_bounce_bufs()
>>> mmc: queue: Factor out mmc_queue_alloc_bounce_sgs()
>>> mmc: queue: Factor out mmc_queue_alloc_sgs()
>>> mmc: queue: Factor out mmc_queue_reqs_free_bufs()
>>> mmc: queue: Introduce queue depth
>>> mmc: queue: Use queue depth to allocate and free
>>> mmc: queue: Allocate queue of size qdepth
>>> mmc: mmc: Add Command Queue definitions
>>> mmc: mmc: Add functions to enable / disable the Command Queue
>>> mmc: mmc_test: Disable Command Queue while mmc_test is used
>>> mmc: block: Disable Command Queue while RPMB is used
>>> mmc: core: Do not prepare a new request twice
>>> mmc: core: Export mmc_retune_hold() and mmc_retune_release()
>>> mmc: block: Factor out mmc_blk_requeue()
>>> mmc: block: Fix 4K native sector check
>>> mmc: block: Use local var for mqrq_cur
>>> mmc: block: Pass mqrq to mmc_blk_prep_packed_list()
>>> mmc: block: Introduce queue semantics
>>> mmc: queue: Share mmc request array between partitions
>>> mmc: queue: Add a function to control wake-up on new requests
>>> mmc: block: Add Software Command Queuing
>>> mmc: mmc: Enable Software Command Queuing
>>> mmc: sdhci-pci: Enable Software Command Queuing for some Intel
>>> controllers
>>> mmc: sdhci-acpi: Enable Software Command Queuing for some Intel
>>> controllers
>>>
>>> Documentation/mmc/mmc-dev-attrs.txt | 1 +
>>> drivers/mmc/card/block.c | 738
>>> +++++++++++++++++++++++++++++++++---
>>> drivers/mmc/card/mmc_test.c | 13 +
>>> drivers/mmc/card/queue.c | 332 ++++++++++------
>>> drivers/mmc/card/queue.h | 35 +-
>>> drivers/mmc/core/core.c | 18 +-
>>> drivers/mmc/core/host.c | 2 +
>>> drivers/mmc/core/host.h | 2 -
>>> drivers/mmc/core/mmc.c | 43 ++-
>>> drivers/mmc/core/mmc_ops.c | 27 ++
>>> drivers/mmc/host/sdhci-acpi.c | 2 +-
>>> drivers/mmc/host/sdhci-pci-core.c | 2 +-
>>> include/linux/mmc/card.h | 9 +
>>> include/linux/mmc/core.h | 5 +
>>> include/linux/mmc/host.h | 4 +-
>>> include/linux/mmc/mmc.h | 17 +
>>> 16 files changed, 1046 insertions(+), 204 deletions(-)
>>>
>>>
>>> Regards
>>> Adrian
>>>
>>
>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* RE: [PATCH 3/8] mmc: core: remove BUG_ONs from mmc
From: Venu Byravarasu @ 2016-11-02 9:40 UTC (permalink / raw)
To: Shawn Lin, Ulf Hansson; +Cc: linux-mmc@vger.kernel.org, Venu Byravarasu
In-Reply-To: <1478071533-12775-1-git-send-email-shawn.lin@rock-chips.com>
> -----Original Message-----
> From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-
> owner@vger.kernel.org] On Behalf Of Shawn Lin
> Sent: Wednesday, November 02, 2016 12:56 PM
> To: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: linux-mmc@vger.kernel.org; Shawn Lin <shawn.lin@rock-chips.com>
> Subject: [PATCH 3/8] mmc: core: remove BUG_ONs from mmc
>
> BUG_ONs doesn't help anything except for stop the system from running. If it
> occurs, it implies we should deploy proper error handling for that. So this patch is
> gonna discard these meaningless BUG_ONs and deploy error handling if needed.
>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
>
> drivers/mmc/core/mmc.c | 14 --------------
> drivers/mmc/core/mmc_ops.c | 17 -----------------
> 2 files changed, 31 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index
> e811bd9..4763a35 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -1464,7 +1464,6 @@ static int mmc_init_card(struct mmc_host *host, u32
> ocr,
> u32 cid[4];
> u32 rocr;
>
> - BUG_ON(!host);
Instead of just removing BUG_ON, it would be worth adding null check and
return from here graciously as you mentioned in patch commit message,
without which next instruction below may crash if host == NULL.
Similar is the case with other removals below.
> WARN_ON(!host->claimed);
>
> /* Set correct bus mode for MMC before attempting init */ @@ -1854,9
> +1853,6 @@ static int mmc_poweroff_notify(struct mmc_card *card, unsigned int
> notify_type)
> */
> static void mmc_remove(struct mmc_host *host) {
> - BUG_ON(!host);
> - BUG_ON(!host->card);
> -
> mmc_remove_card(host->card);
> host->card = NULL;
> }
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information. Any unauthorized review, use, disclosure or distribution
is prohibited. If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
^ permalink raw reply
* Re: [PATCH V5 00/25] mmc: mmc: Add Software Command Queuing
From: Adrian Hunter @ 2016-11-02 10:09 UTC (permalink / raw)
To: Ritesh Harjani, Ulf Hansson
Cc: linux-mmc, Alex Lemberg, Mateusz Nowak, Yuliy Izrailov,
Jaehoon Chung, Dong Aisheng, Das Asutosh, Zhangfei Gao,
Dorfman Konstantin, David Griego, Sahitya Tummala,
Venu Byravarasu
In-Reply-To: <4d64111f-7bdb-22ed-5a53-e23f6bfcb28b@codeaurora.org>
On 02/11/16 11:30, Ritesh Harjani wrote:
> Hi Adrian,
>
> Reply got delayed due to festive season holidays here.
>
> On 10/28/2016 6:18 PM, Adrian Hunter wrote:
>> On 28/10/16 14:48, Ritesh Harjani wrote:
>>> Hi Adrian,
>>>
>>>
>>> Thanks for the re-base. I am trying to apply this patch series and validate.
>>> I am seeing some tuning related errors, it could be due to my setup/device.
>>> I will be working on this.
>>>
>>>
>>> Meanwhile below perf readout was showing some discrepancy (could be that I
>>> am missing something).
>>> In your last test both sequential and random read gives almost same
>>> throughput and even the percentage increase is similar(legacy v/s SW cmdq).
>>> Could be due to benchmark itself ?
>>
>> Normally sequential reading would result in large transfers due to
>> readahead. But the test is using Direct-I/O (-I option) to prevent VFS
>> caching distorting the results. So the sequential read case ends up being
>> more like a random read case anyway.
>>
>>>
>>> Do you think we should get some other benchmarks tested as well?
>>> (may be tio?)
>>>
>>>
>>> Also could you please also add some analysis on where we should expect to
>>> see the score improvement in SW CMDQ and where we may see some decrements
>>> due to SW CMDQ?
>>
>> I would expect random reads to be improved, but there is the overhead of the
>> extra cmdq commands, so a small decrease in performance would be expected
>> otherwise.
>>
>>> You did mention in original cover-letter that we mostly should see
>>> improvement in Random read scores, but below here we are seeing similar or
>>> higher improvement in sequential reads(4 threads). Which is a bit
>>> surprising.
>>
>> As explained above: the test is called "sequential" but the limited record
>> size combined with Direct I/O makes it more like random I/O especially when
>> it is mixed from multiple threads.
> In that case if we see sequential read v/s random read for single thread of
> SWCMDQ, both show the same data. Both SR and RR should not be same ideally no?
Yes, with these iozone tests, SR and RR seem the same.
>
>
>>
>>>
>>>
>>>
>>>
>>> On 10/24/2016 2:07 PM, Adrian Hunter wrote:
>>>> Hi
>>>>
>>>> Here is an updated version of the Software Command Queuing patches,
>>>> re-based on next, with patches 1-5 dropped because they have been applied,
>>>> and 2 fixes that have been rolled in (refer Changes in V5 below).
>>>>
>>>> Performance results:
>>>>
>>>> Results can vary from run to run, but here are some results showing 1, 2
>>>> or 4
>>>> processes with 4k and 32k record sizes. They show up to 40% improvement in
>>>> read performance when there are multiple processes.
>>>>
>>>> iozone -s 8192k -r 4k -i 0 -i 1 -i 2 -i 8 -I -t 1 -F /mnt/mmc/iozone1.tmp
>>>>
>>>> Children see throughput for 1 initial writers = 27909.87
>>>> kB/sec 24204.14 kB/sec -13.28 %
>>>> Children see throughput for 1 rewriters = 28839.28 kB/sec
>>>> 25531.92 kB/sec -11.47 %
>>>> Children see throughput for 1 readers = 25889.65
>>>> kB/sec 24883.23 kB/sec -3.89 %
>>>> Children see throughput for 1 re-readers = 25558.23 kB/sec
>>>> 24679.89 kB/sec -3.44 %
>>>> Children see throughput for 1 random readers = 25571.48
>>>> kB/sec 24689.52 kB/sec -3.45 %
>>>> Children see throughput for 1 mixed workload = 25758.59
>>>> kB/sec 24487.52 kB/sec -4.93 %
>>>> Children see throughput for 1 random writers = 24787.51
>>>> kB/sec 19368.99 kB/sec -21.86 %
>>>>
>>>> iozone -s 8192k -r 32k -i 0 -i 1 -i 2 -i 8 -I -t 1 -F /mnt/mmc/iozone1.tmp
>>>>
>>>> Children see throughput for 1 initial writers = 91344.61
>>>> kB/sec 102008.56 kB/sec 11.67 %
>>>> Children see throughput for 1 rewriters = 87932.36 kB/sec
>>>> 96630.44 kB/sec 9.89 %
>>>> Children see throughput for 1 readers = 134879.82
>>>> kB/sec 110292.79 kB/sec -18.23 %
>>>> Children see throughput for 1 re-readers = 147632.13 kB/sec
>>>> 109053.33 kB/sec -26.13 %
>>>> Children see throughput for 1 random readers = 93547.37
>>>> kB/sec 112225.50 kB/sec 19.97 %
>>>> Children see throughput for 1 mixed workload = 93560.04
>>>> kB/sec 110515.21 kB/sec 18.12 %
>>>> Children see throughput for 1 random writers = 92841.84
>>>> kB/sec 81153.81 kB/sec -12.59 %
>>>>
>>>> iozone -s 8192k -r 4k -i 0 -i 1 -i 2 -i 8 -I -t 2 -F /mnt/mmc/iozone1.tmp
>>>> /mnt/mmc/iozone2.tmp
>>>>
>>>> Children see throughput for 2 initial writers = 31145.43
>>>> kB/sec 33771.25 kB/sec 8.43 %
>>>> Children see throughput for 2 rewriters = 30592.57 kB/sec
>>>> 35916.46 kB/sec 17.40 %
>>>> Children see throughput for 2 readers = 31669.83
>>>> kB/sec 37460.13 kB/sec 18.28 %
>>>> Children see throughput for 2 re-readers = 32079.94 kB/sec
>>>> 37373.33 kB/sec 16.50 %
>>>> Children see throughput for 2 random readers = 27731.19
>>>> kB/sec 37601.65 kB/sec 35.59 %
>>>> Children see throughput for 2 mixed workload = 13927.50
>>>> kB/sec 14617.06 kB/sec 4.95 %
>>>> Children see throughput for 2 random writers = 31250.00
>>>> kB/sec 33106.72 kB/sec 5.94 %
>>>>
>>>> iozone -s 8192k -r 32k -i 0 -i 1 -i 2 -i 8 -I -t 2 -F /mnt/mmc/iozone1.tmp
>>>> /mnt/mmc/iozone2.tmp
>>>>
>>>> Children see throughput for 2 initial writers = 123255.84
>>>> kB/sec 131252.22 kB/sec 6.49 %
>>>> Children see throughput for 2 rewriters = 115234.91 kB/sec
>>>> 107225.74 kB/sec -6.95 %
>>>> Children see throughput for 2 readers = 128921.86
>>>> kB/sec 148562.71 kB/sec 15.23 %
>>>> Children see throughput for 2 re-readers = 127815.24 kB/sec
>>>> 149304.32 kB/sec 16.81 %
>>>> Children see throughput for 2 random readers = 125600.46
>>>> kB/sec 148406.56 kB/sec 18.16 %
>>>> Children see throughput for 2 mixed workload = 44006.94
>>>> kB/sec 50937.36 kB/sec 15.75 %
>>>> Children see throughput for 2 random writers = 120623.95
>>>> kB/sec 103969.05 kB/sec -13.81 %
>>>>
>>>> iozone -s 8192k -r 4k -i 0 -i 1 -i 2 -i 8 -I -t 4 -F /mnt/mmc/iozone1.tmp
>>>> /mnt/mmc/iozone2.tmp /mnt/mmc/iozone3.tmp /mnt/mmc/iozone4.tmp
>>>>
>>>> Children see throughput for 4 initial writers = 24100.96
>>>> kB/sec 33336.58 kB/sec 38.32 %
>>>> Children see throughput for 4 rewriters = 31650.20 kB/sec
>>>> 33091.53 kB/sec 4.55 %
>>>> Children see throughput for 4 readers = 33276.92
>>>> kB/sec 41799.89 kB/sec 25.61 %
>>>> Children see throughput for 4 re-readers = 31786.96 kB/sec
>>>> 41501.74 kB/sec 30.56 %
>>>> Children see throughput for 4 random readers = 31991.65
>>>> kB/sec 40973.93 kB/sec 28.08 %
>>>> Children see throughput for 4 mixed workload = 15804.80
>>>> kB/sec 13581.32 kB/sec -14.07 %
>>>> Children see throughput for 4 random writers = 31231.42
>>>> kB/sec 34537.03 kB/sec 10.58 %
>>>>
>>>> iozone -s 8192k -r 32k -i 0 -i 1 -i 2 -i 8 -I -t 4 -F /mnt/mmc/iozone1.tmp
>>>> /mnt/mmc/iozone2.tmp /mnt/mmc/iozone3.tmp /mnt/mmc/iozone4.tmp
>>>>
>>>> Children see throughput for 4 initial writers = 116567.42
>>>> kB/sec 119280.35 kB/sec 2.33 %
>>>> Children see throughput for 4 rewriters = 115010.96 kB/sec
>>>> 120864.34 kB/sec 5.09 %
>>>> Children see throughput for 4 readers = 130700.29
>>>> kB/sec 177834.21 kB/sec 36.06 %
>>> Do you think sequential read will increase more that of random read. It
>>> should mostly benefit in random reads right. Any idea why it's behaving
>>> differently here?
>>
>> Explained above.
>>
>>>
>>>
>>>> Children see throughput for 4 re-readers = 125392.58 kB/sec
>>>> 175975.28 kB/sec 40.34 %
>>>> Children see throughput for 4 random readers = 132194.57
>>>> kB/sec 176630.46 kB/sec 33.61 %
>>>> Children see throughput for 4 mixed workload = 56464.98
>>>> kB/sec 54140.61 kB/sec -4.12 %
>>>> Children see throughput for 4 random writers = 109128.36
>>>> kB/sec 85359.80 kB/sec -21.78 %
>>> Similarly, we don't expect random write scores to decrease here. Do you know
>>> why this could be the case here?
>>
>> There is a lot of variation from one run to another
>
> These run-to-run variations are due to what, any idea?
Not really, but there is no knowing how much internal work the eMMC is
doing, wear-leveling, erasing blocks, cache flushing etc. Also "random"
writes that hit the same erase block can be combined when flushing the
cache, and random writes to the same sectors can also be combined - so the
potential for variation is very large.
> 21% variations is huge.
>
> Actually can we try and get some data which shows the difference between
> SeqR and RandR and also RandW should not decrease this much.
> May be by changing iozone parameters?
>
>
> How about below data ?
Yes that would be interesting
>
> For Sequential
> ./iozone -s 64m -r 32m -i 0 -i 1 -i 2 -i 8 -I -t 4 -F /data/mmc/iozone1.tmp
> /data/mmc/iozone2.tmp /data/mmc/iozone3.tmp /data/mmc/iozone4.tmp
>
> For random -
> ./iozone -s 64m -r 4k -i 0 -i 1 -i 2 -i 8 -I -t 4 -F /data/mmc/iozone1.tmp
> /data/mmc/iozone2.tmp /data/mmc/iozone3.tmp /data/mmc/iozone4.tmp
>
>
> Regards
> Ritesh
>
^ permalink raw reply
* Re: Regression after "do not use CMD13 to get status after speed mode switch"
From: Chaotian Jing @ 2016-11-02 10:28 UTC (permalink / raw)
To: Adrian Hunter
Cc: Ulf Hansson, Linus Walleij, linux-mmc@vger.kernel.org,
linux-arm-msm@vger.kernel.org, Bjorn Andersson, Stephen Boyd,
Andy Gross
In-Reply-To: <17bdfdf2-5c58-7b09-6d4d-4f5173f6058e@intel.com>
On Wed, 2016-11-02 at 10:19 +0200, Adrian Hunter wrote:
> On 01/11/16 03:43, Chaotian Jing wrote:
> > On Mon, 2016-10-31 at 15:09 +0200, Adrian Hunter wrote:
> >> On 27/10/16 13:04, Ulf Hansson wrote:
> >>> On 20 October 2016 at 09:06, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >>>> On 20 October 2016 at 04:22, Chaotian Jing <chaotian.jing@mediatek.com> wrote:
> >>>>> On Wed, 2016-10-19 at 18:41 +0200, Ulf Hansson wrote:
> >>>>>> Adrian, Linus,
> >>>>>>
> >>>>>> Thanks for looking into this and reporting!
> >>>>>>
> >>>>>> On 18 October 2016 at 15:23, Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>>>>>> On 18/10/16 11:36, Linus Walleij wrote:
> >>>>>>>> On Mon, Oct 17, 2016 at 4:32 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> >>>>>>>>
> >>>>>>>>> Before this patch the eMMC is detected and all partitions enumerated
> >>>>>>>>> immediately, but after the patch it doesn't come up at all, except
> >>>>>>>>> sometimes, when it appears minutes (!) after boot, all of a sudden.
> >>>>>>>>
> >>>>>>>> FYI this is what it looks like when it eventually happens:
> >>>>>>>> root@msm8660:/ [ 627.710175] mmc0: new high speed MMC card at address 0001
> >>>>>>>> [ 627.711641] mmcblk0: mmc0:0001 SEM04G 3.69 GiB
> >>>>>>>> [ 627.715485] mmcblk0boot0: mmc0:0001 SEM04G partition 1 1.00 MiB
> >>>>>>>> [ 627.736654] mmcblk0boot1: mmc0:0001 SEM04G partition 2 1.00 MiB
> >>>>>>>> [ 627.747397] mmcblk0rpmb: mmc0:0001 SEM04G partition 3 128 KiB
> >>>>>>>> [ 627.756326] mmcblk0: p1 p2 p3 p4 < p5 p6 p7 p8 p9 p10 p11 p12 p13
> >>>>>>>> p14 p15 p16 p17 p18 p19 p20 p21 >
> >>>>>>>>
> >>>>>>>> So after 627 seconds, a bit hard for users to wait this long for their
> >>>>>>>> root filesystem.
> >>>>>>>
> >>>>>>> If the driver does not support busy detection and the eMMC card provides
> >>>>>>> zero as the cmd6 generic timeout (which it may especially as cmd6 generic
> >>>>>>> timeout wasn't added until eMMCv4.5), then __mmc_switch() defaults to
> >>>>>>> waiting 10 minutes i.e.
> >>>>>>>
> >>>>>>> #define MMC_OPS_TIMEOUT_MS (10 * 60 * 1000) /* 10 minute timeout */
> >>>>>>
> >>>>>> Urgh! Yes, I have verified that this is exactly what happens.
> >>>>>>
> >>>>>>>
> >>>>>>> So removal of CMD13 polling for HS mode (as per commit
> >>>>>>> 08573eaf1a70104f83fdbee9b84e5be03480e9ed) is going to be a problem for some
> >>>>>>> combinations of eMMC cards and host drivers.
> >>>>>>
> >>>>>> I was looking in the __mmc_switch() function, it's just a pain to walk
> >>>>>> trough it :-) So first out I decided to clean it up and factor out the
> >>>>>> polling parts. I will post the patches first out tomorrow morning,
> >>>>>> running some final test right now.
> >>>>>>
> >>>>>> Although, that of course doesn't solve our problem. As I see it we
> >>>>>> only have a few options here.
> >>>>>>
> >>>>>> 1) In case when cmd6 generic timeout isn't available, let's assign
> >>>>>> another empirically selected value.
> >>>>>> 2) Use a specific timeout when switching to HS mode.
> >>>>>> 3) Even if we deploy 1 (and 2), perhaps we still should allow polling
> >>>>>> with CMD13 for switching to HS mode - unless it causes issues for some
> >>>>>> cards/drivers combination?
> >>>>>>
> >>>>>> BTW, I already tried 2) and it indeed solves the problem, although
> >>>>>> depending on the selected timeout, it might delay the card detection
> >>>>>> to process.
> >>>>>>
> >>>>>> Thoughts?
> >>>>>
> >>>>> I just have a try of switching to HS mode with Hynix EMMC, the first
> >>>>> CMD13 gets response of 0x900, but the EMMC is still pull-low DAT0. so
> >>>>> that CMD13 cannot indicate current card status in this case.
> >>>>
> >>>> Thanks for sharing that. Okay, so clearly we have some cards that
> >>>> don't supports polling with CMD13 when switching to HS mode.
> >>>> One could of course add quirks for these kind of cards and do a fixed
> >>>> delay for them, but then to find out which these cards are is going to
> >>>> be hard.
> >>>>
> >>>> It seems like we are left with using a fixed delay. Any ideas of what
> >>>> such delay should be? And should we have one specific for switch to
> >>>> the various speed modes and a different one that overrides the CMD6
> >>>> generic timout, when it doesn't exist?
> >>>>
> >>>
> >>> Replying to my own earlier response, as I believe the problem could
> >>> also be related to another old commit, see below.
> >>>
> >>> commit a27fbf2f067b0cd6f172c8b696b9a44c58bfaa7a
> >>> Author: Seungwon Jeon <tgih.jun@samsung.com>
> >>> Date: Wed Sep 4 21:21:05 2013 +0900
> >>>
> >>> mmc: add ignorance case for CMD13 CRC error
> >>>
> >>> While speed mode is changed, CMD13 cannot be guaranteed.
> >>> According to the spec., it is not recommended to use CMD13
> >>> to check the busy completion of the timing change.
> >>> If CMD13 is used in this case, CRC error must be ignored.
> >>>
> >>> Signed-off-by: Seungwon Jeon <tgih.jun@samsung.com>
> >>> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
> >>> Signed-off-by: Chris Ball <cjb@laptop.org>
> >>>
> >>>
> >>> The intent with this commit was not really correct. We don't want to
> >>> ignore CRC errors, but instead we should *re-try* sending CMD13 once
> >>> we get a CRC error.
> >>>
> >>> Unfortunate since this commit, instead we tell the host driver to
> >>> *ignore* CRC errors and instead reads the status and returns 0
> >>> (indicating success). In the mmc core, in __mmc_switch(), it will thus
> >>> parse the status reply, even for a reply that might have been received
> >>> with a CRC error. Not good!
> >>
> >> I agree: ignoring CRC errors and then expecting the status in the response
> >> to be correct doesn't make sense.
> >>
> >> However, it raises the question of what to do if there are always CRC errors
> >> e.g. if it only works without CRC errors once the mode and frequency are
> >> changed in the host controller.
> >>
> >>> I am wondering whether this actually is the main problem to why we
> >>> think polling isn't working for some cases. And perhaps that was the
> >>> original problem Chaotian was trying to solve?
> >>>
> >>> Thoughts?
> >>
> >> Does Chaotian have a real problem since his driver has busy detection anyway?
> >
> > In fact, I have not encounter CRC errors of CMD13, I have tried several
> > eMMC cards, after mode switch, CMD13 will only gets 0x800 response and
> > we don't know if card is busy by 0x800 response.
>
> Does it change to 0x900 when it is not busy?
>
No, it will not change to 0x900 when it is not busy.
> But anyway the question was: do you have busy detection in your driver?
>
driver has busy detection ops->card_busy() but seems it's MMC core
layer's responsibility to ensure that card is not busy when driver
starts to issue commands.
^ permalink raw reply
* Re: Regression after "do not use CMD13 to get status after speed mode switch"
From: Adrian Hunter @ 2016-11-02 12:51 UTC (permalink / raw)
To: Chaotian Jing
Cc: Ulf Hansson, Linus Walleij, linux-mmc@vger.kernel.org,
linux-arm-msm@vger.kernel.org, Bjorn Andersson, Stephen Boyd,
Andy Gross
In-Reply-To: <1478082516.3862.3.camel@mhfsdcap03>
On 02/11/16 12:28, Chaotian Jing wrote:
> On Wed, 2016-11-02 at 10:19 +0200, Adrian Hunter wrote:
>> On 01/11/16 03:43, Chaotian Jing wrote:
>>> On Mon, 2016-10-31 at 15:09 +0200, Adrian Hunter wrote:
>>>> On 27/10/16 13:04, Ulf Hansson wrote:
>>>>> On 20 October 2016 at 09:06, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>>> On 20 October 2016 at 04:22, Chaotian Jing <chaotian.jing@mediatek.com> wrote:
>>>>>>> On Wed, 2016-10-19 at 18:41 +0200, Ulf Hansson wrote:
>>>>>>>> Adrian, Linus,
>>>>>>>>
>>>>>>>> Thanks for looking into this and reporting!
>>>>>>>>
>>>>>>>> On 18 October 2016 at 15:23, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>>>>>> On 18/10/16 11:36, Linus Walleij wrote:
>>>>>>>>>> On Mon, Oct 17, 2016 at 4:32 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>>>>>>>
>>>>>>>>>>> Before this patch the eMMC is detected and all partitions enumerated
>>>>>>>>>>> immediately, but after the patch it doesn't come up at all, except
>>>>>>>>>>> sometimes, when it appears minutes (!) after boot, all of a sudden.
>>>>>>>>>>
>>>>>>>>>> FYI this is what it looks like when it eventually happens:
>>>>>>>>>> root@msm8660:/ [ 627.710175] mmc0: new high speed MMC card at address 0001
>>>>>>>>>> [ 627.711641] mmcblk0: mmc0:0001 SEM04G 3.69 GiB
>>>>>>>>>> [ 627.715485] mmcblk0boot0: mmc0:0001 SEM04G partition 1 1.00 MiB
>>>>>>>>>> [ 627.736654] mmcblk0boot1: mmc0:0001 SEM04G partition 2 1.00 MiB
>>>>>>>>>> [ 627.747397] mmcblk0rpmb: mmc0:0001 SEM04G partition 3 128 KiB
>>>>>>>>>> [ 627.756326] mmcblk0: p1 p2 p3 p4 < p5 p6 p7 p8 p9 p10 p11 p12 p13
>>>>>>>>>> p14 p15 p16 p17 p18 p19 p20 p21 >
>>>>>>>>>>
>>>>>>>>>> So after 627 seconds, a bit hard for users to wait this long for their
>>>>>>>>>> root filesystem.
>>>>>>>>>
>>>>>>>>> If the driver does not support busy detection and the eMMC card provides
>>>>>>>>> zero as the cmd6 generic timeout (which it may especially as cmd6 generic
>>>>>>>>> timeout wasn't added until eMMCv4.5), then __mmc_switch() defaults to
>>>>>>>>> waiting 10 minutes i.e.
>>>>>>>>>
>>>>>>>>> #define MMC_OPS_TIMEOUT_MS (10 * 60 * 1000) /* 10 minute timeout */
>>>>>>>>
>>>>>>>> Urgh! Yes, I have verified that this is exactly what happens.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> So removal of CMD13 polling for HS mode (as per commit
>>>>>>>>> 08573eaf1a70104f83fdbee9b84e5be03480e9ed) is going to be a problem for some
>>>>>>>>> combinations of eMMC cards and host drivers.
>>>>>>>>
>>>>>>>> I was looking in the __mmc_switch() function, it's just a pain to walk
>>>>>>>> trough it :-) So first out I decided to clean it up and factor out the
>>>>>>>> polling parts. I will post the patches first out tomorrow morning,
>>>>>>>> running some final test right now.
>>>>>>>>
>>>>>>>> Although, that of course doesn't solve our problem. As I see it we
>>>>>>>> only have a few options here.
>>>>>>>>
>>>>>>>> 1) In case when cmd6 generic timeout isn't available, let's assign
>>>>>>>> another empirically selected value.
>>>>>>>> 2) Use a specific timeout when switching to HS mode.
>>>>>>>> 3) Even if we deploy 1 (and 2), perhaps we still should allow polling
>>>>>>>> with CMD13 for switching to HS mode - unless it causes issues for some
>>>>>>>> cards/drivers combination?
>>>>>>>>
>>>>>>>> BTW, I already tried 2) and it indeed solves the problem, although
>>>>>>>> depending on the selected timeout, it might delay the card detection
>>>>>>>> to process.
>>>>>>>>
>>>>>>>> Thoughts?
>>>>>>>
>>>>>>> I just have a try of switching to HS mode with Hynix EMMC, the first
>>>>>>> CMD13 gets response of 0x900, but the EMMC is still pull-low DAT0. so
>>>>>>> that CMD13 cannot indicate current card status in this case.
>>>>>>
>>>>>> Thanks for sharing that. Okay, so clearly we have some cards that
>>>>>> don't supports polling with CMD13 when switching to HS mode.
>>>>>> One could of course add quirks for these kind of cards and do a fixed
>>>>>> delay for them, but then to find out which these cards are is going to
>>>>>> be hard.
>>>>>>
>>>>>> It seems like we are left with using a fixed delay. Any ideas of what
>>>>>> such delay should be? And should we have one specific for switch to
>>>>>> the various speed modes and a different one that overrides the CMD6
>>>>>> generic timout, when it doesn't exist?
>>>>>>
>>>>>
>>>>> Replying to my own earlier response, as I believe the problem could
>>>>> also be related to another old commit, see below.
>>>>>
>>>>> commit a27fbf2f067b0cd6f172c8b696b9a44c58bfaa7a
>>>>> Author: Seungwon Jeon <tgih.jun@samsung.com>
>>>>> Date: Wed Sep 4 21:21:05 2013 +0900
>>>>>
>>>>> mmc: add ignorance case for CMD13 CRC error
>>>>>
>>>>> While speed mode is changed, CMD13 cannot be guaranteed.
>>>>> According to the spec., it is not recommended to use CMD13
>>>>> to check the busy completion of the timing change.
>>>>> If CMD13 is used in this case, CRC error must be ignored.
>>>>>
>>>>> Signed-off-by: Seungwon Jeon <tgih.jun@samsung.com>
>>>>> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>>> Signed-off-by: Chris Ball <cjb@laptop.org>
>>>>>
>>>>>
>>>>> The intent with this commit was not really correct. We don't want to
>>>>> ignore CRC errors, but instead we should *re-try* sending CMD13 once
>>>>> we get a CRC error.
>>>>>
>>>>> Unfortunate since this commit, instead we tell the host driver to
>>>>> *ignore* CRC errors and instead reads the status and returns 0
>>>>> (indicating success). In the mmc core, in __mmc_switch(), it will thus
>>>>> parse the status reply, even for a reply that might have been received
>>>>> with a CRC error. Not good!
>>>>
>>>> I agree: ignoring CRC errors and then expecting the status in the response
>>>> to be correct doesn't make sense.
>>>>
>>>> However, it raises the question of what to do if there are always CRC errors
>>>> e.g. if it only works without CRC errors once the mode and frequency are
>>>> changed in the host controller.
>>>>
>>>>> I am wondering whether this actually is the main problem to why we
>>>>> think polling isn't working for some cases. And perhaps that was the
>>>>> original problem Chaotian was trying to solve?
>>>>>
>>>>> Thoughts?
>>>>
>>>> Does Chaotian have a real problem since his driver has busy detection anyway?
>>>
>>> In fact, I have not encounter CRC errors of CMD13, I have tried several
>>> eMMC cards, after mode switch, CMD13 will only gets 0x800 response and
>>> we don't know if card is busy by 0x800 response.
>>
>> Does it change to 0x900 when it is not busy?
>>
> No, it will not change to 0x900 when it is not busy.
>
>> But anyway the question was: do you have busy detection in your driver?
>>
> driver has busy detection ops->card_busy() but seems it's MMC core
> layer's responsibility to ensure that card is not busy when driver
> starts to issue commands.
I tried a card here. The time between HS switch response and busy
de-assertion was only 58us i.e. practically instant. The CMD6 response was
0x800 but the subsequent CMD13 response was 0x900.
How long does it take your failing card to switch to HS?
^ permalink raw reply
* [PATCH 0/4] mmc: sdhci: Fixes
From: Adrian Hunter @ 2016-11-02 13:49 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Harjani Ritesh
Hi
Here are 4 small sdhci fixes. Some of the fixes improve sdhci ability to
deal with errors (e.g. CRC errors ) when software command queuing is active.
Adrian Hunter (4):
mmc: sdhci: Fix CMD line reset interfering with ongoing data transfer
mmc: sdhci: Fix unexpected data interrupt handling
mmc: sdhci: Reset cmd and data circuits after tuning failure
mmc: sdhci: Fix missing enhanced strobe setting during runtime resume
drivers/mmc/host/sdhci.c | 36 ++++++++++++++++++++++++++----------
1 file changed, 26 insertions(+), 10 deletions(-)
Regards
Adrian
^ permalink raw reply
* [PATCH 1/4] mmc: sdhci: Fix CMD line reset interfering with ongoing data transfer
From: Adrian Hunter @ 2016-11-02 13:49 UTC (permalink / raw)
To: Ulf Hansson; +Cc: linux-mmc, Harjani Ritesh
In-Reply-To: <1478094551-24819-1-git-send-email-adrian.hunter@intel.com>
CMD line reset during an ongoing data transfer can cause the data transfer
to hang. Fix by delaying the reset until the data transfer is finished.
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Cc: stable@vger.kernel.org # v4.8+
---
drivers/mmc/host/sdhci.c | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 1e25b01600e1..ad135d4449e6 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2283,10 +2283,8 @@ static bool sdhci_request_done(struct sdhci_host *host)
for (i = 0; i < SDHCI_MAX_MRQS; i++) {
mrq = host->mrqs_done[i];
- if (mrq) {
- host->mrqs_done[i] = NULL;
+ if (mrq)
break;
- }
}
if (!mrq) {
@@ -2317,6 +2315,17 @@ static bool sdhci_request_done(struct sdhci_host *host)
* upon error conditions.
*/
if (sdhci_needs_reset(host, mrq)) {
+ /*
+ * Do not finish until command and data lines are available for
+ * reset. Note there can only be one other mrq, so it cannot
+ * also be in mrqs_done, otherwise host->cmd and host->data_cmd
+ * would both be null.
+ */
+ if (host->cmd || host->data_cmd) {
+ spin_unlock_irqrestore(&host->lock, flags);
+ return true;
+ }
+
/* Some controllers need this kick or reset won't work here */
if (host->quirks & SDHCI_QUIRK_CLOCK_BEFORE_RESET)
/* This is to force an update */
@@ -2324,10 +2333,8 @@ static bool sdhci_request_done(struct sdhci_host *host)
/* Spec says we should do both at the same time, but Ricoh
controllers do not like that. */
- if (!host->cmd)
- sdhci_do_reset(host, SDHCI_RESET_CMD);
- if (!host->data_cmd)
- sdhci_do_reset(host, SDHCI_RESET_DATA);
+ sdhci_do_reset(host, SDHCI_RESET_CMD);
+ sdhci_do_reset(host, SDHCI_RESET_DATA);
host->pending_reset = false;
}
@@ -2335,6 +2342,8 @@ static bool sdhci_request_done(struct sdhci_host *host)
if (!sdhci_has_requests(host))
sdhci_led_deactivate(host);
+ host->mrqs_done[i] = NULL;
+
mmiowb();
spin_unlock_irqrestore(&host->lock, flags);
--
1.9.1
^ permalink raw reply related
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