| Message ID | 20230422150728.176512-14-shentey@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Clean up PCI IDE device models | expand |
On Sat, 22 Apr 2023, Bernhard Beschow wrote: > Extract bmdma_clear_status() mirroring bmdma_cmd_writeb(). Is adding a trace point useful? This is called from places that already have traces so I don't think we need another separate trace point here. Also the names don't match but maybe rename function to bmdma_update_status instead as it is more what it does. Regards, BALATON Zoltan > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > include/hw/ide/pci.h | 1 + > hw/ide/cmd646.c | 2 +- > hw/ide/pci.c | 7 +++++++ > hw/ide/piix.c | 2 +- > hw/ide/sii3112.c | 12 +++++------- > hw/ide/via.c | 2 +- > hw/ide/trace-events | 1 + > 7 files changed, 17 insertions(+), 10 deletions(-) > > diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h > index 81e0370202..6a286ad307 100644 > --- a/include/hw/ide/pci.h > +++ b/include/hw/ide/pci.h > @@ -59,6 +59,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); > +void bmdma_clear_status(BMDMAState *bm, uint32_t val); > void pci_ide_create_devs(PCIDevice *dev); > > #endif > diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c > index b9d005a357..973c3ff0dc 100644 > --- a/hw/ide/cmd646.c > +++ b/hw/ide/cmd646.c > @@ -144,7 +144,7 @@ static void bmdma_write(void *opaque, hwaddr addr, > cmd646_update_irq(pci_dev); > break; > case 2: > - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06); > + bmdma_clear_status(bm, val); > break; > case 3: > if (bm == &bm->pci_dev->bmdma[0]) { > diff --git a/hw/ide/pci.c b/hw/ide/pci.c > index 3539b162b7..4aa06be7c6 100644 > --- a/hw/ide/pci.c > +++ b/hw/ide/pci.c > @@ -318,6 +318,13 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val) > bm->cmd = val & 0x09; > } > > +void bmdma_clear_status(BMDMAState *bm, uint32_t val) > +{ > + trace_bmdma_update_status(val); > + > + bm->status = (val & 0x60) | (bm->status & BM_STATUS_DMAING) | (bm->status & ~val & 0x06); > +} > + > static uint64_t bmdma_addr_read(void *opaque, hwaddr addr, > unsigned width) > { > diff --git a/hw/ide/piix.c b/hw/ide/piix.c > index 406a67fa0f..9eab615e35 100644 > --- a/hw/ide/piix.c > +++ b/hw/ide/piix.c > @@ -76,7 +76,7 @@ static void bmdma_write(void *opaque, hwaddr addr, > bmdma_cmd_writeb(bm, val); > break; > case 2: > - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06); > + bmdma_clear_status(bm, val); > break; > } > } > diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c > index 373c0dd1ee..1180ff55e7 100644 > --- a/hw/ide/sii3112.c > +++ b/hw/ide/sii3112.c > @@ -66,7 +66,7 @@ static void sii3112_bmdma_write(void *opaque, hwaddr addr, > uint64_t val, unsigned int size) > { > BMDMAState *bm = opaque; > - SiI3112PCIState *d = SII3112_PCI(bm->pci_dev); > + SiI3112PCIState *s = SII3112_PCI(bm->pci_dev); > int i = (bm == &bm->pci_dev->bmdma[0]) ? 0 : 1; > > trace_sii3112_bmdma_write(size, addr, val); > @@ -75,10 +75,10 @@ static void sii3112_bmdma_write(void *opaque, hwaddr addr, > bmdma_cmd_writeb(bm, val); > break; > case 0x01: > - d->regs[i].swdata = val & 0x3f; > + s->regs[i].swdata = val & 0x3f; > break; > case 0x02: > - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 6); > + bmdma_clear_status(bm, val); > break; > default: > break; > @@ -160,8 +160,7 @@ static void sii3112_reg_write(void *opaque, hwaddr addr, > d->regs[0].swdata = val & 0x3f; > break; > case 0x12: > - d->i.bmdma[0].status = (val & 0x60) | (d->i.bmdma[0].status & 1) | > - (d->i.bmdma[0].status & ~val & 6); > + bmdma_clear_status(&d->i.bmdma[0], val); > break; > case 0x18: > bmdma_cmd_writeb(&d->i.bmdma[1], val); > @@ -170,8 +169,7 @@ static void sii3112_reg_write(void *opaque, hwaddr addr, > d->regs[1].swdata = val & 0x3f; > break; > case 0x1a: > - d->i.bmdma[1].status = (val & 0x60) | (d->i.bmdma[1].status & 1) | > - (d->i.bmdma[1].status & ~val & 6); > + bmdma_clear_status(&d->i.bmdma[1], val); > break; > case 0x100: > d->regs[0].scontrol = val & 0xfff; > diff --git a/hw/ide/via.c b/hw/ide/via.c > index 35dd97e49b..afb97f302a 100644 > --- a/hw/ide/via.c > +++ b/hw/ide/via.c > @@ -75,7 +75,7 @@ static void bmdma_write(void *opaque, hwaddr addr, > bmdma_cmd_writeb(bm, val); > break; > case 2: > - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06); > + bmdma_clear_status(bm, val); > break; > default:; > } > diff --git a/hw/ide/trace-events b/hw/ide/trace-events > index a479525e38..d219c64b61 100644 > --- a/hw/ide/trace-events > +++ b/hw/ide/trace-events > @@ -30,6 +30,7 @@ bmdma_write_cmd646(uint64_t addr, uint64_t val) "bmdma: writeb 0x%"PRIx64" : 0x% > # pci.c > bmdma_reset(void) "" > bmdma_cmd_writeb(uint32_t val) "val: 0x%08x" > +bmdma_update_status(uint32_t val) "val: 0x%08x" > bmdma_addr_read(uint64_t data) "data: 0x%016"PRIx64 > bmdma_addr_write(uint64_t data) "data: 0x%016"PRIx64 > >
On Sat, 22 Apr 2023, Bernhard Beschow wrote: > Extract bmdma_clear_status() mirroring bmdma_cmd_writeb(). > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > include/hw/ide/pci.h | 1 + > hw/ide/cmd646.c | 2 +- > hw/ide/pci.c | 7 +++++++ > hw/ide/piix.c | 2 +- > hw/ide/sii3112.c | 12 +++++------- > hw/ide/via.c | 2 +- > hw/ide/trace-events | 1 + > 7 files changed, 17 insertions(+), 10 deletions(-) > > diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h > index 81e0370202..6a286ad307 100644 > --- a/include/hw/ide/pci.h > +++ b/include/hw/ide/pci.h > @@ -59,6 +59,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); > +void bmdma_clear_status(BMDMAState *bm, uint32_t val); > void pci_ide_create_devs(PCIDevice *dev); > > #endif > diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c > index b9d005a357..973c3ff0dc 100644 > --- a/hw/ide/cmd646.c > +++ b/hw/ide/cmd646.c > @@ -144,7 +144,7 @@ static void bmdma_write(void *opaque, hwaddr addr, > cmd646_update_irq(pci_dev); > break; > case 2: > - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06); > + bmdma_clear_status(bm, val); > break; > case 3: > if (bm == &bm->pci_dev->bmdma[0]) { > diff --git a/hw/ide/pci.c b/hw/ide/pci.c > index 3539b162b7..4aa06be7c6 100644 > --- a/hw/ide/pci.c > +++ b/hw/ide/pci.c > @@ -318,6 +318,13 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val) > bm->cmd = val & 0x09; > } > > +void bmdma_clear_status(BMDMAState *bm, uint32_t val) > +{ > + trace_bmdma_update_status(val); > + > + bm->status = (val & 0x60) | (bm->status & BM_STATUS_DMAING) | (bm->status & ~val & 0x06); > +} > + > static uint64_t bmdma_addr_read(void *opaque, hwaddr addr, > unsigned width) > { > diff --git a/hw/ide/piix.c b/hw/ide/piix.c > index 406a67fa0f..9eab615e35 100644 > --- a/hw/ide/piix.c > +++ b/hw/ide/piix.c > @@ -76,7 +76,7 @@ static void bmdma_write(void *opaque, hwaddr addr, > bmdma_cmd_writeb(bm, val); > break; > case 2: > - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06); > + bmdma_clear_status(bm, val); > break; > } > } > diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c > index 373c0dd1ee..1180ff55e7 100644 > --- a/hw/ide/sii3112.c > +++ b/hw/ide/sii3112.c > @@ -66,7 +66,7 @@ static void sii3112_bmdma_write(void *opaque, hwaddr addr, > uint64_t val, unsigned int size) > { > BMDMAState *bm = opaque; > - SiI3112PCIState *d = SII3112_PCI(bm->pci_dev); > + SiI3112PCIState *s = SII3112_PCI(bm->pci_dev); Also renaming local variable is an unrelated change. May be separate patch but wasn't it added in previous patch? Why not already done there? Regards, BALATON Zoltan > int i = (bm == &bm->pci_dev->bmdma[0]) ? 0 : 1; > > trace_sii3112_bmdma_write(size, addr, val); > @@ -75,10 +75,10 @@ static void sii3112_bmdma_write(void *opaque, hwaddr addr, > bmdma_cmd_writeb(bm, val); > break; > case 0x01: > - d->regs[i].swdata = val & 0x3f; > + s->regs[i].swdata = val & 0x3f; > break; > case 0x02: > - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 6); > + bmdma_clear_status(bm, val); > break; > default: > break; > @@ -160,8 +160,7 @@ static void sii3112_reg_write(void *opaque, hwaddr addr, > d->regs[0].swdata = val & 0x3f; > break; > case 0x12: > - d->i.bmdma[0].status = (val & 0x60) | (d->i.bmdma[0].status & 1) | > - (d->i.bmdma[0].status & ~val & 6); > + bmdma_clear_status(&d->i.bmdma[0], val); > break; > case 0x18: > bmdma_cmd_writeb(&d->i.bmdma[1], val); > @@ -170,8 +169,7 @@ static void sii3112_reg_write(void *opaque, hwaddr addr, > d->regs[1].swdata = val & 0x3f; > break; > case 0x1a: > - d->i.bmdma[1].status = (val & 0x60) | (d->i.bmdma[1].status & 1) | > - (d->i.bmdma[1].status & ~val & 6); > + bmdma_clear_status(&d->i.bmdma[1], val); > break; > case 0x100: > d->regs[0].scontrol = val & 0xfff; > diff --git a/hw/ide/via.c b/hw/ide/via.c > index 35dd97e49b..afb97f302a 100644 > --- a/hw/ide/via.c > +++ b/hw/ide/via.c > @@ -75,7 +75,7 @@ static void bmdma_write(void *opaque, hwaddr addr, > bmdma_cmd_writeb(bm, val); > break; > case 2: > - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06); > + bmdma_clear_status(bm, val); > break; > default:; > } > diff --git a/hw/ide/trace-events b/hw/ide/trace-events > index a479525e38..d219c64b61 100644 > --- a/hw/ide/trace-events > +++ b/hw/ide/trace-events > @@ -30,6 +30,7 @@ bmdma_write_cmd646(uint64_t addr, uint64_t val) "bmdma: writeb 0x%"PRIx64" : 0x% > # pci.c > bmdma_reset(void) "" > bmdma_cmd_writeb(uint32_t val) "val: 0x%08x" > +bmdma_update_status(uint32_t val) "val: 0x%08x" > bmdma_addr_read(uint64_t data) "data: 0x%016"PRIx64 > bmdma_addr_write(uint64_t data) "data: 0x%016"PRIx64 > >
Am 22. April 2023 22:46:08 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >On Sat, 22 Apr 2023, Bernhard Beschow wrote: >> Extract bmdma_clear_status() mirroring bmdma_cmd_writeb(). >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> --- >> include/hw/ide/pci.h | 1 + >> hw/ide/cmd646.c | 2 +- >> hw/ide/pci.c | 7 +++++++ >> hw/ide/piix.c | 2 +- >> hw/ide/sii3112.c | 12 +++++------- >> hw/ide/via.c | 2 +- >> hw/ide/trace-events | 1 + >> 7 files changed, 17 insertions(+), 10 deletions(-) >> >> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h >> index 81e0370202..6a286ad307 100644 >> --- a/include/hw/ide/pci.h >> +++ b/include/hw/ide/pci.h >> @@ -59,6 +59,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); >> +void bmdma_clear_status(BMDMAState *bm, uint32_t val); >> void pci_ide_create_devs(PCIDevice *dev); >> >> #endif >> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c >> index b9d005a357..973c3ff0dc 100644 >> --- a/hw/ide/cmd646.c >> +++ b/hw/ide/cmd646.c >> @@ -144,7 +144,7 @@ static void bmdma_write(void *opaque, hwaddr addr, >> cmd646_update_irq(pci_dev); >> break; >> case 2: >> - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06); >> + bmdma_clear_status(bm, val); >> break; >> case 3: >> if (bm == &bm->pci_dev->bmdma[0]) { >> diff --git a/hw/ide/pci.c b/hw/ide/pci.c >> index 3539b162b7..4aa06be7c6 100644 >> --- a/hw/ide/pci.c >> +++ b/hw/ide/pci.c >> @@ -318,6 +318,13 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val) >> bm->cmd = val & 0x09; >> } >> >> +void bmdma_clear_status(BMDMAState *bm, uint32_t val) >> +{ >> + trace_bmdma_update_status(val); >> + >> + bm->status = (val & 0x60) | (bm->status & BM_STATUS_DMAING) | (bm->status & ~val & 0x06); >> +} >> + >> static uint64_t bmdma_addr_read(void *opaque, hwaddr addr, >> unsigned width) >> { >> diff --git a/hw/ide/piix.c b/hw/ide/piix.c >> index 406a67fa0f..9eab615e35 100644 >> --- a/hw/ide/piix.c >> +++ b/hw/ide/piix.c >> @@ -76,7 +76,7 @@ static void bmdma_write(void *opaque, hwaddr addr, >> bmdma_cmd_writeb(bm, val); >> break; >> case 2: >> - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06); >> + bmdma_clear_status(bm, val); >> break; >> } >> } >> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c >> index 373c0dd1ee..1180ff55e7 100644 >> --- a/hw/ide/sii3112.c >> +++ b/hw/ide/sii3112.c >> @@ -66,7 +66,7 @@ static void sii3112_bmdma_write(void *opaque, hwaddr addr, >> uint64_t val, unsigned int size) >> { >> BMDMAState *bm = opaque; >> - SiI3112PCIState *d = SII3112_PCI(bm->pci_dev); >> + SiI3112PCIState *s = SII3112_PCI(bm->pci_dev); > >Also renaming local variable is an unrelated change. May be separate patch but wasn't it added in previous patch? Why not already done there? This change is unintented. I'll fix it. Thanks for catching! Best regards, Bernhard > >Regards, >BALATON Zoltan > >> int i = (bm == &bm->pci_dev->bmdma[0]) ? 0 : 1; >> >> trace_sii3112_bmdma_write(size, addr, val); >> @@ -75,10 +75,10 @@ static void sii3112_bmdma_write(void *opaque, hwaddr addr, >> bmdma_cmd_writeb(bm, val); >> break; >> case 0x01: >> - d->regs[i].swdata = val & 0x3f; >> + s->regs[i].swdata = val & 0x3f; >> break; >> case 0x02: >> - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 6); >> + bmdma_clear_status(bm, val); >> break; >> default: >> break; >> @@ -160,8 +160,7 @@ static void sii3112_reg_write(void *opaque, hwaddr addr, >> d->regs[0].swdata = val & 0x3f; >> break; >> case 0x12: >> - d->i.bmdma[0].status = (val & 0x60) | (d->i.bmdma[0].status & 1) | >> - (d->i.bmdma[0].status & ~val & 6); >> + bmdma_clear_status(&d->i.bmdma[0], val); >> break; >> case 0x18: >> bmdma_cmd_writeb(&d->i.bmdma[1], val); >> @@ -170,8 +169,7 @@ static void sii3112_reg_write(void *opaque, hwaddr addr, >> d->regs[1].swdata = val & 0x3f; >> break; >> case 0x1a: >> - d->i.bmdma[1].status = (val & 0x60) | (d->i.bmdma[1].status & 1) | >> - (d->i.bmdma[1].status & ~val & 6); >> + bmdma_clear_status(&d->i.bmdma[1], val); >> break; >> case 0x100: >> d->regs[0].scontrol = val & 0xfff; >> diff --git a/hw/ide/via.c b/hw/ide/via.c >> index 35dd97e49b..afb97f302a 100644 >> --- a/hw/ide/via.c >> +++ b/hw/ide/via.c >> @@ -75,7 +75,7 @@ static void bmdma_write(void *opaque, hwaddr addr, >> bmdma_cmd_writeb(bm, val); >> break; >> case 2: >> - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06); >> + bmdma_clear_status(bm, val); >> break; >> default:; >> } >> diff --git a/hw/ide/trace-events b/hw/ide/trace-events >> index a479525e38..d219c64b61 100644 >> --- a/hw/ide/trace-events >> +++ b/hw/ide/trace-events >> @@ -30,6 +30,7 @@ bmdma_write_cmd646(uint64_t addr, uint64_t val) "bmdma: writeb 0x%"PRIx64" : 0x% >> # pci.c >> bmdma_reset(void) "" >> bmdma_cmd_writeb(uint32_t val) "val: 0x%08x" >> +bmdma_update_status(uint32_t val) "val: 0x%08x" >> bmdma_addr_read(uint64_t data) "data: 0x%016"PRIx64 >> bmdma_addr_write(uint64_t data) "data: 0x%016"PRIx64 >> >>
Am 22. April 2023 21:26:00 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >On Sat, 22 Apr 2023, Bernhard Beschow wrote: >> Extract bmdma_clear_status() mirroring bmdma_cmd_writeb(). > >Is adding a trace point useful? This is called from places that already have traces so I don't think we need another separate trace point here. Adding a trace point was my original motivation to have this function. Then I realized that extracting the code in a dedicated function is a merit in itself. The trace point is a leftover, so I'll remove it. >Also the names don't match but maybe rename function to bmdma_update_status instead as it is more what it does. The status attribute models a w1c-style register. Writing to it can only clear bits, hence the name. Indeed I originally named the function bmdma_update_status() but thought it was too vague. I'm open to suggestions though. Best regards, Bernhard > >Regards, >BALATON Zoltan > >> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >> --- >> include/hw/ide/pci.h | 1 + >> hw/ide/cmd646.c | 2 +- >> hw/ide/pci.c | 7 +++++++ >> hw/ide/piix.c | 2 +- >> hw/ide/sii3112.c | 12 +++++------- >> hw/ide/via.c | 2 +- >> hw/ide/trace-events | 1 + >> 7 files changed, 17 insertions(+), 10 deletions(-) >> >> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h >> index 81e0370202..6a286ad307 100644 >> --- a/include/hw/ide/pci.h >> +++ b/include/hw/ide/pci.h >> @@ -59,6 +59,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); >> +void bmdma_clear_status(BMDMAState *bm, uint32_t val); >> void pci_ide_create_devs(PCIDevice *dev); >> >> #endif >> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c >> index b9d005a357..973c3ff0dc 100644 >> --- a/hw/ide/cmd646.c >> +++ b/hw/ide/cmd646.c >> @@ -144,7 +144,7 @@ static void bmdma_write(void *opaque, hwaddr addr, >> cmd646_update_irq(pci_dev); >> break; >> case 2: >> - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06); >> + bmdma_clear_status(bm, val); >> break; >> case 3: >> if (bm == &bm->pci_dev->bmdma[0]) { >> diff --git a/hw/ide/pci.c b/hw/ide/pci.c >> index 3539b162b7..4aa06be7c6 100644 >> --- a/hw/ide/pci.c >> +++ b/hw/ide/pci.c >> @@ -318,6 +318,13 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val) >> bm->cmd = val & 0x09; >> } >> >> +void bmdma_clear_status(BMDMAState *bm, uint32_t val) >> +{ >> + trace_bmdma_update_status(val); >> + >> + bm->status = (val & 0x60) | (bm->status & BM_STATUS_DMAING) | (bm->status & ~val & 0x06); >> +} >> + >> static uint64_t bmdma_addr_read(void *opaque, hwaddr addr, >> unsigned width) >> { >> diff --git a/hw/ide/piix.c b/hw/ide/piix.c >> index 406a67fa0f..9eab615e35 100644 >> --- a/hw/ide/piix.c >> +++ b/hw/ide/piix.c >> @@ -76,7 +76,7 @@ static void bmdma_write(void *opaque, hwaddr addr, >> bmdma_cmd_writeb(bm, val); >> break; >> case 2: >> - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06); >> + bmdma_clear_status(bm, val); >> break; >> } >> } >> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c >> index 373c0dd1ee..1180ff55e7 100644 >> --- a/hw/ide/sii3112.c >> +++ b/hw/ide/sii3112.c >> @@ -66,7 +66,7 @@ static void sii3112_bmdma_write(void *opaque, hwaddr addr, >> uint64_t val, unsigned int size) >> { >> BMDMAState *bm = opaque; >> - SiI3112PCIState *d = SII3112_PCI(bm->pci_dev); >> + SiI3112PCIState *s = SII3112_PCI(bm->pci_dev); >> int i = (bm == &bm->pci_dev->bmdma[0]) ? 0 : 1; >> >> trace_sii3112_bmdma_write(size, addr, val); >> @@ -75,10 +75,10 @@ static void sii3112_bmdma_write(void *opaque, hwaddr addr, >> bmdma_cmd_writeb(bm, val); >> break; >> case 0x01: >> - d->regs[i].swdata = val & 0x3f; >> + s->regs[i].swdata = val & 0x3f; >> break; >> case 0x02: >> - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 6); >> + bmdma_clear_status(bm, val); >> break; >> default: >> break; >> @@ -160,8 +160,7 @@ static void sii3112_reg_write(void *opaque, hwaddr addr, >> d->regs[0].swdata = val & 0x3f; >> break; >> case 0x12: >> - d->i.bmdma[0].status = (val & 0x60) | (d->i.bmdma[0].status & 1) | >> - (d->i.bmdma[0].status & ~val & 6); >> + bmdma_clear_status(&d->i.bmdma[0], val); >> break; >> case 0x18: >> bmdma_cmd_writeb(&d->i.bmdma[1], val); >> @@ -170,8 +169,7 @@ static void sii3112_reg_write(void *opaque, hwaddr addr, >> d->regs[1].swdata = val & 0x3f; >> break; >> case 0x1a: >> - d->i.bmdma[1].status = (val & 0x60) | (d->i.bmdma[1].status & 1) | >> - (d->i.bmdma[1].status & ~val & 6); >> + bmdma_clear_status(&d->i.bmdma[1], val); >> break; >> case 0x100: >> d->regs[0].scontrol = val & 0xfff; >> diff --git a/hw/ide/via.c b/hw/ide/via.c >> index 35dd97e49b..afb97f302a 100644 >> --- a/hw/ide/via.c >> +++ b/hw/ide/via.c >> @@ -75,7 +75,7 @@ static void bmdma_write(void *opaque, hwaddr addr, >> bmdma_cmd_writeb(bm, val); >> break; >> case 2: >> - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06); >> + bmdma_clear_status(bm, val); >> break; >> default:; >> } >> diff --git a/hw/ide/trace-events b/hw/ide/trace-events >> index a479525e38..d219c64b61 100644 >> --- a/hw/ide/trace-events >> +++ b/hw/ide/trace-events >> @@ -30,6 +30,7 @@ bmdma_write_cmd646(uint64_t addr, uint64_t val) "bmdma: writeb 0x%"PRIx64" : 0x% >> # pci.c >> bmdma_reset(void) "" >> bmdma_cmd_writeb(uint32_t val) "val: 0x%08x" >> +bmdma_update_status(uint32_t val) "val: 0x%08x" >> bmdma_addr_read(uint64_t data) "data: 0x%016"PRIx64 >> bmdma_addr_write(uint64_t data) "data: 0x%016"PRIx64 >> >>
On Sun, 23 Apr 2023, Bernhard Beschow wrote: > Am 22. April 2023 21:26:00 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >> On Sat, 22 Apr 2023, Bernhard Beschow wrote: >>> Extract bmdma_clear_status() mirroring bmdma_cmd_writeb(). >> >> Is adding a trace point useful? This is called from places that already have traces so I don't think we need another separate trace point here. > > Adding a trace point was my original motivation to have this function. Then I realized that extracting the code in a dedicated function is a merit in itself. The trace point is a leftover, so I'll remove it. > >> Also the names don't match but maybe rename function to bmdma_update_status instead as it is more what it does. > > The status attribute models a w1c-style register. Writing to it can only > clear bits, hence the name. Indeed I originally named the function > bmdma_update_status() but thought it was too vague. I'm open to > suggestions though. The function seems to clear bits 1-2 but set bits 5-6 so it it has some write 1 to clear and some read write bits. Therefore naming the function clear when it can also set bits is not quite right so I think update is better. Regards, BALATON Zoltan >>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>> --- >>> include/hw/ide/pci.h | 1 + >>> hw/ide/cmd646.c | 2 +- >>> hw/ide/pci.c | 7 +++++++ >>> hw/ide/piix.c | 2 +- >>> hw/ide/sii3112.c | 12 +++++------- >>> hw/ide/via.c | 2 +- >>> hw/ide/trace-events | 1 + >>> 7 files changed, 17 insertions(+), 10 deletions(-) >>> >>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h >>> index 81e0370202..6a286ad307 100644 >>> --- a/include/hw/ide/pci.h >>> +++ b/include/hw/ide/pci.h >>> @@ -59,6 +59,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); >>> +void bmdma_clear_status(BMDMAState *bm, uint32_t val); >>> void pci_ide_create_devs(PCIDevice *dev); >>> >>> #endif >>> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c >>> index b9d005a357..973c3ff0dc 100644 >>> --- a/hw/ide/cmd646.c >>> +++ b/hw/ide/cmd646.c >>> @@ -144,7 +144,7 @@ static void bmdma_write(void *opaque, hwaddr addr, >>> cmd646_update_irq(pci_dev); >>> break; >>> case 2: >>> - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06); >>> + bmdma_clear_status(bm, val); >>> break; >>> case 3: >>> if (bm == &bm->pci_dev->bmdma[0]) { >>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c >>> index 3539b162b7..4aa06be7c6 100644 >>> --- a/hw/ide/pci.c >>> +++ b/hw/ide/pci.c >>> @@ -318,6 +318,13 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val) >>> bm->cmd = val & 0x09; >>> } >>> >>> +void bmdma_clear_status(BMDMAState *bm, uint32_t val) >>> +{ >>> + trace_bmdma_update_status(val); >>> + >>> + bm->status = (val & 0x60) | (bm->status & BM_STATUS_DMAING) | (bm->status & ~val & 0x06); >>> +} >>> + >>> static uint64_t bmdma_addr_read(void *opaque, hwaddr addr, >>> unsigned width) >>> { >>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c >>> index 406a67fa0f..9eab615e35 100644 >>> --- a/hw/ide/piix.c >>> +++ b/hw/ide/piix.c >>> @@ -76,7 +76,7 @@ static void bmdma_write(void *opaque, hwaddr addr, >>> bmdma_cmd_writeb(bm, val); >>> break; >>> case 2: >>> - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06); >>> + bmdma_clear_status(bm, val); >>> break; >>> } >>> } >>> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c >>> index 373c0dd1ee..1180ff55e7 100644 >>> --- a/hw/ide/sii3112.c >>> +++ b/hw/ide/sii3112.c >>> @@ -66,7 +66,7 @@ static void sii3112_bmdma_write(void *opaque, hwaddr addr, >>> uint64_t val, unsigned int size) >>> { >>> BMDMAState *bm = opaque; >>> - SiI3112PCIState *d = SII3112_PCI(bm->pci_dev); >>> + SiI3112PCIState *s = SII3112_PCI(bm->pci_dev); >>> int i = (bm == &bm->pci_dev->bmdma[0]) ? 0 : 1; >>> >>> trace_sii3112_bmdma_write(size, addr, val); >>> @@ -75,10 +75,10 @@ static void sii3112_bmdma_write(void *opaque, hwaddr addr, >>> bmdma_cmd_writeb(bm, val); >>> break; >>> case 0x01: >>> - d->regs[i].swdata = val & 0x3f; >>> + s->regs[i].swdata = val & 0x3f; >>> break; >>> case 0x02: >>> - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 6); >>> + bmdma_clear_status(bm, val); >>> break; >>> default: >>> break; >>> @@ -160,8 +160,7 @@ static void sii3112_reg_write(void *opaque, hwaddr addr, >>> d->regs[0].swdata = val & 0x3f; >>> break; >>> case 0x12: >>> - d->i.bmdma[0].status = (val & 0x60) | (d->i.bmdma[0].status & 1) | >>> - (d->i.bmdma[0].status & ~val & 6); >>> + bmdma_clear_status(&d->i.bmdma[0], val); >>> break; >>> case 0x18: >>> bmdma_cmd_writeb(&d->i.bmdma[1], val); >>> @@ -170,8 +169,7 @@ static void sii3112_reg_write(void *opaque, hwaddr addr, >>> d->regs[1].swdata = val & 0x3f; >>> break; >>> case 0x1a: >>> - d->i.bmdma[1].status = (val & 0x60) | (d->i.bmdma[1].status & 1) | >>> - (d->i.bmdma[1].status & ~val & 6); >>> + bmdma_clear_status(&d->i.bmdma[1], val); >>> break; >>> case 0x100: >>> d->regs[0].scontrol = val & 0xfff; >>> diff --git a/hw/ide/via.c b/hw/ide/via.c >>> index 35dd97e49b..afb97f302a 100644 >>> --- a/hw/ide/via.c >>> +++ b/hw/ide/via.c >>> @@ -75,7 +75,7 @@ static void bmdma_write(void *opaque, hwaddr addr, >>> bmdma_cmd_writeb(bm, val); >>> break; >>> case 2: >>> - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06); >>> + bmdma_clear_status(bm, val); >>> break; >>> default:; >>> } >>> diff --git a/hw/ide/trace-events b/hw/ide/trace-events >>> index a479525e38..d219c64b61 100644 >>> --- a/hw/ide/trace-events >>> +++ b/hw/ide/trace-events >>> @@ -30,6 +30,7 @@ bmdma_write_cmd646(uint64_t addr, uint64_t val) "bmdma: writeb 0x%"PRIx64" : 0x% >>> # pci.c >>> bmdma_reset(void) "" >>> bmdma_cmd_writeb(uint32_t val) "val: 0x%08x" >>> +bmdma_update_status(uint32_t val) "val: 0x%08x" >>> bmdma_addr_read(uint64_t data) "data: 0x%016"PRIx64 >>> bmdma_addr_write(uint64_t data) "data: 0x%016"PRIx64 >>> >>> > >
Am 23. April 2023 10:40:50 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >On Sun, 23 Apr 2023, Bernhard Beschow wrote: >> Am 22. April 2023 21:26:00 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>: >>> On Sat, 22 Apr 2023, Bernhard Beschow wrote: >>>> Extract bmdma_clear_status() mirroring bmdma_cmd_writeb(). >>> >>> Is adding a trace point useful? This is called from places that already have traces so I don't think we need another separate trace point here. >> >> Adding a trace point was my original motivation to have this function. Then I realized that extracting the code in a dedicated function is a merit in itself. The trace point is a leftover, so I'll remove it. >> >>> Also the names don't match but maybe rename function to bmdma_update_status instead as it is more what it does. >> >> The status attribute models a w1c-style register. Writing to it can only clear bits, hence the name. Indeed I originally named the function bmdma_update_status() but thought it was too vague. I'm open to suggestions though. > >The function seems to clear bits 1-2 but set bits 5-6 so it it has some write 1 to clear and some read write bits. Therefore naming the function clear when it can also set bits is not quite right so I think update is better. Convinced. I'll rename to bmdma_update_status() in the next iteration. Best regards, Bernhard > >Regards, >BALATON Zoltan > >>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com> >>>> --- >>>> include/hw/ide/pci.h | 1 + >>>> hw/ide/cmd646.c | 2 +- >>>> hw/ide/pci.c | 7 +++++++ >>>> hw/ide/piix.c | 2 +- >>>> hw/ide/sii3112.c | 12 +++++------- >>>> hw/ide/via.c | 2 +- >>>> hw/ide/trace-events | 1 + >>>> 7 files changed, 17 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h >>>> index 81e0370202..6a286ad307 100644 >>>> --- a/include/hw/ide/pci.h >>>> +++ b/include/hw/ide/pci.h >>>> @@ -59,6 +59,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); >>>> +void bmdma_clear_status(BMDMAState *bm, uint32_t val); >>>> void pci_ide_create_devs(PCIDevice *dev); >>>> >>>> #endif >>>> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c >>>> index b9d005a357..973c3ff0dc 100644 >>>> --- a/hw/ide/cmd646.c >>>> +++ b/hw/ide/cmd646.c >>>> @@ -144,7 +144,7 @@ static void bmdma_write(void *opaque, hwaddr addr, >>>> cmd646_update_irq(pci_dev); >>>> break; >>>> case 2: >>>> - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06); >>>> + bmdma_clear_status(bm, val); >>>> break; >>>> case 3: >>>> if (bm == &bm->pci_dev->bmdma[0]) { >>>> diff --git a/hw/ide/pci.c b/hw/ide/pci.c >>>> index 3539b162b7..4aa06be7c6 100644 >>>> --- a/hw/ide/pci.c >>>> +++ b/hw/ide/pci.c >>>> @@ -318,6 +318,13 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val) >>>> bm->cmd = val & 0x09; >>>> } >>>> >>>> +void bmdma_clear_status(BMDMAState *bm, uint32_t val) >>>> +{ >>>> + trace_bmdma_update_status(val); >>>> + >>>> + bm->status = (val & 0x60) | (bm->status & BM_STATUS_DMAING) | (bm->status & ~val & 0x06); >>>> +} >>>> + >>>> static uint64_t bmdma_addr_read(void *opaque, hwaddr addr, >>>> unsigned width) >>>> { >>>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c >>>> index 406a67fa0f..9eab615e35 100644 >>>> --- a/hw/ide/piix.c >>>> +++ b/hw/ide/piix.c >>>> @@ -76,7 +76,7 @@ static void bmdma_write(void *opaque, hwaddr addr, >>>> bmdma_cmd_writeb(bm, val); >>>> break; >>>> case 2: >>>> - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06); >>>> + bmdma_clear_status(bm, val); >>>> break; >>>> } >>>> } >>>> diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c >>>> index 373c0dd1ee..1180ff55e7 100644 >>>> --- a/hw/ide/sii3112.c >>>> +++ b/hw/ide/sii3112.c >>>> @@ -66,7 +66,7 @@ static void sii3112_bmdma_write(void *opaque, hwaddr addr, >>>> uint64_t val, unsigned int size) >>>> { >>>> BMDMAState *bm = opaque; >>>> - SiI3112PCIState *d = SII3112_PCI(bm->pci_dev); >>>> + SiI3112PCIState *s = SII3112_PCI(bm->pci_dev); >>>> int i = (bm == &bm->pci_dev->bmdma[0]) ? 0 : 1; >>>> >>>> trace_sii3112_bmdma_write(size, addr, val); >>>> @@ -75,10 +75,10 @@ static void sii3112_bmdma_write(void *opaque, hwaddr addr, >>>> bmdma_cmd_writeb(bm, val); >>>> break; >>>> case 0x01: >>>> - d->regs[i].swdata = val & 0x3f; >>>> + s->regs[i].swdata = val & 0x3f; >>>> break; >>>> case 0x02: >>>> - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 6); >>>> + bmdma_clear_status(bm, val); >>>> break; >>>> default: >>>> break; >>>> @@ -160,8 +160,7 @@ static void sii3112_reg_write(void *opaque, hwaddr addr, >>>> d->regs[0].swdata = val & 0x3f; >>>> break; >>>> case 0x12: >>>> - d->i.bmdma[0].status = (val & 0x60) | (d->i.bmdma[0].status & 1) | >>>> - (d->i.bmdma[0].status & ~val & 6); >>>> + bmdma_clear_status(&d->i.bmdma[0], val); >>>> break; >>>> case 0x18: >>>> bmdma_cmd_writeb(&d->i.bmdma[1], val); >>>> @@ -170,8 +169,7 @@ static void sii3112_reg_write(void *opaque, hwaddr addr, >>>> d->regs[1].swdata = val & 0x3f; >>>> break; >>>> case 0x1a: >>>> - d->i.bmdma[1].status = (val & 0x60) | (d->i.bmdma[1].status & 1) | >>>> - (d->i.bmdma[1].status & ~val & 6); >>>> + bmdma_clear_status(&d->i.bmdma[1], val); >>>> break; >>>> case 0x100: >>>> d->regs[0].scontrol = val & 0xfff; >>>> diff --git a/hw/ide/via.c b/hw/ide/via.c >>>> index 35dd97e49b..afb97f302a 100644 >>>> --- a/hw/ide/via.c >>>> +++ b/hw/ide/via.c >>>> @@ -75,7 +75,7 @@ static void bmdma_write(void *opaque, hwaddr addr, >>>> bmdma_cmd_writeb(bm, val); >>>> break; >>>> case 2: >>>> - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06); >>>> + bmdma_clear_status(bm, val); >>>> break; >>>> default:; >>>> } >>>> diff --git a/hw/ide/trace-events b/hw/ide/trace-events >>>> index a479525e38..d219c64b61 100644 >>>> --- a/hw/ide/trace-events >>>> +++ b/hw/ide/trace-events >>>> @@ -30,6 +30,7 @@ bmdma_write_cmd646(uint64_t addr, uint64_t val) "bmdma: writeb 0x%"PRIx64" : 0x% >>>> # pci.c >>>> bmdma_reset(void) "" >>>> bmdma_cmd_writeb(uint32_t val) "val: 0x%08x" >>>> +bmdma_update_status(uint32_t val) "val: 0x%08x" >>>> bmdma_addr_read(uint64_t data) "data: 0x%016"PRIx64 >>>> bmdma_addr_write(uint64_t data) "data: 0x%016"PRIx64 >>>> >>>> >> >>
On 22/04/2023 16:07, Bernhard Beschow wrote: > Extract bmdma_clear_status() mirroring bmdma_cmd_writeb(). > > Signed-off-by: Bernhard Beschow <shentey@gmail.com> > --- > include/hw/ide/pci.h | 1 + > hw/ide/cmd646.c | 2 +- > hw/ide/pci.c | 7 +++++++ > hw/ide/piix.c | 2 +- > hw/ide/sii3112.c | 12 +++++------- > hw/ide/via.c | 2 +- > hw/ide/trace-events | 1 + > 7 files changed, 17 insertions(+), 10 deletions(-) > > diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h > index 81e0370202..6a286ad307 100644 > --- a/include/hw/ide/pci.h > +++ b/include/hw/ide/pci.h > @@ -59,6 +59,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); > +void bmdma_clear_status(BMDMAState *bm, uint32_t val); > void pci_ide_create_devs(PCIDevice *dev); > > #endif > diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c > index b9d005a357..973c3ff0dc 100644 > --- a/hw/ide/cmd646.c > +++ b/hw/ide/cmd646.c > @@ -144,7 +144,7 @@ static void bmdma_write(void *opaque, hwaddr addr, > cmd646_update_irq(pci_dev); > break; > case 2: > - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06); > + bmdma_clear_status(bm, val); > break; > case 3: > if (bm == &bm->pci_dev->bmdma[0]) { > diff --git a/hw/ide/pci.c b/hw/ide/pci.c > index 3539b162b7..4aa06be7c6 100644 > --- a/hw/ide/pci.c > +++ b/hw/ide/pci.c > @@ -318,6 +318,13 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val) > bm->cmd = val & 0x09; > } > > +void bmdma_clear_status(BMDMAState *bm, uint32_t val) > +{ > + trace_bmdma_update_status(val); > + > + bm->status = (val & 0x60) | (bm->status & BM_STATUS_DMAING) | (bm->status & ~val & 0x06); > +} > + > static uint64_t bmdma_addr_read(void *opaque, hwaddr addr, > unsigned width) > { > diff --git a/hw/ide/piix.c b/hw/ide/piix.c > index 406a67fa0f..9eab615e35 100644 > --- a/hw/ide/piix.c > +++ b/hw/ide/piix.c > @@ -76,7 +76,7 @@ static void bmdma_write(void *opaque, hwaddr addr, > bmdma_cmd_writeb(bm, val); > break; > case 2: > - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06); > + bmdma_clear_status(bm, val); > break; > } > } > diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c > index 373c0dd1ee..1180ff55e7 100644 > --- a/hw/ide/sii3112.c > +++ b/hw/ide/sii3112.c > @@ -66,7 +66,7 @@ static void sii3112_bmdma_write(void *opaque, hwaddr addr, > uint64_t val, unsigned int size) > { > BMDMAState *bm = opaque; > - SiI3112PCIState *d = SII3112_PCI(bm->pci_dev); > + SiI3112PCIState *s = SII3112_PCI(bm->pci_dev); > int i = (bm == &bm->pci_dev->bmdma[0]) ? 0 : 1; > > trace_sii3112_bmdma_write(size, addr, val); > @@ -75,10 +75,10 @@ static void sii3112_bmdma_write(void *opaque, hwaddr addr, > bmdma_cmd_writeb(bm, val); > break; > case 0x01: > - d->regs[i].swdata = val & 0x3f; > + s->regs[i].swdata = val & 0x3f; > break; > case 0x02: > - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 6); > + bmdma_clear_status(bm, val); > break; > default: > break; > @@ -160,8 +160,7 @@ static void sii3112_reg_write(void *opaque, hwaddr addr, > d->regs[0].swdata = val & 0x3f; > break; > case 0x12: > - d->i.bmdma[0].status = (val & 0x60) | (d->i.bmdma[0].status & 1) | > - (d->i.bmdma[0].status & ~val & 6); > + bmdma_clear_status(&d->i.bmdma[0], val); > break; > case 0x18: > bmdma_cmd_writeb(&d->i.bmdma[1], val); > @@ -170,8 +169,7 @@ static void sii3112_reg_write(void *opaque, hwaddr addr, > d->regs[1].swdata = val & 0x3f; > break; > case 0x1a: > - d->i.bmdma[1].status = (val & 0x60) | (d->i.bmdma[1].status & 1) | > - (d->i.bmdma[1].status & ~val & 6); > + bmdma_clear_status(&d->i.bmdma[1], val); > break; > case 0x100: > d->regs[0].scontrol = val & 0xfff; > diff --git a/hw/ide/via.c b/hw/ide/via.c > index 35dd97e49b..afb97f302a 100644 > --- a/hw/ide/via.c > +++ b/hw/ide/via.c > @@ -75,7 +75,7 @@ static void bmdma_write(void *opaque, hwaddr addr, > bmdma_cmd_writeb(bm, val); > break; > case 2: > - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06); > + bmdma_clear_status(bm, val); > break; > default:; > } > diff --git a/hw/ide/trace-events b/hw/ide/trace-events > index a479525e38..d219c64b61 100644 > --- a/hw/ide/trace-events > +++ b/hw/ide/trace-events > @@ -30,6 +30,7 @@ bmdma_write_cmd646(uint64_t addr, uint64_t val) "bmdma: writeb 0x%"PRIx64" : 0x% > # pci.c > bmdma_reset(void) "" > bmdma_cmd_writeb(uint32_t val) "val: 0x%08x" > +bmdma_update_status(uint32_t val) "val: 0x%08x" > bmdma_addr_read(uint64_t data) "data: 0x%016"PRIx64 > bmdma_addr_write(uint64_t data) "data: 0x%016"PRIx64 I see there has been further discussion related to naming of the function: FWIW my preference would be bmdma_status_writeb() since it matches the existing convention for bmdma_cmd_writeb() and is pretty clear that it handles byte-only BMDMA status register writes. I don't mind either way regarding the extra trace-event, although generally if someone has added it then it means they have found it useful. Otherwise LGTM, but I'll wait to see what the final outcome is before adding an R-B. ATB, Mark.
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h index 81e0370202..6a286ad307 100644 --- a/include/hw/ide/pci.h +++ b/include/hw/ide/pci.h @@ -59,6 +59,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); +void bmdma_clear_status(BMDMAState *bm, uint32_t val); void pci_ide_create_devs(PCIDevice *dev); #endif diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c index b9d005a357..973c3ff0dc 100644 --- a/hw/ide/cmd646.c +++ b/hw/ide/cmd646.c @@ -144,7 +144,7 @@ static void bmdma_write(void *opaque, hwaddr addr, cmd646_update_irq(pci_dev); break; case 2: - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06); + bmdma_clear_status(bm, val); break; case 3: if (bm == &bm->pci_dev->bmdma[0]) { diff --git a/hw/ide/pci.c b/hw/ide/pci.c index 3539b162b7..4aa06be7c6 100644 --- a/hw/ide/pci.c +++ b/hw/ide/pci.c @@ -318,6 +318,13 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val) bm->cmd = val & 0x09; } +void bmdma_clear_status(BMDMAState *bm, uint32_t val) +{ + trace_bmdma_update_status(val); + + bm->status = (val & 0x60) | (bm->status & BM_STATUS_DMAING) | (bm->status & ~val & 0x06); +} + static uint64_t bmdma_addr_read(void *opaque, hwaddr addr, unsigned width) { diff --git a/hw/ide/piix.c b/hw/ide/piix.c index 406a67fa0f..9eab615e35 100644 --- a/hw/ide/piix.c +++ b/hw/ide/piix.c @@ -76,7 +76,7 @@ static void bmdma_write(void *opaque, hwaddr addr, bmdma_cmd_writeb(bm, val); break; case 2: - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06); + bmdma_clear_status(bm, val); break; } } diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c index 373c0dd1ee..1180ff55e7 100644 --- a/hw/ide/sii3112.c +++ b/hw/ide/sii3112.c @@ -66,7 +66,7 @@ static void sii3112_bmdma_write(void *opaque, hwaddr addr, uint64_t val, unsigned int size) { BMDMAState *bm = opaque; - SiI3112PCIState *d = SII3112_PCI(bm->pci_dev); + SiI3112PCIState *s = SII3112_PCI(bm->pci_dev); int i = (bm == &bm->pci_dev->bmdma[0]) ? 0 : 1; trace_sii3112_bmdma_write(size, addr, val); @@ -75,10 +75,10 @@ static void sii3112_bmdma_write(void *opaque, hwaddr addr, bmdma_cmd_writeb(bm, val); break; case 0x01: - d->regs[i].swdata = val & 0x3f; + s->regs[i].swdata = val & 0x3f; break; case 0x02: - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 6); + bmdma_clear_status(bm, val); break; default: break; @@ -160,8 +160,7 @@ static void sii3112_reg_write(void *opaque, hwaddr addr, d->regs[0].swdata = val & 0x3f; break; case 0x12: - d->i.bmdma[0].status = (val & 0x60) | (d->i.bmdma[0].status & 1) | - (d->i.bmdma[0].status & ~val & 6); + bmdma_clear_status(&d->i.bmdma[0], val); break; case 0x18: bmdma_cmd_writeb(&d->i.bmdma[1], val); @@ -170,8 +169,7 @@ static void sii3112_reg_write(void *opaque, hwaddr addr, d->regs[1].swdata = val & 0x3f; break; case 0x1a: - d->i.bmdma[1].status = (val & 0x60) | (d->i.bmdma[1].status & 1) | - (d->i.bmdma[1].status & ~val & 6); + bmdma_clear_status(&d->i.bmdma[1], val); break; case 0x100: d->regs[0].scontrol = val & 0xfff; diff --git a/hw/ide/via.c b/hw/ide/via.c index 35dd97e49b..afb97f302a 100644 --- a/hw/ide/via.c +++ b/hw/ide/via.c @@ -75,7 +75,7 @@ static void bmdma_write(void *opaque, hwaddr addr, bmdma_cmd_writeb(bm, val); break; case 2: - bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06); + bmdma_clear_status(bm, val); break; default:; } diff --git a/hw/ide/trace-events b/hw/ide/trace-events index a479525e38..d219c64b61 100644 --- a/hw/ide/trace-events +++ b/hw/ide/trace-events @@ -30,6 +30,7 @@ bmdma_write_cmd646(uint64_t addr, uint64_t val) "bmdma: writeb 0x%"PRIx64" : 0x% # pci.c bmdma_reset(void) "" bmdma_cmd_writeb(uint32_t val) "val: 0x%08x" +bmdma_update_status(uint32_t val) "val: 0x%08x" bmdma_addr_read(uint64_t data) "data: 0x%016"PRIx64 bmdma_addr_write(uint64_t data) "data: 0x%016"PRIx64
Extract bmdma_clear_status() mirroring bmdma_cmd_writeb(). Signed-off-by: Bernhard Beschow <shentey@gmail.com> --- include/hw/ide/pci.h | 1 + hw/ide/cmd646.c | 2 +- hw/ide/pci.c | 7 +++++++ hw/ide/piix.c | 2 +- hw/ide/sii3112.c | 12 +++++------- hw/ide/via.c | 2 +- hw/ide/trace-events | 1 + 7 files changed, 17 insertions(+), 10 deletions(-)