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....@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.ang@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
