[PATCH u-boot v2019.04-aspeed-openbmc 2/3] board: ast2600_qualcomm: add initial version of Qualcomm DC-SCM V1 board

[Jae Hyun Yoo]

On 7/5/2022 5:58 PM, Joel Stanley wrote:
> On Tue, 5 Jul 2022 at 14:28, Jae Hyun Yoo <quic_jaehyoo at quicinc.com> wrote:
>>
>> Hello Joel,
>>
>> On 7/4/2022 11:51 PM, Joel Stanley wrote:
>>> On Thu, 30 Jun 2022 at 20:02, Jae Hyun Yoo <quic_jaehyoo at quicinc.com> wrote:
>>>>
>>>> Add initial version of Qualcomm DC-SCM V1 board to support Qualcomm
>>>> specific options.
>>>>
>>>> Signed-off-by: Jae Hyun Yoo <quic_jaehyoo at quicinc.com>
>>>> ---
>>>>    arch/arm/mach-aspeed/ast2600/Kconfig      |  8 ++++++
>>>>    board/aspeed/ast2600_qualcomm/Kconfig     | 15 +++++++++++
>>>>    board/aspeed/ast2600_qualcomm/Makefile    |  1 +
>>>>    board/aspeed/ast2600_qualcomm/dc-scm-v1.c | 33 +++++++++++++++++++++++
>>>>    4 files changed, 57 insertions(+)
>>>>    create mode 100644 board/aspeed/ast2600_qualcomm/Kconfig
>>>>    create mode 100644 board/aspeed/ast2600_qualcomm/Makefile
>>>>    create mode 100644 board/aspeed/ast2600_qualcomm/dc-scm-v1.c
>>>>
>>>> diff --git a/arch/arm/mach-aspeed/ast2600/Kconfig b/arch/arm/mach-aspeed/ast2600/Kconfig
>>>> index 46cc1ad1dbd9..252458846a84 100644
>>>> --- a/arch/arm/mach-aspeed/ast2600/Kconfig
>>>> +++ b/arch/arm/mach-aspeed/ast2600/Kconfig
>>>> @@ -46,6 +46,13 @@ config TARGET_AST2600_INTEL
>>>>             AST2600-INTEL is an Intel Eagle Stream CRB with
>>>>             AST2600 as the BMC.
>>>>
>>>> +config TARGET_AST2600_QUALCOMM_DC_SCM_V1
>>>> +       bool "AST2600_QUALCOMM_DC_SCM_V1"
>>>> +       depends on ASPEED_AST2600
>>>> +       help
>>>> +         AST2600-QUALCOMM-DC-SCM-V1 is a Qualcomm DC-SCM V1 board which is
>>>> +         equipped with AST2600.
>>>> +
>>>>    endchoice
>>>>
>>>>    source "board/aspeed/evb_ast2600/Kconfig"
>>>> @@ -53,5 +60,6 @@ source "board/aspeed/fpga_ast2600/Kconfig"
>>>>    source "board/aspeed/slt_ast2600/Kconfig"
>>>>    source "board/aspeed/ast2600_ibm/Kconfig"
>>>>    source "board/aspeed/ast2600_intel/Kconfig"
>>>> +source "board/aspeed/ast2600_qualcomm/Kconfig"
>>>>
>>>>    endif
>>>> diff --git a/board/aspeed/ast2600_qualcomm/Kconfig b/board/aspeed/ast2600_qualcomm/Kconfig
>>>> new file mode 100644
>>>> index 000000000000..3ede24c34dee
>>>> --- /dev/null
>>>> +++ b/board/aspeed/ast2600_qualcomm/Kconfig
>>>> @@ -0,0 +1,15 @@
>>>> +if TARGET_AST2600_QUALCOMM_DC_SCM_V1
>>>> +
>>>> +config SYS_BOARD
>>>> +       default "ast2600_qualcomm"
>>>> +
>>>> +config SYS_VENDOR
>>>> +       default "aspeed"
>>>
>>> Out of interest, how does u-boot handle this upstream? Does a board
>>> made by company Y with a chip made by company X considered vendor X,
>>> or vendor Y?
>>
>> This code is added into a sub-directory of 'board/aspeed' so the
>> SYS_VENDOR must be set to 'aspeed'. If I set that to 'qcom', then
>> builder will look for an Makefile under 'board/qcom' instead and it
>> makes a failure. The same pattern is already used for ast2600_ibm and
>> ast2600_intel.
> 
> Yes, that is true, but it's not what I'm asking. There's momentum to
> use the upstream tree for u-boot and I want to ensure any patches that
> go in from now on are applicable upstream.
> 
> Can you do a survey of the upstream tree and see what the pattern is?
> 
> If it's to use the manufacturer

Investigated the latest u-boot upstream that it uses manufacturer's sub
folder instead of SoC vendor folder. For an example, RK3399 based Google
'gru' board was added to 'board/google' folder instead of
'board/rockchip'. Means that the current manufacturer board folders in
'board/aspeed' in openbmc u-boot tree also need to be refactored.

I'll submit this change to 'board/qualcomm' in v2.

>>>> +int board_late_init(void)
>>>
>>> Do you need to include this if it's doing nothing?
>>
>> The defconfig has 'CONFIG_BOARD_LATE_INIT=y' so this function should be
>> added as a dummy function otherwise it meets a build failure. This
>> function will be filled by following commits later.
> 
> Doesn't it make sense to not set CONFIG_BOARD_LATE_INIT=y if you don't need it?
> 
> You can introduce it when you need it (or add the code that uses it
> with this commit).

Agree with you. I'll remove the config if it's not needed at this
moment.

>>>> Is there a reason you don't use the gpio driver?
>>
>> Forgot to reply on this comment. This function is called from
>> board_early_init_f and gpio driver is not ready at that timing so
>> it uses direct register access.
> 
> Why not call it at a later boot stage, so the gpio driver is ready?

To set the GPIO output as early as possible but the timing wouldn't much
different so yes, I'll move the gpio init call to board_late_init using
the gpio driver in v2.

Thanks,
Jae

> Cheers,
> 
> Joel