From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.24; helo=mga09.intel.com; envelope-from=star.zeng@intel.com; receiver=edk2-devel@lists.01.org Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) (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 E8FAB2097413E for ; Mon, 18 Jun 2018 19:21:25 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Jun 2018 19:21:25 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,241,1526367600"; d="scan'208";a="50969005" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga006.jf.intel.com with ESMTP; 18 Jun 2018 19:21:25 -0700 Received: from fmsmsx119.amr.corp.intel.com (10.18.124.207) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 18 Jun 2018 19:21:24 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX119.amr.corp.intel.com (10.18.124.207) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 18 Jun 2018 19:21:24 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.223]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.51]) with mapi id 14.03.0319.002; Tue, 19 Jun 2018 10:21:20 +0800 From: "Zeng, Star" To: Leif Lindholm , Evan Lloyd CC: "Ni, Ruiyu" , nd , Stephanie Hughes-Fitt , "Dong, Eric" , "Ard Biesheuvel" , "edk2-devel@lists.01.org" , "Zeng, Star" Thread-Topic: [edk2] [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space Thread-Index: AQHUBLMb5UcT4Fv0TUmfHpcJbBPn+KRg13IAgAATnwCAAA0BAIAF5XGQ Date: Tue, 19 Jun 2018 02:21:18 +0000 Message-ID: <0C09AFA07DD0434D9E2A0C6AEB0483103BB59079@shsmsx102.ccr.corp.intel.com> References: <20180615141348.13020-1-sami.mujawar@arm.com> <20180615161326.7rl74schpgor6h6c@bivouac.eciton.net> In-Reply-To: <20180615161326.7rl74schpgor6h6c@bivouac.eciton.net> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 19 Jun 2018 02:21:26 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Leif, I totally agree with you. Before we have clear direction about this to align code and CCS spec. I pre= fer to align with the immediately surrounding code. Thanks, Star -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Leif= Lindholm Sent: Saturday, June 16, 2018 12:13 AM To: Evan Lloyd Cc: Ni, Ruiyu ; nd ; Stephanie Hughes-Fitt = ; Dong, Eric ; Ard Bies= heuvel ; edk2-devel@lists.01.org; Zeng, Star Subject: Re: [edk2] [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem= space On Fri, Jun 15, 2018 at 03:26:54PM +0000, Evan Lloyd wrote: > Hi Ard, Zeng >=20 > > -----Original Message----- > > From: Ard Biesheuvel > > Sent: 15 June 2018 15:17 > > To: Sami Mujawar > > Cc: edk2-devel@lists.01.org; Zeng, Star ; Eric=20 > > Dong ; Ruiyu Ni ; Leif=20 > > Lindholm ; Matteo Carlini=20 > > ; Stephanie Hughes-Fitt=20 > > ; Evan Lloyd ;=20 > > Thomas Abraham ; nd > > Subject: Re: [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem=20 > > space > >=20 > > On 15 June 2018 at 16:13, Sami Mujawar wrote: > > > The SATA controller driver crashes while accessing the PCI memory=20 > > > [AHCI Base Registers (ABAR)], as the PCI memory space is not enabled. > > > > > > Enable the PCI memory space access to prevent the SATA Controller=20 > > > driver from crashing. > > > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > Signed-off-by: Sami Mujawar > > > --- > > > The changes can be viewed at > > > > > https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_ > > f > > i > > > x_v2 > > > > > > Notes: > > > v2: > > > - Improved log message and code documentation based on=20 > > > feedback > > [SAMI] > > > - Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE = [ZENG] > > > - This SATA Controller driver only uses the PCI BAR5 register > > > space which is the AHCI Base Address (ABAR). According to the > > > 'Serial ATA Advanced Host Controller Interface (AHCI) 1.3.1' > > > specification, section 2.1.11, 'This register allocates space > > > for the HBA memory registers'. > > > The section 2.1.10, allows provision for Optional BARs which > > > may support either memory or I/O spaces. However, in the contex= t > > > of the current SATA controller driver, which only ever access > > > the ABAR, enabling I/O memory space is not required. = [SAMI] > > > - Prefer to use // surrounding comments = [ZENG] > > > - Doing this would violate the edk2 coding standard. See EDK2 > > > Coding Standard Specification, revision 2.20, section 6.2.3. = [SAMI] > > > > >=20 > > Please stop obsessing about the coding standard. The maintainer was=20 > > quite clear what he wanted, and in the past, I have also indicated=20 > > that for the ARM related packages, I prefer readability and=20 > > consistency over adherence to a dubious standard. >=20 > [[Evan Lloyd]] I'd like to make some points here: > 1. It is perfectly reasonable for Sami to explain why he has done > something a certain way - Zeng is then at liberty to respond > with his preference, but we do not (yet) know what that might > be. Yes we do. Zeng responded with that in the first instance. Sami then disput= ed that explicitly stated preference, referring to the coding standard. The= re was no lack of clarity in what Zeng wanted - so I remain unclear on what= this is intending to achieve? > 2. "readability and consistency" is the very purpose of any > coding standard. If consistency is valuable, doesn't that > apply across modules? I understand that it may be > particularly valuable for maintainers within their modules, > but the rest of us benefit from a consistent style - > especially when looking outside our normal demesne. At which point the rule of thumb is: aligning with the immediately surround= ing code always trumps adhering to the specified style. Which is in itself trumped by the maintainer explicitly stating another pre= ference. (Like here.) > 3. Applying a consistent Coding Standard across the whole project > is a necessary pre-condition for automated CI checks on new > submissions. I'd like to aim e.g. for an improved > patchcheck.py, but that requires consistency across modules, > at least for new code. Such a system will _never_ be fully automatable. I will _always_ take a 80 < x < 90 line over one that breaks up an output s= tring or one that reduces readability more than having to side-scroll does. That doesn't make it worthless, far from it. > Note: I am not disputing the dubiosity of the CCS. It could be=20 > improved (a lot). However it is all we have right now. Then a more constructive approach is to recognise that not a single one of = the maintainers are appearing to be respecting the horror vacui rule and su= bmit a patch against https://github.com/tianocore-docs/edk2-CCodingStandard= sSpecification to have section 6.2.3 deleted. (Much like I should at some point get around to have 5.4.2.2.2 deleted, or = at least conditionalised to only apply for !ARM.) Regards, Leif > Regards, > Evan >=20 > >=20 > >=20 > > > v1: > > > - Fix SATA Controller driver crash = [SAMI] > ... > > > -- > > > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' > > > > > > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel