Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
On Wednesday 19 of June 2013 11:19:30 Nicolas Pitre wrote:
> On Wed, 19 Jun 2013, Tomasz Figa wrote:
> > On Wednesday 19 of June 2013 20:26:50 Chander Kashyap wrote:
> > > On 19 June 2013 19:58, Tomasz Figa wrote:
> > > > I mean, calculate register offset based on two parameters - cluster
> > > > ID
> > > > and>
> > > >
> > > > CPU ID, like:
> > > > ...
> > > >
> > > > u32 mpidr = cpu_logical_map(cpu);
> > > > u32 phys_cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> > > >
> > > > if (soc_is_exynos()) {
> > > >
> > > > u32 cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> > > >
> > > > phys_cpu += EXYNOS_CPUS_PER_CLUSTER * cluster;
> > > >
> > > > }
> > > >
> > > > reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
> > > > __raw_writel(0, reg_base);
> > >
> > > This does not seems to viable solution, as eg. clusterID for
> > > exynos4210 is 0x9 and exynos 4412 is 0xa.
> >
> > We don't need to consider cluster ID for any SoC that has just one
> > cluster. That's why there is the if (soc_is_exynos()) clause,
> > where exynos is the SoC that we support and has more clusters.
> >
> > > But if we wass the cpu nodes
> > > thru DT, the we can comfortably rely on the logical cpu number. Also
> > > EXYNOS_CPUS_PER_CLUSTER can vary from cluster to cluster.
> >
> > There is nothing that prevents you from specifying the CPUs in DT in
> > different order. Moreover, even if you specify them in correct order,
> > there is nothing that prevents you from using any of the listed CPUs
> > as boot CPU, which will get the logical ID of 0.
>
> Relying on the logical CPU number to index into hardware related
> register space is wrong, please don't do that.
>
> If the MPIDR allocation isn't linear then this cannot be used either.
>
> The best solution is probably to add this reg_base as a property of each
> CPU node in DT, and extract it at boot time to stash it into an array
> which can be indexed with the logical CPU number afterwards.
Currently we don't specify the cpu node in DT at all for Exynos 4210, 4212
and 4412. It's sure that their device tree sources need to be modified to
correct his, but since DT is considered an ABI, we should keep
compatibility with old device tree blobs.
This means that we can't strictly require this property to be present in
device tree, because it will break existing boards with older device trees.
So my suggestion is to user cluster ID and CPU ID to calculate the offset by
default and add possibility to override it with a property in device tree
if such need shows up.
What do you think?
Best regards,
Tomasz
> Nicolas
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-samsung-soc" in the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
On Wednesday 19 of June 2013 11:19:30 Nicolas Pitre wrote:
On Wed, 19 Jun 2013, Tomasz Figa wrote:
On Wednesday 19 of June 2013 20:26:50 Chander Kashyap wrote:
On 19 June 2013 19:58, Tomasz Figa t.f...@samsung.com wrote:
I mean, calculate register offset based on two parameters - cluster
ID
and
CPU ID, like:
...
u32 mpidr = cpu_logical_map(cpu);
u32 phys_cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
if (soc_is_exynos()) {
u32 cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
phys_cpu += EXYNOS_CPUS_PER_CLUSTER * cluster;
}
reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
__raw_writel(0, reg_base);
This does not seems to viable solution, as eg. clusterID for
exynos4210 is 0x9 and exynos 4412 is 0xa.
We don't need to consider cluster ID for any SoC that has just one
cluster. That's why there is the if (soc_is_exynos()) clause,
where exynos is the SoC that we support and has more clusters.
But if we wass the cpu nodes
thru DT, the we can comfortably rely on the logical cpu number. Also
EXYNOS_CPUS_PER_CLUSTER can vary from cluster to cluster.
There is nothing that prevents you from specifying the CPUs in DT in
different order. Moreover, even if you specify them in correct order,
there is nothing that prevents you from using any of the listed CPUs
as boot CPU, which will get the logical ID of 0.
Relying on the logical CPU number to index into hardware related
register space is wrong, please don't do that.
If the MPIDR allocation isn't linear then this cannot be used either.
The best solution is probably to add this reg_base as a property of each
CPU node in DT, and extract it at boot time to stash it into an array
which can be indexed with the logical CPU number afterwards.
Currently we don't specify the cpu node in DT at all for Exynos 4210, 4212
and 4412. It's sure that their device tree sources need to be modified to
correct his, but since DT is considered an ABI, we should keep
compatibility with old device tree blobs.
This means that we can't strictly require this property to be present in
device tree, because it will break existing boards with older device trees.
So my suggestion is to user cluster ID and CPU ID to calculate the offset by
default and add possibility to override it with a property in device tree
if such need shows up.
What do you think?
Best regards,
Tomasz
Nicolas
--
To unsubscribe from this list: send the line unsubscribe
linux-samsung-soc in the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
On Wed, 19 Jun 2013, Tomasz Figa wrote:
> On Wednesday 19 of June 2013 20:26:50 Chander Kashyap wrote:
> > On 19 June 2013 19:58, Tomasz Figa wrote:
> > > I mean, calculate register offset based on two parameters - cluster ID
> > > and>
> > > CPU ID, like:
> > > ...
> > >
> > > u32 mpidr = cpu_logical_map(cpu);
> > > u32 phys_cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> > >
> > > if (soc_is_exynos()) {
> > >
> > > u32 cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> > >
> > > phys_cpu += EXYNOS_CPUS_PER_CLUSTER * cluster;
> > >
> > > }
> > >
> > > reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
> > > __raw_writel(0, reg_base);
> >
> > This does not seems to viable solution, as eg. clusterID for
> > exynos4210 is 0x9 and exynos 4412 is 0xa.
>
> We don't need to consider cluster ID for any SoC that has just one cluster.
> That's why there is the if (soc_is_exynos()) clause, where exynos
> is the SoC that we support and has more clusters.
>
> > But if we wass the cpu nodes
> > thru DT, the we can comfortably rely on the logical cpu number. Also
> > EXYNOS_CPUS_PER_CLUSTER can vary from cluster to cluster.
>
> There is nothing that prevents you from specifying the CPUs in DT in
> different order. Moreover, even if you specify them in correct order, there
> is nothing that prevents you from using any of the listed CPUs as boot CPU,
> which will get the logical ID of 0.
Relying on the logical CPU number to index into hardware related
register space is wrong, please don't do that.
If the MPIDR allocation isn't linear then this cannot be used either.
The best solution is probably to add this reg_base as a property of each
CPU node in DT, and extract it at boot time to stash it into an array
which can be indexed with the logical CPU number afterwards.
Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
On 19 June 2013 20:31, Tomasz Figa wrote:
> On Wednesday 19 of June 2013 20:26:50 Chander Kashyap wrote:
>> On 19 June 2013 19:58, Tomasz Figa wrote:
>> > On Wednesday 19 of June 2013 19:25:27 Chander Kashyap wrote:
>> >> On 19 June 2013 19:19, Tomasz Figa wrote:
>> >> > On Wednesday 19 of June 2013 14:24:17 Lorenzo Pieralisi wrote:
>> >> >> On Wed, Jun 19, 2013 at 01:50:57PM +0100, Tomasz Figa wrote:
>> >> >> > On Wednesday 19 of June 2013 17:39:21 Chander Kashyap wrote:
>> >> >> > > On 18 June 2013 23:29, Kukjin Kim
> wrote:
>> >> >> > > > On 06/19/13 02:45, Tomasz Figa wrote:
>> >> >> > > >> Ccing Arnd and Olof, because I forgot to add them to git
>> >> >> > > >> send-email...
>> >> >> > > >>
>> >> >> > > >> Sorry for the noise.
>> >> >> > > >>
>> >> >> > > >> On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
>> >> >> > > >>> S5P_ARM_CORE1_* registers affect only core 1. To control
>> >> >> > > >>> further
>> >> >> > > >>> cores
>> >> >> > > >>> properly another registers must be used.
>> >> >> > > >>>
>> >> >> > > >>> This patch replaces S5P_ARM_CORE1_* register definitions
>> >> >> > > >>> with
>> >> >> > > >>> S5P_ARM_CORE_*(x) macro which return addresses of registers
>> >> >> > > >>> for
>> >> >> > > >>> specified core.
>> >> >> > > >>>
>> >> >> > > >>> This fixes CPU hotplug on quad core Exynos SoCs on which
>> >> >> > > >>> currently
>> >> >> > > >>> offlining CPUs 2 or 3 caused CPU 1 to be turned off.
>> >> >> > > >>
>> >> >> > > >> Obviously this doesn't happen currently because of the if
>> >> >> > > >> (cpu
>> >> >> > > >> ==
>> >> >> > > >> 1),
>> >> >> > > >> but>
>> >> >> > > >
>> >> >> > > > Yes, not happened...and just note exynos5440 doesn't support
>> >> >> > > > hotplug :)
>> >> >> > > > so this is available on exynos4412 and added 5420.
>> >> >> > > >
>> >> >> > > >> if logical cpu1 turned out not to be physical cpu1, then it
>> >> >> > > >> would
>> >> >> > > >> crash.
>> >> >> > > >>
>> >> >> > > >> Best regards,
>> >> >> > > >> Tomasz
>> >> >> > > >>
>> >> >> > > >>> In addition,
>> >> >> > > >>> bring-up of CPU 2 and 3 is fixed on boards where bootloader
>> >> >> > > >>> powers
>> >> >> > > >>> off
>> >> >> > > >>> secondary cores by default.
>> >> >> > > >
>> >> >> > > > I need to test on board about above...
>> >> >> > > >
>> >> >> > > >>> Cc: sta...@vger.kernel.org
>> >> >> > > >>> Signed-off-by: Tomasz Figa
>> >> >> > > >>> Signed-off-by: Kyungmin Park
>> >> >> > > >>> ---
>> >> >> > > >>>
>> >> >> > > >>> arch/arm/mach-exynos/hotplug.c | 9
>> >> >> > > >>> +
>> >> >> > > >>> arch/arm/mach-exynos/include/mach/regs-pmu.h | 10
>> >> >> > > >>> +++---
>> >> >> > > >>> arch/arm/mach-exynos/platsmp.c | 9
>> >> >> > > >>> +
>> >> >> > > >>> 3 files changed, 17 insertions(+), 11 deletions(-)
>> >> >> > > >>>
>> >> >> > > >>> diff --git a/arch/arm/mach-exynos/hotplug.c
>> >> >> > > >>> b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943
>> >> >> > > >>> 100644
>> >> >> > > >>> --- a/arch/arm/mach-exynos/hotplug.c
>> >> >> > > >>> +++ b/arch/arm/mach-exynos/hotplug.c
>> >> >> > > >>> @@ -93,10 +93,11 @@ static inline void
>> >> >> > > >>> cpu_leave_lowpower(void)
>> >> >> > > >>>
>> >> >> > > >>> static inline void platform_do_lowpower(unsigned int cpu,
>> >> >> > > >>> int
>> >> >> > > >>>
>> >> >> > > >>> *spurious) {
>> >> >> > > >>>
>> >> >> > > >>> for (;;) {
>> >> >> > > >>>
>> >> >> > > >>> + void __iomem *reg_base;
>> >> >> > > >>> + unsigned int phys_cpu =
>> >> >> > > >>> cpu_logical_map(cpu);
>> >> >> > > >>>
>> >> >> > > >>> - /* make cpu1 to be turned off at next WFI
>> >> >> > > >>> command
>> >> >> > > >>> */
>> >> >> > > >>> - if (cpu == 1)
>> >> >> > > >>> - __raw_writel(0,
>> >> >> > > >>> S5P_ARM_CORE1_CONFIGURATION);
>> >> >> > > >>> + reg_base =
>> >> >> > > >>> S5P_ARM_CORE_CONFIGURATION(phys_cpu);
>> >> >> > >
>> >> >> > > Tomasz,
>> >> >> > > This will break for non-zero, MPIDR value. Say if MPIDR is 1
>> >> >> > > then
>> >> >> > > for
>> >> >> > > cpu0 phys_cpu value will be 0x100,
>> >> >> > > and address calculation will be (S5P_ARM_CORE0_CONFIGURATION
>> >> >> > > +
>> >> >> > > ((0x101) * 0x80)), which is wrong.
>> >> >>
>> >> >> Honestly, I did not understand the reasoning above, please clarify.
>> >> >>
>> >> >> > Hmm, according to the code initializing __cpu_logical_map[] array
>> >> >> > this
>> >> >> > is not true.
>> >> >> >
>> >> >> > Here's the code:
>> >> >> >
>> >> >> > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/
>> >> >> > tre
>> >> >> > e/a
>> >> >> > rch/arm/kernel/setup.c?id=refs/tags/next-20130619#n468
>> >> >> >
>> >> >> > and for used macros and bitmasks:
>> >> >> >
>> >> >> > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/
>> >> >> > tre
>> >> >> > e/a
>> >> >> >
Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
On Wednesday 19 of June 2013 20:26:50 Chander Kashyap wrote:
> On 19 June 2013 19:58, Tomasz Figa wrote:
> > On Wednesday 19 of June 2013 19:25:27 Chander Kashyap wrote:
> >> On 19 June 2013 19:19, Tomasz Figa wrote:
> >> > On Wednesday 19 of June 2013 14:24:17 Lorenzo Pieralisi wrote:
> >> >> On Wed, Jun 19, 2013 at 01:50:57PM +0100, Tomasz Figa wrote:
> >> >> > On Wednesday 19 of June 2013 17:39:21 Chander Kashyap wrote:
> >> >> > > On 18 June 2013 23:29, Kukjin Kim
wrote:
> >> >> > > > On 06/19/13 02:45, Tomasz Figa wrote:
> >> >> > > >> Ccing Arnd and Olof, because I forgot to add them to git
> >> >> > > >> send-email...
> >> >> > > >>
> >> >> > > >> Sorry for the noise.
> >> >> > > >>
> >> >> > > >> On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
> >> >> > > >>> S5P_ARM_CORE1_* registers affect only core 1. To control
> >> >> > > >>> further
> >> >> > > >>> cores
> >> >> > > >>> properly another registers must be used.
> >> >> > > >>>
> >> >> > > >>> This patch replaces S5P_ARM_CORE1_* register definitions
> >> >> > > >>> with
> >> >> > > >>> S5P_ARM_CORE_*(x) macro which return addresses of registers
> >> >> > > >>> for
> >> >> > > >>> specified core.
> >> >> > > >>>
> >> >> > > >>> This fixes CPU hotplug on quad core Exynos SoCs on which
> >> >> > > >>> currently
> >> >> > > >>> offlining CPUs 2 or 3 caused CPU 1 to be turned off.
> >> >> > > >>
> >> >> > > >> Obviously this doesn't happen currently because of the if
> >> >> > > >> (cpu
> >> >> > > >> ==
> >> >> > > >> 1),
> >> >> > > >> but>
> >> >> > > >
> >> >> > > > Yes, not happened...and just note exynos5440 doesn't support
> >> >> > > > hotplug :)
> >> >> > > > so this is available on exynos4412 and added 5420.
> >> >> > > >
> >> >> > > >> if logical cpu1 turned out not to be physical cpu1, then it
> >> >> > > >> would
> >> >> > > >> crash.
> >> >> > > >>
> >> >> > > >> Best regards,
> >> >> > > >> Tomasz
> >> >> > > >>
> >> >> > > >>> In addition,
> >> >> > > >>> bring-up of CPU 2 and 3 is fixed on boards where bootloader
> >> >> > > >>> powers
> >> >> > > >>> off
> >> >> > > >>> secondary cores by default.
> >> >> > > >
> >> >> > > > I need to test on board about above...
> >> >> > > >
> >> >> > > >>> Cc: sta...@vger.kernel.org
> >> >> > > >>> Signed-off-by: Tomasz Figa
> >> >> > > >>> Signed-off-by: Kyungmin Park
> >> >> > > >>> ---
> >> >> > > >>>
> >> >> > > >>> arch/arm/mach-exynos/hotplug.c | 9
> >> >> > > >>> +
> >> >> > > >>> arch/arm/mach-exynos/include/mach/regs-pmu.h | 10
> >> >> > > >>> +++---
> >> >> > > >>> arch/arm/mach-exynos/platsmp.c | 9
> >> >> > > >>> +
> >> >> > > >>> 3 files changed, 17 insertions(+), 11 deletions(-)
> >> >> > > >>>
> >> >> > > >>> diff --git a/arch/arm/mach-exynos/hotplug.c
> >> >> > > >>> b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943
> >> >> > > >>> 100644
> >> >> > > >>> --- a/arch/arm/mach-exynos/hotplug.c
> >> >> > > >>> +++ b/arch/arm/mach-exynos/hotplug.c
> >> >> > > >>> @@ -93,10 +93,11 @@ static inline void
> >> >> > > >>> cpu_leave_lowpower(void)
> >> >> > > >>>
> >> >> > > >>> static inline void platform_do_lowpower(unsigned int cpu,
> >> >> > > >>> int
> >> >> > > >>>
> >> >> > > >>> *spurious) {
> >> >> > > >>>
> >> >> > > >>> for (;;) {
> >> >> > > >>>
> >> >> > > >>> + void __iomem *reg_base;
> >> >> > > >>> + unsigned int phys_cpu =
> >> >> > > >>> cpu_logical_map(cpu);
> >> >> > > >>>
> >> >> > > >>> - /* make cpu1 to be turned off at next WFI
> >> >> > > >>> command
> >> >> > > >>> */
> >> >> > > >>> - if (cpu == 1)
> >> >> > > >>> - __raw_writel(0,
> >> >> > > >>> S5P_ARM_CORE1_CONFIGURATION);
> >> >> > > >>> + reg_base =
> >> >> > > >>> S5P_ARM_CORE_CONFIGURATION(phys_cpu);
> >> >> > >
> >> >> > > Tomasz,
> >> >> > > This will break for non-zero, MPIDR value. Say if MPIDR is 1
> >> >> > > then
> >> >> > > for
> >> >> > > cpu0 phys_cpu value will be 0x100,
> >> >> > > and address calculation will be (S5P_ARM_CORE0_CONFIGURATION
> >> >> > > +
> >> >> > > ((0x101) * 0x80)), which is wrong.
> >> >>
> >> >> Honestly, I did not understand the reasoning above, please clarify.
> >> >>
> >> >> > Hmm, according to the code initializing __cpu_logical_map[] array
> >> >> > this
> >> >> > is not true.
> >> >> >
> >> >> > Here's the code:
> >> >> >
> >> >> > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/
> >> >> > tre
> >> >> > e/a
> >> >> > rch/arm/kernel/setup.c?id=refs/tags/next-20130619#n468
> >> >> >
> >> >> > and for used macros and bitmasks:
> >> >> >
> >> >> > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/
> >> >> > tre
> >> >> > e/a
> >> >> > rch/arm/include/asm/cputype.h?id=refs/tags/next-20130619#n45
> >> >> >
> >> >> > Now the structure of the MPIDR register:
> >> >> >
> >> >> >
Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
On 19 June 2013 19:58, Tomasz Figa wrote:
> On Wednesday 19 of June 2013 19:25:27 Chander Kashyap wrote:
>> On 19 June 2013 19:19, Tomasz Figa wrote:
>> > On Wednesday 19 of June 2013 14:24:17 Lorenzo Pieralisi wrote:
>> >> On Wed, Jun 19, 2013 at 01:50:57PM +0100, Tomasz Figa wrote:
>> >> > On Wednesday 19 of June 2013 17:39:21 Chander Kashyap wrote:
>> >> > > On 18 June 2013 23:29, Kukjin Kim wrote:
>> >> > > > On 06/19/13 02:45, Tomasz Figa wrote:
>> >> > > >> Ccing Arnd and Olof, because I forgot to add them to git
>> >> > > >> send-email...
>> >> > > >>
>> >> > > >> Sorry for the noise.
>> >> > > >>
>> >> > > >> On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
>> >> > > >>> S5P_ARM_CORE1_* registers affect only core 1. To control
>> >> > > >>> further
>> >> > > >>> cores
>> >> > > >>> properly another registers must be used.
>> >> > > >>>
>> >> > > >>> This patch replaces S5P_ARM_CORE1_* register definitions with
>> >> > > >>> S5P_ARM_CORE_*(x) macro which return addresses of registers
>> >> > > >>> for
>> >> > > >>> specified core.
>> >> > > >>>
>> >> > > >>> This fixes CPU hotplug on quad core Exynos SoCs on which
>> >> > > >>> currently
>> >> > > >>> offlining CPUs 2 or 3 caused CPU 1 to be turned off.
>> >> > > >>
>> >> > > >> Obviously this doesn't happen currently because of the if (cpu
>> >> > > >> ==
>> >> > > >> 1),
>> >> > > >> but>
>> >> > > >
>> >> > > > Yes, not happened...and just note exynos5440 doesn't support
>> >> > > > hotplug :)
>> >> > > > so this is available on exynos4412 and added 5420.
>> >> > > >
>> >> > > >> if logical cpu1 turned out not to be physical cpu1, then it
>> >> > > >> would
>> >> > > >> crash.
>> >> > > >>
>> >> > > >> Best regards,
>> >> > > >> Tomasz
>> >> > > >>
>> >> > > >>> In addition,
>> >> > > >>> bring-up of CPU 2 and 3 is fixed on boards where bootloader
>> >> > > >>> powers
>> >> > > >>> off
>> >> > > >>> secondary cores by default.
>> >> > > >
>> >> > > > I need to test on board about above...
>> >> > > >
>> >> > > >>> Cc: sta...@vger.kernel.org
>> >> > > >>> Signed-off-by: Tomasz Figa
>> >> > > >>> Signed-off-by: Kyungmin Park
>> >> > > >>> ---
>> >> > > >>>
>> >> > > >>> arch/arm/mach-exynos/hotplug.c | 9 +
>> >> > > >>> arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++---
>> >> > > >>> arch/arm/mach-exynos/platsmp.c | 9 +
>> >> > > >>> 3 files changed, 17 insertions(+), 11 deletions(-)
>> >> > > >>>
>> >> > > >>> diff --git a/arch/arm/mach-exynos/hotplug.c
>> >> > > >>> b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
>> >> > > >>> --- a/arch/arm/mach-exynos/hotplug.c
>> >> > > >>> +++ b/arch/arm/mach-exynos/hotplug.c
>> >> > > >>> @@ -93,10 +93,11 @@ static inline void
>> >> > > >>> cpu_leave_lowpower(void)
>> >> > > >>>
>> >> > > >>> static inline void platform_do_lowpower(unsigned int cpu,
>> >> > > >>> int
>> >> > > >>>
>> >> > > >>> *spurious) {
>> >> > > >>>
>> >> > > >>> for (;;) {
>> >> > > >>>
>> >> > > >>> + void __iomem *reg_base;
>> >> > > >>> + unsigned int phys_cpu = cpu_logical_map(cpu);
>> >> > > >>>
>> >> > > >>> - /* make cpu1 to be turned off at next WFI
>> >> > > >>> command
>> >> > > >>> */
>> >> > > >>> - if (cpu == 1)
>> >> > > >>> - __raw_writel(0,
>> >> > > >>> S5P_ARM_CORE1_CONFIGURATION);
>> >> > > >>> + reg_base =
>> >> > > >>> S5P_ARM_CORE_CONFIGURATION(phys_cpu);
>> >> > >
>> >> > > Tomasz,
>> >> > > This will break for non-zero, MPIDR value. Say if MPIDR is 1 then
>> >> > > for
>> >> > > cpu0 phys_cpu value will be 0x100,
>> >> > > and address calculation will be (S5P_ARM_CORE0_CONFIGURATION +
>> >> > > ((0x101) * 0x80)), which is wrong.
>> >>
>> >> Honestly, I did not understand the reasoning above, please clarify.
>> >>
>> >> > Hmm, according to the code initializing __cpu_logical_map[] array
>> >> > this
>> >> > is not true.
>> >> >
>> >> > Here's the code:
>> >> >
>> >> > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tre
>> >> > e/a
>> >> > rch/arm/kernel/setup.c?id=refs/tags/next-20130619#n468
>> >> >
>> >> > and for used macros and bitmasks:
>> >> >
>> >> > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tre
>> >> > e/a
>> >> > rch/arm/include/asm/cputype.h?id=refs/tags/next-20130619#n45
>> >> >
>> >> > Now the structure of the MPIDR register:
>> >> >
>> >> > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0388e
>> >> > /CI
>> >> > HEBGFG.html
>> >> >
>> >> > As you can see, the value read from the register in
>> >> > smp_setup_processor_id() is only the physical CPU ID, so I don't see
>> >> > any
>> >> > problem here.
>> >>
>> >> Define "physical CPU ID" :-)
>> >>
>> >> There is a problem here: the MPIDR is not an index, and the
>> >> cpu_logical_map is populated in arm_dt_init_cpu_maps in:
>> >>
>> >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/a
Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
On Wednesday 19 of June 2013 19:25:27 Chander Kashyap wrote:
> On 19 June 2013 19:19, Tomasz Figa wrote:
> > On Wednesday 19 of June 2013 14:24:17 Lorenzo Pieralisi wrote:
> >> On Wed, Jun 19, 2013 at 01:50:57PM +0100, Tomasz Figa wrote:
> >> > On Wednesday 19 of June 2013 17:39:21 Chander Kashyap wrote:
> >> > > On 18 June 2013 23:29, Kukjin Kim wrote:
> >> > > > On 06/19/13 02:45, Tomasz Figa wrote:
> >> > > >> Ccing Arnd and Olof, because I forgot to add them to git
> >> > > >> send-email...
> >> > > >>
> >> > > >> Sorry for the noise.
> >> > > >>
> >> > > >> On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
> >> > > >>> S5P_ARM_CORE1_* registers affect only core 1. To control
> >> > > >>> further
> >> > > >>> cores
> >> > > >>> properly another registers must be used.
> >> > > >>>
> >> > > >>> This patch replaces S5P_ARM_CORE1_* register definitions with
> >> > > >>> S5P_ARM_CORE_*(x) macro which return addresses of registers
> >> > > >>> for
> >> > > >>> specified core.
> >> > > >>>
> >> > > >>> This fixes CPU hotplug on quad core Exynos SoCs on which
> >> > > >>> currently
> >> > > >>> offlining CPUs 2 or 3 caused CPU 1 to be turned off.
> >> > > >>
> >> > > >> Obviously this doesn't happen currently because of the if (cpu
> >> > > >> ==
> >> > > >> 1),
> >> > > >> but>
> >> > > >
> >> > > > Yes, not happened...and just note exynos5440 doesn't support
> >> > > > hotplug :)
> >> > > > so this is available on exynos4412 and added 5420.
> >> > > >
> >> > > >> if logical cpu1 turned out not to be physical cpu1, then it
> >> > > >> would
> >> > > >> crash.
> >> > > >>
> >> > > >> Best regards,
> >> > > >> Tomasz
> >> > > >>
> >> > > >>> In addition,
> >> > > >>> bring-up of CPU 2 and 3 is fixed on boards where bootloader
> >> > > >>> powers
> >> > > >>> off
> >> > > >>> secondary cores by default.
> >> > > >
> >> > > > I need to test on board about above...
> >> > > >
> >> > > >>> Cc: sta...@vger.kernel.org
> >> > > >>> Signed-off-by: Tomasz Figa
> >> > > >>> Signed-off-by: Kyungmin Park
> >> > > >>> ---
> >> > > >>>
> >> > > >>> arch/arm/mach-exynos/hotplug.c | 9 +
> >> > > >>> arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++---
> >> > > >>> arch/arm/mach-exynos/platsmp.c | 9 +
> >> > > >>> 3 files changed, 17 insertions(+), 11 deletions(-)
> >> > > >>>
> >> > > >>> diff --git a/arch/arm/mach-exynos/hotplug.c
> >> > > >>> b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
> >> > > >>> --- a/arch/arm/mach-exynos/hotplug.c
> >> > > >>> +++ b/arch/arm/mach-exynos/hotplug.c
> >> > > >>> @@ -93,10 +93,11 @@ static inline void
> >> > > >>> cpu_leave_lowpower(void)
> >> > > >>>
> >> > > >>> static inline void platform_do_lowpower(unsigned int cpu,
> >> > > >>> int
> >> > > >>>
> >> > > >>> *spurious) {
> >> > > >>>
> >> > > >>> for (;;) {
> >> > > >>>
> >> > > >>> + void __iomem *reg_base;
> >> > > >>> + unsigned int phys_cpu = cpu_logical_map(cpu);
> >> > > >>>
> >> > > >>> - /* make cpu1 to be turned off at next WFI
> >> > > >>> command
> >> > > >>> */
> >> > > >>> - if (cpu == 1)
> >> > > >>> - __raw_writel(0,
> >> > > >>> S5P_ARM_CORE1_CONFIGURATION);
> >> > > >>> + reg_base =
> >> > > >>> S5P_ARM_CORE_CONFIGURATION(phys_cpu);
> >> > >
> >> > > Tomasz,
> >> > > This will break for non-zero, MPIDR value. Say if MPIDR is 1 then
> >> > > for
> >> > > cpu0 phys_cpu value will be 0x100,
> >> > > and address calculation will be (S5P_ARM_CORE0_CONFIGURATION +
> >> > > ((0x101) * 0x80)), which is wrong.
> >>
> >> Honestly, I did not understand the reasoning above, please clarify.
> >>
> >> > Hmm, according to the code initializing __cpu_logical_map[] array
> >> > this
> >> > is not true.
> >> >
> >> > Here's the code:
> >> >
> >> > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tre
> >> > e/a
> >> > rch/arm/kernel/setup.c?id=refs/tags/next-20130619#n468
> >> >
> >> > and for used macros and bitmasks:
> >> >
> >> > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tre
> >> > e/a
> >> > rch/arm/include/asm/cputype.h?id=refs/tags/next-20130619#n45
> >> >
> >> > Now the structure of the MPIDR register:
> >> >
> >> > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0388e
> >> > /CI
> >> > HEBGFG.html
> >> >
> >> > As you can see, the value read from the register in
> >> > smp_setup_processor_id() is only the physical CPU ID, so I don't see
> >> > any
> >> > problem here.
> >>
> >> Define "physical CPU ID" :-)
> >>
> >> There is a problem here: the MPIDR is not an index, and the
> >> cpu_logical_map is populated in arm_dt_init_cpu_maps in:
> >>
> >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/a
> >> rch /arm/kernel/devtree.c?id=refs/tags/v3.10-rc6
> >>
> >> with all affinity levels.
> >
> > OK. This is what I was missing. Thanks.
>
Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
On 19 June 2013 19:19, Tomasz Figa wrote:
> On Wednesday 19 of June 2013 14:24:17 Lorenzo Pieralisi wrote:
>> On Wed, Jun 19, 2013 at 01:50:57PM +0100, Tomasz Figa wrote:
>> > On Wednesday 19 of June 2013 17:39:21 Chander Kashyap wrote:
>> > > On 18 June 2013 23:29, Kukjin Kim wrote:
>> > > > On 06/19/13 02:45, Tomasz Figa wrote:
>> > > >> Ccing Arnd and Olof, because I forgot to add them to git
>> > > >> send-email...
>> > > >>
>> > > >> Sorry for the noise.
>> > > >>
>> > > >> On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
>> > > >>> S5P_ARM_CORE1_* registers affect only core 1. To control further
>> > > >>> cores
>> > > >>> properly another registers must be used.
>> > > >>>
>> > > >>> This patch replaces S5P_ARM_CORE1_* register definitions with
>> > > >>> S5P_ARM_CORE_*(x) macro which return addresses of registers for
>> > > >>> specified core.
>> > > >>>
>> > > >>> This fixes CPU hotplug on quad core Exynos SoCs on which
>> > > >>> currently
>> > > >>> offlining CPUs 2 or 3 caused CPU 1 to be turned off.
>> > > >>
>> > > >> Obviously this doesn't happen currently because of the if (cpu ==
>> > > >> 1),
>> > > >> but>
>> > > >
>> > > > Yes, not happened...and just note exynos5440 doesn't support
>> > > > hotplug :)
>> > > > so this is available on exynos4412 and added 5420.
>> > > >
>> > > >> if logical cpu1 turned out not to be physical cpu1, then it would
>> > > >> crash.
>> > > >>
>> > > >> Best regards,
>> > > >> Tomasz
>> > > >>
>> > > >>> In addition,
>> > > >>> bring-up of CPU 2 and 3 is fixed on boards where bootloader
>> > > >>> powers
>> > > >>> off
>> > > >>> secondary cores by default.
>> > > >
>> > > > I need to test on board about above...
>> > > >
>> > > >>> Cc: sta...@vger.kernel.org
>> > > >>> Signed-off-by: Tomasz Figa
>> > > >>> Signed-off-by: Kyungmin Park
>> > > >>> ---
>> > > >>>
>> > > >>> arch/arm/mach-exynos/hotplug.c | 9 +
>> > > >>> arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++---
>> > > >>> arch/arm/mach-exynos/platsmp.c | 9 +
>> > > >>> 3 files changed, 17 insertions(+), 11 deletions(-)
>> > > >>>
>> > > >>> diff --git a/arch/arm/mach-exynos/hotplug.c
>> > > >>> b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
>> > > >>> --- a/arch/arm/mach-exynos/hotplug.c
>> > > >>> +++ b/arch/arm/mach-exynos/hotplug.c
>> > > >>> @@ -93,10 +93,11 @@ static inline void cpu_leave_lowpower(void)
>> > > >>>
>> > > >>> static inline void platform_do_lowpower(unsigned int cpu, int
>> > > >>>
>> > > >>> *spurious) {
>> > > >>>
>> > > >>> for (;;) {
>> > > >>>
>> > > >>> + void __iomem *reg_base;
>> > > >>> + unsigned int phys_cpu = cpu_logical_map(cpu);
>> > > >>>
>> > > >>> - /* make cpu1 to be turned off at next WFI command
>> > > >>> */
>> > > >>> - if (cpu == 1)
>> > > >>> - __raw_writel(0,
>> > > >>> S5P_ARM_CORE1_CONFIGURATION);
>> > > >>> + reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
>> > >
>> > > Tomasz,
>> > > This will break for non-zero, MPIDR value. Say if MPIDR is 1 then
>> > > for
>> > > cpu0 phys_cpu value will be 0x100,
>> > > and address calculation will be (S5P_ARM_CORE0_CONFIGURATION +
>> > > ((0x101) * 0x80)), which is wrong.
>>
>> Honestly, I did not understand the reasoning above, please clarify.
>>
>> > Hmm, according to the code initializing __cpu_logical_map[] array this
>> > is not true.
>> >
>> > Here's the code:
>> >
>> > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/a
>> > rch/arm/kernel/setup.c?id=refs/tags/next-20130619#n468
>> >
>> > and for used macros and bitmasks:
>> >
>> > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/a
>> > rch/arm/include/asm/cputype.h?id=refs/tags/next-20130619#n45
>> >
>> > Now the structure of the MPIDR register:
>> >
>> > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0388e/CI
>> > HEBGFG.html
>> >
>> > As you can see, the value read from the register in
>> > smp_setup_processor_id() is only the physical CPU ID, so I don't see
>> > any
>> > problem here.
>>
>> Define "physical CPU ID" :-)
>>
>> There is a problem here: the MPIDR is not an index, and the
>> cpu_logical_map is populated in arm_dt_init_cpu_maps in:
>>
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch
>> /arm/kernel/devtree.c?id=refs/tags/v3.10-rc6
>>
>> with all affinity levels.
>
> OK. This is what I was missing. Thanks.
>
>>
>> You need to perform a mapping between logical cpus and registers offset,
>> you can't use the cpu_logical_map directly for that.
>
> Hmm, can't I just extract cluster ID and CPU ID from the MPIDR value and
> use them appropriately to calculate register offsets?
That will create problem for multi-cluster systems. Say we have two
clusters then with clusterID 0 and 1. So phys-cpu will be 0x0/0x100,
0x1/0x101 ans so on.
>
> Best regards,
> Tomasz
>
>> Next
Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
On Wednesday 19 of June 2013 14:24:17 Lorenzo Pieralisi wrote:
> On Wed, Jun 19, 2013 at 01:50:57PM +0100, Tomasz Figa wrote:
> > On Wednesday 19 of June 2013 17:39:21 Chander Kashyap wrote:
> > > On 18 June 2013 23:29, Kukjin Kim wrote:
> > > > On 06/19/13 02:45, Tomasz Figa wrote:
> > > >> Ccing Arnd and Olof, because I forgot to add them to git
> > > >> send-email...
> > > >>
> > > >> Sorry for the noise.
> > > >>
> > > >> On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
> > > >>> S5P_ARM_CORE1_* registers affect only core 1. To control further
> > > >>> cores
> > > >>> properly another registers must be used.
> > > >>>
> > > >>> This patch replaces S5P_ARM_CORE1_* register definitions with
> > > >>> S5P_ARM_CORE_*(x) macro which return addresses of registers for
> > > >>> specified core.
> > > >>>
> > > >>> This fixes CPU hotplug on quad core Exynos SoCs on which
> > > >>> currently
> > > >>> offlining CPUs 2 or 3 caused CPU 1 to be turned off.
> > > >>
> > > >> Obviously this doesn't happen currently because of the if (cpu ==
> > > >> 1),
> > > >> but>
> > > >
> > > > Yes, not happened...and just note exynos5440 doesn't support
> > > > hotplug :)
> > > > so this is available on exynos4412 and added 5420.
> > > >
> > > >> if logical cpu1 turned out not to be physical cpu1, then it would
> > > >> crash.
> > > >>
> > > >> Best regards,
> > > >> Tomasz
> > > >>
> > > >>> In addition,
> > > >>> bring-up of CPU 2 and 3 is fixed on boards where bootloader
> > > >>> powers
> > > >>> off
> > > >>> secondary cores by default.
> > > >
> > > > I need to test on board about above...
> > > >
> > > >>> Cc: sta...@vger.kernel.org
> > > >>> Signed-off-by: Tomasz Figa
> > > >>> Signed-off-by: Kyungmin Park
> > > >>> ---
> > > >>>
> > > >>> arch/arm/mach-exynos/hotplug.c | 9 +
> > > >>> arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++---
> > > >>> arch/arm/mach-exynos/platsmp.c | 9 +
> > > >>> 3 files changed, 17 insertions(+), 11 deletions(-)
> > > >>>
> > > >>> diff --git a/arch/arm/mach-exynos/hotplug.c
> > > >>> b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
> > > >>> --- a/arch/arm/mach-exynos/hotplug.c
> > > >>> +++ b/arch/arm/mach-exynos/hotplug.c
> > > >>> @@ -93,10 +93,11 @@ static inline void cpu_leave_lowpower(void)
> > > >>>
> > > >>> static inline void platform_do_lowpower(unsigned int cpu, int
> > > >>>
> > > >>> *spurious) {
> > > >>>
> > > >>> for (;;) {
> > > >>>
> > > >>> + void __iomem *reg_base;
> > > >>> + unsigned int phys_cpu = cpu_logical_map(cpu);
> > > >>>
> > > >>> - /* make cpu1 to be turned off at next WFI command
> > > >>> */
> > > >>> - if (cpu == 1)
> > > >>> - __raw_writel(0,
> > > >>> S5P_ARM_CORE1_CONFIGURATION);
> > > >>> + reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
> > >
> > > Tomasz,
> > > This will break for non-zero, MPIDR value. Say if MPIDR is 1 then
> > > for
> > > cpu0 phys_cpu value will be 0x100,
> > > and address calculation will be (S5P_ARM_CORE0_CONFIGURATION +
> > > ((0x101) * 0x80)), which is wrong.
>
> Honestly, I did not understand the reasoning above, please clarify.
>
> > Hmm, according to the code initializing __cpu_logical_map[] array this
> > is not true.
> >
> > Here's the code:
> >
> > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/a
> > rch/arm/kernel/setup.c?id=refs/tags/next-20130619#n468
> >
> > and for used macros and bitmasks:
> >
> > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/a
> > rch/arm/include/asm/cputype.h?id=refs/tags/next-20130619#n45
> >
> > Now the structure of the MPIDR register:
> >
> > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0388e/CI
> > HEBGFG.html
> >
> > As you can see, the value read from the register in
> > smp_setup_processor_id() is only the physical CPU ID, so I don't see
> > any
> > problem here.
>
> Define "physical CPU ID" :-)
>
> There is a problem here: the MPIDR is not an index, and the
> cpu_logical_map is populated in arm_dt_init_cpu_maps in:
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch
> /arm/kernel/devtree.c?id=refs/tags/v3.10-rc6
>
> with all affinity levels.
OK. This is what I was missing. Thanks.
>
> You need to perform a mapping between logical cpus and registers offset,
> you can't use the cpu_logical_map directly for that.
Hmm, can't I just extract cluster ID and CPU ID from the MPIDR value and
use them appropriately to calculate register offsets?
Best regards,
Tomasz
> Next accident waiting to happen is GIC code (CONFIG_GIC_NON_BANKED),
> where cpu_logical_map is used erroneously as an index.
>
> Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at
Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
On 19 June 2013 18:54, Lorenzo Pieralisi wrote:
> On Wed, Jun 19, 2013 at 01:50:57PM +0100, Tomasz Figa wrote:
>> On Wednesday 19 of June 2013 17:39:21 Chander Kashyap wrote:
>> > On 18 June 2013 23:29, Kukjin Kim wrote:
>> > > On 06/19/13 02:45, Tomasz Figa wrote:
>> > >> Ccing Arnd and Olof, because I forgot to add them to git send-email...
>> > >>
>> > >> Sorry for the noise.
>> > >>
>> > >> On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
>> > >>> S5P_ARM_CORE1_* registers affect only core 1. To control further
>> > >>> cores
>> > >>> properly another registers must be used.
>> > >>>
>> > >>> This patch replaces S5P_ARM_CORE1_* register definitions with
>> > >>> S5P_ARM_CORE_*(x) macro which return addresses of registers for
>> > >>> specified core.
>> > >>>
>> > >>> This fixes CPU hotplug on quad core Exynos SoCs on which currently
>> > >>> offlining CPUs 2 or 3 caused CPU 1 to be turned off.
>> > >>
>> > >> Obviously this doesn't happen currently because of the if (cpu == 1),
>> > >> but>
>> > > Yes, not happened...and just note exynos5440 doesn't support hotplug :)
>> > > so this is available on exynos4412 and added 5420.
>> > >
>> > >> if logical cpu1 turned out not to be physical cpu1, then it would
>> > >> crash.
>> > >>
>> > >> Best regards,
>> > >> Tomasz
>> > >>
>> > >>> In addition,
>> > >>> bring-up of CPU 2 and 3 is fixed on boards where bootloader powers
>> > >>> off
>> > >>> secondary cores by default.
>> > >
>> > > I need to test on board about above...
>> > >
>> > >>> Cc: sta...@vger.kernel.org
>> > >>> Signed-off-by: Tomasz Figa
>> > >>> Signed-off-by: Kyungmin Park
>> > >>> ---
>> > >>>
>> > >>> arch/arm/mach-exynos/hotplug.c | 9 +
>> > >>> arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++---
>> > >>> arch/arm/mach-exynos/platsmp.c | 9 +
>> > >>> 3 files changed, 17 insertions(+), 11 deletions(-)
>> > >>>
>> > >>> diff --git a/arch/arm/mach-exynos/hotplug.c
>> > >>> b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
>> > >>> --- a/arch/arm/mach-exynos/hotplug.c
>> > >>> +++ b/arch/arm/mach-exynos/hotplug.c
>> > >>> @@ -93,10 +93,11 @@ static inline void cpu_leave_lowpower(void)
>> > >>>
>> > >>> static inline void platform_do_lowpower(unsigned int cpu, int
>> > >>>
>> > >>> *spurious) {
>> > >>>
>> > >>> for (;;) {
>> > >>>
>> > >>> + void __iomem *reg_base;
>> > >>> + unsigned int phys_cpu = cpu_logical_map(cpu);
>> > >>>
>> > >>> - /* make cpu1 to be turned off at next WFI command */
>> > >>> - if (cpu == 1)
>> > >>> - __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
>> > >>> + reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
>> >
>> > Tomasz,
>> > This will break for non-zero, MPIDR value. Say if MPIDR is 1 then for
>> > cpu0 phys_cpu value will be 0x100,
>> > and address calculation will be (S5P_ARM_CORE0_CONFIGURATION +
>> > ((0x101) * 0x80)), which is wrong.
>
> Honestly, I did not understand the reasoning above, please clarify.
Sorry not non-zero MPIDR but non-zero ClusterID.
All the power register addresses are at offset 0x80.
>
>>
>> Hmm, according to the code initializing __cpu_logical_map[] array this is
>> not true.
>>
>> Here's the code:
>>
>> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/kernel/setup.c?id=refs/tags/next-20130619#n468
>>
>> and for used macros and bitmasks:
>>
>> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/include/asm/cputype.h?id=refs/tags/next-20130619#n45
>>
>> Now the structure of the MPIDR register:
>>
>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0388e/CIHEBGFG.html
>>
>> As you can see, the value read from the register in
>> smp_setup_processor_id() is only the physical CPU ID, so I don't see any
>> problem here.
>
> Define "physical CPU ID" :-)
>
> There is a problem here: the MPIDR is not an index, and the cpu_logical_map is
> populated in arm_dt_init_cpu_maps in:
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/kernel/devtree.c?id=refs/tags/v3.10-rc6
>
> with all affinity levels.
>
> You need to perform a mapping between logical cpus and registers offset,
> you can't use the cpu_logical_map directly for that.
>
> Next accident waiting to happen is GIC code (CONFIG_GIC_NON_BANKED), where
> cpu_logical_map is used erroneously as an index.
>
> Lorenzo
>
--
with warm regards,
Chander Kashyap
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
On 19 June 2013 18:20, Tomasz Figa wrote:
> On Wednesday 19 of June 2013 17:39:21 Chander Kashyap wrote:
>> On 18 June 2013 23:29, Kukjin Kim wrote:
>> > On 06/19/13 02:45, Tomasz Figa wrote:
>> >> Ccing Arnd and Olof, because I forgot to add them to git send-email...
>> >>
>> >> Sorry for the noise.
>> >>
>> >> On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
>> >>> S5P_ARM_CORE1_* registers affect only core 1. To control further
>> >>> cores
>> >>> properly another registers must be used.
>> >>>
>> >>> This patch replaces S5P_ARM_CORE1_* register definitions with
>> >>> S5P_ARM_CORE_*(x) macro which return addresses of registers for
>> >>> specified core.
>> >>>
>> >>> This fixes CPU hotplug on quad core Exynos SoCs on which currently
>> >>> offlining CPUs 2 or 3 caused CPU 1 to be turned off.
>> >>
>> >> Obviously this doesn't happen currently because of the if (cpu == 1),
>> >> but>
>> > Yes, not happened...and just note exynos5440 doesn't support hotplug :)
>> > so this is available on exynos4412 and added 5420.
>> >
>> >> if logical cpu1 turned out not to be physical cpu1, then it would
>> >> crash.
>> >>
>> >> Best regards,
>> >> Tomasz
>> >>
>> >>> In addition,
>> >>> bring-up of CPU 2 and 3 is fixed on boards where bootloader powers
>> >>> off
>> >>> secondary cores by default.
>> >
>> > I need to test on board about above...
>> >
>> >>> Cc: sta...@vger.kernel.org
>> >>> Signed-off-by: Tomasz Figa
>> >>> Signed-off-by: Kyungmin Park
>> >>> ---
>> >>>
>> >>> arch/arm/mach-exynos/hotplug.c | 9 +
>> >>> arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++---
>> >>> arch/arm/mach-exynos/platsmp.c | 9 +
>> >>> 3 files changed, 17 insertions(+), 11 deletions(-)
>> >>>
>> >>> diff --git a/arch/arm/mach-exynos/hotplug.c
>> >>> b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
>> >>> --- a/arch/arm/mach-exynos/hotplug.c
>> >>> +++ b/arch/arm/mach-exynos/hotplug.c
>> >>> @@ -93,10 +93,11 @@ static inline void cpu_leave_lowpower(void)
>> >>>
>> >>> static inline void platform_do_lowpower(unsigned int cpu, int
>> >>>
>> >>> *spurious) {
>> >>>
>> >>> for (;;) {
>> >>>
>> >>> + void __iomem *reg_base;
>> >>> + unsigned int phys_cpu = cpu_logical_map(cpu);
>> >>>
>> >>> - /* make cpu1 to be turned off at next WFI command */
>> >>> - if (cpu == 1)
>> >>> - __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
>> >>> + reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
>>
>> Tomasz,
>> This will break for non-zero, MPIDR value. Say if MPIDR is 1 then for
>> cpu0 phys_cpu value will be 0x100,
>> and address calculation will be (S5P_ARM_CORE0_CONFIGURATION +
>> ((0x101) * 0x80)), which is wrong.
>
> Hmm, according to the code initializing __cpu_logical_map[] array this is
> not true.
>
> Here's the code:
>
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/kernel/setup.c?id=refs/tags/next-20130619#n468
>
> and for used macros and bitmasks:
>
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/include/asm/cputype.h?id=refs/tags/next-20130619#n45
>
> Now the structure of the MPIDR register:
>
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0388e/CIHEBGFG.html
>
> As you can see, the value read from the register in
> smp_setup_processor_id() is only the physical CPU ID, so I don't see any
> problem here.
Kindly look at implementation of "arm_dt_init_cpu_maps" in
"arch/arm/kernel/devtree.c". When you pass the cpu nodes thru DT, and
cluster id is non zero, the phys_cpu will include clusterID. Say e.g
exynos4210 has CLusterID=9, hence phys_cpu value for cpu0 will be
0x901.
>
> If I'm wrong, feel free to correct me.
>
> Cc'ing people who should have more knowledge on this.
>
> Best regards,
> Tomasz
>
>> >>> + __raw_writel(0, reg_base);
>> >>>
>> >>> /*
>> >>>
>> >>> * here's the WFI
>> >>>
>> >>> @@ -106,7 +107,7 @@ static inline void platform_do_lowpower(unsigned
>> >>> int
>> >>> cpu, int *spurious)
>> >>>
>> >>> : "memory", "cc");
>> >>>
>> >>> - if (pen_release == cpu_logical_map(cpu)) {
>> >>> + if (pen_release == phys_cpu) {
>> >>>
>> >>> /*
>> >>>
>> >>> * OK, proper wakeup, we're done
>> >>> */
>> >>>
>> >>> diff --git a/arch/arm/mach-exynos/include/mach/regs-pmu.h
>> >>> b/arch/arm/mach-exynos/include/mach/regs-pmu.h index 57344b7..cf40b86
>> >>> 100644
>> >>> --- a/arch/arm/mach-exynos/include/mach/regs-pmu.h
>> >>> +++ b/arch/arm/mach-exynos/include/mach/regs-pmu.h
>> >>> @@ -125,10 +125,14 @@
>> >>>
>> >>> #define S5P_GPS_ALIVE_LOWPWR S5P_PMUREG(0x13A0)
>> >>>
>> >>> #define S5P_ARM_CORE0_CONFIGURATION S5P_PMUREG(0x2000)
>> >>>
>> >>> +#define S5P_ARM_CORE0_STATUS
Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
On Wed, Jun 19, 2013 at 01:50:57PM +0100, Tomasz Figa wrote:
> On Wednesday 19 of June 2013 17:39:21 Chander Kashyap wrote:
> > On 18 June 2013 23:29, Kukjin Kim wrote:
> > > On 06/19/13 02:45, Tomasz Figa wrote:
> > >> Ccing Arnd and Olof, because I forgot to add them to git send-email...
> > >>
> > >> Sorry for the noise.
> > >>
> > >> On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
> > >>> S5P_ARM_CORE1_* registers affect only core 1. To control further
> > >>> cores
> > >>> properly another registers must be used.
> > >>>
> > >>> This patch replaces S5P_ARM_CORE1_* register definitions with
> > >>> S5P_ARM_CORE_*(x) macro which return addresses of registers for
> > >>> specified core.
> > >>>
> > >>> This fixes CPU hotplug on quad core Exynos SoCs on which currently
> > >>> offlining CPUs 2 or 3 caused CPU 1 to be turned off.
> > >>
> > >> Obviously this doesn't happen currently because of the if (cpu == 1),
> > >> but>
> > > Yes, not happened...and just note exynos5440 doesn't support hotplug :)
> > > so this is available on exynos4412 and added 5420.
> > >
> > >> if logical cpu1 turned out not to be physical cpu1, then it would
> > >> crash.
> > >>
> > >> Best regards,
> > >> Tomasz
> > >>
> > >>> In addition,
> > >>> bring-up of CPU 2 and 3 is fixed on boards where bootloader powers
> > >>> off
> > >>> secondary cores by default.
> > >
> > > I need to test on board about above...
> > >
> > >>> Cc: sta...@vger.kernel.org
> > >>> Signed-off-by: Tomasz Figa
> > >>> Signed-off-by: Kyungmin Park
> > >>> ---
> > >>>
> > >>> arch/arm/mach-exynos/hotplug.c | 9 +
> > >>> arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++---
> > >>> arch/arm/mach-exynos/platsmp.c | 9 +
> > >>> 3 files changed, 17 insertions(+), 11 deletions(-)
> > >>>
> > >>> diff --git a/arch/arm/mach-exynos/hotplug.c
> > >>> b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
> > >>> --- a/arch/arm/mach-exynos/hotplug.c
> > >>> +++ b/arch/arm/mach-exynos/hotplug.c
> > >>> @@ -93,10 +93,11 @@ static inline void cpu_leave_lowpower(void)
> > >>>
> > >>> static inline void platform_do_lowpower(unsigned int cpu, int
> > >>>
> > >>> *spurious) {
> > >>>
> > >>> for (;;) {
> > >>>
> > >>> + void __iomem *reg_base;
> > >>> + unsigned int phys_cpu = cpu_logical_map(cpu);
> > >>>
> > >>> - /* make cpu1 to be turned off at next WFI command */
> > >>> - if (cpu == 1)
> > >>> - __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
> > >>> + reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
> >
> > Tomasz,
> > This will break for non-zero, MPIDR value. Say if MPIDR is 1 then for
> > cpu0 phys_cpu value will be 0x100,
> > and address calculation will be (S5P_ARM_CORE0_CONFIGURATION +
> > ((0x101) * 0x80)), which is wrong.
Honestly, I did not understand the reasoning above, please clarify.
>
> Hmm, according to the code initializing __cpu_logical_map[] array this is
> not true.
>
> Here's the code:
>
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/kernel/setup.c?id=refs/tags/next-20130619#n468
>
> and for used macros and bitmasks:
>
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/include/asm/cputype.h?id=refs/tags/next-20130619#n45
>
> Now the structure of the MPIDR register:
>
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0388e/CIHEBGFG.html
>
> As you can see, the value read from the register in
> smp_setup_processor_id() is only the physical CPU ID, so I don't see any
> problem here.
Define "physical CPU ID" :-)
There is a problem here: the MPIDR is not an index, and the cpu_logical_map is
populated in arm_dt_init_cpu_maps in:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/kernel/devtree.c?id=refs/tags/v3.10-rc6
with all affinity levels.
You need to perform a mapping between logical cpus and registers offset,
you can't use the cpu_logical_map directly for that.
Next accident waiting to happen is GIC code (CONFIG_GIC_NON_BANKED), where
cpu_logical_map is used erroneously as an index.
Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
On Wednesday 19 of June 2013 17:39:21 Chander Kashyap wrote:
> On 18 June 2013 23:29, Kukjin Kim wrote:
> > On 06/19/13 02:45, Tomasz Figa wrote:
> >> Ccing Arnd and Olof, because I forgot to add them to git send-email...
> >>
> >> Sorry for the noise.
> >>
> >> On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
> >>> S5P_ARM_CORE1_* registers affect only core 1. To control further
> >>> cores
> >>> properly another registers must be used.
> >>>
> >>> This patch replaces S5P_ARM_CORE1_* register definitions with
> >>> S5P_ARM_CORE_*(x) macro which return addresses of registers for
> >>> specified core.
> >>>
> >>> This fixes CPU hotplug on quad core Exynos SoCs on which currently
> >>> offlining CPUs 2 or 3 caused CPU 1 to be turned off.
> >>
> >> Obviously this doesn't happen currently because of the if (cpu == 1),
> >> but>
> > Yes, not happened...and just note exynos5440 doesn't support hotplug :)
> > so this is available on exynos4412 and added 5420.
> >
> >> if logical cpu1 turned out not to be physical cpu1, then it would
> >> crash.
> >>
> >> Best regards,
> >> Tomasz
> >>
> >>> In addition,
> >>> bring-up of CPU 2 and 3 is fixed on boards where bootloader powers
> >>> off
> >>> secondary cores by default.
> >
> > I need to test on board about above...
> >
> >>> Cc: sta...@vger.kernel.org
> >>> Signed-off-by: Tomasz Figa
> >>> Signed-off-by: Kyungmin Park
> >>> ---
> >>>
> >>> arch/arm/mach-exynos/hotplug.c | 9 +
> >>> arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++---
> >>> arch/arm/mach-exynos/platsmp.c | 9 +
> >>> 3 files changed, 17 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/arch/arm/mach-exynos/hotplug.c
> >>> b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
> >>> --- a/arch/arm/mach-exynos/hotplug.c
> >>> +++ b/arch/arm/mach-exynos/hotplug.c
> >>> @@ -93,10 +93,11 @@ static inline void cpu_leave_lowpower(void)
> >>>
> >>> static inline void platform_do_lowpower(unsigned int cpu, int
> >>>
> >>> *spurious) {
> >>>
> >>> for (;;) {
> >>>
> >>> + void __iomem *reg_base;
> >>> + unsigned int phys_cpu = cpu_logical_map(cpu);
> >>>
> >>> - /* make cpu1 to be turned off at next WFI command */
> >>> - if (cpu == 1)
> >>> - __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
> >>> + reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
>
> Tomasz,
> This will break for non-zero, MPIDR value. Say if MPIDR is 1 then for
> cpu0 phys_cpu value will be 0x100,
> and address calculation will be (S5P_ARM_CORE0_CONFIGURATION +
> ((0x101) * 0x80)), which is wrong.
Hmm, according to the code initializing __cpu_logical_map[] array this is
not true.
Here's the code:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/kernel/setup.c?id=refs/tags/next-20130619#n468
and for used macros and bitmasks:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/include/asm/cputype.h?id=refs/tags/next-20130619#n45
Now the structure of the MPIDR register:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0388e/CIHEBGFG.html
As you can see, the value read from the register in
smp_setup_processor_id() is only the physical CPU ID, so I don't see any
problem here.
If I'm wrong, feel free to correct me.
Cc'ing people who should have more knowledge on this.
Best regards,
Tomasz
> >>> + __raw_writel(0, reg_base);
> >>>
> >>> /*
> >>>
> >>> * here's the WFI
> >>>
> >>> @@ -106,7 +107,7 @@ static inline void platform_do_lowpower(unsigned
> >>> int
> >>> cpu, int *spurious)
> >>>
> >>> : "memory", "cc");
> >>>
> >>> - if (pen_release == cpu_logical_map(cpu)) {
> >>> + if (pen_release == phys_cpu) {
> >>>
> >>> /*
> >>>
> >>> * OK, proper wakeup, we're done
> >>> */
> >>>
> >>> diff --git a/arch/arm/mach-exynos/include/mach/regs-pmu.h
> >>> b/arch/arm/mach-exynos/include/mach/regs-pmu.h index 57344b7..cf40b86
> >>> 100644
> >>> --- a/arch/arm/mach-exynos/include/mach/regs-pmu.h
> >>> +++ b/arch/arm/mach-exynos/include/mach/regs-pmu.h
> >>> @@ -125,10 +125,14 @@
> >>>
> >>> #define S5P_GPS_ALIVE_LOWPWR S5P_PMUREG(0x13A0)
> >>>
> >>> #define S5P_ARM_CORE0_CONFIGURATION S5P_PMUREG(0x2000)
> >>>
> >>> +#define S5P_ARM_CORE0_STATUS S5P_PMUREG(0x2004)
> >>>
> >>> #define S5P_ARM_CORE0_OPTION S5P_PMUREG(0x2008)
> >>>
> >>> -#define S5P_ARM_CORE1_CONFIGURATIONS5P_PMUREG(0x2080)
> >>> -#define S5P_ARM_CORE1_STATUS S5P_PMUREG(0x2084)
> >>> -#define S5P_ARM_CORE1_OPTION S5P_PMUREG(0x2088)
> >>> +#define
Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
On 18 June 2013 23:29, Kukjin Kim wrote:
> On 06/19/13 02:45, Tomasz Figa wrote:
>>
>> Ccing Arnd and Olof, because I forgot to add them to git send-email...
>>
>> Sorry for the noise.
>>
>> On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
>>>
>>> S5P_ARM_CORE1_* registers affect only core 1. To control further cores
>>> properly another registers must be used.
>>>
>>> This patch replaces S5P_ARM_CORE1_* register definitions with
>>> S5P_ARM_CORE_*(x) macro which return addresses of registers for
>>> specified core.
>>>
>>> This fixes CPU hotplug on quad core Exynos SoCs on which currently
>>> offlining CPUs 2 or 3 caused CPU 1 to be turned off.
>>
>>
>> Obviously this doesn't happen currently because of the if (cpu == 1), but
>
>
> Yes, not happened...and just note exynos5440 doesn't support hotplug :) so
> this is available on exynos4412 and added 5420.
>
>
>> if logical cpu1 turned out not to be physical cpu1, then it would crash.
>>
>> Best regards,
>> Tomasz
>>
>>> In addition,
>>> bring-up of CPU 2 and 3 is fixed on boards where bootloader powers off
>>> secondary cores by default.
>>>
> I need to test on board about above...
>
>>> Cc: sta...@vger.kernel.org
>>> Signed-off-by: Tomasz Figa
>>> Signed-off-by: Kyungmin Park
>>> ---
>>> arch/arm/mach-exynos/hotplug.c | 9 +
>>> arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++---
>>> arch/arm/mach-exynos/platsmp.c | 9 +
>>> 3 files changed, 17 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-exynos/hotplug.c
>>> b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
>>> --- a/arch/arm/mach-exynos/hotplug.c
>>> +++ b/arch/arm/mach-exynos/hotplug.c
>>> @@ -93,10 +93,11 @@ static inline void cpu_leave_lowpower(void)
>>> static inline void platform_do_lowpower(unsigned int cpu, int
>>> *spurious) {
>>> for (;;) {
>>> + void __iomem *reg_base;
>>> + unsigned int phys_cpu = cpu_logical_map(cpu);
>>>
>>> - /* make cpu1 to be turned off at next WFI command */
>>> - if (cpu == 1)
>>> - __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
>>> + reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
Tomasz,
This will break for non-zero, MPIDR value. Say if MPIDR is 1 then for
cpu0 phys_cpu value will be 0x100,
and address calculation will be (S5P_ARM_CORE0_CONFIGURATION +
((0x101) * 0x80)), which is wrong.
>>> + __raw_writel(0, reg_base);
>>>
>>> /*
>>> * here's the WFI
>>> @@ -106,7 +107,7 @@ static inline void platform_do_lowpower(unsigned int
>>> cpu, int *spurious)
>>> : "memory", "cc");
>>>
>>> - if (pen_release == cpu_logical_map(cpu)) {
>>> + if (pen_release == phys_cpu) {
>>> /*
>>> * OK, proper wakeup, we're done
>>> */
>>> diff --git a/arch/arm/mach-exynos/include/mach/regs-pmu.h
>>> b/arch/arm/mach-exynos/include/mach/regs-pmu.h index 57344b7..cf40b86
>>> 100644
>>> --- a/arch/arm/mach-exynos/include/mach/regs-pmu.h
>>> +++ b/arch/arm/mach-exynos/include/mach/regs-pmu.h
>>> @@ -125,10 +125,14 @@
>>> #define S5P_GPS_ALIVE_LOWPWR S5P_PMUREG(0x13A0)
>>>
>>> #define S5P_ARM_CORE0_CONFIGURATION S5P_PMUREG(0x2000)
>>> +#define S5P_ARM_CORE0_STATUS S5P_PMUREG(0x2004)
>>> #define S5P_ARM_CORE0_OPTION S5P_PMUREG(0x2008)
>>> -#define S5P_ARM_CORE1_CONFIGURATIONS5P_PMUREG(0x2080)
>>> -#define S5P_ARM_CORE1_STATUS S5P_PMUREG(0x2084)
>>> -#define S5P_ARM_CORE1_OPTION S5P_PMUREG(0x2088)
>>> +#define S5P_ARM_CORE_CONFIGURATION(_nr)\
>>> + (S5P_ARM_CORE0_CONFIGURATION + ((_nr) * 0x80))
>>> +#define S5P_ARM_CORE_STATUS(_nr) \
>>> + (S5P_ARM_CORE0_STATUS + ((_nr) * 0x80))
>>> +#define S5P_ARM_CORE_OPTION(_nr) \
>>> + (S5P_ARM_CORE0_OPTION + ((_nr) * 0x80))
>>>
>>> #define S5P_ARM_COMMON_OPTION S5P_PMUREG(0x2408)
>>> #define S5P_TOP_PWR_OPTIONS5P_PMUREG(0x2C48)
>>> diff --git a/arch/arm/mach-exynos/platsmp.c
>>> b/arch/arm/mach-exynos/platsmp.c index d9c6d0a..2cbabc8 100644
>>> --- a/arch/arm/mach-exynos/platsmp.c
>>> +++ b/arch/arm/mach-exynos/platsmp.c
>>> @@ -109,14 +109,15 @@ static int __cpuinit
>>> exynos_boot_secondary(unsigned int cpu, struct task_struct */
>>> write_pen_release(phys_cpu);
>>>
>>> - if (!(__raw_readl(S5P_ARM_CORE1_STATUS)& S5P_CORE_LOCAL_PWR_EN))
>>
>> {
>>>
>>> + if (!(__raw_readl(S5P_ARM_CORE_STATUS(phys_cpu))
>>> + & S5P_CORE_LOCAL_PWR_EN)) {
>>> __raw_writel(S5P_CORE_LOCAL_PWR_EN,
>>> -S5P_ARM_CORE1_CONFIGURATION);
>>> +S5P_ARM_CORE_CONFIGURATION(phys_cpu));
Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
On 18 June 2013 23:29, Kukjin Kim kgene@samsung.com wrote:
On 06/19/13 02:45, Tomasz Figa wrote:
Ccing Arnd and Olof, because I forgot to add them to git send-email...
Sorry for the noise.
On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
S5P_ARM_CORE1_* registers affect only core 1. To control further cores
properly another registers must be used.
This patch replaces S5P_ARM_CORE1_* register definitions with
S5P_ARM_CORE_*(x) macro which return addresses of registers for
specified core.
This fixes CPU hotplug on quad core Exynos SoCs on which currently
offlining CPUs 2 or 3 caused CPU 1 to be turned off.
Obviously this doesn't happen currently because of the if (cpu == 1), but
Yes, not happened...and just note exynos5440 doesn't support hotplug :) so
this is available on exynos4412 and added 5420.
if logical cpu1 turned out not to be physical cpu1, then it would crash.
Best regards,
Tomasz
In addition,
bring-up of CPU 2 and 3 is fixed on boards where bootloader powers off
secondary cores by default.
I need to test on board about above...
Cc: sta...@vger.kernel.org
Signed-off-by: Tomasz Figat.f...@samsung.com
Signed-off-by: Kyungmin Parkkyungmin.p...@samsung.com
---
arch/arm/mach-exynos/hotplug.c | 9 +
arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++---
arch/arm/mach-exynos/platsmp.c | 9 +
3 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/arch/arm/mach-exynos/hotplug.c
b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
--- a/arch/arm/mach-exynos/hotplug.c
+++ b/arch/arm/mach-exynos/hotplug.c
@@ -93,10 +93,11 @@ static inline void cpu_leave_lowpower(void)
static inline void platform_do_lowpower(unsigned int cpu, int
*spurious) {
for (;;) {
+ void __iomem *reg_base;
+ unsigned int phys_cpu = cpu_logical_map(cpu);
- /* make cpu1 to be turned off at next WFI command */
- if (cpu == 1)
- __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
+ reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
Tomasz,
This will break for non-zero, MPIDR value. Say if MPIDR is 1 then for
cpu0 phys_cpu value will be 0x100,
and address calculation will be (S5P_ARM_CORE0_CONFIGURATION +
((0x101) * 0x80)), which is wrong.
+ __raw_writel(0, reg_base);
/*
* here's the WFI
@@ -106,7 +107,7 @@ static inline void platform_do_lowpower(unsigned int
cpu, int *spurious)
: memory, cc);
- if (pen_release == cpu_logical_map(cpu)) {
+ if (pen_release == phys_cpu) {
/*
* OK, proper wakeup, we're done
*/
diff --git a/arch/arm/mach-exynos/include/mach/regs-pmu.h
b/arch/arm/mach-exynos/include/mach/regs-pmu.h index 57344b7..cf40b86
100644
--- a/arch/arm/mach-exynos/include/mach/regs-pmu.h
+++ b/arch/arm/mach-exynos/include/mach/regs-pmu.h
@@ -125,10 +125,14 @@
#define S5P_GPS_ALIVE_LOWPWR S5P_PMUREG(0x13A0)
#define S5P_ARM_CORE0_CONFIGURATION S5P_PMUREG(0x2000)
+#define S5P_ARM_CORE0_STATUS S5P_PMUREG(0x2004)
#define S5P_ARM_CORE0_OPTION S5P_PMUREG(0x2008)
-#define S5P_ARM_CORE1_CONFIGURATIONS5P_PMUREG(0x2080)
-#define S5P_ARM_CORE1_STATUS S5P_PMUREG(0x2084)
-#define S5P_ARM_CORE1_OPTION S5P_PMUREG(0x2088)
+#define S5P_ARM_CORE_CONFIGURATION(_nr)\
+ (S5P_ARM_CORE0_CONFIGURATION + ((_nr) * 0x80))
+#define S5P_ARM_CORE_STATUS(_nr) \
+ (S5P_ARM_CORE0_STATUS + ((_nr) * 0x80))
+#define S5P_ARM_CORE_OPTION(_nr) \
+ (S5P_ARM_CORE0_OPTION + ((_nr) * 0x80))
#define S5P_ARM_COMMON_OPTION S5P_PMUREG(0x2408)
#define S5P_TOP_PWR_OPTIONS5P_PMUREG(0x2C48)
diff --git a/arch/arm/mach-exynos/platsmp.c
b/arch/arm/mach-exynos/platsmp.c index d9c6d0a..2cbabc8 100644
--- a/arch/arm/mach-exynos/platsmp.c
+++ b/arch/arm/mach-exynos/platsmp.c
@@ -109,14 +109,15 @@ static int __cpuinit
exynos_boot_secondary(unsigned int cpu, struct task_struct */
write_pen_release(phys_cpu);
- if (!(__raw_readl(S5P_ARM_CORE1_STATUS) S5P_CORE_LOCAL_PWR_EN))
{
+ if (!(__raw_readl(S5P_ARM_CORE_STATUS(phys_cpu))
+ S5P_CORE_LOCAL_PWR_EN)) {
__raw_writel(S5P_CORE_LOCAL_PWR_EN,
-S5P_ARM_CORE1_CONFIGURATION);
+S5P_ARM_CORE_CONFIGURATION(phys_cpu));
timeout = 10;
/* wait max 10 ms until cpu1 is on */
- while ((__raw_readl(S5P_ARM_CORE1_STATUS)
+ while ((__raw_readl(S5P_ARM_CORE_STATUS(phys_cpu))
Ditto
Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
On Wednesday 19 of June 2013 17:39:21 Chander Kashyap wrote:
On 18 June 2013 23:29, Kukjin Kim kgene@samsung.com wrote:
On 06/19/13 02:45, Tomasz Figa wrote:
Ccing Arnd and Olof, because I forgot to add them to git send-email...
Sorry for the noise.
On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
S5P_ARM_CORE1_* registers affect only core 1. To control further
cores
properly another registers must be used.
This patch replaces S5P_ARM_CORE1_* register definitions with
S5P_ARM_CORE_*(x) macro which return addresses of registers for
specified core.
This fixes CPU hotplug on quad core Exynos SoCs on which currently
offlining CPUs 2 or 3 caused CPU 1 to be turned off.
Obviously this doesn't happen currently because of the if (cpu == 1),
but
Yes, not happened...and just note exynos5440 doesn't support hotplug :)
so this is available on exynos4412 and added 5420.
if logical cpu1 turned out not to be physical cpu1, then it would
crash.
Best regards,
Tomasz
In addition,
bring-up of CPU 2 and 3 is fixed on boards where bootloader powers
off
secondary cores by default.
I need to test on board about above...
Cc: sta...@vger.kernel.org
Signed-off-by: Tomasz Figat.f...@samsung.com
Signed-off-by: Kyungmin Parkkyungmin.p...@samsung.com
---
arch/arm/mach-exynos/hotplug.c | 9 +
arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++---
arch/arm/mach-exynos/platsmp.c | 9 +
3 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/arch/arm/mach-exynos/hotplug.c
b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
--- a/arch/arm/mach-exynos/hotplug.c
+++ b/arch/arm/mach-exynos/hotplug.c
@@ -93,10 +93,11 @@ static inline void cpu_leave_lowpower(void)
static inline void platform_do_lowpower(unsigned int cpu, int
*spurious) {
for (;;) {
+ void __iomem *reg_base;
+ unsigned int phys_cpu = cpu_logical_map(cpu);
- /* make cpu1 to be turned off at next WFI command */
- if (cpu == 1)
- __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
+ reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
Tomasz,
This will break for non-zero, MPIDR value. Say if MPIDR is 1 then for
cpu0 phys_cpu value will be 0x100,
and address calculation will be (S5P_ARM_CORE0_CONFIGURATION +
((0x101) * 0x80)), which is wrong.
Hmm, according to the code initializing __cpu_logical_map[] array this is
not true.
Here's the code:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/kernel/setup.c?id=refs/tags/next-20130619#n468
and for used macros and bitmasks:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/include/asm/cputype.h?id=refs/tags/next-20130619#n45
Now the structure of the MPIDR register:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0388e/CIHEBGFG.html
As you can see, the value read from the register in
smp_setup_processor_id() is only the physical CPU ID, so I don't see any
problem here.
If I'm wrong, feel free to correct me.
Cc'ing people who should have more knowledge on this.
Best regards,
Tomasz
+ __raw_writel(0, reg_base);
/*
* here's the WFI
@@ -106,7 +107,7 @@ static inline void platform_do_lowpower(unsigned
int
cpu, int *spurious)
: memory, cc);
- if (pen_release == cpu_logical_map(cpu)) {
+ if (pen_release == phys_cpu) {
/*
* OK, proper wakeup, we're done
*/
diff --git a/arch/arm/mach-exynos/include/mach/regs-pmu.h
b/arch/arm/mach-exynos/include/mach/regs-pmu.h index 57344b7..cf40b86
100644
--- a/arch/arm/mach-exynos/include/mach/regs-pmu.h
+++ b/arch/arm/mach-exynos/include/mach/regs-pmu.h
@@ -125,10 +125,14 @@
#define S5P_GPS_ALIVE_LOWPWR S5P_PMUREG(0x13A0)
#define S5P_ARM_CORE0_CONFIGURATION S5P_PMUREG(0x2000)
+#define S5P_ARM_CORE0_STATUS S5P_PMUREG(0x2004)
#define S5P_ARM_CORE0_OPTION S5P_PMUREG(0x2008)
-#define S5P_ARM_CORE1_CONFIGURATIONS5P_PMUREG(0x2080)
-#define S5P_ARM_CORE1_STATUS S5P_PMUREG(0x2084)
-#define S5P_ARM_CORE1_OPTION S5P_PMUREG(0x2088)
+#define S5P_ARM_CORE_CONFIGURATION(_nr)\
+ (S5P_ARM_CORE0_CONFIGURATION + ((_nr) * 0x80))
+#define S5P_ARM_CORE_STATUS(_nr) \
+ (S5P_ARM_CORE0_STATUS + ((_nr) * 0x80))
+#define S5P_ARM_CORE_OPTION(_nr) \
+ (S5P_ARM_CORE0_OPTION + ((_nr) * 0x80))
#define
Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
On Wed, Jun 19, 2013 at 01:50:57PM +0100, Tomasz Figa wrote:
On Wednesday 19 of June 2013 17:39:21 Chander Kashyap wrote:
On 18 June 2013 23:29, Kukjin Kim kgene@samsung.com wrote:
On 06/19/13 02:45, Tomasz Figa wrote:
Ccing Arnd and Olof, because I forgot to add them to git send-email...
Sorry for the noise.
On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
S5P_ARM_CORE1_* registers affect only core 1. To control further
cores
properly another registers must be used.
This patch replaces S5P_ARM_CORE1_* register definitions with
S5P_ARM_CORE_*(x) macro which return addresses of registers for
specified core.
This fixes CPU hotplug on quad core Exynos SoCs on which currently
offlining CPUs 2 or 3 caused CPU 1 to be turned off.
Obviously this doesn't happen currently because of the if (cpu == 1),
but
Yes, not happened...and just note exynos5440 doesn't support hotplug :)
so this is available on exynos4412 and added 5420.
if logical cpu1 turned out not to be physical cpu1, then it would
crash.
Best regards,
Tomasz
In addition,
bring-up of CPU 2 and 3 is fixed on boards where bootloader powers
off
secondary cores by default.
I need to test on board about above...
Cc: sta...@vger.kernel.org
Signed-off-by: Tomasz Figat.f...@samsung.com
Signed-off-by: Kyungmin Parkkyungmin.p...@samsung.com
---
arch/arm/mach-exynos/hotplug.c | 9 +
arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++---
arch/arm/mach-exynos/platsmp.c | 9 +
3 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/arch/arm/mach-exynos/hotplug.c
b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
--- a/arch/arm/mach-exynos/hotplug.c
+++ b/arch/arm/mach-exynos/hotplug.c
@@ -93,10 +93,11 @@ static inline void cpu_leave_lowpower(void)
static inline void platform_do_lowpower(unsigned int cpu, int
*spurious) {
for (;;) {
+ void __iomem *reg_base;
+ unsigned int phys_cpu = cpu_logical_map(cpu);
- /* make cpu1 to be turned off at next WFI command */
- if (cpu == 1)
- __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
+ reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
Tomasz,
This will break for non-zero, MPIDR value. Say if MPIDR is 1 then for
cpu0 phys_cpu value will be 0x100,
and address calculation will be (S5P_ARM_CORE0_CONFIGURATION +
((0x101) * 0x80)), which is wrong.
Honestly, I did not understand the reasoning above, please clarify.
Hmm, according to the code initializing __cpu_logical_map[] array this is
not true.
Here's the code:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/kernel/setup.c?id=refs/tags/next-20130619#n468
and for used macros and bitmasks:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/include/asm/cputype.h?id=refs/tags/next-20130619#n45
Now the structure of the MPIDR register:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0388e/CIHEBGFG.html
As you can see, the value read from the register in
smp_setup_processor_id() is only the physical CPU ID, so I don't see any
problem here.
Define physical CPU ID :-)
There is a problem here: the MPIDR is not an index, and the cpu_logical_map is
populated in arm_dt_init_cpu_maps in:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/kernel/devtree.c?id=refs/tags/v3.10-rc6
with all affinity levels.
You need to perform a mapping between logical cpus and registers offset,
you can't use the cpu_logical_map directly for that.
Next accident waiting to happen is GIC code (CONFIG_GIC_NON_BANKED), where
cpu_logical_map is used erroneously as an index.
Lorenzo
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
On 19 June 2013 18:20, Tomasz Figa t.f...@samsung.com wrote:
On Wednesday 19 of June 2013 17:39:21 Chander Kashyap wrote:
On 18 June 2013 23:29, Kukjin Kim kgene@samsung.com wrote:
On 06/19/13 02:45, Tomasz Figa wrote:
Ccing Arnd and Olof, because I forgot to add them to git send-email...
Sorry for the noise.
On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
S5P_ARM_CORE1_* registers affect only core 1. To control further
cores
properly another registers must be used.
This patch replaces S5P_ARM_CORE1_* register definitions with
S5P_ARM_CORE_*(x) macro which return addresses of registers for
specified core.
This fixes CPU hotplug on quad core Exynos SoCs on which currently
offlining CPUs 2 or 3 caused CPU 1 to be turned off.
Obviously this doesn't happen currently because of the if (cpu == 1),
but
Yes, not happened...and just note exynos5440 doesn't support hotplug :)
so this is available on exynos4412 and added 5420.
if logical cpu1 turned out not to be physical cpu1, then it would
crash.
Best regards,
Tomasz
In addition,
bring-up of CPU 2 and 3 is fixed on boards where bootloader powers
off
secondary cores by default.
I need to test on board about above...
Cc: sta...@vger.kernel.org
Signed-off-by: Tomasz Figat.f...@samsung.com
Signed-off-by: Kyungmin Parkkyungmin.p...@samsung.com
---
arch/arm/mach-exynos/hotplug.c | 9 +
arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++---
arch/arm/mach-exynos/platsmp.c | 9 +
3 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/arch/arm/mach-exynos/hotplug.c
b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
--- a/arch/arm/mach-exynos/hotplug.c
+++ b/arch/arm/mach-exynos/hotplug.c
@@ -93,10 +93,11 @@ static inline void cpu_leave_lowpower(void)
static inline void platform_do_lowpower(unsigned int cpu, int
*spurious) {
for (;;) {
+ void __iomem *reg_base;
+ unsigned int phys_cpu = cpu_logical_map(cpu);
- /* make cpu1 to be turned off at next WFI command */
- if (cpu == 1)
- __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
+ reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
Tomasz,
This will break for non-zero, MPIDR value. Say if MPIDR is 1 then for
cpu0 phys_cpu value will be 0x100,
and address calculation will be (S5P_ARM_CORE0_CONFIGURATION +
((0x101) * 0x80)), which is wrong.
Hmm, according to the code initializing __cpu_logical_map[] array this is
not true.
Here's the code:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/kernel/setup.c?id=refs/tags/next-20130619#n468
and for used macros and bitmasks:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/include/asm/cputype.h?id=refs/tags/next-20130619#n45
Now the structure of the MPIDR register:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0388e/CIHEBGFG.html
As you can see, the value read from the register in
smp_setup_processor_id() is only the physical CPU ID, so I don't see any
problem here.
Kindly look at implementation of arm_dt_init_cpu_maps in
arch/arm/kernel/devtree.c. When you pass the cpu nodes thru DT, and
cluster id is non zero, the phys_cpu will include clusterID. Say e.g
exynos4210 has CLusterID=9, hence phys_cpu value for cpu0 will be
0x901.
If I'm wrong, feel free to correct me.
Cc'ing people who should have more knowledge on this.
Best regards,
Tomasz
+ __raw_writel(0, reg_base);
/*
* here's the WFI
@@ -106,7 +107,7 @@ static inline void platform_do_lowpower(unsigned
int
cpu, int *spurious)
: memory, cc);
- if (pen_release == cpu_logical_map(cpu)) {
+ if (pen_release == phys_cpu) {
/*
* OK, proper wakeup, we're done
*/
diff --git a/arch/arm/mach-exynos/include/mach/regs-pmu.h
b/arch/arm/mach-exynos/include/mach/regs-pmu.h index 57344b7..cf40b86
100644
--- a/arch/arm/mach-exynos/include/mach/regs-pmu.h
+++ b/arch/arm/mach-exynos/include/mach/regs-pmu.h
@@ -125,10 +125,14 @@
#define S5P_GPS_ALIVE_LOWPWR S5P_PMUREG(0x13A0)
#define S5P_ARM_CORE0_CONFIGURATION S5P_PMUREG(0x2000)
+#define S5P_ARM_CORE0_STATUS S5P_PMUREG(0x2004)
#define S5P_ARM_CORE0_OPTION S5P_PMUREG(0x2008)
-#define S5P_ARM_CORE1_CONFIGURATIONS5P_PMUREG(0x2080)
-#define S5P_ARM_CORE1_STATUS S5P_PMUREG(0x2084)
-#define S5P_ARM_CORE1_OPTION S5P_PMUREG(0x2088)
+#define S5P_ARM_CORE_CONFIGURATION(_nr)\
+
Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
On 19 June 2013 18:54, Lorenzo Pieralisi lorenzo.pieral...@arm.com wrote:
On Wed, Jun 19, 2013 at 01:50:57PM +0100, Tomasz Figa wrote:
On Wednesday 19 of June 2013 17:39:21 Chander Kashyap wrote:
On 18 June 2013 23:29, Kukjin Kim kgene@samsung.com wrote:
On 06/19/13 02:45, Tomasz Figa wrote:
Ccing Arnd and Olof, because I forgot to add them to git send-email...
Sorry for the noise.
On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
S5P_ARM_CORE1_* registers affect only core 1. To control further
cores
properly another registers must be used.
This patch replaces S5P_ARM_CORE1_* register definitions with
S5P_ARM_CORE_*(x) macro which return addresses of registers for
specified core.
This fixes CPU hotplug on quad core Exynos SoCs on which currently
offlining CPUs 2 or 3 caused CPU 1 to be turned off.
Obviously this doesn't happen currently because of the if (cpu == 1),
but
Yes, not happened...and just note exynos5440 doesn't support hotplug :)
so this is available on exynos4412 and added 5420.
if logical cpu1 turned out not to be physical cpu1, then it would
crash.
Best regards,
Tomasz
In addition,
bring-up of CPU 2 and 3 is fixed on boards where bootloader powers
off
secondary cores by default.
I need to test on board about above...
Cc: sta...@vger.kernel.org
Signed-off-by: Tomasz Figat.f...@samsung.com
Signed-off-by: Kyungmin Parkkyungmin.p...@samsung.com
---
arch/arm/mach-exynos/hotplug.c | 9 +
arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++---
arch/arm/mach-exynos/platsmp.c | 9 +
3 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/arch/arm/mach-exynos/hotplug.c
b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
--- a/arch/arm/mach-exynos/hotplug.c
+++ b/arch/arm/mach-exynos/hotplug.c
@@ -93,10 +93,11 @@ static inline void cpu_leave_lowpower(void)
static inline void platform_do_lowpower(unsigned int cpu, int
*spurious) {
for (;;) {
+ void __iomem *reg_base;
+ unsigned int phys_cpu = cpu_logical_map(cpu);
- /* make cpu1 to be turned off at next WFI command */
- if (cpu == 1)
- __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
+ reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
Tomasz,
This will break for non-zero, MPIDR value. Say if MPIDR is 1 then for
cpu0 phys_cpu value will be 0x100,
and address calculation will be (S5P_ARM_CORE0_CONFIGURATION +
((0x101) * 0x80)), which is wrong.
Honestly, I did not understand the reasoning above, please clarify.
Sorry not non-zero MPIDR but non-zero ClusterID.
All the power register addresses are at offset 0x80.
Hmm, according to the code initializing __cpu_logical_map[] array this is
not true.
Here's the code:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/kernel/setup.c?id=refs/tags/next-20130619#n468
and for used macros and bitmasks:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/include/asm/cputype.h?id=refs/tags/next-20130619#n45
Now the structure of the MPIDR register:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0388e/CIHEBGFG.html
As you can see, the value read from the register in
smp_setup_processor_id() is only the physical CPU ID, so I don't see any
problem here.
Define physical CPU ID :-)
There is a problem here: the MPIDR is not an index, and the cpu_logical_map is
populated in arm_dt_init_cpu_maps in:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/kernel/devtree.c?id=refs/tags/v3.10-rc6
with all affinity levels.
You need to perform a mapping between logical cpus and registers offset,
you can't use the cpu_logical_map directly for that.
Next accident waiting to happen is GIC code (CONFIG_GIC_NON_BANKED), where
cpu_logical_map is used erroneously as an index.
Lorenzo
--
with warm regards,
Chander Kashyap
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
On Wednesday 19 of June 2013 14:24:17 Lorenzo Pieralisi wrote:
On Wed, Jun 19, 2013 at 01:50:57PM +0100, Tomasz Figa wrote:
On Wednesday 19 of June 2013 17:39:21 Chander Kashyap wrote:
On 18 June 2013 23:29, Kukjin Kim kgene@samsung.com wrote:
On 06/19/13 02:45, Tomasz Figa wrote:
Ccing Arnd and Olof, because I forgot to add them to git
send-email...
Sorry for the noise.
On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
S5P_ARM_CORE1_* registers affect only core 1. To control further
cores
properly another registers must be used.
This patch replaces S5P_ARM_CORE1_* register definitions with
S5P_ARM_CORE_*(x) macro which return addresses of registers for
specified core.
This fixes CPU hotplug on quad core Exynos SoCs on which
currently
offlining CPUs 2 or 3 caused CPU 1 to be turned off.
Obviously this doesn't happen currently because of the if (cpu ==
1),
but
Yes, not happened...and just note exynos5440 doesn't support
hotplug :)
so this is available on exynos4412 and added 5420.
if logical cpu1 turned out not to be physical cpu1, then it would
crash.
Best regards,
Tomasz
In addition,
bring-up of CPU 2 and 3 is fixed on boards where bootloader
powers
off
secondary cores by default.
I need to test on board about above...
Cc: sta...@vger.kernel.org
Signed-off-by: Tomasz Figat.f...@samsung.com
Signed-off-by: Kyungmin Parkkyungmin.p...@samsung.com
---
arch/arm/mach-exynos/hotplug.c | 9 +
arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++---
arch/arm/mach-exynos/platsmp.c | 9 +
3 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/arch/arm/mach-exynos/hotplug.c
b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
--- a/arch/arm/mach-exynos/hotplug.c
+++ b/arch/arm/mach-exynos/hotplug.c
@@ -93,10 +93,11 @@ static inline void cpu_leave_lowpower(void)
static inline void platform_do_lowpower(unsigned int cpu, int
*spurious) {
for (;;) {
+ void __iomem *reg_base;
+ unsigned int phys_cpu = cpu_logical_map(cpu);
- /* make cpu1 to be turned off at next WFI command
*/
- if (cpu == 1)
- __raw_writel(0,
S5P_ARM_CORE1_CONFIGURATION);
+ reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
Tomasz,
This will break for non-zero, MPIDR value. Say if MPIDR is 1 then
for
cpu0 phys_cpu value will be 0x100,
and address calculation will be (S5P_ARM_CORE0_CONFIGURATION +
((0x101) * 0x80)), which is wrong.
Honestly, I did not understand the reasoning above, please clarify.
Hmm, according to the code initializing __cpu_logical_map[] array this
is not true.
Here's the code:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/a
rch/arm/kernel/setup.c?id=refs/tags/next-20130619#n468
and for used macros and bitmasks:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/a
rch/arm/include/asm/cputype.h?id=refs/tags/next-20130619#n45
Now the structure of the MPIDR register:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0388e/CI
HEBGFG.html
As you can see, the value read from the register in
smp_setup_processor_id() is only the physical CPU ID, so I don't see
any
problem here.
Define physical CPU ID :-)
There is a problem here: the MPIDR is not an index, and the
cpu_logical_map is populated in arm_dt_init_cpu_maps in:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch
/arm/kernel/devtree.c?id=refs/tags/v3.10-rc6
with all affinity levels.
OK. This is what I was missing. Thanks.
You need to perform a mapping between logical cpus and registers offset,
you can't use the cpu_logical_map directly for that.
Hmm, can't I just extract cluster ID and CPU ID from the MPIDR value and
use them appropriately to calculate register offsets?
Best regards,
Tomasz
Next accident waiting to happen is GIC code (CONFIG_GIC_NON_BANKED),
where cpu_logical_map is used erroneously as an index.
Lorenzo
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
On 19 June 2013 19:19, Tomasz Figa t.f...@samsung.com wrote:
On Wednesday 19 of June 2013 14:24:17 Lorenzo Pieralisi wrote:
On Wed, Jun 19, 2013 at 01:50:57PM +0100, Tomasz Figa wrote:
On Wednesday 19 of June 2013 17:39:21 Chander Kashyap wrote:
On 18 June 2013 23:29, Kukjin Kim kgene@samsung.com wrote:
On 06/19/13 02:45, Tomasz Figa wrote:
Ccing Arnd and Olof, because I forgot to add them to git
send-email...
Sorry for the noise.
On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
S5P_ARM_CORE1_* registers affect only core 1. To control further
cores
properly another registers must be used.
This patch replaces S5P_ARM_CORE1_* register definitions with
S5P_ARM_CORE_*(x) macro which return addresses of registers for
specified core.
This fixes CPU hotplug on quad core Exynos SoCs on which
currently
offlining CPUs 2 or 3 caused CPU 1 to be turned off.
Obviously this doesn't happen currently because of the if (cpu ==
1),
but
Yes, not happened...and just note exynos5440 doesn't support
hotplug :)
so this is available on exynos4412 and added 5420.
if logical cpu1 turned out not to be physical cpu1, then it would
crash.
Best regards,
Tomasz
In addition,
bring-up of CPU 2 and 3 is fixed on boards where bootloader
powers
off
secondary cores by default.
I need to test on board about above...
Cc: sta...@vger.kernel.org
Signed-off-by: Tomasz Figat.f...@samsung.com
Signed-off-by: Kyungmin Parkkyungmin.p...@samsung.com
---
arch/arm/mach-exynos/hotplug.c | 9 +
arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++---
arch/arm/mach-exynos/platsmp.c | 9 +
3 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/arch/arm/mach-exynos/hotplug.c
b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
--- a/arch/arm/mach-exynos/hotplug.c
+++ b/arch/arm/mach-exynos/hotplug.c
@@ -93,10 +93,11 @@ static inline void cpu_leave_lowpower(void)
static inline void platform_do_lowpower(unsigned int cpu, int
*spurious) {
for (;;) {
+ void __iomem *reg_base;
+ unsigned int phys_cpu = cpu_logical_map(cpu);
- /* make cpu1 to be turned off at next WFI command
*/
- if (cpu == 1)
- __raw_writel(0,
S5P_ARM_CORE1_CONFIGURATION);
+ reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
Tomasz,
This will break for non-zero, MPIDR value. Say if MPIDR is 1 then
for
cpu0 phys_cpu value will be 0x100,
and address calculation will be (S5P_ARM_CORE0_CONFIGURATION +
((0x101) * 0x80)), which is wrong.
Honestly, I did not understand the reasoning above, please clarify.
Hmm, according to the code initializing __cpu_logical_map[] array this
is not true.
Here's the code:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/a
rch/arm/kernel/setup.c?id=refs/tags/next-20130619#n468
and for used macros and bitmasks:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/a
rch/arm/include/asm/cputype.h?id=refs/tags/next-20130619#n45
Now the structure of the MPIDR register:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0388e/CI
HEBGFG.html
As you can see, the value read from the register in
smp_setup_processor_id() is only the physical CPU ID, so I don't see
any
problem here.
Define physical CPU ID :-)
There is a problem here: the MPIDR is not an index, and the
cpu_logical_map is populated in arm_dt_init_cpu_maps in:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch
/arm/kernel/devtree.c?id=refs/tags/v3.10-rc6
with all affinity levels.
OK. This is what I was missing. Thanks.
You need to perform a mapping between logical cpus and registers offset,
you can't use the cpu_logical_map directly for that.
Hmm, can't I just extract cluster ID and CPU ID from the MPIDR value and
use them appropriately to calculate register offsets?
That will create problem for multi-cluster systems. Say we have two
clusters then with clusterID 0 and 1. So phys-cpu will be 0x0/0x100,
0x1/0x101 ans so on.
Best regards,
Tomasz
Next accident waiting to happen is GIC code (CONFIG_GIC_NON_BANKED),
where cpu_logical_map is used erroneously as an index.
Lorenzo
--
with warm regards,
Chander Kashyap
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
On Wednesday 19 of June 2013 19:25:27 Chander Kashyap wrote:
On 19 June 2013 19:19, Tomasz Figa t.f...@samsung.com wrote:
On Wednesday 19 of June 2013 14:24:17 Lorenzo Pieralisi wrote:
On Wed, Jun 19, 2013 at 01:50:57PM +0100, Tomasz Figa wrote:
On Wednesday 19 of June 2013 17:39:21 Chander Kashyap wrote:
On 18 June 2013 23:29, Kukjin Kim kgene@samsung.com wrote:
On 06/19/13 02:45, Tomasz Figa wrote:
Ccing Arnd and Olof, because I forgot to add them to git
send-email...
Sorry for the noise.
On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
S5P_ARM_CORE1_* registers affect only core 1. To control
further
cores
properly another registers must be used.
This patch replaces S5P_ARM_CORE1_* register definitions with
S5P_ARM_CORE_*(x) macro which return addresses of registers
for
specified core.
This fixes CPU hotplug on quad core Exynos SoCs on which
currently
offlining CPUs 2 or 3 caused CPU 1 to be turned off.
Obviously this doesn't happen currently because of the if (cpu
==
1),
but
Yes, not happened...and just note exynos5440 doesn't support
hotplug :)
so this is available on exynos4412 and added 5420.
if logical cpu1 turned out not to be physical cpu1, then it
would
crash.
Best regards,
Tomasz
In addition,
bring-up of CPU 2 and 3 is fixed on boards where bootloader
powers
off
secondary cores by default.
I need to test on board about above...
Cc: sta...@vger.kernel.org
Signed-off-by: Tomasz Figat.f...@samsung.com
Signed-off-by: Kyungmin Parkkyungmin.p...@samsung.com
---
arch/arm/mach-exynos/hotplug.c | 9 +
arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++---
arch/arm/mach-exynos/platsmp.c | 9 +
3 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/arch/arm/mach-exynos/hotplug.c
b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
--- a/arch/arm/mach-exynos/hotplug.c
+++ b/arch/arm/mach-exynos/hotplug.c
@@ -93,10 +93,11 @@ static inline void
cpu_leave_lowpower(void)
static inline void platform_do_lowpower(unsigned int cpu,
int
*spurious) {
for (;;) {
+ void __iomem *reg_base;
+ unsigned int phys_cpu = cpu_logical_map(cpu);
- /* make cpu1 to be turned off at next WFI
command
*/
- if (cpu == 1)
- __raw_writel(0,
S5P_ARM_CORE1_CONFIGURATION);
+ reg_base =
S5P_ARM_CORE_CONFIGURATION(phys_cpu);
Tomasz,
This will break for non-zero, MPIDR value. Say if MPIDR is 1 then
for
cpu0 phys_cpu value will be 0x100,
and address calculation will be (S5P_ARM_CORE0_CONFIGURATION +
((0x101) * 0x80)), which is wrong.
Honestly, I did not understand the reasoning above, please clarify.
Hmm, according to the code initializing __cpu_logical_map[] array
this
is not true.
Here's the code:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tre
e/a
rch/arm/kernel/setup.c?id=refs/tags/next-20130619#n468
and for used macros and bitmasks:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tre
e/a
rch/arm/include/asm/cputype.h?id=refs/tags/next-20130619#n45
Now the structure of the MPIDR register:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0388e
/CI
HEBGFG.html
As you can see, the value read from the register in
smp_setup_processor_id() is only the physical CPU ID, so I don't see
any
problem here.
Define physical CPU ID :-)
There is a problem here: the MPIDR is not an index, and the
cpu_logical_map is populated in arm_dt_init_cpu_maps in:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/a
rch /arm/kernel/devtree.c?id=refs/tags/v3.10-rc6
with all affinity levels.
OK. This is what I was missing. Thanks.
You need to perform a mapping between logical cpus and registers
offset,
you can't use the cpu_logical_map directly for that.
Hmm, can't I just extract cluster ID and CPU ID from the MPIDR value
and
use them appropriately to calculate register offsets?
That will create problem for multi-cluster systems. Say we have two
clusters then with clusterID 0 and 1. So phys-cpu will be 0x0/0x100,
0x1/0x101 ans so on.
I mean, calculate register offset based on two parameters - cluster ID and
CPU ID, like:
...
u32 mpidr = cpu_logical_map(cpu);
u32 phys_cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
if (soc_is_exynos()) {
u32 cluster =
Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
On 19 June 2013 19:58, Tomasz Figa t.f...@samsung.com wrote:
On Wednesday 19 of June 2013 19:25:27 Chander Kashyap wrote:
On 19 June 2013 19:19, Tomasz Figa t.f...@samsung.com wrote:
On Wednesday 19 of June 2013 14:24:17 Lorenzo Pieralisi wrote:
On Wed, Jun 19, 2013 at 01:50:57PM +0100, Tomasz Figa wrote:
On Wednesday 19 of June 2013 17:39:21 Chander Kashyap wrote:
On 18 June 2013 23:29, Kukjin Kim kgene@samsung.com wrote:
On 06/19/13 02:45, Tomasz Figa wrote:
Ccing Arnd and Olof, because I forgot to add them to git
send-email...
Sorry for the noise.
On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
S5P_ARM_CORE1_* registers affect only core 1. To control
further
cores
properly another registers must be used.
This patch replaces S5P_ARM_CORE1_* register definitions with
S5P_ARM_CORE_*(x) macro which return addresses of registers
for
specified core.
This fixes CPU hotplug on quad core Exynos SoCs on which
currently
offlining CPUs 2 or 3 caused CPU 1 to be turned off.
Obviously this doesn't happen currently because of the if (cpu
==
1),
but
Yes, not happened...and just note exynos5440 doesn't support
hotplug :)
so this is available on exynos4412 and added 5420.
if logical cpu1 turned out not to be physical cpu1, then it
would
crash.
Best regards,
Tomasz
In addition,
bring-up of CPU 2 and 3 is fixed on boards where bootloader
powers
off
secondary cores by default.
I need to test on board about above...
Cc: sta...@vger.kernel.org
Signed-off-by: Tomasz Figat.f...@samsung.com
Signed-off-by: Kyungmin Parkkyungmin.p...@samsung.com
---
arch/arm/mach-exynos/hotplug.c | 9 +
arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++---
arch/arm/mach-exynos/platsmp.c | 9 +
3 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/arch/arm/mach-exynos/hotplug.c
b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
--- a/arch/arm/mach-exynos/hotplug.c
+++ b/arch/arm/mach-exynos/hotplug.c
@@ -93,10 +93,11 @@ static inline void
cpu_leave_lowpower(void)
static inline void platform_do_lowpower(unsigned int cpu,
int
*spurious) {
for (;;) {
+ void __iomem *reg_base;
+ unsigned int phys_cpu = cpu_logical_map(cpu);
- /* make cpu1 to be turned off at next WFI
command
*/
- if (cpu == 1)
- __raw_writel(0,
S5P_ARM_CORE1_CONFIGURATION);
+ reg_base =
S5P_ARM_CORE_CONFIGURATION(phys_cpu);
Tomasz,
This will break for non-zero, MPIDR value. Say if MPIDR is 1 then
for
cpu0 phys_cpu value will be 0x100,
and address calculation will be (S5P_ARM_CORE0_CONFIGURATION +
((0x101) * 0x80)), which is wrong.
Honestly, I did not understand the reasoning above, please clarify.
Hmm, according to the code initializing __cpu_logical_map[] array
this
is not true.
Here's the code:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tre
e/a
rch/arm/kernel/setup.c?id=refs/tags/next-20130619#n468
and for used macros and bitmasks:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tre
e/a
rch/arm/include/asm/cputype.h?id=refs/tags/next-20130619#n45
Now the structure of the MPIDR register:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0388e
/CI
HEBGFG.html
As you can see, the value read from the register in
smp_setup_processor_id() is only the physical CPU ID, so I don't see
any
problem here.
Define physical CPU ID :-)
There is a problem here: the MPIDR is not an index, and the
cpu_logical_map is populated in arm_dt_init_cpu_maps in:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/a
rch /arm/kernel/devtree.c?id=refs/tags/v3.10-rc6
with all affinity levels.
OK. This is what I was missing. Thanks.
You need to perform a mapping between logical cpus and registers
offset,
you can't use the cpu_logical_map directly for that.
Hmm, can't I just extract cluster ID and CPU ID from the MPIDR value
and
use them appropriately to calculate register offsets?
That will create problem for multi-cluster systems. Say we have two
clusters then with clusterID 0 and 1. So phys-cpu will be 0x0/0x100,
0x1/0x101 ans so on.
I mean, calculate register offset based on two parameters - cluster ID and
CPU ID, like:
...
u32 mpidr = cpu_logical_map(cpu);
u32 phys_cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
if (soc_is_exynos()) {
u32
Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
On Wednesday 19 of June 2013 20:26:50 Chander Kashyap wrote:
On 19 June 2013 19:58, Tomasz Figa t.f...@samsung.com wrote:
On Wednesday 19 of June 2013 19:25:27 Chander Kashyap wrote:
On 19 June 2013 19:19, Tomasz Figa t.f...@samsung.com wrote:
On Wednesday 19 of June 2013 14:24:17 Lorenzo Pieralisi wrote:
On Wed, Jun 19, 2013 at 01:50:57PM +0100, Tomasz Figa wrote:
On Wednesday 19 of June 2013 17:39:21 Chander Kashyap wrote:
On 18 June 2013 23:29, Kukjin Kim kgene@samsung.com
wrote:
On 06/19/13 02:45, Tomasz Figa wrote:
Ccing Arnd and Olof, because I forgot to add them to git
send-email...
Sorry for the noise.
On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
S5P_ARM_CORE1_* registers affect only core 1. To control
further
cores
properly another registers must be used.
This patch replaces S5P_ARM_CORE1_* register definitions
with
S5P_ARM_CORE_*(x) macro which return addresses of registers
for
specified core.
This fixes CPU hotplug on quad core Exynos SoCs on which
currently
offlining CPUs 2 or 3 caused CPU 1 to be turned off.
Obviously this doesn't happen currently because of the if
(cpu
==
1),
but
Yes, not happened...and just note exynos5440 doesn't support
hotplug :)
so this is available on exynos4412 and added 5420.
if logical cpu1 turned out not to be physical cpu1, then it
would
crash.
Best regards,
Tomasz
In addition,
bring-up of CPU 2 and 3 is fixed on boards where bootloader
powers
off
secondary cores by default.
I need to test on board about above...
Cc: sta...@vger.kernel.org
Signed-off-by: Tomasz Figat.f...@samsung.com
Signed-off-by: Kyungmin Parkkyungmin.p...@samsung.com
---
arch/arm/mach-exynos/hotplug.c | 9
+
arch/arm/mach-exynos/include/mach/regs-pmu.h | 10
+++---
arch/arm/mach-exynos/platsmp.c | 9
+
3 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/arch/arm/mach-exynos/hotplug.c
b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943
100644
--- a/arch/arm/mach-exynos/hotplug.c
+++ b/arch/arm/mach-exynos/hotplug.c
@@ -93,10 +93,11 @@ static inline void
cpu_leave_lowpower(void)
static inline void platform_do_lowpower(unsigned int cpu,
int
*spurious) {
for (;;) {
+ void __iomem *reg_base;
+ unsigned int phys_cpu =
cpu_logical_map(cpu);
- /* make cpu1 to be turned off at next WFI
command
*/
- if (cpu == 1)
- __raw_writel(0,
S5P_ARM_CORE1_CONFIGURATION);
+ reg_base =
S5P_ARM_CORE_CONFIGURATION(phys_cpu);
Tomasz,
This will break for non-zero, MPIDR value. Say if MPIDR is 1
then
for
cpu0 phys_cpu value will be 0x100,
and address calculation will be (S5P_ARM_CORE0_CONFIGURATION
+
((0x101) * 0x80)), which is wrong.
Honestly, I did not understand the reasoning above, please clarify.
Hmm, according to the code initializing __cpu_logical_map[] array
this
is not true.
Here's the code:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/
tre
e/a
rch/arm/kernel/setup.c?id=refs/tags/next-20130619#n468
and for used macros and bitmasks:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/
tre
e/a
rch/arm/include/asm/cputype.h?id=refs/tags/next-20130619#n45
Now the structure of the MPIDR register:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi03
88e
/CI
HEBGFG.html
As you can see, the value read from the register in
smp_setup_processor_id() is only the physical CPU ID, so I don't
see
any
problem here.
Define physical CPU ID :-)
There is a problem here: the MPIDR is not an index, and the
cpu_logical_map is populated in arm_dt_init_cpu_maps in:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tre
e/a
rch /arm/kernel/devtree.c?id=refs/tags/v3.10-rc6
with all affinity levels.
OK. This is what I was missing. Thanks.
You need to perform a mapping between logical cpus and registers
offset,
you can't use the cpu_logical_map directly for that.
Hmm, can't I just extract cluster ID and CPU ID from the MPIDR value
and
use them appropriately to calculate register offsets?
That will create problem for multi-cluster systems. Say we have two
clusters then with clusterID
Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
On 19 June 2013 20:31, Tomasz Figa t.f...@samsung.com wrote:
On Wednesday 19 of June 2013 20:26:50 Chander Kashyap wrote:
On 19 June 2013 19:58, Tomasz Figa t.f...@samsung.com wrote:
On Wednesday 19 of June 2013 19:25:27 Chander Kashyap wrote:
On 19 June 2013 19:19, Tomasz Figa t.f...@samsung.com wrote:
On Wednesday 19 of June 2013 14:24:17 Lorenzo Pieralisi wrote:
On Wed, Jun 19, 2013 at 01:50:57PM +0100, Tomasz Figa wrote:
On Wednesday 19 of June 2013 17:39:21 Chander Kashyap wrote:
On 18 June 2013 23:29, Kukjin Kim kgene@samsung.com
wrote:
On 06/19/13 02:45, Tomasz Figa wrote:
Ccing Arnd and Olof, because I forgot to add them to git
send-email...
Sorry for the noise.
On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
S5P_ARM_CORE1_* registers affect only core 1. To control
further
cores
properly another registers must be used.
This patch replaces S5P_ARM_CORE1_* register definitions
with
S5P_ARM_CORE_*(x) macro which return addresses of registers
for
specified core.
This fixes CPU hotplug on quad core Exynos SoCs on which
currently
offlining CPUs 2 or 3 caused CPU 1 to be turned off.
Obviously this doesn't happen currently because of the if
(cpu
==
1),
but
Yes, not happened...and just note exynos5440 doesn't support
hotplug :)
so this is available on exynos4412 and added 5420.
if logical cpu1 turned out not to be physical cpu1, then it
would
crash.
Best regards,
Tomasz
In addition,
bring-up of CPU 2 and 3 is fixed on boards where bootloader
powers
off
secondary cores by default.
I need to test on board about above...
Cc: sta...@vger.kernel.org
Signed-off-by: Tomasz Figat.f...@samsung.com
Signed-off-by: Kyungmin Parkkyungmin.p...@samsung.com
---
arch/arm/mach-exynos/hotplug.c | 9
+
arch/arm/mach-exynos/include/mach/regs-pmu.h | 10
+++---
arch/arm/mach-exynos/platsmp.c | 9
+
3 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/arch/arm/mach-exynos/hotplug.c
b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943
100644
--- a/arch/arm/mach-exynos/hotplug.c
+++ b/arch/arm/mach-exynos/hotplug.c
@@ -93,10 +93,11 @@ static inline void
cpu_leave_lowpower(void)
static inline void platform_do_lowpower(unsigned int cpu,
int
*spurious) {
for (;;) {
+ void __iomem *reg_base;
+ unsigned int phys_cpu =
cpu_logical_map(cpu);
- /* make cpu1 to be turned off at next WFI
command
*/
- if (cpu == 1)
- __raw_writel(0,
S5P_ARM_CORE1_CONFIGURATION);
+ reg_base =
S5P_ARM_CORE_CONFIGURATION(phys_cpu);
Tomasz,
This will break for non-zero, MPIDR value. Say if MPIDR is 1
then
for
cpu0 phys_cpu value will be 0x100,
and address calculation will be (S5P_ARM_CORE0_CONFIGURATION
+
((0x101) * 0x80)), which is wrong.
Honestly, I did not understand the reasoning above, please clarify.
Hmm, according to the code initializing __cpu_logical_map[] array
this
is not true.
Here's the code:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/
tre
e/a
rch/arm/kernel/setup.c?id=refs/tags/next-20130619#n468
and for used macros and bitmasks:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/
tre
e/a
rch/arm/include/asm/cputype.h?id=refs/tags/next-20130619#n45
Now the structure of the MPIDR register:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi03
88e
/CI
HEBGFG.html
As you can see, the value read from the register in
smp_setup_processor_id() is only the physical CPU ID, so I don't
see
any
problem here.
Define physical CPU ID :-)
There is a problem here: the MPIDR is not an index, and the
cpu_logical_map is populated in arm_dt_init_cpu_maps in:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tre
e/a
rch /arm/kernel/devtree.c?id=refs/tags/v3.10-rc6
with all affinity levels.
OK. This is what I was missing. Thanks.
You need to perform a mapping between logical cpus and registers
offset,
you can't use the cpu_logical_map directly for that.
Hmm, can't I just extract cluster ID and CPU ID from the MPIDR value
and
use them appropriately to calculate register offsets?
That will create problem for multi-cluster systems. Say we have two
Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
On Wed, 19 Jun 2013, Tomasz Figa wrote:
On Wednesday 19 of June 2013 20:26:50 Chander Kashyap wrote:
On 19 June 2013 19:58, Tomasz Figa t.f...@samsung.com wrote:
I mean, calculate register offset based on two parameters - cluster ID
and
CPU ID, like:
...
u32 mpidr = cpu_logical_map(cpu);
u32 phys_cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
if (soc_is_exynos()) {
u32 cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
phys_cpu += EXYNOS_CPUS_PER_CLUSTER * cluster;
}
reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
__raw_writel(0, reg_base);
This does not seems to viable solution, as eg. clusterID for
exynos4210 is 0x9 and exynos 4412 is 0xa.
We don't need to consider cluster ID for any SoC that has just one cluster.
That's why there is the if (soc_is_exynos()) clause, where exynos
is the SoC that we support and has more clusters.
But if we wass the cpu nodes
thru DT, the we can comfortably rely on the logical cpu number. Also
EXYNOS_CPUS_PER_CLUSTER can vary from cluster to cluster.
There is nothing that prevents you from specifying the CPUs in DT in
different order. Moreover, even if you specify them in correct order, there
is nothing that prevents you from using any of the listed CPUs as boot CPU,
which will get the logical ID of 0.
Relying on the logical CPU number to index into hardware related
register space is wrong, please don't do that.
If the MPIDR allocation isn't linear then this cannot be used either.
The best solution is probably to add this reg_base as a property of each
CPU node in DT, and extract it at boot time to stash it into an array
which can be indexed with the logical CPU number afterwards.
Nicolas
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
On 06/19/13 02:45, Tomasz Figa wrote:
Ccing Arnd and Olof, because I forgot to add them to git send-email...
Sorry for the noise.
On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
S5P_ARM_CORE1_* registers affect only core 1. To control further cores
properly another registers must be used.
This patch replaces S5P_ARM_CORE1_* register definitions with
S5P_ARM_CORE_*(x) macro which return addresses of registers for
specified core.
This fixes CPU hotplug on quad core Exynos SoCs on which currently
offlining CPUs 2 or 3 caused CPU 1 to be turned off.
Obviously this doesn't happen currently because of the if (cpu == 1), but
Yes, not happened...and just note exynos5440 doesn't support hotplug :)
so this is available on exynos4412 and added 5420.
if logical cpu1 turned out not to be physical cpu1, then it would crash.
Best regards,
Tomasz
In addition,
bring-up of CPU 2 and 3 is fixed on boards where bootloader powers off
secondary cores by default.
I need to test on board about above...
Cc: sta...@vger.kernel.org
Signed-off-by: Tomasz Figa
Signed-off-by: Kyungmin Park
---
arch/arm/mach-exynos/hotplug.c | 9 +
arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++---
arch/arm/mach-exynos/platsmp.c | 9 +
3 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/arch/arm/mach-exynos/hotplug.c
b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
--- a/arch/arm/mach-exynos/hotplug.c
+++ b/arch/arm/mach-exynos/hotplug.c
@@ -93,10 +93,11 @@ static inline void cpu_leave_lowpower(void)
static inline void platform_do_lowpower(unsigned int cpu, int
*spurious) {
for (;;) {
+ void __iomem *reg_base;
+ unsigned int phys_cpu = cpu_logical_map(cpu);
- /* make cpu1 to be turned off at next WFI command */
- if (cpu == 1)
- __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
+ reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
+ __raw_writel(0, reg_base);
/*
* here's the WFI
@@ -106,7 +107,7 @@ static inline void platform_do_lowpower(unsigned int
cpu, int *spurious)
: "memory", "cc");
- if (pen_release == cpu_logical_map(cpu)) {
+ if (pen_release == phys_cpu) {
/*
* OK, proper wakeup, we're done
*/
diff --git a/arch/arm/mach-exynos/include/mach/regs-pmu.h
b/arch/arm/mach-exynos/include/mach/regs-pmu.h index 57344b7..cf40b86
100644
--- a/arch/arm/mach-exynos/include/mach/regs-pmu.h
+++ b/arch/arm/mach-exynos/include/mach/regs-pmu.h
@@ -125,10 +125,14 @@
#define S5P_GPS_ALIVE_LOWPWR S5P_PMUREG(0x13A0)
#define S5P_ARM_CORE0_CONFIGURATION S5P_PMUREG(0x2000)
+#define S5P_ARM_CORE0_STATUS S5P_PMUREG(0x2004)
#define S5P_ARM_CORE0_OPTION S5P_PMUREG(0x2008)
-#define S5P_ARM_CORE1_CONFIGURATIONS5P_PMUREG(0x2080)
-#define S5P_ARM_CORE1_STATUS S5P_PMUREG(0x2084)
-#define S5P_ARM_CORE1_OPTION S5P_PMUREG(0x2088)
+#define S5P_ARM_CORE_CONFIGURATION(_nr)\
+ (S5P_ARM_CORE0_CONFIGURATION + ((_nr) * 0x80))
+#define S5P_ARM_CORE_STATUS(_nr) \
+ (S5P_ARM_CORE0_STATUS + ((_nr) * 0x80))
+#define S5P_ARM_CORE_OPTION(_nr) \
+ (S5P_ARM_CORE0_OPTION + ((_nr) * 0x80))
#define S5P_ARM_COMMON_OPTION S5P_PMUREG(0x2408)
#define S5P_TOP_PWR_OPTIONS5P_PMUREG(0x2C48)
diff --git a/arch/arm/mach-exynos/platsmp.c
b/arch/arm/mach-exynos/platsmp.c index d9c6d0a..2cbabc8 100644
--- a/arch/arm/mach-exynos/platsmp.c
+++ b/arch/arm/mach-exynos/platsmp.c
@@ -109,14 +109,15 @@ static int __cpuinit
exynos_boot_secondary(unsigned int cpu, struct task_struct */
write_pen_release(phys_cpu);
- if (!(__raw_readl(S5P_ARM_CORE1_STATUS)& S5P_CORE_LOCAL_PWR_EN))
{
+ if (!(__raw_readl(S5P_ARM_CORE_STATUS(phys_cpu))
+ & S5P_CORE_LOCAL_PWR_EN)) {
__raw_writel(S5P_CORE_LOCAL_PWR_EN,
-S5P_ARM_CORE1_CONFIGURATION);
+S5P_ARM_CORE_CONFIGURATION(phys_cpu));
timeout = 10;
/* wait max 10 ms until cpu1 is on */
- while ((__raw_readl(S5P_ARM_CORE1_STATUS)
+ while ((__raw_readl(S5P_ARM_CORE_STATUS(phys_cpu))
& S5P_CORE_LOCAL_PWR_EN) != S5P_CORE_LOCAL_PWR_EN)
{
if (timeout-- == 0)
break;
@@ -125,7 +126,7 @@ static int __cpuinit exynos_boot_secondary(unsigned
int cpu, struct task_struct }
if (timeout == 0) {
- printk(KERN_ERR "cpu1 power enable failed");
+ printk(KERN_ERR "cpu%u power enable
Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
Ccing Arnd and Olof, because I forgot to add them to git send-email...
Sorry for the noise.
On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
> S5P_ARM_CORE1_* registers affect only core 1. To control further cores
> properly another registers must be used.
>
> This patch replaces S5P_ARM_CORE1_* register definitions with
> S5P_ARM_CORE_*(x) macro which return addresses of registers for
> specified core.
>
> This fixes CPU hotplug on quad core Exynos SoCs on which currently
> offlining CPUs 2 or 3 caused CPU 1 to be turned off.
Obviously this doesn't happen currently because of the if (cpu == 1), but
if logical cpu1 turned out not to be physical cpu1, then it would crash.
Best regards,
Tomasz
> In addition,
> bring-up of CPU 2 and 3 is fixed on boards where bootloader powers off
> secondary cores by default.
>
> Cc: sta...@vger.kernel.org
> Signed-off-by: Tomasz Figa
> Signed-off-by: Kyungmin Park
> ---
> arch/arm/mach-exynos/hotplug.c | 9 +
> arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++---
> arch/arm/mach-exynos/platsmp.c | 9 +
> 3 files changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/arch/arm/mach-exynos/hotplug.c
> b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
> --- a/arch/arm/mach-exynos/hotplug.c
> +++ b/arch/arm/mach-exynos/hotplug.c
> @@ -93,10 +93,11 @@ static inline void cpu_leave_lowpower(void)
> static inline void platform_do_lowpower(unsigned int cpu, int
> *spurious) {
> for (;;) {
> + void __iomem *reg_base;
> + unsigned int phys_cpu = cpu_logical_map(cpu);
>
> - /* make cpu1 to be turned off at next WFI command */
> - if (cpu == 1)
> - __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
> + reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
> + __raw_writel(0, reg_base);
>
> /*
>* here's the WFI
> @@ -106,7 +107,7 @@ static inline void platform_do_lowpower(unsigned int
> cpu, int *spurious)
> : "memory", "cc");
>
> - if (pen_release == cpu_logical_map(cpu)) {
> + if (pen_release == phys_cpu) {
> /*
>* OK, proper wakeup, we're done
>*/
> diff --git a/arch/arm/mach-exynos/include/mach/regs-pmu.h
> b/arch/arm/mach-exynos/include/mach/regs-pmu.h index 57344b7..cf40b86
> 100644
> --- a/arch/arm/mach-exynos/include/mach/regs-pmu.h
> +++ b/arch/arm/mach-exynos/include/mach/regs-pmu.h
> @@ -125,10 +125,14 @@
> #define S5P_GPS_ALIVE_LOWPWR S5P_PMUREG(0x13A0)
>
> #define S5P_ARM_CORE0_CONFIGURATION S5P_PMUREG(0x2000)
> +#define S5P_ARM_CORE0_STATUS S5P_PMUREG(0x2004)
> #define S5P_ARM_CORE0_OPTION S5P_PMUREG(0x2008)
> -#define S5P_ARM_CORE1_CONFIGURATION S5P_PMUREG(0x2080)
> -#define S5P_ARM_CORE1_STATUS S5P_PMUREG(0x2084)
> -#define S5P_ARM_CORE1_OPTION S5P_PMUREG(0x2088)
> +#define S5P_ARM_CORE_CONFIGURATION(_nr) \
> + (S5P_ARM_CORE0_CONFIGURATION + ((_nr) * 0x80))
> +#define S5P_ARM_CORE_STATUS(_nr) \
> + (S5P_ARM_CORE0_STATUS + ((_nr) * 0x80))
> +#define S5P_ARM_CORE_OPTION(_nr) \
> + (S5P_ARM_CORE0_OPTION + ((_nr) * 0x80))
>
> #define S5P_ARM_COMMON_OPTIONS5P_PMUREG(0x2408)
> #define S5P_TOP_PWR_OPTION S5P_PMUREG(0x2C48)
> diff --git a/arch/arm/mach-exynos/platsmp.c
> b/arch/arm/mach-exynos/platsmp.c index d9c6d0a..2cbabc8 100644
> --- a/arch/arm/mach-exynos/platsmp.c
> +++ b/arch/arm/mach-exynos/platsmp.c
> @@ -109,14 +109,15 @@ static int __cpuinit
> exynos_boot_secondary(unsigned int cpu, struct task_struct */
> write_pen_release(phys_cpu);
>
> - if (!(__raw_readl(S5P_ARM_CORE1_STATUS) & S5P_CORE_LOCAL_PWR_EN))
{
> + if (!(__raw_readl(S5P_ARM_CORE_STATUS(phys_cpu))
> + & S5P_CORE_LOCAL_PWR_EN)) {
> __raw_writel(S5P_CORE_LOCAL_PWR_EN,
> - S5P_ARM_CORE1_CONFIGURATION);
> + S5P_ARM_CORE_CONFIGURATION(phys_cpu));
>
> timeout = 10;
>
> /* wait max 10 ms until cpu1 is on */
> - while ((__raw_readl(S5P_ARM_CORE1_STATUS)
> + while ((__raw_readl(S5P_ARM_CORE_STATUS(phys_cpu))
> & S5P_CORE_LOCAL_PWR_EN) != S5P_CORE_LOCAL_PWR_EN)
{
> if (timeout-- == 0)
> break;
> @@ -125,7 +126,7 @@ static int __cpuinit exynos_boot_secondary(unsigned
> int cpu, struct task_struct }
>
> if (timeout == 0) {
> - printk(KERN_ERR "cpu1 power enable failed");
> + printk(KERN_ERR "cpu%u power enable failed", cpu);
> spin_unlock(_lock);
> return -ETIMEDOUT;
>
[PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
S5P_ARM_CORE1_* registers affect only core 1. To control further cores
properly another registers must be used.
This patch replaces S5P_ARM_CORE1_* register definitions with
S5P_ARM_CORE_*(x) macro which return addresses of registers for specified
core.
This fixes CPU hotplug on quad core Exynos SoCs on which currently
offlining CPUs 2 or 3 caused CPU 1 to be turned off. In addition,
bring-up of CPU 2 and 3 is fixed on boards where bootloader powers off
secondary cores by default.
Cc: sta...@vger.kernel.org
Signed-off-by: Tomasz Figa
Signed-off-by: Kyungmin Park
---
arch/arm/mach-exynos/hotplug.c | 9 +
arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++---
arch/arm/mach-exynos/platsmp.c | 9 +
3 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/arch/arm/mach-exynos/hotplug.c b/arch/arm/mach-exynos/hotplug.c
index af90cfa..c089943 100644
--- a/arch/arm/mach-exynos/hotplug.c
+++ b/arch/arm/mach-exynos/hotplug.c
@@ -93,10 +93,11 @@ static inline void cpu_leave_lowpower(void)
static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
{
for (;;) {
+ void __iomem *reg_base;
+ unsigned int phys_cpu = cpu_logical_map(cpu);
- /* make cpu1 to be turned off at next WFI command */
- if (cpu == 1)
- __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
+ reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
+ __raw_writel(0, reg_base);
/*
* here's the WFI
@@ -106,7 +107,7 @@ static inline void platform_do_lowpower(unsigned int cpu,
int *spurious)
:
: "memory", "cc");
- if (pen_release == cpu_logical_map(cpu)) {
+ if (pen_release == phys_cpu) {
/*
* OK, proper wakeup, we're done
*/
diff --git a/arch/arm/mach-exynos/include/mach/regs-pmu.h
b/arch/arm/mach-exynos/include/mach/regs-pmu.h
index 57344b7..cf40b86 100644
--- a/arch/arm/mach-exynos/include/mach/regs-pmu.h
+++ b/arch/arm/mach-exynos/include/mach/regs-pmu.h
@@ -125,10 +125,14 @@
#define S5P_GPS_ALIVE_LOWPWR S5P_PMUREG(0x13A0)
#define S5P_ARM_CORE0_CONFIGURATIONS5P_PMUREG(0x2000)
+#define S5P_ARM_CORE0_STATUS S5P_PMUREG(0x2004)
#define S5P_ARM_CORE0_OPTION S5P_PMUREG(0x2008)
-#define S5P_ARM_CORE1_CONFIGURATIONS5P_PMUREG(0x2080)
-#define S5P_ARM_CORE1_STATUS S5P_PMUREG(0x2084)
-#define S5P_ARM_CORE1_OPTION S5P_PMUREG(0x2088)
+#define S5P_ARM_CORE_CONFIGURATION(_nr)\
+ (S5P_ARM_CORE0_CONFIGURATION + ((_nr) * 0x80))
+#define S5P_ARM_CORE_STATUS(_nr) \
+ (S5P_ARM_CORE0_STATUS + ((_nr) * 0x80))
+#define S5P_ARM_CORE_OPTION(_nr) \
+ (S5P_ARM_CORE0_OPTION + ((_nr) * 0x80))
#define S5P_ARM_COMMON_OPTION S5P_PMUREG(0x2408)
#define S5P_TOP_PWR_OPTION S5P_PMUREG(0x2C48)
diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c
index d9c6d0a..2cbabc8 100644
--- a/arch/arm/mach-exynos/platsmp.c
+++ b/arch/arm/mach-exynos/platsmp.c
@@ -109,14 +109,15 @@ static int __cpuinit exynos_boot_secondary(unsigned int
cpu, struct task_struct
*/
write_pen_release(phys_cpu);
- if (!(__raw_readl(S5P_ARM_CORE1_STATUS) & S5P_CORE_LOCAL_PWR_EN)) {
+ if (!(__raw_readl(S5P_ARM_CORE_STATUS(phys_cpu))
+ & S5P_CORE_LOCAL_PWR_EN)) {
__raw_writel(S5P_CORE_LOCAL_PWR_EN,
-S5P_ARM_CORE1_CONFIGURATION);
+S5P_ARM_CORE_CONFIGURATION(phys_cpu));
timeout = 10;
/* wait max 10 ms until cpu1 is on */
- while ((__raw_readl(S5P_ARM_CORE1_STATUS)
+ while ((__raw_readl(S5P_ARM_CORE_STATUS(phys_cpu))
& S5P_CORE_LOCAL_PWR_EN) != S5P_CORE_LOCAL_PWR_EN) {
if (timeout-- == 0)
break;
@@ -125,7 +126,7 @@ static int __cpuinit exynos_boot_secondary(unsigned int
cpu, struct task_struct
}
if (timeout == 0) {
- printk(KERN_ERR "cpu1 power enable failed");
+ printk(KERN_ERR "cpu%u power enable failed", cpu);
spin_unlock(_lock);
return -ETIMEDOUT;
}
--
1.8.2.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
S5P_ARM_CORE1_* registers affect only core 1. To control further cores
properly another registers must be used.
This patch replaces S5P_ARM_CORE1_* register definitions with
S5P_ARM_CORE_*(x) macro which return addresses of registers for specified
core.
This fixes CPU hotplug on quad core Exynos SoCs on which currently
offlining CPUs 2 or 3 caused CPU 1 to be turned off. In addition,
bring-up of CPU 2 and 3 is fixed on boards where bootloader powers off
secondary cores by default.
Cc: sta...@vger.kernel.org
Signed-off-by: Tomasz Figa t.f...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
arch/arm/mach-exynos/hotplug.c | 9 +
arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++---
arch/arm/mach-exynos/platsmp.c | 9 +
3 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/arch/arm/mach-exynos/hotplug.c b/arch/arm/mach-exynos/hotplug.c
index af90cfa..c089943 100644
--- a/arch/arm/mach-exynos/hotplug.c
+++ b/arch/arm/mach-exynos/hotplug.c
@@ -93,10 +93,11 @@ static inline void cpu_leave_lowpower(void)
static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
{
for (;;) {
+ void __iomem *reg_base;
+ unsigned int phys_cpu = cpu_logical_map(cpu);
- /* make cpu1 to be turned off at next WFI command */
- if (cpu == 1)
- __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
+ reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
+ __raw_writel(0, reg_base);
/*
* here's the WFI
@@ -106,7 +107,7 @@ static inline void platform_do_lowpower(unsigned int cpu,
int *spurious)
:
: memory, cc);
- if (pen_release == cpu_logical_map(cpu)) {
+ if (pen_release == phys_cpu) {
/*
* OK, proper wakeup, we're done
*/
diff --git a/arch/arm/mach-exynos/include/mach/regs-pmu.h
b/arch/arm/mach-exynos/include/mach/regs-pmu.h
index 57344b7..cf40b86 100644
--- a/arch/arm/mach-exynos/include/mach/regs-pmu.h
+++ b/arch/arm/mach-exynos/include/mach/regs-pmu.h
@@ -125,10 +125,14 @@
#define S5P_GPS_ALIVE_LOWPWR S5P_PMUREG(0x13A0)
#define S5P_ARM_CORE0_CONFIGURATIONS5P_PMUREG(0x2000)
+#define S5P_ARM_CORE0_STATUS S5P_PMUREG(0x2004)
#define S5P_ARM_CORE0_OPTION S5P_PMUREG(0x2008)
-#define S5P_ARM_CORE1_CONFIGURATIONS5P_PMUREG(0x2080)
-#define S5P_ARM_CORE1_STATUS S5P_PMUREG(0x2084)
-#define S5P_ARM_CORE1_OPTION S5P_PMUREG(0x2088)
+#define S5P_ARM_CORE_CONFIGURATION(_nr)\
+ (S5P_ARM_CORE0_CONFIGURATION + ((_nr) * 0x80))
+#define S5P_ARM_CORE_STATUS(_nr) \
+ (S5P_ARM_CORE0_STATUS + ((_nr) * 0x80))
+#define S5P_ARM_CORE_OPTION(_nr) \
+ (S5P_ARM_CORE0_OPTION + ((_nr) * 0x80))
#define S5P_ARM_COMMON_OPTION S5P_PMUREG(0x2408)
#define S5P_TOP_PWR_OPTION S5P_PMUREG(0x2C48)
diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c
index d9c6d0a..2cbabc8 100644
--- a/arch/arm/mach-exynos/platsmp.c
+++ b/arch/arm/mach-exynos/platsmp.c
@@ -109,14 +109,15 @@ static int __cpuinit exynos_boot_secondary(unsigned int
cpu, struct task_struct
*/
write_pen_release(phys_cpu);
- if (!(__raw_readl(S5P_ARM_CORE1_STATUS) S5P_CORE_LOCAL_PWR_EN)) {
+ if (!(__raw_readl(S5P_ARM_CORE_STATUS(phys_cpu))
+S5P_CORE_LOCAL_PWR_EN)) {
__raw_writel(S5P_CORE_LOCAL_PWR_EN,
-S5P_ARM_CORE1_CONFIGURATION);
+S5P_ARM_CORE_CONFIGURATION(phys_cpu));
timeout = 10;
/* wait max 10 ms until cpu1 is on */
- while ((__raw_readl(S5P_ARM_CORE1_STATUS)
+ while ((__raw_readl(S5P_ARM_CORE_STATUS(phys_cpu))
S5P_CORE_LOCAL_PWR_EN) != S5P_CORE_LOCAL_PWR_EN) {
if (timeout-- == 0)
break;
@@ -125,7 +126,7 @@ static int __cpuinit exynos_boot_secondary(unsigned int
cpu, struct task_struct
}
if (timeout == 0) {
- printk(KERN_ERR cpu1 power enable failed);
+ printk(KERN_ERR cpu%u power enable failed, cpu);
spin_unlock(boot_lock);
return -ETIMEDOUT;
}
--
1.8.2.1
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
Ccing Arnd and Olof, because I forgot to add them to git send-email...
Sorry for the noise.
On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
S5P_ARM_CORE1_* registers affect only core 1. To control further cores
properly another registers must be used.
This patch replaces S5P_ARM_CORE1_* register definitions with
S5P_ARM_CORE_*(x) macro which return addresses of registers for
specified core.
This fixes CPU hotplug on quad core Exynos SoCs on which currently
offlining CPUs 2 or 3 caused CPU 1 to be turned off.
Obviously this doesn't happen currently because of the if (cpu == 1), but
if logical cpu1 turned out not to be physical cpu1, then it would crash.
Best regards,
Tomasz
In addition,
bring-up of CPU 2 and 3 is fixed on boards where bootloader powers off
secondary cores by default.
Cc: sta...@vger.kernel.org
Signed-off-by: Tomasz Figa t.f...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
arch/arm/mach-exynos/hotplug.c | 9 +
arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++---
arch/arm/mach-exynos/platsmp.c | 9 +
3 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/arch/arm/mach-exynos/hotplug.c
b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
--- a/arch/arm/mach-exynos/hotplug.c
+++ b/arch/arm/mach-exynos/hotplug.c
@@ -93,10 +93,11 @@ static inline void cpu_leave_lowpower(void)
static inline void platform_do_lowpower(unsigned int cpu, int
*spurious) {
for (;;) {
+ void __iomem *reg_base;
+ unsigned int phys_cpu = cpu_logical_map(cpu);
- /* make cpu1 to be turned off at next WFI command */
- if (cpu == 1)
- __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
+ reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
+ __raw_writel(0, reg_base);
/*
* here's the WFI
@@ -106,7 +107,7 @@ static inline void platform_do_lowpower(unsigned int
cpu, int *spurious)
: memory, cc);
- if (pen_release == cpu_logical_map(cpu)) {
+ if (pen_release == phys_cpu) {
/*
* OK, proper wakeup, we're done
*/
diff --git a/arch/arm/mach-exynos/include/mach/regs-pmu.h
b/arch/arm/mach-exynos/include/mach/regs-pmu.h index 57344b7..cf40b86
100644
--- a/arch/arm/mach-exynos/include/mach/regs-pmu.h
+++ b/arch/arm/mach-exynos/include/mach/regs-pmu.h
@@ -125,10 +125,14 @@
#define S5P_GPS_ALIVE_LOWPWR S5P_PMUREG(0x13A0)
#define S5P_ARM_CORE0_CONFIGURATION S5P_PMUREG(0x2000)
+#define S5P_ARM_CORE0_STATUS S5P_PMUREG(0x2004)
#define S5P_ARM_CORE0_OPTION S5P_PMUREG(0x2008)
-#define S5P_ARM_CORE1_CONFIGURATION S5P_PMUREG(0x2080)
-#define S5P_ARM_CORE1_STATUS S5P_PMUREG(0x2084)
-#define S5P_ARM_CORE1_OPTION S5P_PMUREG(0x2088)
+#define S5P_ARM_CORE_CONFIGURATION(_nr) \
+ (S5P_ARM_CORE0_CONFIGURATION + ((_nr) * 0x80))
+#define S5P_ARM_CORE_STATUS(_nr) \
+ (S5P_ARM_CORE0_STATUS + ((_nr) * 0x80))
+#define S5P_ARM_CORE_OPTION(_nr) \
+ (S5P_ARM_CORE0_OPTION + ((_nr) * 0x80))
#define S5P_ARM_COMMON_OPTIONS5P_PMUREG(0x2408)
#define S5P_TOP_PWR_OPTION S5P_PMUREG(0x2C48)
diff --git a/arch/arm/mach-exynos/platsmp.c
b/arch/arm/mach-exynos/platsmp.c index d9c6d0a..2cbabc8 100644
--- a/arch/arm/mach-exynos/platsmp.c
+++ b/arch/arm/mach-exynos/platsmp.c
@@ -109,14 +109,15 @@ static int __cpuinit
exynos_boot_secondary(unsigned int cpu, struct task_struct */
write_pen_release(phys_cpu);
- if (!(__raw_readl(S5P_ARM_CORE1_STATUS) S5P_CORE_LOCAL_PWR_EN))
{
+ if (!(__raw_readl(S5P_ARM_CORE_STATUS(phys_cpu))
+ S5P_CORE_LOCAL_PWR_EN)) {
__raw_writel(S5P_CORE_LOCAL_PWR_EN,
- S5P_ARM_CORE1_CONFIGURATION);
+ S5P_ARM_CORE_CONFIGURATION(phys_cpu));
timeout = 10;
/* wait max 10 ms until cpu1 is on */
- while ((__raw_readl(S5P_ARM_CORE1_STATUS)
+ while ((__raw_readl(S5P_ARM_CORE_STATUS(phys_cpu))
S5P_CORE_LOCAL_PWR_EN) != S5P_CORE_LOCAL_PWR_EN)
{
if (timeout-- == 0)
break;
@@ -125,7 +126,7 @@ static int __cpuinit exynos_boot_secondary(unsigned
int cpu, struct task_struct }
if (timeout == 0) {
- printk(KERN_ERR cpu1 power enable failed);
+ printk(KERN_ERR cpu%u power enable failed, cpu);
spin_unlock(boot_lock);
return -ETIMEDOUT;
}
--
To unsubscribe from this list: send the line
Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
On 06/19/13 02:45, Tomasz Figa wrote:
Ccing Arnd and Olof, because I forgot to add them to git send-email...
Sorry for the noise.
On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
S5P_ARM_CORE1_* registers affect only core 1. To control further cores
properly another registers must be used.
This patch replaces S5P_ARM_CORE1_* register definitions with
S5P_ARM_CORE_*(x) macro which return addresses of registers for
specified core.
This fixes CPU hotplug on quad core Exynos SoCs on which currently
offlining CPUs 2 or 3 caused CPU 1 to be turned off.
Obviously this doesn't happen currently because of the if (cpu == 1), but
Yes, not happened...and just note exynos5440 doesn't support hotplug :)
so this is available on exynos4412 and added 5420.
if logical cpu1 turned out not to be physical cpu1, then it would crash.
Best regards,
Tomasz
In addition,
bring-up of CPU 2 and 3 is fixed on boards where bootloader powers off
secondary cores by default.
I need to test on board about above...
Cc: sta...@vger.kernel.org
Signed-off-by: Tomasz Figat.f...@samsung.com
Signed-off-by: Kyungmin Parkkyungmin.p...@samsung.com
---
arch/arm/mach-exynos/hotplug.c | 9 +
arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++---
arch/arm/mach-exynos/platsmp.c | 9 +
3 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/arch/arm/mach-exynos/hotplug.c
b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
--- a/arch/arm/mach-exynos/hotplug.c
+++ b/arch/arm/mach-exynos/hotplug.c
@@ -93,10 +93,11 @@ static inline void cpu_leave_lowpower(void)
static inline void platform_do_lowpower(unsigned int cpu, int
*spurious) {
for (;;) {
+ void __iomem *reg_base;
+ unsigned int phys_cpu = cpu_logical_map(cpu);
- /* make cpu1 to be turned off at next WFI command */
- if (cpu == 1)
- __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
+ reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
+ __raw_writel(0, reg_base);
/*
* here's the WFI
@@ -106,7 +107,7 @@ static inline void platform_do_lowpower(unsigned int
cpu, int *spurious)
: memory, cc);
- if (pen_release == cpu_logical_map(cpu)) {
+ if (pen_release == phys_cpu) {
/*
* OK, proper wakeup, we're done
*/
diff --git a/arch/arm/mach-exynos/include/mach/regs-pmu.h
b/arch/arm/mach-exynos/include/mach/regs-pmu.h index 57344b7..cf40b86
100644
--- a/arch/arm/mach-exynos/include/mach/regs-pmu.h
+++ b/arch/arm/mach-exynos/include/mach/regs-pmu.h
@@ -125,10 +125,14 @@
#define S5P_GPS_ALIVE_LOWPWR S5P_PMUREG(0x13A0)
#define S5P_ARM_CORE0_CONFIGURATION S5P_PMUREG(0x2000)
+#define S5P_ARM_CORE0_STATUS S5P_PMUREG(0x2004)
#define S5P_ARM_CORE0_OPTION S5P_PMUREG(0x2008)
-#define S5P_ARM_CORE1_CONFIGURATIONS5P_PMUREG(0x2080)
-#define S5P_ARM_CORE1_STATUS S5P_PMUREG(0x2084)
-#define S5P_ARM_CORE1_OPTION S5P_PMUREG(0x2088)
+#define S5P_ARM_CORE_CONFIGURATION(_nr)\
+ (S5P_ARM_CORE0_CONFIGURATION + ((_nr) * 0x80))
+#define S5P_ARM_CORE_STATUS(_nr) \
+ (S5P_ARM_CORE0_STATUS + ((_nr) * 0x80))
+#define S5P_ARM_CORE_OPTION(_nr) \
+ (S5P_ARM_CORE0_OPTION + ((_nr) * 0x80))
#define S5P_ARM_COMMON_OPTION S5P_PMUREG(0x2408)
#define S5P_TOP_PWR_OPTIONS5P_PMUREG(0x2C48)
diff --git a/arch/arm/mach-exynos/platsmp.c
b/arch/arm/mach-exynos/platsmp.c index d9c6d0a..2cbabc8 100644
--- a/arch/arm/mach-exynos/platsmp.c
+++ b/arch/arm/mach-exynos/platsmp.c
@@ -109,14 +109,15 @@ static int __cpuinit
exynos_boot_secondary(unsigned int cpu, struct task_struct */
write_pen_release(phys_cpu);
- if (!(__raw_readl(S5P_ARM_CORE1_STATUS) S5P_CORE_LOCAL_PWR_EN))
{
+ if (!(__raw_readl(S5P_ARM_CORE_STATUS(phys_cpu))
+ S5P_CORE_LOCAL_PWR_EN)) {
__raw_writel(S5P_CORE_LOCAL_PWR_EN,
-S5P_ARM_CORE1_CONFIGURATION);
+S5P_ARM_CORE_CONFIGURATION(phys_cpu));
timeout = 10;
/* wait max 10 ms until cpu1 is on */
- while ((__raw_readl(S5P_ARM_CORE1_STATUS)
+ while ((__raw_readl(S5P_ARM_CORE_STATUS(phys_cpu))
S5P_CORE_LOCAL_PWR_EN) != S5P_CORE_LOCAL_PWR_EN)
{
if (timeout-- == 0)
break;
@@ -125,7 +126,7 @@ static int __cpuinit exynos_boot_secondary(unsigned
int cpu, struct task_struct }
if (timeout == 0) {
- printk(KERN_ERR cpu1 power enable failed);
+
