MIME-Version: 1.0
References: <cover.1638515726.git.qinjian@cqplus1.com> <eabfe1b84b889e4aa95e24c30a114c68ef95fd07.1638515726.git.qinjian@cqplus1.com>
In-Reply-To: <eabfe1b84b889e4aa95e24c30a114c68ef95fd07.1638515726.git.qinjian@cqplus1.com>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Fri, 3 Dec 2021 13:59:16 +0100
Message-ID: <CAK8P3a1_coAnp8P3L2UA+smxuRL9widFQv9Y5ZZ0X_Sr9zsZtg@mail.gmail.com>
Subject: Re: [PATCH v5 09/10] ARM: sunplus: Add initial support for Sunplus
 SP7021 SoC
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:

> @@ -487,6 +487,22 @@ config ARCH_S3C24XX
>           (<http://www.simtec.co.uk/products/EB110ITX/>), the IPAQ 1940 or the
>           Samsung SMDK2410 development board (and derivatives).
>
> +config ARCH_SUNPLUS
> +       bool "Sunplus SoCs"
> +       select CLKSRC_OF
> +       select COMMON_CLK
> +       select GENERIC_CLOCKEVENTS
> +       select GENERIC_IRQ_CHIP
> +       select GENERIC_IRQ_MULTI_HANDLER
> +       select USE_OF
> +       select RTC_CLASS
> +       select RESET_SUNPLUS

This is in the wrong place: move the Kconfig entry into
arch/arm/mach-sunplus/Kconfig
and make it 'depends on ARCH_MULTI_V7'.

I think you can remove all the 'select' lines as well because they are
either implied by
ARCH_MULTI_V7 or not actually necessary.

> @@ -152,6 +152,7 @@ textofs-$(CONFIG_ARCH_MSM8X60) := 0x00208000
>  textofs-$(CONFIG_ARCH_MSM8960) := 0x00208000
>  textofs-$(CONFIG_ARCH_MESON) := 0x00208000
>  textofs-$(CONFIG_ARCH_AXXIA) := 0x00308000
> +textofs-$(CONFIG_ARCH_SUNPLUS) := 0x00308000

What is this needed for? If it boots without this line, better avoid
adding it, because
it will increase the kernel size for everyone else (unless they also enable
AXXIA).

> +config SOC_SP7021
> +       bool "Sunplus SP7021 SoC support"
> +       default y

No 'default y' here. You can probably remove this option completely
and fold it into
ARCH_SUNPLUS.

> +       select CPU_V7
> +       select ARM_GIC
> +       select SUNPLUS_SP7021_INTC
> +       select HAVE_SMP
> +       select ARM_PSCI
> +       select COMMON_CLK_SP7021
> +       select PINCTRL
> +       select PINCTRL_SPPCTL
> +       select OF_OVERLAY
> +       select GPIOLIB
> +       help

Again, most of these should be implied by ARCH_MULTI_V7, so remove those.
For individual drivers, try to avoid the 'select' unless this is required by
the respective driver subsystem.

> diff --git a/arch/arm/mach-sunplus/Makefile.boot b/arch/arm/mach-sunplus/Makefile.boot
> new file mode 100644
> index 000000000..401c30840
> --- /dev/null
> +++ b/arch/arm/mach-sunplus/Makefile.boot
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +zreladdr-$(CONFIG_ARCH_SUNPLUS) := 0x00308000

This should not be needed any more.

       Arnd