[U-Boot] [PATCH v2 3/4] arm: socfpga: stratix10: Add Stratix10 FPGA into FPGA device table
Ang, Chee Hong
chee.hong.ang at intel.com
Thu Nov 15 07:13:00 UTC 2018
On Wed, 2018-11-14 at 12:52 +0100, Marek Vasut wrote:
> 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 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 in
> > > > > > > > > > > > tel.
> > > > > > > > > > > > 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 ?
I think I will add a new file arch/arm/mach-socfpga/fpga_devices.c and
put the common code in it so that different platforms can share the
common implementation which override the weak 'socfpga_fpga_add'
function. The new file will have the following code:
#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
*/
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
},
};
#endif
/* 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]);
}
>
> >
> > If you are OK with this implementation, I can submit a new patch
> > for
> > review again. Thanks.
> Submit the patches, yes.
>
More information about the U-Boot
mailing list