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@intel.com >>>> wrote: >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> From: "Ang, Chee Hong" chee.hong.ang@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.