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

[Ang, Chee Hong]

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

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.

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