Re: [PATCH v1 6/6] arm: nuvoton: Add support for Nuvoton NPCM750 BMC

From: Tom Rini <trini@konsulko.com>
To: Jim Liu <jim.t90615@gmail.com>
Cc: JJLIU0@nuvoton.com, YSCHU@nuvoton.com, KWLIU@nuvoton.com,
	lukma@denx.de, seanga2@gmail.com, sjg@chromium.org, sr@denx.de,
	u-boot@lists.denx.de
Subject: Re: [PATCH v1 6/6] arm: nuvoton: Add support for Nuvoton NPCM750 BMC
Date: Wed, 6 Apr 2022 09:48:08 -0400	[thread overview]
Message-ID: <20220406134808.GD14282@bill-the-cat> (raw)
In-Reply-To: <20220406075737.27938-7-JJLIU0@nuvoton.com>

[-- Attachment #1: Type: text/plain, Size: 2631 bytes --]

On Wed, Apr 06, 2022 at 03:57:37PM +0800, Jim Liu wrote:

> Add basic support for the Nuvoton NPCM750 BMC
> 
> Signed-off-by: Jim Liu <JJLIU0@nuvoton.com>
[snip]
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 4567c183fb..45ab2793b2 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -922,6 +922,12 @@ config ARCH_NEXELL
>  	select DM
>  	select GPIO_EXTRA_HEADER
>  
> +config ARCH_NPCM
> +        bool "Support Nuvoton SoCs"
> +        select DM
> +        select OF_CONTROL
> +        imply CMD_DM

I see this in a lot of your Kconfig changes, the spacing is
wrong/inconsistent, please fix to be a single tab of indentation.

> diff --git a/arch/arm/mach-nuvoton/npcm7xx/reset.c b/arch/arm/mach-nuvoton/npcm7xx/reset.c
> new file mode 100644
> index 0000000000..74f925f67b
> --- /dev/null
> +++ b/arch/arm/mach-nuvoton/npcm7xx/reset.c

I feel this should be doing something under/with drivers/reset/ instead.

> +int board_init(void)
> +{
> +	gd->bd->bi_arch_number = CONFIG_MACH_TYPE;
> +	gd->bd->bi_boot_params = (PHYS_SDRAM_1 + 0x100UL);

Do you really need to support non-DT kernels?  CONFIG_MACH_TYPE should
likely not be used here.

> diff --git a/include/configs/poleg.h b/include/configs/poleg.h
> new file mode 100644
> index 0000000000..549b198285
> --- /dev/null
> +++ b/include/configs/poleg.h
> @@ -0,0 +1,77 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (c) 2021 Nuvoton Technology Corp.
> + */
> +
> +#ifndef __CONFIG_POLEG_H
> +#define __CONFIG_POLEG_H
> +
> +#undef  CONFIG_USE_IRQ

Ugh, I see that line made it back in, in another board.  That's not
meaningful.

> +#define CONFIG_ARCH_CPU_INIT
> +#define CONFIG_ENV_OVERWRITE
> +
> +#define CONFIG_MACH_TYPE		        MACH_TYPE_NPCMX50
> +
> +#define CONFIG_SETUP_MEMORY_TAGS
> +#define CONFIG_INITRD_TAG

TAGS are part of Kconfig, if you _really_ need them still.  Please make
use of CI:
https://u-boot.readthedocs.io/en/latest/develop/ci_testing.html
to make sure the series passes there as we have tests to catch things
which need to be in Kconfig not board.h files such as this and some
others that follow.

> +#define CONFIG_SYS_BAUDRATE_TABLE       {115200, 57600, 38400}

Please just use the default baudrate table.

> diff --git a/include/dt-bindings/reset/nuvoton,npcm7xx-reset.h b/include/dt-bindings/reset/nuvoton,npcm7xx-reset.h
> new file mode 100644
> index 0000000000..2831918804
> --- /dev/null
> +++ b/include/dt-bindings/reset/nuvoton,npcm7xx-reset.h

Is this coming in unmodified from Linux?  Thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]