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

[Simon Glass]

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.

> 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.

>
> 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.

Regards,
Simon