Re: [PATCH-for-9.1 v3 1/2] hw/pci-host/gt64120: Reset config registers during RESET phase

[Peter Maydell]

On Thu, 1 Aug 2024 at 18:03, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> +Peter who is tackling our Reset interface limitations,
> +Daniel for deprecation advices.
>
> On 1/8/24 17:37, Michael S. Tsirkin wrote:
> > On Thu, Aug 01, 2024 at 05:30:38PM +0200, BALATON Zoltan wrote:
> >> On Thu, 1 Aug 2024, Philippe Mathieu-Daudé wrote:
> >>> Reset config values in the device RESET phase, not only once
> >>> when the device is realized, because otherwise the device can
> >>> use unknown values at reset.
> >>>
> >>> Mention the datasheet referenced. Remove the "Malta assumptions
> >>> ahead" comment since the reset values from the datasheet are used.
> >>>
> >>> Reported-by: Michael S. Tsirkin <mst@redhat.com>
> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >>> ---
> >>> hw/pci-host/gt64120.c | 14 +++++++++++---
> >>> 1 file changed, 11 insertions(+), 3 deletions(-)
>
>
> >>> @@ -1231,8 +1238,9 @@ static void gt64120_pci_class_init(ObjectClass 
> >>> *klass, void *data)
> >>> {
> >>>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> >>>      DeviceClass *dc = DEVICE_CLASS(klass);
> >>> +    ResettableClass *rc = RESETTABLE_CLASS(klass);
> >>>
> >>> -    k->realize = gt64120_pci_realize;
> >>> +    rc->phases.hold = gt64120_pci_reset_hold;
> >>
> >> Why reset_hold and not a simple reset method which is more usual?
>
> DeviceReset is deprecated since 4 years now, see commit c11256aa6f
> ("hw/core: add Resettable support to BusClass and DeviceClass") and
> the effort to convert the legacy interface to this new API:

> Peter, Daniel, do we have a way to hint developers about
> deprecated API uses (like for versioned machines macros,
> commit a35f8577a0 "hw: add macros for deprecation & removal
> of versioned machines"), to settle on a release when API
> must be converted by?

For reset, the stuff I really want to get converted is
the complex stuff (eg bus reset, cases where a device
reset method needs to call its parent method, etc), and
the ancient legacy stuff (eg qemu_register_reset()).
Converting that will make the reset process more uniform
and allow us to get rid of some annoying inter-compatibility
machinery. (Some of this I've already done -- if you look
at the commits in that list, you'll see that a lot of them
are conversions because those classes were using some API
I wanted to remove like device_class_set_parent_reset().
Still haven't quite got rid of that because s390 CPUs are
doing something a bit awkward...)

Also devices where there's something it needs to do in
reset that should properly be done in a phase other than
'hold' obviously need conversion.

For a simple leaf device reset, a DeviceClass::reset method
and a ResettableClass::reset_hold method are essentially
identical, and the amount of glue code we need to make
the Resettable machinery be able to call a DeviceClass:reset
method is minimal. So I don't care about trying to convert
any of the existing uses in the tree or marking
DeviceClass::reset as deprecated.

If we're adding a reset method to a device which didn't
previously have it, I guess Resettable is preferable,
but I don't feel strongly enough about that to ask for
a change at code-review time, and I suspect I've written
new DeviceClass::reset methods myself.

thanks
-- PMM