Hello,
Is there a way to add a new device in the middle of the device tree (before or after an existing node) from Forth? If I try:
dev / new-device " newdev" device-name finish-device show-devs
/newdev appears at the end of the device-tree but that's not what I want. I need this device to go at a specific place in the middle but I could not find out how to achieve that. Any ideas?
Thank you, BALATON Zoltan
On 2017-May-27 16:59 , BALATON Zoltan wrote:
Is there a way to add a new device in the middle of the device tree (before or after an existing node) from Forth? If I try:
dev / new-device " newdev" device-name finish-device show-devs
/newdev appears at the end of the device-tree but that's not what I want. I need this device to go at a specific place in the middle but I could not find out how to achieve that. Any ideas?
Nope. That's the way it generally works, the node is added at the beginning or end of a linked list for that level. You'd have to fiddle with private structures to change the order.
But why do you care where it appears in the tree? You should always be finding the node by name and/or unit address, so it shouldn't matter what order things are in.
By the way, there is something wrong with your email server. When trying to send you email, your email server at zero.eik.bme.hu complained:
* balaton@eik.bme.hu: host zero.eik.bme.hu[2001:738:2001:2001::2001] said: 550 * 5.7.25 Client host rejected: cannot find your hostname, * [2607:f440::d144:5c9] (in reply to RCPT TO command)
I'm pretty sure there is nothing fishy about my email server (hiwela.pair.com [209.68.5.201]), which suggests that your email server is taking an over-aggressive approach to rejecting email. Not sure why it ended up doing IPv6 addresses either, your MX record says the connection should go to 152.66.115.2 .
On Sat, 27 May 2017, Tarl Neustaedter wrote:
On 2017-May-27 16:59 , BALATON Zoltan wrote:
Is there a way to add a new device in the middle of the device tree (before or after an existing node) from Forth? If I try:
dev / new-device " newdev" device-name finish-device show-devs
/newdev appears at the end of the device-tree but that's not what I want. I need this device to go at a specific place in the middle but I could not find out how to achieve that. Any ideas?
Nope. That's the way it generally works, the node is added at the beginning or end of a linked list for that level. You'd have to fiddle with private structures to change the order.
If there's a way to add it to the beginning instead of the end that may work as well. How to do that? Or I was thinking about copying the exising device tree one by one and insert the new device to the copy at the right place, then replacing the device tree with the new one but I'm not sure how to do that. Could you give some hints?
But why do you care where it appears in the tree? You should always be finding the node by name and/or unit address, so it shouldn't matter what order things are in.
I'm trying to do this so I don't need the patch to OpenBIOS:
https://mail.coreboot.org/pipermail/openbios/2017-January/009867.html
Unfortunately the code relying on this (which I can't change) seems to have some assumptions about order of pci busses in the device tree and only works if added before the exising entry (i.e. /pci@f0000000 has to come before /pci@f2000000).
Regards, BALATON Zoltan
On Sun, May 28, 2017 at 12:38:47PM +0200, BALATON Zoltan wrote:
On Sat, 27 May 2017, Tarl Neustaedter wrote:
On 2017-May-27 16:59 , BALATON Zoltan wrote:
Is there a way to add a new device in the middle of the device tree (before or after an existing node) from Forth? If I try:
dev / new-device " newdev" device-name finish-device show-devs
/newdev appears at the end of the device-tree but that's not what I want. I need this device to go at a specific place in the middle but I could not find out how to achieve that. Any ideas?
Nope. That's the way it generally works, the node is added at the beginning or end of a linked list for that level. You'd have to fiddle with private structures to change the order.
If there's a way to add it to the beginning instead of the end that may work as well. How to do that? Or I was thinking about copying the exising device tree one by one and insert the new device to the copy at the right place, then replacing the device tree with the new one but I'm not sure how to do that. Could you give some hints?
There is no portable way to do it, and this doesn't matter at all; anything that relies on the order of devices in the device tree is broken.
If you want this merely for aesthetic purposes, just arrange for all device nodes to be created in the order you want them to appear in?
But why do you care where it appears in the tree? You should always be finding the node by name and/or unit address, so it shouldn't matter what order things are in.
I'm trying to do this so I don't need the patch to OpenBIOS:
https://mail.coreboot.org/pipermail/openbios/2017-January/009867.html
Unfortunately the code relying on this (which I can't change) seems to have some assumptions about order of pci busses in the device tree and only works if added before the exising entry (i.e. /pci@f0000000 has to come before /pci@f2000000).
What is that code you cannot change? Sounds like OS9?
Segher
On 28/05/17 11:38, BALATON Zoltan wrote:
On Sat, 27 May 2017, Tarl Neustaedter wrote:
On 2017-May-27 16:59 , BALATON Zoltan wrote:
Is there a way to add a new device in the middle of the device tree (before or after an existing node) from Forth? If I try:
dev / new-device " newdev" device-name finish-device show-devs
/newdev appears at the end of the device-tree but that's not what I want. I need this device to go at a specific place in the middle but I could not find out how to achieve that. Any ideas?
Nope. That's the way it generally works, the node is added at the beginning or end of a linked list for that level. You'd have to fiddle with private structures to change the order.
If there's a way to add it to the beginning instead of the end that may work as well. How to do that? Or I was thinking about copying the exising device tree one by one and insert the new device to the copy at the right place, then replacing the device tree with the new one but I'm not sure how to do that. Could you give some hints?
But why do you care where it appears in the tree? You should always be finding the node by name and/or unit address, so it shouldn't matter what order things are in.
I'm trying to do this so I don't need the patch to OpenBIOS:
https://mail.coreboot.org/pipermail/openbios/2017-January/009867.html
Unfortunately the code relying on this (which I can't change) seems to have some assumptions about order of pci busses in the device tree and only works if added before the exising entry (i.e. /pci@f0000000 has to come before /pci@f2000000).
Would it still not be easier just to update OpenBIOS so that the PCI host bridge structure becomes and array, add the bus number, and then add an empty bus to QEMU?
Just refreshing my memory, in the message above you mention that the bus number is wrong - this is because it is currently hardcoded to 0 in OpenBIOS. I do have a local fix for this based upon some work I'm doing to add support for PCI bridges, so is there any reason why combining these things together shouldn't work?
ATB,
Mark.
On Wed, 31 May 2017, Mark Cave-Ayland wrote:
On 28/05/17 11:38, BALATON Zoltan wrote:
On Sat, 27 May 2017, Tarl Neustaedter wrote:
On 2017-May-27 16:59 , BALATON Zoltan wrote:
Is there a way to add a new device in the middle of the device tree (before or after an existing node) from Forth? If I try:
dev / new-device " newdev" device-name finish-device show-devs
/newdev appears at the end of the device-tree but that's not what I want. I need this device to go at a specific place in the middle but I could not find out how to achieve that. Any ideas?
Nope. That's the way it generally works, the node is added at the beginning or end of a linked list for that level. You'd have to fiddle with private structures to change the order.
If there's a way to add it to the beginning instead of the end that may work as well. How to do that? Or I was thinking about copying the exising device tree one by one and insert the new device to the copy at the right place, then replacing the device tree with the new one but I'm not sure how to do that. Could you give some hints?
But why do you care where it appears in the tree? You should always be finding the node by name and/or unit address, so it shouldn't matter what order things are in.
I'm trying to do this so I don't need the patch to OpenBIOS:
https://mail.coreboot.org/pipermail/openbios/2017-January/009867.html
Unfortunately the code relying on this (which I can't change) seems to have some assumptions about order of pci busses in the device tree and only works if added before the exising entry (i.e. /pci@f0000000 has to come before /pci@f2000000).
Would it still not be easier just to update OpenBIOS so that the PCI host bridge structure becomes and array, add the bus number, and then add an empty bus to QEMU?
Just refreshing my memory, in the message above you mention that the bus number is wrong - this is because it is currently hardcoded to 0 in OpenBIOS. I do have a local fix for this based upon some work I'm doing to add support for PCI bridges, so is there any reason why combining these things together shouldn't work?
The only reason is that I don't know how to do that. All I know is to make USB work correctly we need to add a device-tree node for the /pci@f0000000 bus (which can be empty but has to exist at least minimally) and this has to come before the existing /pci@f2000000 (so adding it by patching the device tree does not work unless we can put this node at the beginning which I could not figure out how to do in Forth). How this node is added is not important and I also don't know if the incorrect bus number causes any problem other than some warnings.
Where is the host bridge structure that you refer to above and should be turned into an array? I think at least ob_pci_init needs to be changed but I have no idea how so I'd need some more guidance.
Regards, BALATON Zoltan
On 03/06/17 14:09, BALATON Zoltan wrote:
The only reason is that I don't know how to do that. All I know is to make USB work correctly we need to add a device-tree node for the /pci@f0000000 bus (which can be empty but has to exist at least minimally) and this has to come before the existing /pci@f2000000 (so adding it by patching the device tree does not work unless we can put this node at the beginning which I could not figure out how to do in Forth). How this node is added is not important and I also don't know if the incorrect bus number causes any problem other than some warnings.
Where is the host bridge structure that you refer to above and should be turned into an array? I think at least ob_pci_init needs to be changed but I have no idea how so I'd need some more guidance.
It's the pci_arch_t structure used for each architecture as defined in include/drivers/pci.h. What you need to do is split out the PCI-specific fields and alter the struct to hold an array of these fields, and then update ob_pci_init() to optionally initialise more than one PCI host bridge.
ATB,
Mark.
On 28/05/17 11:38, BALATON Zoltan wrote:
On Sat, 27 May 2017, Tarl Neustaedter wrote:
On 2017-May-27 16:59 , BALATON Zoltan wrote:
Is there a way to add a new device in the middle of the device tree (before or after an existing node) from Forth? If I try:
dev / new-device " newdev" device-name finish-device show-devs
/newdev appears at the end of the device-tree but that's not what I want. I need this device to go at a specific place in the middle but I could not find out how to achieve that. Any ideas?
Nope. That's the way it generally works, the node is added at the beginning or end of a linked list for that level. You'd have to fiddle with private structures to change the order.
If there's a way to add it to the beginning instead of the end that may work as well. How to do that? Or I was thinking about copying the exising device tree one by one and insert the new device to the copy at the right place, then replacing the device tree with the new one but I'm not sure how to do that. Could you give some hints?
But why do you care where it appears in the tree? You should always be finding the node by name and/or unit address, so it shouldn't matter what order things are in.
I'm trying to do this so I don't need the patch to OpenBIOS:
https://mail.coreboot.org/pipermail/openbios/2017-January/009867.html
Unfortunately the code relying on this (which I can't change) seems to have some assumptions about order of pci busses in the device tree and only works if added before the exising entry (i.e. /pci@f0000000 has to come before /pci@f2000000).
FWIW I've just been testing OpenBIOS git master ready for upstreaming to QEMU and noticed the the recent changes to the PCI host bridge properties have fixed PCI bus enumeration on FreeBSD PPC under -M mac99 (it still panics, but hey at least it finally sees a proper set of devices).
If you get a moment, can you try again with git master and report back whether things have improved for you or not?
ATB,
Mark.
On Thu, 13 Jul 2017, Mark Cave-Ayland wrote:
On 28/05/17 11:38, BALATON Zoltan wrote:
I'm trying to do this so I don't need the patch to OpenBIOS:
https://mail.coreboot.org/pipermail/openbios/2017-January/009867.html
Unfortunately the code relying on this (which I can't change) seems to have some assumptions about order of pci busses in the device tree and only works if added before the exising entry (i.e. /pci@f0000000 has to come before /pci@f2000000).
FWIW I've just been testing OpenBIOS git master ready for upstreaming to QEMU and noticed the the recent changes to the PCI host bridge properties have fixed PCI bus enumeration on FreeBSD PPC under -M mac99 (it still panics, but hey at least it finally sees a proper set of devices).
If you get a moment, can you try again with git master and report back whether things have improved for you or not?
It works as good as before: that is I still need the above patch but your changes haven't seem to do any harm nor fix anything for what I have tested.
I'm still not sure how to add another PCI bus to MAC99 because the assumption of a single PCI bus seems to be in the OpenBIOS pci driver. I've noticed some possible redundancy though in the pci arch definition:
[ARCH_MAC99] = { .name = "MAC99", .vendor_id = PCI_VENDOR_ID_APPLE, .device_id = PCI_DEVICE_ID_APPLE_UNI_N_PCI, .cfg_addr = 0xf2800000, .cfg_data = 0xf2c00000, .cfg_base = 0xf2000000, .cfg_len = 0x02000000, .host_pci_base = 0x0, .pci_mem_base = 0x80000000, .mem_len = 0x10000000, .io_base = 0xf2000000, .io_len = 0x00800000, .host_ranges = { { .type = IO_SPACE, .parentaddr = 0, .childaddr = 0xf2000000, .len = 0x00800000 }, { .type = MEMORY_SPACE_32, .parentaddr = 0x80000000, .childaddr = 0x80000000, .len = 0x10000000 }, { .type = 0, .parentaddr = 0, .childaddr = 0, .len = 0 } }, .irqs = { 0x1b, 0x1c, 0x1d, 0x1e } },
Aren't pci_mem_base/mem_len and io_base/io_len the same as the IO and MEMORY ranges in host_ranges (and the cfg_* values offsets into the IO space)? Do we need these separately or could it be simplified?
To add another PCI bus I think this arch structure needs to be extended to be able to define ids and ranges for multiple buses but I'm not sure how to do that without breaking other archs. Do you have any idea how to go about this? (Also does any other arch need this? If not is it worth doing it when the other PCI bus on QEMU is empty with no devices and we only need it to be present in the device tree? So could we just go with the above hack until handling multiple PCI buses are implemented eventually?)
Regards, BALATON Zoltan
On 15/07/17 21:27, BALATON Zoltan wrote:
On Thu, 13 Jul 2017, Mark Cave-Ayland wrote:
On 28/05/17 11:38, BALATON Zoltan wrote:
I'm trying to do this so I don't need the patch to OpenBIOS:
https://mail.coreboot.org/pipermail/openbios/2017-January/009867.html
Unfortunately the code relying on this (which I can't change) seems to have some assumptions about order of pci busses in the device tree and only works if added before the exising entry (i.e. /pci@f0000000 has to come before /pci@f2000000).
FWIW I've just been testing OpenBIOS git master ready for upstreaming to QEMU and noticed the the recent changes to the PCI host bridge properties have fixed PCI bus enumeration on FreeBSD PPC under -M mac99 (it still panics, but hey at least it finally sees a proper set of devices).
If you get a moment, can you try again with git master and report back whether things have improved for you or not?
It works as good as before: that is I still need the above patch but your changes haven't seem to do any harm nor fix anything for what I have tested.
I'm still not sure how to add another PCI bus to MAC99 because the assumption of a single PCI bus seems to be in the OpenBIOS pci driver. I've noticed some possible redundancy though in the pci arch definition:
[ARCH_MAC99] = { .name = "MAC99", .vendor_id = PCI_VENDOR_ID_APPLE, .device_id = PCI_DEVICE_ID_APPLE_UNI_N_PCI, .cfg_addr = 0xf2800000, .cfg_data = 0xf2c00000, .cfg_base = 0xf2000000, .cfg_len = 0x02000000, .host_pci_base = 0x0, .pci_mem_base = 0x80000000, .mem_len = 0x10000000, .io_base = 0xf2000000, .io_len = 0x00800000, .host_ranges = { { .type = IO_SPACE, .parentaddr = 0, .childaddr = 0xf2000000, .len = 0x00800000 }, { .type = MEMORY_SPACE_32, .parentaddr = 0x80000000, .childaddr = 0x80000000, .len = 0x10000000 }, { .type = 0, .parentaddr = 0, .childaddr = 0, .len = 0 } }, .irqs = { 0x1b, 0x1c, 0x1d, 0x1e } },
Aren't pci_mem_base/mem_len and io_base/io_len the same as the IO and MEMORY ranges in host_ranges (and the cfg_* values offsets into the IO space)? Do we need these separately or could it be simplified?
They are for PPC, but unfortunately not for SPARC64. That's why they were split out as separate values in the last set of patches because the existing logic couldn't handle all cases of multiple address spaces with different offsets :(
To add another PCI bus I think this arch structure needs to be extended to be able to define ids and ranges for multiple buses but I'm not sure how to do that without breaking other archs. Do you have any idea how to go about this? (Also does any other arch need this? If not is it worth doing it when the other PCI bus on QEMU is empty with no devices and we only need it to be present in the device tree? So could we just go with the above hack until handling multiple PCI buses are implemented eventually?)
As a starting point, why not make the above struct an array of host bridges, e.g.
[ARCH_MAC99] = [ { .name = "MAC99", .vendor_id = PCI_VENDOR_ID_APPLE, .device_id = PCI_DEVICE_ID_APPLE_UNI_N_PCI, .cfg_addr = 0xf2800000, .cfg_data = 0xf2c00000, .cfg_base = 0xf2000000, .cfg_len = 0x02000000, .host_pci_base = 0x0, .pci_mem_base = 0x80000000, .mem_len = 0x10000000, .io_base = 0xf2000000, .io_len = 0x00800000, .host_ranges = { { .type = IO_SPACE, .parentaddr = 0, .childaddr = 0xf2000000, .len = 0x00800000 }, { .type = MEMORY_SPACE_32, .parentaddr = 0x80000000, .childaddr = 0x80000000, .len = 0x10000000 }, { .type = 0, .parentaddr = 0, .childaddr = 0, .len = 0 } }, .irqs = { 0x1b, 0x1c, 0x1d, 0x1e } }, { .... } ] },
Then remove the "break" from ob_pci_init() and see how you get on. You'll need to figure out what to do with the configuration space, however with the last set of fix-ups I really don't think there is much to do.
Also a good tip: get it working on PPC first and then make the changes to the other architectures and test. I don't mind helping out with this part if you get stuck.
ATB,
Mark.
On Sun, 16 Jul 2017, Mark Cave-Ayland wrote:
On 15/07/17 21:27, BALATON Zoltan wrote:
I've noticed some possible redundancy though in the pci arch definition:
[ARCH_MAC99] = { .name = "MAC99", .vendor_id = PCI_VENDOR_ID_APPLE, .device_id = PCI_DEVICE_ID_APPLE_UNI_N_PCI, .cfg_addr = 0xf2800000, .cfg_data = 0xf2c00000, .cfg_base = 0xf2000000, .cfg_len = 0x02000000, .host_pci_base = 0x0, .pci_mem_base = 0x80000000, .mem_len = 0x10000000, .io_base = 0xf2000000, .io_len = 0x00800000, .host_ranges = { { .type = IO_SPACE, .parentaddr = 0, .childaddr = 0xf2000000, .len = 0x00800000 }, { .type = MEMORY_SPACE_32, .parentaddr = 0x80000000, .childaddr = 0x80000000, .len = 0x10000000 }, { .type = 0, .parentaddr = 0, .childaddr = 0, .len = 0 } }, .irqs = { 0x1b, 0x1c, 0x1d, 0x1e } },
Aren't pci_mem_base/mem_len and io_base/io_len the same as the IO and MEMORY ranges in host_ranges (and the cfg_* values offsets into the IO space)? Do we need these separately or could it be simplified?
They are for PPC, but unfortunately not for SPARC64. That's why they were split out as separate values in the last set of patches because the existing logic couldn't handle all cases of multiple address spaces with different offsets :(
I don't see why SPARC64 is not also redundant. Here's its pci arch struct:
.pci = { .name = "SUNW,sabre", .vendor_id = PCI_VENDOR_ID_SUN, .device_id = PCI_DEVICE_ID_SUN_SABRE, .cfg_addr = APB_SPECIAL_BASE + 0x1000000ULL, // PCI bus configuration space .cfg_data = APB_MEM_BASE, // PCI bus memory space .cfg_base = APB_SPECIAL_BASE, .cfg_len = 0x1000000, .host_pci_base = APB_MEM_BASE, .pci_mem_base = 0x100000, /* avoid VGA at 0xa0000 */ .mem_len = 0xf0000000, .io_base = APB_SPECIAL_BASE + 0x2000000ULL, // PCI Bus I/O space .io_len = 0x1000000, .host_ranges = { { .type = CONFIGURATION_SPACE, .parentaddr = 0, .childaddr = APB_SPECIAL_BASE + 0x1000000ULL, .len = 0x1000000 }, { .type = IO_SPACE, .parentaddr = 0, .childaddr = APB_SPECIAL_BASE + 0x2000000ULL, .len = 0x1000000 }, { .type = MEMORY_SPACE_32, .parentaddr = 0, .childaddr = APB_MEM_BASE, .len = 0xf0000000 }, { .type = 0, .parentaddr = 0, .childaddr = 0, .len = 0 } }, .irqs = { 0, 1, 2, 3 }, },
The addresses and lengths here are also mostly the same as in host_ranges. Where does the difference come from and could we get rid of the individual *_base, *_addr and *_len fields and define everything in the host_ranges structure? (Maybe we need some additional offsets in the config space but that's all we should need I think.)
It looks like that now the host_ranges are used to create the ranges property while the other addresses and lengths are used to create the mapping but these two should match (because the ranges property is supposed to describe these mappings) so one should be computable from the other otherwise we may have a bug (or I'm missing something which is also likely as I know nothing about this). Therefore I think it should be possible to get rid of one of these. What am I missing?
As a starting point, why not make the above struct an array of host bridges, e.g.
[ARCH_MAC99] = [ { }, { .... } ] },
Then remove the "break" from ob_pci_init() and see how you get on. You'll need to figure out what to do with the configuration space, however with the last set of fix-ups I really don't think there is much to do.
Other than understanding a lot of low level pci stuff, how it all worked in OpenBIOS before and how it works now after your changes and also the pci structure of all architectures... I could not get this in the short time I've spent looking at this so I gave up because to me it seems like a lot to do at the moment.
Regards, BALATON Zoltan
On 16/07/17 14:56, BALATON Zoltan wrote:
They are for PPC, but unfortunately not for SPARC64. That's why they were split out as separate values in the last set of patches because the existing logic couldn't handle all cases of multiple address spaces with different offsets :(
I don't see why SPARC64 is not also redundant. Here's its pci arch struct:
.pci = { .name = "SUNW,sabre", .vendor_id = PCI_VENDOR_ID_SUN, .device_id = PCI_DEVICE_ID_SUN_SABRE, .cfg_addr = APB_SPECIAL_BASE + 0x1000000ULL, // PCI bus configuration space .cfg_data = APB_MEM_BASE, // PCI bus memory space .cfg_base = APB_SPECIAL_BASE, .cfg_len = 0x1000000, .host_pci_base = APB_MEM_BASE, .pci_mem_base = 0x100000, /* avoid VGA at 0xa0000 */ .mem_len = 0xf0000000, .io_base = APB_SPECIAL_BASE + 0x2000000ULL, // PCI Bus I/O space .io_len = 0x1000000, .host_ranges = { { .type = CONFIGURATION_SPACE, .parentaddr = 0, .childaddr = APB_SPECIAL_BASE + 0x1000000ULL, .len = 0x1000000 }, { .type = IO_SPACE, .parentaddr = 0, .childaddr = APB_SPECIAL_BASE + 0x2000000ULL, .len = 0x1000000 }, { .type = MEMORY_SPACE_32, .parentaddr = 0, .childaddr = APB_MEM_BASE, .len = 0xf0000000 }, { .type = 0, .parentaddr = 0, .childaddr = 0, .len = 0 } }, .irqs = { 0, 1, 2, 3 }, },
The addresses and lengths here are also mostly the same as in host_ranges. Where does the difference come from and could we get rid of the individual *_base, *_addr and *_len fields and define everything in the host_ranges structure? (Maybe we need some additional offsets in the config space but that's all we should need I think.)
It looks like that now the host_ranges are used to create the ranges property while the other addresses and lengths are used to create the mapping but these two should match (because the ranges property is supposed to describe these mappings) so one should be computable from the other otherwise we may have a bug (or I'm missing something which is also likely as I know nothing about this). Therefore I think it should be possible to get rid of one of these. What am I missing?
For SPARC64 the issue is with pci_mem_base and how it is interpreted in Linux. Before the recent set of patches, the .childaddr for MEMORY_SPACE_32 was generated as APB_MEM_BASE + pci_mem_base, so in effect all we do is miss mapping the first PCI memory section.
However in Linux the kernel calculates the address of the host range is by taking the value of an undocumented register in the Simba host bridge (see simba_config_cb() for details) and then adding the offsets on top meaning that you end up with all your addresses out by pci_mem_base.
Hence the need to apply pci_mem_base as the starting value for assigning devices to PCI memory space, but not including it in the host ranges itself.
As a starting point, why not make the above struct an array of host bridges, e.g.
[ARCH_MAC99] = [ { }, { .... } ] },
Then remove the "break" from ob_pci_init() and see how you get on. You'll need to figure out what to do with the configuration space, however with the last set of fix-ups I really don't think there is much to do.
Other than understanding a lot of low level pci stuff, how it all worked in OpenBIOS before and how it works now after your changes and also the pci structure of all architectures... I could not get this in the short time I've spent looking at this so I gave up because to me it seems like a lot to do at the moment.
I'd say you don't need a deep understanding of PCI to at least rework the pci_arch_t into an array of structs representing each host bridge to see what happens, but as I mentioned above I believe the bulk of the support should now be there.
Unfortunately I don't have much in the way of time to look at this now, but I'm happy to point you in the right direction and reply to any questions you might have. At least that's probably an easier option than sitting down trying to read the PCI specification ;)
ATB,
Mark.
On Sun, 16 Jul 2017, Mark Cave-Ayland wrote:
On 16/07/17 14:56, BALATON Zoltan wrote:
They are for PPC, but unfortunately not for SPARC64. That's why they were split out as separate values in the last set of patches because the existing logic couldn't handle all cases of multiple address spaces with different offsets :(
I don't see why SPARC64 is not also redundant. Here's its pci arch struct:
.pci = { .name = "SUNW,sabre", .vendor_id = PCI_VENDOR_ID_SUN, .device_id = PCI_DEVICE_ID_SUN_SABRE, .cfg_addr = APB_SPECIAL_BASE + 0x1000000ULL, // PCI bus configuration space .cfg_data = APB_MEM_BASE, // PCI bus memory space .cfg_base = APB_SPECIAL_BASE, .cfg_len = 0x1000000, .host_pci_base = APB_MEM_BASE, .pci_mem_base = 0x100000, /* avoid VGA at 0xa0000 */ .mem_len = 0xf0000000, .io_base = APB_SPECIAL_BASE + 0x2000000ULL, // PCI Bus I/O space .io_len = 0x1000000, .host_ranges = { { .type = CONFIGURATION_SPACE, .parentaddr = 0, .childaddr = APB_SPECIAL_BASE + 0x1000000ULL, .len = 0x1000000 }, { .type = IO_SPACE, .parentaddr = 0, .childaddr = APB_SPECIAL_BASE + 0x2000000ULL, .len = 0x1000000 }, { .type = MEMORY_SPACE_32, .parentaddr = 0, .childaddr = APB_MEM_BASE, .len = 0xf0000000 }, { .type = 0, .parentaddr = 0, .childaddr = 0, .len = 0 } }, .irqs = { 0, 1, 2, 3 }, },
The addresses and lengths here are also mostly the same as in host_ranges. Where does the difference come from and could we get rid of the individual *_base, *_addr and *_len fields and define everything in the host_ranges structure? (Maybe we need some additional offsets in the config space but that's all we should need I think.)
It looks like that now the host_ranges are used to create the ranges property while the other addresses and lengths are used to create the mapping but these two should match (because the ranges property is supposed to describe these mappings) so one should be computable from the other otherwise we may have a bug (or I'm missing something which is also likely as I know nothing about this). Therefore I think it should be possible to get rid of one of these. What am I missing?
For SPARC64 the issue is with pci_mem_base and how it is interpreted in Linux. Before the recent set of patches, the .childaddr for MEMORY_SPACE_32 was generated as APB_MEM_BASE + pci_mem_base, so in effect all we do is miss mapping the first PCI memory section.
However in Linux the kernel calculates the address of the host range is by taking the value of an undocumented register in the Simba host bridge (see simba_config_cb() for details) and then adding the offsets on top meaning that you end up with all your addresses out by pci_mem_base.
Hence the need to apply pci_mem_base as the starting value for assigning devices to PCI memory space, but not including it in the host ranges itself.
So does that mean we could theoretically get rid of all the other redundant data and put this pci_mem_base offset for SPARC64 in some #ifdefed code part (or in the undocumented register to counter the offset in Linux) to simplify the current situation where the ranges are defined in two different ways?
I was also thinking if our problems come from trying to make a single driver to manage all the different pci hosts? Are these similar enough for this to work or should it be really split into at least three files: generic pci stuff, Apple host bus and SPARC host bus? Would that make it clearer? There seems to be a lot of #ifdefed code in the pci driver now which suggests maybe these should be really separate drivers and only keep the generic stuff in pci.c.
As a starting point, why not make the above struct an array of host bridges, e.g.
[ARCH_MAC99] = [ { }, { .... } ] },
Then remove the "break" from ob_pci_init() and see how you get on. You'll need to figure out what to do with the configuration space, however with the last set of fix-ups I really don't think there is much to do.
Other than understanding a lot of low level pci stuff, how it all worked in OpenBIOS before and how it works now after your changes and also the pci structure of all architectures... I could not get this in the short time I've spent looking at this so I gave up because to me it seems like a lot to do at the moment.
I'd say you don't need a deep understanding of PCI to at least rework the pci_arch_t into an array of structs representing each host bridge to see what happens, but as I mentioned above I believe the bulk of the support should now be there.
Unfortunately I don't have much in the way of time to look at this now, but I'm happy to point you in the right direction and reply to any questions you might have. At least that's probably an easier option than sitting down trying to read the PCI specification ;)
Same problem here with time and I don't feel like wanting to debug this without understanding how it should work so I pass this for now. If I make any progress I'll let you know. (I think I should at least understand the mappings and address translations and their representation in OpenBIOS to know what am I doing but I don't have that knowledge.)
Regards, BALATON Zoltan
On 16/07/17 16:49, BALATON Zoltan wrote:
On Sun, 16 Jul 2017, Mark Cave-Ayland wrote:
On 16/07/17 14:56, BALATON Zoltan wrote:
They are for PPC, but unfortunately not for SPARC64. That's why they were split out as separate values in the last set of patches because the existing logic couldn't handle all cases of multiple address spaces with different offsets :(
I don't see why SPARC64 is not also redundant. Here's its pci arch struct:
.pci = { .name = "SUNW,sabre", .vendor_id = PCI_VENDOR_ID_SUN, .device_id = PCI_DEVICE_ID_SUN_SABRE, .cfg_addr = APB_SPECIAL_BASE + 0x1000000ULL, // PCI bus configuration space .cfg_data = APB_MEM_BASE, // PCI bus memory space .cfg_base = APB_SPECIAL_BASE, .cfg_len = 0x1000000, .host_pci_base = APB_MEM_BASE, .pci_mem_base = 0x100000, /* avoid VGA at 0xa0000 */ .mem_len = 0xf0000000, .io_base = APB_SPECIAL_BASE + 0x2000000ULL, // PCI Bus I/O space .io_len = 0x1000000, .host_ranges = { { .type = CONFIGURATION_SPACE, .parentaddr = 0, .childaddr = APB_SPECIAL_BASE + 0x1000000ULL, .len = 0x1000000 }, { .type = IO_SPACE, .parentaddr = 0, .childaddr = APB_SPECIAL_BASE + 0x2000000ULL, .len = 0x1000000 }, { .type = MEMORY_SPACE_32, .parentaddr = 0, .childaddr = APB_MEM_BASE, .len = 0xf0000000 }, { .type = 0, .parentaddr = 0, .childaddr = 0, .len = 0 } }, .irqs = { 0, 1, 2, 3 }, },
The addresses and lengths here are also mostly the same as in host_ranges. Where does the difference come from and could we get rid of the individual *_base, *_addr and *_len fields and define everything in the host_ranges structure? (Maybe we need some additional offsets in the config space but that's all we should need I think.)
It looks like that now the host_ranges are used to create the ranges property while the other addresses and lengths are used to create the mapping but these two should match (because the ranges property is supposed to describe these mappings) so one should be computable from the other otherwise we may have a bug (or I'm missing something which is also likely as I know nothing about this). Therefore I think it should be possible to get rid of one of these. What am I missing?
For SPARC64 the issue is with pci_mem_base and how it is interpreted in Linux. Before the recent set of patches, the .childaddr for MEMORY_SPACE_32 was generated as APB_MEM_BASE + pci_mem_base, so in effect all we do is miss mapping the first PCI memory section.
However in Linux the kernel calculates the address of the host range is by taking the value of an undocumented register in the Simba host bridge (see simba_config_cb() for details) and then adding the offsets on top meaning that you end up with all your addresses out by pci_mem_base.
Hence the need to apply pci_mem_base as the starting value for assigning devices to PCI memory space, but not including it in the host ranges itself.
So does that mean we could theoretically get rid of all the other redundant data and put this pci_mem_base offset for SPARC64 in some #ifdefed code part (or in the undocumented register to counter the offset in Linux) to simplify the current situation where the ranges are defined in two different ways?
There was some code for config space registers already #ifdef'd for SPARC64 but the issue there was that with so many conditionals used for building up the PCI host bridge ranges that it was very difficult to see at a glance what the resulting values were.
Bear in mind that PPC also had its own hacks in that it has multiple IO memory spaces listed for the host bridge, so rather than keep going down this maze of conditional logic and #ifdef, I decided to explicitly list the values themselves which solves all the problems in one go.
If you wanted to better document the relationships between the host ranges properties and the corresponding values in pci_arch_t then you could use some #defines, but then if you're working down at that level regardless then I suspect this will be the least of your problems.
I was also thinking if our problems come from trying to make a single driver to manage all the different pci hosts? Are these similar enough for this to work or should it be really split into at least three files: generic pci stuff, Apple host bus and SPARC host bus? Would that make it clearer? There seems to be a lot of #ifdefed code in the pci driver now which suggests maybe these should be really separate drivers and only keep the generic stuff in pci.c.
I think that's part of it, but the specification is lacking in some areas. For example I couldn't figure out from the specs I had whether configuration space should be included in the PCI host bridge ranges - SPARC64 requires it, whereas PPC panics several (Apple) OSs if they are included.
Yes you could argue it would be better to split things into separate files but that's quite far down the priority list at the moment. If people were constantly adding new architectures to OpenBIOS I could see the benefit, but I don't see that ever changing.
As a starting point, why not make the above struct an array of host bridges, e.g.
[ARCH_MAC99] = [ { }, { .... } ] },
Then remove the "break" from ob_pci_init() and see how you get on. You'll need to figure out what to do with the configuration space, however with the last set of fix-ups I really don't think there is much to do.
Other than understanding a lot of low level pci stuff, how it all worked in OpenBIOS before and how it works now after your changes and also the pci structure of all architectures... I could not get this in the short time I've spent looking at this so I gave up because to me it seems like a lot to do at the moment.
I'd say you don't need a deep understanding of PCI to at least rework the pci_arch_t into an array of structs representing each host bridge to see what happens, but as I mentioned above I believe the bulk of the support should now be there.
Unfortunately I don't have much in the way of time to look at this now, but I'm happy to point you in the right direction and reply to any questions you might have. At least that's probably an easier option than sitting down trying to read the PCI specification ;)
Same problem here with time and I don't feel like wanting to debug this without understanding how it should work so I pass this for now. If I make any progress I'll let you know. (I think I should at least understand the mappings and address translations and their representation in OpenBIOS to know what am I doing but I don't have that knowledge.)
Well I'm sure I'll still be around when you do manage to work on it ;)
But I do highly recommend posting questions and RFC patches to the list, as there are people on the mailing list other than myself who know a lot more about PCI than I do.
ATB,
Mark.
On Sun, 16 Jul 2017, Mark Cave-Ayland wrote:
On 16/07/17 16:49, BALATON Zoltan wrote:
On Sun, 16 Jul 2017, Mark Cave-Ayland wrote:
On 16/07/17 14:56, BALATON Zoltan wrote:
They are for PPC, but unfortunately not for SPARC64. That's why they were split out as separate values in the last set of patches because the existing logic couldn't handle all cases of multiple address spaces with different offsets :(
I don't see why SPARC64 is not also redundant. Here's its pci arch struct:
.pci = { .name = "SUNW,sabre", .vendor_id = PCI_VENDOR_ID_SUN, .device_id = PCI_DEVICE_ID_SUN_SABRE, .cfg_addr = APB_SPECIAL_BASE + 0x1000000ULL, // PCI bus configuration space .cfg_data = APB_MEM_BASE, // PCI bus memory space .cfg_base = APB_SPECIAL_BASE, .cfg_len = 0x1000000, .host_pci_base = APB_MEM_BASE, .pci_mem_base = 0x100000, /* avoid VGA at 0xa0000 */ .mem_len = 0xf0000000, .io_base = APB_SPECIAL_BASE + 0x2000000ULL, // PCI Bus I/O space .io_len = 0x1000000, .host_ranges = { { .type = CONFIGURATION_SPACE, .parentaddr = 0, .childaddr = APB_SPECIAL_BASE + 0x1000000ULL, .len = 0x1000000 }, { .type = IO_SPACE, .parentaddr = 0, .childaddr = APB_SPECIAL_BASE + 0x2000000ULL, .len = 0x1000000 }, { .type = MEMORY_SPACE_32, .parentaddr = 0, .childaddr = APB_MEM_BASE, .len = 0xf0000000 }, { .type = 0, .parentaddr = 0, .childaddr = 0, .len = 0 } }, .irqs = { 0, 1, 2, 3 }, },
The addresses and lengths here are also mostly the same as in host_ranges. Where does the difference come from and could we get rid of the individual *_base, *_addr and *_len fields and define everything in the host_ranges structure? (Maybe we need some additional offsets in the config space but that's all we should need I think.)
It looks like that now the host_ranges are used to create the ranges property while the other addresses and lengths are used to create the mapping but these two should match (because the ranges property is supposed to describe these mappings) so one should be computable from the other otherwise we may have a bug (or I'm missing something which is also likely as I know nothing about this). Therefore I think it should be possible to get rid of one of these. What am I missing?
For SPARC64 the issue is with pci_mem_base and how it is interpreted in Linux. Before the recent set of patches, the .childaddr for MEMORY_SPACE_32 was generated as APB_MEM_BASE + pci_mem_base, so in effect all we do is miss mapping the first PCI memory section.
However in Linux the kernel calculates the address of the host range is by taking the value of an undocumented register in the Simba host bridge (see simba_config_cb() for details) and then adding the offsets on top meaning that you end up with all your addresses out by pci_mem_base.
Hence the need to apply pci_mem_base as the starting value for assigning devices to PCI memory space, but not including it in the host ranges itself.
So does that mean we could theoretically get rid of all the other redundant data and put this pci_mem_base offset for SPARC64 in some #ifdefed code part (or in the undocumented register to counter the offset in Linux) to simplify the current situation where the ranges are defined in two different ways?
There was some code for config space registers already #ifdef'd for SPARC64 but the issue there was that with so many conditionals used for building up the PCI host bridge ranges that it was very difficult to see at a glance what the resulting values were.
Bear in mind that PPC also had its own hacks in that it has multiple IO memory spaces listed for the host bridge, so rather than keep going down this maze of conditional logic and #ifdef, I decided to explicitly list the values themselves which solves all the problems in one go.
If you wanted to better document the relationships between the host ranges properties and the corresponding values in pci_arch_t then you could use some #defines, but then if you're working down at that level regardless then I suspect this will be the least of your problems.
No I don't mean defines but something like this:
From: BALATON Zoltan balaton@eik.bme.hu Date: Sun, 16 Jul 2017 22:41:38 +0200 Subject: [PATCH] RFC patch to remove the now redundant io_base and io_len from pci_arch_t that are present in newly added pci ranges. This was only compile tested on PPC and not sure what to do with other redundandant addresses and lengths but similar handling might be possible for those too. Feel free to do anything with this patch.
Signed-off-by: BALATON Zoltan balaton@eik.bme.hu --- arch/ppc/qemu/init.c | 12 ++---------- drivers/pci.c | 39 ++++++++++++++++++++++++++++++--------- include/drivers/pci.h | 19 ++++++++++--------- 3 files changed, 42 insertions(+), 28 deletions(-)
diff --git a/arch/ppc/qemu/init.c b/arch/ppc/qemu/init.c index b0c0cfc..a4746c2 100644 --- a/arch/ppc/qemu/init.c +++ b/arch/ppc/qemu/init.c @@ -104,8 +104,6 @@ static const pci_arch_t known_arch[] = { .host_pci_base = 0xc0000000, .pci_mem_base = 0x100000, /* avoid VGA at 0xa0000 */ .mem_len = 0x10000000, - .io_base = 0x80000000, - .io_len = 0x00010000, .host_ranges = { { .type = IO_SPACE, .parentaddr = 0, .childaddr = 0x80000000, .len = 0x00010000 }, { .type = MEMORY_SPACE_32, .parentaddr = 0, .childaddr = 0xc0100000, .len = 0x10000000 }, @@ -124,8 +122,6 @@ static const pci_arch_t known_arch[] = { .host_pci_base = 0x0, .pci_mem_base = 0x80000000, .mem_len = 0x10000000, - .io_base = 0xf2000000, - .io_len = 0x00800000, .host_ranges = { { .type = IO_SPACE, .parentaddr = 0, .childaddr = 0xf2000000, .len = 0x00800000 }, { .type = MEMORY_SPACE_32, .parentaddr = 0x80000000, .childaddr = 0x80000000, .len = 0x10000000 }, @@ -144,8 +140,6 @@ static const pci_arch_t known_arch[] = { .host_pci_base = 0x0, .pci_mem_base = 0x80000000, .mem_len = 0x10000000, - .io_base = 0xf2000000, - .io_len = 0x00800000, .host_ranges = { { .type = IO_SPACE, .parentaddr = 0, .childaddr = 0xf2000000, .len = 0x00800000 }, { .type = MEMORY_SPACE_32, .parentaddr = 0x80000000, .childaddr = 0x80000000, .len = 0x10000000 }, @@ -164,8 +158,6 @@ static const pci_arch_t known_arch[] = { .host_pci_base = 0x0, .pci_mem_base = 0x80000000, .mem_len = 0x10000000, - .io_base = 0xfe000000, - .io_len = 0x00800000, .host_ranges = { { .type = IO_SPACE, .parentaddr = 0, .childaddr = 0xfe000000, .len = 0x00800000 }, { .type = MEMORY_SPACE_32, .parentaddr = 0, .childaddr = 0xfd000000, .len = 0x01000000 }, @@ -198,8 +190,8 @@ entry(void) arch = &known_arch[machine_id]; } } - - isa_io_base = arch->io_base; + const pci_range_t *range = ob_pci_get_range(IO_SPACE, arch->host_ranges); + isa_io_base = range->childaddr;
#ifdef CONFIG_DEBUG_CONSOLE if (is_apple()) { diff --git a/drivers/pci.c b/drivers/pci.c index f6d9437..2f20dcc 100644 --- a/drivers/pci.c +++ b/drivers/pci.c @@ -140,14 +140,21 @@ static void dump_reg_property(const char* description, int nreg, u32 *reg)
static unsigned long pci_bus_addr_to_host_addr(int space, uint32_t ba) { - if (space == IO_SPACE) { - return arch->io_base + (unsigned long)ba; - } else if (space == MEMORY_SPACE_32) { - return arch->host_pci_base + (unsigned long)ba; - } else { + unsigned long host_addr = (unsigned long)ba; + const pci_range_t *range = ob_pci_get_range(space, arch->host_ranges); + + switch (space) { + case IO_SPACE: + host_addr += range->childaddr; + break; + case MEMORY_SPACE_32: + host_addr += arch->host_pci_base; + break; + default: /* Return unaltered to aid debugging property values */ - return (unsigned long)ba; + break; } + return host_addr; }
static void @@ -491,6 +498,17 @@ static void pci_host_set_ranges(const pci_config_t *config) set_property(dev, "ranges", (char *)props, ncells * sizeof(props[0])); }
+const pci_range_t *ob_pci_get_range(pci_range_type type, const pci_range_t ranges[]) +{ + const pci_range_t *range = NULL; + int i; + for (i = 0; i < 4 && ranges[i].type != SPACE_END; i++) { + if (ranges[i].type == type) + range = &ranges[i]; + } + return range; +} + int host_config_cb(const pci_config_t *config) { //XXX this overrides "reg" property @@ -988,11 +1006,12 @@ int ebus_config_cb(const pci_config_t *config)
int i82378_config_cb(const pci_config_t *config) { + const pci_range_t *range = ob_pci_get_range(IO_SPACE, arch->host_ranges); #ifdef CONFIG_DRIVER_PC_SERIAL - ob_pc_serial_init(config->path, "serial", arch->io_base, 0x3f8ULL, 0); + ob_pc_serial_init(config->path, "serial", range->childaddr, 0x3f8ULL, 0); #endif #ifdef CONFIG_DRIVER_PC_KBD - ob_pc_kbd_init(config->path, "8042", NULL, arch->io_base, 0x60ULL, 0, 0); + ob_pc_kbd_init(config->path, "8042", NULL, range->childaddr, 0x60ULL, 0, 0); #endif #ifdef CONFIG_DRIVER_IDE ob_ide_init(config->path, 0x1f0, 0x3f6, 0x170, 0x376); @@ -1649,12 +1668,14 @@ static void ob_pci_set_available(phandle_t host, unsigned long mem_base, unsigne /* Create an available property for both memory and IO space */ uint32_t props[10]; int ncells; + const pci_range_t *range;
ncells = 0; ncells += pci_encode_phys_addr(props + ncells, 0, MEMORY_SPACE_32, 0, 0, mem_base); ncells += pci_encode_size(props + ncells, arch->mem_len - mem_base); ncells += pci_encode_phys_addr(props + ncells, 0, IO_SPACE, 0, 0, io_base); - ncells += pci_encode_size(props + ncells, arch->io_len - io_base); + range = ob_pci_get_range(IO_SPACE, arch->host_ranges); + ncells += pci_encode_size(props + ncells, range->len - io_base);
set_property(host, "available", (char *)props, ncells * sizeof(props[0])); } diff --git a/include/drivers/pci.h b/include/drivers/pci.h index e9f20a1..e267db2 100644 --- a/include/drivers/pci.h +++ b/include/drivers/pci.h @@ -1,19 +1,20 @@ #ifndef _H_PCI #define _H_PCI
-enum { - CONFIGURATION_SPACE = 0, - IO_SPACE = 1, - MEMORY_SPACE_32 = 2, - MEMORY_SPACE_64 = 3, -}; +typedef enum { + SPACE_END = 0, + CONFIGURATION_SPACE, + IO_SPACE, + MEMORY_SPACE_32, + MEMORY_SPACE_64 +} pci_range_type;
typedef uint32_t pci_addr;
typedef struct pci_range_t pci_range_t;
struct pci_range_t { - unsigned int type; + pci_range_type type; unsigned long childaddr; unsigned long parentaddr; unsigned long len; @@ -32,14 +33,14 @@ struct pci_arch_t { unsigned long host_pci_base; /* offset of PCI memory space within host memory space */ unsigned long pci_mem_base; /* in PCI memory space */ unsigned long mem_len; - unsigned long io_base; - unsigned long io_len; pci_range_t host_ranges[4]; uint8_t irqs[4]; };
extern const pci_arch_t *arch;
+const pci_range_t *ob_pci_get_range(pci_range_type type, const pci_range_t ranges[]); + /* Device tree offsets */
#define PCI_INT_MAP_PCI0 0
On 16/07/17 21:56, BALATON Zoltan wrote:
So does that mean we could theoretically get rid of all the other redundant data and put this pci_mem_base offset for SPARC64 in some #ifdefed code part (or in the undocumented register to counter the offset in Linux) to simplify the current situation where the ranges are defined in two different ways?
There was some code for config space registers already #ifdef'd for SPARC64 but the issue there was that with so many conditionals used for building up the PCI host bridge ranges that it was very difficult to see at a glance what the resulting values were.
Bear in mind that PPC also had its own hacks in that it has multiple IO memory spaces listed for the host bridge, so rather than keep going down this maze of conditional logic and #ifdef, I decided to explicitly list the values themselves which solves all the problems in one go.
If you wanted to better document the relationships between the host ranges properties and the corresponding values in pci_arch_t then you could use some #defines, but then if you're working down at that level regardless then I suspect this will be the least of your problems.
No I don't mean defines but something like this:
From: BALATON Zoltan balaton@eik.bme.hu Date: Sun, 16 Jul 2017 22:41:38 +0200 Subject: [PATCH] RFC patch to remove the now redundant io_base and io_len from pci_arch_t that are present in newly added pci ranges. This was only compile tested on PPC and not sure what to do with other redundandant addresses and lengths but similar handling might be possible for those too. Feel free to do anything with this patch.
Signed-off-by: BALATON Zoltan balaton@eik.bme.hu
arch/ppc/qemu/init.c | 12 ++---------- drivers/pci.c | 39 ++++++++++++++++++++++++++++++--------- include/drivers/pci.h | 19 ++++++++++--------- 3 files changed, 42 insertions(+), 28 deletions(-)
diff --git a/arch/ppc/qemu/init.c b/arch/ppc/qemu/init.c index b0c0cfc..a4746c2 100644 --- a/arch/ppc/qemu/init.c +++ b/arch/ppc/qemu/init.c @@ -104,8 +104,6 @@ static const pci_arch_t known_arch[] = { .host_pci_base = 0xc0000000, .pci_mem_base = 0x100000, /* avoid VGA at 0xa0000 */ .mem_len = 0x10000000,
.io_base = 0x80000000,.io_len = 0x00010000, .host_ranges = { { .type = IO_SPACE, .parentaddr = 0, .childaddr = 0x80000000, .len = 0x00010000 }, { .type = MEMORY_SPACE_32, .parentaddr = 0, .childaddr = 0xc0100000, .len = 0x10000000 },@@ -124,8 +122,6 @@ static const pci_arch_t known_arch[] = { .host_pci_base = 0x0, .pci_mem_base = 0x80000000, .mem_len = 0x10000000,
.io_base = 0xf2000000,.io_len = 0x00800000, .host_ranges = { { .type = IO_SPACE, .parentaddr = 0, .childaddr = 0xf2000000, .len = 0x00800000 }, { .type = MEMORY_SPACE_32, .parentaddr = 0x80000000, .childaddr = 0x80000000, .len = 0x10000000 },@@ -144,8 +140,6 @@ static const pci_arch_t known_arch[] = { .host_pci_base = 0x0, .pci_mem_base = 0x80000000, .mem_len = 0x10000000,
.io_base = 0xf2000000,.io_len = 0x00800000, .host_ranges = { { .type = IO_SPACE, .parentaddr = 0, .childaddr = 0xf2000000, .len = 0x00800000 }, { .type = MEMORY_SPACE_32, .parentaddr = 0x80000000, .childaddr = 0x80000000, .len = 0x10000000 },@@ -164,8 +158,6 @@ static const pci_arch_t known_arch[] = { .host_pci_base = 0x0, .pci_mem_base = 0x80000000, .mem_len = 0x10000000,
.io_base = 0xfe000000,.io_len = 0x00800000, .host_ranges = { { .type = IO_SPACE, .parentaddr = 0, .childaddr = 0xfe000000, .len = 0x00800000 }, { .type = MEMORY_SPACE_32, .parentaddr = 0, .childaddr = 0xfd000000, .len = 0x01000000 },@@ -198,8 +190,8 @@ entry(void) arch = &known_arch[machine_id]; } }
- isa_io_base = arch->io_base;
- const pci_range_t *range = ob_pci_get_range(IO_SPACE, arch->host_ranges);
- isa_io_base = range->childaddr;
#ifdef CONFIG_DEBUG_CONSOLE if (is_apple()) { diff --git a/drivers/pci.c b/drivers/pci.c index f6d9437..2f20dcc 100644 --- a/drivers/pci.c +++ b/drivers/pci.c @@ -140,14 +140,21 @@ static void dump_reg_property(const char* description, int nreg, u32 *reg)
static unsigned long pci_bus_addr_to_host_addr(int space, uint32_t ba) {
- if (space == IO_SPACE) {
return arch->io_base + (unsigned long)ba;- } else if (space == MEMORY_SPACE_32) {
return arch->host_pci_base + (unsigned long)ba;- } else {
- unsigned long host_addr = (unsigned long)ba;
- const pci_range_t *range = ob_pci_get_range(space, arch->host_ranges);
- switch (space) {
- case IO_SPACE:
host_addr += range->childaddr;break;- case MEMORY_SPACE_32:
host_addr += arch->host_pci_base;break;- default: /* Return unaltered to aid debugging property values */
return (unsigned long)ba;
break;- return host_addr;
}
static void @@ -491,6 +498,17 @@ static void pci_host_set_ranges(const pci_config_t *config) set_property(dev, "ranges", (char *)props, ncells * sizeof(props[0])); }
+const pci_range_t *ob_pci_get_range(pci_range_type type, const pci_range_t ranges[]) +{
- const pci_range_t *range = NULL;
- int i;
- for (i = 0; i < 4 && ranges[i].type != SPACE_END; i++) {
if (ranges[i].type == type)range = &ranges[i];- }
- return range;
+}
int host_config_cb(const pci_config_t *config) { //XXX this overrides "reg" property @@ -988,11 +1006,12 @@ int ebus_config_cb(const pci_config_t *config)
int i82378_config_cb(const pci_config_t *config) {
- const pci_range_t *range = ob_pci_get_range(IO_SPACE, arch->host_ranges);
#ifdef CONFIG_DRIVER_PC_SERIAL
- ob_pc_serial_init(config->path, "serial", arch->io_base, 0x3f8ULL, 0);
- ob_pc_serial_init(config->path, "serial", range->childaddr, 0x3f8ULL, 0);
#endif #ifdef CONFIG_DRIVER_PC_KBD
- ob_pc_kbd_init(config->path, "8042", NULL, arch->io_base, 0x60ULL, 0, 0);
- ob_pc_kbd_init(config->path, "8042", NULL, range->childaddr, 0x60ULL, 0, 0);
#endif #ifdef CONFIG_DRIVER_IDE ob_ide_init(config->path, 0x1f0, 0x3f6, 0x170, 0x376); @@ -1649,12 +1668,14 @@ static void ob_pci_set_available(phandle_t host, unsigned long mem_base, unsigne /* Create an available property for both memory and IO space */ uint32_t props[10]; int ncells;
const pci_range_t *range;
ncells = 0; ncells += pci_encode_phys_addr(props + ncells, 0, MEMORY_SPACE_32, 0, 0, mem_base); ncells += pci_encode_size(props + ncells, arch->mem_len - mem_base); ncells += pci_encode_phys_addr(props + ncells, 0, IO_SPACE, 0, 0, io_base);
- ncells += pci_encode_size(props + ncells, arch->io_len - io_base);
range = ob_pci_get_range(IO_SPACE, arch->host_ranges);
ncells += pci_encode_size(props + ncells, range->len - io_base);
set_property(host, "available", (char *)props, ncells * sizeof(props[0]));
} diff --git a/include/drivers/pci.h b/include/drivers/pci.h index e9f20a1..e267db2 100644 --- a/include/drivers/pci.h +++ b/include/drivers/pci.h @@ -1,19 +1,20 @@ #ifndef _H_PCI #define _H_PCI
-enum {
- CONFIGURATION_SPACE = 0,
- IO_SPACE = 1,
- MEMORY_SPACE_32 = 2,
- MEMORY_SPACE_64 = 3,
-}; +typedef enum {
- SPACE_END = 0,
- CONFIGURATION_SPACE,
- IO_SPACE,
- MEMORY_SPACE_32,
- MEMORY_SPACE_64
+} pci_range_type;
The space identifiers need to be identical to the current ones since these are well-known values used by client OSs to enumerate the space types from the device tree.
typedef uint32_t pci_addr;
typedef struct pci_range_t pci_range_t;
struct pci_range_t {
- unsigned int type;
- pci_range_type type; unsigned long childaddr; unsigned long parentaddr; unsigned long len;
@@ -32,14 +33,14 @@ struct pci_arch_t { unsigned long host_pci_base; /* offset of PCI memory space within host memory space */ unsigned long pci_mem_base; /* in PCI memory space */ unsigned long mem_len;
- unsigned long io_base;
- unsigned long io_len; pci_range_t host_ranges[4]; uint8_t irqs[4];
};
extern const pci_arch_t *arch;
+const pci_range_t *ob_pci_get_range(pci_range_type type, const pci_range_t ranges[]);
/* Device tree offsets */
#define PCI_INT_MAP_PCI0 0
I can see what you're trying to do here, however for me the part I like least about this is that it introduces asymmetry between the PCI IO and MMIO memory spaces. Effectively what you're doing is saying that for PCI IO memory you need to extract the information from the range whilst for PCI MMIO memory you need to use the standard struct values, which for me seems something that's easy to trip up developers.
If you could find a way to make the changes consistent between both MMIO and IO memory I think the patch will improve things, although you also need to handle the case for ARCH_HEATHROW where you have multiple 32-bit MMIO ranges and for SPARC64 whereby the MMIO child address is not the same as pci_mem_base.
Note that if you experiment further with this you can also see that .parentaddr is also equivalent to host_pci_base which can help simplify things further.
ATB,
Mark.
On Sun, 16 Jul 2017, Mark Cave-Ayland wrote:
On 16/07/17 21:56, BALATON Zoltan wrote:
So does that mean we could theoretically get rid of all the other redundant data and put this pci_mem_base offset for SPARC64 in some #ifdefed code part (or in the undocumented register to counter the offset in Linux) to simplify the current situation where the ranges are defined in two different ways?
There was some code for config space registers already #ifdef'd for SPARC64 but the issue there was that with so many conditionals used for building up the PCI host bridge ranges that it was very difficult to see at a glance what the resulting values were.
Bear in mind that PPC also had its own hacks in that it has multiple IO memory spaces listed for the host bridge, so rather than keep going down this maze of conditional logic and #ifdef, I decided to explicitly list the values themselves which solves all the problems in one go.
If you wanted to better document the relationships between the host ranges properties and the corresponding values in pci_arch_t then you could use some #defines, but then if you're working down at that level regardless then I suspect this will be the least of your problems.
No I don't mean defines but something like this:
From: BALATON Zoltan balaton@eik.bme.hu Date: Sun, 16 Jul 2017 22:41:38 +0200 Subject: [PATCH] RFC patch to remove the now redundant io_base and io_len from pci_arch_t that are present in newly added pci ranges. This was only compile tested on PPC and not sure what to do with other redundandant addresses and lengths but similar handling might be possible for those too. Feel free to do anything with this patch.
Signed-off-by: BALATON Zoltan balaton@eik.bme.hu
arch/ppc/qemu/init.c | 12 ++---------- drivers/pci.c | 39 ++++++++++++++++++++++++++++++--------- include/drivers/pci.h | 19 ++++++++++--------- 3 files changed, 42 insertions(+), 28 deletions(-)
diff --git a/arch/ppc/qemu/init.c b/arch/ppc/qemu/init.c index b0c0cfc..a4746c2 100644 --- a/arch/ppc/qemu/init.c +++ b/arch/ppc/qemu/init.c @@ -104,8 +104,6 @@ static const pci_arch_t known_arch[] = { .host_pci_base = 0xc0000000, .pci_mem_base = 0x100000, /* avoid VGA at 0xa0000 */ .mem_len = 0x10000000,
.io_base = 0x80000000,.io_len = 0x00010000, .host_ranges = { { .type = IO_SPACE, .parentaddr = 0, .childaddr = 0x80000000, .len = 0x00010000 }, { .type = MEMORY_SPACE_32, .parentaddr = 0, .childaddr = 0xc0100000, .len = 0x10000000 },@@ -124,8 +122,6 @@ static const pci_arch_t known_arch[] = { .host_pci_base = 0x0, .pci_mem_base = 0x80000000, .mem_len = 0x10000000,
.io_base = 0xf2000000,.io_len = 0x00800000, .host_ranges = { { .type = IO_SPACE, .parentaddr = 0, .childaddr = 0xf2000000, .len = 0x00800000 }, { .type = MEMORY_SPACE_32, .parentaddr = 0x80000000, .childaddr = 0x80000000, .len = 0x10000000 },@@ -144,8 +140,6 @@ static const pci_arch_t known_arch[] = { .host_pci_base = 0x0, .pci_mem_base = 0x80000000, .mem_len = 0x10000000,
.io_base = 0xf2000000,.io_len = 0x00800000, .host_ranges = { { .type = IO_SPACE, .parentaddr = 0, .childaddr = 0xf2000000, .len = 0x00800000 }, { .type = MEMORY_SPACE_32, .parentaddr = 0x80000000, .childaddr = 0x80000000, .len = 0x10000000 },@@ -164,8 +158,6 @@ static const pci_arch_t known_arch[] = { .host_pci_base = 0x0, .pci_mem_base = 0x80000000, .mem_len = 0x10000000,
.io_base = 0xfe000000,.io_len = 0x00800000, .host_ranges = { { .type = IO_SPACE, .parentaddr = 0, .childaddr = 0xfe000000, .len = 0x00800000 }, { .type = MEMORY_SPACE_32, .parentaddr = 0, .childaddr = 0xfd000000, .len = 0x01000000 },@@ -198,8 +190,8 @@ entry(void) arch = &known_arch[machine_id]; } }
- isa_io_base = arch->io_base;
- const pci_range_t *range = ob_pci_get_range(IO_SPACE, arch->host_ranges);
- isa_io_base = range->childaddr;
#ifdef CONFIG_DEBUG_CONSOLE if (is_apple()) { diff --git a/drivers/pci.c b/drivers/pci.c index f6d9437..2f20dcc 100644 --- a/drivers/pci.c +++ b/drivers/pci.c @@ -140,14 +140,21 @@ static void dump_reg_property(const char* description, int nreg, u32 *reg)
static unsigned long pci_bus_addr_to_host_addr(int space, uint32_t ba) {
- if (space == IO_SPACE) {
return arch->io_base + (unsigned long)ba;- } else if (space == MEMORY_SPACE_32) {
return arch->host_pci_base + (unsigned long)ba;- } else {
- unsigned long host_addr = (unsigned long)ba;
- const pci_range_t *range = ob_pci_get_range(space, arch->host_ranges);
- switch (space) {
- case IO_SPACE:
host_addr += range->childaddr;break;- case MEMORY_SPACE_32:
host_addr += arch->host_pci_base;break;- default: /* Return unaltered to aid debugging property values */
return (unsigned long)ba;
break;- return host_addr;
}
static void @@ -491,6 +498,17 @@ static void pci_host_set_ranges(const pci_config_t *config) set_property(dev, "ranges", (char *)props, ncells * sizeof(props[0])); }
+const pci_range_t *ob_pci_get_range(pci_range_type type, const pci_range_t ranges[]) +{
- const pci_range_t *range = NULL;
- int i;
- for (i = 0; i < 4 && ranges[i].type != SPACE_END; i++) {
if (ranges[i].type == type)range = &ranges[i];- }
- return range;
+}
int host_config_cb(const pci_config_t *config) { //XXX this overrides "reg" property @@ -988,11 +1006,12 @@ int ebus_config_cb(const pci_config_t *config)
int i82378_config_cb(const pci_config_t *config) {
- const pci_range_t *range = ob_pci_get_range(IO_SPACE, arch->host_ranges);
#ifdef CONFIG_DRIVER_PC_SERIAL
- ob_pc_serial_init(config->path, "serial", arch->io_base, 0x3f8ULL, 0);
- ob_pc_serial_init(config->path, "serial", range->childaddr, 0x3f8ULL, 0);
#endif #ifdef CONFIG_DRIVER_PC_KBD
- ob_pc_kbd_init(config->path, "8042", NULL, arch->io_base, 0x60ULL, 0, 0);
- ob_pc_kbd_init(config->path, "8042", NULL, range->childaddr, 0x60ULL, 0, 0);
#endif #ifdef CONFIG_DRIVER_IDE ob_ide_init(config->path, 0x1f0, 0x3f6, 0x170, 0x376); @@ -1649,12 +1668,14 @@ static void ob_pci_set_available(phandle_t host, unsigned long mem_base, unsigne /* Create an available property for both memory and IO space */ uint32_t props[10]; int ncells;
const pci_range_t *range;
ncells = 0; ncells += pci_encode_phys_addr(props + ncells, 0, MEMORY_SPACE_32, 0, 0, mem_base); ncells += pci_encode_size(props + ncells, arch->mem_len - mem_base); ncells += pci_encode_phys_addr(props + ncells, 0, IO_SPACE, 0, 0, io_base);
- ncells += pci_encode_size(props + ncells, arch->io_len - io_base);
range = ob_pci_get_range(IO_SPACE, arch->host_ranges);
ncells += pci_encode_size(props + ncells, range->len - io_base);
set_property(host, "available", (char *)props, ncells * sizeof(props[0]));
} diff --git a/include/drivers/pci.h b/include/drivers/pci.h index e9f20a1..e267db2 100644 --- a/include/drivers/pci.h +++ b/include/drivers/pci.h @@ -1,19 +1,20 @@ #ifndef _H_PCI #define _H_PCI
-enum {
- CONFIGURATION_SPACE = 0,
- IO_SPACE = 1,
- MEMORY_SPACE_32 = 2,
- MEMORY_SPACE_64 = 3,
-}; +typedef enum {
- SPACE_END = 0,
- CONFIGURATION_SPACE,
- IO_SPACE,
- MEMORY_SPACE_32,
- MEMORY_SPACE_64
+} pci_range_type;
The space identifiers need to be identical to the current ones since these are well-known values used by client OSs to enumerate the space types from the device tree.
Are these PCI bar numbers? Maybe the ranges should be indexed by these then so we don't need the loop to find the right range but we can just get host_ranges[IO_SPACE] and so on. That may be more efficient than always looping through this array.
typedef uint32_t pci_addr;
typedef struct pci_range_t pci_range_t;
struct pci_range_t {
- unsigned int type;
- pci_range_type type; unsigned long childaddr; unsigned long parentaddr; unsigned long len;
@@ -32,14 +33,14 @@ struct pci_arch_t { unsigned long host_pci_base; /* offset of PCI memory space within host memory space */ unsigned long pci_mem_base; /* in PCI memory space */ unsigned long mem_len;
- unsigned long io_base;
- unsigned long io_len; pci_range_t host_ranges[4]; uint8_t irqs[4];
};
extern const pci_arch_t *arch;
+const pci_range_t *ob_pci_get_range(pci_range_type type, const pci_range_t ranges[]);
/* Device tree offsets */
#define PCI_INT_MAP_PCI0 0
I can see what you're trying to do here, however for me the part I like least about this is that it introduces asymmetry between the PCI IO and MMIO memory spaces. Effectively what you're doing is saying that for PCI IO memory you need to extract the information from the range whilst for PCI MMIO memory you need to use the standard struct values, which for me seems something that's easy to trip up developers.
No, I meant it should be done also for MMIO (and maybe config regions as well if possible) but I haven't done that. I've only sent this patch to explain what I meant, this is not finished. I'll leave that to someone interested. My comment was that we should have either the ranges or the struct values but not both as that's redundant. It should be possible to compute the ranges from the values but you've found that was not working and it's clearer to start from ranges. That's fine but then we should not need the struct values and get those from the ranges or if we can't do that then there may a bug somewhere as these should be identical because they represent the same thing, don't they?
If you could find a way to make the changes consistent between both MMIO and IO memory I think the patch will improve things, although you also need to handle the case for ARCH_HEATHROW where you have multiple 32-bit MMIO ranges and for SPARC64 whereby the MMIO child address is not the same as pci_mem_base.
These are the cases that I said it needs understanding of pci structure of each arch in which I'm not really interested and have no time for now. But why does Heathrow have two mmio ranges? I've found this log: https://gist.github.com/tomari/3186021 This mentions that the one at fd000000 is some kind of ISA memory hole so this is probably a special case that could be added by an #ifdefed hack in pci.c and not need to be present in the pci_arch_t? For the SPARC64 case I only know what you've said but if I got that correctly the problem is that while mapping would be the same but if you do that Linux adds an offset from a special register and this makes the values off. So can't you patch this by controlling the special register in QEMU and return the right offset (or the negative of the value by which the mapping is off to counter this problem)? Then SPARC64 should not need special handling but I'm likely missing something here.
Note that if you experiment further with this you can also see that .parentaddr is also equivalent to host_pci_base which can help simplify things further.
And this is where understanding of these pci mappings and representations would help. As I don't have this understanding I don't think I can do it easily. I don't know for example what the cfg_* values are but I think they might be offsets into a configuration space, so at least some of those could also be removed (with that a config space may need to be defined for Apple machines too but an #ifdef in pci.c to not add ranges for that in the device tree).
So all this is just brain storming and don't wait for me to provide a better patch. I'm just trying to get my idea across, I don't intend to work on this in the near future.
Regards, BALATON Zoltan
-
BALATON Zoltan -
Mark Cave-Ayland -
Segher Boessenkool -
Tarl Neustaedter