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

[Nikolay Nikolov]

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.

Signed-off-by: Nikolay Nikolov 
---
 src/hw/floppy.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 69 insertions(+), 14 deletions(-)

diff --git a/src/hw/floppy.c b/src/hw/floppy.c
index f2577c5..674909d 100644
--- 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

// 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);
 }

static int
@@ -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)) {
             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);
+    // 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);
+
+    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 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

[Nikolay Nikolov]

On Tue, 2018-01-30 at 22:23 -0500, Kevin O'Connor wrote:
> 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).

Ok, will do that.
> > // 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?

The motor state, at least. I also try to keep the currently selected
drive. Why? Because, floppy_disable_controller is called in case of
controller init, which is called in case of any error (DOS for example,
retries the read operation several times, reinitializing the disk
controller between each retry). In this case, we want to keep the
floppy motor spinning smoothly (so it keeps a constant speed), instead
of jittering it off and on quickly. As for the currently selected
drive, I keep it just in case. I'm not sure it's necessary. I keep it,
because, if e.g. DOS encounters an error reading drive B:, it will
issue a controller reset command and then retry the read from the same
drive. In that case, we won't change the currently selected drive and
this might help, in case there's some sort of time needed to change the
selected drive. For example, see "note2" in the wiki article, in the
section that describes the DIR register:

https://wiki.osdev.org/Floppy_Disk_Controller#DIR_register.2C_Disk_Chan
ge_bit

"Note2: You must turn on the drive motor bit before you access the DIR
register for a selected drive (you do not have to wait for the motor to
spin up, though). It may also be necessary to read the register five
times (discard the first 4 values) when changing the selected drive --
because "selecting" sometimes takes a little time."

So, changing the selected drive frequently might lead to such problems.
Note that I still haven't tested SeaBIOS with two floppies on a real
hardware, so I don't know if it works at all. I do have two floppies on
the motherboard I'm testing it on, but they are different formats (1st
is 3.5-inch 1.44MB, second is 5.25-inch 1.2MB), so before testing two
floppies, I need to add 1.2MB floppy support first, but I'm planning
all these things in subsequent patches. For now, I want to handle the
most likely scenario people are going to encounter - single 1.44MB
floppy.

So, if you exclude the motor state bits and the drive select bits, the
only two bits are the ones that I reset - one is the actual reset bit
(the only bit, that actually matters), second is the IRQ enable bit.
IMHO, it shouldn't really matter whether we disable the IRQ enable bit
or not - if the reset bit is cleared, the controller isn't supposed to
generate IRQs anyway. However, it doesn't hurt to disable it as well.
> > @@ -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()?

Previously, it didn't behave well on real hardware. It set the timer to
the FLOPPY_MOTOR_TICKS value in the beginning of the disk operation,
which turned off the motor way too early, before the operation can
complete. The value in FLOPPY_MOTOR_TICKS must be set _after_ the
operation, so it keeps the motor spinning for 2 seconds _after_ the
operation, so in case, there's a new disk operation that follows soon,
it keeps the motor spinning and avoids having to wait the spin-up time 
(FLOPPY_STARTUP_TIME) again.

However, since you mentioned it, I decided to do some actual tests on
computers with different BIOSes and inspect the value. I wrote a TSR,
which hooks the IRQ 0 timer interrupt and displays it in hex in the
upper left part of the screen. Then I watched what happens during and
after accessing the floppy and the results are not exactly what my
patch does. But, unfortunately the behaviour was different on the two
computers that I tested:

1) IBM PS/2 Model 76:

The value gets decremented all the time, even when there isn't any
floppy operation. So, when it reaches 0, the motors are turned off, but
the value wraps back to 0xFF and continues counting down back to 0,
where the motors are again turned off (even if they were already off),
ad infinitum.

In the beginning of an actual floppy operation, it is set to 0xFF, so
it doesn't turn off the motor any time soon. It still counts down,
however the BIOS doesn't seem to use it as a timeout.

After the operation, the value is changed to the FLOPPY_MOTOR_TICKS
constant, so it turns the motor off 2 seconds after the operation.

2) AWARD BIOS, Gigabyte GA-K8NF-9 rev 1.0 (the motherboard I'm porting
Coreboot+SeaBIOS to)

Same as the IBM PS/2 Model 76, except, when the value reaches 0, it
stays at 0.

The only difference with my patch is, my patch keeps the value at 0xFF,
during the operation, while the AWARD BIOS keeps counting down, even
though it still doesn't use it as timeout.

---

Honestly, I don't know which behaviour is more correct. I kinda like
AWARD's behaviour better, because it doesn't periodically poke into the
floppy controller's DOR register, when there's no floppy operation.

On the other hand, IBM could be more "IBM compatible" :)

I can also test Phoenix and AMI BIOS under AMD SimNow (not on real
hardware), since I don't have any computer with a floppy and any of
these BIOSes.

I also have other vintage IBMs with floppies, but I suspect they're
going to behave the same way as model 76.
> >          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

Nikolay

[Nikolay Nikolov]

On Sat, 2018-02-03 at 00:09 +0200, Nikolay Nikolov wrote:
> On Tue, 2018-01-30 at 22:23 -0500, Kevin O'Connor wrote:
> > > @@ -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()?
>
> Previously, it didn't behave well on real hardware. It set the timer
> to
> the FLOPPY_MOTOR_TICKS value in the beginning of the disk operation,
> which turned off the motor way too early, before the operation can
> complete. The value in FLOPPY_MOTOR_TICKS must be set _after_ the
> operation, so it keeps the motor spinning for 2 seconds _after_ the
> operation, so in case, there's a new disk operation that follows
> soon,
> it keeps the motor spinning and avoids having to wait the spin-up
> time 
> (FLOPPY_STARTUP_TIME) again.
>
> However, since you mentioned it, I decided to do some actual tests on
> computers with different BIOSes and inspect the value. I wrote a TSR,
> which hooks the IRQ 0 timer interrupt and displays it in hex in the
> upper left part of the screen. Then I watched what happens during and
> after accessing the floppy and the results are not exactly what my
> patch does. But, unfortunately the behaviour was different on the two
> computers that I tested:
> IBM PS/2 Model 76:
>
> The value gets decremented all the time, even when there isn't any
> floppy operation. So, when it reaches 0, the motors are turned off,
> but
> the value wraps back to 0xFF and continues counting down back to 0,
> where the motors are again turned off (even if they were already
> off),
> ad infinitum.
>
> In the beginning of an actual floppy operation, it is set to 0xFF, so
> it doesn't turn off the motor any time soon. It still counts down,
> however the BIOS doesn't seem to use it as a timeout.
>
> After the operation, the value is changed to the FLOPPY_MOTOR_TICKS
> constant, so it turns the motor off 2 seconds after the operation.
> AWARD BIOS, Gigabyte GA-K8NF-9 rev 1.0 (the motherboard I'm
>
> porting
> Coreboot+SeaBIOS to)
>
> Same as the IBM PS/2 Model 76, except, when the value reaches 0, it
> stays at 0.
>
> The only difference with my patch is, my patch keeps the value at
> 0xFF,
> during the operation, while the AWARD BIOS keeps counting down, even
> though it still doesn't use it as timeout.
>
> Honestly, I don't know which behaviour is more correct. I kinda like
> AWARD's behaviour better, because it doesn't periodically poke into
> the
> floppy controller's DOR register, when there's no floppy operation.
>
> On the other hand, IBM could be more "IBM compatible" :)
>
> I can also test Phoenix and AMI BIOS under AMD SimNow (not on real
> hardware), since I don't have any computer with a floppy and any of
> these BIOSes.
>
> I also have other vintage IBMs with floppies, but I suspect they're
> going to behave the same way as model 76.

Ok, tested "Phoenix ServerBIOS" and "AMI BIOS" and they behave like
AWARD. Also tested IBM 5150 (the first IBM PC), it behaves like the IBM
PS/2 Model 76. I guess, that's how early IBMs used to behave back then.
I guess, nowadays we'd better follow AWARD, Phoenix and AMI BIOS?

Nikolay

[Nikolay Nikolov]

Sorry, forgot to reply to one of the small points in my other mail.

On 01/31/2018 05:23 AM, Kevin O'Connor wrote:
> > // 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()?

Yes, it shouldn't be a problem.

Nikolay