MIME-Version: 1.0
References: <cover.1638515726.git.qinjian@cqplus1.com> <c476d24e4492ad4cfff532a10d17a6a09c3a0e1a.1638515726.git.qinjian@cqplus1.com>
In-Reply-To: <c476d24e4492ad4cfff532a10d17a6a09c3a0e1a.1638515726.git.qinjian@cqplus1.com>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Fri, 3 Dec 2021 13:49:03 +0100
Message-ID: <CAK8P3a0d9Z9ikM6AXGZd7p1T_zJUZg0fwHK=e5CeXtdniL3ExA@mail.gmail.com>
Subject: Re: [PATCH v5 10/10] ARM: sp7021_defconfig: Add Sunplus SP7021 defconfig
To:     Qin Jian <qinjian@cqplus1.com>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Marc Zyngier <maz@kernel.org>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Russell King - ARM Linux <linux@armlinux.org.uk>,
        Mark Brown <broonie@kernel.org>, Arnd Bergmann <arnd@arndb.de>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        DTML <devicetree@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-clk <linux-clk@vger.kernel.org>, wells.lu@sunplus.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Dec 3, 2021 at 8:34 AM Qin Jian <qinjian@cqplus1.com> wrote:
>
> Add generic Sunplus SP7021 based board defconfig
>
> Signed-off-by: Qin Jian <qinjian@cqplus1.com>
> ---
>  MAINTAINERS                       |   1 +
>  arch/arm/configs/sp7021_defconfig | 178 ++++++++++++++++++++++++++++++

Most platforms don't even have a custom defconfig any more. If you
think you need this one,
I would suggest naming it sunplus_defconfig so it makes more sense
when other related
SoCs are added to it.

I would also suggest enabling your platform specific drivers in
multi_v7_defconfig,
and testing that one to ensure it works.

> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0ae537a41..9340f8760 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2664,6 +2664,7 @@ F:        Documentation/devicetree/bindings/arm/sunplus,sp7021.yaml
>  F:     Documentation/devicetree/bindings/clock/sunplus,sp7021-clkc.yaml
>  F:     Documentation/devicetree/bindings/interrupt-controller/sunplus,sp7021-intc.yaml
>  F:     Documentation/devicetree/bindings/reset/sunplus,reset.yaml
> +F:     arch/arm/configs/sp7021_*defconfig
>  F:     arch/arm/mach-sunplus/
>  F:     drivers/clk/clk-sp7021.c
>  F:     drivers/irqchip/irq-sp7021-intc.c
> diff --git a/arch/arm/configs/sp7021_defconfig b/arch/arm/configs/sp7021_defconfig
> new file mode 100644
> index 000000000..249fe779e
> --- /dev/null
> +++ b/arch/arm/configs/sp7021_defconfig
> @@ -0,0 +1,178 @@
> +CONFIG_DEFAULT_HOSTNAME="SP7021-Ev"

I'd leave this out of the defconfig

> +CONFIG_SYSVIPC=y
> +CONFIG_USELIB=y

You almost certainly don't need CONFIG_USELIB

> +CONFIG_BLK_DEV_INITRD=y
> +CONFIG_INITRAMFS_SOURCE="../rootfs/initramfs/disk/ ../rootfs/initramfs/initramfs.devnodes"
> +CONFIG_INITRAMFS_ROOT_UID=-1

This will cause a build failure for everyone else I think

> +CONFIG_EMBEDDED=y

Probably not needed, otherwise explain what option you need to touch
that depends on this.

> +CONFIG_PERF_EVENTS=y
> +CONFIG_SLAB=y
> +CONFIG_ARCH_SUNPLUS=y
> +# CONFIG_VDSO is not set
> +CONFIG_SMP=y
> +CONFIG_HAVE_ARM_ARCH_TIMER=y
> +CONFIG_THUMB2_KERNEL=y

Ah nice, we don't have enough defconfigs using THUMB2_KERNEL. This may mean
that you get reports about thumb2 specific bugs that nobody else runs
into, but that is
better than if nobody uses that.

> +CONFIG_HIGHMEM=y
> +# CONFIG_HIGHPTE is not set

What is the maximum amount of supported RAM? Note that HIGHMEM support
will likely
go away at some point, so it may be better to use a good VMSPLIT
setting that works with
all your machines without requiring highmem.

> +CONFIG_ZBOOT_ROM_TEXT=0x98307000
> +CONFIG_ZBOOT_ROM_BSS=0x03400000

Where do these come from? I see you don't set CONFIG_ZBOOT_ROM, so they will be
ignored, but if you do rely on these, it likely means that your
machine is unable to
run a multi_v7_defconfig kernel, which is bad.

> +CONFIG_CMDLINE="root=/dev/ram rw init=/init console=ttyS0,115200 earlyprintk mem=512M@0x0"

Remove this, the configuration should come from the bootloader.

> +CONFIG_AUTO_ZRELADDR=y

This is always enabled

> +CONFIG_CAIF=y

Are you actually using CAIF? Which driver do you use?

> +CONFIG_CEPH_LIB=y
> +CONFIG_CEPH_LIB_USE_DNS_RESOLVER=y

Same here, do you use this?

> +CONFIG_INPUT_MISC=y
> +CONFIG_INPUT_UINPUT=y
> +CONFIG_SERIO_LIBPS2=y

Do you have a physical PS/2 keyboard connector

> +CONFIG_USB_VIDEO_CLASS=y
> +CONFIG_FB=y
> +CONFIG_FB_SP7021=y

CONFIG_FB is no longer recommended for new platforms, please use
CONFIG_SIMPLEDRM for the moment, until you have converted your
driver to the DRM framework.

> +CONFIG_GEMINI_USB=y

What is this?

> +CONFIG_EXT2_FS=y

If you use eMMC storage, I recommend using EXT4_FS instead of EXT2_FS


         Arnd