Re: [SeaBIOS] [PATCH] Fixes to the floppy driver, so it works on real (not emulated) hardware

[Kevin O'Connor]

On Tue, Jan 30, 2018 at 05:41:47AM +0200, Nikolay Nikolov wrote:
> Several fixes to the floppy driver, so it works on a real floppy controller
> with a real floppy. Tested with Coreboot on a motherboard with an ITE IT8712
> Super I/O chip (and its built-in floppy controller) and a 1.44MB floppy.
>
> Only one floppy is supported for now and only 1.44MB, but more formats will be
> added in the future.

Thanks.  See my comments below.
> --- a/src/hw/floppy.c
> +++ b/src/hw/floppy.c
> @@ -34,6 +34,11 @@
>  #define FLOPPY_GAPLEN 0x1B
>  #define FLOPPY_FORMAT_GAPLEN 0x6c
>  #define FLOPPY_PIO_TIMEOUT 1000
> +#define FLOPPY_IRQ_TIMEOUT 5000
> +#define FLOPPY_STARTUP_TIME 8
> +#define FLOPPY_MOTOR_HOLD 255 // Magic value, means motor must not be turned off
> +#define FLOPPY_SPECIFY1 0xAF  // step rate 12ms, head unload 240ms
> +#define FLOPPY_SPECIFY2 0x02  // head load time 4ms, DMA used

It would be good to also update diskette_param_table2 with these new
variables (as is done for the other variables).
> // New diskette parameter table adding 3 parameters from IBM
>  // Since no provisions are made for multiple drive types, most
> @@ -191,7 +196,8 @@ static void
>  floppy_disable_controller(void)
>  {
>      dprintf(2, "Floppy_disable_controller\n");
> floppy_dor_write(0x00);
> // Clear the reset bit (enter reset state) and clear 'enable IRQ and DMA'
> floppy_dor_write(GET_LOW(FloppyDOR) & ~0x0c);

Why do this?  What state needs to preserved in the DOR register on a
disable?
> @@ -199,8 +205,9 @@ floppy_wait_irq(void)
>  {
>      u8 frs = GET_BDA(floppy_recalibration_status);
>      SET_BDA(floppy_recalibration_status, frs & ~FRS_IRQ);
> u32 end = timer_calc(FLOPPY_IRQ_TIMEOUT);
> for (;;) {
>    if (!GET_BDA(floppy_motor_counter)) {
>    if (timer_check(end)) {

The floppy_motor_counter is an external variable that some legacy
programs may inspect.  I'm not sure we can not update it here.  What's
the reason for changing the timeout to use timer_calc()?
>          warn_timeout();
>          floppy_disable_controller();
>          return DISK_RET_ETIMEOUT;
>
> @@ -226,6 +233,7 @@ floppy_wait_irq(void)
>  #define FC_READ        (0xe6 | (8<<8) | (7<<12) | FCF_WAITIRQ)
>  #define FC_WRITE       (0xc5 | (8<<8) | (7<<12) | FCF_WAITIRQ)
>  #define FC_FORMAT      (0x4d | (5<<8) | (7<<12) | FCF_WAITIRQ)
> +#define FC_SPECIFY     (0x03 | (2<<8) | (0<<12))
>
> // Send the specified command and it's parameters to the floppy controller.
>  static int
> @@ -302,9 +310,12 @@ static int
>  floppy_enable_controller(void)
>  {
>      dprintf(2, "Floppy_enable_controller\n");
> SET_BDA(floppy_motor_counter, FLOPPY_MOTOR_TICKS);
> floppy_dor_write(0x00);
> floppy_dor_write(0x0c);
> // Clear the reset bit (enter reset state), but set 'enable IRQ and DMA'
> floppy_dor_write((GET_LOW(FloppyDOR) & ~0x04) | 0x08);
> // Real hardware needs a 4 microsecond delay
> udelay(4);

Can this be a usleep()?
> // Set the reset bit (normal operation) and keep 'enable IRQ and DMA' on
> floppy_dor_write(GET_LOW(FloppyDOR) | 0x0c);
> int ret = floppy_wait_irq();
> if (ret)
>     return ret;
>
> @@ -313,6 +324,30 @@ floppy_enable_controller(void)
>      return floppy_pio(FC_CHECKIRQ, param);
>  }
>
> +static void
> +floppy_turn_on_motor(u8 floppyid)
> +{
> // prevent the motor from being turned off
> SET_BDA(floppy_motor_counter, FLOPPY_MOTOR_HOLD);

I don't see a reference to 255 being a special value in the bios docs
for floppy_motor_counter.  As above, this is an external variable and
I'm not sure we can change it to custom values.  Did you find a
reference to it in a specification?
> u8 motor_mask = 0x10 << floppyid;
> if (GET_LOW(FloppyDOR) & motor_mask) {
>    // If the motor has been started, there's no need to wait for it again
>    floppy_dor_write(motor_mask | 0x0c | floppyid);
> } else {
>    floppy_dor_write(motor_mask | 0x0c | floppyid);
>    msleep(FLOPPY_STARTUP_TIME * 125);
> }
>
> +}
>
> +static void
> +floppy_turn_off_motor_delayed(void)
> +{
> // reset the disk motor timeout value of INT 08
> SET_BDA(floppy_motor_counter, FLOPPY_MOTOR_TICKS);
>
> +}
>
> // Activate a drive and send a command to it.
>  static int
>  floppy_drive_pio(u8 floppyid, int command, u8 *param)
> @@ -324,11 +359,8 @@ floppy_drive_pio(u8 floppyid, int command, u8 *param)
>              return ret;
>      }
> // reset the disk motor timeout value of INT 08
> SET_BDA(floppy_motor_counter, FLOPPY_MOTOR_TICKS);
> // Turn on motor of selected drive, DMA & int enabled, normal operation
> floppy_dor_write((floppyid ? 0x20 : 0x10) | 0x0c | floppyid);
>
> // Turn on motor of selected drive and wait for it to get up to speed
>
> floppy_turn_on_motor(floppyid);
>
> // Send command.
> int ret = floppy_pio(command, param);
>
> @@ -363,6 +395,15 @@ floppy_drive_recal(u8 floppyid)
>      return DISK_RET_SUCCESS;
>  }
>
> +static int
> +floppy_drive_specify(void)
> +{
> u8 param[2];
> param[0] = FLOPPY_SPECIFY1;
> param[1] = FLOPPY_SPECIFY2;
> return floppy_pio(FC_SPECIFY, param);
>
> +}
>
> static int
>  floppy_drive_readid(u8 floppyid, u8 data_rate, u8 head)
>  {
> @@ -433,13 +474,23 @@ floppy_prep(struct drive_s *drive_gf, u8 cylinder)
>          !(GET_BDA(floppy_media_state[floppyid]) & FMS_MEDIA_DRIVE_ESTABLISHED)) {
>          // Recalibrate drive.
>          int ret = floppy_drive_recal(floppyid);
>    if (ret)
>    if (ret) {
>        floppy_turn_off_motor_delayed();
>        return ret;
>    }
>
>    // Sense media.
>    ret = floppy_media_sense(drive_gf);
>    if (ret)
>    if (ret) {
>        floppy_turn_off_motor_delayed();
>        return ret;
>    }
>    ret = floppy_drive_specify();
>    if (ret) {
>        floppy_turn_off_motor_delayed();
>        return ret;
>    }
>
> }
>
> // Seek to cylinder if needed.
>
> @@ -449,8 +500,10 @@ floppy_prep(struct drive_s *drive_gf, u8 cylinder)
>          param[0] = floppyid;
>          param[1] = cylinder;
>          int ret = floppy_drive_pio(floppyid, FC_SEEK, param);
>    if (ret)
>    if (ret) {
>        floppy_turn_off_motor_delayed();
>        return ret;
>    }
>    SET_BDA(floppy_track[floppyid], cylinder);
> }
>
> @@ -489,9 +542,11 @@ floppy_dma_cmd(struct disk_op_s *op, int count, int command, u8 *param)
>          dprintf(1, "floppy error: %02x %02x %02x %02x %02x %02x %02x\n"
>                  , param[0], param[1], param[2], param[3]
>                  , param[4], param[5], param[6]);
>    floppy_turn_off_motor_delayed();
>    return DISK_RET_ECONTROLLER;
>
> }
>
> floppy_turn_off_motor_delayed();
> return DISK_RET_SUCCESS;
>
> }
>
> @@ -663,7 +718,7 @@ floppy_tick(void)
>  // time to turn off drive(s)?
>  u8 fcount = GET_BDA(floppy_motor_counter);
> if (fcount) {
> if (fcount && (fcount != FLOPPY_MOTOR_HOLD)) {
>     fcount--;
>     SET_BDA(floppy_motor_counter, fcount);
>     if (fcount == 0)

-Kevin