From mboxrd@z Thu Jan 1 00:00:00 1970 From: Walter Lozano Date: Wed, 29 Jul 2020 13:00:34 -0300 Subject: [RFC 3/4] dtoc: add support for generate stuct udevice_id In-Reply-To: References: <20200619211140.5081-1-walter.lozano@collabora.com> <20200619211140.5081-4-walter.lozano@collabora.com> <7a17bbb9-a537-09a8-8d18-3f7438ad818d@collabora.com> <7eab15a5-ebac-f9f6-184e-a77d47578837@collabora.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Simon, On 28/7/20 23:42, Simon Glass wrote: > Hi Walter, > > On Sun, 26 Jul 2020 at 20:16, Walter Lozano wrote: >> Hi Simon, >> >> On 26/7/20 11:53, Simon Glass wrote: >>> Hi Walter, >>> >>> On Tue, 7 Jul 2020 at 08:08, Walter Lozano wrote: >>>> Hi Simon >>>> >>>> On 6/7/20 16:21, Simon Glass wrote: >>>>> Hi Walter, >>>>> >>>>> On Fri, 19 Jun 2020 at 15:12, Walter Lozano 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 at 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 at denx.de/ >>>>>> >>>>>> Signed-off-by: Walter Lozano >>>>>> --- >>>>>> 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