Re: [PATCH v5 4/6] configs: am62x_evm_*: Enable USB and DFU support
Jon Humphreys writes: > Martyn Welch writes: > >> On Thu, 2024-05-23 at 15:08 -0500, Jon Humphreys wrote: >>> Martyn Welch writes: >>> >>> > From: Sjoerd Simons >>> > >>> > Provide config fragments to enable USB host as well as USB gadget >>> > and DFU >>> > support for a53 and r5. This relevant fragment is included into the >>> > am62x EVM a53 defconfig. For the r5, due to the smaller available >>> > size, >>> > the config fragment also disables support for persistent storage to >>> > free >>> > up space for USB support. This fragment needs to be included is DFU >>> > booting is desired. >>> > >>> > The CONFIG_DFU_SF option is placed in the defconfig rather than the >>> > fragment as this is known not to be supported on all boards that >>> > can >>> > support DFU. >>> > >>> > Signed-off-by: Sjoerd Simons >>> > Signed-off-by: Martyn Welch >>> > --- >>> > Changes in v5: >>> > - Switch to config fragment for a53 most DFU configuration >>> > >>> > Changes in v4: >>> > - Move R5 dfu config to a config fragment rather then a full >>> > defconfig >>> > - Don't enable XHCI for the R5 SPL, unneeded >>> > >>> > Changes in v3: >>> > - Run savedefconfig to adjust to more recent u-boot >>> > >>> > Changes in v2: >>> > - Create a seperate defconfig for R5 >>> > >>> > >>> > >>> > configs/am62x_a53_usbdfu.config | 30 >>> > ++ >>> > configs/am62x_evm_a53_defconfig | 2 ++ >>> > configs/am62x_r5_usbdfu.config | 28 >>> > 3 files changed, 60 insertions(+) >>> > create mode 100644 configs/am62x_a53_usbdfu.config >>> > create mode 100644 configs/am62x_r5_usbdfu.config >>> > >>> > diff --git a/configs/am62x_a53_usbdfu.config >>> > b/configs/am62x_a53_usbdfu.config >>> > new file mode 100644 >>> > index 00..3a19cf2328 >>> > --- /dev/null >>> > +++ b/configs/am62x_a53_usbdfu.config >>> > @@ -0,0 +1,29 @@ >>> > +CONFIG_SYS_MALLOC_LEN=0x200 >>> > +CONFIG_SPL_ENV_SUPPORT=y >>> > +CONFIG_SPL_RAM_SUPPORT=y >>> > +CONFIG_SPL_RAM_DEVICE=y >>> > +CONFIG_SPL_USB_GADGET=y >>> > +CONFIG_SPL_DFU=y >>> > +CONFIG_CMD_DFU=y >>> > +CONFIG_CMD_USB=y >>> > +CONFIG_SYSCON=y >>> > +CONFIG_SPL_SYSCON=y >>> > +CONFIG_DFU_MMC=y >>> > +CONFIG_DFU_RAM=y >>> > +CONFIG_SYS_DFU_DATA_BUF_SIZE=0x5000 >>> > +CONFIG_SYS_DFU_MAX_FILE_SIZE=0x80 >>> > +CONFIG_USB=y >>> > +CONFIG_DM_USB_GADGET=y >>> > +CONFIG_SPL_DM_USB_GADGET=y >>> > +CONFIG_USB_XHCI_HCD=y >>> > +CONFIG_USB_XHCI_DWC3=y >>> > +CONFIG_USB_DWC3=y >>> > +CONFIG_USB_DWC3_GENERIC=y >>> > +CONFIG_SPL_USB_DWC3_GENERIC=y >>> > +CONFIG_SPL_USB_DWC3_AM62=y >>> > +CONFIG_USB_DWC3_AM62=y >>> > +CONFIG_USB_GADGET=y >>> > +CONFIG_USB_GADGET_MANUFACTURER="Texas Instruments" >>> > +CONFIG_USB_GADGET_VENDOR_NUM=0x0451 >>> > +CONFIG_USB_GADGET_PRODUCT_NUM=0x6165 >>> > +CONFIG_USB_GADGET_DOWNLOAD=y >>> > diff --git a/configs/am62x_evm_a53_defconfig >>> > b/configs/am62x_evm_a53_defconfig >>> > index 6c708dcb05..16294a6a79 100644 >>> > --- a/configs/am62x_evm_a53_defconfig >>> > +++ b/configs/am62x_evm_a53_defconfig >>> > @@ -68,6 +68,7 @@ CONFIG_SPL_OF_TRANSLATE=y >>> > CONFIG_CLK=y >>> > CONFIG_SPL_CLK=y >>> > CONFIG_CLK_TI_SCI=y >>> > +CONFIG_DFU_SF=y >>> > CONFIG_DMA_CHANNELS=y >>> > CONFIG_TI_K3_NAVSS_UDMA=y >>> > CONFIG_TI_SCI_PROTOCOL=y >>> > @@ -111,3 +112,5 @@ CONFIG_SPL_SYSRESET=y >>> > CONFIG_SYSRESET_TI_SCI=y >>> > CONFIG_FS_FAT_MAX_CLUSTSIZE=16384 >>> > CONFIG_EFI_SET_TIME=y >>> > + >>> > +#include >>> > diff --git a/configs/am62x_r5_usbdfu.config >>> > b/configs/am62x_r5_usbdfu.config >>> > new file mode 100644 >>> > index 00..772bb2ab93 >>> > --- /dev/null >>> > +++ b/configs/am62x_r5_usbdfu.config >>
Re: [PATCH v5 4/6] configs: am62x_evm_*: Enable USB and DFU support
Martyn Welch writes: > On Thu, 2024-05-23 at 15:08 -0500, Jon Humphreys wrote: >> Martyn Welch writes: >> >> > From: Sjoerd Simons >> > >> > Provide config fragments to enable USB host as well as USB gadget >> > and DFU >> > support for a53 and r5. This relevant fragment is included into the >> > am62x EVM a53 defconfig. For the r5, due to the smaller available >> > size, >> > the config fragment also disables support for persistent storage to >> > free >> > up space for USB support. This fragment needs to be included is DFU >> > booting is desired. >> > >> > The CONFIG_DFU_SF option is placed in the defconfig rather than the >> > fragment as this is known not to be supported on all boards that >> > can >> > support DFU. >> > >> > Signed-off-by: Sjoerd Simons >> > Signed-off-by: Martyn Welch >> > --- >> > Changes in v5: >> > - Switch to config fragment for a53 most DFU configuration >> > >> > Changes in v4: >> > - Move R5 dfu config to a config fragment rather then a full >> > defconfig >> > - Don't enable XHCI for the R5 SPL, unneeded >> > >> > Changes in v3: >> > - Run savedefconfig to adjust to more recent u-boot >> > >> > Changes in v2: >> > - Create a seperate defconfig for R5 >> > >> > >> > >> > configs/am62x_a53_usbdfu.config | 30 >> > ++ >> > configs/am62x_evm_a53_defconfig | 2 ++ >> > configs/am62x_r5_usbdfu.config | 28 >> > 3 files changed, 60 insertions(+) >> > create mode 100644 configs/am62x_a53_usbdfu.config >> > create mode 100644 configs/am62x_r5_usbdfu.config >> > >> > diff --git a/configs/am62x_a53_usbdfu.config >> > b/configs/am62x_a53_usbdfu.config >> > new file mode 100644 >> > index 00..3a19cf2328 >> > --- /dev/null >> > +++ b/configs/am62x_a53_usbdfu.config >> > @@ -0,0 +1,29 @@ >> > +CONFIG_SYS_MALLOC_LEN=0x200 >> > +CONFIG_SPL_ENV_SUPPORT=y >> > +CONFIG_SPL_RAM_SUPPORT=y >> > +CONFIG_SPL_RAM_DEVICE=y >> > +CONFIG_SPL_USB_GADGET=y >> > +CONFIG_SPL_DFU=y >> > +CONFIG_CMD_DFU=y >> > +CONFIG_CMD_USB=y >> > +CONFIG_SYSCON=y >> > +CONFIG_SPL_SYSCON=y >> > +CONFIG_DFU_MMC=y >> > +CONFIG_DFU_RAM=y >> > +CONFIG_SYS_DFU_DATA_BUF_SIZE=0x5000 >> > +CONFIG_SYS_DFU_MAX_FILE_SIZE=0x80 >> > +CONFIG_USB=y >> > +CONFIG_DM_USB_GADGET=y >> > +CONFIG_SPL_DM_USB_GADGET=y >> > +CONFIG_USB_XHCI_HCD=y >> > +CONFIG_USB_XHCI_DWC3=y >> > +CONFIG_USB_DWC3=y >> > +CONFIG_USB_DWC3_GENERIC=y >> > +CONFIG_SPL_USB_DWC3_GENERIC=y >> > +CONFIG_SPL_USB_DWC3_AM62=y >> > +CONFIG_USB_DWC3_AM62=y >> > +CONFIG_USB_GADGET=y >> > +CONFIG_USB_GADGET_MANUFACTURER="Texas Instruments" >> > +CONFIG_USB_GADGET_VENDOR_NUM=0x0451 >> > +CONFIG_USB_GADGET_PRODUCT_NUM=0x6165 >> > +CONFIG_USB_GADGET_DOWNLOAD=y >> > diff --git a/configs/am62x_evm_a53_defconfig >> > b/configs/am62x_evm_a53_defconfig >> > index 6c708dcb05..16294a6a79 100644 >> > --- a/configs/am62x_evm_a53_defconfig >> > +++ b/configs/am62x_evm_a53_defconfig >> > @@ -68,6 +68,7 @@ CONFIG_SPL_OF_TRANSLATE=y >> > CONFIG_CLK=y >> > CONFIG_SPL_CLK=y >> > CONFIG_CLK_TI_SCI=y >> > +CONFIG_DFU_SF=y >> > CONFIG_DMA_CHANNELS=y >> > CONFIG_TI_K3_NAVSS_UDMA=y >> > CONFIG_TI_SCI_PROTOCOL=y >> > @@ -111,3 +112,5 @@ CONFIG_SPL_SYSRESET=y >> > CONFIG_SYSRESET_TI_SCI=y >> > CONFIG_FS_FAT_MAX_CLUSTSIZE=16384 >> > CONFIG_EFI_SET_TIME=y >> > + >> > +#include >> > diff --git a/configs/am62x_r5_usbdfu.config >> > b/configs/am62x_r5_usbdfu.config >> > new file mode 100644 >> > index 00..772bb2ab93 >> > --- /dev/null >> > +++ b/configs/am62x_r5_usbdfu.config >> > @@ -0,0 +1,28 @@ >> > +CONFIG_SPL_ENV_SUPPORT=y >> > +CONFIG_SYSCON=y >> > +CONFIG_SPL_SYSCON=y >> > +CONFIG_SYS_DFU_DATA_BUF_SIZE=0x5000 >> > +CONFIG_MISC=y >> > +CONFIG_USB=y >> > +CONFIG_DM_USB_GADGET=y >> > +CONFIG_SPL_DM_USB_GADGET=y >> > +CONFIG_USB_DWC3=y >> > +CONFIG_USB_DWC3_GENERIC=y >> > +CONFIG_SPL_USB_DWC3_GENERIC=y >> > +CONFIG_SPL_USB_
Re: [PATCH v5 4/6] configs: am62x_evm_*: Enable USB and DFU support
Martyn Welch writes: > On Thu, 2024-05-23 at 15:08 -0500, Jon Humphreys wrote: >> Martyn Welch writes: >> >> > From: Sjoerd Simons >> > >> > Provide config fragments to enable USB host as well as USB gadget >> > and DFU >> > support for a53 and r5. This relevant fragment is included into the >> > am62x EVM a53 defconfig. For the r5, due to the smaller available >> > size, >> > the config fragment also disables support for persistent storage to >> > free >> > up space for USB support. This fragment needs to be included is DFU >> > booting is desired. >> > >> > The CONFIG_DFU_SF option is placed in the defconfig rather than the >> > fragment as this is known not to be supported on all boards that >> > can >> > support DFU. >> > >> > Signed-off-by: Sjoerd Simons >> > Signed-off-by: Martyn Welch >> > --- >> > Changes in v5: >> > - Switch to config fragment for a53 most DFU configuration >> > >> > Changes in v4: >> > - Move R5 dfu config to a config fragment rather then a full >> > defconfig >> > - Don't enable XHCI for the R5 SPL, unneeded >> > >> > Changes in v3: >> > - Run savedefconfig to adjust to more recent u-boot >> > >> > Changes in v2: >> > - Create a seperate defconfig for R5 >> > >> > >> > >> > configs/am62x_a53_usbdfu.config | 30 >> > ++ >> > configs/am62x_evm_a53_defconfig | 2 ++ >> > configs/am62x_r5_usbdfu.config | 28 >> > 3 files changed, 60 insertions(+) >> > create mode 100644 configs/am62x_a53_usbdfu.config >> > create mode 100644 configs/am62x_r5_usbdfu.config >> > >> > diff --git a/configs/am62x_a53_usbdfu.config >> > b/configs/am62x_a53_usbdfu.config >> > new file mode 100644 >> > index 00..3a19cf2328 >> > --- /dev/null >> > +++ b/configs/am62x_a53_usbdfu.config >> > @@ -0,0 +1,29 @@ >> > +CONFIG_SYS_MALLOC_LEN=0x200 >> > +CONFIG_SPL_ENV_SUPPORT=y >> > +CONFIG_SPL_RAM_SUPPORT=y >> > +CONFIG_SPL_RAM_DEVICE=y >> > +CONFIG_SPL_USB_GADGET=y >> > +CONFIG_SPL_DFU=y >> > +CONFIG_CMD_DFU=y >> > +CONFIG_CMD_USB=y >> > +CONFIG_SYSCON=y >> > +CONFIG_SPL_SYSCON=y >> > +CONFIG_DFU_MMC=y >> > +CONFIG_DFU_RAM=y >> > +CONFIG_SYS_DFU_DATA_BUF_SIZE=0x5000 >> > +CONFIG_SYS_DFU_MAX_FILE_SIZE=0x80 >> > +CONFIG_USB=y >> > +CONFIG_DM_USB_GADGET=y >> > +CONFIG_SPL_DM_USB_GADGET=y >> > +CONFIG_USB_XHCI_HCD=y >> > +CONFIG_USB_XHCI_DWC3=y >> > +CONFIG_USB_DWC3=y >> > +CONFIG_USB_DWC3_GENERIC=y >> > +CONFIG_SPL_USB_DWC3_GENERIC=y >> > +CONFIG_SPL_USB_DWC3_AM62=y >> > +CONFIG_USB_DWC3_AM62=y >> > +CONFIG_USB_GADGET=y >> > +CONFIG_USB_GADGET_MANUFACTURER="Texas Instruments" >> > +CONFIG_USB_GADGET_VENDOR_NUM=0x0451 >> > +CONFIG_USB_GADGET_PRODUCT_NUM=0x6165 >> > +CONFIG_USB_GADGET_DOWNLOAD=y >> > diff --git a/configs/am62x_evm_a53_defconfig >> > b/configs/am62x_evm_a53_defconfig >> > index 6c708dcb05..16294a6a79 100644 >> > --- a/configs/am62x_evm_a53_defconfig >> > +++ b/configs/am62x_evm_a53_defconfig >> > @@ -68,6 +68,7 @@ CONFIG_SPL_OF_TRANSLATE=y >> > CONFIG_CLK=y >> > CONFIG_SPL_CLK=y >> > CONFIG_CLK_TI_SCI=y >> > +CONFIG_DFU_SF=y >> > CONFIG_DMA_CHANNELS=y >> > CONFIG_TI_K3_NAVSS_UDMA=y >> > CONFIG_TI_SCI_PROTOCOL=y >> > @@ -111,3 +112,5 @@ CONFIG_SPL_SYSRESET=y >> > CONFIG_SYSRESET_TI_SCI=y >> > CONFIG_FS_FAT_MAX_CLUSTSIZE=16384 >> > CONFIG_EFI_SET_TIME=y >> > + >> > +#include >> > diff --git a/configs/am62x_r5_usbdfu.config >> > b/configs/am62x_r5_usbdfu.config >> > new file mode 100644 >> > index 00..772bb2ab93 >> > --- /dev/null >> > +++ b/configs/am62x_r5_usbdfu.config >> > @@ -0,0 +1,28 @@ >> > +CONFIG_SPL_ENV_SUPPORT=y >> > +CONFIG_SYSCON=y >> > +CONFIG_SPL_SYSCON=y >> > +CONFIG_SYS_DFU_DATA_BUF_SIZE=0x5000 >> > +CONFIG_MISC=y >> > +CONFIG_USB=y >> > +CONFIG_DM_USB_GADGET=y >> > +CONFIG_SPL_DM_USB_GADGET=y >> > +CONFIG_USB_DWC3=y >> > +CONFIG_USB_DWC3_GENERIC=y >> > +CONFIG_SPL_USB_DWC3_GENERIC=y >> > +CONFIG_SPL_USB_
Re: [PATCH v4 10/14] net-lwip: add wget command
Jerome Forissier writes:
> On 6/24/24 08:21, Jon Humphreys wrote:
>> Jerome Forissier writes:
>>
>>> Add support for the wget command with NET_LWIP.
>>>
>>> Based on code initially developed by Maxim U.
>>>
>>> Signed-off-by: Jerome Forissier
>>> Co-developed-by: Maxim Uvarov
>>> Cc: Maxim Uvarov
>>> Signed-off-by: Jerome Forissier
>>> ---
>>> cmd/Kconfig| 7 ++
>>> cmd/net-lwip.c | 8 ++
>>> include/net-lwip.h | 18 +++
>>> net-lwip/Makefile | 1 +
>>> net-lwip/wget.c| 269 +
>>> 5 files changed, 303 insertions(+)
>>> create mode 100644 net-lwip/wget.c
>>>
>>> diff --git a/cmd/Kconfig b/cmd/Kconfig
>>> index 6ef0b52cd34..d9a86540be6 100644
>>> --- a/cmd/Kconfig
>>> +++ b/cmd/Kconfig
>>> @@ -2117,6 +2117,13 @@ config CMD_TFTPBOOT
>>> help
>>> tftpboot - load file via network using TFTP protocol
>>>
>>> +config CMD_WGET
>>> + bool "wget"
>>> + select PROT_TCP_LWIP
>>> + help
>>> + wget is a simple command to download kernel, or other files,
>>> + from a http server over TCP.
>>> +
>>> endif
>>>
>>> endif
>>> diff --git a/cmd/net-lwip.c b/cmd/net-lwip.c
>>> index c021da6a674..42f8bd6b259 100644
>>> --- a/cmd/net-lwip.c
>>> +++ b/cmd/net-lwip.c
>>> @@ -35,3 +35,11 @@ U_BOOT_CMD(
>>> "hostname [envvar]"
>>> );
>>> #endif
>>> +
>>> +#if defined(CONFIG_CMD_WGET)
>>> +U_BOOT_CMD(
>>> + wget, 3, 1, do_wget,
>>> + "boot image via network using HTTP protocol",
>>> + "[loadAddress] URL"
>>> +);
>>> +#endif
>>> diff --git a/include/net-lwip.h b/include/net-lwip.h
>>> index 4d41b0208a3..ddf25e61417 100644
>>> --- a/include/net-lwip.h
>>> +++ b/include/net-lwip.h
>>> @@ -11,8 +11,26 @@ struct netif *net_lwip_new_netif_noip(void);
>>> void net_lwip_remove_netif(struct netif *netif);
>>> struct netif *net_lwip_get_netif(void);
>>>
>>> +/**
>>> + * wget_with_dns() - runs dns host IP address resulution before wget
>>> + *
>>> + * @dst_addr: destination address to download the file
>>> + * @uri: uri string of target file of wget
>>> + * Return: downloaded file size, negative if failed
>>> + */
>>> +
>>> +int wget_with_dns(ulong dst_addr, char *uri);
>>> +/**
>>> + * wget_validate_uri() - varidate the uri
>>> + *
>>> + * @uri: uri string of target file of wget
>>> + * Return: true if uri is valid, false if uri is invalid
>>> + */
>>> +bool wget_validate_uri(char *uri);
>>> +
>>> int do_dhcp(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
>>> int do_dns(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
>>> int do_ping(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
>>> +int do_wget(struct cmd_tbl *cmdtp, int flag, int argc, char * const
>>> argv[]);
>>>
>>> #endif /* __NET_LWIP_H__ */
>>> diff --git a/net-lwip/Makefile b/net-lwip/Makefile
>>> index aa247859483..bc011bb0e32 100644
>>> --- a/net-lwip/Makefile
>>> +++ b/net-lwip/Makefile
>>> @@ -10,6 +10,7 @@ obj-$(CONFIG_CMD_DHCP) += dhcp.o
>>> obj-$(CONFIG_CMD_DNS) += dns.o
>>> obj-$(CONFIG_CMD_PING) += ping.o
>>> obj-$(CONFIG_CMD_TFTPBOOT) += tftp.o
>>> +obj-$(CONFIG_CMD_WGET) += wget.o
>>>
>>> # Disable this warning as it is triggered by:
>>> # sprintf(buf, index ? "foo%d" : "foo", index)
>>> diff --git a/net-lwip/wget.c b/net-lwip/wget.c
>>> new file mode 100644
>>> index 000..069299bd469
>>> --- /dev/null
>>> +++ b/net-lwip/wget.c
>>> @@ -0,0 +1,269 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/* Copyright (C) 2024 Linaro Ltd. */
>>> +
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +
>>> +#define SERVER_NAME_SIZE 200
>>> +#define HTTP_PORT_DEFAULT 80
>>> +#define PROGRESS_PRINT_STEP_BYTES (100 * 1024)
>>> +
>>> +enum done_state {
>>> +NOT_DONE = 0
Re: [PATCH v4 10/14] net-lwip: add wget command
Subject: [PATCH] net-lwip: Add message if not using http:// for wget
U-Boot's wget only supports http://, so give the user a clue if they don't
use it.
Signed-off-by: Jonathan Humphreys
---
net-lwip/wget.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net-lwip/wget.c b/net-lwip/wget.c
index 63b19b99112..6b9c953ef51 100644
--- a/net-lwip/wget.c
+++ b/net-lwip/wget.c
@@ -35,8 +35,10 @@ static int parse_url(char *url, char *host, u16 *port, char
**path)
long lport;
p = strstr(url, "http://";);
- if (!p)
+ if (!p) {
+ log_err("only http:// is supported\n");
return -EINVAL;
+ }
p += strlen("http://";);
--
2.34.1
Re: [PATCH v4 10/14] net-lwip: add wget command
Subject: [PATCH] net-lwip: fixes off-by-one array access error with wget
When wget parses the url and extracts the host, it is off by one on the
index to terminate the character array.
Signed-off-by: Jonathan Humphreys
---
net-lwip/wget.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net-lwip/wget.c b/net-lwip/wget.c
index 069299bd469..63b19b99112 100644
--- a/net-lwip/wget.c
+++ b/net-lwip/wget.c
@@ -51,7 +51,7 @@ static int parse_url(char *url, char *host, u16 *port, char
**path)
return -EINVAL;
memcpy(host, p, pp - p);
- host[pp - p + 1] = '\0';
+ host[pp - p] = '\0';
if (*pp == ':') {
/* Parse port number */
--
2.34.1
Re: [PATCH v4 10/14] net-lwip: add wget command
Jerome Forissier writes:
> Add support for the wget command with NET_LWIP.
>
> Based on code initially developed by Maxim U.
>
> Signed-off-by: Jerome Forissier
> Co-developed-by: Maxim Uvarov
> Cc: Maxim Uvarov
> Signed-off-by: Jerome Forissier
> ---
> cmd/Kconfig| 7 ++
> cmd/net-lwip.c | 8 ++
> include/net-lwip.h | 18 +++
> net-lwip/Makefile | 1 +
> net-lwip/wget.c| 269 +
> 5 files changed, 303 insertions(+)
> create mode 100644 net-lwip/wget.c
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 6ef0b52cd34..d9a86540be6 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -2117,6 +2117,13 @@ config CMD_TFTPBOOT
> help
> tftpboot - load file via network using TFTP protocol
>
> +config CMD_WGET
> + bool "wget"
> + select PROT_TCP_LWIP
> + help
> + wget is a simple command to download kernel, or other files,
> + from a http server over TCP.
> +
> endif
>
> endif
> diff --git a/cmd/net-lwip.c b/cmd/net-lwip.c
> index c021da6a674..42f8bd6b259 100644
> --- a/cmd/net-lwip.c
> +++ b/cmd/net-lwip.c
> @@ -35,3 +35,11 @@ U_BOOT_CMD(
> "hostname [envvar]"
> );
> #endif
> +
> +#if defined(CONFIG_CMD_WGET)
> +U_BOOT_CMD(
> + wget, 3, 1, do_wget,
> + "boot image via network using HTTP protocol",
> + "[loadAddress] URL"
> +);
> +#endif
> diff --git a/include/net-lwip.h b/include/net-lwip.h
> index 4d41b0208a3..ddf25e61417 100644
> --- a/include/net-lwip.h
> +++ b/include/net-lwip.h
> @@ -11,8 +11,26 @@ struct netif *net_lwip_new_netif_noip(void);
> void net_lwip_remove_netif(struct netif *netif);
> struct netif *net_lwip_get_netif(void);
>
> +/**
> + * wget_with_dns() - runs dns host IP address resulution before wget
> + *
> + * @dst_addr:destination address to download the file
> + * @uri: uri string of target file of wget
> + * Return: downloaded file size, negative if failed
> + */
> +
> +int wget_with_dns(ulong dst_addr, char *uri);
> +/**
> + * wget_validate_uri() - varidate the uri
> + *
> + * @uri: uri string of target file of wget
> + * Return: true if uri is valid, false if uri is invalid
> + */
> +bool wget_validate_uri(char *uri);
> +
> int do_dhcp(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
> int do_dns(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
> int do_ping(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
> +int do_wget(struct cmd_tbl *cmdtp, int flag, int argc, char * const argv[]);
>
> #endif /* __NET_LWIP_H__ */
> diff --git a/net-lwip/Makefile b/net-lwip/Makefile
> index aa247859483..bc011bb0e32 100644
> --- a/net-lwip/Makefile
> +++ b/net-lwip/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_CMD_DHCP) += dhcp.o
> obj-$(CONFIG_CMD_DNS) += dns.o
> obj-$(CONFIG_CMD_PING) += ping.o
> obj-$(CONFIG_CMD_TFTPBOOT) += tftp.o
> +obj-$(CONFIG_CMD_WGET) += wget.o
>
> # Disable this warning as it is triggered by:
> # sprintf(buf, index ? "foo%d" : "foo", index)
> diff --git a/net-lwip/wget.c b/net-lwip/wget.c
> new file mode 100644
> index 000..069299bd469
> --- /dev/null
> +++ b/net-lwip/wget.c
> @@ -0,0 +1,269 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/* Copyright (C) 2024 Linaro Ltd. */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define SERVER_NAME_SIZE 200
> +#define HTTP_PORT_DEFAULT 80
> +#define PROGRESS_PRINT_STEP_BYTES (100 * 1024)
> +
> +enum done_state {
> +NOT_DONE = 0,
> +SUCCESS = 1,
> +FAILURE = 2
> +};
> +
> +struct wget_ctx {
> + ulong daddr;
> + ulong saved_daddr;
> + ulong size;
> + ulong prevsize;
> + ulong start_time;
> + enum done_state done;
> +};
> +
> +static int parse_url(char *url, char *host, u16 *port, char **path)
> +{
> + char *p, *pp;
> + long lport;
> +
> + p = strstr(url, "http://";);
> + if (!p)
> + return -EINVAL;
> +
> + p += strlen("http://";);
> +
> + /* Parse hostname */
> + pp = strchr(p, ':');
> + if (!pp)
> + pp = strchr(p, '/');
> + if (!pp)
> + return -EINVAL;
> +
> + if (p + SERVER_NAME_SIZE <= pp)
> + return -EINVAL;
> +
> + memcpy(host, p, pp - p);
> + host[pp - p + 1] = '\0';
> +
> + if (*pp == ':') {
> + /* Parse port number */
> + p = pp + 1;
> + lport = simple_strtol(p, &pp, 10);
> + if (pp && *pp != '/')
> + return -EINVAL;
> + if (lport > 65535)
> + return -EINVAL;
> + *port = (u16)lport;
> + } else {
> + *port = HTTP_PORT_DEFAULT;
> + }
> + if (*pp != '/')
> + return -EINVAL;
> + *path = pp;
> +
> + return 0;
> +}
> +
> +static err_t httpc_recv_cb(void *arg, struct altcp_pcb *pcb, struct pbuf
> *pbuf,
> +err_t err)
> +
Re: [PATCH] efi_loader: adjust config options for capsule updates
Ilias Apalodimas writes: > Hi Jon, > > On Tue, 18 Jun 2024 at 19:49, Jon Humphreys wrote: >> >> Ilias Apalodimas writes: >> >> > EFI_IGNORE_OSINDICATIONS is used to ignore OsIndications if setvariable >> > at runtime is not supported and allow the platform to perform capsule >> > updates on disk. With the recent changes boards can conditionally enable >> > setvariable at runtime using EFI_RT_VOLATILE_STORE. >> > >> > So let's make the options depend on each other and clarify their >> > functionality. When EFI_RT_VOLATILE_STORE, setvariable at runtime is >> > supported and EFI_IGNORE_OSINDICATIONS, which also breaks the EFI spec, is >> > not needed anymore. >> >> Hi Ilias, >> >> Is there a corresponding effort to update fwupdmgr to set OSIndications >> (and set it correctly so the change persists a reboot)? >> > > We are not trying to fix anything atm, we probably will in the future. > >> Otherwise, fwupdmgr provided capsules will now get ignored for boards that >> enable setvariable at runtime. > > That goes beyond fwupd, EFI variable changes will be ignored in > general unless you sync the file manually -- e.g. dd > if=/sys/firmware/efi/efivars/VarToFile-b2ac5fc9-92b7-4acd-aeac-11e818c3130c > of=/boot/efi/ubootefi.var skip=4 bs=1 > after every variable change. Unless you do that, enabling > EFI_RT_VOLATILE_STORE makes little sense (and that's why it's not > enabled by default). > So, I prefer making EFI_IGNORE_OSINDICATIONS, which was a hack, > mutually exclusive and start going in the right direction. > I agree with the principle. My concern is that we want to encourage people to move to setvariable at runtime, but until LVFS is updated, people who care about LVFS will be deincentivized. Jon > Regards > /Ilias >> >> Jon >> >> > >> > Signed-off-by: Ilias Apalodimas >> > --- >> > lib/efi_loader/Kconfig | 2 ++ >> > 1 file changed, 2 insertions(+) >> > >> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig >> > index 430bb7f0f7dc..c84064de1366 100644 >> > --- a/lib/efi_loader/Kconfig >> > +++ b/lib/efi_loader/Kconfig >> > @@ -220,6 +220,8 @@ config EFI_CAPSULE_ON_DISK >> > config EFI_IGNORE_OSINDICATIONS >> > bool "Ignore OsIndications for CapsuleUpdate on-disk" >> > depends on EFI_CAPSULE_ON_DISK >> > + depends on !EFI_RT_VOLATILE_STORE >> > + default y >> > help >> > There are boards where U-Boot does not support SetVariable at >> > runtime. >> > Select this option if you want to use the capsule-on-disk feature >> > -- >> > 2.45.2
Re: [PATCH] efi_loader: adjust config options for capsule updates
Ilias Apalodimas writes: > EFI_IGNORE_OSINDICATIONS is used to ignore OsIndications if setvariable > at runtime is not supported and allow the platform to perform capsule > updates on disk. With the recent changes boards can conditionally enable > setvariable at runtime using EFI_RT_VOLATILE_STORE. > > So let's make the options depend on each other and clarify their > functionality. When EFI_RT_VOLATILE_STORE, setvariable at runtime is > supported and EFI_IGNORE_OSINDICATIONS, which also breaks the EFI spec, is > not needed anymore. Hi Ilias, Is there a corresponding effort to update fwupdmgr to set OSIndications (and set it correctly so the change persists a reboot)? Otherwise, fwupdmgr provided capsules will now get ignored for boards that enable setvariable at runtime. Jon > > Signed-off-by: Ilias Apalodimas > --- > lib/efi_loader/Kconfig | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig > index 430bb7f0f7dc..c84064de1366 100644 > --- a/lib/efi_loader/Kconfig > +++ b/lib/efi_loader/Kconfig > @@ -220,6 +220,8 @@ config EFI_CAPSULE_ON_DISK > config EFI_IGNORE_OSINDICATIONS > bool "Ignore OsIndications for CapsuleUpdate on-disk" > depends on EFI_CAPSULE_ON_DISK > + depends on !EFI_RT_VOLATILE_STORE > + default y > help > There are boards where U-Boot does not support SetVariable at runtime. > Select this option if you want to use the capsule-on-disk feature > -- > 2.45.2
Re: [PATCH v2 0/2] scripts/Makefile.lib: EFI: Use capsule CRT instead of ESL
Ilias Apalodimas writes: > Hi Jonathan > > On Thu, 13 Jun 2024 at 23:28, Jonathan Humphreys wrote: >> >> Use the capsule's public key certificate rather than a prebuilt ESL >> generated from the certificate. The ESL is now generated as part of the >> build. > > Is there a reason to do this? I understand that the .crt extension > might be well known while the .esl is not, but OTOH the system you > build on after this change *needs* to have cert-to-efi-sig-list > installed > Hi Ilias, In general, I am following the principle that it is better to not include in your source repo derived binaries that can be built at buildtime. As far as the need to have cert-to-efi-sig-list, it is part of efitools and that is already documented as a requirement for the build host ([0] and [1]), and our baseline Docker file also includes it. [0] https://docs.u-boot.org/en/latest/develop/uefi/uefi.html#enabling-capsule-authentication [1] https://docs.u-boot.org/en/latest/develop/uefi/uefi.html#configuring-uefi-secure-boot Jon > Thanks > /Ilias >> >> Changes from v1: >> - Converted the single patch to a series to include a bug fix found during >> development. >> - Created an explicit rule for creating the ESL file for proper makefile >> dependency tracking. v1 had combined creating the ESL file and >> generating the .dtsi include in a single command. >> >> Jonathan Humphreys (2): >> scripts/Makefile.lib: fixes: Embed capsule public key in platform's >> dtb >> scripts/Makefile.lib: EFI: Use capsule CRT instead of ESL file >> >> board/sandbox/capsule_pub_esl_good.esl | Bin 831 -> 0 bytes >> configs/sandbox_defconfig | 2 +- >> configs/sandbox_flattree_defconfig | 2 +- >> doc/develop/uefi/uefi.rst | 8 >> lib/efi_loader/Kconfig | 12 +++- >> scripts/Makefile.lib | 24 +++- >> 6 files changed, 28 insertions(+), 20 deletions(-) >> delete mode 100644 board/sandbox/capsule_pub_esl_good.esl >> >> -- >> 2.34.1 >>
Re: [PATCH] efi: capsule: Improve capsule update porting to new boards
Ilias Apalodimas writes: > Hi Jon, > > On Sat, 8 Jun 2024 at 02:44, Jonathan Humphreys wrote: >> >> Add to the capsule update porting documentation that when capsule >> authentication is enabled (via EFI_CAPSULE_AUTHENTICATE), a public key >> certificate corresponding to the capsule signing key must be specified. >> >> Also add a config (EFI_CAPSULE_BOARD_INSECURE) that is set by default and >> causes a warning (info level) alerting a developer to consult this porting >> section to ensure the capsule update capability is secure for the platform. >> Once this section has been reviewed and confidence is established in the >> security of the capsule update implementation for this board, deselecting >> the EFI_CAPSULE_BOARD_INSECURE config will remove the warning. >> >> This config is to balance the desire to provide a reference implementation >> in upstream for capsule authentication but also provide some safeguards >> to prevent developers from overlooking the need to properly secure their >> implementation. >> >> Signed-off-by: Jonathan Humphreys >> --- >> doc/develop/uefi/uefi.rst| 30 +++--- >> lib/efi_loader/Kconfig | 13 + >> lib/efi_loader/efi_capsule.c | 5 + >> 3 files changed, 41 insertions(+), 7 deletions(-) >> >> diff --git a/doc/develop/uefi/uefi.rst b/doc/develop/uefi/uefi.rst >> index 985956ab85c..1c98c6e4ab4 100644 >> --- a/doc/develop/uefi/uefi.rst >> +++ b/doc/develop/uefi/uefi.rst >> @@ -526,6 +526,7 @@ following command can be issued >>--guid c1b629f1-ce0e-4894-82bf-f0a38387e630 \ >>optee.bin optee.capsule >> >> +.. _Enabling Capsule Authentication: >> >> Enabling Capsule Authentication >> *** >> @@ -631,17 +632,32 @@ where version.dtso looks like:: >> The properties of image-type-id and image-index must match the value >> defined in the efi_fw_image array as image_type_id and image_index. >> >> -Porting Capsule Updates to new boards >> -* >> +Porting Capsule Update to new boards >> + >> >> It is important, when using a reference board as a starting point for a >> custom >> board, that certain steps are taken to properly support Capsule Updates. >> >> -Capsule GUIDs need to be unique for each firmware and board. That is, if two >> -firmwares are built from the same source but result in different binaries >> -because they are built for different boards, they should have different >> GUIDs. >> -Therefore it is important when creating support for a new board, new GUIDs >> are >> -defined in the board's header file. *DO NOT* reuse capsule GUIDs. >> +Capsule GUIDs need to be unique for each firmware and board. That is, even >> +if two firmwares are built from the same source but result in different >> +binaries because they are built for different boards, they should have >> +different GUIDs. Therefore it is important when creating support for a new >> +board, new GUIDs are defined in the board's header file. *DO NOT* reuse >> +capsule GUIDs. >> + >> +When capsule authentication is enabled (via EFI_CAPSULE_AUTHENTICATE), it >> +is also very important to set the public key certificate corresponding to >> +the capsule signing key. See :ref:`Enabling Capsule Authentication` for >> +more information on signing capsules. *DO NOT* reuse the certificate file >> +from the reference board, which usually would be based on the development >> +board keys. >> + >> +By default, the EFI_CAPSULE_BOARD_INSECURE config is set to true and causes >> +a warning (info level) alerting a developer to consult this section to >> +ensure the capsule update capability is secure for the platform. Once this >> +section has been reviewed and confidence is established in the security of >> +the capsule update implementation for this board, deselecting >> +EFI_CAPSULE_BOARD_INSECURE config will remove the warning. >> >> Executing the boot manager >> ~~ >> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig >> index 23079a5709d..f4b1a20ee40 100644 >> --- a/lib/efi_loader/Kconfig >> +++ b/lib/efi_loader/Kconfig >> @@ -236,6 +236,19 @@ config EFI_CAPSULE_ON_DISK_EARLY >> executed as part of U-Boot initialisation so that they will >> surely take place whatever is set to distro_bootcmd. >> >> +config EFI_CAPSULE_BOARD_INSECURE >> + bool "Permit insecure capsule-on-disk support" >> + depends on EFI_HAVE_CAPSULE_SUPPORT >> + default y >> + help >> + Permit insecure capsule update support and emit a warning (info >> level) >> + when applying capsule updates, indicating that the configuration is >> + insecure and including a link to the capsule update porting guide >> in >> + U-Boot docs. Disabling this option removes the warning. A developer >> + configuration doesn't need to be secure, but before making a >> product >> + from t
Re: [PATCH v4 15/19] board: am62px: Define capsule update firmware info
Ilias Apalodimas writes:
> Hi Jon,
>
> On Fri, 24 May 2024 at 18:38, Jon Humphreys wrote:
>>
>> Ilias Apalodimas writes:
>>
>> > Hi Jonathan
>> >
>> > Thanks for working on this
>> >
>> > On Thu, May 09, 2024 at 11:41:19AM -0500, Jonathan Humphreys wrote:
>> >> Define the firmware components updatable via EFI capsule update, including
>> >> defining capsule GUIDs for the various firmware components for the AM62px
>> >> SK.
>> >>
>> >> Signed-off-by: Jonathan Humphreys
>> >> ---
>> >> board/ti/am62px/evm.c| 32
>> >> include/configs/am62px_evm.h | 24
>> >> 2 files changed, 56 insertions(+)
>> >>
>> >> diff --git a/board/ti/am62px/evm.c b/board/ti/am62px/evm.c
>> >> index 97a95ce8cc2..6d0f66e5dc0 100644
>> >> --- a/board/ti/am62px/evm.c
>> >> +++ b/board/ti/am62px/evm.c
>> >> @@ -6,6 +6,7 @@
>> >> *
>> >> */
>> >>
>> >> +#include
>> >> #include
>> >> #include
>> >> #include
>> >> @@ -13,6 +14,37 @@
>> >> #include
>> >> #include
>> >>
>> >> +struct efi_fw_image fw_images[] = {
>> >
>> > It's better if we add an
>> > #if IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT)
>> > for both of the structs that follow (and it applies to all your patches)
>> >
>>
>> Ilias, thanks for the reviews.
>>
>> I had this protected in #if's in an earlier patch set, as you suggest here.
>> However, in those reviews, Roger recommended that we don't do that and put
>> conditions around the use of it in set_dfu_alt_info().
>>
>
> Hmm but the function prototype itself is on an ifdef. If you want to
> remove the ifdef you got to do it everywhere
>
Are you referring to set_dfu_alt_info() which is guarded by
CONFIG_SET_DFU_ALT_INFO?
If so, that is separate but I can add a CONFIG_SET_DFU_ALT_INFO guard
around the definition, for now. But IMO it is a bit of a mess because it's
use and board specific defs are guarded by CONFIG_SET_DFU_ALT_INFO but the
weak/default definition is guarded by CONFIG_EFI_CAPSULE_FIRMWARE, which
causes problems because the configs are not always the same for all builds.
I was wanting to fix that too so I might do that as a separate patch and
make that patch a prerequisite for this series, which then allows me to
remove the definitions of set_dfu_alt_info() in this series.
Jon
> Thanks
> /Ilias
>
>> https://lore.kernel.org/all/b19f02e0-a80a-46d6-8296-5d5165777...@kernel.org/
>>
>> I assume the reasoning is to reduce #if's in the code and rely on the
>> compiler to be smart enough to remove dead data. (Roger, speak up if I
>> misrepresent you.)
>>
>> I'm ok to do either way. What is the preferred way in U-Boot?
>>
>> Thanks
>> Jon
>>
>> >> +{
>> >> +.image_type_id = AM62PX_SK_TIBOOT3_IMAGE_GUID,
>> >> +.fw_name = u"AM62PX_SK_TIBOOT3",
>> >> +.image_index = 1,
>> >> +},
>> >> +{
>> >> +.image_type_id = AM62PX_SK_SPL_IMAGE_GUID,
>> >> +.fw_name = u"AM62PX_SK_SPL",
>> >> +.image_index = 2,
>> >> +},
>> >> +{
>> >> +.image_type_id = AM62PX_SK_UBOOT_IMAGE_GUID,
>> >> +.fw_name = u"AM62PX_SK_UBOOT",
>> >> +.image_index = 3,
>> >> +}
>> >> +};
>> >> +
>> >> +struct efi_capsule_update_info update_info = {
>> >> +.dfu_string = "sf 0:0=tiboot3.bin raw 0 8;"
>> >> +"tispl.bin raw 8 20;u-boot.img raw 28 40",
>> >> +.num_images = ARRAY_SIZE(fw_images),
>> >> +.images = fw_images,
>> >> +};
>> >
>> > I haven't worked on any TI platforms lately so I cant say much about the
>> > naming and the flash regions. The definition seems correct
>> >
>> >
>> >> +
>> >> +void set_dfu_alt_info(char *interface, char *devstr)
>> >> +{
>> >> +if (IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT))
>> >> +env_set("dfu_alt_info", update_info.dfu_string);
>> >> +}
>> >
>> > There's a CONFIG_SET_DFU_ALT_INFO symbol. This better if we add a check
>> > here
>> > as well
>> >
>> >> +
>> >> int board_init(void)
>> >> {
>> >> return 0;
>> >> diff --git a/include/configs/am62px_evm.h b/include/configs/am62px_evm.h
>> >> index 06b12860e21..57a1ba9dc3c 100644
>> >> --- a/include/configs/am62px_evm.h
>> >> +++ b/include/configs/am62px_evm.h
>> >> @@ -8,6 +8,30 @@
>> >> #ifndef __CONFIG_AM62PX_EVM_H
>> >> #define __CONFIG_AM62PX_EVM_H
>> >>
>> > [...]
>> >
>> > Regards
>> > /Ilias
Re: [PATCH v2 4/4] configs: j721s2_evm_*_defconfig: Enable OSPI configs
Manorit Chawdhry writes: > Enable OSPI related configs to boot using OSPI > > Reviewed-by: Apurva Nandan > Signed-off-by: Manorit Chawdhry > --- > configs/j721s2_evm_a72_defconfig | 3 +++ > configs/j721s2_evm_r5_defconfig | 3 +++ > 2 files changed, 6 insertions(+) > > diff --git a/configs/j721s2_evm_a72_defconfig > b/configs/j721s2_evm_a72_defconfig > index 5ed8d00662e3..326beedea411 100644 > --- a/configs/j721s2_evm_a72_defconfig > +++ b/configs/j721s2_evm_a72_defconfig > @@ -51,6 +51,7 @@ CONFIG_SPL_FS_LOAD_PAYLOAD_NAME="u-boot.img" > CONFIG_SPL_I2C=y > CONFIG_SPL_DM_MAILBOX=y > CONFIG_SPL_MTD=y > +CONFIG_SPL_MTD_SUPPORT=y Manorit, CONFIG_SPL_MTD_SUPPORT is no longer a valid config. > CONFIG_SPL_DM_SPI_FLASH=y > CONFIG_SPL_NOR_SUPPORT=y > CONFIG_SPL_DM_RESET=y > @@ -59,6 +60,8 @@ CONFIG_SPL_RAM_SUPPORT=y > CONFIG_SPL_RAM_DEVICE=y > # CONFIG_SPL_SPI_FLASH_TINY is not set > CONFIG_SPL_SPI_FLASH_SFDP_SUPPORT=y > +CONFIG_SF_DEFAULT_MODE=0 > +CONFIG_SF_DEFAULT_SPEED=2500 CONFIG_SF_DEFAULT_MODE defaults to 0 already. Also please order config additions as safedefconfig would. thanks Jon > CONFIG_SPL_SPI_LOAD=y > CONFIG_SYS_SPI_U_BOOT_OFFS=0x28 > CONFIG_SPL_THERMAL=y > diff --git a/configs/j721s2_evm_r5_defconfig b/configs/j721s2_evm_r5_defconfig > index 3c958cafbe8f..4039841488f9 100644 > --- a/configs/j721s2_evm_r5_defconfig > +++ b/configs/j721s2_evm_r5_defconfig > @@ -57,6 +57,7 @@ CONFIG_SPL_FS_EXT4=y > CONFIG_SPL_I2C=y > CONFIG_SPL_DM_MAILBOX=y > CONFIG_SPL_MTD=y > +CONFIG_SPL_MTD_SUPPORT=y > CONFIG_SPL_DM_SPI_FLASH=y > CONFIG_SPL_NOR_SUPPORT=y > CONFIG_SPL_DM_RESET=y > @@ -66,6 +67,8 @@ CONFIG_SPL_RAM_DEVICE=y > CONFIG_SPL_REMOTEPROC=y > # CONFIG_SPL_SPI_FLASH_TINY is not set > CONFIG_SPL_SPI_FLASH_SFDP_SUPPORT=y > +CONFIG_SF_DEFAULT_MODE=0 > +CONFIG_SF_DEFAULT_SPEED=2500 > CONFIG_SPL_SPI_LOAD=y > CONFIG_SYS_SPI_U_BOOT_OFFS=0x8 > CONFIG_SPL_THERMAL=y > > -- > 2.43.2
Re: [PATCH v4 15/19] board: am62px: Define capsule update firmware info
Ilias Apalodimas writes:
> Hi Jonathan
>
> Thanks for working on this
>
> On Thu, May 09, 2024 at 11:41:19AM -0500, Jonathan Humphreys wrote:
>> Define the firmware components updatable via EFI capsule update, including
>> defining capsule GUIDs for the various firmware components for the AM62px
>> SK.
>>
>> Signed-off-by: Jonathan Humphreys
>> ---
>> board/ti/am62px/evm.c| 32
>> include/configs/am62px_evm.h | 24
>> 2 files changed, 56 insertions(+)
>>
>> diff --git a/board/ti/am62px/evm.c b/board/ti/am62px/evm.c
>> index 97a95ce8cc2..6d0f66e5dc0 100644
>> --- a/board/ti/am62px/evm.c
>> +++ b/board/ti/am62px/evm.c
>> @@ -6,6 +6,7 @@
>> *
>> */
>>
>> +#include
>> #include
>> #include
>> #include
>> @@ -13,6 +14,37 @@
>> #include
>> #include
>>
>> +struct efi_fw_image fw_images[] = {
>
> It's better if we add an
> #if IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT)
> for both of the structs that follow (and it applies to all your patches)
>
Ilias, thanks for the reviews.
I had this protected in #if's in an earlier patch set, as you suggest here.
However, in those reviews, Roger recommended that we don't do that and put
conditions around the use of it in set_dfu_alt_info().
https://lore.kernel.org/all/b19f02e0-a80a-46d6-8296-5d5165777...@kernel.org/
I assume the reasoning is to reduce #if's in the code and rely on the
compiler to be smart enough to remove dead data. (Roger, speak up if I
misrepresent you.)
I'm ok to do either way. What is the preferred way in U-Boot?
Thanks
Jon
>> +{
>> +.image_type_id = AM62PX_SK_TIBOOT3_IMAGE_GUID,
>> +.fw_name = u"AM62PX_SK_TIBOOT3",
>> +.image_index = 1,
>> +},
>> +{
>> +.image_type_id = AM62PX_SK_SPL_IMAGE_GUID,
>> +.fw_name = u"AM62PX_SK_SPL",
>> +.image_index = 2,
>> +},
>> +{
>> +.image_type_id = AM62PX_SK_UBOOT_IMAGE_GUID,
>> +.fw_name = u"AM62PX_SK_UBOOT",
>> +.image_index = 3,
>> +}
>> +};
>> +
>> +struct efi_capsule_update_info update_info = {
>> +.dfu_string = "sf 0:0=tiboot3.bin raw 0 8;"
>> +"tispl.bin raw 8 20;u-boot.img raw 28 40",
>> +.num_images = ARRAY_SIZE(fw_images),
>> +.images = fw_images,
>> +};
>
> I haven't worked on any TI platforms lately so I cant say much about the
> naming and the flash regions. The definition seems correct
>
>
>> +
>> +void set_dfu_alt_info(char *interface, char *devstr)
>> +{
>> +if (IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT))
>> +env_set("dfu_alt_info", update_info.dfu_string);
>> +}
>
> There's a CONFIG_SET_DFU_ALT_INFO symbol. This better if we add a check here
> as well
>
>> +
>> int board_init(void)
>> {
>> return 0;
>> diff --git a/include/configs/am62px_evm.h b/include/configs/am62px_evm.h
>> index 06b12860e21..57a1ba9dc3c 100644
>> --- a/include/configs/am62px_evm.h
>> +++ b/include/configs/am62px_evm.h
>> @@ -8,6 +8,30 @@
>> #ifndef __CONFIG_AM62PX_EVM_H
>> #define __CONFIG_AM62PX_EVM_H
>>
> [...]
>
> Regards
> /Ilias
Re: [PATCH v2 4/7] dts: beagleplay: binman: Include firmware capsules binman nodes
Tom Rini writes: > On Wed, May 22, 2024 at 11:12:35PM -0500, Jon Humphreys wrote: >> Tom Rini writes: >> >> > On Tue, May 21, 2024 at 09:20:26PM -0500, Jon Humphreys wrote: >> >> Tom Rini writes: >> >> >> >> > On Fri, Apr 19, 2024 at 04:28:16PM -0500, Jonathan Humphreys wrote: >> >> > >> >> >> Fill in the BeaglePlay's capsule GUID properties of the base binman >> >> >> capsule >> >> >> nodes. >> >> >> >> >> >> Signed-off-by: Jonathan Humphreys >> >> >> --- >> >> >> arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi | 27 >> >> >> arch/arm/dts/k3-am625-r5-beagleplay.dts | 15 +++ >> >> >> 2 files changed, 42 insertions(+) >> >> > >> >> > This series introduces failure to build in CI, and it's a little tricky >> >> > to replicate locally, depending on your environment. You first need to >> >> > NOT have BINMAN_INDIRS set and instead be using fake binaries. Second, >> >> > it seems python version dependent perhaps? I don't see this on my host, >> >> > but by: >> >> > - Using the CI container >> >> > - Setting up a virtualenv inside of it >> >> > - pip install -r tools/buildman/requirements.txt >> >> > I get: >> >> > $ ./tools/buildman/buildman --keep-outputs --reproducible-builds -dvel >> >> > --force-build -PEWM --output /tmp/am62x_beagleplay_r5 --board >> >> > am62x_beagleplay_r5 >> >> > Building current source for 1 boards (1 thread, 12 jobs per thread) >> >> >arm: + am62x_beagleplay_r5 >> >> > +(am62x_beagleplay_r5) Image 'tiboot3-am62x-gp-evm.bin' is missing >> >> > optional external blobs but is still functional: ti-fs-gp.bin >> >> > +(am62x_beagleplay_r5) >> >> > +(am62x_beagleplay_r5) /binman/tiboot3-am62x-gp-evm.bin/ti-fs-gp.bin >> >> > (ti-sysfw/ti-fs-firmware-am62x-gp.bin): >> >> > +(am62x_beagleplay_r5)Missing blob >> >> > +(am62x_beagleplay_r5) binman: object of type 'NoneType' has no len() >> >> > +(am62x_beagleplay_r5) make[1]: *** [Makefile:1126: .binman_stamp] >> >> > Error 1 >> >> > +(am62x_beagleplay_r5) make: *** [Makefile:177: sub-make] Error 2 >> >> > 001 /1 am62x_beagleplay_r5 >> >> >> >> Tom, this is failing in the CI container because the container is missing >> >> the mkeficapsule tool. >> >> >> >> To solve this, we just need to add it to the CI container. >> >> >> >> My understanding of binman's handling of missing bintools is that it >> >> should >> >> gracefully continue with fake data, so that buildman can successfully test >> >> out builds for boards even when you don't have all the required bintools. >> >> If I have that correct, I can also create a patch to properly handle this >> >> when using mkeficapsule. But I want to verify this is the desired >> >> behavior, >> >> since mkeficapsule isn't a unique or vendor specific tool, so shouldn't we >> >> require it as part of the U-Boot build environment and err out if it is >> >> missing? >> > >> > Perhaps it's a binman issue since we build mkeficapsule or at least >> > should be? Neha? >> > >> >> Never mind - I figured it out. >> >> The mkeficapsule tools is built by u-boot if TOOLS_MKEFICAPSULE config is >> set. I didn't explicitly set it because it is implied by >> EFI_CAPSULE_ON_DISK config. But this is only set for the A core u-boot, as >> that is what would apply the capsules. SPL running on the R cores does not >> need this. But that then means that the R core u-boot builds don't have >> TOOLS_MKEFICAPSULE set and if that is all that is being built (as in the >> case of buildman), the mkeficapsule tool isn't built and so is missing. >> >> So I need to explicitly set TOOLS_MKEFICAPSULE in the R core defconfigs. >> I'll repost the patches. > > Interesting. My next thought here is that whatever symbol is allowing > for "make a capsule" should be select'ing TOOLS_MKEFICAPSULE and so the > current logic is a bit flawed. I'm just not sure off-hand where it > should be instead, do you have some ideas now? Thanks. > There is no config that indicates that capsules will be generated for the resulting binary. The only thing I can think of is to scan the board's DTB for the presence of a capsule binman node, and then set the TOOLS_MKEFICAPSULE config. But this seems very complicated to do at build time. Jon > -- > Tom
Re: [PATCH v5 4/6] configs: am62x_evm_*: Enable USB and DFU support
Martyn Welch writes: > From: Sjoerd Simons > > Provide config fragments to enable USB host as well as USB gadget and DFU > support for a53 and r5. This relevant fragment is included into the > am62x EVM a53 defconfig. For the r5, due to the smaller available size, > the config fragment also disables support for persistent storage to free > up space for USB support. This fragment needs to be included is DFU > booting is desired. > > The CONFIG_DFU_SF option is placed in the defconfig rather than the > fragment as this is known not to be supported on all boards that can > support DFU. > > Signed-off-by: Sjoerd Simons > Signed-off-by: Martyn Welch > --- > Changes in v5: > - Switch to config fragment for a53 most DFU configuration > > Changes in v4: > - Move R5 dfu config to a config fragment rather then a full defconfig > - Don't enable XHCI for the R5 SPL, unneeded > > Changes in v3: > - Run savedefconfig to adjust to more recent u-boot > > Changes in v2: > - Create a seperate defconfig for R5 > > > > configs/am62x_a53_usbdfu.config | 30 ++ > configs/am62x_evm_a53_defconfig | 2 ++ > configs/am62x_r5_usbdfu.config | 28 > 3 files changed, 60 insertions(+) > create mode 100644 configs/am62x_a53_usbdfu.config > create mode 100644 configs/am62x_r5_usbdfu.config > > diff --git a/configs/am62x_a53_usbdfu.config b/configs/am62x_a53_usbdfu.config > new file mode 100644 > index 00..3a19cf2328 > --- /dev/null > +++ b/configs/am62x_a53_usbdfu.config > @@ -0,0 +1,29 @@ > +CONFIG_SYS_MALLOC_LEN=0x200 > +CONFIG_SPL_ENV_SUPPORT=y > +CONFIG_SPL_RAM_SUPPORT=y > +CONFIG_SPL_RAM_DEVICE=y > +CONFIG_SPL_USB_GADGET=y > +CONFIG_SPL_DFU=y > +CONFIG_CMD_DFU=y > +CONFIG_CMD_USB=y > +CONFIG_SYSCON=y > +CONFIG_SPL_SYSCON=y > +CONFIG_DFU_MMC=y > +CONFIG_DFU_RAM=y > +CONFIG_SYS_DFU_DATA_BUF_SIZE=0x5000 > +CONFIG_SYS_DFU_MAX_FILE_SIZE=0x80 > +CONFIG_USB=y > +CONFIG_DM_USB_GADGET=y > +CONFIG_SPL_DM_USB_GADGET=y > +CONFIG_USB_XHCI_HCD=y > +CONFIG_USB_XHCI_DWC3=y > +CONFIG_USB_DWC3=y > +CONFIG_USB_DWC3_GENERIC=y > +CONFIG_SPL_USB_DWC3_GENERIC=y > +CONFIG_SPL_USB_DWC3_AM62=y > +CONFIG_USB_DWC3_AM62=y > +CONFIG_USB_GADGET=y > +CONFIG_USB_GADGET_MANUFACTURER="Texas Instruments" > +CONFIG_USB_GADGET_VENDOR_NUM=0x0451 > +CONFIG_USB_GADGET_PRODUCT_NUM=0x6165 > +CONFIG_USB_GADGET_DOWNLOAD=y > diff --git a/configs/am62x_evm_a53_defconfig b/configs/am62x_evm_a53_defconfig > index 6c708dcb05..16294a6a79 100644 > --- a/configs/am62x_evm_a53_defconfig > +++ b/configs/am62x_evm_a53_defconfig > @@ -68,6 +68,7 @@ CONFIG_SPL_OF_TRANSLATE=y > CONFIG_CLK=y > CONFIG_SPL_CLK=y > CONFIG_CLK_TI_SCI=y > +CONFIG_DFU_SF=y > CONFIG_DMA_CHANNELS=y > CONFIG_TI_K3_NAVSS_UDMA=y > CONFIG_TI_SCI_PROTOCOL=y > @@ -111,3 +112,5 @@ CONFIG_SPL_SYSRESET=y > CONFIG_SYSRESET_TI_SCI=y > CONFIG_FS_FAT_MAX_CLUSTSIZE=16384 > CONFIG_EFI_SET_TIME=y > + > +#include > diff --git a/configs/am62x_r5_usbdfu.config b/configs/am62x_r5_usbdfu.config > new file mode 100644 > index 00..772bb2ab93 > --- /dev/null > +++ b/configs/am62x_r5_usbdfu.config > @@ -0,0 +1,28 @@ > +CONFIG_SPL_ENV_SUPPORT=y > +CONFIG_SYSCON=y > +CONFIG_SPL_SYSCON=y > +CONFIG_SYS_DFU_DATA_BUF_SIZE=0x5000 > +CONFIG_MISC=y > +CONFIG_USB=y > +CONFIG_DM_USB_GADGET=y > +CONFIG_SPL_DM_USB_GADGET=y > +CONFIG_USB_DWC3=y > +CONFIG_USB_DWC3_GENERIC=y > +CONFIG_SPL_USB_DWC3_GENERIC=y > +CONFIG_SPL_USB_DWC3_AM62=y > +CONFIG_USB_GADGET=y > +CONFIG_SPL_USB_GADGET=y > +CONFIG_USB_GADGET_MANUFACTURER="Texas Instruments" > +CONFIG_USB_GADGET_VENDOR_NUM=0x0451 > +CONFIG_USB_GADGET_PRODUCT_NUM=0x6165 > +CONFIG_USB_GADGET_DOWNLOAD=y > +CONFIG_SPL_DFU=y > +# CONFIG_SPL_MMC is not set > +# CONFIG_SPL_FS_FAT is not set > +# CONFIG_SPL_LIBDISK_SUPPORT is not set > +# CONFIG_SPL_SPI is not set > +# CONFIG_SPL_SYS_MALLOC is not set > +# CONFIG_CMD_GPT is not set > +# CONFIG_CMD_MMC is not set > +# CONFIG_CMD_FAT is not set > +# CONFIG_MMC_SDHCI is not set > -- > 2.43.0 Hi all, it appears that this patch breaks OSPI DFU on the board. In particular, changing the CONFIG_SYS_DFU_DATA_BUF_SIZE seems to be the culprit. I see the error as a DFU timeout when applying capsules. SF: Detected s28hs512t with page size 256 Bytes, erase size 256 KiB, total 64 MiB jedec_spi_nor flash@0: flash operation timed out #DFU write failed Jon
Re: [PATCH v2 4/7] dts: beagleplay: binman: Include firmware capsules binman nodes
Tom Rini writes: > On Tue, May 21, 2024 at 09:20:26PM -0500, Jon Humphreys wrote: >> Tom Rini writes: >> >> > On Fri, Apr 19, 2024 at 04:28:16PM -0500, Jonathan Humphreys wrote: >> > >> >> Fill in the BeaglePlay's capsule GUID properties of the base binman >> >> capsule >> >> nodes. >> >> >> >> Signed-off-by: Jonathan Humphreys >> >> --- >> >> arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi | 27 >> >> arch/arm/dts/k3-am625-r5-beagleplay.dts | 15 +++ >> >> 2 files changed, 42 insertions(+) >> > >> > This series introduces failure to build in CI, and it's a little tricky >> > to replicate locally, depending on your environment. You first need to >> > NOT have BINMAN_INDIRS set and instead be using fake binaries. Second, >> > it seems python version dependent perhaps? I don't see this on my host, >> > but by: >> > - Using the CI container >> > - Setting up a virtualenv inside of it >> > - pip install -r tools/buildman/requirements.txt >> > I get: >> > $ ./tools/buildman/buildman --keep-outputs --reproducible-builds -dvel >> > --force-build -PEWM --output /tmp/am62x_beagleplay_r5 --board >> > am62x_beagleplay_r5 >> > Building current source for 1 boards (1 thread, 12 jobs per thread) >> >arm: + am62x_beagleplay_r5 >> > +(am62x_beagleplay_r5) Image 'tiboot3-am62x-gp-evm.bin' is missing >> > optional external blobs but is still functional: ti-fs-gp.bin >> > +(am62x_beagleplay_r5) >> > +(am62x_beagleplay_r5) /binman/tiboot3-am62x-gp-evm.bin/ti-fs-gp.bin >> > (ti-sysfw/ti-fs-firmware-am62x-gp.bin): >> > +(am62x_beagleplay_r5)Missing blob >> > +(am62x_beagleplay_r5) binman: object of type 'NoneType' has no len() >> > +(am62x_beagleplay_r5) make[1]: *** [Makefile:1126: .binman_stamp] Error 1 >> > +(am62x_beagleplay_r5) make: *** [Makefile:177: sub-make] Error 2 >> > 001 /1 am62x_beagleplay_r5 >> >> Tom, this is failing in the CI container because the container is missing >> the mkeficapsule tool. >> >> To solve this, we just need to add it to the CI container. >> >> My understanding of binman's handling of missing bintools is that it should >> gracefully continue with fake data, so that buildman can successfully test >> out builds for boards even when you don't have all the required bintools. >> If I have that correct, I can also create a patch to properly handle this >> when using mkeficapsule. But I want to verify this is the desired behavior, >> since mkeficapsule isn't a unique or vendor specific tool, so shouldn't we >> require it as part of the U-Boot build environment and err out if it is >> missing? > > Perhaps it's a binman issue since we build mkeficapsule or at least > should be? Neha? > Never mind - I figured it out. The mkeficapsule tools is built by u-boot if TOOLS_MKEFICAPSULE config is set. I didn't explicitly set it because it is implied by EFI_CAPSULE_ON_DISK config. But this is only set for the A core u-boot, as that is what would apply the capsules. SPL running on the R cores does not need this. But that then means that the R core u-boot builds don't have TOOLS_MKEFICAPSULE set and if that is all that is being built (as in the case of buildman), the mkeficapsule tool isn't built and so is missing. So I need to explicitly set TOOLS_MKEFICAPSULE in the R core defconfigs. I'll repost the patches. Thanks for catching the bug, Jon > -- > Tom
Re: [PATCH v2 4/7] dts: beagleplay: binman: Include firmware capsules binman nodes
Tom Rini writes: > On Fri, Apr 19, 2024 at 04:28:16PM -0500, Jonathan Humphreys wrote: > >> Fill in the BeaglePlay's capsule GUID properties of the base binman capsule >> nodes. >> >> Signed-off-by: Jonathan Humphreys >> --- >> arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi | 27 >> arch/arm/dts/k3-am625-r5-beagleplay.dts | 15 +++ >> 2 files changed, 42 insertions(+) > > This series introduces failure to build in CI, and it's a little tricky > to replicate locally, depending on your environment. You first need to > NOT have BINMAN_INDIRS set and instead be using fake binaries. Second, > it seems python version dependent perhaps? I don't see this on my host, > but by: > - Using the CI container > - Setting up a virtualenv inside of it > - pip install -r tools/buildman/requirements.txt > I get: > $ ./tools/buildman/buildman --keep-outputs --reproducible-builds -dvel > --force-build -PEWM --output /tmp/am62x_beagleplay_r5 --board > am62x_beagleplay_r5 > Building current source for 1 boards (1 thread, 12 jobs per thread) >arm: + am62x_beagleplay_r5 > +(am62x_beagleplay_r5) Image 'tiboot3-am62x-gp-evm.bin' is missing optional > external blobs but is still functional: ti-fs-gp.bin > +(am62x_beagleplay_r5) > +(am62x_beagleplay_r5) /binman/tiboot3-am62x-gp-evm.bin/ti-fs-gp.bin > (ti-sysfw/ti-fs-firmware-am62x-gp.bin): > +(am62x_beagleplay_r5)Missing blob > +(am62x_beagleplay_r5) binman: object of type 'NoneType' has no len() > +(am62x_beagleplay_r5) make[1]: *** [Makefile:1126: .binman_stamp] Error 1 > +(am62x_beagleplay_r5) make: *** [Makefile:177: sub-make] Error 2 > 001 /1 am62x_beagleplay_r5 Tom, this is failing in the CI container because the container is missing the mkeficapsule tool. To solve this, we just need to add it to the CI container. My understanding of binman's handling of missing bintools is that it should gracefully continue with fake data, so that buildman can successfully test out builds for boards even when you don't have all the required bintools. If I have that correct, I can also create a patch to properly handle this when using mkeficapsule. But I want to verify this is the desired behavior, since mkeficapsule isn't a unique or vendor specific tool, so shouldn't we require it as part of the U-Boot build environment and err out if it is missing? thanks Jon > > -- > Tom
Re: [PATCH] mmc: sdhci: Correct ADMA_DESC_LEN to 12
"A. Sverdlin" writes:
> From: Alexander Sverdlin
>
> Commit 37cb626da25d0d ("mmc: sdhci: Add Support for ADMA2") introduced
> ADMA_DESC_LEN == 16 (64 bit case), but it was never used before commit
> 74755c1fed1b0 ("mmc: sdhci: introduce adma_write_desc() hook to struct
> sdhci_ops").
>
> "sizeof(struct sdhci_adma_desc)" (== 12 for 64bit case) was used instead.
>
> Confusion probably originates from Linux commit 685e444bbaa0
> ("mmc: sdhci: Add ADMA2 64-bit addressing support for V4 mode"), but
> the latter "V4 mode" was never ported to U-Boot.
>
> Fixes: 74755c1fed1b0 ("mmc: sdhci: introduce adma_write_desc() hook to struct
> sdhci_ops")
> Signed-off-by: Alexander Sverdlin
> ---
> include/sdhci.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/sdhci.h b/include/sdhci.h
> index d73a725609be3..810ef56e4be66 100644
> --- a/include/sdhci.h
> +++ b/include/sdhci.h
> @@ -300,7 +300,7 @@ struct sdhci_ops {
>
> #define ADMA_MAX_LEN 65532
> #ifdef CONFIG_DMA_ADDR_T_64BIT
> -#define ADMA_DESC_LEN16
> +#define ADMA_DESC_LEN12
> #else
> #define ADMA_DESC_LEN8
> #endif
> --
> 2.44.0
on TI AM64 and AM62p platforms:
Tested-by: Jonathan Humphreys
Re: [PATCH v2 01/16] board: Define GUIDs for firmware images
Ilias Apalodimas writes: > Hi both > > On Wed, 10 Apr 2024 at 10:36, Heinrich Schuchardt wrote: >> >> On 09.04.24 23:05, Jon Humphreys wrote: >> > Heinrich Schuchardt writes: >> > >> >> On 4/9/24 00:31, Jonathan Humphreys wrote: >> >>> Define GUIDs for the different firmware images (tiboot3.bin, tispl.bin, >> >>> u-boot.img, sysfw). > >> >>> Signed-off-by: Jonathan Humphreys >> >>> --- >> >>>include/configs/ti_armv7_common.h | 17 + >> >>>1 file changed, 17 insertions(+) >> >>> >> >>> diff --git a/include/configs/ti_armv7_common.h >> >>> b/include/configs/ti_armv7_common.h >> >>> index 3def7b1027e..4ce14a9b84c 100644 >> >>> --- a/include/configs/ti_armv7_common.h >> >>> +++ b/include/configs/ti_armv7_common.h >> >>> @@ -16,6 +16,23 @@ >> >>>#ifndef __CONFIG_TI_ARMV7_COMMON_H__ >> >>>#define __CONFIG_TI_ARMV7_COMMON_H__ >> >>> >> >>> +/* GUIDs for capsule updatable firmware images */ >> >> >> >> Please, provide code comments for the GUIDs, e.g. >> >> >> >> /** >> >>* define K3_TIBOOT3_IMAGE_GUID - firmware GUID for K3 tiboot3.bin >> >>* >> >>* This GUID is used in capsules updates to identify the tiboot3.bin >> >>* binary. >> >>*/ > > I'd go one step further tbh. Those GUIDs can be used by OEMs/ODMs > producing capsules for their future products. > Currently, we don't have a documented way to allow users to redefine > this (but Linaro is working on it). If both TI and an OEM use the > same GUID and try to upload the firmware to LVFS, we will have a GUID > clash. > I think we should have that on the comment as well, until we can > properly sort it out Ilias, I realized I may have defined the GUID's a bit incorrectly. I was thinking that the GUID's identify the firmware type (eg, SPL vs Uboot for TI devices). However, the SPL binary for say am62 will be different than the SPL binary for say am64, so I should have unique GUID's per firmware type *per board*. Correct? If so, I'll need to move these definitions to the board .h files, and my generic capsule binman nodes will need to have each board override the image GUID used. thanks Jon > > Thanks > /Ilias >> >> >> >> Cf. >> >> https://docs.kernel.org/doc-guide/kernel-doc.html#object-like-macro-documentation >> >> >> >> Best regards >> >> >> >> Heinrich >> >> >> > >> > Heinrich, thanks for reviewing! >> > >> > I modelled the GUID macros after how other boards and even core code >> > defined there's. (eg, include/configs/kontron-sl-mx8mm.h or >> > include/efi_api.h). >> > >> > However, if this is the new direction, I will format as you suggest. >> > Please confirm. >> >> Hello Jon, >> >> Without properly documenting macros we make the live of developers more >> difficult. Yes, we still have a lot of missing code documentation. But >> we should not follow poor example. >> >> Best regards >> >> Heinrich >> >> > >> > thanks >> > Jon >> > >> >>> +#define K3_TIBOOT3_IMAGE_GUID \ >> >>> + EFI_GUID(0xe672b518, 0x7cd7, 0x4014, 0xbd, 0x8d, \ >> >>> +0x40, 0x72, 0x4d, 0x0a, 0xd4, 0xdc) >> >>> + >> >>> +#define K3_SPL_IMAGE_GUID \ >> >>> + EFI_GUID(0x86f710ad, 0x10cf, 0x46ea, 0xac, 0x67, \ >> >>> +0x85, 0x6a, 0xe0, 0x6e, 0xfa, 0xd2) >> >>> + >> >>> +#define K3_UBOOT_IMAGE_GUID \ >> >>> + EFI_GUID(0x81b58fb0, 0x3b00, 0x4add, 0xa2, 0x0a, \ >> >>> +0xc1, 0x85, 0xbb, 0xac, 0xa1, 0xed) >> >>> + >> >>> +#define K3_SYSFW_IMAGE_GUID \ >> >>> + EFI_GUID(0x6fd10680, 0x361b, 0x431f, 0x80, 0xaa, \ >> >>> +0x89, 0x94, 0x55, 0x81, 0x9e, 0x11) >> >>> + >> >>>/* >> >>> * We setup defaults based on constraints from the Linux kernel, >> >>> which should >> >>> * also be safe elsewhere. We have the default load at 32MB into DDR >> >>> (for >>
Re: [PATCH 01/13] ti:keys Add EFI signature list
Ilias Apalodimas writes: > Hi Jon, > > > On Wed, 10 Apr 2024 at 20:35, Jon Humphreys wrote: >> >> Ilias Apalodimas writes: >> >> > On Tue, 9 Apr 2024 at 23:14, Andrew Davis wrote: >> >> >> >> On 4/9/24 2:26 PM, Heinrich Schuchardt wrote: >> >> > On 4/9/24 14:14, Andrew Davis wrote: >> >> >> On 4/8/24 10:34 PM, Heinrich Schuchardt wrote: >> >> >>> On 4/8/24 23:33, Jonathan Humphreys wrote: >> >> >>>> EFI signature list using TI dummy keys. >> >> >>> >> >> >>> Adding vendor public keys into the code base to lock down generated >> >> >>> binaries to the vendors unpublished private key does not match well >> >> >>> with >> >> >>> the intent of the GNU public license. >> >> >>> >> >> >> >> >> >> The matching private keys are already published in this same >> >> >> repo/directory (arch/arm/mach-k3/keys). >> >> >> >> >> >> Andrew >> >> > >> >> > Why should we create signed capsules which are already compromised by >> >> > publishing the private key? >> >> > >> >> >> >> If you buy these devices you have two options, you can burn real >> >> keys, or you can burn these dummy keys. If you burn dummy keys >> >> then these images will boot and so will any image you or anyone >> >> else wants to boot on the device. (since the keys are published >> >> anyone can make images for them, that is how we do GP (general >> >> purpose) devices these days) >> >> >> >> If you burn your own keys, then you switch out these keys here >> >> and your device will only boot images that you permit by signing >> >> with your keys. >> > >> > I am not sure I am following you here. We don't burn anything in the >> > case of EFI keys. They are placed in an elf section and we assume the >> > device will have a chain of trust enabled, naturally verifying those >> > keys along with the u-boot binary. >> > >> >> >> >> You'll find plenty of open source projects do the same and >> >> give out example keys to show how to use real keys, even >> >> official GNU projects. >> > >> > Yes, but the keys defined here are useless unless you have a default >> > defconfig that uses them and embeds them in the binary. I am not cc'ed >> > in all the patches of the series, is that added somewhere? And if you >> >> Yes, they are part of this series >> https://lore.kernel.org/r/20240408213349.96610-1-j-humphr...@ti.com. >> Thanks for the reviews. >> >> > unconditionally enable secure boot It would be far more interesting to >> > embed the MS SHIM key along with that special key you are trying to >> > define, so that firmware can boot COTS distros as well >> >> Yes, we should consider. But since that is outside of the EFI capsule >> use case, I would rather take it up in a separate patch. > > Ok, the commit message wasn't clear, and based on Andrews's initial > response I thought you wanted to use those for UEFI secure boot, not > capsule updates. > Those are your boards so I won't NAK this, but I'd strongly advise > *not* to add this. I assume you want capsule auth by default because > SystemReady-IR >=2.0 mandates it? > > In that case, it would be a far better idea to document the process of > creating signed capsules clearly either in U-Boots EFI docs and/or > your board docs. > I am pretty confident that if we merge this now we will have future > products using the keys above Thanks Ilias. If I understand you correctly, I don't agree with the approach of not having a working implementation so that developers are forced to think through their support. Not having a feature enabled in upstream leads to latent bugs, bit rot, lack of coordination and openess, etc. It worries me that there are so many claims of authenticated capsule support but nothing in upstream on those devices. But I absolutely acknowledge your concern that if we make this 'just work' then developers will overlook the details and not properly secure their solutions. What I suggest to mitigate this: 1) as you say, add documentation, including a 'porting guide' section so developers know what steps they need to take, and 2) Add a developer config that is set by default. With this config set, during capsule updates, emit a warni
Re: [PATCH v2 12/16] arm: dts: k3-am625-sk-u-boot: Add sysreset-controller node
Mattijs Korpershoek writes:
> Hi Jonathan,
>
> Thank you for the patch.
>
> On lun., avril 08, 2024 at 17:31, Jonathan Humphreys
> wrote:
>
>> Signed-off-by: Jonathan Humphreys
>
> Please consider adding a commit message body.
Got it. thanks.
BTW, the next version of this series will drop this patch as Andrew has
submitted another patch removing the need for this one. See
https://lore.kernel.org/r/20240402160908.508974-1-...@ti.com.
>
> On the TI vendor tree, there is a similar patch with a commit message:
> https://git.ti.com/cgit/ti-u-boot/ti-u-boot/commit/?h=ti-u-boot-2023.04&id=c5296d943c2c84dd6dcb3b91305d006ac46f3157
>
> Before patch:
> => reset
> resetting ...
> System reset not supported on this platform
> ### ERROR ### Please RESET the board ###
>
> With patch applied:
> => reset
> resetting ...
>
> Tested-by: Mattijs Korpershoek # on am62x sk evm
>
> Andrew also suggested to me that if we are interested by A53 reset only,
> we can PSCI reset instead for all k3 architecture:
>
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -784,6 +784,9 @@ config ARCH_K3
> bool "Texas Instruments' K3 Architecture"
> select SPL
> select SUPPORT_SPL
> + select PSCI_RESET if ARM64
> + select SYSRESET if ARM64
> + select SYSRESET_PSCI if ARM64
> select FIT
> select REGEX
> select FIT_SIGNATURE if ARM64
>
> Has the above been considered?
I am not aware. I would think that you want full reset, unless you are
thinking about specifying a reset level?
Jon
>
>
>> ---
>> arch/arm/dts/k3-am625-sk-u-boot.dtsi | 9 +
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/arch/arm/dts/k3-am625-sk-u-boot.dtsi
>> b/arch/arm/dts/k3-am625-sk-u-boot.dtsi
>> index fa778b0ff4c..35bfeae75a0 100644
>> --- a/arch/arm/dts/k3-am625-sk-u-boot.dtsi
>> +++ b/arch/arm/dts/k3-am625-sk-u-boot.dtsi
>> @@ -46,3 +46,12 @@
>> &cpsw_port2 {
>> status = "disabled";
>> };
>> +
>> +&dmsc {
>> +bootph-pre-ram;
>> +
>> +k3_sysreset: sysreset-controller {
>> +compatible = "ti,sci-sysreset";
>> +bootph-pre-ram;
>> +};
>> +};
>> --
>> 2.34.1
Re: [PATCH 2/2] arm: dts: k3: Remove unneeded ti, sci-sysreset binding and nodes
Andrew Davis writes:
> This extra binding is non-standard and now unneeded as we bind the
> sysreset driver automatically. This matches what is done in Linux
> and allows us to more closely match the DTBs. Remove the binding
> and all users.
>
> Signed-off-by: Andrew Davis
> ---
> arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi | 7 -
> .../k3-am625-phyboard-lyra-rdk-u-boot.dtsi| 7 -
> .../dts/k3-am625-verdin-wifi-dev-u-boot.dtsi | 7 -
> arch/arm/dts/k3-am62a7-sk-u-boot.dtsi | 4 ---
> arch/arm/dts/k3-am62p5-sk-u-boot.dtsi | 5
> arch/arm/dts/k3-am642-evm-u-boot.dtsi | 7 -
> .../k3-am642-phyboard-electra-rdk-u-boot.dtsi | 4 ---
> arch/arm/dts/k3-am642-sk-u-boot.dtsi | 7 -
> .../dts/k3-am65-iot2050-common-u-boot.dtsi| 4 ---
> .../arm/dts/k3-am68-sk-base-board-u-boot.dtsi | 4 ---
> arch/arm/dts/k3-am69-sk-u-boot.dtsi | 7 -
> .../k3-j7200-common-proc-board-u-boot.dtsi| 4 ---
> .../dts/k3-j721e-beagleboneai64-u-boot.dtsi | 4 ---
> .../k3-j721e-common-proc-board-u-boot.dtsi| 4 ---
> arch/arm/dts/k3-j721e-sk-u-boot.dtsi | 4 ---
> .../k3-j721s2-common-proc-board-u-boot.dtsi | 4 ---
> arch/arm/dts/k3-j784s4-evm-u-boot.dtsi| 7 -
> .../sysreset/ti,sci-sysreset.txt | 29 ---
> 18 files changed, 119 deletions(-)
> delete mode 100644 doc/device-tree-bindings/sysreset/ti,sci-sysreset.txt
>
> diff --git a/arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi
> b/arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi
> index cca0f44b7d8..fb2032068d1 100644
> --- a/arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi
> +++ b/arch/arm/dts/k3-am625-beagleplay-u-boot.dtsi
> @@ -41,13 +41,6 @@
> clock-frequency = <2500>;
> };
>
> -&dmsc {
> - k3_sysreset: sysreset-controller {
> - compatible = "ti,sci-sysreset";
> - bootph-all;
> - };
> -};
> -
> &sd_pins_default {
> /* Force to use SDCD card detect pin */
> pinctrl-single,pins = <
> diff --git a/arch/arm/dts/k3-am625-phyboard-lyra-rdk-u-boot.dtsi
> b/arch/arm/dts/k3-am625-phyboard-lyra-rdk-u-boot.dtsi
> index f6138f3058f..94162282068 100644
> --- a/arch/arm/dts/k3-am625-phyboard-lyra-rdk-u-boot.dtsi
> +++ b/arch/arm/dts/k3-am625-phyboard-lyra-rdk-u-boot.dtsi
> @@ -42,13 +42,6 @@
> bootph-all;
> };
>
> -&dmsc {
> - k3_sysreset: sysreset-controller {
> - compatible = "ti,sci-sysreset";
> - bootph-all;
> - };
> -};
> -
> &fss {
> bootph-all;
> };
> diff --git a/arch/arm/dts/k3-am625-verdin-wifi-dev-u-boot.dtsi
> b/arch/arm/dts/k3-am625-verdin-wifi-dev-u-boot.dtsi
> index 28b697b67ae..7fe7ae41543 100644
> --- a/arch/arm/dts/k3-am625-verdin-wifi-dev-u-boot.dtsi
> +++ b/arch/arm/dts/k3-am625-verdin-wifi-dev-u-boot.dtsi
> @@ -85,13 +85,6 @@
> bootph-all;
> };
>
> -&dmsc {
> - k3_sysreset: sysreset-controller {
> - compatible = "ti,sci-sysreset";
> - bootph-all;
> - };
> -};
> -
> &fss {
> bootph-all;
> };
> diff --git a/arch/arm/dts/k3-am62a7-sk-u-boot.dtsi
> b/arch/arm/dts/k3-am62a7-sk-u-boot.dtsi
> index 31b89b41748..c42dec16194 100644
> --- a/arch/arm/dts/k3-am62a7-sk-u-boot.dtsi
> +++ b/arch/arm/dts/k3-am62a7-sk-u-boot.dtsi
> @@ -119,10 +119,6 @@
>
> &dmsc {
> bootph-all;
> - k3_sysreset: sysreset-controller {
> - compatible = "ti,sci-sysreset";
> - bootph-all;
> - };
> };
>
> &vdd_mmc1 {
> diff --git a/arch/arm/dts/k3-am62p5-sk-u-boot.dtsi
> b/arch/arm/dts/k3-am62p5-sk-u-boot.dtsi
> index c166d655390..cf087c6e343 100644
> --- a/arch/arm/dts/k3-am62p5-sk-u-boot.dtsi
> +++ b/arch/arm/dts/k3-am62p5-sk-u-boot.dtsi
> @@ -15,9 +15,4 @@
>
> &dmsc {
> bootph-pre-ram;
> -
> - k3_sysreset: sysreset-controller {
> - compatible = "ti,sci-sysreset";
> - bootph-pre-ram;
> - };
> };
> diff --git a/arch/arm/dts/k3-am642-evm-u-boot.dtsi
> b/arch/arm/dts/k3-am642-evm-u-boot.dtsi
> index ee6656774d6..705b3baa81c 100644
> --- a/arch/arm/dts/k3-am642-evm-u-boot.dtsi
> +++ b/arch/arm/dts/k3-am642-evm-u-boot.dtsi
> @@ -23,13 +23,6 @@
> bootph-all;
> };
>
> -&dmsc {
> - k3_sysreset: sysreset-controller {
> - compatible = "ti,sci-sysreset";
> - bootph-all;
> - };
> -};
> -
> &sdhci0 {
> bootph-all;
> };
> diff --git a/arch/arm/dts/k3-am642-phyboard-electra-rdk-u-boot.dtsi
> b/arch/arm/dts/k3-am642-phyboard-electra-rdk-u-boot.dtsi
> index 5dfc40a843b..4677c35e2d9 100644
> --- a/arch/arm/dts/k3-am642-phyboard-electra-rdk-u-boot.dtsi
> +++ b/arch/arm/dts/k3-am642-phyboard-electra-rdk-u-boot.dtsi
> @@ -29,10 +29,6 @@
>
> &dmsc {
> bootph-all;
> - k3_sysreset: sysreset-controller {
> - compatible = "ti,sci-sysreset";
> - bootph-all;
> - };
> };
>
> &dmss {
> diff --git a/arch/arm/dts/k3-am642-sk-u-boot.dtsi
> b/arch/arm/d
Re: [PATCH 1/2] firmware: ti_sci: Bind sysreset driver when enabled
Andrew Davis writes:
> The sysreset TI-SCI API is available with TI-SCI always, there is no need
> for a DT node to describe the availability of this. If the sysreset driver
> is available then bind it during ti-sci probe.
>
> Remove the unneeded device tree matching.
>
> Signed-off-by: Andrew Davis
> ---
> drivers/firmware/ti_sci.c | 7 +++
> drivers/sysreset/sysreset-ti-sci.c | 6 --
> 2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c
> index ee092185588..6c581b9df9c 100644
> --- a/drivers/firmware/ti_sci.c
> +++ b/drivers/firmware/ti_sci.c
> @@ -16,6 +16,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -2840,6 +2841,12 @@ static int ti_sci_probe(struct udevice *dev)
>
> INIT_LIST_HEAD(&info->dev_list);
>
> + if (IS_ENABLED(CONFIG_SYSRESET_TI_SCI)) {
> + ret = device_bind_driver(dev, "ti-sci-sysreset", "sysreset",
> NULL);
> + if (ret)
> + dev_warn(dev, "cannot bind SYSRESET (ret = %d)\n", ret);
> + }
> +
> return 0;
> }
>
> diff --git a/drivers/sysreset/sysreset-ti-sci.c
> b/drivers/sysreset/sysreset-ti-sci.c
> index 5fc05c46cb0..0de132633a8 100644
> --- a/drivers/sysreset/sysreset-ti-sci.c
> +++ b/drivers/sysreset/sysreset-ti-sci.c
> @@ -60,15 +60,9 @@ static struct sysreset_ops ti_sci_sysreset_ops = {
> .request = ti_sci_sysreset_request,
> };
>
> -static const struct udevice_id ti_sci_sysreset_of_match[] = {
> - { .compatible = "ti,sci-sysreset", },
> - { /* sentinel */ },
> -};
> -
> U_BOOT_DRIVER(ti_sci_sysreset) = {
> .name = "ti-sci-sysreset",
> .id = UCLASS_SYSRESET,
> - .of_match = ti_sci_sysreset_of_match,
> .probe = ti_sci_sysreset_probe,
> .priv_auto = sizeof(struct ti_sci_sysreset_data),
> .ops = &ti_sci_sysreset_ops,
> --
> 2.39.2
Tested-by: Jonathan Humphreys
Re: [PATCH] arm: mach-k3: security: Lower verbosity of cert message for GP
Andrew Davis writes:
> When we find a certificate on an image to be booted on a GP device we
> print out a message explaining that the certificate is being skipped.
> This message is rather long and is printed for every image. Shorten
> the message and make the long version into a debug message.
>
> Signed-off-by: Andrew Davis
> ---
> arch/arm/mach-k3/security.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-k3/security.c b/arch/arm/mach-k3/security.c
> index 22697a263a8..7c46914d9dd 100644
> --- a/arch/arm/mach-k3/security.c
> +++ b/arch/arm/mach-k3/security.c
> @@ -50,7 +50,7 @@ void ti_secure_image_check_binary(void **p_image, size_t
> *p_size)
>
> if (get_device_type() == K3_DEVICE_TYPE_GP) {
> if (ti_secure_cert_detected(*p_image)) {
> - printf("Warning: Detected image signing certificate on
> GP device. "
> + debug("Warning: Detected image signing certificate on
> GP device. "
> "Skipping certificate to prevent boot failure. "
> "This will fail if the image was also
> encrypted\n");
>
> @@ -60,6 +60,7 @@ void ti_secure_image_check_binary(void **p_image, size_t
> *p_size)
> return;
> }
>
> + printf("Skipping authentication on GP device\n");
> *p_image += cert_length;
> *p_size -= cert_length;
> }
> --
> 2.39.2
Would be nice if it only emitted the message once!
Tested-by: Jonathan Humphreys
Re: [PATCH 01/13] ti:keys Add EFI signature list
Ilias Apalodimas writes:
> On Tue, 9 Apr 2024 at 23:14, Andrew Davis wrote:
>>
>> On 4/9/24 2:26 PM, Heinrich Schuchardt wrote:
>> > On 4/9/24 14:14, Andrew Davis wrote:
>> >> On 4/8/24 10:34 PM, Heinrich Schuchardt wrote:
>> >>> On 4/8/24 23:33, Jonathan Humphreys wrote:
>> EFI signature list using TI dummy keys.
>> >>>
>> >>> Adding vendor public keys into the code base to lock down generated
>> >>> binaries to the vendors unpublished private key does not match well with
>> >>> the intent of the GNU public license.
>> >>>
>> >>
>> >> The matching private keys are already published in this same
>> >> repo/directory (arch/arm/mach-k3/keys).
>> >>
>> >> Andrew
>> >
>> > Why should we create signed capsules which are already compromised by
>> > publishing the private key?
>> >
>>
>> If you buy these devices you have two options, you can burn real
>> keys, or you can burn these dummy keys. If you burn dummy keys
>> then these images will boot and so will any image you or anyone
>> else wants to boot on the device. (since the keys are published
>> anyone can make images for them, that is how we do GP (general
>> purpose) devices these days)
>>
>> If you burn your own keys, then you switch out these keys here
>> and your device will only boot images that you permit by signing
>> with your keys.
>
> I am not sure I am following you here. We don't burn anything in the
> case of EFI keys. They are placed in an elf section and we assume the
> device will have a chain of trust enabled, naturally verifying those
> keys along with the u-boot binary.
>
>>
>> You'll find plenty of open source projects do the same and
>> give out example keys to show how to use real keys, even
>> official GNU projects.
>
> Yes, but the keys defined here are useless unless you have a default
> defconfig that uses them and embeds them in the binary. I am not cc'ed
> in all the patches of the series, is that added somewhere? And if you
Yes, they are part of this series
https://lore.kernel.org/r/20240408213349.96610-1-j-humphr...@ti.com.
Thanks for the reviews.
> unconditionally enable secure boot It would be far more interesting to
> embed the MS SHIM key along with that special key you are trying to
> define, so that firmware can boot COTS distros as well
Yes, we should consider. But since that is outside of the EFI capsule
use case, I would rather take it up in a separate patch.
>
> Thanks
> /Ilias
>
>
>>
>> https://github.com/gpg/gnupg/tree/master/tests/openpgp/samplekeys
>>
>> Andrew
>>
>> > Best regards
>> >
>> > Heinrich
>> >
>> >>
>> >>> Best regards
>> >>>
>> >>> Heinrich
>> >>>
>>
>> Signed-off-by: Jonathan Humphreys
>> ---
>> arch/arm/mach-k3/keys/custMpk.esl | Bin 0 -> 1523 bytes
>> 1 file changed, 0 insertions(+), 0 deletions(-)
>> create mode 100644 arch/arm/mach-k3/keys/custMpk.esl
>>
>> diff --git a/arch/arm/mach-k3/keys/custMpk.esl
>> b/arch/arm/mach-k3/keys/custMpk.esl
>> new file mode 100644
>> index
>> ..2feb704e0a5fd126410de451d3c0fa4d3edccc52
>> GIT binary patch
>> literal 1523
>> zcmZ1&d0^?2Da*aux2_hA(f&~MnUw(yu0v@E4?-F=u^u*PVqVQ8QZ((-^A*$m*Kg7c
>> z&78AJODc2mtxpELY@Awc9&O)w85y}*84Mcd8gd(OvN4CUun9AT2E#ZUJWL@GhWtR)
>> zKpA!(HkZVloWx>7bput902hy3NNPo5v4Uq_aY<2WZfaf$h@G5YRFGekSdyAzC~P1I
>> zQpnB26;PC)oLXF*UsMbeWai-t@l*&dEdVMmF_blshP#N9QH-w`BJNO> zYh-L-W?*PwYGi0=7A4MWYz$;tLb-$9{Y^|t$U)A?%D~*j#Lr;R#Kgta#Kg$3Uu2!<
>> zjryX?*~({Md+?>+QS$x7=il`0?bc6sZ`Vxxl^6N{>i2E;SY*4-T$+0G;)5dxe+2CR
>> z@4+)sDPWdQb@%6KTpDVdm)v}?GSpG(w_UV)&i+#e3fJowDZO)JR83lIcbw(hMu}}Y
>> z2ZZwYAI-LVx@^G;HdkgxaX&Hnl_l3&{H|3l7uX@Vl5di{>fQQ{pDynFlySp2(z~g)
>> z{LIBUzm&K9j_CMw_SIFfPdcT#zmg6g> zFUCK{#b(Lp`M3Uj
>> zGOKycyEe+n{G(Rmg}jB!)0ySk-!kkj_R7#OT+}pcG0VXh?f+ftRvnyw#hUea^Iyfn
>> ze|zgKPKrqe@jYWU?v<50X(n^lZ*G%j$JyCh`*Px|H*K=2WXP)hx>jng+}Q}N^KoDN
>> z8dh8T-~Dmrp2?yk3O6Gqbz7O@> zcmJwBy6&2hKub%G{j3IK(?7m@uI43#1e~wSZJ5sTtDjrp@7@{O3(faN{`Gp}x{$M5
>> z{A7`c@pjfYq1Z=JvgZ^-zCC<(HFTBwYhTX$k`7IJX`SM!H}f`Mv+(Op6uVY(<(^o4
>> zpyXAj9nF_c-1AW=dcRVPvS;*B%(4`P|iK>Vg$XDgN9sr}Df{
>> z7X0es=RPHr8RB+*)}q}h%gn?x9PO4y*Qog};x<> zmcsdI9w!^rFPt^&c~{1?L~DJ4TRPv>t%rn8xi;KBE9A!Dppb9yru|>RCb9PcXWpE>
>> zKlQ}fzw*izXI|}|r!O*nb&cP9#VhHRn;B> zXWR0Vllb2@>`1LC^xvIctvLCYhRA_6yCS~2&!0SH1xwv(O~+5t
>> z^|E$S{MM^8j9J5`sQ6pud{2Lz?k`zncbjvHj%eutjusUol}8;%cbPLCO|e;ZJ^tXe
>> z_N{pmM}uCi3UWO3=hMc> zrslJ=2XA9^$j#UDYwo;ZvZwb!|L%YP%v|ie|7-1PP+q3DZ&vEWgHHrjHv|NzEVjO?
>> zKFeRbXv>iTPl?N16Xv@buq_d@TU`uX0_s&g9M2C6cKx4E;{?
>> zt`1&)Tk-yb?sKMPI~!}xt*d*!tMat!r1`}jul#i@lDB8rnu>ba_-^4!iQ5{|tb3TX
>> z>fTMIw2!Me3{Dw*WZotC<4@h> D66<>
>> literal 0
>> HcmV?d1
>>
>> >>>
>> >
Re: [PATCH 3/7] dts: j721e: binman: Include firmware capsules binman nodes
Andrew Davis writes:
> On 4/8/24 5:17 PM, Jonathan Humphreys wrote:
>> Signed-off-by: Jonathan Humphreys
>> ---
>> arch/arm/dts/k3-j721e-binman.dtsi | 32 +++
>> 1 file changed, 32 insertions(+)
>>
>> diff --git a/arch/arm/dts/k3-j721e-binman.dtsi
>> b/arch/arm/dts/k3-j721e-binman.dtsi
>> index 75a6e9599b9..9169551c422 100644
>> --- a/arch/arm/dts/k3-j721e-binman.dtsi
>> +++ b/arch/arm/dts/k3-j721e-binman.dtsi
>> @@ -207,6 +207,29 @@
>> };
>> };
>> };
>> +
>> +#include "k3-binman-capsule-r5.dtsi"
>> +
>> +// Capsue update GUIDs. See ti_armv7_common.h.
>> +#define K3_SYSFW_IMAGE_UUID_STR "6fd10680-361b-431f-80aa-899455819e11"
>> +
>> +&binman {
>> +capsule-sysfw {
>> +filename = "sysfw-capsule.bin";
>> +efi-capsule {
>> +image-index = <0x4>;
>> +image-guid = K3_SYSFW_IMAGE_UUID_STR;
>> +private-key = "arch/arm/mach-k3/keys/custMpk.pem";
>> +public-key-cert = "arch/arm/mach-k3/keys/custMpk.crt";
>> +monotonic-count = <0x1>;
>> +
>> +blob {
>> +filename = "sysfw.itb";
>> +};
>> +};
>> +};
>> +};
>> +
>> #endif
>>
>> #ifdef CONFIG_TARGET_J721E_A72_EVM
>> @@ -585,4 +608,13 @@
>> };
>> };
>> };
>> +
>> +#include "k3-binman-capsule.dtsi"
>> +&tispl_name {
>> +filename = "tispl.bin_unsigned";
>
> Why use the _unsigned images here? HS devices cannot boot unsigned GP images,
> but both GP and HS devices *can* boot the normal signed images (GP just strips
> the signatures off). So no need to use the _unsigned images anymore (I'm
> planning to just remove them at some point to prevent this confusion).
>
I can do that.
Note that you will then see warnings on GP devices during boot:
Warning: Detected image signing certificate on GP device. Skipping
certificate to prevent boot failure. This will fail if the image was also
encrypted
Jon
> Andrew
>
>> +};
>> +&uboot_name {
>> +filename = "u-boot.img_unsigned";
>> +};
>> +
>> #endif
Re: [PATCH v2 02/16] board: am64x: Define capsule update firmware info
Heinrich Schuchardt writes:
> On 4/9/24 00:31, Jonathan Humphreys wrote:
>> Define the firmwares updatable via EFI capsule update.
>
> Nits:
>
> %s/firmwares/firmware/ (firmware is uncountable).
>
>>
>> Signed-off-by: Jonathan Humphreys
>> ---
>> board/ti/am64x/evm.c | 33 +
>> 1 file changed, 33 insertions(+)
>>
>> diff --git a/board/ti/am64x/evm.c b/board/ti/am64x/evm.c
>> index b8de69da06c..876c5c25d42 100644
>> --- a/board/ti/am64x/evm.c
>> +++ b/board/ti/am64x/evm.c
>> @@ -7,6 +7,7 @@
>>*
>>*/
>>
>> +#include
>> #include
>> #include
>> #include
>> @@ -27,6 +28,38 @@
>>
>> DECLARE_GLOBAL_DATA_PTR;
>>
>> +#if IS_ENABLED(CONFIG_EFI_HAVE_CAPSULE_SUPPORT)
>> +struct efi_fw_image fw_images[] = {
>> +{
>> +.image_type_id = K3_TIBOOT3_IMAGE_GUID,
>> +.fw_name = u"K3_TIBOOT3",
>> +.image_index = 1,
>> +},
>> +{
>> +.image_type_id = K3_SPL_IMAGE_GUID,
>> +.fw_name = u"K3_SPL",
>> +.image_index = 2,
>> +},
>> +{
>> +.image_type_id = K3_UBOOT_IMAGE_GUID,
>> +.fw_name = u"K3_UBOOT",
>> +.image_index = 3,
>> +}
>> +};
>> +
>> +struct efi_capsule_update_info update_info = {
>> +.dfu_string = "sf 0:0=tiboot3.bin raw 0 10;tispl.bin raw 10
>> 20;u-boot.img raw 30 40",
>
> Is TI using other addresses than Phytec?
>
> doc/board/phytec/phycore-am64x.rst:
>
>fatload mmc 1 ${loadaddr} tiboot3.bin
>sf update $loadaddr 0x0 $filesize
>fatload mmc 1 ${loadaddr} tispl.bin
>sf update $loadaddr 0x8 $filesize
>fatload mmc 1 ${loadaddr} u-boot.img
>sf update $loadaddr 0x28 $filesize
>
This patch has the correct offsets for am64x, so I guess phycore defined
the offsets differently.
> Please, add documentation in doc/board/ for all boards in the series
> specifying the location of the firmware in flash memory and how to
> update it(cf. section OSPI in doc/board/ti/am65x_evm.rst).
>
Yes, I will add patches to document OSPI layouts where missing.
>> +.num_images = ARRAY_SIZE(fw_images),
>> +.images = fw_images,
>> +};
>
> The series has been assigned to me in Patchwork.
>
> But changes should be tested and reviewed by the respective board
> maintainers (Vignesh, Tom for AM64x) as I have no access to TI boards.
>
> Best regards
>
> Heinrich
>
>> +
>> +void set_dfu_alt_info(char *interface, char *devstr)
>> +{
>> +env_set("dfu_alt_info", update_info.dfu_string);
>> +}
>> +
>> +#endif /* EFI_HAVE_CAPSULE_SUPPORT */
>> +
>> int board_init(void)
>> {
>> return 0;
Re: [PATCH v2 01/16] board: Define GUIDs for firmware images
Heinrich Schuchardt writes: > On 4/9/24 00:31, Jonathan Humphreys wrote: >> Define GUIDs for the different firmware images (tiboot3.bin, tispl.bin, >> u-boot.img, sysfw). > >> Signed-off-by: Jonathan Humphreys >> --- >> include/configs/ti_armv7_common.h | 17 + >> 1 file changed, 17 insertions(+) >> >> diff --git a/include/configs/ti_armv7_common.h >> b/include/configs/ti_armv7_common.h >> index 3def7b1027e..4ce14a9b84c 100644 >> --- a/include/configs/ti_armv7_common.h >> +++ b/include/configs/ti_armv7_common.h >> @@ -16,6 +16,23 @@ >> #ifndef __CONFIG_TI_ARMV7_COMMON_H__ >> #define __CONFIG_TI_ARMV7_COMMON_H__ >> >> +/* GUIDs for capsule updatable firmware images */ > > Please, provide code comments for the GUIDs, e.g. > > /** > * define K3_TIBOOT3_IMAGE_GUID - firmware GUID for K3 tiboot3.bin > * > * This GUID is used in capsules updates to identify the tiboot3.bin > * binary. > */ > > Cf. > https://docs.kernel.org/doc-guide/kernel-doc.html#object-like-macro-documentation > > Best regards > > Heinrich > Heinrich, thanks for reviewing! I modelled the GUID macros after how other boards and even core code defined there's. (eg, include/configs/kontron-sl-mx8mm.h or include/efi_api.h). However, if this is the new direction, I will format as you suggest. Please confirm. thanks Jon >> +#define K3_TIBOOT3_IMAGE_GUID \ >> +EFI_GUID(0xe672b518, 0x7cd7, 0x4014, 0xbd, 0x8d, \ >> + 0x40, 0x72, 0x4d, 0x0a, 0xd4, 0xdc) >> + >> +#define K3_SPL_IMAGE_GUID \ >> +EFI_GUID(0x86f710ad, 0x10cf, 0x46ea, 0xac, 0x67, \ >> + 0x85, 0x6a, 0xe0, 0x6e, 0xfa, 0xd2) >> + >> +#define K3_UBOOT_IMAGE_GUID \ >> +EFI_GUID(0x81b58fb0, 0x3b00, 0x4add, 0xa2, 0x0a, \ >> + 0xc1, 0x85, 0xbb, 0xac, 0xa1, 0xed) >> + >> +#define K3_SYSFW_IMAGE_GUID \ >> +EFI_GUID(0x6fd10680, 0x361b, 0x431f, 0x80, 0xaa, \ >> + 0x89, 0x94, 0x55, 0x81, 0x9e, 0x11) >> + >> /* >>* We setup defaults based on constraints from the Linux kernel, which >> should >>* also be safe elsewhere. We have the default load at 32MB into DDR (for
Re: [PATCH 1/4] mtd: spi-nor-core: Do not start or end writes at odd address in DTR mode
Manorit Chawdhry writes:
> From: Pratyush Yadav
>
> On DTR capable flashes like Micron Xcella the writes cannot start or end
> at an odd address in DTR mode. Extra 0xff bytes need to be prepended or
> appended respectively to make sure both the start and end addresses are
> even.
>
> Signed-off-by: Pratyush Yadav
> Reviewed-by: Vignesh Raghavendra
> Signed-off-by: Apurva Nandan
> Signed-off-by: Vignesh Raghavendra
> Signed-off-by: Manorit Chawdhry
> ---
> drivers/mtd/spi/spi-nor-core.c | 59
> +++---
> 1 file changed, 55 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index f86003ca8c06..2b000151c97d 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -1805,11 +1805,62 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t
> to, size_t len,
> if (ret < 0)
> return ret;
> #endif
> +
> write_enable(nor);
> - ret = nor->write(nor, addr, page_remain, buf + i);
> - if (ret < 0)
> - goto write_err;
> - written = ret;
> +
> + /*
> + * On DTR capable flashes like Micron Xcella the writes cannot
> + * start or end at an odd address in DTR mode. So we need to
> + * append or prepend extra 0xff bytes to make sure the start
> + * address and end address are even.
> + */
> + if (spi_nor_protocol_is_dtr(nor->write_proto) &&
> + ((addr | page_remain) & 1)) {
> + u_char *tmp;
> + size_t extra_bytes = 0;
> +
> + tmp = kmalloc(nor->page_size, 0);
> + if (!tmp) {
> + ret = -ENOMEM;
> + goto write_err;
> + }
> +
> + /* Prepend a 0xff byte if the start address is odd. */
> + if (addr & 1) {
> + tmp[0] = 0xff;
> + memcpy(tmp + 1, buf + i, page_remain);
> + addr--;
> + page_remain++;
> + extra_bytes++;
> + } else {
> + memcpy(tmp, buf + i, page_remain);
> + }
> +
> + /* Append a 0xff byte if the end address is odd. */
> + if ((addr + page_remain) & 1) {
> + tmp[page_remain + extra_bytes] = 0xff;
> + extra_bytes++;
> + page_remain++;
> + }
> +
> + ret = nor->write(nor, addr, page_remain, tmp);
> +
> + kfree(tmp);
> +
> + if (ret < 0)
> + goto write_err;
> +
> + /*
> + * We write extra bytes but they are not part of the
> + * original write.
> + */
> + written = ret - extra_bytes;
> + } else {
> + ret = nor->write(nor, addr, page_remain, buf + i);
> + if (ret < 0)
> + goto write_err;
> + written = ret;
> + }
>
> ret = spi_nor_wait_till_ready(nor);
> if (ret)
>
> --
> 2.43.2
Thanks for upstreaming!
Tested-by: Jonathan Humphreys
Re: [RFC] Makefile.lib: find capsule ESL dtsi file when CONFIG_OF_UPSTREAM=y
Sughosh Ganu writes:
> On Thu, 28 Mar 2024 at 09:34, Jon Humphreys wrote:
>>
>> Sughosh Ganu writes:
>>
>> > hi Jonathan,
>> >
>> > On Wed, 27 Mar 2024 at 08:05, Jonathan Humphreys
>> > wrote:
>> >>
>> >> When CONFIG_OF_UPSTREAM is enabled, DTS files are in SOC subdirectories
>> >> (vs the
>> >> top level dts directory), but when CONFIG_EFI_CAPSULE_AUTHENTICATE is
>> >> enabled,
>> >> the dynamically created dtsi file containing the capsule ESL DT node is
>> >> in the
>> >> parent directory. This results in a build failure because the #include
>> >> inserted
>> >> in the DTS file is local to the current directory. Update Makefile to
>> >> have the
>> >> DT preprocessing of #includes search in the parent (dts top level)
>> >> directory
>> >> too.
>> >>
>> >> I'm not sure if this is the best solution.
>> >>
>> >> I was also tempted to just manually include the capsule-key property in
>> >> the
>> >> board dts, and avoid the Makefile implicit inclusion trickery. I would
>> >> actually
>> >> prefer this approach as everything is more explicit. But this isn't an
>> >> option
>> >> because if CONFIG_EFI_CAPSULE_AUTHENTICATE is enabled, the implicit
>> >> inclusion of
>> >> the dtsi file happens. It would be better, IMO, if we only included the
>> >> generated dtsi file if CONFIG_EFI_CAPSULE_ESL_FILE is defined. Was only
>> >> supporting the implicit inclusiong approach an intentional design choice?
>> >
>> > I was not sure if users would want to manually insert the contents of
>> > the ESL file, which is a binary file(a few hundred bytes at least)
>> > into the DTS. If you prefer having such an option, we can change the
>> > logic to what you propose. Thanks.
>>
>> What I was thinking is that one would explictly add the capsule-key
>> property to the board dts but do it just as the generated dtsi does
>> where it references the .esl file.
>
> Okay. In any case, that can be done by having the addition of the
> capsule_esl_dtsi to the dtsi_include_list under the
> CONFIG_EFI_CAPSULE_ESL_FILE conditional, isn't it? That way, we have
> both solutions. Users can then define the config flag to automate the
> inclusion of the capsule-key. Those who prefer to explicitly include
> the node to the board dts can do so on similar lines to how it is
> defined in the lib/efi_loader/capsule_esl.dtsi.in file.
>
exactly!
Jon
> -sughosh
>
>>
>>
>> Jon
>>
>> >
>> > -sughosh
>> >
>> >>
>> >> Thanks
>> >> Jon
>> >>
>> >> Signed-off-by: Jonathan Humphreys
>> >> ---
>> >> scripts/Makefile.lib | 2 +-
>> >> 1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
>> >> index 12857316c58..62f87517c09 100644
>> >> --- a/scripts/Makefile.lib
>> >> +++ b/scripts/Makefile.lib
>> >> @@ -334,7 +334,7 @@ cmd_dtc = mkdir -p $(dir ${dtc-tmp}) ; \
>> >> (cat $< > $(pre-tmp)); \
>> >> $(foreach f,$(subst $(quote),,$(dtsi_include_list)), \
>> >> echo '$(pound)include "$(f)"' >> $(pre-tmp);) \
>> >> - $(HOSTCC) -E $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp)
>> >> $(pre-tmp) ; \
>> >> + $(HOSTCC) -E $(dtc_cpp_flags) -I$(obj) -x assembler-with-cpp -o
>> >> $(dtc-tmp) $(pre-tmp) ; \
>> >> $(DTC) -O dtb -o $@ -b 0 \
>> >> -i $(dir $<) -i $(u_boot_dtsi_loc) $(DTC_FLAGS) \
>> >> -d $(depfile).dtc.tmp $(dtc-tmp) || \
>> >> --
>> >> 2.34.1
>> >>
Re: [RFC] Makefile.lib: find capsule ESL dtsi file when CONFIG_OF_UPSTREAM=y
Sughosh Ganu writes:
> hi Jonathan,
>
> On Wed, 27 Mar 2024 at 08:05, Jonathan Humphreys wrote:
>>
>> When CONFIG_OF_UPSTREAM is enabled, DTS files are in SOC subdirectories (vs
>> the
>> top level dts directory), but when CONFIG_EFI_CAPSULE_AUTHENTICATE is
>> enabled,
>> the dynamically created dtsi file containing the capsule ESL DT node is in
>> the
>> parent directory. This results in a build failure because the #include
>> inserted
>> in the DTS file is local to the current directory. Update Makefile to have
>> the
>> DT preprocessing of #includes search in the parent (dts top level) directory
>> too.
>>
>> I'm not sure if this is the best solution.
>>
>> I was also tempted to just manually include the capsule-key property in the
>> board dts, and avoid the Makefile implicit inclusion trickery. I would
>> actually
>> prefer this approach as everything is more explicit. But this isn't an
>> option
>> because if CONFIG_EFI_CAPSULE_AUTHENTICATE is enabled, the implicit
>> inclusion of
>> the dtsi file happens. It would be better, IMO, if we only included the
>> generated dtsi file if CONFIG_EFI_CAPSULE_ESL_FILE is defined. Was only
>> supporting the implicit inclusiong approach an intentional design choice?
>
> I was not sure if users would want to manually insert the contents of
> the ESL file, which is a binary file(a few hundred bytes at least)
> into the DTS. If you prefer having such an option, we can change the
> logic to what you propose. Thanks.
What I was thinking is that one would explictly add the capsule-key
property to the board dts but do it just as the generated dtsi does
where it references the .esl file.
Jon
>
> -sughosh
>
>>
>> Thanks
>> Jon
>>
>> Signed-off-by: Jonathan Humphreys
>> ---
>> scripts/Makefile.lib | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
>> index 12857316c58..62f87517c09 100644
>> --- a/scripts/Makefile.lib
>> +++ b/scripts/Makefile.lib
>> @@ -334,7 +334,7 @@ cmd_dtc = mkdir -p $(dir ${dtc-tmp}) ; \
>> (cat $< > $(pre-tmp)); \
>> $(foreach f,$(subst $(quote),,$(dtsi_include_list)), \
>> echo '$(pound)include "$(f)"' >> $(pre-tmp);) \
>> - $(HOSTCC) -E $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp)
>> $(pre-tmp) ; \
>> + $(HOSTCC) -E $(dtc_cpp_flags) -I$(obj) -x assembler-with-cpp -o
>> $(dtc-tmp) $(pre-tmp) ; \
>> $(DTC) -O dtb -o $@ -b 0 \
>> -i $(dir $<) -i $(u_boot_dtsi_loc) $(DTC_FLAGS) \
>> -d $(depfile).dtc.tmp $(dtc-tmp) || \
>> --
>> 2.34.1
>>
Re: [PATCH v2 2/2] arm: dts: k3-am642-evm/sk: Enable OSPI support in SPL
Tom Rini writes:
> On Sat, Feb 24, 2024 at 11:34:36AM -0600, Jon Humphreys wrote:
>> Tom Rini writes:
>>
>> > On Fri, Feb 23, 2024 at 06:17:02PM -0600, Jonathan Humphreys wrote:
>> >> Add bootph DT tags to enable OSPI in SPL.
>> >> Set OSPI regs for R5 SPL to address OSPI's boot region.
>> >>
>> >> Signed-off-by: Jonathan Humphreys
>> >> ---
>> >> arch/arm/dts/k3-am642-evm-u-boot.dtsi | 16
>> >> arch/arm/dts/k3-am642-r5-evm.dts | 5 +
>> >> arch/arm/dts/k3-am642-r5-sk.dts | 5 +
>> >> arch/arm/dts/k3-am642-sk-u-boot.dtsi | 16
>> >> 4 files changed, 42 insertions(+)
>> >>
>> >> diff --git a/arch/arm/dts/k3-am642-evm-u-boot.dtsi
>> >> b/arch/arm/dts/k3-am642-evm-u-boot.dtsi
>> >> index b843078243..60b219c0be 100644
>> >> --- a/arch/arm/dts/k3-am642-evm-u-boot.dtsi
>> >> +++ b/arch/arm/dts/k3-am642-evm-u-boot.dtsi
>> >> @@ -182,3 +182,19 @@
>> >> &cpsw_port2 {
>> >> status = "disabled";
>> >> };
>> >> +
>> >> +&ospi0_pins_default {
>> >> + bootph-all;
>> >> +};
>> >> +
>> >> +&fss {
>> >> + bootph-all;
>> >> +};
>> >> +
>> >> +&ospi0 {
>> >> + bootph-all;
>> >> +
>> >> + flash@0 {
>> >> + bootph-all;
>> >> + };
>> >> +};
>> >
>> > So this gets back to what I was asking in the first series, is this
>> > needed in SPL or full U-Boot as well? The bootph-* properties are
>> > supposed to be transitive, but originally the tooling didn't handle this
>> > and now the tooling handles SPL but not full U-Boot. Which also brings
>> > back the is this _needed_ question and is bootph-all right, rather than
>> > just the big hammer?
>> >
>> By "this", are you referring to the original phypattern partition nodes,
>> or the ospi0 node itself? The partition nodes are not needed at all, so
>> removed. The ospi node is needed in both SPL and U-Boot. In that case,
>> using the bootph-all tag is the proper way, correct?
>>
>> What do you mean by the 'big hammer'?
>>
>> Please advise and thanks.
>
> So, part of the answer is that the documentation isn't as clear and well
> formatted as I'd like (aside, include/dm/ofnode.h::ofnode_pre_reloc
> comment should be reworded to render better). First, I want to point to
> the schema itself:
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/bootph.yaml
> and then next:
> https://docs.u-boot.org/en/latest/develop/driver-model/design.html#pre-relocation-support
>
> And in this case, the "pre" options are a bit less clear as TI platforms
> don't do the TPL->SPL->Full U-Boot dance that others like say Rockchip
> do but instead the Cortex-R/Cortex-A dance for the K3 architecture.
>
> Which gets back to what I was trying to ask. What, functionally,
> requires that property to be present? And then, for the cortex-a
> platforms these should be in the upstream dtb and I forget if you said
> that's in progress for these platforms or not.
Without those properties, the on-board OSPI flash is not available to
u-boot.
There is a separate action to move the bootph properities (not just the
above) to the upstream DT.
>
> --
> Tom
Re: [PATCH v2 3/3] arm: dts: k3-j721e-sk: Remove OSPI phypattern partition
"Kumar, Udit" writes:
> Thanks Jon
>
> On 2/24/2024 5:53 AM, Jonathan Humphreys wrote:
>> The phy calibration pattern partition isn't needed as the Cadence driver
>> isn't
>> calibrating the phys.
>
>
> Please do mention Fixes tag here
>
> 58d61fb5a77e ("arm: dts: k3-j721e-sk: Add initial A72 specific dts support")
>
I put the fixes tag on the cover letter. Is there a 'correct' way - to
add to the cover letter or the individual patches?
In the case of this particular patch, I don't think the fixes tag is
appropriate as this isn't changing any observable behavior, it is a
clean up.
Jon
>
>> Signed-off-by: Jonathan Humphreys
>> ---
>> arch/arm/dts/k3-j721e-sk-u-boot.dtsi | 4
>> 1 file changed, 4 deletions(-)
>>
>> diff --git a/arch/arm/dts/k3-j721e-sk-u-boot.dtsi
>> b/arch/arm/dts/k3-j721e-sk-u-boot.dtsi
>> index 479b7bcd6f..20b76a84ff 100644
>> --- a/arch/arm/dts/k3-j721e-sk-u-boot.dtsi
>> +++ b/arch/arm/dts/k3-j721e-sk-u-boot.dtsi
>> @@ -157,9 +157,5 @@
>>
>> flash@0 {
>> bootph-all;
>> -
>> -partition@3fc {
>> -bootph-all;
>> -};
>> };
>
>
> This Patch LTGM,
>
>
> + Neha
>
> For further cleanup, we should not have these ospi nodes in u-boot dts file.
>
> should be in board dts (k3-j721e-sk.dts) with right bootph properties ,
>
> and comes via kernel DT.
>
>
>
>> };
Re: [PATCH v2 2/2] arm: dts: k3-am642-evm/sk: Enable OSPI support in SPL
Tom Rini writes:
> On Fri, Feb 23, 2024 at 06:17:02PM -0600, Jonathan Humphreys wrote:
>> Add bootph DT tags to enable OSPI in SPL.
>> Set OSPI regs for R5 SPL to address OSPI's boot region.
>>
>> Signed-off-by: Jonathan Humphreys
>> ---
>> arch/arm/dts/k3-am642-evm-u-boot.dtsi | 16
>> arch/arm/dts/k3-am642-r5-evm.dts | 5 +
>> arch/arm/dts/k3-am642-r5-sk.dts | 5 +
>> arch/arm/dts/k3-am642-sk-u-boot.dtsi | 16
>> 4 files changed, 42 insertions(+)
>>
>> diff --git a/arch/arm/dts/k3-am642-evm-u-boot.dtsi
>> b/arch/arm/dts/k3-am642-evm-u-boot.dtsi
>> index b843078243..60b219c0be 100644
>> --- a/arch/arm/dts/k3-am642-evm-u-boot.dtsi
>> +++ b/arch/arm/dts/k3-am642-evm-u-boot.dtsi
>> @@ -182,3 +182,19 @@
>> &cpsw_port2 {
>> status = "disabled";
>> };
>> +
>> +&ospi0_pins_default {
>> +bootph-all;
>> +};
>> +
>> +&fss {
>> +bootph-all;
>> +};
>> +
>> +&ospi0 {
>> +bootph-all;
>> +
>> +flash@0 {
>> +bootph-all;
>> +};
>> +};
>
> So this gets back to what I was asking in the first series, is this
> needed in SPL or full U-Boot as well? The bootph-* properties are
> supposed to be transitive, but originally the tooling didn't handle this
> and now the tooling handles SPL but not full U-Boot. Which also brings
> back the is this _needed_ question and is bootph-all right, rather than
> just the big hammer?
>
By "this", are you referring to the original phypattern partition nodes,
or the ospi0 node itself? The partition nodes are not needed at all, so
removed. The ospi node is needed in both SPL and U-Boot. In that case,
using the bootph-all tag is the proper way, correct?
What do you mean by the 'big hammer'?
Please advise and thanks.
> --
> Tom
Re: [PATCH 2/2] arm: dts: k3-am642-evm/sk: Enable OSPI support in SPL
Tom Rini writes:
> On Thu, Feb 22, 2024 at 11:24:38PM -0600, Jonathan Humphreys wrote:
>> Add bootph DT tags to enable OSPI in SPL.
>> Set OSPI regs for R5 SPL to address OSPI's boot region.
>>
>> Signed-off-by: Jonathan Humphreys
>> ---
>> arch/arm/dts/k3-am642-evm-u-boot.dtsi | 24
>> arch/arm/dts/k3-am642-r5-evm.dts | 5 +
>> arch/arm/dts/k3-am642-r5-sk.dts | 5 +
>> arch/arm/dts/k3-am642-sk-u-boot.dtsi | 24
>> 4 files changed, 58 insertions(+)
>>
>> diff --git a/arch/arm/dts/k3-am642-evm-u-boot.dtsi
>> b/arch/arm/dts/k3-am642-evm-u-boot.dtsi
>> index b843078243..f7ab9f9a30 100644
>> --- a/arch/arm/dts/k3-am642-evm-u-boot.dtsi
>> +++ b/arch/arm/dts/k3-am642-evm-u-boot.dtsi
>> @@ -182,3 +182,27 @@
>> &cpsw_port2 {
>> status = "disabled";
>> };
>> +
>> +&ospi0_pins_default {
>> +bootph-all;
>> +};
>> +
>> +&fss {
>> +bootph-all;
>> +};
>> +
>> +&ospi0 {
>> +bootph-all;
>> +
>> +flash@0 {
>> +bootph-all;
>> +
>> +partitions {
>> +bootph-all;
>> +
>> +partition@3fc {
>> +bootph-all;
>> +};
>> +};
>> +};
>> +};
>
> Do we really need these tags on the partition table? And if so, do we
> need them in main U-Boot, or just SPL?
>
Tom, these are the OSPI phy calibration patterns. I added them as they
are in the J721e u-boot.dtsi and both use the same OSPI interface.
However, I dug into this more and the upstream Cadence OSPI driver isn't
calibrating the phys.
I will respin this series to remove the .dtsi nodes, and add a patch to
the J721e series to remove it from there as well.
Thanks for catching.
> --
> Tom
Re: [PATCH V2 10/10] include: env: ti: Drop default_findfdt
Nishanth Menon writes:
> We shouldn't need finfdt anymore. Drop the env script.
>
> Signed-off-by: Nishanth Menon
> ---
> Changes from V1: None.
>
> V1: https://lore.kernel.org/r/20240108173301.2692332-11...@ti.com
> include/env/ti/default_findfdt.env | 12
> 1 file changed, 12 deletions(-)
> delete mode 100644 include/env/ti/default_findfdt.env
>
> diff --git a/include/env/ti/default_findfdt.env
> b/include/env/ti/default_findfdt.env
> deleted file mode 100644
> index a2b51dd923bb..
> --- a/include/env/ti/default_findfdt.env
> +++ /dev/null
> @@ -1,12 +0,0 @@
> -default_device_tree=CONFIG_DEFAULT_DEVICE_TREE
> -default_device_tree_arch=ti
> -#ifdef CONFIG_ARM64
> -findfdt=
> - setenv name_fdt ${default_device_tree_arch}/${default_device_tree}.dtb;
> - setenv fdtfile ${name_fdt}
> -#else
> -default_device_tree_subarch=omap
> -findfdt=
> - setenv name_fdt
> ${default_device_tree_arch}/${default_device_tree_subarch}/${default_device_tree}.dtb;
> - setenv fdtfile ${name_fdt}
> -#endif
> --
> 2.43.0
Reviewed-by: Jonathan Humphreys
Re: [PATCH V2 09/10] board: beagle: beagleplay: Set fdtfile from C code instead of findfdt script
Nishanth Menon writes:
> Stop using the findfdt script and switch to setting the fdtfile from C
> code.
>
> Signed-off-by: Nishanth Menon
> ---
> Changes from V1:
> * Just macro name change s/TI_EVM_FDT_FOLDER_PATH/TI_FDT_FOLDER_PATH
> * Commit message update to drop the "warning added to findfdt" since
> that is not done.
>
> I have retained the explicit setting of fdtfile to remove dependency on
> TI evm specific logic. Also, the dynamic population of fdtfile instead
> of env_set("fdtfile", "ti/k3-am625-beagleplay.dtb") to allow for the
> upcoming board variants to be able to configure the dtb name via a
> config fragment.
>
> V1: https://lore.kernel.org/r/20240108173301.2692332-10...@ti.com
> board/beagle/beagleplay/beagleplay.c | 14 ++
> board/beagle/beagleplay/beagleplay.env | 1 -
> configs/am62x_beagleplay_a53_defconfig | 3 ++-
> 3 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/board/beagle/beagleplay/beagleplay.c
> b/board/beagle/beagleplay/beagleplay.c
> index 1c376dea372f..20819ecf45b4 100644
> --- a/board/beagle/beagleplay/beagleplay.c
> +++ b/board/beagle/beagleplay/beagleplay.c
> @@ -27,3 +27,17 @@ int dram_init_banksize(void)
> {
> return fdtdec_setup_memory_banksize();
> }
> +
> +#ifdef CONFIG_BOARD_LATE_INIT
> +int board_late_init(void)
> +{
> + char fdtfile[50];
> +
> + snprintf(fdtfile, sizeof(fdtfile), "%s/%s.dtb",
> + CONFIG_TI_FDT_FOLDER_PATH, CONFIG_DEFAULT_DEVICE_TREE);
> +
> + env_set("fdtfile", fdtfile);
> +
> + return 0;
> +}
> +#endif
> diff --git a/board/beagle/beagleplay/beagleplay.env
> b/board/beagle/beagleplay/beagleplay.env
> index 4f0a94a8113e..647b25d14c8e 100644
> --- a/board/beagle/beagleplay/beagleplay.env
> +++ b/board/beagle/beagleplay/beagleplay.env
> @@ -1,5 +1,4 @@
> #include
> -#include
> #include
>
> name_kern=Image
> diff --git a/configs/am62x_beagleplay_a53_defconfig
> b/configs/am62x_beagleplay_a53_defconfig
> index 0be20045a974..1f43891d10bb 100644
> --- a/configs/am62x_beagleplay_a53_defconfig
> +++ b/configs/am62x_beagleplay_a53_defconfig
> @@ -33,7 +33,8 @@ CONFIG_AUTOBOOT_KEYED=y
> CONFIG_AUTOBOOT_PROMPT="Press SPACE to abort autoboot in %d seconds\n"
> CONFIG_AUTOBOOT_DELAY_STR="d"
> CONFIG_AUTOBOOT_STOP_STR=" "
> -CONFIG_BOOTCOMMAND="run set_led_state_start_load;run findfdt; run envboot;
> bootflow scan -lb;run set_led_state_fail_load"
> +CONFIG_BOOTCOMMAND="run set_led_state_start_load; run envboot; bootflow scan
> -lb;run set_led_state_fail_load"
> +CONFIG_BOARD_LATE_INIT=y
> CONFIG_SPL_MAX_SIZE=0x58000
> CONFIG_SPL_HAS_BSS_LINKER_SECTION=y
> CONFIG_SPL_BSS_START_ADDR=0x80c8
> --
> 2.43.0
Reviewed-by: Jonathan Humphreys
Re: [PATCH V2 08/10] board: beagle: beagleboneai64: Set fdtfile from C code instead of findfdt script
Nishanth Menon writes:
> Stop using the findfdt script and switch to setting the fdtfile from C
> code.
>
> Signed-off-by: Nishanth Menon
> ---
> Changes from V1:
> * Just macro name change s/TI_EVM_FDT_FOLDER_PATH/TI_FDT_FOLDER_PATH
> * Commit message update to drop the "warning added to findfdt" since
> that is not done.
>
> I have retained the explicit setting of fdtfile to remove dependency on
> TI evm specific logic. Also, the dynamic population of fdtfile instead
> of env_set("fdtfile", "ti/k3-j721e-beagleboneai64.dtb") to allow for the
> upcoming board variants to be able to configure the dtb name via a
> config fragment.
>
> V1: https://lore.kernel.org/r/20240108173301.2692332-9...@ti.com
> board/beagle/beagleboneai64/beagleboneai64.c | 14 ++
> board/beagle/beagleboneai64/beagleboneai64.env | 1 -
> configs/j721e_beagleboneai64_a72_defconfig | 3 ++-
> 3 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/board/beagle/beagleboneai64/beagleboneai64.c
> b/board/beagle/beagleboneai64/beagleboneai64.c
> index c8c1c78ae5a2..c5b4ff7df47a 100644
> --- a/board/beagle/beagleboneai64/beagleboneai64.c
> +++ b/board/beagle/beagleboneai64/beagleboneai64.c
> @@ -28,3 +28,17 @@ int dram_init_banksize(void)
> {
> return fdtdec_setup_memory_banksize();
> }
> +
> +#ifdef CONFIG_BOARD_LATE_INIT
> +int board_late_init(void)
> +{
> + char fdtfile[50];
> +
> + snprintf(fdtfile, sizeof(fdtfile), "%s/%s.dtb",
> + CONFIG_TI_FDT_FOLDER_PATH, CONFIG_DEFAULT_DEVICE_TREE);
> +
> + env_set("fdtfile", fdtfile);
> +
> + return 0;
> +}
> +#endif
> diff --git a/board/beagle/beagleboneai64/beagleboneai64.env
> b/board/beagle/beagleboneai64/beagleboneai64.env
> index 4f0a94a8113e..647b25d14c8e 100644
> --- a/board/beagle/beagleboneai64/beagleboneai64.env
> +++ b/board/beagle/beagleboneai64/beagleboneai64.env
> @@ -1,5 +1,4 @@
> #include
> -#include
> #include
>
> name_kern=Image
> diff --git a/configs/j721e_beagleboneai64_a72_defconfig
> b/configs/j721e_beagleboneai64_a72_defconfig
> index 959f86844d32..9e53658eacb9 100644
> --- a/configs/j721e_beagleboneai64_a72_defconfig
> +++ b/configs/j721e_beagleboneai64_a72_defconfig
> @@ -34,7 +34,8 @@ CONFIG_AUTOBOOT_PROMPT="Press SPACE to abort autoboot in %d
> seconds\n"
> CONFIG_AUTOBOOT_DELAY_STR="d"
> CONFIG_AUTOBOOT_STOP_STR=" "
> CONFIG_OF_SYSTEM_SETUP=y
> -CONFIG_BOOTCOMMAND="run set_led_state_start_load;run findfdt; run envboot;
> bootflow scan -lb;run set_led_state_fail_load"
> +CONFIG_BOOTCOMMAND="run set_led_state_start_load; run envboot; bootflow scan
> -lb;run set_led_state_fail_load"
> +CONFIG_BOARD_LATE_INIT=y
> CONFIG_LOGLEVEL=7
> CONFIG_SPL_MAX_SIZE=0xc
> CONFIG_SPL_HAS_BSS_LINKER_SECTION=y
> --
> 2.43.0
Reviewed-by: Jonathan Humphreys
Re: [PATCH V2 07/10] board: ti: j721s2: Set fdtfile from C code instead of findfdt script
Nishanth Menon writes:
> We now can provide a map and have the standard fdtfile variable set from
> code itself. This allows for bootstd to "just work".
>
> While at this, replace findfdt in environment with a warning as it is no
> longer needed.
>
> Signed-off-by: Nishanth Menon
> ---
> Changes from V1: None.
>
> V1: https://lore.kernel.org/r/20240108173301.2692332-8...@ti.com
> board/ti/j721s2/evm.c | 8
> board/ti/j721s2/j721s2.env | 8
> 2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/board/ti/j721s2/evm.c b/board/ti/j721s2/evm.c
> index 1220cd84519b..5a0281d6b483 100644
> --- a/board/ti/j721s2/evm.c
> +++ b/board/ti/j721s2/evm.c
> @@ -23,6 +23,7 @@
> #include
>
> #include "../common/board_detect.h"
> +#include "../common/fdt_ops.h"
>
> DECLARE_GLOBAL_DATA_PTR;
>
> @@ -114,6 +115,12 @@ int checkboard(void)
> return 0;
> }
>
> +static struct ti_fdt_map ti_j721s2_evm_fdt_map[] = {
> + {"j721s2", "k3-j721s2-common-proc-board.dtb"},
> + {"am68-sk", "k3-am68-sk-base-board.dtb"},
> + { /* Sentinel. */ }
> +};
> +
> static void setup_board_eeprom_env(void)
> {
> char *name = "j721s2";
> @@ -131,6 +138,7 @@ static void setup_board_eeprom_env(void)
>
> invalid_eeprom:
> set_board_info_env_am6(name);
> + ti_set_fdt_env(name, ti_j721s2_evm_fdt_map);
> }
>
> static void setup_serial(void)
> diff --git a/board/ti/j721s2/j721s2.env b/board/ti/j721s2/j721s2.env
> index 64e3d9da85f0..9a03b9f30aee 100644
> --- a/board/ti/j721s2/j721s2.env
> +++ b/board/ti/j721s2/j721s2.env
> @@ -7,14 +7,6 @@
> #include
> #endif
>
> -default_device_tree=ti/k3-j721s2-common-proc-board.dtb
> -findfdt=
> - setenv name_fdt ${default_device_tree};
> - if test $board_name = j721s2; then \
> - setenv name_fdt ti/k3-j721s2-common-proc-board.dtb; fi;
> - if test $board_name = am68-sk; then
> - setenv name_fdt ti/k3-am68-sk-base-board.dtb; fi;
> - setenv fdtfile ${name_fdt}
> name_kern=Image
> console=ttyS2,115200n8
> args_all=setenv optargs earlycon=ns16550a,mmio32,0x0288
> --
> 2.43.0
Reviewed-by: Jonathan Humphreys
Re: [PATCH V2 06/10] board: ti: j721e: Set fdtfile from C code instead of findfdt script
Nishanth Menon writes:
> We now can provide a map and have the standard fdtfile variable set from
> code itself. This allows for bootstd to "just work".
>
> While at this, replace findfdt in environment with a warning as it is no
> longer needed.
>
> Signed-off-by: Nishanth Menon
> ---
> Changes from V1: None.
>
> v1: https://lore.kernel.org/r/20240108173301.2692332-7...@ti.com
> board/ti/j721e/evm.c | 8
> board/ti/j721e/j721e.env | 10 --
> 2 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/board/ti/j721e/evm.c b/board/ti/j721e/evm.c
> index c541880107ec..ad6ef4553e04 100644
> --- a/board/ti/j721e/evm.c
> +++ b/board/ti/j721e/evm.c
> @@ -16,6 +16,7 @@
> #include
>
> #include "../common/board_detect.h"
> +#include "../common/fdt_ops.h"
>
> #define board_is_j721e_som() (board_ti_k3_is("J721EX-PM1-SOM") || \
>board_ti_k3_is("J721EX-PM2-SOM"))
> @@ -424,6 +425,12 @@ void configure_serdes_sierra(void)
> }
>
> #ifdef CONFIG_BOARD_LATE_INIT
> +static struct ti_fdt_map ti_j721e_evm_fdt_map[] = {
> + {"j721e", "k3-j721e-common-proc-board.dtb"},
> + {"j721e-sk", "k3-j721e-sk.dtb"},
> + {"j7200", "k3-j7200-common-proc-board.dtb"},
> + { /* Sentinel. */ }
> +};
> static void setup_board_eeprom_env(void)
> {
> char *name = "j721e";
> @@ -443,6 +450,7 @@ static void setup_board_eeprom_env(void)
>
> invalid_eeprom:
> set_board_info_env_am6(name);
> + ti_set_fdt_env(name, ti_j721e_evm_fdt_map);
> }
>
> static void setup_serial(void)
> diff --git a/board/ti/j721e/j721e.env b/board/ti/j721e/j721e.env
> index cb27bf5e2b24..38bfd7d49634 100644
> --- a/board/ti/j721e/j721e.env
> +++ b/board/ti/j721e/j721e.env
> @@ -7,16 +7,6 @@
> #include
> #endif
>
> -default_device_tree=ti/k3-j721e-common-proc-board.dtb
> -findfdt=
> - setenv name_fdt ${default_device_tree};
> - if test $board_name = j721e; then
> - setenv name_fdt ti/k3-j721e-common-proc-board.dtb; fi;
> - if test $board_name = j7200; then
> - setenv name_fdt ti/k3-j7200-common-proc-board.dtb; fi;
> - if test $board_name = j721e-eaik || test $board_name = j721e-sk; then
> - setenv name_fdt ti/k3-j721e-sk.dtb; fi;
> - setenv fdtfile ${name_fdt}
> name_kern=Image
> console=ttyS2,115200n8
> args_all=setenv optargs earlycon=ns16550a,mmio32,0x0280
> --
> 2.43.0
Reviewed-by: Jonathan Humphreys
Re: [PATCH V2 05/10] board: ti: am65x: Set fdtfile from C code instead of findfdt script
Nishanth Menon writes:
> We now can provide a map and have the standard fdtfile variable set from
> code itself. This allows for bootstd to "just work".
>
> While at this, replace findfdt in environment with a warning as it is no
> longer needed.
>
> Signed-off-by: Nishanth Menon
> ---
> Changes from V1: None.
> I have retained the central call ti_set_fdt_env() to retain the
> population of fdtfile name using the CONFIG fall back logic and the
> population of (now deprecated) legacy variables for downstream script
> users.
>
> V1: https://lore.kernel.org/r/20240108173301.2692332-6...@ti.com
> board/ti/am65x/am65x.env | 3 ---
> board/ti/am65x/evm.c | 2 ++
> 2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/board/ti/am65x/am65x.env b/board/ti/am65x/am65x.env
> index 286b9c300c05..814374d68cf0 100644
> --- a/board/ti/am65x/am65x.env
> +++ b/board/ti/am65x/am65x.env
> @@ -5,9 +5,6 @@
> #include
> #endif
>
> -findfdt=
> - setenv name_fdt ti/k3-am654-base-board.dtb;
> - setenv fdtfile ${name_fdt}
> name_kern=Image
> console=ttyS2,115200n8
> args_all=setenv optargs ${optargs} earlycon=ns16550a,mmio32,0x0280
> diff --git a/board/ti/am65x/evm.c b/board/ti/am65x/evm.c
> index df209021c1b7..3109c9a2acac 100644
> --- a/board/ti/am65x/evm.c
> +++ b/board/ti/am65x/evm.c
> @@ -22,6 +22,7 @@
> #include
>
> #include "../common/board_detect.h"
> +#include "../common/fdt_ops.h"
>
> #define board_is_am65x_base_board() board_ti_is("AM6-COMPROCEVM")
>
> @@ -141,6 +142,7 @@ static void setup_board_eeprom_env(void)
>
> invalid_eeprom:
> set_board_info_env_am6(name);
> + ti_set_fdt_env(NULL, NULL);
> }
>
> static int init_daughtercard_det_gpio(char *gpio_name, struct gpio_desc
> *desc)
> --
> 2.43.0
Reviewed-by: Jonathan Humphreys
Re: [PATCH V2 04/10] board: ti: am64x: Set fdtfile from C code instead of findfdt script
Nishanth Menon writes:
> We now can provide a map and have the standard fdtfile variable set from
> code itself. This allows for bootstd to "just work".
>
> While at this, replace findfdt in environment with a warning as it is no
> longer needed.
>
> Signed-off-by: Nishanth Menon
> ---
> Changes from V1: None.
> I have retained the the existing names used in the file as is for now, a
> cleanup as suggested in review is probably something to do in a follow
> on series.
>
> V1: https://lore.kernel.org/r/20240108173301.2692332-5...@ti.com
>
> board/ti/am64x/am64x.env | 9 -
> board/ti/am64x/evm.c | 8
> 2 files changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/board/ti/am64x/am64x.env b/board/ti/am64x/am64x.env
> index efd736b99be4..9a8812d4ee54 100644
> --- a/board/ti/am64x/am64x.env
> +++ b/board/ti/am64x/am64x.env
> @@ -2,14 +2,6 @@
> #include
> #include
>
> -findfdt=
> - if test $board_name = am64x_gpevm; then
> - setenv name_fdt ti/k3-am642-evm.dtb; fi;
> - if test $board_name = am64x_skevm; then
> - setenv name_fdt ti/k3-am642-sk.dtb; fi;
> - if test $name_fdt = undefined; then
> - echo WARNING: Could not determine device tree to use; fi;
> - setenv fdtfile ${name_fdt}
> name_kern=Image
> console=ttyS2,115200n8
> args_all=setenv optargs earlycon=ns16550a,mmio32,0x0280 ${mtdparts}
> @@ -43,7 +35,6 @@ get_fit_usb=load usb ${bootpart} ${addr_fit}
> usbboot=setenv boot usb;
> setenv bootpart 0:2;
> usb start;
> - run findfdt;
> run init_usb;
> run get_kern_usb;
> run get_fdt_usb;
> diff --git a/board/ti/am64x/evm.c b/board/ti/am64x/evm.c
> index a6dcff2eb434..e2f506d2c6ea 100644
> --- a/board/ti/am64x/evm.c
> +++ b/board/ti/am64x/evm.c
> @@ -16,6 +16,7 @@
> #include
>
> #include "../common/board_detect.h"
> +#include "../common/fdt_ops.h"
>
> #define board_is_am64x_gpevm() (board_ti_k3_is("AM64-GPEVM") || \
> board_ti_k3_is("AM64-HSEVM"))
> @@ -180,6 +181,12 @@ int checkboard(void)
> }
>
> #ifdef CONFIG_BOARD_LATE_INIT
> +static struct ti_fdt_map ti_am64_evm_fdt_map[] = {
> + {"am64x_gpevm", "k3-am642-evm.dtb"},
> + {"am64x_skevm", "k3-am642-sk.dtb"},
> + { /* Sentinel. */ }
> +};
> +
> static void setup_board_eeprom_env(void)
> {
> char *name = "am64x_gpevm";
> @@ -197,6 +204,7 @@ static void setup_board_eeprom_env(void)
>
> invalid_eeprom:
> set_board_info_env_am6(name);
> + ti_set_fdt_env(name, ti_am64_evm_fdt_map);
> }
>
> static void setup_serial(void)
> --
> 2.43.0
Reviewed-by: Jonathan Humphreys
Re: [PATCH V2 03/10] board: ti: am62x: Set fdtfile from C code instead of findfdt script
Nishanth Menon writes:
> Stop using the findfdt script and switch to setting the fdtfile from
> C code.
>
> While at this, replace findfdt in environment with a warning as it is
> no longer needed
>
> Signed-off-by: Nishanth Menon
> ---
> Changes from V1: None.
> I have retained the central call ti_set_fdt_env() to retain the
> population of fdtfile name using the CONFIG fall back logic and the
> population of (now deprecated) legacy variables for downstream script
> users.
>
> V1: https://lore.kernel.org/r/20240108173301.2692332-4...@ti.com
> board/ti/am62x/am62x.env| 1 -
> board/ti/am62x/evm.c| 8
> configs/am62x_evm_a53_defconfig | 1 +
> 3 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/board/ti/am62x/am62x.env b/board/ti/am62x/am62x.env
> index e53a55c38fbb..9cb186c2a03c 100644
> --- a/board/ti/am62x/am62x.env
> +++ b/board/ti/am62x/am62x.env
> @@ -1,5 +1,4 @@
> #include
> -#include
> #include
>
> name_kern=Image
> diff --git a/board/ti/am62x/evm.c b/board/ti/am62x/evm.c
> index ad939088402e..b76ef1b94a61 100644
> --- a/board/ti/am62x/evm.c
> +++ b/board/ti/am62x/evm.c
> @@ -54,6 +54,14 @@ int dram_init(void)
> return fdtdec_setup_mem_size_base();
> }
>
> +#ifdef CONFIG_BOARD_LATE_INIT
> +int board_late_init(void)
> +{
> + ti_set_fdt_env(NULL, NULL);
> + return 0;
> +}
> +#endif
> +
> int dram_init_banksize(void)
> {
> return fdtdec_setup_memory_banksize();
> diff --git a/configs/am62x_evm_a53_defconfig b/configs/am62x_evm_a53_defconfig
> index aa96c1b3125c..3cf3b93a93fc 100644
> --- a/configs/am62x_evm_a53_defconfig
> +++ b/configs/am62x_evm_a53_defconfig
> @@ -32,6 +32,7 @@ CONFIG_BOOTSTD_FULL=y
> CONFIG_BOOTSTD_DEFAULTS=y
> CONFIG_SYS_BOOTM_LEN=0x80
> CONFIG_BOOTCOMMAND="run envboot; bootflow scan -lb"
> +CONFIG_BOARD_LATE_INIT=y
> CONFIG_SPL_MAX_SIZE=0x58000
> CONFIG_SPL_HAS_BSS_LINKER_SECTION=y
> CONFIG_SPL_BSS_START_ADDR=0x80c8
> --
> 2.43.0
Reviewed-by: Jonathan Humphreys
Re: [PATCH V2 02/10] board: ti: am62ax: Set fdtfile from C code instead of findfdt script
Nishanth Menon writes:
> Stop using the findfdt script and switch to setting the fdtfile from
> C code.
>
> While at this, replace findfdt in environment with a warning as it is
> no longer needed
>
> Signed-off-by: Nishanth Menon
> ---
> Changes from V1: None.
> I have retained the central call ti_set_fdt_env() to retain the
> population of fdtfile name using the CONFIG fall back logic and the
> population of (now deprecated) legacy variables for downstream script
> users.
>
> V1: https://lore.kernel.org/r/20240108173301.2692332-3...@ti.com
>
> board/ti/am62ax/am62ax.env | 1 -
> board/ti/am62ax/evm.c| 10 ++
> configs/am62ax_evm_a53_defconfig | 1 +
> 3 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/board/ti/am62ax/am62ax.env b/board/ti/am62ax/am62ax.env
> index a6d967e982d4..334374abb73e 100644
> --- a/board/ti/am62ax/am62ax.env
> +++ b/board/ti/am62ax/am62ax.env
> @@ -1,5 +1,4 @@
> #include
> -#include
> #include
>
> name_kern=Image
> diff --git a/board/ti/am62ax/evm.c b/board/ti/am62ax/evm.c
> index cd3360a43029..62d3664936e7 100644
> --- a/board/ti/am62ax/evm.c
> +++ b/board/ti/am62ax/evm.c
> @@ -13,6 +13,8 @@
> #include
> #include
>
> +#include "../common/fdt_ops.h"
> +
> int board_init(void)
> {
> return 0;
> @@ -27,3 +29,11 @@ int dram_init_banksize(void)
> {
> return fdtdec_setup_memory_banksize();
> }
> +
> +#ifdef CONFIG_BOARD_LATE_INIT
> +int board_late_init(void)
> +{
> + ti_set_fdt_env(NULL, NULL);
> + return 0;
> +}
> +#endif
> diff --git a/configs/am62ax_evm_a53_defconfig
> b/configs/am62ax_evm_a53_defconfig
> index 38083586a3ec..e5fcd8cc5b6f 100644
> --- a/configs/am62ax_evm_a53_defconfig
> +++ b/configs/am62ax_evm_a53_defconfig
> @@ -24,6 +24,7 @@ CONFIG_SPL_LOAD_FIT_ADDRESS=0x8100
> CONFIG_BOOTSTD_FULL=y
> CONFIG_BOOTSTD_DEFAULTS=y
> CONFIG_BOOTCOMMAND="run envboot; bootflow scan -lb"
> +CONFIG_BOARD_LATE_INIT=y
> CONFIG_SPL_MAX_SIZE=0x58000
> CONFIG_SPL_PAD_TO=0x0
> CONFIG_SPL_HAS_BSS_LINKER_SECTION=y
> --
> 2.43.0
Reviewed-by: Jonathan Humphreys
Re: [PATCH V2 01/10] board: ti: common: Introduce a common fdt ops library
Nishanth Menon writes:
> Introduce a common fdt operations library for basic device tree
> operations that are common between various boards.
>
> The first library to introduce here is the capability to set up
> fdtfile as a standard variable as part of board identification rather
> than depend on scripted ifdeffery.
>
> Signed-off-by: Nishanth Menon
> ---
> Changes Since v1:
> * s/TI_EVM_FDT_FOLDER_PATH/TI_FDT_FOLDER_PATH s/name_fdt/board_name
> s/TI_NAME_FDT_MAX/TI_BOARD_NAME_MAX
> * Comment updates in various places for review clarification.
> * Still maintain the fall back using CONFIG_DEFAULT_DEVICE_TREE for
> reasons explained in review comment response.
> * Added a specific u-boot version number for deprecation of legacy
> env variables.
>
> V1: https://lore.kernel.org/r/20240108173301.2692332-2...@ti.com
>
> board/ti/common/Kconfig | 12
> board/ti/common/Makefile | 1 +
> board/ti/common/fdt_ops.c | 64 +++
> board/ti/common/fdt_ops.h | 42 +
> 4 files changed, 119 insertions(+)
> create mode 100644 board/ti/common/fdt_ops.c
> create mode 100644 board/ti/common/fdt_ops.h
>
> diff --git a/board/ti/common/Kconfig b/board/ti/common/Kconfig
> index 49edd98014ab..de44e4de2115 100644
> --- a/board/ti/common/Kconfig
> +++ b/board/ti/common/Kconfig
> @@ -49,3 +49,15 @@ config TI_COMMON_CMD_OPTIONS
> imply CMD_SPI
> imply CMD_TIME
> imply CMD_USB if USB
> +
> +config TI_FDT_FOLDER_PATH
> + string "Location of Folder path where dtb is present"
> + default "ti/davinci" if ARCH_DAVINCI
> + default "ti/keystone" if ARCH_KEYSTONE
> + default "ti/omap" if ARCH_OMAP2PLUS
> + default "ti" if ARCH_K3
> + depends on ARCH_DAVINCI || ARCH_KEYSTONE || ARCH_OMAP2PLUS || ARCH_K3
> + help
> +Folder path for kernel device tree default.
> +This is used along with fdtfile path to locate the kernel
> +device tree blob.
> diff --git a/board/ti/common/Makefile b/board/ti/common/Makefile
> index 26bf12e2e6d5..5ac361ba7fcf 100644
> --- a/board/ti/common/Makefile
> +++ b/board/ti/common/Makefile
> @@ -3,3 +3,4 @@
>
> obj-${CONFIG_TI_I2C_BOARD_DETECT} += board_detect.o
> obj-${CONFIG_CMD_EXTENSION} += cape_detect.o
> +obj-${CONFIG_OF_LIBFDT} += fdt_ops.o
> diff --git a/board/ti/common/fdt_ops.c b/board/ti/common/fdt_ops.c
> new file mode 100644
> index ..eb917be9e0da
> --- /dev/null
> +++ b/board/ti/common/fdt_ops.c
> @@ -0,0 +1,64 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Library to support FDT file operations which are common
> + *
> + * Copyright (C) 2024 Texas Instruments Incorporated - https://www.ti.com/
> + */
> +
> +#include
> +#include
> +#include "fdt_ops.h"
> +
> +void ti_set_fdt_env(const char *board_name, struct ti_fdt_map *fdt_map)
> +{
> + char *fdt_file_name = NULL;
> + char fdtfile[TI_FDT_FILE_MAX];
> +
> + if (board_name) {
> + while (fdt_map) {
> + /* Check for NULL terminator in the list */
> + if (!fdt_map->board_name)
> + break;
> + if (!strncmp(fdt_map->board_name, board_name,
> TI_BOARD_NAME_MAX)) {
> + fdt_file_name = fdt_map->fdt_file_name;
> + break;
> + }
> + fdt_map++;
> + }
> + }
> +
> + /* match not found OR null board_name */
> + if (!fdt_file_name) {
> + /*
> + * Prioritize CONFIG_DEFAULT_FDT_FILE - if that is not defined,
> + * or is empty, then use CONFIG_DEFAULT_DEVICE_TREE
> + */
> +#ifdef CONFIG_DEFAULT_FDT_FILE
> + if (strlen(CONFIG_DEFAULT_FDT_FILE)) {
> + snprintf(fdtfile, sizeof(fdtfile), "%s/%s",
> + CONFIG_TI_FDT_FOLDER_PATH,
> CONFIG_DEFAULT_FDT_FILE);
> + } else
> +#endif
> + {
> + snprintf(fdtfile, sizeof(fdtfile), "%s/%s.dtb",
> + CONFIG_TI_FDT_FOLDER_PATH,
> CONFIG_DEFAULT_DEVICE_TREE);
> + }
> + } else {
> + snprintf(fdtfile, sizeof(fdtfile), "%s/%s",
> CONFIG_TI_FDT_FOLDER_PATH,
> + fdt_file_name);
> + }
> +
> + env_set("fdtfile", fdtfile);
> +
> + /*
> + * XXX: DEPRECATION WARNING: 2 u-boot versions (2024.10).
> + *
> + * Maintain compatibility with downstream scripts that may be using
> + * name_fdt
> + */
> + if (board_name)
> + env_set("name_fdt", fdtfile);
> + /* Also set the findfdt legacy script to warn users to stop using this
> */
> + env_set("findfdt",
> + "echo WARN: fdtfile already set. Stop using findfdt in script");
> +}
> diff --git a/board/ti/common/fdt_ops.h b/board/ti/common/fdt_ops.h
> new file mode 100644
> index ..5d304994fb6
Re: [PATCH 02/10] board: ti: am62ax: Set fdtfile from C code instead of findfdt script
Nishanth Menon writes:
> Stop using the findfdt script and switch to setting the fdtfile from
> C code.
>
> While at this, replace findfdt in environment with a warning as it is
> no longer needed
>
> Signed-off-by: Nishanth Menon
> ---
> board/ti/am62ax/am62ax.env | 1 -
> board/ti/am62ax/evm.c| 10 ++
> configs/am62ax_evm_a53_defconfig | 1 +
> 3 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/board/ti/am62ax/am62ax.env b/board/ti/am62ax/am62ax.env
> index a6d967e982d4..334374abb73e 100644
> --- a/board/ti/am62ax/am62ax.env
> +++ b/board/ti/am62ax/am62ax.env
> @@ -1,5 +1,4 @@
> #include
> -#include
> #include
>
> name_kern=Image
> diff --git a/board/ti/am62ax/evm.c b/board/ti/am62ax/evm.c
> index cd3360a43029..62d3664936e7 100644
> --- a/board/ti/am62ax/evm.c
> +++ b/board/ti/am62ax/evm.c
> @@ -13,6 +13,8 @@
> #include
> #include
>
> +#include "../common/fdt_ops.h"
> +
> int board_init(void)
> {
> return 0;
> @@ -27,3 +29,11 @@ int dram_init_banksize(void)
> {
> return fdtdec_setup_memory_banksize();
> }
> +
> +#ifdef CONFIG_BOARD_LATE_INIT
> +int board_late_init(void)
> +{
> + ti_set_fdt_env(NULL, NULL);
If a board only has one FDT possible, why not just hard code the path
here, rather than use the set_fdt_env() logic with NULLs and create/set
TI_EVM_FDT_FOLDER_PATH config that is only used here anyway?
If you think we might add additional board types / FDTs later, then
instead let's formalize the fdt_map concept and create a CONFIG_USES_FDT_MAP
that will then rely on the fdt_map and call the ti_set_fdt_env(), and if
not set, then it just returns a hard coded value, which could be based on
the CONFIG_DEFAULT_DEVICE_TREE config.
> + return 0;
> +}
> +#endif
> diff --git a/configs/am62ax_evm_a53_defconfig
> b/configs/am62ax_evm_a53_defconfig
> index 38083586a3ec..e5fcd8cc5b6f 100644
> --- a/configs/am62ax_evm_a53_defconfig
> +++ b/configs/am62ax_evm_a53_defconfig
> @@ -24,6 +24,7 @@ CONFIG_SPL_LOAD_FIT_ADDRESS=0x8100
> CONFIG_BOOTSTD_FULL=y
> CONFIG_BOOTSTD_DEFAULTS=y
> CONFIG_BOOTCOMMAND="run envboot; bootflow scan -lb"
> +CONFIG_BOARD_LATE_INIT=y
> CONFIG_SPL_MAX_SIZE=0x58000
> CONFIG_SPL_PAD_TO=0x0
> CONFIG_SPL_HAS_BSS_LINKER_SECTION=y
> --
> 2.43.0
Re: [PATCH 08/10] board: beagle: beagleboneai64: Set fdtfile from C code instead of findfdt script
Nishanth Menon writes:
> Stop using the findfdt script and switch to setting the fdtfile from C
> code.
>
> While at this, replace findfdt in environment with a warning as it is
> no longer needed
>
> Signed-off-by: Nishanth Menon
> ---
> board/beagle/beagleboneai64/beagleboneai64.c | 14 ++
> board/beagle/beagleboneai64/beagleboneai64.env | 1 -
> configs/j721e_beagleboneai64_a72_defconfig | 3 ++-
> 3 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/board/beagle/beagleboneai64/beagleboneai64.c
> b/board/beagle/beagleboneai64/beagleboneai64.c
> index c8c1c78ae5a2..1982f738b04e 100644
> --- a/board/beagle/beagleboneai64/beagleboneai64.c
> +++ b/board/beagle/beagleboneai64/beagleboneai64.c
> @@ -28,3 +28,17 @@ int dram_init_banksize(void)
> {
> return fdtdec_setup_memory_banksize();
> }
> +
> +#ifdef CONFIG_BOARD_LATE_INIT
> +int board_late_init(void)
> +{
> + char fdtfile[50];
> +
> + snprintf(fdtfile, sizeof(fdtfile), "%s/%s.dtb",
> + CONFIG_TI_EVM_FDT_FOLDER_PATH, CONFIG_DEFAULT_DEVICE_TREE);
This would set the board to using the control DT, not boot DT. Is that
what you meant?
But anyway, why not just hard code the FDT path/name here since there is
only one for this board? I don't see the value in the extra logic of
using the config values (or having a fdt_map). (Same for beagleplay)
> +
> + env_set("fdtfile", fdtfile);
> +
> + return 0;
> +}
> +#endif
> diff --git a/board/beagle/beagleboneai64/beagleboneai64.env
> b/board/beagle/beagleboneai64/beagleboneai64.env
> index 4f0a94a8113e..647b25d14c8e 100644
> --- a/board/beagle/beagleboneai64/beagleboneai64.env
> +++ b/board/beagle/beagleboneai64/beagleboneai64.env
> @@ -1,5 +1,4 @@
> #include
> -#include
> #include
>
> name_kern=Image
> diff --git a/configs/j721e_beagleboneai64_a72_defconfig
> b/configs/j721e_beagleboneai64_a72_defconfig
> index 959f86844d32..9e53658eacb9 100644
> --- a/configs/j721e_beagleboneai64_a72_defconfig
> +++ b/configs/j721e_beagleboneai64_a72_defconfig
> @@ -34,7 +34,8 @@ CONFIG_AUTOBOOT_PROMPT="Press SPACE to abort autoboot in %d
> seconds\n"
> CONFIG_AUTOBOOT_DELAY_STR="d"
> CONFIG_AUTOBOOT_STOP_STR=" "
> CONFIG_OF_SYSTEM_SETUP=y
> -CONFIG_BOOTCOMMAND="run set_led_state_start_load;run findfdt; run envboot;
> bootflow scan -lb;run set_led_state_fail_load"
> +CONFIG_BOOTCOMMAND="run set_led_state_start_load; run envboot; bootflow scan
> -lb;run set_led_state_fail_load"
> +CONFIG_BOARD_LATE_INIT=y
> CONFIG_LOGLEVEL=7
> CONFIG_SPL_MAX_SIZE=0xc
> CONFIG_SPL_HAS_BSS_LINKER_SECTION=y
> --
> 2.43.0
Re: [PATCH 01/10] board: ti: common: Introduce a common fdt ops library
Nishanth Menon writes:
> Introduce a common fdt operations library for basic device tree
> operations that are common between various boards.
>
> The first library to introduce here is the capability to set up
> fdtfile as a standard variable as part of board identification rather
> than depend on scripted ifdeffery.
>
> Signed-off-by: Nishanth Menon
> ---
> board/ti/common/Kconfig | 12
> board/ti/common/Makefile | 1 +
> board/ti/common/fdt_ops.c | 65 +++
> board/ti/common/fdt_ops.h | 41
> 4 files changed, 119 insertions(+)
> create mode 100644 board/ti/common/fdt_ops.c
> create mode 100644 board/ti/common/fdt_ops.h
>
> diff --git a/board/ti/common/Kconfig b/board/ti/common/Kconfig
> index 49edd98014ab..06a8a36aa1cd 100644
> --- a/board/ti/common/Kconfig
> +++ b/board/ti/common/Kconfig
> @@ -49,3 +49,15 @@ config TI_COMMON_CMD_OPTIONS
> imply CMD_SPI
> imply CMD_TIME
> imply CMD_USB if USB
> +
> +config TI_EVM_FDT_FOLDER_PATH
> + string "Location of Folder path where dtb is present"
> + default "ti/davinci" if ARCH_DAVINCI
> + default "ti/keystone" if ARCH_KEYSTONE
> + default "ti/omap" if ARCH_OMAP2PLUS
> + default "ti" if ARCH_K3
> + depends on ARCH_DAVINCI || ARCH_KEYSTONE || ARCH_OMAP2PLUS || ARCH_K3
> + help
> +Folder path for kernel device tree default.
> +This is used along with fdtfile path to locate the kernel
> +device tree blob.
It's not clear to me why we need the flexibility of specifying a FDT
filename per board independently of the FDT folder path. Why can't the path
be part of the fdt_map?
> diff --git a/board/ti/common/Makefile b/board/ti/common/Makefile
> index 26bf12e2e6d5..5ac361ba7fcf 100644
> --- a/board/ti/common/Makefile
> +++ b/board/ti/common/Makefile
> @@ -3,3 +3,4 @@
>
> obj-${CONFIG_TI_I2C_BOARD_DETECT} += board_detect.o
> obj-${CONFIG_CMD_EXTENSION} += cape_detect.o
> +obj-${CONFIG_OF_LIBFDT} += fdt_ops.o
> diff --git a/board/ti/common/fdt_ops.c b/board/ti/common/fdt_ops.c
> new file mode 100644
> index ..f8770cae4a54
> --- /dev/null
> +++ b/board/ti/common/fdt_ops.c
> @@ -0,0 +1,65 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Library to support FDT file operations which are common
> + *
> + * Copyright (C) 2024 Texas Instruments Incorporated - https://www.ti.com/
> + */
> +
> +#include
> +#include
> +#include "fdt_ops.h"
> +
> +void ti_set_fdt_env(const char *name_fdt, struct ti_fdt_map *fdt_map)
This function takes a board name and sets the FDT name, so why isn't the
first parameter called 'board_name' or similar?
> +{
> + char *fdt_file_name = NULL;
> + char fdtfile[TI_FDT_FILE_MAX];
> +
> + if (name_fdt) {
> + while (fdt_map) {
> + /* Check for NULL terminator in the list */
> + if (!fdt_map->name_fdt)
> + break;
> + if (!strncmp(fdt_map->name_fdt, name_fdt,
> TI_NAME_FDT_MAX)) {
Why do we need a max length? Shouldn't strcmp() be fine given the
name_fdt member of the fdt_map is set in code (ie, not read in)?
> + fdt_file_name = fdt_map->fdt_file_name;
> + break;
> + }
> + fdt_map++;
> + }
> + }
> +
> + /* match not found OR null name_fdt */
> + if (!fdt_file_name) {
> + /*
> + * Prioritize CONFIG_DEFAULT_FDT_FILE - if that is not defined,
> + * or is empty, then use CONFIG_DEFAULT_DEVICE_TREE
> + */
> +#ifdef CONFIG_DEFAULT_FDT_FILE
> + if (strlen(CONFIG_DEFAULT_FDT_FILE)) {
> + snprintf(fdtfile, sizeof(fdtfile), "%s/%s",
> + CONFIG_TI_EVM_FDT_FOLDER_PATH,
> CONFIG_DEFAULT_FDT_FILE);
I do not see where any TI platforms set CONFIG_DEFAULT_FDT_FILE, so why
have logic that checks for it? We don't use it. With this patch
(fdt_map) I don't see why we would start needing it in the future.
> + } else
> +#endif
> + {
> + snprintf(fdtfile, sizeof(fdtfile), "%s/%s.dtb",
> + CONFIG_TI_EVM_FDT_FOLDER_PATH,
> + CONFIG_DEFAULT_DEVICE_TREE);
If fdtfile isn't set, EFI bootmeth falls back to the control DT anyway,
so this is unnecessary duplication of logic.
> + }
> + } else {
> + snprintf(fdtfile, sizeof(fdtfile), "%s/%s",
> CONFIG_TI_EVM_FDT_FOLDER_PATH,
> + fdt_file_name);
> + }
> +
> + env_set("fdtfile", fdtfile);
> +
> + /*
> + * XXX: DEPRECATION WARNING: 2 u-boot versions.
> + *
> + * Maintain compatibility with downstream scripts that may be using
> + * name_fdt
> + */
> + if (name_fdt)
> + env_set("name_fdt", name_fdt);
> + /* Also set t
Re: [PATCH 1/2] board: ti: am62x: am62x.env: Fix boot_targets
Andrew Davis writes: * uEnv.txt processing >>> >>> What command can do this? >>> >> >> run envboot; >> >> https://source.denx.de/u-boot/u-boot/-/blob/master/include/env/ti/mmc.env?ref_type=heads#L20 > > This could be a new bootmeth which looks for uenv.txt on available > devices. It might be better to put the env in a FIT or something with > a checksum. >> >> We use non-FIT boot as well for our non HS platforms, I think the new >> bootmeth might be helpful as you mentioned, would have to look at this >> more as I thought bootmeth is supposed to boot a platform at the end >> only but it seems like here we'll be combining this bootmeth ( that'll >> just import the env ) and then other bootmeth will actually boot Linux. >> > > As long as bootmeths can perform an action then continue with other > bootmeths that could work. This might require complex logic though. > For instance the uenv.txt bootmeth would only need run on some subset > of available bootdevs, and it should scan all those devs for uenv.txt > first, before the Linux finding bootmeths start. > It doesn't make sense to treat envboot as a bootmeth because it's really not about a way of booting but more often just used to modify environment variables before going through another boot flow (although you could define/run env variables that performs it's own boot). It may be better to think of it as a preprocessing step before a bootflow scan. Some other thoughts - envboot is more of a hacky way to solve things: 1) As we move more towards secure boot as the standard, we should understand what use cases the hacks are working around and properly support those use cases. 2) But I don't think we should just remove it altogether because it is a very powerful 'feature' during the development phases to try out changes without modifying/rebuilding/reflashing u-boot. 3) This is a TI feature. Are others solving this need in a better way? Jon
Re: [PATCH] include: env: ti: default_findfdt: Follow the bootstd/distro conventions
Nishanth Menon writes:
> Distroboot and bootstd both mandate a findfdt variable pointing to the
> correct device tree blob. Current mechanism calls a find_fdt function
> to set this variable. We do not need a find_fdt command to set the
> environment variable to a single dtb. Simplify the default_findfdt to
> remove variable expansion while at it.
>
> For legacy scripts depending on a TI convention of name_fdt, provide a
> find_fdt wrapper that behaves like before.
>
> NOTE: As part of this change, we also drop the cooked up
> default_device_tree_arch default_device_tree_subarch variables.
>
> Reported-by: Jonathan Humphreys
> Signed-off-by: Nishanth Menon
> ---
> include/env/ti/default_findfdt.env | 13 -
> 1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/include/env/ti/default_findfdt.env
> b/include/env/ti/default_findfdt.env
> index a2b51dd923bb..1a1ab8406c9e 100644
> --- a/include/env/ti/default_findfdt.env
> +++ b/include/env/ti/default_findfdt.env
> @@ -1,12 +1,7 @@
> -default_device_tree=CONFIG_DEFAULT_DEVICE_TREE
> -default_device_tree_arch=ti
> #ifdef CONFIG_ARM64
> -findfdt=
> - setenv name_fdt ${default_device_tree_arch}/${default_device_tree}.dtb;
> - setenv fdtfile ${name_fdt}
> +fdtfile=ti/CONFIG_DEFAULT_DEVICE_TREE.dtb;
> #else
> -default_device_tree_subarch=omap
> -findfdt=
> - setenv name_fdt
> ${default_device_tree_arch}/${default_device_tree_subarch}/${default_device_tree}.dtb;
> - setenv fdtfile ${name_fdt}
> +fdtfile=ti/omap/CONFIG_DEFAULT_DEVICE_TREE.dtb;
> #endif
> +
> +findfdt=setenv name_fdt ${fdt_file}
> --
> 2.43.0
Nishanth, only am62/beagleplay uses the default_findfdt.env definitions. Others
(eg, j721e) define their own and have a run-time component to them.
Jon
