[U-Boot] [PATCH v2 3/4] arm: socfpga: stratix10: Add Stratix10 FPGA into FPGA device table
Marek Vasut
marex at denx.de
Thu Nov 15 13:40:45 UTC 2018
On 11/15/2018 08:13 AM, Ang, Chee Hong wrote:
> On Wed, 2018-11-14 at 12:52 +0100, Marek Vasut wrote:
>> 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 in
>>>>>>>>>>>>> tel.
>>>>>>>>>>>>> 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 ?
> I think I will add a new file arch/arm/mach-socfpga/fpga_devices.c and
> put the common code in it so that different platforms can share the
> common implementation which override the weak 'socfpga_fpga_add'
> function. The new file will have the following code:
>
> #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
> */
> 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
> },
> };
> #endif
>
> /* 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]);
> }
>
Better submit the whole patch, it's hard to review pieces.
--
Best regards,
Marek Vasut
More information about the U-Boot
mailing list