On 03/04/2014 04:01 PM, Chin Liang See wrote:
Hi Michal,
On Fri, 2014-02-28 at 11:17 +0100, Michal Simek wrote:
On 02/27/2014 05:03 PM, Chin Liang See wrote:
Scan Manager driver will be called to configure the IOCSR scan chain. This configuration will setup the IO buffer settings
Signed-off-by: Chin Liang See clsee@altera.com Cc: Dinh Nguyen dinguyen@altera.com Cc: Wolfgang Denk wd@denx.de CC: Pavel Machek pavel@denx.de Cc: Tom Rini trini@ti.com Cc: Albert Aribaud albert.u.boot@aribaud.net
Changes for v6
- Fixed various coding style issue
Changes for v5
- Removal of additional blank line
- Added comment for magic number
Changes for v4
- avoid code duplication by add goto error
- include underscore to variables name
Changes for v3
- merge the handoff file and driver into single patch
Changes for v2
- rebase with latest v2014.01-rc1
arch/arm/cpu/armv7/socfpga/Makefile | 2 +- arch/arm/cpu/armv7/socfpga/scan_manager.c | 211 +++++++ arch/arm/cpu/armv7/socfpga/spl.c | 4 + arch/arm/include/asm/arch-socfpga/scan_manager.h | 96 +++ .../include/asm/arch-socfpga/socfpga_base_addrs.h | 1 + board/altera/socfpga/iocsr_config.c | 657 ++++++++++++++++++++ board/altera/socfpga/iocsr_config.h | 17 +
I still have problem with content of these two files. In iocsr_config.c is ~600 lines which targets just one specific hardware design configuration. I can't see any reason why this should go to mainline and stay there. Because it brings no value.
I would recommend you just to define that arrays like this
const unsigned long iocsr_scan_chain0_table[]; const unsigned long iocsr_scan_chain0_table[]; ...
- in header
#define CONFIG_HPS_IOCSR_SCANCHAIN0_LENGTH 0 #define CONFIG_HPS_IOCSR_SCANCHAIN1_LENGTH 0 #define CONFIG_HPS_IOCSR_SCANCHAIN2_LENGTH 0 #define CONFIG_HPS_IOCSR_SCANCHAIN3_LENGTH 0
and write these 2 files by hand. Then your users will just replace them by hand for specific board/design.
Actually the intention is that user can pull the code from git and build it. We want to avoid any tools dependency here.
There is tool dependency even you don't want to have it. These files are autogenerated by tools it means you already have dependency.
Also I expect that you can change all pins for uarts/ethernets/spi/i2c/etc that's why there is no golden configuration for socfpga that's why it is better to keep it empty just to compile it.
At same time, these files are located inside board folders. If user have different boards, they will have new set of folders here their own handoff files. From there, there won't the need to regenerate everytime.
Please explain me one thing how many users will use this configuration? Especially these ~600 lines? I also expect that you can generate unlimited number of configuration for the same board. It means this is just one configuration for one board which you will use till the time when you find out a bug in your tools and you will regenerate this.
Also I expect that socfpga_cyclone is family that's why you are also missing connection to the real board that's why every socfpga user will have to open tool and generate this configuration for the board (Or can download it from web or get with the board).
It is the same situation which we have with zynq that's why I didn't add our ps7_init file because it is just useless for others.
Thanks, Michal