[RFC 3/4] dtoc: add support for generate stuct udevice_id
Walter Lozano
walter.lozano at collabora.com
Mon Sep 7 21:10:32 CEST 2020
Hi Simon,
On 6/9/20 22:44, Simon Glass wrote:
> Hi Walter,
>
> On Fri, 7 Aug 2020 at 11:23, Walter Lozano <walter.lozano at collabora.com> wrote:
>> Hi Simon
>>
>> On 7/8/20 13:23, Simon Glass wrote:
>>> Hi Walter,
>>>
>>> On Wed, 29 Jul 2020 at 10:00, Walter Lozano <walter.lozano at collabora.com> wrote:
>>>> 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.
>>> Yes I hit that problem with the tiny-dm experiment and ended up adding
>>> a macro to specify the header.
>> I haven't seen that. I will check it. Thanks.
>>
>>
>>> Do you need FSL_ESDHC_IMX_V2? Is it just to avoid a warning about
>>> usdhc_imx7d_data not being used? If so, we could use _maybe_unused
>> Well, it is not that I really need it, but I try to give the possibility
>> to add some #ifdef or similar based on compatible strings, the
>> usdhc_imx7d_data was just an example. A more interesting example could
>> be some code that makes sense only on specific "compatible string" cases
>> and using #ifdef or if would save some bytes in other cases.
>>
>>>> 2- it case we use #define to avoid having to include several different
>>>> .h probably the errors will be more difficult to catch/debug
>>> Yes we would have to include the real header, not just copy bits out of it.
>> Yes, for that reason I feel it could lead to more issues than benefits.
>> However, it is only a personal opinion, I'm not completely sure.
>>
>>
>>>> What do you think?
>>> I'm not sure overall. On the one hand I don't really like hiding C
>>> code inside macros. On the other, it avoids the horrible manual
>>> #ifdefs. So on balance I think your idea is the best approach. We can
>>> always refine it later and it is easier to iterate on this sort of
>>> thing if it is actually being used by some boards.
>>
>> It is exactly what I feel, we need to find the best balance here, and it
>> always easy to improve it if this is used by some boards.
>>
>>>>>> 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.
>> Thanks for your review and comments.
>>
>> BTW, as this work is based in some of the improvements you developed for
>> tiny-dm, I was wondering what are your plans regarding it.
> I haven't had a lot of time recently. Don't let my side hold you up,
> though. I am hoping to look at the platdata parent stuff soon.
Thanks for the warning. I have planed to send a patch for this but I
have also been a bit busy. However, I hope to have some time this week
to prepare and send it.
Regards,
Walter
More information about the U-Boot
mailing list