[U-Boot] [PATCH 1/3] arm: socfpga: Remove unused FPGA feature from gen5 SPL

[Marek Vasut]

On 07/28/2017 07:16 AM, Chee, Tien Fong wrote:
> On Kha, 2017-07-27 at 11:37 +0200, Marek Vasut wrote:
>> On 07/27/2017 11:21 AM, Chee, Tien Fong wrote:
>>>
>>> On Kha, 2017-07-27 at 10:21 +0200, Marek Vasut wrote:
>>>>
>>>> On 07/27/2017 06:36 AM, tien.fong.chee at intel.com wrote:
>>>>>
>>>>>
>>>>> From: Tien Fong Chee <tien.fong.chee at intel.com>
>>>>>
>>>>> Remove unused FPGA feature for saving some space in gen5 SPL.
>>>> I really dislike the ifdefery. Why is this patch even needed?
>>>> I thought we had no space problems in the Gen5 SPL?
>>>>
>>> I can remove those codes within ifdefery because they are no longer
>>> required.
>> Why ?
>>
> because those functions are testing on FPGA when the bridge is enabled
> in SPL.

That's what those functions do, it doesn't answer my question why
they're no longer needed.

> But, i will keep minimal ifdefery on socfpga_bridges_reset to
> indicate the fpga bridges should not be released in SPL.
>>>
>>> I keep them here just for one day we can use if we need to.
>>> Do you remember we have consent to clean up those useless codes for
>>> SPL, all fpga related drivers will be moved into one driver
>>> location.
>> No, sorry.
>>
> Are you disagree on keeping the ifdefery codes, or disagree on moving
> all FPGA related functions into drivers/fpga/... ?

I dislike the ifdeffery.

>>>
>>>>
>>>>>
>>>>>
>>>>> Signed-off-by: Tien Fong Chee <tien.fong.chee at intel.com>
>>>>> ---
>>>>>  arch/arm/mach-socfpga/reset_manager_gen5.c  |    9 +++++++++
>>>>>  arch/arm/mach-socfpga/system_manager_gen5.c |    6 ------
>>>>>  drivers/ddr/altera/sdram.c                  |    8 +++++++-
>>>>>  3 files changed, 16 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/mach-socfpga/reset_manager_gen5.c
>>>>> b/arch/arm/mach-socfpga/reset_manager_gen5.c
>>>>> index aa88adb..c954063 100644
>>>>> --- a/arch/arm/mach-socfpga/reset_manager_gen5.c
>>>>> +++ b/arch/arm/mach-socfpga/reset_manager_gen5.c
>>>>> @@ -97,6 +97,14 @@ void socfpga_bridges_reset(int enable)
>>>>>  		writel(0, &sysmgr_regs->iswgrp_handoff[0]);
>>>>>  		writel(l3mask, &sysmgr_regs-
>>>>>> iswgrp_handoff[1]);
>>>>>  
>>>>> +/*
>>>>> + * FPGA feature is not required in SPL, so FPGA bridges
>>>>> release
>>>>> from reset
>>>>> + * should not be supported in SPL, but some FPGA releated
>>>>> setting
>>>>> can be stored
>>>>> + * in handoff registers as SPL to U-boot/OS handoff
>>>>> information.
>>>>> These
>>>>> + * handoff information can be used in later phase such as
>>>>> feeding
>>>>> handoff
>>>>> + * information to bridge command when user want to enable FPGA
>>>>> feature in U-boot
>>>>> + */
>>>>> +#ifndef CONFIG_SPL_BUILD
>>>>>  		/* Check signal from FPGA. */
>>>>>  		if (!fpgamgr_test_fpga_ready()) {
>>>>>  			/* FPGA not ready, do nothing. We
>>>>> allow
>>>>> system to boot
>>>>> @@ -110,6 +118,7 @@ void socfpga_bridges_reset(int enable)
>>>>>  
>>>>>  		/* Remap the bridges into memory map */
>>>>>  		writel(l3mask, SOCFPGA_L3REGS_ADDRESS);
>>>> I believe the L3REGS needs to be programmed on Gen5.
>>>>
>>> Yes, this is done when calling command "bridge
>>> enable". iswgrp_handoff[1] contains l3mask, which will be written
>>> into SOCFPGA_L3REGS_ADDRESS.
>> I think this needs to be done earlier, otherwise the first 1 MiB or
>> so
>> of DRAM isn't accessible.
>>
> Yeah, you are right....this is what i missing, the OCRAM should be
> remap to lowest memory region 1 MiB. So i propose just to remap OCRAM,
> and FPGA related bridges can be remaped in U-boot by calling the
> "enable bridge" command.

So basically the user would need to run this random command to fix his
obscure memory issues with the first 1 MiB ? Unacceptable, U-Boot does
this and this behavior will be retained. There's no way such a change of
behavior can be let in.

[...]

-- 
Best regards,
Marek Vasut