On 10/09/2018 05:03 AM, Ang, Chee Hong wrote:
> On Mon, 2018-10-08 at 22:32 +0200, Marek Vasut wrote:
>> On 10/08/2018 05:10 PM, Ang, Chee Hong wrote:
>>>
>>> On Mon, 2018-10-08 at 11:57 +0200, Marek Vasut wrote:
>>>>
>>>> On 10/08/2018 11:48 AM, chee.hong....@intel.com wrote:
>>>>>
>>>>>
>>>>> From: "Ang, Chee Hong" <chee.hong....@intel.com>
>>>>>
>>>>> Enable 'fpga' command in u-boot. User will be able to use the
>>>>> fpga
>>>>> command to program the FPGA on Stratix10 SoC.
>>>>>
>>>>> Signed-off-by: Ang, Chee Hong <chee.hong....@intel.com>
>>>>> ---
>>>>> arch/arm/mach-socfpga/misc.c | 29
>>>>> +++++++++++++++++++++++++++++
>>>>> arch/arm/mach-socfpga/misc_s10.c | 2 ++
>>>>> drivers/fpga/altera.c | 6 ++++++
>>>>> include/altera.h | 4 ++++
>>>>> 4 files changed, 41 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-
>>>>> socfpga/misc.c
>>>>> index a4f6d5c..7986b58 100644
>>>>> --- a/arch/arm/mach-socfpga/misc.c
>>>>> +++ b/arch/arm/mach-socfpga/misc.c
>>>>> @@ -88,6 +88,27 @@ int overwrite_console(void)
>>>>> #endif
>>>>>
>>>>> #ifdef CONFIG_FPGA
>>>>> +#ifdef CONFIG_FPGA_STRATIX10
>>>>> +/*
>>>>> + * FPGA programming support for SoC FPGA Stratix 10
>>>>> + */
>>>>> +static Altera_desc altera_fpga[] = {
>>>>> + {
>>>>> + /* Family */
>>>>> + Intel_FPGA_Stratix10,
>>>>> + /* Interface type */
>>>>> + secure_device_manager_mailbox,
>>>>> + /* No limitation as additional data will be
>>>>> ignored */
>>>>> + -1,
>>>>> + /* No device function table */
>>>>> + NULL,
>>>>> + /* Base interface address specified in driver
>>>>> */
>>>>> + NULL,
>>>>> + /* No cookie implementation */
>>>>> + 0
>>>>> + },
>>>>> +};
>>>>> +#else
>>>>> /*
>>>>> * FPGA programming support for SoC FPGA Cyclone V
>>>>> */
>>>>> @@ -107,6 +128,7 @@ static Altera_desc altera_fpga[] = {
>>>>> 0
>>>>> },
>>>>> };
>>>>> +#endif
>>>>>
>>>>> /* add device descriptor to FPGA device table */
>>>>> void socfpga_fpga_add(void)
>>>>> @@ -116,6 +138,13 @@ void socfpga_fpga_add(void)
>>>>> for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
>>>>> fpga_add(fpga_altera, &altera_fpga[i]);
>>>>> }
>>>>> +
>>>>> +#else
>>>>> +
>>>>> +__weak void socfpga_fpga_add(void)
>>>>> +{
>>>>> +}
>>>> Why is a __weak function defined only in else-statement ?
>>>>
>>>> It should be defined always, with a sane default implementation.
>>> I will remove the empty function in #else-statement and define the
>>> default function like this :
>>>
>>> /* add device descriptor to FPGA device table */
>>> void socfpga_fpga_add(void)
>>> {
>>> #ifdef CONFIG_FPGA
>>> int i;
>>> fpga_init();
>>> for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
>>> fpga_add(fpga_altera, &altera_fpga[i]);
>>> #endif
>>> }
>>>
>>> Is that OK?
>> Can't you have __weak empty implementation of socfpga_fpga_add() and
>> implement a version per platform ? Would that work and make sense ?
> socfpga_fpga_add() as shown above is a generic function for adding FPGA
> devices to FPGA driver which applies to all our platforms. This is the
> reason why it is defined in misc.c instead of misc_<platform_name>.c.
>
> It turned out we already have this defined in misc.h:
> #ifdef CONFIG_FPGA
> void socfpga_fpga_add(void);
> #else
> static inline void socfpga_fpga_add(void) {}
> #endif
Right, if you had one socfpga_fpga_add() per platform + generic empty
one, you could drop that whole thing ^.
> So I don't think I need to make any changes to socfpga_fpga_add() in
> misc.c. I just have to remove ifdef CONFIG_FPGA in misc_s10.c because
> it was unnecessary. I will submit v3 for this patch and you can comment
> further. The v3 patch will be simpler. Thanks.
Please don't submit stuff before the discussion concluded, it's pointless.
>>
>> btw. the best solution would be to fix this proper and implement a
>> DM/DT
>> based probing of the FPGA, including a proper driver(s) in
>> drivers/fpga/
>> instead of putting all the crud into arch/arm/mach-socfpga ...
What do you think about this ^
--
Best regards,
Marek Vasut
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
