[PATCH v5] arch/ppc/qemu: Add a node for the other (empty) PCI bus to the device tree
QEMU mac99 emulates two of the three PCI buses found on real PowerMac3,1 but OpenBIOS only handles a single PCI bus and initialises and puts info in the device tree of the second PCI bus only which is where devices are connected and calls it bus 0. However, some clients may not know about this or have hardcoded assumptions and erroneously use the address of the first bus to access PCI config registers for devices on the second bus which silently fails as these requests will go to the other empty bus emulated and return invalid values. Devices mapped via MMIO still appear to work but they may not be correctly initialised and some cards are not detected because of this. Until support for multiple PCI buses is implemented add an empty node in the device tree for the uninitialised bus to let clients know about it. This fixes detecting PCI devices (such as USB) under MorphOS and may also fix enabling the bus master bit needed with some network cards and allow the workarund for that to be reverted. Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- arch/ppc/qemu/init.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/arch/ppc/qemu/init.c b/arch/ppc/qemu/init.c index f72728c..a996404 100644 --- a/arch/ppc/qemu/init.c +++ b/arch/ppc/qemu/init.c @@ -716,6 +716,46 @@ static void kvm_of_init(void) fword("finish-device"); } +static void empty_pci_bus_init(void) +{ + if (machine_id == ARCH_MAC99) { + activate_device("/"); + fword("new-device"); + push_str("pci"); + fword("device-name"); + push_str("pci"); + fword("device-type"); + PUSH(0xf0000000); + fword("encode-int"); + PUSH(0x02000000); + fword("encode-int"); + fword("encode+"); + push_str("reg"); + fword("property"); + PUSH(3); + fword("encode-int"); + push_str("#address-cells"); + fword("property"); + PUSH(2); + fword("encode-int"); + push_str("#size-cells"); + fword("property"); + PUSH(1); + fword("encode-int"); + push_str("#interrupt-cells"); + fword("property"); + PUSH(0); + fword("encode-int"); + PUSH(0); + fword("encode-int"); + fword("encode+"); + push_str("bus-range"); + fword("property"); + fword("finish-device"); + device_end(); + } +} + /* * filll ( addr bytes quad -- ) */ @@ -859,7 +899,17 @@ arch_of_init(void) feval("['] ppc-dma-sync to (dma-sync)"); #ifdef CONFIG_DRIVER_PCI + /* Add empty node for first pci bus on MAC99 */ + /* Remove this when pci driver is fixed to handle multiple buses */ + empty_pci_bus_init(); ob_pci_init(); + if (machine_id == ARCH_MAC99) { + phandle_t dev = find_dev("/pci@f2000000"); + if (dev) { + u32 props[2] = {0, 1}; + set_property(dev, "bus-range", (char *)props, 2 * sizeof(props[0])); + } + } #endif printk("\n"); -- 2.13.7
On Fri, 28 Jun 2019, BALATON Zoltan wrote:
QEMU mac99 emulates two of the three PCI buses found on real PowerMac3,1 but OpenBIOS only handles a single PCI bus and initialises and puts info in the device tree of the second PCI bus only which is where devices are connected and calls it bus 0. However, some clients may not know about this or have hardcoded assumptions and erroneously use the address of the first bus to access PCI config registers for devices on the second bus which silently fails as these requests will go to the other empty bus emulated and return invalid values. Devices mapped via MMIO still appear to work but they may not be correctly initialised and some cards are not detected because of this.
Until support for multiple PCI buses is implemented add an empty node in the device tree for the uninitialised bus to let clients know about it. This fixes detecting PCI devices (such as USB) under MorphOS and may also fix enabling the bus master bit needed with some network cards and allow the workarund for that to be reverted. ^^^ I've failed to fix this typo again, please fix up during commit.
Regards, BALATON Zoltan
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- arch/ppc/qemu/init.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)
diff --git a/arch/ppc/qemu/init.c b/arch/ppc/qemu/init.c index f72728c..a996404 100644 --- a/arch/ppc/qemu/init.c +++ b/arch/ppc/qemu/init.c @@ -716,6 +716,46 @@ static void kvm_of_init(void) fword("finish-device"); }
+static void empty_pci_bus_init(void) +{ + if (machine_id == ARCH_MAC99) { + activate_device("/"); + fword("new-device"); + push_str("pci"); + fword("device-name"); + push_str("pci"); + fword("device-type"); + PUSH(0xf0000000); + fword("encode-int"); + PUSH(0x02000000); + fword("encode-int"); + fword("encode+"); + push_str("reg"); + fword("property"); + PUSH(3); + fword("encode-int"); + push_str("#address-cells"); + fword("property"); + PUSH(2); + fword("encode-int"); + push_str("#size-cells"); + fword("property"); + PUSH(1); + fword("encode-int"); + push_str("#interrupt-cells"); + fword("property"); + PUSH(0); + fword("encode-int"); + PUSH(0); + fword("encode-int"); + fword("encode+"); + push_str("bus-range"); + fword("property"); + fword("finish-device"); + device_end(); + } +} + /* * filll ( addr bytes quad -- ) */ @@ -859,7 +899,17 @@ arch_of_init(void) feval("['] ppc-dma-sync to (dma-sync)");
#ifdef CONFIG_DRIVER_PCI + /* Add empty node for first pci bus on MAC99 */ + /* Remove this when pci driver is fixed to handle multiple buses */ + empty_pci_bus_init(); ob_pci_init(); + if (machine_id == ARCH_MAC99) { + phandle_t dev = find_dev("/pci@f2000000"); + if (dev) { + u32 props[2] = {0, 1}; + set_property(dev, "bus-range", (char *)props, 2 * sizeof(props[0])); + } + } #endif
printk("\n");
On 28/06/2019 19:59, BALATON Zoltan wrote:
QEMU mac99 emulates two of the three PCI buses found on real PowerMac3,1 but OpenBIOS only handles a single PCI bus and initialises and puts info in the device tree of the second PCI bus only which is where devices are connected and calls it bus 0. However, some clients may not know about this or have hardcoded assumptions and erroneously use the address of the first bus to access PCI config registers for devices on the second bus which silently fails as these requests will go to the other empty bus emulated and return invalid values. Devices mapped via MMIO still appear to work but they may not be correctly initialised and some cards are not detected because of this.
Until support for multiple PCI buses is implemented add an empty node in the device tree for the uninitialised bus to let clients know about it. This fixes detecting PCI devices (such as USB) under MorphOS and may also fix enabling the bus master bit needed with some network cards and allow the workarund for that to be reverted.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- arch/ppc/qemu/init.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)
diff --git a/arch/ppc/qemu/init.c b/arch/ppc/qemu/init.c index f72728c..a996404 100644 --- a/arch/ppc/qemu/init.c +++ b/arch/ppc/qemu/init.c @@ -716,6 +716,46 @@ static void kvm_of_init(void) fword("finish-device"); }
+static void empty_pci_bus_init(void) +{ + if (machine_id == ARCH_MAC99) { + activate_device("/"); + fword("new-device"); + push_str("pci"); + fword("device-name"); + push_str("pci"); + fword("device-type"); + PUSH(0xf0000000); + fword("encode-int"); + PUSH(0x02000000); + fword("encode-int"); + fword("encode+"); + push_str("reg"); + fword("property"); + PUSH(3); + fword("encode-int"); + push_str("#address-cells"); + fword("property"); + PUSH(2); + fword("encode-int"); + push_str("#size-cells"); + fword("property"); + PUSH(1); + fword("encode-int"); + push_str("#interrupt-cells"); + fword("property"); + PUSH(0); + fword("encode-int"); + PUSH(0); + fword("encode-int"); + fword("encode+"); + push_str("bus-range"); + fword("property"); + fword("finish-device"); + device_end(); + } +} + /* * filll ( addr bytes quad -- ) */ @@ -859,7 +899,17 @@ arch_of_init(void) feval("['] ppc-dma-sync to (dma-sync)");
#ifdef CONFIG_DRIVER_PCI + /* Add empty node for first pci bus on MAC99 */ + /* Remove this when pci driver is fixed to handle multiple buses */ + empty_pci_bus_init(); ob_pci_init(); + if (machine_id == ARCH_MAC99) { + phandle_t dev = find_dev("/pci@f2000000"); + if (dev) { + u32 props[2] = {0, 1}; + set_property(dev, "bus-range", (char *)props, 2 * sizeof(props[0])); + } + } #endif
printk("\n");
Patching in the bus-range property after the PCI bus enumeration isn't the right way to do this: as per my previous email, the right solution here is to allow the PCI bus enumeration to start with a non-zero bus id. ATB, Mark.
On Sun, 30 Jun 2019, Mark Cave-Ayland wrote:
On 28/06/2019 19:59, BALATON Zoltan wrote:
QEMU mac99 emulates two of the three PCI buses found on real PowerMac3,1 but OpenBIOS only handles a single PCI bus and initialises and puts info in the device tree of the second PCI bus only which is where devices are connected and calls it bus 0. However, some clients may not know about this or have hardcoded assumptions and erroneously use the address of the first bus to access PCI config registers for devices on the second bus which silently fails as these requests will go to the other empty bus emulated and return invalid values. Devices mapped via MMIO still appear to work but they may not be correctly initialised and some cards are not detected because of this.
Until support for multiple PCI buses is implemented add an empty node in the device tree for the uninitialised bus to let clients know about it. This fixes detecting PCI devices (such as USB) under MorphOS and may also fix enabling the bus master bit needed with some network cards and allow the workarund for that to be reverted.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- arch/ppc/qemu/init.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)
diff --git a/arch/ppc/qemu/init.c b/arch/ppc/qemu/init.c index f72728c..a996404 100644 --- a/arch/ppc/qemu/init.c +++ b/arch/ppc/qemu/init.c @@ -716,6 +716,46 @@ static void kvm_of_init(void) fword("finish-device"); }
+static void empty_pci_bus_init(void) +{ + if (machine_id == ARCH_MAC99) { + activate_device("/"); + fword("new-device"); + push_str("pci"); + fword("device-name"); + push_str("pci"); + fword("device-type"); + PUSH(0xf0000000); + fword("encode-int"); + PUSH(0x02000000); + fword("encode-int"); + fword("encode+"); + push_str("reg"); + fword("property"); + PUSH(3); + fword("encode-int"); + push_str("#address-cells"); + fword("property"); + PUSH(2); + fword("encode-int"); + push_str("#size-cells"); + fword("property"); + PUSH(1); + fword("encode-int"); + push_str("#interrupt-cells"); + fword("property"); + PUSH(0); + fword("encode-int"); + PUSH(0); + fword("encode-int"); + fword("encode+"); + push_str("bus-range"); + fword("property"); + fword("finish-device"); + device_end(); + } +} + /* * filll ( addr bytes quad -- ) */ @@ -859,7 +899,17 @@ arch_of_init(void) feval("['] ppc-dma-sync to (dma-sync)");
#ifdef CONFIG_DRIVER_PCI + /* Add empty node for first pci bus on MAC99 */ + /* Remove this when pci driver is fixed to handle multiple buses */ + empty_pci_bus_init(); ob_pci_init(); + if (machine_id == ARCH_MAC99) { + phandle_t dev = find_dev("/pci@f2000000"); + if (dev) { + u32 props[2] = {0, 1}; + set_property(dev, "bus-range", (char *)props, 2 * sizeof(props[0])); + } + } #endif
printk("\n");
Patching in the bus-range property after the PCI bus enumeration isn't the right way to do this: as per my previous email, the right solution here is to allow the PCI bus enumeration to start with a non-zero bus id.
Then please implement that as I don't know how to do that. The result would be the same so I don't see why this is not acceptable now. This is only touching the qemu init.c and only for MAC99 whereas your suggestion would need changes all over the place that would have a higher chance of breaking something that I can't even test so I won't attempt doing that. If your priorities are to avoid breaking stuff and have device tree corresponding to the real machine then this patch seems to be the easiest way to reach those goals. What is it that you still don't like about this? Regards, BALATON Zoltan
On 30/06/2019 16:53, BALATON Zoltan wrote:
On Sun, 30 Jun 2019, Mark Cave-Ayland wrote:
On 28/06/2019 19:59, BALATON Zoltan wrote:
QEMU mac99 emulates two of the three PCI buses found on real PowerMac3,1 but OpenBIOS only handles a single PCI bus and initialises and puts info in the device tree of the second PCI bus only which is where devices are connected and calls it bus 0. However, some clients may not know about this or have hardcoded assumptions and erroneously use the address of the first bus to access PCI config registers for devices on the second bus which silently fails as these requests will go to the other empty bus emulated and return invalid values. Devices mapped via MMIO still appear to work but they may not be correctly initialised and some cards are not detected because of this.
Until support for multiple PCI buses is implemented add an empty node in the device tree for the uninitialised bus to let clients know about it. This fixes detecting PCI devices (such as USB) under MorphOS and may also fix enabling the bus master bit needed with some network cards and allow the workarund for that to be reverted.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- arch/ppc/qemu/init.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)
diff --git a/arch/ppc/qemu/init.c b/arch/ppc/qemu/init.c index f72728c..a996404 100644 --- a/arch/ppc/qemu/init.c +++ b/arch/ppc/qemu/init.c @@ -716,6 +716,46 @@ static void kvm_of_init(void) fword("finish-device"); }
+static void empty_pci_bus_init(void) +{ + if (machine_id == ARCH_MAC99) { + activate_device("/"); + fword("new-device"); + push_str("pci"); + fword("device-name"); + push_str("pci"); + fword("device-type"); + PUSH(0xf0000000); + fword("encode-int"); + PUSH(0x02000000); + fword("encode-int"); + fword("encode+"); + push_str("reg"); + fword("property"); + PUSH(3); + fword("encode-int"); + push_str("#address-cells"); + fword("property"); + PUSH(2); + fword("encode-int"); + push_str("#size-cells"); + fword("property"); + PUSH(1); + fword("encode-int"); + push_str("#interrupt-cells"); + fword("property"); + PUSH(0); + fword("encode-int"); + PUSH(0); + fword("encode-int"); + fword("encode+"); + push_str("bus-range"); + fword("property"); + fword("finish-device"); + device_end(); + } +} + /* * filll ( addr bytes quad -- ) */ @@ -859,7 +899,17 @@ arch_of_init(void) feval("['] ppc-dma-sync to (dma-sync)");
#ifdef CONFIG_DRIVER_PCI + /* Add empty node for first pci bus on MAC99 */ + /* Remove this when pci driver is fixed to handle multiple buses */ + empty_pci_bus_init(); ob_pci_init(); + if (machine_id == ARCH_MAC99) { + phandle_t dev = find_dev("/pci@f2000000"); + if (dev) { + u32 props[2] = {0, 1}; + set_property(dev, "bus-range", (char *)props, 2 * sizeof(props[0])); + } + } #endif
printk("\n");
Patching in the bus-range property after the PCI bus enumeration isn't the right way to do this: as per my previous email, the right solution here is to allow the PCI bus enumeration to start with a non-zero bus id.
Then please implement that as I don't know how to do that. The result would be the same so I don't see why this is not acceptable now. This is only touching the qemu init.c and only for MAC99 whereas your suggestion would need changes all over the place that would have a higher chance of breaking something that I can't even test so I won't attempt doing that. If your priorities are to avoid breaking stuff and have device tree corresponding to the real machine then this patch seems to be the easiest way to reach those goals. What is it that you still don't like about this?
Errr... I did spend time making the OpenBIOS bridge code generic so that others could use it, so the theory says that all you have to do is feed in an initial value. If we need workarounds then I accept that these are necessary at times, but cases like this where the work is already done then there is no need for this. Also re-reading the patch I'm not sure that {0, 1} is correct under QEMU compared with a real Mac, since there is no PCI bridge present under /pci@f2000000? ATB, Mark.
On Mon, 1 Jul 2019, Mark Cave-Ayland wrote:
Then please implement that as I don't know how to do that. The result would be the same so I don't see why this is not acceptable now. This is only touching the qemu init.c and only for MAC99 whereas your suggestion would need changes all over the place that would have a higher chance of breaking something that I can't even test so I won't attempt doing that. If your priorities are to avoid breaking stuff and have device tree corresponding to the real machine then this patch seems to be the easiest way to reach those goals. What is it that you still don't like about this?
Errr... I did spend time making the OpenBIOS bridge code generic so that others could use it, so the theory says that all you have to do is feed in an initial value. If we need workarounds then I accept that these are necessary at times, but cases like this where the work is already done then there is no need for this.
If you think it's trivial to do then please come up with a patch or provide some detailed hints on what needs to be done because I was not able to find it. Where should I feed an inital value? The ob_pci_init() function has no parameter and adding one would require changing all callers that I can't test so I'd rather not do.
Also re-reading the patch I'm not sure that {0, 1} is correct under QEMU compared with a real Mac, since there is no PCI bridge present under /pci@f2000000?
Feel free to change it to whatever you think is appropriate, MorphOS does not care what value this has and I don't know what other guests would expect. I can resubmit this with any value you suggest but I can't figure out what value should be there other than what I've seen in real hardware device tree dump. (I don't have time to study standards documents so if someone here is familiar with those please share your knowledge to save us time.) Regards, BALATON Zoltan
On Mon, 1 Jul 2019, BALATON Zoltan wrote:
On Mon, 1 Jul 2019, Mark Cave-Ayland wrote:
Errr... I did spend time making the OpenBIOS bridge code generic so that others could use it, so the theory says that all you have to do is feed in an initial value. If we need workarounds then I accept that these are necessary at times, but cases like this where the work is already done then there is no need for this.
If you think it's trivial to do then please come up with a patch or provide some detailed hints on what needs to be done because I was not able to find it. Where should I feed an inital value? The ob_pci_init() function has no parameter and adding one would require changing all callers that I can't test so I'd rather not do.
I've tried adding a bus_num field to arch struct and set bus from that in ob_pci_init() but then OpenBIOS hangs on startup.
Also re-reading the patch I'm not sure that {0, 1} is correct under QEMU compared with a real Mac, since there is no PCI bridge present under /pci@f2000000?
I've also tried setting it to {1, 0} instead but same hang as above so I think OpenBIOS cannot handle anything but bus 0 so I gave up. I'm not going to try to debug this as my workaround works well and all this would be unnecessary when passing device tree from QEMU is implemented as suggested before (that would be needed to support Pegasos2 without too many new hacks and workarounds in OpenBIOS and clean up some of the current mess so would be needed eventually if we want to avoid having yet another open firmware implementation in QEMU). But until then having a workaround to be able to run MorphOS without extra effort would have been nice unless it breaks somthing else. Regards, BALATON Zoltan
On 02/07/2019 12:28, BALATON Zoltan wrote:
On Mon, 1 Jul 2019, BALATON Zoltan wrote:
On Mon, 1 Jul 2019, Mark Cave-Ayland wrote:
Errr... I did spend time making the OpenBIOS bridge code generic so that others could use it, so the theory says that all you have to do is feed in an initial value. If we need workarounds then I accept that these are necessary at times, but cases like this where the work is already done then there is no need for this.
If you think it's trivial to do then please come up with a patch or provide some detailed hints on what needs to be done because I was not able to find it. Where should I feed an inital value? The ob_pci_init() function has no parameter and adding one would require changing all callers that I can't test so I'd rather not do.
I've tried adding a bus_num field to arch struct and set bus from that in ob_pci_init() but then OpenBIOS hangs on startup.
That feels like the right approach, but there certainly shouldn't cause any change in behaviour. Can you push your code somewhere so I can take a look?
Also re-reading the patch I'm not sure that {0, 1} is correct under QEMU compared with a real Mac, since there is no PCI bridge present under /pci@f2000000?
I've also tried setting it to {1, 0} instead but same hang as above so I think OpenBIOS cannot handle anything but bus 0 so I gave up. I'm not going to try to debug this as my workaround works well and all this would be unnecessary when passing device tree from QEMU is implemented as suggested before (that would be needed to support Pegasos2 without too many new hacks and workarounds in OpenBIOS and clean up some of the current mess so would be needed eventually if we want to avoid having yet another open firmware implementation in QEMU). But until then having a workaround to be able to run MorphOS without extra effort would have been nice unless it breaks somthing else.
So just to clarify you're saying that with your v5 patch if you simply switch {0, 1} to {1,0} then that also hangs? Also as discussed before it's not that I'm completely against an FDT-based setup, but it's an awful lot of work... ATB, Mark.
On Thu, 4 Jul 2019, Mark Cave-Ayland wrote:
On 02/07/2019 12:28, BALATON Zoltan wrote:
On Mon, 1 Jul 2019, BALATON Zoltan wrote:
On Mon, 1 Jul 2019, Mark Cave-Ayland wrote:
Errr... I did spend time making the OpenBIOS bridge code generic so that others could use it, so the theory says that all you have to do is feed in an initial value. If we need workarounds then I accept that these are necessary at times, but cases like this where the work is already done then there is no need for this.
If you think it's trivial to do then please come up with a patch or provide some detailed hints on what needs to be done because I was not able to find it. Where should I feed an inital value? The ob_pci_init() function has no parameter and adding one would require changing all callers that I can't test so I'd rather not do.
I've tried adding a bus_num field to arch struct and set bus from that in ob_pci_init() but then OpenBIOS hangs on startup.
That feels like the right approach, but there certainly shouldn't cause any change in behaviour. Can you push your code somewhere so I can take a look?
I've already reverted that patch and no time to reimplement it but I was adding a uint8_t bus_num field to struct pci_arch_t in include/drivers/pci.h then in drivers/pci.c replaced bus = 0 with bus = arch->bus_num and in arch/ppc/qemu/init.c added .bus_num = 1 to [ARCH_MAC99] in the init of known_arch[].
Also re-reading the patch I'm not sure that {0, 1} is correct under QEMU compared with a real Mac, since there is no PCI bridge present under /pci@f2000000?
I've also tried setting it to {1, 0} instead but same hang as above so I think OpenBIOS cannot handle anything but bus 0 so I gave up. I'm not going to try to debug this as my workaround works well and all this would be unnecessary when passing device tree from QEMU is implemented as suggested before (that would be needed to support Pegasos2 without too many new hacks and workarounds in OpenBIOS and clean up some of the current mess so would be needed eventually if we want to avoid having yet another open firmware implementation in QEMU). But until then having a workaround to be able to run MorphOS without extra effort would have been nice unless it breaks somthing else.
So just to clarify you're saying that with your v5 patch if you simply switch {0, 1} to {1,0} then that also hangs?
Yes, that's what I meant. Have you tried it? Even without the above bus_num patch, just trying to use different value than {0, 1} with my v5 patch it hangs the same way as with above. Since it hangs before init-ing serial I could not get debug logs either so I did not try to find out what's wrong but my guess would be that bus != 0 might cause problem somewhere.
Also as discussed before it's not that I'm completely against an FDT-based setup, but it's an awful lot of work...
It's some work but maybe we can reuse code from SLOF so it's not that much work and it would allow to get rid of all the FW_CFG based hacks and hardcoded values we now have in OpenBIOS (these could be set by the machine models in QEMU where it's simpler to do because we don't need to handle all machines only the one it models) and this would allow making fixes in QEMU in the future without needing to also update OpenBIOS every time so it would make code cleaner and more stable. I've planned to try it eventually to see if OpenBIOS could be used for the Pegasos2 emulation but I haven't got to it yet. Regards, BALATON Zoltan
participants (2)
-
BALATON Zoltan -
Mark Cave-Ayland