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

[Marek Vasut]

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 ?

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 ...

-- 
Best regards,
Marek Vasut