Re: [PATCH v2 1/2] ARM: EXYNOS: Map SYSRAM through generic SRAM bindings

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux