On Mon, 13 Mar 2023, Mark Cave-Ayland wrote:
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.
What I've posted above are examples of pci-bus and pci-bridge impelementations of these functions. I meant these as an examole to show how these could be implemented in forth.
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.
Not at all, because these decode-unit and encode-unit methods don't use anthing from the rest of pci.c nor the rest of that file uses these other than adding them as methods to the device node. So there's no good reason for these to be in pci.c other than they were already there. These only do string manupulation for which C might not be the best tool. These functions are long and boring and could be done simpler within forth. That would also avoid having to pass bus number to C so we could simplify this series dropping the first two patches adding the new C wrapper and just replace these funcitons with forth words in pci.fs so pci.c would also become less cluttered. I think that's worth the effort adapting some forth method from above examples or rewriting these based on those exapmles. Doing things in C makes sense when it's simpler to do that way but when it's not it's simpler to do things in 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.
I have suggested neither of the above though. All I said was to do ob_pci_encode_unit and ob_pci_decode_unit in forth moving them to pci.fs and that way we end up with simpler code not touching underlying forth-C bridge which seems like too much effort for something that can be done simpler and this series makes things more complex instead of making them simpler.
Is there anybody here who could come up with forth implementation of these methods (the functions ob_pci_encode_unit and ob_pci_decode_unit in openbiosdrivers/pci.c) so we can test the idea? I could try it based on the above examples but maybe it would take less time for somebody more fluent in forth.
Regards, BALATON Zoltan