[PATCH 0/7] Add SE HMBSC board support

[Caleb Connolly]

On 26/12/2023 10:27, Sumit Garg wrote:
> On Fri, 22 Dec 2023 at 23:04, Caleb Connolly <caleb.connolly at linaro.org> wrote:
>>
>> [snip]
>>
>>>>>> Sumit, could you rebase this series on my generic board support? [1] in
>>>>>> it's current form this series conflicts, and includes some of the major
>>>>>> anti-patterns I'm trying to move away from in mach-snapdragon.
>>>>>
>>>>> Although, I haven't gone through your series but I was expecting those
>>>>> conflicts. Let's work together to make this series compatible on top
>>>>> of yours.
>>>>>
>>>>>>
>>>>>> You should not have to introduce a new CONFIG_TARGET_XYZ variable, and
>>>>>> from what I can tell you shouldn't even need to add the board/schneider
>>>>>> directory at all, you can just set the following in your defconfig:
>>>>>>
>>>>>> CONFIG_SYS_BOARD="dragonboard410c"
>>>>>
>>>>> This is simply a misnomer, its HMIBSC board. I suppose we should
>>>>> rather separate the SoC specific bits into mach-snapdragon and let
>>>>> different boards use them.
>>>>
>>>> Is there any reason why you can't just use the existing db410c board
>>>> code as-is? I don't see why you want to duplicate it.
>>>
>>> I don't want to duplicate it but rather we should make the common
>>> parts as part of arch/arm/mach-snapdragon/ and let derivative boards
>>> use them. The HMIBSC board is an industrial board which doesn't need
>>> any generic distro boot but rather FIT booting with OTA updates via
>>> RAUC [1] along with U-boot environment protection. Doesn't that make
>>> it different from db410c?
>>
>> Right, your series does this board specific configuration in the board
>> header file (include/configs/hmibsc.h) and in the defconfig.
>>
>> The actual board code in board/schneider/hmibsc/hmibsc.c is directly
>> copied from board/qualcomm/dragonboard410c/dragonboard410c.c with just
>> the copyright changed, the serial number code removed, and a style fix
>> to a comment. Ignoring for a moment the copyright issue which would
>> definitely need to be fixed,
> 
> Ah, I should have retained the copyright.
> 
>> what I'm suggesting that you do is just use
>> the board code from db410c.
> 
> This is just initial board support, a direct copy would just limit any
> further board support extensions.

Could you elaborate a little? If there are technical reasons to 
duplicate the board code then they should be explained in the commit 
message - and we can avoid this whole back and forth.
> 
>>
>> The way I have designed the generic board support is so that two similar
>> boards can share the same board code but with different config headers,
>> from what I can tell this would work just fine for you.
> 
> As I have mentioned earlier, the proper way to share code among boards
> is to have something like following:
> 
> arch/arm/mach-snapdragon/board_apq8016.c
> 
> You can take examples from arch/arm/mach-meson/board-*.c. If you don't
> want to do it as part of your series then let me know I will include
> it in this series.

I'm open to moving some of the code from board/qualcomm/dragonboard410c/ 
back to mach-snapdragon, I think my requirements for moving some feature 
code back are as follows:

  * Multiple unrelated Qualcomm boards use it
  * There is no clear path forward for upstream DT bindings
  * (and/therefore) Duplicating the function across boards is silly

Then we should definitely move it to mach-snapdragon.

I again want to be clear that this kind of board specific code IS a 
hack, it gets things working where we don't have a proper DT-based 
solution yet.

I know doing things properly is more effort, but I think with just a bit 
of effort we can find much better solutions that let us get rid of this 
board specific code all-together.
> 
>>
>> --- board/qualcomm/dragonboard410c/dragonboard410c.c
>> +++ board/schneider/hmibsc/hmibsc.c
>> @@ -2,5 +2,5 @@
>>   /*
>> - * Board init file for Dragonboard 410C
>> + * Board init file for SE HMIBSC
>>    *
>> - * (C) Copyright 2015 Mateusz Kulikowski <mateusz.kulikowski at gmail.com>
>> + * (C) Copyright 2023 Sumit Garg <sumit.garg at linaro.org>
>>    */
>> @@ -8,3 +8,2 @@
>>   #include <button.h>
>> -#include <common.h>
>>   #include <cpu_func.h>
>> @@ -137,7 +136,2 @@
>>   {
>> -       char serial[16];
>> -
>> -       memset(serial, 0, 16);
>> -       snprintf(serial, 13, "%x", msm_board_serial());
>> -       env_set("serial#", serial);
>>          return 0;
>> @@ -145,3 +139,4 @@
>>
>> -/* Fixup of DTB for Linux Kernel
>> +/*
>> + * Fixup of DTB for Linux Kernel
>>    * 1. Fixup installed DRAM.
>> @@ -165,3 +160,2 @@
>>
>> -
>>          if (!eth_env_get_enetaddr("btaddr", mac)) {
>> @@ -169,5 +163,6 @@
>>
>> -/* The BD address is same as WLAN MAC address but with
>> - * least significant bit flipped.
>> - */
>> +               /*
>> +                * The BD address is same as WLAN MAC address but with
>> +                * least significant bit flipped.
>> +                */
>>                  mac[0] ^= 0x01;
>>
>>>
>>> [1] https://rauc.io/
>>>
>>>>
>>>> The only reason the db410c and db820c have their board code is because
>>>> they're old platforms and already supported. For adding new support
>>>> there needs to be some very strong justification to have board-specific
>>>> C code.
>>>>
>>>> I think it would be nice to make the db410c code go away, or be toggled
>>>> at runtime, probably most of it will just work and not break any other
>>>> boards anyway. The db820c code is just part of what should be in the
>>>> pinctrl driver...
>>>>
>>>> Let's move away from this old model and towards having more generic
>>>> U-Boot images. This will snowball towards making future bringup even easier.
>>>
>>> As I have mentioned in the other thread, we should give alternatives
>>> to the board code as well. How would you handle the following?
>>>
>>> - Fastboot mode entry on a button press.
>>
>> This can be nicely handled through CONFIG_AUTOBOOT. I currently use
>> AUTOBOOT_STOP_STR but that's quite limited (only works for the power
>> button with "\r"). Probably the right solution here is to use
>> CONFIG_AUTOBOOT_USE_MENUKEY and introduce support for
>> CONFIG_AUTOBOOT_MENUBUTTON to allow specifying a named button instead of
>> an ASCII code. One would then configure "menucmd" in the U-Boot
>> environment to launch fastboot.
> 
> Won't this break existing users who relied on volume buttons to flash
> their board and rather have to purchase a debug uart?

I don't think so(???). This would highly depend on the device and 
configuration. As a developer you should have ways to recover from a 
borked U-Boot build, and you should make sure that any binary releases 
you make don't result in bricked boards.
> 
>>
>> This lets us drop all the weird non-standard keycode handling in board
>> code and just configure it in the defconfig instead. I plan to implement
>> this eventually but would for sure appreciate it if someone beat me to it.
>>> - Configure MAC address for network support.
>>
>> I believe you mentioned elsewhere some board with the MAC address on an
>> i2c EEPROM? The NVMEM framework solves exactly this issue, and U-Boot
>> already supports it, just add support to your ethernet driver to
>> retrieve its MAC address via NVMEM.
> 
> Good to know that.
> 
>>> - Setup board serial number.
>>
>> Same as above, the quick and dirty way to go would be to have
>> mach-snapdragon check for an alias defined in DT and expect that alias
>> to be an NVMEM cell which contains the serial number. On the Android
>> phones this is set in the kernel cmdline from ABL so I would probably
>> add some code to mach-snapdragon to check the bootargs for one.
> 
> And what about boards where U-Boot starts as the first stage
> boot-loader? Is there corresponding DT binding?

I really don't know. Having some code to read the MMC serial number and 
use it as the fallback board serial sounds reasonable to me.
> 
>>
>>>
>>> I suppose we don't need #ifdefry in the
>>> arch/arm/mach-snapdragon/board.c file, right?
>>
>> Nope, we aren't that limited on code size, and the runtime overhead of a
>> few extra branches is really not significant. If we do need to start
>> caring about either of those things then I would rather do this on a
>> per-feature basis than per-board.
> 
> The examples I gave were only limited to what are currently used by
> Qcom boards. But we should be open to future board support extensions
> too.

Absolutely!
> 
>>>
>>>>>
>>>>>> CONFIG_SYS_CONFIG_NAME="hmibsc"
>>>>>>
>>>>>> This will use the db410c board code (which yours is just a copy/paste of
>>>>>> from what I can tell) and your custom include/configs/hmibsc.h header.
>>>>>>
>>>>>> The addresses set in your environment file should be allocated
>>>>>> automatically at runtime too (see the ("mach-snapdragon: dynamic load
>>>>>> addresses") patch).
>>>>>>
>>>>>> You should also switch to an upstream board DTS based on my series, and
>>>>>> drop the "-uboot.dtsi" file.
>>>>>
>>>>> Unfortunately, currently there isn't any upstream DTS for this board
>>>>> but I will check with the SE team regarding their plans. Until then we
>>>>> have to use a U-Boot specific DTS file.
>>>>
>>>> In that case, perhaps we can take whatever DTS they're using in
>>>> production, or a version of it, and at least split the U-Boot changes
>>>> (if any) out into a separate file as I've done with the other platforms.
>>>> This way we stay consistent and can keep track of what U-Boot specific
>>>> DTS changes we need.
>>>
>>> The first step for that is to land that DTS file upstream in the Linux
>>> kernel and then we should make a switch. The middle ground here won't
>>> be maintainable.
>>>
>>>>
>>>> I guess this is all new territory for us, but imho if we're adding
>>>> support for a board that doesn't have an upstream DTS, we should still
>>>> follow the same model, open to input here.
>>>
>>> With the DT rebasing tree, we actually want to make these bits clear.
>>> Either the board support is fully upstream and then we make a switch
>>> to upstream or you can have a u-boot specific DTS subset compliant
>>> with upstream bindings while upstreaming is in progress. There is
>>> always ABI risk involved for using half baked board DTS files.
>>
>> The DTS file submitted in your series is a copy paste of
>> dragonboard410c.dts with the serial port adjusted, the LEDs removed, and
>> the copyright changed...
>>
>> This will make it the only board still using the totally made up DTS
>> style that all the Qualcomm boards used to use, while everything else is
>> standardised on upstream (meaning they #include the SoC and PMIC dtsi
>> files).
>>
>> It doesn't make a difference whether you put the board DT in dt-rebasing
>> or if it just goes in U-Boot, it should be a DT derived from upstream
>> not some hand-crafted thing which will never be supported. Just copy
>> apq8016-sbc.dts when rebasing on my series and it will be fine.
> 
> The major bit that I was concerned about previously is if people start
> to pass that u-boot specific DT to the kernel directly. But with
> dt-rebasing at least we can distinguish among them. So I am fine to
> use the format that you have described.

Ok, great!
> 
>>
>> That said, as I'm writing this I've realised that the pinctrl-apq8016
>> driver is missing a patch to use the upstream GPIO naming. I'll fix this
>> in the next revision of the qualcomm generic support series, but just a
>> heads up if you run into that.
> 
> Good to know.
> 
> -Sumit

-- 
// Caleb (they/them)