u-boot.lists.denx.de archive mirror
 help / color / mirror / Atom feed
* [RFC][PATCH] mtd: spi: Set CONFIG_SF_DEFAULT_MODE default to 0
@ 2021-09-14 18:28 Marek Vasut
  2021-09-14 19:19 ` Tom Rini
  2021-09-28 18:45 ` Tom Rini
  0 siblings, 2 replies; 9+ messages in thread
From: Marek Vasut @ 2021-09-14 18:28 UTC (permalink / raw)
  To: u-boot
  Cc: Marek Vasut, Aleksandar Gerasimovski, Andreas Biessmann,
	Eugen Hristev, Michal Simek, Patrice Chotard, Patrick Delaunay,
	Peng Fan, Siew Chin Lim, Tom Rini, Valentin Longchamp,
	Vignesh Raghavendra

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@denx.de>
Cc: Aleksandar Gerasimovski <aleksandar.gerasimovski@hitachi-powergrids.com>
Cc: Andreas Biessmann <andreas@biessmann.org>
Cc: Eugen Hristev <eugen.hristev@microchip.com>
Cc: Michal Simek <michal.simek@xilinx.com>
Cc: Patrice Chotard <patrice.chotard@foss.st.com>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Siew Chin Lim <elly.siew.chin.lim@intel.com>
Cc: Tom Rini <trini@konsulko.com>
Cc: Valentin Longchamp <valentin.longchamp@hitachi-powergrids.com>
Cc: Vignesh Raghavendra <vigneshr@ti.com>
---
 drivers/mtd/spi/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/spi/Kconfig b/drivers/mtd/spi/Kconfig
index b2291f72905..f03fe05e333 100644
--- a/drivers/mtd/spi/Kconfig
+++ b/drivers/mtd/spi/Kconfig
@@ -57,7 +57,7 @@ config SF_DEFAULT_CS
 config SF_DEFAULT_MODE
 	hex "SPI Flash default mode (see include/spi.h)"
 	depends on SPI_FLASH || DM_SPI_FLASH
-	default 3
+	default 0
 	help
 	  The default mode may be provided by the platform
 	  to handle the common case when only a single serial
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [RFC][PATCH] mtd: spi: Set CONFIG_SF_DEFAULT_MODE default to 0
  2021-09-14 18:28 [RFC][PATCH] mtd: spi: Set CONFIG_SF_DEFAULT_MODE default to 0 Marek Vasut
@ 2021-09-14 19:19 ` Tom Rini
  2021-09-14 20:10   ` Marek Vasut
  2021-09-28 18:45 ` Tom Rini
  1 sibling, 1 reply; 9+ messages in thread
From: Tom Rini @ 2021-09-14 19:19 UTC (permalink / raw)
  To: Marek Vasut
  Cc: u-boot, Aleksandar Gerasimovski, Andreas Biessmann,
	Eugen Hristev, Michal Simek, Patrice Chotard, Patrick Delaunay,
	Peng Fan, Siew Chin Lim, Valentin Longchamp, Vignesh Raghavendra

[-- Attachment #1: Type: text/plain, Size: 3724 bytes --]

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@denx.de>
> Cc: Aleksandar Gerasimovski <aleksandar.gerasimovski@hitachi-powergrids.com>
> Cc: Andreas Biessmann <andreas@biessmann.org>
> Cc: Eugen Hristev <eugen.hristev@microchip.com>
> Cc: Michal Simek <michal.simek@xilinx.com>
> Cc: Patrice Chotard <patrice.chotard@foss.st.com>
> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Siew Chin Lim <elly.siew.chin.lim@intel.com>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: Valentin Longchamp <valentin.longchamp@hitachi-powergrids.com>
> Cc: Vignesh Raghavendra <vigneshr@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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC][PATCH] mtd: spi: Set CONFIG_SF_DEFAULT_MODE default to 0
  2021-09-14 19:19 ` Tom Rini
@ 2021-09-14 20:10   ` Marek Vasut
  2021-09-14 20:45     ` Tom Rini
  0 siblings, 1 reply; 9+ messages in thread
From: Marek Vasut @ 2021-09-14 20:10 UTC (permalink / raw)
  To: Tom Rini
  Cc: u-boot, Aleksandar Gerasimovski, Andreas Biessmann,
	Eugen Hristev, Michal Simek, Patrice Chotard, Patrick Delaunay,
	Peng Fan, Siew Chin Lim, Valentin Longchamp, Vignesh Raghavendra

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@denx.de>
>> Cc: Aleksandar Gerasimovski <aleksandar.gerasimovski@hitachi-powergrids.com>
>> Cc: Andreas Biessmann <andreas@biessmann.org>
>> Cc: Eugen Hristev <eugen.hristev@microchip.com>
>> Cc: Michal Simek <michal.simek@xilinx.com>
>> Cc: Patrice Chotard <patrice.chotard@foss.st.com>
>> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
>> Cc: Peng Fan <peng.fan@nxp.com>
>> Cc: Siew Chin Lim <elly.siew.chin.lim@intel.com>
>> Cc: Tom Rini <trini@konsulko.com>
>> Cc: Valentin Longchamp <valentin.longchamp@hitachi-powergrids.com>
>> Cc: Vignesh Raghavendra <vigneshr@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 ?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC][PATCH] mtd: spi: Set CONFIG_SF_DEFAULT_MODE default to 0
  2021-09-14 20:10   ` Marek Vasut
@ 2021-09-14 20:45     ` Tom Rini
  2021-09-14 21:30       ` Marek Vasut
  0 siblings, 1 reply; 9+ messages in thread
From: Tom Rini @ 2021-09-14 20:45 UTC (permalink / raw)
  To: Marek Vasut
  Cc: u-boot, Aleksandar Gerasimovski, Andreas Biessmann,
	Eugen Hristev, Michal Simek, Patrice Chotard, Patrick Delaunay,
	Peng Fan, Siew Chin Lim, Valentin Longchamp, Vignesh Raghavendra

[-- Attachment #1: Type: text/plain, Size: 4674 bytes --]

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@denx.de>
> > > Cc: Aleksandar Gerasimovski <aleksandar.gerasimovski@hitachi-powergrids.com>
> > > Cc: Andreas Biessmann <andreas@biessmann.org>
> > > Cc: Eugen Hristev <eugen.hristev@microchip.com>
> > > Cc: Michal Simek <michal.simek@xilinx.com>
> > > Cc: Patrice Chotard <patrice.chotard@foss.st.com>
> > > Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> > > Cc: Peng Fan <peng.fan@nxp.com>
> > > Cc: Siew Chin Lim <elly.siew.chin.lim@intel.com>
> > > Cc: Tom Rini <trini@konsulko.com>
> > > Cc: Valentin Longchamp <valentin.longchamp@hitachi-powergrids.com>
> > > Cc: Vignesh Raghavendra <vigneshr@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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC][PATCH] mtd: spi: Set CONFIG_SF_DEFAULT_MODE default to 0
  2021-09-14 20:45     ` Tom Rini
@ 2021-09-14 21:30       ` Marek Vasut
  2021-09-14 21:44         ` Tom Rini
  0 siblings, 1 reply; 9+ messages in thread
From: Marek Vasut @ 2021-09-14 21:30 UTC (permalink / raw)
  To: Tom Rini
  Cc: u-boot, Aleksandar Gerasimovski, Andreas Biessmann,
	Eugen Hristev, Michal Simek, Patrice Chotard, Patrick Delaunay,
	Peng Fan, Siew Chin Lim, Valentin Longchamp, Vignesh Raghavendra

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@denx.de>
>>>> Cc: Aleksandar Gerasimovski <aleksandar.gerasimovski@hitachi-powergrids.com>
>>>> Cc: Andreas Biessmann <andreas@biessmann.org>
>>>> Cc: Eugen Hristev <eugen.hristev@microchip.com>
>>>> Cc: Michal Simek <michal.simek@xilinx.com>
>>>> Cc: Patrice Chotard <patrice.chotard@foss.st.com>
>>>> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
>>>> Cc: Peng Fan <peng.fan@nxp.com>
>>>> Cc: Siew Chin Lim <elly.siew.chin.lim@intel.com>
>>>> Cc: Tom Rini <trini@konsulko.com>
>>>> Cc: Valentin Longchamp <valentin.longchamp@hitachi-powergrids.com>
>>>> Cc: Vignesh Raghavendra <vigneshr@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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC][PATCH] mtd: spi: Set CONFIG_SF_DEFAULT_MODE default to 0
  2021-09-14 21:30       ` Marek Vasut
@ 2021-09-14 21:44         ` Tom Rini
  2021-09-15 17:55           ` Tom Rini
  0 siblings, 1 reply; 9+ messages in thread
From: Tom Rini @ 2021-09-14 21:44 UTC (permalink / raw)
  To: Marek Vasut
  Cc: u-boot, Aleksandar Gerasimovski, Andreas Biessmann,
	Eugen Hristev, Michal Simek, Patrice Chotard, Patrick Delaunay,
	Peng Fan, Siew Chin Lim, Valentin Longchamp, Vignesh Raghavendra

[-- Attachment #1: Type: text/plain, Size: 6101 bytes --]

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@denx.de>
> > > > > Cc: Aleksandar Gerasimovski <aleksandar.gerasimovski@hitachi-powergrids.com>
> > > > > Cc: Andreas Biessmann <andreas@biessmann.org>
> > > > > Cc: Eugen Hristev <eugen.hristev@microchip.com>
> > > > > Cc: Michal Simek <michal.simek@xilinx.com>
> > > > > Cc: Patrice Chotard <patrice.chotard@foss.st.com>
> > > > > Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> > > > > Cc: Peng Fan <peng.fan@nxp.com>
> > > > > Cc: Siew Chin Lim <elly.siew.chin.lim@intel.com>
> > > > > Cc: Tom Rini <trini@konsulko.com>
> > > > > Cc: Valentin Longchamp <valentin.longchamp@hitachi-powergrids.com>
> > > > > Cc: Vignesh Raghavendra <vigneshr@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).

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC][PATCH] mtd: spi: Set CONFIG_SF_DEFAULT_MODE default to 0
  2021-09-14 21:44         ` Tom Rini
@ 2021-09-15 17:55           ` Tom Rini
  2021-09-15 18:52             ` Marek Vasut
  0 siblings, 1 reply; 9+ messages in thread
From: Tom Rini @ 2021-09-15 17:55 UTC (permalink / raw)
  To: Marek Vasut
  Cc: u-boot, Aleksandar Gerasimovski, Andreas Biessmann,
	Eugen Hristev, Michal Simek, Patrice Chotard, Patrick Delaunay,
	Peng Fan, Siew Chin Lim, Valentin Longchamp, Vignesh Raghavendra

[-- Attachment #1: Type: text/plain, Size: 6868 bytes --]

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@denx.de>
> > > > > > Cc: Aleksandar Gerasimovski <aleksandar.gerasimovski@hitachi-powergrids.com>
> > > > > > Cc: Andreas Biessmann <andreas@biessmann.org>
> > > > > > Cc: Eugen Hristev <eugen.hristev@microchip.com>
> > > > > > Cc: Michal Simek <michal.simek@xilinx.com>
> > > > > > Cc: Patrice Chotard <patrice.chotard@foss.st.com>
> > > > > > Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> > > > > > Cc: Peng Fan <peng.fan@nxp.com>
> > > > > > Cc: Siew Chin Lim <elly.siew.chin.lim@intel.com>
> > > > > > Cc: Tom Rini <trini@konsulko.com>
> > > > > > Cc: Valentin Longchamp <valentin.longchamp@hitachi-powergrids.com>
> > > > > > Cc: Vignesh Raghavendra <vigneshr@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC][PATCH] mtd: spi: Set CONFIG_SF_DEFAULT_MODE default to 0
  2021-09-15 17:55           ` Tom Rini
@ 2021-09-15 18:52             ` Marek Vasut
  0 siblings, 0 replies; 9+ messages in thread
From: Marek Vasut @ 2021-09-15 18:52 UTC (permalink / raw)
  To: Tom Rini
  Cc: u-boot, Aleksandar Gerasimovski, Andreas Biessmann,
	Eugen Hristev, Michal Simek, Patrice Chotard, Patrick Delaunay,
	Peng Fan, Siew Chin Lim, Valentin Longchamp, Vignesh Raghavendra

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@denx.de>
>>>>>>> Cc: Aleksandar Gerasimovski <aleksandar.gerasimovski@hitachi-powergrids.com>
>>>>>>> Cc: Andreas Biessmann <andreas@biessmann.org>
>>>>>>> Cc: Eugen Hristev <eugen.hristev@microchip.com>
>>>>>>> Cc: Michal Simek <michal.simek@xilinx.com>
>>>>>>> Cc: Patrice Chotard <patrice.chotard@foss.st.com>
>>>>>>> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
>>>>>>> Cc: Peng Fan <peng.fan@nxp.com>
>>>>>>> Cc: Siew Chin Lim <elly.siew.chin.lim@intel.com>
>>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>>> Cc: Valentin Longchamp <valentin.longchamp@hitachi-powergrids.com>
>>>>>>> Cc: Vignesh Raghavendra <vigneshr@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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC][PATCH] mtd: spi: Set CONFIG_SF_DEFAULT_MODE default to 0
  2021-09-14 18:28 [RFC][PATCH] mtd: spi: Set CONFIG_SF_DEFAULT_MODE default to 0 Marek Vasut
  2021-09-14 19:19 ` Tom Rini
@ 2021-09-28 18:45 ` Tom Rini
  1 sibling, 0 replies; 9+ messages in thread
From: Tom Rini @ 2021-09-28 18:45 UTC (permalink / raw)
  To: Marek Vasut
  Cc: u-boot, Aleksandar Gerasimovski, Andreas Biessmann,
	Eugen Hristev, Michal Simek, Patrice Chotard, Patrick Delaunay,
	Peng Fan, Siew Chin Lim, Valentin Longchamp, Vignesh Raghavendra

[-- Attachment #1: Type: text/plain, Size: 1260 bytes --]

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@denx.de>
> Cc: Aleksandar Gerasimovski <aleksandar.gerasimovski@hitachi-powergrids.com>
> Cc: Andreas Biessmann <andreas@biessmann.org>
> Cc: Eugen Hristev <eugen.hristev@microchip.com>
> Cc: Michal Simek <michal.simek@xilinx.com>
> Cc: Patrice Chotard <patrice.chotard@foss.st.com>
> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Siew Chin Lim <elly.siew.chin.lim@intel.com>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: Valentin Longchamp <valentin.longchamp@hitachi-powergrids.com>
> Cc: Vignesh Raghavendra <vigneshr@ti.com>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-09-28 18:45 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-14 18:28 [RFC][PATCH] mtd: spi: Set CONFIG_SF_DEFAULT_MODE default to 0 Marek Vasut
2021-09-14 19:19 ` Tom Rini
2021-09-14 20:10   ` Marek Vasut
2021-09-14 20:45     ` Tom Rini
2021-09-14 21:30       ` Marek Vasut
2021-09-14 21:44         ` Tom Rini
2021-09-15 17:55           ` Tom Rini
2021-09-15 18:52             ` Marek Vasut
2021-09-28 18:45 ` Tom Rini

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).