Re: [Qemu-devel] [PATCH v2 15/21] aspeed/smc: add support for DMAs

["Cédric Le Goater" <clg@kaod.org>]

On 01/07/2019 15:06, Peter Maydell wrote:
> On Tue, 18 Jun 2019 at 17:55, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> The FMC controller on the Aspeed SoCs support DMA to access the flash
>> modules. It can operate in a normal mode, to copy to or from the flash
>> module mapping window, or in a checksum calculation mode, to evaluate
>> the best clock settings for reads.
>>
>> The model introduces two custom address spaces for DMAs: one for the
>> AHB window of the FMC flash devices and one for the DRAM. The latter
>> is populated using a "dram" link set from the machine with the RAM
>> container region.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> Acked-by: Joel Stanley <joel@jms.id.au>
>> +/*
>> + * Accumulate the result of the reads to provide a checksum that will
>> + * be used to validate the read timing settings.
>> + */
>> +static void aspeed_smc_dma_checksum(AspeedSMCState *s)
>> +{
>> +    MemTxResult result;
>> +    uint32_t data;
>> +
>> +    if (s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "%s: invalid direction for DMA checksum\n",  __func__);
>> +        return;
>> +    }
>> +
>> +    while (s->regs[R_DMA_LEN]) {
>> +        result = address_space_read(&s->flash_as, s->regs[R_DMA_FLASH_ADDR],
>> +                                    MEMTXATTRS_UNSPECIFIED,
>> +                                    (uint8_t *)&data, 4);
> 
> This does a byte-by-byte read into a local uint32_t...
> 
>> +        if (result != MEMTX_OK) {
>> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: Flash read failed @%08x\n",
>> +                          __func__, s->regs[R_DMA_FLASH_ADDR]);
>> +            return;
>> +        }
>> +
>> +        /*
>> +         * When the DMA is on-going, the DMA registers are updated
>> +         * with the current working addresses and length.
>> +         */
>> +        s->regs[R_DMA_CHECKSUM] += data;
> 
> ...which we then use as a (host-endian) 32-bit value.
> 
> This will behave differently on big endian and little endian hosts.
> If the h/w behaviour is to to load a 32-bit data type you probably
> want address_space_ldl_le() (or _be() if it's doing a big-endian load).
> 
>> +        s->regs[R_DMA_FLASH_ADDR] += 4;
>> +        s->regs[R_DMA_LEN] -= 4;
>> +    }
>> +}
>> +
>> +static void aspeed_smc_dma_rw(AspeedSMCState *s)
>> +{
>> +    MemTxResult result;
>> +    uint32_t data;
>> +
>> +    while (s->regs[R_DMA_LEN]) {
>> +        if (s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE) {
>> +            result = address_space_read(&s->dram_as, s->regs[R_DMA_DRAM_ADDR],
>> +                                        MEMTXATTRS_UNSPECIFIED,
>> +                                        (uint8_t *)&data, 4);
>> +            if (result != MEMTX_OK) {
>> +                qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM read failed @%08x\n",
>> +                              __func__, s->regs[R_DMA_DRAM_ADDR]);
>> +                return;
>> +            }
>> +
>> +            result = address_space_write(&s->flash_as,
>> +                                         s->regs[R_DMA_FLASH_ADDR],
>> +                                         MEMTXATTRS_UNSPECIFIED,
>> +                                         (uint8_t *)&data, 4);
>> +            if (result != MEMTX_OK) {
>> +                qemu_log_mask(LOG_GUEST_ERROR, "%s: Flash write failed @%08x\n",
>> +                              __func__, s->regs[R_DMA_FLASH_ADDR]);
>> +                return;
>> +            }
>> +        } else {
>> +            result = address_space_read(&s->flash_as, s->regs[R_DMA_FLASH_ADDR],
>> +                                        MEMTXATTRS_UNSPECIFIED,
>> +                                        (uint8_t *)&data, 4);
>> +            if (result != MEMTX_OK) {
>> +                qemu_log_mask(LOG_GUEST_ERROR, "%s: Flash read failed @%08x\n",
>> +                              __func__, s->regs[R_DMA_FLASH_ADDR]);
>> +                return;
>> +            }
>> +
>> +            result = address_space_write(&s->dram_as, s->regs[R_DMA_DRAM_ADDR],
>> +                                         MEMTXATTRS_UNSPECIFIED,
>> +                                         (uint8_t *)&data, 4);
>> +            if (result != MEMTX_OK) {
>> +                qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM write failed @%08x\n",
>> +                              __func__, s->regs[R_DMA_DRAM_ADDR]);
>> +                return;
>> +            }
>> +        }
> 
> Since the code here doesn't do anything with the data the
> address_space_read/write here aren't wrong, but you might
> prefer to use the ldl and stl functions to avoid the casts
> to uint8_t* and need to specify the length.

yes. I will check with the HW guys to know precisely how the values are
read and accumulated.

Thanks,

C.