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_u-2Dboot_-2D_commit_2243d19e5618122d9d7aba23eb51f63f2719dba5&d=DwIBAg&c=_sEr5x9kUWhuk4_nFwjJtA&r=LYjLexDn7rXIzVmkNPvw5ymA1XTSqHGq8yBP6m6qZZ4njZguQhZhkI_-172IIy1t&m=G-LrZmlVXqwFFza02frCo5F8vUCol4vDYe6RpSOpezwdgWuNQZyIB2hF_SNO4Gz3&s=O9wEK10SUOQNv_9zcY0K2oSdD_soaQtgjga-pw9nAgY&e= > > > > 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+ub...@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/spl-boot-order.c#L18-23 > (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