Re: [PATCH v3 6/6] board: add support for Schneider HMIBSC board

2024-04-08 Thread Caleb Connolly 

Hi Sumit,

On 08/04/2024 11:13, Sumit Garg wrote:

On Fri, 5 Apr 2024 at 20:16, Stephan Gerhold  wrote:


On Fri, Apr 05, 2024 at 02:37:42PM +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 


I don't think this is a big deal but this patch would be a bit easier to
skim over if you move the (unmodified?) import of the Linux
apq8016-schneider-hmibsc.dts to a separate patch with a clear note in
the commit message

  - where it comes from (link to Linux patch), and
  - that it can be removed again with a future update of the upstream DTs
in U-Boot (once it is applied upstream at least).

You kind of have that information in the cover letter but I think it
would be good to have it in the commit message.


Although the general practice in U-Boot is to have the board DTS file
submitted along with board code, if it makes review easier via
separating the unmodified import of the board DTS file then I can do
that.


Given that in a few cycles we intend to drop the DTS you're adding here, 
having a commit we can just revert will make that much simpler.





---
  arch/arm/dts/apq8016-schneider-hmibsc.dts | 491 ++
  board/schneider/hmibsc/MAINTAINERS|   6 +
  configs/hmibsc_defconfig  |  86 
  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, 695 insertions(+)
  create mode 100644 arch/arm/dts/apq8016-schneider-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/include/configs/hmibsc.h b/include/configs/hmibsc.h
new file mode 100644
index 000..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 
+ */
+
+#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=0x9000\0" \
+ "bootcmd="  HMIBSC_BOOTCOMMAND  "\0"
+


The "text-based environment" [1] is preferred nowadays, i.e. defining
these inside board/schneider/hmibsc/hmibsc.env instead (similar to how
DB410c has its environment defined in
board/qualcomm/dragonboard410c/dragonboard410c.env). This should also
avoid all the crazy escaping to encode it as C string. :D


That makes sense, I will convert to that.



However, I suspect that right now it would attempt to load this file
from board/qualcomm/hmibsc/hmibsc.env since you do not seem to have
CONFIG_SYS_VENDOR set correctly. It looks like Caleb removed the option
to customize CONFIG_SYS_VENDOR in commit

Re: [PATCH v3 6/6] board: add support for Schneider HMIBSC board

2024-04-08 Thread Sumit Garg 
On Fri, 5 Apr 2024 at 20:16, Stephan Gerhold  wrote:
>
> On Fri, Apr 05, 2024 at 02:37:42PM +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 
>
> I don't think this is a big deal but this patch would be a bit easier to
> skim over if you move the (unmodified?) import of the Linux
> apq8016-schneider-hmibsc.dts to a separate patch with a clear note in
> the commit message
>
>  - where it comes from (link to Linux patch), and
>  - that it can be removed again with a future update of the upstream DTs
>in U-Boot (once it is applied upstream at least).
>
> You kind of have that information in the cover letter but I think it
> would be good to have it in the commit message.

Although the general practice in U-Boot is to have the board DTS file
submitted along with board code, if it makes review easier via
separating the unmodified import of the board DTS file then I can do
that.

>
> > ---
> >  arch/arm/dts/apq8016-schneider-hmibsc.dts | 491 ++
> >  board/schneider/hmibsc/MAINTAINERS|   6 +
> >  configs/hmibsc_defconfig  |  86 
> >  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, 695 insertions(+)
> >  create mode 100644 arch/arm/dts/apq8016-schneider-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/include/configs/hmibsc.h b/include/configs/hmibsc.h
> > new file mode 100644
> > index 000..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 
> > + */
> > +
> > +#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=0x9000\0" \
> > + "bootcmd="  HMIBSC_BOOTCOMMAND  "\0"
> > +
>
> The "text-based environment" [1] is preferred nowadays, i.e. defining
> these inside board/schneider/hmibsc/hmibsc.env instead (similar to how
> DB410c has its environment defined in
> board/qualcomm/dragonboard410c/dragonboard410c.env). This should also
> avoid all the crazy escaping to encode it as C string. :D

That makes sense, I will convert to that.

>
> However, I suspect that right now it would attempt to load

Re: [PATCH v3 6/6] board: add support for Schneider HMIBSC board

2024-04-05 Thread Stephan Gerhold 
On Fri, Apr 05, 2024 at 02:37:42PM +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 

I don't think this is a big deal but this patch would be a bit easier to
skim over if you move the (unmodified?) import of the Linux
apq8016-schneider-hmibsc.dts to a separate patch with a clear note in
the commit message

 - where it comes from (link to Linux patch), and
 - that it can be removed again with a future update of the upstream DTs
   in U-Boot (once it is applied upstream at least).

You kind of have that information in the cover letter but I think it
would be good to have it in the commit message.

> ---
>  arch/arm/dts/apq8016-schneider-hmibsc.dts | 491 ++
>  board/schneider/hmibsc/MAINTAINERS|   6 +
>  configs/hmibsc_defconfig  |  86 
>  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, 695 insertions(+)
>  create mode 100644 arch/arm/dts/apq8016-schneider-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/include/configs/hmibsc.h b/include/configs/hmibsc.h
> new file mode 100644
> index 000..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 
> + */
> +
> +#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=0x9000\0" \
> + "bootcmd="  HMIBSC_BOOTCOMMAND  "\0"
> +

The "text-based environment" [1] is preferred nowadays, i.e. defining
these inside board/schneider/hmibsc/hmibsc.env instead (similar to how
DB410c has its environment defined in
board/qualcomm/dragonboard410c/dragonboard410c.env). This should also
avoid all the crazy escaping to encode it as C string. :D

However, I suspect that right now it would attempt to load this file
from board/qualcomm/hmibsc/hmibsc.env since you do not seem to have
CONFIG_SYS_VENDOR set correctly. It looks like Caleb removed the option
to customize CONFIG_SYS_VENDOR in commit 059d526af312 ("mach-snapdragon:
generalise board support"). It might be easiest to add a prompt in
arch/arm/mach-snapdragon/Kconfig to allow changing it (similar to
CONFIG_SYS_BOARD).

Thanks,
Stephan

[1]: 
https://docs.u-boot.org/en/latest/usage/environment.html#text-based-environment