Re: [Qemu-devel] [12 patches] FDC: improve emulation

[Blue Swirl]

On 4/16/08, Hervé Poussineau <address@hidden> wrote:
> Hi,
>
>  I've done big improvements on floppy drive controller.
>  For easier reviewing, I've tried to split them up in differents parts.
> Sorry if some parts are still big.
>  Each patch applies on top of the previous one.
>
>  If only some of them are accepted, I can provide a global patch for only
> these ones. Patches which needs more work will be resubmitted later.
>
>  List of attached patches:
>
>  fdc_01_lookuptable.diff
>  - Adds a command lookup table, as suggested by Fabrice at
> http://lists.gnu.org/archive/html/qemu-devel/2008-04/msg00143.html
>  - This also moves initialization functions at the bottom of the file to
> prevent multiple forward declarations.

The command_to_handler table size should be 256.

>  fdc_02_seek_next_sector.diff
>  - Extract seeking to next sector handling in a function. Add a sector seek
> in PIO read and write modes

Looks OK.

>  fdc_03_status_ab.diff
>  - Fixes status A and status B registers. It removes one Sun4m mutation.
> Also removes the internal FD_CTRL_INTR flag.

OK.

>  fdc_04_status_012.diff
>  - Fixes status0, status1 and status2 handling, which were broken in
> read/write commands and in some subtle ways. Status are now calculated
> during command execution and not at the very end.
>  - This allows removing of one hack in
> fdctrl_handle_sense_interrupt_status().

Not good, this makes Sparc64 print
SENSEI c0 00
SENSEI c0 00
forever.

>  fdc_05_dmaen.diff
>  - Handles correctly FD_MSR_NONDMA/FD_DOR_NONDMA flags, and uses them when
> possible. Fixes a problem with SPECIFY command. This allows removing of last
> Sun4m mutation.

I'd still preserve the comment, because it's still true even if there
are no more mutations. On Sun4m DMAGATE bit doesn't control DMA but
enables interrupts, shouldn't that be kept?

>  fdc_06_dor_reg.diff
>  - Better handling of DOR register. DOR register drives external motors, but
> it not limited to existing drives.
>  - Use FD_DOR_nRESET flag instead of internal FD_CTRL_RESET flag.
>  - Support writing to DOR register even in reset mode (as said in
> specification)

No idea.

>  fdc_07_msr_reg.diff
>  - Stores controller state in MSR register instead of internal state field.
> This simplifies the fdctrl_read_main_status() function, which may be called
> in some tight loops.

          */
-        if (FD_STATE(fdctrl->data_state) == FD_STATE_DATA)
+        if (++fdctrl->data_pos == fdctrl->data_len)
             fdctrl_stop_transfer(fdctrl);

Would it be better to make the test >=?

>  fdc_08_cleanup.diff
>  - Removes useless fields in fdrive_t structure.
>  - Adds a message when bdrv_read/bdrv_write calls fail.

The version number could be handy if some machine wants a specific
type of controller, I'd preserve it.

>  fdc_09_tdr_reg.diff
>  - Replaces bootsel field by the whole tdr register. It may be easier if we
> want to later add support for tapes.

OK.

>  fdc_10_4_fds.diff
>  - Supports up to 4 floppy drives if MAX_FD is set to 4.

OK.

>  fdc_11_cur_drv.diff
>  - Replaces access to cur_drv field by macros.

OK.

>  fdc_12_pio_write.diff
>  - Fixes transfer data length.
>  - Fixes buffer overflow in PIO mode write.

The CUR_DRV changes could be merged to 11, other look OK. I'd separate
the buffer overflow fix so that it can be applied to the stable
version.