[RFC 3/4] dtoc: add support for generate stuct udevice_id

[Walter Lozano]

Hi Simon,

On 28/7/20 23:42, Simon Glass wrote:
> Hi Walter,
>
> On Sun, 26 Jul 2020 at 20:16, Walter Lozano <walter.lozano at collabora.com> wrote:
>> Hi Simon,
>>
>> On 26/7/20 11:53, Simon Glass wrote:
>>> Hi Walter,
>>>
>>> On Tue, 7 Jul 2020 at 08:08, Walter Lozano <walter.lozano at collabora.com> wrote:
>>>> Hi Simon
>>>>
>>>> On 6/7/20 16:21, Simon Glass wrote:
>>>>> Hi Walter,
>>>>>
>>>>> On Fri, 19 Jun 2020 at 15:12, Walter Lozano <walter.lozano at collabora.com> wrote:
>>>>>> Based on several reports there is an increasing concern in the impact
>>>>>> of adding additional features to drivers based on compatible strings.
>>>>>> A good example of this situation is found in [1].
>>>>>>
>>>>>> In order to reduce this impact and as an initial step for further
>>>>>> reduction, propose a new way to declare compatible strings, which allows
>>>>>> to only include the useful ones.
>>>>> What are the useful ones?
>>>> The useful ones would be those that are used by the selected DTB by the
>>>> current configuration. The idea of this patch is to declare all the
>>>> possible compatible strings in a way that dtoc can generate code for
>>>> only those which are going to be used, and in this way avoid lots of
>>>> #ifdef like the ones shows in
>>>>
>>>> http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust@denx.de/
>>>>
>>>>
>>>>>> The idea is to define compatible strings in a way to be easily parsed by
>>>>>> dtoc, which will be responsible to build struct udevice_id [] based on
>>>>>> the compatible strings present in the dtb.
>>>>>>
>>>>>> Additional features can be easily added, such as define constants
>>>>>> depending on the presence of compatible strings, which allows to enable
>>>>>> code blocks only in such cases without the need of adding additional
>>>>>> configuration options.
>>>>>>
>>>>>> [1] http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust@denx.de/
>>>>>>
>>>>>> Signed-off-by: Walter Lozano <walter.lozano at collabora.com>
>>>>>> ---
>>>>>>     tools/dtoc/dtb_platdata.py | 32 ++++++++++++++++++++++++++++++++
>>>>>>     1 file changed, 32 insertions(+)
>>>>> I think dtoc should be able to parse the compatible strings as they
>>>>> are today - e.g. see the tiny-dm stuff.
>>>> Yes, I agree. My idea is that dtoc parses compatible strings as they are
>>>> today but also in this new way. The reason for this is to allow dtoc to
>>>> generate the code to include the useful compatible strings. Of course,
>>>> this only makes sense if the idea of generating the compatible string
>>>> associated  code is accepted.
>>>>
>>>> What do you think?
>>> I think this is useful and better than using #ifdef in the source code
>>> for this sort of thing. We need a way to specify the driver_data value
>>> as well, right?
>> Yes, I agree, it is better than #ifdef and c/ould give us some extra
>> functionality.
>>
>> What doe you mean by driver_data value? Are you referring to the data
>> field? like
>>
>> static struct esdhc_soc_data usdhc_imx7d_data = {
>>           .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
>>                           | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
>>                           | ESDHC_FLAG_HS400,
>> };
>>
> Actually I was talking about the .data member in struct udevice_id.
So my example is correct, as usdhc_imx7d_data is the value for .data in 
one case as shown bellow.
>> If that is the case, I was thinking in defining a constant when specific
>> compatible strings are enabled by dtoc, based in the above case
>>
>> #ifdef FSL_ESDHC_IMX_V2
>> static struct esdhc_soc_data usdhc_imx7d_data = {
>>           .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
>>                           | ESDHC_FLAG_HAVE_CAP1 | ESDHC_FLAG_HS200
>>                           | ESDHC_FLAG_HS400,
>> };
>> #endif
>>
>> COMPATIBLE(FSL_ESDHC, "fsl,imx7d-usdhc", &usdhc_imx7d_data, FSL_ESDHC_IMX_V2)
>>
>> So when dtoc parses COMPATIBLE and determines that compatible
>> "fsl,imx7d-usdhc" should be added it also defines FSL_ESDHC_IMX_V2.
> I think we can put that data in the dt-platdata.c file perhaps.

I thought the same at the beginning, but then I changed my mind, because

1- in order to work dt-platdata.c will need to include several different 
.h, in this example, only for fsl_esdhc_imx to work, we will need to 
include fsl_esdhc_imx.h where all the flags are defined.

2- it case we use #define to avoid having to include several different 
.h probably the errors will be more difficult to catch/debug

What do you think?

>
>> This is alsoAs I comment you in the tread about tiny-dm I think that we
>> can save some space following your suggestions, and for instance implement
>>
>>
>>> Re naming, perhaps DT_COMPAT() might be better than COMPATIBLE()? Or
>>> even a name that indicates that it is optional, like DT_OPT_COMPAT() ?
>>>
>> I totally agree, naming is very important, and DT_COMPAT() is much better.
>>
>> What I don't fully understand is what are the cases for DT_OPT_COMPAT(),
>> could you please clarify?
> It's just an alternative name, with OPT meaning optional. But I think
> we can leave out the OPT.

Thanks for clarifying.

Regards,

Walter