[Qemu-devel] [PATCH 0/3] fdc: Fix FD_SR0

All of lore.kernel.org
 help / color / mirror / Atom feed

* [Qemu-devel] [PATCH 0/3] fdc: Fix FD_SR0_SEEK flag
@ 2012-09-04  9:46 Kevin Wolf
  2012-09-04  9:46 ` [Qemu-devel] [PATCH 1/3] fdc: Remove status0 parameter from fdctrl_set_fifo() Kevin Wolf
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Kevin Wolf @ 2012-09-04  9:46 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, phrdina

Pavel, does this make sense to you? It seems to fix the NT 3.1 floppy driver.

Kevin Wolf (3):
  fdc: Remove status0 parameter from fdctrl_set_fifo()
  fdc: Fix false FD_SR0_SEEK
  fdc-test: Check READ ID

 hw/fdc.c         |   38 ++++++++++++++----------------
 tests/fdc-test.c |   68 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 85 insertions(+), 21 deletions(-)

-- 
1.7.6.5

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [Qemu-devel] [PATCH 1/3] fdc: Remove status0 parameter from fdctrl_set_fifo()
  2012-09-04  9:46 [Qemu-devel] [PATCH 0/3] fdc: Fix FD_SR0_SEEK flag Kevin Wolf
@ 2012-09-04  9:46 ` Kevin Wolf
  2012-09-04  9:46 ` [Qemu-devel] [PATCH 2/3] fdc: Fix false FD_SR0_SEEK Kevin Wolf
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 6+ messages in thread
From: Kevin Wolf @ 2012-09-04  9:46 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, phrdina

It decided whether an interrupt is triggered. Only one caller made use
of this functionality, so move the code there.

In this one caller, the interrupt must actually be triggered
unconditionally, like it was before commit 2fee0088. For example, a
successful read without an implied seek can result in st0 = 0, but still
triggers the interrupt.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/fdc.c |   33 ++++++++++++++++-----------------
 1 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/hw/fdc.c b/hw/fdc.c
index 08830c1..78ae064 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -1079,15 +1079,12 @@ static void fdctrl_reset_fifo(FDCtrl *fdctrl)
 }
 
 /* Set FIFO status for the host to read */
-static void fdctrl_set_fifo(FDCtrl *fdctrl, int fifo_len, uint8_t status0)
+static void fdctrl_set_fifo(FDCtrl *fdctrl, int fifo_len)
 {
     fdctrl->data_dir = FD_DIR_READ;
     fdctrl->data_len = fifo_len;
     fdctrl->data_pos = 0;
     fdctrl->msr |= FD_MSR_CMDBUSY | FD_MSR_RQM | FD_MSR_DIO;
-    if (status0) {
-        fdctrl_raise_irq(fdctrl, status0);
-    }
 }
 
 /* Set an error: unimplemented/unknown command */
@@ -1096,7 +1093,7 @@ static void fdctrl_unimplemented(FDCtrl *fdctrl, int direction)
     qemu_log_mask(LOG_UNIMP, "fdc: unimplemented command 0x%02x\n",
                   fdctrl->fifo[0]);
     fdctrl->fifo[0] = FD_SR0_INVCMD;
-    fdctrl_set_fifo(fdctrl, 1, 0);
+    fdctrl_set_fifo(fdctrl, 1);
 }
 
 /* Seek to next sector
@@ -1170,7 +1167,9 @@ static void fdctrl_stop_transfer(FDCtrl *fdctrl, uint8_t status0,
     }
     fdctrl->msr |= FD_MSR_RQM | FD_MSR_DIO;
     fdctrl->msr &= ~FD_MSR_NONDMA;
-    fdctrl_set_fifo(fdctrl, 7, fdctrl->status0);
+
+    fdctrl_set_fifo(fdctrl, 7);
+    fdctrl_raise_irq(fdctrl, fdctrl->status0);
 }
 
 /* Prepare a data transfer (either DMA or FIFO) */
@@ -1538,7 +1537,7 @@ static void fdctrl_handle_lock(FDCtrl *fdctrl, int direction)
 {
     fdctrl->lock = (fdctrl->fifo[0] & 0x80) ? 1 : 0;
     fdctrl->fifo[0] = fdctrl->lock << 4;
-    fdctrl_set_fifo(fdctrl, 1, 0);
+    fdctrl_set_fifo(fdctrl, 1);
 }
 
 static void fdctrl_handle_dumpreg(FDCtrl *fdctrl, int direction)
@@ -1563,20 +1562,20 @@ static void fdctrl_handle_dumpreg(FDCtrl *fdctrl, int direction)
         (cur_drv->perpendicular << 2);
     fdctrl->fifo[8] = fdctrl->config;
     fdctrl->fifo[9] = fdctrl->precomp_trk;
-    fdctrl_set_fifo(fdctrl, 10, 0);
+    fdctrl_set_fifo(fdctrl, 10);
 }
 
 static void fdctrl_handle_version(FDCtrl *fdctrl, int direction)
 {
     /* Controller's version */
     fdctrl->fifo[0] = fdctrl->version;
-    fdctrl_set_fifo(fdctrl, 1, 0);
+    fdctrl_set_fifo(fdctrl, 1);
 }
 
 static void fdctrl_handle_partid(FDCtrl *fdctrl, int direction)
 {
     fdctrl->fifo[0] = 0x41; /* Stepping 1 */
-    fdctrl_set_fifo(fdctrl, 1, 0);
+    fdctrl_set_fifo(fdctrl, 1);
 }
 
 static void fdctrl_handle_restore(FDCtrl *fdctrl, int direction)
@@ -1629,7 +1628,7 @@ static void fdctrl_handle_save(FDCtrl *fdctrl, int direction)
     fdctrl->fifo[12] = fdctrl->pwrd;
     fdctrl->fifo[13] = 0;
     fdctrl->fifo[14] = 0;
-    fdctrl_set_fifo(fdctrl, 15, 0);
+    fdctrl_set_fifo(fdctrl, 15);
 }
 
 static void fdctrl_handle_readid(FDCtrl *fdctrl, int direction)
@@ -1695,7 +1694,7 @@ static void fdctrl_handle_sense_drive_status(FDCtrl *fdctrl, int direction)
         (cur_drv->head << 2) |
         GET_CUR_DRV(fdctrl) |
         0x28;
-    fdctrl_set_fifo(fdctrl, 1, 0);
+    fdctrl_set_fifo(fdctrl, 1);
 }
 
 static void fdctrl_handle_recalibrate(FDCtrl *fdctrl, int direction)
@@ -1720,7 +1719,7 @@ static void fdctrl_handle_sense_interrupt_status(FDCtrl *fdctrl, int direction)
         fdctrl->reset_sensei--;
     } else if (!(fdctrl->sra & FD_SRA_INTPEND)) {
         fdctrl->fifo[0] = FD_SR0_INVCMD;
-        fdctrl_set_fifo(fdctrl, 1, 0);
+        fdctrl_set_fifo(fdctrl, 1);
         return;
     } else {
         fdctrl->fifo[0] =
@@ -1729,7 +1728,7 @@ static void fdctrl_handle_sense_interrupt_status(FDCtrl *fdctrl, int direction)
     }
 
     fdctrl->fifo[1] = cur_drv->track;
-    fdctrl_set_fifo(fdctrl, 2, 0);
+    fdctrl_set_fifo(fdctrl, 2);
     fdctrl_reset_irq(fdctrl);
     fdctrl->status0 = FD_SR0_RDYCHG;
 }
@@ -1771,7 +1770,7 @@ static void fdctrl_handle_powerdown_mode(FDCtrl *fdctrl, int direction)
 {
     fdctrl->pwrd = fdctrl->fifo[1];
     fdctrl->fifo[0] = fdctrl->fifo[1];
-    fdctrl_set_fifo(fdctrl, 1, 0);
+    fdctrl_set_fifo(fdctrl, 1);
 }
 
 static void fdctrl_handle_option(FDCtrl *fdctrl, int direction)
@@ -1790,7 +1789,7 @@ static void fdctrl_handle_drive_specification_command(FDCtrl *fdctrl, int direct
             fdctrl->fifo[0] = fdctrl->fifo[1];
             fdctrl->fifo[2] = 0;
             fdctrl->fifo[3] = 0;
-            fdctrl_set_fifo(fdctrl, 4, 0);
+            fdctrl_set_fifo(fdctrl, 4);
         } else {
             fdctrl_reset_fifo(fdctrl);
         }
@@ -1798,7 +1797,7 @@ static void fdctrl_handle_drive_specification_command(FDCtrl *fdctrl, int direct
         /* ERROR */
         fdctrl->fifo[0] = 0x80 |
             (cur_drv->head << 2) | GET_CUR_DRV(fdctrl);
-        fdctrl_set_fifo(fdctrl, 1, 0);
+        fdctrl_set_fifo(fdctrl, 1);
     }
 }
 
-- 
1.7.6.5

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [Qemu-devel] [PATCH 2/3] fdc: Fix false FD_SR0_SEEK
  2012-09-04  9:46 [Qemu-devel] [PATCH 0/3] fdc: Fix FD_SR0_SEEK flag Kevin Wolf
  2012-09-04  9:46 ` [Qemu-devel] [PATCH 1/3] fdc: Remove status0 parameter from fdctrl_set_fifo() Kevin Wolf
@ 2012-09-04  9:46 ` Kevin Wolf
  2012-09-06 19:17   ` Hervé Poussineau
  2012-09-04  9:46 ` [Qemu-devel] [PATCH 3/3] fdc-test: Check READ ID Kevin Wolf
  2012-09-05  9:28 ` [Qemu-devel] [PATCH 0/3] fdc: Fix FD_SR0_SEEK flag Pavel Hrdina
  3 siblings, 1 reply; 6+ messages in thread
From: Kevin Wolf @ 2012-09-04  9:46 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, phrdina

fdctrl_start/stop_transfer() used to set FD_SR0_SEEK no matter if
there actually was a seek or not. This is obviously wrong.

fdctrl_start_transfer() has this information because it performs the
seek itself. fdctrl_stop_transfer() gets status0 passed and callers
already take care of setting FD_SR0_SEEK where appropriate.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/fdc.c         |    5 ++---
 tests/fdc-test.c |    2 +-
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/fdc.c b/hw/fdc.c
index 78ae064..8ea3341 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -1149,8 +1149,7 @@ static void fdctrl_stop_transfer(FDCtrl *fdctrl, uint8_t status0,
     FDrive *cur_drv;
 
     cur_drv = get_cur_drv(fdctrl);
-    fdctrl->status0 = status0 | FD_SR0_SEEK | (cur_drv->head << 2) |
-                      GET_CUR_DRV(fdctrl);
+    fdctrl->status0 = status0 | (cur_drv->head << 2) | GET_CUR_DRV(fdctrl);
 
     FLOPPY_DPRINTF("transfer status: %02x %02x %02x (%02x)\n",
                    status0, status1, status2, fdctrl->status0);
@@ -1284,7 +1283,7 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
     if (direction != FD_DIR_WRITE)
         fdctrl->msr |= FD_MSR_DIO;
     /* IO based transfer: calculate len */
-    fdctrl_raise_irq(fdctrl, FD_SR0_SEEK);
+    fdctrl_raise_irq(fdctrl, did_seek ? FD_SR0_SEEK : 0);
 
     return;
 }
diff --git a/tests/fdc-test.c b/tests/fdc-test.c
index fa74411..ff96560 100644
--- a/tests/fdc-test.c
+++ b/tests/fdc-test.c
@@ -152,7 +152,7 @@ static uint8_t send_read_command(void)
     }
 
     st0 = floppy_recv();
-    if (st0 != 0x60) {
+    if (st0 != 0x40) {
         ret = 1;
     }
 
-- 
1.7.6.5

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH 2/3] fdc: Fix false FD_SR0_SEEK
  2012-09-04  9:46 ` [Qemu-devel] [PATCH 2/3] fdc: Fix false FD_SR0_SEEK Kevin Wolf
@ 2012-09-06 19:17   ` Hervé Poussineau
  0 siblings, 0 replies; 6+ messages in thread
From: Hervé Poussineau @ 2012-09-06 19:17 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: phrdina, qemu-devel

Kevin Wolf a écrit :
> fdctrl_start/stop_transfer() used to set FD_SR0_SEEK no matter if
> there actually was a seek or not. This is obviously wrong.
>
> fdctrl_start_transfer() has this information because it performs the
> seek itself. fdctrl_stop_transfer() gets status0 passed and callers
> already take care of setting FD_SR0_SEEK where appropriate.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/fdc.c         |    5 ++---
>  tests/fdc-test.c |    2 +-
>  2 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/hw/fdc.c b/hw/fdc.c
> index 78ae064..8ea3341 100644
> --- a/hw/fdc.c
> +++ b/hw/fdc.c
> @@ -1149,8 +1149,7 @@ static void fdctrl_stop_transfer(FDCtrl *fdctrl, uint8_t status0,
>      FDrive *cur_drv;
>  
>      cur_drv = get_cur_drv(fdctrl);
> -    fdctrl->status0 = status0 | FD_SR0_SEEK | (cur_drv->head << 2) |
> -                      GET_CUR_DRV(fdctrl);
> +    fdctrl->status0 = status0 | (cur_drv->head << 2) | GET_CUR_DRV(fdctrl);
>  
>      FLOPPY_DPRINTF("transfer status: %02x %02x %02x (%02x)\n",
>                     status0, status1, status2, fdctrl->status0);
> @@ -1284,7 +1283,7 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
>      if (direction != FD_DIR_WRITE)
>          fdctrl->msr |= FD_MSR_DIO;
>      /* IO based transfer: calculate len */
> -    fdctrl_raise_irq(fdctrl, FD_SR0_SEEK);
> +    fdctrl_raise_irq(fdctrl, did_seek ? FD_SR0_SEEK : 0);
>  
>      return;
>  }
> diff --git a/tests/fdc-test.c b/tests/fdc-test.c
> index fa74411..ff96560 100644
> --- a/tests/fdc-test.c
> +++ b/tests/fdc-test.c
> @@ -152,7 +152,7 @@ static uint8_t send_read_command(void)
>      }
>  
>      st0 = floppy_recv();
> -    if (st0 != 0x60) {
> +    if (st0 != 0x40) {
>          ret = 1;
>      }
>  
>   

NACK for this patch.
It fixes the seek flags DMA transfers, but does not handles PIO transfer 
case.
I just sent a whole serie for floppy improvements on ML, with a fixed 
version of this patch.

Hervé

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [Qemu-devel] [PATCH 3/3] fdc-test: Check READ ID
  2012-09-04  9:46 [Qemu-devel] [PATCH 0/3] fdc: Fix FD_SR0_SEEK flag Kevin Wolf
  2012-09-04  9:46 ` [Qemu-devel] [PATCH 1/3] fdc: Remove status0 parameter from fdctrl_set_fifo() Kevin Wolf
  2012-09-04  9:46 ` [Qemu-devel] [PATCH 2/3] fdc: Fix false FD_SR0_SEEK Kevin Wolf
@ 2012-09-04  9:46 ` Kevin Wolf
  2012-09-05  9:28 ` [Qemu-devel] [PATCH 0/3] fdc: Fix FD_SR0_SEEK flag Pavel Hrdina
  3 siblings, 0 replies; 6+ messages in thread
From: Kevin Wolf @ 2012-09-04  9:46 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, phrdina

ST0 shouldn't include 0x20 (FD_SR0_SEEK) after READ ID.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/fdc-test.c |   66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 66 insertions(+), 0 deletions(-)

diff --git a/tests/fdc-test.c b/tests/fdc-test.c
index ff96560..b825959 100644
--- a/tests/fdc-test.c
+++ b/tests/fdc-test.c
@@ -48,6 +48,7 @@ enum {
 
 enum {
     CMD_SENSE_INT           = 0x08,
+    CMD_READ_ID             = 0x0a,
     CMD_SEEK                = 0x0f,
     CMD_READ                = 0xe6,
     CMD_RELATIVE_SEEK_OUT   = 0x8f,
@@ -320,6 +321,70 @@ static void test_relative_seek(void)
     g_assert(pcn == 0);
 }
 
+static void test_read_id(void)
+{
+    uint8_t drive = 0;
+    uint8_t head = 0;
+    uint8_t cyl;
+    uint8_t st0;
+
+    /* Seek to track 0 and check with READ ID */
+    send_seek(0);
+
+    floppy_send(CMD_READ_ID);
+    g_assert(!get_irq(FLOPPY_IRQ));
+    floppy_send(head << 2 | drive);
+
+    while (!get_irq(FLOPPY_IRQ)) {
+        /* qemu involves a timer with READ ID... */
+        clock_step(1000000000LL / 50);
+    }
+
+    st0 = floppy_recv();
+    floppy_recv();
+    floppy_recv();
+    cyl = floppy_recv();
+    head = floppy_recv();
+    floppy_recv();
+    floppy_recv();
+
+    g_assert_cmpint(cyl, ==, 0);
+    g_assert_cmpint(head, ==, 0);
+    g_assert_cmpint(st0, ==, head << 2);
+
+    /* Seek to track 8 on head 1 and check with READ ID */
+    head = 1;
+    cyl = 8;
+
+    floppy_send(CMD_SEEK);
+    floppy_send(head << 2 | drive);
+    g_assert(!get_irq(FLOPPY_IRQ));
+    floppy_send(cyl);
+    g_assert(get_irq(FLOPPY_IRQ));
+    ack_irq(NULL);
+
+    floppy_send(CMD_READ_ID);
+    g_assert(!get_irq(FLOPPY_IRQ));
+    floppy_send(head << 2 | drive);
+
+    while (!get_irq(FLOPPY_IRQ)) {
+        /* qemu involves a timer with READ ID... */
+        clock_step(1000000000LL / 50);
+    }
+
+    st0 = floppy_recv();
+    floppy_recv();
+    floppy_recv();
+    cyl = floppy_recv();
+    head = floppy_recv();
+    floppy_recv();
+    floppy_recv();
+
+    g_assert_cmpint(cyl, ==, 8);
+    g_assert_cmpint(head, ==, 1);
+    g_assert_cmpint(st0, ==, head << 2);
+}
+
 /* success if no crash or abort */
 static void fuzz_registers(void)
 {
@@ -369,6 +434,7 @@ int main(int argc, char **argv)
     qtest_add_func("/fdc/media_change", test_media_change);
     qtest_add_func("/fdc/sense_interrupt", test_sense_interrupt);
     qtest_add_func("/fdc/relative_seek", test_relative_seek);
+    qtest_add_func("/fdc/read_id", test_read_id);
     qtest_add_func("/fdc/fuzz-registers", fuzz_registers);
 
     ret = g_test_run();
-- 
1.7.6.5

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH 0/3] fdc: Fix FD_SR0_SEEK flag
  2012-09-04  9:46 [Qemu-devel] [PATCH 0/3] fdc: Fix FD_SR0_SEEK flag Kevin Wolf
                   ` (2 preceding siblings ...)
  2012-09-04  9:46 ` [Qemu-devel] [PATCH 3/3] fdc-test: Check READ ID Kevin Wolf
@ 2012-09-05  9:28 ` Pavel Hrdina
  3 siblings, 0 replies; 6+ messages in thread
From: Pavel Hrdina @ 2012-09-05  9:28 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: qemu-devel

On 09/04/2012 11:46 AM, Kevin Wolf wrote:
> Pavel, does this make sense to you? It seems to fix the NT 3.1 floppy driver.
>
> Kevin Wolf (3):
>    fdc: Remove status0 parameter from fdctrl_set_fifo()
>    fdc: Fix false FD_SR0_SEEK
>    fdc-test: Check READ ID
>
>   hw/fdc.c         |   38 ++++++++++++++----------------
>   tests/fdc-test.c |   68 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>   2 files changed, 85 insertions(+), 21 deletions(-)
>

Hi Kevin, yes, it make sense to me. It's definitely better now.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2012-09-06 19:17 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-04  9:46 [Qemu-devel] [PATCH 0/3] fdc: Fix FD_SR0_SEEK flag Kevin Wolf
2012-09-04  9:46 ` [Qemu-devel] [PATCH 1/3] fdc: Remove status0 parameter from fdctrl_set_fifo() Kevin Wolf
2012-09-04  9:46 ` [Qemu-devel] [PATCH 2/3] fdc: Fix false FD_SR0_SEEK Kevin Wolf
2012-09-06 19:17   ` Hervé Poussineau
2012-09-04  9:46 ` [Qemu-devel] [PATCH 3/3] fdc-test: Check READ ID Kevin Wolf
2012-09-05  9:28 ` [Qemu-devel] [PATCH 0/3] fdc: Fix FD_SR0_SEEK flag Pavel Hrdina

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.