Hello,
On Fri, Apr 28, 2023 at 04:11:45PM +1000, David Gibson wrote:
> On Wed, Apr 26, 2023 at 08:25:58PM +0200, Uwe Kleine-König wrote:
> > The data contained in these nodes can be used to restore labels for nodes
> > (from __symbols__ if -@ is given) and to identify which data values are
> > references (from __local_fixups__ if -L is given).
>
> Oh, that's a neat idea.
:-)
> > The resulting dts doesn't necessarily compiles back into the bitwise same
> > dtb,
>
> Hmm.. in exactly what ways can it differ? Is that different from what
> could happen before? Generally a dtb -> dts -> dtb round trip should
> be a no-op to a pretty high degree of fidelity. That's not true of a
> dts -> dtb -> dts round trip, since there are many more ways to
> express things in source form.
The actual values of the phandle properties might differ, and the order
of the contents of the __fixup__, __local_fixup__ and __symbol__ nodes.
>
> > but the result is equivalent and easier to modify without breaking
> > references.
> >
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> > ---
> > dtc.c | 16 +++++++---
> > dtc.h | 2 ++
> > livetree.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > treesource.c | 37 +++++++++++++++------
> > 4 files changed, 132 insertions(+), 13 deletions(-)
> >
> > diff --git a/dtc.c b/dtc.c
> > index d2e4e2b55b5c..e36f0dfad313 100644
> > --- a/dtc.c
> > +++ b/dtc.c
> > @@ -335,12 +335,20 @@ int main(int argc, char *argv[])
> > if (auto_label_aliases)
> > generate_label_tree(dti, "aliases", false);
> >
> > - if (generate_symbols)
> > - generate_label_tree(dti, "__symbols__", true);
> > + if (generate_symbols) {
> > + if (streq(inform, "fs") || streq(inform, "dtb"))
> > + generate_labels_from_tree(dti, "__symbols__");
> > + else
> > + generate_label_tree(dti, "__symbols__", true);
>
> Changing behaviour of what we do to the tree based on the input format
> smells a bit wrong to me. Should we maybe be importing any existing
> label information from __symbols__, then re-generating it all cases
> (effectively merging any symbols from source parsing with ones given
> in __symbols__).
Yeah, that sounds sensible. (Note however that with inform "fs" and
"dtb" there are no labels in dti, so there is nothing lost here.)
> > + }
> >
> > if (generate_fixups) {
> > - generate_fixups_tree(dti, "__fixups__");
> > - generate_local_fixups_tree(dti, "__local_fixups__");
> > + if (streq(inform, "fs") || streq(inform, "dtb")) {
>
> Similar question here.
Similar answer here. :-)
I'll rework the patch.
> > [...]
> > +static void drop_phandles(struct node *n)
>
> I don't really understand why you want to remove the phandle properties.
As the phandle references are restored the phandle properties hold only
duplicate information and they might make it harder to further work with
the resulting dts.
> > @@ -219,6 +219,9 @@ static void write_propval(FILE *f, struct property *prop)
> > if (emit_type == TYPE_NONE || chunk_len == 0)
> > continue;
> >
> > + if (m->offset != 0)
> > + fputc(' ', f);
>
> I'm not sure how this change is related to anything else.
Without this, the resulting dts might have:
clocks = <&clk 17&clk 19>;
I think before my patch this never happend in practise because an array
never had more than one marker?!
> > switch(emit_type) {
> > case TYPE_UINT16:
> > write_propval_int(f, p, chunk_len, 2);
> > @@ -230,10 +233,26 @@ static void write_propval(FILE *f, struct property *prop)
> > break;
> >
> > if (m_phandle) {
> > - if (m_phandle->ref[0] == '/')
> > - fprintf(f, "&{%s}", m_phandle->ref);
> > - else
> > - fprintf(f, "&%s", m_phandle->ref);
> > + if (m_phandle->ref) {
> > + if (m_phandle->ref[0] == '/')
> > + fprintf(f, "&{%s}", m_phandle->ref);
> > + else
> > + fprintf(f, "&%s", m_phandle->ref);
> > + } else {
> > + cell_t phandle = fdt32_to_cpu(*(const fdt32_t *)p);
> > + struct node *n = get_node_by_phandle(root, phandle);
> > +
> > + if (!n) {
> > + fprintf(f, "&??");
>
> In this case you will present strictly less information than before
> "&??" instead of whatever integer value was there. That doesn't seem
> ideal.
I don't think that keeping the previous integer value is beneficial. The
phandle properties generated when compiling to dtb again will likely
differ on recompilation and so the previous value might lead to silent
inconsistencies. So this should result in some human attention.
Generating a broken dts is one option (the one I chose), I think the
only other half-way sane option is to abort with an error.
> > + } else {
> > + if (n->labels) {
> > + fprintf(f, "&%s", n->labels->label);
> > + } else {
> > + fprintf(f, "&{%s}", n->fullpath);
>
> DT paths can have some slightly odd characters in them, have you
> verified that you don't need any escaping here?
what would need escaping? A '}'? Looking at the grammar files I don't
spot a way to quote things here. Am I missing anything?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
