Re: [U-Boot] [PATCH v2 09/19] arm: socfpga: Add drivers for programing FPGA from flash

[Simon Glass]

Hi,

On 25 September 2017 at 02:40,   wrote:
> From: Tien Fong Chee 
>
> These drivers handle FPGA program operation from flash loading
> RBF to memory and then to program FPGA.
>
> Signed-off-by: Tien Fong Chee 
> ---
>  .../include/mach/fpga_manager_arria10.h|  27 ++
>  drivers/fpga/socfpga_arria10.c | 391 
> -
>  include/altera.h   |   6 +
>  include/configs/socfpga_common.h   |   4 +
>  4 files changed, 425 insertions(+), 3 deletions(-)

Agreed on the generic loader, but also, can you please use driver
model? Am I missing something?

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 09/19] arm: socfpga: Add drivers for programing FPGA from flash

[Marek Vasut]

On 09/28/2017 05:14 PM, Chee, Tien Fong wrote:
> On Rab, 2017-09-27 at 11:23 +0200, Marek Vasut wrote:
>> On 09/27/2017 11:13 AM, Chee, Tien Fong wrote:
>>>
>>> On Sel, 2017-09-26 at 12:39 +0200, Marek Vasut wrote:

 On 09/26/2017 11:52 AM, Chee, Tien Fong wrote:
>
>
> On Isn, 2017-09-25 at 11:14 +0200, Marek Vasut wrote:
>>
>>
>> On 09/25/2017 10:40 AM, tien.fong.c...@intel.com wrote:
>>>
>>>
>>>
>>> From: Tien Fong Chee 
>>>
>>> These drivers handle FPGA program operation from flash
>>> loading
>>> RBF to memory and then to program FPGA.
>>>
>>> Signed-off-by: Tien Fong Chee 
 [...]

>
>
>>
>>
>>>
>>>
>>> +const char *get_cff_devpart(const void *fdt, int *len)
>>> +{
>>> +   const char *cff_devpart = NULL;
>>> +   const char *cell;
>>> +   int nodeoffset;
>>> +   nodeoffset = fdtdec_next_compatible(fdt, 0,
>>> +    COMPAT_ALTERA_SOCFPGA_FPGA0);
>>> +
>>> +   cell = fdt_getprop(fdt, nodeoffset,
>>> "bitstream_devpart",
>>> len);
>>> +
>>> +   if (cell)
>>> +   cff_devpart = cell;
>>> +
>>> +   return cff_devpart;
>>> +}
>> Take a look at splash*.c , I believe that can be reworked
>> into
>> generic
>> firmware loader , which you could then use here.
>>
> the devpart is hard coded in splash*.c. The function here is
> getting
> devpart info from DTS. So, is there any similar function in
> splash*.c?
> May be you can share more about your idea.
 The generic loader could use some work of course ...

>>> Sorry, i am still confusing. Allow me to ask you more:
>>> 1. Is the generic firmware loader already exists in splash*.c?
>> No
>>
>>>
>>> 2. Are you talking about get_cff_devpart or whole fpga laodfs?
>>> 3. You want me integrate get_cff_devpart function into splash*.c?
>>> 4. Are you means to hard code the devpart instead providing dynamic
>>> devpart described in DTS?
>> I am talking about factoring out generic firmware loader from
>> splash*c ,
>> since it already contains most of the parts for such a thing.
>>
>>>
>>> Current implementation are located in spl_board_init func
>>> (arcg/arm/mach-socfpga/spl.c). Based on boot device such as mmc,
>>> nand
>>> and QSPI, then reading some info from DTS, setting dev and
>>> partition
>>> with generic fs functions, and reading with generic fs function
>>> before
>>> programming RBF into FPGA. All these are in patch 19.
>> That's what splash*c also does, so adding separate parallel
>> implementation of the same functionality is a no-no.
>>
> After reading through splash*c, i found there are two functions bear a
> close similarity to.
> 1st function -->
> In /common/splash.c : 
> static struct splash_location default_splash_locations[] = {
>   {
>   .name = "sf",
>   .storage = SPLASH_STORAGE_SF,
>   .flags = SPLASH_STORAGE_RAW,
>   .offset = 0x0,
>   },
>   {
>   .name = "mmc_fs",
>   .storage = SPLASH_STORAGE_MMC,
>   .flags = SPLASH_STORAGE_FS,
>   .devpart = "0:1",
>   },
>   {
>   .name = "usb_fs",
>   .storage = SPLASH_STORAGE_USB,
>   .flags = SPLASH_STORAGE_FS,
>   .devpart = "0:1",
>   },
>   {
>   .name = "sata_fs",
>   .storage = SPLASH_STORAGE_SATA,
>   .flags = SPLASH_STORAGE_FS,
>   .devpart = "0:1",
>   },
> };
> 
> In my /arch/arm/mach-socfpga/spl.c (spl_board_init(void))
> bootdev.boot_device = spl_boot_device();
> 
>   if (BOOT_DEVICE_MMC1 == bootdev.boot_device) {
>   struct mmc *mmc = NULL;
>   int err = 0;
> 
>   spl_mmc_find_device(, bootdev.boot_device);
> 
>   err = mmc_init(mmc);
> 
>   if (err) {
> #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
>   printf("spl: mmc init failed with error: %d\n",
> err);
> #endif
>   }
> 
>   fpga_fsinfo.dev_part = (char *)get_cff_devpart(gd-
>> fdt_blob,
>    );
> 
>   fpga_fsinfo.filename = (char *)get_cff_filename(gd-
>> fdt_blob,
>    ,
>   PERIPH_
> RBF);
> 
>   fpga_fsinfo.interface = "mmc";
> 
>   fpga_fsinfo.fstype = FS_TYPE_FAT;
>   } else {
>   printf("Invalid boot device!\n");
>   return;
>   }
> 
>   /* Program peripheral RBF */
>   if (fpga_fsinfo.filename && fpga_fsinfo.dev_part && (len > 0))
>   rval = fpga_fsload(0, buffer, BSIZE, _fsinfo);
> 
> In /common/splash.c, dev_Part and flash 

Re: [U-Boot] [PATCH v2 09/19] arm: socfpga: Add drivers for programing FPGA from flash

[Chee, Tien Fong]

On Rab, 2017-09-27 at 11:23 +0200, Marek Vasut wrote:
> On 09/27/2017 11:13 AM, Chee, Tien Fong wrote:
> > 
> > On Sel, 2017-09-26 at 12:39 +0200, Marek Vasut wrote:
> > > 
> > > On 09/26/2017 11:52 AM, Chee, Tien Fong wrote:
> > > > 
> > > > 
> > > > On Isn, 2017-09-25 at 11:14 +0200, Marek Vasut wrote:
> > > > > 
> > > > > 
> > > > > On 09/25/2017 10:40 AM, tien.fong.c...@intel.com wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > From: Tien Fong Chee 
> > > > > > 
> > > > > > These drivers handle FPGA program operation from flash
> > > > > > loading
> > > > > > RBF to memory and then to program FPGA.
> > > > > > 
> > > > > > Signed-off-by: Tien Fong Chee 
> > > [...]
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > +const char *get_cff_devpart(const void *fdt, int *len)
> > > > > > +{
> > > > > > +   const char *cff_devpart = NULL;
> > > > > > +   const char *cell;
> > > > > > +   int nodeoffset;
> > > > > > +   nodeoffset = fdtdec_next_compatible(fdt, 0,
> > > > > > +    COMPAT_ALTERA_SOCFPGA_FPGA0);
> > > > > > +
> > > > > > +   cell = fdt_getprop(fdt, nodeoffset,
> > > > > > "bitstream_devpart",
> > > > > > len);
> > > > > > +
> > > > > > +   if (cell)
> > > > > > +   cff_devpart = cell;
> > > > > > +
> > > > > > +   return cff_devpart;
> > > > > > +}
> > > > > Take a look at splash*.c , I believe that can be reworked
> > > > > into
> > > > > generic
> > > > > firmware loader , which you could then use here.
> > > > > 
> > > > the devpart is hard coded in splash*.c. The function here is
> > > > getting
> > > > devpart info from DTS. So, is there any similar function in
> > > > splash*.c?
> > > > May be you can share more about your idea.
> > > The generic loader could use some work of course ...
> > > 
> > Sorry, i am still confusing. Allow me to ask you more:
> > 1. Is the generic firmware loader already exists in splash*.c?
> No
> 
> > 
> > 2. Are you talking about get_cff_devpart or whole fpga laodfs?
> > 3. You want me integrate get_cff_devpart function into splash*.c?
> > 4. Are you means to hard code the devpart instead providing dynamic
> > devpart described in DTS?
> I am talking about factoring out generic firmware loader from
> splash*c ,
> since it already contains most of the parts for such a thing.
> 
> > 
> > Current implementation are located in spl_board_init func
> > (arcg/arm/mach-socfpga/spl.c). Based on boot device such as mmc,
> > nand
> > and QSPI, then reading some info from DTS, setting dev and
> > partition
> > with generic fs functions, and reading with generic fs function
> > before
> > programming RBF into FPGA. All these are in patch 19.
> That's what splash*c also does, so adding separate parallel
> implementation of the same functionality is a no-no.
> 
After reading through splash*c, i found there are two functions bear a
close similarity to.
1st function -->
In /common/splash.c : 
static struct splash_location default_splash_locations[] = {
{
.name = "sf",
.storage = SPLASH_STORAGE_SF,
.flags = SPLASH_STORAGE_RAW,
.offset = 0x0,
},
{
.name = "mmc_fs",
.storage = SPLASH_STORAGE_MMC,
.flags = SPLASH_STORAGE_FS,
.devpart = "0:1",
},
{
.name = "usb_fs",
.storage = SPLASH_STORAGE_USB,
.flags = SPLASH_STORAGE_FS,
.devpart = "0:1",
},
{
.name = "sata_fs",
.storage = SPLASH_STORAGE_SATA,
.flags = SPLASH_STORAGE_FS,
.devpart = "0:1",
},
};

In my /arch/arm/mach-socfpga/spl.c (spl_board_init(void))
bootdev.boot_device = spl_boot_device();

if (BOOT_DEVICE_MMC1 == bootdev.boot_device) {
struct mmc *mmc = NULL;
int err = 0;

spl_mmc_find_device(, bootdev.boot_device);

err = mmc_init(mmc);

if (err) {
#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
printf("spl: mmc init failed with error: %d\n",
err);
#endif
}

fpga_fsinfo.dev_part = (char *)get_cff_devpart(gd-
>fdt_blob,
 );

fpga_fsinfo.filename = (char *)get_cff_filename(gd-
>fdt_blob,
 ,
PERIPH_
RBF);

fpga_fsinfo.interface = "mmc";

fpga_fsinfo.fstype = FS_TYPE_FAT;
} else {
printf("Invalid boot device!\n");
return;
}

/* Program peripheral RBF */
if (fpga_fsinfo.filename && fpga_fsinfo.dev_part && (len > 0))
rval = fpga_fsload(0, buffer, BSIZE, 

Re: [U-Boot] [PATCH v2 09/19] arm: socfpga: Add drivers for programing FPGA from flash

[Chee, Tien Fong]

On Rab, 2017-09-27 at 10:30 +0200, Marek Vasut wrote:
> On 09/27/2017 08:05 AM, Chee, Tien Fong wrote:
> > 
> > On Sel, 2017-09-26 at 12:32 +0200, Marek Vasut wrote:
> > > 
> > > On 09/26/2017 10:30 AM, Chee, Tien Fong wrote:
> > > > 
> > > > 
> > > > On Isn, 2017-09-25 at 11:14 +0200, Marek Vasut wrote:
> > > > > 
> > > > > 
> > > > > On 09/25/2017 10:40 AM, tien.fong.c...@intel.com wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > From: Tien Fong Chee 
> > > > > > 
> > > > > > These drivers handle FPGA program operation from flash
> > > > > > loading
> > > > > > RBF to memory and then to program FPGA.
> > > > > > 
> > > > > > Signed-off-by: Tien Fong Chee 
> > > > > Did you run checkpatch on this before submitting ? I presume
> > > > > no
> > > > > ...
> > > > > 
> > > > Yeah, i run checkpatch for all patches. What's the issue here?
> > > It should definitely indicate problem with ie. yoda-notation
> > > +if (0 == flashinfo->remaining) {
> > > and indent ...
> > > 
> > No complaint from checkpath. I know someone saying bad for
> > readbility,
> > but yoda-notation at this simple implementation doesn't impact the
> > readbility, and having benefit to leverage detection of compiler on
> > missing "=". Overall, this can help to improve coding quality. I
> > can
> > remove it if this doesn't favored in U-boot.
> It is not welcome and modern gcc warns you about such things .
> 
Okay.
> > 
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > ---
> > > > > >  .../include/mach/fpga_manager_arria10.h|  27
> > > > > > ++
> > > > > >  drivers/fpga/socfpga_arria10.c | 391
> > > > > > -
> > > > > >  include/altera.h   |   6 +
> > > > > >  include/configs/socfpga_common.h   |   4 +
> > > > > >  4 files changed, 425 insertions(+), 3 deletions(-)
> > > > > [...]
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > @@ -112,13 +122,14 @@ static int
> > > > > > wait_for_nconfig_pin_and_nstatus_pin(void)
> > > > > >     unsigned long mask =
> > > > > > ALT_FPGAMGR_IMGCFG_STAT_F2S_NCONFIG_PIN_SET_MSK |
> > > > > >     ALT_FPGAMGR_IMGCFG_STAT_F2
> > > > > > S_NS
> > > > > > TATU
> > > > > > S_PIN_SET_MSK;
> > > > > >  
> > > > > > -   /* Poll until f2s_nconfig_pin and f2s_nstatus_pin;
> > > > > > loop
> > > > > > until de-asserted,
> > > > > > -    * timeout at 1000ms
> > > > > > +   /*
> > > > > > +    * Poll until f2s_nconfig_pin and f2s_nstatus_pin;
> > > > > > loop
> > > > > > until
> > > > > > +    * de-asserted, timeout at 1000ms
> > > > > >      */
> > > > > >     return wait_for_bit(__func__,
> > > > > >     _manager_base-
> > > > > > >imgcfg_stat,
> > > > > >     mask,
> > > > > > -   false, FPGA_TIMEOUT_MSEC,
> > > > > > false);
> > > > > > +   true, FPGA_TIMEOUT_MSEC,
> > > > > > false);
> > > > > >  }
> > > > > Seems more like a fix, split this out.
> > > > > 
> > > > Okay.
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > >  static int wait_for_f2s_nstatus_pin(unsigned long value)
> > > > > > @@ -469,6 +480,7 @@ int socfpga_load(Altera_desc *desc,
> > > > > > const
> > > > > > void
> > > > > > *rbf_data, size_t rbf_size)
> > > > > >  
> > > > > >     /* Initialize the FPGA Manager */
> > > > > >     status = fpgamgr_program_init((u32 *)rbf_data,
> > > > > > rbf_size);
> > > > > > +
> > > > > >     if (status)
> > > > > >     return status;
> > > > > >  
> > > > > > @@ -477,3 +489,376 @@ int socfpga_load(Altera_desc *desc,
> > > > > > const
> > > > > > void *rbf_data, size_t rbf_size)
> > > > > >  
> > > > > >     return fpgamgr_program_finish();
> > > > > >  }
> > > > > > +
> > > > > > +#if defined(CONFIG_CMD_FPGA_LOADFS)
> > > > > > +const char *get_cff_filename(const void *fdt, int *len,
> > > > > > u32
> > > > > > core)
> > > > > > +{
> > > > > > +   const char *cff_filename = NULL;
> > > > > > +   const char *cell;
> > > > > > +   int nodeoffset;
> > > > > > +   nodeoffset = fdtdec_next_compatible(fdt, 0,
> > > > > > +    COMPAT_ALTERA_SOCFPGA_FPGA0);
> > > > > > +
> > > > > > +   if (nodeoffset >= 0) {
> > > > > > +   if (core)
> > > > > > +   cell = fdt_getprop(fdt,
> > > > > > +   nodeoffset,
> > > > > > +   "bitstream_core",
> > > > > > +   len);
> > > > > > +   else
> > > > > > +   cell = fdt_getprop(fdt,
> > > > > > nodeoffset,
> > > > > > "bitstream_periph",
> > > > > > +    len);
> > > > > > +
> > > > > > +   if (cell)
> > > > > > +   cff_filename = cell;
> > > > > > +   }
> > > > > > +
> > > > > > +   return cff_filename;
> > > > > > +}
> > > > > 

Re: [U-Boot] [PATCH v2 09/19] arm: socfpga: Add drivers for programing FPGA from flash

[Marek Vasut]

On 09/27/2017 11:13 AM, Chee, Tien Fong wrote:
> On Sel, 2017-09-26 at 12:39 +0200, Marek Vasut wrote:
>> On 09/26/2017 11:52 AM, Chee, Tien Fong wrote:
>>>
>>> On Isn, 2017-09-25 at 11:14 +0200, Marek Vasut wrote:

 On 09/25/2017 10:40 AM, tien.fong.c...@intel.com wrote:
>
>
> From: Tien Fong Chee 
>
> These drivers handle FPGA program operation from flash loading
> RBF to memory and then to program FPGA.
>
> Signed-off-by: Tien Fong Chee 
>> [...]
>>
>>>

>
> +const char *get_cff_devpart(const void *fdt, int *len)
> +{
> + const char *cff_devpart = NULL;
> + const char *cell;
> + int nodeoffset;
> + nodeoffset = fdtdec_next_compatible(fdt, 0,
> +  COMPAT_ALTERA_SOCFPGA_FPGA0);
> +
> + cell = fdt_getprop(fdt, nodeoffset,
> "bitstream_devpart",
> len);
> +
> + if (cell)
> + cff_devpart = cell;
> +
> + return cff_devpart;
> +}
 Take a look at splash*.c , I believe that can be reworked into
 generic
 firmware loader , which you could then use here.

>>> the devpart is hard coded in splash*.c. The function here is
>>> getting
>>> devpart info from DTS. So, is there any similar function in
>>> splash*.c?
>>> May be you can share more about your idea.
>> The generic loader could use some work of course ...
>>
> Sorry, i am still confusing. Allow me to ask you more:
> 1. Is the generic firmware loader already exists in splash*.c?

No

> 2. Are you talking about get_cff_devpart or whole fpga laodfs?
> 3. You want me integrate get_cff_devpart function into splash*.c?
> 4. Are you means to hard code the devpart instead providing dynamic
> devpart described in DTS?

I am talking about factoring out generic firmware loader from splash*c ,
since it already contains most of the parts for such a thing.

> Current implementation are located in spl_board_init func
> (arcg/arm/mach-socfpga/spl.c). Based on boot device such as mmc, nand
> and QSPI, then reading some info from DTS, setting dev and partition
> with generic fs functions, and reading with generic fs function before
> programming RBF into FPGA. All these are in patch 19.

That's what splash*c also does, so adding separate parallel
implementation of the same functionality is a no-no.

> Thanks.
>> [...]


-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 09/19] arm: socfpga: Add drivers for programing FPGA from flash

[Chee, Tien Fong]

On Sel, 2017-09-26 at 12:39 +0200, Marek Vasut wrote:
> On 09/26/2017 11:52 AM, Chee, Tien Fong wrote:
> > 
> > On Isn, 2017-09-25 at 11:14 +0200, Marek Vasut wrote:
> > > 
> > > On 09/25/2017 10:40 AM, tien.fong.c...@intel.com wrote:
> > > > 
> > > > 
> > > > From: Tien Fong Chee 
> > > > 
> > > > These drivers handle FPGA program operation from flash loading
> > > > RBF to memory and then to program FPGA.
> > > > 
> > > > Signed-off-by: Tien Fong Chee 
> [...]
> 
> > 
> > > 
> > > > 
> > > > +const char *get_cff_devpart(const void *fdt, int *len)
> > > > +{
> > > > +   const char *cff_devpart = NULL;
> > > > +   const char *cell;
> > > > +   int nodeoffset;
> > > > +   nodeoffset = fdtdec_next_compatible(fdt, 0,
> > > > +    COMPAT_ALTERA_SOCFPGA_FPGA0);
> > > > +
> > > > +   cell = fdt_getprop(fdt, nodeoffset,
> > > > "bitstream_devpart",
> > > > len);
> > > > +
> > > > +   if (cell)
> > > > +   cff_devpart = cell;
> > > > +
> > > > +   return cff_devpart;
> > > > +}
> > > Take a look at splash*.c , I believe that can be reworked into
> > > generic
> > > firmware loader , which you could then use here.
> > > 
> > the devpart is hard coded in splash*.c. The function here is
> > getting
> > devpart info from DTS. So, is there any similar function in
> > splash*.c?
> > May be you can share more about your idea.
> The generic loader could use some work of course ...
> 
Sorry, i am still confusing. Allow me to ask you more:
1. Is the generic firmware loader already exists in splash*.c?
2. Are you talking about get_cff_devpart or whole fpga laodfs?
3. You want me integrate get_cff_devpart function into splash*.c?
4. Are you means to hard code the devpart instead providing dynamic
devpart described in DTS?

Current implementation are located in spl_board_init func
(arcg/arm/mach-socfpga/spl.c). Based on boot device such as mmc, nand
and QSPI, then reading some info from DTS, setting dev and partition
with generic fs functions, and reading with generic fs function before
programming RBF into FPGA. All these are in patch 19.

Thanks.
> [...]
> 
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 09/19] arm: socfpga: Add drivers for programing FPGA from flash

[Marek Vasut]

On 09/27/2017 08:05 AM, Chee, Tien Fong wrote:
> On Sel, 2017-09-26 at 12:32 +0200, Marek Vasut wrote:
>> On 09/26/2017 10:30 AM, Chee, Tien Fong wrote:
>>>
>>> On Isn, 2017-09-25 at 11:14 +0200, Marek Vasut wrote:

 On 09/25/2017 10:40 AM, tien.fong.c...@intel.com wrote:
>
>
> From: Tien Fong Chee 
>
> These drivers handle FPGA program operation from flash loading
> RBF to memory and then to program FPGA.
>
> Signed-off-by: Tien Fong Chee 
 Did you run checkpatch on this before submitting ? I presume no
 ...

>>> Yeah, i run checkpatch for all patches. What's the issue here?
>> It should definitely indicate problem with ie. yoda-notation
>> +if (0 == flashinfo->remaining) {
>> and indent ...
>>
> No complaint from checkpath. I know someone saying bad for readbility,
> but yoda-notation at this simple implementation doesn't impact the
> readbility, and having benefit to leverage detection of compiler on
> missing "=". Overall, this can help to improve coding quality. I can
> remove it if this doesn't favored in U-boot.

It is not welcome and modern gcc warns you about such things .

>>>

>
>
> ---
>  .../include/mach/fpga_manager_arria10.h|  27 ++
>  drivers/fpga/socfpga_arria10.c | 391
> -
>  include/altera.h   |   6 +
>  include/configs/socfpga_common.h   |   4 +
>  4 files changed, 425 insertions(+), 3 deletions(-)
 [...]

>
>
> @@ -112,13 +122,14 @@ static int
> wait_for_nconfig_pin_and_nstatus_pin(void)
>   unsigned long mask =
> ALT_FPGAMGR_IMGCFG_STAT_F2S_NCONFIG_PIN_SET_MSK |
>   ALT_FPGAMGR_IMGCFG_STAT_F2S_NS
> TATU
> S_PIN_SET_MSK;
>  
> - /* Poll until f2s_nconfig_pin and f2s_nstatus_pin;
> loop
> until de-asserted,
> -  * timeout at 1000ms
> + /*
> +  * Poll until f2s_nconfig_pin and f2s_nstatus_pin;
> loop
> until
> +  * de-asserted, timeout at 1000ms
>    */
>   return wait_for_bit(__func__,
>   _manager_base->imgcfg_stat,
>   mask,
> - false, FPGA_TIMEOUT_MSEC, false);
> + true, FPGA_TIMEOUT_MSEC, false);
>  }
 Seems more like a fix, split this out.

>>> Okay.

>
>
>  static int wait_for_f2s_nstatus_pin(unsigned long value)
> @@ -469,6 +480,7 @@ int socfpga_load(Altera_desc *desc, const
> void
> *rbf_data, size_t rbf_size)
>  
>   /* Initialize the FPGA Manager */
>   status = fpgamgr_program_init((u32 *)rbf_data,
> rbf_size);
> +
>   if (status)
>   return status;
>  
> @@ -477,3 +489,376 @@ int socfpga_load(Altera_desc *desc, const
> void *rbf_data, size_t rbf_size)
>  
>   return fpgamgr_program_finish();
>  }
> +
> +#if defined(CONFIG_CMD_FPGA_LOADFS)
> +const char *get_cff_filename(const void *fdt, int *len, u32
> core)
> +{
> + const char *cff_filename = NULL;
> + const char *cell;
> + int nodeoffset;
> + nodeoffset = fdtdec_next_compatible(fdt, 0,
> +  COMPAT_ALTERA_SOCFPGA_FPGA0);
> +
> + if (nodeoffset >= 0) {
> + if (core)
> + cell = fdt_getprop(fdt,
> + nodeoffset,
> + "bitstream_core",
> + len);
> + else
> + cell = fdt_getprop(fdt, nodeoffset,
> "bitstream_periph",
> +  len);
> +
> + if (cell)
> + cff_filename = cell;
> + }
> +
> + return cff_filename;
> +}
> +
> +const char *get_cff_devpart(const void *fdt, int *len)
> +{
> + const char *cff_devpart = NULL;
> + const char *cell;
> + int nodeoffset;
> + nodeoffset = fdtdec_next_compatible(fdt, 0,
> +  COMPAT_ALTERA_SOCFPGA_FPGA0);
> +
> + cell = fdt_getprop(fdt, nodeoffset,
> "bitstream_devpart",
> len);
> +
> + if (cell)
> + cff_devpart = cell;
> +
> + return cff_devpart;
> +}
 Take a look at splash*.c , I believe that can be reworked into
 generic
 firmware loader , which you could then use here.
>> This is important here, I don't want yet another ad-hoc loader ...
>>
> Disucss in another email reply separately. I need time to think.
>>>

 [...]

>
>
> diff --git a/include/configs/socfpga_common.h
> b/include/configs/socfpga_common.h
> index 9897e11..eadce2d 100644
> --- a/include/configs/socfpga_common.h
> +++ b/include/configs/socfpga_common.h
> @@ -27,7 +27,11 @@
>   */
>  #define 

Re: [U-Boot] [PATCH v2 09/19] arm: socfpga: Add drivers for programing FPGA from flash

[Chee, Tien Fong]

On Sel, 2017-09-26 at 12:32 +0200, Marek Vasut wrote:
> On 09/26/2017 10:30 AM, Chee, Tien Fong wrote:
> > 
> > On Isn, 2017-09-25 at 11:14 +0200, Marek Vasut wrote:
> > > 
> > > On 09/25/2017 10:40 AM, tien.fong.c...@intel.com wrote:
> > > > 
> > > > 
> > > > From: Tien Fong Chee 
> > > > 
> > > > These drivers handle FPGA program operation from flash loading
> > > > RBF to memory and then to program FPGA.
> > > > 
> > > > Signed-off-by: Tien Fong Chee 
> > > Did you run checkpatch on this before submitting ? I presume no
> > > ...
> > > 
> > Yeah, i run checkpatch for all patches. What's the issue here?
> It should definitely indicate problem with ie. yoda-notation
> +if (0 == flashinfo->remaining) {
> and indent ...
> 
No complaint from checkpath. I know someone saying bad for readbility,
but yoda-notation at this simple implementation doesn't impact the
readbility, and having benefit to leverage detection of compiler on
missing "=". Overall, this can help to improve coding quality. I can
remove it if this doesn't favored in U-boot.
> > 
> > > 
> > > > 
> > > > 
> > > > ---
> > > >  .../include/mach/fpga_manager_arria10.h|  27 ++
> > > >  drivers/fpga/socfpga_arria10.c | 391
> > > > -
> > > >  include/altera.h   |   6 +
> > > >  include/configs/socfpga_common.h   |   4 +
> > > >  4 files changed, 425 insertions(+), 3 deletions(-)
> > > [...]
> > > 
> > > > 
> > > > 
> > > > @@ -112,13 +122,14 @@ static int
> > > > wait_for_nconfig_pin_and_nstatus_pin(void)
> > > >     unsigned long mask =
> > > > ALT_FPGAMGR_IMGCFG_STAT_F2S_NCONFIG_PIN_SET_MSK |
> > > >     ALT_FPGAMGR_IMGCFG_STAT_F2S_NS
> > > > TATU
> > > > S_PIN_SET_MSK;
> > > >  
> > > > -   /* Poll until f2s_nconfig_pin and f2s_nstatus_pin;
> > > > loop
> > > > until de-asserted,
> > > > -    * timeout at 1000ms
> > > > +   /*
> > > > +    * Poll until f2s_nconfig_pin and f2s_nstatus_pin;
> > > > loop
> > > > until
> > > > +    * de-asserted, timeout at 1000ms
> > > >      */
> > > >     return wait_for_bit(__func__,
> > > >     _manager_base->imgcfg_stat,
> > > >     mask,
> > > > -   false, FPGA_TIMEOUT_MSEC, false);
> > > > +   true, FPGA_TIMEOUT_MSEC, false);
> > > >  }
> > > Seems more like a fix, split this out.
> > > 
> > Okay.
> > > 
> > > > 
> > > > 
> > > >  static int wait_for_f2s_nstatus_pin(unsigned long value)
> > > > @@ -469,6 +480,7 @@ int socfpga_load(Altera_desc *desc, const
> > > > void
> > > > *rbf_data, size_t rbf_size)
> > > >  
> > > >     /* Initialize the FPGA Manager */
> > > >     status = fpgamgr_program_init((u32 *)rbf_data,
> > > > rbf_size);
> > > > +
> > > >     if (status)
> > > >     return status;
> > > >  
> > > > @@ -477,3 +489,376 @@ int socfpga_load(Altera_desc *desc, const
> > > > void *rbf_data, size_t rbf_size)
> > > >  
> > > >     return fpgamgr_program_finish();
> > > >  }
> > > > +
> > > > +#if defined(CONFIG_CMD_FPGA_LOADFS)
> > > > +const char *get_cff_filename(const void *fdt, int *len, u32
> > > > core)
> > > > +{
> > > > +   const char *cff_filename = NULL;
> > > > +   const char *cell;
> > > > +   int nodeoffset;
> > > > +   nodeoffset = fdtdec_next_compatible(fdt, 0,
> > > > +    COMPAT_ALTERA_SOCFPGA_FPGA0);
> > > > +
> > > > +   if (nodeoffset >= 0) {
> > > > +   if (core)
> > > > +   cell = fdt_getprop(fdt,
> > > > +   nodeoffset,
> > > > +   "bitstream_core",
> > > > +   len);
> > > > +   else
> > > > +   cell = fdt_getprop(fdt, nodeoffset,
> > > > "bitstream_periph",
> > > > +    len);
> > > > +
> > > > +   if (cell)
> > > > +   cff_filename = cell;
> > > > +   }
> > > > +
> > > > +   return cff_filename;
> > > > +}
> > > > +
> > > > +const char *get_cff_devpart(const void *fdt, int *len)
> > > > +{
> > > > +   const char *cff_devpart = NULL;
> > > > +   const char *cell;
> > > > +   int nodeoffset;
> > > > +   nodeoffset = fdtdec_next_compatible(fdt, 0,
> > > > +    COMPAT_ALTERA_SOCFPGA_FPGA0);
> > > > +
> > > > +   cell = fdt_getprop(fdt, nodeoffset,
> > > > "bitstream_devpart",
> > > > len);
> > > > +
> > > > +   if (cell)
> > > > +   cff_devpart = cell;
> > > > +
> > > > +   return cff_devpart;
> > > > +}
> > > Take a look at splash*.c , I believe that can be reworked into
> > > generic
> > > firmware loader , which you could then use here.
> This is important here, I don't want yet another ad-hoc loader ...
> 

Re: [U-Boot] [PATCH v2 09/19] arm: socfpga: Add drivers for programing FPGA from flash

[Marek Vasut]

On 09/26/2017 11:52 AM, Chee, Tien Fong wrote:
> On Isn, 2017-09-25 at 11:14 +0200, Marek Vasut wrote:
>> On 09/25/2017 10:40 AM, tien.fong.c...@intel.com wrote:
>>>
>>> From: Tien Fong Chee 
>>>
>>> These drivers handle FPGA program operation from flash loading
>>> RBF to memory and then to program FPGA.
>>>
>>> Signed-off-by: Tien Fong Chee 

[...]

>>> +const char *get_cff_devpart(const void *fdt, int *len)
>>> +{
>>> +   const char *cff_devpart = NULL;
>>> +   const char *cell;
>>> +   int nodeoffset;
>>> +   nodeoffset = fdtdec_next_compatible(fdt, 0,
>>> +    COMPAT_ALTERA_SOCFPGA_FPGA0);
>>> +
>>> +   cell = fdt_getprop(fdt, nodeoffset, "bitstream_devpart",
>>> len);
>>> +
>>> +   if (cell)
>>> +   cff_devpart = cell;
>>> +
>>> +   return cff_devpart;
>>> +}
>> Take a look at splash*.c , I believe that can be reworked into
>> generic
>> firmware loader , which you could then use here.
>>
> the devpart is hard coded in splash*.c. The function here is getting
> devpart info from DTS. So, is there any similar function in splash*.c?
> May be you can share more about your idea.

The generic loader could use some work of course ...

[...]

-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 09/19] arm: socfpga: Add drivers for programing FPGA from flash

[Marek Vasut]

On 09/26/2017 10:30 AM, Chee, Tien Fong wrote:
> On Isn, 2017-09-25 at 11:14 +0200, Marek Vasut wrote:
>> On 09/25/2017 10:40 AM, tien.fong.c...@intel.com wrote:
>>>
>>> From: Tien Fong Chee 
>>>
>>> These drivers handle FPGA program operation from flash loading
>>> RBF to memory and then to program FPGA.
>>>
>>> Signed-off-by: Tien Fong Chee 
>> Did you run checkpatch on this before submitting ? I presume no ...
>>
> Yeah, i run checkpatch for all patches. What's the issue here?

It should definitely indicate problem with ie. yoda-notation
+if (0 == flashinfo->remaining) {
and indent ...

>>>
>>> ---
>>>  .../include/mach/fpga_manager_arria10.h|  27 ++
>>>  drivers/fpga/socfpga_arria10.c | 391
>>> -
>>>  include/altera.h   |   6 +
>>>  include/configs/socfpga_common.h   |   4 +
>>>  4 files changed, 425 insertions(+), 3 deletions(-)
>> [...]
>>
>>>
>>> @@ -112,13 +122,14 @@ static int
>>> wait_for_nconfig_pin_and_nstatus_pin(void)
>>>     unsigned long mask =
>>> ALT_FPGAMGR_IMGCFG_STAT_F2S_NCONFIG_PIN_SET_MSK |
>>>     ALT_FPGAMGR_IMGCFG_STAT_F2S_NSTATU
>>> S_PIN_SET_MSK;
>>>  
>>> -   /* Poll until f2s_nconfig_pin and f2s_nstatus_pin; loop
>>> until de-asserted,
>>> -    * timeout at 1000ms
>>> +   /*
>>> +    * Poll until f2s_nconfig_pin and f2s_nstatus_pin; loop
>>> until
>>> +    * de-asserted, timeout at 1000ms
>>>      */
>>>     return wait_for_bit(__func__,
>>>     _manager_base->imgcfg_stat,
>>>     mask,
>>> -   false, FPGA_TIMEOUT_MSEC, false);
>>> +   true, FPGA_TIMEOUT_MSEC, false);
>>>  }
>> Seems more like a fix, split this out.
>>
> Okay.
>>>
>>>  static int wait_for_f2s_nstatus_pin(unsigned long value)
>>> @@ -469,6 +480,7 @@ int socfpga_load(Altera_desc *desc, const void
>>> *rbf_data, size_t rbf_size)
>>>  
>>>     /* Initialize the FPGA Manager */
>>>     status = fpgamgr_program_init((u32 *)rbf_data, rbf_size);
>>> +
>>>     if (status)
>>>     return status;
>>>  
>>> @@ -477,3 +489,376 @@ int socfpga_load(Altera_desc *desc, const
>>> void *rbf_data, size_t rbf_size)
>>>  
>>>     return fpgamgr_program_finish();
>>>  }
>>> +
>>> +#if defined(CONFIG_CMD_FPGA_LOADFS)
>>> +const char *get_cff_filename(const void *fdt, int *len, u32 core)
>>> +{
>>> +   const char *cff_filename = NULL;
>>> +   const char *cell;
>>> +   int nodeoffset;
>>> +   nodeoffset = fdtdec_next_compatible(fdt, 0,
>>> +    COMPAT_ALTERA_SOCFPGA_FPGA0);
>>> +
>>> +   if (nodeoffset >= 0) {
>>> +   if (core)
>>> +   cell = fdt_getprop(fdt,
>>> +   nodeoffset,
>>> +   "bitstream_core",
>>> +   len);
>>> +   else
>>> +   cell = fdt_getprop(fdt, nodeoffset,
>>> "bitstream_periph",
>>> +    len);
>>> +
>>> +   if (cell)
>>> +   cff_filename = cell;
>>> +   }
>>> +
>>> +   return cff_filename;
>>> +}
>>> +
>>> +const char *get_cff_devpart(const void *fdt, int *len)
>>> +{
>>> +   const char *cff_devpart = NULL;
>>> +   const char *cell;
>>> +   int nodeoffset;
>>> +   nodeoffset = fdtdec_next_compatible(fdt, 0,
>>> +    COMPAT_ALTERA_SOCFPGA_FPGA0);
>>> +
>>> +   cell = fdt_getprop(fdt, nodeoffset, "bitstream_devpart",
>>> len);
>>> +
>>> +   if (cell)
>>> +   cff_devpart = cell;
>>> +
>>> +   return cff_devpart;
>>> +}
>> Take a look at splash*.c , I believe that can be reworked into
>> generic
>> firmware loader , which you could then use here.

This is important here, I don't want yet another ad-hoc loader ...

>> [...]
>>
>>>
>>> diff --git a/include/configs/socfpga_common.h
>>> b/include/configs/socfpga_common.h
>>> index 9897e11..eadce2d 100644
>>> --- a/include/configs/socfpga_common.h
>>> +++ b/include/configs/socfpga_common.h
>>> @@ -27,7 +27,11 @@
>>>   */
>>>  #define CONFIG_NR_DRAM_BANKS   1
>>>  #define PHYS_SDRAM_1   0x0
>>> +#if defined(CONFIG_TARGET_SOCFPGA_GEN5)
>>>  #define CONFIG_SYS_MALLOC_LEN  (64 * 1024 * 1024)
>>> +#elif defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
>>> +#define CONFIG_SYS_MALLOC_LEN  (128 * 1024 * 1024)
>>> +#endif
>> You definitely don't need 128 MiB of malloc area.
>>
> Okay, i will try out with smaller size.

Why do you need such massive area ? It's not a matter of "try out", you
should know why this change was needed for your use-case.

>>>
>>>  #define CONFIG_SYS_MEMTEST_START   PHYS_SDRAM_1
>>>  #define CONFIG_SYS_MEMTEST_END PHYS_SDRAM_1_SIZE
>>>  #if defined(CONFIG_TARGET_SOCFPGA_GEN5)
>>>


-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de

Re: [U-Boot] [PATCH v2 09/19] arm: socfpga: Add drivers for programing FPGA from flash

[Chee, Tien Fong]

On Isn, 2017-09-25 at 11:14 +0200, Marek Vasut wrote:
> On 09/25/2017 10:40 AM, tien.fong.c...@intel.com wrote:
> > 
> > From: Tien Fong Chee 
> > 
> > These drivers handle FPGA program operation from flash loading
> > RBF to memory and then to program FPGA.
> > 
> > Signed-off-by: Tien Fong Chee 
> Did you run checkpatch on this before submitting ? I presume no ...
> 
> > 
> > ---
> >  .../include/mach/fpga_manager_arria10.h|  27 ++
> >  drivers/fpga/socfpga_arria10.c | 391
> > -
> >  include/altera.h   |   6 +
> >  include/configs/socfpga_common.h   |   4 +
> >  4 files changed, 425 insertions(+), 3 deletions(-)
> [...]
> 
> > 
> > @@ -112,13 +122,14 @@ static int
> > wait_for_nconfig_pin_and_nstatus_pin(void)
> >     unsigned long mask =
> > ALT_FPGAMGR_IMGCFG_STAT_F2S_NCONFIG_PIN_SET_MSK |
> >     ALT_FPGAMGR_IMGCFG_STAT_F2S_NSTATU
> > S_PIN_SET_MSK;
> >  
> > -   /* Poll until f2s_nconfig_pin and f2s_nstatus_pin; loop
> > until de-asserted,
> > -    * timeout at 1000ms
> > +   /*
> > +    * Poll until f2s_nconfig_pin and f2s_nstatus_pin; loop
> > until
> > +    * de-asserted, timeout at 1000ms
> >      */
> >     return wait_for_bit(__func__,
> >     _manager_base->imgcfg_stat,
> >     mask,
> > -   false, FPGA_TIMEOUT_MSEC, false);
> > +   true, FPGA_TIMEOUT_MSEC, false);
> >  }
> Seems more like a fix, split this out.
> 
> > 
> >  static int wait_for_f2s_nstatus_pin(unsigned long value)
> > @@ -469,6 +480,7 @@ int socfpga_load(Altera_desc *desc, const void
> > *rbf_data, size_t rbf_size)
> >  
> >     /* Initialize the FPGA Manager */
> >     status = fpgamgr_program_init((u32 *)rbf_data, rbf_size);
> > +
> >     if (status)
> >     return status;
> >  
> > @@ -477,3 +489,376 @@ int socfpga_load(Altera_desc *desc, const
> > void *rbf_data, size_t rbf_size)
> >  
> >     return fpgamgr_program_finish();
> >  }
> > +
> > +#if defined(CONFIG_CMD_FPGA_LOADFS)
> > +const char *get_cff_filename(const void *fdt, int *len, u32 core)
> > +{
> > +   const char *cff_filename = NULL;
> > +   const char *cell;
> > +   int nodeoffset;
> > +   nodeoffset = fdtdec_next_compatible(fdt, 0,
> > +    COMPAT_ALTERA_SOCFPGA_FPGA0);
> > +
> > +   if (nodeoffset >= 0) {
> > +   if (core)
> > +   cell = fdt_getprop(fdt,
> > +   nodeoffset,
> > +   "bitstream_core",
> > +   len);
> > +   else
> > +   cell = fdt_getprop(fdt, nodeoffset,
> > "bitstream_periph",
> > +    len);
> > +
> > +   if (cell)
> > +   cff_filename = cell;
> > +   }
> > +
> > +   return cff_filename;
> > +}
> > +
> > +const char *get_cff_devpart(const void *fdt, int *len)
> > +{
> > +   const char *cff_devpart = NULL;
> > +   const char *cell;
> > +   int nodeoffset;
> > +   nodeoffset = fdtdec_next_compatible(fdt, 0,
> > +    COMPAT_ALTERA_SOCFPGA_FPGA0);
> > +
> > +   cell = fdt_getprop(fdt, nodeoffset, "bitstream_devpart",
> > len);
> > +
> > +   if (cell)
> > +   cff_devpart = cell;
> > +
> > +   return cff_devpart;
> > +}
> Take a look at splash*.c , I believe that can be reworked into
> generic
> firmware loader , which you could then use here.
> 
the devpart is hard coded in splash*.c. The function here is getting
devpart info from DTS. So, is there any similar function in splash*.c?
May be you can share more about your idea.
> [...]
> 
> > 
> > diff --git a/include/configs/socfpga_common.h
> > b/include/configs/socfpga_common.h
> > index 9897e11..eadce2d 100644
> > --- a/include/configs/socfpga_common.h
> > +++ b/include/configs/socfpga_common.h
> > @@ -27,7 +27,11 @@
> >   */
> >  #define CONFIG_NR_DRAM_BANKS   1
> >  #define PHYS_SDRAM_1   0x0
> > +#if defined(CONFIG_TARGET_SOCFPGA_GEN5)
> >  #define CONFIG_SYS_MALLOC_LEN  (64 * 1024 * 1024)
> > +#elif defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
> > +#define CONFIG_SYS_MALLOC_LEN  (128 * 1024 * 1024)
> > +#endif
> You definitely don't need 128 MiB of malloc area.
> 
> > 
> >  #define CONFIG_SYS_MEMTEST_START   PHYS_SDRAM_1
> >  #define CONFIG_SYS_MEMTEST_END PHYS_SDRAM_1_SIZE
> >  #if defined(CONFIG_TARGET_SOCFPGA_GEN5)
> > 
> 
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 09/19] arm: socfpga: Add drivers for programing FPGA from flash

[Chee, Tien Fong]

On Isn, 2017-09-25 at 11:14 +0200, Marek Vasut wrote:
> On 09/25/2017 10:40 AM, tien.fong.c...@intel.com wrote:
> > 
> > From: Tien Fong Chee 
> > 
> > These drivers handle FPGA program operation from flash loading
> > RBF to memory and then to program FPGA.
> > 
> > Signed-off-by: Tien Fong Chee 
> Did you run checkpatch on this before submitting ? I presume no ...
> 
Yeah, i run checkpatch for all patches. What's the issue here?
> > 
> > ---
> >  .../include/mach/fpga_manager_arria10.h|  27 ++
> >  drivers/fpga/socfpga_arria10.c | 391
> > -
> >  include/altera.h   |   6 +
> >  include/configs/socfpga_common.h   |   4 +
> >  4 files changed, 425 insertions(+), 3 deletions(-)
> [...]
> 
> > 
> > @@ -112,13 +122,14 @@ static int
> > wait_for_nconfig_pin_and_nstatus_pin(void)
> >     unsigned long mask =
> > ALT_FPGAMGR_IMGCFG_STAT_F2S_NCONFIG_PIN_SET_MSK |
> >     ALT_FPGAMGR_IMGCFG_STAT_F2S_NSTATU
> > S_PIN_SET_MSK;
> >  
> > -   /* Poll until f2s_nconfig_pin and f2s_nstatus_pin; loop
> > until de-asserted,
> > -    * timeout at 1000ms
> > +   /*
> > +    * Poll until f2s_nconfig_pin and f2s_nstatus_pin; loop
> > until
> > +    * de-asserted, timeout at 1000ms
> >      */
> >     return wait_for_bit(__func__,
> >     _manager_base->imgcfg_stat,
> >     mask,
> > -   false, FPGA_TIMEOUT_MSEC, false);
> > +   true, FPGA_TIMEOUT_MSEC, false);
> >  }
> Seems more like a fix, split this out.
> 
Okay.
> > 
> >  static int wait_for_f2s_nstatus_pin(unsigned long value)
> > @@ -469,6 +480,7 @@ int socfpga_load(Altera_desc *desc, const void
> > *rbf_data, size_t rbf_size)
> >  
> >     /* Initialize the FPGA Manager */
> >     status = fpgamgr_program_init((u32 *)rbf_data, rbf_size);
> > +
> >     if (status)
> >     return status;
> >  
> > @@ -477,3 +489,376 @@ int socfpga_load(Altera_desc *desc, const
> > void *rbf_data, size_t rbf_size)
> >  
> >     return fpgamgr_program_finish();
> >  }
> > +
> > +#if defined(CONFIG_CMD_FPGA_LOADFS)
> > +const char *get_cff_filename(const void *fdt, int *len, u32 core)
> > +{
> > +   const char *cff_filename = NULL;
> > +   const char *cell;
> > +   int nodeoffset;
> > +   nodeoffset = fdtdec_next_compatible(fdt, 0,
> > +    COMPAT_ALTERA_SOCFPGA_FPGA0);
> > +
> > +   if (nodeoffset >= 0) {
> > +   if (core)
> > +   cell = fdt_getprop(fdt,
> > +   nodeoffset,
> > +   "bitstream_core",
> > +   len);
> > +   else
> > +   cell = fdt_getprop(fdt, nodeoffset,
> > "bitstream_periph",
> > +    len);
> > +
> > +   if (cell)
> > +   cff_filename = cell;
> > +   }
> > +
> > +   return cff_filename;
> > +}
> > +
> > +const char *get_cff_devpart(const void *fdt, int *len)
> > +{
> > +   const char *cff_devpart = NULL;
> > +   const char *cell;
> > +   int nodeoffset;
> > +   nodeoffset = fdtdec_next_compatible(fdt, 0,
> > +    COMPAT_ALTERA_SOCFPGA_FPGA0);
> > +
> > +   cell = fdt_getprop(fdt, nodeoffset, "bitstream_devpart",
> > len);
> > +
> > +   if (cell)
> > +   cff_devpart = cell;
> > +
> > +   return cff_devpart;
> > +}
> Take a look at splash*.c , I believe that can be reworked into
> generic
> firmware loader , which you could then use here.
> 
> [...]
> 
> > 
> > diff --git a/include/configs/socfpga_common.h
> > b/include/configs/socfpga_common.h
> > index 9897e11..eadce2d 100644
> > --- a/include/configs/socfpga_common.h
> > +++ b/include/configs/socfpga_common.h
> > @@ -27,7 +27,11 @@
> >   */
> >  #define CONFIG_NR_DRAM_BANKS   1
> >  #define PHYS_SDRAM_1   0x0
> > +#if defined(CONFIG_TARGET_SOCFPGA_GEN5)
> >  #define CONFIG_SYS_MALLOC_LEN  (64 * 1024 * 1024)
> > +#elif defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
> > +#define CONFIG_SYS_MALLOC_LEN  (128 * 1024 * 1024)
> > +#endif
> You definitely don't need 128 MiB of malloc area.
> 
Okay, i will try out with smaller size.
> > 
> >  #define CONFIG_SYS_MEMTEST_START   PHYS_SDRAM_1
> >  #define CONFIG_SYS_MEMTEST_END PHYS_SDRAM_1_SIZE
> >  #if defined(CONFIG_TARGET_SOCFPGA_GEN5)
> > 
> 
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 09/19] arm: socfpga: Add drivers for programing FPGA from flash

[Marek Vasut]

On 09/25/2017 10:40 AM, tien.fong.c...@intel.com wrote:
> From: Tien Fong Chee 
> 
> These drivers handle FPGA program operation from flash loading
> RBF to memory and then to program FPGA.
> 
> Signed-off-by: Tien Fong Chee 

Did you run checkpatch on this before submitting ? I presume no ...

> ---
>  .../include/mach/fpga_manager_arria10.h|  27 ++
>  drivers/fpga/socfpga_arria10.c | 391 
> -
>  include/altera.h   |   6 +
>  include/configs/socfpga_common.h   |   4 +
>  4 files changed, 425 insertions(+), 3 deletions(-)
[...]

> @@ -112,13 +122,14 @@ static int wait_for_nconfig_pin_and_nstatus_pin(void)
>   unsigned long mask = ALT_FPGAMGR_IMGCFG_STAT_F2S_NCONFIG_PIN_SET_MSK |
>   ALT_FPGAMGR_IMGCFG_STAT_F2S_NSTATUS_PIN_SET_MSK;
>  
> - /* Poll until f2s_nconfig_pin and f2s_nstatus_pin; loop until 
> de-asserted,
> -  * timeout at 1000ms
> + /*
> +  * Poll until f2s_nconfig_pin and f2s_nstatus_pin; loop until
> +  * de-asserted, timeout at 1000ms
>*/
>   return wait_for_bit(__func__,
>   _manager_base->imgcfg_stat,
>   mask,
> - false, FPGA_TIMEOUT_MSEC, false);
> + true, FPGA_TIMEOUT_MSEC, false);
>  }

Seems more like a fix, split this out.

>  static int wait_for_f2s_nstatus_pin(unsigned long value)
> @@ -469,6 +480,7 @@ int socfpga_load(Altera_desc *desc, const void *rbf_data, 
> size_t rbf_size)
>  
>   /* Initialize the FPGA Manager */
>   status = fpgamgr_program_init((u32 *)rbf_data, rbf_size);
> +
>   if (status)
>   return status;
>  
> @@ -477,3 +489,376 @@ int socfpga_load(Altera_desc *desc, const void 
> *rbf_data, size_t rbf_size)
>  
>   return fpgamgr_program_finish();
>  }
> +
> +#if defined(CONFIG_CMD_FPGA_LOADFS)
> +const char *get_cff_filename(const void *fdt, int *len, u32 core)
> +{
> + const char *cff_filename = NULL;
> + const char *cell;
> + int nodeoffset;
> + nodeoffset = fdtdec_next_compatible(fdt, 0,
> +  COMPAT_ALTERA_SOCFPGA_FPGA0);
> +
> + if (nodeoffset >= 0) {
> + if (core)
> + cell = fdt_getprop(fdt,
> + nodeoffset,
> + "bitstream_core",
> + len);
> + else
> + cell = fdt_getprop(fdt, nodeoffset, "bitstream_periph",
> +  len);
> +
> + if (cell)
> + cff_filename = cell;
> + }
> +
> + return cff_filename;
> +}
> +
> +const char *get_cff_devpart(const void *fdt, int *len)
> +{
> + const char *cff_devpart = NULL;
> + const char *cell;
> + int nodeoffset;
> + nodeoffset = fdtdec_next_compatible(fdt, 0,
> +  COMPAT_ALTERA_SOCFPGA_FPGA0);
> +
> + cell = fdt_getprop(fdt, nodeoffset, "bitstream_devpart", len);
> +
> + if (cell)
> + cff_devpart = cell;
> +
> + return cff_devpart;
> +}

Take a look at splash*.c , I believe that can be reworked into generic
firmware loader , which you could then use here.

[...]

> diff --git a/include/configs/socfpga_common.h 
> b/include/configs/socfpga_common.h
> index 9897e11..eadce2d 100644
> --- a/include/configs/socfpga_common.h
> +++ b/include/configs/socfpga_common.h
> @@ -27,7 +27,11 @@
>   */
>  #define CONFIG_NR_DRAM_BANKS 1
>  #define PHYS_SDRAM_1 0x0
> +#if defined(CONFIG_TARGET_SOCFPGA_GEN5)
>  #define CONFIG_SYS_MALLOC_LEN(64 * 1024 * 1024)
> +#elif defined(CONFIG_TARGET_SOCFPGA_ARRIA10)
> +#define CONFIG_SYS_MALLOC_LEN(128 * 1024 * 1024)
> +#endif

You definitely don't need 128 MiB of malloc area.

>  #define CONFIG_SYS_MEMTEST_START PHYS_SDRAM_1
>  #define CONFIG_SYS_MEMTEST_END   PHYS_SDRAM_1_SIZE
>  #if defined(CONFIG_TARGET_SOCFPGA_GEN5)
> 


-- 
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH v2 09/19] arm: socfpga: Add drivers for programing FPGA from flash

[tien . fong . chee]

From: Tien Fong Chee 

These drivers handle FPGA program operation from flash loading
RBF to memory and then to program FPGA.

Signed-off-by: Tien Fong Chee 
---
 .../include/mach/fpga_manager_arria10.h|  27 ++
 drivers/fpga/socfpga_arria10.c | 391 -
 include/altera.h   |   6 +
 include/configs/socfpga_common.h   |   4 +
 4 files changed, 425 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h 
b/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h
index 9cbf696..93a9122 100644
--- a/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h
+++ b/arch/arm/mach-socfpga/include/mach/fpga_manager_arria10.h
@@ -8,6 +8,8 @@
 #ifndef _FPGA_MANAGER_ARRIA10_H_
 #define _FPGA_MANAGER_ARRIA10_H_
 
+#include 
+
 #define ALT_FPGAMGR_IMGCFG_STAT_F2S_CRC_ERROR_SET_MSK  BIT(0)
 #define ALT_FPGAMGR_IMGCFG_STAT_F2S_EARLY_USERMODE_SET_MSK BIT(1)
 #define ALT_FPGAMGR_IMGCFG_STAT_F2S_USERMODE_SET_MSK   BIT(2)
@@ -89,11 +91,36 @@ struct socfpga_fpga_manager {
u32  imgcfg_fifo_status;
 };
 
+#if defined(CONFIG_CMD_FPGA_LOADFS)
+enum rbf_type {unknown, periph_section, core_section};
+enum rbf_security {invalid, unencrypted, encrypted};
+
+struct rbf_info {
+   enum rbf_type section;
+   enum rbf_security security;
+};
+
+struct flash_info {
+   char *interface;
+   char *dev_part;
+   char *filename;
+   int fstype;
+   u32 remaining;
+   u32 flash_offset;
+   struct rbf_info rbfinfo;
+   struct image_header header;
+};
+#endif
+
 /* Functions */
 int fpgamgr_program_init(u32 * rbf_data, size_t rbf_size);
 int fpgamgr_program_finish(void);
 int is_fpgamgr_user_mode(void);
 int fpgamgr_wait_early_user_mode(void);
+#if defined(CONFIG_CMD_FPGA_LOADFS)
+const char *get_cff_filename(const void *fdt, int *len, u32 core);
+const char *get_cff_devpart(const void *fdt, int *len);
+#endif
 
 #endif /* __ASSEMBLY__ */
 
diff --git a/drivers/fpga/socfpga_arria10.c b/drivers/fpga/socfpga_arria10.c
index 5c1a68a..5fe57ef 100644
--- a/drivers/fpga/socfpga_arria10.c
+++ b/drivers/fpga/socfpga_arria10.c
@@ -13,6 +13,12 @@
 #include 
 #include 
 #include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
 #include 
 #include 
 
@@ -22,6 +28,10 @@
 #define COMPRESSION_OFFSET 229
 #define FPGA_TIMEOUT_MSEC  1000  /* timeout in ms */
 #define FPGA_TIMEOUT_CNT   0x100
+#define RBF_UNENCRYPTED0xa65c
+#define RBF_ENCRYPTED  0xa65d
+#define ARRIA10RBF_PERIPH  0x0001
+#define ARRIA10RBF_CORE0x8001
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -112,13 +122,14 @@ static int wait_for_nconfig_pin_and_nstatus_pin(void)
unsigned long mask = ALT_FPGAMGR_IMGCFG_STAT_F2S_NCONFIG_PIN_SET_MSK |
ALT_FPGAMGR_IMGCFG_STAT_F2S_NSTATUS_PIN_SET_MSK;
 
-   /* Poll until f2s_nconfig_pin and f2s_nstatus_pin; loop until 
de-asserted,
-* timeout at 1000ms
+   /*
+* Poll until f2s_nconfig_pin and f2s_nstatus_pin; loop until
+* de-asserted, timeout at 1000ms
 */
return wait_for_bit(__func__,
_manager_base->imgcfg_stat,
mask,
-   false, FPGA_TIMEOUT_MSEC, false);
+   true, FPGA_TIMEOUT_MSEC, false);
 }
 
 static int wait_for_f2s_nstatus_pin(unsigned long value)
@@ -469,6 +480,7 @@ int socfpga_load(Altera_desc *desc, const void *rbf_data, 
size_t rbf_size)
 
/* Initialize the FPGA Manager */
status = fpgamgr_program_init((u32 *)rbf_data, rbf_size);
+
if (status)
return status;
 
@@ -477,3 +489,376 @@ int socfpga_load(Altera_desc *desc, const void *rbf_data, 
size_t rbf_size)
 
return fpgamgr_program_finish();
 }
+
+#if defined(CONFIG_CMD_FPGA_LOADFS)
+const char *get_cff_filename(const void *fdt, int *len, u32 core)
+{
+   const char *cff_filename = NULL;
+   const char *cell;
+   int nodeoffset;
+   nodeoffset = fdtdec_next_compatible(fdt, 0,
+COMPAT_ALTERA_SOCFPGA_FPGA0);
+
+   if (nodeoffset >= 0) {
+   if (core)
+   cell = fdt_getprop(fdt,
+   nodeoffset,
+   "bitstream_core",
+   len);
+   else
+   cell = fdt_getprop(fdt, nodeoffset, "bitstream_periph",
+len);
+
+   if (cell)
+   cff_filename = cell;
+   }
+
+   return cff_filename;
+}
+
+const char *get_cff_devpart(const void *fdt, int *len)
+{
+   const char *cff_devpart = NULL;
+   const char *cell;
+   int nodeoffset;
+   nodeoffset =