generate-zephyr-dts.py: refactored version #34
Conversation
|
@hvegh -- Looks like this change needs to be rebased. |
|
FYI - @mateusz-holenko |
There was a problem hiding this comment.
Hi @hvegh,
Thanks for your contribution!
I like the proposed way of generating dtsi - I agree it's cleaner and easier to follow.
I'm not sure about the json vs csv, though. Currently we use the "external" script for parsing because:
- it supports both formats (
jsonandcsv), - this is a common logic of
generate-zephyr-dts.pyandgenerate-renode-scripts.py.
Of course I understand that having a single file might be more convenient, but is this necessary? I'm open for a discussion.
generate-zephyr-dts.py
Outdated
| print('Skipping unsupported peripheral `{}` at {}' | ||
| .format(name, hex(peripheral['address']))) | ||
| continue | ||
| for name, parm in overlay_handlers.items(): |
There was a problem hiding this comment.
By iterating over overlay_handler instead of configuration.peripherals you miss the warnings about unsupported (yet available in the platform) peripherals.
I think this might be important when debugging problems.
There was a problem hiding this comment.
Hi @mateusz-holenko,
Thanks you for the review!
Yes I can see your point, the shared code between the DTS and Renode configuration.
Currently there are 6 parsers or interpretations made:
LiteX to CSV --- CSV to lx.configutation
LiteX to JSON --- JSON to lx.configutation
lx.configuration to renode
lx.configutation to DTS
I would argue to at least ditch the CSV format in favor of JSON as it is better at preserving structure and type. So JSON seems more future proof, and we can ditch CSV at zero cost, nobody gets hurts as far as i can tell..
So then we have to maintain two parsers less:
LiteX to JSON --- JSON to lx.configutation
lx.configuration to renode
lx.configutation to DTS
Similarl since the CSV is a subset of JSON and there are no complex routines that can be shared, for now. The benefits of having a separate configuration class just for abstracting the JSON and CSV inputs is not that usefull. So you could further reduce to:
LiteX to JSON
JSON to renode
JSON to DTS
We end up with two parsers closely coupled to the JSON output format of LITEX.
There was a problem hiding this comment.
By iterating over
overlay_handlerinstead ofconfiguration.peripheralsyou miss the warnings about unsupported (yet available in the platform) peripherals.
I think this might be important when debugging problems.
I agree this should be fixed!
There was a problem hiding this comment.
we can ditch CSV at zero cost, nobody gets hurts as far as i can tell
Do you mean that we can parse CSV files using the json parser? litex-buildenv uses this script to generate Zephyr overlay from CSV: https://github.com/timvideos/litex-buildenv/blob/master/scripts/build-zephyr.sh#L84.
There was a problem hiding this comment.
Additionally https://github.com/google/CFU-Playground also uses the csv format for generating Renode scripts at the moment.
There was a problem hiding this comment.
Nice links, both are interesting projects.
The main drawback for JSON is that the addresses are in decimal instead of hex, so it is not as human readable as CSV.
|
BTW there is no need to close the PR and open a new one - you can always force push the original branch and GitHub will handle this nicely. |
The changes are part of an effort to enable platformio to handle the
different litex-board variants for the zephyr framework.
the configuration class. No external python modules required,
can run as standalone conversion tool.
format.
The resulting changes are aimed for better maintainability.
From a maintenance perspective it would be nice if this file could
be part of the zephyr mainline tree.
Signed-off-by: Henk Vergonet henk.vergonet@gmail.com