On 3 May 2018 at 06:49, Leif Lindholm <[email protected]> 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 <[email protected]>
>> Cc: Ard Biesheuvel <[email protected]>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Haojian Zhuang <[email protected]>
>> ---
>> 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
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
