Re: [PATCH v1] ARM: Add support for Realtek SOC

["Andreas Färber" <afaerber@suse.de>]

Hi James,

Am Mittwoch, den 25.09.2019, 06:43 +0000 schrieb James Tai:
> From: "james.tai" <james.tai@realtek.com>
> 
> This patch adds the basic machine file for
> the Realtek RTD16XX and RTD13XX platform.
> 
> Signed-off-by: james.tai <james.tai@realtek.com>
> ---
> Changes since last version:
> 	- Add RTD13XX platform.
> 	- Add PSCI support.
> 	- Add ARCH_MULTI_V7 config.
> 	- remove 'textofs-$(CONFIG_ARCH_REALTEK) := 0x00208000' from 
> 	  'arch/arm/Makefile'.
> 	- remove map_io,init_time,init_machine and smp_init from
> machine
> 	  descriptor.

Thanks for your patch. Please subscribe yourself and colleagues to the
existing linux-realtek-soc@lists.infradead.org mailing list and make
sure it is in CC for your patches:

http://lists.infradead.org/mailman/listinfo/linux-realtek-soc

Further comments inline:

> ---
>  arch/arm/Kconfig                |  2 ++
>  arch/arm/Makefile               |  1 +
>  arch/arm/mach-realtek/Kconfig   | 20 +++++++++++++
>  arch/arm/mach-realtek/Makefile  |  3 ++
>  arch/arm/mach-realtek/platsmp.c | 51
> +++++++++++++++++++++++++++++++++
>  arch/arm/mach-realtek/platsmp.h |  6 ++++
>  arch/arm/mach-realtek/realtek.c | 43 +++++++++++++++++++++++++++
>  7 files changed, 126 insertions(+)
>  create mode 100644 arch/arm/mach-realtek/Kconfig
>  create mode 100644 arch/arm/mach-realtek/Makefile
>  create mode 100644 arch/arm/mach-realtek/platsmp.c
>  create mode 100644 arch/arm/mach-realtek/platsmp.h
>  create mode 100644 arch/arm/mach-realtek/realtek.c

As Arnd has already expressed, at least RTD13xx (if not also RTD16xx?)
is a 64-bit SoC, and we already have code - contributed and maintained
by me - for arch/arm64/. Please contribute to those efforts instead of
building your own sandcastle in arch/arm/. In fact, your work collides
with patches queued in my rtd129x-next branch for RTD1195, an actual
32-bit platform. Among other commits:

https://github.com/afaerber/linux/commit/b43fa4f790183d46e2b2c7f5af34f3010d315073

Those previous arm64 RTD129x and arm RTD1195 efforts were and are
blocked by irqchip, which I don't see addressed in this single patch.

https://github.com/afaerber/linux/commits/rtd1295-next

> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 33b00579beff..1f7967c97267 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -793,6 +793,8 @@ source "arch/arm/mach-realview/Kconfig"
>  
>  source "arch/arm/mach-rockchip/Kconfig"
>  
> +source "arch/arm/mach-realtek/Kconfig"

Ordering wrong.

> +
>  source "arch/arm/mach-s3c24xx/Kconfig"
>  
>  source "arch/arm/mach-s3c64xx/Kconfig"
> diff --git a/arch/arm/Makefile b/arch/arm/Makefile
> index c3624ca6c0bc..560ae7d72aab 100644
> --- a/arch/arm/Makefile
> +++ b/arch/arm/Makefile
> @@ -205,6 +205,7 @@ machine-$(CONFIG_ARCH_RDA)		+= rda
>  machine-$(CONFIG_ARCH_REALVIEW)		+= realview
>  machine-$(CONFIG_ARCH_ROCKCHIP)		+= rockchip
>  machine-$(CONFIG_ARCH_RPC)		+= rpc
> +machine-$(CONFIG_ARCH_REALTEK)		+= realtek

Ditto.

>  machine-$(CONFIG_ARCH_S3C24XX)		+= s3c24xx
>  machine-$(CONFIG_ARCH_S3C64XX)		+= s3c64xx
>  machine-$(CONFIG_ARCH_S5PV210)		+= s5pv210
> diff --git a/arch/arm/mach-realtek/Kconfig b/arch/arm/mach-
> realtek/Kconfig
> new file mode 100644
> index 000000000000..a638f4322bb2
> --- /dev/null
> +++ b/arch/arm/mach-realtek/Kconfig
> @@ -0,0 +1,20 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +menuconfig ARCH_REALTEK
> +	bool "Realtek SoC Support"
> +	depends on ARCH_MULTI_V7
> +	help
> +	  Support for Realtek rtd16xx & rtd13xx SoCs.
> +
> +if ARCH_REALTEK
> +
> +config ARCH_RTD13XX
> +	bool "Enable support for RTD1319"
> +	select ARM_GIC_V3
> +	select ARM_PSCI
> +
> +config ARCH_RTD16XX
> +	bool "Enable support for RTD1619"
> +	select ARM_GIC_V3
> +	select ARM_PSCI
> +
> +endif
> diff --git a/arch/arm/mach-realtek/Makefile b/arch/arm/mach-
> realtek/Makefile
> new file mode 100644
> index 000000000000..9cdc1f1f2917
> --- /dev/null
> +++ b/arch/arm/mach-realtek/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_ARCH_REALTEK) += realtek.o
> +obj-$(CONFIG_SMP) += platsmp.o
> diff --git a/arch/arm/mach-realtek/platsmp.c b/arch/arm/mach-
> realtek/platsmp.c
> new file mode 100644
> index 000000000000..b3fc99447ad4
> --- /dev/null
> +++ b/arch/arm/mach-realtek/platsmp.c
> @@ -0,0 +1,51 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2019 Realtek Semiconductor Corp.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/delay.h>
> +#include <linux/jiffies.h>
> +#include <linux/io.h>
> +#include <linux/memory.h>
> +#include <linux/smp.h>
> +#include <linux/of.h>
> +#include <linux/arm-smccc.h>
> +#include <asm/smp_plat.h>
> +#include <asm/cacheflush.h>
> +#include <asm/cp15.h>
> +#include <asm/barrier.h>
> +
> +#define BL31_CMD 0x8400ff04
> +#define BL31_DAT 0x00001619
> +#define CORE_PWRDN_EN 0x1
> +
> +#define CPUPWRCTLR __ACCESS_CP15(c15, 0, c2, 7)
> +
> +#ifdef CONFIG_HOTPLUG_CPU
> +
> +static void rtk_cpu_die(unsigned int cpu)
> +{
> +	struct arm_smccc_res res;
> +	unsigned int cpu_pwr_ctrl;
> +
> +	/* notify BL31 cpu hotplug */
> +	arm_smccc_smc(BL31_CMD, BL31_DAT, 0, 0, 0, 0, 0, 0, &res);

BL31 is clearly for 64-bit only and will not work for RTD1195, so the
naming is much too generic.

> +	v7_exit_coherency_flush(louis);
> +
> +	cpu_pwr_ctrl = read_sysreg(CPUPWRCTLR);
> +	cpu_pwr_ctrl |= CORE_PWRDN_EN;
> +	write_sysreg(cpu_pwr_ctrl, CPUPWRCTLR);
> +
> +	dsb(sy);
> +
> +	for (;;)
> +		wfi();
> +}
> +#endif
> +
> +struct smp_operations rtk_smp_ops __initdata = {
> +#ifdef CONFIG_HOTPLUG_CPU
> +	.cpu_die = rtk_cpu_die,
> +#endif
> +};
> diff --git a/arch/arm/mach-realtek/platsmp.h b/arch/arm/mach-
> realtek/platsmp.h
> new file mode 100644
> index 000000000000..c9c4d712369c
> --- /dev/null
> +++ b/arch/arm/mach-realtek/platsmp.h
> @@ -0,0 +1,6 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2019 Realtek Semiconductor Corp.
> + */
> +
> +extern struct smp_operations rtk_smp_ops;
> diff --git a/arch/arm/mach-realtek/realtek.c b/arch/arm/mach-
> realtek/realtek.c
> new file mode 100644
> index 000000000000..2692ac53f59b
> --- /dev/null
> +++ b/arch/arm/mach-realtek/realtek.c
> @@ -0,0 +1,43 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2019 Realtek Semiconductor Corp.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/clocksource.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/irqchip.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/io.h>
> +#include <linux/memblock.h>
> +#include <linux/delay.h>
> +#include <linux/clockchips.h>
> +#include <asm/mach/arch.h>
> +#include <asm/mach/map.h>
> +#include <asm/system_misc.h>
> +#include <asm/system_info.h>
> +
> +#include "platsmp.h"
> +
> +static const char *const rtd13xx_board_dt_compat[] = {
> +	"realtek,rtd1319",
> +	NULL,
> +};
> +
> +static const char *const rtd16xx_board_dt_compat[] = {
> +	"realtek,rtd1619",
> +	NULL,
> +};
> +
> +DT_MACHINE_START(RTD13XX, "Realtek rtd13xx platform")
> +	.dt_compat = rtd13xx_board_dt_compat,
> +	.smp = smp_ops(rtk_smp_ops),
> +MACHINE_END
> +
> +DT_MACHINE_START(RTD16XX, "Realtek rtd16xx platform")
> +	.dt_compat = rtd16xx_board_dt_compat,
> +	.smp = smp_ops(rtk_smp_ops),
> +MACHINE_END

I recall that 32-bit arm SMP can be selected via the DT and then does
not need multiple such structs per SoC, in particular they don't differ
at all and could use the default machine taking the text from the DT.

Also note that whenever possible I personally have a preference for
GPL-2.0-or-later and licensed my code that way whenever possible.

Question: Why are you not just implementing the above CPU logic in TF-A 
BL31 and let Linux use the existing PSCI code paths? That will be the
only upstream-accepted solution for arm64.

Thanks,
Andreas

-- 
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 247165 (AG München)


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel