[PATCH v6 1/2] mtd: spi-nor: Delay the initialization of bank_size

[PATCH v6 1/2] mtd: spi-nor: Delay the initialization of bank_size

[Miquel Raynal]

Bank size is derived from the chip's size, which in
spi_nor_init_default_params() can still be zero if the size is not
hardcoded in a table (eg. sfdp parsing). We need to delay the
initialization of this field in the "late" helpers.

Based on the fact that RWW compatible devices will use the bank_size to
derive in which bank an access is made, we absolutely want to avoid
divisions by 0. Right now there is no such opportunity between
spi_nor_init_default_params() and spi_nor_late_init_params() but it is
probably safer to enforce a default value (assuming 1 bank) as early as
possible, just in case some day code using this variable is moved
earlier than expected.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
I made the choice to keep a default value, to be honest this is not
actually needed today, so feel free to drop the line if you don't like
it, both choices seem reasonable to me.
---
 drivers/mtd/spi-nor/core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 9e6a0730cdb8..f436395ca489 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2882,6 +2882,8 @@ static void spi_nor_late_init_params(struct spi_nor *nor)
 	 */
 	if (nor->flags & SNOR_F_HAS_LOCK && !nor->params->locking_ops)
 		spi_nor_init_default_locking_ops(nor);
+
+	nor->params->bank_size = div64_u64(nor->params->size, nor->info->n_banks);
 }
 
 /**
@@ -2948,7 +2950,7 @@ 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 = div64_u64(params->size, info->n_banks);
+	params->bank_size = params->size;
 	params->page_size = info->page_size;
 
 	if (!(info->flags & SPI_NOR_NO_FR)) {
-- 
2.34.1


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

[PATCH v6 2/2] mtd: spi-nor: macronix: Add support for mx25uw51245g with RWW

[Miquel Raynal]

Describe this new part and provide the RWW flag for it.

There is no public datasheet, but here are the sfdp tables plus base
testing to show it works.

$ cat /sys/bus/spi/devices/spi0.0/spi-nor/partname
mx25uw51245g
$ cat /sys/bus/spi/devices/spi0.0/spi-nor/jedec_id
c2813a
$ cat /sys/bus/spi/devices/spi0.0/spi-nor/manufacturer
macronix
$ xxd -p /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
53464450080104fd00070114400000ff8701011c900000ff0a0001080001
00ff05000105200100ff84000102340100ff0000000000000000ffffffff
ffffffffe5208affffffff1f00ff00ff00ff00ffeeffffffffff00ffffff
00ff0c2010d800ff00ff87790100821200e27704674630b030b0f4bdd55c
000000ff101000200000000000007ca14800000000008888000000000000
00400fd1fff30fd1fff300050090000500b1002b0095002b0096727103b8
727103b80000000090a3188200c069960000000000000000727100987271
00b8727100990000000072710098727100f872710099727100f900000000
00000000011501d0727106d8000086500000060100000000020001030002
00000000060100000000000072060002000000eec0697272717100d8f7f6
000a00001445988043060f0021dcffff
$ md5sum /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
047a884cf44d9ffc2a94d3ab37b48c63  /sys/bus/spi/devices/spi0.0/spi-nor/sfdp

$ dd if=/dev/urandom of=./qspi_test bs=1M count=6
6+0 records in
6+0 records out
$ mtd_debug write /dev/mtd1 0 6291456 qspi_test
Copied 6291456 bytes from qspi_test to address 0x00000000 in flash
$ mtd_debug erase /dev/mtd1 0 6291456
Erased 6291456 bytes from address 0x00000000 in flash
$ mtd_debug read /dev/mtd1 0 6291456 qspi_read
Copied 6291456 bytes from address 0x00000000 in flash to qspi_read
$ hexdump qspi_read
0000000 ffff ffff ffff ffff ffff ffff ffff ffff
*
0600000
$ mtd_debug write /dev/mtd1 0 6291456 qspi_test
Copied 6291456 bytes from qspi_test to address 0x00000000 in flash
$ mtd_debug read /dev/mtd1 0 6291456 qspi_read
Copied 6291456 bytes from address 0x00000000 in flash to qspi_read
$ sha1sum qspi_test qspi_read
d24a9523db829a0df688f34b8dc76a1383b74024  qspi_test
d24a9523db829a0df688f34b8dc76a1383b74024  qspi_read

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/spi-nor/macronix.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
index 6853ec9ae65d..7db1a1a6a1fb 100644
--- a/drivers/mtd/spi-nor/macronix.c
+++ b/drivers/mtd/spi-nor/macronix.c
@@ -82,6 +82,9 @@ static const struct flash_info macronix_nor_parts[] = {
 	{ "mx25u51245g", INFO(0xc2253a, 0, 64 * 1024, 1024)
 		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
 		FIXUP_FLAGS(SPI_NOR_4B_OPCODES) },
+	{ "mx25uw51245g", INFOB(0xc2813a, 0, 0, 0, 4)
+		PARSE_SFDP
+		FLAGS(SPI_NOR_RWW) },
 	{ "mx25v8035f",  INFO(0xc22314, 0, 64 * 1024,  16)
 		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ |
 			      SPI_NOR_QUAD_READ) },
-- 
2.34.1


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

Re: [PATCH v6 1/2] mtd: spi-nor: Delay the initialization of bank_size

[Tudor Ambarus]

On 3/31/23 20:46, Miquel Raynal wrote:
> Bank size is derived from the chip's size, which in
> spi_nor_init_default_params() can still be zero if the size is not
> hardcoded in a table (eg. sfdp parsing). We need to delay the
> initialization of this field in the "late" helpers.
> 
> Based on the fact that RWW compatible devices will use the bank_size to
> derive in which bank an access is made, we absolutely want to avoid
> divisions by 0. Right now there is no such opportunity between
> spi_nor_init_default_params() and spi_nor_late_init_params() but it is
> probably safer to enforce a default value (assuming 1 bank) as early as
> possible, just in case some day code using this variable is moved
> earlier than expected.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
> I made the choice to keep a default value, to be honest this is not
> actually needed today, so feel free to drop the line if you don't like
> it, both choices seem reasonable to me.
> ---
>  drivers/mtd/spi-nor/core.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 9e6a0730cdb8..f436395ca489 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2882,6 +2882,8 @@ static void spi_nor_late_init_params(struct spi_nor *nor)
>  	 */
>  	if (nor->flags & SNOR_F_HAS_LOCK && !nor->params->locking_ops)
>  		spi_nor_init_default_locking_ops(nor);
> +
> +	nor->params->bank_size = div64_u64(nor->params->size, nor->info->n_banks);
>  }
>  
>  /**
> @@ -2948,7 +2950,7 @@ 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 = div64_u64(params->size, info->n_banks);
> +	params->bank_size = params->size;

I'll get rid of this line entirely as you overwrite it anyway in
late_init and it's not used in between.

>  	params->page_size = info->page_size;
>  
>  	if (!(info->flags & SPI_NOR_NO_FR)) {

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

Re: [PATCH v6 2/2] mtd: spi-nor: macronix: Add support for mx25uw51245g with RWW

[Tudor Ambarus]

On 3/31/23 20:46, Miquel Raynal wrote:
> Describe this new part and provide the RWW flag for it.
> 
> There is no public datasheet, but here are the sfdp tables plus base
> testing to show it works.
> 
> $ cat /sys/bus/spi/devices/spi0.0/spi-nor/partname
> mx25uw51245g
> $ cat /sys/bus/spi/devices/spi0.0/spi-nor/jedec_id
> c2813a
> $ cat /sys/bus/spi/devices/spi0.0/spi-nor/manufacturer
> macronix
> $ xxd -p /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
> 53464450080104fd00070114400000ff8701011c900000ff0a0001080001
> 00ff05000105200100ff84000102340100ff0000000000000000ffffffff
> ffffffffe5208affffffff1f00ff00ff00ff00ffeeffffffffff00ffffff
> 00ff0c2010d800ff00ff87790100821200e27704674630b030b0f4bdd55c
> 000000ff101000200000000000007ca14800000000008888000000000000
> 00400fd1fff30fd1fff300050090000500b1002b0095002b0096727103b8
> 727103b80000000090a3188200c069960000000000000000727100987271
> 00b8727100990000000072710098727100f872710099727100f900000000
> 00000000011501d0727106d8000086500000060100000000020001030002
> 00000000060100000000000072060002000000eec0697272717100d8f7f6
> 000a00001445988043060f0021dcffff
> $ md5sum /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
> 047a884cf44d9ffc2a94d3ab37b48c63  /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
> 
> $ dd if=/dev/urandom of=./qspi_test bs=1M count=6
> 6+0 records in
> 6+0 records out
> $ mtd_debug write /dev/mtd1 0 6291456 qspi_test
> Copied 6291456 bytes from qspi_test to address 0x00000000 in flash
> $ mtd_debug erase /dev/mtd1 0 6291456
> Erased 6291456 bytes from address 0x00000000 in flash
> $ mtd_debug read /dev/mtd1 0 6291456 qspi_read
> Copied 6291456 bytes from address 0x00000000 in flash to qspi_read
> $ hexdump qspi_read
> 0000000 ffff ffff ffff ffff ffff ffff ffff ffff
> *
> 0600000
> $ mtd_debug write /dev/mtd1 0 6291456 qspi_test
> Copied 6291456 bytes from qspi_test to address 0x00000000 in flash
> $ mtd_debug read /dev/mtd1 0 6291456 qspi_read
> Copied 6291456 bytes from address 0x00000000 in flash to qspi_read
> $ sha1sum qspi_test qspi_read
> d24a9523db829a0df688f34b8dc76a1383b74024  qspi_test
> d24a9523db829a0df688f34b8dc76a1383b74024  qspi_read
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/mtd/spi-nor/macronix.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
> index 6853ec9ae65d..7db1a1a6a1fb 100644
> --- a/drivers/mtd/spi-nor/macronix.c
> +++ b/drivers/mtd/spi-nor/macronix.c
> @@ -82,6 +82,9 @@ static const struct flash_info macronix_nor_parts[] = {
>  	{ "mx25u51245g", INFO(0xc2253a, 0, 64 * 1024, 1024)
>  		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
>  		FIXUP_FLAGS(SPI_NOR_4B_OPCODES) },
> +	{ "mx25uw51245g", INFOB(0xc2813a, 0, 0, 0, 4)
> +		PARSE_SFDP
> +		FLAGS(SPI_NOR_RWW) },
>  	{ "mx25v8035f",  INFO(0xc22314, 0, 64 * 1024,  16)
>  		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ |
>  			      SPI_NOR_QUAD_READ) },


looks good!

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

Re: [PATCH v6 1/2] mtd: spi-nor: Delay the initialization of bank_size

[Miquel Raynal]

Hi Tudor,

tudor.ambarus@linaro.org wrote on Mon, 3 Apr 2023 09:23:24 +0100:

> On 3/31/23 20:46, Miquel Raynal wrote:
> > Bank size is derived from the chip's size, which in
> > spi_nor_init_default_params() can still be zero if the size is not
> > hardcoded in a table (eg. sfdp parsing). We need to delay the
> > initialization of this field in the "late" helpers.
> > 
> > Based on the fact that RWW compatible devices will use the bank_size to
> > derive in which bank an access is made, we absolutely want to avoid
> > divisions by 0. Right now there is no such opportunity between
> > spi_nor_init_default_params() and spi_nor_late_init_params() but it is
> > probably safer to enforce a default value (assuming 1 bank) as early as
> > possible, just in case some day code using this variable is moved
> > earlier than expected.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> > I made the choice to keep a default value, to be honest this is not
> > actually needed today, so feel free to drop the line if you don't like
> > it, both choices seem reasonable to me.
> > ---
> >  drivers/mtd/spi-nor/core.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index 9e6a0730cdb8..f436395ca489 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -2882,6 +2882,8 @@ static void spi_nor_late_init_params(struct spi_nor *nor)
> >  	 */
> >  	if (nor->flags & SNOR_F_HAS_LOCK && !nor->params->locking_ops)
> >  		spi_nor_init_default_locking_ops(nor);
> > +
> > +	nor->params->bank_size = div64_u64(nor->params->size, nor->info->n_banks);
> >  }
> >  
> >  /**
> > @@ -2948,7 +2950,7 @@ 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 = div64_u64(params->size, info->n_banks);
> > +	params->bank_size = params->size;  
> 
> I'll get rid of this line entirely as you overwrite it anyway in
> late_init and it's not used in between.

Yes, as said above, I'm fine with both approaches. Shall I resend or
will you edit when applying?

> 
> >  	params->page_size = info->page_size;
> >  
> >  	if (!(info->flags & SPI_NOR_NO_FR)) {  


Thanks,
Miquèl

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

Re: [PATCH v6 1/2] mtd: spi-nor: Delay the initialization of bank_size

[Tudor Ambarus]

On 4/3/23 16:31, Miquel Raynal wrote:
> Hi Tudor,
> 

Hi!

> tudor.ambarus@linaro.org wrote on Mon, 3 Apr 2023 09:23:24 +0100:
> 
>> On 3/31/23 20:46, Miquel Raynal wrote:
>>> Bank size is derived from the chip's size, which in
>>> spi_nor_init_default_params() can still be zero if the size is not
>>> hardcoded in a table (eg. sfdp parsing). We need to delay the
>>> initialization of this field in the "late" helpers.
>>>
>>> Based on the fact that RWW compatible devices will use the bank_size to
>>> derive in which bank an access is made, we absolutely want to avoid
>>> divisions by 0. Right now there is no such opportunity between
>>> spi_nor_init_default_params() and spi_nor_late_init_params() but it is
>>> probably safer to enforce a default value (assuming 1 bank) as early as
>>> possible, just in case some day code using this variable is moved
>>> earlier than expected.
>>>
>>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>>> ---
>>> I made the choice to keep a default value, to be honest this is not
>>> actually needed today, so feel free to drop the line if you don't like
>>> it, both choices seem reasonable to me.
>>> ---
>>>  drivers/mtd/spi-nor/core.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>> index 9e6a0730cdb8..f436395ca489 100644
>>> --- a/drivers/mtd/spi-nor/core.c
>>> +++ b/drivers/mtd/spi-nor/core.c
>>> @@ -2882,6 +2882,8 @@ static void spi_nor_late_init_params(struct spi_nor *nor)
>>>  	 */
>>>  	if (nor->flags & SNOR_F_HAS_LOCK && !nor->params->locking_ops)
>>>  		spi_nor_init_default_locking_ops(nor);
>>> +
>>> +	nor->params->bank_size = div64_u64(nor->params->size, nor->info->n_banks);
>>>  }
>>>  
>>>  /**
>>> @@ -2948,7 +2950,7 @@ 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 = div64_u64(params->size, info->n_banks);
>>> +	params->bank_size = params->size;  
>>
>> I'll get rid of this line entirely as you overwrite it anyway in
>> late_init and it's not used in between.
> 
> Yes, as said above, I'm fine with both approaches. Shall I resend or
> will you edit when applying?
> 

I'll handle it.

ta

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

Re: [PATCH v6 1/2] mtd: spi-nor: Delay the initialization of bank_size

[Tudor Ambarus]

On Fri, 31 Mar 2023 21:46:19 +0200, Miquel Raynal wrote:
> Bank size is derived from the chip's size, which in
> spi_nor_init_default_params() can still be zero if the size is not
> hardcoded in a table (eg. sfdp parsing). We need to delay the
> initialization of this field in the "late" helpers.
> 
> Based on the fact that RWW compatible devices will use the bank_size to
> derive in which bank an access is made, we absolutely want to avoid
> divisions by 0. Right now there is no such opportunity between
> spi_nor_init_default_params() and spi_nor_late_init_params() but it is
> probably safer to enforce a default value (assuming 1 bank) as early as
> possible, just in case some day code using this variable is moved
> earlier than expected.
> 
> [...]

Applied to git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git,
spi-nor/next branch. Thanks!

[1/2] mtd: spi-nor: Delay the initialization of bank_size
      https://git.kernel.org/mtd/c/79a4db50192c
[2/2] mtd: spi-nor: macronix: Add support for mx25uw51245g with RWW
      https://git.kernel.org/mtd/c/f0a499acb4b6

Cheers,
-- 
Tudor Ambarus <tudor.ambarus@linaro.org>

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