[U-Boot] [PATCH v3 3/4] rockchip: Add basic support for phyCORE-RK3288 SoM based carrier board

[Simon Glass]

Hi Wadim,

On 13 June 2017 at 03:10, Wadim Egorov <w.egorov at phytec.de> wrote:
>
>
> Am 13.06.2017 um 01:51 schrieb Simon Glass:
>> On 12 June 2017 at 03:59, Wadim Egorov <w.egorov at phytec.de> wrote:
>>> The phyCORE-RK3288 is a SoM (System on Module) containing a RK3288 SoC.
>>> The module can be connected to different carrier boards.
>>> It can be also equipped with different RAM, SPI flash and eMMC variants.
>>> The Rapid Development Kit option is using the following setup:
>>>
>>>   - 1 GB DDR3 RAM (2 Banks)
>>>   - 1x 4 KB EEPROM
>>>   - DP83867 Gigabit Ethernet PHY
>>>   - 16 MB SPI Flash
>>>   - 4 GB eMMC Flash
>>>
>>> Add basic support for the PCM-947 carrier board, a RK3288 based development
>>> board made by PHYTEC. This board works in a combination with
>>> the phyCORE-RK3288 System on Module.
>>>
>>> Signed-off-by: Wadim Egorov <w.egorov at phytec.de>
>>> ---
>>> Changes in v3:
>>> - Added u-boot,dm-pre-reloc to regulators subnode
>>>   (PMIC failed to bind because of the missing regulators node)
>>> - Removed the hacky PMIC setup from phycore_init
>>> - Use CONFIG_TARGET_PHYCORE_RK3288 to cover board-specific stuff
>>>
>>>   Things get ugly when I use of_machine_is_compatible().
>>>   Boards whith CONFIG_SPL_OF_PLATDATA disabled would compile in
>>>   of_machine_is_compatible() which checks for my board.
>>>   phycore_init() calls rk818_spl_configure_*() which depends on
>>>   CONFIG_SPL_POWER_SUPPORT. So this will cause undefined reference linker
>>>   warnings for every other rockchip board.
>>>   That's the reason why I moved back to CONFIG_TARGET_PHYCORE_RK3288.
>>>
>>> ---
>>>  arch/arm/dts/Makefile                        |   1 +
>>>  arch/arm/dts/rk3288-phycore-rdk.dts          | 294 ++++++++++++++++
>>>  arch/arm/dts/rk3288-phycore-som.dtsi         | 506 +++++++++++++++++++++++++++
>>>  arch/arm/mach-rockchip/rk3288-board-spl.c    |  35 ++
>>>  arch/arm/mach-rockchip/rk3288/Kconfig        |  10 +
>>>  board/phytec/phycore_rk3288/Kconfig          |  15 +
>>>  board/phytec/phycore_rk3288/MAINTAINERS      |   6 +
>>>  board/phytec/phycore_rk3288/Makefile         |   8 +
>>>  board/phytec/phycore_rk3288/phycore-rk3288.c |   8 +
>>>  configs/phycore-rk3288_defconfig             |  70 ++++
>>>  include/configs/phycore_rk3288.h             |  23 ++
>>>  11 files changed, 976 insertions(+)
>>>  create mode 100644 arch/arm/dts/rk3288-phycore-rdk.dts
>>>  create mode 100644 arch/arm/dts/rk3288-phycore-som.dtsi
>>>  create mode 100644 board/phytec/phycore_rk3288/Kconfig
>>>  create mode 100644 board/phytec/phycore_rk3288/MAINTAINERS
>>>  create mode 100644 board/phytec/phycore_rk3288/Makefile
>>>  create mode 100644 board/phytec/phycore_rk3288/phycore-rk3288.c
>>>  create mode 100644 configs/phycore-rk3288_defconfig
>>>  create mode 100644 include/configs/phycore_rk3288.h
>> Reviewed-by: Simon Glass <sjg at chromium.org>
>>
>> (assuming we need to set up the battery / charger in SPL)
>>
>> But can you please check the compatible string to decide whether you
>> need to do the phytec stuff, instead of an #ifdef on the target? I
>> suppose if it increase code size it is OK to have both, but the #ifdef
>> should be a size optimisation, not the main method of selecting the
>> code Is there a way to describe what is needed in the DT?
>
> I can do something like that if it is okay for you:
>
> static int phycore_init(void)
> {
>         ...
> #if defined (CONFIG_SPL_POWER_SUPPORT)
>         ret = rk818_spl_configure_usb_input_current(pmic, 2000);
> #endif
>         ...
> }
>
> /* in board init */
> #if !defined(CONFIG_SPL_OF_PLATDATA)
>         if (of_machine_is_compatible("phytec,rk3288-phycore-som"))
>                 ret = phycore_init();
> #endif

This seems OK.

>
>
> Otherwise, I will just add of_machine_is_compatible() covered by
> CONFIG_TARGET_PHYCORE_RK3288.
>
> Sorry, what do you mean with your last question?

It should be possible for the code to build and work without any
CONFIG_TARGET_... #ifdefs in the code. I think it is OK to add the
#ifdefs if needed for code size, but without them the code should
still do the right thing on all platforms. In other words, use
if...then for logic, #ifdef...#endif for code-size optimisation.

Regards,
Simon