embeddedsw icon indicating copy to clipboard operation
embeddedsw copied to clipboard

sw_apps: zynqmp_fsbl: Fixed setting of secondary boot device

Open martinexner opened this issue 3 years ago • 2 comments

In zynqmp_fsbl, two different sets of macros with different values are used to compare the value of FsblInstancePtr->SecondaryBootDevice:

  • XFsbl_SecondaryBootDeviceInit uses XIH_IHT_PPD_*
  • XFsbl_ValidateHeader uses XFSBL_*_BOOT_MODE

This can lead to unexpected behavior as their values do not correspond:

XIH_IHT_PPD_SAME   == XFSBL_JTAG_BOOT_MODE   == 0
XIH_IHT_PPD_QSPI32 == XFSBL_QSPI24_BOOT_MODE == 1
XIH_IHT_PPD_QSPI24 == XFSBL_QSPI32_BOOT_MODE == 2
XIH_IHT_PPD_NAND   == XFSBL_SD0_BOOT_MODE    == 3
XIH_IHT_PPD_SD_0   == XFSBL_NAND_BOOT_MODE   == 4
XIH_IHT_PPD_SD_1   == XFSBL_SD1_BOOT_MODE    == 5
XIH_IHT_PPD_SD_LS  == XFSBL_EMMC_BOOT_MODE   == 6
XIH_IHT_PPD_MMC    == XFSBL_USB_BOOT_MODE    == 7

In more detail:

  1. SecondaryBootDevice is set to the partition present device value read from the image header (for example XIH_IHT_PPD_NAND which is 0x3, see step 4), then, XFsbl_SecondaryBootDeviceInit is called.

https://github.com/Xilinx/embeddedsw/blob/1bfd47e362820c842c1d04214293de21334d3da7/lib/sw_apps/zynqmp_fsbl/src/xfsbl_initialization.c#L428

u32 XFsbl_BootDeviceInitAndValidate(XFsblPs * FsblInstancePtr)
{​
	// ...
	/**
	 * Update the secondary boot device
	 */
	FsblInstancePtr->SecondaryBootDevice =
	 FsblInstancePtr->ImageHeader.ImageHeaderTable.PartitionPresentDevice;

	/**
	 *  Configure the secondary boot device if required
	 */
	if (FsblInstancePtr->SecondaryBootDevice !=
			FsblInstancePtr->PrimaryBootDevice) {
		Status = XFsbl_SecondaryBootDeviceInit(FsblInstancePtr);
		if (XFSBL_SUCCESS != Status) {
			goto END;
		}
	}

END:
	return Status;
}
  1. XFsbl_SecondaryBootDeviceInit switches depending on the previously set SecondaryBootDevice and uses the correct macros (for example XIH_IHT_PPD_NAND), then calls XFsbl_ValidateHeader

https://github.com/Xilinx/embeddedsw/blob/1bfd47e362820c842c1d04214293de21334d3da7/lib/sw_apps/zynqmp_fsbl/src/xfsbl_initialization.c#L1402

static u32 XFsbl_SecondaryBootDeviceInit(XFsblPs * FsblInstancePtr)
{
	// ...
	switch (FsblInstancePtr->SecondaryBootDevice) {
	// ...
	case XIH_IHT_PPD_NAND: {
		XFsbl_Printf(DEBUG_GENERAL, "NAND Boot Mode \n\r");
		// ...
	}
	// ...
	}
	// ...
	/**
	 * Initialize the Secondary Boot Device Driver
	 */
	if (Status == XFSBL_SUCCESS) {
		// ...
		if(Status == XFSBL_SUCCESS) {
			Status = XFsbl_ValidateHeader(FsblInstancePtr);
			// ...
		}
	}

}
  1. XFsbl_ValidateHeader, however, compares SecondaryBootDevice with the wrong macros (e.g. XFSBL_SD0_BOOT_MODE, which is also 0x3, see step 4)

https://github.com/Xilinx/embeddedsw/blob/1bfd47e362820c842c1d04214293de21334d3da7/lib/sw_apps/zynqmp_fsbl/src/xfsbl_initialization.c#L1151

static u32 XFsbl_ValidateHeader(XFsblPs * FsblInstancePtr)
{
	// ...
	else
	{
		if (!((FsblInstancePtr->SecondaryBootDevice == XFSBL_SD0_BOOT_MODE)
				|| (FsblInstancePtr->SecondaryBootDevice == XFSBL_EMMC_BOOT_MODE)
				|| (FsblInstancePtr->SecondaryBootDevice == XFSBL_SD1_BOOT_MODE)
				|| (FsblInstancePtr->SecondaryBootDevice == XFSBL_SD1_LS_BOOT_MODE)
				|| (FsblInstancePtr->SecondaryBootDevice == XFSBL_USB_BOOT_MODE))) {
			FsblInstancePtr->ImageOffsetAddress = MultiBootOffset
					* XFSBL_IMAGE_SEARCH_OFFSET;
		}
	}
	// ...
}
  1. XIH_IHT_PPD_NAND and XFSBL_SD0_BOOT_MODE are both defined as 0x3

https://github.com/Xilinx/embeddedsw/blob/1bfd47e362820c842c1d04214293de21334d3da7/lib/sw_apps/zynqmp_fsbl/src/xfsbl_image_header.h#L122

https://github.com/Xilinx/embeddedsw/blob/1bfd47e362820c842c1d04214293de21334d3da7/lib/sw_apps/zynqmp_fsbl/src/xfsbl_main.h#L117

martinexner avatar Jul 01 '21 15:07 martinexner

@martinexner It is a bug. Thanks for identifying. This will be fixed in 2021.2 release. Will review the patch and get back on it in sometime. Thank You.

bsvikram avatar Jul 07 '21 09:07 bsvikram

@martinexner The logic for your patch is correct but I observed that there are too many changes. I fixed it with fewer changes( lines of code). I just assigned FsblInstancePtr->SecondaryBootDevice = SecBootMode; below line 1589 in xfsbl_initialization.c Going forward, please mail to [email protected] for any issues / bugs you find. We do not accept pull requests. Thank You again for finding the bug.

bsvikram avatar Jul 11 '21 09:07 bsvikram