[PATCH v2 5/5] rockchip: rock-pi-4: dts: spi: Make the index of the spi flash the same in SPL and U-Boot proper

[Quentin Schulz]

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 at fe330000",
>          [BROM_BOOTSOURCE_SPINOR] = "/spi at ff1d0000/flash at 0",
>          [BROM_BOOTSOURCE_SD] = "/mmc at fe320000",
> };
> 
>          } spl_boot_devices_tbl[] = {
>                  { BOOT_DEVICE_MMC1, "/mmc at fe320000" },
>                  { BOOT_DEVICE_MMC2, "/sdhci at fe330000" },
>                  { BOOT_DEVICE_SPI, "/spi at 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 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 at fe310000 {
>                  compatible = "rockchip,rk3399-dw-mshc",
> 
>          sdmmc: mmc at fe320000 {
>                  compatible = "rockchip,rk3399-dw-mshc",
> 
>          sdhci: mmc at 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 at fe310000: 3
> mmc at fe320000: 1 (SD)
> mmc at 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