Re: [PATCH v2 edk-platforms 1/4] Platform/Hisilicon/HiKey960: add gpio platform driver

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

On 2 May 2018 at 23:14, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Mar 08, 2018 at 09:29:50PM +0800, Haojian Zhuang wrote:
>> Add gpio platform driver to enable GPIO in HiKey960 platform.
>>
>> 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                        |  1 +
>>  Platform/Hisilicon/HiKey960/HiKey960.fdf                        |  1 +
>>  Platform/Hisilicon/HiKey960/HiKey960GpioDxe/HiKey960GpioDxe.inf | 35 +++++++++
>>  Platform/Hisilicon/HiKey960/HiKey960GpioDxe/HiKey960GpioDxe.c   | 77 ++++++++++++++++++++
>>  4 files changed, 114 insertions(+)
>>
>> diff --git a/Platform/Hisilicon/HiKey960/HiKey960.dsc b/Platform/Hisilicon/HiKey960/HiKey960.dsc
>> index 36f43956ab40..3da1b8556321 100644
>> --- a/Platform/Hisilicon/HiKey960/HiKey960.dsc
>> +++ b/Platform/Hisilicon/HiKey960/HiKey960.dsc
>> @@ -179,6 +179,7 @@ [Components.common]
>>    #
>>    # GPIO
>>    #
>> +  Platform/Hisilicon/HiKey960/HiKey960GpioDxe/HiKey960GpioDxe.inf
>>    ArmPlatformPkg/Drivers/PL061GpioDxe/PL061GpioDxe.inf
>>
>>    #
>> diff --git a/Platform/Hisilicon/HiKey960/HiKey960.fdf b/Platform/Hisilicon/HiKey960/HiKey960.fdf
>> index 655032a36c53..162dbaaf2646 100644
>> --- a/Platform/Hisilicon/HiKey960/HiKey960.fdf
>> +++ b/Platform/Hisilicon/HiKey960/HiKey960.fdf
>> @@ -120,6 +120,7 @@ [FV.FvMain]
>>    #
>>    # GPIO
>>    #
>> +  INF Platform/Hisilicon/HiKey960/HiKey960GpioDxe/HiKey960GpioDxe.inf
>>    INF ArmPlatformPkg/Drivers/PL061GpioDxe/PL061GpioDxe.inf
>>
>>    #
>> diff --git a/Platform/Hisilicon/HiKey960/HiKey960GpioDxe/HiKey960GpioDxe.inf b/Platform/Hisilicon/HiKey960/HiKey960GpioDxe/HiKey960GpioDxe.inf
>> new file mode 100644
>> index 000000000000..a16213f02520
>> --- /dev/null
>> +++ b/Platform/Hisilicon/HiKey960/HiKey960GpioDxe/HiKey960GpioDxe.inf
>> @@ -0,0 +1,35 @@
>> +#
>> +#  Copyright (c) 2018, Linaro. All rights reserved.
>> +#
>> +#  This program and the accompanying materials
>> +#  are licensed and made available under the terms and conditions of the BSD License
>> +#  which accompanies this distribution.  The full text of the license may be found at
>> +#  http://opensource.org/licenses/bsd-license.php
>> +#
>> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +#
>> +
>> +[Defines]
>> +  INF_VERSION                    = 0x00010019
>
> Probably bump this to 0x0001001a by now.
>

OK

>
> Missing space before '='.
>

OK

>> +  { 0xe8a0b000, 0, 8 },    // GPIO0
>
> It would not improve readability to request all of these live-coded
> values to be replaced by #defines, but barring that, could you add a
> comment header before the definition?:
>
> //  { base address, gpio index, gpio count }
>

OK
>> +  { 0xe8a0c000, 8, 8 },    // GPIO1
>> +  { 0xe8a0d000, 16, 8 },   // GPIO2
>> +  { 0xe8a0e000, 24, 8 },   // GPIO3
>> +  { 0xe8a0f000, 32, 8 },   // GPIO4
>> +  { 0xe8a10000, 40, 8 },   // GPIO5
>> +  { 0xe8a11000, 48, 8 },   // GPIO6
>> +  { 0xe8a12000, 56, 8 },   // GPIO7
>> +  { 0xe8a13000, 64, 8 },   // GPIO8
>> +  { 0xe8a14000, 72, 8 },   // GPIO9
>> +  { 0xe8a15000, 80, 8 },   // GPIO10
>> +  { 0xe8a16000, 88, 8 },   // GPIO11
>> +  { 0xe8a17000, 96, 8 },   // GPIO12
>> +  { 0xe8a18000, 104, 8 },  // GPIO13
>> +  { 0xe8a19000, 112, 8 },  // GPIO14
>> +  { 0xe8a1a000, 120, 8 },  // GPIO15
>> +  { 0xe8a1b000, 128, 8 },  // GPIO16
>> +  { 0xe8a1c000, 136, 8 },  // GPIO17
>> +  { 0xff3b4000, 144, 8 },  // GPIO18
>> +  { 0xff3b5000, 152, 8 },  // GPIO19
>
> I notice that these:
>> +  { 0xe8a1f000, 160, 8 },  // GPIO20
>> +  { 0xe8a20000, 168, 8 },  // GPIO21
> are out of order, from their base address.
> Are the names GPIO20/GPIO21 defined in the TRM?
>

Yes, it's mentioned in the TRM.
(https://github.com/96boards/documentation/blob/master/consumer/hikey960/hardware-docs/HiKey960_SoC_Reference_Manual.pdf)

>> +  232, 29, gGpioDevice
>
> And similarly
> // { global gpio count, gpio controller count, GPIO_CONTROLLER }
>

OK

Best Regards
Haojian