[PATCH v4 06/14] mmc: sdhci-esdhc-imx: add strobe-dll-delay-target support

[PATCH v4 06/14] mmc: sdhci-esdhc-imx: add strobe-dll-delay-target support

[haibo.chen]

From: Haibo Chen <haibo.chen@nxp.com>

strobe-dll-delay-target is the delay cell add on the strobe line.
Strobe line the the uSDHC loopback read clock which is use in HS400
mode. Different strobe-dll-delay-target may need to set for different
board/SoC. If this delay cell is not set to an appropriate value,
we may see some read operation meet CRC error after HS400 mode select
which already pass the tuning.

This patch add the strobe-dll-delay-target setting in driver, so that
user can easily config this delay cell in dts file.

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c          | 12 +++++++++++-
 include/linux/platform_data/mmc-esdhc-imx.h |  1 +
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 6bcdc5743d94..3359153304a3 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -73,6 +73,7 @@
 #define ESDHC_STROBE_DLL_CTRL		0x70
 #define ESDHC_STROBE_DLL_CTRL_ENABLE	(1 << 0)
 #define ESDHC_STROBE_DLL_CTRL_RESET	(1 << 1)
+#define ESDHC_STROBE_DLL_CTRL_SLV_DLY_TARGET_DEFAULT	0x7
 #define ESDHC_STROBE_DLL_CTRL_SLV_DLY_TARGET_SHIFT	3
 #define ESDHC_STROBE_DLL_CTRL_SLV_UPDATE_INT_DEFAULT	(4 << 20)
 
@@ -993,6 +994,9 @@ static int esdhc_change_pinstate(struct sdhci_host *host,
  */
 static void esdhc_set_strobe_dll(struct sdhci_host *host)
 {
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
+	u32 strobe_delay;
 	u32 v;
 
 	/* disable clock before enabling strobe dll */
@@ -1010,9 +1014,13 @@ static void esdhc_set_strobe_dll(struct sdhci_host *host)
 	 * enable strobe dll ctrl and adjust the delay target
 	 * for the uSDHC loopback read clock
 	 */
+	if (imx_data->boarddata.strobe_dll_delay_target)
+		strobe_delay = imx_data->boarddata.strobe_dll_delay_target;
+	else
+		strobe_delay = ESDHC_STROBE_DLL_CTRL_SLV_DLY_TARGET_DEFAULT;
 	v = ESDHC_STROBE_DLL_CTRL_ENABLE |
 		ESDHC_STROBE_DLL_CTRL_SLV_UPDATE_INT_DEFAULT |
-		(7 << ESDHC_STROBE_DLL_CTRL_SLV_DLY_TARGET_SHIFT);
+		(strobe_delay << ESDHC_STROBE_DLL_CTRL_SLV_DLY_TARGET_SHIFT);
 	writel(v, host->ioaddr + ESDHC_STROBE_DLL_CTRL);
 	/* wait 5us to make sure strobe dll status register stable */
 	udelay(5);
@@ -1338,6 +1346,8 @@ sdhci_esdhc_imx_probe_dt(struct platform_device *pdev,
 	of_property_read_u32(np, "fsl,tuning-start-tap",
 			     &boarddata->tuning_start_tap);
 
+	of_property_read_u32(np, "fsl,strobe-dll-delay-target",
+				&boarddata->strobe_dll_delay_target);
 	if (of_find_property(np, "no-1-8-v", NULL))
 		host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
 
diff --git a/include/linux/platform_data/mmc-esdhc-imx.h b/include/linux/platform_data/mmc-esdhc-imx.h
index 6c006078c8a1..0434f68eda86 100644
--- a/include/linux/platform_data/mmc-esdhc-imx.h
+++ b/include/linux/platform_data/mmc-esdhc-imx.h
@@ -37,5 +37,6 @@ struct esdhc_platform_data {
 	unsigned int delay_line;
 	unsigned int tuning_step;       /* The delay cell steps in tuning procedure */
 	unsigned int tuning_start_tap;	/* The start delay cell point in tuning procedure */
+	unsigned int strobe_dll_delay_target;	/* The delay cell for strobe pad (read clock) */
 };
 #endif /* __ASM_ARCH_IMX_ESDHC_H */
-- 
2.17.1

[PATCH v4 07/14] mmc: sdhci-esdhc-imx: optimize the clock setting

[haibo.chen]

From: Haibo Chen <haibo.chen@nxp.com>

When force clock off, check the SDOFF of register PRSSTAT to make sure
the clock is gate off. Before force clock on, check the SDSTB of register
PRSSTAT to make sure the clock is stable, this will eliminate the clock
glitch.

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 24 +++++++++++++++++++++++-
 drivers/mmc/host/sdhci-esdhc.h     |  1 +
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 3359153304a3..6ca01b766604 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -9,6 +9,7 @@
  */
 
 #include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/clk.h>
@@ -312,6 +313,17 @@ static inline void esdhc_clrset_le(struct sdhci_host *host, u32 mask, u32 val, i
 	writel(((readl(base) & ~(mask << shift)) | (val << shift)), base);
 }
 
+static inline void esdhc_wait_for_card_clock_gate_off(struct sdhci_host *host)
+{
+	u32 present_state;
+	int ret;
+
+	ret = readl_poll_timeout(host->ioaddr + ESDHC_PRSSTAT, present_state,
+				(present_state & ESDHC_CLOCK_GATE_OFF), 2, 100);
+	if (ret == -ETIMEDOUT)
+		dev_warn(mmc_dev(host->mmc), "%s: card clock still not gate off in 100us!.\n", __func__);
+}
+
 static u32 esdhc_readl_le(struct sdhci_host *host, int reg)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -525,6 +537,8 @@ static void esdhc_writew_le(struct sdhci_host *host, u16 val, int reg)
 		else
 			new_val &= ~ESDHC_VENDOR_SPEC_FRC_SDCLK_ON;
 		writel(new_val, host->ioaddr + ESDHC_VENDOR_SPEC);
+		if (!(new_val & ESDHC_VENDOR_SPEC_FRC_SDCLK_ON))
+			esdhc_wait_for_card_clock_gate_off(host);
 		return;
 	case SDHCI_HOST_CONTROL2:
 		new_val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
@@ -753,12 +767,14 @@ static inline void esdhc_pltfm_set_clock(struct sdhci_host *host,
 	int ddr_pre_div = imx_data->is_ddr ? 2 : 1;
 	int pre_div = 1;
 	int div = 1;
+	int ret;
 	u32 temp, val;
 
 	if (esdhc_is_usdhc(imx_data)) {
 		val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
 		writel(val & ~ESDHC_VENDOR_SPEC_FRC_SDCLK_ON,
 			host->ioaddr + ESDHC_VENDOR_SPEC);
+		esdhc_wait_for_card_clock_gate_off(host);
 	}
 
 	if (clock == 0) {
@@ -813,13 +829,18 @@ static inline void esdhc_pltfm_set_clock(struct sdhci_host *host,
 		| (pre_div << ESDHC_PREDIV_SHIFT));
 	sdhci_writel(host, temp, ESDHC_SYSTEM_CONTROL);
 
+	/* need to wait the bit 3 of the PRSSTAT to be set, make sure card clock is stable */
+	ret = readl_poll_timeout(host->ioaddr + ESDHC_PRSSTAT, temp,
+				(temp & ESDHC_CLOCK_STABLE), 2, 100);
+	if (ret == -ETIMEDOUT)
+		dev_warn(mmc_dev(host->mmc), "card clock still not stable in 100us!.\n");
+
 	if (esdhc_is_usdhc(imx_data)) {
 		val = readl(host->ioaddr + ESDHC_VENDOR_SPEC);
 		writel(val | ESDHC_VENDOR_SPEC_FRC_SDCLK_ON,
 			host->ioaddr + ESDHC_VENDOR_SPEC);
 	}
 
-	mdelay(1);
 }
 
 static unsigned int esdhc_pltfm_get_ro(struct sdhci_host *host)
@@ -1003,6 +1024,7 @@ static void esdhc_set_strobe_dll(struct sdhci_host *host)
 	writel(readl(host->ioaddr + ESDHC_VENDOR_SPEC) &
 		~ESDHC_VENDOR_SPEC_FRC_SDCLK_ON,
 		host->ioaddr + ESDHC_VENDOR_SPEC);
+	esdhc_wait_for_card_clock_gate_off(host);
 
 	/* force a reset on strobe dll */
 	writel(ESDHC_STROBE_DLL_CTRL_RESET,
diff --git a/drivers/mmc/host/sdhci-esdhc.h b/drivers/mmc/host/sdhci-esdhc.h
index 9289bb4d633e..947212f16bc6 100644
--- a/drivers/mmc/host/sdhci-esdhc.h
+++ b/drivers/mmc/host/sdhci-esdhc.h
@@ -31,6 +31,7 @@
 
 /* Present State Register */
 #define ESDHC_PRSSTAT			0x24
+#define ESDHC_CLOCK_GATE_OFF		0x00000080
 #define ESDHC_CLOCK_STABLE		0x00000008
 
 /* Protocol Control Register */
-- 
2.17.1

[PATCH v4 08/14] mmc: sdhci-esdhc-imx: optimize the strobe dll setting

[haibo.chen]

From: Haibo Chen <haibo.chen@nxp.com>

After set the STROBE SLV delay target value, it need to wait some
time to let the usdhc lock the REF and SLV clock. In normal case,
1~2us is enough for imx8/imx6 and imx7d, and 4~5us is enough for
imx7ulp, but when do reboot stress test or do the bind/unbind stress
test, sometimes need to wait about 10us to get the status lock.

This patch optimize delay handle method, only print the warning
message if the status is still not lock after 1ms delay.

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 6ca01b766604..1ec35cc45fc5 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -1019,6 +1019,7 @@ static void esdhc_set_strobe_dll(struct sdhci_host *host)
 	struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
 	u32 strobe_delay;
 	u32 v;
+	int ret;
 
 	/* disable clock before enabling strobe dll */
 	writel(readl(host->ioaddr + ESDHC_VENDOR_SPEC) &
@@ -1044,15 +1045,13 @@ static void esdhc_set_strobe_dll(struct sdhci_host *host)
 		ESDHC_STROBE_DLL_CTRL_SLV_UPDATE_INT_DEFAULT |
 		(strobe_delay << ESDHC_STROBE_DLL_CTRL_SLV_DLY_TARGET_SHIFT);
 	writel(v, host->ioaddr + ESDHC_STROBE_DLL_CTRL);
-	/* wait 5us to make sure strobe dll status register stable */
-	udelay(5);
-	v = readl(host->ioaddr + ESDHC_STROBE_DLL_STATUS);
-	if (!(v & ESDHC_STROBE_DLL_STS_REF_LOCK))
-		dev_warn(mmc_dev(host->mmc),
-		"warning! HS400 strobe DLL status REF not lock!\n");
-	if (!(v & ESDHC_STROBE_DLL_STS_SLV_LOCK))
+
+	/* wait max 50us to get the REF/SLV lock */
+	ret = readl_poll_timeout(host->ioaddr + ESDHC_STROBE_DLL_STATUS, v,
+		((v & ESDHC_STROBE_DLL_STS_REF_LOCK) && (v & ESDHC_STROBE_DLL_STS_SLV_LOCK)), 1, 50);
+	if (ret == -ETIMEDOUT)
 		dev_warn(mmc_dev(host->mmc),
-		"warning! HS400 strobe DLL status SLV not lock!\n");
+		"warning! HS400 strobe DLL status REF/SLV not lock in 50us, STROBE DLL status is %x!\n", v);
 }
 
 static void esdhc_reset_tuning(struct sdhci_host *host)
-- 
2.17.1

[PATCH v4 09/14] mmc: sdhci-esdhc-imx: add flag ESDHC_FLAG_BROKEN_AUTO_CMD23

[haibo.chen]

From: Haibo Chen <haibo.chen@nxp.com>

Since L4.15, community involve the commit 105819c8a545 ("mmc: core: use mrq->sbc
when sending CMD23 for RPMB"), let the usdhc to decide whether to use ACMD23 for
RPMB. This CMD23 for RPMB need to set the bit 31 to its argument, if not, the
RPMB write operation will return general fail.

According to the sdhci logic, SDMA mode will disable the ACMD23, and only in
ADMA mode, it will chose to use ACMD23 if the host support. But according to
debug, and confirm with IC, the imx6qpdl/imx6sx/imx6sl/imx7d do not support
the ACMD23 feature completely. These SoCs only use the 16 bit block count of
the register 0x4 (BLOCK_ATT) as the CMD23's argument in ACMD23 mode, which
means it will ignore the upper 16 bit of the CMD23's argument. This will block
the reliable write operation in RPMB, because RPMB reliable write need to set
the bit31 of the CMD23's argument. This is the hardware limitation. So for
imx6qpdl/imx6sx/imx6sl/imx7d, it need to broke the ACMD23 for eMMC, SD card do
not has this limitation, because SD card do not support reliable write.

For imx6ul/imx6ull/imx6sll/imx7ulp/imx8, it support the ACMD23 completely, it
change to use the 0x0 register (DS_ADDR) to put the CMD23's argument in ADMA mode.

This patch add a new flag ESDHC_FLAG_BROKEN_AUTO_CMD23, and add this flag to
imx6q/imx6sx/imx6sl/imx7d, and due to the imx6sll share the same compatible string
with imx6sx before, and imx6sll do not has this limitation, so add a new compatible
string for imx6sll.

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 34 ++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 1ec35cc45fc5..98b91b6ff2a9 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -166,6 +166,18 @@
 #define ESDHC_FLAG_STATE_LOST_IN_LPMODE		BIT(14)
 /* The IP lost clock rate in PM_RUNTIME */
 #define ESDHC_FLAG_CLK_RATE_LOST_IN_PM_RUNTIME	BIT(15)
+/*
+ * The IP do not support the ACMD23 feature completely when use ADMA mode.
+ * In ADMA mode, it only use the 16 bit block count of the register 0x4
+ * (BLOCK_ATT) as the CMD23's argument for ACMD23 mode, which means it will
+ * ignore the upper 16 bit of the CMD23's argument. This will block the reliable
+ * write operation in RPMB, because RPMB reliable write need to set the bit31
+ * of the CMD23's argument.
+ * imx6qpdl/imx6sx/imx6sl/imx7d has this limitation only for ADMA mode, SDMA
+ * do not has this limitation. so when these SoC use ADMA mode, it need to
+ * disable the ACMD23 feature.
+ */
+#define ESDHC_FLAG_BROKEN_AUTO_CMD23	BIT(16)
 
 struct esdhc_soc_data {
 	u32 flags;
@@ -188,21 +200,30 @@ static const struct esdhc_soc_data esdhc_imx53_data = {
 };
 
 static const struct esdhc_soc_data usdhc_imx6q_data = {
-	.flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_MAN_TUNING,
+	.flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_MAN_TUNING
+			| ESDHC_FLAG_BROKEN_AUTO_CMD23,
 };
 
 static const struct esdhc_soc_data usdhc_imx6sl_data = {
 	.flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
 			| ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_ERR004536
-			| ESDHC_FLAG_HS200,
+			| ESDHC_FLAG_HS200
+			| ESDHC_FLAG_BROKEN_AUTO_CMD23,
 };
 
-static const struct esdhc_soc_data usdhc_imx6sx_data = {
+static const struct esdhc_soc_data usdhc_imx6sll_data = {
 	.flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
 			| ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
 			| ESDHC_FLAG_STATE_LOST_IN_LPMODE,
 };
 
+static const struct esdhc_soc_data usdhc_imx6sx_data = {
+	.flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
+			| ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
+			| ESDHC_FLAG_STATE_LOST_IN_LPMODE
+			| ESDHC_FLAG_BROKEN_AUTO_CMD23,
+};
+
 static const struct esdhc_soc_data usdhc_imx6ull_data = {
 	.flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
 			| ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
@@ -214,7 +235,8 @@ static const struct esdhc_soc_data usdhc_imx7d_data = {
 	.flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
 			| ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
 			| ESDHC_FLAG_HS400
-			| ESDHC_FLAG_STATE_LOST_IN_LPMODE,
+			| ESDHC_FLAG_STATE_LOST_IN_LPMODE
+			| ESDHC_FLAG_BROKEN_AUTO_CMD23,
 };
 
 static struct esdhc_soc_data usdhc_imx7ulp_data = {
@@ -276,6 +298,7 @@ static const struct of_device_id imx_esdhc_dt_ids[] = {
 	{ .compatible = "fsl,imx53-esdhc", .data = &esdhc_imx53_data, },
 	{ .compatible = "fsl,imx6sx-usdhc", .data = &usdhc_imx6sx_data, },
 	{ .compatible = "fsl,imx6sl-usdhc", .data = &usdhc_imx6sl_data, },
+	{ .compatible = "fsl,imx6sll-usdhc", .data = &usdhc_imx6sll_data, },
 	{ .compatible = "fsl,imx6q-usdhc", .data = &usdhc_imx6q_data, },
 	{ .compatible = "fsl,imx6ull-usdhc", .data = &usdhc_imx6ull_data, },
 	{ .compatible = "fsl,imx7d-usdhc", .data = &usdhc_imx7d_data, },
@@ -1560,6 +1583,9 @@ static int sdhci_esdhc_imx_probe(struct platform_device *pdev)
 	if (imx_data->socdata->flags & ESDHC_FLAG_HS400)
 		host->quirks2 |= SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400;
 
+	if (imx_data->socdata->flags & ESDHC_FLAG_BROKEN_AUTO_CMD23)
+		host->quirks2 |= SDHCI_QUIRK2_ACMD23_BROKEN;
+
 	if (imx_data->socdata->flags & ESDHC_FLAG_HS400_ES) {
 		host->mmc->caps2 |= MMC_CAP2_HS400_ES;
 		host->mmc_host_ops.hs400_enhanced_strobe =
-- 
2.17.1

[PATCH v4 10/14] mmc: sdhci-esdhc-imx: Add an new esdhc_soc_data for i.MX8MM

[haibo.chen]

From: Haibo Chen <haibo.chen@nxp.com>

Add new esdhc_soc_data, with compatible string "fsl,imx8mm-usdhc".

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 98b91b6ff2a9..100c081cfcae 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -255,6 +255,14 @@ static struct esdhc_soc_data usdhc_imx8qxp_data = {
 			| ESDHC_FLAG_CLK_RATE_LOST_IN_PM_RUNTIME,
 };
 
+static struct esdhc_soc_data usdhc_imx8mm_data = {
+	.flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
+			| ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
+			| ESDHC_FLAG_HS400 | ESDHC_FLAG_HS400_ES
+			| ESDHC_FLAG_CQHCI
+			| ESDHC_FLAG_STATE_LOST_IN_LPMODE,
+};
+
 struct pltfm_imx_data {
 	u32 scratchpad;
 	struct pinctrl *pinctrl;
@@ -304,6 +312,7 @@ static const struct of_device_id imx_esdhc_dt_ids[] = {
 	{ .compatible = "fsl,imx7d-usdhc", .data = &usdhc_imx7d_data, },
 	{ .compatible = "fsl,imx7ulp-usdhc", .data = &usdhc_imx7ulp_data, },
 	{ .compatible = "fsl,imx8qxp-usdhc", .data = &usdhc_imx8qxp_data, },
+	{ .compatible = "fsl,imx8mm-usdhc", .data = &usdhc_imx8mm_data, },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, imx_esdhc_dt_ids);
-- 
2.17.1

[PATCH v4 11/14] mmc: sdhci-esdhc-imx: clear pending interrupt and halt cqhci

[haibo.chen]

From: Haibo Chen <haibo.chen@nxp.com>

On i.MX8MM, we are running Dual Linux OS, with 1st Linux using SD Card
as rootfs storage, 2nd Linux using eMMC as rootfs storage. We let the
the 1st linux configure power/clock for the 2nd Linux.

When the 2nd Linux is booting into rootfs stage, we let the 1st Linux
to destroy the 2nd linux, then restart the 2nd linux, we met SDHCI dump
as following, after we clear the pending interrupt and halt CQCTL, issue
gone.

[ 1.334594] mmc2: Got command interrupt 0x00000001 even though no command operation was in progress.
[ 1.334595] mmc2: sdhci: ============ SDHCI REGISTER DUMP ===========
[ 1.334599] mmc2: sdhci: Sys addr: 0xa05dcc00 | Version: 0x00000002
[ 1.345538] mmc2: sdhci: Blk size: 0x00000200 | Blk cnt: 0x00000000
[ 1.345541] mmc2: sdhci: Argument: 0x00018000 | Trn mode: 0x00000033
[ 1.345543] mmc2: sdhci: Present: 0x01f88008 | Host ctl: 0x00000031
[ 1.345547] mmc2: sdhci: Power: 0x00000002 | Blk gap: 0x00000080
[ 1.357903] mmc2: sdhci: Wake-up: 0x00000008 | Clock: 0x0000003f
[ 1.357905] mmc2: sdhci: Timeout: 0x0000008f | Int stat: 0x00000000
[ 1.357908] mmc2: sdhci: Int enab: 0x107f100b | Sig enab: 0x107f100b
[ 1.357911] mmc2: sdhci: AC12 err: 0x00000000 | Slot int: 0x00000502
[ 1.370268] mmc2: sdhci: Caps: 0x07eb0000 | Caps_1: 0x0000b400
[ 1.370270] mmc2: sdhci: Cmd: 0x00000d1a | Max curr: 0x00ffffff
[ 1.370273] mmc2: sdhci: Resp[0]: 0x00000b00 | Resp[1]: 0xffffffff
[ 1.370276] mmc2: sdhci: Resp[2]: 0x328f5903 | Resp[3]: 0x00d00f00
[ 1.382132] mmc2: sdhci: Host ctl2: 0x00000000
[ 1.382135] mmc2: sdhci: ADMA Err: 0x00000000 | ADMA Ptr: 0xa2040208

[ 2.060932] mmc2: Unexpected interrupt 0x00004000.
[ 2.065538] mmc2: sdhci: ============ SDHCI REGISTER DUMP ===========
[ 2.071720] mmc2: sdhci: Sys addr: 0x00000000 | Version: 0x00000002
[ 2.077902] mmc2: sdhci: Blk size: 0x00000200 | Blk cnt: 0x00000001
[ 2.084083] mmc2: sdhci: Argument: 0x00000000 | Trn mode: 0x00000000
[ 2.090264] mmc2: sdhci: Present: 0x01f88009 | Host ctl: 0x00000011
[ 2.096446] mmc2: sdhci: Power: 0x00000002 | Blk gap: 0x00000080
[ 2.102627] mmc2: sdhci: Wake-up: 0x00000008 | Clock: 0x000010ff
[ 2.108809] mmc2: sdhci: Timeout: 0x0000008f | Int stat: 0x00004000
[ 2.114990] mmc2: sdhci: Int enab: 0x007f1003 | Sig enab: 0x007f1003
[ 2.121171] mmc2: sdhci: AC12 err: 0x00000000 | Slot int: 0x00000502
[ 2.127353] mmc2: sdhci: Caps: 0x07eb0000 | Caps_1: 0x0000b400
[ 2.133534] mmc2: sdhci: Cmd: 0x0000371a | Max curr: 0x00ffffff
[ 2.139715] mmc2: sdhci: Resp[0]: 0x00000900 | Resp[1]: 0xffffffff
[ 2.145896] mmc2: sdhci: Resp[2]: 0x328f5903 | Resp[3]: 0x00d00f00
[ 2.152077] mmc2: sdhci: Host ctl2: 0x00000000
[ 2.156342] mmc2: sdhci: ADMA Err: 0x00000000 | ADMA Ptr: 0x00000000

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 100c081cfcae..106097cbd0d4 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -1233,6 +1233,7 @@ static void sdhci_esdhc_imx_hwinit(struct sdhci_host *host)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
+	struct cqhci_host *cq_host = host->mmc->cqe_private;
 	int tmp;
 
 	if (esdhc_is_usdhc(imx_data)) {
@@ -1309,6 +1310,21 @@ static void sdhci_esdhc_imx_hwinit(struct sdhci_host *host)
 			tmp &= ~ESDHC_STD_TUNING_EN;
 			writel(tmp, host->ioaddr + ESDHC_TUNING_CTRL);
 		}
+
+		/*
+		 * On i.MX8MM, we are running Dual Linux OS, with 1st Linux using SD Card
+		 * as rootfs storage, 2nd Linux using eMMC as rootfs storage. We let the
+		 * the 1st linux configure power/clock for the 2nd Linux.
+		 *
+		 * When the 2nd Linux is booting into rootfs stage, we let the 1st Linux
+		 * to destroy the 2nd linux, then restart the 2nd linux, we met SDHCI dump.
+		 * After we clear the pending interrupt and halt CQCTL, issue gone.
+		 */
+		if (cq_host) {
+			tmp = cqhci_readl(cq_host, CQHCI_IS);
+			cqhci_writel(cq_host, tmp, CQHCI_IS);
+			cqhci_writel(cq_host, CQHCI_HALT, CQHCI_CTL);
+		}
 	}
 }
 
-- 
2.17.1

[PATCH v4 12/14] mmc: sdhci-esdhc-imx: clear DMA_SEL when disable DMA mode

[haibo.chen]

From: Haibo Chen <haibo.chen@nxp.com>

Currently, when use standard tuning, driver default disable DMA just before
send tuning command. But on i.MX8 usdhc, this is not enough. Need also clear
DMA_SEL. If not, once the DMA_SEL select AMDA2 before, even dma already disabled,
when send tuning command, usdhc will still prefetch the ADMA script from wrong
DMA address, then we will see IOMMU report some error which show lack of TLB
mapping.

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 106097cbd0d4..786305309eb0 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -639,10 +639,24 @@ static void esdhc_writew_le(struct sdhci_host *host, u16 val, int reg)
 			 * For DMA access restore the levels to default value.
 			 */
 			m = readl(host->ioaddr + ESDHC_WTMK_LVL);
-			if (val & SDHCI_TRNS_DMA)
+			if (val & SDHCI_TRNS_DMA) {
 				wml = ESDHC_WTMK_LVL_WML_VAL_DEF;
-			else
+			} else {
+				u8 ctrl;
 				wml = ESDHC_WTMK_LVL_WML_VAL_MAX;
+
+				/*
+				 * Since already disable DMA mode, so also need
+				 * to clear the DMASEL. Otherwise, for standard
+				 * tuning, when send tuning command, usdhc will
+				 * still prefetch the ADMA script from wrong
+				 * DMA address, then we will see IOMMU report
+				 * some error which show lack of TLB mapping.
+				 */
+				ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
+				ctrl &= ~SDHCI_CTRL_DMA_MASK;
+				sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
+			}
 			m &= ~(ESDHC_WTMK_LVL_RD_WML_MASK |
 			       ESDHC_WTMK_LVL_WR_WML_MASK);
 			m |= (wml << ESDHC_WTMK_LVL_RD_WML_SHIFT) |
-- 
2.17.1

[PATCH v4 13/14] mmc: sdhci-esdhc-imx: restore pin state when resume back

[haibo.chen]

From: Haibo Chen <haibo.chen@nxp.com>

In some low power mode, SoC will lose the pin state, so need to restore
the pin state when resume back.

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 786305309eb0..b38b9d7f0a0d 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -1731,7 +1731,11 @@ static int sdhci_esdhc_suspend(struct device *dev)
 	if (host->tuning_mode != SDHCI_TUNING_MODE_3)
 		mmc_retune_needed(host->mmc);
 
-	return sdhci_suspend_host(host);
+	ret = sdhci_suspend_host(host);
+	if (!ret)
+		return pinctrl_pm_select_sleep_state(dev);
+
+	return ret;
 }
 
 static int sdhci_esdhc_resume(struct device *dev)
@@ -1739,6 +1743,10 @@ static int sdhci_esdhc_resume(struct device *dev)
 	struct sdhci_host *host = dev_get_drvdata(dev);
 	int ret;
 
+	ret = pinctrl_pm_select_default_state(dev);
+	if (ret)
+		return ret;
+
 	/* re-initialize hw state in case it's lost in low power mode */
 	sdhci_esdhc_imx_hwinit(host);
 
-- 
2.17.1

[PATCH v4 14/14] mmc: queue: create dev->dma_parms before call dma_set_max_seg_size()

[haibo.chen]

From: Haibo Chen <haibo.chen@nxp.com>

To make dma_set_max_seg_size() work, need to create dev->dma_parms.

Find this issue on i.MX8QM mek board, this platform config the
max_segment_size to 65535, but this dma_set_max_seg_size do not
actuall work, find sometimes the segment size is 65536, exceed
the hardware max segment limitation, trigger ADMA error.

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
---
 drivers/mmc/core/queue.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
index 9edc08685e86..91094e56f7c9 100644
--- a/drivers/mmc/core/queue.c
+++ b/drivers/mmc/core/queue.c
@@ -359,6 +359,7 @@ static const struct blk_mq_ops mmc_mq_ops = {
 static void mmc_setup_queue(struct mmc_queue *mq, struct mmc_card *card)
 {
 	struct mmc_host *host = card->host;
+	struct device *dev = mmc_dev(host);
 	unsigned block_size = 512;
 
 	blk_queue_flag_set(QUEUE_FLAG_NONROT, mq->queue);
@@ -366,13 +367,12 @@ static void mmc_setup_queue(struct mmc_queue *mq, struct mmc_card *card)
 	if (mmc_can_erase(card))
 		mmc_queue_setup_discard(mq->queue, card);
 
-	if (!mmc_dev(host)->dma_mask || !*mmc_dev(host)->dma_mask)
+	if (!dev->dma_mask || !*dev->dma_mask)
 		blk_queue_bounce_limit(mq->queue, BLK_BOUNCE_HIGH);
 	blk_queue_max_hw_sectors(mq->queue,
 		min(host->max_blk_count, host->max_req_size / 512));
 	if (host->can_dma_map_merge)
-		WARN(!blk_queue_can_use_dma_map_merging(mq->queue,
-							mmc_dev(host)),
+		WARN(!blk_queue_can_use_dma_map_merging(mq->queue, dev),
 		     "merging was advertised but not possible");
 	blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));
 
@@ -389,7 +389,10 @@ static void mmc_setup_queue(struct mmc_queue *mq, struct mmc_card *card)
 		blk_queue_max_segment_size(mq->queue,
 			round_down(host->max_seg_size, block_size));
 
-	dma_set_max_seg_size(mmc_dev(host), queue_max_segment_size(mq->queue));
+	if (!dev->dma_parms)
+		dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms),
+						GFP_KERNEL);
+	dma_set_max_seg_size(dev, queue_max_segment_size(mq->queue));
 
 	INIT_WORK(&mq->recovery_work, mmc_mq_recovery_handler);
 	INIT_WORK(&mq->complete_work, mmc_blk_mq_complete_work);
-- 
2.17.1

Re: [PATCH v4 14/14] mmc: queue: create dev->dma_parms before call dma_set_max_seg_size()

[Ulf Hansson]

+ Christoph, Arnd

On Wed, 19 Feb 2020 at 09:31, <haibo.chen@nxp.com> wrote:
>
> From: Haibo Chen <haibo.chen@nxp.com>
>
> To make dma_set_max_seg_size() work, need to create dev->dma_parms.
>
> Find this issue on i.MX8QM mek board, this platform config the
> max_segment_size to 65535, but this dma_set_max_seg_size do not
> actuall work, find sometimes the segment size is 65536, exceed
> the hardware max segment limitation, trigger ADMA error.

Sounds like we want something along the lines of this to be tagged for
stable as well.

>
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> ---
>  drivers/mmc/core/queue.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
> index 9edc08685e86..91094e56f7c9 100644
> --- a/drivers/mmc/core/queue.c
> +++ b/drivers/mmc/core/queue.c
> @@ -359,6 +359,7 @@ static const struct blk_mq_ops mmc_mq_ops = {
>  static void mmc_setup_queue(struct mmc_queue *mq, struct mmc_card *card)
>  {
>         struct mmc_host *host = card->host;
> +       struct device *dev = mmc_dev(host);
>         unsigned block_size = 512;
>
>         blk_queue_flag_set(QUEUE_FLAG_NONROT, mq->queue);
> @@ -366,13 +367,12 @@ static void mmc_setup_queue(struct mmc_queue *mq, struct mmc_card *card)
>         if (mmc_can_erase(card))
>                 mmc_queue_setup_discard(mq->queue, card);
>
> -       if (!mmc_dev(host)->dma_mask || !*mmc_dev(host)->dma_mask)
> +       if (!dev->dma_mask || !*dev->dma_mask)
>                 blk_queue_bounce_limit(mq->queue, BLK_BOUNCE_HIGH);
>         blk_queue_max_hw_sectors(mq->queue,
>                 min(host->max_blk_count, host->max_req_size / 512));
>         if (host->can_dma_map_merge)
> -               WARN(!blk_queue_can_use_dma_map_merging(mq->queue,
> -                                                       mmc_dev(host)),
> +               WARN(!blk_queue_can_use_dma_map_merging(mq->queue, dev),
>                      "merging was advertised but not possible");

If we plan to send this to stable, I rather make all the changes above
as a separate clean-up patch.

>         blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));
>
> @@ -389,7 +389,10 @@ static void mmc_setup_queue(struct mmc_queue *mq, struct mmc_card *card)
>                 blk_queue_max_segment_size(mq->queue,
>                         round_down(host->max_seg_size, block_size));
>
> -       dma_set_max_seg_size(mmc_dev(host), queue_max_segment_size(mq->queue));
> +       if (!dev->dma_parms)
> +               dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms),
> +                                               GFP_KERNEL);

I think this normally should be managed by the host driver, typically
while it runs its ->probe() function.

On the other hand, it seems like not many host drivers does this,
which makes me wonder a bit of how this is intended to work. I guess
we can always do the allocation here if it's not been done before.

Also, I see the drivers/mmc/host/alcor.c calls dma_set_max_seg_size(),
so I am guessing that calls fails as well. Hmm.


> +       dma_set_max_seg_size(dev, queue_max_segment_size(mq->queue));
>
>         INIT_WORK(&mq->recovery_work, mmc_mq_recovery_handler);
>         INIT_WORK(&mq->complete_work, mmc_blk_mq_complete_work);
> --
> 2.17.1
>

Kind regards
Uffe

Re: [PATCH v4 14/14] mmc: queue: create dev->dma_parms before call dma_set_max_seg_size()

[Christoph Hellwig]

On Wed, Mar 04, 2020 at 02:32:42PM +0100, Ulf Hansson wrote:
> + Christoph, Arnd
> 
> On Wed, 19 Feb 2020 at 09:31, <haibo.chen@nxp.com> wrote:
> >
> > From: Haibo Chen <haibo.chen@nxp.com>
> >
> > To make dma_set_max_seg_size() work, need to create dev->dma_parms.
> >
> > Find this issue on i.MX8QM mek board, this platform config the
> > max_segment_size to 65535, but this dma_set_max_seg_size do not
> > actuall work, find sometimes the segment size is 65536, exceed
> > the hardware max segment limitation, trigger ADMA error.
> 
> Sounds like we want something along the lines of this to be tagged for
> stable as well.

It really is not the job of the upper level driver to allocate the
dma params.  This should be done by the bus driver.

Re: [PATCH v4 14/14] mmc: queue: create dev->dma_parms before call dma_set_max_seg_size()

[Ulf Hansson]

On Wed, 4 Mar 2020 at 17:28, Christoph Hellwig <hch@lst.de> wrote:
>
> On Wed, Mar 04, 2020 at 02:32:42PM +0100, Ulf Hansson wrote:
> > + Christoph, Arnd
> >
> > On Wed, 19 Feb 2020 at 09:31, <haibo.chen@nxp.com> wrote:
> > >
> > > From: Haibo Chen <haibo.chen@nxp.com>
> > >
> > > To make dma_set_max_seg_size() work, need to create dev->dma_parms.
> > >
> > > Find this issue on i.MX8QM mek board, this platform config the
> > > max_segment_size to 65535, but this dma_set_max_seg_size do not
> > > actuall work, find sometimes the segment size is 65536, exceed
> > > the hardware max segment limitation, trigger ADMA error.
> >
> > Sounds like we want something along the lines of this to be tagged for
> > stable as well.
>
> It really is not the job of the upper level driver to allocate the
> dma params.  This should be done by the bus driver.

I agree that allocating the dma_parms doesn't belong in the mmc block
queue layer.

However, it seems silly that each an every mmc host driver needs to do
this themselves. So, perhaps we could manage this from
mmc_alloc_host(), which is a common interface that mmc host drivers
calls during initialization.

Would that make sense - or is there some reasons to why a host driver
really don't want to do the allocation you think?

Kind regards
Uffe

Re: [PATCH v4 14/14] mmc: queue: create dev->dma_parms before call dma_set_max_seg_size()

[Linus Walleij]

On Wed, Mar 4, 2020 at 5:28 PM Christoph Hellwig <hch@lst.de> wrote:
> On Wed, Mar 04, 2020 at 02:32:42PM +0100, Ulf Hansson wrote:
> > + Christoph, Arnd
> >
> > On Wed, 19 Feb 2020 at 09:31, <haibo.chen@nxp.com> wrote:
> > >
> > > From: Haibo Chen <haibo.chen@nxp.com>
> > >
> > > To make dma_set_max_seg_size() work, need to create dev->dma_parms.
> > >
> > > Find this issue on i.MX8QM mek board, this platform config the
> > > max_segment_size to 65535, but this dma_set_max_seg_size do not
> > > actuall work, find sometimes the segment size is 65536, exceed
> > > the hardware max segment limitation, trigger ADMA error.
> >
> > Sounds like we want something along the lines of this to be tagged for
> > stable as well.
>
> It really is not the job of the upper level driver to allocate the
> dma params.  This should be done by the bus driver.

I agree. This relates to my reply to Greg K-H recently:
https://lore.kernel.org/lkml/CACRpkdajhivkOkZ63v-hr7+6ObhTffYOx5uZP0P-MYvuVnyweA@mail.gmail.com/

The core of the problem is that drivers/of/platform.c is
very simple and has no idea what kind of bus it is populating
with devices from Device Tree. It is just guessing.

For example platform.c contains this:

        dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
        if (!dev->dev.dma_mask)
                dev->dev.dma_mask = &dev->dev.coherent_dma_mask;

And again part of the problem is that the device tree parser
mostly just create struct platform_device's on the platform bus
and has no real idea about any constraints on the bus where
it will eventually end up after the platform device is probed and
a new device on some other bus has been created.

Assuming that most problematic MMC controllers (in this
case) are probed from device tree (which I suspect,
but IIUC ACPI devices have the same problem),
my idea for a solution is to start to modify the platform.c
parser to be able to populate devices on some buses
directly, without using an intermediate platform device.

Today MMC isn't using a bus for the host controllers,
it is a class device, so I suppose the "real" solution would
be:

1. Move MMC hosts to a bus instead of a class named
   say mmc_bus. If this is OK with the userspace ABI.

2. Make everything necessary from the struct device
    inside struct mmc_host (today called "class_device")
    copied over from the parent and stop referencing
    the struct device *parent field in struct mmc_host
    directly for everything and its dog.

3. Make struct mmc_host and mmc_alloc_host() and
    mmc_add_host()
    be created and called directly from the device tree in some
    way (like a plug-in mechanism to of/platform.c)
    without creating a platform_device that just end
    up as the intermediary parent of the actual device
    in mmc_host.

4. Now we are creating the device directly on the right
   bus and so we can start thinking about filling in the
   right dma_params and dma_mask and dma_coherent_mask
   when instantiating the device on the mmc_bus
   rather than in every (platform_device) driver.

This is no trivial task because the kernel has gone down
the platform device for all stuff in the device tree for
a while. (Copy/paste same resoning for ACPI I'm afraid.)

I suppose people were looking for a simple solution to
this problem but I can only see a really complicated
solution.

Yours,
Linus Walleij

Re: [PATCH v4 14/14] mmc: queue: create dev->dma_parms before call dma_set_max_seg_size()

[Arnd Bergmann]

On Thu, Mar 5, 2020 at 3:30 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Mar 4, 2020 at 5:28 PM Christoph Hellwig <hch@lst.de> wrote:
> > On Wed, Mar 04, 2020 at 02:32:42PM +0100, Ulf Hansson wrote:
> > > + Christoph, Arnd

> Today MMC isn't using a bus for the host controllers,
> it is a class device, so I suppose the "real" solution would
> be:
>
> 1. Move MMC hosts to a bus instead of a class named
>    say mmc_bus. If this is OK with the userspace ABI.
>
> 2. Make everything necessary from the struct device
>     inside struct mmc_host (today called "class_device")
>     copied over from the parent and stop referencing
>     the struct device *parent field in struct mmc_host
>     directly for everything and its dog.

I don't think that copying the dma settings is a good solution
here. The software model that we have is that a bus can be a
dma master on its parent, which in turn is a master on
a parent bus, all the way up to the root of the tree. This is
not always the reality, but having an intermediate device
on a fake bus won't make it better.

The natural model is that an mmc host is a device on an
arbitrary dma capable bus (platform, pci, ...), and its
children are mmc devices with bus=&mmc_bus_type.

If that is not already the case, that is where I'd start
changing it to simplify the model.

        Arnd

Re: [PATCH v4 14/14] mmc: queue: create dev->dma_parms before call dma_set_max_seg_size()

[Linus Walleij]

On Thu, Mar 5, 2020 at 3:45 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Thu, Mar 5, 2020 at 3:30 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Wed, Mar 4, 2020 at 5:28 PM Christoph Hellwig <hch@lst.de> wrote:
> > > On Wed, Mar 04, 2020 at 02:32:42PM +0100, Ulf Hansson wrote:
> > > > + Christoph, Arnd
>
> > Today MMC isn't using a bus for the host controllers,
> > it is a class device, so I suppose the "real" solution would
> > be:
> >
> > 1. Move MMC hosts to a bus instead of a class named
> >    say mmc_bus. If this is OK with the userspace ABI.
> >
> > 2. Make everything necessary from the struct device
> >     inside struct mmc_host (today called "class_device")
> >     copied over from the parent and stop referencing
> >     the struct device *parent field in struct mmc_host
> >     directly for everything and its dog.
>
> I don't think that copying the dma settings is a good solution
> here. The software model that we have is that a bus can be a
> dma master on its parent, which in turn is a master on
> a parent bus, all the way up to the root of the tree. This is
> not always the reality, but having an intermediate device
> on a fake bus won't make it better.

Alas, that is what we have today, because the platform_bus
is pretty much fake.

My initial reasoning was because Greg has expressed that
he thinks the platform_bus is overused and more specific
buses should be used, so I outlined a plan to make
OF not use the platform bus.

(Which is maybe too hard.)

> The natural model is that an mmc host is a device on an
> arbitrary dma capable bus (platform, pci, ...), and its
> children are mmc devices with bus=&mmc_bus_type.
>
> If that is not already the case, that is where I'd start
> changing it to simplify the model.

The current way that the device tree parser uses the
platform bus is that it has no idea about what it is doing
when it comes to DMA, most of the time.

But maybe if we take a step back: can we make the
platform bus aware of what it is doing?

For example we have dma-ranges and similar properties
in the device tree, but subsystems call directly into
drivers/of/address.c to parse these out.

Would it be possible to tag nodes on the device tree
with the appropriate segment size and boundary
somehow also define the mask in the device tree in
a proper way, and have the platform.c parser actually
set this up correctly?

I am not aware that we have anything like this
currently, apart from the dma-ranges all DMA DT
bindings we have seems to pertain to DMA engines.

(I suppose the same apply for ACPI.)

Yours,
Linus Walleij

Re: [PATCH v4 14/14] mmc: queue: create dev->dma_parms before call dma_set_max_seg_size()

[Christoph Hellwig]

On Thu, Mar 05, 2020 at 11:52:04AM +0100, Ulf Hansson wrote:
> > It really is not the job of the upper level driver to allocate the
> > dma params.  This should be done by the bus driver.
> 
> I agree that allocating the dma_parms doesn't belong in the mmc block
> queue layer.
> 
> However, it seems silly that each an every mmc host driver needs to do
> this themselves. So, perhaps we could manage this from
> mmc_alloc_host(), which is a common interface that mmc host drivers
> calls during initialization.

Neither mmc drivers nor the mmc core should allocate dma_params ever.
This should be once in their bus providers.

Re: [PATCH v4 14/14] mmc: queue: create dev->dma_parms before call dma_set_max_seg_size()

[Christoph Hellwig]

On Thu, Mar 05, 2020 at 03:29:55PM +0100, Linus Walleij wrote:
> I agree. This relates to my reply to Greg K-H recently:
> https://lore.kernel.org/lkml/CACRpkdajhivkOkZ63v-hr7+6ObhTffYOx5uZP0P-MYvuVnyweA@mail.gmail.com/
> 
> The core of the problem is that drivers/of/platform.c is
> very simple and has no idea what kind of bus it is populating
> with devices from Device Tree. It is just guessing.
> 
> For example platform.c contains this:
> 
>         dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
>         if (!dev->dev.dma_mask)
>                 dev->dev.dma_mask = &dev->dev.coherent_dma_mask;
> 
> And again part of the problem is that the device tree parser
> mostly just create struct platform_device's on the platform bus
> and has no real idea about any constraints on the bus where
> it will eventually end up after the platform device is probed and
> a new device on some other bus has been created.

the plaform device code should allocate the dma_params for every
device.  It should not set any limits in that, that is up to the
host drivers.

Re: [PATCH v4 14/14] mmc: queue: create dev->dma_parms before call dma_set_max_seg_size()

[Arnd Bergmann]

On Thu, Mar 5, 2020 at 4:04 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Mar 5, 2020 at 3:45 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Thu, Mar 5, 2020 at 3:30 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> > > On Wed, Mar 4, 2020 at 5:28 PM Christoph Hellwig <hch@lst.de> wrote:
> > > > On Wed, Mar 04, 2020 at 02:32:42PM +0100, Ulf Hansson wrote:
> > > > > + Christoph, Arnd
> >
> > > Today MMC isn't using a bus for the host controllers,
> > > it is a class device, so I suppose the "real" solution would
> > > be:
> > >
> > > 1. Move MMC hosts to a bus instead of a class named
> > >    say mmc_bus. If this is OK with the userspace ABI.
> > >
> > > 2. Make everything necessary from the struct device
> > >     inside struct mmc_host (today called "class_device")
> > >     copied over from the parent and stop referencing
> > >     the struct device *parent field in struct mmc_host
> > >     directly for everything and its dog.
> >
> > I don't think that copying the dma settings is a good solution
> > here. The software model that we have is that a bus can be a
> > dma master on its parent, which in turn is a master on
> > a parent bus, all the way up to the root of the tree. This is
> > not always the reality, but having an intermediate device
> > on a fake bus won't make it better.
>
> Alas, that is what we have today, because the platform_bus
> is pretty much fake.

Note the difference between a fake bus instance and a fake bus
type: the problem in certain drivers is making up a bus instance
for internal abstractions, and this causes problems with managing
DMA.

The 'platform_bus' bus_type being fake is a completely
unrelated problem, this is mostly an artifact of not being
able to model the actual buses because we don't even
know if something is an AXI or AHB bus or something else
in many cases, so we fall back to using platform_bus for
anything device that has mmio, dma, interrupts etc
but doesn't have a discoverable bus type already.

> My initial reasoning was because Greg has expressed that
> he thinks the platform_bus is overused and more specific
> buses should be used, so I outlined a plan to make
> OF not use the platform bus.
>
> (Which is maybe too hard.)

Traditionally we had a separate bus type for devices
probed from DT, but turned out to be a huge hassle because
of all the drivers that were also probed from board files
then needed to register two separate device_driver
structures.


      Arnd

Re: [PATCH v4 14/14] mmc: queue: create dev->dma_parms before call dma_set_max_seg_size()

[Ulf Hansson]

On Thu, 5 Mar 2020 at 16:23, Christoph Hellwig <hch@lst.de> wrote:
>
> On Thu, Mar 05, 2020 at 03:29:55PM +0100, Linus Walleij wrote:
> > I agree. This relates to my reply to Greg K-H recently:
> > https://lore.kernel.org/lkml/CACRpkdajhivkOkZ63v-hr7+6ObhTffYOx5uZP0P-MYvuVnyweA@mail.gmail.com/
> >
> > The core of the problem is that drivers/of/platform.c is
> > very simple and has no idea what kind of bus it is populating
> > with devices from Device Tree. It is just guessing.
> >
> > For example platform.c contains this:
> >
> >         dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> >         if (!dev->dev.dma_mask)
> >                 dev->dev.dma_mask = &dev->dev.coherent_dma_mask;
> >
> > And again part of the problem is that the device tree parser
> > mostly just create struct platform_device's on the platform bus
> > and has no real idea about any constraints on the bus where
> > it will eventually end up after the platform device is probed and
> > a new device on some other bus has been created.
>
> the plaform device code should allocate the dma_params for every
> device.  It should not set any limits in that, that is up to the
> host drivers.

That makes sense to me, but we also have other bus types to consider,
like the amba bus for example.

So, in principle, those buses that may get DMA capable devices
attached, needs to do the allocation, that's what you are proposing?

Kind regards
Uffe

Re: [PATCH v4 14/14] mmc: queue: create dev->dma_parms before call dma_set_max_seg_size()

[Christoph Hellwig]

On Fri, Mar 06, 2020 at 03:02:09PM +0100, Ulf Hansson wrote:
> That makes sense to me, but we also have other bus types to consider,
> like the amba bus for example.
> 
> So, in principle, those buses that may get DMA capable devices
> attached, needs to do the allocation, that's what you are proposing?

Having a dma_params allocated doesn't mean all devices are dma capable,
for example there are PCI devices that aren't as well.

Re: [PATCH v4 14/14] mmc: queue: create dev->dma_parms before call dma_set_max_seg_size()

[Ulf Hansson]

+ Ludovic Barre

On Fri, 6 Mar 2020 at 15:19, Christoph Hellwig <hch@lst.de> wrote:
>
> On Fri, Mar 06, 2020 at 03:02:09PM +0100, Ulf Hansson wrote:
> > That makes sense to me, but we also have other bus types to consider,
> > like the amba bus for example.
> >
> > So, in principle, those buses that may get DMA capable devices
> > attached, needs to do the allocation, that's what you are proposing?
>
> Having a dma_params allocated doesn't mean all devices are dma capable,
> for example there are PCI devices that aren't as well.

Alright. I interpret the silence as that someone (me) should send a
patch, to see if Greg is fine with it.

Please tell me, if you have other ideas of how to fix this.

Kind regards
Uffe