Re: [PATCH 1/3] arm: socfpga: Enable ECC of L2 and OCRAM on startup.

[Thor Thayer <tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>]

On 10/04/2014 11:18 PM, Masami Hiramatsu wrote:
> (2014/10/04 6:42), Dinh Nguyen wrote:
>> On 10/03/2014 04:51 AM, Masami Hiramatsu wrote:
>>> Hi Dinh,
>>>
>>> (2014/10/02 20:38), Dinh Nguyen wrote:
>>>> On 10/1/14, 5:18 PM, Thor Thayer wrote:
>>>>> On 10/01/2014 04:10 PM, Dinh Nguyen wrote:
>>>>>> On 10/1/14, 4:07 PM, Thor Thayer wrote:
>>>>>>> On 10/01/2014 12:13 PM, Dinh Nguyen wrote:
>>>>>>>> On 10/1/14, 11:31 AM,tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org  wrote:
>>>>>>>>> From: Thor Thayer<tthayer-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
>>>>>>>>>
>>>>>> [...]
>>>>>>>>>       static void socfpga_cyclone5_restart(enum reboot_mode mode, const
>>>>>>>>> char *cmd)
>>>>>>>>> @@ -98,6 +101,13 @@ static void socfpga_cyclone5_restart(enum
>>>>>>>>> reboot_mode mode, const char *cmd)
>>>>>>>>>         writel(temp, rst_manager_base_addr + SOCFPGA_RSTMGR_CTRL);
>>>>>>>>>     }
>>>>>>>>>     +static void __init socfpga_cyclone5_init(void)
>>>>>>>>> +{
>>>>>>>>> +    of_platform_populate(NULL, of_default_bus_match_table,
>>>>>>>>> +                 NULL, NULL);
>>>>>>>> Why is this needed?
>>>>>>>>
>>>>>>>> Dinh
>>>>>>> Hi Dinh.
>>>>>>>
>>>>>>> Are you asking why the of_platform_populate() is needed? If so, it is
>>>>>>> used to kick off discovery of devices at the root of the tree.
>>>>>> I was asking about of_platform populate(). This was removed in this
>>>>>> commit:
>>>>>>
>>>>>> 8b5c18f05 ARM: l2c: socfpga: convert to generic l2c OF initialisation
>>>>>>
>>>>>> Just trying to understand what's the need to add it back?
>>>>>>
>>>>>> Dinh
>>>>> It is used to kick off discovery of devices at the root of the tree.
>>>>> This is needed when .init_machine was defined because we're not using
>>>>> the default implementation (which called this function).
>>>>>
>>>> Then, can you please add a separate patch to fix up the removal of the call?
>>> It seems that this patch does enough reasonable thing.
>>>
>>> To initialize ECC on OCRAM, socfpga_init_ocram_ecc() must be invoked
>>> while booting. In that case, he might need this function.
>>>
>>> 8b5c18f05 is to replace l2x0_of_init() with .l2c_aux_* fields, and
>>> this patch has a different reason to call another init function in
>>> .init_machine. In this case, it is natural to define .init_machine
>>> handler in this patch.
>> 8b5c18f05 also removes the need for .init_machine because
>> of_platform_populate() is now called by common arm code.
>>
>>> IOW, both use same socfpga_cyclone5_init() for the function name,
>>> but those have different purpose. I think it is not reviving the
>>> old function, but just adding a new function which has same name.
>>>
>> So I'm just wondering if the call to socfpga_init_ocram_ecc() can be
>> done in socfpga_init_irq() along with the call to socfpga_init_l2_ecc(),
>> so that you don't have to bring back the custom init_machine callback.
> Indeed, it will be better and simpler.
>
> Thank you,
>
Hi. I didn't explain this well in my previous comments.

The call to socfpga_init_ocram_ecc() needs to be in .init_machine 
because socfpga_init_ocram_ecc() needs the device tree structure to be 
populated to dereference the sram handle. It is in the .init_machine 
function that the device tree discovery/population process is kicked off 
so it makes sense to add it here.

I originally tried adding socfpga_init_ocram_ecc() in the 
socfpga_init_irq() but the sram phandle couldn't be parsed.

I'm moving the OCRAM addition into a separate patch as requested by Dinh.

Thor

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html