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?
diff --git a/scripts/dtc/checks.c b/scripts/dtc/checks.c
index 9f31d2607182..a4e94c4b7b08 100644
--- a/scripts/dtc/checks.c
+++ b/scripts/dtc/checks.c
@@ -1197,6 +1197,9 @@ static void check_avoid_default_addr_size(struct check *c, struct dt_info *dti,
if (!node->parent)
return; /* Ignore root node */
+ if (streq(node->name, "__overlay__"))
+ return; /* Ignore overlay nodes */
+
reg = get_property(node, "reg");
ranges = get_property(node, "ranges");
--
cheers,
-roger
