| Message ID | 20230609185119.691152-6-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | cmd646: move device-specific BMDMA registers to separate memory region | expand |
Am 9. Juni 2023 18:51:19 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>: >The aim here is to eliminate any device-specific registers from the main BMDMA >bar memory region so it can potentially be moved into the shared PCI IDE device. > >For each BMDMA bus create a new cmd646-bmdma-specific memory region representing >the device-specific BMDMA registers and then map them using aliases onto the >existing BMDMAState memory region. > >Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >--- > hw/ide/cmd646.c | 111 +++++++++++++++++++++++++++++++--------- > include/hw/ide/cmd646.h | 4 ++ > 2 files changed, 90 insertions(+), 25 deletions(-) > >diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c >index 9103da581f..dd495f2e1b 100644 >--- a/hw/ide/cmd646.c >+++ b/hw/ide/cmd646.c >@@ -90,7 +90,6 @@ static uint64_t bmdma_read(void *opaque, hwaddr addr, > unsigned size) > { > BMDMAState *bm = opaque; >- PCIDevice *pci_dev = PCI_DEVICE(bm->pci_dev); > uint32_t val; > > if (size != 1) { >@@ -101,19 +100,9 @@ static uint64_t bmdma_read(void *opaque, hwaddr addr, > case 0: > val = bm->cmd; > break; >- case 1: >- val = pci_dev->config[MRDMODE]; >- break; > case 2: > val = bm->status; > break; >- case 3: >- if (bm == &bm->pci_dev->bmdma[0]) { >- val = pci_dev->config[UDIDETCR0]; >- } else { >- val = pci_dev->config[UDIDETCR1]; >- } >- break; > default: > val = 0xff; > break; >@@ -127,7 +116,6 @@ static void bmdma_write(void *opaque, hwaddr addr, > uint64_t val, unsigned size) > { > BMDMAState *bm = opaque; >- PCIDevice *pci_dev = PCI_DEVICE(bm->pci_dev); > > if (size != 1) { > return; >@@ -138,23 +126,10 @@ static void bmdma_write(void *opaque, hwaddr addr, > case 0: > bmdma_cmd_writeb(bm, val); > break; >- case 1: >- pci_dev->config[MRDMODE] = >- (pci_dev->config[MRDMODE] & ~0x30) | (val & 0x30); >- cmd646_update_dma_interrupts(pci_dev); >- cmd646_update_irq(pci_dev); >- break; > case 2: > bm->status = (val & 0x60) | (bm->status & 1) | > (bm->status & ~val & 0x06); > break; >- case 3: >- if (bm == &bm->pci_dev->bmdma[0]) { >- pci_dev->config[UDIDETCR0] = val; >- } else { >- pci_dev->config[UDIDETCR1] = val; >- } >- break; > } > } > >@@ -181,6 +156,91 @@ static void bmdma_setup_bar(PCIIDEState *d) > } > } > >+static uint64_t cmd646_bmdma_read(void *opaque, hwaddr addr, unsigned size) >+{ >+ BMDMAState *bm = opaque; >+ PCIDevice *pci_dev = PCI_DEVICE(bm->pci_dev); >+ uint32_t val; >+ >+ if (size != 1) { >+ return ((uint64_t)1 << (size * 8)) - 1; >+ } >+ >+ switch (addr & 3) { >+ case 1: >+ val = pci_dev->config[MRDMODE]; >+ break; >+ case 3: >+ if (bm == &bm->pci_dev->bmdma[0]) { >+ val = pci_dev->config[UDIDETCR0]; >+ } else { >+ val = pci_dev->config[UDIDETCR1]; >+ } >+ break; >+ default: >+ val = 0xff; >+ break; >+ } >+ >+ trace_bmdma_read_cmd646(addr, val); >+ return val; >+} >+ >+static void cmd646_bmdma_write(void *opaque, hwaddr addr, uint64_t val, >+ unsigned size) >+{ >+ BMDMAState *bm = opaque; >+ PCIDevice *pci_dev = PCI_DEVICE(bm->pci_dev); >+ >+ if (size != 1) { >+ return; >+ } >+ >+ trace_bmdma_write_cmd646(addr, val); >+ switch (addr & 3) { >+ case 1: >+ pci_dev->config[MRDMODE] = >+ (pci_dev->config[MRDMODE] & ~0x30) | (val & 0x30); >+ cmd646_update_dma_interrupts(pci_dev); >+ cmd646_update_irq(pci_dev); >+ break; >+ case 3: >+ if (bm == &bm->pci_dev->bmdma[0]) { >+ pci_dev->config[UDIDETCR0] = val; >+ } else { >+ pci_dev->config[UDIDETCR1] = val; >+ } >+ break; >+ } >+} >+ >+static const MemoryRegionOps cmd646_bmdma_ops = { >+ .read = cmd646_bmdma_read, >+ .write = cmd646_bmdma_write, >+}; >+ >+static void cmd646_bmdma_setup(PCIIDEState *d) >+{ >+ CMD646IDEState *s = CMD646_IDE(d); >+ BMDMAState *bm; >+ int i; >+ >+ /* Setup aliases for device-specific BMDMA registers */ >+ for (i = 0; i < 2; i++) { I'd use `ARRAY_SIZE()` instead of the hardcoded constant here. >+ bm = &d->bmdma[i]; >+ memory_region_init_io(&s->bmdma_mem[i], OBJECT(d), &cmd646_bmdma_ops, >+ bm, "cmd646-bmdma", 4); >+ memory_region_init_alias(&s->bmdma_mem_alias[i][0], OBJECT(d), >+ "cmd646-bmdma[1]", &s->bmdma_mem[i], 0x1, 1); >+ memory_region_add_subregion(&bm->extra_io, 0x1, >+ &s->bmdma_mem_alias[i][0]); >+ memory_region_init_alias(&s->bmdma_mem_alias[i][1], OBJECT(d), >+ "cmd646-bmdma[3]", &s->bmdma_mem[i], 0x3, 1); >+ memory_region_add_subregion(&bm->extra_io, 0x3, >+ &s->bmdma_mem_alias[i][1]); >+ } >+} >+ > static void cmd646_update_irq(PCIDevice *pd) > { > int pci_level; >@@ -289,6 +349,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp) > > bmdma_setup_bar(d); > pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar); >+ cmd646_bmdma_setup(d); > > /* TODO: RST# value should be 0 */ > pci_conf[PCI_INTERRUPT_PIN] = 0x01; /* interrupt on pin 1 */ >diff --git a/include/hw/ide/cmd646.h b/include/hw/ide/cmd646.h >index 4780b1132c..5329964533 100644 >--- a/include/hw/ide/cmd646.h >+++ b/include/hw/ide/cmd646.h >@@ -29,10 +29,14 @@ > #define TYPE_CMD646_IDE "cmd646-ide" > OBJECT_DECLARE_SIMPLE_TYPE(CMD646IDEState, CMD646_IDE) > >+#include "exec/memory.h" > #include "hw/ide/pci.h" > > struct CMD646IDEState { > PCIIDEState parent_obj; >+ >+ MemoryRegion bmdma_mem[2]; >+ MemoryRegion bmdma_mem_alias[2][2]; The added complexity of a two-dimensional alias array seems like a tough call for me. I'm not totally against it but I'm reluctant. Best regards, Bernhard > }; > > #endif
On 12/6/23 21:28, Bernhard Beschow wrote: > > > Am 9. Juni 2023 18:51:19 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>: >> The aim here is to eliminate any device-specific registers from the main BMDMA >> bar memory region so it can potentially be moved into the shared PCI IDE device. >> >> For each BMDMA bus create a new cmd646-bmdma-specific memory region representing >> the device-specific BMDMA registers and then map them using aliases onto the >> existing BMDMAState memory region. >> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> --- >> hw/ide/cmd646.c | 111 +++++++++++++++++++++++++++++++--------- >> include/hw/ide/cmd646.h | 4 ++ >> 2 files changed, 90 insertions(+), 25 deletions(-) >> struct CMD646IDEState { >> PCIIDEState parent_obj; >> + >> + MemoryRegion bmdma_mem[2]; >> + MemoryRegion bmdma_mem_alias[2][2]; > > The added complexity of a two-dimensional alias array seems like a tough call for me. I'm not totally against it but I'm reluctant. Alternative: struct { MemoryRegion mem; MemoryRegion mem_alias[2]; } bmdma[2]; >> }; >> >> #endif >
On Tue, Jun 13, 2023 at 12:39 AM Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > On 12/6/23 21:28, Bernhard Beschow wrote: > > > > > > Am 9. Juni 2023 18:51:19 UTC schrieb Mark Cave-Ayland < > mark.cave-ayland@ilande.co.uk>: > >> The aim here is to eliminate any device-specific registers from the > main BMDMA > >> bar memory region so it can potentially be moved into the shared PCI > IDE device. > >> > >> For each BMDMA bus create a new cmd646-bmdma-specific memory region > representing > >> the device-specific BMDMA registers and then map them using aliases > onto the > >> existing BMDMAState memory region. > >> > >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > >> --- > >> hw/ide/cmd646.c | 111 +++++++++++++++++++++++++++++++--------- > >> include/hw/ide/cmd646.h | 4 ++ > >> 2 files changed, 90 insertions(+), 25 deletions(-) > > > >> struct CMD646IDEState { > >> PCIIDEState parent_obj; > >> + > >> + MemoryRegion bmdma_mem[2]; > >> + MemoryRegion bmdma_mem_alias[2][2]; > > > > The added complexity of a two-dimensional alias array seems like a tough > call for me. I'm not totally against it but I'm reluctant. > > Alternative: > > struct { > MemoryRegion mem; > MemoryRegion mem_alias[2]; > If `mem_alias` became an anonymous struct as well we could avoid fiddling with two indices in a matrix, lowering the complexity. Best regards, Bernhard > } bmdma[2]; > > >> }; > >> > >> #endif > > > >
diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c index 9103da581f..dd495f2e1b 100644 --- a/hw/ide/cmd646.c +++ b/hw/ide/cmd646.c @@ -90,7 +90,6 @@ static uint64_t bmdma_read(void *opaque, hwaddr addr, unsigned size) { BMDMAState *bm = opaque; - PCIDevice *pci_dev = PCI_DEVICE(bm->pci_dev); uint32_t val; if (size != 1) { @@ -101,19 +100,9 @@ static uint64_t bmdma_read(void *opaque, hwaddr addr, case 0: val = bm->cmd; break; - case 1: - val = pci_dev->config[MRDMODE]; - break; case 2: val = bm->status; break; - case 3: - if (bm == &bm->pci_dev->bmdma[0]) { - val = pci_dev->config[UDIDETCR0]; - } else { - val = pci_dev->config[UDIDETCR1]; - } - break; default: val = 0xff; break; @@ -127,7 +116,6 @@ static void bmdma_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) { BMDMAState *bm = opaque; - PCIDevice *pci_dev = PCI_DEVICE(bm->pci_dev); if (size != 1) { return; @@ -138,23 +126,10 @@ static void bmdma_write(void *opaque, hwaddr addr, case 0: bmdma_cmd_writeb(bm, val); break; - case 1: - pci_dev->config[MRDMODE] = - (pci_dev->config[MRDMODE] & ~0x30) | (val & 0x30); - cmd646_update_dma_interrupts(pci_dev); - cmd646_update_irq(pci_dev); - break; case 2: bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06); break; - case 3: - if (bm == &bm->pci_dev->bmdma[0]) { - pci_dev->config[UDIDETCR0] = val; - } else { - pci_dev->config[UDIDETCR1] = val; - } - break; } } @@ -181,6 +156,91 @@ static void bmdma_setup_bar(PCIIDEState *d) } } +static uint64_t cmd646_bmdma_read(void *opaque, hwaddr addr, unsigned size) +{ + BMDMAState *bm = opaque; + PCIDevice *pci_dev = PCI_DEVICE(bm->pci_dev); + uint32_t val; + + if (size != 1) { + return ((uint64_t)1 << (size * 8)) - 1; + } + + switch (addr & 3) { + case 1: + val = pci_dev->config[MRDMODE]; + break; + case 3: + if (bm == &bm->pci_dev->bmdma[0]) { + val = pci_dev->config[UDIDETCR0]; + } else { + val = pci_dev->config[UDIDETCR1]; + } + break; + default: + val = 0xff; + break; + } + + trace_bmdma_read_cmd646(addr, val); + return val; +} + +static void cmd646_bmdma_write(void *opaque, hwaddr addr, uint64_t val, + unsigned size) +{ + BMDMAState *bm = opaque; + PCIDevice *pci_dev = PCI_DEVICE(bm->pci_dev); + + if (size != 1) { + return; + } + + trace_bmdma_write_cmd646(addr, val); + switch (addr & 3) { + case 1: + pci_dev->config[MRDMODE] = + (pci_dev->config[MRDMODE] & ~0x30) | (val & 0x30); + cmd646_update_dma_interrupts(pci_dev); + cmd646_update_irq(pci_dev); + break; + case 3: + if (bm == &bm->pci_dev->bmdma[0]) { + pci_dev->config[UDIDETCR0] = val; + } else { + pci_dev->config[UDIDETCR1] = val; + } + break; + } +} + +static const MemoryRegionOps cmd646_bmdma_ops = { + .read = cmd646_bmdma_read, + .write = cmd646_bmdma_write, +}; + +static void cmd646_bmdma_setup(PCIIDEState *d) +{ + CMD646IDEState *s = CMD646_IDE(d); + BMDMAState *bm; + int i; + + /* Setup aliases for device-specific BMDMA registers */ + for (i = 0; i < 2; i++) { + bm = &d->bmdma[i]; + memory_region_init_io(&s->bmdma_mem[i], OBJECT(d), &cmd646_bmdma_ops, + bm, "cmd646-bmdma", 4); + memory_region_init_alias(&s->bmdma_mem_alias[i][0], OBJECT(d), + "cmd646-bmdma[1]", &s->bmdma_mem[i], 0x1, 1); + memory_region_add_subregion(&bm->extra_io, 0x1, + &s->bmdma_mem_alias[i][0]); + memory_region_init_alias(&s->bmdma_mem_alias[i][1], OBJECT(d), + "cmd646-bmdma[3]", &s->bmdma_mem[i], 0x3, 1); + memory_region_add_subregion(&bm->extra_io, 0x3, + &s->bmdma_mem_alias[i][1]); + } +} + static void cmd646_update_irq(PCIDevice *pd) { int pci_level; @@ -289,6 +349,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp) bmdma_setup_bar(d); pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar); + cmd646_bmdma_setup(d); /* TODO: RST# value should be 0 */ pci_conf[PCI_INTERRUPT_PIN] = 0x01; /* interrupt on pin 1 */ diff --git a/include/hw/ide/cmd646.h b/include/hw/ide/cmd646.h index 4780b1132c..5329964533 100644 --- a/include/hw/ide/cmd646.h +++ b/include/hw/ide/cmd646.h @@ -29,10 +29,14 @@ #define TYPE_CMD646_IDE "cmd646-ide" OBJECT_DECLARE_SIMPLE_TYPE(CMD646IDEState, CMD646_IDE) +#include "exec/memory.h" #include "hw/ide/pci.h" struct CMD646IDEState { PCIIDEState parent_obj; + + MemoryRegion bmdma_mem[2]; + MemoryRegion bmdma_mem_alias[2][2]; }; #endif
The aim here is to eliminate any device-specific registers from the main BMDMA bar memory region so it can potentially be moved into the shared PCI IDE device. For each BMDMA bus create a new cmd646-bmdma-specific memory region representing the device-specific BMDMA registers and then map them using aliases onto the existing BMDMAState memory region. Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- hw/ide/cmd646.c | 111 +++++++++++++++++++++++++++++++--------- include/hw/ide/cmd646.h | 4 ++ 2 files changed, 90 insertions(+), 25 deletions(-)