On Tue, Jan 16, 2024 at 02:54:23PM +0200, Roger Quadros wrote:
> Hi David & Rob,
>
> On 14/05/2023 09:42, david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@xxxxxxxxxxxxxxxx wrote:
> > 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-NuS5LvNUpcJWk0Htik3J/w@xxxxxxxxxxxxxxxx> 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-NuS5LvNUpcJWk0Htik3J/w@xxxxxxxxxxxxxxxx>
> >>>>> ---
> >>>>> 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-DgEjT+Ai2ygdnm+yROfE0A@xxxxxxxxxxxxxxxx>
> >>>>
> >>>> 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.
> >
>
> FYI.
> I'm facing a somewhat similar issue with this patch [1]
> [1] https://lore.kernel.org/all/20230923080046.5373-2-rogerq@xxxxxxxxxx/
>
> arch/arm64/boot/dts/ti/k3-am642-evm-nand.dtso:65.8-140.3: Warning (avoid_default_addr_size): /fragment@3/__overlay__: Relying on default #address-cells value
> arch/arm64/boot/dts/ti/k3-am642-evm-nand.dtso:65.8-140.3: Warning (avoid_default_addr_size): /fragment@3/__overlay__: Relying on default #size-cells value
>
> To give some background:
>
> The GPMC block has separate address spaces per chip select.
>
> From Documentation/devicetree/bindings/memory-controllers/ti,gpmc.yaml
> ranges:
> minItems: 1
> description: |
> Must be set up to reflect the memory layout with four
> integer values for each chip-select line in use,
> <cs-number> 0 <physical address of mapping> <size>
>
> The ranges location in the device tree overlay is correct. The overlay is
> obviously meaningless without the base tree. It depends on the #address-cells
> and #size-cells defined in the base tree.
>
> Your proposal to fix this is valid but is definitely not trivial to fix
> especially for someone who is not familiar with dtc internals. :P
>
> Is there something simpler we could do to resolve this issue for overlay nodes.
> e.g. For overlay nodes we skip the "Relying on default address/size" check?
I think the overlay needs to define #address-cells/#size-cells because
otherwise the only way we can ever validate (not just with dtc, but any
tool) an overlay is by applying it. That of course means either you
have to change the overlay target to the parent node or allow something
like this to work:
#address-cells = <1>;
#size-cells = <0>;
&test {
reg = <0x1234>;
};
In this case, the sizes aren't really used, but serve as annotations for
the tools.
Rob
