Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <nick.hawkins@hpe.com> <20220216183704.42084-1-nick.hawkins@hpe.com>
In-Reply-To: <20220216183704.42084-1-nick.hawkins@hpe.com>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Wed, 16 Feb 2022 22:57:10 +0100
Message-ID: <CAK8P3a2Ryjr738XgLsGQngvc7j=Kk620LMjWpu-_HiwdK_fX4Q@mail.gmail.com>
Subject: Re: [PATCH] [v1] arch: arm: configs: gxp_defconfig
To:     "Hawkins, Nick" <nick.hawkins@hpe.com>
Cc:     "Verdun, Jean-Marie" <verdun@hpe.com>,
        Russell King <linux@armlinux.org.uk>,
        Joel Stanley <joel@jms.id.au>,
        Andrew Jeffery <andrew@aj.id.au>,
        Olof Johansson <olof@lixom.net>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Feb 16, 2022 at 7:36 PM <nick.hawkins@hpe.com> wrote:
>
> From: Nick Hawkins <nick.hawkins@hpe.com>
>
> Description: Adding the configuration file for the upcoming
>  hpe gxp soc.
>
> Note: This patch is part of a set with patches:
>   [v4] arch: arm: boot: dts: Create HPE GXP Device Tree
>   [v1] dt-bindings: timer: Add HPE GXP Timer binding
>   [v1] dt-bindings: watchdog: Add HPE GXP Watchdog timer binding
>   [v1]dt-bindings: vendor-prefixes: add HPE Prefix
>   [v1] dt-bindings: soc: Add HPE GXP SOC binding
>
> Additional Note: Maintainers will be updated in separate patch
>  to cover all of the above patches.

Please have a look at other patch series for the style of the submission.

All the information above doesn't really go in the individual patches, as it
is not meant as part of the permanent git history. Instead, keep the
series together as one thread the way that git-format-patch generates it,
and put information about the patch series into the cover letter.

One bit of information that I would like to see in the defconfig patch
is an explanation about why you need a custom defconfig in the
first place, rather than using multi_v7_defconfig. Please also add
a patch to enable your platform in the multi_v7_defconfig, along with
the drivers you need (as loadable modules).

See Documentation/process/submitting-patches.rst for more detail.

> Information: GXP is the name of the HPE SoC.
>  This SoC is used to implement BMC features of HPE servers
>   (all ProLiant, Synergy, and many Apollo, and Superdome machines)
>    It does support many features including:
>     ARMv7 architecture, and it is based on a Cortex A9 core
>     Use an AXI bus to which a memory controller is attached,
>     as well as multiple SPI interfaces to connect boot flash,
>     and ROM flash, a 10/100/1000 Mac engine which
>     supports SGMII (2 ports) and RMII Multiple I2C engines to
>     drive connectivity with a host infrastructure
>     A video engine which support VGA and DP, as well as
>     an hardware video encoder
>     Multiple PCIe ports
>     A PECI interface, and LPC eSPI
>     Multiple UART for debug purpose, and Virtual UART for
>     host connectivity
>     A GPIO engine.

More whitespace damage here, probably from a copy-paste mistake.

> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> ---
>  arch/arm/configs/gxp_defconfig | 243 +++++++++++++++++++++++++++++++++
>  1 file changed, 243 insertions(+)
>  create mode 100644 arch/arm/configs/gxp_defconfig
>
> diff --git a/arch/arm/configs/gxp_defconfig b/arch/arm/configs/gxp_defconfig
> new file mode 100644
> index 000000000000..f37c6630e06d
> --- /dev/null
> +++ b/arch/arm/configs/gxp_defconfig
> @@ -0,0 +1,243 @@
> +CONFIG_KERNEL_XZ=y
> +CONFIG_DEFAULT_HOSTNAME="gxp"
> +CONFIG_SYSVIPC=y
> +CONFIG_NO_HZ=y
> +CONFIG_HIGH_RES_TIMERS=y
> +CONFIG_BSD_PROCESS_ACCT=y
> +CONFIG_BSD_PROCESS_ACCT_V3=y
> +CONFIG_LOG_BUF_SHIFT=18
> +CONFIG_CFS_BANDWIDTH=y
> +CONFIG_RT_GROUP_SCHED=y

Try to trim the bits that you don't actually rely on, such as hostname

> +CONFIG_CGROUP_FREEZER=y
> +CONFIG_CGROUP_DEVICE=y
> +CONFIG_CGROUP_CPUACCT=y
> +CONFIG_NAMESPACES=y
> +CONFIG_SCHED_AUTOGROUP=y
> +CONFIG_RELAY=y
> +CONFIG_BLK_DEV_INITRD=y
> +CONFIG_CC_OPTIMIZE_FOR_SIZE=y
> +CONFIG_KALLSYMS_ALL=y
> +CONFIG_EMBEDDED=y

You probably don't need BLK_DEV_INITRD if you
use initramfs instead, and you should not need EMBEDDED either.

> +# CONFIG_COMPAT_BRK is not set
> +CONFIG_SLAB=y
> +CONFIG_ARCH_MULTI_V6=y

Since there is only one ARMv7 SoC enabled in here, there is no need for enabling
ARMv6. A v7-only kernel will run more efficiently and allow you to
build with more
features such as THUMB2.

> +CONFIG_ZBOOT_ROM_TEXT=0x0
> +CONFIG_ZBOOT_ROM_BSS=0x0

These are just the default

> +CONFIG_NETDEVICES=y
> +# CONFIG_NET_VENDOR_ALACRITECH is not set
> +# CONFIG_NET_VENDOR_AMAZON is not set
> +# CONFIG_NET_VENDOR_AQUANTIA is not set
> +# CONFIG_NET_VENDOR_ARC is not set
> +# CONFIG_NET_VENDOR_AURORA is not set
> +# CONFIG_NET_VENDOR_BROADCOM is not set
> +# CONFIG_NET_VENDOR_CADENCE is not set
> +# CONFIG_NET_VENDOR_CAVIUM is not set
> +# CONFIG_NET_VENDOR_CIRRUS is not set
> +# CONFIG_NET_VENDOR_CORTINA is not set
> +# CONFIG_NET_VENDOR_EZCHIP is not set
> +# CONFIG_NET_VENDOR_FARADAY is not set
> +# CONFIG_NET_VENDOR_GOOGLE is not set
> +# CONFIG_NET_VENDOR_HISILICON is not set
> +# CONFIG_NET_VENDOR_HUAWEI is not set
> +# CONFIG_NET_VENDOR_INTEL is not set
> +# CONFIG_NET_VENDOR_MARVELL is not set
> +# CONFIG_NET_VENDOR_MELLANOX is not set
> +# CONFIG_NET_VENDOR_MICREL is not set
> +# CONFIG_NET_VENDOR_MICROCHIP is not set
> +# CONFIG_NET_VENDOR_MICROSEMI is not set
> +# CONFIG_NET_VENDOR_NATSEMI is not set
> +# CONFIG_NET_VENDOR_NETRONOME is not set
> +# CONFIG_NET_VENDOR_NI is not set
> +# CONFIG_NET_VENDOR_QUALCOMM is not set
> +# CONFIG_NET_VENDOR_RENESAS is not set
> +# CONFIG_NET_VENDOR_ROCKER is not set
> +# CONFIG_NET_VENDOR_SAMSUNG is not set
> +# CONFIG_NET_VENDOR_SEEQ is not set
> +# CONFIG_NET_VENDOR_SOLARFLARE is not set
> +# CONFIG_NET_VENDOR_SMSC is not set
> +# CONFIG_NET_VENDOR_SOCIONEXT is not set
> +# CONFIG_NET_VENDOR_STMICRO is not set
> +# CONFIG_NET_VENDOR_SYNOPSYS is not set
> +# CONFIG_NET_VENDOR_VIA is not set
> +# CONFIG_NET_VENDOR_WIZNET is not set
> +# CONFIG_NET_VENDOR_XILINX is not set

No need to mention all these, leaving them default-enabled is fine.

> +CONFIG_TTY_PRINTK=y
> +CONFIG_IPMI_HANDLER=y
> +CONFIG_IPMI_DEVICE_INTERFACE=y
> +CONFIG_IPMI_SI=y
> +CONFIG_IPMI_SSIF=y
> +CONFIG_HPE_KCS_IPMI_BMC=y
> +CONFIG_HW_RANDOM_TIMERIOMEM=y
> +CONFIG_I2C_CHARDEV=y
> +CONFIG_I2C_GXP=y
> +CONFIG_I2C_SLAVE=y
> +CONFIG_I2C_SLAVE_EEPROM=y
> +CONFIG_SPI=y
> +CONFIG_GPIOLIB=y
> +CONFIG_GPIO_SYSFS=y
> +CONFIG_GPIO_GXP=y
> +CONFIG_SENSORS_EMC1403=y
> +CONFIG_SENSORS_GXP_FAN_CTRL=y
> +CONFIG_SENSORS_GXP_CORETEMP=y
> +CONFIG_SENSORS_GXP_PSU=y
> +CONFIG_SENSORS_GXP_POWER=y

Maybe leave out the custom drivers for now, and only
enable the drivers that are already merged, or added as
part of the same series.

> +CONFIG_WATCHDOG=y
> +CONFIG_GXP_WATCHDOG=y
> +CONFIG_MFD_SYSCON=y
> +CONFIG_FB=y
> +CONFIG_FB_THUMBNAIL=y
> +CONFIG_FB_SIMPLE=y

I would keep CONFIG_FB disabled for new platforms.

> +CONFIG_DEBUG_USER=y
> +CONFIG_DEBUG_LL=y
> +CONFIG_DEBUG_LL_UART_8250=y
> +CONFIG_DEBUG_UART_PHYS=0xC00000F0
> +CONFIG_DEBUG_UART_VIRT=0xF00000F0
> +CONFIG_DEBUG_UART_8250_SHIFT=0
> +CONFIG_EARLY_PRINTK=y

Better leave out the debugging options here, and only list the ones
that you would
enable in a production device.

        Arnd