From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:4864:20::443; helo=mail-wr1-x443.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wr1-x443.google.com (mail-wr1-x443.google.com [IPv6:2a00:1450:4864:20::443]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id D733121962301 for ; Thu, 9 Aug 2018 03:11:54 -0700 (PDT) Received: by mail-wr1-x443.google.com with SMTP id f12-v6so4614656wrv.12 for ; Thu, 09 Aug 2018 03:11:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=yZaC27U9UDoeSxvqFNj2kvi/MrqaPLFnDFu5t+LkWAY=; b=jiSbZDNeCflEPTvHzxgeIm9noNlhZ+C3oeyCic3mT2JcZd0V/8RKw2QN3cXaCcw+vY 9sEItJgNpWbkKkV9RCFgY3tPcp8/FZQELAKajjbD6e+IIyYgpY7IZfrWzk3GnIwUb/03 YZbiHzoELeb98np3DpUDkhjZyH87qST4pe3aU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=yZaC27U9UDoeSxvqFNj2kvi/MrqaPLFnDFu5t+LkWAY=; b=oe7n5rkf24zelXCal+PiBFXPdCX756q9vrWO9vrRXdwQF3wwDwrBI7xJGy/2/SrKFl wWzEehp4pNySkxJwr4sUM/osP8B6gDhBKsvszZl4+8o7ERQoc9m14F5aFdVu59Zfkwzf 6iNlTXh5oBsby/eT8F3jy/A/Yt7dBYtlzv0yTWyDDRaF1QJtMhFCkZz1TIl31PRdw9Gw ExpUwjeBxQXSqnTU9tVbxzJscNj4phvcEUNcld+Q/ZBsOdIKDtdJdxyvG+hM4oL1IUUr cCheoNxplHaK6OnletRmu8Zku3M6CLiRyOojB4cH6kN3pAM4HF1GyGp2FM7Vd5QL/N/A MbKQ== X-Gm-Message-State: AOUpUlEZPLLBydS+0FT5VURLTDISfAXgC+GYeAj+7UUxZp3NsO1f64B6 3Y/nkE685fB+eeqQjpeW7EHmBg== X-Google-Smtp-Source: AA+uWPzPzubqreP1A4W3CEhRhjHayLStly3IxjSXh9uy7YzuZtY0Z7K1dDz3F0EG368xfu48AQKaMA== X-Received: by 2002:adf:f148:: with SMTP id y8-v6mr993834wro.134.1533809512786; Thu, 09 Aug 2018 03:11:52 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id j6-v6sm8770039wrs.91.2018.08.09.03.11.51 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 09 Aug 2018 03:11:51 -0700 (PDT) Date: Thu, 9 Aug 2018 11:11:50 +0100 From: Leif Lindholm To: Chris Co Cc: "edk2-devel@lists.01.org" , Michael D Kinney , Ard Biesheuvel Message-ID: <20180809101149.jvcosdya5dg6larx@bivouac.eciton.net> References: <20180722013021.10788-1-christopher.co@microsoft.com> <20180722013021.10788-2-christopher.co@microsoft.com> <20180807124951.qohmw6a3mvtvxyoh@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH edk2-platforms 1/4] Platform/Solidrun: Add Hummingboard SmBios X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 Aug 2018 10:11:55 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Aug 09, 2018 at 05:42:02AM +0000, Chris Co wrote: > Hi Leif, > > > -----Original Message----- > > From: Leif Lindholm > > Sent: Tuesday, August 7, 2018 5:50 AM > > To: Chris Co > > Cc: edk2-devel@lists.01.org; Michael D Kinney > > ; Ard Biesheuvel > > Subject: Re: [PATCH edk2-platforms 1/4] Platform/Solidrun: Add > > Hummingboard SmBios > > > > I'll start trickling through some generic comments on the code, apart from > > the general ones I made before. > > > > Thank you for these comments and continuing with the review. I do > sincerely apologize about all the code style issues. I should have > been more rigorous about enforcing code style before sending it up > (It was a lot worse before :-( ) Oh, that's what I expected, don't worry about it. I just want the code that goes in to be as clean as practical, since it may be used for reference for future contributions. I'm also getting more strict as time goes on as I see previous sloppiness I accepted in the past coming back to haunt me in new patches. > I am working on generating the new patch set. Currently about > halfway though. I will incorporate these comments into it. Should > have it ready by early next week. That said, if you do have more > feedback on other patches, I would be glad to incorporate them in as > soon as they are available. FYI, I will be off Friday-Monday. So anything you don't see by end of today won't arrive before Tuesday - and no rush getting anything out Monday. > > > +SMBIOS_TABLE_TYPE1 mSysInfoType1 = { > > > + { EFI_SMBIOS_TYPE_SYSTEM_INFORMATION, sizeof > > (SMBIOS_TABLE_TYPE1), 0 }, > > > + 1, // Manufacturer String > > > + 2, // ProductName String > > > + 3, // Version String > > > + 4, // SerialNumber String > > > + { 0x25EF0280, 0xEC82, 0x42B0, { 0x8F, 0xB6, 0x10, 0xAD, 0xCC, 0xC6, > > > +0x7C, 0x02 } }, > > > + SystemWakeupTypePowerSwitch, > > > + 5, // SKUNumber String > > > + 6, // Family String > > > +}; > > > +CHAR8 *mSysInfoType1Strings[] = { > > > + "SolidRun", > > > + "HummingBoard-Edge i4Pro", > > > + "2.0", > > > + "System Serial#", > > > + "hb04w-e-110-00-004-0", > > > > Do we have any way of obtaining the three above programmatically? > > > > This is the default template for Sysinfo, which gets overwritten at > runtime. Given that we are overwriting this table at runtime, I'm > thinking it would be better to just generate the table runtime and > avoid confusion by having this template. Agreed. > > > +VOID > > > +BIOSInfoUpdateSmbiosType0 ( > > > + VOID > > > + ) > > > +{ > > > + LogSmbiosData ((EFI_SMBIOS_TABLE_HEADER *)&mBIOSInfoType0, > > > +mBIOSInfoType0Strings, NULL); } > > > + > > > +VOID > > > +I64ToHexString( > > > > Hmm. We already have several copies of functions doing exactly this in EDK2. > > Just no centrally reusable one. Maybe worth adding to BaseLib? > > > > Regardless, it should probablt be called UINT64ToHexString or suchlike. > > > > I'll rename it to UINT64ToHexString for the next patch set, then > will look into making a centrally reusable version. Thanks! > > > + BoardSerial = ((UINT64)(MmioRead32(RegOCOTP_CFG1Addr))) << 32; > > > + BoardSerial = BoardSerial | > > > + ((UINT64)(MmioRead32(RegOCOTP_CFG0Addr))); > > > > MmioRead32 ( > > A bit heavy on the parantheses. > > > > Will fix. For clarity: I'm all for using more parantheses than necessary where it means avoiding needing to stop and think about operator precedence. Ard isn't :) But here they just don't add anything other than clutter. Regards, Leif