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

[BALATON Zoltan]

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(-)
>
> diff --git a/hw/pci-host/gt64120.c b/hw/pci-host/gt64120.c
> index e02efc9e2e..b68d647753 100644
> --- a/hw/pci-host/gt64120.c
> +++ b/hw/pci-host/gt64120.c
> @@ -1,6 +1,8 @@
> /*
>  * QEMU GT64120 PCI host
>  *
> + * (Datasheet GT-64120 Rev 1.4 from Sep 14, 1999)
> + *
>  * Copyright (c) 2006,2007 Aurelien Jarno
>  *
>  * Permission is hereby granted, free of charge, to any person obtaining a copy
> @@ -1211,19 +1213,24 @@ static void gt64120_realize(DeviceState *dev, Error 
> **errp)
>     empty_slot_init("GT64120", 0, 0x20000000);
> }
>
> -static void gt64120_pci_realize(PCIDevice *d, Error **errp)
> +static void gt64120_pci_reset_hold(Object *obj, ResetType type)
> {
> -    /* FIXME: Malta specific hw assumptions ahead */
> +    PCIDevice *d = PCI_DEVICE(obj);
> +
> +    /* Values from chapter 17.16 "PCI Configuration" */
> +
>     pci_set_word(d->config + PCI_COMMAND, 0);
>     pci_set_word(d->config + PCI_STATUS,
>                  PCI_STATUS_FAST_BACK | PCI_STATUS_DEVSEL_MEDIUM);
>     pci_config_set_prog_interface(d->config, 0);
> +
>     pci_set_long(d->config + PCI_BASE_ADDRESS_0, 0x00000008);
>     pci_set_long(d->config + PCI_BASE_ADDRESS_1, 0x01000008);
>     pci_set_long(d->config + PCI_BASE_ADDRESS_2, 0x1c000000);
>     pci_set_long(d->config + PCI_BASE_ADDRESS_3, 0x1f000000);
>     pci_set_long(d->config + PCI_BASE_ADDRESS_4, 0x14000000);
>     pci_set_long(d->config + PCI_BASE_ADDRESS_5, 0x14000001);
> +
>     pci_set_byte(d->config + 0x3d, 0x01);
> }
>
> @@ -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?  If 
there's an explanation maybe it could be mentioned in the commit message. 
Other than that this should work so you can add:

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

if that helps but I don't know much about this chip (even if it's similar 
to mv6436x).

Regards,
BALATON Zoltan

>     k->vendor_id = PCI_VENDOR_ID_MARVELL;
>     k->device_id = PCI_DEVICE_ID_MARVELL_GT6412X;
>     k->revision = 0x10;