Handle flash ID collisions. Clean spi_nor_scan() and the flash
parameters initialization. Add guideliness on how to submit a new
flash proposal. An overview of the patch set can be seen in the
documentation patch, the last in the series.
Documentation/driver-api/mtd/spi-nor.rst is obsolete and must be
updated, but the series is getting big, and I though submitting
what I have to speed the review process. Will handle the documentation
afterwards.
Tested with sst26vf064b, w25q256jvm, is25lp256, s25fl256s0, gd25q256,
n25q256a and mx25l25635e.
Changes in v2: address Pratyush's comments on initial patch set.
Tudor Ambarus (35):
mtd: spi-nor: core: Introduce SPI_NOR_PARSE_SFDP
mtd: spi-nor: core: Report correct name in case of ID collisions
mtd: spi-nor: macronix: Handle ID collision b/w MX25L3233F and
MX25L3205D
mtd: spi-nor: macronix: Handle ID collision b/w MX25L12805D and
MX25L12835F
mtd: spi-nor: Introduce Manufacturer ID collisions driver
mtd: spi-nor: manuf-id-collisions: Add support for xt25f128b
mtd: spi-nor: manuf-id-collisions: Add support for xm25qh64c
mtd: spi-nor: core: Introduce the ate_init() hook
mtd: spi-nor: atmel: Use flash late_init() for locking
mtd: spi-nor: sst: Use flash late_init() for locking
mtd: spi-nor: winbond: Use manufacturer late_init() for OTP ops
mtd: spi-nor: xilinx: Use manufacturer late_init() to set setup method
mtd: spi-nor: sst: Use manufacturer late_init() to set _write()
mtd: spi-nor: spansion: Use manufacturer late_init()
mtd: spi-nor: core: Call spi_nor_post_sfdp_fixups() only when SFDP is
defined
mtd: spi-nor: core: Mark default_init() as deprecated
mtd: spi-nor: Introduce spi_nor_nonsfdp_flags_init()
mtd: spi-nor: Get rid of SPI_NOR_4B_OPCODES flag
mtd: spi-nor: Get rid of SPI_NOR_IO_MODE_EN_VOLATILE flag
mtd: spi-nor: core: Use container_of to get the pointer to struct
spi_nor
mtd: spi-nor: Introduce spi_nor_set_mtd_info()
mtd: spi-nor: core: Use common naming scheme for setting mtd_info
fields
mtd: spi-nor: Get rid of nor->page_size
mtd: spi-nor: core: Fix spi_nor_flash_parameter otp description
mtd: spi-nor: core: Move spi_nor_set_addr_width() in spi_nor_setup()
mtd: spi-nor: core: Introduce spi_nor_init_default_params()
mtd: spi-nor: core: Init flash params based on SFDP first for new
flash additions
mtd: spi-nor: sst: sst26vf064b: Use SPI_NOR_PARSE_SFDP
mtd: spi-nor: winbond: w25q256jvm: Use SPI_NOR_PARSE_SFDP
mtd: spi-nor: issi: is25lp256: Use SPI_NOR_PARSE_SFDP
mtd: spi-nor: spansion: s25fl256s0: Skip SFDP parsing
mtd: spi-nor: gigadevice: gd25q256: Use SPI_NOR_PARSE_SFDP
mtd: spi-nor: micron-st: n25q256a: Use SPI_NOR_PARSE_SFDP
mtd: spi-nor: macronix: mx25l25635e: Use SPI_NOR_PARSE_SFDP
docs: mtd: spi-nor: Add details about how to propose a new flash
addition
Documentation/driver-api/mtd/spi-nor.rst | 65 +++
drivers/mtd/spi-nor/Makefile | 1 +
drivers/mtd/spi-nor/atmel.c | 30 +-
drivers/mtd/spi-nor/core.c | 482 ++++++++++++----------
drivers/mtd/spi-nor/core.h | 60 +--
drivers/mtd/spi-nor/gigadevice.c | 6 +-
drivers/mtd/spi-nor/issi.c | 12 +-
drivers/mtd/spi-nor/macronix.c | 67 ++-
drivers/mtd/spi-nor/manuf-id-collisions.c | 29 ++
drivers/mtd/spi-nor/micron-st.c | 34 +-
drivers/mtd/spi-nor/otp.c | 2 +-
drivers/mtd/spi-nor/spansion.c | 24 +-
drivers/mtd/spi-nor/sst.c | 19 +-
drivers/mtd/spi-nor/swp.c | 2 +-
drivers/mtd/spi-nor/sysfs.c | 2 +-
drivers/mtd/spi-nor/winbond.c | 11 +-
drivers/mtd/spi-nor/xilinx.c | 25 +-
include/linux/mtd/spi-nor.h | 4 +-
18 files changed, 539 insertions(+), 336 deletions(-)
create mode 100644 drivers/mtd/spi-nor/manuf-id-collisions.c
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
SPI NOR flashes that statically declare one of the
SPI_NOR_{DUAL, QUAD, OCTAL, OCTAL_DTR}_READ flags and do not support
the RDSFDP command are gratuiously receiving the RDSFDP command
in the attempt of parsing the SFDP tables. It is not desirable to issue
commands that are not supported, so introduce a flag to help on this
situation.
New flash additions that support the SFDP standard should be declared
using SPI_NOR_PARSE_SFDP. Support that can be discovered when parsing
SFDP should not be duplicated by explicit flags at flash declaration.
All the flash parameters will be discovered when parsing SFDP.
Sometimes manufacturers wrongly define some fields in the SFDP tables.
If that's the case, SFDP data can be amended with the fixups() hooks.
It is not common, but if the SFDP tables are entirely wrong, and it
does not worth the hassle to tweak the SFDP parameters by using the
fixups hooks, or if the flash does not define the SFDP tables at all,
then statically init the flash with the SPI_NOR_SKIP_SFDP flag and
specify the rest of flash capabilities with the flash info flags.
With time, we want to convert all flashes to SPI_NOR_PARSE_SFDP and
stop triggering the SFDP parsing with the
SPI_NOR_{DUAL, QUAD, OCTAL*}_READ flags. Getting rid of the
SPI_NOR_{OCTAL, OCTAL_DTR}_READ trigger is easily achievable, the rest
are a long term goal.
Signed-off-by: Tudor Ambarus <redacted>
Reviewed-by: Heiko Thiery <redacted>
---
drivers/mtd/spi-nor/core.c | 3 ++-
drivers/mtd/spi-nor/core.h | 4 ++++
2 files changed, 6 insertions(+), 1 deletion(-)
Provide a way to report the correct flash name in case of ID collisions.
There will be a single flash_info entry when flash IDs collide, and the
differentiation between the flash types will be made at runtime
if possible.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/core.c | 7 +++++--
drivers/mtd/spi-nor/sysfs.c | 2 +-
include/linux/mtd/spi-nor.h | 2 ++
3 files changed, 8 insertions(+), 3 deletions(-)
Macronix has a bad habbit of reusing flash IDs. While MX25L3233F supports
RDSFDP opcode, MX25L3205D does not support it and does not recommend
issuing opcodes that are not supported ("It is not recommended to adopt
any other code not in the command definition table, which will potentially
enter the hidden mode.").
We tested the RDSFDP on the MX25L3205D and the conclusion is that the
flash didn't reply anything. Given that it is unlikely that RDSFDP will
cause any problems for the old MX25L3205D, differentiate between the two
flashes by parsing SFDP.
Tested MX25L3233F. Generated a 256 Kbyte random data and did an erase,
write, read back and compare test. The flash uses for reads
SPINOR_OP_READ_1_4_4 0xeb, for erases SPINOR_OP_BE_4K 0x20, and for writes
SPINOR_OP_PP 0x02.
Signed-off-by: Tudor Ambarus <redacted>
Acked-by: Pratyush Yadav <redacted>
---
root@sama5d2-xplained:~# find / -iname spi-nor
/sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor
/sys/devices/platform/ahb/ahb:apb/f8000000.spi/spi_master/spi0/spi0.0/spi-nor
/sys/bus/spi/drivers/spi-nor
root@sama5d2-xplained:~# ls -al /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor
total 0
drwxr-xr-x 2 root root 0 Mar 9 14:51 .
drwxr-xr-x 6 root root 0 Mar 9 14:50 ..
-r--r--r-- 1 root root 4096 Mar 9 14:51 jedec_id
-r--r--r-- 1 root root 4096 Mar 9 14:51 manufacturer
-r--r--r-- 1 root root 4096 Mar 9 14:51 partname
-r--r--r-- 1 root root 0 Mar 9 14:51 sfdp
root@sama5d2-xplained:~# cat /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/jedec_id
c22016
root@sama5d2-xplained:~# cat /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/manufacturer
macronix
root@sama5d2-xplained:~# cat /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/partname
mx25l3233f
root@sama5d2-xplained:~# cat /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/sfdp > mx25l3233f-sfdp
root@sama5d2-xplained:~# hexdump mx25l3233f-sfdp
0000000 4653 5044 0100 ff01 0000 0901 0030 ff00
0000010 00c2 0401 0060 ff00 ffff ffff ffff ffff
0000020 ffff ffff ffff ffff ffff ffff ffff ffff
0000030 20e5 fff1 ffff 01ff eb44 6b08 3b08 bb04
0000040 ffee ffff ffff ff00 ffff ff00 200c 520f
0000050 d810 ff00 ffff ffff ffff ffff ffff ffff
0000060 3600 2650 f99c 6477 cffe ffff ffff ffff
0000070
drivers/mtd/spi-nor/macronix.c | 23 ++++++++++++++++++++++-
1 file changed, 22 insertions(+), 1 deletion(-)
Some manufacturers completely ignore the manufacturer's identification code
standard (JEP106) and do not define the manufacturer ID continuation
scheme. This will result in manufacturer ID collisions.
An an example, JEP106BA requires Boya that it's manufacturer ID to be
preceded by 8 continuation codes. Boya's identification code must be:
0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x68. But Boya ignores the
continuation scheme and its ID collides with the manufacturer defined in
bank one: Convex Computer.
Introduce the manuf-id-collisions driver in order to address ID collisions
between manufacturers. flash_info entries will be added in a first come,
first served manner. Differentiation between flashes will be done at
runtime if possible. Where runtime differentiation is not possible, new
compatibles will be introduced, but this will be done as a last resort.
Every new flash addition that define the SFDP tables, should dump its SFDP
tables in the patch's comment section below the --- line, so that we can
reference it in case of collisions.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/Makefile | 1 +
drivers/mtd/spi-nor/core.c | 1 +
drivers/mtd/spi-nor/core.h | 1 +
drivers/mtd/spi-nor/manuf-id-collisions.c | 22 ++++++++++++++++++++++
4 files changed, 25 insertions(+)
create mode 100644 drivers/mtd/spi-nor/manuf-id-collisions.c
Flash ignores the manufacturer continuation codes and is likely to
collide with other manufacturers flashes.
Signed-off-by: Tudor Ambarus <redacted>
---
Do not apply without the SFDP tables hexdump.
drivers/mtd/spi-nor/manuf-id-collisions.c | 3 +++
1 file changed, 3 insertions(+)
The goal is to get rid of the spaghetti way of initializing the flash
parameters and settings. late_init() hook will be used to tweak various
parameters that are not defined by the SFDP standard. Can be used by
non SFDP compliant flashes in order to tweak flash parameters that
are not/shouldn't be handled by the flash_info flags. Will replace the
default_init() hook.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/core.c | 15 +++++++++++----
drivers/mtd/spi-nor/core.h | 8 ++++++++
2 files changed, 19 insertions(+), 4 deletions(-)
@@ -391,6 +391,11 @@ struct flash_info {/* Part specific fixup hooks. */conststructspi_nor_fixups*fixups;+/*+*InitflashparametersthatarenotdeclaredintheJESD216SFDP+*standard.+*/+void(*constlate_init)(structspi_nor*nor);};/* Used when the "_ext_id" is two bytes at most */
Locking is not described in JESD216 SFDP standard, place the locking
init in late_init().
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/atmel.c | 30 +++++++++++-------------------
1 file changed, 11 insertions(+), 19 deletions(-)
OTP info is not yet discoverable via SFDP, use late_init() to init
the OTP ops.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/winbond.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
post_sfdp was misleading in this case, as SFDP is not supported by
xilinx. Plus, there's no fixup here, just setting the correct
setup method, as required by xilinx parts.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/xilinx.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
Setting the correct nor->mtd._write in a fixup hook was misleading,
since this is not a fixup, just a specific setting for SST, that differs
from the SPI NOR core default init.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/sst.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
spansion_post_sfdp_fixups() was called regardless if the flash defined
SFDP tables or not. A better place for this kind of parameters init is
in manufacturer's late_init() hook. post_sfdp() should be called only
when SFDP is defined. No functional change in this patch.
Instead of doing the 4b opcodes settings at manufacturer level, thus
also for every flash that will be introduced, this should be done
just where it is needed, per flash. I'll let this for other patch.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/spansion.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
spi_nor_post_sfdp_fixups() was called even when there were no SFDP
tables defined and the function name was misleading.
We introduced the late_init() hook which is used to tweak various
parameters that could not be extracted by other means, i.e. when
parameters are not defined in the JESD216 SFDP standard, or when
the flash_info flags are incomplete.
Use spi_nor_post_sfdp_fixups() just to fix SFDP data. post_sfdp()
hook is as of now used just by s28hs512t, mt35xu512aba, and both
support SFDP, there's no functional change with this patch.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/core.c | 66 +++++++++++++++++---------------------
1 file changed, 29 insertions(+), 37 deletions(-)
The goal is to remove the spagetti init of params. The flash should
be initialized by the SFDP data, and when SFDP tables are not defined,
by the flash_info flags. SFDP data can be corrected by the
post_{bfpt, sfdp} when wrong, and in case of flash_info flags init,
we'll use the late_init() hook, where checking for the
SPI_NOR_SKIP_SFDP flag.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/core.h | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
Used to initialize the NOR flags for settings that are not defined
in the JESD216 SFDP standard, thus can not be retrieved when parsing
SFDP. No functional change.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/core.c | 88 ++++++++++++++++++++++----------------
1 file changed, 52 insertions(+), 36 deletions(-)
Get rid of flash_info flags that indicate settings which can be
discovered when parsing SFDP. It will be clearer who sets what,
and we'll restrict the flash settings that a developer can choose to
only settings that are not SFDP discoverable.
Whether a flash supports 4byte opcodes or not, is discoverable when
parsing the optional 4-byte address instruction table. Flashes that
do not support the 4bait SFDP table should set the SNOR_F_4B_OPCODES
flag in the late_init() call. Flashes that define the 4bait SFDP table
but gets it wrong, should set the flag in a post_sfdp fixup hook.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/core.c | 3 ---
drivers/mtd/spi-nor/core.h | 32 ++++++++++++++++----------------
drivers/mtd/spi-nor/gigadevice.c | 7 ++++---
drivers/mtd/spi-nor/issi.c | 12 ++++++------
drivers/mtd/spi-nor/macronix.c | 18 ++++++++++--------
drivers/mtd/spi-nor/micron-st.c | 22 +++++++++++++---------
drivers/mtd/spi-nor/spansion.c | 12 ++++++------
7 files changed, 55 insertions(+), 51 deletions(-)
Get rid of flash_info flags that indicate settings which can be
discovered when parsing SFDP. It will be clearer who sets what,
and we'll restrict the flash settings that a developer can choose to
only settings that are not SFDP discoverable.
SNOR_F_IO_MODE_EN_VOLATILE is discoverable when parsing the optional
SCCR Map SFDP table. Flashes that do not define this table should set
the flag in the late_init() call. Flashes that define the SFDP optional
table but get the value wrong, should fix it in a post_sfdp fixup hook.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/core.c | 3 ---
drivers/mtd/spi-nor/core.h | 9 ++-------
drivers/mtd/spi-nor/micron-st.c | 11 ++++++++---
3 files changed, 10 insertions(+), 13 deletions(-)
Used to init all the mtd_info fields. Move the mtd_info init
the last thing in the spi_nor_scan(), so that we avoid superfluous
initialization of the mtd_info fields in case of errors.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/core.c | 55 +++++++++++++++++++++-----------------
1 file changed, 31 insertions(+), 24 deletions(-)
nor->page_size duplicated what nor->params->page_size indicates
for no good reason. page_size is a flash parameter of fixed value
and it is better suited to be found in nor->params->page_size.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/core.c | 20 +++++++++-----------
drivers/mtd/spi-nor/xilinx.c | 17 ++++++++++-------
include/linux/mtd/spi-nor.h | 2 --
3 files changed, 19 insertions(+), 20 deletions(-)
@@ -1953,6 +1953,7 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,structspi_nor*nor=mtd_to_spi_nor(mtd);size_tpage_offset,page_remain,i;ssize_tret;+u32page_size=nor->params->page_size;dev_dbg(nor->dev,"to 0x%08x, len %zd\n",(u32)to,len);
@@ -1969,16 +1970,15 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,*calculatedwithanANDoperation.Ontheothercaseswe*needtodoamodulusoperation(moreexpensive).*/-if(is_power_of_2(nor->page_size)){-page_offset=addr&(nor->page_size-1);+if(is_power_of_2(page_size)){+page_offset=addr&(page_size-1);}else{uint64_taux=addr;-page_offset=do_div(aux,nor->page_size);+page_offset=do_div(aux,page_size);}/* the size of data remaining on the first page */-page_remain=min_t(size_t,-nor->page_size-page_offset,len-i);+page_remain=min_t(size_t,page_size-page_offset,len-i);addr=spi_nor_convert_addr(nor,addr);
@@ -64,10 +66,11 @@ static int xilinx_nor_setup(struct spi_nor *nor,*/if(nor->bouncebuf[0]&XSR_PAGESIZE){/* Flash in Power of 2 mode */-nor->page_size=(nor->page_size==264)?256:512;-nor->mtd.writebufsize=nor->page_size;-nor->mtd.size=8*nor->page_size*nor->info->n_sectors;-nor->mtd.erasesize=8*nor->page_size;+page_size=(nor->params->page_size==264)?256:512;+nor->params->page_size=page_size;+nor->mtd.writebufsize=page_size;+nor->mtd.size=8*page_size*nor->info->n_sectors;+nor->mtd.erasesize=8*page_size;}else{/* Flash in Default addressing mode */nor->params->convert_addr=s3an_convert_addr;
Update the description of the otp member of the
struct spi_nor_flash_parameter.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/core.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
spi_nor_setup() configures the SPI NOR memory. Setting the addr width
is too a configuration, hence we can move the spi_nor_set_addr_width()
in spi_nor_setup().
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/core.c | 101 +++++++++++++++++++------------------
1 file changed, 52 insertions(+), 49 deletions(-)
@@ -2485,13 +2485,62 @@ static int spi_nor_default_setup(struct spi_nor *nor,return0;}+staticintspi_nor_set_addr_width(structspi_nor*nor)+{+if(nor->addr_width){+/* already configured from SFDP */+}elseif(nor->read_proto==SNOR_PROTO_8_8_8_DTR){+/*+*In8D-8D-8Dmode,onebytetakeshalfacycletotransfer.So+*inthisprotocolanoddaddresswidthcannotbeusedbecause+*thentheaddressphasewouldonlyspanacycleandahalf.+*Halfacyclewouldbeleftover.Wewouldthenhavetostart+*thedummyphaseinthemiddleofacycleandsotoothedata+*phase,andwewillendthetransactionwithhalfacycleleft+*over.+*+*Forceall8D-8D-8Dflashestouseanaddresswidthof4to+*avoidthissituation.+*/+nor->addr_width=4;+}elseif(nor->info->addr_width){+nor->addr_width=nor->info->addr_width;+}else{+nor->addr_width=3;+}++if(nor->addr_width==3&&nor->mtd.size>0x1000000){+/* enable 4-byte addressing if the device exceeds 16MiB */+nor->addr_width=4;+}++if(nor->addr_width>SPI_NOR_MAX_ADDR_WIDTH){+dev_dbg(nor->dev,"address width is too large: %u\n",+nor->addr_width);+return-EINVAL;+}++/* Set 4byte opcodes when possible. */+if(nor->addr_width==4&&nor->flags&SNOR_F_4B_OPCODES&&+!(nor->flags&SNOR_F_HAS_4BAIT))+spi_nor_set_4byte_opcodes(nor);++return0;+}+staticintspi_nor_setup(structspi_nor*nor,conststructspi_nor_hwcaps*hwcaps){+intret;+if(!nor->params->setup)-return0;+returnspi_nor_set_addr_width(nor);-returnnor->params->setup(nor,hwcaps);+ret=nor->params->setup(nor,hwcaps);+if(ret)+returnret;++returnspi_nor_set_addr_width(nor);}/**
@@ -3031,49 +3080,6 @@ static const struct flash_info *spi_nor_match_id(struct spi_nor *nor,returnNULL;}-staticintspi_nor_set_addr_width(structspi_nor*nor)-{-if(nor->addr_width){-/* already configured from SFDP */-}elseif(nor->read_proto==SNOR_PROTO_8_8_8_DTR){-/*-*In8D-8D-8Dmode,onebytetakeshalfacycletotransfer.So-*inthisprotocolanoddaddresswidthcannotbeusedbecause-*thentheaddressphasewouldonlyspanacycleandahalf.-*Halfacyclewouldbeleftover.Wewouldthenhavetostart-*thedummyphaseinthemiddleofacycleandsotoothedata-*phase,andwewillendthetransactionwithhalfacycleleft-*over.-*-*Forceall8D-8D-8Dflashestouseanaddresswidthof4to-*avoidthissituation.-*/-nor->addr_width=4;-}elseif(nor->info->addr_width){-nor->addr_width=nor->info->addr_width;-}else{-nor->addr_width=3;-}--if(nor->addr_width==3&&nor->mtd.size>0x1000000){-/* enable 4-byte addressing if the device exceeds 16MiB */-nor->addr_width=4;-}--if(nor->addr_width>SPI_NOR_MAX_ADDR_WIDTH){-dev_dbg(nor->dev,"address width is too large: %u\n",-nor->addr_width);-return-EINVAL;-}--/* Set 4byte opcodes when possible. */-if(nor->addr_width==4&&nor->flags&SNOR_F_4B_OPCODES&&-!(nor->flags&SNOR_F_HAS_4BAIT))-spi_nor_set_4byte_opcodes(nor);--return0;-}-staticvoidspi_nor_debugfs_init(structspi_nor*nor,conststructflash_info*info){
@@ -3204,15 +3210,12 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,*-selectopcodesfor(Fast)Read,PageProgramandSectorErase.*-setthenumberofdummycycles(modecycles+waitstates).*-settheSPIprotocolsforregisterandmemoryaccesses.+*-settheaddresswidth.*/ret=spi_nor_setup(nor,hwcaps);if(ret)returnret;-ret=spi_nor_set_addr_width(nor);-if(ret)-returnret;-/* Send all the required SPI flash commands to initialize device */ret=spi_nor_init(nor);if(ret)
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Called for all flashes, regardless if they define SFDP tables or not.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/core.c | 92 +++++++++++++++++++++-----------------
1 file changed, 52 insertions(+), 40 deletions(-)
@@ -2543,6 +2543,56 @@ static int spi_nor_setup(struct spi_nor *nor,returnspi_nor_set_addr_width(nor);}+/**+*spi_nor_init_default_params()-Defaultinitializationofflashparameters+*andsettings.Doneforallflashes,regardlessistheydefineSFDPtables+*ornot.+*@nor:pointertoa'structspi_nor'.+*/+staticvoidspi_nor_init_default_params(structspi_nor*nor)+{+structspi_nor_flash_parameter*params=nor->params;+conststructflash_info*info=nor->info;+structdevice_node*np=spi_nor_get_flash_node(nor);++params->quad_enable=spi_nor_sr2_bit1_quad_enable;+params->set_4byte_addr_mode=spansion_set_4byte_addr_mode;+params->setup=spi_nor_default_setup;+params->otp.org=&info->otp_org;++/* Default to 16-bit Write Status (01h) Command */+nor->flags|=SNOR_F_HAS_16BIT_SR;++/* Set SPI NOR sizes. */+params->writesize=1;+params->size=(u64)info->sector_size*info->n_sectors;+params->page_size=info->page_size;++if(!(info->flags&SPI_NOR_NO_FR)){+/* Default to Fast Read for DT and non-DT platform devices. */+params->hwcaps.mask|=SNOR_HWCAPS_READ_FAST;++/* Mask out Fast Read if not requested at DT instantiation. */+if(np&&!of_property_read_bool(np,"m25p,fast-read"))+params->hwcaps.mask&=~SNOR_HWCAPS_READ_FAST;+}++/* (Fast) Read settings. */+params->hwcaps.mask|=SNOR_HWCAPS_READ;+spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ],+0,0,SPINOR_OP_READ,+SNOR_PROTO_1_1_1);++if(params->hwcaps.mask&SNOR_HWCAPS_READ_FAST)+spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_FAST],+0,8,SPINOR_OP_READ_FAST,+SNOR_PROTO_1_1_1);+/* Page Program settings. */+params->hwcaps.mask|=SNOR_HWCAPS_PP;+spi_nor_set_pp_settings(¶ms->page_programs[SNOR_CMD_PP],+SPINOR_OP_PP,SNOR_PROTO_1_1_1);+}+/***spi_nor_manufacturer_init_params()-Initializetheflash'sparametersand*settingsbasedonMFRregisterand->default_init()hook.
@@ -2609,43 +2659,8 @@ static void spi_nor_info_init_params(struct spi_nor *nor)structspi_nor_flash_parameter*params=nor->params;structspi_nor_erase_map*map=¶ms->erase_map;conststructflash_info*info=nor->info;-structdevice_node*np=spi_nor_get_flash_node(nor);u8i,erase_mask;-/* Initialize default flash parameters and settings. */-params->quad_enable=spi_nor_sr2_bit1_quad_enable;-params->set_4byte_addr_mode=spansion_set_4byte_addr_mode;-params->setup=spi_nor_default_setup;-params->otp.org=&info->otp_org;--/* Default to 16-bit Write Status (01h) Command */-nor->flags|=SNOR_F_HAS_16BIT_SR;--/* Set SPI NOR sizes. */-params->writesize=1;-params->size=(u64)info->sector_size*info->n_sectors;-params->page_size=info->page_size;--if(!(info->flags&SPI_NOR_NO_FR)){-/* Default to Fast Read for DT and non-DT platform devices. */-params->hwcaps.mask|=SNOR_HWCAPS_READ_FAST;--/* Mask out Fast Read if not requested at DT instantiation. */-if(np&&!of_property_read_bool(np,"m25p,fast-read"))-params->hwcaps.mask&=~SNOR_HWCAPS_READ_FAST;-}--/* (Fast) Read settings. */-params->hwcaps.mask|=SNOR_HWCAPS_READ;-spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ],-0,0,SPINOR_OP_READ,-SNOR_PROTO_1_1_1);--if(params->hwcaps.mask&SNOR_HWCAPS_READ_FAST)-spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_FAST],-0,8,SPINOR_OP_READ_FAST,-SNOR_PROTO_1_1_1);-if(info->flags&SPI_NOR_DUAL_READ){params->hwcaps.mask|=SNOR_HWCAPS_READ_1_1_2;spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_1_1_2],
Remove the spagetti way of initializing flash parameters and settings,
at least for the new flash additions (for now). All flash entries should
be converted to either use SPI_NOR_PARSE_SFDP or SPI_NOR_SKIP_SFDP.
SPI_NOR_SKIP_SFDP should be set either when the SFDP tables are completely
wrong and we can't parse relevant data, or when the SFDP tables are not
defined at all, or when RDSFDP command is not supported by the flash.
After all the flash entries will be converted to use these flags and after
the default_init() hook will be removed, the
spi_nor_init_params_deprecated() will be replaced by
spi_nor_info_init_params(). The flash parameters and settings will be
initialized either by parsing SFDP, or via the flash info flags.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/core.c | 103 +++++++++++++++++++++++++------------
1 file changed, 70 insertions(+), 33 deletions(-)
s25fl256s0 does not define the SFDP tables nor implements the
RDSFDP 0x5a command. Skip the SFDP parsing in order to avoid
issuing an unsupported command to the flash.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/spansion.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Add some guideliness on how to propose a new flash addition.
Signed-off-by: Tudor Ambarus <redacted>
---
Documentation/driver-api/mtd/spi-nor.rst | 65 ++++++++++++++++++++++++
1 file changed, 65 insertions(+)
@@ -66,3 +66,68 @@ when you want to write a new driver for a SPI NOR controller. Another API is spi_nor_restore(), this is used to restore the status of SPI flash chip such as addressing mode. Call it whenever detach the driver from device or reboot the system.++Part IV - How to propose a new flash addition?+----------------------------------------------++First we have to clarify where the new flash_info entry will reside. Typically+each manufacturer have their own driver and the new flash will be placed in that+specific manufacturer driver. There are cases however, where special care has to+be taken. In case of flash ID collisions between different manufacturers, the+place to add the new flash is in the manuf-id-collisions.c driver. ID collisions+between flashes of the same manufacturer should be handled in their own+manufacturer driver, macronix being an example. There will be a single+flash_info entry for all the ID collisions of the same ID.++manuf-id-collisions.c is the place to add new flash additions where the+manufacturer is ignorant enough to not implement the ID continuation scheme+that is described in the JEP106 JEDEC Standard. One has to dump its flash ID and+compare it with the flash's manufacturer identification code that is defined in+the JEP106 JEDEC Standard. If the manufacturer ID is defined in bank two or+higher and the manufacturer does not implement the ID continuation scheme, then+it is likely that the flash ID will collide with a manufacturer from bank one or+with other manufacturer from other bank that does not implement the ID+continuation scheme as well.++flash_info entries will be added in a first come, first served manner. If there+are ID collisions, differentiation between flashes will be done at runtime if+possible. Where runtime differentiation is not possible, new compatibles will be+introduced, but this will be done as a last resort.++New flash additions that support the SFDP standard should be declared using+SPI_NOR_PARSE_SFDP. Support that can be discovered when parsing SFDP should not+be duplicated by explicit flags at flash declaration. All the SFDP flash+parameters and settings will be discovered when parsing SFDP. There are+flash_info flags that indicate support that is not SFDP discoverable. These+flags initialize non SFDP support in the spi_nor_nonsfdp_flags_init() method.+SPI_NOR_PARSE_SFDP is usually followed by other flash_info flags from the+aforementioned function. Sometimes manufacturers wrongly define some fields in+the SFDP tables. If that's the case, SFDP data can be amended with the fixups()+hooks. It is not common, but if the SFDP tables are entirely wrong, and it does+not worth the hassle to tweak the SFDP parameters by using the fixups hooks, or+if the flash does not define the SFDP tables at all, then one can statically+init the flash with the SPI_NOR_SKIP_SFDP flag and specify the rest of the flash+capabilities with the flash info flags.++With time we want to convert all flashes to either use SPI_NOR_PARSE_SFDP or+SPI_NOR_SKIP_SFDP and stop triggering the SFDP parsing with the+SPI_NOR_{DUAL, QUAD, OCTAL*}_READ flags. There are flashes that support QUAD+mode but do not support the RDSFDP command, we should avoid issuing unsupported+commands to flashes where possible. It is unlikely that RDSFDP will cause any+problems, but still, it's better to avoid it. There are cases however of flash+ID collisions between flashes that define the SFDP tables and flashes that don't+(again, macronix). We usually differentiate between the two by issuing the+RDSFDP command. In such a case one has to declare the SPI_NOR_PARSE_SFDP+together with all the relevant flags from spi_nor_nonsfdp_flags_init() for the+SFDP compatible flash, but should also declare the relevant flags that are used+in the spi_nor_info_init_params() method in order to init support that can't be+discovered via SFDP for the non-SFDP compatible flash.++Every new flash addition that define the SFDP tables, should hexdump its SFDP+tables in the patch's comment section below the --- line, so that we can+reference it in case of ID collisions.++Every flash_info flag declared should be tested. Typically one uses the+mtd-utils and does an erase, verify erase, write, read back and compare test.+Locking and other flags that are declared in the flash_info entry and used in+the spi_nor_nonsfdp_flags_init() should be tested as well.
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Somewhat unrelated, but while you're here, why not get rid of that
__maybe_unused and make this an ordinary "static inline"?
Rasmus
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Michael Walle <hidden> Date: 2021-07-27 07:56:40
Am 2021-07-27 06:52, schrieb Tudor Ambarus:
quoted hunk
Add some guideliness on how to propose a new flash addition.
Signed-off-by: Tudor Ambarus <redacted>
---
Documentation/driver-api/mtd/spi-nor.rst | 65 ++++++++++++++++++++++++
1 file changed, 65 insertions(+)
+Every new flash addition that define the SFDP tables, should hexdump
its SFDP
+tables in the patch's comment section below the --- line, so that we
can
+reference it in case of ID collisions.
Nice, but could you add some guidelines how to do it? That is the exact
commands, maybe with a notice one should use these whenever possible. I
want to prevent having all sorts of variations of the output and I want
to be able to reverse the operation and verify it.
# xxd -p /path/to/sfdp
# md5sum /path/to/sfdp
# cat /path/to/jedec_id
# cat /path/to/partname
# cat /path/to/manufacturer
-michael
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
Am 2021-07-27 06:52, schrieb Tudor Ambarus:
quoted
Add some guideliness on how to propose a new flash addition.
Signed-off-by: Tudor Ambarus <redacted>
---
Documentation/driver-api/mtd/spi-nor.rst | 65 ++++++++++++++++++++++++
1 file changed, 65 insertions(+)
+Every new flash addition that define the SFDP tables, should hexdump
its SFDP
+tables in the patch's comment section below the --- line, so that we
can
+reference it in case of ID collisions.
Nice, but could you add some guidelines how to do it? That is the exact
commands, maybe with a notice one should use these whenever possible. I
want to prevent having all sorts of variations of the output and I want
to be able to reverse the operation and verify it.
Will add some short examples of mtd_debug and the erase, verify, write, read back
and compare too.
Do you have some locking test examples? I'll have to check those too.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Michael Walle <hidden> Date: 2021-07-27 08:52:02
Am 2021-07-27 10:09, schrieb Tudor.Ambarus@microchip.com:
On 7/27/21 10:22 AM, Michael Walle wrote:
quoted
EXTERNAL EMAIL: Do not click links or open attachments unless you know
the content is safe
Am 2021-07-27 06:52, schrieb Tudor Ambarus:
quoted
Add some guideliness on how to propose a new flash addition.
Signed-off-by: Tudor Ambarus <redacted>
---
Documentation/driver-api/mtd/spi-nor.rst | 65
++++++++++++++++++++++++
1 file changed, 65 insertions(+)
+Every new flash addition that define the SFDP tables, should hexdump
its SFDP
+tables in the patch's comment section below the --- line, so that we
can
+reference it in case of ID collisions.
Nice, but could you add some guidelines how to do it? That is the
exact
commands, maybe with a notice one should use these whenever possible.
I
want to prevent having all sorts of variations of the output and I
want
to be able to reverse the operation and verify it.
ok, will do
quoted
# xxd -p /path/to/sfdp
# md5sum /path/to/sfdp
maybe sha1sum here?
sure, that one doesn't really matter. any *sum will work.
Will add some short examples of mtd_debug and the erase, verify,
write, read back
and compare too.
Do you have some locking test examples? I'll have to check those too.
Not really, I usually check the locking by looking at the BP bits,
but there is no easy method to look at them in linux.
-michael
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
My apologies for being ignorant of this, but I'm not 100% sure of these
two values (SPI_NOR_HAS_LOCK and SPI_NOR_HAS_TB), even though I
included them in my original commit. Looking at the datasheet for this
I can see that there are 5 block protect bits (BP0 - BP4) corresponding
to status registers SR2 through SR6. Status register bits SR7 and SR8
correspond to "status register protect 0 and status register protect 1"
bits as well. The Rockchip engineer I was testing the SFC with did
not have these flags as well on their driver they were using for this
chip too. I have tested with and without, and they seem to work
regardless. Is there a way to know for sure if these should or should
not be here?
Here is a link to the datasheet I was working off of:
https://datasheet.lcsc.com/szlcsc/2005251034_XTX-XT25F128BSSIGT_C558844.pdf
When this is confirmed I'll be glad to provide my "Tested-by" line.
Thank you.
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
On Tue, Jul 27, 2021 at 07:51:53AM +0300, Tudor Ambarus wrote:
quoted
Flash does not support continuation codes and may collide with a flash
of other manufacturer, Intersil being an example .
Signed-off-by: Tudor Ambarus <redacted>
---
0000000 4653 5044 0100 ff01 0000 0901 0030 ff00
0000010 000b 0301 0060 ff00 ffff ffff ffff ffff
0000020 ffff ffff ffff ffff ffff ffff ffff ffff
0000030 20e5 fff1 ffff 07ff eb44 6b08 3b08 bb42
0000040 ffee ffff ffff ff00 ffff ff00 200c 520f
0000050 d810 ff00 ffff ffff ffff ffff ffff ffff
0000060 3600 2700 f99f 6477 e8d9 ffff
drivers/mtd/spi-nor/manuf-id-collisions.c | 4 ++++
1 file changed, 4 insertions(+)
My apologies for being ignorant of this, but I'm not 100% sure of these
two values (SPI_NOR_HAS_LOCK and SPI_NOR_HAS_TB), even though I
Oh yes, I forgot you have already mentioned this in the previous email thread.
included them in my original commit. Looking at the datasheet for this
I can see that there are 5 block protect bits (BP0 - BP4) corresponding
to status registers SR2 through SR6. Status register bits SR7 and SR8
correspond to "status register protect 0 and status register protect 1"
bits as well. The Rockchip engineer I was testing the SFC with did
not have these flags as well on their driver they were using for this
chip too. I have tested with and without, and they seem to work
regardless. Is there a way to know for sure if these should or should
not be here?
We should do some locking tests to verify if these flags are ok for
this flash.
SPI NOR flashes that statically declare one of the
SPI_NOR_{DUAL, QUAD, OCTAL, OCTAL_DTR}_READ flags and do not support
the RDSFDP command are gratuiously receiving the RDSFDP command
in the attempt of parsing the SFDP tables. It is not desirable to issue
commands that are not supported, so introduce a flag to help on this
situation.
New flash additions that support the SFDP standard should be declared
using SPI_NOR_PARSE_SFDP. Support that can be discovered when parsing
SFDP should not be duplicated by explicit flags at flash declaration.
All the flash parameters will be discovered when parsing SFDP.
Sometimes manufacturers wrongly define some fields in the SFDP tables.
If that's the case, SFDP data can be amended with the fixups() hooks.
It is not common, but if the SFDP tables are entirely wrong, and it
does not worth the hassle to tweak the SFDP parameters by using the
fixups hooks, or if the flash does not define the SFDP tables at all,
then statically init the flash with the SPI_NOR_SKIP_SFDP flag and
specify the rest of flash capabilities with the flash info flags.
With time, we want to convert all flashes to SPI_NOR_PARSE_SFDP and
stop triggering the SFDP parsing with the
SPI_NOR_{DUAL, QUAD, OCTAL*}_READ flags. Getting rid of the
SPI_NOR_{OCTAL, OCTAL_DTR}_READ trigger is easily achievable, the rest
are a long term goal.
Signed-off-by: Tudor Ambarus <redacted>
Reviewed-by: Heiko Thiery <redacted>
Reviewed-by: Pratyush Yadav <redacted>
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Provide a way to report the correct flash name in case of ID collisions.
There will be a single flash_info entry when flash IDs collide, and the
differentiation between the flash types will be made at runtime
if possible.
I have the same comments as last time around. I am not convinced that
this approach is better than having multiple entries, one for each
colliding flash. I wonder how you will handle different fixups for the
colliding flashes for example, since nor->info is const.
Maybe I will change my mind once I read through the rest of the series,
in which case I will report back here.
Used to init all the mtd_info fields. Move the mtd_info init
the last thing in the spi_nor_scan(), so that we avoid superfluous
initialization of the mtd_info fields in case of errors.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/core.c | 55 +++++++++++++++++++++-----------------
1 file changed, 31 insertions(+), 24 deletions(-)
Rasmus sent me a heads up that I have a leftover here, probably from a rebase.
The priv assignment is no longer needed as per previous patch. Will update in v3.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Some manufacturers completely ignore the manufacturer's identification code
standard (JEP106) and do not define the manufacturer ID continuation
scheme. This will result in manufacturer ID collisions.
An an example, JEP106BA requires Boya that it's manufacturer ID to be
preceded by 8 continuation codes. Boya's identification code must be:
0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x68. But Boya ignores the
continuation scheme and its ID collides with the manufacturer defined in
bank one: Convex Computer.
Introduce the manuf-id-collisions driver in order to address ID collisions
between manufacturers. flash_info entries will be added in a first come,
first served manner. Differentiation between flashes will be done at
runtime if possible. Where runtime differentiation is not possible, new
compatibles will be introduced, but this will be done as a last resort.
Every new flash addition that define the SFDP tables, should dump its SFDP
tables in the patch's comment section below the --- line, so that we can
reference it in case of collisions.
Signed-off-by: Tudor Ambarus <redacted>
Reviewed-by: Pratyush Yadav <redacted>
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Flash ignores the manufacturer continuation codes and is likely to
collide with other manufacturers flashes.
Signed-off-by: Tudor Ambarus <redacted>
---
Do not apply without the SFDP tables hexdump.
... and all the other sysfs params that we usually ask for. See
Michael's replies on patch 35.
^^^^^^^^
You ate the 'l' ;-)
On 27/07/21 07:51AM, Tudor Ambarus wrote:
quoted hunk
The goal is to get rid of the spaghetti way of initializing the flash
parameters and settings. late_init() hook will be used to tweak various
parameters that are not defined by the SFDP standard. Can be used by
non SFDP compliant flashes in order to tweak flash parameters that
are not/shouldn't be handled by the flash_info flags. Will replace the
default_init() hook.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/core.c | 15 +++++++++++----
drivers/mtd/spi-nor/core.h | 8 ++++++++
2 files changed, 19 insertions(+), 4 deletions(-)
What about flashes that do have the SFDP table, but not all of them? For
example, the Micron MT35XU512ABA flash currently uses the post_sfdp()
hook to populate 8D-8D-8D fast read settings, command extension type,
etc. These are supposed to be obtained from the xSPI Profile 1.0 table
(like we do for Spansion/Cypress S28HS512T). But the flash does not
populate this table. Should these go into the late_init() hook or the
post_sfdp() hook?
FWIW, I think it should go into late_init(). post_sfdp() should only be
used for correcting info obtained from the SFDP table. For populating
the info not present in SFDP at all, late_init() should be used.
Thoughts?
*/
static void spi_nor_late_init_params(struct spi_nor *nor)
{
+ if (nor->manufacturer && nor->manufacturer->late_init)
+ nor->manufacturer->late_init(nor);
+
+ if (nor->info->late_init)
+ nor->info->late_init(nor);
Manufacturer late_init goes before flash late_init. Makes sense.
quoted hunk
+
/*
* NOR protection support. When locking_ops are not provided, we pick
* the default ones.
@@ -2713,8 +2720,8 @@ static void spi_nor_late_init_params(struct spi_nor *nor) * wrong). * spi_nor_post_sfdp_fixups() *- * 5/ Late default flash parameters initialization, used when the- * ->default_init() hook or the SFDP parser do not set specific params.+ * 5/ Late flash parameters initialization, used to initialize flash+ * parameters that are not declared in the JESD216 SFDP standard. * spi_nor_late_init_params() */ static int spi_nor_init_params(struct spi_nor *nor)
@@ -391,6 +391,11 @@ struct flash_info {/* Part specific fixup hooks. */conststructspi_nor_fixups*fixups;+/*+*InitflashparametersthatarenotdeclaredintheJESD216SFDP+*standard.+*/+void(*constlate_init)(structspi_nor*nor);};/* Used when the "_ext_id" is two bytes at most */
Locking is not described in JESD216 SFDP standard, place the locking
init in late_init().
You are chaning the order of setting the locking ops here. Earlier, they
were set before we parsed SFDP. Now they are set after we parse SFDP.
Though I don't see it making much of a difference.
@@ -146,34 +142,30 @@ static const struct spi_nor_locking_ops atmel_global_protection_ops = {.is_locked=atmel_is_global_protected,};-staticvoidatmel_global_protection_default_init(structspi_nor*nor)+staticvoidatmel_global_protection_late_init(structspi_nor*nor){nor->params->locking_ops=&atmel_global_protection_ops;}-staticconststructspi_nor_fixupsatmel_global_protection_fixups={-.default_init=atmel_global_protection_default_init,-};-staticconststructflash_infoatmel_parts[]={/* Atmel -- some are (confusingly) marketed as "DataFlash" */{"at25fs010",INFO(0x1f6601,0,32*1024,4,SECT_4K|SPI_NOR_HAS_LOCK)-.fixups=&atmel_at25fs_fixups},+.late_init=atmel_at25fs_late_init},{"at25fs040",INFO(0x1f6604,0,64*1024,8,SECT_4K|SPI_NOR_HAS_LOCK)-.fixups=&atmel_at25fs_fixups},+.late_init=atmel_at25fs_late_init},{"at25df041a",INFO(0x1f4401,0,64*1024,8,SECT_4K|SPI_NOR_HAS_LOCK|SPI_NOR_SWP_IS_VOLATILE)-.fixups=&atmel_global_protection_fixups},+.late_init=atmel_global_protection_late_init},
Won't you be better off setting this in the manufacturer late_init()? It
seems common for most atmel flashes.
Of course, this would cause a problem for atmel flashes that don't have
this at all, since we would set locking for those as well. But I think
we can avoid that by checking for SNOR_F_HAS_LOCK in
spi_nor_register_locking_ops().
Locking ops are not discoverable via SFDP, use late_init() call.
Signed-off-by: Tudor Ambarus <redacted>
Reviewed-by: Pratyush Yadav <redacted>
One question though. Have you tested these flashes (this one and the
ones in the previous patch) to make sure you are not causing any
regressions? While I don't see anything wrong with the patches, it would
be good to have some test reports.
OTP info is not yet discoverable via SFDP, use late_init() to init
the OTP ops.
What do you mean by the "yet"? Does it mean that OTP info is planned to
be added to the next SFDP version? Or does it mean that it is possible
to discover it via SFDP but we just don't support it yet?
If it is neither and it just means "SFDP does not mention OTP at all",
like it is for locking, then you should just drop the "yet". I know this
is very nitpicky but it just caught my eye.
post_sfdp was misleading in this case, as SFDP is not supported by
xilinx. Plus, there's no fixup here, just setting the correct
setup method, as required by xilinx parts.
Signed-off-by: Tudor Ambarus <redacted>
Setting the correct nor->mtd._write in a fixup hook was misleading,
since this is not a fixup, just a specific setting for SST, that differs
from the SPI NOR core default init.
Signed-off-by: Tudor Ambarus <redacted>
spansion_post_sfdp_fixups() was called regardless if the flash defined
SFDP tables or not. A better place for this kind of parameters init is
in manufacturer's late_init() hook. post_sfdp() should be called only
when SFDP is defined. No functional change in this patch.
Instead of doing the 4b opcodes settings at manufacturer level, thus
also for every flash that will be introduced, this should be done
just where it is needed, per flash. I'll let this for other patch.
spi_nor_post_sfdp_fixups() was called even when there were no SFDP
tables defined and the function name was misleading.
We introduced the late_init() hook which is used to tweak various
parameters that could not be extracted by other means, i.e. when
parameters are not defined in the JESD216 SFDP standard, or when
the flash_info flags are incomplete.
Use spi_nor_post_sfdp_fixups() just to fix SFDP data. post_sfdp()
hook is as of now used just by s28hs512t, mt35xu512aba, and both
support SFDP, there's no functional change with this patch.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/core.c | 66 +++++++++++++++++---------------------
1 file changed, 29 insertions(+), 37 deletions(-)
Huh. I didn't know you could do return foo() in a void function if foo()
is also void. Dunno how I feel about this though. It definitely confused
me for a bit.
I feel like the new flow makes these 3 lines more confusing. Earlier,
these were called under if (spi_nor_parse_sfdp()) so it was a bit easier
to make the connection that these are undoing the changes performed by
that function. Now it is a little harder to spot. I think a comment is
in order.
quoted hunk
}
/**
@@ -2643,26 +2663,6 @@ static void spi_nor_info_init_params(struct spi_nor *nor) spi_nor_init_uniform_erase_map(map, erase_mask, params->size); }-/**- * spi_nor_post_sfdp_fixups() - Updates the flash's parameters and settings- * after SFDP has been parsed (is also called for SPI NORs that do not- * support RDSFDP).- * @nor: pointer to a 'struct spi_nor'- *- * Typically used to tweak various parameters that could not be extracted by- * other means (i.e. when information provided by the SFDP/flash_info tables- * are incomplete or wrong).- */-static void spi_nor_post_sfdp_fixups(struct spi_nor *nor)-{- if (nor->manufacturer && nor->manufacturer->fixups &&- nor->manufacturer->fixups->post_sfdp)- nor->manufacturer->fixups->post_sfdp(nor);-- if (nor->info->fixups && nor->info->fixups->post_sfdp)- nor->info->fixups->post_sfdp(nor);-}- /** * spi_nor_late_init_params() - Late initialization of default flash parameters. * @nor: pointer to a 'struct spi_nor'
@@ -2709,18 +2709,12 @@ static void spi_nor_late_init_params(struct spi_nor *nor) * should be more accurate that the above. * spi_nor_sfdp_init_params() *- * Please note that there is a ->post_bfpt() fixup hook that can overwrite- * the flash parameters and settings immediately after parsing the Basic- * Flash Parameter Table.+ * Please note that there are ->post_{bfpt, sfdp}() fixup hooks that can+ * overwrite the flash parameters and settings immediately after table+ * parsing. * * which can be overwritten by:- * 4/ Post SFDP flash parameters initialization. Used to tweak various- * parameters that could not be extracted by other means (i.e. when- * information provided by the SFDP/flash_info tables are incomplete or- * wrong).- * spi_nor_post_sfdp_fixups()- *- * 5/ Late flash parameters initialization, used to initialize flash+ * 4/ Late flash parameters initialization, used to initialize flash * parameters that are not declared in the JESD216 SFDP standard. * spi_nor_late_init_params() */
The goal is to remove the spagetti init of params. The flash should
be initialized by the SFDP data, and when SFDP tables are not defined,
by the flash_info flags. SFDP data can be corrected by the
post_{bfpt, sfdp} when wrong, and in case of flash_info flags init,
we'll use the late_init() hook, where checking for the
SPI_NOR_SKIP_SFDP flag.
Why depreciate it? It is not like we have external callers that we need
to notify. We know and control all the users of this function. Just move
all users to late_init() and delete this. You have already done a large
part of that work in the previous patches. Why not convert all other
callers as well? Is there some complicated piece of code that stops you
from touching it for now?
Used to initialize the NOR flags for settings that are not defined
in the JESD216 SFDP standard, thus can not be retrieved when parsing
SFDP. No functional change.
I am worried if the order in which these flags are set can cause some
subtle bugs.
I can see one instance of it with SNOR_F_HAS_LOCK.
spi_nor_late_init_params() checks for SNOR_F_HAS_LOCK and if there are
no locking ops specified, it sets the default locking ops. This works
fine before this patch because the flag is set before the function is
called. But now, the flag will be set _after_ the function is called,
and so you will never be able to set the default flags.
This is one bug I can spot but I fear some others might be hiding
somewhere as well. SPI NOR has accumulated a lot of spaghetti code over
the years and I certainly felt it when working on my Octal DTR series.
It caused an address width selection bug that was not obvious at all,
and was not even caught during the rc cycles.
I think this series does clean up that spaghetti a lot. But you need to
be careful of such bugs. I think you should definitely let this series
cook in next for some time so it gets some exposure and hopefully some
testing.
@@ -3115,17 +3166,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, mutex_init(&nor->lock);- /*- * Make sure the XSR_RDY flag is set before calling- * spi_nor_wait_till_ready(). Xilinx S3AN share MFR- * with Atmel SPI NOR.- */- if (info->flags & SPI_NOR_XSR_RDY)- nor->flags |= SNOR_F_READY_XSR_RDY;-- if (info->flags & SPI_NOR_HAS_LOCK)- nor->flags |= SNOR_F_HAS_LOCK;- mtd->_write = spi_nor_write; /* Init flash parameters based on flash_info struct and SFDP */
@@ -3147,27 +3187,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, mtd->_get_device = spi_nor_get_device; mtd->_put_device = spi_nor_put_device;- if (info->flags & USE_FSR)- nor->flags |= SNOR_F_USE_FSR;- if (info->flags & SPI_NOR_HAS_TB) {- nor->flags |= SNOR_F_HAS_SR_TB;- if (info->flags & SPI_NOR_TB_SR_BIT6)- nor->flags |= SNOR_F_HAS_SR_TB_BIT6;- }-- if (info->flags & NO_CHIP_ERASE)- nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;- if (info->flags & USE_CLSR)- nor->flags |= SNOR_F_USE_CLSR;- if (info->flags & SPI_NOR_SWP_IS_VOLATILE)- nor->flags |= SNOR_F_SWP_IS_VOLATILE;-- if (info->flags & SPI_NOR_4BIT_BP) {- nor->flags |= SNOR_F_HAS_4BIT_BP;- if (info->flags & SPI_NOR_BP3_SR_BIT6)- nor->flags |= SNOR_F_HAS_SR_BP3_BIT6;- }- if (info->flags & SPI_NOR_NO_ERASE) mtd->flags |= MTD_NO_ERASE;
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
On 27/07/21 07:52AM, Tudor Ambarus wrote:
quoted
Used to initialize the NOR flags for settings that are not defined
in the JESD216 SFDP standard, thus can not be retrieved when parsing
SFDP. No functional change.
I am worried if the order in which these flags are set can cause some
subtle bugs.
I can see one instance of it with SNOR_F_HAS_LOCK.
spi_nor_late_init_params() checks for SNOR_F_HAS_LOCK and if there are
no locking ops specified, it sets the default locking ops. This works
fine before this patch because the flag is set before the function is
called. But now, the flag will be set _after_ the function is called,
and so you will never be able to set the default flags.
This is one bug I can spot but I fear some others might be hiding
somewhere as well. SPI NOR has accumulated a lot of spaghetti code over
the years and I certainly felt it when working on my Octal DTR series.
It caused an address width selection bug that was not obvious at all,
and was not even caught during the rc cycles.
I think this series does clean up that spaghetti a lot. But you need to
be careful of such bugs. I think you should definitely let this series
cook in next for some time so it gets some exposure and hopefully some
testing.
@@ -3115,17 +3166,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, mutex_init(&nor->lock);- /*- * Make sure the XSR_RDY flag is set before calling- * spi_nor_wait_till_ready(). Xilinx S3AN share MFR- * with Atmel SPI NOR.- */- if (info->flags & SPI_NOR_XSR_RDY)- nor->flags |= SNOR_F_READY_XSR_RDY;-- if (info->flags & SPI_NOR_HAS_LOCK)- nor->flags |= SNOR_F_HAS_LOCK;- mtd->_write = spi_nor_write; /* Init flash parameters based on flash_info struct and SFDP */
@@ -3147,27 +3187,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, mtd->_get_device = spi_nor_get_device; mtd->_put_device = spi_nor_put_device;- if (info->flags & USE_FSR)- nor->flags |= SNOR_F_USE_FSR;- if (info->flags & SPI_NOR_HAS_TB) {- nor->flags |= SNOR_F_HAS_SR_TB;- if (info->flags & SPI_NOR_TB_SR_BIT6)- nor->flags |= SNOR_F_HAS_SR_TB_BIT6;- }-- if (info->flags & NO_CHIP_ERASE)- nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;- if (info->flags & USE_CLSR)- nor->flags |= SNOR_F_USE_CLSR;- if (info->flags & SPI_NOR_SWP_IS_VOLATILE)- nor->flags |= SNOR_F_SWP_IS_VOLATILE;-- if (info->flags & SPI_NOR_4BIT_BP) {- nor->flags |= SNOR_F_HAS_4BIT_BP;- if (info->flags & SPI_NOR_BP3_SR_BIT6)- nor->flags |= SNOR_F_HAS_SR_BP3_BIT6;- }- if (info->flags & SPI_NOR_NO_ERASE) mtd->flags |= MTD_NO_ERASE;
As I pointed out above, I think this patch is certainly going in the
right direction. We just need to be careful of the bugs that slip
through.
Right, I'll self review all once I'll prepare v3. And I'll redo the testing,
this time trying to cover all the flash info flags. Would be great if Michael,
Vignesh and others can have a look on the series too. With 2 or 3 persons reviewing
and better test coverage, we should be fine. Would be great if we can have
these cleaning patches without introducing regressions, but if we introduce it's not
a tragedy, we will fix all once reported.
Cheers,
ta
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Get rid of flash_info flags that indicate settings which can be
discovered when parsing SFDP. It will be clearer who sets what,
and we'll restrict the flash settings that a developer can choose to
only settings that are not SFDP discoverable.
Whether a flash supports 4byte opcodes or not, is discoverable when
parsing the optional 4-byte address instruction table. Flashes that
do not support the 4bait SFDP table should set the SNOR_F_4B_OPCODES
flag in the late_init() call. Flashes that define the 4bait SFDP table
but gets it wrong, should set the flag in a post_sfdp fixup hook.
I like the idea, not so much the execution. More on this below.
This flash populated the 4BAIT table, so you can simply drop the flag.
No need for the late_init().
This makes me think that many other flashes might also have the 4BAIT
table but the developers chose to add this flag here since at that time
the norm was to populate all flash capabilities. I think we could be
able to drop many more .late_init like this. But unfortunately someone
needs to do the hard work of checking each flash, and most flash
datasheets don't even list the SFDP contents.
So while I think in the ideal world we would go check each flash, I
think this is an acceptable compromise. Let's not let perfection be the
enemy of good.
While we are on this topic, I find this a bit "ugly". Having to set
late_init() for setting these flags for each flash is not exactly very
clean or readable. I don't know how the future will look like, but if
each flash/family needs its own late_init() to set some flags, it won't
be very readable. We seem to be trading one type of complexity for
another. I dunno which is the lesser evil though...
Get rid of flash_info flags that indicate settings which can be
discovered when parsing SFDP. It will be clearer who sets what,
and we'll restrict the flash settings that a developer can choose to
only settings that are not SFDP discoverable.
SNOR_F_IO_MODE_EN_VOLATILE is discoverable when parsing the optional
SCCR Map SFDP table. Flashes that do not define this table should set
the flag in the late_init() call. Flashes that define the SFDP optional
table but get the value wrong, should fix it in a post_sfdp fixup hook.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/core.c | 3 ---
drivers/mtd/spi-nor/core.h | 9 ++-------
drivers/mtd/spi-nor/micron-st.c | 11 ++++++++---
3 files changed, 10 insertions(+), 13 deletions(-)
I forgot to say this in the previous email, but since this flash is the
same family as the one above, it should also have the 4BAIT table, and
should not need this late_init. But I don't have access to this part so
I can't say with 100% certainty.
Get the pointer to the containing struct spi_nor by using container_of.
Please add an explanation on _why_ you are doing this. I suspect it
would be something boring like "because mtd is embedded in nor, no need
to use mtd->priv", but good to have it here regardless.
Used to init all the mtd_info fields. Move the mtd_info init
the last thing in the spi_nor_scan(), so that we avoid superfluous
initialization of the mtd_info fields in case of errors.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/core.c | 55 +++++++++++++++++++++-----------------
1 file changed, 31 insertions(+), 24 deletions(-)
nor->page_size duplicated what nor->params->page_size indicates
for no good reason. page_size is a flash parameter of fixed value
and it is better suited to be found in nor->params->page_size.
Signed-off-by: Tudor Ambarus <redacted>
Reviewed-by: Pratyush Yadav <redacted>
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
spi_nor_setup() configures the SPI NOR memory. Setting the addr width
is too a configuration, hence we can move the spi_nor_set_addr_width()
in spi_nor_setup().
Signed-off-by: Tudor Ambarus <redacted>
Reviewed-by: Pratyush Yadav <redacted>
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Michael Walle <hidden> Date: 2021-08-23 22:20:21
Am 2021-07-27 06:51, schrieb Tudor Ambarus:
SPI NOR flashes that statically declare one of the
SPI_NOR_{DUAL, QUAD, OCTAL, OCTAL_DTR}_READ flags and do not support
the RDSFDP command are gratuiously receiving the RDSFDP command
in the attempt of parsing the SFDP tables. It is not desirable to issue
commands that are not supported, so introduce a flag to help on this
situation.
New flash additions that support the SFDP standard should be declared
using SPI_NOR_PARSE_SFDP. Support that can be discovered when parsing
SFDP should not be duplicated by explicit flags at flash declaration.
All the flash parameters will be discovered when parsing SFDP.
Sometimes manufacturers wrongly define some fields in the SFDP tables.
If that's the case, SFDP data can be amended with the fixups() hooks.
It is not common, but if the SFDP tables are entirely wrong, and it
does not worth the hassle to tweak the SFDP parameters by using the
fixups hooks, or if the flash does not define the SFDP tables at all,
then statically init the flash with the SPI_NOR_SKIP_SFDP flag and
specify the rest of flash capabilities with the flash info flags.
With time, we want to convert all flashes to SPI_NOR_PARSE_SFDP and
stop triggering the SFDP parsing with the
SPI_NOR_{DUAL, QUAD, OCTAL*}_READ flags. Getting rid of the
SPI_NOR_{OCTAL, OCTAL_DTR}_READ trigger is easily achievable, the rest
are a long term goal.
Signed-off-by: Tudor Ambarus <redacted>
Reviewed-by: Heiko Thiery <redacted>
From: Michael Walle <hidden> Date: 2021-08-23 22:34:56
Am 2021-08-04 10:23, schrieb Pratyush Yadav:
On 27/07/21 07:51AM, Tudor Ambarus wrote:
quoted
Provide a way to report the correct flash name in case of ID
collisions.
There will be a single flash_info entry when flash IDs collide, and
the
differentiation between the flash types will be made at runtime
if possible.
I have the same comments as last time around. I am not convinced that
this approach is better than having multiple entries, one for each
colliding flash. I wonder how you will handle different fixups for the
colliding flashes for example, since nor->info is const.
I thought of multple entries in xx_parts[], too. But how would you
choose
between those? The flash id is the same, only the name would be
different.
Searching by name seems cumbersome. Also, the fixup of the first match
will change the pointer (or maybe an additional pointer, so we still
have the original match), and thus also changes the pointer to the fixup
itself.
-michael
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Michael Walle <hidden> Date: 2021-08-23 22:44:30
Am 2021-07-27 06:51, schrieb Tudor Ambarus:
Macronix has a bad habbit of reusing flash IDs. While MX25L3233F
supports
RDSFDP opcode, MX25L3205D does not support it and does not recommend
issuing opcodes that are not supported ("It is not recommended to adopt
any other code not in the command definition table, which will
potentially
enter the hidden mode.").
We tested the RDSFDP on the MX25L3205D and the conclusion is that the
flash didn't reply anything. Given that it is unlikely that RDSFDP will
cause any problems for the old MX25L3205D, differentiate between the
two
flashes by parsing SFDP.
Tested MX25L3233F. Generated a 256 Kbyte random data and did an erase,
write, read back and compare test. The flash uses for reads
SPINOR_OP_READ_1_4_4 0xeb, for erases SPINOR_OP_BE_4K 0x20, and for
writes
SPINOR_OP_PP 0x02.
Signed-off-by: Tudor Ambarus <redacted>
Acked-by: Pratyush Yadav <redacted>
---
root@sama5d2-xplained:~# find / -iname spi-nor
/sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor
/sys/devices/platform/ahb/ahb:apb/f8000000.spi/spi_master/spi0/spi0.0/spi-nor
/sys/bus/spi/drivers/spi-nor
root@sama5d2-xplained:~# ls -al
/sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor
total 0
drwxr-xr-x 2 root root 0 Mar 9 14:51 .
drwxr-xr-x 6 root root 0 Mar 9 14:50 ..
-r--r--r-- 1 root root 4096 Mar 9 14:51 jedec_id
-r--r--r-- 1 root root 4096 Mar 9 14:51 manufacturer
-r--r--r-- 1 root root 4096 Mar 9 14:51 partname
-r--r--r-- 1 root root 0 Mar 9 14:51 sfdp
root@sama5d2-xplained:~# cat
/sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/jedec_id
c22016
root@sama5d2-xplained:~# cat
/sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/manufacturer
macronix
root@sama5d2-xplained:~# cat
/sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/partname
mx25l3233f
root@sama5d2-xplained:~# cat
/sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/sfdp
quoted
mx25l3233f-sfdp
root@sama5d2-xplained:~# hexdump mx25l3233f-sfdp
use xxd if possible and the sha1sum/md5sum is missing.
|
+ SECT_4K)
+ /* ID collision with mx25l3233f. */
+ .fixups = &mx25l3233f_fixups },
Shouldn't we use mx25l3205d_fixups as name here? What if there are more
flashes with the same id. Using the name of the colliding flash here
doesn't really scale.
-michael
From: Michael Walle <hidden> Date: 2021-08-23 22:49:38
Am 2021-07-27 06:51, schrieb Tudor Ambarus:
quoted hunk
Some manufacturers completely ignore the manufacturer's identification
code
standard (JEP106) and do not define the manufacturer ID continuation
scheme. This will result in manufacturer ID collisions.
An an example, JEP106BA requires Boya that it's manufacturer ID to be
preceded by 8 continuation codes. Boya's identification code must be:
0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x68. But Boya ignores
the
continuation scheme and its ID collides with the manufacturer defined
in
bank one: Convex Computer.
Introduce the manuf-id-collisions driver in order to address ID
collisions
between manufacturers. flash_info entries will be added in a first
come,
first served manner. Differentiation between flashes will be done at
runtime if possible. Where runtime differentiation is not possible, new
compatibles will be introduced, but this will be done as a last resort.
Every new flash addition that define the SFDP tables, should dump its
SFDP
tables in the patch's comment section below the --- line, so that we
can
reference it in case of collisions.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/Makefile | 1 +
drivers/mtd/spi-nor/core.c | 1 +
drivers/mtd/spi-nor/core.h | 1 +
drivers/mtd/spi-nor/manuf-id-collisions.c | 22 ++++++++++++++++++++++
4 files changed, 25 insertions(+)
create mode 100644 drivers/mtd/spi-nor/manuf-id-collisions.c
diff --git a/drivers/mtd/spi-nor/Makefile
b/drivers/mtd/spi-nor/Makefile
index 6b904e439372..48763d10daad 100644
Called for all flashes, regardless if they define SFDP tables or not.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/core.c | 92 +++++++++++++++++++++-----------------
1 file changed, 52 insertions(+), 40 deletions(-)
@@ -2543,6 +2543,56 @@ static int spi_nor_setup(struct spi_nor *nor,returnspi_nor_set_addr_width(nor);}+/**+*spi_nor_init_default_params()-Defaultinitializationofflashparameters+*andsettings.Doneforallflashes,regardlessistheydefineSFDPtables+*ornot.+*@nor:pointertoa'structspi_nor'.+*/+staticvoidspi_nor_init_default_params(structspi_nor*nor)+{+structspi_nor_flash_parameter*params=nor->params;+conststructflash_info*info=nor->info;+structdevice_node*np=spi_nor_get_flash_node(nor);++params->quad_enable=spi_nor_sr2_bit1_quad_enable;+params->set_4byte_addr_mode=spansion_set_4byte_addr_mode;+params->setup=spi_nor_default_setup;+params->otp.org=&info->otp_org;++/* Default to 16-bit Write Status (01h) Command */+nor->flags|=SNOR_F_HAS_16BIT_SR;++/* Set SPI NOR sizes. */+params->writesize=1;+params->size=(u64)info->sector_size*info->n_sectors;+params->page_size=info->page_size;
I think these two lines should go in spi_nor_info_init_params() since
you are using the nor info to initialize these parameters. Otherwise,
what even is the difference between these two functions?
quoted hunk
+
+ if (!(info->flags & SPI_NOR_NO_FR)) {
+ /* Default to Fast Read for DT and non-DT platform devices. */
+ params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST;
+
+ /* Mask out Fast Read if not requested at DT instantiation. */
+ if (np && !of_property_read_bool(np, "m25p,fast-read"))
+ params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;
+ }
+
+ /* (Fast) Read settings. */
+ params->hwcaps.mask |= SNOR_HWCAPS_READ;
+ spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ],
+ 0, 0, SPINOR_OP_READ,
+ SNOR_PROTO_1_1_1);
+
+ if (params->hwcaps.mask & SNOR_HWCAPS_READ_FAST)
+ spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_FAST],
+ 0, 8, SPINOR_OP_READ_FAST,
+ SNOR_PROTO_1_1_1);
+ /* Page Program settings. */
+ params->hwcaps.mask |= SNOR_HWCAPS_PP;
+ spi_nor_set_pp_settings(¶ms->page_programs[SNOR_CMD_PP],
+ SPINOR_OP_PP, SNOR_PROTO_1_1_1);
+}
+
/**
* spi_nor_manufacturer_init_params() - Initialize the flash's parameters and
* settings based on MFR register and ->default_init() hook.
@@ -2609,43 +2659,8 @@ static void spi_nor_info_init_params(struct spi_nor *nor) struct spi_nor_flash_parameter *params = nor->params; struct spi_nor_erase_map *map = ¶ms->erase_map; const struct flash_info *info = nor->info;- struct device_node *np = spi_nor_get_flash_node(nor); u8 i, erase_mask;- /* Initialize default flash parameters and settings. */- params->quad_enable = spi_nor_sr2_bit1_quad_enable;- params->set_4byte_addr_mode = spansion_set_4byte_addr_mode;- params->setup = spi_nor_default_setup;- params->otp.org = &info->otp_org;-- /* Default to 16-bit Write Status (01h) Command */- nor->flags |= SNOR_F_HAS_16BIT_SR;-- /* Set SPI NOR sizes. */- params->writesize = 1;- params->size = (u64)info->sector_size * info->n_sectors;- params->page_size = info->page_size;-- if (!(info->flags & SPI_NOR_NO_FR)) {- /* Default to Fast Read for DT and non-DT platform devices. */- params->hwcaps.mask |= SNOR_HWCAPS_READ_FAST;-- /* Mask out Fast Read if not requested at DT instantiation. */- if (np && !of_property_read_bool(np, "m25p,fast-read"))- params->hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;- }-- /* (Fast) Read settings. */- params->hwcaps.mask |= SNOR_HWCAPS_READ;- spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ],- 0, 0, SPINOR_OP_READ,- SNOR_PROTO_1_1_1);-- if (params->hwcaps.mask & SNOR_HWCAPS_READ_FAST)- spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_FAST],- 0, 8, SPINOR_OP_READ_FAST,- SNOR_PROTO_1_1_1);- if (info->flags & SPI_NOR_DUAL_READ) { params->hwcaps.mask |= SNOR_HWCAPS_READ_1_1_2; spi_nor_set_read_settings(¶ms->reads[SNOR_CMD_READ_1_1_2],
@@ -2823,6 +2833,8 @@ static int spi_nor_init_params(struct spi_nor *nor) if (!nor->params) return -ENOMEM;+ spi_nor_init_default_params(nor);+ spi_nor_info_init_params(nor); spi_nor_manufacturer_init_params(nor);
I am neutral towards this patch. I don't think it improves much, but at
the same time it doesn't make anything worse either.
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Remove the spagetti way of initializing flash parameters and settings,
at least for the new flash additions (for now). All flash entries should
be converted to either use SPI_NOR_PARSE_SFDP or SPI_NOR_SKIP_SFDP.
SPI_NOR_SKIP_SFDP should be set either when the SFDP tables are completely
wrong and we can't parse relevant data, or when the SFDP tables are not
defined at all, or when RDSFDP command is not supported by the flash.
After all the flash entries will be converted to use these flags and after
the default_init() hook will be removed, the
spi_nor_init_params_deprecated() will be replaced by
spi_nor_info_init_params(). The flash parameters and settings will be
initialized either by parsing SFDP, or via the flash info flags.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/core.c | 103 +++++++++++++++++++++++++------------
1 file changed, 70 insertions(+), 33 deletions(-)
Missing documentation for treat_id_collisions.
Also, I assume this will go away when we get rid of
spi_nor_init_params_deprecated(), correct?
+ *
+ * The method has a roll-back mechanism: in case the SFDP parsing fails, the
+ * legacy flash parameters and settings will be restored.
+ */
+static void spi_nor_sfdp_init_params(struct spi_nor *nor,
+ bool treat_id_collisions)
+{
+ struct spi_nor_flash_parameter sfdp_params;
+
+ memcpy(&sfdp_params, nor->params, sizeof(sfdp_params));
+
+ if (!spi_nor_parse_sfdp(nor))
+ return spi_nor_post_sfdp_fixups(nor);
+
+ memcpy(nor->params, &sfdp_params, sizeof(*nor->params));
+ nor->addr_width = 0;
+ nor->flags &= ~SNOR_F_4B_OPCODES;
+
+ if (!treat_id_collisions)
+ return;
No, this doesn't seem quite right. Why would you not want to treat ID
collisions for flashes that use spi_nor_init_params_deprecated()? What
makes this not possible for them?
Anyway, even if we don't want to treat ID collisions for those flashes,
we can't just return here. In the previous code, nor->params is already
initialized by spi_nor_info_init_params() so restoring that via memcpy()
would make sense since it would restore the info-initialized state. Now
it would be just 0, with no useful information in there. You are bound
to run into errors somewhere down the line.
So I think you either need to make this function return an error and
propagate it up the call chain or run spi_nor_info_init_params() for
both type of flashes to make sure we do our best effort to initialize
the flash.
quoted hunk
+ /*
+ * Fallback to flash info params init in case the SFDP parsing fails.
+ * Used to handle ID collisions between flashes that define the SFDP
+ * tables and flashes that don't.
+ */
+ spi_nor_info_init_params(nor);
+ spi_nor_manufacturer_init_params(nor);
+}
+
/**
* spi_nor_late_init_params() - Late initialization of default flash parameters.
* @nor: pointer to a 'struct spi_nor'
@@ -2797,7 +2808,9 @@ static void spi_nor_nonsfdp_flags_init(struct spi_nor *nor) } /**- * spi_nor_init_params() - Initialize the flash's parameters and settings.+ * spi_nor_init_params_deprecated() - Initialize the flash's parameters and+ * settings. The function is deprecated, it will be removed and replaced with+ * spi_nor_info_init_params(). * @nor: pointer to a 'struct spi_nor'. * * The flash parameters and settings are initialized based on a sequence of
@@ -2821,11 +2834,40 @@ static void spi_nor_nonsfdp_flags_init(struct spi_nor *nor) * Please note that there are ->post_{bfpt, sfdp}() fixup hooks that can * overwrite the flash parameters and settings immediately after table * parsing.+ */+static void spi_nor_init_params_deprecated(struct spi_nor *nor)+{+ spi_nor_info_init_params(nor);+ spi_nor_manufacturer_init_params(nor);++ if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |+ SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ)) &&+ !(nor->info->flags & SPI_NOR_SKIP_SFDP))+ spi_nor_sfdp_init_params(nor, false);+}++/**+ * spi_nor_init_params() - Initialize the flash's parameters and settings.+ * @nor: pointer to a 'struct spi_nor'.+ *+ * The flash parameters and settings are initialized based on a sequence of+ * calls that are ordered by priority:+ *+ * 1/ Default flash parameters initialization. The initializations are done+ * for all the flashes, regardless if the support SFDP or not.+ * spi_nor_init_default_params()+ * which can be overwritten by: *+ * 2/ SFDP based or the deprecated way of initializing flash parameters.+ * Ideally at this step the flash parameters init will be done either by+ * parsing SFDP, where supported, or statically via flash_info flags.+ * spi_nor_sfdp_init_params() or spi_nor_init_params_deprecated() * which can be overwritten by:- * 4/ Late flash parameters initialization, used to initialize flash+ *+ * 3/ Late flash parameters initialization, used to initialize flash * parameters that are not declared in the JESD216 SFDP standard. * spi_nor_late_init_params()+ *
Not really related to this patch, but we need to document the return
value here as well.
quoted hunk
*/
static int spi_nor_init_params(struct spi_nor *nor)
{
Add some guideliness on how to propose a new flash addition.
Signed-off-by: Tudor Ambarus <redacted>
---
Documentation/driver-api/mtd/spi-nor.rst | 65 ++++++++++++++++++++++++
1 file changed, 65 insertions(+)
+Every new flash addition that define the SFDP tables, should hexdump
its SFDP
+tables in the patch's comment section below the --- line, so that we
can
+reference it in case of ID collisions.
Nice, but could you add some guidelines how to do it? That is the exact
commands, maybe with a notice one should use these whenever possible. I
want to prevent having all sorts of variations of the output and I want
to be able to reverse the operation and verify it.
# xxd -p /path/to/sfdp
# md5sum /path/to/sfdp
# cat /path/to/jedec_id
# cat /path/to/partname
# cat /path/to/manufacturer
It would be nice if we could have checkpatch.pl check for these. But I
don't know how difficult that would be to add.
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel