From d709d4695fd3740025068cc9225755255875f6ad Mon Sep 17 00:00:00 2001 From: Sam Edwards Date: Mon, 14 Aug 2023 16:34:13 -0600 Subject: [PATCH 01/11] pci: pcie-brcmstb: bring over some robustness improvements from Linux Since the initial U-Boot driver was ported here from Linux, the latter has had a few changes for robustness/stability. This patch brings over two of them: - Do not attempt to access the configuration space of a PCIe device if the link has gone down, as that will result in an asynchronous SError interrupt which will crash U-Boot. - Wait for the recommended 100ms after PERST# is deasserted. I sent this patch while debugging a crash involving PCIe, but these are unrelated improvements. I do not believe that this patch fixes any real-world bug. Signed-off-by: Sam Edwards --- drivers/pci/pcie_brcmstb.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/drivers/pci/pcie_brcmstb.c b/drivers/pci/pcie_brcmstb.c index 1de28021138..a159952822b 100644 --- a/drivers/pci/pcie_brcmstb.c +++ b/drivers/pci/pcie_brcmstb.c @@ -223,6 +223,10 @@ static int brcm_pcie_config_address(const struct udevice *dev, pci_dev_t bdf, return 0; } + /* An access to our HW w/o link-up will cause a CPU Abort */ + if (!brcm_pcie_link_up(pcie)) + return -EINVAL; + /* For devices, write to the config space index register */ idx = PCIE_ECAM_OFFSET(pci_bus, pci_dev, pci_func, 0); @@ -505,6 +509,12 @@ static int brcm_pcie_probe(struct udevice *dev) clrbits_le32(pcie->base + PCIE_RGR1_SW_INIT_1, RGR1_SW_INIT_1_PERST_MASK); + /* + * Wait for 100ms after PERST# deassertion; see PCIe CEM specification + * sections 2.2, PCIe r5.0, 6.6.1. + */ + mdelay(100); + /* Give the RC/EP time to wake up, before trying to configure RC. * Intermittently check status for link-up, up to a total of 100ms. */ From 59bf0cdfa9a76cb6da7532a10584bcd40c3e3a00 Mon Sep 17 00:00:00 2001 From: Sam Edwards Date: Wed, 16 Aug 2023 15:27:53 -0700 Subject: [PATCH 02/11] pci: pcie-brcmstb: do not rely on CLKREQ# signal When the Broadcom STB PCIe controller is initialized, it must be set into one of three CLKREQ# modes: "none"/"aspm"/"l1ss". The Linux driver, through today, hard-codes "aspm" since the vast majority of boards using this driver have a fixed PCIe bus with the CLKREQ# signal wired up. The Raspberry Pi CM4, however, can be connected to a plethora of PCIe devices, some of which do not connect the CLKREQ# line (they just leave it floating). So "aspm" mode is no longer appropriate in all cases. In Linux, there is a proposed patchset [1] to determine the proper mode. This doesn't really make sense in U-Boot's case, so we just change the assumption from "aspm" to "none" (which is always safe). This patch DOES resolve a real-world crash that occurs when U-Boot is running on a Raspberry Pi CM4 installed in slot 3 of a Turing Pi 2 cluster board. [1]: https://lore.kernel.org/all/20230428223500.23337-1-jim2101024@gmail.com/ Signed-off-by: Sam Edwards --- drivers/pci/pcie_brcmstb.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/drivers/pci/pcie_brcmstb.c b/drivers/pci/pcie_brcmstb.c index a159952822b..cd45f0bee9b 100644 --- a/drivers/pci/pcie_brcmstb.c +++ b/drivers/pci/pcie_brcmstb.c @@ -33,6 +33,9 @@ #define PCIE_RC_CFG_PRIV1_ID_VAL3 0x043c #define CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK 0xffffff +#define PCIE_RC_CFG_PRIV1_LINK_CAPABILITY 0x04dc +#define PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK 0xc00 + #define PCIE_RC_DL_MDIO_ADDR 0x1100 #define PCIE_RC_DL_MDIO_WR_DATA 0x1104 #define PCIE_RC_DL_MDIO_RD_DATA 0x1108 @@ -88,7 +91,6 @@ PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LIMIT_HI + ((win) * 8) #define PCIE_MISC_HARD_PCIE_HARD_DEBUG 0x4204 -#define PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK 0x2 #define PCIE_HARD_DEBUG_SERDES_IDDQ_MASK 0x08000000 #define PCIE_MSI_INTR2_CLR 0x4508 @@ -572,12 +574,18 @@ static int brcm_pcie_probe(struct udevice *dev) clrsetbits_le32(base + PCIE_RC_CFG_VENDOR_SPECIFIC_REG1, VENDOR_SPECIFIC_REG1_ENDIAN_MODE_BAR2_MASK, VENDOR_SPECIFIC_REG1_LITTLE_ENDIAN); + /* - * Refclk from RC should be gated with CLKREQ# input when ASPM L0s,L1 - * is enabled => setting the CLKREQ_DEBUG_ENABLE field to 1. + * We used to enable the CLKREQ# input here, but a few PCIe cards don't + * attach anything to the CLKREQ# line, so we shouldn't assume that + * it's connected and working. The controller does allow detecting + * whether the port on the other side of our link is/was driving this + * signal, so we could check before we assume. But because this signal + * is for power management, which doesn't make sense in a bootloader, + * let's instead just unadvertise ASPM support. */ - setbits_le32(base + PCIE_MISC_HARD_PCIE_HARD_DEBUG, - PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK); + clrbits_le32(base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY, + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK); return 0; } From 8d8851e8fd52fdfbafd1dcb5faa66f20f9588d97 Mon Sep 17 00:00:00 2001 From: Oleksandr Suvorov Date: Thu, 17 Aug 2023 18:36:10 +0300 Subject: [PATCH 03/11] tools: image-host: print error messages to stderr The make by default cuts off the stdout output from external tools, so all error messages from the image-host are not shown in a make output. Besides that, it is a common approach to use stderr stream for error messages. Use stderr for all error messages in image-host. Signed-off-by: Oleksandr Suvorov Reviewed-by: Simon Glass --- tools/image-host.c | 202 ++++++++++++++++++++++++--------------------- 1 file changed, 107 insertions(+), 95 deletions(-) diff --git a/tools/image-host.c b/tools/image-host.c index 4a24dee8153..ca4950312f9 100644 --- a/tools/image-host.c +++ b/tools/image-host.c @@ -38,9 +38,9 @@ static int fit_set_hash_value(void *fit, int noffset, uint8_t *value, ret = fdt_setprop(fit, noffset, FIT_VALUE_PROP, value, value_len); if (ret) { - printf("Can't set hash '%s' property for '%s' node(%s)\n", - FIT_VALUE_PROP, fit_get_name(fit, noffset, NULL), - fdt_strerror(ret)); + fprintf(stderr, "Can't set hash '%s' property for '%s' node(%s)\n", + FIT_VALUE_PROP, fit_get_name(fit, noffset, NULL), + fdt_strerror(ret)); return ret == -FDT_ERR_NOSPACE ? -ENOSPC : -EIO; } @@ -72,21 +72,23 @@ static int fit_image_process_hash(void *fit, const char *image_name, node_name = fit_get_name(fit, noffset, NULL); if (fit_image_hash_get_algo(fit, noffset, &algo)) { - printf("Can't get hash algo property for '%s' hash node in '%s' image node\n", - node_name, image_name); + fprintf(stderr, + "Can't get hash algo property for '%s' hash node in '%s' image node\n", + node_name, image_name); return -ENOENT; } if (calculate_hash(data, size, algo, value, &value_len)) { - printf("Unsupported hash algorithm (%s) for '%s' hash node in '%s' image node\n", - algo, node_name, image_name); + fprintf(stderr, + "Unsupported hash algorithm (%s) for '%s' hash node in '%s' image node\n", + algo, node_name, image_name); return -EPROTONOSUPPORT; } ret = fit_set_hash_value(fit, noffset, value, value_len); if (ret) { - printf("Can't set hash value for '%s' hash node in '%s' image node\n", - node_name, image_name); + fprintf(stderr, "Can't set hash value for '%s' hash node in '%s' image node\n", + node_name, image_name); return ret; } @@ -170,8 +172,9 @@ static int fit_image_setup_sig(struct image_sign_info *info, node_name = fit_get_name(fit, noffset, NULL); if (!algo_name) { if (fit_image_hash_get_algo(fit, noffset, &algo_name)) { - printf("Can't get algo property for '%s' signature node in '%s' image node\n", - node_name, image_name); + fprintf(stderr, + "Can't get algo property for '%s' signature node in '%s' image node\n", + node_name, image_name); return -1; } } @@ -191,8 +194,9 @@ static int fit_image_setup_sig(struct image_sign_info *info, info->require_keys = require_keys; info->engine_id = engine_id; if (!info->checksum || !info->crypto) { - printf("Unsupported signature algorithm (%s) for '%s' signature node in '%s' image node\n", - algo_name, node_name, image_name); + fprintf(stderr, + "Unsupported signature algorithm (%s) for '%s' signature node in '%s' image node\n", + algo_name, node_name, image_name); return -1; } @@ -241,8 +245,8 @@ static int fit_image_process_sig(const char *keydir, const char *keyfile, region.size = size; ret = info.crypto->sign(&info, ®ion, 1, &value, &value_len); if (ret) { - printf("Failed to sign '%s' signature node in '%s' image node: %d\n", - node_name, image_name, ret); + fprintf(stderr, "Failed to sign '%s' signature node in '%s' image node: %d\n", + node_name, image_name, ret); /* We allow keys to be missing */ if (ret == -ENOENT) @@ -255,8 +259,9 @@ static int fit_image_process_sig(const char *keydir, const char *keyfile, if (ret) { if (ret == -FDT_ERR_NOSPACE) return -ENOSPC; - printf("Can't write signature for '%s' signature node in '%s' conf node: %s\n", - node_name, image_name, fdt_strerror(ret)); + fprintf(stderr, + "Can't write signature for '%s' signature node in '%s' conf node: %s\n", + node_name, image_name, fdt_strerror(ret)); return -1; } free(value); @@ -272,8 +277,9 @@ static int fit_image_process_sig(const char *keydir, const char *keyfile, if (keydest) { ret = info.crypto->add_verify_data(&info, keydest); if (ret < 0) { - printf("Failed to add verification data for '%s' signature node in '%s' image node\n", - node_name, image_name); + fprintf(stderr, + "Failed to add verification data for '%s' signature node in '%s' image node\n", + node_name, image_name); return ret; } /* Return the node that was written to */ @@ -293,37 +299,37 @@ static int fit_image_read_data(char *filename, unsigned char *data, /* Open file */ fd = open(filename, O_RDONLY | O_BINARY); if (fd < 0) { - printf("Can't open file %s (err=%d => %s)\n", - filename, errno, strerror(errno)); + fprintf(stderr, "Can't open file %s (err=%d => %s)\n", + filename, errno, strerror(errno)); return -1; } /* Compute file size */ if (fstat(fd, &sbuf) < 0) { - printf("Can't fstat file %s (err=%d => %s)\n", - filename, errno, strerror(errno)); + fprintf(stderr, "Can't fstat file %s (err=%d => %s)\n", + filename, errno, strerror(errno)); goto err; } /* Check file size */ if (sbuf.st_size != expected_size) { - printf("File %s don't have the expected size (size=%lld, expected=%d)\n", - filename, (long long)sbuf.st_size, expected_size); + fprintf(stderr, "File %s don't have the expected size (size=%lld, expected=%d)\n", + filename, (long long)sbuf.st_size, expected_size); goto err; } /* Read data */ n = read(fd, data, sbuf.st_size); if (n < 0) { - printf("Can't read file %s (err=%d => %s)\n", - filename, errno, strerror(errno)); + fprintf(stderr, "Can't read file %s (err=%d => %s)\n", + filename, errno, strerror(errno)); goto err; } /* Check that we have read all the file */ if (n != sbuf.st_size) { - printf("Can't read all file %s (read %zd bytes, expected %lld)\n", - filename, n, (long long)sbuf.st_size); + fprintf(stderr, "Can't read all file %s (read %zd bytes, expected %lld)\n", + filename, n, (long long)sbuf.st_size); goto err; } @@ -341,15 +347,15 @@ static int get_random_data(void *data, int size) int i, ret; if (!tmp) { - printf("%s: pointer data is NULL\n", __func__); + fprintf(stderr, "%s: pointer data is NULL\n", __func__); ret = -1; goto out; } ret = clock_gettime(CLOCK_MONOTONIC, &date); if (ret) { - printf("%s: clock_gettime has failed (%s)\n", __func__, - strerror(errno)); + fprintf(stderr, "%s: clock_gettime has failed (%s)\n", __func__, + strerror(errno)); goto out; } @@ -374,8 +380,8 @@ static int fit_image_setup_cipher(struct image_cipher_info *info, int ret = -1; if (fit_image_cipher_get_algo(fit, noffset, &algo_name)) { - printf("Can't get algo name for cipher in image '%s'\n", - image_name); + fprintf(stderr, "Can't get algo name for cipher in image '%s'\n", + image_name); goto out; } @@ -384,8 +390,8 @@ static int fit_image_setup_cipher(struct image_cipher_info *info, /* Read the key name */ info->keyname = fdt_getprop(fit, noffset, FIT_KEY_HINT, NULL); if (!info->keyname) { - printf("Can't get key name for cipher in image '%s'\n", - image_name); + fprintf(stderr, "Can't get key name for cipher in image '%s'\n", + image_name); goto out; } @@ -403,7 +409,7 @@ static int fit_image_setup_cipher(struct image_cipher_info *info, info->cipher = image_get_cipher_algo(algo_name); if (!info->cipher) { - printf("Can't get algo for cipher '%s'\n", image_name); + fprintf(stderr, "Can't get algo for cipher '%s'\n", image_name); goto out; } @@ -412,7 +418,7 @@ static int fit_image_setup_cipher(struct image_cipher_info *info, info->keydir, info->keyname, ".bin"); info->key = malloc(info->cipher->key_len); if (!info->key) { - printf("Can't allocate memory for key\n"); + fprintf(stderr, "Can't allocate memory for key\n"); ret = -1; goto out; } @@ -423,7 +429,7 @@ static int fit_image_setup_cipher(struct image_cipher_info *info, info->iv = malloc(info->cipher->iv_len); if (!info->iv) { - printf("Can't allocate memory for iv\n"); + fprintf(stderr, "Can't allocate memory for iv\n"); ret = -1; goto out; } @@ -457,7 +463,7 @@ int fit_image_write_cipher(void *fit, int image_noffset, int noffset, goto out; } if (ret) { - printf("Can't replace data with ciphered data (err = %d)\n", ret); + fprintf(stderr, "Can't replace data with ciphered data (err = %d)\n", ret); goto out; } @@ -468,7 +474,7 @@ int fit_image_write_cipher(void *fit, int image_noffset, int noffset, goto out; } if (ret) { - printf("Can't add unciphered data size (err = %d)\n", ret); + fprintf(stderr, "Can't add unciphered data size (err = %d)\n", ret); goto out; } @@ -508,8 +514,9 @@ fit_image_process_cipher(const char *keydir, void *keydest, void *fit, if (keydest) { ret = info.cipher->add_cipher_data(&info, keydest, fit, node_noffset); if (ret) { - printf("Failed to add verification data for cipher '%s' in image '%s'\n", - info.keyname, image_name); + fprintf(stderr, + "Failed to add verification data for cipher '%s' in image '%s'\n", + info.keyname, image_name); goto out; } } @@ -538,13 +545,13 @@ int fit_image_cipher_data(const char *keydir, void *keydest, /* Get image name */ image_name = fit_get_name(fit, image_noffset, NULL); if (!image_name) { - printf("Can't get image name\n"); + fprintf(stderr, "Can't get image name\n"); return -1; } /* Get image data and data length */ if (fit_image_get_data(fit, image_noffset, &data, &size)) { - printf("Can't get image data/size\n"); + fprintf(stderr, "Can't get image data/size\n"); return -1; } @@ -558,7 +565,7 @@ int fit_image_cipher_data(const char *keydir, void *keydest, if (fdt_getprop(fit, image_noffset, "data-size-unciphered", &len)) return 0; if (len != -FDT_ERR_NOTFOUND) { - printf("Failure testing for data-size-unciphered\n"); + fprintf(stderr, "Failure testing for data-size-unciphered\n"); return -1; } @@ -568,7 +575,7 @@ int fit_image_cipher_data(const char *keydir, void *keydest, if (cipher_node_offset == -FDT_ERR_NOTFOUND) return 0; if (cipher_node_offset < 0) { - printf("Failure getting cipher node\n"); + fprintf(stderr, "Failure getting cipher node\n"); return -1; } if (!IMAGE_ENABLE_ENCRYPT || !keydir) @@ -624,7 +631,7 @@ int fit_image_add_verification_data(const char *keydir, const char *keyfile, /* Get image data and data length */ if (fit_image_get_data(fit, image_noffset, &data, &size)) { - printf("Can't get image data/size\n"); + fprintf(stderr, "Can't get image data/size\n"); return -1; } @@ -765,8 +772,9 @@ static int fit_config_add_hash(const void *fit, int image_noffset, } if (!hash_count) { - printf("Failed to find any hash nodes in configuration '%s/%s' image '%s' - without these it is not possible to verify this image\n", - conf_name, sig_name, iname); + fprintf(stderr, + "Failed to find any hash nodes in configuration '%s/%s' image '%s' - without these it is not possible to verify this image\n", + conf_name, sig_name, iname); return -ENOMSG; } @@ -775,9 +783,10 @@ static int fit_config_add_hash(const void *fit, int image_noffset, FIT_CIPHER_NODENAME); if (noffset != -FDT_ERR_NOTFOUND) { if (noffset < 0) { - printf("Failed to get cipher node in configuration '%s/%s' image '%s': %s\n", - conf_name, sig_name, iname, - fdt_strerror(noffset)); + fprintf(stderr, + "Failed to get cipher node in configuration '%s/%s' image '%s': %s\n", + conf_name, sig_name, iname, + fdt_strerror(noffset)); return -EIO; } ret = fdt_get_path(fit, noffset, path, sizeof(path)); @@ -790,13 +799,13 @@ static int fit_config_add_hash(const void *fit, int image_noffset, return 0; err_mem: - printf("Out of memory processing configuration '%s/%s'\n", conf_name, - sig_name); + fprintf(stderr, "Out of memory processing configuration '%s/%s'\n", conf_name, + sig_name); return -ENOMEM; err_path: - printf("Failed to get path for image '%s' in configuration '%s/%s': %s\n", - iname, conf_name, sig_name, fdt_strerror(ret)); + fprintf(stderr, "Failed to get path for image '%s' in configuration '%s/%s': %s\n", + iname, conf_name, sig_name, fdt_strerror(ret)); return -ENOENT; } @@ -857,8 +866,9 @@ static int fit_config_get_hash_list(const void *fit, int conf_noffset, iname, index); if (image_noffset < 0) { - printf("Failed to find image '%s' in configuration '%s/%s'\n", - iname, conf_name, sig_name); + fprintf(stderr, + "Failed to find image '%s' in configuration '%s/%s'\n", + iname, conf_name, sig_name); if (allow_missing) continue; @@ -875,16 +885,16 @@ static int fit_config_get_hash_list(const void *fit, int conf_noffset, } if (!image_count) { - printf("Failed to find any images for configuration '%s/%s'\n", - conf_name, sig_name); + fprintf(stderr, "Failed to find any images for configuration '%s/%s'\n", + conf_name, sig_name); return -ENOMSG; } return 0; err_mem: - printf("Out of memory processing configuration '%s/%s'\n", conf_name, - sig_name); + fprintf(stderr, "Out of memory processing configuration '%s/%s'\n", conf_name, + sig_name); return -ENOMEM; } @@ -946,21 +956,21 @@ static int fit_config_get_regions(const void *fit, int conf_noffset, fdt_regions, ARRAY_SIZE(fdt_regions), path, sizeof(path), 1); if (count < 0) { - printf("Failed to hash configuration '%s/%s': %s\n", conf_name, - sig_name, fdt_strerror(ret)); + fprintf(stderr, "Failed to hash configuration '%s/%s': %s\n", conf_name, + sig_name, fdt_strerror(ret)); return -EIO; } if (count == 0) { - printf("No data to hash for configuration '%s/%s': %s\n", - conf_name, sig_name, fdt_strerror(ret)); + fprintf(stderr, "No data to hash for configuration '%s/%s': %s\n", + conf_name, sig_name, fdt_strerror(ret)); return -EINVAL; } /* Build our list of data blocks */ region = fit_region_make_list(fit, fdt_regions, count, NULL); if (!region) { - printf("Out of memory hashing configuration '%s/%s'\n", - conf_name, sig_name); + fprintf(stderr, "Out of memory hashing configuration '%s/%s'\n", + conf_name, sig_name); return -ENOMEM; } @@ -972,8 +982,8 @@ static int fit_config_get_regions(const void *fit, int conf_noffset, } region_prop = malloc(len); if (!region_prop) { - printf("Out of memory setting up regions for configuration '%s/%s'\n", - conf_name, sig_name); + fprintf(stderr, "Out of memory setting up regions for configuration '%s/%s'\n", + conf_name, sig_name); return -ENOMEM; } for (i = len = 0; i < node_inc.count; @@ -1038,8 +1048,8 @@ static int fit_config_process_sig(const char *keydir, const char *keyfile, &value_len); free(region); if (ret) { - printf("Failed to sign '%s' signature node in '%s' conf node\n", - node_name, conf_name); + fprintf(stderr, "Failed to sign '%s' signature node in '%s' conf node\n", + node_name, conf_name); /* We allow keys to be missing */ if (ret == -ENOENT) @@ -1053,8 +1063,9 @@ static int fit_config_process_sig(const char *keydir, const char *keyfile, if (ret) { if (ret == -FDT_ERR_NOSPACE) return -ENOSPC; - printf("Can't write signature for '%s' signature node in '%s' conf node: %s\n", - node_name, conf_name, fdt_strerror(ret)); + fprintf(stderr, + "Can't write signature for '%s' signature node in '%s' conf node: %s\n", + node_name, conf_name, fdt_strerror(ret)); return -1; } free(value); @@ -1067,8 +1078,9 @@ static int fit_config_process_sig(const char *keydir, const char *keyfile, if (keydest) { ret = info.crypto->add_verify_data(&info, keydest); if (ret < 0) { - printf("Failed to add verification data for '%s' signature node in '%s' configuration node\n", - node_name, conf_name); + fprintf(stderr, + "Failed to add verification data for '%s' signature node in '%s' configuration node\n", + node_name, conf_name); } return ret; } @@ -1148,7 +1160,7 @@ static int read_pub_key(const char *keydir, const void *name, /* Read the certificate */ cert = NULL; if (!PEM_read_X509(f, &cert, NULL, NULL)) { - printf("Couldn't read certificate"); + fprintf(stderr, "Couldn't read certificate"); ret = -EINVAL; goto err_cert; } @@ -1156,7 +1168,7 @@ static int read_pub_key(const char *keydir, const void *name, /* Get the public key from the certificate. */ key = X509_get_pubkey(cert); if (!key) { - printf("Couldn't read public key\n"); + fprintf(stderr, "Couldn't read public key\n"); ret = -EINVAL; goto err_pubkey; } @@ -1164,7 +1176,7 @@ static int read_pub_key(const char *keydir, const void *name, /* Get DER form */ ret = i2d_PublicKey(key, pubkey); if (ret < 0) { - printf("Couldn't get DER form\n"); + fprintf(stderr, "Couldn't get DER form\n"); ret = -EINVAL; goto err_pubkey; } @@ -1203,11 +1215,11 @@ int fit_pre_load_data(const char *keydir, void *keydest, void *fit) /* Check that all mandatory properties are present */ if (!algo_name || !key_name) { if (!algo_name) - printf("The property algo-name is missing in the node %s\n", - IMAGE_PRE_LOAD_PATH); + fprintf(stderr, "The property algo-name is missing in the node %s\n", + IMAGE_PRE_LOAD_PATH); if (!key_name) - printf("The property key-name is missing in the node %s\n", - IMAGE_PRE_LOAD_PATH); + fprintf(stderr, "The property key-name is missing in the node %s\n", + IMAGE_PRE_LOAD_PATH); ret = -EINVAL; goto out; } @@ -1221,8 +1233,8 @@ int fit_pre_load_data(const char *keydir, void *keydest, void *fit) ret = fdt_setprop(keydest, pre_load_noffset, "public-key", pubkey, pubkey_len); if (ret) - printf("Can't set public-key in node %s (ret = %d)\n", - IMAGE_PRE_LOAD_PATH, ret); + fprintf(stderr, "Can't set public-key in node %s (ret = %d)\n", + IMAGE_PRE_LOAD_PATH, ret); out: return ret; @@ -1239,8 +1251,8 @@ int fit_cipher_data(const char *keydir, void *keydest, void *fit, /* Find images parent node offset */ images_noffset = fdt_path_offset(fit, FIT_IMAGES_PATH); if (images_noffset < 0) { - printf("Can't find images parent node '%s' (%s)\n", - FIT_IMAGES_PATH, fdt_strerror(images_noffset)); + fprintf(stderr, "Can't find images parent node '%s' (%s)\n", + FIT_IMAGES_PATH, fdt_strerror(images_noffset)); return images_noffset; } @@ -1276,8 +1288,8 @@ int fit_add_verification_data(const char *keydir, const char *keyfile, /* Find images parent node offset */ images_noffset = fdt_path_offset(fit, FIT_IMAGES_PATH); if (images_noffset < 0) { - printf("Can't find images parent node '%s' (%s)\n", - FIT_IMAGES_PATH, fdt_strerror(images_noffset)); + fprintf(stderr, "Can't find images parent node '%s' (%s)\n", + FIT_IMAGES_PATH, fdt_strerror(images_noffset)); return images_noffset; } @@ -1293,9 +1305,9 @@ int fit_add_verification_data(const char *keydir, const char *keyfile, fit, noffset, comment, require_keys, engine_id, cmdname, algo_name); if (ret) { - printf("Can't add verification data for node '%s' (%s)\n", - fdt_get_name(fit, noffset, NULL), - fdt_strerror(ret)); + fprintf(stderr, "Can't add verification data for node '%s' (%s)\n", + fdt_get_name(fit, noffset, NULL), + fdt_strerror(ret)); return ret; } } @@ -1307,8 +1319,8 @@ int fit_add_verification_data(const char *keydir, const char *keyfile, /* Find configurations parent node offset */ confs_noffset = fdt_path_offset(fit, FIT_CONFS_PATH); if (confs_noffset < 0) { - printf("Can't find images parent node '%s' (%s)\n", - FIT_CONFS_PATH, fdt_strerror(confs_noffset)); + fprintf(stderr, "Can't find images parent node '%s' (%s)\n", + FIT_CONFS_PATH, fdt_strerror(confs_noffset)); return -ENOENT; } From fa03568e46c9f4535b79e16faed4841ff7bfbe5b Mon Sep 17 00:00:00 2001 From: Maksim Kiselev Date: Fri, 18 Aug 2023 12:34:30 +0300 Subject: [PATCH 04/11] serial-uclass: reset gd->cur_serial_dev to NULL if serial not found Reset gd->cur_serial_dev pointer to avoid calling non-relocated code from relocated code if a serial driver is not found and CONFIG_REQUIRE_SERIAL_CONSOLE is disabled. Here is detailed explanation of what this patch is trying to fix. U-boot calls the serial_find_console_or_panic() function twice. The first console setup occurs before U-boot relocation in the serial_init(). This stage uses simple FDT parsing and assigns gd->cur_serial_dev to a "serial" device that lives in non-relocated code too. The second console setup after U-boot relocation(from serial_initialize()) may use full live DT (if OF_LIVE enabled) probe sequence with buses, clocks, resets, etc... And if the console setup fails at this step, than we should be caught by panic_str("No serial driver found"). But... If we disable CONFIG_REQUIRE_SERIAL_CONSOLE, than we return from serial_init() with gd->cur_serial_dev pointing to the "old"(non-relocated) serial device. And if this area, where "old" serial device is placed, is changed (e.g. Linux kernel may be relocated at this address), than we will get an unexpected crash on the next call of printf(). Signed-off-by: Maksim Kiselev --- drivers/serial/serial-uclass.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c index 067fae26145..e954f0189bb 100644 --- a/drivers/serial/serial-uclass.c +++ b/drivers/serial/serial-uclass.c @@ -151,6 +151,7 @@ static void serial_find_console_or_panic(void) #ifdef CONFIG_REQUIRE_SERIAL_CONSOLE panic_str("No serial driver found"); #endif + gd->cur_serial_dev = NULL; } #endif /* CONFIG_SERIAL_PRESENT */ From 95311f7a194aabc1a52d4f240fef36f21b3178fd Mon Sep 17 00:00:00 2001 From: Marek Vasut Date: Wed, 23 Aug 2023 02:16:52 +0200 Subject: [PATCH 05/11] fwu: Initialize global fwu library state during CI test The current CI test worked by sheer luck, the g_dev global pointer in the fwu library was never initialized and the test equally well failed on sandbox64. Trigger the main loop in sandbox tests too to initialize that global state, and move the sandbox specific exit from fwu_boottime_checks after g_dev is initialized. Signed-off-by: Marek Vasut Acked-by: Sughosh Ganu Reviewed-by: Simon Glass --- lib/fwu_updates/fwu.c | 12 ++++++------ test/dm/fwu_mdata.c | 12 ++++++++++++ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/lib/fwu_updates/fwu.c b/lib/fwu_updates/fwu.c index 4d0c8b84b9d..22bdc78df59 100644 --- a/lib/fwu_updates/fwu.c +++ b/lib/fwu_updates/fwu.c @@ -623,18 +623,18 @@ static int fwu_boottime_checks(void *ctx, struct event *event) int ret; u32 boot_idx, active_idx; - /* Don't have boot time checks on sandbox */ - if (IS_ENABLED(CONFIG_SANDBOX)) { - boottime_check = 1; - return 0; - } - ret = uclass_first_device_err(UCLASS_FWU_MDATA, &g_dev); if (ret) { log_debug("Cannot find fwu device\n"); return ret; } + /* Don't have boot time checks on sandbox */ + if (IS_ENABLED(CONFIG_SANDBOX)) { + boottime_check = 1; + return 0; + } + ret = fwu_get_mdata(NULL); if (ret) { log_debug("Unable to read meta-data\n"); diff --git a/test/dm/fwu_mdata.c b/test/dm/fwu_mdata.c index 8b5c83ef4e2..52018f610fe 100644 --- a/test/dm/fwu_mdata.c +++ b/test/dm/fwu_mdata.c @@ -93,6 +93,12 @@ static int dm_test_fwu_mdata_read(struct unit_test_state *uts) struct udevice *dev; struct fwu_mdata mdata = { 0 }; + /* + * Trigger lib/fwu_updates/fwu.c fwu_boottime_checks() + * to populate g_dev global pointer in that library. + */ + event_notify_null(EVT_MAIN_LOOP); + ut_assertok(uclass_first_device_err(UCLASS_FWU_MDATA, &dev)); ut_assertok(setup_blk_device(uts)); ut_assertok(populate_mmc_disk_image(uts)); @@ -112,6 +118,12 @@ static int dm_test_fwu_mdata_write(struct unit_test_state *uts) struct udevice *dev; struct fwu_mdata mdata = { 0 }; + /* + * Trigger lib/fwu_updates/fwu.c fwu_boottime_checks() + * to populate g_dev global pointer in that library. + */ + event_notify_null(EVT_MAIN_LOOP); + ut_assertok(setup_blk_device(uts)); ut_assertok(populate_mmc_disk_image(uts)); ut_assertok(write_mmc_blk_device(uts)); From d223dcf31ad04e3b363770f2e20a36f1c7716568 Mon Sep 17 00:00:00 2001 From: Marek Vasut Date: Wed, 23 Aug 2023 02:18:17 +0200 Subject: [PATCH 06/11] drivers/mtd/nvmxip: Trigger post bind as probe on driver level Perform all the block device creation only once, after the driver itself successfully bound. Do not do this in uclass post bind, as this might be triggered multiple times. For example the ut_dm_host test triggers this and triggers a memory leak that way, since there are now multiple block devices created using the blk_create_devicef() . To retain the old probe-on-boot behavior, set DM_FLAG_PROBE_AFTER_BIND flag in uclass post_bind callback, so the driver model would probe the driver at the right time. Rename the function as well, to match similar functions in other block-related subsystems, like the mmc one. Signed-off-by: Marek Vasut --- drivers/mtd/nvmxip/nvmxip-uclass.c | 18 +++++++----------- drivers/mtd/nvmxip/nvmxip_qspi.c | 1 + include/nvmxip.h | 12 ++++++++++++ 3 files changed, 20 insertions(+), 11 deletions(-) diff --git a/drivers/mtd/nvmxip/nvmxip-uclass.c b/drivers/mtd/nvmxip/nvmxip-uclass.c index 6d8eb177b50..36eb056c213 100644 --- a/drivers/mtd/nvmxip/nvmxip-uclass.c +++ b/drivers/mtd/nvmxip/nvmxip-uclass.c @@ -22,17 +22,7 @@ #define DEFAULT_LBA_SZ BIT(DEFAULT_LBA_SHIFT) -/** - * nvmxip_post_bind() - post binding treatments - * @dev: the NVMXIP device - * - * Create and probe a child block device. - * - * Return: - * - * 0 on success. Otherwise, failure - */ -static int nvmxip_post_bind(struct udevice *udev) +int nvmxip_probe(struct udevice *udev) { int ret; struct udevice *bdev = NULL; @@ -67,6 +57,12 @@ static int nvmxip_post_bind(struct udevice *udev) return 0; } +static int nvmxip_post_bind(struct udevice *udev) +{ + dev_or_flags(udev, DM_FLAG_PROBE_AFTER_BIND); + return 0; +} + UCLASS_DRIVER(nvmxip) = { .name = "nvmxip", .id = UCLASS_NVMXIP, diff --git a/drivers/mtd/nvmxip/nvmxip_qspi.c b/drivers/mtd/nvmxip/nvmxip_qspi.c index 7221fd1cb46..1bf0d311fe3 100644 --- a/drivers/mtd/nvmxip/nvmxip_qspi.c +++ b/drivers/mtd/nvmxip/nvmxip_qspi.c @@ -66,5 +66,6 @@ U_BOOT_DRIVER(nvmxip_qspi) = { .id = UCLASS_NVMXIP, .of_match = nvmxip_qspi_ids, .of_to_plat = nvmxip_qspi_of_to_plat, + .probe = nvmxip_probe, .plat_auto = sizeof(struct nvmxip_plat), }; diff --git a/include/nvmxip.h b/include/nvmxip.h index f4ef37725d2..726fffeb3e8 100644 --- a/include/nvmxip.h +++ b/include/nvmxip.h @@ -29,4 +29,16 @@ struct nvmxip_plat { lbaint_t lba; }; +/** + * nvmxip_bind() - post binding treatments + * @dev: the NVMXIP device + * + * Create and probe a child block device. + * + * Return: + * + * 0 on success. Otherwise, failure + */ +int nvmxip_probe(struct udevice *udev); + #endif /* __DRIVER_NVMXIP_H__ */ From 16be2bc72e1c1ce188214b71ba5c7c9d990e6021 Mon Sep 17 00:00:00 2001 From: Marek Vasut Date: Wed, 23 Aug 2023 02:18:18 +0200 Subject: [PATCH 07/11] drivers/mtd/nvmxip: Rework the read accessor to support 32bit systems Get rid of nvmxip_mmio_rawread() and just implement the readl()/readq() reader loop within nvmxip_blk_read(). Cast the destination buffer as needed and increment the read by either 4 or 8 bytes depending on if this is systemd with 32bit or 64bit physical address. Signed-off-by: Marek Vasut --- drivers/mtd/nvmxip/nvmxip.c | 38 ++++++++++++------------------------- 1 file changed, 12 insertions(+), 26 deletions(-) diff --git a/drivers/mtd/nvmxip/nvmxip.c b/drivers/mtd/nvmxip/nvmxip.c index a359e3b4822..0bd98d64275 100644 --- a/drivers/mtd/nvmxip/nvmxip.c +++ b/drivers/mtd/nvmxip/nvmxip.c @@ -15,23 +15,6 @@ #include #include "nvmxip.h" -/** - * nvmxip_mmio_rawread() - read from the XIP flash - * @address: address of the data - * @value: pointer to where storing the value read - * - * Read raw data from the XIP flash. - * - * Return: - * - * Always return 0. - */ -static int nvmxip_mmio_rawread(const phys_addr_t address, u64 *value) -{ - *value = readq(address); - return 0; -} - /** * nvmxip_blk_read() - block device read operation * @dev: the block device @@ -49,15 +32,14 @@ static ulong nvmxip_blk_read(struct udevice *dev, lbaint_t blknr, lbaint_t blkcn { struct nvmxip_plat *plat = dev_get_plat(dev->parent); struct blk_desc *desc = dev_get_uclass_plat(dev); - /* number of the u64 words to read */ - u32 qwords = (blkcnt * desc->blksz) / sizeof(u64); + /* number of bytes to read */ + u32 size = blkcnt * desc->blksz; /* physical address of the first block to read */ phys_addr_t blkaddr = plat->phys_base + blknr * desc->blksz; - u64 *virt_blkaddr; - u64 *pdst = buffer; + void *virt_blkaddr; uint qdata_idx; - if (!pdst) + if (!buffer) return -EINVAL; log_debug("[%s]: reading from blknr: %lu , blkcnt: %lu\n", dev->name, blknr, blkcnt); @@ -66,12 +48,16 @@ static ulong nvmxip_blk_read(struct udevice *dev, lbaint_t blknr, lbaint_t blkcn /* assumption: the data is virtually contiguous */ - for (qdata_idx = 0 ; qdata_idx < qwords ; qdata_idx++) - nvmxip_mmio_rawread((phys_addr_t)(virt_blkaddr + qdata_idx), pdst++); - +#if IS_ENABLED(CONFIG_PHYS_64BIT) + for (qdata_idx = 0 ; qdata_idx < size; qdata_idx += sizeof(u64)) + *(u64 *)(buffer + qdata_idx) = readq(virt_blkaddr + qdata_idx); +#else + for (qdata_idx = 0 ; qdata_idx < size; qdata_idx += sizeof(u32)) + *(u32 *)(buffer + qdata_idx) = readl(virt_blkaddr + qdata_idx); +#endif log_debug("[%s]: src[0]: 0x%llx , dst[0]: 0x%llx , src[-1]: 0x%llx , dst[-1]: 0x%llx\n", dev->name, - *virt_blkaddr, + *(u64 *)virt_blkaddr, *(u64 *)buffer, *(u64 *)((u8 *)virt_blkaddr + desc->blksz * blkcnt - sizeof(u64)), *(u64 *)((u8 *)buffer + desc->blksz * blkcnt - sizeof(u64))); From 6539f71d1d85895afd60c68d039342447713ee50 Mon Sep 17 00:00:00 2001 From: Marek Vasut Date: Wed, 23 Aug 2023 02:18:19 +0200 Subject: [PATCH 08/11] drivers/mtd/nvmxip: Print phys_addr_t without warnings on both 32bit and 64bit systems Cast the address such that it can be printed without warnings on both 32bit and 64bit systems. This really should use some better print formatter, but for the lack of it, do it this way. Signed-off-by: Marek Vasut --- drivers/mtd/nvmxip/nvmxip_qspi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/mtd/nvmxip/nvmxip_qspi.c b/drivers/mtd/nvmxip/nvmxip_qspi.c index 1bf0d311fe3..4d7471118a4 100644 --- a/drivers/mtd/nvmxip/nvmxip_qspi.c +++ b/drivers/mtd/nvmxip/nvmxip_qspi.c @@ -50,8 +50,8 @@ static int nvmxip_qspi_of_to_plat(struct udevice *dev) return -EINVAL; } - log_debug("[%s]: XIP device base addr: 0x%llx , lba_shift: %d , lbas: %lu\n", - dev->name, plat->phys_base, plat->lba_shift, plat->lba); + log_debug("[%s]: XIP device base addr: 0x%p , lba_shift: %d , lbas: %lu\n", + dev->name, (void *)(uintptr_t)plat->phys_base, plat->lba_shift, plat->lba); return 0; } From 8a4289ad727a1a6b47b5690e7b3c0fde64f3aebf Mon Sep 17 00:00:00 2001 From: Marek Vasut Date: Wed, 23 Aug 2023 02:18:20 +0200 Subject: [PATCH 09/11] drivers/mtd/nvmxip: Move sandbox_set_enable_memio() to test The sandbox_set_enable_memio() should only ever be set during sandbox testing, not within driver itself, move it back to test/ . Signed-off-by: Marek Vasut --- drivers/mtd/nvmxip/nvmxip-uclass.c | 4 ---- test/dm/nvmxip.c | 2 ++ 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/mtd/nvmxip/nvmxip-uclass.c b/drivers/mtd/nvmxip/nvmxip-uclass.c index 36eb056c213..9a316d1de39 100644 --- a/drivers/mtd/nvmxip/nvmxip-uclass.c +++ b/drivers/mtd/nvmxip/nvmxip-uclass.c @@ -29,10 +29,6 @@ int nvmxip_probe(struct udevice *udev) char bdev_name[NVMXIP_BLKDEV_NAME_SZ + 1]; int devnum; -#if CONFIG_IS_ENABLED(SANDBOX64) - sandbox_set_enable_memio(true); -#endif - devnum = uclass_id_count(UCLASS_NVMXIP); snprintf(bdev_name, NVMXIP_BLKDEV_NAME_SZ, "blk#%d", devnum); diff --git a/test/dm/nvmxip.c b/test/dm/nvmxip.c index 89bf481f616..f0ad47d4efe 100644 --- a/test/dm/nvmxip.c +++ b/test/dm/nvmxip.c @@ -103,6 +103,8 @@ static int dm_test_nvmxip(struct unit_test_state *uts) void *buffer = NULL; unsigned long flashsz; + sandbox_set_enable_memio(true); + /* set the flash content first for both devices */ dm_nvmxip_flash_sanity(uts, 0, NULL); dm_nvmxip_flash_sanity(uts, 1, NULL); From 77050a7398129b05888e9eb531bdf36b03041ed7 Mon Sep 17 00:00:00 2001 From: Marek Vasut Date: Wed, 23 Aug 2023 02:18:21 +0200 Subject: [PATCH 10/11] configs: sandbox: Enable NVMXIP QSPI driver Enable NVMXIP QSPI driver on sandbox, since it is already enabled on sandbox64. Signed-off-by: Marek Vasut --- configs/sandbox_defconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig index 0f01471367d..98f0bc13a5f 100644 --- a/configs/sandbox_defconfig +++ b/configs/sandbox_defconfig @@ -226,6 +226,7 @@ CONFIG_SPI_FLASH_SPANSION=y CONFIG_SPI_FLASH_STMICRO=y CONFIG_SPI_FLASH_SST=y CONFIG_SPI_FLASH_WINBOND=y +CONFIG_NVMXIP_QSPI=y CONFIG_MULTIPLEXER=y CONFIG_MUX_MMIO=y CONFIG_NVME_PCI=y From 36b900e8bd57fec7b4c200a368883e1e59e4f27f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Stelmach?= Date: Thu, 24 Aug 2023 08:10:25 +0200 Subject: [PATCH 11/11] setexpr: Silence some diagnostic messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Neither successful match nor lack thereof should be considered an extraordinary situation. Thus, neither require printing a message. Signed-off-by: Łukasz Stelmach Reviewed-by: Simon Glass --- cmd/setexpr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/setexpr.c b/cmd/setexpr.c index 4d671e7ef12..233471f6cb7 100644 --- a/cmd/setexpr.c +++ b/cmd/setexpr.c @@ -215,7 +215,7 @@ int setexpr_regex_sub(char *data, uint data_size, char *nbuf, uint nbuf_size, if (res == 0) { if (loop == 0) { - printf("%s: No match\n", data); + debug("%s: No match\n", data); return 1; } else { break; @@ -359,7 +359,7 @@ static int regex_sub_var(const char *name, const char *r, const char *s, if (ret) return 1; - printf("%s=%s\n", name, data); + debug("%s=%s\n", name, data); return env_set(name, data); }