Re: [edk2-devel] [PATCH 0/2] Support FDT library.

["Michael D Kinney" <michael.d.kinney@intel.com>]

> -----Original Message-----
> From: Pedro Falcato <pedro.falcato@gmail.com>
> Sent: Thursday, March 30, 2023 4:27 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: devel@edk2.groups.io; Lin, Benny <benny.lin@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Liu, Zhiguang
> <zhiguang.liu@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Michael Kubacki <mikuback@linux.microsoft.com>; Leif
> Lindholm <llindhol@qti.qualcomm.com>
> Subject: Re: [edk2-devel] [PATCH 0/2] Support FDT library.
> 
> On Thu, Mar 30, 2023 at 11:59 PM Kinney, Michael D
> <michael.d.kinney@intel.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Pedro Falcato
> > > Sent: Thursday, March 30, 2023 2:50 PM
> > > To: devel@edk2.groups.io; Lin, Benny <benny.lin@intel.com>
> > > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Liu, Zhiguang
> > > <zhiguang.liu@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Michael Kubacki <mikuback@linux.microsoft.com>
> > > Subject: Re: [edk2-devel] [PATCH 0/2] Support FDT library.
> > >
> > > On Thu, Mar 30, 2023 at 6:13 PM Benny Lin <benny.lin@intel.com> wrote:
> > > >
> > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4392
> > > > Add FDT support in EDK2 by submodule 3rd party libfdt
> > > > (https://github.com/devicetree-org/pylibfdt/tree/main/libfdt)
> > > >
> > > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > > Cc: Liming Gao <gaoliming@byosoft.com.cn>
> > > > Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> > > > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > > > Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> > > > Signed-off-by: Benny Lin <benny.lin@intel.com>
> > > >
> > > > Benny Lin (2):
> > > >   MdePkg: Support FDT library.
> > > >   .pytool: Support FDT library.
> > > >
> > > >  .gitmodules                               |   3 +
> > > >  .pytool/CISettings.py                     |   2 +
> > > >  MdePkg/Include/Library/FdtLib.h           | 300 ++++++++++++++++++++++
> > > >  MdePkg/Library/BaseFdtLib/BaseFdtLib.inf  |  62 +++++
> > > >  MdePkg/Library/BaseFdtLib/BaseFdtLib.uni  |  14 +
> > > >  MdePkg/Library/BaseFdtLib/FdtLib.c        | 284 ++++++++++++++++++++
> > > >  MdePkg/Library/BaseFdtLib/LibFdtSupport.h | 102 ++++++++
> > > >  MdePkg/Library/BaseFdtLib/LibFdtWrapper.c | 138 ++++++++++
> > > >  MdePkg/Library/BaseFdtLib/libfdt          |   1 +
> > > >  MdePkg/Library/BaseFdtLib/limits.h        |  10 +
> > > >  MdePkg/Library/BaseFdtLib/stdbool.h       |  10 +
> > > >  MdePkg/Library/BaseFdtLib/stddef.h        |  10 +
> > > >  MdePkg/Library/BaseFdtLib/stdint.h        |  10 +
> > > >  MdePkg/Library/BaseFdtLib/stdlib.h        |  10 +
> > > >  MdePkg/Library/BaseFdtLib/string.h        |  10 +
> > > >  MdePkg/MdePkg.ci.yaml                     |  17 +-
> > > >  MdePkg/MdePkg.dec                         |   4 +
> > > >  MdePkg/MdePkg.dsc                         |   2 +
> > > >  ReadMe.rst                                |   1 +
> > > >  19 files changed, 988 insertions(+), 2 deletions(-)
> > > >  create mode 100644 MdePkg/Include/Library/FdtLib.h
> > > >  create mode 100644 MdePkg/Library/BaseFdtLib/BaseFdtLib.inf
> > > >  create mode 100644 MdePkg/Library/BaseFdtLib/BaseFdtLib.uni
> > > >  create mode 100644 MdePkg/Library/BaseFdtLib/FdtLib.c
> > > >  create mode 100644 MdePkg/Library/BaseFdtLib/LibFdtSupport.h
> > > >  create mode 100644 MdePkg/Library/BaseFdtLib/LibFdtWrapper.c
> > > >  create mode 160000 MdePkg/Library/BaseFdtLib/libfdt
> > > >  create mode 100644 MdePkg/Library/BaseFdtLib/limits.h
> > > >  create mode 100644 MdePkg/Library/BaseFdtLib/stdbool.h
> > > >  create mode 100644 MdePkg/Library/BaseFdtLib/stddef.h
> > > >  create mode 100644 MdePkg/Library/BaseFdtLib/stdint.h
> > > >  create mode 100644 MdePkg/Library/BaseFdtLib/stdlib.h
> > > >  create mode 100644 MdePkg/Library/BaseFdtLib/string.h
> > > >
> > > > --
> > > > 2.39.1.windows.1
> > >
> > > There's already a copy of libfdt plus "FdtLib" at
> > > https://github.com/tianocore/edk2/tree/master/EmbeddedPkg/Library/FdtLib.
> > > Please figure out what to do with it.
> > > It's an old copy and has been accidentally uncrustify'd so it's
> > > probably a good idea to at least ditch that specific copy for a git
> > > submodule.
> >
> > I have discussed this overlap with Leif. After this patch series is
> > added, the EmbeddedPkg maintainers can convert that package to use
> > this lib and delete the duplicate sources.
> Ok, SGTM.
> >
> > >
> > > Also, NAK to Yet Another libc Implementation (and not a particularly
> > > good one at that).
> >
> > Please provide constructive feedback on what is not good about this specific libc
> > Implementation so that appropriate code updates could be made for this patch series.
> 
> Done.
> > This is following the same pattern as other libs that are consuming a submodule
> > that requires some amount of libc support.
> >
> > Getting down to one libc wrapper would be great.  Do you want to provide a proposed
> > implementation?  That could be handled as a separate task.
> 
> I would like it if we could stop contributing to that problem. We very
> much *know* there is a problem with both libc fragments and compiler
> intrinsic fragments all over edk2.
> A proper, correct C standard library is not trivial to implement
> (hence the multiple problems I did find from a quick read of the
> patch).

Appreciate the feedback.  Agree that any libc API that is implemented in a
wrapper should conform to the standard.

> We also have a whole libc implementation in edk2-libc that seems to be
> permanently gathering dust until Intel touches it for Python purposes
> from time to time. So between crypto, libfdt, etc, could we try to
> unify things here a bit?

edk2-libc to too large for FW components and it is out of date.

Would you like to volunteer to lead a design and implementation that
is right-sized for FW modules and could be the one wrapper that works
for all current use cases and could be extended in the future as 
needed to support additional use cases?  Don’t need all of libc.  Just
enough to support the APIs used by the submodules used so far.

> 
> Thanks,
> Pedro