[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 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.
> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > 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.