ARM: EXYNOS: Map SYSRAM through generic SRAM bindings
PATCH v2 1/2 ARM: EXYNOS: Map SYSRAM through generic SRAM bindings
Instead of hardcoding the SYSRAM details for each SoC, pass this information through device tree (DT) and make the code SoC agnostic. Generic SRAM bindings are used for achieving this.
---
Changes since v1:
* Addressed Tomasz Figa's comments
- Fixed sram node for universal_c210
- Check the node status before mapping the address
This patch is based on linux next (next-20140501) on top of my Kconfig consolidation patch
http://comments.gmane.org/gmane.linux.kernel.samsung-soc/28642
Tested on 4210/4412 Origen, 5250/5420 Arndale and SMDK5420 boards.
--- arch/arm/Kconfig | 1 + arch/arm/boot/dts/exynos4210-universal_c210.dts | 24 +++++++++ arch/arm/boot/dts/exynos4210.dtsi | 18 +++++++ arch/arm/boot/dts/exynos4x12.dtsi | 18 +++++++ arch/arm/boot/dts/exynos5250.dtsi | 18 +++++++ arch/arm/boot/dts/exynos5420.dtsi | 18 +++++++ arch/arm/mach-exynos/common.h | 1 + arch/arm/mach-exynos/exynos.c | 64 ----------------------- arch/arm/mach-exynos/firmware.c | 8 ++- arch/arm/mach-exynos/include/mach/map.h | 7 --- arch/arm/mach-exynos/platsmp.c | 35 +++++++++++-- 11 files changed, 137 insertions(+), 75 deletions(-)
arch/arm/Kconfig +1 -0
@@ -855,6 +855,7 @@ config ARCH_EXYNOS select S5P_DEV_MFC select SAMSUNG_DMADEV select SPARSE_IRQ + select SRAM select USE_OF help Support for SAMSUNG's EXYNOS SoCs (EXYNOS4/5)
arch/arm/boot/dts/exynos4210-universal_c210.dts +24 -0
@@ -28,6 +28,30 @@ bootargs = "console=ttySAC2,115200N8 root=/dev/mmcblk0p5 rw rootwait earlyprintk panic=5 maxcpus=1"; }; + sram at 02020000 { + status = "disabled"; + smp-sram at 0 { + status = "disabled"; + }; + + smp-sram at 1f000 { + status = "disabled"; + }; + }; + + sram at 02025000 { + compatible = "mmio-sram"; + reg = <0x02025000 0x1000>; + #address-cells = <1>; + #size-cells = <1>; + ranges = <0 0x02025000 0x1000>; + + smp-sram at 0 { + compatible = "samsung,exynos4210-sram"; + reg = <0x0 0x1000>; + }; + }; + mct at 10050000 { compatible = "none"; };
arch/arm/boot/dts/exynos4210.dtsi +18 -0
@@ -31,6 +31,24 @@ pinctrl2 = &pinctrl_2; }; + sram at 02020000 { + compatible = "mmio-sram"; + reg = <0x02020000 0x20000>; + #address-cells = <1>; + #size-cells = <1>; + ranges = <0 0x02020000 0x20000>; + + smp-sram at 0 { + compatible = "samsung,exynos4210-sram"; + reg = <0x0 0x1000>; + }; + + smp-sram at 1f000 { + compatible = "samsung,exynos4210-sram-ns"; + reg = <0x1f000 0x1000>; + }; + }; + pd_lcd1: lcd1-power-domain at 10023CA0 { compatible = "samsung,exynos4210-pd"; reg = <0x10023CA0 0x20>;
arch/arm/boot/dts/exynos4x12.dtsi +18 -0
@@ -37,6 +37,24 @@ interrupts = <2 2>, <3 2>, <18 2>, <19 2>; }; + sram at 02020000 { + compatible = "mmio-sram"; + reg = <0x02020000 0x40000>; + #address-cells = <1>; + #size-cells = <1>; + ranges = <0 0x02020000 0x40000>; + + smp-sram at 0 { + compatible = "samsung,exynos4210-sram"; + reg = <0x0 0x1000>; + }; + + smp-sram at 2f000 { + compatible = "samsung,exynos4210-sram-ns"; + reg = <0x2f000 0x1000>; + }; + }; + pd_isp: isp-power-domain at 10023CA0 { compatible = "samsung,exynos4210-pd"; reg = <0x10023CA0 0x20>;
arch/arm/boot/dts/exynos5250.dtsi +18 -0
@@ -72,6 +72,24 @@ }; }; + sram at 02020000 { + compatible = "mmio-sram"; + reg = <0x02020000 0x30000>; + #address-cells = <1>; + #size-cells = <1>; + ranges = <0 0x02020000 0x30000>; + + smp-sram at 0 { + compatible = "samsung,exynos4210-sram"; + reg = <0x0 0x1000>; + }; + + smp-sram at 2f000 { + compatible = "samsung,exynos4210-sram-ns"; + reg = <0x2f000 0x1000>; + }; + }; + pd_gsc: gsc-power-domain at 10044000 { compatible = "samsung,exynos4210-pd"; reg = <0x10044000 0x20>;
arch/arm/boot/dts/exynos5420.dtsi +18 -0
@@ -110,6 +110,24 @@ }; }; + sram at 02020000 { + compatible = "mmio-sram"; + reg = <0x02020000 0x54000>; + #address-cells = <1>; + #size-cells = <1>; + ranges = <0 0x02020000 0x54000>; + + smp-sram at 0 { + compatible = "samsung,exynos4210-sram"; + reg = <0x0 0x1000>; + }; + + smp-sram at 53000 { + compatible = "samsung,exynos4210-sram-ns"; + reg = <0x53000 0x1000>; + }; + }; + clock: clock-controller at 10010000 { compatible = "samsung,exynos5420-clock"; reg = <0x10010000 0x30000>;
arch/arm/mach-exynos/common.h +1 -0
@@ -18,6 +18,7 @@void mct_init (void __iomem *base,int irq_g0,int irq_l0,int irq_l1);struct map_desc ; +extern void __iomem *sram_ns_base_addr;void exynos_init_io (void );void exynos_restart (enum reboot_mode mode,const char *cmd);void exynos_cpuidle_init (void );
arch/arm/mach-exynos/exynos.c +0 -64
82 lines not on this page · raw message
arch/arm/mach-exynos/firmware.c +7 -1
@@ -18,6 +18,7 @@#include <mach/map.h> +#include "common.h" #include "smc.h" static int exynos_do_idle (void )
@@ -34,7 +35,12 @@ static int exynos_cpu_boot(int cpu)static int exynos_set_cpu_boot_addr (int cpu,unsigned long boot_addr) { -void __iomem *boot_reg =S5P_VA_SYSRAM_NS +0x1c +4 *cpu; +void __iomem *boot_reg; + +if (!sram_ns_base_addr) +return 0 ; + + boot_reg = sram_ns_base_addr +0x1c +4 *cpu;__raw_writel (boot_addr, boot_reg);return 0 ;
arch/arm/mach-exynos/include/mach/map.h +0 -7
@@ -23,13 +23,6 @@#include <plat/map-s5p.h> -#define EXYNOS4_PA_SYSRAM0 0x02025000 -#define EXYNOS4_PA_SYSRAM1 0x02020000 -#define EXYNOS5_PA_SYSRAM 0x02020000 -#define EXYNOS4210_PA_SYSRAM_NS 0x0203F000 -#define EXYNOS4x12_PA_SYSRAM_NS 0x0204F000 -#define EXYNOS5250_PA_SYSRAM_NS 0x0204F000 -#define EXYNOS_PA_CHIPID 0x10000000#define EXYNOS4_PA_SYSCON 0x10010000
arch/arm/mach-exynos/platsmp.c +32 -3
@@ -20,6 +20,7 @@#include <linux/jiffies.h> #include <linux/smp.h> #include <linux/io.h> +#include <linux/of_address.h> #include <asm/cacheflush.h> #include <asm/smp_plat.h>
@@ -33,11 +34,35 @@extern void exynos4_secondary_startup (void ); +static void __iomem *sram_base_addr; +void __iomem *sram_ns_base_addr; + +static void __init exynos_smp_prepare_sram (void ) +{ +struct device_node *node; + +for_each_compatible_node (node,NULL ,"samsung,exynos4210-sram" ) { +if (of_device_is_available (node)) { + sram_base_addr =of_iomap (node,0 ); +if (!sram_base_addr) +pr_err ("Secondary CPU boot address not found\n" ); + } + } + +for_each_compatible_node (node,NULL ,"samsung,exynos4210-sram-ns" ) { +if (of_device_is_available (node)) { + sram_ns_base_addr =of_iomap (node,0 ); +if (!sram_ns_base_addr) +pr_err ("Secondary CPU boot address not found\n" ); + } + } +} +static inline void __iomem *cpu_boot_reg_base (void ) {if (soc_is_exynos4210 () &&samsung_rev () ==EXYNOS4210_REV_1_1 )return S5P_INFORM5 ; -return S5P_VA_SYSRAM ; +return sram_base_addr; }static inline void __iomem *cpu_boot_reg (int cpu)
@@ -147,7 +172,8 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)* and fall back to boot register if it fails. */ if (call_firmware_op (set_cpu_boot_addr, phys_cpu, boot_addr)) -__raw_writel (boot_addr,cpu_boot_reg (phys_cpu)); +if (cpu_boot_reg_base ()) +__raw_writel (boot_addr,cpu_boot_reg (phys_cpu));call_firmware_op (cpu_boot, phys_cpu);
@@ -205,6 +231,8 @@ static void __init exynos_smp_prepare_cpus(unsigned int max_cpus)if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 )scu_enable (scu_base_addr ()); +exynos_smp_prepare_sram (); +/* * Write the address of secondary startup into the * system-wide flags register. The boot monitor waits
@@ -222,7 +250,8 @@ static void __init exynos_smp_prepare_cpus(unsigned int max_cpus) boot_addr =virt_to_phys (exynos4_secondary_startup);if (call_firmware_op (set_cpu_boot_addr, phys_cpu, boot_addr)) -__raw_writel (boot_addr,cpu_boot_reg (phys_cpu)); +if (cpu_boot_reg_base ()) +__raw_writel (boot_addr,cpu_boot_reg (phys_cpu)); } }
Signature · 1 line
1.7.9.5
PATCH v2 2/2 Documentation: DT: Exynos: Bind SRAM though DT
Add SRAM binding documentation.
---
No changes since v1.
--- .../devicetree/bindings/arm/exynos/smp-sram.txt | 38 ++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 Documentation/devicetree/bindings/arm/exynos/smp-sram.txt
Documentation/devicetree/bindings/arm/exynos/smp-sram.txt new file +38 -0
@@ -0,0 +1,38 @@ +Samsung Exynos SRAM for SMP bringup: +------------------------------------ + +Samsung SMP-capable Exynos SoCs use part of the SRAM for the bringup +of the secondary cores. Once the core gets powered up it executes the +code that is residing at some specific location of the SRAM. + +Therefore reserved section sub-nodes have to be added to the mmio-sram +declaration. These nodes are of two types depending upon secure or +non-secure execution environment. + +Required sub-node properties: +- compatible : depending upon boot mode, should be + "samsung,exynos4210-sram" : for Secure SYSRAM + "samsung,exynos4210-sram-ns" : for Non-secure SYSRAM + +The rest of the properties should follow the generic mmio-sram discription +found in ../../misc/sram.txt + +Example: + + sram at 02020000 { + compatible = "mmio-sram"; + reg = <0x02020000 0x54000>; + #address-cells = <1>; + #size-cells = <1>; + ranges = <0 0x02020000 0x54000>; + + smp-sram at 0 { + compatible = "samsung,exynos4210-sram"; + reg = <0x0 0x1000>; + }; + + smp-sram at 53000 { + compatible = "samsung,exynos4210-sram-ns"; + reg = <0x53000 0x1000>; + }; + };
Signature · 1 line
1.7.9.5
PATCH v2 1/2 ARM: EXYNOS: Map SYSRAM through generic SRAM bindings
In reply to Sachin KamatHi Sachin,
Thanks for addressing the comments. I need to verify few things on Universal C210 board first, before I could give my Reviewed-by tag or further comments.
I also have some general comments that I missed before, due to limited time for review. Please see inline.
Quoted 17 lines · Sachin Kamat
Instead of hardcoding the SYSRAM details for each SoC, pass this information through device tree (DT) and make the code SoC agnostic. Generic SRAM bindings are used for achieving this.
Signed-off-by Sachin Kamat <[email protected]>Acked-by Arnd Bergmann <[email protected]>Acked-by Heiko Stuebner <[email protected]>---
Changes since v1:
* Addressed Tomasz Figa's comments
- Fixed sram node foruniversal_c210
- Check the node status before mapping the addressThis patch is based on linux next (next-20140501) on top of my
Kconfigconsolidation patch
http://comments.gmane.org/gmane.linux.kernel.samsung-soc/28642
[snip]
Quoted 21 lines
arch/arm/boot/dts/exynos4210-universal_c210.dts +13
-0@@ -28,6 +28,30 @@ bootargs = "console=ttySAC2,115200N8 root=/dev/mmcblk0p5 rw rootwait earlyprintk panic=5 maxcpus=1"; }; + sram at 02020000 { + status = "disabled"; + smp-sram at 0 { + status = "disabled"; + }; + + smp-sram at 1f000 { + status = "disabled"; + }; + }; + + sram at 02025000 { + compatible = "mmio-sram";
arch/arm/boot/dts/exynos4210-universal_c210.dts line 53
Quoted 10 lines
arch/arm/boot/dts/exynos4210-universal_c210.dts +10
-0+ reg = <0x02025000 0x1000>; + #address-cells = <1>; + #size-cells = <1>; + ranges = <0 0x02025000 0x1000>; + + smp-sram at 0 { + compatible = "samsung,exynos4210-sram"; + reg = <0x0 0x1000>; + }; + };
I wonder if this really should be defined like this. 0x2025000 really looks just like an offset from the normal SRAM address. This is the thing I need to check in documentation and by experiment when I'll return back to work tomorrow, but maybe it could be possible to normally use the sram at 02020000 and just disable smp-sram at 0 and smp-sram at 1f000, replacing them with smp-sram at 5000 on Universal C210.
Quoted 4 lines
arch/arm/boot/dts/exynos4210-universal_c210.dts +1
-0+ mct at 10050000 { compatible = "none"; };
[snip]
Quoted 13 lines
arch/arm/mach-exynos/firmware.c +1
-0@@ -18,6 +18,7 @@#include <mach/map.h> +#include "common.h" #include "smc.h" static int exynos_do_idle (void )
arch/arm/mach-exynos/firmware.c static int exynos_cpu_boot(int cpu) · line 41
Quoted 8 lines
arch/arm/mach-exynos/firmware.c +4
-1@@ -34,7 +35,12 @@ static int exynos_cpu_boot(int cpu)static int exynos_set_cpu_boot_addr (int cpu,unsigned long boot_addr) { -void __iomem *boot_reg =S5P_VA_SYSRAM_NS +0x1c +4 *cpu; +void __iomem *boot_reg; + +if (!sram_ns_base_addr) +return 0 ;
Shouldn't this return an error instead? I'm not sure which one would be appropriate, though, probably one of -ENODEV, -ENXIO or -EFAULT.
Quoted 36 lines
arch/arm/mach-exynos/firmware.c +2
-0+ + boot_reg = sram_ns_base_addr +0x1c +4 *cpu;__raw_writel (boot_addr, boot_reg);return 0 ;arch/arm/mach-exynos/include/mach/map.h +0
-7@@ -23,13 +23,6 @@#include <plat/map-s5p.h> -#define EXYNOS4_PA_SYSRAM0 0x02025000 -#define EXYNOS4_PA_SYSRAM1 0x02020000 -#define EXYNOS5_PA_SYSRAM 0x02020000 -#define EXYNOS4210_PA_SYSRAM_NS 0x0203F000 -#define EXYNOS4x12_PA_SYSRAM_NS 0x0204F000 -#define EXYNOS5250_PA_SYSRAM_NS 0x0204F000 -#define EXYNOS_PA_CHIPID 0x10000000#define EXYNOS4_PA_SYSCON 0x10010000arch/arm/mach-exynos/platsmp.c +1
-0@@ -20,6 +20,7 @@#include <linux/jiffies.h> #include <linux/smp.h> #include <linux/io.h> +#include <linux/of_address.h> #include <asm/cacheflush.h> #include <asm/smp_plat.h>
arch/arm/mach-exynos/platsmp.c line 38
Quoted 5 lines
arch/arm/mach-exynos/platsmp.c +2
-0@@ -33,11 +34,35 @@extern void exynos4_secondary_startup (void ); +static void __iomem *sram_base_addr; +void __iomem *sram_ns_base_addr;
This is probably just a matter of preference, but I'd make this static and provide a getter function, like exynos_get_sram_ns_base();
Also this address seems to be used by the firmware code exclusively. If we want to make the firmware code more self-contained, maybe the mapping of firmware-specific SRAM region could be handled there, instead? This would also eliminate the need for having an exported variable or getter function. What do you think?
arch/arm/mach-exynos/platsmp.c line 48
Quoted 10 lines
arch/arm/mach-exynos/platsmp.c +10
-0+ +static void __init exynos_smp_prepare_sram (void ) +{ +struct device_node *node; + +for_each_compatible_node (node,NULL ,"samsung,exynos4210-sram" ) { +if (of_device_is_available (node)) { + sram_base_addr =of_iomap (node,0 ); +if (!sram_base_addr) +pr_err ("Secondary CPU boot address not found\n" );
I don't think this is an error at this stage. Some platforms might not have either of these SRAM reserved regions (such as those using INFORM registers instead). Instead, the base address should be checked whenever it is needed and errors should be handled then, like in exynos_set_cpu_boot_addr().
arch/arm/mach-exynos/platsmp.c line 50
Quoted 2 lines
arch/arm/mach-exynos/platsmp.c +2
-0+ } + }
Also we don't need to look further in DT after we find a matching node already. So combining both comments the resulting code would be:
for_each_compatible_node(node, NULL, "samsung,exynos4210-sram") {
if (!of_device_is_available(node))
continue;
sram_base_addr = of_iomap(node, 0);
break;
}arch/arm/mach-exynos/platsmp.c line 58
Quoted 8 lines
arch/arm/mach-exynos/platsmp.c +8
-0+ +for_each_compatible_node (node,NULL ,"samsung,exynos4210-sram-ns" ) { +if (of_device_is_available (node)) { + sram_ns_base_addr =of_iomap (node,0 ); +if (!sram_ns_base_addr) +pr_err ("Secondary CPU boot address not found\n" ); + } + }
Same comments here.
Quoted 12 lines
arch/arm/mach-exynos/platsmp.c +3
-1+} +static inline void __iomem *cpu_boot_reg_base (void ) {if (soc_is_exynos4210 () &&samsung_rev () ==EXYNOS4210_REV_1_1 )return S5P_INFORM5 ; -return S5P_VA_SYSRAM ; +return sram_base_addr; }static inline void __iomem *cpu_boot_reg (int cpu)
arch/arm/mach-exynos/platsmp.c static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle) · line 176
Quoted 6 lines
arch/arm/mach-exynos/platsmp.c +2
-1@@ -147,7 +172,8 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)* and fall back to boot register if it fails. */ if (call_firmware_op (set_cpu_boot_addr, phys_cpu, boot_addr)) -__raw_writel (boot_addr,cpu_boot_reg (phys_cpu)); +if (cpu_boot_reg_base ()) +__raw_writel (boot_addr,cpu_boot_reg (phys_cpu));
I'd rework this for proper error handling, e.g.
int ret;
/* ... */
ret = call_firmware_op(set_cpu_boot_addr, phys_cpu, boot_addr);
if (ret && ret != -ENOSYS)
goto fail;
if (ret == -ENOSYS) {
/* Fall back to firmware-less way. */
void __iomem *boot_reg = cpu_boot_reg(phys_cpu);
if (IS_ERR(boot_reg)) {
ret = PTR_ERR(boot_reg);
goto fail;
}
}
/* ... */fail:
/* Clean-up after error */
Of course, cpu_boot_reg() will need to be converted to follow the ERR_PTR() model, but IMHO proper error handling is a good reason to do so.
Quoted 13 lines
arch/arm/mach-exynos/platsmp.c +2
-0call_firmware_op (cpu_boot, phys_cpu);@@ -205,6 +231,8 @@ static void __init exynos_smp_prepare_cpus(unsigned int max_cpus)if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 )scu_enable (scu_base_addr ()); +exynos_smp_prepare_sram (); +/* * Write the address of secondary startup into the * system-wide flags register. The boot monitor waits
arch/arm/mach-exynos/platsmp.c static void __init exynos_smp_prepare_cpus(unsigned int max_cpus) · line 254
Quoted 6 lines
arch/arm/mach-exynos/platsmp.c +2
-1@@ -222,7 +250,8 @@ static void __init exynos_smp_prepare_cpus(unsigned int max_cpus) boot_addr =virt_to_phys (exynos4_secondary_startup);if (call_firmware_op (set_cpu_boot_addr, phys_cpu, boot_addr)) -__raw_writel (boot_addr,cpu_boot_reg (phys_cpu)); +if (cpu_boot_reg_base ()) +__raw_writel (boot_addr,cpu_boot_reg (phys_cpu));
I wonder if setting the addresses at this stage is really needed. IMHO doing it once in exynos_boot_secondary() should be enough. But this is probably a material for further patch.
Best regards,
Tomasz
PATCH v2 1/2 ARM: EXYNOS: Map SYSRAM through generic SRAM bindings
Hi Tomasz,
Quoted 8 lines · Tomasz Figa
Hi Sachin,
Thanks for addressing the comments. I need to verify few things on Universal C210 board first, before I could give my Reviewed-by tag or further comments.
I also have some general comments that I missed before, due to limited time for review. Please see inline.
Thanks for your review.
Quoted 61 lines · Sachin Kamat
Quoted 18 lines
Instead of hardcoding the SYSRAM details for each SoC, pass this information through device tree (DT) and make the code SoC agnostic. Generic SRAM bindings are used for achieving this.
Signed-off-by Sachin Kamat <[email protected]>Acked-by Arnd Bergmann <[email protected]>Acked-by Heiko Stuebner <[email protected]>---
Changes since v1:
* Addressed Tomasz Figa's comments
- Fixed sram node foruniversal_c210
- Check the node status before mapping the addressThis patch is based on linux next (next-20140501) on top of my
Kconfigconsolidation patch
http://comments.gmane.org/gmane.linux.kernel.samsung-soc/28642[snip]
Quoted 23 lines
arch/arm/boot/dts/exynos4210-universal_c210.dts +13
-0@@ -28,6 +28,30 @@ bootargs = "console=ttySAC2,115200N8 root=/dev/mmcblk0p5 rw rootwait earlyprintk panic=5 maxcpus=1"; }; + sram at 02020000 { + status = "disabled"; + smp-sram at 0 { + status = "disabled"; + }; + + smp-sram at 1f000 { + status = "disabled"; + }; + }; + + sram at 02025000 { + compatible = "mmio-sram";arch/arm/boot/dts/exynos4210-universal_c210.dts line 54
Quoted 10 lines
arch/arm/boot/dts/exynos4210-universal_c210.dts +10
-0+ reg = <0x02025000 0x1000>; + #address-cells = <1>; + #size-cells = <1>; + ranges = <0 0x02025000 0x1000>; + + smp-sram at 0 { + compatible = "samsung,exynos4210-sram"; + reg = <0x0 0x1000>; + }; + };
Quoted 6 lines
I wonder if this really should be defined like this.
0x2025000really looks just like an offset from the normal SRAM address. This is the thing I need to check in documentation and by experiment when I'll return back to work tomorrow, but maybe it could be possible to normally use the sram at 02020000 and just disable smp-sram at 0 and smp-sram at 1f000, replacing them with smp-sram at 5000 on Universal C210.
I do not have any info about universal C210 board and need your inputs for the same :)
Quoted 26 lines
Quoted 6 lines
arch/arm/boot/dts/exynos4210-universal_c210.dts +1
-0+ mct at 10050000 { compatible = "none";};[snip]
Quoted 15 lines
arch/arm/mach-exynos/firmware.c +1
-0@@ -18,6 +18,7 @@#include <mach/map.h> +#include "common.h" #include "smc.h" static int exynos_do_idle (void )@@ -34,7 +35,12 @@ static int exynos_cpu_boot(int cpu)
Quoted 11 lines
arch/arm/mach-exynos/firmware.c static int exynos_cpu_boot(int cpu) · line 41
Quoted 7 lines
arch/arm/mach-exynos/firmware.c +4
-1static int exynos_set_cpu_boot_addr (int cpu,unsigned long boot_addr) { -void __iomem *boot_reg =S5P_VA_SYSRAM_NS +0x1c +4 *cpu; +void __iomem *boot_reg; + +if (!sram_ns_base_addr) +return 0 ;Shouldn't this return an error instead? I'm not sure which one would be appropriate, though, probably one of
-ENODEV,-ENXIOor-EFAULT.
IIRC, returning error here causes the system to hang and even primary cpu does not boot.
Since any error or absence of sram nodes should atleast boot the primary CPU, I thought
this is better.
Quoted 50 lines
Quoted 40 lines
arch/arm/mach-exynos/firmware.c +2
-0+ + boot_reg = sram_ns_base_addr +0x1c +4 *cpu;__raw_writel (boot_addr, boot_reg);return 0 ;arch/arm/mach-exynos/include/mach/map.h +0
-7@@ -23,13 +23,6 @@#include <plat/map-s5p.h> -#define EXYNOS4_PA_SYSRAM0 0x02025000 -#define EXYNOS4_PA_SYSRAM1 0x02020000 -#define EXYNOS5_PA_SYSRAM 0x02020000 -#define EXYNOS4210_PA_SYSRAM_NS 0x0203F000 -#define EXYNOS4x12_PA_SYSRAM_NS 0x0204F000 -#define EXYNOS5250_PA_SYSRAM_NS 0x0204F000 -#define EXYNOS_PA_CHIPID 0x10000000#define EXYNOS4_PA_SYSCON 0x10010000arch/arm/mach-exynos/platsmp.c +1
-0@@ -20,6 +20,7 @@#include <linux/jiffies.h> #include <linux/smp.h> #include <linux/io.h> +#include <linux/of_address.h> #include <asm/cacheflush.h> #include <asm/smp_plat.h> arch/arm/mach-exynos/platsmp.c line 38
Quoted 5 lines
arch/arm/mach-exynos/platsmp.c +2
-0@@ -33,11 +34,35 @@extern void exynos4_secondary_startup (void ); +static void __iomem *sram_base_addr; +void __iomem *sram_ns_base_addr;This is probably just a matter of preference, but I'd make this static and provide a getter function, like
exynos_get_sram_ns_base();
Quoted 5 lines
Also this address seems to be used by the firmware code exclusively. If we want to make the firmware code more self-contained, maybe the mapping of firmware-specific SRAM region could be handled there, instead? This would also eliminate the need for having an exported variable or getter function. What do you think?
I thought of the same. However 2 reasons prevented me from implementing this.
1. Code duplication
2. This code should be executed only once. I put it in exynos_firmware_init. However it gave a crash while doing of_iomap. So moved it back to the current location.
Quoted 11 lines
Quoted 11 lines
arch/arm/mach-exynos/platsmp.c +9
-0+ +static void __init exynos_smp_prepare_sram (void ) +{ +struct device_node *node; + +for_each_compatible_node (node,NULL ,"samsung,exynos4210-sram" ) { +if (of_device_is_available (node)) { + sram_base_addr =of_iomap (node,0 ); +if (!sram_base_addr)
Quoted 8 lines
arch/arm/mach-exynos/platsmp.c line 49
Quoted 2 lines
arch/arm/mach-exynos/platsmp.c +1
-0+pr_err ("Secondary CPU boot address not found\n");I don't think this is an error at this stage. Some platforms might not have either of these SRAM reserved regions (such as those using INFORM registers instead). Instead, the base address should be checked whenever it is needed and errors should be handled then, like in
exynos_set_cpu_boot_addr().
Yes. This is more from an information perspective. Probably pr_warn or pr_info would
be better?
Quoted 8 lines
arch/arm/mach-exynos/platsmp.c line 51
Quoted 3 lines
arch/arm/mach-exynos/platsmp.c +2
-0+ } + }Also we don't need to look further in DT after we find a matching node already. So combining both comments the resulting code would be:
Quoted 7 lines
for_each_compatible_node(node, NULL, "samsung,exynos4210-sram") { if (!of_device_is_available(node)) continue; sram_base_addr = of_iomap(node, 0); break;}
OK.
Quoted 13 lines
arch/arm/mach-exynos/platsmp.c line 58
Quoted 11 lines
arch/arm/mach-exynos/platsmp.c +8
-0+ +for_each_compatible_node (node,NULL ,"samsung,exynos4210-sram-ns" ) { +if (of_device_is_available (node)) { + sram_ns_base_addr =of_iomap (node,0 ); +if (!sram_ns_base_addr) +pr_err ("Secondary CPU boot address not found\n"); + } + }
Quoted 1 line
Same comments here.
OK.
Quoted 46 lines
Quoted 14 lines
arch/arm/mach-exynos/platsmp.c +3
-1+} +static inline void __iomem *cpu_boot_reg_base (void ) {if (soc_is_exynos4210 () &&samsung_rev () ==EXYNOS4210_REV_1_1 )return S5P_INFORM5 ; -return S5P_VA_SYSRAM ; +return sram_base_addr;}static inline void
__iomem*cpu_boot_reg(int cpu)arch/arm/mach-exynos/platsmp.c static int exynos_boot_secondary(unsigned int cpu, · line 179
Quoted 9 lines
arch/arm/mach-exynos/platsmp.c +2
-1@@ -147,7 +172,8 @@ static int exynos_boot_secondary(unsigned int cpu,struct task_struct *idle) * and fall back to boot register if it fails. */ if (call_firmware_op (set_cpu_boot_addr, phys_cpu, boot_addr)) -__raw_writel (boot_addr,cpu_boot_reg (phys_cpu)); +if (cpu_boot_reg_base ()) +__raw_writel (boot_addr,cpu_boot_reg (phys_cpu));I'd rework this for proper error handling, e.g.
int ret; /* ... */ ret = call_firmware_op(set_cpu_boot_addr, phys_cpu, boot_addr); if (ret && ret != -ENOSYS) goto fail; if (ret == -ENOSYS) { /* Fall back to firmware-less way. */ void __iomem *boot_reg = cpu_boot_reg(phys_cpu); if (IS_ERR(boot_reg)) { ret = PTR_ERR(boot_reg); goto fail; } } /* ... */
Quoted 5 lines
fail:
/* Clean-up after error */Of course,
cpu_boot_reg()will need to be converted to follow theERR_PTR()model, but IMHO proper error handling is a good reason to do so.
How about handling this separately outside this patch?
Quoted 19 lines
Quoted 19 lines
call_firmware_op(cpu_boot, phys_cpu);arch/arm/mach-exynos/platsmp.c +2
-0@@ -205,6 +231,8 @@ static void __init exynos_smp_prepare_cpus(unsigned int max_cpus)if (read_cpuid_part_number () ==ARM_CPU_PART_CORTEX_A9 )scu_enable (scu_base_addr ()); +exynos_smp_prepare_sram (); +/* * Write the address of secondary startup into the * system-wide flags register. The boot monitor waits
arch/arm/mach-exynos/platsmp.c +0
-0@@ -222,7 +250,8 @@ static void __init exynos_smp_prepare_cpus(unsigned int max_cpus) boot_addr =virt_to_phys (exynos4_secondary_startup);
Quoted 11 lines
arch/arm/mach-exynos/platsmp.c static void __init exynos_smp_prepare_cpus(unsigned · line 257
Quoted 6 lines
arch/arm/mach-exynos/platsmp.c +2
-1if (call_firmware_op (set_cpu_boot_addr, phys_cpu, boot_addr)) -__raw_writel (boot_addr,cpu_boot_reg (phys_cpu)); +if (cpu_boot_reg_base ()) +__raw_writel (boot_addr,cpu_boot_reg (phys_cpu));I wonder if setting the addresses at this stage is really needed. IMHO doing it once in
exynos_boot_secondary()should be enough. But this is probably a material for further patch.
Sure.
Signature · 2 lines
With warm regards, Sachin
PATCH v2 1/2 ARM: EXYNOS: Map SYSRAM through generic SRAM bindings
Hi Tomasz,
Quoted 27 lines · Sachin Kamat
Hi Tomasz,
Quoted 8 lines · Tomasz Figa
Hi Sachin,
Thanks for addressing the comments. I need to verify few things on Universal C210 board first, before I could give my Reviewed-by tag or further comments.
I also have some general comments that I missed before, due to limited time for review. Please see inline.
Thanks for your review.
Quoted 13 lines · Sachin Kamat
Quoted 10 lines
Instead of hardcoding the SYSRAM details for each SoC, pass this information through device tree (DT) and make the code SoC agnostic. Generic SRAM bindings are used for achieving this.
Signed-off-by Sachin Kamat <[email protected]>Acked-by Arnd Bergmann <[email protected]>Acked-by Heiko Stuebner <[email protected]>---
[snip]
Quoted 10 lines
Quoted 10 lines
Quoted 8 lines
static int
exynos_set_cpu_boot_addr(int cpu, unsigned long boot_addr){arch/arm/mach-exynos/firmware.c +4
-1-void __iomem *boot_reg =S5P_VA_SYSRAM_NS +0x1c +4 *cpu; +void __iomem *boot_reg; + +if (!sram_ns_base_addr) +return 0 ;
Quoted 8 lines
Quoted 2 lines
Shouldn't this return an error instead? I'm not sure which one would be appropriate, though, probably one of
-ENODEV,-ENXIOor-EFAULT.IIRC, returning error here causes the system to hang and even primary cpu does not boot.
Since any error or absence of sram nodes should atleast boot the primary CPU, I thought
this is better.
Small correction. The above explanation was for the absence of the check. Sorry about the confusion. Will add -ENODEV here.
Signature · 2 lines
With warm regards, Sachin
PATCH v2 1/2 ARM: EXYNOS: Map SYSRAM through generic SRAM bindings
In reply to Sachin KamatOn 06.05.2014 20:09, Sachin Kamat wrote:
[snip]
Quoted 1 line · Tomasz Figa
[snip]
Quoted 1 line
Quoted 1 line · Sachin Kamat
[snip]
Quoted 30 lines
Quoted 24 lines
Quoted 14 lines
arch/arm/mach-exynos/platsmp.c +1
-0@@ -20,6 +20,7 @@#include <linux/jiffies.h> #include <linux/smp.h> #include <linux/io.h> +#include <linux/of_address.h> #include <asm/cacheflush.h> #include <asm/smp_plat.h> arch/arm/mach-exynos/platsmp.c line 38
Quoted 5 lines
arch/arm/mach-exynos/platsmp.c +2
-0@@ -33,11 +34,35 @@extern void exynos4_secondary_startup (void ); +static void __iomem *sram_base_addr; +void __iomem *sram_ns_base_addr;This is probably just a matter of preference, but I'd make this static and provide a getter function, like
exynos_get_sram_ns_base();Quoted 5 lines
Also this address seems to be used by the firmware code exclusively. If we want to make the firmware code more self-contained, maybe the mapping of firmware-specific SRAM region could be handled there, instead? This would also eliminate the need for having an exported variable or getter function. What do you think?
Quoted 7 lines
I thought of the same. However 2 reasons prevented me from implementing this.
1. Code duplication
2. This code should be executed only once. I put it inexynos_firmware_init. However it gave a crash while doingof_iomap. So moved it back to the current location.
Right, exynos_firmware_init() is called too early, before ioremap() is available, so there is no good place to put this in firmware code. So, let's keep this as is for now and eventually move it to firmware if needed and/or a proper method of initialization is available.
Quoted 20 lines
Quoted 11 lines
Quoted 11 lines
arch/arm/mach-exynos/platsmp.c +9
-0+ +static void __init exynos_smp_prepare_sram (void ) +{ +struct device_node *node; + +for_each_compatible_node (node,NULL ,"samsung,exynos4210-sram" ) { +if (of_device_is_available (node)) { + sram_base_addr =of_iomap (node,0 ); +if (!sram_base_addr)Quoted 8 lines
arch/arm/mach-exynos/platsmp.c line 49
Quoted 2 lines
arch/arm/mach-exynos/platsmp.c +1
-0+pr_err ("Secondary CPU boot address not found\n");I don't think this is an error at this stage. Some platforms might not have either of these SRAM reserved regions (such as those using INFORM registers instead). Instead, the base address should be checked whenever it is needed and errors should be handled then, like in
exynos_set_cpu_boot_addr().
Quoted 4 lines
Yes. This is more from an information perspective. Probably
pr_warnorpr_infowould
be better?
IMHO this shouldn't produce any messages at this stage. Just whenever either of addresses is actually needed and is not initialized a message should be printed.
Quoted 44 lines
Quoted 44 lines
Quoted 12 lines
arch/arm/mach-exynos/platsmp.c +3
-1+} +static inline void __iomem *cpu_boot_reg_base (void ) {if (soc_is_exynos4210 () &&samsung_rev () ==EXYNOS4210_REV_1_1 )return S5P_INFORM5 ; -return S5P_VA_SYSRAM ; +return sram_base_addr; }static inline void __iomem *cpu_boot_reg (int cpu)arch/arm/mach-exynos/platsmp.c static int exynos_boot_secondary(unsigned int cpu, · line 179
Quoted 9 lines
arch/arm/mach-exynos/platsmp.c +2
-1@@ -147,7 +172,8 @@ static int exynos_boot_secondary(unsigned int cpu,struct task_struct *idle) * and fall back to boot register if it fails. */ if (call_firmware_op (set_cpu_boot_addr, phys_cpu, boot_addr)) -__raw_writel (boot_addr,cpu_boot_reg (phys_cpu)); +if (cpu_boot_reg_base ()) +__raw_writel (boot_addr,cpu_boot_reg (phys_cpu));I'd rework this for proper error handling, e.g.
int ret; /* ... */ ret = call_firmware_op(set_cpu_boot_addr, phys_cpu, boot_addr); if (ret && ret != -ENOSYS) goto fail; if (ret == -ENOSYS) { /* Fall back to firmware-less way. */ void __iomem *boot_reg = cpu_boot_reg(phys_cpu); if (IS_ERR(boot_reg)) { ret = PTR_ERR(boot_reg); goto fail; } } /* ... */
Quoted 8 lines
Quoted 5 lines
fail:
/* Clean-up after error */Of course,
cpu_boot_reg()will need to be converted to follow theERR_PTR()model, but IMHO proper error handling is a good reason to do so.How about handling this separately outside this patch?
This patch is already changing surroundings of this code, so I'd say it should do it the right way from the start.
Best regards,
Tomasz
PATCH v2 1/2 ARM: EXYNOS: Map SYSRAM through generic SRAM bindings
Hi Tomasz,
Quoted 44 lines · Tomasz Figa
On 06.05.2014 20:09, Sachin Kamat wrote:
[snip]Quoted 1 line · Tomasz Figa
[snip]
Quoted 1 line
Quoted 1 line · Sachin Kamat
[snip]
Quoted 30 lines
Quoted 24 lines
Quoted 14 lines
arch/arm/mach-exynos/platsmp.c +1
-0@@ -20,6 +20,7 @@#include <linux/jiffies.h> #include <linux/smp.h> #include <linux/io.h> +#include <linux/of_address.h> #include <asm/cacheflush.h> #include <asm/smp_plat.h> arch/arm/mach-exynos/platsmp.c line 38
Quoted 5 lines
arch/arm/mach-exynos/platsmp.c +2
-0@@ -33,11 +34,35 @@extern void exynos4_secondary_startup (void ); +static void __iomem *sram_base_addr; +void __iomem *sram_ns_base_addr;This is probably just a matter of preference, but I'd make this static and provide a getter function, like
exynos_get_sram_ns_base();Quoted 5 lines
Also this address seems to be used by the firmware code exclusively. If we want to make the firmware code more self-contained, maybe the mapping of firmware-specific SRAM region could be handled there, instead? This would also eliminate the need for having an exported variable or getter function. What do you think?
Quoted 7 lines
I thought of the same. However 2 reasons prevented me from implementing this.
1. Code duplication
2. This code should be executed only once. I put it inexynos_firmware_init. However it gave a crash while doingof_iomap. So moved it back to the current location.
Quoted 4 lines
Right,
exynos_firmware_init()is called too early, beforeioremap()is available, so there is no good place to put this in firmware code. So, let's keep this as is for now and eventually move it to firmware if needed and/or a proper method of initialization is available.
OK.
Quoted 21 lines
Quoted 21 lines
Quoted 20 lines
Quoted 4 lines
arch/arm/mach-exynos/platsmp.c +1
-0+arch/arm/mach-exynos/platsmp.c
Quoted 10 lines
arch/arm/mach-exynos/platsmp.c +9
-0+static void __init exynos_smp_prepare_sram (void ) +{ +struct device_node *node; + +for_each_compatible_node (node,NULL ,"samsung,exynos4210-sram" ) { +if (of_device_is_available (node)) { + sram_base_addr =of_iomap (node,0 ); +if (!sram_base_addr) +pr_err ("Secondary CPU boot address not found\n");I don't think this is an error at this stage. Some platforms might not have either of these SRAM reserved regions (such as those using INFORM registers instead). Instead, the base address should be checked whenever it is needed and errors should be handled then, like in
exynos_set_cpu_boot_addr().
Quoted 8 lines
Quoted 4 lines
Yes. This is more from an information perspective. Probably
pr_warnorpr_infowould
be better?IMHO this shouldn't produce any messages at this stage. Just whenever either of addresses is actually needed and is not initialized a message should be printed.
OK.
Quoted 47 lines
Quoted 47 lines
Quoted 47 lines
Quoted 12 lines
arch/arm/mach-exynos/platsmp.c +3
-1+} +static inline void __iomem *cpu_boot_reg_base (void ) {if (soc_is_exynos4210 () &&samsung_rev () ==EXYNOS4210_REV_1_1 )return S5P_INFORM5 ; -return S5P_VA_SYSRAM ; +return sram_base_addr; }static inline void __iomem *cpu_boot_reg (int cpu)arch/arm/mach-exynos/platsmp.c static int exynos_boot_secondary(unsigned int cpu, · line 179
Quoted 9 lines
arch/arm/mach-exynos/platsmp.c +2
-1@@ -147,7 +172,8 @@ static int exynos_boot_secondary(unsigned int cpu,struct task_struct *idle) * and fall back to boot register if it fails. */ if (call_firmware_op (set_cpu_boot_addr, phys_cpu, boot_addr)) -__raw_writel (boot_addr,cpu_boot_reg (phys_cpu)); +if (cpu_boot_reg_base ()) +__raw_writel (boot_addr,cpu_boot_reg (phys_cpu));I'd rework this for proper error handling, e.g.
int ret; /* ... */ ret = call_firmware_op(set_cpu_boot_addr, phys_cpu, boot_addr); if (ret && ret != -ENOSYS) goto fail; if (ret == -ENOSYS) { /* Fall back to firmware-less way. */ void __iomem *boot_reg = cpu_boot_reg(phys_cpu); if (IS_ERR(boot_reg)) { ret = PTR_ERR(boot_reg); goto fail; } } /* ... */fail:
/* Clean-up after error */
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Of course,
cpu_boot_reg()will need to be converted to follow theERR_PTR()model, but IMHO proper error handling is a good reason to do so.How about handling this separately outside this patch?
This patch is already changing surroundings of this code, so I'd say it should do it the right way from the start.
Yes, I have incorporated it in my patch. Will send the same tomorrow.
Signature · 2 lines
With warm regards, Sachin