On Thu, Apr 11, 2024 at 05:27:08PM -0600, Sam Edwards wrote:
> Hi John,
>
> It doesn't look like I was sent the whole series (only 00 and 01), but
> I was able to find it on Patchwork and sift through it. A few general
> comments follow:
>
> The introduction of `SUNXI_BOOTED_FROM_SPINAND` is the right call,
> since the newer sunxis use this for SPI-NAND, and
> `SUNXI_BOOTED_FROM_SPI` for SPI-NOR. The older sunxis, however, will
> use `SUNXI_BOOTED_FROM_SPI` for "it's either SPI-NAND or SPI-NOR, have
> fun figuring out which." While the rationale in 09/15 ("Instead of
> trying to boot from SPI NAND then SPI NOR in series, select one based
> on the current boot device.") is solid, we still need some code
> (perhaps in/called from `sunxi_get_boot_device`?) to handle the
> `SUNXI_BOOTED_FROM_SPI` case by performing flash type detection --
> disabled for those sunxis known to use `SUNXI_BOOTED_FROM_SPINAND`, of
> course.
Is there already code that can do this probing somewhere in U-Boot?
I'd rather not try and support older boards with an ambiguous boot
method, they already don't work.
> 06/15 ("sunxi: enable support for SPI NAND booting on SUNIV") should
> be dropped from the series. You are updating `suniv_get_boot_source`
> when introducing `SUNXI_BOOTED_FROM_SPINAND` anyway, so you can just
> hook up `SUNIV_BOOTED_FROM_NAND` at that time (and a heads-up: I think
> you got it wired backwards in this version of the series).
Some of the redunancy in patches are from including Icenowy's patchset.
> On a more fundamental note: I am hesitant about the overall approach
> of having NAND reading code in `arch/arm/mach-sunxi/spl_spi_sunxi.c`.
> NANDs are more complex than NORs (requiring e.g. bad block handling)
> and U-Boot generally keeps the SPL load methods in
> `common/spl/spl_*.c`. It's good that the UBI-on-SPI-NAND method is
> hooked up there, but the "U-Boot image follows the (good) blocks of
> the SPL in NAND" method should probably live in the same directory.
>
> Here's what I'd like to explore: can we introduce a
> `common/spl/spl_spinand.c` and update the `drivers/mtd/nand/spi/*.c`
> code to support running in SPL? This way
> `arch/arm/mach-sunxi/spl_spi_sunxi.c` must only provide the SPL-mode
> implementation of SPI -- that is, the actual sunxi-specific bit. Also,
> updating the `drivers/mtd/nand/spi/*.c` drivers for SPL-friendliness
> will mean dropping those `SPL_SPINAND_{PAGE,BLOCK}_SIZE` configuration
> options: the tables in the drivers will be the ones providing those
> after NAND autodetection.
This is a little bit of a mess:
common/spl/spl_spi.c implements general SPI NOR reading
drivers/mtd/spi/sf-uclass.c wraps SPI flash access using DM
drivers/spi/spi-sunxi.c implements the SPI controller
arch/arm/mach-sunxi/spl_spi_sunxi.c overrides spl_spi
It would be good to somehow have spl_spi provide functions spl_spi
uses instead of having spl_spi_sunxi implement it directly.
common/spl/spl_nand.c implements general NAND reading
common/spl/spl_ubi.c implements UBI reading
drivers/mtd/nand/raw/sunxi_nand.c implements sunxi NAND reading
drivers/mtd/nand/raw/nand_spl_simple.c implmements a generic solution
drivers/mtd/nand/raw/sunxi_nand_spl.c implements sunxi SPL NAND reading
spl_ubi requires nand_spl_simple callbacks, sunxi_nand_spl doesn't
implement these.
It's possible spl_spi, spl_nand and spl_ubi all work if the DM code is
used for sunxi. Maybe that's a good goal here? Getting DM in SPL working
on these boards then hooking it in to spl_ubi seems like it could solve
some pain. I'm not too concerned about old SoCs at this point.
For non-DM Adding spl_spinand would work for the case of reading the
next good block. I'm not actually sure if the SPI NAND code supports the
bad block table yet. But we would still need an API for spl_ubi to
access.
>
> Thoughts?
>
> Cheers,
> Sam
John.
