Re: [PATCH v3 1/7] ARM: sun8i: smp: Add support for A83T
Hello Chen-Yu, On Tue, 20 Feb 2018 11:32:03 +0800 Chen-Yu Tsai wrote: > On Mon, Feb 19, 2018 at 4:18 PM, Mylène Josserand > wrote: > > Add the support for A83T. > > > > A83T SoC has an additional register than A80 to handle CPU configurations: > > R_CPUS_CFG. Information about the register comes from Allwinner's BSP > > driver. > > An important difference is the Power Off Gating register for clusters > > which is BIT(4) in case of SUN9I-A80 and BIT(0) in case of SUN8I-A83T. > > > > Signed-off-by: Mylène Josserand > > --- > > arch/arm/mach-sunxi/Kconfig | 2 +- > > arch/arm/mach-sunxi/mc_smp.c | 239 > > +++ > > The same high-level comments as Maxime. Splitting the patch > will make this much easier to understand. Yep, I will do it in next version. > > > 2 files changed, 198 insertions(+), 43 deletions(-) > > > > diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig > > index ce53ceaf4cc5..a0ad35c41c02 100644 > > --- a/arch/arm/mach-sunxi/Kconfig > > +++ b/arch/arm/mach-sunxi/Kconfig > > @@ -51,7 +51,7 @@ config MACH_SUN9I > > config ARCH_SUNXI_MC_SMP > > bool > > depends on SMP > > - default MACH_SUN9I > > + default y if MACH_SUN9I || MACH_SUN8I > > select ARM_CCI400_PORT_CTRL > > select ARM_CPU_SUSPEND > > > > diff --git a/arch/arm/mach-sunxi/mc_smp.c b/arch/arm/mach-sunxi/mc_smp.c > > index 11e46c6efb90..fec592bf68b4 100644 > > --- a/arch/arm/mach-sunxi/mc_smp.c > > +++ b/arch/arm/mach-sunxi/mc_smp.c > > @@ -55,20 +55,29 @@ > > #define CPUCFG_CX_RST_CTRL_L2_RST BIT(8) > > #define CPUCFG_CX_RST_CTRL_CX_RST(n) BIT(4 + (n)) > > #define CPUCFG_CX_RST_CTRL_CORE_RST(n) BIT(n) > > +#define CPUCFG_CX_RST_CTRL_CORE_RST_ALL(0xf << 0) > > > > #define PRCM_CPU_PO_RST_CTRL(c)(0x4 + 0x4 * (c)) > > #define PRCM_CPU_PO_RST_CTRL_CORE(n) BIT(n) > > #define PRCM_CPU_PO_RST_CTRL_CORE_ALL 0xf > > #define PRCM_PWROFF_GATING_REG(c) (0x100 + 0x4 * (c)) > > -#define PRCM_PWROFF_GATING_REG_CLUSTER BIT(4) > > +/* The power off register for clusters are different from SUN9I and SUN8I > > */ > > +#define PRCM_PWROFF_GATING_REG_CLUSTER_SUN8I BIT(0) > > +#define PRCM_PWROFF_GATING_REG_CLUSTER_SUN9I BIT(4) > > #define PRCM_PWROFF_GATING_REG_CORE(n) BIT(n) > > #define PRCM_PWR_SWITCH_REG(c, cpu)(0x140 + 0x10 * (c) + 0x4 * (cpu)) > > #define PRCM_CPU_SOFT_ENTRY_REG0x164 > > > > +/* R_CPUCFG registers, specific to SUN8I */ > > +#define R_CPUCFG_CLUSTER_PO_RST_CTRL(c)(0x30 + (c) * 0x4) > > +#define R_CPUCFG_CLUSTER_PO_RST_CTRL_CORE(n) BIT(n) > > +#define R_CPUCFG_CPU_SOFT_ENTRY_REG0x01a4 > > + > > #define CPU0_SUPPORT_HOTPLUG_MAGIC00xFA50392F > > #define CPU0_SUPPORT_HOTPLUG_MAGIC10x790DCA3A > > > > static void __iomem *cpucfg_base; > > +static void __iomem *r_cpucfg_base; > > static void __iomem *prcm_base; > > static void __iomem *sram_b_smp_base; > > > > @@ -157,6 +166,16 @@ static int sunxi_cpu_powerup(unsigned int cpu, > > unsigned int cluster) > > reg &= ~PRCM_CPU_PO_RST_CTRL_CORE(cpu); > > writel(reg, prcm_base + PRCM_CPU_PO_RST_CTRL(cluster)); > > > > + if (r_cpucfg_base) { > > + /* assert cpu power-on reset */ > > + reg = readl(r_cpucfg_base + > > +R_CPUCFG_CLUSTER_PO_RST_CTRL(cluster)); > > + reg &= ~(R_CPUCFG_CLUSTER_PO_RST_CTRL_CORE(cpu)); > > + writel(reg, r_cpucfg_base + > > + R_CPUCFG_CLUSTER_PO_RST_CTRL(cluster)); > > + udelay(10); > > + } > > + > > /* Cortex-A7: hold L1 reset disable signal low */ > > if (!sunxi_core_is_cortex_a15(cpu, cluster)) { > > reg = readl(cpucfg_base + CPUCFG_CX_CTRL_REG0(cluster)); > > @@ -180,17 +199,37 @@ static int sunxi_cpu_powerup(unsigned int cpu, > > unsigned int cluster) > > /* open power switch */ > > sunxi_cpu_power_switch_set(cpu, cluster, true); > > > > + /* Handle A83T bit swap */ > > + if (of_machine_is_compatible("allwinner,sun8i-a83t")) { > > + if (cpu == 0) > > + cpu = 4; > > + } > > + > > /* clear processor power gate */ > > reg = readl(prcm_base + PRCM_PWROFF_GATING_REG(cluster)); > > reg &= ~PRCM_PWROFF_GATING_REG_CORE(cpu); > > writel(reg, prcm_base + PRCM_PWROFF_GATING_REG(cluster)); > > udelay(20); > > > > + if (of_machine_is_compatible("allwinner,sun8i-a83t")) { > > + if (cpu == 4) > > + cpu = 0; > > + } > > + > > /* de-assert processor power-on reset */ > > reg = readl(prcm_base + PRCM_CPU_PO_RST_CTRL(cluster)); > > reg |= PRCM_CPU_PO_RST_CTRL_CORE(cpu); > > writel(reg, prcm_base + PRCM_CPU_PO_RST_CTRL(cluster)); > > > > + if (r_cpucfg_base) { > > + reg = readl(r_cpucfg_ba
Re: [PATCH v3 1/7] ARM: sun8i: smp: Add support for A83T
On Mon, Feb 19, 2018 at 4:18 PM, Mylène Josserand wrote: > Add the support for A83T. > > A83T SoC has an additional register than A80 to handle CPU configurations: > R_CPUS_CFG. Information about the register comes from Allwinner's BSP > driver. > An important difference is the Power Off Gating register for clusters > which is BIT(4) in case of SUN9I-A80 and BIT(0) in case of SUN8I-A83T. > > Signed-off-by: Mylène Josserand > --- > arch/arm/mach-sunxi/Kconfig | 2 +- > arch/arm/mach-sunxi/mc_smp.c | 239 > +++ The same high-level comments as Maxime. Splitting the patch will make this much easier to understand. > 2 files changed, 198 insertions(+), 43 deletions(-) > > diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig > index ce53ceaf4cc5..a0ad35c41c02 100644 > --- a/arch/arm/mach-sunxi/Kconfig > +++ b/arch/arm/mach-sunxi/Kconfig > @@ -51,7 +51,7 @@ config MACH_SUN9I > config ARCH_SUNXI_MC_SMP > bool > depends on SMP > - default MACH_SUN9I > + default y if MACH_SUN9I || MACH_SUN8I > select ARM_CCI400_PORT_CTRL > select ARM_CPU_SUSPEND > > diff --git a/arch/arm/mach-sunxi/mc_smp.c b/arch/arm/mach-sunxi/mc_smp.c > index 11e46c6efb90..fec592bf68b4 100644 > --- a/arch/arm/mach-sunxi/mc_smp.c > +++ b/arch/arm/mach-sunxi/mc_smp.c > @@ -55,20 +55,29 @@ > #define CPUCFG_CX_RST_CTRL_L2_RST BIT(8) > #define CPUCFG_CX_RST_CTRL_CX_RST(n) BIT(4 + (n)) > #define CPUCFG_CX_RST_CTRL_CORE_RST(n) BIT(n) > +#define CPUCFG_CX_RST_CTRL_CORE_RST_ALL(0xf << 0) > > #define PRCM_CPU_PO_RST_CTRL(c)(0x4 + 0x4 * (c)) > #define PRCM_CPU_PO_RST_CTRL_CORE(n) BIT(n) > #define PRCM_CPU_PO_RST_CTRL_CORE_ALL 0xf > #define PRCM_PWROFF_GATING_REG(c) (0x100 + 0x4 * (c)) > -#define PRCM_PWROFF_GATING_REG_CLUSTER BIT(4) > +/* The power off register for clusters are different from SUN9I and SUN8I */ > +#define PRCM_PWROFF_GATING_REG_CLUSTER_SUN8I BIT(0) > +#define PRCM_PWROFF_GATING_REG_CLUSTER_SUN9I BIT(4) > #define PRCM_PWROFF_GATING_REG_CORE(n) BIT(n) > #define PRCM_PWR_SWITCH_REG(c, cpu)(0x140 + 0x10 * (c) + 0x4 * (cpu)) > #define PRCM_CPU_SOFT_ENTRY_REG0x164 > > +/* R_CPUCFG registers, specific to SUN8I */ > +#define R_CPUCFG_CLUSTER_PO_RST_CTRL(c)(0x30 + (c) * 0x4) > +#define R_CPUCFG_CLUSTER_PO_RST_CTRL_CORE(n) BIT(n) > +#define R_CPUCFG_CPU_SOFT_ENTRY_REG0x01a4 > + > #define CPU0_SUPPORT_HOTPLUG_MAGIC00xFA50392F > #define CPU0_SUPPORT_HOTPLUG_MAGIC10x790DCA3A > > static void __iomem *cpucfg_base; > +static void __iomem *r_cpucfg_base; > static void __iomem *prcm_base; > static void __iomem *sram_b_smp_base; > > @@ -157,6 +166,16 @@ static int sunxi_cpu_powerup(unsigned int cpu, unsigned > int cluster) > reg &= ~PRCM_CPU_PO_RST_CTRL_CORE(cpu); > writel(reg, prcm_base + PRCM_CPU_PO_RST_CTRL(cluster)); > > + if (r_cpucfg_base) { > + /* assert cpu power-on reset */ > + reg = readl(r_cpucfg_base + > +R_CPUCFG_CLUSTER_PO_RST_CTRL(cluster)); > + reg &= ~(R_CPUCFG_CLUSTER_PO_RST_CTRL_CORE(cpu)); > + writel(reg, r_cpucfg_base + > + R_CPUCFG_CLUSTER_PO_RST_CTRL(cluster)); > + udelay(10); > + } > + > /* Cortex-A7: hold L1 reset disable signal low */ > if (!sunxi_core_is_cortex_a15(cpu, cluster)) { > reg = readl(cpucfg_base + CPUCFG_CX_CTRL_REG0(cluster)); > @@ -180,17 +199,37 @@ static int sunxi_cpu_powerup(unsigned int cpu, unsigned > int cluster) > /* open power switch */ > sunxi_cpu_power_switch_set(cpu, cluster, true); > > + /* Handle A83T bit swap */ > + if (of_machine_is_compatible("allwinner,sun8i-a83t")) { > + if (cpu == 0) > + cpu = 4; > + } > + > /* clear processor power gate */ > reg = readl(prcm_base + PRCM_PWROFF_GATING_REG(cluster)); > reg &= ~PRCM_PWROFF_GATING_REG_CORE(cpu); > writel(reg, prcm_base + PRCM_PWROFF_GATING_REG(cluster)); > udelay(20); > > + if (of_machine_is_compatible("allwinner,sun8i-a83t")) { > + if (cpu == 4) > + cpu = 0; > + } > + > /* de-assert processor power-on reset */ > reg = readl(prcm_base + PRCM_CPU_PO_RST_CTRL(cluster)); > reg |= PRCM_CPU_PO_RST_CTRL_CORE(cpu); > writel(reg, prcm_base + PRCM_CPU_PO_RST_CTRL(cluster)); > > + if (r_cpucfg_base) { > + reg = readl(r_cpucfg_base + > +R_CPUCFG_CLUSTER_PO_RST_CTRL(cluster)); > + reg |= R_CPUCFG_CLUSTER_PO_RST_CTRL_CORE(cpu); > + writel(reg, r_cpucfg_base + > + R_CPUCFG_CLUSTER_PO_RST_CTRL(cluster)); > + udelay(10); > + } > + > /* de-assert all processor resets */ >
Re: [PATCH v3 1/7] ARM: sun8i: smp: Add support for A83T
Hello Maxime, On Mon, 19 Feb 2018 10:04:01 +0100 Maxime Ripard wrote: > Hi, > > On Mon, Feb 19, 2018 at 09:18:31AM +0100, Mylène Josserand wrote: > > Add the support for A83T. > > > > A83T SoC has an additional register than A80 to handle CPU configurations: > > R_CPUS_CFG. Information about the register comes from Allwinner's BSP > > driver. > > An important difference is the Power Off Gating register for clusters > > which is BIT(4) in case of SUN9I-A80 and BIT(0) in case of SUN8I-A83T. > > > > Signed-off-by: Mylène Josserand > > This is some high-level review, but you should split this patch in > three: > - One to refactor the current code to split the DT parsing function > (and rename the function to sun9i_smp_dt_parse) and the variable renames > - One to enable the A83t SMP (with the function called > sun8i_a83t_smp_dt_parse) > - One to enable the A83t hotplug (merged with your last patch) Thank you for the review. Sure, I will split my patch in three. > > Also, you're calling a lot of times of_machine_is_compatible, and this > is quite inefficient. You should call it once and store the result. True. Thanks, Mylène -- Mylène Josserand, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering http://bootlin.com
Re: [PATCH v3 1/7] ARM: sun8i: smp: Add support for A83T
Hi, On Mon, Feb 19, 2018 at 09:18:31AM +0100, Mylène Josserand wrote: > Add the support for A83T. > > A83T SoC has an additional register than A80 to handle CPU configurations: > R_CPUS_CFG. Information about the register comes from Allwinner's BSP > driver. > An important difference is the Power Off Gating register for clusters > which is BIT(4) in case of SUN9I-A80 and BIT(0) in case of SUN8I-A83T. > > Signed-off-by: Mylène Josserand This is some high-level review, but you should split this patch in three: - One to refactor the current code to split the DT parsing function (and rename the function to sun9i_smp_dt_parse) and the variable renames - One to enable the A83t SMP (with the function called sun8i_a83t_smp_dt_parse) - One to enable the A83t hotplug (merged with your last patch) Also, you're calling a lot of times of_machine_is_compatible, and this is quite inefficient. You should call it once and store the result. Thanks! Maxime -- Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering http://bootlin.com signature.asc Description: PGP signature
[PATCH v3 1/7] ARM: sun8i: smp: Add support for A83T
Add the support for A83T. A83T SoC has an additional register than A80 to handle CPU configurations: R_CPUS_CFG. Information about the register comes from Allwinner's BSP driver. An important difference is the Power Off Gating register for clusters which is BIT(4) in case of SUN9I-A80 and BIT(0) in case of SUN8I-A83T. Signed-off-by: Mylène Josserand --- arch/arm/mach-sunxi/Kconfig | 2 +- arch/arm/mach-sunxi/mc_smp.c | 239 +++ 2 files changed, 198 insertions(+), 43 deletions(-) diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig index ce53ceaf4cc5..a0ad35c41c02 100644 --- a/arch/arm/mach-sunxi/Kconfig +++ b/arch/arm/mach-sunxi/Kconfig @@ -51,7 +51,7 @@ config MACH_SUN9I config ARCH_SUNXI_MC_SMP bool depends on SMP - default MACH_SUN9I + default y if MACH_SUN9I || MACH_SUN8I select ARM_CCI400_PORT_CTRL select ARM_CPU_SUSPEND diff --git a/arch/arm/mach-sunxi/mc_smp.c b/arch/arm/mach-sunxi/mc_smp.c index 11e46c6efb90..fec592bf68b4 100644 --- a/arch/arm/mach-sunxi/mc_smp.c +++ b/arch/arm/mach-sunxi/mc_smp.c @@ -55,20 +55,29 @@ #define CPUCFG_CX_RST_CTRL_L2_RST BIT(8) #define CPUCFG_CX_RST_CTRL_CX_RST(n) BIT(4 + (n)) #define CPUCFG_CX_RST_CTRL_CORE_RST(n) BIT(n) +#define CPUCFG_CX_RST_CTRL_CORE_RST_ALL(0xf << 0) #define PRCM_CPU_PO_RST_CTRL(c)(0x4 + 0x4 * (c)) #define PRCM_CPU_PO_RST_CTRL_CORE(n) BIT(n) #define PRCM_CPU_PO_RST_CTRL_CORE_ALL 0xf #define PRCM_PWROFF_GATING_REG(c) (0x100 + 0x4 * (c)) -#define PRCM_PWROFF_GATING_REG_CLUSTER BIT(4) +/* The power off register for clusters are different from SUN9I and SUN8I */ +#define PRCM_PWROFF_GATING_REG_CLUSTER_SUN8I BIT(0) +#define PRCM_PWROFF_GATING_REG_CLUSTER_SUN9I BIT(4) #define PRCM_PWROFF_GATING_REG_CORE(n) BIT(n) #define PRCM_PWR_SWITCH_REG(c, cpu)(0x140 + 0x10 * (c) + 0x4 * (cpu)) #define PRCM_CPU_SOFT_ENTRY_REG0x164 +/* R_CPUCFG registers, specific to SUN8I */ +#define R_CPUCFG_CLUSTER_PO_RST_CTRL(c)(0x30 + (c) * 0x4) +#define R_CPUCFG_CLUSTER_PO_RST_CTRL_CORE(n) BIT(n) +#define R_CPUCFG_CPU_SOFT_ENTRY_REG0x01a4 + #define CPU0_SUPPORT_HOTPLUG_MAGIC00xFA50392F #define CPU0_SUPPORT_HOTPLUG_MAGIC10x790DCA3A static void __iomem *cpucfg_base; +static void __iomem *r_cpucfg_base; static void __iomem *prcm_base; static void __iomem *sram_b_smp_base; @@ -157,6 +166,16 @@ static int sunxi_cpu_powerup(unsigned int cpu, unsigned int cluster) reg &= ~PRCM_CPU_PO_RST_CTRL_CORE(cpu); writel(reg, prcm_base + PRCM_CPU_PO_RST_CTRL(cluster)); + if (r_cpucfg_base) { + /* assert cpu power-on reset */ + reg = readl(r_cpucfg_base + +R_CPUCFG_CLUSTER_PO_RST_CTRL(cluster)); + reg &= ~(R_CPUCFG_CLUSTER_PO_RST_CTRL_CORE(cpu)); + writel(reg, r_cpucfg_base + + R_CPUCFG_CLUSTER_PO_RST_CTRL(cluster)); + udelay(10); + } + /* Cortex-A7: hold L1 reset disable signal low */ if (!sunxi_core_is_cortex_a15(cpu, cluster)) { reg = readl(cpucfg_base + CPUCFG_CX_CTRL_REG0(cluster)); @@ -180,17 +199,37 @@ static int sunxi_cpu_powerup(unsigned int cpu, unsigned int cluster) /* open power switch */ sunxi_cpu_power_switch_set(cpu, cluster, true); + /* Handle A83T bit swap */ + if (of_machine_is_compatible("allwinner,sun8i-a83t")) { + if (cpu == 0) + cpu = 4; + } + /* clear processor power gate */ reg = readl(prcm_base + PRCM_PWROFF_GATING_REG(cluster)); reg &= ~PRCM_PWROFF_GATING_REG_CORE(cpu); writel(reg, prcm_base + PRCM_PWROFF_GATING_REG(cluster)); udelay(20); + if (of_machine_is_compatible("allwinner,sun8i-a83t")) { + if (cpu == 4) + cpu = 0; + } + /* de-assert processor power-on reset */ reg = readl(prcm_base + PRCM_CPU_PO_RST_CTRL(cluster)); reg |= PRCM_CPU_PO_RST_CTRL_CORE(cpu); writel(reg, prcm_base + PRCM_CPU_PO_RST_CTRL(cluster)); + if (r_cpucfg_base) { + reg = readl(r_cpucfg_base + +R_CPUCFG_CLUSTER_PO_RST_CTRL(cluster)); + reg |= R_CPUCFG_CLUSTER_PO_RST_CTRL_CORE(cpu); + writel(reg, r_cpucfg_base + + R_CPUCFG_CLUSTER_PO_RST_CTRL(cluster)); + udelay(10); + } + /* de-assert all processor resets */ reg = readl(cpucfg_base + CPUCFG_CX_RST_CTRL(cluster)); reg |= CPUCFG_CX_RST_CTRL_DBG_RST(cpu); @@ -212,6 +251,14 @@ static int sunxi_cluster_powerup(unsigned int cluster) if (cluster >= SUNXI_NR_CLUSTERS) return -EINVAL; + /* For A83T, assert cluster cores resets */ + if (of_machine_is_compatible("allwinner,sun8i-a83t")) { +