Re: [Qemu-devel] [PATCH v6 4/6] fdc: fix interrupt handling

[Pavel Hrdina]

On 06/25/2012 02:41 PM, Kevin Wolf wrote:
> Am 22.06.2012 12:33, schrieb Pavel Hrdina:
> > If you call the SENSE INTERRUPT STATUS command while there is no interrupt
> > waiting you get as result unknown command.
> >
> > Fixed status0 register handling for read/write/format commands.
> >
> > Signed-off-by: Pavel Hrdina<address@hidden>
> > ---
> >   hw/fdc.c |   34 +++++++++++++++++++++-------------
> >   1 files changed, 21 insertions(+), 13 deletions(-)
> >
> > diff --git a/hw/fdc.c b/hw/fdc.c
> > index 0270264..e28841c 100644
> > --- a/hw/fdc.c
> > +++ b/hw/fdc.c
> > @@ -307,6 +307,9 @@ enum {
> >   };
> >
> >   enum {
> > +    FD_SR0_DS0      = 0x01,
> > +    FD_SR0_DS1      = 0x02,
> > +    FD_SR0_HEAD     = 0x04,
> >       FD_SR0_EQPMT    = 0x10,
> >       FD_SR0_SEEK     = 0x20,
> >       FD_SR0_ABNTERM  = 0x40,
> > @@ -972,14 +975,15 @@ 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, int do_irq)
> > +static void fdctrl_set_fifo(FDCtrl *fdctrl, int fifo_len, uint8_t status0)
> >   {
> >       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 (do_irq)
> > -        fdctrl_raise_irq(fdctrl, 0x00);
> > +    if (status0) {
> > +        fdctrl_raise_irq(fdctrl, status0);
> > +    }
> Is status0 != 0 the real condition or is it just what we have here and
> what happens to give the right result with our implementation?
It's only to give the right result with our implementation. If you set 
second parameter to some value, it should be correct status0 value and 
then we raise irq.
> >   }
> >
> >   /* Set an error: unimplemented/unknown command */
> > @@ -1044,10 +1048,12 @@ 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);
> > +
> >       FLOPPY_DPRINTF("transfer status: %02x %02x %02x (%02x)\n",
> > -                   status0, status1, status2,
> > -                   status0 | (cur_drv->head<<  2) | GET_CUR_DRV(fdctrl));
> > -    fdctrl->fifo[0] = status0 | (cur_drv->head<<  2) | GET_CUR_DRV(fdctrl);
> > +                   status0, status1, status2, fdctrl->status0);
> > +    fdctrl->fifo[0] = fdctrl->status0;
> >       fdctrl->fifo[1] = status1;
> >       fdctrl->fifo[2] = status2;
> >       fdctrl->fifo[3] = cur_drv->track;
> > @@ -1060,7 +1066,7 @@ 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, 1);
> > +    fdctrl_set_fifo(fdctrl, 7, fdctrl->status0);
> >   }
> >
> >   /* Prepare a data transfer (either DMA or FIFO) */
> > @@ -1175,7 +1181,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, 0x00);
> > +    fdctrl_raise_irq(fdctrl, FD_SR0_SEEK);
> >
> >       return;
> >   }
> > @@ -1604,16 +1610,18 @@ static void fdctrl_handle_sense_interrupt_status(FDCtrl 
> > *fdctrl, int direction)
> >   {
> >       FDrive *cur_drv = get_cur_drv(fdctrl);
> >
> > -    if(fdctrl->reset_sensei>  0) {
> > +    if (fdctrl->reset_sensei>  0) {
> >           fdctrl->fifo[0] =
> >               FD_SR0_RDYCHG + FD_RESET_SENSEI_COUNT - fdctrl->reset_sensei;
> >           fdctrl->reset_sensei--;
> > +    } else if (!(fdctrl->sra&  FD_SRA_INTPEND)) {
> > +        fdctrl->fifo[0] = FD_SR0_INVCMD;
> > +        fdctrl_set_fifo(fdctrl, 1, 0);
> > +        return;
> >       } else {
> > -        /* XXX: status0 handling is broken for read/write
> > -           commands, so we do this hack. It should be suppressed
> > -           ASAP */
> >           fdctrl->fifo[0] =
> > -            FD_SR0_SEEK | (cur_drv->head<<  2) | GET_CUR_DRV(fdctrl);
> > +                (fdctrl->status0&  ~(FD_SR0_HEAD | FD_SR0_DS1 | FD_SR0_DS0))
> > +                | GET_CUR_DRV(fdctrl);
> Why isn't fdctrl->status0 already right?
>
> In any case, I think if you're masking out the head number from status0,
> you need to or it back, even if from a different source.
>
> Kevin
This command always returns head == 0, that's why I only masking out the 
head bit and not setting it again.

Some commands raise irq setting only irq information and no drive 
information. Because of that I set it again for sure.

Pavel