[RFC][PATCH] mtd: spi: Set CONFIG_SF_DEFAULT_MODE default to 0

[Tom Rini]

On Tue, Sep 14, 2021 at 05:44:25PM -0400, Tom Rini wrote:
> On Tue, Sep 14, 2021 at 11:30:01PM +0200, Marek Vasut wrote:
> > On 9/14/21 10:45 PM, Tom Rini wrote:
> > > On Tue, Sep 14, 2021 at 10:10:22PM +0200, Marek Vasut wrote:
> > > > On 9/14/21 9:19 PM, Tom Rini wrote:
> > > > > On Tue, Sep 14, 2021 at 08:28:24PM +0200, Marek Vasut wrote:
> > > > > 
> > > > > > Before e2e95e5e254 ("spi: Update speed/mode on change") most systems
> > > > > > silently defaulted to SF bus mode 0. Now the mode is always updated,
> > > > > > which causes breakage. It seems most SF which are used as boot media
> > > > > > operate in bus mode 0, so switch that as the default.
> > > > > > 
> > > > > > This should fix booting at least on Altera SoCFPGA, ST STM32, Xilinx
> > > > > > ZynqMP, NXP iMX and Rockchip SoCs, which recently ran into trouble
> > > > > > with mode 3. Marvell Kirkwood and Xilinx microblaze need to be checked
> > > > > > as those might need mode 3.
> > > > > > 
> > > > > > Signed-off-by: Marek Vasut <marex at denx.de>
> > > > > > Cc: Aleksandar Gerasimovski <aleksandar.gerasimovski at hitachi-powergrids.com>
> > > > > > Cc: Andreas Biessmann <andreas at biessmann.org>
> > > > > > Cc: Eugen Hristev <eugen.hristev at microchip.com>
> > > > > > Cc: Michal Simek <michal.simek at xilinx.com>
> > > > > > Cc: Patrice Chotard <patrice.chotard at foss.st.com>
> > > > > > Cc: Patrick Delaunay <patrick.delaunay at foss.st.com>
> > > > > > Cc: Peng Fan <peng.fan at nxp.com>
> > > > > > Cc: Siew Chin Lim <elly.siew.chin.lim at intel.com>
> > > > > > Cc: Tom Rini <trini at konsulko.com>
> > > > > > Cc: Valentin Longchamp <valentin.longchamp at hitachi-powergrids.com>
> > > > > > Cc: Vignesh Raghavendra <vigneshr at ti.com>
> > > > > 
> > > > > So, some background.  With commit 88e34e5ff76b ("spl: replace
> > > > > CONFIG_SPL_SPI_* with CONFIG_SF_DEFAULT_*") CONFIG_SF_DEFAULT_MODE and
> > > > > related moved from having their default value of SPI_MODE_3 (which
> > > > > evaluates to 3) be defined in common/cmd_sf.c if not otherwise defined,
> > > > > to include/spi_flash.h, and a more visible global default than it was
> > > > > before.  At that time, the following platforms either set or implied
> > > > > SPI_MODE_3 should be used:
> > > > > alt am335x_boneblack am335x_boneblack_vboot am335x_evm_norboot
> > > > > am335x_evm_nor am335x_evm am335x_evm_spiboot am335x_evm_usbspl
> > > > > am43xx_evm am43xx_evm_qspiboot at91sam9n12ek_mmc at91sam9n12ek_nandflash
> > > > > at91sam9n12ek_spiflash at91sam9x5ek_dataflash at91sam9x5ek_mmc
> > > > > at91sam9x5ek_nandflash at91sam9x5ek_spiflash atstk1004 bf506f-ezkit
> > > > > bf537-stamp cam_enc_4xx coreboot-x86 d2net_v2 da830evm da850_am18xxevm
> > > > > da850evm_direct_nor da850evm dra7xx_evm dra7xx_evm_qspiboot
> > > > > dra7xx_evm_uart3 draco dreamplug dxr2 ea20 ecovec ethernut5 gplugd
> > > > > inetspace_v2 k2e_evm k2hk_evm kmcoge5un km_kirkwood_128m16
> > > > > km_kirkwood_pci km_kirkwood kmnusa kmsugp1 kmsuv31 koelsch lager lschlv2
> > > > > lsxhl M53017EVB M54455EVB mgcoge3un mv88f6281gtw_ge net2big_v2
> > > > > netspace_lite_v2 netspace_max_v2 netspace_mini_v2 netspace_v2
> > > > > nios2-generic pcm051_rev1 pcm051_rev3 portl2 pxm2 rut sama5d3xek_mmc
> > > > > sama5d3xek_nandflash sama5d3xek_spiflash sh7785lcr tseries_spi vf610twr
> > > > > zynq_zc770_xm010
> > > > > 
> > > > > Of those platforms, I have handy dra7xx_evm (also am335x_evm but that
> > > > > needs flipping some DIP switches) which has SPI flash by default.  With
> > > > > current tip of tree, I did "sf probe 0 76800000 3" and a few cycles of
> > > > > reading the whole of flash and crc32'ing it, and getting the same result
> > > > > back.  So, SPI_MODE_3 seems correct / function here, and likely so on
> > > > > the rest of the TI platforms listed above.
> > > > > 
> > > > > With commit 14453fbfadc2 ("Convert CONFIG_SF_DEFAULT_* to Kconfig") we
> > > > > migrate these values to Kconfig, and a quick spot-check shows that yes,
> > > > > the migration did not change any values.
> > > > > 
> > > > > A big change between 88e34e5ff76b and 14453fbfadc2 is that a ton of
> > > > > platforms have been added (it was 5 years, after all) and those
> > > > > platforms seem to be where the problems reside.  I'm not sure if or why
> > > > > SPI_MODE_3 doesn't work on so many new platforms, but I would prefer to
> > > > > see "default 0 if ARCH_SOCFPGA || ARCH_STM32 || .." as needed to switch
> > > > > the default to SPI_MODE_0 if there's not a more fundamental problem to
> > > > > solve on some platforms such that SPI_MODE_3 could / should be enabled.
> > > > 
> > > > I rather suspect mode 3 is special case for atmel/am335x and co. and maybe
> > > > marvell kirkwood from the list above. So shouldn't we default to 0 and
> > > > special-case the two or three instead ?
> > > 
> > > I guess my question boils down to why is that the right default?  My
> > > argument that 3 is the right default is that it's what it was in
> > > introduction back in b6368467e6a9 ("SPI Flash: Add "sf" command").  But
> > > references to something else such as the Linux Kernel defaults to mode 0
> > > or some SPI specs saying you should default to mode 0 or something else
> > > would be much appreciated.
> > 
> > Probably check a couple of board schematics and SoC datasheets to build up
> > your own statistics ? In Linux it is 0 because that's likely what most of
> > the systems expect it to be, and the few odd ones just set it to 3
> > explicitly there (or enable CPOL/CPHA as needed). My statistics indicates
> > that I keep running into "oh, here it is also 3, and it should be 0"
> > recently.
> > 
> > I don't have any of the am335x/atmel boards easily available, so I cannot
> > check those, hence the CC list.
> 
> Sigh, OK.  Lets look at the kernel and there instead of SPI_MODE_N we
> have spi-cpol and spi-cpha as properties.  A quick git grep -l spi-cp
> arch/arm arch/arm64 shows where we likely do and do not need to use
> something other than mode 0.  So yes, mode 0 as a default makes sense,
> so long as we also don't break the easily found boards that do need mode
> 3 (or, mode 1 or 2, as of v5.19 it is true there's 59 matches for each
> property, but it's not always both, sometimes it's one or the other).

So, what we need to do is for U-Boot itself, never reference
CONFIG_SF_DEFAULT_MODE (and probably none of the other SF_DEFAULT_..
values) as DM handles this correctly via the device tree.

That leaves common/spl/spl_spi.c to be handled.  If the value is set
wrong, we aren't going to be able to have booted via SPL on SPI.

So, I'm agreeable to changing the default, so long as we also stop
referencing the default in full U-Boot.  We've already gone past the
DM_SPI convert or be removed release.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210915/7e62f7d1/attachment.sig>