Skip to contentSkip to conversation

mmc: dw_mmc: fix DW MMC cores with 32-bit bus on 64-bit Linux systems

PATCH Superseded Sergey Lisov 2 participants 9 messages Mar 12, 2023

Subsystems (4)
Device tree bindings Arm Samsung Exynos MMC, SD and SDIO Synopsys DesignWare MMC/SD/SDIO driver

v3 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Sergey Lisov

author
#0 lore raw

v3 cover mmc: dw_mmc: fix DW MMC cores with 32-bit bus on 64-bit Linux systems

DesignWare MMC cores have a configurable data bus width of either 16, 32, or 64 bytes. It is possible, and some vendors actually do it, to ship a DW MMC core configured for 32-bit data bus within a 64-bit SoC. In this case the kernel will attempt 64-bit (readq) accesses to certain 64-bit MMIO registers, while the core will expect pairs of 32-bit accesses.

It seems that currently the only register for which the kernel performs 64-bit accesses is the FIFO. The symptom is that the DW MMC core never receives a read on the second half of the register, does not register the datum as being read, and thus not advancing its internal FIFO pointer, breaking further reads. It also seems that this FIFO is only used for small (less than 16 bytes) transfers, which probably means that only some SDIO cards are affected.

Changelog:

v3:
- removed "fifo-access-32bit" devicetree property
- added "samsung,exynos78xx-dw-mshc" compatible string
- added "samsung,exynos78xx-dw-mshc-smu" compatible string

v2:
- added commit messages

v1:
- added "fifo-access-32bit" devicetree property
- added DW_MMC_QUIRK_FIFO64_32
- added new dw_mci_{pull,push}_data* variants (...-data64_32)

Sergey Lisov (2):
dt-bindings: exynos-dw-mshc-common: add exynos78xx variants
mmc: dw_mmc: add an option to force 32-bit access to 64-bit FIFO

 .../bindings/mmc/samsung,exynos-dw-mshc.yaml  |   2 +
 arch/arm64/boot/dts/exynos/exynos7885.dtsi    |   2 +-
 drivers/mmc/host/dw_mmc-exynos.c              |  41 +++++-
 drivers/mmc/host/dw_mmc.c                     | 122 +++++++++++++++++-
 drivers/mmc/host/dw_mmc.h                     |   2 +
 5 files changed, 165 insertions(+), 4 deletions(-)
Signature · 1 line
2.38.3
author
#1 lore raw

PATCH v3 1/2 dt-bindings: exynos-dw-mshc-common: add exynos78xx variants

Some Samsung Exynos boards using the arm64 architecture have DW MMC controllers configured for a 32-bit data bus but a 64-bit FIFO. On these systems the 64-bit FIFO registers must be accessed in two 32-bit halves.

Add two new compatible strings, "samsung,exynos78xx-dw-mshc" and "samsung,exynos78xx-dw-mshc-smu" respectively, to denote exynos78xx boards that need this quirk. But it's very possible that all "samsung,exynos7-dw-mshc" boards are actually affected.

---
 .../devicetree/bindings/mmc/samsung,exynos-dw-mshc.yaml         | 2 ++
 arch/arm64/boot/dts/exynos/exynos7885.dtsi                      | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/mmc/samsung,exynos-dw-mshc.yaml +2 -0
index fdaa18481..a72a67792 100644
@@ -22,6 +22,8 @@ properties:
       - samsung,exynos5420-dw-mshc-smu
       - samsung,exynos7-dw-mshc
       - samsung,exynos7-dw-mshc-smu
+      - samsung,exynos78xx-dw-mshc
+      - samsung,exynos78xx-dw-mshc-smu
       - axis,artpec8-dw-mshc
 
   reg:
arch/arm64/boot/dts/exynos/exynos7885.dtsi +1 -1
index 23c2e0bb0..4b94ac9da 100644
@@ -294,7 +294,7 @@ pmu_system_controller: system-controller@11c80000 {
 		};
 
 		mmc_0: mmc@13500000 {
-			compatible = "samsung,exynos7-dw-mshc-smu";
+			compatible = "samsung,exynos78xx-dw-mshc-smu";
 			reg = <0x13500000 0x2000>;
 			interrupts = <GIC_SPI 146 IRQ_TYPE_LEVEL_HIGH>;
 			#address-cells = <1>;
Signature · 1 line
2.38.3
#2 lore raw

Re: [PATCH v3 1/2] dt-bindings: exynos-dw-mshc-common: add exynos78xx variants

Quoted 11 lines · Sergey Lisov

Some Samsung Exynos boards using the arm64 architecture have DW MMC controllers configured for a 32-bit data bus but a 64-bit FIFO. On these systems the 64-bit FIFO registers must be accessed in two 32-bit halves.

Add two new compatible strings, "samsung,exynos78xx-dw-mshc" and "samsung,exynos78xx-dw-mshc-smu" respectively, to denote exynos78xx boards that need this quirk. But it's very possible that all "samsung,exynos7-dw-mshc" boards are actually affected.

---
 .../devicetree/bindings/mmc/samsung,exynos-dw-mshc.yaml         | 2 ++
 arch/arm64/boot/dts/exynos/exynos7885.dtsi                      | 2 +-

Bindings and DTS (and driver) are always separate.

Documentation/devicetree/bindings/mmc/samsung,exynos-dw-mshc.yaml properties: · line 26

Quoted 12 lines
 2 files changed, 3 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/mmc/samsung,exynos-dw-mshc.yaml +2 -0
index fdaa18481..a72a67792 100644
@@ -22,6 +22,8 @@ properties:
       - samsung,exynos5420-dw-mshc-smu
       - samsung,exynos7-dw-mshc
       - samsung,exynos7-dw-mshc-smu
+      - samsung,exynos78xx-dw-mshc
+      - samsung,exynos78xx-dw-mshc-smu

Compatibles must be specific.

There is also no fallback, but I assume that's intentional - you say these are not compatible?

arch/arm64/boot/dts/exynos/exynos7885.dtsi pmu_system_controller: system-controller@11c80000 { · line 297

Quoted 13 lines
Documentation/devicetree/bindings/mmc/samsung,exynos-dw-mshc.yaml +0 -0
       - axis,artpec8-dw-mshc
 
   reg:
arch/arm64/boot/dts/exynos/exynos7885.dtsi +1 -1
index 23c2e0bb0..4b94ac9da 100644
@@ -294,7 +294,7 @@ pmu_system_controller: system-controller@11c80000 {
 		};
 
 		mmc_0: mmc@13500000 {
-			compatible = "samsung,exynos7-dw-mshc-smu";
+			compatible = "samsung,exynos78xx-dw-mshc-smu";

That's non-bisectable change (also breaking other users of DTS), so you need to explain in commit msg rationale - devices were never compatible and using exynos7 does not work in certain cases.

Best regards,
Krzysztof

author
#3 lore raw

Re: [PATCH v3 1/2] dt-bindings: exynos-dw-mshc-common: add exynos78xx variants

Quoted 1 line

Bindings and DTS (and driver) are always separate.

Okay, will split the patch.

Quoted 1 line

Compatibles must be specific.

No, this way you'd have tons of identical compatibles that only differ in the exynosXXXX digits, and are functionally equivalent.

Quoted 3 lines

That's non-bisectable change (also breaking other users of DTS), so you need to explain in commit msg rationale - devices were never compatible and using exynos7 does not work in certain cases.

Valid point.

author
#4 lore raw

Re: [PATCH v3 1/2] dt-bindings: exynos-dw-mshc-common: add exynos78xx variants

In reply to Krzysztof Kozlowski
Quoted 1 line

Bindings and DTS (and driver) are always separate.

Okay, will split the patch.

Quoted 1 line

Compatibles must be specific.

No, this way you'd have tons of identical compatibles that only differ in the exynosXXXX digits, and are functionally equivalent.

Quoted 3 lines

That's non-bisectable change (also breaking other users of DTS), so you need to explain in commit msg rationale - devices were never compatible and using exynos7 does not work in certain cases.

Valid point.

author
#5 lore raw

Re: [PATCH v3 1/2] dt-bindings: exynos-dw-mshc-common: add exynos78xx variants

In reply to Krzysztof Kozlowski
Quoted 1 line

Bindings and DTS (and driver) are always separate.

Okay, will split the patch.

Quoted 1 line

Compatibles must be specific.

No, this way you'd have tons of identical compatibles that only differ in the exynosXXXX digits, and are functionally equivalent.

Quoted 3 lines

That's non-bisectable change (also breaking other users of DTS), so you need to explain in commit msg rationale - devices were never compatible and using exynos7 does not work in certain cases.

Valid point.

author
#6 lore raw

Re: [PATCH v3 1/2] dt-bindings: exynos-dw-mshc-common: add exynos78xx variants

In reply to Krzysztof Kozlowski
Quoted 1 line

Bindings and DTS (and driver) are always separate.

Okay, will split the patch.

Quoted 1 line

Compatibles must be specific.

No, this way you'd have tons of identical compatibles that only differ in the exynosXXXX digits, and are functionally equivalent.

Quoted 3 lines

That's non-bisectable change (also breaking other users of DTS), so you need to explain in commit msg rationale - devices were never compatible and using exynos7 does not work in certain cases.

Valid point.

author
#7 lore raw

Re: [PATCH v3 1/2] dt-bindings: exynos-dw-mshc-common: add exynos78xx variants

In reply to Krzysztof Kozlowski
Quoted 1 line

Bindings and DTS (and driver) are always separate.

Okay, will split the patch.

Quoted 1 line

Compatibles must be specific.

No, this way you'd have tons of identical compatibles that only differ in the exynosXXXX digits, and are functionally equivalent.

Quoted 3 lines

That's non-bisectable change (also breaking other users of DTS), so you need to explain in commit msg rationale - devices were never compatible and using exynos7 does not work in certain cases.

Valid point.

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

author
#8 lore raw

PATCH v3 2/2 mmc: dw_mmc: add an option to force 32-bit access to 64-bit FIFO

In reply to Sergey Lisov

Some Samsung Exynos boards using the arm64 architecture have DW MMC controllers configured for a 32-bit data bus but a 64-bit FIFO. On these systems the 64-bit FIFO registers must be accessed in two 32-bit halves.

---
 drivers/mmc/host/dw_mmc-exynos.c |  41 ++++++++++-
 drivers/mmc/host/dw_mmc.c        | 122 ++++++++++++++++++++++++++++++-
 drivers/mmc/host/dw_mmc.h        |   2 +
 3 files changed, 162 insertions(+), 3 deletions(-)
drivers/mmc/host/dw_mmc-exynos.c +40 -1
index 9f20ac524..768774f22 100644
@@ -28,6 +28,8 @@ enum dw_mci_exynos_type {
 	DW_MCI_TYPE_EXYNOS5420_SMU,
 	DW_MCI_TYPE_EXYNOS7,
 	DW_MCI_TYPE_EXYNOS7_SMU,
+	DW_MCI_TYPE_EXYNOS78XX,
+	DW_MCI_TYPE_EXYNOS78XX_SMU,
 	DW_MCI_TYPE_ARTPEC8,
 };
 

@@ -70,6 +72,12 @@ static struct dw_mci_exynos_compatible {
 	}, {
 		.compatible	= "samsung,exynos7-dw-mshc-smu",
 		.ctrl_type	= DW_MCI_TYPE_EXYNOS7_SMU,
+	}, {
+		.compatible	= "samsung,exynos78xx-dw-mshc",
+		.ctrl_type	= DW_MCI_TYPE_EXYNOS78XX,
+	}, {
+		.compatible	= "samsung,exynos78xx-dw-mshc-smu",
+		.ctrl_type	= DW_MCI_TYPE_EXYNOS78XX_SMU,
 	}, {
 		.compatible	= "axis,artpec8-dw-mshc",
 		.ctrl_type	= DW_MCI_TYPE_ARTPEC8,

@@ -86,6 +94,8 @@ static inline u8 dw_mci_exynos_get_ciu_div(struct dw_mci *host)
 		return EXYNOS4210_FIXED_CIU_CLK_DIV;
 	else if (priv->ctrl_type == DW_MCI_TYPE_EXYNOS7 ||
 			priv->ctrl_type == DW_MCI_TYPE_EXYNOS7_SMU ||
+			priv->ctrl_type == DW_MCI_TYPE_EXYNOS78XX ||
+			priv->ctrl_type == DW_MCI_TYPE_EXYNOS78XX_SMU ||
 			priv->ctrl_type == DW_MCI_TYPE_ARTPEC8)
 		return SDMMC_CLKSEL_GET_DIV(mci_readl(host, CLKSEL64)) + 1;
 	else

@@ -101,7 +111,8 @@ static void dw_mci_exynos_config_smu(struct dw_mci *host)
 	 * set for non-ecryption mode at this time.
 	 */
 	if (priv->ctrl_type == DW_MCI_TYPE_EXYNOS5420_SMU ||
-		priv->ctrl_type == DW_MCI_TYPE_EXYNOS7_SMU) {
+		priv->ctrl_type == DW_MCI_TYPE_EXYNOS7_SMU ||
+		priv->ctrl_type == DW_MCI_TYPE_EXYNOS78XX_SMU) {
 		mci_writel(host, MPSBEGIN0, 0);
 		mci_writel(host, MPSEND0, SDMMC_ENDING_SEC_NR_MAX);
 		mci_writel(host, MPSCTRL0, SDMMC_MPSCTRL_SECURE_WRITE_BIT |

@@ -127,6 +138,12 @@ static int dw_mci_exynos_priv_init(struct dw_mci *host)
 				DQS_CTRL_GET_RD_DELAY(priv->saved_strobe_ctrl);
 	}
 
+	if (priv->ctrl_type == DW_MCI_TYPE_EXYNOS78XX ||
+		priv->ctrl_type == DW_MCI_TYPE_EXYNOS78XX_SMU) {
+		/* Quirk needed for certain Exynos SoCs */
+		host->quirks |= DW_MMC_QUIRK_FIFO64_32;
+	}
+
 	if (priv->ctrl_type == DW_MCI_TYPE_ARTPEC8) {
 		/* Quirk needed for the ARTPEC-8 SoC */
 		host->quirks |= DW_MMC_QUIRK_EXTENDED_TMOUT;

@@ -144,6 +161,8 @@ static void dw_mci_exynos_set_clksel_timing(struct dw_mci *host, u32 timing)
 
 	if (priv->ctrl_type == DW_MCI_TYPE_EXYNOS7 ||
 		priv->ctrl_type == DW_MCI_TYPE_EXYNOS7_SMU ||
+		priv->ctrl_type == DW_MCI_TYPE_EXYNOS78XX ||
+		priv->ctrl_type == DW_MCI_TYPE_EXYNOS78XX_SMU ||
 		priv->ctrl_type == DW_MCI_TYPE_ARTPEC8)
 		clksel = mci_readl(host, CLKSEL64);
 	else

@@ -153,6 +172,8 @@ static void dw_mci_exynos_set_clksel_timing(struct dw_mci *host, u32 timing)
 
 	if (priv->ctrl_type == DW_MCI_TYPE_EXYNOS7 ||
 		priv->ctrl_type == DW_MCI_TYPE_EXYNOS7_SMU ||
+		priv->ctrl_type == DW_MCI_TYPE_EXYNOS78XX ||
+		priv->ctrl_type == DW_MCI_TYPE_EXYNOS78XX_SMU ||
 		priv->ctrl_type == DW_MCI_TYPE_ARTPEC8)
 		mci_writel(host, CLKSEL64, clksel);
 	else

@@ -223,6 +244,8 @@ static int dw_mci_exynos_resume_noirq(struct device *dev)
 
 	if (priv->ctrl_type == DW_MCI_TYPE_EXYNOS7 ||
 		priv->ctrl_type == DW_MCI_TYPE_EXYNOS7_SMU ||
+		priv->ctrl_type == DW_MCI_TYPE_EXYNOS78XX ||
+		priv->ctrl_type == DW_MCI_TYPE_EXYNOS78XX_SMU ||
 		priv->ctrl_type == DW_MCI_TYPE_ARTPEC8)
 		clksel = mci_readl(host, CLKSEL64);
 	else

@@ -231,6 +254,8 @@ static int dw_mci_exynos_resume_noirq(struct device *dev)
 	if (clksel & SDMMC_CLKSEL_WAKEUP_INT) {
 		if (priv->ctrl_type == DW_MCI_TYPE_EXYNOS7 ||
 			priv->ctrl_type == DW_MCI_TYPE_EXYNOS7_SMU ||
+			priv->ctrl_type == DW_MCI_TYPE_EXYNOS78XX ||
+			priv->ctrl_type == DW_MCI_TYPE_EXYNOS78XX_SMU ||
 			priv->ctrl_type == DW_MCI_TYPE_ARTPEC8)
 			mci_writel(host, CLKSEL64, clksel);
 		else

@@ -410,6 +435,8 @@ static inline u8 dw_mci_exynos_get_clksmpl(struct dw_mci *host)
 
 	if (priv->ctrl_type == DW_MCI_TYPE_EXYNOS7 ||
 		priv->ctrl_type == DW_MCI_TYPE_EXYNOS7_SMU ||
+		priv->ctrl_type == DW_MCI_TYPE_EXYNOS78XX ||
+		priv->ctrl_type == DW_MCI_TYPE_EXYNOS78XX_SMU ||
 		priv->ctrl_type == DW_MCI_TYPE_ARTPEC8)
 		return SDMMC_CLKSEL_CCLK_SAMPLE(mci_readl(host, CLKSEL64));
 	else

@@ -423,6 +450,8 @@ static inline void dw_mci_exynos_set_clksmpl(struct dw_mci *host, u8 sample)
 
 	if (priv->ctrl_type == DW_MCI_TYPE_EXYNOS7 ||
 		priv->ctrl_type == DW_MCI_TYPE_EXYNOS7_SMU ||
+		priv->ctrl_type == DW_MCI_TYPE_EXYNOS78XX ||
+		priv->ctrl_type == DW_MCI_TYPE_EXYNOS78XX_SMU ||
 		priv->ctrl_type == DW_MCI_TYPE_ARTPEC8)
 		clksel = mci_readl(host, CLKSEL64);
 	else

@@ -430,6 +459,8 @@ static inline void dw_mci_exynos_set_clksmpl(struct dw_mci *host, u8 sample)
 	clksel = SDMMC_CLKSEL_UP_SAMPLE(clksel, sample);
 	if (priv->ctrl_type == DW_MCI_TYPE_EXYNOS7 ||
 		priv->ctrl_type == DW_MCI_TYPE_EXYNOS7_SMU ||
+		priv->ctrl_type == DW_MCI_TYPE_EXYNOS78XX ||
+		priv->ctrl_type == DW_MCI_TYPE_EXYNOS78XX_SMU ||
 		priv->ctrl_type == DW_MCI_TYPE_ARTPEC8)
 		mci_writel(host, CLKSEL64, clksel);
 	else

@@ -444,6 +475,8 @@ static inline u8 dw_mci_exynos_move_next_clksmpl(struct dw_mci *host)
 
 	if (priv->ctrl_type == DW_MCI_TYPE_EXYNOS7 ||
 		priv->ctrl_type == DW_MCI_TYPE_EXYNOS7_SMU ||
+		priv->ctrl_type == DW_MCI_TYPE_EXYNOS78XX ||
+		priv->ctrl_type == DW_MCI_TYPE_EXYNOS78XX_SMU ||
 		priv->ctrl_type == DW_MCI_TYPE_ARTPEC8)
 		clksel = mci_readl(host, CLKSEL64);
 	else

@@ -454,6 +487,8 @@ static inline u8 dw_mci_exynos_move_next_clksmpl(struct dw_mci *host)
 
 	if (priv->ctrl_type == DW_MCI_TYPE_EXYNOS7 ||
 		priv->ctrl_type == DW_MCI_TYPE_EXYNOS7_SMU ||
+		priv->ctrl_type == DW_MCI_TYPE_EXYNOS78XX ||
+		priv->ctrl_type == DW_MCI_TYPE_EXYNOS78XX_SMU ||
 		priv->ctrl_type == DW_MCI_TYPE_ARTPEC8)
 		mci_writel(host, CLKSEL64, clksel);
 	else

@@ -633,6 +668,10 @@ static const struct of_device_id dw_mci_exynos_match[] = {
 			.data = &exynos_drv_data, },
 	{ .compatible = "samsung,exynos7-dw-mshc-smu",
 			.data = &exynos_drv_data, },
+	{ .compatible = "samsung,exynos78xx-dw-mshc",
+			.data = &exynos_drv_data, },
+	{ .compatible = "samsung,exynos78xx-dw-mshc-smu",
+			.data = &exynos_drv_data, },
 	{ .compatible = "axis,artpec8-dw-mshc",
 			.data = &artpec_drv_data, },
 	{},
drivers/mmc/host/dw_mmc.c +120 -2

134 lines not on this page · raw message

drivers/mmc/host/dw_mmc.h +2 -0
index 4ed81f94f..edd642b92 100644
@@ -280,6 +280,8 @@ struct dw_mci_board {
 
 /* Support for longer data read timeout */
 #define DW_MMC_QUIRK_EXTENDED_TMOUT            BIT(0)
+/* Force 32-bit access to the FIFO */
+#define DW_MMC_QUIRK_FIFO64_32                 BIT(1)
 
 #define DW_MMC_240A		0x240a
 #define DW_MMC_280A		0x280a
Signature · 1 line
2.38.3