On Sat, 19 Feb 2022, Liav Albani wrote:
Instead of letting each implementation to duplicate this code, we can
share these functions between IDE PIIX3/4 and VIA implementations.
OK but there's a way to take this even further as cmd646 also uses similar
functions just with more cases so you could remove the cases handled by
these functions and only leave the difference and call the default
function from the default case. E.g. (untested, just to show the idea):
hw/ide/cmd646.c:
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) {
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 = bmdma_default_read(opaque, addr, size);
break;
}
trace_bmdma_read_cmd646(addr, val);
return val;
}
Signed-off-by: Liav Albani <liav...@gmail.com>
---
hw/ide/pci.c | 47 ++++++++++++++++++++++++++++++++++++++++
hw/ide/piix.c | 50 ++-----------------------------------------
hw/ide/via.c | 51 ++------------------------------------------
include/hw/ide/pci.h | 4 ++++
4 files changed, 55 insertions(+), 97 deletions(-)
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 84ba733548..c8b867659a 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -502,6 +502,53 @@ static const struct IDEDMAOps bmdma_ops = {
.reset = bmdma_reset,
};
+uint64_t bmdma_default_read(void *opaque, hwaddr addr,
+ unsigned size)
Indentation off? Also everywhere else, usually we indent not with the
parenthesis but with the list within. (Auto indent in most editors does
that probably.)
Regards,
BALATON Zoltan
+{
+ BMDMAState *bm = opaque;
+ uint32_t val;
+
+ if (size != 1) {
+ return ((uint64_t)1 << (size * 8)) - 1;
+ }
+
+ switch (addr & 3) {
+ case 0:
+ val = bm->cmd;
+ break;
+ case 2:
+ val = bm->status;
+ break;
+ default:
+ val = 0xff;
+ break;
+ }
+
+ trace_bmdma_read_via(addr, val);
+ return val;
+}
+
+void bmdma_default_write(void *opaque, hwaddr addr,
+ uint64_t val, unsigned size)
+{
+ BMDMAState *bm = opaque;
+
+ if (size != 1) {
+ return;
+ }
+
+ trace_bmdma_write_via(addr, val);
+ switch (addr & 3) {
+ case 0:
+ bmdma_cmd_writeb(bm, val);
+ break;
+ case 2:
+ bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val &
0x06);
+ break;
+ default:;
+ }
+}
+
void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d)
{
if (bus->dma == &bm->dma) {
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index ce89fd0aa3..fdf3a04cb1 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -35,55 +35,9 @@
#include "hw/ide/pci.h"
#include "trace.h"
-static uint64_t bmdma_read(void *opaque, hwaddr addr, unsigned size)
-{
- BMDMAState *bm = opaque;
- uint32_t val;
-
- if (size != 1) {
- return ((uint64_t)1 << (size * 8)) - 1;
- }
-
- switch(addr & 3) {
- case 0:
- val = bm->cmd;
- break;
- case 2:
- val = bm->status;
- break;
- default:
- val = 0xff;
- break;
- }
-
- trace_bmdma_read(addr, val);
- return val;
-}
-
-static void bmdma_write(void *opaque, hwaddr addr,
- uint64_t val, unsigned size)
-{
- BMDMAState *bm = opaque;
-
- if (size != 1) {
- return;
- }
-
- trace_bmdma_write(addr, val);
-
- switch(addr & 3) {
- case 0:
- bmdma_cmd_writeb(bm, val);
- break;
- case 2:
- bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val &
0x06);
- break;
- }
-}
-
static const MemoryRegionOps piix_bmdma_ops = {
- .read = bmdma_read,
- .write = bmdma_write,
+ .read = bmdma_default_read,
+ .write = bmdma_default_write,
};
static void bmdma_setup_bar(PCIIDEState *d)
diff --git a/hw/ide/via.c b/hw/ide/via.c
index 82def819c4..13f27c9514 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -33,56 +33,9 @@
#include "hw/ide/pci.h"
#include "trace.h"
-static uint64_t bmdma_read(void *opaque, hwaddr addr,
- unsigned size)
-{
- BMDMAState *bm = opaque;
- uint32_t val;
-
- if (size != 1) {
- return ((uint64_t)1 << (size * 8)) - 1;
- }
-
- switch (addr & 3) {
- case 0:
- val = bm->cmd;
- break;
- case 2:
- val = bm->status;
- break;
- default:
- val = 0xff;
- break;
- }
-
- trace_bmdma_read_via(addr, val);
- return val;
-}
-
-static void bmdma_write(void *opaque, hwaddr addr,
- uint64_t val, unsigned size)
-{
- BMDMAState *bm = opaque;
-
- if (size != 1) {
- return;
- }
-
- trace_bmdma_write_via(addr, val);
- switch (addr & 3) {
- case 0:
- bmdma_cmd_writeb(bm, val);
- break;
- case 2:
- bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val &
0x06);
- break;
- default:;
- }
-}
-
static const MemoryRegionOps via_bmdma_ops = {
- .read = bmdma_read,
- .write = bmdma_write,
+ .read = bmdma_default_read,
+ .write = bmdma_default_write,
};
static void bmdma_setup_bar(PCIIDEState *d)
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index d8384e1c42..159136f055 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -62,6 +62,10 @@ static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
}
void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d);
+uint64_t bmdma_default_read(void *opaque, hwaddr addr,
+ unsigned size);
+void bmdma_default_write(void *opaque, hwaddr addr,
+ uint64_t val, unsigned size);
void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
extern MemoryRegionOps bmdma_addr_ioport_ops;
void pci_ide_create_devs(PCIDevice *dev);
