Message ID | 20230422150728.176512-7-shentey@gmail.com |
---|---|
State | New |
Headers | show |
Series | Clean up PCI IDE device models | expand |
On 22/4/23 17:07, Bernhard Beschow wrote: > There are three private copies of bmdma_setup_bar() with small adaptions. > Consolidate them into one public implementation. > > While at it rename the function to bmdma_init_ops() to reflect that the memory > regions being initialized represent BMDMA operations. The actual mapping as a > PCI BAR is still performed separately in each device. > > Note that the bmdma_bar attribute will be renamed in a separate commit. > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > include/hw/ide/pci.h | 1 + > hw/ide/cmd646.c | 20 +------------------- > hw/ide/pci.c | 16 ++++++++++++++++ > hw/ide/piix.c | 19 +------------------ > hw/ide/via.c | 19 +------------------ > 5 files changed, 20 insertions(+), 55 deletions(-) Nice. Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Am 23. April 2023 17:43:22 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>: >On 22/4/23 17:07, Bernhard Beschow wrote: >> There are three private copies of bmdma_setup_bar() with small adaptions. >> Consolidate them into one public implementation. >> >> While at it rename the function to bmdma_init_ops() to reflect that the memory >> regions being initialized represent BMDMA operations. The actual mapping as a >> PCI BAR is still performed separately in each device. >> >> Note that the bmdma_bar attribute will be renamed in a separate commit. >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> --- >> include/hw/ide/pci.h | 1 + >> hw/ide/cmd646.c | 20 +------------------- >> hw/ide/pci.c | 16 ++++++++++++++++ >> hw/ide/piix.c | 19 +------------------ >> hw/ide/via.c | 19 +------------------ >> 5 files changed, 20 insertions(+), 55 deletions(-) > >Nice. > >Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> I'd rework this patch in the next iteration. I think that most of the memory region initialization can be moved to pci_ide_init(). Unlike realize methods, the nice thing about these instance_init() methods is that the parent implementation is called implicitly rather than being overridden by the child implementation, similar to C++ constructors. This allows for code reuse without much gymnastics. Best regards, Bernhard
On 22/04/2023 16:07, Bernhard Beschow wrote: > There are three private copies of bmdma_setup_bar() with small adaptions. > Consolidate them into one public implementation. > > While at it rename the function to bmdma_init_ops() to reflect that the memory > regions being initialized represent BMDMA operations. The actual mapping as a > PCI BAR is still performed separately in each device. > > Note that the bmdma_bar attribute will be renamed in a separate commit. > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > include/hw/ide/pci.h | 1 + > hw/ide/cmd646.c | 20 +------------------- > hw/ide/pci.c | 16 ++++++++++++++++ > hw/ide/piix.c | 19 +------------------ > hw/ide/via.c | 19 +------------------ > 5 files changed, 20 insertions(+), 55 deletions(-) > > diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h > index 7bc4e53d02..597c77c7ad 100644 > --- a/include/hw/ide/pci.h > +++ b/include/hw/ide/pci.h > @@ -57,6 +57,7 @@ struct PCIIDEState { > }; > > void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d); > +void bmdma_init_ops(PCIIDEState *d, const MemoryRegionOps *bmdma_ops); > void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val); > extern MemoryRegionOps bmdma_addr_ioport_ops; > void pci_ide_create_devs(PCIDevice *dev); > diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c > index 9aabf80e52..6fd09fe74e 100644 > --- a/hw/ide/cmd646.c > +++ b/hw/ide/cmd646.c > @@ -161,24 +161,6 @@ static const MemoryRegionOps cmd646_bmdma_ops = { > .write = bmdma_write, > }; > > -static void bmdma_setup_bar(PCIIDEState *d) > -{ > - BMDMAState *bm; > - int i; > - > - memory_region_init(&d->bmdma_bar, OBJECT(d), "cmd646-bmdma", 16); > - for(i = 0;i < 2; i++) { > - bm = &d->bmdma[i]; > - memory_region_init_io(&bm->extra_io, OBJECT(d), &cmd646_bmdma_ops, bm, > - "cmd646-bmdma-bus", 4); > - memory_region_add_subregion(&d->bmdma_bar, i * 8, &bm->extra_io); > - memory_region_init_io(&bm->addr_ioport, OBJECT(d), > - &bmdma_addr_ioport_ops, bm, > - "cmd646-bmdma-ioport", 4); > - memory_region_add_subregion(&d->bmdma_bar, i * 8 + 4, &bm->addr_ioport); > - } > -} > - > static void cmd646_update_irq(PCIDevice *pd) > { > int pci_level; > @@ -285,7 +267,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp) > &d->bus[1], "cmd646-cmd1", 4); > pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]); > > - bmdma_setup_bar(d); > + bmdma_init_ops(d, &cmd646_bmdma_ops); > pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar); > > /* TODO: RST# value should be 0 */ > diff --git a/hw/ide/pci.c b/hw/ide/pci.c > index 8bea92e394..65ed6f7f72 100644 > --- a/hw/ide/pci.c > +++ b/hw/ide/pci.c > @@ -523,6 +523,22 @@ void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d) > bm->pci_dev = d; > } > > +void bmdma_init_ops(PCIIDEState *d, const MemoryRegionOps *bmdma_ops) > +{ > + size_t i; > + > + memory_region_init(&d->bmdma_bar, OBJECT(d), "bmdma-container", 16); > + for (i = 0; i < ARRAY_SIZE(d->bmdma); i++) { > + BMDMAState *bm = &d->bmdma[i]; > + > + memory_region_init_io(&bm->extra_io, OBJECT(d), bmdma_ops, bm, "bmdma-ops", 4); > + memory_region_add_subregion(&d->bmdma_bar, i * 8, &bm->extra_io); > + memory_region_init_io(&bm->addr_ioport, OBJECT(d), &bmdma_addr_ioport_ops, bm, > + "bmdma-ioport-ops", 4); > + memory_region_add_subregion(&d->bmdma_bar, i * 8 + 4, &bm->addr_ioport); > + } > +} > + > static void pci_ide_init(Object *obj) > { > PCIIDEState *d = PCI_IDE(obj); > diff --git a/hw/ide/piix.c b/hw/ide/piix.c > index 4e6ca99123..5611473d37 100644 > --- a/hw/ide/piix.c > +++ b/hw/ide/piix.c > @@ -86,23 +86,6 @@ static const MemoryRegionOps piix_bmdma_ops = { > .write = bmdma_write, > }; > > -static void bmdma_setup_bar(PCIIDEState *d) > -{ > - int i; > - > - memory_region_init(&d->bmdma_bar, OBJECT(d), "piix-bmdma-container", 16); > - for(i = 0;i < 2; i++) { > - BMDMAState *bm = &d->bmdma[i]; > - > - memory_region_init_io(&bm->extra_io, OBJECT(d), &piix_bmdma_ops, bm, > - "piix-bmdma", 4); > - memory_region_add_subregion(&d->bmdma_bar, i * 8, &bm->extra_io); > - memory_region_init_io(&bm->addr_ioport, OBJECT(d), > - &bmdma_addr_ioport_ops, bm, "bmdma", 4); > - memory_region_add_subregion(&d->bmdma_bar, i * 8 + 4, &bm->addr_ioport); > - } > -} > - > static void piix_ide_reset(DeviceState *dev) > { > PCIIDEState *d = PCI_IDE(dev); > @@ -156,7 +139,7 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp) > > pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode > > - bmdma_setup_bar(d); > + bmdma_init_ops(d, &piix_bmdma_ops); > pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar); > > for (unsigned i = 0; i < 2; i++) { > diff --git a/hw/ide/via.c b/hw/ide/via.c > index 287143a005..40704e2857 100644 > --- a/hw/ide/via.c > +++ b/hw/ide/via.c > @@ -86,23 +86,6 @@ static const MemoryRegionOps via_bmdma_ops = { > .write = bmdma_write, > }; > > -static void bmdma_setup_bar(PCIIDEState *d) > -{ > - int i; > - > - memory_region_init(&d->bmdma_bar, OBJECT(d), "via-bmdma-container", 16); > - for (i = 0; i < ARRAY_SIZE(d->bmdma); i++) { > - BMDMAState *bm = &d->bmdma[i]; > - > - memory_region_init_io(&bm->extra_io, OBJECT(d), &via_bmdma_ops, bm, > - "via-bmdma", 4); > - memory_region_add_subregion(&d->bmdma_bar, i * 8, &bm->extra_io); > - memory_region_init_io(&bm->addr_ioport, OBJECT(d), > - &bmdma_addr_ioport_ops, bm, "bmdma", 4); > - memory_region_add_subregion(&d->bmdma_bar, i * 8 + 4, &bm->addr_ioport); > - } > -} > - > static void via_ide_set_irq(void *opaque, int n, int level) > { > PCIIDEState *s = opaque; > @@ -187,7 +170,7 @@ static void via_ide_realize(PCIDevice *dev, Error **errp) > &d->bus[1], "via-ide1-cmd", 4); > pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]); > > - bmdma_setup_bar(d); > + bmdma_init_ops(d, &via_bmdma_ops); > pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar); > > qdev_init_gpio_in(ds, via_ide_set_irq, ARRAY_SIZE(d->bus)); Nice! In actual fact, with some more refactoring of the CMD646 device I believe you could remove the per-implementation ops and move everything into hw/ide/pci.c. The main reason this is a bit difficult now is because of the "Device specific" registers intertwined with the BMDMA registers, but there's no reason that CMD646 couldn't manually attach a fallback MemoryRegion to PCIIDEState::bmdma_bar and implement its device-specific registers there. Unfortunately this isn't just a cut/paste job because there is also some mirroring of the BMDMA in PCI configuration space which will need some extra untangling: let's leave this as-is for now since it makes it easier for a follow-up patch to improve this later. On that basis: Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> ATB, Mark.
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h index 7bc4e53d02..597c77c7ad 100644 --- a/include/hw/ide/pci.h +++ b/include/hw/ide/pci.h @@ -57,6 +57,7 @@ struct PCIIDEState { }; void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d); +void bmdma_init_ops(PCIIDEState *d, const MemoryRegionOps *bmdma_ops); void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val); extern MemoryRegionOps bmdma_addr_ioport_ops; void pci_ide_create_devs(PCIDevice *dev); diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c index 9aabf80e52..6fd09fe74e 100644 --- a/hw/ide/cmd646.c +++ b/hw/ide/cmd646.c @@ -161,24 +161,6 @@ static const MemoryRegionOps cmd646_bmdma_ops = { .write = bmdma_write, }; -static void bmdma_setup_bar(PCIIDEState *d) -{ - BMDMAState *bm; - int i; - - memory_region_init(&d->bmdma_bar, OBJECT(d), "cmd646-bmdma", 16); - for(i = 0;i < 2; i++) { - bm = &d->bmdma[i]; - memory_region_init_io(&bm->extra_io, OBJECT(d), &cmd646_bmdma_ops, bm, - "cmd646-bmdma-bus", 4); - memory_region_add_subregion(&d->bmdma_bar, i * 8, &bm->extra_io); - memory_region_init_io(&bm->addr_ioport, OBJECT(d), - &bmdma_addr_ioport_ops, bm, - "cmd646-bmdma-ioport", 4); - memory_region_add_subregion(&d->bmdma_bar, i * 8 + 4, &bm->addr_ioport); - } -} - static void cmd646_update_irq(PCIDevice *pd) { int pci_level; @@ -285,7 +267,7 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp) &d->bus[1], "cmd646-cmd1", 4); pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]); - bmdma_setup_bar(d); + bmdma_init_ops(d, &cmd646_bmdma_ops); pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar); /* TODO: RST# value should be 0 */ diff --git a/hw/ide/pci.c b/hw/ide/pci.c index 8bea92e394..65ed6f7f72 100644 --- a/hw/ide/pci.c +++ b/hw/ide/pci.c @@ -523,6 +523,22 @@ void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d) bm->pci_dev = d; } +void bmdma_init_ops(PCIIDEState *d, const MemoryRegionOps *bmdma_ops) +{ + size_t i; + + memory_region_init(&d->bmdma_bar, OBJECT(d), "bmdma-container", 16); + for (i = 0; i < ARRAY_SIZE(d->bmdma); i++) { + BMDMAState *bm = &d->bmdma[i]; + + memory_region_init_io(&bm->extra_io, OBJECT(d), bmdma_ops, bm, "bmdma-ops", 4); + memory_region_add_subregion(&d->bmdma_bar, i * 8, &bm->extra_io); + memory_region_init_io(&bm->addr_ioport, OBJECT(d), &bmdma_addr_ioport_ops, bm, + "bmdma-ioport-ops", 4); + memory_region_add_subregion(&d->bmdma_bar, i * 8 + 4, &bm->addr_ioport); + } +} + static void pci_ide_init(Object *obj) { PCIIDEState *d = PCI_IDE(obj); diff --git a/hw/ide/piix.c b/hw/ide/piix.c index 4e6ca99123..5611473d37 100644 --- a/hw/ide/piix.c +++ b/hw/ide/piix.c @@ -86,23 +86,6 @@ static const MemoryRegionOps piix_bmdma_ops = { .write = bmdma_write, }; -static void bmdma_setup_bar(PCIIDEState *d) -{ - int i; - - memory_region_init(&d->bmdma_bar, OBJECT(d), "piix-bmdma-container", 16); - for(i = 0;i < 2; i++) { - BMDMAState *bm = &d->bmdma[i]; - - memory_region_init_io(&bm->extra_io, OBJECT(d), &piix_bmdma_ops, bm, - "piix-bmdma", 4); - memory_region_add_subregion(&d->bmdma_bar, i * 8, &bm->extra_io); - memory_region_init_io(&bm->addr_ioport, OBJECT(d), - &bmdma_addr_ioport_ops, bm, "bmdma", 4); - memory_region_add_subregion(&d->bmdma_bar, i * 8 + 4, &bm->addr_ioport); - } -} - static void piix_ide_reset(DeviceState *dev) { PCIIDEState *d = PCI_IDE(dev); @@ -156,7 +139,7 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp) pci_conf[PCI_CLASS_PROG] = 0x80; // legacy ATA mode - bmdma_setup_bar(d); + bmdma_init_ops(d, &piix_bmdma_ops); pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar); for (unsigned i = 0; i < 2; i++) { diff --git a/hw/ide/via.c b/hw/ide/via.c index 287143a005..40704e2857 100644 --- a/hw/ide/via.c +++ b/hw/ide/via.c @@ -86,23 +86,6 @@ static const MemoryRegionOps via_bmdma_ops = { .write = bmdma_write, }; -static void bmdma_setup_bar(PCIIDEState *d) -{ - int i; - - memory_region_init(&d->bmdma_bar, OBJECT(d), "via-bmdma-container", 16); - for (i = 0; i < ARRAY_SIZE(d->bmdma); i++) { - BMDMAState *bm = &d->bmdma[i]; - - memory_region_init_io(&bm->extra_io, OBJECT(d), &via_bmdma_ops, bm, - "via-bmdma", 4); - memory_region_add_subregion(&d->bmdma_bar, i * 8, &bm->extra_io); - memory_region_init_io(&bm->addr_ioport, OBJECT(d), - &bmdma_addr_ioport_ops, bm, "bmdma", 4); - memory_region_add_subregion(&d->bmdma_bar, i * 8 + 4, &bm->addr_ioport); - } -} - static void via_ide_set_irq(void *opaque, int n, int level) { PCIIDEState *s = opaque; @@ -187,7 +170,7 @@ static void via_ide_realize(PCIDevice *dev, Error **errp) &d->bus[1], "via-ide1-cmd", 4); pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]); - bmdma_setup_bar(d); + bmdma_init_ops(d, &via_bmdma_ops); pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar); qdev_init_gpio_in(ds, via_ide_set_irq, ARRAY_SIZE(d->bus));
There are three private copies of bmdma_setup_bar() with small adaptions. Consolidate them into one public implementation. While at it rename the function to bmdma_init_ops() to reflect that the memory regions being initialized represent BMDMA operations. The actual mapping as a PCI BAR is still performed separately in each device. Note that the bmdma_bar attribute will be renamed in a separate commit. Signed-off-by: Bernhard Beschow <shentey@gmail.com> --- include/hw/ide/pci.h | 1 + hw/ide/cmd646.c | 20 +------------------- hw/ide/pci.c | 16 ++++++++++++++++ hw/ide/piix.c | 19 +------------------ hw/ide/via.c | 19 +------------------ 5 files changed, 20 insertions(+), 55 deletions(-)