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:c09::233; helo=mail-wm0-x233.google.com; envelope-from=haojian.zhuang@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm0-x233.google.com (mail-wm0-x233.google.com [IPv6:2a00:1450:400c:c09::233]) (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 7EE4B20945DBF for ; Wed, 9 May 2018 03:59:56 -0700 (PDT) Received: by mail-wm0-x233.google.com with SMTP id t11so23931330wmt.0 for ; Wed, 09 May 2018 03:59:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=34ili9+VgFEqS5xvX9NrE9B37D3ZVCC628D38agB6Aw=; b=d4JowcKuw/sVi+EbGGWQbCGo6501MT/Xo5w0Pt6zEY8Sp90HLDecr/YSWfP/jU8fd7 A+P8CrJSxBRsNSUO8+fIyO4Ic7tywPvZ+EB0MfFgFzKywyI5N2HSxoTsrqduFzsAYjRs 17YmHq2pwwL8oy4NuFVVLgo1aHZ78z6u4nj1k= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=34ili9+VgFEqS5xvX9NrE9B37D3ZVCC628D38agB6Aw=; b=TqUu0wN6ljXLdRVY1Bh4nDr9ShDk+3zRg+GRjGTQXDoR7n8h4PubP9LvVpPCEyfKUk gpMPC+vaDRAZzV9GZHwryUvuWi7pLkIm4aSHM1v11QuG8KW24e+DhkzFmJgXakfllvUA Fm2Yh0iiHTBTnbCXLlUk0aT3mnAvwCC7d9EZ3OEW47R3oovXXha2tKKoNGBrbuHb4ntf fqd82j2Fc/1d6Vtt9wDg8QXMb5UFZ/x/6Qurkg4nKdJQwSB4HEOyi7s9UDymU1ArvehV kqbzYiz+BnN2d2sE+F+6y4bEgmzEQKlwwtrAoxA1PkfEzidt5uDCSEwRFHWmTzbhRaKf mi9g== X-Gm-Message-State: ALKqPwf72L+VqJuIqzOX3oKofJhgG+TrJFeMEoeFLzOVdx2MfojcUtiZ FdhbLl7bCOIIbTyvIujLyzh8iau4djnjp5QL85UJbQ== X-Google-Smtp-Source: AB8JxZr/vn5bPk3WTVFeqnKMKzh+TOsgpG92fZ4YwnsNucrqBF5bV11vIl7awkny3DijwOWPhdg/vDR8PfYokecRxZ4= X-Received: by 2002:a1c:ca:: with SMTP id 193-v6mr5015548wma.99.1525863594313; Wed, 09 May 2018 03:59:54 -0700 (PDT) MIME-Version: 1.0 Received: by 10.28.150.22 with HTTP; Wed, 9 May 2018 03:59:53 -0700 (PDT) In-Reply-To: <20180502224959.pfynroenmayae3sy@bivouac.eciton.net> References: <1520515804-29577-1-git-send-email-haojian.zhuang@linaro.org> <20180502224959.pfynroenmayae3sy@bivouac.eciton.net> From: Haojian Zhuang Date: Wed, 9 May 2018 18:59:53 +0800 Message-ID: To: Leif Lindholm Cc: "edk2-devel@lists.01.org" , Ard Biesheuvel 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 10:59:56 -0000 Content-Type: text/plain; charset="UTF-8" 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. >> + 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. > 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. >> + 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. > >> + MicroSecondDelay (100); > > Why this delay? > Does this need a barrier? > Barrier doesn't help. Just wait regulator stable. >> + >> + // 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. Best Regards Haojian