On Thu, Dec 14, 2017 at 2:23 PM, Alistair Francis <alistair.fran...@xilinx.com> wrote: > On Wed, Dec 13, 2017 at 11:58 AM, Philippe Mathieu-Daudé > <f4...@amsat.org> wrote: >> This makes the code slightly safer, also easier to review. >> >> Signed-off-by: Philippe Mathieu-Daudé <f4...@amsat.org> >> --- >> hw/sd/sdhci.c | 23 +++++++---------------- >> 1 file changed, 7 insertions(+), 16 deletions(-) >> >> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c >> index 312b167bfa..e39623baba 100644 >> --- a/hw/sd/sdhci.c >> +++ b/hw/sd/sdhci.c >> @@ -603,14 +603,12 @@ typedef struct ADMADescr { >> >> static void get_adma_description(SDHCIState *s, ADMADescr *dscr) >> { >> - uint32_t adma1 = 0; >> - uint64_t adma2 = 0; >> + uint32_t adma1; >> + uint64_t adma2; >> hwaddr entry_addr = (hwaddr)s->admasysaddr; >> switch (SDHC_DMA_TYPE(s->hostctl)) { >> case SDHC_CTRL_ADMA2_32: >> - dma_memory_read(&address_space_memory, entry_addr, (uint8_t >> *)&adma2, >> - sizeof(adma2)); >> - adma2 = le64_to_cpu(adma2); >> + adma2 = ldq_le_dma(&address_space_memory, entry_addr); >> /* The spec does not specify endianness of descriptor table. >> * We currently assume that it is LE. >> */ >> @@ -620,9 +618,7 @@ static void get_adma_description(SDHCIState *s, >> ADMADescr *dscr) >> dscr->incr = 8; >> break; >> case SDHC_CTRL_ADMA1_32: >> - dma_memory_read(&address_space_memory, entry_addr, (uint8_t >> *)&adma1, >> - sizeof(adma1)); >> - adma1 = le32_to_cpu(adma1); >> + adma1 = ldl_le_dma(&address_space_memory, entry_addr); >> dscr->addr = (hwaddr)(adma1 & 0xFFFFF000); >> dscr->attr = (uint8_t)extract32(adma1, 0, 7); >> dscr->incr = 4; >> @@ -633,14 +629,9 @@ static void get_adma_description(SDHCIState *s, >> ADMADescr *dscr) >> } >> break; >> case SDHC_CTRL_ADMA2_64: >> - dma_memory_read(&address_space_memory, entry_addr, >> - (uint8_t *)(&dscr->attr), 1); >> - dma_memory_read(&address_space_memory, entry_addr + 2, >> - (uint8_t *)(&dscr->length), 2); >> - dscr->length = le16_to_cpu(dscr->length); >> - dma_memory_read(&address_space_memory, entry_addr + 4, >> - (uint8_t *)(&dscr->addr), 8); >> - dscr->attr = le64_to_cpu(dscr->attr); > >> + dscr->attr = ldub_dma(&address_space_memory, entry_addr); >> + dscr->length = lduw_le_dma(&address_space_memory, entry_addr + 2); >> + dscr->attr = ldq_le_dma(&address_space_memory, entry_addr + 4); > > Why this is being overwritten?
Do you mean 'rewritten'? > It looks like you have dropped an addr as well. Which addr? The diff is a bit compressed, if you separate it is more obvious: - dma_memory_read(&address_space_memory, entry_addr, - (uint8_t *)(&dscr->attr), 1); + dscr->attr = ldub_dma(&address_space_memory, entry_addr); - dma_memory_read(&address_space_memory, entry_addr + 2, - (uint8_t *)(&dscr->length), 2); - dscr->length = le16_to_cpu(dscr->length); + dscr->length = lduw_le_dma(&address_space_memory, entry_addr + 2); - dma_memory_read(&address_space_memory, entry_addr + 4, - (uint8_t *)(&dscr->addr), 8); - dscr->attr = le64_to_cpu(dscr->attr); + dscr->attr = ldq_le_dma(&address_space_memory, entry_addr + 4); The API is detailled in this file: docs/devel/loads-stores.rst I decided to rewrite this code with this API because it looks easier to understand (maybe once you read the API doc), so easier to review :)