[PATCH v4 0/3] Renesas R9A06G032 SMP enabler

[PATCH v4 0/3] Renesas R9A06G032 SMP enabler

[Michel Pollet]

*WARNING -- this requires the base R9A06G032 support patches
already posted

This patch series is for enabling the second CA7 of the R9A06G032.
It's based on a spin_table method, and it reuses the same binding
property as that driver.

v4:
  + Geert's comments adressed.
  + Renamed symbols to r9a06g032 to match the rest of patchset
  + Rebased on base patch v8
v3:
  + Removed mentions of rz/?n1d?
  + Rebased on base patch v7
v2:
  + Added suggestions from Florian Fainelli
  + Use __pa_symbol()
  + Simplified logic in prepare_cpu()
  + Reordered the patches
  + Rebased on RZN1 Base patch v5

Michel Pollet (3):
  dt-bindings: cpu: Add Renesas R9A06G032 SMP enable method.
  arm: shmobile: Add the R9A06G032 SMP enabler driver
  ARM: dts: Renesas R9A06G032 SMP enable method

 Documentation/devicetree/bindings/arm/cpus.txt |  1 +
 arch/arm/boot/dts/r9a06g032.dtsi               |  3 +
 arch/arm/mach-shmobile/Makefile                |  1 +
 arch/arm/mach-shmobile/smp-r9a06g032.c         | 79 ++++++++++++++++++++++++++
 4 files changed, 84 insertions(+)
 create mode 100644 arch/arm/mach-shmobile/smp-r9a06g032.c

-- 
2.7.4

[PATCH v4 1/3] dt-bindings: cpu: Add Renesas R9A06G032 SMP enable method.

[Michel Pollet]

Add a special enable method for second CA7 of the R9A06G032

Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/arm/cpus.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
index 29e1dc5..b395d107 100644
--- a/Documentation/devicetree/bindings/arm/cpus.txt
+++ b/Documentation/devicetree/bindings/arm/cpus.txt
@@ -219,6 +219,7 @@ described below.
 			    "qcom,kpss-acc-v1"
 			    "qcom,kpss-acc-v2"
 			    "renesas,apmu"
+			    "renesas,r9a06g032-smp"
 			    "rockchip,rk3036-smp"
 			    "rockchip,rk3066-smp"
 			    "ste,dbx500-smp"
-- 
2.7.4

[PATCH v4 2/3] arm: shmobile: Add the R9A06G032 SMP enabler driver

[Michel Pollet]

The Renesas R9A06G032 second CA7 is parked in a ROM pen at boot time, it
requires a special enable method to get it started.

Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
---
 arch/arm/mach-shmobile/Makefile        |  1 +
 arch/arm/mach-shmobile/smp-r9a06g032.c | 79 ++++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+)
 create mode 100644 arch/arm/mach-shmobile/smp-r9a06g032.c

diff --git a/arch/arm/mach-shmobile/Makefile b/arch/arm/mach-shmobile/Makefile
index 1939f52..d7fc98f 100644
--- a/arch/arm/mach-shmobile/Makefile
+++ b/arch/arm/mach-shmobile/Makefile
@@ -34,6 +34,7 @@ smp-$(CONFIG_ARCH_SH73A0)	+= smp-sh73a0.o headsmp-scu.o platsmp-scu.o
 smp-$(CONFIG_ARCH_R8A7779)	+= smp-r8a7779.o headsmp-scu.o platsmp-scu.o
 smp-$(CONFIG_ARCH_R8A7790)	+= smp-r8a7790.o
 smp-$(CONFIG_ARCH_R8A7791)	+= smp-r8a7791.o
+smp-$(CONFIG_ARCH_R9A06G032)	+= smp-r9a06g032.o
 smp-$(CONFIG_ARCH_EMEV2)	+= smp-emev2.o headsmp-scu.o platsmp-scu.o
 
 # PM objects
diff --git a/arch/arm/mach-shmobile/smp-r9a06g032.c b/arch/arm/mach-shmobile/smp-r9a06g032.c
new file mode 100644
index 0000000..cd40e6e
--- /dev/null
+++ b/arch/arm/mach-shmobile/smp-r9a06g032.c
@@ -0,0 +1,79 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * R9A06G032 Second CA7 enabler.
+ *
+ * Copyright (C) 2018 Renesas Electronics Europe Limited
+ *
+ * Michel Pollet <michel.pollet@bp.renesas.com>, <buserror@gmail.com>
+ * Derived from action,s500-smp
+ */
+
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/smp.h>
+
+/*
+ * The second CPU is parked in ROM at boot time. It requires waking it after
+ * writing an address into the BOOTADDR register of sysctrl.
+ *
+ * So the default value of the "cpu-release-addr" corresponds to BOOTADDR...
+ *
+ * *However* the BOOTADDR register is not available when the kernel
+ * starts in NONSEC mode.
+ *
+ * So for NONSEC mode, the bootloader re-parks the second CPU into a pen
+ * in SRAM, and changes the "cpu-release-addr" of linux's DT to a SRAM address,
+ * which is not restricted.
+ */
+
+static void __iomem *cpu_bootaddr;
+
+static DEFINE_SPINLOCK(cpu_lock);
+
+static int r9a06g032_smp_boot_secondary(unsigned int cpu, struct task_struct *idle)
+{
+	if (!cpu_bootaddr)
+		return -ENODEV;
+
+	spin_lock(&cpu_lock);
+
+	writel(__pa_symbol(secondary_startup), cpu_bootaddr);
+	arch_send_wakeup_ipi_mask(cpumask_of(cpu));
+
+	spin_unlock(&cpu_lock);
+
+	return 0;
+}
+
+static void __init r9a06g032_smp_prepare_cpus(unsigned int max_cpus)
+{
+	struct device_node *dn;
+	int ret;
+	u32 bootaddr;
+
+	dn = of_get_cpu_node(1, NULL);
+	if (!dn) {
+		pr_err("CPU#1: missing device tree node\n");
+		return;
+	}
+	/*
+	 * Determine the address from which the CPU is polling.
+	 * The bootloader *does* change this property
+	 */
+	ret = of_property_read_u32(dn, "cpu-release-addr", &bootaddr);
+	of_node_put(dn);
+	if (ret) {
+		pr_err("CPU#1: invalid cpu-release-addr property\n");
+		return;
+	}
+	pr_info("CPU#1: cpu-release-addr %08x\n", bootaddr);
+
+	cpu_bootaddr = ioremap(bootaddr, sizeof(bootaddr));
+}
+
+static const struct smp_operations r9a06g032_smp_ops __initconst = {
+	.smp_prepare_cpus = r9a06g032_smp_prepare_cpus,
+	.smp_boot_secondary = r9a06g032_smp_boot_secondary,
+};
+CPU_METHOD_OF_DECLARE(r9a06g032_smp, "renesas,r9a06g032-smp", &r9a06g032_smp_ops);
-- 
2.7.4

[PATCH v4 3/3] ARM: dts: Renesas R9A06G032 SMP enable method

[Michel Pollet]

Add a special enable method for the second CA7 of the R9A06G032
as well as the default value for the "cpu-release-addr" property.

Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
---
 arch/arm/boot/dts/r9a06g032.dtsi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/boot/dts/r9a06g032.dtsi b/arch/arm/boot/dts/r9a06g032.dtsi
index 40827bb..9d7e74a 100644
--- a/arch/arm/boot/dts/r9a06g032.dtsi
+++ b/arch/arm/boot/dts/r9a06g032.dtsi
@@ -1,3 +1,4 @@
+
 // SPDX-License-Identifier: GPL-2.0
 /*
  * Base Device Tree Source for the Renesas RZ/N1D (R9A06G032)
@@ -30,6 +31,8 @@
 			compatible = "arm,cortex-a7";
 			reg = <1>;
 			clocks = <&sysctrl R9A06G032_CLK_A7MP>;
+			enable-method = "renesas,r9a06g032-smp";
+			cpu-release-addr = <0x4000c204>;
 		};
 	};
 
-- 
2.7.4

Re: [PATCH v4 1/3] dt-bindings: cpu: Add Renesas R9A06G032 SMP enable method.

[Geert Uytterhoeven]

On Tue, Jun 5, 2018 at 1:28 PM, Michel Pollet
<michel.pollet@bp.renesas.com> wrote:
> Add a special enable method for second CA7 of the R9A06G032
>
> Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
> Reviewed-by: Rob Herring <robh@kernel.org>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v4 2/3] arm: shmobile: Add the R9A06G032 SMP enabler driver

[Geert Uytterhoeven]

On Tue, Jun 5, 2018 at 1:28 PM, Michel Pollet
<michel.pollet@bp.renesas.com> wrote:
> The Renesas R9A06G032 second CA7 is parked in a ROM pen at boot time, it
> requires a special enable method to get it started.
>
> Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- /dev/null
> +++ b/arch/arm/mach-shmobile/smp-r9a06g032.c
> @@ -0,0 +1,79 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * R9A06G032 Second CA7 enabler.
> + *
> + * Copyright (C) 2018 Renesas Electronics Europe Limited
> + *
> + * Michel Pollet <michel.pollet@bp.renesas.com>, <buserror@gmail.com>
> + * Derived from action,s500-smp

actions

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v4 3/3] ARM: dts: Renesas R9A06G032 SMP enable method

[Geert Uytterhoeven]

On Tue, Jun 5, 2018 at 1:28 PM, Michel Pollet
<michel.pollet@bp.renesas.com> wrote:
> Add a special enable method for the second CA7 of the R9A06G032
> as well as the default value for the "cpu-release-addr" property.
>
> Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v4 3/3] ARM: dts: Renesas R9A06G032 SMP enable method

[Geert Uytterhoeven]

On Tue, Jun 5, 2018 at 1:28 PM, Michel Pollet
<michel.pollet@bp.renesas.com> wrote:
> Add a special enable method for the second CA7 of the R9A06G032
> as well as the default value for the "cpu-release-addr" property.
>
> Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- a/arch/arm/boot/dts/r9a06g032.dtsi
> +++ b/arch/arm/boot/dts/r9a06g032.dtsi
> @@ -1,3 +1,4 @@
> +

Bogus change

>  // SPDX-License-Identifier: GPL-2.0
>  /*
>   * Base Device Tree Source for the Renesas RZ/N1D (R9A06G032)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v4 2/3] arm: shmobile: Add the R9A06G032 SMP enabler driver

[Frank Rowand]

On 06/05/18 04:28, Michel Pollet wrote:
> The Renesas R9A06G032 second CA7 is parked in a ROM pen at boot time, it
> requires a special enable method to get it started.
> 
> Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
> ---
>  arch/arm/mach-shmobile/Makefile        |  1 +
>  arch/arm/mach-shmobile/smp-r9a06g032.c | 79 ++++++++++++++++++++++++++++++++++
>  2 files changed, 80 insertions(+)
>  create mode 100644 arch/arm/mach-shmobile/smp-r9a06g032.c
> 
> diff --git a/arch/arm/mach-shmobile/Makefile b/arch/arm/mach-shmobile/Makefile
> index 1939f52..d7fc98f 100644
> --- a/arch/arm/mach-shmobile/Makefile
> +++ b/arch/arm/mach-shmobile/Makefile
> @@ -34,6 +34,7 @@ smp-$(CONFIG_ARCH_SH73A0)	+= smp-sh73a0.o headsmp-scu.o platsmp-scu.o
>  smp-$(CONFIG_ARCH_R8A7779)	+= smp-r8a7779.o headsmp-scu.o platsmp-scu.o
>  smp-$(CONFIG_ARCH_R8A7790)	+= smp-r8a7790.o
>  smp-$(CONFIG_ARCH_R8A7791)	+= smp-r8a7791.o
> +smp-$(CONFIG_ARCH_R9A06G032)	+= smp-r9a06g032.o
>  smp-$(CONFIG_ARCH_EMEV2)	+= smp-emev2.o headsmp-scu.o platsmp-scu.o
>  
>  # PM objects
> diff --git a/arch/arm/mach-shmobile/smp-r9a06g032.c b/arch/arm/mach-shmobile/smp-r9a06g032.c
> new file mode 100644
> index 0000000..cd40e6e
> --- /dev/null
> +++ b/arch/arm/mach-shmobile/smp-r9a06g032.c
> @@ -0,0 +1,79 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * R9A06G032 Second CA7 enabler.
> + *
> + * Copyright (C) 2018 Renesas Electronics Europe Limited
> + *
> + * Michel Pollet <michel.pollet@bp.renesas.com>, <buserror@gmail.com>
> + * Derived from action,s500-smp
> + */
> +
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/smp.h>
> +
> +/*
> + * The second CPU is parked in ROM at boot time. It requires waking it after
> + * writing an address into the BOOTADDR register of sysctrl.
> + *
> + * So the default value of the "cpu-release-addr" corresponds to BOOTADDR...
> + *
> + * *However* the BOOTADDR register is not available when the kernel
> + * starts in NONSEC mode.
> + *
> + * So for NONSEC mode, the bootloader re-parks the second CPU into a pen
> + * in SRAM, and changes the "cpu-release-addr" of linux's DT to a SRAM address,
> + * which is not restricted.

The binding document for cpu-release-addr does not have a definition for 32 bit
arm.  The existing definition is only 64 bit arm.  Please add the definition
for 32 bit arm to patch 1.

-Frank


> + */
> +
> +static void __iomem *cpu_bootaddr;
> +
> +static DEFINE_SPINLOCK(cpu_lock);
> +
> +static int r9a06g032_smp_boot_secondary(unsigned int cpu, struct task_struct *idle)
> +{
> +	if (!cpu_bootaddr)
> +		return -ENODEV;
> +
> +	spin_lock(&cpu_lock);
> +
> +	writel(__pa_symbol(secondary_startup), cpu_bootaddr);
> +	arch_send_wakeup_ipi_mask(cpumask_of(cpu));
> +
> +	spin_unlock(&cpu_lock);
> +
> +	return 0;
> +}
> +
> +static void __init r9a06g032_smp_prepare_cpus(unsigned int max_cpus)
> +{
> +	struct device_node *dn;
> +	int ret;
> +	u32 bootaddr;
> +
> +	dn = of_get_cpu_node(1, NULL);
> +	if (!dn) {
> +		pr_err("CPU#1: missing device tree node\n");
> +		return;
> +	}
> +	/*
> +	 * Determine the address from which the CPU is polling.
> +	 * The bootloader *does* change this property
> +	 */
> +	ret = of_property_read_u32(dn, "cpu-release-addr", &bootaddr);
> +	of_node_put(dn);
> +	if (ret) {
> +		pr_err("CPU#1: invalid cpu-release-addr property\n");
> +		return;
> +	}
> +	pr_info("CPU#1: cpu-release-addr %08x\n", bootaddr);
> +
> +	cpu_bootaddr = ioremap(bootaddr, sizeof(bootaddr));
> +}
> +
> +static const struct smp_operations r9a06g032_smp_ops __initconst = {
> +	.smp_prepare_cpus = r9a06g032_smp_prepare_cpus,
> +	.smp_boot_secondary = r9a06g032_smp_boot_secondary,
> +};
> +CPU_METHOD_OF_DECLARE(r9a06g032_smp, "renesas,r9a06g032-smp", &r9a06g032_smp_ops);
> 

RE: [PATCH v4 2/3] arm: shmobile: Add the R9A06G032 SMP enabler driver

[Michel Pollet]

Hi Frank,

On 05 June 2018 18:34, Frank wrote:
> On 06/05/18 04:28, Michel Pollet wrote:
> > The Renesas R9A06G032 second CA7 is parked in a ROM pen at boot time,
> > it requires a special enable method to get it started.
> >
> > Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
> > ---
> >  arch/arm/mach-shmobile/Makefile        |  1 +
> >  arch/arm/mach-shmobile/smp-r9a06g032.c | 79
> > ++++++++++++++++++++++++++++++++++
> >  2 files changed, 80 insertions(+)
> >  create mode 100644 arch/arm/mach-shmobile/smp-r9a06g032.c
> >
> > diff --git a/arch/arm/mach-shmobile/Makefile
> > b/arch/arm/mach-shmobile/Makefile index 1939f52..d7fc98f 100644
> > --- a/arch/arm/mach-shmobile/Makefile
> > +++ b/arch/arm/mach-shmobile/Makefile
> > @@ -34,6 +34,7 @@ smp-$(CONFIG_ARCH_SH73A0)+= smp-sh73a0.o
> headsmp-scu.o platsmp-scu.o
> >  smp-$(CONFIG_ARCH_R8A7779)+= smp-r8a7779.o headsmp-scu.o
> platsmp-scu.o
> >  smp-$(CONFIG_ARCH_R8A7790)+= smp-r8a7790.o
> >  smp-$(CONFIG_ARCH_R8A7791)+= smp-r8a7791.o
> > +smp-$(CONFIG_ARCH_R9A06G032)+= smp-r9a06g032.o
> >  smp-$(CONFIG_ARCH_EMEV2)+= smp-emev2.o headsmp-scu.o
> platsmp-scu.o
> >
> >  # PM objects
> > diff --git a/arch/arm/mach-shmobile/smp-r9a06g032.c
> > b/arch/arm/mach-shmobile/smp-r9a06g032.c
> > new file mode 100644
> > index 0000000..cd40e6e
> > --- /dev/null
> > +++ b/arch/arm/mach-shmobile/smp-r9a06g032.c
> > @@ -0,0 +1,79 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * R9A06G032 Second CA7 enabler.
> > + *
> > + * Copyright (C) 2018 Renesas Electronics Europe Limited
> > + *
> > + * Michel Pollet <michel.pollet@bp.renesas.com>,
> <buserror@gmail.com>
> > + * Derived from action,s500-smp
> > + */
> > +
> > +#include <linux/io.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/smp.h>
> > +
> > +/*
> > + * The second CPU is parked in ROM at boot time. It requires waking
> > +it after
> > + * writing an address into the BOOTADDR register of sysctrl.
> > + *
> > + * So the default value of the "cpu-release-addr" corresponds to
> BOOTADDR...
> > + *
> > + * *However* the BOOTADDR register is not available when the kernel
> > + * starts in NONSEC mode.
> > + *
> > + * So for NONSEC mode, the bootloader re-parks the second CPU into a
> > +pen
> > + * in SRAM, and changes the "cpu-release-addr" of linux's DT to a
> > +SRAM address,
> > + * which is not restricted.
>
> The binding document for cpu-release-addr does not have a definition for 32
> bit arm.  The existing definition is only 64 bit arm.  Please add the definition
> for 32 bit arm to patch 1.

Hmmm I do find a definition in
Documentation/devicetree/bindings/arm/cpus.txt -- just under where I
added my 'enable-method' -- And it is already used as 32 bits in at least
arch/arm/boot/dts/stih407-family.dtsi.

What do you want me to add to this exactly? Do you want me to just
change "required for systems that have an "enable-method" property
value of "spin-table" to also specify renesas,r9a06g032 ?

Thanks!
Michel

>
> -Frank
>
>
> > + */
> > +
> > +static void __iomem *cpu_bootaddr;
> > +
> > +static DEFINE_SPINLOCK(cpu_lock);
> > +
> > +static int r9a06g032_smp_boot_secondary(unsigned int cpu, struct
> > +task_struct *idle) {
> > +if (!cpu_bootaddr)
> > +return -ENODEV;
> > +
> > +spin_lock(&cpu_lock);
> > +
> > +writel(__pa_symbol(secondary_startup), cpu_bootaddr);
> > +arch_send_wakeup_ipi_mask(cpumask_of(cpu));
> > +
> > +spin_unlock(&cpu_lock);
> > +
> > +return 0;
> > +}
> > +
> > +static void __init r9a06g032_smp_prepare_cpus(unsigned int max_cpus)
> > +{
> > +struct device_node *dn;
> > +int ret;
> > +u32 bootaddr;
> > +
> > +dn = of_get_cpu_node(1, NULL);
> > +if (!dn) {
> > +pr_err("CPU#1: missing device tree node\n");
> > +return;
> > +}
> > +/*
> > + * Determine the address from which the CPU is polling.
> > + * The bootloader *does* change this property
> > + */
> > +ret = of_property_read_u32(dn, "cpu-release-addr", &bootaddr);
> > +of_node_put(dn);
> > +if (ret) {
> > +pr_err("CPU#1: invalid cpu-release-addr property\n");
> > +return;
> > +}
> > +pr_info("CPU#1: cpu-release-addr %08x\n", bootaddr);
> > +
> > +cpu_bootaddr = ioremap(bootaddr, sizeof(bootaddr)); }
> > +
> > +static const struct smp_operations r9a06g032_smp_ops __initconst = {
> > +.smp_prepare_cpus = r9a06g032_smp_prepare_cpus,
> > +.smp_boot_secondary = r9a06g032_smp_boot_secondary, };
> > +CPU_METHOD_OF_DECLARE(r9a06g032_smp, "renesas,r9a06g032-smp",
> > +&r9a06g032_smp_ops);
> >




Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Re: [PATCH v4 1/3] dt-bindings: cpu: Add Renesas R9A06G032 SMP enable method.

[Simon Horman]

On Tue, Jun 05, 2018 at 12:28:46PM +0100, Michel Pollet wrote:
> Add a special enable method for second CA7 of the R9A06G032
> 
> Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
> Reviewed-by: Rob Herring <robh@kernel.org>

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

Re: [PATCH v4 2/3] arm: shmobile: Add the R9A06G032 SMP enabler driver

[Frank Rowand]

Hi Michel,

On 06/05/18 23:36, Michel Pollet wrote:
> Hi Frank,
> 
> On 05 June 2018 18:34, Frank wrote:
>> On 06/05/18 04:28, Michel Pollet wrote:
>>> The Renesas R9A06G032 second CA7 is parked in a ROM pen at boot time,
>>> it requires a special enable method to get it started.
>>>
>>> Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
>>> ---
>>>  arch/arm/mach-shmobile/Makefile        |  1 +
>>>  arch/arm/mach-shmobile/smp-r9a06g032.c | 79
>>> ++++++++++++++++++++++++++++++++++
>>>  2 files changed, 80 insertions(+)
>>>  create mode 100644 arch/arm/mach-shmobile/smp-r9a06g032.c
>>>
>>> diff --git a/arch/arm/mach-shmobile/Makefile
>>> b/arch/arm/mach-shmobile/Makefile index 1939f52..d7fc98f 100644
>>> --- a/arch/arm/mach-shmobile/Makefile
>>> +++ b/arch/arm/mach-shmobile/Makefile
>>> @@ -34,6 +34,7 @@ smp-$(CONFIG_ARCH_SH73A0)+= smp-sh73a0.o
>> headsmp-scu.o platsmp-scu.o
>>>  smp-$(CONFIG_ARCH_R8A7779)+= smp-r8a7779.o headsmp-scu.o
>> platsmp-scu.o
>>>  smp-$(CONFIG_ARCH_R8A7790)+= smp-r8a7790.o
>>>  smp-$(CONFIG_ARCH_R8A7791)+= smp-r8a7791.o
>>> +smp-$(CONFIG_ARCH_R9A06G032)+= smp-r9a06g032.o
>>>  smp-$(CONFIG_ARCH_EMEV2)+= smp-emev2.o headsmp-scu.o
>> platsmp-scu.o
>>>
>>>  # PM objects
>>> diff --git a/arch/arm/mach-shmobile/smp-r9a06g032.c
>>> b/arch/arm/mach-shmobile/smp-r9a06g032.c
>>> new file mode 100644
>>> index 0000000..cd40e6e
>>> --- /dev/null
>>> +++ b/arch/arm/mach-shmobile/smp-r9a06g032.c
>>> @@ -0,0 +1,79 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * R9A06G032 Second CA7 enabler.
>>> + *
>>> + * Copyright (C) 2018 Renesas Electronics Europe Limited
>>> + *
>>> + * Michel Pollet <michel.pollet@bp.renesas.com>,
>> <buserror@gmail.com>
>>> + * Derived from action,s500-smp
>>> + */
>>> +
>>> +#include <linux/io.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_address.h>
>>> +#include <linux/smp.h>
>>> +
>>> +/*
>>> + * The second CPU is parked in ROM at boot time. It requires waking
>>> +it after
>>> + * writing an address into the BOOTADDR register of sysctrl.
>>> + *
>>> + * So the default value of the "cpu-release-addr" corresponds to
>> BOOTADDR...
>>> + *
>>> + * *However* the BOOTADDR register is not available when the kernel
>>> + * starts in NONSEC mode.
>>> + *
>>> + * So for NONSEC mode, the bootloader re-parks the second CPU into a
>>> +pen
>>> + * in SRAM, and changes the "cpu-release-addr" of linux's DT to a
>>> +SRAM address,
>>> + * which is not restricted.
>>
>> The binding document for cpu-release-addr does not have a definition for 32
>> bit arm.  The existing definition is only 64 bit arm.  Please add the definition
>> for 32 bit arm to patch 1.
> 
> Hmmm I do find a definition in
> Documentation/devicetree/bindings/arm/cpus.txt -- just under where I
> added my 'enable-method' -- And it is already used as 32 bits in at least
> arch/arm/boot/dts/stih407-family.dtsi.

>From cpus.txt:

        - cpu-release-addr
                Usage: required for systems that have an "enable-method"
                       property value of "spin-table".
                Value type: <prop-encoded-array>
                Definition:
                        # On ARM v8 64-bit systems must be a two cell
                          property identifying a 64-bit zero-initialised
                          memory location.

The definition specifies a two cell property for 64-bit systems.

Please add to the definition that cpu-release-addr is a one cell property
for 32-bit systems.

-Frank

> 
> What do you want me to add to this exactly? Do you want me to just
> change "required for systems that have an "enable-method" property
> value of "spin-table" to also specify renesas,r9a06g032 ?
> 
> Thanks!
> Michel
> 
>>
>> -Frank
>>
>>
>>> + */
>>> +
>>> +static void __iomem *cpu_bootaddr;
>>> +
>>> +static DEFINE_SPINLOCK(cpu_lock);
>>> +
>>> +static int r9a06g032_smp_boot_secondary(unsigned int cpu, struct
>>> +task_struct *idle) {
>>> +if (!cpu_bootaddr)
>>> +return -ENODEV;
>>> +
>>> +spin_lock(&cpu_lock);
>>> +
>>> +writel(__pa_symbol(secondary_startup), cpu_bootaddr);
>>> +arch_send_wakeup_ipi_mask(cpumask_of(cpu));
>>> +
>>> +spin_unlock(&cpu_lock);
>>> +
>>> +return 0;
>>> +}
>>> +
>>> +static void __init r9a06g032_smp_prepare_cpus(unsigned int max_cpus)
>>> +{
>>> +struct device_node *dn;
>>> +int ret;
>>> +u32 bootaddr;
>>> +
>>> +dn = of_get_cpu_node(1, NULL);
>>> +if (!dn) {
>>> +pr_err("CPU#1: missing device tree node\n");
>>> +return;
>>> +}
>>> +/*
>>> + * Determine the address from which the CPU is polling.
>>> + * The bootloader *does* change this property
>>> + */
>>> +ret = of_property_read_u32(dn, "cpu-release-addr", &bootaddr);
>>> +of_node_put(dn);
>>> +if (ret) {
>>> +pr_err("CPU#1: invalid cpu-release-addr property\n");
>>> +return;
>>> +}
>>> +pr_info("CPU#1: cpu-release-addr %08x\n", bootaddr);
>>> +
>>> +cpu_bootaddr = ioremap(bootaddr, sizeof(bootaddr)); }
>>> +
>>> +static const struct smp_operations r9a06g032_smp_ops __initconst = {
>>> +.smp_prepare_cpus = r9a06g032_smp_prepare_cpus,
>>> +.smp_boot_secondary = r9a06g032_smp_boot_secondary, };
>>> +CPU_METHOD_OF_DECLARE(r9a06g032_smp, "renesas,r9a06g032-smp",
>>> +&r9a06g032_smp_ops);
>>>
> 
> 
> 
> 
> Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
> 

Re: [PATCH v4 2/3] arm: shmobile: Add the R9A06G032 SMP enabler driver

[Rob Herring]

On Wed, Jun 6, 2018 at 2:30 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> Hi Michel,
>
> On 06/05/18 23:36, Michel Pollet wrote:
>> Hi Frank,
>>
>> On 05 June 2018 18:34, Frank wrote:
>>> On 06/05/18 04:28, Michel Pollet wrote:
>>>> The Renesas R9A06G032 second CA7 is parked in a ROM pen at boot time,
>>>> it requires a special enable method to get it started.
>>>>
>>>> Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
>>>> ---
>>>>  arch/arm/mach-shmobile/Makefile        |  1 +
>>>>  arch/arm/mach-shmobile/smp-r9a06g032.c | 79
>>>> ++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 80 insertions(+)
>>>>  create mode 100644 arch/arm/mach-shmobile/smp-r9a06g032.c
>>>>
>>>> diff --git a/arch/arm/mach-shmobile/Makefile
>>>> b/arch/arm/mach-shmobile/Makefile index 1939f52..d7fc98f 100644
>>>> --- a/arch/arm/mach-shmobile/Makefile
>>>> +++ b/arch/arm/mach-shmobile/Makefile
>>>> @@ -34,6 +34,7 @@ smp-$(CONFIG_ARCH_SH73A0)+= smp-sh73a0.o
>>> headsmp-scu.o platsmp-scu.o
>>>>  smp-$(CONFIG_ARCH_R8A7779)+= smp-r8a7779.o headsmp-scu.o
>>> platsmp-scu.o
>>>>  smp-$(CONFIG_ARCH_R8A7790)+= smp-r8a7790.o
>>>>  smp-$(CONFIG_ARCH_R8A7791)+= smp-r8a7791.o
>>>> +smp-$(CONFIG_ARCH_R9A06G032)+= smp-r9a06g032.o
>>>>  smp-$(CONFIG_ARCH_EMEV2)+= smp-emev2.o headsmp-scu.o
>>> platsmp-scu.o
>>>>
>>>>  # PM objects
>>>> diff --git a/arch/arm/mach-shmobile/smp-r9a06g032.c
>>>> b/arch/arm/mach-shmobile/smp-r9a06g032.c
>>>> new file mode 100644
>>>> index 0000000..cd40e6e
>>>> --- /dev/null
>>>> +++ b/arch/arm/mach-shmobile/smp-r9a06g032.c
>>>> @@ -0,0 +1,79 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * R9A06G032 Second CA7 enabler.
>>>> + *
>>>> + * Copyright (C) 2018 Renesas Electronics Europe Limited
>>>> + *
>>>> + * Michel Pollet <michel.pollet@bp.renesas.com>,
>>> <buserror@gmail.com>
>>>> + * Derived from action,s500-smp
>>>> + */
>>>> +
>>>> +#include <linux/io.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_address.h>
>>>> +#include <linux/smp.h>
>>>> +
>>>> +/*
>>>> + * The second CPU is parked in ROM at boot time. It requires waking
>>>> +it after
>>>> + * writing an address into the BOOTADDR register of sysctrl.
>>>> + *
>>>> + * So the default value of the "cpu-release-addr" corresponds to
>>> BOOTADDR...
>>>> + *
>>>> + * *However* the BOOTADDR register is not available when the kernel
>>>> + * starts in NONSEC mode.
>>>> + *
>>>> + * So for NONSEC mode, the bootloader re-parks the second CPU into a
>>>> +pen
>>>> + * in SRAM, and changes the "cpu-release-addr" of linux's DT to a
>>>> +SRAM address,
>>>> + * which is not restricted.
>>>
>>> The binding document for cpu-release-addr does not have a definition for 32
>>> bit arm.  The existing definition is only 64 bit arm.  Please add the definition
>>> for 32 bit arm to patch 1.
>>
>> Hmmm I do find a definition in
>> Documentation/devicetree/bindings/arm/cpus.txt -- just under where I
>> added my 'enable-method' -- And it is already used as 32 bits in at least
>> arch/arm/boot/dts/stih407-family.dtsi.
>
> From cpus.txt:
>
>         - cpu-release-addr
>                 Usage: required for systems that have an "enable-method"
>                        property value of "spin-table".
>                 Value type: <prop-encoded-array>
>                 Definition:
>                         # On ARM v8 64-bit systems must be a two cell
>                           property identifying a 64-bit zero-initialised
>                           memory location.
>
> The definition specifies a two cell property for 64-bit systems.
>
> Please add to the definition that cpu-release-addr is a one cell property
> for 32-bit systems.

Actually, this is all already documented in the DT spec and it is
always 2 cells[1]. We should perhaps just remove whatever is
duplicated from the spec.

Rob

[1]
   ``cpu-release-addr``   | SD  | ``<u64>``        The
cpu-release-addr property is required for
                                                   cpu nodes that have
an enable-method property
                                                   value of
``"spin-table"``. The value specifies the
                                                   physical address of
a spin table entry that
                                                   releases a
secondary CPU from its spin loop.

Re: [PATCH v4 2/3] arm: shmobile: Add the R9A06G032 SMP enabler driver

[Florian Fainelli]

On 06/06/2018 12:30 PM, Frank Rowand wrote:
> Hi Michel,
> 
> On 06/05/18 23:36, Michel Pollet wrote:
>> Hi Frank,
>>
>> On 05 June 2018 18:34, Frank wrote:
>>> On 06/05/18 04:28, Michel Pollet wrote:
>>>> The Renesas R9A06G032 second CA7 is parked in a ROM pen at boot time,
>>>> it requires a special enable method to get it started.
>>>>
>>>> Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
>>>> ---
>>>>  arch/arm/mach-shmobile/Makefile        |  1 +
>>>>  arch/arm/mach-shmobile/smp-r9a06g032.c | 79
>>>> ++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 80 insertions(+)
>>>>  create mode 100644 arch/arm/mach-shmobile/smp-r9a06g032.c
>>>>
>>>> diff --git a/arch/arm/mach-shmobile/Makefile
>>>> b/arch/arm/mach-shmobile/Makefile index 1939f52..d7fc98f 100644
>>>> --- a/arch/arm/mach-shmobile/Makefile
>>>> +++ b/arch/arm/mach-shmobile/Makefile
>>>> @@ -34,6 +34,7 @@ smp-$(CONFIG_ARCH_SH73A0)+= smp-sh73a0.o
>>> headsmp-scu.o platsmp-scu.o
>>>>  smp-$(CONFIG_ARCH_R8A7779)+= smp-r8a7779.o headsmp-scu.o
>>> platsmp-scu.o
>>>>  smp-$(CONFIG_ARCH_R8A7790)+= smp-r8a7790.o
>>>>  smp-$(CONFIG_ARCH_R8A7791)+= smp-r8a7791.o
>>>> +smp-$(CONFIG_ARCH_R9A06G032)+= smp-r9a06g032.o
>>>>  smp-$(CONFIG_ARCH_EMEV2)+= smp-emev2.o headsmp-scu.o
>>> platsmp-scu.o
>>>>
>>>>  # PM objects
>>>> diff --git a/arch/arm/mach-shmobile/smp-r9a06g032.c
>>>> b/arch/arm/mach-shmobile/smp-r9a06g032.c
>>>> new file mode 100644
>>>> index 0000000..cd40e6e
>>>> --- /dev/null
>>>> +++ b/arch/arm/mach-shmobile/smp-r9a06g032.c
>>>> @@ -0,0 +1,79 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * R9A06G032 Second CA7 enabler.
>>>> + *
>>>> + * Copyright (C) 2018 Renesas Electronics Europe Limited
>>>> + *
>>>> + * Michel Pollet <michel.pollet@bp.renesas.com>,
>>> <buserror@gmail.com>
>>>> + * Derived from action,s500-smp
>>>> + */
>>>> +
>>>> +#include <linux/io.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_address.h>
>>>> +#include <linux/smp.h>
>>>> +
>>>> +/*
>>>> + * The second CPU is parked in ROM at boot time. It requires waking
>>>> +it after
>>>> + * writing an address into the BOOTADDR register of sysctrl.
>>>> + *
>>>> + * So the default value of the "cpu-release-addr" corresponds to
>>> BOOTADDR...
>>>> + *
>>>> + * *However* the BOOTADDR register is not available when the kernel
>>>> + * starts in NONSEC mode.
>>>> + *
>>>> + * So for NONSEC mode, the bootloader re-parks the second CPU into a
>>>> +pen
>>>> + * in SRAM, and changes the "cpu-release-addr" of linux's DT to a
>>>> +SRAM address,
>>>> + * which is not restricted.
>>>
>>> The binding document for cpu-release-addr does not have a definition for 32
>>> bit arm.  The existing definition is only 64 bit arm.  Please add the definition
>>> for 32 bit arm to patch 1.
>>
>> Hmmm I do find a definition in
>> Documentation/devicetree/bindings/arm/cpus.txt -- just under where I
>> added my 'enable-method' -- And it is already used as 32 bits in at least
>> arch/arm/boot/dts/stih407-family.dtsi.
> 
> From cpus.txt:
> 
>         - cpu-release-addr
>                 Usage: required for systems that have an "enable-method"
>                        property value of "spin-table".
>                 Value type: <prop-encoded-array>
>                 Definition:
>                         # On ARM v8 64-bit systems must be a two cell
>                           property identifying a 64-bit zero-initialised
>                           memory location.
> 
> The definition specifies a two cell property for 64-bit systems.
> 
> Please add to the definition that cpu-release-addr is a one cell property
> for 32-bit systems.

Or maybe phrase it such that the number of cells encoded in
cpu-release-addr must exactly match the CPU node's #address-cells size?
-- 
Florian

Re: [PATCH v4 2/3] arm: shmobile: Add the R9A06G032 SMP enabler driver

[Geert Uytterhoeven]

Hi Rob,

On Wed, Jun 6, 2018 at 9:35 PM, Rob Herring <robh+dt@kernel.org> wrote:
> On Wed, Jun 6, 2018 at 2:30 PM, Frank Rowand <frowand.list@gmail.com> wrote:
>> On 06/05/18 23:36, Michel Pollet wrote:
>>> On 05 June 2018 18:34, Frank wrote:
>>>> On 06/05/18 04:28, Michel Pollet wrote:
>>>>> The Renesas R9A06G032 second CA7 is parked in a ROM pen at boot time,
>>>>> it requires a special enable method to get it started.
>>>>>
>>>>> Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>

>>>>> --- /dev/null
>>>>> +++ b/arch/arm/mach-shmobile/smp-r9a06g032.c

>>>>> +/*
>>>>> + * The second CPU is parked in ROM at boot time. It requires waking
>>>>> +it after
>>>>> + * writing an address into the BOOTADDR register of sysctrl.
>>>>> + *
>>>>> + * So the default value of the "cpu-release-addr" corresponds to
>>>> BOOTADDR...
>>>>> + *
>>>>> + * *However* the BOOTADDR register is not available when the kernel
>>>>> + * starts in NONSEC mode.
>>>>> + *
>>>>> + * So for NONSEC mode, the bootloader re-parks the second CPU into a
>>>>> +pen
>>>>> + * in SRAM, and changes the "cpu-release-addr" of linux's DT to a
>>>>> +SRAM address,
>>>>> + * which is not restricted.
>>>>
>>>> The binding document for cpu-release-addr does not have a definition for 32
>>>> bit arm.  The existing definition is only 64 bit arm.  Please add the definition
>>>> for 32 bit arm to patch 1.
>>>
>>> Hmmm I do find a definition in
>>> Documentation/devicetree/bindings/arm/cpus.txt -- just under where I
>>> added my 'enable-method' -- And it is already used as 32 bits in at least
>>> arch/arm/boot/dts/stih407-family.dtsi.
>>
>> From cpus.txt:
>>
>>         - cpu-release-addr
>>                 Usage: required for systems that have an "enable-method"
>>                        property value of "spin-table".
>>                 Value type: <prop-encoded-array>
>>                 Definition:
>>                         # On ARM v8 64-bit systems must be a two cell
>>                           property identifying a 64-bit zero-initialised
>>                           memory location.
>>
>> The definition specifies a two cell property for 64-bit systems.
>>
>> Please add to the definition that cpu-release-addr is a one cell property
>> for 32-bit systems.
>
> Actually, this is all already documented in the DT spec and it is
> always 2 cells[1]. We should perhaps just remove whatever is
> duplicated from the spec.
>
> Rob
>
> [1]
>    ``cpu-release-addr``   | SD  | ``<u64>``        The
> cpu-release-addr property is required for
>                                                    cpu nodes that have
> an enable-method property
>                                                    value of
> ``"spin-table"``. The value specifies the
>                                                    physical address of
> a spin table entry that
>                                                    releases a
> secondary CPU from its spin loop.

Interesting. But why is this <u64>, and not just following #address-cells?
Due to the ePAPR-spec being 64-bit Power System-centric?

There's also "initial-mapped-area", which must use 64-bit values for effective
and physical addresses, according to ePAPR.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v4 2/3] arm: shmobile: Add the R9A06G032 SMP enabler driver

[Geert Uytterhoeven]

Hi Florian,

On Wed, Jun 6, 2018 at 9:37 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 06/06/2018 12:30 PM, Frank Rowand wrote:
>> On 06/05/18 23:36, Michel Pollet wrote:
>>> On 05 June 2018 18:34, Frank wrote:
>>>> On 06/05/18 04:28, Michel Pollet wrote:
>>>>> The Renesas R9A06G032 second CA7 is parked in a ROM pen at boot time,
>>>>> it requires a special enable method to get it started.
>>>>>
>>>>> Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>

>>>>> --- /dev/null
>>>>> +++ b/arch/arm/mach-shmobile/smp-r9a06g032.c

>>>>> +/*
>>>>> + * The second CPU is parked in ROM at boot time. It requires waking
>>>>> +it after
>>>>> + * writing an address into the BOOTADDR register of sysctrl.
>>>>> + *
>>>>> + * So the default value of the "cpu-release-addr" corresponds to
>>>> BOOTADDR...
>>>>> + *
>>>>> + * *However* the BOOTADDR register is not available when the kernel
>>>>> + * starts in NONSEC mode.
>>>>> + *
>>>>> + * So for NONSEC mode, the bootloader re-parks the second CPU into a
>>>>> +pen
>>>>> + * in SRAM, and changes the "cpu-release-addr" of linux's DT to a
>>>>> +SRAM address,
>>>>> + * which is not restricted.
>>>>
>>>> The binding document for cpu-release-addr does not have a definition for 32
>>>> bit arm.  The existing definition is only 64 bit arm.  Please add the definition
>>>> for 32 bit arm to patch 1.
>>>
>>> Hmmm I do find a definition in
>>> Documentation/devicetree/bindings/arm/cpus.txt -- just under where I
>>> added my 'enable-method' -- And it is already used as 32 bits in at least
>>> arch/arm/boot/dts/stih407-family.dtsi.
>>
>> From cpus.txt:
>>
>>         - cpu-release-addr
>>                 Usage: required for systems that have an "enable-method"
>>                        property value of "spin-table".
>>                 Value type: <prop-encoded-array>
>>                 Definition:
>>                         # On ARM v8 64-bit systems must be a two cell
>>                           property identifying a 64-bit zero-initialised
>>                           memory location.
>>
>> The definition specifies a two cell property for 64-bit systems.
>>
>> Please add to the definition that cpu-release-addr is a one cell property
>> for 32-bit systems.
>
> Or maybe phrase it such that the number of cells encoded in
> cpu-release-addr must exactly match the CPU node's #address-cells size?

The CPU node's #address-cells size is unrelated.
You need the #address-cells value from the SoC bus (typically the root
node, not considering heterogeneous systems with multiple CPUs ;-).

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v4 2/3] arm: shmobile: Add the R9A06G032 SMP enabler driver

[Rob Herring]

On Wed, Jun 6, 2018 at 2:42 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Rob,
>
> On Wed, Jun 6, 2018 at 9:35 PM, Rob Herring <robh+dt@kernel.org> wrote:
>> On Wed, Jun 6, 2018 at 2:30 PM, Frank Rowand <frowand.list@gmail.com> wrote:
>>> On 06/05/18 23:36, Michel Pollet wrote:
>>>> On 05 June 2018 18:34, Frank wrote:
>>>>> On 06/05/18 04:28, Michel Pollet wrote:
>>>>>> The Renesas R9A06G032 second CA7 is parked in a ROM pen at boot time,
>>>>>> it requires a special enable method to get it started.
>>>>>>
>>>>>> Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
>
>>>>>> --- /dev/null
>>>>>> +++ b/arch/arm/mach-shmobile/smp-r9a06g032.c
>
>>>>>> +/*
>>>>>> + * The second CPU is parked in ROM at boot time. It requires waking
>>>>>> +it after
>>>>>> + * writing an address into the BOOTADDR register of sysctrl.
>>>>>> + *
>>>>>> + * So the default value of the "cpu-release-addr" corresponds to
>>>>> BOOTADDR...
>>>>>> + *
>>>>>> + * *However* the BOOTADDR register is not available when the kernel
>>>>>> + * starts in NONSEC mode.
>>>>>> + *
>>>>>> + * So for NONSEC mode, the bootloader re-parks the second CPU into a
>>>>>> +pen
>>>>>> + * in SRAM, and changes the "cpu-release-addr" of linux's DT to a
>>>>>> +SRAM address,
>>>>>> + * which is not restricted.
>>>>>
>>>>> The binding document for cpu-release-addr does not have a definition for 32
>>>>> bit arm.  The existing definition is only 64 bit arm.  Please add the definition
>>>>> for 32 bit arm to patch 1.
>>>>
>>>> Hmmm I do find a definition in
>>>> Documentation/devicetree/bindings/arm/cpus.txt -- just under where I
>>>> added my 'enable-method' -- And it is already used as 32 bits in at least
>>>> arch/arm/boot/dts/stih407-family.dtsi.
>>>
>>> From cpus.txt:
>>>
>>>         - cpu-release-addr
>>>                 Usage: required for systems that have an "enable-method"
>>>                        property value of "spin-table".
>>>                 Value type: <prop-encoded-array>
>>>                 Definition:
>>>                         # On ARM v8 64-bit systems must be a two cell
>>>                           property identifying a 64-bit zero-initialised
>>>                           memory location.
>>>
>>> The definition specifies a two cell property for 64-bit systems.
>>>
>>> Please add to the definition that cpu-release-addr is a one cell property
>>> for 32-bit systems.
>>
>> Actually, this is all already documented in the DT spec and it is
>> always 2 cells[1]. We should perhaps just remove whatever is
>> duplicated from the spec.
>>
>> Rob
>>
>> [1]
>>    ``cpu-release-addr``   | SD  | ``<u64>``        The
>> cpu-release-addr property is required for
>>                                                    cpu nodes that have
>> an enable-method property
>>                                                    value of
>> ``"spin-table"``. The value specifies the
>>                                                    physical address of
>> a spin table entry that
>>                                                    releases a
>> secondary CPU from its spin loop.
>
> Interesting. But why is this <u64>, and not just following #address-cells?

As you said in your other email, it's not the same.

> Due to the ePAPR-spec being 64-bit Power System-centric?

Other than #address-cells already having another defined purpose in
/cpus, my guess is 64-bit works for either and 32-bit SMP systems
didn't predate 64-bit (for the ePAPR author's perspective).

> There's also "initial-mapped-area", which must use 64-bit values for effective
> and physical addresses, according to ePAPR.

I would have thought that followed #{size,address}-cells being
/memory. Though, I guess the bootloader fills this in and it is much
easier to work with fixed sizes.

Rob

Re: [PATCH v4 2/3] arm: shmobile: Add the R9A06G032 SMP enabler driver

[Frank Rowand]

On 06/06/18 12:35, Rob Herring wrote:
> On Wed, Jun 6, 2018 at 2:30 PM, Frank Rowand <frowand.list@gmail.com> wrote:
>> Hi Michel,
>>
>> On 06/05/18 23:36, Michel Pollet wrote:
>>> Hi Frank,
>>>
>>> On 05 June 2018 18:34, Frank wrote:
>>>> On 06/05/18 04:28, Michel Pollet wrote:
>>>>> The Renesas R9A06G032 second CA7 is parked in a ROM pen at boot time,
>>>>> it requires a special enable method to get it started.
>>>>>
>>>>> Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
>>>>> ---
>>>>>  arch/arm/mach-shmobile/Makefile        |  1 +
>>>>>  arch/arm/mach-shmobile/smp-r9a06g032.c | 79
>>>>> ++++++++++++++++++++++++++++++++++
>>>>>  2 files changed, 80 insertions(+)
>>>>>  create mode 100644 arch/arm/mach-shmobile/smp-r9a06g032.c
>>>>>
>>>>> diff --git a/arch/arm/mach-shmobile/Makefile
>>>>> b/arch/arm/mach-shmobile/Makefile index 1939f52..d7fc98f 100644
>>>>> --- a/arch/arm/mach-shmobile/Makefile
>>>>> +++ b/arch/arm/mach-shmobile/Makefile
>>>>> @@ -34,6 +34,7 @@ smp-$(CONFIG_ARCH_SH73A0)+= smp-sh73a0.o
>>>> headsmp-scu.o platsmp-scu.o
>>>>>  smp-$(CONFIG_ARCH_R8A7779)+= smp-r8a7779.o headsmp-scu.o
>>>> platsmp-scu.o
>>>>>  smp-$(CONFIG_ARCH_R8A7790)+= smp-r8a7790.o
>>>>>  smp-$(CONFIG_ARCH_R8A7791)+= smp-r8a7791.o
>>>>> +smp-$(CONFIG_ARCH_R9A06G032)+= smp-r9a06g032.o
>>>>>  smp-$(CONFIG_ARCH_EMEV2)+= smp-emev2.o headsmp-scu.o
>>>> platsmp-scu.o
>>>>>
>>>>>  # PM objects
>>>>> diff --git a/arch/arm/mach-shmobile/smp-r9a06g032.c
>>>>> b/arch/arm/mach-shmobile/smp-r9a06g032.c
>>>>> new file mode 100644
>>>>> index 0000000..cd40e6e
>>>>> --- /dev/null
>>>>> +++ b/arch/arm/mach-shmobile/smp-r9a06g032.c
>>>>> @@ -0,0 +1,79 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>> +/*
>>>>> + * R9A06G032 Second CA7 enabler.
>>>>> + *
>>>>> + * Copyright (C) 2018 Renesas Electronics Europe Limited
>>>>> + *
>>>>> + * Michel Pollet <michel.pollet@bp.renesas.com>,
>>>> <buserror@gmail.com>
>>>>> + * Derived from action,s500-smp
>>>>> + */
>>>>> +
>>>>> +#include <linux/io.h>
>>>>> +#include <linux/of.h>
>>>>> +#include <linux/of_address.h>
>>>>> +#include <linux/smp.h>
>>>>> +
>>>>> +/*
>>>>> + * The second CPU is parked in ROM at boot time. It requires waking
>>>>> +it after
>>>>> + * writing an address into the BOOTADDR register of sysctrl.
>>>>> + *
>>>>> + * So the default value of the "cpu-release-addr" corresponds to
>>>> BOOTADDR...
>>>>> + *
>>>>> + * *However* the BOOTADDR register is not available when the kernel
>>>>> + * starts in NONSEC mode.
>>>>> + *
>>>>> + * So for NONSEC mode, the bootloader re-parks the second CPU into a
>>>>> +pen
>>>>> + * in SRAM, and changes the "cpu-release-addr" of linux's DT to a
>>>>> +SRAM address,
>>>>> + * which is not restricted.
>>>>
>>>> The binding document for cpu-release-addr does not have a definition for 32
>>>> bit arm.  The existing definition is only 64 bit arm.  Please add the definition
>>>> for 32 bit arm to patch 1.
>>>
>>> Hmmm I do find a definition in
>>> Documentation/devicetree/bindings/arm/cpus.txt -- just under where I
>>> added my 'enable-method' -- And it is already used as 32 bits in at least
>>> arch/arm/boot/dts/stih407-family.dtsi.
>>
>> From cpus.txt:
>>
>>         - cpu-release-addr
>>                 Usage: required for systems that have an "enable-method"
>>                        property value of "spin-table".
>>                 Value type: <prop-encoded-array>
>>                 Definition:
>>                         # On ARM v8 64-bit systems must be a two cell
>>                           property identifying a 64-bit zero-initialised
>>                           memory location.
>>
>> The definition specifies a two cell property for 64-bit systems.
>>
>> Please add to the definition that cpu-release-addr is a one cell property
>> for 32-bit systems.
> 
> Actually, this is all already documented in the DT spec and it is
> always 2 cells[1]. We should perhaps just remove whatever is
> duplicated from the spec.

Thanks for noting that.  I jumped to the (incorrect) conclusion that the
property should be one cell based on the code and the .dtsi.

There are about 4 more emails following this in the thread that discuss
what size cpu-release-addr should be.  Whatever the end result is (one
cell or two or based on some #XXX-calls value), it needs to be documented
consistently in the binding and in the DT spec (or preferably only in the
DT spec), and if it is a two cell property for this system then
smp-r9a06g032.c and r9a06g032.dtsi need to be adjusted to reflect that.

-Frank

> 
> Rob
> 
> [1]
>    ``cpu-release-addr``   | SD  | ``<u64>``        The
> cpu-release-addr property is required for
>                                                    cpu nodes that have
> an enable-method property
>                                                    value of
> ``"spin-table"``. The value specifies the
>                                                    physical address of
> a spin table entry that
>                                                    releases a
> secondary CPU from its spin loop.
> 

Re: [PATCH v4 2/3] arm: shmobile: Add the R9A06G032 SMP enabler driver

[Frank Rowand]

On 06/05/18 23:36, Michel Pollet wrote:
> Hi Frank,
> 
> On 05 June 2018 18:34, Frank wrote:
>> On 06/05/18 04:28, Michel Pollet wrote:
>>> The Renesas R9A06G032 second CA7 is parked in a ROM pen at boot time,
>>> it requires a special enable method to get it started.
>>>
>>> Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
>>> ---
>>>  arch/arm/mach-shmobile/Makefile        |  1 +
>>>  arch/arm/mach-shmobile/smp-r9a06g032.c | 79
>>> ++++++++++++++++++++++++++++++++++
>>>  2 files changed, 80 insertions(+)
>>>  create mode 100644 arch/arm/mach-shmobile/smp-r9a06g032.c
>>>
>>> diff --git a/arch/arm/mach-shmobile/Makefile
>>> b/arch/arm/mach-shmobile/Makefile index 1939f52..d7fc98f 100644
>>> --- a/arch/arm/mach-shmobile/Makefile
>>> +++ b/arch/arm/mach-shmobile/Makefile
>>> @@ -34,6 +34,7 @@ smp-$(CONFIG_ARCH_SH73A0)+= smp-sh73a0.o
>> headsmp-scu.o platsmp-scu.o
>>>  smp-$(CONFIG_ARCH_R8A7779)+= smp-r8a7779.o headsmp-scu.o
>> platsmp-scu.o
>>>  smp-$(CONFIG_ARCH_R8A7790)+= smp-r8a7790.o
>>>  smp-$(CONFIG_ARCH_R8A7791)+= smp-r8a7791.o
>>> +smp-$(CONFIG_ARCH_R9A06G032)+= smp-r9a06g032.o
>>>  smp-$(CONFIG_ARCH_EMEV2)+= smp-emev2.o headsmp-scu.o
>> platsmp-scu.o
>>>
>>>  # PM objects
>>> diff --git a/arch/arm/mach-shmobile/smp-r9a06g032.c
>>> b/arch/arm/mach-shmobile/smp-r9a06g032.c
>>> new file mode 100644
>>> index 0000000..cd40e6e
>>> --- /dev/null
>>> +++ b/arch/arm/mach-shmobile/smp-r9a06g032.c
>>> @@ -0,0 +1,79 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * R9A06G032 Second CA7 enabler.
>>> + *
>>> + * Copyright (C) 2018 Renesas Electronics Europe Limited
>>> + *
>>> + * Michel Pollet <michel.pollet@bp.renesas.com>,
>> <buserror@gmail.com>
>>> + * Derived from action,s500-smp
>>> + */
>>> +
>>> +#include <linux/io.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_address.h>
>>> +#include <linux/smp.h>
>>> +
>>> +/*
>>> + * The second CPU is parked in ROM at boot time. It requires waking
>>> +it after
>>> + * writing an address into the BOOTADDR register of sysctrl.
>>> + *
>>> + * So the default value of the "cpu-release-addr" corresponds to
>> BOOTADDR...
>>> + *
>>> + * *However* the BOOTADDR register is not available when the kernel
>>> + * starts in NONSEC mode.
>>> + *
>>> + * So for NONSEC mode, the bootloader re-parks the second CPU into a
>>> +pen
>>> + * in SRAM, and changes the "cpu-release-addr" of linux's DT to a
>>> +SRAM address,
>>> + * which is not restricted.
>>
>> The binding document for cpu-release-addr does not have a definition for 32
>> bit arm.  The existing definition is only 64 bit arm.  Please add the definition
>> for 32 bit arm to patch 1.
> 
> Hmmm I do find a definition in
> Documentation/devicetree/bindings/arm/cpus.txt -- just under where I
> added my 'enable-method' -- And it is already used as 32 bits in at least
> arch/arm/boot/dts/stih407-family.dtsi.

If the correct answer is for cpu-release-addr to be 64 bits in certain
cases (that discussion is ongoing further downthread) then one approach
to maintain compatibility _and_ to fix the devicetree source files is
to change the source code that currently gets cpu-release-addr as a
32 bit object to check the size of the property and get it as either
a 32 bit or 64 bit object, based on the actual size of the property
in the device tree and then change the value in the devicetree source
files to be two cells.  BUT this does not consider the bootloader
complication.  arch/arm/boot/dts/axm5516-cpus.dtsi has a note
"// Fixed by the boot loader", so the boot loader also has to be
modified to be able to handle the possibility that the property
could be either 32 bits or 64 bits.  I don't know how to maintain
compatibility with the boot loader since we can't force it to
change synchronously with changes in the kernel.

You can consider this comment to be a drive-by observation.  I think
Rob and Geert and people like that are likely to be more helpful with
what to actually do, and you can treat my comment more as pointing out
the issue than as providing the perfect solution.

-Frnak


> 
> What do you want me to add to this exactly? Do you want me to just
> change "required for systems that have an "enable-method" property
> value of "spin-table" to also specify renesas,r9a06g032 ?
> 
> Thanks!
> Michel
> 
>>
>> -Frank
>>
>>
>>> + */
>>> +
>>> +static void __iomem *cpu_bootaddr;
>>> +
>>> +static DEFINE_SPINLOCK(cpu_lock);
>>> +
>>> +static int r9a06g032_smp_boot_secondary(unsigned int cpu, struct
>>> +task_struct *idle) {
>>> +if (!cpu_bootaddr)
>>> +return -ENODEV;
>>> +
>>> +spin_lock(&cpu_lock);
>>> +
>>> +writel(__pa_symbol(secondary_startup), cpu_bootaddr);
>>> +arch_send_wakeup_ipi_mask(cpumask_of(cpu));
>>> +
>>> +spin_unlock(&cpu_lock);
>>> +
>>> +return 0;
>>> +}
>>> +
>>> +static void __init r9a06g032_smp_prepare_cpus(unsigned int max_cpus)
>>> +{
>>> +struct device_node *dn;
>>> +int ret;
>>> +u32 bootaddr;
>>> +
>>> +dn = of_get_cpu_node(1, NULL);
>>> +if (!dn) {
>>> +pr_err("CPU#1: missing device tree node\n");
>>> +return;
>>> +}
>>> +/*
>>> + * Determine the address from which the CPU is polling.
>>> + * The bootloader *does* change this property
>>> + */
>>> +ret = of_property_read_u32(dn, "cpu-release-addr", &bootaddr);
>>> +of_node_put(dn);
>>> +if (ret) {
>>> +pr_err("CPU#1: invalid cpu-release-addr property\n");
>>> +return;
>>> +}
>>> +pr_info("CPU#1: cpu-release-addr %08x\n", bootaddr);
>>> +
>>> +cpu_bootaddr = ioremap(bootaddr, sizeof(bootaddr)); }
>>> +
>>> +static const struct smp_operations r9a06g032_smp_ops __initconst = {
>>> +.smp_prepare_cpus = r9a06g032_smp_prepare_cpus,
>>> +.smp_boot_secondary = r9a06g032_smp_boot_secondary, };
>>> +CPU_METHOD_OF_DECLARE(r9a06g032_smp, "renesas,r9a06g032-smp",
>>> +&r9a06g032_smp_ops);
>>>
> 
> 
> 
> 
> Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
> 

Re: [PATCH v4 2/3] arm: shmobile: Add the R9A06G032 SMP enabler driver

[Frank Rowand]

On 06/06/18 14:48, Frank Rowand wrote:
> On 06/05/18 23:36, Michel Pollet wrote:
>> Hi Frank,
>>
>> On 05 June 2018 18:34, Frank wrote:
>>> On 06/05/18 04:28, Michel Pollet wrote:
>>>> The Renesas R9A06G032 second CA7 is parked in a ROM pen at boot time,
>>>> it requires a special enable method to get it started.
>>>>
>>>> Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
>>>> ---
>>>>  arch/arm/mach-shmobile/Makefile        |  1 +
>>>>  arch/arm/mach-shmobile/smp-r9a06g032.c | 79
>>>> ++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 80 insertions(+)
>>>>  create mode 100644 arch/arm/mach-shmobile/smp-r9a06g032.c
>>>>
>>>> diff --git a/arch/arm/mach-shmobile/Makefile
>>>> b/arch/arm/mach-shmobile/Makefile index 1939f52..d7fc98f 100644
>>>> --- a/arch/arm/mach-shmobile/Makefile
>>>> +++ b/arch/arm/mach-shmobile/Makefile
>>>> @@ -34,6 +34,7 @@ smp-$(CONFIG_ARCH_SH73A0)+= smp-sh73a0.o
>>> headsmp-scu.o platsmp-scu.o
>>>>  smp-$(CONFIG_ARCH_R8A7779)+= smp-r8a7779.o headsmp-scu.o
>>> platsmp-scu.o
>>>>  smp-$(CONFIG_ARCH_R8A7790)+= smp-r8a7790.o
>>>>  smp-$(CONFIG_ARCH_R8A7791)+= smp-r8a7791.o
>>>> +smp-$(CONFIG_ARCH_R9A06G032)+= smp-r9a06g032.o
>>>>  smp-$(CONFIG_ARCH_EMEV2)+= smp-emev2.o headsmp-scu.o
>>> platsmp-scu.o
>>>>
>>>>  # PM objects
>>>> diff --git a/arch/arm/mach-shmobile/smp-r9a06g032.c
>>>> b/arch/arm/mach-shmobile/smp-r9a06g032.c
>>>> new file mode 100644
>>>> index 0000000..cd40e6e
>>>> --- /dev/null
>>>> +++ b/arch/arm/mach-shmobile/smp-r9a06g032.c
>>>> @@ -0,0 +1,79 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * R9A06G032 Second CA7 enabler.
>>>> + *
>>>> + * Copyright (C) 2018 Renesas Electronics Europe Limited
>>>> + *
>>>> + * Michel Pollet <michel.pollet@bp.renesas.com>,
>>> <buserror@gmail.com>
>>>> + * Derived from action,s500-smp
>>>> + */
>>>> +
>>>> +#include <linux/io.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_address.h>
>>>> +#include <linux/smp.h>
>>>> +
>>>> +/*
>>>> + * The second CPU is parked in ROM at boot time. It requires waking
>>>> +it after
>>>> + * writing an address into the BOOTADDR register of sysctrl.
>>>> + *
>>>> + * So the default value of the "cpu-release-addr" corresponds to
>>> BOOTADDR...
>>>> + *
>>>> + * *However* the BOOTADDR register is not available when the kernel
>>>> + * starts in NONSEC mode.
>>>> + *
>>>> + * So for NONSEC mode, the bootloader re-parks the second CPU into a
>>>> +pen
>>>> + * in SRAM, and changes the "cpu-release-addr" of linux's DT to a
>>>> +SRAM address,
>>>> + * which is not restricted.
>>>
>>> The binding document for cpu-release-addr does not have a definition for 32
>>> bit arm.  The existing definition is only 64 bit arm.  Please add the definition
>>> for 32 bit arm to patch 1.
>>
>> Hmmm I do find a definition in
>> Documentation/devicetree/bindings/arm/cpus.txt -- just under where I
>> added my 'enable-method' -- And it is already used as 32 bits in at least
>> arch/arm/boot/dts/stih407-family.dtsi.
> 
> If the correct answer is for cpu-release-addr to be 64 bits in certain
> cases (that discussion is ongoing further downthread) then one approach
> to maintain compatibility _and_ to fix the devicetree source files is
> to change the source code that currently gets cpu-release-addr as a
> 32 bit object to check the size of the property and get it as either
> a 32 bit or 64 bit object, based on the actual size of the property
> in the device tree and then change the value in the devicetree source
> files to be two cells.  BUT this does not consider the bootloader
> complication.  arch/arm/boot/dts/axm5516-cpus.dtsi has a note
> "// Fixed by the boot loader", so the boot loader also has to be
> modified to be able to handle the possibility that the property
> could be either 32 bits or 64 bits.  I don't know how to maintain
> compatibility with the boot loader since we can't force it to
> change synchronously with changes in the kernel.
> 
> You can consider this comment to be a drive-by observation.  I think
> Rob and Geert and people like that are likely to be more helpful with
> what to actually do, and you can treat my comment more as pointing out
> the issue than as providing the perfect solution.

Darn it, hit <send> too quickly.

I meant to mention that there are several devicetree source files that
have a single cell value for cpu-release-addr, and thus potentially
face the same situation, depending on what the final decision is on
the proper size for cpu-release-addr. As of v4.17, a git grep shows
one cell values in:

  arch/arm/boot/dts/axm5516-cpus.dtsi
  arch/arm/boot/dts/stih407-family.dtsi
  arch/arm/boot/dts/stih418.dtsi

-Frank

> -Frnak
> 
> 
>>
>> What do you want me to add to this exactly? Do you want me to just
>> change "required for systems that have an "enable-method" property
>> value of "spin-table" to also specify renesas,r9a06g032 ?
>>
>> Thanks!
>> Michel
>>
>>>
>>> -Frank
>>>
>>>
>>>> + */
>>>> +
>>>> +static void __iomem *cpu_bootaddr;
>>>> +
>>>> +static DEFINE_SPINLOCK(cpu_lock);
>>>> +
>>>> +static int r9a06g032_smp_boot_secondary(unsigned int cpu, struct
>>>> +task_struct *idle) {
>>>> +if (!cpu_bootaddr)
>>>> +return -ENODEV;
>>>> +
>>>> +spin_lock(&cpu_lock);
>>>> +
>>>> +writel(__pa_symbol(secondary_startup), cpu_bootaddr);
>>>> +arch_send_wakeup_ipi_mask(cpumask_of(cpu));
>>>> +
>>>> +spin_unlock(&cpu_lock);
>>>> +
>>>> +return 0;
>>>> +}
>>>> +
>>>> +static void __init r9a06g032_smp_prepare_cpus(unsigned int max_cpus)
>>>> +{
>>>> +struct device_node *dn;
>>>> +int ret;
>>>> +u32 bootaddr;
>>>> +
>>>> +dn = of_get_cpu_node(1, NULL);
>>>> +if (!dn) {
>>>> +pr_err("CPU#1: missing device tree node\n");
>>>> +return;
>>>> +}
>>>> +/*
>>>> + * Determine the address from which the CPU is polling.
>>>> + * The bootloader *does* change this property
>>>> + */
>>>> +ret = of_property_read_u32(dn, "cpu-release-addr", &bootaddr);
>>>> +of_node_put(dn);
>>>> +if (ret) {
>>>> +pr_err("CPU#1: invalid cpu-release-addr property\n");
>>>> +return;
>>>> +}
>>>> +pr_info("CPU#1: cpu-release-addr %08x\n", bootaddr);
>>>> +
>>>> +cpu_bootaddr = ioremap(bootaddr, sizeof(bootaddr)); }
>>>> +
>>>> +static const struct smp_operations r9a06g032_smp_ops __initconst = {
>>>> +.smp_prepare_cpus = r9a06g032_smp_prepare_cpus,
>>>> +.smp_boot_secondary = r9a06g032_smp_boot_secondary, };
>>>> +CPU_METHOD_OF_DECLARE(r9a06g032_smp, "renesas,r9a06g032-smp",
>>>> +&r9a06g032_smp_ops);
>>>>
>>
>>
>>
>>
>> Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
>>
> 
> 

RE: [PATCH v4 2/3] arm: shmobile: Add the R9A06G032 SMP enabler driver

[Michel Pollet]

On 06 June 2018 22:53, Frank wrote:
> On 06/06/18 14:48, Frank Rowand wrote:
> > On 06/05/18 23:36, Michel Pollet wrote:
> >> Hi Frank,
> >>
> >> On 05 June 2018 18:34, Frank wrote:
> >>> On 06/05/18 04:28, Michel Pollet wrote:
> >>>> The Renesas R9A06G032 second CA7 is parked in a ROM pen at boot
> >>>> time, it requires a special enable method to get it started.
> >>>>
> >>>> Signed-off-by: Michel Pollet <michel.pollet@bp.renesas.com>
> >>>> ---
> >>>>  arch/arm/mach-shmobile/Makefile        |  1 +
> >>>>  arch/arm/mach-shmobile/smp-r9a06g032.c | 79
> >>>> ++++++++++++++++++++++++++++++++++
> >>>>  2 files changed, 80 insertions(+)
> >>>>  create mode 100644 arch/arm/mach-shmobile/smp-r9a06g032.c
> >>>>
> >>>> diff --git a/arch/arm/mach-shmobile/Makefile
> >>>> b/arch/arm/mach-shmobile/Makefile index 1939f52..d7fc98f 100644
> >>>> --- a/arch/arm/mach-shmobile/Makefile
> >>>> +++ b/arch/arm/mach-shmobile/Makefile
> >>>> @@ -34,6 +34,7 @@ smp-$(CONFIG_ARCH_SH73A0)+= smp-sh73a0.o
> >>> headsmp-scu.o platsmp-scu.o
> >>>>  smp-$(CONFIG_ARCH_R8A7779)+= smp-r8a7779.o headsmp-scu.o
> >>> platsmp-scu.o
> >>>>  smp-$(CONFIG_ARCH_R8A7790)+= smp-r8a7790.o
> >>>> smp-$(CONFIG_ARCH_R8A7791)+= smp-r8a7791.o
> >>>> +smp-$(CONFIG_ARCH_R9A06G032)+= smp-r9a06g032.o
> >>>>  smp-$(CONFIG_ARCH_EMEV2)+= smp-emev2.o headsmp-scu.o
> >>> platsmp-scu.o
> >>>>
> >>>>  # PM objects
> >>>> diff --git a/arch/arm/mach-shmobile/smp-r9a06g032.c
> >>>> b/arch/arm/mach-shmobile/smp-r9a06g032.c
> >>>> new file mode 100644
> >>>> index 0000000..cd40e6e
> >>>> --- /dev/null
> >>>> +++ b/arch/arm/mach-shmobile/smp-r9a06g032.c
> >>>> @@ -0,0 +1,79 @@
> >>>> +// SPDX-License-Identifier: GPL-2.0
> >>>> +/*
> >>>> + * R9A06G032 Second CA7 enabler.
> >>>> + *
> >>>> + * Copyright (C) 2018 Renesas Electronics Europe Limited
> >>>> + *
> >>>> + * Michel Pollet <michel.pollet@bp.renesas.com>,
> >>> <buserror@gmail.com>
> >>>> + * Derived from action,s500-smp
> >>>> + */
> >>>> +
> >>>> +#include <linux/io.h>
> >>>> +#include <linux/of.h>
> >>>> +#include <linux/of_address.h>
> >>>> +#include <linux/smp.h>
> >>>> +
> >>>> +/*
> >>>> + * The second CPU is parked in ROM at boot time. It requires
> >>>> +waking it after
> >>>> + * writing an address into the BOOTADDR register of sysctrl.
> >>>> + *
> >>>> + * So the default value of the "cpu-release-addr" corresponds to
> >>> BOOTADDR...
> >>>> + *
> >>>> + * *However* the BOOTADDR register is not available when the
> >>>> +kernel
> >>>> + * starts in NONSEC mode.
> >>>> + *
> >>>> + * So for NONSEC mode, the bootloader re-parks the second CPU into
> >>>> +a pen
> >>>> + * in SRAM, and changes the "cpu-release-addr" of linux's DT to a
> >>>> +SRAM address,
> >>>> + * which is not restricted.
> >>>
> >>> The binding document for cpu-release-addr does not have a definition
> >>> for 32 bit arm.  The existing definition is only 64 bit arm.  Please
> >>> add the definition for 32 bit arm to patch 1.
> >>
> >> Hmmm I do find a definition in
> >> Documentation/devicetree/bindings/arm/cpus.txt -- just under where I
> >> added my 'enable-method' -- And it is already used as 32 bits in at
> >> least arch/arm/boot/dts/stih407-family.dtsi.
> >
> > If the correct answer is for cpu-release-addr to be 64 bits in certain
> > cases (that discussion is ongoing further downthread) then one
> > approach to maintain compatibility _and_ to fix the devicetree source
> > files is to change the source code that currently gets
> > cpu-release-addr as a
> > 32 bit object to check the size of the property and get it as either a
> > 32 bit or 64 bit object, based on the actual size of the property in
> > the device tree and then change the value in the devicetree source
> > files to be two cells.  BUT this does not consider the bootloader
> > complication.  arch/arm/boot/dts/axm5516-cpus.dtsi has a note "//
> > Fixed by the boot loader", so the boot loader also has to be modified
> > to be able to handle the possibility that the property could be either
> > 32 bits or 64 bits.  I don't know how to maintain compatibility with
> > the boot loader since we can't force it to change synchronously with
> > changes in the kernel.
> >
> > You can consider this comment to be a drive-by observation.  I think
> > Rob and Geert and people like that are likely to be more helpful with
> > what to actually do, and you can treat my comment more as pointing out
> > the issue than as providing the perfect solution.
>
> Darn it, hit <send> too quickly.
>
> I meant to mention that there are several devicetree source files that have a
> single cell value for cpu-release-addr, and thus potentially face the same
> situation, depending on what the final decision is on the proper size for cpu-
> release-addr. As of v4.17, a git grep shows one cell values in:
>
>   arch/arm/boot/dts/axm5516-cpus.dtsi
>   arch/arm/boot/dts/stih407-family.dtsi
>   arch/arm/boot/dts/stih418.dtsi

Yes, I had grepped before I used 32 bits on mine...

Now, what is the decision here? Our bootloader is already modified to set it to 32 bits, so I propose that

+ I change the driver to handle 32 and 64 bits properties
+ I add this to the cpu.txt, as a separate patch:
# On other systems, the property can be either
  32 bits or 64 bits, it is the driver's responsibility
  to deal with either sizes.

Michel

>
> -Frank
>
> > -Frnak
> >
> >
> >>
> >> What do you want me to add to this exactly? Do you want me to just
> >> change "required for systems that have an "enable-method" property
> >> value of "spin-table" to also specify renesas,r9a06g032 ?
> >>
> >> Thanks!
> >> Michel
> >>
> >>>
> >>> -Frank
> >>>
> >>>
> >>>> + */
> >>>> +
> >>>> +static void __iomem *cpu_bootaddr;
> >>>> +
> >>>> +static DEFINE_SPINLOCK(cpu_lock);
> >>>> +
> >>>> +static int r9a06g032_smp_boot_secondary(unsigned int cpu, struct
> >>>> +task_struct *idle) { if (!cpu_bootaddr) return -ENODEV;
> >>>> +
> >>>> +spin_lock(&cpu_lock);
> >>>> +
> >>>> +writel(__pa_symbol(secondary_startup), cpu_bootaddr);
> >>>> +arch_send_wakeup_ipi_mask(cpumask_of(cpu));
> >>>> +
> >>>> +spin_unlock(&cpu_lock);
> >>>> +
> >>>> +return 0;
> >>>> +}
> >>>> +
> >>>> +static void __init r9a06g032_smp_prepare_cpus(unsigned int
> >>>> +max_cpus) { struct device_node *dn; int ret;
> >>>> +u32 bootaddr;
> >>>> +
> >>>> +dn = of_get_cpu_node(1, NULL);
> >>>> +if (!dn) {
> >>>> +pr_err("CPU#1: missing device tree node\n"); return; }
> >>>> +/*
> >>>> + * Determine the address from which the CPU is polling.
> >>>> + * The bootloader *does* change this property  */ ret =
> >>>> +of_property_read_u32(dn, "cpu-release-addr", &bootaddr);
> >>>> +of_node_put(dn); if (ret) {
> >>>> +pr_err("CPU#1: invalid cpu-release-addr property\n"); return; }
> >>>> +pr_info("CPU#1: cpu-release-addr %08x\n", bootaddr);
> >>>> +
> >>>> +cpu_bootaddr = ioremap(bootaddr, sizeof(bootaddr)); }
> >>>> +
> >>>> +static const struct smp_operations r9a06g032_smp_ops __initconst =
> >>>> +{ .smp_prepare_cpus = r9a06g032_smp_prepare_cpus,
> >>>> +.smp_boot_secondary = r9a06g032_smp_boot_secondary, };
> >>>> +CPU_METHOD_OF_DECLARE(r9a06g032_smp, "renesas,r9a06g032-
> smp",
> >>>> +&r9a06g032_smp_ops);
> >>>>
> >>
> >>
> >>
> >>
> >> Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne
> End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under
> Registered No. 04586709.
> >>
> >
> >




Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Re: [PATCH v4 2/3] arm: shmobile: Add the R9A06G032 SMP enabler driver

[Rob Herring]

On Thu, Jun 7, 2018 at 1:59 AM, Michel Pollet
<michel.pollet@bp.renesas.com> wrote:
> On 06 June 2018 22:53, Frank wrote:
>> On 06/06/18 14:48, Frank Rowand wrote:
>> > On 06/05/18 23:36, Michel Pollet wrote:
>> >> Hi Frank,
>> >>
>> >> On 05 June 2018 18:34, Frank wrote:
>> >>> On 06/05/18 04:28, Michel Pollet wrote:
>> >>>> The Renesas R9A06G032 second CA7 is parked in a ROM pen at boot
>> >>>> time, it requires a special enable method to get it started.

[...]

>> >>>> + * The second CPU is parked in ROM at boot time. It requires
>> >>>> +waking it after
>> >>>> + * writing an address into the BOOTADDR register of sysctrl.
>> >>>> + *
>> >>>> + * So the default value of the "cpu-release-addr" corresponds to
>> >>> BOOTADDR...
>> >>>> + *
>> >>>> + * *However* the BOOTADDR register is not available when the
>> >>>> +kernel
>> >>>> + * starts in NONSEC mode.
>> >>>> + *
>> >>>> + * So for NONSEC mode, the bootloader re-parks the second CPU into
>> >>>> +a pen
>> >>>> + * in SRAM, and changes the "cpu-release-addr" of linux's DT to a
>> >>>> +SRAM address,
>> >>>> + * which is not restricted.
>> >>>
>> >>> The binding document for cpu-release-addr does not have a definition
>> >>> for 32 bit arm.  The existing definition is only 64 bit arm.  Please
>> >>> add the definition for 32 bit arm to patch 1.
>> >>
>> >> Hmmm I do find a definition in
>> >> Documentation/devicetree/bindings/arm/cpus.txt -- just under where I
>> >> added my 'enable-method' -- And it is already used as 32 bits in at
>> >> least arch/arm/boot/dts/stih407-family.dtsi.
>> >
>> > If the correct answer is for cpu-release-addr to be 64 bits in certain
>> > cases (that discussion is ongoing further downthread) then one
>> > approach to maintain compatibility _and_ to fix the devicetree source
>> > files is to change the source code that currently gets
>> > cpu-release-addr as a
>> > 32 bit object to check the size of the property and get it as either a
>> > 32 bit or 64 bit object, based on the actual size of the property in
>> > the device tree and then change the value in the devicetree source
>> > files to be two cells.  BUT this does not consider the bootloader
>> > complication.  arch/arm/boot/dts/axm5516-cpus.dtsi has a note "//
>> > Fixed by the boot loader", so the boot loader also has to be modified
>> > to be able to handle the possibility that the property could be either
>> > 32 bits or 64 bits.  I don't know how to maintain compatibility with
>> > the boot loader since we can't force it to change synchronously with
>> > changes in the kernel.
>> >
>> > You can consider this comment to be a drive-by observation.  I think
>> > Rob and Geert and people like that are likely to be more helpful with
>> > what to actually do, and you can treat my comment more as pointing out
>> > the issue than as providing the perfect solution.
>>
>> Darn it, hit <send> too quickly.
>>
>> I meant to mention that there are several devicetree source files that have a
>> single cell value for cpu-release-addr, and thus potentially face the same
>> situation, depending on what the final decision is on the proper size for cpu-
>> release-addr. As of v4.17, a git grep shows one cell values in:
>>
>>   arch/arm/boot/dts/axm5516-cpus.dtsi
>>   arch/arm/boot/dts/stih407-family.dtsi
>>   arch/arm/boot/dts/stih418.dtsi
>
> Yes, I had grepped before I used 32 bits on mine...
>
> Now, what is the decision here? Our bootloader is already modified to set it to 32 bits, so I propose that

And too late to fix the bootloader?

>
> + I change the driver to handle 32 and 64 bits properties

That's fine if you can't fix the bootloader.

> + I add this to the cpu.txt, as a separate patch:
> # On other systems, the property can be either
>   32 bits or 64 bits, it is the driver's responsibility
>   to deal with either sizes.

That is definitely not what we want to say. Use of 32-bit should be
considered out of spec. Yes, we have a few platforms in that category,
but they already handle that themselves. Would be nice to fix them,
but at least the STi platforms don't seem too active.

IMO, we should delete whatever text we can here and at most just refer
to the spec.

Rob

RE: [PATCH v4 2/3] arm: shmobile: Add the R9A06G032 SMP enabler driver

[Michel Pollet]

On 07 June 2018 16:55, Rob wrote:
>
> On Thu, Jun 7, 2018 at 1:59 AM, Michel Pollet
> <michel.pollet@bp.renesas.com> wrote:
> > On 06 June 2018 22:53, Frank wrote:
> >> On 06/06/18 14:48, Frank Rowand wrote:
> >> > On 06/05/18 23:36, Michel Pollet wrote:
> >> >> Hi Frank,
> >> >>
> >> >> On 05 June 2018 18:34, Frank wrote:
> >> >>> On 06/05/18 04:28, Michel Pollet wrote:
> >> >>>> The Renesas R9A06G032 second CA7 is parked in a ROM pen at boot
> >> >>>> time, it requires a special enable method to get it started.
>
> [...]
>
> >> >>>> + * The second CPU is parked in ROM at boot time. It requires
> >> >>>> +waking it after
> >> >>>> + * writing an address into the BOOTADDR register of sysctrl.
> >> >>>> + *
> >> >>>> + * So the default value of the "cpu-release-addr" corresponds
> >> >>>> +to
> >> >>> BOOTADDR...
> >> >>>> + *
> >> >>>> + * *However* the BOOTADDR register is not available when the
> >> >>>> +kernel
> >> >>>> + * starts in NONSEC mode.
> >> >>>> + *
> >> >>>> + * So for NONSEC mode, the bootloader re-parks the second CPU
> >> >>>> +into a pen
> >> >>>> + * in SRAM, and changes the "cpu-release-addr" of linux's DT to
> >> >>>> +a SRAM address,
> >> >>>> + * which is not restricted.
> >> >>>
> >> >>> The binding document for cpu-release-addr does not have a
> >> >>> definition for 32 bit arm.  The existing definition is only 64
> >> >>> bit arm.  Please add the definition for 32 bit arm to patch 1.
> >> >>
> >> >> Hmmm I do find a definition in
> >> >> Documentation/devicetree/bindings/arm/cpus.txt -- just under where
> >> >> I added my 'enable-method' -- And it is already used as 32 bits in
> >> >> at least arch/arm/boot/dts/stih407-family.dtsi.
> >> >
> >> > If the correct answer is for cpu-release-addr to be 64 bits in
> >> > certain cases (that discussion is ongoing further downthread) then
> >> > one approach to maintain compatibility _and_ to fix the devicetree
> >> > source files is to change the source code that currently gets
> >> > cpu-release-addr as a
> >> > 32 bit object to check the size of the property and get it as
> >> > either a
> >> > 32 bit or 64 bit object, based on the actual size of the property
> >> > in the device tree and then change the value in the devicetree
> >> > source files to be two cells.  BUT this does not consider the
> >> > bootloader complication.  arch/arm/boot/dts/axm5516-cpus.dtsi has a
> >> > note "// Fixed by the boot loader", so the boot loader also has to
> >> > be modified to be able to handle the possibility that the property
> >> > could be either
> >> > 32 bits or 64 bits.  I don't know how to maintain compatibility
> >> > with the boot loader since we can't force it to change
> >> > synchronously with changes in the kernel.
> >> >
> >> > You can consider this comment to be a drive-by observation.  I
> >> > think Rob and Geert and people like that are likely to be more
> >> > helpful with what to actually do, and you can treat my comment more
> >> > as pointing out the issue than as providing the perfect solution.
> >>
> >> Darn it, hit <send> too quickly.
> >>
> >> I meant to mention that there are several devicetree source files
> >> that have a single cell value for cpu-release-addr, and thus
> >> potentially face the same situation, depending on what the final
> >> decision is on the proper size for cpu- release-addr. As of v4.17, a git grep
> shows one cell values in:
> >>
> >>   arch/arm/boot/dts/axm5516-cpus.dtsi
> >>   arch/arm/boot/dts/stih407-family.dtsi
> >>   arch/arm/boot/dts/stih418.dtsi
> >
> > Yes, I had grepped before I used 32 bits on mine...
> >
> > Now, what is the decision here? Our bootloader is already modified to
> > set it to 32 bits, so I propose that
>
> And too late to fix the bootloader?


Well not too late, but read further on...

>
> >
> > + I change the driver to handle 32 and 64 bits properties
>
> That's fine if you can't fix the bootloader.
>
> > + I add this to the cpu.txt, as a separate patch:
> > # On other systems, the property can be either
> >   32 bits or 64 bits, it is the driver's responsibility
> >   to deal with either sizes.
>
> That is definitely not what we want to say. Use of 32-bit should be
> considered out of spec. Yes, we have a few platforms in that category, but
> they already handle that themselves. Would be nice to fix them, but at least
> the STi platforms don't seem too active.
>
> IMO, we should delete whatever text we can here and at most just refer to
> the spec.

So actually I didn't use 32 bits by plain chance, I read the cpu.txt file which says
that 64 bits systems use 64 bits property, concluded that in my case I ought to
use 32 bits, then grepped around and found other systems using 32 bits, therefore
I went forward and used it..

Nothing said here that it should be 64 bits everywhere -- So the documentation
needs fixing somehow. Right now it certainly led me wrong.

>
> Rob

Michel




Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

Re: [PATCH v4 2/3] arm: shmobile: Add the R9A06G032 SMP enabler driver

[Geert Uytterhoeven]

On Fri, Jun 8, 2018 at 8:50 AM, Michel Pollet
<michel.pollet@bp.renesas.com> wrote:
> On 07 June 2018 16:55, Rob wrote:
>> On Thu, Jun 7, 2018 at 1:59 AM, Michel Pollet
>> <michel.pollet@bp.renesas.com> wrote:
>> > On 06 June 2018 22:53, Frank wrote:
>> >> On 06/06/18 14:48, Frank Rowand wrote:
>> >> > On 06/05/18 23:36, Michel Pollet wrote:
>> >> >> On 05 June 2018 18:34, Frank wrote:
>> >> >>> On 06/05/18 04:28, Michel Pollet wrote:
>> >> >>>> The Renesas R9A06G032 second CA7 is parked in a ROM pen at boot
>> >> >>>> time, it requires a special enable method to get it started.
>>
>> [...]
>>
>> >> >>>> + * The second CPU is parked in ROM at boot time. It requires
>> >> >>>> +waking it after
>> >> >>>> + * writing an address into the BOOTADDR register of sysctrl.
>> >> >>>> + *
>> >> >>>> + * So the default value of the "cpu-release-addr" corresponds
>> >> >>>> +to
>> >> >>> BOOTADDR...
>> >> >>>> + *
>> >> >>>> + * *However* the BOOTADDR register is not available when the
>> >> >>>> +kernel
>> >> >>>> + * starts in NONSEC mode.
>> >> >>>> + *
>> >> >>>> + * So for NONSEC mode, the bootloader re-parks the second CPU
>> >> >>>> +into a pen
>> >> >>>> + * in SRAM, and changes the "cpu-release-addr" of linux's DT to
>> >> >>>> +a SRAM address,
>> >> >>>> + * which is not restricted.
>> >> >>>
>> >> >>> The binding document for cpu-release-addr does not have a
>> >> >>> definition for 32 bit arm.  The existing definition is only 64
>> >> >>> bit arm.  Please add the definition for 32 bit arm to patch 1.
>> >> >>
>> >> >> Hmmm I do find a definition in
>> >> >> Documentation/devicetree/bindings/arm/cpus.txt -- just under where
>> >> >> I added my 'enable-method' -- And it is already used as 32 bits in
>> >> >> at least arch/arm/boot/dts/stih407-family.dtsi.
>> >> >
>> >> > If the correct answer is for cpu-release-addr to be 64 bits in
>> >> > certain cases (that discussion is ongoing further downthread) then
>> >> > one approach to maintain compatibility _and_ to fix the devicetree
>> >> > source files is to change the source code that currently gets
>> >> > cpu-release-addr as a
>> >> > 32 bit object to check the size of the property and get it as
>> >> > either a
>> >> > 32 bit or 64 bit object, based on the actual size of the property
>> >> > in the device tree and then change the value in the devicetree
>> >> > source files to be two cells.  BUT this does not consider the
>> >> > bootloader complication.  arch/arm/boot/dts/axm5516-cpus.dtsi has a
>> >> > note "// Fixed by the boot loader", so the boot loader also has to
>> >> > be modified to be able to handle the possibility that the property
>> >> > could be either
>> >> > 32 bits or 64 bits.  I don't know how to maintain compatibility
>> >> > with the boot loader since we can't force it to change
>> >> > synchronously with changes in the kernel.
>> >> >
>> >> > You can consider this comment to be a drive-by observation.  I
>> >> > think Rob and Geert and people like that are likely to be more
>> >> > helpful with what to actually do, and you can treat my comment more
>> >> > as pointing out the issue than as providing the perfect solution.
>> >>
>> >> Darn it, hit <send> too quickly.
>> >>
>> >> I meant to mention that there are several devicetree source files
>> >> that have a single cell value for cpu-release-addr, and thus
>> >> potentially face the same situation, depending on what the final
>> >> decision is on the proper size for cpu- release-addr. As of v4.17, a git grep
>> shows one cell values in:
>> >>
>> >>   arch/arm/boot/dts/axm5516-cpus.dtsi
>> >>   arch/arm/boot/dts/stih407-family.dtsi
>> >>   arch/arm/boot/dts/stih418.dtsi
>> >
>> > Yes, I had grepped before I used 32 bits on mine...
>> >
>> > Now, what is the decision here? Our bootloader is already modified to
>> > set it to 32 bits, so I propose that
>>
>> And too late to fix the bootloader?
>
>
> Well not too late, but read further on...
>
>>
>> >
>> > + I change the driver to handle 32 and 64 bits properties
>>
>> That's fine if you can't fix the bootloader.
>>
>> > + I add this to the cpu.txt, as a separate patch:
>> > # On other systems, the property can be either
>> >   32 bits or 64 bits, it is the driver's responsibility
>> >   to deal with either sizes.
>>
>> That is definitely not what we want to say. Use of 32-bit should be
>> considered out of spec. Yes, we have a few platforms in that category, but
>> they already handle that themselves. Would be nice to fix them, but at least
>> the STi platforms don't seem too active.
>>
>> IMO, we should delete whatever text we can here and at most just refer to
>> the spec.
>
> So actually I didn't use 32 bits by plain chance, I read the cpu.txt file which says
> that 64 bits systems use 64 bits property, concluded that in my case I ought to
> use 32 bits, then grepped around and found other systems using 32 bits, therefore
> I went forward and used it..
>
> Nothing said here that it should be 64 bits everywhere -- So the documentation
> needs fixing somehow. Right now it certainly led me wrong.

Perhaps we should add to Documentation/devicetree/bindings/ the standard
bindings from ePAPR and successors, too?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v4 2/3] arm: shmobile: Add the R9A06G032 SMP enabler driver

[Rob Herring]

On Fri, Jun 8, 2018 at 2:47 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Fri, Jun 8, 2018 at 8:50 AM, Michel Pollet
> <michel.pollet@bp.renesas.com> wrote:
>> On 07 June 2018 16:55, Rob wrote:
>>> On Thu, Jun 7, 2018 at 1:59 AM, Michel Pollet
>>> <michel.pollet@bp.renesas.com> wrote:
>>> > On 06 June 2018 22:53, Frank wrote:
>>> >> On 06/06/18 14:48, Frank Rowand wrote:
>>> >> > On 06/05/18 23:36, Michel Pollet wrote:
>>> >> >> On 05 June 2018 18:34, Frank wrote:
>>> >> >>> On 06/05/18 04:28, Michel Pollet wrote:
>>> >> >>>> The Renesas R9A06G032 second CA7 is parked in a ROM pen at boot
>>> >> >>>> time, it requires a special enable method to get it started.
>>>
>>> [...]
>>>
>>> >> >>>> + * The second CPU is parked in ROM at boot time. It requires
>>> >> >>>> +waking it after
>>> >> >>>> + * writing an address into the BOOTADDR register of sysctrl.
>>> >> >>>> + *
>>> >> >>>> + * So the default value of the "cpu-release-addr" corresponds
>>> >> >>>> +to
>>> >> >>> BOOTADDR...
>>> >> >>>> + *
>>> >> >>>> + * *However* the BOOTADDR register is not available when the
>>> >> >>>> +kernel
>>> >> >>>> + * starts in NONSEC mode.
>>> >> >>>> + *
>>> >> >>>> + * So for NONSEC mode, the bootloader re-parks the second CPU
>>> >> >>>> +into a pen
>>> >> >>>> + * in SRAM, and changes the "cpu-release-addr" of linux's DT to
>>> >> >>>> +a SRAM address,
>>> >> >>>> + * which is not restricted.
>>> >> >>>
>>> >> >>> The binding document for cpu-release-addr does not have a
>>> >> >>> definition for 32 bit arm.  The existing definition is only 64
>>> >> >>> bit arm.  Please add the definition for 32 bit arm to patch 1.
>>> >> >>
>>> >> >> Hmmm I do find a definition in
>>> >> >> Documentation/devicetree/bindings/arm/cpus.txt -- just under where
>>> >> >> I added my 'enable-method' -- And it is already used as 32 bits in
>>> >> >> at least arch/arm/boot/dts/stih407-family.dtsi.
>>> >> >
>>> >> > If the correct answer is for cpu-release-addr to be 64 bits in
>>> >> > certain cases (that discussion is ongoing further downthread) then
>>> >> > one approach to maintain compatibility _and_ to fix the devicetree
>>> >> > source files is to change the source code that currently gets
>>> >> > cpu-release-addr as a
>>> >> > 32 bit object to check the size of the property and get it as
>>> >> > either a
>>> >> > 32 bit or 64 bit object, based on the actual size of the property
>>> >> > in the device tree and then change the value in the devicetree
>>> >> > source files to be two cells.  BUT this does not consider the
>>> >> > bootloader complication.  arch/arm/boot/dts/axm5516-cpus.dtsi has a
>>> >> > note "// Fixed by the boot loader", so the boot loader also has to
>>> >> > be modified to be able to handle the possibility that the property
>>> >> > could be either
>>> >> > 32 bits or 64 bits.  I don't know how to maintain compatibility
>>> >> > with the boot loader since we can't force it to change
>>> >> > synchronously with changes in the kernel.
>>> >> >
>>> >> > You can consider this comment to be a drive-by observation.  I
>>> >> > think Rob and Geert and people like that are likely to be more
>>> >> > helpful with what to actually do, and you can treat my comment more
>>> >> > as pointing out the issue than as providing the perfect solution.
>>> >>
>>> >> Darn it, hit <send> too quickly.
>>> >>
>>> >> I meant to mention that there are several devicetree source files
>>> >> that have a single cell value for cpu-release-addr, and thus
>>> >> potentially face the same situation, depending on what the final
>>> >> decision is on the proper size for cpu- release-addr. As of v4.17, a git grep
>>> shows one cell values in:
>>> >>
>>> >>   arch/arm/boot/dts/axm5516-cpus.dtsi
>>> >>   arch/arm/boot/dts/stih407-family.dtsi
>>> >>   arch/arm/boot/dts/stih418.dtsi
>>> >
>>> > Yes, I had grepped before I used 32 bits on mine...
>>> >
>>> > Now, what is the decision here? Our bootloader is already modified to
>>> > set it to 32 bits, so I propose that
>>>
>>> And too late to fix the bootloader?
>>
>>
>> Well not too late, but read further on...
>>
>>>
>>> >
>>> > + I change the driver to handle 32 and 64 bits properties
>>>
>>> That's fine if you can't fix the bootloader.
>>>
>>> > + I add this to the cpu.txt, as a separate patch:
>>> > # On other systems, the property can be either
>>> >   32 bits or 64 bits, it is the driver's responsibility
>>> >   to deal with either sizes.
>>>
>>> That is definitely not what we want to say. Use of 32-bit should be
>>> considered out of spec. Yes, we have a few platforms in that category, but
>>> they already handle that themselves. Would be nice to fix them, but at least
>>> the STi platforms don't seem too active.
>>>
>>> IMO, we should delete whatever text we can here and at most just refer to
>>> the spec.
>>
>> So actually I didn't use 32 bits by plain chance, I read the cpu.txt file which says
>> that 64 bits systems use 64 bits property, concluded that in my case I ought to
>> use 32 bits, then grepped around and found other systems using 32 bits, therefore
>> I went forward and used it..
>>
>> Nothing said here that it should be 64 bits everywhere -- So the documentation
>> needs fixing somehow. Right now it certainly led me wrong.
>
> Perhaps we should add to Documentation/devicetree/bindings/ the standard
> bindings from ePAPR and successors, too?

I hope you mean *reference* here, not duplicate the bindings here. We
want to move in the other direction and move the common bindings out
of the kernel and into the spec.

The real solution here is validation which I'm working on. I had
already converted cpus.txt. Here's an example of the results of the
validation:

arch/arm/boot/dts/stih410-b2120.dt.yaml:1962:7: cpu@0: 'enable-method'
is a dependency of 'cpu-release-addr'
arch/arm/boot/dts/stih410-b2120.dt.yaml:1965:26:
cpu@0:cpu-release-addr: [155254948] is too short

The schema is up on my yaml-bindings branch. Will send out more details soon.

Rob

Re: [PATCH v4 2/3] arm: shmobile: Add the R9A06G032 SMP enabler driver

[Geert Uytterhoeven]

Hi Rob,

On Fri, Jun 8, 2018 at 10:41 PM Rob Herring <robh+dt@kernel.org> wrote:
> On Fri, Jun 8, 2018 at 2:47 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Fri, Jun 8, 2018 at 8:50 AM, Michel Pollet
> > <michel.pollet@bp.renesas.com> wrote:
> >>> > + I add this to the cpu.txt, as a separate patch:
> >>> > # On other systems, the property can be either
> >>> >   32 bits or 64 bits, it is the driver's responsibility
> >>> >   to deal with either sizes.
> >>>
> >>> That is definitely not what we want to say. Use of 32-bit should be
> >>> considered out of spec. Yes, we have a few platforms in that category, but
> >>> they already handle that themselves. Would be nice to fix them, but at least
> >>> the STi platforms don't seem too active.
> >>>
> >>> IMO, we should delete whatever text we can here and at most just refer to
> >>> the spec.
> >>
> >> So actually I didn't use 32 bits by plain chance, I read the cpu.txt file which says
> >> that 64 bits systems use 64 bits property, concluded that in my case I ought to
> >> use 32 bits, then grepped around and found other systems using 32 bits, therefore
> >> I went forward and used it..
> >>
> >> Nothing said here that it should be 64 bits everywhere -- So the documentation
> >> needs fixing somehow. Right now it certainly led me wrong.
> >
> > Perhaps we should add to Documentation/devicetree/bindings/ the standard
> > bindings from ePAPR and successors, too?
>
> I hope you mean *reference* here, not duplicate the bindings here. We
> want to move in the other direction and move the common bindings out
> of the kernel and into the spec.

I did mean copy... I usually grep in Documentation/devicetree/bindings/,
and fall back to the spec only rarely, mostly because the spec usually
doesn't cover what I need.

I am aware of the ongoing work on updating the spec. I guess it's a
chicken-and-egg problem...

A list of standardized properties under Documentation/devicetree/bindings/,
referring to the spec may be a good interim solution. So at least it would show
it with git grep.

> The real solution here is validation which I'm working on. I had
> already converted cpus.txt. Here's an example of the results of the
> validation:
>
> arch/arm/boot/dts/stih410-b2120.dt.yaml:1962:7: cpu@0: 'enable-method'
> is a dependency of 'cpu-release-addr'
> arch/arm/boot/dts/stih410-b2120.dt.yaml:1965:26:
> cpu@0:cpu-release-addr: [155254948] is too short

Thanks, nice!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds