On 11/03/2024 11:10, Sumit Garg wrote:

Enabling LINUX_KERNEL_IMAGE_HEADER by default doesn't allow ENABLE_ARM_SOC_BOOT0_HOOK to work properly on db410c when U-Boot is loaded as a first stage bootloader. It leads to secondary CPUs bringup failure and later causing the Linux kernel to freeze.

So fix it via selectively enabling LINUX_KERNEL_IMAGE_HEADER where it's actually required.

Fixes: 059d526af312 ("mach-snapdragon: generalise board support") Signed-off-by: Sumit Garg sumit.garg@linaro.org

Reviewed-by: Caleb Connolly caleb.connolly@linaro.org

---

arch/arm/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 65fa7ba4ce7..27f3d9a43e1 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -1090,7 +1090,7 @@ config ARCH_SNAPDRAGON select BOARD_LATE_INIT select OF_BOARD select SAVE_PREV_BL_FDT_ADDR

• select LINUX_KERNEL_IMAGE_HEADER

• select LINUX_KERNEL_IMAGE_HEADER if !ENABLE_ARM_SOC_BOOT0_HOOK imply CMD_DM

config ARCH_SOCFPGA

Hi Sumit,

On 11/03/2024 11:10, Sumit Garg wrote:

SE HMIBSC board uses UART1 as the main debug console, so add corresponding clocks and pinmux support. Along with that update instructions to enable clocks for debug UART support.

Signed-off-by: Sumit Garg sumit.garg@linaro.org

drivers/clk/qcom/clock-apq8016.c | 50 +++++++++++++++++++++----- drivers/pinctrl/qcom/pinctrl-apq8016.c | 1 + drivers/serial/serial_msm.c | 6 ++-- 3 files changed, 47 insertions(+), 10 deletions(-)

diff --git a/drivers/clk/qcom/clock-apq8016.c b/drivers/clk/qcom/clock-apq8016.c index e6647f7c41d..a620a10a520 100644 --- a/drivers/clk/qcom/clock-apq8016.c +++ b/drivers/clk/qcom/clock-apq8016.c @@ -43,6 +43,14 @@ #define BLSP1_UART2_APPS_N (0x3040) #define BLSP1_UART2_APPS_D (0x3044)

+#define BLSP1_UART1_BCR (0x2038) +#define BLSP1_UART1_APPS_CBCR (0x203C) +#define BLSP1_UART1_APPS_CMD_RCGR (0x2044) +#define BLSP1_UART1_APPS_CFG_RCGR (0x2048) +#define BLSP1_UART1_APPS_M (0x204C) +#define BLSP1_UART1_APPS_N (0x2050) +#define BLSP1_UART1_APPS_D (0x2054)

/* GPLL0 clock control registers */ #define GPLL0_STATUS_ACTIVE BIT(17)

@@ -77,7 +85,7 @@ static struct vote_clk gcc_blsp1_ahb_clk = { };

/* SDHCI */ -static int clk_init_sdc(struct msm_clk_priv *priv, int slot, uint rate) +static int apq8016_clk_init_sdc(struct msm_clk_priv *priv, int slot, uint rate)

This seems like an unrelated change, I don't think we need to namespace this function as it's static.

{ int div = 15; /* 100MHz default */

@@ -94,6 +102,33 @@ static int clk_init_sdc(struct msm_clk_priv *priv, int slot, uint rate) return rate; }

+static const struct bcr_regs uart1_regs = {

• .cfg_rcgr = BLSP1_UART1_APPS_CFG_RCGR,
• .cmd_rcgr = BLSP1_UART1_APPS_CMD_RCGR,
• .M = BLSP1_UART1_APPS_M,
• .N = BLSP1_UART1_APPS_N,
• .D = BLSP1_UART1_APPS_D,

+};

+/* UART: 115200 */ +static int apq8016_clk_init_uart1(phys_addr_t base)

I know we're still dealing with some tech debt here, but I'm not a big fan of this approach. I notice that the RCG and CBCR registers are all offset by exactly 0xff0 between UART1 and UART2, what about adding a second "index" parameter to apq8016_clk_init_uart() and then offsetting the addresses by (0xff0 * index)?

This will get cleaner once we drop the bcr_regs struct.

+{

• /* Enable AHB clock */
• clk_enable_vote_clk(base, &gcc_blsp1_ahb_clk);
• /* 7372800 uart block clock @ GPLL0 */
• clk_rcg_set_rate_mnd(base, &uart1_regs, 1, 144, 15625,

• 	     CFG_CLK_SRC_GPLL0, 16);
  

• /* Vote for gpll0 clock */
• clk_enable_gpll0(base, &gpll0_vote_clk);
• /* Enable core clk */
• clk_enable_cbc(base + BLSP1_UART1_APPS_CBCR);
• return 0;

+}

static const struct bcr_regs uart2_regs = { .cfg_rcgr = BLSP1_UART2_APPS_CFG_RCGR, .cmd_rcgr = BLSP1_UART2_APPS_CMD_RCGR, @@ -103,7 +138,7 @@ static const struct bcr_regs uart2_regs = { };

/* UART: 115200 */ -int apq8016_clk_init_uart(phys_addr_t base) +int apq8016_clk_init_uart2(phys_addr_t base) { /* Enable AHB clock */ clk_enable_vote_clk(base, &gcc_blsp1_ahb_clk); @@ -127,14 +162,13 @@ static ulong apq8016_clk_set_rate(struct clk *clk, ulong rate)

switch (clk->id) { case GCC_SDCC1_APPS_CLK: /* SDC1 */

• return clk_init_sdc(priv, 0, rate);
  

• break;
  

• return apq8016_clk_init_sdc(priv, 0, rate);
  

  case GCC_SDCC2_APPS_CLK: /* SDC2 */

• return clk_init_sdc(priv, 1, rate);
  

• break;
  

• return apq8016_clk_init_sdc(priv, 1, rate);
  

  case GCC_BLSP1_UART2_APPS_CLK: /* UART2 */

• return apq8016_clk_init_uart(priv->base);
  

• break;
  

• return apq8016_clk_init_uart2(priv->base);
  

• case GCC_BLSP1_UART1_APPS_CLK: /* UART1 */

• return apq8016_clk_init_uart1(priv->base);
  

  default: return 0; }

diff --git a/drivers/pinctrl/qcom/pinctrl-apq8016.c b/drivers/pinctrl/qcom/pinctrl-apq8016.c index db0e2124684..cb0e2227079 100644 --- a/drivers/pinctrl/qcom/pinctrl-apq8016.c +++ b/drivers/pinctrl/qcom/pinctrl-apq8016.c @@ -29,6 +29,7 @@ static const char * const msm_pinctrl_pins[] = { };

static const struct pinctrl_function msm_pinctrl_functions[] = {

• {"blsp_uart1", 2}, {"blsp_uart2", 2},

};

diff --git a/drivers/serial/serial_msm.c b/drivers/serial/serial_msm.c index ac4280c6c4c..eaf024a55b0 100644 --- a/drivers/serial/serial_msm.c +++ b/drivers/serial/serial_msm.c @@ -248,12 +248,14 @@ static struct msm_serial_data init_serial_data = { #include <debug_uart.h>

/* Uncomment to turn on UART clocks when debugging U-Boot as aboot on MSM8916 */

Please update the comment to offer some hints about which UART should be enabled.

-//int apq8016_clk_init_uart(phys_addr_t gcc_base); +//int apq8016_clk_init_uart1(phys_addr_t gcc_base); +//int apq8016_clk_init_uart2(phys_addr_t gcc_base);

static inline void _debug_uart_init(void) { /* Uncomment to turn on UART clocks when debugging U-Boot as aboot on MSM8916 */

• //apq8016_clk_init_uart(0x1800000);

• //apq8016_clk_init_uart1(0x1800000);
• //apq8016_clk_init_uart2(0x1800000); uart_dm_init(&init_serial_data);

}

On 11/03/2024 13:32, Stephan Gerhold wrote:

On Mon, Mar 11, 2024 at 12:27:11PM +0000, Caleb Connolly wrote:

...

On 11/03/2024 11:10, Sumit Garg wrote:

...

SE HMIBSC board uses UART1 as the main debug console, so add corresponding clocks and pinmux support. Along with that update instructions to enable clocks for debug UART support.

Signed-off-by: Sumit Garg sumit.garg@linaro.org

drivers/clk/qcom/clock-apq8016.c | 50 +++++++++++++++++++++----- drivers/pinctrl/qcom/pinctrl-apq8016.c | 1 + drivers/serial/serial_msm.c | 6 ++-- 3 files changed, 47 insertions(+), 10 deletions(-)

diff --git a/drivers/clk/qcom/clock-apq8016.c b/drivers/clk/qcom/clock-apq8016.c index e6647f7c41d..a620a10a520 100644 --- a/drivers/clk/qcom/clock-apq8016.c +++ b/drivers/clk/qcom/clock-apq8016.c @@ -43,6 +43,14 @@ #define BLSP1_UART2_APPS_N (0x3040) #define BLSP1_UART2_APPS_D (0x3044)

+#define BLSP1_UART1_BCR (0x2038) +#define BLSP1_UART1_APPS_CBCR (0x203C) +#define BLSP1_UART1_APPS_CMD_RCGR (0x2044) +#define BLSP1_UART1_APPS_CFG_RCGR (0x2048) +#define BLSP1_UART1_APPS_M (0x204C) +#define BLSP1_UART1_APPS_N (0x2050) +#define BLSP1_UART1_APPS_D (0x2054)

/* GPLL0 clock control registers */ #define GPLL0_STATUS_ACTIVE BIT(17)

[...] @@ -94,6 +102,33 @@ static int clk_init_sdc(struct msm_clk_priv *priv, int slot, uint rate) return rate; }

+static const struct bcr_regs uart1_regs = {

• .cfg_rcgr = BLSP1_UART1_APPS_CFG_RCGR,
• .cmd_rcgr = BLSP1_UART1_APPS_CMD_RCGR,
• .M = BLSP1_UART1_APPS_M,
• .N = BLSP1_UART1_APPS_N,
• .D = BLSP1_UART1_APPS_D,

+};

+/* UART: 115200 */ +static int apq8016_clk_init_uart1(phys_addr_t base)

I know we're still dealing with some tech debt here, but I'm not a big fan of this approach. I notice that the RCG and CBCR registers are all offset by exactly 0xff0 between UART1 and UART2, what about adding a second "index" parameter to apq8016_clk_init_uart() and then offsetting the addresses by (0xff0 * index)?

This would work for MSM8916 where you have just two UARTs, but it might be misleading since the UART blocks are actually separated in 4 KiB (0x1000) blocks and not offset by 0xff0. UART2 is a special case that has different offsets within the 4 KiB block, for some weird reason.

If you want to calculate the register offsets properly you would need special handling for UART2, e.g. I have the following (admittedly rather ugly) define in the TF-A port for MSM8916 and similar [1]:

#define GCC_BLSP1_UART_APPS_CBCR(n) (GCC_BASE + \ (((n) == 2) ? (0x0302c) : (0x0203c + (((n) - 1) * 0x1000))))

where n is the UART number (here 1 or 2). As a different example, MDM9607 has 5 UARTs (1 to 5) and there only UART2 is special-cased, while all others follow the standard offset with 0x1000 offset.

fwiw I would also be fine with just treating it like a binary switch and only allowing UART1 or UART2 to be selected if that's simpler.

Thanks, Stephan

On Mon, 11 Mar 2024 at 17:57, Caleb Connolly caleb.connolly@linaro.org wrote:

Hi Sumit,

On 11/03/2024 11:10, Sumit Garg wrote:

...

SE HMIBSC board uses UART1 as the main debug console, so add corresponding clocks and pinmux support. Along with that update instructions to enable clocks for debug UART support.

Signed-off-by: Sumit Garg sumit.garg@linaro.org

drivers/clk/qcom/clock-apq8016.c | 50 +++++++++++++++++++++----- drivers/pinctrl/qcom/pinctrl-apq8016.c | 1 + drivers/serial/serial_msm.c | 6 ++-- 3 files changed, 47 insertions(+), 10 deletions(-)

diff --git a/drivers/clk/qcom/clock-apq8016.c b/drivers/clk/qcom/clock-apq8016.c index e6647f7c41d..a620a10a520 100644 --- a/drivers/clk/qcom/clock-apq8016.c +++ b/drivers/clk/qcom/clock-apq8016.c @@ -43,6 +43,14 @@ #define BLSP1_UART2_APPS_N (0x3040) #define BLSP1_UART2_APPS_D (0x3044)

+#define BLSP1_UART1_BCR (0x2038) +#define BLSP1_UART1_APPS_CBCR (0x203C) +#define BLSP1_UART1_APPS_CMD_RCGR (0x2044) +#define BLSP1_UART1_APPS_CFG_RCGR (0x2048) +#define BLSP1_UART1_APPS_M (0x204C) +#define BLSP1_UART1_APPS_N (0x2050) +#define BLSP1_UART1_APPS_D (0x2054)

/* GPLL0 clock control registers */ #define GPLL0_STATUS_ACTIVE BIT(17)

@@ -77,7 +85,7 @@ static struct vote_clk gcc_blsp1_ahb_clk = { };

/* SDHCI */ -static int clk_init_sdc(struct msm_clk_priv *priv, int slot, uint rate) +static int apq8016_clk_init_sdc(struct msm_clk_priv *priv, int slot, uint rate)

This seems like an unrelated change, I don't think we need to namespace this function as it's static.

We should follow the same naming convention within a driver to avoid confusion.

[snip]

...

diff --git a/drivers/serial/serial_msm.c b/drivers/serial/serial_msm.c index ac4280c6c4c..eaf024a55b0 100644 --- a/drivers/serial/serial_msm.c +++ b/drivers/serial/serial_msm.c @@ -248,12 +248,14 @@ static struct msm_serial_data init_serial_data = { #include <debug_uart.h>

/* Uncomment to turn on UART clocks when debugging U-Boot as aboot on MSM8916 */

Please update the comment to offer some hints about which UART should be enabled.

Okay I can add a hint for UART to be board specific.

-Sumit

...

-//int apq8016_clk_init_uart(phys_addr_t gcc_base); +//int apq8016_clk_init_uart1(phys_addr_t gcc_base); +//int apq8016_clk_init_uart2(phys_addr_t gcc_base);

static inline void _debug_uart_init(void) { /* Uncomment to turn on UART clocks when debugging U-Boot as aboot on MSM8916 */

• //apq8016_clk_init_uart(0x1800000);
  

• //apq8016_clk_init_uart1(0x1800000);
  

• //apq8016_clk_init_uart2(0x1800000);
  uart_dm_init(&init_serial_data);
  

}

-- // Caleb (they/them)

On 11/03/2024 11:10, Sumit Garg wrote:

SE HMIBSC board debug console requires RS232 flow control, so enable corresponding support if RS232 gpios are present.

Signed-off-by: Sumit Garg sumit.garg@linaro.org

Reviewed-by: Caleb Connolly caleb.connolly@linaro.org

---

drivers/serial/serial_msm.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/serial/serial_msm.c b/drivers/serial/serial_msm.c index eaf024a55b0..7b2a3fdb2c1 100644 --- a/drivers/serial/serial_msm.c +++ b/drivers/serial/serial_msm.c @@ -53,10 +53,11 @@ #define UARTDM_TF 0x100 /* UART Transmit FIFO register */ #define UARTDM_RF 0x140 /* UART Receive FIFO register */

-#define UART_DM_CLK_RX_TX_BIT_RATE 0xCC -#define MSM_BOOT_UART_DM_8_N_1_MODE 0x34 -#define MSM_BOOT_UART_DM_CMD_RESET_RX 0x10 -#define MSM_BOOT_UART_DM_CMD_RESET_TX 0x20 +#define UART_DM_CLK_RX_TX_BIT_RATE 0xCC +#define MSM_BOOT_UART_DM_8_N_1_MODE 0x34 +#define MSM_BOOT_UART_DM_CMD_RESET_RX 0x10 +#define MSM_BOOT_UART_DM_CMD_RESET_TX 0x20 +#define MSM_UART_MR1_RX_RDY_CTL BIT(7)

DECLARE_GLOBAL_DATA_PTR;

@@ -182,7 +183,9 @@ static void uart_dm_init(struct msm_serial_data *priv) mdelay(5);

writel(priv->clk_bit_rate, priv->base + UARTDM_CSR);

• writel(0x0, priv->base + UARTDM_MR1);

• /* Enable RS232 flow control to support RS232 db9 connector */
• writel(MSM_UART_MR1_RX_RDY_CTL, priv->base + UARTDM_MR1); writel(MSM_BOOT_UART_DM_8_N_1_MODE, priv->base + UARTDM_MR2); writel(MSM_BOOT_UART_DM_CMD_RESET_RX, priv->base + UARTDM_CR); writel(MSM_BOOT_UART_DM_CMD_RESET_TX, priv->base + UARTDM_CR);

Hi Sumit,

On 11/03/2024 11:10, Sumit Garg wrote:

Add support for driving the GPIO pins as output low or high.

Ohh, this is why it was never working for me >,<

Signed-off-by: Sumit Garg sumit.garg@linaro.org

drivers/pinctrl/qcom/pinctrl-apq8016.c | 1 + drivers/pinctrl/qcom/pinctrl-qcom.c | 26 +++++++++++++++++++++----- 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-apq8016.c b/drivers/pinctrl/qcom/pinctrl-apq8016.c index cb0e2227079..04b501c563d 100644 --- a/drivers/pinctrl/qcom/pinctrl-apq8016.c +++ b/drivers/pinctrl/qcom/pinctrl-apq8016.c @@ -29,6 +29,7 @@ static const char * const msm_pinctrl_pins[] = { };

static const struct pinctrl_function msm_pinctrl_functions[] = {

• {"gpio", 0},

Please split this into a separate patch.

{"blsp_uart1", 2}, {"blsp_uart2", 2}, }; diff --git a/drivers/pinctrl/qcom/pinctrl-qcom.c b/drivers/pinctrl/qcom/pinctrl-qcom.c index ee0624df296..932be7c4840 100644 --- a/drivers/pinctrl/qcom/pinctrl-qcom.c +++ b/drivers/pinctrl/qcom/pinctrl-qcom.c @@ -29,15 +29,25 @@ struct msm_pinctrl_priv { #define GPIO_CONFIG_REG(priv, x) \ (qcom_pin_offset((priv)->data->pin_data.pin_offsets, x))

-#define TLMM_GPIO_PULL_MASK GENMASK(1, 0) -#define TLMM_FUNC_SEL_MASK GENMASK(5, 2) -#define TLMM_DRV_STRENGTH_MASK GENMASK(8, 6) -#define TLMM_GPIO_DISABLE BIT(9) +#define GPIO_IN_OUT_REG(priv, x) \

• (GPIO_CONFIG_REG(priv, x) + 0x4)

+#define TLMM_GPIO_PULL_MASK GENMASK(1, 0) +#define TLMM_FUNC_SEL_MASK GENMASK(5, 2) +#define TLMM_DRV_STRENGTH_MASK GENMASK(8, 6) +#define TLMM_GPIO_OUTPUT_MASK BIT(1) +#define TLMM_GPIO_OE_MASK BIT(9)

+/* GPIO_IN_OUT register shifts. */ +#define GPIO_IN 0 +#define GPIO_OUT 1

Are there two separate bits? GPIO_IN is never used? Maybe just define GPIO_OUT as BIT(0) instead?

static const struct pinconf_param msm_conf_params[] = { { "drive-strength", PIN_CONFIG_DRIVE_STRENGTH, 2 }, { "bias-disable", PIN_CONFIG_BIAS_DISABLE, 0 }, { "bias-pull-up", PIN_CONFIG_BIAS_PULL_UP, 3 },

• { "output-high", PIN_CONFIG_OUTPUT, 1, },
• { "output-low", PIN_CONFIG_OUTPUT, 0, },

};

static int msm_get_functions_count(struct udevice *dev) @@ -89,7 +99,7 @@ static int msm_pinmux_set(struct udevice *dev, unsigned int pin_selector, return 0;

clrsetbits_le32(priv->base + GPIO_CONFIG_REG(priv, pin_selector),

• 	TLMM_FUNC_SEL_MASK | TLMM_GPIO_DISABLE,
  

• 	TLMM_FUNC_SEL_MASK | TLMM_GPIO_OE_MASK,
  priv->data->get_function_mux(func_selector) << 2);
  

  return 0;

} @@ -117,6 +127,12 @@ static int msm_pinconf_set(struct udevice *dev, unsigned int pin_selector, clrsetbits_le32(priv->base + GPIO_CONFIG_REG(priv, pin_selector), TLMM_GPIO_PULL_MASK, argument); break;

• case PIN_CONFIG_OUTPUT:

• writel(argument << GPIO_OUT,
  

Then this can be "argument ? GPIO_OUT : GPIO_IN" which feels much cleaner. Or even just !!argument if it's bit 0.

•        priv->base + GPIO_IN_OUT_REG(priv, pin_selector));
  

• setbits_le32(priv->base + GPIO_CONFIG_REG(priv, pin_selector),
  

• 	     TLMM_GPIO_OE_MASK);
  

• break;
  

  default: return 0; }

Hi Stephan,

On Mon, 11 Mar 2024 at 20:07, Stephan Gerhold stephan@gerhold.net wrote:

On Mon, Mar 11, 2024 at 04:40:26PM +0530, Sumit Garg wrote:

...

Support for Schneider Electric HMIBSC. Features:

• Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306)
• 2GiB RAM
• 64GiB eMMC, SD slot
• WiFi and Bluetooth
• 2x Host, 1x Device USB port
• HDMI
• Discrete TPM2 chip over SPI

Features enabled in U-Boot:

• RAUC updates
• Environment protection
• USB based ethernet adaptors

Signed-off-by: Sumit Garg sumit.garg@linaro.org

I'm entirely sure which requirements or conventions we are following for adding device trees directly to U-Boot instead of Linux. My understanding is that the goal is to get U-Boot DTs as close as possible to the upstream Linux DTs, so I effectively looked at this as if it were submitted to linux-arm-msm. I think most of my comments should be trivial to fix anyway without much effort. :-)

Thanks for your review and yeah I would be happy to incorporate your comments.

With the comments fixed it would be likely also easy to get it in upstream in Linux, so I wonder if it's worth first adding it here in U-Boot (I know you discussed this on v1 already a bit).

I will post a DTS patch for Linux kernel too as soon as I get a go ahead from the vendor. But for the time being it shouldn't be a barrier to U-Boot support.

...

---

arch/arm/dts/apq8016-hmibsc.dts | 496 +++++++++++++++++++++++++++++ board/schneider/hmibsc/MAINTAINERS | 6 + configs/hmibsc_defconfig | 87 +++++ doc/board/index.rst | 1 + doc/board/schneider/hmibsc.rst | 45 +++ doc/board/schneider/index.rst | 9 + include/configs/hmibsc.h | 57 ++++ 7 files changed, 701 insertions(+) create mode 100644 arch/arm/dts/apq8016-hmibsc.dts create mode 100644 board/schneider/hmibsc/MAINTAINERS create mode 100644 configs/hmibsc_defconfig create mode 100644 doc/board/schneider/hmibsc.rst create mode 100644 doc/board/schneider/index.rst create mode 100644 include/configs/hmibsc.h

diff --git a/arch/arm/dts/apq8016-hmibsc.dts b/arch/arm/dts/apq8016-hmibsc.dts new file mode 100644 index 00000000000..490ab5fd2fa --- /dev/null +++ b/arch/arm/dts/apq8016-hmibsc.dts @@ -0,0 +1,496 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*

• • Copyright (c) 2015, The Linux Foundation. All rights reserved.
• • Copyright (c) 2024, Linaro Ltd.
• */

+/dts-v1/;

+#include "msm8916-pm8916.dtsi" +#include <dt-bindings/gpio/gpio.h> +#include <dt-bindings/input/input.h> +#include <dt-bindings/leds/common.h> +#include <dt-bindings/pinctrl/qcom,pmic-gpio.h> +#include <dt-bindings/pinctrl/qcom,pmic-mpp.h> +#include <dt-bindings/sound/apq8016-lpass.h>

+/ {

• model = "Schneider Electric HMIBSC Board";
  

• compatible = "qcom,apq8016-hmibsc", "qcom,apq8016";
  

A Schneider Electric specific compatible would be likely more accurate, since I assume this board wasn't designed by Qualcomm?

Okay, I will make it: "schneider,apq8016-hmibsc" instead.

I would personally also prefer to use the "apq8016-<vendor>-<board>.dts" naming convention that we typically use for smartphones/tablets upstream, although you can also keep it as-is since e.g. apq8039-t2.dts is also named without vendor.

That sounds better. I will rename DTS file as "apq8016-schneider-hmibsc.dts"

...

• aliases {
  

•         serial0 = &blsp_uart1;
  

•         serial1 = &blsp_uart2;
  

•         usid0 = &pm8916_0;
  

•         i2c1 = &blsp_i2c6;
  

•         i2c3 = &blsp_i2c4;
  

•         i2c4 = &blsp_i2c3;
  

•         spi0 = &blsp_spi5;
  

You might want to add mmcX aliases here to ensure consistent naming of eMMC and SD card (this used to be in msm8916.dtsi but not anymore).

Ack.

...

[...] +&blsp_i2c6 {

• status = "okay";
  

• label = "I2C1";
  

• rtc1: s35390a@30 {
  

rtc@

Ack.

...

•         compatible = "sii,s35390a";
  

•         reg = <0x30>;
  

• };
  

• eeprom1: 24c256@50 {
  

eeprom@

Ack.

...

•         compatible = "atmel,24c256";
  

•         reg = <0x50>;
  

• };
  

+};

+&blsp_i2c3 {

i2c3 should come before i2c6 (sorted alphabetically)

Ack.

...

• status = "okay";
  

• label = "I2C4";
  

• eeprom: 24c32@50 {
  

eeprom@

Ack.

...

•         compatible = "onsemi,24c32";
  

•         reg = <0x50>;
  

• };
  

+};

[...]

+&pm8916_0 {

• pon@800 {
  

•         pwrkey {
  

•                 status = "okay";
  

•                 interrupts = <0x0 0x8 1 IRQ_TYPE_EDGE_BOTH>;
  

This line would really benefit from a comment that explains what exactly it does and why this is done. :)

It looks like you are redefining the pwrkey with the resin interrupt. I guess your goal is to have KEY_POWER assigned to the resin pin? In that case, I think it would be cleaner to describe this using:

&pm8916_resin { status = "okay"; linux,code = <KEY_POWER>; };

This works much better, I will use it instead.

and leave the pwrkey node alone (or perhaps disable it if it causes trouble).

Aside from the confusion, I think overriding only the interrupt of the pwrkey will also misbehave in unexpected ways since e.g. the Linux pm8941-pwrkey driver will still write the configured debounce time and pull up to the pwrkey registers, and not to the resin ones.

...

•         };
  

• };
  

+};

[...]

+&tlmm {

• pinctrl-names = "default";
  

• pinctrl-0 = <&gpio_rs232_high &gpio_rs232_low>;
  

• sdc2_cd_default: sdc2-cd-default-state {
  

•         pins = "gpio38";
  

•         function = "gpio";
  

•         drive-strength = <2>;
  

•         bias-disable;
  

• };
  

• usb_id_default: usb-id-default-state {
  

•         pins = "gpio110";
  

•         function = "gpio";
  

•         drive-strength = <8>;
  

•         bias-pull-up;
  

• };
  

• adv7533_int_active: adv533-int-active-state {
  

•         pins = "gpio31";
  

•         function = "gpio";
  

•         drive-strength = <16>;
  

•         bias-disable;
  

• };
  

• adv7533_int_suspend: adv7533-int-suspend-state {
  

•         pins = "gpio31";
  

•         function = "gpio";
  

•         drive-strength = <2>;
  

•         bias-disable;
  

• };
  

• adv7533_switch_active: adv7533-switch-active-state {
  

•         pins = "gpio32";
  

•         function = "gpio";
  

•         drive-strength = <16>;
  

•         bias-disable;
  

• };
  

• adv7533_switch_suspend: adv7533-switch-suspend-state {
  

•         pins = "gpio32";
  

•         function = "gpio";
  

•         drive-strength = <2>;
  

•         bias-disable;
  

• };
  

• msm_key_volp_n_default: msm-key-volp-n-default-state {
  

•         pins = "gpio107";
  

•         function = "gpio";
  

•         drive-strength = <8>;
  

•         bias-pull-up;
  

• };
  

• gpio_rs232_high: gpio_rs232_high {
  

Pretty sure DT schema checks would complain about this node name (need -state suffix, no underscores).

We have the dtbs_check in U-Boot too. I will use that before posting the next version.

...

•         bootph-all;
  

•         pins = "gpio99";
  

•         function = "gpio";
  

•         drive-strength = <16>;
  

•         bias-disable;
  

•         output-high;
  

• };
  

• gpio_rs232_low: gpio_rs232_low {
  

Same here.

Also, since I'm looking at this a bit more closely now, are there maybe more clear label/node names you could use here, or a comment you could add what exactly these pins do? I guess this enables something about RS232 but it's not clear what exactly.

Actually these GPIOs are a mux to select among different UART modes (RS232/422/485). This configuration allows you to select RS232 mode. How about following label/node names?

uart1_mux0_rs232_high: uart1-mux0-rs232-state

uart1_mux1_rs232_low: uart1-mux1-rs232-state

...

•         bootph-all;
  

•         pins = "gpio100";
  

•         function = "gpio";
  

•         drive-strength = <16>;
  

•         bias-disable;
  

•         output-low;
  

• };
  

+};

[...] diff --git a/configs/hmibsc_defconfig b/configs/hmibsc_defconfig new file mode 100644 index 00000000000..02b9615114b --- /dev/null +++ b/configs/hmibsc_defconfig @@ -0,0 +1,87 @@ +CONFIG_ARM=y +CONFIG_SYS_BOARD="hmibsc" +CONFIG_COUNTER_FREQUENCY=19000000

I see you just copied this from the existing defconfigs but CONFIG_COUNTER_FREQUENCY should be unneeded, since TZ is the one responsible (and only one capable) of configuring this. And it also looks wrong to me, because the timer frequency on these Qualcomm boards is 19.2 MHz and not 19.0 MHz. :'D

I guess I'll prepare a patch to fix it for the existing boards.

Okay I will drop that config.

-Sumit

Thanks, Stephan