Re: [edk2-devel] [PATCH v5 edk2-platforms 1/1] Silicon/DesignWare/Driver: DwEmacSnpDxe: Add DesignWare EMAC driver

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

Hi Tzy Way,

On Thu, Jul 04, 2019 at 06:17:25AM +0000, Ooi, Tzy Way wrote:
> Thank you for reviewing the source code. I will change the source
> code according to the comment. I would like to confirm with you
> about my understanding about the comment as shown below:
> 
> == 1 ==
> >>- Use recent version for EDK2 specific file formats
> >This does not appear to be the case. Have you sent out the correct
> revision?
> 
> Tzy Way: Sorry, this is my fault. I misunderstand Ard's comment. I
> thought I should change the format to 1.xx instead of using
> 0xXXXXXXX to become recent format. May I confirm with you the recent
> version is 1.27?

I can never remember, so I tend to go have a look at
https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Specifications,
which lists the latest revisions of everything.

(I also can never remember the URL to that, so I search for 'edk2 inf
specification', which tends to give it as the first hit.)

> == 2 ==
> > +// Libraries used by this driver
> > +#include <Library/UefiLib.h>
> > +#include <Library/DebugLib.h>
> > +#include <Library/UefiBootServicesTableLib.h>
> > +#include <Library/MemoryAllocationLib.h>
> > +#include <Library/NetLib.h>
> > +#include <Library/DevicePathLib.h>
> > +#include <Library/DmaLib.h>
> 
> >I get the impression both here and from the .c files below that these
> >include all the headers required by the .c files, as opposed to the
> >ones required for definitions in here.
> >That hides useful information when looking through the source code.
> >Please include in .h files only the headers required for definitions,
> >and let the .c files declare their own includes.
> >(This applies to all .h files in this patch.)
> 
> Tzy Way: The above include files are needed by the file
> DwEmacSnpDxe.c. Sorry that I couldn't get what you are trying to
> explain. Would you mind to explain it to me again? Do you mean I
> should place the library include to .c file?

Yes please.
It is helpful to see in each .c file which interfaces it actually
makes use of.

> == 3 ==
> > +typedef struct {
> > +  UINT32 Tdes0;
> > +  UINT32 Tdes1;
> > +  UINT32 Addr;
>  
> >32-bit addresses? Is this a hardware limitation? This could be
> >problematic on some platforms.
> 
> Tzy Way: The Addr is refer to the buffer address. According to the
> user guide for the EMAC controller, it is stated as 32 bits. Hence,
> it is coded as 32 bit addresses. Is it ok to maintain it?

Of course. Thank you for confirming.
It just means this controller can only be used in 64-bit systems if it
is behind an iommu.

> == 4 ==
> >Also, PHY_DXE is too generic an include guard.
> >Please add DW_EMAC_ or something to prevent accidental future clashes.
> 
> Tzy Way: Is it ok if I named it as KSZ9031_PHY_DXE?

Yes, totally fine.

> == 5 ==
> 
> > +  Status = DmaAllocateBuffer (EfiBootServicesData,
> > +             EFI_SIZE_TO_PAGES (sizeof (DESIGNWARE_HW_DESCRIPTOR)), (VOID*)&Snp->MacDriver.TxdescRing[Index]);
> > +  if (EFI_ERROR (Status)) {
> > +    DEBUG ((DEBUG_ERROR, "%a () for TxdescRing: %r\n", __FUNCTION__, Status));
> > +    return Status
> 
> Please indent whole block.
> 
> Tzy Way: May I confirm with you where indent whole block means I should change to the format as shown below:
> 
> Status = DmaAllocateBuffer (EfiBootServicesData,
>                                                     EFI_SIZE_TO_PAGES (sizeof (DESIGNWARE_HW_DESCRIPTOR)), 
>                                                     (VOID*)&Snp->MacDriver.TxdescRing[Index]);

The comment referred to the body of the for loop this snippet is part
of, beginning just before it:

+  //DMA TxdescRing allocate buffer and map
+  for (int Index=0; Index < DESC_NUM; Index++) {

The whole body of this for loop is missing indentation, including the
bit you include above. I indicated the block with

  Misleading indentation from here...
  ... to here.

Best Regards,

Leif