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

[Leif Lindholm <leif.lindholm@linaro.org>]

On Wed, May 09, 2018 at 06:59:53PM +0800, Haojian Zhuang wrote:
> 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.

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