On Sun, Sep 17, 2017 at 06:15:44PM +0100, Mark Cave-Ayland wrote: > Signed-off-by: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk>
Nice, but there are some details I'm not sure about. > --- > hw/ide/macio.c | 154 > +++++++++++++++++++++----------------------------------- > 1 file changed, 56 insertions(+), 98 deletions(-) > > diff --git a/hw/ide/macio.c b/hw/ide/macio.c > index db5db39..428fbfc 100644 > --- a/hw/ide/macio.c > +++ b/hw/ide/macio.c > @@ -255,131 +255,89 @@ static void pmac_ide_flush(DBDMA_io *io) > } > > /* PowerMac IDE memory IO */ > -static void pmac_ide_writeb (void *opaque, > - hwaddr addr, uint32_t val) > +static uint64_t pmac_ide_read(void *opaque, hwaddr addr, unsigned size) > { > MACIOIDEState *d = opaque; > + uint64_t retval; > + addr = (addr & 0xfff) >> 4; Do you need the mask? Using the new mmio interfaces, the given address should already be just an offset within the region. I also dislike re-using the 'addr' name when it's now essentially a register index. > > - addr = (addr & 0xFFF) >> 4; > switch (addr) { > - case 1 ... 7: > - ide_ioport_write(&d->bus, addr, val); > - break; > - case 8: > - case 22: > - ide_cmd_write(&d->bus, 0, val); > - break; > - default: > + case 0x0: > + if (size == 2) { > + retval = ide_data_readw(&d->bus, 0); > + } else if (size == 4) { > + retval = ide_data_readl(&d->bus, 0); > + } else { > + retval = 0xffffffff; Are single byte reads not permitted, or will they just return parts of the data? > + } > break; > - } > -} > - > -static uint32_t pmac_ide_readb (void *opaque,hwaddr addr) > -{ > - uint8_t retval; > - MACIOIDEState *d = opaque; > - > - addr = (addr & 0xFFF) >> 4; > - switch (addr) { > - case 1 ... 7: > + case 0x1 ... 0x7: > retval = ide_ioport_read(&d->bus, addr); > break; > - case 8: > - case 22: > + case 0x8: > + case 0x16: I think you need some sort of size check for these other registers - you've declared yourself as both supporting and implementing 1..4 byte accesses, but I don't think these make sense for size != 4. > retval = ide_status_read(&d->bus, 0); > break; > + case 0x20: > + retval = d->timing_reg; > + break; > + case 0x30: > + /* This is an interrupt state register that only exists > + * in the KeyLargo and later variants. Bit 0x8000_0000 > + * latches the DMA interrupt and has to be written to > + * clear. Bit 0x4000_0000 is an image of the disk > + * interrupt. MacOS X relies on this and will hang if > + * we don't provide at least the disk interrupt > + */ > + retval = d->irq_reg; > + break; > default: > - retval = 0xFF; > + retval = 0xffffffff; > break; > } > - return retval; > -} > - > -static void pmac_ide_writew (void *opaque, > - hwaddr addr, uint32_t val) > -{ > - MACIOIDEState *d = opaque; > - > - addr = (addr & 0xFFF) >> 4; > - val = bswap16(val); > - if (addr == 0) { > - ide_data_writew(&d->bus, 0, val); > - } > -} > > -static uint32_t pmac_ide_readw (void *opaque,hwaddr addr) > -{ > - uint16_t retval; > - MACIOIDEState *d = opaque; > - > - addr = (addr & 0xFFF) >> 4; > - if (addr == 0) { > - retval = ide_data_readw(&d->bus, 0); > - } else { > - retval = 0xFFFF; > - } > - retval = bswap16(retval); > return retval; > } > > -static void pmac_ide_writel (void *opaque, > - hwaddr addr, uint32_t val) > + > +static void pmac_ide_write(void *opaque, hwaddr addr, uint64_t val, > + unsigned size) Similar comments on the write path. > { > MACIOIDEState *d = opaque; > + addr = (addr & 0xfff) >> 4; > > - addr = (addr & 0xFFF) >> 4; > - val = bswap32(val); > - if (addr == 0) { > - ide_data_writel(&d->bus, 0, val); > - } else if (addr == 0x20) { > + switch (addr) { > + case 0x0: > + if (size == 2) { > + ide_data_writew(&d->bus, 0, val); > + } else if (size == 4) { > + ide_data_writel(&d->bus, 0, val); > + } > + break; > + case 0x1 ... 0x7: > + ide_ioport_write(&d->bus, addr, val); > + break; > + case 0x8: > + case 0x16: > + ide_cmd_write(&d->bus, 0, val); > + break; > + case 0x20: > d->timing_reg = val; > - } else if (addr == 0x30) { > + break; > + case 0x30: > if (val & 0x80000000u) { > d->irq_reg &= 0x7fffffff; > } > + break; > } > } > > -static uint32_t pmac_ide_readl (void *opaque,hwaddr addr) > -{ > - uint32_t retval; > - MACIOIDEState *d = opaque; > - > - addr = (addr & 0xFFF) >> 4; > - if (addr == 0) { > - retval = ide_data_readl(&d->bus, 0); > - } else if (addr == 0x20) { > - retval = d->timing_reg; > - } else if (addr == 0x30) { > - /* This is an interrupt state register that only exists > - * in the KeyLargo and later variants. Bit 0x8000_0000 > - * latches the DMA interrupt and has to be written to > - * clear. Bit 0x4000_0000 is an image of the disk > - * interrupt. MacOS X relies on this and will hang if > - * we don't provide at least the disk interrupt > - */ > - retval = d->irq_reg; > - } else { > - retval = 0xFFFFFFFF; > - } > - retval = bswap32(retval); > - return retval; > -} > - > static const MemoryRegionOps pmac_ide_ops = { > - .old_mmio = { > - .write = { > - pmac_ide_writeb, > - pmac_ide_writew, > - pmac_ide_writel, > - }, > - .read = { > - pmac_ide_readb, > - pmac_ide_readw, > - pmac_ide_readl, > - }, > - }, > - .endianness = DEVICE_NATIVE_ENDIAN, > + .read = pmac_ide_read, > + .write = pmac_ide_write, > + .valid.min_access_size = 1, > + .valid.max_access_size = 4, Remember you can set .impl. different from .valid if you want the mmio core to do the work of splitting accesses up. > + .endianness = DEVICE_LITTLE_ENDIAN, > }; > > static const VMStateDescription vmstate_pmac = { -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature