Series comparison

-[RFC.PATCH v1 0/2] sd:sdhci Fix data transfer did not complete
+[PATCH v2 0/2] sd:sdhci Fix data transfer did not complete
 v1:
 . Fix boundary_count overflow
 . Fix data transfer did not complete if data size is bigger then SDMA Buffer Boundary
+v2:
+. fix typo
+. update to none RFC patch
+. check the most upper byte of SDMA System Address Register (0x00) is written,
+   then restarts SDMA data transfer.
 Jamin Lin (2):
-  RFC:sd:sdhci: Fix boundary_count overflow in
+  hw/sd/sdhci: Fix boundary_count overflow in
     sdhci_sdma_transfer_multi_blocks
-  RFC:sd:sdhci: Fix data transfer did not complete if data size is
+  hw/sd/sdhci: Fix data transfer did not complete if data size is bigger
-    bigger then SDMA Buffer Boundary
+    than SDMA Buffer Boundary
- hw/sd/sdhci.c | 27 +++++++++++++--------------
+ hw/sd/sdhci.c | 20 ++++++++++++++++----
-file changed, 13 insertions(+), 14 deletions(-)
+file changed, 16 insertions(+), 4 deletions(-)
 --
 .34.1

-[RFC.PATCH v1 1/2] sd:sdhci: Fix boundary_count overflow in sdhci_sdma_transfer_multi_blocks
+[PATCH v2 1/2] hw/sd/sdhci: Fix boundary_count overflow in sdhci_sdma_transfer_multi_blocks
 How to reproduce it:
 . The value of "s->blksie" was 0x7200. The bits[14:12] was "111", so the buffer
-   boundary was 0x80000.(512Kbytes). This SDMA buffer boundary the same as
+   boundary was 0x80000.(512Kbytes). This SDMA buffer boundary was the same as
    u-boot default value.
-   The bit[11:0] is "001000000000", so the block size is 0x200.(512bytes)
+   The bit[11:0] was "001000000000", so the block size was 0x200.(512bytes)
 . The SDMA address was 0x83123456 which was not page aligned and
    "s->sdmasysad % boundary_chk" was 0x23456. The value of boundary_count was
 x5cbaa.("boundary_chk - (s->sdmasysad % boundary_chk)" -->
    "(0x80000 - 0x23456)")
-However, boundary_count did not aligned the block size 512 bytes and the SDMA
+However, boundary_count did not align the block size 512 bytes and the SDMA
-address is not page aligned(0x80000), so the following if-statement never be true,
+address was not page aligned(0x80000), so the following if-statement never be true,
 ```
 if (((boundary_count + begin) < block_size) && page_aligned)
 ````
 Finally, it caused boundary_count overflow because its data type was uint32_t.
 Ex: the last boundary_count was 0x1aa and "0x1aa - 0x200" became "0xffffffaa".
 It is the wrong behavior.
-To fix it, it seems we can directly check the "boundary_count < blocksize"
+To fix it, change to check boundary_count smaller than block size if system
-instead of "boundary_count < blocksize && page_aligned"
+address did not page align
 Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
 ---
- hw/sd/sdhci.c | 18 ++++--------------
+ hw/sd/sdhci.c | 8 ++++----
-file changed, 4 insertions(+), 14 deletions(-)
+file changed, 4 insertions(+), 4 deletions(-)
 diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
 index XXXXXXX..XXXXXXX 100644
 --- a/hw/sd/sdhci.c
 +++ b/hw/sd/sdhci.c
-@@ -XXX,XX +XXX,XX @@ static void sdhci_write_dataport(SDHCIState *s, uint32_t value, unsigned size)
- /* Multi block SDMA transfer */
- static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
- {
--    bool page_aligned = false;
-     unsigned int begin;
-     const uint16_t block_size = s->blksize & BLOCK_SIZE_MASK;
-     uint32_t boundary_chk = 1 << (((s->blksize & ~BLOCK_SIZE_MASK) >> 12) + 12);
-@@ -XXX,XX +XXX,XX @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
-         return;
-     }
--    /*
--     * XXX: Some sd/mmc drivers (for example, u-boot-slp) do not account for
--     * possible stop at page boundary if initial address is not page aligned,
--     * allow them to work properly
--     */
--    if ((s->sdmasysad % boundary_chk) == 0) {
--        page_aligned = true;
--    }
--
-     s->prnsts |= SDHC_DATA_INHIBIT | SDHC_DAT_LINE_ACTIVE;
-     if (s->trnmod & SDHC_TRNS_READ) {
-         s->prnsts |= SDHC_DOING_READ;
 @@ -XXX,XX +XXX,XX @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
                  sdbus_read_data(&s->sdbus, s->fifo_buffer, block_size);
              }
              begin = s->data_count;
 -            if (((boundary_count + begin) < block_size) && page_aligned) {
-+            if (((boundary_count + begin) < block_size)) {
++            if (((boundary_count + begin) < block_size) && !page_aligned) {
                  s->data_count = boundary_count + begin;
                  boundary_count = 0;
               } else {
 @@ -XXX,XX +XXX,XX @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
              if (s->data_count == block_size) {
 ...
 @@ -XXX,XX +XXX,XX @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
          s->prnsts |= SDHC_DOING_WRITE;
          while (s->blkcnt) {
              begin = s->data_count;
 -            if (((boundary_count + begin) < block_size) && page_aligned) {
-+            if (((boundary_count + begin) < block_size)) {
++            if (((boundary_count + begin) < block_size) && !page_aligned) {
                  s->data_count = boundary_count + begin;
                  boundary_count = 0;
               } else {
 @@ -XXX,XX +XXX,XX @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
                      s->blkcnt--;
 ...

-[RFC.PATCH v1 2/2] sd:sdhci: Fix data transfer did not complete if data size is bigger then SDMA Buffer Boundary
+[PATCH v2 2/2] hw/sd/sdhci: Fix data transfer did not complete if data size is bigger than SDMA Buffer Boundary
 According to the design of sdhci_sdma_transfer_multi_blocks, if the
-"s->blkcnt * 512" was bigger than the SDMA Buffer boundary, it breaked the
+"s->blkcnt * 512" was bigger than the SDMA Buffer boundary, it break the
-while loop of data transfer and set SDHC_NISEN_DMA in the normal interreupt
+while loop of data transfer and set SDHC_NISEN_DMA in the normal interrupt
 status to notify the firmware that this SDMA boundary buffer Transfer Complete
 and firmware should set the system address of the next SDMA boundary buffer
-for the remaining data transfer.
+for the remained data transfer.
 However, after firmware set the system address of the next SDMA boundary buffer
-in the SDMA System Address Register(0x00), SDHCI modle did not start the data
+in the SDMA System Address Register(0x00), SDHCI model did not restart the data
-transfer, again. Finally, firmware breaked the data transfer because firmware
+transfer, again. Finally, firmware break the data transfer because firmware
-did not receive the DMA Interrupt and Tansfer Complete Interrupt from SDHCI
+did not receive the either "DMA Interrupt" or "Transfer Complete Interrupt"
-model.
+from SDHCI model.
 Error log from u-boot
 ```
 sdhci_transfer_data: Transfer data timeout
  ** fs_devread read error - block
 ```
-According to the following mention from SDMA System Address Refister of SDHCI
+According to the following mention from SDMA System Address Register of SDHCI
 spec,
 '''
 This register contains the system memory address for an SDMA transfer in
 -bit addressing mode. When the Host Controller stops an SDMA transfer,
 this register shall point to the system address of the next contiguous data
 ...
 Driver sets the next system address of the next data position to this register.
 When the most upper byte of this register (003h) is written, the Host Controller
 restarts the SDMA transfer.
 ''',
-restrat the data transfer if firmware set the SDMA System Address, s->blkcnt
+restart the data transfer if firmware writes the most upper byte of SDMA System
-is bigger than 0 and SDHCI is in the data transfer state.
+Address, s->blkcnt is bigger than 0 and SDHCI is in the data transfer state.
 Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
 ---
- hw/sd/sdhci.c | 9 +++++++++
+ hw/sd/sdhci.c | 12 ++++++++++++
-file changed, 9 insertions(+)
+file changed, 12 insertions(+)
 diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
 index XXXXXXX..XXXXXXX 100644
 --- a/hw/sd/sdhci.c
 +++ b/hw/sd/sdhci.c
 @@ -XXX,XX +XXX,XX @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
                      sdhci_sdma_transfer_single_block(s);
                  }
              }
 +        } else if (TRANSFERRING_DATA(s->prnsts)) {
-+            /* restarts the SDMA transfer if blkcnt is not zero  */
++            s->sdmasysad = (s->sdmasysad & mask) | value;
-+            if (s->blkcnt && SDHC_DMA_TYPE(s->hostctl1) == SDHC_CTRL_SDMA) {
++            MASKED_WRITE(s->sdmasysad, mask, value);
 +            /* restarts the SDMA transfer if the most upper byte is written */
 +            if ((s->sdmasysad & 0xFF000000) && s->blkcnt &&
 +                SDHC_DMA_TYPE(s->hostctl1) == SDHC_CTRL_SDMA) {
 +                if (s->trnmod & SDHC_TRNS_MULTI) {
 +                    sdhci_sdma_transfer_multi_blocks(s);
 +                } else {
 +                    sdhci_sdma_transfer_single_block(s);
 +                }
 +            }
          }
          break;
      case SDHC_BLKSIZE:
 --
 .34.1

How to reproduce it:
1. The value of "s->blksie" was 0x7200. The bits[14:12] was "111", so the buffer
   boundary was 0x80000.(512Kbytes). This SDMA buffer boundary the same as
   u-boot default value.
   The bit[11:0] is "001000000000", so the block size is 0x200.(512bytes)
2. The SDMA address was 0x83123456 which was not page aligned and
   "s->sdmasysad % boundary_chk" was 0x23456. The value of boundary_count was
   0x5cbaa.("boundary_chk - (s->sdmasysad % boundary_chk)" -->
   "(0x80000 - 0x23456)")

However, boundary_count did not aligned the block size 512 bytes and the SDMA
address is not page aligned(0x80000), so the following if-statement never be true,
```
if (((boundary_count + begin) < block_size) && page_aligned)
````

Finally, it caused boundary_count overflow because its data type was uint32_t.
Ex: the last boundary_count was 0x1aa and "0x1aa - 0x200" became "0xffffffaa".
It is the wrong behavior.

To fix it, it seems we can directly check the "boundary_count < blocksize"
instead of "boundary_count < blocksize && page_aligned"

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/sd/sdhci.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index XXXXXXX..XXXXXXX 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -XXX,XX +XXX,XX @@ static void sdhci_write_dataport(SDHCIState *s, uint32_t value, unsigned size)
 /* Multi block SDMA transfer */
 static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
 {
-    bool page_aligned = false;
     unsigned int begin;
     const uint16_t block_size = s->blksize & BLOCK_SIZE_MASK;
     uint32_t boundary_chk = 1 << (((s->blksize & ~BLOCK_SIZE_MASK) >> 12) + 12);
@@ -XXX,XX +XXX,XX @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
         return;
     }
 
-    /*
-     * XXX: Some sd/mmc drivers (for example, u-boot-slp) do not account for
-     * possible stop at page boundary if initial address is not page aligned,
-     * allow them to work properly
-     */
-    if ((s->sdmasysad % boundary_chk) == 0) {
-        page_aligned = true;
-    }
-
     s->prnsts |= SDHC_DATA_INHIBIT | SDHC_DAT_LINE_ACTIVE;
     if (s->trnmod & SDHC_TRNS_READ) {
         s->prnsts |= SDHC_DOING_READ;
@@ -XXX,XX +XXX,XX @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
                 sdbus_read_data(&s->sdbus, s->fifo_buffer, block_size);
             }
             begin = s->data_count;
-            if (((boundary_count + begin) < block_size) && page_aligned) {
+            if (((boundary_count + begin) < block_size)) {
                 s->data_count = boundary_count + begin;
                 boundary_count = 0;
              } else {
@@ -XXX,XX +XXX,XX @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
             if (s->data_count == block_size) {
                 s->data_count = 0;
             }
-            if (page_aligned && boundary_count == 0) {
+            if (boundary_count == 0) {
                 break;
             }
         }
@@ -XXX,XX +XXX,XX @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
         s->prnsts |= SDHC_DOING_WRITE;
         while (s->blkcnt) {
             begin = s->data_count;
-            if (((boundary_count + begin) < block_size) && page_aligned) {
+            if (((boundary_count + begin) < block_size)) {
                 s->data_count = boundary_count + begin;
                 boundary_count = 0;
              } else {
@@ -XXX,XX +XXX,XX @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
                     s->blkcnt--;
                 }
             }
-            if (page_aligned && boundary_count == 0) {
+            if (boundary_count == 0) {
                 break;
             }
         }
-- 
2.34.1

According to the design of sdhci_sdma_transfer_multi_blocks, if the
"s->blkcnt * 512" was bigger than the SDMA Buffer boundary, it breaked the
while loop of data transfer and set SDHC_NISEN_DMA in the normal interreupt
status to notify the firmware that this SDMA boundary buffer Transfer Complete
and firmware should set the system address of the next SDMA boundary buffer
for the remaining data transfer.

However, after firmware set the system address of the next SDMA boundary buffer
in the SDMA System Address Register(0x00), SDHCI modle did not start the data
transfer, again. Finally, firmware breaked the data transfer because firmware
did not receive the DMA Interrupt and Tansfer Complete Interrupt from SDHCI
model.

Error log from u-boot
```
sdhci_transfer_data: Transfer data timeout
 ** fs_devread read error - block
```

According to the following mention from SDMA System Address Refister of SDHCI
spec,
'''
This register contains the system memory address for an SDMA transfer in
32-bit addressing mode. When the Host Controller stops an SDMA transfer,
this register shall point to the system address of the next contiguous data
position.
It can be accessed only if no transaction is executing (i.e., after a transaction
has stopped). Reading this register during SDMA transfers may return an
invalid value.
The Host Driver shall initialize this register before starting an SDMA
transaction.
After SDMA has stopped, the next system address of the next contiguous
data position can be read from this register.
The SDMA transfer waits at the every boundary specified by the SDMA
Buffer Boundary in the Block Size register. The Host Controller generates
DMA Interrupt to request the Host Driver to update this register. The Host
Driver sets the next system address of the next data position to this register.
When the most upper byte of this register (003h) is written, the Host Controller
restarts the SDMA transfer.
''',

restrat the data transfer if firmware set the SDMA System Address, s->blkcnt
is bigger than 0 and SDHCI is in the data transfer state.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/sd/sdhci.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index XXXXXXX..XXXXXXX 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -XXX,XX +XXX,XX @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
                     sdhci_sdma_transfer_single_block(s);
                 }
             }
+        } else if (TRANSFERRING_DATA(s->prnsts)) {
+            /* restarts the SDMA transfer if blkcnt is not zero  */
+            if (s->blkcnt && SDHC_DMA_TYPE(s->hostctl1) == SDHC_CTRL_SDMA) {
+                if (s->trnmod & SDHC_TRNS_MULTI) {
+                    sdhci_sdma_transfer_multi_blocks(s);
+                } else {
+                    sdhci_sdma_transfer_single_block(s);
+                }
+            }
         }
         break;
     case SDHC_BLKSIZE:
-- 
2.34.1

How to reproduce it:
1. The value of "s->blksie" was 0x7200. The bits[14:12] was "111", so the buffer
   boundary was 0x80000.(512Kbytes). This SDMA buffer boundary was the same as
   u-boot default value.
   The bit[11:0] was "001000000000", so the block size was 0x200.(512bytes)
2. The SDMA address was 0x83123456 which was not page aligned and
   "s->sdmasysad % boundary_chk" was 0x23456. The value of boundary_count was
   0x5cbaa.("boundary_chk - (s->sdmasysad % boundary_chk)" -->
   "(0x80000 - 0x23456)")

However, boundary_count did not align the block size 512 bytes and the SDMA
address was not page aligned(0x80000), so the following if-statement never be true,
```
if (((boundary_count + begin) < block_size) && page_aligned)
````

Finally, it caused boundary_count overflow because its data type was uint32_t.
Ex: the last boundary_count was 0x1aa and "0x1aa - 0x200" became "0xffffffaa".
It is the wrong behavior.

To fix it, change to check boundary_count smaller than block size if system
address did not page align

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/sd/sdhci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index XXXXXXX..XXXXXXX 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -XXX,XX +XXX,XX @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
                 sdbus_read_data(&s->sdbus, s->fifo_buffer, block_size);
             }
             begin = s->data_count;
-            if (((boundary_count + begin) < block_size) && page_aligned) {
+            if (((boundary_count + begin) < block_size) && !page_aligned) {
                 s->data_count = boundary_count + begin;
                 boundary_count = 0;
              } else {
@@ -XXX,XX +XXX,XX @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
             if (s->data_count == block_size) {
                 s->data_count = 0;
             }
-            if (page_aligned && boundary_count == 0) {
+            if (boundary_count == 0) {
                 break;
             }
         }
@@ -XXX,XX +XXX,XX @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
         s->prnsts |= SDHC_DOING_WRITE;
         while (s->blkcnt) {
             begin = s->data_count;
-            if (((boundary_count + begin) < block_size) && page_aligned) {
+            if (((boundary_count + begin) < block_size) && !page_aligned) {
                 s->data_count = boundary_count + begin;
                 boundary_count = 0;
              } else {
@@ -XXX,XX +XXX,XX @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
                     s->blkcnt--;
                 }
             }
-            if (page_aligned && boundary_count == 0) {
+            if (boundary_count == 0) {
                 break;
             }
         }
-- 
2.34.1

According to the design of sdhci_sdma_transfer_multi_blocks, if the
"s->blkcnt * 512" was bigger than the SDMA Buffer boundary, it break the
while loop of data transfer and set SDHC_NISEN_DMA in the normal interrupt
status to notify the firmware that this SDMA boundary buffer Transfer Complete
and firmware should set the system address of the next SDMA boundary buffer
for the remained data transfer.

However, after firmware set the system address of the next SDMA boundary buffer
in the SDMA System Address Register(0x00), SDHCI model did not restart the data
transfer, again. Finally, firmware break the data transfer because firmware
did not receive the either "DMA Interrupt" or "Transfer Complete Interrupt"
from SDHCI model.

Error log from u-boot
```
sdhci_transfer_data: Transfer data timeout
 ** fs_devread read error - block
```

According to the following mention from SDMA System Address Register of SDHCI
spec,
'''
This register contains the system memory address for an SDMA transfer in
32-bit addressing mode. When the Host Controller stops an SDMA transfer,
this register shall point to the system address of the next contiguous data
position.
It can be accessed only if no transaction is executing (i.e., after a transaction
has stopped). Reading this register during SDMA transfers may return an
invalid value.
The Host Driver shall initialize this register before starting an SDMA
transaction.
After SDMA has stopped, the next system address of the next contiguous
data position can be read from this register.
The SDMA transfer waits at the every boundary specified by the SDMA
Buffer Boundary in the Block Size register. The Host Controller generates
DMA Interrupt to request the Host Driver to update this register. The Host
Driver sets the next system address of the next data position to this register.
When the most upper byte of this register (003h) is written, the Host Controller
restarts the SDMA transfer.
''',

restart the data transfer if firmware writes the most upper byte of SDMA System
Address, s->blkcnt is bigger than 0 and SDHCI is in the data transfer state.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/sd/sdhci.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index XXXXXXX..XXXXXXX 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -XXX,XX +XXX,XX @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
                     sdhci_sdma_transfer_single_block(s);
                 }
             }
+        } else if (TRANSFERRING_DATA(s->prnsts)) {
+            s->sdmasysad = (s->sdmasysad & mask) | value;
+            MASKED_WRITE(s->sdmasysad, mask, value);
+            /* restarts the SDMA transfer if the most upper byte is written */
+            if ((s->sdmasysad & 0xFF000000) && s->blkcnt &&
+                SDHC_DMA_TYPE(s->hostctl1) == SDHC_CTRL_SDMA) {
+                if (s->trnmod & SDHC_TRNS_MULTI) {
+                    sdhci_sdma_transfer_multi_blocks(s);
+                } else {
+                    sdhci_sdma_transfer_single_block(s);
+                }
+            }
         }
         break;
     case SDHC_BLKSIZE:
-- 
2.34.1