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

[Marek Vasut]

On 06/06/2017 05:46 AM, Chee, Tien Fong wrote:
> On Isn, 2017-06-05 at 15:00 +0200, Marek Vasut wrote:
>> 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.co
>>>>>>>>>>>>> m
>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> From: Tien Fong Chee <tien.fong.chee at intel.co
>>>>>>>>>>>>>> m>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This patch is for enabling the FPGA driver
>>>>>>>>>>>>>> support on
>>>>>>>>>>>>>> SPL.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Signed-off-by: Tien Fong Chee <tien.fong.chee
>>>>>>>>>>>>>> @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.
>>
> How about i just pull the patch convert all to Kconfig into this patch
> set, and this will replace the patch 3. I just recalled why i need to
> to enable the SPL FPGA build on patch 4 because patch 5 FPGA manager
> already being moved to drivers/fpga/ hence some functions from FPGA
> manager are required in SPL phase. In this arrangement, we can avoid
> duplicate effort and time spending since we already have that Kconfig
> patch.

I'll check the patches.

> Are you OK?

I'm fine, thank you for your concern.

-- 
Best regards,
Marek Vasut