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

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

[Krzysztof Kozlowski]

On 11/03/2023 13:54, Sergey Lisov wrote:
> ---
>  .../devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml    | 6 ++++++

I received your patches twice, so I already responded to one thread. The
comments apply also here.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 11/03/2023 18:57, Sergey Lisov wrote:
>> Why did you remove the subject? Please keep the mailing process matching
>> mailing lists. It messes with mailboxes, filters and reading process.
> 
> Sorry about this, I just wanted to get rid of those X-Something headers and
> accidentially deleted this one.
> 
>> Yeah, Rob acked it so I will let him to judge this. To me it looks like
>> unnecessary fragmentation - this looks like compatible specific, not
>> board. Anyway you need to resend to fix all the mailing mess.
>>
>> Best regards,
>> Krzysztof
> 
> Should I resend the whole patchset from scratch, as if nothing happened?

No, sorry, I was not specific. You need anyway v2 with all the fixes I
pointed already. Lack of commit msg is a no-go.

Best regards,
Krzysztof

Re: broken subject?

[Krzysztof Kozlowski]

On 11/03/2023 18:40, Sergey Lisov wrote:
>>> ---
>>>  .../devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml    | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>

Why did you remove the subject? Please keep the mailing process matching
mailing lists. It messes with mailboxes, filters and reading process.

>>> diff --git a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml
>>> index 8dfad89c7..2bc5ac528 100644
>>> --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml
>>> +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml
>>> @@ -57,6 +57,12 @@ properties:
>>>        force fifo watermark setting accordingly.
>>>      $ref: /schemas/types.yaml#/definitions/flag
>>>  
>>> +  fifo-access-32bit:
>>
>> Missing type boolean.
> 
> Thanks, will add the same $ref as for the entry above.
> 
>>> +    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.
>>
>> And why the device would require this? If it has 64-bit registers in the
>> first place, they can be accessed in 64-bit. Otherwise these are not
>> 64-bit registers, but just lower/upper 32-bit, right?
>>
>> Also, why this cannot be implied from compatible? Why different boards
>> with same SoC should have different FIFO access?
> 
> It probably can be implied, but I am not exactly sure on which boards it
> affects, so I decided to go for a new devicetree option. Anyway, the same
> argument applies to the "data-addr" property, which is already in the
> spec, so I supposed that adding such knobs is fine.

Yeah, Rob acked it so I will let him to judge this. To me it looks like
unnecessary fragmentation - this looks like compatible specific, not
board. Anyway you need to resend to fix all the mailing mess.

Best regards,
Krzysztof

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

[Sergey Lisov]

> Why did you remove the subject? Please keep the mailing process matching
> mailing lists. It messes with mailboxes, filters and reading process.

Sorry about this, I just wanted to get rid of those X-Something headers and
accidentially deleted this one.

> Yeah, Rob acked it so I will let him to judge this. To me it looks like
> unnecessary fragmentation - this looks like compatible specific, not
> board. Anyway you need to resend to fix all the mailing mess.
> 
> Best regards,
> Krzysztof

Should I resend the whole patchset from scratch, as if nothing happened?

[PATCH 0/2] 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):
  devicetree: synopsys-dw-mshc-common: add "fifo-access-32bit" property
  dw_mmc: add an option to force 32-bit accesses to 64-bit device
    registers

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

-- 
2.38.3

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

[Sergey Lisov]

---
 .../devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml    | 6 ++++++
 1 file changed, 6 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..2bc5ac528 100644
--- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml
+++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml
@@ -57,6 +57,12 @@ 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.
+
   dmas:
     maxItems: 1
 
-- 
2.38.3

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

[Krzysztof Kozlowski]

On 11/03/2023 16:22, Sergey Lisov wrote:

Use subject prefixes matching the subsystem (which you can get for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching).

Missing commit msg, which should answer why you are doing this and give
more background.

> ---
>  .../devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml    | 6 ++++++
>  1 file changed, 6 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..2bc5ac528 100644
> --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml
> +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml
> @@ -57,6 +57,12 @@ properties:
>        force fifo watermark setting accordingly.
>      $ref: /schemas/types.yaml#/definitions/flag
>  
> +  fifo-access-32bit:

Missing type boolean.

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

And why the device would require this? If it has 64-bit registers in the
first place, they can be accessed in 64-bit. Otherwise these are not
64-bit registers, but just lower/upper 32-bit, right?

Also, why this cannot be implied from compatible? Why different boards
with same SoC should have different FIFO access?



Best regards,
Krzysztof

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

[Sergey Lisov]

---
 .../devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml    | 6 ++++++
 1 file changed, 6 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..2bc5ac528 100644
--- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml
+++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc-common.yaml
@@ -57,6 +57,12 @@ 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.
+
   dmas:
     maxItems: 1
 
-- 
2.38.3