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....@intel.com wrote:
>>>
>>> From: "Ang, Chee Hong" <chee.hong....@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....@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
