On Sat, 11 Mar 2023, Mark Cave-Ayland wrote:
According to the IEEE-1275 specification the encode-unit and decode-unit words are static methods which mean they can be called without a current instance.
The PCI bus bindings explicitly mention that the decode-unit methods should insert the bus number into the numerical representation of the bus, a value which is known by the package when the node is created.
In order for this to work when encode-unit and decode-unit are called externally it is necessary to hold a copy of the phandle within the package that can be accessed from C in order to obtain the bus number.
Use the new BIND_NODE_STATIC_METHOD macro to convert encode-unit and decode-unit to static methods, which allows the node to use its own phandle to determine the PCI bus number rather than using the active package.
I wonder if the previous two patches could be dropped if you moved pci-encode-unit and pci-decode-unit to Forth instead? The other Open Firmware implementations likely have code that could be copied and then we don't have to mess with the Forth-C binding part. In OpenFirmware it's here I think: https://github.com/openbios/openfirmware/blob/master/dev/pcibus.fth these seem to be similar to the OpenBoot version which may have some more comments here: https://github.com/openbios/openboot/blob/master/obp/dev/pci/unit.fth
The one in SLOF is even simpler: https://github.com/aik/SLOF/blob/master/board-qemu/slof/pci-phb.fs where hex-decode-unit is the same as parse-nhex I've added recently, although according to the comment SLOF may not handle bus number other than 0 but e.g. in https://github.com/aik/SLOF/blob/master/slof/fs/pci-bridge.fs it just seems to save it in a my-bus variable so it should be simple to add that.
This allows executing decode-unit from a different package (as occurs in set-args where the active package is the child node of the bus parent, and not the bus node itself) to return the correct unit value for PCI devices.
Also OpenFirmware has a comment about the bridge's encode-unit decode-unit here: https://github.com/openbios/openfirmware/blob/master/dev/pci/pcibridg.fth#L1... which seem to be used earlier here: https://github.com/openbios/openfirmware/blob/master/dev/pci/pcibridg.fth#L3... Does that mean we should not change the pci-bus's methods but those of the child nodes instead?
The series likely works but looks to be trying too hard to mix Forth and C where it might be simpler to just do things in one way or other instead of mixing the different parts.
Regards, BALATON Zoltan
Signed-off-by: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk
drivers/pci.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/pci.c b/drivers/pci.c index f30e427..6c1ce66 100644 --- a/drivers/pci.c +++ b/drivers/pci.c @@ -190,7 +190,7 @@ ob_pci_close(int *idx) /* ( str len -- phys.lo phys.mid phys.hi ) */
static void -ob_pci_decode_unit(int *idx) +ob_pci_decode_unit(ucell ph) { ucell hi, mid, lo; const char *arg = pop_fstr_copy(); @@ -287,7 +287,7 @@ ob_pci_decode_unit(int *idx) } free((char*)arg);
- bus = get_int_property(get_cur_dev(), "bus-range", &len);
bus = get_int_property(ph, "bus-range", &len);
hi = n | p | t | (ss << 24) | (bus << 16) | (dev << 11) | (fn << 8) | reg;
@@ -303,7 +303,7 @@ ob_pci_decode_unit(int *idx) /* ( phys.lo phy.mid phys.hi -- str len ) */
static void -ob_pci_encode_unit(int *idx) +ob_pci_encode_unit(ucell ph) { char buf[28]; cell hi = POP(); @@ -457,8 +457,6 @@ ob_pci_dma_sync(int *idx) NODE_METHODS(ob_pci_bus_node) = { { "open", ob_pci_open }, { "close", ob_pci_close },
- { "decode-unit", ob_pci_decode_unit },
- { "encode-unit", ob_pci_encode_unit }, { "pci-map-in", ob_pci_bus_map_in }, { "dma-alloc", ob_pci_dma_alloc }, { "dma-free", ob_pci_dma_free },
@@ -479,8 +477,6 @@ ob_pci_bridge_map_in(int *idx) NODE_METHODS(ob_pci_bridge_node) = { { "open", ob_pci_open }, { "close", ob_pci_close },
- { "decode-unit", ob_pci_decode_unit },
- { "encode-unit", ob_pci_encode_unit }, { "pci-map-in", ob_pci_bridge_map_in }, { "dma-alloc", ob_pci_dma_alloc }, { "dma-free", ob_pci_dma_free },
@@ -1834,6 +1830,9 @@ static phandle_t ob_configure_pci_device(const char* parent_path, } else { BIND_NODE_METHODS(phandle, ob_pci_bus_node); }
BIND_NODE_STATIC_METHOD(phandle, "encode-unit", ob_pci_encode_unit);BIND_NODE_STATIC_METHOD(phandle, "decode-unit", ob_pci_decode_unit); break;