[PATCH v2 0/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[PATCH v2 0/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Sumit Garg]

Add Schneider Electric HMIBSC board DTS. The HMIBSC board is an IIoT Edge
Box Core board based on the Qualcomm APQ8016E SoC. For more information
refer to the product page [1].

One of the major difference from db410c is serial port where HMIBSC board
uses UART1 as the debug console with a default RS232 mode (UART1 mode mux
configured via gpio99 and gpio100).

Support for Schneider Electric HMIBSC. Features:
- Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306)
- 1GiB RAM
- 8GiB eMMC, SD slot
- WiFi and Bluetooth
- 2x Host, 1x Device USB port
- HDMI
- Discrete TPM2 chip over SPI
- USB ethernet adaptors (soldered)

This series is a v2 since v1 of this DTS file has been reviewed on the
U-Boot mailing list [2].

Changes in v2:
- Fix DT schema warnings.
- Incorporate suggestions from Stephan.
- Document UART1 mode GPIOs based mux.

[1] https://www.se.com/us/en/product/HMIBSCEA53D1L0T/iiot-edge-box-core-harmony-ipc-emmc-dc-linux-tpm/
[2] https://patchwork.ozlabs.org/project/uboot/patch/20240311111027.44577-6-sumit.garg@linaro.org/

Sumit Garg (3):
  dt-bindings: vendor-prefixes: Add Schneider Electric
  dt-bindings: arm: qcom: Add Schneider Electric HMIBSC board
  arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

 .../devicetree/bindings/arm/qcom.yaml         |   1 +
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 arch/arm64/boot/dts/qcom/Makefile             |   1 +
 .../dts/qcom/apq8016-schneider-hmibsc.dts     | 519 ++++++++++++++++++
 4 files changed, 523 insertions(+)
 create mode 100644 arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts

-- 
2.34.1

[PATCH v2 1/3] dt-bindings: vendor-prefixes: Add Schneider Electric

[Sumit Garg]

Add vendor prefix for Schneider Electric (https://www.se.com/).

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 1a0dc04f1db4..4ef38573e411 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1211,6 +1211,8 @@ patternProperties:
     description: Smart Battery System
   "^schindler,.*":
     description: Schindler
+  "^schneider,.*":
+    description: Schneider Electric
   "^seagate,.*":
     description: Seagate Technology PLC
   "^seeed,.*":
-- 
2.34.1

Re: [PATCH v2 1/3] dt-bindings: vendor-prefixes: Add Schneider Electric

[Krzysztof Kozlowski]

On 13/03/2024 13:30, Sumit Garg wrote:
> Add vendor prefix for Schneider Electric (https://www.se.com/).
> 
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> ---
>  Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH v2 2/3] dt-bindings: arm: qcom: Add Schneider Electric HMIBSC board

[Sumit Garg]

Document the compatible for the Schneider Electric HMIBSC IIoT edge box
core board based on the Qualcomm APQ8016E SoC.

Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 Documentation/devicetree/bindings/arm/qcom.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/arm/qcom.yaml b/Documentation/devicetree/bindings/arm/qcom.yaml
index 1a5fb889a444..c8c91754fe04 100644
--- a/Documentation/devicetree/bindings/arm/qcom.yaml
+++ b/Documentation/devicetree/bindings/arm/qcom.yaml
@@ -137,6 +137,7 @@ properties:
       - items:
           - enum:
               - qcom,apq8016-sbc
+              - schneider,apq8016-hmibsc
           - const: qcom,apq8016
 
       - items:
-- 
2.34.1

Re: [PATCH v2 2/3] dt-bindings: arm: qcom: Add Schneider Electric HMIBSC board

[Krzysztof Kozlowski]

On 13/03/2024 13:30, Sumit Garg wrote:
> Document the compatible for the Schneider Electric HMIBSC IIoT edge box
> core board based on the Qualcomm APQ8016E SoC.
> 
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> ---

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Sumit Garg]

Add Schneider Electric HMIBSC board DTS. The HMIBSC board is an IIoT Edge
Box Core board based on the Qualcomm APQ8016E SoC.

Support for Schneider Electric HMIBSC. Features:
- Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306)
- 1GiB RAM
- 8GiB eMMC, SD slot
- WiFi and Bluetooth
- 2x Host, 1x Device USB port
- HDMI
- Discrete TPM2 chip over SPI
- USB ethernet adaptors (soldered)

Co-developed-by: Jagdish Gediya <jagdish.gediya@linaro.org>
Signed-off-by: Jagdish Gediya <jagdish.gediya@linaro.org>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 arch/arm64/boot/dts/qcom/Makefile             |   1 +
 .../dts/qcom/apq8016-schneider-hmibsc.dts     | 519 ++++++++++++++++++
 2 files changed, 520 insertions(+)
 create mode 100644 arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts

diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
index 39889d5f8e12..ad55e52e950b 100644
--- a/arch/arm64/boot/dts/qcom/Makefile
+++ b/arch/arm64/boot/dts/qcom/Makefile
@@ -5,6 +5,7 @@ apq8016-sbc-usb-host-dtbs	:= apq8016-sbc.dtb apq8016-sbc-usb-host.dtbo
 
 dtb-$(CONFIG_ARCH_QCOM)	+= apq8016-sbc-usb-host.dtb
 dtb-$(CONFIG_ARCH_QCOM)	+= apq8016-sbc-d3-camera-mezzanine.dtb
+dtb-$(CONFIG_ARCH_QCOM)	+= apq8016-schneider-hmibsc.dtb
 dtb-$(CONFIG_ARCH_QCOM)	+= apq8039-t2.dtb
 dtb-$(CONFIG_ARCH_QCOM)	+= apq8094-sony-xperia-kitakami-karin_windy.dtb
 dtb-$(CONFIG_ARCH_QCOM)	+= apq8096-db820c.dtb
diff --git a/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts
new file mode 100644
index 000000000000..2f6d394feb87
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts
@@ -0,0 +1,519 @@
+// 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 = "schneider,apq8016-hmibsc", "qcom,apq8016";
+
+	aliases {
+		mmc0 = &sdhc_1; /* eMMC */
+		mmc1 = &sdhc_2; /* SD card */
+		serial0 = &blsp_uart1;
+		serial1 = &blsp_uart2;
+		usid0 = &pm8916_0;
+		i2c1 = &blsp_i2c6;
+		i2c3 = &blsp_i2c4;
+		i2c4 = &blsp_i2c3;
+		spi0 = &blsp_spi5;
+	};
+
+	chosen {
+		stdout-path = "serial0";
+	};
+
+	memory@80000000 {
+		reg = <0 0x80000000 0 0x40000000>;
+	};
+
+	reserved-memory {
+		ramoops@bff00000 {
+			compatible = "ramoops";
+			reg = <0x0 0xbff00000 0x0 0x100000>;
+
+			record-size = <0x20000>;
+			console-size = <0x20000>;
+			ftrace-size = <0x20000>;
+		};
+	};
+
+	usb2513 {
+		compatible = "smsc,usb3503";
+		reset-gpios = <&pm8916_gpios 1 GPIO_ACTIVE_LOW>;
+		initial-mode = <1>;
+	};
+
+	usb_id: usb-id {
+		compatible = "linux,extcon-usb-gpio";
+		id-gpios = <&tlmm 110 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&usb_id_default>;
+	};
+
+	hdmi-out {
+		compatible = "hdmi-connector";
+		type = "a";
+
+		port {
+			hdmi_con: endpoint {
+				remote-endpoint = <&adv7533_out>;
+			};
+		};
+	};
+
+	gpio-keys {
+		compatible = "gpio-keys";
+		autorepeat;
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&msm_key_volp_n_default>;
+
+		button {
+			label = "Volume Up";
+			linux,code = <KEY_VOLUMEUP>;
+			gpios = <&tlmm 107 GPIO_ACTIVE_LOW>;
+		};
+	};
+
+	leds {
+		pinctrl-names = "default";
+		pinctrl-0 = <&pm8916_mpps_leds>;
+
+		compatible = "gpio-leds";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		led@5 {
+			reg = <5>;
+			label = "apq8016-hmibsc:green:wlan";
+			function = LED_FUNCTION_WLAN;
+			color = <LED_COLOR_ID_YELLOW>;
+			gpios = <&pm8916_mpps 2 GPIO_ACTIVE_HIGH>;
+			linux,default-trigger = "phy0tx";
+			default-state = "off";
+		};
+
+		led@6 {
+			reg = <6>;
+			label = "apq8016-hmibsc:yellow:bt";
+			function = LED_FUNCTION_BLUETOOTH;
+			color = <LED_COLOR_ID_BLUE>;
+			gpios = <&pm8916_mpps 3 GPIO_ACTIVE_HIGH>;
+			linux,default-trigger = "bluetooth-power";
+			default-state = "off";
+		};
+	};
+};
+
+&blsp_i2c3 {
+	status = "okay";
+
+	eeprom@50 {
+		compatible = "atmel,24c32";
+		reg = <0x50>;
+	};
+};
+
+&blsp_i2c4 {
+	status = "okay";
+
+	adv_bridge: bridge@39 {
+		status = "okay";
+
+		compatible = "adi,adv7533";
+		reg = <0x39>;
+
+		interrupt-parent = <&tlmm>;
+		interrupts = <31 IRQ_TYPE_EDGE_FALLING>;
+
+		adi,dsi-lanes = <4>;
+		clocks = <&rpmcc RPM_SMD_BB_CLK2>;
+		clock-names = "cec";
+
+		pd-gpios = <&tlmm 32 GPIO_ACTIVE_HIGH>;
+
+		avdd-supply = <&pm8916_l6>;
+		a2vdd-supply = <&pm8916_l6>;
+		dvdd-supply = <&pm8916_l6>;
+		pvdd-supply = <&pm8916_l6>;
+		v1p2-supply = <&pm8916_l6>;
+		v3p3-supply = <&pm8916_l17>;
+
+		pinctrl-names = "default","sleep";
+		pinctrl-0 = <&adv7533_int_active &adv7533_switch_active>;
+		pinctrl-1 = <&adv7533_int_suspend &adv7533_switch_suspend>;
+		#sound-dai-cells = <1>;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				adv7533_in: endpoint {
+					remote-endpoint = <&mdss_dsi0_out>;
+				};
+			};
+
+			port@1 {
+				reg = <1>;
+				adv7533_out: endpoint {
+					remote-endpoint = <&hdmi_con>;
+				};
+			};
+		};
+	};
+};
+
+&blsp_i2c6 {
+	status = "okay";
+
+	rtc@30 {
+		compatible = "sii,s35390a";
+		reg = <0x30>;
+	};
+
+	eeprom@50 {
+		compatible = "atmel,24c256";
+		reg = <0x50>;
+	};
+};
+
+&blsp_spi5 {
+	status = "okay";
+	cs-gpios = <&tlmm 18 GPIO_ACTIVE_LOW>;
+
+	tpm@0 {
+		compatible = "tcg,tpm_tis-spi";
+		reg = <0>;
+		spi-max-frequency = <500000>;
+	};
+};
+
+&blsp_uart1 {
+	status = "okay";
+	label = "UART0";
+};
+
+&blsp_uart2 {
+	status = "okay";
+	label = "UART1";
+};
+
+&lpass {
+	status = "okay";
+};
+
+&mdss {
+	status = "okay";
+};
+
+&mdss_dsi0_out {
+	data-lanes = <0 1 2 3>;
+	remote-endpoint = <&adv7533_in>;
+};
+
+&pm8916_codec {
+	status = "okay";
+	qcom,mbhc-vthreshold-low = <75 150 237 450 500>;
+	qcom,mbhc-vthreshold-high = <75 150 237 450 500>;
+};
+
+&pm8916_resin {
+	status = "okay";
+	linux,code = <KEY_POWER>;
+};
+
+&pm8916_rpm_regulators {
+	pm8916_l17: l17 {
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+	};
+};
+
+&sdhc_1 {
+	status = "okay";
+};
+
+&sdhc_2 {
+	status = "okay";
+
+	pinctrl-names = "default", "sleep";
+	pinctrl-0 = <&sdc2_default &sdc2_cd_default>;
+	pinctrl-1 = <&sdc2_sleep &sdc2_cd_default>;
+
+	cd-gpios = <&tlmm 38 GPIO_ACTIVE_LOW>;
+};
+
+&sound {
+	status = "okay";
+
+	pinctrl-0 = <&cdc_pdm_default &sec_mi2s_default>;
+	pinctrl-1 = <&cdc_pdm_sleep &sec_mi2s_sleep>;
+	pinctrl-names = "default", "sleep";
+	model = "DB410c";
+	audio-routing =
+		"AMIC2", "MIC BIAS Internal2",
+		"AMIC3", "MIC BIAS External1";
+
+	quaternary-dai-link {
+		link-name = "ADV7533";
+		cpu {
+			sound-dai = <&lpass MI2S_QUATERNARY>;
+		};
+		codec {
+			sound-dai = <&adv_bridge 0>;
+		};
+	};
+
+	primary-dai-link {
+		link-name = "WCD";
+		cpu {
+			sound-dai = <&lpass MI2S_PRIMARY>;
+		};
+		codec {
+			sound-dai = <&lpass_codec 0>, <&pm8916_codec 0>;
+		};
+	};
+
+	tertiary-dai-link {
+		link-name = "WCD-Capture";
+		cpu {
+			sound-dai = <&lpass MI2S_TERTIARY>;
+		};
+		codec {
+			sound-dai = <&lpass_codec 1>, <&pm8916_codec 1>;
+		};
+	};
+};
+
+&usb {
+	status = "okay";
+	extcon = <&usb_id>, <&usb_id>;
+
+	pinctrl-names = "default", "device";
+	pinctrl-0 = <&usb_sw_sel_pm &usb_hub_reset_pm>;
+	pinctrl-1 = <&usb_sw_sel_pm_device &usb_hub_reset_pm_device>;
+};
+
+&usb_hs_phy {
+	extcon = <&usb_id>;
+};
+
+&wcnss {
+	status = "okay";
+	firmware-name = "qcom/apq8016/wcnss.mbn";
+};
+
+&wcnss_ctrl {
+	firmware-name = "qcom/apq8016/WCNSS_qcom_wlan_nv_sbc.bin";
+};
+
+&wcnss_iris {
+	compatible = "qcom,wcn3620";
+};
+
+&wcnss_mem {
+	status = "okay";
+};
+
+/* Enable CoreSight */
+&cti0 { status = "okay"; };
+&cti1 { status = "okay"; };
+&cti12 { status = "okay"; };
+&cti13 { status = "okay"; };
+&cti14 { status = "okay"; };
+&cti15 { status = "okay"; };
+&debug0 { status = "okay"; };
+&debug1 { status = "okay"; };
+&debug2 { status = "okay"; };
+&debug3 { status = "okay"; };
+&etf { status = "okay"; };
+&etm0 { status = "okay"; };
+&etm1 { status = "okay"; };
+&etm2 { status = "okay"; };
+&etm3 { status = "okay"; };
+&etr { status = "okay"; };
+&funnel0 { status = "okay"; };
+&funnel1 { status = "okay"; };
+&replicator { status = "okay"; };
+&stm { status = "okay"; };
+&tpiu { status = "okay"; };
+
+/*
+ * 2mA drive strength is not enough when connecting multiple
+ * I2C devices with different pull up resistors.
+ */
+
+&blsp_i2c4_default {
+	drive-strength = <16>;
+};
+
+&blsp_i2c6_default {
+	drive-strength = <16>;
+};
+
+&tlmm {
+	pinctrl-names = "default";
+	pinctrl-0 = <&uart1_mux0_rs232_high &uart1_mux1_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;
+	};
+
+	/*
+	 * UART1 being the debug console supports various modes of
+	 * operation (RS-232/485/422) controlled via GPIOs configured
+	 * mux as follows:
+	 *
+	 *   gpio100    gpio99    UART mode
+	 *   0          0         loopback
+	 *   0          1         RS-232
+	 *   1          0         RS-485
+	 *   1          1         RS-422
+	 *
+	 * The default mode configured here is RS-232 mode.
+	 */
+	uart1_mux0_rs232_high: uart1-mux0-rs232-state {
+		bootph-all;
+		pins = "gpio99";
+		function = "gpio";
+
+		drive-strength = <16>;
+		bias-disable;
+		output-high;
+	};
+
+	uart1_mux1_rs232_low: uart1-mux1-rs232-state {
+		bootph-all;
+		pins = "gpio100";
+		function = "gpio";
+
+		drive-strength = <16>;
+		bias-disable;
+		output-low;
+	};
+};
+
+&pm8916_gpios {
+	gpio-line-names =
+		"USB_HUB_RESET_N_PM",
+		"USB_SW_SEL_PM",
+		"NC",
+		"NC";
+
+	usb_hub_reset_pm: usb-hub-reset-pm-state {
+		pins = "gpio1";
+		function = PMIC_GPIO_FUNC_NORMAL;
+
+		input-disable;
+		output-high;
+	};
+
+	usb_hub_reset_pm_device: usb-hub-reset-pm-device-state {
+		pins = "gpio1";
+		function = PMIC_GPIO_FUNC_NORMAL;
+
+		output-low;
+	};
+
+	usb_sw_sel_pm: usb-sw-sel-pm-state {
+		pins = "gpio2";
+		function = PMIC_GPIO_FUNC_NORMAL;
+
+		power-source = <PM8916_GPIO_VPH>;
+		input-disable;
+		output-high;
+	};
+
+	usb_sw_sel_pm_device: usb-sw-sel-pm-device-state {
+		pins = "gpio2";
+		function = PMIC_GPIO_FUNC_NORMAL;
+
+		power-source = <PM8916_GPIO_VPH>;
+		input-disable;
+		output-low;
+	};
+};
+
+&pm8916_mpps {
+	gpio-line-names =
+		"NC",
+		"WLAN_LED_CTRL",
+		"BT_LED_CTRL",
+		"NC";
+
+	pm8916_mpps_leds: pm8916-mpps-state {
+		pins = "mpp2", "mpp3";
+		function = "digital";
+
+		output-low;
+	};
+};
+
+&blsp_uart1_default {
+	bootph-all;
+};
-- 
2.34.1

Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Caleb Connolly]

Hi Sumit,

On 13/03/2024 12:30, Sumit Garg wrote:
> Add Schneider Electric HMIBSC board DTS. The HMIBSC board is an IIoT Edge
> Box Core board based on the Qualcomm APQ8016E SoC.
> 
> Support for Schneider Electric HMIBSC. Features:
> - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306)
> - 1GiB RAM
> - 8GiB eMMC, SD slot
> - WiFi and Bluetooth
> - 2x Host, 1x Device USB port
> - HDMI
> - Discrete TPM2 chip over SPI
> - USB ethernet adaptors (soldered)
> 
> Co-developed-by: Jagdish Gediya <jagdish.gediya@linaro.org>
> Signed-off-by: Jagdish Gediya <jagdish.gediya@linaro.org>
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/Makefile             |   1 +
>  .../dts/qcom/apq8016-schneider-hmibsc.dts     | 519 ++++++++++++++++++
>  2 files changed, 520 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts
> 
> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> index 39889d5f8e12..ad55e52e950b 100644
> --- a/arch/arm64/boot/dts/qcom/Makefile
> +++ b/arch/arm64/boot/dts/qcom/Makefile
> @@ -5,6 +5,7 @@ apq8016-sbc-usb-host-dtbs	:= apq8016-sbc.dtb apq8016-sbc-usb-host.dtbo
>  
>  dtb-$(CONFIG_ARCH_QCOM)	+= apq8016-sbc-usb-host.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= apq8016-sbc-d3-camera-mezzanine.dtb
> +dtb-$(CONFIG_ARCH_QCOM)	+= apq8016-schneider-hmibsc.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= apq8039-t2.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= apq8094-sony-xperia-kitakami-karin_windy.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= apq8096-db820c.dtb
> diff --git a/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts
> new file mode 100644
> index 000000000000..2f6d394feb87
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts
> @@ -0,0 +1,519 @@
> +// 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 = "schneider,apq8016-hmibsc", "qcom,apq8016";
> +
> +	aliases {
> +		mmc0 = &sdhc_1; /* eMMC */
> +		mmc1 = &sdhc_2; /* SD card */
> +		serial0 = &blsp_uart1;
> +		serial1 = &blsp_uart2;
> +		usid0 = &pm8916_0;
> +		i2c1 = &blsp_i2c6;
> +		i2c3 = &blsp_i2c4;
> +		i2c4 = &blsp_i2c3;
> +		spi0 = &blsp_spi5;
> +	};
> +
> +	chosen {
> +		stdout-path = "serial0";
> +	};
> +
> +	memory@80000000 {
> +		reg = <0 0x80000000 0 0x40000000>;
> +	};
> +
> +	reserved-memory {
> +		ramoops@bff00000 {
> +			compatible = "ramoops";
> +			reg = <0x0 0xbff00000 0x0 0x100000>;
> +
> +			record-size = <0x20000>;
> +			console-size = <0x20000>;
> +			ftrace-size = <0x20000>;
> +		};
> +	};
> +
> +	usb2513 {
> +		compatible = "smsc,usb3503";
> +		reset-gpios = <&pm8916_gpios 1 GPIO_ACTIVE_LOW>;
> +		initial-mode = <1>;
> +	};
> +
> +	usb_id: usb-id {
> +		compatible = "linux,extcon-usb-gpio";
> +		id-gpios = <&tlmm 110 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&usb_id_default>;
> +	};
> +
> +	hdmi-out {
> +		compatible = "hdmi-connector";
> +		type = "a";
> +
> +		port {
> +			hdmi_con: endpoint {
> +				remote-endpoint = <&adv7533_out>;
> +			};
> +		};
> +	};
> +
> +	gpio-keys {
> +		compatible = "gpio-keys";
> +		autorepeat;
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&msm_key_volp_n_default>;
> +
> +		button {
> +			label = "Volume Up";
> +			linux,code = <KEY_VOLUMEUP>;
> +			gpios = <&tlmm 107 GPIO_ACTIVE_LOW>;
> +		};
> +	};
> +
> +	leds {
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pm8916_mpps_leds>;
> +
> +		compatible = "gpio-leds";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		led@5 {
> +			reg = <5>;
> +			label = "apq8016-hmibsc:green:wlan";
> +			function = LED_FUNCTION_WLAN;
> +			color = <LED_COLOR_ID_YELLOW>;
> +			gpios = <&pm8916_mpps 2 GPIO_ACTIVE_HIGH>;
> +			linux,default-trigger = "phy0tx";
> +			default-state = "off";
> +		};
> +
> +		led@6 {
> +			reg = <6>;
> +			label = "apq8016-hmibsc:yellow:bt";
> +			function = LED_FUNCTION_BLUETOOTH;
> +			color = <LED_COLOR_ID_BLUE>;
> +			gpios = <&pm8916_mpps 3 GPIO_ACTIVE_HIGH>;
> +			linux,default-trigger = "bluetooth-power";
> +			default-state = "off";
> +		};
> +	};
> +};
> +
> +&blsp_i2c3 {
> +	status = "okay";
> +
> +	eeprom@50 {
> +		compatible = "atmel,24c32";
> +		reg = <0x50>;
> +	};
> +};
> +
> +&blsp_i2c4 {
> +	status = "okay";
> +
> +	adv_bridge: bridge@39 {
> +		status = "okay";
> +
> +		compatible = "adi,adv7533";
> +		reg = <0x39>;
> +
> +		interrupt-parent = <&tlmm>;
> +		interrupts = <31 IRQ_TYPE_EDGE_FALLING>;
> +
> +		adi,dsi-lanes = <4>;
> +		clocks = <&rpmcc RPM_SMD_BB_CLK2>;
> +		clock-names = "cec";
> +
> +		pd-gpios = <&tlmm 32 GPIO_ACTIVE_HIGH>;
> +
> +		avdd-supply = <&pm8916_l6>;
> +		a2vdd-supply = <&pm8916_l6>;
> +		dvdd-supply = <&pm8916_l6>;
> +		pvdd-supply = <&pm8916_l6>;
> +		v1p2-supply = <&pm8916_l6>;
> +		v3p3-supply = <&pm8916_l17>;
> +
> +		pinctrl-names = "default","sleep";
> +		pinctrl-0 = <&adv7533_int_active &adv7533_switch_active>;
> +		pinctrl-1 = <&adv7533_int_suspend &adv7533_switch_suspend>;
> +		#sound-dai-cells = <1>;
> +
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			port@0 {
> +				reg = <0>;
> +				adv7533_in: endpoint {
> +					remote-endpoint = <&mdss_dsi0_out>;
> +				};
> +			};
> +
> +			port@1 {
> +				reg = <1>;
> +				adv7533_out: endpoint {
> +					remote-endpoint = <&hdmi_con>;
> +				};
> +			};
> +		};
> +	};
> +};
> +
> +&blsp_i2c6 {
> +	status = "okay";
> +
> +	rtc@30 {
> +		compatible = "sii,s35390a";
> +		reg = <0x30>;
> +	};
> +
> +	eeprom@50 {
> +		compatible = "atmel,24c256";
> +		reg = <0x50>;
> +	};
> +};
> +
> +&blsp_spi5 {
> +	status = "okay";
> +	cs-gpios = <&tlmm 18 GPIO_ACTIVE_LOW>;
> +
> +	tpm@0 {
> +		compatible = "tcg,tpm_tis-spi";
> +		reg = <0>;
> +		spi-max-frequency = <500000>;
> +	};
> +};
> +
> +&blsp_uart1 {
> +	status = "okay";
> +	label = "UART0";
> +};
> +
> +&blsp_uart2 {
> +	status = "okay";
> +	label = "UART1";
> +};
> +
> +&lpass {
> +	status = "okay";
> +};
> +
> +&mdss {
> +	status = "okay";
> +};
> +
> +&mdss_dsi0_out {
> +	data-lanes = <0 1 2 3>;
> +	remote-endpoint = <&adv7533_in>;
> +};
> +
> +&pm8916_codec {
> +	status = "okay";
> +	qcom,mbhc-vthreshold-low = <75 150 237 450 500>;
> +	qcom,mbhc-vthreshold-high = <75 150 237 450 500>;
> +};
> +
> +&pm8916_resin {
> +	status = "okay";
> +	linux,code = <KEY_POWER>;
> +};
> +
> +&pm8916_rpm_regulators {
> +	pm8916_l17: l17 {
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +	};
> +};
> +
> +&sdhc_1 {
> +	status = "okay";
> +};
> +
> +&sdhc_2 {
> +	status = "okay";
> +
> +	pinctrl-names = "default", "sleep";
> +	pinctrl-0 = <&sdc2_default &sdc2_cd_default>;
> +	pinctrl-1 = <&sdc2_sleep &sdc2_cd_default>;
> +
> +	cd-gpios = <&tlmm 38 GPIO_ACTIVE_LOW>;
> +};
> +
> +&sound {
> +	status = "okay";
> +
> +	pinctrl-0 = <&cdc_pdm_default &sec_mi2s_default>;
> +	pinctrl-1 = <&cdc_pdm_sleep &sec_mi2s_sleep>;
> +	pinctrl-names = "default", "sleep";
> +	model = "DB410c";
> +	audio-routing =
> +		"AMIC2", "MIC BIAS Internal2",
> +		"AMIC3", "MIC BIAS External1";
> +
> +	quaternary-dai-link {
> +		link-name = "ADV7533";
> +		cpu {
> +			sound-dai = <&lpass MI2S_QUATERNARY>;
> +		};
> +		codec {
> +			sound-dai = <&adv_bridge 0>;
> +		};
> +	};
> +
> +	primary-dai-link {
> +		link-name = "WCD";
> +		cpu {
> +			sound-dai = <&lpass MI2S_PRIMARY>;
> +		};
> +		codec {
> +			sound-dai = <&lpass_codec 0>, <&pm8916_codec 0>;
> +		};
> +	};
> +
> +	tertiary-dai-link {
> +		link-name = "WCD-Capture";
> +		cpu {
> +			sound-dai = <&lpass MI2S_TERTIARY>;
> +		};
> +		codec {
> +			sound-dai = <&lpass_codec 1>, <&pm8916_codec 1>;
> +		};
> +	};
> +};
> +
> +&usb {
> +	status = "okay";
> +	extcon = <&usb_id>, <&usb_id>;
> +
> +	pinctrl-names = "default", "device";
> +	pinctrl-0 = <&usb_sw_sel_pm &usb_hub_reset_pm>;
> +	pinctrl-1 = <&usb_sw_sel_pm_device &usb_hub_reset_pm_device>;
> +};
> +
> +&usb_hs_phy {
> +	extcon = <&usb_id>;
> +};
> +
> +&wcnss {
> +	status = "okay";
> +	firmware-name = "qcom/apq8016/wcnss.mbn";
> +};
> +
> +&wcnss_ctrl {
> +	firmware-name = "qcom/apq8016/WCNSS_qcom_wlan_nv_sbc.bin";
> +};
> +
> +&wcnss_iris {
> +	compatible = "qcom,wcn3620";
> +};
> +
> +&wcnss_mem {
> +	status = "okay";
> +};
> +
> +/* Enable CoreSight */
> +&cti0 { status = "okay"; };
> +&cti1 { status = "okay"; };
> +&cti12 { status = "okay"; };
> +&cti13 { status = "okay"; };
> +&cti14 { status = "okay"; };
> +&cti15 { status = "okay"; };
> +&debug0 { status = "okay"; };
> +&debug1 { status = "okay"; };
> +&debug2 { status = "okay"; };
> +&debug3 { status = "okay"; };
> +&etf { status = "okay"; };
> +&etm0 { status = "okay"; };
> +&etm1 { status = "okay"; };
> +&etm2 { status = "okay"; };
> +&etm3 { status = "okay"; };
> +&etr { status = "okay"; };
> +&funnel0 { status = "okay"; };
> +&funnel1 { status = "okay"; };
> +&replicator { status = "okay"; };
> +&stm { status = "okay"; };
> +&tpiu { status = "okay"; };
> +
> +/*
> + * 2mA drive strength is not enough when connecting multiple
> + * I2C devices with different pull up resistors.
> + */
> +
> +&blsp_i2c4_default {
> +	drive-strength = <16>;
> +};
> +
> +&blsp_i2c6_default {
> +	drive-strength = <16>;
> +};
> +
> +&tlmm {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&uart1_mux0_rs232_high &uart1_mux1_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;
> +	};
> +
> +	/*
> +	 * UART1 being the debug console supports various modes of
> +	 * operation (RS-232/485/422) controlled via GPIOs configured
> +	 * mux as follows:
> +	 *
> +	 *   gpio100    gpio99    UART mode
> +	 *   0          0         loopback
> +	 *   0          1         RS-232
> +	 *   1          0         RS-485
> +	 *   1          1         RS-422
> +	 *
> +	 * The default mode configured here is RS-232 mode.
> +	 */
> +	uart1_mux0_rs232_high: uart1-mux0-rs232-state {
> +		bootph-all;
> +		pins = "gpio99";
> +		function = "gpio";
> +
> +		drive-strength = <16>;
> +		bias-disable;
> +		output-high;
> +	};
> +
> +	uart1_mux1_rs232_low: uart1-mux1-rs232-state {
> +		bootph-all;
> +		pins = "gpio100";
> +		function = "gpio";
> +
> +		drive-strength = <16>;
> +		bias-disable;
> +		output-low;
> +	};
> +};
> +
> +&pm8916_gpios {
> +	gpio-line-names =
> +		"USB_HUB_RESET_N_PM",
> +		"USB_SW_SEL_PM",
> +		"NC",
> +		"NC";
> +
> +	usb_hub_reset_pm: usb-hub-reset-pm-state {
> +		pins = "gpio1";
> +		function = PMIC_GPIO_FUNC_NORMAL;
> +
> +		input-disable;
> +		output-high;
> +	};
> +
> +	usb_hub_reset_pm_device: usb-hub-reset-pm-device-state {
> +		pins = "gpio1";
> +		function = PMIC_GPIO_FUNC_NORMAL;
> +
> +		output-low;
> +	};
> +
> +	usb_sw_sel_pm: usb-sw-sel-pm-state {
> +		pins = "gpio2";
> +		function = PMIC_GPIO_FUNC_NORMAL;
> +
> +		power-source = <PM8916_GPIO_VPH>;
> +		input-disable;
> +		output-high;
> +	};
> +
> +	usb_sw_sel_pm_device: usb-sw-sel-pm-device-state {
> +		pins = "gpio2";
> +		function = PMIC_GPIO_FUNC_NORMAL;
> +
> +		power-source = <PM8916_GPIO_VPH>;
> +		input-disable;
> +		output-low;
> +	};
> +};
> +
> +&pm8916_mpps {
> +	gpio-line-names =
> +		"NC",
> +		"WLAN_LED_CTRL",
> +		"BT_LED_CTRL",
> +		"NC";
> +
> +	pm8916_mpps_leds: pm8916-mpps-state {
> +		pins = "mpp2", "mpp3";
> +		function = "digital";
> +
> +		output-low;
> +	};
> +};
> +
> +&blsp_uart1_default {
> +	bootph-all;
> +};

-- 
// Caleb (they/them)

Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Krzysztof Kozlowski]

On 13/03/2024 13:30, Sumit Garg wrote:
> Add Schneider Electric HMIBSC board DTS. The HMIBSC board is an IIoT Edge
> Box Core board based on the Qualcomm APQ8016E SoC.
> 

...

> +
> +/ {
> +	model = "Schneider Electric HMIBSC Board";
> +	compatible = "schneider,apq8016-hmibsc", "qcom,apq8016";
> +
> +	aliases {
> +		mmc0 = &sdhc_1; /* eMMC */
> +		mmc1 = &sdhc_2; /* SD card */
> +		serial0 = &blsp_uart1;
> +		serial1 = &blsp_uart2;
> +		usid0 = &pm8916_0;
> +		i2c1 = &blsp_i2c6;
> +		i2c3 = &blsp_i2c4;
> +		i2c4 = &blsp_i2c3;

The aliases should match schematics of the board, so I assume missing
i2c2 is intentional, right?

> +		spi0 = &blsp_spi5;
> +	};
> +
> +	chosen {
> +		stdout-path = "serial0";
> +	};
> +
> +	memory@80000000 {
> +		reg = <0 0x80000000 0 0x40000000>;
> +	};
> +
> +	reserved-memory {
> +		ramoops@bff00000 {
> +			compatible = "ramoops";
> +			reg = <0x0 0xbff00000 0x0 0x100000>;
> +
> +			record-size = <0x20000>;
> +			console-size = <0x20000>;
> +			ftrace-size = <0x20000>;
> +		};
> +	};
> +
> +	usb2513 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
e.g. usb-hub



> +		compatible = "smsc,usb3503";
> +		reset-gpios = <&pm8916_gpios 1 GPIO_ACTIVE_LOW>;
> +		initial-mode = <1>;
> +	};
> +
> +	usb_id: usb-id {
> +		compatible = "linux,extcon-usb-gpio";
> +		id-gpios = <&tlmm 110 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&usb_id_default>;
> +	};
> +
> +	hdmi-out {
> +		compatible = "hdmi-connector";
> +		type = "a";
> +
> +		port {
> +			hdmi_con: endpoint {
> +				remote-endpoint = <&adv7533_out>;
> +			};
> +		};
> +	};
> +
> +	gpio-keys {
> +		compatible = "gpio-keys";
> +		autorepeat;
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&msm_key_volp_n_default>;
> +
> +		button {
> +			label = "Volume Up";
> +			linux,code = <KEY_VOLUMEUP>;
> +			gpios = <&tlmm 107 GPIO_ACTIVE_LOW>;
> +		};
> +	};
> +
> +	leds {
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pm8916_mpps_leds>;

First property is always compatible. Please apply DTS coding style rules.

> +
> +		compatible = "gpio-leds";
> +		#address-cells = <1>;
> +		#size-cells = <0>;

That's not a bus.

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

> +
> +		led@5 {
> +			reg = <5>;
> +			label = "apq8016-hmibsc:green:wlan";
> +			function = LED_FUNCTION_WLAN;
> +			color = <LED_COLOR_ID_YELLOW>;
> +			gpios = <&pm8916_mpps 2 GPIO_ACTIVE_HIGH>;
> +			linux,default-trigger = "phy0tx";
> +			default-state = "off";
> +		};
> +
> +		led@6 {
> +			reg = <6>;
> +			label = "apq8016-hmibsc:yellow:bt";
> +			function = LED_FUNCTION_BLUETOOTH;
> +			color = <LED_COLOR_ID_BLUE>;
> +			gpios = <&pm8916_mpps 3 GPIO_ACTIVE_HIGH>;
> +			linux,default-trigger = "bluetooth-power";
> +			default-state = "off";
> +		};
> +	};
> +};
> +
> +&blsp_i2c3 {
> +	status = "okay";
> +
> +	eeprom@50 {
> +		compatible = "atmel,24c32";
> +		reg = <0x50>;
> +	};
> +};
> +
> +&blsp_i2c4 {
> +	status = "okay";
> +
> +	adv_bridge: bridge@39 {
> +		status = "okay";

Why do you need it? Was it disabled?

And why this is before compatible? If this stays, please use DTS coding
style rules for placement.

> +
> +		compatible = "adi,adv7533";
> +		reg = <0x39>;
> +
> +		interrupt-parent = <&tlmm>;
> +		interrupts = <31 IRQ_TYPE_EDGE_FALLING>;
> +
> +		adi,dsi-lanes = <4>;
> +		clocks = <&rpmcc RPM_SMD_BB_CLK2>;
> +		clock-names = "cec";
> +
> +		pd-gpios = <&tlmm 32 GPIO_ACTIVE_HIGH>;
> +
> +		avdd-supply = <&pm8916_l6>;
> +		a2vdd-supply = <&pm8916_l6>;
> +		dvdd-supply = <&pm8916_l6>;
> +		pvdd-supply = <&pm8916_l6>;
> +		v1p2-supply = <&pm8916_l6>;
> +		v3p3-supply = <&pm8916_l17>;
> +
> +		pinctrl-names = "default","sleep";
> +		pinctrl-0 = <&adv7533_int_active &adv7533_switch_active>;
> +		pinctrl-1 = <&adv7533_int_suspend &adv7533_switch_suspend>;
> +		#sound-dai-cells = <1>;
> +
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			port@0 {
> +				reg = <0>;
> +				adv7533_in: endpoint {
> +					remote-endpoint = <&mdss_dsi0_out>;
> +				};
> +			};
> +
> +			port@1 {
> +				reg = <1>;
> +				adv7533_out: endpoint {
> +					remote-endpoint = <&hdmi_con>;
> +				};
> +			};
> +		};
> +	};
> +};
> +
> +&blsp_i2c6 {
> +	status = "okay";
> +
> +	rtc@30 {
> +		compatible = "sii,s35390a";
> +		reg = <0x30>;
> +	};
> +
> +	eeprom@50 {
> +		compatible = "atmel,24c256";
> +		reg = <0x50>;
> +	};
> +};
> +
> +&blsp_spi5 {
> +	status = "okay";
> +	cs-gpios = <&tlmm 18 GPIO_ACTIVE_LOW>;
> +
> +	tpm@0 {
> +		compatible = "tcg,tpm_tis-spi";
> +		reg = <0>;
> +		spi-max-frequency = <500000>;
> +	};
> +};
> +
> +&blsp_uart1 {
> +	status = "okay";
> +	label = "UART0";
> +};
> +
> +&blsp_uart2 {
> +	status = "okay";
> +	label = "UART1";
> +};
> +
> +&lpass {
> +	status = "okay";
> +};
> +
> +&mdss {
> +	status = "okay";
> +};
> +
> +&mdss_dsi0_out {
> +	data-lanes = <0 1 2 3>;
> +	remote-endpoint = <&adv7533_in>;
> +};
> +
> +&pm8916_codec {
> +	status = "okay";
> +	qcom,mbhc-vthreshold-low = <75 150 237 450 500>;
> +	qcom,mbhc-vthreshold-high = <75 150 237 450 500>;
> +};
> +
> +&pm8916_resin {
> +	status = "okay";
> +	linux,code = <KEY_POWER>;
> +};
> +
> +&pm8916_rpm_regulators {
> +	pm8916_l17: l17 {
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +	};
> +};
> +
> +&sdhc_1 {
> +	status = "okay";
> +};
> +
> +&sdhc_2 {
> +	status = "okay";
> +
> +	pinctrl-names = "default", "sleep";
> +	pinctrl-0 = <&sdc2_default &sdc2_cd_default>;
> +	pinctrl-1 = <&sdc2_sleep &sdc2_cd_default>;
> +
> +	cd-gpios = <&tlmm 38 GPIO_ACTIVE_LOW>;
> +};
> +
> +&sound {
> +	status = "okay";

Is thi sneeded?

> +
> +	pinctrl-0 = <&cdc_pdm_default &sec_mi2s_default>;
> +	pinctrl-1 = <&cdc_pdm_sleep &sec_mi2s_sleep>;
> +	pinctrl-names = "default", "sleep";
> +	model = "DB410c";
> +	audio-routing =
> +		"AMIC2", "MIC BIAS Internal2",
> +		"AMIC3", "MIC BIAS External1";
> +
> +	quaternary-dai-link {
> +		link-name = "ADV7533";
> +		cpu {
> +			sound-dai = <&lpass MI2S_QUATERNARY>;
> +		};
> +		codec {
> +			sound-dai = <&adv_bridge 0>;
> +		};
> +	};
> +
> +	primary-dai-link {
> +		link-name = "WCD";
> +		cpu {
> +			sound-dai = <&lpass MI2S_PRIMARY>;
> +		};
> +		codec {
> +			sound-dai = <&lpass_codec 0>, <&pm8916_codec 0>;
> +		};
> +	};
> +
> +	tertiary-dai-link {
> +		link-name = "WCD-Capture";
> +		cpu {
> +			sound-dai = <&lpass MI2S_TERTIARY>;
> +		};
> +		codec {
> +			sound-dai = <&lpass_codec 1>, <&pm8916_codec 1>;
> +		};
> +	};
> +};
> +
> +&usb {
> +	status = "okay";
> +	extcon = <&usb_id>, <&usb_id>;
> +
> +	pinctrl-names = "default", "device";
> +	pinctrl-0 = <&usb_sw_sel_pm &usb_hub_reset_pm>;
> +	pinctrl-1 = <&usb_sw_sel_pm_device &usb_hub_reset_pm_device>;
> +};
> +
> +&usb_hs_phy {
> +	extcon = <&usb_id>;
> +};
> +
> +&wcnss {
> +	status = "okay";
> +	firmware-name = "qcom/apq8016/wcnss.mbn";
> +};
> +
> +&wcnss_ctrl {
> +	firmware-name = "qcom/apq8016/WCNSS_qcom_wlan_nv_sbc.bin";
> +};
> +
> +&wcnss_iris {
> +	compatible = "qcom,wcn3620";
> +};
> +
> +&wcnss_mem {
> +	status = "okay";
> +};
> +
> +/* Enable CoreSight */
> +&cti0 { status = "okay"; };
> +&cti1 { status = "okay"; };
> +&cti12 { status = "okay"; };
> +&cti13 { status = "okay"; };
> +&cti14 { status = "okay"; };
> +&cti15 { status = "okay"; };
> +&debug0 { status = "okay"; };
> +&debug1 { status = "okay"; };
> +&debug2 { status = "okay"; };
> +&debug3 { status = "okay"; };
> +&etf { status = "okay"; };
> +&etm0 { status = "okay"; };
> +&etm1 { status = "okay"; };
> +&etm2 { status = "okay"; };
> +&etm3 { status = "okay"; };
> +&etr { status = "okay"; };
> +&funnel0 { status = "okay"; };
> +&funnel1 { status = "okay"; };
> +&replicator { status = "okay"; };
> +&stm { status = "okay"; };
> +&tpiu { status = "okay"; };
> +
> +/*
> + * 2mA drive strength is not enough when connecting multiple
> + * I2C devices with different pull up resistors.
> + */
> +
> +&blsp_i2c4_default {

None of your overrides look like have proper alphabetical order. Please
use alphabetical order.



Best regards,
Krzysztof

Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Sumit Garg]

Hi Krzysztof,

On Wed, 13 Mar 2024 at 18:30, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 13/03/2024 13:30, Sumit Garg wrote:
> > Add Schneider Electric HMIBSC board DTS. The HMIBSC board is an IIoT Edge
> > Box Core board based on the Qualcomm APQ8016E SoC.
> >
>
> ...
>
> > +
> > +/ {
> > +     model = "Schneider Electric HMIBSC Board";
> > +     compatible = "schneider,apq8016-hmibsc", "qcom,apq8016";
> > +
> > +     aliases {
> > +             mmc0 = &sdhc_1; /* eMMC */
> > +             mmc1 = &sdhc_2; /* SD card */
> > +             serial0 = &blsp_uart1;
> > +             serial1 = &blsp_uart2;
> > +             usid0 = &pm8916_0;
> > +             i2c1 = &blsp_i2c6;
> > +             i2c3 = &blsp_i2c4;
> > +             i2c4 = &blsp_i2c3;
>
> The aliases should match schematics of the board, so I assume missing
> i2c2 is intentional, right?

Yeah that is intentional as per board schematics.

>
> > +             spi0 = &blsp_spi5;
> > +     };
> > +
> > +     chosen {
> > +             stdout-path = "serial0";
> > +     };
> > +
> > +     memory@80000000 {
> > +             reg = <0 0x80000000 0 0x40000000>;
> > +     };
> > +
> > +     reserved-memory {
> > +             ramoops@bff00000 {
> > +                     compatible = "ramoops";
> > +                     reg = <0x0 0xbff00000 0x0 0x100000>;
> > +
> > +                     record-size = <0x20000>;
> > +                     console-size = <0x20000>;
> > +                     ftrace-size = <0x20000>;
> > +             };
> > +     };
> > +
> > +     usb2513 {
>
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> e.g. usb-hub
>

Okay, I will rename it to usb-hub.

>
>
> > +             compatible = "smsc,usb3503";
> > +             reset-gpios = <&pm8916_gpios 1 GPIO_ACTIVE_LOW>;
> > +             initial-mode = <1>;
> > +     };
> > +
> > +     usb_id: usb-id {
> > +             compatible = "linux,extcon-usb-gpio";
> > +             id-gpios = <&tlmm 110 GPIO_ACTIVE_HIGH>;
> > +             pinctrl-names = "default";
> > +             pinctrl-0 = <&usb_id_default>;
> > +     };
> > +
> > +     hdmi-out {
> > +             compatible = "hdmi-connector";
> > +             type = "a";
> > +
> > +             port {
> > +                     hdmi_con: endpoint {
> > +                             remote-endpoint = <&adv7533_out>;
> > +                     };
> > +             };
> > +     };
> > +
> > +     gpio-keys {
> > +             compatible = "gpio-keys";
> > +             autorepeat;
> > +
> > +             pinctrl-names = "default";
> > +             pinctrl-0 = <&msm_key_volp_n_default>;
> > +
> > +             button {
> > +                     label = "Volume Up";
> > +                     linux,code = <KEY_VOLUMEUP>;
> > +                     gpios = <&tlmm 107 GPIO_ACTIVE_LOW>;
> > +             };
> > +     };
> > +
> > +     leds {
> > +             pinctrl-names = "default";
> > +             pinctrl-0 = <&pm8916_mpps_leds>;
>
> First property is always compatible. Please apply DTS coding style rules.

Ack.

>
> > +
> > +             compatible = "gpio-leds";
> > +             #address-cells = <1>;
> > +             #size-cells = <0>;
>
> That's not a bus.
>
> It does not look like you tested the DTS against bindings. Please run
> `make dtbs_check W=1` (see
> Documentation/devicetree/bindings/writing-schema.rst or
> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> for instructions).

I assumed earlier that W=1 is sufficient for DT schema checks but it
looks like those are two different entities. However, I added these
address and size cells properties only to get rid of warnings reported
by W=1, see below:

$ make qcom/apq8016-schneider-hmibsc.dtb W=1
  DTC     arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dtb
arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts:96.9-103.5:
Warning (unit_address_vs_reg): /leds/led@5: node has a unit name, but
no reg or ranges property
arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts:105.9-112.5:
Warning (unit_address_vs_reg): /leds/led@6: node has a unit name, but
no reg or ranges property
<snip>

So it looks like W=1 is reporting false warnings and we should rather
rely on dtbs_check only.

>
> > +
> > +             led@5 {
> > +                     reg = <5>;
> > +                     label = "apq8016-hmibsc:green:wlan";
> > +                     function = LED_FUNCTION_WLAN;
> > +                     color = <LED_COLOR_ID_YELLOW>;
> > +                     gpios = <&pm8916_mpps 2 GPIO_ACTIVE_HIGH>;
> > +                     linux,default-trigger = "phy0tx";
> > +                     default-state = "off";
> > +             };
> > +
> > +             led@6 {
> > +                     reg = <6>;
> > +                     label = "apq8016-hmibsc:yellow:bt";
> > +                     function = LED_FUNCTION_BLUETOOTH;
> > +                     color = <LED_COLOR_ID_BLUE>;
> > +                     gpios = <&pm8916_mpps 3 GPIO_ACTIVE_HIGH>;
> > +                     linux,default-trigger = "bluetooth-power";
> > +                     default-state = "off";
> > +             };
> > +     };
> > +};
> > +
> > +&blsp_i2c3 {
> > +     status = "okay";
> > +
> > +     eeprom@50 {
> > +             compatible = "atmel,24c32";
> > +             reg = <0x50>;
> > +     };
> > +};
> > +
> > +&blsp_i2c4 {
> > +     status = "okay";
> > +
> > +     adv_bridge: bridge@39 {
> > +             status = "okay";
>
> Why do you need it? Was it disabled?
>
> And why this is before compatible? If this stays, please use DTS coding
> style rules for placement.

Okay I will remove it.

>
> > +
> > +             compatible = "adi,adv7533";
> > +             reg = <0x39>;
> > +
> > +             interrupt-parent = <&tlmm>;
> > +             interrupts = <31 IRQ_TYPE_EDGE_FALLING>;
> > +
> > +             adi,dsi-lanes = <4>;
> > +             clocks = <&rpmcc RPM_SMD_BB_CLK2>;
> > +             clock-names = "cec";
> > +
> > +             pd-gpios = <&tlmm 32 GPIO_ACTIVE_HIGH>;
> > +
> > +             avdd-supply = <&pm8916_l6>;
> > +             a2vdd-supply = <&pm8916_l6>;
> > +             dvdd-supply = <&pm8916_l6>;
> > +             pvdd-supply = <&pm8916_l6>;
> > +             v1p2-supply = <&pm8916_l6>;
> > +             v3p3-supply = <&pm8916_l17>;
> > +
> > +             pinctrl-names = "default","sleep";
> > +             pinctrl-0 = <&adv7533_int_active &adv7533_switch_active>;
> > +             pinctrl-1 = <&adv7533_int_suspend &adv7533_switch_suspend>;
> > +             #sound-dai-cells = <1>;
> > +
> > +             ports {
> > +                     #address-cells = <1>;
> > +                     #size-cells = <0>;
> > +
> > +                     port@0 {
> > +                             reg = <0>;
> > +                             adv7533_in: endpoint {
> > +                                     remote-endpoint = <&mdss_dsi0_out>;
> > +                             };
> > +                     };
> > +
> > +                     port@1 {
> > +                             reg = <1>;
> > +                             adv7533_out: endpoint {
> > +                                     remote-endpoint = <&hdmi_con>;
> > +                             };
> > +                     };
> > +             };
> > +     };
> > +};
> > +
> > +&blsp_i2c6 {
> > +     status = "okay";
> > +
> > +     rtc@30 {
> > +             compatible = "sii,s35390a";
> > +             reg = <0x30>;
> > +     };
> > +
> > +     eeprom@50 {
> > +             compatible = "atmel,24c256";
> > +             reg = <0x50>;
> > +     };
> > +};
> > +
> > +&blsp_spi5 {
> > +     status = "okay";
> > +     cs-gpios = <&tlmm 18 GPIO_ACTIVE_LOW>;
> > +
> > +     tpm@0 {
> > +             compatible = "tcg,tpm_tis-spi";
> > +             reg = <0>;
> > +             spi-max-frequency = <500000>;
> > +     };
> > +};
> > +
> > +&blsp_uart1 {
> > +     status = "okay";
> > +     label = "UART0";
> > +};
> > +
> > +&blsp_uart2 {
> > +     status = "okay";
> > +     label = "UART1";
> > +};
> > +
> > +&lpass {
> > +     status = "okay";
> > +};
> > +
> > +&mdss {
> > +     status = "okay";
> > +};
> > +
> > +&mdss_dsi0_out {
> > +     data-lanes = <0 1 2 3>;
> > +     remote-endpoint = <&adv7533_in>;
> > +};
> > +
> > +&pm8916_codec {
> > +     status = "okay";
> > +     qcom,mbhc-vthreshold-low = <75 150 237 450 500>;
> > +     qcom,mbhc-vthreshold-high = <75 150 237 450 500>;
> > +};
> > +
> > +&pm8916_resin {
> > +     status = "okay";
> > +     linux,code = <KEY_POWER>;
> > +};
> > +
> > +&pm8916_rpm_regulators {
> > +     pm8916_l17: l17 {
> > +             regulator-min-microvolt = <3300000>;
> > +             regulator-max-microvolt = <3300000>;
> > +     };
> > +};
> > +
> > +&sdhc_1 {
> > +     status = "okay";
> > +};
> > +
> > +&sdhc_2 {
> > +     status = "okay";
> > +
> > +     pinctrl-names = "default", "sleep";
> > +     pinctrl-0 = <&sdc2_default &sdc2_cd_default>;
> > +     pinctrl-1 = <&sdc2_sleep &sdc2_cd_default>;
> > +
> > +     cd-gpios = <&tlmm 38 GPIO_ACTIVE_LOW>;
> > +};
> > +
> > +&sound {
> > +     status = "okay";
>
> Is thi sneeded?

Yeah it is disabled by default.

>
> > +
> > +     pinctrl-0 = <&cdc_pdm_default &sec_mi2s_default>;
> > +     pinctrl-1 = <&cdc_pdm_sleep &sec_mi2s_sleep>;
> > +     pinctrl-names = "default", "sleep";
> > +     model = "DB410c";
> > +     audio-routing =
> > +             "AMIC2", "MIC BIAS Internal2",
> > +             "AMIC3", "MIC BIAS External1";
> > +
> > +     quaternary-dai-link {
> > +             link-name = "ADV7533";
> > +             cpu {
> > +                     sound-dai = <&lpass MI2S_QUATERNARY>;
> > +             };
> > +             codec {
> > +                     sound-dai = <&adv_bridge 0>;
> > +             };
> > +     };
> > +
> > +     primary-dai-link {
> > +             link-name = "WCD";
> > +             cpu {
> > +                     sound-dai = <&lpass MI2S_PRIMARY>;
> > +             };
> > +             codec {
> > +                     sound-dai = <&lpass_codec 0>, <&pm8916_codec 0>;
> > +             };
> > +     };
> > +
> > +     tertiary-dai-link {
> > +             link-name = "WCD-Capture";
> > +             cpu {
> > +                     sound-dai = <&lpass MI2S_TERTIARY>;
> > +             };
> > +             codec {
> > +                     sound-dai = <&lpass_codec 1>, <&pm8916_codec 1>;
> > +             };
> > +     };
> > +};
> > +
> > +&usb {
> > +     status = "okay";
> > +     extcon = <&usb_id>, <&usb_id>;
> > +
> > +     pinctrl-names = "default", "device";
> > +     pinctrl-0 = <&usb_sw_sel_pm &usb_hub_reset_pm>;
> > +     pinctrl-1 = <&usb_sw_sel_pm_device &usb_hub_reset_pm_device>;
> > +};
> > +
> > +&usb_hs_phy {
> > +     extcon = <&usb_id>;
> > +};
> > +
> > +&wcnss {
> > +     status = "okay";
> > +     firmware-name = "qcom/apq8016/wcnss.mbn";
> > +};
> > +
> > +&wcnss_ctrl {
> > +     firmware-name = "qcom/apq8016/WCNSS_qcom_wlan_nv_sbc.bin";
> > +};
> > +
> > +&wcnss_iris {
> > +     compatible = "qcom,wcn3620";
> > +};
> > +
> > +&wcnss_mem {
> > +     status = "okay";
> > +};
> > +
> > +/* Enable CoreSight */
> > +&cti0 { status = "okay"; };
> > +&cti1 { status = "okay"; };
> > +&cti12 { status = "okay"; };
> > +&cti13 { status = "okay"; };
> > +&cti14 { status = "okay"; };
> > +&cti15 { status = "okay"; };
> > +&debug0 { status = "okay"; };
> > +&debug1 { status = "okay"; };
> > +&debug2 { status = "okay"; };
> > +&debug3 { status = "okay"; };
> > +&etf { status = "okay"; };
> > +&etm0 { status = "okay"; };
> > +&etm1 { status = "okay"; };
> > +&etm2 { status = "okay"; };
> > +&etm3 { status = "okay"; };
> > +&etr { status = "okay"; };
> > +&funnel0 { status = "okay"; };
> > +&funnel1 { status = "okay"; };
> > +&replicator { status = "okay"; };
> > +&stm { status = "okay"; };
> > +&tpiu { status = "okay"; };
> > +
> > +/*
> > + * 2mA drive strength is not enough when connecting multiple
> > + * I2C devices with different pull up resistors.
> > + */
> > +
> > +&blsp_i2c4_default {
>
> None of your overrides look like have proper alphabetical order. Please
> use alphabetical order.
>

Although these are already following the same order as
apq8016-sbc.dts, would you like the two DTS files based on the same
SoC to follow different orders?

-Sumit

>
>
> Best regards,
> Krzysztof
>

Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Krzysztof Kozlowski]

On 14/03/2024 09:19, Sumit Garg wrote:
>>> +             compatible = "smsc,usb3503";
>>> +             reset-gpios = <&pm8916_gpios 1 GPIO_ACTIVE_LOW>;
>>> +             initial-mode = <1>;
>>> +     };
>>> +
>>> +     usb_id: usb-id {
>>> +             compatible = "linux,extcon-usb-gpio";
>>> +             id-gpios = <&tlmm 110 GPIO_ACTIVE_HIGH>;
>>> +             pinctrl-names = "default";
>>> +             pinctrl-0 = <&usb_id_default>;
>>> +     };
>>> +
>>> +     hdmi-out {
>>> +             compatible = "hdmi-connector";
>>> +             type = "a";
>>> +
>>> +             port {
>>> +                     hdmi_con: endpoint {
>>> +                             remote-endpoint = <&adv7533_out>;
>>> +                     };
>>> +             };
>>> +     };
>>> +
>>> +     gpio-keys {
>>> +             compatible = "gpio-keys";
>>> +             autorepeat;
>>> +
>>> +             pinctrl-names = "default";
>>> +             pinctrl-0 = <&msm_key_volp_n_default>;
>>> +
>>> +             button {
>>> +                     label = "Volume Up";
>>> +                     linux,code = <KEY_VOLUMEUP>;
>>> +                     gpios = <&tlmm 107 GPIO_ACTIVE_LOW>;
>>> +             };
>>> +     };
>>> +
>>> +     leds {
>>> +             pinctrl-names = "default";
>>> +             pinctrl-0 = <&pm8916_mpps_leds>;
>>
>> First property is always compatible. Please apply DTS coding style rules.
> 
> Ack.
> 
>>
>>> +
>>> +             compatible = "gpio-leds";
>>> +             #address-cells = <1>;
>>> +             #size-cells = <0>;
>>
>> That's not a bus.
>>
>> It does not look like you tested the DTS against bindings. Please run
>> `make dtbs_check W=1` (see
>> Documentation/devicetree/bindings/writing-schema.rst or
>> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
>> for instructions).
> 
> I assumed earlier that W=1 is sufficient for DT schema checks but it

W=1 as in make? No, it is not. It's flag changing the build process.
dtbs_check is separate target.

> looks like those are two different entities. However, I added these
> address and size cells properties only to get rid of warnings reported
> by W=1, see below:
> 
> $ make qcom/apq8016-schneider-hmibsc.dtb W=1
>   DTC     arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dtb
> arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts:96.9-103.5:
> Warning (unit_address_vs_reg): /leds/led@5: node has a unit name, but
> no reg or ranges property
> arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts:105.9-112.5:
> Warning (unit_address_vs_reg): /leds/led@6: node has a unit name, but
> no reg or ranges property

Wait, so you saw the warnings and ignored them? These are legitimate
warnings, although they don't give you full answer.

> <snip>
> 
> So it looks like W=1 is reporting false warnings and we should rather

Warnings were true.

> rely on dtbs_check only.

It's really independent. There is only one case where W=1 produces
warnings you could ignore (ports/port in graphs). At least I am not
aware of anything else.

Although Qualcomm does not use clean-check-maintainer-profile, but
already some archs do (RISC-V, Samsung). For these YOU MUST RUN
DTBS_CHECK and fix ALL new warnings. But even for Qualcomm, you are
expected to run dtbs_check. And why would you not run it? You can
automate checks and save reviewers time with automatic tools, but you
decide to skip it? Srsly, that's huge waste of reviewers time!

...

>>> +
>>> +&blsp_i2c4_default {
>>
>> None of your overrides look like have proper alphabetical order. Please
>> use alphabetical order.
>>
> 
> Although these are already following the same order as
> apq8016-sbc.dts, would you like the two DTS files based on the same
> SoC to follow different orders?

I don't know about Konrad and Bjorn, but to me it does not matter that
some existing board has obvious style issues. What matters to me, that
new code does not have these obvious style issues.

You can wait for Konrad's point of view on that, if you want to be sure.

Best regards,
Krzysztof

Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Sumit Garg]

On Thu, 14 Mar 2024 at 14:00, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> >>> +
> >>> +             compatible = "gpio-leds";
> >>> +             #address-cells = <1>;
> >>> +             #size-cells = <0>;
> >>
> >> That's not a bus.
> >>
> >> It does not look like you tested the DTS against bindings. Please run
> >> `make dtbs_check W=1` (see
> >> Documentation/devicetree/bindings/writing-schema.rst or
> >> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> >> for instructions).
> >
> > I assumed earlier that W=1 is sufficient for DT schema checks but it
>
> W=1 as in make? No, it is not. It's flag changing the build process.
> dtbs_check is separate target.
>
> > looks like those are two different entities. However, I added these
> > address and size cells properties only to get rid of warnings reported
> > by W=1, see below:
> >
> > $ make qcom/apq8016-schneider-hmibsc.dtb W=1
> >   DTC     arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dtb
> > arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts:96.9-103.5:
> > Warning (unit_address_vs_reg): /leds/led@5: node has a unit name, but
> > no reg or ranges property
> > arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts:105.9-112.5:
> > Warning (unit_address_vs_reg): /leds/led@6: node has a unit name, but
> > no reg or ranges property
>
> Wait, so you saw the warnings and ignored them?

Sorry but you are ignoring what I am trying to say.

> These are legitimate
> warnings, although they don't give you full answer.
>
> > <snip>
> >
> > So it looks like W=1 is reporting false warnings and we should rather
>
> Warnings were true.
>

That's was my initial impression too and I fixed them via following diff:

diff --git a/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts
b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts
index 8f9cacf8de89..a366d3aff3c5 100644
--- a/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts
+++ b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts
@@ -92,8 +92,11 @@ leds {
                pinctrl-0 = <&pm8916_mpps_leds>;

                compatible = "gpio-leds";
+               #address-cells = <1>;
+               #size-cells = <0>;

                led@5 {
+                       reg = <5>;
                        label = "apq8016-hmibsc:green:wlan";
                        function = LED_FUNCTION_WLAN;
                        color = <LED_COLOR_ID_YELLOW>;
@@ -103,6 +106,7 @@ led@5 {
                };

                led@6 {
+                       reg = <6>;
                        label = "apq8016-hmibsc:yellow:bt";
                        function = LED_FUNCTION_BLUETOOTH;
                        color = <LED_COLOR_ID_BLUE>;

But it then broke dtbs_check. Are you aware of any other saner way to
fix those warnings properly?

-Sumit

Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Sumit Garg]

On Thu, 14 Mar 2024 at 14:47, Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Thu, 14 Mar 2024 at 14:00, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
> >
> > >>> +
> > >>> +             compatible = "gpio-leds";
> > >>> +             #address-cells = <1>;
> > >>> +             #size-cells = <0>;
> > >>
> > >> That's not a bus.
> > >>
> > >> It does not look like you tested the DTS against bindings. Please run
> > >> `make dtbs_check W=1` (see
> > >> Documentation/devicetree/bindings/writing-schema.rst or
> > >> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> > >> for instructions).
> > >
> > > I assumed earlier that W=1 is sufficient for DT schema checks but it
> >
> > W=1 as in make? No, it is not. It's flag changing the build process.
> > dtbs_check is separate target.
> >
> > > looks like those are two different entities. However, I added these
> > > address and size cells properties only to get rid of warnings reported
> > > by W=1, see below:
> > >
> > > $ make qcom/apq8016-schneider-hmibsc.dtb W=1
> > >   DTC     arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dtb
> > > arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts:96.9-103.5:
> > > Warning (unit_address_vs_reg): /leds/led@5: node has a unit name, but
> > > no reg or ranges property
> > > arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts:105.9-112.5:
> > > Warning (unit_address_vs_reg): /leds/led@6: node has a unit name, but
> > > no reg or ranges property
> >
> > Wait, so you saw the warnings and ignored them?
>
> Sorry but you are ignoring what I am trying to say.
>
> > These are legitimate
> > warnings, although they don't give you full answer.
> >
> > > <snip>
> > >
> > > So it looks like W=1 is reporting false warnings and we should rather
> >
> > Warnings were true.
> >
>
> That's was my initial impression too and I fixed them via following diff:
>
> diff --git a/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts
> b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts
> index 8f9cacf8de89..a366d3aff3c5 100644
> --- a/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts
> +++ b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts
> @@ -92,8 +92,11 @@ leds {
>                 pinctrl-0 = <&pm8916_mpps_leds>;
>
>                 compatible = "gpio-leds";
> +               #address-cells = <1>;
> +               #size-cells = <0>;
>
>                 led@5 {
> +                       reg = <5>;
>                         label = "apq8016-hmibsc:green:wlan";
>                         function = LED_FUNCTION_WLAN;
>                         color = <LED_COLOR_ID_YELLOW>;
> @@ -103,6 +106,7 @@ led@5 {
>                 };
>
>                 led@6 {
> +                       reg = <6>;
>                         label = "apq8016-hmibsc:yellow:bt";
>                         function = LED_FUNCTION_BLUETOOTH;
>                         color = <LED_COLOR_ID_BLUE>;
>
> But it then broke dtbs_check.

See following breakage afterwards:

$ make qcom/apq8016-schneider-hmibsc.dtb dtbs_check
<snip>
/home/sumit/build/upstream/linux/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dtb:
leds: led@5: Unevaluated properties are not allowed ('reg' was
unexpected)
from schema $id: http://devicetree.org/schemas/leds/leds-gpio.yaml#
/home/sumit/build/upstream/linux/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dtb:
leds: led@6: Unevaluated properties are not allowed ('reg' was
unexpected)
from schema $id: http://devicetree.org/schemas/leds/leds-gpio.yaml#
/home/sumit/build/upstream/linux/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dtb:
leds: '#address-cells', '#size-cells' do not match any of the regexes:
'(^led-[0-9a-f]$|led)', 'pinctrl-[0-9]+'
<snip>

> Are you aware of any other saner way to
> fix those warnings properly?
>

-Sumit

Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Krzysztof Kozlowski]

On 14/03/2024 10:36, Sumit Garg wrote:
>>
>> But it then broke dtbs_check.
> 
> See following breakage afterwards:
> 
> $ make qcom/apq8016-schneider-hmibsc.dtb dtbs_check
> <snip>
> /home/sumit/build/upstream/linux/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dtb:
> leds: led@5: Unevaluated properties are not allowed ('reg' was
> unexpected)
> from schema $id: http://devicetree.org/schemas/leds/leds-gpio.yaml#
> /home/sumit/build/upstream/linux/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dtb:
> leds: led@6: Unevaluated properties are not allowed ('reg' was
> unexpected)
> from schema $id: http://devicetree.org/schemas/leds/leds-gpio.yaml#
> /home/sumit/build/upstream/linux/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dtb:
> leds: '#address-cells', '#size-cells' do not match any of the regexes:
> '(^led-[0-9a-f]$|led)', 'pinctrl-[0-9]+'

That's obvious, I don't get what is the question. Adding not correct
properties is not a solution and dtbs_check correctly tells you that.

If you only opened absolutely any existing upstream source, you would
see how the gpio leds are represented in DTS.

Best regards,
Krzysztof

Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Sumit Garg]

On Thu, 14 Mar 2024 at 15:36, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 14/03/2024 10:36, Sumit Garg wrote:
> >>
> >> But it then broke dtbs_check.
> >
> > See following breakage afterwards:
> >
> > $ make qcom/apq8016-schneider-hmibsc.dtb dtbs_check
> > <snip>
> > /home/sumit/build/upstream/linux/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dtb:
> > leds: led@5: Unevaluated properties are not allowed ('reg' was
> > unexpected)
> > from schema $id: http://devicetree.org/schemas/leds/leds-gpio.yaml#
> > /home/sumit/build/upstream/linux/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dtb:
> > leds: led@6: Unevaluated properties are not allowed ('reg' was
> > unexpected)
> > from schema $id: http://devicetree.org/schemas/leds/leds-gpio.yaml#
> > /home/sumit/build/upstream/linux/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dtb:
> > leds: '#address-cells', '#size-cells' do not match any of the regexes:
> > '(^led-[0-9a-f]$|led)', 'pinctrl-[0-9]+'
>
> That's obvious, I don't get what is the question. Adding not correct
> properties is not a solution and dtbs_check correctly tells you that.
>
> If you only opened absolutely any existing upstream source, you would
> see how the gpio leds are represented in DTS.
>

It looks like my reference example:
arch/arm64/boot/dts/qcom/apq8016-sbc.dts wasn't correct then. I will
drop unit addresses then.

-Sumit

> Best regards,
> Krzysztof
>

Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Krzysztof Kozlowski]

On 14/03/2024 11:26, Sumit Garg wrote:
> On Thu, 14 Mar 2024 at 15:36, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 14/03/2024 10:36, Sumit Garg wrote:
>>>>
>>>> But it then broke dtbs_check.
>>>
>>> See following breakage afterwards:
>>>
>>> $ make qcom/apq8016-schneider-hmibsc.dtb dtbs_check
>>> <snip>
>>> /home/sumit/build/upstream/linux/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dtb:
>>> leds: led@5: Unevaluated properties are not allowed ('reg' was
>>> unexpected)
>>> from schema $id: http://devicetree.org/schemas/leds/leds-gpio.yaml#
>>> /home/sumit/build/upstream/linux/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dtb:
>>> leds: led@6: Unevaluated properties are not allowed ('reg' was
>>> unexpected)
>>> from schema $id: http://devicetree.org/schemas/leds/leds-gpio.yaml#
>>> /home/sumit/build/upstream/linux/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dtb:
>>> leds: '#address-cells', '#size-cells' do not match any of the regexes:
>>> '(^led-[0-9a-f]$|led)', 'pinctrl-[0-9]+'
>>
>> That's obvious, I don't get what is the question. Adding not correct
>> properties is not a solution and dtbs_check correctly tells you that.
>>
>> If you only opened absolutely any existing upstream source, you would
>> see how the gpio leds are represented in DTS.
>>
> 
> It looks like my reference example:
> arch/arm64/boot/dts/qcom/apq8016-sbc.dts wasn't correct then. I will
> drop unit addresses then.

Oops, unlucky for you, that is one of old DTS which was not yet fixed. I
am surprised that I missed it, I'll fix it now.

Best regards,
Krzysztof

Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Dmitry Baryshkov]

On Thu, 14 Mar 2024 at 11:37, Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Thu, 14 Mar 2024 at 14:47, Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > On Thu, 14 Mar 2024 at 14:00, Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> > >
> > > >>> +
> > > >>> +             compatible = "gpio-leds";
> > > >>> +             #address-cells = <1>;
> > > >>> +             #size-cells = <0>;
> > > >>
> > > >> That's not a bus.
> > > >>
> > > >> It does not look like you tested the DTS against bindings. Please run
> > > >> `make dtbs_check W=1` (see
> > > >> Documentation/devicetree/bindings/writing-schema.rst or
> > > >> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> > > >> for instructions).
> > > >
> > > > I assumed earlier that W=1 is sufficient for DT schema checks but it
> > >
> > > W=1 as in make? No, it is not. It's flag changing the build process.
> > > dtbs_check is separate target.
> > >
> > > > looks like those are two different entities. However, I added these
> > > > address and size cells properties only to get rid of warnings reported
> > > > by W=1, see below:
> > > >
> > > > $ make qcom/apq8016-schneider-hmibsc.dtb W=1
> > > >   DTC     arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dtb
> > > > arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts:96.9-103.5:
> > > > Warning (unit_address_vs_reg): /leds/led@5: node has a unit name, but
> > > > no reg or ranges property
> > > > arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts:105.9-112.5:
> > > > Warning (unit_address_vs_reg): /leds/led@6: node has a unit name, but
> > > > no reg or ranges property
> > >
> > > Wait, so you saw the warnings and ignored them?
> >
> > Sorry but you are ignoring what I am trying to say.
> >
> > > These are legitimate
> > > warnings, although they don't give you full answer.
> > >
> > > > <snip>
> > > >
> > > > So it looks like W=1 is reporting false warnings and we should rather
> > >
> > > Warnings were true.
> > >
> >
> > That's was my initial impression too and I fixed them via following diff:
> >
> > diff --git a/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts
> > b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts
> > index 8f9cacf8de89..a366d3aff3c5 100644
> > --- a/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts
> > +++ b/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dts
> > @@ -92,8 +92,11 @@ leds {
> >                 pinctrl-0 = <&pm8916_mpps_leds>;
> >
> >                 compatible = "gpio-leds";
> > +               #address-cells = <1>;
> > +               #size-cells = <0>;
> >
> >                 led@5 {
> > +                       reg = <5>;
> >                         label = "apq8016-hmibsc:green:wlan";
> >                         function = LED_FUNCTION_WLAN;
> >                         color = <LED_COLOR_ID_YELLOW>;
> > @@ -103,6 +106,7 @@ led@5 {
> >                 };
> >
> >                 led@6 {
> > +                       reg = <6>;
> >                         label = "apq8016-hmibsc:yellow:bt";
> >                         function = LED_FUNCTION_BLUETOOTH;
> >                         color = <LED_COLOR_ID_BLUE>;
> >
> > But it then broke dtbs_check.
>
> See following breakage afterwards:
>
> $ make qcom/apq8016-schneider-hmibsc.dtb dtbs_check
> <snip>
> /home/sumit/build/upstream/linux/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dtb:
> leds: led@5: Unevaluated properties are not allowed ('reg' was
> unexpected)
> from schema $id: http://devicetree.org/schemas/leds/leds-gpio.yaml#
> /home/sumit/build/upstream/linux/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dtb:
> leds: led@6: Unevaluated properties are not allowed ('reg' was
> unexpected)
> from schema $id: http://devicetree.org/schemas/leds/leds-gpio.yaml#
> /home/sumit/build/upstream/linux/arch/arm64/boot/dts/qcom/apq8016-schneider-hmibsc.dtb:
> leds: '#address-cells', '#size-cells' do not match any of the regexes:
> '(^led-[0-9a-f]$|led)', 'pinctrl-[0-9]+'
> <snip>

That's because there is no addressing for gpio-leds. Just use names as
led-5, led-6.

>
> > Are you aware of any other saner way to
> > fix those warnings properly?
> >
>
> -Sumit
>


-- 
With best wishes
Dmitry

Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Krzysztof Kozlowski]

On 14/03/2024 10:17, Sumit Garg wrote:
> 
>                 compatible = "gpio-leds";
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> 
>                 led@5 {
> +                       reg = <5>;
>                         label = "apq8016-hmibsc:green:wlan";
>                         function = LED_FUNCTION_WLAN;
>                         color = <LED_COLOR_ID_YELLOW>;
> @@ -103,6 +106,7 @@ led@5 {
>                 };
> 
>                 led@6 {
> +                       reg = <6>;
>                         label = "apq8016-hmibsc:yellow:bt";
>                         function = LED_FUNCTION_BLUETOOTH;
>                         color = <LED_COLOR_ID_BLUE>;
> 
> But it then broke dtbs_check. Are you aware of any other saner way to
> fix those warnings properly?

You must drop unit address, because it is not applicable, instead of
adding some properties to mask it.

Best regards,
Krzysztof

Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Konrad Dybcio]

On 3/13/24 13:30, Sumit Garg wrote:
> Add Schneider Electric HMIBSC board DTS. The HMIBSC board is an IIoT Edge
> Box Core board based on the Qualcomm APQ8016E SoC.
> 
> Support for Schneider Electric HMIBSC. Features:
> - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306)
> - 1GiB RAM
> - 8GiB eMMC, SD slot
> - WiFi and Bluetooth
> - 2x Host, 1x Device USB port
> - HDMI
> - Discrete TPM2 chip over SPI
> - USB ethernet adaptors (soldered)
> 
> Co-developed-by: Jagdish Gediya <jagdish.gediya@linaro.org>
> Signed-off-by: Jagdish Gediya <jagdish.gediya@linaro.org>
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> ---

[...]

> +	memory@80000000 {
> +		reg = <0 0x80000000 0 0x40000000>;
> +	};

I'm not sure the entirety of DRAM is accessible..

This override should be unnecessary, as bootloaders generally update
the size field anyway.

> +
> +	reserved-memory {
> +		ramoops@bff00000 {
> +			compatible = "ramoops";
> +			reg = <0x0 0xbff00000 0x0 0x100000>;
> +
> +			record-size = <0x20000>;
> +			console-size = <0x20000>;
> +			ftrace-size = <0x20000>;

No ecc?

> +		};
> +	};
> +
> +	usb2513 {

Please use a generic node name, like usb-hub and keep the nodes
sorted.

> +		compatible = "smsc,usb3503";
> +		reset-gpios = <&pm8916_gpios 1 GPIO_ACTIVE_LOW>;
> +		initial-mode = <1>;
> +	};
> +
> +	usb_id: usb-id {
> +		compatible = "linux,extcon-usb-gpio";
> +		id-gpios = <&tlmm 110 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&usb_id_default>;

property-n
property-names

[...]

> +		led@5 {
> +			reg = <5>;
> +			label = "apq8016-hmibsc:green:wlan";

These names look overly complicated.. s/apq8016-hmibsc://g?


> +
> +&blsp_i2c3 {
> +	status = "okay";
> +
> +	eeprom@50 {
> +		compatible = "atmel,24c32";
> +		reg = <0x50>;
> +	};
> +};
> +
> +&blsp_i2c4 {
> +	status = "okay";
> +
> +	adv_bridge: bridge@39 {
> +		status = "okay";

???

> +
> +		compatible = "adi,adv7533";
> +		reg = <0x39>;
> +
> +		interrupt-parent = <&tlmm>;
> +		interrupts = <31 IRQ_TYPE_EDGE_FALLING>;

interrupts-extended

Konrad

Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Krzysztof Kozlowski]

On 13/03/2024 14:04, Konrad Dybcio wrote:
> 
>> +		led@5 {
>> +			reg = <5>;
>> +			label = "apq8016-hmibsc:green:wlan";
> 
> These names look overly complicated.. s/apq8016-hmibsc://g?
> 

It should be dropped entirely in fact. There is color and function.

Best regards,
Krzysztof

Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Sumit Garg]

Hi Konrad,

On Wed, 13 Mar 2024 at 18:34, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
>
>
> On 3/13/24 13:30, Sumit Garg wrote:
> > Add Schneider Electric HMIBSC board DTS. The HMIBSC board is an IIoT Edge
> > Box Core board based on the Qualcomm APQ8016E SoC.
> >
> > Support for Schneider Electric HMIBSC. Features:
> > - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306)
> > - 1GiB RAM
> > - 8GiB eMMC, SD slot
> > - WiFi and Bluetooth
> > - 2x Host, 1x Device USB port
> > - HDMI
> > - Discrete TPM2 chip over SPI
> > - USB ethernet adaptors (soldered)
> >
> > Co-developed-by: Jagdish Gediya <jagdish.gediya@linaro.org>
> > Signed-off-by: Jagdish Gediya <jagdish.gediya@linaro.org>
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > ---
>
> [...]
>
> > +     memory@80000000 {
> > +             reg = <0 0x80000000 0 0x40000000>;
> > +     };
>
> I'm not sure the entirety of DRAM is accessible..
>
> This override should be unnecessary, as bootloaders generally update
> the size field anyway.

On this board, U-Boot is used as the first stage bootloader (replacing
Little Kernel (LK), thanks to Stephan's work). And U-Boot consumes
memory range from DT as Linux does but doesn't require any memory to
be reserved for U-Boot itself. So apart from reserved memory nodes
explicitly described in DT all the other DRAM regions are accessible.

>
> > +
> > +     reserved-memory {
> > +             ramoops@bff00000 {
> > +                     compatible = "ramoops";
> > +                     reg = <0x0 0xbff00000 0x0 0x100000>;
> > +
> > +                     record-size = <0x20000>;
> > +                     console-size = <0x20000>;
> > +                     ftrace-size = <0x20000>;
>
> No ecc?

Okay I can add that.

>
> > +             };
> > +     };
> > +
> > +     usb2513 {
>
> Please use a generic node name, like usb-hub

Ack.

> and keep the nodes
> sorted.

See other email thread...

>
> > +             compatible = "smsc,usb3503";
> > +             reset-gpios = <&pm8916_gpios 1 GPIO_ACTIVE_LOW>;
> > +             initial-mode = <1>;
> > +     };
> > +
> > +     usb_id: usb-id {
> > +             compatible = "linux,extcon-usb-gpio";
> > +             id-gpios = <&tlmm 110 GPIO_ACTIVE_HIGH>;
> > +             pinctrl-names = "default";
> > +             pinctrl-0 = <&usb_id_default>;
>
> property-n
> property-names

Ack.

>
> [...]
>
> > +             led@5 {
> > +                     reg = <5>;
> > +                     label = "apq8016-hmibsc:green:wlan";
>
> These names look overly complicated.. s/apq8016-hmibsc://g?
>

I will drop them as suggested by Krzysztof.

>
> > +
> > +&blsp_i2c3 {
> > +     status = "okay";
> > +
> > +     eeprom@50 {
> > +             compatible = "atmel,24c32";
> > +             reg = <0x50>;
> > +     };
> > +};
> > +
> > +&blsp_i2c4 {
> > +     status = "okay";
> > +
> > +     adv_bridge: bridge@39 {
> > +             status = "okay";
>
> ???

Will drop it.

>
> > +
> > +             compatible = "adi,adv7533";
> > +             reg = <0x39>;
> > +
> > +             interrupt-parent = <&tlmm>;
> > +             interrupts = <31 IRQ_TYPE_EDGE_FALLING>;
>
> interrupts-extended
>

Please see Documentation/devicetree/bindings/display/bridge/adi,adv7533.yaml.

-Sumit

> Konrad

Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Konrad Dybcio]

On 3/14/24 10:04, Sumit Garg wrote:
> Hi Konrad,
> 
> On Wed, 13 Mar 2024 at 18:34, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>
>>
>>
>> On 3/13/24 13:30, Sumit Garg wrote:
>>> Add Schneider Electric HMIBSC board DTS. The HMIBSC board is an IIoT Edge
>>> Box Core board based on the Qualcomm APQ8016E SoC.
>>>
>>> Support for Schneider Electric HMIBSC. Features:
>>> - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306)
>>> - 1GiB RAM
>>> - 8GiB eMMC, SD slot
>>> - WiFi and Bluetooth
>>> - 2x Host, 1x Device USB port
>>> - HDMI
>>> - Discrete TPM2 chip over SPI
>>> - USB ethernet adaptors (soldered)
>>>
>>> Co-developed-by: Jagdish Gediya <jagdish.gediya@linaro.org>
>>> Signed-off-by: Jagdish Gediya <jagdish.gediya@linaro.org>
>>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
>>> ---
>>
>> [...]
>>
>>> +     memory@80000000 {
>>> +             reg = <0 0x80000000 0 0x40000000>;
>>> +     };
>>
>> I'm not sure the entirety of DRAM is accessible..
>>
>> This override should be unnecessary, as bootloaders generally update
>> the size field anyway.
> 
> On this board, U-Boot is used as the first stage bootloader (replacing
> Little Kernel (LK), thanks to Stephan's work). And U-Boot consumes
> memory range from DT as Linux does but doesn't require any memory to
> be reserved for U-Boot itself. So apart from reserved memory nodes
> explicitly described in DT all the other DRAM regions are accessible.

Still, u-boot has code to fetch the size dynamically, no?

[...]

>>
>>> +
>>> +             compatible = "adi,adv7533";
>>> +             reg = <0x39>;
>>> +
>>> +             interrupt-parent = <&tlmm>;
>>> +             interrupts = <31 IRQ_TYPE_EDGE_FALLING>;
>>
>> interrupts-extended
>>
> 
> Please see Documentation/devicetree/bindings/display/bridge/adi,adv7533.yaml.

Okay, and what am I supposed to see there?

Konrad

Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Sumit Garg]

On Thu, 14 Mar 2024 at 14:48, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
>
>
> On 3/14/24 10:04, Sumit Garg wrote:
> > Hi Konrad,
> >
> > On Wed, 13 Mar 2024 at 18:34, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> >>
> >>
> >>
> >> On 3/13/24 13:30, Sumit Garg wrote:
> >>> Add Schneider Electric HMIBSC board DTS. The HMIBSC board is an IIoT Edge
> >>> Box Core board based on the Qualcomm APQ8016E SoC.
> >>>
> >>> Support for Schneider Electric HMIBSC. Features:
> >>> - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306)
> >>> - 1GiB RAM
> >>> - 8GiB eMMC, SD slot
> >>> - WiFi and Bluetooth
> >>> - 2x Host, 1x Device USB port
> >>> - HDMI
> >>> - Discrete TPM2 chip over SPI
> >>> - USB ethernet adaptors (soldered)
> >>>
> >>> Co-developed-by: Jagdish Gediya <jagdish.gediya@linaro.org>
> >>> Signed-off-by: Jagdish Gediya <jagdish.gediya@linaro.org>
> >>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> >>> ---
> >>
> >> [...]
> >>
> >>> +     memory@80000000 {
> >>> +             reg = <0 0x80000000 0 0x40000000>;
> >>> +     };
> >>
> >> I'm not sure the entirety of DRAM is accessible..
> >>
> >> This override should be unnecessary, as bootloaders generally update
> >> the size field anyway.
> >
> > On this board, U-Boot is used as the first stage bootloader (replacing
> > Little Kernel (LK), thanks to Stephan's work). And U-Boot consumes
> > memory range from DT as Linux does but doesn't require any memory to
> > be reserved for U-Boot itself. So apart from reserved memory nodes
> > explicitly described in DT all the other DRAM regions are accessible.
>
> Still, u-boot has code to fetch the size dynamically, no?
>

No U-Boot being the first stage bootloader fetches size from DT which
is bundled into U-Boot binary.

> [...]
>
> >>
> >>> +
> >>> +             compatible = "adi,adv7533";
> >>> +             reg = <0x39>;
> >>> +
> >>> +             interrupt-parent = <&tlmm>;
> >>> +             interrupts = <31 IRQ_TYPE_EDGE_FALLING>;
> >>
> >> interrupts-extended
> >>
> >
> > Please see Documentation/devicetree/bindings/display/bridge/adi,adv7533.yaml.
>
> Okay, and what am I supposed to see there?

I meant you to refer to an example there but looks like
interrupts-extended is a valid replacement too. I will use that
instead.

-Sumit

>
> Konrad

Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Krzysztof Kozlowski]

On 14/03/2024 10:32, Sumit Garg wrote:
>>>>
>>>>> +
>>>>> +             compatible = "adi,adv7533";
>>>>> +             reg = <0x39>;
>>>>> +
>>>>> +             interrupt-parent = <&tlmm>;
>>>>> +             interrupts = <31 IRQ_TYPE_EDGE_FALLING>;
>>>>
>>>> interrupts-extended
>>>>
>>>
>>> Please see Documentation/devicetree/bindings/display/bridge/adi,adv7533.yaml.
>>
>> Okay, and what am I supposed to see there?
> 
> I meant you to refer to an example there but looks like
> interrupts-extended is a valid replacement too. I will use that

That's just an example. It does not matter as an argument whether code
can be done better.

Best regards,
Krzysztof

Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Stephan Gerhold]

On Thu, Mar 14, 2024 at 03:02:31PM +0530, Sumit Garg wrote:
> On Thu, 14 Mar 2024 at 14:48, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> > On 3/14/24 10:04, Sumit Garg wrote:
> > > On Wed, 13 Mar 2024 at 18:34, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> > >> On 3/13/24 13:30, Sumit Garg wrote:
> > >>> Add Schneider Electric HMIBSC board DTS. The HMIBSC board is an IIoT Edge
> > >>> Box Core board based on the Qualcomm APQ8016E SoC.
> > >>>
> > >>> Support for Schneider Electric HMIBSC. Features:
> > >>> - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306)
> > >>> - 1GiB RAM
> > >>> - 8GiB eMMC, SD slot
> > >>> - WiFi and Bluetooth
> > >>> - 2x Host, 1x Device USB port
> > >>> - HDMI
> > >>> - Discrete TPM2 chip over SPI
> > >>> - USB ethernet adaptors (soldered)
> > >>>
> > >>> Co-developed-by: Jagdish Gediya <jagdish.gediya@linaro.org>
> > >>> Signed-off-by: Jagdish Gediya <jagdish.gediya@linaro.org>
> > >>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > >>> ---
> > >>
> > >> [...]
> > >>
> > >>> +     memory@80000000 {
> > >>> +             reg = <0 0x80000000 0 0x40000000>;
> > >>> +     };
> > >>
> > >> I'm not sure the entirety of DRAM is accessible..
> > >>
> > >> This override should be unnecessary, as bootloaders generally update
> > >> the size field anyway.
> > >
> > > On this board, U-Boot is used as the first stage bootloader (replacing
> > > Little Kernel (LK), thanks to Stephan's work). And U-Boot consumes
> > > memory range from DT as Linux does but doesn't require any memory to
> > > be reserved for U-Boot itself. So apart from reserved memory nodes
> > > explicitly described in DT all the other DRAM regions are accessible.
> >
> > Still, u-boot has code to fetch the size dynamically, no?
> >
> 
> No U-Boot being the first stage bootloader fetches size from DT which
> is bundled into U-Boot binary.
> 

Back when I added support for using U-Boot as first stage bootloader on
DB410c the way it worked is that U-Boot used a fixed amount of DRAM
(originally 968 MiB, later 1 GiB since I fixed this in commit
1d667227ea51 ("board: dragonboard410c: Fix PHYS_SDRAM_1_SIZE") [1]).
When booting Linux, the Linux DT was dynamically patched with the right
amount of DRAM (obtained from SMEM). So if you had e.g. a Geniatech DB4
board with 2 GiB DRAM, U-Boot was only using 1 GiB of DRAM, but Linux
later got the full 2 GiB patched into its DTB.

I didn't have much time for testing U-Boot myself lately but a quick
look at the recent changes suggest that Caleb accidentally removed that
functionality in the recent cleanup. Specifically, the SMEM-based DRAM
size detection was removed in commit 14868845db54 ("board:
dragonboard410c: import board code from mach-snapdragon" [2]), the
msm_fixup_memory() function does not seem to exist anymore now. :')

Also, the DRAM size is now always taken from the DT (which is probably
better than the previous hardcoded size in the U-Boot board code).

I think we should bring the dynamic DRAM size detection back, because
there are quite some boards available with varying DRAM size. Restoring
msm_fixup_memory() would likely be easiest, I guess the ideal solution
would be to parse SMEM in U-Boot's dram_init() function so even U-Boot
has the correct amount of DRAM to work with.

Thanks,
Stephan

[1]: https://source.denx.de/u-boot/u-boot/-/commit/1d667227ea512537b8453abeb49abbf19a1a18e8
[2]: https://source.denx.de/u-boot/u-boot/-/commit/14868845db54b4f64701977385dc9a6e951e4139

Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Sumit Garg]

On Thu, 14 Mar 2024 at 16:13, Stephan Gerhold <stephan@gerhold.net> wrote:
>
> On Thu, Mar 14, 2024 at 03:02:31PM +0530, Sumit Garg wrote:
> > On Thu, 14 Mar 2024 at 14:48, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> > > On 3/14/24 10:04, Sumit Garg wrote:
> > > > On Wed, 13 Mar 2024 at 18:34, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> > > >> On 3/13/24 13:30, Sumit Garg wrote:
> > > >>> Add Schneider Electric HMIBSC board DTS. The HMIBSC board is an IIoT Edge
> > > >>> Box Core board based on the Qualcomm APQ8016E SoC.
> > > >>>
> > > >>> Support for Schneider Electric HMIBSC. Features:
> > > >>> - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306)
> > > >>> - 1GiB RAM
> > > >>> - 8GiB eMMC, SD slot
> > > >>> - WiFi and Bluetooth
> > > >>> - 2x Host, 1x Device USB port
> > > >>> - HDMI
> > > >>> - Discrete TPM2 chip over SPI
> > > >>> - USB ethernet adaptors (soldered)
> > > >>>
> > > >>> Co-developed-by: Jagdish Gediya <jagdish.gediya@linaro.org>
> > > >>> Signed-off-by: Jagdish Gediya <jagdish.gediya@linaro.org>
> > > >>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > > >>> ---
> > > >>
> > > >> [...]
> > > >>
> > > >>> +     memory@80000000 {
> > > >>> +             reg = <0 0x80000000 0 0x40000000>;
> > > >>> +     };
> > > >>
> > > >> I'm not sure the entirety of DRAM is accessible..
> > > >>
> > > >> This override should be unnecessary, as bootloaders generally update
> > > >> the size field anyway.
> > > >
> > > > On this board, U-Boot is used as the first stage bootloader (replacing
> > > > Little Kernel (LK), thanks to Stephan's work). And U-Boot consumes
> > > > memory range from DT as Linux does but doesn't require any memory to
> > > > be reserved for U-Boot itself. So apart from reserved memory nodes
> > > > explicitly described in DT all the other DRAM regions are accessible.
> > >
> > > Still, u-boot has code to fetch the size dynamically, no?
> > >
> >
> > No U-Boot being the first stage bootloader fetches size from DT which
> > is bundled into U-Boot binary.
> >
>
> Back when I added support for using U-Boot as first stage bootloader on
> DB410c the way it worked is that U-Boot used a fixed amount of DRAM
> (originally 968 MiB, later 1 GiB since I fixed this in commit
> 1d667227ea51 ("board: dragonboard410c: Fix PHYS_SDRAM_1_SIZE") [1]).
> When booting Linux, the Linux DT was dynamically patched with the right
> amount of DRAM (obtained from SMEM). So if you had e.g. a Geniatech DB4
> board with 2 GiB DRAM, U-Boot was only using 1 GiB of DRAM, but Linux
> later got the full 2 GiB patched into its DTB.
>
> I didn't have much time for testing U-Boot myself lately but a quick
> look at the recent changes suggest that Caleb accidentally removed that
> functionality in the recent cleanup. Specifically, the SMEM-based DRAM
> size detection was removed in commit 14868845db54 ("board:
> dragonboard410c: import board code from mach-snapdragon" [2]), the
> msm_fixup_memory() function does not seem to exist anymore now. :')

Ah now I see the reasoning for that particular piece of code. Is SMEM
based approach the standardized way used by early stage boot-loaders
on other Qcom SoCs too?

>
> Also, the DRAM size is now always taken from the DT (which is probably
> better than the previous hardcoded size in the U-Boot board code).
>
> I think we should bring the dynamic DRAM size detection back, because
> there are quite some boards available with varying DRAM size. Restoring
> msm_fixup_memory() would likely be easiest, I guess the ideal solution
> would be to parse SMEM in U-Boot's dram_init() function so even U-Boot
> has the correct amount of DRAM to work with.

In the context of the HMIBSC board, it has 1 GB RAM LPDDR3 internal
not expandable. IMO, having it in DT as default should be fine.

-Sumit

>
> Thanks,
> Stephan
>
> [1]: https://source.denx.de/u-boot/u-boot/-/commit/1d667227ea512537b8453abeb49abbf19a1a18e8
> [2]: https://source.denx.de/u-boot/u-boot/-/commit/14868845db54b4f64701977385dc9a6e951e4139

Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Stephan Gerhold]

On Thu, Mar 14, 2024 at 05:26:27PM +0530, Sumit Garg wrote:
> On Thu, 14 Mar 2024 at 16:13, Stephan Gerhold <stephan@gerhold.net> wrote:
> > On Thu, Mar 14, 2024 at 03:02:31PM +0530, Sumit Garg wrote:
> > > On Thu, 14 Mar 2024 at 14:48, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> > > > On 3/14/24 10:04, Sumit Garg wrote:
> > > > > On Wed, 13 Mar 2024 at 18:34, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> > > > >> On 3/13/24 13:30, Sumit Garg wrote:
> > > > >>> Add Schneider Electric HMIBSC board DTS. The HMIBSC board is an IIoT Edge
> > > > >>> Box Core board based on the Qualcomm APQ8016E SoC.
> > > > >>>
> > > > >>> Support for Schneider Electric HMIBSC. Features:
> > > > >>> - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306)
> > > > >>> - 1GiB RAM
> > > > >>> - 8GiB eMMC, SD slot
> > > > >>> - WiFi and Bluetooth
> > > > >>> - 2x Host, 1x Device USB port
> > > > >>> - HDMI
> > > > >>> - Discrete TPM2 chip over SPI
> > > > >>> - USB ethernet adaptors (soldered)
> > > > >>>
> > > > >>> Co-developed-by: Jagdish Gediya <jagdish.gediya@linaro.org>
> > > > >>> Signed-off-by: Jagdish Gediya <jagdish.gediya@linaro.org>
> > > > >>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > > > >>> ---
> > > > >>
> > > > >> [...]
> > > > >>
> > > > >>> +     memory@80000000 {
> > > > >>> +             reg = <0 0x80000000 0 0x40000000>;
> > > > >>> +     };
> > > > >>
> > > > >> I'm not sure the entirety of DRAM is accessible..
> > > > >>
> > > > >> This override should be unnecessary, as bootloaders generally update
> > > > >> the size field anyway.
> > > > >
> > > > > On this board, U-Boot is used as the first stage bootloader (replacing
> > > > > Little Kernel (LK), thanks to Stephan's work). And U-Boot consumes
> > > > > memory range from DT as Linux does but doesn't require any memory to
> > > > > be reserved for U-Boot itself. So apart from reserved memory nodes
> > > > > explicitly described in DT all the other DRAM regions are accessible.
> > > >
> > > > Still, u-boot has code to fetch the size dynamically, no?
> > > >
> > >
> > > No U-Boot being the first stage bootloader fetches size from DT which
> > > is bundled into U-Boot binary.
> > >
> >
> > Back when I added support for using U-Boot as first stage bootloader on
> > DB410c the way it worked is that U-Boot used a fixed amount of DRAM
> > (originally 968 MiB, later 1 GiB since I fixed this in commit
> > 1d667227ea51 ("board: dragonboard410c: Fix PHYS_SDRAM_1_SIZE") [1]).
> > When booting Linux, the Linux DT was dynamically patched with the right
> > amount of DRAM (obtained from SMEM). So if you had e.g. a Geniatech DB4
> > board with 2 GiB DRAM, U-Boot was only using 1 GiB of DRAM, but Linux
> > later got the full 2 GiB patched into its DTB.
> >
> > I didn't have much time for testing U-Boot myself lately but a quick
> > look at the recent changes suggest that Caleb accidentally removed that
> > functionality in the recent cleanup. Specifically, the SMEM-based DRAM
> > size detection was removed in commit 14868845db54 ("board:
> > dragonboard410c: import board code from mach-snapdragon" [2]), the
> > msm_fixup_memory() function does not seem to exist anymore now. :')
> 
> Ah now I see the reasoning for that particular piece of code. Is SMEM
> based approach the standardized way used by early stage boot-loaders
> on other Qcom SoCs too?
> 

It is definitely used on all the SoCs that were deployed with LK. I am
not entirely sure about the newer ABL/UEFI-based ones. A quick look at
the ABL source code suggests it is abstracted through an EFI protocol
there (so we cannot see where the information comes from with just the
open-source code). However, in my experience SMEM data structures are
usually kept quite stable (or properly versioned), so it is quite likely
that we could use this approach for all Qualcomm SoCs.

> >
> > Also, the DRAM size is now always taken from the DT (which is probably
> > better than the previous hardcoded size in the U-Boot board code).
> >
> > I think we should bring the dynamic DRAM size detection back, because
> > there are quite some boards available with varying DRAM size. Restoring
> > msm_fixup_memory() would likely be easiest, I guess the ideal solution
> > would be to parse SMEM in U-Boot's dram_init() function so even U-Boot
> > has the correct amount of DRAM to work with.
> 
> In the context of the HMIBSC board, it has 1 GB RAM LPDDR3 internal
> not expandable. IMO, having it in DT as default should be fine.
> 

Right. I was more talking in terms of DB410c here, where the lack of
this feature is kind of a regression.

Personally, I'm fine if you put the memory node here like this. If there
are concerns from others you could also move it to the -u-boot.dtsi and
omit it for Linux.

Thanks,
Stephan

Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Sumit Garg]

On Thu, 14 Mar 2024 at 18:54, Stephan Gerhold <stephan@gerhold.net> wrote:
>
> On Thu, Mar 14, 2024 at 05:26:27PM +0530, Sumit Garg wrote:
> > On Thu, 14 Mar 2024 at 16:13, Stephan Gerhold <stephan@gerhold.net> wrote:
> > > On Thu, Mar 14, 2024 at 03:02:31PM +0530, Sumit Garg wrote:
> > > > On Thu, 14 Mar 2024 at 14:48, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> > > > > On 3/14/24 10:04, Sumit Garg wrote:
> > > > > > On Wed, 13 Mar 2024 at 18:34, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> > > > > >> On 3/13/24 13:30, Sumit Garg wrote:
> > > > > >>> Add Schneider Electric HMIBSC board DTS. The HMIBSC board is an IIoT Edge
> > > > > >>> Box Core board based on the Qualcomm APQ8016E SoC.
> > > > > >>>
> > > > > >>> Support for Schneider Electric HMIBSC. Features:
> > > > > >>> - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306)
> > > > > >>> - 1GiB RAM
> > > > > >>> - 8GiB eMMC, SD slot
> > > > > >>> - WiFi and Bluetooth
> > > > > >>> - 2x Host, 1x Device USB port
> > > > > >>> - HDMI
> > > > > >>> - Discrete TPM2 chip over SPI
> > > > > >>> - USB ethernet adaptors (soldered)
> > > > > >>>
> > > > > >>> Co-developed-by: Jagdish Gediya <jagdish.gediya@linaro.org>
> > > > > >>> Signed-off-by: Jagdish Gediya <jagdish.gediya@linaro.org>
> > > > > >>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > > > > >>> ---
> > > > > >>
> > > > > >> [...]
> > > > > >>
> > > > > >>> +     memory@80000000 {
> > > > > >>> +             reg = <0 0x80000000 0 0x40000000>;
> > > > > >>> +     };
> > > > > >>
> > > > > >> I'm not sure the entirety of DRAM is accessible..
> > > > > >>
> > > > > >> This override should be unnecessary, as bootloaders generally update
> > > > > >> the size field anyway.
> > > > > >
> > > > > > On this board, U-Boot is used as the first stage bootloader (replacing
> > > > > > Little Kernel (LK), thanks to Stephan's work). And U-Boot consumes
> > > > > > memory range from DT as Linux does but doesn't require any memory to
> > > > > > be reserved for U-Boot itself. So apart from reserved memory nodes
> > > > > > explicitly described in DT all the other DRAM regions are accessible.
> > > > >
> > > > > Still, u-boot has code to fetch the size dynamically, no?
> > > > >
> > > >
> > > > No U-Boot being the first stage bootloader fetches size from DT which
> > > > is bundled into U-Boot binary.
> > > >
> > >
> > > Back when I added support for using U-Boot as first stage bootloader on
> > > DB410c the way it worked is that U-Boot used a fixed amount of DRAM
> > > (originally 968 MiB, later 1 GiB since I fixed this in commit
> > > 1d667227ea51 ("board: dragonboard410c: Fix PHYS_SDRAM_1_SIZE") [1]).
> > > When booting Linux, the Linux DT was dynamically patched with the right
> > > amount of DRAM (obtained from SMEM). So if you had e.g. a Geniatech DB4
> > > board with 2 GiB DRAM, U-Boot was only using 1 GiB of DRAM, but Linux
> > > later got the full 2 GiB patched into its DTB.
> > >
> > > I didn't have much time for testing U-Boot myself lately but a quick
> > > look at the recent changes suggest that Caleb accidentally removed that
> > > functionality in the recent cleanup. Specifically, the SMEM-based DRAM
> > > size detection was removed in commit 14868845db54 ("board:
> > > dragonboard410c: import board code from mach-snapdragon" [2]), the
> > > msm_fixup_memory() function does not seem to exist anymore now. :')
> >
> > Ah now I see the reasoning for that particular piece of code. Is SMEM
> > based approach the standardized way used by early stage boot-loaders
> > on other Qcom SoCs too?
> >
>
> It is definitely used on all the SoCs that were deployed with LK. I am
> not entirely sure about the newer ABL/UEFI-based ones. A quick look at
> the ABL source code suggests it is abstracted through an EFI protocol
> there (so we cannot see where the information comes from with just the
> open-source code). However, in my experience SMEM data structures are
> usually kept quite stable (or properly versioned), so it is quite likely
> that we could use this approach for all Qualcomm SoCs.
>

If the SoCs which support this standardized way to dynamic discover
DRAM size via SMEM then why do we need to rely on DT at all for those
SoCs? Can't U-Boot and Linux have the same driver to fetch DRAM size
via SMEM? I am not sure if it's an appropriate way for U-Boot to fixup
DT when that information can be discovered dynamically.

> > >
> > > Also, the DRAM size is now always taken from the DT (which is probably
> > > better than the previous hardcoded size in the U-Boot board code).
> > >
> > > I think we should bring the dynamic DRAM size detection back, because
> > > there are quite some boards available with varying DRAM size. Restoring
> > > msm_fixup_memory() would likely be easiest, I guess the ideal solution
> > > would be to parse SMEM in U-Boot's dram_init() function so even U-Boot
> > > has the correct amount of DRAM to work with.
> >
> > In the context of the HMIBSC board, it has 1 GB RAM LPDDR3 internal
> > not expandable. IMO, having it in DT as default should be fine.
> >
>
> Right. I was more talking in terms of DB410c here, where the lack of
> this feature is kind of a regression.
>
> Personally, I'm fine if you put the memory node here like this. If there
> are concerns from others you could also move it to the -u-boot.dtsi and
> omit it for Linux.

In U-Boot we encourage people to drop -u-boot.dtsi entirely rather
than the opposite such that we have a canonical devicetree usage
especially with OF_UPSTREAM enabled.

-Sumit

>
> Thanks,
> Stephan

Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Konrad Dybcio]

On 3/14/24 14:50, Sumit Garg wrote:
> On Thu, 14 Mar 2024 at 18:54, Stephan Gerhold <stephan@gerhold.net> wrote:
>>
>> On Thu, Mar 14, 2024 at 05:26:27PM +0530, Sumit Garg wrote:
>>> On Thu, 14 Mar 2024 at 16:13, Stephan Gerhold <stephan@gerhold.net> wrote:
>>>> On Thu, Mar 14, 2024 at 03:02:31PM +0530, Sumit Garg wrote:
>>>>> On Thu, 14 Mar 2024 at 14:48, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>>>>> On 3/14/24 10:04, Sumit Garg wrote:
>>>>>>> On Wed, 13 Mar 2024 at 18:34, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>>>>>>> On 3/13/24 13:30, Sumit Garg wrote:
>>>>>>>>> Add Schneider Electric HMIBSC board DTS. The HMIBSC board is an IIoT Edge
>>>>>>>>> Box Core board based on the Qualcomm APQ8016E SoC.
>>>>>>>>>
>>>>>>>>> Support for Schneider Electric HMIBSC. Features:
>>>>>>>>> - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306)
>>>>>>>>> - 1GiB RAM
>>>>>>>>> - 8GiB eMMC, SD slot
>>>>>>>>> - WiFi and Bluetooth
>>>>>>>>> - 2x Host, 1x Device USB port
>>>>>>>>> - HDMI
>>>>>>>>> - Discrete TPM2 chip over SPI
>>>>>>>>> - USB ethernet adaptors (soldered)
>>>>>>>>>
>>>>>>>>> Co-developed-by: Jagdish Gediya <jagdish.gediya@linaro.org>
>>>>>>>>> Signed-off-by: Jagdish Gediya <jagdish.gediya@linaro.org>
>>>>>>>>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
>>>>>>>>> ---
>>>>>>>>
>>>>>>>> [...]
>>>>>>>>
>>>>>>>>> +     memory@80000000 {
>>>>>>>>> +             reg = <0 0x80000000 0 0x40000000>;
>>>>>>>>> +     };
>>>>>>>>
>>>>>>>> I'm not sure the entirety of DRAM is accessible..
>>>>>>>>
>>>>>>>> This override should be unnecessary, as bootloaders generally update
>>>>>>>> the size field anyway.
>>>>>>>
>>>>>>> On this board, U-Boot is used as the first stage bootloader (replacing
>>>>>>> Little Kernel (LK), thanks to Stephan's work). And U-Boot consumes
>>>>>>> memory range from DT as Linux does but doesn't require any memory to
>>>>>>> be reserved for U-Boot itself. So apart from reserved memory nodes
>>>>>>> explicitly described in DT all the other DRAM regions are accessible.
>>>>>>
>>>>>> Still, u-boot has code to fetch the size dynamically, no?
>>>>>>
>>>>>
>>>>> No U-Boot being the first stage bootloader fetches size from DT which
>>>>> is bundled into U-Boot binary.
>>>>>
>>>>
>>>> Back when I added support for using U-Boot as first stage bootloader on
>>>> DB410c the way it worked is that U-Boot used a fixed amount of DRAM
>>>> (originally 968 MiB, later 1 GiB since I fixed this in commit
>>>> 1d667227ea51 ("board: dragonboard410c: Fix PHYS_SDRAM_1_SIZE") [1]).
>>>> When booting Linux, the Linux DT was dynamically patched with the right
>>>> amount of DRAM (obtained from SMEM). So if you had e.g. a Geniatech DB4
>>>> board with 2 GiB DRAM, U-Boot was only using 1 GiB of DRAM, but Linux
>>>> later got the full 2 GiB patched into its DTB.
>>>>
>>>> I didn't have much time for testing U-Boot myself lately but a quick
>>>> look at the recent changes suggest that Caleb accidentally removed that
>>>> functionality in the recent cleanup. Specifically, the SMEM-based DRAM
>>>> size detection was removed in commit 14868845db54 ("board:
>>>> dragonboard410c: import board code from mach-snapdragon" [2]), the
>>>> msm_fixup_memory() function does not seem to exist anymore now. :')
>>>
>>> Ah now I see the reasoning for that particular piece of code. Is SMEM
>>> based approach the standardized way used by early stage boot-loaders
>>> on other Qcom SoCs too?
>>>
>>
>> It is definitely used on all the SoCs that were deployed with LK. I am
>> not entirely sure about the newer ABL/UEFI-based ones. A quick look at
>> the ABL source code suggests it is abstracted through an EFI protocol
>> there (so we cannot see where the information comes from with just the
>> open-source code). However, in my experience SMEM data structures are
>> usually kept quite stable (or properly versioned), so it is quite likely
>> that we could use this approach for all Qualcomm SoCs.
>>
> 
> If the SoCs which support this standardized way to dynamic discover
> DRAM size via SMEM then why do we need to rely on DT at all for those
> SoCs? Can't U-Boot and Linux have the same driver to fetch DRAM size
> via SMEM? I am not sure if it's an appropriate way for U-Boot to fixup
> DT when that information can be discovered dynamically.

You're mixing two things. Linux expects a devicetree where /memory/reg[size]
is valid. Such driver should indeed be (re)implemented in u-boot to provide
this information.

As for linux, I am working on making Linux aware of the DDR capabilities
on Snapdragons, for other reasons, but it's on the back burner, as it
still needs some broad thinking about integrating it with the interested
consumers.. Bottom line is, Linux should be fed a devicetree with DRAM size
filled.

Konrad

Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Caleb Connolly]

On 14/03/2024 15:20, Konrad Dybcio wrote:
> 
> 
> On 3/14/24 14:50, Sumit Garg wrote:
>> On Thu, 14 Mar 2024 at 18:54, Stephan Gerhold <stephan@gerhold.net>
>> wrote:
>>>
>>> On Thu, Mar 14, 2024 at 05:26:27PM +0530, Sumit Garg wrote:
>>>> On Thu, 14 Mar 2024 at 16:13, Stephan Gerhold <stephan@gerhold.net>
>>>> wrote:
>>>>> On Thu, Mar 14, 2024 at 03:02:31PM +0530, Sumit Garg wrote:
>>>>>> On Thu, 14 Mar 2024 at 14:48, Konrad Dybcio
>>>>>> <konrad.dybcio@linaro.org> wrote:
>>>>>>> On 3/14/24 10:04, Sumit Garg wrote:
>>>>>>>> On Wed, 13 Mar 2024 at 18:34, Konrad Dybcio
>>>>>>>> <konrad.dybcio@linaro.org> wrote:
>>>>>>>>> On 3/13/24 13:30, Sumit Garg wrote:
>>>>>>>>>> Add Schneider Electric HMIBSC board DTS. The HMIBSC board is
>>>>>>>>>> an IIoT Edge
>>>>>>>>>> Box Core board based on the Qualcomm APQ8016E SoC.
>>>>>>>>>>
>>>>>>>>>> Support for Schneider Electric HMIBSC. Features:
>>>>>>>>>> - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno
>>>>>>>>>> 306)
>>>>>>>>>> - 1GiB RAM
>>>>>>>>>> - 8GiB eMMC, SD slot
>>>>>>>>>> - WiFi and Bluetooth
>>>>>>>>>> - 2x Host, 1x Device USB port
>>>>>>>>>> - HDMI
>>>>>>>>>> - Discrete TPM2 chip over SPI
>>>>>>>>>> - USB ethernet adaptors (soldered)
>>>>>>>>>>
>>>>>>>>>> Co-developed-by: Jagdish Gediya <jagdish.gediya@linaro.org>
>>>>>>>>>> Signed-off-by: Jagdish Gediya <jagdish.gediya@linaro.org>
>>>>>>>>>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
>>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> [...]
>>>>>>>>>
>>>>>>>>>> +     memory@80000000 {
>>>>>>>>>> +             reg = <0 0x80000000 0 0x40000000>;
>>>>>>>>>> +     };
>>>>>>>>>
>>>>>>>>> I'm not sure the entirety of DRAM is accessible..
>>>>>>>>>
>>>>>>>>> This override should be unnecessary, as bootloaders generally
>>>>>>>>> update
>>>>>>>>> the size field anyway.
>>>>>>>>
>>>>>>>> On this board, U-Boot is used as the first stage bootloader
>>>>>>>> (replacing
>>>>>>>> Little Kernel (LK), thanks to Stephan's work). And U-Boot consumes
>>>>>>>> memory range from DT as Linux does but doesn't require any
>>>>>>>> memory to
>>>>>>>> be reserved for U-Boot itself. So apart from reserved memory nodes
>>>>>>>> explicitly described in DT all the other DRAM regions are
>>>>>>>> accessible.
>>>>>>>
>>>>>>> Still, u-boot has code to fetch the size dynamically, no?
>>>>>>>
>>>>>>
>>>>>> No U-Boot being the first stage bootloader fetches size from DT which
>>>>>> is bundled into U-Boot binary.
>>>>>>
>>>>>
>>>>> Back when I added support for using U-Boot as first stage
>>>>> bootloader on
>>>>> DB410c the way it worked is that U-Boot used a fixed amount of DRAM
>>>>> (originally 968 MiB, later 1 GiB since I fixed this in commit
>>>>> 1d667227ea51 ("board: dragonboard410c: Fix PHYS_SDRAM_1_SIZE") [1]).
>>>>> When booting Linux, the Linux DT was dynamically patched with the
>>>>> right
>>>>> amount of DRAM (obtained from SMEM). So if you had e.g. a Geniatech
>>>>> DB4
>>>>> board with 2 GiB DRAM, U-Boot was only using 1 GiB of DRAM, but Linux
>>>>> later got the full 2 GiB patched into its DTB.
>>>>>
>>>>> I didn't have much time for testing U-Boot myself lately but a quick
>>>>> look at the recent changes suggest that Caleb accidentally removed
>>>>> that
>>>>> functionality in the recent cleanup. Specifically, the SMEM-based DRAM
>>>>> size detection was removed in commit 14868845db54 ("board:
>>>>> dragonboard410c: import board code from mach-snapdragon" [2]), the
>>>>> msm_fixup_memory() function does not seem to exist anymore now. :')
>>>>
>>>> Ah now I see the reasoning for that particular piece of code. Is SMEM
>>>> based approach the standardized way used by early stage boot-loaders
>>>> on other Qcom SoCs too?
>>>>
>>>
>>> It is definitely used on all the SoCs that were deployed with LK. I am
>>> not entirely sure about the newer ABL/UEFI-based ones. A quick look at
>>> the ABL source code suggests it is abstracted through an EFI protocol
>>> there (so we cannot see where the information comes from with just the
>>> open-source code). However, in my experience SMEM data structures are
>>> usually kept quite stable (or properly versioned), so it is quite likely
>>> that we could use this approach for all Qualcomm SoCs.
>>>
>>
>> If the SoCs which support this standardized way to dynamic discover
>> DRAM size via SMEM then why do we need to rely on DT at all for those
>> SoCs? Can't U-Boot and Linux have the same driver to fetch DRAM size
>> via SMEM? I am not sure if it's an appropriate way for U-Boot to fixup
>> DT when that information can be discovered dynamically.

"standardized" I'm not so sure... But yes, smem does offer this. The
definition in DT here is for U-Boot, ABL will always clobber it, and so
does U-Boot before handing over to the kernel. Linux should never see
this without a bootloader having looked at it.

The reason I decided to hardcode this in DT for U-Boot is because SMEM
currently relies on the driver model and isn't available early enough.

Also admittedly I just wasn't that familiar with the U-Boot codebase. I
just wanted to avoid hardcoding this in C code, and given that this was
already supported for the "chainloading from ABL" usecase, just
hardcoding the values was the obvious solution.

I would definitely be open to revisiting this in U-Boot, having an SMEM
framework that we could use without the driver model which would just
take a base address and then let us interact with SMEM and populate the
dram bank data would be a good improvement, and would let us avoid
hardcoding the memory layout in DT. We'd just need to manually find the
SMEM base address in the FDT as part of "dram_init_banksize()" and
retrieve the data there.

That all being said, I don't see any reason not to define the memory
layout in DT, it's a hardware feature, DT describes the hardware. The
whole "bootloader will fill this in" implies that the bootloader isn't
also using DT as the source of truth, but now with U-Boot it actually
is, so it's all the more important that DT be accurate ;P
> 
> You're mixing two things. Linux expects a devicetree where
> /memory/reg[size]
> is valid. Such driver should indeed be (re)implemented in u-boot to provide
> this information.
> 
> As for linux, I am working on making Linux aware of the DDR capabilities
> on Snapdragons, for other reasons, but it's on the back burner, as it
> still needs some broad thinking about integrating it with the interested
> consumers.. Bottom line is, Linux should be fed a devicetree with DRAM size
> filled.
> 
> Konrad

-- 
// Caleb (they/them)

Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Sumit Garg]

On Thu, 14 Mar 2024 at 21:07, Caleb Connolly <caleb.connolly@linaro.org> wrote:
>
>
>
> On 14/03/2024 15:20, Konrad Dybcio wrote:
> >
> >
> > On 3/14/24 14:50, Sumit Garg wrote:
> >> On Thu, 14 Mar 2024 at 18:54, Stephan Gerhold <stephan@gerhold.net>
> >> wrote:
> >>>
> >>> On Thu, Mar 14, 2024 at 05:26:27PM +0530, Sumit Garg wrote:
> >>>> On Thu, 14 Mar 2024 at 16:13, Stephan Gerhold <stephan@gerhold.net>
> >>>> wrote:
> >>>>> On Thu, Mar 14, 2024 at 03:02:31PM +0530, Sumit Garg wrote:
> >>>>>> On Thu, 14 Mar 2024 at 14:48, Konrad Dybcio
> >>>>>> <konrad.dybcio@linaro.org> wrote:
> >>>>>>> On 3/14/24 10:04, Sumit Garg wrote:
> >>>>>>>> On Wed, 13 Mar 2024 at 18:34, Konrad Dybcio
> >>>>>>>> <konrad.dybcio@linaro.org> wrote:
> >>>>>>>>> On 3/13/24 13:30, Sumit Garg wrote:
> >>>>>>>>>> Add Schneider Electric HMIBSC board DTS. The HMIBSC board is
> >>>>>>>>>> an IIoT Edge
> >>>>>>>>>> Box Core board based on the Qualcomm APQ8016E SoC.
> >>>>>>>>>>
> >>>>>>>>>> Support for Schneider Electric HMIBSC. Features:
> >>>>>>>>>> - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno
> >>>>>>>>>> 306)
> >>>>>>>>>> - 1GiB RAM
> >>>>>>>>>> - 8GiB eMMC, SD slot
> >>>>>>>>>> - WiFi and Bluetooth
> >>>>>>>>>> - 2x Host, 1x Device USB port
> >>>>>>>>>> - HDMI
> >>>>>>>>>> - Discrete TPM2 chip over SPI
> >>>>>>>>>> - USB ethernet adaptors (soldered)
> >>>>>>>>>>
> >>>>>>>>>> Co-developed-by: Jagdish Gediya <jagdish.gediya@linaro.org>
> >>>>>>>>>> Signed-off-by: Jagdish Gediya <jagdish.gediya@linaro.org>
> >>>>>>>>>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> >>>>>>>>>> ---
> >>>>>>>>>
> >>>>>>>>> [...]
> >>>>>>>>>
> >>>>>>>>>> +     memory@80000000 {
> >>>>>>>>>> +             reg = <0 0x80000000 0 0x40000000>;
> >>>>>>>>>> +     };
> >>>>>>>>>
> >>>>>>>>> I'm not sure the entirety of DRAM is accessible..
> >>>>>>>>>
> >>>>>>>>> This override should be unnecessary, as bootloaders generally
> >>>>>>>>> update
> >>>>>>>>> the size field anyway.
> >>>>>>>>
> >>>>>>>> On this board, U-Boot is used as the first stage bootloader
> >>>>>>>> (replacing
> >>>>>>>> Little Kernel (LK), thanks to Stephan's work). And U-Boot consumes
> >>>>>>>> memory range from DT as Linux does but doesn't require any
> >>>>>>>> memory to
> >>>>>>>> be reserved for U-Boot itself. So apart from reserved memory nodes
> >>>>>>>> explicitly described in DT all the other DRAM regions are
> >>>>>>>> accessible.
> >>>>>>>
> >>>>>>> Still, u-boot has code to fetch the size dynamically, no?
> >>>>>>>
> >>>>>>
> >>>>>> No U-Boot being the first stage bootloader fetches size from DT which
> >>>>>> is bundled into U-Boot binary.
> >>>>>>
> >>>>>
> >>>>> Back when I added support for using U-Boot as first stage
> >>>>> bootloader on
> >>>>> DB410c the way it worked is that U-Boot used a fixed amount of DRAM
> >>>>> (originally 968 MiB, later 1 GiB since I fixed this in commit
> >>>>> 1d667227ea51 ("board: dragonboard410c: Fix PHYS_SDRAM_1_SIZE") [1]).
> >>>>> When booting Linux, the Linux DT was dynamically patched with the
> >>>>> right
> >>>>> amount of DRAM (obtained from SMEM). So if you had e.g. a Geniatech
> >>>>> DB4
> >>>>> board with 2 GiB DRAM, U-Boot was only using 1 GiB of DRAM, but Linux
> >>>>> later got the full 2 GiB patched into its DTB.
> >>>>>
> >>>>> I didn't have much time for testing U-Boot myself lately but a quick
> >>>>> look at the recent changes suggest that Caleb accidentally removed
> >>>>> that
> >>>>> functionality in the recent cleanup. Specifically, the SMEM-based DRAM
> >>>>> size detection was removed in commit 14868845db54 ("board:
> >>>>> dragonboard410c: import board code from mach-snapdragon" [2]), the
> >>>>> msm_fixup_memory() function does not seem to exist anymore now. :')
> >>>>
> >>>> Ah now I see the reasoning for that particular piece of code. Is SMEM
> >>>> based approach the standardized way used by early stage boot-loaders
> >>>> on other Qcom SoCs too?
> >>>>
> >>>
> >>> It is definitely used on all the SoCs that were deployed with LK. I am
> >>> not entirely sure about the newer ABL/UEFI-based ones. A quick look at
> >>> the ABL source code suggests it is abstracted through an EFI protocol
> >>> there (so we cannot see where the information comes from with just the
> >>> open-source code). However, in my experience SMEM data structures are
> >>> usually kept quite stable (or properly versioned), so it is quite likely
> >>> that we could use this approach for all Qualcomm SoCs.
> >>>
> >>
> >> If the SoCs which support this standardized way to dynamic discover
> >> DRAM size via SMEM then why do we need to rely on DT at all for those
> >> SoCs? Can't U-Boot and Linux have the same driver to fetch DRAM size
> >> via SMEM? I am not sure if it's an appropriate way for U-Boot to fixup
> >> DT when that information can be discovered dynamically.
>
> "standardized" I'm not so sure... But yes, smem does offer this. The
> definition in DT here is for U-Boot,

We should move away from that thinking that U-Boot has its own DT and
Linux kernel has its own. IMO, that's just the opposite of the true DT
definition.

> ABL will always clobber it, and so
> does U-Boot before handing over to the kernel. Linux should never see
> this without a bootloader having looked at it.

Where does U-Boot clobber SMEM? I would be interested to see if ABL
clobbers it too?

>
> The reason I decided to hardcode this in DT for U-Boot is because SMEM
> currently relies on the driver model and isn't available early enough.
>
> Also admittedly I just wasn't that familiar with the U-Boot codebase. I
> just wanted to avoid hardcoding this in C code, and given that this was
> already supported for the "chainloading from ABL" usecase, just
> hardcoding the values was the obvious solution.
>
> I would definitely be open to revisiting this in U-Boot, having an SMEM
> framework that we could use without the driver model which would just
> take a base address and then let us interact with SMEM and populate the
> dram bank data would be a good improvement, and would let us avoid
> hardcoding the memory layout in DT. We'd just need to manually find the
> SMEM base address in the FDT as part of "dram_init_banksize()" and
> retrieve the data there.

These are the similar problems Linux has to deal with too but on Qcom
platforms it is rather offloaded to bootloaders to rather implement
this. It leads to custom DT modification or board code in U-Boot which
is hard to maintain. If we want to implement it properly then
corresponding bindings should be upstreamed too regarding how DRAM
range is to be discovered via SMEM.

>
> That all being said, I don't see any reason not to define the memory
> layout in DT, it's a hardware feature, DT describes the hardware. The
> whole "bootloader will fill this in" implies that the bootloader isn't
> also using DT as the source of truth, but now with U-Boot it actually
> is, so it's all the more important that DT be accurate ;P

I agree DT should be accurate and not a fan of DT fixups. However,
when it comes to some hardware configuration being discoverable then
IMHO DT isn't the appropriate place for that. For the time being I am
fine with the DRAM range to be specified in DT.

> >
> > You're mixing two things. Linux expects a devicetree where
> > /memory/reg[size]
> > is valid. Such driver should indeed be (re)implemented in u-boot to provide
> > this information.

No, I don't think so. We should rather start thinking about the
overall stack rather than just being Linux kernel centric. Do you have
a generic solution for U-Boot regarding how this should be
implemented?

-Sumit

> >
> > As for linux, I am working on making Linux aware of the DDR capabilities
> > on Snapdragons, for other reasons, but it's on the back burner, as it
> > still needs some broad thinking about integrating it with the interested
> > consumers.. Bottom line is, Linux should be fed a devicetree with DRAM size
> > filled.
> >
> > Konrad
>
> --
> // Caleb (they/them)

Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Caleb Connolly]

On 15/03/2024 09:31, Sumit Garg wrote:
> On Thu, 14 Mar 2024 at 21:07, Caleb Connolly <caleb.connolly@linaro.org> wrote:
>>
>>
>>
>> On 14/03/2024 15:20, Konrad Dybcio wrote:
>>>
>>>
>>> On 3/14/24 14:50, Sumit Garg wrote:
>>>> On Thu, 14 Mar 2024 at 18:54, Stephan Gerhold <stephan@gerhold.net>
>>>> wrote:
>>>>>
>>>>> On Thu, Mar 14, 2024 at 05:26:27PM +0530, Sumit Garg wrote:
>>>>>> On Thu, 14 Mar 2024 at 16:13, Stephan Gerhold <stephan@gerhold.net>
>>>>>> wrote:
>>>>>>> On Thu, Mar 14, 2024 at 03:02:31PM +0530, Sumit Garg wrote:
>>>>>>>> On Thu, 14 Mar 2024 at 14:48, Konrad Dybcio
>>>>>>>> <konrad.dybcio@linaro.org> wrote:
>>>>>>>>> On 3/14/24 10:04, Sumit Garg wrote:
>>>>>>>>>> On Wed, 13 Mar 2024 at 18:34, Konrad Dybcio
>>>>>>>>>> <konrad.dybcio@linaro.org> wrote:
>>>>>>>>>>> On 3/13/24 13:30, Sumit Garg wrote:
>>>>>>>>>>>> Add Schneider Electric HMIBSC board DTS. The HMIBSC board is
>>>>>>>>>>>> an IIoT Edge
>>>>>>>>>>>> Box Core board based on the Qualcomm APQ8016E SoC.
>>>>>>>>>>>>
>>>>>>>>>>>> Support for Schneider Electric HMIBSC. Features:
>>>>>>>>>>>> - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno
>>>>>>>>>>>> 306)
>>>>>>>>>>>> - 1GiB RAM
>>>>>>>>>>>> - 8GiB eMMC, SD slot
>>>>>>>>>>>> - WiFi and Bluetooth
>>>>>>>>>>>> - 2x Host, 1x Device USB port
>>>>>>>>>>>> - HDMI
>>>>>>>>>>>> - Discrete TPM2 chip over SPI
>>>>>>>>>>>> - USB ethernet adaptors (soldered)
>>>>>>>>>>>>
>>>>>>>>>>>> Co-developed-by: Jagdish Gediya <jagdish.gediya@linaro.org>
>>>>>>>>>>>> Signed-off-by: Jagdish Gediya <jagdish.gediya@linaro.org>
>>>>>>>>>>>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
>>>>>>>>>>>> ---
>>>>>>>>>>>
>>>>>>>>>>> [...]
>>>>>>>>>>>
>>>>>>>>>>>> +     memory@80000000 {
>>>>>>>>>>>> +             reg = <0 0x80000000 0 0x40000000>;
>>>>>>>>>>>> +     };
>>>>>>>>>>>
>>>>>>>>>>> I'm not sure the entirety of DRAM is accessible..
>>>>>>>>>>>
>>>>>>>>>>> This override should be unnecessary, as bootloaders generally
>>>>>>>>>>> update
>>>>>>>>>>> the size field anyway.
>>>>>>>>>>
>>>>>>>>>> On this board, U-Boot is used as the first stage bootloader
>>>>>>>>>> (replacing
>>>>>>>>>> Little Kernel (LK), thanks to Stephan's work). And U-Boot consumes
>>>>>>>>>> memory range from DT as Linux does but doesn't require any
>>>>>>>>>> memory to
>>>>>>>>>> be reserved for U-Boot itself. So apart from reserved memory nodes
>>>>>>>>>> explicitly described in DT all the other DRAM regions are
>>>>>>>>>> accessible.
>>>>>>>>>
>>>>>>>>> Still, u-boot has code to fetch the size dynamically, no?
>>>>>>>>>
>>>>>>>>
>>>>>>>> No U-Boot being the first stage bootloader fetches size from DT which
>>>>>>>> is bundled into U-Boot binary.
>>>>>>>>
>>>>>>>
>>>>>>> Back when I added support for using U-Boot as first stage
>>>>>>> bootloader on
>>>>>>> DB410c the way it worked is that U-Boot used a fixed amount of DRAM
>>>>>>> (originally 968 MiB, later 1 GiB since I fixed this in commit
>>>>>>> 1d667227ea51 ("board: dragonboard410c: Fix PHYS_SDRAM_1_SIZE") [1]).
>>>>>>> When booting Linux, the Linux DT was dynamically patched with the
>>>>>>> right
>>>>>>> amount of DRAM (obtained from SMEM). So if you had e.g. a Geniatech
>>>>>>> DB4
>>>>>>> board with 2 GiB DRAM, U-Boot was only using 1 GiB of DRAM, but Linux
>>>>>>> later got the full 2 GiB patched into its DTB.
>>>>>>>
>>>>>>> I didn't have much time for testing U-Boot myself lately but a quick
>>>>>>> look at the recent changes suggest that Caleb accidentally removed
>>>>>>> that
>>>>>>> functionality in the recent cleanup. Specifically, the SMEM-based DRAM
>>>>>>> size detection was removed in commit 14868845db54 ("board:
>>>>>>> dragonboard410c: import board code from mach-snapdragon" [2]), the
>>>>>>> msm_fixup_memory() function does not seem to exist anymore now. :')
>>>>>>
>>>>>> Ah now I see the reasoning for that particular piece of code. Is SMEM
>>>>>> based approach the standardized way used by early stage boot-loaders
>>>>>> on other Qcom SoCs too?
>>>>>>
>>>>>
>>>>> It is definitely used on all the SoCs that were deployed with LK. I am
>>>>> not entirely sure about the newer ABL/UEFI-based ones. A quick look at
>>>>> the ABL source code suggests it is abstracted through an EFI protocol
>>>>> there (so we cannot see where the information comes from with just the
>>>>> open-source code). However, in my experience SMEM data structures are
>>>>> usually kept quite stable (or properly versioned), so it is quite likely
>>>>> that we could use this approach for all Qualcomm SoCs.
>>>>>
>>>>
>>>> If the SoCs which support this standardized way to dynamic discover
>>>> DRAM size via SMEM then why do we need to rely on DT at all for those
>>>> SoCs? Can't U-Boot and Linux have the same driver to fetch DRAM size
>>>> via SMEM? I am not sure if it's an appropriate way for U-Boot to fixup
>>>> DT when that information can be discovered dynamically.
>>
>> "standardized" I'm not so sure... But yes, smem does offer this. The
>> definition in DT here is for U-Boot,
> 
> We should move away from that thinking that U-Boot has its own DT and
> Linux kernel has its own. IMO, that's just the opposite of the true DT
> definition.

I was clarifying here that the memory node being defined with real
values was because U-Boot uses them. Just in case some folks thought
that Linux was using whatever values were here.
> 
>> ABL will always clobber it, and so
>> does U-Boot before handing over to the kernel. Linux should never see
>> this without a bootloader having looked at it.
> 
> Where does U-Boot clobber SMEM? I would be interested to see if ABL
> clobbers it too?

Not SMEM, the /memory node.
> 
>>
>> The reason I decided to hardcode this in DT for U-Boot is because SMEM
>> currently relies on the driver model and isn't available early enough.
>>
>> Also admittedly I just wasn't that familiar with the U-Boot codebase. I
>> just wanted to avoid hardcoding this in C code, and given that this was
>> already supported for the "chainloading from ABL" usecase, just
>> hardcoding the values was the obvious solution.
>>
>> I would definitely be open to revisiting this in U-Boot, having an SMEM
>> framework that we could use without the driver model which would just
>> take a base address and then let us interact with SMEM and populate the
>> dram bank data would be a good improvement, and would let us avoid
>> hardcoding the memory layout in DT. We'd just need to manually find the
>> SMEM base address in the FDT as part of "dram_init_banksize()" and
>> retrieve the data there.
> 
> These are the similar problems Linux has to deal with too but on Qcom
> platforms it is rather offloaded to bootloaders to rather implement
> this. It leads to custom DT modification or board code in U-Boot which
> is hard to maintain. If we want to implement it properly then
> corresponding bindings should be upstreamed too regarding how DRAM
> range is to be discovered via SMEM.
> 
>>
>> That all being said, I don't see any reason not to define the memory
>> layout in DT, it's a hardware feature, DT describes the hardware. The
>> whole "bootloader will fill this in" implies that the bootloader isn't
>> also using DT as the source of truth, but now with U-Boot it actually
>> is, so it's all the more important that DT be accurate ;P
> 
> I agree DT should be accurate and not a fan of DT fixups. However,
> when it comes to some hardware configuration being discoverable then
> IMHO DT isn't the appropriate place for that. For the time being I am
> fine with the DRAM range to be specified in DT.

Agreed
> 
>>>
>>> You're mixing two things. Linux expects a devicetree where
>>> /memory/reg[size]
>>> is valid. Such driver should indeed be (re)implemented in u-boot to provide
>>> this information.
> 
> No, I don't think so. We should rather start thinking about the
> overall stack rather than just being Linux kernel centric. Do you have
> a generic solution for U-Boot regarding how this should be
> implemented?
> 
> -Sumit
> 
>>>
>>> As for linux, I am working on making Linux aware of the DDR capabilities
>>> on Snapdragons, for other reasons, but it's on the back burner, as it
>>> still needs some broad thinking about integrating it with the interested
>>> consumers.. Bottom line is, Linux should be fed a devicetree with DRAM size
>>> filled.
>>>
>>> Konrad
>>
>> --
>> // Caleb (they/them)

-- 
// Caleb (they/them)

Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Stephan Gerhold]

On Fri, Mar 15, 2024 at 03:01:27PM +0530, Sumit Garg wrote:
> On Thu, 14 Mar 2024 at 21:07, Caleb Connolly <caleb.connolly@linaro.org> wrote:
> > On 14/03/2024 15:20, Konrad Dybcio wrote:
> > > On 3/14/24 14:50, Sumit Garg wrote:
> > >> On Thu, 14 Mar 2024 at 18:54, Stephan Gerhold <stephan@gerhold.net>
> > >> wrote:
> > >>>
> > >>> On Thu, Mar 14, 2024 at 05:26:27PM +0530, Sumit Garg wrote:
> > >>>> On Thu, 14 Mar 2024 at 16:13, Stephan Gerhold <stephan@gerhold.net>
> > >>>> wrote:
> > >>>>> On Thu, Mar 14, 2024 at 03:02:31PM +0530, Sumit Garg wrote:
> > >>>>>> On Thu, 14 Mar 2024 at 14:48, Konrad Dybcio
> > >>>>>> <konrad.dybcio@linaro.org> wrote:
> > >>>>>>> On 3/14/24 10:04, Sumit Garg wrote:
> > >>>>>>>> On Wed, 13 Mar 2024 at 18:34, Konrad Dybcio
> > >>>>>>>> <konrad.dybcio@linaro.org> wrote:
> > >>>>>>>>> On 3/13/24 13:30, Sumit Garg wrote:
> > >>>>>>>>>> Add Schneider Electric HMIBSC board DTS. The HMIBSC board is
> > >>>>>>>>>> an IIoT Edge
> > >>>>>>>>>> Box Core board based on the Qualcomm APQ8016E SoC.
> > >>>>>>>>>>
> > >>>>>>>>>> Support for Schneider Electric HMIBSC. Features:
> > >>>>>>>>>> - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno
> > >>>>>>>>>> 306)
> > >>>>>>>>>> - 1GiB RAM
> > >>>>>>>>>> - 8GiB eMMC, SD slot
> > >>>>>>>>>> - WiFi and Bluetooth
> > >>>>>>>>>> - 2x Host, 1x Device USB port
> > >>>>>>>>>> - HDMI
> > >>>>>>>>>> - Discrete TPM2 chip over SPI
> > >>>>>>>>>> - USB ethernet adaptors (soldered)
> > >>>>>>>>>>
> > >>>>>>>>>> Co-developed-by: Jagdish Gediya <jagdish.gediya@linaro.org>
> > >>>>>>>>>> Signed-off-by: Jagdish Gediya <jagdish.gediya@linaro.org>
> > >>>>>>>>>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > >>>>>>>>>> ---
> > >>>>>>>>>
> > >>>>>>>>> [...]
> > >>>>>>>>>
> > >>>>>>>>>> +     memory@80000000 {
> > >>>>>>>>>> +             reg = <0 0x80000000 0 0x40000000>;
> > >>>>>>>>>> +     };
> > >>>>>>>>>
> > >>>>>>>>> I'm not sure the entirety of DRAM is accessible..
> > >>>>>>>>>
> > >>>>>>>>> This override should be unnecessary, as bootloaders generally
> > >>>>>>>>> update
> > >>>>>>>>> the size field anyway.
> > >>>>>>>>
> > >>>>>>>> On this board, U-Boot is used as the first stage bootloader
> > >>>>>>>> (replacing
> > >>>>>>>> Little Kernel (LK), thanks to Stephan's work). And U-Boot consumes
> > >>>>>>>> memory range from DT as Linux does but doesn't require any
> > >>>>>>>> memory to
> > >>>>>>>> be reserved for U-Boot itself. So apart from reserved memory nodes
> > >>>>>>>> explicitly described in DT all the other DRAM regions are
> > >>>>>>>> accessible.
> > >>>>>>>
> > >>>>>>> Still, u-boot has code to fetch the size dynamically, no?
> > >>>>>>>
> > >>>>>>
> > >>>>>> No U-Boot being the first stage bootloader fetches size from DT which
> > >>>>>> is bundled into U-Boot binary.
> > >>>>>>
> > >>>>>
> > >>>>> Back when I added support for using U-Boot as first stage
> > >>>>> bootloader on
> > >>>>> DB410c the way it worked is that U-Boot used a fixed amount of DRAM
> > >>>>> (originally 968 MiB, later 1 GiB since I fixed this in commit
> > >>>>> 1d667227ea51 ("board: dragonboard410c: Fix PHYS_SDRAM_1_SIZE") [1]).
> > >>>>> When booting Linux, the Linux DT was dynamically patched with the
> > >>>>> right
> > >>>>> amount of DRAM (obtained from SMEM). So if you had e.g. a Geniatech
> > >>>>> DB4
> > >>>>> board with 2 GiB DRAM, U-Boot was only using 1 GiB of DRAM, but Linux
> > >>>>> later got the full 2 GiB patched into its DTB.
> > >>>>>
> > >>>>> I didn't have much time for testing U-Boot myself lately but a quick
> > >>>>> look at the recent changes suggest that Caleb accidentally removed
> > >>>>> that
> > >>>>> functionality in the recent cleanup. Specifically, the SMEM-based DRAM
> > >>>>> size detection was removed in commit 14868845db54 ("board:
> > >>>>> dragonboard410c: import board code from mach-snapdragon" [2]), the
> > >>>>> msm_fixup_memory() function does not seem to exist anymore now. :')
> > >>>>
> > >>>> Ah now I see the reasoning for that particular piece of code. Is SMEM
> > >>>> based approach the standardized way used by early stage boot-loaders
> > >>>> on other Qcom SoCs too?
> > >>>>
> > >>>
> > >>> It is definitely used on all the SoCs that were deployed with LK. I am
> > >>> not entirely sure about the newer ABL/UEFI-based ones. A quick look at
> > >>> the ABL source code suggests it is abstracted through an EFI protocol
> > >>> there (so we cannot see where the information comes from with just the
> > >>> open-source code). However, in my experience SMEM data structures are
> > >>> usually kept quite stable (or properly versioned), so it is quite likely
> > >>> that we could use this approach for all Qualcomm SoCs.
> > >>>
> > >>
> > >> If the SoCs which support this standardized way to dynamic discover
> > >> DRAM size via SMEM then why do we need to rely on DT at all for those
> > >> SoCs? Can't U-Boot and Linux have the same driver to fetch DRAM size
> > >> via SMEM? I am not sure if it's an appropriate way for U-Boot to fixup
> > >> DT when that information can be discovered dynamically.
> >
> > [...]
> >
> > The reason I decided to hardcode this in DT for U-Boot is because SMEM
> > currently relies on the driver model and isn't available early enough.
> >
> > Also admittedly I just wasn't that familiar with the U-Boot codebase. I
> > just wanted to avoid hardcoding this in C code, and given that this was
> > already supported for the "chainloading from ABL" usecase, just
> > hardcoding the values was the obvious solution.
> >
> > I would definitely be open to revisiting this in U-Boot, having an SMEM
> > framework that we could use without the driver model which would just
> > take a base address and then let us interact with SMEM and populate the
> > dram bank data would be a good improvement, and would let us avoid
> > hardcoding the memory layout in DT. We'd just need to manually find the
> > SMEM base address in the FDT as part of "dram_init_banksize()" and
> > retrieve the data there.
> 
> These are the similar problems Linux has to deal with too but on Qcom
> platforms it is rather offloaded to bootloaders to rather implement
> this. It leads to custom DT modification or board code in U-Boot which
> is hard to maintain. If we want to implement it properly then
> corresponding bindings should be upstreamed too regarding how DRAM
> range is to be discovered via SMEM.
> 
> >
> > That all being said, I don't see any reason not to define the memory
> > layout in DT, it's a hardware feature, DT describes the hardware. The
> > whole "bootloader will fill this in" implies that the bootloader isn't
> > also using DT as the source of truth, but now with U-Boot it actually
> > is, so it's all the more important that DT be accurate ;P
> 
> I agree DT should be accurate and not a fan of DT fixups. However,
> when it comes to some hardware configuration being discoverable then
> IMHO DT isn't the appropriate place for that. For the time being I am
> fine with the DRAM range to be specified in DT.
> 
> > >
> > > You're mixing two things. Linux expects a devicetree where
> > > /memory/reg[size]
> > > is valid. Such driver should indeed be (re)implemented in u-boot to provide
> > > this information.
> 
> No, I don't think so. We should rather start thinking about the
> overall stack rather than just being Linux kernel centric. Do you have
> a generic solution for U-Boot regarding how this should be
> implemented?
> 

Detecting available memory via /memory in the DT or other firmware
services (such as UEFI GetMemoryMap()) is *the* generic solution used
everywhere, independent of the hardware (e.g. Qualcomm) and the
operating system (e.g. Linux or Xen).

It allows us to implement the board/Qualcomm-specific memory detection
in a single project, rather than having extra porting overhead for each
operating system for something as essential as available memory.

If we implement the SMEM-based memory detection in U-Boot (probably in
dram_init_banksize() as Caleb suggested) everything else will just work
without any Qualcomm-specific DT patching in U-Boot, and without any
special code in the operating system:

 - U-Boot automatically updates the /memory node in generic code (see
   fdt_fixup_memory_banks() call in arch/arm/lib/bootm-fdt.c). If you
   are using UEFI for booting, U-Boot will provide the same information
   via GetMemoryMap(). (The DT spec says /memory should be ignored on
   UEFI systems.)

 - Linux, Xen, and any other operating system can obtain the memory size
   via the standard method, and do not need any Qualcomm-specific at all
   (at least for memory detection).

I don't think there is anything Linux kernel centric for the memory
detection here. Quite the opposite really. :)

Thanks,
Stephan

Re: [PATCH v2 3/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Sumit Garg]

On Fri, 15 Mar 2024 at 20:01, Stephan Gerhold <stephan@gerhold.net> wrote:
>
> On Fri, Mar 15, 2024 at 03:01:27PM +0530, Sumit Garg wrote:
> > On Thu, 14 Mar 2024 at 21:07, Caleb Connolly <caleb.connolly@linaro.org> wrote:
> > > On 14/03/2024 15:20, Konrad Dybcio wrote:
> > > > On 3/14/24 14:50, Sumit Garg wrote:
> > > >> On Thu, 14 Mar 2024 at 18:54, Stephan Gerhold <stephan@gerhold.net>
> > > >> wrote:
> > > >>>
> > > >>> On Thu, Mar 14, 2024 at 05:26:27PM +0530, Sumit Garg wrote:
> > > >>>> On Thu, 14 Mar 2024 at 16:13, Stephan Gerhold <stephan@gerhold.net>
> > > >>>> wrote:
> > > >>>>> On Thu, Mar 14, 2024 at 03:02:31PM +0530, Sumit Garg wrote:
> > > >>>>>> On Thu, 14 Mar 2024 at 14:48, Konrad Dybcio
> > > >>>>>> <konrad.dybcio@linaro.org> wrote:
> > > >>>>>>> On 3/14/24 10:04, Sumit Garg wrote:
> > > >>>>>>>> On Wed, 13 Mar 2024 at 18:34, Konrad Dybcio
> > > >>>>>>>> <konrad.dybcio@linaro.org> wrote:
> > > >>>>>>>>> On 3/13/24 13:30, Sumit Garg wrote:
> > > >>>>>>>>>> Add Schneider Electric HMIBSC board DTS. The HMIBSC board is
> > > >>>>>>>>>> an IIoT Edge
> > > >>>>>>>>>> Box Core board based on the Qualcomm APQ8016E SoC.
> > > >>>>>>>>>>
> > > >>>>>>>>>> Support for Schneider Electric HMIBSC. Features:
> > > >>>>>>>>>> - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno
> > > >>>>>>>>>> 306)
> > > >>>>>>>>>> - 1GiB RAM
> > > >>>>>>>>>> - 8GiB eMMC, SD slot
> > > >>>>>>>>>> - WiFi and Bluetooth
> > > >>>>>>>>>> - 2x Host, 1x Device USB port
> > > >>>>>>>>>> - HDMI
> > > >>>>>>>>>> - Discrete TPM2 chip over SPI
> > > >>>>>>>>>> - USB ethernet adaptors (soldered)
> > > >>>>>>>>>>
> > > >>>>>>>>>> Co-developed-by: Jagdish Gediya <jagdish.gediya@linaro.org>
> > > >>>>>>>>>> Signed-off-by: Jagdish Gediya <jagdish.gediya@linaro.org>
> > > >>>>>>>>>> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > > >>>>>>>>>> ---
> > > >>>>>>>>>
> > > >>>>>>>>> [...]
> > > >>>>>>>>>
> > > >>>>>>>>>> +     memory@80000000 {
> > > >>>>>>>>>> +             reg = <0 0x80000000 0 0x40000000>;
> > > >>>>>>>>>> +     };
> > > >>>>>>>>>
> > > >>>>>>>>> I'm not sure the entirety of DRAM is accessible..
> > > >>>>>>>>>
> > > >>>>>>>>> This override should be unnecessary, as bootloaders generally
> > > >>>>>>>>> update
> > > >>>>>>>>> the size field anyway.
> > > >>>>>>>>
> > > >>>>>>>> On this board, U-Boot is used as the first stage bootloader
> > > >>>>>>>> (replacing
> > > >>>>>>>> Little Kernel (LK), thanks to Stephan's work). And U-Boot consumes
> > > >>>>>>>> memory range from DT as Linux does but doesn't require any
> > > >>>>>>>> memory to
> > > >>>>>>>> be reserved for U-Boot itself. So apart from reserved memory nodes
> > > >>>>>>>> explicitly described in DT all the other DRAM regions are
> > > >>>>>>>> accessible.
> > > >>>>>>>
> > > >>>>>>> Still, u-boot has code to fetch the size dynamically, no?
> > > >>>>>>>
> > > >>>>>>
> > > >>>>>> No U-Boot being the first stage bootloader fetches size from DT which
> > > >>>>>> is bundled into U-Boot binary.
> > > >>>>>>
> > > >>>>>
> > > >>>>> Back when I added support for using U-Boot as first stage
> > > >>>>> bootloader on
> > > >>>>> DB410c the way it worked is that U-Boot used a fixed amount of DRAM
> > > >>>>> (originally 968 MiB, later 1 GiB since I fixed this in commit
> > > >>>>> 1d667227ea51 ("board: dragonboard410c: Fix PHYS_SDRAM_1_SIZE") [1]).
> > > >>>>> When booting Linux, the Linux DT was dynamically patched with the
> > > >>>>> right
> > > >>>>> amount of DRAM (obtained from SMEM). So if you had e.g. a Geniatech
> > > >>>>> DB4
> > > >>>>> board with 2 GiB DRAM, U-Boot was only using 1 GiB of DRAM, but Linux
> > > >>>>> later got the full 2 GiB patched into its DTB.
> > > >>>>>
> > > >>>>> I didn't have much time for testing U-Boot myself lately but a quick
> > > >>>>> look at the recent changes suggest that Caleb accidentally removed
> > > >>>>> that
> > > >>>>> functionality in the recent cleanup. Specifically, the SMEM-based DRAM
> > > >>>>> size detection was removed in commit 14868845db54 ("board:
> > > >>>>> dragonboard410c: import board code from mach-snapdragon" [2]), the
> > > >>>>> msm_fixup_memory() function does not seem to exist anymore now. :')
> > > >>>>
> > > >>>> Ah now I see the reasoning for that particular piece of code. Is SMEM
> > > >>>> based approach the standardized way used by early stage boot-loaders
> > > >>>> on other Qcom SoCs too?
> > > >>>>
> > > >>>
> > > >>> It is definitely used on all the SoCs that were deployed with LK. I am
> > > >>> not entirely sure about the newer ABL/UEFI-based ones. A quick look at
> > > >>> the ABL source code suggests it is abstracted through an EFI protocol
> > > >>> there (so we cannot see where the information comes from with just the
> > > >>> open-source code). However, in my experience SMEM data structures are
> > > >>> usually kept quite stable (or properly versioned), so it is quite likely
> > > >>> that we could use this approach for all Qualcomm SoCs.
> > > >>>
> > > >>
> > > >> If the SoCs which support this standardized way to dynamic discover
> > > >> DRAM size via SMEM then why do we need to rely on DT at all for those
> > > >> SoCs? Can't U-Boot and Linux have the same driver to fetch DRAM size
> > > >> via SMEM? I am not sure if it's an appropriate way for U-Boot to fixup
> > > >> DT when that information can be discovered dynamically.
> > >
> > > [...]
> > >
> > > The reason I decided to hardcode this in DT for U-Boot is because SMEM
> > > currently relies on the driver model and isn't available early enough.
> > >
> > > Also admittedly I just wasn't that familiar with the U-Boot codebase. I
> > > just wanted to avoid hardcoding this in C code, and given that this was
> > > already supported for the "chainloading from ABL" usecase, just
> > > hardcoding the values was the obvious solution.
> > >
> > > I would definitely be open to revisiting this in U-Boot, having an SMEM
> > > framework that we could use without the driver model which would just
> > > take a base address and then let us interact with SMEM and populate the
> > > dram bank data would be a good improvement, and would let us avoid
> > > hardcoding the memory layout in DT. We'd just need to manually find the
> > > SMEM base address in the FDT as part of "dram_init_banksize()" and
> > > retrieve the data there.
> >
> > These are the similar problems Linux has to deal with too but on Qcom
> > platforms it is rather offloaded to bootloaders to rather implement
> > this. It leads to custom DT modification or board code in U-Boot which
> > is hard to maintain. If we want to implement it properly then
> > corresponding bindings should be upstreamed too regarding how DRAM
> > range is to be discovered via SMEM.
> >
> > >
> > > That all being said, I don't see any reason not to define the memory
> > > layout in DT, it's a hardware feature, DT describes the hardware. The
> > > whole "bootloader will fill this in" implies that the bootloader isn't
> > > also using DT as the source of truth, but now with U-Boot it actually
> > > is, so it's all the more important that DT be accurate ;P
> >
> > I agree DT should be accurate and not a fan of DT fixups. However,
> > when it comes to some hardware configuration being discoverable then
> > IMHO DT isn't the appropriate place for that. For the time being I am
> > fine with the DRAM range to be specified in DT.
> >
> > > >
> > > > You're mixing two things. Linux expects a devicetree where
> > > > /memory/reg[size]
> > > > is valid. Such driver should indeed be (re)implemented in u-boot to provide
> > > > this information.
> >
> > No, I don't think so. We should rather start thinking about the
> > overall stack rather than just being Linux kernel centric. Do you have
> > a generic solution for U-Boot regarding how this should be
> > implemented?
> >
>
> Detecting available memory via /memory in the DT

I would rather call that as "Hardcoding available memory via ...". The
basic DT description [1] says:

"A DTSpec-compliant devicetree describes device information in a
system that cannot necessarily be dynamically detected by a client
program."

[1] https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#overview

> or other firmware
> services (such as UEFI GetMemoryMap()) is *the* generic solution used
> everywhere, independent of the hardware (e.g. Qualcomm) and the
> operating system (e.g. Linux or Xen).

That's an appropriate alternate example of how available memory
information can be passed to the operating system. But on non-UEFI
systems, bootloaders are just stuck with DT fixups as the operating
system doesn't support an alternate method to retrieve memory
information.

IMHO, whether it's a UEFI system or a non-UEFI system the DT should be
identical without the need for fixups.

>
> It allows us to implement the board/Qualcomm-specific memory detection
> in a single project,

I suppose in your view U-Boot is the only bootloader project out there
but you should at least check out [2].

[2] https://en.wikipedia.org/wiki/Bootloader

> rather than having extra porting overhead for each
> operating system for something as essential as available memory.
>
> If we implement the SMEM-based memory detection in U-Boot (probably in
> dram_init_banksize() as Caleb suggested) everything else will just work
> without any Qualcomm-specific DT patching in U-Boot, and without any
> special code in the operating system:
>
>  - U-Boot automatically updates the /memory node in generic code (see
>    fdt_fixup_memory_banks() call in arch/arm/lib/bootm-fdt.c). If you
>    are using UEFI for booting, U-Boot will provide the same information
>    via GetMemoryMap(). (The DT spec says /memory should be ignored on
>    UEFI systems.)
>
>  - Linux, Xen, and any other operating system can obtain the memory size
>    via the standard method, and do not need any Qualcomm-specific at all
>    (at least for memory detection).
>
> I don't think there is anything Linux kernel centric for the memory
> detection here. Quite the opposite really. :)

The question here is really about the thinking that people still
consider DT as a way to describe information which should rather be
detected dynamically. The DT fixups seems to be an outcome of this
thinking.

On a non-UEFI system, if people are instead looking for a generic way
to pass information then we should be able to consider firmware
handoff data structures [3] too. However, if we can just have a
reference in DT /memory node to a particular data structure then we
should just be fine with the corresponding driver extracting the
memory information.

[3] https://github.com/FirmwareHandoff/firmware_handoff

-Sumit

>
> Thanks,
> Stephan

Re: [PATCH v2 0/3] arm64: dts: qcom: apq8016: Add Schneider HMIBSC board DTS

[Krzysztof Kozlowski]

On 13/03/2024 13:30, Sumit Garg wrote:
> Add Schneider Electric HMIBSC board DTS. The HMIBSC board is an IIoT Edge
> Box Core board based on the Qualcomm APQ8016E SoC. For more information
> refer to the product page [1].
> 
> One of the major difference from db410c is serial port where HMIBSC board
> uses UART1 as the debug console with a default RS232 mode (UART1 mode mux
> configured via gpio99 and gpio100).
> 
> Support for Schneider Electric HMIBSC. Features:
> - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306)
> - 1GiB RAM
> - 8GiB eMMC, SD slot
> - WiFi and Bluetooth
> - 2x Host, 1x Device USB port
> - HDMI
> - Discrete TPM2 chip over SPI
> - USB ethernet adaptors (soldered)
> 
> This series is a v2 since v1 of this DTS file has been reviewed on the
> U-Boot mailing list [2].
> 
> Changes in v2:
> - Fix DT schema warnings.

Quick look tells me this is not the case.

Best regards,
Krzysztof