[U-Boot] [PATCH 3/9] apf9328: Add Armadeus Project board APF9328

[Stefano Babic]

On 08/12/2011 01:41 AM, Eric Jarrige wrote:

Hi Eric,

>>> +int board_init(void) +{ +	gd->bd->bi_arch_number =
>>> CONFIG_MACH_TYPE;
>> 
>> Is there no MACH_TYPE for this board ? It is uncommon for an ARM
>> board. Should this board run Linux ?
> The MACH_TYPE for this board is 906. This board runs linux and the
> integration in linux is on going.

Ok, I have understood. The only thing you can directly set
MACH_TYPE_APF9328 here. IMHO it is more readable.

>>> +#if (CONFIG_DRIVER_DM9000) +	imx_gpio_mode(GPIO_PORTB | GPIO_DR
>>> | GPIO_IN | 14);
>> 
>> Is there a reason to put this code here and not in board_eth_init ?
>> It is related to Ethernet and I am supposing this setup should be
>> done before dm9000_initialize.
> The idea is to keep the configuration  of the CPU pins at one place
> and to keep the initialization of the ethernet controller in a
> separate function. Here this principle of segregation is not obvious
> as there is only one GPIO pin to configure.

Ok, understood. Maybe it is better to move this kind of initialization
adding a board_early_init_f() function (you need
CONFIG_BOARD_EARLY_INIT_F in your config). This assures that the gpio is
always set before the ethernet is initialized, as I suppose it must be.
Letting in the misc_init() works now, but the order could be changed in
future - the erarly entry point is thought for hardware initialization
before starting the drivers.

>> This function seems to me not very useful. Is it not better to drop
>> it ? It is not strictly required. You set also #undef
>> CONFIG_SHOW_BOOT_PROGRESS in the configuration file.
> Well I choosed to keep it in place in empty state to be the template
> for the final user to add their own debug method if needed. If it is
> an issue I would prefer to define the CONFIG_SHOW_BOOT_PROGRESS in
> the configuration file than removing this empty show_boot_progress as
> the main purpose of the board is education and trainings on embedded
> developments.

However, if the function is empty, the functionality is not shown - it
is quite dead code and does not teach very much. What about to add some
very simple code, as a print() or switching a led, for example, to
indicate which is the goal of the function ?

>> Do you have more as one FPGA on your board ? And if this is true,
>> they share the same firmware ? (I see only one
>> CONFIG_FIRMWARE_ADDR..)
> The APF9328 is a SOM therefore it possible to connect some other FPGA
> to the board. In such a situation the idea is to concatenate all the
> bitstream files in the firmware partition. During the download
> process to the FPGAs the end of one FPGA download starts the download
> of the next one using the FPGA signal PRG_DONE. So that it possible
> to have all firmware in only one partition and save memory space by
> using bitstream files. To be honest I do not remember if someone use
> this feature on this board but that is .

You are the best to say if it is work. Reading the code, I have expected
to see one fpga_load for each defined fpga structure, using offset
inside the loop to pass the correct start and length for each image.

Can you add a comment to explain the behavior and the fact the FPGAs are
concatenated ?


> For the time being this switch applies to this board only. So I did
> not plan to document it in the README. It is not clear for me if I
> should add a documentation in the README since this switch is only
> used in the APF9328 source files.

Then drop all this stuff from the configuration file and rename in a way
it is clear it is specific for your board only. You do not need to
document them in the README, because they are not general CONFIG_ switches.

> Well, What do you think to add the prototype directly in apf9328.c:
> extern int apf9328_init_fpga(void); This will avoid to expose the
> fpga_xx_fn to apf9328.c that does not care how the firmware is moved
> from CPU to FPGA and therefore remove the very small apf9328_fpga.h

Agree.

>> However, this code is quite the same I can find on other IMX
>> boards. Can we factorize it and push it as common code ?
> 
> The other boards use hardcoded values that are not compatible with
> the apf9328 board. We do not use the same components and so cannot
> reuse or merge their code. A factorization would require deep changes
> in the config files of every board. For this old CPU there is no
> chance to convince the maintainers of the other boards to rework
> their implementation and specifically the boot code in assembly.

This is right. These processors are obsolete, there are no more active
development for them. It is enough to see how many patches for theses
processors were sent to the list in the last years (only 1, to fix the
mx1ads board).

> 
> On the other hand I am working on some other ARM SOC more recent, so
> It may worth the pain to invest some effrot of  factorization on
> modern CPUs. What is your opinion?

Of course, this is a must and duplication of code must be avoided.
However, is there a chance to move your assembly file into
arch/arm/cpu/arm920t/imx/, making it available if someone else needs to
use these old-ancient processors ? We do not need to touch the other boards.

I see you load most values from the configuration file, and the code is
straightforward - storing these values into the registers. Any chance ?

Best regards,
Stefano Babic


-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================