Re: [PATCH] EmbeddedPkg/DwEmacSnpDxe: Add designware emac support This add support for designware emac controller

[Leif Lindholm <leif.lindholm@linaro.org>]

Hi Tzy Way,

On Thu, Feb 28, 2019 at 08:39:32AM +0000, Ooi, Tzy Way wrote:
> Thanks for your comment. I will modify the driver to comply to UEFI
> driver model.

Excellent, thanks.

> I would like to ask why this driver should be submitted to edk2-
> platforms instead of edk2? This driver is a generic driver which is
> target to work on various platform.

This was the decision we made when formalising the process for
bringing hardware support into TianoCore, whereas the edk2 repository
would strictly import support for industry-standard components.

With DesignWare, a case could perhaps be made that like ARM
PrimeCells, they exist in many different devices from many different
vendors. But if so, EmbeddedPkg is not their natural home.

Some of the drivers already under EmbeddedPkg clearly violate this
definition, but that is because they predate the definition of the
process.

Hmm, I probably should add a Readme.md about this in
EmbeddedPkg/Drivers.

Best Regards,

Leif

> Best regards,
> Tzy Way
> 
> 
> On Thu, 2019-01-31 at 18:22 +0000, Leif Lindholm wrote:
> > Hi Tzy Way,
> > 
> > Thank you for this contribution.
> > 
> > I do have some high-level comments.
> > 
> > First of all, my best guess is that you have used Lan9118Dxe for
> > reference when developing this driver. This is somewhat unfortunate.
> > I am reminded that
> > a) we badly need to migrate that driver (and Lan91xDxe) to
> >    edk2-platforms.
> > b) we badly need to convert those drivers to UEFI driver model and
> >    NonDiscoverableDeviceRegistrationLib.
> > Those two predate the NonDiscoverable implementation, so have been
> > left as is, but any new drivers really need to implement proper
> > driver
> > model.
> > Additionally, this driver should be submitted to edk2-platforms
> > rather
> > than edk2.
> > 
> > Secondly, searching online for "designware emac" does not find
> > unambigously the product this refers to. This is where it would be
> > usful with a proper commit message and explain in a bit more detail
> > what the driver is.
> > 
> > On Thu, Jan 31, 2019 at 04:32:00PM +0800, tzy.way.ooi@intel.com
> > wrote:
> > > From: "Ooi, Tzy Way" <tzy.way.ooi@intel.com>
> > > 
> > > Contributed-under: TianCore Contribution Agreement 1.1
> > > Signed-off-by: Ooi, Tzy Way <tzy.way.ooi@intel.com>
> > > ---
> > >  .../Drivers/DwEmacSnpDxe/DwEmacSnpDxe.c       | 1368
> > > +++++++++++++++++
> > >  .../Drivers/DwEmacSnpDxe/DwEmacSnpDxe.h       |  236 +++
> > >  .../Drivers/DwEmacSnpDxe/DwEmacSnpDxe.inf     |   69 +
> > >  .../Drivers/DwEmacSnpDxe/EmacDxeUtil.c        |  676 ++++++++
> > >  .../Drivers/DwEmacSnpDxe/EmacDxeUtil.h        |  378 +++++
> > >  EmbeddedPkg/Drivers/DwEmacSnpDxe/PhyDxeUtil.c |  604 ++++++++
> > >  EmbeddedPkg/Drivers/DwEmacSnpDxe/PhyDxeUtil.h |  324 ++++
> > >  EmbeddedPkg/EmbeddedPkg.dec                   |    4 +
> > >  EmbeddedPkg/EmbeddedPkg.dsc                   |    1 +
> > >  9 files changed, 3660 insertions(+)
> > 
> > Thirdly, please generate patches as described in
> > 
> https://github.com/tianocore/tianocore.github.io/wiki/Laszlo%27s-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-23
> > This greatly simplifies review.
> > 
> > Best Regards,
> > 
> > Leif