[1/2] hw/mips/jazz: create ESP device directly via qdev

MIPS jazz is the last user of the legacy esp_init() function so move creation
of the ESP device over to use qdev.

Note that the esp_reset and dma_enable qemu_irqs are currently unused and so
we do not wire these up and instead remove the variables to prevent the
compiler emitting unused variable warnings.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/mips/mips_jazz.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

On 13/06/2018 11:47, Mark Cave-Ayland wrote:
> +    dev = qdev_create(NULL, TYPE_ESP);
> +    sysbus_esp = ESP_STATE(dev);
> +    esp = &sysbus_esp->esp;
> +    esp->dma_memory_read = rc4030_dma_read;
> +    esp->dma_memory_write = rc4030_dma_write;
> +    esp->dma_opaque = dmas[0];

Poking at the functions here is a bit ugly, and it's the last user of
rc4030_dma_{read,write}.  It would be nicer if ESP could get a memory
region like it's done a bit above for the NIC.  I guess it's not a big
deal, but perhaps there could be a TODO comment.

I'm mostly mentioning this because Hervé is copied and because SPARC DMA
has the same issue of using function pointers instead of an IOMMU memory
region...

Thanks,

Paolo

> +    sysbus_esp->it_shift = 0;
> +    /* XXX for now until rc4030 has been changed to use DMA enable signal */
> +    esp->dma_enabled = 1;
> +    qdev_init_nofail(dev);
> +
> +    sysbus = SYS_BUS_DEVICE(dev);
> +    sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(rc4030, 5));
> +    sysbus_mmio_map(sysbus, 0, 0x80002000);
> +

On 13/06/18 11:06, Paolo Bonzini wrote:

> On 13/06/2018 11:47, Mark Cave-Ayland wrote:
>> +    dev = qdev_create(NULL, TYPE_ESP);
>> +    sysbus_esp = ESP_STATE(dev);
>> +    esp = &sysbus_esp->esp;
>> +    esp->dma_memory_read = rc4030_dma_read;
>> +    esp->dma_memory_write = rc4030_dma_write;
>> +    esp->dma_opaque = dmas[0];
> 
> Poking at the functions here is a bit ugly, and it's the last user of
> rc4030_dma_{read,write}.  It would be nicer if ESP could get a memory
> region like it's done a bit above for the NIC.  I guess it's not a big
> deal, but perhaps there could be a TODO comment.

Okay - I'll wait for Hervé to confirm it passes his tests and then see 
if wants a respin with a TODO added (or can add it himself).

> I'm mostly mentioning this because Hervé is copied and because SPARC DMA
> has the same issue of using function pointers instead of an IOMMU memory
> region...

Ahhh this is a fun one :)

As part of the SPARC32 cleanups for the last release there is now a 
proper sun4m IOMMU implementation, but from memory the big issue was 
that the DMA functions need access to the device state to affect the 
transfer.

Check out hw/dma/sparc32_dma.c for some ugly examples: 
espdma_memory_read()/espdma_memory_write() update the DMA address 
pointer register after each read/write, and 
ledma_memory_read()/ledma_memory_write() need to determine if the pcnet 
card has byte-swapping enabled: if so, then don't perform the required 
lance 16-bit swap and if not, do perform the 16-bit swap.

So yes, it's fairly ugly but I can't think of good solution involving 
just IOMMU memory regions.


ATB,

Mark.

On 13/06/2018 12:36, Mark Cave-Ayland wrote:
> Check out hw/dma/sparc32_dma.c for some ugly examples:
> espdma_memory_read()/espdma_memory_write() update the DMA address
> pointer register after each read/write, and
> ledma_memory_read()/ledma_memory_write() need to determine if the pcnet
> card has byte-swapping enabled: if so, then don't perform the required
> lance 16-bit swap and if not, do perform the 16-bit swap.

Heh, those are disgusting indeed. :)  So I guess it would have to stay,
only MIPS can use the pure MemoryRegion-based approach.

Regarding pcnet, is CSR_BSWP really a no-op on PCI cards?  If not, an
option could be to move that handling to pcnet.c - making the ledma swap
unconditional and removing the do_bswap argument.  The disadvantage is
that SPARC would swap twice, and you'd have to keep the callback because
of s->dmaregs[3], but maybe it's still worthwhile.

Thanks,

Paolo

On 13/06/18 12:19, Paolo Bonzini wrote:

> On 13/06/2018 12:36, Mark Cave-Ayland wrote:
>> Check out hw/dma/sparc32_dma.c for some ugly examples:
>> espdma_memory_read()/espdma_memory_write() update the DMA address
>> pointer register after each read/write, and
>> ledma_memory_read()/ledma_memory_write() need to determine if the pcnet
>> card has byte-swapping enabled: if so, then don't perform the required
>> lance 16-bit swap and if not, do perform the 16-bit swap.
> 
> Heh, those are disgusting indeed. :)  So I guess it would have to stay,
> only MIPS can use the pure MemoryRegion-based approach.

The only option I can think of is inserting an AddressSpace between the 
esp/ledma device and the IOMMU AddressSpace which intercepts the DMA 
request (addr, len, direction).

I can then grab a reference to the device from the MemoryRegion opaque, 
perform the magic, and then manually invoke address_space_rw() on iommu_as.

Is there a hook somewhere in the memory API that could allow me to do this?

> Regarding pcnet, is CSR_BSWP really a no-op on PCI cards?  If not, an
> option could be to move that handling to pcnet.c - making the ledma swap
> unconditional and removing the do_bswap argument.  The disadvantage is
> that SPARC would swap twice, and you'd have to keep the callback because
> of s->dmaregs[3], but maybe it's still worthwhile.

Hmmm good question. If we can intercept the request above, that would be 
my preferred option as something tells it me it might be useful for 
other similar situations.


ATB,

Mark.

On 13/06/2018 15:06, Mark Cave-Ayland wrote:
>>
>> Heh, those are disgusting indeed. :)  So I guess it would have to stay,
>> only MIPS can use the pure MemoryRegion-based approach.
> 
> The only option I can think of is inserting an AddressSpace between the
> esp/ledma device and the IOMMU AddressSpace which intercepts the DMA
> request (addr, len, direction).
> 
> I can then grab a reference to the device from the MemoryRegion opaque,
> perform the magic, and then manually invoke address_space_rw() on iommu_as.
> 
> Is there a hook somewhere in the memory API that could allow me to do this?

No, I don't think so.  Only MMIO regions intercept reads/writes, and
they only do it at 1/2/4/8 byte granularity.

>> Regarding pcnet, is CSR_BSWP really a no-op on PCI cards?  If not, an
>> option could be to move that handling to pcnet.c - making the ledma swap
>> unconditional and removing the do_bswap argument.  The disadvantage is
>> that SPARC would swap twice, and you'd have to keep the callback because
>> of s->dmaregs[3], but maybe it's still worthwhile.
> 
> Hmmm good question. If we can intercept the request above, that would be
> my preferred option as something tells it me it might be useful for
> other similar situations.

Yeah, it wouldn't be a great improvement, but there would be the benefit
of more accurate emulation.

Paolo