| Message ID | 9b78e8c6e453feab6275d04bf503051645770d85.1629799776.git.fthain@linux-m68k.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,01/10] hw/mos6522: Remove get_load_time() methods and functions | expand |
On 8/24/21 12:09 PM, Finn Thain wrote: > This code appears to be unnecessary. > > Signed-off-by: Finn Thain <fthain@linux-m68k.org> > --- > hw/misc/mos6522.c | 22 +--------------------- > 1 file changed, 1 insertion(+), 21 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
On 24/08/2021 11:09, Finn Thain wrote: > This code appears to be unnecessary. > > Signed-off-by: Finn Thain <fthain@linux-m68k.org> > --- > hw/misc/mos6522.c | 22 +--------------------- > 1 file changed, 1 insertion(+), 21 deletions(-) > > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c > index 1c57332b40..a478c1ca43 100644 > --- a/hw/misc/mos6522.c > +++ b/hw/misc/mos6522.c > @@ -63,17 +63,6 @@ static uint64_t get_counter_value(MOS6522State *s, MOS6522Timer *ti) > } > } > > -static uint64_t get_load_time(MOS6522State *s, MOS6522Timer *ti) > -{ > - MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s); > - > - if (ti->index == 0) { > - return mdc->get_timer1_load_time(s, ti); > - } else { > - return mdc->get_timer2_load_time(s, ti); > - } > -} > - > static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti) > { > int64_t d; > @@ -98,7 +87,7 @@ static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti) > static void set_counter(MOS6522State *s, MOS6522Timer *ti, unsigned int val) > { > trace_mos6522_set_counter(1 + ti->index, val); > - ti->load_time = get_load_time(s, ti); > + ti->load_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > ti->counter_value = val; > if (ti->index == 0) { > mos6522_timer1_update(s, ti, ti->load_time); > @@ -208,13 +197,6 @@ static uint64_t mos6522_get_counter_value(MOS6522State *s, MOS6522Timer *ti) > ti->frequency, NANOSECONDS_PER_SECOND); > } > > -static uint64_t mos6522_get_load_time(MOS6522State *s, MOS6522Timer *ti) > -{ > - uint64_t load_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > - > - return load_time; > -} > - > static void mos6522_portA_write(MOS6522State *s) > { > qemu_log_mask(LOG_UNIMP, "portA_write unimplemented\n"); > @@ -518,8 +500,6 @@ static void mos6522_class_init(ObjectClass *oc, void *data) > mdc->update_irq = mos6522_update_irq; > mdc->get_timer1_counter_value = mos6522_get_counter_value; > mdc->get_timer2_counter_value = mos6522_get_counter_value; > - mdc->get_timer1_load_time = mos6522_get_load_time; > - mdc->get_timer2_load_time = mos6522_get_load_time; > } > > static const TypeInfo mos6522_type_info = { Both the get_counter_value() and get_load_time() callbacks are used as part of the CUDA emulation in hw/misc/macio/cuda.c as per the comment: /* MacOS uses timer 1 for calibration on startup, so we use * the timebase frequency and cuda_get_counter_value() with * cuda_get_load_time() to steer MacOS to calculate calibrate its timers * correctly for both TCG and KVM (see commit b981289c49 "PPC: Cuda: Use cuda * timer to expose tbfreq to guest" for more information) */ Certainly for the 6522 device it is worth configuring with --target-list="ppc-softmmu m68k-softmmu" to make sure that you don't inadvertently break anything in the PPC world. A bit of history here: the original mos6522.c was extracted from hw/misc/macio/cuda.c when Laurent presented his initial q800 patches since they also had their own implementation of the 6522, and it was better to move the implementation into a separate QEMU device so that the logic could be shared. The Darwin kernel timer calibration loop is quite hard to get right: see https://opensource.apple.com/source/xnu/xnu-123.5/pexpert/ppc/pe_clock_speed_asm.s.auto.html and https://opensource.apple.com/source/xnu/xnu-123.5/pexpert/ppc/pe_clock_speed.c.auto.html. Ben/Alex came up with the current mechanism to fool the calibration routine, and I simply added in those callbacks to allow it to be implemented as part of the now-generic 6522 device. ATB, Mark.
On Wed, 25 Aug 2021, Mark Cave-Ayland wrote: > On 24/08/2021 11:09, Finn Thain wrote: > > > This code appears to be unnecessary. > > > > Signed-off-by: Finn Thain <fthain@linux-m68k.org> > > --- > > hw/misc/mos6522.c | 22 +--------------------- > > 1 file changed, 1 insertion(+), 21 deletions(-) > > > > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c > > index 1c57332b40..a478c1ca43 100644 > > --- a/hw/misc/mos6522.c > > +++ b/hw/misc/mos6522.c > > @@ -63,17 +63,6 @@ static uint64_t get_counter_value(MOS6522State *s, > > MOS6522Timer *ti) > > } > > } > > -static uint64_t get_load_time(MOS6522State *s, MOS6522Timer *ti) > > -{ > > - MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s); > > - > > - if (ti->index == 0) { > > - return mdc->get_timer1_load_time(s, ti); > > - } else { > > - return mdc->get_timer2_load_time(s, ti); > > - } > > -} > > - > > static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti) > > { > > int64_t d; > > @@ -98,7 +87,7 @@ static unsigned int get_counter(MOS6522State *s, > > MOS6522Timer *ti) > > static void set_counter(MOS6522State *s, MOS6522Timer *ti, unsigned int > > val) > > { > > trace_mos6522_set_counter(1 + ti->index, val); > > - ti->load_time = get_load_time(s, ti); > > + ti->load_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > > ti->counter_value = val; > > if (ti->index == 0) { > > mos6522_timer1_update(s, ti, ti->load_time); > > @@ -208,13 +197,6 @@ static uint64_t mos6522_get_counter_value(MOS6522State > > *s, MOS6522Timer *ti) > > ti->frequency, NANOSECONDS_PER_SECOND); > > } > > -static uint64_t mos6522_get_load_time(MOS6522State *s, MOS6522Timer *ti) > > -{ > > - uint64_t load_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > > - > > - return load_time; > > -} > > - > > static void mos6522_portA_write(MOS6522State *s) > > { > > qemu_log_mask(LOG_UNIMP, "portA_write unimplemented\n"); > > @@ -518,8 +500,6 @@ static void mos6522_class_init(ObjectClass *oc, void > > *data) > > mdc->update_irq = mos6522_update_irq; > > mdc->get_timer1_counter_value = mos6522_get_counter_value; > > mdc->get_timer2_counter_value = mos6522_get_counter_value; > > - mdc->get_timer1_load_time = mos6522_get_load_time; > > - mdc->get_timer2_load_time = mos6522_get_load_time; > > } > > static const TypeInfo mos6522_type_info = { > > Both the get_counter_value() and get_load_time() callbacks are used as part of > the CUDA emulation in hw/misc/macio/cuda.c as per the comment: > > /* MacOS uses timer 1 for calibration on startup, so we use > * the timebase frequency and cuda_get_counter_value() with > * cuda_get_load_time() to steer MacOS to calculate calibrate its timers > * correctly for both TCG and KVM (see commit b981289c49 "PPC: Cuda: Use cuda > * timer to expose tbfreq to guest" for more information) */ > > Certainly for the 6522 device it is worth configuring with > --target-list="ppc-softmmu m68k-softmmu" to make sure that you don't > inadvertently break anything in the PPC world. > No build failure here. Maybe your tree is different? > A bit of history here: the original mos6522.c was extracted from > hw/misc/macio/cuda.c when Laurent presented his initial q800 patches since > they also had their own implementation of the 6522, and it was better to move > the implementation into a separate QEMU device so that the logic could be > shared. > > The Darwin kernel timer calibration loop is quite hard to get right: see > https://opensource.apple.com/source/xnu/xnu-123.5/pexpert/ppc/pe_clock_speed_asm.s.auto.html > and > https://opensource.apple.com/source/xnu/xnu-123.5/pexpert/ppc/pe_clock_speed.c.auto.html. > Ben/Alex came up with the current mechanism to fool the calibration routine, > and I simply added in those callbacks to allow it to be implemented as part of > the now-generic 6522 device. > I didn't find any references to these methods (get_timer1_counter_value, get_timer2_counter_value, get_timer1_load_time and get_timer2_load_time). It appears to be dead code, and it adds complexity and harms readability. The Linux kernel project has a policy that no code is added if it lacks any in-tree usage. Does QEMU have the same policy?
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c index 1c57332b40..a478c1ca43 100644 --- a/hw/misc/mos6522.c +++ b/hw/misc/mos6522.c @@ -63,17 +63,6 @@ static uint64_t get_counter_value(MOS6522State *s, MOS6522Timer *ti) } } -static uint64_t get_load_time(MOS6522State *s, MOS6522Timer *ti) -{ - MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s); - - if (ti->index == 0) { - return mdc->get_timer1_load_time(s, ti); - } else { - return mdc->get_timer2_load_time(s, ti); - } -} - static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti) { int64_t d; @@ -98,7 +87,7 @@ static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti) static void set_counter(MOS6522State *s, MOS6522Timer *ti, unsigned int val) { trace_mos6522_set_counter(1 + ti->index, val); - ti->load_time = get_load_time(s, ti); + ti->load_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); ti->counter_value = val; if (ti->index == 0) { mos6522_timer1_update(s, ti, ti->load_time); @@ -208,13 +197,6 @@ static uint64_t mos6522_get_counter_value(MOS6522State *s, MOS6522Timer *ti) ti->frequency, NANOSECONDS_PER_SECOND); } -static uint64_t mos6522_get_load_time(MOS6522State *s, MOS6522Timer *ti) -{ - uint64_t load_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); - - return load_time; -} - static void mos6522_portA_write(MOS6522State *s) { qemu_log_mask(LOG_UNIMP, "portA_write unimplemented\n"); @@ -518,8 +500,6 @@ static void mos6522_class_init(ObjectClass *oc, void *data) mdc->update_irq = mos6522_update_irq; mdc->get_timer1_counter_value = mos6522_get_counter_value; mdc->get_timer2_counter_value = mos6522_get_counter_value; - mdc->get_timer1_load_time = mos6522_get_load_time; - mdc->get_timer2_load_time = mos6522_get_load_time; } static const TypeInfo mos6522_type_info = {
This code appears to be unnecessary. Signed-off-by: Finn Thain <fthain@linux-m68k.org> --- hw/misc/mos6522.c | 22 +--------------------- 1 file changed, 1 insertion(+), 21 deletions(-)