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

Thu Oct 11 10:03:37 UTC 2018

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.

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

Thanks

-- 
Best regards,
Marek Vasut