Re: [Qemu-devel] [PATCH v2 00/20] Add Faraday A36x SoC platform support

[Andreas Färber]

Am 25.01.2013 09:19, schrieb Kuo-Jung Su:
> From: Kuo-Jung Su <address@hidden>
> 
> These patches introduce Faraday A36x SoC platform support.
> 
> Faraday provides ARMv4/ARMv5TE compatible solutions, 
> and focus on ASIC design service. 
> 
> Here are some public documents for you reference.
> 
> http://www.faraday-tech.com/html/documentation/index.html
> 
> The pre-built images are also available at my Google Drive:
> 
> https://docs.google.com/folder/d/0BwfiewvSmUgAX2pTTmtUMGFCZW8/edit
> 
> Here is the image file list:
> 
> 1. android-4.0.4/zImage: A369 linux-3.0.31
> 2. android-4.0.4/romfs-4.0.4.tar.bz2: android-4.0.4
> 3. nand.img.bz2: A369 nand flash image
> 4. rom.img.bz2: A369 embedded ROM image
> 5. u-boot: A369 u-boot-2012.10 ELF file
> 6. zImage: A369 linux-3.4.12 + initramfs
> 7. README

Thank you for posting these test images. Could you also publish your
patches on some public git branch so that one doesn't need to manually
apply 20 or more patches to test it? (e.g., GitHub, repo.or.cz, ...)

Another thing you could look into is qemu-jeos.git support - could you
supply configs for building an upstream Linux kernel for testing? You
can look at arm-vexpress.cfg as template.

> Changes for v2:
> 
>    1. coding style fixes (verified with checkpatch.pl)
>    2. add Faraday A360 support
>    3. add Faraday USB 2.0 EHCI support
>    4. merge a369_scu.c into a369.c
>    5. introduce QOM coding style
>    6. remove lowercase Macros: min(), max()
>    7. name all struct as CamelCase style
>    8. move function prototypes from .c to faraday.h
>    9. use switch instead of if statement in a369_ahbc_write
>   10. remove debug prints
>   11. update all uarts in A36x to DEVICE_LITTLE_ENDIAN.
>   12. move the variable definitions to the start of the function,
>       instead of inside a do { } while(0)
>   13. remove disabled and commented out code.
>   14. use hw_error() and exit() upon pflash register failed.
>   15. add const prior to TypeInfo (i.e. static *const* 
>       TypeInfo ftrtc011_info)
>   16. add le32_to_cpu/cpu_to_le32 to the descriptor processing in 
>       FTGMAC100/FTMAC110.
>   17. update the GPL license to GPL v2 (except for FTLCDC200, 
>       it's based on pl110.c which is LGPL.)
>   18. add const to src_* in the DMA controllers (FTAPBBRG020/FTDMAC020)
> 
>   Note:   
>   1. Thanks to the comments from KONRAD, Andreas, Paul and Blue.
>      But if there is still something missed, please kindly remind me to fix.
>      Thanks!

Your detailed change log is very helpful, thank you. When you post a v3
however, please post the patch series as a top-level patchset (i.e., the
cover letter not as a reply, just the actual patches to the cover
letter), the change log then should be sufficient to understand its
historical context and that makes it a little easier for humans or
Anthony's scripts to find the latest patch versions.

For all your new SysBus devices (i.e., types with .parent =
SYS_BUS_DEVICE) please use a QOM realizefn instead of a qdev initfn -
not just the function naming but dc->realize rather than sdc->init. This
avoids one of us having to go through your code later and doing the
conversion. (I showed you how to do that for EHCI as an example.)

What I'm still missing in v2 is qtest test cases for the RTC and a
libqos driver for your I2C master. If you have questions how to go about
that, feel free to ask on IRC.

>   2. There are still two coding style errors reported by checkpatch.pl,
>      but I think they might be false alarms, here is the error log:
> 
>   ERROR: need consistent spacing around '*' (ctx:WxV)
>   #74: FILE: hw/a360.c:32:
>   +    i2c_bus      *i2c[2];                ^
> 
>   ERROR: open brace '{' following function declarations go on the next line
>   #4680: FILE: hw/ftlcdc200_template.h:59:
>   +static drawfn glue(ftlcdc200_draw_fn_, BITS)[48] = {

Yes, your analysis looks correct, you can ignore both "errors".

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg