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

[Simon Glass]

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.

Regards,
SImon