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

[Tony Dinh]

Hi Stefan,

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.

I sent in the patch that should go into u-boot-marvell/next.

Thanks,
Tony