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

[Joel Stanley]

On Wed, 6 Jul 2022 at 14:29, Jae Hyun Yoo <quic_jaehyoo at quicinc.com> wrote:
>
> 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.

Okay. I'm not sure that makes it easier to maintain, but in the
interests of getting support upstream lets go with that.

>
> >>>> +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.

Great!

>
> Thanks,
> Jae
>
> > Cheers,
> >
> > Joel