This series provides a set of fixes required to allow Forth PCI config access words to work correctly on both 32-bit and 64-bit platforms, along with Zoltan's original v4 patch. There are 2 main fixes included:
- Implement and convert PCI encode-unit/decode-unit to static methods - Fix my-address and my-unit on 64-bit platforms
With this series applied it is possible to execute the Forth PCI config words as documented in various drivers as below:
PPC (32-bit):
0 > showstack ok 0 > " /pci/QEMU,VGA" open-dev to my-self ok 0 > my-space " config-w@" $call-parent u. 1234 ok 0 > my-space ok 800 < 1 > my-address ok 800 0 0 < 3 >
SPARC64 (64-bit, including across PCI bridge):
0 > showstack ok 0 > " /pci/pci@1,1/QEMU,VGA" open-dev to my-self ok 0 > my-space " config-w@" $call-parent u. 1234 ok 0 > my-space ok 11000 < 1 > my-address ok 11000 0 0 < 3 >
Signed-off-by: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk
BALATON Zoltan (1): drivers/pci: Implement config access words
Mark Cave-Ayland (5): libopenbios/clib.fs: add new is-2arg-cfunc for calling into C libopenbios: introduce BIND_NODE_STATIC_METHOD and supporting functions drivers/pci.c: convert encode-unit and decode-unit words to static methods package.fs: fix my-address on 64-bit systems package.fs: fix my-unit on 64-bit systems
drivers/pci.c | 115 +++++++++++++++++++++++++++++++-- forth/device/package.fs | 8 +-- include/libopenbios/bindings.h | 7 ++ libopenbios/bindings.c | 26 ++++++++ libopenbios/clib.fs | 10 +++ 5 files changed, 155 insertions(+), 11 deletions(-)
In order to implement static methods it is necessary to add a new Forth-C binding that allows 2 parameters to be passed to the C bindings.
Signed-off-by: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk --- libopenbios/clib.fs | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/libopenbios/clib.fs b/libopenbios/clib.fs index 04dd0aa..4f34c7b 100644 --- a/libopenbios/clib.fs +++ b/libopenbios/clib.fs @@ -34,3 +34,13 @@ swap ['] (lit) , , ['] (lit) , , ['] call , is-func-end ; + +\ is-2arg-cfunc installs a function with 2 arguments +: is-2arg-cfunc ( funcarg2 funcarg1 funcaddr word word-len -- ) + is-func-begin + rot ['] (lit) , , + swap ['] (lit) , , + ['] (lit) , , + ['] call , + is-func-end +;
This macro can be used to generating a C binding for a static method, passing the phandle into the target C function.
The key difference between this and BIND_NODE_METHODS is that the C parameter is a package variable rather than an instance variable which cannot exist in a static method context.
Signed-off-by: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk --- include/libopenbios/bindings.h | 7 +++++++ libopenbios/bindings.c | 26 ++++++++++++++++++++++++++ 2 files changed, 33 insertions(+)
diff --git a/include/libopenbios/bindings.h b/include/libopenbios/bindings.h index 6b0302d..9874c81 100644 --- a/include/libopenbios/bindings.h +++ b/include/libopenbios/bindings.h @@ -82,6 +82,7 @@ extern cell feval( const char *str ); extern void bind_xtfunc( const char *name, xt_t xt, ucell arg, void (*func)(void) ); extern void bind_func( const char *name, void (*func)(void) ); +extern void bind_2argfunc( const char *name, ucell arg2, ucell arg1, void (*func)(void) ); extern xt_t bind_noname_func( void (*func)(void) ); extern void push_str( const char *str ); extern char *pop_fstr_copy( void ); @@ -121,6 +122,10 @@ typedef struct { name##_m, sizeof(name##_m)/sizeof(method_t)); \ } while(0)
+#define BIND_NODE_STATIC_METHOD(ph, name, func) do { \ + bind_node_static_method(ph, name, ph, func); \ +} while(0) + #define DECLARE_UNNAMED_NODE( name, flags, size ) \ static const int name##_flags_ = flags; \ static const int name##_size_ = size; @@ -147,6 +152,8 @@ extern void bind_node( int flags, int size, const char * const *paths, int npat extern phandle_t bind_new_node( int flags, int size, const char *name, const method_t *methods, int nmethods );
+extern void bind_node_static_method( phandle_t ph, const char *name, ucell arg, void *func ); + #define INSTALL_OPEN 1 /* install trivial open and close methods */
diff --git a/libopenbios/bindings.c b/libopenbios/bindings.c index 9b4308b..2ef8d57 100644 --- a/libopenbios/bindings.c +++ b/libopenbios/bindings.c @@ -116,6 +116,16 @@ bind_xtfunc( const char *name, xt_t xt, ucell arg, void (*func)(void) ) fword("is-xt-cfunc"); }
+void +bind_2argfunc( const char *name, ucell arg2, ucell arg1, void (*func)(void) ) +{ + PUSH( arg2 ); + PUSH( arg1 ); + PUSH( pointer2cell(func) ); + push_str( name ); + fword("is-2arg-cfunc"); +} + xt_t bind_noname_func( void (*func)(void) ) { @@ -494,6 +504,22 @@ bind_node_methods(phandle_t ph, int flags, int size, const method_t *methods, in activate_dev( save_ph ); }
+static void +add_static_method(const char *name, ucell arg, void *func) +{ + bind_2argfunc( name, arg, pointer2cell(func), &call1_func ); +} + +void +bind_node_static_method(phandle_t ph, const char *name, ucell arg, void *func) +{ + phandle_t save_ph = get_cur_dev(); + + activate_dev(ph); + add_static_method(name, arg, func); + activate_dev( save_ph ); +} + void bind_node( int flags, int size, const char * const *paths, int npaths, const method_t *methods, int nmet )
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.
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.
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; }
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;
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.
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
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.
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.
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);
I still think this series should not need the first two patches and there must be a simpler way to do this so this is where I start from. I've checked what openfirmware and openboot does and their pcibus encode-unit and decode-unit are the same as these only in forth, except they don't mess with bus-range. That means we don't need to convert these to Forth (although that may still reduce the cultter in pci.c but since we already have these here they can stay) but this line should be removed and be done the same way as openboot does instead which seems to be the following. See:
https://github.com/openbios/openboot/blob/master/obp/dev/pci-bridge/pcinode....
The bridge has a different decode-unit method which calls this method in the parent then adds the bus number to that. So what I think this patch should do instead is to remove this line and copy those methods from openboot to pci.fs such as:
0 value my-bus# defer parent-decode-unit : decode-unit ( adr len -- phys.lo..hi ) parent-decode-unit lwsplit drop my-bus# wljoin ; defer encode-unit ( phys.lo..hi -- adr len ) \ decode-unit and encode-unit must be static methods, so they can't use \ $call-parent at run-time
" decode-unit" my-parent ihandle>phandle find-method drop ( xt ) to parent-decode-unit
" encode-unit" my-parent ihandle>phandle find-method drop ( xt ) to encode-unit
and use these as the bridge methods. I'm still not sure how to install these in the bridge node but maybe the above should be within an is-pci-brodge word which is called to define these when the bridge node is created passing the bus number to it. What do you thing about that?
Regards, BALATON Zoltan
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;
According to structures.fs the fields in struct instance are cell size which is 4 bytes on 32-bit systems and 8 bytes on 64-bit systems.
my-address incorrectly assumes that the fields are always "l" (4 byte) size which causes it to return the wrong value on SPARC64. Fix the issue by updating my-address to use the cell size to calculate its offsets.
Signed-off-by: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk --- forth/device/package.fs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/forth/device/package.fs b/forth/device/package.fs index f83ef7a..e0fd2d2 100644 --- a/forth/device/package.fs +++ b/forth/device/package.fs @@ -183,9 +183,9 @@ defer find-dev : my-address ( -- phys.lo ... ) ?my-self >in.device-node @
dn.probe-addr
- my-#acells tuck /l* + swap 1- 0 + my-#acells tuck /n* + swap 1- 0 ?do - /l - dup l@ swap + /n - dup @ swap loop drop ;
According to structures.fs the fields in struct instance are cell size which is 4 bytes on 32-bit systems and 8 bytes on 64-bit systems.
my-unit incorrectly assumes that the fields are always "l" (4 byte) size which causes it to return the wrong value on SPARC64. Fix the issue by updating my-unit to use the cell size to calculate its offsets.
Signed-off-by: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk --- forth/device/package.fs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/forth/device/package.fs b/forth/device/package.fs index e0fd2d2..8a28263 100644 --- a/forth/device/package.fs +++ b/forth/device/package.fs @@ -197,8 +197,8 @@ defer find-dev
: my-unit ( -- phys.lo ... phys.hi ) ?my-self >in.my-unit - my-#acells tuck /l* + swap 0 ?do - /l - dup l@ swap + my-#acells tuck /n* + swap 0 ?do + /n - dup @ swap loop drop ;
On Sat, 11 Mar 2023, Mark Cave-Ayland wrote:
According to structures.fs the fields in struct instance are cell size which is 4 bytes on 32-bit systems and 8 bytes on 64-bit systems.
my-unit incorrectly assumes that the fields are always "l" (4 byte) size which causes it to return the wrong value on SPARC64. Fix the issue by updating my-unit to use the cell size to calculate its offsets.
This and the previous patch doing the same look good to me but I don't know enough about it to give a reviewed-by. That would be better done by somebody who knows Forth well. So this message does not help much I guess.
Regards, BALATON Zoltan
Signed-off-by: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk
forth/device/package.fs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/forth/device/package.fs b/forth/device/package.fs index e0fd2d2..8a28263 100644 --- a/forth/device/package.fs +++ b/forth/device/package.fs @@ -197,8 +197,8 @@ defer find-dev
: my-unit ( -- phys.lo ... phys.hi ) ?my-self >in.my-unit
- my-#acells tuck /l* + swap 0 ?do
- /l - dup l@ swap
- my-#acells tuck /n* + swap 0 ?do
- /n - dup @ swap loop drop ;
On 12/03/2023 15:23, BALATON Zoltan wrote:
On Sat, 11 Mar 2023, Mark Cave-Ayland wrote:
According to structures.fs the fields in struct instance are cell size which is 4 bytes on 32-bit systems and 8 bytes on 64-bit systems.
my-unit incorrectly assumes that the fields are always "l" (4 byte) size which causes it to return the wrong value on SPARC64. Fix the issue by updating my-unit to use the cell size to calculate its offsets.
This and the previous patch doing the same look good to me but I don't know enough about it to give a reviewed-by. That would be better done by somebody who knows Forth well. So this message does not help much I guess.
Regards, BALATON Zoltan
The change here is to reflect that the device node (phandle) structure in forth/device/structures.fs is defined in terms of cells (/n) rather than 4-byte units (/l), which isn't correct for 64-bit systems which use 8-byte cells.
Certainly more Forth reviews would be welcome, however it's quite straightforward in this case to validate that my-space, my-address and my-unit are now working as documented for the PCI devices.
ATB,
Mark.
On 11/03/2023 19:05, Mark Cave-Ayland wrote:
According to structures.fs the fields in struct instance are cell size which is 4 bytes on 32-bit systems and 8 bytes on 64-bit systems.
my-unit incorrectly assumes that the fields are always "l" (4 byte) size which causes it to return the wrong value on SPARC64. Fix the issue by updating my-unit to use the cell size to calculate its offsets.
Signed-off-by: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk
forth/device/package.fs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/forth/device/package.fs b/forth/device/package.fs index e0fd2d2..8a28263 100644 --- a/forth/device/package.fs +++ b/forth/device/package.fs @@ -197,8 +197,8 @@ defer find-dev
: my-unit ( -- phys.lo ... phys.hi ) ?my-self >in.my-unit
- my-#acells tuck /l* + swap 0 ?do
- /l - dup l@ swap
- my-#acells tuck /n* + swap 0 ?do
- /n - dup @ swap loop drop ;
During final testing of my OpenBIOS queue it seems this patch causes path resolution to generate incorrect device paths for some SPARC64 devices. I suspect it will take me some time to debug this, so I'll postpone pushing my queue to Github until I've posted a fixed version (likely after the QEMU 9.2 development cycle opens).
ATB,
Mark.
From: BALATON Zoltan balaton@eik.bme.hu
Signed-off-by: BALATON Zoltan balaton@eik.bme.hu Signed-off-by: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk --- drivers/pci.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 102 insertions(+)
diff --git a/drivers/pci.c b/drivers/pci.c index 6c1ce66..52368ae 100644 --- a/drivers/pci.c +++ b/drivers/pci.c @@ -424,6 +424,60 @@ ob_pci_bus_map_in(int *idx) PUSH(virt); }
+static void +ob_pci_config_read8(int *idx) +{ + cell hi = POP(); + 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); +} + +static void +ob_pci_config_write8(int *idx) +{ + cell hi = POP(); + pci_addr addr = PCI_ADDR(PCI_BUS(hi), PCI_DEV(hi), PCI_FN(hi)); + cell val = POP(); + pci_config_write8(addr, hi & 0xff, val); +} + +static void +ob_pci_config_read16(int *idx) +{ + cell hi = POP(); + pci_addr addr = PCI_ADDR(PCI_BUS(hi), PCI_DEV(hi), PCI_FN(hi)); + uint16_t val = pci_config_read16(addr, hi & 0xff); + PUSH(val); +} + +static void +ob_pci_config_write16(int *idx) +{ + cell hi = POP(); + pci_addr addr = PCI_ADDR(PCI_BUS(hi), PCI_DEV(hi), PCI_FN(hi)); + cell val = POP(); + pci_config_write16(addr, hi & 0xff, val); +} + +static void +ob_pci_config_read32(int *idx) +{ + cell hi = POP(); + pci_addr addr = PCI_ADDR(PCI_BUS(hi), PCI_DEV(hi), PCI_FN(hi)); + uint32_t val = pci_config_read32(addr, hi & 0xff); + PUSH(val); +} + +static void +ob_pci_config_write32(int *idx) +{ + cell hi = POP(); + pci_addr addr = PCI_ADDR(PCI_BUS(hi), PCI_DEV(hi), PCI_FN(hi)); + cell val = POP(); + pci_config_write32(addr, hi & 0xff, val); +} + static void ob_pci_dma_alloc(int *idx) { @@ -458,6 +512,12 @@ NODE_METHODS(ob_pci_bus_node) = { { "open", ob_pci_open }, { "close", ob_pci_close }, { "pci-map-in", ob_pci_bus_map_in }, + { "config-b@", ob_pci_config_read8 }, + { "config-b!", ob_pci_config_write8 }, + { "config-w@", ob_pci_config_read16 }, + { "config-w!", ob_pci_config_write16 }, + { "config-l@", ob_pci_config_read32 }, + { "config-l!", ob_pci_config_write32 }, { "dma-alloc", ob_pci_dma_alloc }, { "dma-free", ob_pci_dma_free }, { "dma-map-in", ob_pci_dma_map_in }, @@ -474,10 +534,52 @@ ob_pci_bridge_map_in(int *idx) call_parent_method("pci-map-in"); }
+static void +ob_pci_bridge_cread8(void) +{ + call_parent_method("config-b@"); +} + +static void +ob_pci_bridge_cwrite8(void) +{ + call_parent_method("config-b!"); +} + +static void +ob_pci_bridge_cread16(void) +{ + call_parent_method("config-w@"); +} + +static void +ob_pci_bridge_cwrite16(void) +{ + call_parent_method("config-w!"); +} + +static void +ob_pci_bridge_cread32(void) +{ + call_parent_method("config-l@"); +} + +static void +ob_pci_bridge_cwrite32(void) +{ + call_parent_method("config-l!"); +} + NODE_METHODS(ob_pci_bridge_node) = { { "open", ob_pci_open }, { "close", ob_pci_close }, { "pci-map-in", ob_pci_bridge_map_in }, + { "config-b@", ob_pci_bridge_cread8 }, + { "config-b!", ob_pci_bridge_cwrite8 }, + { "config-w@", ob_pci_bridge_cread16 }, + { "config-w!", ob_pci_bridge_cwrite16 }, + { "config-l@", ob_pci_bridge_cread32 }, + { "config-l!", ob_pci_bridge_cwrite32 }, { "dma-alloc", ob_pci_dma_alloc }, { "dma-free", ob_pci_dma_free }, { "dma-map-in", ob_pci_dma_map_in },
On 11/03/2023 19:05, Mark Cave-Ayland wrote:
This series provides a set of fixes required to allow Forth PCI config access words to work correctly on both 32-bit and 64-bit platforms, along with Zoltan's original v4 patch. There are 2 main fixes included:
- Implement and convert PCI encode-unit/decode-unit to static methods
- Fix my-address and my-unit on 64-bit platforms
With this series applied it is possible to execute the Forth PCI config words as documented in various drivers as below:
PPC (32-bit):
0 > showstack ok 0 > " /pci/QEMU,VGA" open-dev to my-self ok 0 > my-space " config-w@" $call-parent u. 1234 ok 0 > my-space ok 800 < 1 > my-address ok 800 0 0 < 3 >
SPARC64 (64-bit, including across PCI bridge):
0 > showstack ok 0 > " /pci/pci@1,1/QEMU,VGA" open-dev to my-self ok 0 > my-space " config-w@" $call-parent u. 1234 ok 0 > my-space ok 11000 < 1 > my-address ok 11000 0 0 < 3 >
Signed-off-by: Mark Cave-Ayland mark.cave-ayland@ilande.co.uk
BALATON Zoltan (1): drivers/pci: Implement config access words
Mark Cave-Ayland (5): libopenbios/clib.fs: add new is-2arg-cfunc for calling into C libopenbios: introduce BIND_NODE_STATIC_METHOD and supporting functions drivers/pci.c: convert encode-unit and decode-unit words to static methods package.fs: fix my-address on 64-bit systems package.fs: fix my-unit on 64-bit systems
drivers/pci.c | 115 +++++++++++++++++++++++++++++++-- forth/device/package.fs | 8 +-- include/libopenbios/bindings.h | 7 ++ libopenbios/bindings.c | 26 ++++++++ libopenbios/clib.fs | 10 +++ 5 files changed, 155 insertions(+), 11 deletions(-)
I think this series has been waiting long enough: re-reading through the earlier threads, this is still my preferred method so I'll queue it for master.
ATB,
Mark.
-
BALATON Zoltan -
Mark Cave-Ayland