qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH-for-9.1 v4 0/2] hw/pci-host/gt64120: Set PCI base address register write mask
@ 2024-08-02 17:10 Philippe Mathieu-Daudé
  2024-08-02 17:10 ` [PATCH-for-9.1 v4 1/2] " Philippe Mathieu-Daudé
  2024-08-02 17:10 ` [PATCH-for-9.1 v4 2/2] hw/pci-host/gt64120: Reset config registers during RESET phase Philippe Mathieu-Daudé
  0 siblings, 2 replies; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-08-02 17:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Aurelien Jarno, Philippe Mathieu-Daudé, Michael S . Tsirkin,
	BALATON Zoltan

v4:
- invert patches order
- set wmask in DeviceRealize
- update patch descriptions (migration, Resettable API)

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

 hw/pci-host/gt64120.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

-- 
2.45.2



^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH-for-9.1 v4 1/2] hw/pci-host/gt64120: Set PCI base address register write mask
  2024-08-02 17:10 [PATCH-for-9.1 v4 0/2] hw/pci-host/gt64120: Set PCI base address register write mask Philippe Mathieu-Daudé
@ 2024-08-02 17:10 ` Philippe Mathieu-Daudé
  2024-08-02 19:21   ` BALATON Zoltan
  2024-08-02 17:10 ` [PATCH-for-9.1 v4 2/2] hw/pci-host/gt64120: Reset config registers during RESET phase Philippe Mathieu-Daudé
  1 sibling, 1 reply; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-08-02 17:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Aurelien Jarno, Philippe Mathieu-Daudé, Michael S . Tsirkin,
	BALATON Zoltan

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]

Since this device is only used by MIPS machines which aren't
versioned, we don't need to update migration compat machinery.

Mention the datasheet referenced. Remove the "Malta assumptions
ahead" comment since the reset values from the datasheet are used.

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

diff --git a/hw/pci-host/gt64120.c b/hw/pci-host/gt64120.c
index e02efc9e2e..7df2855f32 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
@@ -1213,17 +1215,27 @@ static void gt64120_realize(DeviceState *dev, Error **errp)
 
 static void gt64120_pci_realize(PCIDevice *d, Error **errp)
 {
-    /* FIXME: Malta specific hw assumptions ahead */
+    /* 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->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);
     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);
 }
 
-- 
2.45.2



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH-for-9.1 v4 2/2] hw/pci-host/gt64120: Reset config registers during RESET phase
  2024-08-02 17:10 [PATCH-for-9.1 v4 0/2] hw/pci-host/gt64120: Set PCI base address register write mask Philippe Mathieu-Daudé
  2024-08-02 17:10 ` [PATCH-for-9.1 v4 1/2] " Philippe Mathieu-Daudé
@ 2024-08-02 17:10 ` Philippe Mathieu-Daudé
  1 sibling, 0 replies; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-08-02 17:10 UTC (permalink / raw)
  To: qemu-devel
  Cc: Aurelien Jarno, Philippe Mathieu-Daudé, Michael S . Tsirkin,
	BALATON Zoltan

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.

Since we are adding a new reset method, use the preferred
Resettable API (for a simple leaf device reset, a
DeviceClass::reset method and a ResettableClass::reset_hold
method are essentially identical).

Reported-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/pci-host/gt64120.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/hw/pci-host/gt64120.c b/hw/pci-host/gt64120.c
index 7df2855f32..b0d05db577 100644
--- a/hw/pci-host/gt64120.c
+++ b/hw/pci-host/gt64120.c
@@ -1217,17 +1217,24 @@ static void gt64120_pci_realize(PCIDevice *d, Error **errp)
 {
     /* 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->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);
+}
+
+static void gt64120_pci_reset_hold(Object *obj, ResetType type)
+{
+    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);
@@ -1243,7 +1250,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);
 
+    rc->phases.hold = gt64120_pci_reset_hold;
     k->realize = gt64120_pci_realize;
     k->vendor_id = PCI_VENDOR_ID_MARVELL;
     k->device_id = PCI_DEVICE_ID_MARVELL_GT6412X;
-- 
2.45.2



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH-for-9.1 v4 1/2] hw/pci-host/gt64120: Set PCI base address register write mask
  2024-08-02 17:10 ` [PATCH-for-9.1 v4 1/2] " Philippe Mathieu-Daudé
@ 2024-08-02 19:21   ` BALATON Zoltan
  2024-08-02 21:30     ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 5+ messages in thread
From: BALATON Zoltan @ 2024-08-02 19:21 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-devel, Aurelien Jarno, Michael S . Tsirkin

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

On Fri, 2 Aug 2024, Philippe Mathieu-Daudé wrote:
> 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]
>
> Since this device is only used by MIPS machines which aren't
> versioned, we don't need to update migration compat machinery.
>
> Mention the datasheet referenced. Remove the "Malta assumptions
> ahead" comment since the reset values from the datasheet are used.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> hw/pci-host/gt64120.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/hw/pci-host/gt64120.c b/hw/pci-host/gt64120.c
> index e02efc9e2e..7df2855f32 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
> @@ -1213,17 +1215,27 @@ static void gt64120_realize(DeviceState *dev, Error **errp)
>
> static void gt64120_pci_realize(PCIDevice *d, Error **errp)
> {
> -    /* FIXME: Malta specific hw assumptions ahead */
> +    /* 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->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);

Documentation says bit 0 is read only 0 for these? Why mask ending with 9 
not 8? Also prefetch bit 3 is read only 0 for the last one BAR4. 
Otherwise:

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

Regards,
BALATON Zoltan

> +    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);
>     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);
> }
>
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH-for-9.1 v4 1/2] hw/pci-host/gt64120: Set PCI base address register write mask
  2024-08-02 19:21   ` BALATON Zoltan
@ 2024-08-02 21:30     ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-08-02 21:30 UTC (permalink / raw)
  To: BALATON Zoltan; +Cc: qemu-devel, Aurelien Jarno, Michael S . Tsirkin

On 2/8/24 21:21, BALATON Zoltan wrote:
> On Fri, 2 Aug 2024, Philippe Mathieu-Daudé wrote:
>> 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]
>>
>> Since this device is only used by MIPS machines which aren't
>> versioned, we don't need to update migration compat machinery.
>>
>> Mention the datasheet referenced. Remove the "Malta assumptions
>> ahead" comment since the reset values from the datasheet are used.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> hw/pci-host/gt64120.c | 14 +++++++++++++-
>> 1 file changed, 13 insertions(+), 1 deletion(-)


>> static void gt64120_pci_realize(PCIDevice *d, Error **errp)
>> {
>> -    /* FIXME: Malta specific hw assumptions ahead */
>> +    /* 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->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);
> 
> Documentation says bit 0 is read only 0 for these? Why mask ending with 
> 9 not 8? Also prefetch bit 3 is read only 0 for the last one BAR4.

Oops indeed, and also Type/Prefetch for BAR5; updated as:

pci_set_long(d->wmask + PCI_BASE_ADDRESS_0, 0xfffff008); /* SCS[1:0] */
pci_set_long(d->wmask + PCI_BASE_ADDRESS_1, 0xfffff008); /* SCS[3:2] */
pci_set_long(d->wmask + PCI_BASE_ADDRESS_2, 0xfffff008); /* CS[2:0] */
pci_set_long(d->wmask + PCI_BASE_ADDRESS_3, 0xfffff008); /* CS[3], BootCS */
pci_set_long(d->wmask + PCI_BASE_ADDRESS_4, 0xfffff000); /* ISD MMIO */
pci_set_long(d->wmask + PCI_BASE_ADDRESS_5, 0xfffff001); /* ISD I/O */

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

Thanks!

Phil.


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2024-08-02 21:31 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-02 17:10 [PATCH-for-9.1 v4 0/2] hw/pci-host/gt64120: Set PCI base address register write mask Philippe Mathieu-Daudé
2024-08-02 17:10 ` [PATCH-for-9.1 v4 1/2] " Philippe Mathieu-Daudé
2024-08-02 19:21   ` BALATON Zoltan
2024-08-02 21:30     ` Philippe Mathieu-Daudé
2024-08-02 17:10 ` [PATCH-for-9.1 v4 2/2] hw/pci-host/gt64120: Reset config registers during RESET phase Philippe Mathieu-Daudé

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).