On Tue, Apr 18, 2023 at 12:33:26PM -0500, Rob Herring wrote:
> On Thu, Apr 13, 2023 at 7:44 AM Qun-wei Lin (林群崴)
> <Qun-wei.Lin@xxxxxxxxxxxx> wrote:
> >
> > On Thu, 2023-03-09 at 09:12 -0600, Rob Herring wrote:
> > > On Wed, Mar 8, 2023 at 3:17 AM Qun-Wei Lin <qun-wei.lin@xxxxxxxxxxxx>
> > > wrote:
> > > >
> > > > The overlay fragment is a special case where some properties are
> > > > not
> > > > present in the overlay source file, but in the base file.
> > > >
> > > > example:
> > > > +-----------------------------+--------------------+
> > > > > base.dts | overlay.dts |
> > > >
> > > > +-----------------------------+--------------------+
> > > > > /dts-v1/; | /dts-v1/; |
> > > > > | /plugin/; |
> > > > > /{ | |
> > > > > parent: test { | &parent { |
> > > > > #address-cells = <1>; | child@0 { |
> > > > > #size-cells = <0>; | reg = <0x0>; |
> > > > > }; | }; |
> > > > > }; | }; |
> > > >
> > > > +-----------------------------+--------------------+
> > > >
> > > > It will cause the following false alarms when compiling the overlay
> > > > dts.
> > > >
> > > > 1. /fragment@0/__overlay__: Character '_' not recommended in node
> > > > name
> > > > 2. /fragment@0/__overlay__: Relying on default #address-cells value
> > > > 3. /fragment@0/__overlay__: Relying on default #size-cells value
> > > > 4. /fragment@0/__overlay__:reg: property has invalid length (4
> > > > bytes)
> > > > (#address-cells == 2, #size-cells == 1)
> > > >
> > > > This workaround will fix them by skip checking for node named
> > > > __overlay__.
> > > >
> > > > Signed-off-by: Qun-Wei Lin <qun-wei.lin@xxxxxxxxxxxx>
> > > > ---
> > > > V1 -> V2:
> > > > - Add is_overlay_node() helper
> > > > - Skip anything starting with "__" in
> > > > check_node_name_chars_strict()
> > > >
> > > > checks.c | 18 ++++++++++++++++++
> > > > 1 file changed, 18 insertions(+)
> > >
> > > Reviewed-by: Rob Herring <robh@xxxxxxxxxx>
> > >
> > > Though I do wonder if as a matter of policy on overlay structure, if
> > > we should require an overlay to have the parent node with
> > > #address-cells/#size-cells. In the end that would be duplicated data,
> > > but without it there's no way to parse and validate reg/ranges in an
> > > unapplied overlay. That's just one example issue in being able to
> > > validate overlays.
> > >
> > > Rob
> >
> > Hi Rob,
> >
> > Thank you for your review.
> >
> > I think I've found another problem:
> > +-------------------------+----------------+
> > | base.dts | overlay.dts |
> > +-------------------------+----------------+
> > | /dts-v1/; | /dts-v1/; |
> > | /{ | /plugin/; |
> > | #address-cells = <1>; | |
> > | #size-cells = <0>; | &test { |
> > | test: example@0 { | reg = <0x1>; |
> > | reg = <0x0>; | }; |
> > | }; | |
> > | }; | |
> > +-------------------------+----------------+
> >
> > The following error message is printed when compiling:
> > Warning (reg_format): /fragment@0/__overlay__:reg: property has invalid
> > length (4 bytes) (#address-cells == 2, #size-cells == 1)
> > Warning (unit_address_vs_reg): /fragment@0/__overlay__: node has a reg
> > or ranges property, but no unit name
> > Warning (avoid_default_addr_size): /fragment@0/__overlay__: Relying on
> > default #address-cells value
> > Warning (avoid_default_addr_size): /fragment@0/__overlay__: Relying on
> > default #size-cells value
> >
> > We can't get the #address-cells/#size-cells of the parent of the
> > example node in the overlay structure.
> > Do you think we should change it to is_overlay_node(node) instead of
> > is_overlay_node(node->parent)?
> > Or we just need to skip checking for node names starting with "__" in
> > check_node_name_chars_strict()?
>
> I think this is the tip of the iceberg in terms of being able to
> validate overlays. Turning off address checks just kicks the problem
> to schema validation. Perhaps the overlay should be structured to
> include the parent bus node with #address-cells and #size-cells.
Right. So, I think to handle this properly we need to change the
structure of dtc:
* Rather than applying source level overlays as we parse them, we
should parse them each separately into a structure, then
internally apply them
* Checks would then need to be divided into those (A) that can be
checked on any individual overlay fragment, and those (B) that can
only be checked on a complete tree
* We'd run the (A) checks before merging the overlays in dtc, and
the (B) checks only after doing so.
* For runtime overlays (/plugin/) we'd skip the (B) checks entirely,
which would accomplish what you're aiming for here.
I had plans to make a restructure like this quite a while ago, but
I've never had the time to go ahead with it. If you want to give it a
go, that would be great.
--
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
