On 22/01/2024 18:01, Rob Herring wrote:
> 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.
But one address/size-cells might not fit all cases in an overlay as the
nodes in the overlay may have parents with different address/size-cells.
--
cheers,
-roger
