Re: [EXT] [PATCH] ARM: imx: romapi: Repair FlexSPI NOR boot offset

[Marek Vasut <marex@denx.de>]

On 3/31/22 20:02, Tim Harvey wrote:
> On Thu, Mar 31, 2022 at 9:41 AM Marek Vasut <marex@denx.de> wrote:
>>
>> On 3/31/22 18:03, Tim Harvey wrote:
>>> On Thu, Mar 31, 2022 at 8:26 AM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 3/31/22 17:09, Tim Harvey wrote:
>>>>> On Mon, Mar 28, 2022 at 7:55 AM Marek Vasut <marex@denx.de> wrote:
>>>>>>
>>>>>> On 3/28/22 08:54, Ye Li wrote:
>>>>>>> Hi Marek,
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>>>> 2. Update the u-boot.itb offset in imx8mp-u-boot.dtsi, set the
>>>>>>>>> offset
>>>>>>>>> to 0x5f000.  The previous offset 0x58000 is for SD, calculated by
>>>>>>>>> 0x60000 - 0x8000 (32KB image offset).
>>>>>>>>>
>>>>>>>>>                   uboot: blob-ext@2 {
>>>>>>>>>                           filename = "u-boot.itb";
>>>>>>>>>                           offset = <0x5f000>;
>>>>>>>>>                   };
>>>>>>>> But that breaks booting from SD card for me ?
>>>>>>>
>>>>>>> Do you want to use one flash.bin for both SD and flexspi?
>>>>>>
>>>>>> Yes, the board I use can boot from SD/eMMC/FlexSPI. I don't want to
>>>>>> build multiple confusing "flash.bin" files, one for each boot media.
>>>>>>
>>>>>>> When first introduced 8m support by imx8mimage.c, we expected the u-
>>>>>>> boot.itb at same device offset (0x60000) on SD/emmc and flexspi. The
>>>>>>> imx8mimage will calculate the offset inside the flash.bin automatically
>>>>>>> according to different IVT offset. The ROMAPI driver also works
>>>>>>> correspondingly.
>>>>>>> After using binman, the u-boot.itb offset inside the flash.bin has to
>>>>>>> be manually set in this DTS node. To follow the original design, this
>>>>>>> offset should be different. That's why I asked to update this dts node
>>>>>>> for flexspi.
>>>>>>
>>>>>> This does imply that there are currently no users that boot from flexspi
>>>>>> in upstream U-Boot, because such users would have to manually modify
>>>>>> both arch/arm/dts/imx8m?-u-boot.dtsi and board/*/imximage.cfg to
>>>>>> generate suitable flash.bin which can be started from FlexSPI.
>>>>>>
>>>>>> Also, git grep confirms that there are no users:
>>>>>>
>>>>>> u-boot$ git grep BOOT_FROM.*fspi
>>>>>> doc/imx/mkimage/imx8image.txt:BOOT_FROM
>>>>>> [sd|emmc_fastboot|fspi|nand_4k|nand_8k|nand_16k] [sector_size]
>>>>>>
>>>>>>> If you change the ROM API driver, that will break our design. You can
>>>>>>> try to overwrite spl_romapi_get_uboot_base for your board only.
>>>>>>
>>>>>> Since there are no users which boot from flexspi upstream, this design
>>>>>> can still be fixed such that it does not require different flash.bin for
>>>>>> different boot media, but rather one flash.bin works on all boot media.
>>>>>> I think that is much better.
>>>>>>
>>>>>
>>>>> Marek,
>>>>>
>>>>> I'm also a fan of single U-Boot binaries supporting multiple boards
>>>>> and configurations.
>>>>>
>>>>> How are you handling env for the various boot devices? I haven't
>>>>> looked in a while... can multiple env support co-exist now between
>>>>> MMC/SPI and be selected at runtime?
>>>>
>>>> Do '$ git grep env_get_location' , you will find board-level examples
>>>> where this function is implemented and it returns ENVL_* based on
>>>> various hardware-specific conditions.
>>>>
>>>> I _think_ there is even imx arch specific implementation for this
>>>> env_get_location() which automatically picks SD/eMMC/FlexSPI for you
>>>> right now.
>>>
>>> Marek,
>>>
>>> I guess where I keep getting hung up is how to configure the ENV for
>>> multiple env drivers as there is only one CONFIG_ENV_OFFSET Kconfig
>>> which means for all devices your going to have the same offset/redund
>>> configuration which is likely not appropriate for mmc/nand/spi for
>>> example. I don't see any way currently to configure those separately.
>>> Unless I'm missing something obvious I suppose get_env_location()
>>> could be augmented to be able to return board-specific env location
>>> config as well.
>>
>> e.g. env/mmc.c has the following:
>>
>>    78         } dt_prop = {
>>    79                 .offset_redund = "u-boot,mmc-env-offset-redundant",
>>    80                 .partition = "u-boot,mmc-env-partition",
>>    81                 .offset = "u-boot,mmc-env-offset",
>>    82         };
>>
>> So use that to specify per-env-driver offset. Indeed, the
>> CONFIG_ENV*OFFSET is shared by all env driver.
> 
> Right... but I think the dt overrides there only exist for MMC (I
> don't see then in env/nand.c for example)... but I can add that. I've
> always wanted to have a single binary for IMX6 nand vs emmc boards and
> this has been the ony hangup.

Then I think, send a patch for env SF. The DT overrides seem to be the 
right approach, it is certainly better than hard-coded Kconfigs.