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
