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

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

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.