[PATCH v10 00/12] Actions S700 SoC support

Fri Apr 17 18:42:19 CEST 2020

On 17/04/2020 13:44, Tom Rini wrote:
> On Fri, Apr 17, 2020 at 01:34:36PM +0100, André Przywara wrote:
>> 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?
> 
> It doesn't quite work fine, and DISTRO_DEFAULTS is the case where it
> shows up badly (which is why everyone else does imply/select).
> 
>>> 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.
> 
> The problem is that Kconfig doesn't work that way.  All of the SYS_foo
> stuff we have in board/SoC Kconfig files has some unfortunate
> side-effects that Yamada-san has noted elsewhere.

Ah, OK, thanks for the heads up. I just found those examples, but didn't
know that they are actually considered somewhat broken.

> The long list of
> "default ... if ...." aren't ideal, but my hope is that by consolidating
> who wants/needs what values we can first come up with better defaults
> and then perhaps come up with a better tool for everyone (how do you
> manage kernel defconfigs is its own problem).

Fair enough, if those "default ... if" sequences are the way to go, then
so be it.
I was just hoping there would be a cleaner way of expressing: "this
*platform* relies on/always sets this option". Unfortunately both select
and imply seem to come with their own set of issues, at least for
certain kind of options.

Cheers,
Andre