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

[Tomasz Figa]

On 06.05.2014 20:09, Sachin Kamat wrote:
[snip]
> On 6 May 2014 22:14, Tomasz Figa <tomasz.figa at gmail.com> wrote:
[snip]
>> On 06.05.2014 10:10, Sachin Kamat wrote:
[snip]
>>> 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.
>

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.

>>
>>
>>> +
>>> +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?
> 

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.

>>> +}
>>> +
>>>   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?
> 

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