[PATCH] ARM: Add support for Realtek SOC

[PATCH] ARM: Add support for Realtek SOC

[jamestai.sky]

From: "james.tai" <james.tai@realtek.com>

This patch adds the basic machine file for
the Realtek RTD16XX platform.

Signed-off-by: james.tai <james.tai@realtek.com>
---
 arch/arm/Kconfig                |  2 +
 arch/arm/Makefile               |  2 +
 arch/arm/mach-realtek/Kconfig   | 32 ++++++++++++
 arch/arm/mach-realtek/Makefile  |  3 ++
 arch/arm/mach-realtek/platsmp.c | 91 +++++++++++++++++++++++++++++++++
 arch/arm/mach-realtek/platsmp.h |  7 +++
 arch/arm/mach-realtek/realtek.c | 78 ++++++++++++++++++++++++++++
 7 files changed, 215 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

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 33b00579beff..c7c9a3662eb7 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -836,6 +836,8 @@ source "arch/arm/mach-zx/Kconfig"
 
 source "arch/arm/mach-zynq/Kconfig"
 
+source "arch/arm/mach-realtek/Kconfig"
+
 # ARMv7-M architecture
 config ARCH_EFM32
 	bool "Energy Micro efm32"
diff --git a/arch/arm/Makefile b/arch/arm/Makefile
index c3624ca6c0bc..1f0926449d47 100644
--- a/arch/arm/Makefile
+++ b/arch/arm/Makefile
@@ -148,6 +148,7 @@ endif
 textofs-$(CONFIG_ARCH_MSM8X60) := 0x00208000
 textofs-$(CONFIG_ARCH_MSM8960) := 0x00208000
 textofs-$(CONFIG_ARCH_MESON) := 0x00208000
+textofs-$(CONFIG_ARCH_REALTEK) := 0x00208000
 textofs-$(CONFIG_ARCH_AXXIA) := 0x00308000
 
 # Machine directory name.  This list is sorted alphanumerically
@@ -225,6 +226,7 @@ machine-$(CONFIG_ARCH_VT8500)		+= vt8500
 machine-$(CONFIG_ARCH_W90X900)		+= w90x900
 machine-$(CONFIG_ARCH_ZX)		+= zx
 machine-$(CONFIG_ARCH_ZYNQ)		+= zynq
+machine-$(CONFIG_ARCH_REALTEK)		+= realtek
 machine-$(CONFIG_PLAT_SPEAR)		+= spear
 
 # Platform directory name.  This list is sorted alphanumerically
diff --git a/arch/arm/mach-realtek/Kconfig b/arch/arm/mach-realtek/Kconfig
new file mode 100644
index 000000000000..a8269964dbdb
--- /dev/null
+++ b/arch/arm/mach-realtek/Kconfig
@@ -0,0 +1,32 @@
+# SPDX-License-Identifier: GPL-2.0-only
+menuconfig ARCH_REALTEK
+	bool "Realtek SoCs"
+	select ARM_GLOBAL_TIMER
+	select CLKDEV_LOOKUP
+	select HAVE_SMP
+	select HAVE_MACH_CLKDEV
+	select GENERIC_CLOCKEVENTS
+	select HAVE_SCHED_CLOCK
+	select ARCH_HAS_CPUFREQ
+	select CLKSRC_OF
+	select ARCH_REQUIRE_GPIOLIB
+	select GENERIC_IRQ_CHIP
+	select IRQ_DOMAIN
+	select PINCTRL
+	select COMMON_CLK
+	select ARCH_HAS_BARRIERS
+	select SPARSE_IRQ
+	select PM_OPP
+	select ARM_HAS_SG_CHAIN
+	select ARM_PATCH_PHYS_VIRT
+	select AUTO_ZRELADDR
+	select MIGHT_HAVE_PCI
+	select MULTI_IRQ_HANDLER
+	select PCI_DOMAINS if PCI
+	select USE_OF
+
+config ARCH_RTD16XX
+	bool "Enable support for RTD1619"
+	depends on ARCH_REALTEK
+	select ARM_GIC_V3
+	select ARM_PSCI
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..5c4368fe1520
--- /dev/null
+++ b/arch/arm/mach-realtek/platsmp.c
@@ -0,0 +1,91 @@
+// 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 CPUID 28
+#define CORE_PWRDN_EN 0x1
+
+#define CPUPWRCTLR __ACCESS_CP15(c15, 0, c2, 7)
+
+static u32 __iomem *cpu_release_virt;
+
+static int rtk_boot_secondary(unsigned int cpu, struct task_struct *idle)
+{
+	unsigned long entry_pa = __pa_symbol(secondary_startup);
+
+	writel_relaxed(entry_pa | (cpu << CPUID), cpu_release_virt);
+
+	arch_send_wakeup_ipi_mask(cpumask_of(cpu));
+
+	return 0;
+}
+
+void rtk_prepare_cpus(unsigned int max_cpus)
+{
+	struct device_node *np;
+	u32 release_phys;
+	int cpu;
+
+	for_each_possible_cpu(cpu) {
+
+		np = of_get_cpu_node(cpu, NULL);
+		if (!np)
+			continue;
+
+		if (of_property_read_u32(np, "cpu-release-addr", &release_phys))
+			continue;
+
+		cpu_release_virt = ioremap(release_phys, sizeof(u32));
+
+		set_cpu_present(cpu, true);
+	}
+}
+
+#ifdef CONFIG_HOTPLUG_CPU
+
+static void rtk_cpu_die(unsigned int cpu)
+{
+	struct arm_smccc_res res;
+	unsigned int cpu_pwr_ctrl;
+
+	writel_relaxed(0x0, cpu_release_virt);
+
+	/* notify BL31 cpu hotplug */
+	arm_smccc_smc(BL31_CMD, BL31_DAT, 0, 0, 0, 0, 0, 0, &res);
+	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 = {
+	.smp_prepare_cpus = rtk_prepare_cpus,
+	.smp_boot_secondary = rtk_boot_secondary,
+#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..9c411d097c14
--- /dev/null
+++ b/arch/arm/mach-realtek/platsmp.h
@@ -0,0 +1,7 @@
+/* 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..d248e19f4c1d
--- /dev/null
+++ b/arch/arm/mach-realtek/realtek.c
@@ -0,0 +1,78 @@
+// 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"
+
+#define RBUS_BASE_PHYS (0x98000000)
+#define RBUS_BASE_VIRT (0xfe000000)
+#define RBUS_BASE_SIZE (0x00100000)
+
+static struct map_desc rtk_io_desc[] __initdata = {
+	{
+		.virtual = (unsigned long) IOMEM(RBUS_BASE_VIRT),
+		.pfn = __phys_to_pfn(RBUS_BASE_PHYS),
+		.length = RBUS_BASE_SIZE,
+		.type = MT_DEVICE,
+	},
+};
+
+void __init rtk_map_io(void)
+{
+	debug_ll_io_init();
+	iotable_init(rtk_io_desc, ARRAY_SIZE(rtk_io_desc));
+}
+
+static void __init rtk_dt_init(void)
+{
+	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
+}
+
+static void __init rtk_timer_init(void)
+{
+#ifdef CONFIG_COMMON_CLK
+	of_clk_init(NULL);
+#endif
+	timer_probe();
+	tick_setup_hrtimer_broadcast();
+}
+
+bool __init rtk_smp_init_ops(void)
+{
+	smp_set_ops(smp_ops(rtk_smp_ops));
+
+	return true;
+}
+
+static const char *const rtd16xx_board_dt_compat[] = {
+	"realtek,rtd1619",
+	NULL,
+};
+
+DT_MACHINE_START(RTD16XX, "Realtek rtd16xx platform")
+	.map_io = rtk_map_io,
+	.init_machine = rtk_dt_init,
+	.init_time = rtk_timer_init,
+	.dt_compat = rtd16xx_board_dt_compat,
+	.smp = smp_ops(rtk_smp_ops),
+	.smp_init = smp_init_ops(rtk_smp_init_ops),
+MACHINE_END
-- 
2.17.1

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

[Arnd Bergmann]

On Thu, Sep 5, 2019 at 7:48 AM <jamestai.sky@gmail.com> wrote:
>
> From: "james.tai" <james.tai@realtek.com>
>
> This patch adds the basic machine file for
> the Realtek RTD16XX platform.
>
> Signed-off-by: james.tai <james.tai@realtek.com>

Hi James,

Thanks a lot for your submission! I'm glad to see interest in upstream
support for this SoC family. I have a few small comments on
details, mostly where I would either like to see an explanation
in the patch description, or things that looks like they can be
left out from the patch.

> index 33b00579beff..c7c9a3662eb7 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -836,6 +836,8 @@ source "arch/arm/mach-zx/Kconfig"
>
>  source "arch/arm/mach-zynq/Kconfig"
>
> +source "arch/arm/mach-realtek/Kconfig"
> +
> diff --git a/arch/arm/mach-realtek/Kconfig b/arch/arm/mach-realtek/Kconfig
> @@ -225,6 +226,7 @@ machine-$(CONFIG_ARCH_VT8500)               += vt8500
>  machine-$(CONFIG_ARCH_W90X900)         += w90x900
>  machine-$(CONFIG_ARCH_ZX)              += zx
>  machine-$(CONFIG_ARCH_ZYNQ)            += zynq
> +machine-$(CONFIG_ARCH_REALTEK)         += realtek
>  machine-$(CONFIG_PLAT_SPEAR)           += spear
>
>  # Platform directory name.  This list is sorted alphanumerically

Please keep these lists in alphabetical order.

>  # ARMv7-M architecture
>  config ARCH_EFM32
>         bool "Energy Micro efm32"
> diff --git a/arch/arm/Makefile b/arch/arm/Makefile
> index c3624ca6c0bc..1f0926449d47 100644
> --- a/arch/arm/Makefile
> +++ b/arch/arm/Makefile
> @@ -148,6 +148,7 @@ endif
>  textofs-$(CONFIG_ARCH_MSM8X60) := 0x00208000
>  textofs-$(CONFIG_ARCH_MSM8960) := 0x00208000
>  textofs-$(CONFIG_ARCH_MESON) := 0x00208000
> +textofs-$(CONFIG_ARCH_REALTEK) := 0x00208000
>  textofs-$(CONFIG_ARCH_AXXIA) := 0x00308000

Can you explain why this is needed for your platform?

>  # Machine directory name.  This list is sorted alphanumerically
> new file mode 100644
> index 000000000000..a8269964dbdb
> --- /dev/null
> +++ b/arch/arm/mach-realtek/Kconfig
> @@ -0,0 +1,32 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +menuconfig ARCH_REALTEK
> +       bool "Realtek SoCs"

Please add "depends on ARCH_MULTI_V7" to avoid
compile time issues when selecting it on an earlier
architecture.

> +       select ARM_GLOBAL_TIMER
> +       select CLKDEV_LOOKUP
> +       select HAVE_SMP
> +       select HAVE_MACH_CLKDEV
> +       select GENERIC_CLOCKEVENTS
> +       select HAVE_SCHED_CLOCK
> +       select ARCH_HAS_CPUFREQ
> +       select CLKSRC_OF
> +       select ARCH_REQUIRE_GPIOLIB
> +       select GENERIC_IRQ_CHIP
> +       select IRQ_DOMAIN
> +       select PINCTRL
> +       select COMMON_CLK
> +       select ARCH_HAS_BARRIERS
> +       select SPARSE_IRQ
> +       select PM_OPP
> +       select ARM_HAS_SG_CHAIN
> +       select ARM_PATCH_PHYS_VIRT
> +       select AUTO_ZRELADDR
> +       select MIGHT_HAVE_PCI
> +       select MULTI_IRQ_HANDLER
> +       select PCI_DOMAINS if PCI
> +       select USE_OF

Almost all of the symbols above are implied by
ARCH_MULTI_V7 and should not be selected
separately.

> +config ARCH_RTD16XX
> +       bool "Enable support for RTD1619"
> +       depends on ARCH_REALTEK
> +       select ARM_GIC_V3
> +       select ARM_PSCI

As I understand, this chip uses a Cortex-A55. What is the reason
for adding support only to the 32-bit ARM architecture rather than
64-bit?

Most 64-bit SoCs are only supported with arch/arm64, but generally
speaking that is not a requirement. My rule of thumb is that on
systems with 1GB of RAM or more, one would want to run a 64-bit
kernel, while systems with less than that are better off with a 32-bit
one, but that is clearly not the only reason for picking one over the
other.

> +
> +static int rtk_boot_secondary(unsigned int cpu, struct task_struct *idle)
> +{
> +       unsigned long entry_pa = __pa_symbol(secondary_startup);
> +
> +       writel_relaxed(entry_pa | (cpu << CPUID), cpu_release_virt);
> +
> +       arch_send_wakeup_ipi_mask(cpumask_of(cpu));
> +
> +       return 0;
> +}

It's very unusual to see custom smp operations on an ARMv8
system, as we normally use PSCI here. Can you explain what
is going on here? Are you able to use a boot wrapper that implements
these in psci instead?
> +
> +#include "platsmp.h"
> +
> +#define RBUS_BASE_PHYS (0x98000000)
> +#define RBUS_BASE_VIRT (0xfe000000)
> +#define RBUS_BASE_SIZE (0x00100000)
> +
> +static struct map_desc rtk_io_desc[] __initdata = {
> +       {
> +               .virtual = (unsigned long) IOMEM(RBUS_BASE_VIRT),
> +               .pfn = __phys_to_pfn(RBUS_BASE_PHYS),
> +               .length = RBUS_BASE_SIZE,
> +               .type = MT_DEVICE,
> +       },
> +};

This needs a comment: Why do you require a static mapping for
"RBUS_BASE_PHYS"? Normally device drivers should just use
ioremap() for mapping whichever registers they want to access.

> +static void __init rtk_dt_init(void)
> +{
> +       of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> +}

This should be taken care of by the
of_platform_default_populate_init() and can be dropped.

> +static void __init rtk_timer_init(void)
> +{
> +#ifdef CONFIG_COMMON_CLK
> +       of_clk_init(NULL);
> +#endif

COMMON_CLK is implied by ARCH_MULTI_V7, so the
#ifdef can be dropped.

> +       timer_probe();
> +       tick_setup_hrtimer_broadcast();
> +}

What do you need tick_setup_hrtimer_broadcast() for? I don't
see any other platform calling this.

> +bool __init rtk_smp_init_ops(void)
> +{
> +       smp_set_ops(smp_ops(rtk_smp_ops));
> +
> +       return true;
> +}

I think this can also be dropped, as you set the smp_ops in the
machine descriptor.


       Arnd

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

[James Tai[戴志峰]]

> Subject: Re: [PATCH] ARM: Add support for Realtek SOC
> 
> On Thu, Sep 5, 2019 at 7:48 AM <jamestai.sky@gmail.com> wrote:
> >
> > From: "james.tai" <james.tai@realtek.com>
> >
> > This patch adds the basic machine file for the Realtek RTD16XX
> > platform.
> >
> > Signed-off-by: james.tai <james.tai@realtek.com>
> 
> Hi James,
> 
> Thanks a lot for your submission! I'm glad to see interest in upstream support
> for this SoC family. I have a few small comments on details, mostly where I
> would either like to see an explanation in the patch description, or things that
> looks like they can be left out from the patch.
>
Thanks for your reply.

> > index 33b00579beff..c7c9a3662eb7 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -836,6 +836,8 @@ source "arch/arm/mach-zx/Kconfig"
> >
> >  source "arch/arm/mach-zynq/Kconfig"
> >
> > +source "arch/arm/mach-realtek/Kconfig"
> > +
> > diff --git a/arch/arm/mach-realtek/Kconfig
> b/arch/arm/mach-realtek/Kconfig
> > @@ -225,6 +226,7 @@ machine-$(CONFIG_ARCH_VT8500)
> += vt8500
> >  machine-$(CONFIG_ARCH_W90X900)         += w90x900
> >  machine-$(CONFIG_ARCH_ZX)              += zx
> >  machine-$(CONFIG_ARCH_ZYNQ)            += zynq
> > +machine-$(CONFIG_ARCH_REALTEK)         += realtek
> >  machine-$(CONFIG_PLAT_SPEAR)           += spear
> >
> >  # Platform directory name.  This list is sorted alphanumerically
> 
> Please keep these lists in alphabetical order.
> 
I will keep these lists in alphabetical order in new version patch.

> >  # ARMv7-M architecture
> >  config ARCH_EFM32
> >         bool "Energy Micro efm32"
> > diff --git a/arch/arm/Makefile b/arch/arm/Makefile index
> > c3624ca6c0bc..1f0926449d47 100644
> > --- a/arch/arm/Makefile
> > +++ b/arch/arm/Makefile
> > @@ -148,6 +148,7 @@ endif
> >  textofs-$(CONFIG_ARCH_MSM8X60) := 0x00208000
> >  textofs-$(CONFIG_ARCH_MSM8960) := 0x00208000
> >  textofs-$(CONFIG_ARCH_MESON) := 0x00208000
> > +textofs-$(CONFIG_ARCH_REALTEK) := 0x00208000
> >  textofs-$(CONFIG_ARCH_AXXIA) := 0x00308000
> 
> Can you explain why this is needed for your platform?
> 
We need to reserve memory (0x00000000 ~ 0x001B0000) for rom and boot code.

> >  # Machine directory name.  This list is sorted alphanumerically new
> > file mode 100644 index 000000000000..a8269964dbdb
> > --- /dev/null
> > +++ b/arch/arm/mach-realtek/Kconfig
> > @@ -0,0 +1,32 @@
> > +# SPDX-License-Identifier: GPL-2.0-only menuconfig ARCH_REALTEK
> > +       bool "Realtek SoCs"
> 
> Please add "depends on ARCH_MULTI_V7" to avoid compile time issues when
> selecting it on an earlier architecture.
> 
I will add "depends on ARCH_MULTI_V7" in new version patch.

> > +       select ARM_GLOBAL_TIMER
> > +       select CLKDEV_LOOKUP
> > +       select HAVE_SMP
> > +       select HAVE_MACH_CLKDEV
> > +       select GENERIC_CLOCKEVENTS
> > +       select HAVE_SCHED_CLOCK
> > +       select ARCH_HAS_CPUFREQ
> > +       select CLKSRC_OF
> > +       select ARCH_REQUIRE_GPIOLIB
> > +       select GENERIC_IRQ_CHIP
> > +       select IRQ_DOMAIN
> > +       select PINCTRL
> > +       select COMMON_CLK
> > +       select ARCH_HAS_BARRIERS
> > +       select SPARSE_IRQ
> > +       select PM_OPP
> > +       select ARM_HAS_SG_CHAIN
> > +       select ARM_PATCH_PHYS_VIRT
> > +       select AUTO_ZRELADDR
> > +       select MIGHT_HAVE_PCI
> > +       select MULTI_IRQ_HANDLER
> > +       select PCI_DOMAINS if PCI
> > +       select USE_OF
> 
> Almost all of the symbols above are implied by
> ARCH_MULTI_V7 and should not be selected separately.
>
OK, I understand.

> > +config ARCH_RTD16XX
> > +       bool "Enable support for RTD1619"
> > +       depends on ARCH_REALTEK
> > +       select ARM_GIC_V3
> > +       select ARM_PSCI
> 
> As I understand, this chip uses a Cortex-A55. What is the reason for adding
> support only to the 32-bit ARM architecture rather than 64-bit?

The RTD16XX platform also support the 64-bit ARM architecture.
I will add the 64-bit ARM architecture in new version patch.

> Most 64-bit SoCs are only supported with arch/arm64, but generally speaking
> that is not a requirement. My rule of thumb is that on systems with 1GB of
> RAM or more, one would want to run a 64-bit kernel, while systems with less
> than that are better off with a 32-bit one, but that is clearly not the only reason
> for picking one over the other.
> 
Support 32-bit ARM architecture is for application compatibility.

> > +
> > +static int rtk_boot_secondary(unsigned int cpu, struct task_struct
> > +*idle) {
> > +       unsigned long entry_pa = __pa_symbol(secondary_startup);
> > +
> > +       writel_relaxed(entry_pa | (cpu << CPUID), cpu_release_virt);
> > +
> > +       arch_send_wakeup_ipi_mask(cpumask_of(cpu));
> > +
> > +       return 0;
> > +}
> 
> It's very unusual to see custom smp operations on an ARMv8 system, as we
> normally use PSCI here. Can you explain what is going on here? Are you able to
> use a boot wrapper that implements these in psci instead?
>
The smp operations is porting form other Realtek platform.

Currently, The RTD16XX platform can use the PSCI method.
I will add PSCI method in new version patch.

> > +
> > +#include "platsmp.h"
> > +
> > +#define RBUS_BASE_PHYS (0x98000000)
> > +#define RBUS_BASE_VIRT (0xfe000000)
> > +#define RBUS_BASE_SIZE (0x00100000)
> > +
> > +static struct map_desc rtk_io_desc[] __initdata = {
> > +       {
> > +               .virtual = (unsigned long) IOMEM(RBUS_BASE_VIRT),
> > +               .pfn = __phys_to_pfn(RBUS_BASE_PHYS),
> > +               .length = RBUS_BASE_SIZE,
> > +               .type = MT_DEVICE,
> > +       },
> > +};
> 
> This needs a comment: Why do you require a static mapping for
> "RBUS_BASE_PHYS"? Normally device drivers should just use
> ioremap() for mapping whichever registers they want to access.
> 
The static mapping is for old Realtek devices driver.
I will 'use ioremap()' to replace with static mapping.

> > +static void __init rtk_dt_init(void)
> > +{
> > +       of_platform_populate(NULL, of_default_bus_match_table, NULL,
> > +NULL); }
> 
> This should be taken care of by the
> of_platform_default_populate_init() and can be dropped.
> 
I will remove rtk_dt_init() in new version patch.

> > +static void __init rtk_timer_init(void) { #ifdef CONFIG_COMMON_CLK
> > +       of_clk_init(NULL);
> > +#endif
> 
> COMMON_CLK is implied by ARCH_MULTI_V7, so the #ifdef can be dropped.
> 
OK, I understand.

> > +       timer_probe();
> > +       tick_setup_hrtimer_broadcast(); }
> 
> What do you need tick_setup_hrtimer_broadcast() for? I don't see any other
> platform calling this.
> 
I want to initialize the HR timer.

> > +bool __init rtk_smp_init_ops(void)
> > +{
> > +       smp_set_ops(smp_ops(rtk_smp_ops));
> > +
> > +       return true;
> > +}
> 
> I think this can also be dropped, as you set the smp_ops in the machine
> descriptor.
> 
OK, I understand.

>        Arnd
> 
> ------Please consider the environment before printing this e-mail.

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

[Arnd Bergmann]

On Wed, Sep 11, 2019 at 9:46 AM James Tai[戴志峰] <james.tai@realtek.com> wrote:
> > Subject: Re: [PATCH] ARM: Add support for Realtek SOC

> > > @@ -148,6 +148,7 @@ endif
> > >  textofs-$(CONFIG_ARCH_MSM8X60) := 0x00208000
> > >  textofs-$(CONFIG_ARCH_MSM8960) := 0x00208000
> > >  textofs-$(CONFIG_ARCH_MESON) := 0x00208000
> > > +textofs-$(CONFIG_ARCH_REALTEK) := 0x00208000
> > >  textofs-$(CONFIG_ARCH_AXXIA) := 0x00308000
> >
> > Can you explain why this is needed for your platform?
> >
> We need to reserve memory (0x00000000 ~ 0x001B0000) for rom and boot code.

Ok.

> > > +config ARCH_RTD16XX
> > > +       bool "Enable support for RTD1619"
> > > +       depends on ARCH_REALTEK
> > > +       select ARM_GIC_V3
> > > +       select ARM_PSCI
> >
> > As I understand, this chip uses a Cortex-A55. What is the reason for adding
> > support only to the 32-bit ARM architecture rather than 64-bit?
>
> The RTD16XX platform also support the 64-bit ARM architecture.
> I will add the 64-bit ARM architecture in new version patch.
>
> > Most 64-bit SoCs are only supported with arch/arm64, but generally speaking
> > that is not a requirement. My rule of thumb is that on systems with 1GB of
> > RAM or more, one would want to run a 64-bit kernel, while systems with less
> > than that are better off with a 32-bit one, but that is clearly not the only reason
> > for picking one over the other.
> >
> Support 32-bit ARM architecture is for application compatibility.

Generally speaking, a 64-bit kernel should work better on 64-bit hardware
even when you are running mostly 32-bit applications. However, you may
have device drivers that do not correctly set compat_ioctl handlers.

As I said, it's no problem to allow both, just explain this in the
changelog text for the driver, along with the need for the textofs
setting.

> > It's very unusual to see custom smp operations on an ARMv8 system, as we
> > normally use PSCI here. Can you explain what is going on here? Are you able to
> > use a boot wrapper that implements these in psci instead?
> >
> The smp operations is porting form other Realtek platform.
>
> Currently, The RTD16XX platform can use the PSCI method.
> I will add PSCI method in new version patch.

Ok, perfect!

> > > +       timer_probe();
> > > +       tick_setup_hrtimer_broadcast(); }
> >
> > What do you need tick_setup_hrtimer_broadcast() for? I don't see any other
> > platform calling this.
> >
> I want to initialize the HR timer.

I'm still unsure about this one. My feeling is that it should not be
in the platform
code, but I don't quite understand which hardware needs it. I see that
Lorenzo Pieralisi added the same code to arm64 in commit 9358d755bd5c
("arm64: kernel: initialize broadcast hrtimer based clock event device"),
but nothing ever calls it on 32-bit arch/arm even though the code does
get built in to the kernel.

My feeling is that either you don't really need it, or this is something
that other platforms should really do as well, and it should be called from
the generic time_init() function in arch/arm/kernel/time.c instead.

Can you try to find out more of the background here, and then
move the call to time_init() assuming it is indeed useful?

       Arnd

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

[Masahiro Yamada]

On Wed, Sep 11, 2019 at 5:17 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Wed, Sep 11, 2019 at 9:46 AM James Tai[戴志峰] <james.tai@realtek.com> wrote:
> > > Subject: Re: [PATCH] ARM: Add support for Realtek SOC
>
> > > > @@ -148,6 +148,7 @@ endif
> > > >  textofs-$(CONFIG_ARCH_MSM8X60) := 0x00208000
> > > >  textofs-$(CONFIG_ARCH_MSM8960) := 0x00208000
> > > >  textofs-$(CONFIG_ARCH_MESON) := 0x00208000
> > > > +textofs-$(CONFIG_ARCH_REALTEK) := 0x00208000
> > > >  textofs-$(CONFIG_ARCH_AXXIA) := 0x00308000
> > >
> > > Can you explain why this is needed for your platform?
> > >
> > We need to reserve memory (0x00000000 ~ 0x001B0000) for rom and boot code.
>
> Ok.


I do not like this much.

This platform is ARCH_MULTI_V7.

ARM_PATCH_PHYS_VIRT allows you to place the kernel image
anywhere in memory as long as the base is aligned at 16MB.

The minimum 'textofs-y := 0x0008000' + extra 16MB offset
will create a space (0x00000000 ~ 0x01008000).

This is more than needed, but it is not a big deal
to waste some megabytes of memory.


-- 
Best Regards
Masahiro Yamada

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

[James Tai]

> Subject: Re: [PATCH] ARM: Add support for Realtek SOC
> 
> On Wed, Sep 11, 2019 at 5:17 PM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Wed, Sep 11, 2019 at 9:46 AM James Tai[戴志峰]
> <james.tai@realtek.com> wrote:
> > > > Subject: Re: [PATCH] ARM: Add support for Realtek SOC
> >
> > > > > @@ -148,6 +148,7 @@ endif
> > > > >  textofs-$(CONFIG_ARCH_MSM8X60) := 0x00208000
> > > > >  textofs-$(CONFIG_ARCH_MSM8960) := 0x00208000
> > > > >  textofs-$(CONFIG_ARCH_MESON) := 0x00208000
> > > > > +textofs-$(CONFIG_ARCH_REALTEK) := 0x00208000
> > > > >  textofs-$(CONFIG_ARCH_AXXIA) := 0x00308000
> > > >
> > > > Can you explain why this is needed for your platform?
> > > >
> > > We need to reserve memory (0x00000000 ~ 0x001B0000) for rom and boot
> code.
> >
> > Ok.
> 
> 
> I do not like this much.
> 
> This platform is ARCH_MULTI_V7.
> 
> ARM_PATCH_PHYS_VIRT allows you to place the kernel image anywhere in
> memory as long as the base is aligned at 16MB.
> 
> The minimum 'textofs-y := 0x0008000' + extra 16MB offset will create a space
> (0x00000000 ~ 0x01008000).
> 
> This is more than needed, but it is not a big deal to waste some megabytes of
> memory.
> 

OK. I understand.

> --
> Best Regards
> Masahiro Yamada
> 
> ------Please consider the environment before printing this e-mail.

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

[James Tai]

> <james.tai@realtek.com> wrote:
> > > Subject: Re: [PATCH] ARM: Add support for Realtek SOC
> 
> > > > @@ -148,6 +148,7 @@ endif
> > > >  textofs-$(CONFIG_ARCH_MSM8X60) := 0x00208000
> > > >  textofs-$(CONFIG_ARCH_MSM8960) := 0x00208000
> > > >  textofs-$(CONFIG_ARCH_MESON) := 0x00208000
> > > > +textofs-$(CONFIG_ARCH_REALTEK) := 0x00208000
> > > >  textofs-$(CONFIG_ARCH_AXXIA) := 0x00308000
> > >
> > > Can you explain why this is needed for your platform?
> > >
> > We need to reserve memory (0x00000000 ~ 0x001B0000) for rom and boot
> code.
> 
> Ok.
> 
> > > > +config ARCH_RTD16XX
> > > > +       bool "Enable support for RTD1619"
> > > > +       depends on ARCH_REALTEK
> > > > +       select ARM_GIC_V3
> > > > +       select ARM_PSCI
> > >
> > > As I understand, this chip uses a Cortex-A55. What is the reason for
> > > adding support only to the 32-bit ARM architecture rather than 64-bit?
> >
> > The RTD16XX platform also support the 64-bit ARM architecture.
> > I will add the 64-bit ARM architecture in new version patch.
> >
> > > Most 64-bit SoCs are only supported with arch/arm64, but generally
> > > speaking that is not a requirement. My rule of thumb is that on
> > > systems with 1GB of RAM or more, one would want to run a 64-bit
> > > kernel, while systems with less than that are better off with a
> > > 32-bit one, but that is clearly not the only reason for picking one over the
> other.
> > >
> > Support 32-bit ARM architecture is for application compatibility.
> 
> Generally speaking, a 64-bit kernel should work better on 64-bit hardware
> even when you are running mostly 32-bit applications. However, you may have
> device drivers that do not correctly set compat_ioctl handlers.
> 
> As I said, it's no problem to allow both, just explain this in the changelog text
> for the driver, along with the need for the textofs setting.
> 
OK.

> > > It's very unusual to see custom smp operations on an ARMv8 system,
> > > as we normally use PSCI here. Can you explain what is going on here?
> > > Are you able to use a boot wrapper that implements these in psci instead?
> > >
> > The smp operations is porting form other Realtek platform.
> >
> > Currently, The RTD16XX platform can use the PSCI method.
> > I will add PSCI method in new version patch.
> 
> Ok, perfect!
> 
> > > > +       timer_probe();
> > > > +       tick_setup_hrtimer_broadcast(); }
> > >
> > > What do you need tick_setup_hrtimer_broadcast() for? I don't see any
> > > other platform calling this.
> > >
> > I want to initialize the HR timer.
> 
> I'm still unsure about this one. My feeling is that it should not be in the
> platform code, but I don't quite understand which hardware needs it. I see that
> Lorenzo Pieralisi added the same code to arm64 in commit 9358d755bd5c
> ("arm64: kernel: initialize broadcast hrtimer based clock event device"), but
> nothing ever calls it on 32-bit arch/arm even though the code does get built in
> to the kernel.

I will add the 'hrtimer' initialization flow in related devices drivers.

> My feeling is that either you don't really need it, or this is something that other
> platforms should really do as well, and it should be called from the generic
> time_init() function in arch/arm/kernel/time.c instead.
> 
OK. I understand.

> Can you try to find out more of the background here, and then move the call to
> time_init() assuming it is indeed useful?

I agree with you. It is not necessary to call 'time_init()' function in platform code.

>        Arnd
> 
> ------Please consider the environment before printing this e-mail.