Hi Stephan,
On Mon, 11 Mar 2024 at 20:07, Stephan Gerhold <step...@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.g...@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
