Clean spi_nor_scan() and the flash parameters initialization code.
Tested all the flashes from patch set. If someone can test the
locking ops on few flashes would be great. It seems that my flashes
have the non volatile bits weared out.
This patch set is split from:
https://lore.kernel.org/linux-mtd/20210727045222.905056-1-tudor.ambarus@microchip.com/
The ID collision series will be set in a dedicated patch set that will
depend on this one.
Changes in v3:
- move late_init() in struct spi_nor_fixups and update patches accordingly.
R-b tags were dropped.
- new patch "mtd: spi-nor: sst: Get rid of SST_WRITE flash_info flag"
moves manufacturer specific flag out of the core.
- update methods description to make it clear who sets when
- introduce flash_info flag masks to make it clear when one should be set.
- rework "parse SFDP first idea".
Tudor Ambarus (25):
mtd: spi-nor: core: Fix spi_nor_flash_parameter otp description
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: Get rid of nor->page_size
mtd: spi-nor: core: Introduce the late_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: sst: Get rid of SST_WRITE flash_info flag
mtd: spi-nor: Introduce flash_info flags masks
mtd: spi-nor: Introduce spi_nor_nonsfdp_init_flags()
mtd: spi-nor: Introduce spi_nor_init_fixup_flags()
mtd: spi-nor: core: Introduce SPI_NOR_PARSE_SFDP
mtd: spi-nor: core: Init flash params based on SFDP first for new
flash additions
mtd: spi-nor: core: Move spi_nor_set_addr_width() in spi_nor_setup()
mtd: spi-nor: sst: sst26vf064b: Init flash based on SFDP
mtd: spi-nor: winbond: w25q256jvm: Init flash based on SFDP
mtd: spi-nor: spansion: s25fl256s0: Skip SFDP parsing
mtd: spi-nor: gigadevice: gd25q256: Init flash based on SFDP
mtd: spi-nor: issi: is25lp256: Init flash based on SFDP
mtd: spi-nor: macronix: mx25l25635e: Init flash based on SFDP
drivers/mtd/spi-nor/atmel.c | 8 +-
drivers/mtd/spi-nor/core.c | 480 +++++++++++++++++--------------
drivers/mtd/spi-nor/core.h | 106 ++++---
drivers/mtd/spi-nor/gigadevice.c | 7 +-
drivers/mtd/spi-nor/issi.c | 6 +-
drivers/mtd/spi-nor/macronix.c | 15 +-
drivers/mtd/spi-nor/micron-st.c | 20 +-
drivers/mtd/spi-nor/otp.c | 2 +-
drivers/mtd/spi-nor/spansion.c | 15 +-
drivers/mtd/spi-nor/sst.c | 105 ++++---
drivers/mtd/spi-nor/swp.c | 2 +-
drivers/mtd/spi-nor/winbond.c | 7 +-
drivers/mtd/spi-nor/xilinx.c | 21 +-
include/linux/mtd/spi-nor.h | 2 -
14 files changed, 460 insertions(+), 336 deletions(-)
--
2.25.1
_______________________________________________
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-11-09 08:21:21
Am 2021-10-29 19:26, schrieb Tudor Ambarus:
Update the description of the otp member of the
struct spi_nor_flash_parameter.
Signed-off-by: Tudor Ambarus <redacted>
Reviewed-by: Pratyush Yadav <redacted>
---
You've forgot to add my former tag here. Anyway:
Reviewed-by: Michael Walle <redacted>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
"struct mtd_info mtd" is member of "struct spi_nor", there's no need
to use "mtd->priv". Get the pointer to the containing struct spi_nor
by using container_of. While here, make the function inline and
get rid of the __maybe_unused.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/core.c | 1 -
drivers/mtd/spi-nor/core.h | 4 ++--
2 files changed, 2 insertions(+), 3 deletions(-)
From: Michael Walle <hidden> Date: 2021-11-09 08:23:17
Am 2021-10-29 19:26, schrieb Tudor Ambarus:
"struct mtd_info mtd" is member of "struct spi_nor", there's no need
to use "mtd->priv". Get the pointer to the containing struct spi_nor
by using container_of. While here, make the function inline and
get rid of the __maybe_unused.
Signed-off-by: Tudor Ambarus <redacted>
"struct mtd_info mtd" is member of "struct spi_nor", there's no need
to use "mtd->priv". Get the pointer to the containing struct spi_nor
by using container_of. While here, make the function inline and
get rid of the __maybe_unused.
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
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>
---
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(-)
@@ -1952,6 +1952,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);
@@ -1968,16 +1969,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;
From: Michael Walle <hidden> Date: 2021-11-09 08:27:16
Am 2021-10-29 19:26, schrieb Tudor Ambarus:
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>
IIRC this patch already had my:
Reviewed-by: Michael Walle <redacted>
_______________________________________________
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-10-29 19:26, schrieb Tudor Ambarus:
quoted
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>
IIRC this patch already had my:
You're right, sorry. Next time I'll download the patches
from ml, to get the tags automatically.
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.
While here use common naming scheme for functions that are setting
mtd_info fields:
s/spi_nor_register_locking_ops/spi_nor_set_mtd_locking_ops
s/spi_nor_otp_init/spi_nor_set_mtd_otp_ops
The functions names are self explanatory, get rid of the comment
for the OTP function.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/core.c | 54 +++++++++++++++++++++-----------------
drivers/mtd/spi-nor/core.h | 4 +--
drivers/mtd/spi-nor/otp.c | 2 +-
drivers/mtd/spi-nor/swp.c | 2 +-
4 files changed, 34 insertions(+), 28 deletions(-)
@@ -3071,6 +3071,35 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,returninfo;}+staticvoidspi_nor_set_mtd_info(structspi_nor*nor)+{+structmtd_info*mtd=&nor->mtd;+structdevice*dev=nor->dev;++spi_nor_set_mtd_locking_ops(nor);+spi_nor_set_mtd_otp_ops(nor);++mtd->dev.parent=dev;+if(!mtd->name)+mtd->name=dev_name(dev);+mtd->type=MTD_NORFLASH;+mtd->flags=MTD_CAP_NORFLASH;+if(nor->info->flags&SPI_NOR_NO_ERASE)+mtd->flags|=MTD_NO_ERASE;+mtd->writesize=nor->params->writesize;+mtd->writebufsize=nor->page_size;+mtd->size=nor->params->size;+mtd->_erase=spi_nor_erase;+mtd->_read=spi_nor_read;+/* Might be already set by some SST flashes. */+if(!mtd->_write)+mtd->_write=spi_nor_write;+mtd->_suspend=spi_nor_suspend;+mtd->_resume=spi_nor_resume;+mtd->_get_device=spi_nor_get_device;+mtd->_put_device=spi_nor_put_device;+}+intspi_nor_scan(structspi_nor*nor,constchar*name,conststructspi_nor_hwcaps*hwcaps){
@@ -3125,26 +3154,11 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,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 */ret=spi_nor_init_params(nor);if(ret)returnret;-if(!mtd->name)-mtd->name=dev_name(dev);-mtd->type=MTD_NORFLASH;-mtd->writesize=nor->params->writesize;-mtd->flags=MTD_CAP_NORFLASH;-mtd->size=nor->params->size;-mtd->_erase=spi_nor_erase;-mtd->_read=spi_nor_read;-mtd->_suspend=spi_nor_suspend;-mtd->_resume=spi_nor_resume;-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){
From: Michael Walle <hidden> Date: 2021-11-09 08:25:05
Am 2021-10-29 19:26, schrieb Tudor Ambarus:
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.
While here use common naming scheme for functions that are setting
mtd_info fields:
s/spi_nor_register_locking_ops/spi_nor_set_mtd_locking_ops
s/spi_nor_otp_init/spi_nor_set_mtd_otp_ops
The functions names are self explanatory, get rid of the comment
for the OTP function.
Signed-off-by: Tudor Ambarus <redacted>
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.
While here use common naming scheme for functions that are setting
mtd_info fields:
s/spi_nor_register_locking_ops/spi_nor_set_mtd_locking_ops
s/spi_nor_otp_init/spi_nor_set_mtd_otp_ops
The functions names are self explanatory, get rid of the comment
for the OTP function.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/core.c | 54 +++++++++++++++++++++-----------------
drivers/mtd/spi-nor/core.h | 4 +--
drivers/mtd/spi-nor/otp.c | 2 +-
drivers/mtd/spi-nor/swp.c | 2 +-
4 files changed, 34 insertions(+), 28 deletions(-)
@@ -3071,6 +3071,35 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,returninfo;}+staticvoidspi_nor_set_mtd_info(structspi_nor*nor)+{+structmtd_info*mtd=&nor->mtd;+structdevice*dev=nor->dev;++spi_nor_set_mtd_locking_ops(nor);+spi_nor_set_mtd_otp_ops(nor);++mtd->dev.parent=dev;+if(!mtd->name)+mtd->name=dev_name(dev);+mtd->type=MTD_NORFLASH;+mtd->flags=MTD_CAP_NORFLASH;+if(nor->info->flags&SPI_NOR_NO_ERASE)+mtd->flags|=MTD_NO_ERASE;+mtd->writesize=nor->params->writesize;+mtd->writebufsize=nor->page_size;+mtd->size=nor->params->size;+mtd->_erase=spi_nor_erase;+mtd->_read=spi_nor_read;+/* Might be already set by some SST flashes. */+if(!mtd->_write)+mtd->_write=spi_nor_write;+mtd->_suspend=spi_nor_suspend;+mtd->_resume=spi_nor_resume;+mtd->_get_device=spi_nor_get_device;+mtd->_put_device=spi_nor_put_device;
You should also merge in spi_nor_debugfs_init() in here which
initializes mtd->dbg.
@@ -3196,15 +3205,12 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, if (ret) return ret;- spi_nor_register_locking_ops(nor);- /* Send all the required SPI flash commands to initialize device */ ret = spi_nor_init(nor); if (ret) return ret;- /* Configure OTP parameters and ops */- spi_nor_otp_init(nor);+ spi_nor_set_mtd_info(nor);
This will break multiple things.
- spi_nor_set_addr_width(), which is called by spi_nor-setup(). It
checks for nor->mtd.size which has not been set yet.
- spi_nor_spimem_check_op(), which is called (indirectly) by
spi_nor_default_setup(). It check for nor->mtd.size.
- spi_nor_try_unlock_all(), which is called by spi_nor_init(). I don't
think it actually uses any values you initialize here but still worth
pointing out.
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
On 29/10/21 08:26PM, Tudor Ambarus wrote:
quoted
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.
While here use common naming scheme for functions that are setting
mtd_info fields:
s/spi_nor_register_locking_ops/spi_nor_set_mtd_locking_ops
s/spi_nor_otp_init/spi_nor_set_mtd_otp_ops
The functions names are self explanatory, get rid of the comment
for the OTP function.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/core.c | 54 +++++++++++++++++++++-----------------
drivers/mtd/spi-nor/core.h | 4 +--
drivers/mtd/spi-nor/otp.c | 2 +-
drivers/mtd/spi-nor/swp.c | 2 +-
4 files changed, 34 insertions(+), 28 deletions(-)
@@ -3071,6 +3071,35 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,returninfo;}+staticvoidspi_nor_set_mtd_info(structspi_nor*nor)+{+structmtd_info*mtd=&nor->mtd;+structdevice*dev=nor->dev;++spi_nor_set_mtd_locking_ops(nor);+spi_nor_set_mtd_otp_ops(nor);++mtd->dev.parent=dev;+if(!mtd->name)+mtd->name=dev_name(dev);+mtd->type=MTD_NORFLASH;+mtd->flags=MTD_CAP_NORFLASH;+if(nor->info->flags&SPI_NOR_NO_ERASE)+mtd->flags|=MTD_NO_ERASE;+mtd->writesize=nor->params->writesize;+mtd->writebufsize=nor->page_size;+mtd->size=nor->params->size;+mtd->_erase=spi_nor_erase;+mtd->_read=spi_nor_read;+/* Might be already set by some SST flashes. */+if(!mtd->_write)+mtd->_write=spi_nor_write;+mtd->_suspend=spi_nor_suspend;+mtd->_resume=spi_nor_resume;+mtd->_get_device=spi_nor_get_device;+mtd->_put_device=spi_nor_put_device;
You should also merge in spi_nor_debugfs_init() in here which
initializes mtd->dbg.
I was thinking of getting rid of debugfs entries since they duplicate
those on sysfs.
@@ -3196,15 +3205,12 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, if (ret) return ret;- spi_nor_register_locking_ops(nor);- /* Send all the required SPI flash commands to initialize device */ ret = spi_nor_init(nor); if (ret) return ret;- /* Configure OTP parameters and ops */- spi_nor_otp_init(nor);+ spi_nor_set_mtd_info(nor);
This will break multiple things.
this is gold :), thanks
- spi_nor_set_addr_width(), which is called by spi_nor-setup(). It
checks for nor->mtd.size which has not been set yet.> - spi_nor_spimem_check_op(), which is called (indirectly) by
spi_nor_default_setup(). It check for nor->mtd.size.
Let's use NOR parameters in SPI NOR core, thus nor->params->size instead,
and let the mtd fields for mtd. This will spare us of these problems.
And it's cleaner too.
- spi_nor_try_unlock_all(), which is called by spi_nor_init(). I don't
think it actually uses any values you initialize here but still worth
pointing out.
we are safe here, the pointer to mtd is used just to get the pointer to
nor.
Thanks again.
ta
On 16/11/21 02:25PM, Tudor.Ambarus@microchip.com wrote:
On 11/15/21 8:52 PM, Pratyush Yadav wrote:
quoted
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
On 29/10/21 08:26PM, Tudor Ambarus wrote:
quoted
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.
While here use common naming scheme for functions that are setting
mtd_info fields:
s/spi_nor_register_locking_ops/spi_nor_set_mtd_locking_ops
s/spi_nor_otp_init/spi_nor_set_mtd_otp_ops
The functions names are self explanatory, get rid of the comment
for the OTP function.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/core.c | 54 +++++++++++++++++++++-----------------
drivers/mtd/spi-nor/core.h | 4 +--
drivers/mtd/spi-nor/otp.c | 2 +-
drivers/mtd/spi-nor/swp.c | 2 +-
4 files changed, 34 insertions(+), 28 deletions(-)
@@ -3071,6 +3071,35 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,returninfo;}+staticvoidspi_nor_set_mtd_info(structspi_nor*nor)+{+structmtd_info*mtd=&nor->mtd;+structdevice*dev=nor->dev;++spi_nor_set_mtd_locking_ops(nor);+spi_nor_set_mtd_otp_ops(nor);++mtd->dev.parent=dev;+if(!mtd->name)+mtd->name=dev_name(dev);+mtd->type=MTD_NORFLASH;+mtd->flags=MTD_CAP_NORFLASH;+if(nor->info->flags&SPI_NOR_NO_ERASE)+mtd->flags|=MTD_NO_ERASE;+mtd->writesize=nor->params->writesize;+mtd->writebufsize=nor->page_size;+mtd->size=nor->params->size;+mtd->_erase=spi_nor_erase;+mtd->_read=spi_nor_read;+/* Might be already set by some SST flashes. */+if(!mtd->_write)+mtd->_write=spi_nor_write;+mtd->_suspend=spi_nor_suspend;+mtd->_resume=spi_nor_resume;+mtd->_get_device=spi_nor_get_device;+mtd->_put_device=spi_nor_put_device;
You should also merge in spi_nor_debugfs_init() in here which
initializes mtd->dbg.
I was thinking of getting rid of debugfs entries since they duplicate
those on sysfs.
@@ -3196,15 +3205,12 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, if (ret) return ret;- spi_nor_register_locking_ops(nor);- /* Send all the required SPI flash commands to initialize device */ ret = spi_nor_init(nor); if (ret) return ret;- /* Configure OTP parameters and ops */- spi_nor_otp_init(nor);+ spi_nor_set_mtd_info(nor);
This will break multiple things.
this is gold :), thanks
quoted
- spi_nor_set_addr_width(), which is called by spi_nor-setup(). It
checks for nor->mtd.size which has not been set yet.> - spi_nor_spimem_check_op(), which is called (indirectly) by
spi_nor_default_setup(). It check for nor->mtd.size.
Let's use NOR parameters in SPI NOR core, thus nor->params->size instead,
and let the mtd fields for mtd. This will spare us of these problems.
And it's cleaner too.
I agree.
quoted
- spi_nor_try_unlock_all(), which is called by spi_nor_init(). I don't
think it actually uses any values you initialize here but still worth
pointing out.
we are safe here, the pointer to mtd is used just to get the pointer to
nor.
Yeah, but who knows if that might change some time later. I would prefer
we don't use a member we haven't initialized yet.
- spi_nor_try_unlock_all(), which is called by spi_nor_init(). I don't
think it actually uses any values you initialize here but still worth
pointing out.
we are safe here, the pointer to mtd is used just to get the pointer to
nor.
Yeah, but who knows if that might change some time later. I would prefer
we don't use a member we haven't initialized yet.
If it weren't for the SPI NOR controller drivers that use
spi_nor_scan(), I would put the spi_nor_set_mtd_info() just
above the mtd_device_register(). It will indicate that no mtd_info
field is used up to that point, less things to worry about.
spi_nor_try_unlock_all() calls
spi_nor_unlock(&nor->mtd, 0, nor->params->size);
I can't see for now if we will ever need some specific mtd_info
parameter. I would say that we won't, we're just unlocking the full
flash, every info we would need we can obtain from NOR. The discussion
would be different if it were about mtd partitions, but it isn't, we're
dealing with the entire flash.
Would you accept the place where I put spi_nor_set_mtd_info() if I add
a comment before calling it? Something like:
/* No mtd_info fields are used up to this point. */
spi_nor_set_mtd_info();
Cheers,
ta
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 17/11/21 02:36PM, Tudor.Ambarus@microchip.com wrote:
On 11/16/21 8:11 PM, Pratyush Yadav wrote:
quoted
quoted
quoted
- spi_nor_try_unlock_all(), which is called by spi_nor_init(). I don't
think it actually uses any values you initialize here but still worth
pointing out.
we are safe here, the pointer to mtd is used just to get the pointer to
nor.
Yeah, but who knows if that might change some time later. I would prefer
we don't use a member we haven't initialized yet.
If it weren't for the SPI NOR controller drivers that use
spi_nor_scan(), I would put the spi_nor_set_mtd_info() just
above the mtd_device_register(). It will indicate that no mtd_info
field is used up to that point, less things to worry about.
spi_nor_try_unlock_all() calls
spi_nor_unlock(&nor->mtd, 0, nor->params->size);
I can't see for now if we will ever need some specific mtd_info
parameter. I would say that we won't, we're just unlocking the full
flash, every info we would need we can obtain from NOR. The discussion
would be different if it were about mtd partitions, but it isn't, we're
dealing with the entire flash.
Would you accept the place where I put spi_nor_set_mtd_info() if I add
a comment before calling it? Something like:
/* No mtd_info fields are used up to this point. */
spi_nor_set_mtd_info();
I see that everything that spi_nor_set_mtd_info() needs is set by the
time spi_nor_init_params() is finished. Everything after that is
concerned about selecting the protocol and sending the init commands to
the flash. So why can't you call it right after spi_nor_init_params()?
That and updating spi_nor_spimem_check_op() and spi_nor_set_addr_width()
to use nor->params->size instead of nor->mtd.size should do the trick.
I think that it is implied that mtd_info fields are not being used until
they are initialized so I don't think the comment itself is of much use,
but I don't care much about it either way.
--
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
Hi, Pratyush,
On 11/19/21 8:23 PM, Pratyush Yadav wrote:
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
On 17/11/21 02:36PM, Tudor.Ambarus@microchip.com wrote:
quoted
On 11/16/21 8:11 PM, Pratyush Yadav wrote:
quoted
quoted
quoted
- spi_nor_try_unlock_all(), which is called by spi_nor_init(). I don't
think it actually uses any values you initialize here but still worth
pointing out.
we are safe here, the pointer to mtd is used just to get the pointer to
nor.
Yeah, but who knows if that might change some time later. I would prefer
we don't use a member we haven't initialized yet.
If it weren't for the SPI NOR controller drivers that use
spi_nor_scan(), I would put the spi_nor_set_mtd_info() just
above the mtd_device_register(). It will indicate that no mtd_info
field is used up to that point, less things to worry about.
spi_nor_try_unlock_all() calls
spi_nor_unlock(&nor->mtd, 0, nor->params->size);
I can't see for now if we will ever need some specific mtd_info
parameter. I would say that we won't, we're just unlocking the full
flash, every info we would need we can obtain from NOR. The discussion
would be different if it were about mtd partitions, but it isn't, we're
dealing with the entire flash.
Would you accept the place where I put spi_nor_set_mtd_info() if I add
a comment before calling it? Something like:
/* No mtd_info fields are used up to this point. */
spi_nor_set_mtd_info();
I see that everything that spi_nor_set_mtd_info() needs is set by the
time spi_nor_init_params() is finished. Everything after that is
concerned about selecting the protocol and sending the init commands to
the flash. So why can't you call it right after spi_nor_init_params()?
Because I would like to move it just above mtd_device_register() in the future.
If unlock_all() will need some mtd fields in the future, we can introduce a
spi_nor_prepare_mtd_for_unlock_all(). I don't want the mtd fields init to be
scattered through the SPI NOR core. They shouldn't be used in the NOR's
probe sequence of calls anyway, keeping them closer to mtd_device_register()
makes the code easier to grasp I think.
I will respin the series soon and wanted to let you know why I kept
spi_nor_set_mtd_info() where it is in this patch set.
Cheers,
ta
That and updating spi_nor_spimem_check_op() and spi_nor_set_addr_width()
to use nor->params->size instead of nor->mtd.size should do the trick.
I think that it is implied that mtd_info fields are not being used until
they are initialized so I don't think the comment itself is of much use,
but I don't care much about it either way.
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Flash parameters init is done in a spaghetti way right now.
There is the init based on the flash_info data, then there is the
default_init() hook, then SFDP init, an intermediary post_bft(),
then post_sfdp() and a spi_nor_late_init_params(). Each method can
overwrite previuosly initialized parameters.
We want to separate what is SFDP and non-SFDP specific. late_init()
will replace the default_init() hook and will be used only to initialize
flash parameters that are not declared in the JESD216 SFDP standard, or
where SFDP tables are not defined at all.
We cut a member in the chain of initializing parameters by getting rid
of the default_init() hook, and we make it clear that everything that is
in late_init() is not covered by the SFDP tables defined by the flash.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/core.c | 17 +++++++++++++----
drivers/mtd/spi-nor/core.h | 4 ++++
2 files changed, 17 insertions(+), 4 deletions(-)
From: Michael Walle <hidden> Date: 2021-11-09 09:33:22
Am 2021-10-29 19:26, schrieb Tudor Ambarus:
Flash parameters init is done in a spaghetti way right now.
There is the init based on the flash_info data, then there is the
default_init() hook, then SFDP init, an intermediary post_bft(),
then post_sfdp() and a spi_nor_late_init_params(). Each method can
overwrite previuosly initialized parameters.
We want to separate what is SFDP and non-SFDP specific. late_init()
will replace the default_init() hook and will be used only to
initialize
flash parameters that are not declared in the JESD216 SFDP standard, or
where SFDP tables are not defined at all.
We cut a member in the chain of initializing parameters by getting rid
of the default_init() hook, and we make it clear that everything that
is
in late_init() is not covered by the SFDP tables defined by the flash.
Signed-off-by: Tudor Ambarus <redacted>
Flash parameters init is done in a spaghetti way right now.
There is the init based on the flash_info data, then there is the
default_init() hook, then SFDP init, an intermediary post_bft(),
then post_sfdp() and a spi_nor_late_init_params(). Each method can
overwrite previuosly initialized parameters.
We want to separate what is SFDP and non-SFDP specific. late_init()
will replace the default_init() hook and will be used only to initialize
flash parameters that are not declared in the JESD216 SFDP standard, or
where SFDP tables are not defined at all.
We cut a member in the chain of initializing parameters by getting rid
of the default_init() hook, and we make it clear that everything that is
in late_init() is not covered by the SFDP tables defined by the flash.
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
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 | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
OTP is not described in the JESD216 SFDP standard, place the
OTP ops init in late_init().
We can't get rid of the default_init() hook for winbond, as the
4byte_addr_mode is SFDP specific and will require to have all
flashes at hand, in order to check which has the SFDP tables defined,
in which case there's nothing to do if the SFDP tables are corect,
and which of the flashes do not define the SFDP tables in which case
each flash should declare a late_init() fixup.
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/winbond.c | 5 +++++
1 file changed, 5 insertions(+)
From: Michael Walle <hidden> Date: 2021-11-09 09:41:44
Am 2021-10-29 19:26, schrieb Tudor Ambarus:
OTP is not described in the JESD216 SFDP standard, place the
OTP ops init in late_init().
We can't get rid of the default_init() hook for winbond, as the
4byte_addr_mode is SFDP specific and will require to have all
flashes at hand, in order to check which has the SFDP tables defined,
in which case there's nothing to do if the SFDP tables are corect,
and which of the flashes do not define the SFDP tables in which case
each flash should declare a late_init() fixup.
Signed-off-by: Tudor Ambarus <redacted>
OTP is not described in the JESD216 SFDP standard, place the
OTP ops init in late_init().
We can't get rid of the default_init() hook for winbond, as the
4byte_addr_mode is SFDP specific and will require to have all
flashes at hand, in order to check which has the SFDP tables defined,
in which case there's nothing to do if the SFDP tables are corect,
and which of the flashes do not define the SFDP tables in which case
each flash should declare a late_init() fixup.
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
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/sst.c | 4 ++--
1 file changed, 2 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 | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Michael Walle <hidden> Date: 2021-11-09 09:45:03
Am 2021-10-29 19:26, schrieb Tudor Ambarus:
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>
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>
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
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 | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Michael Walle <hidden> Date: 2021-11-09 09:49:47
Am 2021-10-29 19:26, schrieb Tudor Ambarus:
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>
Reviewed-by: Michael Walle <redacted>
Just a comment below.
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
Am 2021-10-29 19:26, schrieb Tudor Ambarus:
quoted
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>
Reviewed-by: Michael Walle <redacted>
Just a comment below.
SST_WRITE is only used during to set this. Shouldn't this be
a fixup per flash so we don't need that flag.
We should get rid of the SST_WRITE indeed. Do you want me to handle this
or do you want to take care of it yourself?
As a side note, I find the manufacturer fixup hooks and generic settings
not scalable. We may consider to remove the per manufacturer settings
in the future.
Cheers,
ta
SST_WRITE is only used during to set this. Shouldn't this be
a fixup per flash so we don't need that flag.
We should get rid of the SST_WRITE indeed. Do you want me to handle this
or do you want to take care of it yourself?
oh, I forgot, I have already took care of it, check:
[PATCH v3 13/25] mtd: spi-nor: sst: Get rid of SST_WRITE flash_info flag
_______________________________________________
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-11-09 10:27:27
quoted
SST_WRITE is only used during to set this. Shouldn't this be
a fixup per flash so we don't need that flag.
We should get rid of the SST_WRITE indeed. Do you want me to handle
this
or do you want to take care of it yourself?
Well you already got a patch (in this series) which will do exactly this
;)
But I didn't notice, because its later in this series.
-michael
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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>
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
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 | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Michael Walle <hidden> Date: 2021-11-09 09:56:35
Am 2021-10-29 19:26, schrieb Tudor Ambarus:
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>
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>
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_post_sfdp_fixups() was called even when there were no SFDP
tables defined. late_init() should be instead used for flashes that
do not define SFDP tables.
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 | 56 ++++++++++++++++++--------------------
1 file changed, 26 insertions(+), 30 deletions(-)
From: Michael Walle <hidden> Date: 2021-11-09 10:19:47
Am 2021-10-29 19:26, schrieb Tudor Ambarus:
quoted hunk
spi_nor_post_sfdp_fixups() was called even when there were no SFDP
tables defined. late_init() should be instead used for flashes that
do not define SFDP tables.
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 | 56 ++++++++++++++++++--------------------
1 file changed, 26 insertions(+), 30 deletions(-)
spi_nor_manufacturer_init_params(struct spi_nor *nor)
nor->info->fixups->default_init(nor);
}
+/**
+ * spi_nor_post_sfdp_fixups() - Updates the flash's parameters and
settings
+ * after SFDP has been parsed. Called only for flashes that define
JESD216 SFDP
+ * tables.
+ * @nor: pointer to a 'struct spi_nor'
+ *
+ * Used to tweak various flash parameters when information provided by
the SFDP
+ * tables are 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_sfdp_init_params() - Initialize the flash's parameters and
settings
* based on JESD216 SFDP standard.
spi_nor *nor)
memcpy(&sfdp_params, nor->params, sizeof(sfdp_params));
- if (spi_nor_parse_sfdp(nor)) {
+ if (!spi_nor_parse_sfdp(nor)) {
+ spi_nor_post_sfdp_fixups(nor);
I find this function particulary confusing. Why is it
copying the params around? I know the function doc
says rollback, but can't we make this better?
Either make spi_nor_parse_sfdp() commit the nor->params update
atomically, or pass a second parameter sfdp_params, which are
copied to nor->params here if spi_nor_parse_sfdp() was successful.
Also the control flow could be better.
ret = spi_nor_parse_sfdp(nor, &sfdp_params);
if (!ret) {
/* clever comment, why is addr_width = 0 here */
nor->addr_width = 0;
nor->flags &= ~SNOR_F_4B_OPCODES;
return 0;
}
memcpy(nor->params, &sfdp_params, sizeof(*nor->params));
spi_nor_post_sfdp_fixups(nor);
Having an even closer look, addr_width is set to 0 because
spi_nor_parse_sfdp() is also changing that. nor->flags
is also changed and not only the SNOR_F_4B_OPCODES bit. But
only that one is cleared?!
I think this deserves another cleanup series. Having the
rollback here makes no sense. You'd have to keep both in
sync.
IMHO best would be a second parameter and make sure all
code in sfdp.c don't change anything else. Which would
probably mean that addr_width and flags will move to
nor->params.
Anyway, for now:
Reviewed-by: Michael Walle <redacted>
-michael
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
The flash_info flags should be generic and not manufacturer specific.
Get rid of the manufacturer specific flag and use the late_init() fixup
hook instead.
Please note that sst_write is now set at flash level and not globally,
per manufacturer. Manufacturer hooks are generally a bad idea, because
it affects settings for all the flashes and we might end up with fixups
for "manufacturer settings".
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/core.h | 1 -
drivers/mtd/spi-nor/sst.c | 100 +++++++++++++++++++++----------------
2 files changed, 58 insertions(+), 43 deletions(-)
@@ -55,42 +55,6 @@ static const struct spi_nor_fixups sst26vf_fixups = {.late_init=sst26vf_late_init,};-staticconststructflash_infosst_parts[]={-/* SST -- large erase sizes are "overlays", "sectors" are 4K */-{"sst25vf040b",INFO(0xbf258d,0,64*1024,8,-SECT_4K|SST_WRITE|SPI_NOR_HAS_LOCK|SPI_NOR_SWP_IS_VOLATILE)},-{"sst25vf080b",INFO(0xbf258e,0,64*1024,16,-SECT_4K|SST_WRITE|SPI_NOR_HAS_LOCK|SPI_NOR_SWP_IS_VOLATILE)},-{"sst25vf016b",INFO(0xbf2541,0,64*1024,32,-SECT_4K|SST_WRITE|SPI_NOR_HAS_LOCK|SPI_NOR_SWP_IS_VOLATILE)},-{"sst25vf032b",INFO(0xbf254a,0,64*1024,64,-SECT_4K|SST_WRITE|SPI_NOR_HAS_LOCK|SPI_NOR_SWP_IS_VOLATILE)},-{"sst25vf064c",INFO(0xbf254b,0,64*1024,128,-SECT_4K|SPI_NOR_4BIT_BP|SPI_NOR_HAS_LOCK|-SPI_NOR_SWP_IS_VOLATILE)},-{"sst25wf512",INFO(0xbf2501,0,64*1024,1,-SECT_4K|SST_WRITE|SPI_NOR_HAS_LOCK|SPI_NOR_SWP_IS_VOLATILE)},-{"sst25wf010",INFO(0xbf2502,0,64*1024,2,-SECT_4K|SST_WRITE|SPI_NOR_HAS_LOCK|SPI_NOR_SWP_IS_VOLATILE)},-{"sst25wf020",INFO(0xbf2503,0,64*1024,4,-SECT_4K|SST_WRITE|SPI_NOR_HAS_LOCK|SPI_NOR_SWP_IS_VOLATILE)},-{"sst25wf020a",INFO(0x621612,0,64*1024,4,SECT_4K|SPI_NOR_HAS_LOCK)},-{"sst25wf040b",INFO(0x621613,0,64*1024,8,SECT_4K|SPI_NOR_HAS_LOCK)},-{"sst25wf040",INFO(0xbf2504,0,64*1024,8,-SECT_4K|SST_WRITE|SPI_NOR_HAS_LOCK|SPI_NOR_SWP_IS_VOLATILE)},-{"sst25wf080",INFO(0xbf2505,0,64*1024,16,-SECT_4K|SST_WRITE|SPI_NOR_HAS_LOCK|SPI_NOR_SWP_IS_VOLATILE)},-{"sst26wf016b",INFO(0xbf2651,0,64*1024,32,-SECT_4K|SPI_NOR_DUAL_READ|-SPI_NOR_QUAD_READ)},-{"sst26vf016b",INFO(0xbf2641,0,64*1024,32,-SECT_4K|SPI_NOR_DUAL_READ)},-{"sst26vf064b",INFO(0xbf2643,0,64*1024,128,-SECT_4K|SPI_NOR_DUAL_READ|SPI_NOR_QUAD_READ|-SPI_NOR_HAS_LOCK|SPI_NOR_SWP_IS_VOLATILE)-.fixups=&sst26vf_fixups},-};-staticintsst_write(structmtd_info*mtd,loff_tto,size_tlen,size_t*retlen,constu_char*buf){
@@ -177,19 +141,71 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,returnret;}-staticvoidsst_late_init(structspi_nor*nor)+staticvoidsst_write_late_init(structspi_nor*nor){-if(nor->info->flags&SST_WRITE)-nor->mtd._write=sst_write;+nor->mtd._write=sst_write;}-staticconststructspi_nor_fixupssst_fixups={-.late_init=sst_late_init,+staticconststructspi_nor_fixupssst_write_fixup={+.late_init=sst_write_late_init,+};++staticconststructflash_infosst_parts[]={+/* SST -- large erase sizes are "overlays", "sectors" are 4K */+{"sst25vf040b",INFO(0xbf258d,0,64*1024,8,+SECT_4K|SPI_NOR_HAS_LOCK|+SPI_NOR_SWP_IS_VOLATILE)+.fixups=&sst_write_fixup},+{"sst25vf080b",INFO(0xbf258e,0,64*1024,16,+SECT_4K|SPI_NOR_HAS_LOCK|+SPI_NOR_SWP_IS_VOLATILE)+.fixups=&sst_write_fixup},+{"sst25vf016b",INFO(0xbf2541,0,64*1024,32,+SECT_4K|SPI_NOR_HAS_LOCK|+SPI_NOR_SWP_IS_VOLATILE)+.fixups=&sst_write_fixup},+{"sst25vf032b",INFO(0xbf254a,0,64*1024,64,+SECT_4K|SPI_NOR_HAS_LOCK|+SPI_NOR_SWP_IS_VOLATILE)+.fixups=&sst_write_fixup},+{"sst25vf064c",INFO(0xbf254b,0,64*1024,128,+SECT_4K|SPI_NOR_4BIT_BP|SPI_NOR_HAS_LOCK|+SPI_NOR_SWP_IS_VOLATILE)},+{"sst25wf512",INFO(0xbf2501,0,64*1024,1,+SECT_4K|SPI_NOR_HAS_LOCK|+SPI_NOR_SWP_IS_VOLATILE)+.fixups=&sst_write_fixup},+{"sst25wf010",INFO(0xbf2502,0,64*1024,2,+SECT_4K|SPI_NOR_HAS_LOCK|+SPI_NOR_SWP_IS_VOLATILE)+.fixups=&sst_write_fixup},+{"sst25wf020",INFO(0xbf2503,0,64*1024,4,+SECT_4K|SPI_NOR_HAS_LOCK|+SPI_NOR_SWP_IS_VOLATILE)+.fixups=&sst_write_fixup},+{"sst25wf020a",INFO(0x621612,0,64*1024,4,SECT_4K|SPI_NOR_HAS_LOCK)},+{"sst25wf040b",INFO(0x621613,0,64*1024,8,SECT_4K|SPI_NOR_HAS_LOCK)},+{"sst25wf040",INFO(0xbf2504,0,64*1024,8,+SECT_4K|SPI_NOR_HAS_LOCK|+SPI_NOR_SWP_IS_VOLATILE)+.fixups=&sst_write_fixup},+{"sst25wf080",INFO(0xbf2505,0,64*1024,16,+SECT_4K|SPI_NOR_HAS_LOCK|+SPI_NOR_SWP_IS_VOLATILE)+.fixups=&sst_write_fixup},+{"sst26wf016b",INFO(0xbf2651,0,64*1024,32,+SECT_4K|SPI_NOR_DUAL_READ|+SPI_NOR_QUAD_READ)},+{"sst26vf016b",INFO(0xbf2641,0,64*1024,32,+SECT_4K|SPI_NOR_DUAL_READ)},+{"sst26vf064b",INFO(0xbf2643,0,64*1024,128,+SECT_4K|SPI_NOR_DUAL_READ|SPI_NOR_QUAD_READ|+SPI_NOR_HAS_LOCK|SPI_NOR_SWP_IS_VOLATILE)+.fixups=&sst26vf_fixups},};conststructspi_nor_manufacturerspi_nor_sst={.name="sst",.parts=sst_parts,.nparts=ARRAY_SIZE(sst_parts),-.fixups=&sst_fixups,};
--
2.25.1
_______________________________________________
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-11-09 12:23:24
Am 2021-10-29 19:26, schrieb Tudor Ambarus:
The flash_info flags should be generic and not manufacturer specific.
Get rid of the manufacturer specific flag and use the late_init() fixup
hook instead.
Please note that sst_write is now set at flash level and not globally,
per manufacturer. Manufacturer hooks are generally a bad idea, because
it affects settings for all the flashes and we might end up with fixups
for "manufacturer settings".
Signed-off-by: Tudor Ambarus <redacted>
Reviewed-by: Michael Walle <redacted>
I'm still not sure, if having a just one fixup function will scale with
different flashes. What do you think about having an additional (opaque
to the core) (bit)field for the manufacturer and flash fixups functions?
In this case, you can reuse the same function - and then a manufacturer
will make more sense (addressing Pratyush comment about a common
manufacturer fixup here).
I.e. the SST_WRITE flag would go into these flags, set per flash device
and the fixup can still remain in the manufacturer fixup.
-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-10-29 19:26, schrieb Tudor Ambarus:
quoted
The flash_info flags should be generic and not manufacturer specific.
Get rid of the manufacturer specific flag and use the late_init() fixup
hook instead.
Please note that sst_write is now set at flash level and not globally,
per manufacturer. Manufacturer hooks are generally a bad idea, because
it affects settings for all the flashes and we might end up with fixups
for "manufacturer settings".
Signed-off-by: Tudor Ambarus <redacted>
Reviewed-by: Michael Walle <redacted>
I'm still not sure, if having a just one fixup function will scale with
different flashes. What do you think about having an additional (opaque
to the core) (bit)field for the manufacturer and flash fixups functions?
Sounds fine. I can reserve few bits to manufacturers and introduce a
MANUF_FLAGS genmask for the flash_info flags. Each specific manufacturer
flag will be visible just for that manufacturer. Please check patch 14/25,
I can extend the idea from there.
In this case, you can reuse the same function - and then a manufacturer
will make more sense (addressing Pratyush comment about a common
manufacturer fixup here).
I.e. the SST_WRITE flag would go into these flags, set per flash device
and the fixup can still remain in the manufacturer fixup.
Right. Let me know if a MANUF_FLAGS genmask is better than this patch.
Cheers,
ta
_______________________________________________
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-11-12 21:30:07
Am 2021-11-09 13:31, schrieb Tudor.Ambarus@microchip.com:
On 11/9/21 2:21 PM, Michael Walle wrote:
quoted
EXTERNAL EMAIL: Do not click links or open attachments unless you know
the content is safe
Am 2021-10-29 19:26, schrieb Tudor Ambarus:
quoted
The flash_info flags should be generic and not manufacturer specific.
Get rid of the manufacturer specific flag and use the late_init()
fixup
hook instead.
Please note that sst_write is now set at flash level and not
globally,
per manufacturer. Manufacturer hooks are generally a bad idea,
because
it affects settings for all the flashes and we might end up with
fixups
for "manufacturer settings".
Signed-off-by: Tudor Ambarus <redacted>
Reviewed-by: Michael Walle <redacted>
I'm still not sure, if having a just one fixup function will scale
with
different flashes. What do you think about having an additional
(opaque
to the core) (bit)field for the manufacturer and flash fixups
functions?
Sounds fine. I can reserve few bits to manufacturers and introduce a
MANUF_FLAGS genmask for the flash_info flags. Each specific
manufacturer
flag will be visible just for that manufacturer. Please check patch
14/25,
I can extend the idea from there.
quoted
In this case, you can reuse the same function - and then a
manufacturer
will make more sense (addressing Pratyush comment about a common
manufacturer fixup here).
I.e. the SST_WRITE flag would go into these flags, set per flash
device
and the fixup can still remain in the manufacturer fixup.
Right. Let me know if a MANUF_FLAGS genmask is better than this patch.
Clarify for what the flash_info flags are used for. Split them in
three categories:
1/ NON_SFDP_FLAGS: flags that indicate support that is not defined
by the JESD216 standard in its SFDP tables.
2/ SFDP_FLAGS: flags that indicate support that can be discovered
via SFDP. These flags are used when the flash does not define the
SFDP tables. Used together with SPI_NOR_SKIP_SFDP flag.
3/ FIXUP_FLAGS: flags that indicate support that can be discovered
via SFDP ideally, but can not be discovered for this particular flash
because the SFDP table that indicates this support is not defined by
the flash. In case the table for this support is defined but has wrong
values, one should instead use a post_sfdp() hook to set the SNOR_F
equivalent flag.
Manufacturer specific flags like USE_CLSR, USE_FSR, SPI_NOR_XSR_RDY,
will be removed in a future series.
BIT(0) was kept for SPI_NOR_PARSE_SFDP (will be introduced in a
further patch).
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/core.h | 89 ++++++++++++++++++++++++--------------
1 file changed, 57 insertions(+), 32 deletions(-)
From: Michael Walle <hidden> Date: 2021-11-12 21:51:27
Am 2021-10-29 19:26, schrieb Tudor Ambarus:
Clarify for what the flash_info flags are used for. Split them in
three categories:
1/ NON_SFDP_FLAGS: flags that indicate support that is not defined
by the JESD216 standard in its SFDP tables.
2/ SFDP_FLAGS: flags that indicate support that can be discovered
via SFDP. These flags are used when the flash does not define the
SFDP tables. Used together with SPI_NOR_SKIP_SFDP flag.
3/ FIXUP_FLAGS: flags that indicate support that can be discovered
via SFDP ideally, but can not be discovered for this particular
flash
because the SFDP table that indicates this support is not defined by
the flash. In case the table for this support is defined but has
wrong
values, one should instead use a post_sfdp() hook to set the SNOR_F
equivalent flag.
Manufacturer specific flags like USE_CLSR, USE_FSR, SPI_NOR_XSR_RDY,
will be removed in a future series.
While i like the partitioning of the flags, I don't really like
that masks etc. Why don't you just use different members, like
non_sfdp_flags sfdp_flags and fixup_flags?
This will make sure, the flags won't get mixed up. Also I'm
not sure about your intention of the
info_flags = nor->info->flags & SOME_MASK;
pattern in the subsequent patches. But if you have different
members you don't have to do that (although its superfluous
anyway).
The flags of each group can start with BIT(0) and reorganizations
of the flags won't affect other groups and will then make these
patches smaller and easier to review.
If your concern is memory, then you can also use bitfields, no?
To populate the flash_info structure, you already introduced a
new macro. This can then just use the new member instead of
".flags".
quoted hunk
BIT(0) was kept for SPI_NOR_PARSE_SFDP (will be introduced in a
further patch).
Signed-off-by: Tudor Ambarus <redacted>
---
drivers/mtd/spi-nor/core.h | 89 ++++++++++++++++++++++++--------------
1 file changed, 57 insertions(+), 32 deletions(-)
@@ -334,56 +334,81 @@ struct flash_info {u16addr_width;u32flags;-#define SECT_4K BIT(0) /* SPINOR_OP_BE_4K works uniformly */-#define SPI_NOR_NO_ERASE BIT(1) /* No erase command needed */-#define SPI_NOR_NO_FR BIT(3) /* Can't do fastread */-#define SECT_4K_PMC BIT(4) /* SPINOR_OP_BE_4K_PMC works uniformly */-#define SPI_NOR_DUAL_READ BIT(5) /* Flash supports Dual Read */-#define SPI_NOR_QUAD_READ BIT(6) /* Flash supports Quad Read */-#define USE_FSR BIT(7) /* use flag status register */-#define SPI_NOR_HAS_LOCK BIT(8) /* Flash supports lock/unlock via SR
*/
-#define SPI_NOR_HAS_TB BIT(9) /*
+#define SPI_NOR_SKIP_SFDP BIT(1) /* Skip parsing of SFDP tables */
+
+/*
+ * Flags that indicate support that is not defined by the JESD216
standard in
+ * its SFDP tables.
+ */
+#define NON_SFDP_FLAGS_MASK GENMASK(15, 2)
+#define NON_SFDP_FLAGS(x) ((x) & NON_SFDP_FLAGS_MASK)
+#define SPI_NOR_HAS_LOCK BIT(2) /* Flash supports lock/unlock via SR
*/
+#define SPI_NOR_HAS_TB BIT(3) /*
* Flash SR has Top/Bottom (TB) protect
* bit. Must be used with
* SPI_NOR_HAS_LOCK.
*/
-#define SPI_NOR_XSR_RDY BIT(10) /*
- * S3AN flashes have specific opcode to
- * read the status register.
- */
-#define SPI_NOR_4B_OPCODES BIT(11) /*
- * Use dedicated 4byte address op codes
- * to support memory size above 128Mib.
- */
-#define NO_CHIP_ERASE BIT(12) /* Chip does not support chip erase */
-#define SPI_NOR_SKIP_SFDP BIT(13) /* Skip parsing of SFDP tables */
-#define USE_CLSR BIT(14) /* use CLSR command */
-#define SPI_NOR_OCTAL_READ BIT(15) /* Flash supports Octal Read */
-#define SPI_NOR_TB_SR_BIT6 BIT(16) /*
+#define SPI_NOR_TB_SR_BIT6 BIT(4) /*
* Top/Bottom (TB) is bit 6 of
* status register. Must be used with
* SPI_NOR_HAS_TB.
*/
-#define SPI_NOR_4BIT_BP BIT(17) /*
+#define SPI_NOR_4BIT_BP BIT(5) /*
* Flash SR has 4 bit fields (BP0-3)
* for block protection.
*/
-#define SPI_NOR_BP3_SR_BIT6 BIT(18) /*
+#define SPI_NOR_BP3_SR_BIT6 BIT(6) /*
* BP3 is bit 6 of status register.
* Must be used with SPI_NOR_4BIT_BP.
*/
-#define SPI_NOR_OCTAL_DTR_READ BIT(19) /* Flash supports octal DTR
Read. */
-#define SPI_NOR_OCTAL_DTR_PP BIT(20) /* Flash supports Octal DTR Page
Program */
-#define SPI_NOR_IO_MODE_EN_VOLATILE BIT(21) /*
- * Flash enables the best
- * available I/O mode via a
- * volatile bit.
- */
-#define SPI_NOR_SWP_IS_VOLATILE BIT(22) /*
+#define SPI_NOR_SWP_IS_VOLATILE BIT(7) /*
* Flash has volatile software write
* protection bits. Usually these will
* power-up in a write-protected state.
*/
+#define SPI_NOR_NO_ERASE BIT(8) /* No erase command needed */
+#define NO_CHIP_ERASE BIT(9) /* Chip does not support chip erase */
+#define SPI_NOR_NO_FR BIT(10) /* Can't do fastread */
+#define USE_CLSR BIT(11) /* use CLSR command */
+#define USE_FSR BIT(12) /* use flag status register */
+#define SPI_NOR_XSR_RDY BIT(13) /*
+ * S3AN flashes have specific opcode to
+ * read the status register.
+ */
+
+/*
+ * Flags that indicate support that can be discovered via SFDP. Used
when SFDP
+ * tables are not defined in the flash. These flags are used together
with the
+ * SPI_NOR_SKIP_SFDP flag.
+ */
+#define SFDP_FLAGS_MASK GENMASK(23, 16)
+#define SFDP_FLAGS(x) ((x) & SFDP_FLAGS_MASK)
+#define SECT_4K BIT(16) /* SPINOR_OP_BE_4K works uniformly */
+#define SECT_4K_PMC BIT(17) /* SPINOR_OP_BE_4K_PMC works uniformly */
+#define SPI_NOR_DUAL_READ BIT(18) /* Flash supports Dual Read */
+#define SPI_NOR_QUAD_READ BIT(19) /* Flash supports Quad Read */
+#define SPI_NOR_OCTAL_READ BIT(20) /* Flash supports Octal Read */
+#define SPI_NOR_OCTAL_DTR_READ BIT(21) /* Flash supports octal DTR
Read. */
+#define SPI_NOR_OCTAL_DTR_PP BIT(22) /* Flash supports Octal DTR Page
Program */
+
+/*
+ * Flags that indicate support that can be discovered via SFDP
ideally, but can
+ * not be discovered for this particular flash because the SFDP table
that
+ * indicates this support is not defined by the flash. In case the
table for
+ * this support is defined but has wrong values, one should instead
use a
+ * post_sfdp() hook to set the SNOR_F equivalent flag.
+ */
+#define FIXUP_FLAGS_MASK GENMASK(31, 24)
+#define FIXUP_FLAGS(x) ((x) & FIXUP_FLAGS_MASK)
+#define SPI_NOR_4B_OPCODES BIT(24) /*
+ * Use dedicated 4byte address op codes
+ * to support memory size above 128Mib.
+ */
+#define SPI_NOR_IO_MODE_EN_VOLATILE BIT(25) /*
+ * Flash enables the best
+ * available I/O mode via a
+ * volatile bit.
+ */
const struct spi_nor_otp_organization otp_org;
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
Am 2021-10-29 19:26, schrieb Tudor Ambarus:
quoted
Clarify for what the flash_info flags are used for. Split them in
three categories:
1/ NON_SFDP_FLAGS: flags that indicate support that is not defined
by the JESD216 standard in its SFDP tables.
2/ SFDP_FLAGS: flags that indicate support that can be discovered
via SFDP. These flags are used when the flash does not define the
SFDP tables. Used together with SPI_NOR_SKIP_SFDP flag.
3/ FIXUP_FLAGS: flags that indicate support that can be discovered
via SFDP ideally, but can not be discovered for this particular
flash
because the SFDP table that indicates this support is not defined by
the flash. In case the table for this support is defined but has
wrong
values, one should instead use a post_sfdp() hook to set the SNOR_F
equivalent flag.
Manufacturer specific flags like USE_CLSR, USE_FSR, SPI_NOR_XSR_RDY,
will be removed in a future series.
While i like the partitioning of the flags, I don't really like
that masks etc. Why don't you just use different members, like
non_sfdp_flags sfdp_flags and fixup_flags?
because of memory concerns. But I think I can substitute the u32 flags
with u16 and u8 equivalents, so will end with the same memory footprint.
Will implement this, thanks.
ta
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel