fdc: Fix FD_SR0_SEEK flag
cover fdc: Fix FD_SR0_SEEK flag
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(-)
Signature · 1 line
1.7.6.5
PATCH 1/3 fdc: Remove status0 parameter from fdctrl_set_fifo()
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.
--- hw/fdc.c | 33 ++++++++++++++++----------------- 1 files changed, 16 insertions(+), 17 deletions(-)
hw/fdc.c +16 -17
@@ -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 ); } }
Signature · 1 line
1.7.6.5
PATCH 2/3 fdc: Fix false FD_SR0_SEEK
In reply to Kevin Wolffdctrl_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.
--- hw/fdc.c | 5 ++--- tests/fdc-test.c | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-)
hw/fdc.c +2 -3
@@ -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 ; }
tests/fdc-test.c +1 -1
@@ -152,7 +152,7 @@ static uint8_t send_read_command(void) } st0 =floppy_recv (); -if (st0 !=0x60 ) { +if (st0 !=0x40 ) { ret =1 ; }
Signature · 1 line
1.7.6.5
Re: [Qemu-devel] [PATCH 2/3] fdc: Fix false FD_SR0_SEEK
Quoted 42 lines · Kevin Wolf
fdctrl_start/stop_transfer() used to set
FD_SR0_SEEKno 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 settingFD_SR0_SEEKwhere appropriate.Signed-off-by Kevin Wolf <[email protected]>--- hw/fdc.c | 5 ++--- tests/fdc-test.c | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-)hw/fdc.c +2
-3@@ -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 ; }tests/fdc-test.c +0
-0
tests/fdc-test.c static uint8_t send_read_command(void) · line 158
Quoted 8 lines
tests/fdc-test.c +1
-1@@ -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é
PATCH 3/3 fdc-test: Check READ ID
In reply to Kevin WolfST0 shouldn't include 0x20 (FD_SR0_SEEK) after READ ID.
--- tests/fdc-test.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 66 insertions(+), 0 deletions(-)
tests/fdc-test.c +66 -0
@@ -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 ();
Signature · 1 line
1.7.6.5
Quoted 11 lines · Kevin Wolf
Pavel, does this make sense to you? It seems to fix the NT 3.1 floppy driver.
Kevin Wolf (3):
fdc: Remove status0 parameter fromfdctrl_set_fifo()
fdc: Fix falseFD_SR0_SEEK
fdc-test: Check READ IDhw/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.