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

[Michael S. Tsirkin]

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(-)
> > 
> > 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?

Good point. And I'd keep it limited, e.g. wmask can be set once,
no need to tweak it on reset.

>  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;
> >