Re: [PATCH v2 00/42] esp: consolidate PDMA transfer buffers and other fixes

[Philippe Mathieu-Daudé]

On 2/25/21 10:54 AM, Mark Cave-Ayland wrote:
> On 23/02/2021 21:32, Philippe Mathieu-Daudé wrote:
> 
>> Hi Mark,
>>
>> On 2/9/21 8:29 PM, Mark Cave-Ayland wrote:
>>> This patch series comes from an experimental branch that I've been
>>> working on
>>> to try and boot a MacOS toolbox ROM under the QEMU q800 machine. The
>>> effort is
>>> far from complete, but it seems worth submitting these patches
>>> separately since
>>> they are limited to the ESP device and form a substantial part of the
>>> work to
>>> date.
>>>
>>> As part of Laurent's recent q800 work so-called PDMA (pseudo-DMA)
>>> support was
>>> added to the ESP device. This is whereby the DREQ (DMA request) line
>>> is used
>>> to signal to the host CPU that it can transfer data to/from the
>>> device over
>>> the SCSI bus.
>>>
>>> The existing PDMA tracks 4 separate transfer data sources as
>>> indicated by the
>>> ESP pdma_origin variable: PDMA, TI, CMD and ASYNC with an independent
>>> variable
>>> pdma_len to store the transfer length. This works well with Linux
>>> which uses a
>>> single PDMA request to transfer a number of sectors in a single request.
>>>
>>> Unfortunately the MacOS toolbox ROM has other ideas here: it sends
>>> data to the
>>> ESP as a mixture of FIFO and PDMA transfers and then uses a mixture
>>> of the FIFO
>>> and DMA counters to confirm that the correct number of bytes have been
>>> transferred. For this to work correctly the PDMA buffers and separate
>>> pdma_len
>>> transfer counter must be consolidated into the FIFO to allow mixing
>>> of both
>>> types of transfer within a single request.
>>>
>>> The patchset is split into several sections:
>>>
>>> - Patches 1-7 are minor patches which make esp.c checkpatch friendly,
>>> QOMify ESPState,
>>>    and also fix up some trace events ready for later patches in the
>>> series
>>>
>>> - Patches 8-13 unify the DMA transfer count. In particular there are
>>> 2 synthetic
>>>    variables dma_counter and dma_left within ESPState which do not
>>> need to exist.
>>>    DMA transfer lengths are programmed into the TC (transfer count)
>>> register which is
>>>    decremented for each byte transferred, generating an interrupt
>>> when it reaches zero.
>>>    These patches add helper functions to read the TC and STC
>>> registers directly and
>>>    remove these synthetic variables so that the DMA transfer length
>>> is now tracked in
>>>    a single place.
>>>
>>> - Now that the TC register represents the authoritative DMA transfer
>>> length, patches
>>>    14-25 work to eliminate the separate PDMA variables pdma_start,
>>> pdma_cur, pdma_len
>>>    and separate PDMA buffers PDMA and CMD. The PDMA position
>>> variables can be replaced
>>>    by the existing ESP cmdlen and ti_wptr/ti_rptr, whilst the FIFO
>>> (TI) buffer is used
>>>    for incoming data with commands being accumulated in cmdbuf as per
>>> standard DMA
>>>    requests.
>>
>> I tried to help reviewing up to this point.
>>
>> The next parts are too specific to me.
> 
> Thanks Phil - I understand that a set of 42 patches for a 25 year old
> disk controller is never going to be the top of most people's review
> list, and some parts are almost impossible to review unless you have a
> good understanding of the datasheet.

Well I also have a series for a 30+ years old MIPS board and am
not confident to post it because probably little interest for
the community, although it is very interesting to compare with
actual SoC and see how the IP blocks are indeed reused and improved
over the time -- or not... i.e. when someone report a hw bug in a 2020
product and the same bug is present in the IP core from the 80th it
inherited ;)

> I'll see if Laurent has any comments over the next few days, but other
> than that I'd be inclined to send a v3 followed soon by a PR to avoid me
> having to update these regularly (I already see a slight conflict with
> Paolo's SCSI error handling changes, for example).

I'll have a look at your v3 and Cc you when I post this MIPS board :D

Regards,

Phil.