From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43)
	id 1LU0rM-0001wX-Uq
	for qemu-devel@nongnu.org; Mon, 02 Feb 2009 10:37:16 -0500
Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43)
	id 1LU0rL-0001w9-AQ
	for qemu-devel@nongnu.org; Mon, 02 Feb 2009 10:37:15 -0500
Received: from [199.232.76.173] (port=41932 helo=monty-python.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1LU0rL-0001w6-7R
	for qemu-devel@nongnu.org; Mon, 02 Feb 2009 10:37:15 -0500
Received: from csmtp1.one.com ([195.47.247.21]:59724 helo=csmtp1.b-one.net)
	by monty-python.gnu.org with esmtp (Exim 4.60)
	(envelope-from <hans-erik.floryd@rt-labs.com>) id 1LU0rK-0006Ev-Nt
	for qemu-devel@nongnu.org; Mon, 02 Feb 2009 10:37:15 -0500
Received: from [192.168.10.151] (static-213-115-7-226.sme.bredbandsbolaget.se
	[213.115.7.226])
	by csmtp1.b-one.net (Postfix) with ESMTP id 03BAC1BC07C11
	for <qemu-devel@nongnu.org>; Mon,  2 Feb 2009 16:37:04 +0100 (CET)
Message-ID: <49871318.6040506@rt-labs.com>
Date: Mon, 02 Feb 2009 16:36:56 +0100
From: Hans-Erik Floryd <hans-erik.floryd@rt-labs.com>
MIME-Version: 1.0
Subject: Re: [Qemu-devel] PL181 write problem
References: <4981F759.2040803@rt-labs.com>
	<200901301440.47265.paul@codesourcery.com>
In-Reply-To: <200901301440.47265.paul@codesourcery.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Reply-To: qemu-devel@nongnu.org
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/pipermail/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: qemu-devel@nongnu.org

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