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

[Ang, Chee Hong <chee.hong.ang@intel.com>]

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@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 ?
If you are OK with this implementation, I can submit a new patch for
review again. Thanks.
> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > 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