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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

> On 17 Nov 2016, at 02:53, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
> 
> 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
> 

No. This protocol does not describe pci devices, and it is a peculiarity of the edk2 driver stack that some non-pci devices can only be driven by pci drivers.

in other words, pci is part of the /driver/ side, and it is perfectly possible for, e.g., a non-discoverable ahci device to be driven by a different non-pci driver in the future.

>>> +
>>> +//
>>> +// 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.
> 

Again, i think this is a bad idea. This is meant to describe the /device/, not the edk2 implementation detail that some standardized host controller interfaces were implemented in a way that requires pci. It would also make it impossible to describe AMBA devices

>>> +  //
>>> +  // 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