[PATCH-for-9.1 v3 0/2] hw/pci-host/gt64120: Set PCI base address register write mask

[PATCH-for-9.1 v3 0/2] hw/pci-host/gt64120: Set PCI base address register write mask

[Philippe Mathieu-Daudé]

v3: Move comment from patch 2 to 1
v2: Move reset values from Realize to ResetHold (mst)

Philippe Mathieu-Daudé (2):
  hw/pci-host/gt64120: Reset config registers during RESET phase
  hw/pci-host/gt64120: Set PCI base address register write mask

 hw/pci-host/gt64120.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

-- 
2.45.2

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

[Philippe Mathieu-Daudé]

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;
     k->vendor_id = PCI_VENDOR_ID_MARVELL;
     k->device_id = PCI_DEVICE_ID_MARVELL_GT6412X;
     k->revision = 0x10;
-- 
2.45.2

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

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2878 bytes --]

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

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

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

[Philippe Mathieu-Daudé]

+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:

$ git log --oneline |egrep -i 'convert.*(Resettable|3.*phase)'
cee78fa513 hw/arm/stellaris: Convert I2C controller to Resettable interface
bebd89e166 hw/arm/stellaris: Convert ADC controller to Resettable interface
ead17ebf53 hw/intc/pxa2xx: Convert to Resettable interface
d43e967f69 q800-glue.c: convert to Resettable interface
4dd5cb5d84 hw/pci-host/bonito: Convert to 3-phase reset
a0c2e80afc hw/pci-host/pnv_phb3_msi: Convert TYPE_PHB3_MSI to 3-phase reset
a359da4c62 hw/intc/xics: Convert TYPE_ICS to 3-phase reset
f4c636b0c2 pci: Convert child classes of TYPE_PCIE_ROOT_PORT to 3-phase 
reset
bb27210c8c pci: Convert TYPE_PCIE_ROOT_PORT to 3-phase reset
0d89890466 hw/display/virtio-vga: Convert TYPE_VIRTIO_VGA_BASE to 
3-phase reset
54da41834f hw/virtio: Convert TYPE_VIRTIO_PCI to 3-phase reset
d66e64dd00 target/xtensa: Convert to 3-phase reset
efcc10682e target/tricore: Convert to 3-phase reset
3b4fff1bd5 target/sparc: Convert to 3-phase reset
9049383002 target/sh4: Convert to 3-phase reset
88c41e4082 target/rx: Convert to 3-phase reset
4fa485a78e target/riscv: Convert to 3-phase reset
a1c5d644b7 target/ppc: Convert to 3-phase reset
0409750479 target/openrisc: Convert to 3-phase reset
4245a71662 target/nios2: Convert to 3-phase reset
c08dfb7ae2 target/mips: Convert to 3-phase reset
d4bc6c1a79 target/microblaze: Convert to 3-phase reset
bf90b345d7 target/m68k: Convert to 3-phase reset
f78b49ae8d target/loongarch: Convert to 3-phase reset
e86787d33b target/i386: Convert to 3-phase reset
ab85156d8a target/hexagon: Convert to 3-phase reset
1d2eb1c0c5 target/cris: Convert to 3-phase reset
605787606e target/avr: Convert to 3-phase reset
9130cade5f target/arm: Convert to 3-phase reset
3b750f1b1a hw/core/cpu-common: Convert TYPE_CPU class to 3-phase reset
ed053e8997 hw/misc: Convert TYPE_MOS6522 subclasses to 3-phase reset
8bdaed0f30 hw/misc/mos6522: Convert TYPE_MOS6522 to 3-phase reset
fc2fc3c1ed hw/input/ps2.c: Convert TYPE_PS2_{KBD, MOUSE}_DEVICE to 
3-phase reset
2bb3f93037 hw/input/ps2: Convert TYPE_PS2_DEVICE to 3-phase reset
227b5866c0 hw/intc: Convert TYPE_KVM_ARM_ITS to 3-phase reset
1bcb90762b hw/intc: Convert TYPE_ARM_GICV3_ITS to 3-phase reset
1f6887616f hw/intc: Convert TYPE_ARM_GICV3_ITS_COMMON to 3-phase reset
823300f0fc hw/intc: Convert TYPE_KVM_ARM_GICV3 to 3-phase reset
183cac319e hw/intc: Convert TYPE_ARM_GICV3_COMMON to 3-phase reset
d39270b559 hw/intc: Convert TYPE_ARM_GIC_KVM to 3-phase reset
fe3c6174f2 hw/intc: Convert TYPE_ARM_GIC_COMMON to 3-phase reset
503819a347 hw/arm: Convert TYPE_ARM_SMMUV3 to 3-phase reset
3c1a7c4197 hw/arm: Convert TYPE_ARM_SMMU to 3-phase reset
ef4989b0a8 hw/gpio/pl061: Convert to 3-phase reset and assert GPIO lines 
correctly on reset
fae5a04207 hw/rtc/mc146818rtc: Convert to 3-phase reset (Resettable 
interface)
72fe4742c6 hw/timer/etraxfs_timer: Convert to 3-phase reset (Resettable 
interface)

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?

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

I'll set wmask in the DeviceRealize handler.

Thanks,

Phil.

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

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

[Philippe Mathieu-Daudé]

On 1/8/24 19:13, Peter Maydell wrote:
> 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 for clarifying. I concur Resettable is preferable
when adding a reset method.

[PATCH-for-9.1 v3 2/2] hw/pci-host/gt64120: Set PCI base address register write mask

[Philippe Mathieu-Daudé]

When booting Linux we see:

  PCI host bridge to bus 0000:00
  pci_bus 0000:00: root bus resource [mem 0x10000000-0x17ffffff]
  pci_bus 0000:00: root bus resource [io  0x1000-0x1fffff]
  pci_bus 0000:00: No busn resource found for root bus, will use [bus 00-ff]
  pci 0000:00:00.0: [11ab:4620] type 00 class 0x060000
  pci 0000:00:00.0: [Firmware Bug]: reg 0x14: invalid BAR (can't size)
  pci 0000:00:00.0: [Firmware Bug]: reg 0x18: invalid BAR (can't size)
  pci 0000:00:00.0: [Firmware Bug]: reg 0x1c: invalid BAR (can't size)
  pci 0000:00:00.0: [Firmware Bug]: reg 0x20: invalid BAR (can't size)
  pci 0000:00:00.0: [Firmware Bug]: reg 0x24: invalid BAR (can't size)

This is due to missing base address register write mask.
Add it to get:

  PCI host bridge to bus 0000:00
  pci_bus 0000:00: root bus resource [mem 0x10000000-0x17ffffff]
  pci_bus 0000:00: root bus resource [io  0x1000-0x1fffff]
  pci_bus 0000:00: No busn resource found for root bus, will use [bus 00-ff]
  pci 0000:00:00.0: [11ab:4620] type 00 class 0x060000
  pci 0000:00:00.0: reg 0x10: [mem 0x00000000-0x00000fff pref]
  pci 0000:00:00.0: reg 0x14: [mem 0x01000000-0x01000fff pref]
  pci 0000:00:00.0: reg 0x18: [mem 0x1c000000-0x1c000fff]
  pci 0000:00:00.0: reg 0x1c: [mem 0x1f000000-0x1f000fff]
  pci 0000:00:00.0: reg 0x20: [mem 0x1be00000-0x1be00fff]
  pci 0000:00:00.0: reg 0x24: [io  0x14000000-0x14000fff]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/pci-host/gt64120.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/pci-host/gt64120.c b/hw/pci-host/gt64120.c
index b68d647753..344baf55db 100644
--- a/hw/pci-host/gt64120.c
+++ b/hw/pci-host/gt64120.c
@@ -1224,6 +1224,13 @@ static void gt64120_pci_reset_hold(Object *obj, ResetType type)
                  PCI_STATUS_FAST_BACK | PCI_STATUS_DEVSEL_MEDIUM);
     pci_config_set_prog_interface(d->config, 0);
 
+    pci_set_long(d->wmask + PCI_BASE_ADDRESS_0, 0xfffff009);
+    pci_set_long(d->wmask + PCI_BASE_ADDRESS_1, 0xfffff009);
+    pci_set_long(d->wmask + PCI_BASE_ADDRESS_2, 0xfffff009);
+    pci_set_long(d->wmask + PCI_BASE_ADDRESS_3, 0xfffff009);
+    pci_set_long(d->wmask + PCI_BASE_ADDRESS_4, 0xfffff009);
+    pci_set_long(d->wmask + PCI_BASE_ADDRESS_5, 0xfffff001);
+
     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);
-- 
2.45.2