| Message ID | 20240801150021.52977-2-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/pci-host/gt64120: Set PCI base address register write mask | expand |
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; >
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; > >
+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.
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
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.
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;
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(-)