| Message ID | 20200129022316.129492-1-aik@ozlabs.ru |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fdt: Fix creating new nodes at H_CAS | expand |
On Wed, Jan 29, 2020 at 01:23:16PM +1100, Alexey Kardashevskiy wrote: > So far we only allowed new ibm,dynamic-reconfiguration-memory and memory > nodes in the FDT update blob at ibm,client-architecture-support (CAS). > DRC do not have unit addresses and are easy, for memory nodee we use > an address from the node name. > > For early hot plugged PCI devices (plugged after reset but before CAS) > we have to have a similar hack as for memory@ but parse the address > differently because of different binding. > > Instead, this changes new nodes creation. At pass#0 when we copy phandles > from the FDT update blob to SLOF, we create new nodes with all > new properties and call "finish-device" only after all properties are > copied to the new nodes. At this point we particularly care about "reg" > as this is the unit address which SLOF parses for us and sets the unit > address in "finish-device"; we could skip other properties for later > passes. > > Note this creates naked nodes with no methods normally added to the nodes > as this bypasses normal discovery which SLOF performs at start. So > if pass#1 does not find the node created in pass#0, this points to > missing "decode-unit" at the new node parent (happens when adding bridge- > under-bridge) and this prints a message and resets. > > While at this, fix few trailing spaces and comments. It's a bit of a hack, but it makes things better than they were, so I like the idea. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > board-qemu/slof/fdt.fs | 100 ++++++++++++++++++++++------------------- > 1 file changed, 54 insertions(+), 46 deletions(-) > > diff --git a/board-qemu/slof/fdt.fs b/board-qemu/slof/fdt.fs > index fc39a82a10b5..eb726cc990ae 100644 > --- a/board-qemu/slof/fdt.fs > +++ b/board-qemu/slof/fdt.fs > @@ -112,7 +112,7 @@ fdt-check-header > ; > > \ Lookup a string by index > -: fdt-fetch-string ( index -- str-addr str-len ) > +: fdt-fetch-string ( index -- str-addr str-len ) > fdt-strings + dup from-cstring > ; > > @@ -128,7 +128,7 @@ fdt-check-header > ; > > \ Encode fdt property to OF property > -: fdt-encode-prop ( addr len -- ) > +: fdt-encode-prop ( addr len -- pa ps ) > 2dup fdt-prop-is-string? IF > 1- encode-string > ELSE > @@ -443,33 +443,6 @@ r> drop > device-end > ; > > -: fdt-create-cas-node ( name -- ) > - 2dup > - 2dup " memory@" find-substr 0 = IF > - fdt-debug IF ." Creating memory@ " cr THEN > - new-device > - 2dup " @" find-substr nip device-name \ Parse the node name > - 2dup > - 2dup " @" find-substr rot over + 1 + -rot - 1 - \ Jump to addr afte "@" > - parse-2int nip xlsplit set-unit \ Parse and set unit > - finish-device > - ELSE > - 2dup " ibm,dynamic-reconfiguration-memory" find-substr 0 = IF > - fdt-debug IF ." Creating ibm,dynamic-reconfiguration-memory " cr THEN > - new-device > - device-name > - finish-device > - ELSE > - 2drop 2drop > - false to fdt-cas-fix? > - ." Node not supported " cr > - EXIT > - THEN > - THEN > - > - find-node ?dup 0 <> IF set-node THEN > -; > - > : str=phandle? ( s len -- true|false ) > 2dup s" phandle" str= >r > s" linux,phandle" str= > @@ -483,7 +456,7 @@ r> drop > false to fdt-cas-fix? > EXIT > THEN drop > - fdt-fetch-unit > + fdt-fetch-unit ( a1 $name ) > dup 0 = IF drop drop " /" THEN > 40 left-parse-string > 2swap ?dup 0 <> IF > @@ -492,29 +465,52 @@ r> drop > ELSE > drop > THEN > - fdt-debug IF ." Setting node: " 2dup type cr THEN > 2dup find-node ?dup 0 <> IF > - set-node 2drop > + set-node > + fdt-debug IF ." Setting node: " 2dup type cr THEN > + 2drop > + \ newnode?=0: updating the existing node, i.e. pass1 adds only phandles > + 0 > ELSE > + fdt-cas-pass 0 <> IF > + \ We could not find the node added in the previous pass, > + \ most likely because it is hotplug-under-hotplug case > + \ (such as PCI brigde under bridge) when missing new node methods > + \ such as "decode-unit" are critical. > + \ Reboot when detect such case which is expected as it is a part of > + \ ibm,client-architecture-support. > + ." Cannot handle FDT update for the " 2dup type > + ." node, rebooting" cr > + reset-all > + THEN > fdt-debug IF ." Creating node: " 2dup type cr THEN > - fdt-create-cas-node > + new-device > + 2dup " @" find-substr nip > + device-name > + \ newnode?=1: adding new node, i.e. pass1 adds all properties, > + \ most importantly "reg". After reading properties, we call > + \ "finish-device" which sets the unit address from "reg". > + 1 > THEN > - fdt-debug IF ." Current now: " pwd cr THEN > + swap ( newnode? a1 ) > + > + fdt-debug IF ." Current now: " pwd get-node ." = " . cr THEN > BEGIN > fdt-next-tag dup OF_DT_END_NODE <> > WHILE > + ( newnode? a1 tag ) > dup OF_DT_PROP = IF > - drop dup ( drop tag, dup addr : a1 a1 ) > - dup l@ dup rot 4 + ( fetch size, stack is : a1 s s a2) > - dup l@ swap 4 + ( fetch nameid, stack is : a1 s s i a3 ) > - rot ( we now have: a1 s i a3 s ) > - fdt-encode-prop rot ( a1 s pa ps i) > - fdt-fetch-string ( a1 s pa ps na ns ) > + drop dup ( newnode? a1 a1 ) > + dup l@ dup rot 4 + ( newnode? a1 s s a2) > + dup l@ swap 4 + ( newnode? a1 s s i a3 ) > + rot ( newnode? a1 s i a3 s ) > + fdt-encode-prop rot ( newnode? a1 s pa ps i) > + fdt-fetch-string ( newnode? a1 s pa ps na ns ) > > fdt-cas-pass CASE > 0 OF > - 2dup str=phandle? IF > - fdt-debug IF 4dup ." Phandle: " type ." =" swap ." @" . ." " .d ." bytes" cr THEN > + 2dup str=phandle? 7 pick or IF > + fdt-debug IF 4dup ." Property: " type ." =" swap ." @" . ." " .d ." bytes" cr THEN > property > ELSE > 4drop > @@ -541,8 +537,14 @@ r> drop > ENDCASE > > + 8 + 3 + fffffffc and > - ELSE dup OF_DT_BEGIN_NODE = IF > - drop ( drop tag ) > + ELSE ( newnode? a1 tag ) > + dup OF_DT_BEGIN_NODE = IF > + 2 pick IF > + rot drop 0 -rot > + get-node finish-device set-node > + fdt-debug IF ." Finished node: " pwd get-node ." = " . cr THEN > + THEN > + drop ( a1 ) > 4 - > (fdt-fix-cas-node) > get-parent set-node > @@ -554,13 +556,19 @@ r> drop > THEN > THEN > REPEAT > - drop \ drop tag > + ( newnode? a1 tag ) > + drop > + swap ( a1 newnode? ) > + IF > + get-node finish-device set-node > + fdt-debug IF ." Finished subnode: " pwd get-node ." = " . cr THEN > + THEN > ; > > : fdt-fix-cas-node ( start -- ) > 0 to fdt-cas-pass dup (fdt-fix-cas-node) drop \ Add phandles > 1 to fdt-cas-pass dup (fdt-fix-cas-node) drop \ Patch+add other properties > - 2 to fdt-cas-pass dup (fdt-fix-cas-node) drop \ Delete phandles from pass 1 > + 2 to fdt-cas-pass dup (fdt-fix-cas-node) drop \ Delete phandles from pass 0 > drop > ; >
On Wed, 29 Jan 2020 13:23:16 +1100 Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > So far we only allowed new ibm,dynamic-reconfiguration-memory and memory > nodes in the FDT update blob at ibm,client-architecture-support (CAS). > DRC do not have unit addresses and are easy, for memory nodee we use > an address from the node name. > > For early hot plugged PCI devices (plugged after reset but before CAS) > we have to have a similar hack as for memory@ but parse the address > differently because of different binding. > > Instead, this changes new nodes creation. At pass#0 when we copy phandles > from the FDT update blob to SLOF, we create new nodes with all > new properties and call "finish-device" only after all properties are > copied to the new nodes. At this point we particularly care about "reg" > as this is the unit address which SLOF parses for us and sets the unit > address in "finish-device"; we could skip other properties for later "finish-device" only sets the first entry of the "reg" property as a fallback. This will be an issue when we start seeing new nodes with bigger unit values (eg, a new PHB). The setting of the unit from "reg" is actually handled by "fdt-unflatten-node" which calls "fdt-reg-unit": 2dup s" reg" str= IF 2swap 2dup fdt-reg-unit 2swap THEN Something similar could be done... > passes. > > Note this creates naked nodes with no methods normally added to the nodes > as this bypasses normal discovery which SLOF performs at start. So > if pass#1 does not find the node created in pass#0, this points to > missing "decode-unit" at the new node parent (happens when adding bridge- > under-bridge) and this prints a message and resets. > > While at this, fix few trailing spaces and comments. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > board-qemu/slof/fdt.fs | 100 ++++++++++++++++++++++------------------- > 1 file changed, 54 insertions(+), 46 deletions(-) > > diff --git a/board-qemu/slof/fdt.fs b/board-qemu/slof/fdt.fs > index fc39a82a10b5..eb726cc990ae 100644 > --- a/board-qemu/slof/fdt.fs > +++ b/board-qemu/slof/fdt.fs > @@ -112,7 +112,7 @@ fdt-check-header > ; > > \ Lookup a string by index > -: fdt-fetch-string ( index -- str-addr str-len ) > +: fdt-fetch-string ( index -- str-addr str-len ) > fdt-strings + dup from-cstring > ; > > @@ -128,7 +128,7 @@ fdt-check-header > ; > > \ Encode fdt property to OF property > -: fdt-encode-prop ( addr len -- ) > +: fdt-encode-prop ( addr len -- pa ps ) > 2dup fdt-prop-is-string? IF > 1- encode-string > ELSE > @@ -443,33 +443,6 @@ r> drop > device-end > ; > > -: fdt-create-cas-node ( name -- ) > - 2dup > - 2dup " memory@" find-substr 0 = IF > - fdt-debug IF ." Creating memory@ " cr THEN > - new-device > - 2dup " @" find-substr nip device-name \ Parse the node name > - 2dup > - 2dup " @" find-substr rot over + 1 + -rot - 1 - \ Jump to addr afte "@" > - parse-2int nip xlsplit set-unit \ Parse and set unit > - finish-device > - ELSE > - 2dup " ibm,dynamic-reconfiguration-memory" find-substr 0 = IF > - fdt-debug IF ." Creating ibm,dynamic-reconfiguration-memory " cr THEN > - new-device > - device-name > - finish-device > - ELSE > - 2drop 2drop > - false to fdt-cas-fix? > - ." Node not supported " cr > - EXIT > - THEN > - THEN > - > - find-node ?dup 0 <> IF set-node THEN > -; > - > : str=phandle? ( s len -- true|false ) > 2dup s" phandle" str= >r > s" linux,phandle" str= > @@ -483,7 +456,7 @@ r> drop > false to fdt-cas-fix? > EXIT > THEN drop > - fdt-fetch-unit > + fdt-fetch-unit ( a1 $name ) > dup 0 = IF drop drop " /" THEN > 40 left-parse-string > 2swap ?dup 0 <> IF > @@ -492,29 +465,52 @@ r> drop > ELSE > drop > THEN > - fdt-debug IF ." Setting node: " 2dup type cr THEN > 2dup find-node ?dup 0 <> IF > - set-node 2drop > + set-node > + fdt-debug IF ." Setting node: " 2dup type cr THEN > + 2drop > + \ newnode?=0: updating the existing node, i.e. pass1 adds only phandles > + 0 > ELSE > + fdt-cas-pass 0 <> IF > + \ We could not find the node added in the previous pass, > + \ most likely because it is hotplug-under-hotplug case > + \ (such as PCI brigde under bridge) when missing new node methods > + \ such as "decode-unit" are critical. > + \ Reboot when detect such case which is expected as it is a part of > + \ ibm,client-architecture-support. > + ." Cannot handle FDT update for the " 2dup type > + ." node, rebooting" cr > + reset-all > + THEN > fdt-debug IF ." Creating node: " 2dup type cr THEN > - fdt-create-cas-node > + new-device > + 2dup " @" find-substr nip > + device-name > + \ newnode?=1: adding new node, i.e. pass1 adds all properties, > + \ most importantly "reg". After reading properties, we call > + \ "finish-device" which sets the unit address from "reg". > + 1 > THEN > - fdt-debug IF ." Current now: " pwd cr THEN > + swap ( newnode? a1 ) > + > + fdt-debug IF ." Current now: " pwd get-node ." = " . cr THEN > BEGIN > fdt-next-tag dup OF_DT_END_NODE <> > WHILE > + ( newnode? a1 tag ) > dup OF_DT_PROP = IF > - drop dup ( drop tag, dup addr : a1 a1 ) > - dup l@ dup rot 4 + ( fetch size, stack is : a1 s s a2) > - dup l@ swap 4 + ( fetch nameid, stack is : a1 s s i a3 ) > - rot ( we now have: a1 s i a3 s ) > - fdt-encode-prop rot ( a1 s pa ps i) > - fdt-fetch-string ( a1 s pa ps na ns ) > + drop dup ( newnode? a1 a1 ) > + dup l@ dup rot 4 + ( newnode? a1 s s a2) > + dup l@ swap 4 + ( newnode? a1 s s i a3 ) > + rot ( newnode? a1 s i a3 s ) > + fdt-encode-prop rot ( newnode? a1 s pa ps i) > + fdt-fetch-string ( newnode? a1 s pa ps na ns ) > > fdt-cas-pass CASE > 0 OF > - 2dup str=phandle? IF > - fdt-debug IF 4dup ." Phandle: " type ." =" swap ." @" . ." " .d ." bytes" cr THEN > + 2dup str=phandle? 7 pick or IF > + fdt-debug IF 4dup ." Property: " type ." =" swap ." @" . ." " .d ." bytes" cr THEN > property > ELSE > 4drop > @@ -541,8 +537,14 @@ r> drop > ENDCASE > > + 8 + 3 + fffffffc and > - ELSE dup OF_DT_BEGIN_NODE = IF > - drop ( drop tag ) > + ELSE ( newnode? a1 tag ) > + dup OF_DT_BEGIN_NODE = IF > + 2 pick IF > + rot drop 0 -rot > + get-node finish-device set-node > + fdt-debug IF ." Finished node: " pwd get-node ." = " . cr THEN > + THEN > + drop ( a1 ) > 4 - > (fdt-fix-cas-node) > get-parent set-node > @@ -554,13 +556,19 @@ r> drop > THEN > THEN > REPEAT > - drop \ drop tag > + ( newnode? a1 tag ) > + drop > + swap ( a1 newnode? ) > + IF ... here. " reg" get-node get-package-property IF ELSE fdt-reg-unit THEN This doesn't address the case of a hotplug-over-hotplug since the new parent node doesn't have a "decode-unit" method, but at least it covers all cases where the parent node was created at boot time. > + get-node finish-device set-node > + fdt-debug IF ." Finished subnode: " pwd get-node ." = " . cr THEN > + THEN > ; > > : fdt-fix-cas-node ( start -- ) > 0 to fdt-cas-pass dup (fdt-fix-cas-node) drop \ Add phandles > 1 to fdt-cas-pass dup (fdt-fix-cas-node) drop \ Patch+add other properties > - 2 to fdt-cas-pass dup (fdt-fix-cas-node) drop \ Delete phandles from pass 1 > + 2 to fdt-cas-pass dup (fdt-fix-cas-node) drop \ Delete phandles from pass 0 > drop > ; >
On 29/01/2020 22:33, Greg Kurz wrote: > On Wed, 29 Jan 2020 13:23:16 +1100 > Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > >> So far we only allowed new ibm,dynamic-reconfiguration-memory and memory >> nodes in the FDT update blob at ibm,client-architecture-support (CAS). >> DRC do not have unit addresses and are easy, for memory nodee we use >> an address from the node name. >> >> For early hot plugged PCI devices (plugged after reset but before CAS) >> we have to have a similar hack as for memory@ but parse the address >> differently because of different binding. >> >> Instead, this changes new nodes creation. At pass#0 when we copy phandles >> from the FDT update blob to SLOF, we create new nodes with all >> new properties and call "finish-device" only after all properties are >> copied to the new nodes. At this point we particularly care about "reg" >> as this is the unit address which SLOF parses for us and sets the unit >> address in "finish-device"; we could skip other properties for later > > "finish-device" only sets the first entry of the "reg" property as a > fallback. This will be an issue when we start seeing new nodes with > bigger unit values (eg, a new PHB). > > The setting of the unit from "reg" is actually handled by > "fdt-unflatten-node" which calls "fdt-reg-unit": > > 2dup s" reg" str= IF > 2swap 2dup fdt-reg-unit 2swap > THEN > > Something similar could be done... Huh. Should not we then fix "finish-device"? > >> passes. >> >> Note this creates naked nodes with no methods normally added to the nodes >> as this bypasses normal discovery which SLOF performs at start. So >> if pass#1 does not find the node created in pass#0, this points to >> missing "decode-unit" at the new node parent (happens when adding bridge- >> under-bridge) and this prints a message and resets. >> >> While at this, fix few trailing spaces and comments. >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> --- >> board-qemu/slof/fdt.fs | 100 ++++++++++++++++++++++------------------- >> 1 file changed, 54 insertions(+), 46 deletions(-) >> >> diff --git a/board-qemu/slof/fdt.fs b/board-qemu/slof/fdt.fs >> index fc39a82a10b5..eb726cc990ae 100644 >> --- a/board-qemu/slof/fdt.fs >> +++ b/board-qemu/slof/fdt.fs >> @@ -112,7 +112,7 @@ fdt-check-header >> ; >> >> \ Lookup a string by index >> -: fdt-fetch-string ( index -- str-addr str-len ) >> +: fdt-fetch-string ( index -- str-addr str-len ) >> fdt-strings + dup from-cstring >> ; >> >> @@ -128,7 +128,7 @@ fdt-check-header >> ; >> >> \ Encode fdt property to OF property >> -: fdt-encode-prop ( addr len -- ) >> +: fdt-encode-prop ( addr len -- pa ps ) >> 2dup fdt-prop-is-string? IF >> 1- encode-string >> ELSE >> @@ -443,33 +443,6 @@ r> drop >> device-end >> ; >> >> -: fdt-create-cas-node ( name -- ) >> - 2dup >> - 2dup " memory@" find-substr 0 = IF >> - fdt-debug IF ." Creating memory@ " cr THEN >> - new-device >> - 2dup " @" find-substr nip device-name \ Parse the node name >> - 2dup >> - 2dup " @" find-substr rot over + 1 + -rot - 1 - \ Jump to addr afte "@" >> - parse-2int nip xlsplit set-unit \ Parse and set unit >> - finish-device >> - ELSE >> - 2dup " ibm,dynamic-reconfiguration-memory" find-substr 0 = IF >> - fdt-debug IF ." Creating ibm,dynamic-reconfiguration-memory " cr THEN >> - new-device >> - device-name >> - finish-device >> - ELSE >> - 2drop 2drop >> - false to fdt-cas-fix? >> - ." Node not supported " cr >> - EXIT >> - THEN >> - THEN >> - >> - find-node ?dup 0 <> IF set-node THEN >> -; >> - >> : str=phandle? ( s len -- true|false ) >> 2dup s" phandle" str= >r >> s" linux,phandle" str= >> @@ -483,7 +456,7 @@ r> drop >> false to fdt-cas-fix? >> EXIT >> THEN drop >> - fdt-fetch-unit >> + fdt-fetch-unit ( a1 $name ) >> dup 0 = IF drop drop " /" THEN >> 40 left-parse-string >> 2swap ?dup 0 <> IF >> @@ -492,29 +465,52 @@ r> drop >> ELSE >> drop >> THEN >> - fdt-debug IF ." Setting node: " 2dup type cr THEN >> 2dup find-node ?dup 0 <> IF >> - set-node 2drop >> + set-node >> + fdt-debug IF ." Setting node: " 2dup type cr THEN >> + 2drop >> + \ newnode?=0: updating the existing node, i.e. pass1 adds only phandles >> + 0 >> ELSE >> + fdt-cas-pass 0 <> IF >> + \ We could not find the node added in the previous pass, >> + \ most likely because it is hotplug-under-hotplug case >> + \ (such as PCI brigde under bridge) when missing new node methods >> + \ such as "decode-unit" are critical. >> + \ Reboot when detect such case which is expected as it is a part of >> + \ ibm,client-architecture-support. >> + ." Cannot handle FDT update for the " 2dup type >> + ." node, rebooting" cr >> + reset-all >> + THEN >> fdt-debug IF ." Creating node: " 2dup type cr THEN >> - fdt-create-cas-node >> + new-device >> + 2dup " @" find-substr nip >> + device-name >> + \ newnode?=1: adding new node, i.e. pass1 adds all properties, >> + \ most importantly "reg". After reading properties, we call >> + \ "finish-device" which sets the unit address from "reg". >> + 1 >> THEN >> - fdt-debug IF ." Current now: " pwd cr THEN >> + swap ( newnode? a1 ) >> + >> + fdt-debug IF ." Current now: " pwd get-node ." = " . cr THEN >> BEGIN >> fdt-next-tag dup OF_DT_END_NODE <> >> WHILE >> + ( newnode? a1 tag ) >> dup OF_DT_PROP = IF >> - drop dup ( drop tag, dup addr : a1 a1 ) >> - dup l@ dup rot 4 + ( fetch size, stack is : a1 s s a2) >> - dup l@ swap 4 + ( fetch nameid, stack is : a1 s s i a3 ) >> - rot ( we now have: a1 s i a3 s ) >> - fdt-encode-prop rot ( a1 s pa ps i) >> - fdt-fetch-string ( a1 s pa ps na ns ) >> + drop dup ( newnode? a1 a1 ) >> + dup l@ dup rot 4 + ( newnode? a1 s s a2) >> + dup l@ swap 4 + ( newnode? a1 s s i a3 ) >> + rot ( newnode? a1 s i a3 s ) >> + fdt-encode-prop rot ( newnode? a1 s pa ps i) >> + fdt-fetch-string ( newnode? a1 s pa ps na ns ) >> >> fdt-cas-pass CASE >> 0 OF >> - 2dup str=phandle? IF >> - fdt-debug IF 4dup ." Phandle: " type ." =" swap ." @" . ." " .d ." bytes" cr THEN >> + 2dup str=phandle? 7 pick or IF >> + fdt-debug IF 4dup ." Property: " type ." =" swap ." @" . ." " .d ." bytes" cr THEN >> property >> ELSE >> 4drop >> @@ -541,8 +537,14 @@ r> drop >> ENDCASE >> >> + 8 + 3 + fffffffc and >> - ELSE dup OF_DT_BEGIN_NODE = IF >> - drop ( drop tag ) >> + ELSE ( newnode? a1 tag ) >> + dup OF_DT_BEGIN_NODE = IF >> + 2 pick IF >> + rot drop 0 -rot The proposed change from below is also needed here then, or since it has grown enough, this can make a separate function then. Can you please take over this and finish the change (merge yours 1/2 into this one and I will comment on 2/2)? Thanks, >> + get-node finish-device set-node >> + fdt-debug IF ." Finished node: " pwd get-node ." = " . cr THEN >> + THEN >> + drop ( a1 ) >> 4 - >> (fdt-fix-cas-node) >> get-parent set-node >> @@ -554,13 +556,19 @@ r> drop >> THEN >> THEN >> REPEAT >> - drop \ drop tag >> + ( newnode? a1 tag ) >> + drop >> + swap ( a1 newnode? ) >> + IF > > ... here. > > " reg" get-node get-package-property IF ELSE fdt-reg-unit THEN > > This doesn't address the case of a hotplug-over-hotplug since the > new parent node doesn't have a "decode-unit" method, but at least > it covers all cases where the parent node was created at boot time. > >> + get-node finish-device set-node >> + fdt-debug IF ." Finished subnode: " pwd get-node ." = " . cr THEN >> + THEN >> ; >> >> : fdt-fix-cas-node ( start -- ) >> 0 to fdt-cas-pass dup (fdt-fix-cas-node) drop \ Add phandles >> 1 to fdt-cas-pass dup (fdt-fix-cas-node) drop \ Patch+add other properties >> - 2 to fdt-cas-pass dup (fdt-fix-cas-node) drop \ Delete phandles from pass 1 >> + 2 to fdt-cas-pass dup (fdt-fix-cas-node) drop \ Delete phandles from pass 0 >> drop >> ; >> >
On Thu, Jan 30, 2020 at 10:09:56AM +1100, Alexey Kardashevskiy wrote: > On 29/01/2020 22:33, Greg Kurz wrote: > >> Instead, this changes new nodes creation. At pass#0 when we copy phandles > >> from the FDT update blob to SLOF, we create new nodes with all > >> new properties and call "finish-device" only after all properties are > >> copied to the new nodes. At this point we particularly care about "reg" > >> as this is the unit address which SLOF parses for us and sets the unit > >> address in "finish-device"; we could skip other properties for later > > > > "finish-device" only sets the first entry of the "reg" property as a > > fallback. This will be an issue when we start seeing new nodes with > > bigger unit values (eg, a new PHB). finish-device should never change (or create or delete or whatever) any property. It should finish up the device node, and that's that. > > The setting of the unit from "reg" is actually handled by > > "fdt-unflatten-node" which calls "fdt-reg-unit": > > > > 2dup s" reg" str= IF > > 2swap 2dup fdt-reg-unit 2swap > > THEN > > > > Something similar could be done... > > Huh. Should not we then fix "finish-device"? This whole unit-address setting thing is an implementation detail in SLOF, it's an optimisation. It should be kept pretty much invisible. > > This doesn't address the case of a hotplug-over-hotplug since the > > new parent node doesn't have a "decode-unit" method, but at least > > it covers all cases where the parent node was created at boot time. If a bus node has no decode-unit, you get problems. Big problems. Segher
On Wed, 29 Jan 2020 18:46:03 -0600 Segher Boessenkool <segher@kernel.crashing.org> wrote: > On Thu, Jan 30, 2020 at 10:09:56AM +1100, Alexey Kardashevskiy wrote: > > On 29/01/2020 22:33, Greg Kurz wrote: > > >> Instead, this changes new nodes creation. At pass#0 when we copy phandles > > >> from the FDT update blob to SLOF, we create new nodes with all > > >> new properties and call "finish-device" only after all properties are > > >> copied to the new nodes. At this point we particularly care about "reg" > > >> as this is the unit address which SLOF parses for us and sets the unit > > >> address in "finish-device"; we could skip other properties for later > > > > > > "finish-device" only sets the first entry of the "reg" property as a > > > fallback. This will be an issue when we start seeing new nodes with > > > bigger unit values (eg, a new PHB). > > finish-device should never change (or create or delete or whatever) any > property. It should finish up the device node, and that's that. > Sorry I mis-phrased. "finish-device" sets the unit name according to the only the first entry of "reg". It doesn't changes any property. > > > The setting of the unit from "reg" is actually handled by > > > "fdt-unflatten-node" which calls "fdt-reg-unit": > > > > > > 2dup s" reg" str= IF > > > 2swap 2dup fdt-reg-unit 2swap > > > THEN > > > > > > Something similar could be done... > > > > Huh. Should not we then fix "finish-device"? > > This whole unit-address setting thing is an implementation detail in SLOF, > it's an optimisation. It should be kept pretty much invisible. > Is this a suggestion to consolidate most if not all of the unit address setting in one place instead of adding _yet_ another guy that does it ? > > > This doesn't address the case of a hotplug-over-hotplug since the > > > new parent node doesn't have a "decode-unit" method, but at least > > > it covers all cases where the parent node was created at boot time. > > If a bus node has no decode-unit, you get problems. Big problems. > Yeah I saw that when I tried to hotplug a PCI device on a hotplugged PHB. > > Segher
On Thu, 30 Jan 2020 10:09:56 +1100 Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > > > On 29/01/2020 22:33, Greg Kurz wrote: > > On Wed, 29 Jan 2020 13:23:16 +1100 > > Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > > > >> So far we only allowed new ibm,dynamic-reconfiguration-memory and memory > >> nodes in the FDT update blob at ibm,client-architecture-support (CAS). > >> DRC do not have unit addresses and are easy, for memory nodee we use > >> an address from the node name. > >> > >> For early hot plugged PCI devices (plugged after reset but before CAS) > >> we have to have a similar hack as for memory@ but parse the address > >> differently because of different binding. > >> > >> Instead, this changes new nodes creation. At pass#0 when we copy phandles > >> from the FDT update blob to SLOF, we create new nodes with all > >> new properties and call "finish-device" only after all properties are > >> copied to the new nodes. At this point we particularly care about "reg" > >> as this is the unit address which SLOF parses for us and sets the unit > >> address in "finish-device"; we could skip other properties for later > > > > "finish-device" only sets the first entry of the "reg" property as a > > fallback. This will be an issue when we start seeing new nodes with > > bigger unit values (eg, a new PHB). > > > > The setting of the unit from "reg" is actually handled by > > "fdt-unflatten-node" which calls "fdt-reg-unit": > > > > 2dup s" reg" str= IF > > 2swap 2dup fdt-reg-unit 2swap > > THEN > > > > Something similar could be done... > > Huh. Should not we then fix "finish-device"? > According to the commit that adds unit setting to "finish-device": commit 82d381b5cffbeac520b33e2c5f907fd018226696 Author: Thomas Huth <thuth@linux.vnet.ibm.com> Date: Thu Dec 15 15:41:34 2011 +0100 Two minor improvements for the device tree node code ... Second fix is about "my-unit". According to IEEE1275, this value should be initialized to the first component of the "reg" property in case it has not been specified by other means. So I'm not sure we should replace this fallback behavior described in the spec with anything else. > > > > >> passes. > >> > >> Note this creates naked nodes with no methods normally added to the nodes > >> as this bypasses normal discovery which SLOF performs at start. So > >> if pass#1 does not find the node created in pass#0, this points to > >> missing "decode-unit" at the new node parent (happens when adding bridge- > >> under-bridge) and this prints a message and resets. > >> > >> While at this, fix few trailing spaces and comments. > >> > >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > >> --- > >> board-qemu/slof/fdt.fs | 100 ++++++++++++++++++++++------------------- > >> 1 file changed, 54 insertions(+), 46 deletions(-) > >> > >> diff --git a/board-qemu/slof/fdt.fs b/board-qemu/slof/fdt.fs > >> index fc39a82a10b5..eb726cc990ae 100644 > >> --- a/board-qemu/slof/fdt.fs > >> +++ b/board-qemu/slof/fdt.fs > >> @@ -112,7 +112,7 @@ fdt-check-header > >> ; > >> > >> \ Lookup a string by index > >> -: fdt-fetch-string ( index -- str-addr str-len ) > >> +: fdt-fetch-string ( index -- str-addr str-len ) > >> fdt-strings + dup from-cstring > >> ; > >> > >> @@ -128,7 +128,7 @@ fdt-check-header > >> ; > >> > >> \ Encode fdt property to OF property > >> -: fdt-encode-prop ( addr len -- ) > >> +: fdt-encode-prop ( addr len -- pa ps ) > >> 2dup fdt-prop-is-string? IF > >> 1- encode-string > >> ELSE > >> @@ -443,33 +443,6 @@ r> drop > >> device-end > >> ; > >> > >> -: fdt-create-cas-node ( name -- ) > >> - 2dup > >> - 2dup " memory@" find-substr 0 = IF > >> - fdt-debug IF ." Creating memory@ " cr THEN > >> - new-device > >> - 2dup " @" find-substr nip device-name \ Parse the node name > >> - 2dup > >> - 2dup " @" find-substr rot over + 1 + -rot - 1 - \ Jump to addr afte "@" > >> - parse-2int nip xlsplit set-unit \ Parse and set unit > >> - finish-device > >> - ELSE > >> - 2dup " ibm,dynamic-reconfiguration-memory" find-substr 0 = IF > >> - fdt-debug IF ." Creating ibm,dynamic-reconfiguration-memory " cr THEN > >> - new-device > >> - device-name > >> - finish-device > >> - ELSE > >> - 2drop 2drop > >> - false to fdt-cas-fix? > >> - ." Node not supported " cr > >> - EXIT > >> - THEN > >> - THEN > >> - > >> - find-node ?dup 0 <> IF set-node THEN > >> -; > >> - > >> : str=phandle? ( s len -- true|false ) > >> 2dup s" phandle" str= >r > >> s" linux,phandle" str= > >> @@ -483,7 +456,7 @@ r> drop > >> false to fdt-cas-fix? > >> EXIT > >> THEN drop > >> - fdt-fetch-unit > >> + fdt-fetch-unit ( a1 $name ) > >> dup 0 = IF drop drop " /" THEN > >> 40 left-parse-string > >> 2swap ?dup 0 <> IF > >> @@ -492,29 +465,52 @@ r> drop > >> ELSE > >> drop > >> THEN > >> - fdt-debug IF ." Setting node: " 2dup type cr THEN > >> 2dup find-node ?dup 0 <> IF > >> - set-node 2drop > >> + set-node > >> + fdt-debug IF ." Setting node: " 2dup type cr THEN > >> + 2drop > >> + \ newnode?=0: updating the existing node, i.e. pass1 adds only phandles > >> + 0 > >> ELSE > >> + fdt-cas-pass 0 <> IF > >> + \ We could not find the node added in the previous pass, > >> + \ most likely because it is hotplug-under-hotplug case > >> + \ (such as PCI brigde under bridge) when missing new node methods > >> + \ such as "decode-unit" are critical. > >> + \ Reboot when detect such case which is expected as it is a part of > >> + \ ibm,client-architecture-support. > >> + ." Cannot handle FDT update for the " 2dup type > >> + ." node, rebooting" cr > >> + reset-all > >> + THEN > >> fdt-debug IF ." Creating node: " 2dup type cr THEN > >> - fdt-create-cas-node > >> + new-device > >> + 2dup " @" find-substr nip > >> + device-name > >> + \ newnode?=1: adding new node, i.e. pass1 adds all properties, > >> + \ most importantly "reg". After reading properties, we call > >> + \ "finish-device" which sets the unit address from "reg". > >> + 1 > >> THEN > >> - fdt-debug IF ." Current now: " pwd cr THEN > >> + swap ( newnode? a1 ) > >> + > >> + fdt-debug IF ." Current now: " pwd get-node ." = " . cr THEN > >> BEGIN > >> fdt-next-tag dup OF_DT_END_NODE <> > >> WHILE > >> + ( newnode? a1 tag ) > >> dup OF_DT_PROP = IF > >> - drop dup ( drop tag, dup addr : a1 a1 ) > >> - dup l@ dup rot 4 + ( fetch size, stack is : a1 s s a2) > >> - dup l@ swap 4 + ( fetch nameid, stack is : a1 s s i a3 ) > >> - rot ( we now have: a1 s i a3 s ) > >> - fdt-encode-prop rot ( a1 s pa ps i) > >> - fdt-fetch-string ( a1 s pa ps na ns ) > >> + drop dup ( newnode? a1 a1 ) > >> + dup l@ dup rot 4 + ( newnode? a1 s s a2) > >> + dup l@ swap 4 + ( newnode? a1 s s i a3 ) > >> + rot ( newnode? a1 s i a3 s ) > >> + fdt-encode-prop rot ( newnode? a1 s pa ps i) > >> + fdt-fetch-string ( newnode? a1 s pa ps na ns ) > >> > >> fdt-cas-pass CASE > >> 0 OF > >> - 2dup str=phandle? IF > >> - fdt-debug IF 4dup ." Phandle: " type ." =" swap ." @" . ." " .d ." bytes" cr THEN > >> + 2dup str=phandle? 7 pick or IF > >> + fdt-debug IF 4dup ." Property: " type ." =" swap ." @" . ." " .d ." bytes" cr THEN > >> property > >> ELSE > >> 4drop > >> @@ -541,8 +537,14 @@ r> drop > >> ENDCASE > >> > >> + 8 + 3 + fffffffc and > >> - ELSE dup OF_DT_BEGIN_NODE = IF > >> - drop ( drop tag ) > >> + ELSE ( newnode? a1 tag ) > >> + dup OF_DT_BEGIN_NODE = IF > >> + 2 pick IF > >> + rot drop 0 -rot > > > The proposed change from below is also needed here then, or since it has > grown enough, this can make a separate function then. > Yeah, I realized that after posting this patch. We can probably come up with a word that can be used in both the _node_ and _subnode_ cases. > Can you please take over this and finish the change (merge yours 1/2 > into this one and I will comment on 2/2)? Thanks, > Sure. This will give you cycles to make progress on killing SLOF :-P > > >> + get-node finish-device set-node > >> + fdt-debug IF ." Finished node: " pwd get-node ." = " . cr THEN > >> + THEN > >> + drop ( a1 ) > >> 4 - > >> (fdt-fix-cas-node) > >> get-parent set-node > >> @@ -554,13 +556,19 @@ r> drop > >> THEN > >> THEN > >> REPEAT > >> - drop \ drop tag > >> + ( newnode? a1 tag ) > >> + drop > >> + swap ( a1 newnode? ) > >> + IF > > > > ... here. > > > > " reg" get-node get-package-property IF ELSE fdt-reg-unit THEN > > > > This doesn't address the case of a hotplug-over-hotplug since the > > new parent node doesn't have a "decode-unit" method, but at least > > it covers all cases where the parent node was created at boot time. > > > >> + get-node finish-device set-node > >> + fdt-debug IF ." Finished subnode: " pwd get-node ." = " . cr THEN > >> + THEN > >> ; > >> > >> : fdt-fix-cas-node ( start -- ) > >> 0 to fdt-cas-pass dup (fdt-fix-cas-node) drop \ Add phandles > >> 1 to fdt-cas-pass dup (fdt-fix-cas-node) drop \ Patch+add other properties > >> - 2 to fdt-cas-pass dup (fdt-fix-cas-node) drop \ Delete phandles from pass 1 > >> + 2 to fdt-cas-pass dup (fdt-fix-cas-node) drop \ Delete phandles from pass 0 > >> drop > >> ; > >> > > >
On Thu, Jan 30, 2020 at 08:47:40AM +0100, Greg Kurz wrote: > On Wed, 29 Jan 2020 18:46:03 -0600 > Segher Boessenkool <segher@kernel.crashing.org> wrote: > > > On Thu, Jan 30, 2020 at 10:09:56AM +1100, Alexey Kardashevskiy wrote: > > > On 29/01/2020 22:33, Greg Kurz wrote: > > > > "finish-device" only sets the first entry of the "reg" property as a > > > > fallback. This will be an issue when we start seeing new nodes with > > > > bigger unit values (eg, a new PHB). > > > > finish-device should never change (or create or delete or whatever) any > > property. It should finish up the device node, and that's that. > > > > Sorry I mis-phrased. "finish-device" sets the unit name according to > the only the first entry of "reg". It doesn't changes any property. Ah okay, never mind, that is all just fine then :-) Surgery here is although not necessarily fragile, certainly very touchy. > > > > The setting of the unit from "reg" is actually handled by > > > > "fdt-unflatten-node" which calls "fdt-reg-unit": > > > > > > > > 2dup s" reg" str= IF > > > > 2swap 2dup fdt-reg-unit 2swap > > > > THEN > > > > > > > > Something similar could be done... > > > > > > Huh. Should not we then fix "finish-device"? > > > > This whole unit-address setting thing is an implementation detail in SLOF, > > it's an optimisation. It should be kept pretty much invisible. > > Is this a suggestion to consolidate most if not all of the unit address > setting in one place instead of adding _yet_ another guy that does it ? It wasn't, but yes that is a good idea. Now the trick of course is to design a good interface for it, before changing 30 callers. > > > > This doesn't address the case of a hotplug-over-hotplug since the > > > > new parent node doesn't have a "decode-unit" method, but at least > > > > it covers all cases where the parent node was created at boot time. > > > > If a bus node has no decode-unit, you get problems. Big problems. > > Yeah I saw that when I tried to hotplug a PCI device on a hotplugged PHB. They are not the kind of problems you easily overlook, yup. Segher
diff --git a/board-qemu/slof/fdt.fs b/board-qemu/slof/fdt.fs index fc39a82a10b5..eb726cc990ae 100644 --- a/board-qemu/slof/fdt.fs +++ b/board-qemu/slof/fdt.fs @@ -112,7 +112,7 @@ fdt-check-header ; \ Lookup a string by index -: fdt-fetch-string ( index -- str-addr str-len ) +: fdt-fetch-string ( index -- str-addr str-len ) fdt-strings + dup from-cstring ; @@ -128,7 +128,7 @@ fdt-check-header ; \ Encode fdt property to OF property -: fdt-encode-prop ( addr len -- ) +: fdt-encode-prop ( addr len -- pa ps ) 2dup fdt-prop-is-string? IF 1- encode-string ELSE @@ -443,33 +443,6 @@ r> drop device-end ; -: fdt-create-cas-node ( name -- ) - 2dup - 2dup " memory@" find-substr 0 = IF - fdt-debug IF ." Creating memory@ " cr THEN - new-device - 2dup " @" find-substr nip device-name \ Parse the node name - 2dup - 2dup " @" find-substr rot over + 1 + -rot - 1 - \ Jump to addr afte "@" - parse-2int nip xlsplit set-unit \ Parse and set unit - finish-device - ELSE - 2dup " ibm,dynamic-reconfiguration-memory" find-substr 0 = IF - fdt-debug IF ." Creating ibm,dynamic-reconfiguration-memory " cr THEN - new-device - device-name - finish-device - ELSE - 2drop 2drop - false to fdt-cas-fix? - ." Node not supported " cr - EXIT - THEN - THEN - - find-node ?dup 0 <> IF set-node THEN -; - : str=phandle? ( s len -- true|false ) 2dup s" phandle" str= >r s" linux,phandle" str= @@ -483,7 +456,7 @@ r> drop false to fdt-cas-fix? EXIT THEN drop - fdt-fetch-unit + fdt-fetch-unit ( a1 $name ) dup 0 = IF drop drop " /" THEN 40 left-parse-string 2swap ?dup 0 <> IF @@ -492,29 +465,52 @@ r> drop ELSE drop THEN - fdt-debug IF ." Setting node: " 2dup type cr THEN 2dup find-node ?dup 0 <> IF - set-node 2drop + set-node + fdt-debug IF ." Setting node: " 2dup type cr THEN + 2drop + \ newnode?=0: updating the existing node, i.e. pass1 adds only phandles + 0 ELSE + fdt-cas-pass 0 <> IF + \ We could not find the node added in the previous pass, + \ most likely because it is hotplug-under-hotplug case + \ (such as PCI brigde under bridge) when missing new node methods + \ such as "decode-unit" are critical. + \ Reboot when detect such case which is expected as it is a part of + \ ibm,client-architecture-support. + ." Cannot handle FDT update for the " 2dup type + ." node, rebooting" cr + reset-all + THEN fdt-debug IF ." Creating node: " 2dup type cr THEN - fdt-create-cas-node + new-device + 2dup " @" find-substr nip + device-name + \ newnode?=1: adding new node, i.e. pass1 adds all properties, + \ most importantly "reg". After reading properties, we call + \ "finish-device" which sets the unit address from "reg". + 1 THEN - fdt-debug IF ." Current now: " pwd cr THEN + swap ( newnode? a1 ) + + fdt-debug IF ." Current now: " pwd get-node ." = " . cr THEN BEGIN fdt-next-tag dup OF_DT_END_NODE <> WHILE + ( newnode? a1 tag ) dup OF_DT_PROP = IF - drop dup ( drop tag, dup addr : a1 a1 ) - dup l@ dup rot 4 + ( fetch size, stack is : a1 s s a2) - dup l@ swap 4 + ( fetch nameid, stack is : a1 s s i a3 ) - rot ( we now have: a1 s i a3 s ) - fdt-encode-prop rot ( a1 s pa ps i) - fdt-fetch-string ( a1 s pa ps na ns ) + drop dup ( newnode? a1 a1 ) + dup l@ dup rot 4 + ( newnode? a1 s s a2) + dup l@ swap 4 + ( newnode? a1 s s i a3 ) + rot ( newnode? a1 s i a3 s ) + fdt-encode-prop rot ( newnode? a1 s pa ps i) + fdt-fetch-string ( newnode? a1 s pa ps na ns ) fdt-cas-pass CASE 0 OF - 2dup str=phandle? IF - fdt-debug IF 4dup ." Phandle: " type ." =" swap ." @" . ." " .d ." bytes" cr THEN + 2dup str=phandle? 7 pick or IF + fdt-debug IF 4dup ." Property: " type ." =" swap ." @" . ." " .d ." bytes" cr THEN property ELSE 4drop @@ -541,8 +537,14 @@ r> drop ENDCASE + 8 + 3 + fffffffc and - ELSE dup OF_DT_BEGIN_NODE = IF - drop ( drop tag ) + ELSE ( newnode? a1 tag ) + dup OF_DT_BEGIN_NODE = IF + 2 pick IF + rot drop 0 -rot + get-node finish-device set-node + fdt-debug IF ." Finished node: " pwd get-node ." = " . cr THEN + THEN + drop ( a1 ) 4 - (fdt-fix-cas-node) get-parent set-node @@ -554,13 +556,19 @@ r> drop THEN THEN REPEAT - drop \ drop tag + ( newnode? a1 tag ) + drop + swap ( a1 newnode? ) + IF + get-node finish-device set-node + fdt-debug IF ." Finished subnode: " pwd get-node ." = " . cr THEN + THEN ; : fdt-fix-cas-node ( start -- ) 0 to fdt-cas-pass dup (fdt-fix-cas-node) drop \ Add phandles 1 to fdt-cas-pass dup (fdt-fix-cas-node) drop \ Patch+add other properties - 2 to fdt-cas-pass dup (fdt-fix-cas-node) drop \ Delete phandles from pass 1 + 2 to fdt-cas-pass dup (fdt-fix-cas-node) drop \ Delete phandles from pass 0 drop ;
So far we only allowed new ibm,dynamic-reconfiguration-memory and memory nodes in the FDT update blob at ibm,client-architecture-support (CAS). DRC do not have unit addresses and are easy, for memory nodee we use an address from the node name. For early hot plugged PCI devices (plugged after reset but before CAS) we have to have a similar hack as for memory@ but parse the address differently because of different binding. Instead, this changes new nodes creation. At pass#0 when we copy phandles from the FDT update blob to SLOF, we create new nodes with all new properties and call "finish-device" only after all properties are copied to the new nodes. At this point we particularly care about "reg" as this is the unit address which SLOF parses for us and sets the unit address in "finish-device"; we could skip other properties for later passes. Note this creates naked nodes with no methods normally added to the nodes as this bypasses normal discovery which SLOF performs at start. So if pass#1 does not find the node created in pass#0, this points to missing "decode-unit" at the new node parent (happens when adding bridge- under-bridge) and this prints a message and resets. While at this, fix few trailing spaces and comments. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- board-qemu/slof/fdt.fs | 100 ++++++++++++++++++++++------------------- 1 file changed, 54 insertions(+), 46 deletions(-)