Re: [PATCH v3 1/5] MdeModulePkg: introduce non-discoverable device protocol

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

Ard,
I have two comments in below.

Thanks/Ray

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Leif Lindholm
> Sent: Thursday, November 17, 2016 1:49 AM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org;
> afish@apple.com; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: Re: [edk2] [PATCH v3 1/5] MdeModulePkg: introduce non-
> discoverable device protocol
> 
> On Wed, Nov 16, 2016 at 04:59:27PM +0000, Ard Biesheuvel wrote:
> > Introduce a protocol that can be exposed by a platform for devices
> > that are not discoverable, usually because they are wired straight to
> > the memory bus rather than to an enumerable bus like PCI or USB.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  MdeModulePkg/Include/Protocol/NonDiscoverableDevice.h | 90
> ++++++++++++++++++++
> >  MdeModulePkg/MdeModulePkg.dec                         |  3 +
> >  2 files changed, 93 insertions(+)
> >
> > diff --git a/MdeModulePkg/Include/Protocol/NonDiscoverableDevice.h
> > b/MdeModulePkg/Include/Protocol/NonDiscoverableDevice.h
> > new file mode 100644
> > index 000000000000..47ed841b407b
> > --- /dev/null
> > +++ b/MdeModulePkg/Include/Protocol/NonDiscoverableDevice.h
> > @@ -0,0 +1,90 @@
> > +/** @file
> > +  Protocol to describe devices that are not on a discoverable bus
> > +
> > +  Copyright (c) 2016, Linaro, Ltd. All rights reserved.<BR>
> > +
> > +  This program and the accompanying materials  are licensed and made
> > + available under the terms and conditions of the BSD License  which
> > + accompanies this distribution.  The full text of the license may be
> > + found at  http://opensource.org/licenses/bsd-license.php
> > +
> > +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
> > + BASIS,  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
> EITHER EXPRESS OR IMPLIED.
> > +
> > +**/
> > +
> > +#ifndef __NON_DISCOVERABLE_DEVICE_H__ #define
> > +__NON_DISCOVERABLE_DEVICE_H__
> > +
> > +#include <IndustryStandard/Acpi.h>
> > +
> > +#define EDKII_NON_DISCOVERABLE_DEVICE_PROTOCOL_GUID \
> > +  { 0x0d51905b, 0xb77e, 0x452a, {0xa2, 0xc0, 0xec, 0xa0, 0xcc, 0x8d,
> > +0x51, 0x4a } }

1. Can you add "PCI" keyword into the protocol name?
e.g.: EDKII_NON_DISCOVERABLE_PCI_DEVICE_PROTOCOL_GUID

> > +
> > +//
> > +// Protocol interface structure
> > +//
> > +typedef struct _NON_DISCOVERABLE_DEVICE
> NON_DISCOVERABLE_DEVICE;
> > +
> > +//
> > +// Data Types
> > +//
> > +typedef enum {
> > +  NonDiscoverableDeviceTypeAmba,
> > +  NonDiscoverableDeviceTypeOhci,
> > +  NonDiscoverableDeviceTypeUhci,
> > +  NonDiscoverableDeviceTypeEhci,
> > +  NonDiscoverableDeviceTypeXhci,
> > +  NonDiscoverableDeviceTypeAhci,
> > +  NonDiscoverableDeviceTypeSdhci,
> > +  NonDiscoverableDeviceTypeUfs,
> > +  NonDiscoverableDeviceTypeNvme,


> 
> Just one OCD comment/question left:
> Can we keep these sorted alphabetically?
> (Also in switch statements in later patches?)
> 
> Other than that, I'm (very) happy with this series.
> 
> /
>     Leif
> 
> > +  NonDiscoverableDeviceTypeMax,
> > +} NON_DISCOVERABLE_DEVICE_TYPE;
> > +
> > +typedef enum {
> > +  NonDiscoverableDeviceDmaTypeCoherent,
> > +  NonDiscoverableDeviceDmaTypeNonCoherent,
> > +  NonDiscoverableDeviceDmaTypeMax,
> > +} NON_DISCOVERABLE_DEVICE_DMA_TYPE;
> > +
> > +//
> > +// Function Prototypes
> > +//
> > +
> > +/**
> > +  Perform device specific initialization before the device is started
> > +
> > +  @param  This          The non-discoverable device protocol pointer
> > +
> > +  @retval EFI_SUCCESS   Initialization successful, the device may be used
> > +  @retval Other         Initialization failed, device should not be started
> > +**/
> > +typedef
> > +EFI_STATUS
> > +(EFIAPI *NON_DISCOVERABLE_DEVICE_INIT) (
> > +  IN  NON_DISCOVERABLE_DEVICE       *This
> > +  );
> > +
> > +struct _NON_DISCOVERABLE_DEVICE {
> > +  //
> > +  // The type of device
> > +  //
> > +  NON_DISCOVERABLE_DEVICE_TYPE        Type;
2. Can you use PCI class code to replace the enum type here?
e.g.: UINT8 Class; UINT8 SubClass; UINT8 Programming Interface;
The enum type can be defined in the helper library.
In this way, we make the protocol definition stable enough.

> > +  //
> > +  // Whether this device is DMA coherent
> > +  //
> > +  NON_DISCOVERABLE_DEVICE_DMA_TYPE    DmaType;
> > +  //
> > +  // Initialization function for the device
> > +  //
> > +  NON_DISCOVERABLE_DEVICE_INIT        Initialize;
> > +  //
> > +  // The MMIO and I/O regions owned by the device
> > +  //
> > +  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR   *Resources;
> > +};
> > +
> > +extern EFI_GUID gEdkiiNonDiscoverableDeviceProtocolGuid;
> > +
> > +#endif
> > diff --git a/MdeModulePkg/MdeModulePkg.dec
> > b/MdeModulePkg/MdeModulePkg.dec index 74b870051c67..6b956fc80c93
> > 100644
> > --- a/MdeModulePkg/MdeModulePkg.dec
> > +++ b/MdeModulePkg/MdeModulePkg.dec
> > @@ -505,6 +505,9 @@ [Protocols]
> >    #  Include/Protocol/Ps2Policy.h
> >    gEfiPs2PolicyProtocolGuid = { 0x4DF19259, 0xDC71, 0x4D46, { 0xBE,
> > 0xF1, 0x35, 0x7B, 0xB5, 0x78, 0xC4, 0x18 } }
> >
> > +  ## Include/Protocol/NonDiscoverableDevice.h
> > +  gEdkiiNonDiscoverableDeviceProtocolGuid = { 0x0d51905b, 0xb77e,
> > + 0x452a, {0xa2, 0xc0, 0xec, 0xa0, 0xcc, 0x8d, 0x51, 0x4a } }
> > +
> >  #
> >  # [Error.gEfiMdeModulePkgTokenSpaceGuid]
> >  #   0x80000001 | Invalid value provided.
> > --
> > 2.7.4
> >
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel