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

Wed Oct 10 05:30:58 UTC 2018

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 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 I don't think I need to make any changes to socfpga_fpga_add()
> > in
> > misc.c. I just have to remove ifdef CONFIG_FPGA in misc_s10.c
> > because
> > it was unnecessary. I will submit v3 for this patch and you can
> > comment
> > further. The v3 patch will be simpler. Thanks.
> Please don't submit stuff before the discussion concluded, it's
> pointless.
OK.
> 
> > 
> > > 
> > > 
> > > 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.

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.

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.