Hi Tomasz, On 6 May 2014 22:14, Tomasz Figa <tomasz.figa@xxxxxxxxx> wrote: > 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. > > > On 06.05.2014 10:10, Sachin Kamat wrote: >> >> 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 <sachin.kamat@xxxxxxxxxx> >> Acked-by: Arnd Bergmann <arnd@xxxxxxxx> >> Acked-by: Heiko Stuebner <heiko@xxxxxxxxx> >> --- >> 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 > > > [snip] > > >> diff --git a/arch/arm/boot/dts/exynos4210-universal_c210.dts >> b/arch/arm/boot/dts/exynos4210-universal_c210.dts >> index 63e34b24b04f..9813b068cfd8 100644 >> --- a/arch/arm/boot/dts/exynos4210-universal_c210.dts >> +++ b/arch/arm/boot/dts/exynos4210-universal_c210.dts >> @@ -28,6 +28,30 @@ >> bootargs = "console=ttySAC2,115200N8 root=/dev/mmcblk0p5 >> rw rootwait earlyprintk panic=5 maxcpus=1"; >> }; >> >> + sram@02020000 { >> + status = "disabled"; >> + smp-sram@0 { >> + status = "disabled"; >> + }; >> + >> + smp-sram@1f000 { >> + status = "disabled"; >> + }; >> + }; >> + >> + sram@02025000 { >> + compatible = "mmio-sram"; >> + reg = <0x02025000 0x1000>; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + ranges = <0 0x02025000 0x1000>; >> + >> + smp-sram@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@02020000 > and just disable smp-sram@0 and smp-sram@1f000, replacing them with > smp-sram@5000 on Universal C210. I do not have any info about universal C210 board and need your inputs for the same :) > > >> + >> mct@10050000 { >> compatible = "none"; >> }; > > > [snip] > > >> diff --git a/arch/arm/mach-exynos/firmware.c >> b/arch/arm/mach-exynos/firmware.c >> index 932129ef26c6..7d583cb73850 100644 >> --- a/arch/arm/mach-exynos/firmware.c >> +++ b/arch/arm/mach-exynos/firmware.c >> @@ -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; > > > Shouldn't this return an error instead? I'm not sure which one would be > appropriate, though, probably one of -ENODEV, -ENXIO or -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. > > >> + >> + boot_reg = sram_ns_base_addr + 0x1c + 4*cpu; >> >> __raw_writel(boot_addr, boot_reg); >> return 0; >> diff --git a/arch/arm/mach-exynos/include/mach/map.h >> b/arch/arm/mach-exynos/include/mach/map.h >> index 7b046b59d9ec..548269a60634 100644 >> --- a/arch/arm/mach-exynos/include/mach/map.h >> +++ b/arch/arm/mach-exynos/include/mach/map.h >> @@ -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 >> diff --git a/arch/arm/mach-exynos/platsmp.c >> b/arch/arm/mach-exynos/platsmp.c >> index 03e5e9f94705..ccbcdd7b8a86 100644 >> --- a/arch/arm/mach-exynos/platsmp.c >> +++ b/arch/arm/mach-exynos/platsmp.c >> @@ -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; > > > 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? 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. > > >> + >> +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(). Yes. This is more from an information perspective. Probably pr_warn or pr_info would be better? > >> + } >> + } > > > 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; > > } OK. > >> + >> + 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. OK. > > >> +} >> + >> 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)); > > > 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. How about handling this separately outside this patch? > > >> >> 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)); > > > 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. -- With warm regards, Sachin -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
