[RFC 3/4] dtoc: add support for generate stuct udevice_id
Walter Lozano
walter.lozano at collabora.com
Fri Aug 7 19:23:22 CEST 2020
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.
Regards,
Walter
More information about the U-Boot
mailing list