Am 9. Juni 2023 18:51:19 UTC schrieb Mark Cave-Ayland
<mark.cave-ayl...@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-ayl...@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
