* [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
@ 2022-07-15 16:30 Xavier Drudis Ferran
2022-07-18 8:33 ` Quentin Schulz
0 siblings, 1 reply; 10+ messages in thread
From: Xavier Drudis Ferran @ 2022-07-15 16:30 UTC (permalink / raw)
To: u-boot; +Cc: Simon Glass, Philipp Tomsich, Kever Yang
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.
Changes since v1:
- new patch in v2.
Signed-off-by: Xavier Drudis Ferran <xdrudis@tinet.cat>
Cc: Simon Glass <sjg@chromium.org>
Cc: Philipp Tomsich <philipp.tomsich@vrull.eu>
Cc: Kever Yang <kever.yang@rock-chips.com>
---
arch/arm/dts/rk3399-rock-pi-4-u-boot.dtsi | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/arm/dts/rk3399-rock-pi-4-u-boot.dtsi b/arch/arm/dts/rk3399-rock-pi-4-u-boot.dtsi
index 4c2fe8f6bc..68b6b752d6 100644
--- a/arch/arm/dts/rk3399-rock-pi-4-u-boot.dtsi
+++ b/arch/arm/dts/rk3399-rock-pi-4-u-boot.dtsi
@@ -16,6 +16,12 @@
regulator-init-microvolt = <950000>;
};
+/* not needed, but nicer to keep SF_DEFAULT_BUS in SPL the same index as in U-Boot proper */
+&spi0 {
+ u-boot,dm-pre-reloc;
+ status = "okay";
+};
+
&spi1 {
status = "okay";
spi-max-frequency = <40000000>;
--
2.20.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* 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
2022-07-15 16:30 [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 Xavier Drudis Ferran
@ 2022-07-18 8:33 ` Quentin Schulz
2022-07-18 8:39 ` Michal Suchánek
2022-07-18 9:09 ` Xavier Drudis Ferran
0 siblings, 2 replies; 10+ messages in thread
From: Quentin Schulz @ 2022-07-18 8:33 UTC (permalink / raw)
To: Xavier Drudis Ferran, u-boot; +Cc: Simon Glass, Philipp Tomsich, Kever Yang
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://source.denx.de/u-boot/u-boot/-/commit/2243d19e5618122d9d7aba23eb51f63f2719dba5
for how to do it today?
> Changes since v1:
> - new patch in v2.
>
Your patch series got sent with each commit in their individual thread
instead of the usual way, with 1/X patch as the first mail in the
thread. I think this could make this difficult to merge for the
maintainer, could you please try to put all your patch into a single
thread for the next version/patch series you will send? `git send-email
*.patch` should do that for you by default I'm pretty sure.
What happened with your patches:
https://lore.kernel.org/u-boot/20220715162712.GB2143@begut/T/#u
https://lore.kernel.org/u-boot/20220715162802.GC2143@begut/T/#u
What should have had happened:
https://lore.kernel.org/u-boot/20220715163435.1725-1-afd@ti.com/T/#m6339d543cec31cf6a30355d0e44fd3b99492a30d
Cheers,
Quentin
^ permalink raw reply [flat|nested] 10+ messages in thread
* 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
2022-07-18 8:33 ` Quentin Schulz
@ 2022-07-18 8:39 ` Michal Suchánek
2022-07-18 9:09 ` Xavier Drudis Ferran
1 sibling, 0 replies; 10+ messages in thread
From: Michal Suchánek @ 2022-07-18 8:39 UTC (permalink / raw)
To: Quentin Schulz
Cc: Xavier Drudis Ferran, u-boot, Simon Glass, Philipp Tomsich, Kever Yang
On Mon, Jul 18, 2022 at 10:33:18AM +0200, Quentin Schulz wrote:
> 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
It does not seem to work for mmc, though.
I have mmc2 and mmc1 in SPL, and mmc1 and mmc0 in u-boot.
There are actually 3 mmc interfaces using 2 drivers so the situations is
.. complex.
Thanks
Michal
^ permalink raw reply [flat|nested] 10+ messages in thread
* 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
2022-07-18 8:33 ` Quentin Schulz
2022-07-18 8:39 ` Michal Suchánek
@ 2022-07-18 9:09 ` Xavier Drudis Ferran
2022-07-18 11:00 ` Michal Suchánek
1 sibling, 1 reply; 10+ messages in thread
From: Xavier Drudis Ferran @ 2022-07-18 9:09 UTC (permalink / raw)
To: Quentin Schulz
Cc: Xavier Drudis Ferran, u-boot, Simon Glass, Philipp Tomsich, Kever Yang
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://source.denx.de/u-boot/u-boot/-/commit/2243d19e5618122d9d7aba23eb51f63f2719dba5
> 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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* 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
2022-07-18 9:09 ` Xavier Drudis Ferran
@ 2022-07-18 11:00 ` Michal Suchánek
2022-07-18 13:01 ` Quentin Schulz
2022-07-18 13:14 ` Xavier Drudis Ferran
0 siblings, 2 replies; 10+ messages in thread
From: Michal Suchánek @ 2022-07-18 11:00 UTC (permalink / raw)
To: Xavier Drudis Ferran
Cc: Quentin Schulz, u-boot, Simon Glass, Philipp Tomsich, Kever Yang
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://source.denx.de/u-boot/u-boot/-/commit/2243d19e5618122d9d7aba23eb51f63f2719dba5
> > 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" },
};
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);
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.
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.
Thanks
Michal
^ permalink raw reply [flat|nested] 10+ messages in thread
* 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
2022-07-18 11:00 ` Michal Suchánek
@ 2022-07-18 13:01 ` Quentin Schulz
2022-07-18 14:06 ` Michal Suchánek
2022-08-27 3:51 ` Kever Yang
2022-07-18 13:14 ` Xavier Drudis Ferran
1 sibling, 2 replies; 10+ messages in thread
From: Quentin Schulz @ 2022-07-18 13:01 UTC (permalink / raw)
To: Michal Suchánek, Xavier Drudis Ferran
Cc: u-boot, Simon Glass, Philipp Tomsich, Kever Yang
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 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* 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
2022-07-18 11:00 ` Michal Suchánek
2022-07-18 13:01 ` Quentin Schulz
@ 2022-07-18 13:14 ` Xavier Drudis Ferran
2022-07-18 14:09 ` Michal Suchánek
1 sibling, 1 reply; 10+ messages in thread
From: Xavier Drudis Ferran @ 2022-07-18 13:14 UTC (permalink / raw)
To: Michal Suchánek
Cc: Xavier Drudis Ferran, Quentin Schulz, u-boot, Simon Glass,
Philipp Tomsich, Kever Yang
El Mon, Jul 18, 2022 at 01:00:03PM +0200, Michal Suchánek deia:
> mmc@fe310000: 3
> mmc@fe320000: 1 (SD)
> mmc@fe330000: 0 (eMMC)
>
> This is not consistent with any of the above.
>
I agree, but this is mmc, and this thread was about spi.
^ permalink raw reply [flat|nested] 10+ messages in thread
* 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
2022-07-18 13:01 ` Quentin Schulz
@ 2022-07-18 14:06 ` Michal Suchánek
2022-08-27 3:51 ` Kever Yang
1 sibling, 0 replies; 10+ messages in thread
From: Michal Suchánek @ 2022-07-18 14:06 UTC (permalink / raw)
To: Quentin Schulz
Cc: Xavier Drudis Ferran, u-boot, Simon Glass, Philipp Tomsich, Kever Yang
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* 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
2022-07-18 13:14 ` Xavier Drudis Ferran
@ 2022-07-18 14:09 ` Michal Suchánek
0 siblings, 0 replies; 10+ messages in thread
From: Michal Suchánek @ 2022-07-18 14:09 UTC (permalink / raw)
To: Xavier Drudis Ferran
Cc: Quentin Schulz, u-boot, Simon Glass, Philipp Tomsich, Kever Yang
On Mon, Jul 18, 2022 at 03:14:33PM +0200, Xavier Drudis Ferran wrote:
> El Mon, Jul 18, 2022 at 01:00:03PM +0200, Michal Suchánek deia:
>
> > mmc@fe310000: 3
> > mmc@fe320000: 1 (SD)
> > mmc@fe330000: 0 (eMMC)
> >
> > This is not consistent with any of the above.
> >
>
> I agree, but this is mmc, and this thread was about spi.
Sorry for hijacking your thread then.
I was probably looking for the other patch that adjusts the mmc indexes.
Thanks
Michal
^ permalink raw reply [flat|nested] 10+ messages in thread
* 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
2022-07-18 13:01 ` Quentin Schulz
2022-07-18 14:06 ` Michal Suchánek
@ 2022-08-27 3:51 ` Kever Yang
1 sibling, 0 replies; 10+ messages in thread
From: Kever Yang @ 2022-08-27 3:51 UTC (permalink / raw)
To: Quentin Schulz, Michal Suchánek, Xavier Drudis Ferran
Cc: u-boot, Simon Glass, Philipp Tomsich
Hi Michal, Quentin, Xavier,
On 2022/7/18 21:01, 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 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.
>>
For the boot sequence in SPL and U-Boot proper, two issue here:
1. mmc index
There is no alias for mmc few years ago when we add alias for mmc
in U-Boot, we would like to make the boot order like this:
SDMMC -> eMMC
And SDIO is not in consideration because it's not storage in most
case, we add alias for mmc so that we can use the boot script in
include/configs/rockchip-common.h to get the Fixed boot order:
/* First try to boot from SD (index 1), then eMMC (index 0) */
The SD card is removable and easy to replace, and not easy to break
the board system, it's better to boot from SD card if there is available
firmware inside.
And after this rule in U-Boot works with boot script and mmc alias
in -u-boot.dtsi, the kernel also add the alias without consideration for
the boot order, then we can see the difference in u-boot dts now.
2. boot order in SPL and U-Boot proper
For SPL, it's recommend to follow the BootRom boot device, because
the loader until U-Boot usually in the same storage media;
For U-Boot proper, it's per board different:
- for most SBC and product, it only support eMMC and SD, we would like
to boot from SD card first so that we can upgrade the firmware to eMMC;
- for device with SPI, system will boot from SPI in loader stage, and
boot from eMMC or NvMe in U-Boot proper for OS;
Hope the information is enough for you.
Thanks,
- Kever
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-08-27 3:52 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-15 16:30 [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 Xavier Drudis Ferran
2022-07-18 8:33 ` Quentin Schulz
2022-07-18 8:39 ` Michal Suchánek
2022-07-18 9:09 ` Xavier Drudis Ferran
2022-07-18 11:00 ` Michal Suchánek
2022-07-18 13:01 ` Quentin Schulz
2022-07-18 14:06 ` Michal Suchánek
2022-08-27 3:51 ` Kever Yang
2022-07-18 13:14 ` Xavier Drudis Ferran
2022-07-18 14:09 ` Michal Suchánek
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.