On Fri, 22 Dec 2023 at 23:04, Caleb Connolly <caleb.conno...@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.
>
> 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.
>
> --- 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.kulikow...@gmail.com>
> + * (C) Copyright 2023 Sumit Garg <sumit.g...@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?
>
> 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 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.
> >
> >>>
> >>>> 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.
>
> 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
