[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 6/8] fdc: Disentangle phases in fdctrl_read_data
From: |
John Snow |
Subject: |
Re: [Qemu-devel] [PATCH 6/8] fdc: Disentangle phases in fdctrl_read_data() |
Date: |
Tue, 19 May 2015 16:40:39 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 |
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 <address@hidden>
> ---
> 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 <address@hidden>
[Qemu-devel] [PATCH 8/8] fdc-test: Test state for existing cases more thoroughly, Kevin Wolf, 2015/05/19
Re: [Qemu-devel] [PATCH 0/8] fdc: Clean up and fix command processing, John Snow, 2015/05/19