Auto-generate device register C headers from SystemRDL description#290
Auto-generate device register C headers from SystemRDL description#290ziuziakowska merged 4 commits intolowRISC:mainfrom
Conversation
6862ff2 to
51a054e
Compare
51a054e to
e87afb1
Compare
462e38c to
9e17dea
Compare
ec17380 to
80ea546
Compare
80ea546 to
9ad7a11
Compare
AlexJones0
left a comment
There was a problem hiding this comment.
Thanks for all the work on fine-tuning this. I've left some comments and there's a couple of parts where I would appreciate just a little more clarity to help my understanding.
I do think that if we need to extend this in the future, it may be more maintainable to use a templating engine (to deal with the presentation) and just pass in dataclasses corresponding to the different C constructs and declarations, but I'm reasonably convinced that it would be similarly complicated at this point and so sticking with dynamic string construction is fine.
| pass | ||
| input_file = Path(args.input_file) | ||
| with input_file.open("r") as f: | ||
| rdl = json.load(f) |
There was a problem hiding this comment.
It's fine to leave this as is for this PR, but as a general comment it would be nice in the future if code like this would use e.g. a Pydantic dataclass. That way we can define the structure we expect out of the RDL JSON file (and validate it at load-time), and we can then encapsulate the various computed fields as properties on this model. check_register_rdl(...) could then just be defined as a validator on the Pydantic model, and registers_are_structurally_equal(...) as __eq__ or similar.
I don't have too much context on the RDL flows, but maybe these models are something they should provide for you and expose as part of their library if the format is known and consistent?
There was a problem hiding this comment.
I'm not familiar at all with SystemRDL tooling as I am just consuming the generated JSON blob of the RDL data that we've got, but these Pydantic dataclasses seem very useful as typing (for example) lists of registers as just list[dict] is not as type-safe as I would like it. I think something like this can be addressed in a future PR, as I'm more concerned about the format of the generated output.
There was a problem hiding this comment.
I had a comment a few days ago but I forgot to hit the Submit review button.
We should use templates for this codegen, it will significantly reduce python code, it's easier to read and maintain.
9b32d46 to
c1c2737
Compare
c1c2737 to
566812a
Compare
566812a to
546a736
Compare
|
|
||
| def get_and_emit_register_types( | ||
| device_name: str, registers: list[dict] | ||
| ) -> tuple[list[tuple[dict, str]], str]: |
There was a problem hiding this comment.
Long-term, for maintainability, we should probably be capturing this compound information inside an object/dataclass.
546a736 to
9b88732
Compare
AlexJones0
left a comment
There was a problem hiding this comment.
Thanks for the work on this, @ziuziakowska.
As mentioned in other comments it would be nice to move to Pydantic models and templating in the future, but I think that's a goal for refactoring and this works well for now.
It would be good to also have @engdoreis check this.
9b88732 to
1b8e631
Compare
|
Minor changes from discussion in #299: device structs suffixed with |
Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>
Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>
Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>
Signed-off-by: Alice Ziuziakowska <a.ziuziakowska@lowrisc.org>
1b8e631 to
6cd92a7
Compare
This PR extends
util/rdlgenerator.pyto generate device and register definition header files. The generated files live inlib/hal/autogenfor the corresponding library code inlib/halto make use of.The generated files consist of a
structdeclaration of the device's layout in memory, which is to be interacted with through a volatile pointertypedefin library code. The fields of thisstructcorrespond to the registers of the device, separated by reserved padding fields to match the memory layout.The layouts of individual registers are also generated and are used as the types of the registers in the device
structdeclaration, with some processing to reduce the amount of code generated and simplify usage by the HAL:uint32_ttype. This avoids the need to emit astructwith oneuint32_tfield of usually the same name as the register (e.gtimer.TIMER_V_{LOWER,UPPER}0,timer.COMPARE_{LOWER_UPPER}0_0), or a large structure with many distinct bits where software might want to expose toggling arbitrarily bits for ease of use (spi_device.CMD_FILTER).enumwith each flag bit as a variant. This is to enable a usage pattern where arbitrary combinations of the flags can be queried or atomically set/unset (e.guart.STATUS,spi_device.INTERCEPT_EN, various interrupt enable/state/test registers). This is not done for registers with a single flag field as there is no benefit to be had over emitting astruct.structwith bitfields corresponding to the register fields, with unnamed padding bitfields between them if necessary.enumregisters that have equivalent register fields are combined into a singleenumdeclaration that is used for all those registers. This de-duplicates theenumtypes for each device'sINTR_{STATE,ENABLE,TEST}registers. These are handled by an initial first pass.Attributes and assertions are used to ensure that all register types are 4 bytes in size and naturally aligned, and that all registers are at their expected offsets within the device's memory layout.
This PR only consists of the auto-generator changes. Rework of the HAL to use these is in #299 which builds upon the changes in this branch.
Closes: #29.