[PATCH 1/2] dm: core: Add functions to read 8/16-bit integers

[Stefan Herbrechtsmeier]

Hi Heinrich,

Am 14.07.2022 um 14:58 schrieb Heinrich Schuchardt:
> On 7/14/22 12:37, Stefan Herbrechtsmeier wrote:
>> Hi Simon,
>>
>> Am 14.07.2022 um 12:22 schrieb Simon Glass:
>>> Hi Stefan,
>>>
>>> On Wed, 13 Jul 2022 at 10:08, Stefan Herbrechtsmeier
>>> <stefan.herbrechtsmeier-oss at weidmueller.com> wrote:
>>>>
>>>> Hi Simon,
>>>>
>>>> Am 13.07.2022 um 17:28 schrieb Simon Glass:
>>>>> Hi Stefan,
>>>>>
>>>>> On Tue, 12 Jul 2022 at 12:31, Stefan Herbrechtsmeier
>>>>> <stefan.herbrechtsmeier-oss at weidmueller.com> wrote:
>>>>>>
>>>>>> Hi Simon,
>>>>>>
>>>>>> Am 12.07.2022 um 12:58 schrieb Simon Glass:
>>>>>>> Hi Stefan,
>>>>>>>
>>>>>>> On Tue, 14 Jun 2022 at 07:22, Stefan Herbrechtsmeier
>>>>>>> <stefan.herbrechtsmeier-oss at weidmueller.com> wrote:
>>>>>>>>
>>>>>>>> From: Stefan Herbrechtsmeier
>>>>>>>> <stefan.herbrechtsmeier at weidmueller.com>
>>>>>>>>
>>>>>>>> Add functions to read 8/16-bit integers like the existing
>>>>>>>> functions for
>>>>>>>> 32/64-bit to simplify read of 8/16-bit integers from device tree
>>>>>>>> properties.
>>>>>>>>
>>>>>>>> Signed-off-by: Stefan Herbrechtsmeier
>>>>>>>> <stefan.herbrechtsmeier at weidmueller.com>
>>>>>>>> ---
>>>>>>>>
>>>>>>>>     arch/sandbox/dts/test.dts |  2 ++
>>>>>>>>     drivers/core/of_access.c  | 38 +++++++++++++++++++++++
>>>>>>>>     drivers/core/ofnode.c     | 62
>>>>>>>> +++++++++++++++++++++++++++++++++++++
>>>>>>>>     drivers/core/read.c       | 21 +++++++++++++
>>>>>>>>     include/dm/of_access.h    | 32 +++++++++++++++++++
>>>>>>>>     include/dm/ofnode.h       | 40 ++++++++++++++++++++++++
>>>>>>>>     include/dm/read.h         | 65
>>>>>>>> +++++++++++++++++++++++++++++++++++++++
>>>>>>>>     test/dm/test-fdt.c        | 19 ++++++++++++
>>>>>>>>     8 files changed, 279 insertions(+)
>>>>>>>
>>>>>>> This looks good but is very expensive in terms of code size. Can you
>>>>>>> update your u8 and u16 functions to reuse the existing u32 function
>>>>>>> and just convert the value?
>>>>>>
>>>>>> The u32 function requires a 32 bit value inside the device tree
>>>>>> because
>>>>>> it checks the size and maybe swap the bytes.
>>>>>>
>>>>>> The u8 and u16 function requires only a 8 and 16 bit value inside the
>>>>>> device tree.
>>>>>
>>>>> Yes that's true. What is the use case for these functions?
>>>>
>>>> The usb251xb driver requires this functions [1]. The usb251xb device
>>>> tree binding [2] defines the ids as 16 bit values and the Linux driver
>>>> use 8 bit for an unspecified property. Without this changes the driver
>>>> doesn't satisfy the specification and is incompatible to the Linux
>>>> driver.
>>>
>>> I wonder if that binding is a bit ambiguous. From what I have seen we
>>> normally use a single cell for int values, partly so that fdtdump
>>> works and partly because the format doesn't allow using any less space
>>> anyway.
>>>
>>> IMO that binding should use a whole cell for the byte and u16 values.
>>
>> How should we go on? The specification is 5 years old. I can ignore the
>> specification  and remove the "/bits/ 16" from my device tree source.
> 
> We will not change the binding due to a deficiency of U-Boot.
> 
> For the 8 bit field:
> 
> The length field is set to 1 byte.
> 3 zero bytes are following for alignment.
> 
> We tend to check the length field.
> 
> We could reduce the number functions if on most levels of function
> indirection we would pass a size field.
> 
> E.g. replace of_read_u*_array() by macros all invoking the same function
> with different values (1, 2, 4, 8) of the element size parameter.

This doesn't help because you have to adapt the byte order and on most 
levels the functions are simple wrappers.

The rework isn't easy because we have a combination of wrappers and 
duplicate implementations. Thereby the ofnode functions are wrappers and 
implementations at the same time:

dev_read_u32
-> ofnode_read_u32
    -> ofnode_read_u32_index
       - if ofnode_is_np
         -> of_read_u32_index
            -> of_find_property_value_of_size
            -> be32_to_cpup
       - else
         -> fdt_getprop
         -> fdt32_to_cpu

We need to remove the second implementations (of_read_) to reduce the size.

Regards
   Stefan