Hi Stephan,
On Mon, 11 Mar 2024 at 20:07, Stephan Gerhold stephan@gerhold.net wrote:
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. :-)
Thanks for your review and yeah I would be happy to incorporate your comments.
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).
I will post a DTS patch for Linux kernel too as soon as I get a go ahead from the vendor. But for the time being it shouldn't be a barrier to U-Boot support.
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?
Okay, I will make it: "schneider,apq8016-hmibsc" instead.
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.
That sounds better. I will rename DTS file as "apq8016-schneider-hmibsc.dts"
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).
Ack.
[...] +&blsp_i2c6 {
status = "okay";label = "I2C1";rtc1: s35390a@30 {rtc@
Ack.
compatible = "sii,s35390a";reg = <0x30>;};eeprom1: 24c256@50 {eeprom@
Ack.
compatible = "atmel,24c256";reg = <0x50>;};+};
+&blsp_i2c3 {
i2c3 should come before i2c6 (sorted alphabetically)
Ack.
status = "okay";label = "I2C4";eeprom: 24c32@50 {eeprom@
Ack.
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>; };
This works much better, I will use it instead.
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).
We have the dtbs_check in U-Boot too. I will use that before posting the next version.
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.
Actually these GPIOs are a mux to select among different UART modes (RS232/422/485). This configuration allows you to select RS232 mode. How about following label/node names?
uart1_mux0_rs232_high: uart1-mux0-rs232-state
uart1_mux1_rs232_low: uart1-mux1-rs232-state
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.
Okay I will drop that config.
-Sumit
Thanks, Stephan