Re: [PATCH v2 5/5] board: add support for Schneider HMIBSC board

[Stephan Gerhold <stephan@gerhold.net>]

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