On Fri, Feb 23, 2018 at 9:37 PM, Mylène Josserand
<[email protected]> wrote:
> On Cortex-A7, the CNTVOFF register from arch timer is uninitialized.
> It should be done by the bootloader but it is currently not the case,
> even for boot CPU because this SoC is booting in secure mode.
> It leads to an random offset value meaning that each CPU will have a
> different time, which isn't working very well.
>
> Add assembly code used for boot CPU and secondary CPU cores to make
> sure that the CNTVOFF register is initialized.
>
> Signed-off-by: Mylène Josserand <[email protected]>
> ---
> arch/arm/mach-sunxi/headsmp.S | 21 +++++++++++++++++++++
> arch/arm/mach-sunxi/sunxi.c | 4 ++++
> 2 files changed, 25 insertions(+)
>
> diff --git a/arch/arm/mach-sunxi/headsmp.S b/arch/arm/mach-sunxi/headsmp.S
> index d5c97e945e69..605896251927 100644
> --- a/arch/arm/mach-sunxi/headsmp.S
> +++ b/arch/arm/mach-sunxi/headsmp.S
> @@ -65,9 +65,30 @@ ENTRY(sunxi_mc_smp_cluster_cache_enable)
> first: .word sunxi_mc_smp_first_comer - .
> ENDPROC(sunxi_mc_smp_cluster_cache_enable)
>
> +ENTRY(sunxi_init_cntvoff)
> + /*
> + * CNTVOFF has to be initialized either from non-secure Hypervisor
> + * mode or secure Monitor mode with SCR.NS==1. If TrustZone is enabled
> + * then it should be handled by the secure code
> + */
> + cps #MON_MODE
> + mrc p15, 0, r1, c1, c1, 0 /* Get Secure Config */
> + orr r0, r1, #1
> + mcr p15, 0, r0, c1, c1, 0 /* Set Non Secure bit */
> + instr_sync
> + mov r0, #0
> + mcrr p15, 4, r0, r0, c14 /* CNTVOFF = 0 */
> + instr_sync
> + mcr p15, 0, r1, c1, c1, 0 /* Set Secure bit */
> + instr_sync
> + cps #SVC_MODE
> + ret lr
> +ENDPROC(sunxi_init_cntvoff)

There is no need to move all the assembly into a separate file, just
to add this function. Everything can be inlined as a naked function.
The "instr_sync" macro can be replaced with "isb", which is what it
expands to anyway.

I really want to keep everything self-contained without global symbols,
and in C files if possible.

> +
> #ifdef CONFIG_SMP
> ENTRY(sunxi_boot)
> bl sunxi_mc_smp_cluster_cache_enable
> + bl sunxi_init_cntvoff
> b secondary_startup
> ENDPROC(sunxi_boot)
>
> diff --git a/arch/arm/mach-sunxi/sunxi.c b/arch/arm/mach-sunxi/sunxi.c
> index 5e9602ce1573..4bb041492b54 100644
> --- a/arch/arm/mach-sunxi/sunxi.c
> +++ b/arch/arm/mach-sunxi/sunxi.c
> @@ -37,8 +37,12 @@ static const char * const sun6i_board_dt_compat[] = {
> };
>
> extern void __init sun6i_reset_init(void);
> +extern void sunxi_init_cntvoff(void);
> +
> static void __init sun6i_timer_init(void)
> {
> + sunxi_init_cntvoff();

You should check the enable-method to see if PSCI is set or not,
as an indicator whether the kernel is booted secure or non-secure.
AFAIK trying to set CNTVOFF under non-secure would be very bad.

Regards
ChenYu

> +
> of_clk_init(NULL);
> if (IS_ENABLED(CONFIG_RESET_CONTROLLER))
> sun6i_reset_init();
> --
> 2.11.0
>

2018-02-25 15:21:25

by Mylène Josserand

[permalink] [raw]

Subject: Re: [PATCH v4 01/10] ARM: sun9i: smp: Add sun9i dt parsing function

Hi,

On Fri, 23 Feb 2018 15:54:23 +0100
Maxime Ripard <[email protected]> wrote:

> Hi,
>
> On Fri, Feb 23, 2018 at 02:37:33PM +0100, Mylène Josserand wrote:
> > To prepare the support for sun8i-a83t, create a new function
> > that handles all the resources needed on sun9i-a80 (because
> > it will be different from sun8i-a83t).
> >
> > All the parsing of device tree is moved into this new function:
> > sun9i_dt_parsing. Create also a function to release the resources
> > retrieved during the dt parsing in case there is an error in init
> > function.
> >
> > Signed-off-by: Mylène Josserand <[email protected]>
> > ---
> > arch/arm/mach-sunxi/mc_smp.c | 99 ++++++++++++++++++++++++++------------------
> > 1 file changed, 58 insertions(+), 41 deletions(-)
> >
> > diff --git a/arch/arm/mach-sunxi/mc_smp.c b/arch/arm/mach-sunxi/mc_smp.c
> > index 11e46c6efb90..650a2ad4398f 100644
> > --- a/arch/arm/mach-sunxi/mc_smp.c
> > +++ b/arch/arm/mach-sunxi/mc_smp.c
> > @@ -684,35 +684,22 @@ static int __init sunxi_mc_smp_lookback(void)
> > return ret;
> > }
> >
> > -static int __init sunxi_mc_smp_init(void)
> > +static int sun9i_dt_parsing(struct resource res)
>
> Like I told you in the previous version, this should be _parse_dt, and
> not _dt_parsing.

Sorry, I forgot this review.

>
> Also, I'm not sure why you are passing by copy the resource structure?
>
> > {
> > - struct device_node *cpucfg_node, *sram_node, *node;
> > - struct resource res;
> > + struct device_node *prcm_node, *cpucfg_node, *sram_node;
> > int ret;
> >
> > - if (!of_machine_is_compatible("allwinner,sun9i-a80"))
> > - return -ENODEV;
> > -
> > - if (!sunxi_mc_smp_cpu_table_init())
> > - return -EINVAL;
> > -
> > - if (!cci_probed()) {
> > - pr_err("%s: CCI-400 not available\n", __func__);
> > - return -ENODEV;
> > - }
> > -
> > - node = of_find_compatible_node(NULL, NULL, "allwinner,sun9i-a80-prcm");
> > - if (!node) {
> > - pr_err("%s: PRCM not available\n", __func__);
> > + prcm_node = of_find_compatible_node(NULL, NULL,
> > + "allwinner,sun9i-a80-prcm");
> > + if (!prcm_node)
> > return -ENODEV;
> > - }
> >
> > /*
> > * Unfortunately we can not request the I/O region for the PRCM.
> > * It is shared with the PRCM clock.
> > */
> > - prcm_base = of_iomap(node, 0);
> > - of_node_put(node);
> > + prcm_base = of_iomap(prcm_node, 0);
>
> So it does more a bit more than just parsing the DT?
>
> Can you maybe just fill the DT nodes and have the common part map the
> memory regions if the pointer is not NULL?

Yep, I will rework this function to only parse device tree nodes.

Thanks,

Mylène

--
Mylène Josserand, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

2018-02-25 15:21:42

by Mylène Josserand

[permalink] [raw]

Subject: Re: [PATCH v4 02/10] ARM: sun9i: smp: Rename clusters's power-off register

Hi,

On Fri, 23 Feb 2018 15:56:50 +0100
Maxime Ripard <[email protected]> wrote:

> On Fri, Feb 23, 2018 at 02:37:34PM +0100, Mylène Josserand wrote:
> > To prepare the support for sun8i-a83t, rename the variable name
> > that handles the power-off of clusters because it is different from
> > sun9i-a80 to sun8i-a83t.
> >
> > The power off register for clusters are different from SUN9I and SUN8I
>
> This is just a nitpick, and I see you use them everywhere, but sun8i
> and sun9i are the family of SoCs, and A80 and A83t the name of SoCs
> within their respective families.
>
> So in your commit log, you want to support the A83t because it's
> different than the A80, and the sun9i and sun8i mentionned in your
> last sentence is not really meaningful.

True, I will pay attention next time.

>
> > Signed-off-by: Mylène Josserand <[email protected]>
>
> Acked-by: Maxime Ripard <[email protected]>
>
> Maxime
>

Thanks,

Mylène

--
Mylène Josserand, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

2018-02-25 15:28:00

by Mylène Josserand

[permalink] [raw]

Subject: Re: [PATCH v4 03/10] ARM: sun8i: smp: Add support for A83T

Hi,

On Fri, 23 Feb 2018 16:03:05 +0100
Maxime Ripard <[email protected]> wrote:

> On Fri, Feb 23, 2018 at 02:37:35PM +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 <[email protected]>
> > ---
> > arch/arm/mach-sunxi/Kconfig | 2 +-
> > arch/arm/mach-sunxi/mc_smp.c | 168 +++++++++++++++++++++++++++++++++++++++++--
> > 2 files changed, 162 insertions(+), 8 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 de02e5662557..3bd9066a1422 100644
> > --- a/arch/arm/mach-sunxi/mc_smp.c
> > +++ b/arch/arm/mach-sunxi/mc_smp.c
> > @@ -55,22 +55,32 @@
> > #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))
> > +/* 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_REG 0x164
> >
> > +/* 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_REG 0x01a4
> > +
> > #define CPU0_SUPPORT_HOTPLUG_MAGIC0 0xFA50392F
> > #define CPU0_SUPPORT_HOTPLUG_MAGIC1 0x790DCA3A
> >
> > static void __iomem *cpucfg_base;
> > +static void __iomem *r_cpucfg_base;
> > static void __iomem *prcm_base;
> > static void __iomem *sram_b_smp_base;
> > +static bool is_sun9i;
>
> Since you always check for that condition to always be false, can't
> you do the opposite, ie have it called is_a83t, and verify it to be
> true?

Yes, I will switch the test.

>
> > /* Set the hardware entry point address */
> > - writel(__pa_symbol(sunxi_mc_smp_secondary_startup),
> > - prcm_base + PRCM_CPU_SOFT_ENTRY_REG);
> > + if (is_sun9i)
> > + writel(__pa_symbol(sunxi_mc_smp_secondary_startup),
> > + prcm_base + PRCM_CPU_SOFT_ENTRY_REG);
> > + else
> > + writel(__pa_symbol(sunxi_mc_smp_secondary_startup),
> > + r_cpucfg_base + R_CPUCFG_CPU_SOFT_ENTRY_REG);
>
> if (is_a83t)
> reg = prcm_base + PRCM_CPU_SOFT_ENTRY_REG;
> else
> reg = r_cpucfg_base + R_CPUCFG_CPU_SOFT_ENTRY_REG;
> writel(__pa_symbol(sunxi_mc_smp_secondary_startup), reg);
>

Make sense, thanks for the review.

Mylene

--
Mylène Josserand, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

2018-02-26 06:23:54

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v4 08/10] ARM: sunxi: smp: Move assembly code into a file

Hi Myl?ne,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on next-20180223]
[cannot apply to arm-soc/for-next robh/for-next linux-rpi/for-rpi-next v4.16-rc2 v4.16-rc1 v4.15 v4.16-rc3]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Myl-ne-Josserand/Sunxi-Add-SMP-support-on-A83T/20180226-035312
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm

All errors (new ones prefixed by >>):

arch/arm/mach-sunxi/headsmp.S: Assembler messages:
>> arch/arm/mach-sunxi/headsmp.S:22: Error: selected processor does not support `movw r2,#(0xff00fff0&0xffff)' in ARM mode
>> arch/arm/mach-sunxi/headsmp.S:23: Error: selected processor does not support `movt r2,#(0xff00fff0>>16)' in ARM mode
arch/arm/mach-sunxi/headsmp.S:25: Error: selected processor does not support `movw r2,#(0x4100c0f0&0xffff)' in ARM mode
arch/arm/mach-sunxi/headsmp.S:26: Error: selected processor does not support `movt r2,#(0x4100c0f0>>16)' in ARM mode

vim +22 arch/arm/mach-sunxi/headsmp.S

13
14 ENTRY(sunxi_mc_smp_cluster_cache_enable)
15 /*
16 * Enable cluster-level coherency, in preparation for turning on the MMU.
17 *
18 * Also enable regional clock gating and L2 data latency settings for
19 * Cortex-A15. These settings are from the vendor kernel.
20 */
21 mrc p15, 0, r1, c0, c0, 0
> 22 movw r2, #(0xff00fff0&0xffff)
> 23 movt r2, #(0xff00fff0>>16)
24 and r1, r1, r2
25 movw r2, #(0x4100c0f0&0xffff)
26 movt r2, #(0x4100c0f0>>16)
27 cmp r1, r2
28 bne not_a15
29
30 /* The following is Cortex-A15 specific */
31
32 /* ACTLR2: Enable CPU regional clock gates */
33 mrc p15, 1, r1, c15, c0, 4
34 orr r1, r1, #(0x1<<31)
35 mcr p15, 1, r1, c15, c0, 4
36
37 /* L2ACTLR */
38 mrc p15, 1, r1, c15, c0, 0
39 /* Enable L2, GIC, and Timer regional clock gates */
40 orr r1, r1, #(0x1<<26)
41 /* Disable clean/evict from being pushed to external */
42 orr r1, r1, #(0x1<<3)
43 mcr p15, 1, r1, c15, c0, 0
44
45 /* L2CTRL: L2 data RAM latency */
46 mrc p15, 1, r1, c9, c0, 2
47 bic r1, r1, #(0x7<<0)
48 orr r1, r1, #(0x3<<0)
49 mcr p15, 1, r1, c9, c0, 2
50
51 /* End of Cortex-A15 specific setup */
52 not_a15:
53
54 /* Get value of sunxi_mc_smp_first_comer */
55 adr r1, first
56 ldr r0, [r1]
57 ldr r0, [r1, r0]
58
59 /* Skip cci_enable_port_for_self if not first comer */
60 cmp r0, #0
61 bxeq lr
62 b cci_enable_port_for_self
63
64 .align 2
65 first: .word sunxi_mc_smp_first_comer - .
66 ENDPROC(sunxi_mc_smp_cluster_cache_enable)
67

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Attachments:

(No filename) (3.27 kB)
.config.gz (63.77 kB)
Download all attachments

2018-02-26 10:14:27

[permalink] [raw]

Subject: Re: [PATCH v4 10/10] ARM: sunxi: smp: Add initialization of CNTVOFF

On Mon, Mar 19, 2018 at 10:14:19AM +0800, Chen-Yu Tsai wrote:
> On Mon, Mar 19, 2018 at 3:07 AM, Myl?ne Josserand
> <[email protected]> wrote:
> > Hello Mark,
> >
> > Please, excuse me for this late answer and thank you for the review!
> >
> > On Wed, 7 Mar 2018 12:18:33 +0000
> > Marc Zyngier <[email protected]> wrote:
> >
> >> On 23/02/18 13:37, Myl?ne Josserand wrote:
> >> > On Cortex-A7, the CNTVOFF register from arch timer is uninitialized.
> >>
> >> Only on A7? Is that specific to your platform?
> >
> > I do not really know other Allwinner's platforms about this subject. At
> > least, the sun9i-a80 which is a Cortex-a15/a7 does not need that but it
> > is necessary for sun8i-a83t which is a cortex-a7. Maybe, Chen-Yu or
> > Maxime could help us on it.
>
> AFAIK all Allwinner CPUs need it if there isn't a firmware (PSCI) layer
> beneath the kernel that will do the setup. We just have
> "arm,cpu-registers-not-fw-configured" set for all the other SoCs that
> have in-kernel SMP support, which includes the A31, A23, A33 and A80.

Most of these ones are here for historical reasons though. Now that we
have U-Boot properly setting it up, we could probably remove it.

Maxime

--
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com