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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#43255): https://edk2.groups.io/g/devel/message/43255 Mute This Topic: https://groups.io/mt/32269485/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
