[Qemu-devel] PL181 write problem

[Qemu-devel] PL181 write problem

[Hans-Erik Floryd]

Hello,

The PL181 sdcard module (hw/pl181.c) doesn't seem to handle writes 
correctly. Only every fourth byte gets written to the card. I've tracked 
the problem down to the following loop in pl181_fifo_run():

         limit = is_read ? PL181_FIFO_LEN : 0;
         n = 0;
         value = 0;
         while (s->datacnt && s->fifo_len != limit) {
             if (is_read) {
                 value |= (uint32_t)sd_read_data(s->card) << (n * 8);
                 n++;
                 if (n == 4) {
                     pl181_fifo_push(s, value);
                     value = 0;
                     n = 0;
                 }
             } else {
                 if (n == 0) {
                     value = pl181_fifo_pop(s);
                     n = 4;
                 }
                 sd_write_data(s->card, value & 0xff);
                 value >>= 8;
                 n--;
             }
             s->datacnt--;
         }

When writing, a 32-bit value is popped from the fifo, followed by a 
write of 1 byte. If there was only one value in the fifo, fifo_len will 
now be 0. This causes the loop to terminate and the remaining three 
bytes will not be written.

The following patch fixes this by writing all four bytes before checking 
fifo_len.

Let me know if there's anything else I need to do to get the patch accepted.

Thanks

Index: hw/pl181.c
===================================================================
--- hw/pl181.c  (revision 5648)
+++ hw/pl181.c  (working copy)
@@ -202,16 +202,20 @@
                      value = 0;
                      n = 0;
                  }
+                s->datacnt--;
              } else {
                  if (n == 0) {
                      value = pl181_fifo_pop(s);
                      n = 4;
                  }
-                sd_write_data(s->card, value & 0xff);
-                value >>= 8;
-                n--;
+                while (s->datacnt && n != 0)
+                {
+                   sd_write_data(s->card, value & 0xff);
+                   value >>= 8;
+                   n--;
+                   s->datacnt--;
+                }
              }
-            s->datacnt--;
          }
          if (n && is_read) {
              pl181_fifo_push(s, value);

Re: [Qemu-devel] PL181 write problem

[Paul Brook]

> The following patch fixes this by writing all four bytes before checking
> fifo_len.

I think your analysis is probably correct, but I'm not so keen on your 
solution. You've got redundant loops/conditions.

Paul

Re: [Qemu-devel] PL181 write problem

[Hans-Erik Floryd]

>> The following patch fixes this by writing all four bytes before checking
>> fifo_len.
> 
> I think your analysis is probably correct, but I'm not so keen on your 
> solution. You've got redundant loops/conditions.
> 

It seemed like a good idea to not modify the read path since it works, 
but it does make the code a bit complicated. In the following patch I've 
broken read and write into separate loops which hopefully makes the code 
easier to follow. s->datacnt needs to be checked for every byte written 
or read since it might not be a multiple of 4.

Index: hw/pl181.c
===================================================================
--- hw/pl181.c  (revision 5648)
+++ hw/pl181.c  (working copy)
@@ -184,38 +184,28 @@
      uint32_t bits;
      uint32_t value;
      int n;
-    int limit;
      int is_read;

      is_read = (s->datactrl & PL181_DATA_DIRECTION) != 0;
      if (s->datacnt != 0 && (!is_read || sd_data_ready(s->card))
              && !s->linux_hack) {
-        limit = is_read ? PL181_FIFO_LEN : 0;
-        n = 0;
-        value = 0;
-        while (s->datacnt && s->fifo_len != limit) {
-            if (is_read) {
+       if (is_read) {
+          while (s->datacnt > 0 && s->fifo_len < PL181_FIFO_LEN) {
+             value = 0;
+             for (n = 0; n < 4 && s->datacnt > 0; n++, s->datacnt--) {
                  value |= (uint32_t)sd_read_data(s->card) << (n * 8);
-                n++;
-                if (n == 4) {
-                    pl181_fifo_push(s, value);
-                    value = 0;
-                    n = 0;
-                }
-            } else {
-                if (n == 0) {
-                    value = pl181_fifo_pop(s);
-                    n = 4;
-                }
+             }
+             pl181_fifo_push(s, value);
+          }
+       } else {
+          while (s->datacnt > 0 && s->fifo_len > 0) {
+             value = pl181_fifo_pop(s);
+             for (n = 0; n < 4 && s->datacnt > 0; n++, s->datacnt--) {
                  sd_write_data(s->card, value & 0xff);
                  value >>= 8;
-                n--;
-            }
-            s->datacnt--;
-        }
-        if (n && is_read) {
-            pl181_fifo_push(s, value);
-        }
+             }
+          }
+       }
      }
      s->status &= ~(PL181_STATUS_RX_FIFO | PL181_STATUS_TX_FIFO);
      if (s->datacnt == 0) {