Re: [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

["Michal Suchánek" <msuchanek@suse.de>]

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+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/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