| Message ID | 1432049762-2184-7-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 05/19/2015 11:36 AM, Kevin Wolf wrote: > This commit makes similar improvements as have already been made to the > write function: Instead of relying on a flag in the MSR to distinguish > controller phases, use the explicit phase that we store now. Assertions > of the right MSR flags are added. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > hw/block/fdc.c | 33 +++++++++++++++++++++++---------- > 1 file changed, 23 insertions(+), 10 deletions(-) > > diff --git a/hw/block/fdc.c b/hw/block/fdc.c > index cbf7abf..8d322e0 100644 > --- a/hw/block/fdc.c > +++ b/hw/block/fdc.c > @@ -1533,9 +1533,16 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl) > FLOPPY_DPRINTF("error: controller not ready for reading\n"); > return 0; > } > + > + /* If data_len spans multiple sectors, the current position in the FIFO > + * wraps around while fdctrl->data_pos is the real position in the whole > + * request. */ > pos = fdctrl->data_pos; > pos %= FD_SECTOR_LEN; > - if (fdctrl->msr & FD_MSR_NONDMA) { > + > + switch (fdctrl->phase) { > + case FD_PHASE_EXECUTION: > + assert(fdctrl->msr & FD_MSR_NONDMA); > if (pos == 0) { > if (fdctrl->data_pos != 0) > if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) { > @@ -1551,20 +1558,26 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl) > memset(fdctrl->fifo, 0, FD_SECTOR_LEN); > } > } > - } > - retval = fdctrl->fifo[pos]; > - if (++fdctrl->data_pos == fdctrl->data_len) { > - fdctrl->data_pos = 0; I suppose data_pos is now reset by either stop_transfer (via to_result_phase) or to_command_phase, so this is OK. > - /* Switch from transfer mode to status mode > - * then from status mode to command mode > - */ > - if (fdctrl->msr & FD_MSR_NONDMA) { > + > + if (++fdctrl->data_pos == fdctrl->data_len) { > fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00); > - } else { > + } > + break; > + > + case FD_PHASE_RESULT: > + assert(!(fdctrl->msr & FD_MSR_NONDMA)); > + if (++fdctrl->data_pos == fdctrl->data_len) { Not a terribly big fan of moving this pointer independently inside of each case statement, but I guess the alternative does look a lot worse. Having things separated by phases is a lot easier to follow. > fdctrl_to_command_phase(fdctrl); > fdctrl_reset_irq(fdctrl); > } > + break; > + > + case FD_PHASE_COMMAND: > + default: > + abort(); > } > + > + retval = fdctrl->fifo[pos]; > FLOPPY_DPRINTF("data register: 0x%02x\n", retval); > > return retval; > Reviewed-by: John Snow <jsnow@redhat.com>
Am 19.05.2015 um 22:40 hat John Snow geschrieben: > > > On 05/19/2015 11:36 AM, Kevin Wolf wrote: > > This commit makes similar improvements as have already been made to the > > write function: Instead of relying on a flag in the MSR to distinguish > > controller phases, use the explicit phase that we store now. Assertions > > of the right MSR flags are added. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > hw/block/fdc.c | 33 +++++++++++++++++++++++---------- > > 1 file changed, 23 insertions(+), 10 deletions(-) > > > > diff --git a/hw/block/fdc.c b/hw/block/fdc.c > > index cbf7abf..8d322e0 100644 > > --- a/hw/block/fdc.c > > +++ b/hw/block/fdc.c > > @@ -1533,9 +1533,16 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl) > > FLOPPY_DPRINTF("error: controller not ready for reading\n"); > > return 0; > > } > > + > > + /* If data_len spans multiple sectors, the current position in the FIFO > > + * wraps around while fdctrl->data_pos is the real position in the whole > > + * request. */ > > pos = fdctrl->data_pos; > > pos %= FD_SECTOR_LEN; > > - if (fdctrl->msr & FD_MSR_NONDMA) { > > + > > + switch (fdctrl->phase) { > > + case FD_PHASE_EXECUTION: > > + assert(fdctrl->msr & FD_MSR_NONDMA); > > if (pos == 0) { > > if (fdctrl->data_pos != 0) > > if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) { > > @@ -1551,20 +1558,26 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl) > > memset(fdctrl->fifo, 0, FD_SECTOR_LEN); > > } > > } > > - } > > - retval = fdctrl->fifo[pos]; > > - if (++fdctrl->data_pos == fdctrl->data_len) { > > - fdctrl->data_pos = 0; > > I suppose data_pos is now reset by either stop_transfer (via > to_result_phase) or to_command_phase, so this is OK. Yes, that was redundant code. > > - /* Switch from transfer mode to status mode > > - * then from status mode to command mode > > - */ > > - if (fdctrl->msr & FD_MSR_NONDMA) { > > + > > + if (++fdctrl->data_pos == fdctrl->data_len) { > > fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00); > > - } else { > > + } > > + break; > > + > > + case FD_PHASE_RESULT: > > + assert(!(fdctrl->msr & FD_MSR_NONDMA)); > > + if (++fdctrl->data_pos == fdctrl->data_len) { > > Not a terribly big fan of moving this pointer independently inside of > each case statement, but I guess the alternative does look a lot worse. > Having things separated by phases is a lot easier to follow. I'm not too happy about it either, but I couldn't think of anything better. Having two different switches almost immediately after each other, with only the if line in between, would look really awkward and be hard to read. And the old code isn't nice either. If you have any idea for a better solution, let me know. Kevin
On 05/20/2015 04:25 AM, Kevin Wolf wrote: > Am 19.05.2015 um 22:40 hat John Snow geschrieben: >> >> >> On 05/19/2015 11:36 AM, Kevin Wolf wrote: >>> This commit makes similar improvements as have already been made to the >>> write function: Instead of relying on a flag in the MSR to distinguish >>> controller phases, use the explicit phase that we store now. Assertions >>> of the right MSR flags are added. >>> >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >>> --- >>> hw/block/fdc.c | 33 +++++++++++++++++++++++---------- >>> 1 file changed, 23 insertions(+), 10 deletions(-) >>> >>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c >>> index cbf7abf..8d322e0 100644 >>> --- a/hw/block/fdc.c >>> +++ b/hw/block/fdc.c >>> @@ -1533,9 +1533,16 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl) >>> FLOPPY_DPRINTF("error: controller not ready for reading\n"); >>> return 0; >>> } >>> + >>> + /* If data_len spans multiple sectors, the current position in the FIFO >>> + * wraps around while fdctrl->data_pos is the real position in the whole >>> + * request. */ >>> pos = fdctrl->data_pos; >>> pos %= FD_SECTOR_LEN; >>> - if (fdctrl->msr & FD_MSR_NONDMA) { >>> + >>> + switch (fdctrl->phase) { >>> + case FD_PHASE_EXECUTION: >>> + assert(fdctrl->msr & FD_MSR_NONDMA); >>> if (pos == 0) { >>> if (fdctrl->data_pos != 0) >>> if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) { >>> @@ -1551,20 +1558,26 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl) >>> memset(fdctrl->fifo, 0, FD_SECTOR_LEN); >>> } >>> } >>> - } >>> - retval = fdctrl->fifo[pos]; >>> - if (++fdctrl->data_pos == fdctrl->data_len) { >>> - fdctrl->data_pos = 0; >> >> I suppose data_pos is now reset by either stop_transfer (via >> to_result_phase) or to_command_phase, so this is OK. > > Yes, that was redundant code. > >>> - /* Switch from transfer mode to status mode >>> - * then from status mode to command mode >>> - */ >>> - if (fdctrl->msr & FD_MSR_NONDMA) { >>> + >>> + if (++fdctrl->data_pos == fdctrl->data_len) { >>> fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00); >>> - } else { >>> + } >>> + break; >>> + >>> + case FD_PHASE_RESULT: >>> + assert(!(fdctrl->msr & FD_MSR_NONDMA)); >>> + if (++fdctrl->data_pos == fdctrl->data_len) { >> >> Not a terribly big fan of moving this pointer independently inside of >> each case statement, but I guess the alternative does look a lot worse. >> Having things separated by phases is a lot easier to follow. > > I'm not too happy about it either, but I couldn't think of anything > better. Having two different switches almost immediately after each > other, with only the if line in between, would look really awkward and > be hard to read. And the old code isn't nice either. > > If you have any idea for a better solution, let me know. > > Kevin > I'm all complaints and no solutions. I believe I gave you my R-b anyway. :)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c index cbf7abf..8d322e0 100644 --- a/hw/block/fdc.c +++ b/hw/block/fdc.c @@ -1533,9 +1533,16 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl) FLOPPY_DPRINTF("error: controller not ready for reading\n"); return 0; } + + /* If data_len spans multiple sectors, the current position in the FIFO + * wraps around while fdctrl->data_pos is the real position in the whole + * request. */ pos = fdctrl->data_pos; pos %= FD_SECTOR_LEN; - if (fdctrl->msr & FD_MSR_NONDMA) { + + switch (fdctrl->phase) { + case FD_PHASE_EXECUTION: + assert(fdctrl->msr & FD_MSR_NONDMA); if (pos == 0) { if (fdctrl->data_pos != 0) if (!fdctrl_seek_to_next_sect(fdctrl, cur_drv)) { @@ -1551,20 +1558,26 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl) memset(fdctrl->fifo, 0, FD_SECTOR_LEN); } } - } - retval = fdctrl->fifo[pos]; - if (++fdctrl->data_pos == fdctrl->data_len) { - fdctrl->data_pos = 0; - /* Switch from transfer mode to status mode - * then from status mode to command mode - */ - if (fdctrl->msr & FD_MSR_NONDMA) { + + if (++fdctrl->data_pos == fdctrl->data_len) { fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00); - } else { + } + break; + + case FD_PHASE_RESULT: + assert(!(fdctrl->msr & FD_MSR_NONDMA)); + if (++fdctrl->data_pos == fdctrl->data_len) { fdctrl_to_command_phase(fdctrl); fdctrl_reset_irq(fdctrl); } + break; + + case FD_PHASE_COMMAND: + default: + abort(); } + + retval = fdctrl->fifo[pos]; FLOPPY_DPRINTF("data register: 0x%02x\n", retval); return retval;
This commit makes similar improvements as have already been made to the write function: Instead of relying on a flag in the MSR to distinguish controller phases, use the explicit phase that we store now. Assertions of the right MSR flags are added. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- hw/block/fdc.c | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-)