[U-Boot] [PATCH v2 3/4] arm: socfpga: stratix10: Add Stratix10 FPGA into FPGA device table

[Marek Vasut]

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.ang at intel.com wrote:
>>>>>
>>>>>
>>>>> From: "Ang, Chee Hong" <chee.hong.ang at 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.ang at 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