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

[James Tai <james.tai@realtek.com>]

> Subject: Re: [PATCH v1] ARM: Add support for Realtek SOC
> 
> 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

Hi Andreas,

Thanks for your reply.

I can't open the URL. 
Please check this URL is correct.
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:

OK. I understand.

We want to open source for our platforms.
The newly platforms source code should be patch to rtd129x-next branch? 
These platforms is included arm64 and arm.

> https://github.com/afaerber/linux/commit/b43fa4f790183d46e2b2c7f5af34f3
> 010d315073
> 
> 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

What kind of irq issue did you have with the development.
For rtd129x platform, I can't find the irq mux driver in 'drivers/irqchip'.

> >
> > 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.

OK. Thank you.

> > +
> >  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.

OK. I will modify naming in new version patch.

> > +	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.

OK. I understand.

> 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.

For rtd129x platform, We don't implement PSCI in BL31, 
but our newly platforms(rtd139x/rtd13xx/rtd16xx) already implement PSCI in BL31.

> Thanks,
> Andreas
> 
> --
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer
> HRB 247165 (AG München)
> 
> 
> ------Please consider the environment before printing this e-mail.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel