| Message ID | 20231218072428.1802969-1-sumit.garg@linaro.org |
|---|---|
| Headers | show |
| Series | Add SE HMBSC board support | expand |
Hi Sumit, On Mon, 18 Dec 2023 at 00:24, Sumit Garg <sumit.garg@linaro.org> wrote: > > SE HMIBSC board is based on Qcom APQ8016 SoC. One of the major Could you please add a doc/ file for this board and explain how to build it and how to run U-Boot on it? > difference from db410c is serial port where HMIBSC board uses UART1 as > the debug console with an RS232 port, patch #1 - #3 adds corresponding > driver support. > > Patch #4 adds main HMIBSC board specific bits, features: > - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306) > - 2GiB RAM > - 64GiB eMMC, SD slot > - WiFi and Bluetooth > - 2x Host, 1x Device USB port > - HDMI > - Discrete TPM2 chip over SPI > > Patch #5 - #7 enables specific board features like RAUC support, > environment protection and USB networking support. > > This patch series is based on top of Qcom maintainer tree [1] + the latest > PMIC patch-set [2]. Feedback is very much welcome. > > [1] https://source.denx.de/u-boot/custodians/u-boot-snapdragon/-/commits/u-boot-qcom-next?ref_type=heads > [2] https://patchwork.ozlabs.org/project/uboot/list/?series=385322 > > Sumit Garg (7): > clk: apq8016: Add support for UART1 clocks > serial_msm: Add support for RS232 GPIOs > serial_msm: Enable RS232 flow control > board: Add SE HMIBSC board support > hmibsc: Enable RAUC support > hmibsc: enable U-Boot Environment variables protection > hmibsc: Enable LAN75XX USB ethernet driver > > arch/arm/dts/Makefile | 1 + > arch/arm/dts/hmibsc-uboot.dtsi | 43 +++++++ > arch/arm/dts/hmibsc.dts | 188 +++++++++++++++++++++++++++++ > arch/arm/mach-snapdragon/Kconfig | 18 +++ > arch/arm/mach-snapdragon/Makefile | 1 + > board/schneider/hmibsc/Kconfig | 15 +++ > board/schneider/hmibsc/MAINTAINERS | 6 + > board/schneider/hmibsc/Makefile | 5 + > board/schneider/hmibsc/hmibsc.c | 179 +++++++++++++++++++++++++++ > board/schneider/hmibsc/hmibsc.env | 11 ++ > configs/hmibsc_defconfig | 79 ++++++++++++ > drivers/clk/qcom/clock-apq8016.c | 44 ++++++- > drivers/serial/serial_msm.c | 23 +++- > drivers/usb/host/Kconfig | 1 + > include/configs/hmibsc.h | 59 +++++++++ > 15 files changed, 665 insertions(+), 8 deletions(-) > create mode 100644 arch/arm/dts/hmibsc-uboot.dtsi > create mode 100644 arch/arm/dts/hmibsc.dts > create mode 100644 board/schneider/hmibsc/Kconfig > create mode 100644 board/schneider/hmibsc/MAINTAINERS > create mode 100644 board/schneider/hmibsc/Makefile > create mode 100644 board/schneider/hmibsc/hmibsc.c > create mode 100644 board/schneider/hmibsc/hmibsc.env > create mode 100644 configs/hmibsc_defconfig > create mode 100644 include/configs/hmibsc.h > > -- > 2.34.1 > Regards, Simon
Hi Simon, On Mon, 18 Dec 2023 at 20:32, Simon Glass <sjg@chromium.org> wrote: > > Hi Sumit, > > On Mon, 18 Dec 2023 at 00:24, Sumit Garg <sumit.garg@linaro.org> wrote: > > > > SE HMIBSC board is based on Qcom APQ8016 SoC. One of the major > > Could you please add a doc/ file for this board and explain how to > build it and how to run U-Boot on it? Ah I forgot to add that since the build/boot instructions are quite similar to db410c. BTW, I will add that in the next spin. -Sumit > > > difference from db410c is serial port where HMIBSC board uses UART1 as > > the debug console with an RS232 port, patch #1 - #3 adds corresponding > > driver support. > > > > Patch #4 adds main HMIBSC board specific bits, features: > > - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306) > > - 2GiB RAM > > - 64GiB eMMC, SD slot > > - WiFi and Bluetooth > > - 2x Host, 1x Device USB port > > - HDMI > > - Discrete TPM2 chip over SPI > > > > Patch #5 - #7 enables specific board features like RAUC support, > > environment protection and USB networking support. > > > > This patch series is based on top of Qcom maintainer tree [1] + the latest > > PMIC patch-set [2]. Feedback is very much welcome. > > > > [1] https://source.denx.de/u-boot/custodians/u-boot-snapdragon/-/commits/u-boot-qcom-next?ref_type=heads > > [2] https://patchwork.ozlabs.org/project/uboot/list/?series=385322 > > > > Sumit Garg (7): > > clk: apq8016: Add support for UART1 clocks > > serial_msm: Add support for RS232 GPIOs > > serial_msm: Enable RS232 flow control > > board: Add SE HMIBSC board support > > hmibsc: Enable RAUC support > > hmibsc: enable U-Boot Environment variables protection > > hmibsc: Enable LAN75XX USB ethernet driver > > > > arch/arm/dts/Makefile | 1 + > > arch/arm/dts/hmibsc-uboot.dtsi | 43 +++++++ > > arch/arm/dts/hmibsc.dts | 188 +++++++++++++++++++++++++++++ > > arch/arm/mach-snapdragon/Kconfig | 18 +++ > > arch/arm/mach-snapdragon/Makefile | 1 + > > board/schneider/hmibsc/Kconfig | 15 +++ > > board/schneider/hmibsc/MAINTAINERS | 6 + > > board/schneider/hmibsc/Makefile | 5 + > > board/schneider/hmibsc/hmibsc.c | 179 +++++++++++++++++++++++++++ > > board/schneider/hmibsc/hmibsc.env | 11 ++ > > configs/hmibsc_defconfig | 79 ++++++++++++ > > drivers/clk/qcom/clock-apq8016.c | 44 ++++++- > > drivers/serial/serial_msm.c | 23 +++- > > drivers/usb/host/Kconfig | 1 + > > include/configs/hmibsc.h | 59 +++++++++ > > 15 files changed, 665 insertions(+), 8 deletions(-) > > create mode 100644 arch/arm/dts/hmibsc-uboot.dtsi > > create mode 100644 arch/arm/dts/hmibsc.dts > > create mode 100644 board/schneider/hmibsc/Kconfig > > create mode 100644 board/schneider/hmibsc/MAINTAINERS > > create mode 100644 board/schneider/hmibsc/Makefile > > create mode 100644 board/schneider/hmibsc/hmibsc.c > > create mode 100644 board/schneider/hmibsc/hmibsc.env > > create mode 100644 configs/hmibsc_defconfig > > create mode 100644 include/configs/hmibsc.h > > > > -- > > 2.34.1 > > > > Regards, > Simon
On 19/12/2023 06:25, Sumit Garg wrote: > Hi Simon, > > On Mon, 18 Dec 2023 at 20:32, Simon Glass <sjg@chromium.org> wrote: >> >> Hi Sumit, >> >> On Mon, 18 Dec 2023 at 00:24, Sumit Garg <sumit.garg@linaro.org> wrote: >>> >>> SE HMIBSC board is based on Qcom APQ8016 SoC. One of the major >> >> Could you please add a doc/ file for this board and explain how to >> build it and how to run U-Boot on it? > > Ah I forgot to add that since the build/boot instructions are quite > similar to db410c. BTW, I will add that in the next spin. Probably just referencing that document or making the language generic would be good. 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. 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" 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. [1]: https://lore.kernel.org/u-boot/20231219-b4-qcom-common-target-v2-0-b6dd9704219e@linaro.org/ Kind regards, > > -Sumit > >> >>> difference from db410c is serial port where HMIBSC board uses UART1 as >>> the debug console with an RS232 port, patch #1 - #3 adds corresponding >>> driver support. >>> >>> Patch #4 adds main HMIBSC board specific bits, features: >>> - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306) >>> - 2GiB RAM >>> - 64GiB eMMC, SD slot >>> - WiFi and Bluetooth >>> - 2x Host, 1x Device USB port >>> - HDMI >>> - Discrete TPM2 chip over SPI >>> >>> Patch #5 - #7 enables specific board features like RAUC support, >>> environment protection and USB networking support. >>> >>> This patch series is based on top of Qcom maintainer tree [1] + the latest >>> PMIC patch-set [2]. Feedback is very much welcome. >>> >>> [1] https://source.denx.de/u-boot/custodians/u-boot-snapdragon/-/commits/u-boot-qcom-next?ref_type=heads >>> [2] https://patchwork.ozlabs.org/project/uboot/list/?series=385322 >>> >>> Sumit Garg (7): >>> clk: apq8016: Add support for UART1 clocks >>> serial_msm: Add support for RS232 GPIOs >>> serial_msm: Enable RS232 flow control >>> board: Add SE HMIBSC board support >>> hmibsc: Enable RAUC support >>> hmibsc: enable U-Boot Environment variables protection >>> hmibsc: Enable LAN75XX USB ethernet driver >>> >>> arch/arm/dts/Makefile | 1 + >>> arch/arm/dts/hmibsc-uboot.dtsi | 43 +++++++ >>> arch/arm/dts/hmibsc.dts | 188 +++++++++++++++++++++++++++++ >>> arch/arm/mach-snapdragon/Kconfig | 18 +++ >>> arch/arm/mach-snapdragon/Makefile | 1 + >>> board/schneider/hmibsc/Kconfig | 15 +++ >>> board/schneider/hmibsc/MAINTAINERS | 6 + >>> board/schneider/hmibsc/Makefile | 5 + >>> board/schneider/hmibsc/hmibsc.c | 179 +++++++++++++++++++++++++++ >>> board/schneider/hmibsc/hmibsc.env | 11 ++ >>> configs/hmibsc_defconfig | 79 ++++++++++++ >>> drivers/clk/qcom/clock-apq8016.c | 44 ++++++- >>> drivers/serial/serial_msm.c | 23 +++- >>> drivers/usb/host/Kconfig | 1 + >>> include/configs/hmibsc.h | 59 +++++++++ >>> 15 files changed, 665 insertions(+), 8 deletions(-) >>> create mode 100644 arch/arm/dts/hmibsc-uboot.dtsi >>> create mode 100644 arch/arm/dts/hmibsc.dts >>> create mode 100644 board/schneider/hmibsc/Kconfig >>> create mode 100644 board/schneider/hmibsc/MAINTAINERS >>> create mode 100644 board/schneider/hmibsc/Makefile >>> create mode 100644 board/schneider/hmibsc/hmibsc.c >>> create mode 100644 board/schneider/hmibsc/hmibsc.env >>> create mode 100644 configs/hmibsc_defconfig >>> create mode 100644 include/configs/hmibsc.h >>> >>> -- >>> 2.34.1 >>> >> >> Regards, >> Simon
On Tue, 19 Dec 2023 at 21:56, Caleb Connolly <caleb.connolly@linaro.org> wrote: > > > > On 19/12/2023 06:25, Sumit Garg wrote: > > Hi Simon, > > > > On Mon, 18 Dec 2023 at 20:32, Simon Glass <sjg@chromium.org> wrote: > >> > >> Hi Sumit, > >> > >> On Mon, 18 Dec 2023 at 00:24, Sumit Garg <sumit.garg@linaro.org> wrote: > >>> > >>> SE HMIBSC board is based on Qcom APQ8016 SoC. One of the major > >> > >> Could you please add a doc/ file for this board and explain how to > >> build it and how to run U-Boot on it? > > > > Ah I forgot to add that since the build/boot instructions are quite > > similar to db410c. BTW, I will add that in the next spin. > > Probably just referencing that document or making the language generic > would be good. > I will rename that doc to be rather SoC specific with board specific sub-sections. > 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. > 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. -Sumit > > [1]: > https://lore.kernel.org/u-boot/20231219-b4-qcom-common-target-v2-0-b6dd9704219e@linaro.org/ > > Kind regards, > > > > > -Sumit > > > >> > >>> difference from db410c is serial port where HMIBSC board uses UART1 as > >>> the debug console with an RS232 port, patch #1 - #3 adds corresponding > >>> driver support. > >>> > >>> Patch #4 adds main HMIBSC board specific bits, features: > >>> - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306) > >>> - 2GiB RAM > >>> - 64GiB eMMC, SD slot > >>> - WiFi and Bluetooth > >>> - 2x Host, 1x Device USB port > >>> - HDMI > >>> - Discrete TPM2 chip over SPI > >>> > >>> Patch #5 - #7 enables specific board features like RAUC support, > >>> environment protection and USB networking support. > >>> > >>> This patch series is based on top of Qcom maintainer tree [1] + the latest > >>> PMIC patch-set [2]. Feedback is very much welcome. > >>> > >>> [1] https://source.denx.de/u-boot/custodians/u-boot-snapdragon/-/commits/u-boot-qcom-next?ref_type=heads > >>> [2] https://patchwork.ozlabs.org/project/uboot/list/?series=385322 > >>> > >>> Sumit Garg (7): > >>> clk: apq8016: Add support for UART1 clocks > >>> serial_msm: Add support for RS232 GPIOs > >>> serial_msm: Enable RS232 flow control > >>> board: Add SE HMIBSC board support > >>> hmibsc: Enable RAUC support > >>> hmibsc: enable U-Boot Environment variables protection > >>> hmibsc: Enable LAN75XX USB ethernet driver > >>> > >>> arch/arm/dts/Makefile | 1 + > >>> arch/arm/dts/hmibsc-uboot.dtsi | 43 +++++++ > >>> arch/arm/dts/hmibsc.dts | 188 +++++++++++++++++++++++++++++ > >>> arch/arm/mach-snapdragon/Kconfig | 18 +++ > >>> arch/arm/mach-snapdragon/Makefile | 1 + > >>> board/schneider/hmibsc/Kconfig | 15 +++ > >>> board/schneider/hmibsc/MAINTAINERS | 6 + > >>> board/schneider/hmibsc/Makefile | 5 + > >>> board/schneider/hmibsc/hmibsc.c | 179 +++++++++++++++++++++++++++ > >>> board/schneider/hmibsc/hmibsc.env | 11 ++ > >>> configs/hmibsc_defconfig | 79 ++++++++++++ > >>> drivers/clk/qcom/clock-apq8016.c | 44 ++++++- > >>> drivers/serial/serial_msm.c | 23 +++- > >>> drivers/usb/host/Kconfig | 1 + > >>> include/configs/hmibsc.h | 59 +++++++++ > >>> 15 files changed, 665 insertions(+), 8 deletions(-) > >>> create mode 100644 arch/arm/dts/hmibsc-uboot.dtsi > >>> create mode 100644 arch/arm/dts/hmibsc.dts > >>> create mode 100644 board/schneider/hmibsc/Kconfig > >>> create mode 100644 board/schneider/hmibsc/MAINTAINERS > >>> create mode 100644 board/schneider/hmibsc/Makefile > >>> create mode 100644 board/schneider/hmibsc/hmibsc.c > >>> create mode 100644 board/schneider/hmibsc/hmibsc.env > >>> create mode 100644 configs/hmibsc_defconfig > >>> create mode 100644 include/configs/hmibsc.h > >>> > >>> -- > >>> 2.34.1 > >>> > >> > >> Regards, > >> Simon > > -- > // Caleb (they/them)
On 20/12/2023 13:36, Sumit Garg wrote: > On Tue, 19 Dec 2023 at 21:56, Caleb Connolly <caleb.connolly@linaro.org> wrote: >> >> >> >> On 19/12/2023 06:25, Sumit Garg wrote: >>> Hi Simon, >>> >>> On Mon, 18 Dec 2023 at 20:32, Simon Glass <sjg@chromium.org> wrote: >>>> >>>> Hi Sumit, >>>> >>>> On Mon, 18 Dec 2023 at 00:24, Sumit Garg <sumit.garg@linaro.org> wrote: >>>>> >>>>> SE HMIBSC board is based on Qcom APQ8016 SoC. One of the major >>>> >>>> Could you please add a doc/ file for this board and explain how to >>>> build it and how to run U-Boot on it? >>> >>> Ah I forgot to add that since the build/boot instructions are quite >>> similar to db410c. BTW, I will add that in the next spin. >> >> Probably just referencing that document or making the language generic >> would be good. >> > > I will rename that doc to be rather SoC specific with board specific > sub-sections. > >> 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. 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. > >> 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. 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. > > -Sumit > >> >> [1]: >> https://lore.kernel.org/u-boot/20231219-b4-qcom-common-target-v2-0-b6dd9704219e@linaro.org/ >> >> Kind regards, >> >>> >>> -Sumit >>> >>>> >>>>> difference from db410c is serial port where HMIBSC board uses UART1 as >>>>> the debug console with an RS232 port, patch #1 - #3 adds corresponding >>>>> driver support. >>>>> >>>>> Patch #4 adds main HMIBSC board specific bits, features: >>>>> - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306) >>>>> - 2GiB RAM >>>>> - 64GiB eMMC, SD slot >>>>> - WiFi and Bluetooth >>>>> - 2x Host, 1x Device USB port >>>>> - HDMI >>>>> - Discrete TPM2 chip over SPI >>>>> >>>>> Patch #5 - #7 enables specific board features like RAUC support, >>>>> environment protection and USB networking support. >>>>> >>>>> This patch series is based on top of Qcom maintainer tree [1] + the latest >>>>> PMIC patch-set [2]. Feedback is very much welcome. >>>>> >>>>> [1] https://source.denx.de/u-boot/custodians/u-boot-snapdragon/-/commits/u-boot-qcom-next?ref_type=heads >>>>> [2] https://patchwork.ozlabs.org/project/uboot/list/?series=385322 >>>>> >>>>> Sumit Garg (7): >>>>> clk: apq8016: Add support for UART1 clocks >>>>> serial_msm: Add support for RS232 GPIOs >>>>> serial_msm: Enable RS232 flow control >>>>> board: Add SE HMIBSC board support >>>>> hmibsc: Enable RAUC support >>>>> hmibsc: enable U-Boot Environment variables protection >>>>> hmibsc: Enable LAN75XX USB ethernet driver >>>>> >>>>> arch/arm/dts/Makefile | 1 + >>>>> arch/arm/dts/hmibsc-uboot.dtsi | 43 +++++++ >>>>> arch/arm/dts/hmibsc.dts | 188 +++++++++++++++++++++++++++++ >>>>> arch/arm/mach-snapdragon/Kconfig | 18 +++ >>>>> arch/arm/mach-snapdragon/Makefile | 1 + >>>>> board/schneider/hmibsc/Kconfig | 15 +++ >>>>> board/schneider/hmibsc/MAINTAINERS | 6 + >>>>> board/schneider/hmibsc/Makefile | 5 + >>>>> board/schneider/hmibsc/hmibsc.c | 179 +++++++++++++++++++++++++++ >>>>> board/schneider/hmibsc/hmibsc.env | 11 ++ >>>>> configs/hmibsc_defconfig | 79 ++++++++++++ >>>>> drivers/clk/qcom/clock-apq8016.c | 44 ++++++- >>>>> drivers/serial/serial_msm.c | 23 +++- >>>>> drivers/usb/host/Kconfig | 1 + >>>>> include/configs/hmibsc.h | 59 +++++++++ >>>>> 15 files changed, 665 insertions(+), 8 deletions(-) >>>>> create mode 100644 arch/arm/dts/hmibsc-uboot.dtsi >>>>> create mode 100644 arch/arm/dts/hmibsc.dts >>>>> create mode 100644 board/schneider/hmibsc/Kconfig >>>>> create mode 100644 board/schneider/hmibsc/MAINTAINERS >>>>> create mode 100644 board/schneider/hmibsc/Makefile >>>>> create mode 100644 board/schneider/hmibsc/hmibsc.c >>>>> create mode 100644 board/schneider/hmibsc/hmibsc.env >>>>> create mode 100644 configs/hmibsc_defconfig >>>>> create mode 100644 include/configs/hmibsc.h >>>>> >>>>> -- >>>>> 2.34.1 >>>>> >>>> >>>> Regards, >>>> Simon >> >> -- >> // Caleb (they/them)
On Thu, 21 Dec 2023 at 19:09, Caleb Connolly <caleb.connolly@linaro.org> wrote: > > > > On 20/12/2023 13:36, Sumit Garg wrote: > > On Tue, 19 Dec 2023 at 21:56, Caleb Connolly <caleb.connolly@linaro.org> wrote: > >> > >> > >> > >> On 19/12/2023 06:25, Sumit Garg wrote: > >>> Hi Simon, > >>> > >>> On Mon, 18 Dec 2023 at 20:32, Simon Glass <sjg@chromium.org> wrote: > >>>> > >>>> Hi Sumit, > >>>> > >>>> On Mon, 18 Dec 2023 at 00:24, Sumit Garg <sumit.garg@linaro.org> wrote: > >>>>> > >>>>> SE HMIBSC board is based on Qcom APQ8016 SoC. One of the major > >>>> > >>>> Could you please add a doc/ file for this board and explain how to > >>>> build it and how to run U-Boot on it? > >>> > >>> Ah I forgot to add that since the build/boot instructions are quite > >>> similar to db410c. BTW, I will add that in the next spin. > >> > >> Probably just referencing that document or making the language generic > >> would be good. > >> > > > > I will rename that doc to be rather SoC specific with board specific > > sub-sections. > > > >> 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? [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. - Configure MAC address for network support. - Setup board serial number. I suppose we don't need #ifdefry in the arch/arm/mach-snapdragon/board.c file, right? > > > >> 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. -Sumit > > > > -Sumit > > > >> > >> [1]: > >> https://lore.kernel.org/u-boot/20231219-b4-qcom-common-target-v2-0-b6dd9704219e@linaro.org/ > >> > >> Kind regards, > >> > >>> > >>> -Sumit > >>> > >>>> > >>>>> difference from db410c is serial port where HMIBSC board uses UART1 as > >>>>> the debug console with an RS232 port, patch #1 - #3 adds corresponding > >>>>> driver support. > >>>>> > >>>>> Patch #4 adds main HMIBSC board specific bits, features: > >>>>> - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306) > >>>>> - 2GiB RAM > >>>>> - 64GiB eMMC, SD slot > >>>>> - WiFi and Bluetooth > >>>>> - 2x Host, 1x Device USB port > >>>>> - HDMI > >>>>> - Discrete TPM2 chip over SPI > >>>>> > >>>>> Patch #5 - #7 enables specific board features like RAUC support, > >>>>> environment protection and USB networking support. > >>>>> > >>>>> This patch series is based on top of Qcom maintainer tree [1] + the latest > >>>>> PMIC patch-set [2]. Feedback is very much welcome. > >>>>> > >>>>> [1] https://source.denx.de/u-boot/custodians/u-boot-snapdragon/-/commits/u-boot-qcom-next?ref_type=heads > >>>>> [2] https://patchwork.ozlabs.org/project/uboot/list/?series=385322 > >>>>> > >>>>> Sumit Garg (7): > >>>>> clk: apq8016: Add support for UART1 clocks > >>>>> serial_msm: Add support for RS232 GPIOs > >>>>> serial_msm: Enable RS232 flow control > >>>>> board: Add SE HMIBSC board support > >>>>> hmibsc: Enable RAUC support > >>>>> hmibsc: enable U-Boot Environment variables protection > >>>>> hmibsc: Enable LAN75XX USB ethernet driver > >>>>> > >>>>> arch/arm/dts/Makefile | 1 + > >>>>> arch/arm/dts/hmibsc-uboot.dtsi | 43 +++++++ > >>>>> arch/arm/dts/hmibsc.dts | 188 +++++++++++++++++++++++++++++ > >>>>> arch/arm/mach-snapdragon/Kconfig | 18 +++ > >>>>> arch/arm/mach-snapdragon/Makefile | 1 + > >>>>> board/schneider/hmibsc/Kconfig | 15 +++ > >>>>> board/schneider/hmibsc/MAINTAINERS | 6 + > >>>>> board/schneider/hmibsc/Makefile | 5 + > >>>>> board/schneider/hmibsc/hmibsc.c | 179 +++++++++++++++++++++++++++ > >>>>> board/schneider/hmibsc/hmibsc.env | 11 ++ > >>>>> configs/hmibsc_defconfig | 79 ++++++++++++ > >>>>> drivers/clk/qcom/clock-apq8016.c | 44 ++++++- > >>>>> drivers/serial/serial_msm.c | 23 +++- > >>>>> drivers/usb/host/Kconfig | 1 + > >>>>> include/configs/hmibsc.h | 59 +++++++++ > >>>>> 15 files changed, 665 insertions(+), 8 deletions(-) > >>>>> create mode 100644 arch/arm/dts/hmibsc-uboot.dtsi > >>>>> create mode 100644 arch/arm/dts/hmibsc.dts > >>>>> create mode 100644 board/schneider/hmibsc/Kconfig > >>>>> create mode 100644 board/schneider/hmibsc/MAINTAINERS > >>>>> create mode 100644 board/schneider/hmibsc/Makefile > >>>>> create mode 100644 board/schneider/hmibsc/hmibsc.c > >>>>> create mode 100644 board/schneider/hmibsc/hmibsc.env > >>>>> create mode 100644 configs/hmibsc_defconfig > >>>>> create mode 100644 include/configs/hmibsc.h > >>>>> > >>>>> -- > >>>>> 2.34.1 > >>>>> > >>>> > >>>> Regards, > >>>> Simon > >> > >> -- > >> // Caleb (they/them) > > -- > // Caleb (they/them)
[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, what I'm suggesting that you do is just use the board code from db410c. 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. --- 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@gmail.com> + * (C) Copyright 2023 Sumit Garg <sumit.garg@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. 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. > - 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. > > 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. > >>> >>>> 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. 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. > > -Sumit > >>> >>> -Sumit >>> >>>> >>>> [1]: >>>> https://lore.kernel.org/u-boot/20231219-b4-qcom-common-target-v2-0-b6dd9704219e@linaro.org/ >>>> >>>> Kind regards, >>>> >>>>> >>>>> -Sumit >>>>> >>>>>> >>>>>>> difference from db410c is serial port where HMIBSC board uses UART1 as >>>>>>> the debug console with an RS232 port, patch #1 - #3 adds corresponding >>>>>>> driver support. >>>>>>> >>>>>>> Patch #4 adds main HMIBSC board specific bits, features: >>>>>>> - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306) >>>>>>> - 2GiB RAM >>>>>>> - 64GiB eMMC, SD slot >>>>>>> - WiFi and Bluetooth >>>>>>> - 2x Host, 1x Device USB port >>>>>>> - HDMI >>>>>>> - Discrete TPM2 chip over SPI >>>>>>> >>>>>>> Patch #5 - #7 enables specific board features like RAUC support, >>>>>>> environment protection and USB networking support. >>>>>>> >>>>>>> This patch series is based on top of Qcom maintainer tree [1] + the latest >>>>>>> PMIC patch-set [2]. Feedback is very much welcome. >>>>>>> >>>>>>> [1] https://source.denx.de/u-boot/custodians/u-boot-snapdragon/-/commits/u-boot-qcom-next?ref_type=heads >>>>>>> [2] https://patchwork.ozlabs.org/project/uboot/list/?series=385322 >>>>>>> >>>>>>> Sumit Garg (7): >>>>>>> clk: apq8016: Add support for UART1 clocks >>>>>>> serial_msm: Add support for RS232 GPIOs >>>>>>> serial_msm: Enable RS232 flow control >>>>>>> board: Add SE HMIBSC board support >>>>>>> hmibsc: Enable RAUC support >>>>>>> hmibsc: enable U-Boot Environment variables protection >>>>>>> hmibsc: Enable LAN75XX USB ethernet driver >>>>>>> >>>>>>> arch/arm/dts/Makefile | 1 + >>>>>>> arch/arm/dts/hmibsc-uboot.dtsi | 43 +++++++ >>>>>>> arch/arm/dts/hmibsc.dts | 188 +++++++++++++++++++++++++++++ >>>>>>> arch/arm/mach-snapdragon/Kconfig | 18 +++ >>>>>>> arch/arm/mach-snapdragon/Makefile | 1 + >>>>>>> board/schneider/hmibsc/Kconfig | 15 +++ >>>>>>> board/schneider/hmibsc/MAINTAINERS | 6 + >>>>>>> board/schneider/hmibsc/Makefile | 5 + >>>>>>> board/schneider/hmibsc/hmibsc.c | 179 +++++++++++++++++++++++++++ >>>>>>> board/schneider/hmibsc/hmibsc.env | 11 ++ >>>>>>> configs/hmibsc_defconfig | 79 ++++++++++++ >>>>>>> drivers/clk/qcom/clock-apq8016.c | 44 ++++++- >>>>>>> drivers/serial/serial_msm.c | 23 +++- >>>>>>> drivers/usb/host/Kconfig | 1 + >>>>>>> include/configs/hmibsc.h | 59 +++++++++ >>>>>>> 15 files changed, 665 insertions(+), 8 deletions(-) >>>>>>> create mode 100644 arch/arm/dts/hmibsc-uboot.dtsi >>>>>>> create mode 100644 arch/arm/dts/hmibsc.dts >>>>>>> create mode 100644 board/schneider/hmibsc/Kconfig >>>>>>> create mode 100644 board/schneider/hmibsc/MAINTAINERS >>>>>>> create mode 100644 board/schneider/hmibsc/Makefile >>>>>>> create mode 100644 board/schneider/hmibsc/hmibsc.c >>>>>>> create mode 100644 board/schneider/hmibsc/hmibsc.env >>>>>>> create mode 100644 configs/hmibsc_defconfig >>>>>>> create mode 100644 include/configs/hmibsc.h >>>>>>> >>>>>>> -- >>>>>>> 2.34.1 >>>>>>> >>>>>> >>>>>> Regards, >>>>>> Simon >>>> >>>> -- >>>> // Caleb (they/them) >> >> -- >> // Caleb (they/them)
On Fri, 22 Dec 2023 at 23:04, Caleb Connolly <caleb.connolly@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.kulikowski@gmail.com> > + * (C) Copyright 2023 Sumit Garg <sumit.garg@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
On 26/12/2023 10:27, Sumit Garg wrote: > On Fri, 22 Dec 2023 at 23:04, Caleb Connolly <caleb.connolly@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@gmail.com> >> + * (C) Copyright 2023 Sumit Garg <sumit.garg@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