Re: [PATCH v2 edk-platforms 2/4] Platform/Hisilicon/HiKey960: enable virtual keyboard

[Haojian Zhuang <haojian.zhuang@linaro.org>]

On 3 May 2018 at 06:49, Leif Lindholm <leif.lindholm@linaro.org> 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 <leif.lindholm@linaro.org>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
>> ---
>>  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