[PATCH 0/2] hw/audio/intel-hda: Don't use device_legacy_reset()

[PATCH 0/2] hw/audio/intel-hda: Don't use device_legacy_reset()

[Peter Maydell]

This patchset removes a use of device_legacy_reset() from the
intel-hda audio device, and removes a duplicated reset of the
codec devices.

Currently the intel-hda device has a reset method which manually
resets all the codecs by calling device_legacy_reset() on them.  This
means they get reset twice, once because child devices on a qbus get
reset before the parent device's reset method is called, and then
again because we're manually resetting them.

Patch 1 drops the manual reset call.  To ensure that codecs are still
reset when the guest does a reset via ICH6_GCTL_RESET we make that
codepath call device_cold_reset() on the HDA device (which resets all
the devices on the qbus as well as the device itself) instead of
making a direct call to the reset function.

This is a slight ordering change because the (only) codec reset now
happens before the controller registers etc are reset, rather than
once before and then once after, but the codec reset function
hda_audio_reset() doesn't care.

Patch 2 is a trivial cleanup I noticed while I was there.

The patchset has been tested with 'make check' and
'make check-avocado' (and a little bit of use of gdb to confirm
that the reset functions still get called as expected).

thanks
-- PMM

Peter Maydell (2):
  hw/audio/intel-hda: don't reset codecs twice
  hw/audio/intel-hda: Drop unnecessary prototype

 hw/audio/intel-hda.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

-- 
2.25.1

[PATCH 1/2] hw/audio/intel-hda: don't reset codecs twice

[Peter Maydell]

Currently the intel-hda device has a reset method which manually
resets all the codecs by calling device_legacy_reset() on them.  This
means they get reset twice, once because child devices on a qbus get
reset before the parent device's reset method is called, and then
again because we're manually resetting them.

Drop the manual reset call, and ensure that codecs are still reset
when the guest does a reset via ICH6_GCTL_RESET by using
device_cold_reset() (which resets all the devices on the qbus as well
as the device itself) instead of a direct call to the reset function.

This is a slight ordering change because the (only) codec reset now
happens before the controller registers etc are reset, rather than
once before and then once after, but the codec reset function
hda_audio_reset() doesn't care.

This lets us drop a use of device_legacy_reset(), which is
deprecated.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/audio/intel-hda.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index f38117057b9..38cfa20262e 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -516,7 +516,7 @@ static void intel_hda_notify_codecs(IntelHDAState *d, uint32_t stream, bool runn
 static void intel_hda_set_g_ctl(IntelHDAState *d, const IntelHDAReg *reg, uint32_t old)
 {
     if ((d->g_ctl & ICH6_GCTL_RESET) == 0) {
-        intel_hda_reset(DEVICE(d));
+        device_cold_reset(DEVICE(d));
     }
 }
 
@@ -1083,11 +1083,9 @@ static void intel_hda_reset(DeviceState *dev)
     intel_hda_regs_reset(d);
     d->wall_base_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
 
-    /* reset codecs */
     QTAILQ_FOREACH(kid, &d->codecs.qbus.children, sibling) {
         DeviceState *qdev = kid->child;
         cdev = HDA_CODEC_DEVICE(qdev);
-        device_legacy_reset(DEVICE(cdev));
         d->state_sts |= (1 << cdev->cad);
     }
     intel_hda_update_irq(d);
-- 
2.25.1

Re: [PATCH 1/2] hw/audio/intel-hda: don't reset codecs twice

[Philippe Mathieu-Daudé]

On 14/10/22 16:26, Peter Maydell wrote:
> Currently the intel-hda device has a reset method which manually
> resets all the codecs by calling device_legacy_reset() on them.  This
> means they get reset twice, once because child devices on a qbus get
> reset before the parent device's reset method is called, and then
> again because we're manually resetting them.
> 
> Drop the manual reset call, and ensure that codecs are still reset
> when the guest does a reset via ICH6_GCTL_RESET by using
> device_cold_reset() (which resets all the devices on the qbus as well
> as the device itself) instead of a direct call to the reset function.
> 
> This is a slight ordering change because the (only) codec reset now
> happens before the controller registers etc are reset, rather than
> once before and then once after, but the codec reset function
> hda_audio_reset() doesn't care.
> 
> This lets us drop a use of device_legacy_reset(), which is
> deprecated.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/audio/intel-hda.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 2/2] hw/audio/intel-hda: Drop unnecessary prototype

[Peter Maydell]

The only use of intel_hda_reset() is after its definition, so we
don't need to separately declare its prototype at the top of the
file; drop the unnecessary line.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/audio/intel-hda.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index 38cfa20262e..b9ed231fe84 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -220,8 +220,6 @@ struct IntelHDAReg {
     void       (*rhandler)(IntelHDAState *d, const IntelHDAReg *reg);
 };
 
-static void intel_hda_reset(DeviceState *dev);
-
 /* --------------------------------------------------------------------- */
 
 static hwaddr intel_hda_addr(uint32_t lbase, uint32_t ubase)
-- 
2.25.1

Re: [PATCH 2/2] hw/audio/intel-hda: Drop unnecessary prototype

[Philippe Mathieu-Daudé]

On 14/10/22 16:26, Peter Maydell wrote:
> The only use of intel_hda_reset() is after its definition, so we
> don't need to separately declare its prototype at the top of the
> file; drop the unnecessary line.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/audio/intel-hda.c | 2 --
>   1 file changed, 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>