| Message ID | 1528198148-23308-3-git-send-email-michel.pollet@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Simon Horman |
| Headers | show |
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
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); >
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.
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. >
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.
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?
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
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
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
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. >
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. >
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. >> > >
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.
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
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.
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
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
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
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);
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