[U-Boot] [PATCH v3 00/10] Add Marvell Dove and SolidRun CuBox

[Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>]

On 02/11/2013 04:39 AM, Prafulla Wadaskar wrote:
>> -----Original Message-----
>> From: Sebastian Hesselbarth [mailto:sebastian.hesselbarth at gmail.com]
>> Sent: 17 January 2013 00:55
>> To: Sebastian Hesselbarth
>> Cc: u-boot at lists.denx.de; Rabeeh Khoury; Albert Aribaud; Prafulla
>> Wadaskar; Andy Fleming; Joe Hershberger; Daniel Stodden; Luka Perkov
>> Subject: [PATCH v3 00/10] Add Marvell Dove and SolidRun CuBox
>>
>> This patch set add support for the Marvell Dove 88AP510 SoC and
>> the SolidRun CuBox board based on that SoC. The patch set is divided
>> into the four following sections:
>
> Dear Sabastian
> First of all I express my thanks for initiating this Soc Support in the u-boot.
> Secondly I express my apology since I could not review then immediately.
> Please find my comments for entire patch series

Prafulla,

thanks for the detailled review! I will quickly give some comments about
some of your review questions and will do a full re-review later.

* About the orion/dove/kirkwood naming convention in linux kernel (that
this patch set picks up):

When I started hacking on linux and marvell SoCs in particular there
was support for orion5x, mv78xx0, kirkwood, and dove. As those SoCs
share a bunch of internal peripherals (you might know better) the
drivers shared between those SoCs were prepended with "orion_" that
should reflect the family of SoCs with that peripherals. I personally
find the naming collision with orion5x "unlucky" but that is the way
it is.

Then there came some effort to also mainline Armada 370/XP series that
of course also share "orion" drivers. The final name for all SoCs
(including "orion") was mvebu as it is the only common naming scheme
that fit (mvebu stands for some business unit within Marvell).

Plans for using just "marvell" or "mv" were rejected because there are
also pxa SoCs from marvell sitting in the kernel tree. They likely share
some of the internal peripherals but that is not reflected within kernel
tree, as there are two different hacking communities I guess.

New drivers that fit all SoCs (namely pinctrl and gpio, because linux
API changes required new drivers from scratch) get mvebu_, drivers that
still fit in "orion" get orion_, new soc specific ones get either
dove_, kirkwood_, armada_370_, or whatever the code name of that SoC is.
Old driver names remain untouched even if they are reused on Armada
370/XP.

Whenever I used orion_ in this patch set this refers to orion as in SoC
family, not orion5x that you might have been irritated with.

* About the kwboot/kwbimage patches:
u-boot has kwboot for kirkwood since ages, but the general functionality
to boot through UART boot mode also applies to above SoCs. I took
kwbimage to reflect that kwboot should be used for booting this image.
IMHO introducing new abbreviated image names like dvbimage will just
distract people from using the correct tool to boot it. kwboot will not
be renamed, will it?

* About the ordering of patches/patch sets:
>> (1) Patches 1-5:
>
> I have suggestions to break down your patches as mentioned below
>
>> Add support for the Dove SoC and related drivers. Where possible
>> drivers from Marvell Kirkwood are reused (mvsata, mvgbe), or
>> forked to allow more generic usage (SPI, GPIO). The SDHCI driver
>> is different and a new driver is added for it. The forked drivers
>> can also be reused on Kirkwood but that would have required patching
>> existing boards.
>>
>> (2) Patches 6-8:
>> Allow mvgbe to use the phylib API, add support for 88E1310 PHY and
>> allow Dove to use the driver.
>>
>> (3) Patch 9
>> Add the SolidRun CuBox as the first board based on Marvell Dove SoC.
>>
>> (4) Patch 10
>> Add support for different UART boot mode found on Dove.
>>
>> Changelog:
>> v1->v2:
>> - respect review comments by Luka Perkov
>> - fix commenting styles and typos
>> - add MAINTAINERS entry
>> - also update kwboot.1 manpage
>>
>> v2->v3:
>> - integrate kwboot patch from Daniel Stodden
>> - rebase on release v2013.01
>>
>> Sebastian Hesselbarth (10):
>>    ARM: dove: add support for Marvell Dove SoC
>>    GPIO: add gpio driver for Orion SoCs
>>    MMC: sdhci: Add support for dove sdhci
>>    SPI: Add Orion SPI driver
>>    block: mvsata: add dove include
>>    NET: phy: add 88E1310 PHY initialization
>>    NET: mvgbe: add phylib support
>>    NET: mvgbe: add support for Dove
>>    Boards: Add support for SolidRun CuBox
>>    tools: Add support for Dove to kwboot
>>
>>   MAINTAINERS                             |    4 +
>>   arch/arm/cpu/armv7/dove/Makefile        |   49 +++++
>>   arch/arm/cpu/armv7/dove/cpu.c           |  266
>> ++++++++++++++++++++++++++
>>   arch/arm/cpu/armv7/dove/dram.c          |  118 ++++++++++++
>>   arch/arm/cpu/armv7/dove/lowlevel_init.S |   83 ++++++++
>>   arch/arm/cpu/armv7/dove/mpp.c           |  318
>> +++++++++++++++++++++++++++++++
>>   arch/arm/cpu/armv7/dove/timer.c         |  176 +++++++++++++++++
>>   arch/arm/cpu/armv7/dove/usb.c           |  101 ++++++++++
>>   arch/arm/include/asm/arch-dove/config.h |  153 +++++++++++++++
>>   arch/arm/include/asm/arch-dove/cpu.h    |  204 ++++++++++++++++++++
>>   arch/arm/include/asm/arch-dove/dove.h   |   93 +++++++++
>>   arch/arm/include/asm/arch-dove/gpio.h   |   35 ++++
>>   arch/arm/include/asm/arch-dove/mpp.h    |  283
>
> Basic Dove support (one patch series)

Ok. Just to make sure, you want me to leave driver specific stuff, e.g.
orion_spi_* out of this patch set and re-add the calls within the
corresponding driver patches?

>>   drivers/gpio/Makefile                   |    1 +
>>   include/dove or mv_gpio.h                    |   64 +++++++
>>   drivers/gpio/dove or mv_gpio.c               |  167 ++++++++++++++++
>
> GPIO driver support for Dove

With the above comments about SoC naming, I'd like to stick with
either orion_ or mvebu_ if the basic driver framework can be reused
on Armada 370/XP (when they get mainline support in u-boot).

>>   drivers/spi/Makefile                    |    1 +
>>   drivers/spi/dove_spi.c                 |  217
>
> Spi driver support for dove

Ditto.

>>   drivers/block/mvsata_ide.c              |    2 +
>>   drivers/mmc/Makefile                    |    1 +
>>   drivers/mmc/dove_sdhci.c                |  101 ++++++++++
>>   drivers/net/mvgbe.c                     |   70 ++++++-
>>   drivers/net/mvgbe.h                     |    7 +
>>   drivers/net/phy/marvell.c               |   48 +++++
>
> Other driver supports for dove (explain supported drivers)

Ack.

>> +++++++++++++++++++++++++++
>>   board/solidrun/cubox/Makefile           |   45 +++++
>>   board/solidrun/cubox/cubox.c            |  141 ++++++++++++++
>>   board/solidrun/cubox/kwbimage.cfg       |   76 ++++++++
>>   boards.cfg                              |    1 +
>>   include/configs/cubox.h                 |  175 +++++++++++++++++
>
> Board support patches

Ack.

>>   doc/kwboot.1                            |   13 +-
> +++++++++++++++++++++
>>   tools/Makefile                          |    2 +
>>   tools/kwboot.c                          |   25 ++-
>
> Add dove support for boot tool.

Ack.

> FYI: I have gone through all patches in this series

Thanks for that again! I will flip through all the individual patches
later and give detailled comments for sure.

Regards,
   Sebastian