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

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

Regards,
Ray

>-----Original Message-----
>From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>Sent: Thursday, November 17, 2016 6:43 PM
>To: Ni, Ruiyu <ruiyu.ni@intel.com>
>Cc: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>;
>afish@apple.com; Leif Lindholm <leif.lindholm@linaro.org>
>Subject: Re: [edk2] [PATCH v3 1/5] MdeModulePkg: introduce non-discoverable device protocol
>
>
>> On 17 Nov 2016, at 08:52, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
>>
>>
>>
>> Thanks/Ray
>>
>>> -----Original Message-----
>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>>> Ard Biesheuvel
>>> Sent: Thursday, November 17, 2016 2:07 PM
>>> To: Ni, Ruiyu <ruiyu.ni@intel.com>
>>> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-
>>> devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; afish@apple.com;
>>> Leif Lindholm <leif.lindholm@linaro.org>
>>> Subject: Re: [edk2] [PATCH v3 1/5] MdeModulePkg: introduce non-
>>> discoverable device protocol
>>>
>>>
>>>
>>>> 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.
>>>
>>
>> I see. So some types of devices are handled by the current
>> NonDiscoveablePciDevice driver, and some other types of devices may be
>> handled by a future NonDiscoverableXXXDevice driver.
>> Now since the AHCI type is already handled by the NonDiscoverablePciDevice
>> driver, when there is a new NonDiscoverableXXXDevice driver, how can the two
>> know whether it should manage the AHCI type device or not?
>
>Good question. But how does the UEFI driver model deal with that? What happens if i have two drivers that both support the
>Ahci Pci class codes?
PCI CFG header contains VendorID/DeviceID fields which can be used to distinguish
them.

>
>> Besides since now all the EDKII Host Controller drivers are based on
>> PciIo, it implicitly requires all the low layer needs to produce PciIo interface
>> in order to re-use the EDKII Host Controller drivers.
>>
>
>Yes, that is the whole point of these patches. My preferred solution would be to split the ?hci drivers from pci i/o, by
>introducing intermediate protocols, but we both know that is unlikely to be accepted
>
>>>>>> +
>>>>>> +//
>>>>>> +// 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
>> Does AMBA  stand for Advanced Microcontroller Bus Architecture?
>> I have no idea about the AMBA.
>> Can you explain more why it's impossible to describe AMBA devices?
>>
>
>Amba devices are identifiable but not discoverable. If you know the base address, you know where the id registers are
>because they are always at the same register offset
>
>Thanks,
>Ard.
>
>>>
>>>>>> +  //
>>>>>> +  // 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
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel