On Fri, Apr 28, 2023 at 09:11:30PM +0200, Uwe Kleine-König wrote:
> 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,
Oof.. sorry, I don't think that's an acceptable change for a round
trip of this type. Couldn't you avoid that if you don't strip out the
'phandle' properties?
> and the order
> of the contents of the __fixup__, __local_fixup__ and __symbol__ nodes.
This.. isn't ideal, but might be tolerable.
> > > 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.
Thanks.
> > > [...]
> > > +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.
Hrm.. that's not really true, though, since without those you lose the
exact numerical values of the phandles. Usually that's unimportant of
course, but in certain debugging situations it might matter.
> > > @@ -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?!
Huh, right. I'd prefer to see this bugfix separated out into a
preliminary patch.
> > > 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?
Ah.. yes, I think you're right. Actually, it more or less has to be
ok, because we don't do any de-escaping when parsing references like
that.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
Attachment:
signature.asc
Description: PGP signature
