On Mon, Mar 11, 2024 at 04:40:26PM +0530, Sumit Garg wrote:
Support for Schneider Electric HMIBSC. 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
Features enabled in U-Boot:
- RAUC updates
- Environment protection
- USB based ethernet adaptors
Signed-off-by: Sumit Garg sumit.garg@linaro.org
I'm entirely sure which requirements or conventions we are following for adding device trees directly to U-Boot instead of Linux. My understanding is that the goal is to get U-Boot DTs as close as possible to the upstream Linux DTs, so I effectively looked at this as if it were submitted to linux-arm-msm. I think most of my comments should be trivial to fix anyway without much effort. :-)
With the comments fixed it would be likely also easy to get it in upstream in Linux, so I wonder if it's worth first adding it here in U-Boot (I know you discussed this on v1 already a bit).
arch/arm/dts/apq8016-hmibsc.dts | 496 +++++++++++++++++++++++++++++ board/schneider/hmibsc/MAINTAINERS | 6 + configs/hmibsc_defconfig | 87 +++++ doc/board/index.rst | 1 + doc/board/schneider/hmibsc.rst | 45 +++ doc/board/schneider/index.rst | 9 + include/configs/hmibsc.h | 57 ++++ 7 files changed, 701 insertions(+) create mode 100644 arch/arm/dts/apq8016-hmibsc.dts create mode 100644 board/schneider/hmibsc/MAINTAINERS create mode 100644 configs/hmibsc_defconfig create mode 100644 doc/board/schneider/hmibsc.rst create mode 100644 doc/board/schneider/index.rst create mode 100644 include/configs/hmibsc.h
diff --git a/arch/arm/dts/apq8016-hmibsc.dts b/arch/arm/dts/apq8016-hmibsc.dts new file mode 100644 index 00000000000..490ab5fd2fa --- /dev/null +++ b/arch/arm/dts/apq8016-hmibsc.dts @@ -0,0 +1,496 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*
- Copyright (c) 2015, The Linux Foundation. All rights reserved.
- Copyright (c) 2024, Linaro Ltd.
- */
+/dts-v1/;
+#include "msm8916-pm8916.dtsi" +#include <dt-bindings/gpio/gpio.h> +#include <dt-bindings/input/input.h> +#include <dt-bindings/leds/common.h> +#include <dt-bindings/pinctrl/qcom,pmic-gpio.h> +#include <dt-bindings/pinctrl/qcom,pmic-mpp.h> +#include <dt-bindings/sound/apq8016-lpass.h>
+/ {
- model = "Schneider Electric HMIBSC Board";
- compatible = "qcom,apq8016-hmibsc", "qcom,apq8016";
A Schneider Electric specific compatible would be likely more accurate, since I assume this board wasn't designed by Qualcomm?
I would personally also prefer to use the "apq8016-<vendor>-<board>.dts" naming convention that we typically use for smartphones/tablets upstream, although you can also keep it as-is since e.g. apq8039-t2.dts is also named without vendor.
- aliases {
serial0 = &blsp_uart1;serial1 = &blsp_uart2;usid0 = &pm8916_0;i2c1 = &blsp_i2c6;i2c3 = &blsp_i2c4;i2c4 = &blsp_i2c3;spi0 = &blsp_spi5;
You might want to add mmcX aliases here to ensure consistent naming of eMMC and SD card (this used to be in msm8916.dtsi but not anymore).
[...] +&blsp_i2c6 {
- status = "okay";
- label = "I2C1";
- rtc1: s35390a@30 {
rtc@
compatible = "sii,s35390a";reg = <0x30>;- };
- eeprom1: 24c256@50 {
eeprom@
compatible = "atmel,24c256";reg = <0x50>;- };
+};
+&blsp_i2c3 {
i2c3 should come before i2c6 (sorted alphabetically)
- status = "okay";
- label = "I2C4";
- eeprom: 24c32@50 {
eeprom@
compatible = "onsemi,24c32";reg = <0x50>;- };
+};
[...]
+&pm8916_0 {
- pon@800 {
pwrkey {status = "okay";interrupts = <0x0 0x8 1 IRQ_TYPE_EDGE_BOTH>;
This line would really benefit from a comment that explains what exactly it does and why this is done. :)
It looks like you are redefining the pwrkey with the resin interrupt. I guess your goal is to have KEY_POWER assigned to the resin pin? In that case, I think it would be cleaner to describe this using:
&pm8916_resin { status = "okay"; linux,code = <KEY_POWER>; };
and leave the pwrkey node alone (or perhaps disable it if it causes trouble).
Aside from the confusion, I think overriding only the interrupt of the pwrkey will also misbehave in unexpected ways since e.g. the Linux pm8941-pwrkey driver will still write the configured debounce time and pull up to the pwrkey registers, and not to the resin ones.
};- };
+};
[...]
+&tlmm {
- pinctrl-names = "default";
- pinctrl-0 = <&gpio_rs232_high &gpio_rs232_low>;
- sdc2_cd_default: sdc2-cd-default-state {
pins = "gpio38";function = "gpio";drive-strength = <2>;bias-disable;- };
- usb_id_default: usb-id-default-state {
pins = "gpio110";function = "gpio";drive-strength = <8>;bias-pull-up;- };
- adv7533_int_active: adv533-int-active-state {
pins = "gpio31";function = "gpio";drive-strength = <16>;bias-disable;- };
- adv7533_int_suspend: adv7533-int-suspend-state {
pins = "gpio31";function = "gpio";drive-strength = <2>;bias-disable;- };
- adv7533_switch_active: adv7533-switch-active-state {
pins = "gpio32";function = "gpio";drive-strength = <16>;bias-disable;- };
- adv7533_switch_suspend: adv7533-switch-suspend-state {
pins = "gpio32";function = "gpio";drive-strength = <2>;bias-disable;- };
- msm_key_volp_n_default: msm-key-volp-n-default-state {
pins = "gpio107";function = "gpio";drive-strength = <8>;bias-pull-up;- };
- gpio_rs232_high: gpio_rs232_high {
Pretty sure DT schema checks would complain about this node name (need -state suffix, no underscores).
bootph-all;pins = "gpio99";function = "gpio";drive-strength = <16>;bias-disable;output-high;- };
- gpio_rs232_low: gpio_rs232_low {
Same here.
Also, since I'm looking at this a bit more closely now, are there maybe more clear label/node names you could use here, or a comment you could add what exactly these pins do? I guess this enables something about RS232 but it's not clear what exactly.
bootph-all;pins = "gpio100";function = "gpio";drive-strength = <16>;bias-disable;output-low;- };
+};
[...] diff --git a/configs/hmibsc_defconfig b/configs/hmibsc_defconfig new file mode 100644 index 00000000000..02b9615114b --- /dev/null +++ b/configs/hmibsc_defconfig @@ -0,0 +1,87 @@ +CONFIG_ARM=y +CONFIG_SYS_BOARD="hmibsc" +CONFIG_COUNTER_FREQUENCY=19000000
I see you just copied this from the existing defconfigs but CONFIG_COUNTER_FREQUENCY should be unneeded, since TZ is the one responsible (and only one capable) of configuring this. And it also looks wrong to me, because the timer frequency on these Qualcomm boards is 19.2 MHz and not 19.0 MHz. :'D
I guess I'll prepare a patch to fix it for the existing boards.
Thanks, Stephan