From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c0c::235; helo=mail-wr0-x235.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wr0-x235.google.com (mail-wr0-x235.google.com [IPv6:2a00:1450:400c:c0c::235]) (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 D6D982096AEC9 for ; Wed, 9 May 2018 06:32:17 -0700 (PDT) Received: by mail-wr0-x235.google.com with SMTP id y15-v6so23938634wrg.11 for ; Wed, 09 May 2018 06:32:17 -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=N67U/+23/frzP3rdsXQQgCJPWhAHyxt1yfAmeHKP3N4=; b=PKIx8qINlIOoCf6rVUtLu0PIsGSdZkIpaLl8LhCmWx+h4abrtaJyb8zvX/XyCJdw40 7Lesyv3F9AR6lFLLTY+BCsNafc1iGY2LT1wQJwB5a7WTtTT972t4a7L1erCkZGrLi8kk BP+P7jZZRCD2FZ/cw/62uYCbqtkvI5OCwqfcw= 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=N67U/+23/frzP3rdsXQQgCJPWhAHyxt1yfAmeHKP3N4=; b=fEfNeSxVicGH1cq0djZG88+zxcEehu2+0QMmOIkdwDXlLFXGBrRndbAU6HlhuMQQ4w peO2c4n7CkB/A1Rf/AuGuXLC1hg8MPQXfUHtBymHvqc4/zTjhBLOwLtIfp3wRnjVHBaF toBVxYwxSIG5xQsWt0Xg0GXu5HbvXI4Km4/ljjINNlvZWtoASi2jL4VOQYByRgYXZXVG TrBcFILZ6+xfvYbZpUJpAB7eCaDDAdGJ8w2G0/7Ze5iiDUzP6e818Ofhs+vKNAUbuywz dXS04e1MRzvshGVwduSE24StH3d4RhGhctjoj1yNjtmmdw6FFDmJx8R/dR/hQtKDTcFf Z5MQ== X-Gm-Message-State: ALQs6tCy27Gl8+sMTvuKbaP/C7CtgwKEU9Ctvxp/hThd7/mV+m12BoFy 92tDFsHA4+Cf9hqLrgbLOkNR8AQEjiw= X-Google-Smtp-Source: AB8JxZrxJGQZBuPLIC1H9fLEha3vwnhJRxbpYarg3/XUewASH153lCsytgugWcqRwoaVx22McYj/mw== X-Received: by 2002:adf:a294:: with SMTP id s20-v6mr34732769wra.114.1525872735893; Wed, 09 May 2018 06:32:15 -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 m9-v6sm35944897wrf.72.2018.05.09.06.32.14 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 09 May 2018 06:32:14 -0700 (PDT) Date: Wed, 9 May 2018 14:32:13 +0100 From: Leif Lindholm To: Haojian Zhuang Cc: "edk2-devel@lists.01.org" , Ard Biesheuvel Message-ID: <20180509133212.u5qisf7dtducuhae@bivouac.eciton.net> References: <1520515804-29577-1-git-send-email-haojian.zhuang@linaro.org> <20180502224959.pfynroenmayae3sy@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH v2 edk-platforms 2/4] Platform/Hisilicon/HiKey960: enable virtual keyboard 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: Wed, 09 May 2018 13:32:18 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Wed, May 09, 2018 at 06:59:53PM +0800, Haojian Zhuang wrote: > On 3 May 2018 at 06:49, Leif Lindholm wrote: > > On Thu, Mar 08, 2018 at 09:30:04PM +0800, Haojian Zhuang wrote: > >> Enable virtual keyboard on HiKey960 platform. The platform > >> driver read pattern from memory or GPIO pin. When the value > >> is matched, it simulates a hotkey that is used to adjust > >> sequence of boot options. > > > > This patch looks like it contains an awful lot more than described > > here. Can it be split up? > > > >> Cc: Leif Lindholm > >> Cc: Ard Biesheuvel > >> Contributed-under: TianoCore Contribution Agreement 1.1 > >> Signed-off-by: Haojian Zhuang > >> --- > >> Platform/Hisilicon/HiKey960/HiKey960.dsc | 10 + > >> Platform/Hisilicon/HiKey960/HiKey960.fdf | 7 + > >> Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.inf | 54 +++ > >> Silicon/Hisilicon/Hi3660/Include/Hi3660.h | 147 ++++++ > >> Silicon/Hisilicon/Hi3660/Include/Hkadc.h | 66 +++ > >> Platform/Hisilicon/HiKey960/HiKey960Dxe/HiKey960Dxe.c | 491 ++++++++++++++++++++ > >> 6 files changed, 775 insertions(+) > >> > >> +STATIC > >> +VOID > >> +InitAdc ( > >> + VOID > >> + ) > >> +{ > >> + // reset hkadc > >> + MmioWrite32 (CRG_PERRSTEN2, PERRSTEN2_HKADCSSI); > >> + // wait a few clock cycles > >> + MicroSecondDelay (2); > >> + MmioWrite32 (CRG_PERRSTDIS2, PERRSTEN2_HKADCSSI); > >> + MicroSecondDelay (2); > >> + // enable hkadc clock > >> + MmioWrite32 (CRG_PERDIS2, PEREN2_HKADCSSI); > >> + MicroSecondDelay (2); > >> + MmioWrite32 (CRG_PEREN2, PEREN2_HKADCSSI); > >> + MicroSecondDelay (2); > > > > Why the need for a few clock cycles? Why was 2 chosen? Barriers needed? > > > > Need to wait hardware stabilization. Barriers don't help at here. Understood, but please add a comment to that effect. One comment for each block of these. > >> + Adcin1Remap = AdcinDataRemap (Adcin1); > >> + DEBUG ((DEBUG_INFO, "[BDID]Adcin1Remap:%d\n", Adcin1Remap)); > >> + if (Adcin1Remap == BOARDID_UNKNOW) { > >> + return EFI_INVALID_PARAMETER; > >> + } > >> + // read ADC channel2 data > >> + AdcGetValue (ADC_ADCIN2, &Adcin2); > >> + DEBUG ((DEBUG_INFO, "[BDID]Adcin2:%d\n", Adcin2)); > >> + Adcin2Remap = AdcinDataRemap (Adcin2); > >> + DEBUG ((DEBUG_INFO, "[BDID]Adcin2Remap:%d\n", Adcin2Remap)); > >> + if (Adcin2Remap == BOARDID_UNKNOW) { > >> + return EFI_INVALID_PARAMETER; > >> + } > >> + *Id = BOARDID3_BASE * 1000 + (Adcin2Remap * 100) + (Adcin1Remap * 10) + Adcin0Remap; > > > > 1) Please use parentheses consistently. > > 2) This is the only use of BOARDID3_BASE - why is it being multiplied? > > It's used to generate the same prefix of board family. > > > 3) What is the format of this ID? Is it described somewhere or is it > > completely arbitrary? > > BoardID is based on the voltage of a resistor. Since the resistor > value is fixed, > the BoardID is same on all HiKey960 platforms. > > For example, BoardID v1 is different from BoardID v2. But the prefix is same. Can you add a comment describing this before the assignment to *Id? I would like to understand how we can ensure analog errors do not cause misidentification. And this would be easier with hex values and masks, but if the algorithm is already defined and used, we could have max/min values for each, test against them, and return error if invalid values are found. Trying to make sense of the code, I also think these should all be AcdIn... rather than Acdin... (upper case I) Functions and variables. > > 4) What is the purpose of this Id? This function is used to populate > > mBoardId, but that variable is not used anywhere. > > It'll be used later. It will fill different DTB contents according to > different versions > of the HiKey960 platforms. Then could it be postponed until we get those patches as well, so we can see its use and not just its definition? > >> + DEBUG ((DEBUG_INFO, "[BDID]boardid: %d\n", *Id)); > >> + return EFI_SUCCESS; > >> +} > >> + > >> +STATIC > >> +VOID > >> +InitSdCard ( > >> + IN VOID > >> + ) > >> +{ > >> + UINT32 Data; > >> + > >> + // LDO16 > >> + Data = MmioRead32 (PMU_REG_BASE + (0x79 << 2)) & 7; > >> + Data |= 6; > >> + MmioWrite32 (PMU_REG_BASE + (0x79 << 2), Data); > >> + MmioOr32 (PMU_REG_BASE + (0x78 << 2), 2); > > > > Please replace all live-coded values with #defines. > > I'll try to replace the hard-coded values as I could. But I can't get everything > with document. Thanks. > >> + MicroSecondDelay (100); > > > > Why this delay? > > Does this need a barrier? > > Barrier doesn't help. Just wait regulator stable. Add comment please. > >> + > >> + // LDO9 > >> + Data = MmioRead32 (PMU_REG_BASE + (0x6b << 2)) & 7; > >> + Data |= 5; > >> + MmioWrite32 (PMU_REG_BASE + (0x6b << 2), Data); > >> + MmioOr32 (PMU_REG_BASE + (0x6a << 2), 2); > > > > Please replace all live-coded values with #defines. > > > >> + MicroSecondDelay (100); > > > > Why this delay? > > Does this need a barrier? > > > > Barrier doesn't help. Just wait regulator stable. Add comment please. Regards, Leif