[U-Boot] [PATCH v7 3/6] arm: socfpga: Enable FPGA driver on SPL

Tue Jun 6 03:46:13 UTC 2017

On Isn, 2017-06-05 at 15:00 +0200, Marek Vasut wrote:
> On 06/05/2017 02:57 PM, Chee, Tien Fong wrote:
> > 
> > On Isn, 2017-06-05 at 14:25 +0200, Marek Vasut wrote:
> > > 
> > > On 06/05/2017 02:23 PM, Chee, Tien Fong wrote:
> > > > 
> > > > 
> > > > On Isn, 2017-06-05 at 14:15 +0200, Marek Vasut wrote:
> > > > > 
> > > > > 
> > > > > On 06/05/2017 02:11 PM, Chee, Tien Fong wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On Isn, 2017-06-05 at 14:08 +0200, Marek Vasut wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On 06/05/2017 02:06 PM, Chee, Tien Fong wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > On Isn, 2017-06-05 at 13:24 +0200, Marek Vasut wrote:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > On 06/05/2017 06:02 AM, Chee, Tien Fong wrote:
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > On Isn, 2017-06-05 at 11:32 +0800, Chee, Tien Fong
> > > > > > > > > > wrote:
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > > 
> > > > > > > > > > > > > 
> > > > > > > > > > > > > 
> > > > > > > > > > > > > 
> > > > > > > > > > > > > 
> > > > > > > > > > > > > 
> > > > > > > > > > > On Sab, 2017-06-03 at 13:27 +0200, Marek Vasut
> > > > > > > > > > > wrote:
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > On 05/29/2017 06:00 AM, tien.fong.chee at intel.co
> > > > > > > > > > > > m
> > > > > > > > > > > > wrote:
> > > > > > > > > > > > > 
> > > > > > > > > > > > > 
> > > > > > > > > > > > > 
> > > > > > > > > > > > > 
> > > > > > > > > > > > > 
> > > > > > > > > > > > > 
> > > > > > > > > > > > > 
> > > > > > > > > > > > > From: Tien Fong Chee <tien.fong.chee at intel.co
> > > > > > > > > > > > > m>
> > > > > > > > > > > > > 
> > > > > > > > > > > > > This patch is for enabling the FPGA driver
> > > > > > > > > > > > > support on
> > > > > > > > > > > > > SPL.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Signed-off-by: Tien Fong Chee <tien.fong.chee
> > > > > > > > > > > > > @int
> > > > > > > > > > > > > el.c
> > > > > > > > > > > > > om>
> > > > > > > > > > > > > ---
> > > > > > > > > > > > >  include/configs/socfpga_common.h | 1 +
> > > > > > > > > > > > >  1 file changed, 1 insertion(+)
> > > > > > > > > > > > > 
> > > > > > > > > > > > > diff --git a/include/configs/socfpga_common.h
> > > > > > > > > > > > > b/include/configs/socfpga_common.h
> > > > > > > > > > > > > index da7e4ad..e74e7eb 100644
> > > > > > > > > > > > > --- a/include/configs/socfpga_common.h
> > > > > > > > > > > > > +++ b/include/configs/socfpga_common.h
> > > > > > > > > > > > > @@ -110,6 +110,7 @@
> > > > > > > > > > > > >  #define CONFIG_FPGA
> > > > > > > > > > > > >  #define CONFIG_FPGA_ALTERA
> > > > > > > > > > > > >  #define CONFIG_FPGA_SOCFPGA
> > > > > > > > > > > > > +#define CONFIG_SPL_FPGA_SUPPORT
> > > > > > > > > > > > This should be Kconfig symbol selected in board
> > > > > > > > > > > > config.
> > > > > > > > > > > > Why
> > > > > > > > > > > > is
> > > > > > > > > > > > it
> > > > > > > > > > > > not
> > > > > > > > > > > > Kconfig symbol ?
> > > > > > > > > > > > 
> > > > > > > > > > Ahha....ijust recalled there is a very important
> > > > > > > > > > reason
> > > > > > > > > > i
> > > > > > > > > > did
> > > > > > > > > > this
> > > > > > > > > > because CONFIG_CMD_FPGA which is wrapping all these
> > > > > > > > > > FPGA
> > > > > > > > > > related
> > > > > > > > > > CONFIG
> > > > > > > > > > is not part of kconfig.
> > > > > > > > > it is, so this is nonsense, try git grep next time
> > > > > > > > > ...
> > > > > > > > > cmd/Kconfig:config CMD_FPGA
> > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > During the build, kconfig and Macro #define are
> > > > > > > > > > in two different phase/order, so we might having
> > > > > > > > > > build
> > > > > > > > > > issue
> > > > > > > > > > during
> > > > > > > > > > kconfig build(1st phase) when some Macro #define
> > > > > > > > > > which
> > > > > > > > > > is
> > > > > > > > > > 2nd
> > > > > > > > > > phase
> > > > > > > > > > are
> > > > > > > > > > required.
> > > > > > > > > I don't understand this, but I think this is bogus.
> > > > > > > > > 
> > > > > > > > Okay. For example, CONFIG_A in Kconfig depend on
> > > > > > > > CONFIG_B
> > > > > > > > which
> > > > > > > > is
> > > > > > > > defined in socfpga_common.h. When SPL build, CONFIG_A
> > > > > > > > would
> > > > > > > > not
> > > > > > > > be
> > > > > > > > set
> > > > > > > > to one even CONFIG_B is defined to one in header file,
> > > > > > > > because
> > > > > > > > at
> > > > > > > > that
> > > > > > > > moment build, CONFIG_B is not detected, but header file
> > > > > > > > only
> > > > > > > > get
> > > > > > > > build
> > > > > > > > after Kconfig build done.
> > > > > > > Then convert whatever is missing to Kconfig and it's done
> > > > > > > ?
> > > > > > > 
> > > > > > Yeah, i will send out immediately after this patch set.
> > > > > > 
> > > > > Please do things in sensible order ...
> > > > > 
> > > > I thought we already reach agreement that specific patch for
> > > > converting
> > > > all to kconfig would be after this patch set, since it needs
> > > > more
> > > > time
> > > > for the tool to execute all the convertion, right?
> > > SPL_FPGA_SUPPORT is Kconfig'd already, so do not add it into
> > > include/configs/*h . If there is a bug, fix it.
> > > 
> > I don't think that is bug, this is how the build order is designed,
> > Kconfig is build 1st, then only compiling the header/C files. So i
> > would suggest all related/dependency CONFIG put in one place,either
> > all
> > in Kconfig or in header file. 
> If we put anything which is already using Kconfig into the header
> file
> again, what's the point of using Kconfig ?
> 
> > 
> > So, i suggest to submit a seprate patch for converting all to
> > Kconfig
> > after this patchset, or i can pull this patch into this patchset.
> > 
> > If you prefer adding SPL_FPGA_SUPPORT into this patchset, i need to
> > rerun the tool for this macro, because a lot defconfigs require
> > FPGA
> > feature enabled in SPL build. I also need to run all defconfigs
> > compilation and make sure above example is not hit, otherwise FPGA
> > driver could be failed in the build.
> > 
> > What do you think?
> I think the SPL_FPGA_SUPPORT is already Kconfig-ified and I don't
> quite
> understand what's the problem you have, sorry. But I am positive any
> Kconfig option shouldn't be selected in the config header file.
> 
How about i just pull the patch convert all to Kconfig into this patch
set, and this will replace the patch 3. I just recalled why i need to
to enable the SPL FPGA build on patch 4 because patch 5 FPGA manager
already being moved to drivers/fpga/ hence some functions from FPGA
manager are required in SPL phase. In this arrangement, we can avoid
duplicate effort and time spending since we already have that Kconfig
patch.

Are you OK?
> >
> > Thanks.
> > 
>