From: Rob Herring <robh@kernel.org>
To: Roger Quadros <rogerq@kernel.org>
Cc: david@gibson.dropbear.id.au,
"Qun-wei Lin (林群崴)"
<Qun-wei.Lin-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>,
"Casper Li (李中榮)"
<casper.li-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>,
"Kuan-Ying Lee (李冠穎)"
<Kuan-Ying.Lee-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>,
"Chinwen Chang (張錦文)"
<chinwen.chang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>,
"Ivan Tseng (曾志軒)"
<ivan.tseng-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>,
devicetree-compiler@vger.kernel.org, "Bajjuri,
Praneeth" <praneeth@ti.com>
Subject: Re: [PATCH v2] checks: Suppress warnings on overlay fragments
Date: Mon, 22 Jan 2024 10:01:33 -0600 [thread overview]
Message-ID: <20240122160133.GD601827-robh@kernel.org> (raw)
In-Reply-To: <a1f53131-f436-4998-b370-4956557818fd@kernel.org>
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+@public.gmane.org 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@public.gmane.org> 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@mediatek.com>
> >>>> 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@public.gmane.org>
> >>>>> ---
> >>>>> 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@public.gmane.org>
> >>>>
> >>>> 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@kernel.org/
>
> 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
next prev parent reply other threads:[~2024-01-22 16:01 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-08 9:15 [PATCH v2] checks: Suppress warnings on overlay fragments Qun-Wei Lin
[not found] ` <20230308091539.11178-1-qun-wei.lin-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
2023-03-09 15:12 ` Rob Herring
[not found] ` <CAL_JsqKTM=gaQGZhrBCRkBusYYMci0mJGAFf9RTvCx00G2OJzg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2023-04-13 12:43 ` Qun-wei Lin (林群崴)
[not found] ` <347151917fb777e66a54e983efbbe0303f63e01a.camel-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
2023-04-18 17:33 ` Rob Herring
[not found] ` <CAL_Jsq+L50RZ-s55tncFMHA1AwL5An13n2OVPzknnpu=uOvzhQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2023-05-14 6:42 ` david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+
2024-01-16 12:54 ` Roger Quadros
2024-01-22 16:01 ` Rob Herring [this message]
2024-01-23 14:49 ` Roger Quadros
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240122160133.GD601827-robh@kernel.org \
--to=robh@kernel.org \
--cc=Kuan-Ying.Lee-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org \
--cc=Qun-wei.Lin-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org \
--cc=casper.li-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org \
--cc=chinwen.chang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org \
--cc=david@gibson.dropbear.id.au \
--cc=devicetree-compiler@vger.kernel.org \
--cc=ivan.tseng-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org \
--cc=praneeth@ti.com \
--cc=rogerq@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).