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

[Marek Vasut]

On 9/15/21 7:55 PM, Tom Rini wrote:
> 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.

I am still hoping the platform maintainers will chime in regarding their 
mode setting for the SPI NOR. I can only tell which is the sensible 
default for a subset of them.