[U-Boot] [PATCH v7 3/6] arm: socfpga: Enable FPGA driver on SPL

[Marek Vasut]

On 06/05/2017 02:57 PM, Chee, Tien Fong wrote:
> On Isn, 2017-06-05 at 14:25 +0200, Marek Vasut wrote:
>> On 06/05/2017 02:23 PM, Chee, Tien Fong wrote:
>>>
>>> On Isn, 2017-06-05 at 14:15 +0200, Marek Vasut wrote:
>>>>
>>>> On 06/05/2017 02:11 PM, Chee, Tien Fong wrote:
>>>>>
>>>>>
>>>>> On Isn, 2017-06-05 at 14:08 +0200, Marek Vasut wrote:
>>>>>>
>>>>>>
>>>>>> On 06/05/2017 02:06 PM, Chee, Tien Fong wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Isn, 2017-06-05 at 13:24 +0200, Marek Vasut wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 06/05/2017 06:02 AM, Chee, Tien Fong wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Isn, 2017-06-05 at 11:32 +0800, Chee, Tien Fong
>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>> On Sab, 2017-06-03 at 13:27 +0200, Marek Vasut wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 05/29/2017 06:00 AM, tien.fong.chee at intel.com
>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> From: Tien Fong Chee <tien.fong.chee at intel.com>
>>>>>>>>>>>>
>>>>>>>>>>>> This patch is for enabling the FPGA driver
>>>>>>>>>>>> support on
>>>>>>>>>>>> SPL.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Tien Fong Chee <tien.fong.chee at int
>>>>>>>>>>>> el.c
>>>>>>>>>>>> om>
>>>>>>>>>>>> ---
>>>>>>>>>>>>  include/configs/socfpga_common.h | 1 +
>>>>>>>>>>>>  1 file changed, 1 insertion(+)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/include/configs/socfpga_common.h
>>>>>>>>>>>> b/include/configs/socfpga_common.h
>>>>>>>>>>>> index da7e4ad..e74e7eb 100644
>>>>>>>>>>>> --- a/include/configs/socfpga_common.h
>>>>>>>>>>>> +++ b/include/configs/socfpga_common.h
>>>>>>>>>>>> @@ -110,6 +110,7 @@
>>>>>>>>>>>>  #define CONFIG_FPGA
>>>>>>>>>>>>  #define CONFIG_FPGA_ALTERA
>>>>>>>>>>>>  #define CONFIG_FPGA_SOCFPGA
>>>>>>>>>>>> +#define CONFIG_SPL_FPGA_SUPPORT
>>>>>>>>>>> This should be Kconfig symbol selected in board
>>>>>>>>>>> config.
>>>>>>>>>>> Why
>>>>>>>>>>> is
>>>>>>>>>>> it
>>>>>>>>>>> not
>>>>>>>>>>> Kconfig symbol ?
>>>>>>>>>>>
>>>>>>>>> Ahha....ijust recalled there is a very important reason
>>>>>>>>> i
>>>>>>>>> did
>>>>>>>>> this
>>>>>>>>> because CONFIG_CMD_FPGA which is wrapping all these
>>>>>>>>> FPGA
>>>>>>>>> related
>>>>>>>>> CONFIG
>>>>>>>>> is not part of kconfig.
>>>>>>>> it is, so this is nonsense, try git grep next time ...
>>>>>>>> cmd/Kconfig:config CMD_FPGA
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> During the build, kconfig and Macro #define are
>>>>>>>>> in two different phase/order, so we might having build
>>>>>>>>> issue
>>>>>>>>> during
>>>>>>>>> kconfig build(1st phase) when some Macro #define which
>>>>>>>>> is
>>>>>>>>> 2nd
>>>>>>>>> phase
>>>>>>>>> are
>>>>>>>>> required.
>>>>>>>> I don't understand this, but I think this is bogus.
>>>>>>>>
>>>>>>> Okay. For example, CONFIG_A in Kconfig depend on CONFIG_B
>>>>>>> which
>>>>>>> is
>>>>>>> defined in socfpga_common.h. When SPL build, CONFIG_A would
>>>>>>> not
>>>>>>> be
>>>>>>> set
>>>>>>> to one even CONFIG_B is defined to one in header file,
>>>>>>> because
>>>>>>> at
>>>>>>> that
>>>>>>> moment build, CONFIG_B is not detected, but header file
>>>>>>> only
>>>>>>> get
>>>>>>> build
>>>>>>> after Kconfig build done.
>>>>>> Then convert whatever is missing to Kconfig and it's done ?
>>>>>>
>>>>> Yeah, i will send out immediately after this patch set.
>>>>>
>>>> Please do things in sensible order ...
>>>>
>>> I thought we already reach agreement that specific patch for
>>> converting
>>> all to kconfig would be after this patch set, since it needs more
>>> time
>>> for the tool to execute all the convertion, right?
>> SPL_FPGA_SUPPORT is Kconfig'd already, so do not add it into
>> include/configs/*h . If there is a bug, fix it.
>>
> I don't think that is bug, this is how the build order is designed,
> Kconfig is build 1st, then only compiling the header/C files. So i
> would suggest all related/dependency CONFIG put in one place,either all
> in Kconfig or in header file. 

If we put anything which is already using Kconfig into the header file
again, what's the point of using Kconfig ?

> So, i suggest to submit a seprate patch for converting all to Kconfig
> after this patchset, or i can pull this patch into this patchset.
> 
> If you prefer adding SPL_FPGA_SUPPORT into this patchset, i need to
> rerun the tool for this macro, because a lot defconfigs require FPGA
> feature enabled in SPL build. I also need to run all defconfigs
> compilation and make sure above example is not hit, otherwise FPGA
> driver could be failed in the build.
> 
> What do you think?

I think the SPL_FPGA_SUPPORT is already Kconfig-ified and I don't quite
understand what's the problem you have, sorry. But I am positive any
Kconfig option shouldn't be selected in the config header file.

> Thanks.
> 


-- 
Best regards,
Marek Vasut