From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 6DB3481F29 for ; Wed, 16 Nov 2016 23:52:16 -0800 (PST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga103.fm.intel.com with ESMTP; 16 Nov 2016 23:52:21 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,504,1473145200"; d="scan'208";a="1060556550" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga001.jf.intel.com with ESMTP; 16 Nov 2016 23:52:20 -0800 Received: from fmsmsx153.amr.corp.intel.com (10.18.125.6) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 16 Nov 2016 23:52:20 -0800 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by FMSMSX153.amr.corp.intel.com (10.18.125.6) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 16 Nov 2016 23:52:20 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.142]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.138]) with mapi id 14.03.0248.002; Thu, 17 Nov 2016 15:52:18 +0800 From: "Ni, Ruiyu" To: Ard Biesheuvel CC: "Kinney, Michael D" , "edk2-devel@lists.01.org" , "Gao, Liming" , "afish@apple.com" , Leif Lindholm Thread-Topic: [edk2] [PATCH v3 1/5] MdeModulePkg: introduce non-discoverable device protocol Thread-Index: AQHSQCraEwMvOFFyHUuvZ27kFPDix6DbXOsAgAEaWfD//7PxAIAAoCmg Date: Thu, 17 Nov 2016 07:52:17 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D58E7D6F5@SHSMSX104.ccr.corp.intel.com> References: <1479315571-14953-1-git-send-email-ard.biesheuvel@linaro.org> <1479315571-14953-2-git-send-email-ard.biesheuvel@linaro.org> <20161116174848.GC27644@bivouac.eciton.net> <734D49CCEBEEF84792F5B80ED585239D58E7C6D4@SHSMSX104.ccr.corp.intel.com> <4E810E45-F1CC-429C-B3F4-FC6182F7D9B2@linaro.org> In-Reply-To: <4E810E45-F1CC-429C-B3F4-FC6182F7D9B2@linaro.org> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH v3 1/5] MdeModulePkg: introduce non-discoverable device protocol X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 17 Nov 2016 07:52:16 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 > Cc: Kinney, Michael D ; edk2- > devel@lists.01.org; Gao, Liming ; afish@apple.com; > Leif Lindholm > Subject: Re: [edk2] [PATCH v3 1/5] MdeModulePkg: introduce non- > discoverable device protocol >=20 >=20 >=20 > > On 17 Nov 2016, at 02:53, Ni, Ruiyu 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 > >> Cc: Ni, Ruiyu ; edk2-devel@lists.01.org; > >> afish@apple.com; Gao, Liming ; Kinney, Michael > >> D > >> 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 > >>> --- > >>> 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.
> >>> + > >>> + 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 > >>> + > >>> +#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 > > >=20 > 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 dri= vers. >=20 > in other words, pci is part of the /driver/ side, and it is perfectly pos= sible for, > e.g., a non-discoverable ahci device to be driven by a different non-pci = driver > in the future. >=20 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 tw= o know whether it should manage the AHCI type device or not? Besides since now all the EDKII Host Controller drivers are based on PciIo, it implicitly requires all the low layer needs to produce PciIo inte= rface in order to re-use the EDKII Host Controller drivers. > >>> + > >>> +// > >>> +// 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. > > >=20 > 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 mak= e 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? >=20 > >>> + // > >>> + // 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 =3D { 0x4DF19259, 0xDC71, 0x4D46, { 0xBE, > >>> 0xF1, 0x35, 0x7B, 0xB5, 0x78, 0xC4, 0x18 } } > >>> > >>> + ## Include/Protocol/NonDiscoverableDevice.h > >>> + gEdkiiNonDiscoverableDeviceProtocolGuid =3D { 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