On Mon, Jul 18, 2022 at 03:01:25PM +0200, Quentin Schulz wrote:
Hi Michal,
On 7/18/22 13:00, Michal Suchánek wrote:
On Mon, Jul 18, 2022 at 11:09:56AM +0200, Xavier Drudis Ferran wrote:
El Mon, Jul 18, 2022 at 10:33:18AM +0200, Quentin Schulz deia:
Hi Xavier,
On 7/15/22 18:30, Xavier Drudis Ferran wrote:
Spi0 is not needed in SPL and SPL could be a little smaller without it, but then the SF_DEFAULT_BOOT would have to be 0 to refer to spi1, and that's confusing, because once U-Boot proper runs, it numbers the bus 1.
Add spi0 to the pre-reloc and spl trees so that the flash is always connected to bus 1.
Mmmm... Could we instead make U-Boot use the bus number from the alias in the aliases DT node? I think the mmc subsystem does this already and it would mean we don't need to enable unnecessary devices. Also, relying on boot order for the bus number is brittle in Linux, I don't know about U-Boot, but if we can avoid this assumption, it'd be great :)
See: https://urldefense.proofpoint.com/v2/url?u=https-3A__source.denx.de_u-2Dboot... for how to do it today?
Maybe I should just drop this patch and try to define CONFIG_SPL_DM_SEQ_ALIAS in configs/rock-pi-4-rk3399 instead ? Let me test this a little...
I have CONFIG_DM_SEQ_ALIAS=y but CONFIG_SPL_DM_SEQ_ALIAS unset.
Your patch series got sent with each commit in their individual thread
I know. Sorry for the lapsus. I did it right in v1, wrong in v2, and will do right in v3.
What is actually the correct naming here?
We have in arch/arm/mach-rockchip/rk3399/rk3399.c
const char * const boot_devices[BROM_LAST_BOOTSOURCE + 1] = { [BROM_BOOTSOURCE_EMMC] = "/sdhci@fe330000", [BROM_BOOTSOURCE_SPINOR] = "/spi@ff1d0000/flash@0", [BROM_BOOTSOURCE_SD] = "/mmc@fe320000", };
} spl_boot_devices_tbl[] = { { BOOT_DEVICE_MMC1, "/mmc@fe320000" }, { BOOT_DEVICE_MMC2, "/sdhci@fe330000" }, { BOOT_DEVICE_SPI, "/spi@ff1d0000" }, };This one was incorrect for boards not redefining the aliases nodes for mmc devices from rk3399-u-boot.dtsi and I've sent a patch for it: https://lore.kernel.org/u-boot/20220715151552.953654-1-foss+uboot@0leil.net/
Which actually sounds reasonable.
which then presumably gets converted in common/spl/spl_mmc.c
SPL_LOAD_IMAGE_METHOD("MMC1", 0, BOOT_DEVICE_MMC1, spl_mmc_load_image); SPL_LOAD_IMAGE_METHOD("MMC2", 0, BOOT_DEVICE_MMC2, spl_mmc_load_image); SPL_LOAD_IMAGE_METHOD("MMC2_2", 0, BOOT_DEVICE_MMC2_2, spl_mmc_load_image);
I actually think from spl_mmc_get_device_index()?
The above gives the user-visible string.
spl_mmc_get_device_index establishes the off-by-one between boot devices and indexes:
static int spl_mmc_get_device_index(u32 boot_device) { switch (boot_device) { case BOOT_DEVICE_MMC1: return 0; case BOOT_DEVICE_MMC2: case BOOT_DEVICE_MMC2_2: return 1; }
We then have this in rk3399.dtsi:
sdio0: mmc@fe310000 { compatible = "rockchip,rk3399-dw-mshc", sdmmc: mmc@fe320000 { compatible = "rockchip,rk3399-dw-mshc", sdhci: mmc@fe330000 { compatible = "rockchip,rk3399-sdhci-5.1", "arasan,sdhci-5.1";and this in rk3399-u-boot.dtsi
mmc0 = &sdhci; mmc1 = &sdmmc;and this in arch/arm/dts/rk3399-pinebook-pro.dts
aliases { mmc0 = &sdio0; mmc1 = &sdmmc; mmc2 = &sdhci; };mmc@fe310000: 3 mmc@fe320000: 1 (SD) mmc@fe330000: 0 (eMMC)
Which would then mean that this is off-by-one and consistent with spl_mmc_get_device_index and the SoC dtsi but not the board dts.
It's also what's seen in upstream Linux
mmc0: SDHCI controller on fe330000.mmc [fe330000.mmc] using ADMA mmc0: new HS200 MMC card at address 0001 mmcblk0: mmc0:0001 DA4064
mmc1: new ultra high speed SDR104 SDHC card at address aaaa mmcblk1: mmc1:aaaa SC32G
mmc3: new ultra high speed SDR104 SDIO card at address 0001
Where does the 3 come from is a mystery.
This is not consistent with any of the above.
I think spl_decode_boot_device() assumes the index is the same for all rk3399 boards which does not seem to be the case for the Pinebook Pro (and is a bad assumption I can agree on that :) ). I guess a way to correctly
Or maybe it's not a good idea to override the aliases per-board.
But because there are u-boot specific aliases for the SoC, and board-specific aliases from Linux this is really a discrepancy between u-boot and Linux.
support your device would be to read the aliases node and do the mapping between DT's mmc0 and BOOT_DEVICE_MMC1. I am not sure there ever was an interest/desire to document/guarantee what a BOOT_DEVICE_MMCx would represent, see https://source.denx.de/u-boot/u-boot/-/blob/master/arch/arm/mach-rockchip/sp... (or maybe it's a convoluted way to say "BOOT_DEVICE_MMC1 is for mmc0, but mmc0 depends on your device tree definition"?) but I guess this mapping
But maybe it shoudn't because BOOT_DEVICE_MMC1 is also something that corresponds to a specific value passed from the boot rom, and then it should always mean the same thing on the same SoC.
would make sense. We need to keep the current implementation though, in order to support SPL without CONFIG_SPL_DM_SEQ_ALIAS enabled.
There's also no BOOT_DEVICE_MMC3 but that could be added pretty easily I guess. I assume you'd need a new entry in spl_mmc_get_device_index too.
It's not really bootable, anyway. There are only two mmc boot sources defined by boot rom. You could load u-boot from a non-botable mmc storage but in practice there is typically wifi on the third mmc interface.
Thanks
Michal