From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Sun, 6 Sep 2020 19:44:16 -0600 Subject: [RFC 3/4] dtoc: add support for generate stuct udevice_id In-Reply-To: <9adeec31-984c-362e-b23e-5cbd07c2b811@collabora.com> 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> <9adeec31-984c-362e-b23e-5cbd07c2b811@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 Walter, On Fri, 7 Aug 2020 at 11:23, Walter Lozano wrote: > > Hi Simon > > On 7/8/20 13:23, Simon Glass wrote: > > Hi Walter, > > > > On Wed, 29 Jul 2020 at 10:00, Walter Lozano wrote: > >> 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. > > 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