[U-Boot] [PATCH v6] socfpga: Adding Scan Manager driver

[Michal Simek]

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 at altera.com>
>>> Cc: Dinh Nguyen <dinguyen at altera.com>
>>> Cc: Wolfgang Denk <wd at denx.de>
>>> CC: Pavel Machek <pavel at denx.de>
>>> Cc: Tom Rini <trini at ti.com>
>>> Cc: Albert Aribaud <albert.u.boot at 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

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140304/a54e5eb5/attachment.pgp>