[PATCH v10 00/12] Actions S700 SoC support

[André Przywara]

On 17/04/2020 13:11, Tom Rini wrote:
> On Fri, Apr 17, 2020 at 08:31:38PM +0900, Masahiro Yamada wrote:
>> On Fri, Apr 17, 2020 at 6:07 PM André Przywara <andre.przywara at arm.com> wrote:
>>>
>>> On 17/04/2020 04:05, Tom Rini wrote:
>>>
>>> (adding Masahiro for Kconfig)
>>>
>>>> On Mon, Apr 06, 2020 at 05:58:19PM +0530, Amit Singh Tomar wrote:
>>>>
>>>>> This adds Cubieboard7[1] support based on Action Semi's S700 SoC[2], It's Quad-core ARMv8 SoC
>>>>> with Cortex-A53 cores. Peripheral like UART seems to be compatible with S900 SoC(basic support
>>>>> for it is alreay present in u-boot).
>>>>>
>>>>> This series(v10) takes care the commments provided by Mani and patches 04/12, 07/12 and 12/12
>>>>> has been changed to address those comments.
>>>>>
>>>>> Previous series(v9) fixes a Bug that breaks bubblegum96 board boot(reported by Mani). It was
>>>>> due to fact that driver data read is not proper in the clock driver. There are changes in
>>>>> patch 06/12 to fix it.
>>>>>
>>>>> Series(v8) removes the SoC specific include instead just uses owl-common. For this
>>>>> patch 01/12 and 09/12 changes a bit.
>>>>>
>>>>> Series(v7) fixes a serious Bug that breaks S900, it was there since v5.Thanks to Andre
>>>>> for pointing it out.
>>>>>
>>>>> Series(v6)[3] does following changes:
>>>>>
>>>>> * [PATCH v5 06/11] becomes [PATCH v6 03/11]
>>>>> * [PATCH v5 03/11] becomes [PATCH v6 04/11]
>>>>> * Introduce a new patch to move defconfig options to Kconfig which is [PATCH v6 10/12]
>>>>>
>>>>> Series(v5)[4] just re-orders the patches so that U-BOOT(with bubblegum96_defconfig) builds
>>>>> after every patch of the series(suggested by Andre).
>>>>>
>>>>> S700 support is tested[5] on Cubieboard7-lite board and S900 support is just compiled tested.
>>>>>
>>>>> This patch series can be tested using below tree:
>>>>> https://github.com/Atomar25/u-boot/commits/s700_v10
>>>>>
>>>>> [1]: http://www.cubietech.com/product-detail/cubieboard7/
>>>>> [2]: http://www.actions-semi.com/en/productview.aspx?id=225
>>>>> [3]: http://u-boot.10912.n7.nabble.com/PATCH-v6-00-12-Actions-S700-SoC-support-td403562.html#a403567
>>>>> [4]: http://u-boot.10912.n7.nabble.com/PATCH-v5-00-11-Actions-S700-SoC-support-td402752.html#a402762
>>>>> [5]: https://paste.ubuntu.com/p/TbBtk5dPGS/
>>>>>
>>>>> Amit Singh Tomar (12):
>>>>>   arm: actions: Add common framework for Actions Owl Semi SoCs
>>>>>   arm: actions: rename sysmap-s900 to sysmap-owl
>>>>>   serial: actions: add compatible string
>>>>>   arm: dts: sync dts for Action Semi S900
>>>>>   arm: dts: actions: s900: add u-boot specific dtsi file
>>>>>   clk: actions: Add common clock driver
>>>>>   arm: actions: add S700 SoC device tree
>>>>>   arm: dts: actions: s700: add u-boot specific dtsi file
>>>>>   arm: add support Actions Semi S700
>>>>>   actions: Move defconfig options to Kconfig
>>>>>   arm: add Cubieboard7 board support
>>>>>   doc: boards: add Cubieboard7 documentation
>>>>
>>>> A few problems.  First, "actions: Move defconfig options to Kconfig"
>>>> breaks a large number of boards including p2371-2180 in one way and
>>>> libretech_all_h5_cc_h5 (along with lots of other sunxi platforms) in a
>>>> different but related way.
>>>
>>> (Masahiro: it's about this patch:
>>> https://lists.denx.de/pipermail/u-boot/2020-April/405672.html)
>>>
>>> Tom, many thanks for the heads up, I can confirm the problem, but am
>>> totally clueless as of *why* this happens:
>>> The changes in this patch add options to arch/arm/mach-owl/Kconfig, and
>>> are totally contained inside an "if ARCH_OWL .. endif" clamp, so how
>>> could this even affect other platforms (which are clearly not defining
>>> ARCH_OWL)?
>>>
>>
>>
>> scripts/diffconfig in Linux is useful to
>> see how the resulted .config has changed.
>>
>> This is the before/after diff of p2371-2180.
>>
>>
>> -BOOTCOMMAND "run distro_bootcmd"
>> -BOOTP_PXE y
>> -BOOTP_PXE_CLIENTARCH 0x16
>> -CMD_EXT4_WRITE y
>> -EXT4_WRITE y
>> -FAT_WRITE y
>> -FS_FAT_MAX_CLUSTSIZE 65536
>> -MENU y
>>  CMD_DHCP y -> n
>>  CMD_EXT2 y -> n
>>  CMD_EXT4 y -> n
>>  CMD_FAT y -> n
>>  CMD_FS_GENERIC y -> n
>>  CMD_MII y -> n
>>  CMD_PART y -> n
>>  CMD_PING y -> n
>>  CMD_PXE y -> n
>>  CMD_SYSBOOT y -> n
>>  DISTRO_DEFAULTS y -> n
>>  DOS_PARTITION y -> n
>>  ENV_VARS_UBOOT_CONFIG y -> n
>>  FS_EXT4 y -> n
>>  FS_FAT y -> n
>>  HUSH_PARSER y -> n
>>  SUPPORT_RAW_INITRD y -> n
>>  USB_STORAGE y -> n
>>  USE_BOOTCOMMAND y -> n
>>
>>
>>
>> It turned off DISTRO_DEFAULTS.
>>
>> The menuconfig help shows
>> it now depends on 'ARM && ARCH_OWL'.
>>
>> Presumably Kconfig was confused
>> by DISTRO_DEFAULTS being defined
>> multiple times.

It is, but only for ARCH_OWL, where it actually works as expected. I
don't get how the additional listing of just DISTRO_DEFAULTS (without a
type!) *guarded by if ARCH_OWL* would affect other platforms.

Besides, we do this all over the place for stuff like IDENT_STRING,
SYS_CLK_FREQ, and, most prominently SYS_CONFIG_NAME, SYS_SOC and
SYS_BOARD. And there it works fine. So what is the difference here?

> Ah, right.  And they shouldn't be defined twice, it should be imply'd
> under ARCH_OWL (and the rest should be in the defconfigs, no re-listed
> in arch/arm/mach-owl/Kconfig).  Thanks!

So yes, the fix is relatively easy (Amit actually had it this way
before). It's just that I actually recommended this approach here to
Amit, to avoid putting platform specific defaults into generic Kconfig
files (like we do for sunxi at the moment).
Conceptually I find it cleaner to gather platform specific defaults in
the platform Kconfig instead of spreading this out all over the tree.

Cheers,
Andre.