Openamp xlnx cleanup for domain access

[bentheredonethat]

@zeddii here is the updates

vendor specific builds at this point are using another repo/branch so should be ok to review

[zeddii]

I've started making some comments as I read the commits themselves. But before I spend too much more time making comments that don't match your overall restructuring, can you post a summary of what operations you are trying to move to what assists/lops. i.e. "all openamp assists are in: " "all yaml expansion is in " the system device tree specification and library functions are ".

[bentheredonethat]

• yaml expansion - take in yaml and translate to DTS - this only
• openamp expansion - given openamp relations do DT processing

[bentheredonethat]

@zeddii
The restructuring separates the YAML, domain, and OpenAMP logic into distinct assists. The YAML assist now handles only YAML → DTS translation. It reads the system YAML (e.g. system-topology.yaml), appends the processed YAML content into a DTS fragment, and applies it to the internal tree. It also parses OpenAMP-related YAML metadata but performs no OpenAMP transformations. The OpenAMP assist runs after YAML translation, using the expanded DTS to generate inter-domain structures (shared memory, vrings, rproc links). The domain-access plugin remains responsible only for domain generation — it performs no YAML or OpenAMP work.

Example Linux domain flow:

python3 lopper.py -f --permissive --enhanced -x '*.yaml' -i $YAML $SDT out.dts # YAML translation
python3 lopper.py -f --permissive --enhanced out.dts APU_Linux.dts -- domain_access -t /domains/APU_Linux
python3 lopper.py -f --enhanced -i $LOPS_DIR/lop-a78-imux.dts APU_Linux.dts APU_Linux.dts
python3 lopper.py -f APU_Linux.dts openamp_APU_Linux.dts -- openamp cortexa78_0 linux_dt
python3 lopper.py -f --enhanced openamp_APU_Linux.dts linux.dts -- gen_domain_dts cortexa78_0 linux_dt

Confirming that OpenAMP logic (e.g. resource linking) has been moved out of YAML expansion and placed in a dedicated OpenAMP assist, maintaining strict separation between YAML translation, domain processing, and OpenAMP generation.

[zeddii]

The problem with this, is that subsystem isn't only used by openamp. It is a generalized application of the system device tree subsystem specification.

So something like this really belongs in an openamp specific assist, or somehow be data driven on more than the name of the node. This is all very specific to a node layout.

[zeddii]

Thinking on this more, this is still too much of a special case. lops run before assists. If this information is to be saved for something else down the pipeline, it belongs in a lop (which can be much more focused) versus in this assists.

[bentheredonethat]

hi @zeddii the reason i added these parts (RPU only btw) is for AMD-xilinx specific case where some of the rpu information is used AFTER RPUs are removed by domain access in the openamp case.

[zeddii]

I understand that there may be a reason, but the subsystem routines are generic to the spec. If we need to store something for openamp/xilinx, it needs to be done in an openamp/xilinx specific location.

[bentheredonethat]

@zeddii currently it is in the domain top level. what if i store it in the remoteproc relation area instead? something

so instead of this:

RPU_Zephyr { compatible = "openamp,domain-v1"; cpus = <&cpus_r52_0 0x1 0x80000003>; os,type = "zephyr"; sram = <0x9800100 0x5ff00 0xeba00000 0x10000 0xeba10000 0x8000 0xeba20000 0x8000>; reserved-memory = <0x26a 0x265>; access = <&ddrboot9800100 0x0>, <&ipc9860000 0x0>, <&ttc2 0x0>, <&ipi1 0x0>, <&ipi_nobuf2 0x0>; rpu_pd_val = <0xc1 0x181100bf>; cpu_config_str = "split"; core_num = "0"; access-json = "[{\"dev\": \"ddrboot@9800100\", \"flags\": {}}, {\"dev\": \"ipc@9860000\", \"flags\": {}}, {\"dev\": \"timer@f1e80000\", \"flags\": {}, \"label\": \"ttc2\"}, {\"dev\": \"mailbox@eb340000\", \"flags\": {}, \"label\": \"ipi1\"}, {\"dev\": \"mailbox@eb3b1000\", \"flags\": {}, \"label\": \"ipi_nobuf2\"}]"; phandle = <0x26b>;

it will be:
` RPU_Zephyr {
compatible = "openamp,domain-v1";
cpus = <&cpus_r52_0 0x1 0x80000003>;
os,type = "zephyr";
sram = <0x9800100 0x5ff00 0xeba00000 0x10000 0xeba10000 0x8000 0xeba20000 0x8000>;
reserved-memory = <0x26a 0x265>;
access = <&ddrboot9800100 0x0>,
<&ipc9860000 0x0>,
<&ttc2 0x0>,
<&ipi1 0x0>,
<&ipi_nobuf2 0x0>;
access-json = "[{"dev": "ddrboot@9800100", "flags": {}}, {"dev": "ipc@9860000", "flags": {}}, {"dev": "timer@f1e80000", "flags": {}, "label": "ttc2"}, {"dev": "mailbox@eb340000", "flags": {}, "label": "ipi1"}, {"dev": "mailbox@eb3b1000", "flags": {}, "label": "ipi_nobuf2"}]";
phandle = <0x26b>;

                    domain-to-domain {
                            compatible = "openamp,domain-to-domain-v1";
                            cluster_cpu = "cortexr52_0";
                            rpu_pd_val = <0xc1 0x181100bf>;
                            cpu_config_str = "split";
                            core_num = "0";

                            rpmsg-relation {
                                    compatible = "openamp,rpmsg-v1";

                                    relation0 {
                                            host = <0x26e>;
                                            mbox = <0x1da>;
                                            carveouts = <0x26a>;
                                    };    
                            };    
                    };    
            }; `

[zeddii]

It is less about what is stored where, but what assist/lop is doing the storing. Can we move this to a separate routine that is triggered only when the use case that needs it runs, versus having the conditional in the general routines ?

[bentheredonethat]

Hi @zeddii i get your point about what is being stored.

instead of adding another lop or assist (which makes it harder to debug and track) can we move this information processing to singular routine, like openamp_remote_domain_expand(), that stores the information in the openamp remoteproc relation?

that way the logic is in 1 place but we are acting to reduce the spaghetti code effect :)

please advise!

[zeddii]

A clearly named routine is a good middle ground. I can definitely live with that.

[zeddii]

What about the simpler cases ? We shouldn't insist on the (slightly) more complexity of needing to define nodes for reserved memory. There are cases where we just want a simple start and size in the yaml and have that become the reserved memory node.

[bentheredonethat]

@zeddii but if a user defines start/size then why not also just add to this routine the start/size chunk handling?

this routine only gets if a user wants to have reserved-memory anyway right?

[zeddii]

It only gets run if they want to add / define reserved memory in addition to what is already in a device tree (i.e. /reserved-memory as a top level node).

What I'm trying to sort out is whether or not we are insisting on it being a plain-text (versus anchor) reference to a node, and have no support for specification of start/size notation.

I'd prefer to support both.

It isn't strictly part of the spec (the notation, since this is a short hand), so we have some wiggle room.

[bentheredonethat]

ok so few things we can do here in addition

1. support start/size tuples ( i feel like there should be a helper for this already? since its a common thing to do in yaml)
2. anchor support - what do you suggest here? the thing is - anchor usage is a weird case... i feel like that is a hook we can add in separately and currently can leave a comment of TODO on that. right now using anchors introduces weird dupes to handle later

once those two points are addressed is this ok?

[zeddii]

yes. once we figure out what to do with them, we are ok. There may be existing anchor usage in yaml files, so we can't just remove the support without a warning or migration plan.

At a minimum the yaml expansion needs to convert human readable start/size fields to numbers.

What they get converted to (i.e. reg property or something else) is dependent on where they are being used.

So in this case, yaml expansion can do some of it and we should add routines to lopper_lib that do specific outputs, i.e. lopper_lib.expand_start_size_to_reg()

[zeddii]

We should document what inputs result in no memory chunks coming through. Is there somewhere earlier we could avoid the processing ?

[bentheredonethat]

i can add another commit later that adds comments if that is what you are asking for here?

[zeddii]

that works.

[bentheredonethat]

Hi @zeddii

1. added documentation / comments per review
2. i checked and start/size handling is already there in this tree for reserved memory expansion.

please let me know if this is now sufficient with latest tree.

thanks!

[zeddii]

I've staged this locally to do some extended integration testing. Your openamp plugin is now throwing an assertion:

xowbrucea41 [/home/bruc...cea/lopper]> ./lopper_sanity.py --openamp
[tree.py][WARNING]: Exception ('/reserved-memory') raised by code block:
assist_dir = os.path.dirname(os.path.realpath(file)) + '/assists/'
sys.path.append(assist_dir)
import importlib
yaml_to_dts_expansion = importlib.import_module( '.yaml_to_dts_expansion', package='lopper.assists' )
def __node_test_block():

                                    if __selected__ == []:
                                        return

                                    for n in __selected__[0].subnodes(children_only=True, name='domain-to-domain$'):
                                        if n.abs_path.endswith('/domain-to-domain'):
                                            yaml_to_dts_expansion.domain_to_domain_expand( tree, n)

__nret = __node_test_block()

Can you queue a commit onto the end of the series to get it passing again ? (There are more issues than just this explicit error).

[bentheredonethat]

@zeddii sanity test now passing

[zeddii]

ok. I see the scripts it is now calling, which are a sequences of lopper commands. But I'm not seeing how they are checking the details for a pass/fail ? Is it purely on the return code from lopper ? Something else that I'm missing ?

[bentheredonethat]

@zeddii it now does 2 parts

1. run script (sequence of lopper commands) as you noted - this is simpler for me then constructing the set of lopper commands to run
2. if you look at the commit, at the bottom, you will see it ALSO changes lopper sanity.py here d5fd301#diff-151c729b98b2f603b1d49e635d7b350867a7d57695ec7ac32e8d49d1b14b0bebL2118-R2115 to check if nodes come in after all the lopper runs
   i can extend this in the future to do more fine grained checks but im doing a minimal sanity for now to get this in and we can file an issue / internal tracker to extend this alter