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

[Marek Vasut]

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.

>>>> 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 ^
>>
> I do agree DM/DT is the proper way to implement driver.
> But right now those FPGA drivers in drivers/fpga need to be at least
> call fpga_add() to add themselves into FPGA device table so that their
> callback functions can be invoked correctly when user issue 'fpga
> load', 'fpga info' at the command prompt.
> So in other words, all drivers in drivers/fpga rely on
> drivers/fpga/fpga.c (FPGA core driver) to work.

Well, that should be fixed so that they probe from DT, just like any
other driver. I'm not fond of adding stuff to arch/arm/ ...

> We already have all our fpga drivers in drivers/fpga :
> drivers/fpga/stratix10.c (NEW. In this patchset)
> drivers/fpga/stratixII.c (upstreamed)
> drivers/fpga/strixv.c (upstreamed)
> drivers/fpga/cyclon2.c (upstreamed)
> and others...
> 
> We only define the FPGA device structure in arch/arm/mach-
> socfpga/misc.c and call fpga_add() to add our FPGA device driver into
> the global FPGA device table then FPGA core driver will handle the FPGA
> operations by invoking the FPGA driver's callback functions.

Right, which should be moved to drivers too and which should use DT.

> So for proper DM/DT implementation, drivers/fpga/fpga.c need to be
> changed as well because this is the core of the FPGA driver.I think
> changing the core of the FPGA driver to support DM/DT would make more
> sense than I only change my FPGA driver to extract info from DTB file
> into a device structure then specifically call fpga_add() again to add
> the device structure to the FPGA core driver.

Yes, can you add it to your list once we flesh out this patchset ?

-- 
Best regards,
Marek Vasut