[PATCH v3 2/9] mtd: spi-nor: Introduce the concept of bank
Miquel Raynal
miquel.raynal at bootlin.com
Fri Mar 24 08:13:55 PDT 2023
Hi Tudor,
tudor.ambarus at linaro.org wrote on Fri, 17 Mar 2023 03:33:02 +0000:
> On 2/1/23 11:32, Miquel Raynal wrote:
> > Hi Tudor,
> >
> > Jaime, a few questions for you below.
> >
> > tudor.ambarus at linaro.org wrote on Thu, 19 Jan 2023 16:34:28 +0000:
> >
> >> Hi, Miquel,
> >>
> >> On 12/15/22 08:12, Miquel Raynal wrote:
> >>> SPI-NOR chips are made of pages, which gathered in small groups make
> >>
> >> nit: s/SPI-NOR/ SPI NOR
> >
> > Noted.
> >
> >>
> >>> (erase) sectors. Sectors, gathered together, make banks inside the
> >>> chip. So far there was only one bank per device supported, but we are
> >>> about to introduce support for new chips featuring several banks (up to
> >>> 4 so far) where different operations may happen in parallel.
> >>>
> >>> Let's allow describing these additional bank parameters.
> >>>
> >>> Signed-off-by: Miquel Raynal <miquel.raynal at bootlin.com>
> >>> Reviewed-by: Pratyush Yadav <pratyush at kernel.org>
> >>> ---
> >>> drivers/mtd/spi-nor/core.c | 3 ++-
> >>> drivers/mtd/spi-nor/core.h | 16 +++++++++++-----
> >>> 2 files changed, 13 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> >>> index f2c64006f8d7..38a57aac6754 100644
> >>> --- a/drivers/mtd/spi-nor/core.c
> >>> +++ b/drivers/mtd/spi-nor/core.c
> >>> @@ -2539,7 +2539,8 @@ static void spi_nor_init_default_params(struct spi_nor *nor)
> >>> > /* Set SPI NOR sizes. */
> >>> params->writesize = 1;
> >>> - params->size = (u64)info->sector_size * info->n_sectors;
> >>> + params->bank_size = (u64)info->sector_size * info->n_sectors;
> >>> + params->size = params->bank_size * info->n_banks;
> >>
> >> Is the datasheet for these chips public? I see JESD216 says nothing
> >> about flash banks.
> >
> > Jaime, do you have a public datasheet for the MX25UW51245G ?
> >
> >> I'm wondering whether we should keep the n_sectors as the total number of sectors per flash or not.
> >
> > This is indeed a good point and I did think about it when I wrote the
> > initial support. For me it looks like the banks are only relevant for
> > the RWW purpose (for now) so I decided I would keep the changes minimal
> > and not mess with the existing variables further. So I just added a
> > "bank" member, and if you want the number of sectors per bank, you can
> > divide nsectors by the number of banks. Another approach might be, as
> > you ask, to count the number of sectors based on the number of sectors
> > per bank and the number of banks. We can move to this approach later I
> > believe, if ever useful.
> >
> >> Does this flash type support Software Block
> >> Protection? How do they count the sectors on Block Protection, per flash
> >> or per bank?
> >
> > Yes it supports block protection, AFAICS it is a per-sector
> > configuration, which does not care about banks at all. It looks like
> > you can protect 2^n sectors (called "blocks" in the datasheet) from the
> > start or from the end of the device.
>
> then you shouldn't change the n_sectors meaning, and keep it per flash,
> and not per bank. Otherwise you break the block protection support for
> these flashes.
That's totally right, good point. I'll update. Actually that was my
initial intention and the commit log reflected this idea, instead of
what I actually implemented in a second time.
> If you really need the number of sectors per bank, you
> can divide n_sectors (per flash) by nbanks, can't you? So bank_size
> should look like (n_sectors / nbanks) * sector_size.
>
> >
> > Jaime, do you confirm?
> >
> >>> params->page_size = info->page_size;
> >>> > if (!(info->flags & SPI_NOR_NO_FR)) {
> >>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> >>> index dc74c7be3e28..8a067d56c995 100644
> >>> --- a/drivers/mtd/spi-nor/core.h
> >>> +++ b/drivers/mtd/spi-nor/core.h
> >>> @@ -336,7 +336,8 @@ struct spi_nor_otp {
> >>> * by the spi_nor_fixups hooks, or dynamically when parsing the JESD216
> >>> * Serial Flash Discoverable Parameters (SFDP) tables.
> >>> *
> >>> - * @size: the flash memory density in bytes.
> >>> + * @bank_size: the flash memory bank density in bytes.
> >>> + * @size: the total flash memory density in bytes.
> >>> * @writesize Minimal writable flash unit size. Defaults to 1. Set to
> >>> * ECC unit size for ECC-ed flashes.
> >>> * @page_size: the page size of the SPI NOR flash memory.
> >>> @@ -374,6 +375,7 @@ struct spi_nor_otp {
> >>> * @locking_ops: SPI NOR locking methods.
> >>> */
> >>> struct spi_nor_flash_parameter {
> >>> + u64 bank_size;
> >>> u64 size;
> >>> u32 writesize;
> >>> u32 page_size;
> >>> @@ -434,7 +436,8 @@ struct spi_nor_fixups {
> >>> * @id_len: the number of bytes of ID.
> >>> * @sector_size: the size listed here is what works with SPINOR_OP_SE, which
> >>> * isn't necessarily called a "sector" by the vendor.
> >>> - * @n_sectors: the number of sectors.
> >>> + * @n_sectors: the number of sectors per bank.
> >>> + * @n_banks: the number of banks.
> >>> * @page_size: the flash's page size.
> >>> * @addr_nbytes: number of address bytes to send.
> >>> *
> >>> @@ -493,6 +496,7 @@ struct flash_info {
> >>> u8 id_len;
> >>> unsigned sector_size;
> >>> u16 n_sectors;
> >>> + u16 n_banks;
> >>> u16 page_size;
> >>
> >> We can try u8 nbanks for now. And we would define it here, to avoid
> >> struct padding.
> >
> > Ok.
> >
> >>> u8 addr_nbytes;
> >>> > @@ -538,23 +542,25 @@ struct flash_info {
> >>> .id = { SPI_NOR_ID_3ITEMS(_jedec_id), SPI_NOR_ID_3ITEMS(_ext_id) }, \
> >>> .id_len = 6
> >>> > -#define SPI_NOR_GEOMETRY(_sector_size, _n_sectors) \
> >>> +#define SPI_NOR_GEOMETRY(_sector_size, _n_sectors, _n_banks) \
> >>> .sector_size = (_sector_size), \
> >>> .n_sectors = (_n_sectors), \
> >>> + .n_banks = (_n_banks), \
> >>> .page_size = 256
> >>> > /* Used when the "_ext_id" is two bytes at most */
> >>> #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors) \
> >>> SPI_NOR_ID((_jedec_id), (_ext_id)), \
> >>> - SPI_NOR_GEOMETRY((_sector_size), (_n_sectors)),
> >>> + SPI_NOR_GEOMETRY((_sector_size), (_n_sectors), 1),
> >>> > #define INFO6(_jedec_id, _ext_id, _sector_size, _n_sectors) \
> >>> SPI_NOR_ID6((_jedec_id), (_ext_id)), \
> >>> - SPI_NOR_GEOMETRY((_sector_size), (_n_sectors)),
> >>> + SPI_NOR_GEOMETRY((_sector_size), (_n_sectors), 1),
> >>> > #define CAT25_INFO(_sector_size, _n_sectors, _page_size, _addr_nbytes) \
> >>> .sector_size = (_sector_size), \
> >>> .n_sectors = (_n_sectors), \
> >>> + .n_banks = 1, \
> >>> .page_size = (_page_size), \
> >>> .addr_nbytes = (_addr_nbytes), \
> >>> .flags = SPI_NOR_NO_ERASE | SPI_NOR_NO_FR, \
> >>
> >> you need to update S3AN_INFO as well.
> >
> > Completely missed that one, thanks!
> >
> > Cheers,
> > Miquèl
Thanks,
Miquèl
More information about the linux-mtd
mailing list