On 12/03/2023 15:21, BALATON Zoltan wrote:
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?
I'm not sure what you're suggesting here? encode-unit and decode-unit are PCI bus methods rather than PCI device methods.
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.
There could be some argument for this if the majority of the OpenBIOS PCI bindings were already written in Forth, but they are entirely written in C. So as per your opinion above that mixing Forth and C can be complicated, that would be exactly what you would end up doing by switching the encode-unit and decode-unit methods to Forth.
Certainly from my perspective it seems that adding a small amount of C glue similar to as already exists for instances is better than either i) embedding large amounts of Forth with feval() or ii) attempting a full conversion of OpenBIOS's PCI bindings from C to Forth.
ATB,
Mark.