On Sun, 21 Jul 2019, Mark Cave-Ayland wrote:
On 21/07/2019 15:16, BALATON Zoltan wrote:
OK I came up with the attched patch (on top of your previous series and set-args patch) which is I think about what you've suggested. I kept ob_pci_map for existing callers but renamed it to ob_pci_map_in to keep symmetry between ob_pci_map and ob_pci_unmap with ob_pci_map now only doing mapping as its name suggests (although I think this should still be simpler) and ob_pci_bus_map_in is doing its own address decoding now. The previous ob_pci_map (now called ob_pci_map_in which is a C helper for some of what map-in should do) can be cleaned up as you like. Also cleaned up some white space (tab vs. spaces) but that's excluded from this patch for brevity.
Now I'm really confused - I had a quick look at your patch and compared it with
Good that you're only confused now. :-) I was and still am confused about it all the time so please bear with me.
pci_config_*() words and they show that you can't memory map PCI config space on a Mac. So how did you get to the conclusion earlier in the thread that this was what was required?
Apparently I was confused... Now I realise that this usage of map-in in FCode ROM really wants to map the IO BAR1. My confusion probably comes from that after mapping it it uses config-b[!@] to tweak some regs so I thought it may want config space. (I'm still confused though after reading the description of map-in and addresses in the PCI binding doc. And we probably will need access to config space if we want to implement config-* words in Forth. We could add callbacks to C for those but that's not the nicest way.)
But it does not seem to work yet:
0 > " /pci@f2000000/ATY" open-dev to my-self
ob_pci_bar_map_in idx=1fc5ac54 pci_bus_addr_to_host_addr space=2 ba=0x82000000 ob_pci_map: phys=0x82000000 size=16384 ob_pci_bar_map_in idx=1fc5ac54 pci_bus_addr_to_host_addr space=2 ba=0x81000000 ob_pci_map: phys=0x81000000 size=16777216
?ok
Here we see 2 MMIO BARs being mapped: one of 16K and another of 16M. And this happens as soon as you open the device? I think you'll have to explain more about your command line and how you are getting OpenBIOS to execute the FCode, since I can't tell if this is OpenBIOS or the FCode.
No FCode here, just started OpenBIOS with patched openbios-elf and typed commands that I've seen in the FCode. All this comes from OpenBIOS, don't ask me why it calls this for open-dev.
0 > my-unit? ok 3 > . 7800? ok 2 > . 0? ok 1 > . 0? ok 0 > my-space? ok 1 > . 7800? ok 0 > my-address? ok 2 > . 0? ok 1 > . 0? ok 0 > my-address h# 1000014 my-space + h# 100? ok 4 > .s <4> 0 0 1007814 100 ?ok 4 > " map-in" $call-parent
ob_pci_bar_map_in idx=1fc5ac54 pci_bus_addr_to_host_addr space=1 ba=0x0 ob_pci_map: phys=0xf2000000 size=256
?ok
This is trying to map an IO BAR with 256 bytes, but the phys address isn't going to be correct because OpenBIOS copies arch->io_base into *io_base (and also *mem_base for MMIO) during PCI configuration and advances that copy to hold the start of the next free address.
Where does OpenBIOS copy and advance these values and what do *io_base and *mem_base refer to here? I assume the map-in word runs after the device tree is already constucted so these should already be mapped and map-in might just need to return a virt address for the already mapped bars.
At a very minimum you're going to have to teach the PCI map-in how to use these values instead of making the assumption that the BAR has already been mapped by
Which values?
ob_pci_configure_bar() and the address can be calculated by adding the BAR address to arch->io_base or arch->mem_base according to the space type. This is how pci_bus_addr_to_host_addr() currently works.
My patch called pci_bus_addr_to_host_addr in ob_pci_bus_map_in with the space decoded from phys.hi on the stack yet the virt address came out wrong at the end.
Now I've tried to do instead what SLOF's map-in does:
https://github.com/aik/SLOF/blob/master/slof/fs/pci-config-bridge.fs#L59
and ignore everything except phys.hi and get bar address from that like:
size = POP(); phys[0] = POP(); phys[1] = POP(); phys[2] = POP();
PCI_DPRINTF("%s idx=%p, hi=%"PRIx32", mid=%"PRIx32", lo=%"PRIx32", size=" FMT_ucell"\n", __func__, idx, phys[0], phys[1], phys[2], size); pci_decode_phys_addr(phys, &flags, &space_code, &dev, ®, &addr); /* Check if reg is actually a Base Address Reg */ if (reg < PCI_BASE_ADDR_0 || (reg > PCI_BASE_ADDR_5 && reg != PCI_ROM_ADDRESS)) { PUSH(-1); return; } addr = pci_config_read32(reg, dev); PCI_DPRINTF("%s bar=%"PRIx32"\n", __func__, addr); virt = ob_pci_map(addr, size);
but then bar==-1 (even for MMIO regions which were mapped before) so it looks like it's not mapped at this point so map-in should program the BAR reg or call another function for that?
The other issue is that the PCI enumeration code will already have mapped the ATI BARs once so you'll have to come up with a solution so that if you detect an FCode ROM then the existing PCI configuration code is ignored. Otherwise you'll find that you have mapped the 16M MMIO BAR twice which is going to take quite a bit of MMIO memory and lead to more confusion.
Does it map then unmap the region without freeing the allocated MMIO space? Or if it's still mapped we should just return a pointer to the already mapped space. Could we just get that from assigned-addresses?
Finally one other thing: I'm finding this thread really hard to follow - too much information is spread across too many emails, and the subject line isn't really helping me understand what topic we're following or where I can find related information. May I suggest that it makes sense to break this down into a series of smaller, more concise threads?
I've tried to do that but forgot to update the subject. This thread is now about implementing map-in. The goal is still to run FCode ROMs but first step is to get map-in working.
Thank you, BALATON Zoltan