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

Wed Nov 14 11:52:22 UTC 2018

On 11/14/2018 08:09 AM, Ang, Chee Hong wrote:
> On Thu, 2018-10-11 at 10:03 +0000, Marek Vasut wrote:
>> On 10/11/2018 08:21 AM, Ang, Chee Hong wrote:
>>>
>>> On Wed, 2018-10-10 at 12:27 +0200, Marek Vasut wrote:
>>>>
>>>> On 10/10/2018 07:30 AM, Ang, Chee Hong wrote:
>>>>>
>>>>>
>>>>> On Tue, 2018-10-09 at 14:48 +0200, Marek Vasut wrote:
>>>>>>
>>>>>>
>>>>>> 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 ^.
>>>>> Yes. It's being addressed in v3 patch:
>>>>> https://lists.denx.de/pipermail/u-boot/2018-October/343561.html
>>>> So where did the function go in there ? I don't see any __weak
>>>> anything.
>>> I don't have to add anything in my v3 patchsets to make this work.
>>> It's already taken care by arch/arm/mach-
>>> socfpga/include/mach/misc.h as
>>> shown below:
>>>
>>> #ifdef CONFIG_FPGA
>>> void socfpga_fpga_add(void);
>>> #else
>>> static inline void socfpga_fpga_add(void) {}
>>> #endif
>>>
>>> An empty default socfpga_fpga_add() will be defined if CONFIG_FPGA
>>> is
>>> not defined.
>> I was hoping to turn this into __weak function.
> 
> Below are the new changes for new patch:
> Empty weak function in arch/arm/mach-socfpga/misc.c:
> 
> /* add device descriptor to FPGA device table */
> __weak void socfpga_fpga_add(void)
> {
> }
> 
> 
> In arch/arm/mach-socfpga/misc_aria10.c and arch/arm/mach-
> socfpga/misc_gen5.c:
> 
> #ifdef CONFIG_FPGA
> /*
>  * FPGA programming support for SoC FPGA Cyclone V
>  */
> static Altera_desc altera_fpga[] = {
> 	{
> 		/* Family */
> 		Altera_SoCFPGA,
> 		/* Interface type */
> 		fast_passive_parallel,
> 		/* 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
> 	},
> };
> 
> /* add device descriptor to FPGA device table */
> void socfpga_fpga_add(void)
> {
> 	int i;
> 	fpga_init();
> 	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
> 		fpga_add(fpga_altera, &altera_fpga[i]);
> }
> #endif
> 
> 
> In arch/arm/mach-socfpga/misc_s10.c:
> 
> #ifdef CONFIG_FPGA
> /*
>  * 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
> 	},
> };
> 
> /* add device descriptor to FPGA device table */
> void socfpga_fpga_add(void)
> {
> 	int i;
> 	fpga_init();
> 	for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
> 		fpga_add(fpga_altera, &altera_fpga[i]);
> }
> #endif
> 
> With this new implementation, each platform overrides the
> 'socfpga_fpga_add' to add its own fpga device. The problem here is
> since our aria10 and gen5 are adding same fpga device, there will be
> duplication of code for these 2 platforms.
> What do you think ?

I think you can create a common code for Gen5 somehow, right ?

> If you are OK with this implementation, I can submit a new patch for
> review again. Thanks.

Submit the patches, yes.

-- 
Best regards,
Marek Vasut