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

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

[Sergey Lisov]

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.

Sergey Lisov (2):
  dt-bindings: synopsys-dw-mshc-common: add "fifo-access-32bit" property
  mmc: dw_mmc: add an option to force 32-bit access to 64-bit FIFO

 .../bindings/mmc/synopsys-dw-mshc-common.yaml |   7 +
 drivers/mmc/host/dw_mmc.c                     | 125 +++++++++++++++++-
 drivers/mmc/host/dw_mmc.h                     |   2 +
 3 files changed, 132 insertions(+), 2 deletions(-)

-- 
2.38.3

[PATCH v2 1/2] dt-bindings: synopsys-dw-mshc-common: add "fifo-access-32bit" property

[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.
---
 .../devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml   | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml
index 8dfad89c7..d025b38ca 100644
--- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml
+++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml
@@ -57,6 +57,13 @@ properties:
       force fifo watermark setting accordingly.
     $ref: /schemas/types.yaml#/definitions/flag
 
+  fifo-access-32bit:
+    description:
+      Specifies that this device requires accesses to its 64-bit registers
+      to be done as pairs of 32-bit accesses, even on architectures where
+      readq is available.
+    $ref: /schemas/types.yaml#/definitions/flag
+
   dmas:
     maxItems: 1
 
-- 
2.38.3

Re: [PATCH v2 1/2] dt-bindings: synopsys-dw-mshc-common: add "fifo-access-32bit" property

[Krzysztof Kozlowski]

On 11/03/2023 19:15, 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.
> ---
>  .../devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml   | 7 +++++++
>  1 file changed, 7 insertions(+)

Missing changelog (I did not get cover letter, so no changelog there
either).

> diff --git a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml
> index 8dfad89c7..d025b38ca 100644
> --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml
> +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml
> @@ -57,6 +57,13 @@ properties:
>        force fifo watermark setting accordingly.
>      $ref: /schemas/types.yaml#/definitions/flag
>  
> +  fifo-access-32bit:
> +    description:
> +      Specifies that this device requires accesses to its 64-bit registers
> +      to be done as pairs of 32-bit accesses, even on architectures where
> +      readq is available.
> +    $ref: /schemas/types.yaml#/definitions/flag

Anyway, I said last time this looks compatible-specific, so I don't
think we need another property.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: synopsys-dw-mshc-common: add "fifo-access-32bit" property

[Krzysztof Kozlowski]

On 12/03/2023 11:18, Krzysztof Kozlowski wrote:
> On 11/03/2023 19:15, 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.
>> ---
>>  .../devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml   | 7 +++++++
>>  1 file changed, 7 insertions(+)
> 
> Missing changelog (I did not get cover letter, so no changelog there
> either).

Correction: I see the cover letter, but anyway without changelog :)


Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: synopsys-dw-mshc-common: add "fifo-access-32bit" property

[Sergey Lisov]

> 
> Missing changelog (I did not get cover letter, so no changelog there
> either).
> 

Got it, will fix in the next revision.

> 
> Anyway, I said last time this looks compatible-specific, so I don't
> think we need another property.
> 
> Best regards,
> Krzysztof

I agree, but I'm afraid of introducing regressions by enabling this
workaround on systems that don't actually need it.

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

[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.c | 125 +++++++++++++++++++++++++++++++++++++-
 drivers/mmc/host/dw_mmc.h |   2 +
 2 files changed, 125 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 581614196..eee430620 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;
@@ -3239,6 +3352,9 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
 	if (device_property_present(dev, "fifo-watermark-aligned"))
 		host->wm_aligned = true;
 
+	if (device_property_present(dev, "fifo-access-32bit"))
+		host->quirks |= DW_MMC_QUIRK_FIFO64_32;
+
 	if (!device_property_read_u32(dev, "clock-frequency", &clock_frequency))
 		pdata->bus_hz = clock_frequency;
 
@@ -3367,8 +3483,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

Re: [PATCH v2 1/2] dt-bindings: synopsys-dw-mshc-common: add "fifo-access-32bit" property

[Krzysztof Kozlowski]

On 12/03/2023 11:49, Sergey Lisov wrote:
>>
>> Anyway, I said last time this looks compatible-specific, so I don't
>> think we need another property.
>>
>> Best regards,
>> Krzysztof
> 
> I agree, but I'm afraid of introducing regressions by enabling this
> workaround on systems that don't actually need it.

I don't understand why would you enable it for systems which do not need it?

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: synopsys-dw-mshc-common: add "fifo-access-32bit" property

[Sergey Lisov]

> On 12/03/2023 11:49, Sergey Lisov wrote:
>>>
>>> Anyway, I said last time this looks compatible-specific, so I don't
>>> think we need another property.
>>>
>>> Best regards,
>>> Krzysztof
>> 
>> I agree, but I'm afraid of introducing regressions by enabling this
>> workaround on systems that don't actually need it.
> 
> I don't understand why would you enable it for systems which do not need it?

OK, then how do I find out which boards have the bug? My only idea is
"search for samsung,exynos7-dw-mshc through all devicetrees, find vendor
kernels for each of those boards, and check if they have the workaround".
Is it really that better than enabling it selectively only for
known-affected boards?

Re: [PATCH v2 1/2] dt-bindings: synopsys-dw-mshc-common: add "fifo-access-32bit" property

[Krzysztof Kozlowski]

On 12/03/2023 12:00, Sergey Lisov wrote:
>> On 12/03/2023 11:49, Sergey Lisov wrote:
>>>>
>>>> Anyway, I said last time this looks compatible-specific, so I don't
>>>> think we need another property.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>
>>> I agree, but I'm afraid of introducing regressions by enabling this
>>> workaround on systems that don't actually need it.
>>
>> I don't understand why would you enable it for systems which do not need it?
> 
> OK, then how do I find out which boards have the bug? My only idea is
> "search for samsung,exynos7-dw-mshc through all devicetrees, find vendor
> kernels for each of those boards, and check if they have the workaround".
> Is it really that better than enabling it selectively only for
> known-affected boards?

There is no way this is board specific. This is SoC specific. I
mentioned it last time.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: synopsys-dw-mshc-common: add "fifo-access-32bit" property

[Sergey Lisov]

> There is no way this is board specific. This is SoC specific. I
> mentioned it last time.

The same compatible string ("samsung,exynos7-dw-mshc{,-smu}") is used by
several devices on different Exynos SoCs. And I was only able to find
a vendor kernel fork for one of them (exynos7885-jackpotlte, it has the
workaround, but it depends on a configuration option and I don't have the
config file for that device).

Re: [PATCH v2 1/2] dt-bindings: synopsys-dw-mshc-common: add "fifo-access-32bit" property

[Krzysztof Kozlowski]

On 12/03/2023 12:26, Sergey Lisov wrote:
>> There is no way this is board specific. This is SoC specific. I
>> mentioned it last time.
> 
> The same compatible string ("samsung,exynos7-dw-mshc{,-smu}") is used by
> several devices on different Exynos SoCs.

... and should not be. It should come with a specific compatible
followed by this fallback. The specific compatible then will define
32-bit access.

> And I was only able to find
> a vendor kernel fork for one of them (exynos7885-jackpotlte, it has the
> workaround, but it depends on a configuration option and I don't have the
> config file for that device).

Vendor kernel have configs, I think that's requirement of GPL.

See jackpotlte_00_defconfig in my
samsung/galaxy-a8-2018-jackpotlte-sm-a530f-exynos7885-dump branch of
vendor-backup repo.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: synopsys-dw-mshc-common: add "fifo-access-32bit" property

[Sergey Lisov]

> ... and should not be. It should come with a specific compatible
> followed by this fallback. The specific compatible then will define
> 32-bit access.

OK, I'll then make a new revision with this baked into a compatible
string.