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

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

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

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