From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=Bm+w94Jq; spf=pass (domain: linaro.org, ip: 209.85.221.66, mailfrom: leif.lindholm@linaro.org) Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) by groups.io with SMTP; Thu, 04 Jul 2019 02:12:01 -0700 Received: by mail-wr1-f66.google.com with SMTP id x4so5761123wrt.6 for ; Thu, 04 Jul 2019 02:12:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=sDZfUMfpHbqGYXYEq9nlHwVv64rx2kCTAoQ0hKu2S1U=; b=Bm+w94JqHrl4BxyHEcCw/qdZBUgI+ILRhwidBOvKAHAm9XGzH+cKq0nmdOQdr1w3O2 MbFg5OEvn1dja91nSFID9qdRYpL+3koE99q8aePXHZulj9lf9Hhat8oT2ZG8BxbHMBxO vpFNuyANw1J3G39f22FHjtPso3MAo38pcqXwlyIOIkcZ7AuFSvmVFA+7mdbOesNOrp9K O7Yi6ZYYdlpSaYttBd0Pq3tm8PIqSlkMn/ozutQ4aG2k1+VGZIr1dzAlmiDCaHcEH7+y manxwFIXo2ShLKnWYrGO53dWhXOC5BjEF5Dp5lXe9IMAQpJysTkSs/eaGLYcUUsXAX43 k8ew== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=sDZfUMfpHbqGYXYEq9nlHwVv64rx2kCTAoQ0hKu2S1U=; b=d8JvhzpizS0/JItmlmvXLa9lwshVrZbB9r/0M3ZdG3is3NBArYOdU2WYPrwduhIZbV Q4cd3xlLqI3jzm0FlJjxK0DFQ+X7+LZvGRHkscXpKd0ROPMn7e0+pZI7m5VbvP78Occg jVeDCstLkKOHB/GNijvDqmNG8gDnGcchiUI9oKgYkzx13lXxJc1O32x0Azz3/ZydYvkI LFTugoh15uqwaEWdsJrxb5WgXyBaJQDF7zxqxI9f8+z9k7TfS3CPvy40x2vugbrdafSI uWGDHK0Eha02lOpUy0ADT2wYk1ubl0WXfm+1zzeRq4gfazVsYNQw7bZEMHa7fHUdFv6+ 5QqQ== X-Gm-Message-State: APjAAAUZMAQLMKuK0ZKcVzqUP3D+gOXOthQ49VciL1zIZ7xbk32jdXb0 DLb3rmiJOajf2hfWxar0sbKcqW3DIt4= X-Google-Smtp-Source: APXvYqxpBIXGTBr4x7UWRQgM7Ua83e2yTwlzKbUhBbsjXMqZZDuOOhMl2q83IwcV7PdaZiacswWTjA== X-Received: by 2002:a5d:4647:: with SMTP id j7mr35187169wrs.334.1562231518970; Thu, 04 Jul 2019 02:11:58 -0700 (PDT) Return-Path: Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id s25sm652239wmc.21.2019.07.04.02.11.57 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 04 Jul 2019 02:11:58 -0700 (PDT) Date: Thu, 4 Jul 2019 10:11:56 +0100 From: "Leif Lindholm" To: devel@edk2.groups.io, tzy.way.ooi@intel.com Cc: Ard BieSheuvel , "Kinney, Michael D" , "Loh, Tien Hock" Subject: Re: [edk2-devel] [PATCH v5 edk2-platforms 1/1] Silicon/DesignWare/Driver: DwEmacSnpDxe: Add DesignWare EMAC driver Message-ID: <20190704091156.ftetslmwqivvdywj@bivouac.eciton.net> References: <20190701085517.2587-1-tzy.way.ooi@intel.com> <20190703200846.eedetm6chvzt2zfv@bivouac.eciton.net> <5F1105621EDF844291AF8B109E27C06D34D28B03@PGSMSX109.gar.corp.intel.com> MIME-Version: 1.0 In-Reply-To: <5F1105621EDF844291AF8B109E27C06D34D28B03@PGSMSX109.gar.corp.intel.com> User-Agent: NeoMutt/20170113 (1.7.2) Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit 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 > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > >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