[PATCH 2/2] arm: mvebu: clearfog: Add defconfig for SPI booting

[Tony Dinh]

Hi Pali,

On Mon, Feb 27, 2023 at 3:41 PM Tony Dinh <mibodhi at gmail.com> wrote:
>
> Hi Pali,
>
> On Mon, Feb 27, 2023 at 1:57 PM Tony Dinh <mibodhi at gmail.com> wrote:
> >
> > Hi Stefan,
> >
> > On Sun, Feb 26, 2023 at 11:40 PM Stefan Roese <sr at denx.de> wrote:
> > >
> > > Hi Tony,
> > >
> > > On 2/27/23 01:11, Tony Dinh wrote:
> > > > Hi Pali,
> > > >
> > > > On Sun, Feb 26, 2023 at 2:52 AM Pali Rohár <pali at kernel.org> wrote:
> > > >>
> > > >> On Saturday 25 February 2023 20:56:10 Tony Dinh wrote:
> > > >>> Hi Martin,
> > > >>>
> > > >>> On Sat, Feb 25, 2023 at 6:17 PM Martin Rowe <martin.p.rowe at gmail.com> wrote:
> > > >>>>
> > > >>>>>>> I'm not sure how to run proper timing tests on the process, but
> > > >>>>>>> stopwatch timing just between seeing "Trying to boot" and "U-Boot
> > > >>>>>>> 2023.04-rc2" showed the return to BootROM under 1 second, and the
> > > >>>>>>> direct from SPI around 4 seconds. I thought the goal of loading from
> > > >>>>>>> SPI directly was speed, but returning to BootROM is significantly
> > > >>>>>>> faster on this board.
> > > >>>>>>
> > > >>>>>> You should check SPI speed in DTS file and also in the defconfig.
> > > >>>>>
> > > >>>>> I think we have probably seen this slowdown before. There is a TODO in
> > > >>>>> the way the DTS nodes are parsed by DM uclass. So this config must be
> > > >>>>> set in defconfig to get around that bug:
> > > >>>>> CONFIG_SF_DEFAULT_SPEED=50000000
> > > >>>>
> > > >>>> That works and is much faster now. I'll submit it in a V2 for these
> > > >>>> two patches once Pali's mvebu changes are accepted. Only question I
> > > >>>> have is: should the faster speed be applied to all three defconfigs?
> > > >>>> CONFIG_SF_DEFAULT_SPEED=1000000 (50x less) is implicitly added to the
> > > >>>> MMC and SATA configs at the moment.
> > > >>>
> > > >>> This is only a workaround to get the SPI probe to work correctly. The
> > > >>> real fix should be in spi_flash_probe()
> > > >>> ./common/spl/spl_spi.c
> > > >>>          flash = spi_flash_probe(sf_bus, sf_cs,
> > > >>>                                  CONFIG_SF_DEFAULT_SPEED,
> > > >>>                                  CONFIG_SF_DEFAULT_MODE);
> > > >>> If spi_flash_probe() failed to get SPI max_hz from the DTS, the
> > > >>> fallback is CONFIG_SF_DEFAULT_SPEED.
> > > >>>
> > > >>> IMO, perhaps we could add CONFIG_SF_DEFAULT_SPEED and
> > > >>> CONFIG_SF_DEFAULT_MODE selection to arch/arm/mach-mvebu/Kconfig or
> > > >>> some other common place. And when we will have fixed the DTS parsing
> > > >>> problem, they can be removed.
> > > >>>
> > > >>> I'd like to hear from Stefan and Pali if this approach sounds good.
> > > >>>
> > > >>> All the best,
> > > >>> Tony
> > > >>
> > > >> Well, the maximal SPI speed depends on the wiring. You can have on the
> > > >> board some SPI device which does not support high frequency.
> > > >>
> > > >> But having some sane defaults in Kconfig for mvebu makes sense.
> > >
> > > Agreed.
> > >
> > > > The CONFIG_SF_DEFAULT_SPEED is set to 1_000_000 if the board defconfig
> > > > does not specify it. I think the sane default value is 10_000_000
> > > > (grepping the Armada* DTS files showing the slowest spi-max-frequency
> > > > is 10_000_000). While a big improvement, it is not fast enough for
> > > > many other boards. So we still need to define it in those board
> > > > defconfigs (before fixing that bug), or use return to BootROM.
> > >
> > > I'm fine with setting CONFIG_SF_DEFAULT_SPEED to 10.000.000 for MVEBU.
> > > Not sure if this should be done in drivers/mtd/spi/Kconfig, as this
> > > currently has no platform- / board- specific configurations. Perhaps
> > > it can be done in a mach-mvebu Kconfig file instead?
> >
> > Yes, I think it should be done in mach-mvebu Kconfig, too. I will run
> > some tests.
>
> It is not related to this patch series (I also tested without the
> patch series to confirm). But it is strange that I can no longer get
> the configuration to boot from SPI. The 1st device in the boot order
> is alway BOOTROM. The spl_boot_list is printed out below.
>
> <BEGIN LOG>
> High speed PHY - Ended Successfully
> mv_ddr: 14.0.0
> DDR4 Training Sequence - Switching XBAR Window to FastPath Window
> mv_ddr: completed successfully
> board_boot_order spl_boot_list[0] = 15
> Trying to boot from BOOTROM
> Returning to BootROM (return address 0xffff05c4)...
> BootROM: Image checksum verification PASSED
> <END LOG>
>
> The SPL SPI configs (board Thecus N2350) are:
> # grep SPL .config| grep SPI
>
> CONFIG_MVEBU_SPL_BOOT_DEVICE_SPI=y
> CONFIG_SPL_DM_SPI=y
> CONFIG_SPL_SPI_FLASH_SUPPORT=y
> CONFIG_SPL_SPI=y
> CONFIG_SPL_DM_SPI_FLASH=y
> CONFIG_SPL_SPI_FLASH_TINY=y
> # CONFIG_SPL_SPI_FLASH_MTD is not set
> CONFIG_SPL_SPI_LOAD=y
>
> Did I miss something new lately?
>
> Thanks,
> Tony
>
> Trying to boot from BOOTROM
> Returning to BootROM (return address 0xffff05c4)...
> BootROM: Image checksum verification PASSED

It turns out that the board strapping register itself is the problem.
boot_device=0x9 was printed out in arch/arm/mach-mvebu/cpu.c. It
surely does not match what we expected for A38x  (#define
BOOT_FROM_SPI 0x32). Actually 0x9 is not defined in cpu.c at all. So
it fell to the default case, which is BOOTROM.

<BEGIN LOG>
U-Boot SPL 2023.04-rc2-tld-1-00089-g3fe03f96fc-dirty (Feb 27 2023 -
16:24:01 -0800)
High speed PHY - Version: 2.0
Detected Device ID 6820
board SerDes lanes topology details:
 | Lane # | Speed |  Type       |
 --------------------------------
 |   0    |   0   | SGMII0 |
 |   1    |   3   | SATA0 |
 |   2    |   3   | SATA1 |
 |   4    |   5   | USB3 HOST0 |
 |   5    |   5   | USB3 HOST1 |
 --------------------------------
High speed PHY - Ended Successfully
mv_ddr: 14.0.0
DDR4 Training Sequence - Switching XBAR Window to FastPath Window
mv_ddr: completed successfully
BOOTROM_REG=0x97001000 boot_device=0x9
spl_boot_device boot_device = 15
board_boot_order spl_boot_list[0] = 15
Trying to boot from BOOTROM
Returning to BootROM (return address 0xffff05c4)...
BootROM: Image checksum verification PASSED
<END LOG>

Is there a chance this value 0x9 means something that we have not come across?

Thanks,
Tony