Hi Sumit, On 03/01/2024 13:07, Sumit Garg wrote: > In order to avoid duplication of APQ8016 derived boards specific code, > move common bits like USB pinctrl and fastboot to board-apq8016.c file. > This will allow future board support like SE HMIBSC board etc to reuse it.
You still haven't demonstrated a justification for having the HMIBSD board NOT simply point to board/qualcomm/dragonboard410c. Do you have a branch with additional features where it clearly makes sense to have a separate board directory? > > Signed-off-by: Sumit Garg <[email protected]> > --- > > This patch is based on Caleb's Qcom common target branch [1]. As per > Caleb's comments here [2], there is no scope for people to add board > code for new board support. And even people can't reuse existing board > code like for db410c for further new board support but rather they have > to make db410c generic first. > > I fear this would be couter productive and motivate OEM vendors (which > aren't Qcom reference designs) to keep their board support code > downstream. This is the same argument made by those defending mach-* code in Linux. What I'm advocating for here is that we spend just a little more time doing things right, so that when the next vendor, or next gen board comes along we aren't copy/pasting board files. > IMO, the approach should be to allow board code reuse which > this patch is all about. Board code reuse is exactly what I want, just that we do our due diligence in not blindly putting code behind arbitrary compile-time configuration. > The next steps should be to make the underlying > board features to rather come from DT but that as you know requires > bindings to be accepted first. We can have non-standard DT if we *need* to, I would prefer that over this board code because then there's at least an obvious path forwards. In fact this is exactly what the below USB role switching code does, I already put the work in to make it generic (before it literally found the GPIO device and set specific GPIO pins). Below I have outlined the changes that would be necessary to have the functionality here built in for all mach-snapdragon boards, so this whole SOC_APQ8016 thing can be dropped. It's ridiculous to gate this code behind a particular SoC when none of it is SoC specific. > > Tom, Simon, > > Is there any U-Boot policy in regards to board code going forward? Are > we moving in a direction to get rid of most board specific stubs from > generic U-Boot code? > > [1] > https://source.denx.de/u-boot/custodians/u-boot-snapdragon/-/commits/b4%2Fqcom-common-target > [2] > https://lore.kernel.org/all/[email protected]/ > > arch/arm/mach-snapdragon/Kconfig | 5 ++ > arch/arm/mach-snapdragon/Makefile | 1 + > arch/arm/mach-snapdragon/board-apq8016.c | 64 +++++++++++++++++++ > .../dragonboard410c/dragonboard410c.c | 62 ------------------ > configs/dragonboard410c_defconfig | 1 + > 5 files changed, 71 insertions(+), 62 deletions(-) > create mode 100644 arch/arm/mach-snapdragon/board-apq8016.c > > diff --git a/arch/arm/mach-snapdragon/Kconfig > b/arch/arm/mach-snapdragon/Kconfig > index 96e44e2c549..8069a89bf78 100644 > --- a/arch/arm/mach-snapdragon/Kconfig > +++ b/arch/arm/mach-snapdragon/Kconfig > @@ -3,6 +3,11 @@ if ARCH_SNAPDRAGON > config SYS_SOC > default "snapdragon" > > +config SOC_APQ8016 > + bool "APQ8016" > + help > + Select this if your SoC is an APQ8016 > + > config SYS_VENDOR > default "qualcomm" > > diff --git a/arch/arm/mach-snapdragon/Makefile > b/arch/arm/mach-snapdragon/Makefile > index 857171e593d..7d210e6d872 100644 > --- a/arch/arm/mach-snapdragon/Makefile > +++ b/arch/arm/mach-snapdragon/Makefile > @@ -3,3 +3,4 @@ > # (C) Copyright 2015 Mateusz Kulikowski <[email protected]> > > obj-y += board.o > +obj-$(CONFIG_SOC_APQ8016) += board-apq8016.o > diff --git a/arch/arm/mach-snapdragon/board-apq8016.c > b/arch/arm/mach-snapdragon/board-apq8016.c > new file mode 100644 > index 00000000000..bb9a3a9f0fb > --- /dev/null > +++ b/arch/arm/mach-snapdragon/board-apq8016.c > @@ -0,0 +1,64 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Board init file for APQ8016 SoC derived boards > + * > + * (C) Copyright 2015 Mateusz Kulikowski <[email protected]> > + */ > + > +#include <button.h> > +#include <dm.h> > +#include <dm/pinctrl.h> > +#include <env.h> > +#include <init.h> > +#include <usb.h> > +#include <fdt_support.h> > + > +int board_usb_init(int index, enum usb_init_type init) This function can trivially be made generic, in such a way that it could be done for all mach-snapdragon devices: > +{ > + struct udevice *usb; > + int ret = 0; > + > + /* USB device */ > + ret = device_find_global_by_ofnode(ofnode_path("/soc/usb"), &usb); Instead do: ret = uclass_find_device_by_seq(UCLASS_USB, index, &usb); > + if (ret) { > + printf("Cannot find USB device\n"); > + return ret; > + } Here we must check to see if the "device" pinctrl exists, use this as a litmus test to see if this board needs pinctrl state changes ret = dev_read_stringlist_search(usb, "pinctrl-names", "device"); /* No "device" pinctrl state, so just bail */ if (ret < 0) return 0; > + > + /* Select "default" or "device" pinctrl */ > + switch (init) { > + case USB_INIT_HOST: > + pinctrl_select_state(usb, "default"); > + break; > + case USB_INIT_DEVICE: > + pinctrl_select_state(usb, "device"); > + break; > + default: > + debug("Unknown usb_init_type %d\n", init); > + break; > + } > + > + return 0; > +} > + > +/* Check for vol- button - if pressed - stop autoboot */ > +int misc_init_r(void) As I said before, this code only exists because U-Boot's "autoboot" implementation hasn't been updated with good button support, this is also a fairly simple fix and would benefit all the boards, not just Qualcomm. If you really don't have the time of experience to implement such a feature, and this is really such a blocker for your board support, then that is a totally different class of issue which we can find a solution to. > +{ > + struct udevice *btn; > + int ret; > + enum button_state_t state; > + > + ret = button_get_by_label("vol_down", &btn); > + if (ret < 0) { > + printf("Couldn't find power button!\n"); > + return ret; > + } > + > + state = button_get_state(btn); > + if (state == BUTTON_ON) { > + env_set("preboot", "setenv preboot; fastboot 0"); > + printf("vol_down pressed - Starting fastboot.\n"); > + } > + > + return 0; > +} > diff --git a/board/qualcomm/dragonboard410c/dragonboard410c.c > b/board/qualcomm/dragonboard410c/dragonboard410c.c > index 2a502c7c284..f7c6a6cbc0a 100644 > --- a/board/qualcomm/dragonboard410c/dragonboard410c.c > +++ b/board/qualcomm/dragonboard410c/dragonboard410c.c > @@ -5,78 +5,16 @@ > * (C) Copyright 2015 Mateusz Kulikowski <[email protected]> > */ > > -#include <button.h> > #include <common.h> > -#include <cpu_func.h> > #include <dm.h> > -#include <dm/pinctrl.h> > #include <env.h> > #include <init.h> > #include <net.h> > -#include <usb.h> > -#include <asm/cache.h> > -#include <asm/global_data.h> > -#include <asm/gpio.h> > #include <fdt_support.h> > #include <linux/delay.h> > > #include "misc.h" > > -DECLARE_GLOBAL_DATA_PTR; > - > -#define USB_HUB_RESET_GPIO 2 > -#define USB_SW_SELECT_GPIO 3 > - > -int board_usb_init(int index, enum usb_init_type init) > -{ > - struct udevice *usb; > - int ret = 0; > - > - /* USB device */ > - ret = device_find_global_by_ofnode(ofnode_path("/soc/usb"), &usb); > - if (ret) { > - printf("Cannot find USB device\n"); > - return ret; > - } > - > - /* Select "default" or "device" pinctrl */ > - switch (init) { > - case USB_INIT_HOST: > - pinctrl_select_state(usb, "default"); > - break; > - case USB_INIT_DEVICE: > - pinctrl_select_state(usb, "device"); > - break; > - default: > - debug("Unknown usb_init_type %d\n", init); > - break; > - } > - > - return 0; > -} > - > -/* Check for vol- button - if pressed - stop autoboot */ > -int misc_init_r(void) > -{ > - struct udevice *btn; > - int ret; > - enum button_state_t state; > - > - ret = button_get_by_label("vol_down", &btn); > - if (ret < 0) { > - printf("Couldn't find power button!\n"); > - return ret; > - } > - > - state = button_get_state(btn); > - if (state == BUTTON_ON) { > - env_set("preboot", "setenv preboot; fastboot 0"); > - printf("vol_down pressed - Starting fastboot.\n"); > - } > - > - return 0; > -} > - > int qcom_late_init(void) > { > char serial[16]; > diff --git a/configs/dragonboard410c_defconfig > b/configs/dragonboard410c_defconfig > index 453cb9d04c7..23024066f82 100644 > --- a/configs/dragonboard410c_defconfig > +++ b/configs/dragonboard410c_defconfig > @@ -3,6 +3,7 @@ CONFIG_SYS_BOARD="dragonboard410c" > CONFIG_COUNTER_FREQUENCY=19000000 > CONFIG_ENABLE_ARM_SOC_BOOT0_HOOK=y > CONFIG_ARCH_SNAPDRAGON=y > +CONFIG_SOC_APQ8016=y > CONFIG_TEXT_BASE=0x8f600000 > CONFIG_SYS_MALLOC_LEN=0x802000 > CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y -- // Caleb (they/them)
