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 <kw...@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. :)
