[PATCH v3 0/2] mmc: dw_mmc: fix DW MMC cores with 32-bit bus on 64-bit Linux systems

linux-samsung-soc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH v3 0/2] mmc: dw_mmc: fix DW MMC cores with 32-bit bus on 64-bit Linux systems
@ 2023-03-12 13:03 Sergey Lisov
  2023-03-12 13:03 ` [PATCH v3 2/2] mmc: dw_mmc: add an option to force 32-bit access to 64-bit FIFO Sergey Lisov
  2023-03-12 13:03 ` [PATCH v3 1/2] dt-bindings: exynos-dw-mshc-common: add exynos78xx variants Sergey Lisov
  0 siblings, 2 replies; 8+ messages in thread
From: Sergey Lisov @ 2023-03-12 13:03 UTC (permalink / raw)
  To: Ulf Hansson, Rob Herring, Krzysztof Kozlowski, Alim Akhtar,
	Jaehoon Chung
  Cc: linux-mmc, devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel

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(-)

-- 
2.38.3

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v3 2/2] mmc: dw_mmc: add an option to force 32-bit access to 64-bit FIFO
  2023-03-12 13:03 [PATCH v3 0/2] mmc: dw_mmc: fix DW MMC cores with 32-bit bus on 64-bit Linux systems Sergey Lisov
@ 2023-03-12 13:03 ` Sergey Lisov
  2023-03-12 13:03 ` [PATCH v3 1/2] dt-bindings: exynos-dw-mshc-common: add exynos78xx variants Sergey Lisov
  1 sibling, 0 replies; 8+ messages in thread
From: Sergey Lisov @ 2023-03-12 13:03 UTC (permalink / raw)
  To: Ulf Hansson, Rob Herring, Krzysztof Kozlowski, Alim Akhtar,
	Jaehoon Chung
  Cc: linux-mmc, devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel

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(-)

diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c
index 9f20ac524..768774f22 100644
--- a/drivers/mmc/host/dw_mmc-exynos.c
+++ b/drivers/mmc/host/dw_mmc-exynos.c
@@ -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, },
 	{},
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 581614196..9fe816c61 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2575,6 +2575,119 @@ static void dw_mci_pull_data64(struct dw_mci *host, void *buf, int cnt)
 	}
 }
 
+/*
+  Some dw_mmc devices have 64-bit FIFOs, but expect them to be
+  accessed using two 32-bit accesses. If such controller is used
+  with a 64-bit kernel, this has to be done explicitly.
+
+  XXX: Is this issue specific to Exynos7?
+*/
+
+static inline uint64_t mci_fifo_readq_32(void __iomem *addr)
+{
+	uint64_t ans;
+	uint32_t proxy[2];
+
+	proxy[0] = mci_fifo_readl(addr);
+	proxy[1] = mci_fifo_readl(addr+4);
+	memcpy(&ans, proxy, 8);
+	return ans;
+}
+
+static inline void mci_fifo_writeq_32(void __iomem *addr, uint64_t value)
+{
+	uint32_t proxy[2];
+
+	memcpy(proxy, &value, 8);
+	mci_fifo_writel(addr, proxy[0]);
+	mci_fifo_writel(addr+4, proxy[1]);
+}
+
+static void dw_mci_push_data64_32(struct dw_mci *host, void *buf, int cnt)
+{
+	struct mmc_data *data = host->data;
+	int init_cnt = cnt;
+
+	/* try and push anything in the part_buf */
+	if (unlikely(host->part_buf_count)) {
+		int len = dw_mci_push_part_bytes(host, buf, cnt);
+
+		buf += len;
+		cnt -= len;
+
+		if (host->part_buf_count == 8) {
+			mci_fifo_writeq_32(host->fifo_reg, host->part_buf);
+			host->part_buf_count = 0;
+		}
+	}
+#ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
+	if (unlikely((unsigned long)buf & 0x7)) {
+		while (cnt >= 8) {
+			u64 aligned_buf[16];
+			int len = min(cnt & -8, (int)sizeof(aligned_buf));
+			int items = len >> 3;
+			int i;
+			/* memcpy from input buffer into aligned buffer */
+			memcpy(aligned_buf, buf, len);
+			buf += len;
+			cnt -= len;
+			/* push data from aligned buffer into fifo */
+			for (i = 0; i < items; ++i)
+				mci_fifo_writeq_32(host->fifo_reg, aligned_buf[i]);
+		}
+	} else
+#endif
+	{
+		u64 *pdata = buf;
+
+		for (; cnt >= 8; cnt -= 8)
+			mci_fifo_writeq_32(host->fifo_reg, *pdata++);
+		buf = pdata;
+	}
+	/* put anything remaining in the part_buf */
+	if (cnt) {
+		dw_mci_set_part_bytes(host, buf, cnt);
+		/* Push data if we have reached the expected data length */
+		if ((data->bytes_xfered + init_cnt) ==
+		    (data->blksz * data->blocks))
+			mci_fifo_writeq_32(host->fifo_reg, host->part_buf);
+	}
+}
+
+static void dw_mci_pull_data64_32(struct dw_mci *host, void *buf, int cnt)
+{
+#ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
+	if (unlikely((unsigned long)buf & 0x7)) {
+		while (cnt >= 8) {
+			/* pull data from fifo into aligned buffer */
+			u64 aligned_buf[16];
+			int len = min(cnt & -8, (int)sizeof(aligned_buf));
+			int items = len >> 3;
+			int i;
+
+			for (i = 0; i < items; ++i)
+				aligned_buf[i] = mci_fifo_readq_32(host->fifo_reg);
+
+			/* memcpy from aligned buffer into output buffer */
+			memcpy(buf, aligned_buf, len);
+			buf += len;
+			cnt -= len;
+		}
+	} else
+#endif
+	{
+		u64 *pdata = buf;
+
+		for (; cnt >= 8; cnt -= 8)
+			*pdata++ = mci_fifo_readq_32(host->fifo_reg);
+		buf = pdata;
+	}
+	if (cnt) {
+		host->part_buf = mci_fifo_readq_32(host->fifo_reg);
+		dw_mci_pull_final_bytes(host, buf, cnt);
+	}
+}
+
 static void dw_mci_pull_data(struct dw_mci *host, void *buf, int cnt)
 {
 	int len;
@@ -3367,8 +3480,13 @@ int dw_mci_probe(struct dw_mci *host)
 		width = 16;
 		host->data_shift = 1;
 	} else if (i == 2) {
-		host->push_data = dw_mci_push_data64;
-		host->pull_data = dw_mci_pull_data64;
+		if ((host->quirks & DW_MMC_QUIRK_FIFO64_32)) {
+			host->push_data = dw_mci_push_data64_32;
+			host->pull_data = dw_mci_pull_data64_32;
+		} else {
+			host->push_data = dw_mci_push_data64;
+			host->pull_data = dw_mci_pull_data64;
+		}
 		width = 64;
 		host->data_shift = 3;
 	} else {
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index 4ed81f94f..edd642b92 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -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
-- 
2.38.3



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v3 1/2] dt-bindings: exynos-dw-mshc-common: add exynos78xx variants
  2023-03-12 13:03 [PATCH v3 0/2] mmc: dw_mmc: fix DW MMC cores with 32-bit bus on 64-bit Linux systems Sergey Lisov
  2023-03-12 13:03 ` [PATCH v3 2/2] mmc: dw_mmc: add an option to force 32-bit access to 64-bit FIFO Sergey Lisov
@ 2023-03-12 13:03 ` Sergey Lisov
  2023-03-12 13:35   ` Krzysztof Kozlowski
  1 sibling, 1 reply; 8+ messages in thread
From: Sergey Lisov @ 2023-03-12 13:03 UTC (permalink / raw)
  To: Ulf Hansson, Rob Herring, Krzysztof Kozlowski, Alim Akhtar,
	Jaehoon Chung
  Cc: linux-mmc, devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel

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(-)

diff --git a/Documentation/devicetree/bindings/mmc/samsung,exynos-dw-mshc.yaml b/Documentation/devicetree/bindings/mmc/samsung,exynos-dw-mshc.yaml
index fdaa18481..a72a67792 100644
--- a/Documentation/devicetree/bindings/mmc/samsung,exynos-dw-mshc.yaml
+++ b/Documentation/devicetree/bindings/mmc/samsung,exynos-dw-mshc.yaml
@@ -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:
diff --git a/arch/arm64/boot/dts/exynos/exynos7885.dtsi b/arch/arm64/boot/dts/exynos/exynos7885.dtsi
index 23c2e0bb0..4b94ac9da 100644
--- a/arch/arm64/boot/dts/exynos/exynos7885.dtsi
+++ b/arch/arm64/boot/dts/exynos/exynos7885.dtsi
@@ -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>;
-- 
2.38.3



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: exynos-dw-mshc-common: add exynos78xx variants
  2023-03-12 13:03 ` [PATCH v3 1/2] dt-bindings: exynos-dw-mshc-common: add exynos78xx variants Sergey Lisov
@ 2023-03-12 13:35   ` Krzysztof Kozlowski
  2023-03-12 16:43     ` Sergey Lisov
  0 siblings, 1 reply; 8+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-12 13:35 UTC (permalink / raw)
  To: Sergey Lisov, Ulf Hansson, Rob Herring, Krzysztof Kozlowski,
	Alim Akhtar, Jaehoon Chung
  Cc: linux-mmc, devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel

On 12/03/2023 14:03, Sergey Lisov wrote:
> 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.

>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/samsung,exynos-dw-mshc.yaml b/Documentation/devicetree/bindings/mmc/samsung,exynos-dw-mshc.yaml
> index fdaa18481..a72a67792 100644
> --- a/Documentation/devicetree/bindings/mmc/samsung,exynos-dw-mshc.yaml
> +++ b/Documentation/devicetree/bindings/mmc/samsung,exynos-dw-mshc.yaml
> @@ -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?

>        - axis,artpec8-dw-mshc
>  
>    reg:
> diff --git a/arch/arm64/boot/dts/exynos/exynos7885.dtsi b/arch/arm64/boot/dts/exynos/exynos7885.dtsi
> index 23c2e0bb0..4b94ac9da 100644
> --- a/arch/arm64/boot/dts/exynos/exynos7885.dtsi
> +++ b/arch/arm64/boot/dts/exynos/exynos7885.dtsi
> @@ -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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: exynos-dw-mshc-common: add exynos78xx variants
  2023-03-12 13:35   ` Krzysztof Kozlowski
@ 2023-03-12 16:43     ` Sergey Lisov
  0 siblings, 0 replies; 8+ messages in thread
From: Sergey Lisov @ 2023-03-12 16:43 UTC (permalink / raw)
  To: Ulf Hansson, Rob Herring, Krzysztof Kozlowski, Alim Akhtar,
	Jaehoon Chung
  Cc: linux-mmc, devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel

> Bindings and DTS (and driver) are always separate.

Okay, will split the patch.

> 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.

> 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.


^ permalink raw reply	[flat|nested] 8+ messages in thread

[parent not found: <640e0136.c20a0220.2d5bf.1959@mx.google.com>]

* Re: [PATCH v3 1/2] dt-bindings: exynos-dw-mshc-common: add exynos78xx variants
       [not found] <640e0136.c20a0220.2d5bf.1959@mx.google.com>
@ 2023-03-12 17:09 ` Krzysztof Kozlowski
  2023-03-12 17:26   ` Sergey Lisov
  0 siblings, 1 reply; 8+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-12 17:09 UTC (permalink / raw)
  To: Sergey Lisov, Ulf Hansson, Rob Herring, Krzysztof Kozlowski,
	Alim Akhtar, Jaehoon Chung
  Cc: linux-mmc, devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel

On 12/03/2023 17:43, Sergey Lisov wrote:
>> Bindings and DTS (and driver) are always separate.
> 
> Okay, will split the patch.
> 
>> 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.

Thanks for letting me know.

https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42

>> 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.

BTW, this rationale was only example - you need to come with something real.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: exynos-dw-mshc-common: add exynos78xx variants
  2023-03-12 17:09 ` Krzysztof Kozlowski
@ 2023-03-12 17:26   ` Sergey Lisov
  0 siblings, 0 replies; 8+ messages in thread
From: Sergey Lisov @ 2023-03-12 17:26 UTC (permalink / raw)
  To: Ulf Hansson, Rob Herring, Krzysztof Kozlowski, Alim Akhtar,
	Jaehoon Chung
  Cc: linux-mmc, devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel

> Thanks for letting me know.
> 
> https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42

OK, at least its codified somewhere. Still, this results in the opposite
effect: DTBs written for one SoC, using compatibles from other SoCs just
because "they are the same anyway". And doing this properly, well, results
in essentially duplicate compatibles.

And "fallback compatibles" won't solve this case anyway, as there is no
common compatible that denotes "Exynos7 DW-MMC that has the bug".

>>> 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.

Probably it makes sense to put this patch after the actual implementation,
so that git bisect always gives a working setup.

> BTW, this rationale was only example - you need to come with something real.

Pretty much the only thing that is broken are SDIO cards, because they run
very short transfers (below the DMA threshold) over the data lines. That's
exactly what I stated.

^ permalink raw reply	[flat|nested] 8+ messages in thread

[parent not found: <640e0b2d.c20a0220.63661.19a4@mx.google.com>]

* Re: [PATCH v3 1/2] dt-bindings: exynos-dw-mshc-common: add exynos78xx variants
       [not found] <640e0b2d.c20a0220.63661.19a4@mx.google.com>
@ 2023-03-12 17:29 ` Krzysztof Kozlowski
  0 siblings, 0 replies; 8+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-12 17:29 UTC (permalink / raw)
  To: Sergey Lisov, Ulf Hansson, Rob Herring, Krzysztof Kozlowski,
	Alim Akhtar, Jaehoon Chung
  Cc: linux-mmc, devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel

On 12/03/2023 18:26, Sergey Lisov wrote:
>> Thanks for letting me know.
>>
>> https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42
> 
> OK, at least its codified somewhere.

You got feedback from Devicetree maintainer. It should have been enough.

> Still, this results in the opposite
> effect: DTBs written for one SoC, using compatibles from other SoCs just
> because "they are the same anyway". And doing this properly, well, results
> in essentially duplicate compatibles.
> 
> And "fallback compatibles" won't solve this case anyway, as there is no
> common compatible that denotes "Exynos7 DW-MMC that has the bug".

Your explanation is not correct and we talked about this so many times.
No wildcards in compatibles. That's the rule. You cannot find any
argument good enough to break that rule.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2023-03-12 17:29 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-12 13:03 [PATCH v3 0/2] mmc: dw_mmc: fix DW MMC cores with 32-bit bus on 64-bit Linux systems Sergey Lisov
2023-03-12 13:03 ` [PATCH v3 2/2] mmc: dw_mmc: add an option to force 32-bit access to 64-bit FIFO Sergey Lisov
2023-03-12 13:03 ` [PATCH v3 1/2] dt-bindings: exynos-dw-mshc-common: add exynos78xx variants Sergey Lisov
2023-03-12 13:35   ` Krzysztof Kozlowski
2023-03-12 16:43     ` Sergey Lisov
     [not found] <640e0136.c20a0220.2d5bf.1959@mx.google.com>
2023-03-12 17:09 ` Krzysztof Kozlowski
2023-03-12 17:26   ` Sergey Lisov
     [not found] <640e0b2d.c20a0220.63661.19a4@mx.google.com>
2023-03-12 17:29 ` Krzysztof Kozlowski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).