[PATCH v2 0/5] Add SE HMBSC board support

[PATCH v2 0/5] Add SE HMBSC board support

[Sumit Garg]

SE HMIBSC board is based on Qcom APQ8016 SoC. One of the major
difference from db410c is serial port where HMIBSC board uses UART1 as
the debug console with an RS232 port, patch #2 - #4 adds corresponding
driver support.

Patch #5 adds main HMIBSC board specific bits, 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 (refer [2] for more details)
- Environment protection
- USB based ethernet adaptors

Feedback is very much welcome.

Changes in v2:
- Rebased on top on qcom-next [1]
- Added patch#1 as a fix for generic qcom board support.
- Added patch#4 to enable driving GPIO pins based on pinctrl
  configuration. This replaces the custom GPIO configuration.
- Added proper DTS file for HMIBSC board based on Linux DT pattern.
- Merged board support patches into a single patch#5.

[1] https://source.denx.de/u-boot/custodians/u-boot-snapdragon/-/commits/qcom-next?ref_type=heads
[2] https://rauc.readthedocs.io/en/latest/

Sumit Garg (5):
  qcom: Don't enable LINUX_KERNEL_IMAGE_HEADER by default
  apq8016: Add support for UART1 clocks and pinmux
  serial_msm: Enable RS232 flow control
  pinctrl: qcom: Add support for driving GPIO pins output
  board: add support for Schneider HMIBSC board

 arch/arm/Kconfig                       |   2 +-
 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 +
 drivers/clk/qcom/clock-apq8016.c       |  50 ++-
 drivers/pinctrl/qcom/pinctrl-apq8016.c |   2 +
 drivers/pinctrl/qcom/pinctrl-qcom.c    |  26 +-
 drivers/serial/serial_msm.c            |  19 +-
 include/configs/hmibsc.h               |  57 +++
 12 files changed, 779 insertions(+), 21 deletions(-)
 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

-- 
2.34.1

[PATCH v2 1/5] qcom: Don't enable LINUX_KERNEL_IMAGE_HEADER by default

[Sumit Garg]

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>
---
 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
-- 
2.34.1

Re: [PATCH v2 1/5] qcom: Don't enable LINUX_KERNEL_IMAGE_HEADER by default

[Caleb Connolly]

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

-- 
// Caleb (they/them)

[PATCH v2 2/5] apq8016: Add support for UART1 clocks and pinmux

[Sumit Garg]

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)
 {
 	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)
+{
+	/* 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 */
-//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);
 }
 
-- 
2.34.1

Re: [PATCH v2 2/5] apq8016: Add support for UART1 clocks and pinmux

[Caleb Connolly]

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);
>  }
>  

-- 
// Caleb (they/them)

Re: [PATCH v2 2/5] apq8016: Add support for UART1 clocks and pinmux

[Stephan Gerhold]

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.

Thanks,
Stephan

[1]: https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/plat/qti/msm8916/msm8916_setup.c?h=v2.10#n40

Re: [PATCH v2 2/5] apq8016: Add support for UART1 clocks and pinmux

[Caleb Connolly]

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
> 
> [1]: https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/plat/qti/msm8916/msm8916_setup.c?h=v2.10#n40

-- 
// Caleb (they/them)

Re: [PATCH v2 2/5] apq8016: Add support for UART1 clocks and pinmux

[Sumit Garg]

On Mon, 11 Mar 2024 at 20:20, Caleb Connolly <caleb.connolly@linaro.org> wrote:
>
>
>
> 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.

Let me rather pass GCC_BLSP1_UART1_APPS_CLK/GCC_BLSP1_UART2_APPS_CLK
as an argument to the apq8016_clk_init_uart().

-Sumit

> >
> > Thanks,
> > Stephan
> >
> > [1]: https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/plat/qti/msm8916/msm8916_setup.c?h=v2.10#n40
>
> --
> // Caleb (they/them)

Re: [PATCH v2 2/5] apq8016: Add support for UART1 clocks and pinmux

[Sumit Garg]

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)

[PATCH v2 3/5] serial_msm: Enable RS232 flow control

[Sumit Garg]

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>
---
 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);
-- 
2.34.1

Re: [PATCH v2 3/5] serial_msm: Enable RS232 flow control

[Caleb Connolly]

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);

-- 
// Caleb (they/them)

[PATCH v2 4/5] pinctrl: qcom: Add support for driving GPIO pins output

[Sumit Garg]

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

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},
 	{"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
 
 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,
+		       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;
 	}
-- 
2.34.1

Re: [PATCH v2 4/5] pinctrl: qcom: Add support for driving GPIO pins output

[Caleb Connolly]

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;
>  	}

-- 
// Caleb (they/them)

Re: [PATCH v2 4/5] pinctrl: qcom: Add support for driving GPIO pins output

[Sumit Garg]

On Mon, 11 Mar 2024 at 18:07, Caleb Connolly <caleb.connolly@linaro.org> wrote:
>
> 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 >,<

Yeah you only implemented it for PMIC GPIOs.

> >
> > 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.

Okay I can do that.

> >       {"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?

Yeah these are two separate bits. I can rename them as GPIO_IN_SHIFT
and GPIO_OUT_SHIFT if that is more clear.

> GPIO_IN is never used?

Okay I can drop it as it is very unlikely for the pinctrl driver to
read GPIO value.

> 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.

No, here GPIO_OUT is rather a shift for the output bit and "argument"
is the value to be written. As above I can change it to "argument <<
GPIO_OUT_SHIFT" to be more clear.

-Sumit

> > +                    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;
> >       }
>
> --
> // Caleb (they/them)

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

[Sumit Garg]

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>
---
 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";
+
+	aliases {
+		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";
+
+		led@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 {
+			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_i2c4 {
+	status = "okay";
+	label = "I2C2";
+
+	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";
+	label = "I2C1";
+
+	rtc1: s35390a@30 {
+		compatible = "sii,s35390a";
+		reg = <0x30>;
+	};
+
+	eeprom1: 24c256@50 {
+		compatible = "atmel,24c256";
+		reg = <0x50>;
+	};
+};
+
+&blsp_i2c3 {
+	status = "okay";
+	label = "I2C4";
+
+	eeprom: 24c32@50 {
+		compatible = "onsemi,24c32";
+		reg = <0x50>;
+	};
+};
+
+&blsp_spi5 {
+	status = "okay";
+	label = "SPI0";
+	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_0 {
+	pon@800 {
+		pwrkey {
+			status = "okay";
+			interrupts = <0x0 0x8 1 IRQ_TYPE_EDGE_BOTH>;
+		};
+	};
+};
+
+&pm8916_codec {
+	status = "okay";
+	qcom,mbhc-vthreshold-low = <75 150 237 450 500>;
+	qcom,mbhc-vthreshold-high = <75 150 237 450 500>;
+};
+
+&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";
+};
+
+/* 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 = <&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 {
+		bootph-all;
+		pins = "gpio99";
+		function = "gpio";
+
+		drive-strength = <16>;
+		bias-disable;
+		output-high;
+	};
+
+	gpio_rs232_low: gpio_rs232_low {
+		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";
+
+	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 =
+		"WLAN_LED_CTRL",
+		"BT_LED_CTRL";
+
+	pm8916_mpps_leds: pm8916-mpps-state {
+		pins = "mpp2", "mpp3";
+		function = "digital";
+
+		output-low;
+	};
+};
+
+&blsp_uart1_default {
+	bootph-all;
+};
diff --git a/board/schneider/hmibsc/MAINTAINERS b/board/schneider/hmibsc/MAINTAINERS
new file mode 100644
index 00000000000..0f31bbda966
--- /dev/null
+++ b/board/schneider/hmibsc/MAINTAINERS
@@ -0,0 +1,6 @@
+HMIBSC BOARD
+M:	Sumit Garg <sumit.garg@linaro.org>
+S:	Maintained
+F:	board/schneider/hmibsc/
+F:	include/configs/hmibsc.h
+F:	configs/hmibsc_defconfig
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
+CONFIG_ENABLE_ARM_SOC_BOOT0_HOOK=y
+CONFIG_ARCH_SNAPDRAGON=y
+CONFIG_TEXT_BASE=0x8f600000
+CONFIG_SYS_MALLOC_LEN=0x802000
+CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
+CONFIG_CUSTOM_SYS_INIT_SP_ADDR=0x8007fff0
+CONFIG_ENV_SIZE=0x2000
+CONFIG_ENV_OFFSET=0x0
+CONFIG_DEFAULT_DEVICE_TREE="apq8016-hmibsc"
+# CONFIG_OF_UPSTREAM is not set
+CONFIG_OF_LIBFDT_OVERLAY=y
+CONFIG_IDENT_STRING="\nSchneider Electric-HMIBSC"
+CONFIG_SYS_LOAD_ADDR=0x80080000
+CONFIG_REMAKE_ELF=y
+# CONFIG_ANDROID_BOOT_IMAGE is not set
+CONFIG_FIT=y
+CONFIG_HUSH_PARSER=y
+CONFIG_SYS_CBSIZE=2048
+# CONFIG_DISPLAY_CPUINFO is not set
+# CONFIG_DISPLAY_BOARDINFO is not set
+CONFIG_SYS_PROMPT="hmibsc => "
+CONFIG_SYS_MAXARGS=64
+CONFIG_CMD_DHCP=y
+CONFIG_CMD_PING=y
+CONFIG_CMD_FS_GENERIC=y
+# CONFIG_CMD_IMI is not set
+CONFIG_CMD_MD5SUM=y
+CONFIG_CMD_MEMINFO=y
+CONFIG_CMD_EXT2=y
+CONFIG_CMD_EXT4=y
+CONFIG_CMD_FAT=y
+CONFIG_CMD_GPIO=y
+CONFIG_CMD_GPT=y
+CONFIG_CMD_MMC=y
+CONFIG_CMD_PART=y
+CONFIG_CMD_USB=y
+CONFIG_BOOTP_BOOTFILESIZE=y
+CONFIG_CMD_CACHE=y
+CONFIG_CMD_TIMER=y
+CONFIG_CMD_ENV_FLAGS=y
+CONFIG_CMD_ENV_EXISTS=y
+CONFIG_CMD_NVEDIT_INFO=y
+CONFIG_ENV_WRITEABLE_LIST=y
+CONFIG_ENV_ACCESS_IGNORE_FORCE=y
+CONFIG_ENV_IS_IN_MMC=y
+CONFIG_SYS_RELOC_GD_ENV_ADDR=y
+CONFIG_SYS_MMC_ENV_PART=2
+CONFIG_BUTTON_QCOM_PMIC=y
+CONFIG_CLK=y
+CONFIG_CLK_QCOM_APQ8016=y
+CONFIG_USB_FUNCTION_FASTBOOT=y
+CONFIG_FASTBOOT_BUF_ADDR=0x91000000
+CONFIG_FASTBOOT_FLASH=y
+CONFIG_FASTBOOT_FLASH_MMC_DEV=0
+CONFIG_MSM_GPIO=y
+CONFIG_QCOM_PMIC_GPIO=y
+CONFIG_LED=y
+CONFIG_LED_GPIO=y
+CONFIG_MMC_SDHCI=y
+CONFIG_MMC_SDHCI_MSM=y
+CONFIG_PHY=y
+CONFIG_PINCTRL=y
+CONFIG_PINCONF=y
+CONFIG_PINCTRL_QCOM_APQ8016=y
+CONFIG_DM_PMIC=y
+CONFIG_PMIC_QCOM=y
+CONFIG_MSM_SERIAL=y
+CONFIG_SPMI_MSM=y
+CONFIG_USB=y
+CONFIG_USB_EHCI_HCD=y
+CONFIG_USB_EHCI_MSM=y
+CONFIG_USB_ULPI_VIEWPORT=y
+CONFIG_USB_ULPI=y
+CONFIG_USB_HOST_ETHER=y
+CONFIG_USB_ETHER_ASIX=y
+CONFIG_USB_ETHER_ASIX88179=y
+CONFIG_USB_ETHER_MCS7830=y
+CONFIG_USB_ETHER_SMSC95XX=y
+CONFIG_PHYLIB=y
+CONFIG_USB_ETHER_LAN75XX=y
+CONFIG_USB_GADGET=y
+CONFIG_USB_GADGET_VENDOR_NUM=0x18d1
+CONFIG_USB_GADGET_PRODUCT_NUM=0xd00d
+CONFIG_CI_UDC=y
diff --git a/doc/board/index.rst b/doc/board/index.rst
index 62357c99388..bc6c27a8457 100644
--- a/doc/board/index.rst
+++ b/doc/board/index.rst
@@ -42,6 +42,7 @@ Board-specific doc
    renesas/index
    rockchip/index
    samsung/index
+   schneider/index
    sielaff/index
    siemens/index
    sifive/index
diff --git a/doc/board/schneider/hmibsc.rst b/doc/board/schneider/hmibsc.rst
new file mode 100644
index 00000000000..f09fb5af1b3
--- /dev/null
+++ b/doc/board/schneider/hmibsc.rst
@@ -0,0 +1,45 @@
+.. SPDX-License-Identifier: GPL-2.0+
+.. sectionauthor:: Sumit Garg <sumit.garg@linaro.org>
+
+HMIBSC
+======
+
+The HMIBSC is an IIoT Edge Box Core board based on the Qualcomm APQ8016E SoC.
+More information can be found on the `SE product page`_.
+
+U-Boot can be used as a replacement for Qualcomm's original Android bootloader
+(a fork of Little Kernel/LK). Like LK, it is installed directly into the ``aboot``
+partition. Note that the U-Boot port used to be loaded as an Android boot image
+through LK. This is no longer the case, now U-Boot can replace LK entirely.
+
+.. _SE product page: https://www.se.com/us/en/product/HMIBSCEA53D1L0T/iiot-edge-box-core-harmony-ipc-emmc-dc-linux-tpm/
+
+Build steps
+-----------
+
+First, setup ``CROSS_COMPILE`` for aarch64. Then, build U-Boot for ``hmibsc``::
+
+  $ export CROSS_COMPILE=<aarch64 toolchain prefix>
+  $ make hmibsc_defconfig
+  $ make
+
+This will build ``u-boot.elf`` in the configured output directory.
+
+Installation
+------------
+
+Although the HMIBSC does not have secure boot set up by default, the firmware
+still expects firmware ELF images to be "signed". The signature does not provide
+any security in this case, but it provides the firmware with some required
+metadata.
+
+To "sign" ``u-boot.elf`` you can use e.g. `qtestsign`_::
+
+  $ ./qtestsign.py aboot u-boot.elf
+
+Then install the resulting ``u-boot-test-signed.mbn`` to the ``aboot`` partition
+on your device, e.g. with ``fastboot flash aboot u-boot-test-signed.mbn``.
+
+U-Boot should be running after a reboot (``fastboot reboot``).
+
+.. _qtestsign: https://github.com/msm8916-mainline/qtestsign
diff --git a/doc/board/schneider/index.rst b/doc/board/schneider/index.rst
new file mode 100644
index 00000000000..55792ed3100
--- /dev/null
+++ b/doc/board/schneider/index.rst
@@ -0,0 +1,9 @@
+.. SPDX-License-Identifier: GPL-2.0+
+
+Schneider Electric
+==================
+
+.. toctree::
+   :maxdepth: 2
+
+   hmibsc
diff --git a/include/configs/hmibsc.h b/include/configs/hmibsc.h
new file mode 100644
index 00000000000..66dfa549ce1
--- /dev/null
+++ b/include/configs/hmibsc.h
@@ -0,0 +1,57 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Board configuration file for HMIBSC
+ *
+ * (C) Copyright 2024 Sumit Garg <sumit.garg@linaro.org>
+ */
+
+#ifndef __CONFIGS_HMIBSC_H
+#define __CONFIGS_HMIBSC_H
+
+/* PHY needs a longer aneg time */
+#define PHY_ANEG_TIMEOUT		8000
+
+#define HMIBSC_BOOTCOMMAND \
+	"setenv devtype mmc; setenv devnum 0; " \
+	"test -n \"${BOOT_ORDER}\" || setenv BOOT_ORDER \"A B\"; " \
+	"test -n \"${BOOT_A_LEFT}\" || setenv BOOT_A_LEFT 3; " \
+	"test -n \"${BOOT_B_LEFT}\" || setenv BOOT_B_LEFT 3; " \
+	"setenv raucslot; " \
+	"for BOOT_SLOT in \"${BOOT_ORDER}\"; do " \
+	"  if test \"x${raucslot}\" != \"x\"; then " \
+	"      echo \"skip remaining slots...\"; " \
+	"  elif test \"x${BOOT_SLOT}\" = \"xA\"; then " \
+	"    if test ${BOOT_A_LEFT} -gt 0; then " \
+	"      setexpr BOOT_A_LEFT ${BOOT_A_LEFT} - 1; " \
+	"      echo \"Found valid RAUC slot A\"; " \
+	"      setenv raucslot \"rauc.slot=A\"; " \
+	"      setenv raucpart A; setenv distro_bootpart 6;" \
+	"    fi; " \
+	"  elif test \"x${BOOT_SLOT}\" = \"xB\"; then " \
+	"    if test ${BOOT_B_LEFT} -gt 0; then " \
+	"      setexpr BOOT_B_LEFT ${BOOT_B_LEFT} - 1; " \
+	"      echo \"Found valid RAUC slot B\"; " \
+	"      setenv raucslot \"rauc.slot=B\"; " \
+	"      setenv raucpart B; setenv distro_bootpart 7;" \
+	"    fi; " \
+	"  fi; " \
+	"done; " \
+	"if test -n \"${raucslot}\"; then " \
+	"  setenv bootargs console=ttyMSM1 root=PARTLABEL=rootfs_${raucpart} rw rootwait ${raucslot}; " \
+	"  saveenv; " \
+	"else " \
+	"  echo \"No valid RAUC slot found. Resetting tries to 3\"; " \
+	"  setenv BOOT_A_LEFT 3; " \
+	"  setenv BOOT_B_LEFT 3; " \
+	"  saveenv; " \
+	"  reset; " \
+	"fi; " \
+	"load ${devtype} ${devnum}:${distro_bootpart} ${loadaddr} /boot/fitImage && bootm"
+
+#define CFG_EXTRA_ENV_SETTINGS \
+	"loadaddr=0x90000000\0" \
+	"bootcmd="	HMIBSC_BOOTCOMMAND		"\0"
+
+#define CFG_ENV_FLAGS_LIST_STATIC "BOOT_A_LEFT:dw,BOOT_B_LEFT:dw,BOOT_ORDER:sw"
+
+#endif
-- 
2.34.1

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

[Stephan Gerhold]

On Mon, Mar 11, 2024 at 04:40:26PM +0530, Sumit Garg wrote:
> Support for Schneider Electric HMIBSC. Features:
> - Qualcomm Snapdragon 410C SoC - APQ8016 (4xCortex A53, Adreno 306)
> - 2GiB RAM
> - 64GiB eMMC, SD slot
> - WiFi and Bluetooth
> - 2x Host, 1x Device USB port
> - HDMI
> - Discrete TPM2 chip over SPI
> 
> Features enabled in U-Boot:
> - RAUC updates
> - Environment protection
> - USB based ethernet adaptors
> 
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>

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

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

> ---
>  arch/arm/dts/apq8016-hmibsc.dts    | 496 +++++++++++++++++++++++++++++
>  board/schneider/hmibsc/MAINTAINERS |   6 +
>  configs/hmibsc_defconfig           |  87 +++++
>  doc/board/index.rst                |   1 +
>  doc/board/schneider/hmibsc.rst     |  45 +++
>  doc/board/schneider/index.rst      |   9 +
>  include/configs/hmibsc.h           |  57 ++++
>  7 files changed, 701 insertions(+)
>  create mode 100644 arch/arm/dts/apq8016-hmibsc.dts
>  create mode 100644 board/schneider/hmibsc/MAINTAINERS
>  create mode 100644 configs/hmibsc_defconfig
>  create mode 100644 doc/board/schneider/hmibsc.rst
>  create mode 100644 doc/board/schneider/index.rst
>  create mode 100644 include/configs/hmibsc.h
> 
> diff --git a/arch/arm/dts/apq8016-hmibsc.dts b/arch/arm/dts/apq8016-hmibsc.dts
> new file mode 100644
> index 00000000000..490ab5fd2fa
> --- /dev/null
> +++ b/arch/arm/dts/apq8016-hmibsc.dts
> @@ -0,0 +1,496 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2015, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2024, Linaro Ltd.
> + */
> +
> +/dts-v1/;
> +
> +#include "msm8916-pm8916.dtsi"
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/leds/common.h>
> +#include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
> +#include <dt-bindings/pinctrl/qcom,pmic-mpp.h>
> +#include <dt-bindings/sound/apq8016-lpass.h>
> +
> +/ {
> +	model = "Schneider Electric HMIBSC Board";
> +	compatible = "qcom,apq8016-hmibsc", "qcom,apq8016";

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

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

> +
> +	aliases {
> +		serial0 = &blsp_uart1;
> +		serial1 = &blsp_uart2;
> +		usid0 = &pm8916_0;
> +		i2c1 = &blsp_i2c6;
> +		i2c3 = &blsp_i2c4;
> +		i2c4 = &blsp_i2c3;
> +		spi0 = &blsp_spi5;

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

> [...]
> +&blsp_i2c6 {
> +	status = "okay";
> +	label = "I2C1";
> +
> +	rtc1: s35390a@30 {

rtc@

> +		compatible = "sii,s35390a";
> +		reg = <0x30>;
> +	};
> +
> +	eeprom1: 24c256@50 {

eeprom@

> +		compatible = "atmel,24c256";
> +		reg = <0x50>;
> +	};
> +};
> +
> +&blsp_i2c3 {

i2c3 should come before i2c6 (sorted alphabetically)

> +	status = "okay";
> +	label = "I2C4";
> +
> +	eeprom: 24c32@50 {

eeprom@

> +		compatible = "onsemi,24c32";
> +		reg = <0x50>;
> +	};
> +};
> +
> [...]
> +
> +&pm8916_0 {
> +	pon@800 {
> +		pwrkey {
> +			status = "okay";
> +			interrupts = <0x0 0x8 1 IRQ_TYPE_EDGE_BOTH>;

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

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

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

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

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

> +		};
> +	};
> +};
> +
> [...]
> +
> +&tlmm {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&gpio_rs232_high &gpio_rs232_low>;
> +
> +	sdc2_cd_default: sdc2-cd-default-state {
> +		pins = "gpio38";
> +		function = "gpio";
> +		drive-strength = <2>;
> +		bias-disable;
> +	};
> +
> +	usb_id_default: usb-id-default-state {
> +		pins = "gpio110";
> +		function = "gpio";
> +
> +		drive-strength = <8>;
> +		bias-pull-up;
> +	};
> +
> +	adv7533_int_active: adv533-int-active-state {
> +		pins = "gpio31";
> +		function = "gpio";
> +
> +		drive-strength = <16>;
> +		bias-disable;
> +	};
> +
> +	adv7533_int_suspend: adv7533-int-suspend-state {
> +		pins = "gpio31";
> +		function = "gpio";
> +
> +		drive-strength = <2>;
> +		bias-disable;
> +	};
> +
> +	adv7533_switch_active: adv7533-switch-active-state {
> +		pins = "gpio32";
> +		function = "gpio";
> +
> +		drive-strength = <16>;
> +		bias-disable;
> +	};
> +
> +	adv7533_switch_suspend: adv7533-switch-suspend-state {
> +		pins = "gpio32";
> +		function = "gpio";
> +
> +		drive-strength = <2>;
> +		bias-disable;
> +	};
> +
> +	msm_key_volp_n_default: msm-key-volp-n-default-state {
> +		pins = "gpio107";
> +		function = "gpio";
> +
> +		drive-strength = <8>;
> +		bias-pull-up;
> +	};
> +
> +	gpio_rs232_high: gpio_rs232_high {

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

> +		bootph-all;
> +		pins = "gpio99";
> +		function = "gpio";
> +
> +		drive-strength = <16>;
> +		bias-disable;
> +		output-high;
> +	};
> +
> +	gpio_rs232_low: gpio_rs232_low {

Same here.

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

> +		bootph-all;
> +		pins = "gpio100";
> +		function = "gpio";
> +
> +		drive-strength = <16>;
> +		bias-disable;
> +		output-low;
> +	};
> +};
> +
> [...]
> diff --git a/configs/hmibsc_defconfig b/configs/hmibsc_defconfig
> new file mode 100644
> index 00000000000..02b9615114b
> --- /dev/null
> +++ b/configs/hmibsc_defconfig
> @@ -0,0 +1,87 @@
> +CONFIG_ARM=y
> +CONFIG_SYS_BOARD="hmibsc"
> +CONFIG_COUNTER_FREQUENCY=19000000

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

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

Thanks,
Stephan

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

[Sumit Garg]

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

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

[Stephan Gerhold]

On Wed, Mar 13, 2024 at 12:08:58PM +0530, Sumit Garg wrote:
> 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>
> >
> [...]
> > > +     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
> 

Hm, is it a 2 bit mux selector like

	gpio99	gpio100	UART mode
	0	0	?
	0	1	?
	1	0	RS232
	1	1	?

and the others are RS422 and RS485? If yes, a comment with the table of
the function assignments would help a lot for clarity. With that,
precise naming would not be that important anymore. :-)

Thanks,
Stephan

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

[Sumit Garg]

On Wed, 13 Mar 2024 at 16:59, Stephan Gerhold <stephan@gerhold.net> wrote:
>
> On Wed, Mar 13, 2024 at 12:08:58PM +0530, Sumit Garg wrote:
> > 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>
> > >
> > [...]
> > > > +     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
> >
>
> Hm, is it a 2 bit mux selector like
>
>         gpio99  gpio100 UART mode
>         0       0       ?
>         0       1       ?
>         1       0       RS232
>         1       1       ?
>
> and the others are RS422 and RS485?

Yeah, just to complete that table:

         gpio100  gpio99 UART mode
         0             0            loopback
         0             1            RS-232
         1             0            RS-485
         1             1            RS-422

> If yes, a comment with the table of
> the function assignments would help a lot for clarity.

Sure I will add that.

> With that,
> precise naming would not be that important anymore. :-)
>

I will keep the updated naming too.

-Sumit

> Thanks,
> Stephan