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 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()?
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)
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 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 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.
Also on upstream kernel eMMC is mmc0 and SD mmc1 (somewhat consistently
with the above), while on downstream kernel it seems these are mmc1 and
mmc2, respectively.
I'm afraid that will not be the only discrepancy you'll stumble upon
between upstream and downstream.
Cheers,
Quentin
