Re: [PATCH v3 2/9] mtd: spi-nor: Introduce the concept of bank

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Tudor,

tudor.ambarus@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@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@bootlin.com>
> >>> Reviewed-by: Pratyush Yadav <pratyush@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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/