On Mon, 9 Jan 2023 at 21:34, Petr Štetiar <yn...@true.cz> wrote: > > Martin Rowe <martin.p.r...@gmail.com> [2023-01-03 03:20:49]: > > [Adding Florian, Tomasz and Oli to the Cc: loop] > > Hi, > > > I'm following up on this bug report from September ( > > https://lists.denx.de/pipermail/u-boot/2022-September/494811.html ) > > I also hit this issue on my Clearfog Pro when booting with eMMC. > > FYI we've recently got similar bug report in OpenWrt as well > https://github.com/openwrt/openwrt/issues/11661 > > There Oli is having problem to boot v2022.07 from SD card on his Clearfog Pro, > version v2021.01 was working fine for him. > > > Florian's bug report was incredibly helpful in isolating the issue. > > Indeed, that eMMC handling looks quite different, as it seems that BootROM is > looking at offset 0x0: > > -> BootROM - 1.73 > -> Booting from MMC > -> BootROM: Bad header at offset 00000000 > -> BootROM: Bad header at offset 00200000 > -> Switching BootPartitions. > -> BootROM: Invalid HDR source addr > > but in Oli's SD card boot log we can see, that it looks at 0x200 offset first: > > BootROM - 1.73 > Booting from MMC > BootROM: Bad header at offset 00000200 > BootROM: Bad header at offset 00004400 > BootROM: Bad header at offset 00200000 > > So perhaps SD and eMMC booting needs to be treated differently?
Much of the work that introduced these issues appears to have been making a single kwboot version that works for all cases, rather than having a board specific tool. When I tried creating a patch, that's where I got stuck because there doesn't seem to be enough information in the headers to decide between SD and eMMC. They both use the same final header address, so maybe there's a possible solution, though it still doesn't help with the sector size calculation. > > > After binary patching the the srcaddr (srcaddr = srcaddr * 512) in the > > > kwbimage v1 header, I got the BootROM to execute the SPL again. > > So this means following commit 501a54a29cc2 ("tools: kwbimage: Fix > generation of SATA, SDIO and PCIe images") ? As reverting that commit yields > following kwb image diff related to srcaddr changes: > > --- v2021.10-rc1-108-g501a54a29cc2.xxd 2023-01-09 20:37:08.250562742 +0100 > +++ v2021.10-rc1-108-g501a54a29cc2-revert.xxd 2023-01-09 > 20:37:17.902718024 +0100 > @@ -1,5 +1,5 @@ > -00000000: ae00 0000 0098 0600 0102 0080 4001 0000 ............@... > -00000010: c0ff 7f00 0000 8000 0000 0000 0000 01c2 ................ > +00000000: ae00 0000 4496 0600 0102 0080 0080 0200 ....D........... > +00000010: c0ff 7f00 0000 8000 0000 0000 0000 0145 ...............E > 00000020: 0201 60df 0200 0000 5b00 0000 6800 0000 ..`.....[...h... > 00000030: 0000 0000 0000 0000 0000 0000 0000 0000 ................ > 00000040: 0f00 00ea 14f0 9fe5 14f0 9fe5 14f0 9fe5 ................ > > Which in human readable form is following: > > --- v2021.10-rc1-108-g501a54a29cc2.header 2023-01-09 20:51:56.116867268 > +0100 > +++ v2021.10-rc1-108-g501a54a29cc2-revert.header 2023-01-09 > 20:52:55.987832692 +0100 > @@ -1,9 +1,9 @@ > blockid = 0xae, > nandeccmode = 0x0, > nandpagesize = 0x0, > -blocksize = 0x69800, > +blocksize = 0x69644, > rsvd1 = 0x80000201, > -srcaddr = 0x140, > +srcaddr = 0x28000, > destaddr = 0x7fffc0, > execaddr = 0x800000, > satapiomode = 0x0, > @@ -11,4 +11,4 @@ > ddrinitdelay = 0x0, > rsvd2 = 0x0, > ext = 0x1, > -checksum = 0xd8 > +checksum = 0x51 > > Does it mean, that going back to the commit before this change, thus commit > bd487ce08135 ("tools: kwbimage: Add constant for SDIO bootfrom") makes your > device bootable again from SD, SATA and eMMC? > > In other words, it would help to know, which commit after v2021.01 breaks > booting of SD/SATA/eMMC for you. Could you help find it? The commit from the git bisect is one of dozens that are associated with a major refactoring of kwboot and kwbimage; you probably want to go back to the previous merge commit to get a working build. > > I've gone a bit further and gotten eMMC boot working again, but I'm > > not a C programmer so this is just a proof of concept/hack, not a > > patch, and it would break SD card boot. Happy to test any proper > > patches, though! > > > > These steps require a working u-boot from another boot medium. I used > > SolidRun's u-boot-clearfog-pro-sata.kwb from > > https://images.solid-run.com/A38X/U-Boot > > So am I parsing it properly, that you're not even able to boot v2022.10 from > SATA? I didn't test SATA; I used a known working image to help isolate the problem and have a stable base to test from. > > Steps to fix eMMC boot: > > # In u-boot source > > 1. Apply the patches at the bottom of the email to v2022.10 release > > 2. make clearfog_defconfig > > 3. make menuconfig > > - Set CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR=0x1 > > - I also selected > > CONFIG_CLEARFOG_CON3_SATA=y > > CONFIG_CLEARFOG_CON2_SATA=y > > CONFIG_CLEARFOG_SFP_25GB=y > > CONFIG_DDR_RESET_ON_TRAINING_FAILURE=y > > but they should not affect eMMC booting > > 4. make > > 5. Copy u-boot-spl.kwb to booted linux on Clearfog Pro > > # In linux on Clearfog Pro > > 6. echo 0 > /sys/block/mmcblk0boot0/force_ro > > 7. dd if=/dev/zero of=/dev/mmcblk0boot0 conv=sync [probably not required] > > 8. dd if=u-boot-spl.kwb of=/dev/mmcblk0boot0 conv=sync > > 9. dd if=u-boot-spl.kwb of=/dev/mmcblk0 bs=512 seek=1 > > 10. reboot > > # In working u-boot on Clearfog Pro > > 11. mmc partconf 0 1 1 0 [doesn't seem to be needed, but recommended > > by board/solidrun/clearfog/README] > > # On board > > 12. Set boot select DIP to eMMC (00111) > > 13. Power on and boot succeeds > > > > The instructions also work with > > CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR=0x0 and step 9 changed to `dd > > if=u-boot-spl.kwb of=/dev/mmcblk0 conv=sync`, but this will clobber > > any partition table that exists on eMMC making it mostly useless other > > than for booting. > > > > Weirdly both the boot and the data parts of the eMMC need to have > > u-boot. It looks like SPL comes from a boot partition, but then tries > > to load u-boot proper from the data partition. This is also noted > > here: https://wiki.debian.org/InstallingDebianOn/ClearFog > > > > Pali Rohar appears to have made some significant improvements to mvebu > > booting around July 2021, but these changes seem to only account for > > an SD card for MMC boot, not eMMC which doesn't appear to need the > > sector (512) multiplier. > > Well, Oli has reported broken SD card booting as well, so probably UART/SPI > boot source is the only tested/working combination? > > > Most of the patches below remove special handling added for the > > IBR_HDR_SDIO_ID case which almost certainly breaks SD card booting. > > The removal of the error for CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR != 0 > > appears to have no side effects for other boot modes. > > > > The dts change is a long known issue and > > functionally matches a patch carried by armbian and discussed here: > > https://forum.armbian.com/topic/3072-clearfog-pro-emmc-requires-sd-card-to-detect-device/ > > > > Let me know if I can help get a permanent fix merged. > > > > Martin > > > > > > diff --git a/arch/arm/dts/armada-388-clearfog.dts > > b/arch/arm/dts/armada-388-clearfog.dts > > index e4164f49b2..29a608abcf 100644 > > --- a/arch/arm/dts/armada-388-clearfog.dts > > +++ b/arch/arm/dts/armada-388-clearfog.dts > > @@ -101,7 +101,7 @@ > > > > sdhci@d8000 { > > bus-width = <4>; > > - cd-gpios = <&gpio0 20 GPIO_ACTIVE_LOW>; > > + non-removable; > > no-1-8-v; > > pinctrl-0 = <µsom_sdhci_pins > > &clearfog_sdhci_cd_pins>; > > diff --git a/arch/arm/mach-mvebu/spl.c b/arch/arm/mach-mvebu/spl.c > > index ca2d5a59d7..00334562cf 100644 > > --- a/arch/arm/mach-mvebu/spl.c > > +++ b/arch/arm/mach-mvebu/spl.c > > @@ -44,9 +44,6 @@ > > #ifdef CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION > > #error CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION is unsupported > > #endif > > -#if defined(CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR) && > > CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR != 0 > > -#error CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR must be set to 0 > > -#endif > > #if defined(CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_DATA_PART_OFFSET) && \ > > CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_DATA_PART_OFFSET != 0 > > #error CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_DATA_PART_OFFSET must be set to 0 > > @@ -196,14 +193,6 @@ int spl_parse_board_header(struct spl_image_info > > *spl_image, > > spl_image->offset *= 512; > > } > > > > - /* > > - * For SDIO (eMMC) srcaddr is specified in number of sectors. > > - * This expects that sector size is 512 bytes and recalculates > > - * data offset to bytes. > > - */ > > - if (IS_ENABLED(CONFIG_SPL_MMC) && mhdr->blockid == IBR_HDR_SDIO_ID) > > - spl_image->offset *= 512; > > - > > if (spl_image->offset % 4 != 0) { > > printf("ERROR: Wrong srcaddr (0x%08x) in kwbimage\n", > > spl_image->offset); > > diff --git a/tools/kwbimage.c b/tools/kwbimage.c > > index 94b7685392..43b51ffd1a 100644 > > --- a/tools/kwbimage.c > > +++ b/tools/kwbimage.c > > @@ -1021,15 +1021,6 @@ static void *image_create_v0(size_t *imagesz, > > struct image_tool_params *params, > > if (main_hdr->blockid == IBR_HDR_SATA_ID) > > main_hdr->srcaddr = cpu_to_le32(headersz / 512 + 1); > > > > - /* > > - * For SDIO srcaddr is specified in number of sectors starting from > > - * sector 0. The main header is stored at sector number 0. > > - * This expects sector size to be 512 bytes. > > - * Header size is already aligned. > > - */ > > - if (main_hdr->blockid == IBR_HDR_SDIO_ID) > > - main_hdr->srcaddr = cpu_to_le32(headersz / 512); > > - > > /* For PCIe srcaddr is not used and must be set to 0xFFFFFFFF. */ > > if (main_hdr->blockid == IBR_HDR_PEX_ID) > > main_hdr->srcaddr = cpu_to_le32(0xFFFFFFFF); > > @@ -1478,15 +1469,6 @@ static void *image_create_v1(size_t *imagesz, > > struct image_tool_params *params, > > if (main_hdr->blockid == IBR_HDR_SATA_ID) > > main_hdr->srcaddr = cpu_to_le32(headersz / 512 + 1); > > > > - /* > > - * For SDIO srcaddr is specified in number of sectors starting from > > - * sector 0. The main header is stored at sector number 0. > > - * This expects sector size to be 512 bytes. > > - * Header size is already aligned. > > - */ > > - if (main_hdr->blockid == IBR_HDR_SDIO_ID) > > - main_hdr->srcaddr = cpu_to_le32(headersz / 512); > > - > > /* For PCIe srcaddr is not used and must be set to 0xFFFFFFFF. */ > > if (main_hdr->blockid == IBR_HDR_PEX_ID) > > main_hdr->srcaddr = cpu_to_le32(0xFFFFFFFF); > > @@ -2036,14 +2018,6 @@ static int kwbimage_verify_header(unsigned char > > *ptr, int image_size, > > offset *= 512; > > } > > > > - /* > > - * For SDIO srcaddr is specified in number of sectors. > > - * This expects that sector size is 512 bytes and recalculates > > - * data offset to bytes. > > - */ > > - if (blockid == IBR_HDR_SDIO_ID) > > - offset *= 512; > > - > > /* > > * For PCIe srcaddr is always set to 0xFFFFFFFF. > > * This expects that data starts after all headers. > > @@ -2405,9 +2379,6 @@ static int kwbimage_extract_subimage(void *ptr, > > struct image_tool_params *params > > offset *= 512; > > } > > > > - if (mhdr->blockid == IBR_HDR_SDIO_ID) > > - offset *= 512; > > - > > if (mhdr->blockid == IBR_HDR_PEX_ID && offset == 0xFFFFFFFF) > > offset = header_size; > > > > diff --git a/tools/kwboot.c b/tools/kwboot.c > > index da4fe32da2..188f944263 100644 > > --- a/tools/kwboot.c > > +++ b/tools/kwboot.c > > @@ -1894,10 +1894,6 @@ kwboot_img_patch(void *img, size_t *size, int > > baudrate) > > hdr->srcaddr = cpu_to_le32((srcaddr - 1) * 512); > > break; > > > > - case IBR_HDR_SDIO_ID: > > - hdr->srcaddr = cpu_to_le32(srcaddr * 512); > > - break; > > - > > case IBR_HDR_PEX_ID: > > if (srcaddr == 0xFFFFFFFF) > > hdr->srcaddr = cpu_to_le32(hdrsz); > > > > > > Kernel fix for eMMC detect pin for completeness (without it kernel > > won't see eMMC, but u-boot will): > > diff --git a/arch/arm/boot/dts/armada-388-clearfog.dtsi > > b/arch/arm/boot/dts/armada-388-clearfog.dtsi > > index f8a06ae4a3c9..2276844d26ca 100644 > > --- a/arch/arm/boot/dts/armada-388-clearfog.dtsi > > +++ b/arch/arm/boot/dts/armada-388-clearfog.dtsi > > @@ -42,7 +42,7 @@ sata@e0000 { > > > > sdhci@d8000 { > > bus-width = <4>; > > - cd-gpios = <&gpio0 20 GPIO_ACTIVE_LOW>; > > + non-removable; > > no-1-8-v; > > pinctrl-0 = <µsom_sdhci_pins > > &clearfog_sdhci_cd_pins>;