Hello,
Thanks for the review.
On Sat, 26 Oct 2019, Mark Cave-Ayland wrote:
On 05/08/2019 21:15, BALATON Zoltan wrote:
diff --git a/drivers/pci.c b/drivers/pci.c index 4bc02c9..eb5f7d3 100644 --- a/drivers/pci.c +++ b/drivers/pci.c @@ -408,20 +408,32 @@ static void ob_pci_unmap(ucell virt, ucell size) { static void ob_pci_bus_map_in(int *idx) {
- uint32_t ba;
- ucell size;
- ucell virt;
- PCI_DPRINTF("ob_pci_bar_map_in idx=%p\n", idx);
- fword("pci-map-in");
+}
- size = POP();
- POP();
- POP();
- ba = POP();
+static void +ob_pci_bus_map_out(int *idx) +{
- /* Should call pci-map-out but nothing to do in it yet */
- fword("2drop");
+}
- virt = ob_pci_map(ba, size);
+static void +ob_pci_config_read8(int *idx) +{
- cell hi = POP();
Should this not be ucell?
- pci_addr addr = PCI_ADDR(PCI_BUS(hi), PCI_DEV(hi), PCI_FN(hi));
- uint8_t val = pci_config_read8(addr, hi & 0xff);
- PUSH(val);
+}
- PUSH(virt);
+static void +ob_pci_config_write8(int *idx) +{
- cell hi = POP();
and here too?
Not sure, I must have copied these from somewhere but I can't remember from where. Maybe from ob_pci_encode_unit? I can change it to ucell if that makes more sense, I can't tell.
- pci_addr addr = PCI_ADDR(PCI_BUS(hi), PCI_DEV(hi), PCI_FN(hi));
- cell val = POP();
- pci_config_write8(addr, hi & 0xff, val & 0xff);
}
What about the other config-* words? Given that you're done the byte implementation then you might as well do the same for the other accesses.
Those were not needed for the ROM I've tried to run so I did not bother in this first version that I've submitted for comments and to let others also test with the ROM. I can look at those as well when I get some time for it unless someone is willing to contribute them later based on this example.
static void @@ -459,7 +471,10 @@ NODE_METHODS(ob_pci_bus_node) = { { "close", ob_pci_close }, { "decode-unit", ob_pci_decode_unit }, { "encode-unit", ob_pci_encode_unit },
- { "pci-map-in", ob_pci_bus_map_in },
- { "map-in", ob_pci_bus_map_in },
- { "map-out", ob_pci_bus_map_out },
- { "config-b@", ob_pci_config_read8 },
- { "config-b!", ob_pci_config_write8 }, { "dma-alloc", ob_pci_dma_alloc }, { "dma-free", ob_pci_dma_free }, { "dma-map-in", ob_pci_dma_map_in },
@@ -473,7 +488,14 @@ static void ob_pci_bridge_map_in(int *idx) { /* As per the IEEE-1275 PCI specification, chain up to the parent */
- call_parent_method("pci-map-in");
- call_parent_method("map-in");
+}
+static void +ob_pci_bridge_map_out(int *idx) +{
- /* As per the IEEE-1275 PCI specification, chain up to the parent */
- call_parent_method("map-out");
}
NODE_METHODS(ob_pci_bridge_node) = { @@ -481,7 +503,8 @@ NODE_METHODS(ob_pci_bridge_node) = { { "close", ob_pci_close }, { "decode-unit", ob_pci_decode_unit }, { "encode-unit", ob_pci_encode_unit },
- { "pci-map-in", ob_pci_bridge_map_in },
- { "map-in", ob_pci_bridge_map_in },
- { "map-out", ob_pci_bridge_map_out }, { "dma-alloc", ob_pci_dma_alloc }, { "dma-free", ob_pci_dma_free }, { "dma-map-in", ob_pci_dma_map_in },
diff --git a/drivers/pci.fs b/drivers/pci.fs index a7b56e1..327c9c9 100644 --- a/drivers/pci.fs +++ b/drivers/pci.fs @@ -37,4 +37,45 @@ then ;
+: pci-map-in ( addr.lo addr.mid addr.hi size -- virt )
- \ Ignore everything but addr.hi and find assigned addr for BAR
- drop nip nip
- \ Save my-self and set it to same as active package because otherwise
- \ decode-phys below seems to be confused. Maybe this should not be needed
- my-self swap active-package ihandle>phandle to my-self
- " assigned-addresses" active-package get-package-property 0= if
- begin
decode-phys ( bar prop prop-len phys.lo phys.mid phys.hi )dup 0fffffff and 6 pick 0fffffff and = if2drop -rotdecode-int drop decode-int drop 2drop( bar addr.lo )\ Now translate addr.lo, This may be wrong\ maybe we should look at parent's ranges instead?over 18 rshift 3 and 1 = if" reg" active-package parent get-package-property 0= if( bar addr.lo prop prop-len reg.addr reg.size )decode-int -rot decode-int 3drop+thenthennip swap to my-selfexitelse3drop \ no match, try nextthen\ Drop the size as we don't need itdecode-int drop decode-int dropdup 0=- until
- 3drop
- to my-self
- -1 \ not found
- else
- drop
- to my-self
- -1 \ could not find assigned-addresses
- then
- ;
[THEN]
A couple of comments here: using Forth here makes this the only PCI word that is implemented in Forth rather than C which tends to make debugging a lot more difficult. I'd much prefer to have the implementation in one place and then consider a conversion later if required.
Since this accesses properties this seemed to be easier in Forth than C I think but can't remember the details now.
Secondly this doesn't seem to be correct in that it doesn't take the physical address and map it to a virtual address and so it assumes this area has been pre-mapped. Is this another piece of code borrowed from SLOF, since it runs in real mode rather than virtual mode?
The idea is from SLOF but I think the I had to rewrite it for OpenBIOS so the code is not the same. It does translate phys address using the reg property (although as the comment says I wasn't sure if that's the correct way) but it does not try to map an unmapped bar. But that's probably not needed because OpenBIOS will map BARs on detecting devices so this word only needs to return the correct address for the already mapped bar (despite what its name may suggest).
diff --git a/forth/device/other.fs b/forth/device/other.fs index 1bed9b8..0ff34b6 100644 --- a/forth/device/other.fs +++ b/forth/device/other.fs @@ -58,21 +58,27 @@ defer (poke) \ 5.3.7.2 Device-register access
: rb@ ( addr -- byte )
- ioc@ ;
: rw@ ( waddr -- w )
- iow@ ;
: rl@ ( qaddr -- quad )
- iol@ ;
: rb! ( byte addr -- )
- ioc! ;
: rw! ( w waddr -- )
- iow! ;
: rl! ( quad qaddr -- )
- iol! ;
: rx@ ( oaddr - o )
I've had a read of the specifications and it's not very clear what type of addresses these are supposed to take - are they PCI addresses or virtual addresses? Can you tell from your ATI FCode ROM?
I can't but here are the relevant parts in case someone can tell:
1316: b(lit) 0x100 1321: new-token 0x88b 1324: b(value)
5814: new-token select-reg-indexed \ 0x990 5817: b(:) 5818: dup 5819: 3 5820: invert 5821: and 5822: var_mapped_mmio_base 5824: const_REG_MM_INDEX 5826: + 5827: rl! 5829: 3 5830: and 5831: var_mapped_mmio_base 5833: const_REG_MM_DATA 5835: + 5836: + 5837: b(;) 5838: new-token mapped-io-base+ \ 0x991 5841: b(:) 5842: var_mapped_mmio_base 5844: + 5845: b(;) 5846: new-token select-reg \ 0x992, uses index/data reg until regs are memory mapped 5849: b(:) 5850: dup 5851: (unnamed-fcode) [0x88b] 5853: < 5854: var_mapped_ram_base 5856: 0<> 5857: or 5858: b?branch 0x0008 ( dest = 5867 ) 5861: mapped-io-base+ 5863: bbranch 0x0006 ( dest = 5870 ) 5866: b(>resolve) 5867: select-reg-indexed 5869: b(>resolve) 5870: b(;) 5871: new-token ati-reg-l@ \ 0x993 5874: b(:) 5875: select-reg 5877: rl@ 5879: b(;) 5880: new-token ati-reg-w@ \ 0x994 5883: b(:) 5884: select-reg 5886: rw@ 5888: b(;) 5889: new-token ati-reg-b@ \ 0x995 5892: b(:) 5893: select-reg 5895: rb@ 5897: b(;) 5898: new-token ati-reg-l! \ 0x996 5901: b(:) 5902: select-reg 5904: rl! 5906: b(;) 5907: new-token ati-reg-w! \ 0x997 5910: b(:) 5911: select-reg 5913: rw! 5915: b(;) 5916: new-token ati-reg-b! \ 0x998 5919: b(:) 5920: select-reg 5922: rb! 5924: b(;)
I think it's memory address but not sure. Also this is on PPC where everything is MMIO so not sure what it would do on other platforms.
The other issue is that I'm not convinced that these words belong in this file, since you're overriding the default implementations with ones that assume little-endian access. I think the correct solution here is implement these words as part of the PCI bus node and use set-token to override the FCode implementation as detailed in the IEEE-1275 specification.
Any examples how to do that? Searching for set-token did not turn up anything useful for me. I also remember Segher had similar suggestions in this thread:
https://mail.coreboot.org/hyperkitty/list/openbios@openbios.org/message/HBBC...
but I've never understood how to implement that. He also said the existing io* words may not be correct on PPC as some of those might need sync or something simlilar but that's a preexisting problem independent of this patch and I don't know what would be the correct way to access mmio so I did not try to fix that (also it appears that's not needed on QEMU and that's only what I care about so I just mention it as a reminder; I don't intend to attempt to fix it because I don't know PPC well enough and can't test it).
Regards, BALATON Zoltan