From: Ang, Chee Hong <chee.hong.ang@intel.com>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH v2 3/4] arm: socfpga: stratix10: Add Stratix10 FPGA into FPGA device table
Date: Thu, 15 Nov 2018 07:13:00 +0000 [thread overview]
Message-ID: <1542265979.62436.20.camel@intel.com> (raw)
In-Reply-To: <a7b50837-1ed0-0363-c19c-7db2fe450be1@denx.de>
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@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@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.
>
next prev parent reply other threads:[~2018-11-15 7:13 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-08 9:48 [U-Boot] [PATCH v2 0/4] Stratix10 FPGA reconfiguration support chee.hong.ang at intel.com
2018-10-08 9:48 ` [U-Boot] [PATCH v2 1/4] arm: socfpga: stratix10: Add macros for mailbox's arguments chee.hong.ang at intel.com
2018-10-08 9:48 ` [U-Boot] [PATCH v2 2/4] arm: socfpga: stratix10: Add Stratix 10 FPGA Reconfiguration Driver chee.hong.ang at intel.com
2018-10-08 9:48 ` [U-Boot] [PATCH v2 3/4] arm: socfpga: stratix10: Add Stratix10 FPGA into FPGA device table chee.hong.ang at intel.com
2018-10-08 9:57 ` Marek Vasut
2018-10-08 15:10 ` Ang, Chee Hong
2018-10-08 20:32 ` Marek Vasut
2018-10-09 3:03 ` Ang, Chee Hong
2018-10-09 12:48 ` Marek Vasut
2018-10-10 5:30 ` Ang, Chee Hong
2018-10-10 10:27 ` Marek Vasut
2018-10-11 6:21 ` Ang, Chee Hong
2018-10-11 10:03 ` Marek Vasut
2018-11-14 7:09 ` Ang, Chee Hong
2018-11-14 11:52 ` Marek Vasut
2018-11-15 7:13 ` Ang, Chee Hong [this message]
2018-11-15 13:40 ` Marek Vasut
2018-10-08 9:48 ` [U-Boot] [PATCH v2 4/4] arm: socfpga: stratix10: Enable Stratix10 FPGA Reconfiguration chee.hong.ang at intel.com
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1542265979.62436.20.camel@intel.com \
--to=chee.hong.ang@intel.com \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.