ARM: shmobile: CPU Hotplug and SMP CA9 SCU rework part 2
[PATCH 01/08] ARM: shmobile: Fix base address readout in headsmp-scu.S
[PATCH 02/08] ARM: shmobile: Rework SH73A0_SCU_BASE IOMEM() usage
[PATCH 03/08] ARM: shmobile: Use R8A7779_SCU_BASE with TWD
[PATCH 04/08] ARM: shmobile: Update r8a7779 to check SCU for hotplug
[PATCH 05/08] ARM: shmobile: Update r8a7779 to use scu_power_mode()
[PATCH 06/08] ARM: shmobile: Use sh73a0-specific cpu disable code
[PATCH 07/08] ARM: shmobile: Rearrange r8a7779 cpu hotplug code
[PATCH 08/08] ARM: shmobile: Remove unused hotplug.c
Finalize the SCU rework by performing minor fixes and converting
the r8a7779 SMP code to make use of common scu_power_mode() code
and the early SCU setup code in headsmp-scu.S.
Also, to save a few line remove hotplug.c while at it.
The only remaining itch to scratch here is to fix up CPU Hotplug
of CPU0 on r8a7779, but for that to happen we first need to start
accounting for spurious interrupts during shut down.
Signed-off-by: Magnus Damm <redacted>
---
No known dependencies apart from previous SMP cleanup series
"[PATCH 00/06] ARM: shmobile: SMP Cortex-A9 SCU rework"
Written against -next in renesas.git.
arch/arm/mach-shmobile/Makefile | 1
arch/arm/mach-shmobile/headsmp-scu.S | 3
arch/arm/mach-shmobile/hotplug.c | 64 ---------
arch/arm/mach-shmobile/include/mach/common.h | 10 -
arch/arm/mach-shmobile/smp-r8a7779.c | 173 +++++++++++++-------------
arch/arm/mach-shmobile/smp-sh73a0.c | 11 +
6 files changed, 99 insertions(+), 163 deletions(-)
From: Magnus Damm <redacted>
Rework the early SCU setup code in headsmp-scu.S to read
the base address in the same way as we use to fetch the
address of the invalidation function.
Reported-by: Bastian Hecht <redacted>
Signed-off-by: Magnus Damm <redacted>
---
Tested on r8a7779. Feel free to add incrementally or fold into:
[PATCH 05/06] ARM: shmobile: Common shmobile_scu_base in headsmp-scu.S
Bastian, does this solve your problem?
arch/arm/mach-shmobile/headsmp-scu.S | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
@@ -39,7 +39,7 @@ ENTRY(shmobile_secondary_vector_scu)mrcp15,0,r0,c0,c0,5@readMIPDRandr0,r0,#3 @ mask out cpu IDlslr0,r0,#3 @ we will shift by cpu_id * 8 bits-ldrr1,=shmobile_scu_base+ldrr1,2fldrr1,[r1]@SCUbaseaddressldrr2,[r1,#8] @ SCU Power Status Registermovr3,#3
From: Magnus Damm <redacted>
Rework the IOMEM() usage for the SCU base address in the
case of sh73a0. Removes recently introduced build warnings:
arch/arm/mach-shmobile/smp-sh73a0.c:45:15: warning: initialization makes integer from pointer without a cast [enabled by default]
arch/arm/mach-shmobile/smp-sh73a0.c:45:15: warning: (near initialization for 'twd_local_timer.res[0].start') [enabled by default]
arch/arm/mach-shmobile/smp-sh73a0.c:45:15: warning: initialization makes integer from pointer without a cast [enabled by default]
/arch/arm/mach-shmobile/smp-sh73a0.c:45:15: warning: (near initialization for 'twd_local_timer.res[0].end') [enabled by default]
Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Magnus Damm <redacted>
---
arch/arm/mach-shmobile/smp-sh73a0.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Magnus Damm <redacted>
Rework the IOMEM() usage for the SCU base address in the
case of r8a7779. Adjusts the TWD to use R8A7779_SCU_BASE.
Signed-off-by: Magnus Damm <redacted>
---
arch/arm/mach-shmobile/smp-r8a7779.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
From: Magnus Damm <redacted>
Update the r8a7779 CPU Hotplug code to use SCU PSR
to wait for the target CPU core. Previously the
shared code in hotplug.c was used to let cpu_kill()
wait for cpu_die(). With this change in place the
r8a7779 SMP code does not depend on hotplug.c anymore.
Signed-off-by: Magnus Damm <redacted>
---
arch/arm/mach-shmobile/smp-r8a7779.c | 38 ++++++++++++++++++++++++++++------
1 file changed, 32 insertions(+), 6 deletions(-)
@@ -110,7 +118,7 @@ static int __maybe_unused r8a7779_cpu_ki*finishbeforeaskingSoC-specificcodetopowerofftheCPUcore.*/for(k=0;k<1000;k++){-if(shmobile_cpu_is_dead(cpu))+if(r8a7779_scu_psr_core_disabled(cpu))returnr8a7779_platform_cpu_kill(cpu);mdelay(1);
@@ -119,6 +127,24 @@ static int __maybe_unused r8a7779_cpu_kireturn0;}+staticvoid__maybe_unusedr8a7779_cpu_die(unsignedintcpu)+{+dsb();+flush_cache_all();++/* disable cache coherency */+modify_scu_cpu_psr(3<<(cpu*8),0);++/* Endless loop until power off from r8a7779_cpu_kill() */+while(1)+cpu_do_idle();+}++staticint__maybe_unusedr8a7779_cpu_disable(unsignedintcpu)+{+/* only CPU1->3 have power domains, do not allow hotplug of CPU0 */+returncpu==0?-EPERM:0;+}staticvoid__cpuinitr8a7779_secondary_init(unsignedintcpu){
From: Magnus Damm <redacted>
Update the SMP code for R8A7779 to make use of the
shared SCU function scu_power_mode() together with
the early setup code in shmobile_secondary_vector_scu.
With this patch in place the secondary CPUs modify the
SCU setting during early boot instead of letting other
CPUs deal with the coherency setting before boot. In
other words, we used to setup coherency before boot
in r8a7779_boot_secondary() but that bit is now instead
handled by the code in shmobile_secondary_vector_scu.
Signed-off-by: Magnus Damm <redacted>
---
arch/arm/mach-shmobile/smp-r8a7779.c | 32 +++++---------------------------
1 file changed, 5 insertions(+), 27 deletions(-)
@@ -79,20 +76,6 @@ static int r8a7779_scu_psr_core_disabledreturn0;}-staticvoidmodify_scu_cpu_psr(unsignedlongset,unsignedlongclr)-{-void__iomem*scu_base=shmobile_scu_base;--spin_lock(&scu_lock);-tmp=__raw_readl(scu_base+8);-tmp&=~clr;-tmp|=set;-spin_unlock(&scu_lock);--/* disable cache coherency after releasing the lock */-__raw_writel(tmp,scu_base+8);-}-staticintr8a7779_platform_cpu_kill(unsignedintcpu){structr8a7779_pm_ch*ch=NULL;
@@ -133,7 +116,7 @@ static void __maybe_unused r8a7779_cpu_dflush_cache_all();/* disable cache coherency */-modify_scu_cpu_psr(3<<(cpu*8),0);+scu_power_mode(shmobile_scu_base,SCU_PM_POWEROFF);/* Endless loop until power off from r8a7779_cpu_kill() */while(1)
@@ -172,15 +152,13 @@ static int __cpuinit r8a7779_boot_secondstaticvoid__initr8a7779_smp_prepare_cpus(unsignedintmax_cpus){-intcpu=cpu_logical_map(0);-scu_enable(shmobile_scu_base);-/* Map the reset vector (in headsmp.S) */-__raw_writel(__pa(shmobile_secondary_vector),AVECR);+/* Map the reset vector (in headsmp-scu.S) */+__raw_writel(__pa(shmobile_secondary_vector_scu),AVECR);-/* enable cache coherency on CPU0 */-modify_scu_cpu_psr(0,3<<(cpu*8));+/* enable cache coherency on booting CPU */+scu_power_mode(shmobile_scu_base,SCU_PM_NORMAL);r8a7779_pm_init();
From: Magnus Damm <redacted>
Convert the sh73a0 CPU Hotplug code to use a local
implementation of ->cpu_disable(). With this change
in place the sh73a0 SMP code does no longer depend
on hotplug.c.
Signed-off-by: Magnus Damm <redacted>
---
arch/arm/mach-shmobile/smp-sh73a0.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
From: Magnus Damm <redacted>
Update the r8a7779 SMP code and CPU Hotplug in particular
to follow the same style as sh73a0. This means dropping
__maybe_unused for #ifdef CONFIG_HOTPLUG_CPU.
Signed-off-by: Magnus Damm <redacted>
---
arch/arm/mach-shmobile/smp-r8a7779.c | 96 +++++++++++++++++-----------------
1 file changed, 49 insertions(+), 47 deletions(-)
@@ -92,43 +82,6 @@ static int r8a7779_platform_cpu_kill(unsreturnret?ret:1;}-staticint__maybe_unusedr8a7779_cpu_kill(unsignedintcpu)-{-intk;--/* this function is running on another CPU than the offline target,-*hereweneedwaitforshutdowncodeinplatform_cpu_die()to-*finishbeforeaskingSoC-specificcodetopowerofftheCPUcore.-*/-for(k=0;k<1000;k++){-if(r8a7779_scu_psr_core_disabled(cpu))-returnr8a7779_platform_cpu_kill(cpu);--mdelay(1);-}--return0;-}--staticvoid__maybe_unusedr8a7779_cpu_die(unsignedintcpu)-{-dsb();-flush_cache_all();--/* disable cache coherency */-scu_power_mode(shmobile_scu_base,SCU_PM_POWEROFF);--/* Endless loop until power off from r8a7779_cpu_kill() */-while(1)-cpu_do_idle();-}--staticint__maybe_unusedr8a7779_cpu_disable(unsignedintcpu)-{-/* only CPU1->3 have power domains, do not allow hotplug of CPU0 */-returncpu==0?-EPERM:0;-}-staticvoid__cpuinitr8a7779_secondary_init(unsignedintcpu){gic_secondary_init(0);
@@ -176,6 +129,55 @@ static void __init r8a7779_smp_init_cpusshmobile_smp_init_cpus(scu_get_core_count(shmobile_scu_base));}+#ifdef CONFIG_HOTPLUG_CPU+staticintr8a7779_scu_psr_core_disabled(intcpu)+{+unsignedlongmask=3<<(cpu*8);++if((__raw_readl(shmobile_scu_base+8)&mask)==mask)+return1;++return0;+}++staticintr8a7779_cpu_kill(unsignedintcpu)+{+intk;++/* this function is running on another CPU than the offline target,+*hereweneedwaitforshutdowncodeinplatform_cpu_die()to+*finishbeforeaskingSoC-specificcodetopowerofftheCPUcore.+*/+for(k=0;k<1000;k++){+if(r8a7779_scu_psr_core_disabled(cpu))+returnr8a7779_platform_cpu_kill(cpu);++mdelay(1);+}++return0;+}++staticvoidr8a7779_cpu_die(unsignedintcpu)+{+dsb();+flush_cache_all();++/* disable cache coherency */+scu_power_mode(shmobile_scu_base,SCU_PM_POWEROFF);++/* Endless loop until power off from r8a7779_cpu_kill() */+while(1)+cpu_do_idle();+}++staticintr8a7779_cpu_disable(unsignedintcpu)+{+/* only CPU1->3 have power domains, do not allow hotplug of CPU0 */+returncpu==0?-EPERM:0;+}+#endif /* CONFIG_HOTPLUG_CPU */+structsmp_operationsr8a7779_smp_ops__initdata={.smp_init_cpus=r8a7779_smp_init_cpus,.smp_prepare_cpus=r8a7779_smp_prepare_cpus,
From: Magnus Damm <redacted>
Each CPU Hotplug implementation for mach-shmobile
is now self-contained, so this change removes unused
helper code in hotplug.c. The two CPU Hotplug capable
SoCs sh73a0 and r8a7779 remain unchanged.
Signed-off-by: Magnus Damm <redacted>
---
arch/arm/mach-shmobile/Makefile | 1
arch/arm/mach-shmobile/hotplug.c | 64 --------------------------
arch/arm/mach-shmobile/include/mach/common.h | 10 ----
3 files changed, 75 deletions(-)
Ok, this gets rid of the warning, but I'm a bit worried about
how it is hardwiring the fact that the SCU physical address has
the same bit pattern as the __iomem token.
While I realize that you already rely on this in a lot of places
in the shmobile code, I see a red light going off every time I read
code like this, and it is not any more logical than the previous
version.
It would be nice to keep these address spaces separate at least
in new code, mostly in order to not confuse reviewers with code
that is based on assumptions which are not generally true, but also
to be more flexible with the virtual memory layout. On a related
topic, you are using an entire 256 MB section of your virtual
address space for sh73a0 and sh7372 and 160 MB for r8a7740. Putting
less of that into the identity mapped area would free up space for
vmalloc, but it's hard to prove that doing this is correct when
you have all sorts of code using a hardcoded virtual MMIO address
token.
Arnd
Ok, this gets rid of the warning, but I'm a bit worried about
how it is hardwiring the fact that the SCU physical address has
the same bit pattern as the __iomem token.
While I realize that you already rely on this in a lot of places
in the shmobile code, I see a red light going off every time I read
code like this, and it is not any more logical than the previous
version.
It would be nice to keep these address spaces separate at least
in new code, mostly in order to not confuse reviewers with code
that is based on assumptions which are not generally true, but also
to be more flexible with the virtual memory layout. On a related
topic, you are using an entire 256 MB section of your virtual
address space for sh73a0 and sh7372 and 160 MB for r8a7740. Putting
less of that into the identity mapped area would free up space for
vmalloc, but it's hard to prove that doing this is correct when
you have all sorts of code using a hardcoded virtual MMIO address
token.
To clarify my rant: I'm absolutely fine with this code going in
for now, but I'd like to see a long-term plan about what to do
with the hardcoded virtual address hacks.
Arnd
Hi Magnus,
yes using the physical address works.
Thanks,
Bastian
2013/2/18 Magnus Damm [off-list ref]:
quoted hunk
From: Magnus Damm <redacted>
Rework the early SCU setup code in headsmp-scu.S to read
the base address in the same way as we use to fetch the
address of the invalidation function.
Reported-by: Bastian Hecht <redacted>
Signed-off-by: Magnus Damm <redacted>
---
Tested on r8a7779. Feel free to add incrementally or fold into:
[PATCH 05/06] ARM: shmobile: Common shmobile_scu_base in headsmp-scu.S
Bastian, does this solve your problem?
arch/arm/mach-shmobile/headsmp-scu.S | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
@@ -39,7 +39,7 @@ ENTRY(shmobile_secondary_vector_scu)mrcp15,0,r0,c0,c0,5@readMIPDRandr0,r0,#3 @ mask out cpu IDlslr0,r0,#3 @ we will shift by cpu_id * 8 bits-ldrr1,=shmobile_scu_base+ldrr1,2fldrr1,[r1]@SCUbaseaddressldrr2,[r1,#8] @ SCU Power Status Registermovr3,#3
On Mon, Feb 18, 2013 at 10:46:48PM +0900, Magnus Damm wrote:
ARM: shmobile: CPU Hotplug and SMP CA9 SCU rework part 2
[PATCH 01/08] ARM: shmobile: Fix base address readout in headsmp-scu.S
[PATCH 02/08] ARM: shmobile: Rework SH73A0_SCU_BASE IOMEM() usage
[PATCH 03/08] ARM: shmobile: Use R8A7779_SCU_BASE with TWD
[PATCH 04/08] ARM: shmobile: Update r8a7779 to check SCU for hotplug
[PATCH 05/08] ARM: shmobile: Update r8a7779 to use scu_power_mode()
[PATCH 06/08] ARM: shmobile: Use sh73a0-specific cpu disable code
[PATCH 07/08] ARM: shmobile: Rearrange r8a7779 cpu hotplug code
[PATCH 08/08] ARM: shmobile: Remove unused hotplug.c
Finalize the SCU rework by performing minor fixes and converting
the r8a7779 SMP code to make use of common scu_power_mode() code
and the early SCU setup code in headsmp-scu.S.
Also, to save a few line remove hotplug.c while at it.
The only remaining itch to scratch here is to fix up CPU Hotplug
of CPU0 on r8a7779, but for that to happen we first need to start
accounting for spurious interrupts during shut down.
Signed-off-by: Magnus Damm <redacted>
---
No known dependencies apart from previous SMP cleanup series
"[PATCH 00/06] ARM: shmobile: SMP Cortex-A9 SCU rework"
Written against -next in renesas.git.
I believe those are queued up for upstream in the soc5 branch.
Please let me know when you are comfortable with me applying these patches
to that branch. I'm fine with that answer being "now".
On Tue, Feb 19, 2013 at 9:49 AM, Simon Horman [off-list ref] wrote:
On Mon, Feb 18, 2013 at 10:46:48PM +0900, Magnus Damm wrote:
quoted
ARM: shmobile: CPU Hotplug and SMP CA9 SCU rework part 2
[PATCH 01/08] ARM: shmobile: Fix base address readout in headsmp-scu.S
[PATCH 02/08] ARM: shmobile: Rework SH73A0_SCU_BASE IOMEM() usage
[PATCH 03/08] ARM: shmobile: Use R8A7779_SCU_BASE with TWD
[PATCH 04/08] ARM: shmobile: Update r8a7779 to check SCU for hotplug
[PATCH 05/08] ARM: shmobile: Update r8a7779 to use scu_power_mode()
[PATCH 06/08] ARM: shmobile: Use sh73a0-specific cpu disable code
[PATCH 07/08] ARM: shmobile: Rearrange r8a7779 cpu hotplug code
[PATCH 08/08] ARM: shmobile: Remove unused hotplug.c
Finalize the SCU rework by performing minor fixes and converting
the r8a7779 SMP code to make use of common scu_power_mode() code
and the early SCU setup code in headsmp-scu.S.
Also, to save a few line remove hotplug.c while at it.
The only remaining itch to scratch here is to fix up CPU Hotplug
of CPU0 on r8a7779, but for that to happen we first need to start
accounting for spurious interrupts during shut down.
Signed-off-by: Magnus Damm <redacted>
---
No known dependencies apart from previous SMP cleanup series
"[PATCH 00/06] ARM: shmobile: SMP Cortex-A9 SCU rework"
Written against -next in renesas.git.
I believe those are queued up for upstream in the soc5 branch.
Please let me know when you are comfortable with me applying these patches
to that branch. I'm fine with that answer being "now".
On Mon, Feb 18, 2013 at 11:44 PM, Arnd Bergmann [off-list ref] wrote:
On Monday 18 February 2013, Arnd Bergmann wrote:
quoted
Ok, this gets rid of the warning, but I'm a bit worried about
how it is hardwiring the fact that the SCU physical address has
the same bit pattern as the __iomem token.
While I realize that you already rely on this in a lot of places
in the shmobile code, I see a red light going off every time I read
code like this, and it is not any more logical than the previous
version.
It would be nice to keep these address spaces separate at least
in new code, mostly in order to not confuse reviewers with code
that is based on assumptions which are not generally true, but also
to be more flexible with the virtual memory layout. On a related
topic, you are using an entire 256 MB section of your virtual
address space for sh73a0 and sh7372 and 160 MB for r8a7740. Putting
less of that into the identity mapped area would free up space for
vmalloc, but it's hard to prove that doing this is correct when
you have all sorts of code using a hardcoded virtual MMIO address
token.
To clarify my rant: I'm absolutely fine with this code going in
for now, but I'd like to see a long-term plan about what to do
with the hardcoded virtual address hacks.
Thanks for the clarification.
For mach-shmobile the three major components that rely on entity
mapped memory maps are SMP, clocks and power domains. The clocks
should really be moved in the common direction and I intend to get
people to focus on that in the not too distant future (next 6 months).
Power domains should be rather easy to convert. SMP tends to be a bit
of a headache because last time I checked I couldn't use ioremap() at
->smp_init_cpus() time. What I recall is that ioremap() hanged instead
of returning something.
Anyway, if I track down the ioremap() issue, would it be possible for
you to check if it can be reproduced on some other sub-architecture?
Thanks,
/ magnus
On Tue, Feb 19, 2013 at 07:09:14PM +0900, Magnus Damm wrote:
On Tue, Feb 19, 2013 at 9:49 AM, Simon Horman [off-list ref] wrote:
quoted
On Mon, Feb 18, 2013 at 10:46:48PM +0900, Magnus Damm wrote:
quoted
ARM: shmobile: CPU Hotplug and SMP CA9 SCU rework part 2
[PATCH 01/08] ARM: shmobile: Fix base address readout in headsmp-scu.S
[PATCH 02/08] ARM: shmobile: Rework SH73A0_SCU_BASE IOMEM() usage
[PATCH 03/08] ARM: shmobile: Use R8A7779_SCU_BASE with TWD
[PATCH 04/08] ARM: shmobile: Update r8a7779 to check SCU for hotplug
[PATCH 05/08] ARM: shmobile: Update r8a7779 to use scu_power_mode()
[PATCH 06/08] ARM: shmobile: Use sh73a0-specific cpu disable code
[PATCH 07/08] ARM: shmobile: Rearrange r8a7779 cpu hotplug code
[PATCH 08/08] ARM: shmobile: Remove unused hotplug.c
Finalize the SCU rework by performing minor fixes and converting
the r8a7779 SMP code to make use of common scu_power_mode() code
and the early SCU setup code in headsmp-scu.S.
Also, to save a few line remove hotplug.c while at it.
The only remaining itch to scratch here is to fix up CPU Hotplug
of CPU0 on r8a7779, but for that to happen we first need to start
accounting for spurious interrupts during shut down.
Signed-off-by: Magnus Damm <redacted>
---
No known dependencies apart from previous SMP cleanup series
"[PATCH 00/06] ARM: shmobile: SMP Cortex-A9 SCU rework"
Written against -next in renesas.git.
I believe those are queued up for upstream in the soc5 branch.
Please let me know when you are comfortable with me applying these patches
to that branch. I'm fine with that answer being "now".
Now sounds wonderful. Thanks Simon.
[some time passes]
Thanks, applied to the soc5 branch.
For mach-shmobile the three major components that rely on entity
mapped memory maps are SMP, clocks and power domains. The clocks
should really be moved in the common direction and I intend to get
people to focus on that in the not too distant future (next 6 months).
Power domains should be rather easy to convert. SMP tends to be a bit
of a headache because last time I checked I couldn't use ioremap() at
->smp_init_cpus() time. What I recall is that ioremap() hanged instead
of returning something.
Anyway, if I track down the ioremap() issue, would it be possible for
you to check if it can be reproduced on some other sub-architecture?
You are right that ioremap cannot be used from ->smp_init_cpus() and any
code called from there needs to use a static mapping for accessing
MMIO registers. There is nothing wrong with that. There are in fact
three distinct reasons why people use static MMIO mappings with
iotable_init():
1. For MMIO registers that need to be accessed before ioremap works.
This usually means the SMP startup and the early printk (which I
believe shmobile is not using).
2. For getting hugetlb mappings of MMIO registers into the kernel
address space. If you have a lot of registers in the same area,
using a single TLB to map them is more efficient, even when
accessing the registers through ioremap from a device driver.
3. For hardcoding the virtual address to a location that is passed
to device drivers as compile-time constants.
The first two are absolutely fine, there are no objections to those.
The third one is tradtitionally used on a lot of the older platforms,
but with the multiplatform work, we are moving away from it, towards
passing resources in the platform device (ideally from DT, but that
is an orthogonal question here). AFAICT, shmobile is the only "modern"
platform that still relies on fixed virtual addresses, and it is the
only one I know that uses a mapping where the virtual address equals
the physical address.
Arnd
On Tue, Feb 26, 2013 at 7:18 PM, Arnd Bergmann [off-list ref] wrote:
On Monday 25 February 2013, Magnus Damm wrote:
quoted
For mach-shmobile the three major components that rely on entity
mapped memory maps are SMP, clocks and power domains. The clocks
should really be moved in the common direction and I intend to get
people to focus on that in the not too distant future (next 6 months).
Power domains should be rather easy to convert. SMP tends to be a bit
of a headache because last time I checked I couldn't use ioremap() at
->smp_init_cpus() time. What I recall is that ioremap() hanged instead
of returning something.
Anyway, if I track down the ioremap() issue, would it be possible for
you to check if it can be reproduced on some other sub-architecture?
You are right that ioremap cannot be used from ->smp_init_cpus() and any
code called from there needs to use a static mapping for accessing
MMIO registers. There is nothing wrong with that. There are in fact
three distinct reasons why people use static MMIO mappings with
iotable_init():
1. For MMIO registers that need to be accessed before ioremap works.
This usually means the SMP startup and the early printk (which I
believe shmobile is not using).
Thanks for describing these.
Is there any particular reason why SMP startup needs to happen earlier
than ioremap() is available?
From a hardware point of view on Cortex-A9 the SCU needs to be enabled
and the number of available cores need to be determined. The SCU
enabling can probably happen later and the number of cores are already
limited to the kernel configuration maximum number of cores setting,
so it should be possible to use that to size any early per-cpu
variables if needed. So I wonder why we're not enabling SMP later than
we actually do? Using maxcpus=1 and late CPU hotplug from user space
is certainly working fine.
Regarding early printk, you are correct that we're not using that ARM
specific debug output. Instead we are relying on earlyprintk via early
platform devices. This way we are not only multi-soc and multi-subarch
already, we are also multi-arch. For really early console output we
rely on the clocks and pin function being initialized by the boot
loader and we also require 1:1 entity mappings so we can use printouts
before ioremap() is functional. So yes, we like using 1:1 virt-phys
memory maps for early printouts.
We do not use early printk with DT at this point. If we would be able
to move the SMP init later then perhaps we could debug SMP issues with
serial ports described by DT in the future?
2. For getting hugetlb mappings of MMIO registers into the kernel
address space. If you have a lot of registers in the same area,
using a single TLB to map them is more efficient, even when
accessing the registers through ioremap from a device driver.
Sure.
3. For hardcoding the virtual address to a location that is passed
to device drivers as compile-time constants.
The first two are absolutely fine, there are no objections to those.
Ok. As you probably can tell by now - I would like to get rid of the
SMP case if possible.
The third one is tradtitionally used on a lot of the older platforms,
but with the multiplatform work, we are moving away from it, towards
passing resources in the platform device (ideally from DT, but that
is an orthogonal question here). AFAICT, shmobile is the only "modern"
platform that still relies on fixed virtual addresses, and it is the
only one I know that uses a mapping where the virtual address equals
the physical address.
The 1:1 mapping is deliberately chosen to be simple. So in the case
when people do register I/O without ioremap() then at least we can
look up the address in the data sheet. I've seen too many examples of
people not using ioremap and instead inventing their own magic mapping
table with undocumented hard coded address that map to something even
more unknown. Of course we should be aiming at using ioremap(). If we
for some reason can't then we should use 1:1 mappings.
While I agree to move more towards using ioremap(), I can't really see
how this affects our multiplatform situation. Our device drivers have
always been using the driver model and we do never export any virtual
addresses in any header files. If you have any particular area that
you think needs work related to ioremap() then perhaps we can get
together on next conference and talk it through?
As I mentioned before, from my point of view the main limiting factor
for mach-shmobile multiplatform at this point is the clock framework.
The SH clock framework does already support ioremap() though, so it is
just a matter of making the clock code actually use it. And while
we're doing that we may as well solve the multiplatform issue to and
move towards common clocks.
Thanks,
/ magnus
On Tue, Feb 26, 2013 at 7:18 PM, Arnd Bergmann [off-list ref] wrote:
quoted
On Monday 25 February 2013, Magnus Damm wrote:
You are right that ioremap cannot be used from ->smp_init_cpus() and any
code called from there needs to use a static mapping for accessing
MMIO registers. There is nothing wrong with that. There are in fact
three distinct reasons why people use static MMIO mappings with
iotable_init():
1. For MMIO registers that need to be accessed before ioremap works.
This usually means the SMP startup and the early printk (which I
believe shmobile is not using).
Thanks for describing these.
Is there any particular reason why SMP startup needs to happen earlier
than ioremap() is available?
I think it's mostly traditional reason I think.
From a hardware point of view on Cortex-A9 the SCU needs to be enabled
and the number of available cores need to be determined. The SCU
enabling can probably happen later and the number of cores are already
limited to the kernel configuration maximum number of cores setting,
so it should be possible to use that to size any early per-cpu
variables if needed. So I wonder why we're not enabling SMP later than
we actually do? Using maxcpus=1 and late CPU hotplug from user space
is certainly working fine.
AFAIK, on Cortex-A15 we already rely on getting the number of cores from
the device tree, which is also available at the right time, without
the need for an early mapping. It would not be hard to do the same
on Cortex-A9. Then again, the static mapping there does not do harm
as I said.
Regarding early printk, you are correct that we're not using that ARM
specific debug output. Instead we are relying on earlyprintk via early
platform devices. This way we are not only multi-soc and multi-subarch
already, we are also multi-arch. For really early console output we
rely on the clocks and pin function being initialized by the boot
loader and we also require 1:1 entity mappings so we can use printouts
before ioremap() is functional. So yes, we like using 1:1 virt-phys
memory maps for early printouts.
Ok.
We do not use early printk with DT at this point. If we would be able
to move the SMP init later then perhaps we could debug SMP issues with
serial ports described by DT in the future?
quoted
3. For hardcoding the virtual address to a location that is passed
to device drivers as compile-time constants.
The first two are absolutely fine, there are no objections to those.
Ok. As you probably can tell by now - I would like to get rid of the
SMP case if possible.
I would certainly welcome a patch that moves the SMP initialization
to a later point. I'm not sure if it requires changes to architecture
independent code, but it does sound like a good idea.
quoted
The third one is tradtitionally used on a lot of the older platforms,
but with the multiplatform work, we are moving away from it, towards
passing resources in the platform device (ideally from DT, but that
is an orthogonal question here). AFAICT, shmobile is the only "modern"
platform that still relies on fixed virtual addresses, and it is the
only one I know that uses a mapping where the virtual address equals
the physical address.
The 1:1 mapping is deliberately chosen to be simple. So in the case
when people do register I/O without ioremap() then at least we can
look up the address in the data sheet. I've seen too many examples of
people not using ioremap and instead inventing their own magic mapping
table with undocumented hard coded address that map to something even
more unknown. Of course we should be aiming at using ioremap(). If we
for some reason can't then we should use 1:1 mappings.
Well, I would argue that when someone doesn't understand the basic
interfaces we expose to device drivers, they probably shouldn't
be writing kernel code. ;)
While I agree to move more towards using ioremap(), I can't really see
how this affects our multiplatform situation. Our device drivers have
always been using the driver model and we do never export any virtual
addresses in any header files. If you have any particular area that
you think needs work related to ioremap() then perhaps we can get
together on next conference and talk it through?
It may not be as bad as I thought. I know that at least the intc
controller is fundamentally built around this assumption (I tried changing
it, and that didn't end well), but that may be the only one, following
the recent cleanup of the pfc driver.
The main worry is probably that people will take the platform code
as example when writing device drivers, and that uses hardcoded
IOMEM() macros. There are probably a couple of instances where that
is the best solution, but for those, I would suggest using offsets
from a base register that gets passed into iotable_init() rather
than literal numbers.
As I mentioned before, from my point of view the main limiting factor
for mach-shmobile multiplatform at this point is the clock framework.
The SH clock framework does already support ioremap() though, so it is
just a matter of making the clock code actually use it. And while
we're doing that we may as well solve the multiplatform issue to and
move towards common clocks.
Hi Arnd,
On Wed, Feb 27, 2013 at 1:12 AM, Arnd Bergmann [off-list ref] wrote:
On Tuesday 26 February 2013, Magnus Damm wrote:
quoted
On Tue, Feb 26, 2013 at 7:18 PM, Arnd Bergmann [off-list ref] wrote:
quoted
On Monday 25 February 2013, Magnus Damm wrote:
You are right that ioremap cannot be used from ->smp_init_cpus() and any
code called from there needs to use a static mapping for accessing
MMIO registers. There is nothing wrong with that. There are in fact
three distinct reasons why people use static MMIO mappings with
iotable_init():
1. For MMIO registers that need to be accessed before ioremap works.
This usually means the SMP startup and the early printk (which I
believe shmobile is not using).
Thanks for describing these.
Is there any particular reason why SMP startup needs to happen earlier
than ioremap() is available?
I think it's mostly traditional reason I think.
I think so too.
quoted
From a hardware point of view on Cortex-A9 the SCU needs to be enabled
and the number of available cores need to be determined. The SCU
enabling can probably happen later and the number of cores are already
limited to the kernel configuration maximum number of cores setting,
so it should be possible to use that to size any early per-cpu
variables if needed. So I wonder why we're not enabling SMP later than
we actually do? Using maxcpus=1 and late CPU hotplug from user space
is certainly working fine.
AFAIK, on Cortex-A15 we already rely on getting the number of cores from
the device tree, which is also available at the right time, without
the need for an early mapping. It would not be hard to do the same
on Cortex-A9. Then again, the static mapping there does not do harm
as I said.
I understand that you feel that static mapping in the case of SMP is acceptable.
quoted
Regarding early printk, you are correct that we're not using that ARM
specific debug output. Instead we are relying on earlyprintk via early
platform devices. This way we are not only multi-soc and multi-subarch
already, we are also multi-arch. For really early console output we
rely on the clocks and pin function being initialized by the boot
loader and we also require 1:1 entity mappings so we can use printouts
before ioremap() is functional. So yes, we like using 1:1 virt-phys
memory maps for early printouts.
Ok.
quoted
We do not use early printk with DT at this point. If we would be able
to move the SMP init later then perhaps we could debug SMP issues with
serial ports described by DT in the future?
quoted
3. For hardcoding the virtual address to a location that is passed
to device drivers as compile-time constants.
The first two are absolutely fine, there are no objections to those.
Ok. As you probably can tell by now - I would like to get rid of the
SMP case if possible.
I would certainly welcome a patch that moves the SMP initialization
to a later point. I'm not sure if it requires changes to architecture
independent code, but it does sound like a good idea.
Good to hear that that this may be a move in the right direction!
quoted
quoted
The third one is tradtitionally used on a lot of the older platforms,
but with the multiplatform work, we are moving away from it, towards
passing resources in the platform device (ideally from DT, but that
is an orthogonal question here). AFAICT, shmobile is the only "modern"
platform that still relies on fixed virtual addresses, and it is the
only one I know that uses a mapping where the virtual address equals
the physical address.
The 1:1 mapping is deliberately chosen to be simple. So in the case
when people do register I/O without ioremap() then at least we can
look up the address in the data sheet. I've seen too many examples of
people not using ioremap and instead inventing their own magic mapping
table with undocumented hard coded address that map to something even
more unknown. Of course we should be aiming at using ioremap(). If we
for some reason can't then we should use 1:1 mappings.
Well, I would argue that when someone doesn't understand the basic
interfaces we expose to device drivers, they probably shouldn't
be writing kernel code. ;)
I am not sure how this is related to device drivers actually. The
example I was thinking about was snapshot-style development on some
ancient kernel version. In that case the developers simply seemed to
follow the at-that-point common coding style in the ARM architecture.
I am happy to see that the ARM architecture code is getting cleaner
bit by bit.
quoted
While I agree to move more towards using ioremap(), I can't really see
how this affects our multiplatform situation. Our device drivers have
always been using the driver model and we do never export any virtual
addresses in any header files. If you have any particular area that
you think needs work related to ioremap() then perhaps we can get
together on next conference and talk it through?
It may not be as bad as I thought. I know that at least the intc
controller is fundamentally built around this assumption (I tried changing
it, and that didn't end well), but that may be the only one, following
the recent cleanup of the pfc driver.
Uhm, I am not sure where you got that idea about the INTC driver.
Allow me to clarify.
Regarding the shared INTC code base I recall implementing ioremap()
support there 2010, feel free to search the archives for "[PATCH] sh:
INTC ioremap support V2".
As for actual SoC support, this varies with interrupt controller and
SoC. It is basically a matter of if I/O memory windows are passed to
the INTC driver or not.
A typical example would be sh7372 that has two interrupt controllers:
INTCA and INTCS. In intc-sh7372.c you have the following:
INTCA (no resources - using the 0xe6xxxxxx 1:1 mapping):
static DECLARE_INTC_DESC(intca_desc, "sh7372-intca",
intca_vectors, intca_groups,
intca_mask_registers, intca_prio_registers,
NULL);
INTCS (resources - relies on ioremap()):
static struct resource intcs_resources[] __initdata = {
[0] = {
.start = 0xffd20000,
.end = 0xffd201ff,
.flags = IORESOURCE_MEM,
},
[1] = {
.start = 0xffd50000,
.end = 0xffd501ff,
.flags = IORESOURCE_MEM,
}
};
static struct intc_desc intcs_desc __initdata = {
.name = "sh7372-intcs",
.force_enable = ENABLED_INTCS,
.skip_syscore_suspend = true,
.resource = intcs_resources,
.num_resources = ARRAY_SIZE(intcs_resources),
.hw = INTC_HW_DESC(intcs_vectors, intcs_groups, intcs_mask_registers,
intcs_prio_registers, NULL, NULL),
};
Adding I/O memory resources to the already existing INTC controllers
is not a particularly difficult task. Would you like us to perform
such a change?
The main worry is probably that people will take the platform code
as example when writing device drivers, and that uses hardcoded
IOMEM() macros. There are probably a couple of instances where that
is the best solution, but for those, I would suggest using offsets
from a base register that gets passed into iotable_init() rather
than literal numbers.
If you look at all our regular device drivers we use a base register +
offset. In such cases that kind of design makes a lot of sense.
In the case of INTC and PFC we do not follow this style. This since
each version of the hardware block varies quite a bit and there often
are a couple of I/O memory windows associated with each hardware block
instance. So the design has been to use the same physical addresses as
are described in the data sheet, and for bit fields and register width
follow the same type of representation as the data sheet to allow easy
development and validation.
Keep in mind that in arch/sh and arch/arm we have over 30 different
variants of INTC hardware blocks.
So to summarize, INTC, PFC and CPG (clocks) have ioremap support
included in the actual driver code. If the SoC makes use of it or not
is a different question. =)
quoted
As I mentioned before, from my point of view the main limiting factor
for mach-shmobile multiplatform at this point is the clock framework.
The SH clock framework does already support ioremap() though, so it is
just a matter of making the clock code actually use it. And while
we're doing that we may as well solve the multiplatform issue to and
move towards common clocks.
Ok, good to hear.
So how would you like to proceed with this matter?
Thanks,
/ magnus