public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Leif Lindholm" <leif.lindholm@linaro.org>
To: devel@edk2.groups.io, tzy.way.ooi@intel.com
Cc: Ard BieSheuvel <ard.biesheuvel@linaro.org>,
	"Kinney, Michael D" <michael.d.kinney@intel.com>,
	"Loh, Tien Hock" <tien.hock.loh@intel.com>
Subject: Re: [edk2-devel] [PATCH v5 edk2-platforms 1/1] Silicon/DesignWare/Driver: DwEmacSnpDxe: Add DesignWare EMAC driver
Date: Thu, 4 Jul 2019 10:11:56 +0100	[thread overview]
Message-ID: <20190704091156.ftetslmwqivvdywj@bivouac.eciton.net> (raw)
In-Reply-To: <5F1105621EDF844291AF8B109E27C06D34D28B03@PGSMSX109.gar.corp.intel.com>

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

      reply	other threads:[~2019-07-04  9:12 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-01  8:55 [PATCH v5 edk2-platforms 1/1] Silicon/DesignWare/Driver: DwEmacSnpDxe: Add DesignWare EMAC driver Ooi, Tzy Way
2019-07-03 20:08 ` Leif Lindholm
2019-07-04  6:17   ` [edk2-devel] " Ooi, Tzy Way
2019-07-04  9:11     ` Leif Lindholm [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190704091156.ftetslmwqivvdywj@bivouac.eciton.net \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox